LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering
@ 2019-07-12 23:52 Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 01/11] driver core: Add support for linking devices during device addition Saravana Kannan
                   ` (10 more replies)
  0 siblings, 11 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

Add device-links to track functional dependencies between devices
after they are created (but before they are probed) by looking at
their common DT bindings like clocks, interconnects, etc.

Having functional dependencies automatically added before the devices
are probed, provides the following benefits:

- Optimizes device probe order and avoids the useless work of
  attempting probes of devices that will not probe successfully
  (because their suppliers aren't present or haven't probed yet).

  For example, in a commonly available mobile SoC, registering just
  one consumer device's driver at an initcall level earlier than the
  supplier device's driver causes 11 failed probe attempts before the
  consumer device probes successfully. This was with a kernel with all
  the drivers statically compiled in. This problem gets a lot worse if
  all the drivers are loaded as modules without direct symbol
  dependencies.

- Supplier devices like clock providers, interconnect providers, etc
  need to keep the resources they provide active and at a particular
  state(s) during boot up even if their current set of consumers don't
  request the resource to be active. This is because the rest of the
  consumers might not have probed yet and turning off the resource
  before all the consumers have probed could lead to a hang or
  undesired user experience.

  Some frameworks (Eg: regulator) handle this today by turning off
  "unused" resources at late_initcall_sync and hoping all the devices
  have probed by then. This is not a valid assumption for systems with
  loadable modules. Other frameworks (Eg: clock) just don't handle
  this due to the lack of a clear signal for when they can turn off
  resources. This leads to downstream hacks to handle cases like this
  that can easily be solved in the upstream kernel.

  By linking devices before they are probed, we give suppliers a clear
  count of the number of dependent consumers. Once all of the
  consumers are active, the suppliers can turn off the unused
  resources without making assumptions about the number of consumers.

By default we just add device-links to track "driver presence" (probe
succeeded) of the supplier device. If any other functionality provided
by device-links are needed, it is left to the consumer/supplier
devices to change the link when they probe.

v1 -> v2:
- Drop patch to speed up of_find_device_by_node()
- Drop depends-on property and use existing bindings
v2 -> v3:
- Refactor the code to have driver core initiate the linking of devs
- Have driver core link consumers to supplier before it's probed
- Add support for drivers to edit the device links before probing
v3 -> v4
- Tested edit_links() on system with cyclic dependency. Works.
- Added some checks to make sure device link isn't attempted from
  parent device node to child device node.
- Added way to pause/resume sync_state callbacks across
  of_platform_populate().
- Recursively parse DT node to create device links from parent to
  suppliers of parent and all child nodes.
v4 -> v5
- Fixed copy-pasta bugs with linked list handling
- Walk up the phandle reference till I find an actual device (needed
  for regulators to work)
- Added support for linking devices from regulator DT bindings
- Tested the whole series again to make sure cyclic dependencies are
  broken with edit_links() and regulator links are created properly.

I could probably squash some of the patches, but leaving them like this
because I think it's easier to understand this way.

I've also not updated this patch series to handle the new patch [1] from
Rafael. Will do that once this patch series is close to being Acked.

[1] - https://lore.kernel.org/lkml/3121545.4lOhFoIcdQ@kreacher/

-Saravana


Saravana Kannan (11):
  driver core: Add support for linking devices during device addition
  of/platform: Add functional dependency link from DT bindings
  driver core: Add sync_state driver/bus callback
  driver core: Add edit_links() callback for drivers
  driver core: Add APIs to pause/resume sync state callbacks
  of/platform: Pause/resume sync state in of_platform_populate()
  of/platform: Sanity check DT bindings before creating device links
  of/platform: Make sure supplier DT node is device when creating device
    links
  of/platform: Create device links for all child-supplier depencencies
  of/platform: Add functional dependency link from DT regulator bindings
  of/platform: Don't create device links default busses

 .../admin-guide/kernel-parameters.txt         |   5 +
 drivers/base/core.c                           | 168 ++++++++++++++++++
 drivers/base/dd.c                             |  29 +++
 drivers/of/platform.c                         | 132 ++++++++++++++
 include/linux/device.h                        |  47 +++++
 5 files changed, 381 insertions(+)

-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 01/11] driver core: Add support for linking devices during device addition
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings Saravana Kannan
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

When devices are added, the bus might want to create device links to track
functional dependencies between supplier and consumer devices. This
tracking of supplier-consumer relationship allows optimizing device probe
order and tracking whether all consumers of a supplier are active. The
add_links bus callback is added to support this.

However, when consumer devices are added, they might not have a supplier
device to link to despite needing mandatory resources/functionality from
one or more suppliers. A waiting_for_suppliers list is created to track
such consumers and retry linking them when new devices get added.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 83 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  8 ++++
 2 files changed, 91 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index fd7511e04e62..0705926d362f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -44,6 +44,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
 #endif
 
 /* Device links support. */
+static LIST_HEAD(wait_for_suppliers);
+static DEFINE_MUTEX(wfs_lock);
 
 #ifdef CONFIG_SRCU
 static DEFINE_MUTEX(device_links_lock);
@@ -401,6 +403,51 @@ struct device_link *device_link_add(struct device *consumer,
 }
 EXPORT_SYMBOL_GPL(device_link_add);
 
+/**
+ * device_link_wait_for_supplier - Mark device as waiting for supplier
+ * @consumer: Consumer device
+ *
+ * Marks the consumer device as waiting for suppliers to become available. The
+ * consumer device will never be probed until it's unmarked as waiting for
+ * suppliers. The caller is responsible for adding the link to the supplier
+ * once the supplier device is present.
+ *
+ * This function is NOT meant to be called from the probe function of the
+ * consumer but rather from code that creates/adds the consumer device.
+ */
+static void device_link_wait_for_supplier(struct device *consumer)
+{
+	mutex_lock(&wfs_lock);
+	list_add_tail(&consumer->links.needs_suppliers, &wait_for_suppliers);
+	mutex_unlock(&wfs_lock);
+}
+
+/**
+ * device_link_check_waiting_consumers - Try to unmark waiting consumers
+ *
+ * Loops through all consumers waiting on suppliers and tries to add all their
+ * supplier links. If that succeeds, the consumer device is unmarked as waiting
+ * for suppliers. Otherwise, they are left marked as waiting on suppliers,
+ *
+ * The add_links bus callback is expected to return 0 if it has found and added
+ * all the supplier links for the consumer device. It should return an error if
+ * it isn't able to do so.
+ *
+ * The caller of device_link_wait_for_supplier() is expected to call this once
+ * it's aware of potential suppliers becoming available.
+ */
+static void device_link_check_waiting_consumers(void)
+{
+	struct device *dev, *tmp;
+
+	mutex_lock(&wfs_lock);
+	list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
+				 links.needs_suppliers)
+		if (!dev->bus->add_links(dev))
+			list_del_init(&dev->links.needs_suppliers);
+	mutex_unlock(&wfs_lock);
+}
+
 static void device_link_free(struct device_link *link)
 {
 	while (refcount_dec_not_one(&link->rpm_active))
@@ -535,6 +582,19 @@ int device_links_check_suppliers(struct device *dev)
 	struct device_link *link;
 	int ret = 0;
 
+	/*
+	 * If a device is waiting for one or more suppliers (in
+	 * wait_for_suppliers list), it is not ready to probe yet. So just
+	 * return -EPROBE_DEFER without having to check the links with existing
+	 * suppliers.
+	 */
+	mutex_lock(&wfs_lock);
+	if (!list_empty(&dev->links.needs_suppliers)) {
+		mutex_unlock(&wfs_lock);
+		return -EPROBE_DEFER;
+	}
+	mutex_unlock(&wfs_lock);
+
 	device_links_write_lock();
 
 	list_for_each_entry(link, &dev->links.suppliers, c_node) {
@@ -812,6 +872,10 @@ static void device_links_purge(struct device *dev)
 {
 	struct device_link *link, *ln;
 
+	mutex_lock(&wfs_lock);
+	list_del(&dev->links.needs_suppliers);
+	mutex_unlock(&wfs_lock);
+
 	/*
 	 * Delete all of the remaining links from this device to any other
 	 * devices (either consumers or suppliers).
@@ -1673,6 +1737,7 @@ void device_initialize(struct device *dev)
 #endif
 	INIT_LIST_HEAD(&dev->links.consumers);
 	INIT_LIST_HEAD(&dev->links.suppliers);
+	INIT_LIST_HEAD(&dev->links.needs_suppliers);
 	dev->links.status = DL_DEV_NO_DRIVER;
 }
 EXPORT_SYMBOL_GPL(device_initialize);
@@ -2108,6 +2173,24 @@ int device_add(struct device *dev)
 					     BUS_NOTIFY_ADD_DEVICE, dev);
 
 	kobject_uevent(&dev->kobj, KOBJ_ADD);
+
+	/*
+	 * Check if any of the other devices (consumers) have been waiting for
+	 * this device (supplier) to be added so that they can create a device
+	 * link to it.
+	 *
+	 * This needs to happen after device_pm_add() because device_link_add()
+	 * requires the supplier be registered before it's called.
+	 *
+	 * But this also needs to happe before bus_probe_device() to make sure
+	 * waiting consumers can link to it before the driver is bound to the
+	 * device and the driver sync_state callback is called for this device.
+	 */
+	device_link_check_waiting_consumers();
+
+	if (dev->bus && dev->bus->add_links && dev->bus->add_links(dev))
+		device_link_wait_for_supplier(dev);
+
 	bus_probe_device(dev);
 	if (parent)
 		klist_add_tail(&dev->p->knode_parent,
diff --git a/include/linux/device.h b/include/linux/device.h
index 848fc71c6ba6..7f8ae7e5fc6b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -77,6 +77,11 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  *		-EPROBE_DEFER it will queue the device for deferred probing.
  * @uevent:	Called when a device is added, removed, or a few other things
  *		that generate uevents to add the environment variables.
+ * @add_links:	Called, perhaps multiple times, when a new device is added to
+ *		this bus. The function is expected to create all the device
+ *		links for the new device and return 0 if it was completed
+ *		successfully or return an error if it needs to be reattempted
+ *		in the future.
  * @probe:	Called when a new device or driver add to this bus, and callback
  *		the specific driver's probe to initial the matched device.
  * @remove:	Called when a device removed from this bus.
@@ -121,6 +126,7 @@ struct bus_type {
 
 	int (*match)(struct device *dev, struct device_driver *drv);
 	int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
+	int (*add_links)(struct device *dev);
 	int (*probe)(struct device *dev);
 	int (*remove)(struct device *dev);
 	void (*shutdown)(struct device *dev);
@@ -888,11 +894,13 @@ enum dl_dev_state {
  * struct dev_links_info - Device data related to device links.
  * @suppliers: List of links to supplier devices.
  * @consumers: List of links to consumer devices.
+ * @needs_suppliers: Hook to global list of devices waiting for suppliers.
  * @status: Driver status information.
  */
 struct dev_links_info {
 	struct list_head suppliers;
 	struct list_head consumers;
+	struct list_head needs_suppliers;
 	enum dl_dev_state status;
 };
 
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 01/11] driver core: Add support for linking devices during device addition Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  2019-07-16 23:43   ` Rob Herring
  2019-07-12 23:52 ` [PATCH v5 03/11] driver core: Add sync_state driver/bus callback Saravana Kannan
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand, Jonathan Corbet
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
	kernel-team, linux-doc

Add device-links after the devices are created (but before they are
probed) by looking at common DT bindings like clocks and
interconnects.

Automatically adding device-links for functional dependencies at the
framework level provides the following benefits:

- Optimizes device probe order and avoids the useless work of
  attempting probes of devices that will not probe successfully
  (because their suppliers aren't present or haven't probed yet).

  For example, in a commonly available mobile SoC, registering just
  one consumer device's driver at an initcall level earlier than the
  supplier device's driver causes 11 failed probe attempts before the
  consumer device probes successfully. This was with a kernel with all
  the drivers statically compiled in. This problem gets a lot worse if
  all the drivers are loaded as modules without direct symbol
  dependencies.

- Supplier devices like clock providers, interconnect providers, etc
  need to keep the resources they provide active and at a particular
  state(s) during boot up even if their current set of consumers don't
  request the resource to be active. This is because the rest of the
  consumers might not have probed yet and turning off the resource
  before all the consumers have probed could lead to a hang or
  undesired user experience.

  Some frameworks (Eg: regulator) handle this today by turning off
  "unused" resources at late_initcall_sync and hoping all the devices
  have probed by then. This is not a valid assumption for systems with
  loadable modules. Other frameworks (Eg: clock) just don't handle
  this due to the lack of a clear signal for when they can turn off
  resources. This leads to downstream hacks to handle cases like this
  that can easily be solved in the upstream kernel.

  By linking devices before they are probed, we give suppliers a clear
  count of the number of dependent consumers. Once all of the
  consumers are active, the suppliers can turn off the unused
  resources without making assumptions about the number of consumers.

By default we just add device-links to track "driver presence" (probe
succeeded) of the supplier device. If any other functionality provided
by device-links are needed, it is left to the consumer/supplier
devices to change the link when they probe.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 .../admin-guide/kernel-parameters.txt         |  5 ++
 drivers/of/platform.c                         | 57 +++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 138f6664b2e2..109b4310844f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3141,6 +3141,11 @@
 			This can be set from sysctl after boot.
 			See Documentation/sysctl/vm.txt for details.
 
+	of_devlink	[KNL] Make device links from common DT bindings. Useful
+			for optimizing probe order and making sure resources
+			aren't turned off before the consumer devices have
+			probed.
+
 	ohci1394_dma=early	[HW] enable debugging via the ohci1394 driver.
 			See Documentation/debugging-via-ohci1394.txt for more
 			info.
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 04ad312fd85b..0930f9f89571 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -509,6 +509,62 @@ int of_platform_default_populate(struct device_node *root,
 }
 EXPORT_SYMBOL_GPL(of_platform_default_populate);
 
+static int of_link_binding(struct device *dev,
+			   const char *binding, const char *cell)
+{
+	struct of_phandle_args sup_args;
+	struct platform_device *sup_dev;
+	unsigned int i = 0, links = 0;
+	u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
+
+	while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
+					   &sup_args)) {
+		i++;
+		sup_dev = of_find_device_by_node(sup_args.np);
+		of_node_put(sup_args.np);
+		if (!sup_dev)
+			continue;
+		if (device_link_add(dev, &sup_dev->dev, dl_flags))
+			links++;
+		put_device(&sup_dev->dev);
+	}
+	if (links < i)
+		return -ENODEV;
+	return 0;
+}
+
+static bool of_devlink;
+core_param(of_devlink, of_devlink, bool, 0);
+
+/*
+ * List of bindings and their cell names (use NULL if no cell names) from which
+ * device links need to be created.
+ */
+static const char * const link_bindings[] = {
+	"clocks", "#clock-cells",
+	"interconnects", "#interconnect-cells",
+};
+
+static int of_link_to_suppliers(struct device *dev)
+{
+	unsigned int i = 0;
+	bool done = true;
+
+	if (!of_devlink)
+		return 0;
+	if (unlikely(!dev->of_node))
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
+		if (of_link_binding(dev, link_bindings[i * 2],
+					link_bindings[i * 2 + 1]))
+			done = false;
+
+	if (!done)
+		return -ENODEV;
+	return 0;
+}
+
 #ifndef CONFIG_PPC
 static const struct of_device_id reserved_mem_matches[] = {
 	{ .compatible = "qcom,rmtfs-mem" },
@@ -524,6 +580,7 @@ static int __init of_platform_default_populate_init(void)
 	if (!of_have_populated_dt())
 		return -ENODEV;
 
+	platform_bus_type.add_links = of_link_to_suppliers;
 	/*
 	 * Handle certain compatibles explicitly, since we don't want to create
 	 * platform_devices for every node in /reserved-memory with a
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 03/11] driver core: Add sync_state driver/bus callback
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 01/11] driver core: Add support for linking devices during device addition Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 04/11] driver core: Add edit_links() callback for drivers Saravana Kannan
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

This sync_state driver/bus callback is called once all the consumers
of a supplier have probed successfully.

This allows the supplier device's driver/bus to sync the supplier
device's state to the software state with the guarantee that all the
consumers are actively managing the resources provided by the supplier
device.

To maintain backwards compatibility and ease transition from existing
frameworks and resource cleanup schemes, late_initcall_sync is the
earliest when the sync_state callback might be called.

There is no upper bound on the time by which the sync_state callback
has to be called. This is because if a consumer device never probes,
the supplier has to maintain its resources in the state left by the
bootloader. For example, if the bootloader leaves the display
backlight at a fixed voltage and the backlight driver is never probed,
you don't want the backlight to ever be turned off after boot up.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 39 +++++++++++++++++++++++++++++++++++++++
 drivers/of/platform.c  |  9 +++++++++
 include/linux/device.h | 19 +++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0705926d362f..8b8b812d26f1 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -46,6 +46,7 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
 /* Device links support. */
 static LIST_HEAD(wait_for_suppliers);
 static DEFINE_MUTEX(wfs_lock);
+static bool supplier_sync_state_enabled;
 
 #ifdef CONFIG_SRCU
 static DEFINE_MUTEX(device_links_lock);
@@ -614,6 +615,41 @@ int device_links_check_suppliers(struct device *dev)
 	return ret;
 }
 
+static void __device_links_supplier_sync_state(struct device *dev)
+{
+	struct device_link *link;
+
+	if (dev->state_synced)
+		return;
+
+	list_for_each_entry(link, &dev->links.consumers, s_node) {
+		if (link->flags & DL_FLAG_STATELESS)
+			continue;
+		if (link->status != DL_STATE_ACTIVE)
+			return;
+	}
+
+	if (dev->bus->sync_state)
+		dev->bus->sync_state(dev);
+	else if (dev->driver && dev->driver->sync_state)
+		dev->driver->sync_state(dev);
+
+	dev->state_synced = true;
+}
+
+int device_links_supplier_sync_state(struct device *dev, void *data)
+{
+	device_links_write_lock();
+	__device_links_supplier_sync_state(dev);
+	device_links_write_unlock();
+	return 0;
+}
+
+void device_links_supplier_sync_state_enable(void)
+{
+	supplier_sync_state_enabled = true;
+}
+
 /**
  * device_links_driver_bound - Update device links after probing its driver.
  * @dev: Device to update the links for.
@@ -658,6 +694,9 @@ void device_links_driver_bound(struct device *dev)
 
 		WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
 		WRITE_ONCE(link->status, DL_STATE_ACTIVE);
+
+		if (supplier_sync_state_enabled)
+			__device_links_supplier_sync_state(link->supplier);
 	}
 
 	dev->links.status = DL_DEV_DRIVER_BOUND;
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 0930f9f89571..4d12d6658999 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -601,6 +601,15 @@ static int __init of_platform_default_populate_init(void)
 	return 0;
 }
 arch_initcall_sync(of_platform_default_populate_init);
+
+static int __init of_platform_sync_state_init(void)
+{
+	device_links_supplier_sync_state_enable();
+	bus_for_each_dev(&platform_bus_type, NULL, NULL,
+			 device_links_supplier_sync_state);
+	return 0;
+}
+late_initcall_sync(of_platform_sync_state_init);
 #endif
 
 int of_platform_device_destroy(struct device *dev, void *data)
diff --git a/include/linux/device.h b/include/linux/device.h
index 7f8ae7e5fc6b..4a0db34ae650 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -84,6 +84,13 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  *		in the future.
  * @probe:	Called when a new device or driver add to this bus, and callback
  *		the specific driver's probe to initial the matched device.
+ * @sync_state:	Called to sync device state to software state after all the
+ *		state tracking consumers linked to this device (present at
+ *		the time of late_initcall) have successfully bound to a
+ *		driver. If the device has no consumers, this function will
+ *		be called at late_initcall_sync level. If the device has
+ *		consumers that are never bound to a driver, this function
+ *		will never get called until they do.
  * @remove:	Called when a device removed from this bus.
  * @shutdown:	Called at shut-down time to quiesce the device.
  *
@@ -128,6 +135,7 @@ struct bus_type {
 	int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
 	int (*add_links)(struct device *dev);
 	int (*probe)(struct device *dev);
+	void (*sync_state)(struct device *dev);
 	int (*remove)(struct device *dev);
 	void (*shutdown)(struct device *dev);
 
@@ -257,6 +265,13 @@ enum probe_type {
  * @probe:	Called to query the existence of a specific device,
  *		whether this driver can work with it, and bind the driver
  *		to a specific device.
+ * @sync_state:	Called to sync device state to software state after all the
+ *		state tracking consumers linked to this device (present at
+ *		the time of late_initcall) have successfully bound to a
+ *		driver. If the device has no consumers, this function will
+ *		be called at late_initcall_sync level. If the device has
+ *		consumers that are never bound to a driver, this function
+ *		will never get called until they do.
  * @remove:	Called when the device is removed from the system to
  *		unbind a device from this driver.
  * @shutdown:	Called at shut-down time to quiesce the device.
@@ -294,6 +309,7 @@ struct device_driver {
 	const struct acpi_device_id	*acpi_match_table;
 
 	int (*probe) (struct device *dev);
+	void (*sync_state)(struct device *dev);
 	int (*remove) (struct device *dev);
 	void (*shutdown) (struct device *dev);
 	int (*suspend) (struct device *dev, pm_message_t state);
@@ -1065,6 +1081,7 @@ struct device {
 	bool			offline_disabled:1;
 	bool			offline:1;
 	bool			of_node_reused:1;
+	bool			state_synced:1;
 #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
     defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
     defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
@@ -1404,6 +1421,8 @@ struct device_link *device_link_add(struct device *consumer,
 				    struct device *supplier, u32 flags);
 void device_link_del(struct device_link *link);
 void device_link_remove(void *consumer, struct device *supplier);
+int device_links_supplier_sync_state(struct device *dev, void *data);
+void device_links_supplier_sync_state_enable(void);
 
 #ifndef dev_fmt
 #define dev_fmt(fmt) fmt
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 04/11] driver core: Add edit_links() callback for drivers
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (2 preceding siblings ...)
  2019-07-12 23:52 ` [PATCH v5 03/11] driver core: Add sync_state driver/bus callback Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 05/11] driver core: Add APIs to pause/resume sync state callbacks Saravana Kannan
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

