[PATCH 0/2] Add framework for user controlled driver probes

[PATCH 0/2] Add framework for user controlled driver probes

[Nayeemahmed Badebade]

Hi,

This patch series introduces a new framework in the form of a driver
probe-control, aimed at addressing the need for deferring the probes
from built-in drivers in kernels where modules are not used.
In such scenario, delaying the initialization of certain devices such
as pcie based devices not needed during boot and giving user the control
on probing these devices post boot, can help reduce overall boot time.
This is achieved without modifying the driver code, simply by configuring
the platform device tree.

This patch series includes 2 patches:

1) dt-binding document for the probe-control driver
   This document explains how device tree of a platform can be configured
   to use probe-control devices for deferring the probes of certain
   devices.

2) probe-control driver implementation
   This provides actual driver implementation along with relevant ABI
   documentation for the sys interfaces that driver provides to the user:
   /sys/kernel/probe_control/trigger - For triggering the probes
   /sys/kernel/debug/probe_control_status - For checking current probe status

TODO:
 * Fix changenotice warning related to MAINTAINERS file update based on
   community feedback for the patches proposed.

Thanks, Nayeem

Nayeemahmed Badebade (2):
  dt-bindings: probe-control: add probe control driver
  driver: core: add probe control driver

 .../ABI/testing/debugfs-probe-control         |  14 +
 .../ABI/testing/sysfs-kernel-probe-control    |  13 +
 .../probe-control/linux,probe-controller.yaml |  59 ++++
 drivers/base/Makefile                         |   1 +
 drivers/base/probe_control.c                  | 275 ++++++++++++++++++
 5 files changed, 362 insertions(+)
 create mode 100644 Documentation/ABI/testing/debugfs-probe-control
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-probe-control
 create mode 100644 Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml
 create mode 100644 drivers/base/probe_control.c


base-commit: 47ac09b91befbb6a235ab620c32af719f8208399
-- 
2.34.1

[PATCH 1/2] dt-bindings: probe-control: add probe control driver

[Nayeemahmed Badebade]

Device tree binding document for the probe-control driver

Signed-off-by: Toyama Yoshihiro <yoshihiro.toyama@sony.com>
Signed-off-by: Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>
---
 .../probe-control/linux,probe-controller.yaml | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml

diff --git a/Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml b/Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml
new file mode 100644
index 000000000000..1945a7a5ab3c
--- /dev/null
+++ b/Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2024 Sony Group Corporation
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/linux,probe-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Probe control device
+
+maintainers:
+  - Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>
+  - Toyama Yoshihiro <yoshihiro.toyama@sony.com>
+
+description: |
+  This binding is for controlling the probes of a set of devices in the system.
+  Probe control device is a dummy device that can be used to control the probe
+  of a group of devices. To have finer control, the devices can further be
+  divided into multiple groups and for each group a probe control device can
+  be assigned. This way, individual groups can be managed independently.
+  For example, one group can be for pcie based devices and other can be
+  scsi or usb devices.
+  Probe control device is provider node and the devices whose probes need to be
+  controlled, are consumer nodes. To establish control over consumer device
+  probes, each consumer device node need to refer the probe control provider
+  node by the phandle.
+
+properties:
+  compatible:
+    const: linux,probe-control
+
+  probe-control-supply:
+    description:
+      Phandle to the probe control provider node.
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    // The node below defines a probe control device/provider node
+    prb_ctrl_dev_0: prb_ctrl_dev_0 {
+        compatible = "linux,probe-control";
+    };
+
+    // The node below is the consumer device node that refers to provider
+    // node by its phandle and a result will not be probed until provider
+    // node is probed.
+    pcie@1ffc000 {
+        reg = <0x01ffc000 0x04000>, <0x01f00000 0x80000>;
+        #address-cells = <3>;
+        #size-cells = <2>;
+        device_type = "pci";
+        ranges = <0x81000000 0 0          0x01f80000 0 0x00010000>,
+                 <0x82000000 0 0x01000000 0x01000000 0 0x00f00000>;
+
+        probe-control-supply = <&prb_ctrl_dev_0>;
+    };
-- 
2.34.1

[PATCH 2/2] driver: core: add probe control driver

[Nayeemahmed Badebade]

Probe control driver framework allows deferring the probes of a group of
devices to an arbitrary time, giving the user control to trigger the probes
after boot. This is useful for deferring probes from builtin drivers that
are not required during boot and probe when user wants after boot.

This is achieved by adding a dummy device aka probe control device node
as provider to a group of devices(consumer nodes) in platform's device
tree. Consumers are the devices we want to probe after boot.

To establish control over consumer device probes, each consumer device node
need to refer the probe control provider node by the phandle.
'probe-control-supply' property is used for this.

Example:
    // The node below defines a probe control device/provider node
    prb_ctrl_dev_0: prb_ctrl_dev_0 {
        compatible = "linux,probe-control";
    };

    // The node below is the consumer device node that refers to provider
    // node by its phandle and a result will not be probed until provider
    // node is probed.
    pcie@1ffc000 {
        reg = <0x01ffc000 0x04000>, <0x01f00000 0x80000>;
        #address-cells = <3>;
        #size-cells = <2>;
        device_type = "pci";
        ranges = <0x81000000 0 0          0x01f80000 0 0x00010000>,
                 <0x82000000 0 0x01000000 0x01000000 0 0x00f00000>;

        probe-control-supply = <&prb_ctrl_dev_0>;
    };

fw_devlink ensures consumers are not probed until provider is probed
successfully. The provider probe during boot returns -ENXIO and is not
re-probed again.

The driver provides debug interface /sys/kernel/debug/probe_control_status
for checking probe control status of registered probe control devices.
 # cat /sys/kernel/debug/probe_control_status
 prb_ctrl_dev_0: [not triggered]
  Consumers: 1ffc000.pcie

Interface /sys/kernel/probe_control/trigger is provided for triggering
probes of the probe control devices. User can write to this interface to
trigger specific or all device probes managed by this driver.
Once the probe is triggered by user, provider probe control device is added
to deferred_probe_pending_list and driver_deferred_probe_trigger() is
triggered. This time the probe of probe control device will be
successful and its consumers will then be probed.

To trigger specific provider probe:
  # echo prb_ctrl_dev_0 > /sys/kernel/probe_control/trigger

To trigger all registered provider probes
  # echo all > /sys/kernel/probe_control/trigger

Signed-off-by: Toyama Yoshihiro <yoshihiro.toyama@sony.com>
Signed-off-by: Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>
---
 .../ABI/testing/debugfs-probe-control         |  14 +
 .../ABI/testing/sysfs-kernel-probe-control    |  13 +
 drivers/base/Makefile                         |   1 +
 drivers/base/probe_control.c                  | 275 ++++++++++++++++++
 4 files changed, 303 insertions(+)
 create mode 100644 Documentation/ABI/testing/debugfs-probe-control
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-probe-control
 create mode 100644 drivers/base/probe_control.c

diff --git a/Documentation/ABI/testing/debugfs-probe-control b/Documentation/ABI/testing/debugfs-probe-control
new file mode 100644
index 000000000000..3cd08906031a
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-probe-control
@@ -0,0 +1,14 @@
+What:		/sys/kernel/debug/probe_control_status
+Date:		September 2024
+KernelVersion:	6.11.0
+Contact:	Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>
+		Toyama Yoshihiro <yoshihiro.toyama@sony.com>
+Description:
+		Probe control driver debug interface, that shows registered
+		probe control devices which were defined in platform's device
+		tree, their probe status and respective consumer devices.
+		Sample output::
+
+		    # cat /sys/kernel/debug/probe_control_status
+		    prb_ctrl_dev_0: [not triggered]
+		     Consumers: 1ffc000.pcie
diff --git a/Documentation/ABI/testing/sysfs-kernel-probe-control b/Documentation/ABI/testing/sysfs-kernel-probe-control
new file mode 100644
index 000000000000..4602f41aa025
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-probe-control
@@ -0,0 +1,13 @@
+What:		/sys/kernel/probe_control/trigger
+Date:		September 2024
+KernelVersion:	6.11.0
+Contact:	Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>
+		Toyama Yoshihiro <yoshihiro.toyama@sony.com>
+Description:
+		Write-only attribute that allows user to trigger probing of
+		probe control devices.
+		Write specific probe control device name to trigger probing
+		of only that device or write 'all' to trigger probing of all
+		probe control devices.
+		Writing a probe control device name that is already probed,
+		will result in an error.
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 7fb21768ca36..4e2b115ea929 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -7,6 +7,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   attribute_container.o transport_class.o \
 			   topology.o container.o property.o cacheinfo.o \
 			   swnode.o
