[v3,01/10] Add auxiliary bus support

Commit Message

Ertman, David M Oct. 23, 2020, 12:33 a.m. UTC

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 <[e-mail]>
Signed-off-by: Kiran Patil <[e-mail]>
Co-developed-by: Ranjani Sridharan <[e-mail]>
Signed-off-by: Ranjani Sridharan <[e-mail]>
Co-developed-by: Fred Oh <[e-mail]>
Signed-off-by: Fred Oh <[e-mail]>
Co-developed-by: Leon Romanovsky <[e-mail]>
Signed-off-by: Leon Romanovsky <[e-mail]>
Reviewed-by: Pierre-Louis Bossart <[e-mail]>
Reviewed-by: Shiraz Saleem <[e-mail]>
Reviewed-by: Parav Pandit <[e-mail]>
Reviewed-by: Dan Williams <[e-mail]>
Signed-off-by: Dave Ertman <[e-mail]>
---
 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

Comments

Dan Williams Nov. 5, 2020, 9:19 a.m. UTC | #1

Some doc fixups, and minor code feedback. Otherwise looks good to me.

On Thu, Oct 22, 2020 at 5:35 PM Dave Ertman <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Kiran Patil <[e-mail]>
> Co-developed-by: Ranjani Sridharan <[e-mail]>
> Signed-off-by: Ranjani Sridharan <[e-mail]>
> Co-developed-by: Fred Oh <[e-mail]>
> Signed-off-by: Fred Oh <[e-mail]>
> Co-developed-by: Leon Romanovsky <[e-mail]>
> Signed-off-by: Leon Romanovsky <[e-mail]>
> Reviewed-by: Pierre-Louis Bossart <[e-mail]>
> Reviewed-by: Shiraz Saleem <[e-mail]>
> Reviewed-by: Parav Pandit <[e-mail]>
> Reviewed-by: Dan Williams <[e-mail]>
> Signed-off-by: Dave Ertman <[e-mail]>
> ---
>  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)."

> + 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"

> 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.

> +
> +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.

> 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.

> +
> +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/

> +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"

>  The PCI driver will allocate and register an auxiliary_device for

Fix tense confusion:

s/will allocate and register/allocates and registers/

> +each physical function on the NIC.  The rdma driver will register an

s/rdma/RDMA/
s/will register/registers/

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

s/that will be matched with and probed for/that claims/

> +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.

> +
> +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?

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

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

s/created and registered to represent/represents/

> +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//


> +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/

> +followed by a call to auxiliary_device_unin().

s/followed by a call to/call/

> +
> +.. 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...

> +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.

> +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.

> +
> +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/

> +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/

> +
> +.. 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...

> +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

> +
> +.. 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.

> +
>  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.

> + *
> + * 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().

> + * 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()"

> + * 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.

> + */
> +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");

> +MODULE_DESCRIPTION("Auxiliary Bus");
> +MODULE_AUTHOR("David Ertman <[e-mail]>");
> +MODULE_AUTHOR("Kiran Patil <[e-mail]>");
> 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.

> +       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
>

Leon Romanovsky Nov. 5, 2020, 9:47 a.m. UTC | #2

On Thu, Nov 05, 2020 at 01:19:09AM -0800, Dan Williams wrote:
> Some doc fixups, and minor code feedback. Otherwise looks good to me.
>
> On Thu, Oct 22, 2020 at 5:35 PM Dave Ertman <[e-mail]> wrote:
> >

<...>

> >
> > +config AUXILIARY_BUS
> > +       bool
>
> tristate? Unless you need non-exported symbols, might as well let this
> be a module.

I asked it to be "bool", because bus as a module is an invitation for
a disaster. For example if I compile-in mlx5 which is based on this bus,
and won't add auxiliary_bus as a module to initramfs, the system won't boot.

<...>

>
> Per above SPDX is v2 only, so...

Isn't it default for the Linux kernel?

>
> MODULE_LICENSE("GPL v2");

Thanks

Dan Williams Nov. 5, 2020, 5:12 p.m. UTC | #3

