| Message ID | 20250218-aux-device-create-helper-v4-1-c3d7dfdea2e6@baylibre.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | driver core: auxiliary bus: add device creation helper | expand |
On Tue, Feb 18, 2025 at 08:29:46PM +0100, Jerome Brunet wrote: > Add helper functions to create a device on the auxiliary bus. > > This is meant for fairly simple usage of the auxiliary bus, to avoid having > the same code repeated in the different drivers. > > Suggested-by: Stephen Boyd <sboyd@kernel.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/base/auxiliary.c | 108 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/auxiliary_bus.h | 17 +++++++ > 2 files changed, 125 insertions(+) > > diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c > index afa4df4c5a3f371b91d8dd8c4325495d32ad1291..a6d46c2759be81a0739f07528d5959c2a76eb8a8 100644 > --- a/drivers/base/auxiliary.c > +++ b/drivers/base/auxiliary.c > @@ -385,6 +385,114 @@ void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv) > } > EXPORT_SYMBOL_GPL(auxiliary_driver_unregister); > > +static void auxiliary_device_release(struct device *dev) > +{ > + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); > + > + kfree(auxdev); > +} > + > +/** > + * auxiliary_device_create - create a device on the auxiliary bus > + * @dev: parent device > + * @modname: module name used to create the auxiliary driver name. > + * @devname: auxiliary bus device name > + * @platform_data: auxiliary bus device platform data > + * @id: auxiliary bus device id > + * > + * Helper to create an auxiliary bus device. > + * The device created matches driver 'modname.devname' on the auxiliary bus. > + */ > +struct auxiliary_device *auxiliary_device_create(struct device *dev, > + const char *modname, > + const char *devname, > + void *platform_data, > + int id) > +{ > + struct auxiliary_device *auxdev; > + int ret; > + > + auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL); > + if (!auxdev) > + return NULL; > + > + auxdev->id = id; > + auxdev->name = devname; > + auxdev->dev.parent = dev; > + auxdev->dev.platform_data = platform_data; > + auxdev->dev.release = auxiliary_device_release; > + device_set_of_node_from_dev(&auxdev->dev, dev); > + > + ret = auxiliary_device_init(auxdev); > + if (ret) { > + kfree(auxdev); > + return NULL; > + } > + > + ret = __auxiliary_device_add(auxdev, modname); > + if (ret) { This loses possible error return values from __auxiliary_device_add(). I'd suggest to return ERR_PTR(ret) here and in the auxiliary_device_init() chunks and ERR_PTR(-ENOMEM) in case of kzalloc() failure. > + /* > + * It may look odd but auxdev should not be freed here. > + * auxiliary_device_uninit() calls device_put() which call > + * the device release function, freeing auxdev. > + */ > + auxiliary_device_uninit(auxdev); > + return NULL; > + } > + > + return auxdev; > +} > +EXPORT_SYMBOL_GPL(auxiliary_device_create); > +
On Wed, Feb 19, 2025 at 11:06:02AM +0200, Dmitry Baryshkov wrote: > On Tue, Feb 18, 2025 at 08:29:46PM +0100, Jerome Brunet wrote: > > Add helper functions to create a device on the auxiliary bus. > > > > This is meant for fairly simple usage of the auxiliary bus, to avoid having > > the same code repeated in the different drivers. > > > > Suggested-by: Stephen Boyd <sboyd@kernel.org> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > --- > > drivers/base/auxiliary.c | 108 ++++++++++++++++++++++++++++++++++++++++++ > > include/linux/auxiliary_bus.h | 17 +++++++ > > 2 files changed, 125 insertions(+) > > > > diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c > > index afa4df4c5a3f371b91d8dd8c4325495d32ad1291..a6d46c2759be81a0739f07528d5959c2a76eb8a8 100644 > > --- a/drivers/base/auxiliary.c > > +++ b/drivers/base/auxiliary.c > > @@ -385,6 +385,114 @@ void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv) > > } > > EXPORT_SYMBOL_GPL(auxiliary_driver_unregister); > > > > +static void auxiliary_device_release(struct device *dev) > > +{ > > + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); > > + > > + kfree(auxdev); > > +} > > + > > +/** > > + * auxiliary_device_create - create a device on the auxiliary bus > > + * @dev: parent device > > + * @modname: module name used to create the auxiliary driver name. > > + * @devname: auxiliary bus device name > > + * @platform_data: auxiliary bus device platform data > > + * @id: auxiliary bus device id > > + * > > + * Helper to create an auxiliary bus device. > > + * The device created matches driver 'modname.devname' on the auxiliary bus. > > + */ > > +struct auxiliary_device *auxiliary_device_create(struct device *dev, > > + const char *modname, > > + const char *devname, > > + void *platform_data, > > + int id) > > +{ > > + struct auxiliary_device *auxdev; > > + int ret; > > + > > + auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL); > > + if (!auxdev) > > + return NULL; > > + > > + auxdev->id = id; > > + auxdev->name = devname; > > + auxdev->dev.parent = dev; > > + auxdev->dev.platform_data = platform_data; > > + auxdev->dev.release = auxiliary_device_release; > > + device_set_of_node_from_dev(&auxdev->dev, dev); > > + > > + ret = auxiliary_device_init(auxdev); > > + if (ret) { > > + kfree(auxdev); > > + return NULL; > > + } > > + > > + ret = __auxiliary_device_add(auxdev, modname); > > + if (ret) { > > This loses possible error return values from __auxiliary_device_add(). Why does that really matter? > I'd suggest to return ERR_PTR(ret) here and in the > auxiliary_device_init() chunks and ERR_PTR(-ENOMEM) in case of kzalloc() > failure. Will the caller do something different based on the error value here? All we care is that this worked or not, the specific error isn't going to matter for device creation like this. thanks, greg k-h
On Wed, Feb 19, 2025 at 11:13:14AM +0100, Greg Kroah-Hartman wrote: > On Wed, Feb 19, 2025 at 11:06:02AM +0200, Dmitry Baryshkov wrote: > > On Tue, Feb 18, 2025 at 08:29:46PM +0100, Jerome Brunet wrote: > > > Add helper functions to create a device on the auxiliary bus. > > > > > > This is meant for fairly simple usage of the auxiliary bus, to avoid having > > > the same code repeated in the different drivers. > > > > > > Suggested-by: Stephen Boyd <sboyd@kernel.org> > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > > --- > > > drivers/base/auxiliary.c | 108 ++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/auxiliary_bus.h | 17 +++++++ > > > 2 files changed, 125 insertions(+) > > > > > > diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c > > > index afa4df4c5a3f371b91d8dd8c4325495d32ad1291..a6d46c2759be81a0739f07528d5959c2a76eb8a8 100644 > > > --- a/drivers/base/auxiliary.c > > > +++ b/drivers/base/auxiliary.c > > > @@ -385,6 +385,114 @@ void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv) > > > } > > > EXPORT_SYMBOL_GPL(auxiliary_driver_unregister); > > > > > > +static void auxiliary_device_release(struct device *dev) > > > +{ > > > + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); > > > + > > > + kfree(auxdev); > > > +} > > > + > > > +/** > > > + * auxiliary_device_create - create a device on the auxiliary bus > > > + * @dev: parent device > > > + * @modname: module name used to create the auxiliary driver name. > > > + * @devname: auxiliary bus device name > > > + * @platform_data: auxiliary bus device platform data > > > + * @id: auxiliary bus device id > > > + * > > > + * Helper to create an auxiliary bus device. > > > + * The device created matches driver 'modname.devname' on the auxiliary bus. > > > + */ > > > +struct auxiliary_device *auxiliary_device_create(struct device *dev, > > > + const char *modname, > > > + const char *devname, > > > + void *platform_data, > > > + int id) > > > +{ > > > + struct auxiliary_device *auxdev; > > > + int ret; > > > + > > > + auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL); > > > + if (!auxdev) > > > + return NULL; > > > + > > > + auxdev->id = id; > > > + auxdev->name = devname; > > > + auxdev->dev.parent = dev; > > > + auxdev->dev.platform_data = platform_data; > > > + auxdev->dev.release = auxiliary_device_release; > > > + device_set_of_node_from_dev(&auxdev->dev, dev); > > > + > > > + ret = auxiliary_device_init(auxdev); > > > + if (ret) { > > > + kfree(auxdev); > > > + return NULL; > > > + } > > > + > > > + ret = __auxiliary_device_add(auxdev, modname); > > > + if (ret) { > > > > This loses possible error return values from __auxiliary_device_add(). > > Why does that really matter? At the very least the caller (or caller of a caller) can call dev_err_probe() or dev_err("%pe"). With the current implementation as everybody maps NULL to -ENOMEM the error message will be cryptic. Or just having a cryptic value in the logs. > > I'd suggest to return ERR_PTR(ret) here and in the > > auxiliary_device_init() chunks and ERR_PTR(-ENOMEM) in case of kzalloc() > > failure. > > Will the caller do something different based on the error value here? > All we care is that this worked or not, the specific error isn't going > to matter for device creation like this. The caller might not, the developer might.
On Wed, Feb 19, 2025 at 02:08:22PM +0200, Dmitry Baryshkov wrote: > On Wed, Feb 19, 2025 at 11:13:14AM +0100, Greg Kroah-Hartman wrote: > > On Wed, Feb 19, 2025 at 11:06:02AM +0200, Dmitry Baryshkov wrote: > > > On Tue, Feb 18, 2025 at 08:29:46PM +0100, Jerome Brunet wrote: > > > > Add helper functions to create a device on the auxiliary bus. > > > > > > > > This is meant for fairly simple usage of the auxiliary bus, to avoid having > > > > the same code repeated in the different drivers. > > > > > > > > Suggested-by: Stephen Boyd <sboyd@kernel.org> > > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > > > --- > > > > drivers/base/auxiliary.c | 108 ++++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/auxiliary_bus.h | 17 +++++++ > > > > 2 files changed, 125 insertions(+) > > > > > > > > diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c > > > > index afa4df4c5a3f371b91d8dd8c4325495d32ad1291..a6d46c2759be81a0739f07528d5959c2a76eb8a8 100644 > > > > --- a/drivers/base/auxiliary.c > > > > +++ b/drivers/base/auxiliary.c > > > > @@ -385,6 +385,114 @@ void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv) > > > > } > > > > EXPORT_SYMBOL_GPL(auxiliary_driver_unregister); > > > > > > > > +static void auxiliary_device_release(struct device *dev) > > > > +{ > > > > + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); > > > > + > > > > + kfree(auxdev); > > > > +} > > > > + > > > > +/** > > > > + * auxiliary_device_create - create a device on the auxiliary bus > > > > + * @dev: parent device > > > > + * @modname: module name used to create the auxiliary driver name. > > > > + * @devname: auxiliary bus device name > > > > + * @platform_data: auxiliary bus device platform data > > > > + * @id: auxiliary bus device id > > > > + * > > > > + * Helper to create an auxiliary bus device. > > > > + * The device created matches driver 'modname.devname' on the auxiliary bus. > > > > + */ > > > > +struct auxiliary_device *auxiliary_device_create(struct device *dev, > > > > + const char *modname, > > > > + const char *devname, > > > > + void *platform_data, > > > > + int id) > > > > +{ > > > > + struct auxiliary_device *auxdev; > > > > + int ret; > > > > + > > > > + auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL); > > > > + if (!auxdev) > > > > + return NULL; > > > > + > > > > + auxdev->id = id; > > > > + auxdev->name = devname; > > > > + auxdev->dev.parent = dev; > > > > + auxdev->dev.platform_data = platform_data; > > > > + auxdev->dev.release = auxiliary_device_release; > > > > + device_set_of_node_from_dev(&auxdev->dev, dev); > > > > + > > > > + ret = auxiliary_device_init(auxdev); > > > > + if (ret) { > > > > + kfree(auxdev); > > > > + return NULL; > > > > + } > > > > + > > > > + ret = __auxiliary_device_add(auxdev, modname); > > > > + if (ret) { > > > > > > This loses possible error return values from __auxiliary_device_add(). > > > > Why does that really matter? > > At the very least the caller (or caller of a caller) can call > dev_err_probe() or dev_err("%pe"). With the current implementation as > everybody maps NULL to -ENOMEM the error message will be cryptic. > > Or just having a cryptic value in the logs. So all you can get here could be: -ENOMEM - memory couldn't be allocated somewhere -EINVAL - wrong parameters sent to auxiliary_device_init() or __auxiliary_device_add() -EEXIST - duplicate name And if -EEXIST happens, you will get a kernel log splat from sysfs showing you that something went wrong. So while I understand the need to be specific here in reporting the exact error, I fail to understand how it really matters at all. A driver writer really only wants to know "did it work?" and have a simple way to test it. IS_ERR_OR_NULL() and then getting the error using PTR_ERR() is rough and feels like boilerplate code that everyone gets wrong (how many times do people accidentally only check for NULL?). Anyway, I'm for simple apis, and NULL or valid pointer seems simple to me. thanks, greg k-h
On Tue, Feb 18, 2025 at 08:29:46PM +0100, Jerome Brunet wrote: > Add helper functions to create a device on the auxiliary bus. > > This is meant for fairly simple usage of the auxiliary bus, to avoid having > the same code repeated in the different drivers. > > Suggested-by: Stephen Boyd <sboyd@kernel.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Jerome Brunet wrote: > Add helper functions to create a device on the auxiliary bus. > > This is meant for fairly simple usage of the auxiliary bus, to avoid having > the same code repeated in the different drivers. > > Suggested-by: Stephen Boyd <sboyd@kernel.org> > Cc: Arnd Bergmann <arnd@arndb.de> Reviewed-by: Ira Weiny <ira.weiny@intel.com> [snip]
diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c index afa4df4c5a3f371b91d8dd8c4325495d32ad1291..a6d46c2759be81a0739f07528d5959c2a76eb8a8 100644 --- a/drivers/base/auxiliary.c +++ b/drivers/base/auxiliary.c @@ -385,6 +385,114 @@ void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv) } EXPORT_SYMBOL_GPL(auxiliary_driver_unregister); +static void auxiliary_device_release(struct device *dev) +{ + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); + + kfree(auxdev); +} + +/** + * auxiliary_device_create - create a device on the auxiliary bus + * @dev: parent device + * @modname: module name used to create the auxiliary driver name. + * @devname: auxiliary bus device name + * @platform_data: auxiliary bus device platform data + * @id: auxiliary bus device id + * + * Helper to create an auxiliary bus device. + * The device created matches driver 'modname.devname' on the auxiliary bus. + */ +struct auxiliary_device *auxiliary_device_create(struct device *dev, + const char *modname, + const char *devname, + void *platform_data, + int id) +{ + struct auxiliary_device *auxdev; + int ret; + + auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL); + if (!auxdev) + return NULL; + + auxdev->id = id; + auxdev->name = devname; + auxdev->dev.parent = dev; + auxdev->dev.platform_data = platform_data; + auxdev->dev.release = auxiliary_device_release; + device_set_of_node_from_dev(&auxdev->dev, dev); + + ret = auxiliary_device_init(auxdev); + if (ret) { + kfree(auxdev); + return NULL; + } + + ret = __auxiliary_device_add(auxdev, modname); + if (ret) { + /* + * It may look odd but auxdev should not be freed here. + * auxiliary_device_uninit() calls device_put() which call + * the device release function, freeing auxdev. + */ + auxiliary_device_uninit(auxdev); + return NULL; + } + + return auxdev; +} +EXPORT_SYMBOL_GPL(auxiliary_device_create); + +/** + * auxiliary_device_destroy - remove an auxiliary device + * @auxdev: pointer to the auxdev to be removed + * + * Helper to remove an auxiliary device created with + * auxiliary_device_create() + */ +void auxiliary_device_destroy(void *auxdev) +{ + struct auxiliary_device *_auxdev = auxdev; + + auxiliary_device_delete(_auxdev); + auxiliary_device_uninit(_auxdev); +} +EXPORT_SYMBOL_GPL(auxiliary_device_destroy); + +/** + * __devm_auxiliary_device_create - create a managed device on the auxiliary bus + * @dev: parent device + * @modname: module name used to create the auxiliary driver name. + * @devname: auxiliary bus device name + * @platform_data: auxiliary bus device platform data + * @id: auxiliary bus device id + * + * Device managed helper to create an auxiliary bus device. + * The device created matches driver 'modname.devname' on the auxiliary bus. + */ +struct auxiliary_device *__devm_auxiliary_device_create(struct device *dev, + const char *modname, + const char *devname, + void *platform_data, + int id) +{ + struct auxiliary_device *auxdev; + int ret; + + auxdev = auxiliary_device_create(dev, modname, devname, platform_data, id); + if (IS_ERR(auxdev)) + return auxdev; + + ret = devm_add_action_or_reset(dev, auxiliary_device_destroy, + auxdev); + if (ret) + return ERR_PTR(ret); + + return auxdev; +} +EXPORT_SYMBOL_GPL(__devm_auxiliary_device_create); + void __init auxiliary_bus_init(void) { WARN_ON(bus_register(&auxiliary_bus_type)); diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h index 65dd7f15437474468acf0e28f6932a7ff2cfff2c..4086afd0cc6b96084c190f24acc304cde5d1749a 100644 --- a/include/linux/auxiliary_bus.h +++ b/include/linux/auxiliary_bus.h @@ -254,6 +254,23 @@ int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct module * void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv); +struct auxiliary_device *auxiliary_device_create(struct device *dev, + const char *modname, + const char *devname, + void *platform_data, + int id); +void auxiliary_device_destroy(void *auxdev); + +struct auxiliary_device *__devm_auxiliary_device_create(struct device *dev, + const char *modname, + const char *devname, + void *platform_data, + int id); + +#define devm_auxiliary_device_create(dev, devname, platform_data) \ + __devm_auxiliary_device_create(dev, KBUILD_MODNAME, devname, \ + platform_data, 0) + /** * module_auxiliary_driver() - Helper macro for registering an auxiliary driver * @__auxiliary_driver: auxiliary driver struct
Add helper functions to create a device on the auxiliary bus. This is meant for fairly simple usage of the auxiliary bus, to avoid having the same code repeated in the different drivers. Suggested-by: Stephen Boyd <sboyd@kernel.org> Cc: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/base/auxiliary.c | 108 ++++++++++++++++++++++++++++++++++++++++++ include/linux/auxiliary_bus.h | 17 +++++++ 2 files changed, 125 insertions(+)