+obj-$(CONFIG_OF) += probe_control.o
 obj-$(CONFIG_AUXILIARY_BUS) += auxiliary.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-y			+= power/
diff --git a/drivers/base/probe_control.c b/drivers/base/probe_control.c
new file mode 100644
index 000000000000..6cfc03df6c33
--- /dev/null
+++ b/drivers/base/probe_control.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * probe_control.c - Probe control driver
+ *
+ * Copyright (c) 2024 Sony Group Corporation
+ */
+
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+#include <linux/kobject.h>
+#include <linux/debugfs.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/seq_file.h>
+#include <linux/device/driver.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+
+#include "base.h"
+
+/*
+ * Overview
+ *
+ * Probe control driver framework allows deferring the probes of a group of
+ * devices to an arbitrary time, giving the user control to trigger the probes
+ * after boot. This is useful for deferring probes from builtin drivers that
+ * are not required during boot and probe when user wants after boot.
+ *
+ * This is achieved by adding a dummy device aka probe control device node
+ * as provider to a group of devices(consumer nodes) in platform's device tree.
+ * Consumers are the devices we want to probe after boot.
+ * fw_devlink ensures consumers are not probed until provider is probed
+ * successfully. The provider probe during boot returns -ENXIO and is not
+ * re-probed again.
+ *
+ * The driver provides debug interface /sys/kernel/debug/probe_control_status
+ * for checking probe control status of registered probe control devices.
+ *  # cat /sys/kernel/debug/probe_control_status
+ *  prb_ctrl_dev_0: [not triggered]
+ *   Consumers: 1ffc000.pcie
+ *
+ * Interface /sys/kernel/probe_control/trigger is provided for triggering
+ * probes of the probe control devices. User can write to this interface to
+ * trigger specific or all device probes managed by this driver.
+ * Once the probe is triggered by user, provider probe control device is added
+ * to deferred_probe_pending_list and driver_deferred_probe_trigger() is
+ * triggered. This time provider probe will return successfully and consumer
+ * devices will then be probed.
+ * To trigger specific provider probe:
+ *   # echo prb_ctrl_dev_0 > /sys/kernel/probe_control/trigger
+ *
+ * To trigger all registered provider probes
+ *   # echo all > /sys/kernel/probe_control/trigger
+ *
+ * For details on configuring probe control devices in platform device tree,
+ * refer:
+ *  Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml
+ *
+ */
+
+#define MAX_PROBE_CTRL_DEVS 50
+
+struct probe_ctrl_dev_data {
+	struct device *dev;
+	bool probe;
+	struct list_head list;
+};
+
+static LIST_HEAD(probe_ctrl_dev_list);
+static DEFINE_MUTEX(probe_ctrl_dev_list_mutex);
+static atomic_t probes_pending = ATOMIC_INIT(0);
+static struct kobject *probe_ctrl_kobj;
+
+static int probe_ctrl_status_show(struct seq_file *s, void *v)
+{
+	struct probe_ctrl_dev_data *data;
+	struct device_link *link;
+
+	mutex_lock(&probe_ctrl_dev_list_mutex);
+	list_for_each_entry(data, &probe_ctrl_dev_list, list) {
+		seq_printf(s, "%s: [%s]\n", dev_name(data->dev),
+			data->probe ? "triggered" : "not triggered");
+		seq_puts(s, " Consumers:");
+		if (list_empty(&data->dev->links.consumers)) {
+			seq_puts(s, " None\n");
+			continue;
+		}
+		list_for_each_entry(link, &data->dev->links.consumers, s_node) {
+			seq_printf(s, " %s", dev_name(link->consumer));
+		}
+		seq_puts(s, "\n");
+	}
+	mutex_unlock(&probe_ctrl_dev_list_mutex);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(probe_ctrl_status);
+
+static void enable_probe(struct probe_ctrl_dev_data *data)
+{
+	/*
+	 * add the device to deferred_probe_pending_list
+	 */
+	driver_deferred_probe_add(data->dev);
+	data->probe = true;
+	atomic_dec(&probes_pending);
+	dev_dbg(data->dev, "enabled probe\n");
+}
+
+static ssize_t probe_ctrl_trigger_store(struct kobject *kobj,
+					struct kobj_attribute *attr,
+					const char *buf, size_t count)
+{
+	struct probe_ctrl_dev_data *data;
+	int probes_pending_l;
+	int ret;
+
+	probes_pending_l = atomic_read(&probes_pending);
+	if (probes_pending_l == 0)
+		return -EINVAL;
+
+	mutex_lock(&probe_ctrl_dev_list_mutex);
+	if (sysfs_streq(buf, "all")) {
+		list_for_each_entry(data, &probe_ctrl_dev_list, list) {
+			if (!data->probe)
+				enable_probe(data);
+		}
+	} else {
+		list_for_each_entry(data, &probe_ctrl_dev_list, list) {
+			if (sysfs_streq(dev_name(data->dev), buf)) {
+				if (!data->probe)
+					enable_probe(data);
+				break;
+			}
+		}
+	}
+	mutex_unlock(&probe_ctrl_dev_list_mutex);
+
+	if (probes_pending_l == atomic_read(&probes_pending)) {
+		ret = -EINVAL;
+		goto out;
+	}
+	/*
+	 * Re-probe deferred devices and
+	 * wait for device probing to be completed
+	 */
+	driver_deferred_probe_trigger();
+	wait_for_device_probe();
+	ret = count;
+
+out:
+	return ret;
+}
+
+static struct kobj_attribute probe_ctrl_attribute =
+	__ATTR(trigger, 0200, NULL, probe_ctrl_trigger_store);
+
+static struct attribute *attrs[] = {
+	&probe_ctrl_attribute.attr,
+	NULL,
+};
+
+static struct attribute_group attr_group = {
+	.attrs = attrs,
+};
+
+static int probe_control_dev_probe(struct platform_device *pdev)
+{
+	struct probe_ctrl_dev_data *data;
+	int ret;
+
+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev,
+			"driver only supports devices from device tree\n");
+		return -EINVAL;
+	}
+
+	mutex_lock(&probe_ctrl_dev_list_mutex);
+	list_for_each_entry(data, &probe_ctrl_dev_list, list) {
+		if (&pdev->dev == data->dev) {
+			ret = data->probe ? 0 : -ENXIO;
+			mutex_unlock(&probe_ctrl_dev_list_mutex);
+			dev_dbg(data->dev, "probe return: %d\n", ret);
+			return ret;
+		}
+	}
+	mutex_unlock(&probe_ctrl_dev_list_mutex);
+
+	if (atomic_read(&probes_pending) == MAX_PROBE_CTRL_DEVS) {
+		dev_dbg(&pdev->dev,
+			"Probe control device limit exceeded, probing now\n");
+		return 0;
+	}
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->dev = &pdev->dev;
+	data->probe = false;
+
+	mutex_lock(&probe_ctrl_dev_list_mutex);
+	list_add_tail(&data->list, &probe_ctrl_dev_list);
+	atomic_inc(&probes_pending);
+	mutex_unlock(&probe_ctrl_dev_list_mutex);
+
+	dev_dbg(data->dev, "Added dev to probe control list\n");
+
+	return -ENXIO;
+}
+
+static const struct of_device_id probe_ctrl_of_match[] = {
+	{ .compatible = "linux,probe-control" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, probe_ctrl_of_match);
+
+static struct platform_driver probe_control_driver = {
+	.probe          = probe_control_dev_probe,
+	.driver = {
+		.name	= "probe-control",
+		.of_match_table = probe_ctrl_of_match,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+};
+
+static int __init probe_control_init(void)
+{
+	int ret;
+
+	probe_ctrl_kobj = kobject_create_and_add("probe_control", kernel_kobj);
+	if (!probe_ctrl_kobj)
+		return -ENOMEM;
+
+	ret = sysfs_create_group(probe_ctrl_kobj, &attr_group);
+	if (ret)
+		goto out_err;
+
+	ret = platform_driver_register(&probe_control_driver);
+	if (ret)
+		goto out_err;
+
+	debugfs_create_file("probe_control_status", 0444, NULL, NULL,
+			    &probe_ctrl_status_fops);
+	return ret;
+
+out_err:
+	kobject_put(probe_ctrl_kobj);
+	return ret;
+}
+
+late_initcall(probe_control_init);
+
+static void __exit probe_control_exit(void)
+{
+	struct probe_ctrl_dev_data *data, *tmp_data;
+
+	kobject_put(probe_ctrl_kobj);
+	debugfs_lookup_and_remove("probe_control_status", NULL);
+
+	mutex_lock(&probe_ctrl_dev_list_mutex);
+	list_for_each_entry_safe(data, tmp_data, &probe_ctrl_dev_list, list) {
+		list_del(&data->list);
+		kfree(data);
+	}
+	mutex_unlock(&probe_ctrl_dev_list_mutex);
+
+	platform_driver_unregister(&probe_control_driver);
+}
+
+__exitcall(probe_control_exit);
-- 
2.34.1

Re: [PATCH 2/2] driver: core: add probe control driver

[Rob Herring]

On Wed, Sep 11, 2024 at 07:53:19PM +0530, Nayeemahmed Badebade wrote:
> Probe control driver framework allows deferring the probes of a group of
> devices to an arbitrary time, giving the user control to trigger the probes
> after boot. This is useful for deferring probes from builtin drivers that
> are not required during boot and probe when user wants after boot.

This seems like the wrong way around to me. Why not define what you want 
to probe first or some priority order? I could see use for kernel to 
probe whatever is the console device first. Or the rootfs device... You 
don't need anything added to DT for those.

Of course, there's the issue that Linux probes are triggered bottom-up 
rather than top-down.


> This is achieved by adding a dummy device aka probe control device node
> as provider to a group of devices(consumer nodes) in platform's device
> tree. Consumers are the devices we want to probe after boot.

There's the obvious question of then why not make those devices modules 
instead of built-in?

> 
> To establish control over consumer device probes, each consumer device node
> need to refer the probe control provider node by the phandle.
> 'probe-control-supply' property is used for this.
> 
> Example:
>     // The node below defines a probe control device/provider node
>     prb_ctrl_dev_0: prb_ctrl_dev_0 {
>         compatible = "linux,probe-control";
>     };
> 
>     // The node below is the consumer device node that refers to provider
>     // node by its phandle and a result will not be probed until provider
>     // node is probed.
>     pcie@1ffc000 {
>         reg = <0x01ffc000 0x04000>, <0x01f00000 0x80000>;
>         #address-cells = <3>;
>         #size-cells = <2>;
>         device_type = "pci";
>         ranges = <0x81000000 0 0          0x01f80000 0 0x00010000>,
>                  <0x82000000 0 0x01000000 0x01000000 0 0x00f00000>;
> 
>         probe-control-supply = <&prb_ctrl_dev_0>;
>     };

Sorry, but this isn't going to happen in DT.

> 
> fw_devlink ensures consumers are not probed until provider is probed
> successfully. The provider probe during boot returns -ENXIO and is not
> re-probed again.
> 
> The driver provides debug interface /sys/kernel/debug/probe_control_status
> for checking probe control status of registered probe control devices.
>  # cat /sys/kernel/debug/probe_control_status
>  prb_ctrl_dev_0: [not triggered]
>   Consumers: 1ffc000.pcie
> 
> Interface /sys/kernel/probe_control/trigger is provided for triggering
> probes of the probe control devices. User can write to this interface to
> trigger specific or all device probes managed by this driver.
> Once the probe is triggered by user, provider probe control device is added
> to deferred_probe_pending_list and driver_deferred_probe_trigger() is
> triggered. This time the probe of probe control device will be
> successful and its consumers will then be probed.
> 
> To trigger specific provider probe:
>   # echo prb_ctrl_dev_0 > /sys/kernel/probe_control/trigger
> 
> To trigger all registered provider probes
>   # echo all > /sys/kernel/probe_control/trigger
> 
> Signed-off-by: Toyama Yoshihiro <yoshihiro.toyama@sony.com>
> Signed-off-by: Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>

This is wrong. Either Toyama is the author and you need to fix the git 
author, or you both are authors and you need a Co-developed-by tag for 
Toyama.

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Greg KH]

On Wed, Sep 11, 2024 at 07:53:17PM +0530, Nayeemahmed Badebade wrote:
> Hi,

If Rob hadn't responded, I wouldn't have noticed these as they ended up
in spam for some reason.  You might want to check your email settings...

> This patch series introduces a new framework in the form of a driver
> probe-control, aimed at addressing the need for deferring the probes
> from built-in drivers in kernels where modules are not used.

Wait, why?

> In such scenario, delaying the initialization of certain devices such
> as pcie based devices not needed during boot and giving user the control
> on probing these devices post boot, can help reduce overall boot time.
> This is achieved without modifying the driver code, simply by configuring
> the platform device tree.

PCI devices should not be on the platform device tree.

And what's wrong with async probing?  That was written for this very
issue.

> This patch series includes 2 patches:
> 
> 1) dt-binding document for the probe-control driver
>    This document explains how device tree of a platform can be configured
>    to use probe-control devices for deferring the probes of certain
>    devices.

