Re: [RFC PATCH v4 02/25] ice: Create and register virtual bus for RDMA

[Jason Gunthorpe <jgg@ziepe.ca>]

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@redhat.com/

(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