On Thu, Nov 5, 2020 at 1:47 AM Leon Romanovsky <[e-mail]> wrote:
>
> On Thu, Nov 05, 2020 at 01:19:09AM -0800, Dan Williams wrote:
> > Some doc fixups, and minor code feedback. Otherwise looks good to me.
> >
> > On Thu, Oct 22, 2020 at 5:35 PM Dave Ertman <[e-mail]> wrote:
> > >
>
> <...>
>
> > >
> > > +config AUXILIARY_BUS
> > > +       bool
> >
> > tristate? Unless you need non-exported symbols, might as well let this
> > be a module.
>
> I asked it to be "bool", because bus as a module is an invitation for
> a disaster. For example if I compile-in mlx5 which is based on this bus,
> and won't add auxiliary_bus as a module to initramfs, the system won't boot.

Something is broken if module dependencies don't arrange for
auxiliary_bus.ko to be added to the initramfs automatically, but yes,
it is another degree of freedom for something to go wrong if you build
the initramfs by hand.

>
> <...>
>
> >
> > Per above SPDX is v2 only, so...
>
> Isn't it default for the Linux kernel?

SPDX eliminated the need to guess a default, and MODULE_LICENSE("GPL")
implies the "or later" language. The only default assumption is that
the license is GPL v2 compatible, those possibilities are myriad, but
v2-only is the first preference.

Ertman, David M Nov. 5, 2020, 7:27 p.m. UTC | #4

> -----Original Message-----
> From: Dan Williams <[e-mail]>
> Sent: Thursday, November 5, 2020 1:19 AM
> To: Ertman, David M <[e-mail]>
> Cc: [e-mail]; Takashi Iwai <[e-mail]>; Mark Brown
> <[e-mail]>; linux-rdma <[e-mail]>; Jason
> Gunthorpe <[e-mail]>; Doug Ledford <[e-mail]>;
> Netdev <[e-mail]>; David Miller <[e-mail]>;
> Jakub Kicinski <[e-mail]>; Greg KH <[e-mail]>;
> Ranjani Sridharan <[e-mail]>; Pierre-Louis Bossart
> <[e-mail]>; Fred Oh <[e-mail]>;
> Parav Pandit <[e-mail]>; Saleem, Shiraz
> <[e-mail]>; Patil, Kiran <[e-mail]>; Linux
> Kernel Mailing List <[e-mail]>; Leon Romanovsky
> <[e-mail]>
> 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 <[e-mail]>
> 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 <[e-mail]>
> > Signed-off-by: Kiran Patil <[e-mail]>
> > Co-developed-by: Ranjani Sridharan <[e-mail]>
> > Signed-off-by: Ranjani Sridharan <[e-mail]>
> > Co-developed-by: Fred Oh <[e-mail]>
> > Signed-off-by: Fred Oh <[e-mail]>
> > Co-developed-by: Leon Romanovsky <[e-mail]>
> > Signed-off-by: Leon Romanovsky <[e-mail]>
> > Reviewed-by: Pierre-Louis Bossart <[e-mail]>
> > Reviewed-by: Shiraz Saleem <[e-mail]>
> > Reviewed-by: Parav Pandit <[e-mail]>
> > Reviewed-by: Dan Williams <[e-mail]>
> > Signed-off-by: Dave Ertman <[e-mail]>
> > ---
> >  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 <[e-mail]>");
> > +MODULE_AUTHOR("Kiran Patil <[e-mail]>");
> > 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

Leon Romanovsky Nov. 5, 2020, 7:30 p.m. UTC | #5

On Thu, Nov 05, 2020 at 09:12:51AM -0800, Dan Williams wrote:
> On Thu, Nov 5, 2020 at 1:47 AM Leon Romanovsky <[e-mail]> wrote:
> >
> > On Thu, Nov 05, 2020 at 01:19:09AM -0800, Dan Williams wrote:
> > > Some doc fixups, and minor code feedback. Otherwise looks good to me.
> > >
> > > On Thu, Oct 22, 2020 at 5:35 PM Dave Ertman <[e-mail]> wrote:
> > > >
> >
> > <...>
> >
> > > >
> > > > +config AUXILIARY_BUS
> > > > +       bool
> > >
> > > tristate? Unless you need non-exported symbols, might as well let this
> > > be a module.
> >
> > I asked it to be "bool", because bus as a module is an invitation for
> > a disaster. For example if I compile-in mlx5 which is based on this bus,
> > and won't add auxiliary_bus as a module to initramfs, the system won't boot.
>
> Something is broken if module dependencies don't arrange for
> auxiliary_bus.ko to be added to the initramfs automatically, but yes,
> it is another degree of freedom for something to go wrong if you build
> the initramfs by hand.

And this is something that I would like to avoid for now.

