* [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
@ 2020-10-01 5:05 Dave Ertman
2020-10-01 5:05 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
` (6 more replies)
0 siblings, 7 replies; 48+ messages in thread
From: Dave Ertman @ 2020-10-01 5:05 UTC (permalink / raw)
To: linux-rdma
Brief history of Ancillary Bus
==============================
The ancillary bus code was originally submitted upstream as virtual
bus, and was submitted through the netdev tree. This process generated
up to v4. This discussion can be found here:
https://lore.kernel.org/netdev/0200520070227.3392100-2-jeffrey.t.kirsher@intel.com/T/#u
At this point, GregKH requested that we take the review and revision
process to an internal mailing list and garner the buy-in of a respected
kernel contributor.
The ancillary bus (then known as virtual bus) was originally submitted
along with implementation code for the ice driver and irdma drive,
causing the complication of also having dependencies in the rdma tree.
This new submission is utilizing an ancillary bus consumer in only the
sound driver tree to create the initial implementation and a single
user.
Since implementation work has started on this patch set, there have been
multiple inquiries about the time frame of its completion. It appears
that there will be numerous consumers of this functionality.
The process of internal review and implementation using the sound
drivers generated 19 internal versions. The changes, including the name
change from virtual bus to ancillary bus, from these versions can be
summarized as the following:
- Fixed compilation and checkpatch errors
- Improved documentation to address the motivation for virtual bus.
- Renamed virtual bus to ancillary bus
- increased maximum device name size
- Correct order in Kconfig and Makefile
- removed the mid-layer adev->release layer for device unregister
- pushed adev->id management to parent driver
- all error paths out of ancillary_device_register return error code
- all error paths out of ancillary_device_register use put_device
- added adev->name element
- modname in register cannot be NULL
- added KBUILD_MODNAME as prefix for match_name
- push adev->id responsibility to registering driver
- uevent now parses adev->dev name
- match_id function now parses adev->dev name
- changed drivers probe function to also take an ancillary_device_id param
- split ancillary_device_register into device_initialize and device_add
- adjusted what is done in device_initialize and device_add
- change adev to ancildev and adrv to ancildrv
- change adev to ancildev in documentation
This submission is the first time that this patch set will be sent to
the alsa-devel mailing list, so it is currently being submitted as
version 1.
==========================
Introduces the ancillary bus implementation along with the example usage
in the Sound Open Firmware(SOF) audio driver.
In some subsystems, the functionality of the core device
(PCI/ACPI/other) may be too complex for a single device to be managed as
a monolithic block or a part of the functionality might need to be
exposed to a different subsystem. Splitting the functionality into
smaller orthogonal devices makes it easier to manage data, power
management and domain-specific communication with the hardware. Also,
common ancillary_device functionality across primary devices can be
handled by a common ancillary_device. 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 it 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 handles 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 tightly coupled with handling the
hardware-specific logic and communication.
The ancillary bus is intended to be minimal, generic and avoid
domain-specific assumptions. Each ancillary bus device represents a part
of its parent functionality. The generic behavior can be extended and
specialized as needed by encapsulating an ancillary bus device within
other domain-specific structures and the use of .ops callbacks.
The SOF driver adopts the ancillary bus for implementing the
multi-client support. A client in the context of the SOF driver
represents a part of the core device's functionality. It is not a
physical device but rather an ancillary device that needs to communicate
with the DSP via IPCs. With multi-client support,the sound card can be
separated into multiple orthogonal ancillary devices for local devices
(mic/speakers etc), HDMI, sensing, probes, debug etc. In this series,
we demonstrate the usage of the ancillary bus with the help of the IPC
test client which is used for testing the serialization of IPCs when
multiple clients talk to the DSP at the same time.
Dave Ertman (1):
Add ancillary bus support
Fred Oh (1):
ASoC: SOF: debug: Remove IPC flood test support in SOF core
Ranjani Sridharan (4):
ASoC: SOF: Introduce descriptors for SOF client
ASoC: SOF: Create client driver for IPC test
ASoC: SOF: ops: Add ops for client registration
ASoC: SOF: Intel: Define ops for client registration
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 +
sound/soc/sof/Kconfig | 29 +-
sound/soc/sof/Makefile | 7 +
sound/soc/sof/core.c | 12 +
sound/soc/sof/debug.c | 230 ---------------
sound/soc/sof/intel/Kconfig | 9 +
sound/soc/sof/intel/Makefile | 3 +
sound/soc/sof/intel/apl.c | 18 ++
sound/soc/sof/intel/bdw.c | 18 ++
sound/soc/sof/intel/byt.c | 22 ++
sound/soc/sof/intel/cnl.c | 18 ++
sound/soc/sof/intel/intel-client.c | 49 ++++
sound/soc/sof/intel/intel-client.h | 26 ++
sound/soc/sof/ops.h | 14 +
sound/soc/sof/sof-client.c | 117 ++++++++
sound/soc/sof/sof-client.h | 65 +++++
sound/soc/sof/sof-ipc-test-client.c | 314 +++++++++++++++++++++
sound/soc/sof/sof-priv.h | 16 +-
26 files changed, 1233 insertions(+), 239 deletions(-)
create mode 100644 Documentation/driver-api/ancillary_bus.rst
create mode 100644 drivers/bus/ancillary.c
create mode 100644 include/linux/ancillary_bus.h
create mode 100644 sound/soc/sof/intel/intel-client.c
create mode 100644 sound/soc/sof/intel/intel-client.h
create mode 100644 sound/soc/sof/sof-client.c
create mode 100644 sound/soc/sof/sof-client.h
create mode 100644 sound/soc/sof/sof-ipc-test-client.c
--
2.26.2
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 1/6] Add ancillary bus support
2020-10-01 5:05 [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Dave Ertman
@ 2020-10-01 5:05 ` Dave Ertman
2020-10-01 8:32 ` Leon Romanovsky
2020-10-01 5:05 ` [PATCH 2/6] ASoC: SOF: Introduce descriptors for SOF client Dave Ertman
` (5 subsequent siblings)
6 siblings, 1 reply; 48+ messages in thread
From: Dave Ertman @ 2020-10-01 5:05 UTC (permalink / raw)
To: linux-rdma
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().
+
+.. code-block:: c
+
+ struct ancillary_device {
+ struct device dev;
+ const char *name;
+ u32 id;
+ };
+
+If two ancillary_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 ancillary_device.dev.type.release or ancillary_device.dev.release must be
+populated with a non-NULL pointer to successfully register the ancillary_device.
+
+The ancillary_device.dev.parent must also be populated.
+
+Ancillary Device Memory Model and Lifespan
+------------------------------------------
+
+When a kernel driver registers an ancillary_device on the ancillary 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 ancillary_device and register it
+on the ancillary 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
+ancillary_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 ancillary_driver's registering module to perform a container_of()
+call to go from the pointer to the ancillary_device, that is passed during the
+call to the ancillary_driver's probe function, up to the parent object, and then
+have access to the shared object(s).
+
+The memory for the ancillary_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 ancillary_device. The ancillary_driver
+should only consider that this shared object is valid as long as the
+ancillary_device is still registered on the ancillary 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 ancillary_device.
+
+Registering driver must unregister all ancillary devices before its registering
+parent device's remove() is completed.
+
+Ancillary Drivers
+=================
+
+Ancillary 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 ancillary_driver {
+ int (*probe)(struct ancillary_device *,
+ const struct ancillary_device_id *id);
+ int (*remove)(struct ancillary_device *);
+ void (*shutdown)(struct ancillary_device *);
+ int (*suspend)(struct ancillary_device *, pm_message_t);
+ int (*resume)(struct ancillary_device *);
+ struct device_driver driver;
+ const struct ancillary_device_id *id_table;
+ };
+
+Ancillary drivers register themselves with the bus by calling
+ancillary_driver_register(). The id_table contains the match_names of ancillary
+devices that a driver can bind with.
+
+Example Usage
+=============
+
+Ancillary 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 ancillary_device would be to encapsulate it within a
+domain-specific structure defined by the parent device. This structure contains
+the ancillary_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 ancillary_device ancildev;
+ void (*connect)(struct ancillary_device *ancildev);
+ void (*disconnect)(struct ancillary_device *ancildev);
+ void *data;
+ };
+
+The parent device would then register the ancillary_device by calling
+ancillary_device_initialize(), and then ancillary_device_add(), with the pointer
+to the ancildev member of the above structure. The parent would provide a name
+for the ancillary_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 ancillary_driver is registered, based on the match_name, the
+ancillary_driver's probe() is invoked for the matching devices. The
+ancillary_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 ancillary_device *ancildev);
+ void (*receive)(struct ancillary_device *ancildev);
+ };
+
+
+ struct my_driver {
+ struct ancillary_driver ancillary_drv;
+ const struct my_ops ops;
+ };
+
+An example of this type of usage would be:
+
+.. code-block:: c
+
+ const struct ancillary_device_id my_ancillary_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 = {
+ .ancillary_drv = {
+ .driver = {
+ .name = "myancillarydrv",
+ },
+ .id_table = my_ancillary_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 5ef2cfe3a16b..9584ac2ed1f5 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -74,6 +74,7 @@ available subsections can be seen below.
thermal/index
fpga/index
acpi/index
+ ancillary_bus
backlight/lp855x-driver.rst
connector
console
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 0c262c2aeaf2..ba82a045b847 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -5,6 +5,9 @@
menu "Bus devices"
+config ANCILLARY_BUS
+ tristate
+
config ARM_CCI
bool
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index 397e35392bff..1fd238094543 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -3,6 +3,9 @@
# Makefile for the bus drivers.
#
+#Ancillary bus driver
+obj-$(CONFIG_ANCILLARY_BUS) += ancillary.o
+
# Interconnect bus drivers for ARM platforms
obj-$(CONFIG_ARM_CCI) += arm-cci.o
obj-$(CONFIG_ARM_INTEGRATOR_LM) += arm-integrator-lm.o
diff --git a/drivers/bus/ancillary.c b/drivers/bus/ancillary.c
new file mode 100644
index 000000000000..2d940fe5717a
--- /dev/null
+++ b/drivers/bus/ancillary.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Software based bus for Ancillary devices
+ *
+ * Copyright (c) 2019-2020 Intel Corporation
+ *
+ * Please see Documentation/driver-api/ancillary_bus.rst for more information.
+ */
+
+#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/ancillary_bus.h>
+
+static const struct ancillary_device_id *ancillary_match_id(const struct ancillary_device_id *id,
+ const struct ancillary_device *ancildev)
+{
+
+ while (id->name[0]) {
+ const char *p = strrchr(dev_name(&ancildev->dev), '.');
+ int match_size;
+
+ if (!p)
+ continue;
+ match_size = p - dev_name(&ancildev->dev);
+
+ /* use dev_name(&ancildev->dev) prefix before last '.' char to match to */
+ if (!strncmp(dev_name(&ancildev->dev), id->name, match_size))
+ return id;
+ id++;
+ }
+ return NULL;
+}
+
+static int ancillary_match(struct device *dev, struct device_driver *drv)
+{
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+ struct ancillary_driver *ancildrv = to_ancillary_drv(drv);
+
+ return !!ancillary_match_id(ancildrv->id_table, ancildev);
+}
+
+static int ancillary_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", ANCILLARY_MODULE_PREFIX, (int)(p - name),
+ name);
+}
+
+static const struct dev_pm_ops ancillary_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)
+};
+
+struct bus_type ancillary_bus_type = {
+ .name = "ancillary",
+ .match = ancillary_match,
+ .uevent = ancillary_uevent,
+ .pm = &ancillary_dev_pm_ops,
+};
+
+/**
+ * ancillary_device_initialize - check ancillary_device and initialize
+ * @ancildev: ancillary device struct
+ */
+int ancillary_device_initialize(struct ancillary_device *ancildev)
+{
+ struct device *dev = &ancildev->dev;
+
+ dev->bus = &ancillary_bus_type;
+
+ if (WARN_ON(!dev->parent) || WARN_ON(!ancildev->name) ||
+ WARN_ON(!(dev->type && dev->type->release) && !dev->release))
+ return -EINVAL;
+
+ device_initialize(&ancildev->dev);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ancillary_device_initialize);
+
+/**
+ * __ancillary_device_add - add an ancillary bus device
+ * @ancildev: ancillary bus device to add to the bus
+ * @modname: name of the parent device's driver module
+ */
+int __ancillary_device_add(struct ancillary_device *ancildev, const char *modname)
+{
+ struct device *dev = &ancildev->dev;
+ int ret;
+
+ if (WARN_ON(!modname))
+ return -EINVAL;
+
+ ret = dev_set_name(dev, "%s.%s.%d", modname, ancildev->name, ancildev->id);
+ if (ret) {
+ dev_err(dev->parent, "dev_set_name failed for device: %d\n", ret);
+ return ret;
+ }
+
+ ret = device_add(dev);
+ if (ret)
+ dev_err(dev, "adding device failed!: %d\n", ret);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(__ancillary_device_add);
+
+static int ancillary_probe_driver(struct device *dev)
+{
+ struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+ int ret;
+
+ ret = dev_pm_domain_attach(dev, true);
+ if (ret) {
+ dev_warn(&ancildev->dev, "Failed to attach to PM Domain : %d\n", ret);
+ return ret;
+ }
+
+ ret = ancildrv->probe(ancildev, ancillary_match_id(ancildrv->id_table, ancildev));
+ if (ret)
+ dev_pm_domain_detach(dev, true);
+
+ return ret;
+}
+
+static int ancillary_remove_driver(struct device *dev)
+{
+ struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+ int ret;
+
+ ret = ancildrv->remove(ancildev);
+ dev_pm_domain_detach(dev, true);
+
+ return ret;
+}
+
+static void ancillary_shutdown_driver(struct device *dev)
+{
+ struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+
+ ancildrv->shutdown(ancildev);
+}
+
+/**
+ * __ancillary_driver_register - register a driver for ancillary bus devices
+ * @ancildrv: ancillary_driver structure
+ * @owner: owning module/driver
+ */
+int __ancillary_driver_register(struct ancillary_driver *ancildrv, struct module *owner)
+{
+ if (WARN_ON(!ancildrv->probe) || WARN_ON(!ancildrv->remove) ||
+ WARN_ON(!ancildrv->shutdown) || WARN_ON(!ancildrv->id_table))
+ return -EINVAL;
+
+ ancildrv->driver.owner = owner;
+ ancildrv->driver.bus = &ancillary_bus_type;
+ ancildrv->driver.probe = ancillary_probe_driver;
+ ancildrv->driver.remove = ancillary_remove_driver;
+ ancildrv->driver.shutdown = ancillary_shutdown_driver;
+
+ return driver_register(&ancildrv->driver);
+}
+EXPORT_SYMBOL_GPL(__ancillary_driver_register);
+
+static int __init ancillary_bus_init(void)
+{
+ return bus_register(&ancillary_bus_type);
+}
+
+static void __exit ancillary_bus_exit(void)
+{
+ bus_unregister(&ancillary_bus_type);
+}
+
+module_init(ancillary_bus_init);
+module_exit(ancillary_bus_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Ancillary Bus");
+MODULE_AUTHOR("David Ertman <david.m.ertman@intel.com>");
+MODULE_AUTHOR("Kiran Patil <kiran.patil@intel.com>");
diff --git a/include/linux/ancillary_bus.h b/include/linux/ancillary_bus.h
new file mode 100644
index 000000000000..73b13b56403d
--- /dev/null
+++ b/include/linux/ancillary_bus.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2019-2020 Intel Corporation
+ *
+ * Please see Documentation/driver-api/ancillary_bus.rst for more information.
+ */
+
+#ifndef _ANCILLARY_BUS_H_
+#define _ANCILLARY_BUS_H_
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+
+struct ancillary_device {
+ struct device dev;
+ const char *name;
+ u32 id;
+};
+
+struct ancillary_driver {
+ int (*probe)(struct ancillary_device *ancildev, const struct ancillary_device_id *id);
+ int (*remove)(struct ancillary_device *ancildev);
+ void (*shutdown)(struct ancillary_device *ancildev);
+ int (*suspend)(struct ancillary_device *ancildev, pm_message_t state);
+ int (*resume)(struct ancillary_device *ancildev);
+ struct device_driver driver;
+ const struct ancillary_device_id *id_table;
+};
+
+static inline struct ancillary_device *to_ancillary_dev(struct device *dev)
+{
+ return container_of(dev, struct ancillary_device, dev);
+}
+
+static inline struct ancillary_driver *to_ancillary_drv(struct device_driver *drv)
+{
+ return container_of(drv, struct ancillary_driver, driver);
+}
+
+int ancillary_device_initialize(struct ancillary_device *ancildev);
+int __ancillary_device_add(struct ancillary_device *ancildev, const char *modname);
+#define ancillary_device_add(ancildev) __ancillary_device_add(ancildev, KBUILD_MODNAME)
+
+static inline void ancillary_device_unregister(struct ancillary_device *ancildev)
+{
+ device_unregister(&ancildev->dev);
+}
+
+int __ancillary_driver_register(struct ancillary_driver *ancildrv, struct module *owner);
+#define ancillary_driver_register(ancildrv) __ancillary_driver_register(ancildrv, THIS_MODULE)
+
+static inline void ancillary_driver_unregister(struct ancillary_driver *ancildrv)
+{
+ driver_unregister(&ancildrv->driver);
+}
+
+#endif /* _ANCILLARY_BUS_H_ */
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5b08a473cdba..7d596dc30833 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 ANCILLARY_NAME_SIZE 32
+#define ANCILLARY_MODULE_PREFIX "ancillary:"
+
+struct ancillary_device_id {
+ char name[ANCILLARY_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..79e37c4c25b3 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(ancillary_device_id);
+ DEVID_FIELD(ancillary_device_id, name);
+
return 0;
}
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2417dd1dee33..99c4fcd82bf3 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_ancillary_entry(const char *filename, void *symval, char *alias)
+{
+ DEF_FIELD_ADDR(symval, ancillary_device_id, name);
+ sprintf(alias, ANCILLARY_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},
+ {"ancillary", SIZE_ancillary_device_id, do_ancillary_entry},
};
/* Create MODULE_ALIAS() statements.
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 2/6] ASoC: SOF: Introduce descriptors for SOF client
2020-10-01 5:05 [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Dave Ertman
2020-10-01 5:05 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
@ 2020-10-01 5:05 ` Dave Ertman
2020-10-01 5:05 ` [PATCH 3/6] ASoC: SOF: Create client driver for IPC test Dave Ertman
` (4 subsequent siblings)
6 siblings, 0 replies; 48+ messages in thread
From: Dave Ertman @ 2020-10-01 5:05 UTC (permalink / raw)
To: linux-rdma
From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
A client in the SOF (Sound Open Firmware) context is a
device that needs to communicate with the DSP via IPC
messages. The SOF core is responsible for serializing the
IPC messages to the DSP from the different clients. One
example of an SOF client would be an IPC test client that
floods the DSP with test IPC messages to validate if the
serialization works as expected. Multi-client support will
also add the ability to split the existing audio cards
into multiple ones, so as to e.g. to deal with HDMI with a
dedicated client instead of adding HDMI to all cards.
This patch introduces descriptors for SOF client driver
and SOF client device along with APIs for registering
and unregistering a SOF client driver, sending IPCs from
a client device and accessing the SOF core debugfs root entry.
Along with this, add a couple of new members to struct
snd_sof_dev that will be used for maintaining the list of
clients.
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@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>
Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
---
sound/soc/sof/Kconfig | 19 ++++++
sound/soc/sof/Makefile | 3 +
sound/soc/sof/core.c | 2 +
sound/soc/sof/sof-client.c | 117 +++++++++++++++++++++++++++++++++++++
sound/soc/sof/sof-client.h | 65 +++++++++++++++++++++
sound/soc/sof/sof-priv.h | 6 ++
6 files changed, 212 insertions(+)
create mode 100644 sound/soc/sof/sof-client.c
create mode 100644 sound/soc/sof/sof-client.h
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index 4dda4b62509f..cea7efedafef 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -50,6 +50,24 @@ config SND_SOC_SOF_DEBUG_PROBES
Say Y if you want to enable probes.
If unsure, select "N".
+config SND_SOC_SOF_CLIENT
+ tristate
+ select ANCILLARY_BUS
+ help
+ This option is not user-selectable but automagically handled by
+ 'select' statements at a higher level
+
+config SND_SOC_SOF_CLIENT_SUPPORT
+ bool "SOF enable clients"
+ depends on SND_SOC_SOF
+ help
+ This adds support for ancillary client devices to separate out the debug
+ functionality for IPC tests, probes etc. into separate devices. This
+ option would also allow adding client devices based on DSP FW
+ capabilities and ACPI/OF device information.
+ Say Y if you want to enable clients with SOF.
+ If unsure select "N".
+
config SND_SOC_SOF_DEVELOPER_SUPPORT
bool "SOF developer options support"
depends on EXPERT
@@ -186,6 +204,7 @@ endif ## SND_SOC_SOF_DEVELOPER_SUPPORT
config SND_SOC_SOF
tristate
+ select SND_SOC_SOF_CLIENT if SND_SOC_SOF_CLIENT_SUPPORT
select SND_SOC_TOPOLOGY
select SND_SOC_SOF_NOCODEC if SND_SOC_SOF_NOCODEC_SUPPORT
help
diff --git a/sound/soc/sof/Makefile b/sound/soc/sof/Makefile
index 05718dfe6cd2..5e46f25a3851 100644
--- a/sound/soc/sof/Makefile
+++ b/sound/soc/sof/Makefile
@@ -2,6 +2,7 @@
snd-sof-objs := core.o ops.o loader.o ipc.o pcm.o pm.o debug.o topology.o\
control.o trace.o utils.o sof-audio.o
+snd-sof-client-objs := sof-client.o
snd-sof-$(CONFIG_SND_SOC_SOF_DEBUG_PROBES) += probe.o compress.o
snd-sof-pci-objs := sof-pci-dev.o
@@ -18,6 +19,8 @@ obj-$(CONFIG_SND_SOC_SOF_ACPI) += snd-sof-acpi.o
obj-$(CONFIG_SND_SOC_SOF_OF) += snd-sof-of.o
obj-$(CONFIG_SND_SOC_SOF_PCI) += snd-sof-pci.o
+obj-$(CONFIG_SND_SOC_SOF_CLIENT) += snd-sof-client.o
+
obj-$(CONFIG_SND_SOC_SOF_INTEL_TOPLEVEL) += intel/
obj-$(CONFIG_SND_SOC_SOF_IMX_TOPLEVEL) += imx/
obj-$(CONFIG_SND_SOC_SOF_XTENSA) += xtensa/
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
index adc7c37145d6..72a97219395f 100644
--- a/sound/soc/sof/core.c
+++ b/sound/soc/sof/core.c
@@ -314,8 +314,10 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
INIT_LIST_HEAD(&sdev->widget_list);
INIT_LIST_HEAD(&sdev->dai_list);
INIT_LIST_HEAD(&sdev->route_list);
+ INIT_LIST_HEAD(&sdev->client_list);
spin_lock_init(&sdev->ipc_lock);
spin_lock_init(&sdev->hw_lock);
+ mutex_init(&sdev->client_mutex);
if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))
INIT_WORK(&sdev->probe_work, sof_probe_work);
diff --git a/sound/soc/sof/sof-client.c b/sound/soc/sof/sof-client.c
new file mode 100644
index 000000000000..f7e476d99ff6
--- /dev/null
+++ b/sound/soc/sof/sof-client.c
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright(c) 2020 Intel Corporation. All rights reserved.
+//
+// Author: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
+//
+
+#include <linux/debugfs.h>
+#include <linux/errno.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include "sof-client.h"
+#include "sof-priv.h"
+
+static void sof_client_ancildev_release(struct device *dev)
+{
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+ struct sof_client_dev *cdev = ancillary_dev_to_sof_client_dev(ancildev);
+
+ ida_simple_remove(cdev->client_ida, ancildev->id);
+ kfree(cdev);
+}
+
+static struct sof_client_dev *sof_client_dev_alloc(struct snd_sof_dev *sdev, const char *name,
+ struct ida *client_ida)
+{
+ struct sof_client_dev *cdev;
+ struct ancillary_device *ancildev;
+ int ret;
+
+ cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
+ if (!cdev)
+ return NULL;
+
+ cdev->sdev = sdev;
+ cdev->client_ida = client_ida;
+ ancildev = &cdev->ancildev;
+ ancildev->name = name;
+ ancildev->dev.parent = sdev->dev;
+ ancildev->dev.release = sof_client_ancildev_release;
+
+ ancildev->id = ida_alloc(client_ida, GFP_KERNEL);
+ if (ancildev->id < 0) {
+ dev_err(sdev->dev, "error: get IDA idx for ancillary device %s failed\n", name);
+ ret = ancildev->id;
+ goto err_free;
+ }
+
+ ret = ancillary_device_initialize(ancildev);
+ if (ret < 0) {
+ dev_err(sdev->dev, "error: failed to initialize client dev %s\n", name);
+ ida_simple_remove(client_ida, ancildev->id);
+ goto err_free;
+ }
+
+ return cdev;
+
+err_free:
+ kfree(cdev);
+ return NULL;
+}
+
+int sof_client_dev_register(struct snd_sof_dev *sdev, const char *name, struct ida *client_ida)
+{
+ struct sof_client_dev *cdev;
+ int ret;
+
+ cdev = sof_client_dev_alloc(sdev, name, client_ida);
+ if (!cdev)
+ return -ENODEV;
+
+ ret = ancillary_device_add(&cdev->ancildev);
+ if (ret < 0) {
+ dev_err(sdev->dev, "error: failed to add client dev %s\n", name);
+ put_device(&cdev->ancildev.dev);
+ return ret;
+ }
+
+ /* add to list of SOF client devices */
+ mutex_lock(&sdev->client_mutex);
+ list_add(&cdev->list, &sdev->client_list);
+ mutex_unlock(&sdev->client_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_NS_GPL(sof_client_dev_register, SND_SOC_SOF_CLIENT);
+
+void sof_client_dev_unregister(struct sof_client_dev *cdev)
+{
+ struct snd_sof_dev *sdev = cdev->sdev;
+
+ /* remove from list of SOF client devices */
+ mutex_lock(&sdev->client_mutex);
+ list_del(&cdev->list);
+ mutex_unlock(&sdev->client_mutex);
+
+ ancillary_device_unregister(&cdev->ancildev);
+}
+EXPORT_SYMBOL_NS_GPL(sof_client_dev_unregister, SND_SOC_SOF_CLIENT);
+
+int sof_client_ipc_tx_message(struct sof_client_dev *cdev, u32 header, void *msg_data,
+ size_t msg_bytes, void *reply_data, size_t reply_bytes)
+{
+ return sof_ipc_tx_message(cdev->sdev->ipc, header, msg_data, msg_bytes,
+ reply_data, reply_bytes);
+}
+EXPORT_SYMBOL_NS_GPL(sof_client_ipc_tx_message, SND_SOC_SOF_CLIENT);
+
+struct dentry *sof_client_get_debugfs_root(struct sof_client_dev *cdev)
+{
+ return cdev->sdev->debugfs_root;
+}
+EXPORT_SYMBOL_NS_GPL(sof_client_get_debugfs_root, SND_SOC_SOF_CLIENT);
+
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/sof/sof-client.h b/sound/soc/sof/sof-client.h
new file mode 100644
index 000000000000..62212f69c236
--- /dev/null
+++ b/sound/soc/sof/sof-client.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: (GPL-2.0-only) */
+
+#ifndef __SOUND_SOC_SOF_CLIENT_H
+#define __SOUND_SOC_SOF_CLIENT_H
+
+#include <linux/ancillary_bus.h>
+
+#define SOF_CLIENT_PROBE_TIMEOUT_MS 2000
+
+struct snd_sof_dev;
+
+/* SOF client device */
+struct sof_client_dev {
+ struct ancillary_device ancildev;
+ struct snd_sof_dev *sdev;
+ struct list_head list; /* item in SOF core client dev list */
+ struct ida *client_ida;
+ void *data;
+};
+
+/* client-specific ops, all optional */
+struct sof_client_ops {
+ int (*client_ipc_rx)(struct sof_client_dev *cdev, u32 msg_cmd);
+};
+
+struct sof_client_drv {
+ const char *name;
+ const struct sof_client_ops ops;
+ struct ancillary_driver ancillary_drv;
+};
+
+#define ancillary_dev_to_sof_client_dev(ancillary_dev) \
+ container_of(ancillary_dev, struct sof_client_dev, ancildev)
+
+static inline int sof_client_drv_register(struct sof_client_drv *drv)
+{
+ return ancillary_driver_register(&drv->ancillary_drv);
+}
+
+static inline void sof_client_drv_unregister(struct sof_client_drv *drv)
+{
+ ancillary_driver_unregister(&drv->ancillary_drv);
+}
+
+int sof_client_dev_register(struct snd_sof_dev *sdev, const char *name, struct ida *client_ida);
+void sof_client_dev_unregister(struct sof_client_dev *cdev);
+
+int sof_client_ipc_tx_message(struct sof_client_dev *cdev, u32 header, void *msg_data,
+ size_t msg_bytes, void *reply_data, size_t reply_bytes);
+
+struct dentry *sof_client_get_debugfs_root(struct sof_client_dev *cdev);
+
+/**
+ * module_sof_client_driver() - Helper macro for registering an SOF Client
+ * driver
+ * @__sof_client_driver: SOF client driver struct
+ *
+ * Helper macro for SOF client 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_sof_client_driver(__sof_client_driver) \
+ module_driver(__sof_client_driver, sof_client_drv_register, sof_client_drv_unregister)
+
+#endif
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 64f28e082049..043fcec5a288 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -438,6 +438,12 @@ struct snd_sof_dev {
bool msi_enabled;
+ /* list of client devices */
+ struct list_head client_list;
+
+ /* mutex to protect client list */
+ struct mutex client_mutex;
+
void *private; /* core does not touch this */
};
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 3/6] ASoC: SOF: Create client driver for IPC test
2020-10-01 5:05 [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Dave Ertman
2020-10-01 5:05 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
2020-10-01 5:05 ` [PATCH 2/6] ASoC: SOF: Introduce descriptors for SOF client Dave Ertman
@ 2020-10-01 5:05 ` Dave Ertman
2020-10-01 5:05 ` [PATCH 4/6] ASoC: SOF: ops: Add ops for client registration Dave Ertman
` (3 subsequent siblings)
6 siblings, 0 replies; 48+ messages in thread
From: Dave Ertman @ 2020-10-01 5:05 UTC (permalink / raw)
To: linux-rdma
From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Create an SOF client driver for IPC flood test. This
driver is used to set up the debugfs entries and the
read/write ops for initiating the IPC flood test that
would be used to measure the min/max/avg response times
for sending IPCs to the DSP.
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@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>
Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
---
sound/soc/sof/Kconfig | 10 +
sound/soc/sof/Makefile | 4 +
sound/soc/sof/sof-ipc-test-client.c | 314 ++++++++++++++++++++++++++++
3 files changed, 328 insertions(+)
create mode 100644 sound/soc/sof/sof-ipc-test-client.c
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index cea7efedafef..55a2a20c3ec9 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -190,6 +190,16 @@ config SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST
Say Y if you want to enable IPC flood test.
If unsure, select "N".
+config SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_CLIENT
+ tristate "SOF enable IPC flood test client"
+ depends on SND_SOC_SOF_CLIENT
+ help
+ This option enables a separate client device for IPC flood test
+ which can be used to flood the DSP with test IPCs and gather stats
+ about response times.
+ Say Y if you want to enable IPC flood test.
+ If unsure, select "N".
+
config SND_SOC_SOF_DEBUG_RETAIN_DSP_CONTEXT
bool "SOF retain DSP context on any FW exceptions"
help
diff --git a/sound/soc/sof/Makefile b/sound/soc/sof/Makefile
index 5e46f25a3851..baa93fe2cc9a 100644
--- a/sound/soc/sof/Makefile
+++ b/sound/soc/sof/Makefile
@@ -9,6 +9,8 @@ snd-sof-pci-objs := sof-pci-dev.o
snd-sof-acpi-objs := sof-acpi-dev.o
snd-sof-of-objs := sof-of-dev.o
+snd-sof-ipc-test-objs := sof-ipc-test-client.o
+
snd-sof-nocodec-objs := nocodec.o
obj-$(CONFIG_SND_SOC_SOF) += snd-sof.o
@@ -21,6 +23,8 @@ obj-$(CONFIG_SND_SOC_SOF_PCI) += snd-sof-pci.o
obj-$(CONFIG_SND_SOC_SOF_CLIENT) += snd-sof-client.o
+obj-$(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_CLIENT) += snd-sof-ipc-test.o
+
obj-$(CONFIG_SND_SOC_SOF_INTEL_TOPLEVEL) += intel/
obj-$(CONFIG_SND_SOC_SOF_IMX_TOPLEVEL) += imx/
obj-$(CONFIG_SND_SOC_SOF_XTENSA) += xtensa/
diff --git a/sound/soc/sof/sof-ipc-test-client.c b/sound/soc/sof/sof-ipc-test-client.c
new file mode 100644
index 000000000000..c39d5009c75b
--- /dev/null
+++ b/sound/soc/sof/sof-ipc-test-client.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright(c) 2020 Intel Corporation. All rights reserved.
+//
+// Author: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
+//
+
+#include <linux/completion.h>
+#include <linux/debugfs.h>
+#include <linux/ktime.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/ancillary_bus.h>
+#include <sound/sof/header.h>
+#include "sof-client.h"
+
+#define MAX_IPC_FLOOD_DURATION_MS 1000
+#define MAX_IPC_FLOOD_COUNT 10000
+#define IPC_FLOOD_TEST_RESULT_LEN 512
+#define SOF_IPC_CLIENT_SUSPEND_DELAY_MS 3000
+
+struct sof_ipc_client_data {
+ struct dentry *dfs_root;
+ char *buf;
+};
+
+/* helper function to perform the flood test */
+static int sof_debug_ipc_flood_test(struct sof_client_dev *cdev, bool flood_duration_test,
+ unsigned long ipc_duration_ms, unsigned long ipc_count)
+{
+ struct sof_ipc_client_data *ipc_client_data = cdev->data;
+ struct device *dev = &cdev->ancildev.dev;
+ struct sof_ipc_cmd_hdr hdr;
+ struct sof_ipc_reply reply;
+ u64 min_response_time = U64_MAX;
+ u64 avg_response_time = 0;
+ u64 max_response_time = 0;
+ ktime_t cur = ktime_get();
+ ktime_t test_end;
+ int i = 0;
+ int ret = 0;
+
+ /* configure test IPC */
+ hdr.cmd = SOF_IPC_GLB_TEST_MSG | SOF_IPC_TEST_IPC_FLOOD;
+ hdr.size = sizeof(hdr);
+
+ /* set test end time for duration flood test */
+ test_end = ktime_get_ns() + ipc_duration_ms * NSEC_PER_MSEC;
+
+ /* send test IPC's */
+ for (i = 0; flood_duration_test ? ktime_to_ns(cur) < test_end : i < ipc_count; i++) {
+ ktime_t start;
+ u64 ipc_response_time;
+
+ start = ktime_get();
+ ret = sof_client_ipc_tx_message(cdev, hdr.cmd,
+ &hdr, hdr.size, &reply,
+ sizeof(reply));
+ if (ret < 0)
+ break;
+ cur = ktime_get();
+
+ /* compute min and max response times */
+ ipc_response_time = ktime_to_ns(ktime_sub(cur, start));
+ min_response_time = min(min_response_time, ipc_response_time);
+ max_response_time = max(max_response_time, ipc_response_time);
+
+ /* sum up response times */
+ avg_response_time += ipc_response_time;
+ }
+
+ if (ret < 0)
+ return ret;
+
+ /* return if the first IPC fails */
+ if (!i)
+ return ret;
+
+ /* compute average response time */
+ DIV_ROUND_CLOSEST(avg_response_time, i);
+
+ /* clear previous test output */
+ memset(ipc_client_data->buf, 0, IPC_FLOOD_TEST_RESULT_LEN);
+
+ if (flood_duration_test) {
+ dev_dbg(dev, "IPC Flood test duration: %lums\n", ipc_duration_ms);
+ snprintf(ipc_client_data->buf, IPC_FLOOD_TEST_RESULT_LEN,
+ "IPC Flood test duration: %lums\n", ipc_duration_ms);
+ }
+
+ dev_dbg(dev,
+ "IPC Flood count: %d, Avg response time: %lluns\n", i, avg_response_time);
+ dev_dbg(dev, "Max response time: %lluns\n", max_response_time);
+ dev_dbg(dev, "Min response time: %lluns\n", min_response_time);
+
+ /* format output string and save test results */
+ snprintf(ipc_client_data->buf + strlen(ipc_client_data->buf),
+ IPC_FLOOD_TEST_RESULT_LEN - strlen(ipc_client_data->buf),
+ "IPC Flood count: %d\nAvg response time: %lluns\n", i, avg_response_time);
+
+ snprintf(ipc_client_data->buf + strlen(ipc_client_data->buf),
+ IPC_FLOOD_TEST_RESULT_LEN - strlen(ipc_client_data->buf),
+ "Max response time: %lluns\nMin response time: %lluns\n",
+ max_response_time, min_response_time);
+
+ return ret;
+}
+
+/*
+ * Writing to the debugfs entry initiates the IPC flood test based on
+ * the IPC count or the duration specified by the user.
+ */
+static ssize_t sof_ipc_dfsentry_write(struct file *file, const char __user *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct dentry *dentry = file->f_path.dentry;
+ struct sof_client_dev *cdev = file->private_data;
+ struct device *dev = &cdev->ancildev.dev;
+ unsigned long ipc_duration_ms = 0;
+ bool flood_duration_test;
+ unsigned long ipc_count = 0;
+ char *string;
+ size_t size;
+ int err;
+ int ret;
+
+ string = kzalloc(count, GFP_KERNEL);
+ if (!string)
+ return -ENOMEM;
+
+ size = simple_write_to_buffer(string, count, ppos, buffer, count);
+
+ flood_duration_test = !strcmp(dentry->d_name.name, "ipc_flood_duration_ms");
+
+ /* set test completion criterion */
+ ret = flood_duration_test ? kstrtoul(string, 0, &ipc_duration_ms) :
+ kstrtoul(string, 0, &ipc_count);
+ if (ret < 0)
+ goto out;
+
+ /* limit max duration/ipc count for flood test */
+ if (flood_duration_test) {
+ if (!ipc_duration_ms) {
+ ret = size;
+ goto out;
+ }
+
+ ipc_duration_ms = min_t(unsigned long, ipc_duration_ms, MAX_IPC_FLOOD_DURATION_MS);
+ } else {
+ if (!ipc_count) {
+ ret = size;
+ goto out;
+ }
+
+ ipc_count = min_t(unsigned long, ipc_count, MAX_IPC_FLOOD_COUNT);
+ }
+
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0 && ret != -EACCES) {
+ dev_err_ratelimited(dev, "error: debugfs write failed to resume %d\n", ret);
+ pm_runtime_put_noidle(dev);
+ goto out;
+ }
+
+ /* flood test */
+ ret = sof_debug_ipc_flood_test(cdev, flood_duration_test, ipc_duration_ms, ipc_count);
+
+ pm_runtime_mark_last_busy(dev);
+ err = pm_runtime_put_autosuspend(dev);
+ if (err < 0) {
+ ret = err;
+ goto out;
+ }
+
+ /* return size if test is successful */
+ if (ret >= 0)
+ ret = size;
+out:
+ kfree(string);
+ return ret;
+}
+
+/* return the result of the last IPC flood test */
+static ssize_t sof_ipc_dfsentry_read(struct file *file, char __user *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct sof_client_dev *cdev = file->private_data;
+ struct sof_ipc_client_data *ipc_client_data = cdev->data;
+ size_t size_ret;
+
+ if (*ppos)
+ return 0;
+
+ /* return results of the last IPC test */
+ count = min_t(size_t, count, strlen(ipc_client_data->buf));
+ size_ret = copy_to_user(buffer, ipc_client_data->buf, count);
+ if (size_ret)
+ return -EFAULT;
+
+ *ppos += count;
+ return count;
+}
+
+static const struct file_operations sof_ipc_dfs_fops = {
+ .open = simple_open,
+ .read = sof_ipc_dfsentry_read,
+ .llseek = default_llseek,
+ .write = sof_ipc_dfsentry_write,
+};
+
+/*
+ * The IPC test client creates a couple of debugfs entries that will be used
+ * flood tests. Users can write to these entries to execute the IPC flood test
+ * by specifying either the number of IPCs to flood the DSP with or the duration
+ * (in ms) for which the DSP should be flooded with test IPCs. At the
+ * end of each test, the average, min and max response times are reported back.
+ * The results of the last flood test can be accessed by reading the debugfs
+ * entries.
+ */
+static int sof_ipc_test_probe(struct ancillary_device *ancildev,
+ const struct ancillary_device_id *id)
+{
+ struct sof_client_dev *cdev = ancillary_dev_to_sof_client_dev(ancildev);
+ struct sof_ipc_client_data *ipc_client_data;
+
+ /*
+ * The ancillary device has a usage count of 0 even before runtime PM
+ * is enabled. So, increment the usage count to let the device
+ * suspend after probe is complete.
+ */
+ pm_runtime_get_noresume(&ancildev->dev);
+
+ /* allocate memory for client data */
+ ipc_client_data = devm_kzalloc(&ancildev->dev, sizeof(*ipc_client_data), GFP_KERNEL);
+ if (!ipc_client_data)
+ return -ENOMEM;
+
+ ipc_client_data->buf = devm_kzalloc(&ancildev->dev, IPC_FLOOD_TEST_RESULT_LEN, GFP_KERNEL);
+ if (!ipc_client_data->buf)
+ return -ENOMEM;
+
+ cdev->data = ipc_client_data;
+
+ /* create debugfs root folder with device name under parent SOF dir */
+ ipc_client_data->dfs_root = debugfs_create_dir(dev_name(&ancildev->dev),
+ sof_client_get_debugfs_root(cdev));
+
+ /* create read-write ipc_flood_count debugfs entry */
+ debugfs_create_file("ipc_flood_count", 0644, ipc_client_data->dfs_root,
+ cdev, &sof_ipc_dfs_fops);
+
+ /* create read-write ipc_flood_duration_ms debugfs entry */
+ debugfs_create_file("ipc_flood_duration_ms", 0644, ipc_client_data->dfs_root,
+ cdev, &sof_ipc_dfs_fops);
+
+ /* enable runtime PM */
+ pm_runtime_set_autosuspend_delay(&ancildev->dev, SOF_IPC_CLIENT_SUSPEND_DELAY_MS);
+ pm_runtime_use_autosuspend(&ancildev->dev);
+ pm_runtime_set_active(&ancildev->dev);
+ pm_runtime_enable(&ancildev->dev);
+ pm_runtime_mark_last_busy(&ancildev->dev);
+ pm_runtime_put_autosuspend(&ancildev->dev);
+
+ return 0;
+}
+
+static int sof_ipc_test_cleanup(struct ancillary_device *ancildev)
+{
+ pm_runtime_disable(&ancildev->dev);
+
+ return 0;
+}
+
+static int sof_ipc_test_remove(struct ancillary_device *ancildev)
+{
+ return sof_ipc_test_cleanup(ancildev);
+}
+
+static void sof_ipc_test_shutdown(struct ancillary_device *ancildev)
+{
+ sof_ipc_test_cleanup(ancildev);
+}
+
+static const struct ancillary_device_id sof_ipc_ancilbus_id_table[] = {
+ { .name = "snd_sof_client.ipc_test" },
+ {},
+};
+MODULE_DEVICE_TABLE(ancillary, sof_ipc_ancilbus_id_table);
+
+/*
+ * No need for driver pm_ops as the generic pm callbacks in the ancillary bus type are enough to
+ * ensure that the parent SOF device resumes to bring the DSP back to D0.
+ */
+static struct sof_client_drv sof_ipc_test_client_drv = {
+ .name = "sof-ipc-test-client-drv",
+ .ancillary_drv = {
+ .driver = {
+ .name = "sof-ipc-test-ancilbus-drv",
+ },
+ .id_table = sof_ipc_ancilbus_id_table,
+ .probe = sof_ipc_test_probe,
+ .remove = sof_ipc_test_remove,
+ .shutdown = sof_ipc_test_shutdown,
+ },
+};
+
+module_sof_client_driver(sof_ipc_test_client_drv);
+
+MODULE_DESCRIPTION("SOF IPC Test Client Driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(SND_SOC_SOF_CLIENT);
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 4/6] ASoC: SOF: ops: Add ops for client registration
2020-10-01 5:05 [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Dave Ertman
` (2 preceding siblings ...)
2020-10-01 5:05 ` [PATCH 3/6] ASoC: SOF: Create client driver for IPC test Dave Ertman
@ 2020-10-01 5:05 ` Dave Ertman
2020-10-01 5:05 ` [PATCH 5/6] ASoC: SOF: Intel: Define " Dave Ertman
` (2 subsequent siblings)
6 siblings, 0 replies; 48+ messages in thread
From: Dave Ertman @ 2020-10-01 5:05 UTC (permalink / raw)
To: linux-rdma
From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add new ops for registering/unregistering clients based
on DSP capabilities and/or DT information.
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
---
sound/soc/sof/core.c | 10 ++++++++++
sound/soc/sof/ops.h | 14 ++++++++++++++
sound/soc/sof/sof-priv.h | 4 ++++
3 files changed, 28 insertions(+)
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
index 72a97219395f..ddb9a12d5aac 100644
--- a/sound/soc/sof/core.c
+++ b/sound/soc/sof/core.c
@@ -246,8 +246,17 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
if (plat_data->sof_probe_complete)
plat_data->sof_probe_complete(sdev->dev);
+ /* If registering certain clients fails, unregister the previously registered clients. */
+ ret = snd_sof_register_clients(sdev);
+ if (ret < 0) {
+ dev_err(sdev->dev, "error: failed to register clients %d\n", ret);
+ goto client_reg_err;
+ }
+
return 0;
+client_reg_err:
+ snd_sof_unregister_clients(sdev);
fw_trace_err:
snd_sof_free_trace(sdev);
fw_run_err:
@@ -356,6 +365,7 @@ int snd_sof_device_remove(struct device *dev)
dev_warn(dev, "error: %d failed to prepare DSP for device removal",
ret);
+ snd_sof_unregister_clients(sdev);
snd_sof_fw_unload(sdev);
snd_sof_ipc_free(sdev);
snd_sof_free_debug(sdev);
diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h
index b21632f5511a..0e5660d7a2fd 100644
--- a/sound/soc/sof/ops.h
+++ b/sound/soc/sof/ops.h
@@ -470,6 +470,20 @@ snd_sof_set_mach_params(const struct snd_soc_acpi_mach *mach,
sof_ops(sdev)->set_mach_params(mach, dev);
}
+static inline int snd_sof_register_clients(struct snd_sof_dev *sdev)
+{
+ if (sof_ops(sdev) && sof_ops(sdev)->register_clients)
+ return sof_ops(sdev)->register_clients(sdev);
+
+ return 0;
+}
+
+static inline void snd_sof_unregister_clients(struct snd_sof_dev *sdev)
+{
+ if (sof_ops(sdev) && sof_ops(sdev)->unregister_clients)
+ return sof_ops(sdev)->unregister_clients(sdev);
+}
+
static inline const struct snd_sof_dsp_ops
*sof_get_ops(const struct sof_dev_desc *d,
const struct sof_ops_table mach_ops[], int asize)
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 043fcec5a288..151614224f47 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -249,6 +249,10 @@ struct snd_sof_dsp_ops {
void (*set_mach_params)(const struct snd_soc_acpi_mach *mach,
struct device *dev); /* optional */
+ /* client ops */
+ int (*register_clients)(struct snd_sof_dev *sdev); /* optional */
+ void (*unregister_clients)(struct snd_sof_dev *sdev); /* optional */
+
/* DAI ops */
struct snd_soc_dai_driver *drv;
int num_drv;
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 5/6] ASoC: SOF: Intel: Define ops for client registration
2020-10-01 5:05 [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Dave Ertman
` (3 preceding siblings ...)
2020-10-01 5:05 ` [PATCH 4/6] ASoC: SOF: ops: Add ops for client registration Dave Ertman
@ 2020-10-01 5:05 ` Dave Ertman
2020-10-01 5:05 ` [PATCH 6/6] ASoC: SOF: debug: Remove IPC flood test support in SOF core Dave Ertman
2020-10-03 9:04 ` [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Leon Romanovsky
6 siblings, 0 replies; 48+ messages in thread
From: Dave Ertman @ 2020-10-01 5:05 UTC (permalink / raw)
To: linux-rdma
From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Define client ops for Intel platforms. For now, we only add
2 IPC test clients that will be used for run tandem IPC flood
tests for.
For ACPI platforms, change the Kconfig to select
SND_SOC_SOF_PROBE_WORK_QUEUE to allow the ancillary driver
to probe when the client is registered.
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@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>
Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
---
sound/soc/sof/intel/Kconfig | 9 ++++++
sound/soc/sof/intel/Makefile | 3 ++
sound/soc/sof/intel/apl.c | 18 +++++++++++
sound/soc/sof/intel/bdw.c | 18 +++++++++++
sound/soc/sof/intel/byt.c | 22 ++++++++++++++
sound/soc/sof/intel/cnl.c | 18 +++++++++++
sound/soc/sof/intel/intel-client.c | 49 ++++++++++++++++++++++++++++++
sound/soc/sof/intel/intel-client.h | 26 ++++++++++++++++
8 files changed, 163 insertions(+)
create mode 100644 sound/soc/sof/intel/intel-client.c
create mode 100644 sound/soc/sof/intel/intel-client.h
diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
index 3aaf25e4f766..28aba42f4658 100644
--- a/sound/soc/sof/intel/Kconfig
+++ b/sound/soc/sof/intel/Kconfig
@@ -13,6 +13,8 @@ config SND_SOC_SOF_INTEL_ACPI
def_tristate SND_SOC_SOF_ACPI
select SND_SOC_SOF_BAYTRAIL if SND_SOC_SOF_BAYTRAIL_SUPPORT
select SND_SOC_SOF_BROADWELL if SND_SOC_SOF_BROADWELL_SUPPORT
+ select SND_SOC_SOF_PROBE_WORK_QUEUE if SND_SOC_SOF_CLIENT
+ select SND_SOC_SOF_INTEL_CLIENT if SND_SOC_SOF_CLIENT
help
This option is not user-selectable but automagically handled by
'select' statements at a higher level
@@ -29,6 +31,7 @@ config SND_SOC_SOF_INTEL_PCI
select SND_SOC_SOF_TIGERLAKE if SND_SOC_SOF_TIGERLAKE_SUPPORT
select SND_SOC_SOF_ELKHARTLAKE if SND_SOC_SOF_ELKHARTLAKE_SUPPORT
select SND_SOC_SOF_JASPERLAKE if SND_SOC_SOF_JASPERLAKE_SUPPORT
+ select SND_SOC_SOF_INTEL_CLIENT if SND_SOC_SOF_CLIENT
help
This option is not user-selectable but automagically handled by
'select' statements at a higher level
@@ -57,6 +60,12 @@ config SND_SOC_SOF_INTEL_COMMON
This option is not user-selectable but automagically handled by
'select' statements at a higher level
+config SND_SOC_SOF_INTEL_CLIENT
+ tristate
+ help
+ This option is not user-selectable but automagically handled by
+ 'select' statements at a higher level
+
if SND_SOC_SOF_INTEL_ACPI
config SND_SOC_SOF_BAYTRAIL_SUPPORT
diff --git a/sound/soc/sof/intel/Makefile b/sound/soc/sof/intel/Makefile
index f7e9358f1f06..50e40caaa787 100644
--- a/sound/soc/sof/intel/Makefile
+++ b/sound/soc/sof/intel/Makefile
@@ -5,6 +5,8 @@ snd-sof-intel-bdw-objs := bdw.o
snd-sof-intel-ipc-objs := intel-ipc.o
+snd-sof-intel-client-objs := intel-client.o
+
snd-sof-intel-hda-common-objs := hda.o hda-loader.o hda-stream.o hda-trace.o \
hda-dsp.o hda-ipc.o hda-ctrl.o hda-pcm.o \
hda-dai.o hda-bus.o \
@@ -18,3 +20,4 @@ obj-$(CONFIG_SND_SOC_SOF_BROADWELL) += snd-sof-intel-bdw.o
obj-$(CONFIG_SND_SOC_SOF_INTEL_HIFI_EP_IPC) += snd-sof-intel-ipc.o
obj-$(CONFIG_SND_SOC_SOF_HDA_COMMON) += snd-sof-intel-hda-common.o
obj-$(CONFIG_SND_SOC_SOF_HDA) += snd-sof-intel-hda.o
+obj-$(CONFIG_SND_SOC_SOF_INTEL_CLIENT) += snd-sof-intel-client.o
diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c
index 9e29d4fd393a..b31353b1a3ea 100644
--- a/sound/soc/sof/intel/apl.c
+++ b/sound/soc/sof/intel/apl.c
@@ -15,9 +15,12 @@
* Hardware interface for audio DSP on Apollolake and GeminiLake
*/
+#include <linux/list.h>
#include "../sof-priv.h"
#include "hda.h"
#include "../sof-audio.h"
+#include "../sof-client.h"
+#include "intel-client.h"
static const struct snd_sof_debugfs_map apl_dsp_debugfs[] = {
{"hda", HDA_DSP_HDA_BAR, 0, 0x4000, SOF_DEBUGFS_ACCESS_ALWAYS},
@@ -25,6 +28,16 @@ static const struct snd_sof_debugfs_map apl_dsp_debugfs[] = {
{"dsp", HDA_DSP_BAR, 0, 0x10000, SOF_DEBUGFS_ACCESS_ALWAYS},
};
+static int apl_register_clients(struct snd_sof_dev *sdev)
+{
+ return intel_register_ipc_test_clients(sdev);
+}
+
+static void apl_unregister_clients(struct snd_sof_dev *sdev)
+{
+ intel_unregister_ipc_test_clients(sdev);
+}
+
/* apollolake ops */
const struct snd_sof_dsp_ops sof_apl_ops = {
/* probe and remove */
@@ -101,6 +114,10 @@ const struct snd_sof_dsp_ops sof_apl_ops = {
.trace_release = hda_dsp_trace_release,
.trace_trigger = hda_dsp_trace_trigger,
+ /* client ops */
+ .register_clients = apl_register_clients,
+ .unregister_clients = apl_unregister_clients,
+
/* DAI drivers */
.drv = skl_dai,
.num_drv = SOF_SKL_NUM_DAIS,
@@ -140,3 +157,4 @@ const struct sof_intel_dsp_desc apl_chip_info = {
.ssp_base_offset = APL_SSP_BASE_OFFSET,
};
EXPORT_SYMBOL_NS(apl_chip_info, SND_SOC_SOF_INTEL_HDA_COMMON);
+MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_CLIENT);
diff --git a/sound/soc/sof/intel/bdw.c b/sound/soc/sof/intel/bdw.c
index 99fd0bd7276e..b14026c5fa97 100644
--- a/sound/soc/sof/intel/bdw.c
+++ b/sound/soc/sof/intel/bdw.c
@@ -12,12 +12,15 @@
* Hardware interface for audio DSP on Broadwell
*/
+#include <linux/list.h>
#include <linux/module.h>
#include <sound/sof.h>
#include <sound/sof/xtensa.h>
#include "../ops.h"
#include "shim.h"
#include "../sof-audio.h"
+#include "../sof-client.h"
+#include "intel-client.h"
/* BARs */
#define BDW_DSP_BAR 0
@@ -563,6 +566,16 @@ static void bdw_set_mach_params(const struct snd_soc_acpi_mach *mach,
mach_params->platform = dev_name(dev);
}
+static int bdw_register_clients(struct snd_sof_dev *sdev)
+{
+ return intel_register_ipc_test_clients(sdev);
+}
+
+static void bdw_unregister_clients(struct snd_sof_dev *sdev)
+{
+ intel_unregister_ipc_test_clients(sdev);
+}
+
/* Broadwell DAIs */
static struct snd_soc_dai_driver bdw_dai[] = {
{
@@ -638,6 +651,10 @@ const struct snd_sof_dsp_ops sof_bdw_ops = {
/*Firmware loading */
.load_firmware = snd_sof_load_firmware_memcpy,
+ /* client ops */
+ .register_clients = bdw_register_clients,
+ .unregister_clients = bdw_unregister_clients,
+
/* DAI drivers */
.drv = bdw_dai,
.num_drv = ARRAY_SIZE(bdw_dai),
@@ -662,3 +679,4 @@ EXPORT_SYMBOL_NS(bdw_chip_info, SND_SOC_SOF_BROADWELL);
MODULE_LICENSE("Dual BSD/GPL");
MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC);
MODULE_IMPORT_NS(SND_SOC_SOF_XTENSA);
+MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_CLIENT);
diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c
index 49f67f1b94e0..8951f756d078 100644
--- a/sound/soc/sof/intel/byt.c
+++ b/sound/soc/sof/intel/byt.c
@@ -12,13 +12,16 @@
* Hardware interface for audio DSP on Baytrail, Braswell and Cherrytrail.
*/
+#include <linux/list.h>
#include <linux/module.h>
#include <sound/sof.h>
#include <sound/sof/xtensa.h>
#include "../ops.h"
#include "shim.h"
#include "../sof-audio.h"
+#include "../sof-client.h"
#include "../../intel/common/soc-intel-quirks.h"
+#include "intel-client.h"
/* DSP memories */
#define IRAM_OFFSET 0x0C0000
@@ -821,6 +824,16 @@ static int byt_acpi_probe(struct snd_sof_dev *sdev)
return ret;
}
+static int byt_register_clients(struct snd_sof_dev *sdev)
+{
+ return intel_register_ipc_test_clients(sdev);
+}
+
+static void byt_unregister_clients(struct snd_sof_dev *sdev)
+{
+ intel_unregister_ipc_test_clients(sdev);
+}
+
/* baytrail ops */
const struct snd_sof_dsp_ops sof_byt_ops = {
/* device init */
@@ -879,6 +892,10 @@ const struct snd_sof_dsp_ops sof_byt_ops = {
.suspend = byt_suspend,
.resume = byt_resume,
+ /* client ops */
+ .register_clients = byt_register_clients,
+ .unregister_clients = byt_unregister_clients,
+
/* DAI drivers */
.drv = byt_dai,
.num_drv = 3, /* we have only 3 SSPs on byt*/
@@ -958,6 +975,10 @@ const struct snd_sof_dsp_ops sof_cht_ops = {
.suspend = byt_suspend,
.resume = byt_resume,
+ /* client ops */
+ .register_clients = byt_register_clients,
+ .unregister_clients = byt_unregister_clients,
+
/* DAI drivers */
.drv = byt_dai,
/* all 6 SSPs may be available for cherrytrail */
@@ -985,3 +1006,4 @@ EXPORT_SYMBOL_NS(cht_chip_info, SND_SOC_SOF_BAYTRAIL);
MODULE_LICENSE("Dual BSD/GPL");
MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC);
MODULE_IMPORT_NS(SND_SOC_SOF_XTENSA);
+MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_CLIENT);
diff --git a/sound/soc/sof/intel/cnl.c b/sound/soc/sof/intel/cnl.c
index 16db0f50d139..5d7c2a667798 100644
--- a/sound/soc/sof/intel/cnl.c
+++ b/sound/soc/sof/intel/cnl.c
@@ -15,10 +15,13 @@
* Hardware interface for audio DSP on Cannonlake.
*/
+#include <linux/list.h>
#include "../ops.h"
#include "hda.h"
#include "hda-ipc.h"
#include "../sof-audio.h"
+#include "../sof-client.h"
+#include "intel-client.h"
static const struct snd_sof_debugfs_map cnl_dsp_debugfs[] = {
{"hda", HDA_DSP_HDA_BAR, 0, 0x4000, SOF_DEBUGFS_ACCESS_ALWAYS},
@@ -231,6 +234,16 @@ static void cnl_ipc_dump(struct snd_sof_dev *sdev)
hipcida, hipctdr, hipcctl);
}
+static int cnl_register_clients(struct snd_sof_dev *sdev)
+{
+ return intel_register_ipc_test_clients(sdev);
+}
+
+static void cnl_unregister_clients(struct snd_sof_dev *sdev)
+{
+ intel_unregister_ipc_test_clients(sdev);
+}
+
/* cannonlake ops */
const struct snd_sof_dsp_ops sof_cnl_ops = {
/* probe and remove */
@@ -307,6 +320,10 @@ const struct snd_sof_dsp_ops sof_cnl_ops = {
.trace_release = hda_dsp_trace_release,
.trace_trigger = hda_dsp_trace_trigger,
+ /* client ops */
+ .register_clients = cnl_register_clients,
+ .unregister_clients = cnl_unregister_clients,
+
/* DAI drivers */
.drv = skl_dai,
.num_drv = SOF_SKL_NUM_DAIS,
@@ -417,3 +434,4 @@ const struct sof_intel_dsp_desc jsl_chip_info = {
.ssp_base_offset = CNL_SSP_BASE_OFFSET,
};
EXPORT_SYMBOL_NS(jsl_chip_info, SND_SOC_SOF_INTEL_HDA_COMMON);
+MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_CLIENT);
diff --git a/sound/soc/sof/intel/intel-client.c b/sound/soc/sof/intel/intel-client.c
new file mode 100644
index 000000000000..76811fcf65a9
--- /dev/null
+++ b/sound/soc/sof/intel/intel-client.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright(c) 2020 Intel Corporation. All rights reserved.
+//
+// Author: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
+//
+
+#include <linux/module.h>
+#include "../sof-priv.h"
+#include "../sof-client.h"
+#include "intel-client.h"
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_CLIENT)
+DEFINE_IDA(sof_ipc_test_client_ida);
+
+int intel_register_ipc_test_clients(struct snd_sof_dev *sdev)
+{
+ int ret;
+
+ /*
+ * Register 2 IPC clients to facilitate tandem flood test. The device name below is
+ * appended with the device ID assigned automatically when the ancillary device is
+ * registered making them unique.
+ */
+ ret = sof_client_dev_register(sdev, "ipc_test", &sof_ipc_test_client_ida);
+ if (ret < 0)
+ return ret;
+
+ return sof_client_dev_register(sdev, "ipc_test", &sof_ipc_test_client_ida);
+}
+EXPORT_SYMBOL_NS_GPL(intel_register_ipc_test_clients, SND_SOC_SOF_INTEL_CLIENT);
+
+void intel_unregister_ipc_test_clients(struct snd_sof_dev *sdev)
+{
+ struct sof_client_dev *cdev, *_cdev;
+
+ /* unregister ipc_test clients */
+ list_for_each_entry_safe(cdev, _cdev, &sdev->client_list, list) {
+ if (!strcmp(cdev->ancildev.name, "ipc_test"))
+ sof_client_dev_unregister(cdev);
+ }
+
+ ida_destroy(&sof_ipc_test_client_ida);
+}
+EXPORT_SYMBOL_NS_GPL(intel_unregister_ipc_test_clients, SND_SOC_SOF_INTEL_CLIENT);
+#endif
+
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(SND_SOC_SOF_CLIENT);
diff --git a/sound/soc/sof/intel/intel-client.h b/sound/soc/sof/intel/intel-client.h
new file mode 100644
index 000000000000..49b2c6c0dcc4
--- /dev/null
+++ b/sound/soc/sof/intel/intel-client.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause) */
+/*
+ * This file is provided under a dual BSD/GPLv2 license. When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * Copyright(c) 2020 Intel Corporation. All rights reserved.
+ *
+ * Author: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
+ */
+
+#ifndef __INTEL_CLIENT_H
+#define __INTEL_CLIENT_H
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_CLIENT)
+int intel_register_ipc_test_clients(struct snd_sof_dev *sdev);
+void intel_unregister_ipc_test_clients(struct snd_sof_dev *sdev);
+#else
+static inline int intel_register_ipc_test_clients(struct snd_sof_dev *sdev)
+{
+ return 0;
+}
+
+static void intel_unregister_ipc_test_clients(struct snd_sof_dev *sdev) {}
+#endif
+
+#endif
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 6/6] ASoC: SOF: debug: Remove IPC flood test support in SOF core
2020-10-01 5:05 [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Dave Ertman
` (4 preceding siblings ...)
2020-10-01 5:05 ` [PATCH 5/6] ASoC: SOF: Intel: Define " Dave Ertman
@ 2020-10-01 5:05 ` Dave Ertman
2020-10-03 9:04 ` [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Leon Romanovsky
6 siblings, 0 replies; 48+ messages in thread
From: Dave Ertman @ 2020-10-01 5:05 UTC (permalink / raw)
To: linux-rdma
From: Fred Oh <fred.oh@linux.intel.com>
Remove the IPC flood test support in the SOF core as it is
now added in the IPC flood test client.
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Fred Oh <fred.oh@linux.intel.com>
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
---
sound/soc/sof/Kconfig | 8 --
sound/soc/sof/debug.c | 230 ---------------------------------------
sound/soc/sof/sof-priv.h | 6 +-
3 files changed, 1 insertion(+), 243 deletions(-)
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index 55a2a20c3ec9..4046e96eed92 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -182,14 +182,6 @@ config SND_SOC_SOF_DEBUG_ENABLE_FIRMWARE_TRACE
module parameter (similar to dynamic debug)
If unsure, select "N".
-config SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST
- bool "SOF enable IPC flood test"
- help
- This option enables the IPC flood test which can be used to flood
- the DSP with test IPCs and gather stats about response times.
- Say Y if you want to enable IPC flood test.
- If unsure, select "N".
-
config SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_CLIENT
tristate "SOF enable IPC flood test client"
depends on SND_SOC_SOF_CLIENT
diff --git a/sound/soc/sof/debug.c b/sound/soc/sof/debug.c
index 8e15f105d1d5..d224641768da 100644
--- a/sound/soc/sof/debug.c
+++ b/sound/soc/sof/debug.c
@@ -232,120 +232,10 @@ static int snd_sof_debugfs_probe_item(struct snd_sof_dev *sdev,
}
#endif
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)
-#define MAX_IPC_FLOOD_DURATION_MS 1000
-#define MAX_IPC_FLOOD_COUNT 10000
-#define IPC_FLOOD_TEST_RESULT_LEN 512
-
-static int sof_debug_ipc_flood_test(struct snd_sof_dev *sdev,
- struct snd_sof_dfsentry *dfse,
- bool flood_duration_test,
- unsigned long ipc_duration_ms,
- unsigned long ipc_count)
-{
- struct sof_ipc_cmd_hdr hdr;
- struct sof_ipc_reply reply;
- u64 min_response_time = U64_MAX;
- ktime_t start, end, test_end;
- u64 avg_response_time = 0;
- u64 max_response_time = 0;
- u64 ipc_response_time;
- int i = 0;
- int ret;
-
- /* configure test IPC */
- hdr.cmd = SOF_IPC_GLB_TEST_MSG | SOF_IPC_TEST_IPC_FLOOD;
- hdr.size = sizeof(hdr);
-
- /* set test end time for duration flood test */
- if (flood_duration_test)
- test_end = ktime_get_ns() + ipc_duration_ms * NSEC_PER_MSEC;
-
- /* send test IPC's */
- while (1) {
- start = ktime_get();
- ret = sof_ipc_tx_message(sdev->ipc, hdr.cmd, &hdr, hdr.size,
- &reply, sizeof(reply));
- end = ktime_get();
-
- if (ret < 0)
- break;
-
- /* compute min and max response times */
- ipc_response_time = ktime_to_ns(ktime_sub(end, start));
- min_response_time = min(min_response_time, ipc_response_time);
- max_response_time = max(max_response_time, ipc_response_time);
-
- /* sum up response times */
- avg_response_time += ipc_response_time;
- i++;
-
- /* test complete? */
- if (flood_duration_test) {
- if (ktime_to_ns(end) >= test_end)
- break;
- } else {
- if (i == ipc_count)
- break;
- }
- }
-
- if (ret < 0)
- dev_err(sdev->dev,
- "error: ipc flood test failed at %d iterations\n", i);
-
- /* return if the first IPC fails */
- if (!i)
- return ret;
-
- /* compute average response time */
- do_div(avg_response_time, i);
-
- /* clear previous test output */
- memset(dfse->cache_buf, 0, IPC_FLOOD_TEST_RESULT_LEN);
-
- if (flood_duration_test) {
- dev_dbg(sdev->dev, "IPC Flood test duration: %lums\n",
- ipc_duration_ms);
- snprintf(dfse->cache_buf, IPC_FLOOD_TEST_RESULT_LEN,
- "IPC Flood test duration: %lums\n", ipc_duration_ms);
- }
-
- dev_dbg(sdev->dev,
- "IPC Flood count: %d, Avg response time: %lluns\n",
- i, avg_response_time);
- dev_dbg(sdev->dev, "Max response time: %lluns\n",
- max_response_time);
- dev_dbg(sdev->dev, "Min response time: %lluns\n",
- min_response_time);
-
- /* format output string */
- snprintf(dfse->cache_buf + strlen(dfse->cache_buf),
- IPC_FLOOD_TEST_RESULT_LEN - strlen(dfse->cache_buf),
- "IPC Flood count: %d\nAvg response time: %lluns\n",
- i, avg_response_time);
-
- snprintf(dfse->cache_buf + strlen(dfse->cache_buf),
- IPC_FLOOD_TEST_RESULT_LEN - strlen(dfse->cache_buf),
- "Max response time: %lluns\nMin response time: %lluns\n",
- max_response_time, min_response_time);
-
- return ret;
-}
-#endif
static ssize_t sof_dfsentry_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)
- struct snd_sof_dfsentry *dfse = file->private_data;
- struct snd_sof_dev *sdev = dfse->sdev;
- unsigned long ipc_duration_ms = 0;
- bool flood_duration_test = false;
- unsigned long ipc_count = 0;
- struct dentry *dentry;
- int err;
-#endif
size_t size;
char *string;
int ret;
@@ -357,78 +247,6 @@ static ssize_t sof_dfsentry_write(struct file *file, const char __user *buffer,
size = simple_write_to_buffer(string, count, ppos, buffer, count);
ret = size;
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)
- /*
- * write op is only supported for ipc_flood_count or
- * ipc_flood_duration_ms debugfs entries atm.
- * ipc_flood_count floods the DSP with the number of IPC's specified.
- * ipc_duration_ms test floods the DSP for the time specified
- * in the debugfs entry.
- */
- dentry = file->f_path.dentry;
- if (strcmp(dentry->d_name.name, "ipc_flood_count") &&
- strcmp(dentry->d_name.name, "ipc_flood_duration_ms")) {
- ret = -EINVAL;
- goto out;
- }
-
- if (!strcmp(dentry->d_name.name, "ipc_flood_duration_ms"))
- flood_duration_test = true;
-
- /* test completion criterion */
- if (flood_duration_test)
- ret = kstrtoul(string, 0, &ipc_duration_ms);
- else
- ret = kstrtoul(string, 0, &ipc_count);
- if (ret < 0)
- goto out;
-
- /* limit max duration/ipc count for flood test */
- if (flood_duration_test) {
- if (!ipc_duration_ms) {
- ret = size;
- goto out;
- }
-
- /* find the minimum. min() is not used to avoid warnings */
- if (ipc_duration_ms > MAX_IPC_FLOOD_DURATION_MS)
- ipc_duration_ms = MAX_IPC_FLOOD_DURATION_MS;
- } else {
- if (!ipc_count) {
- ret = size;
- goto out;
- }
-
- /* find the minimum. min() is not used to avoid warnings */
- if (ipc_count > MAX_IPC_FLOOD_COUNT)
- ipc_count = MAX_IPC_FLOOD_COUNT;
- }
-
- ret = pm_runtime_get_sync(sdev->dev);
- if (ret < 0) {
- dev_err_ratelimited(sdev->dev,
- "error: debugfs write failed to resume %d\n",
- ret);
- pm_runtime_put_noidle(sdev->dev);
- goto out;
- }
-
- /* flood test */
- ret = sof_debug_ipc_flood_test(sdev, dfse, flood_duration_test,
- ipc_duration_ms, ipc_count);
-
- pm_runtime_mark_last_busy(sdev->dev);
- err = pm_runtime_put_autosuspend(sdev->dev);
- if (err < 0)
- dev_err_ratelimited(sdev->dev,
- "error: debugfs write failed to idle %d\n",
- err);
-
- /* return size if test is successful */
- if (ret >= 0)
- ret = size;
-out:
-#endif
kfree(string);
return ret;
}
@@ -444,25 +262,6 @@ static ssize_t sof_dfsentry_read(struct file *file, char __user *buffer,
int size;
u8 *buf;
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)
- struct dentry *dentry;
-
- dentry = file->f_path.dentry;
- if ((!strcmp(dentry->d_name.name, "ipc_flood_count") ||
- !strcmp(dentry->d_name.name, "ipc_flood_duration_ms")) &&
- dfse->cache_buf) {
- if (*ppos)
- return 0;
-
- count = strlen(dfse->cache_buf);
- size_ret = copy_to_user(buffer, dfse->cache_buf, count);
- if (size_ret)
- return -EFAULT;
-
- *ppos += count;
- return count;
- }
-#endif
size = dfse->size;
/* validate position & count */
@@ -606,17 +405,6 @@ int snd_sof_debugfs_buf_item(struct snd_sof_dev *sdev,
dfse->size = size;
dfse->sdev = sdev;
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)
- /*
- * cache_buf is unused for SOF_DFSENTRY_TYPE_BUF debugfs entries.
- * So, use it to save the results of the last IPC flood test.
- */
- dfse->cache_buf = devm_kzalloc(sdev->dev, IPC_FLOOD_TEST_RESULT_LEN,
- GFP_KERNEL);
- if (!dfse->cache_buf)
- return -ENOMEM;
-#endif
-
debugfs_create_file(name, mode, sdev->debugfs_root, dfse,
&sof_dfs_fops);
/* add to dfsentry list */
@@ -662,24 +450,6 @@ int snd_sof_dbg_init(struct snd_sof_dev *sdev)
return err;
#endif
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)
- /* create read-write ipc_flood_count debugfs entry */
- err = snd_sof_debugfs_buf_item(sdev, NULL, 0,
- "ipc_flood_count", 0666);
-
- /* errors are only due to memory allocation, not debugfs */
- if (err < 0)
- return err;
-
- /* create read-write ipc_flood_duration_ms debugfs entry */
- err = snd_sof_debugfs_buf_item(sdev, NULL, 0,
- "ipc_flood_duration_ms", 0666);
-
- /* errors are only due to memory allocation, not debugfs */
- if (err < 0)
- return err;
-#endif
-
return 0;
}
EXPORT_SYMBOL_GPL(snd_sof_dbg_init);
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 151614224f47..ece5fce97460 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -50,10 +50,6 @@ extern int sof_core_debug;
#define SOF_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE | \
SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_FLOAT)
-#define ENABLE_DEBUGFS_CACHEBUF \
- (IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_ENABLE_DEBUGFS_CACHE) || \
- IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST))
-
/* DSP power state */
enum sof_dsp_power_states {
SOF_DSP_PM_D0,
@@ -298,7 +294,7 @@ struct snd_sof_dfsentry {
* or if it is accessible only when the DSP is in D0.
*/
enum sof_debugfs_access_type access_type;
-#if ENABLE_DEBUGFS_CACHEBUF
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_ENABLE_DEBUGFS_CACHE)
char *cache_buf; /* buffer to cache the contents of debugfs memory */
#endif
struct snd_sof_dev *sdev;
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 5:05 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
@ 2020-10-01 8:32 ` Leon Romanovsky
2020-10-01 17:20 ` Ertman, David M
0 siblings, 1 reply; 48+ messages in thread
From: Leon Romanovsky @ 2020-10-01 8:32 UTC (permalink / raw)
To: Dave Ertman; +Cc: linux-rdma
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
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH 1/6] Add ancillary bus support
2020-10-01 8:32 ` Leon Romanovsky
@ 2020-10-01 17:20 ` Ertman, David M
2020-10-01 17:40 ` Leon Romanovsky
0 siblings, 1 reply; 48+ messages in thread
From: Ertman, David M @ 2020-10-01 17:20 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: linux-rdma
> -----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.
-DaveE
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 17:20 ` Ertman, David M
@ 2020-10-01 17:40 ` Leon Romanovsky
2020-10-01 18:29 ` Parav Pandit
0 siblings, 1 reply; 48+ messages in thread
From: Leon Romanovsky @ 2020-10-01 17:40 UTC (permalink / raw)
To: Ertman, David M; +Cc: linux-rdma
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
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH 1/6] Add ancillary bus support
2020-10-01 17:40 ` Leon Romanovsky
@ 2020-10-01 18:29 ` Parav Pandit
2020-10-01 19:32 ` Leon Romanovsky
0 siblings, 1 reply; 48+ messages in thread
From: Parav Pandit @ 2020-10-01 18:29 UTC (permalink / raw)
To: Leon Romanovsky, Ertman, David M; +Cc: linux-rdma
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Thursday, October 1, 2020 11:10 PM
>
> 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()
>
device_initialize(), device_add() and device_unregister() is the pattern widely followed in the core.
> 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.
For one mlx5 (pf/vf/sf) device, there are three class erivers (ib, vdpa, en).
So there should be one ida allocate per mlx5 device type.
Static const mlx5_adev_names[MLX5_INTERFACE_PROTOCOL_MAX] = {
"rdma",
"eth",
"vdpa"
};
Something like for current mlx5_register_device(),
mlx5_register_device()
{
id = ida_alloc(0, UINT_MAX, GFP_KERNEL);
for (i = 0; I < MLX5_INTERFACE_PROTOCOL_MAX; i++) {
adev = kzalloc(sizeof(*adev), GFP_KERNEL);
adev.name = mlx5_adev_names[i];
adev.id = ret;
adev.dev.parent = mlx5_core_dev->device;
adev->coredev = mlx5_core_dev;
ret = ancillary_device_initialize(&adev);
ret = ancillary_device_register(adev);
}
This will create 3 ancillary devices for each PCI PF/VF/SF.
mlx5_core.rdma.1
mlx5_core.eth.1
mlx5_core.vdpa.1
and mlx5_ib driver will do
ancillary_driver_register()
{
For ID of mlx5_core.rdma.
}
mlx5_vdpa driver does,
ancillary_driver_register()
{
For ID of mlx5_core.vdpa
}
This is uniform for pf/vf/sf for one or more all protocols.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 18:29 ` Parav Pandit
@ 2020-10-01 19:32 ` Leon Romanovsky
2020-10-02 5:29 ` Parav Pandit
0 siblings, 1 reply; 48+ messages in thread
From: Leon Romanovsky @ 2020-10-01 19:32 UTC (permalink / raw)
To: Parav Pandit; +Cc: Ertman, David M, linux-rdma
On Thu, Oct 01, 2020 at 06:29:22PM +0000, Parav Pandit wrote:
>
>
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Thursday, October 1, 2020 11:10 PM
> >
> > 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()
> >
> device_initialize(), device_add() and device_unregister() is the pattern widely followed in the core.
It doesn't mean that I need to agree with that, right?
>
> > 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.
>
> For one mlx5 (pf/vf/sf) device, there are three class erivers (ib, vdpa, en).
>
> So there should be one ida allocate per mlx5 device type.
>
> Static const mlx5_adev_names[MLX5_INTERFACE_PROTOCOL_MAX] = {
> "rdma",
> "eth",
> "vdpa"
> };
>
> Something like for current mlx5_register_device(),
I know it and already implemented it, this is why I'm saying that it is
not what I would expect from the implementation.
It is wrong create mlx5_core.rdma.1 device that is equal to mlx5_core.eth.1
just to connect our mlx5_ib.ko to it, while documentation explains about
creating single PCI code device and other drivers connect to it.
Thanks
> mlx5_register_device()
> {
> id = ida_alloc(0, UINT_MAX, GFP_KERNEL);
>
> for (i = 0; I < MLX5_INTERFACE_PROTOCOL_MAX; i++) {
> adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> adev.name = mlx5_adev_names[i];
> adev.id = ret;
> adev.dev.parent = mlx5_core_dev->device;
> adev->coredev = mlx5_core_dev;
> ret = ancillary_device_initialize(&adev);
> ret = ancillary_device_register(adev);
> }
>
> This will create 3 ancillary devices for each PCI PF/VF/SF.
> mlx5_core.rdma.1
> mlx5_core.eth.1
> mlx5_core.vdpa.1
>
> and mlx5_ib driver will do
>
> ancillary_driver_register()
> {
> For ID of mlx5_core.rdma.
> }
>
> mlx5_vdpa driver does,
>
> ancillary_driver_register()
> {
> For ID of mlx5_core.vdpa
> }
>
> This is uniform for pf/vf/sf for one or more all protocols.
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH 1/6] Add ancillary bus support
2020-10-01 19:32 ` Leon Romanovsky
@ 2020-10-02 5:29 ` Parav Pandit
2020-10-02 6:20 ` Leon Romanovsky
0 siblings, 1 reply; 48+ messages in thread
From: Parav Pandit @ 2020-10-02 5:29 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Ertman, David M, linux-rdma
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Friday, October 2, 2020 1:02 AM
> > > > > > +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()
> > >
> > device_initialize(), device_add() and device_unregister() is the pattern widely
> followed in the core.
>
> It doesn't mean that I need to agree with that, right?
>
Right. May be I misunderstood your comment where you said "I expect device_register() and device_add()" in response to "device_initialize() and device_add".
I interpreted your comment as to replace initialize() with register().
Because that is odd naming and completely out of sync from the core APIs.
A helper like below that wraps initialize() and add() is buggy, because when register() returns an error, it doesn't know if should do kfree() or put_device().
ancillary_device_register(adev)
{
ret = ancillary_device_initialize();
if (ret)
return ret;
ret = ancillary_device_add();
if (ret)
put_device(dev);
return ret;
}
> >
> > > 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.
> >
> > For one mlx5 (pf/vf/sf) device, there are three class erivers (ib, vdpa, en).
> >
> > So there should be one ida allocate per mlx5 device type.
> >
> > Static const mlx5_adev_names[MLX5_INTERFACE_PROTOCOL_MAX] = {
> > "rdma",
> > "eth",
> > "vdpa"
> > };
> >
> > Something like for current mlx5_register_device(),
>
> I know it and already implemented it, this is why I'm saying that it is not what I
> would expect from the implementation.
>
> It is wrong create mlx5_core.rdma.1 device that is equal to mlx5_core.eth.1 just
> to connect our mlx5_ib.ko to it, while documentation explains about creating
Ancillary bus's documentation? If so, it should be corrected.
Do you have specific text snippet to point to that should be fixed?
For purpose of matching service functionality, for each different class (ib, eth, vdpa) there is one ancillary device created.
What exactly is wrong here? (and why is it wrong now and not in previous version of the RFC?)
Do you want to create one device and 3 drivers to bind to it? If you think that way, may be pci core should be extended to support that, ancillary bus may not be required.
But that is different solution than what is being proposed here.
Not sure I understand your comment about wrong create mlx5_core.rdma1 equal to core.eth.1", because they are not equal.
To begin with, they have different id for matching service, and in future it should be extended to pass 'only' needed info.
mlx5_core exposing the 'whole' mlx5_core_dev to other drivers doesn't look correct.
> single PCI code device and other drivers connect to it.
>
> Thanks
>
>
> > mlx5_register_device()
> > {
> > id = ida_alloc(0, UINT_MAX, GFP_KERNEL);
> >
> > for (i = 0; I < MLX5_INTERFACE_PROTOCOL_MAX; i++) {
> > adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> > adev.name = mlx5_adev_names[i];
> > adev.id = ret;
> > adev.dev.parent = mlx5_core_dev->device;
> > adev->coredev = mlx5_core_dev;
> > ret = ancillary_device_initialize(&adev);
> > ret = ancillary_device_register(adev); }
> >
> > This will create 3 ancillary devices for each PCI PF/VF/SF.
> > mlx5_core.rdma.1
> > mlx5_core.eth.1
> > mlx5_core.vdpa.1
> >
> > and mlx5_ib driver will do
> >
> > ancillary_driver_register()
> > {
> > For ID of mlx5_core.rdma.
> > }
> >
> > mlx5_vdpa driver does,
> >
> > ancillary_driver_register()
> > {
> > For ID of mlx5_core.vdpa
> > }
> >
> > This is uniform for pf/vf/sf for one or more all protocols.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-02 5:29 ` Parav Pandit
@ 2020-10-02 6:20 ` Leon Romanovsky
2020-10-02 8:42 ` Parav Pandit
0 siblings, 1 reply; 48+ messages in thread
From: Leon Romanovsky @ 2020-10-02 6:20 UTC (permalink / raw)
To: Parav Pandit; +Cc: Ertman, David M, linux-rdma
On Fri, Oct 02, 2020 at 05:29:26AM +0000, Parav Pandit wrote:
>
>
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Friday, October 2, 2020 1:02 AM
>
> > > > > > > +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()
> > > >
> > > device_initialize(), device_add() and device_unregister() is the pattern widely
> > followed in the core.
> >
> > It doesn't mean that I need to agree with that, right?
> >
> Right. May be I misunderstood your comment where you said "I expect device_register() and device_add()" in response to "device_initialize() and device_add".
> I interpreted your comment as to replace initialize() with register().
> Because that is odd naming and completely out of sync from the core APIs.
>
> A helper like below that wraps initialize() and add() is buggy, because when register() returns an error, it doesn't know if should do kfree() or put_device().
I wrote above (14 lines above this line) that I understand and support
the request to separate init and add parts. There is only one thing that
I didn't like that we have _unregister() but don't have _register().
It is not a big deal.
>
> ancillary_device_register(adev)
> {
> ret = ancillary_device_initialize();
> if (ret)
> return ret;
>
> ret = ancillary_device_add();
> if (ret)
> put_device(dev);
> return ret;
> }
>
> > >
> > > > 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.
> > >
> > > For one mlx5 (pf/vf/sf) device, there are three class erivers (ib, vdpa, en).
> > >
> > > So there should be one ida allocate per mlx5 device type.
> > >
> > > Static const mlx5_adev_names[MLX5_INTERFACE_PROTOCOL_MAX] = {
> > > "rdma",
> > > "eth",
> > > "vdpa"
> > > };
> > >
> > > Something like for current mlx5_register_device(),
> >
> > I know it and already implemented it, this is why I'm saying that it is not what I
> > would expect from the implementation.
> >
> > It is wrong create mlx5_core.rdma.1 device that is equal to mlx5_core.eth.1 just
> > to connect our mlx5_ib.ko to it, while documentation explains about creating
>
> Ancillary bus's documentation? If so, it should be corrected.
> Do you have specific text snippet to point to that should be fixed?
+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.
>
> For purpose of matching service functionality, for each different class (ib, eth, vdpa) there is one ancillary device created.
> What exactly is wrong here? (and why is it wrong now and not in previous version of the RFC?)
Here the example of two real systems, see how many links we created to
same mlx5_core PCI logic. I imagine that Qlogic and Chelsio drivers will
look even worse, because they spread over more subsystems than mlx5.
This is first time when I see real implementation of real device.
System with 2 IB and 1 RoCE cards:
[leonro@vm ~]$ ls -l /sys/bus/ancillary/devices/
mlx5_core.eth.0 -> ../../../devices/pci0000:00/0000:00:09.0/mlx5_core.eth.0
mlx5_core.eth.1 -> ../../../devices/pci0000:00/0000:00:0a.0/mlx5_core.eth.1
mlx5_core.eth.2 -> ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.eth.2
mlx5_core.ib.0 -> ../../../devices/pci0000:00/0000:00:09.0/mlx5_core.ib.0
mlx5_core.ib.1 -> ../../../devices/pci0000:00/0000:00:0a.0/mlx5_core.ib.1
mlx5_core.ib.2 -> ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.ib.2
mlx5_core.vdpa.0 -> ../../../devices/pci0000:00/0000:00:09.0/mlx5_core.ib.0
mlx5_core.vdpa.1 -> ../../../devices/pci0000:00/0000:00:0a.0/mlx5_core.ib.1
mlx5_core.vdpa.2 -> ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.ib.2
[leonro@vm ~]$ rdma dev
0: ibp0s9: node_type ca fw 4.6.9999 node_guid 5254:00c0:fe12:3455 sys_image_guid 5254:00c0:fe12:3455
1: ibp0s10: node_type ca fw 4.6.9999 node_guid 5254:00c0:fe12:3456 sys_image_guid 5254:00c0:fe12:3456
2: rdmap0s11: node_type ca fw 4.6.9999 node_guid 5254:00c0:fe12:3457 sys_image_guid 5254:00c0:fe12:3457
System with RoCE SR-IOV card with 4 VFs:
[leonro@vm ~]$ ls -l /sys/bus/ancillary/devices/
mlx5_core.eth.0 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.0/mlx5_core.eth.0
mlx5_core.eth.1 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.1/mlx5_core.eth.1
mlx5_core.eth.2 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.2/mlx5_core.eth.2
mlx5_core.eth.3 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.3/mlx5_core.eth.3
mlx5_core.eth.4 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.4/mlx5_core.eth.4
mlx5_core.ib.0 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.0/mlx5_core.ib.0
mlx5_core.ib.1 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.1/mlx5_core.ib.1
mlx5_core.ib.2 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.2/mlx5_core.ib.2
mlx5_core.ib.3 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.3/mlx5_core.ib.3
mlx5_core.ib.4 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.4/mlx5_core.ib.4
mlx5_core.vdpa.0 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.0/mlx5_core.ib.0
mlx5_core.vdpa.1 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.1/mlx5_core.ib.1
mlx5_core.vdpa.2 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.2/mlx5_core.ib.2
mlx5_core.vdpa.3 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.3/mlx5_core.ib.3
mlx5_core.vdpa.4 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.4/mlx5_core.ib.4
[leonro@vm ~]$ rdma dev
0: rocep1s0f0: node_type ca fw 4.6.9999 node_guid 5254:00c0:fe12:3455 sys_image_guid 5254:00c0:fe12:3455
1: rocep1s0f0v0: node_type ca fw 4.6.9999 node_guid 0000:0000:0000:0000 sys_image_guid 5254:00c0:fe12:3456
2: rocep1s0f0v1: node_type ca fw 4.6.9999 node_guid 0000:0000:0000:0000 sys_image_guid 5254:00c0:fe12:3457
3: rocep1s0f0v2: node_type ca fw 4.6.9999 node_guid 0000:0000:0000:0000 sys_image_guid 5254:00c0:fe12:3458
4: rocep1s0f0v3: node_type ca fw 4.6.9999 node_guid 0000:0000:0000:0000 sys_image_guid 5254:00c0:fe12:3459
>
> Do you want to create one device and 3 drivers to bind to it? If you think that way, may be pci core should be extended to support that, ancillary bus may not be required.
> But that is different solution than what is being proposed here.
This is my expectation, I see this virtual bus as glue logic for the
drivers between different subsystems.
> Not sure I understand your comment about wrong create mlx5_core.rdma1 equal to core.eth.1", because they are not equal.
> To begin with, they have different id for matching service, and in future it should be extended to pass 'only' needed info.
> mlx5_core exposing the 'whole' mlx5_core_dev to other drivers doesn't look correct.
Maybe, what I would like to see is:
[leonro@vm ~]$ ls -l /sys/bus/ancillary/devices/
mlx5_core.pf.0 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.0/mlx5_core.pf.0
mlx5_core.vf.0 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.1/mlx5_core.vf.0
mlx5_core.vf.1 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.2/mlx5_core.vf.1
mlx5_core.vf.2 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.3/mlx5_core.vf.2
mlx5_core.vf.3 -> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.4/mlx5_core.vf.3
The mlx5_ib, mlx5_vdpa and mlx5_en will connect to one of mlx5_core.XX.YY devices.
Thanks
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH 1/6] Add ancillary bus support
2020-10-02 6:20 ` Leon Romanovsky
@ 2020-10-02 8:42 ` Parav Pandit
2020-10-02 11:13 ` Leon Romanovsky
0 siblings, 1 reply; 48+ messages in thread
From: Parav Pandit @ 2020-10-02 8:42 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Ertman, David M, linux-rdma
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Friday, October 2, 2020 11:50 AM
>
> On Fri, Oct 02, 2020 at 05:29:26AM +0000, Parav Pandit wrote:
> >
> >
> > > From: Leon Romanovsky <leon@kernel.org>
> > > Sent: Friday, October 2, 2020 1:02 AM
> >
> > > > > > > > +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()
> > > > >
> > > > device_initialize(), device_add() and device_unregister() is the
> > > > pattern widely
> > > followed in the core.
> > >
> > > It doesn't mean that I need to agree with that, right?
> > >
> > Right. May be I misunderstood your comment where you said "I expect
> device_register() and device_add()" in response to "device_initialize() and
> device_add".
> > I interpreted your comment as to replace initialize() with register().
> > Because that is odd naming and completely out of sync from the core APIs.
> >
> > A helper like below that wraps initialize() and add() is buggy, because when
> register() returns an error, it doesn't know if should do kfree() or
> put_device().
>
> I wrote above (14 lines above this line) that I understand and support the
> request to separate init and add parts. There is only one thing that I didn't
> like that we have _unregister() but don't have _register().
> It is not a big deal.
>
> >
> > ancillary_device_register(adev)
> > {
> > ret = ancillary_device_initialize();
> > if (ret)
> > return ret;
> >
> > ret = ancillary_device_add();
> > if (ret)
> > put_device(dev);
> > return ret;
> > }
> >
> > > >
> > > > > 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.
> > > >
> > > > For one mlx5 (pf/vf/sf) device, there are three class erivers (ib, vdpa,
> en).
> > > >
> > > > So there should be one ida allocate per mlx5 device type.
> > > >
> > > > Static const mlx5_adev_names[MLX5_INTERFACE_PROTOCOL_MAX] =
> {
> > > > "rdma",
> > > > "eth",
> > > > "vdpa"
> > > > };
> > > >
> > > > Something like for current mlx5_register_device(),
> > >
> > > I know it and already implemented it, this is why I'm saying that it
> > > is not what I would expect from the implementation.
> > >
> > > It is wrong create mlx5_core.rdma.1 device that is equal to
> > > mlx5_core.eth.1 just to connect our mlx5_ib.ko to it, while
> > > documentation explains about creating
> >
> > Ancillary bus's documentation? If so, it should be corrected.
> > Do you have specific text snippet to point to that should be fixed?
>
> +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.
>
> >
> > For purpose of matching service functionality, for each different class (ib,
> eth, vdpa) there is one ancillary device created.
> > What exactly is wrong here? (and why is it wrong now and not in
> > previous version of the RFC?)
>
> Here the example of two real systems, see how many links we created to
> same mlx5_core PCI logic. I imagine that Qlogic and Chelsio drivers will look
> even worse, because they spread over more subsystems than mlx5.
>
> This is first time when I see real implementation of real device.
>
> System with 2 IB and 1 RoCE cards:
> [leonro@vm ~]$ ls -l /sys/bus/ancillary/devices/
> mlx5_core.eth.0 -> ../../../devices/pci0000:00/0000:00:09.0/mlx5_core.eth.0
> mlx5_core.eth.1 -> ../../../devices/pci0000:00/0000:00:0a.0/mlx5_core.eth.1
> mlx5_core.eth.2 -> ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.eth.2
This gives you the ability to not load the netdevice and rdma device of a VF and only load the vdpa device.
These are real use case that users have asked for.
In use case one, they are only interested in rdma device.
In second use case only vdpa device.
How shall one achieve that without spinning of the device for each class?
> mlx5_core.ib.0 -> ../../../devices/pci0000:00/0000:00:09.0/mlx5_core.ib.0
> mlx5_core.ib.1 -> ../../../devices/pci0000:00/0000:00:0a.0/mlx5_core.ib.1
> mlx5_core.ib.2 -> ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.ib.2
> mlx5_core.vdpa.0 -> ../../../devices/pci0000:00/0000:00:09.0/mlx5_core.ib.0
> mlx5_core.vdpa.1 -> ../../../devices/pci0000:00/0000:00:0a.0/mlx5_core.ib.1
> mlx5_core.vdpa.2 -> ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.ib.2
> [leonro@vm ~]$ rdma dev
> 0: ibp0s9: node_type ca fw 4.6.9999 node_guid 5254:00c0:fe12:3455
> sys_image_guid 5254:00c0:fe12:3455
> 1: ibp0s10: node_type ca fw 4.6.9999 node_guid 5254:00c0:fe12:3456
> sys_image_guid 5254:00c0:fe12:3456
> 2: rdmap0s11: node_type ca fw 4.6.9999 node_guid 5254:00c0:fe12:3457
> sys_image_guid 5254:00c0:fe12:3457
>
> System with RoCE SR-IOV card with 4 VFs:
>
> Maybe, what I would like to see is:
> [leonro@vm ~]$ ls -l /sys/bus/ancillary/devices/
> mlx5_core.pf.0 ->
> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.0/mlx5_core.pf.0
> mlx5_core.vf.0 ->
> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.1/mlx5_core.vf.0
> mlx5_core.vf.1 ->
> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.2/mlx5_core.vf.1
> mlx5_core.vf.2 ->
> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.3/mlx5_core.vf.2
> mlx5_core.vf.3 ->
> ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.4/mlx5_core.vf.3
>
> The mlx5_ib, mlx5_vdpa and mlx5_en will connect to one of mlx5_core.XX.YY
> devices.
In that case, I really don't see the need of creating ancillary device at all.
A generic wrapper around blocking_notifier_chain_register() with a notion of id, and some well defined structure as library can serve the purpose.
But it will miss out power suspend() resume() hooks, which we get for free now using ancillary device, in addition to the ability to selectively load only few class device.
Each 'struct device's is close to 744 bytes in size in 5.9 kernel.
Is so many 'struct device' of ancillary type a real problem, that aims to address these use cases?
>
> Thanks
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-02 8:42 ` Parav Pandit
@ 2020-10-02 11:13 ` Leon Romanovsky
2020-10-02 11:27 ` Parav Pandit
0 siblings, 1 reply; 48+ messages in thread
From: Leon Romanovsky @ 2020-10-02 11:13 UTC (permalink / raw)
To: Parav Pandit; +Cc: Ertman, David M, linux-rdma
On Fri, Oct 02, 2020 at 08:42:13AM +0000, Parav Pandit wrote:
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Friday, October 2, 2020 11:50 AM
> >
> > On Fri, Oct 02, 2020 at 05:29:26AM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Leon Romanovsky <leon@kernel.org>
> > > > Sent: Friday, October 2, 2020 1:02 AM
> > >
> > > > > > > > > +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()
> > > > > >
> > > > > device_initialize(), device_add() and device_unregister() is the
> > > > > pattern widely
> > > > followed in the core.
> > > >
> > > > It doesn't mean that I need to agree with that, right?
> > > >
> > > Right. May be I misunderstood your comment where you said "I expect
> > device_register() and device_add()" in response to "device_initialize() and
> > device_add".
> > > I interpreted your comment as to replace initialize() with register().
> > > Because that is odd naming and completely out of sync from the core APIs.
> > >
> > > A helper like below that wraps initialize() and add() is buggy, because when
> > register() returns an error, it doesn't know if should do kfree() or
> > put_device().
> >
> > I wrote above (14 lines above this line) that I understand and support the
> > request to separate init and add parts. There is only one thing that I didn't
> > like that we have _unregister() but don't have _register().
> > It is not a big deal.
> >
> > >
> > > ancillary_device_register(adev)
> > > {
> > > ret = ancillary_device_initialize();
> > > if (ret)
> > > return ret;
> > >
> > > ret = ancillary_device_add();
> > > if (ret)
> > > put_device(dev);
> > > return ret;
> > > }
> > >
> > > > >
> > > > > > 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.
> > > > >
> > > > > For one mlx5 (pf/vf/sf) device, there are three class erivers (ib, vdpa,
> > en).
> > > > >
> > > > > So there should be one ida allocate per mlx5 device type.
> > > > >
> > > > > Static const mlx5_adev_names[MLX5_INTERFACE_PROTOCOL_MAX] =
> > {
> > > > > "rdma",
> > > > > "eth",
> > > > > "vdpa"
> > > > > };
> > > > >
> > > > > Something like for current mlx5_register_device(),
> > > >
> > > > I know it and already implemented it, this is why I'm saying that it
> > > > is not what I would expect from the implementation.
> > > >
> > > > It is wrong create mlx5_core.rdma.1 device that is equal to
> > > > mlx5_core.eth.1 just to connect our mlx5_ib.ko to it, while
> > > > documentation explains about creating
> > >
> > > Ancillary bus's documentation? If so, it should be corrected.
> > > Do you have specific text snippet to point to that should be fixed?
> >
> > +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.
> >
> > >
> > > For purpose of matching service functionality, for each different class (ib,
> > eth, vdpa) there is one ancillary device created.
> > > What exactly is wrong here? (and why is it wrong now and not in
> > > previous version of the RFC?)
> >
> > Here the example of two real systems, see how many links we created to
> > same mlx5_core PCI logic. I imagine that Qlogic and Chelsio drivers will look
> > even worse, because they spread over more subsystems than mlx5.
> >
> > This is first time when I see real implementation of real device.
> >
> > System with 2 IB and 1 RoCE cards:
> > [leonro@vm ~]$ ls -l /sys/bus/ancillary/devices/
> > mlx5_core.eth.0 -> ../../../devices/pci0000:00/0000:00:09.0/mlx5_core.eth.0
> > mlx5_core.eth.1 -> ../../../devices/pci0000:00/0000:00:0a.0/mlx5_core.eth.1
> > mlx5_core.eth.2 -> ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.eth.2
> This gives you the ability to not load the netdevice and rdma device of a VF and only load the vdpa device.
> These are real use case that users have asked for.
> In use case one, they are only interested in rdma device.
> In second use case only vdpa device.
> How shall one achieve that without spinning of the device for each class?
Why will it be different if ancillary device is small PCI core?
If you want RDMA, you will use specific ancillary driver that connects
to that small PCI logic.
>
> > mlx5_core.ib.0 -> ../../../devices/pci0000:00/0000:00:09.0/mlx5_core.ib.0
> > mlx5_core.ib.1 -> ../../../devices/pci0000:00/0000:00:0a.0/mlx5_core.ib.1
> > mlx5_core.ib.2 -> ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.ib.2
> > mlx5_core.vdpa.0 -> ../../../devices/pci0000:00/0000:00:09.0/mlx5_core.ib.0
> > mlx5_core.vdpa.1 -> ../../../devices/pci0000:00/0000:00:0a.0/mlx5_core.ib.1
> > mlx5_core.vdpa.2 -> ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.ib.2
> > [leonro@vm ~]$ rdma dev
> > 0: ibp0s9: node_type ca fw 4.6.9999 node_guid 5254:00c0:fe12:3455
> > sys_image_guid 5254:00c0:fe12:3455
> > 1: ibp0s10: node_type ca fw 4.6.9999 node_guid 5254:00c0:fe12:3456
> > sys_image_guid 5254:00c0:fe12:3456
> > 2: rdmap0s11: node_type ca fw 4.6.9999 node_guid 5254:00c0:fe12:3457
> > sys_image_guid 5254:00c0:fe12:3457
> >
> > System with RoCE SR-IOV card with 4 VFs:
> >
> > Maybe, what I would like to see is:
> > [leonro@vm ~]$ ls -l /sys/bus/ancillary/devices/
> > mlx5_core.pf.0 ->
> > ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.0/mlx5_core.pf.0
> > mlx5_core.vf.0 ->
> > ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.1/mlx5_core.vf.0
> > mlx5_core.vf.1 ->
> > ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.2/mlx5_core.vf.1
> > mlx5_core.vf.2 ->
> > ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.3/mlx5_core.vf.2
> > mlx5_core.vf.3 ->
> > ../../../devices/pci0000:00/0000:00:09.0/0000:01:00.4/mlx5_core.vf.3
> >
> > The mlx5_ib, mlx5_vdpa and mlx5_en will connect to one of mlx5_core.XX.YY
> > devices.
> In that case, I really don't see the need of creating ancillary device at all.
> A generic wrapper around blocking_notifier_chain_register() with a notion of id, and some well defined structure as library can serve the purpose.
Ancillary bus provides generic implementation instead of existing copy/paste code.
> But it will miss out power suspend() resume() hooks, which we get for free now using ancillary device, in addition to the ability to selectively load only few class device.
While I'm interested to load specific device and limit supported
classes later and not vice-versa as it is now.
> Each 'struct device's is close to 744 bytes in size in 5.9 kernel.
> Is so many 'struct device' of ancillary type a real problem, that aims to address these use cases?
Being nice to the users and provide clean abstraction are important
goals too.
Thanks
>
> >
> > Thanks
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH 1/6] Add ancillary bus support
2020-10-02 11:13 ` Leon Romanovsky
@ 2020-10-02 11:27 ` Parav Pandit
2020-10-02 11:45 ` Leon Romanovsky
0 siblings, 1 reply; 48+ messages in thread
From: Parav Pandit @ 2020-10-02 11:27 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Ertman, David M, linux-rdma
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Friday, October 2, 2020 4:44 PM
[..]
> > > ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.eth.2
> > This gives you the ability to not load the netdevice and rdma device of a VF
> and only load the vdpa device.
> > These are real use case that users have asked for.
> > In use case one, they are only interested in rdma device.
> > In second use case only vdpa device.
> > How shall one achieve that without spinning of the device for each class?
>
> Why will it be different if ancillary device is small PCI core?
> If you want RDMA, you will use specific ancillary driver that connects to that
> small PCI logic.
I didn't follow, wwhat is PCI core and PCI logic in this context?
Not sure if you understood the use case.
Let me try again.
Let say there are 4 VFs enabled.
User would not like to create netdev for 3 VFs (0 to 2) ; user only wants rdma device for these VFs 0 to 2.
User wants only vdpa device for 4th VF.
User doesn't want to create rdma device and netdevice for the 4th VF.
How one shall achieve this?
It is easily achievable with current ancillary device instantiation per class proposal.
> Being nice to the users and provide clean abstraction are important goals too.
Which part of this makes not_nice_to_users and what is not abstracted.
I lost you.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-02 11:27 ` Parav Pandit
@ 2020-10-02 11:45 ` Leon Romanovsky
2020-10-02 11:56 ` Parav Pandit
0 siblings, 1 reply; 48+ messages in thread
From: Leon Romanovsky @ 2020-10-02 11:45 UTC (permalink / raw)
To: Parav Pandit; +Cc: Ertman, David M, linux-rdma
On Fri, Oct 02, 2020 at 11:27:43AM +0000, Parav Pandit wrote:
>
>
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Friday, October 2, 2020 4:44 PM
>
> [..]
> > > > ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.eth.2
> > > This gives you the ability to not load the netdevice and rdma device of a VF
> > and only load the vdpa device.
> > > These are real use case that users have asked for.
> > > In use case one, they are only interested in rdma device.
> > > In second use case only vdpa device.
> > > How shall one achieve that without spinning of the device for each class?
> >
> > Why will it be different if ancillary device is small PCI core?
> > If you want RDMA, you will use specific ancillary driver that connects to that
> > small PCI logic.
>
> I didn't follow, wwhat is PCI core and PCI logic in this context?
mlx5_core is PCI core/logic - ancillary device
mlx5_ib/mlx5_en/mlx5_vdpa - ancillary drivers
>
> Not sure if you understood the use case.
> Let me try again.
> Let say there are 4 VFs enabled.
> User would not like to create netdev for 3 VFs (0 to 2) ; user only wants rdma device for these VFs 0 to 2.
> User wants only vdpa device for 4th VF.
> User doesn't want to create rdma device and netdevice for the 4th VF.
> How one shall achieve this?
It depends on how N-to-1 bus will be implemented. For example, devlink
already allows to disable RoCE on specific function, nothing prohibits
to extend it to support other classes.
> It is easily achievable with current ancillary device instantiation per class proposal.
It is byproduct of 1-to-1 connection and not specific design decision.
>
> > Being nice to the users and provide clean abstraction are important goals too.
> Which part of this makes not_nice_to_users and what is not abstracted.
> I lost you.
Your use case perfectly presented not_nice_to_users thing. Users are
interested to work on functions (VF/PF/SF) and configure them without
need to dig into the sysfs directories to connect ancillary classes
and their indexes to the real functions.
Thanks
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH 1/6] Add ancillary bus support
2020-10-02 11:45 ` Leon Romanovsky
@ 2020-10-02 11:56 ` Parav Pandit
0 siblings, 0 replies; 48+ messages in thread
From: Parav Pandit @ 2020-10-02 11:56 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Ertman, David M, linux-rdma
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Friday, October 2, 2020 5:16 PM
>
> On Fri, Oct 02, 2020 at 11:27:43AM +0000, Parav Pandit wrote:
> >
> >
> > > From: Leon Romanovsky <leon@kernel.org>
> > > Sent: Friday, October 2, 2020 4:44 PM
> >
> > [..]
> > > > > ../../../devices/pci0000:00/0000:00:0b.0/mlx5_core.eth.2
> > > > This gives you the ability to not load the netdevice and rdma
> > > > device of a VF
> > > and only load the vdpa device.
> > > > These are real use case that users have asked for.
> > > > In use case one, they are only interested in rdma device.
> > > > In second use case only vdpa device.
> > > > How shall one achieve that without spinning of the device for each
> class?
> > >
> > > Why will it be different if ancillary device is small PCI core?
> > > If you want RDMA, you will use specific ancillary driver that
> > > connects to that small PCI logic.
> >
> > I didn't follow, wwhat is PCI core and PCI logic in this context?
>
> mlx5_core is PCI core/logic - ancillary device mlx5_ib/mlx5_en/mlx5_vdpa -
> ancillary drivers
>
> >
> > Not sure if you understood the use case.
> > Let me try again.
> > Let say there are 4 VFs enabled.
> > User would not like to create netdev for 3 VFs (0 to 2) ; user only wants
> rdma device for these VFs 0 to 2.
> > User wants only vdpa device for 4th VF.
> > User doesn't want to create rdma device and netdevice for the 4th VF.
> > How one shall achieve this?
>
> It depends on how N-to-1 bus will be implemented. For example, devlink
> already allows to disable RoCE on specific function, nothing prohibits to
> extend it to support other classes.
Sure. Please go through the our internal RFC dated 7/7/20 with subject "devlink device params to disable rdma, netdev" which exactly achieves similar.
And recent 3rd internal RFC "devlink to set driver parameters" discussion with Eli, Jason, Jiri and others that describes to have one ancillary_device_per_class instead of devlink.
Lets discuss it offline as we have multiple proposals floating.
>
> > It is easily achievable with current ancillary device instantiation per class
> proposal.
>
> It is byproduct of 1-to-1 connection and not specific design decision.
>
> >
> > > Being nice to the users and provide clean abstraction are important goals
> too.
> > Which part of this makes not_nice_to_users and what is not abstracted.
> > I lost you.
>
> Your use case perfectly presented not_nice_to_users thing. Users are
> interested to work on functions (VF/PF/SF) and configure them without
> need to dig into the sysfs directories to connect ancillary classes and their
> indexes to the real functions.
>
> Thanks
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
2020-10-01 5:05 [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Dave Ertman
` (5 preceding siblings ...)
2020-10-01 5:05 ` [PATCH 6/6] ASoC: SOF: debug: Remove IPC flood test support in SOF core Dave Ertman
@ 2020-10-03 9:04 ` Leon Romanovsky
2020-10-03 9:10 ` Greg KH
6 siblings, 1 reply; 48+ messages in thread
From: Leon Romanovsky @ 2020-10-03 9:04 UTC (permalink / raw)
To: Dave Ertman, gregkh; +Cc: linux-rdma, linux-netdev, alsa-devel
Hi Dave,
I don't know why did you send this series separately to every mailing
list, but it is not correct thing to do.
RDMA ML and discussion:
https://lore.kernel.org/linux-rdma/20201001050534.890666-1-david.m.ertman@intel.com/T/#t
Netdev ML (completely separated):
https://lore.kernel.org/netdev/20201001050851.890722-1-david.m.ertman@intel.com/
Alsa ML (separated too):
https://lore.kernel.org/alsa-devel/20200930225051.889607-1-david.m.ertman@intel.com/
Thanks
On Wed, Sep 30, 2020 at 10:05:28PM -0700, Dave Ertman wrote:
> Brief history of Ancillary Bus
> ==============================
> The ancillary bus code was originally submitted upstream as virtual
> bus, and was submitted through the netdev tree. This process generated
> up to v4. This discussion can be found here:
> https://lore.kernel.org/netdev/0200520070227.3392100-2-jeffrey.t.kirsher@intel.com/T/#u
>
> At this point, GregKH requested that we take the review and revision
> process to an internal mailing list and garner the buy-in of a respected
> kernel contributor.
>
> The ancillary bus (then known as virtual bus) was originally submitted
> along with implementation code for the ice driver and irdma drive,
> causing the complication of also having dependencies in the rdma tree.
> This new submission is utilizing an ancillary bus consumer in only the
> sound driver tree to create the initial implementation and a single
> user.
>
> Since implementation work has started on this patch set, there have been
> multiple inquiries about the time frame of its completion. It appears
> that there will be numerous consumers of this functionality.
>
> The process of internal review and implementation using the sound
> drivers generated 19 internal versions. The changes, including the name
> change from virtual bus to ancillary bus, from these versions can be
> summarized as the following:
>
> - Fixed compilation and checkpatch errors
> - Improved documentation to address the motivation for virtual bus.
> - Renamed virtual bus to ancillary bus
> - increased maximum device name size
> - Correct order in Kconfig and Makefile
> - removed the mid-layer adev->release layer for device unregister
> - pushed adev->id management to parent driver
> - all error paths out of ancillary_device_register return error code
> - all error paths out of ancillary_device_register use put_device
> - added adev->name element
> - modname in register cannot be NULL
> - added KBUILD_MODNAME as prefix for match_name
> - push adev->id responsibility to registering driver
> - uevent now parses adev->dev name
> - match_id function now parses adev->dev name
> - changed drivers probe function to also take an ancillary_device_id param
> - split ancillary_device_register into device_initialize and device_add
> - adjusted what is done in device_initialize and device_add
> - change adev to ancildev and adrv to ancildrv
> - change adev to ancildev in documentation
>
> This submission is the first time that this patch set will be sent to
> the alsa-devel mailing list, so it is currently being submitted as
> version 1.
>
> ==========================
>
> Introduces the ancillary bus implementation along with the example usage
> in the Sound Open Firmware(SOF) audio driver.
>
> In some subsystems, the functionality of the core device
> (PCI/ACPI/other) may be too complex for a single device to be managed as
> a monolithic block or a part of the functionality might need to be
> exposed to a different subsystem. Splitting the functionality into
> smaller orthogonal devices makes it easier to manage data, power
> management and domain-specific communication with the hardware. Also,
> common ancillary_device functionality across primary devices can be
> handled by a common ancillary_device. 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 it 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 handles 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 tightly coupled with handling the
> hardware-specific logic and communication.
>
> The ancillary bus is intended to be minimal, generic and avoid
> domain-specific assumptions. Each ancillary bus device represents a part
> of its parent functionality. The generic behavior can be extended and
> specialized as needed by encapsulating an ancillary bus device within
> other domain-specific structures and the use of .ops callbacks.
>
> The SOF driver adopts the ancillary bus for implementing the
> multi-client support. A client in the context of the SOF driver
> represents a part of the core device's functionality. It is not a
> physical device but rather an ancillary device that needs to communicate
> with the DSP via IPCs. With multi-client support,the sound card can be
> separated into multiple orthogonal ancillary devices for local devices
> (mic/speakers etc), HDMI, sensing, probes, debug etc. In this series,
> we demonstrate the usage of the ancillary bus with the help of the IPC
> test client which is used for testing the serialization of IPCs when
> multiple clients talk to the DSP at the same time.
>
> Dave Ertman (1):
> Add ancillary bus support
>
> Fred Oh (1):
> ASoC: SOF: debug: Remove IPC flood test support in SOF core
>
> Ranjani Sridharan (4):
> ASoC: SOF: Introduce descriptors for SOF client
> ASoC: SOF: Create client driver for IPC test
> ASoC: SOF: ops: Add ops for client registration
> ASoC: SOF: Intel: Define ops for client registration
>
> 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 +
> sound/soc/sof/Kconfig | 29 +-
> sound/soc/sof/Makefile | 7 +
> sound/soc/sof/core.c | 12 +
> sound/soc/sof/debug.c | 230 ---------------
> sound/soc/sof/intel/Kconfig | 9 +
> sound/soc/sof/intel/Makefile | 3 +
> sound/soc/sof/intel/apl.c | 18 ++
> sound/soc/sof/intel/bdw.c | 18 ++
> sound/soc/sof/intel/byt.c | 22 ++
> sound/soc/sof/intel/cnl.c | 18 ++
> sound/soc/sof/intel/intel-client.c | 49 ++++
> sound/soc/sof/intel/intel-client.h | 26 ++
> sound/soc/sof/ops.h | 14 +
> sound/soc/sof/sof-client.c | 117 ++++++++
> sound/soc/sof/sof-client.h | 65 +++++
> sound/soc/sof/sof-ipc-test-client.c | 314 +++++++++++++++++++++
> sound/soc/sof/sof-priv.h | 16 +-
> 26 files changed, 1233 insertions(+), 239 deletions(-)
> create mode 100644 Documentation/driver-api/ancillary_bus.rst
> create mode 100644 drivers/bus/ancillary.c
> create mode 100644 include/linux/ancillary_bus.h
> create mode 100644 sound/soc/sof/intel/intel-client.c
> create mode 100644 sound/soc/sof/intel/intel-client.h
> create mode 100644 sound/soc/sof/sof-client.c
> create mode 100644 sound/soc/sof/sof-client.h
> create mode 100644 sound/soc/sof/sof-ipc-test-client.c
>
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
2020-10-03 9:04 ` [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Leon Romanovsky
@ 2020-10-03 9:10 ` Greg KH
0 siblings, 0 replies; 48+ messages in thread
From: Greg KH @ 2020-10-03 9:10 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Dave Ertman, linux-rdma, linux-netdev, alsa-devel
On Sat, Oct 03, 2020 at 12:04:52PM +0300, Leon Romanovsky wrote:
> Hi Dave,
>
> I don't know why did you send this series separately to every mailing
> list, but it is not correct thing to do.
>
> RDMA ML and discussion:
> https://lore.kernel.org/linux-rdma/20201001050534.890666-1-david.m.ertman@intel.com/T/#t
> Netdev ML (completely separated):
> https://lore.kernel.org/netdev/20201001050851.890722-1-david.m.ertman@intel.com/
> Alsa ML (separated too):
> https://lore.kernel.org/alsa-devel/20200930225051.889607-1-david.m.ertman@intel.com/
Seems like the goal was to spread it around to different places so that
no one could strongly object or review it :(
greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
@ 2020-10-03 9:10 ` Greg KH
0 siblings, 0 replies; 48+ messages in thread
From: Greg KH @ 2020-10-03 9:10 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: linux-rdma, Dave Ertman, alsa-devel, linux-netdev
On Sat, Oct 03, 2020 at 12:04:52PM +0300, Leon Romanovsky wrote:
> Hi Dave,
>
> I don't know why did you send this series separately to every mailing
> list, but it is not correct thing to do.
>
> RDMA ML and discussion:
> https://lore.kernel.org/linux-rdma/20201001050534.890666-1-david.m.ertman@intel.com/T/#t
> Netdev ML (completely separated):
> https://lore.kernel.org/netdev/20201001050851.890722-1-david.m.ertman@intel.com/
> Alsa ML (separated too):
> https://lore.kernel.org/alsa-devel/20200930225051.889607-1-david.m.ertman@intel.com/
Seems like the goal was to spread it around to different places so that
no one could strongly object or review it :(
greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
2020-10-03 9:10 ` Greg KH
@ 2020-10-03 9:24 ` Leon Romanovsky
-1 siblings, 0 replies; 48+ messages in thread
From: Leon Romanovsky @ 2020-10-03 9:24 UTC (permalink / raw)
To: Greg KH; +Cc: Dave Ertman, linux-rdma, linux-netdev, alsa-devel
On Sat, Oct 03, 2020 at 11:10:36AM +0200, Greg KH wrote:
> On Sat, Oct 03, 2020 at 12:04:52PM +0300, Leon Romanovsky wrote:
> > Hi Dave,
> >
> > I don't know why did you send this series separately to every mailing
> > list, but it is not correct thing to do.
> >
> > RDMA ML and discussion:
> > https://lore.kernel.org/linux-rdma/20201001050534.890666-1-david.m.ertman@intel.com/T/#t
> > Netdev ML (completely separated):
> > https://lore.kernel.org/netdev/20201001050851.890722-1-david.m.ertman@intel.com/
> > Alsa ML (separated too):
> > https://lore.kernel.org/alsa-devel/20200930225051.889607-1-david.m.ertman@intel.com/
>
> Seems like the goal was to spread it around to different places so that
> no one could strongly object or review it :(
It took me time to realize why I was alone expressing my thoughts :).
BTW, I'm looking on ALSA thread and happy to see that people didn't like
"ancillary" name. It is far from intuitive name for any non-English speaker.
Thanks
>
> greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
@ 2020-10-03 9:24 ` Leon Romanovsky
0 siblings, 0 replies; 48+ messages in thread
From: Leon Romanovsky @ 2020-10-03 9:24 UTC (permalink / raw)
To: Greg KH; +Cc: linux-rdma, Dave Ertman, alsa-devel, linux-netdev
On Sat, Oct 03, 2020 at 11:10:36AM +0200, Greg KH wrote:
> On Sat, Oct 03, 2020 at 12:04:52PM +0300, Leon Romanovsky wrote:
> > Hi Dave,
> >
> > I don't know why did you send this series separately to every mailing
> > list, but it is not correct thing to do.
> >
> > RDMA ML and discussion:
> > https://lore.kernel.org/linux-rdma/20201001050534.890666-1-david.m.ertman@intel.com/T/#t
> > Netdev ML (completely separated):
> > https://lore.kernel.org/netdev/20201001050851.890722-1-david.m.ertman@intel.com/
> > Alsa ML (separated too):
> > https://lore.kernel.org/alsa-devel/20200930225051.889607-1-david.m.ertman@intel.com/
>
> Seems like the goal was to spread it around to different places so that
> no one could strongly object or review it :(
It took me time to realize why I was alone expressing my thoughts :).
BTW, I'm looking on ALSA thread and happy to see that people didn't like
"ancillary" name. It is far from intuitive name for any non-English speaker.
Thanks
>
> greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
2020-10-03 9:24 ` Leon Romanovsky
@ 2020-10-03 9:32 ` Greg KH
-1 siblings, 0 replies; 48+ messages in thread
From: Greg KH @ 2020-10-03 9:32 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Dave Ertman, linux-rdma, linux-netdev, alsa-devel
On Sat, Oct 03, 2020 at 12:24:07PM +0300, Leon Romanovsky wrote:
> On Sat, Oct 03, 2020 at 11:10:36AM +0200, Greg KH wrote:
> > On Sat, Oct 03, 2020 at 12:04:52PM +0300, Leon Romanovsky wrote:
> > > Hi Dave,
> > >
> > > I don't know why did you send this series separately to every mailing
> > > list, but it is not correct thing to do.
> > >
> > > RDMA ML and discussion:
> > > https://lore.kernel.org/linux-rdma/20201001050534.890666-1-david.m.ertman@intel.com/T/#t
> > > Netdev ML (completely separated):
> > > https://lore.kernel.org/netdev/20201001050851.890722-1-david.m.ertman@intel.com/
> > > Alsa ML (separated too):
> > > https://lore.kernel.org/alsa-devel/20200930225051.889607-1-david.m.ertman@intel.com/
> >
> > Seems like the goal was to spread it around to different places so that
> > no one could strongly object or review it :(
>
> It took me time to realize why I was alone expressing my thoughts :).
>
> BTW, I'm looking on ALSA thread and happy to see that people didn't like
> "ancillary" name. It is far from intuitive name for any non-English speaker.
It's not intuitive for a native-english speaker either :)
greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
@ 2020-10-03 9:32 ` Greg KH
0 siblings, 0 replies; 48+ messages in thread
From: Greg KH @ 2020-10-03 9:32 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: linux-rdma, Dave Ertman, alsa-devel, linux-netdev
On Sat, Oct 03, 2020 at 12:24:07PM +0300, Leon Romanovsky wrote:
> On Sat, Oct 03, 2020 at 11:10:36AM +0200, Greg KH wrote:
> > On Sat, Oct 03, 2020 at 12:04:52PM +0300, Leon Romanovsky wrote:
> > > Hi Dave,
> > >
> > > I don't know why did you send this series separately to every mailing
> > > list, but it is not correct thing to do.
> > >
> > > RDMA ML and discussion:
> > > https://lore.kernel.org/linux-rdma/20201001050534.890666-1-david.m.ertman@intel.com/T/#t
> > > Netdev ML (completely separated):
> > > https://lore.kernel.org/netdev/20201001050851.890722-1-david.m.ertman@intel.com/
> > > Alsa ML (separated too):
> > > https://lore.kernel.org/alsa-devel/20200930225051.889607-1-david.m.ertman@intel.com/
> >
> > Seems like the goal was to spread it around to different places so that
> > no one could strongly object or review it :(
>
> It took me time to realize why I was alone expressing my thoughts :).
>
> BTW, I'm looking on ALSA thread and happy to see that people didn't like
> "ancillary" name. It is far from intuitive name for any non-English speaker.
It's not intuitive for a native-english speaker either :)
greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
2020-10-03 9:10 ` Greg KH
(?)
(?)
@ 2020-10-05 1:20 ` Ertman, David M
-1 siblings, 0 replies; 48+ messages in thread
From: Ertman, David M @ 2020-10-05 1:20 UTC (permalink / raw)
To: Greg KH, Leon Romanovsky; +Cc: linux-rdma, linux-netdev, alsa-devel
> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Saturday, October 3, 2020 2:11 AM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: Ertman, David M <david.m.ertman@intel.com>; linux-
> rdma@vger.kernel.org; linux-netdev <netdev@vger.kernel.org>; alsa-
> devel@alsa-project.org
> Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client
> support
>
> On Sat, Oct 03, 2020 at 12:04:52PM +0300, Leon Romanovsky wrote:
> > Hi Dave,
> >
> > I don't know why did you send this series separately to every mailing
> > list, but it is not correct thing to do.
> >
> > RDMA ML and discussion:
> > https://lore.kernel.org/linux-rdma/20201001050534.890666-1-
> david.m.ertman@intel.com/T/#t
> > Netdev ML (completely separated):
> > https://lore.kernel.org/netdev/20201001050851.890722-1-
> david.m.ertman@intel.com/
> > Alsa ML (separated too):
> > https://lore.kernel.org/alsa-devel/20200930225051.889607-1-
> david.m.ertman@intel.com/
>
> Seems like the goal was to spread it around to different places so that
> no one could strongly object or review it :(
>
> greg k-h
This was my first time sending a patchset to more than one mailing list
and I screwed it up.
I will be sending a v2 soon (Monday maybe?) and I will be sending it to
all CC's and mailing list in one send so that everyone will see everyone's
response.
Sorry for the mistake.
-DaveE.
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH 1/6] Add ancillary bus support
2020-10-01 12:15 ` Greg KH
@ 2020-10-01 18:26 ` Ertman, David M
0 siblings, 0 replies; 48+ messages in thread
From: Ertman, David M @ 2020-10-01 18:26 UTC (permalink / raw)
To: Greg KH, Jason Gunthorpe
Cc: alsa-devel, Patil, Kiran, tiwai, Sridharan, Ranjani,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Williams, Dan J, Saleem, Shiraz, Parav Pandit
> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Thursday, October 1, 2020 5:16 AM
> To: Jason Gunthorpe <jgg@nvidia.com>
> Cc: Ertman, David M <david.m.ertman@intel.com>; alsa-devel@alsa-
> project.org; tiwai@suse.de; broonie@kernel.org; pierre-
> louis.bossart@linux.intel.com; Sridharan, Ranjani
> <ranjani.sridharan@intel.com>; parav@nvidia.com; Patil, Kiran
> <kiran.patil@intel.com>; Ranjani Sridharan
> <ranjani.sridharan@linux.intel.com>; Fred Oh <fred.oh@linux.intel.com>;
> Saleem, Shiraz <shiraz.saleem@intel.com>; Parav Pandit
> <parav@mellanox.com>; Williams, Dan J <dan.j.williams@intel.com>
> Subject: Re: [PATCH 1/6] Add ancillary bus support
>
> On Thu, Oct 01, 2020 at 09:02:12AM -0300, Jason Gunthorpe wrote:
> > On Thu, Oct 01, 2020 at 01:54:02PM +0200, Greg KH wrote:
> > > On Thu, Oct 01, 2020 at 08:46:08AM -0300, Jason Gunthorpe wrote:
> > > > On Thu, Oct 01, 2020 at 01:01:20PM +0200, Greg KH wrote:
> > > > > On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> > > > > > +int ancillary_device_initialize(struct ancillary_device *ancildev)
> > > > > > +{
> > > > > > + struct device *dev = &ancildev->dev;
> > > > > > +
> > > > > > + dev->bus = &ancillary_bus_type;
> > > > > > +
> > > > > > + if (WARN_ON(!dev->parent) || WARN_ON(!ancildev-
> >name) ||
> > > > > > + WARN_ON(!(dev->type && dev->type->release) && !dev-
> >release))
> > > > > > + return -EINVAL;
> > > > >
> > > > > You have a lot of WARN_ON() calls in this patch. That blows up
> anyone
> > > > > who runs with panic-on-warn, right?
> > > >
> > > > AFAIK this is the standard pattern to code a "can't happen"
> > > > assertion. Linus has been clear not to use BUG_ON, but to try and
> > > > recover. The WARN_ON directly points to the faulty driver so it can be
> > > > fixed.
> > >
> > > Printing an error and returning an error value also does the same exact
> > > thing, the developer will not have a working system.
> > >
> > > Please don't abuse WARN_ON() for things that should just be normal
> error
> > > checking logic of api calls.
> >
> > This is not normal error checking, it is precondition
> > assertion. Something has gone badly wrong if it ever triggers.
> >
> > If you don't want to use WARN_ON for assertions then when should it be
> > used?
> >
> > pr_err is not the same thing, it doesn't trigger reports from fuzzers.
>
> fuzzers shouldn't be messing with device registration functions :)
>
> just do a normal pr_err() and all is fine, again, this is like any other
> in-kernel api that is trying to check for valid values being passed to
> it.
I will remove these for next version.
-DaveE
>
> thanks,
>
> grteg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 14:38 ` Greg KH
2020-10-01 16:06 ` Pierre-Louis Bossart
@ 2020-10-01 17:42 ` Jason Gunthorpe
1 sibling, 0 replies; 48+ messages in thread
From: Jason Gunthorpe @ 2020-10-01 17:42 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Dave Ertman, Dan Williams, Shiraz Saleem, Parav Pandit
On Thu, Oct 01, 2020 at 04:38:55PM +0200, Greg KH wrote:
> On Thu, Oct 01, 2020 at 11:33:34AM -0300, Jason Gunthorpe wrote:
> > On Thu, Oct 01, 2020 at 02:14:23PM +0200, Greg KH wrote:
> > > On Thu, Oct 01, 2020 at 08:58:47AM -0300, Jason Gunthorpe wrote:
> > > > On Thu, Oct 01, 2020 at 01:05:51PM +0200, Greg KH wrote:
> > > >
> > > > > You have to be _VERY_ careful after calling
> > > > > ancillary_device_initialize(), as now you can not just free up the
> > > > > memory if something goes wrong before ancillary_device_add() is called,
> > > > > right?
> > > >
> > > > I've looked at way too many versions of this patch and related. This
> > > > is the only one so far that I didn't find various bugs on the error
> > > > cases.
> > >
> > > But you haven't seen the callers of this function. Without this
> > > documented, you will have problems.
> >
> > I've seen the Intel irdma, both versions of the SOF stuff and an
> > internal mlx5 patch..
> >
> > Look at the SOF example, it has perfectly paired error unwinds. Each
> > function has unwind that cleans up exactly what it creates. Every
> > 'free' unwind is paired with an 'alloc' in the same function. Simple.
> > Easy to audit. Easy to correctly enhance down the road.
> >
> > This is the common kernel goto error design pattern.
>
> But that's where people get this wrong.
People get everything wrong :( At least this pattern is easy to notice
and review.
> Once device_initialize() is called, the "free" can not be called,
> something else must be, device_put().
Yep!
However, with the one step device_register() pattern code usually
makes this class of mistake:
https://elixir.bootlin.com/linux/latest/source/drivers/firewire/core-device.c#L722
'goto skip_unit' does kfree() on something that already has been
device_initialized(). This is a real bug because this code called
dev_set_name() on line 713 and not doing the put_device() leaked the
name allocation. I think < v10 had this mistake.
dev_set_name() is a common error, here is another version:
https://elixir.bootlin.com/linux/latest/source/drivers/dma/idxd/cdev.c#L226
This correctly gets the switch to put_device() after
device_register(), but it calls kfree on line 220 after
dev_set_name(). This leaks memory too. Something like v16 of this
series had this bug as well.
BTW, want a patch to add a kref_read(dev->kref) == 0 warning to
dev_set_name() ? This seems pretty common, these were the first two
random choices from LXR I checked :\
> Sure, but without a real user that _NEEDS_ this two-step process, let's
> not include it. Why bake complexity into the system from the start that
> is never used?
It just needs to not have these common error unwind bugs :(
Jason
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH 1/6] Add ancillary bus support
2020-10-01 11:02 ` Greg KH
@ 2020-10-01 16:30 ` Ertman, David M
0 siblings, 0 replies; 48+ messages in thread
From: Ertman, David M @ 2020-10-01 16:30 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, Patil, Kiran, tiwai, Sridharan, Ranjani,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
jgg, Williams, Dan J, Saleem, Shiraz, Parav Pandit
> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Thursday, October 1, 2020 4:02 AM
> To: Ertman, David M <david.m.ertman@intel.com>
> Cc: alsa-devel@alsa-project.org; tiwai@suse.de; broonie@kernel.org; pierre-
> louis.bossart@linux.intel.com; Sridharan, Ranjani
> <ranjani.sridharan@intel.com>; jgg@nvidia.com; parav@nvidia.com; Patil,
> Kiran <kiran.patil@intel.com>; Ranjani Sridharan
> <ranjani.sridharan@linux.intel.com>; Fred Oh <fred.oh@linux.intel.com>;
> Saleem, Shiraz <shiraz.saleem@intel.com>; Parav Pandit
> <parav@mellanox.com>; Williams, Dan J <dan.j.williams@intel.com>
> Subject: Re: [PATCH 1/6] Add ancillary bus support
>
> On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> > index 397e35392bff..1fd238094543 100644
> > --- a/drivers/bus/Makefile
> > +++ b/drivers/bus/Makefile
> > @@ -3,6 +3,9 @@
> > # Makefile for the bus drivers.
> > #
> >
> > +#Ancillary bus driver
>
> Nit, you need a space :)
Fixed for next version 😊
>
> > +obj-$(CONFIG_ANCILLARY_BUS) += ancillary.o
>
> And why not put this in drivers/base/? Why way over here in
> drivers/bus?
My only reason was that this is a bus, and I wagered that buses should
go into the bus subdirectory. 😊
>
> thanks,
>
> greg k-h
-DaveE
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 14:38 ` Greg KH
@ 2020-10-01 16:06 ` Pierre-Louis Bossart
2020-10-01 17:42 ` Jason Gunthorpe
1 sibling, 0 replies; 48+ messages in thread
From: Pierre-Louis Bossart @ 2020-10-01 16:06 UTC (permalink / raw)
To: Greg KH, Jason Gunthorpe
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
Ranjani Sridharan, Fred Oh, broonie, parav, Dave Ertman,
Dan Williams, Shiraz Saleem, Parav Pandit
On 10/1/20 9:38 AM, Greg KH wrote:
> On Thu, Oct 01, 2020 at 11:33:34AM -0300, Jason Gunthorpe wrote:
>> On Thu, Oct 01, 2020 at 02:14:23PM +0200, Greg KH wrote:
>>> On Thu, Oct 01, 2020 at 08:58:47AM -0300, Jason Gunthorpe wrote:
>>>> On Thu, Oct 01, 2020 at 01:05:51PM +0200, Greg KH wrote:
>>>>
>>>>> You have to be _VERY_ careful after calling
>>>>> ancillary_device_initialize(), as now you can not just free up the
>>>>> memory if something goes wrong before ancillary_device_add() is called,
>>>>> right?
>>>>
>>>> I've looked at way too many versions of this patch and related. This
>>>> is the only one so far that I didn't find various bugs on the error
>>>> cases.
>>>
>>> But you haven't seen the callers of this function. Without this
>>> documented, you will have problems.
>>
>> I've seen the Intel irdma, both versions of the SOF stuff and an
>> internal mlx5 patch..
>>
>> Look at the SOF example, it has perfectly paired error unwinds. Each
>> function has unwind that cleans up exactly what it creates. Every
>> 'free' unwind is paired with an 'alloc' in the same function. Simple.
>> Easy to audit. Easy to correctly enhance down the road.
>>
>> This is the common kernel goto error design pattern.
>
> But that's where people get this wrong. Once device_initialize() is
> called, the "free" can not be called, something else must be,
> device_put().
>
> Tricky, yes. Messy, yes. Sorry.
>
>>> Why is this two-step process even needed here?
>>
>> Initializing the refcount close to the allocation is a common design
>> pattern as is initializing it close to registration. Both options are
>> tricky, both have various common subtle bugs, both have awkward
>> elements.
>>
>> At the end of the day, after something like 20 iterations, this is the
>> first series that actually doesn't have error unwind bugs.
>>
>> Can we empower Dave to make this choice? It is not like it is wild or
>> weird, the driver core already exposes _initialize and _add functions
>> that work in exactly the same way.
>
> Sure, but without a real user that _NEEDS_ this two-step process, let's
> not include it. Why bake complexity into the system from the start that
> is never used?
>
> Iteration and evolution is fine, it's not like this is going to be
> set-in-stone for forever.
We initially had a single ancillary_device_register(). At some point,
there was an ask to simplify the error handling by moving some of it to
the caller, and an ask to let the IDAs be managed at the parent level to
avoid possible discontinuities in the numbers allocated.
Both changes made it hard to deal with errors flow on the caller side.
As you describe it above, we had to either free memory if the error
happened before device_initialize() was called (e.g. missing .release
callback, etc), or use put_device() afterwards.
Splitting the two appeared to be the only way to make sure the resources
are released in the right way, with a single function we had several
cases where the caller couldn't figure out whether to free memory or
call put_device().
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 14:39 ` Parav Pandit
@ 2020-10-01 14:43 ` Greg KH
0 siblings, 0 replies; 48+ messages in thread
From: Greg KH @ 2020-10-01 14:43 UTC (permalink / raw)
To: Parav Pandit
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie,
Jason Gunthorpe, Dave Ertman, Dan Williams, Shiraz Saleem,
Parav Pandit
On Thu, Oct 01, 2020 at 02:39:06PM +0000, Parav Pandit wrote:
> > > Why is this two-step process even needed here?
> > > Without this documented, you will have problems.
>
> And it is also documented in "Ancillary device" section in the Documentation/driver-api/ancillary_bus.rst
> Below is the snippet from the patch.
>
> It is likely worth to add this part of the documentation using standard kernel kdoc format where export function definition of initialize() and add() resides, so that it cannot be missed out.
That is what I asked for...
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH 1/6] Add ancillary bus support
2020-10-01 14:33 ` Jason Gunthorpe
2020-10-01 14:38 ` Greg KH
@ 2020-10-01 14:39 ` Parav Pandit
2020-10-01 14:43 ` Greg KH
1 sibling, 1 reply; 48+ messages in thread
From: Parav Pandit @ 2020-10-01 14:39 UTC (permalink / raw)
To: Jason Gunthorpe, Greg KH
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie,
Dave Ertman, Dan Williams, Shiraz Saleem, Parav Pandit
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, October 1, 2020 8:04 PM
>
> On Thu, Oct 01, 2020 at 02:14:23PM +0200, Greg KH wrote:
> > On Thu, Oct 01, 2020 at 08:58:47AM -0300, Jason Gunthorpe wrote:
> > > On Thu, Oct 01, 2020 at 01:05:51PM +0200, Greg KH wrote:
> > >
> > > > You have to be _VERY_ careful after calling
> > > > ancillary_device_initialize(), as now you can not just free up the
> > > > memory if something goes wrong before ancillary_device_add() is
> > > > called, right?
> > >
> > > I've looked at way too many versions of this patch and related. This
> > > is the only one so far that I didn't find various bugs on the error
> > > cases.
> >
> > But you haven't seen the callers of this function. Without this
> > documented, you will have problems.
>
> I've seen the Intel irdma, both versions of the SOF stuff and an internal mlx5
> patch..
>
> Look at the SOF example, it has perfectly paired error unwinds. Each function
> has unwind that cleans up exactly what it creates. Every 'free' unwind is
> paired with an 'alloc' in the same function. Simple.
> Easy to audit. Easy to correctly enhance down the road.
>
> This is the common kernel goto error design pattern.
>
> > Why is this two-step process even needed here?
>
> Initializing the refcount close to the allocation is a common design pattern as
> is initializing it close to registration. Both options are tricky, both have various
> common subtle bugs, both have awkward elements.
>
> At the end of the day, after something like 20 iterations, this is the first series
> that actually doesn't have error unwind bugs.
>
> Can we empower Dave to make this choice? It is not like it is wild or weird,
> the driver core already exposes _initialize and _add functions that work in
> exactly the same way.
>
> > Why is this two-step process even needed here?
> > Without this documented, you will have problems.
And it is also documented in "Ancillary device" section in the Documentation/driver-api/ancillary_bus.rst
Below is the snippet from the patch.
It is likely worth to add this part of the documentation using standard kernel kdoc format where export function definition of initialize() and add() resides, so that it cannot be missed out.
+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.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 14:33 ` Jason Gunthorpe
@ 2020-10-01 14:38 ` Greg KH
2020-10-01 16:06 ` Pierre-Louis Bossart
2020-10-01 17:42 ` Jason Gunthorpe
2020-10-01 14:39 ` Parav Pandit
1 sibling, 2 replies; 48+ messages in thread
From: Greg KH @ 2020-10-01 14:38 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Dave Ertman, Dan Williams, Shiraz Saleem, Parav Pandit
On Thu, Oct 01, 2020 at 11:33:34AM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 01, 2020 at 02:14:23PM +0200, Greg KH wrote:
> > On Thu, Oct 01, 2020 at 08:58:47AM -0300, Jason Gunthorpe wrote:
> > > On Thu, Oct 01, 2020 at 01:05:51PM +0200, Greg KH wrote:
> > >
> > > > You have to be _VERY_ careful after calling
> > > > ancillary_device_initialize(), as now you can not just free up the
> > > > memory if something goes wrong before ancillary_device_add() is called,
> > > > right?
> > >
> > > I've looked at way too many versions of this patch and related. This
> > > is the only one so far that I didn't find various bugs on the error
> > > cases.
> >
> > But you haven't seen the callers of this function. Without this
> > documented, you will have problems.
>
> I've seen the Intel irdma, both versions of the SOF stuff and an
> internal mlx5 patch..
>
> Look at the SOF example, it has perfectly paired error unwinds. Each
> function has unwind that cleans up exactly what it creates. Every
> 'free' unwind is paired with an 'alloc' in the same function. Simple.
> Easy to audit. Easy to correctly enhance down the road.
>
> This is the common kernel goto error design pattern.
But that's where people get this wrong. Once device_initialize() is
called, the "free" can not be called, something else must be,
device_put().
Tricky, yes. Messy, yes. Sorry.
> > Why is this two-step process even needed here?
>
> Initializing the refcount close to the allocation is a common design
> pattern as is initializing it close to registration. Both options are
> tricky, both have various common subtle bugs, both have awkward
> elements.
>
> At the end of the day, after something like 20 iterations, this is the
> first series that actually doesn't have error unwind bugs.
>
> Can we empower Dave to make this choice? It is not like it is wild or
> weird, the driver core already exposes _initialize and _add functions
> that work in exactly the same way.
Sure, but without a real user that _NEEDS_ this two-step process, let's
not include it. Why bake complexity into the system from the start that
is never used?
Iteration and evolution is fine, it's not like this is going to be
set-in-stone for forever.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 12:14 ` Greg KH
@ 2020-10-01 14:33 ` Jason Gunthorpe
2020-10-01 14:38 ` Greg KH
2020-10-01 14:39 ` Parav Pandit
0 siblings, 2 replies; 48+ messages in thread
From: Jason Gunthorpe @ 2020-10-01 14:33 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Dave Ertman, Dan Williams, Shiraz Saleem, Parav Pandit
On Thu, Oct 01, 2020 at 02:14:23PM +0200, Greg KH wrote:
> On Thu, Oct 01, 2020 at 08:58:47AM -0300, Jason Gunthorpe wrote:
> > On Thu, Oct 01, 2020 at 01:05:51PM +0200, Greg KH wrote:
> >
> > > You have to be _VERY_ careful after calling
> > > ancillary_device_initialize(), as now you can not just free up the
> > > memory if something goes wrong before ancillary_device_add() is called,
> > > right?
> >
> > I've looked at way too many versions of this patch and related. This
> > is the only one so far that I didn't find various bugs on the error
> > cases.
>
> But you haven't seen the callers of this function. Without this
> documented, you will have problems.
I've seen the Intel irdma, both versions of the SOF stuff and an
internal mlx5 patch..
Look at the SOF example, it has perfectly paired error unwinds. Each
function has unwind that cleans up exactly what it creates. Every
'free' unwind is paired with an 'alloc' in the same function. Simple.
Easy to audit. Easy to correctly enhance down the road.
This is the common kernel goto error design pattern.
> Why is this two-step process even needed here?
Initializing the refcount close to the allocation is a common design
pattern as is initializing it close to registration. Both options are
tricky, both have various common subtle bugs, both have awkward
elements.
At the end of the day, after something like 20 iterations, this is the
first series that actually doesn't have error unwind bugs.
Can we empower Dave to make this choice? It is not like it is wild or
weird, the driver core already exposes _initialize and _add functions
that work in exactly the same way.
Jason
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-09-30 22:50 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
` (3 preceding siblings ...)
2020-10-01 11:05 ` Greg KH
@ 2020-10-01 13:27 ` Mark Brown
4 siblings, 0 replies; 48+ messages in thread
From: Mark Brown @ 2020-10-01 13:27 UTC (permalink / raw)
To: Dave Ertman
Cc: alsa-devel, Kiran Patil, tiwai, gregkh, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, parav, jgg,
Dan Williams, Shiraz Saleem, Parav Pandit
[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]
On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> Documentation/driver-api/ancillary_bus.rst | 230 +++++++++++++++++++++
> Documentation/driver-api/index.rst | 1 +
It would probably be useful to have the documentation in a separate
patch, it's a huge proportion of the patch and would make it much more
approachable.
> +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.
See my commments on the cover letter about MFD, this is just not true.
> +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
This is not a requirement of the audio subsystem, this is to do with how
the Intel audio hardware has been implemented on their modern SoCs.
> +int ancillary_device_initialize(struct ancillary_device *ancildev)
> +{
> +int __ancillary_device_add(struct ancillary_device *ancildev, const char *modname)
> +{
It can be useful to have this split but there's also going to be plenty
of cases where people just need to register a device based on the struct
ancilliary_device straight away so it would be good to at least have a
standard ancilliary_device_new() (or whatever) that does both steps in
one. As Greg said in his review this split model is a bit more fiddly
to use and frequently leads to error handling problems in drivers.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 12:02 ` Jason Gunthorpe
@ 2020-10-01 12:15 ` Greg KH
2020-10-01 18:26 ` Ertman, David M
0 siblings, 1 reply; 48+ messages in thread
From: Greg KH @ 2020-10-01 12:15 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Dave Ertman, Dan Williams, Shiraz Saleem, Parav Pandit
On Thu, Oct 01, 2020 at 09:02:12AM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 01, 2020 at 01:54:02PM +0200, Greg KH wrote:
> > On Thu, Oct 01, 2020 at 08:46:08AM -0300, Jason Gunthorpe wrote:
> > > On Thu, Oct 01, 2020 at 01:01:20PM +0200, Greg KH wrote:
> > > > On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> > > > > +int ancillary_device_initialize(struct ancillary_device *ancildev)
> > > > > +{
> > > > > + struct device *dev = &ancildev->dev;
> > > > > +
> > > > > + dev->bus = &ancillary_bus_type;
> > > > > +
> > > > > + if (WARN_ON(!dev->parent) || WARN_ON(!ancildev->name) ||
> > > > > + WARN_ON(!(dev->type && dev->type->release) && !dev->release))
> > > > > + return -EINVAL;
> > > >
> > > > You have a lot of WARN_ON() calls in this patch. That blows up anyone
> > > > who runs with panic-on-warn, right?
> > >
> > > AFAIK this is the standard pattern to code a "can't happen"
> > > assertion. Linus has been clear not to use BUG_ON, but to try and
> > > recover. The WARN_ON directly points to the faulty driver so it can be
> > > fixed.
> >
> > Printing an error and returning an error value also does the same exact
> > thing, the developer will not have a working system.
> >
> > Please don't abuse WARN_ON() for things that should just be normal error
> > checking logic of api calls.
>
> This is not normal error checking, it is precondition
> assertion. Something has gone badly wrong if it ever triggers.
>
> If you don't want to use WARN_ON for assertions then when should it be
> used?
>
> pr_err is not the same thing, it doesn't trigger reports from fuzzers.
fuzzers shouldn't be messing with device registration functions :)
just do a normal pr_err() and all is fine, again, this is like any other
in-kernel api that is trying to check for valid values being passed to
it.
thanks,
grteg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 11:58 ` Jason Gunthorpe
@ 2020-10-01 12:14 ` Greg KH
2020-10-01 14:33 ` Jason Gunthorpe
0 siblings, 1 reply; 48+ messages in thread
From: Greg KH @ 2020-10-01 12:14 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Dave Ertman, Dan Williams, Shiraz Saleem, Parav Pandit
On Thu, Oct 01, 2020 at 08:58:47AM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 01, 2020 at 01:05:51PM +0200, Greg KH wrote:
>
> > You have to be _VERY_ careful after calling
> > ancillary_device_initialize(), as now you can not just free up the
> > memory if something goes wrong before ancillary_device_add() is called,
> > right?
>
> I've looked at way too many versions of this patch and related. This
> is the only one so far that I didn't find various bugs on the error
> cases.
But you haven't seen the callers of this function. Without this
documented, you will have problems.
Why is this two-step process even needed here?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 11:54 ` Greg KH
@ 2020-10-01 12:02 ` Jason Gunthorpe
2020-10-01 12:15 ` Greg KH
0 siblings, 1 reply; 48+ messages in thread
From: Jason Gunthorpe @ 2020-10-01 12:02 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Dave Ertman, Dan Williams, Shiraz Saleem, Parav Pandit
On Thu, Oct 01, 2020 at 01:54:02PM +0200, Greg KH wrote:
> On Thu, Oct 01, 2020 at 08:46:08AM -0300, Jason Gunthorpe wrote:
> > On Thu, Oct 01, 2020 at 01:01:20PM +0200, Greg KH wrote:
> > > On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> > > > +int ancillary_device_initialize(struct ancillary_device *ancildev)
> > > > +{
> > > > + struct device *dev = &ancildev->dev;
> > > > +
> > > > + dev->bus = &ancillary_bus_type;
> > > > +
> > > > + if (WARN_ON(!dev->parent) || WARN_ON(!ancildev->name) ||
> > > > + WARN_ON(!(dev->type && dev->type->release) && !dev->release))
> > > > + return -EINVAL;
> > >
> > > You have a lot of WARN_ON() calls in this patch. That blows up anyone
> > > who runs with panic-on-warn, right?
> >
> > AFAIK this is the standard pattern to code a "can't happen"
> > assertion. Linus has been clear not to use BUG_ON, but to try and
> > recover. The WARN_ON directly points to the faulty driver so it can be
> > fixed.
>
> Printing an error and returning an error value also does the same exact
> thing, the developer will not have a working system.
>
> Please don't abuse WARN_ON() for things that should just be normal error
> checking logic of api calls.
This is not normal error checking, it is precondition
assertion. Something has gone badly wrong if it ever triggers.
If you don't want to use WARN_ON for assertions then when should it be
used?
pr_err is not the same thing, it doesn't trigger reports from fuzzers.
Jason
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 11:05 ` Greg KH
@ 2020-10-01 11:58 ` Jason Gunthorpe
2020-10-01 12:14 ` Greg KH
0 siblings, 1 reply; 48+ messages in thread
From: Jason Gunthorpe @ 2020-10-01 11:58 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Dave Ertman, Dan Williams, Shiraz Saleem, Parav Pandit
On Thu, Oct 01, 2020 at 01:05:51PM +0200, Greg KH wrote:
> You have to be _VERY_ careful after calling
> ancillary_device_initialize(), as now you can not just free up the
> memory if something goes wrong before ancillary_device_add() is called,
> right?
I've looked at way too many versions of this patch and related. This
is the only one so far that I didn't find various bugs on the error
cases.
Jason
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 11:46 ` Jason Gunthorpe
@ 2020-10-01 11:54 ` Greg KH
2020-10-01 12:02 ` Jason Gunthorpe
0 siblings, 1 reply; 48+ messages in thread
From: Greg KH @ 2020-10-01 11:54 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Dave Ertman, Dan Williams, Shiraz Saleem, Parav Pandit
On Thu, Oct 01, 2020 at 08:46:08AM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 01, 2020 at 01:01:20PM +0200, Greg KH wrote:
> > On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> > > +int ancillary_device_initialize(struct ancillary_device *ancildev)
> > > +{
> > > + struct device *dev = &ancildev->dev;
> > > +
> > > + dev->bus = &ancillary_bus_type;
> > > +
> > > + if (WARN_ON(!dev->parent) || WARN_ON(!ancildev->name) ||
> > > + WARN_ON(!(dev->type && dev->type->release) && !dev->release))
> > > + return -EINVAL;
> >
> > You have a lot of WARN_ON() calls in this patch. That blows up anyone
> > who runs with panic-on-warn, right?
>
> AFAIK this is the standard pattern to code a "can't happen"
> assertion. Linus has been clear not to use BUG_ON, but to try and
> recover. The WARN_ON directly points to the faulty driver so it can be
> fixed.
Printing an error and returning an error value also does the same exact
thing, the developer will not have a working system.
Please don't abuse WARN_ON() for things that should just be normal error
checking logic of api calls.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-10-01 11:01 ` Greg KH
@ 2020-10-01 11:46 ` Jason Gunthorpe
2020-10-01 11:54 ` Greg KH
0 siblings, 1 reply; 48+ messages in thread
From: Jason Gunthorpe @ 2020-10-01 11:46 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Dave Ertman, Dan Williams, Shiraz Saleem, Parav Pandit
On Thu, Oct 01, 2020 at 01:01:20PM +0200, Greg KH wrote:
> On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> > +int ancillary_device_initialize(struct ancillary_device *ancildev)
> > +{
> > + struct device *dev = &ancildev->dev;
> > +
> > + dev->bus = &ancillary_bus_type;
> > +
> > + if (WARN_ON(!dev->parent) || WARN_ON(!ancildev->name) ||
> > + WARN_ON(!(dev->type && dev->type->release) && !dev->release))
> > + return -EINVAL;
>
> You have a lot of WARN_ON() calls in this patch. That blows up anyone
> who runs with panic-on-warn, right?
AFAIK this is the standard pattern to code a "can't happen"
assertion. Linus has been clear not to use BUG_ON, but to try and
recover. The WARN_ON directly points to the faulty driver so it can be
fixed.
panic-on-warn is a good thing because it causes fuzzers to report a
"can't happen" condition as a failure.
In a real production system if any of these trigger it means the
kernel has detected an internal integrity problem (corrupted memory,
code, ROP attempt, etc). People using panic-on-warn absolutely want
their kernel to stop of it is not functioning properly to protect
data-integrity.
Jason
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-09-30 22:50 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
` (2 preceding siblings ...)
2020-10-01 11:02 ` Greg KH
@ 2020-10-01 11:05 ` Greg KH
2020-10-01 11:58 ` Jason Gunthorpe
2020-10-01 13:27 ` Mark Brown
4 siblings, 1 reply; 48+ messages in thread
From: Greg KH @ 2020-10-01 11:05 UTC (permalink / raw)
To: Dave Ertman
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
jgg, Dan Williams, Shiraz Saleem, Parav Pandit
On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> +The parent device would then register the ancillary_device by calling
> +ancillary_device_initialize(), and then ancillary_device_add(), with the pointer
> +to the ancildev member of the above structure. The parent would provide a name
> +for the ancillary_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.
Why do you split this into two different calls?
You have to be _VERY_ careful after calling
ancillary_device_initialize(), as now you can not just free up the
memory if something goes wrong before ancillary_device_add() is called,
right?
You need to document the heck out of that, otherwise people will get it
wrong, and memory will leak.
greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-09-30 22:50 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
2020-09-30 23:05 ` Jason Gunthorpe
2020-10-01 11:01 ` Greg KH
@ 2020-10-01 11:02 ` Greg KH
2020-10-01 16:30 ` Ertman, David M
2020-10-01 11:05 ` Greg KH
2020-10-01 13:27 ` Mark Brown
4 siblings, 1 reply; 48+ messages in thread
From: Greg KH @ 2020-10-01 11:02 UTC (permalink / raw)
To: Dave Ertman
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
jgg, Dan Williams, Shiraz Saleem, Parav Pandit
On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index 397e35392bff..1fd238094543 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -3,6 +3,9 @@
> # Makefile for the bus drivers.
> #
>
> +#Ancillary bus driver
Nit, you need a space :)
> +obj-$(CONFIG_ANCILLARY_BUS) += ancillary.o
And why not put this in drivers/base/? Why way over here in
drivers/bus?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-09-30 22:50 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
2020-09-30 23:05 ` Jason Gunthorpe
@ 2020-10-01 11:01 ` Greg KH
2020-10-01 11:46 ` Jason Gunthorpe
2020-10-01 11:02 ` Greg KH
` (2 subsequent siblings)
4 siblings, 1 reply; 48+ messages in thread
From: Greg KH @ 2020-10-01 11:01 UTC (permalink / raw)
To: Dave Ertman
Cc: alsa-devel, Kiran Patil, tiwai, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
jgg, Dan Williams, Shiraz Saleem, Parav Pandit
On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> +int ancillary_device_initialize(struct ancillary_device *ancildev)
> +{
> + struct device *dev = &ancildev->dev;
> +
> + dev->bus = &ancillary_bus_type;
> +
> + if (WARN_ON(!dev->parent) || WARN_ON(!ancildev->name) ||
> + WARN_ON(!(dev->type && dev->type->release) && !dev->release))
> + return -EINVAL;
You have a lot of WARN_ON() calls in this patch. That blows up anyone
who runs with panic-on-warn, right?
If these are things that we have to have, then just test and properly
return an error, don't cause a potential crash.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 1/6] Add ancillary bus support
2020-10-01 5:08 Dave Ertman
@ 2020-10-01 5:08 ` Dave Ertman
0 siblings, 0 replies; 48+ messages in thread
From: Dave Ertman @ 2020-10-01 5:08 UTC (permalink / raw)
To: netdev
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().
+
+.. code-block:: c
+
+ struct ancillary_device {
+ struct device dev;
+ const char *name;
+ u32 id;
+ };
+
+If two ancillary_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 ancillary_device.dev.type.release or ancillary_device.dev.release must be
+populated with a non-NULL pointer to successfully register the ancillary_device.
+
+The ancillary_device.dev.parent must also be populated.
+
+Ancillary Device Memory Model and Lifespan
+------------------------------------------
+
+When a kernel driver registers an ancillary_device on the ancillary 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 ancillary_device and register it
+on the ancillary 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
+ancillary_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 ancillary_driver's registering module to perform a container_of()
+call to go from the pointer to the ancillary_device, that is passed during the
+call to the ancillary_driver's probe function, up to the parent object, and then
+have access to the shared object(s).
+
+The memory for the ancillary_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 ancillary_device. The ancillary_driver
+should only consider that this shared object is valid as long as the
+ancillary_device is still registered on the ancillary 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 ancillary_device.
+
+Registering driver must unregister all ancillary devices before its registering
+parent device's remove() is completed.
+
+Ancillary Drivers
+=================
+
+Ancillary 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 ancillary_driver {
+ int (*probe)(struct ancillary_device *,
+ const struct ancillary_device_id *id);
+ int (*remove)(struct ancillary_device *);
+ void (*shutdown)(struct ancillary_device *);
+ int (*suspend)(struct ancillary_device *, pm_message_t);
+ int (*resume)(struct ancillary_device *);
+ struct device_driver driver;
+ const struct ancillary_device_id *id_table;
+ };
+
+Ancillary drivers register themselves with the bus by calling
+ancillary_driver_register(). The id_table contains the match_names of ancillary
+devices that a driver can bind with.
+
+Example Usage
+=============
+
+Ancillary 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 ancillary_device would be to encapsulate it within a
+domain-specific structure defined by the parent device. This structure contains
+the ancillary_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 ancillary_device ancildev;
+ void (*connect)(struct ancillary_device *ancildev);
+ void (*disconnect)(struct ancillary_device *ancildev);
+ void *data;
+ };
+
+The parent device would then register the ancillary_device by calling
+ancillary_device_initialize(), and then ancillary_device_add(), with the pointer
+to the ancildev member of the above structure. The parent would provide a name
+for the ancillary_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 ancillary_driver is registered, based on the match_name, the
+ancillary_driver's probe() is invoked for the matching devices. The
+ancillary_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 ancillary_device *ancildev);
+ void (*receive)(struct ancillary_device *ancildev);
+ };
+
+
+ struct my_driver {
+ struct ancillary_driver ancillary_drv;
+ const struct my_ops ops;
+ };
+
+An example of this type of usage would be:
+
+.. code-block:: c
+
+ const struct ancillary_device_id my_ancillary_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 = {
+ .ancillary_drv = {
+ .driver = {
+ .name = "myancillarydrv",
+ },
+ .id_table = my_ancillary_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 5ef2cfe3a16b..9584ac2ed1f5 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -74,6 +74,7 @@ available subsections can be seen below.
thermal/index
fpga/index
acpi/index
+ ancillary_bus
backlight/lp855x-driver.rst
connector
console
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 0c262c2aeaf2..ba82a045b847 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -5,6 +5,9 @@
menu "Bus devices"
+config ANCILLARY_BUS
+ tristate
+
config ARM_CCI
bool
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index 397e35392bff..1fd238094543 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -3,6 +3,9 @@
# Makefile for the bus drivers.
#
+#Ancillary bus driver
+obj-$(CONFIG_ANCILLARY_BUS) += ancillary.o
+
# Interconnect bus drivers for ARM platforms
obj-$(CONFIG_ARM_CCI) += arm-cci.o
obj-$(CONFIG_ARM_INTEGRATOR_LM) += arm-integrator-lm.o
diff --git a/drivers/bus/ancillary.c b/drivers/bus/ancillary.c
new file mode 100644
index 000000000000..2d940fe5717a
--- /dev/null
+++ b/drivers/bus/ancillary.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Software based bus for Ancillary devices
+ *
+ * Copyright (c) 2019-2020 Intel Corporation
+ *
+ * Please see Documentation/driver-api/ancillary_bus.rst for more information.
+ */
+
+#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/ancillary_bus.h>
+
+static const struct ancillary_device_id *ancillary_match_id(const struct ancillary_device_id *id,
+ const struct ancillary_device *ancildev)
+{
+
+ while (id->name[0]) {
+ const char *p = strrchr(dev_name(&ancildev->dev), '.');
+ int match_size;
+
+ if (!p)
+ continue;
+ match_size = p - dev_name(&ancildev->dev);
+
+ /* use dev_name(&ancildev->dev) prefix before last '.' char to match to */
+ if (!strncmp(dev_name(&ancildev->dev), id->name, match_size))
+ return id;
+ id++;
+ }
+ return NULL;
+}
+
+static int ancillary_match(struct device *dev, struct device_driver *drv)
+{
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+ struct ancillary_driver *ancildrv = to_ancillary_drv(drv);
+
+ return !!ancillary_match_id(ancildrv->id_table, ancildev);
+}
+
+static int ancillary_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", ANCILLARY_MODULE_PREFIX, (int)(p - name),
+ name);
+}
+
+static const struct dev_pm_ops ancillary_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)
+};
+
+struct bus_type ancillary_bus_type = {
+ .name = "ancillary",
+ .match = ancillary_match,
+ .uevent = ancillary_uevent,
+ .pm = &ancillary_dev_pm_ops,
+};
+
+/**
+ * ancillary_device_initialize - check ancillary_device and initialize
+ * @ancildev: ancillary device struct
+ */
+int ancillary_device_initialize(struct ancillary_device *ancildev)
+{
+ struct device *dev = &ancildev->dev;
+
+ dev->bus = &ancillary_bus_type;
+
+ if (WARN_ON(!dev->parent) || WARN_ON(!ancildev->name) ||
+ WARN_ON(!(dev->type && dev->type->release) && !dev->release))
+ return -EINVAL;
+
+ device_initialize(&ancildev->dev);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ancillary_device_initialize);
+
+/**
+ * __ancillary_device_add - add an ancillary bus device
+ * @ancildev: ancillary bus device to add to the bus
+ * @modname: name of the parent device's driver module
+ */
+int __ancillary_device_add(struct ancillary_device *ancildev, const char *modname)
+{
+ struct device *dev = &ancildev->dev;
+ int ret;
+
+ if (WARN_ON(!modname))
+ return -EINVAL;
+
+ ret = dev_set_name(dev, "%s.%s.%d", modname, ancildev->name, ancildev->id);
+ if (ret) {
+ dev_err(dev->parent, "dev_set_name failed for device: %d\n", ret);
+ return ret;
+ }
+
+ ret = device_add(dev);
+ if (ret)
+ dev_err(dev, "adding device failed!: %d\n", ret);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(__ancillary_device_add);
+
+static int ancillary_probe_driver(struct device *dev)
+{
+ struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+ int ret;
+
+ ret = dev_pm_domain_attach(dev, true);
+ if (ret) {
+ dev_warn(&ancildev->dev, "Failed to attach to PM Domain : %d\n", ret);
+ return ret;
+ }
+
+ ret = ancildrv->probe(ancildev, ancillary_match_id(ancildrv->id_table, ancildev));
+ if (ret)
+ dev_pm_domain_detach(dev, true);
+
+ return ret;
+}
+
+static int ancillary_remove_driver(struct device *dev)
+{
+ struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+ int ret;
+
+ ret = ancildrv->remove(ancildev);
+ dev_pm_domain_detach(dev, true);
+
+ return ret;
+}
+
+static void ancillary_shutdown_driver(struct device *dev)
+{
+ struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+
+ ancildrv->shutdown(ancildev);
+}
+
+/**
+ * __ancillary_driver_register - register a driver for ancillary bus devices
+ * @ancildrv: ancillary_driver structure
+ * @owner: owning module/driver
+ */
+int __ancillary_driver_register(struct ancillary_driver *ancildrv, struct module *owner)
+{
+ if (WARN_ON(!ancildrv->probe) || WARN_ON(!ancildrv->remove) ||
+ WARN_ON(!ancildrv->shutdown) || WARN_ON(!ancildrv->id_table))
+ return -EINVAL;
+
+ ancildrv->driver.owner = owner;
+ ancildrv->driver.bus = &ancillary_bus_type;
+ ancildrv->driver.probe = ancillary_probe_driver;
+ ancildrv->driver.remove = ancillary_remove_driver;
+ ancildrv->driver.shutdown = ancillary_shutdown_driver;
+
+ return driver_register(&ancildrv->driver);
+}
+EXPORT_SYMBOL_GPL(__ancillary_driver_register);
+
+static int __init ancillary_bus_init(void)
+{
+ return bus_register(&ancillary_bus_type);
+}
+
+static void __exit ancillary_bus_exit(void)
+{
+ bus_unregister(&ancillary_bus_type);
+}
+
+module_init(ancillary_bus_init);
+module_exit(ancillary_bus_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Ancillary Bus");
+MODULE_AUTHOR("David Ertman <david.m.ertman@intel.com>");
+MODULE_AUTHOR("Kiran Patil <kiran.patil@intel.com>");
diff --git a/include/linux/ancillary_bus.h b/include/linux/ancillary_bus.h
new file mode 100644
index 000000000000..73b13b56403d
--- /dev/null
+++ b/include/linux/ancillary_bus.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2019-2020 Intel Corporation
+ *
+ * Please see Documentation/driver-api/ancillary_bus.rst for more information.
+ */
+
+#ifndef _ANCILLARY_BUS_H_
+#define _ANCILLARY_BUS_H_
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+
+struct ancillary_device {
+ struct device dev;
+ const char *name;
+ u32 id;
+};
+
+struct ancillary_driver {
+ int (*probe)(struct ancillary_device *ancildev, const struct ancillary_device_id *id);
+ int (*remove)(struct ancillary_device *ancildev);
+ void (*shutdown)(struct ancillary_device *ancildev);
+ int (*suspend)(struct ancillary_device *ancildev, pm_message_t state);
+ int (*resume)(struct ancillary_device *ancildev);
+ struct device_driver driver;
+ const struct ancillary_device_id *id_table;
+};
+
+static inline struct ancillary_device *to_ancillary_dev(struct device *dev)
+{
+ return container_of(dev, struct ancillary_device, dev);
+}
+
+static inline struct ancillary_driver *to_ancillary_drv(struct device_driver *drv)
+{
+ return container_of(drv, struct ancillary_driver, driver);
+}
+
+int ancillary_device_initialize(struct ancillary_device *ancildev);
+int __ancillary_device_add(struct ancillary_device *ancildev, const char *modname);
+#define ancillary_device_add(ancildev) __ancillary_device_add(ancildev, KBUILD_MODNAME)
+
+static inline void ancillary_device_unregister(struct ancillary_device *ancildev)
+{
+ device_unregister(&ancildev->dev);
+}
+
+int __ancillary_driver_register(struct ancillary_driver *ancildrv, struct module *owner);
+#define ancillary_driver_register(ancildrv) __ancillary_driver_register(ancildrv, THIS_MODULE)
+
+static inline void ancillary_driver_unregister(struct ancillary_driver *ancildrv)
+{
+ driver_unregister(&ancildrv->driver);
+}
+
+#endif /* _ANCILLARY_BUS_H_ */
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5b08a473cdba..7d596dc30833 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 ANCILLARY_NAME_SIZE 32
+#define ANCILLARY_MODULE_PREFIX "ancillary:"
+
+struct ancillary_device_id {
+ char name[ANCILLARY_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..79e37c4c25b3 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(ancillary_device_id);
+ DEVID_FIELD(ancillary_device_id, name);
+
return 0;
}
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2417dd1dee33..99c4fcd82bf3 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_ancillary_entry(const char *filename, void *symval, char *alias)
+{
+ DEF_FIELD_ADDR(symval, ancillary_device_id, name);
+ sprintf(alias, ANCILLARY_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},
+ {"ancillary", SIZE_ancillary_device_id, do_ancillary_entry},
};
/* Create MODULE_ALIAS() statements.
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Add ancillary bus support
2020-09-30 22:50 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
@ 2020-09-30 23:05 ` Jason Gunthorpe
2020-10-01 11:01 ` Greg KH
` (3 subsequent siblings)
4 siblings, 0 replies; 48+ messages in thread
From: Jason Gunthorpe @ 2020-09-30 23:05 UTC (permalink / raw)
To: Dave Ertman
Cc: alsa-devel, Kiran Patil, tiwai, gregkh, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
Dan Williams, Shiraz Saleem, Parav Pandit
On Wed, Sep 30, 2020 at 03:50:46PM -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
I think you need to send this patch to a lot more mailing lists,
netdev, rdma and linux-kernel at least
The Intel IDXD team also needs this
Jason
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 1/6] Add ancillary bus support
2020-09-30 22:50 [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Dave Ertman
@ 2020-09-30 22:50 ` Dave Ertman
2020-09-30 23:05 ` Jason Gunthorpe
` (4 more replies)
0 siblings, 5 replies; 48+ messages in thread
From: Dave Ertman @ 2020-09-30 22:50 UTC (permalink / raw)
To: alsa-devel
Cc: Kiran Patil, tiwai, gregkh, ranjani.sridharan,
pierre-louis.bossart, Ranjani Sridharan, Fred Oh, broonie, parav,
jgg, Dan Williams, Shiraz Saleem, Parav Pandit
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().
+
+.. code-block:: c
+
+ struct ancillary_device {
+ struct device dev;
+ const char *name;
+ u32 id;
+ };
+
+If two ancillary_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 ancillary_device.dev.type.release or ancillary_device.dev.release must be
+populated with a non-NULL pointer to successfully register the ancillary_device.
+
+The ancillary_device.dev.parent must also be populated.
+
+Ancillary Device Memory Model and Lifespan
+------------------------------------------
+
+When a kernel driver registers an ancillary_device on the ancillary 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 ancillary_device and register it
+on the ancillary 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
+ancillary_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 ancillary_driver's registering module to perform a container_of()
+call to go from the pointer to the ancillary_device, that is passed during the
+call to the ancillary_driver's probe function, up to the parent object, and then
+have access to the shared object(s).
+
+The memory for the ancillary_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 ancillary_device. The ancillary_driver
+should only consider that this shared object is valid as long as the
+ancillary_device is still registered on the ancillary 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 ancillary_device.
+
+Registering driver must unregister all ancillary devices before its registering
+parent device's remove() is completed.
+
+Ancillary Drivers
+=================
+
+Ancillary 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 ancillary_driver {
+ int (*probe)(struct ancillary_device *,
+ const struct ancillary_device_id *id);
+ int (*remove)(struct ancillary_device *);
+ void (*shutdown)(struct ancillary_device *);
+ int (*suspend)(struct ancillary_device *, pm_message_t);
+ int (*resume)(struct ancillary_device *);
+ struct device_driver driver;
+ const struct ancillary_device_id *id_table;
+ };
+
+Ancillary drivers register themselves with the bus by calling
+ancillary_driver_register(). The id_table contains the match_names of ancillary
+devices that a driver can bind with.
+
+Example Usage
+=============
+
+Ancillary 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 ancillary_device would be to encapsulate it within a
+domain-specific structure defined by the parent device. This structure contains
+the ancillary_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 ancillary_device ancildev;
+ void (*connect)(struct ancillary_device *ancildev);
+ void (*disconnect)(struct ancillary_device *ancildev);
+ void *data;
+ };
+
+The parent device would then register the ancillary_device by calling
+ancillary_device_initialize(), and then ancillary_device_add(), with the pointer
+to the ancildev member of the above structure. The parent would provide a name
+for the ancillary_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 ancillary_driver is registered, based on the match_name, the
+ancillary_driver's probe() is invoked for the matching devices. The
+ancillary_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 ancillary_device *ancildev);
+ void (*receive)(struct ancillary_device *ancildev);
+ };
+
+
+ struct my_driver {
+ struct ancillary_driver ancillary_drv;
+ const struct my_ops ops;
+ };
+
+An example of this type of usage would be:
+
+.. code-block:: c
+
+ const struct ancillary_device_id my_ancillary_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 = {
+ .ancillary_drv = {
+ .driver = {
+ .name = "myancillarydrv",
+ },
+ .id_table = my_ancillary_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 5ef2cfe3a16b..9584ac2ed1f5 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -74,6 +74,7 @@ available subsections can be seen below.
thermal/index
fpga/index
acpi/index
+ ancillary_bus
backlight/lp855x-driver.rst
connector
console
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 0c262c2aeaf2..ba82a045b847 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -5,6 +5,9 @@
menu "Bus devices"
+config ANCILLARY_BUS
+ tristate
+
config ARM_CCI
bool
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index 397e35392bff..1fd238094543 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -3,6 +3,9 @@
# Makefile for the bus drivers.
#
+#Ancillary bus driver
+obj-$(CONFIG_ANCILLARY_BUS) += ancillary.o
+
# Interconnect bus drivers for ARM platforms
obj-$(CONFIG_ARM_CCI) += arm-cci.o
obj-$(CONFIG_ARM_INTEGRATOR_LM) += arm-integrator-lm.o
diff --git a/drivers/bus/ancillary.c b/drivers/bus/ancillary.c
new file mode 100644
index 000000000000..2d940fe5717a
--- /dev/null
+++ b/drivers/bus/ancillary.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Software based bus for Ancillary devices
+ *
+ * Copyright (c) 2019-2020 Intel Corporation
+ *
+ * Please see Documentation/driver-api/ancillary_bus.rst for more information.
+ */
+
+#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/ancillary_bus.h>
+
+static const struct ancillary_device_id *ancillary_match_id(const struct ancillary_device_id *id,
+ const struct ancillary_device *ancildev)
+{
+
+ while (id->name[0]) {
+ const char *p = strrchr(dev_name(&ancildev->dev), '.');
+ int match_size;
+
+ if (!p)
+ continue;
+ match_size = p - dev_name(&ancildev->dev);
+
+ /* use dev_name(&ancildev->dev) prefix before last '.' char to match to */
+ if (!strncmp(dev_name(&ancildev->dev), id->name, match_size))
+ return id;
+ id++;
+ }
+ return NULL;
+}
+
+static int ancillary_match(struct device *dev, struct device_driver *drv)
+{
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+ struct ancillary_driver *ancildrv = to_ancillary_drv(drv);
+
+ return !!ancillary_match_id(ancildrv->id_table, ancildev);
+}
+
+static int ancillary_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", ANCILLARY_MODULE_PREFIX, (int)(p - name),
+ name);
+}
+
+static const struct dev_pm_ops ancillary_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)
+};
+
+struct bus_type ancillary_bus_type = {
+ .name = "ancillary",
+ .match = ancillary_match,
+ .uevent = ancillary_uevent,
+ .pm = &ancillary_dev_pm_ops,
+};
+
+/**
+ * ancillary_device_initialize - check ancillary_device and initialize
+ * @ancildev: ancillary device struct
+ */
+int ancillary_device_initialize(struct ancillary_device *ancildev)
+{
+ struct device *dev = &ancildev->dev;
+
+ dev->bus = &ancillary_bus_type;
+
+ if (WARN_ON(!dev->parent) || WARN_ON(!ancildev->name) ||
+ WARN_ON(!(dev->type && dev->type->release) && !dev->release))
+ return -EINVAL;
+
+ device_initialize(&ancildev->dev);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ancillary_device_initialize);
+
+/**
+ * __ancillary_device_add - add an ancillary bus device
+ * @ancildev: ancillary bus device to add to the bus
+ * @modname: name of the parent device's driver module
+ */
+int __ancillary_device_add(struct ancillary_device *ancildev, const char *modname)
+{
+ struct device *dev = &ancildev->dev;
+ int ret;
+
+ if (WARN_ON(!modname))
+ return -EINVAL;
+
+ ret = dev_set_name(dev, "%s.%s.%d", modname, ancildev->name, ancildev->id);
+ if (ret) {
+ dev_err(dev->parent, "dev_set_name failed for device: %d\n", ret);
+ return ret;
+ }
+
+ ret = device_add(dev);
+ if (ret)
+ dev_err(dev, "adding device failed!: %d\n", ret);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(__ancillary_device_add);
+
+static int ancillary_probe_driver(struct device *dev)
+{
+ struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+ int ret;
+
+ ret = dev_pm_domain_attach(dev, true);
+ if (ret) {
+ dev_warn(&ancildev->dev, "Failed to attach to PM Domain : %d\n", ret);
+ return ret;
+ }
+
+ ret = ancildrv->probe(ancildev, ancillary_match_id(ancildrv->id_table, ancildev));
+ if (ret)
+ dev_pm_domain_detach(dev, true);
+
+ return ret;
+}
+
+static int ancillary_remove_driver(struct device *dev)
+{
+ struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+ int ret;
+
+ ret = ancildrv->remove(ancildev);
+ dev_pm_domain_detach(dev, true);
+
+ return ret;
+}
+
+static void ancillary_shutdown_driver(struct device *dev)
+{
+ struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
+ struct ancillary_device *ancildev = to_ancillary_dev(dev);
+
+ ancildrv->shutdown(ancildev);
+}
+
+/**
+ * __ancillary_driver_register - register a driver for ancillary bus devices
+ * @ancildrv: ancillary_driver structure
+ * @owner: owning module/driver
+ */
+int __ancillary_driver_register(struct ancillary_driver *ancildrv, struct module *owner)
+{
+ if (WARN_ON(!ancildrv->probe) || WARN_ON(!ancildrv->remove) ||
+ WARN_ON(!ancildrv->shutdown) || WARN_ON(!ancildrv->id_table))
+ return -EINVAL;
+
+ ancildrv->driver.owner = owner;
+ ancildrv->driver.bus = &ancillary_bus_type;
+ ancildrv->driver.probe = ancillary_probe_driver;
+ ancildrv->driver.remove = ancillary_remove_driver;
+ ancildrv->driver.shutdown = ancillary_shutdown_driver;
+
+ return driver_register(&ancildrv->driver);
+}
+EXPORT_SYMBOL_GPL(__ancillary_driver_register);
+
+static int __init ancillary_bus_init(void)
+{
+ return bus_register(&ancillary_bus_type);
+}
+
+static void __exit ancillary_bus_exit(void)
+{
+ bus_unregister(&ancillary_bus_type);
+}
+
+module_init(ancillary_bus_init);
+module_exit(ancillary_bus_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Ancillary Bus");
+MODULE_AUTHOR("David Ertman <david.m.ertman@intel.com>");
+MODULE_AUTHOR("Kiran Patil <kiran.patil@intel.com>");
diff --git a/include/linux/ancillary_bus.h b/include/linux/ancillary_bus.h
new file mode 100644
index 000000000000..73b13b56403d
--- /dev/null
+++ b/include/linux/ancillary_bus.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2019-2020 Intel Corporation
+ *
+ * Please see Documentation/driver-api/ancillary_bus.rst for more information.
+ */
+
+#ifndef _ANCILLARY_BUS_H_
+#define _ANCILLARY_BUS_H_
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+
+struct ancillary_device {
+ struct device dev;
+ const char *name;
+ u32 id;
+};
+
+struct ancillary_driver {
+ int (*probe)(struct ancillary_device *ancildev, const struct ancillary_device_id *id);
+ int (*remove)(struct ancillary_device *ancildev);
+ void (*shutdown)(struct ancillary_device *ancildev);
+ int (*suspend)(struct ancillary_device *ancildev, pm_message_t state);
+ int (*resume)(struct ancillary_device *ancildev);
+ struct device_driver driver;
+ const struct ancillary_device_id *id_table;
+};
+
+static inline struct ancillary_device *to_ancillary_dev(struct device *dev)
+{
+ return container_of(dev, struct ancillary_device, dev);
+}
+
+static inline struct ancillary_driver *to_ancillary_drv(struct device_driver *drv)
+{
+ return container_of(drv, struct ancillary_driver, driver);
+}
+
+int ancillary_device_initialize(struct ancillary_device *ancildev);
+int __ancillary_device_add(struct ancillary_device *ancildev, const char *modname);
+#define ancillary_device_add(ancildev) __ancillary_device_add(ancildev, KBUILD_MODNAME)
+
+static inline void ancillary_device_unregister(struct ancillary_device *ancildev)
+{
+ device_unregister(&ancildev->dev);
+}
+
+int __ancillary_driver_register(struct ancillary_driver *ancildrv, struct module *owner);
+#define ancillary_driver_register(ancildrv) __ancillary_driver_register(ancildrv, THIS_MODULE)
+
+static inline void ancillary_driver_unregister(struct ancillary_driver *ancildrv)
+{
+ driver_unregister(&ancildrv->driver);
+}
+
+#endif /* _ANCILLARY_BUS_H_ */
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5b08a473cdba..7d596dc30833 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 ANCILLARY_NAME_SIZE 32
+#define ANCILLARY_MODULE_PREFIX "ancillary:"
+
+struct ancillary_device_id {
+ char name[ANCILLARY_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..79e37c4c25b3 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(ancillary_device_id);
+ DEVID_FIELD(ancillary_device_id, name);
+
return 0;
}
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2417dd1dee33..99c4fcd82bf3 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_ancillary_entry(const char *filename, void *symval, char *alias)
+{
+ DEF_FIELD_ADDR(symval, ancillary_device_id, name);
+ sprintf(alias, ANCILLARY_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},
+ {"ancillary", SIZE_ancillary_device_id, do_ancillary_entry},
};
/* Create MODULE_ALIAS() statements.
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
end of thread, other threads:[~2020-10-05 1:21 UTC | newest]
Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-01 5:05 [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Dave Ertman
2020-10-01 5:05 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
2020-10-01 8:32 ` Leon Romanovsky
2020-10-01 17:20 ` Ertman, David M
2020-10-01 17:40 ` Leon Romanovsky
2020-10-01 18:29 ` Parav Pandit
2020-10-01 19:32 ` Leon Romanovsky
2020-10-02 5:29 ` Parav Pandit
2020-10-02 6:20 ` Leon Romanovsky
2020-10-02 8:42 ` Parav Pandit
2020-10-02 11:13 ` Leon Romanovsky
2020-10-02 11:27 ` Parav Pandit
2020-10-02 11:45 ` Leon Romanovsky
2020-10-02 11:56 ` Parav Pandit
2020-10-01 5:05 ` [PATCH 2/6] ASoC: SOF: Introduce descriptors for SOF client Dave Ertman
2020-10-01 5:05 ` [PATCH 3/6] ASoC: SOF: Create client driver for IPC test Dave Ertman
2020-10-01 5:05 ` [PATCH 4/6] ASoC: SOF: ops: Add ops for client registration Dave Ertman
2020-10-01 5:05 ` [PATCH 5/6] ASoC: SOF: Intel: Define " Dave Ertman
2020-10-01 5:05 ` [PATCH 6/6] ASoC: SOF: debug: Remove IPC flood test support in SOF core Dave Ertman
2020-10-03 9:04 ` [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Leon Romanovsky
2020-10-03 9:10 ` Greg KH
2020-10-03 9:10 ` Greg KH
2020-10-03 9:24 ` Leon Romanovsky
2020-10-03 9:24 ` Leon Romanovsky
2020-10-03 9:32 ` Greg KH
2020-10-03 9:32 ` Greg KH
2020-10-05 1:20 ` Ertman, David M
-- strict thread matches above, loose matches on Subject: below --
2020-10-01 5:08 Dave Ertman
2020-10-01 5:08 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
2020-09-30 22:50 [PATCH 0/6] Ancillary bus implementation and SOF multi-client support Dave Ertman
2020-09-30 22:50 ` [PATCH 1/6] Add ancillary bus support Dave Ertman
2020-09-30 23:05 ` Jason Gunthorpe
2020-10-01 11:01 ` Greg KH
2020-10-01 11:46 ` Jason Gunthorpe
2020-10-01 11:54 ` Greg KH
2020-10-01 12:02 ` Jason Gunthorpe
2020-10-01 12:15 ` Greg KH
2020-10-01 18:26 ` Ertman, David M
2020-10-01 11:02 ` Greg KH
2020-10-01 16:30 ` Ertman, David M
2020-10-01 11:05 ` Greg KH
2020-10-01 11:58 ` Jason Gunthorpe
2020-10-01 12:14 ` Greg KH
2020-10-01 14:33 ` Jason Gunthorpe
2020-10-01 14:38 ` Greg KH
2020-10-01 16:06 ` Pierre-Louis Bossart
2020-10-01 17:42 ` Jason Gunthorpe
2020-10-01 14:39 ` Parav Pandit
2020-10-01 14:43 ` Greg KH
2020-10-01 13:27 ` Mark Brown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.