Re: [PATCH 4/4] firmware: arm_scmi: Validate Powercap domains before state access

On Tue, May 19, 2026 at 11:04:41AM +0100, Cristian Marussi wrote:
> On Sun, May 17, 2026 at 08:02:43PM +0100, Sudeep Holla wrote:
> > Powercap protocol v2 keeps local enable and last-cap state per
> > domain. Some public operations indexed that state before checking that
> > the supplied domain id was valid, and cap_enable_get() updated it even
> > when cap_get() failed.
> > 
> > Validate the domain before touching the per-domain state and only
> > refresh cached enable state after a successful cap_get().
> > 
> 

[...]

> >  	/*
> >  	 * Report always real platform state; platform could have ignored
> >  	 * a previous disable request. Default true on any error.
> >  	 */
> >  	ret = scmi_powercap_cap_get(ph, domain_id, &power_cap);
> > -	if (!ret)
> > +	if (!ret) {
> >  		*enable = !!power_cap;
> >  
> > -	/* Update internal state with current real platform state */
> > -	pi->states[domain_id].enabled = *enable;
> > +		/* Update internal state with current real platform state */
> > +		pi->states[domain_id].enabled = *enable;
> > +	}
> 
> Mmm, this changes the logic as stated in the above comments...now the
> problem is recalling WHY I adopted this logic :<
> 

The comment captures only ignored disabled request in which case
enabled=true is correct. But I fail to understand what would happen
if the previous call is scmi_powercap_cap_enable_set(..,false); and
assume the scmi_powercap_cap_get() from scmi_powercap_cap_enable_set()
also returned false, but failure to get the state in
scmi_powercap_cap_enable_get() will update it to true.

I wonder if it is due to the SCMI versions to maintain compatibility or the
kernel default sysfs value. scmi_powercap_cap_set() treats cached disabled
state specially. If cap_enable_get() defaulted the cache to false after
a failed read, later cap updates could be suppressed even though the platform
might actually still be enabled. Defaulting to true is the conservative
choice. I will drop this change/hunk. I will assume your reviewed by with
that dropped, please shout if not.

The generic powercap sysfs layer itself defaults enabled to true, but if a
driver get_enable() returns an error it reports false. SCMI avoids that by
returning success with true I assume.

-- 
Regards,
Sudeep