All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering
@ 2019-07-20  6:16 Saravana Kannan
  2019-07-20  6:16 ` [PATCH v6 1/7] driver core: Add support for linking devices during device addition Saravana Kannan
                   ` (7 more replies)
  0 siblings, 8 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-20  6:16 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.

v5 -> v6:
- Split, squashed and reordered some of the patches.
- Refactored the device linking code to follow the same code pattern for
  any property.

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 (7):
  driver core: Add support for linking devices during device addition
  driver core: Add edit_links() callback for drivers
  of/platform: Add functional dependency link from DT bindings
  driver core: Add sync_state driver/bus callback
  of/platform: Pause/resume sync state during init and
    of_platform_populate()
  of/platform: Create device links for all child-supplier depencencies
  of/platform: Don't create device links for default busses

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

-- 
2.22.0.657.g960e92d24f-goog


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

* [PATCH v6 1/7] driver core: Add support for linking devices during device addition
  2019-07-20  6:16 [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
@ 2019-07-20  6:16 ` Saravana Kannan
  2019-07-20  6:16 ` [PATCH v6 2/7] driver core: Add edit_links() callback for drivers Saravana Kannan
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-20  6:16 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.657.g960e92d24f-goog


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

* [PATCH v6 2/7] driver core: Add edit_links() callback for drivers
  2019-07-20  6:16 [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
  2019-07-20  6:16 ` [PATCH v6 1/7] driver core: Add support for linking devices during device addition Saravana Kannan
@ 2019-07-20  6:16 ` Saravana Kannan
  2019-07-20  6:16 ` [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings Saravana Kannan
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-20  6:16 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 supplier-consumer dependencies by default
enables functional dependencies to be tracked correctly even when the
consumer devices haven't had their drivers registered or loaded (if they
are modules).

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.

Neither devices will get probed since the supplier is marked as
dependent on the consumer. And the consumer will never probe because the
consumer 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.
14. Device-C probes.

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 0705926d362f..6eb14093e905 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -422,6 +422,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
  *
@@ -439,12 +452,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 7f8ae7e5fc6b..a2d509a36ebe 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -254,6 +254,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.
@@ -293,6 +307,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);
 	int (*remove) (struct device *dev);
 	void (*shutdown) (struct device *dev);
@@ -1065,6 +1080,7 @@ struct device {
 	bool			offline_disabled:1;
 	bool			offline:1;
 	bool			of_node_reused: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)
@@ -1314,6 +1330,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);
 
@@ -1404,6 +1421,7 @@ 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);
+void device_link_remove_from_wfs(struct device *consumer);
 
 #ifndef dev_fmt
 #define dev_fmt(fmt) fmt
-- 
2.22.0.657.g960e92d24f-goog


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

* [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings
  2019-07-20  6:16 [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
  2019-07-20  6:16 ` [PATCH v6 1/7] driver core: Add support for linking devices during device addition Saravana Kannan
  2019-07-20  6:16 ` [PATCH v6 2/7] driver core: Add edit_links() callback for drivers Saravana Kannan
@ 2019-07-20  6:16 ` Saravana Kannan
  2019-07-23 18:06   ` Rob Herring
  2019-07-20  6:16 ` [PATCH v6 4/7] driver core: Add sync_state driver/bus callback Saravana Kannan
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Saravana Kannan @ 2019-07-20  6:16 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                         | 158 ++++++++++++++++++
 2 files changed, 163 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..88a2086e26fa 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -509,6 +509,163 @@ 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);
+	/*
+	 * Don't allow linking a device node as a consumer of one of its
+	 * descendant nodes. By definition, a child node can't be a functional
+	 * dependency for the parent node.
+	 */
+	while (sup) {
+		if (sup == con) {
+			of_node_put(sup);
+			return false;
+		}
+		sup = of_get_next_parent(sup);
+	}
+	return true;
+}
+
+static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
+{
+	struct platform_device *sup_dev;
+	u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
+	int ret = 0;
+
+	/*
+	 * 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);
+		return 0;
+	}
+	sup_dev = of_find_device_by_node(sup_np);
+	of_node_put(sup_np);
+	if (!sup_dev)
+		return -ENODEV;
+	if (!device_link_add(dev, &sup_dev->dev, dl_flags))
+		ret = -ENODEV;
+	put_device(&sup_dev->dev);
+	return ret;
+}
+
+static struct device_node *parse_prop_cells(struct device_node *np,
+					    const char *prop, int i,
+					    const char *binding,
+					    const char *cell)
+{
+	struct of_phandle_args sup_args;
+
+	if (!i && strcmp(prop, binding))
+		return NULL;
+
+	if (of_parse_phandle_with_args(np, binding, cell, i, &sup_args))
+		return NULL;
+
+	return sup_args.np;
+}
+
+static struct device_node *parse_clocks(struct device_node *np,
+					const char *prop, int i)
+{
+	return parse_prop_cells(np, prop, i, "clocks", "#clock-cells");
+}
+
+static struct device_node *parse_interconnects(struct device_node *np,
+					       const char *prop, int i)
+{
+	return parse_prop_cells(np, prop, i, "interconnects",
+				"#interconnect-cells");
+}
+
+static int strcmp_suffix(const char *str, const char *suffix)
+{
+	unsigned int len, suffix_len;
+
+	len = strlen(str);
+	suffix_len = strlen(suffix);
+	if (len <= suffix_len)
+		return -1;
+	return strcmp(str + len - suffix_len, suffix);
+}
+
+static struct device_node *parse_regulators(struct device_node *np,
+					    const char *prop, int i)
+{
+	if (i || strcmp_suffix(prop, "-supply"))
+		return NULL;
+
+	return of_parse_phandle(np, prop, 0);
+}
+
+/**
+ * struct supplier_bindings - Information for parsing supplier DT binding
+ *
+ * @parse_prop:		If the function cannot parse the property, return NULL.
+ *			Otherwise, return the phandle listed in the property
+ *			that corresponds to index i.
+ */
+struct supplier_bindings {
+	struct device_node *(*parse_prop)(struct device_node *np,
+					  const char *name, int i);
+};
+
+struct supplier_bindings bindings[] = {
+	{ .parse_prop = parse_clocks, },
+	{ .parse_prop = parse_interconnects, },
+	{ .parse_prop = parse_regulators, },
+	{ },
+};
+
+static bool of_link_property(struct device *dev, struct device_node *con_np,
+			     const char *prop)
+{
+	struct device_node *phandle;
+	struct supplier_bindings *s = bindings;
+	unsigned int i = 0;
+	bool done = true;
+
+	while (!i && s->parse_prop) {
+		while ((phandle = s->parse_prop(con_np, prop, i))) {
+			i++;
+			if (of_link_to_phandle(dev, phandle))
+				done = false;
+		}
+		s++;
+	}
+	return done ? 0 : -ENODEV;
+}
+
+static bool of_devlink;
+core_param(of_devlink, of_devlink, bool, 0);
+
+static int of_link_to_suppliers(struct device *dev)
+{
+	struct property *p;
+	bool done = true;
+
+	if (!of_devlink)
+		return 0;
+	if (unlikely(!dev->of_node))
+		return 0;
+
+	for_each_property_of_node(dev->of_node, p)
+		if (of_link_property(dev, dev->of_node, p->name))
+			done = false;
+
+	return done ? 0 : -ENODEV;
+}
+
 #ifndef CONFIG_PPC
 static const struct of_device_id reserved_mem_matches[] = {
 	{ .compatible = "qcom,rmtfs-mem" },
@@ -524,6 +681,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.657.g960e92d24f-goog


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

* [PATCH v6 4/7] driver core: Add sync_state driver/bus callback
  2019-07-20  6:16 [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (2 preceding siblings ...)
  2019-07-20  6:16 ` [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings Saravana Kannan
@ 2019-07-20  6:16 ` Saravana Kannan
  2019-07-20  6:16 ` [PATCH v6 5/7] of/platform: Pause/resume sync state during init and of_platform_populate() Saravana Kannan
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-20  6:16 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.

Also, 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 devices
haven't had a chance to create device links to the suppliers.

To handle this correctly, this change also provides 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    | 65 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h | 21 ++++++++++++++
 2 files changed, 86 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6eb14093e905..b03e679faea4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -46,6 +46,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
 /* Device links support. */
 static LIST_HEAD(wait_for_suppliers);
 static DEFINE_MUTEX(wfs_lock);
+static LIST_HEAD(deferred_sync);
+static unsigned int supplier_sync_state_disabled;
 
 #ifdef CONFIG_SRCU
 static DEFINE_MUTEX(device_links_lock);
@@ -634,6 +636,62 @@ 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;
+}
+
+void device_links_supplier_sync_state_pause(void)
+{
+	device_links_write_lock();
+	supplier_sync_state_disabled++;
+	device_links_write_unlock();
+}
+
+void device_links_supplier_sync_state_resume(void)
+{
+	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);
+}
+
 /**
  * device_links_driver_bound - Update device links after probing its driver.
  * @dev: Device to update the links for.
@@ -678,6 +736,11 @@ 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_disabled)
+			__device_links_supplier_defer_sync(link->supplier);
+		else
+			__device_links_supplier_sync_state(link->supplier);
 	}
 
 	dev->links.status = DL_DEV_DRIVER_BOUND;
@@ -787,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();
@@ -1758,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/include/linux/device.h b/include/linux/device.h
index a2d509a36ebe..489d9f4e3104 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);
 
@@ -271,6 +279,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.
@@ -309,6 +324,7 @@ struct device_driver {
 
 	int (*edit_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);
 	int (*suspend) (struct device *dev, pm_message_t state);
@@ -910,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;
 };
 
@@ -1081,6 +1099,7 @@ struct device {
 	bool			offline:1;
 	bool			of_node_reused:1;
 	bool			has_edit_links: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)
@@ -1422,6 +1441,8 @@ struct device_link *device_link_add(struct device *consumer,
 void device_link_del(struct device_link *link);
 void device_link_remove(void *consumer, struct device *supplier);
 void device_link_remove_from_wfs(struct device *consumer);
+void device_links_supplier_sync_state_pause(void);
+void device_links_supplier_sync_state_resume(void);
 
 #ifndef dev_fmt
 #define dev_fmt(fmt) fmt
-- 
2.22.0.657.g960e92d24f-goog


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

* [PATCH v6 5/7] of/platform: Pause/resume sync state during init and of_platform_populate()
  2019-07-20  6:16 [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (3 preceding siblings ...)
  2019-07-20  6:16 ` [PATCH v6 4/7] driver core: Add sync_state driver/bus callback Saravana Kannan
@ 2019-07-20  6:16 ` Saravana Kannan
  2019-07-20  6:16 ` [PATCH v6 6/7] of/platform: Create device links for all child-supplier depencencies Saravana Kannan
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-20  6:16 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 all the top level devices are populated from DT during kernel
init, the supplier devices could be added and probed before the
consumer devices are added and linked to the suppliers. To avoid the
sync_state() callback from being called prematurely, pause the
sync_state() callbacks before populating the devices and resume them
at late_initcall_sync().

Similarly, when children devices are populated after kernel init using
of_platform_populate(), there could be supplier-consumer dependencies
between the children devices that are populated. To avoid the same
problem with sync_state() being called prematurely, pause and resume
sync_state() callbacks across of_platform_populate().

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

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 88a2086e26fa..d0c2f6443247 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);
@@ -682,6 +685,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
@@ -702,6 +706,13 @@ 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_resume();
+	return 0;
+}
+late_initcall_sync(of_platform_sync_state_init);
 #endif
 
 int of_platform_device_destroy(struct device *dev, void *data)
-- 
2.22.0.657.g960e92d24f-goog


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

* [PATCH v6 6/7] of/platform: Create device links for all child-supplier depencencies
  2019-07-20  6:16 [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (4 preceding siblings ...)
  2019-07-20  6:16 ` [PATCH v6 5/7] of/platform: Pause/resume sync state during init and of_platform_populate() Saravana Kannan
@ 2019-07-20  6:16 ` Saravana Kannan
  2019-07-20  6:16 ` [PATCH v6 7/7] of/platform: Don't create device links for default busses Saravana Kannan
  2019-07-22 23:47   ` Saravana Kannan
  7 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-20  6:16 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 | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index d0c2f6443247..c1a116f7a087 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -649,24 +649,35 @@ static bool of_link_property(struct device *dev, struct device_node *con_np,
 	return done ? 0 : -ENODEV;
 }
 
+static int __of_link_to_suppliers(struct device *dev,
+				  struct device_node *con_np)
+{
+	struct device_node *child;
+	struct property *p;
+	bool done = true;
+
+	for_each_property_of_node(con_np, p)
+		if (of_link_property(dev, con_np, p->name))
+			done = false;
+
+	for_each_child_of_node(con_np, child)
+		if (__of_link_to_suppliers(dev, child))
+			done = false;
+
+	return done ? 0 : -ENODEV;
+}
+
 static bool of_devlink;
 core_param(of_devlink, of_devlink, bool, 0);
 
 static int of_link_to_suppliers(struct device *dev)
 {
-	struct property *p;
-	bool done = true;
-
 	if (!of_devlink)
 		return 0;
 	if (unlikely(!dev->of_node))
 		return 0;
 
-	for_each_property_of_node(dev->of_node, p)
-		if (of_link_property(dev, dev->of_node, p->name))
-			done = false;
-
-	return done ? 0 : -ENODEV;
+	return __of_link_to_suppliers(dev, dev->of_node);
 }
 
 #ifndef CONFIG_PPC
-- 
2.22.0.657.g960e92d24f-goog


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

* [PATCH v6 7/7] of/platform: Don't create device links for default busses
  2019-07-20  6:16 [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
                   ` (5 preceding siblings ...)
  2019-07-20  6:16 ` [PATCH v6 6/7] of/platform: Create device links for all child-supplier depencencies Saravana Kannan
@ 2019-07-20  6:16 ` Saravana Kannan
  2019-07-22 23:47   ` Saravana Kannan
  7 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-20  6:16 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index c1a116f7a087..8bf975ee2ff7 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -676,6 +676,8 @@ static int of_link_to_suppliers(struct device *dev)
 		return 0;
 	if (unlikely(!dev->of_node))
 		return 0;
+	if (of_match_node(of_default_bus_match_table, dev->of_node))
+		return 0;
 
 	return __of_link_to_suppliers(dev, dev->of_node);
 }
-- 
2.22.0.657.g960e92d24f-goog


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

* Re: [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering
  2019-07-20  6:16 [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
@ 2019-07-22 23:47   ` Saravana Kannan
  2019-07-20  6:16 ` [PATCH v6 2/7] driver core: Add edit_links() callback for drivers Saravana Kannan
                     ` (6 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-22 23:47 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	David Collins, Android Kernel Team

On Fri, Jul 19, 2019 at 11:16 PM Saravana Kannan <saravanak@google.com> wrote:
>
> 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.
>
> v5 -> v6:
> - Split, squashed and reordered some of the patches.
> - Refactored the device linking code to follow the same code pattern for
>   any property.
>
> 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 (7):
>   driver core: Add support for linking devices during device addition
>   driver core: Add edit_links() callback for drivers
>   of/platform: Add functional dependency link from DT bindings
>   driver core: Add sync_state driver/bus callback
>   of/platform: Pause/resume sync state during init and
>     of_platform_populate()
>   of/platform: Create device links for all child-supplier depencencies
>   of/platform: Don't create device links for default busses
>
>  .../admin-guide/kernel-parameters.txt         |   5 +
>  drivers/base/core.c                           | 168 ++++++++++++++++
>  drivers/base/dd.c                             |  29 +++
>  drivers/of/platform.c                         | 182 ++++++++++++++++++
>  include/linux/device.h                        |  47 +++++
>  5 files changed, 431 insertions(+)

Update: Tested this refactor on hardware by backporting to a 4.14
kernel. Works just as it did before the refactor.

Also, nudge to make sure this series isn't lost over the weekend email snooze.

-Saravana

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

* Re: [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering
@ 2019-07-22 23:47   ` Saravana Kannan
  0 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-22 23:47 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
	Frank Rowand
  Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	David Collins, Android Kernel Team

On Fri, Jul 19, 2019 at 11:16 PM Saravana Kannan <saravanak@google.com> wrote:
>
> 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.
>
> v5 -> v6:
> - Split, squashed and reordered some of the patches.
> - Refactored the device linking code to follow the same code pattern for
>   any property.
>
> 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 (7):
>   driver core: Add support for linking devices during device addition
>   driver core: Add edit_links() callback for drivers
>   of/platform: Add functional dependency link from DT bindings
>   driver core: Add sync_state driver/bus callback
>   of/platform: Pause/resume sync state during init and
>     of_platform_populate()
>   of/platform: Create device links for all child-supplier depencencies
>   of/platform: Don't create device links for default busses
>
>  .../admin-guide/kernel-parameters.txt         |   5 +
>  drivers/base/core.c                           | 168 ++++++++++++++++
>  drivers/base/dd.c                             |  29 +++
>  drivers/of/platform.c                         | 182 ++++++++++++++++++
>  include/linux/device.h                        |  47 +++++
>  5 files changed, 431 insertions(+)

Update: Tested this refactor on hardware by backporting to a 4.14
kernel. Works just as it did before the refactor.

Also, nudge to make sure this series isn't lost over the weekend email snooze.

-Saravana

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

* Re: [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings
  2019-07-20  6:16 ` [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings Saravana Kannan
@ 2019-07-23 18:06   ` Rob Herring
  2019-07-23 20:48       ` Saravana Kannan
  0 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2019-07-23 18:06 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 Sat, Jul 20, 2019 at 12:17 AM 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.

The structure now looks a lot better to me. A few minor things below.

>
> 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                         | 158 ++++++++++++++++++
>  2 files changed, 163 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..88a2086e26fa 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -509,6 +509,163 @@ 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);
> +       /*
> +        * Don't allow linking a device node as a consumer of one of its
> +        * descendant nodes. By definition, a child node can't be a functional
> +        * dependency for the parent node.
> +        */
> +       while (sup) {
> +               if (sup == con) {
> +                       of_node_put(sup);
> +                       return false;
> +               }
> +               sup = of_get_next_parent(sup);
> +       }
> +       return true;
> +}
> +
> +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> +{
> +       struct platform_device *sup_dev;
> +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> +       int ret = 0;
> +
> +       /*
> +        * 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);
> +               return 0;
> +       }
> +       sup_dev = of_find_device_by_node(sup_np);
> +       of_node_put(sup_np);
> +       if (!sup_dev)
> +               return -ENODEV;
> +       if (!device_link_add(dev, &sup_dev->dev, dl_flags))
> +               ret = -ENODEV;
> +       put_device(&sup_dev->dev);
> +       return ret;
> +}
> +
> +static struct device_node *parse_prop_cells(struct device_node *np,
> +                                           const char *prop, int i,

I like 'i' for for loops, but less so for function params. Perhaps
'index' instead like of_parse_phandle_with_args.

> +                                           const char *binding,
> +                                           const char *cell)
> +{
> +       struct of_phandle_args sup_args;
> +
> +       if (!i && strcmp(prop, binding))

Why the '!i' test?

> +               return NULL;
> +
> +       if (of_parse_phandle_with_args(np, binding, cell, i, &sup_args))
> +               return NULL;
> +
> +       return sup_args.np;
> +}
> +
> +static struct device_node *parse_clocks(struct device_node *np,
> +                                       const char *prop, int i)
> +{
> +       return parse_prop_cells(np, prop, i, "clocks", "#clock-cells");
> +}
> +
> +static struct device_node *parse_interconnects(struct device_node *np,
> +                                              const char *prop, int i)
> +{
> +       return parse_prop_cells(np, prop, i, "interconnects",
> +                               "#interconnect-cells");
> +}
> +
> +static int strcmp_suffix(const char *str, const char *suffix)
> +{
> +       unsigned int len, suffix_len;
> +
> +       len = strlen(str);
> +       suffix_len = strlen(suffix);
> +       if (len <= suffix_len)
> +               return -1;
> +       return strcmp(str + len - suffix_len, suffix);
> +}
> +
> +static struct device_node *parse_regulators(struct device_node *np,
> +                                           const char *prop, int i)
> +{
> +       if (i || strcmp_suffix(prop, "-supply"))
> +               return NULL;
> +
> +       return of_parse_phandle(np, prop, 0);
> +}
> +
> +/**
> + * struct supplier_bindings - Information for parsing supplier DT binding
> + *
> + * @parse_prop:                If the function cannot parse the property, return NULL.
> + *                     Otherwise, return the phandle listed in the property
> + *                     that corresponds to index i.
> + */
> +struct supplier_bindings {
> +       struct device_node *(*parse_prop)(struct device_node *np,
> +                                         const char *name, int i);
> +};
> +
> +struct supplier_bindings bindings[] = {

static const

> +       { .parse_prop = parse_clocks, },
> +       { .parse_prop = parse_interconnects, },
> +       { .parse_prop = parse_regulators, },
> +       { },
> +};
> +
> +static bool of_link_property(struct device *dev, struct device_node *con_np,
> +                            const char *prop)
> +{
> +       struct device_node *phandle;
> +       struct supplier_bindings *s = bindings;
> +       unsigned int i = 0;
> +       bool done = true;
> +
> +       while (!i && s->parse_prop) {

Using 'i' is a little odd. Perhaps a 'matched' bool would be easier to read.

> +               while ((phandle = s->parse_prop(con_np, prop, i))) {
> +                       i++;
> +                       if (of_link_to_phandle(dev, phandle))
> +                               done = false;

Just return here. No point in continuing as 'done' is never set back to true.

> +               }
> +               s++;
> +       }
> +       return done ? 0 : -ENODEV;
> +}
> +
> +static bool of_devlink;
> +core_param(of_devlink, of_devlink, bool, 0);
> +
> +static int of_link_to_suppliers(struct device *dev)
> +{
> +       struct property *p;
> +       bool done = true;
> +
> +       if (!of_devlink)
> +               return 0;
> +       if (unlikely(!dev->of_node))
> +               return 0;
> +
> +       for_each_property_of_node(dev->of_node, p)
> +               if (of_link_property(dev, dev->of_node, p->name))
> +                       done = false;
> +
> +       return done ? 0 : -ENODEV;
> +}
> +
>  #ifndef CONFIG_PPC
>  static const struct of_device_id reserved_mem_matches[] = {
>         { .compatible = "qcom,rmtfs-mem" },
> @@ -524,6 +681,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.657.g960e92d24f-goog
>

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

* Re: [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings
  2019-07-23 18:06   ` Rob Herring
@ 2019-07-23 20:48       ` Saravana Kannan
  0 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-23 20:48 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 23, 2019 at 11:06 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Sat, Jul 20, 2019 at 12:17 AM 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.
>
> The structure now looks a lot better to me. A few minor things below.

Thanks.

> >
> > 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                         | 158 ++++++++++++++++++
> >  2 files changed, 163 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..88a2086e26fa 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -509,6 +509,163 @@ 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);
> > +       /*
> > +        * Don't allow linking a device node as a consumer of one of its
> > +        * descendant nodes. By definition, a child node can't be a functional
> > +        * dependency for the parent node.
> > +        */
> > +       while (sup) {
> > +               if (sup == con) {
> > +                       of_node_put(sup);
> > +                       return false;
> > +               }
> > +               sup = of_get_next_parent(sup);
> > +       }
> > +       return true;
> > +}
> > +
> > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> > +{
> > +       struct platform_device *sup_dev;
> > +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > +       int ret = 0;
> > +
> > +       /*
> > +        * 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);
> > +               return 0;
> > +       }
> > +       sup_dev = of_find_device_by_node(sup_np);
> > +       of_node_put(sup_np);
> > +       if (!sup_dev)
> > +               return -ENODEV;
> > +       if (!device_link_add(dev, &sup_dev->dev, dl_flags))
> > +               ret = -ENODEV;
> > +       put_device(&sup_dev->dev);
> > +       return ret;
> > +}
> > +
> > +static struct device_node *parse_prop_cells(struct device_node *np,
> > +                                           const char *prop, int i,
>
> I like 'i' for for loops, but less so for function params. Perhaps
> 'index' instead like of_parse_phandle_with_args.

Sounds good.

>
> > +                                           const char *binding,
> > +                                           const char *cell)
> > +{
> > +       struct of_phandle_args sup_args;
> > +
> > +       if (!i && strcmp(prop, binding))
>
> Why the '!i' test?

To avoid a string comparison for every index. It's kinda wasteful once
the first index passes.

> > +               return NULL;
> > +
> > +       if (of_parse_phandle_with_args(np, binding, cell, i, &sup_args))
> > +               return NULL;
> > +
> > +       return sup_args.np;
> > +}
> > +
> > +static struct device_node *parse_clocks(struct device_node *np,
> > +                                       const char *prop, int i)
> > +{
> > +       return parse_prop_cells(np, prop, i, "clocks", "#clock-cells");
> > +}
> > +
> > +static struct device_node *parse_interconnects(struct device_node *np,
> > +                                              const char *prop, int i)
> > +{
> > +       return parse_prop_cells(np, prop, i, "interconnects",
> > +                               "#interconnect-cells");
> > +}
> > +
> > +static int strcmp_suffix(const char *str, const char *suffix)
> > +{
> > +       unsigned int len, suffix_len;
> > +
> > +       len = strlen(str);
> > +       suffix_len = strlen(suffix);
> > +       if (len <= suffix_len)
> > +               return -1;
> > +       return strcmp(str + len - suffix_len, suffix);
> > +}
> > +
> > +static struct device_node *parse_regulators(struct device_node *np,
> > +                                           const char *prop, int i)
> > +{
> > +       if (i || strcmp_suffix(prop, "-supply"))
> > +               return NULL;
> > +
> > +       return of_parse_phandle(np, prop, 0);
> > +}
> > +
> > +/**
> > + * struct supplier_bindings - Information for parsing supplier DT binding
> > + *
> > + * @parse_prop:                If the function cannot parse the property, return NULL.
> > + *                     Otherwise, return the phandle listed in the property
> > + *                     that corresponds to index i.
> > + */
> > +struct supplier_bindings {
> > +       struct device_node *(*parse_prop)(struct device_node *np,
> > +                                         const char *name, int i);
> > +};
> > +
> > +struct supplier_bindings bindings[] = {
>
> static const

Will do.

>
> > +       { .parse_prop = parse_clocks, },
> > +       { .parse_prop = parse_interconnects, },
> > +       { .parse_prop = parse_regulators, },
> > +       { },
> > +};
> > +
> > +static bool of_link_property(struct device *dev, struct device_node *con_np,
> > +                            const char *prop)
> > +{
> > +       struct device_node *phandle;
> > +       struct supplier_bindings *s = bindings;
> > +       unsigned int i = 0;
> > +       bool done = true;
> > +
> > +       while (!i && s->parse_prop) {
>
> Using 'i' is a little odd. Perhaps a 'matched' bool would be easier to read.

That's how I wrote it first (locally) and then redid it this way
because the bool felt very superfluous. I don't think this is that
hard to understand.

> > +               while ((phandle = s->parse_prop(con_np, prop, i))) {
> > +                       i++;
> > +                       if (of_link_to_phandle(dev, phandle))
> > +                               done = false;
>
> Just return here. No point in continuing as 'done' is never set back to true.

Actually, there is a point for this. Say Device-C depends on suppliers
Device-S1 and Device-S2 and they are listed in DT in that order.

Say, S1 gets populated after late_initcall_sync but S2 is probes way
before that. If I don't continue past a "failed linking" to S1 and
also link up to S2, then S2 will get a sync_state() callback before C
is probed. So I have to go through all possible suppliers and as many
as possible.

Let me add a comment about this somewhere in the code (probably the
header that defines the add_links() ops).

-Saravana

> > +               }
> > +               s++;
> > +       }
> > +       return done ? 0 : -ENODEV;
> > +}
> > +
> > +static bool of_devlink;
> > +core_param(of_devlink, of_devlink, bool, 0);
> > +
> > +static int of_link_to_suppliers(struct device *dev)
> > +{
> > +       struct property *p;
> > +       bool done = true;
> > +
> > +       if (!of_devlink)
> > +               return 0;
> > +       if (unlikely(!dev->of_node))
> > +               return 0;
> > +
> > +       for_each_property_of_node(dev->of_node, p)
> > +               if (of_link_property(dev, dev->of_node, p->name))
> > +                       done = false;
> > +
> > +       return done ? 0 : -ENODEV;
> > +}
> > +
> >  #ifndef CONFIG_PPC
> >  static const struct of_device_id reserved_mem_matches[] = {
> >         { .compatible = "qcom,rmtfs-mem" },
> > @@ -524,6 +681,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.657.g960e92d24f-goog
> >

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

* Re: [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings
@ 2019-07-23 20:48       ` Saravana Kannan
  0 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-23 20:48 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 23, 2019 at 11:06 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Sat, Jul 20, 2019 at 12:17 AM 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.
>
> The structure now looks a lot better to me. A few minor things below.

Thanks.

> >
> > 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                         | 158 ++++++++++++++++++
> >  2 files changed, 163 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..88a2086e26fa 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -509,6 +509,163 @@ 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);
> > +       /*
> > +        * Don't allow linking a device node as a consumer of one of its
> > +        * descendant nodes. By definition, a child node can't be a functional
> > +        * dependency for the parent node.
> > +        */
> > +       while (sup) {
> > +               if (sup == con) {
> > +                       of_node_put(sup);
> > +                       return false;
> > +               }
> > +               sup = of_get_next_parent(sup);
> > +       }
> > +       return true;
> > +}
> > +
> > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> > +{
> > +       struct platform_device *sup_dev;
> > +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > +       int ret = 0;
> > +
> > +       /*
> > +        * 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);
> > +               return 0;
> > +       }
> > +       sup_dev = of_find_device_by_node(sup_np);
> > +       of_node_put(sup_np);
> > +       if (!sup_dev)
> > +               return -ENODEV;
> > +       if (!device_link_add(dev, &sup_dev->dev, dl_flags))
> > +               ret = -ENODEV;
> > +       put_device(&sup_dev->dev);
> > +       return ret;
> > +}
> > +
> > +static struct device_node *parse_prop_cells(struct device_node *np,
> > +                                           const char *prop, int i,
>
> I like 'i' for for loops, but less so for function params. Perhaps
> 'index' instead like of_parse_phandle_with_args.

Sounds good.

>
> > +                                           const char *binding,
> > +                                           const char *cell)
> > +{
> > +       struct of_phandle_args sup_args;
> > +
> > +       if (!i && strcmp(prop, binding))
>
> Why the '!i' test?

To avoid a string comparison for every index. It's kinda wasteful once
the first index passes.

> > +               return NULL;
> > +
> > +       if (of_parse_phandle_with_args(np, binding, cell, i, &sup_args))
> > +               return NULL;
> > +
> > +       return sup_args.np;
> > +}
> > +
> > +static struct device_node *parse_clocks(struct device_node *np,
> > +                                       const char *prop, int i)
> > +{
> > +       return parse_prop_cells(np, prop, i, "clocks", "#clock-cells");
> > +}
> > +
> > +static struct device_node *parse_interconnects(struct device_node *np,
> > +                                              const char *prop, int i)
> > +{
> > +       return parse_prop_cells(np, prop, i, "interconnects",
> > +                               "#interconnect-cells");
> > +}
> > +
> > +static int strcmp_suffix(const char *str, const char *suffix)
> > +{
> > +       unsigned int len, suffix_len;
> > +
> > +       len = strlen(str);
> > +       suffix_len = strlen(suffix);
> > +       if (len <= suffix_len)
> > +               return -1;
> > +       return strcmp(str + len - suffix_len, suffix);
> > +}
> > +
> > +static struct device_node *parse_regulators(struct device_node *np,
> > +                                           const char *prop, int i)
> > +{
> > +       if (i || strcmp_suffix(prop, "-supply"))
> > +               return NULL;
> > +
> > +       return of_parse_phandle(np, prop, 0);
> > +}
> > +
> > +/**
> > + * struct supplier_bindings - Information for parsing supplier DT binding
> > + *
> > + * @parse_prop:                If the function cannot parse the property, return NULL.
> > + *                     Otherwise, return the phandle listed in the property
> > + *                     that corresponds to index i.
> > + */
> > +struct supplier_bindings {
> > +       struct device_node *(*parse_prop)(struct device_node *np,
> > +                                         const char *name, int i);
> > +};
> > +
> > +struct supplier_bindings bindings[] = {
>
> static const

Will do.

>
> > +       { .parse_prop = parse_clocks, },
> > +       { .parse_prop = parse_interconnects, },
> > +       { .parse_prop = parse_regulators, },
> > +       { },
> > +};
> > +
> > +static bool of_link_property(struct device *dev, struct device_node *con_np,
> > +                            const char *prop)
> > +{
> > +       struct device_node *phandle;
> > +       struct supplier_bindings *s = bindings;
> > +       unsigned int i = 0;
> > +       bool done = true;
> > +
> > +       while (!i && s->parse_prop) {
>
> Using 'i' is a little odd. Perhaps a 'matched' bool would be easier to read.

That's how I wrote it first (locally) and then redid it this way
because the bool felt very superfluous. I don't think this is that
hard to understand.

> > +               while ((phandle = s->parse_prop(con_np, prop, i))) {
> > +                       i++;
> > +                       if (of_link_to_phandle(dev, phandle))
> > +                               done = false;
>
> Just return here. No point in continuing as 'done' is never set back to true.

Actually, there is a point for this. Say Device-C depends on suppliers
Device-S1 and Device-S2 and they are listed in DT in that order.

Say, S1 gets populated after late_initcall_sync but S2 is probes way
before that. If I don't continue past a "failed linking" to S1 and
also link up to S2, then S2 will get a sync_state() callback before C
is probed. So I have to go through all possible suppliers and as many
as possible.

Let me add a comment about this somewhere in the code (probably the
header that defines the add_links() ops).

-Saravana

> > +               }
> > +               s++;
> > +       }
> > +       return done ? 0 : -ENODEV;
> > +}
> > +
> > +static bool of_devlink;
> > +core_param(of_devlink, of_devlink, bool, 0);
> > +
> > +static int of_link_to_suppliers(struct device *dev)
> > +{
> > +       struct property *p;
> > +       bool done = true;
> > +
> > +       if (!of_devlink)
> > +               return 0;
> > +       if (unlikely(!dev->of_node))
> > +               return 0;
> > +
> > +       for_each_property_of_node(dev->of_node, p)
> > +               if (of_link_property(dev, dev->of_node, p->name))
> > +                       done = false;
> > +
> > +       return done ? 0 : -ENODEV;
> > +}
> > +
> >  #ifndef CONFIG_PPC
> >  static const struct of_device_id reserved_mem_matches[] = {
> >         { .compatible = "qcom,rmtfs-mem" },
> > @@ -524,6 +681,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.657.g960e92d24f-goog
> >

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

* Re: [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings
  2019-07-23 20:48       ` Saravana Kannan
@ 2019-07-23 22:18         ` Rob Herring
  -1 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2019-07-23 22:18 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 23, 2019 at 2:49 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Jul 23, 2019 at 11:06 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Sat, Jul 20, 2019 at 12:17 AM 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.
> >
> > The structure now looks a lot better to me. A few minor things below.
>
> Thanks.
>
> > >
> > > 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                         | 158 ++++++++++++++++++
> > >  2 files changed, 163 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..88a2086e26fa 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -509,6 +509,163 @@ 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);
> > > +       /*
> > > +        * Don't allow linking a device node as a consumer of one of its
> > > +        * descendant nodes. By definition, a child node can't be a functional
> > > +        * dependency for the parent node.
> > > +        */
> > > +       while (sup) {
> > > +               if (sup == con) {
> > > +                       of_node_put(sup);
> > > +                       return false;
> > > +               }
> > > +               sup = of_get_next_parent(sup);
> > > +       }
> > > +       return true;
> > > +}
> > > +
> > > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> > > +{
> > > +       struct platform_device *sup_dev;
> > > +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > > +       int ret = 0;
> > > +
> > > +       /*
> > > +        * 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);
> > > +               return 0;
> > > +       }
> > > +       sup_dev = of_find_device_by_node(sup_np);
> > > +       of_node_put(sup_np);
> > > +       if (!sup_dev)
> > > +               return -ENODEV;
> > > +       if (!device_link_add(dev, &sup_dev->dev, dl_flags))
> > > +               ret = -ENODEV;
> > > +       put_device(&sup_dev->dev);
> > > +       return ret;
> > > +}
> > > +
> > > +static struct device_node *parse_prop_cells(struct device_node *np,
> > > +                                           const char *prop, int i,
> >
> > I like 'i' for for loops, but less so for function params. Perhaps
> > 'index' instead like of_parse_phandle_with_args.
>
> Sounds good.
>
> >
> > > +                                           const char *binding,
> > > +                                           const char *cell)
> > > +{
> > > +       struct of_phandle_args sup_args;
> > > +
> > > +       if (!i && strcmp(prop, binding))
> >
> > Why the '!i' test?
>
> To avoid a string comparison for every index. It's kinda wasteful once
> the first index passes.

That's not very obvious and pretty fragile though this is a static
function. Perhaps we should split to match() and parse() functions. At
least put a comment here as to what we're doing.

>
> > > +               return NULL;
> > > +
> > > +       if (of_parse_phandle_with_args(np, binding, cell, i, &sup_args))
> > > +               return NULL;
> > > +
> > > +       return sup_args.np;
> > > +}
> > > +
> > > +static struct device_node *parse_clocks(struct device_node *np,
> > > +                                       const char *prop, int i)
> > > +{
> > > +       return parse_prop_cells(np, prop, i, "clocks", "#clock-cells");
> > > +}
> > > +
> > > +static struct device_node *parse_interconnects(struct device_node *np,
> > > +                                              const char *prop, int i)
> > > +{
> > > +       return parse_prop_cells(np, prop, i, "interconnects",
> > > +                               "#interconnect-cells");
> > > +}
> > > +
> > > +static int strcmp_suffix(const char *str, const char *suffix)
> > > +{
> > > +       unsigned int len, suffix_len;
> > > +
> > > +       len = strlen(str);
> > > +       suffix_len = strlen(suffix);
> > > +       if (len <= suffix_len)
> > > +               return -1;
> > > +       return strcmp(str + len - suffix_len, suffix);
> > > +}
> > > +
> > > +static struct device_node *parse_regulators(struct device_node *np,
> > > +                                           const char *prop, int i)
> > > +{
> > > +       if (i || strcmp_suffix(prop, "-supply"))
> > > +               return NULL;
> > > +
> > > +       return of_parse_phandle(np, prop, 0);
> > > +}
> > > +
> > > +/**
> > > + * struct supplier_bindings - Information for parsing supplier DT binding
> > > + *
> > > + * @parse_prop:                If the function cannot parse the property, return NULL.
> > > + *                     Otherwise, return the phandle listed in the property
> > > + *                     that corresponds to index i.
> > > + */
> > > +struct supplier_bindings {
> > > +       struct device_node *(*parse_prop)(struct device_node *np,
> > > +                                         const char *name, int i);
> > > +};
> > > +
> > > +struct supplier_bindings bindings[] = {
> >
> > static const
>
> Will do.
>
> >
> > > +       { .parse_prop = parse_clocks, },
> > > +       { .parse_prop = parse_interconnects, },
> > > +       { .parse_prop = parse_regulators, },
> > > +       { },
> > > +};
> > > +
> > > +static bool of_link_property(struct device *dev, struct device_node *con_np,
> > > +                            const char *prop)
> > > +{
> > > +       struct device_node *phandle;
> > > +       struct supplier_bindings *s = bindings;
> > > +       unsigned int i = 0;
> > > +       bool done = true;
> > > +
> > > +       while (!i && s->parse_prop) {
> >
> > Using 'i' is a little odd. Perhaps a 'matched' bool would be easier to read.
>
> That's how I wrote it first (locally) and then redid it this way
> because the bool felt very superfluous. I don't think this is that
> hard to understand.

Alright...

> > > +               while ((phandle = s->parse_prop(con_np, prop, i))) {
> > > +                       i++;
> > > +                       if (of_link_to_phandle(dev, phandle))
> > > +                               done = false;
> >
> > Just return here. No point in continuing as 'done' is never set back to true.
>
> Actually, there is a point for this. Say Device-C depends on suppliers
> Device-S1 and Device-S2 and they are listed in DT in that order.
>
> Say, S1 gets populated after late_initcall_sync but S2 is probes way
> before that. If I don't continue past a "failed linking" to S1 and
> also link up to S2, then S2 will get a sync_state() callback before C
> is probed. So I have to go through all possible suppliers and as many
> as possible.
>
> Let me add a comment about this somewhere in the code (probably the
> header that defines the add_links() ops).

Okay, makes sense.

Rob

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

* Re: [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings
@ 2019-07-23 22:18         ` Rob Herring
  0 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2019-07-23 22:18 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 23, 2019 at 2:49 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Jul 23, 2019 at 11:06 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Sat, Jul 20, 2019 at 12:17 AM 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.
> >
> > The structure now looks a lot better to me. A few minor things below.
>
> Thanks.
>
> > >
> > > 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                         | 158 ++++++++++++++++++
> > >  2 files changed, 163 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..88a2086e26fa 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -509,6 +509,163 @@ 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);
> > > +       /*
> > > +        * Don't allow linking a device node as a consumer of one of its
> > > +        * descendant nodes. By definition, a child node can't be a functional
> > > +        * dependency for the parent node.
> > > +        */
> > > +       while (sup) {
> > > +               if (sup == con) {
> > > +                       of_node_put(sup);
> > > +                       return false;
> > > +               }
> > > +               sup = of_get_next_parent(sup);
> > > +       }
> > > +       return true;
> > > +}
> > > +
> > > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> > > +{
> > > +       struct platform_device *sup_dev;
> > > +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > > +       int ret = 0;
> > > +
> > > +       /*
> > > +        * 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);
> > > +               return 0;
> > > +       }
> > > +       sup_dev = of_find_device_by_node(sup_np);
> > > +       of_node_put(sup_np);
> > > +       if (!sup_dev)
> > > +               return -ENODEV;
> > > +       if (!device_link_add(dev, &sup_dev->dev, dl_flags))
> > > +               ret = -ENODEV;
> > > +       put_device(&sup_dev->dev);
> > > +       return ret;
> > > +}
> > > +
> > > +static struct device_node *parse_prop_cells(struct device_node *np,
> > > +                                           const char *prop, int i,
> >
> > I like 'i' for for loops, but less so for function params. Perhaps
> > 'index' instead like of_parse_phandle_with_args.
>
> Sounds good.
>
> >
> > > +                                           const char *binding,
> > > +                                           const char *cell)
> > > +{
> > > +       struct of_phandle_args sup_args;
> > > +
> > > +       if (!i && strcmp(prop, binding))
> >
> > Why the '!i' test?
>
> To avoid a string comparison for every index. It's kinda wasteful once
> the first index passes.

That's not very obvious and pretty fragile though this is a static
function. Perhaps we should split to match() and parse() functions. At
least put a comment here as to what we're doing.

>
> > > +               return NULL;
> > > +
> > > +       if (of_parse_phandle_with_args(np, binding, cell, i, &sup_args))
> > > +               return NULL;
> > > +
> > > +       return sup_args.np;
> > > +}
> > > +
> > > +static struct device_node *parse_clocks(struct device_node *np,
> > > +                                       const char *prop, int i)
> > > +{
> > > +       return parse_prop_cells(np, prop, i, "clocks", "#clock-cells");
> > > +}
> > > +
> > > +static struct device_node *parse_interconnects(struct device_node *np,
> > > +                                              const char *prop, int i)
> > > +{
> > > +       return parse_prop_cells(np, prop, i, "interconnects",
> > > +                               "#interconnect-cells");
> > > +}
> > > +
> > > +static int strcmp_suffix(const char *str, const char *suffix)
> > > +{
> > > +       unsigned int len, suffix_len;
> > > +
> > > +       len = strlen(str);
> > > +       suffix_len = strlen(suffix);
> > > +       if (len <= suffix_len)
> > > +               return -1;
> > > +       return strcmp(str + len - suffix_len, suffix);
> > > +}
> > > +
> > > +static struct device_node *parse_regulators(struct device_node *np,
> > > +                                           const char *prop, int i)
> > > +{
> > > +       if (i || strcmp_suffix(prop, "-supply"))
> > > +               return NULL;
> > > +
> > > +       return of_parse_phandle(np, prop, 0);
> > > +}
> > > +
> > > +/**
> > > + * struct supplier_bindings - Information for parsing supplier DT binding
> > > + *
> > > + * @parse_prop:                If the function cannot parse the property, return NULL.
> > > + *                     Otherwise, return the phandle listed in the property
> > > + *                     that corresponds to index i.
> > > + */
> > > +struct supplier_bindings {
> > > +       struct device_node *(*parse_prop)(struct device_node *np,
> > > +                                         const char *name, int i);
> > > +};
> > > +
> > > +struct supplier_bindings bindings[] = {
> >
> > static const
>
> Will do.
>
> >
> > > +       { .parse_prop = parse_clocks, },
> > > +       { .parse_prop = parse_interconnects, },
> > > +       { .parse_prop = parse_regulators, },
> > > +       { },
> > > +};
> > > +
> > > +static bool of_link_property(struct device *dev, struct device_node *con_np,
> > > +                            const char *prop)
> > > +{
> > > +       struct device_node *phandle;
> > > +       struct supplier_bindings *s = bindings;
> > > +       unsigned int i = 0;
> > > +       bool done = true;
> > > +
> > > +       while (!i && s->parse_prop) {
> >
> > Using 'i' is a little odd. Perhaps a 'matched' bool would be easier to read.
>
> That's how I wrote it first (locally) and then redid it this way
> because the bool felt very superfluous. I don't think this is that
> hard to understand.

Alright...

> > > +               while ((phandle = s->parse_prop(con_np, prop, i))) {
> > > +                       i++;
> > > +                       if (of_link_to_phandle(dev, phandle))
> > > +                               done = false;
> >
> > Just return here. No point in continuing as 'done' is never set back to true.
>
> Actually, there is a point for this. Say Device-C depends on suppliers
> Device-S1 and Device-S2 and they are listed in DT in that order.
>
> Say, S1 gets populated after late_initcall_sync but S2 is probes way
> before that. If I don't continue past a "failed linking" to S1 and
> also link up to S2, then S2 will get a sync_state() callback before C
> is probed. So I have to go through all possible suppliers and as many
> as possible.
>
> Let me add a comment about this somewhere in the code (probably the
> header that defines the add_links() ops).

Okay, makes sense.

Rob

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

* Re: [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings
  2019-07-23 22:18         ` Rob Herring
@ 2019-07-23 23:57           ` Saravana Kannan
  -1 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-23 23:57 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 23, 2019 at 3:18 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Jul 23, 2019 at 2:49 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Tue, Jul 23, 2019 at 11:06 AM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Sat, Jul 20, 2019 at 12:17 AM 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.
> > >
> > > The structure now looks a lot better to me. A few minor things below.
> >
> > Thanks.
> >
> > > >
> > > > 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                         | 158 ++++++++++++++++++
> > > >  2 files changed, 163 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..88a2086e26fa 100644
> > > > --- a/drivers/of/platform.c
> > > > +++ b/drivers/of/platform.c
> > > > @@ -509,6 +509,163 @@ 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);
> > > > +       /*
> > > > +        * Don't allow linking a device node as a consumer of one of its
> > > > +        * descendant nodes. By definition, a child node can't be a functional
> > > > +        * dependency for the parent node.
> > > > +        */
> > > > +       while (sup) {
> > > > +               if (sup == con) {
> > > > +                       of_node_put(sup);
> > > > +                       return false;
> > > > +               }
> > > > +               sup = of_get_next_parent(sup);
> > > > +       }
> > > > +       return true;
> > > > +}
> > > > +
> > > > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> > > > +{
> > > > +       struct platform_device *sup_dev;
> > > > +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > > > +       int ret = 0;
> > > > +
> > > > +       /*
> > > > +        * 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);
> > > > +               return 0;
> > > > +       }
> > > > +       sup_dev = of_find_device_by_node(sup_np);
> > > > +       of_node_put(sup_np);
> > > > +       if (!sup_dev)
> > > > +               return -ENODEV;
> > > > +       if (!device_link_add(dev, &sup_dev->dev, dl_flags))
> > > > +               ret = -ENODEV;
> > > > +       put_device(&sup_dev->dev);
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +static struct device_node *parse_prop_cells(struct device_node *np,
> > > > +                                           const char *prop, int i,
> > >
> > > I like 'i' for for loops, but less so for function params. Perhaps
> > > 'index' instead like of_parse_phandle_with_args.
> >
> > Sounds good.
> >
> > >
> > > > +                                           const char *binding,
> > > > +                                           const char *cell)
> > > > +{
> > > > +       struct of_phandle_args sup_args;
> > > > +
> > > > +       if (!i && strcmp(prop, binding))
> > >
> > > Why the '!i' test?
> >
> > To avoid a string comparison for every index. It's kinda wasteful once
> > the first index passes.
>
> That's not very obvious and pretty fragile though this is a static
> function. Perhaps we should split to match() and parse() functions.

Yeah, I did think about doing this. That's why I made it a struct for
supplier_bindings instead of just an array of function pointers. But
having a parse function just for a strcmp() was creating a lot of code
noise. So went ahead and did it this way. We can keep it this way and
if we later see the need for a separate parse function, it should be
easy to do so (because it's already a struct for each binding).

> At
> least put a comment here as to what we're doing.

Done.

> >
> > > > +               return NULL;
> > > > +
> > > > +       if (of_parse_phandle_with_args(np, binding, cell, i, &sup_args))
> > > > +               return NULL;
> > > > +
> > > > +       return sup_args.np;
> > > > +}
> > > > +
> > > > +static struct device_node *parse_clocks(struct device_node *np,
> > > > +                                       const char *prop, int i)
> > > > +{
> > > > +       return parse_prop_cells(np, prop, i, "clocks", "#clock-cells");
> > > > +}
> > > > +
> > > > +static struct device_node *parse_interconnects(struct device_node *np,
> > > > +                                              const char *prop, int i)
> > > > +{
> > > > +       return parse_prop_cells(np, prop, i, "interconnects",
> > > > +                               "#interconnect-cells");
> > > > +}
> > > > +
> > > > +static int strcmp_suffix(const char *str, const char *suffix)
> > > > +{
> > > > +       unsigned int len, suffix_len;
> > > > +
> > > > +       len = strlen(str);
> > > > +       suffix_len = strlen(suffix);
> > > > +       if (len <= suffix_len)
> > > > +               return -1;
> > > > +       return strcmp(str + len - suffix_len, suffix);
> > > > +}
> > > > +
> > > > +static struct device_node *parse_regulators(struct device_node *np,
> > > > +                                           const char *prop, int i)
> > > > +{
> > > > +       if (i || strcmp_suffix(prop, "-supply"))
> > > > +               return NULL;
> > > > +
> > > > +       return of_parse_phandle(np, prop, 0);
> > > > +}
> > > > +
> > > > +/**
> > > > + * struct supplier_bindings - Information for parsing supplier DT binding
> > > > + *
> > > > + * @parse_prop:                If the function cannot parse the property, return NULL.
> > > > + *                     Otherwise, return the phandle listed in the property
> > > > + *                     that corresponds to index i.
> > > > + */
> > > > +struct supplier_bindings {
> > > > +       struct device_node *(*parse_prop)(struct device_node *np,
> > > > +                                         const char *name, int i);
> > > > +};
> > > > +
> > > > +struct supplier_bindings bindings[] = {
> > >
> > > static const
> >
> > Will do.
> >
> > >
> > > > +       { .parse_prop = parse_clocks, },
> > > > +       { .parse_prop = parse_interconnects, },
> > > > +       { .parse_prop = parse_regulators, },
> > > > +       { },
> > > > +};
> > > > +
> > > > +static bool of_link_property(struct device *dev, struct device_node *con_np,
> > > > +                            const char *prop)
> > > > +{
> > > > +       struct device_node *phandle;
> > > > +       struct supplier_bindings *s = bindings;
> > > > +       unsigned int i = 0;
> > > > +       bool done = true;
> > > > +
> > > > +       while (!i && s->parse_prop) {
> > >
> > > Using 'i' is a little odd. Perhaps a 'matched' bool would be easier to read.
> >
> > That's how I wrote it first (locally) and then redid it this way
> > because the bool felt very superfluous. I don't think this is that
> > hard to understand.
>
> Alright...

I like the name "matched" over "found" that I had used locally. So, I
actually went ahead and did this.

-Saravana

> > > > +               while ((phandle = s->parse_prop(con_np, prop, i))) {
> > > > +                       i++;
> > > > +                       if (of_link_to_phandle(dev, phandle))
> > > > +                               done = false;
> > >
> > > Just return here. No point in continuing as 'done' is never set back to true.
> >
> > Actually, there is a point for this. Say Device-C depends on suppliers
> > Device-S1 and Device-S2 and they are listed in DT in that order.
> >
> > Say, S1 gets populated after late_initcall_sync but S2 is probes way
> > before that. If I don't continue past a "failed linking" to S1 and
> > also link up to S2, then S2 will get a sync_state() callback before C
> > is probed. So I have to go through all possible suppliers and as many
> > as possible.
> >
> > Let me add a comment about this somewhere in the code (probably the
> > header that defines the add_links() ops).
>
> Okay, makes sense.
>
> Rob

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

* Re: [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings
@ 2019-07-23 23:57           ` Saravana Kannan
  0 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2019-07-23 23:57 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 23, 2019 at 3:18 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Jul 23, 2019 at 2:49 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Tue, Jul 23, 2019 at 11:06 AM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Sat, Jul 20, 2019 at 12:17 AM 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.
> > >
> > > The structure now looks a lot better to me. A few minor things below.
> >
> > Thanks.
> >
> > > >
> > > > 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                         | 158 ++++++++++++++++++
> > > >  2 files changed, 163 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..88a2086e26fa 100644
> > > > --- a/drivers/of/platform.c
> > > > +++ b/drivers/of/platform.c
> > > > @@ -509,6 +509,163 @@ 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);
> > > > +       /*
> > > > +        * Don't allow linking a device node as a consumer of one of its
> > > > +        * descendant nodes. By definition, a child node can't be a functional
> > > > +        * dependency for the parent node.
> > > > +        */
> > > > +       while (sup) {
> > > > +               if (sup == con) {
> > > > +                       of_node_put(sup);
> > > > +                       return false;
> > > > +               }
> > > > +               sup = of_get_next_parent(sup);
> > > > +       }
> > > > +       return true;
> > > > +}
> > > > +
> > > > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> > > > +{
> > > > +       struct platform_device *sup_dev;
> > > > +       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > > > +       int ret = 0;
> > > > +
> > > > +       /*
> > > > +        * 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);
> > > > +               return 0;
> > > > +       }
> > > > +       sup_dev = of_find_device_by_node(sup_np);
> > > > +       of_node_put(sup_np);
> > > > +       if (!sup_dev)
> > > > +               return -ENODEV;
> > > > +       if (!device_link_add(dev, &sup_dev->dev, dl_flags))
> > > > +               ret = -ENODEV;
> > > > +       put_device(&sup_dev->dev);
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +static struct device_node *parse_prop_cells(struct device_node *np,
> > > > +                                           const char *prop, int i,
> > >
> > > I like 'i' for for loops, but less so for function params. Perhaps
> > > 'index' instead like of_parse_phandle_with_args.
> >
> > Sounds good.
> >
> > >
> > > > +                                           const char *binding,
> > > > +                                           const char *cell)
> > > > +{
> > > > +       struct of_phandle_args sup_args;
> > > > +
> > > > +       if (!i && strcmp(prop, binding))
> > >
> > > Why the '!i' test?
> >
> > To avoid a string comparison for every index. It's kinda wasteful once
> > the first index passes.
>
> That's not very obvious and pretty fragile though this is a static
> function. Perhaps we should split to match() and parse() functions.

Yeah, I did think about doing this. That's why I made it a struct for
supplier_bindings instead of just an array of function pointers. But
having a parse function just for a strcmp() was creating a lot of code
noise. So went ahead and did it this way. We can keep it this way and
if we later see the need for a separate parse function, it should be
easy to do so (because it's already a struct for each binding).

> At
> least put a comment here as to what we're doing.

Done.

> >
> > > > +               return NULL;
> > > > +
> > > > +       if (of_parse_phandle_with_args(np, binding, cell, i, &sup_args))
> > > > +               return NULL;
> > > > +
> > > > +       return sup_args.np;
> > > > +}
> > > > +
> > > > +static struct device_node *parse_clocks(struct device_node *np,
> > > > +                                       const char *prop, int i)
> > > > +{
> > > > +       return parse_prop_cells(np, prop, i, "clocks", "#clock-cells");
> > > > +}
> > > > +
> > > > +static struct device_node *parse_interconnects(struct device_node *np,
> > > > +                                              const char *prop, int i)
> > > > +{
> > > > +       return parse_prop_cells(np, prop, i, "interconnects",
> > > > +                               "#interconnect-cells");
> > > > +}
> > > > +
> > > > +static int strcmp_suffix(const char *str, const char *suffix)
> > > > +{
> > > > +       unsigned int len, suffix_len;
> > > > +
> > > > +       len = strlen(str);
> > > > +       suffix_len = strlen(suffix);
> > > > +       if (len <= suffix_len)
> > > > +               return -1;
> > > > +       return strcmp(str + len - suffix_len, suffix);
> > > > +}
> > > > +
> > > > +static struct device_node *parse_regulators(struct device_node *np,
> > > > +                                           const char *prop, int i)
> > > > +{
> > > > +       if (i || strcmp_suffix(prop, "-supply"))
> > > > +               return NULL;
> > > > +
> > > > +       return of_parse_phandle(np, prop, 0);
> > > > +}
> > > > +
> > > > +/**
> > > > + * struct supplier_bindings - Information for parsing supplier DT binding
> > > > + *
> > > > + * @parse_prop:                If the function cannot parse the property, return NULL.
> > > > + *                     Otherwise, return the phandle listed in the property
> > > > + *                     that corresponds to index i.
> > > > + */
> > > > +struct supplier_bindings {
> > > > +       struct device_node *(*parse_prop)(struct device_node *np,
> > > > +                                         const char *name, int i);
> > > > +};
> > > > +
> > > > +struct supplier_bindings bindings[] = {
> > >
> > > static const
> >
> > Will do.
> >
> > >
> > > > +       { .parse_prop = parse_clocks, },
> > > > +       { .parse_prop = parse_interconnects, },
> > > > +       { .parse_prop = parse_regulators, },
> > > > +       { },
> > > > +};
> > > > +
> > > > +static bool of_link_property(struct device *dev, struct device_node *con_np,
> > > > +                            const char *prop)
> > > > +{
> > > > +       struct device_node *phandle;
> > > > +       struct supplier_bindings *s = bindings;
> > > > +       unsigned int i = 0;
> > > > +       bool done = true;
> > > > +
> > > > +       while (!i && s->parse_prop) {
> > >
> > > Using 'i' is a little odd. Perhaps a 'matched' bool would be easier to read.
> >
> > That's how I wrote it first (locally) and then redid it this way
> > because the bool felt very superfluous. I don't think this is that
> > hard to understand.
>
> Alright...

I like the name "matched" over "found" that I had used locally. So, I
actually went ahead and did this.

-Saravana

> > > > +               while ((phandle = s->parse_prop(con_np, prop, i))) {
> > > > +                       i++;
> > > > +                       if (of_link_to_phandle(dev, phandle))
> > > > +                               done = false;
> > >
> > > Just return here. No point in continuing as 'done' is never set back to true.
> >
> > Actually, there is a point for this. Say Device-C depends on suppliers
> > Device-S1 and Device-S2 and they are listed in DT in that order.
> >
> > Say, S1 gets populated after late_initcall_sync but S2 is probes way
> > before that. If I don't continue past a "failed linking" to S1 and
> > also link up to S2, then S2 will get a sync_state() callback before C
> > is probed. So I have to go through all possible suppliers and as many
> > as possible.
> >
> > Let me add a comment about this somewhere in the code (probably the
> > header that defines the add_links() ops).
>
> Okay, makes sense.
>
> Rob

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

end of thread, other threads:[~2019-07-23 23:58 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-20  6:16 [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
2019-07-20  6:16 ` [PATCH v6 1/7] driver core: Add support for linking devices during device addition Saravana Kannan
2019-07-20  6:16 ` [PATCH v6 2/7] driver core: Add edit_links() callback for drivers Saravana Kannan
2019-07-20  6:16 ` [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings Saravana Kannan
2019-07-23 18:06   ` Rob Herring
2019-07-23 20:48     ` Saravana Kannan
2019-07-23 20:48       ` Saravana Kannan
2019-07-23 22:18       ` Rob Herring
2019-07-23 22:18         ` Rob Herring
2019-07-23 23:57         ` Saravana Kannan
2019-07-23 23:57           ` Saravana Kannan
2019-07-20  6:16 ` [PATCH v6 4/7] driver core: Add sync_state driver/bus callback Saravana Kannan
2019-07-20  6:16 ` [PATCH v6 5/7] of/platform: Pause/resume sync state during init and of_platform_populate() Saravana Kannan
2019-07-20  6:16 ` [PATCH v6 6/7] of/platform: Create device links for all child-supplier depencencies Saravana Kannan
2019-07-20  6:16 ` [PATCH v6 7/7] of/platform: Don't create device links for default busses Saravana Kannan
2019-07-22 23:47 ` [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
2019-07-22 23:47   ` Saravana Kannan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.