Re: [RFC PATCH v4 01/25] virtual-bus: Implementation of Virtual Bus

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Feb 14, 2020 at 08:01:54PM -0400, Jason Gunthorpe wrote:
> On Fri, Feb 14, 2020 at 03:43:41PM -0500, Greg KH wrote:
> > On Fri, Feb 14, 2020 at 04:34:55PM -0400, Jason Gunthorpe wrote:
> > > On Fri, Feb 14, 2020 at 09:02:40AM -0800, Greg KH wrote:
> > > > > +/**
> > > > > + * virtbus_dev_register - add a virtual bus device
> > > > > + * @vdev: virtual bus device to add
> > > > > + */
> > > > > +int virtbus_dev_register(struct virtbus_device *vdev)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!vdev->release) {
> > > > > +		dev_err(&vdev->dev, "virtbus_device .release callback NULL\n");
> > > > 
> > > > "virtbus_device MUST have a .release callback that does something!\n" 
> > > > 
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	device_initialize(&vdev->dev);
> > > > > +
> > > > > +	vdev->dev.bus = &virtual_bus_type;
> > > > > +	vdev->dev.release = virtbus_dev_release;
> > > > > +	/* 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");
> > > > > +		put_device(&vdev->dev);
> > > > 
> > > > If you allocate the number before device_initialize(), no need to call
> > > > put_device().  Just a minor thing, no big deal.
> > > 
> > > If *_regster does put_device on error then it must always do
> > > put_device on any error, for instance the above return -EINVAL with
> > > no put_device leaks memory.
> > 
> > That's why I said to move the ida_simple_get() call to before
> > device_initialize() is called.  Once device_initialize() is called, you
> > HAVE to call put_device().
> 
> Yes put_device() becomes mandatory, but if the ida is moved up then
> the caller doesn't know how to handle an error:
> 
>    if (ida_simple_get() < 0)
>        return -EINVAL; // caller must do kfree
>    device_initialize();
>    if (device_register())
>        return -EINVAL // caller must do put_device

No, call put_device() before returning.

Ugh, anyway, this is all trivial stuff, the code is correct as-is,
nevermind.  If it bugs me enough, I'll send a patch that ends up
removing one more line of code than adding :)

greg k-h