[v4,1/9] driver core: add a faux bus for use when a simple device/bus is needed

Commit Message

Greg Kroah-Hartman Feb. 10, 2025, 12:30 p.m. UTC

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.

Reviewed-by: Jonathan Cameron <[e-mail]>
Reviewed-by: Danilo Krummrich <[e-mail]>
Reviewed-by: Lyude Paul <[e-mail]>
Reviewed-by: Thomas Weißschuh <[e-mail]>
Signed-off-by: Greg Kroah-Hartman <[e-mail]>
---
v4: - really removed the name logic
    - added #include <linux/container_of.h> to faux.h
    - added parent pointer to api call
    - minor documentation updates
    - made probe synchronous
v3: - loads of documentation updates and rewrites
    - added to the documentation build
    - removed name[] array as it's no longer needed
    - added faux_device_create_with_groups()
    - added functions to get/set devdata
    - renamed faux_driver_ops -> faux_device_ops
    - made faux_device_ops a const *
    - minor cleanups
    - tested it, again.

v2: - renamed bus and root device to just "faux" thanks to Thomas
    - removed the one-driver-per-device and now just have one driver
      entirely thanks to Danilo
    - kerneldoc fixups and additions and string handling bounds checks
      thanks to Andy
    - coding style fix thanks to Jonathan
    - tested that the destroy path actually works
 Documentation/driver-api/infrastructure.rst |   6 +
 drivers/base/Makefile                       |   2 +-
 drivers/base/base.h                         |   1 +
 drivers/base/faux.c                         | 232 ++++++++++++++++++++
 drivers/base/init.c                         |   1 +
 include/linux/device/faux.h                 |  69 ++++++
 6 files changed, 310 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/faux.c
 create mode 100644 include/linux/device/faux.h

Comments

Kurt Borja Feb. 10, 2025, 2:29 p.m. UTC | #1

Hi Greg,

