[PATCH v6 0/22] On-demand device probing

[PATCH v6 0/22] On-demand device probing

[Tomeu Vizoso]

Hello,

I have a problem with the panel on my Tegra Chromebook taking longer
than expected to be ready during boot (Stéphane Marchesin reported what
is basically the same issue in [0]), and have looked into ordered
probing as a better way of solving this than moving nodes around in the
DT or playing with initcall levels and linking order.

While reading the thread [1] that Alexander Holler started with his
series to make probing order deterministic, it occurred to me that it
should be possible to achieve the same by probing devices as they are
referenced by other devices.

This basically reuses the information that is already implicit in the
probe() implementations, saving us from refactoring existing drivers or
adding information to DTBs.

During review of v1 of this series Linus Walleij suggested that it
should be the device driver core to make sure that dependencies are
ready before probing a device. I gave this idea a try [2] but Mark Brown
pointed out to the logic duplication between the resource acquisition
and dependency discovery code paths (though I think it's fairly minor).

To address that code duplication I experimented with Arnd's devm_probe
[3] concept of having drivers declare their dependencies instead of
acquiring them during probe, and while it worked [4], I don't think we
end up winning anything when compared to just probing devices on-demand
from resource getters.

One remaining objection is to the "sprinkling" of calls to
of_device_probe() in the resource getters of each subsystem, but I think
it's the right thing to do given that the storage of resources is
currently subsystem-specific.

We could avoid the above by moving resource storage into the core, but I
don't think there's a compelling case for that.

I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
OMAP SoCs, and these patches were enough to eliminate all the deferred
probes (except one in PandaBoard because omap_dma_system doesn't have a
firmware node as of yet).

Have submitted a branch [5][6][7] with these patches on top of today's
linux-next (20150921) to kernelci.org and I don't see any issues that
could be caused by them.

With this series I get the kernel to output to the panel in 0.5s,
instead of 2.8s.

Regards,

Tomeu

[0] http://lists.freedesktop.org/archives/dri-devel/2014-August/066527.html

[1] https://lkml.org/lkml/2014/5/12/452

[2] https://lkml.org/lkml/2015/6/17/305

[3] http://article.gmane.org/gmane.linux.ports.arm.kernel/277689

[4] https://lkml.org/lkml/2015/7/21/441a

[5] https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=on-demand-probes-v8

[6] http://kernelci.org/boot/all/job/collabora/kernel/v4.3-rc2-2587-gf92b0ab33d14/

[7] http://kernelci.org/boot/all/job/next/kernel/next-20150921

Changes in v6:
- Drop bus_type.pre_probe and read the periphid in match() instead as
  suggested by Alan Stern.
- Merge changes to the regulator subsystem's locking so no references
  are leaked between commits.

Changes in v5:
- Set the pointer to struct device also for AMBA devices
- Unset the pointer to struct device when the platform device is about
  to be unregistered
- Increase the reference count of the device before returning from
  of_find_device_by_node()
- Move the assignment to device_node->device for AMBA devices to another
  commit.
- Hold a reference to the struct device while it's in use in
  of_device_probe().
- Use regulator_class' klist of devices instead of regulator_list to
  store and lookup regulator devices.

Changes in v4:
- Added bus.pre_probe callback so the probes of Primecell devices can be
  deferred if their device IDs cannot be yet read because of the clock
  driver not having probed when they are registered. Maybe this goes
  overboard and the matching information should be in the DT if there is
  one.
- Rename of_platform_probe to of_device_probe
- Use device_node.device instead of device_node.platform_dev
- Add Kconfig DELAY_DEVICE_PROBES to allow disabling delayed probing in
  machines with initcalls that depend on devices probing at a given time.
- Start processing deferred probes in device_initcall_sync
- Also defer probes of AMBA devices registered from the DT as they can
  also request resources.

Changes in v3:
- Set and use device_node.platform_dev instead of reversing the logic to
  find the platform device that encloses a device node.
- Drop the fwnode API to probe firmware nodes and add OF-only API for
  now. I think this same scheme could be used for machines with ACPI,
  but I haven't been able to find one that had to defer its probes because
  of the device probe order.

Tomeu Vizoso (22):
  driver core: handle -EPROBE_DEFER from bus_type.match()
  ARM: amba: Move reading of periphid to amba_match()
  of/platform: Point to struct device from device node
  of: add function to allow probing a device from a OF node
  gpio: Probe GPIO drivers on demand
  gpio: Probe pinctrl devices on demand
  regulator: core: Remove regulator_list
  regulator: core: Probe regulators on demand
  drm: Probe panels on demand
  drm/tegra: Probe dpaux devices on demand
  i2c: core: Probe i2c adapters and devices on demand
  pwm: Probe PWM chip devices on demand
  backlight: Probe backlight devices on demand
  usb: phy: Probe phy devices on demand
  clk: Probe clk providers on demand
  pinctrl: Probe pinctrl devices on demand
  phy: core: Probe phy providers on demand
  dma: of: Probe DMA controllers on demand
  power-supply: Probe power supplies on demand
  driver core: Allow deferring probes until late init
  driver core: Start processing deferred probes earlier
  of/platform: Defer probes of registered devices

 drivers/amba/bus.c                  |  88 ++++++------
 drivers/base/Kconfig                |  18 +++
 drivers/base/dd.c                   |  35 ++++-
 drivers/clk/clk.c                   |   3 +
 drivers/dma/of-dma.c                |   3 +
 drivers/gpio/gpiolib-of.c           |   5 +
 drivers/gpu/drm/drm_panel.c         |   3 +
 drivers/gpu/drm/tegra/dpaux.c       |   3 +
 drivers/i2c/i2c-core.c              |   4 +
 drivers/of/device.c                 |  61 +++++++++
 drivers/of/platform.c               |  30 +++--
 drivers/phy/phy-core.c              |   3 +
 drivers/pinctrl/devicetree.c        |   3 +
 drivers/power/power_supply_core.c   |   3 +
 drivers/pwm/core.c                  |   3 +
 drivers/regulator/core.c            | 257 +++++++++++++++++++++++-------------
 drivers/usb/phy/phy.c               |   3 +
 drivers/video/backlight/backlight.c |   3 +
 include/linux/device.h              |   4 +-
 include/linux/of.h                  |   1 +
 include/linux/of_device.h           |   3 +
 21 files changed, 386 insertions(+), 150 deletions(-)

-- 
2.4.3

[PATCH v6 01/22] driver core: handle -EPROBE_DEFER from bus_type.match()

[Tomeu Vizoso]

Lets implementations of the match() callback in struct bus_type to
return errors and if it's -EPROBE_DEFER then queue the device for
deferred probing.

This is useful to buses such as AMBA in which devices are registered
before their matching information can be retrieved from the HW
(typically because a clock driver hasn't probed yet).

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/base/dd.c      | 24 ++++++++++++++++++++++--
 include/linux/device.h |  2 +-
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index be0eb4639128..7dc04ee81c8b 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -488,6 +488,7 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
 	struct device_attach_data *data = _data;
 	struct device *dev = data->dev;
 	bool async_allowed;
+	int ret;
 
 	/*
 	 * Check if device has already been claimed. This may
@@ -498,8 +499,17 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
 	if (dev->driver)
 		return -EBUSY;
 
-	if (!driver_match_device(drv, dev))
+	ret = driver_match_device(drv, dev);
+	if (!ret)
 		return 0;
+	else if (ret < 0) {
+		if (ret == -EPROBE_DEFER) {
+			dev_dbg(dev, "Device match requests probe deferral\n");
+			driver_deferred_probe_add(dev);
+		} else
+			dev_warn(dev, "Bus failed to match device: %d", ret);
+		return ret;
+	}
 
 	async_allowed = driver_allows_async_probing(drv);
 
@@ -619,6 +629,7 @@ void device_initial_probe(struct device *dev)
 static int __driver_attach(struct device *dev, void *data)
 {
 	struct device_driver *drv = data;
+	int ret;
 
 	/*
 	 * Lock device and try to bind to it. We drop the error
@@ -630,8 +641,17 @@ static int __driver_attach(struct device *dev, void *data)
 	 * is an error.
 	 */
 
-	if (!driver_match_device(drv, dev))
+	ret = driver_match_device(drv, dev);
+	if (!ret)
+		return 0;
+	else if (ret < 0) {
+		if (ret == -EPROBE_DEFER) {
+			dev_dbg(dev, "Device match requests probe deferral\n");
+			driver_deferred_probe_add(dev);
+		} else
+			dev_warn(dev, "Bus failed to match device: %d", ret);
 		return 0;
+	}
 
 	if (dev->parent)	/* Needed for USB */
 		device_lock(dev->parent);
diff --git a/include/linux/device.h b/include/linux/device.h
index 5d7bc6349930..8e7b806f0744 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -70,7 +70,7 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @dev_groups:	Default attributes of the devices on the bus.
  * @drv_groups: Default attributes of the device drivers on the bus.
  * @match:	Called, perhaps multiple times, whenever a new device or driver
- *		is added for this bus. It should return a nonzero value if the
+ *		is added for this bus. It should return a positive value if the
  *		given device can be handled by the given driver.
  * @uevent:	Called when a device is added, removed, or a few other things
  *		that generate uevents to add the environment variables.
-- 
2.4.3

[PATCH v6 02/22] ARM: amba: Move reading of periphid to amba_match()

[Tomeu Vizoso]

Reading the periphid when the Primecell device is registered means that
the apb pclk must be available by then or the device won't be registered
at all.

By reading the periphid in amba_match() we can return -EPROBE_DEFER if
the apb pclk isn't there yet and the device will be retried later.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v6:
- Drop bus_type.pre_probe and read the periphid in match() instead as
  suggested by Alan Stern.

Changes in v4:
- Added bus.pre_probe callback so the probes of Primecell devices can be
  deferred if their device IDs cannot be yet read because of the clock
  driver not having probed when they are registered. Maybe this goes
  overboard and the matching information should be in the DT if there is
  one.

 drivers/amba/bus.c | 88 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 42 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index f0099360039e..72ebf9b1c715 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -24,6 +24,8 @@
 
 #define to_amba_driver(d)	container_of(d, struct amba_driver, drv)
 
+static int read_periphid(struct amba_device *d, unsigned int *periphid);
+
 static const struct amba_id *
 amba_lookup(const struct amba_id *table, struct amba_device *dev)
 {
@@ -43,11 +45,22 @@ static int amba_match(struct device *dev, struct device_driver *drv)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *pcdrv = to_amba_driver(drv);
+	int ret;
 
 	/* When driver_override is set, only bind to the matching driver */
 	if (pcdev->driver_override)
 		return !strcmp(pcdev->driver_override, drv->name);
 
+	if (!pcdev->periphid) {
+		ret = read_periphid(pcdev, &pcdev->periphid);
+		if (ret) {
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to read periphid: %d",
+					ret);
+			return ret;
+		}
+	}
+
 	return amba_lookup(pcdrv->id_table, pcdev) != NULL;
 }
 
@@ -336,44 +349,22 @@ static void amba_device_release(struct device *dev)
 	kfree(d);
 }
 
-/**
- *	amba_device_add - add a previously allocated AMBA device structure
- *	@dev: AMBA device allocated by amba_device_alloc
- *	@parent: resource parent for this devices resources
- *
- *	Claim the resource, and read the device cell ID if not already
- *	initialized.  Register the AMBA device with the Linux device
- *	manager.
- */
-int amba_device_add(struct amba_device *dev, struct resource *parent)
+static int read_periphid(struct amba_device *d, unsigned int *periphid)
 {
 	u32 size;
 	void __iomem *tmp;
-	int i, ret;
-
-	WARN_ON(dev->irq[0] == (unsigned int)-1);
-	WARN_ON(dev->irq[1] == (unsigned int)-1);
-
-	ret = request_resource(parent, &dev->res);
-	if (ret)
-		goto err_out;
-
-	/* Hard-coded primecell ID instead of plug-n-play */
-	if (dev->periphid != 0)
-		goto skip_probe;
+	int i, ret = 0;
 
 	/*
 	 * Dynamically calculate the size of the resource
 	 * and use this for iomap
 	 */
-	size = resource_size(&dev->res);
-	tmp = ioremap(dev->res.start, size);
-	if (!tmp) {
-		ret = -ENOMEM;
-		goto err_release;
-	}
+	size = resource_size(&d->res);
+	tmp = ioremap(d->res.start, size);
+	if (!tmp)
+		return -ENOMEM;
 
-	ret = amba_get_enable_pclk(dev);
+	ret = amba_get_enable_pclk(d);
 	if (ret == 0) {
 		u32 pid, cid;
 
@@ -388,37 +379,50 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
 			cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
 				(i * 8);
 
-		amba_put_disable_pclk(dev);
+		amba_put_disable_pclk(d);
 
 		if (cid == AMBA_CID || cid == CORESIGHT_CID)
-			dev->periphid = pid;
+			*periphid = pid;
 
-		if (!dev->periphid)
+		if (!*periphid)
 			ret = -ENODEV;
 	}
 
 	iounmap(tmp);
 
+	return ret;
+}
+
+/**
+ *	amba_device_add - add a previously allocated AMBA device structure
+ *	@dev: AMBA device allocated by amba_device_alloc
+ *	@parent: resource parent for this devices resources
+ *
+ *	Claim the resource, and register the AMBA device with the Linux device
+ *	manager.
+ */
+int amba_device_add(struct amba_device *dev, struct resource *parent)
+{
+	int ret;
+
+	WARN_ON(dev->irq[0] == (unsigned int)-1);
+	WARN_ON(dev->irq[1] == (unsigned int)-1);
+
+	ret = request_resource(parent, &dev->res);
 	if (ret)
-		goto err_release;
+		return ret;
 
- skip_probe:
 	ret = device_add(&dev->dev);
 	if (ret)
-		goto err_release;
+		return ret;
 
 	if (dev->irq[0])
 		ret = device_create_file(&dev->dev, &dev_attr_irq0);
 	if (ret == 0 && dev->irq[1])
 		ret = device_create_file(&dev->dev, &dev_attr_irq1);
-	if (ret == 0)
-		return ret;
-
-	device_unregister(&dev->dev);
+	if (ret)
+		device_unregister(&dev->dev);
 
- err_release:
-	release_resource(&dev->res);
- err_out:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(amba_device_add);
-- 
2.4.3

[PATCH v6 03/22] of/platform: Point to struct device from device node

[Tomeu Vizoso]

When adding platform and AMBA devices, set the device node's device
member to point to it.

This speeds lookups considerably and is safe because we only create one
of these devices for any given device node.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v5:
- Set the pointer to struct device also for AMBA devices
- Unset the pointer to struct device when the platform device is about
  to be unregistered
- Increase the reference count of the device before returning from
  of_find_device_by_node()

 drivers/of/platform.c | 19 ++++++++++---------
 include/linux/of.h    |  1 +
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 1001efaedcb8..408d89f1d124 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -32,11 +32,6 @@ const struct of_device_id of_default_bus_match_table[] = {
 	{} /* Empty terminated list */
 };
 
-static int of_dev_node_match(struct device *dev, void *data)
-{
-	return dev->of_node == data;
-}
-
 /**
  * of_find_device_by_node - Find the platform_device associated with a node
  * @np: Pointer to device tree node
@@ -45,10 +40,10 @@ static int of_dev_node_match(struct device *dev, void *data)
  */
 struct platform_device *of_find_device_by_node(struct device_node *np)
 {
-	struct device *dev;
-
-	dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
-	return dev ? to_platform_device(dev) : NULL;
+	if (np->device && np->device->bus == &platform_bus_type &&
+	    get_device(np->device))
+		return to_platform_device(np->device);
+	return NULL;
 }
 EXPORT_SYMBOL(of_find_device_by_node);
 
@@ -192,6 +187,8 @@ static struct platform_device *of_platform_device_create_pdata(
 		goto err_clear_flag;
 	}
 
+	np->device = &dev->dev;
+
 	return dev;
 
 err_clear_flag:
@@ -272,6 +269,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
 		goto err_free;
 	}
 
+	node->device = &dev->dev;
+
 	return dev;
 
 err_free:
@@ -476,6 +475,8 @@ static int of_platform_device_destroy(struct device *dev, void *data)
 	if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
 		device_for_each_child(dev, NULL, of_platform_device_destroy);
 
+	dev->of_node->device = NULL;
+
 	if (dev->bus == &platform_bus_type)
 		platform_device_unregister(to_platform_device(dev));
 #ifdef CONFIG_ARM_AMBA
