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

[Cristian Marussi <cristian.marussi@arm.com>]

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

Hi,

> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> ---
>  drivers/firmware/arm_scmi/powercap.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
> index ab9733f4458b..eb5c35cad026 100644
> --- a/drivers/firmware/arm_scmi/powercap.c
> +++ b/drivers/firmware/arm_scmi/powercap.c
> @@ -453,10 +453,14 @@ static int scmi_powercap_cap_set(const struct scmi_protocol_handle *ph,
>  		return -EINVAL;
>  
>  	/* Just log the last set request if acting on a disabled domain */
> -	if (PROTOCOL_REV_MAJOR(ph->version) >= 0x2 &&
> -	    !pi->states[domain_id].enabled) {
> -		pi->states[domain_id].last_pcap = power_cap;
> -		return 0;
> +	if (PROTOCOL_REV_MAJOR(ph->version) >= 0x2) {
> +		if (!scmi_powercap_dom_info_get(ph, domain_id))
> +			return -EINVAL;
> +
> +		if (!pi->states[domain_id].enabled) {
> +			pi->states[domain_id].last_pcap = power_cap;
> +			return 0;
> +		}
>  	}

Yes, definitely better.

>  
>  	return __scmi_powercap_cap_set(ph, pi, domain_id,
> @@ -637,6 +641,9 @@ static int scmi_powercap_cap_enable_set(const struct scmi_protocol_handle *ph,
>  	if (PROTOCOL_REV_MAJOR(ph->version) < 0x2)
>  		return -EINVAL;
>  
> +	if (!scmi_powercap_dom_info_get(ph, domain_id))
> +		return -EINVAL;
> +
>  	if (enable == pi->states[domain_id].enabled)
>  		return 0;
>  
> @@ -678,16 +685,20 @@ static int scmi_powercap_cap_enable_get(const struct scmi_protocol_handle *ph,
>  	if (PROTOCOL_REV_MAJOR(ph->version) < 0x2)
>  		return 0;
>  
> +	if (!scmi_powercap_dom_info_get(ph, domain_id))
> +		return -EINVAL;
> +

Ok.

>  	/*
>  	 * 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 :<

Thanks,
Cristian