>
> >
> > <...>
> >
> > >
> > > Per above SPDX is v2 only, so...
> >
> > Isn't it default for the Linux kernel?
>
> SPDX eliminated the need to guess a default, and MODULE_LICENSE("GPL")
> implies the "or later" language. The only default assumption is that
> the license is GPL v2 compatible, those possibilities are myriad, but
> v2-only is the first preference.

I mean that plain GPL == GPL v2 in the kernel.

Thanks

Pierre-Louis Bossart Nov. 5, 2020, 7:32 p.m. UTC | #6

>>> +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.

"GPL v2" is the same as "GPL" here, it does not have any additional meaning.

https://www.kernel.org/doc/html/latest/process/license-rules.html

“GPL”	Module is licensed under GPL version 2. This does not express any 
distinction between GPL-2.0-only or GPL-2.0-or-later. The exact license 
information can only be determined via the license information in the 
corresponding source files.

“GPL v2”	Same as “GPL”. It exists for historic reasons.

Leon Romanovsky Nov. 5, 2020, 7:35 p.m. UTC | #7

On Thu, Nov 05, 2020 at 07:27:56PM +0000, Ertman, David M wrote:
> > -----Original Message-----
> > From: Dan Williams <[e-mail]>
> > Sent: Thursday, November 5, 2020 1:19 AM
> > To: Ertman, David M <[e-mail]>
> > Cc: [e-mail]; Takashi Iwai <[e-mail]>; Mark Brown
> > <[e-mail]>; linux-rdma <[e-mail]>; Jason
> > Gunthorpe <[e-mail]>; Doug Ledford <[e-mail]>;
> > Netdev <[e-mail]>; David Miller <[e-mail]>;
> > Jakub Kicinski <[e-mail]>; Greg KH <[e-mail]>;
> > Ranjani Sridharan <[e-mail]>; Pierre-Louis Bossart
> > <[e-mail]>; Fred Oh <[e-mail]>;
> > Parav Pandit <[e-mail]>; Saleem, Shiraz
> > <[e-mail]>; Patil, Kiran <[e-mail]>; Linux
> > Kernel Mailing List <[e-mail]>; Leon Romanovsky
> > <[e-mail]>
> > 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 <[e-mail]>
> > wrote:

<...>

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

Everything here can go as a Fixes, the review comments are valuable and need
to be fixed, but they don't change anything dramatically that prevent from
merging v3.

Thanks

>
> -DaveE

Leon Romanovsky Nov. 5, 2020, 7:37 p.m. UTC | #8

On Thu, Nov 05, 2020 at 01:32:40PM -0600, Pierre-Louis Bossart wrote:
>
> > > > +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.
>
> "GPL v2" is the same as "GPL" here, it does not have any additional meaning.
>
> https://www.kernel.org/doc/html/latest/process/license-rules.html
>
> “GPL”	Module is licensed under GPL version 2. This does not express any
> distinction between GPL-2.0-only or GPL-2.0-or-later. The exact license
> information can only be determined via the license information in the
> corresponding source files.

+1,
https://lore.kernel.org/lkml/20201105193009.GA5475@unreal

>
> “GPL v2”	Same as “GPL”. It exists for historic reasons.
>

Parav Pandit Nov. 5, 2020, 7:40 p.m. UTC | #9

> From: Ertman, David M <[e-mail]>
> Sent: Friday, November 6, 2020 12:58 AM
> Subject: RE: [PATCH v3 01/10] Add auxiliary bus support
> 
> > -----Original Message-----
> > From: Dan Williams <[e-mail]>
> > Sent: Thursday, November 5, 2020 1:19 AM
> >

[..]
> > > +
> > > +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.
> 
This example describes the mlx5 PCI subfunction use case.
I didn't follow your question about 'explicit example'.
What part is missing to identify it as explicit example?

Greg KH Nov. 5, 2020, 8:22 p.m. UTC | #10

On Thu, Nov 05, 2020 at 09:12:51AM -0800, Dan Williams wrote:
> > >
> > > Per above SPDX is v2 only, so...
> >
> > Isn't it default for the Linux kernel?
> 
> SPDX eliminated the need to guess a default, and MODULE_LICENSE("GPL")
> implies the "or later" language. The only default assumption is that
> the license is GPL v2 compatible, those possibilities are myriad, but
> v2-only is the first preference.

No, MODULE_LICENSE("GPL") does not imply "or later" at all.  Please see
include/linux/module.h, it means "GPL version 2".

thanks,