diff --git a/include/linux/of.h b/include/linux/of.h
index 2194b8ca41f9..eb091be0f8ee 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -52,6 +52,7 @@ struct device_node {
 	phandle phandle;
 	const char *full_name;
 	struct fwnode_handle fwnode;
+	struct device *device;
 
 	struct	property *properties;
 	struct	property *deadprops;	/* removed properties */
-- 
2.4.3

[PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Tomeu Vizoso]

Walks the OF tree up and finds the closest ancestor that has a struct
device associated with it, probing it if isn't bound to a driver yet.

The above should ensure that the dependency represented by the passed OF
node is available, because probing a device should cause its descendants
to be probed as well (when they get registered).

Subsystems can use this when looking up resources for drivers, to reduce
the chances of deferred probes because of the probing order of devices.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v5:
- Move the assignment to device_node->device for AMBA devices to another
  commit.
- Hold a reference to the struct device while it's in use in
  of_device_probe().

Changes in v4:
- Rename of_platform_probe to of_device_probe
- Use device_node.device instead of device_node.platform_dev

Changes in v3:
- Set and use device_node.platform_dev instead of reversing the logic to
  find the platform device that encloses a device node.
- Drop the fwnode API to probe firmware nodes and add OF-only API for
  now. I think this same scheme could be used for machines with ACPI,
  but I haven't been able to find one that had to defer its probes because
  of the device probe order.

 drivers/of/device.c       | 61 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_device.h |  3 +++
 2 files changed, 64 insertions(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 8b91ea241b10..836be71fc90e 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -286,3 +286,64 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
 
 	return 0;
 }
+
+/**
+ * of_device_probe() - Probe device associated with OF node
+ * @np: node to probe
+ *
+ * Probe the device associated with the passed device node.
+ */
+void of_device_probe(struct device_node *np)
+{
+	struct device_node *target;
+	struct device *dev = NULL;
+
+	if (!of_root || !of_node_check_flag(of_root, OF_POPULATED_BUS))
+		return;
+
+	if (!np)
+		return;
+
+	of_node_get(np);
+
+	/* Find the closest ancestor that has a device associated */
+	for (target = np;
+	     !of_node_is_root(target);
+	     target = of_get_next_parent(target))
+		if (get_device(target->device)) {
+			dev = target->device;
+			break;
+		}
+
+	of_node_put(target);
+
+	if (!dev) {
+		pr_warn("Couldn't find a device for node '%s'\n",
+			of_node_full_name(np));
+		return;
+	}
+
+	/*
+	 * Device is bound or is being probed right now. If we have bad luck
+	 * and the dependency isn't ready when it's needed, deferred probe
+	 * will save us.
+	 */
+	if (dev->driver)
+		goto out;
+
+	/*
+	 * Probing a device should cause its descendants to be probed as
+	 * well, which includes the passed device node.
+	 */
+	if (device_attach(dev) != 1)
+		/*
+		 * This cannot be a warning for now because clock nodes have a
+		 * compatible string but the clock framework doesn't follow
+		 * the device/driver model yet.
+		 */
+		dev_dbg(dev, "Probe failed for %s\n", of_node_full_name(np));
+
+out:
+	put_device(dev);
+}
+EXPORT_SYMBOL_GPL(of_device_probe);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index cc7dd687a89d..da8d489e73ad 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -40,6 +40,7 @@ extern ssize_t of_device_get_modalias(struct device *dev,
 
 extern void of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
 extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env);
+extern void of_device_probe(struct device_node *np);
 
 static inline void of_device_node_put(struct device *dev)
 {
@@ -84,6 +85,8 @@ static inline int of_device_uevent_modalias(struct device *dev,
 	return -ENODEV;
 }
 
+static inline void of_device_probe(struct device_node *np) { }
+
 static inline void of_device_node_put(struct device *dev) { }
 
 static inline const struct of_device_id *__of_match_device(
-- 
2.4.3

[PATCH v6 05/22] gpio: Probe GPIO drivers on demand

[Tomeu Vizoso]

When looking up a gpiochip through its firmware node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/gpio/gpiolib-of.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index fa6e3c8823d6..9a439dab7a87 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -19,6 +19,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/of_gpio.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/slab.h>
@@ -95,6 +96,8 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 		return ERR_PTR(ret);
 	}
 
+	of_device_probe(gg_data.gpiospec.np);
+
 	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
 
 	of_node_put(gg_data.gpiospec.np);
-- 
2.4.3

[PATCH v6 06/22] gpio: Probe pinctrl devices on demand

[Tomeu Vizoso]

When looking up a pin controller through its OF node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/gpio/gpiolib-of.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 9a439dab7a87..05da9a56608d 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -359,6 +359,8 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
 		if (ret)
 			break;
 
+		of_device_probe(pinspec.np);
+
 		pctldev = of_pinctrl_get(pinspec.np);
 		if (!pctldev)
 			return -EPROBE_DEFER;
-- 
2.4.3

[PATCH v6 07/22] regulator: core: Remove regulator_list

[Tomeu Vizoso]

As we are already registering a device with regulator_class for each
regulator device, regulator_list is redundant and can be replaced with
calls to class_find_device() and class_for_each_device().

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v6:
- Merge changes to the regulator subsystem's locking so no references
  are leaked between commits.

Changes in v5:
- Use regulator_class' klist of devices instead of regulator_list to
  store and lookup regulator devices.

 drivers/regulator/core.c | 255 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 164 insertions(+), 91 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e5dbbe2f5212..af045e5a83ef 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -51,7 +51,6 @@
 	pr_debug("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
 
 static DEFINE_MUTEX(regulator_list_mutex);
-static LIST_HEAD(regulator_list);
 static LIST_HEAD(regulator_map_list);
 static LIST_HEAD(regulator_ena_gpio_list);
 static LIST_HEAD(regulator_supply_alias_list);
@@ -59,6 +58,8 @@ static bool has_full_constraints;
 
 static struct dentry *debugfs_root;
 
+static struct class regulator_class;
+
 /*
  * struct regulator_map
  *
@@ -1325,6 +1326,47 @@ static void regulator_supply_alias(struct device **dev, const char **supply)
 	}
 }
 
+static int of_node_match(struct device *dev, const void *data)
+{
+	return dev->of_node == data;
+}
+
+static struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
+{
+	struct device *dev;
+
+	dev = class_find_device(&regulator_class, NULL, np, of_node_match);
+
+	return dev ? dev_to_rdev(dev) : NULL;
+}
+
+static int regulator_match(struct device *dev, const void *data)
+{
+	struct regulator_dev *r = dev_to_rdev(dev);
+
+	return strcmp(rdev_get_name(r), data) == 0;
+}
+
+static struct regulator_dev *regulator_lookup_by_name(const char *name)
+{
+	struct device *dev;
+
+	dev = class_find_device(&regulator_class, NULL, name, regulator_match);
+
+	return dev ? dev_to_rdev(dev) : NULL;
+}
+
+/**
+ * regulator_dev_lookup - lookup a regulator device.
+ * @dev: device for regulator "consumer".
+ * @supply: Supply name or regulator ID.
+ * @ret: 0 on success, -ENODEV if lookup fails permanently, -EPROBE_DEFER if
+ * lookup could succeed in the future.
+ *
+ * If successful, returns a struct regulator_dev that corresponds to the name
+ * @supply and with the embedded struct device refcount incremented by one,
+ * or NULL on failure. The refcount must be dropped by calling put_device().
+ */
 static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 						  const char *supply,
 						  int *ret)
@@ -1340,10 +1382,9 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	if (dev && dev->of_node) {
 		node = of_get_regulator(dev, supply);
 		if (node) {
-			list_for_each_entry(r, &regulator_list, list)
-				if (r->dev.parent &&
-					node == r->dev.of_node)
-					return r;
+			r = of_find_regulator_by_node(node);
+			if (r)
+				return r;
 			*ret = -EPROBE_DEFER;
 			return NULL;
 		} else {
@@ -1361,20 +1402,24 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	if (dev)
 		devname = dev_name(dev);
 
-	list_for_each_entry(r, &regulator_list, list)
-		if (strcmp(rdev_get_name(r), supply) == 0)
-			return r;
+	r = regulator_lookup_by_name(supply);
+	if (r)
+		return r;
 
+	mutex_lock(&regulator_list_mutex);
 	list_for_each_entry(map, &regulator_map_list, list) {
 		/* If the mapping has a device set up it must match */
 		if (map->dev_name &&
 		    (!devname || strcmp(map->dev_name, devname)))
 			continue;
 
-		if (strcmp(map->supply, supply) == 0)
+		if (strcmp(map->supply, supply) == 0 &&
+		    get_device(&map->regulator->dev)) {
+			mutex_unlock(&regulator_list_mutex);
 			return map->regulator;
+		}
 	}
-
+	mutex_unlock(&regulator_list_mutex);
 
 	return NULL;
 }
@@ -1405,6 +1450,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 
 		if (have_full_constraints()) {
 			r = dummy_regulator_rdev;
+			get_device(&r->dev);
 		} else {
 			dev_err(dev, "Failed to resolve %s-supply for %s\n",
 				rdev->supply_name, rdev->desc->name);
@@ -1414,12 +1460,16 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 
 	/* Recursively resolve the supply of the supply */
 	ret = regulator_resolve_supply(r);
-	if (ret < 0)
+	if (ret < 0) {
+		put_device(&r->dev);
 		return ret;
+	}
 
 	ret = set_supply(rdev, r);
-	if (ret < 0)
+	if (ret < 0) {
+		put_device(&r->dev);
 		return ret;
+	}
 
 	/* Cascade always-on state to supply */
 	if (_regulator_is_enabled(rdev) && rdev->supply) {
@@ -1455,8 +1505,6 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	else
 		ret = -EPROBE_DEFER;
 
-	mutex_lock(&regulator_list_mutex);
-
 	rdev = regulator_dev_lookup(dev, id, &ret);
 	if (rdev)
 		goto found;
@@ -1468,7 +1516,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	 * succeed, so, quit with appropriate error value
 	 */
 	if (ret && ret != -ENODEV)
-		goto out;
+		return regulator;
 
 	if (!devname)
 		devname = "deviceless";
@@ -1482,40 +1530,46 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 			devname, id);
 
 		rdev = dummy_regulator_rdev;
+		get_device(&rdev->dev);
 		goto found;
 	/* Don't log an error when called from regulator_get_optional() */
 	} else if (!have_full_constraints() || exclusive) {
 		dev_warn(dev, "dummy supplies not allowed\n");
 	}
 
-	mutex_unlock(&regulator_list_mutex);
 	return regulator;
 
 found:
 	if (rdev->exclusive) {
 		regulator = ERR_PTR(-EPERM);
-		goto out;
+		put_device(&rdev->dev);
+		return regulator;
 	}
 
 	if (exclusive && rdev->open_count) {
 		regulator = ERR_PTR(-EBUSY);
-		goto out;
+		put_device(&rdev->dev);
+		return regulator;
 	}
 
 	ret = regulator_resolve_supply(rdev);
 	if (ret < 0) {
 		regulator = ERR_PTR(ret);
-		goto out;
+		put_device(&rdev->dev);
+		return regulator;
 	}
 
-	if (!try_module_get(rdev->owner))
-		goto out;
+	if (!try_module_get(rdev->owner)) {
+		put_device(&rdev->dev);
+		return regulator;
+	}
 
 	regulator = create_regulator(rdev, dev, id);
 	if (regulator == NULL) {
 		regulator = ERR_PTR(-ENOMEM);
+		put_device(&rdev->dev);
 		module_put(rdev->owner);
-		goto out;
+		return regulator;
 	}
 
 	rdev->open_count++;
@@ -1529,9 +1583,6 @@ found:
 			rdev->use_count = 0;
 	}
 
-out:
-	mutex_unlock(&regulator_list_mutex);
-
 	return regulator;
 }
 
@@ -1629,6 +1680,7 @@ static void _regulator_put(struct regulator *regulator)
 
 	rdev->open_count--;
 	rdev->exclusive = 0;
+	put_device(&rdev->dev);
 	mutex_unlock(&rdev->mutex);
 
 	kfree(regulator->supply_name);
@@ -3806,8 +3858,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		}
 	}
 
-	list_add(&rdev->list, &regulator_list);
-
 	rdev_init_debugfs(rdev);
 out:
 	mutex_unlock(&regulator_list_mutex);
@@ -3861,6 +3911,19 @@ void regulator_unregister(struct regulator_dev *rdev)
 }
 EXPORT_SYMBOL_GPL(regulator_unregister);
 
+static int _regulator_suspend_prepare(struct device *dev, void *data)
+{
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+	const suspend_state_t *state = data;
+	int ret;
+
+	mutex_lock(&rdev->mutex);
+	ret = suspend_prepare(rdev, *state);
+	mutex_unlock(&rdev->mutex);
+
+	return ret;
+}
+
 /**
  * regulator_suspend_prepare - prepare regulators for system wide suspend
  * @state: system suspend state
@@ -3870,30 +3933,45 @@ EXPORT_SYMBOL_GPL(regulator_unregister);
  */
 int regulator_suspend_prepare(suspend_state_t state)
 {
-	struct regulator_dev *rdev;
-	int ret = 0;
-
 	/* ON is handled by regulator active state */
 	if (state == PM_SUSPEND_ON)
 		return -EINVAL;
 
-	mutex_lock(&regulator_list_mutex);
-	list_for_each_entry(rdev, &regulator_list, list) {
+	return class_for_each_device(&regulator_class, NULL, &state,
+				     _regulator_suspend_prepare);
+}
+EXPORT_SYMBOL_GPL(regulator_suspend_prepare);
 
-		mutex_lock(&rdev->mutex);
-		ret = suspend_prepare(rdev, state);
-		mutex_unlock(&rdev->mutex);
+static int _regulator_suspend_finish(struct device *dev, void *data)
+{
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+	int ret;
 
-		if (ret < 0) {
-			rdev_err(rdev, "failed to prepare\n");
-			goto out;
+	mutex_lock(&rdev->mutex);
+	if (rdev->use_count > 0  || rdev->constraints->always_on) {
+		if (!_regulator_is_enabled(rdev)) {
+			ret = _regulator_do_enable(rdev);
+			if (ret)
+				dev_err(dev,
+					"Failed to resume regulator %d\n",
+					ret);
 		}
+	} else {
+		if (!have_full_constraints())
+			goto unlock;
+		if (!_regulator_is_enabled(rdev))
+			goto unlock;
+
+		ret = _regulator_do_disable(rdev);
+		if (ret)
+			dev_err(dev, "Failed to suspend regulator %d\n", ret);
 	}
-out:
-	mutex_unlock(&regulator_list_mutex);
-	return ret;
+unlock:
+	mutex_unlock(&rdev->mutex);
+
+	/* Keep processing regulators in spite of any errors */
+	return 0;
 }
-EXPORT_SYMBOL_GPL(regulator_suspend_prepare);
 
 /**
  * regulator_suspend_finish - resume regulators from system wide suspend
@@ -3903,33 +3981,8 @@ EXPORT_SYMBOL_GPL(regulator_suspend_prepare);
  */
 int regulator_suspend_finish(void)
 {
-	struct regulator_dev *rdev;
-	int ret = 0, error;
-
-	mutex_lock(&regulator_list_mutex);
-	list_for_each_entry(rdev, &regulator_list, list) {
-		mutex_lock(&rdev->mutex);
-		if (rdev->use_count > 0  || rdev->constraints->always_on) {
-			if (!_regulator_is_enabled(rdev)) {
-				error = _regulator_do_enable(rdev);
-				if (error)
-					ret = error;
-			}
-		} else {
-			if (!have_full_constraints())
-				goto unlock;
-			if (!_regulator_is_enabled(rdev))
-				goto unlock;
-
-			error = _regulator_do_disable(rdev);
-			if (error)
-				ret = error;
-		}
-unlock:
-		mutex_unlock(&rdev->mutex);
-	}
-	mutex_unlock(&regulator_list_mutex);
-	return ret;
+	return class_for_each_device(&regulator_class, NULL, NULL,
+				     _regulator_suspend_finish);
 }
 EXPORT_SYMBOL_GPL(regulator_suspend_finish);
 
@@ -4049,14 +4102,35 @@ static const struct file_operations supply_map_fops = {
 };
 
 #ifdef CONFIG_DEBUG_FS
+struct summary_data {
+	struct seq_file *s;
+	struct regulator_dev *parent;
+	int level;
+};
+
+static void regulator_summary_show_subtree(struct seq_file *s,
+					   struct regulator_dev *rdev,
+					   int level);
+
+static int regulator_summary_show_children(struct device *dev, void *data)
+{
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+	struct summary_data *summary_data = data;
+
+	if (rdev->supply && rdev->supply->rdev == summary_data->parent)
+		regulator_summary_show_subtree(summary_data->s, rdev,
+					       summary_data->level + 1);
+
+	return 0;
+}
+
 static void regulator_summary_show_subtree(struct seq_file *s,
 					   struct regulator_dev *rdev,
 					   int level)
 {
-	struct list_head *list = s->private;
-	struct regulator_dev *child;
 	struct regulation_constraints *c;
 	struct regulator *consumer;
+	struct summary_data summary_data;
 
 	if (!rdev)
 		return;
@@ -4106,33 +4180,32 @@ static void regulator_summary_show_subtree(struct seq_file *s,
 		seq_puts(s, "\n");
 	}
 
-	list_for_each_entry(child, list, list) {
-		/* handle only non-root regulators supplied by current rdev */
-		if (!child->supply || child->supply->rdev != rdev)
-			continue;
+	summary_data.s = s;
+	summary_data.level = level;
+	summary_data.parent = rdev;
 
-		regulator_summary_show_subtree(s, child, level + 1);
-	}
+	class_for_each_device(&regulator_class, NULL, &summary_data,
+			      regulator_summary_show_children);
 }
 
-static int regulator_summary_show(struct seq_file *s, void *data)
+static int regulator_summary_show_roots(struct device *dev, void *data)
 {
-	struct list_head *list = s->private;
-	struct regulator_dev *rdev;
-
-	seq_puts(s, " regulator                      use open bypass voltage current     min     max\n");
-	seq_puts(s, "-------------------------------------------------------------------------------\n");
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+	struct seq_file *s = data;
 
-	mutex_lock(&regulator_list_mutex);
+	if (!rdev->supply)
+		regulator_summary_show_subtree(s, rdev, 0);
 
-	list_for_each_entry(rdev, list, list) {
-		if (rdev->supply)
-			continue;
+	return 0;
+}
 
-		regulator_summary_show_subtree(s, rdev, 0);
-	}
+static int regulator_summary_show(struct seq_file *s, void *data)
+{
+	seq_puts(s, " regulator                      use open bypass voltage current     min     max\n");
+	seq_puts(s, "-------------------------------------------------------------------------------\n");
 
-	mutex_unlock(&regulator_list_mutex);
+	class_for_each_device(&regulator_class, NULL, s,
+			      regulator_summary_show_roots);
 
 	return 0;
 }
@@ -4166,7 +4239,7 @@ static int __init regulator_init(void)
 			    &supply_map_fops);
 
 	debugfs_create_file("regulator_summary", 0444, debugfs_root,
-			    &regulator_list, &regulator_summary_fops);
+			    NULL, &regulator_summary_fops);
 
 	regulator_dummy_init();
 
-- 
2.4.3

[PATCH v6 08/22] regulator: core: Probe regulators on demand

[Tomeu Vizoso]

