| Message ID | 20200417171034.1533253-2-jeffrey.t.kirsher@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | 100GbE Intel Wired LAN Driver Updates 2020-04-17 | expand |
On 4/17/20 12:10 PM, Jeff Kirsher wrote: > From: Dave Ertman <david.m.ertman@intel.com> > > This is the initial implementation of the Virtual Bus, > virtbus_device and virtbus_driver. The virtual bus is > a software based bus intended to support registering > virtbus_devices and virtbus_drivers and provide matching > between them and probing of the registered drivers. > > The bus will support probe/remove shutdown and > suspend/resume callbacks. > > Kconfig and Makefile alterations are included > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > Signed-off-by: Kiran Patil <kiran.patil@intel.com> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> FWIW we are planning to use this Virtual Bus to support multiple clients for the Sound Open Firmware driver, instead of using platform devices as suggested by GregKH. Minor nit-picks below, please feel free to add my tag for patch 1/9: Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > +config VIRTUAL_BUS > + tristate "Software based Virtual Bus" > + help > + Provides a software bus for virtbus_devices to be added to it > + and virtbus_drivers to be registered on it. It matches driver > + and device based on id and calls the driver's pobe routine. typo: probe [...] > diff --git a/drivers/bus/virtual_bus.c b/drivers/bus/virtual_bus.c > new file mode 100644 > index 000000000000..fb14cca40eea > --- /dev/null > +++ b/drivers/bus/virtual_bus.c > @@ -0,0 +1,270 @@ > +// SPDX-License-Identifier: GPL-2.0 did you mean GPL-2.0-only, as done for virtual-bus.h? > +/* > + * virtual_bus.c - lightweight software based bus for virtual devices > + * > + * Copyright (c) 2019-20 Intel Corporation I think the recommendation is to use 2019-2020 explicitly. > + * > + * Please see Documentation/driver-api/virtual_bus.rst for > + * more information > + */ > + > +#include <linux/string.h> > +#include <linux/virtual_bus.h> > +#include <linux/of_irq.h> is of_irq.h required? > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/pm_runtime.h> > +#include <linux/pm_domain.h> > +#include <linux/acpi.h> is there any ACPI dependency? > +#include <linux/device.h> alphabetical order for the headers maybe? [...] > +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; > + } > + > + /* 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; > + } > + > + extra line > + vdev->dev.bus = &virtual_bus_type; > + vdev->dev.release = virtbus_release_device; > + > + vdev->id = ret; > + dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id); > + > + dev_dbg(&vdev->dev, "Registering virtbus device '%s'\n", > + dev_name(&vdev->dev)); > + > + ret = device_add(&vdev->dev); > + if (ret) > + goto device_add_err; > + > + return 0; > + > +device_add_err: > + ida_simple_remove(&virtbus_dev_ida, vdev->id); > + > +device_pre_err: > + dev_err(&vdev->dev, "Add device to virtbus failed!: %d\n", ret); > + put_device(&vdev->dev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(virtbus_register_device); > + > +/** > + * 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); looks like device_unregister(&vdev->dev) ? > +} > +EXPORT_SYMBOL_GPL(virtbus_unregister_device); > + > +static int virtbus_probe_driver(struct device *_dev) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > + int ret; > + > + ret = dev_pm_domain_attach(_dev, true); > + if (ret) { > + dev_warn(_dev, "Failed to attatch to PM Domain : %d\n", ret); typo: attach [...] > diff --git a/include/linux/virtual_bus.h b/include/linux/virtual_bus.h > new file mode 100644 > index 000000000000..4df06178e72f > --- /dev/null > +++ 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 2019-2020?
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) > +/** > + * 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? > + } > + > + /* 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() > + } > + > + > + vdev->dev.bus = &virtual_bus_type; > + vdev->dev.release = virtbus_release_device; And this immediately after > + > + vdev->id = ret; > + dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id); Missing check of return code Can't understand why vdev->name is being passed in with the struct, why not just a function argument? > + 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. > +/** > + * 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 > +/** > + * __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 > 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 > +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
On Fri, Apr 17, 2020 at 10:10:26AM -0700, Jeff Kirsher wrote: > @@ -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; struct device already has a name, why do you need another one? > + void (*release)(struct virtbus_device *); A bus should have the release function, not the actual device itself. A device should not need function pointers. > + int id; Shouldn't that be a specific type, like u64 or something? thanks, greg k-h
> -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Saturday, April 18, 2020 5:51 AM > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com> > Cc: davem@davemloft.net; Ertman, David M <david.m.ertman@intel.com>; > netdev@vger.kernel.org; linux-rdma@vger.kernel.org; nhorman@redhat.com; > sassmann@redhat.com; jgg@ziepe.ca; parav@mellanox.com; > galpress@amazon.com; selvin.xavier@broadcom.com; > sriharsha.basavapatna@broadcom.com; benve@cisco.com; > bharat@chelsio.com; xavier.huwei@huawei.com; yishaih@mellanox.com; > leonro@mellanox.com; mkalderon@marvell.com; aditr@vmware.com; > ranjani.sridharan@linux.intel.com; pierre-louis.bossart@linux.intel.com; Patil, > Kiran <kiran.patil@intel.com>; Bowers, AndrewX <andrewx.bowers@intel.com> > Subject: Re: [net-next 1/9] Implementation of Virtual Bus > > On Fri, Apr 17, 2020 at 10:10:26AM -0700, Jeff Kirsher wrote: > > @@ -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; > > struct device already has a name, why do you need another one? The name in dev is the base name appended with the id to make sure each device has unique name. The name in vdev is the abbreviated one (without the id) which will be used in the matching function, so that a driver can claim to support <name> and will be matched with all <name>.<id> devices for all id's. This is similar logic to platform_device's name field. > > > + void (*release)(struct virtbus_device *); > > A bus should have the release function, not the actual device itself. A > device should not need function pointers. > The bus does have a release function, but it is a wrapper to call the release defined by the device. This is where the KO registering the virtbus_device is expected to clean up the resources allocated for this device (e.g. free memory, etc). Having the virtual_bus_release call a release callback in the virtual_device allows for extra cleanup from the originating KO if necessary. The memory model of virtual bus is for the originating KO to manage the lifespan of the memory for the virtual_device. The virtual_bus expects the KO defining the virtbus_device have the memory allocated before registering a virtbus_device and to clean up that memory when the release is called. The platform_device also has function pointers in it, by including a MFD object, but the platform_bus is managing the memory for the platform_bus_object that contains the platform_device which it why it using a generic kref_put to free memory. > > + int id; > > Shouldn't that be a specific type, like u64 or something? Changed to be a u32 to match struct device.id. > > thanks, > > greg k-h -DaveE
> -----Original Message----- > From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Friday, April 17, 2020 12:52 PM > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com> > Cc: davem@davemloft.net; gregkh@linuxfoundation.org; Ertman, David M > <david.m.ertman@intel.com>; netdev@vger.kernel.org; linux- > rdma@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com; > parav@mellanox.com; galpress@amazon.com; selvin.xavier@broadcom.com; > sriharsha.basavapatna@broadcom.com; benve@cisco.com; > bharat@chelsio.com; xavier.huwei@huawei.com; yishaih@mellanox.com; > leonro@mellanox.com; mkalderon@marvell.com; aditr@vmware.com; > ranjani.sridharan@linux.intel.com; pierre-louis.bossart@linux.intel.com; Patil, > Kiran <kiran.patil@intel.com>; Bowers, AndrewX <andrewx.bowers@intel.com> > 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
On Fri, 2020-04-17 at 10:10 -0700, Jeff Kirsher wrote: > From: Dave Ertman <david.m.ertman@intel.com> > > This is the initial implementation of the Virtual Bus, > virtbus_device and virtbus_driver. The virtual bus is > a software based bus intended to support registering > virtbus_devices and virtbus_drivers and provide matching > between them and probing of the registered drivers. > > The bus will support probe/remove shutdown and > suspend/resume callbacks. > > Kconfig and Makefile alterations are included > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > Signed-off-by: Kiran Patil <kiran.patil@intel.com> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > Documentation/driver-api/virtual_bus.rst | 62 ++++++ > drivers/bus/Kconfig | 10 + > drivers/bus/Makefile | 2 + > drivers/bus/virtual_bus.c | 270 > +++++++++++++++++++++++ > include/linux/mod_devicetable.h | 8 + > include/linux/virtual_bus.h | 53 +++++ > scripts/mod/devicetable-offsets.c | 3 + > scripts/mod/file2alias.c | 7 + > 8 files changed, 415 insertions(+) > create mode 100644 Documentation/driver-api/virtual_bus.rst > create mode 100644 drivers/bus/virtual_bus.c > create mode 100644 include/linux/virtual_bus.h > > diff --git a/Documentation/driver-api/virtual_bus.rst > b/Documentation/driver-api/virtual_bus.rst > new file mode 100644 > index 000000000000..a79db0e9231e > --- /dev/null > +++ b/Documentation/driver-api/virtual_bus.rst > @@ -0,0 +1,62 @@ > +=============================== > +Virtual Bus Devices and Drivers > +=============================== > + > +See <linux/virtual_bus.h> for the models for virtbus_device and > virtbus_driver. > +This bus is meant to be a lightweight software based bus to attach > generic > +devices and drivers to so that a chunk of data can be passed between > them. > + > +One use case example is an RDMA driver needing to connect with > several > +different types of PCI LAN devices to be able to request resources > from > +them (queue sets). Each LAN driver that supports RDMA will register > a > +virtbus_device on the virtual bus for each physical function. The > RDMA > +driver will register as a virtbus_driver on the virtual bus to be > +matched up with multiple virtbus_devices and receive a pointer to a > +struct containing the callbacks that the PCI LAN drivers support for > +registering with them. > + > +Sections in this document: > + Virtbus devices > + Virtbus drivers > + Device Enumeration > + Device naming and driver binding > + Virtual Bus API entry points > + > +Virtbus devices > +~~~~~~~~~~~~~~~ > +Virtbus_devices support the minimal device functionality. Devices > will > +accept a name, and then, when added to the virtual bus, an > automatically > +generated index is concatenated onto it for the virtbus_device- > >name. > + > +Virtbus drivers > +~~~~~~~~~~~~~~~ > +Virtbus drivers register with the virtual bus to be matched with > virtbus > +devices. They expect to be registered with a probe and remove > callback, > +and also support shutdown, suspend, and resume callbacks. They > otherwise > +follow the standard driver behavior of having discovery and > enumeration > +handled in the bus infrastructure. > + > +Virtbus drivers register themselves with the API entry point > +virtbus_register_driver and unregister with > virtbus_unregister_driver. > + > +Device Enumeration > +~~~~~~~~~~~~~~~~~~ > +Enumeration is handled automatically by the bus infrastructure via > the > +ida_simple methods. > + > +Device naming and driver binding > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > +The virtbus_device.dev.name is the canonical name for the device. It > is > +built from two other parts: > + > + - virtbus_device.name (also used for matching). > + - virtbus_device.id (generated automatically from ida_simple > calls) > + > +Virtbus device IDs are always in "<name>.<instance>" > format. Instances are > +automatically selected through an ida_simple_get so are positive > integers. > +Name is taken from the device name field. > + > +Driver IDs are simple <name>. > + > +Need to extract the name from the Virtual Device compare to name of > the > +driver. > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > index 6d4e4497b59b..8b6c709730ba 100644 > --- a/drivers/bus/Kconfig > +++ b/drivers/bus/Kconfig > @@ -203,4 +203,14 @@ config DA8XX_MSTPRI > source "drivers/bus/fsl-mc/Kconfig" > source "drivers/bus/mhi/Kconfig" > > +config VIRTUAL_BUS > + tristate "Software based Virtual Bus" > + help > + Provides a software bus for virtbus_devices to be added to > it > + and virtbus_drivers to be registered on it. It matches > driver > + and device based on id and calls the driver's pobe routine. > + One example is the irdma driver needing to connect with > various > + PCI LAN drivers to request resources (queues) to be able to > perform > + its function. > + > endmenu > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > index 05f32cd694a4..d30828a4768c 100644 > --- a/drivers/bus/Makefile > +++ b/drivers/bus/Makefile > @@ -37,3 +37,5 @@ obj-$(CONFIG_DA8XX_MSTPRI) += da8xx-mstpri.o > > # MHI > obj-$(CONFIG_MHI_BUS) += mhi/ > + > +obj-$(CONFIG_VIRTUAL_BUS) += virtual_bus.o > diff --git a/drivers/bus/virtual_bus.c b/drivers/bus/virtual_bus.c > new file mode 100644 > index 000000000000..fb14cca40eea > --- /dev/null > +++ b/drivers/bus/virtual_bus.c > @@ -0,0 +1,270 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * virtual_bus.c - lightweight software based bus for virtual > devices > + * > + * Copyright (c) 2019-20 Intel Corporation > + * > + * Please see Documentation/driver-api/virtual_bus.rst for > + * more information > + */ > + > +#include <linux/string.h> > +#include <linux/virtual_bus.h> > +#include <linux/of_irq.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/pm_runtime.h> > +#include <linux/pm_domain.h> > +#include <linux/acpi.h> > +#include <linux/device.h> > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Virtual Bus"); > +MODULE_AUTHOR("David Ertman <david.m.ertman@intel.com>"); > +MODULE_AUTHOR("Kiran Patil <kiran.patil@intel.com>"); > + > +static DEFINE_IDA(virtbus_dev_ida); > + > +static const > +struct virtbus_dev_id *virtbus_match_id(const struct virtbus_dev_id > *id, > + struct virtbus_device *vdev) > +{ > + while (id->name[0]) { > + if (!strcmp(vdev->name, id->name)) > + return id; > + id++; > + } > + return NULL; > +} > + > +static int virtbus_match(struct device *dev, struct device_driver > *drv) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(drv); > + struct virtbus_device *vdev = to_virtbus_dev(dev); > + > + return virtbus_match_id(vdrv->id_table, vdev) != NULL; > +} > + > +static int virtbus_probe(struct device *dev) > +{ > + return dev->driver->probe(dev); > +} > + > +static int virtbus_remove(struct device *dev) > +{ > + return dev->driver->remove(dev); > +} > + > +static void virtbus_shutdown(struct device *dev) > +{ > + dev->driver->shutdown(dev); > +} > + > +static int virtbus_suspend(struct device *dev, pm_message_t state) > +{ > + if (dev->driver->suspend) > + return dev->driver->suspend(dev, state); > + > + return 0; > +} > + > +static int virtbus_resume(struct device *dev) > +{ > + if (dev->driver->resume) > + return dev->driver->resume(dev); > + > + return 0; > +} > + > +struct bus_type virtual_bus_type = { > + .name = "virtbus", > + .match = virtbus_match, > + .probe = virtbus_probe, > + .remove = virtbus_remove, > + .shutdown = virtbus_shutdown, > + .suspend = virtbus_suspend, > + .resume = virtbus_resume, > +}; > + > +/** > + * 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); > +} > + > +/** > + * 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; > + } > + > + /* 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; > + } > + > + > + vdev->dev.bus = &virtual_bus_type; > + vdev->dev.release = virtbus_release_device; > + > + vdev->id = ret; > + dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id); > + > + dev_dbg(&vdev->dev, "Registering virtbus device '%s'\n", > + dev_name(&vdev->dev)); > + > + ret = device_add(&vdev->dev); > + if (ret) > + goto device_add_err; > + > + return 0; > + > +device_add_err: > + ida_simple_remove(&virtbus_dev_ida, vdev->id); > + > +device_pre_err: > + dev_err(&vdev->dev, "Add device to virtbus failed!: %d\n", > ret); > + put_device(&vdev->dev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(virtbus_register_device); > + > +/** > + * 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); > + > +static int virtbus_probe_driver(struct device *_dev) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > + int ret; > + > + ret = dev_pm_domain_attach(_dev, true); > + if (ret) { > + dev_warn(_dev, "Failed to attatch to PM Domain : %d\n", > ret); > + return ret; > + } > + > + ret = vdrv->probe(vdev); > + if (ret) { > + dev_err(&vdev->dev, "Probe returned error\n"); > + dev_pm_domain_detach(_dev, true); > + } > + > + return ret; > +} > + > +static int virtbus_remove_driver(struct device *_dev) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > + int ret = 0; > + > + ret = vdrv->remove(vdev); > + dev_pm_domain_detach(_dev, true); > + > + return ret; > +} > + > +static void virtbus_shutdown_driver(struct device *_dev) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > + > + vdrv->shutdown(vdev); > +} > + > +static int virtbus_suspend_driver(struct device *_dev, pm_message_t > state) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > + > + if (vdrv->suspend) > + return vdrv->suspend(vdev, state); > + > + return 0; > +} > + > +static int virtbus_resume_driver(struct device *_dev) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > + > + if (vdrv->resume) > + return vdrv->resume(vdev); > + > + return 0; > +} > + > +/** > + * __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; Do we need the above callbacks here and in bus_type? I'm seeing the following message when I experimented with the virtualbus implementation in the audio driver. Removing them from the bus_type gets rid of this message. "Driver 'sof-ipc-test-virtbus-drv' needs updating - please use bus_type methods" Thanks, Ranjani > + > + 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); > + > +static int __init virtual_bus_init(void) > +{ > + return bus_register(&virtual_bus_type); > +} > + > +static void __exit virtual_bus_exit(void) > +{ > + bus_unregister(&virtual_bus_type); > + ida_destroy(&virtbus_dev_ida); > +} > + > +module_init(virtual_bus_init); > +module_exit(virtual_bus_exit); > diff --git a/include/linux/mod_devicetable.h > b/include/linux/mod_devicetable.h > index 4c2ddd0941a7..60bcfe75fb94 100644 > --- a/include/linux/mod_devicetable.h > +++ b/include/linux/mod_devicetable.h > @@ -832,4 +832,12 @@ struct mhi_device_id { > kernel_ulong_t driver_data; > }; > > +#define VIRTBUS_NAME_SIZE 20 > +#define VIRTBUS_MODULE_PREFIX "virtbus:" > + > +struct virtbus_dev_id { > + char name[VIRTBUS_NAME_SIZE]; > + kernel_ulong_t driver_data; > +}; > + > #endif /* LINUX_MOD_DEVICETABLE_H */ > diff --git a/include/linux/virtual_bus.h > b/include/linux/virtual_bus.h > new file mode 100644 > index 000000000000..4df06178e72f > --- /dev/null > +++ 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; > +}; > + > +struct virtbus_driver { > + int (*probe)(struct virtbus_device *); > + int (*remove)(struct virtbus_device *); > + void (*shutdown)(struct virtbus_device *); > + int (*suspend)(struct virtbus_device *, pm_message_t); > + int (*resume)(struct virtbus_device *); > + struct device_driver driver; > + const struct virtbus_dev_id *id_table; > +}; > + > +static inline > +struct virtbus_device *to_virtbus_dev(struct device *dev) > +{ > + return container_of(dev, struct virtbus_device, dev); > +} > + > +static inline > +struct virtbus_driver *to_virtbus_drv(struct device_driver *drv) > +{ > + return container_of(drv, struct virtbus_driver, driver); > +} > + > +int virtbus_register_device(struct virtbus_device *vdev); > +void virtbus_unregister_device(struct virtbus_device *vdev); > +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) > + > +#endif /* _VIRTUAL_BUS_H_ */ > diff --git a/scripts/mod/devicetable-offsets.c > b/scripts/mod/devicetable-offsets.c > index 010be8ba2116..0c8e0e3a7c84 100644 > --- a/scripts/mod/devicetable-offsets.c > +++ b/scripts/mod/devicetable-offsets.c > @@ -241,5 +241,8 @@ int main(void) > DEVID(mhi_device_id); > DEVID_FIELD(mhi_device_id, chan); > > + DEVID(virtbus_dev_id); > + DEVID_FIELD(virtbus_dev_id, name); > + > return 0; > } > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c > index 02d5d79da284..7d78fa3fba34 100644 > --- a/scripts/mod/file2alias.c > +++ b/scripts/mod/file2alias.c > @@ -1358,7 +1358,13 @@ static int do_mhi_entry(const char *filename, > void *symval, char *alias) > { > DEF_FIELD_ADDR(symval, mhi_device_id, chan); > sprintf(alias, MHI_DEVICE_MODALIAS_FMT, *chan); > + return 1; > +} > > +static int do_virtbus_entry(const char *filename, void *symval, char > *alias) > +{ > + DEF_FIELD_ADDR(symval, virtbus_dev_id, name); > + sprintf(alias, VIRTBUS_MODULE_PREFIX "%s", *name); > return 1; > } > > @@ -1436,6 +1442,7 @@ static const struct devtable devtable[] = { > {"tee", SIZE_tee_client_device_id, do_tee_entry}, > {"wmi", SIZE_wmi_device_id, do_wmi_entry}, > {"mhi", SIZE_mhi_device_id, do_mhi_entry}, > + {"virtbus", SIZE_virtbus_dev_id, do_virtbus_entry}, > }; > > /* Create MODULE_ALIAS() statements.
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
On Mon, Apr 20, 2020 at 10:59:12PM +0000, Ertman, David M wrote: > > -----Original Message----- > > From: Greg KH <gregkh@linuxfoundation.org> > > Sent: Saturday, April 18, 2020 5:51 AM > > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com> > > Cc: davem@davemloft.net; Ertman, David M <david.m.ertman@intel.com>; > > netdev@vger.kernel.org; linux-rdma@vger.kernel.org; nhorman@redhat.com; > > sassmann@redhat.com; jgg@ziepe.ca; parav@mellanox.com; > > galpress@amazon.com; selvin.xavier@broadcom.com; > > sriharsha.basavapatna@broadcom.com; benve@cisco.com; > > bharat@chelsio.com; xavier.huwei@huawei.com; yishaih@mellanox.com; > > leonro@mellanox.com; mkalderon@marvell.com; aditr@vmware.com; > > ranjani.sridharan@linux.intel.com; pierre-louis.bossart@linux.intel.com; Patil, > > Kiran <kiran.patil@intel.com>; Bowers, AndrewX <andrewx.bowers@intel.com> > > Subject: Re: [net-next 1/9] Implementation of Virtual Bus > > > > On Fri, Apr 17, 2020 at 10:10:26AM -0700, Jeff Kirsher wrote: > > > @@ -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; > > > > struct device already has a name, why do you need another one? > > The name in dev is the base name appended with the id to make sure each device > has unique name. The name in vdev is the abbreviated one (without the id) which > will be used in the matching function, so that a driver can claim to support > <name> and will be matched with all <name>.<id> devices for all id's. > > This is similar logic to platform_device's name field. Don't treat platform_device as a good example of much :) I still think this is duplicated stuff, but I'll let it go for now... > > > + void (*release)(struct virtbus_device *); > > > > A bus should have the release function, not the actual device itself. A > > device should not need function pointers. > > > > The bus does have a release function, but it is a wrapper to call the release defined by > the device. odd. That is normally handled by the bus, not by the device itself. > This is where the KO registering the virtbus_device is expected to clean up > the resources allocated for this device (e.g. free memory, etc). Having the virtual_bus_release > call a release callback in the virtual_device allows for extra cleanup from the originating KO > if necessary. > > The memory model of virtual bus is for the originating KO to manage the lifespan of the > memory for the virtual_device. The virtual_bus expects the KO defining the virtbus_device > have the memory allocated before registering a virtbus_device and to clean up that memory > when the release is called. > > The platform_device also has function pointers in it, by including a MFD object, but the > platform_bus is managing the memory for the platform_bus_object that contains the > platform_device which it why it using a generic kref_put to free memory. Again, platform_devices are not good things to emulate, they have grown into a total mess. Ok, given that you are going to be putting lots of different things on this "generic" type of bus, a release function for the device can make sense. Still feels odd, I wonder if you should just do something with the type of the device instead. thanks, greg k-h
> -----Original Message----- > From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Monday, April 20, 2020 5:45 PM > To: Ertman, David M <david.m.ertman@intel.com> > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; davem@davemloft.net; > gregkh@linuxfoundation.org; netdev@vger.kernel.org; linux- > rdma@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com; > parav@mellanox.com; galpress@amazon.com; > selvin.xavier@broadcom.com; sriharsha.basavapatna@broadcom.com; > benve@cisco.com; bharat@chelsio.com; xavier.huwei@huawei.com; > yishaih@mellanox.com; leonro@mellanox.com; mkalderon@marvell.com; > aditr@vmware.com; ranjani.sridharan@linux.intel.com; pierre- > louis.bossart@linux.intel.com; Patil, Kiran <kiran.patil@intel.com>; Bowers, > AndrewX <andrewx.bowers@intel.com> > 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
diff --git a/Documentation/driver-api/virtual_bus.rst b/Documentation/driver-api/virtual_bus.rst new file mode 100644 index 000000000000..a79db0e9231e --- /dev/null +++ b/Documentation/driver-api/virtual_bus.rst @@ -0,0 +1,62 @@ +=============================== +Virtual Bus Devices and Drivers +=============================== + +See <linux/virtual_bus.h> for the models for virtbus_device and virtbus_driver. +This bus is meant to be a lightweight software based bus to attach generic +devices and drivers to so that a chunk of data can be passed between them. + +One use case example is an RDMA driver needing to connect with several +different types of PCI LAN devices to be able to request resources from +them (queue sets). Each LAN driver that supports RDMA will register a +virtbus_device on the virtual bus for each physical function. The RDMA +driver will register as a virtbus_driver on the virtual bus to be +matched up with multiple virtbus_devices and receive a pointer to a +struct containing the callbacks that the PCI LAN drivers support for +registering with them. + +Sections in this document: + Virtbus devices + Virtbus drivers + Device Enumeration + Device naming and driver binding + Virtual Bus API entry points + +Virtbus devices +~~~~~~~~~~~~~~~ +Virtbus_devices support the minimal device functionality. Devices will +accept a name, and then, when added to the virtual bus, an automatically +generated index is concatenated onto it for the virtbus_device->name. + +Virtbus drivers +~~~~~~~~~~~~~~~ +Virtbus drivers register with the virtual bus to be matched with virtbus +devices. They expect to be registered with a probe and remove callback, +and also support shutdown, suspend, and resume callbacks. They otherwise +follow the standard driver behavior of having discovery and enumeration +handled in the bus infrastructure. + +Virtbus drivers register themselves with the API entry point +virtbus_register_driver and unregister with virtbus_unregister_driver. + +Device Enumeration +~~~~~~~~~~~~~~~~~~ +Enumeration is handled automatically by the bus infrastructure via the +ida_simple methods. + +Device naming and driver binding +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +The virtbus_device.dev.name is the canonical name for the device. It is +built from two other parts: + + - virtbus_device.name (also used for matching). + - virtbus_device.id (generated automatically from ida_simple calls) + +Virtbus device IDs are always in "<name>.<instance>" format. Instances are +automatically selected through an ida_simple_get so are positive integers. +Name is taken from the device name field. + +Driver IDs are simple <name>. + +Need to extract the name from the Virtual Device compare to name of the +driver. diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index 6d4e4497b59b..8b6c709730ba 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -203,4 +203,14 @@ config DA8XX_MSTPRI source "drivers/bus/fsl-mc/Kconfig" source "drivers/bus/mhi/Kconfig" +config VIRTUAL_BUS + tristate "Software based Virtual Bus" + help + Provides a software bus for virtbus_devices to be added to it + and virtbus_drivers to be registered on it. It matches driver + and device based on id and calls the driver's pobe routine. + One example is the irdma driver needing to connect with various + PCI LAN drivers to request resources (queues) to be able to perform + its function. + endmenu diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile index 05f32cd694a4..d30828a4768c 100644 --- a/drivers/bus/Makefile +++ b/drivers/bus/Makefile @@ -37,3 +37,5 @@ obj-$(CONFIG_DA8XX_MSTPRI) += da8xx-mstpri.o # MHI obj-$(CONFIG_MHI_BUS) += mhi/ + +obj-$(CONFIG_VIRTUAL_BUS) += virtual_bus.o diff --git a/drivers/bus/virtual_bus.c b/drivers/bus/virtual_bus.c new file mode 100644 index 000000000000..fb14cca40eea --- /dev/null +++ b/drivers/bus/virtual_bus.c @@ -0,0 +1,270 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * virtual_bus.c - lightweight software based bus for virtual devices + * + * Copyright (c) 2019-20 Intel Corporation + * + * Please see Documentation/driver-api/virtual_bus.rst for + * more information + */ + +#include <linux/string.h> +#include <linux/virtual_bus.h> +#include <linux/of_irq.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/pm_runtime.h> +#include <linux/pm_domain.h> +#include <linux/acpi.h> +#include <linux/device.h> + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Virtual Bus"); +MODULE_AUTHOR("David Ertman <david.m.ertman@intel.com>"); +MODULE_AUTHOR("Kiran Patil <kiran.patil@intel.com>"); + +static DEFINE_IDA(virtbus_dev_ida); + +static const +struct virtbus_dev_id *virtbus_match_id(const struct virtbus_dev_id *id, + struct virtbus_device *vdev) +{ + while (id->name[0]) { + if (!strcmp(vdev->name, id->name)) + return id; + id++; + } + return NULL; +} + +static int virtbus_match(struct device *dev, struct device_driver *drv) +{ + struct virtbus_driver *vdrv = to_virtbus_drv(drv); + struct virtbus_device *vdev = to_virtbus_dev(dev); + + return virtbus_match_id(vdrv->id_table, vdev) != NULL; +} + +static int virtbus_probe(struct device *dev) +{ + return dev->driver->probe(dev); +} + +static int virtbus_remove(struct device *dev) +{ + return dev->driver->remove(dev); +} + +static void virtbus_shutdown(struct device *dev) +{ + dev->driver->shutdown(dev); +} + +static int virtbus_suspend(struct device *dev, pm_message_t state) +{ + if (dev->driver->suspend) + return dev->driver->suspend(dev, state); + + return 0; +} + +static int virtbus_resume(struct device *dev) +{ + if (dev->driver->resume) + return dev->driver->resume(dev); + + return 0; +} + +struct bus_type virtual_bus_type = { + .name = "virtbus", + .match = virtbus_match, + .probe = virtbus_probe, + .remove = virtbus_remove, + .shutdown = virtbus_shutdown, + .suspend = virtbus_suspend, + .resume = virtbus_resume, +}; + +/** + * 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); +} + +/** + * 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; + } + + /* 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; + } + + + vdev->dev.bus = &virtual_bus_type; + vdev->dev.release = virtbus_release_device; + + vdev->id = ret; + dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id); + + dev_dbg(&vdev->dev, "Registering virtbus device '%s'\n", + dev_name(&vdev->dev)); + + ret = device_add(&vdev->dev); + if (ret) + goto device_add_err; + + return 0; + +device_add_err: + ida_simple_remove(&virtbus_dev_ida, vdev->id); + +device_pre_err: + dev_err(&vdev->dev, "Add device to virtbus failed!: %d\n", ret); + put_device(&vdev->dev); + + return ret; +} +EXPORT_SYMBOL_GPL(virtbus_register_device); + +/** + * 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); + +static int virtbus_probe_driver(struct device *_dev) +{ + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); + struct virtbus_device *vdev = to_virtbus_dev(_dev); + int ret; + + ret = dev_pm_domain_attach(_dev, true); + if (ret) { + dev_warn(_dev, "Failed to attatch to PM Domain : %d\n", ret); + return ret; + } + + ret = vdrv->probe(vdev); + if (ret) { + dev_err(&vdev->dev, "Probe returned error\n"); + dev_pm_domain_detach(_dev, true); + } + + return ret; +} + +static int virtbus_remove_driver(struct device *_dev) +{ + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); + struct virtbus_device *vdev = to_virtbus_dev(_dev); + int ret = 0; + + ret = vdrv->remove(vdev); + dev_pm_domain_detach(_dev, true); + + return ret; +} + +static void virtbus_shutdown_driver(struct device *_dev) +{ + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); + struct virtbus_device *vdev = to_virtbus_dev(_dev); + + vdrv->shutdown(vdev); +} + +static int virtbus_suspend_driver(struct device *_dev, pm_message_t state) +{ + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); + struct virtbus_device *vdev = to_virtbus_dev(_dev); + + if (vdrv->suspend) + return vdrv->suspend(vdev, state); + + return 0; +} + +static int virtbus_resume_driver(struct device *_dev) +{ + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); + struct virtbus_device *vdev = to_virtbus_dev(_dev); + + if (vdrv->resume) + return vdrv->resume(vdev); + + return 0; +} + +/** + * __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); + +static int __init virtual_bus_init(void) +{ + return bus_register(&virtual_bus_type); +} + +static void __exit virtual_bus_exit(void) +{ + bus_unregister(&virtual_bus_type); + ida_destroy(&virtbus_dev_ida); +} + +module_init(virtual_bus_init); +module_exit(virtual_bus_exit); diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index 4c2ddd0941a7..60bcfe75fb94 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -832,4 +832,12 @@ struct mhi_device_id { kernel_ulong_t driver_data; }; +#define VIRTBUS_NAME_SIZE 20 +#define VIRTBUS_MODULE_PREFIX "virtbus:" + +struct virtbus_dev_id { + char name[VIRTBUS_NAME_SIZE]; + kernel_ulong_t driver_data; +}; + #endif /* LINUX_MOD_DEVICETABLE_H */ diff --git a/include/linux/virtual_bus.h b/include/linux/virtual_bus.h new file mode 100644 index 000000000000..4df06178e72f --- /dev/null +++ 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; +}; + +struct virtbus_driver { + int (*probe)(struct virtbus_device *); + int (*remove)(struct virtbus_device *); + void (*shutdown)(struct virtbus_device *); + int (*suspend)(struct virtbus_device *, pm_message_t); + int (*resume)(struct virtbus_device *); + struct device_driver driver; + const struct virtbus_dev_id *id_table; +}; + +static inline +struct virtbus_device *to_virtbus_dev(struct device *dev) +{ + return container_of(dev, struct virtbus_device, dev); +} + +static inline +struct virtbus_driver *to_virtbus_drv(struct device_driver *drv) +{ + return container_of(drv, struct virtbus_driver, driver); +} + +int virtbus_register_device(struct virtbus_device *vdev); +void virtbus_unregister_device(struct virtbus_device *vdev); +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) + +#endif /* _VIRTUAL_BUS_H_ */ diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c index 010be8ba2116..0c8e0e3a7c84 100644 --- a/scripts/mod/devicetable-offsets.c +++ b/scripts/mod/devicetable-offsets.c @@ -241,5 +241,8 @@ int main(void) DEVID(mhi_device_id); DEVID_FIELD(mhi_device_id, chan); + DEVID(virtbus_dev_id); + DEVID_FIELD(virtbus_dev_id, name); + return 0; } diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c index 02d5d79da284..7d78fa3fba34 100644 --- a/scripts/mod/file2alias.c +++ b/scripts/mod/file2alias.c @@ -1358,7 +1358,13 @@ static int do_mhi_entry(const char *filename, void *symval, char *alias) { DEF_FIELD_ADDR(symval, mhi_device_id, chan); sprintf(alias, MHI_DEVICE_MODALIAS_FMT, *chan); + return 1; +} +static int do_virtbus_entry(const char *filename, void *symval, char *alias) +{ + DEF_FIELD_ADDR(symval, virtbus_dev_id, name); + sprintf(alias, VIRTBUS_MODULE_PREFIX "%s", *name); return 1; } @@ -1436,6 +1442,7 @@ static const struct devtable devtable[] = { {"tee", SIZE_tee_client_device_id, do_tee_entry}, {"wmi", SIZE_wmi_device_id, do_wmi_entry}, {"mhi", SIZE_mhi_device_id, do_mhi_entry}, + {"virtbus", SIZE_virtbus_dev_id, do_virtbus_entry}, }; /* Create MODULE_ALIAS() statements.