Re: [RFC 1/5] misc: introduce notify-device driver

[Greg Kroah-Hartman]

On Thu, Oct 27, 2022 at 06:33:33PM +0200, Matthias Schiffer wrote:
> On Wed, 2022-10-26 at 16:37 +0200, Greg Kroah-Hartman wrote:
> > On Wed, Oct 26, 2022 at 03:15:30PM +0200, Matthias Schiffer wrote:
> > > A notify-device is a synchronization facility that allows to query
> > > "readiness" across drivers, without creating a direct dependency between
> > > the driver modules. The notify-device can also be used to trigger deferred
> > > probes.
> > > 
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/misc/Kconfig          |   4 ++
> > >  drivers/misc/Makefile         |   1 +
> > >  drivers/misc/notify-device.c  | 109 ++++++++++++++++++++++++++++++++++
> > >  include/linux/notify-device.h |  33 ++++++++++
> > >  4 files changed, 147 insertions(+)
> > >  create mode 100644 drivers/misc/notify-device.c
> > >  create mode 100644 include/linux/notify-device.h
> > > 
> > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > > index 358ad56f6524..63559e9f854c 100644
> > > --- a/drivers/misc/Kconfig
> > > +++ b/drivers/misc/Kconfig
> > > @@ -496,6 +496,10 @@ config VCPU_STALL_DETECTOR
> > >  
> > >  	  If you do not intend to run this kernel as a guest, say N.
> > >  
> > > +config NOTIFY_DEVICE
> > > +	tristate "Notify device"
> > > +	depends on OF
> > > +
> > >  source "drivers/misc/c2port/Kconfig"
> > >  source "drivers/misc/eeprom/Kconfig"
> > >  source "drivers/misc/cb710/Kconfig"
> > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > > index ac9b3e757ba1..1e8012112b43 100644
> > > --- a/drivers/misc/Makefile
> > > +++ b/drivers/misc/Makefile
> > > @@ -62,3 +62,4 @@ obj-$(CONFIG_HI6421V600_IRQ)	+= hi6421v600-irq.o
> > >  obj-$(CONFIG_OPEN_DICE)		+= open-dice.o
> > >  obj-$(CONFIG_GP_PCI1XXXX)	+= mchp_pci1xxxx/
> > >  obj-$(CONFIG_VCPU_STALL_DETECTOR)	+= vcpu_stall_detector.o
> > > +obj-$(CONFIG_NOTIFY_DEVICE)	+= notify-device.o
> > > diff --git a/drivers/misc/notify-device.c b/drivers/misc/notify-device.c
> > > new file mode 100644
> > > index 000000000000..42e0980394ea
> > > --- /dev/null
> > > +++ b/drivers/misc/notify-device.c
> > > @@ -0,0 +1,109 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +
> > > +#include <linux/device/class.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/notify-device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +
> > > +static void notify_device_release(struct device *dev)
> > > +{
> > > +	of_node_put(dev->of_node);
> > > +	kfree(dev);
> > > +}
> > > +
> > > +static struct class notify_device_class = {
> > > +	.name = "notify-device",
> > > +	.owner = THIS_MODULE,
> > > +	.dev_release = notify_device_release,
> > > +};
> > > +
> > > +static struct platform_driver notify_device_driver = {
> 
> [Pruning the CC list a bit, to avoid clogging people's inboxes]
> 
> > 
> > Ick, wait, this is NOT a platform device, nor driver, so it shouldn't be
> > either here.  Worst case, it's a virtual device on the virtual bus.
> 
> This part of the code is inspired by mac80211_hwsim, which uses a
> platform driver in a similar way, for a plain struct device. Should
> this rather use a plain struct device_driver?

It should NOT be using a platform device.

Again, a platform device should NEVER be used as a child of a device in
the tree that is on a discoverable bus.

Use the aux bus code if you don't want to create virtual devices with no
real bus, that is what it is there for.

> Also, what's the virtual bus? Grepping the Linux code and documentation
> didn't turn up anything.

Look at the stuff that ends up in /sys/devices/virtual/  Lots of users
there.

> > But why is this a class at all?  Classes are a representation of a type
> > of device that userspace can see, how is this anything that userspace
> > cares about?
> 
> Makes sense, I will remove the class.
> 
> > 
> > Doesn't the device link stuff handle all of this type of "when this
> > device is done being probed, now I can" problems?  Why is a whole new
> > thing needed?
> 
> The issue here is that (as I understand it) the device link and
> deferred probing infrastructore only cares about whether the supplier
> device has been probed successfully.
> 
> This is insuffient in the case of the dependency between mwifiex and
> hci_uart/hci_mrvl that I want to express: mwifiex loads its firmware
> asynchronously, so finishing the mwifiex probe is too early to retry
> probing the Bluetooth driver.

Welcome to deferred probing hell :)

> While mwifiex does create a few devices (ieee80211, netdevice) when the
> firmware has loaded, none of these bind to a driver, so they don't
> trigger the deferred probes. Using their existence as a condition for
> allowing the Bluetooth driver to probe also seems ugly too me
> (ieee80211 currently can't be looked up by OF node, and netdevices can
> be created and deleted dynamically).
> 
> Because of this, I came to the conclusion that creating and binding a
> device specifically for this purpose is a good solution, as it solves
> two problems at once:
> - The driver bind triggers deferred probes
> - The driver allows to look up the device by OF node
> 
> Integrating this with device links might make sense as well, but I
> haven't looked much into that yet.

Try looking at device links, I think this fits exactly what that solves.
If not, please figure out why.

thanks,

greg k-h