greg k-h

Dan Williams Nov. 5, 2020, 8:24 p.m. UTC | #11

On Thu, Nov 5, 2020 at 12:21 PM Greg KH <[e-mail]> wrote:
>
> On Thu, Nov 05, 2020 at 09:12:51AM -0800, Dan Williams wrote:
> > > >
> > > > Per above SPDX is v2 only, so...
> > >
> > > Isn't it default for the Linux kernel?
> >
> > SPDX eliminated the need to guess a default, and MODULE_LICENSE("GPL")
> > implies the "or later" language. The only default assumption is that
> > the license is GPL v2 compatible, those possibilities are myriad, but
> > v2-only is the first preference.
>
> No, MODULE_LICENSE("GPL") does not imply "or later" at all.  Please see
> include/linux/module.h, it means "GPL version 2".
>

Oh, I did, and stopped before getting to:

  "only/or later" distinction is completely irrelevant and does neither
 *replace the proper license identifiers in the corresponding source file

...sorry for the noise.

Dan Williams Nov. 5, 2020, 8:26 p.m. UTC | #12

On Thu, Nov 5, 2020 at 11:40 AM Parav Pandit <[e-mail]> wrote:
>
>
>
> > From: Ertman, David M <[e-mail]>
> > Sent: Friday, November 6, 2020 12:58 AM
> > Subject: RE: [PATCH v3 01/10] Add auxiliary bus support
> >
> > > -----Original Message-----
> > > From: Dan Williams <[e-mail]>
> > > Sent: Thursday, November 5, 2020 1:19 AM
> > >
>
> [..]
> > > > +
> > > > +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.
> >
> This example describes the mlx5 PCI subfunction use case.
> I didn't follow your question about 'explicit example'.
> What part is missing to identify it as explicit example?

Specifically listing "mlx5" so if someone reading this document thinks
to themselves "hey mlx5 sounds like my use case" they can go grep for
that.

Dan Williams Nov. 5, 2020, 8:27 p.m. UTC | #13

On Thu, Nov 5, 2020 at 11:30 AM Leon Romanovsky <[e-mail]> wrote:
>
> On Thu, Nov 05, 2020 at 09:12:51AM -0800, Dan Williams wrote:
> > On Thu, Nov 5, 2020 at 1:47 AM Leon Romanovsky <[e-mail]> wrote:
> > >
> > > On Thu, Nov 05, 2020 at 01:19:09AM -0800, Dan Williams wrote:
> > > > Some doc fixups, and minor code feedback. Otherwise looks good to me.
> > > >
> > > > On Thu, Oct 22, 2020 at 5:35 PM Dave Ertman <[e-mail]> wrote:
> > > > >
> > >
> > > <...>
> > >
> > > > >
> > > > > +config AUXILIARY_BUS
> > > > > +       bool
> > > >
> > > > tristate? Unless you need non-exported symbols, might as well let this
> > > > be a module.
> > >
> > > I asked it to be "bool", because bus as a module is an invitation for
> > > a disaster. For example if I compile-in mlx5 which is based on this bus,
> > > and won't add auxiliary_bus as a module to initramfs, the system won't boot.
> >
> > Something is broken if module dependencies don't arrange for
> > auxiliary_bus.ko to be added to the initramfs automatically, but yes,
> > it is another degree of freedom for something to go wrong if you build
> > the initramfs by hand.
>
> And this is something that I would like to avoid for now.

Fair enough.

>
> >
> > >
> > > <...>
> > >
> > > >
> > > > Per above SPDX is v2 only, so...
> > >
> > > Isn't it default for the Linux kernel?
> >
> > SPDX eliminated the need to guess a default, and MODULE_LICENSE("GPL")
> > implies the "or later" language. The only default assumption is that
> > the license is GPL v2 compatible, those possibilities are myriad, but
> > v2-only is the first preference.
>
> I mean that plain GPL == GPL v2 in the kernel.

You are right, I was wrong.

Parav Pandit Nov. 5, 2020, 8:37 p.m. UTC | #14

