* Re: [net-next 1/1] virtual-bus: Implementation of Virtual Bus
2019-11-12 21:28 ` Greg KH
@ 2019-11-13 0:08 ` Jason Gunthorpe
2019-11-13 1:03 ` Parav Pandit
2019-11-13 1:09 ` Ertman, David M
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2019-11-13 0:08 UTC (permalink / raw)
To: Greg KH
Cc: Jeff Kirsher, davem, Dave Ertman, netdev, linux-rdma, nhorman,
sassmann, parav, Kiran Patil
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [net-next 1/1] virtual-bus: Implementation of Virtual Bus
2019-11-13 0:08 ` Jason Gunthorpe
@ 2019-11-13 1:03 ` Parav Pandit
2019-11-13 1:10 ` Jason Gunthorpe
0 siblings, 1 reply; 11+ messages in thread
From: Parav Pandit @ 2019-11-13 1:03 UTC (permalink / raw)
To: Jason Gunthorpe, Greg KH
Cc: Jeff Kirsher, davem, Dave Ertman, netdev, linux-rdma, nhorman,
sassmann, Kiran Patil
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 */
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [net-next 1/1] virtual-bus: Implementation of Virtual Bus
2019-11-13 1:03 ` Parav Pandit
@ 2019-11-13 1:10 ` Jason Gunthorpe
2019-11-13 6:44 ` Parav Pandit
0 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2019-11-13 1:10 UTC (permalink / raw)
To: Parav Pandit
Cc: Greg KH, Jeff Kirsher, davem, Dave Ertman, netdev, linux-rdma,
nhorman, sassmann, Kiran Patil
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [net-next 1/1] virtual-bus: Implementation of Virtual Bus
2019-11-13 1:10 ` Jason Gunthorpe
@ 2019-11-13 6:44 ` Parav Pandit
0 siblings, 0 replies; 11+ messages in thread
From: Parav Pandit @ 2019-11-13 6:44 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Greg KH, Jeff Kirsher, davem, Dave Ertman, netdev, linux-rdma,
nhorman, sassmann, Kiran Patil
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [net-next 1/1] virtual-bus: Implementation of Virtual Bus
2019-11-12 21:28 ` Greg KH
2019-11-13 0:08 ` Jason Gunthorpe
@ 2019-11-13 1:09 ` Ertman, David M
2019-11-13 7:03 ` Parav Pandit
2019-11-15 21:17 ` Ertman, David M
3 siblings, 0 replies; 11+ messages in thread
From: Ertman, David M @ 2019-11-13 1:09 UTC (permalink / raw)
To: Greg KH, Kirsher, Jeffrey T
Cc: davem, netdev, linux-rdma, nhorman, sassmann, parav, jgg, Patil, Kiran
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [net-next 1/1] virtual-bus: Implementation of Virtual Bus
2019-11-12 21:28 ` Greg KH
2019-11-13 0:08 ` Jason Gunthorpe
2019-11-13 1:09 ` Ertman, David M
@ 2019-11-13 7:03 ` Parav Pandit
2019-11-15 21:17 ` Ertman, David M
3 siblings, 0 replies; 11+ messages in thread
From: Parav Pandit @ 2019-11-13 7:03 UTC (permalink / raw)
To: Greg KH, Jeff Kirsher
Cc: davem, Dave Ertman, netdev, linux-rdma, nhorman, sassmann, jgg,
Kiran Patil
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [net-next 1/1] virtual-bus: Implementation of Virtual Bus
2019-11-12 21:28 ` Greg KH
` (2 preceding siblings ...)
2019-11-13 7:03 ` Parav Pandit
@ 2019-11-15 21:17 ` Ertman, David M
3 siblings, 0 replies; 11+ messages in thread
From: Ertman, David M @ 2019-11-15 21:17 UTC (permalink / raw)
To: 'Greg KH', Kirsher, Jeffrey T
Cc: davem, netdev, linux-rdma, nhorman, sassmann, parav, jgg, Patil, Kiran
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread