Re: [PATCH v3 1/7] driver core: auxiliary bus: add device creation helpers

Greg Kroah-Hartman
2025-02-15T06:56:07Z

On Fri, Feb 14, 2025 at 07:16:30PM +0100, Jerome Brunet wrote:
> On Fri 14 Feb 2025 at 17:33, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>
> > On Tue, Feb 11, 2025 at 06:27:58PM +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 | 88 +++++++++++++++++++++++++++++++++++++++++++
> >> include/linux/auxiliary_bus.h | 10 +++++
> >> 2 files changed, 98 insertions(+)
> >
> > I like the idea, see much the same of what I recently did for the "faux"
> > bus here:
> > https://lore.kernel.org/all/2025021023-sandstorm-precise-9f5d@gregkh/
>
> Reading this, I'm getting the feeling that some (most?) simple auxiliary
> driver might be better off migrating to "faux", instead of what I'm
> proposing here ? Is this what you are suggesting ?

For any that do not actually talk to any real hardware (i.e. they are
NOT sharing resources with a parent device), then yes, they should. I
was also trying to point out that "simple" apis like what you created
here are a good thing in my opinion, I like it!

> Few Q:
> Is there some sort of 'platform_data' (sorry for the lack of a better
> term, no provocation intended ;) ) ... it there a
> simple way to pass an arbitrary struct to the created device with 'faux' ?

There are at least 2 ways to do this:
- embed a faux_device inside a larger structure and then do a
container_of() in any sysfs callback to get to your real structure
- in a provided probe() callback, set the driverdata field with a call
to faux_device_set_drvdata()

> The difference between aux and faux I'm seeing it that aux seems to
> decouple things a bit more. The only thing aux needs is a module name to
> pop something up, while faux needs a reference to the ops instead.

aux is needed for when you want multiple drivers to be bound to the same
hardware resource and need some way to share all of that. faux is used
for "fake" devices where you just need a struct device in the /sys/ tree
to be used for "something" or as a parent device for something else.
See the examples in the above patch series where I convert many
different types of drivers over to use faux.

> I can see the appeal to use aux for maintainers trying to decouple
> different subsystems.

Again aux is needed for "sharing" a real device. faux is there for fake
ones that people previously were using platform devices for.

> > Some review comments:
> >
> >> diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
> >> index afa4df4c5a3f371b91d8dd8c4325495d32ad1291..0f697c9c243dc9a50498a52362806db594345faf 100644
> >> --- a/drivers/base/auxiliary.c
> >> +++ b/drivers/base/auxiliary.c
> >> @@ -385,6 +385,94 @@ 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);
> >> +}
> >> +
> >> +static struct auxiliary_device *auxiliary_device_create(struct device *dev,
> >> + const char *modname,
> >> + const char *devname,
> >> + void *platform_data,
> >
> > Can you have the caller set the platform_data if they need/want it after
> > the device is created? Or do you need that in the probe callback?
>
> My assumption was that it is needed in probe, but I guess that entirely
> depends on the driver. If that was ever needed, it could be added later
> I think.
>
> >
> > And can't this be a global function too for those that don't want to
> > deal with devm stuff?
>
> There was a note about that in the cover-letter of the v1 but I did not
> repeat it after.
>
> It can be exported but I had no use for it so I thought It was better not
> export it until it was actually needed. I really do not have a strong
> preference over this.
>
> >
> >> + int id)
> >> +{
> >> + struct auxiliary_device *auxdev;
> >> + int ret;
> >> +
> >> + auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL);
> >> + if (!auxdev)
> >> + return ERR_PTR(-ENOMEM);
> >
> > Ick, who cares what the error value really is? Why not just do NULL or
> > a valid pointer? That makes the caller much simpler to handle, right?
> >
>
> Sure why not
>
> >> +
> >> + 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);
> >
> > Only way this will fail is if you forgot to set parent or a valid name.
> > So why not check for devname being non-NULL above this?
>
> If auxiliary_device_init() ever changes it would be easy to forget to
> update that and lead to something nasty to debug, don't you think ?

Yes, this is being more defensive, I take back my objection, thanks.

> >> + if (ret) {
> >> + kfree(auxdev);
> >> + return ERR_PTR(ret);
> >> + }
> >> +
> >> + ret = __auxiliary_device_add(auxdev, modname);
> >> + if (ret) {
> >> + /*
> >> + * NOTE: 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);
> >
> > Yes it is odd, are you SURE you should be calling device_del() on the
> > device if this fails? auxiliary_device_uninit(), makes sense so why not
> > just call that here?
>
> I'm confused ... I am call auxiliary_device_uninit() here. What do you
> mean ?

Oh wow, I got this wrong, sorry, I was thinking you were calling
auxiliary_device_destroy(). Nevermind, ugh, it was a long day...

thanks,

greg k-h