Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
From: "Ertman, David M" <david.m.ertman@intel.com>
To: "Williams, Dan J" <dan.j.williams@intel.com>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	Takashi Iwai <tiwai@suse.de>, Mark Brown <broonie@kernel.org>,
	linux-rdma <linux-rdma@vger.kernel.org>,
	Jason Gunthorpe <jgg@nvidia.com>,
	Doug Ledford <dledford@redhat.com>,
	Netdev <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Fred Oh <fred.oh@linux.intel.com>,
	Parav Pandit <parav@mellanox.com>,
	"Saleem, Shiraz" <shiraz.saleem@intel.com>,
	"Patil, Kiran" <kiran.patil@intel.com>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	Leon Romanovsky <leonro@nvidia.com>
Subject: RE: [PATCH v3 01/10] Add auxiliary bus support
Date: Thu, 5 Nov 2020 19:27:56 +0000
Message-ID: <DM6PR11MB284191BAA817540E52E4E2C4DDEE0@DM6PR11MB2841.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CAPcyv4i9s=CsO5VJOhPnS77K=bD0LTQ8TUAbhLd+0OmyU8YQ3g@mail.gmail.com>

> -----Original Message-----
> From: Dan Williams <dan.j.williams@intel.com>
> Sent: Thursday, November 5, 2020 1:19 AM
> To: Ertman, David M <david.m.ertman@intel.com>
> Cc: alsa-devel@alsa-project.org; Takashi Iwai <tiwai@suse.de>; Mark Brown
> <broonie@kernel.org>; linux-rdma <linux-rdma@vger.kernel.org>; Jason
> Gunthorpe <jgg@nvidia.com>; Doug Ledford <dledford@redhat.com>;
> Netdev <netdev@vger.kernel.org>; David Miller <davem@davemloft.net>;
> Jakub Kicinski <kuba@kernel.org>; Greg KH <gregkh@linuxfoundation.org>;
> Ranjani Sridharan <ranjani.sridharan@linux.intel.com>; Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com>; Fred Oh <fred.oh@linux.intel.com>;
> Parav Pandit <parav@mellanox.com>; Saleem, Shiraz
> <shiraz.saleem@intel.com>; Patil, Kiran <kiran.patil@intel.com>; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>; Leon Romanovsky
> <leonro@nvidia.com>
> Subject: Re: [PATCH v3 01/10] Add auxiliary bus support
> 
> Some doc fixups, and minor code feedback. Otherwise looks good to me.
> 
> On Thu, Oct 22, 2020 at 5:35 PM Dave Ertman <david.m.ertman@intel.com>
> wrote:
> >
> > Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver.
> > It enables drivers to create an auxiliary_device and bind an
> > auxiliary_driver to it.
> >
> > The bus supports probe/remove shutdown and suspend/resume callbacks.
> > Each auxiliary_device has a unique string based id; driver binds to
> > an auxiliary_device based on this id through the bus.
> >
> > Co-developed-by: Kiran Patil <kiran.patil@intel.com>
> > Signed-off-by: Kiran Patil <kiran.patil@intel.com>
> > Co-developed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> > Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> > Co-developed-by: Fred Oh <fred.oh@linux.intel.com>
> > Signed-off-by: Fred Oh <fred.oh@linux.intel.com>
> > Co-developed-by: Leon Romanovsky <leonro@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Reviewed-by: Shiraz Saleem <shiraz.saleem@intel.com>
> > Reviewed-by: Parav Pandit <parav@mellanox.com>
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> > ---
> >  Documentation/driver-api/auxiliary_bus.rst | 228 ++++++++++++++++++
> >  Documentation/driver-api/index.rst         |   1 +
> >  drivers/base/Kconfig                       |   3 +
> >  drivers/base/Makefile                      |   1 +
> >  drivers/base/auxiliary.c                   | 267 +++++++++++++++++++++
> >  include/linux/auxiliary_bus.h              |  78 ++++++
> >  include/linux/mod_devicetable.h            |   8 +
> >  scripts/mod/devicetable-offsets.c          |   3 +
> >  scripts/mod/file2alias.c                   |   8 +
> >  9 files changed, 597 insertions(+)
> >  create mode 100644 Documentation/driver-api/auxiliary_bus.rst
> >  create mode 100644 drivers/base/auxiliary.c
> >  create mode 100644 include/linux/auxiliary_bus.h
> >
> > diff --git a/Documentation/driver-api/auxiliary_bus.rst
> b/Documentation/driver-api/auxiliary_bus.rst
> > new file mode 100644
> > index 000000000000..500f29692c81
> > --- /dev/null
> > +++ b/Documentation/driver-api/auxiliary_bus.rst
> > @@ -0,0 +1,228 @@
> > +.. SPDX-License-Identifier: GPL-2.0-only
> > +
> > +=============
> > +Auxiliary Bus
> > +=============
> > +
> > +In some subsystems, the functionality of the core device
> (PCI/ACPI/other) is
> > +too complex for a single device to be managed as a monolithic block or a
> part of
> > +the functionality needs to be exposed to a different subsystem.
> 
> I think there are three identified use cases for this now, so call out
> those examples for others to compare if aux bus is a fit for their use
> case.
> 
> "In some subsystems, the functionality of the core device
> (PCI/ACPI/other) is too complex to be handled by a monolithic driver
> (e.g. Sound Open Firmware), multiple devices might implement a common
> intersection of functionality (e.g. NICs + RDMA), or a driver may want
> to export an interface for another subsystem to drive (e.g. SIOV
> Physical Function export Virtual Function management)."
>

