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 <sb...@kernel.org>
> > > > Cc: Arnd Bergmann <a...@arndb.de>
> > > > Signed-off-by: Jerome Brunet <jbru...@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
