> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Friday, April 17, 2020 12:52 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@xxxxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Ertman, David M
> <david.m.ertman@xxxxxxxxx>; 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 Fri, Apr 17, 2020 at 10:10:26AM -0700, Jeff Kirsher wrote:
>
> > +/**
> > + * virtbus_release_device - Destroy a virtbus device
> > + * @_dev: device to release
> > + */
> > +static void virtbus_release_device(struct device *_dev)
> > +{
> > + struct virtbus_device *vdev = to_virtbus_dev(_dev);
> > +
> > + ida_simple_remove(&virtbus_dev_ida, vdev->id);
> > + vdev->release(vdev);
>
> This order should probably be swapped (store vdev->id on the stack)
Done
>
> > +/**
> > + * 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.
>
> > + }
> > +
> > + /* 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.
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.
Also refactoring so that the check for a .release() callback is done before the device_initialize since a put_device()
is useless in this context if a release() doesn't exist.
GregKH was pretty insistent that all error paths out of this function go through a put_device() when possible.
>
> > + }
> > +
> > +
> > + vdev->dev.bus = &virtual_bus_type;
> > + vdev->dev.release = virtbus_release_device;
>
> And this immediately after
Done.
>
> > +
> > + vdev->id = ret;
> > + dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id);
>
> Missing check of return code
Done
>
> 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.
>
> > + dev_dbg(&vdev->dev, "Registering virtbus device '%s'\n",
> > + dev_name(&vdev->dev));
> > +
> > + ret = device_add(&vdev->dev);
> > + if (ret)
> > + goto device_add_err;
>
> This looks like it does ida_simple_remove twice, once in the goto and
> once from the release function called by put_device.
Nice catch. Done
>
> > +/**
> > + * 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.
>
> > +/**
> > + * __virtbus_register_driver - register a driver for virtual bus devices
> > + * @vdrv: virtbus_driver structure
> > + * @owner: owning module/driver
> > + */
> > +int __virtbus_register_driver(struct virtbus_driver *vdrv, struct module
> *owner)
> > +{
> > + if (!vdrv->probe || !vdrv->remove || !vdrv->shutdown || !vdrv-
> >id_table)
> > + return -EINVAL;
> > +
> > + vdrv->driver.owner = owner;
> > + vdrv->driver.bus = &virtual_bus_type;
> > + vdrv->driver.probe = virtbus_probe_driver;
> > + vdrv->driver.remove = virtbus_remove_driver;
> > + vdrv->driver.shutdown = virtbus_shutdown_driver;
> > + vdrv->driver.suspend = virtbus_suspend_driver;
> > + vdrv->driver.resume = virtbus_resume_driver;
> > +
> > + return driver_register(&vdrv->driver);
> > +}
> > +EXPORT_SYMBOL_GPL(__virtbus_register_driver);
> > +
> > +/**
> > + * virtbus_unregister_driver - unregister a driver for virtual bus devices
> > + * @vdrv: virtbus_driver structure
> > + */
> > +void virtbus_unregister_driver(struct virtbus_driver *vdrv)
> > +{
> > + driver_unregister(&vdrv->driver);
> > +}
> > +EXPORT_SYMBOL_GPL(virtbus_unregister_driver);
>
> Also just inline this
Same as above on EXPORT_SYMBOL and inline.
>
> > diff --git a/include/linux/virtual_bus.h b/include/linux/virtual_bus.h
> > new file mode 100644
> > index 000000000000..4df06178e72f
> > +++ b/include/linux/virtual_bus.h
> > @@ -0,0 +1,53 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * virtual_bus.h - lightweight software bus
> > + *
> > + * Copyright (c) 2019-20 Intel Corporation
> > + *
> > + * Please see Documentation/driver-api/virtual_bus.rst for more information
> > + */
> > +
> > +#ifndef _VIRTUAL_BUS_H_
> > +#define _VIRTUAL_BUS_H_
> > +
> > +#include <linux/device.h>
> > +
> > +struct virtbus_device {
> > + struct device dev;
> > + const char *name;
> > + void (*release)(struct virtbus_device *);
> > + int id;
>
> id can't be negative
Done. Changed to match the u32 in struct device.
>
> > +int virtbus_register_device(struct virtbus_device *vdev);
> > +void virtbus_unregister_device(struct virtbus_device *vdev);
>
> I continue to think the alloc/register pattern, eg as demonstrated by
> vdpa_alloc_device() and vdpa_register_device() is easier for drivers
> to implement correctly.
>
> > +int
> > +__virtbus_register_driver(struct virtbus_driver *vdrv, struct module *owner);
> > +void virtbus_unregister_driver(struct virtbus_driver *vdrv);
> > +
> > +#define virtbus_register_driver(vdrv) \
> > + __virtbus_register_driver(vdrv, THIS_MODULE)
> > +
>
> It is reasonable to also provide a module_driver() macro.
>
> Jason
-DaveE
