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

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

On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > Hi Cristian,
> > > 
> > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > Add proper error handling on failure to enumerate clocks features or
> > > > rates.
> > > >
> > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > 
> > Hi,
> > 
> > > 
> > > Thanks for your patch!
> > > 
> > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > 
> > > > @@ -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;
> > > 
> > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > do not support the SCMI CLOCK_ATTRIBUTES command.
> > > Before, these clocks were still instantiated, but were further unusable.
> > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > are available at all.
> > 
> > ...and this is exactly what I feared while doing this sort of hardening :P
> > 
> > So there are a few possible solutions (beside reverting this straight away)
> > 
> > The easy fix would be instead change the above in a
> > 
> > 	if (ret)
> > 		continue;
> > 
> > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > FW releases to fix this :D
> > 
> > Another option could be leave it as it is, since indeed it is the correct enforced
> > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > quirk targeting the affected FW releases...
> > 
> > This latter option, though, while enforcing the correct behaviour AND
> > fixing your R-Car issue, leaves open the door for a number of possible
> > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> > 
> > ...that could be solved with more quirks of course...but...worth it ?
> > 
> > Thoughts ?
> > 
> > Let's see also what @Sudeep thinks about this...
> > 
> 
> I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> the firmware baselines are derived from it. In the worst case, we can relax
> the hardening until we figure out a proper quirk-based solution.

Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
Geert with proper versioning....so I can check that there are no
surprises round the (quirked) corner...

Thanks,
Cristian