On Mon Feb 10, 2025 at 7:30 AM -05, 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.
>
> Reviewed-by: Jonathan Cameron <[e-mail]>
> Reviewed-by: Danilo Krummrich <[e-mail]>
> Reviewed-by: Lyude Paul <[e-mail]>
> Reviewed-by: Thomas Weißschuh <[e-mail]>
> Signed-off-by: Greg Kroah-Hartman <[e-mail]>
> ---
> v4: - really removed the name logic
>     - added #include <linux/container_of.h> to faux.h
>     - added parent pointer to api call
>     - minor documentation updates
>     - made probe synchronous
> v3: - loads of documentation updates and rewrites
>     - added to the documentation build
>     - removed name[] array as it's no longer needed
>     - added faux_device_create_with_groups()
>     - added functions to get/set devdata
>     - renamed faux_driver_ops -> faux_device_ops
>     - made faux_device_ops a const *
>     - minor cleanups
>     - tested it, again.
>
> v2: - renamed bus and root device to just "faux" thanks to Thomas
>     - removed the one-driver-per-device and now just have one driver
>       entirely thanks to Danilo
>     - kerneldoc fixups and additions and string handling bounds checks
>       thanks to Andy
>     - coding style fix thanks to Jonathan
>     - tested that the destroy path actually works
>  Documentation/driver-api/infrastructure.rst |   6 +
>  drivers/base/Makefile                       |   2 +-
>  drivers/base/base.h                         |   1 +
>  drivers/base/faux.c                         | 232 ++++++++++++++++++++
>  drivers/base/init.c                         |   1 +
>  include/linux/device/faux.h                 |  69 ++++++
>  6 files changed, 310 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/base/faux.c
>  create mode 100644 include/linux/device/faux.h
>
> diff --git a/Documentation/driver-api/infrastructure.rst b/Documentation/driver-api/infrastructure.rst
> index 3d52dfdfa9fd..35e36fee4238 100644
> --- a/Documentation/driver-api/infrastructure.rst
> +++ b/Documentation/driver-api/infrastructure.rst
> @@ -41,6 +41,12 @@ Device Drivers Base
>  .. kernel-doc:: drivers/base/class.c
>     :export:
>  
> +.. kernel-doc:: include/linux/device/faux.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/base/faux.c
> +   :export:
> +
>  .. kernel-doc:: drivers/base/node.c
>     :internal:
>  
> 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..531e9d789ee0
> --- /dev/null
> +++ b/drivers/base/faux.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
> + * Copyright (c) 2025 The Linux Foundation
> + *
> + * A "simple" faux bus that allows devices to be created and added
> + * automatically to it.  This is to be used whenever you need to create a
> + * device that is not associated with any "real" system resources, and do
> + * not want to have to deal with a bus/driver binding logic.  It is
> + * intended to be very simple, with only a create and a destroy function
> + * available.
> + */
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/container_of.h>
> +#include <linux/device/faux.h>
> +#include "base.h"
> +
> +/*
> + * Internal wrapper structure so we can hold a pointer to the
> + * faux_device_ops for this device.
> + */
> +struct faux_object {
> +	struct faux_device faux_dev;
> +	const struct faux_device_ops *faux_ops;
> +};
> +#define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
> +
> +static struct device faux_bus_root = {
> +	.init_name	= "faux",
> +};
> +
> +static int faux_match(struct device *dev, const struct device_driver *drv)
> +{
> +	/* Match always succeeds, we only have one driver */
> +	return 1;
> +}
> +
> +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_device_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_device_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",
> +	.match		= faux_match,
> +	.probe		= faux_probe,
> +	.remove		= faux_remove,
> +};
> +
> +static struct device_driver faux_driver = {
> +	.name		= "faux_driver",
> +	.bus		= &faux_bus_type,
> +	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
> +};
> +
> +static void faux_device_release(struct device *dev)
> +{
> +	struct faux_object *faux_obj = to_faux_object(dev);
> +
> +	kfree(faux_obj);
> +}
> +
> +/**
> + * faux_device_create_with_groups - Create and register with the driver
> + *		core a faux device and populate the device with an initial
> + *		set of sysfs attributes.
> + * @name:	The name of the device we are adding, must be unique for
> + *		all faux devices.
> + * @parent:	Pointer to a potential parent struct device.  If set to
> + *		NULL, the device will be created in the "root" of the faux
> + *		device tree in sysfs.
> + * @faux_ops:	struct faux_device_ops that the new device will call back
> + *		into, can be NULL.
> + * @groups:	The set of sysfs attributes that will be created for this
> + *		device when it is registered with the driver core.
> + *
> + * Create a new faux device and register it in the driver core properly.
> + * If present, callbacks in @faux_ops will be called with the device that
> + * for the caller to do something with at the proper time given the
> + * device's lifecycle.
> + *
> + * Note, when this function is called, the functions specified in struct
> + * faux_ops can be called before the function returns, so be prepared for
> + * everything to be properly initialized before that point in time.
> + *
> + * Return:
> + * * NULL if an error happened with creating the device
> + * * pointer to a valid struct faux_device that is registered with sysfs
> + */
> +struct faux_device *faux_device_create_with_groups(const char *name,
> +						   struct device *parent,
> +						   const struct faux_device_ops *faux_ops,
> +						   const struct attribute_group **groups)
> +{
> +	struct faux_object *faux_obj;
> +	struct faux_device *faux_dev;
> +	struct device *dev;
> +	int ret;
> +
> +	faux_obj = kzalloc(sizeof(*faux_obj), GFP_KERNEL);
> +	if (!faux_obj)
> +		return NULL;
> +
> +	/* Save off the callbacks so we can use them in the future */
> +	faux_obj->faux_ops = faux_ops;
> +
> +	/* 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;
> +	if (parent)
> +		dev->parent = parent;
> +	else
> +		dev->parent = &faux_bus_root;
> +	dev->bus = &faux_bus_type;
> +	dev->groups = groups;
> +	dev_set_name(dev, "%s", name);
> +
> +	ret = device_add(dev);
> +	if (ret) {
> +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
> +		       __func__, name, ret);
> +		put_device(dev);
> +		return NULL;
> +	}

Now that the probe is synchronous, what do you think about returning
-ENODEV if the device failed to bind to the driver?

This would be useful for modules that may want to unload if the probe
fails.

Greg Kroah-Hartman Feb. 10, 2025, 2:45 p.m. UTC | #2

On Mon, Feb 10, 2025 at 09:29:52AM -0500, Kurt Borja wrote:
> Hi Greg,
> 
> On Mon Feb 10, 2025 at 7:30 AM -05, 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.
> >
> > Reviewed-by: Jonathan Cameron <[e-mail]>
> > Reviewed-by: Danilo Krummrich <[e-mail]>
> > Reviewed-by: Lyude Paul <[e-mail]>
> > Reviewed-by: Thomas Weißschuh <[e-mail]>
> > Signed-off-by: Greg Kroah-Hartman <[e-mail]>
> > ---
> > v4: - really removed the name logic
> >     - added #include <linux/container_of.h> to faux.h
> >     - added parent pointer to api call
> >     - minor documentation updates
> >     - made probe synchronous
> > v3: - loads of documentation updates and rewrites
> >     - added to the documentation build
> >     - removed name[] array as it's no longer needed
> >     - added faux_device_create_with_groups()
> >     - added functions to get/set devdata
> >     - renamed faux_driver_ops -> faux_device_ops
> >     - made faux_device_ops a const *
> >     - minor cleanups
> >     - tested it, again.
> >
> > v2: - renamed bus and root device to just "faux" thanks to Thomas
> >     - removed the one-driver-per-device and now just have one driver
> >       entirely thanks to Danilo
> >     - kerneldoc fixups and additions and string handling bounds checks
> >       thanks to Andy
> >     - coding style fix thanks to Jonathan
> >     - tested that the destroy path actually works
> >  Documentation/driver-api/infrastructure.rst |   6 +
> >  drivers/base/Makefile                       |   2 +-
> >  drivers/base/base.h                         |   1 +
> >  drivers/base/faux.c                         | 232 ++++++++++++++++++++
> >  drivers/base/init.c                         |   1 +
> >  include/linux/device/faux.h                 |  69 ++++++
> >  6 files changed, 310 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/base/faux.c
> >  create mode 100644 include/linux/device/faux.h
> >
> > diff --git a/Documentation/driver-api/infrastructure.rst b/Documentation/driver-api/infrastructure.rst
> > index 3d52dfdfa9fd..35e36fee4238 100644
> > --- a/Documentation/driver-api/infrastructure.rst
> > +++ b/Documentation/driver-api/infrastructure.rst
> > @@ -41,6 +41,12 @@ Device Drivers Base
> >  .. kernel-doc:: drivers/base/class.c
> >     :export:
> >  
> > +.. kernel-doc:: include/linux/device/faux.h
> > +   :internal:
> > +
> > +.. kernel-doc:: drivers/base/faux.c
> > +   :export:
> > +
> >  .. kernel-doc:: drivers/base/node.c
> >     :internal:
> >  
> > 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..531e9d789ee0
> > --- /dev/null
> > +++ b/drivers/base/faux.c
> > @@ -0,0 +1,232 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
> > + * Copyright (c) 2025 The Linux Foundation
> > + *
> > + * A "simple" faux bus that allows devices to be created and added
> > + * automatically to it.  This is to be used whenever you need to create a
> > + * device that is not associated with any "real" system resources, and do
> > + * not want to have to deal with a bus/driver binding logic.  It is
> > + * intended to be very simple, with only a create and a destroy function
> > + * available.
> > + */
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/container_of.h>
> > +#include <linux/device/faux.h>
> > +#include "base.h"
> > +
> > +/*
> > + * Internal wrapper structure so we can hold a pointer to the
> > + * faux_device_ops for this device.
> > + */
> > +struct faux_object {
> > +	struct faux_device faux_dev;
> > +	const struct faux_device_ops *faux_ops;
> > +};
> > +#define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
> > +
> > +static struct device faux_bus_root = {
> > +	.init_name	= "faux",
> > +};
> > +
> > +static int faux_match(struct device *dev, const struct device_driver *drv)
> > +{
> > +	/* Match always succeeds, we only have one driver */
> > +	return 1;
> > +}
> > +
> > +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_device_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_device_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",
> > +	.match		= faux_match,
> > +	.probe		= faux_probe,
> > +	.remove		= faux_remove,
> > +};
> > +
> > +static struct device_driver faux_driver = {
> > +	.name		= "faux_driver",
> > +	.bus		= &faux_bus_type,
> > +	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
> > +};
> > +
> > +static void faux_device_release(struct device *dev)
> > +{
> > +	struct faux_object *faux_obj = to_faux_object(dev);
> > +
> > +	kfree(faux_obj);
> > +}
> > +
> > +/**
> > + * faux_device_create_with_groups - Create and register with the driver
> > + *		core a faux device and populate the device with an initial
> > + *		set of sysfs attributes.
> > + * @name:	The name of the device we are adding, must be unique for
> > + *		all faux devices.
> > + * @parent:	Pointer to a potential parent struct device.  If set to
> > + *		NULL, the device will be created in the "root" of the faux
> > + *		device tree in sysfs.
> > + * @faux_ops:	struct faux_device_ops that the new device will call back
> > + *		into, can be NULL.
> > + * @groups:	The set of sysfs attributes that will be created for this
> > + *		device when it is registered with the driver core.
> > + *
> > + * Create a new faux device and register it in the driver core properly.
> > + * If present, callbacks in @faux_ops will be called with the device that
> > + * for the caller to do something with at the proper time given the
> > + * device's lifecycle.
> > + *
> > + * Note, when this function is called, the functions specified in struct
> > + * faux_ops can be called before the function returns, so be prepared for
> > + * everything to be properly initialized before that point in time.
> > + *
> > + * Return:
> > + * * NULL if an error happened with creating the device
> > + * * pointer to a valid struct faux_device that is registered with sysfs
> > + */
> > +struct faux_device *faux_device_create_with_groups(const char *name,
> > +						   struct device *parent,
> > +						   const struct faux_device_ops *faux_ops,
> > +						   const struct attribute_group **groups)
> > +{
> > +	struct faux_object *faux_obj;
> > +	struct faux_device *faux_dev;
> > +	struct device *dev;
> > +	int ret;
> > +
> > +	faux_obj = kzalloc(sizeof(*faux_obj), GFP_KERNEL);
> > +	if (!faux_obj)
> > +		return NULL;
> > +
> > +	/* Save off the callbacks so we can use them in the future */
> > +	faux_obj->faux_ops = faux_ops;
> > +
> > +	/* 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;
> > +	if (parent)
> > +		dev->parent = parent;
> > +	else
> > +		dev->parent = &faux_bus_root;
> > +	dev->bus = &faux_bus_type;
> > +	dev->groups = groups;
> > +	dev_set_name(dev, "%s", name);
> > +
> > +	ret = device_add(dev);
> > +	if (ret) {
> > +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
> > +		       __func__, name, ret);
> > +		put_device(dev);
> > +		return NULL;
> > +	}
> 
> Now that the probe is synchronous, what do you think about returning
> -ENODEV if the device failed to bind to the driver?

Nope, that would make all callers have to deal with a pointer or a NULL,
or an error value encorporated in a pointer.

> This would be useful for modules that may want to unload if the probe
> fails.

Then just test for NULL, there's nothing preventing that, right?

Also, the only way probe can fail is if the probe you passed into the
call fails.  Or if you picked a name that is already in the system, both
of which you have full control over.

thanks,

greg k-h

Kurt Borja Feb. 10, 2025, 2:58 p.m. UTC | #3

On Mon Feb 10, 2025 at 9:45 AM -05, Greg Kroah-Hartman wrote:
> On Mon, Feb 10, 2025 at 09:29:52AM -0500, Kurt Borja wrote:
>> Hi Greg,
>> 
>> On Mon Feb 10, 2025 at 7:30 AM -05, 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.
>> >
>> > Reviewed-by: Jonathan Cameron <[e-mail]>
>> > Reviewed-by: Danilo Krummrich <[e-mail]>
>> > Reviewed-by: Lyude Paul <[e-mail]>
>> > Reviewed-by: Thomas Weißschuh <[e-mail]>
>> > Signed-off-by: Greg Kroah-Hartman <[e-mail]>
>> > ---
>> > v4: - really removed the name logic
>> >     - added #include <linux/container_of.h> to faux.h
>> >     - added parent pointer to api call
>> >     - minor documentation updates
>> >     - made probe synchronous
>> > v3: - loads of documentation updates and rewrites
>> >     - added to the documentation build
>> >     - removed name[] array as it's no longer needed
>> >     - added faux_device_create_with_groups()
>> >     - added functions to get/set devdata
>> >     - renamed faux_driver_ops -> faux_device_ops
>> >     - made faux_device_ops a const *
>> >     - minor cleanups
>> >     - tested it, again.
>> >
>> > v2: - renamed bus and root device to just "faux" thanks to Thomas
>> >     - removed the one-driver-per-device and now just have one driver
>> >       entirely thanks to Danilo
>> >     - kerneldoc fixups and additions and string handling bounds checks
>> >       thanks to Andy
>> >     - coding style fix thanks to Jonathan
>> >     - tested that the destroy path actually works
>> >  Documentation/driver-api/infrastructure.rst |   6 +
>> >  drivers/base/Makefile                       |   2 +-
>> >  drivers/base/base.h                         |   1 +
>> >  drivers/base/faux.c                         | 232 ++++++++++++++++++++
>> >  drivers/base/init.c                         |   1 +
>> >  include/linux/device/faux.h                 |  69 ++++++
>> >  6 files changed, 310 insertions(+), 1 deletion(-)
>> >  create mode 100644 drivers/base/faux.c
>> >  create mode 100644 include/linux/device/faux.h
>> >
>> > diff --git a/Documentation/driver-api/infrastructure.rst b/Documentation/driver-api/infrastructure.rst
>> > index 3d52dfdfa9fd..35e36fee4238 100644
>> > --- a/Documentation/driver-api/infrastructure.rst
>> > +++ b/Documentation/driver-api/infrastructure.rst
>> > @@ -41,6 +41,12 @@ Device Drivers Base
>> >  .. kernel-doc:: drivers/base/class.c
>> >     :export:
>> >  
>> > +.. kernel-doc:: include/linux/device/faux.h
>> > +   :internal:
>> > +
>> > +.. kernel-doc:: drivers/base/faux.c
>> > +   :export:
>> > +
>> >  .. kernel-doc:: drivers/base/node.c
>> >     :internal:
>> >  
>> > 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..531e9d789ee0
>> > --- /dev/null
>> > +++ b/drivers/base/faux.c
>> > @@ -0,0 +1,232 @@
>> > +// SPDX-License-Identifier: GPL-2.0-only
>> > +/*
>> > + * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
>> > + * Copyright (c) 2025 The Linux Foundation
>> > + *
>> > + * A "simple" faux bus that allows devices to be created and added
>> > + * automatically to it.  This is to be used whenever you need to create a
>> > + * device that is not associated with any "real" system resources, and do
>> > + * not want to have to deal with a bus/driver binding logic.  It is
>> > + * intended to be very simple, with only a create and a destroy function
>> > + * available.
>> > + */
>> > +#include <linux/err.h>
>> > +#include <linux/init.h>
>> > +#include <linux/slab.h>
>> > +#include <linux/string.h>
>> > +#include <linux/container_of.h>
>> > +#include <linux/device/faux.h>
>> > +#include "base.h"
>> > +
>> > +/*
>> > + * Internal wrapper structure so we can hold a pointer to the
>> > + * faux_device_ops for this device.
>> > + */
>> > +struct faux_object {
>> > +	struct faux_device faux_dev;
>> > +	const struct faux_device_ops *faux_ops;
>> > +};
>> > +#define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
>> > +
>> > +static struct device faux_bus_root = {
>> > +	.init_name	= "faux",
>> > +};
>> > +
>> > +static int faux_match(struct device *dev, const struct device_driver *drv)
>> > +{
>> > +	/* Match always succeeds, we only have one driver */
>> > +	return 1;
>> > +}
>> > +
>> > +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_device_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_device_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",
>> > +	.match		= faux_match,
>> > +	.probe		= faux_probe,
>> > +	.remove		= faux_remove,
>> > +};
>> > +
>> > +static struct device_driver faux_driver = {
>> > +	.name		= "faux_driver",
>> > +	.bus		= &faux_bus_type,
>> > +	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
>> > +};
>> > +
>> > +static void faux_device_release(struct device *dev)
>> > +{
>> > +	struct faux_object *faux_obj = to_faux_object(dev);
>> > +
>> > +	kfree(faux_obj);
>> > +}
>> > +
>> > +/**
>> > + * faux_device_create_with_groups - Create and register with the driver
>> > + *		core a faux device and populate the device with an initial
>> > + *		set of sysfs attributes.
>> > + * @name:	The name of the device we are adding, must be unique for
>> > + *		all faux devices.
>> > + * @parent:	Pointer to a potential parent struct device.  If set to
>> > + *		NULL, the device will be created in the "root" of the faux
>> > + *		device tree in sysfs.
>> > + * @faux_ops:	struct faux_device_ops that the new device will call back
>> > + *		into, can be NULL.
>> > + * @groups:	The set of sysfs attributes that will be created for this
>> > + *		device when it is registered with the driver core.
>> > + *
>> > + * Create a new faux device and register it in the driver core properly.
>> > + * If present, callbacks in @faux_ops will be called with the device that
>> > + * for the caller to do something with at the proper time given the
>> > + * device's lifecycle.
>> > + *
>> > + * Note, when this function is called, the functions specified in struct
>> > + * faux_ops can be called before the function returns, so be prepared for
>> > + * everything to be properly initialized before that point in time.
>> > + *
>> > + * Return:
>> > + * * NULL if an error happened with creating the device
>> > + * * pointer to a valid struct faux_device that is registered with sysfs
>> > + */
>> > +struct faux_device *faux_device_create_with_groups(const char *name,
>> > +						   struct device *parent,
>> > +						   const struct faux_device_ops *faux_ops,
>> > +						   const struct attribute_group **groups)
>> > +{
>> > +	struct faux_object *faux_obj;
>> > +	struct faux_device *faux_dev;
>> > +	struct device *dev;
>> > +	int ret;
>> > +
>> > +	faux_obj = kzalloc(sizeof(*faux_obj), GFP_KERNEL);
>> > +	if (!faux_obj)
>> > +		return NULL;
>> > +
>> > +	/* Save off the callbacks so we can use them in the future */
>> > +	faux_obj->faux_ops = faux_ops;
>> > +
>> > +	/* 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;
>> > +	if (parent)
>> > +		dev->parent = parent;
>> > +	else
>> > +		dev->parent = &faux_bus_root;
>> > +	dev->bus = &faux_bus_type;
>> > +	dev->groups = groups;
>> > +	dev_set_name(dev, "%s", name);
>> > +
>> > +	ret = device_add(dev);
>> > +	if (ret) {
>> > +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
>> > +		       __func__, name, ret);
>> > +		put_device(dev);
>> > +		return NULL;
>> > +	}
>> 
>> Now that the probe is synchronous, what do you think about returning
>> -ENODEV if the device failed to bind to the driver?
>
> Nope, that would make all callers have to deal with a pointer or a NULL,
> or an error value encorporated in a pointer.

Right! I thought for a sec this function returned ERR_PTRs.

>
>> This would be useful for modules that may want to unload if the probe
>> fails.
>
> Then just test for NULL, there's nothing preventing that, right?

Please, correct me if I'm wrong. If the probe the user provided fails
the device would still be added successfully to the bus. That means this
function would return a valid pointer and the module has no way of
knowing if the probe succeeded in an __init section.

>
> Also, the only way probe can fail is if the probe you passed into the
> call fails.  Or if you picked a name that is already in the system, both

Exactly. I'm thinking of modules that are very simple and just care
about the probe succeeding, so having the device hanging around in the
bus and the module loaded would be a waste of resources.

Greg Kroah-Hartman Feb. 10, 2025, 3:36 p.m. UTC | #4

On Mon, Feb 10, 2025 at 09:58:24AM -0500, Kurt Borja wrote:
> On Mon Feb 10, 2025 at 9:45 AM -05, Greg Kroah-Hartman wrote:
> > On Mon, Feb 10, 2025 at 09:29:52AM -0500, Kurt Borja wrote:
> >> Hi Greg,
> >> 
> >> On Mon Feb 10, 2025 at 7:30 AM -05, 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.
> >> >
> >> > Reviewed-by: Jonathan Cameron <[e-mail]>
> >> > Reviewed-by: Danilo Krummrich <[e-mail]>
> >> > Reviewed-by: Lyude Paul <[e-mail]>
> >> > Reviewed-by: Thomas Weißschuh <[e-mail]>
> >> > Signed-off-by: Greg Kroah-Hartman <[e-mail]>
> >> > ---
> >> > v4: - really removed the name logic
> >> >     - added #include <linux/container_of.h> to faux.h
> >> >     - added parent pointer to api call
> >> >     - minor documentation updates
> >> >     - made probe synchronous
> >> > v3: - loads of documentation updates and rewrites
> >> >     - added to the documentation build
> >> >     - removed name[] array as it's no longer needed
> >> >     - added faux_device_create_with_groups()
> >> >     - added functions to get/set devdata
> >> >     - renamed faux_driver_ops -> faux_device_ops
> >> >     - made faux_device_ops a const *
> >> >     - minor cleanups
> >> >     - tested it, again.
> >> >
> >> > v2: - renamed bus and root device to just "faux" thanks to Thomas
> >> >     - removed the one-driver-per-device and now just have one driver
> >> >       entirely thanks to Danilo
> >> >     - kerneldoc fixups and additions and string handling bounds checks
> >> >       thanks to Andy
> >> >     - coding style fix thanks to Jonathan
> >> >     - tested that the destroy path actually works
> >> >  Documentation/driver-api/infrastructure.rst |   6 +
> >> >  drivers/base/Makefile                       |   2 +-
> >> >  drivers/base/base.h                         |   1 +
> >> >  drivers/base/faux.c                         | 232 ++++++++++++++++++++
> >> >  drivers/base/init.c                         |   1 +
> >> >  include/linux/device/faux.h                 |  69 ++++++
> >> >  6 files changed, 310 insertions(+), 1 deletion(-)
> >> >  create mode 100644 drivers/base/faux.c
> >> >  create mode 100644 include/linux/device/faux.h
> >> >
> >> > diff --git a/Documentation/driver-api/infrastructure.rst b/Documentation/driver-api/infrastructure.rst
> >> > index 3d52dfdfa9fd..35e36fee4238 100644
> >> > --- a/Documentation/driver-api/infrastructure.rst
> >> > +++ b/Documentation/driver-api/infrastructure.rst
> >> > @@ -41,6 +41,12 @@ Device Drivers Base
> >> >  .. kernel-doc:: drivers/base/class.c
> >> >     :export:
> >> >  
> >> > +.. kernel-doc:: include/linux/device/faux.h
> >> > +   :internal:
> >> > +
> >> > +.. kernel-doc:: drivers/base/faux.c
> >> > +   :export:
> >> > +
> >> >  .. kernel-doc:: drivers/base/node.c
> >> >     :internal:
> >> >  
> >> > 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..531e9d789ee0
> >> > --- /dev/null
> >> > +++ b/drivers/base/faux.c
> >> > @@ -0,0 +1,232 @@
> >> > +// SPDX-License-Identifier: GPL-2.0-only
> >> > +/*
> >> > + * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
> >> > + * Copyright (c) 2025 The Linux Foundation
> >> > + *
> >> > + * A "simple" faux bus that allows devices to be created and added
> >> > + * automatically to it.  This is to be used whenever you need to create a
> >> > + * device that is not associated with any "real" system resources, and do
> >> > + * not want to have to deal with a bus/driver binding logic.  It is
> >> > + * intended to be very simple, with only a create and a destroy function
> >> > + * available.
> >> > + */
> >> > +#include <linux/err.h>
> >> > +#include <linux/init.h>
> >> > +#include <linux/slab.h>
> >> > +#include <linux/string.h>
> >> > +#include <linux/container_of.h>
> >> > +#include <linux/device/faux.h>
> >> > +#include "base.h"
> >> > +
> >> > +/*
> >> > + * Internal wrapper structure so we can hold a pointer to the
> >> > + * faux_device_ops for this device.
> >> > + */
> >> > +struct faux_object {
> >> > +	struct faux_device faux_dev;
> >> > +	const struct faux_device_ops *faux_ops;
> >> > +};
> >> > +#define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
> >> > +
> >> > +static struct device faux_bus_root = {
> >> > +	.init_name	= "faux",
> >> > +};
> >> > +
> >> > +static int faux_match(struct device *dev, const struct device_driver *drv)
> >> > +{
> >> > +	/* Match always succeeds, we only have one driver */
> >> > +	return 1;
> >> > +}
> >> > +
> >> > +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_device_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_device_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",
> >> > +	.match		= faux_match,
> >> > +	.probe		= faux_probe,
> >> > +	.remove		= faux_remove,
> >> > +};
> >> > +
> >> > +static struct device_driver faux_driver = {
> >> > +	.name		= "faux_driver",
> >> > +	.bus		= &faux_bus_type,
> >> > +	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
> >> > +};
> >> > +
> >> > +static void faux_device_release(struct device *dev)
> >> > +{
> >> > +	struct faux_object *faux_obj = to_faux_object(dev);
> >> > +
> >> > +	kfree(faux_obj);
> >> > +}
> >> > +
> >> > +/**
> >> > + * faux_device_create_with_groups - Create and register with the driver
> >> > + *		core a faux device and populate the device with an initial
> >> > + *		set of sysfs attributes.
> >> > + * @name:	The name of the device we are adding, must be unique for
> >> > + *		all faux devices.
> >> > + * @parent:	Pointer to a potential parent struct device.  If set to
> >> > + *		NULL, the device will be created in the "root" of the faux
> >> > + *		device tree in sysfs.
> >> > + * @faux_ops:	struct faux_device_ops that the new device will call back
> >> > + *		into, can be NULL.
> >> > + * @groups:	The set of sysfs attributes that will be created for this
> >> > + *		device when it is registered with the driver core.
> >> > + *
> >> > + * Create a new faux device and register it in the driver core properly.
> >> > + * If present, callbacks in @faux_ops will be called with the device that
> >> > + * for the caller to do something with at the proper time given the
> >> > + * device's lifecycle.
> >> > + *
> >> > + * Note, when this function is called, the functions specified in struct
> >> > + * faux_ops can be called before the function returns, so be prepared for
> >> > + * everything to be properly initialized before that point in time.
> >> > + *
> >> > + * Return:
> >> > + * * NULL if an error happened with creating the device
> >> > + * * pointer to a valid struct faux_device that is registered with sysfs
> >> > + */
> >> > +struct faux_device *faux_device_create_with_groups(const char *name,
> >> > +						   struct device *parent,
> >> > +						   const struct faux_device_ops *faux_ops,
> >> > +						   const struct attribute_group **groups)
> >> > +{
> >> > +	struct faux_object *faux_obj;
> >> > +	struct faux_device *faux_dev;
> >> > +	struct device *dev;
> >> > +	int ret;
> >> > +
> >> > +	faux_obj = kzalloc(sizeof(*faux_obj), GFP_KERNEL);
> >> > +	if (!faux_obj)
> >> > +		return NULL;
> >> > +
> >> > +	/* Save off the callbacks so we can use them in the future */
> >> > +	faux_obj->faux_ops = faux_ops;
> >> > +
> >> > +	/* 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;
> >> > +	if (parent)
> >> > +		dev->parent = parent;
> >> > +	else
> >> > +		dev->parent = &faux_bus_root;
> >> > +	dev->bus = &faux_bus_type;
> >> > +	dev->groups = groups;
> >> > +	dev_set_name(dev, "%s", name);
> >> > +
> >> > +	ret = device_add(dev);
> >> > +	if (ret) {
> >> > +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
> >> > +		       __func__, name, ret);
> >> > +		put_device(dev);
> >> > +		return NULL;
> >> > +	}
> >> 
> >> Now that the probe is synchronous, what do you think about returning
> >> -ENODEV if the device failed to bind to the driver?
> >
> > Nope, that would make all callers have to deal with a pointer or a NULL,
> > or an error value encorporated in a pointer.
> 
> Right! I thought for a sec this function returned ERR_PTRs.
> 
> >
> >> This would be useful for modules that may want to unload if the probe
> >> fails.
> >
> > Then just test for NULL, there's nothing preventing that, right?
> 
> Please, correct me if I'm wrong. If the probe the user provided fails
> the device would still be added successfully to the bus. That means this
> function would return a valid pointer and the module has no way of
> knowing if the probe succeeded in an __init section.

Ah, yes, you are totally correct here.  You don't know if the probe
failed or not.  But your device is still "live" and you can only get rid
of it by calling faux_device_destroy(), all that might be wrong with it
is that it's not really associated with the bus.

You can still assign resources to it, AND the resources will be freed up
when the device goes away (see the comment in device_release() for
specifics as to this happening today for platform devices.)

I guess we can test for this and handle it, if you feel it is necessary,
it wouldn't be hard to do so, but let me add this later as it's the same
problem with platform devices and odds are we want to fix that issue up
there too, right?

> > Also, the only way probe can fail is if the probe you passed into the
> > call fails.  Or if you picked a name that is already in the system, both
> 
> Exactly. I'm thinking of modules that are very simple and just care
> about the probe succeeding, so having the device hanging around in the
> bus and the module loaded would be a waste of resources.

Modules usually don't need to do the probe callback at all.  I show one
example in this series (the regulator dummy driver), but it can be
easily rewritten to not need that at all.

And bonus, the rust binding doesn't allow you to provide a probe()
callback that could fail, so any code written using that will not have
this issue at all :)

thanks,

greg k-h

Kurt Borja Feb. 10, 2025, 3:52 p.m. UTC | #5

On Mon Feb 10, 2025 at 10:36 AM -05, Greg Kroah-Hartman wrote:
> On Mon, Feb 10, 2025 at 09:58:24AM -0500, Kurt Borja wrote:
>> On Mon Feb 10, 2025 at 9:45 AM -05, Greg Kroah-Hartman wrote:
>> > On Mon, Feb 10, 2025 at 09:29:52AM -0500, Kurt Borja wrote:
>> >> Hi Greg,
>> >> 
>> >> On Mon Feb 10, 2025 at 7:30 AM -05, 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.
>> >> >
>> >> > Reviewed-by: Jonathan Cameron <[e-mail]>
>> >> > Reviewed-by: Danilo Krummrich <[e-mail]>
>> >> > Reviewed-by: Lyude Paul <[e-mail]>
>> >> > Reviewed-by: Thomas Weißschuh <[e-mail]>
>> >> > Signed-off-by: Greg Kroah-Hartman <[e-mail]>
>> >> > ---
>> >> > v4: - really removed the name logic
>> >> >     - added #include <linux/container_of.h> to faux.h
>> >> >     - added parent pointer to api call
>> >> >     - minor documentation updates
>> >> >     - made probe synchronous
>> >> > v3: - loads of documentation updates and rewrites
>> >> >     - added to the documentation build
>> >> >     - removed name[] array as it's no longer needed
>> >> >     - added faux_device_create_with_groups()
>> >> >     - added functions to get/set devdata
>> >> >     - renamed faux_driver_ops -> faux_device_ops
>> >> >     - made faux_device_ops a const *
>> >> >     - minor cleanups
>> >> >     - tested it, again.
>> >> >
>> >> > v2: - renamed bus and root device to just "faux" thanks to Thomas
>> >> >     - removed the one-driver-per-device and now just have one driver
>> >> >       entirely thanks to Danilo
>> >> >     - kerneldoc fixups and additions and string handling bounds checks
>> >> >       thanks to Andy
>> >> >     - coding style fix thanks to Jonathan
>> >> >     - tested that the destroy path actually works
>> >> >  Documentation/driver-api/infrastructure.rst |   6 +
>> >> >  drivers/base/Makefile                       |   2 +-
>> >> >  drivers/base/base.h                         |   1 +
>> >> >  drivers/base/faux.c                         | 232 ++++++++++++++++++++
>> >> >  drivers/base/init.c                         |   1 +
>> >> >  include/linux/device/faux.h                 |  69 ++++++
>> >> >  6 files changed, 310 insertions(+), 1 deletion(-)
>> >> >  create mode 100644 drivers/base/faux.c
>> >> >  create mode 100644 include/linux/device/faux.h
>> >> >
>> >> > diff --git a/Documentation/driver-api/infrastructure.rst b/Documentation/driver-api/infrastructure.rst
>> >> > index 3d52dfdfa9fd..35e36fee4238 100644
>> >> > --- a/Documentation/driver-api/infrastructure.rst
>> >> > +++ b/Documentation/driver-api/infrastructure.rst
>> >> > @@ -41,6 +41,12 @@ Device Drivers Base
>> >> >  .. kernel-doc:: drivers/base/class.c
>> >> >     :export:
>> >> >  
>> >> > +.. kernel-doc:: include/linux/device/faux.h
>> >> > +   :internal:
>> >> > +
>> >> > +.. kernel-doc:: drivers/base/faux.c
>> >> > +   :export:
>> >> > +
>> >> >  .. kernel-doc:: drivers/base/node.c
>> >> >     :internal:
>> >> >  
>> >> > 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..531e9d789ee0
>> >> > --- /dev/null
>> >> > +++ b/drivers/base/faux.c
>> >> > @@ -0,0 +1,232 @@
>> >> > +// SPDX-License-Identifier: GPL-2.0-only
>> >> > +/*
>> >> > + * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
>> >> > + * Copyright (c) 2025 The Linux Foundation
>> >> > + *
>> >> > + * A "simple" faux bus that allows devices to be created and added
>> >> > + * automatically to it.  This is to be used whenever you need to create a
>> >> > + * device that is not associated with any "real" system resources, and do
>> >> > + * not want to have to deal with a bus/driver binding logic.  It is
>> >> > + * intended to be very simple, with only a create and a destroy function
>> >> > + * available.
>> >> > + */
>> >> > +#include <linux/err.h>
>> >> > +#include <linux/init.h>
>> >> > +#include <linux/slab.h>
>> >> > +#include <linux/string.h>
>> >> > +#include <linux/container_of.h>
>> >> > +#include <linux/device/faux.h>
>> >> > +#include "base.h"
>> >> > +
>> >> > +/*
>> >> > + * Internal wrapper structure so we can hold a pointer to the
>> >> > + * faux_device_ops for this device.
>> >> > + */
>> >> > +struct faux_object {
>> >> > +	struct faux_device faux_dev;
>> >> > +	const struct faux_device_ops *faux_ops;
>> >> > +};
>> >> > +#define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
>> >> > +
>> >> > +static struct device faux_bus_root = {
>> >> > +	.init_name	= "faux",
>> >> > +};
>> >> > +
>> >> > +static int faux_match(struct device *dev, const struct device_driver *drv)
>> >> > +{
>> >> > +	/* Match always succeeds, we only have one driver */
>> >> > +	return 1;
>> >> > +}
>> >> > +
>> >> > +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_device_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_device_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",
>> >> > +	.match		= faux_match,
>> >> > +	.probe		= faux_probe,
>> >> > +	.remove		= faux_remove,
>> >> > +};
>> >> > +
>> >> > +static struct device_driver faux_driver = {
>> >> > +	.name		= "faux_driver",
>> >> > +	.bus		= &faux_bus_type,
>> >> > +	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
>> >> > +};
>> >> > +
>> >> > +static void faux_device_release(struct device *dev)
>> >> > +{
>> >> > +	struct faux_object *faux_obj = to_faux_object(dev);
>> >> > +
>> >> > +	kfree(faux_obj);
>> >> > +}
>> >> > +
>> >> > +/**
>> >> > + * faux_device_create_with_groups - Create and register with the driver
>> >> > + *		core a faux device and populate the device with an initial
>> >> > + *		set of sysfs attributes.
>> >> > + * @name:	The name of the device we are adding, must be unique for
>> >> > + *		all faux devices.
>> >> > + * @parent:	Pointer to a potential parent struct device.  If set to
>> >> > + *		NULL, the device will be created in the "root" of the faux
>> >> > + *		device tree in sysfs.
>> >> > + * @faux_ops:	struct faux_device_ops that the new device will call back
>> >> > + *		into, can be NULL.
>> >> > + * @groups:	The set of sysfs attributes that will be created for this
>> >> > + *		device when it is registered with the driver core.
>> >> > + *
>> >> > + * Create a new faux device and register it in the driver core properly.
>> >> > + * If present, callbacks in @faux_ops will be called with the device that
>> >> > + * for the caller to do something with at the proper time given the
>> >> > + * device's lifecycle.
>> >> > + *
>> >> > + * Note, when this function is called, the functions specified in struct
>> >> > + * faux_ops can be called before the function returns, so be prepared for
>> >> > + * everything to be properly initialized before that point in time.
>> >> > + *
>> >> > + * Return:
>> >> > + * * NULL if an error happened with creating the device
>> >> > + * * pointer to a valid struct faux_device that is registered with sysfs
>> >> > + */
>> >> > +struct faux_device *faux_device_create_with_groups(const char *name,
>> >> > +						   struct device *parent,
>> >> > +						   const struct faux_device_ops *faux_ops,
>> >> > +						   const struct attribute_group **groups)
>> >> > +{
>> >> > +	struct faux_object *faux_obj;
>> >> > +	struct faux_device *faux_dev;
>> >> > +	struct device *dev;
>> >> > +	int ret;
>> >> > +
>> >> > +	faux_obj = kzalloc(sizeof(*faux_obj), GFP_KERNEL);
>> >> > +	if (!faux_obj)
>> >> > +		return NULL;
>> >> > +
>> >> > +	/* Save off the callbacks so we can use them in the future */
>> >> > +	faux_obj->faux_ops = faux_ops;
>> >> > +
>> >> > +	/* 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;
>> >> > +	if (parent)
>> >> > +		dev->parent = parent;
>> >> > +	else
>> >> > +		dev->parent = &faux_bus_root;
>> >> > +	dev->bus = &faux_bus_type;
>> >> > +	dev->groups = groups;
>> >> > +	dev_set_name(dev, "%s", name);
>> >> > +
>> >> > +	ret = device_add(dev);
>> >> > +	if (ret) {
>> >> > +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
>> >> > +		       __func__, name, ret);
>> >> > +		put_device(dev);
>> >> > +		return NULL;
>> >> > +	}
>> >> 
>> >> Now that the probe is synchronous, what do you think about returning
>> >> -ENODEV if the device failed to bind to the driver?
>> >
>> > Nope, that would make all callers have to deal with a pointer or a NULL,
>> > or an error value encorporated in a pointer.
>> 
>> Right! I thought for a sec this function returned ERR_PTRs.
>> 
>> >
>> >> This would be useful for modules that may want to unload if the probe
>> >> fails.
>> >
>> > Then just test for NULL, there's nothing preventing that, right?
>> 
>> Please, correct me if I'm wrong. If the probe the user provided fails
>> the device would still be added successfully to the bus. That means this
>> function would return a valid pointer and the module has no way of
>> knowing if the probe succeeded in an __init section.
>
> Ah, yes, you are totally correct here.  You don't know if the probe
> failed or not.  But your device is still "live" and you can only get rid
> of it by calling faux_device_destroy(), all that might be wrong with it
> is that it's not really associated with the bus.

I'm curious of what happens with the sysfs groups you pass if the probe
fails. Do they get assigned on driver attachment or right after
device_add()? Because those sysfs attributes may depend on resources
initialized on the probe.

>
> You can still assign resources to it, AND the resources will be freed up
> when the device goes away (see the comment in device_release() for
> specifics as to this happening today for platform devices.)
>
> I guess we can test for this and handle it, if you feel it is necessary,
> it wouldn't be hard to do so, but let me add this later as it's the same
> problem with platform devices and odds are we want to fix that issue up
> there too, right?

Platform devices have platform_create_bundle() which does check if the
probe succeeded.

>
>> > Also, the only way probe can fail is if the probe you passed into the
>> > call fails.  Or if you picked a name that is already in the system, both
>> 
>> Exactly. I'm thinking of modules that are very simple and just care
>> about the probe succeeding, so having the device hanging around in the
>> bus and the module loaded would be a waste of resources.
>
> Modules usually don't need to do the probe callback at all.  I show one
> example in this series (the regulator dummy driver), but it can be
> easily rewritten to not need that at all.

This is a good point, but from a developer standpoint I would always try
to use the probe callback. This API seems to suggest that's the
appropiate use.

Also it would be amazing if the probe could reside in an __init section.

>
> And bonus, the rust binding doesn't allow you to provide a probe()
> callback that could fail, so any code written using that will not have
> this issue at all :)

This is very interesting. I will check the bindings and learn some rust
in the process :)

Kurt Borja Feb. 10, 2025, 5:56 p.m. UTC | #6

On Mon Feb 10, 2025 at 7:30 AM -05, 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.
>
> Reviewed-by: Jonathan Cameron <[e-mail]>
> Reviewed-by: Danilo Krummrich <[e-mail]>
> Reviewed-by: Lyude Paul <[e-mail]>
> Reviewed-by: Thomas Weißschuh <[e-mail]>
> Signed-off-by: Greg Kroah-Hartman <[e-mail]>
> ---
> v4: - really removed the name logic
>     - added #include <linux/container_of.h> to faux.h
>     - added parent pointer to api call
>     - minor documentation updates
>     - made probe synchronous
> v3: - loads of documentation updates and rewrites
>     - added to the documentation build
>     - removed name[] array as it's no longer needed
>     - added faux_device_create_with_groups()
>     - added functions to get/set devdata
>     - renamed faux_driver_ops -> faux_device_ops
>     - made faux_device_ops a const *
>     - minor cleanups
>     - tested it, again.
>
> v2: - renamed bus and root device to just "faux" thanks to Thomas
>     - removed the one-driver-per-device and now just have one driver
>       entirely thanks to Danilo
>     - kerneldoc fixups and additions and string handling bounds checks
>       thanks to Andy
>     - coding style fix thanks to Jonathan
>     - tested that the destroy path actually works
>  Documentation/driver-api/infrastructure.rst |   6 +
>  drivers/base/Makefile                       |   2 +-
>  drivers/base/base.h                         |   1 +
>  drivers/base/faux.c                         | 232 ++++++++++++++++++++
>  drivers/base/init.c                         |   1 +
>  include/linux/device/faux.h                 |  69 ++++++
>  6 files changed, 310 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/base/faux.c
>  create mode 100644 include/linux/device/faux.h
>
> diff --git a/Documentation/driver-api/infrastructure.rst b/Documentation/driver-api/infrastructure.rst
> index 3d52dfdfa9fd..35e36fee4238 100644
> --- a/Documentation/driver-api/infrastructure.rst
> +++ b/Documentation/driver-api/infrastructure.rst
> @@ -41,6 +41,12 @@ Device Drivers Base
>  .. kernel-doc:: drivers/base/class.c
>     :export:
>  
> +.. kernel-doc:: include/linux/device/faux.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/base/faux.c
> +   :export:
> +
>  .. kernel-doc:: drivers/base/node.c
>     :internal:
>  
> 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..531e9d789ee0
> --- /dev/null
> +++ b/drivers/base/faux.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
> + * Copyright (c) 2025 The Linux Foundation
> + *
> + * A "simple" faux bus that allows devices to be created and added
> + * automatically to it.  This is to be used whenever you need to create a
> + * device that is not associated with any "real" system resources, and do
> + * not want to have to deal with a bus/driver binding logic.  It is
> + * intended to be very simple, with only a create and a destroy function
> + * available.
> + */
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/container_of.h>
> +#include <linux/device/faux.h>
> +#include "base.h"
> +
> +/*
> + * Internal wrapper structure so we can hold a pointer to the
> + * faux_device_ops for this device.
> + */
> +struct faux_object {
> +	struct faux_device faux_dev;
> +	const struct faux_device_ops *faux_ops;
> +};
> +#define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
> +
> +static struct device faux_bus_root = {
> +	.init_name	= "faux",
> +};
> +
> +static int faux_match(struct device *dev, const struct device_driver *drv)
> +{
> +	/* Match always succeeds, we only have one driver */
> +	return 1;
> +}
> +
> +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_device_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_device_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",
> +	.match		= faux_match,
> +	.probe		= faux_probe,
> +	.remove		= faux_remove,
> +};
> +
> +static struct device_driver faux_driver = {
> +	.name		= "faux_driver",
> +	.bus		= &faux_bus_type,
> +	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
> +};
> +
> +static void faux_device_release(struct device *dev)
> +{
> +	struct faux_object *faux_obj = to_faux_object(dev);
> +
> +	kfree(faux_obj);
> +}
> +
> +/**
> + * faux_device_create_with_groups - Create and register with the driver
> + *		core a faux device and populate the device with an initial
> + *		set of sysfs attributes.
> + * @name:	The name of the device we are adding, must be unique for
> + *		all faux devices.
> + * @parent:	Pointer to a potential parent struct device.  If set to
> + *		NULL, the device will be created in the "root" of the faux
> + *		device tree in sysfs.
> + * @faux_ops:	struct faux_device_ops that the new device will call back
> + *		into, can be NULL.
> + * @groups:	The set of sysfs attributes that will be created for this
> + *		device when it is registered with the driver core.
> + *
> + * Create a new faux device and register it in the driver core properly.
> + * If present, callbacks in @faux_ops will be called with the device that
> + * for the caller to do something with at the proper time given the
> + * device's lifecycle.
> + *
> + * Note, when this function is called, the functions specified in struct
> + * faux_ops can be called before the function returns, so be prepared for
> + * everything to be properly initialized before that point in time.
> + *
> + * Return:
> + * * NULL if an error happened with creating the device
> + * * pointer to a valid struct faux_device that is registered with sysfs
> + */
> +struct faux_device *faux_device_create_with_groups(const char *name,
> +						   struct device *parent,
> +						   const struct faux_device_ops *faux_ops,
> +						   const struct attribute_group **groups)
> +{
> +	struct faux_object *faux_obj;
> +	struct faux_device *faux_dev;
> +	struct device *dev;
> +	int ret;
> +
> +	faux_obj = kzalloc(sizeof(*faux_obj), GFP_KERNEL);
> +	if (!faux_obj)
> +		return NULL;
> +
> +	/* Save off the callbacks so we can use them in the future */
> +	faux_obj->faux_ops = faux_ops;
> +
> +	/* 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;
> +	if (parent)
> +		dev->parent = parent;
> +	else
> +		dev->parent = &faux_bus_root;
> +	dev->bus = &faux_bus_type;
> +	dev->groups = groups;

Just as I feared, adding groups this way is bug prone if we don't check
if the device attached to the driver successfully. Consider the
following example module:

static attr1_show(...)
{
	struct priv *priv = dev_get_drvdata(dev);
	...
}
DEVICE_ATTR_RO(attr1)

...

static const struct attribute_group faux_groups[] = {
	...
}

/* It would be nice to have the probe in __init */
static int __init faux_probe(struct faux_device *fdev)
{
	...
	/* Probe may fail */
	if (con)
		return -EINVAL;
	...
	faux_device_set_drvdata(fdev, priv);
	...
}

static struct faux_device_ops faux_ops = {
	...
}

static int __init module_init()
{
	...
	fdev = faux_device_create_with_groups("foo", NULL, &faux_ops,
					      &faux_groups);
	if (!fdev)
		return -ENODEV;
}

In this example we have no way of knowing if the probe failed, so the
module as well as the device will remain loaded. Furthermore, the sysfs
groups WILL be added anyway, which is dangerous because we have no
gurantee about drvdata's lifetime nor if it was even initialized
correctly, which sysfs show/store methods may assume is the case.

So we have two problems here. First, this module only cares about the
probe succeeding so keeping the device alive after it fails wastes
resources. Second, we don't have any gurantee about drvdata validity.

> +	dev_set_name(dev, "%s", name);
> +
> +	ret = device_add(dev);
> +	if (ret) {
> +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
> +		       __func__, name, ret);
> +		put_device(dev);
> +		return NULL;
> +	}

To solve this, I suggest we do this here:

	/* Check if the device attached correctly */
	if (dev->driver != &faux_driver)
		return ERR_PTR(-ENODEV); /* or NULL */

	/* 
	 * Add groups after the driver is attached, to avoid lifetime
	 * issues 
	 */
	device_add_groups(dev, groups);

If I'm missing something, or I'm wrong about something let me know!

Zijun Hu Feb. 11, 2025, 2:49 a.m. UTC | #7

On 2/10/2025 8:30 PM, 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.
> 
> Reviewed-by: Jonathan Cameron <[e-mail]>
> Reviewed-by: Danilo Krummrich <[e-mail]>
> Reviewed-by: Lyude Paul <[e-mail]>
> Reviewed-by: Thomas Weißschuh <[e-mail]>
> Signed-off-by: Greg Kroah-Hartman <[e-mail]>
> ---
> v4: - really removed the name logic
>     - added #include <linux/container_of.h> to faux.h
>     - added parent pointer to api call
>     - minor documentation updates
>     - made probe synchronous
> v3: - loads of documentation updates and rewrites
>     - added to the documentation build
>     - removed name[] array as it's no longer needed
>     - added faux_device_create_with_groups()
>     - added functions to get/set devdata
>     - renamed faux_driver_ops -> faux_device_ops
>     - made faux_device_ops a const *
>     - minor cleanups
>     - tested it, again.
> 
> v2: - renamed bus and root device to just "faux" thanks to Thomas
>     - removed the one-driver-per-device and now just have one driver
>       entirely thanks to Danilo
>     - kerneldoc fixups and additions and string handling bounds checks
>       thanks to Andy
>     - coding style fix thanks to Jonathan
>     - tested that the destroy path actually works
>  Documentation/driver-api/infrastructure.rst |   6 +
>  drivers/base/Makefile                       |   2 +-
>  drivers/base/base.h                         |   1 +
>  drivers/base/faux.c                         | 232 ++++++++++++++++++++
>  drivers/base/init.c                         |   1 +
>  include/linux/device/faux.h                 |  69 ++++++
>  6 files changed, 310 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/base/faux.c
>  create mode 100644 include/linux/device/faux.h
> 
> diff --git a/Documentation/driver-api/infrastructure.rst b/Documentation/driver-api/infrastructure.rst
> index 3d52dfdfa9fd..35e36fee4238 100644
> --- a/Documentation/driver-api/infrastructure.rst
> +++ b/Documentation/driver-api/infrastructure.rst
> @@ -41,6 +41,12 @@ Device Drivers Base
>  .. kernel-doc:: drivers/base/class.c
>     :export:
>  
> +.. kernel-doc:: include/linux/device/faux.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/base/faux.c
> +   :export:
> +
>  .. kernel-doc:: drivers/base/node.c
>     :internal:
>  
> 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..531e9d789ee0
> --- /dev/null
> +++ b/drivers/base/faux.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
> + * Copyright (c) 2025 The Linux Foundation
> + *
> + * A "simple" faux bus that allows devices to be created and added
> + * automatically to it.  This is to be used whenever you need to create a
> + * device that is not associated with any "real" system resources, and do
> + * not want to have to deal with a bus/driver binding logic.  It is
> + * intended to be very simple, with only a create and a destroy function
> + * available.
> + */
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/container_of.h>
> +#include <linux/device/faux.h>
> +#include "base.h"
> +
> +/*
> + * Internal wrapper structure so we can hold a pointer to the
> + * faux_device_ops for this device.
> + */
> +struct faux_object {
> +	struct faux_device faux_dev;
> +	const struct faux_device_ops *faux_ops;
> +};
> +#define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
> +
> +static struct device faux_bus_root = {
> +	.init_name	= "faux",
> +};
> +
> +static int faux_match(struct device *dev, const struct device_driver *drv)
> +{
> +	/* Match always succeeds, we only have one driver */
> +	return 1;
> +}
> +
> +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_device_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_device_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",
> +	.match		= faux_match,
> +	.probe		= faux_probe,
> +	.remove		= faux_remove,
> +};
> +
> +static struct device_driver faux_driver = {
> +	.name		= "faux_driver",
> +	.bus		= &faux_bus_type,
> +	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
> +};
> +
> +static void faux_device_release(struct device *dev)
> +{
> +	struct faux_object *faux_obj = to_faux_object(dev);
> +
> +	kfree(faux_obj);
> +}
> +
> +/**
> + * faux_device_create_with_groups - Create and register with the driver
> + *		core a faux device and populate the device with an initial
> + *		set of sysfs attributes.
> + * @name:	The name of the device we are adding, must be unique for
> + *		all faux devices.
> + * @parent:	Pointer to a potential parent struct device.  If set to
> + *		NULL, the device will be created in the "root" of the faux
> + *		device tree in sysfs.
> + * @faux_ops:	struct faux_device_ops that the new device will call back
> + *		into, can be NULL.
> + * @groups:	The set of sysfs attributes that will be created for this
> + *		device when it is registered with the driver core.
> + *
> + * Create a new faux device and register it in the driver core properly.
> + * If present, callbacks in @faux_ops will be called with the device that
> + * for the caller to do something with at the proper time given the
> + * device's lifecycle.
> + *
> + * Note, when this function is called, the functions specified in struct
> + * faux_ops can be called before the function returns, so be prepared for
> + * everything to be properly initialized before that point in time.
> + *
> + * Return:
> + * * NULL if an error happened with creating the device
> + * * pointer to a valid struct faux_device that is registered with sysfs
> + */
> +struct faux_device *faux_device_create_with_groups(const char *name,
> +						   struct device *parent,
> +						   const struct faux_device_ops *faux_ops,
> +						   const struct attribute_group **groups)
> +{
> +	struct faux_object *faux_obj;
> +	struct faux_device *faux_dev;
> +	struct device *dev;
> +	int ret;
> +
> +	faux_obj = kzalloc(sizeof(*faux_obj), GFP_KERNEL);
> +	if (!faux_obj)
> +		return NULL;
> +
> +	/* Save off the callbacks so we can use them in the future */
> +	faux_obj->faux_ops = faux_ops;
> +
> +	/* 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;
> +	if (parent)
> +		dev->parent = parent;
> +	else
> +		dev->parent = &faux_bus_root;
> +	dev->bus = &faux_bus_type;
> +	dev->groups = groups;
> +	dev_set_name(dev, "%s", name);
> +
> +	ret = device_add(dev);
> +	if (ret) {
> +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
> +		       __func__, name, ret);
> +		put_device(dev);
> +		return NULL;
> +	}
> +
> +	return faux_dev;
> +}
> +EXPORT_SYMBOL_GPL(faux_device_create_with_groups);
> +
> +/**
> + * faux_device_create - create and register with the driver core a faux device
> + * @name:	The name of the device we are adding, must be unique for all
> + *		faux devices.
> + * @parent:	Pointer to a potential parent struct device.  If set to
> + *		NULL, the device will be created in the "root" of the faux
> + *		device tree in sysfs.
> + * @faux_ops:	struct faux_device_ops that the new device will call back
> + *		into, can be NULL.
> + *
> + * Create a new faux device and register it in the driver core properly.
> + * If present, callbacks in @faux_ops will be called with the device that
> + * for the caller to do something with at the proper time given the
> + * device's lifecycle.
> + *
> + * Note, when this function is called, the functions specified in struct
> + * faux_ops can be called before the function returns, so be prepared for
> + * everything to be properly initialized before that point in time.
> + *
> + * Return:
> + * * NULL if an error happened with creating the device
> + * * pointer to a valid struct faux_device that is registered with sysfs
> + */
> +struct faux_device *faux_device_create(const char *name,
> +				       struct device *parent,
> +				       const struct faux_device_ops *faux_ops)
> +{
> +	return faux_device_create_with_groups(name, parent, faux_ops, NULL);
> +}
> +EXPORT_SYMBOL_GPL(faux_device_create);
> +
> +/**
> + * faux_device_destroy - destroy a faux device
> + * @faux_dev:	faux device to destroy
> + *
> + * Unregisters and cleans up a device that was created with a call to
> + * faux_device_create()
> + */
> +void faux_device_destroy(struct faux_device *faux_dev)
> +{
> +	struct device *dev = &faux_dev->dev;
> +
> +	if (!faux_dev)
> +		return;
> +
> +	device_del(dev);
> +
> +	/* The final put_device() will clean up the memory we allocated for this device. */
> +	put_device(dev);
> +}
> +EXPORT_SYMBOL_GPL(faux_device_destroy);
> +
> +int __init faux_bus_init(void)
> +{
> +	int ret;
> +
> +	ret = device_register(&faux_bus_root);
> +	if (ret) {
> +		put_device(&faux_bus_root);
> +		return ret;
> +	}
> +
> +	ret = bus_register(&faux_bus_type);
> +	if (ret)
> +		goto error_bus;
> +
> +	ret = driver_register(&faux_driver);
> +	if (ret)
> +		goto error_driver;
> +
> +	return ret;
> +
> +error_driver:
> +	bus_unregister(&faux_bus_type);
> +
> +error_bus:
> +	device_unregister(&faux_bus_root);
> +	return ret;
> +}
> 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..9f43c0e46aa4
> --- /dev/null
> +++ b/include/linux/device/faux.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
> + * Copyright (c) 2025 The Linux Foundation
> + *
> + * A "simple" faux bus that allows devices to be created and added
> + * automatically to it.  This is to be used whenever you need to create a
> + * device that is not associated with any "real" system resources, and do
> + * not want to have to deal with a bus/driver binding logic.  It is
> + * intended to be very simple, with only a create and a destroy function
> + * available.
> + */
> +#ifndef _FAUX_DEVICE_H_
> +#define _FAUX_DEVICE_H_
> +
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +
> +/**
> + * struct faux_device - a "faux" device
> + * @dev:	internal struct device of the object
> + *
> + * A simple faux device that can be created/destroyed.  To be used when a
> + * driver only needs to have a device to "hang" something off.  This can be
> + * used for downloading firmware or other basic tasks.  Use this instead of
> + * a struct platform_device if the device has no resources assigned to
> + * it at all.
> + */
> +struct faux_device {
> +	struct device dev;
> +};
> +#define to_faux_device(x) container_of_const((x), struct faux_device, dev)
> +
> +/**
> + * struct faux_device_ops - a set of callbacks for a struct faux_device
> + * @probe:	called when a faux device is probed by the driver core
> + *		before the device is fully bound to the internal faux bus
> + *		code.  If probe succeeds, return 0, otherwise return a
> + *		negative error number to stop the probe sequence from
> + *		succeeding.
> + * @remove:	called when a faux device is removed from the system
> + *
> + * Both @probe and @remove are optional, if not needed, set to NULL.
> + */
> +struct faux_device_ops {
> +	int (*probe)(struct faux_device *faux_dev);
> +	void (*remove)(struct faux_device *faux_dev);
> +};
> +
> +struct faux_device *faux_device_create(const char *name,
> +				       struct device *parent,
> +				       const struct faux_device_ops *faux_ops);
> +struct faux_device *faux_device_create_with_groups(const char *name,
> +						   struct device *parent,
> +						   const struct faux_device_ops *faux_ops,
> +						   const struct attribute_group **groups);
> +void faux_device_destroy(struct faux_device *faux_dev);
> +
> +static inline void *faux_device_get_drvdata(const struct faux_device *faux_dev)
> +{
> +	return dev_get_drvdata(&faux_dev->dev);
> +}
> +
> +static inline void faux_device_set_drvdata(struct faux_device *faux_dev, void *data)
> +{
> +	dev_set_drvdata(&faux_dev->dev, data);
> +}
> +
> +#endif /* _FAUX_DEVICE_H_ */

Reviewed-by: Zijun Hu <[e-mail]>

Greg Kroah-Hartman Feb. 11, 2025, 5:50 a.m. UTC | #8

On Mon, Feb 10, 2025 at 10:52:47AM -0500, Kurt Borja wrote:
> On Mon Feb 10, 2025 at 10:36 AM -05, Greg Kroah-Hartman wrote:
> > On Mon, Feb 10, 2025 at 09:58:24AM -0500, Kurt Borja wrote:
> >> On Mon Feb 10, 2025 at 9:45 AM -05, Greg Kroah-Hartman wrote:
> >> > On Mon, Feb 10, 2025 at 09:29:52AM -0500, Kurt Borja wrote:
> >> >> Hi Greg,
> >> >> 
> >> >> On Mon Feb 10, 2025 at 7:30 AM -05, 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.
> >> >> >
> >> >> > Reviewed-by: Jonathan Cameron <[e-mail]>
> >> >> > Reviewed-by: Danilo Krummrich <[e-mail]>
> >> >> > Reviewed-by: Lyude Paul <[e-mail]>
> >> >> > Reviewed-by: Thomas Weißschuh <[e-mail]>
> >> >> > Signed-off-by: Greg Kroah-Hartman <[e-mail]>
> >> >> > ---
> >> >> > v4: - really removed the name logic
> >> >> >     - added #include <linux/container_of.h> to faux.h
> >> >> >     - added parent pointer to api call
> >> >> >     - minor documentation updates
> >> >> >     - made probe synchronous
> >> >> > v3: - loads of documentation updates and rewrites
> >> >> >     - added to the documentation build
> >> >> >     - removed name[] array as it's no longer needed
> >> >> >     - added faux_device_create_with_groups()
> >> >> >     - added functions to get/set devdata
> >> >> >     - renamed faux_driver_ops -> faux_device_ops
> >> >> >     - made faux_device_ops a const *
> >> >> >     - minor cleanups
> >> >> >     - tested it, again.
> >> >> >
> >> >> > v2: - renamed bus and root device to just "faux" thanks to Thomas
> >> >> >     - removed the one-driver-per-device and now just have one driver
> >> >> >       entirely thanks to Danilo
> >> >> >     - kerneldoc fixups and additions and string handling bounds checks
> >> >> >       thanks to Andy
> >> >> >     - coding style fix thanks to Jonathan
> >> >> >     - tested that the destroy path actually works
> >> >> >  Documentation/driver-api/infrastructure.rst |   6 +
> >> >> >  drivers/base/Makefile                       |   2 +-
> >> >> >  drivers/base/base.h                         |   1 +
> >> >> >  drivers/base/faux.c                         | 232 ++++++++++++++++++++
> >> >> >  drivers/base/init.c                         |   1 +
> >> >> >  include/linux/device/faux.h                 |  69 ++++++
> >> >> >  6 files changed, 310 insertions(+), 1 deletion(-)
> >> >> >  create mode 100644 drivers/base/faux.c
> >> >> >  create mode 100644 include/linux/device/faux.h
> >> >> >
> >> >> > diff --git a/Documentation/driver-api/infrastructure.rst b/Documentation/driver-api/infrastructure.rst
> >> >> > index 3d52dfdfa9fd..35e36fee4238 100644
> >> >> > --- a/Documentation/driver-api/infrastructure.rst
> >> >> > +++ b/Documentation/driver-api/infrastructure.rst
> >> >> > @@ -41,6 +41,12 @@ Device Drivers Base
> >> >> >  .. kernel-doc:: drivers/base/class.c
> >> >> >     :export:
> >> >> >  
> >> >> > +.. kernel-doc:: include/linux/device/faux.h
> >> >> > +   :internal:
> >> >> > +
> >> >> > +.. kernel-doc:: drivers/base/faux.c
> >> >> > +   :export:
> >> >> > +
> >> >> >  .. kernel-doc:: drivers/base/node.c
> >> >> >     :internal:
> >> >> >  
> >> >> > 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..531e9d789ee0
> >> >> > --- /dev/null
> >> >> > +++ b/drivers/base/faux.c
> >> >> > @@ -0,0 +1,232 @@
> >> >> > +// SPDX-License-Identifier: GPL-2.0-only
> >> >> > +/*
> >> >> > + * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
> >> >> > + * Copyright (c) 2025 The Linux Foundation
> >> >> > + *
> >> >> > + * A "simple" faux bus that allows devices to be created and added
> >> >> > + * automatically to it.  This is to be used whenever you need to create a
> >> >> > + * device that is not associated with any "real" system resources, and do
> >> >> > + * not want to have to deal with a bus/driver binding logic.  It is
> >> >> > + * intended to be very simple, with only a create and a destroy function
> >> >> > + * available.
> >> >> > + */
> >> >> > +#include <linux/err.h>
> >> >> > +#include <linux/init.h>
> >> >> > +#include <linux/slab.h>
> >> >> > +#include <linux/string.h>
> >> >> > +#include <linux/container_of.h>
> >> >> > +#include <linux/device/faux.h>
> >> >> > +#include "base.h"
> >> >> > +
> >> >> > +/*
> >> >> > + * Internal wrapper structure so we can hold a pointer to the
> >> >> > + * faux_device_ops for this device.
> >> >> > + */
> >> >> > +struct faux_object {
> >> >> > +	struct faux_device faux_dev;
> >> >> > +	const struct faux_device_ops *faux_ops;
> >> >> > +};
> >> >> > +#define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
> >> >> > +
> >> >> > +static struct device faux_bus_root = {
> >> >> > +	.init_name	= "faux",
> >> >> > +};
> >> >> > +
> >> >> > +static int faux_match(struct device *dev, const struct device_driver *drv)
> >> >> > +{
> >> >> > +	/* Match always succeeds, we only have one driver */
> >> >> > +	return 1;
> >> >> > +}
> >> >> > +
> >> >> > +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_device_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_device_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",
> >> >> > +	.match		= faux_match,
> >> >> > +	.probe		= faux_probe,
> >> >> > +	.remove		= faux_remove,
> >> >> > +};
> >> >> > +
> >> >> > +static struct device_driver faux_driver = {
> >> >> > +	.name		= "faux_driver",
> >> >> > +	.bus		= &faux_bus_type,
> >> >> > +	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
> >> >> > +};
> >> >> > +
> >> >> > +static void faux_device_release(struct device *dev)
> >> >> > +{
> >> >> > +	struct faux_object *faux_obj = to_faux_object(dev);
> >> >> > +
> >> >> > +	kfree(faux_obj);
> >> >> > +}
> >> >> > +
> >> >> > +/**
> >> >> > + * faux_device_create_with_groups - Create and register with the driver
> >> >> > + *		core a faux device and populate the device with an initial
> >> >> > + *		set of sysfs attributes.
> >> >> > + * @name:	The name of the device we are adding, must be unique for
> >> >> > + *		all faux devices.
> >> >> > + * @parent:	Pointer to a potential parent struct device.  If set to
> >> >> > + *		NULL, the device will be created in the "root" of the faux
> >> >> > + *		device tree in sysfs.
> >> >> > + * @faux_ops:	struct faux_device_ops that the new device will call back
> >> >> > + *		into, can be NULL.
> >> >> > + * @groups:	The set of sysfs attributes that will be created for this
> >> >> > + *		device when it is registered with the driver core.
> >> >> > + *
> >> >> > + * Create a new faux device and register it in the driver core properly.
> >> >> > + * If present, callbacks in @faux_ops will be called with the device that
> >> >> > + * for the caller to do something with at the proper time given the
> >> >> > + * device's lifecycle.
> >> >> > + *
> >> >> > + * Note, when this function is called, the functions specified in struct
> >> >> > + * faux_ops can be called before the function returns, so be prepared for
> >> >> > + * everything to be properly initialized before that point in time.
> >> >> > + *
> >> >> > + * Return:
> >> >> > + * * NULL if an error happened with creating the device
> >> >> > + * * pointer to a valid struct faux_device that is registered with sysfs
> >> >> > + */
> >> >> > +struct faux_device *faux_device_create_with_groups(const char *name,
> >> >> > +						   struct device *parent,
> >> >> > +						   const struct faux_device_ops *faux_ops,
> >> >> > +						   const struct attribute_group **groups)
> >> >> > +{
> >> >> > +	struct faux_object *faux_obj;
> >> >> > +	struct faux_device *faux_dev;
> >> >> > +	struct device *dev;
> >> >> > +	int ret;
> >> >> > +
> >> >> > +	faux_obj = kzalloc(sizeof(*faux_obj), GFP_KERNEL);
> >> >> > +	if (!faux_obj)
> >> >> > +		return NULL;
> >> >> > +
> >> >> > +	/* Save off the callbacks so we can use them in the future */
> >> >> > +	faux_obj->faux_ops = faux_ops;
> >> >> > +
> >> >> > +	/* 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;
> >> >> > +	if (parent)
> >> >> > +		dev->parent = parent;
> >> >> > +	else
> >> >> > +		dev->parent = &faux_bus_root;
> >> >> > +	dev->bus = &faux_bus_type;
> >> >> > +	dev->groups = groups;
> >> >> > +	dev_set_name(dev, "%s", name);
> >> >> > +
> >> >> > +	ret = device_add(dev);
> >> >> > +	if (ret) {
> >> >> > +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
> >> >> > +		       __func__, name, ret);
> >> >> > +		put_device(dev);
> >> >> > +		return NULL;
> >> >> > +	}
> >> >> 
> >> >> Now that the probe is synchronous, what do you think about returning
> >> >> -ENODEV if the device failed to bind to the driver?
> >> >
> >> > Nope, that would make all callers have to deal with a pointer or a NULL,
> >> > or an error value encorporated in a pointer.
> >> 
> >> Right! I thought for a sec this function returned ERR_PTRs.
> >> 
> >> >
> >> >> This would be useful for modules that may want to unload if the probe
> >> >> fails.
> >> >
> >> > Then just test for NULL, there's nothing preventing that, right?
> >> 
> >> Please, correct me if I'm wrong. If the probe the user provided fails
> >> the device would still be added successfully to the bus. That means this
> >> function would return a valid pointer and the module has no way of
> >> knowing if the probe succeeded in an __init section.
> >
> > Ah, yes, you are totally correct here.  You don't know if the probe
> > failed or not.  But your device is still "live" and you can only get rid
> > of it by calling faux_device_destroy(), all that might be wrong with it
> > is that it's not really associated with the bus.
> 
> I'm curious of what happens with the sysfs groups you pass if the probe
> fails. Do they get assigned on driver attachment or right after
> device_add()? Because those sysfs attributes may depend on resources
> initialized on the probe.

The driver core handles this for us, for a default device attribute
group, nothing new here.

> > You can still assign resources to it, AND the resources will be freed up
> > when the device goes away (see the comment in device_release() for
> > specifics as to this happening today for platform devices.)
> >
> > I guess we can test for this and handle it, if you feel it is necessary,
> > it wouldn't be hard to do so, but let me add this later as it's the same
> > problem with platform devices and odds are we want to fix that issue up
> > there too, right?
> 
> Platform devices have platform_create_bundle() which does check if the
> probe succeeded.

Almost no one uses that function, and I'm really not sure that it does
that check either (got lost in the maze yesterday, I'll look into it
later today.)

This api is to replace the examples in this series that do NOT use the
create_bundle() api, which has the exact same functionality of "we don't
know if the driver was bound or not" logic.

> >> > Also, the only way probe can fail is if the probe you passed into the
> >> > call fails.  Or if you picked a name that is already in the system, both
> >> 
> >> Exactly. I'm thinking of modules that are very simple and just care
> >> about the probe succeeding, so having the device hanging around in the
> >> bus and the module loaded would be a waste of resources.
> >
> > Modules usually don't need to do the probe callback at all.  I show one
> > example in this series (the regulator dummy driver), but it can be
> > easily rewritten to not need that at all.
> 
> This is a good point, but from a developer standpoint I would always try
> to use the probe callback. This API seems to suggest that's the
> appropiate use.
> 
> Also it would be amazing if the probe could reside in an __init section.

Probes should NEVER be in an init section for obvious reasons.  Please
don't do that today.

thanks,

greg k-h

Greg Kroah-Hartman Feb. 11, 2025, 7:27 a.m. UTC | #9

On Mon, Feb 10, 2025 at 12:56:46PM -0500, Kurt Borja wrote:
> On Mon Feb 10, 2025 at 7:30 AM -05, 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.
> >
> > Reviewed-by: Jonathan Cameron <[e-mail]>
> > Reviewed-by: Danilo Krummrich <[e-mail]>
> > Reviewed-by: Lyude Paul <[e-mail]>
> > Reviewed-by: Thomas Weißschuh <[e-mail]>
> > Signed-off-by: Greg Kroah-Hartman <[e-mail]>
> > ---
> > v4: - really removed the name logic
> >     - added #include <linux/container_of.h> to faux.h
> >     - added parent pointer to api call
> >     - minor documentation updates
> >     - made probe synchronous
> > v3: - loads of documentation updates and rewrites
> >     - added to the documentation build
> >     - removed name[] array as it's no longer needed
> >     - added faux_device_create_with_groups()
> >     - added functions to get/set devdata
> >     - renamed faux_driver_ops -> faux_device_ops
> >     - made faux_device_ops a const *
> >     - minor cleanups
> >     - tested it, again.
> >
> > v2: - renamed bus and root device to just "faux" thanks to Thomas
> >     - removed the one-driver-per-device and now just have one driver
> >       entirely thanks to Danilo
> >     - kerneldoc fixups and additions and string handling bounds checks
> >       thanks to Andy
> >     - coding style fix thanks to Jonathan
> >     - tested that the destroy path actually works
> >  Documentation/driver-api/infrastructure.rst |   6 +
> >  drivers/base/Makefile                       |   2 +-
> >  drivers/base/base.h                         |   1 +
> >  drivers/base/faux.c                         | 232 ++++++++++++++++++++
> >  drivers/base/init.c                         |   1 +
> >  include/linux/device/faux.h                 |  69 ++++++
> >  6 files changed, 310 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/base/faux.c
> >  create mode 100644 include/linux/device/faux.h
> >
> > diff --git a/Documentation/driver-api/infrastructure.rst b/Documentation/driver-api/infrastructure.rst
> > index 3d52dfdfa9fd..35e36fee4238 100644
> > --- a/Documentation/driver-api/infrastructure.rst
> > +++ b/Documentation/driver-api/infrastructure.rst
> > @@ -41,6 +41,12 @@ Device Drivers Base
> >  .. kernel-doc:: drivers/base/class.c
> >     :export:
> >  
> > +.. kernel-doc:: include/linux/device/faux.h
> > +   :internal:
> > +
> > +.. kernel-doc:: drivers/base/faux.c
> > +   :export:
> > +
> >  .. kernel-doc:: drivers/base/node.c
> >     :internal:
> >  
> > 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..531e9d789ee0
> > --- /dev/null
> > +++ b/drivers/base/faux.c
> > @@ -0,0 +1,232 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
> > + * Copyright (c) 2025 The Linux Foundation
> > + *
> > + * A "simple" faux bus that allows devices to be created and added
> > + * automatically to it.  This is to be used whenever you need to create a
> > + * device that is not associated with any "real" system resources, and do
> > + * not want to have to deal with a bus/driver binding logic.  It is
> > + * intended to be very simple, with only a create and a destroy function
> > + * available.
> > + */
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/container_of.h>
> > +#include <linux/device/faux.h>
> > +#include "base.h"
> > +
> > +/*
> > + * Internal wrapper structure so we can hold a pointer to the
> > + * faux_device_ops for this device.
> > + */
> > +struct faux_object {
> > +	struct faux_device faux_dev;
> > +	const struct faux_device_ops *faux_ops;
> > +};
> > +#define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
> > +
> > +static struct device faux_bus_root = {
> > +	.init_name	= "faux",
> > +};
> > +
> > +static int faux_match(struct device *dev, const struct device_driver *drv)
> > +{
> > +	/* Match always succeeds, we only have one driver */
> > +	return 1;
> > +}
> > +
> > +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_device_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_device_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",
> > +	.match		= faux_match,
> > +	.probe		= faux_probe,
> > +	.remove		= faux_remove,
> > +};
> > +
> > +static struct device_driver faux_driver = {
> > +	.name		= "faux_driver",
> > +	.bus		= &faux_bus_type,
> > +	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
> > +};
> > +
> > +static void faux_device_release(struct device *dev)
> > +{
> > +	struct faux_object *faux_obj = to_faux_object(dev);
> > +
> > +	kfree(faux_obj);
> > +}
> > +
> > +/**
> > + * faux_device_create_with_groups - Create and register with the driver
> > + *		core a faux device and populate the device with an initial
> > + *		set of sysfs attributes.
> > + * @name:	The name of the device we are adding, must be unique for
> > + *		all faux devices.
> > + * @parent:	Pointer to a potential parent struct device.  If set to
> > + *		NULL, the device will be created in the "root" of the faux
> > + *		device tree in sysfs.
> > + * @faux_ops:	struct faux_device_ops that the new device will call back
> > + *		into, can be NULL.
> > + * @groups:	The set of sysfs attributes that will be created for this
> > + *		device when it is registered with the driver core.
> > + *
> > + * Create a new faux device and register it in the driver core properly.
> > + * If present, callbacks in @faux_ops will be called with the device that
> > + * for the caller to do something with at the proper time given the
> > + * device's lifecycle.
> > + *
> > + * Note, when this function is called, the functions specified in struct
> > + * faux_ops can be called before the function returns, so be prepared for
> > + * everything to be properly initialized before that point in time.
> > + *
> > + * Return:
> > + * * NULL if an error happened with creating the device
> > + * * pointer to a valid struct faux_device that is registered with sysfs
> > + */
> > +struct faux_device *faux_device_create_with_groups(const char *name,
> > +						   struct device *parent,
> > +						   const struct faux_device_ops *faux_ops,
> > +						   const struct attribute_group **groups)
> > +{
> > +	struct faux_object *faux_obj;
> > +	struct faux_device *faux_dev;
> > +	struct device *dev;
> > +	int ret;
> > +
> > +	faux_obj = kzalloc(sizeof(*faux_obj), GFP_KERNEL);
> > +	if (!faux_obj)
> > +		return NULL;
> > +
> > +	/* Save off the callbacks so we can use them in the future */
> > +	faux_obj->faux_ops = faux_ops;
> > +
> > +	/* 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;
> > +	if (parent)
> > +		dev->parent = parent;
> > +	else
> > +		dev->parent = &faux_bus_root;
> > +	dev->bus = &faux_bus_type;
> > +	dev->groups = groups;
> 
> Just as I feared, adding groups this way is bug prone if we don't check
> if the device attached to the driver successfully. Consider the
> following example module:
> 
> static attr1_show(...)
> {
> 	struct priv *priv = dev_get_drvdata(dev);
> 	...
> }
> DEVICE_ATTR_RO(attr1)
> 
> ...
> 
> static const struct attribute_group faux_groups[] = {
> 	...
> }
> 
> /* It would be nice to have the probe in __init */
> static int __init faux_probe(struct faux_device *fdev)
> {
> 	...
> 	/* Probe may fail */
> 	if (con)
> 		return -EINVAL;
> 	...
> 	faux_device_set_drvdata(fdev, priv);
> 	...
> }
> 
> static struct faux_device_ops faux_ops = {
> 	...
> }
> 
> static int __init module_init()
> {
> 	...
> 	fdev = faux_device_create_with_groups("foo", NULL, &faux_ops,
> 					      &faux_groups);
> 	if (!fdev)
> 		return -ENODEV;
> }
> 
> In this example we have no way of knowing if the probe failed, so the
> module as well as the device will remain loaded. Furthermore, the sysfs
> groups WILL be added anyway, which is dangerous because we have no
> gurantee about drvdata's lifetime nor if it was even initialized
> correctly, which sysfs show/store methods may assume is the case.
> 
> So we have two problems here. First, this module only cares about the
> probe succeeding so keeping the device alive after it fails wastes
> resources. Second, we don't have any gurantee about drvdata validity.
> 
> > +	dev_set_name(dev, "%s", name);
> > +
> > +	ret = device_add(dev);
> > +	if (ret) {
> > +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
> > +		       __func__, name, ret);
> > +		put_device(dev);
> > +		return NULL;
> > +	}
> 
> To solve this, I suggest we do this here:
> 
> 	/* Check if the device attached correctly */
> 	if (dev->driver != &faux_driver)
> 		return ERR_PTR(-ENODEV); /* or NULL */
> 
> 	/* 
> 	 * Add groups after the driver is attached, to avoid lifetime
> 	 * issues 
> 	 */
> 	device_add_groups(dev, groups);

