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

[Geert Uytterhoeven]

Hi Cristian,

On Fri, 24 Apr 2026 at 15:32, Cristian Marussi <cristian.marussi at arm.com> wrote:
> On Fri, Apr 24, 2026 at 02:07:59PM +0200, Nicolas Frattaroli wrote:
> > 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 at 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
>
> Certainly the naming in the subject was chosen badly (by me!)...indeed it
> should be more something like "Enforce strict protocol compliance",
> because at the end all of the broken platforms really run a slighly odd
> out of spec SCMI firmware that does NOT implement one or more of the SCMI
> mandatory command...
>
> > 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
>
> ..well yes we definitely dont want to break deployed platforms BUT also
> we dont want to legalize this kind of out of spec behaviour in future
> firmwares...hence (a number ?) of quirks an FW_BUG warns probably to
> let already broken deployed platforms survive while discouraging such
> implementation in future fw implementations...
>
> These firmware most certainly wont pass the SCMI compliance test suite [1],
> which indeed we do not mandate, but the reason these bugs happened is
> exactly because the kernel SCMI stack was buggy and left that door open...
>
> More specifically these kind of out-of-spec behaviours are not really just
> a matter being 'picky', the problem is that any resource set in any
> SCMI protocol is defined by the spec such as to be described by a
> contiguos set of IDs and the drivers are designed anyway under that
> assumption from the allocation point of view, so allowing a clock ID to
> just fail one of the mandatory commands and skip a domain would jeopardize
> all of this and, even if clearly is NOT a problem here, seems a fragile
> assumption.

How can you have all of:
  1. a contiguous list of IDs,
  2. implement all mandatory commands,
  2. restrict the use of some clocks to a subset of the agents in the system?
Use a different list of IDs for each agent?
What if a mistake was made, and a clock was exposed to an agent by
accident?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds