Messages in this thread | | | Date | Tue, 26 Apr 2022 17:25:28 +0100 | From | Cristian Marussi <> | Subject | Re: [PATCH 02/22] firmware: arm_scmi: Make protocols init fail on basic errors |
| |
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
Thanks, Cristian
| |