Nope, sorry, let the driver core handle the group creation/removal at
the proper time.

I'll work on adding "if probe failed, don't let the device be created"
logic as it's a simple change, BUT it is a functionally different change
from what the current api that this code is replacing is doing (i.e. the
current platform device creation code does this today and no one has
ever hit this in their use of it in the past few decades.)

thanks,

greg k-h

Greg Kroah-Hartman Feb. 11, 2025, 7:33 a.m. UTC | #10

On Tue, Feb 11, 2025 at 08:27:26AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Feb 10, 2025 at 12:56:46PM -0500, Kurt Borja wrote:
> I'll work on adding "if probe failed, don't let the device be created"
> logic as it's a simple change, BUT it is a functionally different change
> from what the current api that this code is replacing is doing (i.e. the
> current platform device creation code does this today and no one has
> ever hit this in their use of it in the past few decades.)

How about something as simple as this change, does that provide what you
are thinking about here?  Only compile tested, not runtime tested at
all:


diff --git a/drivers/base/faux.c b/drivers/base/faux.c
index 531e9d789ee0..e2de0697c0e3 100644
--- a/drivers/base/faux.c
+++ b/drivers/base/faux.c
@@ -25,6 +25,7 @@
 struct faux_object {
 	struct faux_device faux_dev;
 	const struct faux_device_ops *faux_ops;
+	bool probe_was_successful;
 };
 #define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
 
