Re: [PATCH v2 1/5] driver core: add a faux bus for use when a simple device/bus is needed

[Greg Kroah-Hartman]

On Tue, Feb 04, 2025 at 01:04:13PM +0100, Danilo Krummrich wrote:
> On Tue, Feb 04, 2025 at 12:52:34PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Feb 04, 2025 at 12:44:03PM +0100, Danilo Krummrich wrote:
> > > On Tue, Feb 04, 2025 at 12:09:13PM +0100, Greg Kroah-Hartman wrote:
> > > > Many drivers abuse the platform driver/bus system as it provides a
> > > > simple way to create and bind a device to a driver-specific set of
> > > > probe/release functions.  Instead of doing that, and wasting all of the
> > > > memory associated with a platform device, here is a "faux" bus that
> > > > can be used instead.
> > > > 
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >  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
> > > >        hanks to Andy
> > > >      - coding style fix thanks to Jonathan
> > > >      - tested that the destroy path actually works
> > > > 
> > > >  drivers/base/Makefile       |   2 +-
> > > >  drivers/base/base.h         |   1 +
> > > >  drivers/base/faux.c         | 196 ++++++++++++++++++++++++++++++++++++
> > > >  drivers/base/init.c         |   1 +
> > > >  include/linux/device/faux.h |  31 ++++++
> > > >  5 files changed, 230 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/base/faux.c
> > > >  create mode 100644 include/linux/device/faux.h
> > > 
> > > I really like it, it's as simply as it can be.
> > > 
> > > Please find one nit below, otherwise
> > > 
> > > Reviewed-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> > > 
> > > > 
> > > > +/**
> > > > + * faux_device_destroy - destroy a faux device
> > > > + * @faux_dev: faux device to destroy
> > > > + *
> > > > + * Unregister and free all memory associated with a faux device that was
> > > > + * previously created with a call to faux_device_create().
> > > 
> > > Can we really claim that this frees all memory? Someone can still have a
> > > reference to the underlying struct device, right?
> > 
> > That "someone" is the person that had the original device pointer passed
> > to it, so if that person then calls faux_device_destroy(), yes, that
> > should all be properly cleaned up.
> > 
> > But even if it isn't, the device is destroyed and gone from sysfs, and
> > whenever that final final put_device() is called, the memory will then
> > be freed by the driver core itself.
> 
> Oh indeed, the code here is perfectly fine. I just wanted to say that calling
> faux_device_destroy() is not a guarantee that "all memory associated with a
> faux device" is actually freed, as the kernel-doc comment above says (or at
> least implies).
> 
> So, the concern only was that the comment could be confusing, as in "How can
> faux_device_destroy() free the memory, if I still have a separate reference to
> this thing?" (which it clearly would not).

Documentation is hard :)

Can you think of some wording here that would explain this better?
Something like "after you call this, you can't touch the pointer you
passed into here" is what I'm going for.

I guess the documentation for device_destroy() would work here as well,
which says:

	 * This call unregisters and cleans up a device that was created with a
	 * call to device_create().

Would that be better?

thanks,

greg k-h