Additional example added.  Thanks for the review Dan!
 
> > + Splitting the
> > +functionality into smaller orthogonal devices would make it easier to
> manage
> > +data, power management and domain-specific interaction with the
> hardware.
> 
> Passive voice and I don't understand what is meant by the word "orthogonal"
> 
> "A split of the functionality into child-devices representing
> sub-domains of functionality makes it possible to compartmentalize,
> layer, and distribute domain-specific concerns via a Linux
> device-driver model"
>

verbiage changed.

> > A key
> > +requirement for such a split is that there is no dependency on a physical
> bus,
> > +device, register accesses or regmap support.
> > These individual devices split from
> > +the core cannot live on the platform bus as they are not physical devices
> that
> > +are controlled by DT/ACPI. The same argument applies for not using MFD
> in this
> > +scenario as MFD relies on individual function devices being physical
> devices.
> 
> These last few sentences are confusing the justification of the
> existence of auxiliary bus, not the explanation of when why and how to
> use it.
> 
> The "When Should the Auxiliary Bus Be Used" should mention where
> Auxiliary bus fits relative to the platform-bus and MFD, perhaps in an
> explicit "When not to use Auxiliary Bus" section to direct people to
> platform-bus and MFD when those are a better fit.
> 

Moved this content into the "When to use" section.

> > +
> > +An example for this kind of requirement is the audio subsystem where a
> single
> > +IP is handling multiple entities such as HDMI, Soundwire, local devices
> such as
> > +mics/speakers etc. The split for the core's functionality can be arbitrary or
> > +be defined by the DSP firmware topology and include hooks for
> test/debug. This
> > +allows for the audio core device to be minimal and focused on hardware-
> specific
> > +control and communication.
> > +
> > +The auxiliary bus is intended to be minimal, generic and avoid domain-
> specific
> > +assumptions.
> 
> As this file will be sitting in Documentation/ it will be too late to
> "intend" it either does and or it doesn't. This again feels more like
> justification for existence that should already be in the changelog,
> it can go from the Documentation.
> 

removed the sentence.

> > Each auxiliary_device represents a part of its parent
> > +functionality. The generic behavior can be extended and specialized as
> needed
> > +by encapsulating an auxiliary_device within other domain-specific
> structures and
> > +the use of .ops callbacks. Devices on the auxiliary bus do not share any
> > +structures and the use of a communication channel with the parent is
> > +domain-specific.
> 
> Should there be any guidance here on when to use ops and when to just
> export functions from parent driver to child. EXPORT_SYMBOL_NS() seems
> a perfect fit for publishing shared routines between parent and child.
> 

I would leave this up the driver writers to determine what is best for them.

> > +
> > +When Should the Auxiliary Bus Be Used
> > +=====================================
> > +
> > +The auxiliary bus is to be used when a driver and one or more kernel
> modules,
> > +who share a common header file with the driver, need a mechanism to
> connect and
> > +provide access to a shared object allocated by the auxiliary_device's
> > +registering driver.  The registering driver for the auxiliary_device(s) and
> the
> > +kernel module(s) registering auxiliary_drivers can be from the same
> subsystem,
> > +or from multiple subsystems.
> > +
> > +The emphasis here is on a common generic interface that keeps
> subsystem
> > +customization out of the bus infrastructure.
> > +
> > +One example could be a multi-port PCI network device that is rdma-
> capable and
> 
> s/could be/is/
> s/multi-port//
> s/rdma-capable/RDMA-capable/
> 

Changes made

> > +needs to export this functionality and attach to an rdma driver in another
> > +subsystem.
> 
> "exports a child device to be driven by an auxiliary_driver in the
> RDMA subsystem"
> 

Change made

> >  The PCI driver will allocate and register an auxiliary_device for
> 
> Fix tense confusion:
> 
> s/will allocate and register/allocates and registers/
> 

Change made.

> > +each physical function on the NIC.  The rdma driver will register an
> 
> s/rdma/RDMA/
> s/will register/registers/
> 

Change made

> > +auxiliary_driver that will be matched with and probed for each of these
> 
> s/that will be matched with and probed for/that claims/
> 

Change made

> > +auxiliary_devices.  This will give the rdma driver access to the shared
> data/ops
> > +in the PCI drivers shared object to establish a connection with the PCI
> driver.
> 
> How about?
> 
> This conveys data/ops published by the parent PCI device/driver to the
> RDMA auxiliary_driver.
> 

Change made