> From: Dan Williams <[e-mail]>
> Sent: Friday, November 6, 2020 1:56 AM
> 
> On Thu, Nov 5, 2020 at 11:40 AM Parav Pandit <[e-mail]> wrote:
> >
> >
> >
> > > From: Ertman, David M <[e-mail]>
> > > Sent: Friday, November 6, 2020 12:58 AM
> > > Subject: RE: [PATCH v3 01/10] Add auxiliary bus support
> > >
> > > > -----Original Message-----
> > > > From: Dan Williams <[e-mail]>
> > > > Sent: Thursday, November 5, 2020 1:19 AM
> > > >
> >
> > [..]
> > > > > +
> > > > > +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.
> > >
> > This example describes the mlx5 PCI subfunction use case.
> > I didn't follow your question about 'explicit example'.
> > What part is missing to identify it as explicit example?
> 
> Specifically listing "mlx5" so if someone reading this document thinks to
> themselves "hey mlx5 sounds like my use case" they can go grep for that.
Ah, I see.
"mlx5" is not listed explicitly, because it is not included in this patchset.
In various previous discussions in this thread, mlx5 subfunction use case is described that justifies the existence of the bus.
I will be happy to update this documentation once mlx5 subfunction will be part of kernel so that grep actually shows valid output.
(waiting to post them as it uses auxiliary bus :-)).

Ertman, David M Nov. 5, 2020, 8:52 p.m. UTC | #15

> -----Original Message-----
> From: Leon Romanovsky <[e-mail]>
> Sent: Thursday, November 5, 2020 11:35 AM
> To: Ertman, David M <[e-mail]>
> Cc: Williams, Dan J <[e-mail]>; [e-mail];
> Takashi Iwai <[e-mail]>; Mark Brown <[e-mail]>; linux-
> rdma <[e-mail]>; Jason Gunthorpe <[e-mail]>;
> Doug Ledford <[e-mail]>; Netdev <[e-mail]>;
> David Miller <[e-mail]>; Jakub Kicinski <[e-mail]>;
> Greg KH <[e-mail]>; Ranjani Sridharan
> <[e-mail]>; Pierre-Louis Bossart <pierre-
> [e-mail]>; Fred Oh <[e-mail]>; Parav
> Pandit <[e-mail]>; Saleem, Shiraz <[e-mail]>;
> Patil, Kiran <[e-mail]>; Linux Kernel Mailing List <linux-
> [e-mail]>
> Subject: Re: [PATCH v3 01/10] Add auxiliary bus support
> 
> On Thu, Nov 05, 2020 at 07:27:56PM +0000, Ertman, David M wrote:
> > > -----Original Message-----
> > > From: Dan Williams <[e-mail]>
> > > Sent: Thursday, November 5, 2020 1:19 AM
> > > To: Ertman, David M <[e-mail]>
> > > Cc: [e-mail]; Takashi Iwai <[e-mail]>; Mark
> Brown
> > > <[e-mail]>; linux-rdma <[e-mail]>; Jason
> > > Gunthorpe <[e-mail]>; Doug Ledford <[e-mail]>;
> > > Netdev <[e-mail]>; David Miller
> <[e-mail]>;
> > > Jakub Kicinski <[e-mail]>; Greg KH
> <[e-mail]>;
> > > Ranjani Sridharan <[e-mail]>; Pierre-Louis
> Bossart
> > > <[e-mail]>; Fred Oh
> <[e-mail]>;
> > > Parav Pandit <[e-mail]>; Saleem, Shiraz
> > > <[e-mail]>; Patil, Kiran <[e-mail]>; Linux
> > > Kernel Mailing List <[e-mail]>; Leon Romanovsky
> > > <[e-mail]>
> > > 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
> <[e-mail]>
> > > wrote:
> 
> <...>
> 
> >
> > Again, thanks for the review Dan.  Changes will be in next release (v4) once
> I give
> > stake-holders a little time to respond.
> 
> Everything here can go as a Fixes, the review comments are valuable and
> need
> to be fixed, but they don't change anything dramatically that prevent from
> merging v3.
> 

This works for me - I have the changes saved into an add-on patch that I haven't
squashed into the main patch yet.

> Thanks
> 
> >
> > -DaveE

Dan Williams Nov. 5, 2020, 10 p.m. UTC | #16

On Thu, Nov 5, 2020 at 11:28 AM Ertman, David M
<[e-mail]> wrote:
[..]
> > > 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.

I think there is a pathological case that can be avoided with a
statement like the following:

"Note that ops are intended as a way to augment instance behavior
within a class of auxiliary devices, it is not the mechanism for
exporting common infrastructure from the parent. Consider
EXPORT_SYMBOL_NS() to convey infrastructure from the parent module to
the auxiliary module(s)."

As for your other dispositions of the feedback, looks good to me.