The driver core/bus adding dependencies by default makes sure that
suppliers don't sync the hardware state with software state before all the
consumers have their drivers loaded (if they are modules) and are probed.

However, when the bus incorrectly adds dependencies that it shouldn't have
added, the devices might never probe.

For example, if device-C is a consumer of device-S and they have phandles
to each other in DT, the following could happen:

1.  Device-S get added first.
2.  The bus add_links() callback will (incorrectly) try to link it as
    a consumer of device-C.
3.  Since device-C isn't present, device-S will be put in
    "waiting-for-supplier" list.
4.  Device-C gets added next.
5.  All devices in "waiting-for-supplier" list are retried for linking.
6.  Device-S gets linked as consumer to Device-C.
7.  The bus add_links() callback will (correctly) try to link it as
    a consumer of device-S.
8.  This isn't allowed because it would create a cyclic device links.

So neither devices will get probed since the supplier is dependent on a
consumer that'll never probe (because it can't get resources from the
supplier).

Without this patch, things stay in this broken state. However, with this
patch, the execution will continue like this:

9.  Device-C's driver is loaded.
10. Device-C's driver removes Device-S as a consumer of Device-C.
11. Device-C's driver adds Device-C as a consumer of Device-S.
12. Device-S probes.
13. Device-S sync_state() isn't called because Device-C hasn't probed yet.
14. Device-C probes.
15. Device-S's sync_state() callback is called.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 24 ++++++++++++++++++++++--
 drivers/base/dd.c      | 29 +++++++++++++++++++++++++++++
 include/linux/device.h | 18 ++++++++++++++++++
 3 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8b8b812d26f1..dce97b5f3536 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -423,6 +423,19 @@ static void device_link_wait_for_supplier(struct device *consumer)
 	mutex_unlock(&wfs_lock);
 }
 