> > +
> > +Another use case is for the PCI device to be split out into multiple sub
> > +functions.  For each sub function an auxiliary_device will be created.  A PCI
> > +sub function driver will bind to such devices that will create its own one or
> > +more class devices.  A PCI sub function auxiliary device will likely be
> > +contained in a struct with additional attributes such as user defined sub
> > +function number and optional attributes such as resources and a link to
> the
> > +parent device.  These attributes could be used by systemd/udev; and
> hence should
> > +be initialized before a driver binds to an auxiliary_device.
> 
> This does not read like an explicit example like the previous 2. Did
> you have something specific in mind?
> 

This was added by request of Parav.

> Here's where the "when not to" / "MFD platform-bus" redirect section can
> go.
> 

Content moved to this location

> > +
> > +Auxiliary Device
> > +================
> > +
> > +An auxiliary_device is created and registered to represent a part of its
> parent
> 
> s/created and registered to represent/represents/
> 

Change made.

> > +device's functionality. It is given a name that, combined with the
> registering
> > +drivers KBUILD_MODNAME, creates a match_name that is used for driver
> binding,
> > +and an id that combined with the match_name provide a unique name to
> register
> > +with the bus subsystem.
> > +
> > +Registering an auxiliary_device is a two-step process.  First you must call
> 
> Imperative implied:
> 
> s/you must//
> 

Removed.

> 
> > +auxiliary_device_init(), which will check several aspects of the
> > +auxiliary_device struct and perform a device_initialize().  After this step
> > +completes, any error state must have a call to auxiliary_device_unin() in
> its
> > +resolution path.  The second step in registering an auxiliary_device is to
> > +perform a call to auxiliary_device_add(), which will set the name of the
> device
> > +and add the device to the bus.
> > +
> > +Unregistering an auxiliary_device is also a two-step process to mirror the
> > +register process.  First will be a call to auxiliary_device_delete(), then
> 
> s/will be a call to/call/
> 

changed

> > +followed by a call to auxiliary_device_unin().
> 
> s/followed by a call to/call/
> 

changed. also fixed typo s/device_unin/device_uninit/

> > +
> > +.. code-block:: c
> > +
> > +       struct auxiliary_device {
> > +               struct device dev;
> > +                const char *name;
> > +               u32 id;
> > +       };
> > +
> > +If two auxiliary_devices both with a match_name "mod.foo" are
> registered onto
> > +the bus, they must have unique id values (e.g. "x" and "y") so that the
> > +registered devices names will be "mod.foo.x" and "mod.foo.y".  If
> match_name +
> > +id are not unique, then the device_add will fail and generate an error
> message.
> > +
> > +The auxiliary_device.dev.type.release or auxiliary_device.dev.release
> must be
> > +populated with a non-NULL pointer to successfully register the
> auxiliary_device.
> > +
> > +The auxiliary_device.dev.parent must also be populated.
> > +
> > +Auxiliary Device Memory Model and Lifespan
> > +------------------------------------------
> > +
> > +When a kernel driver registers an auxiliary_device on the auxiliary bus, we
> will
> > +use the nomenclature to refer to this kernel driver as a registering driver.
> 
> Kill this sentence, it adds nothing and has a dreaded "we". Just say:
> 
> The registering driver is the entity...

Killed sentence and changed.

> 
> > +is the entity that will allocate memory for the auxiliary_device and register
> it
> > +on the auxiliary bus.  It is important to note that, as opposed to the
> platform
> > +bus, the registering driver is wholly responsible for the management for
> the
> > +memory used for the driver object.
> > +
> > +A parent object, defined in the shared header file, will contain the
> 
> Another "will", and more below. Convert all to present tense please.
> 

Changed many instances of will.

> > +auxiliary_device.  It will also contain a pointer to the shared object(s),
> which
> > +will also be defined in the shared header.  Both the parent object and the
> > +shared object(s) will be allocated by the registering driver.  This layout
> > +allows the auxiliary_driver's registering module to perform a
> container_of()
> > +call to go from the pointer to the auxiliary_device, that is passed during
> the
> > +call to the auxiliary_driver's probe function, up to the parent object, and
> then
> > +have access to the shared object(s).
> > +
> > +The memory for the auxiliary_device will be freed only in its release()
> > +callback flow as defined by its registering driver.
> > +
> > +The memory for the shared object(s) must have a lifespan equal to, or
> greater
> > +than, the lifespan of the memory for the auxiliary_device.  The
> auxiliary_driver
> > +should only consider that this shared object is valid as long as the
> > +auxiliary_device is still registered on the auxiliary bus.  It is up to the
> > +registering driver to manage (e.g. free or keep available) the memory for
> the
> > +shared object beyond the life of the auxiliary_device.
> > +
> > +Registering driver must unregister all auxiliary devices before its
> registering
> > +parent device's remove() is completed.
> 
> Too many "registerings"
> 
> The registering driver must unregister all auxiliary devices before
> its own driver.remove() callback is completed.
> 

Changed.

