* FAILED: patch "[PATCH] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE" failed to apply to 5.15-stable tree
@ 2025-07-08 15:14 gregkh
2025-07-21 0:15 ` [PATCH 5.15.y] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed Sasha Levin
0 siblings, 1 reply; 6+ messages in thread
From: gregkh @ 2025-07-08 15:14 UTC (permalink / raw)
To: rui.zhang, rafael.j.wysocki, srinivas.pandruvada, stable; +Cc: stable
The patch below does not apply to the 5.15-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.
To reproduce the conflict and resubmit, you may use the following commands:
git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-5.15.y
git checkout FETCH_HEAD
git cherry-pick -x 964209202ebe1569c858337441e87ef0f9d71416
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<stable@vger.kernel.org>' --in-reply-to '2025070818-buddhism-wikipedia-516a@gregkh' --subject-prefix 'PATCH 5.15.y' HEAD^..
Possible dependencies:
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 964209202ebe1569c858337441e87ef0f9d71416 Mon Sep 17 00:00:00 2001
From: Zhang Rui <rui.zhang@intel.com>
Date: Thu, 19 Jun 2025 15:13:40 +0800
Subject: [PATCH] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE
bit cannot be changed
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>
Fixes: 2d281d8196e3 ("PowerCap: Introduce Intel RAPL power capping driver")
Cc: All applicable <stable@vger.kernel.org>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Link: https://patch.msgid.link/20250619071340.384782-1-rui.zhang@intel.com
[ rjw: Use str_enabled_disabled() instead of open-coded equivalent ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
index e3be40adc0d7..faa0b6bc5b53 100644
--- a/drivers/powercap/intel_rapl_common.c
+++ b/drivers/powercap/intel_rapl_common.c
@@ -341,12 +341,28 @@ 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,
+ str_enabled_disabled(mode));
+ goto end;
+ }
+
+ if (defaults->set_floor_freq)
defaults->set_floor_freq(rd, mode);
+
+end:
cpus_read_unlock();
return ret;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 5.15.y] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed
2025-07-08 15:14 FAILED: patch "[PATCH] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE" failed to apply to 5.15-stable tree gregkh
@ 2025-07-21 0:15 ` Sasha Levin
2025-07-23 6:41 ` Harshit Mogalapalli
0 siblings, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2025-07-21 0:15 UTC (permalink / raw)
To: stable; +Cc: Zhang Rui, Srinivas Pandruvada, Rafael J . Wysocki, Sasha Levin
From: Zhang Rui <rui.zhang@intel.com>
[ Upstream commit 964209202ebe1569c858337441e87ef0f9d71416 ]
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>
Fixes: 2d281d8196e3 ("PowerCap: Introduce Intel RAPL power capping driver")
Cc: All applicable <stable@vger.kernel.org>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Link: https://patch.msgid.link/20250619071340.384782-1-rui.zhang@intel.com
[ rjw: Use str_enabled_disabled() instead of open-coded equivalent ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[ replaced rapl_write_pl_data() and rapl_read_pl_data() with rapl_write_data_raw() and rapl_read_data_raw() ]
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/powercap/intel_rapl_common.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
index 9dfc053878fda..40d149d9dce85 100644
--- a/drivers/powercap/intel_rapl_common.c
+++ b/drivers/powercap/intel_rapl_common.c
@@ -212,12 +212,33 @@ static int find_nr_power_limit(struct rapl_domain *rd)
static int set_domain_enable(struct powercap_zone *power_zone, bool mode)
{
struct rapl_domain *rd = power_zone_to_rapl_domain(power_zone);
+ u64 val;
+ int ret;
if (rd->state & DOMAIN_STATE_BIOS_LOCKED)
return -EACCES;
cpus_read_lock();
- rapl_write_data_raw(rd, PL1_ENABLE, mode);
+ ret = rapl_write_data_raw(rd, PL1_ENABLE, mode);
+ if (ret) {
+ cpus_read_unlock();
+ return ret;
+ }
+
+ /* Check if the ENABLE bit was actually changed */
+ ret = rapl_read_data_raw(rd, PL1_ENABLE, true, &val);
+ if (ret) {
+ cpus_read_unlock();
+ return ret;
+ }
+
+ if (mode != val) {
+ pr_debug("%s cannot be %s\n", power_zone->name,
+ mode ? "enabled" : "disabled");
+ cpus_read_unlock();
+ return 0;
+ }
+
if (rapl_defaults->set_floor_freq)
rapl_defaults->set_floor_freq(rd, mode);
cpus_read_unlock();
--
2.39.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 5.15.y] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed
2025-07-21 0:15 ` [PATCH 5.15.y] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed Sasha Levin
@ 2025-07-23 6:41 ` Harshit Mogalapalli
2025-07-23 13:48 ` srinivas pandruvada
0 siblings, 1 reply; 6+ messages in thread
From: Harshit Mogalapalli @ 2025-07-23 6:41 UTC (permalink / raw)
To: Sasha Levin, stable; +Cc: Zhang Rui, Srinivas Pandruvada, Rafael J . Wysocki
Hi Sasha,
Question inline from a backport point of view:
On 21/07/25 05:45, Sasha Levin wrote:
> From: Zhang Rui <rui.zhang@intel.com>
>
> [ Upstream commit 964209202ebe1569c858337441e87ef0f9d71416 ]
>
> 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>
> Fixes: 2d281d8196e3 ("PowerCap: Introduce Intel RAPL power capping driver")
> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Link: https://patch.msgid.link/20250619071340.384782-1-rui.zhang@intel.com
> [ rjw: Use str_enabled_disabled() instead of open-coded equivalent ]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> [ replaced rapl_write_pl_data() and rapl_read_pl_data() with rapl_write_data_raw() and rapl_read_data_raw() ]
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> drivers/powercap/intel_rapl_common.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
> index 9dfc053878fda..40d149d9dce85 100644
> --- a/drivers/powercap/intel_rapl_common.c
> +++ b/drivers/powercap/intel_rapl_common.c
> @@ -212,12 +212,33 @@ static int find_nr_power_limit(struct rapl_domain *rd)
> static int set_domain_enable(struct powercap_zone *power_zone, bool mode)
> {
> struct rapl_domain *rd = power_zone_to_rapl_domain(power_zone);
> + u64 val;
> + int ret;
>
> if (rd->state & DOMAIN_STATE_BIOS_LOCKED)
> return -EACCES;
>
> cpus_read_lock();
> - rapl_write_data_raw(rd, PL1_ENABLE, mode);
> + ret = rapl_write_data_raw(rd, PL1_ENABLE, mode);
> + if (ret) {
> + cpus_read_unlock();
> + return ret;
> + }
> +
> + /* Check if the ENABLE bit was actually changed */
> + ret = rapl_read_data_raw(rd, PL1_ENABLE, true, &val);
> + if (ret) {
> + cpus_read_unlock();
> + return ret;
> + }
Shouldn't this be rapl_read_data_raw(rd, PL1_ENABLE, false, &val); ?
Thanks
Harshit
> +
> + if (mode != val) {
> + pr_debug("%s cannot be %s\n", power_zone->name,
> + mode ? "enabled" : "disabled");
> + cpus_read_unlock();
> + return 0;
> + }
> +
> if (rapl_defaults->set_floor_freq)
> rapl_defaults->set_floor_freq(rd, mode);
> cpus_read_unlock();
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 5.15.y] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed
2025-07-23 6:41 ` Harshit Mogalapalli
@ 2025-07-23 13:48 ` srinivas pandruvada
2025-07-23 15:06 ` Sasha Levin
0 siblings, 1 reply; 6+ messages in thread
From: srinivas pandruvada @ 2025-07-23 13:48 UTC (permalink / raw)
To: Harshit Mogalapalli, Sasha Levin, stable; +Cc: Zhang Rui, Rafael J . Wysocki
On Wed, 2025-07-23 at 12:11 +0530, Harshit Mogalapalli wrote:
> Hi Sasha,
>
>
> Question inline from a backport point of view:
> On 21/07/25 05:45, Sasha Levin wrote:
> > From: Zhang Rui <rui.zhang@intel.com>
> >
> > [ Upstream commit 964209202ebe1569c858337441e87ef0f9d71416 ]
> >
> > 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>
> > Fixes: 2d281d8196e3 ("PowerCap: Introduce Intel RAPL power capping
> > driver")
> > Cc: All applicable <stable@vger.kernel.org>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > Link:
> > https://patch.msgid.link/20250619071340.384782-1-rui.zhang@intel.com
> > [ rjw: Use str_enabled_disabled() instead of open-coded equivalent
> > ]
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > [ replaced rapl_write_pl_data() and rapl_read_pl_data() with
> > rapl_write_data_raw() and rapl_read_data_raw() ]
> > Signed-off-by: Sasha Levin <sashal@kernel.org>
> > ---
> > drivers/powercap/intel_rapl_common.c | 23 ++++++++++++++++++++++-
> > 1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/powercap/intel_rapl_common.c
> > b/drivers/powercap/intel_rapl_common.c
> > index 9dfc053878fda..40d149d9dce85 100644
> > --- a/drivers/powercap/intel_rapl_common.c
> > +++ b/drivers/powercap/intel_rapl_common.c
> > @@ -212,12 +212,33 @@ static int find_nr_power_limit(struct
> > rapl_domain *rd)
> > static int set_domain_enable(struct powercap_zone *power_zone,
> > bool mode)
> > {
> > struct rapl_domain *rd =
> > power_zone_to_rapl_domain(power_zone);
> > + u64 val;
> > + int ret;
> >
> > if (rd->state & DOMAIN_STATE_BIOS_LOCKED)
> > return -EACCES;
> >
> > cpus_read_lock();
> > - rapl_write_data_raw(rd, PL1_ENABLE, mode);
> > + ret = rapl_write_data_raw(rd, PL1_ENABLE, mode);
> > + if (ret) {
> > + cpus_read_unlock();
> > + return ret;
> > + }
> > +
> > + /* Check if the ENABLE bit was actually changed */
> > + ret = rapl_read_data_raw(rd, PL1_ENABLE, true, &val);
> > + if (ret) {
> > + cpus_read_unlock();
> > + return ret;
> > + }
>
> Shouldn't this be rapl_read_data_raw(rd, PL1_ENABLE, false, &val); ?
>
>
Correct. This will result in additional call to rapl_unit_xlate(), but
since for primitive PL1_ENABLE, the unit is ARBITRARY_UNIT, this will
not translate and return the same value.
Thanks,
Srinivas
> Thanks
> Harshit
> > +
> > + if (mode != val) {
> > + pr_debug("%s cannot be %s\n", power_zone->name,
> > + mode ? "enabled" : "disabled");
> > + cpus_read_unlock();
> > + return 0;
> > + }
> > +
> > if (rapl_defaults->set_floor_freq)
> > rapl_defaults->set_floor_freq(rd, mode);
> > cpus_read_unlock();
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 5.15.y] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed
2025-07-23 13:48 ` srinivas pandruvada
@ 2025-07-23 15:06 ` Sasha Levin
2025-07-23 17:09 ` srinivas pandruvada
0 siblings, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2025-07-23 15:06 UTC (permalink / raw)
To: srinivas pandruvada
Cc: Harshit Mogalapalli, stable, Zhang Rui, Rafael J . Wysocki
On Wed, Jul 23, 2025 at 06:48:08AM -0700, srinivas pandruvada wrote:
>On Wed, 2025-07-23 at 12:11 +0530, Harshit Mogalapalli wrote:
>> > + /* Check if the ENABLE bit was actually changed */
>> > + ret = rapl_read_data_raw(rd, PL1_ENABLE, true, &val);
>> > + if (ret) {
>> > + cpus_read_unlock();
>> > + return ret;
>> > + }
>>
>> Shouldn't this be rapl_read_data_raw(rd, PL1_ENABLE, false, &val); ?
>>
>>
>Correct. This will result in additional call to rapl_unit_xlate(), but
>since for primitive PL1_ENABLE, the unit is ARBITRARY_UNIT, this will
>not translate and return the same value.
I was thinking we need to call rapl_unit_xlate() so we won't just get
whatever the raw value is, but yes - since it's ARBITRARY_UNIT then it
won't really do much.
I guess in this case it doesn't really matter if we pass true or false
here?
--
Thanks,
Sasha
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 5.15.y] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed
2025-07-23 15:06 ` Sasha Levin
@ 2025-07-23 17:09 ` srinivas pandruvada
0 siblings, 0 replies; 6+ messages in thread
From: srinivas pandruvada @ 2025-07-23 17:09 UTC (permalink / raw)
To: Sasha Levin; +Cc: Harshit Mogalapalli, stable, Zhang Rui, Rafael J . Wysocki
On Wed, 2025-07-23 at 11:06 -0400, Sasha Levin wrote:
> On Wed, Jul 23, 2025 at 06:48:08AM -0700, srinivas pandruvada wrote:
> > On Wed, 2025-07-23 at 12:11 +0530, Harshit Mogalapalli wrote:
> > > > + /* Check if the ENABLE bit was actually changed */
> > > > + ret = rapl_read_data_raw(rd, PL1_ENABLE, true, &val);
> > > > + if (ret) {
> > > > + cpus_read_unlock();
> > > > + return ret;
> > > > + }
> > >
> > > Shouldn't this be rapl_read_data_raw(rd, PL1_ENABLE, false,
> > > &val); ?
> > >
> > >
> > Correct. This will result in additional call to rapl_unit_xlate(),
> > but
> > since for primitive PL1_ENABLE, the unit is ARBITRARY_UNIT, this
> > will
> > not translate and return the same value.
>
> I was thinking we need to call rapl_unit_xlate() so we won't just get
> whatever the raw value is, but yes - since it's ARBITRARY_UNIT then
> it
> won't really do much.
>
> I guess in this case it doesn't really matter if we pass true or
> false
> here?
correct, you just save one additional call.
Thanks,
Srinivas
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-23 17:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-08 15:14 FAILED: patch "[PATCH] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE" failed to apply to 5.15-stable tree gregkh
2025-07-21 0:15 ` [PATCH 5.15.y] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE bit cannot be changed Sasha Levin
2025-07-23 6:41 ` Harshit Mogalapalli
2025-07-23 13:48 ` srinivas pandruvada
2025-07-23 15:06 ` Sasha Levin
2025-07-23 17:09 ` srinivas pandruvada
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.