diff mbox series

[V2] powercap/intel_rapl: Fix handling for large time window

Message ID 20230211031710.277459-1-rui.zhang@intel.com
State New
Headers
Series [V2] powercap/intel_rapl: Fix handling for large time window |

Commit Message

Zhang, Rui Feb. 11, 2023, 3:17 a.m. UTC 
  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>
---
Change since V1:
1. drop pr_warn message for bogus userspace input.
2. Add a comment when handling the large exponent.
---
 drivers/powercap/intel_rapl_common.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Rafael J. Wysocki Feb. 13, 2023, 4:02 p.m. UTC | #1 
On Sat, Feb 11, 2023 at 4:17 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>
> ---
> Change since V1:
> 1. drop pr_warn message for bogus userspace input.
> 2. Add a comment when handling the large exponent.
> ---
>  drivers/powercap/intel_rapl_common.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
> index 26d00b1853b4..69526d21699d 100644
> --- a/drivers/powercap/intel_rapl_common.c
> +++ b/drivers/powercap/intel_rapl_common.c
> @@ -999,7 +999,15 @@ 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);
> +
> +               /*
> +                * The target hardware field has 7 bits, return all ones if
> +                * the exponent is too large.
> +                */
> +               if (y > 0x1f)
> +                       return 0x7f;
> +
> +               f = div64_u64(4 * (value - (1ULL << y)), 1ULL << y);
>                 value = (y & 0x1f) | ((f & 0x3) << 5);
>         }
>         return value;
> --

Applied as 6.3 material with a minor adjustment of the new comment, thanks!
diff mbox series

Patch

diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
index 26d00b1853b4..69526d21699d 100644
--- a/drivers/powercap/intel_rapl_common.c
+++ b/drivers/powercap/intel_rapl_common.c
@@ -999,7 +999,15 @@  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);
+
+		/*
+		 * The target hardware field has 7 bits, return all ones if
+		 * the exponent is too large.
+		 */
+		if (y > 0x1f)
+			return 0x7f;
+
+		f = div64_u64(4 * (value - (1ULL << y)), 1ULL << y);
 		value = (y & 0x1f) | ((f & 0x3) << 5);
 	}
 	return value;