Re: [PATCH 02/22] firmware: arm_scmi: Make protocols init fail on basic errors

Sudeep Holla
2022-04-28T10:26:47Z

On Tue, Apr 26, 2022 at 05:25:28PM +0100, Cristian Marussi wrote:
> On Tue, Apr 26, 2022 at 04:35:28PM +0100, Sudeep Holla wrote:
> > On Wed, Mar 30, 2022 at 04:05:31PM +0100, Cristian Marussi wrote:
> > > Bail out of protocol initialization routine early when basic information
> > > about protocol version and attributes could not be retrieved: failing to
> > > act this way can lead to a successfully initialized SCMI protocol which
> > > is in fact not fully functional.
> > >
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > ---
> > > drivers/firmware/arm_scmi/base.c | 5 ++++-
> > > drivers/firmware/arm_scmi/clock.c | 8 ++++++--
> > > drivers/firmware/arm_scmi/perf.c | 10 +++++++---
> > > drivers/firmware/arm_scmi/power.c | 10 +++++++---
> > > drivers/firmware/arm_scmi/reset.c | 10 +++++++---
> > > drivers/firmware/arm_scmi/sensors.c | 4 +++-
> > > drivers/firmware/arm_scmi/system.c | 5 ++++-
> > > 7 files changed, 38 insertions(+), 14 deletions(-)
> > >
>
> Hi Sudeep,
>
> thanks for the review first of all...
>
> > > @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > if (!cinfo)
> > > return -ENOMEM;
> > >
> > > - scmi_clock_protocol_attributes_get(ph, cinfo);
> > > + ret = scmi_clock_protocol_attributes_get(ph, cinfo);
> > > + if (ret)
> > > + return ret;
> >
> > Does this result in removal of scmi_dev associated with devm_* calls ?
> > Otherwise we may need to free the allocated buffers ? I am not sure
> > if the dev here is individual scmi_dev or the platform scmi device.
> > I assume latter and it is unlikely to be removed/freed with the error in
> > the above path.
> >
> > Similarly in couple of other instances/protocols.
>
> So, ph->dev used in the above devm_ is indeed the arm_scmi platform device
> and I was *almost* gonna tell you 'Good catch', BUT then, rereading my own
> code (O_o), I saw/remembered that when a protocol instance is initialized on
> it first usage, there is indeed a devres_group internally managed by
> the SCMI core, so that:
>
> scmi_get_protocol_instance()
>
> @first_protocol_usage (refcount pi->users):
>
> --> scmi_get_protocol() // just in case was LKM proto
> --> scmi_alloc_init_protocol_instance()
> gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);
>
> ret = pi->proto->instance_init(&pi->ph);
> ====>>> i.e. scmi_clock_protocol_init(ph)
> if (ret)
> goto clean;
> .....
>
> clean:
> devres_release_group(handle->dev, gid);
>
>
> So basically all that happens at initialization time in scmi_clock_protocol_init,
> BUT also everything that happens implicitly inside scmi_alloc_init_protocol_instance
> during that protocol initialization (like the events registration) is undone on
> failure transparently by the SCMI core init/free management functions
> (via devres_ groups...)
>
> All of the above is because each protocol is initialized only once on
> its first usage, no matter how many SCMI driver users (and scmi_devs) are
> using it...only in case (unsupported) we have multiple SCMI instances
> (platforms) there will be one instance of protocol initialized per SCMI
> server.
>
> ... having said that, now I'll go and double check (test) this behaviour since I
> even had forgot about having implemented this kind of design :P
>

Makes sense, thanks for the detailed explanation. I had totally forgotten how
devres_group works 🙁, my bad.

--
Regards,
Sudeep