+/**
+ * device_link_remove_from_wfs - Unmark device as waiting for supplier
+ * @consumer: Consumer device
+ *
+ * Unmark the consumer device as waiting for suppliers to become available.
+ */
+void device_link_remove_from_wfs(struct device *consumer)
+{
+	mutex_lock(&wfs_lock);
+	list_del_init(&consumer->links.needs_suppliers);
+	mutex_unlock(&wfs_lock);
+}
+
 /**
  * device_link_check_waiting_consumers - Try to unmark waiting consumers
  *
@@ -440,12 +453,19 @@ static void device_link_wait_for_supplier(struct device *consumer)
 static void device_link_check_waiting_consumers(void)
 {
 	struct device *dev, *tmp;
+	int ret;
 
 	mutex_lock(&wfs_lock);
 	list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
-				 links.needs_suppliers)
-		if (!dev->bus->add_links(dev))
+				 links.needs_suppliers) {
+		ret = 0;
+		if (dev->has_edit_links)
+			ret = driver_edit_links(dev);
+		else if (dev->bus->add_links)
+			ret = dev->bus->add_links(dev);
+		if (!ret)
 			list_del_init(&dev->links.needs_suppliers);
+	}
 	mutex_unlock(&wfs_lock);
 }
 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 0df9b4461766..842fc7b704f9 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -659,6 +659,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
+	if (drv->edit_links) {
+		if (drv->edit_links(dev))
+			dev->has_edit_links = true;
+		else
+			device_link_remove_from_wfs(dev);
+	}
 	pm_runtime_get_suppliers(dev);
 	if (dev->parent)
 		pm_runtime_get_sync(dev->parent);
@@ -747,6 +753,29 @@ struct device_attach_data {
 	bool have_async;
 };
 
+static int __driver_edit_links(struct device_driver *drv, void *data)
+{
+	struct device *dev = data;
+
+	if (!drv->edit_links)
+		return 0;
+
+	if (driver_match_device(drv, dev) <= 0)
+		return 0;
+
+	return drv->edit_links(dev);
+}
+
+int driver_edit_links(struct device *dev)
+{
+	int ret;
+
+	device_lock(dev);
+	ret = bus_for_each_drv(dev->bus, NULL, dev, __driver_edit_links);
+	device_unlock(dev);
+	return ret;
+}
+
 static int __device_attach_driver(struct device_driver *drv, void *_data)
 {
 	struct device_attach_data *data = _data;
diff --git a/include/linux/device.h b/include/linux/device.h
index 4a0db34ae650..d3c9e70052d8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -262,6 +262,20 @@ enum probe_type {
  * @probe_type:	Type of the probe (synchronous or asynchronous) to use.
  * @of_match_table: The open firmware table.
  * @acpi_match_table: The ACPI match table.
+ * @edit_links:	Called to allow a matched driver to edit the device links the
+ *		bus might have added incorrectly. This will be useful to handle
+ *		cases where the bus incorrectly adds functional dependencies
+ *		that aren't true or tries to create cyclic dependencies. But
+ *		doesn't correctly handle functional dependencies that are
+ *		missed by the bus as the supplier's sync_state might get to
+ *		execute before the driver for a missing consumer is loaded and
+ *		gets to edit the device links for the consumer.
+ *
+ *		This function might be called multiple times after a new device
+ *		is added.  The function is expected to create all the device
+ *		links for the new device and return 0 if it was completed
+ *		successfully or return an error if it needs to be reattempted
+ *		in the future.
  * @probe:	Called to query the existence of a specific device,
  *		whether this driver can work with it, and bind the driver
  *		to a specific device.
@@ -308,6 +322,7 @@ struct device_driver {
 	const struct of_device_id	*of_match_table;
 	const struct acpi_device_id	*acpi_match_table;
 
+	int (*edit_links)(struct device *dev);
 	int (*probe) (struct device *dev);
 	void (*sync_state)(struct device *dev);
 	int (*remove) (struct device *dev);
@@ -1082,6 +1097,7 @@ struct device {
 	bool			offline:1;
 	bool			of_node_reused:1;
 	bool			state_synced:1;
+	bool			has_edit_links:1;
 #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
     defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
     defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
@@ -1331,6 +1347,7 @@ extern int  __must_check device_attach(struct device *dev);
 extern int __must_check driver_attach(struct device_driver *drv);
 extern void device_initial_probe(struct device *dev);
 extern int __must_check device_reprobe(struct device *dev);
+extern int driver_edit_links(struct device *dev);
 
 extern bool device_is_bound(struct device *dev);
 
@@ -1423,6 +1440,7 @@ void device_link_del(struct device_link *link);
 void device_link_remove(void *consumer, struct device *supplier);
 int device_links_supplier_sync_state(struct device *dev, void *data);
 void device_links_supplier_sync_state_enable(void);
+void device_link_remove_from_wfs(struct device *consumer);
 
 #ifndef dev_fmt
 #define dev_fmt(fmt) fmt
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 05/11] driver core: Add APIs to pause/resume sync state callbacks
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (3 preceding siblings ...)
  2019-07-12 23:52 ` [PATCH v5 04/11] driver core: Add edit_links() callback for drivers Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 06/11] of/platform: Pause/resume sync state in of_platform_populate() Saravana Kannan
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

When multiple devices are added after kernel init, some suppliers could be
added before their consumer devices get added. In these instances, the
supplier devices could get their sync_state callback called right after
they probe because the consumers haven't had a chance to create device
links to the suppliers.

This change adds APIs to pause/resume sync state callbacks so that when
multiple devices are added, their sync_state callback evaluation can be
postponed to happen after all of them are added.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 40 +++++++++++++++++++++++++++++++++-------
 drivers/of/platform.c  |  5 ++---
 include/linux/device.h |  6 ++++--
 3 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index dce97b5f3536..b03e679faea4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -46,7 +46,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
 /* Device links support. */
 static LIST_HEAD(wait_for_suppliers);
 static DEFINE_MUTEX(wfs_lock);