> > +
> > +Auxiliary Drivers
> > +=================
> > +
> > +Auxiliary drivers follow the standard driver model convention, where
> > +discovery/enumeration is handled by the core, and drivers
> > +provide probe() and remove() methods. They support power
> management
> > +and shutdown notifications using the standard conventions.
> > +
> > +.. code-block:: c
> > +
> > +       struct auxiliary_driver {
> > +               int (*probe)(struct auxiliary_device *,
> > +                             const struct auxiliary_device_id *id);
> > +               int (*remove)(struct auxiliary_device *);
> > +               void (*shutdown)(struct auxiliary_device *);
> > +               int (*suspend)(struct auxiliary_device *, pm_message_t);
> > +               int (*resume)(struct auxiliary_device *);
> > +               struct device_driver driver;
> > +               const struct auxiliary_device_id *id_table;
> > +       };
> > +
> > +Auxiliary drivers register themselves with the bus by calling
> > +auxiliary_driver_register(). The id_table contains the match_names of
> auxiliary
> > +devices that a driver can bind with.
> > +
> > +Example Usage
> > +=============
> > +
> > +Auxiliary devices are created and registered by a subsystem-level core
> device
> > +that needs to break up its functionality into smaller fragments. One way
> to
> > +extend the scope of an auxiliary_device would be to encapsulate it within
> a
> 
> More passive tense
> 
> s/would be/is/
> 
Changed.

> > +domain-specific structure defined by the parent device. This structure
> contains
> > +the auxiliary_device and any associated shared data/callbacks needed to
> > +establish the connection with the parent.
> > +
> > +An example would be:
> 
> s/would be/is/
> 

Changed.

> > +
> > +.. code-block:: c
> > +
> > +        struct foo {
> > +               struct auxiliary_device auxdev;
> > +               void (*connect)(struct auxiliary_device *auxdev);
> > +               void (*disconnect)(struct auxiliary_device *auxdev);
> > +               void *data;
> > +        };
> > +
> > +The parent device would then register the auxiliary_device by calling
> 
> again with the passive...
> 

Changed.  Also changed the one below.

> > +auxiliary_device_init(), and then auxiliary_device_add(), with the pointer
> to
> > +the auxdev member of the above structure. The parent would provide a
> name for
> > +the auxiliary_device that, combined with the parent's
> KBUILD_MODNAME, will
> > +create a match_name that will be used for matching and binding with a
> driver.
> > +
> > +Whenever an auxiliary_driver is registered, based on the match_name,
> the
> > +auxiliary_driver's probe() is invoked for the matching devices.  The
> > +auxiliary_driver can also be encapsulated inside custom drivers that make
> the
> > +core device's functionality extensible by adding additional domain-specific
> ops
> > +as follows:
> > +
> > +.. code-block:: c
> > +
> > +       struct my_ops {
> > +               void (*send)(struct auxiliary_device *auxdev);
> > +               void (*receive)(struct auxiliary_device *auxdev);
> > +       };
> > +
> > +
> > +       struct my_driver {
> > +               struct auxiliary_driver auxiliary_drv;
> > +               const struct my_ops ops;
> > +       };
> > +
> > +An example of this type of usage would be:
> 
> *is
> 

Changed

> > +
> > +.. code-block:: c
> > +
> > +       const struct auxiliary_device_id my_auxiliary_id_table[] = {
> > +               { .name = "foo_mod.foo_dev" },
> > +               { },
> > +       };
> > +
> > +       const struct my_ops my_custom_ops = {
> > +               .send = my_tx,
> > +               .receive = my_rx,
> > +       };
> > +
> > +       const struct my_driver my_drv = {
> > +               .auxiliary_drv = {
> > +                       .name = "myauxiliarydrv",
> > +                       .id_table = my_auxiliary_id_table,
> > +                       .probe = my_probe,
> > +                       .remove = my_remove,
> > +                       .shutdown = my_shutdown,
> > +               },
> > +               .ops = my_custom_ops,
> > +       };
> > diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-
> api/index.rst
> > index 987d6e74ea6a..af206dc816ca 100644
> > --- a/Documentation/driver-api/index.rst
> > +++ b/Documentation/driver-api/index.rst
> > @@ -72,6 +72,7 @@ available subsections can be seen below.
> >     thermal/index
> >     fpga/index
> >     acpi/index
> > +   auxiliary_bus
> >     backlight/lp855x-driver.rst
> >     connector
> >     console
> > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> > index 8d7001712062..040be48ce046 100644
> > --- a/drivers/base/Kconfig
> > +++ b/drivers/base/Kconfig
> > @@ -1,6 +1,9 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  menu "Generic Driver Options"
> >
> > +config AUXILIARY_BUS
> > +       bool
> 
> tristate? Unless you need non-exported symbols, might as well let this
> be a module.
> 

As per Leon's response - leaving this as bool.

> > +
> >  config UEVENT_HELPER
> >         bool "Support for uevent helper"
> >         help
> > diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> > index 41369fc7004f..5e7bf9669a81 100644
> > --- a/drivers/base/Makefile
> > +++ b/drivers/base/Makefile
> > @@ -7,6 +7,7 @@ obj-y                   := component.o core.o bus.o dd.o
> syscore.o \
> >                            attribute_container.o transport_class.o \
> >                            topology.o container.o property.o cacheinfo.o \
> >                            swnode.o
> > +obj-$(CONFIG_AUXILIARY_BUS) += auxiliary.o
> >  obj-$(CONFIG_DEVTMPFS) += devtmpfs.o
> >  obj-y                  += power/
> >  obj-$(CONFIG_ISA_BUS_API)      += isa.o
> > diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
> > new file mode 100644
> > index 000000000000..b7c66785352e
> > --- /dev/null
> > +++ b/drivers/base/auxiliary.c
> > @@ -0,0 +1,267 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Software based bus for Auxiliary devices
> 
> I'd just remove this line, it doesn't add anything.
> 

