[resend/standalone PATCH v4] Add auxiliary bus support
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com --- This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
For example implementations incorporating this patch, see Dave Ertman's SOF series:
https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com
...and Leon's mlx5 series:
http://lore.kernel.org/r/20201026111849.1035786-1-leon@kernel.org
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Documentation/driver-api/auxiliary_bus.rst | 234 ++++++++++++++++++++++++ Documentation/driver-api/index.rst | 1 drivers/base/Kconfig | 3 drivers/base/Makefile | 1 drivers/base/auxiliary.c | 268 ++++++++++++++++++++++++++++ include/linux/auxiliary_bus.h | 78 ++++++++ include/linux/mod_devicetable.h | 8 + scripts/mod/devicetable-offsets.c | 3 scripts/mod/file2alias.c | 8 + 9 files changed, 604 insertions(+) create mode 100644 Documentation/driver-api/auxiliary_bus.rst create mode 100644 drivers/base/auxiliary.c create mode 100644 include/linux/auxiliary_bus.h
diff --git a/Documentation/driver-api/auxiliary_bus.rst b/Documentation/driver-api/auxiliary_bus.rst new file mode 100644 index 000000000000..5dd7804631ef --- /dev/null +++ b/Documentation/driver-api/auxiliary_bus.rst @@ -0,0 +1,234 @@ +.. SPDX-License-Identifier: GPL-2.0-only + +============= +Auxiliary Bus +============= + +In some subsystems, the functionality of the core device (PCI/ACPI/other) is +too complex for a single device to be managed by a monolithic driver +(e.g. Sound Open Firmware), multiple devices might implement a common +intersection of functionality (e.g. NICs + RDMA), or a driver may want to +export an interface for another subsystem to drive (e.g. SIOV Physical Function +export Virtual Function management). A split of the functinoality into child- +devices representing sub-domains of functionality makes it possible to +compartmentalize, layer, and distribute domain-specific concerns via a Linux +device-driver model. + +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. + +Each auxiliary_device represents a part of its parent functionality. The +generic behavior can be extended and specialized as needed by encapsulating an +auxiliary_device within other domain-specific structures and the use of .ops +callbacks. Devices on the auxiliary bus do not share any structures and the use +of a communication channel with the parent is domain-specific. + +Note that ops are intended as a way to augment instance behavior within a class +of auxiliary devices, it is not the mechanism for exporting common +infrastructure from the parent. Consider EXPORT_SYMBOL_NS() to convey +infrastructure from the parent module to the auxiliary module(s). + + +When Should the Auxiliary Bus Be Used +===================================== + +The auxiliary bus is to be used when a driver and one or more kernel modules, +who share a common header file with the driver, need a mechanism to connect and +provide access to a shared object allocated by the auxiliary_device's +registering driver. The registering driver for the auxiliary_device(s) and the +kernel module(s) registering auxiliary_drivers can be from the same subsystem, +or from multiple subsystems. + +The emphasis here is on a common generic interface that keeps subsystem +customization out of the bus infrastructure. + +One example is a PCI network device that is RDMA-capable and exports a child +device to be driven by an auxiliary_driver in the RDMA subsystem. The PCI +driver allocates and registers an auxiliary_device for each physical +function on the NIC. The RDMA driver registers an auxiliary_driver that claims +each of these auxiliary_devices. This conveys data/ops published by the parent +PCI device/driver to the RDMA auxiliary_driver. + +Another use case is for the PCI device to be split out into multiple sub +functions. For each sub function an auxiliary_device is created. A PCI sub +function driver binds to such devices that creates its own one or more class +devices. A PCI sub function auxiliary device is likely to be contained in a +struct with additional attributes such as user defined sub function number and +optional attributes such as resources and a link to the parent device. These +attributes could be used by systemd/udev; and hence should be initialized +before a driver binds to an auxiliary_device. + +A key requirement for utilizing the auxiliary bus 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. + +Auxiliary Device +================ + +An auxiliary_device represents a part of its parent device's functionality. It +is given a name that, combined with the registering drivers KBUILD_MODNAME, +creates a match_name that is used for driver binding, and an id that combined +with the match_name provide a unique name to register with the bus subsystem. + +Registering an auxiliary_device is a two-step process. First call +auxiliary_device_init(), which checks several aspects of the auxiliary_device +struct and performs a device_initialize(). After this step completes, any +error state must have a call to auxiliary_device_uninit() in its resolution path. +The second step in registering an auxiliary_device is to perform a call to +auxiliary_device_add(), which sets the name of the device and add the device to +the bus. + +Unregistering an auxiliary_device is also a two-step process to mirror the +register process. First call auxiliary_device_delete(), then call +auxiliary_device_uninit(). + +.. code-block:: c + + struct auxiliary_device { + struct device dev; + const char *name; + u32 id; + }; + +If two auxiliary_devices both with a match_name "mod.foo" are registered onto +the bus, they must have unique id values (e.g. "x" and "y") so that the +registered devices names are "mod.foo.x" and "mod.foo.y". If match_name + id +are not unique, then the device_add fails and generates an error message. + +The auxiliary_device.dev.type.release or auxiliary_device.dev.release must be +populated with a non-NULL pointer to successfully register the auxiliary_device. + +The auxiliary_device.dev.parent must also be populated. + +Auxiliary Device Memory Model and Lifespan +------------------------------------------ + +The registering driver is the entity that allocates memory for the +auxiliary_device and register it on the auxiliary bus. It is important to note +that, as opposed to the platform bus, the registering driver is wholly +responsible for the management for the memory used for the driver object. + +A parent object, defined in the shared header file, contains the +auxiliary_device. It also contains a pointer to the shared object(s), which +also is defined in the shared header. Both the parent object and the shared +object(s) are allocated by the registering driver. This layout allows the +auxiliary_driver's registering module to perform a container_of() call to go +from the pointer to the auxiliary_device, that is passed during the call to the +auxiliary_driver's probe function, up to the parent object, and then have +access to the shared object(s). + +The memory for the auxiliary_device is freed only in its release() callback +flow as defined by its registering driver. + +The memory for the shared object(s) must have a lifespan equal to, or greater +than, the lifespan of the memory for the auxiliary_device. The auxiliary_driver +should only consider that this shared object is valid as long as the +auxiliary_device is still registered on the auxiliary bus. It is up to the +registering driver to manage (e.g. free or keep available) the memory for the +shared object beyond the life of the auxiliary_device. + +The registering driver must unregister all auxiliary devices before its own +driver.remove() is completed. + +Auxiliary Drivers +================= + +Auxiliary drivers follow the standard driver model convention, where +discovery/enumeration is handled by the core, and drivers +provide probe() and remove() methods. They support power management +and shutdown notifications using the standard conventions. + +.. code-block:: c + + struct auxiliary_driver { + int (*probe)(struct auxiliary_device *, + const struct auxiliary_device_id *id); + int (*remove)(struct auxiliary_device *); + void (*shutdown)(struct auxiliary_device *); + int (*suspend)(struct auxiliary_device *, pm_message_t); + int (*resume)(struct auxiliary_device *); + struct device_driver driver; + const struct auxiliary_device_id *id_table; + }; + +Auxiliary drivers register themselves with the bus by calling +auxiliary_driver_register(). The id_table contains the match_names of auxiliary +devices that a driver can bind with. + +Example Usage +============= + +Auxiliary devices are created and registered by a subsystem-level core device +that needs to break up its functionality into smaller fragments. One way to +extend the scope of an auxiliary_device is to encapsulate it within a domain- +pecific structure defined by the parent device. This structure contains the +auxiliary_device and any associated shared data/callbacks needed to establish +the connection with the parent. + +An example is: + +.. code-block:: c + + struct foo { + struct auxiliary_device auxdev; + void (*connect)(struct auxiliary_device *auxdev); + void (*disconnect)(struct auxiliary_device *auxdev); + void *data; + }; + +The parent device then registers the auxiliary_device by calling +auxiliary_device_init(), and then auxiliary_device_add(), with the pointer to +the auxdev member of the above structure. The parent provides a name for the +auxiliary_device that, combined with the parent's KBUILD_MODNAME, creates a +match_name that is be used for matching and binding with a driver. + +Whenever an auxiliary_driver is registered, based on the match_name, the +auxiliary_driver's probe() is invoked for the matching devices. The +auxiliary_driver can also be encapsulated inside custom drivers that make the +core device's functionality extensible by adding additional domain-specific ops +as follows: + +.. code-block:: c + + struct my_ops { + void (*send)(struct auxiliary_device *auxdev); + void (*receive)(struct auxiliary_device *auxdev); + }; + + + struct my_driver { + struct auxiliary_driver auxiliary_drv; + const struct my_ops ops; + }; + +An example of this type of usage is: + +.. code-block:: c + + const struct auxiliary_device_id my_auxiliary_id_table[] = { + { .name = "foo_mod.foo_dev" }, + { }, + }; + + const struct my_ops my_custom_ops = { + .send = my_tx, + .receive = my_rx, + }; + + const struct my_driver my_drv = { + .auxiliary_drv = { + .name = "myauxiliarydrv", + .id_table = my_auxiliary_id_table, + .probe = my_probe, + .remove = my_remove, + .shutdown = my_shutdown, + }, + .ops = my_custom_ops, + }; diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst index f357f3eb400c..86759a74b7f1 100644 --- a/Documentation/driver-api/index.rst +++ b/Documentation/driver-api/index.rst @@ -72,6 +72,7 @@ available subsections can be seen below. thermal/index fpga/index acpi/index + auxiliary_bus backlight/lp855x-driver.rst connector console diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 8d7001712062..040be48ce046 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -1,6 +1,9 @@ # SPDX-License-Identifier: GPL-2.0 menu "Generic Driver Options"
+config AUXILIARY_BUS + bool + config UEVENT_HELPER bool "Support for uevent helper" help diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 41369fc7004f..5e7bf9669a81 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -7,6 +7,7 @@ obj-y := component.o core.o bus.o dd.o syscore.o \ attribute_container.o transport_class.o \ topology.o container.o property.o cacheinfo.o \ swnode.o +obj-$(CONFIG_AUXILIARY_BUS) += auxiliary.o obj-$(CONFIG_DEVTMPFS) += devtmpfs.o obj-y += power/ obj-$(CONFIG_ISA_BUS_API) += isa.o diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c new file mode 100644 index 000000000000..ef2af417438b --- /dev/null +++ b/drivers/base/auxiliary.c @@ -0,0 +1,268 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2019-2020 Intel Corporation + * + * Please see Documentation/driver-api/auxiliary_bus.rst for more information. + */ + +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__ + +#include <linux/device.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/pm_domain.h> +#include <linux/pm_runtime.h> +#include <linux/string.h> +#include <linux/auxiliary_bus.h> + +static const struct auxiliary_device_id *auxiliary_match_id(const struct auxiliary_device_id *id, + const struct auxiliary_device *auxdev) +{ + for (; id->name[0]; id++) { + const char *p = strrchr(dev_name(&auxdev->dev), '.'); + int match_size; + + if (!p) + continue; + match_size = p - dev_name(&auxdev->dev); + + /* use dev_name(&auxdev->dev) prefix before last '.' char to match to */ + if (strlen(id->name) == match_size && + !strncmp(dev_name(&auxdev->dev), id->name, match_size)) + return id; + } + return NULL; +} + +static int auxiliary_match(struct device *dev, struct device_driver *drv) +{ + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); + struct auxiliary_driver *auxdrv = to_auxiliary_drv(drv); + + return !!auxiliary_match_id(auxdrv->id_table, auxdev); +} + +static int auxiliary_uevent(struct device *dev, struct kobj_uevent_env *env) +{ + const char *name, *p; + + name = dev_name(dev); + p = strrchr(name, '.'); + + return add_uevent_var(env, "MODALIAS=%s%.*s", AUXILIARY_MODULE_PREFIX, (int)(p - name), + name); +} + +static const struct dev_pm_ops auxiliary_dev_pm_ops = { + SET_RUNTIME_PM_OPS(pm_generic_runtime_suspend, pm_generic_runtime_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend, pm_generic_resume) +}; + +static int auxiliary_bus_probe(struct device *dev) +{ + struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver); + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); + int ret; + + ret = dev_pm_domain_attach(dev, true); + if (ret) { + dev_warn(dev, "Failed to attach to PM Domain : %d\n", ret); + return ret; + } + + ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table, auxdev)); + if (ret) + dev_pm_domain_detach(dev, true); + + return ret; +} + +static int auxiliary_bus_remove(struct device *dev) +{ + struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver); + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); + int ret = 0; + + if (auxdrv->remove) + ret = auxdrv->remove(auxdev); + dev_pm_domain_detach(dev, true); + + return ret; +} + +static void auxiliary_bus_shutdown(struct device *dev) +{ + struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver); + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); + + if (auxdrv->shutdown) + auxdrv->shutdown(auxdev); +} + +static struct bus_type auxiliary_bus_type = { + .name = "auxiliary", + .probe = auxiliary_bus_probe, + .remove = auxiliary_bus_remove, + .shutdown = auxiliary_bus_shutdown, + .match = auxiliary_match, + .uevent = auxiliary_uevent, + .pm = &auxiliary_dev_pm_ops, +}; + +/** + * auxiliary_device_init - check auxiliary_device and initialize + * @auxdev: auxiliary device struct + * + * This is the first step in the two-step process to register an auxiliary_device. + * + * When this function returns an error code, then the device_initialize will *not* have + * been performed, and the caller will be responsible to free any memory allocated for the + * auxiliary_device in the error path directly. + * + * It returns 0 on success. On success, the device_initialize has been performed. After this + * point any error unwinding will need to include a call to auxiliary_device_uninit(). + * In this post-initialize error scenario, a call to the device's .release callback will be + * triggered, and all memory clean-up is expected to be handled there. + */ +int auxiliary_device_init(struct auxiliary_device *auxdev) +{ + struct device *dev = &auxdev->dev; + + if (!dev->parent) { + pr_err("auxiliary_device has a NULL dev->parent\n"); + return -EINVAL; + } + + if (!auxdev->name) { + pr_err("auxiliary_device has a NULL name\n"); + return -EINVAL; + } + + dev->bus = &auxiliary_bus_type; + device_initialize(&auxdev->dev); + return 0; +} +EXPORT_SYMBOL_GPL(auxiliary_device_init); + +/** + * __auxiliary_device_add - add an auxiliary bus device + * @auxdev: auxiliary bus device to add to the bus + * @modname: name of the parent device's driver module + * + * This is the second step in the two-step process to register an auxiliary_device. + * + * This function must be called after a successful call to auxiliary_device_init(), which + * will perform the device_initialize. This means that if this returns an error code, then a + * call to auxiliary_device_uninit() must be performed so that the .release callback will + * be triggered to free the memory associated with the auxiliary_device. + * + * The expectation is that users will call the "auxiliary_device_add" macro so that the caller's + * KBUILD_MODNAME is automatically inserted for the modname parameter. Only if a user requires + * a custom name would this version be called directly. + */ +int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname) +{ + struct device *dev = &auxdev->dev; + int ret; + + if (!modname) { + pr_err("auxiliary device modname is NULL\n"); + return -EINVAL; + } + + ret = dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id); + if (ret) { + pr_err("auxiliary device dev_set_name failed: %d\n", ret); + return ret; + } + + ret = device_add(dev); + if (ret) + dev_err(dev, "adding auxiliary device failed!: %d\n", ret); + + return ret; +} +EXPORT_SYMBOL_GPL(__auxiliary_device_add); + +/** + * auxiliary_find_device - auxiliary device iterator for locating a particular device. + * @start: Device to begin with + * @data: Data to pass to match function + * @match: Callback function to check device + * + * This function returns a reference to a device that is 'found' + * for later use, as determined by the @match callback. + * + * The callback should return 0 if the device doesn't match and non-zero + * if it does. If the callback returns non-zero, this function will + * return to the caller and not iterate over any more devices. + */ +struct auxiliary_device * +auxiliary_find_device(struct device *start, const void *data, + int (*match)(struct device *dev, const void *data)) +{ + struct device *dev; + + dev = bus_find_device(&auxiliary_bus_type, start, data, match); + if (!dev) + return NULL; + + return to_auxiliary_dev(dev); +} +EXPORT_SYMBOL_GPL(auxiliary_find_device); + +/** + * __auxiliary_driver_register - register a driver for auxiliary bus devices + * @auxdrv: auxiliary_driver structure + * @owner: owning module/driver + * @modname: KBUILD_MODNAME for parent driver + */ +int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct module *owner, + const char *modname) +{ + if (WARN_ON(!auxdrv->probe) || WARN_ON(!auxdrv->id_table)) + return -EINVAL; + + if (auxdrv->name) + auxdrv->driver.name = kasprintf(GFP_KERNEL, "%s.%s", modname, auxdrv->name); + else + auxdrv->driver.name = kasprintf(GFP_KERNEL, "%s", modname); + if (!auxdrv->driver.name) + return -ENOMEM; + + auxdrv->driver.owner = owner; + auxdrv->driver.bus = &auxiliary_bus_type; + auxdrv->driver.mod_name = modname; + + return driver_register(&auxdrv->driver); +} +EXPORT_SYMBOL_GPL(__auxiliary_driver_register); + +/** + * auxiliary_driver_unregister - unregister a driver + * @auxdrv: auxiliary_driver structure + */ +void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv) +{ + driver_unregister(&auxdrv->driver); + kfree(auxdrv->driver.name); +} +EXPORT_SYMBOL_GPL(auxiliary_driver_unregister); + +static int __init auxiliary_bus_init(void) +{ + return bus_register(&auxiliary_bus_type); +} + +static void __exit auxiliary_bus_exit(void) +{ + bus_unregister(&auxiliary_bus_type); +} + +module_init(auxiliary_bus_init); +module_exit(auxiliary_bus_exit); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Auxiliary Bus"); +MODULE_AUTHOR("David Ertman david.m.ertman@intel.com"); +MODULE_AUTHOR("Kiran Patil kiran.patil@intel.com"); diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h new file mode 100644 index 000000000000..282fbf7bf9af --- /dev/null +++ b/include/linux/auxiliary_bus.h @@ -0,0 +1,78 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2019-2020 Intel Corporation + * + * Please see Documentation/driver-api/auxiliary_bus.rst for more information. + */ + +#ifndef _AUXILIARY_BUS_H_ +#define _AUXILIARY_BUS_H_ + +#include <linux/device.h> +#include <linux/mod_devicetable.h> +#include <linux/slab.h> + +struct auxiliary_device { + struct device dev; + const char *name; + u32 id; +}; + +struct auxiliary_driver { + int (*probe)(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id); + int (*remove)(struct auxiliary_device *auxdev); + void (*shutdown)(struct auxiliary_device *auxdev); + int (*suspend)(struct auxiliary_device *auxdev, pm_message_t state); + int (*resume)(struct auxiliary_device *auxdev); + const char *name; + struct device_driver driver; + const struct auxiliary_device_id *id_table; +}; + +static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev) +{ + return container_of(dev, struct auxiliary_device, dev); +} + +static inline struct auxiliary_driver *to_auxiliary_drv(struct device_driver *drv) +{ + return container_of(drv, struct auxiliary_driver, driver); +} + +int auxiliary_device_init(struct auxiliary_device *auxdev); +int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname); +#define auxiliary_device_add(auxdev) __auxiliary_device_add(auxdev, KBUILD_MODNAME) + +static inline void auxiliary_device_uninit(struct auxiliary_device *auxdev) +{ + put_device(&auxdev->dev); +} + +static inline void auxiliary_device_delete(struct auxiliary_device *auxdev) +{ + device_del(&auxdev->dev); +} + +int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct module *owner, + const char *modname); +#define auxiliary_driver_register(auxdrv) \ + __auxiliary_driver_register(auxdrv, THIS_MODULE, KBUILD_MODNAME) + +void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv); + +/** + * module_auxiliary_driver() - Helper macro for registering an auxiliary driver + * @__auxiliary_driver: auxiliary driver struct + * + * Helper macro for auxiliary drivers which do not do anything special in + * module init/exit. This eliminates a lot of boilerplate. Each module may only + * use this macro once, and calling it replaces module_init() and module_exit() + */ +#define module_auxiliary_driver(__auxiliary_driver) \ + module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister) + +struct auxiliary_device * +auxiliary_find_device(struct device *start, const void *data, + int (*match)(struct device *dev, const void *data)); + +#endif /* _AUXILIARY_BUS_H_ */ diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index 5b08a473cdba..c425290b21e2 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -838,4 +838,12 @@ struct mhi_device_id { kernel_ulong_t driver_data; };
+#define AUXILIARY_NAME_SIZE 32 +#define AUXILIARY_MODULE_PREFIX "auxiliary:" + +struct auxiliary_device_id { + char name[AUXILIARY_NAME_SIZE]; + kernel_ulong_t driver_data; +}; + #endif /* LINUX_MOD_DEVICETABLE_H */ diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c index 27007c18e754..e377f52dbfa3 100644 --- a/scripts/mod/devicetable-offsets.c +++ b/scripts/mod/devicetable-offsets.c @@ -243,5 +243,8 @@ int main(void) DEVID(mhi_device_id); DEVID_FIELD(mhi_device_id, chan);
+ DEVID(auxiliary_device_id); + DEVID_FIELD(auxiliary_device_id, name); + return 0; } diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c index 2417dd1dee33..fb4827027536 100644 --- a/scripts/mod/file2alias.c +++ b/scripts/mod/file2alias.c @@ -1364,6 +1364,13 @@ static int do_mhi_entry(const char *filename, void *symval, char *alias) { DEF_FIELD_ADDR(symval, mhi_device_id, chan); sprintf(alias, MHI_DEVICE_MODALIAS_FMT, *chan); + return 1; +} + +static int do_auxiliary_entry(const char *filename, void *symval, char *alias) +{ + DEF_FIELD_ADDR(symval, auxiliary_device_id, name); + sprintf(alias, AUXILIARY_MODULE_PREFIX "%s", *name);
return 1; } @@ -1442,6 +1449,7 @@ static const struct devtable devtable[] = { {"tee", SIZE_tee_client_device_id, do_tee_entry}, {"wmi", SIZE_wmi_device_id, do_wmi_entry}, {"mhi", SIZE_mhi_device_id, do_mhi_entry}, + {"auxiliary", SIZE_auxiliary_device_id, do_auxiliary_entry}, };
/* Create MODULE_ALIAS() statements.
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
I have been trying to carve out some time to review this. At my initial glance, I still have objections, so please, give me a few more days to get this done...
thanks,
greg k-h
On Thu, Dec 03, 2020 at 04:06:24PM +0100, Greg KH wrote:
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
I have been trying to carve out some time to review this. At my initial glance, I still have objections, so please, give me a few more days to get this done...
There are still several more days till the merge window, but I am going to ask Leon to get the mlx5 series, and this version of the auxbus patch it depends on, into linux-next with the intention to forward it to Linus if there are no substantive comments.
Regardless of fault or reason this whole 1.5 year odyssey seems to have brought misery to everyone involved and it really is time to move on.
Leon and his team did a good deed 6 weeks ago to quickly turn around and build another user example. For their efforts they have been rewarded with major merge conflicts and alot of delayed work due to the invasive nature of the mlx5 changes. To continue to push this out is disrespectful to him and his team's efforts.
A major part of my time as RDMA maintainer has been to bring things away from vendor trees and into a common opensource community. Intel shipping a large out of tree RDMA driver and abandoning their intree driver is really harmful. This auxbus is a substantial blocker to them normalizing their operations, thus I view it as important to resolve. Even after this it is going to take a long time and alot of effort to review their new RDMA driver.
Regards, Jason
On Thu, Dec 3, 2020 at 6:34 PM Jason Gunthorpe jgg@nvidia.com wrote:
On Thu, Dec 03, 2020 at 04:06:24PM +0100, Greg KH wrote:
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
I have been trying to carve out some time to review this. At my initial glance, I still have objections, so please, give me a few more days to get this done...
There are still several more days till the merge window, but I am going to ask Leon to get the mlx5 series, and this version of the auxbus patch it depends on, into linux-next with the intention to forward it to Linus if there are no substantive comments.
Regardless of fault or reason this whole 1.5 year odyssey seems to have brought misery to everyone involved and it really is time to move on.
Leon and his team did a good deed 6 weeks ago to quickly turn around and build another user example. For their efforts they have been rewarded with major merge conflicts and alot of delayed work due to the invasive nature of the mlx5 changes. To continue to push this out is disrespectful to him and his team's efforts.
A major part of my time as RDMA maintainer has been to bring things away from vendor trees and into a common opensource community. Intel shipping a large out of tree RDMA driver and abandoning their intree driver is really harmful. This auxbus is a substantial blocker to them normalizing their operations, thus I view it as important to resolve. Even after this it is going to take a long time and alot of effort to review their new RDMA driver.
When you have 3 independent driver teams (mlx5, i40e, sof) across 2 companies (NVIDIA and Intel), and multiple subsystem maintainers with a positive track record of upstream engagement all agreeing on a piece of infrastructure, I struggle to imagine a stronger case for merging. I did get word of a fixup needed in the shutdown code, I'll get that folded. Then, barring a concrete objection, I'll look to publish a commit that others can pull in to start building soak time in -next this time tomorrow.
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Ah, but it's not only you that should be helping out here. Why isn't anyone else who is wanting this patch merged willing to also help out with patch review and bug fixes that have higher priority than adding new features like this one?
It's not your fault by any means, but the lack of anyone else willing to do this is quite sad :(
greg k-h
On Thu, Dec 03, 2020 at 04:07:42PM +0100, Greg KH wrote:
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Ah, but it's not only you that should be helping out here. Why isn't anyone else who is wanting this patch merged willing to also help out with patch review and bug fixes that have higher priority than adding new features like this one?
It's not your fault by any means, but the lack of anyone else willing to do this is quite sad :(
First step to get help is to ask for the help. From whom did you ask? And where did you ask? I never heard any request to help with newcomer patches, nor direct, nor in korg-users/newbies MLs.
Thanks
greg k-h
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
For example implementations incorporating this patch, see Dave Ertman's SOF series:
https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com
...and Leon's mlx5 series:
http://lore.kernel.org/r/20201026111849.1035786-1-leon@kernel.org
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Ok, I spent some hours today playing around with this. I wrote up a small test-patch for this (how did anyone test this thing???) and while it feels awkward in places, and it feels like there is still way too much "boilerplate" code that a user has to write and manage, I don't have the time myself to fix it up right now.
So I'll go apply this to my tree, and provide a tag for everyone else to be able to pull from for their different development trees so they can work on.
I do have 3 follow-on patches that I will send to the list in response to this message that I will be applying on top of this patch. They do some minor code formatting changes, as well as change the return type of the remove function to make it more future-proof. That last change will require users of this code to change their implementations, but it will be obvious what to do as you will get a build warning.
Note, I'm still not comfortable with a few things here. The documentation feels odd, and didn't really help me out in writing any test code, which doesn't seem right. Also the use of strings and '.' as part of the api feels awkward, and messy, and of course, totally undocumented.
But, as the use of '.' is undocumented, that means we can change it in the future! Because no driver or device name should ever be a user api reliant thing, if we come up with a better way to do all of this in the future, that shouldn't be a problem to change existing users over to this. So this is a warning to everyone, you CAN NOT depend on the sysfs name of a device or bus name for any tool. If so, your userspace tool is broken.
Thanks for everyone in sticking with this, I know it's been a long slog, hopefully this will help some driver authors move forward with their crazy complex devices :)
thanks,
greg k-h
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
No need to include slab.h in include/linux/auxiliary_bus.h, as it is not needed there. Move it to drivers/base/auxiliary.c instead.
Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/base/auxiliary.c | 1 + include/linux/auxiliary_bus.h | 1 - 2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c index ef2af417438b..eca36d6284d0 100644 --- a/drivers/base/auxiliary.c +++ b/drivers/base/auxiliary.c @@ -9,6 +9,7 @@
#include <linux/device.h> #include <linux/init.h> +#include <linux/slab.h> #include <linux/module.h> #include <linux/pm_domain.h> #include <linux/pm_runtime.h> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h index 282fbf7bf9af..3580743d0e8d 100644 --- a/include/linux/auxiliary_bus.h +++ b/include/linux/auxiliary_bus.h @@ -10,7 +10,6 @@
#include <linux/device.h> #include <linux/mod_devicetable.h> -#include <linux/slab.h>
struct auxiliary_device { struct device dev;
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
There's an effort to move the remove() callback in the driver core to not return an int, as nothing can be done if this function fails. To make that effort easier, make the aux bus remove function void to start with so that no users have to be changed sometime in the future.
Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- Documentation/driver-api/auxiliary_bus.rst | 2 +- drivers/base/auxiliary.c | 5 ++--- include/linux/auxiliary_bus.h | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/Documentation/driver-api/auxiliary_bus.rst b/Documentation/driver-api/auxiliary_bus.rst index 5dd7804631ef..2312506b0674 100644 --- a/Documentation/driver-api/auxiliary_bus.rst +++ b/Documentation/driver-api/auxiliary_bus.rst @@ -150,7 +150,7 @@ and shutdown notifications using the standard conventions. struct auxiliary_driver { int (*probe)(struct auxiliary_device *, const struct auxiliary_device_id *id); - int (*remove)(struct auxiliary_device *); + void (*remove)(struct auxiliary_device *); void (*shutdown)(struct auxiliary_device *); int (*suspend)(struct auxiliary_device *, pm_message_t); int (*resume)(struct auxiliary_device *); diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c index eca36d6284d0..c44e85802b43 100644 --- a/drivers/base/auxiliary.c +++ b/drivers/base/auxiliary.c @@ -82,13 +82,12 @@ static int auxiliary_bus_remove(struct device *dev) { struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver); struct auxiliary_device *auxdev = to_auxiliary_dev(dev); - int ret = 0;
if (auxdrv->remove) - ret = auxdrv->remove(auxdev); + auxdrv->remove(auxdev); dev_pm_domain_detach(dev, true);
- return ret; + return 0; }
static void auxiliary_bus_shutdown(struct device *dev) diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h index 3580743d0e8d..d67b17606210 100644 --- a/include/linux/auxiliary_bus.h +++ b/include/linux/auxiliary_bus.h @@ -19,7 +19,7 @@ struct auxiliary_device {
struct auxiliary_driver { int (*probe)(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id); - int (*remove)(struct auxiliary_device *auxdev); + void (*remove)(struct auxiliary_device *auxdev); void (*shutdown)(struct auxiliary_device *auxdev); int (*suspend)(struct auxiliary_device *auxdev, pm_message_t state); int (*resume)(struct auxiliary_device *auxdev);
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
For some reason, the original aux bus patch had some really long lines in a few places, probably due to it being a very long-lived patch in development by many different people. Fix that up so that the two files all have the same length lines and function formatting styles.
Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/base/Kconfig | 2 +- drivers/base/auxiliary.c | 58 ++++++++++++++++++++++------------------ 2 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 040be48ce046..ba52b2c40202 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -2,7 +2,7 @@ menu "Generic Driver Options"
config AUXILIARY_BUS - bool + tristate "aux bus!"
config UEVENT_HELPER bool "Support for uevent helper" diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c index c44e85802b43..f303daadf843 100644 --- a/drivers/base/auxiliary.c +++ b/drivers/base/auxiliary.c @@ -50,8 +50,8 @@ static int auxiliary_uevent(struct device *dev, struct kobj_uevent_env *env) name = dev_name(dev); p = strrchr(name, '.');
- return add_uevent_var(env, "MODALIAS=%s%.*s", AUXILIARY_MODULE_PREFIX, (int)(p - name), - name); + return add_uevent_var(env, "MODALIAS=%s%.*s", AUXILIARY_MODULE_PREFIX, + (int)(p - name), name); }
static const struct dev_pm_ops auxiliary_dev_pm_ops = { @@ -113,16 +113,18 @@ static struct bus_type auxiliary_bus_type = { * auxiliary_device_init - check auxiliary_device and initialize * @auxdev: auxiliary device struct * - * This is the first step in the two-step process to register an auxiliary_device. + * This is the first step in the two-step process to register an + * auxiliary_device. * - * When this function returns an error code, then the device_initialize will *not* have - * been performed, and the caller will be responsible to free any memory allocated for the - * auxiliary_device in the error path directly. + * When this function returns an error code, then the device_initialize will + * *not* have been performed, and the caller will be responsible to free any + * memory allocated for the auxiliary_device in the error path directly. * - * It returns 0 on success. On success, the device_initialize has been performed. After this - * point any error unwinding will need to include a call to auxiliary_device_uninit(). - * In this post-initialize error scenario, a call to the device's .release callback will be - * triggered, and all memory clean-up is expected to be handled there. + * It returns 0 on success. On success, the device_initialize has been + * performed. After this point any error unwinding will need to include a call + * to auxiliary_device_uninit(). In this post-initialize error scenario, a call + * to the device's .release callback will be triggered, and all memory clean-up + * is expected to be handled there. */ int auxiliary_device_init(struct auxiliary_device *auxdev) { @@ -149,16 +151,19 @@ EXPORT_SYMBOL_GPL(auxiliary_device_init); * @auxdev: auxiliary bus device to add to the bus * @modname: name of the parent device's driver module * - * This is the second step in the two-step process to register an auxiliary_device. + * This is the second step in the two-step process to register an + * auxiliary_device. * - * This function must be called after a successful call to auxiliary_device_init(), which - * will perform the device_initialize. This means that if this returns an error code, then a - * call to auxiliary_device_uninit() must be performed so that the .release callback will - * be triggered to free the memory associated with the auxiliary_device. + * This function must be called after a successful call to + * auxiliary_device_init(), which will perform the device_initialize. This + * means that if this returns an error code, then a call to + * auxiliary_device_uninit() must be performed so that the .release callback + * will be triggered to free the memory associated with the auxiliary_device. * - * The expectation is that users will call the "auxiliary_device_add" macro so that the caller's - * KBUILD_MODNAME is automatically inserted for the modname parameter. Only if a user requires - * a custom name would this version be called directly. + * The expectation is that users will call the "auxiliary_device_add" macro so + * that the caller's KBUILD_MODNAME is automatically inserted for the modname + * parameter. Only if a user requires a custom name would this version be + * called directly. */ int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname) { @@ -166,13 +171,13 @@ int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname) int ret;
if (!modname) { - pr_err("auxiliary device modname is NULL\n"); + dev_err(dev, "auxiliary device modname is NULL\n"); return -EINVAL; }
ret = dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id); if (ret) { - pr_err("auxiliary device dev_set_name failed: %d\n", ret); + dev_err(dev, "auxiliary device dev_set_name failed: %d\n", ret); return ret; }
@@ -197,9 +202,9 @@ EXPORT_SYMBOL_GPL(__auxiliary_device_add); * if it does. If the callback returns non-zero, this function will * return to the caller and not iterate over any more devices. */ -struct auxiliary_device * -auxiliary_find_device(struct device *start, const void *data, - int (*match)(struct device *dev, const void *data)) +struct auxiliary_device *auxiliary_find_device(struct device *start, + const void *data, + int (*match)(struct device *dev, const void *data)) { struct device *dev;
@@ -217,14 +222,15 @@ EXPORT_SYMBOL_GPL(auxiliary_find_device); * @owner: owning module/driver * @modname: KBUILD_MODNAME for parent driver */ -int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct module *owner, - const char *modname) +int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, + struct module *owner, const char *modname) { if (WARN_ON(!auxdrv->probe) || WARN_ON(!auxdrv->id_table)) return -EINVAL;
if (auxdrv->name) - auxdrv->driver.name = kasprintf(GFP_KERNEL, "%s.%s", modname, auxdrv->name); + auxdrv->driver.name = kasprintf(GFP_KERNEL, "%s.%s", modname, + auxdrv->name); else auxdrv->driver.name = kasprintf(GFP_KERNEL, "%s", modname); if (!auxdrv->driver.name)
On Fri, Dec 04, 2020 at 12:44:24PM +0100, Greg KH wrote:
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
For some reason, the original aux bus patch had some really long lines in a few places, probably due to it being a very long-lived patch in development by many different people. Fix that up so that the two files all have the same length lines and function formatting styles.
Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/base/Kconfig | 2 +- drivers/base/auxiliary.c | 58 ++++++++++++++++++++++------------------ 2 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 040be48ce046..ba52b2c40202 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -2,7 +2,7 @@ menu "Generic Driver Options"
config AUXILIARY_BUS
- bool
- tristate "aux bus!"
config UEVENT_HELPER bool "Support for uevent helper"
Argh, wrong version of this patch, this was added locally for me to test with, let me fix up and resend a v2 of this patch.
thanks,
greg k-h
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
For some reason, the original aux bus patch had some really long lines in a few places, probably due to it being a very long-lived patch in development by many different people. Fix that up so that the two files all have the same length lines and function formatting styles.
Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- v2: include the right files in the patch...
drivers/base/auxiliary.c | 58 +++++++++++++++++++---------------- include/linux/auxiliary_bus.h | 6 ++-- 2 files changed, 35 insertions(+), 29 deletions(-)
diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c index c44e85802b43..f303daadf843 100644 --- a/drivers/base/auxiliary.c +++ b/drivers/base/auxiliary.c @@ -50,8 +50,8 @@ static int auxiliary_uevent(struct device *dev, struct kobj_uevent_env *env) name = dev_name(dev); p = strrchr(name, '.');
- return add_uevent_var(env, "MODALIAS=%s%.*s", AUXILIARY_MODULE_PREFIX, (int)(p - name), - name); + return add_uevent_var(env, "MODALIAS=%s%.*s", AUXILIARY_MODULE_PREFIX, + (int)(p - name), name); }
static const struct dev_pm_ops auxiliary_dev_pm_ops = { @@ -113,16 +113,18 @@ static struct bus_type auxiliary_bus_type = { * auxiliary_device_init - check auxiliary_device and initialize * @auxdev: auxiliary device struct * - * This is the first step in the two-step process to register an auxiliary_device. + * This is the first step in the two-step process to register an + * auxiliary_device. * - * When this function returns an error code, then the device_initialize will *not* have - * been performed, and the caller will be responsible to free any memory allocated for the - * auxiliary_device in the error path directly. + * When this function returns an error code, then the device_initialize will + * *not* have been performed, and the caller will be responsible to free any + * memory allocated for the auxiliary_device in the error path directly. * - * It returns 0 on success. On success, the device_initialize has been performed. After this - * point any error unwinding will need to include a call to auxiliary_device_uninit(). - * In this post-initialize error scenario, a call to the device's .release callback will be - * triggered, and all memory clean-up is expected to be handled there. + * It returns 0 on success. On success, the device_initialize has been + * performed. After this point any error unwinding will need to include a call + * to auxiliary_device_uninit(). In this post-initialize error scenario, a call + * to the device's .release callback will be triggered, and all memory clean-up + * is expected to be handled there. */ int auxiliary_device_init(struct auxiliary_device *auxdev) { @@ -149,16 +151,19 @@ EXPORT_SYMBOL_GPL(auxiliary_device_init); * @auxdev: auxiliary bus device to add to the bus * @modname: name of the parent device's driver module * - * This is the second step in the two-step process to register an auxiliary_device. + * This is the second step in the two-step process to register an + * auxiliary_device. * - * This function must be called after a successful call to auxiliary_device_init(), which - * will perform the device_initialize. This means that if this returns an error code, then a - * call to auxiliary_device_uninit() must be performed so that the .release callback will - * be triggered to free the memory associated with the auxiliary_device. + * This function must be called after a successful call to + * auxiliary_device_init(), which will perform the device_initialize. This + * means that if this returns an error code, then a call to + * auxiliary_device_uninit() must be performed so that the .release callback + * will be triggered to free the memory associated with the auxiliary_device. * - * The expectation is that users will call the "auxiliary_device_add" macro so that the caller's - * KBUILD_MODNAME is automatically inserted for the modname parameter. Only if a user requires - * a custom name would this version be called directly. + * The expectation is that users will call the "auxiliary_device_add" macro so + * that the caller's KBUILD_MODNAME is automatically inserted for the modname + * parameter. Only if a user requires a custom name would this version be + * called directly. */ int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname) { @@ -166,13 +171,13 @@ int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname) int ret;
if (!modname) { - pr_err("auxiliary device modname is NULL\n"); + dev_err(dev, "auxiliary device modname is NULL\n"); return -EINVAL; }
ret = dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id); if (ret) { - pr_err("auxiliary device dev_set_name failed: %d\n", ret); + dev_err(dev, "auxiliary device dev_set_name failed: %d\n", ret); return ret; }
@@ -197,9 +202,9 @@ EXPORT_SYMBOL_GPL(__auxiliary_device_add); * if it does. If the callback returns non-zero, this function will * return to the caller and not iterate over any more devices. */ -struct auxiliary_device * -auxiliary_find_device(struct device *start, const void *data, - int (*match)(struct device *dev, const void *data)) +struct auxiliary_device *auxiliary_find_device(struct device *start, + const void *data, + int (*match)(struct device *dev, const void *data)) { struct device *dev;
@@ -217,14 +222,15 @@ EXPORT_SYMBOL_GPL(auxiliary_find_device); * @owner: owning module/driver * @modname: KBUILD_MODNAME for parent driver */ -int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct module *owner, - const char *modname) +int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, + struct module *owner, const char *modname) { if (WARN_ON(!auxdrv->probe) || WARN_ON(!auxdrv->id_table)) return -EINVAL;
if (auxdrv->name) - auxdrv->driver.name = kasprintf(GFP_KERNEL, "%s.%s", modname, auxdrv->name); + auxdrv->driver.name = kasprintf(GFP_KERNEL, "%s.%s", modname, + auxdrv->name); else auxdrv->driver.name = kasprintf(GFP_KERNEL, "%s", modname); if (!auxdrv->driver.name) diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h index d67b17606210..fc51d45f106b 100644 --- a/include/linux/auxiliary_bus.h +++ b/include/linux/auxiliary_bus.h @@ -70,8 +70,8 @@ void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv); #define module_auxiliary_driver(__auxiliary_driver) \ module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
-struct auxiliary_device * -auxiliary_find_device(struct device *start, const void *data, - int (*match)(struct device *dev, const void *data)); +struct auxiliary_device *auxiliary_find_device(struct device *start, + const void *data, + int (*match)(struct device *dev, const void *data));
#endif /* _AUXILIARY_BUS_H_ */
On Fri, Dec 04, 2020 at 12:42:46PM +0100, Greg KH wrote:
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
For example implementations incorporating this patch, see Dave Ertman's SOF series:
https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com
...and Leon's mlx5 series:
http://lore.kernel.org/r/20201026111849.1035786-1-leon@kernel.org
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Ok, I spent some hours today playing around with this. I wrote up a small test-patch for this (how did anyone test this thing???).
We are running all verifications tests that we have over our mlx5 driver. It includes devices reloads, power failures, FW reconfiguration to emulate different devices with and without error injections and many more. Up till now, no new bugs that are not known to us were found.
<...>
Note, I'm still not comfortable with a few things here. The documentation feels odd, and didn't really help me out in writing any test code, which doesn't seem right. Also the use of strings and '.' as part of the api feels awkward, and messy, and of course, totally undocumented.
Agree, I didn't look on the documentation at all when implemented mlx5. But from driver perspective the usage is quite straightforward.
<...>
Thanks for everyone in sticking with this, I know it's been a long slog, hopefully this will help some driver authors move forward with their crazy complex devices :)
Thanks a lot.
thanks,
greg k-h
From: Leon Romanovsky leonro@nvidia.com Sent: Friday, December 4, 2020 6:02 PM
On Fri, Dec 04, 2020 at 12:42:46PM +0100, Greg KH wrote:
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume
callbacks.
Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@in tel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other
stakeholders.
Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
For example implementations incorporating this patch, see Dave Ertman's SOF series:
https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@in tel.com
...and Leon's mlx5 series:
http://lore.kernel.org/r/20201026111849.1035786-1-leon@kernel.org
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Ok, I spent some hours today playing around with this. I wrote up a small test-patch for this (how did anyone test this thing???).
We are running all verifications tests that we have over our mlx5 driver. It includes devices reloads, power failures, FW reconfiguration to emulate different devices with and without error injections and many more. Up till now, no new bugs that are not known to us were found.
Subfunction patchset [1] that is using auxiliary bus in mlx5 driver is also been used by verification and performance tests.
[1] https://lore.kernel.org/linux-rdma/20201112192424.2742-1-parav@nvidia.com/
On Fri, Dec 04, 2020 at 02:32:07PM +0200, Leon Romanovsky wrote:
On Fri, Dec 04, 2020 at 12:42:46PM +0100, Greg KH wrote:
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
For example implementations incorporating this patch, see Dave Ertman's SOF series:
https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com
...and Leon's mlx5 series:
http://lore.kernel.org/r/20201026111849.1035786-1-leon@kernel.org
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Ok, I spent some hours today playing around with this. I wrote up a small test-patch for this (how did anyone test this thing???).
We are running all verifications tests that we have over our mlx5 driver. It includes devices reloads, power failures, FW reconfiguration to emulate different devices with and without error injections and many more. Up till now, no new bugs that are not known to us were found.
Yes, sorry, I was implying that the authors here had to create _some_ code to test this with, it would have been nice to include that as well here. We are collecting more and more in-kernel tests, having one for this code would be nice to also have so we make sure not to break any functionality in the future.
thanks,
greg k-h
On Fri, 2020-12-04 at 13:59 +0100, Greg KH wrote:
On Fri, Dec 04, 2020 at 02:32:07PM +0200, Leon Romanovsky wrote:
On Fri, Dec 04, 2020 at 12:42:46PM +0100, Greg KH wrote:
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan < ranjani.sridharan@linux.intel.com> Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan < ranjani.sridharan@linux.intel.com> Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
For example implementations incorporating this patch, see Dave Ertman's SOF series:
https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com
...and Leon's mlx5 series:
http://lore.kernel.org/r/20201026111849.1035786-1-leon@kernel.org
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Ok, I spent some hours today playing around with this. I wrote up a small test-patch for this (how did anyone test this thing???).
We are running all verifications tests that we have over our mlx5 driver. It includes devices reloads, power failures, FW reconfiguration to emulate different devices with and without error injections and many more. Up till now, no new bugs that are not known to us were found.
Yes, sorry, I was implying that the authors here had to create _some_ code to test this with, it would have been nice to include that as well here. We are collecting more and more in-kernel tests, having one for this code would be nice to also have so we make sure not to break any functionality in the future.
Hi Greg,
Thanks for your patience with this series. The v4 version submitted by Dave included the SOF usage code to demonstrate the usage. We have run all tests for device registration, module reload, PM etc and have not observed any regressions in the SOF audio driver.
Thanks, Ranjani
On Fri, Dec 04, 2020 at 09:10:34AM -0800, Ranjani Sridharan wrote:
On Fri, 2020-12-04 at 13:59 +0100, Greg KH wrote:
On Fri, Dec 04, 2020 at 02:32:07PM +0200, Leon Romanovsky wrote:
On Fri, Dec 04, 2020 at 12:42:46PM +0100, Greg KH wrote:
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan < ranjani.sridharan@linux.intel.com> Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan < ranjani.sridharan@linux.intel.com> Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
For example implementations incorporating this patch, see Dave Ertman's SOF series:
https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com
...and Leon's mlx5 series:
http://lore.kernel.org/r/20201026111849.1035786-1-leon@kernel.org
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Ok, I spent some hours today playing around with this. I wrote up a small test-patch for this (how did anyone test this thing???).
We are running all verifications tests that we have over our mlx5 driver. It includes devices reloads, power failures, FW reconfiguration to emulate different devices with and without error injections and many more. Up till now, no new bugs that are not known to us were found.
Yes, sorry, I was implying that the authors here had to create _some_ code to test this with, it would have been nice to include that as well here. We are collecting more and more in-kernel tests, having one for this code would be nice to also have so we make sure not to break any functionality in the future.
Hi Greg,
Thanks for your patience with this series. The v4 version submitted by Dave included the SOF usage code to demonstrate the usage. We have run all tests for device registration, module reload, PM etc and have not observed any regressions in the SOF audio driver.
Yes, that works great if you have that specific hardware to test with. If you don't, then it's kind of impossible to test this code :(
thanks,
greg k-h
On Fri, Dec 4, 2020 at 3:41 AM Greg KH gregkh@linuxfoundation.org wrote:
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
For example implementations incorporating this patch, see Dave Ertman's SOF series:
https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com
...and Leon's mlx5 series:
http://lore.kernel.org/r/20201026111849.1035786-1-leon@kernel.org
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Ok, I spent some hours today playing around with this. I wrote up a small test-patch for this (how did anyone test this thing???) and while it feels awkward in places, and it feels like there is still way too much "boilerplate" code that a user has to write and manage, I don't have the time myself to fix it up right now.
So I'll go apply this to my tree, and provide a tag for everyone else to be able to pull from for their different development trees so they can work on.
I do have 3 follow-on patches that I will send to the list in response to this message that I will be applying on top of this patch. They do some minor code formatting changes, as well as change the return type of the remove function to make it more future-proof. That last change will require users of this code to change their implementations, but it will be obvious what to do as you will get a build warning.
Note, I'm still not comfortable with a few things here. The documentation feels odd, and didn't really help me out in writing any test code, which doesn't seem right. Also the use of strings and '.' as part of the api feels awkward, and messy, and of course, totally undocumented.
But, as the use of '.' is undocumented, that means we can change it in the future! Because no driver or device name should ever be a user api reliant thing, if we come up with a better way to do all of this in the future, that shouldn't be a problem to change existing users over to this. So this is a warning to everyone, you CAN NOT depend on the sysfs name of a device or bus name for any tool. If so, your userspace tool is broken.
Thanks for everyone in sticking with this, I know it's been a long slog, hopefully this will help some driver authors move forward with their crazy complex devices :)
To me, the documentation was written, and reviewed, more from the perspective of "why not open code a custom bus instead". So I can see after the fact how that is a bit too much theory and justification and not enough practical application. Before the fact though this was a bold mechanism to propose and it was not clear that everyone was grokking the "why" and the tradeoffs.
I also think it was a bit early to identify consistent design patterns across the implementations and codify those. I expect this to evolve convenience macros just like other parts of the driver-core gained over time. Now that it is in though, another pass through the documentation to pull in more examples seems warranted.
On Fri, Dec 04, 2020 at 08:41:09AM -0800, Dan Williams wrote:
On Fri, Dec 4, 2020 at 3:41 AM Greg KH gregkh@linuxfoundation.org wrote:
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
For example implementations incorporating this patch, see Dave Ertman's SOF series:
https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com
...and Leon's mlx5 series:
http://lore.kernel.org/r/20201026111849.1035786-1-leon@kernel.org
PS: Greg I know I promised some review on newcomer patches to help with your queue, unfortunately Intel-internal review is keeping my plate full. Again, I do not want other stakeholder to be waiting on me to resolve that backlog.
Ok, I spent some hours today playing around with this. I wrote up a small test-patch for this (how did anyone test this thing???) and while it feels awkward in places, and it feels like there is still way too much "boilerplate" code that a user has to write and manage, I don't have the time myself to fix it up right now.
So I'll go apply this to my tree, and provide a tag for everyone else to be able to pull from for their different development trees so they can work on.
I do have 3 follow-on patches that I will send to the list in response to this message that I will be applying on top of this patch. They do some minor code formatting changes, as well as change the return type of the remove function to make it more future-proof. That last change will require users of this code to change their implementations, but it will be obvious what to do as you will get a build warning.
Note, I'm still not comfortable with a few things here. The documentation feels odd, and didn't really help me out in writing any test code, which doesn't seem right. Also the use of strings and '.' as part of the api feels awkward, and messy, and of course, totally undocumented.
But, as the use of '.' is undocumented, that means we can change it in the future! Because no driver or device name should ever be a user api reliant thing, if we come up with a better way to do all of this in the future, that shouldn't be a problem to change existing users over to this. So this is a warning to everyone, you CAN NOT depend on the sysfs name of a device or bus name for any tool. If so, your userspace tool is broken.
Thanks for everyone in sticking with this, I know it's been a long slog, hopefully this will help some driver authors move forward with their crazy complex devices :)
To me, the documentation was written, and reviewed, more from the perspective of "why not open code a custom bus instead". So I can see after the fact how that is a bit too much theory and justification and not enough practical application. Before the fact though this was a bold mechanism to propose and it was not clear that everyone was grokking the "why" and the tradeoffs.
Understood, I guess I read this from the "of course you should do this, now how do I use it?" point of view. Which still needs to be addressed I feel.
I also think it was a bit early to identify consistent design patterns across the implementations and codify those. I expect this to evolve convenience macros just like other parts of the driver-core gained over time. Now that it is in though, another pass through the documentation to pull in more examples seems warranted.
A real, working, example would be great to have, so that people can know how to use this. Trying to dig through the sound or IB patches to view how it is being used is not a trivial thing to do, which is why reviewing this took so much work. Having a simple example test module, that creates a number of devices on a bus, ideally tied into the ktest framework, would be great. I'll attach below a .c file that I used for some basic local testing to verify some of this working, but it does not implement a aux bus driver, which needs to be also tested.
thanks,
greg k-h
-------------
// SPDX-License-Identifier: GPL-2.0-only
#include <linux/init.h> #include <linux/device.h> #include <linux/module.h> #include <linux/slab.h> #include <linux/auxiliary_bus.h> #include <linux/platform_device.h>
struct aux_test_device { struct auxiliary_device auxdev; int foo; void *data; };
#define aux_dev_to_test_device(auxdev) \ container_of(auxdev, struct aux_test_device, auxdev)
static void aux_test_dev_release(struct device *dev) { struct auxiliary_device *auxdev = to_auxiliary_dev(dev); struct aux_test_device *test_dev = aux_dev_to_test_device(auxdev);
kfree(test_dev); }
static struct aux_test_device *test_device_alloc(struct device *parent, const char *name, u32 id) { struct aux_test_device *test_dev; struct auxiliary_device *auxdev; int retval;
test_dev = kzalloc(sizeof(*test_dev), GFP_KERNEL); if (!test_dev) return NULL;
auxdev= &test_dev->auxdev; auxdev->name = name; auxdev->dev.parent = parent; auxdev->dev.release = aux_test_dev_release; auxdev->id = id;
retval = auxiliary_device_init(auxdev); if (retval) { dev_err(parent, "aux device failed to init\n"); kfree(test_dev); return NULL; }
return test_dev; }
static struct aux_test_device *test_device_create(struct device *parent, const char *name, u32 id) { struct aux_test_device *test_dev; int retval;
test_dev = test_device_alloc(parent, name, id); if (!test_dev) { dev_err(parent, "aux device %s failed to be created\n", name); return NULL; }
retval = auxiliary_device_add(&test_dev->auxdev); if (retval) { dev_err(parent, "aux device %s failed to be added, error %d\n", name, retval); auxiliary_device_uninit(&test_dev->auxdev); return NULL; }
return test_dev; }
static void test_dev_del(struct aux_test_device *test_dev) { if (!test_dev) return;
auxiliary_device_delete(&test_dev->auxdev); auxiliary_device_uninit(&test_dev->auxdev); }
static struct aux_test_device *tdev1, *tdev2, *tdev3;
/* Make a random device to be the "parent" of our tests */ static struct platform_device *root_device;
static void root_device_release(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); kfree(pdev); }
static int __init aux_test_init(void) { int retval;
root_device = kzalloc(sizeof(*root_device), GFP_KERNEL); if (!root_device) return -ENOMEM;
root_device->name = "aux_test_root"; root_device->dev.release = root_device_release;
retval = platform_device_register(root_device); if (retval) { kfree(root_device); return retval; }
/* Allocate 3 test devices as a child of this parent */ tdev1 = test_device_create(&root_device->dev, "test_dev_1", 21); tdev2 = test_device_create(&root_device->dev, "test_dev_2", 32); tdev3 = test_device_create(&root_device->dev, "test_dev_3", 43);
return 0; }
static void __exit aux_test_exit(void) { test_dev_del(tdev1); test_dev_del(tdev2); test_dev_del(tdev3); platform_device_unregister(root_device);
}
module_init(aux_test_init); module_exit(aux_test_exit);
MODULE_LICENSE("GPL v2");
Hello,
On 05/12/2020 16:51:36+0100, Greg KH wrote:
To me, the documentation was written, and reviewed, more from the perspective of "why not open code a custom bus instead". So I can see after the fact how that is a bit too much theory and justification and not enough practical application. Before the fact though this was a bold mechanism to propose and it was not clear that everyone was grokking the "why" and the tradeoffs.
Understood, I guess I read this from the "of course you should do this, now how do I use it?" point of view. Which still needs to be addressed I feel.
I also think it was a bit early to identify consistent design patterns across the implementations and codify those. I expect this to evolve convenience macros just like other parts of the driver-core gained over time. Now that it is in though, another pass through the documentation to pull in more examples seems warranted.
A real, working, example would be great to have, so that people can know how to use this. Trying to dig through the sound or IB patches to view how it is being used is not a trivial thing to do, which is why reviewing this took so much work. Having a simple example test module, that creates a number of devices on a bus, ideally tied into the ktest framework, would be great. I'll attach below a .c file that I used for some basic local testing to verify some of this working, but it does not implement a aux bus driver, which needs to be also tested.
There is something I don't get from the documentation and it is what is this introducing that couldn't already be done using platform drivers and platform devices?
We already have a bunch of drivers in tree that have to share a state and register other drivers from other subsystems for the same device. How is the auxiliary bus different?
On Thu, Dec 17, 2020 at 1:20 PM Alexandre Belloni alexandre.belloni@bootlin.com wrote:
Hello,
On 05/12/2020 16:51:36+0100, Greg KH wrote:
To me, the documentation was written, and reviewed, more from the perspective of "why not open code a custom bus instead". So I can see after the fact how that is a bit too much theory and justification and not enough practical application. Before the fact though this was a bold mechanism to propose and it was not clear that everyone was grokking the "why" and the tradeoffs.
Understood, I guess I read this from the "of course you should do this, now how do I use it?" point of view. Which still needs to be addressed I feel.
I also think it was a bit early to identify consistent design patterns across the implementations and codify those. I expect this to evolve convenience macros just like other parts of the driver-core gained over time. Now that it is in though, another pass through the documentation to pull in more examples seems warranted.
A real, working, example would be great to have, so that people can know how to use this. Trying to dig through the sound or IB patches to view how it is being used is not a trivial thing to do, which is why reviewing this took so much work. Having a simple example test module, that creates a number of devices on a bus, ideally tied into the ktest framework, would be great. I'll attach below a .c file that I used for some basic local testing to verify some of this working, but it does not implement a aux bus driver, which needs to be also tested.
There is something I don't get from the documentation and it is what is this introducing that couldn't already be done using platform drivers and platform devices?
There is room for documentation improvement here. I realize reading it back now that much of the justification for "why not platform bus?" happened on the list, but only a small mention made it into the document. It turns out that platform-bus has some special integrations and hacks with platform-firmware implementations. For example, the ACPI companion magic and specific platform firmware integrations in platform_match(). It's also an awkward bus name to use because these devices do not belong to the platform. The platform bus is for devices that do not have an enumeration mechanism besides board files or firmware descriptions.
So while many of the auxiliary device use cases might be able to be squeezed into a platform-bus scheme it further overloads what is already a wide responsibility.
In comparison, the auxiliary-bus is tailored to the "sub-function of a parent device/driver" use case. It lets the host driver be the root of a namespace of sub-functionality in a standard template way.
We already have a bunch of drivers in tree that have to share a state and register other drivers from other subsystems for the same device. How is the auxiliary bus different?
There's also custom subsystem buses that do this. Why not other alternatives? They didn't capture the simultaneous mindshare of RDMA, SOF, and NETDEV developers. Personally my plans for using auxiliary-bus do not map cleanly to anything else in the tree. I want to use it for attaching an NPEM driver (Native PCIE Enclosure Management) to any PCI device driver that opts-in, but it would be overkill to go create an "npem" bus for this.
On Thu, Dec 17, 2020 at 06:39:55PM -0800, Dan Williams wrote:
There is room for documentation improvement here. I realize reading it back now that much of the justification for "why not platform bus?" happened on the list, but only a small mention made it into the
It wasn't clear from the list discussions either TBH, or at least the bits I happened to see (I did miss several versions of this).
document. It turns out that platform-bus has some special integrations and hacks with platform-firmware implementations. For example, the ACPI companion magic
Could you be more specific about the problems that these cause for users of the bus?
and specific platform firmware integrations inplatform_match(). It's also an awkward bus name to use because these
Going through a bunch of possible firmware interfaces is standard for buses that can't be enumerated, SPI has almost exactly the same code for example. Again, I'm not clear what problem this causes?
devices do not belong to the platform. The platform bus is for devices that do not have an enumeration mechanism besides board files or firmware descriptions.
This is the one thing I was getting from what I did see, it was an abstraction thing. I'm still unclear what the abstraction is supposed to be - I had thought that it was supposed to be purely for MMIO devices but in a parallel reply Greg is suggesting that it applies to at least "firmware direct" devices which I guess is things enumerated by board files or firmware but that makes things even less clear for me as it's kind of random if people try to describe the internals of devices in DT or not, and ACPI goes the other way and doesn't really describe some things that physically exist.
We already have a bunch of drivers in tree that have to share a state and register other drivers from other subsystems for the same device. How is the auxiliary bus different?
There's also custom subsystem buses that do this. Why not other alternatives? They didn't capture the simultaneous mindshare of RDMA, SOF, and NETDEV developers. Personally my plans for using
At least in the case of SOF they were getting active pushback from somewhere telling them not to use MFD.
auxiliary-bus do not map cleanly to anything else in the tree. I want to use it for attaching an NPEM driver (Native PCIE Enclosure Management) to any PCI device driver that opts-in, but it would be overkill to go create an "npem" bus for this.
This is why everyone is using platform devices here - people were making custom buses but people (including Greg!) pointed out that these were just carbon copies of the platform bus.
On Thu, Dec 17, 2020 at 10:19:37PM +0100, Alexandre Belloni wrote:
Hello,
On 05/12/2020 16:51:36+0100, Greg KH wrote:
To me, the documentation was written, and reviewed, more from the perspective of "why not open code a custom bus instead". So I can see after the fact how that is a bit too much theory and justification and not enough practical application. Before the fact though this was a bold mechanism to propose and it was not clear that everyone was grokking the "why" and the tradeoffs.
Understood, I guess I read this from the "of course you should do this, now how do I use it?" point of view. Which still needs to be addressed I feel.
I also think it was a bit early to identify consistent design patterns across the implementations and codify those. I expect this to evolve convenience macros just like other parts of the driver-core gained over time. Now that it is in though, another pass through the documentation to pull in more examples seems warranted.
A real, working, example would be great to have, so that people can know how to use this. Trying to dig through the sound or IB patches to view how it is being used is not a trivial thing to do, which is why reviewing this took so much work. Having a simple example test module, that creates a number of devices on a bus, ideally tied into the ktest framework, would be great. I'll attach below a .c file that I used for some basic local testing to verify some of this working, but it does not implement a aux bus driver, which needs to be also tested.
There is something I don't get from the documentation and it is what is this introducing that couldn't already be done using platform drivers and platform devices?
Because platform drivers and devices should ONLY be for actual platform devices. Do NOT use that interface to fake up a non-platform device (i.e. something that is NOT connected to a cpu through a memory-mapped or direct-firmware interface).
Do not abuse the platform code anymore than it currently is, it's bad enough what has been done to it over time, let's not make it any worse.
thanks,
greg k-h
On Fri, Dec 18, 2020 at 08:10:51AM +0100, Greg KH wrote:
On Thu, Dec 17, 2020 at 10:19:37PM +0100, Alexandre Belloni wrote:
There is something I don't get from the documentation and it is what is this introducing that couldn't already be done using platform drivers and platform devices?
Because platform drivers and devices should ONLY be for actual platform devices. Do NOT use that interface to fake up a non-platform device (i.e. something that is NOT connected to a cpu through a memory-mapped or direct-firmware interface).
Do not abuse the platform code anymore than it currently is, it's bad enough what has been done to it over time, let's not make it any worse.
I am not clear on why you're giving direct-firmware devices (which I assume means things like ARM SCMI where we're talking directly to some firmware?) a pass here but not for example a GPIO controlled devices. If this is mainly about improving abstractions it seems like the boundary here isn't great. Or perhaps I'm just missing what direct-firmware is supposed to mean?
In any case, to be clear part of what you're saying here is that all I2C and SPI MFDs should be rewritten to use this new bus - I've just copied Lee in again since he keeps getting missed from these threads. As previously discussed this will need the auxilliary bus extending to support at least interrupts and possibly also general resources.
On Fri, 18 Dec 2020, Mark Brown wrote:
On Fri, Dec 18, 2020 at 08:10:51AM +0100, Greg KH wrote:
On Thu, Dec 17, 2020 at 10:19:37PM +0100, Alexandre Belloni wrote:
There is something I don't get from the documentation and it is what is this introducing that couldn't already be done using platform drivers and platform devices?
Because platform drivers and devices should ONLY be for actual platform devices. Do NOT use that interface to fake up a non-platform device (i.e. something that is NOT connected to a cpu through a memory-mapped or direct-firmware interface).
Do not abuse the platform code anymore than it currently is, it's bad enough what has been done to it over time, let's not make it any worse.
I am not clear on why you're giving direct-firmware devices (which I assume means things like ARM SCMI where we're talking directly to some firmware?) a pass here but not for example a GPIO controlled devices. If this is mainly about improving abstractions it seems like the boundary here isn't great. Or perhaps I'm just missing what direct-firmware is supposed to mean?
In any case, to be clear part of what you're saying here is that all I2C and SPI MFDs should be rewritten to use this new bus - I've just copied Lee in again since he keeps getting missed from these threads. As previously discussed this will need the auxilliary bus extending to support at least interrupts and possibly also general resources.
Thanks Mark.
Not entirely sure why this needed an entirely new subsystem to handle non-MMIO Multi-Functional Devices (MFD). Or why I was not approached by any of the developers during the process.
Having 2 entirely separate subsystems where MFDs can now be registered sounds confusing and convoluted at best. Why not simply extend actual MFD to be capable of registering non-pure platform devices via other means? By doing so you keep things bound to a central location resulting in less chance of misuse.
I turn away MFD implementation abuses all the time. Seeing as the 2 subsystems are totally disjoint, this just unwittingly opened up another back-channel opportunity for those abuses to make it into the mainline kernel.
On Fri, Dec 18, 2020 at 01:17:09PM +0000, Mark Brown wrote:
As previously discussed this will need the auxilliary bus extending to support at least interrupts and possibly also general resources.
I thought the recent LWN article summed it up nicely, auxillary bus is for gluing to subsystems together using a driver specific software API to connect to the HW, MFD is for splitting a physical HW into disjoint regions of HW.
Maybe there is some overlap, but if you want to add HW representations to the general auxillary device then I think you are using it for the wrong thing.
Jason
On Fri, Dec 18, 2020 at 10:08:54AM -0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 01:17:09PM +0000, Mark Brown wrote:
As previously discussed this will need the auxilliary bus extending to support at least interrupts and possibly also general resources.
I thought the recent LWN article summed it up nicely, auxillary bus is for gluing to subsystems together using a driver specific software API to connect to the HW, MFD is for splitting a physical HW into disjoint regions of HW.
This conflicts with the statements from Greg about not using the platform bus for things that aren't memory mapped or "direct firmware", a large proportion of MFD subfunctions are neither at least in so far as I can understand what direct firmware means.
To be honest I don't find the LWN article clarifies things particularly here, the rationale appears to involve some misconceptions about what MFDs look like. It looks like it assumes that MFD functions have physically separate register sets for example which is not a reliable feature of MFDs, nor is the assumption that there's no shared functionality which appears to be there. It also appears to assume that MFD subfunctions can clearly be described by ACPI (where it would be unidiomatic, we just don't see this happening for the MFDs that appear on ACPI systems and I'm not sure bindings exist within ACPI) or DT (where even where subfunctions are individually described it's rarely doing more than enumerating that things exist).
Maybe there is some overlap, but if you want to add HW representations to the general auxillary device then I think you are using it for the wrong thing.
Even for the narrowest use case for auxiliary devices that I can think of I think the assumption that nobody will ever design something which can wire an interrupt intended to be serviced by a subfunction is a bit optimistic. If Greg's statements about not using platform buses for MMIO or direct firmware devices are accurate then those cases already exist, if nothing else a common subfunction for MFDs is an interrupt controller.
On Fri, Dec 18, 2020 at 03:52:04PM +0000, Mark Brown wrote:
On Fri, Dec 18, 2020 at 10:08:54AM -0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 01:17:09PM +0000, Mark Brown wrote:
As previously discussed this will need the auxilliary bus extending to support at least interrupts and possibly also general resources.
I thought the recent LWN article summed it up nicely, auxillary bus is for gluing to subsystems together using a driver specific software API to connect to the HW, MFD is for splitting a physical HW into disjoint regions of HW.
This conflicts with the statements from Greg about not using the platform bus for things that aren't memory mapped or "direct firmware", a large proportion of MFD subfunctions are neither at least in so far as I can understand what direct firmware means.
I assume MFD will keep existing and it will somehow stop using platform device for the children it builds.
That doesn't mean MFD must use aux device, so I don't see what you mean by conflicts?
If someone has a PCI device and they want to split it up, they should choose between aux device and MFD (assuming MFD gets fixed, as Greg has basically blanket NAK'd adding more of them to MFD as is)
To be honest I don't find the LWN article clarifies things particularly here, the rationale appears to involve some misconceptions about what MFDs look like. It looks like it assumes that MFD functions have physically separate register sets for example which is not a reliable feature of MFDs, nor is the assumption that there's no shared functionality which appears to be there. It also appears to assume that MFD subfunctions can clearly be described by ACPI (where it would be unidiomatic, we just don't see this happening for the MFDs that appear on ACPI systems and I'm not sure bindings exist within ACPI) or DT (where even where subfunctions are individually described it's rarely doing more than enumerating that things exist).
I think the MFD cell model is probably the deciding feature. If that cell description scheme suites the device, and it is very HW focused, then MFD is probably the answer.
The places I see aux device being used are a terrible fit for the cell idea. If there are MFD drivers that are awkardly crammed into that cell description then maybe they should be aux devices?
Maybe there is some overlap, but if you want to add HW representations to the general auxillary device then I think you are using it for the wrong thing.
Even for the narrowest use case for auxiliary devices that I can think of I think the assumption that nobody will ever design something which can wire an interrupt intended to be serviced by a subfunction is a bit optimistic.
mlx5, for example, uses interrupts but an aux device is not assigned an exclusive MSI interrupt list.
These devices have a very dynamic interrupt scheme, pre-partitioning the MSI vector table is completely the wrong API.
The "interrupt" API is more like:
mlx5_register_event_handler(hw_object, my_function);
Which would call my_function from some MSI interrupt vector when hw_object has an event to report. There might be 1000's of dynamic hw_objects in the system any moment.
As I said, I see aux device as being something that exposes a driver specifc SW API, not a list of generic HW resources.
Jason
On 18/12/2020 12:28:17-0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 03:52:04PM +0000, Mark Brown wrote:
On Fri, Dec 18, 2020 at 10:08:54AM -0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 01:17:09PM +0000, Mark Brown wrote:
As previously discussed this will need the auxilliary bus extending to support at least interrupts and possibly also general resources.
I thought the recent LWN article summed it up nicely, auxillary bus is for gluing to subsystems together using a driver specific software API to connect to the HW, MFD is for splitting a physical HW into disjoint regions of HW.
This conflicts with the statements from Greg about not using the platform bus for things that aren't memory mapped or "direct firmware", a large proportion of MFD subfunctions are neither at least in so far as I can understand what direct firmware means.
I assume MFD will keep existing and it will somehow stop using platform device for the children it builds.
That doesn't mean MFD must use aux device, so I don't see what you mean by conflicts?
If someone has a PCI device and they want to split it up, they should choose between aux device and MFD (assuming MFD gets fixed, as Greg has basically blanket NAK'd adding more of them to MFD as is)
I have an SoC with for example, a designware SPI controller (handled by drivers/spi/spi-dw-mmio.c), a designware I2C controller (handled by drivers/i2c/busses/i2c-designware-platdrv.c). So, those are MMIO and described using device tree. On this particular SoC, I can disable the CPU and connect it to another SoC using PCIe. In that case it will expose one BAR, with all the HW IPs.
The question here is why would I use something different from platform devices to register the SPI and I2C controllers? It seems that by using auxiliary devices, I would have to reinvent parsing device property and children/clients description. This isn't great from a code reuse perspective.
On Fri, Dec 18, 2020 at 12:28:17PM -0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 03:52:04PM +0000, Mark Brown wrote:
On Fri, Dec 18, 2020 at 10:08:54AM -0400, Jason Gunthorpe wrote:
I thought the recent LWN article summed it up nicely, auxillary bus is for gluing to subsystems together using a driver specific software API to connect to the HW, MFD is for splitting a physical HW into disjoint regions of HW.
This conflicts with the statements from Greg about not using the platform bus for things that aren't memory mapped or "direct firmware", a large proportion of MFD subfunctions are neither at least in so far as I can understand what direct firmware means.
I assume MFD will keep existing and it will somehow stop using platform device for the children it builds.
If it's not supposed to use platform devices so I'm assuming that the intention is that it should use aux devices, otherwise presumably it'd be making some new clone of the platform bus but I've not seen anyone suggesting this.
That doesn't mean MFD must use aux device, so I don't see what you mean by conflicts?
I was excluding the possibility that we have to make a third bus which clones platform bus which nobody has been visibly suggesting.
If someone has a PCI device and they want to split it up, they should choose between aux device and MFD (assuming MFD gets fixed, as Greg has basically blanket NAK'd adding more of them to MFD as is)
It is unclear to me how one is intended to choose between these approaches, especially for systems that have a range of subdevices with a range of characteristics.
To be honest I don't find the LWN article clarifies things particularly here, the rationale appears to involve some misconceptions about what MFDs look like. It looks like it assumes that MFD functions have physically separate register sets for example which is not a reliable feature of MFDs, nor is the assumption that there's no shared functionality which appears to be there. It also appears to assume that
I think the MFD cell model is probably the deciding feature. If that cell description scheme suites the device, and it is very HW focused, then MFD is probably the answer.
The places I see aux device being used are a terrible fit for the cell idea. If there are MFD drivers that are awkardly crammed into that cell description then maybe they should be aux devices?
When you say the MFD cell model it's not clear what you mean - I *think* you're referring to the idea of the subdevices getting all the information they need to talk to the hardware from the device resources. That's actually relatively uncommon with I2C/SPI MFDs, usually there's at least some element of just knowing what's going on and the mfd_cells are to some extent just a list of things to register rather than a model of anything.
Look at something like wm8994 for example - the subdevices just know which addresses in the device I2C/SPI regmap to work with but some of them have interrupts passed through to them (and could potentially also have separate subdevices for clocks and pinctrl). These subdevices are not memory mapped, not enumerated by firmware and the hardware has indistinct separation of functions in the register map compared to how Linux models the chips.
Maybe there is some overlap, but if you want to add HW representations to the general auxillary device then I think you are using it for the wrong thing.
Even for the narrowest use case for auxiliary devices that I can think of I think the assumption that nobody will ever design something which can wire an interrupt intended to be serviced by a subfunction is a bit optimistic.
mlx5, for example, uses interrupts but an aux device is not assigned an exclusive MSI interrupt list.
These devices have a very dynamic interrupt scheme, pre-partitioning the MSI vector table is completely the wrong API.
I'm not saying dynamic interrupt schemes or event queues from firmware can't exist, I'm saying not all interrupt schemes are dynamic.
On Fri, Dec 18, 2020 at 06:03:10PM +0000, Mark Brown wrote:
On Fri, Dec 18, 2020 at 12:28:17PM -0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 03:52:04PM +0000, Mark Brown wrote:
On Fri, Dec 18, 2020 at 10:08:54AM -0400, Jason Gunthorpe wrote:
I thought the recent LWN article summed it up nicely, auxillary bus is for gluing to subsystems together using a driver specific software API to connect to the HW, MFD is for splitting a physical HW into disjoint regions of HW.
This conflicts with the statements from Greg about not using the platform bus for things that aren't memory mapped or "direct firmware", a large proportion of MFD subfunctions are neither at least in so far as I can understand what direct firmware means.
I assume MFD will keep existing and it will somehow stop using platform device for the children it builds.
If it's not supposed to use platform devices so I'm assuming that the intention is that it should use aux devices, otherwise presumably it'd be making some new clone of the platform bus but I've not seen anyone suggesting this.
I wouldn't assume that, I certainly don't want to see all the HW related items in platform_device cloned roughly into aux device.
I've understood the bus type should be basically related to the thing that is creating the device. In a clean view platform code creates platform devices. DT should create DT devices, ACPI creates ACPI devices, PNP does pnp devices, etc
So, I strongly suspect, MFD should create mfd devices on a MFD bus type.
Alexandre's point is completely valid, and I think is the main challenge here, somehow avoiding duplication.
If we were to look at it with some OOP viewpoint I'd say the generic HW resource related parts should be some shared superclass between 'struct device' and 'struct platform/pnp/pci/acpi/mfd/etc_device'.
To be honest I don't find the LWN article clarifies things particularly here, the rationale appears to involve some misconceptions about what MFDs look like. It looks like it assumes that MFD functions have physically separate register sets for example which is not a reliable feature of MFDs, nor is the assumption that there's no shared functionality which appears to be there. It also appears to assume that
I think the MFD cell model is probably the deciding feature. If that cell description scheme suites the device, and it is very HW focused, then MFD is probably the answer.
The places I see aux device being used are a terrible fit for the cell idea. If there are MFD drivers that are awkardly crammed into that cell description then maybe they should be aux devices?
When you say the MFD cell model it's not clear what you mean - I *think* you're referring to the idea of the subdevices getting all the
I mean using static "struct mfd_cell" arrays to describe things.
Look at something like wm8994 for example - the subdevices just know which addresses in the device I2C/SPI regmap to work with but some of them have interrupts passed through to them (and could potentially also have separate subdevices for clocks and pinctrl). These subdevices are not memory mapped, not enumerated by firmware and the hardware has indistinct separation of functions in the register map compared to how Linux models the chips.
wm8994 seems to fit in the mfd_cell static arrays pretty well..
Jason
On Fri, 18 Dec 2020, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 06:03:10PM +0000, Mark Brown wrote:
On Fri, Dec 18, 2020 at 12:28:17PM -0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 03:52:04PM +0000, Mark Brown wrote:
On Fri, Dec 18, 2020 at 10:08:54AM -0400, Jason Gunthorpe wrote:
I thought the recent LWN article summed it up nicely, auxillary bus is for gluing to subsystems together using a driver specific software API to connect to the HW, MFD is for splitting a physical HW into disjoint regions of HW.
This conflicts with the statements from Greg about not using the platform bus for things that aren't memory mapped or "direct firmware", a large proportion of MFD subfunctions are neither at least in so far as I can understand what direct firmware means.
I assume MFD will keep existing and it will somehow stop using platform device for the children it builds.
If it's not supposed to use platform devices so I'm assuming that the intention is that it should use aux devices, otherwise presumably it'd be making some new clone of the platform bus but I've not seen anyone suggesting this.
I wouldn't assume that, I certainly don't want to see all the HW related items in platform_device cloned roughly into aux device.
I've understood the bus type should be basically related to the thing that is creating the device. In a clean view platform code creates platform devices. DT should create DT devices, ACPI creates ACPI devices, PNP does pnp devices, etc
So, I strongly suspect, MFD should create mfd devices on a MFD bus type.
Alexandre's point is completely valid, and I think is the main challenge here, somehow avoiding duplication.
If we were to look at it with some OOP viewpoint I'd say the generic HW resource related parts should be some shared superclass between 'struct device' and 'struct platform/pnp/pci/acpi/mfd/etc_device'.
You're confusing things here.
ACPI, DT and MFD are not busses. They are just methods to describe/register devices which will operate on buses.
Busses include things like; I2C, SPI, PCI, USB and Platform (MMIO).
On Fri, Dec 18, 2020 at 07:09:11PM +0000, Lee Jones wrote:
ACPI, DT and MFD are not busses.
And yet ACPI and PNP have a bus: extern struct bus_type acpi_bus_type; extern struct bus_type pnp_bus_type;
Why? Because in the driver core if you subclass struct device and want to bind drivers, as both PNP and ACPI do, you must place those devices on a bus with a bus_type matching the device type. Thus subclassing the device means subclassing the bus as well.
The purpose of the bus_type is to match drivers to devices and provide methods to the driver core. The bus_type also defines the unique name space of the device names.
It is confusing because the word bus immediately makes people think of physical objects like I2C, PCI, etc, but that is not what bus_type does in the object model of the driver core, IMHO.
So, if you subclass struct device for MFD's usage, then you must also create a bus_type to handle driver binding. The MFD bus_type. Just like auxillary does.
Making a mfd subclass is the logical thing for a subsystem to do, co-opting another subsystem's bus_type is just really weird/abusive.
auxillary bus shows how all these parts work, and it is simple enough to see the pieces clearly.
Jason
On Fri, Dec 18, 2020 at 02:41:50PM -0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 06:03:10PM +0000, Mark Brown wrote:
If it's not supposed to use platform devices so I'm assuming that the intention is that it should use aux devices, otherwise presumably it'd be making some new clone of the platform bus but I've not seen anyone suggesting this.
I wouldn't assume that, I certainly don't want to see all the HW related items in platform_device cloned roughly into aux device.
I've understood the bus type should be basically related to the thing that is creating the device. In a clean view platform code creates platform devices. DT should create DT devices, ACPI creates ACPI devices, PNP does pnp devices, etc
Ah, so we *used* to do that and in fact at least acpi_device still exists but it was realized that this was causing a lot of effort with boilerplate - like Lee said board files, ACPI and DT are all just enumeration methods which have zero effect on the underlying hardware so you end up having duplication on both the bus and driver side. Since this applies to all non-enumerable buses this process gets repeated for all of them, we wouldn't just have an of_device we'd have of_i2c_device, of_spi_device, of_1wire_device and so on or have to jump through hoops to map things into the actual bus type. See eca3930163ba8884060ce9d9 (of: Merge of_platform_bus_type with platform_bus_type) for part of this getting unwound.
Fundamentally this is conflating physical bus type and enumeration method, for enumerable buses they are of course the same (mostly) but for non-enumerable buses not so much.
So, I strongly suspect, MFD should create mfd devices on a MFD bus type.
Historically people did try to create custom bus types, as I have pointed out before there was then pushback that these were duplicating the platform bus so everything uses platform bus.
Alexandre's point is completely valid, and I think is the main challenge here, somehow avoiding duplication.
If we were to look at it with some OOP viewpoint I'd say the generic HW resource related parts should be some shared superclass between 'struct device' and 'struct platform/pnp/pci/acpi/mfd/etc_device'.
Right, duplication is the big issue with separate firmware based bus types particularly as we consider all non-enumerable buses. I think what you're looking for here is multiple inheritance, that's potentially interesting but it's pretty much what we have already TBH. We have the physical bus type as a primary type for devices but we also can enquire if they also have the properties of a DT or ACPI object and then use those APIs on them.
Consider also FPGAs which can have the same problem Alexandre raised, there's the parent device for the FPGA and then we can instantiate bitstreams within that which may expose standard IPs which can also appear directly within a SoC.
The places I see aux device being used are a terrible fit for the cell idea. If there are MFD drivers that are awkardly crammed into that cell description then maybe they should be aux devices?
When you say the MFD cell model it's not clear what you mean - I *think* you're referring to the idea of the subdevices getting all the
I mean using static "struct mfd_cell" arrays to describe things.
OK, but then SOF has been actively pushed into using auxiliary devices since there is a desire to avoid using mfd_cells on PCI devices rather than the fact that it wasn't able to use a static array, and of course you might have devices with a mix of static and dynamic functions, or functions that can be both static and dynamic.
Look at something like wm8994 for example - the subdevices just know which addresses in the device I2C/SPI regmap to work with but some of them have interrupts passed through to them (and could potentially also have separate subdevices for clocks and pinctrl). These subdevices are not memory mapped, not enumerated by firmware and the hardware has indistinct separation of functions in the register map compared to how Linux models the chips.
wm8994 seems to fit in the mfd_cell static arrays pretty well..
I can't tell the difference between what it's doing and what SOF is doing, the code I've seen is just looking at the system it's running on and registering a fixed set of client devices. It looks slightly different because it's registering a device at a time with some wrapper functions involved but that's what the code actually does.
Clearly there's something other than just the registration method going on here.
On Fri, Dec 18, 2020 at 08:32:11PM +0000, Mark Brown wrote:
So, I strongly suspect, MFD should create mfd devices on a MFD bus type.
Historically people did try to create custom bus types, as I have pointed out before there was then pushback that these were duplicating the platform bus so everything uses platform bus.
Yes, I vaugely remember..
I don't know what to say, it seems Greg doesn't share this view of platform devices as a universal device.
Reading between the lines, I suppose things would have been happier with some kind of inheritance scheme where platform device remained as only instantiated directly in board files, while drivers could bind to OF/DT/ACPI/FPGA/etc device instantiations with minimal duplication & boilerplate.
And maybe that is exactly what we have today with platform devices, though the name is now unfortunate.
I can't tell the difference between what it's doing and what SOF is doing, the code I've seen is just looking at the system it's running on and registering a fixed set of client devices. It looks slightly different because it's registering a device at a time with some wrapper functions involved but that's what the code actually does.
SOF's aux bus usage in general seems weird to me, but if you think it fits the mfd scheme of primarily describing HW to partition vs describing a SW API then maybe it should use mfd.
The only problem with mfd as far as SOF is concerned was Greg was not happy when he saw PCI stuff in the MFD subsystem.
This whole thing started when Intel first proposed to directly create platform_device's in their ethernet driver and Greg had a quite strong NAK to that.
MFD still doesn't fit what mlx5 and others in the netdev area are trying to do. Though it could have been soe-horned it would have been really weird to create a platform device with an empty HW resource list. At a certain point the bus type has to mean *something*!
Jason
On 18/12/2020 16:58:56-0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 08:32:11PM +0000, Mark Brown wrote:
So, I strongly suspect, MFD should create mfd devices on a MFD bus type.
Historically people did try to create custom bus types, as I have pointed out before there was then pushback that these were duplicating the platform bus so everything uses platform bus.
Yes, I vaugely remember..
I don't know what to say, it seems Greg doesn't share this view of platform devices as a universal device.
Reading between the lines, I suppose things would have been happier with some kind of inheritance scheme where platform device remained as only instantiated directly in board files, while drivers could bind to OF/DT/ACPI/FPGA/etc device instantiations with minimal duplication & boilerplate.
And maybe that is exactly what we have today with platform devices, though the name is now unfortunate.
I can't tell the difference between what it's doing and what SOF is doing, the code I've seen is just looking at the system it's running on and registering a fixed set of client devices. It looks slightly different because it's registering a device at a time with some wrapper functions involved but that's what the code actually does.
SOF's aux bus usage in general seems weird to me, but if you think it fits the mfd scheme of primarily describing HW to partition vs describing a SW API then maybe it should use mfd.
The only problem with mfd as far as SOF is concerned was Greg was not happy when he saw PCI stuff in the MFD subsystem.
But then again, what about non-enumerable devices on the PCI device? I feel this would exactly fit MFD. This is a collection of IPs that exist as standalone but in this case are grouped in a single device.
Note that I then have another issue because the kernel doesn't support irq controllers on PCI and this is exactly what my SoC has. But for now, I can just duplicate the irqchip driver in the MFD driver.
This whole thing started when Intel first proposed to directly create platform_device's in their ethernet driver and Greg had a quite strong NAK to that.
Let me point to drivers/net/ethernet/cadence/macb_pci.c which is a fairly recent example. It does exactly that and I'm not sure you could do it otherwise while still not having to duplicate most of macb_probe.
On Fri, Dec 18, 2020 at 1:17 PM Alexandre Belloni alexandre.belloni@bootlin.com wrote:
On 18/12/2020 16:58:56-0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 08:32:11PM +0000, Mark Brown wrote:
So, I strongly suspect, MFD should create mfd devices on a MFD bus type.
Historically people did try to create custom bus types, as I have pointed out before there was then pushback that these were duplicating the platform bus so everything uses platform bus.
Yes, I vaugely remember..
I don't know what to say, it seems Greg doesn't share this view of platform devices as a universal device.
Reading between the lines, I suppose things would have been happier with some kind of inheritance scheme where platform device remained as only instantiated directly in board files, while drivers could bind to OF/DT/ACPI/FPGA/etc device instantiations with minimal duplication & boilerplate.
And maybe that is exactly what we have today with platform devices, though the name is now unfortunate.
I can't tell the difference between what it's doing and what SOF is doing, the code I've seen is just looking at the system it's running on and registering a fixed set of client devices. It looks slightly different because it's registering a device at a time with some wrapper functions involved but that's what the code actually does.
SOF's aux bus usage in general seems weird to me, but if you think it fits the mfd scheme of primarily describing HW to partition vs describing a SW API then maybe it should use mfd.
The only problem with mfd as far as SOF is concerned was Greg was not happy when he saw PCI stuff in the MFD subsystem.
But then again, what about non-enumerable devices on the PCI device? I feel this would exactly fit MFD. This is a collection of IPs that exist as standalone but in this case are grouped in a single device.
Note that I then have another issue because the kernel doesn't support irq controllers on PCI and this is exactly what my SoC has. But for now, I can just duplicate the irqchip driver in the MFD driver.
This whole thing started when Intel first proposed to directly create platform_device's in their ethernet driver and Greg had a quite strong NAK to that.
Let me point to drivers/net/ethernet/cadence/macb_pci.c which is a fairly recent example. It does exactly that and I'm not sure you could do it otherwise while still not having to duplicate most of macb_probe.
This still feels an orthogonal example to the problem auxiliary-bus is solving. If a platform-device and a pci-device surface an IP with a shared programming model that's an argument for a shared library, like libata to house the commonality. In contrast auxiliary-bus is a software model for software-defined sub-functionality to be wrapped in a driver model. It assumes a parent-device / parent-driver hierarchy that platform-bus and pci-bus do not imply.
On Fri, Dec 18, 2020 at 10:16:58PM +0100, Alexandre Belloni wrote:
But then again, what about non-enumerable devices on the PCI device? I feel this would exactly fit MFD. This is a collection of IPs that exist as standalone but in this case are grouped in a single device.
So, if mfd had a mfd_device and a mfd bus_type then drivers would need to have both a mfd_driver and a platform_driver to bind. Look at something like drivers/char/tpm/tpm_tis.c to see how a multi-probe driver is structured
See Mark's remarks about the old of_platform_device, to explain why we don't have a 'dt_device' today
Note that I then have another issue because the kernel doesn't support irq controllers on PCI and this is exactly what my SoC has. But for now, I can just duplicate the irqchip driver in the MFD driver.
I think Thomas fixed that recently on x86 at least..
Having to put dummy irq chip drivers in MFD anything sounds scary :|
Let me point to drivers/net/ethernet/cadence/macb_pci.c which is a fairly recent example. It does exactly that and I'm not sure you could do it otherwise while still not having to duplicate most of macb_probe.
Creating a platform_device to avoid restructuring the driver's probe and device logic to be generic is a *really* horrible reason to use a platform device.
Jason
On 18/12/2020 19:36:08-0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 10:16:58PM +0100, Alexandre Belloni wrote:
But then again, what about non-enumerable devices on the PCI device? I feel this would exactly fit MFD. This is a collection of IPs that exist as standalone but in this case are grouped in a single device.
So, if mfd had a mfd_device and a mfd bus_type then drivers would need to have both a mfd_driver and a platform_driver to bind. Look at something like drivers/char/tpm/tpm_tis.c to see how a multi-probe driver is structured
See Mark's remarks about the old of_platform_device, to explain why we don't have a 'dt_device' today
So, what would that mfd_driver have that the platform_driver doesn't already provide?
Note that I then have another issue because the kernel doesn't support irq controllers on PCI and this is exactly what my SoC has. But for now, I can just duplicate the irqchip driver in the MFD driver.
I think Thomas fixed that recently on x86 at least..
Having to put dummy irq chip drivers in MFD anything sounds scary :|
This isn't a dummy driver it is a real irqchip, what issue is there to register an irqchip from MFD ?
Let me point to drivers/net/ethernet/cadence/macb_pci.c which is a fairly recent example. It does exactly that and I'm not sure you could do it otherwise while still not having to duplicate most of macb_probe.
Creating a platform_device to avoid restructuring the driver's probe and device logic to be generic is a *really* horrible reason to use a platform device.
Definitively but it made it in and seemed reasonable at the time it seems. I stumbled upon that a while ago because I wanted to remove platform_data support from the macb driver and this is the last user. I never got the time to tackle that.
On Fri, Dec 18, 2020 at 04:58:56PM -0400, Jason Gunthorpe wrote:
On Fri, Dec 18, 2020 at 08:32:11PM +0000, Mark Brown wrote:
Historically people did try to create custom bus types, as I have pointed out before there was then pushback that these were duplicating the platform bus so everything uses platform bus.
Yes, I vaugely remember..
I don't know what to say, it seems Greg doesn't share this view of platform devices as a universal device.
He did at the time, he seems to have changed his mind at some point for unclear reasons
Reading between the lines, I suppose things would have been happier with some kind of inheritance scheme where platform device remained as only instantiated directly in board files, while drivers could bind to OF/DT/ACPI/FPGA/etc device instantiations with minimal duplication & boilerplate.
Like I said in my previous message that is essentially what we have now. It's not worded in quite that way but it's how all the non-enumerable buses work.
BTW I did have a bit of a scan through some of the ACPI devices and for a good proportion of them it seems fairly clear that they are not platform devices at all - they were mostly interacting with ACPI firmware functionality rather than hardware, something you can't really do with FDT at all.
I can't tell the difference between what it's doing and what SOF is doing, the code I've seen is just looking at the system it's running on and registering a fixed set of client devices. It looks slightly different because it's registering a device at a time with some wrapper functions involved but that's what the code actually does.
SOF's aux bus usage in general seems weird to me, but if you think it fits the mfd scheme of primarily describing HW to partition vs describing a SW API then maybe it should use mfd.
The only problem with mfd as far as SOF is concerned was Greg was not happy when he saw PCI stuff in the MFD subsystem.
This is a huge part of the problem here - there's no clearly articulated logic, it's all coming back to these sorts of opinion statements about specific cases which aren't really something you can base anything on. Personally I'm even struggling to identify a practical problem that we're trying to solve here. Like Alexandre says what would an mfd_driver actually buy us?
MFD still doesn't fit what mlx5 and others in the netdev area are trying to do. Though it could have been soe-horned it would have been really weird to create a platform device with an empty HW resource list. At a certain point the bus type has to mean *something*!
I have some bad news for you about the hardware description problem space. Among other things we have a bunch of platform devices that don't have any resources exposed through the resource API but are still things like chips on a board, doing some combination of exposing resources for other devices (eg, a fixed voltage regulator) and consuming things like clocks or GPIOs that don't appear in the resource API. We *could* make a new bus type and move all these things over to that but it is not clear what the upside of doing that would be, especially given the amount of upheval it would generate and the classification issues that will inevitably result.
On Mon, Dec 21, 2020 at 06:51:40PM +0000, Mark Brown wrote:
with some kind of inheritance scheme where platform device remained as only instantiated directly in board files, while drivers could bind to OF/DT/ACPI/FPGA/etc device instantiations with minimal duplication & boilerplate.
Like I said in my previous message that is essentially what we have now. It's not worded in quite that way but it's how all the non-enumerable buses work.
I think it is about half way there. We jammed everything into platform device and platform bus and then had a few api aspects to tell if which of the subtypes it might be.
That functions sort of like an object model with inheritance, but a single type and 'is it a XXX' queries is not quite the same thing.
BTW I did have a bit of a scan through some of the ACPI devices and for a good proportion of them it seems fairly clear that they are not platform devices at all - they were mostly interacting with ACPI firmware functionality rather than hardware, something you can't really do with FDT at all.
Right, that is kind of the point. We also have cases where ACPI devices are just an ioresource list and don't have any special ACPIness. IIRC DT has a similar issue where there are DT drivers that just don't work without the OF stuff. Why are they platform drivers?
IMHO the point of the bus type is to tell the driver what API set you have. If you have a of_device then you have an OF node and can do all the of operations. Same for PCI/ACPI/etc.
We fake this idea out by being able to convert platform to DT and OF, but if platform is to be the universal device then why do we have PCI device and not a 'platform to pci' operator instead? None of this is consistent.
Regardless of the shortcut to make everything a struct platform_device, I think it was a mistake to put OF devices on platform_bus. Those should have remained on some of_bus even if they are represented by struct platform_device and fiddling in the core done to make that work OK.
It is much easier to identify what a bus_type is (the unique collection of APIs) and thus when to create those.
If the bus_type should contain struct platform_device or a unqiue struct then becomes a different question.
Yes that is very hacky, but it feels less hacky than the platform bus/device is everything and can be used everwhere idea.
The only problem with mfd as far as SOF is concerned was Greg was not happy when he saw PCI stuff in the MFD subsystem.
This is a huge part of the problem here - there's no clearly articulated logic, it's all coming back to these sorts of opinion statements about specific cases which aren't really something you can base anything on.
I agree with this, IMHO there is no really cohesive explanation for when to create a bus vs use the "universal bus" (platform) that can also explain the things platform is already doing.
This feels like a good conference topic someday..
Personally I'm even struggling to identify a practical problem that we're trying to solve here. Like Alexandre says what would an mfd_driver actually buy us?
Well, there is the minor issue of name collision eg /sys/bus/XX/devices/* must list all devices in the system with no collisions.
The owner of the bus is supposed to define the stable naming scheme and all the devices are supposed to follow it. platform doesn't have this:
$ ls /sys/bus/platform/devices/ ACPI000C:00 dell-smbios.0 'Fixed MDIO bus.0' INT33A1:00 microcode PNP0C04:00 PNP0C0B:03 PNP0C14:00 alarmtimer.0.auto dell-smbios.1 GHES.0 intel_rapl_msr.0 MSFT0101:00 PNP0C0B:00 PNP0C0B:04 PNP0C14:01 coretemp.0 efi-framebuffer.0 GHES.1 iTCO_wdt pcspkr PNP0C0B:01 PNP0C0C:00 reg-dummy dcdbas eisa.0 INT0800:00 kgdboc PNP0103:00 PNP0C0B:02 PNP0C0E:00 serial8250
Why are ACPI names in here? It looks like "because platform drivers were used to bind to ACPI devices"
eg INT33A1 is pmc_core_driver so the device was moved from acpi_bus to platform_bus? How does that make sense??
Why is pmc_core_driver a platform device instead of ACPI? Because some platforms don't have ACPI and the board file properly creates a platform device in C code.
I have some bad news for you about the hardware description problem space. Among other things we have a bunch of platform devices that don't have any resources exposed through the resource API but are still things like chips on a board, doing some combination of exposing resources for other devices (eg, a fixed voltage regulator) and consuming things like clocks or GPIOs that don't appear in the resource API.
So in these cases how do I use the generic platform bus API to find the GPIOs, regulators, and so on to connect with?
If drivers take a platform device and immediately covert it to an OF object and use OF APIs to find those connections then it really *never* was a platform device in the first place and coding an OF driver as platform is an abuse.
A decent step would be to accept that 'platform_device' is something weird and special and split its bus_type. Only devices created direclty in C code should be on the platform_bus, OF/ACPI/etc should all be on their own bus_types, even though they all use the same 'struct platform_bus'
Yes, it is super hacky, but at least the bus side would follow the basic architecture..
that but it is not clear what the upside of doing that would be, especially given the amount of upheval it would generate and the classification issues that will inevitably result.
Well, I think the upside for existing is very small, but I would like to see a shared idea about how to answer questions like 'when should I use a existing device type' and 'when should I make a new device type'.
Jason
On Mon, Jan 04, 2021 at 02:08:31PM -0400, Jason Gunthorpe wrote:
On Mon, Dec 21, 2020 at 06:51:40PM +0000, Mark Brown wrote:
BTW I did have a bit of a scan through some of the ACPI devices and for a good proportion of them it seems fairly clear that they are not platform devices at all - they were mostly interacting with ACPI firmware functionality rather than hardware, something you can't really do with FDT at all.
Right, that is kind of the point. We also have cases where ACPI devices are just an ioresource list and don't have any special ACPIness. IIRC DT has a similar issue where there are DT drivers that just don't work without the OF stuff. Why are they platform drivers?
There *might* be some very legacy ones for actual OF systems but that's not really at all a thing for FDT which is essentially all DT usage at this point - to the extent that things won't work it's due to the non-enumarability of the hardware so it's just a question of where the data comes from rather than one of communicating with a firmware (and for more generic things like GPIOs exactly where the data comes from ends up abstracted away from the driver anyway which is all some devices need). The idiom with DT is more that it's just a different way of filling out the data you'd get from a board file, it's not a runtime thing like ACPI.
We fake this idea out by being able to convert platform to DT and OF, but if platform is to be the universal device then why do we have PCI device and not a 'platform to pci' operator instead? None of this is consistent.
If it were more common for there to be IPs that appear as both PCI and platform devices (Intel do it a bit but otherwise it's quite rare) I'd guess we would actually have helpers for that translation.
Regardless of the shortcut to make everything a struct platform_device, I think it was a mistake to put OF devices on platform_bus. Those should have remained on some of_bus even if they
Like I keep saying the same thing applies to all non-enumerable buses - exactly the same considerations exist for all the other buses like I2C (including the ACPI naming issue you mention below), and for that matter with enumerable buses which can have firmware info.
are represented by struct platform_device and fiddling in the core done to make that work OK.
What exactly is the fiddling in the core here, I'm a bit unclear?
I agree with this, IMHO there is no really cohesive explanation for when to create a bus vs use the "universal bus" (platform) that can also explain the things platform is already doing.
This feels like a good conference topic someday..
We should have this discussion *before* we get too far along with trying to implement things, we should at least have some idea where we want to head there.
Personally I'm even struggling to identify a practical problem that we're trying to solve here. Like Alexandre says what would an mfd_driver actually buy us?
Well, there is the minor issue of name collision eg /sys/bus/XX/devices/* must list all devices in the system with no collisions.
The owner of the bus is supposed to define the stable naming scheme and all the devices are supposed to follow it. platform doesn't have this:
$ ls /sys/bus/platform/devices/ ACPI000C:00 dell-smbios.0 'Fixed MDIO bus.0' INT33A1:00 microcode PNP0C04:00 PNP0C0B:03 PNP0C14:00 alarmtimer.0.auto dell-smbios.1 GHES.0 intel_rapl_msr.0 MSFT0101:00 PNP0C0B:00 PNP0C0B:04 PNP0C14:01 coretemp.0 efi-framebuffer.0 GHES.1 iTCO_wdt pcspkr PNP0C0B:01 PNP0C0C:00 reg-dummy dcdbas eisa.0 INT0800:00 kgdboc PNP0103:00 PNP0C0B:02 PNP0C0E:00 serial8250
Why are ACPI names in here? It looks like "because platform drivers were used to bind to ACPI devices"
I think we decided that the ACPI namespace was sufficiently divergent from our general conventions that we could just safely use the string, ICBW though.
I have some bad news for you about the hardware description problem space. Among other things we have a bunch of platform devices that don't have any resources exposed through the resource API but are still things like chips on a board, doing some combination of exposing resources for other devices (eg, a fixed voltage regulator) and consuming things like clocks or GPIOs that don't appear in the resource API.
So in these cases how do I use the generic platform bus API to find the GPIOs, regulators, and so on to connect with?
If drivers take a platform device and immediately covert it to an OF object and use OF APIs to find those connections then it really *never* was a platform device in the first place and coding an OF driver as platform is an abuse.
Those APIs all take a struct device for lookup so it's the same call for looking things up regardless of the bus the device is on or what firmware the system is using - where there are firmware specific lookup functions they're generally historical and shouldn't be used for new code. It's generally something in the form
api_type *api_get(struct device *dev, const char *name);
with the strings being defined according to something in the hardware spec so there's a good chance of them working generically (and realistically it's only DT that's actually putting these names in firmware, otherwise it's just board files that we totally control, so this really is fine).
A decent step would be to accept that 'platform_device' is something weird and special and split its bus_type. Only devices created direclty in C code should be on the platform_bus, OF/ACPI/etc should all be on their own bus_types, even though they all use the same 'struct platform_bus'
...and then do the same thing for every other bus with firmware bindings. If it's about the firmware interfaces it really isn't a platform bus specific thing. It's not clear to me if that's what it is though or if this is just some tangent.
that but it is not clear what the upside of doing that would be, especially given the amount of upheval it would generate and the classification issues that will inevitably result.
Well, I think the upside for existing is very small, but I would like to see a shared idea about how to answer questions like 'when should I use a existing device type' and 'when should I make a new device type'.
Yes, very much so.
On Mon, Jan 04, 2021 at 09:19:30PM +0000, Mark Brown wrote:
Regardless of the shortcut to make everything a struct platform_device, I think it was a mistake to put OF devices on platform_bus. Those should have remained on some of_bus even if they
Like I keep saying the same thing applies to all non-enumerable buses - exactly the same considerations exist for all the other buses like I2C (including the ACPI naming issue you mention below), and for that matter with enumerable buses which can have firmware info.
And most busses do already have their own bus type. ACPI, I2C, PCI, etc. It is just a few that have been squished into platform, notably OF.
are represented by struct platform_device and fiddling in the core done to make that work OK.
What exactly is the fiddling in the core here, I'm a bit unclear?
I'm not sure, but I bet there is a small fall out to making bus_type not 1:1 with the struct device type.. Would have to attempt it to see
This feels like a good conference topic someday..
We should have this discussion *before* we get too far along with trying to implement things, we should at least have some idea where we want to head there.
Well, auxillary bus is clearly following the original bus model intention with a dedicated bus type with a controlled naming scheme. The debate here seems to be "what about platform bus" and "what to do with mfd"?
Those APIs all take a struct device for lookup so it's the same call for looking things up regardless of the bus the device is on or what firmware the system is using - where there are firmware specific lookup functions they're generally historical and shouldn't be used for new code. It's generally something in the form
api_type *api_get(struct device *dev, const char *name);
Well, that is a nice improvement since a few years back when I last worked on this stuff.
But now it begs the question, why not push harder to make 'struct device' the generic universal access point and add some resource_get() API along these lines so even a platform_device * isn't needed?
Then the path seems much clearer, add a multi-bus-type device_driver that has a probe(struct device *) and uses the 'universal api_get()' style interface to find the generic 'resources'.
The actual bus types and bus structs can then be split properly without the boilerplate that caused them all to be merged to platform, even PCI could be substantially merged like this.
Bonus points to replace the open coded method disptach:
int gpiod_count(struct device *dev, const char *con_id) { int count = -ENOENT;
if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) count = of_gpio_get_count(dev, con_id); else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) count = acpi_gpio_count(dev, con_id);
if (count < 0) count = platform_gpio_count(dev, con_id);
With an actual bus specific virtual function:
return dev->bus->gpio_count(dev);
...and then do the same thing for every other bus with firmware bindings. If it's about the firmware interfaces it really isn't a platform bus specific thing. It's not clear to me if that's what it is though or if this is just some tangent.
It should be split up based on the unique naming scheme and any bus specific API elements - like raw access to ACPI or OF data or what have you for other FW bus types.
Jason
On Mon, Jan 4, 2021 at 4:14 PM Jason Gunthorpe jgg@nvidia.com wrote:
On Mon, Jan 04, 2021 at 09:19:30PM +0000, Mark Brown wrote:
Regardless of the shortcut to make everything a struct platform_device, I think it was a mistake to put OF devices on platform_bus. Those should have remained on some of_bus even if they
Like I keep saying the same thing applies to all non-enumerable buses - exactly the same considerations exist for all the other buses like I2C (including the ACPI naming issue you mention below), and for that matter with enumerable buses which can have firmware info.
And most busses do already have their own bus type. ACPI, I2C, PCI, etc. It is just a few that have been squished into platform, notably OF.
I'll note that ACPI is an outlier that places devices on 2 buses, where new acpi_driver instances are discouraged [1] in favor of platform_drivers. ACPI scan handlers are awkwardly integrated into the Linux device model.
So while I agree with sentiment that an "ACPI bus" should theoretically stand on its own there is legacy to unwind.
I only bring that up to keep the focus on how to organize drivers going forward, because trying to map some of these arguments backwards runs into difficulties.
[1]: http://lore.kernel.org/r/CAJZ5v0j_ReK3AGDdw7fLvmw_7knECCg2U_huKgJzQeLCy8smug...
are represented by struct platform_device and fiddling in the core done to make that work OK.
What exactly is the fiddling in the core here, I'm a bit unclear?
I'm not sure, but I bet there is a small fall out to making bus_type not 1:1 with the struct device type.. Would have to attempt it to see
This feels like a good conference topic someday..
We should have this discussion *before* we get too far along with trying to implement things, we should at least have some idea where we want to head there.
Well, auxillary bus is clearly following the original bus model intention with a dedicated bus type with a controlled naming scheme. The debate here seems to be "what about platform bus" and "what to do with mfd"?
Those APIs all take a struct device for lookup so it's the same call for looking things up regardless of the bus the device is on or what firmware the system is using - where there are firmware specific lookup functions they're generally historical and shouldn't be used for new code. It's generally something in the form
api_type *api_get(struct device *dev, const char *name);Well, that is a nice improvement since a few years back when I last worked on this stuff.
But now it begs the question, why not push harder to make 'struct device' the generic universal access point and add some resource_get() API along these lines so even a platform_device * isn't needed?
Then the path seems much clearer, add a multi-bus-type device_driver that has a probe(struct device *) and uses the 'universal api_get()' style interface to find the generic 'resources'.
The actual bus types and bus structs can then be split properly without the boilerplate that caused them all to be merged to platform, even PCI could be substantially merged like this.
Bonus points to replace the open coded method disptach:
int gpiod_count(struct device *dev, const char *con_id) { int count = -ENOENT;
if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) count = of_gpio_get_count(dev, con_id); else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) count = acpi_gpio_count(dev, con_id); if (count < 0) count = platform_gpio_count(dev, con_id);With an actual bus specific virtual function:
return dev->bus->gpio_count(dev);...and then do the same thing for every other bus with firmware bindings. If it's about the firmware interfaces it really isn't a platform bus specific thing. It's not clear to me if that's what it is though or if this is just some tangent.
It should be split up based on the unique naming scheme and any bus specific API elements - like raw access to ACPI or OF data or what have you for other FW bus types.
I agree that the pendulum may have swung too far towards "reuse existing bus_type", and auxiliary-bus unwinds some of that, but does the bus_type really want to be an indirection for driver apis outside of bus-specific operations?
On Mon, Jan 04, 2021 at 04:51:51PM -0800, Dan Williams wrote:
On Mon, Jan 4, 2021 at 4:14 PM Jason Gunthorpe jgg@nvidia.com wrote:
On Mon, Jan 04, 2021 at 09:19:30PM +0000, Mark Brown wrote:
Regardless of the shortcut to make everything a struct platform_device, I think it was a mistake to put OF devices on platform_bus. Those should have remained on some of_bus even if they
Like I keep saying the same thing applies to all non-enumerable buses - exactly the same considerations exist for all the other buses like I2C (including the ACPI naming issue you mention below), and for that matter with enumerable buses which can have firmware info.
And most busses do already have their own bus type. ACPI, I2C, PCI, etc. It is just a few that have been squished into platform, notably OF.
I'll note that ACPI is an outlier that places devices on 2 buses, where new acpi_driver instances are discouraged [1] in favor of platform_drivers. ACPI scan handlers are awkwardly integrated into the Linux device model.
So while I agree with sentiment that an "ACPI bus" should theoretically stand on its own there is legacy to unwind.
I only bring that up to keep the focus on how to organize drivers going forward, because trying to map some of these arguments backwards runs into difficulties.
Well, this is the exact kind of thing I think we are talking about here..
It should be split up based on the unique naming scheme and any bus specific API elements - like raw access to ACPI or OF data or what have you for other FW bus types.
I agree that the pendulum may have swung too far towards "reuse existing bus_type", and auxiliary-bus unwinds some of that, but does the bus_type really want to be an indirection for driver apis outside of bus-specific operations?
If the bus is the "enumeration entity" and we define that things like name, resources, gpio's, regulators, etc are a generic part of what is enumerated, then it makes sense that the bus would have methods to handle those things too.
In other words, the only way to learn what GPIO 'resource' is to ask the enumeration mechnism that is providing the bus. If the enumeration and bus are 1:1 then you can use a function pointer on the bus type instead of open coding a dispatch based on an indirect indication.
Jason
On Mon, Jan 4, 2021 at 5:53 PM Jason Gunthorpe jgg@nvidia.com wrote:
On Mon, Jan 04, 2021 at 04:51:51PM -0800, Dan Williams wrote:
On Mon, Jan 4, 2021 at 4:14 PM Jason Gunthorpe jgg@nvidia.com wrote:
On Mon, Jan 04, 2021 at 09:19:30PM +0000, Mark Brown wrote:
Regardless of the shortcut to make everything a struct platform_device, I think it was a mistake to put OF devices on platform_bus. Those should have remained on some of_bus even if they
Like I keep saying the same thing applies to all non-enumerable buses - exactly the same considerations exist for all the other buses like I2C (including the ACPI naming issue you mention below), and for that matter with enumerable buses which can have firmware info.
And most busses do already have their own bus type. ACPI, I2C, PCI, etc. It is just a few that have been squished into platform, notably OF.
I'll note that ACPI is an outlier that places devices on 2 buses, where new acpi_driver instances are discouraged [1] in favor of platform_drivers. ACPI scan handlers are awkwardly integrated into the Linux device model.
So while I agree with sentiment that an "ACPI bus" should theoretically stand on its own there is legacy to unwind.
I only bring that up to keep the focus on how to organize drivers going forward, because trying to map some of these arguments backwards runs into difficulties.
Well, this is the exact kind of thing I think we are talking about here..
It should be split up based on the unique naming scheme and any bus specific API elements - like raw access to ACPI or OF data or what have you for other FW bus types.
I agree that the pendulum may have swung too far towards "reuse existing bus_type", and auxiliary-bus unwinds some of that, but does the bus_type really want to be an indirection for driver apis outside of bus-specific operations?
If the bus is the "enumeration entity" and we define that things like name, resources, gpio's, regulators, etc are a generic part of what is enumerated, then it makes sense that the bus would have methods to handle those things too.
In other words, the only way to learn what GPIO 'resource' is to ask the enumeration mechnism that is providing the bus. If the enumeration and bus are 1:1 then you can use a function pointer on the bus type instead of open coding a dispatch based on an indirect indication.
I get that, but I'm fearing a gigantic bus_ops structure that has narrow helpers like ->gpio_count() that mean nothing to the many other clients of the bus. Maybe I'm overestimating the pressure there will be to widen the ops structure at the bus level.
On Mon, Jan 04, 2021 at 07:12:47PM -0800, Dan Williams wrote:
I get that, but I'm fearing a gigantic bus_ops structure that has narrow helpers like ->gpio_count() that mean nothing to the many other clients of the bus. Maybe I'm overestimating the pressure there will be to widen the ops structure at the bus level.
If we want a 'universal device' then that stuff must live someplace.. Open coding the dispatch as is today is also not the end of the world, just seeing that is just usually a sign something is not ideal with the object model.
Jason
On Mon, Jan 04, 2021 at 08:13:41PM -0400, Jason Gunthorpe wrote:
On Mon, Jan 04, 2021 at 09:19:30PM +0000, Mark Brown wrote:
Like I keep saying the same thing applies to all non-enumerable buses - exactly the same considerations exist for all the other buses like I2C (including the ACPI naming issue you mention below), and for that matter with enumerable buses which can have firmware info.
And most busses do already have their own bus type. ACPI, I2C, PCI, etc. It is just a few that have been squished into platform, notably OF.
You're missing the point there. I2C is enumerated by firmware in exactly the same way as the platform bus is, it's not discoverable from the hardware (and similarly for a bunch of other buses). If we were to say that we need separate device types for platform devices enumerated using firmware then by analogy we should do the same for devices on these other buses that happen to be enumerated by firmware.
I'm not convinced this is actually the design that's being pushed by Greg here, to the extent anything is being actively pushed.
But now it begs the question, why not push harder to make 'struct device' the generic universal access point and add some resource_get() API along these lines so even a platform_device * isn't needed?
We still need a struct device of some kind so the discussions about exactly which bus type one is supposed to use in which circumstances remain given that you're not supposed to have raw struct devices.
Then the path seems much clearer, add a multi-bus-type device_driver that has a probe(struct device *) and uses the 'universal api_get()' style interface to find the generic 'resources'.
That's exactly how things like mixed I2C/SPI devices work at present, given that they can usually use regmap to hide register I/O.
int gpiod_count(struct device *dev, const char *con_id) { int count = -ENOENT;
if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) count = of_gpio_get_count(dev, con_id); else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) count = acpi_gpio_count(dev, con_id);
if (count < 0) count = platform_gpio_count(dev, con_id);
With an actual bus specific virtual function:
return dev->bus->gpio_count(dev);
That won't work, you might have a mix of enumeration types for a given bus type in a single system so you'd need to do this per device. It's also not clear to me that it is useful to spread things out like this, it makes it more hassle to add new APIs since you have to change core code.
On Tue, Jan 05, 2021 at 01:42:56PM +0000, Mark Brown wrote:
On Mon, Jan 04, 2021 at 08:13:41PM -0400, Jason Gunthorpe wrote:
On Mon, Jan 04, 2021 at 09:19:30PM +0000, Mark Brown wrote:
Like I keep saying the same thing applies to all non-enumerable buses - exactly the same considerations exist for all the other buses like I2C (including the ACPI naming issue you mention below), and for that matter with enumerable buses which can have firmware info.
And most busses do already have their own bus type. ACPI, I2C, PCI, etc. It is just a few that have been squished into platform, notably OF.
You're missing the point there. I2C is enumerated by firmware in exactly the same way as the platform bus is, it's not discoverable from the hardware (and similarly for a bunch of other buses). If we were to say that we need separate device types for platform devices enumerated using firmware then by analogy we should do the same for devices on these other buses that happen to be enumerated by firmware.
No, I understand how I2C works and I think it is fine as is because the enumeration outcome is all standard. You always end up with a stable I2C device address (the name) and you always end up with the I2C programming API. So it doesn't matter how I2C gets enumerated, it is always an I2C device.
PCI does this too, pci_device gets crossed over to the DT data, but it is still a pci_device.
I see a big difference between attaching FW data to an existing subsystem's HW centric bus (and possibly guiding enumeration of a HW bus from FW data) and directly creating struct devices based on FW data unconnected to any existing subsystem.
The latter case is where the enumerating FW should stay on its own bus_type because there is no standardized subsystem bus providing an API or naming rules, so the FW type should provide those rules instead.
With an actual bus specific virtual function:
return dev->bus->gpio_count(dev);That won't work, you might have a mix of enumeration types for a given bus type in a single system so you'd need to do this per device.
I'm being very general here, probably what we want is a little more formal 'fw_type' concept, so a device is on a bus and also has a FW attachment which can provide this other data.
Jason
On Tue, Jan 05, 2021 at 10:36:27AM -0400, Jason Gunthorpe wrote:
On Tue, Jan 05, 2021 at 01:42:56PM +0000, Mark Brown wrote:
You're missing the point there. I2C is enumerated by firmware in exactly the same way as the platform bus is, it's not discoverable from the hardware (and similarly for a bunch of other buses). If we were to
No, I understand how I2C works and I think it is fine as is because the enumeration outcome is all standard. You always end up with a stable I2C device address (the name) and you always end up with the I2C programming API. So it doesn't matter how I2C gets enumerated, it is always an I2C device.
I don't follow this logic at all, sorry - whatever the platonic ideal of what a platform device actually turns out to be when we get down to using the hardware it's the same hardware which we interact with in the same way no matter how we figured out that it was present.
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
Here is now a signed tag for everyone else to pull from and build on top of, for 5.11-rc1, that includes this patch, and the 3 others I sent in this series.
Please let me know if anyone has any problems with this tag. I'll keep it around until 5.11-rc1 is released, after which it doesn't make any sense to be there.
thanks,
greg k-h
---------------
The following changes since commit f8394f232b1eab649ce2df5c5f15b0e528c92091:
Linux 5.10-rc3 (2020-11-08 16:10:16 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git tags/auxbus-5.11-rc1
for you to fetch changes up to 0d2bf11a6b3e275a526b8d42d8d4a3a6067cf953:
driver core: auxiliary bus: minor coding style tweaks (2020-12-04 13:30:59 +0100)
---------------------------------------------------------------- Auxiliary Bus support tag for 5.11-rc1
This is a signed tag for other subsystems to be able to pull in the auxiliary bus support into their trees for the 5.11-rc1 merge.
Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
---------------------------------------------------------------- Dave Ertman (1): Add auxiliary bus support
Greg Kroah-Hartman (3): driver core: auxiliary bus: move slab.h from include file driver core: auxiliary bus: make remove function return void driver core: auxiliary bus: minor coding style tweaks
Documentation/driver-api/auxiliary_bus.rst | 234 ++++++++++++++++++++++++ Documentation/driver-api/index.rst | 1 + drivers/base/Kconfig | 3 + drivers/base/Makefile | 1 + drivers/base/auxiliary.c | 274 +++++++++++++++++++++++++++++ include/linux/auxiliary_bus.h | 77 ++++++++ include/linux/mod_devicetable.h | 8 + scripts/mod/devicetable-offsets.c | 3 + scripts/mod/file2alias.c | 8 + 9 files changed, 609 insertions(+) create mode 100644 Documentation/driver-api/auxiliary_bus.rst create mode 100644 drivers/base/auxiliary.c create mode 100644 include/linux/auxiliary_bus.h
On Fri, Dec 04, 2020 at 01:35:05PM +0100, Greg KH wrote:
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
Here is now a signed tag for everyone else to pull from and build on top of, for 5.11-rc1, that includes this patch, and the 3 others I sent in this series.
Please let me know if anyone has any problems with this tag. I'll keep it around until 5.11-rc1 is released, after which it doesn't make any sense to be there.
Thanks, pulled to mlx5-next
Jason, Jakob,
Can you please pull that mlx5-next branch to your trees? git://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git
Thanks
On Fri, 4 Dec 2020 14:54:55 +0200 Leon Romanovsky wrote:
Thanks, pulled to mlx5-next
Jason, Jakob,
Can you please pull that mlx5-next branch to your trees? git://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git
Could you post a PR with a proper description and so on?
Thanks!
On Fri, 2020-12-04 at 08:25 -0800, Jakub Kicinski wrote:
On Fri, 4 Dec 2020 14:54:55 +0200 Leon Romanovsky wrote:
Thanks, pulled to mlx5-next
Jason, Jakob,
Can you please pull that mlx5-next branch to your trees? git://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux.git
Could you post a PR with a proper description and so on?
Thanks!
I will do that.
Thanks!
On Fri, 2020-12-04 at 13:35 +0100, Greg KH wrote:
On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote:
From: Dave Ertman david.m.ertman@intel.com
Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. It enables drivers to create an auxiliary_device and bind an auxiliary_driver to it.
The bus supports probe/remove shutdown and suspend/resume callbacks. Each auxiliary_device has a unique string based id; driver binds to an auxiliary_device based on this id through the bus.
Co-developed-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan < ranjani.sridharan@linux.intel.com> Co-developed-by: Fred Oh fred.oh@linux.intel.com Co-developed-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Kiran Patil kiran.patil@intel.com Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com
Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Leon Romanovsky leonro@nvidia.com Signed-off-by: Dave Ertman david.m.ertman@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 Reviewed-by: Martin Habets mhabets@solarflare.com Link: https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@intel.com Signed-off-by: Dan Williams dan.j.williams@intel.com
This patch is "To:" the maintainers that have a pending backlog of driver updates dependent on this facility, and "Cc:" Greg. Greg, I understand you have asked for more time to fully review this and apply it to driver-core.git, likely for v5.12, but please consider Acking it for v5.11 instead. It looks good to me and several other stakeholders. Namely, stakeholders that have pressure building up behind this facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, and later on Compute Express Link.
I will take the blame for the 2 months of silence that made this awkward to take through driver-core.git, but at the same time I do not want to see that communication mistake inconvenience other parties that reasonably thought this was shaping up to land in v5.11.
I am willing to host this version at:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux tags/auxiliary-bus-for-5.11
...for all the independent drivers to have a common commit baseline. It is not there yet pending Greg's Ack.
Here is now a signed tag for everyone else to pull from and build on top of, for 5.11-rc1, that includes this patch, and the 3 others I sent in this series.
Please let me know if anyone has any problems with this tag. I'll keep it around until 5.11-rc1 is released, after which it doesn't make any sense to be there. thanks,
greg k-h
Hi Mark,
Could I request you to please pull from this tag and will we re-submit the SOF changes soon after.
Thanks, Ranjani
On 12/2/20 5:54 PM, Dan Williams wrote:
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 8d7001712062..040be48ce046 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -1,6 +1,9 @@ # SPDX-License-Identifier: GPL-2.0 menu "Generic Driver Options"
+config AUXILIARY_BUS
- bool
config UEVENT_HELPER bool "Support for uevent helper" help
Missing a description and without it does not appear in menuconfig or in the config file.
Could use a blurb in the help as well.
On Sat, Dec 5, 2020 at 4:24 PM David Ahern dsahern@gmail.com wrote:
On 12/2/20 5:54 PM, Dan Williams wrote:
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 8d7001712062..040be48ce046 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -1,6 +1,9 @@ # SPDX-License-Identifier: GPL-2.0 menu "Generic Driver Options"
+config AUXILIARY_BUS
boolconfig UEVENT_HELPER bool "Support for uevent helper" help
Missing a description and without it does not appear in menuconfig or in the config file.
Could use a blurb in the help as well.
It doesn't have a description or help because it is a select-only symbol, but a comment to that effect and a pointer to the documentation would help.
participants (13)
-
Alexandre Belloni -
Dan Williams -
David Ahern -
Greg KH -
Jakub Kicinski -
Jason Gunthorpe -
Jason Gunthorpe -
Lee Jones -
Leon Romanovsky -
Mark Brown -
Parav Pandit -
Ranjani Sridharan -
Saeed Mahameed