-static bool supplier_sync_state_enabled;
+static LIST_HEAD(deferred_sync);
+static unsigned int supplier_sync_state_disabled;
 
 #ifdef CONFIG_SRCU
 static DEFINE_MUTEX(device_links_lock);
@@ -657,17 +658,38 @@ static void __device_links_supplier_sync_state(struct device *dev)
 	dev->state_synced = true;
 }
 
-int device_links_supplier_sync_state(struct device *dev, void *data)
+void device_links_supplier_sync_state_pause(void)
 {
 	device_links_write_lock();
-	__device_links_supplier_sync_state(dev);
+	supplier_sync_state_disabled++;
 	device_links_write_unlock();
-	return 0;
 }
 
-void device_links_supplier_sync_state_enable(void)
+void device_links_supplier_sync_state_resume(void)
 {
-	supplier_sync_state_enabled = true;
+	struct device *dev, *tmp;
+
+	device_links_write_lock();
+	if (!supplier_sync_state_disabled) {
+		WARN(true, "Unmatched sync_state pause/resume!");
+		goto out;
+	}
+	supplier_sync_state_disabled--;
+	if (supplier_sync_state_disabled)
+		goto out;
+
+	list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_sync) {
+		__device_links_supplier_sync_state(dev);
+		list_del_init(&dev->links.defer_sync);
+	}
+out:
+	device_links_write_unlock();
+}
+
+static void __device_links_supplier_defer_sync(struct device *sup)
+{
+	if (list_empty(&sup->links.defer_sync))
+		list_add_tail(&sup->links.defer_sync, &deferred_sync);
 }
 
 /**
@@ -715,7 +737,9 @@ void device_links_driver_bound(struct device *dev)
 		WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
 		WRITE_ONCE(link->status, DL_STATE_ACTIVE);
 
-		if (supplier_sync_state_enabled)
+		if (supplier_sync_state_disabled)
+			__device_links_supplier_defer_sync(link->supplier);
+		else
 			__device_links_supplier_sync_state(link->supplier);
 	}
 
@@ -826,6 +850,7 @@ void device_links_driver_cleanup(struct device *dev)
 		WRITE_ONCE(link->status, DL_STATE_DORMANT);
 	}
 
+	list_del_init(&dev->links.defer_sync);
 	__device_links_no_driver(dev);
 
 	device_links_write_unlock();
@@ -1797,6 +1822,7 @@ void device_initialize(struct device *dev)
 	INIT_LIST_HEAD(&dev->links.consumers);
 	INIT_LIST_HEAD(&dev->links.suppliers);
 	INIT_LIST_HEAD(&dev->links.needs_suppliers);
+	INIT_LIST_HEAD(&dev->links.defer_sync);
 	dev->links.status = DL_DEV_NO_DRIVER;
 }
 EXPORT_SYMBOL_GPL(device_initialize);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 4d12d6658999..56b718f09929 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -581,6 +581,7 @@ static int __init of_platform_default_populate_init(void)
 		return -ENODEV;
 
 	platform_bus_type.add_links = of_link_to_suppliers;
+	device_links_supplier_sync_state_pause();
 	/*
 	 * Handle certain compatibles explicitly, since we don't want to create
 	 * platform_devices for every node in /reserved-memory with a
@@ -604,9 +605,7 @@ arch_initcall_sync(of_platform_default_populate_init);
 
 static int __init of_platform_sync_state_init(void)
 {
-	device_links_supplier_sync_state_enable();
-	bus_for_each_dev(&platform_bus_type, NULL, NULL,
-			 device_links_supplier_sync_state);
+	device_links_supplier_sync_state_resume();
 	return 0;
 }
 late_initcall_sync(of_platform_sync_state_init);
diff --git a/include/linux/device.h b/include/linux/device.h
index d3c9e70052d8..0ea28cb8c77e 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -926,12 +926,14 @@ enum dl_dev_state {
  * @suppliers: List of links to supplier devices.
  * @consumers: List of links to consumer devices.
  * @needs_suppliers: Hook to global list of devices waiting for suppliers.
+ * @defer_sync: Hook to global list of devices that have deferred sync_state.
  * @status: Driver status information.
  */
 struct dev_links_info {
 	struct list_head suppliers;
 	struct list_head consumers;
 	struct list_head needs_suppliers;
+	struct list_head defer_sync;
 	enum dl_dev_state status;
 };
 
@@ -1438,8 +1440,8 @@ struct device_link *device_link_add(struct device *consumer,
 				    struct device *supplier, u32 flags);
 void device_link_del(struct device_link *link);
 void device_link_remove(void *consumer, struct device *supplier);
-int device_links_supplier_sync_state(struct device *dev, void *data);
-void device_links_supplier_sync_state_enable(void);
+void device_links_supplier_sync_state_pause(void);
+void device_links_supplier_sync_state_resume(void);
 void device_link_remove_from_wfs(struct device *consumer);
 
 #ifndef dev_fmt
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 06/11] of/platform: Pause/resume sync state in of_platform_populate()
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (4 preceding siblings ...)
  2019-07-12 23:52 ` [PATCH v5 05/11] driver core: Add APIs to pause/resume sync state callbacks Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 07/11] of/platform: Sanity check DT bindings before creating device links Saravana Kannan
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

When multiple child devices are populated using of_platform_populate()
after kernel init, there could be supplier-consumer dependencies between
the child devices.

Wait for all the devices to be added and linked before calling sync_state()
on all the suppliers.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/platform.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 56b718f09929..dba962a0ee50 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -486,6 +486,7 @@ int of_platform_populate(struct device_node *root,
 	pr_debug("%s()\n", __func__);
 	pr_debug(" starting at: %pOF\n", root);
 
+	device_links_supplier_sync_state_pause();
 	for_each_child_of_node(root, child) {
 		rc = of_platform_bus_create(child, matches, lookup, parent, true);
 		if (rc) {
@@ -493,6 +494,8 @@ int of_platform_populate(struct device_node *root,
 			break;
 		}
 	}
+	device_links_supplier_sync_state_resume();
+
 	of_node_set_flag(root, OF_POPULATED_BUS);
 
 	of_node_put(root);
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 07/11] of/platform: Sanity check DT bindings before creating device links
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (5 preceding siblings ...)
  2019-07-12 23:52 ` [PATCH v5 06/11] of/platform: Pause/resume sync state in of_platform_populate() Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 08/11] of/platform: Make sure supplier DT node is device when " Saravana Kannan
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

If a common DT binding is pointing to a child DT node of a particular
parent DT node, don't add device links for such DT references. This is
because, by definition, a child node can't be a functional dependency for
the parent node.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/platform.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index dba962a0ee50..98414ba53b1f 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -512,6 +512,19 @@ int of_platform_default_populate(struct device_node *root,
 }
 EXPORT_SYMBOL_GPL(of_platform_default_populate);
 