@@ -48,6 +49,9 @@ static int faux_probe(struct device *dev)
 	if (faux_ops && faux_ops->probe)
 		ret = faux_ops->probe(faux_dev);
 
+	if (!ret)
+		faux_obj->probe_was_successful = true;
+
 	return ret;
 }
 
@@ -147,6 +151,15 @@ struct faux_device *faux_device_create_with_groups(const char *name,
 		return NULL;
 	}
 
+	/*
+	 * The probe callback will set probe_was_successful if it
+	 * succeeded, if not, then we need to tear things down here
+	 */
+	if (!faux_obj->probe_was_successful) {
+		faux_device_destroy(faux_dev);
+		return NULL;
+	}
+
 	return faux_dev;
 }
 EXPORT_SYMBOL_GPL(faux_device_create_with_groups);

Kurt Borja Feb. 11, 2025, 7:43 a.m. UTC | #11

On Tue Feb 11, 2025 at 2:33 AM -05, Greg Kroah-Hartman wrote:
> On Tue, Feb 11, 2025 at 08:27:26AM +0100, Greg Kroah-Hartman wrote:
>> On Mon, Feb 10, 2025 at 12:56:46PM -0500, Kurt Borja wrote:
>> I'll work on adding "if probe failed, don't let the device be created"
>> logic as it's a simple change, BUT it is a functionally different change
>> from what the current api that this code is replacing is doing (i.e. the
>> current platform device creation code does this today and no one has
>> ever hit this in their use of it in the past few decades.)
>
> How about something as simple as this change, does that provide what you
> are thinking about here?  Only compile tested, not runtime tested at
> all:

Yes, LGTM. However dev->driver is set to NULL if the probe fails so
wouldn't

	if (!dev->driver)

do the job?

I understand your point about groups. This of course solves it, although
isn't there a small windows between device_add() and device_destroy() in
the failed probe path, in which a show/store/visibility method could
dereference a NULL drvdata?

Greg Kroah-Hartman Feb. 11, 2025, 8:17 a.m. UTC | #12

On Tue, Feb 11, 2025 at 02:43:20AM -0500, Kurt Borja wrote:
> On Tue Feb 11, 2025 at 2:33 AM -05, Greg Kroah-Hartman wrote:
> > On Tue, Feb 11, 2025 at 08:27:26AM +0100, Greg Kroah-Hartman wrote:
> >> On Mon, Feb 10, 2025 at 12:56:46PM -0500, Kurt Borja wrote:
> >> I'll work on adding "if probe failed, don't let the device be created"
> >> logic as it's a simple change, BUT it is a functionally different change
> >> from what the current api that this code is replacing is doing (i.e. the
> >> current platform device creation code does this today and no one has
> >> ever hit this in their use of it in the past few decades.)
> >
> > How about something as simple as this change, does that provide what you
> > are thinking about here?  Only compile tested, not runtime tested at
> > all:
> 
> Yes, LGTM. However dev->driver is set to NULL if the probe fails so
> wouldn't
> 
> 	if (!dev->driver)
> 
> do the job?

