[PATCH] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed

[PATCH] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed

[Zhang Rui]

PL1 cannot be disabled on some platforms. The ENABLE bit is still set
after software clears it. This behavior leads to a scenario where, upon
user request to disable the Power Limit through the powercap sysfs, the
ENABLE bit remains set while the CLAMPING bit is inadvertently cleared.

According to the Intel Software Developer's Manual, the CLAMPING bit,
"When set, allows the processor to go below the OS requested P states in
order to maintain the power below specified Platform Power Limit value."

Thus this means the system may operate at higher power levels than
intended on such platforms.

Enhance the code to check ENABLE bit after writing to it, and stop
further processing if ENABLE bit cannot be changed.

Reported-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/powercap/intel_rapl_common.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
index e3be40adc0d7..602f540cbe15 100644
--- a/drivers/powercap/intel_rapl_common.c
+++ b/drivers/powercap/intel_rapl_common.c
@@ -341,12 +341,27 @@ static int set_domain_enable(struct powercap_zone *power_zone, bool mode)
 {
 	struct rapl_domain *rd = power_zone_to_rapl_domain(power_zone);
 	struct rapl_defaults *defaults = get_defaults(rd->rp);
+	u64 val;
 	int ret;
 
 	cpus_read_lock();
 	ret = rapl_write_pl_data(rd, POWER_LIMIT1, PL_ENABLE, mode);
-	if (!ret && defaults->set_floor_freq)
+	if (ret)
+		goto end;
+
+	ret = rapl_read_pl_data(rd, POWER_LIMIT1, PL_ENABLE, false, &val);
+	if (ret)
+		goto end;
+
+	if (mode != val) {
+		pr_debug("%s cannot be %s\n", power_zone->name, mode ? "enabled" : "disabled");
+		goto end;
+	}
+
+	if (defaults->set_floor_freq)
 		defaults->set_floor_freq(rd, mode);
+
+end:
 	cpus_read_unlock();
 
 	return ret;
-- 
2.43.0

Re: [PATCH] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed

[Rafael J. Wysocki]

On Wed, Jun 18, 2025 at 5:03 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> PL1 cannot be disabled on some platforms. The ENABLE bit is still set
> after software clears it. This behavior leads to a scenario where, upon
> user request to disable the Power Limit through the powercap sysfs, the
> ENABLE bit remains set while the CLAMPING bit is inadvertently cleared.
>
> According to the Intel Software Developer's Manual, the CLAMPING bit,
> "When set, allows the processor to go below the OS requested P states in
> order to maintain the power below specified Platform Power Limit value."
>
> Thus this means the system may operate at higher power levels than
> intended on such platforms.
>
> Enhance the code to check ENABLE bit after writing to it, and stop
> further processing if ENABLE bit cannot be changed.
>
> Reported-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

If this is a fix, I would appreciate a Fixes: tag.

> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/powercap/intel_rapl_common.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
> index e3be40adc0d7..602f540cbe15 100644
> --- a/drivers/powercap/intel_rapl_common.c
> +++ b/drivers/powercap/intel_rapl_common.c
> @@ -341,12 +341,27 @@ static int set_domain_enable(struct powercap_zone *power_zone, bool mode)
>  {
>         struct rapl_domain *rd = power_zone_to_rapl_domain(power_zone);
>         struct rapl_defaults *defaults = get_defaults(rd->rp);
> +       u64 val;
>         int ret;
>
>         cpus_read_lock();
>         ret = rapl_write_pl_data(rd, POWER_LIMIT1, PL_ENABLE, mode);
> -       if (!ret && defaults->set_floor_freq)
> +       if (ret)
> +               goto end;
> +
> +       ret = rapl_read_pl_data(rd, POWER_LIMIT1, PL_ENABLE, false, &val);
> +       if (ret)
> +               goto end;
> +
> +       if (mode != val) {
> +               pr_debug("%s cannot be %s\n", power_zone->name, mode ? "enabled" : "disabled");
> +               goto end;
> +       }
> +
> +       if (defaults->set_floor_freq)
>                 defaults->set_floor_freq(rd, mode);
> +
> +end:
>         cpus_read_unlock();
>
>         return ret;
> --
> 2.43.0
>
>

Re: [PATCH] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed

[Zhang, Rui]

On Wed, 2025-06-18 at 19:04 +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 18, 2025 at 5:03 AM Zhang Rui <rui.zhang@intel.com> wrote:
> > 
> > PL1 cannot be disabled on some platforms. The ENABLE bit is still set
> > after software clears it. This behavior leads to a scenario where,
> > upon
> > user request to disable the Power Limit through the powercap sysfs,
> > the
> > ENABLE bit remains set while the CLAMPING bit is inadvertently
> > cleared.
> > 
> > According to the Intel Software Developer's Manual, the CLAMPING bit,
> > "When set, allows the processor to go below the OS requested P states
> > in
> > order to maintain the power below specified Platform Power Limit
> > value."
> > 
> > Thus this means the system may operate at higher power levels than
> > intended on such platforms.
> > 
> > Enhance the code to check ENABLE bit after writing to it, and stop
> > further processing if ENABLE bit cannot be changed.
> > 
> > Reported-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>
> 
> If this is a fix, I would appreciate a Fixes: tag.

The problem is always there since Day One, in this commit 2d281d8196e3
("PowerCap: Introduce Intel RAPL power capping driver").

I can use this commit in the Fixes tag. And I think this should be stable
candidate. Will send V2 soon.

thanks,
rui
> 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/powercap/intel_rapl_common.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/powercap/intel_rapl_common.c
> > b/drivers/powercap/intel_rapl_common.c
> > index e3be40adc0d7..602f540cbe15 100644
> > --- a/drivers/powercap/intel_rapl_common.c
> > +++ b/drivers/powercap/intel_rapl_common.c
> > @@ -341,12 +341,27 @@ static int set_domain_enable(struct
> > powercap_zone *power_zone, bool mode)
> >  {
> >         struct rapl_domain *rd =
> > power_zone_to_rapl_domain(power_zone);
> >         struct rapl_defaults *defaults = get_defaults(rd->rp);
> > +       u64 val;
> >         int ret;
> > 
> >         cpus_read_lock();
> >         ret = rapl_write_pl_data(rd, POWER_LIMIT1, PL_ENABLE, mode);
> > -       if (!ret && defaults->set_floor_freq)
> > +       if (ret)
> > +               goto end;
> > +
> > +       ret = rapl_read_pl_data(rd, POWER_LIMIT1, PL_ENABLE, false,
> > &val);
> > +       if (ret)
> > +               goto end;
> > +
> > +       if (mode != val) {
> > +               pr_debug("%s cannot be %s\n", power_zone->name, mode
> > ? "enabled" : "disabled");
> > +               goto end;
> > +       }
> > +
> > +       if (defaults->set_floor_freq)
> >                 defaults->set_floor_freq(rd, mode);
> > +
> > +end:
> >         cpus_read_unlock();
> > 
> >         return ret;
> > --
> > 2.43.0
> > 
> >