But what does that have to do with PCI devices?

> 2) probe-control driver implementation
>    This provides actual driver implementation along with relevant ABI
>    documentation for the sys interfaces that driver provides to the user:
>    /sys/kernel/probe_control/trigger - For triggering the probes

What's wrong with the existing userspace api to trigger a probe again?
Why doesn't that work?

I think you need to explain and prove why the existing apis we have that
were designed to resolve stuff like this don't work.

And if you all are abusing platform drivers and the bus there, well, I
hate to say I told you so, but...

thanks,

greg k-h

Re: [PATCH 2/2] driver: core: add probe control driver

[Greg KH]

On Wed, Sep 11, 2024 at 07:53:19PM +0530, Nayeemahmed Badebade wrote:
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-probe-control
> @@ -0,0 +1,14 @@
> +What:		/sys/kernel/debug/probe_control_status
> +Date:		September 2024
> +KernelVersion:	6.11.0

Minor nit, there's no way this can go into 6.11, you all know that :(

Re: [PATCH 2/2] driver: core: add probe control driver

[Nayeemahmed Badebade]

Hi Rob,

Thank you for taking the time to check our patch and provide
valuable feedback. We appreciate your comments/suggestions.

Please find our reply to your comments.

On Thu, Sep 12, 2024 at 03:46:34PM -0500, Rob Herring wrote:
> On Wed, Sep 11, 2024 at 07:53:19PM +0530, Nayeemahmed Badebade wrote:
> > Probe control driver framework allows deferring the probes of a group of
> > devices to an arbitrary time, giving the user control to trigger the probes
> > after boot. This is useful for deferring probes from builtin drivers that
> > are not required during boot and probe when user wants after boot.
> 
> This seems like the wrong way around to me. Why not define what you want 
> to probe first or some priority order? I could see use for kernel to 
> probe whatever is the console device first. Or the rootfs device... You 
> don't need anything added to DT for those.
> 
> Of course, there's the issue that Linux probes are triggered bottom-up 
> rather than top-down.
> 

Our intention is to only postpone some driver probes not required during
boot, similar to https://elinux.org/Deferred_Initcalls. But instead of
delaying initcall execution(which requires initmem to be kept and not
freed during boot) we are trying to delay driver probes as this is much
simpler.