True, that would work, and be much simpler, I'll go add that AND
actually test it :)

> I understand your point about groups. This of course solves it, although
> isn't there a small windows between device_add() and device_destroy() in
> the failed probe path, in which a show/store/visibility method could
> dereference a NULL drvdata?

Ok, I looked it up and it turns out that the groups wouldn't have even
been created if probe() failed (see the call to call_driver_probe() in
really_probe() in drivers/base/dd.c)  So all should be good here.

thanks,

greg k-h

Kurt Borja Feb. 11, 2025, 8:36 a.m. UTC | #13

On Tue Feb 11, 2025 at 3:17 AM -05, Greg Kroah-Hartman wrote:
> On Tue, Feb 11, 2025 at 02:43:20AM -0500, Kurt Borja wrote:
>> On Tue Feb 11, 2025 at 2:33 AM -05, Greg Kroah-Hartman wrote:
>> > On Tue, Feb 11, 2025 at 08:27:26AM +0100, Greg Kroah-Hartman wrote:
>> >> On Mon, Feb 10, 2025 at 12:56:46PM -0500, Kurt Borja wrote:
>> >> I'll work on adding "if probe failed, don't let the device be created"
>> >> logic as it's a simple change, BUT it is a functionally different change
>> >> from what the current api that this code is replacing is doing (i.e. the
>> >> current platform device creation code does this today and no one has
>> >> ever hit this in their use of it in the past few decades.)
>> >
>> > How about something as simple as this change, does that provide what you
>> > are thinking about here?  Only compile tested, not runtime tested at
>> > all:
>> 
>> Yes, LGTM. However dev->driver is set to NULL if the probe fails so
>> wouldn't
>> 
>> 	if (!dev->driver)
>> 
>> do the job?
>
> True, that would work, and be much simpler, I'll go add that AND
> actually test it :)

Nice :)

>
>> I understand your point about groups. This of course solves it, although
>> isn't there a small windows between device_add() and device_destroy() in
>> the failed probe path, in which a show/store/visibility method could
>> dereference a NULL drvdata?
>
> Ok, I looked it up and it turns out that the groups wouldn't have even
> been created if probe() failed (see the call to call_driver_probe() in
> really_probe() in drivers/base/dd.c)  So all should be good here.

Are you refering to this line [1]? Because those are the driver's
dev_groups.

dev->groups are added by device_add_attrs() in device_add(). Here is the
line [2]. This happens *way* before the device is added to the bus.

Also I tested with a sample faux device (faux faux device :)) and the
groups do get added, even if the probe fails.

[1] https://elixir.bootlin.com/linux/v6.14-rc1/source/drivers/base/core.c#L2887
[2] https://elixir.bootlin.com/linux/v6.14-rc1/source/drivers/base/core.c#L2887

Zijun Hu Feb. 11, 2025, 3:29 p.m. UTC | #14

On 2025/2/10 22:29, Kurt Borja wrote:
>> +
>> +	ret = device_add(dev);
>> +	if (ret) {
>> +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
>> +		       __func__, name, ret);
>> +		put_device(dev);
>> +		return NULL;
>> +	}
> Now that the probe is synchronous, what do you think about returning
> -ENODEV if the device failed to bind to the driver?
> 

Result of device registering @ret is not, should not be, effected by
"device binding driver (probe result)"

if device binging driver failed, you may return -ENODEV in
faux_ops->probe(). not here.

> This would be useful for modules that may want to unload if the probe
> fails.

may need to root cause if probe failure happens.

how to unload module automatically if probe() failure ?

Kurt Borja Feb. 11, 2025, 3:49 p.m. UTC | #15

On Tue Feb 11, 2025 at 10:29 AM -05, Zijun Hu wrote:
> On 2025/2/10 22:29, Kurt Borja wrote:
>>> +
>>> +	ret = device_add(dev);
>>> +	if (ret) {
>>> +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
>>> +		       __func__, name, ret);
>>> +		put_device(dev);
>>> +		return NULL;
>>> +	}
>> Now that the probe is synchronous, what do you think about returning
>> -ENODEV if the device failed to bind to the driver?
>> 
>
> Result of device registering @ret is not, should not be, effected by
> "device binding driver (probe result)"
>
> if device binging driver failed, you may return -ENODEV in
> faux_ops->probe(). not here.

After thinking about this discussion, I understand why this might be the
expected behavior. I'm thinking about very simple modules that would
remain loaded even if the probe fails. But of course this may cause
problems to other modules.

In the end, this is just my opinion so it would be up to Greg to decide.
However, there is still an issue with the groups added to the device,
which a user might expect they are tied to an "attached" device
lifetime and this currently not the case.

>
>> This would be useful for modules that may want to unload if the probe
>> fails.
>
> may need to root cause if probe failure happens.
>
> how to unload module automatically if probe() failure ?

If we check for !dev->driver, a module might propagate an error to the
module_init, thus making it fail to load.

Lyude Paul Feb. 11, 2025, 8:06 p.m. UTC | #16

On Mon, 2025-02-10 at 10:52 -0500, Kurt Borja wrote:
> > Modules usually don't need to do the probe callback at all.  I show one
> > example in this series (the regulator dummy driver), but it can be
> > easily rewritten to not need that at all.
> 
> This is a good point, but from a developer standpoint I would always try
> to use the probe callback. This API seems to suggest that's the
> appropiate use.
> 
> Also it would be amazing if the probe could reside in an __init section.

IMO I think it is fine without the probe callback, plus we're the ones making
the API - it can say whatever we want :).

To be clear though, generally I'm fairly sure the only reason for drivers to
be using probe() at all is because you want a driver-callback the kernel is
responsible for executing in response to a new device being added on a bus.
This doesn't really make sense for a virtual bus, since we're in control of
adding devices - and thus probe() would just be an unnecessary layer for work
that can already easily be done from the same call site that added the device.
So I think it's fine for this to be a special case imo.

Kurt Borja Feb. 11, 2025, 9:57 p.m. UTC | #17

Hi Lyude,

