| Message ID | 20191111192219.30259-1-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [net-next,1/1] virtual-bus: Implementation of Virtual Bus | expand |
On Mon, Nov 11, 2019 at 11:22:19AM -0800, 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 lightweight > devices and drivers and provide matching between them > and probing of the registered drivers. > > Files added: > drivers/bus/virtual_bus.c > include/linux/virtual_bus.h > Documentation/driver-api/virtual_bus.rst > > The primary purpose of the virual bus is to provide > matching services and to pass the data pointer > contained in the virtbus_device to the virtbus_driver > during its probe call. This will allow two separate > kernel objects to match up and start communication. I think this is the 'multi_subsystem_device' idea I threw out in this thread. ie pass an opaque void *pointer, done here by virtbus_get_devdata(): https://lore.kernel.org/r/20191109084659.GB1289838@kroah.com And Greg said 'Ick, no'.. So each driver should makes its own bus, and perhaps we should provide some helper stuff for the repeating code, like PM function reflection? Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Tuesday, November 12, 2019 1:00 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; Patil, Kiran <kiran.patil@intel.com> > Subject: Re: [net-next 1/1] virtual-bus: Implementation of Virtual Bus > > On Mon, Nov 11, 2019 at 11:22:19AM -0800, 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 lightweight devices and drivers and provide matching > > between them and probing of the registered drivers. > > > > Files added: > > drivers/bus/virtual_bus.c > > include/linux/virtual_bus.h > > Documentation/driver-api/virtual_bus.rst > > > > The primary purpose of the virual bus is to provide matching services > > and to pass the data pointer contained in the virtbus_device to the > > virtbus_driver during its probe call. This will allow two separate > > kernel objects to match up and start communication. > > I think this is the 'multi_subsystem_device' idea I threw out in this thread. ie > pass an opaque void *pointer, done here by > virtbus_get_devdata(): > > https://lore.kernel.org/r/20191109084659.GB1289838@kroah.com > > And Greg said 'Ick, no'.. > > So each driver should makes its own bus, and perhaps we should provide > some helper stuff for the repeating code, like PM function reflection? > > Jason This submission is from a thread with GregKH where I put forth a design proposal of implementing a new software based bus. I originally was going to name it generic bus, and he suggested virtual bus. Here is the meat of the thread: >> We could add a new module to the kernel named generic_bus.ko. This >> would create a new generic software bus and a set of APIs that would >> allow for adding and removing simple generic virtual devices and >> drivers, not as a MFD cell or a platform device. The power management >> events would also be handled by the generic_bus infrastructure (suspend, resume, shutdown). >> We would use this for matching up by having the irdma driver register >> with this generic bus and hook to virtual devices that were added from >> different PCI LAN drivers. >> >> Pros: >> 1) This would avoid us attaching anything to the platform bus >> 2) Avoid having each PCI LAN driver creating its own software bus >> 3) Provide a common matching ground for generic devices and drivers >> that eliminates problems caused by load order (all dependent on >> generic_bus.ko) >> 4) Usable by any other entity that wants a lightweight matching system >> or information exchange mechanism >> >> Cons: >> 1) Duplicates part of the platform bus functionality >> 2) Adds a new software bus to the kernel architecture >> >> Is this path forward acceptable? > >Yes, that is much better. But how about calling it a "virtual bus"? >It's not really virtualization, but we already have virtual devices today when you look in sysfs for devices that are created that are not >associated with any specific bus. So this could take those over quite nicely! Look at how /sys/devices/virtual/ works for specifics, you could >create a new virtual bus of a specific "name" and then add devices to that bus directly. > >thanks, > >greg k-h I am hoping that I didn't completely misunderstand him. -Dave E
On Mon, Nov 11, 2019 at 11:22:19AM -0800, 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 lightweight > devices and drivers and provide matching between them > and probing of the registered drivers. > > Files added: > drivers/bus/virtual_bus.c > include/linux/virtual_bus.h > Documentation/driver-api/virtual_bus.rst > > The primary purpose of the virual bus is to provide > matching services and to pass the data pointer > contained in the virtbus_device to the virtbus_driver > during its probe call. This will allow two separate > kernel objects to match up and start communication. > > 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> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> Interesting, and kind of what I was thinking of, but the implementation is odd and I don't really see how you can use it. Can you provide a second patch that actually uses this api? Code comments below for when you resend: > +Virtual Bus Structs > +~~~~~~~~~~~~~~~~~~~ > +struct device virtual_bus = { > + .init_name = "virtbus", > + .release = virtual_bus_release, > +}; > + > +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, > +}; > + > +struct virtbus_device { > + const char *name; > + int id; > + const struct virtbus_dev_id *dev_id; > + struct device dev; > + void *data; > +}; > + > +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 state); > + int (*resume)(struct virtbus_device *); > + struct device_driver driver; > +}; All of the above should come straight from the .c/.h files, no need to duplicate it in a text file that will be guaranteed to get out of sync. > diff --git a/drivers/bus/virtual_bus.c b/drivers/bus/virtual_bus.c > new file mode 100644 > index 000000000000..af3f6d9b60f4 > --- /dev/null > +++ b/drivers/bus/virtual_bus.c > @@ -0,0 +1,339 @@ > +// 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("Lightweight Virtual Bus"); > +MODULE_AUTHOR("David Ertman <david.m.ertman@intel.com>"); > +MODULE_AUTHOR("Kiran Patil <kiran.patil@intel.com>"); > + > +static DEFINE_IDA(virtbus_dev_id); > + > +static void virtual_bus_release(struct device *dev) > +{ > + pr_info("Removing virtual bus device.\n"); > +} This is just one step away from doing horrible things. A release function should free the memory. Not just print a message :( Also, this is the driver code, use dev_info() and friends, never use pr_*() Same goes for all places in this code. So this is a debugging line, why? How can this be called? You only use it: > + > +struct device virtual_bus = { > + .init_name = "virtbus", > + .release = virtual_bus_release, > +}; > +EXPORT_SYMBOL_GPL(virtual_bus); Here. Ick. A static struct device? Called 'bus'? That's _REALLY_ confusing. What is this for? And why export it? That's guaranteed to cause problems (hint, code lifecycle vs. data lifecycles...) > +/** > + * virtbus_add_dev - add a virtual bus device > + * @vdev: virtual bus device to add > + */ > +int virtbus_dev_add(struct virtbus_device *vdev) > +{ > + int ret; > + > + if (!vdev) > + return -EINVAL; > + > + device_initialize(&vdev->dev); > + if (!vdev->dev.parent) > + vdev->dev.parent = &virtual_bus; So it's a parent? Ok, then why export it? Again I want to see a user please... > + > + vdev->dev.bus = &virtual_bus_type; > + /* All device IDs are automatically allocated */ > + ret = ida_simple_get(&virtbus_dev_id, 0, 0, GFP_KERNEL); > + if (ret < 0) > + return ret; > + > + vdev->id = ret; > + dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id); > + > + pr_debug("Registering VirtBus device '%s'. Parent at %s\n", > + dev_name(&vdev->dev), dev_name(vdev->dev.parent)); dev_dbg(). > + > + ret = device_add(&vdev->dev); > + if (!ret) > + return ret; > + > + /* Error adding virtual device */ > + ida_simple_remove(&virtbus_dev_id, vdev->id); > + vdev->id = VIRTBUS_DEVID_NONE; That's all you need to clean up? Did you read the device_add() documentation? Please do so. And what's up with this DEVID_NONE stuff? > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(virtbus_dev_add); > + > +/** > + * virtbus_dev_del - remove a virtual bus device > + * vdev: virtual bus device we are removing > + */ > +void virtbus_dev_del(struct virtbus_device *vdev) > +{ > + if (!IS_ERR_OR_NULL(vdev)) { > + device_del(&vdev->dev); > + > + ida_simple_remove(&virtbus_dev_id, vdev->id); > + vdev->id = VIRTBUS_DEVID_NONE; > + } > +} > +EXPORT_SYMBOL_GPL(virtbus_dev_del); > + > +struct virtbus_object { > + struct virtbus_device vdev; > + char name[]; > +}; A device has a name, why have another one? > + > +/** > + * virtbus_dev_release - Destroy a virtbus device > + * @vdev: virtual device to release > + * > + * Note that the vdev->data which is separately allocated needs to be > + * separately freed on it own. > + */ > +static void virtbus_dev_release(struct device *dev) > +{ > + struct virtbus_object *vo = container_of(dev, struct virtbus_object, > + vdev.dev); > + > + kfree(vo); > +} > + > +/** > + * virtbus_dev_alloc - allocate a virtbus device > + * @name: name to associate with the vdev > + * @data: pointer to data to be associated with this device > + */ > +struct virtbus_device *virtbus_dev_alloc(const char *name, void *data) > +{ > + struct virtbus_object *vo; > + > + /* Create a virtbus object to contain the vdev and name. This > + * avoids a problem with the const attribute of name in the vdev. > + * The virtbus_object will be allocated here and freed in the > + * release function. > + */ > + vo = kzalloc(sizeof(*vo) + strlen(name) + 1, GFP_KERNEL); > + if (!vo) > + return NULL; What problem are you trying to work around with the name? > + > + strcpy(vo->name, name); > + vo->vdev.name = vo->name; > + vo->vdev.data = data; > + vo->vdev.dev.release = virtbus_dev_release; > + > + return &vo->vdev; > +} > +EXPORT_SYMBOL_GPL(virtbus_dev_alloc); Why have an alloc/add pair of functions? Why not just one? > + > +static int virtbus_drv_probe(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; > + } > + > + if (vdrv->probe) { > + ret = vdrv->probe(vdev); > + if (ret) > + dev_pm_domain_detach(_dev, true); > + } > + > + return ret; > +} > + > +static int virtbus_drv_remove(struct device *_dev) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > + int ret = 0; > + > + if (vdrv->remove) > + ret = vdrv->remove(vdev); > + > + dev_pm_domain_detach(_dev, true); > + > + return ret; > +} > + > +static void virtbus_drv_shutdown(struct device *_dev) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > + > + if (vdrv->shutdown) > + vdrv->shutdown(vdev); > +} > + > +static int virtbus_drv_suspend(struct device *_dev, pm_message_t state) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > + > + return vdrv->suspend ? vdrv->suspend(vdev, state) : 0; > +} > + > +static int virtbus_drv_resume(struct device *_dev) > +{ > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > + > + return vdrv->resume ? vdrv->resume(vdev) : 0; > +} > + > +/** > + * virtbus_drv_register - register a driver for virtual bus devices > + * @vdrv: virtbus_driver structure > + * @owner: owning module/driver > + */ > +int virtbus_drv_register(struct virtbus_driver *vdrv, struct module *owner) Don't force someone to type THIS_MODULE for this call, use the macro trick instead that most subsystems use. Again, I want to see a user, that will cause lots of these types of things to be painfully obvious :) > +{ > + vdrv->driver.owner = owner; > + vdrv->driver.bus = &virtual_bus_type; > + vdrv->driver.probe = virtbus_drv_probe; > + vdrv->driver.remove = virtbus_drv_remove; > + vdrv->driver.shutdown = virtbus_drv_shutdown; > + vdrv->driver.suspend = virtbus_drv_suspend; > + vdrv->driver.resume = virtbus_drv_resume; > + > + return driver_register(&vdrv->driver); > +} > +EXPORT_SYMBOL_GPL(virtbus_drv_register); > + > +/** > + * virtbus_drv_unregister - unregister a driver for virtual bus devices > + * @drv: virtbus_driver structure > + */ > +void virtbus_drv_unregister(struct virtbus_driver *vdrv) > +{ > + driver_unregister(&vdrv->driver); > +} > +EXPORT_SYMBOL_GPL(virtbus_drv_unregister); > + > +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) == 0) { > + vdev->dev_id = id; > + return id; > + } > + id++; > + } > + return NULL; > +} > + > +/** > + * virtbus_match - bind virtbus device to virtbus driver > + * @dev: device > + * @drv: driver > + * > + * Virtbus device IDs are always in "<name>.<instance>" format. > + * Instances are automatically selected through an ida_simple_get so > + * are positive integers. Names are 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. > + */ > +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); > + > + if (vdrv->id_table) > + return virtbus_match_id(vdrv->id_table, vdev) != NULL; > + > + return (strcmp(vdev->name, drv->name) == 0); > +} > + > +/** > + * virtbus_probe - call probe of the virtbus_drv > + * @dev: device struct > + */ > +static int virtbus_probe(struct device *dev) > +{ > + return dev->driver->probe ? dev->driver->probe(dev) : 0; > +} > + > +static int virtbus_remove(struct device *dev) > +{ > + return dev->driver->remove ? dev->driver->remove(dev) : 0; > +} > + > +static void virtbus_shutdown(struct device *dev) > +{ > + if (dev->driver->shutdown) > + 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); You have two different styles here with these calls, use this one instead of the crazy ? : style above in probe/remove please. > + > + 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, > +}; > +EXPORT_SYMBOL_GPL(virtual_bus_type); Why is this exported? > + > +static int __init virtual_bus_init(void) > +{ > + int err; > + > + err = device_register(&virtual_bus); > + if (err) { > + put_device(&virtual_bus); > + return err; > + } > + > + err = bus_register(&virtual_bus_type); > + if (err) { > + device_unregister(&virtual_bus); > + return err; > + } > + > + pr_debug("Virtual Bus (virtbus) registered with kernel\n"); Don't be noisy. And remove your debugging code :) > + return err; > +} > + > +static void __exit virtual_bus_exit(void) > +{ > + bus_unregister(&virtual_bus_type); > + device_unregister(&virtual_bus); > +} > + > +module_init(virtual_bus_init); > +module_exit(virtual_bus_exit); > diff --git a/include/linux/virtual_bus.h b/include/linux/virtual_bus.h > new file mode 100644 > index 000000000000..722f020ac53b > --- /dev/null > +++ b/include/linux/virtual_bus.h > @@ -0,0 +1,64 @@ > +/* 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> > + > +#define VIRTBUS_DEVID_NONE (-1) What is this for? > +#define VIRTBUS_NAME_SIZE 20 Why? Why is 20 "ok"? > + > +struct virtbus_dev_id { > + char name[VIRTBUS_NAME_SIZE]; > + u64 driver_data; u64 or a pointer? You use both, pick one please. > +}; > + > +/* memory allocation for dev_id is expected to be done by the virtbus_driver > + * that will match with the virtbus_device, and the matching process will > + * copy the pointer from the matched element from the driver to the device. What pointer? I don't understand. > + */ > +struct virtbus_device { > + const char *name; > + int id; > + const struct virtbus_dev_id *dev_id; > + struct device dev; > + void *data; > +}; > + > +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 state); > + int (*resume)(struct virtbus_device *); > + struct device_driver driver; > + const struct virtbus_dev_id *id_table; > +}; > + > +#define virtbus_get_dev_id(vdev) ((vdev)->id_entry) > +#define virtbus_get_devdata(dev) ((dev)->devdata) What are these for? > +#define dev_is_virtbus(dev) ((dev)->bus == &virtbus_type) Who needs this? > +#define to_virtbus_dev(x) container_of((x), struct virtbus_device, dev) > +#define to_virtbus_drv(drv) (container_of((drv), struct virtbus_driver, \ > + driver)) Why are these in a public .h file? > + > +extern struct bus_type virtual_bus_type; > +extern struct device virtual_bus; Again, why exported? > + > +int virtbus_dev_add(struct virtbus_device *vdev); > +void virtbus_dev_del(struct virtbus_device *vdev); > +struct virtbus_device *virtbus_dev_alloc(const char *name, void *devdata); > +int virtbus_drv_register(struct virtbus_driver *vdrv, struct module *owner); > +void virtbus_drv_unregister(struct virtbus_driver *vdrv); > + > +int virtbus_for_each_dev(void *data, int (*fn)(struct device *, void *)); > +int virtbus_for_each_drv(void *data, int(*fn)(struct device_driver *, void *)); pick a coding style and stick with it please... thanks, greg k-h
On Tue, Nov 12, 2019 at 10:28:26PM +0100, Greg KH wrote: > > + */ > > +struct virtbus_device { > > + const char *name; > > + int id; > > + const struct virtbus_dev_id *dev_id; > > + struct device dev; > > + void *data; > > +}; > > + > > +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 state); > > + int (*resume)(struct virtbus_device *); > > + struct device_driver driver; > > + const struct virtbus_dev_id *id_table; > > +}; > > + > > +#define virtbus_get_dev_id(vdev) ((vdev)->id_entry) > > +#define virtbus_get_devdata(dev) ((dev)->devdata) > > What are these for? As far as I can see, the scheme here, using the language from the most recent discussion is: // in core or netdev module int mlx5_core_create() { struct mlx5_core_dev *core = kzalloc(..) [..] core->vdev = virtbus_dev_alloc("mlx5_core", core); } // in rdma module static int mlx5_rdma_probe(struct virtbus_device *dev) { // Get the value passed to virtbus_dev_alloc() struct mlx5_core_dev *core = virtbus_get_devdata(dev) // Use the huge API surrounding struct mlx5_core_dev qp = mlx5_core_create_qp(core, ...); } static struct virtbus_driver mlx5_rdma_driver = { .probe = mlx5_rdma_probe, .match = {"mlx5_core"} } Drivers that match "mlx5_core" know that the opaque 'virtbus_get_devdata()' is a 'struct mlx5_core_dev *' and use that access the core driver. A "ice_core" would know it is some 'struct ice_core_dev *' for Intel and uses that pointer, etc. ie it is just a way to a pass a 'void *' from one module to another while using the driver core to manage module autoloading and binding. Jason
Hi Jason, > From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Tuesday, November 12, 2019 6:08 PM > > On Tue, Nov 12, 2019 at 10:28:26PM +0100, Greg KH wrote: > > > > + */ > > > +struct virtbus_device { > > > + const char *name; > > > + int id; > > > + const struct virtbus_dev_id *dev_id; > > > + struct device dev; > > > + void *data; > > > +}; > > > + > > > +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 state); > > > + int (*resume)(struct virtbus_device *); > > > + struct device_driver driver; > > > + const struct virtbus_dev_id *id_table; }; > > > + > > > +#define virtbus_get_dev_id(vdev) ((vdev)->id_entry) > > > +#define virtbus_get_devdata(dev) ((dev)->devdata) > > > > What are these for? > > As far as I can see, the scheme here, using the language from the most > recent discussion is: > > // in core or netdev module > int mlx5_core_create() > { > struct mlx5_core_dev *core = kzalloc(..) > > [..] > > core->vdev = virtbus_dev_alloc("mlx5_core", core); > } > > > // in rdma module > static int mlx5_rdma_probe(struct virtbus_device *dev) > { > // Get the value passed to virtbus_dev_alloc() > struct mlx5_core_dev *core = virtbus_get_devdata(dev) > > // Use the huge API surrounding struct mlx5_core_dev > qp = mlx5_core_create_qp(core, ...); > } > > static struct virtbus_driver mlx5_rdma_driver = { > .probe = mlx5_rdma_probe, > .match = {"mlx5_core"} > } > > Drivers that match "mlx5_core" know that the opaque > 'virtbus_get_devdata()' is a 'struct mlx5_core_dev *' and use that access the > core driver. > > A "ice_core" would know it is some 'struct ice_core_dev *' for Intel and uses > that pointer, etc. > > ie it is just a way to a pass a 'void *' from one module to another while using > the driver core to manage module autoloading and binding. A small improvement below, because get_drvdata() and set_drvdata() is supposed to be called by the bus driver, not its creator. And below data structure achieve strong type checks, no void* casts, and exactly achieves the foo_device example. Isn't it better? mlx5_virtbus_device { struct virtbus_device dev; struct mlx5_core_dev *dev; }; mlx5_core_create(const struct mlx5_core_dev *coredev) { struct mlx5_virtbus_device *dev; dev = virtdev_alloc(sizeof(*dev)); dev->core = coredev; /* this should not be stored in drvdata */ virtdev_register(dev); } mlx5_rdma_probe(struct virtbus_device *dev) { struct mlx5_core_dev *coredev; struct mlx5_virtdev *virtdev; struct mlx5_ib_dev *ibdev; virtdev = container_of(virtdev, struct mlx5_virtdev, dev); coredev = virtdev->coredev; ibdev = ib_alloc_dev(); if (!bdev) return -ENOMEM; virtdev_set_drvdata(dev, ibdev); /* setup.. */ return 0; } mlx5_rdma_remove(struct virtbus_device *dev) { struct mlx5_ib_dev *ibdev; ibdev = virtdev_get_drvdata(dev); /* previous load failed */ if(!ibdev) return; [..] /* mirror of probe */ }
> -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Tuesday, November 12, 2019 1:28 PM > 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; parav@mellanox.com; > jgg@ziepe.ca; Patil, Kiran <kiran.patil@intel.com> > Subject: Re: [net-next 1/1] virtual-bus: Implementation of Virtual Bus > > On Mon, Nov 11, 2019 at 11:22:19AM -0800, 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 lightweight devices and drivers and provide matching > > between them and probing of the registered drivers. > > > > Files added: > > drivers/bus/virtual_bus.c > > include/linux/virtual_bus.h > > Documentation/driver-api/virtual_bus.rst > > > > The primary purpose of the virual bus is to provide matching services > > and to pass the data pointer contained in the virtbus_device to the > > virtbus_driver during its probe call. This will allow two separate > > kernel objects to match up and start communication. > > > > 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> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > Interesting, and kind of what I was thinking of, but the implementation is odd > and I don't really see how you can use it. > > Can you provide a second patch that actually uses this api? > > Code comments below for when you resend: > > > +Virtual Bus Structs > > +~~~~~~~~~~~~~~~~~~~ > > +struct device virtual_bus = { > > + .init_name = "virtbus", > > + .release = virtual_bus_release, > > +}; > > + > > +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, > > +}; > > + > > +struct virtbus_device { > > + const char *name; > > + int id; > > + const struct virtbus_dev_id *dev_id; > > + struct device dev; > > + void *data; > > +}; > > + > > +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 state); > > + int (*resume)(struct virtbus_device *); > > + struct device_driver driver; > > +}; > > > All of the above should come straight from the .c/.h files, no need to > duplicate it in a text file that will be guaranteed to get out of sync. > > > diff --git a/drivers/bus/virtual_bus.c b/drivers/bus/virtual_bus.c new > > file mode 100644 index 000000000000..af3f6d9b60f4 > > --- /dev/null > > +++ b/drivers/bus/virtual_bus.c > > @@ -0,0 +1,339 @@ > > +// 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("Lightweight Virtual Bus"); > MODULE_AUTHOR("David > > +Ertman <david.m.ertman@intel.com>"); MODULE_AUTHOR("Kiran Patil > > +<kiran.patil@intel.com>"); > > + > > +static DEFINE_IDA(virtbus_dev_id); > > + > > +static void virtual_bus_release(struct device *dev) { > > + pr_info("Removing virtual bus device.\n"); } > > This is just one step away from doing horrible things. > > A release function should free the memory. Not just print a message :( > > Also, this is the driver code, use dev_info() and friends, never use > pr_*() Same goes for all places in this code. > > So this is a debugging line, why? > How can this be called? You only use it: > > > + > > +struct device virtual_bus = { > > + .init_name = "virtbus", > > + .release = virtual_bus_release, > > +}; > > +EXPORT_SYMBOL_GPL(virtual_bus); > > Here. > > Ick. > > A static struct device? Called 'bus'? That's _REALLY_ confusing. What is this > for? And why export it? That's guaranteed to cause problems (hint, code > lifecycle vs. data lifecycles...) > > > +/** > > + * virtbus_add_dev - add a virtual bus device > > + * @vdev: virtual bus device to add > > + */ > > +int virtbus_dev_add(struct virtbus_device *vdev) { > > + int ret; > > + > > + if (!vdev) > > + return -EINVAL; > > + > > + device_initialize(&vdev->dev); > > + if (!vdev->dev.parent) > > + vdev->dev.parent = &virtual_bus; > > So it's a parent? Ok, then why export it? > > Again I want to see a user please... > > > + > > + vdev->dev.bus = &virtual_bus_type; > > + /* All device IDs are automatically allocated */ > > + ret = ida_simple_get(&virtbus_dev_id, 0, 0, GFP_KERNEL); > > + if (ret < 0) > > + return ret; > > + > > + vdev->id = ret; > > + dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id); > > + > > + pr_debug("Registering VirtBus device '%s'. Parent at %s\n", > > + dev_name(&vdev->dev), dev_name(vdev->dev.parent)); > > dev_dbg(). > > > + > > + ret = device_add(&vdev->dev); > > + if (!ret) > > + return ret; > > + > > + /* Error adding virtual device */ > > + ida_simple_remove(&virtbus_dev_id, vdev->id); > > + vdev->id = VIRTBUS_DEVID_NONE; > > That's all you need to clean up? Did you read the device_add() > documentation? Please do so. > > And what's up with this DEVID_NONE stuff? > > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(virtbus_dev_add); > > + > > +/** > > + * virtbus_dev_del - remove a virtual bus device > > + * vdev: virtual bus device we are removing */ void > > +virtbus_dev_del(struct virtbus_device *vdev) { > > + if (!IS_ERR_OR_NULL(vdev)) { > > + device_del(&vdev->dev); > > + > > + ida_simple_remove(&virtbus_dev_id, vdev->id); > > + vdev->id = VIRTBUS_DEVID_NONE; > > + } > > +} > > +EXPORT_SYMBOL_GPL(virtbus_dev_del); > > + > > +struct virtbus_object { > > + struct virtbus_device vdev; > > + char name[]; > > +}; > > A device has a name, why have another one? > > > + > > +/** > > + * virtbus_dev_release - Destroy a virtbus device > > + * @vdev: virtual device to release > > + * > > + * Note that the vdev->data which is separately allocated needs to be > > + * separately freed on it own. > > + */ > > +static void virtbus_dev_release(struct device *dev) { > > + struct virtbus_object *vo = container_of(dev, struct virtbus_object, > > + vdev.dev); > > + > > + kfree(vo); > > +} > > + > > +/** > > + * virtbus_dev_alloc - allocate a virtbus device > > + * @name: name to associate with the vdev > > + * @data: pointer to data to be associated with this device */ > > +struct virtbus_device *virtbus_dev_alloc(const char *name, void > > +*data) { > > + struct virtbus_object *vo; > > + > > + /* Create a virtbus object to contain the vdev and name. This > > + * avoids a problem with the const attribute of name in the vdev. > > + * The virtbus_object will be allocated here and freed in the > > + * release function. > > + */ > > + vo = kzalloc(sizeof(*vo) + strlen(name) + 1, GFP_KERNEL); > > + if (!vo) > > + return NULL; > > What problem are you trying to work around with the name? > > > + > > + strcpy(vo->name, name); > > + vo->vdev.name = vo->name; > > + vo->vdev.data = data; > > + vo->vdev.dev.release = virtbus_dev_release; > > + > > + return &vo->vdev; > > +} > > +EXPORT_SYMBOL_GPL(virtbus_dev_alloc); > > Why have an alloc/add pair of functions? Why not just one? > > > + > > +static int virtbus_drv_probe(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; > > + } > > + > > + if (vdrv->probe) { > > + ret = vdrv->probe(vdev); > > + if (ret) > > + dev_pm_domain_detach(_dev, true); > > + } > > + > > + return ret; > > +} > > + > > +static int virtbus_drv_remove(struct device *_dev) { > > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > > + int ret = 0; > > + > > + if (vdrv->remove) > > + ret = vdrv->remove(vdev); > > + > > + dev_pm_domain_detach(_dev, true); > > + > > + return ret; > > +} > > + > > +static void virtbus_drv_shutdown(struct device *_dev) { > > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > > + > > + if (vdrv->shutdown) > > + vdrv->shutdown(vdev); > > +} > > + > > +static int virtbus_drv_suspend(struct device *_dev, pm_message_t > > +state) { > > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > > + > > + return vdrv->suspend ? vdrv->suspend(vdev, state) : 0; } > > + > > +static int virtbus_drv_resume(struct device *_dev) { > > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > > + > > + return vdrv->resume ? vdrv->resume(vdev) : 0; } > > + > > +/** > > + * virtbus_drv_register - register a driver for virtual bus devices > > + * @vdrv: virtbus_driver structure > > + * @owner: owning module/driver > > + */ > > +int virtbus_drv_register(struct virtbus_driver *vdrv, struct module > > +*owner) > > Don't force someone to type THIS_MODULE for this call, use the macro trick > instead that most subsystems use. > > Again, I want to see a user, that will cause lots of these types of things to be > painfully obvious :) > > > > +{ > > + vdrv->driver.owner = owner; > > + vdrv->driver.bus = &virtual_bus_type; > > + vdrv->driver.probe = virtbus_drv_probe; > > + vdrv->driver.remove = virtbus_drv_remove; > > + vdrv->driver.shutdown = virtbus_drv_shutdown; > > + vdrv->driver.suspend = virtbus_drv_suspend; > > + vdrv->driver.resume = virtbus_drv_resume; > > + > > + return driver_register(&vdrv->driver); } > > +EXPORT_SYMBOL_GPL(virtbus_drv_register); > > + > > +/** > > + * virtbus_drv_unregister - unregister a driver for virtual bus > > +devices > > + * @drv: virtbus_driver structure > > + */ > > +void virtbus_drv_unregister(struct virtbus_driver *vdrv) { > > + driver_unregister(&vdrv->driver); > > +} > > +EXPORT_SYMBOL_GPL(virtbus_drv_unregister); > > + > > +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) == 0) { > > + vdev->dev_id = id; > > + return id; > > + } > > + id++; > > + } > > + return NULL; > > +} > > + > > +/** > > + * virtbus_match - bind virtbus device to virtbus driver > > + * @dev: device > > + * @drv: driver > > + * > > + * Virtbus device IDs are always in "<name>.<instance>" format. > > + * Instances are automatically selected through an ida_simple_get so > > + * are positive integers. Names are 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. > > + */ > > +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); > > + > > + if (vdrv->id_table) > > + return virtbus_match_id(vdrv->id_table, vdev) != NULL; > > + > > + return (strcmp(vdev->name, drv->name) == 0); } > > + > > +/** > > + * virtbus_probe - call probe of the virtbus_drv > > + * @dev: device struct > > + */ > > +static int virtbus_probe(struct device *dev) { > > + return dev->driver->probe ? dev->driver->probe(dev) : 0; } > > + > > +static int virtbus_remove(struct device *dev) { > > + return dev->driver->remove ? dev->driver->remove(dev) : 0; } > > + > > +static void virtbus_shutdown(struct device *dev) { > > + if (dev->driver->shutdown) > > + 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); > > You have two different styles here with these calls, use this one instead of > the crazy ? : style above in probe/remove please. > > > + > > + 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, > > +}; > > +EXPORT_SYMBOL_GPL(virtual_bus_type); > > Why is this exported? > > > + > > +static int __init virtual_bus_init(void) { > > + int err; > > + > > + err = device_register(&virtual_bus); > > + if (err) { > > + put_device(&virtual_bus); > > + return err; > > + } > > + > > + err = bus_register(&virtual_bus_type); > > + if (err) { > > + device_unregister(&virtual_bus); > > + return err; > > + } > > + > > + pr_debug("Virtual Bus (virtbus) registered with kernel\n"); > > Don't be noisy. And remove your debugging code :) > > > > + return err; > > +} > > + > > +static void __exit virtual_bus_exit(void) { > > + bus_unregister(&virtual_bus_type); > > + device_unregister(&virtual_bus); > > +} > > + > > +module_init(virtual_bus_init); > > +module_exit(virtual_bus_exit); > > diff --git a/include/linux/virtual_bus.h b/include/linux/virtual_bus.h > > new file mode 100644 index 000000000000..722f020ac53b > > --- /dev/null > > +++ b/include/linux/virtual_bus.h > > @@ -0,0 +1,64 @@ > > +/* 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> > > + > > +#define VIRTBUS_DEVID_NONE (-1) > > What is this for? > > > +#define VIRTBUS_NAME_SIZE 20 > > Why? Why is 20 "ok"? > > > + > > +struct virtbus_dev_id { > > + char name[VIRTBUS_NAME_SIZE]; > > + u64 driver_data; > > u64 or a pointer? You use both, pick one please. > > > +}; > > + > > +/* memory allocation for dev_id is expected to be done by the > > +virtbus_driver > > + * that will match with the virtbus_device, and the matching process > > +will > > + * copy the pointer from the matched element from the driver to the > device. > > What pointer? I don't understand. > > > + */ > > +struct virtbus_device { > > + const char *name; > > + int id; > > + const struct virtbus_dev_id *dev_id; > > + struct device dev; > > + void *data; > > +}; > > + > > +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 state); > > + int (*resume)(struct virtbus_device *); > > + struct device_driver driver; > > + const struct virtbus_dev_id *id_table; }; > > + > > +#define virtbus_get_dev_id(vdev) ((vdev)->id_entry) > > +#define virtbus_get_devdata(dev) ((dev)->devdata) > > What are these for? > > > +#define dev_is_virtbus(dev) ((dev)->bus == &virtbus_type) > > Who needs this? > > > +#define to_virtbus_dev(x) container_of((x), struct virtbus_device, dev) > > +#define to_virtbus_drv(drv) (container_of((drv), struct > virtbus_driver, \ > > + driver)) > > Why are these in a public .h file? > > > + > > +extern struct bus_type virtual_bus_type; extern struct device > > +virtual_bus; > > Again, why exported? > > > + > > +int virtbus_dev_add(struct virtbus_device *vdev); void > > +virtbus_dev_del(struct virtbus_device *vdev); struct virtbus_device > > +*virtbus_dev_alloc(const char *name, void *devdata); int > > +virtbus_drv_register(struct virtbus_driver *vdrv, struct module > > +*owner); void virtbus_drv_unregister(struct virtbus_driver *vdrv); > > + > > +int virtbus_for_each_dev(void *data, int (*fn)(struct device *, void > > +*)); int virtbus_for_each_drv(void *data, int(*fn)(struct > > +device_driver *, void *)); > > pick a coding style and stick with it please... > > thanks, > > greg k-h Thanks for the quick feedback!! Working on requests and feedback. -Dave E
On Wed, Nov 13, 2019 at 01:03:44AM +0000, Parav Pandit wrote: > A small improvement below, because get_drvdata() and set_drvdata() Here it was called 'devdata' not the existing drvdata - so something different, I was confused for a bit too.. > is supposed to be called by the bus driver, not its creator. And > below data structure achieve strong type checks, no void* casts, and > exactly achieves the foo_device example. Isn't it better? > mlx5_virtbus_device { > struct virtbus_device dev; > struct mlx5_core_dev *dev; > }; This does seem a bit cleaner than using the void * trick (more, OOPy at least) Jason
> From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Tuesday, November 12, 2019 7:11 PM > > > A small improvement below, because get_drvdata() and set_drvdata() > > Here it was called 'devdata' not the existing drvdata - so something different, I > was confused for a bit too.. > Oh ok. but looks buggy in the patch as virtbus_dev doesn't have devdata field. Anyways, container_of() is better type checked anyway as below. +#define virtbus_get_devdata(dev) ((dev)->devdata) > > is supposed to be called by the bus driver, not its creator. And > > below data structure achieve strong type checks, no void* casts, and > > exactly achieves the foo_device example. Isn't it better? > > > mlx5_virtbus_device { > > struct virtbus_device dev; > > struct mlx5_core_dev *dev; > > }; > > This does seem a bit cleaner than using the void * trick (more, OOPy at least) > Ok. thanks.
Hi Greg, Jason, > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Tuesday, November 12, 2019 3:28 PM > > 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 lightweight devices and drivers and provide matching > > between them and probing of the registered drivers. > > > > Files added: > > drivers/bus/virtual_bus.c > > include/linux/virtual_bus.h > > Documentation/driver-api/virtual_bus.rst > > > > The primary purpose of the virual bus is to provide matching services > > and to pass the data pointer contained in the virtbus_device to the > > virtbus_driver during its probe call. This will allow two separate > > kernel objects to match up and start communication. > > > > The bus will support probe/remove shutdown and suspend/resume > > callbacks. > > > > Kconfig and Makefile alterations are included > > [..] I have a basic question with this bus. I read device-driver model [1] few times but couldn't get the clarity. mlx5_core driver will create virtbus device on virtbus. mlx5_ib driver binds to virtbus device in probe(). However mlx5_ib driver's probe() will create rdma device whose parent device will be PCI device and not virtbus_device. Is that correct? If so, bus driver of bus A, creating devices binding to device of bus B (pci) adheres to the linux device model? If so, such cross binding is not just limited to virtbus, right? [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/driver-model
> -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Tuesday, November 12, 2019 1:28 PM > 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; parav@mellanox.com; > jgg@ziepe.ca; Patil, Kiran <kiran.patil@intel.com> > Subject: Re: [net-next 1/1] virtual-bus: Implementation of Virtual Bus > > On Mon, Nov 11, 2019 at 11:22:19AM -0800, 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 lightweight devices and drivers and provide matching > > between them and probing of the registered drivers. > > > > Files added: > > drivers/bus/virtual_bus.c > > include/linux/virtual_bus.h > > Documentation/driver-api/virtual_bus.rst > > > > The primary purpose of the virual bus is to provide matching services > > and to pass the data pointer contained in the virtbus_device to the > > virtbus_driver during its probe call. This will allow two separate > > kernel objects to match up and start communication. > > > > 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> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > Interesting, and kind of what I was thinking of, but the implementation is odd > and I don't really see how you can use it. > > Can you provide a second patch that actually uses this api? > > Code comments below for when you resend: > > > +Virtual Bus Structs > > +~~~~~~~~~~~~~~~~~~~ > > +struct device virtual_bus = { > > + .init_name = "virtbus", > > + .release = virtual_bus_release, > > +}; > > + > > +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, > > +}; > > + > > +struct virtbus_device { > > + const char *name; > > + int id; > > + const struct virtbus_dev_id *dev_id; > > + struct device dev; > > + void *data; > > +}; > > + > > +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 state); > > + int (*resume)(struct virtbus_device *); > > + struct device_driver driver; > > +}; > > > All of the above should come straight from the .c/.h files, no need to > duplicate it in a text file that will be guaranteed to get out of sync. Removed > > > diff --git a/drivers/bus/virtual_bus.c b/drivers/bus/virtual_bus.c new > > file mode 100644 index 000000000000..af3f6d9b60f4 > > --- /dev/null > > +++ b/drivers/bus/virtual_bus.c > > @@ -0,0 +1,339 @@ > > +// 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("Lightweight Virtual Bus"); > MODULE_AUTHOR("David > > +Ertman <david.m.ertman@intel.com>"); MODULE_AUTHOR("Kiran Patil > > +<kiran.patil@intel.com>"); > > + > > +static DEFINE_IDA(virtbus_dev_id); > > + > > +static void virtual_bus_release(struct device *dev) { > > + pr_info("Removing virtual bus device.\n"); } > > This is just one step away from doing horrible things. > > A release function should free the memory. Not just print a message :( > > Also, this is the driver code, use dev_info() and friends, never use > pr_*() Same goes for all places in this code. > > So this is a debugging line, why? > How can this be called? You only use it: This was leftover and not necessary, sorry for the thrash. Removed unnecessary code. > > > + > > +struct device virtual_bus = { > > + .init_name = "virtbus", > > + .release = virtual_bus_release, > > +}; > > +EXPORT_SYMBOL_GPL(virtual_bus); > > Here. > > Ick. > > A static struct device? Called 'bus'? That's _REALLY_ confusing. What is this > for? And why export it? That's guaranteed to cause problems (hint, code > lifecycle vs. data lifecycles...) > EXPORT_SYMBOL removed. This was originally going to act as a default parent device for the bus devices, but after further investigation, this was unnecessary for the intended functionality of the bus - removed. > > +/** > > + * virtbus_add_dev - add a virtual bus device > > + * @vdev: virtual bus device to add > > + */ > > +int virtbus_dev_add(struct virtbus_device *vdev) { > > + int ret; > > + > > + if (!vdev) > > + return -EINVAL; > > + > > + device_initialize(&vdev->dev); > > + if (!vdev->dev.parent) > > + vdev->dev.parent = &virtual_bus; > > So it's a parent? Ok, then why export it? > > Again I want to see a user please... Removed, and consumers will be sent :) > > > + > > + vdev->dev.bus = &virtual_bus_type; > > + /* All device IDs are automatically allocated */ > > + ret = ida_simple_get(&virtbus_dev_id, 0, 0, GFP_KERNEL); > > + if (ret < 0) > > + return ret; > > + > > + vdev->id = ret; > > + dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id); > > + > > + pr_debug("Registering VirtBus device '%s'. Parent at %s\n", > > + dev_name(&vdev->dev), dev_name(vdev->dev.parent)); > > dev_dbg(). > > > + > > + ret = device_add(&vdev->dev); > > + if (!ret) > > + return ret; > > + > > + /* Error adding virtual device */ > > + ida_simple_remove(&virtbus_dev_id, vdev->id); > > + vdev->id = VIRTBUS_DEVID_NONE; > > That's all you need to clean up? Did you read the device_add() > documentation? Please do so. Device_del() added to error chain. > > And what's up with this DEVID_NONE stuff? Using VIRTBUS_DEVID_NONE defined to be -1, this is so the consumer can check The vdev->id field to see if it contains a valid id (0, 1, ..., n). > > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(virtbus_dev_add); > > + > > +/** > > + * virtbus_dev_del - remove a virtual bus device > > + * vdev: virtual bus device we are removing */ void > > +virtbus_dev_del(struct virtbus_device *vdev) { > > + if (!IS_ERR_OR_NULL(vdev)) { > > + device_del(&vdev->dev); > > + > > + ida_simple_remove(&virtbus_dev_id, vdev->id); > > + vdev->id = VIRTBUS_DEVID_NONE; > > + } > > +} > > +EXPORT_SYMBOL_GPL(virtbus_dev_del); > > + > > +struct virtbus_object { > > + struct virtbus_device vdev; > > + char name[]; > > +}; > > A device has a name, why have another one? I also followed the platform device's method here for addressing a problem around assigning the string fed in for name to the const char* for name in the struct. We both created an object to contain the memory for both the device and the string for the name, then just assign the pointer in the device for name to this memory location. This way the allocated space for the name lives as long as the device does. > > > + > > +/** > > + * virtbus_dev_release - Destroy a virtbus device > > + * @vdev: virtual device to release > > + * > > + * Note that the vdev->data which is separately allocated needs to be > > + * separately freed on it own. > > + */ > > +static void virtbus_dev_release(struct device *dev) { > > + struct virtbus_object *vo = container_of(dev, struct virtbus_object, > > + vdev.dev); > > + > > + kfree(vo); > > +} > > + > > +/** > > + * virtbus_dev_alloc - allocate a virtbus device > > + * @name: name to associate with the vdev > > + * @data: pointer to data to be associated with this device */ > > +struct virtbus_device *virtbus_dev_alloc(const char *name, void > > +*data) { > > + struct virtbus_object *vo; > > + > > + /* Create a virtbus object to contain the vdev and name. This > > + * avoids a problem with the const attribute of name in the vdev. > > + * The virtbus_object will be allocated here and freed in the > > + * release function. > > + */ > > + vo = kzalloc(sizeof(*vo) + strlen(name) + 1, GFP_KERNEL); > > + if (!vo) > > + return NULL; > > What problem are you trying to work around with the name? Since the name is a const char, you cannot strcpy into it. This allows for just assigning a pointer for this struct element to an already created string and being sure that the memory allocated for the string lives as long as the virtbus_device lives. > > > + > > + strcpy(vo->name, name); > > + vo->vdev.name = vo->name; > > + vo->vdev.data = data; > > + vo->vdev.dev.release = virtbus_dev_release; > > + > > + return &vo->vdev; > > +} > > +EXPORT_SYMBOL_GPL(virtbus_dev_alloc); > > Why have an alloc/add pair of functions? Why not just one? Once the device has been allocated, the consumer has the option of directly changing anything in the newly created struct and associated device before adding it to the bus (e.g. changing parent of device). > > > + > > +static int virtbus_drv_probe(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; > > + } > > + > > + if (vdrv->probe) { > > + ret = vdrv->probe(vdev); > > + if (ret) > > + dev_pm_domain_detach(_dev, true); > > + } > > + > > + return ret; > > +} > > + > > +static int virtbus_drv_remove(struct device *_dev) { > > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > > + int ret = 0; > > + > > + if (vdrv->remove) > > + ret = vdrv->remove(vdev); > > + > > + dev_pm_domain_detach(_dev, true); > > + > > + return ret; > > +} > > + > > +static void virtbus_drv_shutdown(struct device *_dev) { > > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > > + > > + if (vdrv->shutdown) > > + vdrv->shutdown(vdev); > > +} > > + > > +static int virtbus_drv_suspend(struct device *_dev, pm_message_t > > +state) { > > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > > + > > + return vdrv->suspend ? vdrv->suspend(vdev, state) : 0; } > > + > > +static int virtbus_drv_resume(struct device *_dev) { > > + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); > > + struct virtbus_device *vdev = to_virtbus_dev(_dev); > > + > > + return vdrv->resume ? vdrv->resume(vdev) : 0; } > > + > > +/** > > + * virtbus_drv_register - register a driver for virtual bus devices > > + * @vdrv: virtbus_driver structure > > + * @owner: owning module/driver > > + */ > > +int virtbus_drv_register(struct virtbus_driver *vdrv, struct module > > +*owner) > > Don't force someone to type THIS_MODULE for this call, use the macro trick > instead that most subsystems use. Changed to use macro trick. > > Again, I want to see a user, that will cause lots of these types of things to be > painfully obvious :) > > > > +{ > > + vdrv->driver.owner = owner; > > + vdrv->driver.bus = &virtual_bus_type; > > + vdrv->driver.probe = virtbus_drv_probe; > > + vdrv->driver.remove = virtbus_drv_remove; > > + vdrv->driver.shutdown = virtbus_drv_shutdown; > > + vdrv->driver.suspend = virtbus_drv_suspend; > > + vdrv->driver.resume = virtbus_drv_resume; > > + > > + return driver_register(&vdrv->driver); } > > +EXPORT_SYMBOL_GPL(virtbus_drv_register); > > + > > +/** > > + * virtbus_drv_unregister - unregister a driver for virtual bus > > +devices > > + * @drv: virtbus_driver structure > > + */ > > +void virtbus_drv_unregister(struct virtbus_driver *vdrv) { > > + driver_unregister(&vdrv->driver); > > +} > > +EXPORT_SYMBOL_GPL(virtbus_drv_unregister); > > + > > +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) == 0) { > > + vdev->dev_id = id; > > + return id; > > + } > > + id++; > > + } > > + return NULL; > > +} > > + > > +/** > > + * virtbus_match - bind virtbus device to virtbus driver > > + * @dev: device > > + * @drv: driver > > + * > > + * Virtbus device IDs are always in "<name>.<instance>" format. > > + * Instances are automatically selected through an ida_simple_get so > > + * are positive integers. Names are 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. > > + */ > > +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); > > + > > + if (vdrv->id_table) > > + return virtbus_match_id(vdrv->id_table, vdev) != NULL; > > + > > + return (strcmp(vdev->name, drv->name) == 0); } > > + > > +/** > > + * virtbus_probe - call probe of the virtbus_drv > > + * @dev: device struct > > + */ > > +static int virtbus_probe(struct device *dev) { > > + return dev->driver->probe ? dev->driver->probe(dev) : 0; } > > + > > +static int virtbus_remove(struct device *dev) { > > + return dev->driver->remove ? dev->driver->remove(dev) : 0; } > > + > > +static void virtbus_shutdown(struct device *dev) { > > + if (dev->driver->shutdown) > > + 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); > > You have two different styles here with these calls, use this one instead of > the crazy ? : style above in probe/remove please. Style changed to use this one up above. > > > + > > + 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, > > +}; > > +EXPORT_SYMBOL_GPL(virtual_bus_type); > > Why is this exported? Export removed > > > + > > +static int __init virtual_bus_init(void) { > > + int err; > > + > > + err = device_register(&virtual_bus); > > + if (err) { > > + put_device(&virtual_bus); > > + return err; > > + } > > + > > + err = bus_register(&virtual_bus_type); > > + if (err) { > > + device_unregister(&virtual_bus); > > + return err; > > + } > > + > > + pr_debug("Virtual Bus (virtbus) registered with kernel\n"); > > Don't be noisy. And remove your debugging code :) Removed :) > > > > + return err; > > +} > > + > > +static void __exit virtual_bus_exit(void) { > > + bus_unregister(&virtual_bus_type); > > + device_unregister(&virtual_bus); > > +} > > + > > +module_init(virtual_bus_init); > > +module_exit(virtual_bus_exit); > > diff --git a/include/linux/virtual_bus.h b/include/linux/virtual_bus.h > > new file mode 100644 index 000000000000..722f020ac53b > > --- /dev/null > > +++ b/include/linux/virtual_bus.h > > @@ -0,0 +1,64 @@ > > +/* 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> > > + > > +#define VIRTBUS_DEVID_NONE (-1) > > What is this for? Negative values cannot be valid device ids obtained from the IDA method. Using this allows for the consumer to check that the id is >=0 or maybe != VIRTBUS_DEVID_NONE to see if they have a valid virtbus_device. > > > +#define VIRTBUS_NAME_SIZE 20 > > Why? Why is 20 "ok"? Arbitrary value. I wanted something not too big and not too small, so I looked at what platform bus was using and copied them. > > > + > > +struct virtbus_dev_id { > > + char name[VIRTBUS_NAME_SIZE]; > > + u64 driver_data; > > u64 or a pointer? You use both, pick one please. Are you thinking of the void *data in the struct virtbus_device? This u64 element is part of the id_table that the driver will use if needed to be able to match with more than a single device name. I will discuss how this is used below in answering your next question below. > > > +}; > > + > > +/* memory allocation for dev_id is expected to be done by the > > +virtbus_driver > > + * that will match with the virtbus_device, and the matching process > > +will > > + * copy the pointer from the matched element from the driver to the > device. > > What pointer? I don't understand. The matching process can take one of two forms: 1 - fallback matching (no pointer issues in this method) This is a simple do the names match in the driver and device. 2 - id_table matching. If the virtbus driver wants to be able to be matched with more than one virtbus_device, they can supply an id_table that looks like - { name data } { name data } ... In this method, the element in id_table (of struct virtbus_dev_id *) that is matched by name, will be copied to the virtbus_device's dev_id element. That way each different device type can have its own unique driver_data fed back to the driver via its probe. I will make the comment (hopefully) more understandable. > > > + */ > > +struct virtbus_device { > > + const char *name; > > + int id; > > + const struct virtbus_dev_id *dev_id; > > + struct device dev; > > + void *data; > > +}; > > + > > +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 state); > > + int (*resume)(struct virtbus_device *); > > + struct device_driver driver; > > + const struct virtbus_dev_id *id_table; }; > > + > > +#define virtbus_get_dev_id(vdev) ((vdev)->id_entry) > > +#define virtbus_get_devdata(dev) ((dev)->devdata) > > What are these for? > > > +#define dev_is_virtbus(dev) ((dev)->bus == &virtbus_type) > > Who needs this? I was planning on using these when I was doing setup and ended up not using them. Removing. > > > +#define to_virtbus_dev(x) container_of((x), struct virtbus_device, dev) > > +#define to_virtbus_drv(drv) (container_of((drv), struct > virtbus_driver, \ > > + driver)) > > Why are these in a public .h file? Moved to .c file > > > + > > +extern struct bus_type virtual_bus_type; extern struct device > > +virtual_bus; > > Again, why exported? Export removed, extern removed > > > + > > +int virtbus_dev_add(struct virtbus_device *vdev); void > > +virtbus_dev_del(struct virtbus_device *vdev); struct virtbus_device > > +*virtbus_dev_alloc(const char *name, void *devdata); int > > +virtbus_drv_register(struct virtbus_driver *vdrv, struct module > > +*owner); void virtbus_drv_unregister(struct virtbus_driver *vdrv); > > + > > +int virtbus_for_each_dev(void *data, int (*fn)(struct device *, void > > +*)); int virtbus_for_each_drv(void *data, int(*fn)(struct > > +device_driver *, void *)); > > pick a coding style and stick with it please... Opted for add/del since it is shorter to type :) > > thanks, Thank You!! Great feedback! > > greg k-h I have included a small sample driver and device in: tools/testing/selftests/virtual_bus/virtual_bus_[drv|dev]/ These will add their respective elements to the bus and in the probe of the driver, it will access the data from the device in a simplistic way. Also, the rework for the ice driver using the virtbus is done, and going through the process of heading upstream if you need a real world example of how we plan to use this. -Dave E
diff --git a/Documentation/driver-api/virtual_bus.rst b/Documentation/driver-api/virtual_bus.rst new file mode 100644 index 000000000000..72e440464e95 --- /dev/null +++ b/Documentation/driver-api/virtual_bus.rst @@ -0,0 +1,112 @@ +=============================== +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 Structs + Virtual Bus API entry points + +Virtbus devices +~~~~~~~~~~~~~~~ +Virtbus_devices are lightweight objects that support the minimal device +functionality. Devices will accept a name, and then an automatically +generated index is concatenated onto it for the virtbus_device->name. + +The memory backing the "void *data" element of the virtbus_device is +expected to be allocated and freed outside the context of the bus +operations. This memory is also expected to remain viable for the +duration of the time that the virtbus_device is registered to the +virtual bus. (e.g. from before the virtbus_device_add until after +the paired virtbus_dev_del). + +The provided API for virtbus_dev_alloc is an efficient way of allocating +the memory for the virtbus_device (except for the data element) and +automatically freeing it when the device is removed from the bus. + +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_drv_reg +and unregister with virtbus_drv_unreg. + +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) + +This allows for multiple virtbus_devices with the same name, which will all +be matched to the same virtbus_driver. Driver binding is performed by the +driver core, invoking driver probe() after finding a match between device and driver. + +Virtual Bus Structs +~~~~~~~~~~~~~~~~~~~ +struct device virtual_bus = { + .init_name = "virtbus", + .release = virtual_bus_release, +}; + +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, +}; + +struct virtbus_device { + const char *name; + int id; + const struct virtbus_dev_id *dev_id; + struct device dev; + void *data; +}; + +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 state); + int (*resume)(struct virtbus_device *); + struct device_driver driver; +}; + + +Virtual Bus API entry points +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +struct virtbus_device *virtbus_dev_alloc(const char *name, void *data) +int virtbus_dev_add(struct virtbus_device *vdev) +void virtbus_dev_del(struct virtbus_device *vdev) +int virtbus_drv_register(struct virtbus_driver *vdrv, struct module *owner) +void virtbus_drv_unregister(struct virtbus_driver *vdrv) diff --git a/MAINTAINERS b/MAINTAINERS index 3e57fc1d9962..0b21319dd7e8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17353,6 +17353,14 @@ F: include/linux/vbox_utils.h F: include/uapi/linux/vbox*.h F: drivers/virt/vboxguest/ +VIRTUAL BUS DRIVER +M: Jeff Kirsher <jeffrey.t.kirsher@intel.com> +L: intel-wired-lan@lists.osuosl.org +S: Maintained +F: drivers/bus/virtual_bus.c +F: include/linux/virtual_bus.h +F: Documentation/driver-api/virtual_bus.rst + VIRTUAL SERIO DEVICE DRIVER M: Stephen Chandler Paul <thatslyude@gmail.com> S: Maintained diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index 6b331061d34b..30cef35b0c30 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -193,4 +193,18 @@ config DA8XX_MSTPRI source "drivers/bus/fsl-mc/Kconfig" +config VIRTUAL_BUS + tristate "lightweight Virtual Bus" + depends on PM + help + Provides a lightweight bus for virtbus_devices to be added to it + and virtbus_drivers to be registered on it. Will create a match + between the driver and device, then call the driver's probe with + the virtbus_device's struct (including a pointer for data). + 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. The data in the virtbus_device created by the + PCI LAN driver is a set of ops (function pointers) for the irdma + driver to use to register and communicate with the PCI LAN driver. + endmenu diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile index 16b43d3468c6..0b0ba53cbe5b 100644 --- a/drivers/bus/Makefile +++ b/drivers/bus/Makefile @@ -33,3 +33,4 @@ obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o obj-$(CONFIG_DA8XX_MSTPRI) += da8xx-mstpri.o +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..af3f6d9b60f4 --- /dev/null +++ b/drivers/bus/virtual_bus.c @@ -0,0 +1,339 @@ +// 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("Lightweight Virtual Bus"); +MODULE_AUTHOR("David Ertman <david.m.ertman@intel.com>"); +MODULE_AUTHOR("Kiran Patil <kiran.patil@intel.com>"); + +static DEFINE_IDA(virtbus_dev_id); + +static void virtual_bus_release(struct device *dev) +{ + pr_info("Removing virtual bus device.\n"); +} + +struct device virtual_bus = { + .init_name = "virtbus", + .release = virtual_bus_release, +}; +EXPORT_SYMBOL_GPL(virtual_bus); + +/** + * virtbus_add_dev - add a virtual bus device + * @vdev: virtual bus device to add + */ +int virtbus_dev_add(struct virtbus_device *vdev) +{ + int ret; + + if (!vdev) + return -EINVAL; + + device_initialize(&vdev->dev); + if (!vdev->dev.parent) + vdev->dev.parent = &virtual_bus; + + vdev->dev.bus = &virtual_bus_type; + /* All device IDs are automatically allocated */ + ret = ida_simple_get(&virtbus_dev_id, 0, 0, GFP_KERNEL); + if (ret < 0) + return ret; + + vdev->id = ret; + dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id); + + pr_debug("Registering VirtBus device '%s'. Parent at %s\n", + dev_name(&vdev->dev), dev_name(vdev->dev.parent)); + + ret = device_add(&vdev->dev); + if (!ret) + return ret; + + /* Error adding virtual device */ + ida_simple_remove(&virtbus_dev_id, vdev->id); + vdev->id = VIRTBUS_DEVID_NONE; + + return ret; +} +EXPORT_SYMBOL_GPL(virtbus_dev_add); + +/** + * virtbus_dev_del - remove a virtual bus device + * vdev: virtual bus device we are removing + */ +void virtbus_dev_del(struct virtbus_device *vdev) +{ + if (!IS_ERR_OR_NULL(vdev)) { + device_del(&vdev->dev); + + ida_simple_remove(&virtbus_dev_id, vdev->id); + vdev->id = VIRTBUS_DEVID_NONE; + } +} +EXPORT_SYMBOL_GPL(virtbus_dev_del); + +struct virtbus_object { + struct virtbus_device vdev; + char name[]; +}; + +/** + * virtbus_dev_release - Destroy a virtbus device + * @vdev: virtual device to release + * + * Note that the vdev->data which is separately allocated needs to be + * separately freed on it own. + */ +static void virtbus_dev_release(struct device *dev) +{ + struct virtbus_object *vo = container_of(dev, struct virtbus_object, + vdev.dev); + + kfree(vo); +} + +/** + * virtbus_dev_alloc - allocate a virtbus device + * @name: name to associate with the vdev + * @data: pointer to data to be associated with this device + */ +struct virtbus_device *virtbus_dev_alloc(const char *name, void *data) +{ + struct virtbus_object *vo; + + /* Create a virtbus object to contain the vdev and name. This + * avoids a problem with the const attribute of name in the vdev. + * The virtbus_object will be allocated here and freed in the + * release function. + */ + vo = kzalloc(sizeof(*vo) + strlen(name) + 1, GFP_KERNEL); + if (!vo) + return NULL; + + strcpy(vo->name, name); + vo->vdev.name = vo->name; + vo->vdev.data = data; + vo->vdev.dev.release = virtbus_dev_release; + + return &vo->vdev; +} +EXPORT_SYMBOL_GPL(virtbus_dev_alloc); + +static int virtbus_drv_probe(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; + } + + if (vdrv->probe) { + ret = vdrv->probe(vdev); + if (ret) + dev_pm_domain_detach(_dev, true); + } + + return ret; +} + +static int virtbus_drv_remove(struct device *_dev) +{ + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); + struct virtbus_device *vdev = to_virtbus_dev(_dev); + int ret = 0; + + if (vdrv->remove) + ret = vdrv->remove(vdev); + + dev_pm_domain_detach(_dev, true); + + return ret; +} + +static void virtbus_drv_shutdown(struct device *_dev) +{ + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); + struct virtbus_device *vdev = to_virtbus_dev(_dev); + + if (vdrv->shutdown) + vdrv->shutdown(vdev); +} + +static int virtbus_drv_suspend(struct device *_dev, pm_message_t state) +{ + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); + struct virtbus_device *vdev = to_virtbus_dev(_dev); + + return vdrv->suspend ? vdrv->suspend(vdev, state) : 0; +} + +static int virtbus_drv_resume(struct device *_dev) +{ + struct virtbus_driver *vdrv = to_virtbus_drv(_dev->driver); + struct virtbus_device *vdev = to_virtbus_dev(_dev); + + return vdrv->resume ? vdrv->resume(vdev) : 0; +} + +/** + * virtbus_drv_register - register a driver for virtual bus devices + * @vdrv: virtbus_driver structure + * @owner: owning module/driver + */ +int virtbus_drv_register(struct virtbus_driver *vdrv, struct module *owner) +{ + vdrv->driver.owner = owner; + vdrv->driver.bus = &virtual_bus_type; + vdrv->driver.probe = virtbus_drv_probe; + vdrv->driver.remove = virtbus_drv_remove; + vdrv->driver.shutdown = virtbus_drv_shutdown; + vdrv->driver.suspend = virtbus_drv_suspend; + vdrv->driver.resume = virtbus_drv_resume; + + return driver_register(&vdrv->driver); +} +EXPORT_SYMBOL_GPL(virtbus_drv_register); + +/** + * virtbus_drv_unregister - unregister a driver for virtual bus devices + * @drv: virtbus_driver structure + */ +void virtbus_drv_unregister(struct virtbus_driver *vdrv) +{ + driver_unregister(&vdrv->driver); +} +EXPORT_SYMBOL_GPL(virtbus_drv_unregister); + +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) == 0) { + vdev->dev_id = id; + return id; + } + id++; + } + return NULL; +} + +/** + * virtbus_match - bind virtbus device to virtbus driver + * @dev: device + * @drv: driver + * + * Virtbus device IDs are always in "<name>.<instance>" format. + * Instances are automatically selected through an ida_simple_get so + * are positive integers. Names are 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. + */ +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); + + if (vdrv->id_table) + return virtbus_match_id(vdrv->id_table, vdev) != NULL; + + return (strcmp(vdev->name, drv->name) == 0); +} + +/** + * virtbus_probe - call probe of the virtbus_drv + * @dev: device struct + */ +static int virtbus_probe(struct device *dev) +{ + return dev->driver->probe ? dev->driver->probe(dev) : 0; +} + +static int virtbus_remove(struct device *dev) +{ + return dev->driver->remove ? dev->driver->remove(dev) : 0; +} + +static void virtbus_shutdown(struct device *dev) +{ + if (dev->driver->shutdown) + 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, +}; +EXPORT_SYMBOL_GPL(virtual_bus_type); + +static int __init virtual_bus_init(void) +{ + int err; + + err = device_register(&virtual_bus); + if (err) { + put_device(&virtual_bus); + return err; + } + + err = bus_register(&virtual_bus_type); + if (err) { + device_unregister(&virtual_bus); + return err; + } + + pr_debug("Virtual Bus (virtbus) registered with kernel\n"); + return err; +} + +static void __exit virtual_bus_exit(void) +{ + bus_unregister(&virtual_bus_type); + device_unregister(&virtual_bus); +} + +module_init(virtual_bus_init); +module_exit(virtual_bus_exit); diff --git a/include/linux/virtual_bus.h b/include/linux/virtual_bus.h new file mode 100644 index 000000000000..722f020ac53b --- /dev/null +++ b/include/linux/virtual_bus.h @@ -0,0 +1,64 @@ +/* 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> + +#define VIRTBUS_DEVID_NONE (-1) +#define VIRTBUS_NAME_SIZE 20 + +struct virtbus_dev_id { + char name[VIRTBUS_NAME_SIZE]; + u64 driver_data; +}; + +/* memory allocation for dev_id is expected to be done by the virtbus_driver + * that will match with the virtbus_device, and the matching process will + * copy the pointer from the matched element from the driver to the device. + */ +struct virtbus_device { + const char *name; + int id; + const struct virtbus_dev_id *dev_id; + struct device dev; + void *data; +}; + +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 state); + int (*resume)(struct virtbus_device *); + struct device_driver driver; + const struct virtbus_dev_id *id_table; +}; + +#define virtbus_get_dev_id(vdev) ((vdev)->id_entry) +#define virtbus_get_devdata(dev) ((dev)->devdata) +#define dev_is_virtbus(dev) ((dev)->bus == &virtbus_type) +#define to_virtbus_dev(x) container_of((x), struct virtbus_device, dev) +#define to_virtbus_drv(drv) (container_of((drv), struct virtbus_driver, \ + driver)) + +extern struct bus_type virtual_bus_type; +extern struct device virtual_bus; + +int virtbus_dev_add(struct virtbus_device *vdev); +void virtbus_dev_del(struct virtbus_device *vdev); +struct virtbus_device *virtbus_dev_alloc(const char *name, void *devdata); +int virtbus_drv_register(struct virtbus_driver *vdrv, struct module *owner); +void virtbus_drv_unregister(struct virtbus_driver *vdrv); + +int virtbus_for_each_dev(void *data, int (*fn)(struct device *, void *)); +int virtbus_for_each_drv(void *data, int(*fn)(struct device_driver *, void *)); + +#endif /* _VIRTUAL_BUS_H_ */