Ertman, David M Nov. 5, 2020, 11:48 p.m. UTC | #17

> -----Original Message-----
> From: Dan Williams <[e-mail]>
> Sent: Thursday, November 5, 2020 2:00 PM
> To: Ertman, David M <[e-mail]>
> Cc: [e-mail]; Takashi Iwai <[e-mail]>; Mark Brown
> <[e-mail]>; linux-rdma <[e-mail]>; Jason
> Gunthorpe <[e-mail]>; Doug Ledford <[e-mail]>;
> Netdev <[e-mail]>; David Miller <[e-mail]>;
> Jakub Kicinski <[e-mail]>; Greg KH <[e-mail]>;
> Ranjani Sridharan <[e-mail]>; Pierre-Louis Bossart
> <[e-mail]>; Fred Oh <[e-mail]>;
> Parav Pandit <[e-mail]>; Saleem, Shiraz
> <[e-mail]>; Patil, Kiran <[e-mail]>; Linux
> Kernel Mailing List <[e-mail]>; Leon Romanovsky
> <[e-mail]>
> Subject: Re: [PATCH v3 01/10] Add auxiliary bus support
> 
> On Thu, Nov 5, 2020 at 11:28 AM Ertman, David M
> <[e-mail]> wrote:
> [..]
> > > > 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.
> 
> I think there is a pathological case that can be avoided with a
> statement like the following:
> 
> "Note that ops are intended as a way to augment instance behavior
> within a class of auxiliary devices, it is not the mechanism for
> exporting common infrastructure from the parent. Consider
> EXPORT_SYMBOL_NS() to convey infrastructure from the parent module to
> the auxiliary module(s)."
> 
> As for your other dispositions of the feedback, looks good to me.

I will add this in.

-DaveE

Mark Brown Nov. 6, 2020, 7:35 p.m. UTC | #18

On Thu, Nov 05, 2020 at 08:37:14PM +0000, Parav Pandit wrote:

> > > This example describes the mlx5 PCI subfunction use case.
> > > I didn't follow your question about 'explicit example'.
> > > What part is missing to identify it as explicit example?

> > Specifically listing "mlx5" so if someone reading this document thinks to
> > themselves "hey mlx5 sounds like my use case" they can go grep for that.

> Ah, I see.
> "mlx5" is not listed explicitly, because it is not included in this patchset.
> In various previous discussions in this thread, mlx5 subfunction use case is described that justifies the existence of the bus.
> I will be happy to update this documentation once mlx5 subfunction will be part of kernel so that grep actually shows valid output.
> (waiting to post them as it uses auxiliary bus :-)).

For ease of review if there's a new version it might be as well to just
reference it anyway, hopefully the mlx5 code will be merged fairly
quickly once the bus itself is merged.  It's probably easier all round
than adding the reference later, it seems more likely that mlx5 will get
merged than that it'll fall by the wayside.

Oded Gabbay Nov. 10, 2020, 7:23 a.m. UTC | #19

On Fri, Nov 06, 2020 at 07:35:37PM +0000, Mark Brown wrote:
> On Thu, Nov 05, 2020 at 08:37:14PM +0000, Parav Pandit wrote:
> 
> > > > This example describes the mlx5 PCI subfunction use case.
> > > > I didn't follow your question about 'explicit example'.
> > > > What part is missing to identify it as explicit example?
> 
> > > Specifically listing "mlx5" so if someone reading this document thinks to
> > > themselves "hey mlx5 sounds like my use case" they can go grep for that.
> 
> > Ah, I see.
> > "mlx5" is not listed explicitly, because it is not included in this patchset.
> > In various previous discussions in this thread, mlx5 subfunction use case is described that justifies the existence of the bus.
> > I will be happy to update this documentation once mlx5 subfunction will be part of kernel so that grep actually shows valid output.
> > (waiting to post them as it uses auxiliary bus :-)).
> 
> For ease of review if there's a new version it might be as well to just
> reference it anyway, hopefully the mlx5 code will be merged fairly
> quickly once the bus itself is merged.  It's probably easier all round
> than adding the reference later, it seems more likely that mlx5 will get
> merged than that it'll fall by the wayside.

Another use-case for this patch-set is going to be the habanalabs driver.
The GAUDI ASIC is a PCI H/W accelerator for deep-learning which also exposes 
network ports.We are going to use this auxiliary-bus feature to separate our 
monolithic driver into several parts that will reside in different subsystems 
and communicate between them through the bus.

