| Message ID | 20250211-aux-device-create-helper-v3-1-7edb50524909@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | driver core: auxiliary bus: add device creation helper | expand |
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/ 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? And can't this be a global function too for those that don't want to deal with devm stuff? > + 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? > + > + 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 (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? > + return ERR_PTR(ret); > + } > + > + return auxdev; > +} > + > +static void auxiliary_device_destroy(void *_auxdev) > +{ > + struct auxiliary_device *auxdev = _auxdev; > + > + auxiliary_device_delete(auxdev); > + auxiliary_device_uninit(auxdev); > +} > + > +/** > + * __devm_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 > + * > + * Device managed helper to create an auxiliary bus device. > + * The device create 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); Oh this is going to be messy, but I trust that callers know what they are doing here. Good luck! :) thanks, greg k-h
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 ? 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' ? 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. I can see the appeal to use aux for maintainers trying to decouple different subsystems. > > 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 ? If you are OK with this, I could update in this direction. > >> + 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 ? > >> + return ERR_PTR(ret); >> + } >> + >> + return auxdev; >> +} >> + >> +static void auxiliary_device_destroy(void *_auxdev) >> +{ >> + struct auxiliary_device *auxdev = _auxdev; >> + >> + auxiliary_device_delete(auxdev); >> + auxiliary_device_uninit(auxdev); >> +} >> + >> +/** >> + * __devm_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 >> + * >> + * Device managed helper to create an auxiliary bus device. >> + * The device create 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); > > Oh this is going to be messy, but I trust that callers know what they > are doing here. Good luck! :) > > thanks, > > greg k-h
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
On Sat 15 Feb 2025 at 07:53, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: [...] >> >> > >> >> + 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 I have tried the 'NULL or valid' approach. In the consumers, which mostly return an integer from their various init function, I got this weird to come up with one from NULL. EINVAL, ENOMEM, etc ... can't really pick one. It is actually easier to pass something along. >> >> >> + >> >> + 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... > No worries > thanks, > > greg k-h
On Mon, Feb 17, 2025 at 07:10:54PM +0100, Jerome Brunet wrote: > On Sat 15 Feb 2025 at 07:53, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > [...] > > >> > >> > > >> >> + 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 > > I have tried the 'NULL or valid' approach. In the consumers, > which mostly return an integer from their various init function, I got > this weird to come up with one from NULL. EINVAL, ENOMEM, etc ... can't > really pick one. > > It is actually easier to pass something along. Ok, fair enough, thanks for trying. But I would have returned just -ENODEV in all cases, as that's what the end result was :) thanks, greg k-h
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, + int id) +{ + struct auxiliary_device *auxdev; + int ret; + + auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL); + if (!auxdev) + return ERR_PTR(-ENOMEM); + + 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 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); + return ERR_PTR(ret); + } + + return auxdev; +} + +static void auxiliary_device_destroy(void *_auxdev) +{ + struct auxiliary_device *auxdev = _auxdev; + + auxiliary_device_delete(auxdev); + auxiliary_device_uninit(auxdev); +} + +/** + * __devm_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 + * + * Device managed helper to create an auxiliary bus device. + * The device create 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..c098568eeed2a518b055afbf1f1e68623a2c109a 100644 --- a/include/linux/auxiliary_bus.h +++ b/include/linux/auxiliary_bus.h @@ -254,6 +254,16 @@ int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct module * void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv); +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, id) \ + __devm_auxiliary_device_create(dev, KBUILD_MODNAME, devname, \ + platform_data, id) + /** * 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 | 88 +++++++++++++++++++++++++++++++++++++++++++ include/linux/auxiliary_bus.h | 10 +++++ 2 files changed, 98 insertions(+)