Re: [PATCH 1/6] Add ancillary bus support

[Leon Romanovsky <leon@kernel.org>]

On Thu, Oct 01, 2020 at 05:20:35PM +0000, Ertman, David M wrote:
> > -----Original Message-----
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Thursday, October 1, 2020 1:32 AM
> > To: Ertman, David M <david.m.ertman@intel.com>
> > Cc: linux-rdma@vger.kernel.org
> > Subject: Re: [PATCH 1/6] Add ancillary bus support
> >
> > On Wed, Sep 30, 2020 at 10:05:29PM -0700, Dave Ertman wrote:
> > > Add support for the Ancillary Bus, ancillary_device and ancillary_driver.
> > > It enables drivers to create an ancillary_device and bind an
> > > ancillary_driver to it.
> > >
> > > The bus supports probe/remove shutdown and suspend/resume callbacks.
> > > Each ancillary_device has a unique string based id; driver binds to
> > > an ancillary_device based on this id through the bus.
> > >
> > > Co-developed-by: Kiran Patil <kiran.patil@intel.com>
> > > Signed-off-by: Kiran Patil <kiran.patil@intel.com>
> > > Co-developed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> > > Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> > > Co-developed-by: Fred Oh <fred.oh@linux.intel.com>
> > > Signed-off-by: Fred Oh <fred.oh@linux.intel.com>
> > > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > Reviewed-by: Shiraz Saleem <shiraz.saleem@intel.com>
> > > Reviewed-by: Parav Pandit <parav@mellanox.com>
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> > > ---
> > >  Documentation/driver-api/ancillary_bus.rst | 230
> > +++++++++++++++++++++
> > >  Documentation/driver-api/index.rst         |   1 +
> > >  drivers/bus/Kconfig                        |   3 +
> > >  drivers/bus/Makefile                       |   3 +
> > >  drivers/bus/ancillary.c                    | 191 +++++++++++++++++
> > >  include/linux/ancillary_bus.h              |  58 ++++++
> > >  include/linux/mod_devicetable.h            |   8 +
> > >  scripts/mod/devicetable-offsets.c          |   3 +
> > >  scripts/mod/file2alias.c                   |   8 +
> > >  9 files changed, 505 insertions(+)
> > >  create mode 100644 Documentation/driver-api/ancillary_bus.rst
> > >  create mode 100644 drivers/bus/ancillary.c
> > >  create mode 100644 include/linux/ancillary_bus.h
> > >
> > > diff --git a/Documentation/driver-api/ancillary_bus.rst
> > b/Documentation/driver-api/ancillary_bus.rst
> > > new file mode 100644
> > > index 000000000000..0a11979aa927
> > > --- /dev/null
> > > +++ b/Documentation/driver-api/ancillary_bus.rst
> > > @@ -0,0 +1,230 @@
> > > +.. SPDX-License-Identifier: GPL-2.0-only
> > > +
> > > +=============
> > > +Ancillary 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
> > > +that are DT enumerated.
> > > +
> > > +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 ancillary bus is intended to be minimal, generic and avoid domain-
> > specific
> > > +assumptions. Each ancillary_device represents a part of its parent
> > > +functionality. The generic behavior can be extended and specialized as
> > needed
> > > +by encapsulating an ancillary_device within other domain-specific
> > structures and
> > > +the use of .ops callbacks. Devices on the ancillary bus do not share any
> > > +structures and the use of a communication channel with the parent is
> > > +domain-specific.
> > > +
> > > +When Should the Ancillary Bus Be Used
> > > +=====================================
> > > +
> > > +The ancillary 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 ancillary_device's
> > > +registering driver.  The registering driver for the ancillary_device(s) and
> > the
> > > +kernel module(s) registering ancillary_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 ancillary_device for
> > > +each physical function on the NIC.  The rdma driver will register an
> > > +ancillary_driver that will be matched with and probed for each of these
> > > +ancillary_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 a PCI device to be split out into multiple sub
> > > +functions.  For each sub function an ancillary_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 ancillary 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 ancillary_device.
> > > +
> > > +Ancillary Device
> > > +================
> > > +
> > > +An ancillary_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 ancillary_device is a two-step process.  First you must call
> > > +ancillary_device_initialize(), which will check several aspects of the
> > > +ancillary_device struct and perform a device_initialize().  After this step
> > > +completes, any error state must have a call to put_device() in its
> > resolution
> > > +path.  The second step in registering an ancillary_device is to perform a
> > call
> > > +to ancillary_device_add(), which will set the name of the device and add
> > the
> > > +device to the bus.
> > > +
> > > +To unregister an ancillary_device, just a call to
> > ancillary_device_unregister()
> > > +is used.  This will perform both a device_del() and a put_device().
> >
> > Why did you chose ancillary_device_initialize() and not
> > ancillary_device_register() to be paired with ancillary_device_unregister()?
> >
> > Thanks
>
> We originally had a single call to ancillary_device_register() that paired with
> unregister, but there was an ask to separate the register into an initialize and
> add to make the error condition unwind more compartimentalized.

It is correct thing to separate, but I would expect:
ancillary_device_register()
ancillary_device_add()

vs.
ancillary_device_unregister()

It is not a big deal, just curious.

The much more big deal is that I'm required to create 1-to-1 mapping
between device and driver, and I can't connect all my different modules
to one xxx_core.pf.y device in N-to-1 mapping. "N" represents different
protocols (IB, ETH, SCSI) and "1" is one PCI core.

Thanks

>
> -DaveE