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

[Sudeep Holla]

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@xxxxxxx>
> > > ---
> > >  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