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

[Cristian Marussi]

On Mon, Mar 16, 2026 at 05:35:26PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Mon, 16 Mar 2026 at 17:14, Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
> > On Mon, Mar 16, 2026 at 04:50:17PM +0100, Geert Uytterhoeven wrote:
> > > On Thu, 12 Mar 2026 at 17:36, Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
> > > > 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:
> > > > > > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
> > > > > > > > Add proper error handling on failure to enumerate clocks features or
> > > > > > > > rates.
> > > > > > > >
> > > > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> 
> > > > > > > > --- 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.
> > >
> > > Apparently it is not just CLOCK_ATTRIBUTES, but also
> > > CLOCK_DESCRIBE_RATES.
> >
> > I was indeed suspicious that I could have opened a can of worms by
> > more strictly enfrocing these...
> >
> > > > > > > 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...
> > >
> > > Unfortunately you cannot "continue" from a quirk, without resorting
> > > to a goto, so I sent a fix: "[PATCH] firmware: arm_scmi: Support loop
> > > control in quirk code snippets"[1].
> >
> > Yes ... just realized that this afternoon when trying to draft a
> > quirk... (see other thread)
> >
> > > Then I came up with the following preliminary (have to check more
> > > firmware versions) quirk (Gmail whitespace-damaged):
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c
> > > b/drivers/firmware/arm_scmi/clock.c
> > > index f62f9492bd42afbc..6f2af6e9084836c6 100644
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
> > > clk_protocol_events = {
> > >         .num_events = ARRAY_SIZE(clk_events),
> > >  };
> > >
> > > +#define QUIRK_RCAR_X5H_NO_ATTRIBUTES                                   \
> > > +       ({                                                              \
> > > +               if (ret == -EREMOTEIO || ret == -EOPNOTSUPP)            \
> > > +                       continue;                                       \
> > > +       })
> > > +
> > > +#define QUIRK_RCAR_X5H_NO_RATES
> > >          \
> > > +       ({                                                              \
> > > +               if (ret == -EOPNOTSUPP)                                 \
> > > +                       ret = 0;                                        \
> > > +       })
> > > +
> > >  static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > >  {
> > >         int clkid, ret;
> > > @@ -1254,10 +1266,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);
> > > +               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > > QUIRK_RCAR_X5H_NO_ATTRIBUTES);
> > >                 if (ret)
> > >                         return ret;
> > >
> > >                 ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > +               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > > QUIRK_RCAR_X5H_NO_RATES);
> > >                 if (ret)
> > >                         return ret;
> > >         }
> 
> > > Does that look like what you have in mind?
> > > Thanks!
> >
> > Yes in quirk I was only addressing NOT_ATTRIBUTES and mimicing the old
> > behaviour with continue, BUT if the set of clocks not supporting attributes
> > and the set of clocks not suppporting rates is disjoint, I feel we need your
> > double quirks :P
> 
> I could have used
> 
>     SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_ATTRIBUTES);
> 
> after both scmi_clock_attributes_get() and
> scmi_clock_describe_rates_get(), but I wanted to keep the check as
> strict as possible: the former returns two error codes to ignore,
> the latter only one.
> 
> > If you are still finding out the exact FW versions that are failing maybe
> > it is better if you carry on and test the quirk-framework fix above together
> > with your quirks and we can make sure to pick all up together...
> 
> It is not urgent, as R-Car X5H SCMI support is not yet upstream.

Ah ok.

> 
> > ...OR maybe better I can also drop for now my offending patch that breaks
> > your FW from my V3 series and you can pick it up and post it later with
> > your quirks and the Quirk framework fix you propsoed so that we are sure
> > that we dont break anything while fixing all of this...
> 
> While there is indeed a chance that this hardening regresses on
> platforms that are already upstream...

Yes indeed...

> 
> > Also because we are already in V4 and I dont want to risk to post the
> > breaking fix (that was at the end broke since forever) BUT not the quirks...
> 
> s/in V4/at rc4/?

yep... -rc4 I meant..

> 
> > Let's see what @Sudeep thinks
> 
> OK.
>

Thanks,
Cristian