Re: [net-next 1/9] Implementation of Virtual Bus

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Apr 20, 2020 at 11:16:38PM +0000, Ertman, David M wrote:
> > > +/**
> > > + * virtbus_register_device - add a virtual bus device
> > > + * @vdev: virtual bus device to add
> > > + */
> > > +int virtbus_register_device(struct virtbus_device *vdev)
> > > +{
> > > +	int ret;
> > > +
> > > +	/* Do this first so that all error paths perform a put_device */
> > > +	device_initialize(&vdev->dev);
> > > +
> > > +	if (!vdev->release) {
> > > +		ret = -EINVAL;
> > > +		dev_err(&vdev->dev, "virtbus_device MUST have a .release
> > callback that does something.\n");
> > > +		goto device_pre_err;
> > 
> > This does put_device but the release() hasn't been set yet. Doesn't it
> > leak memory?
> 
> The KO registering the virtbus_device is responsible for allocating
> and freeing the memory for the virtbus_device (which should be done
> in the release() function).  If there is no release function
> defined, then the originating KO needs to handle this.  We are
> trying to not recreate the platform_bus, so the design philosophy
> behind virtual_bus is minimalist.

Oh, a precondition assertion should just be written as something like:

   if (WARN_ON(!vdev->release))
       return -EINVAL;

And done before device_initialize

But I wouldn't bother, things will just reliably crash on null pointer
exceptions if a driver mis-uses the API.

> > > +	}
> > > +
> > > +	/* All device IDs are automatically allocated */
> > > +	ret = ida_simple_get(&virtbus_dev_ida, 0, 0, GFP_KERNEL);
> > > +	if (ret < 0) {
> > > +		dev_err(&vdev->dev, "get IDA idx for virtbus device failed!\n");
> > > +		goto device_pre_err;
> > 
> > Do this before device_initialize()
>
> The memory for virtbus device is allocated by the KO registering the
> virtbus_device before it calls virtbus_register_device().  If this
> function is exiting on an error, then we have to do a put_device()
> so that the release is called (if it exists) to clean up the memory.

put_device() must call virtbus_release_device(), which does
ida_simple_remove() on vdev->id which hasn't been set yet.

Also ->release wasn't initialized at this point so its leaks memory..

> The ida_simple_get is not used until later in the function when
> setting the vdev->id.  It doesn't matter what IDA it is used, as
> long as it is unique.  So, since we cannot have the error state
> before the device_initialize, there is no reason to have the
> ida_sinple_get before the device_initialization.

I was a bit wrong on this advice because no matter what you have to do
put_device(), so you need to add some kind of flag that the vdev->id
is not valid.

It is ugly. It is nicer to arrange thing so initialization is done
after kalloc but before device_initialize. For instance look how
vdpa_alloc_device() and vdpa_register() work, very clean, very simple
goto error unwinds everywhere.

> GregKH was pretty insistent that all error paths out of this
> function go through a put_device() when possible.

After device_initialize() is called all error paths must go through
put_device.

> > Can't understand why vdev->name is being passed in with the struct,
> > why not just a function argument?
> 
> This avoids having the calling KO have to manage a separate piece of memory
> to hold the name during the call to virtbus_device_regsiter.  It is a cleaner
> memory model to just store it once in the virtbus_device itself.  This name is
> the abbreviated name without the ID appended on the end, which will be used
> for matching drivers and devices.

Your other explanation was better. This would be less confusing if it
was called match_name/device_label/driver_key or something, as it is
not the 'name'.

> > > + * virtbus_unregister_device - remove a virtual bus device
> > > + * @vdev: virtual bus device we are removing
> > > + */
> > > +void virtbus_unregister_device(struct virtbus_device *vdev)
> > > +{
> > > +	device_del(&vdev->dev);
> > > +	put_device(&vdev->dev);
> > > +}
> > > +EXPORT_SYMBOL_GPL(virtbus_unregister_device);
> > 
> > Just inline this as wrapper around device_unregister
> 
> I thought that EXPORT_SYMBOL makes inline meaningless?
> But, putting device_unregister here is a good catch.

I mean move it to the header file and inline it

Jason