Re: [net-next 1/9] Implementation of Virtual Bus

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Apr 20, 2020 at 10:59:12PM +0000, Ertman, David M wrote:
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Saturday, April 18, 2020 5:51 AM
> > 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; jgg@ziepe.ca; parav@mellanox.com;
> > galpress@amazon.com; selvin.xavier@broadcom.com;
> > sriharsha.basavapatna@broadcom.com; benve@cisco.com;
> > bharat@chelsio.com; xavier.huwei@huawei.com; yishaih@mellanox.com;
> > leonro@mellanox.com; mkalderon@marvell.com; aditr@vmware.com;
> > ranjani.sridharan@linux.intel.com; pierre-louis.bossart@linux.intel.com; Patil,
> > Kiran <kiran.patil@intel.com>; Bowers, AndrewX <andrewx.bowers@intel.com>
> > Subject: Re: [net-next 1/9] Implementation of Virtual Bus
> > 
> > On Fri, Apr 17, 2020 at 10:10:26AM -0700, Jeff Kirsher wrote:
> > > @@ -0,0 +1,53 @@
> > > +/* 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>
> > > +
> > > +struct virtbus_device {
> > > +	struct device dev;
> > > +	const char *name;
> > 
> > struct device already has a name, why do you need another one?
> 
> The name in dev is the base name appended with the id to make sure each device
> has unique name.  The name in vdev is the abbreviated one (without the id) which
> will be used in the matching function, so that a driver can claim to support
> <name> and will be matched with all <name>.<id> devices for all id's.
> 
> This is similar logic to platform_device's name field.

Don't treat platform_device as a good example of much :)

I still think this is duplicated stuff, but I'll let it go for now...

> > > +	void (*release)(struct virtbus_device *);
> > 
> > A bus should have the release function, not the actual device itself.  A
> > device should not need function pointers.
> > 
> 
> The bus does have a release function, but it is a wrapper to call the release defined by
> the device.

odd.  That is normally handled by the bus, not by the device itself.

> This is where the KO registering the virtbus_device is expected to clean up
> the resources allocated for this device (e.g. free memory, etc).  Having the virtual_bus_release
> call a release callback in the virtual_device allows for extra cleanup from the originating KO
> if necessary.
> 
> The memory model of virtual bus is for the originating KO to manage the lifespan of the
> memory for the virtual_device.  The virtual_bus expects the KO defining the virtbus_device
> have the memory allocated before registering a virtbus_device and to clean up that memory
> when the release is called.
> 
> The platform_device also has function pointers in it, by including a MFD object, but the
> platform_bus is managing the memory for the platform_bus_object that contains the
> platform_device which it why it using a generic kref_put to free memory.

Again, platform_devices are not good things to emulate, they have grown
into a total mess.

Ok, given that you are going to be putting lots of different things on
this "generic" type of bus, a release function for the device can make
sense.  Still feels odd, I wonder if you should just do something with
the type of the device instead.

thanks,

greg k-h