> -----Original Message----- > From: Pierre-Louis Bossart <[email protected]> > Sent: Wednesday, October 7, 2020 1:59 PM > To: Ertman, David M <[email protected]>; Parav Pandit > <[email protected]>; Leon Romanovsky <[email protected]> > Cc: [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; Jason Gunthorpe > <[email protected]>; [email protected]; [email protected]; Williams, > Dan J <[email protected]>; Saleem, Shiraz > <[email protected]>; [email protected]; Patil, Kiran > <[email protected]> > Subject: Re: [PATCH v2 1/6] Add ancillary bus support > > > > >> Below is most simple, intuitive and matching with core APIs for name and > >> design pattern wise. > >> init() > >> { > >> err = ancillary_device_initialize(); > >> if (err) > >> return ret; > >> > >> err = ancillary_device_add(); > >> if (ret) > >> goto err_unwind; > >> > >> err = some_foo(); > >> if (err) > >> goto err_foo; > >> return 0; > >> > >> err_foo: > >> ancillary_device_del(adev); > >> err_unwind: > >> ancillary_device_put(adev->dev); > >> return err; > >> } > >> > >> cleanup() > >> { > >> ancillary_device_de(adev); > >> ancillary_device_put(adev); > >> /* It is common to have a one wrapper for this as > >> ancillary_device_unregister(). > >> * This will match with core device_unregister() that has precise > >> documentation. > >> * but given fact that init() code need proper error unwinding, like > >> above, > >> * it make sense to have two APIs, and no need to export another > >> symbol for unregister(). > >> * This pattern is very easy to audit and code. > >> */ > >> } > > > > I like this flow +1 > > > > But ... since the init() function is performing both device_init and > > device_add - it should probably be called ancillary_device_register, > > and we are back to a single exported API for both register and > > unregister. > > Kind reminder that we introduced the two functions to allow the caller > to know if it needed to free memory when initialize() fails, and it > didn't need to free memory when add() failed since put_device() takes > care of it. If you have a single init() function it's impossible to know > which behavior to select on error. > > I also have a case with SoundWire where it's nice to first initialize, > then set some data and then add. >
The flow as outlined by Parav above does an initialize as the first step, so every error path out of the function has to do a put_device(), so you would never need to manually free the memory in the setup function. It would be freed in the release call. -DaveE > > > > At that point, do we need wrappers on the primitives init, add, del, > > and put? > > > > -DaveE > >
