| Message ID | 2025020326-backer-vendetta-7094@gregkh (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Driver core: faux bus | expand |
On Mon, Feb 03, 2025 at 03:25:17PM +0100, Greg Kroah-Hartman wrote: > Many drivers abuse the platform driver/bus system as it provides a > simple way to create and bind a device to a driver-specific set of > probe/release functions. Instead of doing that, and wasting all of the > memory associated with a platform device, here is a "faux" bus that > can be used instead. ... > +#include <linux/device/faux.h> I would rather think that this goes after generic inclusions... > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/string.h> ...somewhere here. But looking into organisation of device.h and device/*.h, I would rather think of the linux/faux_device.h. > +#include "base.h" I don't remember by heart what it does include, I would go with IWYU principle and list above all what we use. container_of.h device.h export.h printk.h types.h ... > +static int faux_match(struct device *dev, const struct device_driver *drv) > +{ > + struct faux_object *faux_obj = to_faux_object(dev); > + > + /* Match is simple, strcmp()! */ > + return (strcmp(faux_obj->name, drv->name) == 0); Outer parentheses are not needed. > +} ... > +/** > + * __faux_device_create - create and register a faux device and driver > + * @name: name of the device and driver we are adding > + * @faux_ops: struct faux_driver_ops that the new device will call back into, can be NULL > + * @owner: module owner of the device/driver > + * > + * Create a new faux device and driver, both with the same name, and register > + * them in the driver core properly. The probe() callback of @faux_ops will be > + * called with the new device that is created for the caller to do something > + * with. The kernel-doc will complain on missing Return: section. > + */ > +struct faux_device *__faux_device_create(const char *name, > + struct faux_driver_ops *faux_ops, > + struct module *owner) > +{ > + struct device_driver *drv; > + struct device *dev; > + struct faux_object *faux_obj; > + struct faux_device *faux_dev; > + int ret; > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); Potential overflow. To avoid one may use struct_size() from overflow.h. > + if (!faux_obj) > + return NULL; > + > + /* Save off the name of the object into local memory */ > + strcpy(faux_obj->name, name); > + > + /* Initialize the driver portion and register it with the driver core */ > + faux_obj->faux_ops = faux_ops; > + drv = &faux_obj->driver; > + > + drv->owner = owner; > + drv->name = faux_obj->name; > + drv->bus = &faux_bus_type; > + drv->probe_type = PROBE_PREFER_ASYNCHRONOUS; > + > + ret = driver_register(drv); > + if (ret) { > + pr_err("%s: driver_register for %s faux driver failed with %d\n", > + __func__, name, ret); > + kfree(faux_obj); > + return NULL; > + } > + > + /* Initialize the device portion and register it with the driver core */ > + faux_dev = &faux_obj->faux_dev; > + dev = &faux_dev->dev; > + > + device_initialize(dev); > + dev->release = faux_device_release; > + dev->parent = &faux_bus_root; > + dev->bus = &faux_bus_type; > + dev_set_name(dev, "%s", name); > + > + ret = device_add(dev); > + if (ret) { > + pr_err("%s: device_add for %s faux device failed with %d\n", > + __func__, name, ret); > + put_device(dev); > + return NULL; > + } > + > + return faux_dev; > +} > +EXPORT_SYMBOL_GPL(__faux_device_create); ... > +#ifndef _FAUX_DEVICE_H_ > +#define _FAUX_DEVICE_H_ > +#include <linux/module.h> + container_of.h > +#include <linux/device.h> > +struct faux_device { > + struct device dev; > +}; > +#define to_faux_device(x) container_of_const((x), struct faux_device, dev) > + > +struct faux_driver_ops { > + int (*probe)(struct faux_device *faux_dev); > + void (*remove)(struct faux_device *faux_dev); > +}; > + > +#define faux_device_create(name, faux_ops) __faux_device_create(name, faux_ops, THIS_MODULE) > +struct faux_device *__faux_device_create(const char *name, > + struct faux_driver_ops *faux_ops, > + struct module *module); > +void faux_device_destroy(struct faux_device *faux_dev); > + > +#endif /* _FAUX_DEVICE_H_ */
On Mon, Feb 03, 2025 at 05:11:03PM +0200, Andy Shevchenko wrote: > On Mon, Feb 03, 2025 at 03:25:17PM +0100, Greg Kroah-Hartman wrote: ... > I don't remember by heart what it does include, I would go with IWYU principle > and list above all what we use. > > container_of.h > device.h > export.h > printk.h > types.h Probably types.h is too much and stddef.h would suffice (as it provides NULL pointer definition).
On Mon, Feb 03, 2025 at 05:11:03PM +0200, Andy Shevchenko wrote: > On Mon, Feb 03, 2025 at 03:25:17PM +0100, Greg Kroah-Hartman wrote: > > Many drivers abuse the platform driver/bus system as it provides a > > simple way to create and bind a device to a driver-specific set of > > probe/release functions. Instead of doing that, and wasting all of the > > memory associated with a platform device, here is a "faux" bus that > > can be used instead. > > ... > > > +#include <linux/device/faux.h> > > I would rather think that this goes after generic inclusions... > > > +#include <linux/err.h> > > +#include <linux/init.h> > > +#include <linux/slab.h> > > +#include <linux/string.h> > > ...somewhere here. > > But looking into organisation of device.h and device/*.h, > I would rather think of the linux/faux_device.h. It can go anywhere, there is no need to sort things :) > > +#include "base.h" > > I don't remember by heart what it does include, I would go with IWYU principle > and list above all what we use. > > container_of.h > device.h > export.h > printk.h > types.h That's not what the driver core ever did, so no need to worry about it, thanks. > > ... > > > +static int faux_match(struct device *dev, const struct device_driver *drv) > > +{ > > + struct faux_object *faux_obj = to_faux_object(dev); > > + > > + /* Match is simple, strcmp()! */ > > + return (strcmp(faux_obj->name, drv->name) == 0); > > Outer parentheses are not needed. Makes me feel good as it is an assignment test, and that's what platform.c has for the past few decades. > > +/** > > + * __faux_device_create - create and register a faux device and driver > > + * @name: name of the device and driver we are adding > > + * @faux_ops: struct faux_driver_ops that the new device will call back into, can be NULL > > + * @owner: module owner of the device/driver > > + * > > + * Create a new faux device and driver, both with the same name, and register > > + * them in the driver core properly. The probe() callback of @faux_ops will be > > + * called with the new device that is created for the caller to do something > > + * with. > > The kernel-doc will complain on missing Return: section. Is that new? Does that mean platform.c has lots of complaints in it as well? What does platform_find_device_by_driver() give you for a documentation issue? And as I didn't hook this up to the kernel documentation build yet, it shouldn't produce any warnings anywhere :) > > + */ > > +struct faux_device *__faux_device_create(const char *name, > > + struct faux_driver_ops *faux_ops, > > + struct module *owner) > > +{ > > + struct device_driver *drv; > > + struct device *dev; > > + struct faux_object *faux_obj; > > + struct faux_device *faux_dev; > > + int ret; > > > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); > > Potential overflow. To avoid one may use struct_size() from overflow.h. Users should not be providing the string here. Again, this comes from platform.c. > > +#ifndef _FAUX_DEVICE_H_ > > +#define _FAUX_DEVICE_H_ > > > +#include <linux/module.h> > > + container_of.h Not needed to compile this file, only if someone uses the #define in it. thanks, greg k-h
On Mon, Feb 03, 2025 at 04:35:45PM +0100, Greg Kroah-Hartman wrote: > > > + */ > > > +struct faux_device *__faux_device_create(const char *name, > > > + struct faux_driver_ops *faux_ops, > > > + struct module *owner) > > > +{ > > > + struct device_driver *drv; > > > + struct device *dev; > > > + struct faux_object *faux_obj; > > > + struct faux_device *faux_dev; > > > + int ret; > > > > > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); > > > > Potential overflow. To avoid one may use struct_size() from overflow.h. > > Users should not be providing the string here. Again, this comes from > platform.c. Sima just proved me wrong, I'll go check for this now, thanks for pointing it out. greg k-h
On Mon, Feb 03, 2025 at 04:35:45PM +0100, Greg Kroah-Hartman wrote: > On Mon, Feb 03, 2025 at 05:11:03PM +0200, Andy Shevchenko wrote: > > On Mon, Feb 03, 2025 at 03:25:17PM +0100, Greg Kroah-Hartman wrote: ... > > > +#include <linux/device/faux.h> > > > > I would rather think that this goes after generic inclusions... > > > > > +#include <linux/err.h> > > > +#include <linux/init.h> > > > +#include <linux/slab.h> > > > +#include <linux/string.h> > > > > ...somewhere here. > > > > But looking into organisation of device.h and device/*.h, > > I would rather think of the linux/faux_device.h. > > It can go anywhere, there is no need to sort things :) It's not about sorting, it's about grouping from more generic to less generic. > > > +#include "base.h" > > > > I don't remember by heart what it does include, I would go with IWYU principle > > and list above all what we use. > > > > container_of.h > > device.h > > export.h > > printk.h > > types.h > > That's not what the driver core ever did, so no need to worry about it, > thanks. But it doesn't mean that driver code does its best. No big worries, of course. ... > > > + return (strcmp(faux_obj->name, drv->name) == 0); > > > > Outer parentheses are not needed. > > Makes me feel good as it is an assignment test, and that's what > platform.c has for the past few decades. Sure, it also can be written as return !strcmp(faux_obj->name, drv->name); that makes the same without explicit comparing to 0. But it's matter of taste. ... > > > +/** > > > + * __faux_device_create - create and register a faux device and driver > > > + * @name: name of the device and driver we are adding > > > + * @faux_ops: struct faux_driver_ops that the new device will call back into, can be NULL > > > + * @owner: module owner of the device/driver > > > + * > > > + * Create a new faux device and driver, both with the same name, and register > > > + * them in the driver core properly. The probe() callback of @faux_ops will be > > > + * called with the new device that is created for the caller to do something > > > + * with. > > > > The kernel-doc will complain on missing Return: section. > > Is that new? Does that mean platform.c has lots of complaints in it as > well? What does platform_find_device_by_driver() give you for a > documentation issue? > > And as I didn't hook this up to the kernel documentation build yet, it > shouldn't produce any warnings anywhere :) I would rather say it's old. Run kernel-doc -Wall -none -v ...your file... and find the warning. During the kernel builds this is moved to extra warnings. > > > + */ ... > > > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); > > > > Potential overflow. To avoid one may use struct_size() from overflow.h. > > Users should not be providing the string here. Again, this comes from > platform.c. I'm not sure I follow. The name parameter is not limited anyhow, so one may provide non-terminated string and strlen() will return an arbitrary number. Potentially this can lead to big numbers and become an overflow when gets to a parameter for kmalloc(). This most likely never happen in real life, but still the overflow is possible.
On Mon, Feb 03, 2025 at 04:46:56PM +0100, Greg Kroah-Hartman wrote: > On Mon, Feb 03, 2025 at 04:35:45PM +0100, Greg Kroah-Hartman wrote: > > > > + */ > > > > +struct faux_device *__faux_device_create(const char *name, > > > > + struct faux_driver_ops *faux_ops, > > > > + struct module *owner) > > > > +{ > > > > + struct device_driver *drv; > > > > + struct device *dev; > > > > + struct faux_object *faux_obj; > > > > + struct faux_device *faux_dev; > > > > + int ret; > > > > > > > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); > > > > > > Potential overflow. To avoid one may use struct_size() from overflow.h. > > > > Users should not be providing the string here. Again, this comes from > > platform.c. > > Sima just proved me wrong, I'll go check for this now, thanks for > pointing it out. Ah, you are welcome!
On Mon, Feb 03, 2025 at 06:05:20PM +0200, Andy Shevchenko wrote: > On Mon, Feb 03, 2025 at 04:35:45PM +0100, Greg Kroah-Hartman wrote: > > On Mon, Feb 03, 2025 at 05:11:03PM +0200, Andy Shevchenko wrote: > > > On Mon, Feb 03, 2025 at 03:25:17PM +0100, Greg Kroah-Hartman wrote: ... > > > > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); > > > > > > Potential overflow. To avoid one may use struct_size() from overflow.h. > > > > Users should not be providing the string here. Again, this comes from > > platform.c. > > I'm not sure I follow. The name parameter is not limited anyhow, so one may > provide non-terminated string and strlen() will return an arbitrary number. > Potentially this can lead to big numbers and become an overflow when gets > to a parameter for kmalloc(). This most likely never happen in real life, > but still the overflow is possible. After reading your other messages I got what you are talking about here. Now it's all clear.
On Mon, Feb 03, 2025 at 06:05:19PM +0200, Andy Shevchenko wrote: > > > > +/** > > > > + * __faux_device_create - create and register a faux device and driver > > > > + * @name: name of the device and driver we are adding > > > > + * @faux_ops: struct faux_driver_ops that the new device will call back into, can be NULL > > > > + * @owner: module owner of the device/driver > > > > + * > > > > + * Create a new faux device and driver, both with the same name, and register > > > > + * them in the driver core properly. The probe() callback of @faux_ops will be > > > > + * called with the new device that is created for the caller to do something > > > > + * with. > > > > > > The kernel-doc will complain on missing Return: section. > > > > Is that new? Does that mean platform.c has lots of complaints in it as > > well? What does platform_find_device_by_driver() give you for a > > documentation issue? > > > > And as I didn't hook this up to the kernel documentation build yet, it > > shouldn't produce any warnings anywhere :) > > I would rather say it's old. > > Run > > kernel-doc -Wall -none -v ...your file... > > and find the warning. During the kernel builds this is moved to extra warnings. Ah, nice, didn't know about that. Now fixed up. > > > > + */ > > ... > > > > > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); > > > > > > Potential overflow. To avoid one may use struct_size() from overflow.h. > > > > Users should not be providing the string here. Again, this comes from > > platform.c. > > I'm not sure I follow. The name parameter is not limited anyhow, so one may > provide non-terminated string and strlen() will return an arbitrary number. > Potentially this can lead to big numbers and become an overflow when gets > to a parameter for kmalloc(). This most likely never happen in real life, > but still the overflow is possible. I've now bounded at 256, because really, who needs a bigger name for a device than that :) thanks, greg k-h
On Mon, Feb 03, 2025 at 05:13:28PM +0100, Greg Kroah-Hartman wrote: > On Mon, Feb 03, 2025 at 06:05:19PM +0200, Andy Shevchenko wrote: ... > > > > > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); > > > > > > > > Potential overflow. To avoid one may use struct_size() from overflow.h. > > > > > > Users should not be providing the string here. Again, this comes from > > > platform.c. > > > > I'm not sure I follow. The name parameter is not limited anyhow, so one may > > provide non-terminated string and strlen() will return an arbitrary number. > > Potentially this can lead to big numbers and become an overflow when gets > > to a parameter for kmalloc(). This most likely never happen in real life, > > but still the overflow is possible. > > I've now bounded at 256, because really, who needs a bigger name for a > device than that :) Works for me! With printable ASCII characters it can be estimated as up to 64^256 combinations, which "ought to be enough for everybody" (of course it will be much less if we count only human-readable strings). :-)
On Mon, 3 Feb 2025 15:25:17 +0100 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > Many drivers abuse the platform driver/bus system as it provides a > simple way to create and bind a device to a driver-specific set of > probe/release functions. Instead of doing that, and wasting all of the > memory associated with a platform device, here is a "faux" bus that > can be used instead. > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Nice. One trivial thing inline. > diff --git a/drivers/base/faux.c b/drivers/base/faux.c > new file mode 100644 > index 000000000000..0eba89a5cd57 > --- /dev/null > +++ b/drivers/base/faux.c > @@ -0,0 +1,189 @@ > +int __init faux_bus_init(void) > +{ > + int error; > + > + error = device_register(&faux_bus_root); > + if (error) { > + put_device(&faux_bus_root); > + return error; > + } > + > + error = bus_register(&faux_bus_type); Odd bonus space after = > + if (error) > + device_unregister(&faux_bus_root); > + > + return error; > +}
On 2025-02-03 15:25:17+0100, Greg Kroah-Hartman wrote: > Many drivers abuse the platform driver/bus system as it provides a > simple way to create and bind a device to a driver-specific set of > probe/release functions. Instead of doing that, and wasting all of the > memory associated with a platform device, here is a "faux" bus that > can be used instead. > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/base/Makefile | 2 +- > drivers/base/base.h | 1 + > drivers/base/faux.c | 189 ++++++++++++++++++++++++++++++++++++ > drivers/base/init.c | 1 + > include/linux/device/faux.h | 33 +++++++ > 5 files changed, 225 insertions(+), 1 deletion(-) > create mode 100644 drivers/base/faux.c > create mode 100644 include/linux/device/faux.h <snip> > diff --git a/drivers/base/faux.c b/drivers/base/faux.c > new file mode 100644 > index 000000000000..0eba89a5cd57 > --- /dev/null > +++ b/drivers/base/faux.c > @@ -0,0 +1,189 @@ > +// SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2025 Greg Kroah-Hartman <gregkh@linuxfoundation.org> > + * Copyright (c) 2025 The Linux Foundation > + * > + * A "simple" faux bus that allows devices to be created and added > + * automatically to it. Whenever you need a device that is not "real", > + * use this interface instead of even thinking of using a platform device. > + * > + */ > +#include <linux/device/faux.h> > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/string.h> > +#include "base.h" > + > +/* > + * Internal wrapper structure so we can hold the memory > + * for the driver and the name string of the faux device. > + */ > +struct faux_object { > + struct faux_device faux_dev; > + struct device_driver driver; > + const struct faux_driver_ops *faux_ops; > + char name[]; > +}; > +#define to_faux_object(x) container_of_const(dev, struct faux_object, faux_dev.dev); > + > +static struct device faux_bus_root = { > + .init_name = "faux_bus", > +}; > + > +static int faux_match(struct device *dev, const struct device_driver *drv) > +{ > + struct faux_object *faux_obj = to_faux_object(dev); > + > + /* Match is simple, strcmp()! */ > + return (strcmp(faux_obj->name, drv->name) == 0); > +} > + > +static int faux_probe(struct device *dev) > +{ > + struct faux_object *faux_obj = to_faux_object(dev); > + struct faux_device *faux_dev = &faux_obj->faux_dev; > + const struct faux_driver_ops *faux_ops = faux_obj->faux_ops; > + int ret = 0; > + > + if (faux_ops && faux_ops->probe) > + ret = faux_ops->probe(faux_dev); > + > + return ret; > +} > + > +static void faux_remove(struct device *dev) > +{ > + struct faux_object *faux_obj = to_faux_object(dev); > + struct faux_device *faux_dev = &faux_obj->faux_dev; > + const struct faux_driver_ops *faux_ops = faux_obj->faux_ops; > + > + if (faux_ops && faux_ops->remove) > + faux_ops->remove(faux_dev); > +} > + > +static const struct bus_type faux_bus_type = { > + .name = "faux_bus", Is the _bus suffix intentional? Other busses don't have it. > + .match = faux_match, > + .probe = faux_probe, > + .remove = faux_remove, > +}; > + > +static void faux_device_release(struct device *dev) > +{ > + struct faux_object *faux_obj = to_faux_object(dev); > + struct device_driver *drv = &faux_obj->driver; > + > + /* > + * Now that the device is going away, it has been unbound from the > + * driver we created for it, so it is safe to unregister the driver from > + * the system. > + */ > + driver_unregister(drv); > + > + kfree(faux_obj); > +} > + > +/** > + * __faux_device_create - create and register a faux device and driver > + * @name: name of the device and driver we are adding > + * @faux_ops: struct faux_driver_ops that the new device will call back into, can be NULL > + * @owner: module owner of the device/driver > + * > + * Create a new faux device and driver, both with the same name, and register > + * them in the driver core properly. The probe() callback of @faux_ops will be > + * called with the new device that is created for the caller to do something > + * with. > + */ > +struct faux_device *__faux_device_create(const char *name, > + struct faux_driver_ops *faux_ops, const > + struct module *owner) What about attributes? > +{ > + struct device_driver *drv; > + struct device *dev; > + struct faux_object *faux_obj; > + struct faux_device *faux_dev; > + int ret; > + > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); > + if (!faux_obj) > + return NULL; > + > + /* Save off the name of the object into local memory */ > + strcpy(faux_obj->name, name); > + > + /* Initialize the driver portion and register it with the driver core */ > + faux_obj->faux_ops = faux_ops; > + drv = &faux_obj->driver; > + > + drv->owner = owner; > + drv->name = faux_obj->name; Assuming most names are constant, this would be better with kstrdup_const(). Which is also used by dev_set_name() under the hood. > + drv->bus = &faux_bus_type; > + drv->probe_type = PROBE_PREFER_ASYNCHRONOUS; > + > + ret = driver_register(drv); > + if (ret) { > + pr_err("%s: driver_register for %s faux driver failed with %d\n", > + __func__, name, ret); > + kfree(faux_obj); > + return NULL; > + } > + > + /* Initialize the device portion and register it with the driver core */ > + faux_dev = &faux_obj->faux_dev; > + dev = &faux_dev->dev; > + > + device_initialize(dev); > + dev->release = faux_device_release; > + dev->parent = &faux_bus_root; > + dev->bus = &faux_bus_type; > + dev_set_name(dev, "%s", name); > + > + ret = device_add(dev); > + if (ret) { > + pr_err("%s: device_add for %s faux device failed with %d\n", > + __func__, name, ret); > + put_device(dev); > + return NULL; > + } > + > + return faux_dev; > +} > +EXPORT_SYMBOL_GPL(__faux_device_create); <snip>
On Tue, Feb 04, 2025 at 09:25:32AM +0000, Jonathan Cameron wrote: > On Mon, 3 Feb 2025 15:25:17 +0100 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > Many drivers abuse the platform driver/bus system as it provides a > > simple way to create and bind a device to a driver-specific set of > > probe/release functions. Instead of doing that, and wasting all of the > > memory associated with a platform device, here is a "faux" bus that > > can be used instead. > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Nice. One trivial thing inline. > > > > diff --git a/drivers/base/faux.c b/drivers/base/faux.c > > new file mode 100644 > > index 000000000000..0eba89a5cd57 > > --- /dev/null > > +++ b/drivers/base/faux.c > > @@ -0,0 +1,189 @@ > > > > +int __init faux_bus_init(void) > > +{ > > + int error; > > + > > + error = device_register(&faux_bus_root); > > + if (error) { > > + put_device(&faux_bus_root); > > + return error; > > + } > > + > > + error = bus_register(&faux_bus_type); > > Odd bonus space after = Thanks! I caught that when I just rewrote this function, and I finally got around to running checkpatch which found another error in it that I missed. It's sometimes easier to notice changes in other's code than your own :) greg k-h
On Tue, Feb 04, 2025 at 11:08:11AM +0100, Thomas Weißschuh wrote: > On 2025-02-03 15:25:17+0100, Greg Kroah-Hartman wrote: > > +static void faux_remove(struct device *dev) > > +{ > > + struct faux_object *faux_obj = to_faux_object(dev); > > + struct faux_device *faux_dev = &faux_obj->faux_dev; > > + const struct faux_driver_ops *faux_ops = faux_obj->faux_ops; > > + > > + if (faux_ops && faux_ops->remove) > > + faux_ops->remove(faux_dev); > > +} > > + > > +static const struct bus_type faux_bus_type = { > > + .name = "faux_bus", > > Is the _bus suffix intentional? It was intentional. > Other busses don't have it. True. Naming is hard. I guess /sys/bus/faux/ makes sense, I will go rename it. But for the "root" device, does /sys/devices/faux_bus/ make sense, or should it be /sys/devices/faux/ as well? I'm now leaning toward the latter... > > + .match = faux_match, > > + .probe = faux_probe, > > + .remove = faux_remove, > > +}; > > + > > +static void faux_device_release(struct device *dev) > > +{ > > + struct faux_object *faux_obj = to_faux_object(dev); > > + struct device_driver *drv = &faux_obj->driver; > > + > > + /* > > + * Now that the device is going away, it has been unbound from the > > + * driver we created for it, so it is safe to unregister the driver from > > + * the system. > > + */ > > + driver_unregister(drv); > > + > > + kfree(faux_obj); > > +} > > + > > +/** > > + * __faux_device_create - create and register a faux device and driver > > + * @name: name of the device and driver we are adding > > + * @faux_ops: struct faux_driver_ops that the new device will call back into, can be NULL > > + * @owner: module owner of the device/driver > > + * > > + * Create a new faux device and driver, both with the same name, and register > > + * them in the driver core properly. The probe() callback of @faux_ops will be > > + * called with the new device that is created for the caller to do something > > + * with. > > + */ > > +struct faux_device *__faux_device_create(const char *name, > > + struct faux_driver_ops *faux_ops, > > const > > > + struct module *owner) > > What about attributes? What in-kernel user of this wants an attribute for such a device? And again, if we find one, we can make a faux_device_create_groups() call that takes a pointer to an attribute group structure if it's really needed. > > > +{ > > + struct device_driver *drv; > > + struct device *dev; > > + struct faux_object *faux_obj; > > + struct faux_device *faux_dev; > > + int ret; > > + > > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); > > + if (!faux_obj) > > + return NULL; > > + > > + /* Save off the name of the object into local memory */ > > + strcpy(faux_obj->name, name); > > + > > + /* Initialize the driver portion and register it with the driver core */ > > + faux_obj->faux_ops = faux_ops; > > + drv = &faux_obj->driver; > > + > > + drv->owner = owner; > > + drv->name = faux_obj->name; > > Assuming most names are constant, this would be better with kstrdup_const(). > Which is also used by dev_set_name() under the hood. I've now removed the additional driver, but note that this is just a pointer assignment, which is fine to do here as the lifespan of faux_obj->name outlived the driver structure's lifespan. thanks for the review, much appreciated! greg k-h
On 2025-02-04 11:20:43+0100, Greg Kroah-Hartman wrote: > On Tue, Feb 04, 2025 at 11:08:11AM +0100, Thomas Weißschuh wrote: > > On 2025-02-03 15:25:17+0100, Greg Kroah-Hartman wrote: > > > +static void faux_remove(struct device *dev) > > > +{ > > > + struct faux_object *faux_obj = to_faux_object(dev); > > > + struct faux_device *faux_dev = &faux_obj->faux_dev; > > > + const struct faux_driver_ops *faux_ops = faux_obj->faux_ops; > > > + > > > + if (faux_ops && faux_ops->remove) > > > + faux_ops->remove(faux_dev); > > > +} > > > + > > > +static const struct bus_type faux_bus_type = { > > > + .name = "faux_bus", > > > > Is the _bus suffix intentional? > > It was intentional. > > > Other busses don't have it. > > True. Naming is hard. I guess /sys/bus/faux/ makes sense, I will go > rename it. > > But for the "root" device, does /sys/devices/faux_bus/ make sense, or > should it be /sys/devices/faux/ as well? I'm now leaning toward the > latter... I'm leaning slightly towards the former. But my naming skills are beyond limited. > > > + .match = faux_match, > > > + .probe = faux_probe, > > > + .remove = faux_remove, > > > +}; > > > + > > > +static void faux_device_release(struct device *dev) > > > +{ > > > + struct faux_object *faux_obj = to_faux_object(dev); > > > + struct device_driver *drv = &faux_obj->driver; > > > + > > > + /* > > > + * Now that the device is going away, it has been unbound from the > > > + * driver we created for it, so it is safe to unregister the driver from > > > + * the system. > > > + */ > > > + driver_unregister(drv); > > > + > > > + kfree(faux_obj); > > > +} > > > + > > > +/** > > > + * __faux_device_create - create and register a faux device and driver > > > + * @name: name of the device and driver we are adding > > > + * @faux_ops: struct faux_driver_ops that the new device will call back into, can be NULL > > > + * @owner: module owner of the device/driver > > > + * > > > + * Create a new faux device and driver, both with the same name, and register > > > + * them in the driver core properly. The probe() callback of @faux_ops will be > > > + * called with the new device that is created for the caller to do something > > > + * with. > > > + */ > > > +struct faux_device *__faux_device_create(const char *name, > > > + struct faux_driver_ops *faux_ops, > > > > const > > > > > + struct module *owner) > > > > What about attributes? > > What in-kernel user of this wants an attribute for such a device? It was mostly a guess. However drivers/video/fbdev/gbefb.c seems to be an example. > And again, if we find one, we can make a faux_device_create_groups() > call that takes a pointer to an attribute group structure if it's really > needed. Fair enough. > > > +{ > > > + struct device_driver *drv; > > > + struct device *dev; > > > + struct faux_object *faux_obj; > > > + struct faux_device *faux_dev; > > > + int ret; > > > + > > > + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); > > > + if (!faux_obj) > > > + return NULL; > > > + > > > + /* Save off the name of the object into local memory */ > > > + strcpy(faux_obj->name, name); > > > + > > > + /* Initialize the driver portion and register it with the driver core */ > > > + faux_obj->faux_ops = faux_ops; > > > + drv = &faux_obj->driver; > > > + > > > + drv->owner = owner; > > > + drv->name = faux_obj->name; > > > > Assuming most names are constant, this would be better with kstrdup_const(). > > Which is also used by dev_set_name() under the hood. > > I've now removed the additional driver, but note that this is just a > pointer assignment, which is fine to do here as the lifespan of > faux_obj->name outlived the driver structure's lifespan. It outlives it because there is extra space allocated for it in faux_obj. With kstrdup_const() that space would not be needed. In the end it shouldn't really matter one way or another. Thomas
On Tue, Feb 04, 2025 at 11:44:41AM +0100, Thomas Weißschuh wrote: > On 2025-02-04 11:20:43+0100, Greg Kroah-Hartman wrote: > > On Tue, Feb 04, 2025 at 11:08:11AM +0100, Thomas Weißschuh wrote: > > > On 2025-02-03 15:25:17+0100, Greg Kroah-Hartman wrote: > > > > +static void faux_remove(struct device *dev) > > > > +{ > > > > + struct faux_object *faux_obj = to_faux_object(dev); > > > > + struct faux_device *faux_dev = &faux_obj->faux_dev; > > > > + const struct faux_driver_ops *faux_ops = faux_obj->faux_ops; > > > > + > > > > + if (faux_ops && faux_ops->remove) > > > > + faux_ops->remove(faux_dev); > > > > +} > > > > + > > > > +static const struct bus_type faux_bus_type = { > > > > + .name = "faux_bus", > > > > > > Is the _bus suffix intentional? > > > > It was intentional. > > > > > Other busses don't have it. > > > > True. Naming is hard. I guess /sys/bus/faux/ makes sense, I will go > > rename it. > > > > But for the "root" device, does /sys/devices/faux_bus/ make sense, or > > should it be /sys/devices/faux/ as well? I'm now leaning toward the > > latter... > > I'm leaning slightly towards the former. > But my naming skills are beyond limited. > > > > > + .match = faux_match, > > > > + .probe = faux_probe, > > > > + .remove = faux_remove, > > > > +}; > > > > + > > > > +static void faux_device_release(struct device *dev) > > > > +{ > > > > + struct faux_object *faux_obj = to_faux_object(dev); > > > > + struct device_driver *drv = &faux_obj->driver; > > > > + > > > > + /* > > > > + * Now that the device is going away, it has been unbound from the > > > > + * driver we created for it, so it is safe to unregister the driver from > > > > + * the system. > > > > + */ > > > > + driver_unregister(drv); > > > > + > > > > + kfree(faux_obj); > > > > +} > > > > + > > > > +/** > > > > + * __faux_device_create - create and register a faux device and driver > > > > + * @name: name of the device and driver we are adding > > > > + * @faux_ops: struct faux_driver_ops that the new device will call back into, can be NULL > > > > + * @owner: module owner of the device/driver > > > > + * > > > > + * Create a new faux device and driver, both with the same name, and register > > > > + * them in the driver core properly. The probe() callback of @faux_ops will be > > > > + * called with the new device that is created for the caller to do something > > > > + * with. > > > > + */ > > > > +struct faux_device *__faux_device_create(const char *name, > > > > + struct faux_driver_ops *faux_ops, > > > > > > const > > > > > > > + struct module *owner) > > > > > > What about attributes? > > > > What in-kernel user of this wants an attribute for such a device? > > It was mostly a guess. > However drivers/video/fbdev/gbefb.c seems to be an example. As that driver is allocateing io memory and the like, it really looks like a "real" platform driver/device to me. Do you think it should not be one? Also, that driver should be converted to use an attribute group instead of manually adding those sysfs files, if you wanted to touch it in the future :) thanks, greg k-h
diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 7fb21768ca36..8074a10183dc 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -6,7 +6,7 @@ obj-y := component.o core.o bus.o dd.o syscore.o \ cpu.o firmware.o init.o map.o devres.o \ attribute_container.o transport_class.o \ topology.o container.o property.o cacheinfo.o \ - swnode.o + swnode.o faux.o obj-$(CONFIG_AUXILIARY_BUS) += auxiliary.o obj-$(CONFIG_DEVTMPFS) += devtmpfs.o obj-y += power/ diff --git a/drivers/base/base.h b/drivers/base/base.h index 8cf04a557bdb..0042e4774b0c 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -137,6 +137,7 @@ int hypervisor_init(void); static inline int hypervisor_init(void) { return 0; } #endif int platform_bus_init(void); +int faux_bus_init(void); void cpu_dev_init(void); void container_dev_init(void); #ifdef CONFIG_AUXILIARY_BUS diff --git a/drivers/base/faux.c b/drivers/base/faux.c new file mode 100644 index 000000000000..0eba89a5cd57 --- /dev/null +++ b/drivers/base/faux.c @@ -0,0 +1,189 @@ +// SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2025 Greg Kroah-Hartman <gregkh@linuxfoundation.org> + * Copyright (c) 2025 The Linux Foundation + * + * A "simple" faux bus that allows devices to be created and added + * automatically to it. Whenever you need a device that is not "real", + * use this interface instead of even thinking of using a platform device. + * + */ +#include <linux/device/faux.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/string.h> +#include "base.h" + +/* + * Internal wrapper structure so we can hold the memory + * for the driver and the name string of the faux device. + */ +struct faux_object { + struct faux_device faux_dev; + struct device_driver driver; + const struct faux_driver_ops *faux_ops; + char name[]; +}; +#define to_faux_object(x) container_of_const(dev, struct faux_object, faux_dev.dev); + +static struct device faux_bus_root = { + .init_name = "faux_bus", +}; + +static int faux_match(struct device *dev, const struct device_driver *drv) +{ + struct faux_object *faux_obj = to_faux_object(dev); + + /* Match is simple, strcmp()! */ + return (strcmp(faux_obj->name, drv->name) == 0); +} + +static int faux_probe(struct device *dev) +{ + struct faux_object *faux_obj = to_faux_object(dev); + struct faux_device *faux_dev = &faux_obj->faux_dev; + const struct faux_driver_ops *faux_ops = faux_obj->faux_ops; + int ret = 0; + + if (faux_ops && faux_ops->probe) + ret = faux_ops->probe(faux_dev); + + return ret; +} + +static void faux_remove(struct device *dev) +{ + struct faux_object *faux_obj = to_faux_object(dev); + struct faux_device *faux_dev = &faux_obj->faux_dev; + const struct faux_driver_ops *faux_ops = faux_obj->faux_ops; + + if (faux_ops && faux_ops->remove) + faux_ops->remove(faux_dev); +} + +static const struct bus_type faux_bus_type = { + .name = "faux_bus", + .match = faux_match, + .probe = faux_probe, + .remove = faux_remove, +}; + +static void faux_device_release(struct device *dev) +{ + struct faux_object *faux_obj = to_faux_object(dev); + struct device_driver *drv = &faux_obj->driver; + + /* + * Now that the device is going away, it has been unbound from the + * driver we created for it, so it is safe to unregister the driver from + * the system. + */ + driver_unregister(drv); + + kfree(faux_obj); +} + +/** + * __faux_device_create - create and register a faux device and driver + * @name: name of the device and driver we are adding + * @faux_ops: struct faux_driver_ops that the new device will call back into, can be NULL + * @owner: module owner of the device/driver + * + * Create a new faux device and driver, both with the same name, and register + * them in the driver core properly. The probe() callback of @faux_ops will be + * called with the new device that is created for the caller to do something + * with. + */ +struct faux_device *__faux_device_create(const char *name, + struct faux_driver_ops *faux_ops, + struct module *owner) +{ + struct device_driver *drv; + struct device *dev; + struct faux_object *faux_obj; + struct faux_device *faux_dev; + int ret; + + faux_obj = kzalloc(sizeof(*faux_obj) + strlen(name) + 1, GFP_KERNEL); + if (!faux_obj) + return NULL; + + /* Save off the name of the object into local memory */ + strcpy(faux_obj->name, name); + + /* Initialize the driver portion and register it with the driver core */ + faux_obj->faux_ops = faux_ops; + drv = &faux_obj->driver; + + drv->owner = owner; + drv->name = faux_obj->name; + drv->bus = &faux_bus_type; + drv->probe_type = PROBE_PREFER_ASYNCHRONOUS; + + ret = driver_register(drv); + if (ret) { + pr_err("%s: driver_register for %s faux driver failed with %d\n", + __func__, name, ret); + kfree(faux_obj); + return NULL; + } + + /* Initialize the device portion and register it with the driver core */ + faux_dev = &faux_obj->faux_dev; + dev = &faux_dev->dev; + + device_initialize(dev); + dev->release = faux_device_release; + dev->parent = &faux_bus_root; + dev->bus = &faux_bus_type; + dev_set_name(dev, "%s", name); + + ret = device_add(dev); + if (ret) { + pr_err("%s: device_add for %s faux device failed with %d\n", + __func__, name, ret); + put_device(dev); + return NULL; + } + + return faux_dev; +} +EXPORT_SYMBOL_GPL(__faux_device_create); + +/** + * faux_device_destroy - destroy a faux device + * @faux_dev: faux device to destroy + * + * Unregister and free all memory associated with a faux device. + */ +void faux_device_destroy(struct faux_device *faux_dev) +{ + struct device *dev = &faux_dev->dev; + + if (IS_ERR_OR_NULL(faux_dev)) + return; + + device_del(dev); + + /* The final put_device() will clean up the driver we created for this device. */ + put_device(dev); +} +EXPORT_SYMBOL_GPL(faux_device_destroy); + +int __init faux_bus_init(void) +{ + int error; + + error = device_register(&faux_bus_root); + if (error) { + put_device(&faux_bus_root); + return error; + } + + error = bus_register(&faux_bus_type); + if (error) + device_unregister(&faux_bus_root); + + return error; +} diff --git a/drivers/base/init.c b/drivers/base/init.c index c4954835128c..9d2b06d65dfc 100644 --- a/drivers/base/init.c +++ b/drivers/base/init.c @@ -32,6 +32,7 @@ void __init driver_init(void) /* These are also core pieces, but must come after the * core core pieces. */ + faux_bus_init(); of_core_init(); platform_bus_init(); auxiliary_bus_init(); diff --git a/include/linux/device/faux.h b/include/linux/device/faux.h new file mode 100644 index 000000000000..b1a51fbb6f39 --- /dev/null +++ b/include/linux/device/faux.h @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2025 Greg Kroah-Hartman <gregkh@linuxfoundation.org> + * Copyright (c) 2025 The Linux Foundation + * + * A "simple" faux bus that allows devices to be created and added + * automatically to it. Whenever you need a device that is not "real", + * use this interface instead of even thinking of using a platform device. + * + */ +#ifndef _FAUX_DEVICE_H_ +#define _FAUX_DEVICE_H_ + +#include <linux/module.h> +#include <linux/device.h> + +struct faux_device { + struct device dev; +}; +#define to_faux_device(x) container_of_const((x), struct faux_device, dev) + +struct faux_driver_ops { + int (*probe)(struct faux_device *faux_dev); + void (*remove)(struct faux_device *faux_dev); +}; + +#define faux_device_create(name, faux_ops) __faux_device_create(name, faux_ops, THIS_MODULE) +struct faux_device *__faux_device_create(const char *name, + struct faux_driver_ops *faux_ops, + struct module *module); +void faux_device_destroy(struct faux_device *faux_dev); + +#endif /* _FAUX_DEVICE_H_ */
Many drivers abuse the platform driver/bus system as it provides a simple way to create and bind a device to a driver-specific set of probe/release functions. Instead of doing that, and wasting all of the memory associated with a platform device, here is a "faux" bus that can be used instead. Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/base/Makefile | 2 +- drivers/base/base.h | 1 + drivers/base/faux.c | 189 ++++++++++++++++++++++++++++++++++++ drivers/base/init.c | 1 + include/linux/device/faux.h | 33 +++++++ 5 files changed, 225 insertions(+), 1 deletion(-) create mode 100644 drivers/base/faux.c create mode 100644 include/linux/device/faux.h