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

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

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();
>