> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Monday, April 20, 2020 5:45 PM
> To: Ertman, David M <david.m.ertman@xxxxxxxxx>
> Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@xxxxxxxxx>; davem@xxxxxxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> rdma@xxxxxxxxxxxxxxx; nhorman@xxxxxxxxxx; sassmann@xxxxxxxxxx;
> parav@xxxxxxxxxxxx; galpress@xxxxxxxxxx;
> selvin.xavier@xxxxxxxxxxxx; sriharsha.basavapatna@xxxxxxxxxxxx;
> benve@xxxxxxxxx; bharat@xxxxxxxxxxx; xavier.huwei@xxxxxxxxxx;
> yishaih@xxxxxxxxxxxx; leonro@xxxxxxxxxxxx; mkalderon@xxxxxxxxxxx;
> aditr@xxxxxxxxxx; ranjani.sridharan@xxxxxxxxxxxxxxx; pierre-
> louis.bossart@xxxxxxxxxxxxxxx; Patil, Kiran <kiran.patil@xxxxxxxxx>; Bowers,
> AndrewX <andrewx.bowers@xxxxxxxxx>
> 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