> 
> > This is achieved by adding a dummy device aka probe control device node
> > as provider to a group of devices(consumer nodes) in platform's device
> > tree. Consumers are the devices we want to probe after boot.
> 
> There's the obvious question of then why not make those devices modules 
> instead of built-in?
> 

Yes we can use modules for this, but there are drivers that cannot be
built as modules and this framework is specifically for such scenario.
Example: drivers/pci/controller/dwc/pci-imx6.c

> > 
> > To establish control over consumer device probes, each consumer device node
> > need to refer the probe control provider node by the phandle.
> > 'probe-control-supply' property is used for this.
> > 
> > Example:
> >     // The node below defines a probe control device/provider node
> >     prb_ctrl_dev_0: prb_ctrl_dev_0 {
> >         compatible = "linux,probe-control";
> >     };
> > 
> >     // The node below is the consumer device node that refers to provider
> >     // node by its phandle and a result will not be probed until provider
> >     // node is probed.
> >     pcie@1ffc000 {
> >         reg = <0x01ffc000 0x04000>, <0x01f00000 0x80000>;
> >         #address-cells = <3>;
> >         #size-cells = <2>;
> >         device_type = "pci";
> >         ranges = <0x81000000 0 0          0x01f80000 0 0x00010000>,
> >                  <0x82000000 0 0x01000000 0x01000000 0 0x00f00000>;
> > 
> >         probe-control-supply = <&prb_ctrl_dev_0>;
> >     };
> 
> Sorry, but this isn't going to happen in DT.
> 

You mean we cannot add custom properties like this to an existing
device node in DT?

> > 
> > fw_devlink ensures consumers are not probed until provider is probed
> > successfully. The provider probe during boot returns -ENXIO and is not
> > re-probed again.
> > 
> > The driver provides debug interface /sys/kernel/debug/probe_control_status
> > for checking probe control status of registered probe control devices.
> >  # cat /sys/kernel/debug/probe_control_status
> >  prb_ctrl_dev_0: [not triggered]
> >   Consumers: 1ffc000.pcie
> > 
> > Interface /sys/kernel/probe_control/trigger is provided for triggering
> > probes of the probe control devices. User can write to this interface to
> > trigger specific or all device probes managed by this driver.
> > Once the probe is triggered by user, provider probe control device is added
> > to deferred_probe_pending_list and driver_deferred_probe_trigger() is
> > triggered. This time the probe of probe control device will be
> > successful and its consumers will then be probed.
> > 
> > To trigger specific provider probe:
> >   # echo prb_ctrl_dev_0 > /sys/kernel/probe_control/trigger
> > 
> > To trigger all registered provider probes
> >   # echo all > /sys/kernel/probe_control/trigger
> > 
> > Signed-off-by: Toyama Yoshihiro <yoshihiro.toyama@sony.com>
> > Signed-off-by: Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>
> 
> This is wrong. Either Toyama is the author and you need to fix the git 
> author, or you both are authors and you need a Co-developed-by tag for 
> Toyama.

Sorry about that, we will fix this.

Thanks,
Nayeem

Re: [PATCH 1/2] dt-bindings: probe-control: add probe control driver

[Krzysztof Kozlowski]

On 11/09/2024 16:23, Nayeemahmed Badebade wrote:
> Device tree binding document for the probe-control driver

Describe the hardware, not driver...

> 
> Signed-off-by: Toyama Yoshihiro <yoshihiro.toyama@sony.com>
> Signed-off-by: Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>
> ---
>  .../probe-control/linux,probe-controller.yaml | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml
> 
> diff --git a/Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml b/Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml
> new file mode 100644
> index 000000000000..1945a7a5ab3c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2024 Sony Group Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/linux,probe-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Probe control device
> +
> +maintainers:
> +  - Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>
> +  - Toyama Yoshihiro <yoshihiro.toyama@sony.com>
> +
> +description: |
> +  This binding is for controlling the probes of a set of devices in the system.
> +  Probe control device is a dummy device that can be used to control the probe
> +  of a group of devices. To have finer control, the devices can further be
> +  divided into multiple groups and for each group a probe control device can
> +  be assigned. This way, individual groups can be managed independently.
> +  For example, one group can be for pcie based devices and other can be
> +  scsi or usb devices.
> +  Probe control device is provider node and the devices whose probes need to be
> +  controlled, are consumer nodes. To establish control over consumer device
> +  probes, each consumer device node need to refer the probe control provider
> +  node by the phandle.

So all this looks like not suitable for DT at all.

> +
> +properties:
> +  compatible:
> +    const: linux,probe-control
> +
> +  probe-control-supply:
> +    description:
> +      Phandle to the probe control provider node.

I don't understand this. Regulator supply is not a provider node.

> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    // The node below defines a probe control device/provider node
> +    prb_ctrl_dev_0: prb_ctrl_dev_0 {

No underscores in node names.

> +        compatible = "linux,probe-control";

Where are the resources? It's empty?

> +    };
> +
> +    // The node below is the consumer device node that refers to provider
> +    // node by its phandle and a result will not be probed until provider
> +    // node is probed.
> +    pcie@1ffc000 {
> +        reg = <0x01ffc000 0x04000>, <0x01f00000 0x80000>;
> +        #address-cells = <3>;
> +        #size-cells = <2>;
> +        device_type = "pci";
> +        ranges = <0x81000000 0 0          0x01f80000 0 0x00010000>,
> +                 <0x82000000 0 0x01000000 0x01000000 0 0x00f00000>;
> +
> +        probe-control-supply = <&prb_ctrl_dev_0>;
> +    };

Best regards,
Krzysztof

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Krzysztof Kozlowski]

On 17/09/2024 11:06, Nayeemahmed Badebade wrote:
> Hi Greg,
> 
> Thank you for taking the time to check our patch and provide
> valuable feedback. We appreciate your comments/suggestions.
> 
> Please find our reply to your comments.
> 
> On Fri, Sep 13, 2024 at 06:36:38AM +0200, Greg KH wrote:
>> On Wed, Sep 11, 2024 at 07:53:17PM +0530, Nayeemahmed Badebade wrote:
>>> Hi,
>>
>> If Rob hadn't responded, I wouldn't have noticed these as they ended up
>> in spam for some reason.  You might want to check your email settings...
>>
> 
> I have ensured standard settings which we have been using are used this
> time, let me know if this email is received properly.
> 
>>> This patch series introduces a new framework in the form of a driver
>>> probe-control, aimed at addressing the need for deferring the probes
>>> from built-in drivers in kernels where modules are not used.
>>
>> Wait, why?
>>
> 
> We have a scenario where a driver cannot be built as a module and ends up
> as a built-in driver. We don't want to probe this driver during boot as its

Fix this instead.

> not required at the time of booting.
> Example: drivers/pci/controller/dwc/pci-imx6.c
> So the intention is to only postpone some driver probes similar to:
> https://elinux.org/Deferred_Initcalls
> But instead of delaying initcall execution(which requires initmem to be kept
> and not freed during boot) we are trying to delay driver probes as this is
> much simpler.
> The proposed driver is a generic solution for managing such driver
> probes.
> 
>>> In such scenario, delaying the initialization of certain devices such
>>> as pcie based devices not needed during boot and giving user the control
>>> on probing these devices post boot, can help reduce overall boot time.
>>> This is achieved without modifying the driver code, simply by configuring
>>> the platform device tree.
>>
>> PCI devices should not be on the platform device tree.
>>
> 
> You are right, we are referring to the pci host controller that will be
> listed in device tree and skipping its probe during boot as an example
> here.
> 
>> And what's wrong with async probing?  That was written for this very
>> issue.
>>
> 
> We have explored async probe as an option, but we noticed below:
> 1) Probe initiated via async
> 2) Boot continues with other setup.
> 3) Boot reaches stage where ip configuration is to be done via
>    ip_auto_config() and 1) is still in progress, then boot waits for all
>    async calls to be completed before proceeding with network setup.
>    ip_auto_config()
>     -> wait_for_devices()
>       -> wait_for_device_probe()
>          -> async_synchronize_full()
> 4) Similar thing happens with rootfs mount step in prepare_namespace()
>    initcall
> 
> So to avoid getting blocked on these probes which are not required
> during boot, we proposed this driver for managing such built-in driver
> probes execution.
> 
>>> This patch series includes 2 patches:
>>>
>>> 1) dt-binding document for the probe-control driver
>>>    This document explains how device tree of a platform can be configured
>>>    to use probe-control devices for deferring the probes of certain
>>>    devices.
>>
>> But what does that have to do with PCI devices?
> 
> As explained before, the driver is generic one and is for managing probes of
> drivers that are built-in.
> 
> To delay such probes, in DT we add dummy devices managed by the proposed
> driver. These dummy devices(probe control devices) will be setup as
> supplier to the device nodes that we want to probe later.

