> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, November 12, 2019 1:28 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@xxxxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; Ertman, David M <david.m.ertman@xxxxxxxxx>;
> netdev@xxxxxxxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx;
> nhorman@xxxxxxxxxx; sassmann@xxxxxxxxxx; parav@xxxxxxxxxxxx;
> jgg@xxxxxxxx; Patil, Kiran <kiran.patil@xxxxxxxxx>
> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Kiran Patil <kiran.patil@xxxxxxxxx>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@xxxxxxxxx>
>
> 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@xxxxxxxxx>"); MODULE_AUTHOR("Kiran Patil
> > +<kiran.patil@xxxxxxxxx>");
> > +
> > +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
