[RFC 0/5] "notify-device" for cross-driver readiness notification

[RFC 0/5] "notify-device" for cross-driver readiness notification

[Matthias Schiffer]

This patch series is obviously missing documentation, MAINTAINERS
entries, etc., but I'd like to solicit some basic feedback on whether
this approach makes sense at all before I proceed. If it does, the
naming is also very much open for bikeshedding - I'm not too happy with
"notify-device".

The basic problem that the notify-device tries to solve is the
synchronization of firmware loading readiness between the Marvell/NXP
WLAN and Bluetooth drivers, but it may also be applicable to other
drivers.

The WLAN and Bluetooth adapters are handled by separate drivers, and may
be connected to the CPU using different interfaces (for example SDIO for
WLAN and UART for Bluetooth). However, both adapters share a single
firmware that may be uploaded via either interface.

For the SDIO+UART case, uploading the firmware via SDIO is usually
preferable, but even when the interface doesn't matter, it seems like a
good idea to clearly define which driver should handle it. To avoid
making the Bluetooth driver more complicated than necessary in this case,
we'd like to defer the probing of the driver until the firmware is ready.

For this purpose, we are introducing a notify-device, with the following
properties:

- The device is created by a driver as soon as some "readiness
  condition" is satisfied
- Creating the device also binds a stub driver, so deferred probes are
  triggered
- Looking up the notify device is possible via OF node / phandle reference

This approach avoids a hard dependency between the WLAN and Bluetooth
driver, and works regardless of the driver load order.

The first patch implementes the notify-device driver itself, and the
rest shows how the device could be hooked up to the mwifiex and hci_mrvl
drivers. A device tree making use of the notify-device could look like
the following:

    &sdhci1 {
        wifi@1 {
            compatible = "marvell,sd8987";
            reg = <1>;
    
            wifi_firmware: firmware-notifier {};
        };
    };

    &main_uart3 {
        bluetooth {
            compatible = "marvell,sd8987-bt";
            firmware-ready = <&wifi_firmware>;
        };
    };


Matthias Schiffer (5):
  misc: introduce notify-device driver
  wireless: mwifiex: signal firmware readiness using notify-device
  bluetooth: hci_mrvl: select firmwares to load by match data
  bluetooth: hci_mrvl: add support for SD8987
  bluetooth: hci_mrvl: allow waiting for firmware load using
    notify-device

 drivers/bluetooth/hci_mrvl.c                |  77 ++++++++++++--
 drivers/misc/Kconfig                        |   4 +
 drivers/misc/Makefile                       |   1 +
 drivers/misc/notify-device.c                | 109 ++++++++++++++++++++
 drivers/net/wireless/marvell/mwifiex/main.c |  14 +++
 drivers/net/wireless/marvell/mwifiex/main.h |   1 +
 include/linux/notify-device.h               |  33 ++++++
 7 files changed, 228 insertions(+), 11 deletions(-)
 create mode 100644 drivers/misc/notify-device.c
 create mode 100644 include/linux/notify-device.h

-- 
2.25.1

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