You embedded OS policy into DT. That's not really the way to go. Look
how boot phase does it. First of all - it already solves your problem.
Second - it's property of each device, not some fake provider.

> dt-binding doc patch explains this process with pci controller node as
> an example that needs to be probed later after the boot.
> 
>>


Best regards,
Krzysztof

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Nayeemahmed Badebade]

Hi Greg,

Thank you for taking the time to check our patch and provide
valuable feedback. We appreciate your comments/suggestions.

Please find our reply to your comments.

On Fri, Sep 13, 2024 at 06:36:38AM +0200, Greg KH wrote:
> On Wed, Sep 11, 2024 at 07:53:17PM +0530, Nayeemahmed Badebade wrote:
> > Hi,
> 
> If Rob hadn't responded, I wouldn't have noticed these as they ended up
> in spam for some reason.  You might want to check your email settings...
> 

I have ensured standard settings which we have been using are used this
time, let me know if this email is received properly.

> > This patch series introduces a new framework in the form of a driver
> > probe-control, aimed at addressing the need for deferring the probes
> > from built-in drivers in kernels where modules are not used.
> 
> Wait, why?
>

We have a scenario where a driver cannot be built as a module and ends up
as a built-in driver. We don't want to probe this driver during boot as its
not required at the time of booting.
Example: drivers/pci/controller/dwc/pci-imx6.c
So the intention is to only postpone some driver probes similar to:
https://elinux.org/Deferred_Initcalls
But instead of delaying initcall execution(which requires initmem to be kept
and not freed during boot) we are trying to delay driver probes as this is
much simpler.
The proposed driver is a generic solution for managing such driver
probes.

> > In such scenario, delaying the initialization of certain devices such
> > as pcie based devices not needed during boot and giving user the control
> > on probing these devices post boot, can help reduce overall boot time.
> > This is achieved without modifying the driver code, simply by configuring
> > the platform device tree.
> 
> PCI devices should not be on the platform device tree.
>

You are right, we are referring to the pci host controller that will be
listed in device tree and skipping its probe during boot as an example
here.

> And what's wrong with async probing?  That was written for this very
> issue.
>

We have explored async probe as an option, but we noticed below:
1) Probe initiated via async
2) Boot continues with other setup.
3) Boot reaches stage where ip configuration is to be done via
   ip_auto_config() and 1) is still in progress, then boot waits for all
   async calls to be completed before proceeding with network setup.
   ip_auto_config()
    -> wait_for_devices()
      -> wait_for_device_probe()
         -> async_synchronize_full()
4) Similar thing happens with rootfs mount step in prepare_namespace()
   initcall

So to avoid getting blocked on these probes which are not required
during boot, we proposed this driver for managing such built-in driver
probes execution.

> > This patch series includes 2 patches:
> > 
> > 1) dt-binding document for the probe-control driver
> >    This document explains how device tree of a platform can be configured
> >    to use probe-control devices for deferring the probes of certain
> >    devices.
> 
> But what does that have to do with PCI devices?

As explained before, the driver is generic one and is for managing probes of
drivers that are built-in.

To delay such probes, in DT we add dummy devices managed by the proposed
driver. These dummy devices(probe control devices) will be setup as
supplier to the device nodes that we want to probe later.
dt-binding doc patch explains this process with pci controller node as
an example that needs to be probed later after the boot.

> 
> > 2) probe-control driver implementation
> >    This provides actual driver implementation along with relevant ABI
> >    documentation for the sys interfaces that driver provides to the user:
> >    /sys/kernel/probe_control/trigger - For triggering the probes
> 
> What's wrong with the existing userspace api to trigger a probe again?
> Why doesn't that work?
> 

The interface is specific to triggering probes of these dummy device
nodes(probe control devices) setup as supplier to device node we want
to probe later.
As multiple probe control devices can be setup in DT, this one common
interface allows triggering of specific probe control device or all
probe control devices.
The probes of these dummy device node return success only when they are
probed like this and existing kernel framework will then probe their
consumers automatically(these are the devices we wanted to probe later
after the boot).

> I think you need to explain and prove why the existing apis we have that
> were designed to resolve stuff like this don't work.
> 
> And if you all are abusing platform drivers and the bus there, well, I
> hate to say I told you so, but...
> 
> thanks,
> 
> greg k-h

We have explained the scenario for which this driver was written and why
existing options such as async probe may not work in such scenario.
Let us know your comments on this.

Thanks,
Nayeem

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Greg KH]

On Tue, Sep 17, 2024 at 11:03:14AM +0200, Krzysztof Kozlowski wrote:
> On 17/09/2024 11:06, Nayeemahmed Badebade wrote:
> > Hi Greg,
> > 
> > Thank you for taking the time to check our patch and provide
> > valuable feedback. We appreciate your comments/suggestions.
> > 
> > Please find our reply to your comments.
> > 
> > On Fri, Sep 13, 2024 at 06:36:38AM +0200, Greg KH wrote:
> >> On Wed, Sep 11, 2024 at 07:53:17PM +0530, Nayeemahmed Badebade wrote:
> >>> Hi,
> >>
> >> If Rob hadn't responded, I wouldn't have noticed these as they ended up
> >> in spam for some reason.  You might want to check your email settings...
> >>
> > 
> > I have ensured standard settings which we have been using are used this
> > time, let me know if this email is received properly.
> > 
> >>> This patch series introduces a new framework in the form of a driver
> >>> probe-control, aimed at addressing the need for deferring the probes
> >>> from built-in drivers in kernels where modules are not used.
> >>
> >> Wait, why?
> >>
> > 
> > We have a scenario where a driver cannot be built as a module and ends up
> > as a built-in driver. We don't want to probe this driver during boot as its
> 
> Fix this instead.

Agreed, that should be much simpler to do instead of adding core driver
code that will affect all drivers/devices because just one driver
doesn't seem to be able to be fixed?

What driver is this that is causing the problem?

> > not required at the time of booting.
> > Example: drivers/pci/controller/dwc/pci-imx6.c

Just this one?  I don't see anything obvious that can't turn that into a
module, have you tried?  What went wrong?

thanks,

greg k-h

Re: [PATCH 2/2] driver: core: add probe control driver

[Nayeemahmed Badebade]

On Fri, Sep 13, 2024 at 06:37:59AM +0200, Greg KH wrote:
> On Wed, Sep 11, 2024 at 07:53:19PM +0530, Nayeemahmed Badebade wrote:
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/debugfs-probe-control
> > @@ -0,0 +1,14 @@
> > +What:		/sys/kernel/debug/probe_control_status
> > +Date:		September 2024
> > +KernelVersion:	6.11.0
> 
> Minor nit, there's no way this can go into 6.11, you all know that :(
>

Thank you for checking the patch and pointing this out.
We will update this in next iteration based on feedback for the
proposed patchset.

Thanks,
Nayeem

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Greg KH]

On Tue, Sep 17, 2024 at 02:36:55PM +0530, Nayeemahmed Badebade wrote:
> Hi Greg,
> 
> Thank you for taking the time to check our patch and provide
> valuable feedback. We appreciate your comments/suggestions.
> 
> Please find our reply to your comments.
> 
> On Fri, Sep 13, 2024 at 06:36:38AM +0200, Greg KH wrote:
> > On Wed, Sep 11, 2024 at 07:53:17PM +0530, Nayeemahmed Badebade wrote:
> > > Hi,
> > 
> > If Rob hadn't responded, I wouldn't have noticed these as they ended up
> > in spam for some reason.  You might want to check your email settings...
> > 
> 
> I have ensured standard settings which we have been using are used this
> time, let me know if this email is received properly.

