On Mon, Apr 20, 2020 at 10:59:12PM +0000, Ertman, David M wrote:
> > -----Original Message-----
> > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Sent: Saturday, April 18, 2020 5:51 AM
> > 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; jgg@xxxxxxxx; parav@xxxxxxxxxxxx;
> > galpress@xxxxxxxxxx; selvin.xavier@xxxxxxxxxxxx;
> > sriharsha.basavapatna@xxxxxxxxxxxx; benve@xxxxxxxxx;
> > bharat@xxxxxxxxxxx; xavier.huwei@xxxxxxxxxx; yishaih@xxxxxxxxxxxx;
> > leonro@xxxxxxxxxxxx; mkalderon@xxxxxxxxxxx; aditr@xxxxxxxxxx;
> > ranjani.sridharan@xxxxxxxxxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx; Patil,
> > Kiran <kiran.patil@xxxxxxxxx>; Bowers, AndrewX <andrewx.bowers@xxxxxxxxx>
> > 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