Removed.

> > + *
> > + * Copyright (c) 2019-2020 Intel Corporation
> > + *
> > + * Please see Documentation/driver-api/auxiliary_bus.rst for more
> information.
> > + */
> > +
> > +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__
> > +
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/string.h>
> > +#include <linux/auxiliary_bus.h>
> > +
> > +static const struct auxiliary_device_id *auxiliary_match_id(const struct
> auxiliary_device_id *id,
> > +                                                           const struct auxiliary_device *auxdev)
> > +{
> > +       for (; id->name[0]; id++) {
> > +               const char *p = strrchr(dev_name(&auxdev->dev), '.');
> > +               int match_size;
> > +
> > +               if (!p)
> > +                       continue;
> > +               match_size = p - dev_name(&auxdev->dev);
> > +
> > +               /* use dev_name(&auxdev->dev) prefix before last '.' char to
> match to */
> > +               if (strlen(id->name) == match_size &&
> > +                   !strncmp(dev_name(&auxdev->dev), id->name, match_size))
> > +                       return id;
> > +       }
> > +       return NULL;
> > +}
> > +
> > +static int auxiliary_match(struct device *dev, struct device_driver *drv)
> > +{
> > +       struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +       struct auxiliary_driver *auxdrv = to_auxiliary_drv(drv);
> > +
> > +       return !!auxiliary_match_id(auxdrv->id_table, auxdev);
> > +}
> > +
> > +static int auxiliary_uevent(struct device *dev, struct kobj_uevent_env
> *env)
> > +{
> > +       const char *name, *p;
> > +
> > +       name = dev_name(dev);
> > +       p = strrchr(name, '.');
> > +
> > +       return add_uevent_var(env, "MODALIAS=%s%.*s",
> AUXILIARY_MODULE_PREFIX, (int)(p - name),
> > +                             name);
> > +}
> > +
> > +static const struct dev_pm_ops auxiliary_dev_pm_ops = {
> > +       SET_RUNTIME_PM_OPS(pm_generic_runtime_suspend,
> pm_generic_runtime_resume, NULL)
> > +       SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend,
> pm_generic_resume)
> > +};
> > +
> > +static int auxiliary_bus_probe(struct device *dev)
> > +{
> > +       struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver);
> > +       struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +       int ret;
> > +
> > +       ret = dev_pm_domain_attach(dev, true);
> > +       if (ret) {
> > +               dev_warn(dev, "Failed to attach to PM Domain : %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table,
> auxdev));
> > +       if (ret)
> > +               dev_pm_domain_detach(dev, true);
> > +
> > +       return ret;
> > +}
> > +
> > +static int auxiliary_bus_remove(struct device *dev)
> > +{
> > +       struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver);
> > +       struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +       int ret = 0;
> > +
> > +       if (auxdrv->remove)
> > +               ret = auxdrv->remove(auxdev);
> > +       dev_pm_domain_detach(dev, true);
> > +
> > +       return ret;
> > +}
> > +
> > +static void auxiliary_bus_shutdown(struct device *dev)
> > +{
> > +       struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver);
> > +       struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +
> > +       if (auxdrv->shutdown)
> > +               auxdrv->shutdown(auxdev);
> > +}
> > +
> > +static struct bus_type auxiliary_bus_type = {
> > +       .name = "auxiliary",
> > +       .probe = auxiliary_bus_probe,
> > +       .remove = auxiliary_bus_remove,
> > +       .shutdown = auxiliary_bus_shutdown,
> > +       .match = auxiliary_match,
> > +       .uevent = auxiliary_uevent,
> > +       .pm = &auxiliary_dev_pm_ops,
> > +};
> > +
> > +/**
> > + * auxiliary_device_init - check auxiliary_device and initialize
> > + * @auxdev: auxiliary device struct
> > + *
> > + * This is the first step in the two-step process to register an
> auxiliary_device.
> > + *
> > + * When this function returns an error code, then the device_initialize will
> *not* have
> > + * been performed, and the caller will be responsible to free any memory
> allocated for the
> > + * auxiliary_device in the error path directly.
> > + *
> > + * It returns 0 on success.  On success, the device_initialize has been
> performed.  After this
> > + * point any error unwinding will need to include a call to
> auxiliary_device_init().
> 
> Whoops, you meant to say auxiliary_device_uninit().
> 

yikes - yes I did!  Changed.

> > + * In this post-initialize error scenario, a call to the device's .release
> callback will be
> > + * triggered by auxiliary_device_uninit(), and all memory clean-up is
> expected to be
> 
> with this function already mentioned above you can drop "by
> auxiliary_device_uninit()"
> 

dropped.

> > + * handled there.
> > + */
> > +int auxiliary_device_init(struct auxiliary_device *auxdev)
> > +{
> > +       struct device *dev = &auxdev->dev;
> > +
> > +       if (!dev->parent) {
> > +               pr_err("auxiliary_device has a NULL dev->parent\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (!auxdev->name) {
> > +               pr_err("auxiliary_device has a NULL name\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       dev->bus = &auxiliary_bus_type;
> > +       device_initialize(&auxdev->dev);
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(auxiliary_device_init);
> > +
> > +/**
> > + * __auxiliary_device_add - add an auxiliary bus device
> > + * @auxdev: auxiliary bus device to add to the bus
> > + * @modname: name of the parent device's driver module
> > + *
> > + * This is the second step in the two-step process to register an
> auxiliary_device.
> > + *
> > + * This function must be called after a successful call to
> auxiliary_device_init(), which
> > + * will perform the device_initialize.  This means that if this returns an
> error code, then a
> > + * call to auxiliary_device_uninit() must be performed so that the .release
> callback will
> > + * be triggered to free the memory associated with the auxiliary_device.
> 
> Might want to mention the typical expectation is that users call
> auxiliary_device_add without underbars. Only if custom names are
> needed would this direct version be used.
> 

Added in verbiage to that effect.

> > + */
> > +int __auxiliary_device_add(struct auxiliary_device *auxdev, const char
> *modname)
> > +{
> > +       struct device *dev = &auxdev->dev;
> > +       int ret;
> > +
> > +       if (!modname) {
> > +               pr_err("auxiliary device modname is NULL\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = dev_set_name(dev, "%s.%s.%d", modname, auxdev->name,
> auxdev->id);
> > +       if (ret) {
> > +               pr_err("auxiliary device dev_set_name failed: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ret = device_add(dev);
> > +       if (ret)
> > +               dev_err(dev, "adding auxiliary device failed!: %d\n", ret);
> > +
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(__auxiliary_device_add);
> > +
> > +/**
> > + * auxiliary_find_device - auxiliary device iterator for locating a particular
> device.
> > + * @start: Device to begin with
> > + * @data: Data to pass to match function
> > + * @match: Callback function to check device
> > + *
> > + * This function returns a reference to a device that is 'found'
> > + * for later use, as determined by the @match callback.
> > + *
> > + * The callback should return 0 if the device doesn't match and non-zero
> > + * if it does.  If the callback returns non-zero, this function will
> > + * return to the caller and not iterate over any more devices.
> > + */
> > +struct auxiliary_device *
> > +auxiliary_find_device(struct device *start, const void *data,
> > +                     int (*match)(struct device *dev, const void *data))
> > +{
> > +       struct device *dev;
> > +
> > +       dev = bus_find_device(&auxiliary_bus_type, start, data, match);
> > +       if (!dev)
> > +               return NULL;
> > +
> > +       return to_auxiliary_dev(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(auxiliary_find_device);
> > +
> > +/**
> > + * __auxiliary_driver_register - register a driver for auxiliary bus devices
> > + * @auxdrv: auxiliary_driver structure
> > + * @owner: owning module/driver
> > + * @modname: KBUILD_MODNAME for parent driver
> > + */
> > +int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct
> module *owner,
> > +                               const char *modname)
> > +{
> > +       if (WARN_ON(!auxdrv->probe) || WARN_ON(!auxdrv->id_table))
> > +               return -EINVAL;
> > +
> > +       if (auxdrv->name)
> > +               auxdrv->driver.name = kasprintf(GFP_KERNEL, "%s.%s",
> modname, auxdrv->name);
> > +       else
> > +               auxdrv->driver.name = kasprintf(GFP_KERNEL, "%s", modname);
> > +       if (!auxdrv->driver.name)
> > +               return -ENOMEM;
> > +
> > +       auxdrv->driver.owner = owner;
> > +       auxdrv->driver.bus = &auxiliary_bus_type;
> > +       auxdrv->driver.mod_name = modname;
> > +
> > +       return driver_register(&auxdrv->driver);
> > +}
> > +EXPORT_SYMBOL_GPL(__auxiliary_driver_register);
> > +
> > +/**
> > + * auxiliary_driver_unregister - unregister a driver
> > + * @auxdrv: auxiliary_driver structure
> > + */
> > +void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv)
> > +{
> > +       driver_unregister(&auxdrv->driver);
> > +       kfree(auxdrv->driver.name);
> > +}
> > +EXPORT_SYMBOL_GPL(auxiliary_driver_unregister);
> > +
> > +static int __init auxiliary_bus_init(void)
> > +{
> > +       return bus_register(&auxiliary_bus_type);
> > +}
> > +
> > +static void __exit auxiliary_bus_exit(void)
> > +{
> > +       bus_unregister(&auxiliary_bus_type);
> > +}
> > +
> > +module_init(auxiliary_bus_init);
> > +module_exit(auxiliary_bus_exit);
> > +
> > +MODULE_LICENSE("GPL");
> 
> Per above SPDX is v2 only, so...
> 
> MODULE_LICENSE("GPL v2");
> 

added v2.

> > +MODULE_DESCRIPTION("Auxiliary Bus");
> > +MODULE_AUTHOR("David Ertman <david.m.ertman@intel.com>");
> > +MODULE_AUTHOR("Kiran Patil <kiran.patil@intel.com>");
> > diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
> > new file mode 100644
> > index 000000000000..282fbf7bf9af
> > --- /dev/null
> > +++ b/include/linux/auxiliary_bus.h
> > @@ -0,0 +1,78 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2019-2020 Intel Corporation
> > + *
> > + * Please see Documentation/driver-api/auxiliary_bus.rst for more
> information.
> > + */
> > +
> > +#ifndef _AUXILIARY_BUS_H_
> > +#define _AUXILIARY_BUS_H_
> > +
> > +#include <linux/device.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/slab.h>
> > +
> > +struct auxiliary_device {
> > +       struct device dev;
> > +       const char *name;
> 
> I'd say name this "suffix" since it is only a component of the name.
> 

This is a mandatory field though, and suffix lends itself to be considered optional.  Also, name is
more intuitive in its meaning than suffix would be.

> > +       u32 id;
> > +};
> > +
> > +struct auxiliary_driver {
> > +       int (*probe)(struct auxiliary_device *auxdev, const struct
> auxiliary_device_id *id);
> > +       int (*remove)(struct auxiliary_device *auxdev);
> > +       void (*shutdown)(struct auxiliary_device *auxdev);
> > +       int (*suspend)(struct auxiliary_device *auxdev, pm_message_t
> state);
> > +       int (*resume)(struct auxiliary_device *auxdev);
> > +       const char *name;
> > +       struct device_driver driver;
> > +       const struct auxiliary_device_id *id_table;
> > +};
> > +
> > +static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev)
> > +{
> > +       return container_of(dev, struct auxiliary_device, dev);
> > +}
> > +
> > +static inline struct auxiliary_driver *to_auxiliary_drv(struct device_driver
> *drv)
> > +{
> > +       return container_of(drv, struct auxiliary_driver, driver);
> > +}
> > +
> > +int auxiliary_device_init(struct auxiliary_device *auxdev);
> > +int __auxiliary_device_add(struct auxiliary_device *auxdev, const char
> *modname);
> > +#define auxiliary_device_add(auxdev) __auxiliary_device_add(auxdev,
> KBUILD_MODNAME)
> > +
> > +static inline void auxiliary_device_uninit(struct auxiliary_device *auxdev)
> > +{
> > +       put_device(&auxdev->dev);
> > +}
> > +
> > +static inline void auxiliary_device_delete(struct auxiliary_device *auxdev)
> > +{
> > +       device_del(&auxdev->dev);
> > +}
> > +
> > +int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct
> module *owner,
> > +                               const char *modname);
> > +#define auxiliary_driver_register(auxdrv) \
> > +       __auxiliary_driver_register(auxdrv, THIS_MODULE,
> KBUILD_MODNAME)
> > +
> > +void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv);
> > +
> > +/**
> > + * module_auxiliary_driver() - Helper macro for registering an auxiliary
> driver
> > + * @__auxiliary_driver: auxiliary driver struct
> > + *
> > + * Helper macro for auxiliary drivers which do not do anything special in
> > + * module init/exit. This eliminates a lot of boilerplate. Each module may
> only
> > + * use this macro once, and calling it replaces module_init() and
> module_exit()
> > + */
> > +#define module_auxiliary_driver(__auxiliary_driver) \
> > +       module_driver(__auxiliary_driver, auxiliary_driver_register,
> auxiliary_driver_unregister)
> > +
> > +struct auxiliary_device *
> > +auxiliary_find_device(struct device *start, const void *data,
> > +                     int (*match)(struct device *dev, const void *data));
> > +
> > +#endif /* _AUXILIARY_BUS_H_ */
> > diff --git a/include/linux/mod_devicetable.h
> b/include/linux/mod_devicetable.h
> > index 5b08a473cdba..c425290b21e2 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -838,4 +838,12 @@ struct mhi_device_id {
> >         kernel_ulong_t driver_data;
> >  };
> >
> > +#define AUXILIARY_NAME_SIZE 32
> > +#define AUXILIARY_MODULE_PREFIX "auxiliary:"
> > +
> > +struct auxiliary_device_id {
> > +       char name[AUXILIARY_NAME_SIZE];
> > +       kernel_ulong_t driver_data;
> > +};
> > +
> >  #endif /* LINUX_MOD_DEVICETABLE_H */
> > diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-
> offsets.c
> > index 27007c18e754..e377f52dbfa3 100644
> > --- a/scripts/mod/devicetable-offsets.c
> > +++ b/scripts/mod/devicetable-offsets.c
> > @@ -243,5 +243,8 @@ int main(void)
> >         DEVID(mhi_device_id);
> >         DEVID_FIELD(mhi_device_id, chan);
> >
> > +       DEVID(auxiliary_device_id);
> > +       DEVID_FIELD(auxiliary_device_id, name);
> > +
> >         return 0;
> >  }
> > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > index 2417dd1dee33..fb4827027536 100644
> > --- a/scripts/mod/file2alias.c
> > +++ b/scripts/mod/file2alias.c
> > @@ -1364,6 +1364,13 @@ static int do_mhi_entry(const char *filename,
> void *symval, char *alias)
> >  {
> >         DEF_FIELD_ADDR(symval, mhi_device_id, chan);
> >         sprintf(alias, MHI_DEVICE_MODALIAS_FMT, *chan);
> > +       return 1;
> > +}
> > +
> > +static int do_auxiliary_entry(const char *filename, void *symval, char
> *alias)
> > +{
> > +       DEF_FIELD_ADDR(symval, auxiliary_device_id, name);
> > +       sprintf(alias, AUXILIARY_MODULE_PREFIX "%s", *name);
> >
> >         return 1;
> >  }
> > @@ -1442,6 +1449,7 @@ static const struct devtable devtable[] = {
> >         {"tee", SIZE_tee_client_device_id, do_tee_entry},
> >         {"wmi", SIZE_wmi_device_id, do_wmi_entry},
> >         {"mhi", SIZE_mhi_device_id, do_mhi_entry},
> > +       {"auxiliary", SIZE_auxiliary_device_id, do_auxiliary_entry},
> >  };
> >
> >  /* Create MODULE_ALIAS() statements.
> > --
> > 2.26.2
> >

Again, thanks for the review Dan.  Changes will be in next release (v4) once I give
stake-holders a little time to respond.

-DaveE

  parent reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-23  0:33 [PATCH v3 00/10] Auxiliary bus implementation and SOF multi-client support Dave Ertman
2020-10-23  0:33 ` [PATCH v3 01/10] Add auxiliary bus support Dave Ertman
2020-11-05  9:19   ` Dan Williams
2020-11-05  9:47     ` Leon Romanovsky
2020-11-05 17:12       ` Dan Williams
2020-11-05 19:30         ` Leon Romanovsky
2020-11-05 20:27           ` Dan Williams
2020-11-05 20:22         ` Greg KH
2020-11-05 20:24           ` Dan Williams
2020-11-05 19:27     ` Ertman, David M [this message]
2020-11-05 19:32       ` Pierre-Louis Bossart
2020-11-05 19:37         ` Leon Romanovsky
2020-11-05 19:35       ` Leon Romanovsky
2020-11-05 20:52         ` Ertman, David M
2020-11-05 19:40       ` Parav Pandit
2020-11-05 20:26         ` Dan Williams
2020-11-05 20:37           ` Parav Pandit
2020-11-06 19:35             ` Mark Brown
2020-11-10  7:23               ` Oded Gabbay
2020-11-05 22:00       ` Dan Williams
2020-11-05 23:48         ` Ertman, David M
2020-11-13 15:50   ` Greg KH
2020-11-13 16:07     ` Ertman, David M
2020-11-13 23:21       ` Greg KH
2020-11-15  6:48         ` Leon Romanovsky
2020-10-23  0:33 ` [PATCH v3 02/10] ASoC: SOF: Introduce descriptors for SOF client Dave Ertman
2020-10-23  0:33 ` [PATCH v3 03/10] ASoC: SOF: Create client driver for IPC test Dave Ertman
2020-10-23  0:33 ` [PATCH v3 04/10] ASoC: SOF: ops: Add ops for client registration Dave Ertman
2020-10-23  0:33 ` [PATCH v3 05/10] ASoC: SOF: Intel: Define " Dave Ertman
2020-10-23  0:33 ` [PATCH v3 06/10] ASoC: SOF: Intel: Remove IPC flood test support in SOF core Dave Ertman
2020-10-23  0:33 ` [PATCH v3 07/10] ASoC: SOF: sof-client: Add client APIs to access probes ops Dave Ertman
2020-10-23  0:33 ` [PATCH v3 08/10] ASoC: SOF: compress: move and export sof_probe_compr_ops Dave Ertman
2020-10-23  0:33 ` [PATCH v3 09/10] ASoC: SOF: Add new client driver for probes support Dave Ertman
2020-10-23  0:33 ` [PATCH v3 10/10] ASoC: SOF: Intel: CNL: register probes client Dave Ertman
2020-10-23  6:49 ` [PATCH v3 00/10] Auxiliary bus implementation and SOF multi-client support Leon Romanovsky
2020-10-23  6:56   ` Greg KH
2020-10-23 15:48     ` Leon Romanovsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM6PR11MB284191BAA817540E52E4E2C4DDEE0@DM6PR11MB2841.namprd11.prod.outlook.com \
    --to=david.m.ertman@intel.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=dan.j.williams@intel.com \
    --cc=davem@davemloft.net \
    --cc=dledford@redhat.com \
    --cc=fred.oh@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jgg@nvidia.com \
    --cc=kiran.patil@intel.com \
    --cc=kuba@kernel.org \
    --cc=leonro@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=parav@mellanox.com \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=ranjani.sridharan@linux.intel.com \
    --cc=shiraz.saleem@intel.com \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git