+bool of_link_is_valid(struct device_node *con, struct device_node *sup)
+{
+	of_node_get(sup);
+	while (sup) {
+		if (sup == con) {
+			of_node_put(sup);
+			return false;
+		}
+		sup = of_get_next_parent(sup);
+	}
+	return true;
+}
+
 static int of_link_binding(struct device *dev,
 			   const char *binding, const char *cell)
 {
@@ -522,6 +535,10 @@ static int of_link_binding(struct device *dev,
 
 	while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
 					   &sup_args)) {
+		if (!of_link_is_valid(dev->of_node, sup_args.np)) {
+			of_node_put(sup_args.np);
+			continue;
+		}
 		i++;
 		sup_dev = of_find_device_by_node(sup_args.np);
 		of_node_put(sup_args.np);
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 08/11] of/platform: Make sure supplier DT node is device when creating device links
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (6 preceding siblings ...)
  2019-07-12 23:52 ` [PATCH v5 07/11] of/platform: Sanity check DT bindings before creating device links Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 09/11] of/platform: Create device links for all child-supplier depencencies Saravana Kannan
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

While most phandle references in common bindings point to the supplier
device node, there are also common bindings where the phandle can
pointing to a child node of the supplier device node.

Therefore, when trying to find the supplier device that corresponds to a
supplier phandle, we need to make sure we are using the supplier's
device node. Otherwise, we'll never find the supplier device.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/platform.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 98414ba53b1f..cf8625abe30c 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -529,19 +529,33 @@ static int of_link_binding(struct device *dev,
 			   const char *binding, const char *cell)
 {
 	struct of_phandle_args sup_args;
+	struct device_node *sup_np;
 	struct platform_device *sup_dev;
 	unsigned int i = 0, links = 0;
 	u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
 
 	while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
 					   &sup_args)) {
-		if (!of_link_is_valid(dev->of_node, sup_args.np)) {
-			of_node_put(sup_args.np);
+		sup_np = sup_args.np;
+		/*
+		 * Since we are trying to create device links, we need to find
+		 * the actual device node that owns this supplier phandle.
+		 * Often times it's the same node, but sometimes it can be one
+		 * of the parents. So walk up the parent till you find a
+		 * device.
+		 */
+		while (sup_np && !of_find_property(sup_np, "compatible", NULL))
+			sup_np = of_get_next_parent(sup_np);
+		if (!sup_np)
+			continue;
+
+		if (!of_link_is_valid(dev->of_node, sup_np)) {
+			of_node_put(sup_np);
 			continue;
 		}
 		i++;
-		sup_dev = of_find_device_by_node(sup_args.np);
-		of_node_put(sup_args.np);
+		sup_dev = of_find_device_by_node(sup_np);
+		of_node_put(sup_np);
 		if (!sup_dev)
 			continue;
 		if (device_link_add(dev, &sup_dev->dev, dl_flags))
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 09/11] of/platform: Create device links for all child-supplier depencencies
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (7 preceding siblings ...)
  2019-07-12 23:52 ` [PATCH v5 08/11] of/platform: Make sure supplier DT node is device when " Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 10/11] of/platform: Add functional dependency link from DT regulator bindings Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 11/11] of/platform: Don't create device links default busses Saravana Kannan
  10 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

A parent device can have child devices that it adds when it probes. But
this probing of the parent device can happen way after kernel init is done
-- for example, when the parent device's driver is loaded as a module.

In such cases, if the child devices depend on a supplier in the system, we
need to make sure the supplier gets the sync_state() callback only after
these child devices are added and probed.

To achieve this, when creating device links for a device by looking at its
DT node, don't just look at DT references at the top node level. Look at DT
references in all the descendant nodes too and create device links from the
ancestor device to all these supplier devices.

This way, when the parent device probes and adds child devices, the child
devices can then create their own device links to the suppliers and further
delay the supplier's sync_state() callback to after the child devices are
probed.

Example:
In this illustration, -> denotes DT references and indentation
represents child status.

Device node A
	Device node B -> D
	Device node C -> B, D

Device node D

Assume all these devices have their drivers loaded as modules.

Without this patch, this is the sequence of events:
1. D is added.
2. A is added.
3. Device D probes.
4. Device D gets its sync_state() callback.
5. Device B and C might malfunction because their resources got
   altered/turned off before they can make active requests for them.

With this patch, this is the sequence of events:
1. D is added.
2. A is added and creates device links to D.
3. Device link from A to B is not added because A is a parent of B.
4. Device D probes.
5. Device D does not get it's sync_state() callback because consumer A
   hasn't probed yet.
5. Device A probes.
5. a. Devices B and C are added.
5. b. Device links from B and C to D are added.
5. c. Device A's probe completes.
6. Device D does not get it's sync_state() callback because consumer A
   has probed but consumers B and C haven't probed yet.
7. Device B and C probe.
8. Device D gets it's sync_state() callback because all its consumers
   have probed.
9. None of the devices malfunction.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/platform.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index cf8625abe30c..6523d07ef2d7 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -525,7 +525,7 @@ bool of_link_is_valid(struct device_node *con, struct device_node *sup)
 	return true;
 }
 
