FAILED: patch "[PATCH] powercap: intel_rapl: Do not change CLAMPING bit if ENABLE" failed to apply to 5.15-stable tree
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;
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();
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();
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();
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?
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
linux-stable-mirror@lists.linaro.org
-
gregkh＠linuxfoundation.org -
Harshit Mogalapalli -
Sasha Levin -
srinivas pandruvada