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
