Message-ID: <2B0E3F215D1AB84DA946C8BEE234CCC97B3008C1@ORSMSX101.amr.corp.intel.com>
Date:   Fri, 15 Nov 2019 21:17:41 +0000
From:   "Ertman, David M" <david.m.ertman@...el.com>
To:     'Greg KH' <gregkh@...uxfoundation.org>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "parav@...lanox.com" <parav@...lanox.com>,
        "jgg@...pe.ca" <jgg@...pe.ca>,
        "Patil, Kiran" <kiran.patil@...el.com>
Subject: RE: [net-next 1/1] virtual-bus: Implementation of Virtual Bus

> -----Original Message-----
> From: Greg KH <gregkh@...uxfoundation.org>
> Sent: Tuesday, November 12, 2019 1:28 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>
> Cc: davem@...emloft.net; Ertman, David M <david.m.ertman@...el.com>;
> netdev@...r.kernel.org; linux-rdma@...r.kernel.org;
> nhorman@...hat.com; sassmann@...hat.com; parav@...lanox.com;
> jgg@...pe.ca; Patil, Kiran <kiran.patil@...el.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@...el.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@...el.com>
> > Signed-off-by: Kiran Patil <kiran.patil@...el.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.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@...el.com>"); MODULE_AUTHOR("Kiran Patil
> > +<kiran.patil@...el.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

Powered by blists - more mailing lists