Messages in this thread | | | From | "Zhang, Rui" <> | Subject | Re: [PATCH] powercap/intel_rapl: Fix handling for large time window | Date | Sat, 11 Feb 2023 03:20:11 +0000 |
| |
On Thu, 2023-02-09 at 18:06 +0100, Rafael J. Wysocki wrote: > On Thu, Feb 2, 2023 at 2:25 AM Zhang Rui <rui.zhang@intel.com> wrote: > > When setting the power limit time window, software updates the 'y' > > bits > > and 'f' bits in the power limit register, and the value hardware > > takes > > follows the formula below > > > > Time window = 2 ^ y * (1 + f / 4) * Time_Unit > > > > When handling large time window input from userspace, using left > > shifting breaks in two cases, > > 1. when ilog2(value) is bigger than 31, in expression "1 << y", > > left > > shifting by more than 31 bits has undefined behavior. This > > breaks > > 'y'. For example, on an Alderlake platform, "1 << 32" returns 1. > > 2. when ilog2(value) equals 31, "1 << 31" returns negative value > > because '1' is recognized as signed int. And this breaks 'f'. > > > > Given that 'y' has 5 bits and hardware can never take a value > > larger > > than 31, fix the first problem by clamp the time window to the > > maximum > > possible value that the hardware can take. > > > > Fix the second problem by using unsigned bit left shift. > > > > Note that hardware has its own maximum time window limitation, > > which > > may be lower than the time window value retrieved from the power > > limit > > register. When this happens, hardware clamps the input to its > > maximum > > time window limitation. That is why a software clamp is preferred > > to > > handle the problem on hand. > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > --- > > drivers/powercap/intel_rapl_common.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/powercap/intel_rapl_common.c > > b/drivers/powercap/intel_rapl_common.c > > index 26d00b1853b4..8b30e5259d3b 100644 > > --- a/drivers/powercap/intel_rapl_common.c > > +++ b/drivers/powercap/intel_rapl_common.c > > @@ -999,7 +999,12 @@ static u64 > > rapl_compute_time_window_core(struct rapl_package *rp, u64 value, > > > > do_div(value, rp->time_unit); > > y = ilog2(value); > > - f = div64_u64(4 * (value - (1 << y)), 1 << y); > > + if (y > 0x1f) { > > + pr_warn("%s: time window too large, > > clamped\n", rp->name); > > IIUC this happens when user space provides a value that is too large. > Why do you want to log a kernel warning in that case? > Right. I don't know any API for this purpose, so just removed the warning in patch v2.
> > + return 0x7f; > > Because the target hardware field has 7 bits, the function will > return > all ones if the exponent is too large. It would be good to put a > comment stating this here. > sure.
thanks, rui
> > + } > > + > > + f = div64_u64(4 * (value - (1ULL << y)), 1ULL << > > y); > > value = (y & 0x1f) | ((f & 0x3) << 5); > > } > > return value; > > --
| |