-static int of_link_binding(struct device *dev,
+static int of_link_binding(struct device *dev, struct device_node *con_np,
 			   const char *binding, const char *cell)
 {
 	struct of_phandle_args sup_args;
@@ -534,7 +534,7 @@ static int of_link_binding(struct device *dev,
 	unsigned int i = 0, links = 0;
 	u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
 
-	while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
+	while (!of_parse_phandle_with_args(con_np, binding, cell, i,
 					   &sup_args)) {
 		sup_np = sup_args.np;
 		/*
@@ -579,26 +579,37 @@ static const char * const link_bindings[] = {
 	"interconnects", "#interconnect-cells",
 };
 
-static int of_link_to_suppliers(struct device *dev)
+static int __of_link_to_suppliers(struct device *dev,
+				  struct device_node *con_np)
 {
 	unsigned int i = 0;
 	bool done = true;
-
-	if (!of_devlink)
-		return 0;
-	if (unlikely(!dev->of_node))
-		return 0;
+	struct device_node *child;
 
 	for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
-		if (of_link_binding(dev, link_bindings[i * 2],
+		if (of_link_binding(dev, con_np, link_bindings[i * 2],
 					link_bindings[i * 2 + 1]))
 			done = false;
 
+	for_each_child_of_node(con_np, child)
+		if (__of_link_to_suppliers(dev, child))
+			done = false;
+
 	if (!done)
 		return -ENODEV;
 	return 0;
 }
 
+static int of_link_to_suppliers(struct device *dev)
+{
+	if (!of_devlink)
+		return 0;
+	if (unlikely(!dev->of_node))
+		return 0;
+
+	return __of_link_to_suppliers(dev, dev->of_node);
+}
+
 #ifndef CONFIG_PPC
 static const struct of_device_id reserved_mem_matches[] = {
 	{ .compatible = "qcom,rmtfs-mem" },
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 10/11] of/platform: Add functional dependency link from DT regulator bindings
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (8 preceding siblings ...)
  2019-07-12 23:52 ` [PATCH v5 09/11] of/platform: Create device links for all child-supplier depencencies Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  2019-07-12 23:52 ` [PATCH v5 11/11] of/platform: Don't create device links default busses Saravana Kannan
  10 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

Similar to creating functional dependency links from clock and
interconnect DT bindings, also create functional dependency links from
regulator DT bindings.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/platform.c | 83 ++++++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 32 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 6523d07ef2d7..9f3b7e1801bc 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -525,46 +525,50 @@ bool of_link_is_valid(struct device_node *con, struct device_node *sup)
 	return true;
 }
 
-static int of_link_binding(struct device *dev, struct device_node *con_np,
-			   const char *binding, const char *cell)
+static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
 {
-	struct of_phandle_args sup_args;
-	struct device_node *sup_np;
 	struct platform_device *sup_dev;
-	unsigned int i = 0, links = 0;
 	u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
+	int ret = 0;
 
-	while (!of_parse_phandle_with_args(con_np, binding, cell, i,
-					   &sup_args)) {
-		sup_np = sup_args.np;
-		/*
-		 * Since we are trying to create device links, we need to find
-		 * the actual device node that owns this supplier phandle.
-		 * Often times it's the same node, but sometimes it can be one
-		 * of the parents. So walk up the parent till you find a
-		 * device.
-		 */
-		while (sup_np && !of_find_property(sup_np, "compatible", NULL))
-			sup_np = of_get_next_parent(sup_np);
-		if (!sup_np)
-			continue;
+	/*
+	 * Since we are trying to create device links, we need to find
+	 * the actual device node that owns this supplier phandle.
+	 * Often times it's the same node, but sometimes it can be one
+	 * of the parents. So walk up the parent till you find a
+	 * device.
+	 */
+	while (sup_np && !of_find_property(sup_np, "compatible", NULL))
+		sup_np = of_get_next_parent(sup_np);
+	if (!sup_np)
+		return 0;
 
-		if (!of_link_is_valid(dev->of_node, sup_np)) {
-			of_node_put(sup_np);
-			continue;
-		}
-		i++;
-		sup_dev = of_find_device_by_node(sup_np);
+	if (!of_link_is_valid(dev->of_node, sup_np)) {
 		of_node_put(sup_np);
-		if (!sup_dev)
-			continue;
-		if (device_link_add(dev, &sup_dev->dev, dl_flags))
-			links++;
-		put_device(&sup_dev->dev);
+		return 0;
 	}
-	if (links < i)
+	sup_dev = of_find_device_by_node(sup_np);
+	of_node_put(sup_np);
+	if (!sup_dev)
 		return -ENODEV;
-	return 0;
+	if (!device_link_add(dev, &sup_dev->dev, dl_flags))
+		ret = -ENODEV;
+	put_device(&sup_dev->dev);
+	return ret;
+}
+
+static int of_link_binding(struct device *dev, struct device_node *con_np,
+			   const char *binding, const char *cell)
+{
+	struct of_phandle_args sup_args;
+	unsigned int i = 0;
+	bool done = true;
+
+	while (!of_parse_phandle_with_args(con_np, binding, cell, i++,
+					   &sup_args))
+		if (of_link_to_phandle(dev, sup_args.np))
+			done = false;
+	return done ? 0 : -ENODEV;
 }
 
 static bool of_devlink;
@@ -579,18 +583,33 @@ static const char * const link_bindings[] = {
 	"interconnects", "#interconnect-cells",
 };
 
+#define REG_SUFFIX	"-supply"
 static int __of_link_to_suppliers(struct device *dev,
 				  struct device_node *con_np)
 {
 	unsigned int i = 0;
 	bool done = true;
 	struct device_node *child;
+	struct property *p;
+	unsigned int len, reg_len;
 
 	for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
 		if (of_link_binding(dev, con_np, link_bindings[i * 2],
 					link_bindings[i * 2 + 1]))
 			done = false;
 
+	reg_len = strlen(REG_SUFFIX);
+	for_each_property_of_node(con_np, p) {
+		len = strlen(p->name);
+		if (len <= reg_len)
+			continue;
+		if (strcmp(p->name + len - reg_len, REG_SUFFIX))
+			continue;
+		if (of_link_to_phandle(dev,
+				of_parse_phandle(con_np, p->name, 0)))
+			done = false;
+	}
+
 	for_each_child_of_node(con_np, child)
 		if (__of_link_to_suppliers(dev, child))
 			done = false;
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 11/11] of/platform: Don't create device links default busses
  2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (9 preceding siblings ...)
  2019-07-12 23:52 ` [PATCH v5 10/11] of/platform: Add functional dependency link from DT regulator bindings Saravana Kannan
@ 2019-07-12 23:52 ` Saravana Kannan
  10 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-12 23:52 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: Saravana Kannan, devicetree, linux-kernel, David Collins, kernel-team

Default busses also have devices created for them. But there's no point
in creating device links for them. It's especially wasteful as it'll
cause the traversal of the entire device tree and also spend a lot of
time checking and figuring out that creating those links isn't allowed.
So check for default busses and skip trying to create device links for
them.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/platform.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 9f3b7e1801bc..b02dbaa01bfe 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -593,6 +593,9 @@ static int __of_link_to_suppliers(struct device *dev,
 	struct property *p;
 	unsigned int len, reg_len;
 
+	if (of_match_node(of_default_bus_match_table, con_np))
+		return 0;
+
 	for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
 		if (of_link_binding(dev, con_np, link_bindings[i * 2],
 					link_bindings[i * 2 + 1]))
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings
  2019-07-12 23:52 ` [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings Saravana Kannan
@ 2019-07-16 23:43   ` Rob Herring
  2019-07-16 23:53     ` Saravana Kannan
  0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2019-07-16 23:43 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand, Jonathan Corbet, devicetree, linux-kernel,
	David Collins, Android Kernel Team, Linux Doc Mailing List

On Fri, Jul 12, 2019 at 5:52 PM Saravana Kannan <saravanak@google.com> wrote:
>
> Add device-links after the devices are created (but before they are
> probed) by looking at common DT bindings like clocks and
> interconnects.
>
> Automatically adding device-links for functional dependencies at the
> framework level provides the following benefits:
>
> - Optimizes device probe order and avoids the useless work of
>   attempting probes of devices that will not probe successfully
>   (because their suppliers aren't present or haven't probed yet).
>
>   For example, in a commonly available mobile SoC, registering just
>   one consumer device's driver at an initcall level earlier than the
>   supplier device's driver causes 11 failed probe attempts before the
>   consumer device probes successfully. This was with a kernel with all
>   the drivers statically compiled in. This problem gets a lot worse if
>   all the drivers are loaded as modules without direct symbol
>   dependencies.
>
> - Supplier devices like clock providers, interconnect providers, etc
>   need to keep the resources they provide active and at a particular
>   state(s) during boot up even if their current set of consumers don't
>   request the resource to be active. This is because the rest of the
>   consumers might not have probed yet and turning off the resource
>   before all the consumers have probed could lead to a hang or
>   undesired user experience.
>
>   Some frameworks (Eg: regulator) handle this today by turning off
>   "unused" resources at late_initcall_sync and hoping all the devices
>   have probed by then. This is not a valid assumption for systems with
>   loadable modules. Other frameworks (Eg: clock) just don't handle
>   this due to the lack of a clear signal for when they can turn off
>   resources. This leads to downstream hacks to handle cases like this
>   that can easily be solved in the upstream kernel.
>
>   By linking devices before they are probed, we give suppliers a clear
>   count of the number of dependent consumers. Once all of the
>   consumers are active, the suppliers can turn off the unused
>   resources without making assumptions about the number of consumers.
>
> By default we just add device-links to track "driver presence" (probe
> succeeded) of the supplier device. If any other functionality provided
> by device-links are needed, it is left to the consumer/supplier
> devices to change the link when they probe.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  5 ++
>  drivers/of/platform.c                         | 57 +++++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 138f6664b2e2..109b4310844f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3141,6 +3141,11 @@
>                         This can be set from sysctl after boot.
>                         See Documentation/sysctl/vm.txt for details.
>
> +       of_devlink      [KNL] Make device links from common DT bindings. Useful
> +                       for optimizing probe order and making sure resources
> +                       aren't turned off before the consumer devices have
> +                       probed.
> +
>         ohci1394_dma=early      [HW] enable debugging via the ohci1394 driver.
>                         See Documentation/debugging-via-ohci1394.txt for more
>                         info.
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 04ad312fd85b..0930f9f89571 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -509,6 +509,62 @@ int of_platform_default_populate(struct device_node *root,
>  }
>  EXPORT_SYMBOL_GPL(of_platform_default_populate);
>
> +static int of_link_binding(struct device *dev,
> +                          const char *binding, const char *cell)
> +{
> +       struct of_phandle_args sup_args;
> +       struct platform_device *sup_dev;
> +       unsigned int i = 0, links = 0;
> +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> +
> +       while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
> +                                          &sup_args)) {
> +               i++;
> +               sup_dev = of_find_device_by_node(sup_args.np);
> +               of_node_put(sup_args.np);
> +               if (!sup_dev)
> +                       continue;
> +               if (device_link_add(dev, &sup_dev->dev, dl_flags))
> +                       links++;
> +               put_device(&sup_dev->dev);
> +       }
> +       if (links < i)
> +               return -ENODEV;
> +       return 0;
> +}
> +
> +static bool of_devlink;
> +core_param(of_devlink, of_devlink, bool, 0);
> +
> +/*
> + * List of bindings and their cell names (use NULL if no cell names) from which
> + * device links need to be created.
> + */
> +static const char * const link_bindings[] = {
> +       "clocks", "#clock-cells",
> +       "interconnects", "#interconnect-cells",
> +};
> +
> +static int of_link_to_suppliers(struct device *dev)
> +{
> +       unsigned int i = 0;
> +       bool done = true;
> +
> +       if (!of_devlink)
> +               return 0;
> +       if (unlikely(!dev->of_node))
> +               return 0;
> +
> +       for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
> +               if (of_link_binding(dev, link_bindings[i * 2],
> +                                       link_bindings[i * 2 + 1]))
> +                       done = false;

Given the pending addition of regulators I think this should be
structured a bit differently so that we abstract out the matching and
phandle look-up so there's a clean separation of binding specifics.
It's kind of messy with 2 patterns to parse already and if we added a
3rd? I would iterate over the properties as you do for regulators in
both cases and for each property call a binding specific match
function. The common pattern can of course be a common function. Let
me know if that makes sense. If not I can try to flesh it out some
more.

Rob

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings
  2019-07-16 23:43   ` Rob Herring
@ 2019-07-16 23:53     ` Saravana Kannan
  2019-07-17 14:35       ` Rob Herring
  0 siblings, 1 reply; 16+ messages in thread
From: Saravana Kannan @ 2019-07-16 23:53 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand, Jonathan Corbet,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, David Collins, Android Kernel Team,
	Linux Doc Mailing List

On Tue, Jul 16, 2019 at 4:43 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Fri, Jul 12, 2019 at 5:52 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Add device-links after the devices are created (but before they are
> > probed) by looking at common DT bindings like clocks and
> > interconnects.
> >
> > Automatically adding device-links for functional dependencies at the
> > framework level provides the following benefits:
> >
> > - Optimizes device probe order and avoids the useless work of
> >   attempting probes of devices that will not probe successfully
> >   (because their suppliers aren't present or haven't probed yet).
> >
> >   For example, in a commonly available mobile SoC, registering just
> >   one consumer device's driver at an initcall level earlier than the
> >   supplier device's driver causes 11 failed probe attempts before the
> >   consumer device probes successfully. This was with a kernel with all
> >   the drivers statically compiled in. This problem gets a lot worse if
> >   all the drivers are loaded as modules without direct symbol
> >   dependencies.
> >
> > - Supplier devices like clock providers, interconnect providers, etc
> >   need to keep the resources they provide active and at a particular
> >   state(s) during boot up even if their current set of consumers don't
> >   request the resource to be active. This is because the rest of the
> >   consumers might not have probed yet and turning off the resource
> >   before all the consumers have probed could lead to a hang or
> >   undesired user experience.
> >
> >   Some frameworks (Eg: regulator) handle this today by turning off
> >   "unused" resources at late_initcall_sync and hoping all the devices
> >   have probed by then. This is not a valid assumption for systems with
> >   loadable modules. Other frameworks (Eg: clock) just don't handle
> >   this due to the lack of a clear signal for when they can turn off
> >   resources. This leads to downstream hacks to handle cases like this
> >   that can easily be solved in the upstream kernel.
> >
> >   By linking devices before they are probed, we give suppliers a clear
> >   count of the number of dependent consumers. Once all of the
> >   consumers are active, the suppliers can turn off the unused
> >   resources without making assumptions about the number of consumers.
> >
> > By default we just add device-links to track "driver presence" (probe
> > succeeded) of the supplier device. If any other functionality provided
> > by device-links are needed, it is left to the consumer/supplier
> > devices to change the link when they probe.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  .../admin-guide/kernel-parameters.txt         |  5 ++
> >  drivers/of/platform.c                         | 57 +++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 138f6664b2e2..109b4310844f 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -3141,6 +3141,11 @@
> >                         This can be set from sysctl after boot.
> >                         See Documentation/sysctl/vm.txt for details.
> >
> > +       of_devlink      [KNL] Make device links from common DT bindings. Useful
> > +                       for optimizing probe order and making sure resources
> > +                       aren't turned off before the consumer devices have
> > +                       probed.
> > +
> >         ohci1394_dma=early      [HW] enable debugging via the ohci1394 driver.
> >                         See Documentation/debugging-via-ohci1394.txt for more
> >                         info.
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 04ad312fd85b..0930f9f89571 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -509,6 +509,62 @@ int of_platform_default_populate(struct device_node *root,
> >  }
> >  EXPORT_SYMBOL_GPL(of_platform_default_populate);
> >
> > +static int of_link_binding(struct device *dev,
> > +                          const char *binding, const char *cell)
> > +{
> > +       struct of_phandle_args sup_args;
> > +       struct platform_device *sup_dev;
> > +       unsigned int i = 0, links = 0;
> > +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > +
> > +       while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
> > +                                          &sup_args)) {
> > +               i++;
> > +               sup_dev = of_find_device_by_node(sup_args.np);
> > +               of_node_put(sup_args.np);
> > +               if (!sup_dev)
> > +                       continue;
> > +               if (device_link_add(dev, &sup_dev->dev, dl_flags))
> > +                       links++;
> > +               put_device(&sup_dev->dev);
> > +       }
> > +       if (links < i)
> > +               return -ENODEV;
> > +       return 0;
> > +}
> > +
> > +static bool of_devlink;
> > +core_param(of_devlink, of_devlink, bool, 0);
> > +
> > +/*
> > + * List of bindings and their cell names (use NULL if no cell names) from which
> > + * device links need to be created.
> > + */
> > +static const char * const link_bindings[] = {
> > +       "clocks", "#clock-cells",
> > +       "interconnects", "#interconnect-cells",
> > +};
> > +
> > +static int of_link_to_suppliers(struct device *dev)
> > +{
> > +       unsigned int i = 0;
> > +       bool done = true;
> > +
> > +       if (!of_devlink)
> > +               return 0;
> > +       if (unlikely(!dev->of_node))
> > +               return 0;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
> > +               if (of_link_binding(dev, link_bindings[i * 2],
> > +                                       link_bindings[i * 2 + 1]))
> > +                       done = false;
>
> Given the pending addition of regulators I think this should be
> structured a bit differently so that we abstract out the matching and
> phandle look-up so there's a clean separation of binding specifics.
> It's kind of messy with 2 patterns to parse already and if we added a
> 3rd? I would iterate over the properties as you do for regulators in
> both cases and for each property call a binding specific match
> function. The common pattern can of course be a common function. Let
> me know if that makes sense. If not I can try to flesh it out some
> more.

I've added regulator support in this series and I've refactored this
code as I went along. I fully expect to squash some of the refactors
once the final result of the series is acceptable.

It's not clear to me if you got to the end of the series and still
think the final result needs to be refactored. Let me know what you
think about this towards the end of the series and I can clean it up
if you still think it needs some clean up.

-Saravana

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings
  2019-07-16 23:53     ` Saravana Kannan
@ 2019-07-17 14:35       ` Rob Herring
  2019-07-17 20:38         ` Saravana Kannan
  0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2019-07-17 14:35 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand, Jonathan Corbet,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, David Collins, Android Kernel Team,
	Linux Doc Mailing List

On Tue, Jul 16, 2019 at 5:54 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Jul 16, 2019 at 4:43 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Fri, Jul 12, 2019 at 5:52 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > Add device-links after the devices are created (but before they are
> > > probed) by looking at common DT bindings like clocks and
> > > interconnects.
> > >
> > > Automatically adding device-links for functional dependencies at the
> > > framework level provides the following benefits:
> > >
> > > - Optimizes device probe order and avoids the useless work of
> > >   attempting probes of devices that will not probe successfully
> > >   (because their suppliers aren't present or haven't probed yet).
> > >
> > >   For example, in a commonly available mobile SoC, registering just
> > >   one consumer device's driver at an initcall level earlier than the
> > >   supplier device's driver causes 11 failed probe attempts before the
> > >   consumer device probes successfully. This was with a kernel with all
> > >   the drivers statically compiled in. This problem gets a lot worse if
> > >   all the drivers are loaded as modules without direct symbol
> > >   dependencies.
> > >
> > > - Supplier devices like clock providers, interconnect providers, etc
> > >   need to keep the resources they provide active and at a particular
> > >   state(s) during boot up even if their current set of consumers don't
> > >   request the resource to be active. This is because the rest of the
> > >   consumers might not have probed yet and turning off the resource
> > >   before all the consumers have probed could lead to a hang or
> > >   undesired user experience.
> > >
> > >   Some frameworks (Eg: regulator) handle this today by turning off
> > >   "unused" resources at late_initcall_sync and hoping all the devices
> > >   have probed by then. This is not a valid assumption for systems with
> > >   loadable modules. Other frameworks (Eg: clock) just don't handle
> > >   this due to the lack of a clear signal for when they can turn off
> > >   resources. This leads to downstream hacks to handle cases like this
> > >   that can easily be solved in the upstream kernel.
> > >
> > >   By linking devices before they are probed, we give suppliers a clear
> > >   count of the number of dependent consumers. Once all of the
> > >   consumers are active, the suppliers can turn off the unused
> > >   resources without making assumptions about the number of consumers.
> > >
> > > By default we just add device-links to track "driver presence" (probe
> > > succeeded) of the supplier device. If any other functionality provided
> > > by device-links are needed, it is left to the consumer/supplier
> > > devices to change the link when they probe.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  .../admin-guide/kernel-parameters.txt         |  5 ++
> > >  drivers/of/platform.c                         | 57 +++++++++++++++++++
> > >  2 files changed, 62 insertions(+)
> > >
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 138f6664b2e2..109b4310844f 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -3141,6 +3141,11 @@
> > >                         This can be set from sysctl after boot.
> > >                         See Documentation/sysctl/vm.txt for details.
> > >
> > > +       of_devlink      [KNL] Make device links from common DT bindings. Useful
> > > +                       for optimizing probe order and making sure resources
> > > +                       aren't turned off before the consumer devices have
> > > +                       probed.
> > > +
> > >         ohci1394_dma=early      [HW] enable debugging via the ohci1394 driver.
> > >                         See Documentation/debugging-via-ohci1394.txt for more
> > >                         info.
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index 04ad312fd85b..0930f9f89571 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -509,6 +509,62 @@ int of_platform_default_populate(struct device_node *root,
> > >  }
> > >  EXPORT_SYMBOL_GPL(of_platform_default_populate);
> > >
> > > +static int of_link_binding(struct device *dev,
> > > +                          const char *binding, const char *cell)
> > > +{
> > > +       struct of_phandle_args sup_args;
> > > +       struct platform_device *sup_dev;
> > > +       unsigned int i = 0, links = 0;
> > > +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > > +
> > > +       while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
> > > +                                          &sup_args)) {
> > > +               i++;
> > > +               sup_dev = of_find_device_by_node(sup_args.np);
> > > +               of_node_put(sup_args.np);
> > > +               if (!sup_dev)
> > > +                       continue;
> > > +               if (device_link_add(dev, &sup_dev->dev, dl_flags))
> > > +                       links++;
> > > +               put_device(&sup_dev->dev);
> > > +       }
> > > +       if (links < i)
> > > +               return -ENODEV;
> > > +       return 0;
> > > +}
> > > +
> > > +static bool of_devlink;
> > > +core_param(of_devlink, of_devlink, bool, 0);
> > > +
> > > +/*
> > > + * List of bindings and their cell names (use NULL if no cell names) from which
> > > + * device links need to be created.
> > > + */
> > > +static const char * const link_bindings[] = {
> > > +       "clocks", "#clock-cells",
> > > +       "interconnects", "#interconnect-cells",
> > > +};
> > > +
> > > +static int of_link_to_suppliers(struct device *dev)
> > > +{
> > > +       unsigned int i = 0;
> > > +       bool done = true;
> > > +
> > > +       if (!of_devlink)
> > > +               return 0;
> > > +       if (unlikely(!dev->of_node))
> > > +               return 0;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
> > > +               if (of_link_binding(dev, link_bindings[i * 2],
> > > +                                       link_bindings[i * 2 + 1]))
> > > +                       done = false;
> >
> > Given the pending addition of regulators I think this should be
> > structured a bit differently so that we abstract out the matching and
> > phandle look-up so there's a clean separation of binding specifics.
> > It's kind of messy with 2 patterns to parse already and if we added a
> > 3rd? I would iterate over the properties as you do for regulators in
> > both cases and for each property call a binding specific match
> > function. The common pattern can of course be a common function. Let
> > me know if that makes sense. If not I can try to flesh it out some
> > more.
>
> I've added regulator support in this series and I've refactored this
> code as I went along. I fully expect to squash some of the refactors
> once the final result of the series is acceptable.

It would be easier to review the final result than incremental changes
which change the prior patches especially if the latter patches are
ultimately required.

> It's not clear to me if you got to the end of the series and still
> think the final result needs to be refactored. Let me know what you
> think about this towards the end of the series and I can clean it up
> if you still think it needs some clean up.

I probably should have replied on the regulator addition, but yes,
looking at that is what prompted my concerns here.

Rob

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings
  2019-07-17 14:35       ` Rob Herring
@ 2019-07-17 20:38         ` Saravana Kannan
  0 siblings, 0 replies; 16+ messages in thread
From: Saravana Kannan @ 2019-07-17 20:38 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand, Jonathan Corbet,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, David Collins, Android Kernel Team,
	Linux Doc Mailing List

On Wed, Jul 17, 2019 at 7:35 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Jul 16, 2019 at 5:54 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Tue, Jul 16, 2019 at 4:43 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Fri, Jul 12, 2019 at 5:52 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > Add device-links after the devices are created (but before they are
> > > > probed) by looking at common DT bindings like clocks and
> > > > interconnects.
> > > >
> > > > Automatically adding device-links for functional dependencies at the
> > > > framework level provides the following benefits:
> > > >
> > > > - Optimizes device probe order and avoids the useless work of
> > > >   attempting probes of devices that will not probe successfully
> > > >   (because their suppliers aren't present or haven't probed yet).
> > > >
> > > >   For example, in a commonly available mobile SoC, registering just
> > > >   one consumer device's driver at an initcall level earlier than the
> > > >   supplier device's driver causes 11 failed probe attempts before the
> > > >   consumer device probes successfully. This was with a kernel with all
> > > >   the drivers statically compiled in. This problem gets a lot worse if
> > > >   all the drivers are loaded as modules without direct symbol
> > > >   dependencies.
> > > >
> > > > - Supplier devices like clock providers, interconnect providers, etc
> > > >   need to keep the resources they provide active and at a particular
> > > >   state(s) during boot up even if their current set of consumers don't
> > > >   request the resource to be active. This is because the rest of the
> > > >   consumers might not have probed yet and turning off the resource
> > > >   before all the consumers have probed could lead to a hang or
> > > >   undesired user experience.
> > > >
> > > >   Some frameworks (Eg: regulator) handle this today by turning off
> > > >   "unused" resources at late_initcall_sync and hoping all the devices
> > > >   have probed by then. This is not a valid assumption for systems with
> > > >   loadable modules. Other frameworks (Eg: clock) just don't handle
> > > >   this due to the lack of a clear signal for when they can turn off
> > > >   resources. This leads to downstream hacks to handle cases like this
> > > >   that can easily be solved in the upstream kernel.
> > > >
> > > >   By linking devices before they are probed, we give suppliers a clear
> > > >   count of the number of dependent consumers. Once all of the
> > > >   consumers are active, the suppliers can turn off the unused
> > > >   resources without making assumptions about the number of consumers.
> > > >
> > > > By default we just add device-links to track "driver presence" (probe
> > > > succeeded) of the supplier device. If any other functionality provided
> > > > by device-links are needed, it is left to the consumer/supplier
> > > > devices to change the link when they probe.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  .../admin-guide/kernel-parameters.txt         |  5 ++
> > > >  drivers/of/platform.c                         | 57 +++++++++++++++++++
> > > >  2 files changed, 62 insertions(+)
> > > >
> > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > > index 138f6664b2e2..109b4310844f 100644
> > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > @@ -3141,6 +3141,11 @@
> > > >                         This can be set from sysctl after boot.
> > > >                         See Documentation/sysctl/vm.txt for details.
> > > >
> > > > +       of_devlink      [KNL] Make device links from common DT bindings. Useful
> > > > +                       for optimizing probe order and making sure resources
> > > > +                       aren't turned off before the consumer devices have
> > > > +                       probed.
> > > > +
> > > >         ohci1394_dma=early      [HW] enable debugging via the ohci1394 driver.
> > > >                         See Documentation/debugging-via-ohci1394.txt for more
> > > >                         info.
> > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > > index 04ad312fd85b..0930f9f89571 100644
> > > > --- a/drivers/of/platform.c
> > > > +++ b/drivers/of/platform.c
> > > > @@ -509,6 +509,62 @@ int of_platform_default_populate(struct device_node *root,
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(of_platform_default_populate);
> > > >
> > > > +static int of_link_binding(struct device *dev,
> > > > +                          const char *binding, const char *cell)
> > > > +{
> > > > +       struct of_phandle_args sup_args;
> > > > +       struct platform_device *sup_dev;
> > > > +       unsigned int i = 0, links = 0;
> > > > +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > > > +
> > > > +       while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
> > > > +                                          &sup_args)) {
> > > > +               i++;
> > > > +               sup_dev = of_find_device_by_node(sup_args.np);
> > > > +               of_node_put(sup_args.np);
> > > > +               if (!sup_dev)
> > > > +                       continue;
> > > > +               if (device_link_add(dev, &sup_dev->dev, dl_flags))
> > > > +                       links++;
> > > > +               put_device(&sup_dev->dev);
> > > > +       }
> > > > +       if (links < i)
> > > > +               return -ENODEV;
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static bool of_devlink;
> > > > +core_param(of_devlink, of_devlink, bool, 0);
> > > > +
> > > > +/*
> > > > + * List of bindings and their cell names (use NULL if no cell names) from which
> > > > + * device links need to be created.
> > > > + */
> > > > +static const char * const link_bindings[] = {
> > > > +       "clocks", "#clock-cells",
> > > > +       "interconnects", "#interconnect-cells",
> > > > +};
> > > > +
> > > > +static int of_link_to_suppliers(struct device *dev)
> > > > +{
> > > > +       unsigned int i = 0;
> > > > +       bool done = true;
> > > > +
> > > > +       if (!of_devlink)
> > > > +               return 0;
> > > > +       if (unlikely(!dev->of_node))
> > > > +               return 0;
> > > > +
> > > > +       for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
> > > > +               if (of_link_binding(dev, link_bindings[i * 2],
> > > > +                                       link_bindings[i * 2 + 1]))
> > > > +                       done = false;
> > >
> > > Given the pending addition of regulators I think this should be
> > > structured a bit differently so that we abstract out the matching and
> > > phandle look-up so there's a clean separation of binding specifics.
> > > It's kind of messy with 2 patterns to parse already and if we added a
> > > 3rd? I would iterate over the properties as you do for regulators in
> > > both cases and for each property call a binding specific match
> > > function. The common pattern can of course be a common function. Let
> > > me know if that makes sense. If not I can try to flesh it out some
> > > more.
> >
> > I've added regulator support in this series and I've refactored this
> > code as I went along. I fully expect to squash some of the refactors
> > once the final result of the series is acceptable.
>
> It would be easier to review the final result than incremental changes
> which change the prior patches especially if the latter patches are
> ultimately required.
>
> > It's not clear to me if you got to the end of the series and still
> > think the final result needs to be refactored. Let me know what you
> > think about this towards the end of the series and I can clean it up
> > if you still think it needs some clean up.
>
> I probably should have replied on the regulator addition, but yes,
> looking at that is what prompted my concerns here.

Sounds good. Let me refactor the series and send it out again. Btw, if
you have other issues you noticed, I'd be happy to fix those too
before sending out the v6 of the series again.

-Saravana

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2019-07-17 20:39 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-12 23:52 [PATCH v5 00/11] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 01/11] driver core: Add support for linking devices during device addition Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings Saravana Kannan
2019-07-16 23:43   ` Rob Herring
2019-07-16 23:53     ` Saravana Kannan
2019-07-17 14:35       ` Rob Herring
2019-07-17 20:38         ` Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 03/11] driver core: Add sync_state driver/bus callback Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 04/11] driver core: Add edit_links() callback for drivers Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 05/11] driver core: Add APIs to pause/resume sync state callbacks Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 06/11] of/platform: Pause/resume sync state in of_platform_populate() Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 07/11] of/platform: Sanity check DT bindings before creating device links Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 08/11] of/platform: Make sure supplier DT node is device when " Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 09/11] of/platform: Create device links for all child-supplier depencencies Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 10/11] of/platform: Add functional dependency link from DT regulator bindings Saravana Kannan
2019-07-12 23:52 ` [PATCH v5 11/11] of/platform: Don't create device links default busses Saravana Kannan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).