Thanks,
Oded

Greg KH Nov. 13, 2020, 3:50 p.m. UTC | #20

On Thu, Oct 22, 2020 at 05:33:29PM -0700, Dave Ertman 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 <[e-mail]>
> Signed-off-by: Kiran Patil <[e-mail]>
> Co-developed-by: Ranjani Sridharan <[e-mail]>
> Signed-off-by: Ranjani Sridharan <[e-mail]>
> Co-developed-by: Fred Oh <[e-mail]>
> Signed-off-by: Fred Oh <[e-mail]>
> Co-developed-by: Leon Romanovsky <[e-mail]>
> Signed-off-by: Leon Romanovsky <[e-mail]>
> Reviewed-by: Pierre-Louis Bossart <[e-mail]>
> Reviewed-by: Shiraz Saleem <[e-mail]>
> Reviewed-by: Parav Pandit <[e-mail]>
> Reviewed-by: Dan Williams <[e-mail]>
> Signed-off-by: Dave Ertman <[e-mail]>
> ---

Is this really the "latest" version of this patch submission?

I see a number of comments on it already, have you sent out a newer one,
or is this the same one that the mlx5 driver conversion was done on top
of?

thanks,

greg k-h

Ertman, David M Nov. 13, 2020, 4:07 p.m. UTC | #21

> -----Original Message-----
> From: Greg KH <[e-mail]>
> Sent: Friday, November 13, 2020 7:50 AM
> To: Ertman, David M <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Saleem, Shiraz
> <[e-mail]>; Williams, Dan J <[e-mail]>;
> Patil, Kiran <[e-mail]>; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v3 01/10] Add auxiliary bus support
> 
> On Thu, Oct 22, 2020 at 05:33:29PM -0700, Dave Ertman 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 <[e-mail]>
> > Signed-off-by: Kiran Patil <[e-mail]>
> > Co-developed-by: Ranjani Sridharan <[e-mail]>
> > Signed-off-by: Ranjani Sridharan <[e-mail]>
> > Co-developed-by: Fred Oh <[e-mail]>
> > Signed-off-by: Fred Oh <[e-mail]>
> > Co-developed-by: Leon Romanovsky <[e-mail]>
> > Signed-off-by: Leon Romanovsky <[e-mail]>
> > Reviewed-by: Pierre-Louis Bossart <[e-mail]>
> > Reviewed-by: Shiraz Saleem <[e-mail]>
> > Reviewed-by: Parav Pandit <[e-mail]>
> > Reviewed-by: Dan Williams <[e-mail]>
> > Signed-off-by: Dave Ertman <[e-mail]>
> > ---
> 
> Is this really the "latest" version of this patch submission?
> 
> I see a number of comments on it already, have you sent out a newer one,
> or is this the same one that the mlx5 driver conversion was done on top
> of?
> 
> thanks,
> 
> greg k-h

V3 is the latest sent so far.  There was a suggestion that v3 might be merged and
the documentation changes could be in a follow up patch.  I have those changes done
and ready though, so no reason not to merge them in and do a resend.

Please expect v4 in just a little while.

Thanks,
-DaveE

Greg KH Nov. 13, 2020, 11:21 p.m. UTC | #22

On Fri, Nov 13, 2020 at 04:07:57PM +0000, Ertman, David M wrote:
> > -----Original Message-----
> > From: Greg KH <[e-mail]>
> > Sent: Friday, November 13, 2020 7:50 AM
> > To: Ertman, David M <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail]; linux-
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Saleem, Shiraz
> > <[e-mail]>; Williams, Dan J <[e-mail]>;
> > Patil, Kiran <[e-mail]>; [e-mail];
> > [e-mail]
> > Subject: Re: [PATCH v3 01/10] Add auxiliary bus support
> > 
> > On Thu, Oct 22, 2020 at 05:33:29PM -0700, Dave Ertman 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 <[e-mail]>
> > > Signed-off-by: Kiran Patil <[e-mail]>
> > > Co-developed-by: Ranjani Sridharan <[e-mail]>
> > > Signed-off-by: Ranjani Sridharan <[e-mail]>
> > > Co-developed-by: Fred Oh <[e-mail]>
> > > Signed-off-by: Fred Oh <[e-mail]>
> > > Co-developed-by: Leon Romanovsky <[e-mail]>
> > > Signed-off-by: Leon Romanovsky <[e-mail]>
> > > Reviewed-by: Pierre-Louis Bossart <[e-mail]>
> > > Reviewed-by: Shiraz Saleem <[e-mail]>
> > > Reviewed-by: Parav Pandit <[e-mail]>
> > > Reviewed-by: Dan Williams <[e-mail]>
> > > Signed-off-by: Dave Ertman <[e-mail]>
> > > ---
> > 
> > Is this really the "latest" version of this patch submission?
> > 
> > I see a number of comments on it already, have you sent out a newer one,
> > or is this the same one that the mlx5 driver conversion was done on top
> > of?
> > 
> > thanks,
> > 
> > greg k-h
> 
> V3 is the latest sent so far.  There was a suggestion that v3 might be merged and
> the documentation changes could be in a follow up patch.  I have those changes done
> and ready though, so no reason not to merge them in and do a resend.
> 
> Please expect v4 in just a little while.

