On Thu, Feb 20, 2020 at 06:48:04PM +0000, Ertman, David M wrote:
> > You need to go through all of this really carefully and make some kind
> > of sane lifetime model and fix all the error unwinding :(
>
> Thanks for catching this. A failure in virtbus_register_device() does
> *not* require a call virtbus_unregister_device. The failure path for the
> register function handles this. Also, we need to remain consistent with freeing
> on unwind.
Be careful it is all correct on v5 :)
> > Why doesn't the release() function of vbo trigger the free of all this
> > peer related stuff?
> >
> > Use a sane design model of splitting into functions to allocate single
> > peices of memory, goto error unwind each function, and build things up
> > properly.
> >
> > Jason
>
> I am going to add this to the documentation to record the following information.
Well, there is nothing special here. All the driver core users
basically work this way. You shouldn't need to document anything
uniquely for virtbus.
The trouble I see in this patch is mixing three different lifetime
models together (devm, release, and 'until unregister'). It is just
unnecessary and is bound to create errors.
Follow the normal, proven flow of four functions, each handling one of
the lifetimes
1) 'alloc and initialize' function
Allocates memory, and ends with device initialize().
This gets things ready to the point that put_device() and
release() will work.
Everything allocated here is valid until release.
2) Initialize stuff with a lifetime of 'until unregister'
function
This function starts with alloc'd memory from #1 and typically ends
with some register()
Every allocation is either:
- undone by release()
In this case the goto unwind is simply put_device()
[discouraged, but sometimes unavoidable]
- undone by #3, after calling unregister()
In this case the goto unwind is a mirror of the deallocs
in #3
If register() fails, it does the full goto unwind ending in
put_device().
devm is not used.
3) unregister the device function
call uregister and then do everything from the goto unwind
of #2 in reverse order.
4) Release the device function
Free all the allocations of #1 and all !NULL allocations of #2
(mostly mirrors the goto unwind of #1)
It is easy to audit, has highly symmetric goto unwind error handling,
and is fairly easy to 'do right' once you get the idea.
There are many examples of this in the kernel, look at alloc_netdev,
ib_alloc_device, tpm_chip_alloc, register_netdevice,
ib_register_device, tpm_chip_regsiter, etc.
The schema is nestable, so if the virtbus core has these four
functions (virtbus_alloc, virtbus_register, virtbus_unregister,
release), then the driver using it can have four functions too,
'driver alloc', probe, remove, release (called by core release).
Look at the recent VDPA patches for some idea how it can look:
https://lore.kernel.org/kvm/20200220061141.29390-4-jasowang@xxxxxxxxxx/
(though, IMHO, the pattern works better if the alloc also encompasses
the caller's needed struct, ie by passing in a size_t)
Notice:
- vdpa_alloc_device() returns a memory block that is freed using put_device.
At this point dev_info/etc work and the kref works. Having
dev_err/etc early on is really nice
Caller *never* does kfree()
* Notice to get dev_info() working with the right name we have to
call dev_set_name() and the error unwind for dev_set_name must be
put_device()!
- vdpa_register_device() doesn't destroy the memory on failure.
This means goto error unwind at the caller works symmetrically
- release drops the IDA because vdpa_alloc_device() created it.
This means so long as the kref is valid the name is unique.
- Unregister does not destroy the memory. This allows the caller
to continue on to free any other memory (#3 above) in their
private part of the structure
Jason
