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().
Just trying to make code smaller :)
greg k-h
