Re: [PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Tuesday, 10 March 2026 19:40:25 Central European Summer Time Cristian Marussi wrote:
> Add proper error handling on failure to enumerate clocks features or
> rates.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/clock.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index c9b62edce4fd..bf956305a8fe 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -402,10 +402,16 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
>  		    SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
>  			clk->rate_change_requested_notifications = true;
>  		if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
> -			if (SUPPORTS_PARENT_CLOCK(attributes))
> -				scmi_clock_possible_parents(ph, clk_id, cinfo);
> -			if (SUPPORTS_GET_PERMISSIONS(attributes))
> -				scmi_clock_get_permissions(ph, clk_id, clk);
> +			if (SUPPORTS_PARENT_CLOCK(attributes)) {
> +				ret = scmi_clock_possible_parents(ph, clk_id, cinfo);
> +				if (ret)
> +					return ret;
> +			}
> +			if (SUPPORTS_GET_PERMISSIONS(attributes)) {
> +				ret = scmi_clock_get_permissions(ph, clk_id, clk);
> +				if (ret)
> +					return ret;
> +			}
>  			if (SUPPORTS_EXTENDED_CONFIG(attributes))
>  				clk->extended_config = true;
>  		}
> @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
>  	for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
>  		cinfo->clkds[clkid].id = clkid;
>  		ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> -		if (!ret)
> -			scmi_clock_describe_rates_get(ph, clkid, cinfo);
> +		if (ret)
> +			return ret;
> +
> +		ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
> 

I see that a quirk is being added for this, but I thought I should chime
in with my opinion for future approaches in this direction.

I don't see how this hardens anything. All this does is break platforms
that were previously working by returning early. At most, this should
be a warning (as in not WARN but pr_warn/dev_warn/...). If firmware
returns nonsense, a clock driver should imho try its best to work
around the nonsense in a safe way, because the alternative is that
a major part of the system (and thus likely the entire system) no
longer works. It's basically the same reason why we avoid BUG(): sure,
you prevented a problem, but you tore down the entire system to tell
the user about it.

Kind regards,
Nicolas Frattaroli