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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 21 Apr 2020 08:52:23 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     "Ertman, David M" <david.m.ertman@...el.com>
Cc:     "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "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>,
        "jgg@...pe.ca" <jgg@...pe.ca>,
        "parav@...lanox.com" <parav@...lanox.com>,
        "galpress@...zon.com" <galpress@...zon.com>,
        "selvin.xavier@...adcom.com" <selvin.xavier@...adcom.com>,
        "sriharsha.basavapatna@...adcom.com" 
        <sriharsha.basavapatna@...adcom.com>,
        "benve@...co.com" <benve@...co.com>,
        "bharat@...lsio.com" <bharat@...lsio.com>,
        "xavier.huwei@...wei.com" <xavier.huwei@...wei.com>,
        "yishaih@...lanox.com" <yishaih@...lanox.com>,
        "leonro@...lanox.com" <leonro@...lanox.com>,
        "mkalderon@...vell.com" <mkalderon@...vell.com>,
        "aditr@...are.com" <aditr@...are.com>,
        "ranjani.sridharan@...ux.intel.com" 
        <ranjani.sridharan@...ux.intel.com>,
        "pierre-louis.bossart@...ux.intel.com" 
        <pierre-louis.bossart@...ux.intel.com>,
        "Patil, Kiran" <kiran.patil@...el.com>,
        "Bowers, AndrewX" <andrewx.bowers@...el.com>
Subject: Re: [net-next 1/9] Implementation of Virtual Bus

On Mon, Apr 20, 2020 at 10:59:12PM +0000, Ertman, David M wrote:
> > -----Original Message-----
> > From: Greg KH <gregkh@...uxfoundation.org>
> > Sent: Saturday, April 18, 2020 5:51 AM
> > 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; jgg@...pe.ca; parav@...lanox.com;
> > galpress@...zon.com; selvin.xavier@...adcom.com;
> > sriharsha.basavapatna@...adcom.com; benve@...co.com;
> > bharat@...lsio.com; xavier.huwei@...wei.com; yishaih@...lanox.com;
> > leonro@...lanox.com; mkalderon@...vell.com; aditr@...are.com;
> > ranjani.sridharan@...ux.intel.com; pierre-louis.bossart@...ux.intel.com; Patil,
> > Kiran <kiran.patil@...el.com>; Bowers, AndrewX <andrewx.bowers@...el.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