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
