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

["Ertman, David M" <david.m.ertman@intel.com>]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Monday, April 20, 2020 5:45 PM
> To: Ertman, David M <david.m.ertman@intel.com>
> Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; davem@davemloft.net;
> gregkh@linuxfoundation.org; netdev@vger.kernel.org; linux-
> rdma@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com;
> parav@mellanox.com; galpress@amazon.com;
> selvin.xavier@broadcom.com; sriharsha.basavapatna@broadcom.com;
> benve@cisco.com; bharat@chelsio.com; xavier.huwei@huawei.com;
> yishaih@mellanox.com; leonro@mellanox.com; mkalderon@marvell.com;
> aditr@vmware.com; ranjani.sridharan@linux.intel.com; pierre-
> louis.bossart@linux.intel.com; Patil, Kiran <kiran.patil@intel.com>; Bowers,
> AndrewX <andrewx.bowers@intel.com>
> Subject: Re: [net-next 1/9] Implementation of Virtual Bus
> 
> 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.
> 

Done.

> > > > +	}
> > > > +
> > > > +	/* 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..

->release assignment moved to before ida_simple_get evaluation,
and added a define for VIRTBUS_INVALID_ID and a check in release
to not do ida_simple_remove for an invalid ID.

> 
> > 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.
> 

Did just that 😊

> 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'.
> 

changing the vdev->name to vdev->match_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

Done.

> 
> Jason

-DaveE