When looking up a regulator through its OF node, probe it if it hasn't
already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/regulator/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index af045e5a83ef..a6ed657c691b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -26,6 +26,7 @@
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/regmap.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/consumer.h>
@@ -1382,6 +1383,7 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	if (dev && dev->of_node) {
 		node = of_get_regulator(dev, supply);
 		if (node) {
+			of_device_probe(node);
 			r = of_find_regulator_by_node(node);
 			if (r)
 				return r;
-- 
2.4.3

[PATCH v6 09/22] drm: Probe panels on demand

[Tomeu Vizoso]

When looking up a panel through its OF node, probe it if it hasn't
already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/gpu/drm/drm_panel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 2ef988e037b7..ad79a7b9c74d 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -23,6 +23,7 @@
 
 #include <linux/err.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 
 #include <drm/drm_crtc.h>
 #include <drm/drm_panel.h>
@@ -80,6 +81,8 @@ struct drm_panel *of_drm_find_panel(struct device_node *np)
 {
 	struct drm_panel *panel;
 
+	of_device_probe(np);
+
 	mutex_lock(&panel_lock);
 
 	list_for_each_entry(panel, &panel_list, list) {
-- 
2.4.3

[PATCH v6 10/22] drm/tegra: Probe dpaux devices on demand

[Tomeu Vizoso]

When looking up a dpaux device through its OF node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/gpu/drm/tegra/dpaux.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
index 6aecb6647313..40bdc2a98548 100644
--- a/drivers/gpu/drm/tegra/dpaux.c
+++ b/drivers/gpu/drm/tegra/dpaux.c
@@ -12,6 +12,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/of_gpio.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
 #include <linux/regulator/consumer.h>
@@ -440,6 +441,8 @@ struct tegra_dpaux *tegra_dpaux_find_by_of_node(struct device_node *np)
 {
 	struct tegra_dpaux *dpaux;
 
+	of_device_probe(np);
+
 	mutex_lock(&dpaux_lock);
 
 	list_for_each_entry(dpaux, &dpaux_list, list)
-- 
2.4.3

[PATCH v6 11/22] i2c: core: Probe i2c adapters and devices on demand

[Tomeu Vizoso]

When looking up an i2c adapter or device through its OF node, probe it
if it hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/i2c/i2c-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 5f89f1e3c2f2..02da3acbbd35 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1413,6 +1413,8 @@ struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
 	struct device *dev;
 	struct i2c_client *client;
 
+	of_device_probe(node);
+
 	dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
 	if (!dev)
 		return NULL;
@@ -1431,6 +1433,8 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 	struct device *dev;
 	struct i2c_adapter *adapter;
 
+	of_device_probe(node);
+
 	dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
 	if (!dev)
 		return NULL;
-- 
2.4.3

[PATCH v6 12/22] pwm: Probe PWM chip devices on demand

[Tomeu Vizoso]

When looking up a PWM chip through its OF node, probe it if it hasn't
already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/pwm/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3f9df3ea3350..794a923df0d8 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -29,6 +29,7 @@
 #include <linux/device.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
+#include <linux/of_device.h>
 
 #include <dt-bindings/pwm/pwm.h>
 
@@ -516,6 +517,8 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
 {
 	struct pwm_chip *chip;
 
+	of_device_probe(np);
+
 	mutex_lock(&pwm_lock);
 
 	list_for_each_entry(chip, &pwm_chips, list)
-- 
2.4.3

[PATCH v6 13/22] backlight: Probe backlight devices on demand

[Tomeu Vizoso]

When looking up a backlight device through its OF node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/video/backlight/backlight.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index bddc8b17a4d8..9bcdc16eacdf 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/fb.h>
 #include <linux/slab.h>
+#include <linux/of_device.h>
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 #include <asm/backlight.h>
@@ -559,6 +560,8 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
 {
 	struct device *dev;
 
+	of_device_probe(node);
+
 	dev = class_find_device(backlight_class, NULL, node, of_parent_match);
 
 	return dev ? to_backlight_device(dev) : NULL;
-- 
2.4.3

[PATCH v6 14/22] usb: phy: Probe phy devices on demand

[Tomeu Vizoso]

When looking up a phy through its OF node, probe it if it hasn't
already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/usb/phy/phy.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
index 98f75d2842b7..fb0b650bb494 100644
--- a/drivers/usb/phy/phy.c
+++ b/drivers/usb/phy/phy.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <linux/usb/phy.h>
 
@@ -196,6 +197,8 @@ struct  usb_phy *devm_usb_get_phy_by_node(struct device *dev,
 		goto err0;
 	}
 
+	of_device_probe(node);
+
 	spin_lock_irqsave(&phy_lock, flags);
 
 	phy = __of_usb_find_phy(node);
-- 
2.4.3

[PATCH v6 15/22] clk: Probe clk providers on demand

[Tomeu Vizoso]

When looking up a clock through its OF node, probe it if it hasn't
already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b005f666e3a1..8986c9d876a4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -19,6 +19,7 @@
 #include <linux/list.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/sched.h>
@@ -3005,6 +3006,8 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 	if (!clkspec)
 		return ERR_PTR(-EINVAL);
 
+	of_device_probe(clkspec->np);
+
 	/* Check if we have such a provider in our array */
 	mutex_lock(&of_clk_mutex);
 	list_for_each_entry(provider, &of_clk_providers, link) {
-- 
2.4.3

[PATCH v6 16/22] pinctrl: Probe pinctrl devices on demand

[Tomeu Vizoso]

When looking up a pin controller through its OF node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/pinctrl/devicetree.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fe04e748dfe4..f5340b8e1dbe 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -18,6 +18,7 @@
 
 #include <linux/device.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/slab.h>
 
@@ -110,6 +111,8 @@ static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
 	struct pinctrl_map *map;
 	unsigned num_maps;
 
+	of_device_probe(np_config);
+
 	/* Find the pin controller containing np_config */
 	np_pctldev = of_node_get(np_config);
 	for (;;) {
-- 
2.4.3

[PATCH v6 17/22] phy: core: Probe phy providers on demand

[Tomeu Vizoso]

When looking up a phy provider through its OF node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/phy/phy-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index fc48fac003a6..94e90031d7f3 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -18,6 +18,7 @@
 #include <linux/device.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/phy/phy.h>
 #include <linux/idr.h>
 #include <linux/pm_runtime.h>
@@ -363,6 +364,8 @@ static struct phy *_of_phy_get(struct device_node *np, int index)
 	if (ret)
 		return ERR_PTR(-ENODEV);
 
+	of_device_probe(args.np);
+
 	mutex_lock(&phy_provider_mutex);
 	phy_provider = of_phy_provider_lookup(args.np);
 	if (IS_ERR(phy_provider) || !try_module_get(phy_provider->owner)) {
-- 
2.4.3

[PATCH v6 18/22] dma: of: Probe DMA controllers on demand

[Tomeu Vizoso]

When looking up a DMA controller through its OF node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/dma/of-dma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 1e1f2986eba8..e899832f7df3 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -16,6 +16,7 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_dma.h>
 
 static LIST_HEAD(of_dma_list);
@@ -263,6 +264,8 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 		if (of_dma_match_channel(np, name, i, &dma_spec))
 			continue;
 
+		of_device_probe(dma_spec.np);
+
 		mutex_lock(&of_dma_lock);
 		ofdma = of_dma_find_controller(&dma_spec);
 
-- 
2.4.3

[PATCH v6 19/22] power-supply: Probe power supplies on demand

[Tomeu Vizoso]

When looking up a power supply through its OF node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---


 drivers/power/power_supply_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 456987c88baa..80bc89f4ae89 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -19,6 +19,7 @@
 #include <linux/err.h>
 #include <linux/power_supply.h>
 #include <linux/thermal.h>
+#include <linux/of_device.h>
 #include "power_supply.h"
 
 /* exported for the APM Power driver, APM emulation */
@@ -206,6 +207,8 @@ static int power_supply_find_supply_from_node(struct device_node *supply_node)
 {
 	int error;
 
+	of_device_probe(supply_node);
+
 	/*
 	 * class_for_each_device() either returns its own errors or values
 	 * returned by __power_supply_find_supply_from_node().
-- 
2.4.3

[PATCH v6 20/22] driver core: Allow deferring probes until late init

[Tomeu Vizoso]

Add a field to struct device that instructs the device-driver core to
defer the probe of this device until the late_initcall level.

By letting all built-in drivers to register before starting to probe, we
can avoid any deferred probes by probing dependencies on demand.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

---

Changes in v4:
- Add Kconfig DELAY_DEVICE_PROBES to allow disabling delayed probing in
  machines with initcalls that depend on devices probing at a given time.

 drivers/base/Kconfig   | 18 ++++++++++++++++++
 drivers/base/dd.c      |  7 +++++++
 include/linux/device.h |  2 ++
 3 files changed, 27 insertions(+)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 98504ec99c7d..44b5d33b1f49 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -324,4 +324,22 @@ config CMA_ALIGNMENT
 
 endif
 
+config DELAY_DEVICE_PROBES
+	bool "Allow delaying the probe of some devices"
+	default y
+	help
+	  Devices can be matched to a driver and probed from the moment they
+	  are registered, but early during boot their probes are likely to be
+	  deferred because some dependency isn't available yet because most
+	  drivers haven't been registered yet.
+
+	  Enabling this option allows the device registration code to delay the
+	  probing of a specific device until device_initcall_sync, when all
+	  built-in drivers have been registered already.
+
+	  In some platforms there may be implicit assumptions about when some
+	  devices are probed, so enabling this option could cause problems there.
+
+	  If unsure, say Y here.
+
 endmenu
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 7dc04ee81c8b..269ea76c1a4f 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -417,6 +417,13 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
 	if (!device_is_registered(dev))
 		return -ENODEV;
 
+#if IS_ENABLED(CONFIG_DELAY_DEVICE_PROBES)
+	if (!driver_deferred_probe_enable && dev->probe_late) {
+		driver_deferred_probe_add(dev);
+		return 0;
+	}
+#endif
+
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 8e7b806f0744..e64f4c7e243d 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -744,6 +744,7 @@ struct device_dma_parameters {
  *
  * @offline_disabled: If set, the device is permanently online.
  * @offline:	Set after successful invocation of bus type's .offline().
+ * @probe_late:	If set, device will be probed in the late initcall level.
  *
  * At the lowest level, every device in a Linux system is represented by an
  * instance of struct device. The device structure contains the information
@@ -828,6 +829,7 @@ struct device {
 
 	bool			offline_disabled:1;
 	bool			offline:1;
+	bool			probe_late:1;
 };
 
 static inline struct device *kobj_to_dev(struct kobject *kobj)
-- 
2.4.3

[PATCH v6 21/22] driver core: Start processing deferred probes earlier

[Tomeu Vizoso]

Some initcalls in the late level assume that some devices will have
already probed without explicitly checking for that.

After the recent move to defer most device probes when they are
registered, pressure increased in the late initcall level.

By starting the processing of the deferred queue in device_initcall_sync
we increase the chances that the initcalls mentioned before will find
the devices they depend on to have already probed.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v4:
- Start processing deferred probes in device_initcall_sync

 drivers/base/dd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 269ea76c1a4f..f0ef9233fcd6 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -176,7 +176,7 @@ static void driver_deferred_probe_trigger(void)
  *
  * We don't want to get in the way when the bulk of drivers are getting probed.
  * Instead, this initcall makes sure that deferred probing is delayed until
- * late_initcall time.
+ * device_initcall_sync time.
  */
 static int deferred_probe_initcall(void)
 {
@@ -190,7 +190,7 @@ static int deferred_probe_initcall(void)
 	flush_workqueue(deferred_wq);
 	return 0;
 }
-late_initcall(deferred_probe_initcall);
+device_initcall_sync(deferred_probe_initcall);
 
 static void driver_bound(struct device *dev)
 {
-- 
2.4.3

[PATCH v6 22/22] of/platform: Defer probes of registered devices

[Tomeu Vizoso]

Instead of trying to match and probe platform and AMBA devices right
after each is registered, delay their probes until device_initcall_sync.

This means that devices will start probing once all built-in drivers
have registered, and after all platform and AMBA devices from the DT
have been registered already.

This allows us to prevent deferred probes by probing dependencies on
demand.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v4:
- Also defer probes of AMBA devices registered from the DT as they can
  also request resources.

 drivers/of/platform.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 408d89f1d124..7b33e0369374 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -164,7 +164,8 @@ static struct platform_device *of_platform_device_create_pdata(
 					struct device_node *np,
 					const char *bus_id,
 					void *platform_data,
-					struct device *parent)
+					struct device *parent,
+					bool probe_late)
 {
 	struct platform_device *dev;
 
@@ -178,6 +179,7 @@ static struct platform_device *of_platform_device_create_pdata(
 
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
+	dev->dev.probe_late = probe_late;
 	of_dma_configure(&dev->dev, dev->dev.of_node);
 	of_msi_configure(&dev->dev, dev->dev.of_node);
 
@@ -209,7 +211,8 @@ struct platform_device *of_platform_device_create(struct device_node *np,
 					    const char *bus_id,
 					    struct device *parent)
 {
-	return of_platform_device_create_pdata(np, bus_id, NULL, parent);
+	return of_platform_device_create_pdata(np, bus_id, NULL, parent,
+					       false);
 }
 EXPORT_SYMBOL(of_platform_device_create);
 
@@ -240,6 +243,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
 	dev->dev.of_node = of_node_get(node);
 	dev->dev.parent = parent ? : &platform_bus;
 	dev->dev.platform_data = platform_data;
+	dev->dev.probe_late = true;
 	if (bus_id)
 		dev_set_name(&dev->dev, "%s", bus_id);
 	else
@@ -358,7 +362,8 @@ static int of_platform_bus_create(struct device_node *bus,
 		return 0;
 	}
 
-	dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
+	dev = of_platform_device_create_pdata(bus, bus_id, platform_data,
+					      parent, true);
 	if (!dev || !of_match_node(matches, bus))
 		return 0;
 
-- 
2.4.3

Re: [PATCH v6 07/22] regulator: core: Remove regulator_list

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On Mon, Sep 21, 2015 at 04:02:47PM +0200, Tomeu Vizoso wrote:
> As we are already registering a device with regulator_class for each
> regulator device, regulator_list is redundant and can be replaced with
> calls to class_find_device() and class_for_each_device().

I've done an initial review of this and will push it to git so it can go
into the various test systems.  It's on a branch by itself so if there's
issues it's easy to back out, either from testing or from review from me
or others.  If we get to the point of merging the whole series then I'll
tag the code for cross merging.

Thanks a lot for working on this, this is something I'd started looking
at myself and it's a lot easier to just review code from someone else!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v6 08/22] regulator: core: Probe regulators on demand

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

On Mon, Sep 21, 2015 at 04:02:48PM +0200, Tomeu Vizoso wrote:
> When looking up a regulator through its OF node, probe it if it hasn't
> already.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v6 03/22] of/platform: Point to struct device from device node

[Rob Herring]

On Mon, Sep 21, 2015 at 9:02 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> When adding platform and AMBA devices, set the device node's device
> member to point to it.
>
> This speeds lookups considerably and is safe because we only create one
> of these devices for any given device node.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
> Changes in v5:
> - Set the pointer to struct device also for AMBA devices
> - Unset the pointer to struct device when the platform device is about
>   to be unregistered
> - Increase the reference count of the device before returning from
>   of_find_device_by_node()
>
>  drivers/of/platform.c | 19 ++++++++++---------
>  include/linux/of.h    |  1 +
>  2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 1001efaedcb8..408d89f1d124 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -32,11 +32,6 @@ const struct of_device_id of_default_bus_match_table[] = {
>         {} /* Empty terminated list */
>  };
>
> -static int of_dev_node_match(struct device *dev, void *data)
> -{
> -       return dev->of_node == data;
> -}
> -
>  /**
>   * of_find_device_by_node - Find the platform_device associated with a node
>   * @np: Pointer to device tree node
> @@ -45,10 +40,10 @@ static int of_dev_node_match(struct device *dev, void *data)
>   */
>  struct platform_device *of_find_device_by_node(struct device_node *np)
>  {
> -       struct device *dev;
> -
> -       dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
> -       return dev ? to_platform_device(dev) : NULL;
> +       if (np->device && np->device->bus == &platform_bus_type &&
> +           get_device(np->device))

Where do we drop the reference incremented by get_device?

However, bus_find_device also took a reference I think, so you aren't
really changing behavior.

Rob

Re: [PATCH v6 03/22] of/platform: Point to struct device from device node

[Tomeu Vizoso]

On 22 September 2015 at 02:39, Rob Herring <robh@kernel.org> wrote:
> On Mon, Sep 21, 2015 at 9:02 AM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> When adding platform and AMBA devices, set the device node's device
>> member to point to it.
>>
>> This speeds lookups considerably and is safe because we only create one
>> of these devices for any given device node.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>> Changes in v5:
>> - Set the pointer to struct device also for AMBA devices
>> - Unset the pointer to struct device when the platform device is about
>>   to be unregistered
>> - Increase the reference count of the device before returning from
>>   of_find_device_by_node()
>>
>>  drivers/of/platform.c | 19 ++++++++++---------
>>  include/linux/of.h    |  1 +
>>  2 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 1001efaedcb8..408d89f1d124 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -32,11 +32,6 @@ const struct of_device_id of_default_bus_match_table[] = {
>>         {} /* Empty terminated list */
>>  };
>>
>> -static int of_dev_node_match(struct device *dev, void *data)
>> -{
>> -       return dev->of_node == data;
>> -}
>> -
>>  /**
>>   * of_find_device_by_node - Find the platform_device associated with a node
>>   * @np: Pointer to device tree node
>> @@ -45,10 +40,10 @@ static int of_dev_node_match(struct device *dev, void *data)
>>   */
>>  struct platform_device *of_find_device_by_node(struct device_node *np)
>>  {
>> -       struct device *dev;
>> -
>> -       dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
>> -       return dev ? to_platform_device(dev) : NULL;
>> +       if (np->device && np->device->bus == &platform_bus_type &&
>> +           get_device(np->device))
>
> Where do we drop the reference incremented by get_device?
>
> However, bus_find_device also took a reference I think, so you aren't
> really changing behavior.

Yeah, Intel's 0day build bot warned of the missing get_device() when
running the OF unittests.

Regards,

Tomeu

> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 07/22] regulator: core: Remove regulator_list

[Tomeu Vizoso]

On 21 September 2015 at 21:38, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Sep 21, 2015 at 04:02:47PM +0200, Tomeu Vizoso wrote:
>> As we are already registering a device with regulator_class for each
>> regulator device, regulator_list is redundant and can be replaced with
>> calls to class_find_device() and class_for_each_device().
>
> I've done an initial review of this and will push it to git so it can go
> into the various test systems.  It's on a branch by itself so if there's
> issues it's easy to back out, either from testing or from review from me
> or others.  If we get to the point of merging the whole series then I'll
> tag the code for cross merging.
>
> Thanks a lot for working on this, this is something I'd started looking
> at myself and it's a lot easier to just review code from someone else!

Thanks for your patience, will keep an eye on possible regressions.

Regards,

Tomeu

Re: [PATCH v6 20/22] driver core: Allow deferring probes until late init

[Rob Herring]

On 09/21/2015 09:03 AM, Tomeu Vizoso wrote:
> Add a field to struct device that instructs the device-driver core to
> defer the probe of this device until the late_initcall level.

This is true until the next patch with moves deferred probe processing
to device_initcall_sync. So disabling this option alone won't totally
revert to current behaviour. I guess patch 21 could be reverted if
necessary.

> 
> By letting all built-in drivers to register before starting to probe, we
> can avoid any deferred probes by probing dependencies on demand.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> 
> ---
> 
> Changes in v4:
> - Add Kconfig DELAY_DEVICE_PROBES to allow disabling delayed probing in
>   machines with initcalls that depend on devices probing at a given time.
> 
>  drivers/base/Kconfig   | 18 ++++++++++++++++++
>  drivers/base/dd.c      |  7 +++++++
>  include/linux/device.h |  2 ++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 98504ec99c7d..44b5d33b1f49 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -324,4 +324,22 @@ config CMA_ALIGNMENT
>  
>  endif
>  
> +config DELAY_DEVICE_PROBES
> +	bool "Allow delaying the probe of some devices"

I'd like to hide visibility of this behind EXPERT.

> +	default y
> +	help
> +	  Devices can be matched to a driver and probed from the moment they
> +	  are registered, but early during boot their probes are likely to be
> +	  deferred because some dependency isn't available yet because most
> +	  drivers haven't been registered yet.
> +
> +	  Enabling this option allows the device registration code to delay the
> +	  probing of a specific device until device_initcall_sync, when all
> +	  built-in drivers have been registered already.
> +
> +	  In some platforms there may be implicit assumptions about when some
> +	  devices are probed, so enabling this option could cause problems there.
> +
> +	  If unsure, say Y here.
> +
>  endmenu
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 7dc04ee81c8b..269ea76c1a4f 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -417,6 +417,13 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>  	if (!device_is_registered(dev))
>  		return -ENODEV;
>  
> +#if IS_ENABLED(CONFIG_DELAY_DEVICE_PROBES)

This can be moved to the if.

> +	if (!driver_deferred_probe_enable && dev->probe_late) {
> +		driver_deferred_probe_add(dev);
> +		return 0;
> +	}
> +#endif
> +
>  	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
>  		 drv->bus->name, __func__, dev_name(dev), drv->name);
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 8e7b806f0744..e64f4c7e243d 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -744,6 +744,7 @@ struct device_dma_parameters {
>   *
>   * @offline_disabled: If set, the device is permanently online.
>   * @offline:	Set after successful invocation of bus type's .offline().
> + * @probe_late:	If set, device will be probed in the late initcall level.
>   *
>   * At the lowest level, every device in a Linux system is represented by an
>   * instance of struct device. The device structure contains the information
> @@ -828,6 +829,7 @@ struct device {
>  
>  	bool			offline_disabled:1;
>  	bool			offline:1;
> +	bool			probe_late:1;
>  };
>  
>  static inline struct device *kobj_to_dev(struct kobject *kobj)
> 

Re: [PATCH v6 0/22] On-demand device probing

[Rob Herring]

On 09/21/2015 09:02 AM, Tomeu Vizoso wrote:
> Hello,
> 
> I have a problem with the panel on my Tegra Chromebook taking longer
> than expected to be ready during boot (Stéphane Marchesin reported what
> is basically the same issue in [0]), and have looked into ordered
> probing as a better way of solving this than moving nodes around in the
> DT or playing with initcall levels and linking order.
> 
> While reading the thread [1] that Alexander Holler started with his
> series to make probing order deterministic, it occurred to me that it
> should be possible to achieve the same by probing devices as they are
> referenced by other devices.
> 
> This basically reuses the information that is already implicit in the
> probe() implementations, saving us from refactoring existing drivers or
> adding information to DTBs.
> 
> During review of v1 of this series Linus Walleij suggested that it
> should be the device driver core to make sure that dependencies are
> ready before probing a device. I gave this idea a try [2] but Mark Brown
> pointed out to the logic duplication between the resource acquisition
> and dependency discovery code paths (though I think it's fairly minor).
> 
> To address that code duplication I experimented with Arnd's devm_probe
> [3] concept of having drivers declare their dependencies instead of
> acquiring them during probe, and while it worked [4], I don't think we
> end up winning anything when compared to just probing devices on-demand
> from resource getters.
> 
> One remaining objection is to the "sprinkling" of calls to
> of_device_probe() in the resource getters of each subsystem, but I think
> it's the right thing to do given that the storage of resources is
> currently subsystem-specific.
> 
> We could avoid the above by moving resource storage into the core, but I
> don't think there's a compelling case for that.
> 
> I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
> OMAP SoCs, and these patches were enough to eliminate all the deferred
> probes (except one in PandaBoard because omap_dma_system doesn't have a
> firmware node as of yet).
> 
> Have submitted a branch [5][6][7] with these patches on top of today's
> linux-next (20150921) to kernelci.org and I don't see any issues that
> could be caused by them.
> 
> With this series I get the kernel to output to the panel in 0.5s,
> instead of 2.8s.

I think we're pretty close other than some minor comments. I would like
to see ack's from Greg and some reviewed-bys from others. The subsystem
changes are minor and there has been plenty of chance to comment, so I
don't think acks from all subsystems are needed.

Your branch is based on -next. Is there any dependence on something in
-next? I want to get this into -next soon, but need a branch not based
on -next. Please send me a pull request with the collected acks and
minor comments I have addressed.

Rob

> 
> Regards,
> 
> Tomeu
> 
> [0] http://lists.freedesktop.org/archives/dri-devel/2014-August/066527.html
> 
> [1] https://lkml.org/lkml/2014/5/12/452
> 
> [2] https://lkml.org/lkml/2015/6/17/305
> 
> [3] http://article.gmane.org/gmane.linux.ports.arm.kernel/277689
> 
> [4] https://lkml.org/lkml/2015/7/21/441a
> 
> [5] https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=on-demand-probes-v8
> 
> [6] http://kernelci.org/boot/all/job/collabora/kernel/v4.3-rc2-2587-gf92b0ab33d14/
> 
> [7] http://kernelci.org/boot/all/job/next/kernel/next-20150921
> 
> Changes in v6:
> - Drop bus_type.pre_probe and read the periphid in match() instead as
>   suggested by Alan Stern.
> - Merge changes to the regulator subsystem's locking so no references
>   are leaked between commits.
> 
> Changes in v5:
> - Set the pointer to struct device also for AMBA devices
> - Unset the pointer to struct device when the platform device is about
>   to be unregistered
> - Increase the reference count of the device before returning from
>   of_find_device_by_node()
> - Move the assignment to device_node->device for AMBA devices to another
>   commit.
> - Hold a reference to the struct device while it's in use in
>   of_device_probe().
> - Use regulator_class' klist of devices instead of regulator_list to
>   store and lookup regulator devices.
> 
> Changes in v4:
> - Added bus.pre_probe callback so the probes of Primecell devices can be
>   deferred if their device IDs cannot be yet read because of the clock
>   driver not having probed when they are registered. Maybe this goes
>   overboard and the matching information should be in the DT if there is
>   one.
> - Rename of_platform_probe to of_device_probe
> - Use device_node.device instead of device_node.platform_dev
> - Add Kconfig DELAY_DEVICE_PROBES to allow disabling delayed probing in
>   machines with initcalls that depend on devices probing at a given time.
> - Start processing deferred probes in device_initcall_sync
> - Also defer probes of AMBA devices registered from the DT as they can
>   also request resources.
> 
> Changes in v3:
> - Set and use device_node.platform_dev instead of reversing the logic to
>   find the platform device that encloses a device node.
> - Drop the fwnode API to probe firmware nodes and add OF-only API for
>   now. I think this same scheme could be used for machines with ACPI,
>   but I haven't been able to find one that had to defer its probes because
>   of the device probe order.
> 
> Tomeu Vizoso (22):
>   driver core: handle -EPROBE_DEFER from bus_type.match()
>   ARM: amba: Move reading of periphid to amba_match()
>   of/platform: Point to struct device from device node
>   of: add function to allow probing a device from a OF node
>   gpio: Probe GPIO drivers on demand
>   gpio: Probe pinctrl devices on demand
>   regulator: core: Remove regulator_list
>   regulator: core: Probe regulators on demand
>   drm: Probe panels on demand
>   drm/tegra: Probe dpaux devices on demand
>   i2c: core: Probe i2c adapters and devices on demand
>   pwm: Probe PWM chip devices on demand
>   backlight: Probe backlight devices on demand
>   usb: phy: Probe phy devices on demand
>   clk: Probe clk providers on demand
>   pinctrl: Probe pinctrl devices on demand
>   phy: core: Probe phy providers on demand
>   dma: of: Probe DMA controllers on demand
>   power-supply: Probe power supplies on demand
>   driver core: Allow deferring probes until late init
>   driver core: Start processing deferred probes earlier
>   of/platform: Defer probes of registered devices
> 
>  drivers/amba/bus.c                  |  88 ++++++------
>  drivers/base/Kconfig                |  18 +++
>  drivers/base/dd.c                   |  35 ++++-
>  drivers/clk/clk.c                   |   3 +
>  drivers/dma/of-dma.c                |   3 +
>  drivers/gpio/gpiolib-of.c           |   5 +
>  drivers/gpu/drm/drm_panel.c         |   3 +
>  drivers/gpu/drm/tegra/dpaux.c       |   3 +
>  drivers/i2c/i2c-core.c              |   4 +
>  drivers/of/device.c                 |  61 +++++++++
>  drivers/of/platform.c               |  30 +++--
>  drivers/phy/phy-core.c              |   3 +
>  drivers/pinctrl/devicetree.c        |   3 +
>  drivers/power/power_supply_core.c   |   3 +
>  drivers/pwm/core.c                  |   3 +
>  drivers/regulator/core.c            | 257 +++++++++++++++++++++++-------------
>  drivers/usb/phy/phy.c               |   3 +
>  drivers/video/backlight/backlight.c |   3 +
>  include/linux/device.h              |   4 +-
>  include/linux/of.h                  |   1 +
>  include/linux/of_device.h           |   3 +
>  21 files changed, 386 insertions(+), 150 deletions(-)
> 

Re: [PATCH v6 0/22] On-demand device probing

[Greg Kroah-Hartman]

On Sat, Sep 26, 2015 at 01:17:04PM -0500, Rob Herring wrote:
> On 09/21/2015 09:02 AM, Tomeu Vizoso wrote:
> > Hello,
> > 
> > I have a problem with the panel on my Tegra Chromebook taking longer
> > than expected to be ready during boot (Stéphane Marchesin reported what
> > is basically the same issue in [0]), and have looked into ordered
> > probing as a better way of solving this than moving nodes around in the
> > DT or playing with initcall levels and linking order.
> > 
> > While reading the thread [1] that Alexander Holler started with his
> > series to make probing order deterministic, it occurred to me that it
> > should be possible to achieve the same by probing devices as they are
> > referenced by other devices.
> > 
> > This basically reuses the information that is already implicit in the
> > probe() implementations, saving us from refactoring existing drivers or
> > adding information to DTBs.
> > 
> > During review of v1 of this series Linus Walleij suggested that it
> > should be the device driver core to make sure that dependencies are
> > ready before probing a device. I gave this idea a try [2] but Mark Brown
> > pointed out to the logic duplication between the resource acquisition
> > and dependency discovery code paths (though I think it's fairly minor).
> > 
> > To address that code duplication I experimented with Arnd's devm_probe
> > [3] concept of having drivers declare their dependencies instead of
> > acquiring them during probe, and while it worked [4], I don't think we
> > end up winning anything when compared to just probing devices on-demand
> > from resource getters.
> > 
> > One remaining objection is to the "sprinkling" of calls to
> > of_device_probe() in the resource getters of each subsystem, but I think
> > it's the right thing to do given that the storage of resources is
> > currently subsystem-specific.
> > 
> > We could avoid the above by moving resource storage into the core, but I
> > don't think there's a compelling case for that.
> > 
> > I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
> > OMAP SoCs, and these patches were enough to eliminate all the deferred
> > probes (except one in PandaBoard because omap_dma_system doesn't have a
> > firmware node as of yet).
> > 
> > Have submitted a branch [5][6][7] with these patches on top of today's
> > linux-next (20150921) to kernelci.org and I don't see any issues that
> > could be caused by them.
> > 
> > With this series I get the kernel to output to the panel in 0.5s,
> > instead of 2.8s.
> 
> I think we're pretty close other than some minor comments. I would like
> to see ack's from Greg and some reviewed-bys from others. The subsystem
> changes are minor and there has been plenty of chance to comment, so I
> don't think acks from all subsystems are needed.
> 
> Your branch is based on -next. Is there any dependence on something in
> -next? I want to get this into -next soon, but need a branch not based
> on -next. Please send me a pull request with the collected acks and
> minor comments I have addressed.

Let me review this on Monday and I'll let you know...

thanks,

greg k-h

Re: [PATCH v6 20/22] driver core: Allow deferring probes until late init

[Tomeu Vizoso]

On 26 September 2015 at 20:15, Rob Herring <robh@kernel.org> wrote:
> On 09/21/2015 09:03 AM, Tomeu Vizoso wrote:
>> Add a field to struct device that instructs the device-driver core to
>> defer the probe of this device until the late_initcall level.
>
> This is true until the next patch with moves deferred probe processing
> to device_initcall_sync. So disabling this option alone won't totally
> revert to current behaviour. I guess patch 21 could be reverted if
> necessary.

Actually, the goal with that commit was to prevent potential problems
due to the increased pressure on late_initcall, as suggested by
Grygorii Strashko, but I haven't found yet any evidence of it being
needed, and in my testing the series boot all boards in kernelci with
or without this commit. So I would just not commit it for now and only
consider applying it later if someone reports a problem.

>> By letting all built-in drivers to register before starting to probe, we
>> can avoid any deferred probes by probing dependencies on demand.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>
>> ---
>>
>> Changes in v4:
>> - Add Kconfig DELAY_DEVICE_PROBES to allow disabling delayed probing in
>>   machines with initcalls that depend on devices probing at a given time.
>>
>>  drivers/base/Kconfig   | 18 ++++++++++++++++++
>>  drivers/base/dd.c      |  7 +++++++
>>  include/linux/device.h |  2 ++
>>  3 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> index 98504ec99c7d..44b5d33b1f49 100644
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -324,4 +324,22 @@ config CMA_ALIGNMENT
>>
>>  endif
>>
>> +config DELAY_DEVICE_PROBES
>> +     bool "Allow delaying the probe of some devices"
>
> I'd like to hide visibility of this behind EXPERT.

Done.

>> +     default y
>> +     help
>> +       Devices can be matched to a driver and probed from the moment they
>> +       are registered, but early during boot their probes are likely to be
>> +       deferred because some dependency isn't available yet because most
>> +       drivers haven't been registered yet.
>> +
>> +       Enabling this option allows the device registration code to delay the
>> +       probing of a specific device until device_initcall_sync, when all
>> +       built-in drivers have been registered already.
>> +
>> +       In some platforms there may be implicit assumptions about when some
>> +       devices are probed, so enabling this option could cause problems there.
>> +
>> +       If unsure, say Y here.
>> +
>>  endmenu
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 7dc04ee81c8b..269ea76c1a4f 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -417,6 +417,13 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>>       if (!device_is_registered(dev))
>>               return -ENODEV;
>>
>> +#if IS_ENABLED(CONFIG_DELAY_DEVICE_PROBES)
>
> This can be moved to the if.

Done.

Thanks,

Tomeu

>> +     if (!driver_deferred_probe_enable && dev->probe_late) {
>> +             driver_deferred_probe_add(dev);
>> +             return 0;
>> +     }
>> +#endif
>> +
>>       pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
>>                drv->bus->name, __func__, dev_name(dev), drv->name);
>>
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 8e7b806f0744..e64f4c7e243d 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -744,6 +744,7 @@ struct device_dma_parameters {
>>   *
>>   * @offline_disabled: If set, the device is permanently online.
>>   * @offline: Set after successful invocation of bus type's .offline().
>> + * @probe_late:      If set, device will be probed in the late initcall level.
>>   *
>>   * At the lowest level, every device in a Linux system is represented by an
>>   * instance of struct device. The device structure contains the information
>> @@ -828,6 +829,7 @@ struct device {
>>
>>       bool                    offline_disabled:1;
>>       bool                    offline:1;
>> +     bool                    probe_late:1;
>>  };
>>
>>  static inline struct device *kobj_to_dev(struct kobject *kobj)
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 0/22] On-demand device probing

[Tomeu Vizoso]

On 26 September 2015 at 20:17, Rob Herring <robh@kernel.org> wrote:
> On 09/21/2015 09:02 AM, Tomeu Vizoso wrote:
>> Hello,
>>
>> I have a problem with the panel on my Tegra Chromebook taking longer
>> than expected to be ready during boot (Stéphane Marchesin reported what
>> is basically the same issue in [0]), and have looked into ordered
>> probing as a better way of solving this than moving nodes around in the
>> DT or playing with initcall levels and linking order.
>>
>> While reading the thread [1] that Alexander Holler started with his
>> series to make probing order deterministic, it occurred to me that it
>> should be possible to achieve the same by probing devices as they are
>> referenced by other devices.
>>
>> This basically reuses the information that is already implicit in the
>> probe() implementations, saving us from refactoring existing drivers or
>> adding information to DTBs.
>>
>> During review of v1 of this series Linus Walleij suggested that it
>> should be the device driver core to make sure that dependencies are
>> ready before probing a device. I gave this idea a try [2] but Mark Brown
>> pointed out to the logic duplication between the resource acquisition
>> and dependency discovery code paths (though I think it's fairly minor).
>>
>> To address that code duplication I experimented with Arnd's devm_probe
>> [3] concept of having drivers declare their dependencies instead of
>> acquiring them during probe, and while it worked [4], I don't think we
>> end up winning anything when compared to just probing devices on-demand
>> from resource getters.
>>
>> One remaining objection is to the "sprinkling" of calls to
>> of_device_probe() in the resource getters of each subsystem, but I think
>> it's the right thing to do given that the storage of resources is
>> currently subsystem-specific.
>>
>> We could avoid the above by moving resource storage into the core, but I
>> don't think there's a compelling case for that.
>>
>> I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
>> OMAP SoCs, and these patches were enough to eliminate all the deferred
>> probes (except one in PandaBoard because omap_dma_system doesn't have a
>> firmware node as of yet).
>>
>> Have submitted a branch [5][6][7] with these patches on top of today's
>> linux-next (20150921) to kernelci.org and I don't see any issues that
>> could be caused by them.
>>
>> With this series I get the kernel to output to the panel in 0.5s,
>> instead of 2.8s.
>
> I think we're pretty close other than some minor comments. I would like
> to see ack's from Greg and some reviewed-bys from others. The subsystem
> changes are minor and there has been plenty of chance to comment, so I
> don't think acks from all subsystems are needed.
>
> Your branch is based on -next. Is there any dependence on something in
> -next? I want to get this into -next soon, but need a branch not based
> on -next. Please send me a pull request with the collected acks and
> minor comments I have addressed.

Great, I'm going to send one more revision rebased on top of v4.3-rc1,
without 21 and with the minor changes you suggested, and once Greg is
happy I can send the pull request.

Thanks,

Tomeu

> Rob
>
>>
>> Regards,
>>
>> Tomeu
>>
>> [0] http://lists.freedesktop.org/archives/dri-devel/2014-August/066527.html
>>
>> [1] https://lkml.org/lkml/2014/5/12/452
>>
>> [2] https://lkml.org/lkml/2015/6/17/305
>>
>> [3] http://article.gmane.org/gmane.linux.ports.arm.kernel/277689
>>
>> [4] https://lkml.org/lkml/2015/7/21/441a
>>
>> [5] https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=on-demand-probes-v8
>>
>> [6] http://kernelci.org/boot/all/job/collabora/kernel/v4.3-rc2-2587-gf92b0ab33d14/
>>
>> [7] http://kernelci.org/boot/all/job/next/kernel/next-20150921
>>
>> Changes in v6:
>> - Drop bus_type.pre_probe and read the periphid in match() instead as
>>   suggested by Alan Stern.
>> - Merge changes to the regulator subsystem's locking so no references
>>   are leaked between commits.
>>
>> Changes in v5:
>> - Set the pointer to struct device also for AMBA devices
>> - Unset the pointer to struct device when the platform device is about
>>   to be unregistered
>> - Increase the reference count of the device before returning from
>>   of_find_device_by_node()
>> - Move the assignment to device_node->device for AMBA devices to another
>>   commit.
>> - Hold a reference to the struct device while it's in use in
>>   of_device_probe().
>> - Use regulator_class' klist of devices instead of regulator_list to
>>   store and lookup regulator devices.
>>
>> Changes in v4:
>> - Added bus.pre_probe callback so the probes of Primecell devices can be
>>   deferred if their device IDs cannot be yet read because of the clock
>>   driver not having probed when they are registered. Maybe this goes
>>   overboard and the matching information should be in the DT if there is
>>   one.
>> - Rename of_platform_probe to of_device_probe
>> - Use device_node.device instead of device_node.platform_dev
>> - Add Kconfig DELAY_DEVICE_PROBES to allow disabling delayed probing in
>>   machines with initcalls that depend on devices probing at a given time.
>> - Start processing deferred probes in device_initcall_sync
>> - Also defer probes of AMBA devices registered from the DT as they can
>>   also request resources.
>>
>> Changes in v3:
>> - Set and use device_node.platform_dev instead of reversing the logic to
>>   find the platform device that encloses a device node.
>> - Drop the fwnode API to probe firmware nodes and add OF-only API for
>>   now. I think this same scheme could be used for machines with ACPI,
>>   but I haven't been able to find one that had to defer its probes because
>>   of the device probe order.
>>
>> Tomeu Vizoso (22):
>>   driver core: handle -EPROBE_DEFER from bus_type.match()
>>   ARM: amba: Move reading of periphid to amba_match()
>>   of/platform: Point to struct device from device node
>>   of: add function to allow probing a device from a OF node
>>   gpio: Probe GPIO drivers on demand
>>   gpio: Probe pinctrl devices on demand
>>   regulator: core: Remove regulator_list
>>   regulator: core: Probe regulators on demand
>>   drm: Probe panels on demand
>>   drm/tegra: Probe dpaux devices on demand
>>   i2c: core: Probe i2c adapters and devices on demand
>>   pwm: Probe PWM chip devices on demand
>>   backlight: Probe backlight devices on demand
>>   usb: phy: Probe phy devices on demand
>>   clk: Probe clk providers on demand
>>   pinctrl: Probe pinctrl devices on demand
>>   phy: core: Probe phy providers on demand
>>   dma: of: Probe DMA controllers on demand
>>   power-supply: Probe power supplies on demand
>>   driver core: Allow deferring probes until late init
>>   driver core: Start processing deferred probes earlier
>>   of/platform: Defer probes of registered devices
>>
>>  drivers/amba/bus.c                  |  88 ++++++------
>>  drivers/base/Kconfig                |  18 +++
>>  drivers/base/dd.c                   |  35 ++++-
>>  drivers/clk/clk.c                   |   3 +
>>  drivers/dma/of-dma.c                |   3 +
>>  drivers/gpio/gpiolib-of.c           |   5 +
>>  drivers/gpu/drm/drm_panel.c         |   3 +
>>  drivers/gpu/drm/tegra/dpaux.c       |   3 +
>>  drivers/i2c/i2c-core.c              |   4 +
>>  drivers/of/device.c                 |  61 +++++++++
>>  drivers/of/platform.c               |  30 +++--
>>  drivers/phy/phy-core.c              |   3 +
>>  drivers/pinctrl/devicetree.c        |   3 +
>>  drivers/power/power_supply_core.c   |   3 +
>>  drivers/pwm/core.c                  |   3 +
>>  drivers/regulator/core.c            | 257 +++++++++++++++++++++++-------------
>>  drivers/usb/phy/phy.c               |   3 +
>>  drivers/video/backlight/backlight.c |   3 +
>>  include/linux/device.h              |   4 +-
>>  include/linux/of.h                  |   1 +
>>  include/linux/of_device.h           |   3 +
>>  21 files changed, 386 insertions(+), 150 deletions(-)
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 20/22] driver core: Allow deferring probes until late init

[Rob Herring]

On Tue, Sep 29, 2015 at 3:05 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> On 26 September 2015 at 20:15, Rob Herring <robh@kernel.org> wrote:
>> On 09/21/2015 09:03 AM, Tomeu Vizoso wrote:
>>> Add a field to struct device that instructs the device-driver core to
>>> defer the probe of this device until the late_initcall level.
>>
>> This is true until the next patch with moves deferred probe processing
>> to device_initcall_sync. So disabling this option alone won't totally
>> revert to current behaviour. I guess patch 21 could be reverted if
>> necessary.
>
> Actually, the goal with that commit was to prevent potential problems
> due to the increased pressure on late_initcall, as suggested by
> Grygorii Strashko, but I haven't found yet any evidence of it being
> needed, and in my testing the series boot all boards in kernelci with
> or without this commit. So I would just not commit it for now and only
> consider applying it later if someone reports a problem.

I had similar concerns with assumptions about ordering WRT
late_initcall. I would keep this for now.

Rob

Re: [PATCH v6 0/22] On-demand device probing

[Tomeu Vizoso]

On 26 September 2015 at 21:22, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Sep 26, 2015 at 01:17:04PM -0500, Rob Herring wrote:
>> On 09/21/2015 09:02 AM, Tomeu Vizoso wrote:
>> > Hello,
>> >
>> > I have a problem with the panel on my Tegra Chromebook taking longer
>> > than expected to be ready during boot (Stéphane Marchesin reported what
>> > is basically the same issue in [0]), and have looked into ordered
>> > probing as a better way of solving this than moving nodes around in the
>> > DT or playing with initcall levels and linking order.
>> >
>> > While reading the thread [1] that Alexander Holler started with his
>> > series to make probing order deterministic, it occurred to me that it
>> > should be possible to achieve the same by probing devices as they are
>> > referenced by other devices.
>> >
>> > This basically reuses the information that is already implicit in the
>> > probe() implementations, saving us from refactoring existing drivers or
>> > adding information to DTBs.
>> >
>> > During review of v1 of this series Linus Walleij suggested that it
>> > should be the device driver core to make sure that dependencies are
>> > ready before probing a device. I gave this idea a try [2] but Mark Brown
>> > pointed out to the logic duplication between the resource acquisition
>> > and dependency discovery code paths (though I think it's fairly minor).
>> >
>> > To address that code duplication I experimented with Arnd's devm_probe
>> > [3] concept of having drivers declare their dependencies instead of
>> > acquiring them during probe, and while it worked [4], I don't think we
>> > end up winning anything when compared to just probing devices on-demand
>> > from resource getters.
>> >
>> > One remaining objection is to the "sprinkling" of calls to
>> > of_device_probe() in the resource getters of each subsystem, but I think
>> > it's the right thing to do given that the storage of resources is
>> > currently subsystem-specific.
>> >
>> > We could avoid the above by moving resource storage into the core, but I
>> > don't think there's a compelling case for that.
>> >
>> > I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
>> > OMAP SoCs, and these patches were enough to eliminate all the deferred
>> > probes (except one in PandaBoard because omap_dma_system doesn't have a
>> > firmware node as of yet).
>> >
>> > Have submitted a branch [5][6][7] with these patches on top of today's
>> > linux-next (20150921) to kernelci.org and I don't see any issues that
>> > could be caused by them.
>> >
>> > With this series I get the kernel to output to the panel in 0.5s,
>> > instead of 2.8s.
>>
>> I think we're pretty close other than some minor comments. I would like
>> to see ack's from Greg and some reviewed-bys from others. The subsystem
>> changes are minor and there has been plenty of chance to comment, so I
>> don't think acks from all subsystems are needed.
>>
>> Your branch is based on -next. Is there any dependence on something in
>> -next? I want to get this into -next soon, but need a branch not based
>> on -next. Please send me a pull request with the collected acks and
>> minor comments I have addressed.
>
> Let me review this on Monday and I'll let you know...

Hi Greg, hope you don't mind that I ping you regarding this, just in
case it fell through some crack.

Regards,

Tomeu

> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 0/22] On-demand device probing

[Greg Kroah-Hartman]

On Wed, Sep 30, 2015 at 12:09:27PM +0200, Tomeu Vizoso wrote:
> On 26 September 2015 at 21:22, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Sep 26, 2015 at 01:17:04PM -0500, Rob Herring wrote:
> >> On 09/21/2015 09:02 AM, Tomeu Vizoso wrote:
> >> > Hello,
> >> >
> >> > I have a problem with the panel on my Tegra Chromebook taking longer
> >> > than expected to be ready during boot (Stéphane Marchesin reported what
> >> > is basically the same issue in [0]), and have looked into ordered
> >> > probing as a better way of solving this than moving nodes around in the
> >> > DT or playing with initcall levels and linking order.
> >> >
> >> > While reading the thread [1] that Alexander Holler started with his
> >> > series to make probing order deterministic, it occurred to me that it
> >> > should be possible to achieve the same by probing devices as they are
> >> > referenced by other devices.
> >> >
> >> > This basically reuses the information that is already implicit in the
> >> > probe() implementations, saving us from refactoring existing drivers or
> >> > adding information to DTBs.
> >> >
> >> > During review of v1 of this series Linus Walleij suggested that it
> >> > should be the device driver core to make sure that dependencies are
> >> > ready before probing a device. I gave this idea a try [2] but Mark Brown
> >> > pointed out to the logic duplication between the resource acquisition
> >> > and dependency discovery code paths (though I think it's fairly minor).
> >> >
> >> > To address that code duplication I experimented with Arnd's devm_probe
> >> > [3] concept of having drivers declare their dependencies instead of
> >> > acquiring them during probe, and while it worked [4], I don't think we
> >> > end up winning anything when compared to just probing devices on-demand
> >> > from resource getters.
> >> >
> >> > One remaining objection is to the "sprinkling" of calls to
> >> > of_device_probe() in the resource getters of each subsystem, but I think
> >> > it's the right thing to do given that the storage of resources is
> >> > currently subsystem-specific.
> >> >
> >> > We could avoid the above by moving resource storage into the core, but I
> >> > don't think there's a compelling case for that.
> >> >
> >> > I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
> >> > OMAP SoCs, and these patches were enough to eliminate all the deferred
> >> > probes (except one in PandaBoard because omap_dma_system doesn't have a
> >> > firmware node as of yet).
> >> >
> >> > Have submitted a branch [5][6][7] with these patches on top of today's
> >> > linux-next (20150921) to kernelci.org and I don't see any issues that
> >> > could be caused by them.
> >> >
> >> > With this series I get the kernel to output to the panel in 0.5s,
> >> > instead of 2.8s.
> >>
> >> I think we're pretty close other than some minor comments. I would like
> >> to see ack's from Greg and some reviewed-bys from others. The subsystem
> >> changes are minor and there has been plenty of chance to comment, so I
> >> don't think acks from all subsystems are needed.
> >>
> >> Your branch is based on -next. Is there any dependence on something in
> >> -next? I want to get this into -next soon, but need a branch not based
> >> on -next. Please send me a pull request with the collected acks and
> >> minor comments I have addressed.
> >
> > Let me review this on Monday and I'll let you know...
> 
> Hi Greg, hope you don't mind that I ping you regarding this, just in
> case it fell through some crack.

It's on my todo list, sorry, am at a conference for the next few days,
didn't get to it on Monday, hopefully soon...

greg "my todo list is too long" k-h

Re: [PATCH v6 21/22] driver core: Start processing deferred probes earlier

[Frank Rowand]

On 9/21/2015 7:03 AM, Tomeu Vizoso wrote:
> Some initcalls in the late level assume that some devices will have
> already probed without explicitly checking for that.
> 
> After the recent move to defer most device probes when they are
> registered, pressure increased in the late initcall level.
> 
> By starting the processing of the deferred queue in device_initcall_sync
> we increase the chances that the initcalls mentioned before will find
> the devices they depend on to have already probed.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
> Changes in v4:
> - Start processing deferred probes in device_initcall_sync
> 
>  drivers/base/dd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 269ea76c1a4f..f0ef9233fcd6 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -176,7 +176,7 @@ static void driver_deferred_probe_trigger(void)
>   *
>   * We don't want to get in the way when the bulk of drivers are getting probed.
>   * Instead, this initcall makes sure that deferred probing is delayed until
> - * late_initcall time.
> + * device_initcall_sync time.

This is a misuse of the *_sync initcall level.

If a deferred probe created a thread and expected a wait at the
following *_sync level then the wait would not occur because
there is no subsequent *_sync level.  This is not a problem today
because (as far as I know) there are no threads in the probe
functions.  But there has been interest in adding parallel
probing and that could expose this problem.

The purpose of the *_sync initcall levels is to allow the corresponding
initcall level to use multiple threads of execution instead of a single
thread.  The *_sync level provides a location for a wait for all of the
threads at the corresponding init level to complete.  This is better
explained in the git log:

commit 735a7ffb739b6efeaeb1e720306ba308eaaeb20e
Author: Andrew Morton <akpm@osdl.org>
Date:   Fri Oct 27 11:42:37 2006 -0700

    [PATCH] drivers: wait for threaded probes between initcall levels

    The multithreaded-probing code has a problem: after one initcall level (eg,
    core_initcall) has been processed, we will then start processing the next
    level (postcore_initcall) while the kernel threads which are handling
    core_initcall are still executing.  This breaks the guarantees which the
    layered initcalls previously gave us.

    IOW, we want to be multithreaded _within_ an initcall level, but not between
    different levels.

    Fix that up by causing the probing code to wait for all outstanding probes at
    one level to complete before we start processing the next level.

>   */
>  static int deferred_probe_initcall(void)
>  {
> @@ -190,7 +190,7 @@ static int deferred_probe_initcall(void)
>  	flush_workqueue(deferred_wq);
>  	return 0;
>  }
> -late_initcall(deferred_probe_initcall);
> +device_initcall_sync(deferred_probe_initcall);
>  
>  static void driver_bound(struct device *dev)
>  {
> 

Re: [PATCH v6 21/22] driver core: Start processing deferred probes earlier

[Rob Herring]

On Mon, Oct 5, 2015 at 6:52 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 9/21/2015 7:03 AM, Tomeu Vizoso wrote:
>> Some initcalls in the late level assume that some devices will have
>> already probed without explicitly checking for that.
>>
>> After the recent move to defer most device probes when they are
>> registered, pressure increased in the late initcall level.
>>
>> By starting the processing of the deferred queue in device_initcall_sync
>> we increase the chances that the initcalls mentioned before will find
>> the devices they depend on to have already probed.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>> Changes in v4:
>> - Start processing deferred probes in device_initcall_sync
>>
>>  drivers/base/dd.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 269ea76c1a4f..f0ef9233fcd6 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -176,7 +176,7 @@ static void driver_deferred_probe_trigger(void)
>>   *
>>   * We don't want to get in the way when the bulk of drivers are getting probed.
>>   * Instead, this initcall makes sure that deferred probing is delayed until
>> - * late_initcall time.
>> + * device_initcall_sync time.
>
> This is a misuse of the *_sync initcall level.
>
> If a deferred probe created a thread and expected a wait at the
> following *_sync level then the wait would not occur because
> there is no subsequent *_sync level.  This is not a problem today
> because (as far as I know) there are no threads in the probe
> functions.  But there has been interest in adding parallel
> probing and that could expose this problem.
>
> The purpose of the *_sync initcall levels is to allow the corresponding
> initcall level to use multiple threads of execution instead of a single
> thread.  The *_sync level provides a location for a wait for all of the
> threads at the corresponding init level to complete.  This is better
> explained in the git log:

The things I was worried about like clk and regulator disabling are
actually late_initcall_sync, so maybe late_initcall is fine here after
all.

However, looking at other *_initcall_sync users, I'm not so sure they
are doing the same abuse.

Rob

>
> commit 735a7ffb739b6efeaeb1e720306ba308eaaeb20e
> Author: Andrew Morton <akpm@osdl.org>
> Date:   Fri Oct 27 11:42:37 2006 -0700
>
>     [PATCH] drivers: wait for threaded probes between initcall levels
>
>     The multithreaded-probing code has a problem: after one initcall level (eg,
>     core_initcall) has been processed, we will then start processing the next
>     level (postcore_initcall) while the kernel threads which are handling
>     core_initcall are still executing.  This breaks the guarantees which the
>     layered initcalls previously gave us.
>
>     IOW, we want to be multithreaded _within_ an initcall level, but not between
>     different levels.
>
>     Fix that up by causing the probing code to wait for all outstanding probes at
>     one level to complete before we start processing the next level.
>
>>   */
>>  static int deferred_probe_initcall(void)
>>  {
>> @@ -190,7 +190,7 @@ static int deferred_probe_initcall(void)
>>       flush_workqueue(deferred_wq);
>>       return 0;
>>  }
>> -late_initcall(deferred_probe_initcall);
>> +device_initcall_sync(deferred_probe_initcall);
>>
>>  static void driver_bound(struct device *dev)
>>  {
>>
>

Re: [PATCH v6 21/22] driver core: Start processing deferred probes earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

On Mon, Oct 05, 2015 at 09:49:38PM -0500, Rob Herring wrote:
> On Mon, Oct 5, 2015 at 6:52 PM, Frank Rowand <frowand.list@gmail.com> wrote:

> > The purpose of the *_sync initcall levels is to allow the corresponding
> > initcall level to use multiple threads of execution instead of a single
> > thread.  The *_sync level provides a location for a wait for all of the
> > threads at the corresponding init level to complete.  This is better
> > explained in the git log:

> The things I was worried about like clk and regulator disabling are
> actually late_initcall_sync, so maybe late_initcall is fine here after
> all.

> However, looking at other *_initcall_sync users, I'm not so sure they
> are doing the same abuse.

They're trying to run at the point where we've completed deferred
probe processing - in other words, at the sync point.  What they really
want is an additional callback at the point where the syncs have
actually happened.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v6 0/22] On-demand device probing

[Tomeu Vizoso]

On 26 September 2015 at 20:17, Rob Herring <robh@kernel.org> wrote:
> On 09/21/2015 09:02 AM, Tomeu Vizoso wrote:
>> Hello,
>>
>> I have a problem with the panel on my Tegra Chromebook taking longer
>> than expected to be ready during boot (Stéphane Marchesin reported what
>> is basically the same issue in [0]), and have looked into ordered
>> probing as a better way of solving this than moving nodes around in the
>> DT or playing with initcall levels and linking order.
>>
>> While reading the thread [1] that Alexander Holler started with his
>> series to make probing order deterministic, it occurred to me that it
>> should be possible to achieve the same by probing devices as they are
>> referenced by other devices.
>>
>> This basically reuses the information that is already implicit in the
>> probe() implementations, saving us from refactoring existing drivers or
>> adding information to DTBs.
>>
>> During review of v1 of this series Linus Walleij suggested that it
>> should be the device driver core to make sure that dependencies are
>> ready before probing a device. I gave this idea a try [2] but Mark Brown
>> pointed out to the logic duplication between the resource acquisition
>> and dependency discovery code paths (though I think it's fairly minor).
>>
>> To address that code duplication I experimented with Arnd's devm_probe
>> [3] concept of having drivers declare their dependencies instead of
>> acquiring them during probe, and while it worked [4], I don't think we
>> end up winning anything when compared to just probing devices on-demand
>> from resource getters.
>>
>> One remaining objection is to the "sprinkling" of calls to
>> of_device_probe() in the resource getters of each subsystem, but I think
>> it's the right thing to do given that the storage of resources is
>> currently subsystem-specific.
>>
>> We could avoid the above by moving resource storage into the core, but I
>> don't think there's a compelling case for that.
>>
>> I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
>> OMAP SoCs, and these patches were enough to eliminate all the deferred
>> probes (except one in PandaBoard because omap_dma_system doesn't have a
>> firmware node as of yet).
>>
>> Have submitted a branch [5][6][7] with these patches on top of today's
>> linux-next (20150921) to kernelci.org and I don't see any issues that
>> could be caused by them.
>>
>> With this series I get the kernel to output to the panel in 0.5s,
>> instead of 2.8s.
>
> I think we're pretty close other than some minor comments. I would like
> to see ack's from Greg and some reviewed-bys from others. The subsystem
> changes are minor and there has been plenty of chance to comment, so I
> don't think acks from all subsystems are needed.

Hi Rob,

I'm not sure we are going to get much more feedback by just waiting or
resending what has been sent so many times.

Did you have in mind specific people you wanted to see reviewed-bys from?

Guess Greg will get to this sometime soon.

Regards,

Tomeu

> Your branch is based on -next. Is there any dependence on something in
> -next? I want to get this into -next soon, but need a branch not based
> on -next. Please send me a pull request with the collected acks and
> minor comments I have addressed.
>
> Rob
>
>>
>> Regards,
>>
>> Tomeu
>>
>> [0] http://lists.freedesktop.org/archives/dri-devel/2014-August/066527.html
>>
>> [1] https://lkml.org/lkml/2014/5/12/452
>>
>> [2] https://lkml.org/lkml/2015/6/17/305
>>
>> [3] http://article.gmane.org/gmane.linux.ports.arm.kernel/277689
>>
>> [4] https://lkml.org/lkml/2015/7/21/441a
>>
>> [5] https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=on-demand-probes-v8
>>
>> [6] http://kernelci.org/boot/all/job/collabora/kernel/v4.3-rc2-2587-gf92b0ab33d14/
>>
>> [7] http://kernelci.org/boot/all/job/next/kernel/next-20150921
>>
>> Changes in v6:
>> - Drop bus_type.pre_probe and read the periphid in match() instead as
>>   suggested by Alan Stern.
>> - Merge changes to the regulator subsystem's locking so no references
>>   are leaked between commits.
>>
>> Changes in v5:
>> - Set the pointer to struct device also for AMBA devices
>> - Unset the pointer to struct device when the platform device is about
>>   to be unregistered
>> - Increase the reference count of the device before returning from
>>   of_find_device_by_node()
>> - Move the assignment to device_node->device for AMBA devices to another
>>   commit.
>> - Hold a reference to the struct device while it's in use in
>>   of_device_probe().
>> - Use regulator_class' klist of devices instead of regulator_list to
>>   store and lookup regulator devices.
>>
>> Changes in v4:
>> - Added bus.pre_probe callback so the probes of Primecell devices can be
>>   deferred if their device IDs cannot be yet read because of the clock
>>   driver not having probed when they are registered. Maybe this goes
>>   overboard and the matching information should be in the DT if there is
>>   one.
>> - Rename of_platform_probe to of_device_probe
>> - Use device_node.device instead of device_node.platform_dev
>> - Add Kconfig DELAY_DEVICE_PROBES to allow disabling delayed probing in
>>   machines with initcalls that depend on devices probing at a given time.
>> - Start processing deferred probes in device_initcall_sync
>> - Also defer probes of AMBA devices registered from the DT as they can
>>   also request resources.
>>
>> Changes in v3:
>> - Set and use device_node.platform_dev instead of reversing the logic to
>>   find the platform device that encloses a device node.
>> - Drop the fwnode API to probe firmware nodes and add OF-only API for
>>   now. I think this same scheme could be used for machines with ACPI,
>>   but I haven't been able to find one that had to defer its probes because
>>   of the device probe order.
>>
>> Tomeu Vizoso (22):
>>   driver core: handle -EPROBE_DEFER from bus_type.match()
>>   ARM: amba: Move reading of periphid to amba_match()
>>   of/platform: Point to struct device from device node
>>   of: add function to allow probing a device from a OF node
>>   gpio: Probe GPIO drivers on demand
>>   gpio: Probe pinctrl devices on demand
>>   regulator: core: Remove regulator_list
>>   regulator: core: Probe regulators on demand
>>   drm: Probe panels on demand
>>   drm/tegra: Probe dpaux devices on demand
>>   i2c: core: Probe i2c adapters and devices on demand
>>   pwm: Probe PWM chip devices on demand
>>   backlight: Probe backlight devices on demand
>>   usb: phy: Probe phy devices on demand
>>   clk: Probe clk providers on demand
>>   pinctrl: Probe pinctrl devices on demand
>>   phy: core: Probe phy providers on demand
>>   dma: of: Probe DMA controllers on demand
>>   power-supply: Probe power supplies on demand
>>   driver core: Allow deferring probes until late init
>>   driver core: Start processing deferred probes earlier
>>   of/platform: Defer probes of registered devices
>>
>>  drivers/amba/bus.c                  |  88 ++++++------
>>  drivers/base/Kconfig                |  18 +++
>>  drivers/base/dd.c                   |  35 ++++-
>>  drivers/clk/clk.c                   |   3 +
>>  drivers/dma/of-dma.c                |   3 +
>>  drivers/gpio/gpiolib-of.c           |   5 +
>>  drivers/gpu/drm/drm_panel.c         |   3 +
>>  drivers/gpu/drm/tegra/dpaux.c       |   3 +
>>  drivers/i2c/i2c-core.c              |   4 +
>>  drivers/of/device.c                 |  61 +++++++++
>>  drivers/of/platform.c               |  30 +++--
>>  drivers/phy/phy-core.c              |   3 +
>>  drivers/pinctrl/devicetree.c        |   3 +
>>  drivers/power/power_supply_core.c   |   3 +
>>  drivers/pwm/core.c                  |   3 +
>>  drivers/regulator/core.c            | 257 +++++++++++++++++++++++-------------
>>  drivers/usb/phy/phy.c               |   3 +
>>  drivers/video/backlight/backlight.c |   3 +
>>  include/linux/device.h              |   4 +-
>>  include/linux/of.h                  |   1 +
>>  include/linux/of_device.h           |   3 +
>>  21 files changed, 386 insertions(+), 150 deletions(-)
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 0/22] On-demand device probing

[Rob Herring]

On Tue, Oct 13, 2015 at 2:57 PM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> On 26 September 2015 at 20:17, Rob Herring <robh@kernel.org> wrote:
>> On 09/21/2015 09:02 AM, Tomeu Vizoso wrote:
>>> Hello,
>>>
>>> I have a problem with the panel on my Tegra Chromebook taking longer
>>> than expected to be ready during boot (Stéphane Marchesin reported what
>>> is basically the same issue in [0]), and have looked into ordered
>>> probing as a better way of solving this than moving nodes around in the
>>> DT or playing with initcall levels and linking order.
>>>
>>> While reading the thread [1] that Alexander Holler started with his
>>> series to make probing order deterministic, it occurred to me that it
>>> should be possible to achieve the same by probing devices as they are
>>> referenced by other devices.
>>>
>>> This basically reuses the information that is already implicit in the
>>> probe() implementations, saving us from refactoring existing drivers or
>>> adding information to DTBs.
>>>
>>> During review of v1 of this series Linus Walleij suggested that it
>>> should be the device driver core to make sure that dependencies are
>>> ready before probing a device. I gave this idea a try [2] but Mark Brown
>>> pointed out to the logic duplication between the resource acquisition
>>> and dependency discovery code paths (though I think it's fairly minor).
>>>
>>> To address that code duplication I experimented with Arnd's devm_probe
>>> [3] concept of having drivers declare their dependencies instead of
>>> acquiring them during probe, and while it worked [4], I don't think we
>>> end up winning anything when compared to just probing devices on-demand
>>> from resource getters.
>>>
>>> One remaining objection is to the "sprinkling" of calls to
>>> of_device_probe() in the resource getters of each subsystem, but I think
>>> it's the right thing to do given that the storage of resources is
>>> currently subsystem-specific.
>>>
>>> We could avoid the above by moving resource storage into the core, but I
>>> don't think there's a compelling case for that.
>>>
>>> I have tested this on boards with Tegra, iMX.6, Exynos, Rockchip and
>>> OMAP SoCs, and these patches were enough to eliminate all the deferred
>>> probes (except one in PandaBoard because omap_dma_system doesn't have a
>>> firmware node as of yet).
>>>
>>> Have submitted a branch [5][6][7] with these patches on top of today's
>>> linux-next (20150921) to kernelci.org and I don't see any issues that
>>> could be caused by them.
>>>
>>> With this series I get the kernel to output to the panel in 0.5s,
>>> instead of 2.8s.
>>
>> I think we're pretty close other than some minor comments. I would like
>> to see ack's from Greg and some reviewed-bys from others. The subsystem
>> changes are minor and there has been plenty of chance to comment, so I
>> don't think acks from all subsystems are needed.
>
> Hi Rob,
>
> I'm not sure we are going to get much more feedback by just waiting or
> resending what has been sent so many times.

Agreed.

> Did you have in mind specific people you wanted to see reviewed-bys from?

Mainly Greg.

Please send me a pull req. I want to get this into -next and can
always drop it if there is further review.

Rob

Re: [PATCH v6 22/22] of/platform: Defer probes of registered devices

[Scott Wood]

On Mon, 2015-09-21 at 16:03 +0200, Tomeu Vizoso wrote:
> Instead of trying to match and probe platform and AMBA devices right
> after each is registered, delay their probes until device_initcall_sync.
> 
> This means that devices will start probing once all built-in drivers
> have registered, and after all platform and AMBA devices from the DT
> have been registered already.
> 
> This allows us to prevent deferred probes by probing dependencies on
> demand.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
> Changes in v4:
> - Also defer probes of AMBA devices registered from the DT as they can
>   also request resources.
> 
>  drivers/of/platform.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

This breaks arch/powerpc/sysdev/fsl_pci.c.  The PCI bus is an OF platform 
device, and it must be probed before pcibios_init() which is a 
subsys_initcall(), or else the PCI bus never gets scanned.

-Scott

Re: [PATCH v6 22/22] of/platform: Defer probes of registered devices

[Rob Herring]

On Wed, Oct 21, 2015 at 12:54 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Mon, 2015-09-21 at 16:03 +0200, Tomeu Vizoso wrote:
>> Instead of trying to match and probe platform and AMBA devices right
>> after each is registered, delay their probes until device_initcall_sync.
>>
>> This means that devices will start probing once all built-in drivers
>> have registered, and after all platform and AMBA devices from the DT
>> have been registered already.
>>
>> This allows us to prevent deferred probes by probing dependencies on
>> demand.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>> Changes in v4:
>> - Also defer probes of AMBA devices registered from the DT as they can
>>   also request resources.
>>
>>  drivers/of/platform.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> This breaks arch/powerpc/sysdev/fsl_pci.c.  The PCI bus is an OF platform
> device, and it must be probed before pcibios_init() which is a
> subsys_initcall(), or else the PCI bus never gets scanned.

Thanks for the report. This is probably getting dropped, but it could
be disabled for PPC.

Any plans to fix this and make PCI hosts hotplugable? For the scanning
part, generally the host controller drivers are responsible for
scanning their bus now.

Rob

Re: [PATCH v6 22/22] of/platform: Defer probes of registered devices

[Scott Wood]

On Wed, 2015-10-21 at 08:44 -0500, Rob Herring wrote:
> On Wed, Oct 21, 2015 at 12:54 AM, Scott Wood <scottwood@freescale.com> 
> wrote:
> > On Mon, 2015-09-21 at 16:03 +0200, Tomeu Vizoso wrote:
> > > Instead of trying to match and probe platform and AMBA devices right
> > > after each is registered, delay their probes until device_initcall_sync.
> > > 
> > > This means that devices will start probing once all built-in drivers
> > > have registered, and after all platform and AMBA devices from the DT
> > > have been registered already.
> > > 
> > > This allows us to prevent deferred probes by probing dependencies on
> > > demand.
> > > 
> > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > > ---
> > > 
> > > Changes in v4:
> > > - Also defer probes of AMBA devices registered from the DT as they can
> > >   also request resources.
> > > 
> > >  drivers/of/platform.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > This breaks arch/powerpc/sysdev/fsl_pci.c.  The PCI bus is an OF platform
> > device, and it must be probed before pcibios_init() which is a
> > subsys_initcall(), or else the PCI bus never gets scanned.
> 
> Thanks for the report. This is probably getting dropped, but it could
> be disabled for PPC.

I don't think that adding another arbitrary arch difference would be the 
right solution.

> Any plans to fix this and make PCI hosts hotplugable? For the scanning
> part, generally the host controller drivers are responsible for
> scanning their bus now.

Scanning from the host controller driver seems like a reasonable goal, though 

it'd take a bit of digging to extract whatever other things fsl_pci may 
depend on from the common PPC PCI code, in particular the various things that 

pcibios_resource_survey() does after all PCI buses have been scanned.

There's also check_swiotlb_enabled(), another subsys_initcall, which frees 
the swiotlb memory if ppc_swiotlb_enable hasn't been set.  The PCI host 
controller probe sets ppc_swiotlb_enable if it wasn't able to create an 
inbound mapping for all RAM.  Even if we were to change that to a later 
initcall, there's nothing later than late_initcall that we could use.

-Scott

Re: [PATCH v6 22/22] of/platform: Defer probes of registered devices

[Michael Ellerman]

On Wed, 2015-10-21 at 00:54 -0500, Scott Wood wrote:

> On Mon, 2015-09-21 at 16:03 +0200, Tomeu Vizoso wrote:

> > Instead of trying to match and probe platform and AMBA devices right
> > after each is registered, delay their probes until device_initcall_sync.
> > 
> > This means that devices will start probing once all built-in drivers
> > have registered, and after all platform and AMBA devices from the DT
> > have been registered already.
> > 
> > This allows us to prevent deferred probes by probing dependencies on
> > demand.
> > 
> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > ---
> > 
> > Changes in v4:
> > - Also defer probes of AMBA devices registered from the DT as they can
> >   also request resources.
> > 
> >  drivers/of/platform.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> This breaks arch/powerpc/sysdev/fsl_pci.c.  The PCI bus is an OF platform 
> device, and it must be probed before pcibios_init() which is a 
> subsys_initcall(), or else the PCI bus never gets scanned.

Ah right. This is presumably why I'm not seeing any PCI devices on my p5020ds
with linux-next.

cheers

Re: [PATCH v6 01/22] driver core: handle -EPROBE_DEFER from bus_type.match()

[Rafael J. Wysocki]

On Monday, September 21, 2015 04:02:41 PM Tomeu Vizoso wrote:
> Lets implementations of the match() callback in struct bus_type to
> return errors and if it's -EPROBE_DEFER then queue the device for
> deferred probing.
> 
> This is useful to buses such as AMBA in which devices are registered
> before their matching information can be retrieved from the HW
> (typically because a clock driver hasn't probed yet).
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
> 
>  drivers/base/dd.c      | 24 ++++++++++++++++++++++--
>  include/linux/device.h |  2 +-
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index be0eb4639128..7dc04ee81c8b 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -488,6 +488,7 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
>  	struct device_attach_data *data = _data;
>  	struct device *dev = data->dev;
>  	bool async_allowed;
> +	int ret;
>  
>  	/*
>  	 * Check if device has already been claimed. This may
> @@ -498,8 +499,17 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
>  	if (dev->driver)
>  		return -EBUSY;
>  
> -	if (!driver_match_device(drv, dev))
> +	ret = driver_match_device(drv, dev);
> +	if (!ret)
>  		return 0;
> +	else if (ret < 0) {
> +		if (ret == -EPROBE_DEFER) {
> +			dev_dbg(dev, "Device match requests probe deferral\n");
> +			driver_deferred_probe_add(dev);
> +		} else
> +			dev_warn(dev, "Bus failed to match device: %d", ret);
> +		return ret;
> +	}

The code below was reachable if an error was returned from driver_match_device()
before this patch.  What's the reason for changing that?

Shouldn't the code here really be:

	ret = driver_match_device(drv, dev);
	if (!ret)
		return 0;

	if (ret == -EPROBE_DEFER) {
		driver_deferred_probe_add(dev);
		return ret;
	}

>  	async_allowed = driver_allows_async_probing(drv);
>  
> @@ -619,6 +629,7 @@ void device_initial_probe(struct device *dev)
>  static int __driver_attach(struct device *dev, void *data)
>  {
>  	struct device_driver *drv = data;
> +	int ret;
>  
>  	/*
>  	 * Lock device and try to bind to it. We drop the error
> @@ -630,8 +641,17 @@ static int __driver_attach(struct device *dev, void *data)
>  	 * is an error.
>  	 */
>  
> -	if (!driver_match_device(drv, dev))
> +	ret = driver_match_device(drv, dev);
> +	if (!ret)
> +		return 0;
> +	else if (ret < 0) {
> +		if (ret == -EPROBE_DEFER) {
> +			dev_dbg(dev, "Device match requests probe deferral\n");
> +			driver_deferred_probe_add(dev);
> +		} else
> +			dev_warn(dev, "Bus failed to match device: %d", ret);
>  		return 0;

Same comment here, plus why do we return 0 at this point?

> +	}
>  
>  	if (dev->parent)	/* Needed for USB */
>  		device_lock(dev->parent);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 5d7bc6349930..8e7b806f0744 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -70,7 +70,7 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>   * @dev_groups:	Default attributes of the devices on the bus.
>   * @drv_groups: Default attributes of the device drivers on the bus.
>   * @match:	Called, perhaps multiple times, whenever a new device or driver
> - *		is added for this bus. It should return a nonzero value if the
> + *		is added for this bus. It should return a positive value if the
>   *		given device can be handled by the given driver.
>   * @uevent:	Called when a device is added, removed, or a few other things
>   *		that generate uevents to add the environment variables.
> 

Thanks,
Rafael

Re: [PATCH v6 03/22] of/platform: Point to struct device from device node

[Rafael J. Wysocki]

On Monday, September 21, 2015 04:02:43 PM Tomeu Vizoso wrote:
> When adding platform and AMBA devices, set the device node's device
> member to point to it.
> 
> This speeds lookups considerably and is safe because we only create one
> of these devices for any given device node.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
> Changes in v5:
> - Set the pointer to struct device also for AMBA devices
> - Unset the pointer to struct device when the platform device is about
>   to be unregistered
> - Increase the reference count of the device before returning from
>   of_find_device_by_node()
> 
>  drivers/of/platform.c | 19 ++++++++++---------
>  include/linux/of.h    |  1 +
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 1001efaedcb8..408d89f1d124 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -32,11 +32,6 @@ const struct of_device_id of_default_bus_match_table[] = {
>  	{} /* Empty terminated list */
>  };
>  
> -static int of_dev_node_match(struct device *dev, void *data)
> -{
> -	return dev->of_node == data;
> -}
> -
>  /**
>   * of_find_device_by_node - Find the platform_device associated with a node
>   * @np: Pointer to device tree node
> @@ -45,10 +40,10 @@ static int of_dev_node_match(struct device *dev, void *data)
>   */
>  struct platform_device *of_find_device_by_node(struct device_node *np)
>  {
> -	struct device *dev;
> -
> -	dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
> -	return dev ? to_platform_device(dev) : NULL;
> +	if (np->device && np->device->bus == &platform_bus_type &&
> +	    get_device(np->device))
> +		return to_platform_device(np->device);
> +	return NULL;
>  }
>  EXPORT_SYMBOL(of_find_device_by_node);
>  
> @@ -192,6 +187,8 @@ static struct platform_device *of_platform_device_create_pdata(
>  		goto err_clear_flag;
>  	}
>  
> +	np->device = &dev->dev;
> +
>  	return dev;
>  
>  err_clear_flag:
> @@ -272,6 +269,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
>  		goto err_free;
>  	}
>  
> +	node->device = &dev->dev;
> +
>  	return dev;
>  
>  err_free:
> @@ -476,6 +475,8 @@ static int of_platform_device_destroy(struct device *dev, void *data)
>  	if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
>  		device_for_each_child(dev, NULL, of_platform_device_destroy);
>  
> +	dev->of_node->device = NULL;
> +
>  	if (dev->bus == &platform_bus_type)
>  		platform_device_unregister(to_platform_device(dev));
>  #ifdef CONFIG_ARM_AMBA
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 2194b8ca41f9..eb091be0f8ee 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -52,6 +52,7 @@ struct device_node {
>  	phandle phandle;
>  	const char *full_name;
>  	struct fwnode_handle fwnode;
> +	struct device *device;

There are cases in which multiple device objects point to the same device_node,
so is the single struct device pointer here really sufficient?

>  
>  	struct	property *properties;
>  	struct	property *deadprops;	/* removed properties */
> 

Thanks,
Rafael

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Rafael J. Wysocki]

On Monday, September 21, 2015 04:02:44 PM Tomeu Vizoso wrote:
> Walks the OF tree up and finds the closest ancestor that has a struct
> device associated with it, probing it if isn't bound to a driver yet.
> 
> The above should ensure that the dependency represented by the passed OF
> node is available, because probing a device should cause its descendants
> to be probed as well (when they get registered).
> 
> Subsystems can use this when looking up resources for drivers, to reduce
> the chances of deferred probes because of the probing order of devices.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
> Changes in v5:
> - Move the assignment to device_node->device for AMBA devices to another
>   commit.
> - Hold a reference to the struct device while it's in use in
>   of_device_probe().
> 
> Changes in v4:
> - Rename of_platform_probe to of_device_probe
> - Use device_node.device instead of device_node.platform_dev
> 
> Changes in v3:
> - Set and use device_node.platform_dev instead of reversing the logic to
>   find the platform device that encloses a device node.
> - Drop the fwnode API to probe firmware nodes and add OF-only API for
>   now. I think this same scheme could be used for machines with ACPI,
>   but I haven't been able to find one that had to defer its probes because
>   of the device probe order.
> 
>  drivers/of/device.c       | 61 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_device.h |  3 +++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 8b91ea241b10..836be71fc90e 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -286,3 +286,64 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>  
>  	return 0;
>  }
> +
> +/**
> + * of_device_probe() - Probe device associated with OF node
> + * @np: node to probe
> + *
> + * Probe the device associated with the passed device node.
> + */
> +void of_device_probe(struct device_node *np)

Same question as from Greg: How does a subsystem know whether or not to use
this function?

> +{
> +	struct device_node *target;
> +	struct device *dev = NULL;
> +
> +	if (!of_root || !of_node_check_flag(of_root, OF_POPULATED_BUS))
> +		return;
> +
> +	if (!np)
> +		return;
> +
> +	of_node_get(np);
> +
> +	/* Find the closest ancestor that has a device associated */
> +	for (target = np;
> +	     !of_node_is_root(target);
> +	     target = of_get_next_parent(target))
> +		if (get_device(target->device)) {
> +			dev = target->device;
> +			break;
> +		}
> +
> +	of_node_put(target);
> +
> +	if (!dev) {
> +		pr_warn("Couldn't find a device for node '%s'\n",
> +			of_node_full_name(np));
> +		return;
> +	}
> +
> +	/*
> +	 * Device is bound or is being probed right now. If we have bad luck
> +	 * and the dependency isn't ready when it's needed, deferred probe
> +	 * will save us.
> +	 */
> +	if (dev->driver)
> +		goto out;
> +
> +	/*
> +	 * Probing a device should cause its descendants to be probed as
> +	 * well, which includes the passed device node.
> +	 */
> +	if (device_attach(dev) != 1)
> +		/*
> +		 * This cannot be a warning for now because clock nodes have a
> +		 * compatible string but the clock framework doesn't follow
> +		 * the device/driver model yet.
> +		 */
> +		dev_dbg(dev, "Probe failed for %s\n", of_node_full_name(np));
> +
> +out:
> +	put_device(dev);
> +}
> +EXPORT_SYMBOL_GPL(of_device_probe);
> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> index cc7dd687a89d..da8d489e73ad 100644
> --- a/include/linux/of_device.h
> +++ b/include/linux/of_device.h
> @@ -40,6 +40,7 @@ extern ssize_t of_device_get_modalias(struct device *dev,
>  
>  extern void of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
>  extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env);
> +extern void of_device_probe(struct device_node *np);
>  
>  static inline void of_device_node_put(struct device *dev)
>  {
> @@ -84,6 +85,8 @@ static inline int of_device_uevent_modalias(struct device *dev,
>  	return -ENODEV;
>  }
>  
> +static inline void of_device_probe(struct device_node *np) { }
> +
>  static inline void of_device_node_put(struct device *dev) { }
>  
>  static inline const struct of_device_id *__of_match_device(
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 03/22] of/platform: Point to struct device from device node

[Tomeu Vizoso]

On 22 October 2015 at 03:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, September 21, 2015 04:02:43 PM Tomeu Vizoso wrote:
>> When adding platform and AMBA devices, set the device node's device
>> member to point to it.
>>
>> This speeds lookups considerably and is safe because we only create one
>> of these devices for any given device node.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>> Changes in v5:
>> - Set the pointer to struct device also for AMBA devices
>> - Unset the pointer to struct device when the platform device is about
>>   to be unregistered
>> - Increase the reference count of the device before returning from
>>   of_find_device_by_node()
>>
>>  drivers/of/platform.c | 19 ++++++++++---------
>>  include/linux/of.h    |  1 +
>>  2 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 1001efaedcb8..408d89f1d124 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -32,11 +32,6 @@ const struct of_device_id of_default_bus_match_table[] = {
>>       {} /* Empty terminated list */
>>  };
>>
>> -static int of_dev_node_match(struct device *dev, void *data)
>> -{
>> -     return dev->of_node == data;
>> -}
>> -
>>  /**
>>   * of_find_device_by_node - Find the platform_device associated with a node
>>   * @np: Pointer to device tree node
>> @@ -45,10 +40,10 @@ static int of_dev_node_match(struct device *dev, void *data)
>>   */
>>  struct platform_device *of_find_device_by_node(struct device_node *np)
>>  {
>> -     struct device *dev;
>> -
>> -     dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
>> -     return dev ? to_platform_device(dev) : NULL;
>> +     if (np->device && np->device->bus == &platform_bus_type &&
>> +         get_device(np->device))
>> +             return to_platform_device(np->device);
>> +     return NULL;
>>  }
>>  EXPORT_SYMBOL(of_find_device_by_node);
>>
>> @@ -192,6 +187,8 @@ static struct platform_device *of_platform_device_create_pdata(
>>               goto err_clear_flag;
>>       }
>>
>> +     np->device = &dev->dev;
>> +
>>       return dev;
>>
>>  err_clear_flag:
>> @@ -272,6 +269,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
>>               goto err_free;
>>       }
>>
>> +     node->device = &dev->dev;
>> +
>>       return dev;
>>
>>  err_free:
>> @@ -476,6 +475,8 @@ static int of_platform_device_destroy(struct device *dev, void *data)
>>       if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
>>               device_for_each_child(dev, NULL, of_platform_device_destroy);
>>
>> +     dev->of_node->device = NULL;
>> +
>>       if (dev->bus == &platform_bus_type)
>>               platform_device_unregister(to_platform_device(dev));
>>  #ifdef CONFIG_ARM_AMBA
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 2194b8ca41f9..eb091be0f8ee 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -52,6 +52,7 @@ struct device_node {
>>       phandle phandle;
>>       const char *full_name;
>>       struct fwnode_handle fwnode;
>> +     struct device *device;
>
> There are cases in which multiple device objects point to the same device_node,
> so is the single struct device pointer here really sufficient?

Only one struct device is registered for a given DT device node when
the DT is scanned (otherwise, of_find_device_by_node wouldn't be
safe).

If some driver makes a struct device to point to a device_node that
already has a struct device associated with it, that's fine.

Regards,

Tomeu

>>
>>       struct  property *properties;
>>       struct  property *deadprops;    /* removed properties */
>>
>
> Thanks,
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Tomeu Vizoso]

On 22 October 2015 at 03:06, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, September 21, 2015 04:02:44 PM Tomeu Vizoso wrote:
>> Walks the OF tree up and finds the closest ancestor that has a struct
>> device associated with it, probing it if isn't bound to a driver yet.
>>
>> The above should ensure that the dependency represented by the passed OF
>> node is available, because probing a device should cause its descendants
>> to be probed as well (when they get registered).
>>
>> Subsystems can use this when looking up resources for drivers, to reduce
>> the chances of deferred probes because of the probing order of devices.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>> Changes in v5:
>> - Move the assignment to device_node->device for AMBA devices to another
>>   commit.
>> - Hold a reference to the struct device while it's in use in
>>   of_device_probe().
>>
>> Changes in v4:
>> - Rename of_platform_probe to of_device_probe
>> - Use device_node.device instead of device_node.platform_dev
>>
>> Changes in v3:
>> - Set and use device_node.platform_dev instead of reversing the logic to
>>   find the platform device that encloses a device node.
>> - Drop the fwnode API to probe firmware nodes and add OF-only API for
>>   now. I think this same scheme could be used for machines with ACPI,
>>   but I haven't been able to find one that had to defer its probes because
>>   of the device probe order.
>>
>>  drivers/of/device.c       | 61 +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/of_device.h |  3 +++
>>  2 files changed, 64 insertions(+)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 8b91ea241b10..836be71fc90e 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -286,3 +286,64 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>>
>>       return 0;
>>  }
>> +
>> +/**
>> + * of_device_probe() - Probe device associated with OF node
>> + * @np: node to probe
>> + *
>> + * Probe the device associated with the passed device node.
>> + */
>> +void of_device_probe(struct device_node *np)
>
> Same question as from Greg: How does a subsystem know whether or not to use
> this function?

Maybe I don't understand the comment, but as the commit message says,
subsystems can use this when looking up resources for drivers. Or you
mean that this information should be in the API docs?

Thanks,

Tomeu

>> +{
>> +     struct device_node *target;
>> +     struct device *dev = NULL;
>> +
>> +     if (!of_root || !of_node_check_flag(of_root, OF_POPULATED_BUS))
>> +             return;
>> +
>> +     if (!np)
>> +             return;
>> +
>> +     of_node_get(np);
>> +
>> +     /* Find the closest ancestor that has a device associated */
>> +     for (target = np;
>> +          !of_node_is_root(target);
>> +          target = of_get_next_parent(target))
>> +             if (get_device(target->device)) {
>> +                     dev = target->device;
>> +                     break;
>> +             }
>> +
>> +     of_node_put(target);
>> +
>> +     if (!dev) {
>> +             pr_warn("Couldn't find a device for node '%s'\n",
>> +                     of_node_full_name(np));
>> +             return;
>> +     }
>> +
>> +     /*
>> +      * Device is bound or is being probed right now. If we have bad luck
>> +      * and the dependency isn't ready when it's needed, deferred probe
>> +      * will save us.
>> +      */
>> +     if (dev->driver)
>> +             goto out;
>> +
>> +     /*
>> +      * Probing a device should cause its descendants to be probed as
>> +      * well, which includes the passed device node.
>> +      */
>> +     if (device_attach(dev) != 1)
>> +             /*
>> +              * This cannot be a warning for now because clock nodes have a
>> +              * compatible string but the clock framework doesn't follow
>> +              * the device/driver model yet.
>> +              */
>> +             dev_dbg(dev, "Probe failed for %s\n", of_node_full_name(np));
>> +
>> +out:
>> +     put_device(dev);
>> +}
>> +EXPORT_SYMBOL_GPL(of_device_probe);
>> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
>> index cc7dd687a89d..da8d489e73ad 100644
>> --- a/include/linux/of_device.h
>> +++ b/include/linux/of_device.h
>> @@ -40,6 +40,7 @@ extern ssize_t of_device_get_modalias(struct device *dev,
>>
>>  extern void of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
>>  extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env);
>> +extern void of_device_probe(struct device_node *np);
>>
>>  static inline void of_device_node_put(struct device *dev)
>>  {
>> @@ -84,6 +85,8 @@ static inline int of_device_uevent_modalias(struct device *dev,
>>       return -ENODEV;
>>  }
>>
>> +static inline void of_device_probe(struct device_node *np) { }
>> +
>>  static inline void of_device_node_put(struct device *dev) { }
>>
>>  static inline const struct of_device_id *__of_match_device(
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 22/22] of/platform: Defer probes of registered devices

[Tomeu Vizoso]

On 22 October 2015 at 00:51, Scott Wood <scottwood@freescale.com> wrote:
> On Wed, 2015-10-21 at 08:44 -0500, Rob Herring wrote:
>> On Wed, Oct 21, 2015 at 12:54 AM, Scott Wood <scottwood@freescale.com>
>> wrote:
>> > On Mon, 2015-09-21 at 16:03 +0200, Tomeu Vizoso wrote:
>> > > Instead of trying to match and probe platform and AMBA devices right
>> > > after each is registered, delay their probes until device_initcall_sync.
>> > >
>> > > This means that devices will start probing once all built-in drivers
>> > > have registered, and after all platform and AMBA devices from the DT
>> > > have been registered already.
>> > >
>> > > This allows us to prevent deferred probes by probing dependencies on
>> > > demand.
>> > >
>> > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> > > ---
>> > >
>> > > Changes in v4:
>> > > - Also defer probes of AMBA devices registered from the DT as they can
>> > >   also request resources.
>> > >
>> > >  drivers/of/platform.c | 11 ++++++++---
>> > >  1 file changed, 8 insertions(+), 3 deletions(-)
>> >
>> > This breaks arch/powerpc/sysdev/fsl_pci.c.  The PCI bus is an OF platform
>> > device, and it must be probed before pcibios_init() which is a
>> > subsys_initcall(), or else the PCI bus never gets scanned.
>>
>> Thanks for the report. This is probably getting dropped, but it could
>> be disabled for PPC.
>
> I don't think that adding another arbitrary arch difference would be the
> right solution.

I think Rob meant temporarily disable it while things get fixed. At
least, I don't see any reason why PPC wouldn't benefit from this
series.

Regards,

Tomeu

>> Any plans to fix this and make PCI hosts hotplugable? For the scanning
>> part, generally the host controller drivers are responsible for
>> scanning their bus now.
>
> Scanning from the host controller driver seems like a reasonable goal, though
>
> it'd take a bit of digging to extract whatever other things fsl_pci may
> depend on from the common PPC PCI code, in particular the various things that
>
> pcibios_resource_survey() does after all PCI buses have been scanned.
>
> There's also check_swiotlb_enabled(), another subsys_initcall, which frees
> the swiotlb memory if ppc_swiotlb_enable hasn't been set.  The PCI host
> controller probe sets ppc_swiotlb_enable if it wasn't able to create an
> inbound mapping for all RAM.  Even if we were to change that to a later
> initcall, there's nothing later than late_initcall that we could use.
>
> -Scott
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 22/22] of/platform: Defer probes of registered devices

[Scott Wood]

On Thu, 2015-10-22 at 15:04 +0200, Tomeu Vizoso wrote:
> On 22 October 2015 at 00:51, Scott Wood <scottwood@freescale.com> wrote:
> > On Wed, 2015-10-21 at 08:44 -0500, Rob Herring wrote:
> > > On Wed, Oct 21, 2015 at 12:54 AM, Scott Wood <scottwood@freescale.com>
> > > wrote:
> > > > On Mon, 2015-09-21 at 16:03 +0200, Tomeu Vizoso wrote:
> > > > > Instead of trying to match and probe platform and AMBA devices right
> > > > > after each is registered, delay their probes until 
> > > > > device_initcall_sync.
> > > > > 
> > > > > This means that devices will start probing once all built-in drivers
> > > > > have registered, and after all platform and AMBA devices from the DT
> > > > > have been registered already.
> > > > > 
> > > > > This allows us to prevent deferred probes by probing dependencies on
> > > > > demand.
> > > > > 
> > > > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > > > > ---
> > > > > 
> > > > > Changes in v4:
> > > > > - Also defer probes of AMBA devices registered from the DT as they 
> > > > > can
> > > > >   also request resources.
> > > > > 
> > > > >  drivers/of/platform.c | 11 ++++++++---
> > > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > 
> > > > This breaks arch/powerpc/sysdev/fsl_pci.c.  The PCI bus is an OF 
> > > > platform
> > > > device, and it must be probed before pcibios_init() which is a
> > > > subsys_initcall(), or else the PCI bus never gets scanned.
> > > 
> > > Thanks for the report. This is probably getting dropped, but it could
> > > be disabled for PPC.
> > 
> > I don't think that adding another arbitrary arch difference would be the
> > right solution.
> 
> I think Rob meant temporarily disable it while things get fixed. At
> least, 

So, what is the permanent fix for the swiotlb issue (or more generally, the 
inability to have a late_initcall that runs after non-module, non-hotplug 
platform devices have been probed)?

> I don't see any reason why PPC wouldn't benefit from this
> series.

It's not clear to me what the benefit of this is at all, much less for PPC.   
What is the fundamental problem with deferred probes?  In the cover letter 
you say this change saves 2.3 seconds, but where is that time being consumed? 
 Are the drivers taking too long in their probe function trying to initialize 
and then deferring, rather than checking for dependencies up front?  Or are 
there really so many devices and such a pessimal ordering that most of the 
time is spent iterating through and reordering the list, with each defer 
happening quickly?

Even if something different does need to be done at this level, forcing all 
OF platform devices to be probed at the late_initcall level seems quite 
intrusive.  You limited it to OF because people complained that other things 
will break.  Things still broke.  Surely there's a better way to address the 
problem.  Can't the delay be requested by drivers that might otherwise need 
to defer (which could be done incrementally, focusing on the worst 
performance problems), rather than enabling it for everything?

-Scott

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

On Thu, Oct 22, 2015 at 03:03:37PM +0200, Tomeu Vizoso wrote:
> On 22 October 2015 at 03:06, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> > Same question as from Greg: How does a subsystem know whether or not to use
> > this function?

> Maybe I don't understand the comment, but as the commit message says,
> subsystems can use this when looking up resources for drivers. Or you
> mean that this information should be in the API docs?

I'm pretty sure what he's suggesting here is changing to "should always"
(ie, explain the situations when a subsystem would do this, including
decision criteria if it's optional).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v6 22/22] of/platform: Defer probes of registered devices

[Rafael J. Wysocki]

On Thursday, October 22, 2015 04:27:10 PM Scott Wood wrote:
> On Thu, 2015-10-22 at 15:04 +0200, Tomeu Vizoso wrote:
> > On 22 October 2015 at 00:51, Scott Wood <scottwood@freescale.com> wrote:
> > > On Wed, 2015-10-21 at 08:44 -0500, Rob Herring wrote:
> > > > On Wed, Oct 21, 2015 at 12:54 AM, Scott Wood <scottwood@freescale.com>
> > > > wrote:
> > > > > On Mon, 2015-09-21 at 16:03 +0200, Tomeu Vizoso wrote:
> > > > > > Instead of trying to match and probe platform and AMBA devices right
> > > > > > after each is registered, delay their probes until 
> > > > > > device_initcall_sync.
> > > > > > 
> > > > > > This means that devices will start probing once all built-in drivers
> > > > > > have registered, and after all platform and AMBA devices from the DT
> > > > > > have been registered already.
> > > > > > 
> > > > > > This allows us to prevent deferred probes by probing dependencies on
> > > > > > demand.
> > > > > > 
> > > > > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > > > > > ---
> > > > > > 
> > > > > > Changes in v4:
> > > > > > - Also defer probes of AMBA devices registered from the DT as they 
> > > > > > can
> > > > > >   also request resources.
> > > > > > 
> > > > > >  drivers/of/platform.c | 11 ++++++++---
> > > > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > > 
> > > > > This breaks arch/powerpc/sysdev/fsl_pci.c.  The PCI bus is an OF 
> > > > > platform
> > > > > device, and it must be probed before pcibios_init() which is a
> > > > > subsys_initcall(), or else the PCI bus never gets scanned.
> > > > 
> > > > Thanks for the report. This is probably getting dropped, but it could
> > > > be disabled for PPC.
> > > 
> > > I don't think that adding another arbitrary arch difference would be the
> > > right solution.
> > 
> > I think Rob meant temporarily disable it while things get fixed. At
> > least, 
> 
> So, what is the permanent fix for the swiotlb issue (or more generally, the 
> inability to have a late_initcall that runs after non-module, non-hotplug 
> platform devices have been probed)?
> 
> > I don't see any reason why PPC wouldn't benefit from this
> > series.
> 
> It's not clear to me what the benefit of this is at all, much less for PPC.   
> What is the fundamental problem with deferred probes?  In the cover letter 
> you say this change saves 2.3 seconds, but where is that time being consumed? 
>  Are the drivers taking too long in their probe function trying to initialize 
> and then deferring, rather than checking for dependencies up front?  Or are 
> there really so many devices and such a pessimal ordering that most of the 
> time is spent iterating through and reordering the list, with each defer 
> happening quickly?
> 
> Even if something different does need to be done at this level, forcing all 
> OF platform devices to be probed at the late_initcall level seems quite 
> intrusive.

Totally agreed.

> You limited it to OF because people complained that other things 
> will break.

Right.

And I'm not sure why that was regarded as a good enough reason to do it.

> Things still broke.

Yes, they did.

> Surely there's a better way to address the 
> problem.  Can't the delay be requested by drivers that might otherwise need 
> to defer (which could be done incrementally, focusing on the worst 
> performance problems), rather than enabling it for everything?

Well, I was suggesting to use an opt-in flag there, but I'm not sure if Tomeu
took that into consideration.

In any case, probing is just one aspect of a deeper issue, which is that
we have no way to represent functional dependencies between devices.

Thanks,
Rafael

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Rafael J. Wysocki]

On Thursday, October 22, 2015 03:03:37 PM Tomeu Vizoso wrote:
> On 22 October 2015 at 03:06, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, September 21, 2015 04:02:44 PM Tomeu Vizoso wrote:
> >> Walks the OF tree up and finds the closest ancestor that has a struct
> >> device associated with it, probing it if isn't bound to a driver yet.
> >>
> >> The above should ensure that the dependency represented by the passed OF
> >> node is available, because probing a device should cause its descendants
> >> to be probed as well (when they get registered).
> >>
> >> Subsystems can use this when looking up resources for drivers, to reduce
> >> the chances of deferred probes because of the probing order of devices.
> >>
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> ---
> >>
> >> Changes in v5:
> >> - Move the assignment to device_node->device for AMBA devices to another
> >>   commit.
> >> - Hold a reference to the struct device while it's in use in
> >>   of_device_probe().
> >>
> >> Changes in v4:
> >> - Rename of_platform_probe to of_device_probe
> >> - Use device_node.device instead of device_node.platform_dev
> >>
> >> Changes in v3:
> >> - Set and use device_node.platform_dev instead of reversing the logic to
> >>   find the platform device that encloses a device node.
> >> - Drop the fwnode API to probe firmware nodes and add OF-only API for
> >>   now. I think this same scheme could be used for machines with ACPI,
> >>   but I haven't been able to find one that had to defer its probes because
> >>   of the device probe order.
> >>
> >>  drivers/of/device.c       | 61 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/of_device.h |  3 +++
> >>  2 files changed, 64 insertions(+)
> >>
> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
> >> index 8b91ea241b10..836be71fc90e 100644
> >> --- a/drivers/of/device.c
> >> +++ b/drivers/of/device.c
> >> @@ -286,3 +286,64 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
> >>
> >>       return 0;
> >>  }
> >> +
> >> +/**
> >> + * of_device_probe() - Probe device associated with OF node
> >> + * @np: node to probe
> >> + *
> >> + * Probe the device associated with the passed device node.
> >> + */
> >> +void of_device_probe(struct device_node *np)
> >
> > Same question as from Greg: How does a subsystem know whether or not to use
> > this function?
> 
> Maybe I don't understand the comment, but as the commit message says,
> subsystems can use this when looking up resources for drivers. Or you
> mean that this information should be in the API docs?

I'm not really sure this is the only case.

Most likely, you'd end up with that being called by every subsystem using DT
just in case.

And then each of those subsystems would need to call acpi_device_probe(), and
then another_platform_firmware_interface_device_probe() and so on.

Don't you see a problem here?

Thanks,
Rafael

Re: [PATCH v6 03/22] of/platform: Point to struct device from device node

[Rafael J. Wysocki]

On Thursday, October 22, 2015 03:01:45 PM Tomeu Vizoso wrote:
> On 22 October 2015 at 03:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, September 21, 2015 04:02:43 PM Tomeu Vizoso wrote:
> >> When adding platform and AMBA devices, set the device node's device
> >> member to point to it.
> >>
> >> This speeds lookups considerably and is safe because we only create one
> >> of these devices for any given device node.
> >>
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> ---
> >>
> >> Changes in v5:
> >> - Set the pointer to struct device also for AMBA devices
> >> - Unset the pointer to struct device when the platform device is about
> >>   to be unregistered
> >> - Increase the reference count of the device before returning from
> >>   of_find_device_by_node()
> >>
> >>  drivers/of/platform.c | 19 ++++++++++---------
> >>  include/linux/of.h    |  1 +
> >>  2 files changed, 11 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >> index 1001efaedcb8..408d89f1d124 100644
> >> --- a/drivers/of/platform.c
> >> +++ b/drivers/of/platform.c
> >> @@ -32,11 +32,6 @@ const struct of_device_id of_default_bus_match_table[] = {
> >>       {} /* Empty terminated list */
> >>  };
> >>
> >> -static int of_dev_node_match(struct device *dev, void *data)
> >> -{
> >> -     return dev->of_node == data;
> >> -}
> >> -
> >>  /**
> >>   * of_find_device_by_node - Find the platform_device associated with a node
> >>   * @np: Pointer to device tree node
> >> @@ -45,10 +40,10 @@ static int of_dev_node_match(struct device *dev, void *data)
> >>   */
> >>  struct platform_device *of_find_device_by_node(struct device_node *np)
> >>  {
> >> -     struct device *dev;
> >> -
> >> -     dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
> >> -     return dev ? to_platform_device(dev) : NULL;
> >> +     if (np->device && np->device->bus == &platform_bus_type &&
> >> +         get_device(np->device))
> >> +             return to_platform_device(np->device);
> >> +     return NULL;
> >>  }
> >>  EXPORT_SYMBOL(of_find_device_by_node);
> >>
> >> @@ -192,6 +187,8 @@ static struct platform_device *of_platform_device_create_pdata(
> >>               goto err_clear_flag;
> >>       }
> >>
> >> +     np->device = &dev->dev;
> >> +
> >>       return dev;
> >>
> >>  err_clear_flag:
> >> @@ -272,6 +269,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
> >>               goto err_free;
> >>       }
> >>
> >> +     node->device = &dev->dev;
> >> +
> >>       return dev;
> >>
> >>  err_free:
> >> @@ -476,6 +475,8 @@ static int of_platform_device_destroy(struct device *dev, void *data)
> >>       if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
> >>               device_for_each_child(dev, NULL, of_platform_device_destroy);
> >>
> >> +     dev->of_node->device = NULL;
> >> +
> >>       if (dev->bus == &platform_bus_type)
> >>               platform_device_unregister(to_platform_device(dev));
> >>  #ifdef CONFIG_ARM_AMBA
> >> diff --git a/include/linux/of.h b/include/linux/of.h
> >> index 2194b8ca41f9..eb091be0f8ee 100644
> >> --- a/include/linux/of.h
> >> +++ b/include/linux/of.h
> >> @@ -52,6 +52,7 @@ struct device_node {
> >>       phandle phandle;
> >>       const char *full_name;
> >>       struct fwnode_handle fwnode;
> >> +     struct device *device;
> >
> > There are cases in which multiple device objects point to the same device_node,
> > so is the single struct device pointer here really sufficient?
> 
> Only one struct device is registered for a given DT device node when
> the DT is scanned (otherwise, of_find_device_by_node wouldn't be
> safe).
> 
> If some driver makes a struct device to point to a device_node that
> already has a struct device associated with it, that's fine.

Well, once that has happened, your new device pointer in the given device_node
becomes useless.  Why exactly is that fine?

Thanks,
Rafael

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Rafael J. Wysocki]

On Friday, October 23, 2015 08:54:19 AM Mark Brown wrote:
> 
> --7cm2iqirTL37Ot+N
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> 
> On Thu, Oct 22, 2015 at 03:03:37PM +0200, Tomeu Vizoso wrote:
> > On 22 October 2015 at 03:06, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 
> > > Same question as from Greg: How does a subsystem know whether or not to use
> > > this function?
> 
> > Maybe I don't understand the comment, but as the commit message says,
> > subsystems can use this when looking up resources for drivers. Or you
> > mean that this information should be in the API docs?
> 
> I'm pretty sure what he's suggesting here is changing to "should always"
> (ie, explain the situations when a subsystem would do this, including
> decision criteria if it's optional).

Right.

Thanks,
Rafael

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Rob Herring]

On 10/24/2015 08:55 AM, Rafael J. Wysocki wrote:
> On Thursday, October 22, 2015 03:03:37 PM Tomeu Vizoso wrote:
>> On 22 October 2015 at 03:06, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Monday, September 21, 2015 04:02:44 PM Tomeu Vizoso wrote:
>>>> Walks the OF tree up and finds the closest ancestor that has a struct
>>>> device associated with it, probing it if isn't bound to a driver yet.
>>>>
>>>> The above should ensure that the dependency represented by the passed OF
>>>> node is available, because probing a device should cause its descendants
>>>> to be probed as well (when they get registered).
>>>>
>>>> Subsystems can use this when looking up resources for drivers, to reduce
>>>> the chances of deferred probes because of the probing order of devices.
>>>>
>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>> ---
>>>>
>>>> Changes in v5:
>>>> - Move the assignment to device_node->device for AMBA devices to another
>>>>   commit.
>>>> - Hold a reference to the struct device while it's in use in
>>>>   of_device_probe().
>>>>
>>>> Changes in v4:
>>>> - Rename of_platform_probe to of_device_probe
>>>> - Use device_node.device instead of device_node.platform_dev
>>>>
>>>> Changes in v3:
>>>> - Set and use device_node.platform_dev instead of reversing the logic to
>>>>   find the platform device that encloses a device node.
>>>> - Drop the fwnode API to probe firmware nodes and add OF-only API for
>>>>   now. I think this same scheme could be used for machines with ACPI,
>>>>   but I haven't been able to find one that had to defer its probes because
>>>>   of the device probe order.
>>>>
>>>>  drivers/of/device.c       | 61 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/of_device.h |  3 +++
>>>>  2 files changed, 64 insertions(+)
>>>>
>>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>>> index 8b91ea241b10..836be71fc90e 100644
>>>> --- a/drivers/of/device.c
>>>> +++ b/drivers/of/device.c
>>>> @@ -286,3 +286,64 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>>>>
>>>>       return 0;
>>>>  }
>>>> +
>>>> +/**
>>>> + * of_device_probe() - Probe device associated with OF node
>>>> + * @np: node to probe
>>>> + *
>>>> + * Probe the device associated with the passed device node.
>>>> + */
>>>> +void of_device_probe(struct device_node *np)
>>>
>>> Same question as from Greg: How does a subsystem know whether or not to use
>>> this function?
>>
>> Maybe I don't understand the comment, but as the commit message says,
>> subsystems can use this when looking up resources for drivers. Or you
>> mean that this information should be in the API docs?
> 
> I'm not really sure this is the only case.
> 
> Most likely, you'd end up with that being called by every subsystem using DT
> just in case.

No, it is binding dependent. If a consumer is looking up the provider
from the binding, then yes it needs a call. There may be some cases
where it doesn't make sense. But that is why it is called from the
binding specific code.

> And then each of those subsystems would need to call acpi_device_probe(), and
> then another_platform_firmware_interface_device_probe() and so on.
> 
> Don't you see a problem here?

Only for the 3rd firmware interface. ;)

Let's get agreement on the flow and structure and how to address other
issues like suspend, then we can worry about whether this needs to be
abstracted from subsystems. We can discuss more this week at KS.

Rob

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

On Sat, Oct 24, 2015 at 03:09:06PM -0500, Rob Herring wrote:

> Let's get agreement on the flow and structure and how to address other
> issues like suspend, then we can worry about whether this needs to be
> abstracted from subsystems. We can discuss more this week at KS.

Should we try to schedule an ad-hoc session today (Monday) for those of
us who are here to talk this over?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Dmitry Torokhov]

On October 26, 2015 9:13:01 AM GMT+09:00, Mark Brown <broonie@kernel.org> wrote:
>On Sat, Oct 24, 2015 at 03:09:06PM -0500, Rob Herring wrote:
>
>> Let's get agreement on the flow and structure and how to address
>other
>> issues like suspend, then we can worry about whether this needs to be
>> abstracted from subsystems. We can discuss more this week at KS.
>
>Should we try to schedule an ad-hoc session today (Monday) for those of
>us who are here to talk this over?


That would be great.

Thanks.

-- 
Dmitry

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Rafael J. Wysocki]

On Mon, Oct 26, 2015 at 1:13 AM, Mark Brown <broonie@kernel.org> wrote:
> On Sat, Oct 24, 2015 at 03:09:06PM -0500, Rob Herring wrote:
>
>> Let's get agreement on the flow and structure and how to address other
>> issues like suspend, then we can worry about whether this needs to be
>> abstracted from subsystems. We can discuss more this week at KS.
>
> Should we try to schedule an ad-hoc session today (Monday) for those of
> us who are here to talk this over?

I won't mind doing that, what about after the Linus+Dirk session?

Thanks,
Rafael

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

On Mon, Oct 26, 2015 at 03:48:44AM +0100, Rafael J. Wysocki wrote:
> On Mon, Oct 26, 2015 at 1:13 AM, Mark Brown <broonie@kernel.org> wrote:

> > Should we try to schedule an ad-hoc session today (Monday) for those of
> > us who are here to talk this over?

> I won't mind doing that, what about after the Linus+Dirk session?

It's looking hard to round people up...  I asked Ted for a slot
tomorrow, hopefully we can get one and if not it's probably going to be
easier to find everyone at once.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Geert Uytterhoeven]

On Mon, Sep 21, 2015 at 4:02 PM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> Walks the OF tree up and finds the closest ancestor that has a struct
> device associated with it, probing it if isn't bound to a driver yet.
>
> The above should ensure that the dependency represented by the passed OF
> node is available, because probing a device should cause its descendants
> to be probed as well (when they get registered).
>
> Subsystems can use this when looking up resources for drivers, to reduce
> the chances of deferred probes because of the probing order of devices.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Don't know to which response I should post this comment, so I'm responding
to the original email.

Some subsystems already do this.
If you call e.g. syscon_regmap_lookup_by_phandle(), it will call
of_syscon_register() if the syscon device pointed to hasn't been registered yet.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v6 04/22] of: add function to allow probing a device from a OF node

[Tomeu Vizoso]

On 26 October 2015 at 09:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, Sep 21, 2015 at 4:02 PM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> Walks the OF tree up and finds the closest ancestor that has a struct
>> device associated with it, probing it if isn't bound to a driver yet.
>>
>> The above should ensure that the dependency represented by the passed OF
>> node is available, because probing a device should cause its descendants
>> to be probed as well (when they get registered).
>>
>> Subsystems can use this when looking up resources for drivers, to reduce
>> the chances of deferred probes because of the probing order of devices.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> Don't know to which response I should post this comment, so I'm responding
> to the original email.
>
> Some subsystems already do this.
> If you call e.g. syscon_regmap_lookup_by_phandle(), it will call
> of_syscon_register() if the syscon device pointed to hasn't been registered yet.

Hi Geert,

I think I prefer if devices are registered as early as possible and
then only probed on-demand, as registration is very much
device-specific but probing is the same for all devices.

But I think in general we should be doing more things on demand and
depending less on the order in which we mass initialize stuff
(devices, classes, drivers, etc).

Regards,

Tomeu

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 03/22] of/platform: Point to struct device from device node

[Tomeu Vizoso]

On 24 October 2015 at 15:57, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, October 22, 2015 03:01:45 PM Tomeu Vizoso wrote:
>> On 22 October 2015 at 03:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Monday, September 21, 2015 04:02:43 PM Tomeu Vizoso wrote:
>> >> When adding platform and AMBA devices, set the device node's device
>> >> member to point to it.
>> >>
>> >> This speeds lookups considerably and is safe because we only create one
>> >> of these devices for any given device node.
>> >>
>> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> >> ---
>> >>
>> >> Changes in v5:
>> >> - Set the pointer to struct device also for AMBA devices
>> >> - Unset the pointer to struct device when the platform device is about
>> >>   to be unregistered
>> >> - Increase the reference count of the device before returning from
>> >>   of_find_device_by_node()
>> >>
>> >>  drivers/of/platform.c | 19 ++++++++++---------
>> >>  include/linux/of.h    |  1 +
>> >>  2 files changed, 11 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> >> index 1001efaedcb8..408d89f1d124 100644
>> >> --- a/drivers/of/platform.c
>> >> +++ b/drivers/of/platform.c
>> >> @@ -32,11 +32,6 @@ const struct of_device_id of_default_bus_match_table[] = {
>> >>       {} /* Empty terminated list */
>> >>  };
>> >>
>> >> -static int of_dev_node_match(struct device *dev, void *data)
>> >> -{
>> >> -     return dev->of_node == data;
>> >> -}
>> >> -
>> >>  /**
>> >>   * of_find_device_by_node - Find the platform_device associated with a node
>> >>   * @np: Pointer to device tree node
>> >> @@ -45,10 +40,10 @@ static int of_dev_node_match(struct device *dev, void *data)
>> >>   */
>> >>  struct platform_device *of_find_device_by_node(struct device_node *np)
>> >>  {
>> >> -     struct device *dev;
>> >> -
>> >> -     dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
>> >> -     return dev ? to_platform_device(dev) : NULL;
>> >> +     if (np->device && np->device->bus == &platform_bus_type &&
>> >> +         get_device(np->device))
>> >> +             return to_platform_device(np->device);
>> >> +     return NULL;
>> >>  }
>> >>  EXPORT_SYMBOL(of_find_device_by_node);
>> >>
>> >> @@ -192,6 +187,8 @@ static struct platform_device *of_platform_device_create_pdata(
>> >>               goto err_clear_flag;
>> >>       }
>> >>
>> >> +     np->device = &dev->dev;
>> >> +
>> >>       return dev;
>> >>
>> >>  err_clear_flag:
>> >> @@ -272,6 +269,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
>> >>               goto err_free;
>> >>       }
>> >>
>> >> +     node->device = &dev->dev;
>> >> +
>> >>       return dev;
>> >>
>> >>  err_free:
>> >> @@ -476,6 +475,8 @@ static int of_platform_device_destroy(struct device *dev, void *data)
>> >>       if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
>> >>               device_for_each_child(dev, NULL, of_platform_device_destroy);
>> >>
>> >> +     dev->of_node->device = NULL;
>> >> +
>> >>       if (dev->bus == &platform_bus_type)
>> >>               platform_device_unregister(to_platform_device(dev));
>> >>  #ifdef CONFIG_ARM_AMBA
>> >> diff --git a/include/linux/of.h b/include/linux/of.h
>> >> index 2194b8ca41f9..eb091be0f8ee 100644
>> >> --- a/include/linux/of.h
>> >> +++ b/include/linux/of.h
>> >> @@ -52,6 +52,7 @@ struct device_node {
>> >>       phandle phandle;
>> >>       const char *full_name;
>> >>       struct fwnode_handle fwnode;
>> >> +     struct device *device;
>> >
>> > There are cases in which multiple device objects point to the same device_node,
>> > so is the single struct device pointer here really sufficient?
>>
>> Only one struct device is registered for a given DT device node when
>> the DT is scanned (otherwise, of_find_device_by_node wouldn't be
>> safe).
>>
>> If some driver makes a struct device to point to a device_node that
>> already has a struct device associated with it, that's fine.
>
> Well, once that has happened, your new device pointer in the given device_node
> becomes useless.  Why exactly is that fine?

Why do you think it's useless? It's the device that was registered
from that device_node. The other one just happens to want to point to
the same device_node for whatever other reason, but I don't see how is
that relevant.

Regards,

Tomeu

Re: [PATCH v6 03/22] of/platform: Point to struct device from device node

[Rafael J. Wysocki]

On Tuesday, October 27, 2015 03:48:40 PM Tomeu Vizoso wrote:
> On 24 October 2015 at 15:57, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

[...]

> >
> > Well, once that has happened, your new device pointer in the given device_node
> > becomes useless.  Why exactly is that fine?
> 
> Why do you think it's useless? It's the device that was registered
> from that device_node. The other one just happens to want to point to
> the same device_node for whatever other reason, but I don't see how is
> that relevant.

It is useless as a reverse mapping from OF nodes to devices.  It tells us
which device pointing to the given device node has been found first, but
that's just it.

You can't say whether or not it is special in any way with respect to the
other devices hanging out of the same OF node unless you have some additional
information on how those devices are related to each other.

Thanks,
Rafael

Re: [PATCH v6 03/22] of/platform: Point to struct device from device node

[Rob Herring]

On Tue, Oct 27, 2015 at 10:43 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, October 27, 2015 03:48:40 PM Tomeu Vizoso wrote:
>> On 24 October 2015 at 15:57, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> [...]
>
>> >
>> > Well, once that has happened, your new device pointer in the given device_node
>> > becomes useless.  Why exactly is that fine?
>>
>> Why do you think it's useless? It's the device that was registered
>> from that device_node. The other one just happens to want to point to
>> the same device_node for whatever other reason, but I don't see how is
>> that relevant.
>
> It is useless as a reverse mapping from OF nodes to devices.  It tells us
> which device pointing to the given device node has been found first, but
> that's just it.
>
> You can't say whether or not it is special in any way with respect to the
> other devices hanging out of the same OF node unless you have some additional
> information on how those devices are related to each other.

As Tomeu says this struct device is the one created from scanning the
DT. There should never be more than one. Now some drivers create sub
devices and maybe they set the DT node on the child devices, but that
is wrong. Doing that would also break the current code:

struct platform_device *of_find_device_by_node(struct device_node *np)
{
        struct device *dev;

        dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
        return dev ? to_platform_device(dev) : NULL;
}

I can't think of any other possible cases.

Rob

Re: [PATCH v6 22/22] of/platform: Defer probes of registered devices

[Tomeu Vizoso]

On 22 October 2015 at 23:27, Scott Wood <scottwood@freescale.com> wrote:
> On Thu, 2015-10-22 at 15:04 +0200, Tomeu Vizoso wrote:
>> On 22 October 2015 at 00:51, Scott Wood <scottwood@freescale.com> wrote:
>> > On Wed, 2015-10-21 at 08:44 -0500, Rob Herring wrote:
>> > > On Wed, Oct 21, 2015 at 12:54 AM, Scott Wood <scottwood@freescale.com>
>> > > wrote:
>> > > > On Mon, 2015-09-21 at 16:03 +0200, Tomeu Vizoso wrote:
>> > > > > Instead of trying to match and probe platform and AMBA devices right
>> > > > > after each is registered, delay their probes until
>> > > > > device_initcall_sync.
>> > > > >
>> > > > > This means that devices will start probing once all built-in drivers
>> > > > > have registered, and after all platform and AMBA devices from the DT
>> > > > > have been registered already.
>> > > > >
>> > > > > This allows us to prevent deferred probes by probing dependencies on
>> > > > > demand.
>> > > > >
>> > > > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> > > > > ---
>> > > > >
>> > > > > Changes in v4:
>> > > > > - Also defer probes of AMBA devices registered from the DT as they
>> > > > > can
>> > > > >   also request resources.
>> > > > >
>> > > > >  drivers/of/platform.c | 11 ++++++++---
>> > > > >  1 file changed, 8 insertions(+), 3 deletions(-)
>> > > >
>> > > > This breaks arch/powerpc/sysdev/fsl_pci.c.  The PCI bus is an OF
>> > > > platform
>> > > > device, and it must be probed before pcibios_init() which is a
>> > > > subsys_initcall(), or else the PCI bus never gets scanned.
>> > >
>> > > Thanks for the report. This is probably getting dropped, but it could
>> > > be disabled for PPC.
>> >
>> > I don't think that adding another arbitrary arch difference would be the
>> > right solution.
>>
>> I think Rob meant temporarily disable it while things get fixed. At
>> least,
>
> So, what is the permanent fix for the swiotlb issue (or more generally, the
> inability to have a late_initcall that runs after non-module, non-hotplug
> platform devices have been probed)?

If the code in pcibios_init() depends on the PCI bus device having
probed, then I would recommend making that dependency explicit by
calling of_device_probe() on the OF node of the PCI controller when
looking it up.

>> I don't see any reason why PPC wouldn't benefit from this
>> series.
>
> It's not clear to me what the benefit of this is at all, much less for PPC.
> What is the fundamental problem with deferred probes?  In the cover letter
> you say this change saves 2.3 seconds, but where is that time being consumed?
>  Are the drivers taking too long in their probe function trying to initialize
> and then deferring, rather than checking for dependencies up front?  Or are
> there really so many devices and such a pessimal ordering that most of the
> time is spent iterating through and reordering the list, with each defer
> happening quickly?

The problem is that a device that defers its probe is currently sent
to the back of the queue, and that's undesired in some use cases in
which there's a device that should be up as soon as possible during
boot (and boot takes a long time). So the goal is to change the order
in which devices with dependencies end up probing.

> Even if something different does need to be done at this level, forcing all
> OF platform devices to be probed at the late_initcall level seems quite
> intrusive.  You limited it to OF because people complained that other things
> will break.  Things still broke.  Surely there's a better way to address the
> problem.  Can't the delay be requested by drivers that might otherwise need
> to defer (which could be done incrementally, focusing on the worst
> performance problems), rather than enabling it for everything?

Yes, given the amount of breakage that's a sensible option. But in any
case and even if this series is most likely to be dropped, I recommend
to make explicit as many implicit dependencies as possible.

Regards,

Tomeu

> -Scott
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 22/22] of/platform: Defer probes of registered devices

[Rob Herring]

On Wed, Oct 28, 2015 at 9:40 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> On 22 October 2015 at 23:27, Scott Wood <scottwood@freescale.com> wrote:
>> On Thu, 2015-10-22 at 15:04 +0200, Tomeu Vizoso wrote:
>>> On 22 October 2015 at 00:51, Scott Wood <scottwood@freescale.com> wrote:
>>> > On Wed, 2015-10-21 at 08:44 -0500, Rob Herring wrote:
>>> > > On Wed, Oct 21, 2015 at 12:54 AM, Scott Wood <scottwood@freescale.com>
>>> > > wrote:
>>> > > > On Mon, 2015-09-21 at 16:03 +0200, Tomeu Vizoso wrote:
>>> > > > > Instead of trying to match and probe platform and AMBA devices right
>>> > > > > after each is registered, delay their probes until
>>> > > > > device_initcall_sync.
>>> > > > >
>>> > > > > This means that devices will start probing once all built-in drivers
>>> > > > > have registered, and after all platform and AMBA devices from the DT
>>> > > > > have been registered already.
>>> > > > >
>>> > > > > This allows us to prevent deferred probes by probing dependencies on
>>> > > > > demand.
>>> > > > >
>>> > > > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> > > > > ---
>>> > > > >
>>> > > > > Changes in v4:
>>> > > > > - Also defer probes of AMBA devices registered from the DT as they
>>> > > > > can
>>> > > > >   also request resources.
>>> > > > >
>>> > > > >  drivers/of/platform.c | 11 ++++++++---
>>> > > > >  1 file changed, 8 insertions(+), 3 deletions(-)
>>> > > >
>>> > > > This breaks arch/powerpc/sysdev/fsl_pci.c.  The PCI bus is an OF
>>> > > > platform
>>> > > > device, and it must be probed before pcibios_init() which is a
>>> > > > subsys_initcall(), or else the PCI bus never gets scanned.
>>> > >
>>> > > Thanks for the report. This is probably getting dropped, but it could
>>> > > be disabled for PPC.
>>> >
>>> > I don't think that adding another arbitrary arch difference would be the
>>> > right solution.
>>>
>>> I think Rob meant temporarily disable it while things get fixed. At
>>> least,
>>
>> So, what is the permanent fix for the swiotlb issue (or more generally, the
>> inability to have a late_initcall that runs after non-module, non-hotplug
>> platform devices have been probed)?
>
> If the code in pcibios_init() depends on the PCI bus device having
> probed, then I would recommend making that dependency explicit by
> calling of_device_probe() on the OF node of the PCI controller when
> looking it up.
>
>>> I don't see any reason why PPC wouldn't benefit from this
>>> series.
>>
>> It's not clear to me what the benefit of this is at all, much less for PPC.
>> What is the fundamental problem with deferred probes?  In the cover letter
>> you say this change saves 2.3 seconds, but where is that time being consumed?
>>  Are the drivers taking too long in their probe function trying to initialize
>> and then deferring, rather than checking for dependencies up front?  Or are
>> there really so many devices and such a pessimal ordering that most of the
>> time is spent iterating through and reordering the list, with each defer
>> happening quickly?
>
> The problem is that a device that defers its probe is currently sent
> to the back of the queue, and that's undesired in some use cases in
> which there's a device that should be up as soon as possible during
> boot (and boot takes a long time). So the goal is to change the order
> in which devices with dependencies end up probing.
>
>> Even if something different does need to be done at this level, forcing all
>> OF platform devices to be probed at the late_initcall level seems quite
>> intrusive.  You limited it to OF because people complained that other things
>> will break.  Things still broke.  Surely there's a better way to address the
>> problem.  Can't the delay be requested by drivers that might otherwise need
>> to defer (which could be done incrementally, focusing on the worst
>> performance problems), rather than enabling it for everything?
>
> Yes, given the amount of breakage that's a sensible option. But in any
> case and even if this series is most likely to be dropped, I recommend
> to make explicit as many implicit dependencies as possible.

It is dropped for now, but I expect much of this to still integrate
with Rafael's proposal. However, it will need to be opt-in it seems.
That is somewhat unfortunate because things like async probe are
opt-in and seem to rarely get used. As to how we can make it opt-in, I
think we can make of_platform_populate() callable multiple times at
the root level. This will allow a platform to skip calling it and then
a late call to it can register the devices after drivers have
registered. This late call will then be a nop for platforms that don't
opt-in and call of_platform_populate themselves.

Rob

Re: [PATCH v6 22/22] of/platform: Defer probes of registered devices

[Scott Wood]

On Wed, 2015-10-28 at 15:40 +0100, Tomeu Vizoso wrote:
> On 22 October 2015 at 23:27, Scott Wood <scottwood@freescale.com> wrote:
> > On Thu, 2015-10-22 at 15:04 +0200, Tomeu Vizoso wrote:
> > > On 22 October 2015 at 00:51, Scott Wood <scottwood@freescale.com> wrote:
> > > > On Wed, 2015-10-21 at 08:44 -0500, Rob Herring wrote:
> > > > > On Wed, Oct 21, 2015 at 12:54 AM, Scott Wood <
> > > > > scottwood@freescale.com>
> > > > > wrote:
> > > > > > On Mon, 2015-09-21 at 16:03 +0200, Tomeu Vizoso wrote:
> > > > > > > Instead of trying to match and probe platform and AMBA devices 
> > > > > > > right
> > > > > > > after each is registered, delay their probes until
> > > > > > > device_initcall_sync.
> > > > > > > 
> > > > > > > This means that devices will start probing once all built-in 
> > > > > > > drivers
> > > > > > > have registered, and after all platform and AMBA devices from 
> > > > > > > the DT
> > > > > > > have been registered already.
> > > > > > > 
> > > > > > > This allows us to prevent deferred probes by probing 
> > > > > > > dependencies on
> > > > > > > demand.
> > > > > > > 
> > > > > > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > Changes in v4:
> > > > > > > - Also defer probes of AMBA devices registered from the DT as 
> > > > > > > they
> > > > > > > can
> > > > > > >   also request resources.
> > > > > > > 
> > > > > > >  drivers/of/platform.c | 11 ++++++++---
> > > > > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > This breaks arch/powerpc/sysdev/fsl_pci.c.  The PCI bus is an OF
> > > > > > platform
> > > > > > device, and it must be probed before pcibios_init() which is a
> > > > > > subsys_initcall(), or else the PCI bus never gets scanned.
> > > > > 
> > > > > Thanks for the report. This is probably getting dropped, but it 
> > > > > could
> > > > > be disabled for PPC.
> > > > 
> > > > I don't think that adding another arbitrary arch difference would be 
> > > > the
> > > > right solution.
> > > 
> > > I think Rob meant temporarily disable it while things get fixed. At
> > > least,
> > 
> > So, what is the permanent fix for the swiotlb issue (or more generally, 
> > the
> > inability to have a late_initcall that runs after non-module, non-hotplug
> > platform devices have been probed)?
> 
> If the code in pcibios_init() depends on the PCI bus device having
> probed, then I would recommend making that dependency explicit by
> calling of_device_probe() on the OF node of the PCI controller when
> looking it up.

"when looking it up"?  pcibios_init() doesn't do anything with the OF node or 
know any details about the particular PCI bus host implementation.

> > > I don't see any reason why PPC wouldn't benefit from this
> > > series.
> > 
> > It's not clear to me what the benefit of this is at all, much less for 
> > PPC.
> > What is the fundamental problem with deferred probes?  In the cover letter
> > you say this change saves 2.3 seconds, but where is that time being 
> > consumed?
> >  Are the drivers taking too long in their probe function trying to 
> > initialize
> > and then deferring, rather than checking for dependencies up front?  Or 
> > are
> > there really so many devices and such a pessimal ordering that most of the
> > time is spent iterating through and reordering the list, with each defer
> > happening quickly?
> 
> The problem is that a device that defers its probe is currently sent
> to the back of the queue, and that's undesired in some use cases in
> which there's a device that should be up as soon as possible during
> boot (and boot takes a long time). So the goal is to change the order
> in which devices with dependencies end up probing.

That doesn't answer my question about where the time is being spent.  Did you 
profile?

-Scott