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

[Matthias Schiffer <matthias.schiffer@ew.tq-group.com>]

On Thu, 2022-10-27 at 18:48 +0200, Greg Kroah-Hartman wrote:
> 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@ew.tq-group.com>
> > > > ---
> > > >  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.

Thanks, the auxiliary bus seems to be what I'm looking 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.

According to [1], what triggers device link state changes is the
binding of drivers. As mentioned, this doesn't help in the mwifiex
case: The mwifiex probe does not wait for the firmware to load, as the
probe would otherwise take too long (see [2]).

So unless we want to extend the device link facility with a manual mode
where the supplier explicitly sets the link to a "ready" state, we
still need to extend mwifiex with a child device to attach the link to,
triggering the state change by binding it to a driver at the right
moment. Which is what this patch implements in a generic way (just
without device links so far).

Thanks,
Matthias


[1] https://www.kernel.org/doc/html/latest/driver-api/device_link.html#state-machine
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=59a4cc2539076f868f2a3fcd7a3385a26928a27a