[PATCH 0/6] Ancillary bus implementation and SOF multi-client support
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@int...
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
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.
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
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
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
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
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
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
-----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
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
-----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
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
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
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
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
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
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().
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....
'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
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.
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...
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.
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 */ };
On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
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
As you are creating new sysfs directories, you should have some documentation for them :(
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 debugfunctionality for IPC tests, probes etc. into separate devices. Thisoption would also allow adding client devices based on DSP FWcapabilities 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;
No reference counting? How can you guarantee the lifespan is ok?
- cdev->client_ida = client_ida;
- ancildev = &cdev->ancildev;
- ancildev->name = name;
- ancildev->dev.parent = sdev->dev;
Ah, you guarantee it by making it the parent. Sneaky, but is it really needed?
- 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);
Why have you split this up into two calls? Why not just "sof_client_dev_create() or something like that?
Having to make a sof_* call, and then a separate ancillary_device_* call feels pretty ackward, right?
- if (ret < 0) {
dev_err(sdev->dev, "error: failed to add client dev %s\n", name);put_device(&cdev->ancildev.dev);
Ugh that's a deep knowledge of the ancil code, would be nicer if the caller function handled all of that for you, right?
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);
So you add and remove things from the list, but do not do anything with that list? Why a list at all?
- ancillary_device_unregister(&cdev->ancildev);
Does this free the expected memory? I think so, but commenting that it does is nice :)
+} +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) */
Why the "()"?
thanks,
greg k-h
On Thu, 2020-10-01 at 15:02 +0200, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
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
As you are creating new sysfs directories, you should have some documentation for them :(
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 handledby
'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 separateout the debug
functionality for IPC tests, probes etc. into separatedevices. This
option would also allow adding client devices based on DSP FWcapabilities 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;
No reference counting? How can you guarantee the lifespan is ok?
- cdev->client_ida = client_ida;
- ancildev = &cdev->ancildev;
- ancildev->name = name;
- ancildev->dev.parent = sdev->dev;
Ah, you guarantee it by making it the parent. Sneaky, but is it really needed?
Thanks for your review, Greg. This is needed because ancillary_device_initialize() actually requires the parent to be set as a precondition for it to succeed.
- 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 ancillarydevice %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 clientdev %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);
Why have you split this up into two calls? Why not just "sof_client_dev_create() or something like that?
Having to make a sof_* call, and then a separate ancillary_device_* call feels pretty ackward, right?
The split was suggested to make the error unwind calls more intuitive. In the case of sof_client_dev_alloc() failure, we'd free the IDA and free the cdev. Whereas in the case of ancillary_device_add() failure, we'd rely upon put_device() to free the memory.
But as far as users go, they only call they'd need to make to register a client is sof_client_dev_register().
- if (ret < 0) {
dev_err(sdev->dev, "error: failed to add client dev%s\n", name);
put_device(&cdev->ancildev.dev);Ugh that's a deep knowledge of the ancil code, would be nicer if the caller function handled all of that for you, right?
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);
So you add and remove things from the list, but do not do anything with that list? Why a list at all?
In the current implementation, we dont really use the list but as we add more clients SOF driver, the list will be used to determine which client to pass the IPC messages from the DSP to for example.
- ancillary_device_unregister(&cdev->ancildev);
Does this free the expected memory? I think so, but commenting that it does is nice :)
Sure, will do.
+} +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) */
Why the "()"?
Will remove.
Thanks, Ranjani
On Thu, 2020-10-01 at 15:02 +0200, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
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
As you are creating new sysfs directories, you should have some documentation for them :(
Hi Greg,
We are not adding any sysfs entries in this series.
Thanks, Ranjani
On Thu, Oct 01, 2020 at 10:16:00PM +0000, Sridharan, Ranjani wrote:
On Thu, 2020-10-01 at 15:02 +0200, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
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
As you are creating new sysfs directories, you should have some documentation for them :(
Hi Greg,
We are not adding any sysfs entries in this series.
You added directories in sysfs, right?
On Fri, 2020-10-02 at 06:53 +0200, gregkh@linuxfoundation.org wrote:
On Thu, Oct 01, 2020 at 10:16:00PM +0000, Sridharan, Ranjani wrote:
On Thu, 2020-10-01 at 15:02 +0200, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
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
As you are creating new sysfs directories, you should have some documentation for them :(
Hi Greg,
We are not adding any sysfs entries in this series.
You added directories in sysfs, right?
Hi Greg,
We are not adding any sysfs directories. The only change in the /sys directory will be the new ancillary devices created in the /sys/bus/ancillary/devices directory ie snd_sof_client.ipc_test.0 and snd_sof_client.ipc_test.1. In the following patches, we're adding debugfs entries for the ipc test clients but no other sysfs changes.
Is it required to add documentation for these as well?
Thanks, Ranjani
On Fri, Oct 02, 2020 at 05:07:13PM +0000, Sridharan, Ranjani wrote:
On Fri, 2020-10-02 at 06:53 +0200, gregkh@linuxfoundation.org wrote:
On Thu, Oct 01, 2020 at 10:16:00PM +0000, Sridharan, Ranjani wrote:
On Thu, 2020-10-01 at 15:02 +0200, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
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
As you are creating new sysfs directories, you should have some documentation for them :(
Hi Greg,
We are not adding any sysfs entries in this series.
You added directories in sysfs, right?
Hi Greg,
We are not adding any sysfs directories.
Really? Then what does creating these new devices do in sysfs? If nothing, then why are they being used at all? :)
The only change in the /sys directory will be the new ancillary devices created in the /sys/bus/ancillary/devices directory ie snd_sof_client.ipc_test.0 and snd_sof_client.ipc_test.1.
That is what I was referring to.
In the following patches, we're adding debugfs entries for the ipc test clients but no other sysfs changes.
Is it required to add documentation for these as well?
Why would you not document them? If you don't do anything with these devices, then why even use them? debugfs does not require sysfs entries, so I fail to see the need for using this api at all here...
thanks,
greg k-h
On Sat, 2020-10-03 at 11:02 +0200, gregkh@linuxfoundation.org wrote:
On Fri, Oct 02, 2020 at 05:07:13PM +0000, Sridharan, Ranjani wrote:
On Fri, 2020-10-02 at 06:53 +0200, gregkh@linuxfoundation.org wrote:
On Thu, Oct 01, 2020 at 10:16:00PM +0000, Sridharan, Ranjani wrote:
On Thu, 2020-10-01 at 15:02 +0200, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
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
As you are creating new sysfs directories, you should have some documentation for them :(
Hi Greg,
We are not adding any sysfs entries in this series.
You added directories in sysfs, right?
Hi Greg,
We are not adding any sysfs directories.
Really? Then what does creating these new devices do in sysfs? If nothing, then why are they being used at all? :)
The only change in the /sys directory will be the new ancillary devices created in the /sys/bus/ancillary/devices directory ie snd_sof_client.ipc_test.0 and snd_sof_client.ipc_test.1.
That is what I was referring to.
In the following patches, we're adding debugfs entries for the ipc test clients but no other sysfs changes.
Is it required to add documentation for these as well?
Why would you not document them? If you don't do anything with these devices, then why even use them? debugfs does not require sysfs entries, so I fail to see the need for using this api at all here...
Hi Greg,
Pardon my ignorance here a bit. Typically, when registering platform devices, we've not added any documentation to highlight how they are used. Of course thats no excuse for not doing this right.
But just to clarify so that we can fix this properly in the next version, are we expected to add documentation for the directories added in the /sys/bus (ie /sys/bus/ancillary, /sys/bus/ancillary/devices, /sys/bus/ancillary/drivers etc) and also for the devices and drivers added by the SOF driver?
Thanks, Ranjani
On Mon, Oct 05, 2020 at 02:35:09AM +0000, Sridharan, Ranjani wrote:
On Sat, 2020-10-03 at 11:02 +0200, gregkh@linuxfoundation.org wrote:
On Fri, Oct 02, 2020 at 05:07:13PM +0000, Sridharan, Ranjani wrote:
On Fri, 2020-10-02 at 06:53 +0200, gregkh@linuxfoundation.org wrote:
On Thu, Oct 01, 2020 at 10:16:00PM +0000, Sridharan, Ranjani wrote:
On Thu, 2020-10-01 at 15:02 +0200, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote: > 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
As you are creating new sysfs directories, you should have some documentation for them :(
Hi Greg,
We are not adding any sysfs entries in this series.
You added directories in sysfs, right?
Hi Greg,
We are not adding any sysfs directories.
Really? Then what does creating these new devices do in sysfs? If nothing, then why are they being used at all? :)
The only change in the /sys directory will be the new ancillary devices created in the /sys/bus/ancillary/devices directory ie snd_sof_client.ipc_test.0 and snd_sof_client.ipc_test.1.
That is what I was referring to.
In the following patches, we're adding debugfs entries for the ipc test clients but no other sysfs changes.
Is it required to add documentation for these as well?
Why would you not document them? If you don't do anything with these devices, then why even use them? debugfs does not require sysfs entries, so I fail to see the need for using this api at all here...
Hi Greg,
Pardon my ignorance here a bit. Typically, when registering platform devices, we've not added any documentation to highlight how they are used. Of course thats no excuse for not doing this right.
But just to clarify so that we can fix this properly in the next version, are we expected to add documentation for the directories added in the /sys/bus (ie /sys/bus/ancillary, /sys/bus/ancillary/devices, /sys/bus/ancillary/drivers etc) and also for the devices and drivers added by the SOF driver?
You are using a brand-new interface, which is designed to represent things in the driver model and sysfs properly, and yet your usage doesn't actually take advantage of the driver model or sysfs at all?
That implies to me that you are using this incorrectly.
greg k-h
> As you are creating new sysfs directories, you should have > some > documentation for them :( Hi Greg,
We are not adding any sysfs entries in this series.
You added directories in sysfs, right?
Hi Greg,
We are not adding any sysfs directories.
Really? Then what does creating these new devices do in sysfs? If nothing, then why are they being used at all? :)
The only change in the /sys directory will be the new ancillary devices created in the /sys/bus/ancillary/devices directory ie snd_sof_client.ipc_test.0 and snd_sof_client.ipc_test.1.
That is what I was referring to.
In the following patches, we're adding debugfs entries for the ipc test clients but no other sysfs changes.
Is it required to add documentation for these as well?
Why would you not document them? If you don't do anything with these devices, then why even use them? debugfs does not require sysfs entries, so I fail to see the need for using this api at all here...
Hi Greg,
Pardon my ignorance here a bit. Typically, when registering platform devices, we've not added any documentation to highlight how they are used. Of course thats no excuse for not doing this right.
But just to clarify so that we can fix this properly in the next version, are we expected to add documentation for the directories added in the /sys/bus (ie /sys/bus/ancillary, /sys/bus/ancillary/devices, /sys/bus/ancillary/drivers etc) and also for the devices and drivers added by the SOF driver?
You are using a brand-new interface, which is designed to represent things in the driver model and sysfs properly, and yet your usage doesn't actually take advantage of the driver model or sysfs at all?
That implies to me that you are using this incorrectly.
We are taking advantage of 'standard' sysfs capabilities, e.g. we get a power/ directory and can disable pm_runtime if we chose to do so.
But the point is that for now we haven't added domain specific properties with attributes.
For example, I noodled with this code last week to replace the platform devices with ancillary devices in the Intel SoundWire code, and I get this in sysfs:
root@plb:/sys/bus/ancillary/devices/soundwire_intel.link.0# ls -l total 0 lrwxrwxrwx 1 root root 0 Oct 2 15:48 driver -> ../../../../bus/ancillary/drivers/intel-sdw lrwxrwxrwx 1 root root 0 Oct 5 10:12 firmware_node -> ../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:63/PRP00001:00 drwxr-xr-x 2 root root 0 Oct 5 10:12 power drwxr-xr-x 5 root root 0 Oct 2 15:48 sdw-master-0 lrwxrwxrwx 1 root root 0 Oct 2 15:48 subsystem -> ../../../../bus/ancillary -rw-r--r-- 1 root root 4096 Oct 2 15:48 uevent
What would you want us to document here?
On Mon, Oct 05, 2020 at 10:18:21AM -0500, Pierre-Louis Bossart wrote:
> > As you are creating new sysfs directories, you should have > > some > > documentation for them :( > Hi Greg, > > We are not adding any sysfs entries in this series.
You added directories in sysfs, right?
Hi Greg,
We are not adding any sysfs directories.
Really? Then what does creating these new devices do in sysfs? If nothing, then why are they being used at all? :)
The only change in the /sys directory will be the new ancillary devices created in the /sys/bus/ancillary/devices directory ie snd_sof_client.ipc_test.0 and snd_sof_client.ipc_test.1.
That is what I was referring to.
In the following patches, we're adding debugfs entries for the ipc test clients but no other sysfs changes.
Is it required to add documentation for these as well?
Why would you not document them? If you don't do anything with these devices, then why even use them? debugfs does not require sysfs entries, so I fail to see the need for using this api at all here...
Hi Greg,
Pardon my ignorance here a bit. Typically, when registering platform devices, we've not added any documentation to highlight how they are used. Of course thats no excuse for not doing this right.
But just to clarify so that we can fix this properly in the next version, are we expected to add documentation for the directories added in the /sys/bus (ie /sys/bus/ancillary, /sys/bus/ancillary/devices, /sys/bus/ancillary/drivers etc) and also for the devices and drivers added by the SOF driver?
You are using a brand-new interface, which is designed to represent things in the driver model and sysfs properly, and yet your usage doesn't actually take advantage of the driver model or sysfs at all?
That implies to me that you are using this incorrectly.
We are taking advantage of 'standard' sysfs capabilities, e.g. we get a power/ directory and can disable pm_runtime if we chose to do so.
But the point is that for now we haven't added domain specific properties with attributes.
For example, I noodled with this code last week to replace the platform devices with ancillary devices in the Intel SoundWire code, and I get this in sysfs:
root@plb:/sys/bus/ancillary/devices/soundwire_intel.link.0# ls -l total 0 lrwxrwxrwx 1 root root 0 Oct 2 15:48 driver -> ../../../../bus/ancillary/drivers/intel-sdw lrwxrwxrwx 1 root root 0 Oct 5 10:12 firmware_node -> ../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:63/PRP00001:00 drwxr-xr-x 2 root root 0 Oct 5 10:12 power drwxr-xr-x 5 root root 0 Oct 2 15:48 sdw-master-0 lrwxrwxrwx 1 root root 0 Oct 2 15:48 subsystem -> ../../../../bus/ancillary -rw-r--r-- 1 root root 4096 Oct 2 15:48 uevent
What would you want us to document here?
What you think should be in the documentation to help people out with these devices...
Anyway, this thread is long-dead due to it being only on alsa-devel, I'm not going to respond anymore here, thanks.
greg k-h
On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
- 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);
It would be nice to have the ida_simple_remove() done in this unwind section here too for consistency.
+int sof_client_dev_register(struct snd_sof_dev *sdev, const char *name, struct ida *client_ida) +{
I know the hard limit on line length got raised but can we wrap this please?
On Thu, 2020-10-01 at 14:38 +0100, Mark Brown wrote:
On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
- ret = ancillary_device_initialize(ancildev);
- if (ret < 0) {
dev_err(sdev->dev, "error: failed to initialize clientdev %s\n", name);
ida_simple_remove(client_ida, ancildev->id);goto err_free;- }
- return cdev;
+err_free:
- kfree(cdev);
It would be nice to have the ida_simple_remove() done in this unwind section here too for consistency.
ack
+int sof_client_dev_register(struct snd_sof_dev *sdev, const char *name, struct ida *client_ida) +{
I know the hard limit on line length got raised but can we wrap this please?
ack.
Thanks,Ranjani
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);
On Wed, Sep 30, 2020 at 03:50:48PM -0700, Dave Ertman wrote:
+/*
- 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);
These debugfs files are never removed, why not?
thanks,
greg k-h
On Thu, 2020-10-01 at 15:04 +0200, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:48PM -0700, Dave Ertman wrote:
+/*
- 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 beforeruntime 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);These debugfs files are never removed, why not?
Looks like a miss. Will fix.
Thanks, Ranjani
On Wed, Sep 30, 2020 at 03:50:48PM -0700, Dave Ertman wrote:
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
Am I reading this series correct in that this is the only "user" of the new ancilicary bus/driver code?
If so, why is it even needed? These are just debugfs files for testing, why does that need to be in a separate device? What is being "shared" here that needs multiple struct devices to handle?
confused,
greg k-h
Thanks for the review Greg.
On 10/1/20 8:09 AM, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:48PM -0700, Dave Ertman wrote:
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
Am I reading this series correct in that this is the only "user" of the new ancilicary bus/driver code?
This is the first user, and it was meant to demonstrate how the client is instantiated and communicates with hardware controlled by the parent. The next users will be 'audio probes', which provides the ability to extract/inject data into the DSP. We also want to split the existing audio cards into several pieces, e.g. the HDMI parts should really be presented as a separate card.
The other users will be networking/RDMA, which were actually the first to suggest this bus.
If so, why is it even needed? These are just debugfs files for testing, why does that need to be in a separate device? What is being "shared" here that needs multiple struct devices to handle?
confused,
The parent PCI device provides access to the DSP firmware/hardware and is in complete control of the IPC with the DSP firmware. The parent plays the role of a 'server' providing shared hardware access to multiple clients.
Why is this needed?
With the current audio solutions, we have a monolithic solution that has proven difficult to maintain. We'd really like to expose unrelated DSP functionality with different devices.
The best example is really HDMI. HDMI/DP audio interfaces are controlled by the same hardware, but are logically independent. What we end-up doing is re-adding the same solution over and over for each machine driver:
sound/soc/intel/boards$ git grep hda_dsp_hdmi_build_controls bxt_da7219_max98357a.c: return hda_dsp_hdmi_build_controls(card, component); bxt_rt298.c: return hda_dsp_hdmi_build_controls(card, component); cml_rt1011_rt5682.c: return hda_dsp_hdmi_build_controls(card, component); ehl_rt5660.c: return hda_dsp_hdmi_build_controls(card, pcm->codec_dai->component); glk_rt5682_max98357a.c: return hda_dsp_hdmi_build_controls(card, component); hda_dsp_common.c:int hda_dsp_hdmi_build_controls(struct snd_soc_card *card, hda_dsp_common.h:int hda_dsp_hdmi_build_controls(struct snd_soc_card *card, hda_dsp_common.h:static inline int hda_dsp_hdmi_build_controls(struct snd_soc_card *card, skl_hda_dsp_common.h: return hda_dsp_hdmi_build_controls(card, component); sof_da7219_max98373.c: return hda_dsp_hdmi_build_controls(card, sof_pcm512x.c: return hda_dsp_hdmi_build_controls(card, pcm->codec_dai->component); sof_rt5682.c: return hda_dsp_hdmi_build_controls(card, component); sof_sdw_hdmi.c: return hda_dsp_hdmi_build_controls(card, component);
and we also keep adding HDMI-related ASoC topology definitions for all the cards.
It would make a lot more sense if we could have ONE HDMI/DP card which is created, instead of managing HDMI/DP from the card that is supposed to deal with local accessories based on HDaudio/DMIC/SoundWire/I2S.
The audio probes are similar, we want to have a single probe client instead of adding audio probes to every single card we have to maintain.
On platforms where the DSP can deal with sensors, this would also allow the parent to expose IIO and HID clients.
Going back to this IPC test, maybe the commit message is unclear: we already have this functionality in the mainline, it's been very helpful for stress tests. What this patch shows is that moving the functionality to a client makes it possible to scale to 2 or more clients with a simple set of register/unregister. The device model makes it really easy to scale.
So yes, you are correct that for now there is a single user with very limited functionality. This is intentional to make the reviews simpler, but if/when this bus is part of the mainline we'll have additional users, and not just from Intel if you look at the reviewed-by tags.
We might even remove the platform devices used for the SoundWire master and use this instead :-)
Does this help?
-----Original Message----- From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Sent: Thursday, October 1, 2020 6:55 AM To: Greg KH gregkh@linuxfoundation.org; Ertman, David M david.m.ertman@intel.com Cc: alsa-devel@alsa-project.org; tiwai@suse.de; broonie@kernel.org; Sridharan, Ranjani ranjani.sridharan@intel.com; jgg@nvidia.com; parav@nvidia.com; Ranjani Sridharan ranjani.sridharan@linux.intel.com; Fred Oh fred.oh@linux.intel.com Subject: Re: [PATCH 3/6] ASoC: SOF: Create client driver for IPC test
Thanks for the review Greg.
On 10/1/20 8:09 AM, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:48PM -0700, Dave Ertman wrote:
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
Am I reading this series correct in that this is the only "user" of the new ancilicary bus/driver code?
This is the first user, and it was meant to demonstrate how the client is instantiated and communicates with hardware controlled by the parent. The next users will be 'audio probes', which provides the ability to extract/inject data into the DSP. We also want to split the existing audio cards into several pieces, e.g. the HDMI parts should really be presented as a separate card.
The other users will be networking/RDMA, which were actually the first to suggest this bus.
If so, why is it even needed? These are just debugfs files for testing, why does that need to be in a separate device? What is being "shared" here that needs multiple struct devices to handle?
confused,
The parent PCI device provides access to the DSP firmware/hardware and is in complete control of the IPC with the DSP firmware. The parent plays the role of a 'server' providing shared hardware access to multiple clients.
Why is this needed?
With the current audio solutions, we have a monolithic solution that has proven difficult to maintain. We'd really like to expose unrelated DSP functionality with different devices.
The best example is really HDMI. HDMI/DP audio interfaces are controlled by the same hardware, but are logically independent. What we end-up doing is re-adding the same solution over and over for each machine driver:
sound/soc/intel/boards$ git grep hda_dsp_hdmi_build_controls bxt_da7219_max98357a.c: return hda_dsp_hdmi_build_controls(card, component); bxt_rt298.c: return hda_dsp_hdmi_build_controls(card, component); cml_rt1011_rt5682.c: return hda_dsp_hdmi_build_controls(card, component); ehl_rt5660.c: return hda_dsp_hdmi_build_controls(card, pcm->codec_dai->component); glk_rt5682_max98357a.c: return hda_dsp_hdmi_build_controls(card, component); hda_dsp_common.c:int hda_dsp_hdmi_build_controls(struct snd_soc_card *card, hda_dsp_common.h:int hda_dsp_hdmi_build_controls(struct snd_soc_card *card, hda_dsp_common.h:static inline int hda_dsp_hdmi_build_controls(struct snd_soc_card *card, skl_hda_dsp_common.h: return hda_dsp_hdmi_build_controls(card, component); sof_da7219_max98373.c: return hda_dsp_hdmi_build_controls(card, sof_pcm512x.c: return hda_dsp_hdmi_build_controls(card, pcm->codec_dai->component); sof_rt5682.c: return hda_dsp_hdmi_build_controls(card, component); sof_sdw_hdmi.c: return hda_dsp_hdmi_build_controls(card, component);
and we also keep adding HDMI-related ASoC topology definitions for all the cards.
It would make a lot more sense if we could have ONE HDMI/DP card which is created, instead of managing HDMI/DP from the card that is supposed to deal with local accessories based on HDaudio/DMIC/SoundWire/I2S.
The audio probes are similar, we want to have a single probe client instead of adding audio probes to every single card we have to maintain.
On platforms where the DSP can deal with sensors, this would also allow the parent to expose IIO and HID clients.
Going back to this IPC test, maybe the commit message is unclear: we already have this functionality in the mainline, it's been very helpful for stress tests. What this patch shows is that moving the functionality to a client makes it possible to scale to 2 or more clients with a simple set of register/unregister. The device model makes it really easy to scale.
So yes, you are correct that for now there is a single user with very limited functionality. This is intentional to make the reviews simpler, but if/when this bus is part of the mainline we'll have additional users, and not just from Intel if you look at the reviewed-by tags.
We might even remove the platform devices used for the SoundWire master and use this instead :-)
Does this help?
The reason we switched from using RDMA/LAN as an example was the impasse in trying to get patches for all three moving parts up through two different trees at the same time (netdev and rdma). It was becoming a huge mess. The Intel sound driver folks provided a consumer in a single tree to make submission smoother.
The patches for the LAN driver and RDMA are waiting to see what the final form of ancillary bus will be so that they can match to the final API. We still have the same use case in the LAN/RDMA model.
-DaveE
On Wed, Sep 30, 2020 at 03:50:48PM -0700, Dave Ertman wrote:
+/* 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)
Again, some word wrapping would be nice.
The flood_duration_test parameter is boolean which is often a warning sign for uncler interfaces and...
- 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);- }
...appears to only control this debug print which I'd never have guessed from the name.
- /* set test completion criterion */
- ret = flood_duration_test ? kstrtoul(string, 0, &ipc_duration_ms) :
kstrtoul(string, 0, &ipc_count);- if (ret < 0)
goto out;
Please write normal conditional statements for legibility.
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;
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
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;
Sorry about the two copies. That was my fault. On my first send my subscription to alsa-devel had not gone through and I forgot to suppress-cc on the second send.
-DaveE
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Wednesday, September 30, 2020 10:58 PM 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 Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Wed, Sep 30, 2020 at 03:50:45PM -0700, Dave Ertman wrote:
Brief history of Ancillary Bus
<snip>
Did you send 2 copies of this? Which one is the "correct" one to review?
confused,
greg k-h
On Wed, Sep 30, 2020 at 03:50:45PM -0700, Dave Ertman wrote:
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.
So this will not work for the ice driver and/or irdma drivers? It would be great to see how they work for this as well as getting those maintainers to review and sign off on this implementation as well. Don't ignore those developers, that's a bit "odd", don't you think?
To drop them from the review process is actually kind of rude, what happens if this gets merged without their input?
And the name, why was it changed and what does it mean? For non-native english speakers this is going to be rough, given that I as a native english speaker had to go look up the word in a dictionary to fully understand what you are trying to do with that name.
Naming is hard, but I think this name is really hard to explain and understand, don't you think?
thanks,
greg k-h
I have also sent this patch set to netdev and linux-rdma mailing lists.
-DaveE
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Thursday, October 1, 2020 12:14 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 Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Wed, Sep 30, 2020 at 03:50:45PM -0700, Dave Ertman wrote:
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.
So this will not work for the ice driver and/or irdma drivers? It would be great to see how they work for this as well as getting those maintainers to review and sign off on this implementation as well. Don't ignore those developers, that's a bit "odd", don't you think?
To drop them from the review process is actually kind of rude, what happens if this gets merged without their input?
And the name, why was it changed and what does it mean? For non-native english speakers this is going to be rough, given that I as a native english speaker had to go look up the word in a dictionary to fully understand what you are trying to do with that name.
Naming is hard, but I think this name is really hard to explain and understand, don't you think?
thanks,
greg k-h
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Thursday, October 1, 2020 9:11 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 Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Thu, Oct 01, 2020 at 03:55:46PM +0000, Ertman, David M wrote:
I have also sent this patch set to netdev and linux-rdma mailing lists.
And not cc:ed us on it? Wonderful, we will never see the review comments, and the developers there will not see ours :(
{sigh}
greg k-h
Yes, mea culpa, I did hose up this first version submission. I am not used to dealing with multiple mailing lists at the same time.
The submission of version 2 will be all mailing lists and cc'd in a single send.
I will learn from my mistake.
-DaveE
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Thursday, October 1, 2020 12:14 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 Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Wed, Sep 30, 2020 at 03:50:45PM -0700, Dave Ertman wrote:
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.
So this will not work for the ice driver and/or irdma drivers? It would be great to see how they work for this as well as getting those maintainers to review and sign off on this implementation as well. Don't ignore those developers, that's a bit "odd", don't you think?
To drop them from the review process is actually kind of rude, what happens if this gets merged without their input?
And the name, why was it changed and what does it mean? For non-native english speakers this is going to be rough, given that I as a native english speaker had to go look up the word in a dictionary to fully understand what you are trying to do with that name.
Through our internal review process, objections were raised on naming the new bus virtual bus. The main objection was that virtual bus was too close to virtio, virtchnl, etc., that /sys/bus/virtual would be confused with /sys/bus/virtio, and there is just a lot of 'virt' stuff in the kernel already.
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
adjective - providing necessary support to the primary activities or operation of an organization, institution, industry, or system.
Changing from ancillary would be a small pain, but do-able if ancillary is really objectionable. Do you have any suggestions on a name that would be more tolerable?
I would like to finalize the name issue before going farther though 😊
Thanks for your review! -DaveE
Naming is hard, but I think this name is really hard to explain and understand, don't you think?
thanks,
greg k-h
On Fri, Oct 02, 2020 at 08:23:49PM +0000, Ertman, David M wrote:
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Thursday, October 1, 2020 12:14 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 Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Wed, Sep 30, 2020 at 03:50:45PM -0700, Dave Ertman wrote:
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.
So this will not work for the ice driver and/or irdma drivers? It would be great to see how they work for this as well as getting those maintainers to review and sign off on this implementation as well. Don't ignore those developers, that's a bit "odd", don't you think?
To drop them from the review process is actually kind of rude, what happens if this gets merged without their input?
And the name, why was it changed and what does it mean? For non-native english speakers this is going to be rough, given that I as a native english speaker had to go look up the word in a dictionary to fully understand what you are trying to do with that name.
Through our internal review process, objections were raised on naming the new bus virtual bus. The main objection was that virtual bus was too close to virtio, virtchnl, etc., that /sys/bus/virtual would be confused with /sys/bus/virtio, and there is just a lot of 'virt' stuff in the kernel already.
We already have a virtual bus/location in the driver model today, has that confused anyone? I see this as an extension of that logic and ideally, those users will be moved over to this interface over time as well.
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
"not hating it", while sometimes is a good thing, for something that I am going to have to tell everyone to go use, I would like to at least "like it". And right now I don't like it...
I think we should go back to "virtual" for now, or, if the people who didn't like it on your "internal" reviews wish to participate here and defend their choice, I would be glad to listen to that reasoning.
thanks,
greg k-h
On Sat, Oct 03, 2020 at 11:08:55AM +0200, Greg KH wrote:
On Fri, Oct 02, 2020 at 08:23:49PM +0000, Ertman, David M wrote:
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Thursday, October 1, 2020 12:14 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 Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Wed, Sep 30, 2020 at 03:50:45PM -0700, Dave Ertman wrote:
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.
So this will not work for the ice driver and/or irdma drivers? It would be great to see how they work for this as well as getting those maintainers to review and sign off on this implementation as well. Don't ignore those developers, that's a bit "odd", don't you think?
To drop them from the review process is actually kind of rude, what happens if this gets merged without their input?
And the name, why was it changed and what does it mean? For non-native english speakers this is going to be rough, given that I as a native english speaker had to go look up the word in a dictionary to fully understand what you are trying to do with that name.
Through our internal review process, objections were raised on naming the new bus virtual bus. The main objection was that virtual bus was too close to virtio, virtchnl, etc., that /sys/bus/virtual would be confused with /sys/bus/virtio, and there is just a lot of 'virt' stuff in the kernel already.
We already have a virtual bus/location in the driver model today, has that confused anyone? I see this as an extension of that logic and ideally, those users will be moved over to this interface over time as well.
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
"not hating it", while sometimes is a good thing, for something that I am going to have to tell everyone to go use, I would like to at least "like it". And right now I don't like it...
I think we should go back to "virtual" for now, or, if the people who didn't like it on your "internal" reviews wish to participate here and defend their choice, I would be glad to listen to that reasoning.
Also, the fact that we are even talking about a core kernel function on only the alsa-devel list is pretty horrible. I'm just going to drop this whole thread and wait until you can submit it properly before responding anymore on it.
greg k-h
From: Greg KH gregkh@linuxfoundation.org Sent: Saturday, October 3, 2020 2:39 PM
On Fri, Oct 02, 2020 at 08:23:49PM +0000, Ertman, David M wrote:
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Thursday, October 1, 2020 12:14 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 Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Wed, Sep 30, 2020 at 03:50:45PM -0700, Dave Ertman wrote:
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.
So this will not work for the ice driver and/or irdma drivers? It would be great to see how they work for this as well as getting those maintainers to review and sign off on this implementation as well. Don't ignore those developers, that's a bit "odd", don't you think?
To drop them from the review process is actually kind of rude, what happens if this gets merged without their input?
And the name, why was it changed and what does it mean? For non-native english speakers this is going to be rough, given that I as a native english speaker had to go look up the word in a dictionary to fully understand what you are trying to do with that name.
Through our internal review process, objections were raised on naming the new bus virtual bus. The main objection was that virtual bus was too close to virtio, virtchnl, etc., that /sys/bus/virtual would be confused with /sys/bus/virtio, and there is just a lot of 'virt' stuff in the kernel
already.
We already have a virtual bus/location in the driver model today, has that confused anyone? I see this as an extension of that logic and ideally, those users will be moved over to this interface over time as well.
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
"not hating it", while sometimes is a good thing, for something that I am going to have to tell everyone to go use, I would like to at least "like it". And right now I don't like it...
I think we should go back to "virtual" for now, or, if the people who didn't like it on your "internal" reviews wish to participate here and defend their choice, I would be glad to listen to that reasoning.
Like Greg and Leon, I was no exception to look up dictionary to understand the meaning on my first review. But I don't have strong opinion.
Since intended use of the bus is to create sub devices, either for matching service purpose or for actual subdevice usage,
How about naming the bus, 'subdev_bus'?
[ Ugh, as other have lameneted, I was not copied on this thread so I could not respond in real time. Let me be another to say, please copy all impacted lists and stakeholders on patches. ]
On Sat, 2020-10-03 at 11:08 +0200, Greg KH wrote: [..]
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
"not hating it", while sometimes is a good thing, for something that I am going to have to tell everyone to go use, I would like to at least "like it". And right now I don't like it...
I think we should go back to "virtual" for now, or, if the people who didn't like it on your "internal" reviews wish to participate here and defend their choice, I would be glad to listen to that reasoning.
I came out strongly against "virtual" because there is nothing virtual about these devices, they are functional partitions of the parent device. Also, /sys/devices/virtual is already the land of unparented / software-defined devices. Having /sys/devices/virtual alongside that is not quite a namespace collision, but it's certainly namespace confusion in my view.
I proposed other names, the team came back with "ancillary" which was not my first choice, but perfectly suitable. In deference to the people doing the work I let their choice stand.
It is an uncomfortable position being a middle tier reviewer of pre- release patch sets when the patch set can still be de-railed by preference nits. A lack of deference makes it a difficult job to convince people "hey my internal review will save you some time upstream", when in practice they can see the opposite is true.
-----Original Message----- From: Williams, Dan J dan.j.williams@intel.com Sent: Sunday, October 4, 2020 4:46 PM To: Ertman, David M david.m.ertman@intel.com; gregkh@linuxfoundation.org Cc: pierre-louis.bossart@linux.intel.com; parav@nvidia.com; broonie@kernel.org; jgg@nvidia.com; Sridharan, Ranjani ranjani.sridharan@intel.com; alsa-devel@alsa-project.org; tiwai@suse.de Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
[ Ugh, as other have lameneted, I was not copied on this thread so I could not respond in real time. Let me be another to say, please copy all impacted lists and stakeholders on patches. ]
On Sat, 2020-10-03 at 11:08 +0200, Greg KH wrote: [..]
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
"not hating it", while sometimes is a good thing, for something that I am going to have to tell everyone to go use, I would like to at least "like it". And right now I don't like it...
I think we should go back to "virtual" for now, or, if the people who didn't like it on your "internal" reviews wish to participate here and defend their choice, I would be glad to listen to that reasoning.
I came out strongly against "virtual" because there is nothing virtual about these devices, they are functional partitions of the parent device. Also, /sys/devices/virtual is already the land of unparented / software-defined devices. Having /sys/devices/virtual alongside that is not quite a namespace collision, but it's certainly namespace confusion in my view.
I proposed other names, the team came back with "ancillary" which was not my first choice, but perfectly suitable. In deference to the people doing the work I let their choice stand.
It is an uncomfortable position being a middle tier reviewer of pre- release patch sets when the patch set can still be de-railed by preference nits. A lack of deference makes it a difficult job to convince people "hey my internal review will save you some time upstream", when in practice they can see the opposite is true.
I have to admit that I am biased towards the virtual bus (or virtbus) name as that is what I was using during the original implementation of this code.
That being said, I can also understand Dan's objection to the name.
At first, I didn't object to Parav's suggestion of subdev bus, but the more I think of it, I don't want to have to describe to someone how to use a subdev device's sub device element (yikes, what a mouthful!).
At this point, I just want a name that is easy to understand and doesn't generate a lot of confusion or tongue twisting alliteration.
Can we call it the super_useful bus? (j/k 😊 ).
Some possible names: aggregate (useable across subsystems) gob (bunch group or block - kinda reminds me of git) collect (brings together drivers from different subsystems) flock heap sect (splinter group) not_platform 😉
Any of these interest anybody?
From: Ertman, David M david.m.ertman@intel.com Sent: Monday, October 5, 2020 6:49 AM
-----Original Message----- From: Williams, Dan J dan.j.williams@intel.com Sent: Sunday, October 4, 2020 4:46 PM To: Ertman, David M david.m.ertman@intel.com; gregkh@linuxfoundation.org Cc: pierre-louis.bossart@linux.intel.com; parav@nvidia.com; broonie@kernel.org; jgg@nvidia.com; Sridharan, Ranjani ranjani.sridharan@intel.com; alsa-devel@alsa-project.org; tiwai@suse.de Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
[ Ugh, as other have lameneted, I was not copied on this thread so I could not respond in real time. Let me be another to say, please copy all impacted lists and stakeholders on patches. ]
On Sat, 2020-10-03 at 11:08 +0200, Greg KH wrote: [..]
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
"not hating it", while sometimes is a good thing, for something that I am going to have to tell everyone to go use, I would like to at least "like it". And right now I don't like it...
I think we should go back to "virtual" for now, or, if the people who didn't like it on your "internal" reviews wish to participate here and defend their choice, I would be glad to listen to that reasoning.
I came out strongly against "virtual" because there is nothing virtual about these devices, they are functional partitions of the parent device. Also, /sys/devices/virtual is already the land of unparented / software-defined devices. Having /sys/devices/virtual alongside that is not quite a namespace collision, but it's certainly namespace confusion in my view.
I proposed other names, the team came back with "ancillary" which was not my first choice, but perfectly suitable. In deference to the people doing the work I let their choice stand.
It is an uncomfortable position being a middle tier reviewer of pre- release patch sets when the patch set can still be de-railed by preference nits. A lack of deference makes it a difficult job to convince people "hey my internal review will save you some time upstream", when in practice they can see the opposite is true.
I have to admit that I am biased towards the virtual bus (or virtbus) name as that is what I was using during the original implementation of this code.
That being said, I can also understand Dan's objection to the name.
At first, I didn't object to Parav's suggestion of subdev bus, but the more I think of it, I don't want to have to describe to someone how to use a subdev device's sub device element (yikes, what a mouthful!).
We have fair documentation that describes what an ancillary device is in the Documentation file of this patch. Do you plan to remove that after renaming the bus in spirit of not describing comment above?
As Dan clearly described what devices are ancillary device in previous email -> " they are functional partitions of the parent device ", How about subfunction_bus or partdev_bus?
So I have 3 simpler names that describes it multiple use cases. (a) subdev_bus (b) subfunction_bus (c) partdev_bus
On Sun, Oct 04, 2020 at 11:45:41PM +0000, Williams, Dan J wrote:
[ Ugh, as other have lameneted, I was not copied on this thread so I could not respond in real time. Let me be another to say, please copy all impacted lists and stakeholders on patches. ]
On Sat, 2020-10-03 at 11:08 +0200, Greg KH wrote: [..]
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
"not hating it", while sometimes is a good thing, for something that I am going to have to tell everyone to go use, I would like to at least "like it". And right now I don't like it...
I think we should go back to "virtual" for now, or, if the people who didn't like it on your "internal" reviews wish to participate here and defend their choice, I would be glad to listen to that reasoning.
I came out strongly against "virtual" because there is nothing virtual about these devices, they are functional partitions of the parent device. Also, /sys/devices/virtual is already the land of unparented / software-defined devices. Having /sys/devices/virtual alongside that is not quite a namespace collision, but it's certainly namespace confusion in my view.
I proposed other names, the team came back with "ancillary" which was not my first choice, but perfectly suitable. In deference to the people doing the work I let their choice stand.
It is an uncomfortable position being a middle tier reviewer of pre- release patch sets when the patch set can still be de-railed by preference nits. A lack of deference makes it a difficult job to convince people "hey my internal review will save you some time upstream", when in practice they can see the opposite is true.
I will publically state that without those middle-tier reviews, this would have been a worse submission, which is why I am _REQUIRING_ them from Intel at the moment.
So your review did not happen in vain, and if developers complain about this, send them to me please.
As for the name, why is everyone ignoring the fact that we have had /sys/devices/virtual/ for decades now, with no one being confused about that name usage? I see this as an extension of that existing code pattern, is everyone missing that?
thanks,
greg k-h
On Mon, Oct 5, 2020 at 4:25 AM gregkh@linuxfoundation.org gregkh@linuxfoundation.org wrote:
On Sun, Oct 04, 2020 at 11:45:41PM +0000, Williams, Dan J wrote:
[ Ugh, as other have lameneted, I was not copied on this thread so I could not respond in real time. Let me be another to say, please copy all impacted lists and stakeholders on patches. ]
On Sat, 2020-10-03 at 11:08 +0200, Greg KH wrote: [..]
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
"not hating it", while sometimes is a good thing, for something that I am going to have to tell everyone to go use, I would like to at least "like it". And right now I don't like it...
I think we should go back to "virtual" for now, or, if the people who didn't like it on your "internal" reviews wish to participate here and defend their choice, I would be glad to listen to that reasoning.
I came out strongly against "virtual" because there is nothing virtual about these devices, they are functional partitions of the parent device. Also, /sys/devices/virtual is already the land of unparented / software-defined devices. Having /sys/devices/virtual alongside that is not quite a namespace collision, but it's certainly namespace confusion in my view.
I proposed other names, the team came back with "ancillary" which was not my first choice, but perfectly suitable. In deference to the people doing the work I let their choice stand.
It is an uncomfortable position being a middle tier reviewer of pre- release patch sets when the patch set can still be de-railed by preference nits. A lack of deference makes it a difficult job to convince people "hey my internal review will save you some time upstream", when in practice they can see the opposite is true.
I will publically state that without those middle-tier reviews, this would have been a worse submission, which is why I am _REQUIRING_ them from Intel at the moment.
So your review did not happen in vain, and if developers complain about this, send them to me please.
As for the name, why is everyone ignoring the fact that we have had /sys/devices/virtual/ for decades now, with no one being confused about that name usage? I see this as an extension of that existing code pattern, is everyone missing that?
Oh, I was specifically reacting to /sys/devices/virtual being a place where un-parented devices land. Things like raid composite devices and other devices that just do not fit cleanly in the parent device hierarchy, each of them has independent lifetime rules, some do not attach to drivers they just exist in an "unbound" state when active... it's an anything goes catch all space. This bus is the opposite, all devices have a maximum lifetime tied to their parent device bind lifetime, and they are not active without drivers. Placing them under /sys/bus/virtual/devices confuses what the "virtual" term means to sysfs.
"ancillary" devices and drivers has meaning to me, in a way that "virtual" devices and drivers does not. If "ancillary" does not hit the right tone what about "aux" for "auxiliary" (/sys/bus/aux/devices)?
On Tue, Oct 06, 2020 at 03:40:58PM -0700, Dan Williams wrote:
On Mon, Oct 5, 2020 at 4:25 AM gregkh@linuxfoundation.org gregkh@linuxfoundation.org wrote:
On Sun, Oct 04, 2020 at 11:45:41PM +0000, Williams, Dan J wrote:
[ Ugh, as other have lameneted, I was not copied on this thread so I could not respond in real time. Let me be another to say, please copy all impacted lists and stakeholders on patches. ]
On Sat, 2020-10-03 at 11:08 +0200, Greg KH wrote: [..]
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
"not hating it", while sometimes is a good thing, for something that I am going to have to tell everyone to go use, I would like to at least "like it". And right now I don't like it...
I think we should go back to "virtual" for now, or, if the people who didn't like it on your "internal" reviews wish to participate here and defend their choice, I would be glad to listen to that reasoning.
I came out strongly against "virtual" because there is nothing virtual about these devices, they are functional partitions of the parent device. Also, /sys/devices/virtual is already the land of unparented / software-defined devices. Having /sys/devices/virtual alongside that is not quite a namespace collision, but it's certainly namespace confusion in my view.
I proposed other names, the team came back with "ancillary" which was not my first choice, but perfectly suitable. In deference to the people doing the work I let their choice stand.
It is an uncomfortable position being a middle tier reviewer of pre- release patch sets when the patch set can still be de-railed by preference nits. A lack of deference makes it a difficult job to convince people "hey my internal review will save you some time upstream", when in practice they can see the opposite is true.
I will publically state that without those middle-tier reviews, this would have been a worse submission, which is why I am _REQUIRING_ them from Intel at the moment.
So your review did not happen in vain, and if developers complain about this, send them to me please.
As for the name, why is everyone ignoring the fact that we have had /sys/devices/virtual/ for decades now, with no one being confused about that name usage? I see this as an extension of that existing code pattern, is everyone missing that?
Oh, I was specifically reacting to /sys/devices/virtual being a place where un-parented devices land. Things like raid composite devices and other devices that just do not fit cleanly in the parent device hierarchy, each of them has independent lifetime rules, some do not attach to drivers they just exist in an "unbound" state when active... it's an anything goes catch all space. This bus is the opposite, all devices have a maximum lifetime tied to their parent device bind lifetime, and they are not active without drivers. Placing them under /sys/bus/virtual/devices confuses what the "virtual" term means to sysfs.
"virtual" here means "not real" :)
"ancillary" devices and drivers has meaning to me, in a way that "virtual" devices and drivers does not. If "ancillary" does not hit the right tone what about "aux" for "auxiliary" (/sys/bus/aux/devices)?
"aux" is nice, I'll think about that. simple is good, and naming is hard...
thanks,
greg k-h
On Wed, Oct 7, 2020 at 2:14 AM gregkh@linuxfoundation.org gregkh@linuxfoundation.org wrote:
On Tue, Oct 06, 2020 at 03:40:58PM -0700, Dan Williams wrote:
On Mon, Oct 5, 2020 at 4:25 AM gregkh@linuxfoundation.org gregkh@linuxfoundation.org wrote:
On Sun, Oct 04, 2020 at 11:45:41PM +0000, Williams, Dan J wrote:
[ Ugh, as other have lameneted, I was not copied on this thread so I could not respond in real time. Let me be another to say, please copy all impacted lists and stakeholders on patches. ]
On Sat, 2020-10-03 at 11:08 +0200, Greg KH wrote: [..]
Several names were suggested (like peer bus, which was shot down because in parts on the English speaking world the peerage means nobility), finally "ancillary bus" was arrived at by consensus of not hating it.
"not hating it", while sometimes is a good thing, for something that I am going to have to tell everyone to go use, I would like to at least "like it". And right now I don't like it...
I think we should go back to "virtual" for now, or, if the people who didn't like it on your "internal" reviews wish to participate here and defend their choice, I would be glad to listen to that reasoning.
I came out strongly against "virtual" because there is nothing virtual about these devices, they are functional partitions of the parent device. Also, /sys/devices/virtual is already the land of unparented / software-defined devices. Having /sys/devices/virtual alongside that is not quite a namespace collision, but it's certainly namespace confusion in my view.
I proposed other names, the team came back with "ancillary" which was not my first choice, but perfectly suitable. In deference to the people doing the work I let their choice stand.
It is an uncomfortable position being a middle tier reviewer of pre- release patch sets when the patch set can still be de-railed by preference nits. A lack of deference makes it a difficult job to convince people "hey my internal review will save you some time upstream", when in practice they can see the opposite is true.
I will publically state that without those middle-tier reviews, this would have been a worse submission, which is why I am _REQUIRING_ them from Intel at the moment.
So your review did not happen in vain, and if developers complain about this, send them to me please.
As for the name, why is everyone ignoring the fact that we have had /sys/devices/virtual/ for decades now, with no one being confused about that name usage? I see this as an extension of that existing code pattern, is everyone missing that?
Oh, I was specifically reacting to /sys/devices/virtual being a place where un-parented devices land. Things like raid composite devices and other devices that just do not fit cleanly in the parent device hierarchy, each of them has independent lifetime rules, some do not attach to drivers they just exist in an "unbound" state when active... it's an anything goes catch all space. This bus is the opposite, all devices have a maximum lifetime tied to their parent device bind lifetime, and they are not active without drivers. Placing them under /sys/bus/virtual/devices confuses what the "virtual" term means to sysfs.
"virtual" here means "not real" :)
Which of these aux device use cases is not a real device? One of my planned usages for this facility is for the NPEM (Native PCIE Enclosure Management) mechanism that can appear on any PCIE bridge/endpoint. While it is true that the NPEM register set does not get its own PCI-b:d:f address on the host bus, it is still discoverable by a standard enumeration scheme. It is real auxiliary device functionality that can appear on any PCIE device where the kernel can now have one common NPEM driver for all instances in the topology.
"ancillary" devices and drivers has meaning to me, in a way that "virtual" devices and drivers does not. If "ancillary" does not hit the right tone what about "aux" for "auxiliary" (/sys/bus/aux/devices)?
"aux" is nice, I'll think about that. simple is good, and naming is hard...
I have a soft spot for functional three letter abbreviations. Certainly "aux" is clearer than "ancil", and less overloaded than "virt".
On Wed, Oct 07, 2020 at 09:19:32AM -0700, Dan Williams wrote:
On Wed, Oct 7, 2020 at 2:14 AM gregkh@linuxfoundation.org
"virtual" here means "not real" :)
Which of these aux device use cases is not a real device? One of my planned usages for this facility is for the NPEM (Native PCIE Enclosure Management) mechanism that can appear on any PCIE bridge/endpoint. While it is true that the NPEM register set does not get its own PCI-b:d:f address on the host bus, it is still discoverable by a standard enumeration scheme. It is real auxiliary device functionality that can appear on any PCIE device where the kernel can now have one common NPEM driver for all instances in the topology.
Some if not all of the SOF cases are entirely software defined by the firmware downloaded to the audio DSPs.
On Wed, Oct 7, 2020 at 9:23 AM Mark Brown broonie@kernel.org wrote:
On Wed, Oct 07, 2020 at 09:19:32AM -0700, Dan Williams wrote:
On Wed, Oct 7, 2020 at 2:14 AM gregkh@linuxfoundation.org
"virtual" here means "not real" :)
Which of these aux device use cases is not a real device? One of my planned usages for this facility is for the NPEM (Native PCIE Enclosure Management) mechanism that can appear on any PCIE bridge/endpoint. While it is true that the NPEM register set does not get its own PCI-b:d:f address on the host bus, it is still discoverable by a standard enumeration scheme. It is real auxiliary device functionality that can appear on any PCIE device where the kernel can now have one common NPEM driver for all instances in the topology.
Some if not all of the SOF cases are entirely software defined by the firmware downloaded to the audio DSPs.
"Software-defined" in the kernel context to me means software Linux kernel developers have control over, so device-mapper devices, other things that show up under /sys/devices/virtual, or /sys/devices/system/memory/. Firmware changing device functionality is more like just-in-time hardware than software-defined. In other words kernel software is not involved in constructing the device functionality.
"virtual" here means "not real" :)
Which of these aux device use cases is not a real device? One of my planned usages for this facility is for the NPEM (Native PCIE Enclosure Management) mechanism that can appear on any PCIE bridge/endpoint. While it is true that the NPEM register set does not get its own PCI-b:d:f address on the host bus, it is still discoverable by a standard enumeration scheme. It is real auxiliary device functionality that can appear on any PCIE device where the kernel can now have one common NPEM driver for all instances in the topology.
Some if not all of the SOF cases are entirely software defined by the firmware downloaded to the audio DSPs.
Correct for DSP processing/debug stuff. In some cases though, the firmware deals with different IOs (HDMI, I2C) and having multiple 'aux' devices helps expose unrelated physical functions in a more modular way.
The non-SOF audio case I can think of is SoundWire. We want to expose SoundWire links as separate devices even though they are not exposed in the platform firmware or PCI configs (decision driven by Windows). We currently use platform devices for this, but we'd like to transition to this 'aux' bus
Hi Pierre, Mark, Dan,
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Sent: Wednesday, October 7, 2020 10:12 PM
"virtual" here means "not real" :)
Which of these aux device use cases is not a real device? One of my planned usages for this facility is for the NPEM (Native PCIE Enclosure Management) mechanism that can appear on any PCIE bridge/endpoint. While it is true that the NPEM register set does not get its own PCI-b:d:f address on the host bus, it is still discoverable by a standard enumeration scheme. It is real auxiliary device functionality that can appear on any PCIE device where the kernel can now have one common NPEM driver for all instances in the topology.
Some if not all of the SOF cases are entirely software defined by the firmware downloaded to the audio DSPs.
Correct for DSP processing/debug stuff. In some cases though, the firmware deals with different IOs (HDMI, I2C) and having multiple 'aux' devices helps expose unrelated physical functions in a more modular way.
The non-SOF audio case I can think of is SoundWire. We want to expose SoundWire links as separate devices even though they are not exposed in the platform firmware or PCI configs (decision driven by Windows). We currently use platform devices for this, but we'd like to transition to this 'aux' bus
There is more updated version of the patch [1] from Dave which is covering multiple mailing list who are also going to consume this bus. This includes (a) mlx5 subdevices for netdev, rdma and more carved from a pci device. (b) mlx5 matching service to load multiple drivers on for a given PCI PF/VF/subfunction. (similar use case as irdma)
Greg also mentioned that he likes to see other mailing list CCed, which Dave already did in PATCHv2 at [1]. So lets please discuss in thread [1]?
[1] https://lore.kernel.org/linux-rdma/20201005182446.977325-1-david.m.ertman@in...
On 2020-10-01 12:50 AM, 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@int...
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.
Given the description and number of possible users, one could safely say: usage is assured. So, why not submit this bus as a standalone patch? Isn't it better to first have a stable, complete version present in Linus' tree and only then append the usage?
All other patches target ASoC SOF solution directly while as stated in the commit's message, this isn't SOF specific - see the delta provided at the bottom of cover-letter and a wide range of SOF files it touches.
Czarek
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
On Thu, Oct 01, 2020 at 10:05:13AM +0000, Rojewski, Cezary wrote:
On 2020-10-01 12:50 AM, 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@int...
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.
Given the description and number of possible users, one could safely say: usage is assured. So, why not submit this bus as a standalone patch? Isn't it better to first have a stable, complete version present in Linus' tree and only then append the usage?
Because I want to see patches that actually _use_ the new code. So far the previous versions of this implementation have not shown how all of the code will be used, making it impossible to review to see if it fits the needs of people.
We don't add infrastructure without users. And the normal rule of thumb of "if we have 3 users, then it is a semi-sane api" really applies here.
thanks,
greg k-h
On Thu, Oct 01, 2020 at 12:59:25PM +0200, gregkh@linuxfoundation.org wrote:
We don't add infrastructure without users. And the normal rule of thumb of "if we have 3 users, then it is a semi-sane api" really applies here.
Based on recent discussions I'm expecting: - Intel SOF - New Intel RDMA driver - mlx5 RDMA driver conversion - mlx4 RDMA driver conversion - mlx5 subdevice feature for netdev - Intel IDXD vfio-mdev - Habana Labs Gaudi netdev driver
Will use this in the short term.
I would like, but don't expect too see, the other RDMA RoCE drivers converted - cxgb3/4, i40iw, hns, ocrdma, and qedr. It solves an annoying module loading problem we have.
We've seen the New Intel RDMA driver many months ago, if patch 1 is going to stay the same we should post some of the mlx items next week.
It is hard to co-ordinate all of this already, having some general agreement that there is nothing fundamentally objectionable about ancillary bus will help alot.
Jason
On Thu, Oct 01, 2020 at 09:49:00AM -0300, Jason Gunthorpe wrote:
On Thu, Oct 01, 2020 at 12:59:25PM +0200, gregkh@linuxfoundation.org wrote:
We don't add infrastructure without users. And the normal rule of thumb of "if we have 3 users, then it is a semi-sane api" really applies here.
Based on recent discussions I'm expecting:
- Intel SOF
- New Intel RDMA driver
- mlx5 RDMA driver conversion
- mlx4 RDMA driver conversion
- mlx5 subdevice feature for netdev
- Intel IDXD vfio-mdev
- Habana Labs Gaudi netdev driver
Will use this in the short term.
I would like, but don't expect too see, the other RDMA RoCE drivers converted - cxgb3/4, i40iw, hns, ocrdma, and qedr. It solves an annoying module loading problem we have.
We've seen the New Intel RDMA driver many months ago, if patch 1 is going to stay the same we should post some of the mlx items next week.
It is hard to co-ordinate all of this already, having some general agreement that there is nothing fundamentally objectionable about ancillary bus will help alot.
I agree, but with just one user (in a very odd way I do have to say, more on that on the review of that specific patch), it's hard to judge if this is useful are not, right?
So, what happened to at least the Intel SOF driver usage? That was the original user of this bus (before it was renamed), surely that patchset should be floating around somewhere in Intel, right?
thanks,
greg k-h
On Thu, Oct 01, 2020 at 02:55:26PM +0200, gregkh@linuxfoundation.org wrote:
I agree, but with just one user (in a very odd way I do have to say, more on that on the review of that specific patch), it's hard to judge if this is useful are not, right?
I agree with you completely, this SOF usage is quite weird and not what I think is representative. I never imagined this stuff would be used inside a single driver in a single subsystem. It was imagined for cross-subsystem sharing.
So, what happened to at least the Intel SOF driver usage? That was the original user of this bus (before it was renamed), surely that patchset should be floating around somewhere in Intel, right?
The first user was irdma (the New Intel RDMA driver):
https://lore.kernel.org/linux-rdma/20200520070415.3392210-1-jeffrey.t.kirshe...
(look at patch 1, search for virtbus)
I kicked it off when I said I was sick of RDMA RoCE drivers re-implementing the driver core register/unregister and module management to share a PCI device between netdev and RDMA.
This has been going on for almost two years now. I did not think it would be so hard.
Jason
On Thu, Oct 01, 2020 at 10:26:59AM -0300, Jason Gunthorpe wrote:
On Thu, Oct 01, 2020 at 02:55:26PM +0200, gregkh@linuxfoundation.org wrote:
So, what happened to at least the Intel SOF driver usage? That was the original user of this bus (before it was renamed), surely that patchset should be floating around somewhere in Intel, right?
The first user was irdma (the New Intel RDMA driver):
https://lore.kernel.org/linux-rdma/20200520070415.3392210-1-jeffrey.t.kirshe...
(look at patch 1, search for virtbus)
My apologies, you are correct, it's been so long "in flight" that I can't remember...
I kicked it off when I said I was sick of RDMA RoCE drivers re-implementing the driver core register/unregister and module management to share a PCI device between netdev and RDMA.
This has been going on for almost two years now. I did not think it would be so hard.
It really isn't, I have no idea why it has taken so long.
For a while I thought it was people doing the traditional, "if I submit something so bad, it will make the maintainer take pity and just do it correctly themselves" method of kernel development, and if so, it failed horribly. Now I just have no idea why it has taken so long, sad...
greg k-h
From: gregkh@linuxfoundation.org gregkh@linuxfoundation.org Sent: Thursday, October 1, 2020 6:25 PM
On Thu, Oct 01, 2020 at 09:49:00AM -0300, Jason Gunthorpe wrote:
On Thu, Oct 01, 2020 at 12:59:25PM +0200, gregkh@linuxfoundation.org
wrote:
We don't add infrastructure without users. And the normal rule of thumb of "if we have 3 users, then it is a semi-sane api" really applies
here.
Based on recent discussions I'm expecting:
- Intel SOF
- New Intel RDMA driver
- mlx5 RDMA driver conversion
- mlx4 RDMA driver conversion
- mlx5 subdevice feature for netdev
- Intel IDXD vfio-mdev
- Habana Labs Gaudi netdev driver
Will use this in the short term.
I would like, but don't expect too see, the other RDMA RoCE drivers converted - cxgb3/4, i40iw, hns, ocrdma, and qedr. It solves an annoying module loading problem we have.
We've seen the New Intel RDMA driver many months ago, if patch 1 is going to stay the same we should post some of the mlx items next week.
It is hard to co-ordinate all of this already, having some general agreement that there is nothing fundamentally objectionable about ancillary bus will help alot.
I agree, but with just one user (in a very odd way I do have to say, more on that on the review of that specific patch), it's hard to judge if this is useful are not, right?
As Jason mentioned above, mlx5 subdevice feature, I like to provide more context before posting the patches.
I have rebased and tested mlx5 subfunction devices for netdev to use ancillary device as per the RFC posted at [1]. These subdevices are created dynamically on user request. Typically then are in range of hundreds. Please grep for virtbus to see its intended use in [1].
To refresh the memory, before working on the RFC [1], mlx5 subfunction use is also discussed further with Greg at [2]. Recently I further discussed ancillary bus (virtbus) intended use for mlx5 subfunction with netdev community at [3] and summarized in [4] , jump to last slide 22.
mlx5 series is bit long and waiting for mainly ancillary bus to be available apart from some internal reviews to finish.
[1] https://lore.kernel.org/netdev/20200519092258.GF4655@nanopsycho/ [2] https://patchwork.kernel.org/patch/11280547/#23056985 [3] https://netdevconf.info/0x14/pub/papers/45/0x14-paper45-talk-paper.pdf [4] https://netdevconf.info/0x14/pub/slides/45/sf_mgmt_using_devlink_netdevconf_...
So, what happened to at least the Intel SOF driver usage? That was the original user of this bus (before it was renamed), surely that patchset should be floating around somewhere in Intel, right?
thanks,
greg k-h
On Wed, Sep 30, 2020 at 03:50:45PM -0700, Dave Ertman wrote:
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
I have to say that I find the motivation behind this bus to be a bit confusing. In code terms it's essentially a stripped back copy of the platform bus and we're basically assigning devices between the two based on if they end up having a physical resource passed through to them. That seems to result in some duplication, has some potential for devices to need to churn between the two buses and for duplication in parent devices if they need to create both platform and auxiliary devices. What exactly is the problem we're trying to solve here beyond the labelling one? I can see that it's a bit messy but this whole area is a bit messy and I'm not clear that we're not just pushing the mess around.
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.
MFD has no reliance on devices being DT enumerated, it works on systems that don't have DT and in many cases it's not clear that the split you'd want for the way Linux describes devices is a sensible one for other operating systems so we don't want to put it into DT. Forcing things to be DT enumerated would just create needless ABIs.
On Thu, Oct 01, 2020 at 01:50:38PM +0100, Mark Brown wrote:
On Wed, Sep 30, 2020 at 03:50:45PM -0700, Dave Ertman wrote:
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
I have to say that I find the motivation behind this bus to be a bit confusing. In code terms it's essentially a stripped back copy of the platform bus and we're basically assigning devices between the two based on if they end up having a physical resource passed through to them. That seems to result in some duplication, has some potential for devices to need to churn between the two buses and for duplication in parent devices if they need to create both platform and auxiliary devices. What exactly is the problem we're trying to solve here beyond the labelling one? I can see that it's a bit messy but this whole area is a bit messy and I'm not clear that we're not just pushing the mess around.
This series doesn't really show how this is ment to be used, from what I can tell.
The goal is to NOT need a platform device/bus as that's an overloaded/abused subsystem, and to just use a much lighter-weight subsystem that allows one "device" (PCI/USB/whatever) to have multiple child devices that then are bound to different subsystems (networking, tty, input, etc.) Yes, there will be some core "sharing" needed, but that's up to the driver that implements this, not the generic code.
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.
MFD has no reliance on devices being DT enumerated, it works on systems that don't have DT and in many cases it's not clear that the split you'd want for the way Linux describes devices is a sensible one for other operating systems so we don't want to put it into DT. Forcing things to be DT enumerated would just create needless ABIs.
This new bus doesn't need DT at all. Or at least it better not...
But again, this series doesn't really feel like it is showing what this is really for to me as there is just one "child device" that is being created, just to handle debugfs files, which aren't even part of the driver model.
Again, I could have this totally wrong, if so, someone needs to point out my errors in reviewing this.
thanks,
greg k-h
On Thu, Oct 01, 2020 at 03:12:52PM +0200, Greg KH wrote:
On Thu, Oct 01, 2020 at 01:50:38PM +0100, Mark Brown wrote:
That seems to result in some duplication, has some potential for devices to need to churn between the two buses and for duplication in parent devices if they need to create both platform and auxiliary devices. What exactly is the problem we're trying to solve here beyond the labelling one? I can see that it's a bit messy but this whole area is a bit messy and I'm not clear that we're not just pushing the mess around.
This series doesn't really show how this is ment to be used, from what I can tell.
The goal is to NOT need a platform device/bus as that's an overloaded/abused subsystem, and to just use a much lighter-weight subsystem that allows one "device" (PCI/USB/whatever) to have multiple child devices that then are bound to different subsystems (networking, tty, input, etc.) Yes, there will be some core "sharing" needed, but that's up to the driver that implements this, not the generic code.
Right, so my concern is that as soon as we decide we want to pass some resources or platform data through to one of the subdevices it needs to move over into being a platform device and vice versa. That feels like something that's going to add to the mess for some of the uses.
On Thu, Oct 01, 2020 at 03:40:19PM +0100, Mark Brown wrote:
On Thu, Oct 01, 2020 at 03:12:52PM +0200, Greg KH wrote:
On Thu, Oct 01, 2020 at 01:50:38PM +0100, Mark Brown wrote:
That seems to result in some duplication, has some potential for devices to need to churn between the two buses and for duplication in parent devices if they need to create both platform and auxiliary devices. What exactly is the problem we're trying to solve here beyond the labelling one? I can see that it's a bit messy but this whole area is a bit messy and I'm not clear that we're not just pushing the mess around.
This series doesn't really show how this is ment to be used, from what I can tell.
The goal is to NOT need a platform device/bus as that's an overloaded/abused subsystem, and to just use a much lighter-weight subsystem that allows one "device" (PCI/USB/whatever) to have multiple child devices that then are bound to different subsystems (networking, tty, input, etc.) Yes, there will be some core "sharing" needed, but that's up to the driver that implements this, not the generic code.
Right, so my concern is that as soon as we decide we want to pass some resources or platform data through to one of the subdevices it needs to move over into being a platform device and vice versa. That feels like something that's going to add to the mess for some of the uses.
There shouldn't be a need for resources or platform data to be passed that way as they are all "owned" by the parent device that creates these.
I don't want to see platform devices become children of real devices (like PCI and USB and others), which is the goal here. platform devices are overloaded and abused enough as it is, let's not make it worse.
thanks,
greg k-h
On Thu, Oct 01, 2020 at 05:32:07PM +0200, Greg KH wrote:
On Thu, Oct 01, 2020 at 03:40:19PM +0100, Mark Brown wrote:
Right, so my concern is that as soon as we decide we want to pass some resources or platform data through to one of the subdevices it needs to move over into being a platform device and vice versa. That feels like something that's going to add to the mess for some of the uses.
There shouldn't be a need for resources or platform data to be passed that way as they are all "owned" by the parent device that creates these.
I don't want to see platform devices become children of real devices (like PCI and USB and others), which is the goal here. platform devices are overloaded and abused enough as it is, let's not make it worse.
How does this interact with the situation where someone makes a PCI device that's basically a bunch of IPs glued together in a PCI memory region (or similarly for other buses)? The IPs all have distinct memory regions and other resources like interrupt lines which makes them unsuitable for auxilliary devices as proposed, especially in cases where there's more than one copy of the IP instantiated. There's a bunch of PCI MFDs in tree already of admittedly varying degrees of taste, and MFDs on other buses also use the resource passing stuff.
On Thu, Oct 01, 2020 at 05:03:16PM +0100, Mark Brown wrote:
On Thu, Oct 01, 2020 at 05:32:07PM +0200, Greg KH wrote:
On Thu, Oct 01, 2020 at 03:40:19PM +0100, Mark Brown wrote:
Right, so my concern is that as soon as we decide we want to pass some resources or platform data through to one of the subdevices it needs to move over into being a platform device and vice versa. That feels like something that's going to add to the mess for some of the uses.
There shouldn't be a need for resources or platform data to be passed that way as they are all "owned" by the parent device that creates these.
I don't want to see platform devices become children of real devices (like PCI and USB and others), which is the goal here. platform devices are overloaded and abused enough as it is, let's not make it worse.
How does this interact with the situation where someone makes a PCI device that's basically a bunch of IPs glued together in a PCI memory region (or similarly for other buses)? The IPs all have distinct memory regions and other resources like interrupt lines which makes them unsuitable for auxilliary devices as proposed, especially in cases where there's more than one copy of the IP instantiated. There's a bunch of PCI MFDs in tree already of admittedly varying degrees of taste, and MFDs on other buses also use the resource passing stuff.
I would like to move those PCI MFDs away from that, and into this bus instead.
If there needs to have a way to pass/share resources, great, let's add it, there's no objection from me.
thanks,
greg k-h
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Thursday, October 1, 2020 11:16 AM To: Mark Brown broonie@kernel.org Cc: Ertman, David M david.m.ertman@intel.com; alsa-devel@alsa- project.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com; Sridharan, Ranjani ranjani.sridharan@intel.com; jgg@nvidia.com; parav@nvidia.com Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Thu, Oct 01, 2020 at 05:03:16PM +0100, Mark Brown wrote:
On Thu, Oct 01, 2020 at 05:32:07PM +0200, Greg KH wrote:
On Thu, Oct 01, 2020 at 03:40:19PM +0100, Mark Brown wrote:
Right, so my concern is that as soon as we decide we want to pass some resources or platform data through to one of the subdevices it needs to move over into being a platform device and vice versa. That feels like something that's going to add to the mess for some of the uses.
There shouldn't be a need for resources or platform data to be passed that way as they are all "owned" by the parent device that creates these.
I don't want to see platform devices become children of real devices (like PCI and USB and others), which is the goal here. platform devices are overloaded and abused enough as it is, let's not make it worse.
How does this interact with the situation where someone makes a PCI device that's basically a bunch of IPs glued together in a PCI memory region (or similarly for other buses)? The IPs all have distinct memory regions and other resources like interrupt lines which makes them unsuitable for auxilliary devices as proposed, especially in cases where there's more than one copy of the IP instantiated. There's a bunch of PCI MFDs in tree already of admittedly varying degrees of taste, and MFDs on other buses also use the resource passing stuff.
I would like to move those PCI MFDs away from that, and into this bus instead.
If there needs to have a way to pass/share resources, great, let's add it, there's no objection from me.
thanks,
greg k-h
The sharing of information is done by having the parent driver declare the ancillary_device as an element in a parent_struct that also contains a pointer to the shared information. This way, when the ancillary_driver is probed, and a pointer to ancillary_device is passed, it can perform a container_of on the ancillary_device and gain access to the shared data.
This keeps all requirements out of the ancillary bus code and it can be as flexible as the implementer wants it to be.
-DaveE
On Thu, Oct 01, 2020 at 06:29:58PM +0000, Ertman, David M wrote:
I would like to move those PCI MFDs away from that, and into this bus instead.
If there needs to have a way to pass/share resources, great, let's add it, there's no objection from me.
OK, if we can add resource passing then no huge problem from my end since there'd be no case where you couldn't use an ancillairy device. It's still a bit confusing but there's no cases where we're supposed to use an ancilliary device but it doesn't have required features.
The sharing of information is done by having the parent driver declare the ancillary_device as an element in a parent_struct that also contains a pointer to the shared information. This way, when the ancillary_driver is probed, and a pointer to ancillary_device is passed, it can perform a container_of on the ancillary_device and gain access to the shared data.
This keeps all requirements out of the ancillary bus code and it can be as flexible as the implementer wants it to be.
This means that every device has to reinvent the wheel for common resources like interrupts, including things like looking them up by name and so on. That doesn't seem ideal.
-----Original Message----- From: Mark Brown broonie@kernel.org Sent: Thursday, October 1, 2020 12:39 PM To: Ertman, David M david.m.ertman@intel.com Cc: Greg KH gregkh@linuxfoundation.org; alsa-devel@alsa-project.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com; Sridharan, Ranjani ranjani.sridharan@intel.com; jgg@nvidia.com; parav@nvidia.com Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Thu, Oct 01, 2020 at 06:29:58PM +0000, Ertman, David M wrote:
I would like to move those PCI MFDs away from that, and into this bus instead.
If there needs to have a way to pass/share resources, great, let's add it, there's no objection from me.
OK, if we can add resource passing then no huge problem from my end since there'd be no case where you couldn't use an ancillairy device. It's still a bit confusing but there's no cases where we're supposed to use an ancilliary device but it doesn't have required features.
The sharing of information is done by having the parent driver declare the ancillary_device as an element in a parent_struct that also contains a pointer to the shared information. This way, when the ancillary_driver is probed, and a pointer to ancillary_device is passed, it can perform a container_of on the ancillary_device and gain access to the shared data.
This keeps all requirements out of the ancillary bus code and it can be as flexible as the implementer wants it to be.
This means that every device has to reinvent the wheel for common resources like interrupts, including things like looking them up by name and so on. That doesn't seem ideal.
It's a shared header file between the device and driver implementer. If they want to share a "struct irq_data *irq" as an element in the shared data, that is fine, but not everyone will need to include that. This is why we left what is shared up to the implementer, so that we don't impose a debt on some other implementation that doesn't need it.
I suppose it is not the end of the world adding elements to the definition of struct ancillary_device, but what would be a "universal" element to add? Where do you draw the line on what you allow into the bus internals? The overriding goal was to make a subsystem agnostic bus that doesn't impose implementation specific details from any single subsystem.
-DaveE
-DaveE
On Thu, Oct 01, 2020 at 07:54:26PM +0000, Ertman, David M wrote:
This means that every device has to reinvent the wheel for common resources like interrupts, including things like looking them up by name and so on. That doesn't seem ideal.
It's a shared header file between the device and driver implementer. If they want to share a "struct irq_data *irq" as an element in the shared data, that is fine, but not everyone will need to include that. This is why we left what is shared up to the implementer, so that we don't impose a debt on some other implementation that doesn't need it.
Realistically I think you're saying this because this has taken so long already and you're being asked for more changes rather than because it's a good design decision. That is entirely understandable and reasonable but I'm not sure it's the best decision when we have so many common patterns between devices, one of the things that the current situtation handles well is allowing lots of common stuff to just be data definitions rather than code - there used to be a lot more open coding of resource sharing.
One thing we should agree here is that we don't actually have to implement everything right now, we just need to understand what the design and direction of travel are. That means that at this point it's probably just a fairly quick documentation update rather than substantial code changes.
I suppose it is not the end of the world adding elements to the definition of struct ancillary_device, but what would be a "universal" element to add? Where do you draw the line on what you allow into the bus internals? The overriding goal was to make a subsystem agnostic bus that doesn't impose implementation specific details from any single subsystem.
I think that this needs to grow everything that platform and MFD have so that we can avoid using platform devices to represent things that are not in the very strictest sense platform devices (which I interpret to be memory mapped devices that can't be enumerated in some fashion). My understanding here is that the goal is an abstraction cleanup, it's possible I've misunderstood though.
On Thu, Oct 01, 2020 at 09:17:18PM +0100, Mark Brown wrote:
I suppose it is not the end of the world adding elements to the definition of struct ancillary_device, but what would be a "universal" element to add? Where do you draw the line on what you allow into the bus internals? The overriding goal was to make a subsystem agnostic bus that doesn't impose implementation specific details from any single subsystem.
I think that this needs to grow everything that platform and MFD have so that we can avoid using platform devices to represent things that are not in the very strictest sense platform devices (which I interpret to be memory mapped devices that can't be enumerated in some fashion). My understanding here is that the goal is an abstraction cleanup, it's possible I've misunderstood though.
Instead of making ancillary bus giant, it might be interesting to use a library technique to avoid the code duplication you are talking about, eg
struct my_ancillary_data { struct ancillary_device adev; struct ancillary_resource resources; }
Then each user can access the facets they need.
Not sure what else is in platform_device or mfd_cell that is relevant: - There is no DMA, these devices are not for DMA. The physical device pointer should be used with the DMA API. - resources as above - id/override/archdata not relavent
From mfd_cell
- enable/disable suspend/resume, these look like they duplicate the driver core stuff? - pdata/of_compatible/acpi_match - not applicable - properties - use struct members and container of - resources as above - regulators - another struct member? Only two users.
Jason
On Thu, Oct 01, 2020 at 09:47:40PM -0300, Jason Gunthorpe wrote:
On Thu, Oct 01, 2020 at 09:17:18PM +0100, Mark Brown wrote:
Instead of making ancillary bus giant, it might be interesting to use a library technique to avoid the code duplication you are talking about, eg
struct my_ancillary_data { struct ancillary_device adev; struct ancillary_resource resources; }
Then each user can access the facets they need.
The way most of this stuff works already it's not a table in the device itself, it's an array of resources with type information. Your second struct there is presumably just going to be the pointer and size information which you *could* split out but I'm really not sure what you're buying there.
The interesting bit isn't really the data in the devices themselves, it's the code that allows devices to just supply a data table and have things mapped through to their child devices. That isn't particularly complex either, but it's still worth avoiding having lots of copies of it. *None* of this bus stuff is hugely complex but we still don't want each device with children having to open code it all when they could just do something data driven.
-----Original Message----- From: Mark Brown broonie@kernel.org Sent: Friday, October 2, 2020 4:20 AM To: Jason Gunthorpe jgg@nvidia.com Cc: Ertman, David M david.m.ertman@intel.com; Greg KH gregkh@linuxfoundation.org; alsa-devel@alsa-project.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com; Sridharan, Ranjani ranjani.sridharan@intel.com; parav@nvidia.com Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Thu, Oct 01, 2020 at 09:47:40PM -0300, Jason Gunthorpe wrote:
On Thu, Oct 01, 2020 at 09:17:18PM +0100, Mark Brown wrote:
Instead of making ancillary bus giant, it might be interesting to use a library technique to avoid the code duplication you are talking about, eg
struct my_ancillary_data { struct ancillary_device adev; struct ancillary_resource resources; }
Then each user can access the facets they need.
The way most of this stuff works already it's not a table in the device itself, it's an array of resources with type information. Your second struct there is presumably just going to be the pointer and size information which you *could* split out but I'm really not sure what you're buying there.
The interesting bit isn't really the data in the devices themselves, it's the code that allows devices to just supply a data table and have things mapped through to their child devices. That isn't particularly complex either, but it's still worth avoiding having lots of copies of it. *None* of this bus stuff is hugely complex but we still don't want each device with children having to open code it all when they could just do something data driven.
Would you recommend adding two elements to the ancillary_device like: void *ancillary_data; u32 ancildata_size; like the platform_device uses?
-DaveE
On Fri, Oct 02, 2020 at 05:23:53PM +0000, Ertman, David M wrote:
From: Mark Brown broonie@kernel.org Sent: Friday, October 2, 2020 4:20 AM To: Jason Gunthorpe jgg@nvidia.com Cc: Ertman, David M david.m.ertman@intel.com; Greg KH gregkh@linuxfoundation.org; alsa-devel@alsa-project.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com; Sridharan, Ranjani ranjani.sridharan@intel.com; parav@nvidia.com Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Thu, Oct 01, 2020 at 09:47:40PM -0300, Jason Gunthorpe wrote:
On Thu, Oct 01, 2020 at 09:17:18PM +0100, Mark Brown wrote:
Instead of making ancillary bus giant, it might be interesting to use a library technique to avoid the code duplication you are talking about, eg
struct my_ancillary_data { struct ancillary_device adev; struct ancillary_resource resources; }
Then each user can access the facets they need.
The way most of this stuff works already it's not a table in the device itself, it's an array of resources with type information. Your second struct there is presumably just going to be the pointer and size information which you *could* split out but I'm really not sure what you're buying there.
The interesting bit isn't really the data in the devices themselves, it's the code that allows devices to just supply a data table and have things mapped through to their child devices. That isn't particularly complex either, but it's still worth avoiding having lots of copies of it. *None* of this bus stuff is hugely complex but we still don't want each device with children having to open code it all when they could just do something data driven.
Would you recommend adding two elements to the ancillary_device like: void *ancillary_data; u32 ancildata_size; like the platform_device uses?
That doesn't seem useful here, the intent is to use container_of, if the creator wants to pass private data then it should be structured into the containing struct.
platform_devices/etc don't use container_of so have this side band way to pass more data.
Jason
On Fri, Oct 02, 2020 at 02:25:58PM -0300, Jason Gunthorpe wrote:
On Fri, Oct 02, 2020 at 05:23:53PM +0000, Ertman, David M wrote:
Would you recommend adding two elements to the ancillary_device like: void *ancillary_data; u32 ancildata_size; like the platform_device uses?
That doesn't seem useful here, the intent is to use container_of, if the creator wants to pass private data then it should be structured into the containing struct.
platform_devices/etc don't use container_of so have this side band way to pass more data.
The other thing platform_data lets you do is keep constant data separate from dynamic data - if you have to use container_of() then you need to either allocate a pointer to any constant data in the container or copy it into the container. Since the platform_data is in struct device it's going to be there anyway and may as well get used.
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.
MFD has no reliance on devices being DT enumerated, it works on systems that don't have DT and in many cases it's not clear that the split you'd want for the way Linux describes devices is a sensible one for other operating systems so we don't want to put it into DT. Forcing things to be DT enumerated would just create needless ABIs.
I agree the "DT enumerated" part should be removed.
To the best of my knowledge, the part of 'individual function devices being physical devices' is correct though. MFDs typically expose different functions on a single physical bus, and the functions are separated out by register maps. In the case where there's no physical bus/device and no register map it's unclear how MFDs would help?
On Thu, Oct 01, 2020 at 09:07:19AM -0500, Pierre-Louis Bossart wrote:
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.
MFD has no reliance on devices being DT enumerated, it works on systems that don't have DT and in many cases it's not clear that the split you'd
...
To the best of my knowledge, the part of 'individual function devices being physical devices' is correct though. MFDs typically expose different functions on a single physical bus, and the functions are separated out by register maps. In the case where there's no physical bus/device and no register map it's unclear how MFDs would help?
MFD doesn't care. All MFD is doing is instantiating platform devices and providing mechanisms to pass resources through from the parent device to the child devices. It doesn't really matter to it which if any combination of resources are being provided to the children or what the devices represent.
On 10/1/20 10:24 AM, Mark Brown wrote:
On Thu, Oct 01, 2020 at 09:07:19AM -0500, Pierre-Louis Bossart wrote:
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.
MFD has no reliance on devices being DT enumerated, it works on systems that don't have DT and in many cases it's not clear that the split you'd
...
To the best of my knowledge, the part of 'individual function devices being physical devices' is correct though. MFDs typically expose different functions on a single physical bus, and the functions are separated out by register maps. In the case where there's no physical bus/device and no register map it's unclear how MFDs would help?
MFD doesn't care. All MFD is doing is instantiating platform devices and providing mechanisms to pass resources through from the parent device to the child devices. It doesn't really matter to it which if any combination of resources are being provided to the children or what the devices represent.
I have nothing against MFD, but if this boils down to platform devices we are back to the initial open "are platform devices suitable as children of PCI devices"? I've heard Greg say no for the last year and a half - and he just re-stated this earlier in this thread.
On Thu, Oct 01, 2020 at 11:20:39AM -0500, Pierre-Louis Bossart wrote:
I have nothing against MFD, but if this boils down to platform devices we are back to the initial open "are platform devices suitable as children of PCI devices"? I've heard Greg say no for the last year and a half - and he just re-stated this earlier in this thread.
As you'll have seen from this thread and the prior version (which was the first time I became aware of this stuff) I'm not clear how that desire maps on to hardware, as soon as subdevices start needing to get regions and interrupts mapped then we're going to end up reinventing resources and regmaps AFAICT.
On Thu, Oct 01, 2020 at 05:51:37PM +0100, Mark Brown wrote:
On Thu, Oct 01, 2020 at 11:20:39AM -0500, Pierre-Louis Bossart wrote:
I have nothing against MFD, but if this boils down to platform devices we are back to the initial open "are platform devices suitable as children of PCI devices"? I've heard Greg say no for the last year and a half - and he just re-stated this earlier in this thread.
As you'll have seen from this thread and the prior version (which was the first time I became aware of this stuff) I'm not clear how that desire maps on to hardware, as soon as subdevices start needing to get regions and interrupts mapped then we're going to end up reinventing resources and regmaps AFAICT.
I think the truth is MFD and anciallary bus are solving almost the same problem and could meet in the middle at some point.
Since Greg has completely NAK'd using pci_device inside MFD it looks like this is the preference.
If people have a use case for regmaps/IRQs then they should add them here. Right now the places I know of don't need this.
The target for this is queue-based devices where the sharing granule is a queue. The actual thing being shared to the subsystem from the PCI driver basically the ability to create a queue.
Jason
On Thu, Oct 01, 2020 at 03:04:48PM -0300, Jason Gunthorpe wrote:
On Thu, Oct 01, 2020 at 05:51:37PM +0100, Mark Brown wrote:
On Thu, Oct 01, 2020 at 11:20:39AM -0500, Pierre-Louis Bossart wrote:
I have nothing against MFD, but if this boils down to platform devices we are back to the initial open "are platform devices suitable as children of PCI devices"? I've heard Greg say no for the last year and a half - and he just re-stated this earlier in this thread.
As you'll have seen from this thread and the prior version (which was the first time I became aware of this stuff) I'm not clear how that desire maps on to hardware, as soon as subdevices start needing to get regions and interrupts mapped then we're going to end up reinventing resources and regmaps AFAICT.
I think the truth is MFD and anciallary bus are solving almost the same problem and could meet in the middle at some point.
Agreed.
Since Greg has completely NAK'd using pci_device inside MFD it looks like this is the preference.
I have NAK'd using platform devices as children for things that are not platform devices. MFD is the thing that is wed to platform devices at the moment, so unless you want to change MFD to not enforce that (and last I looked the MFD maintainer said no), then yes, we can't use MFD.
If people have a use case for regmaps/IRQs then they should add them here. Right now the places I know of don't need this.
The target for this is queue-based devices where the sharing granule is a queue. The actual thing being shared to the subsystem from the PCI driver basically the ability to create a queue.
And PCI resources, you will have drivers that want that. But those are easy to share :)
thanks,
greg k-h
On Thu, Oct 01, 2020 at 03:04:48PM -0300, Jason Gunthorpe wrote:
On Thu, Oct 01, 2020 at 05:51:37PM +0100, Mark Brown wrote:
As you'll have seen from this thread and the prior version (which was the first time I became aware of this stuff) I'm not clear how that desire maps on to hardware, as soon as subdevices start needing to get regions and interrupts mapped then we're going to end up reinventing resources and regmaps AFAICT.
I think the truth is MFD and anciallary bus are solving almost the same problem and could meet in the middle at some point.
I do too, which is why I am pushing back.
Since Greg has completely NAK'd using pci_device inside MFD it looks like this is the preference.
I know Greg has said he doesn't want this, I would like to if not change his mind at least understand why so we can understand what we are supposed to be doing here and ideally capture it in the documentation.
If people have a use case for regmaps/IRQs then they should add them here. Right now the places I know of don't need this.
If it is actually the case that we're not supposed to instantiate platform devices as children of devices on actual physical buses then there's a huge chunk of existing MFDs that are use cases, including some PCI ones (though the PCI ones aren't great for the most part). If it is just PCI and USB devices then it becomes difficult to articulate the underlying logic about why those bus types in particular. I would expect at least some devices instantiated on PCI attached FPGAs to also need resources (it looks like at least one of the PCI MFDs that's been in the tree for a long time, timberdale, is actually such a device although not very dynamic like they could be).
I think the fundamental problem here is that this is all representing hardware which doesn't fit neatly into abstractions and that trying to separate out things based on abstract criteria (especially poorly defined abstract criteria) is going to leave us with a pile of sharp edges for people to run into, at least in terms of confusion and hassle. The messiness that exists is I fear a reflection of reality.
-----Original Message----- From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Sent: Thursday, October 1, 2020 7:07 AM To: Mark Brown broonie@kernel.org; Ertman, David M david.m.ertman@intel.com Cc: alsa-devel@alsa-project.org; tiwai@suse.de; gregkh@linuxfoundation.org; Sridharan, Ranjani ranjani.sridharan@intel.com; parav@nvidia.com; jgg@nvidia.com Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
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.
MFD has no reliance on devices being DT enumerated, it works on systems that don't have DT and in many cases it's not clear that the split you'd want for the way Linux describes devices is a sensible one for other operating systems so we don't want to put it into DT. Forcing things to be DT enumerated would just create needless ABIs.
I agree the "DT enumerated" part should be removed.
To the best of my knowledge, the part of 'individual function devices being physical devices' is correct though. MFDs typically expose different functions on a single physical bus, and the functions are separated out by register maps. In the case where there's no physical bus/device and no register map it's unclear how MFDs would help?
The MFD bus also uses parts of the platform bus in the background, including platform_data and the such. We submitted a version of the RDMA/LAN solution using MFD and it was NACK'd by GregKH.
-DaveE
On Thu, Oct 01, 2020 at 04:50:26PM +0000, Ertman, David M wrote:
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.
...
The MFD bus also uses parts of the platform bus in the background, including platform_data and the such. We submitted a version of the RDMA/LAN solution using MFD and it was NACK'd by GregKH.
That does not mean that it's a good idea to write documentation that says things that are not true. It would be better to just not say anything here rather than mislead people.
-----Original Message----- From: Mark Brown broonie@kernel.org Sent: Thursday, October 1, 2020 10:10 AM To: Ertman, David M david.m.ertman@intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com; alsa- devel@alsa-project.org; tiwai@suse.de; gregkh@linuxfoundation.org; Sridharan, Ranjani ranjani.sridharan@intel.com; parav@nvidia.com; jgg@nvidia.com Subject: Re: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
On Thu, Oct 01, 2020 at 04:50:26PM +0000, Ertman, David M wrote:
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.
...
The MFD bus also uses parts of the platform bus in the background,
including
platform_data and the such. We submitted a version of the RDMA/LAN
solution
using MFD and it was NACK'd by GregKH.
That does not mean that it's a good idea to write documentation that says things that are not true. It would be better to just not say anything here rather than mislead people.
I have removed the line about MFD devices requiring DT enumeration.
-DaveE
-DaveE
-----Original Message----- From: Alsa-devel alsa-devel-bounces@alsa-project.org On Behalf Of Dave Ertman Sent: Wednesday, September 30, 2020 3:51 PM To: alsa-devel@alsa-project.org Cc: tiwai@suse.de; gregkh@linuxfoundation.org; Sridharan, Ranjani ranjani.sridharan@intel.com; pierre-louis.bossart@linux.intel.com; broonie@kernel.org; parav@nvidia.com; jgg@nvidia.com Subject: [PATCH 0/6] Ancillary bus implementation and SOF multi-client support
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
Sorry, this is a borked link. Will fix with actual link:
https://lore.kernel.org/netdev/20191111192219.30259-1-jeffrey.t.kirsher@inte...
this was the original submission of virtual bus in Nov 2019.
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
participants (12)
-
Dan Williams -
Dave Ertman -
Ertman, David M
-
Greg KH -
gregkh＠linuxfoundation.org
-
Jason Gunthorpe -
Mark Brown -
Parav Pandit -
Pierre-Louis Bossart -
Rojewski, Cezary -
Sridharan, Ranjani -
Williams, Dan J