Thank you, follow-up patches aren't usually a good idea :)

Leon Romanovsky Nov. 15, 2020, 6:48 a.m. UTC | #23

On Sat, Nov 14, 2020 at 12:21:39AM +0100, Greg KH wrote:
> On Fri, Nov 13, 2020 at 04:07:57PM +0000, Ertman, David M wrote:
> > > -----Original Message-----
> > > From: Greg KH <[e-mail]>
> > > Sent: Friday, November 13, 2020 7:50 AM
> > > To: Ertman, David M <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; [e-mail]; linux-
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; Saleem, Shiraz
> > > <[e-mail]>; Williams, Dan J <[e-mail]>;
> > > Patil, Kiran <[e-mail]>; [e-mail];
> > > [e-mail]
> > > Subject: Re: [PATCH v3 01/10] Add auxiliary bus support
> > >
> > > On Thu, Oct 22, 2020 at 05:33:29PM -0700, Dave Ertman 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 <[e-mail]>
> > > > Signed-off-by: Kiran Patil <[e-mail]>
> > > > Co-developed-by: Ranjani Sridharan <[e-mail]>
> > > > Signed-off-by: Ranjani Sridharan <[e-mail]>
> > > > Co-developed-by: Fred Oh <[e-mail]>
> > > > Signed-off-by: Fred Oh <[e-mail]>
> > > > Co-developed-by: Leon Romanovsky <[e-mail]>
> > > > Signed-off-by: Leon Romanovsky <[e-mail]>
> > > > Reviewed-by: Pierre-Louis Bossart <[e-mail]>
> > > > Reviewed-by: Shiraz Saleem <[e-mail]>
> > > > Reviewed-by: Parav Pandit <[e-mail]>
> > > > Reviewed-by: Dan Williams <[e-mail]>
> > > > Signed-off-by: Dave Ertman <[e-mail]>
> > > > ---
> > >
> > > Is this really the "latest" version of this patch submission?
> > >
> > > I see a number of comments on it already, have you sent out a newer one,
> > > or is this the same one that the mlx5 driver conversion was done on top
> > > of?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > V3 is the latest sent so far.  There was a suggestion that v3 might be merged and
> > the documentation changes could be in a follow up patch.  I have those changes done
> > and ready though, so no reason not to merge them in and do a resend.
> >
> > Please expect v4 in just a little while.
>
> Thank you, follow-up patches aren't usually a good idea :)

The changes were in documentation area that will be changed
anyway after dust will settle and we all see real users and
more or less stable in-kernel API.

Thanks

Patch

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.  Splitting the
+functionality into smaller orthogonal devices would make it easier to manage
+data, power management and domain-specific interaction with the hardware. 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.
+
+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. 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.
+
+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
+needs to export this functionality and attach to an rdma driver in another
+subsystem.  The PCI driver will allocate and register an auxiliary_device for
+each physical function on the NIC.  The rdma driver will register an
+auxiliary_driver that will be matched with and probed for each of these
+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.
+
+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.
+
+Auxiliary Device
+================
+
+An auxiliary_device is created and registered to represent a part of its parent
+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
+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
+followed by a call to auxiliary_device_unin().
+
+.. 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.  It
+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
+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.
+
+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
+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:
+
+.. 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
+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:
+
+.. 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
+
 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
+ *
+ * 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().
+ * 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
+ * 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.
+ */
+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");
+MODULE_DESCRIPTION("Auxiliary Bus");
+MODULE_AUTHOR("David Ertman <[e-mail]>");
+MODULE_AUTHOR("Kiran Patil <[e-mail]>");
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;
+	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.