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

[Cristian Marussi]

On Thu, Apr 28, 2022 at 11:25:24AM +0100, Sudeep Holla wrote:
> 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.
> 

Well I had even forgot to have used it in the SCMI core :P

Thanks,
Cristian