I got it this time, thanks.

> > > This patch series introduces a new framework in the form of a driver
> > > probe-control, aimed at addressing the need for deferring the probes
> > > from built-in drivers in kernels where modules are not used.
> > 
> > Wait, why?
> >
> 
> We have a scenario where a driver cannot be built as a module and ends up
> as a built-in driver. We don't want to probe this driver during boot as its
> not required at the time of booting.
> Example: drivers/pci/controller/dwc/pci-imx6.c
> So the intention is to only postpone some driver probes similar to:
> https://elinux.org/Deferred_Initcalls
> But instead of delaying initcall execution(which requires initmem to be kept
> and not freed during boot) we are trying to delay driver probes as this is
> much simpler.
> The proposed driver is a generic solution for managing such driver
> probes.

Again, please fix the drivers that are having problems with this, and
build them as a module and load them when you need/want them and can be
probed correctly.

> > > In such scenario, delaying the initialization of certain devices such
> > > as pcie based devices not needed during boot and giving user the control
> > > on probing these devices post boot, can help reduce overall boot time.
> > > This is achieved without modifying the driver code, simply by configuring
> > > the platform device tree.
> > 
> > PCI devices should not be on the platform device tree.
> >
> 
> You are right, we are referring to the pci host controller that will be
> listed in device tree and skipping its probe during boot as an example
> here.

pci host controllers should really be availble at boot time, wow your
hardware is b0rked, sorry.  Just make it a module and load it when you
want/need it.

> > And what's wrong with async probing?  That was written for this very
> > issue.
> >
> 
> We have explored async probe as an option, but we noticed below:
> 1) Probe initiated via async
> 2) Boot continues with other setup.
> 3) Boot reaches stage where ip configuration is to be done via
>    ip_auto_config() and 1) is still in progress, then boot waits for all
>    async calls to be completed before proceeding with network setup.
>    ip_auto_config()
>     -> wait_for_devices()
>       -> wait_for_device_probe()
>          -> async_synchronize_full()
> 4) Similar thing happens with rootfs mount step in prepare_namespace()
>    initcall

Again, if you make the problem driver as a module you should be ok,
right?

> So to avoid getting blocked on these probes which are not required
> during boot, we proposed this driver for managing such built-in driver
> probes execution.

Fix the broken drivers please :)

thanks,

greg k-h

Re: [PATCH 2/2] driver: core: add probe control driver

[Rob Herring]

On Tue, Sep 17, 2024 at 02:25:50PM +0530, Nayeemahmed Badebade wrote:
> Hi Rob,
> 
> Thank you for taking the time to check our patch and provide
> valuable feedback. We appreciate your comments/suggestions.
> 
> Please find our reply to your comments.
> 
> On Thu, Sep 12, 2024 at 03:46:34PM -0500, Rob Herring wrote:
> > On Wed, Sep 11, 2024 at 07:53:19PM +0530, Nayeemahmed Badebade wrote:
> > > Probe control driver framework allows deferring the probes of a group of
> > > devices to an arbitrary time, giving the user control to trigger the probes
> > > after boot. This is useful for deferring probes from builtin drivers that
> > > are not required during boot and probe when user wants after boot.
> > 
> > This seems like the wrong way around to me. Why not define what you want 
> > to probe first or some priority order? I could see use for kernel to 
> > probe whatever is the console device first. Or the rootfs device... You 
> > don't need anything added to DT for those.
> > 
> > Of course, there's the issue that Linux probes are triggered bottom-up 
> > rather than top-down.
> > 
> 
> Our intention is to only postpone some driver probes not required during
> boot, similar to https://elinux.org/Deferred_Initcalls. But instead of
> delaying initcall execution(which requires initmem to be kept and not
> freed during boot) we are trying to delay driver probes as this is much
> simpler.
> 
> > 
> > > This is achieved by adding a dummy device aka probe control device node
> > > as provider to a group of devices(consumer nodes) in platform's device
> > > tree. Consumers are the devices we want to probe after boot.
> > 
> > There's the obvious question of then why not make those devices modules 
> > instead of built-in?
> > 
> 
> Yes we can use modules for this, but there are drivers that cannot be
> built as modules and this framework is specifically for such scenario.
> Example: drivers/pci/controller/dwc/pci-imx6.c

Then fix the driver to work as a module. Or to use async probe which is 
not the default and is opt-in per driver.

> 
> > > 
> > > To establish control over consumer device probes, each consumer device node
> > > need to refer the probe control provider node by the phandle.
> > > 'probe-control-supply' property is used for this.
> > > 
> > > Example:
> > >     // The node below defines a probe control device/provider node
> > >     prb_ctrl_dev_0: prb_ctrl_dev_0 {
> > >         compatible = "linux,probe-control";
> > >     };
> > > 
> > >     // The node below is the consumer device node that refers to provider
> > >     // node by its phandle and a result will not be probed until provider
> > >     // node is probed.
> > >     pcie@1ffc000 {
> > >         reg = <0x01ffc000 0x04000>, <0x01f00000 0x80000>;
> > >         #address-cells = <3>;
> > >         #size-cells = <2>;
> > >         device_type = "pci";
> > >         ranges = <0x81000000 0 0          0x01f80000 0 0x00010000>,
> > >                  <0x82000000 0 0x01000000 0x01000000 0 0x00f00000>;
> > > 
> > >         probe-control-supply = <&prb_ctrl_dev_0>;
> > >     };
> > 
> > Sorry, but this isn't going to happen in DT.
> > 
> 
> You mean we cannot add custom properties like this to an existing
> device node in DT?

Sure, you can add properties. It happens all the time. This is too tied 
to some OS implementation/behavior and therefore is not appropriate for 
DT.

Rob

Re: [PATCH 1/2] dt-bindings: probe-control: add probe control driver

[Nayeemahmed Badebade]

On Tue, Sep 17, 2024 at 11:00:23AM +0200, Krzysztof Kozlowski wrote:

Hi Krzysztof,

Sorry for the delay in our response. 
Thank you for checking our patchset and sharing your comments for it.
We appreciate your feedback.


> On 11/09/2024 16:23, Nayeemahmed Badebade wrote:
> > Device tree binding document for the probe-control driver
> 
> Describe the hardware, not driver...
> 
> > 
> > Signed-off-by: Toyama Yoshihiro <yoshihiro.toyama@sony.com>
> > Signed-off-by: Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>
> > ---
> >  .../probe-control/linux,probe-controller.yaml | 59 +++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml b/Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml
> > new file mode 100644
> > index 000000000000..1945a7a5ab3c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/probe-control/linux,probe-controller.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (c) 2024 Sony Group Corporation
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/linux,probe-controller.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Probe control device
> > +
> > +maintainers:
> > +  - Nayeemahmed Badebade <nayeemahmed.badebade@sony.com>
> > +  - Toyama Yoshihiro <yoshihiro.toyama@sony.com>
> > +
> > +description: |
> > +  This binding is for controlling the probes of a set of devices in the system.
> > +  Probe control device is a dummy device that can be used to control the probe
> > +  of a group of devices. To have finer control, the devices can further be
> > +  divided into multiple groups and for each group a probe control device can
> > +  be assigned. This way, individual groups can be managed independently.
> > +  For example, one group can be for pcie based devices and other can be
> > +  scsi or usb devices.
> > +  Probe control device is provider node and the devices whose probes need to be
> > +  controlled, are consumer nodes. To establish control over consumer device
> > +  probes, each consumer device node need to refer the probe control provider
> > +  node by the phandle.
> 
> So all this looks like not suitable for DT at all.
> 

We understand now that this approach is not suitable for DT.