On Tue Feb 11, 2025 at 3:06 PM -05, Lyude Paul wrote:
> On Mon, 2025-02-10 at 10:52 -0500, Kurt Borja wrote:
>> > Modules usually don't need to do the probe callback at all.  I show one
>> > example in this series (the regulator dummy driver), but it can be
>> > easily rewritten to not need that at all.
>> 
>> This is a good point, but from a developer standpoint I would always try
>> to use the probe callback. This API seems to suggest that's the
>> appropiate use.
>> 
>> Also it would be amazing if the probe could reside in an __init section.
>
> IMO I think it is fine without the probe callback, plus we're the ones making
> the API - it can say whatever we want :).

IMO I think you are right.

>
> To be clear though, generally I'm fairly sure the only reason for drivers to
> be using probe() at all is because you want a driver-callback the kernel is
> responsible for executing in response to a new device being added on a bus.
> This doesn't really make sense for a virtual bus, since we're in control of
> adding devices - and thus probe() would just be an unnecessary layer for work
> that can already easily be done from the same call site that added the device.
> So I think it's fine for this to be a special case imo.

I'm still just a newbie in kernel development so keep that in mind.

For me having probe and remove callbacks on "real" hardware devices has
always been about gurantees. Gurantees about lifetimes of resources
assigned by the driver/bus/subsystem (I just started learning rust :p).
Me as a driver, I know I can initialize a bunch of memory, interfaces,
etc. In a safe way, inside the probe because I know everything is
guranteed to be valid until just after the .remove callback.

Of course, this does not apply to this bus because the bus itself and
the single driver are very minimalistic, so we can achieve the same
gurantees in module __init and __exit.

So I agree, the probe probably should just be dropped. However, if the
probe goes then so must faux_device_create_with_groups() because without
a probe the API can't give any gurantees about the lifetime of for
example the drvdata opaque pointer.

I think this is a small problem, because in platform-drivers-x86 for
example, there is a lot of "fake" platform devices that expose a sysfs
interface. So if this devices were made today, they could use this faux
bus, but they would need to create/remove the sysfs groups at the
appropiate time, as to not mess with lifetimes, which is a bit 
bug-prone.

Additionally the current faux device API is very misleading, because
although it gives you a choice to add groups, this groups get added way
before the probe you provide is even executed, and even if it fails. I
believe this could lead to a lot of misbehaviors and bugs.

Greg Kroah-Hartman Feb. 12, 2025, 7:39 a.m. UTC | #18

On Tue, Feb 11, 2025 at 10:49:34AM -0500, Kurt Borja wrote:
> On Tue Feb 11, 2025 at 10:29 AM -05, Zijun Hu wrote:
> > On 2025/2/10 22:29, Kurt Borja wrote:
> >>> +
> >>> +	ret = device_add(dev);
> >>> +	if (ret) {
> >>> +		pr_err("%s: device_add for faux device '%s' failed with %d\n",
> >>> +		       __func__, name, ret);
> >>> +		put_device(dev);
> >>> +		return NULL;
> >>> +	}
> >> Now that the probe is synchronous, what do you think about returning
> >> -ENODEV if the device failed to bind to the driver?
> >> 
> >
> > Result of device registering @ret is not, should not be, effected by
> > "device binding driver (probe result)"
> >
> > if device binging driver failed, you may return -ENODEV in
> > faux_ops->probe(). not here.
> 
> After thinking about this discussion, I understand why this might be the
> expected behavior. I'm thinking about very simple modules that would
> remain loaded even if the probe fails. But of course this may cause
> problems to other modules.
> 
> In the end, this is just my opinion so it would be up to Greg to decide.
> However, there is still an issue with the groups added to the device,
> which a user might expect they are tied to an "attached" device
> lifetime and this currently not the case.

I agree with you here, this could be confusing and cause problems, and
we should be creating apis that "work properly and simply".  Having a
probe callback is good to add device data like you mention, so that you
can properly add the information before the sysfs files are accessed,
removing that race condition.

> >> This would be useful for modules that may want to unload if the probe
> >> fails.
> >
> > may need to root cause if probe failure happens.
> >
> > how to unload module automatically if probe() failure ?
> 
> If we check for !dev->driver, a module might propagate an error to the
> module_init, thus making it fail to load.

Agreed.  Thanks so much for your review comments, they are greatly
appreciated.  When I get time next week I'll make these changes and send
out some patches.

thanks,

greg k-h

Patch

diff --git a/Documentation/driver-api/infrastructure.rst b/Documentation/driver-api/infrastructure.rst
index 3d52dfdfa9fd..35e36fee4238 100644
--- a/Documentation/driver-api/infrastructure.rst
+++ b/Documentation/driver-api/infrastructure.rst
@@ -41,6 +41,12 @@  Device Drivers Base
 .. kernel-doc:: drivers/base/class.c
    :export:
 
+.. kernel-doc:: include/linux/device/faux.h
+   :internal:
+
+.. kernel-doc:: drivers/base/faux.c
+   :export:
+
 .. kernel-doc:: drivers/base/node.c
    :internal:
 
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..531e9d789ee0
--- /dev/null
+++ b/drivers/base/faux.c
@@ -0,0 +1,232 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
+ * Copyright (c) 2025 The Linux Foundation
+ *
+ * A "simple" faux bus that allows devices to be created and added
+ * automatically to it.  This is to be used whenever you need to create a
+ * device that is not associated with any "real" system resources, and do
+ * not want to have to deal with a bus/driver binding logic.  It is
+ * intended to be very simple, with only a create and a destroy function
+ * available.
+ */
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/container_of.h>
+#include <linux/device/faux.h>
+#include "base.h"
+
+/*
+ * Internal wrapper structure so we can hold a pointer to the
+ * faux_device_ops for this device.
+ */
+struct faux_object {
+	struct faux_device faux_dev;
+	const struct faux_device_ops *faux_ops;
+};
+#define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
+
+static struct device faux_bus_root = {
+	.init_name	= "faux",
+};
+
+static int faux_match(struct device *dev, const struct device_driver *drv)
+{
+	/* Match always succeeds, we only have one driver */
+	return 1;
+}
+
+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_device_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_device_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",
+	.match		= faux_match,
+	.probe		= faux_probe,
+	.remove		= faux_remove,
+};
+
+static struct device_driver faux_driver = {
+	.name		= "faux_driver",
+	.bus		= &faux_bus_type,
+	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
+};
+
+static void faux_device_release(struct device *dev)
+{
+	struct faux_object *faux_obj = to_faux_object(dev);
+
+	kfree(faux_obj);
+}
+
+/**
+ * faux_device_create_with_groups - Create and register with the driver
+ *		core a faux device and populate the device with an initial
+ *		set of sysfs attributes.
+ * @name:	The name of the device we are adding, must be unique for
+ *		all faux devices.
+ * @parent:	Pointer to a potential parent struct device.  If set to
+ *		NULL, the device will be created in the "root" of the faux
+ *		device tree in sysfs.
+ * @faux_ops:	struct faux_device_ops that the new device will call back
+ *		into, can be NULL.
+ * @groups:	The set of sysfs attributes that will be created for this
+ *		device when it is registered with the driver core.
+ *
+ * Create a new faux device and register it in the driver core properly.
+ * If present, callbacks in @faux_ops will be called with the device that
+ * for the caller to do something with at the proper time given the
+ * device's lifecycle.
+ *
+ * Note, when this function is called, the functions specified in struct
+ * faux_ops can be called before the function returns, so be prepared for
+ * everything to be properly initialized before that point in time.
+ *
+ * Return:
+ * * NULL if an error happened with creating the device
+ * * pointer to a valid struct faux_device that is registered with sysfs
+ */
+struct faux_device *faux_device_create_with_groups(const char *name,
+						   struct device *parent,
+						   const struct faux_device_ops *faux_ops,
+						   const struct attribute_group **groups)
+{
+	struct faux_object *faux_obj;
+	struct faux_device *faux_dev;
+	struct device *dev;
+	int ret;
+
+	faux_obj = kzalloc(sizeof(*faux_obj), GFP_KERNEL);
+	if (!faux_obj)
+		return NULL;
+
+	/* Save off the callbacks so we can use them in the future */
+	faux_obj->faux_ops = faux_ops;
+
+	/* 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;
+	if (parent)
+		dev->parent = parent;
+	else
+		dev->parent = &faux_bus_root;
+	dev->bus = &faux_bus_type;
+	dev->groups = groups;
+	dev_set_name(dev, "%s", name);
+
+	ret = device_add(dev);
+	if (ret) {
+		pr_err("%s: device_add for faux device '%s' failed with %d\n",
+		       __func__, name, ret);
+		put_device(dev);
+		return NULL;
+	}
+
+	return faux_dev;
+}
+EXPORT_SYMBOL_GPL(faux_device_create_with_groups);
+
+/**
+ * faux_device_create - create and register with the driver core a faux device
+ * @name:	The name of the device we are adding, must be unique for all
+ *		faux devices.
+ * @parent:	Pointer to a potential parent struct device.  If set to
+ *		NULL, the device will be created in the "root" of the faux
+ *		device tree in sysfs.
+ * @faux_ops:	struct faux_device_ops that the new device will call back
+ *		into, can be NULL.
+ *
+ * Create a new faux device and register it in the driver core properly.
+ * If present, callbacks in @faux_ops will be called with the device that
+ * for the caller to do something with at the proper time given the
+ * device's lifecycle.
+ *
+ * Note, when this function is called, the functions specified in struct
+ * faux_ops can be called before the function returns, so be prepared for
+ * everything to be properly initialized before that point in time.
+ *
+ * Return:
+ * * NULL if an error happened with creating the device
+ * * pointer to a valid struct faux_device that is registered with sysfs
+ */
+struct faux_device *faux_device_create(const char *name,
+				       struct device *parent,
+				       const struct faux_device_ops *faux_ops)
+{
+	return faux_device_create_with_groups(name, parent, faux_ops, NULL);
+}
+EXPORT_SYMBOL_GPL(faux_device_create);
+
+/**
+ * faux_device_destroy - destroy a faux device
+ * @faux_dev:	faux device to destroy
+ *
+ * Unregisters and cleans up a device that was created with a call to
+ * faux_device_create()
+ */
+void faux_device_destroy(struct faux_device *faux_dev)
+{
+	struct device *dev = &faux_dev->dev;
+
+	if (!faux_dev)
+		return;
+
+	device_del(dev);
+
+	/* The final put_device() will clean up the memory we allocated for this device. */
+	put_device(dev);
+}
+EXPORT_SYMBOL_GPL(faux_device_destroy);
+
+int __init faux_bus_init(void)
+{
+	int ret;
+
+	ret = device_register(&faux_bus_root);
+	if (ret) {
+		put_device(&faux_bus_root);
+		return ret;
+	}
+
+	ret = bus_register(&faux_bus_type);
+	if (ret)
+		goto error_bus;
+
+	ret = driver_register(&faux_driver);
+	if (ret)
+		goto error_driver;
+
+	return ret;
+
+error_driver:
+	bus_unregister(&faux_bus_type);
+
+error_bus:
+	device_unregister(&faux_bus_root);
+	return ret;
+}
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..9f43c0e46aa4
--- /dev/null
+++ b/include/linux/device/faux.h
@@ -0,0 +1,69 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2025 Greg Kroah-Hartman <[e-mail]>
+ * Copyright (c) 2025 The Linux Foundation
+ *
+ * A "simple" faux bus that allows devices to be created and added
+ * automatically to it.  This is to be used whenever you need to create a
+ * device that is not associated with any "real" system resources, and do
+ * not want to have to deal with a bus/driver binding logic.  It is
+ * intended to be very simple, with only a create and a destroy function
+ * available.
+ */
+#ifndef _FAUX_DEVICE_H_
+#define _FAUX_DEVICE_H_
+
+#include <linux/container_of.h>
+#include <linux/device.h>
+
+/**
+ * struct faux_device - a "faux" device
+ * @dev:	internal struct device of the object
+ *
+ * A simple faux device that can be created/destroyed.  To be used when a
+ * driver only needs to have a device to "hang" something off.  This can be
+ * used for downloading firmware or other basic tasks.  Use this instead of
+ * a struct platform_device if the device has no resources assigned to
+ * it at all.
+ */
+struct faux_device {
+	struct device dev;
+};
+#define to_faux_device(x) container_of_const((x), struct faux_device, dev)
+
+/**
+ * struct faux_device_ops - a set of callbacks for a struct faux_device
+ * @probe:	called when a faux device is probed by the driver core
+ *		before the device is fully bound to the internal faux bus
+ *		code.  If probe succeeds, return 0, otherwise return a
+ *		negative error number to stop the probe sequence from
+ *		succeeding.
+ * @remove:	called when a faux device is removed from the system
+ *
+ * Both @probe and @remove are optional, if not needed, set to NULL.
+ */
+struct faux_device_ops {
+	int (*probe)(struct faux_device *faux_dev);
+	void (*remove)(struct faux_device *faux_dev);
+};
+
+struct faux_device *faux_device_create(const char *name,
+				       struct device *parent,
+				       const struct faux_device_ops *faux_ops);
+struct faux_device *faux_device_create_with_groups(const char *name,
+						   struct device *parent,
+						   const struct faux_device_ops *faux_ops,
+						   const struct attribute_group **groups);
+void faux_device_destroy(struct faux_device *faux_dev);
+
+static inline void *faux_device_get_drvdata(const struct faux_device *faux_dev)
+{
+	return dev_get_drvdata(&faux_dev->dev);
+}
+
+static inline void faux_device_set_drvdata(struct faux_device *faux_dev, void *data)
+{
+	dev_set_drvdata(&faux_dev->dev, data);
+}
+
+#endif /* _FAUX_DEVICE_H_ */