Re: [PATCH v2] powercap: intel_rapl: Fix invalid setting of Power Limit 4

[Rafael J. Wysocki]

On Wed, Sep 6, 2023 at 9:08 PM Srinivas Pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
>
> System runs at minimum performance, once powercap RAPL package domain
> enabled flag is changed from 1 to 0 to 1.
>
> Setting RAPL package domain enabled flag to 0, results in setting of
> power limit 4 (PL4) MSR 0x601 to 0. This implies disabling PL4 limit.
> The PL4 limit controls the peak power. So setting 0, results in some
> undesirable performance, which depends on hardware implementation.
>
> Even worse, when the enabled flag is set to 1 again. This will set PL4
> MSR value to 0x01, which means reduce peak power to 0.125W. This will
> force system to run at the lowest possible performance on every PL4
> supported system.
>
> Setting enabled flag should only affect the "enable" bit, not other
> bits. Here it is changing power limit.
>
> This is caused by a change which assumes that there is an enable bit in
> the PL4 MSR like other power limits. Although PL4 enable/disable bit is
> present with TPMI RAPL interface, it is not present with the MSR
> interface.
>
> There is a rapl_primitive_info defined for non existent PL4 enable bit
> and then it is used with the commit 9050a9cd5e4c ("powercap: intel_rapl:
> Cleanup Power Limits support") to enable PL4. This is wrong, hence remove
> this rapl primitive for PL4. Also in the function
> rapl_detect_powerlimit(), PL_ENABLE is used to check for the presence of
> power limits. Replace PL_ENABLE with PL_LIMIT, as PL_LIMIT must be
> present. Without this change, PL4 controls will not be available in the
> sysfs once rapl primitive for PL4 is removed.
>
> Fixes: 9050a9cd5e4c ("powercap: intel_rapl: Cleanup Power Limits support")
> Suggested-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # v6.5+
> ---
> v2
> - Remove RAPL primitive for PL4 instead as suggedted by Rui
> - Replace PL_ENABLE with PL_LIMIT for domain detect
> - Update change log and header
>
>  drivers/powercap/intel_rapl_common.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
> index 5c2e6d5eea2a..40a2cc649c79 100644
> --- a/drivers/powercap/intel_rapl_common.c
> +++ b/drivers/powercap/intel_rapl_common.c
> @@ -658,8 +658,6 @@ static struct rapl_primitive_info rpi_msr[NR_RAPL_PRIMITIVES] = {
>                             RAPL_DOMAIN_REG_LIMIT, ARBITRARY_UNIT, 0),
>         [PL2_CLAMP] = PRIMITIVE_INFO_INIT(PL2_CLAMP, POWER_LIMIT2_CLAMP, 48,
>                             RAPL_DOMAIN_REG_LIMIT, ARBITRARY_UNIT, 0),
> -       [PL4_ENABLE] = PRIMITIVE_INFO_INIT(PL4_ENABLE, POWER_LIMIT4_MASK, 0,
> -                               RAPL_DOMAIN_REG_PL4, ARBITRARY_UNIT, 0),
>         [TIME_WINDOW1] = PRIMITIVE_INFO_INIT(TIME_WINDOW1, TIME_WINDOW1_MASK, 17,
>                             RAPL_DOMAIN_REG_LIMIT, TIME_UNIT, 0),
>         [TIME_WINDOW2] = PRIMITIVE_INFO_INIT(TIME_WINDOW2, TIME_WINDOW2_MASK, 49,
> @@ -1458,7 +1456,7 @@ static void rapl_detect_powerlimit(struct rapl_domain *rd)
>                         }
>                 }
>
> -               if (rapl_read_pl_data(rd, i, PL_ENABLE, false, &val64))
> +               if (rapl_read_pl_data(rd, i, PL_LIMIT, false, &val64))
>                         rd->rpl[i].name = NULL;
>         }
>  }
> --

Applied as 6.6-rc material, thanks!