On Thu, Oct 08, 2020 at 08:29:00AM -0500, Pierre-Louis Bossart wrote:
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.
err = ancillary_device_initialize(); if (err) return ret;
where is the put_device() here? if the release function does any sort of kfree, then you'd need to do it manually in this case.
Since device_initialize() failed, put_device() cannot be done here. So yes, pseudo code should have shown, if (err) { kfree(adev); return err; }
This doesn't work if the adev is part of a larger structure allocated by the parent, which is pretty much the intent to extent the basic bus and pass additional information which can be accessed with container_of().
Please take a look how ib_alloc_device() is implemented. It does all that you wrote above in very similar manner to netdev_alloc.
In a nutshell, ib_alloc_device receives needed size from the user and requires from the users to extend their structures below "general" one.
Thanks