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.
> > 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.
--
With best wishes
Dmitry