[Matthias Schiffer]

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 = {
+	.driver = {
+		.name = "notify-device",
+	},
+};
+
+struct device *notify_device_create(struct device *parent, const char *child)
+{
+	struct device_node *node;
+	struct device *dev;
+	int err;
+
+	if (!parent->of_node)
+		return ERR_PTR(-EINVAL);
+
+	node = of_get_child_by_name(parent->of_node, child);
+	if (!node)
+		return NULL;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev) {
+		of_node_put(node);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	dev_set_name(dev, "%s:%s", dev_name(parent), child);
+	dev->class = &notify_device_class;
+	dev->parent = parent;
+	dev->of_node = node;
+	err = device_register(dev);
+	if (err) {
+		put_device(dev);
+		return ERR_PTR(err);
+	}
+
+	dev->driver = &notify_device_driver.driver;
+	err = device_bind_driver(dev);
+	if (err) {
+		device_unregister(dev);
+		return ERR_PTR(err);
+	}
+
+	return dev;
+}
+EXPORT_SYMBOL_GPL(notify_device_create);
+
+void notify_device_destroy(struct device *dev)
+{
+	if (!dev)
+		return;
+
+	device_release_driver(dev);
+	device_unregister(dev);
+}
+EXPORT_SYMBOL_GPL(notify_device_destroy);
+
+struct device *notify_device_find_by_of_node(struct device_node *node)
+{
+	return class_find_device_by_of_node(&notify_device_class, node);
+}
+EXPORT_SYMBOL_GPL(notify_device_find_by_of_node);
+
+static int __init notify_device_init(void)
+{
+	int err;
+
+	err = class_register(&notify_device_class);
+	if (err)
+		return err;
+
+	err = platform_driver_register(&notify_device_driver);
+	if (err) {
+		class_unregister(&notify_device_class);
+		return err;
+	}
+
+	return 0;
+}
+
+static void __exit notify_device_exit(void)
+{
+	platform_driver_unregister(&notify_device_driver);
+	class_unregister(&notify_device_class);
+}
+
+module_init(notify_device_init);
+module_exit(notify_device_exit);
+MODULE_LICENSE("GPL");
diff --git a/include/linux/notify-device.h b/include/linux/notify-device.h
new file mode 100644
index 000000000000..f8c3e15d3b8f
--- /dev/null
+++ b/include/linux/notify-device.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_NOTIFY_DEVICE_H
+#define _LINUX_NOTIFY_DEVICE_H
+#include <linux/device.h>
+#include <linux/of.h>
+
+#ifdef CONFIG_NOTIFY_DEVICE
+
+struct device *notify_device_create(struct device *parent, const char *child);
+void notify_device_destroy(struct device *dev);
+struct device *notify_device_find_by_of_node(struct device_node *node);
+
+#else
+
+static inline struct device *notify_device_create(struct device *parent,
+						  const char *child)
+{
+	return NULL;
+}
+
+static inline void notify_device_destroy(struct device *dev)
+{
+}
+
+static inline struct device *notify_device_find_by_of_node(struct device_node *node)
+{
+	return NULL;
+}
+
+#endif
+
+#endif /* _LINUX_NOTIFY_DEVICE_H */
-- 
2.25.1

[RFC 2/5] wireless: mwifiex: signal firmware readiness using notify-device

[Matthias Schiffer]

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/net/wireless/marvell/mwifiex/main.c | 14 ++++++++++++++
 drivers/net/wireless/marvell/mwifiex/main.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index da2e6557e684..92176e90b11e 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -5,6 +5,7 @@
  * Copyright 2011-2020 NXP
  */
 
+#include <linux/notify-device.h>
 #include <linux/suspend.h>
 
 #include "main.h"
@@ -661,6 +662,16 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, void *context)
 	mwifiex_drv_get_driver_version(adapter, fmt, sizeof(fmt) - 1);
 	mwifiex_dbg(adapter, MSG, "driver_version = %s\n", fmt);
 	adapter->is_up = true;
+
+	adapter->notify_dev = notify_device_create(adapter->dev, "firmware-notifier");
+	if (IS_ERR(adapter->notify_dev)) {
+		/* This error is not fatal */
+		mwifiex_dbg(adapter, ERROR,
+			    "cannot create firmware notify device: %d\n",
+			    PTR_ERR(adapter->notify_dev));
+		adapter->notify_dev = NULL;
+	}
+
 	goto done;
 
 err_add_intf:
@@ -1482,6 +1493,9 @@ static void mwifiex_uninit_sw(struct mwifiex_adapter *adapter)
 		rtnl_unlock();
 	}
 
