Messages in this thread | | | Date | Thu, 28 Apr 2022 11:25:24 +0100 | From | Sudeep Holla <> | Subject | Re: [PATCH 02/22] firmware: arm_scmi: Make protocols init fail on basic errors |
| |
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
| |