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