> > +
> > +properties:
> > +  compatible:
> > +    const: linux,probe-control
> > +
> > +  probe-control-supply:
> > +    description:
> > +      Phandle to the probe control provider node.
> 
> I don't understand this. Regulator supply is not a provider node.
> 
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    // The node below defines a probe control device/provider node
> > +    prb_ctrl_dev_0: prb_ctrl_dev_0 {
> 
> No underscores in node names.

Got it.

> 
> > +        compatible = "linux,probe-control";
> 
> Where are the resources? It's empty?
>

Yes, this was a dummy device node for controlling probes of actual devices.
But we understand now that this approach is not right.
Thank you.

Regards,
Nayeem

Re: [PATCH 2/2] driver: core: add probe control driver

[Nayeemahmed Badebade]

Hi Rob,

Sorry for the delay in our response. 
Please find our reply to your comments.

On Wed, Sep 18, 2024 at 09:55:16AM -0500, Rob Herring wrote:
> > On Thu, Sep 12, 2024 at 03:46:34PM -0500, Rob Herring wrote:
> > > On Wed, Sep 11, 2024 at 07:53:19PM +0530, Nayeemahmed Badebade wrote:
> > > > Probe control driver framework allows deferring the probes of a group of
> > > > devices to an arbitrary time, giving the user control to trigger the probes
> > > > after boot. This is useful for deferring probes from builtin drivers that
> > > > are not required during boot and probe when user wants after boot.
> > > 
> > > This seems like the wrong way around to me. Why not define what you want 
> > > to probe first or some priority order? I could see use for kernel to 
> > > probe whatever is the console device first. Or the rootfs device... You 
> > > don't need anything added to DT for those.
> > > 
> > > Of course, there's the issue that Linux probes are triggered bottom-up 
> > > rather than top-down.
> > > 
> > 
> > Our intention is to only postpone some driver probes not required during
> > boot, similar to https://elinux.org/Deferred_Initcalls. But instead of
> > delaying initcall execution(which requires initmem to be kept and not
> > freed during boot) we are trying to delay driver probes as this is much
> > simpler.
> > 
> > > 
> > > > This is achieved by adding a dummy device aka probe control device node
> > > > as provider to a group of devices(consumer nodes) in platform's device
> > > > tree. Consumers are the devices we want to probe after boot.
> > > 
> > > There's the obvious question of then why not make those devices modules 
> > > instead of built-in?
> > > 
> > 
> > Yes we can use modules for this, but there are drivers that cannot be
> > built as modules and this framework is specifically for such scenario.
> > Example: drivers/pci/controller/dwc/pci-imx6.c
> 
> Then fix the driver to work as a module. Or to use async probe which is 
> not the default and is opt-in per driver.
> 

Sure, we will try to fix the driver and also explore how async probe can be
used for this kind of scenario.

> > 
> > > > 
> > > > To establish control over consumer device probes, each consumer device node
> > > > need to refer the probe control provider node by the phandle.
> > > > 'probe-control-supply' property is used for this.
> > > > 
> > > > Example:
> > > >     // The node below defines a probe control device/provider node
> > > >     prb_ctrl_dev_0: prb_ctrl_dev_0 {
> > > >         compatible = "linux,probe-control";
> > > >     };
> > > > 
> > > >     // The node below is the consumer device node that refers to provider
> > > >     // node by its phandle and a result will not be probed until provider
> > > >     // node is probed.
> > > >     pcie@1ffc000 {
> > > >         reg = <0x01ffc000 0x04000>, <0x01f00000 0x80000>;
> > > >         #address-cells = <3>;
> > > >         #size-cells = <2>;
> > > >         device_type = "pci";
> > > >         ranges = <0x81000000 0 0          0x01f80000 0 0x00010000>,
> > > >                  <0x82000000 0 0x01000000 0x01000000 0 0x00f00000>;
> > > > 
> > > >         probe-control-supply = <&prb_ctrl_dev_0>;
> > > >     };
> > > 
> > > Sorry, but this isn't going to happen in DT.
> > > 
> > 
> > You mean we cannot add custom properties like this to an existing
> > device node in DT?
> 
> Sure, you can add properties. It happens all the time. This is too tied 
> to some OS implementation/behavior and therefore is not appropriate for 
> DT.
> 
> Rob

We understand now that this approach is not appropriate for DT.
Thank you for your feedback.

Regards,
Nayeem

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Nayeemahmed Badebade]

Hi,

Sorry for the delay in our response.
Please find our reply to your comments:

On Tue, Sep 17, 2024 at 11:21:32AM +0200, Greg KH wrote:
> On Tue, Sep 17, 2024 at 11:03:14AM +0200, Krzysztof Kozlowski wrote:
> > On 17/09/2024 11:06, Nayeemahmed Badebade wrote:
> > > Hi Greg,
> > > 
> > > Thank you for taking the time to check our patch and provide
> > > valuable feedback. We appreciate your comments/suggestions.
> > > 
> > > Please find our reply to your comments.
> > > 
> > > On Fri, Sep 13, 2024 at 06:36:38AM +0200, Greg KH wrote:
> > >> On Wed, Sep 11, 2024 at 07:53:17PM +0530, Nayeemahmed Badebade wrote:
> > >>> Hi,
> > >>
> > >> If Rob hadn't responded, I wouldn't have noticed these as they ended up
> > >> in spam for some reason.  You might want to check your email settings...
> > >>
> > > 
> > > I have ensured standard settings which we have been using are used this
> > > time, let me know if this email is received properly.
> > > 
> > >>> This patch series introduces a new framework in the form of a driver
> > >>> probe-control, aimed at addressing the need for deferring the probes
> > >>> from built-in drivers in kernels where modules are not used.
> > >>
> > >> Wait, why?
> > >>
> > > 
> > > We have a scenario where a driver cannot be built as a module and ends up
> > > as a built-in driver. We don't want to probe this driver during boot as its
> > 
> > Fix this instead.
> 
> Agreed, that should be much simpler to do instead of adding core driver
> code that will affect all drivers/devices because just one driver
> doesn't seem to be able to be fixed?
> 
> What driver is this that is causing the problem?
> 
> > > not required at the time of booting.
> > > Example: drivers/pci/controller/dwc/pci-imx6.c
> 
> Just this one?  I don't see anything obvious that can't turn that into a
> module, have you tried?  What went wrong?
> 

Yes we have tried building it as a module.
This driver currently registers abort handler for pci using function
hook_fault_code() on arm. This function is not exported and marked with __init
tag. So we can't use it post boot.
Also from past attempt made to modularize this driver in community, we saw the
hook is not safe to be used post boot.
Reference:
 https://lore.kernel.org/linux-arm-kernel/1454889644-27830-2-git-send-email-paul.gortmaker@windriver.com/T/#m8995c6dcc40c54baef0665a7ee16d4209cb59655

We will further check this.
Thank you for your feedback.

Regards,
Nayeem

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Krzysztof Kozlowski]

On 26/09/2024 13:07, Nayeemahmed Badebade wrote:
>>
>>>> not required at the time of booting.
>>>> Example: drivers/pci/controller/dwc/pci-imx6.c
>>
>> Just this one?  I don't see anything obvious that can't turn that into a
>> module, have you tried?  What went wrong?
>>
> 
> Yes we have tried building it as a module.
> This driver currently registers abort handler for pci using function
> hook_fault_code() on arm. This function is not exported and marked with __init
> tag. So we can't use it post boot.

Then this is something to fix.

> Also from past attempt made to modularize this driver in community, we saw the
> hook is not safe to be used post boot.
> Reference:
>  https://lore.kernel.org/linux-arm-kernel/1454889644-27830-2-git-send-email-paul.gortmaker@windriver.com/T/#m8995c6dcc40c54baef0665a7ee16d4209cb59655


Best regards,
Krzysztof

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Nayeemahmed Badebade]

Hi Greg,

Sorry for the delay in our response.
Please find our reply to your comments:

On Tue, Sep 17, 2024 at 12:11:22PM +0200, Greg KH wrote:
> > On Fri, Sep 13, 2024 at 06:36:38AM +0200, Greg KH wrote:
> > > > This patch series introduces a new framework in the form of a driver
> > > > probe-control, aimed at addressing the need for deferring the probes
> > > > from built-in drivers in kernels where modules are not used.
> > > 
> > > Wait, why?
> > >
> > 
> > We have a scenario where a driver cannot be built as a module and ends up
> > as a built-in driver. We don't want to probe this driver during boot as its
> > not required at the time of booting.
> > Example: drivers/pci/controller/dwc/pci-imx6.c
> > So the intention is to only postpone some driver probes similar to:
> > https://elinux.org/Deferred_Initcalls
> > But instead of delaying initcall execution(which requires initmem to be kept
> > and not freed during boot) we are trying to delay driver probes as this is
> > much simpler.
> > The proposed driver is a generic solution for managing such driver
> > probes.
> 
> Again, please fix the drivers that are having problems with this, and
> build them as a module and load them when you need/want them and can be
> probed correctly.
> 

Sure, we will try to do that.

> > > > In such scenario, delaying the initialization of certain devices such
> > > > as pcie based devices not needed during boot and giving user the control
> > > > on probing these devices post boot, can help reduce overall boot time.
> > > > This is achieved without modifying the driver code, simply by configuring
> > > > the platform device tree.
> > > 
> > > PCI devices should not be on the platform device tree.
> > >
> > 
> > You are right, we are referring to the pci host controller that will be
> > listed in device tree and skipping its probe during boot as an example
> > here.
> 
> pci host controllers should really be availble at boot time, wow your
> hardware is b0rked, sorry.  Just make it a module and load it when you
> want/need it.
> 
> > > And what's wrong with async probing?  That was written for this very
> > > issue.
> > >
> > 
> > We have explored async probe as an option, but we noticed below:
> > 1) Probe initiated via async
> > 2) Boot continues with other setup.
> > 3) Boot reaches stage where ip configuration is to be done via
> >    ip_auto_config() and 1) is still in progress, then boot waits for all
> >    async calls to be completed before proceeding with network setup.
> >    ip_auto_config()
> >     -> wait_for_devices()
> >       -> wait_for_device_probe()
> >          -> async_synchronize_full()
> > 4) Similar thing happens with rootfs mount step in prepare_namespace()
> >    initcall
> 
> Again, if you make the problem driver as a module you should be ok,
> right?
> 

Yes.

> > So to avoid getting blocked on these probes which are not required
> > during boot, we proposed this driver for managing such built-in driver
> > probes execution.
> 
> Fix the broken drivers please :)
> 

Sure, we will do that.
Thank you for your feedback.

Regards,
Nayeem

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Nayeemahmed Badebade]

On Thu, Sep 26, 2024 at 02:34:26PM +0200, Krzysztof Kozlowski wrote:
> On 26/09/2024 13:07, Nayeemahmed Badebade wrote:
> >>
> >>>> not required at the time of booting.
> >>>> Example: drivers/pci/controller/dwc/pci-imx6.c
> >>
> >> Just this one?  I don't see anything obvious that can't turn that into a
> >> module, have you tried?  What went wrong?
> >>
> > 
> > Yes we have tried building it as a module.
> > This driver currently registers abort handler for pci using function
> > hook_fault_code() on arm. This function is not exported and marked with __init
> > tag. So we can't use it post boot.
> 
> Then this is something to fix.
> 
Thank you for the suggestion.
As per discussion in below link, its mentioned that hooks should be static and
should not change during runtime due to locking support not being there.
So its not safe to export this function to use in modules as per the comments
there.
We would appreciate any suggestions you might have on any possible
alternatives.
> > Also from past attempt made to modularize this driver in community, we saw the
> > hook is not safe to be used post boot.
> > Reference:
> >  https://lore.kernel.org/linux-arm-kernel/1454889644-27830-2-git-send-email-paul.gortmaker@windriver.com/T/#m8995c6dcc40c54baef0665a7ee16d4209cb59655
> 
> 
> Best regards,
> Krzysztof
> 

Thank you.

Regards,
Nayeem

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Rob Herring]

On Fri, Sep 27, 2024 at 10:04 AM Nayeemahmed Badebade
<nayeemahmed.badebade@sony.com> wrote:
>
> On Thu, Sep 26, 2024 at 02:34:26PM +0200, Krzysztof Kozlowski wrote:
> > On 26/09/2024 13:07, Nayeemahmed Badebade wrote:
> > >>
> > >>>> not required at the time of booting.
> > >>>> Example: drivers/pci/controller/dwc/pci-imx6.c
> > >>
> > >> Just this one?  I don't see anything obvious that can't turn that into a
> > >> module, have you tried?  What went wrong?
> > >>
> > >
> > > Yes we have tried building it as a module.
> > > This driver currently registers abort handler for pci using function
> > > hook_fault_code() on arm. This function is not exported and marked with __init
> > > tag. So we can't use it post boot.
> >
> > Then this is something to fix.
> >
> Thank you for the suggestion.
> As per discussion in below link, its mentioned that hooks should be static and
> should not change during runtime due to locking support not being there.
> So its not safe to export this function to use in modules as per the comments
> there.
> We would appreciate any suggestions you might have on any possible
> alternatives.
> > > Also from past attempt made to modularize this driver in community, we saw the
> > > hook is not safe to be used post boot.
> > > Reference:
> > >  https://lore.kernel.org/linux-arm-kernel/1454889644-27830-2-git-send-email-paul.gortmaker@windriver.com/T/#m8995c6dcc40c54baef0665a7ee16d4209cb59655

The hook implementations have no interaction with the drivers other
than being installed by the driver. So move them out of the drivers
and the handler can be built-in with the driver as a module. For
example, see arch/arm/mach-bcm/bcm_5301x.c. Could possibly combine
some implementations. I haven't figured out why imx6 checks 2
different instructions while keystone only handles one. But imx6's
implementation being a superset should work for keystone perhaps.

Rob

Re: [PATCH 0/2] Add framework for user controlled driver probes

[Nayeemahmed Badebade]

On Fri, Sep 27, 2024 at 12:36:40PM -0500, Rob Herring wrote:
> On Fri, Sep 27, 2024 at 10:04 AM Nayeemahmed Badebade
> <nayeemahmed.badebade@sony.com> wrote:
> >
> > On Thu, Sep 26, 2024 at 02:34:26PM +0200, Krzysztof Kozlowski wrote:
> > > On 26/09/2024 13:07, Nayeemahmed Badebade wrote:
> > > >>
> > > >>>> not required at the time of booting.
> > > >>>> Example: drivers/pci/controller/dwc/pci-imx6.c
> > > >>
> > > >> Just this one?  I don't see anything obvious that can't turn that into a
> > > >> module, have you tried?  What went wrong?
> > > >>
> > > >
> > > > Yes we have tried building it as a module.
> > > > This driver currently registers abort handler for pci using function
> > > > hook_fault_code() on arm. This function is not exported and marked with __init
> > > > tag. So we can't use it post boot.
> > >
> > > Then this is something to fix.
> > >
> > Thank you for the suggestion.
> > As per discussion in below link, its mentioned that hooks should be static and
> > should not change during runtime due to locking support not being there.
> > So its not safe to export this function to use in modules as per the comments
> > there.
> > We would appreciate any suggestions you might have on any possible
> > alternatives.
> > > > Also from past attempt made to modularize this driver in community, we saw the
> > > > hook is not safe to be used post boot.
> > > > Reference:
> > > >  https://lore.kernel.org/linux-arm-kernel/1454889644-27830-2-git-send-email-paul.gortmaker@windriver.com/T/#m8995c6dcc40c54baef0665a7ee16d4209cb59655
> 
> The hook implementations have no interaction with the drivers other
> than being installed by the driver. So move them out of the drivers
> and the handler can be built-in with the driver as a module. For
> example, see arch/arm/mach-bcm/bcm_5301x.c. Could possibly combine
> some implementations. I haven't figured out why imx6 checks 2
> different instructions while keystone only handles one. But imx6's
> implementation being a superset should work for keystone perhaps.
> 
> Rob

Thank you for your suggestions Rob.
Yes, this seems reasonable and we will work on updating the driver
accordingly to build it as a module.

Regards,
Nayeem