+	notify_device_destroy(adapter->notify_dev);
+	adapter->notify_dev = NULL;
+
 	wiphy_unregister(adapter->wiphy);
 	wiphy_free(adapter->wiphy);
 	adapter->wiphy = NULL;
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 63f861e6b28a..b28e90db3128 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -870,6 +870,7 @@ struct mwifiex_adapter {
 	int winner;
 	struct device *dev;
 	struct wiphy *wiphy;
+	struct device *notify_dev;
 	u8 perm_addr[ETH_ALEN];
 	unsigned long work_flags;
 	u32 fw_release_number;
-- 
2.25.1

[RFC 3/5] bluetooth: hci_mrvl: select firmwares to load by match data

[Matthias Schiffer]

Make the driver more generic by adding a driver info struct. We also add
support for devices without firmware (for example when the firmware is
loaded by the WLAN driver on a combined module).

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/bluetooth/hci_mrvl.c | 57 +++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
index fbc3f7c3a5c7..5d191687a34a 100644
--- a/drivers/bluetooth/hci_mrvl.c
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/tty.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/serdev.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -33,6 +34,20 @@ enum {
 	STATE_FW_REQ_PENDING,
 };
 
+struct mrvl_driver_info {
+	const char *firmware_helper;
+	const char *firmware;
+};
+
+static const struct mrvl_driver_info mrvl_driver_info_8897 = {
+	.firmware_helper = "mrvl/helper_uart_3000000.bin",
+	.firmware = "mrvl/uart8897_bt.bin",
+};
+
+/* Fallback for non-OF instances */
+static const struct mrvl_driver_info *const mrvl_driver_info_default =
+	&mrvl_driver_info_8897;
+
 struct mrvl_data {
 	struct sk_buff *rx_skb;
 	struct sk_buff_head txq;
@@ -44,6 +59,7 @@ struct mrvl_data {
 
 struct mrvl_serdev {
 	struct hci_uart hu;
+	const struct mrvl_driver_info *info;
 };
 
 struct hci_mrvl_pkt {
@@ -353,18 +369,29 @@ static int mrvl_load_firmware(struct hci_dev *hdev, const char *name)
 
 static int mrvl_setup(struct hci_uart *hu)
 {
+	const struct mrvl_driver_info *info;
 	int err;
 
-	hci_uart_set_flow_control(hu, true);
+	if (hu->serdev) {
+		struct mrvl_serdev *mrvldev = serdev_device_get_drvdata(hu->serdev);
 
-	err = mrvl_load_firmware(hu->hdev, "mrvl/helper_uart_3000000.bin");
-	if (err) {
-		bt_dev_err(hu->hdev, "Unable to download firmware helper");
-		return -EINVAL;
+		info = mrvldev->info;
+	} else {
+		info = mrvl_driver_info_default;
 	}
 
-	/* Let the final ack go out before switching the baudrate */
-	hci_uart_wait_until_sent(hu);
+	if (info->firmware_helper) {
+		hci_uart_set_flow_control(hu, true);
+
+		err = mrvl_load_firmware(hu->hdev, info->firmware_helper);
+		if (err) {
+			bt_dev_err(hu->hdev, "Unable to download firmware helper");
+			return -EINVAL;
+		}
+
+		/* Let the final ack go out before switching the baudrate */
+		hci_uart_wait_until_sent(hu);
+	}
 
 	if (hu->serdev)
 		serdev_device_set_baudrate(hu->serdev, 3000000);
@@ -373,9 +400,11 @@ static int mrvl_setup(struct hci_uart *hu)
 
 	hci_uart_set_flow_control(hu, false);
 
-	err = mrvl_load_firmware(hu->hdev, "mrvl/uart8897_bt.bin");
-	if (err)
-		return err;
+	if (info->firmware) {
+		err = mrvl_load_firmware(hu->hdev, info->firmware);
+		if (err)
+			return err;
+	}
 
 	return 0;
 }
@@ -401,6 +430,12 @@ static int mrvl_serdev_probe(struct serdev_device *serdev)
 	if (!mrvldev)
 		return -ENOMEM;
 
+	if (IS_ENABLED(CONFIG_OF)) {
+		mrvldev->info = of_device_get_match_data(&serdev->dev);
+		if (!mrvldev->info)
+			return -ENODEV;
+	}
+
 	mrvldev->hu.serdev = serdev;
 	serdev_device_set_drvdata(serdev, mrvldev);
 
@@ -416,7 +451,7 @@ static void mrvl_serdev_remove(struct serdev_device *serdev)
 
 #ifdef CONFIG_OF
 static const struct of_device_id mrvl_bluetooth_of_match[] = {
-	{ .compatible = "mrvl,88w8897" },
+	{ .compatible = "mrvl,88w8897", .data = &mrvl_driver_info_8897 },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, mrvl_bluetooth_of_match);
-- 
2.25.1

[RFC 4/5] bluetooth: hci_mrvl: add support for SD8987

[Matthias Schiffer]

Do not load any firmwares, and instead expect the firmware to be
initialized by the WLAN driver.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/bluetooth/hci_mrvl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
index 5d191687a34a..b7d764e6010f 100644
--- a/drivers/bluetooth/hci_mrvl.c
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -44,6 +44,8 @@ static const struct mrvl_driver_info mrvl_driver_info_8897 = {
 	.firmware = "mrvl/uart8897_bt.bin",
 };
 
+static const struct mrvl_driver_info mrvl_driver_info_8987 = {};
+
 /* Fallback for non-OF instances */
 static const struct mrvl_driver_info *const mrvl_driver_info_default =
 	&mrvl_driver_info_8897;
@@ -452,6 +454,7 @@ static void mrvl_serdev_remove(struct serdev_device *serdev)
 #ifdef CONFIG_OF
 static const struct of_device_id mrvl_bluetooth_of_match[] = {
 	{ .compatible = "mrvl,88w8897", .data = &mrvl_driver_info_8897 },
+	{ .compatible = "marvell,sd8987-bt", .data = &mrvl_driver_info_8987 },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, mrvl_bluetooth_of_match);
-- 
2.25.1

[RFC 5/5] bluetooth: hci_mrvl: allow waiting for firmware load using notify-device

[Matthias Schiffer]

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/bluetooth/hci_mrvl.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
index b7d764e6010f..dc55053574a9 100644
--- a/drivers/bluetooth/hci_mrvl.c
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -12,6 +12,7 @@
 #include <linux/skbuff.h>
 #include <linux/firmware.h>
 #include <linux/module.h>
+#include <linux/notify-device.h>
 #include <linux/tty.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -433,9 +434,25 @@ static int mrvl_serdev_probe(struct serdev_device *serdev)
 		return -ENOMEM;
 
 	if (IS_ENABLED(CONFIG_OF)) {
+		struct device_node *firmware_ready_node;
+		struct device *firmware_ready;
+
 		mrvldev->info = of_device_get_match_data(&serdev->dev);
 		if (!mrvldev->info)
 			return -ENODEV;
+
+		firmware_ready_node = of_parse_phandle(serdev->dev.of_node,
+						       "firmware-ready", 0);
+		if (firmware_ready_node) {
+			firmware_ready =
+				notify_device_find_by_of_node(firmware_ready_node);
+			if (!firmware_ready)
+				return -EPROBE_DEFER;
+			if (IS_ERR(firmware_ready))
+				return PTR_ERR(firmware_ready);
+			put_device(firmware_ready);
+		}
+
 	}
 
 	mrvldev->hu.serdev = serdev;
-- 
2.25.1

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

[Greg Kroah-Hartman]

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 = {

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.

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?

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?

thanks,

greg k-h

Re: [RFC 5/5] bluetooth: hci_mrvl: allow waiting for firmware load using notify-device

[Krzysztof Kozlowski]

On 26/10/2022 09:15, Matthias Schiffer wrote:
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/bluetooth/hci_mrvl.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
> index b7d764e6010f..dc55053574a9 100644
> --- a/drivers/bluetooth/hci_mrvl.c
> +++ b/drivers/bluetooth/hci_mrvl.c
> @@ -12,6 +12,7 @@
>  #include <linux/skbuff.h>
>  #include <linux/firmware.h>
>  #include <linux/module.h>
> +#include <linux/notify-device.h>
>  #include <linux/tty.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -433,9 +434,25 @@ static int mrvl_serdev_probe(struct serdev_device *serdev)
>  		return -ENOMEM;
>  
>  	if (IS_ENABLED(CONFIG_OF)) {
> +		struct device_node *firmware_ready_node;
> +		struct device *firmware_ready;
> +
>  		mrvldev->info = of_device_get_match_data(&serdev->dev);
>  		if (!mrvldev->info)
>  			return -ENODEV;
> +
> +		firmware_ready_node = of_parse_phandle(serdev->dev.of_node,
> +						       "firmware-ready", 0);

So you want us to go through five patches, find properties and OF-code,
create in our minds bindings you think about and comment on that
imaginary bindings.

I think it should work otherwise - send bindings for all of your DT
properties.

Best regards,
Krzysztof

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

[Matthias Schiffer]

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?

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

> 
> 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.

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.

Thanks,
Matthias


> 
> thanks,
> 
> greg k-h

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@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.

> 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

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

[Matthias Schiffer]

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