[PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Saravana Kannan]

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.

v6 -> v7:
- No functional changes.
- Renamed i to index
- Added comment to clarify not having to check property name for every
  index
- Added "matched" variable to clarify code. No functional change.
- Added comments to include/linux/device.h for add_links()

v7 -> v8:
- Rebased on top of linux-next to handle device link changes in [1]

v8 -> v9:
- Fixed kbuild test bot reported errors (docs and const)

[1] - https://lore.kernel.org/lkml/2305283.AStDPdUUnE@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                         | 189 ++++++++++++++++++
 include/linux/device.h                        |  60 ++++++
 5 files changed, 451 insertions(+)

-- 
2.22.0.709.g102302147b-goog

[PATCH v9 1/7] driver core: Add support for linking devices during device addition

[Saravana Kannan]

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 | 14 +++++++
 2 files changed, 97 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 950e3bd0f45c..62d416e667bd 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);
@@ -416,6 +418,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))
@@ -550,6 +597,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) {
@@ -834,6 +894,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).
@@ -1698,6 +1762,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);
@@ -2133,6 +2198,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 5093691f56e7..dbcf1de5e9fa 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -78,6 +78,17 @@ 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 per device, after a device is
+ *		added to this bus.  The function is expected to create device
+ *		links to all the suppliers of the input device that are
+ *		available at the time this function is called.  As in, the
+ *		function should NOT stop at the first failed device link if
+ *		other unlinked supplier devices are present in the system.
+ *
+ *		Return 0 if device links have been successfully created to all
+ *		the suppliers of this device.  Return an error if some of the
+ *		suppliers are not yet available and this function 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.
@@ -122,6 +133,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);
@@ -895,11 +907,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.709.g102302147b-goog

[PATCH v9 2/7] driver core: Add edit_links() callback for drivers

[Saravana Kannan]

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.

kbuild test robot reported missing documentation for device.has_edit_links
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 24 ++++++++++++++++++++++--
 drivers/base/dd.c      | 29 +++++++++++++++++++++++++++++
 include/linux/device.h | 20 ++++++++++++++++++++
 3 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 62d416e667bd..fec2e8ae75fe 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -437,6 +437,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
  *
@@ -454,12 +467,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 994a90747420..5e7041ede0d7 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -698,6 +698,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);
@@ -786,6 +792,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 dbcf1de5e9fa..4e18337f99fd 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -263,6 +263,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.
@@ -302,6 +316,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);
@@ -989,6 +1004,8 @@ struct dev_links_info {
  * @offline:	Set after successful invocation of bus type's .offline().
  * @of_node_reused: Set if the device-tree node is shared with an ancestor
  *              device.
+ * @has_edit_links: This device has a driver than is capable of
+ *		    editing the device links created by driver core.
  * @dma_coherent: this particular device is dma coherent, even if the
  *		architecture supports non-coherent devices.
  *
@@ -1085,6 +1102,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)
@@ -1336,6 +1354,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);
 
@@ -1427,6 +1446,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.709.g102302147b-goog

[PATCH v9 3/7] of/platform: Add functional dependency link from DT bindings

[Saravana Kannan]

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.

kbuild test robot reported clang error about missing const
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 .../admin-guide/kernel-parameters.txt         |   5 +
 drivers/of/platform.c                         | 165 ++++++++++++++++++
 2 files changed, 170 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 7ccd158b3894..dba3200d3516 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3170,6 +3170,11 @@
 			This can be set from sysctl after boot.
 			See Documentation/admin-guide/sysctl/vm.rst 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 7801e25e6895..64c4b91988f2 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -508,6 +508,170 @@ 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 index,
+					    const char *binding,
+					    const char *cell)
+{
+	struct of_phandle_args sup_args;
+
+	/* Don't need to check property name for every index. */
+	if (!index && strcmp(prop, binding))
+		return NULL;
+
+	if (of_parse_phandle_with_args(np, binding, cell, index, &sup_args))
+		return NULL;
+
+	return sup_args.np;
+}
+
+static struct device_node *parse_clocks(struct device_node *np,
+					const char *prop, int index)
+{
+	return parse_prop_cells(np, prop, index, "clocks", "#clock-cells");
+}
+
+static struct device_node *parse_interconnects(struct device_node *np,
+					       const char *prop, int index)
+{
+	return parse_prop_cells(np, prop, index, "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 index)
+{
+	if (index || 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 the index.
+ */
+struct supplier_bindings {
+	struct device_node *(*parse_prop)(struct device_node *np,
+					  const char *name, int index);
+};
+
+static const 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;
+	const struct supplier_bindings *s = bindings;
+	unsigned int i = 0;
+	bool done = true, matched = false;
+
+	while (!matched && s->parse_prop) {
+		while ((phandle = s->parse_prop(con_np, prop, i))) {
+			matched = true;
+			i++;
+			if (of_link_to_phandle(dev, phandle))
+				/*
+				 * Don't stop at the first failure. See
+				 * Documentation for bus_type.add_links for
+				 * more details.
+				 */
+				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" },
@@ -523,6 +687,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.709.g102302147b-goog

[PATCH v9 4/7] driver core: Add sync_state driver/bus callback

[Saravana Kannan]

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.

kbuild test robot reported missing documentation for device.state_synced
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 65 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h | 26 +++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index fec2e8ae75fe..8528b5298e14 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);
@@ -649,6 +651,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_MANAGED))
+			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.
@@ -693,6 +751,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;
@@ -809,6 +872,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();
@@ -1783,6 +1847,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 4e18337f99fd..4d43b1e4b2c2 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -84,6 +84,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  *		available at the time this function is called.  As in, the
  *		function should NOT stop at the first failed device link if
  *		other unlinked supplier devices are present in the system.
+ *		This is necessary for the sync_state() callback to work
+ *		correctly.
  *
  *		Return 0 if device links have been successfully created to all
  *		the suppliers of this device.  Return an error if some of the
@@ -91,6 +93,13 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  *		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.
+ * @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.
  *
@@ -135,6 +144,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);
 
@@ -280,6 +290,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.
@@ -318,6 +335,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);
@@ -923,12 +941,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;
 };
 
@@ -1006,6 +1026,9 @@ struct dev_links_info {
  *              device.
  * @has_edit_links: This device has a driver than is capable of
  *		    editing the device links created by driver core.
+ * @state_synced: The hardware state of this device has been synced to match
+ *		  the software state of this device by calling the driver/bus
+ *		  sync_state() callback.
  * @dma_coherent: this particular device is dma coherent, even if the
  *		architecture supports non-coherent devices.
  *
@@ -1103,6 +1126,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)
@@ -1447,6 +1471,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.709.g102302147b-goog

[PATCH v9 5/7] of/platform: Pause/resume sync state during init and of_platform_populate()

[Saravana Kannan]

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 64c4b91988f2..6c9c8dcee912 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -485,6 +485,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) {
@@ -492,6 +493,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);
@@ -688,6 +691,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
@@ -708,6 +712,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.709.g102302147b-goog

[PATCH v9 6/7] of/platform: Create device links for all child-supplier depencencies

[Saravana Kannan]

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 6c9c8dcee912..36e25136e807 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -655,24 +655,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.709.g102302147b-goog

[PATCH v9 7/7] of/platform: Don't create device links for default busses

[Saravana Kannan]

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 36e25136e807..33cac801e50b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -682,6 +682,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.709.g102302147b-goog

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Greg Kroah-Hartman]

On Wed, Jul 31, 2019 at 03:17:13PM -0700, Saravana Kannan 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.

All now queued up in my driver-core-testing branch, and if 0-day is
happy with this, will move it to my "real" driver-core-next branch in a
day or so to get included in linux-next.

thanks for sticking with this!

greg k-h

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Frank Rowand]

Hi Greg,

On 7/31/19 11:12 PM, Greg Kroah-Hartman wrote:
> On Wed, Jul 31, 2019 at 03:17:13PM -0700, Saravana Kannan 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.
> 
> All now queued up in my driver-core-testing branch, and if 0-day is
> happy with this, will move it to my "real" driver-core-next branch in a
> day or so to get included in linux-next.

I have been slow in getting my review out.

This patch series is not yet ready for sending to Linus, so if putting
this in linux-next implies that it will be in your next pull request
to Linus, please do not put it in linux-next.

Thanks,

Frank

> 
> thanks for sticking with this!
> 
> greg k-h
> 

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Greg Kroah-Hartman]

On Thu, Aug 01, 2019 at 12:28:13PM -0700, Frank Rowand wrote:
> Hi Greg,
> 
> On 7/31/19 11:12 PM, Greg Kroah-Hartman wrote:
> > On Wed, Jul 31, 2019 at 03:17:13PM -0700, Saravana Kannan 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.
> > 
> > All now queued up in my driver-core-testing branch, and if 0-day is
> > happy with this, will move it to my "real" driver-core-next branch in a
> > day or so to get included in linux-next.
> 
> I have been slow in getting my review out.
> 
> This patch series is not yet ready for sending to Linus, so if putting
> this in linux-next implies that it will be in your next pull request
> to Linus, please do not put it in linux-next.

It means that it will be in my pull request for 5.4-rc1, many many
waeeks away from now.

thanks,

greg k-h

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Frank Rowand]

On 8/1/19 12:32 PM, Greg Kroah-Hartman wrote:
> On Thu, Aug 01, 2019 at 12:28:13PM -0700, Frank Rowand wrote:
>> Hi Greg,
>>
>> On 7/31/19 11:12 PM, Greg Kroah-Hartman wrote:
>>> On Wed, Jul 31, 2019 at 03:17:13PM -0700, Saravana Kannan 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.
>>>
>>> All now queued up in my driver-core-testing branch, and if 0-day is
>>> happy with this, will move it to my "real" driver-core-next branch in a
>>> day or so to get included in linux-next.
>>
>> I have been slow in getting my review out.
>>
>> This patch series is not yet ready for sending to Linus, so if putting
>> this in linux-next implies that it will be in your next pull request
>> to Linus, please do not put it in linux-next.
> 
> It means that it will be in my pull request for 5.4-rc1, many many
> waeeks away from now.

If you are willing to revert the series before the pull request _if_ I
have significant review issues in the next couple of days, then I am happy
to see the patches get exposure in linux-next.

-Frank

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Greg Kroah-Hartman]

On Thu, Aug 01, 2019 at 12:59:25PM -0700, Frank Rowand wrote:
> On 8/1/19 12:32 PM, Greg Kroah-Hartman wrote:
> > On Thu, Aug 01, 2019 at 12:28:13PM -0700, Frank Rowand wrote:
> >> Hi Greg,
> >>
> >> On 7/31/19 11:12 PM, Greg Kroah-Hartman wrote:
> >>> On Wed, Jul 31, 2019 at 03:17:13PM -0700, Saravana Kannan 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.
> >>>
> >>> All now queued up in my driver-core-testing branch, and if 0-day is
> >>> happy with this, will move it to my "real" driver-core-next branch in a
> >>> day or so to get included in linux-next.
> >>
> >> I have been slow in getting my review out.
> >>
> >> This patch series is not yet ready for sending to Linus, so if putting
> >> this in linux-next implies that it will be in your next pull request
> >> to Linus, please do not put it in linux-next.
> > 
> > It means that it will be in my pull request for 5.4-rc1, many many
> > waeeks away from now.
> 
> If you are willing to revert the series before the pull request _if_ I
> have significant review issues in the next couple of days, then I am happy
> to see the patches get exposure in linux-next.

If you have significant review issues, yes, I will be glad to revert them.

thanks,

greg k-h

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Frank Rowand]

Hi Greg, Saravana,

On 8/1/19 11:37 PM, Greg Kroah-Hartman wrote:
> On Thu, Aug 01, 2019 at 12:59:25PM -0700, Frank Rowand wrote:
>> On 8/1/19 12:32 PM, Greg Kroah-Hartman wrote:
>>> On Thu, Aug 01, 2019 at 12:28:13PM -0700, Frank Rowand wrote:
>>>> Hi Greg,
>>>>
>>>> On 7/31/19 11:12 PM, Greg Kroah-Hartman wrote:
>>>>> On Wed, Jul 31, 2019 at 03:17:13PM -0700, Saravana Kannan 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.
>>>>>
>>>>> All now queued up in my driver-core-testing branch, and if 0-day is
>>>>> happy with this, will move it to my "real" driver-core-next branch in a
>>>>> day or so to get included in linux-next.
>>>>
>>>> I have been slow in getting my review out.
>>>>
>>>> This patch series is not yet ready for sending to Linus, so if putting
>>>> this in linux-next implies that it will be in your next pull request
>>>> to Linus, please do not put it in linux-next.
>>>
>>> It means that it will be in my pull request for 5.4-rc1, many many
>>> waeeks away from now.
>>
>> If you are willing to revert the series before the pull request _if_ I
>> have significant review issues in the next couple of days, then I am happy
>> to see the patches get exposure in linux-next.
> 
> If you have significant review issues, yes, I will be glad to revert them.

Just a heads up that I have sent review issues in reply to version 7 of this
patch series.

We'll see what the responses are to my review comments, but I am expecting
the changes are big enough to result in a new version (or a couple more
versions) of the patch series.

No rush to revert version 9 since your 5.4-rc1 pull request is still not
near, and I am glad for whatever exposure these patches are getting in
linux-next.

Thanks,

Frank

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Frank Rowand]

Hi Saravana,

On 7/31/19 3:17 PM, Saravana Kannan 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.
> 
> v6 -> v7:
> - No functional changes.
> - Renamed i to index
> - Added comment to clarify not having to check property name for every
>   index
> - Added "matched" variable to clarify code. No functional change.
> - Added comments to include/linux/device.h for add_links()
> 
> v7 -> v8:
> - Rebased on top of linux-next to handle device link changes in [1]
> 


> v8 -> v9:
> - Fixed kbuild test bot reported errors (docs and const)

Some maintainers have strong opinions about whether change logs should be:

  (1) only in patch 0
  (2) only in the specific patches that are changed
  (3) both in patch 0 and in the specific patches that are changed.

I can adapt to any of the three styles.  But for style "(1)" please
list which specific patch has changed for each item in the change list.

-Frank


> 
> [1] - https://lore.kernel.org/lkml/2305283.AStDPdUUnE@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                         | 189 ++++++++++++++++++
>  include/linux/device.h                        |  60 ++++++
>  5 files changed, 451 insertions(+)
> 

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Saravana Kannan]

On Fri, Aug 9, 2019 at 7:57 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> Hi Saravana,
>
> On 7/31/19 3:17 PM, Saravana Kannan 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.
> >
> > v6 -> v7:
> > - No functional changes.
> > - Renamed i to index
> > - Added comment to clarify not having to check property name for every
> >   index
> > - Added "matched" variable to clarify code. No functional change.
> > - Added comments to include/linux/device.h for add_links()
> >
> > v7 -> v8:
> > - Rebased on top of linux-next to handle device link changes in [1]
> >
>
>
> > v8 -> v9:
> > - Fixed kbuild test bot reported errors (docs and const)
>
> Some maintainers have strong opinions about whether change logs should be:
>
>   (1) only in patch 0
>   (2) only in the specific patches that are changed
>   (3) both in patch 0 and in the specific patches that are changed.
>
> I can adapt to any of the three styles.  But for style "(1)" please
> list which specific patch has changed for each item in the change list.
>

Thanks for the context Frank. I'm okay with (1) or (2) but I'll stick
with (1) for this series. Didn't realize there were options (2) and
(3). Since you started reviewing from v7, I'll do that in the future
updates? Also, I haven't forgotten your emails. Just tied up with
something else for a few days. I'll get to your emails next week.

Thanks,
Saravana

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Frank Rowand]

On 8/9/19 10:00 PM, Saravana Kannan wrote:
> On Fri, Aug 9, 2019 at 7:57 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> Hi Saravana,
>>
>> On 7/31/19 3:17 PM, Saravana Kannan 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.
>>>
>>> v6 -> v7:
>>> - No functional changes.
>>> - Renamed i to index
>>> - Added comment to clarify not having to check property name for every
>>>   index
>>> - Added "matched" variable to clarify code. No functional change.
>>> - Added comments to include/linux/device.h for add_links()
>>>
>>> v7 -> v8:
>>> - Rebased on top of linux-next to handle device link changes in [1]
>>>
>>
>>
>>> v8 -> v9:
>>> - Fixed kbuild test bot reported errors (docs and const)
>>
>> Some maintainers have strong opinions about whether change logs should be:
>>
>>   (1) only in patch 0
>>   (2) only in the specific patches that are changed
>>   (3) both in patch 0 and in the specific patches that are changed.
>>
>> I can adapt to any of the three styles.  But for style "(1)" please
>> list which specific patch has changed for each item in the change list.
>>
> 
> Thanks for the context Frank. I'm okay with (1) or (2) but I'll stick
> with (1) for this series. Didn't realize there were options (2) and
> (3). Since you started reviewing from v7, I'll do that in the future
> updates? Also, I haven't forgotten your emails. Just tied up with
> something else for a few days. I'll get to your emails next week.

Yes, starting with future updates is fine, no need to redo the v9
change logs.

No problem on the timing.  I figured you were busy or away from the
internet.

-Frank

> 
> Thanks,
> Saravana
> 

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Saravana Kannan]

On Fri, Aug 9, 2019 at 10:20 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 8/9/19 10:00 PM, Saravana Kannan wrote:
> > On Fri, Aug 9, 2019 at 7:57 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> Hi Saravana,
> >>
> >> On 7/31/19 3:17 PM, Saravana Kannan 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.
> >>>
> >>> v6 -> v7:
> >>> - No functional changes.
> >>> - Renamed i to index
> >>> - Added comment to clarify not having to check property name for every
> >>>   index
> >>> - Added "matched" variable to clarify code. No functional change.
> >>> - Added comments to include/linux/device.h for add_links()
> >>>
> >>> v7 -> v8:
> >>> - Rebased on top of linux-next to handle device link changes in [1]
> >>>
> >>
> >>
> >>> v8 -> v9:
> >>> - Fixed kbuild test bot reported errors (docs and const)
> >>
> >> Some maintainers have strong opinions about whether change logs should be:
> >>
> >>   (1) only in patch 0
> >>   (2) only in the specific patches that are changed
> >>   (3) both in patch 0 and in the specific patches that are changed.
> >>
> >> I can adapt to any of the three styles.  But for style "(1)" please
> >> list which specific patch has changed for each item in the change list.
> >>
> >
> > Thanks for the context Frank. I'm okay with (1) or (2) but I'll stick
> > with (1) for this series. Didn't realize there were options (2) and
> > (3). Since you started reviewing from v7, I'll do that in the future
> > updates? Also, I haven't forgotten your emails. Just tied up with
> > something else for a few days. I'll get to your emails next week.
>
> Yes, starting with future updates is fine, no need to redo the v9
> change logs.
>
> No problem on the timing.  I figured you were busy or away from the
> internet.

I'm replying to your comments on the other 3 patches. Okay with a
majority of them. I'll wait for your reply to see where we settle for
some of the points before I send out any patches though.

For now I'm thinking of sending them as separate clean up patches so
that Greg doesn't have to deal with reverts in his "next" branch. We
can squash them later if we really need to rip out what's in there and
push it again.

-Saravana

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Frank Rowand]

Hi Saravana,

On 8/15/19 6:50 PM, Saravana Kannan wrote:
> On Fri, Aug 9, 2019 at 10:20 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 8/9/19 10:00 PM, Saravana Kannan wrote:
>>> On Fri, Aug 9, 2019 at 7:57 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> Hi Saravana,
>>>>
>>>> On 7/31/19 3:17 PM, Saravana Kannan 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.
>>>>>
>>>>> v6 -> v7:
>>>>> - No functional changes.
>>>>> - Renamed i to index
>>>>> - Added comment to clarify not having to check property name for every
>>>>>   index
>>>>> - Added "matched" variable to clarify code. No functional change.
>>>>> - Added comments to include/linux/device.h for add_links()
>>>>>
>>>>> v7 -> v8:
>>>>> - Rebased on top of linux-next to handle device link changes in [1]
>>>>>
>>>>
>>>>
>>>>> v8 -> v9:
>>>>> - Fixed kbuild test bot reported errors (docs and const)
>>>>
>>>> Some maintainers have strong opinions about whether change logs should be:
>>>>
>>>>   (1) only in patch 0
>>>>   (2) only in the specific patches that are changed
>>>>   (3) both in patch 0 and in the specific patches that are changed.
>>>>
>>>> I can adapt to any of the three styles.  But for style "(1)" please
>>>> list which specific patch has changed for each item in the change list.
>>>>
>>>
>>> Thanks for the context Frank. I'm okay with (1) or (2) but I'll stick
>>> with (1) for this series. Didn't realize there were options (2) and
>>> (3). Since you started reviewing from v7, I'll do that in the future
>>> updates? Also, I haven't forgotten your emails. Just tied up with
>>> something else for a few days. I'll get to your emails next week.
>>
>> Yes, starting with future updates is fine, no need to redo the v9
>> change logs.
>>
>> No problem on the timing.  I figured you were busy or away from the
>> internet.
> 
> I'm replying to your comments on the other 3 patches. Okay with a
> majority of them. I'll wait for your reply to see where we settle for
> some of the points before I send out any patches though.
> 
> For now I'm thinking of sending them as separate clean up patches so
> that Greg doesn't have to deal with reverts in his "next" branch. We
> can squash them later if we really need to rip out what's in there and
> push it again.
> 
> -Saravana
> 

Please do not do separate clean up patches.  The series that Greg has is
not ready for acceptance and I am going to ask him to revert it as we
work through the needed changes.

I suspect there will be at least two more versions of the series.  The
first is to get the patches I commented in good shape.  Then I will
look at the patches later in the series to see how they fit into the
big picture.

In the end, there should be one coherent patch series that implements
the feature.

-Frank

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Greg Kroah-Hartman]

On Thu, Aug 15, 2019 at 08:09:19PM -0700, Frank Rowand wrote:
> Hi Saravana,
> 
> On 8/15/19 6:50 PM, Saravana Kannan wrote:
> > On Fri, Aug 9, 2019 at 10:20 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> On 8/9/19 10:00 PM, Saravana Kannan wrote:
> >>> On Fri, Aug 9, 2019 at 7:57 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>
> >>>> Hi Saravana,
> >>>>
> >>>> On 7/31/19 3:17 PM, Saravana Kannan 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.
> >>>>>
> >>>>> v6 -> v7:
> >>>>> - No functional changes.
> >>>>> - Renamed i to index
> >>>>> - Added comment to clarify not having to check property name for every
> >>>>>   index
> >>>>> - Added "matched" variable to clarify code. No functional change.
> >>>>> - Added comments to include/linux/device.h for add_links()
> >>>>>
> >>>>> v7 -> v8:
> >>>>> - Rebased on top of linux-next to handle device link changes in [1]
> >>>>>
> >>>>
> >>>>
> >>>>> v8 -> v9:
> >>>>> - Fixed kbuild test bot reported errors (docs and const)
> >>>>
> >>>> Some maintainers have strong opinions about whether change logs should be:
> >>>>
> >>>>   (1) only in patch 0
> >>>>   (2) only in the specific patches that are changed
> >>>>   (3) both in patch 0 and in the specific patches that are changed.
> >>>>
> >>>> I can adapt to any of the three styles.  But for style "(1)" please
> >>>> list which specific patch has changed for each item in the change list.
> >>>>
> >>>
> >>> Thanks for the context Frank. I'm okay with (1) or (2) but I'll stick
> >>> with (1) for this series. Didn't realize there were options (2) and
> >>> (3). Since you started reviewing from v7, I'll do that in the future
> >>> updates? Also, I haven't forgotten your emails. Just tied up with
> >>> something else for a few days. I'll get to your emails next week.
> >>
> >> Yes, starting with future updates is fine, no need to redo the v9
> >> change logs.
> >>
> >> No problem on the timing.  I figured you were busy or away from the
> >> internet.
> > 
> > I'm replying to your comments on the other 3 patches. Okay with a
> > majority of them. I'll wait for your reply to see where we settle for
> > some of the points before I send out any patches though.
> > 
> > For now I'm thinking of sending them as separate clean up patches so
> > that Greg doesn't have to deal with reverts in his "next" branch. We
> > can squash them later if we really need to rip out what's in there and
> > push it again.
> > 
> > -Saravana
> > 
> 
> Please do not do separate clean up patches.  The series that Greg has is
> not ready for acceptance and I am going to ask him to revert it as we
> work through the needed changes.
> 
> I suspect there will be at least two more versions of the series.  The
> first is to get the patches I commented in good shape.  Then I will
> look at the patches later in the series to see how they fit into the
> big picture.
> 
> In the end, there should be one coherent patch series that implements
> the feature.

Incremental patches to fix up the comments and documentation is fine, no
need to respin the whole mess.

thanks,

greg k-h

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Frank Rowand]

i Greg,

On 8/16/19 2:10 AM, Greg Kroah-Hartman wrote:
> On Thu, Aug 15, 2019 at 08:09:19PM -0700, Frank Rowand wrote:
>> Hi Saravana,
>>
>> On 8/15/19 6:50 PM, Saravana Kannan wrote:
>>> On Fri, Aug 9, 2019 at 10:20 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> On 8/9/19 10:00 PM, Saravana Kannan wrote:
>>>>> On Fri, Aug 9, 2019 at 7:57 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>
>>>>>> Hi Saravana,
>>>>>>
>>>>>> On 7/31/19 3:17 PM, Saravana Kannan 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.
>>>>>>>
>>>>>>> v6 -> v7:
>>>>>>> - No functional changes.
>>>>>>> - Renamed i to index
>>>>>>> - Added comment to clarify not having to check property name for every
>>>>>>>   index
>>>>>>> - Added "matched" variable to clarify code. No functional change.
>>>>>>> - Added comments to include/linux/device.h for add_links()
>>>>>>>
>>>>>>> v7 -> v8:
>>>>>>> - Rebased on top of linux-next to handle device link changes in [1]
>>>>>>>
>>>>>>
>>>>>>
>>>>>>> v8 -> v9:
>>>>>>> - Fixed kbuild test bot reported errors (docs and const)
>>>>>>
>>>>>> Some maintainers have strong opinions about whether change logs should be:
>>>>>>
>>>>>>   (1) only in patch 0
>>>>>>   (2) only in the specific patches that are changed
>>>>>>   (3) both in patch 0 and in the specific patches that are changed.
>>>>>>
>>>>>> I can adapt to any of the three styles.  But for style "(1)" please
>>>>>> list which specific patch has changed for each item in the change list.
>>>>>>
>>>>>
>>>>> Thanks for the context Frank. I'm okay with (1) or (2) but I'll stick
>>>>> with (1) for this series. Didn't realize there were options (2) and
>>>>> (3). Since you started reviewing from v7, I'll do that in the future
>>>>> updates? Also, I haven't forgotten your emails. Just tied up with
>>>>> something else for a few days. I'll get to your emails next week.
>>>>
>>>> Yes, starting with future updates is fine, no need to redo the v9
>>>> change logs.
>>>>
>>>> No problem on the timing.  I figured you were busy or away from the
>>>> internet.
>>>
>>> I'm replying to your comments on the other 3 patches. Okay with a
>>> majority of them. I'll wait for your reply to see where we settle for
>>> some of the points before I send out any patches though.
>>>
>>> For now I'm thinking of sending them as separate clean up patches so
>>> that Greg doesn't have to deal with reverts in his "next" branch. We
>>> can squash them later if we really need to rip out what's in there and
>>> push it again.
>>>
>>> -Saravana
>>>
>>
>> Please do not do separate clean up patches.  The series that Greg has is
>> not ready for acceptance and I am going to ask him to revert it as we
>> work through the needed changes.
>>
>> I suspect there will be at least two more versions of the series.  The
>> first is to get the patches I commented in good shape.  Then I will
>> look at the patches later in the series to see how they fit into the
>> big picture.
>>
>> In the end, there should be one coherent patch series that implements
>> the feature.
> 
> Incremental patches to fix up the comments and documentation is fine, no
> need to respin the whole mess.

The problem is that the whole thing is a "mess" at this point.  I expect
the series to go through at least two or three more versions.

Please revert the series for now.

-Frank

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Greg Kroah-Hartman]

On Fri, Aug 16, 2019 at 07:05:06AM -0700, Frank Rowand wrote:
> i Greg,
> 
> On 8/16/19 2:10 AM, Greg Kroah-Hartman wrote:
> > On Thu, Aug 15, 2019 at 08:09:19PM -0700, Frank Rowand wrote:
> >> Hi Saravana,
> >>
> >> On 8/15/19 6:50 PM, Saravana Kannan wrote:
> >>> On Fri, Aug 9, 2019 at 10:20 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>
> >>>> On 8/9/19 10:00 PM, Saravana Kannan wrote:
> >>>>> On Fri, Aug 9, 2019 at 7:57 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>>>
> >>>>>> Hi Saravana,
> >>>>>>
> >>>>>> On 7/31/19 3:17 PM, Saravana Kannan 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.
> >>>>>>>
> >>>>>>> v6 -> v7:
> >>>>>>> - No functional changes.
> >>>>>>> - Renamed i to index
> >>>>>>> - Added comment to clarify not having to check property name for every
> >>>>>>>   index
> >>>>>>> - Added "matched" variable to clarify code. No functional change.
> >>>>>>> - Added comments to include/linux/device.h for add_links()
> >>>>>>>
> >>>>>>> v7 -> v8:
> >>>>>>> - Rebased on top of linux-next to handle device link changes in [1]
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> v8 -> v9:
> >>>>>>> - Fixed kbuild test bot reported errors (docs and const)
> >>>>>>
> >>>>>> Some maintainers have strong opinions about whether change logs should be:
> >>>>>>
> >>>>>>   (1) only in patch 0
> >>>>>>   (2) only in the specific patches that are changed
> >>>>>>   (3) both in patch 0 and in the specific patches that are changed.
> >>>>>>
> >>>>>> I can adapt to any of the three styles.  But for style "(1)" please
> >>>>>> list which specific patch has changed for each item in the change list.
> >>>>>>
> >>>>>
> >>>>> Thanks for the context Frank. I'm okay with (1) or (2) but I'll stick
> >>>>> with (1) for this series. Didn't realize there were options (2) and
> >>>>> (3). Since you started reviewing from v7, I'll do that in the future
> >>>>> updates? Also, I haven't forgotten your emails. Just tied up with
> >>>>> something else for a few days. I'll get to your emails next week.
> >>>>
> >>>> Yes, starting with future updates is fine, no need to redo the v9
> >>>> change logs.
> >>>>
> >>>> No problem on the timing.  I figured you were busy or away from the
> >>>> internet.
> >>>
> >>> I'm replying to your comments on the other 3 patches. Okay with a
> >>> majority of them. I'll wait for your reply to see where we settle for
> >>> some of the points before I send out any patches though.
> >>>
> >>> For now I'm thinking of sending them as separate clean up patches so
> >>> that Greg doesn't have to deal with reverts in his "next" branch. We
> >>> can squash them later if we really need to rip out what's in there and
> >>> push it again.
> >>>
> >>> -Saravana
> >>>
> >>
> >> Please do not do separate clean up patches.  The series that Greg has is
> >> not ready for acceptance and I am going to ask him to revert it as we
> >> work through the needed changes.
> >>
> >> I suspect there will be at least two more versions of the series.  The
> >> first is to get the patches I commented in good shape.  Then I will
> >> look at the patches later in the series to see how they fit into the
> >> big picture.
> >>
> >> In the end, there should be one coherent patch series that implements
> >> the feature.
> > 
> > Incremental patches to fix up the comments and documentation is fine, no
> > need to respin the whole mess.
> 
> The problem is that the whole thing is a "mess" at this point.  I expect
> the series to go through at least two or three more versions.

I'm confused.  All I see so far is objections about some documentation
in comments that can be cleaned up, and a disagreement about the name of
some things (naming is hard, tie goes to the submitter).

But no logic issues, right?  Documentation and names can be fixed
anytime, the logic is all working properly, right?

What am I missing here?

thanks,

greg k-h

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Frank Rowand]

On 8/16/19 8:23 AM, Greg Kroah-Hartman wrote:
> On Fri, Aug 16, 2019 at 07:05:06AM -0700, Frank Rowand wrote:
>> i Greg,
>>
>> On 8/16/19 2:10 AM, Greg Kroah-Hartman wrote:
>>> On Thu, Aug 15, 2019 at 08:09:19PM -0700, Frank Rowand wrote:
>>>> Hi Saravana,
>>>>
>>>> On 8/15/19 6:50 PM, Saravana Kannan wrote:
>>>>> On Fri, Aug 9, 2019 at 10:20 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>
>>>>>> On 8/9/19 10:00 PM, Saravana Kannan wrote:
>>>>>>> On Fri, Aug 9, 2019 at 7:57 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>>>
>>>>>>>> Hi Saravana,
>>>>>>>>
>>>>>>>> On 7/31/19 3:17 PM, Saravana Kannan 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.
>>>>>>>>>
>>>>>>>>> v6 -> v7:
>>>>>>>>> - No functional changes.
>>>>>>>>> - Renamed i to index
>>>>>>>>> - Added comment to clarify not having to check property name for every
>>>>>>>>>   index
>>>>>>>>> - Added "matched" variable to clarify code. No functional change.
>>>>>>>>> - Added comments to include/linux/device.h for add_links()
>>>>>>>>>
>>>>>>>>> v7 -> v8:
>>>>>>>>> - Rebased on top of linux-next to handle device link changes in [1]
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> v8 -> v9:
>>>>>>>>> - Fixed kbuild test bot reported errors (docs and const)
>>>>>>>>
>>>>>>>> Some maintainers have strong opinions about whether change logs should be:
>>>>>>>>
>>>>>>>>   (1) only in patch 0
>>>>>>>>   (2) only in the specific patches that are changed
>>>>>>>>   (3) both in patch 0 and in the specific patches that are changed.
>>>>>>>>
>>>>>>>> I can adapt to any of the three styles.  But for style "(1)" please
>>>>>>>> list which specific patch has changed for each item in the change list.
>>>>>>>>
>>>>>>>
>>>>>>> Thanks for the context Frank. I'm okay with (1) or (2) but I'll stick
>>>>>>> with (1) for this series. Didn't realize there were options (2) and
>>>>>>> (3). Since you started reviewing from v7, I'll do that in the future
>>>>>>> updates? Also, I haven't forgotten your emails. Just tied up with
>>>>>>> something else for a few days. I'll get to your emails next week.
>>>>>>
>>>>>> Yes, starting with future updates is fine, no need to redo the v9
>>>>>> change logs.
>>>>>>
>>>>>> No problem on the timing.  I figured you were busy or away from the
>>>>>> internet.
>>>>>
>>>>> I'm replying to your comments on the other 3 patches. Okay with a
>>>>> majority of them. I'll wait for your reply to see where we settle for
>>>>> some of the points before I send out any patches though.
>>>>>
>>>>> For now I'm thinking of sending them as separate clean up patches so
>>>>> that Greg doesn't have to deal with reverts in his "next" branch. We
>>>>> can squash them later if we really need to rip out what's in there and
>>>>> push it again.
>>>>>
>>>>> -Saravana
>>>>>
>>>>
>>>> Please do not do separate clean up patches.  The series that Greg has is
>>>> not ready for acceptance and I am going to ask him to revert it as we
>>>> work through the needed changes.
>>>>
>>>> I suspect there will be at least two more versions of the series.  The
>>>> first is to get the patches I commented in good shape.  Then I will
>>>> look at the patches later in the series to see how they fit into the
>>>> big picture.
>>>>
>>>> In the end, there should be one coherent patch series that implements
>>>> the feature.
>>>
>>> Incremental patches to fix up the comments and documentation is fine, no
>>> need to respin the whole mess.
>>
>> The problem is that the whole thing is a "mess" at this point.  I expect
>> the series to go through at least two or three more versions.
> 
> I'm confused.  All I see so far is objections about some documentation
> in comments that can be cleaned up, and a disagreement about the name of
> some things (naming is hard, tie goes to the submitter).

Yes naming is hard. No,tie does not go to the submitter is the naming
makes the code difficult to understand.

Naming is one of the reasons why I have found this series so difficult
to understand.


> But no logic issues, right?  Documentation and names can be fixed
> anytime, the logic is all working properly, right?

Yes, there are logic issues.  I do not agree will all of the explanations
in the replies.

Without going into detail about all the issues, one key is that I
need to see an example of the edit_links() function, which Saravana
says he will provide.  I don't want a bunch of ad hoc edit_links()
functions that each deal with cyclic dependencies in different ways.

There is also disagreement over whether the complexity of the
dev->has_edit_links field and driver_edit_links() are needed.

My biggest meta-issue is that this patch series is papering over the
real problem that prompted the patches.  The real problem is that the
boot loader has enabled a power supply, but the power subsystem is
not aware that there is an active consumer.  I have been hopeful that
this series can be implemented in a way that makes me comfortable
that it is _not_ just papering over the true problem.  I still
retain that hope.


> 
> What am I missing here?
> 
> thanks,
> 
> greg k-h
> 

-Frank

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Frank Rowand]

On 8/16/19 1:52 PM, Frank Rowand wrote:
> On 8/16/19 8:23 AM, Greg Kroah-Hartman wrote:
>> On Fri, Aug 16, 2019 at 07:05:06AM -0700, Frank Rowand wrote:
>>> i Greg,
>>>
>>> On 8/16/19 2:10 AM, Greg Kroah-Hartman wrote:
>>>> On Thu, Aug 15, 2019 at 08:09:19PM -0700, Frank Rowand wrote:
>>>>> Hi Saravana,
>>>>>
>>>>> On 8/15/19 6:50 PM, Saravana Kannan wrote:
>>>>>> On Fri, Aug 9, 2019 at 10:20 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>>
>>>>>>> On 8/9/19 10:00 PM, Saravana Kannan wrote:
>>>>>>>> On Fri, Aug 9, 2019 at 7:57 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi Saravana,
>>>>>>>>>
>>>>>>>>> On 7/31/19 3:17 PM, Saravana Kannan 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.
>>>>>>>>>>
>>>>>>>>>> v6 -> v7:
>>>>>>>>>> - No functional changes.
>>>>>>>>>> - Renamed i to index
>>>>>>>>>> - Added comment to clarify not having to check property name for every
>>>>>>>>>>   index
>>>>>>>>>> - Added "matched" variable to clarify code. No functional change.
>>>>>>>>>> - Added comments to include/linux/device.h for add_links()
>>>>>>>>>>
>>>>>>>>>> v7 -> v8:
>>>>>>>>>> - Rebased on top of linux-next to handle device link changes in [1]
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> v8 -> v9:
>>>>>>>>>> - Fixed kbuild test bot reported errors (docs and const)
>>>>>>>>>
>>>>>>>>> Some maintainers have strong opinions about whether change logs should be:
>>>>>>>>>
>>>>>>>>>   (1) only in patch 0
>>>>>>>>>   (2) only in the specific patches that are changed
>>>>>>>>>   (3) both in patch 0 and in the specific patches that are changed.
>>>>>>>>>
>>>>>>>>> I can adapt to any of the three styles.  But for style "(1)" please
>>>>>>>>> list which specific patch has changed for each item in the change list.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks for the context Frank. I'm okay with (1) or (2) but I'll stick
>>>>>>>> with (1) for this series. Didn't realize there were options (2) and
>>>>>>>> (3). Since you started reviewing from v7, I'll do that in the future
>>>>>>>> updates? Also, I haven't forgotten your emails. Just tied up with
>>>>>>>> something else for a few days. I'll get to your emails next week.
>>>>>>>
>>>>>>> Yes, starting with future updates is fine, no need to redo the v9
>>>>>>> change logs.
>>>>>>>
>>>>>>> No problem on the timing.  I figured you were busy or away from the
>>>>>>> internet.
>>>>>>
>>>>>> I'm replying to your comments on the other 3 patches. Okay with a
>>>>>> majority of them. I'll wait for your reply to see where we settle for
>>>>>> some of the points before I send out any patches though.
>>>>>>
>>>>>> For now I'm thinking of sending them as separate clean up patches so
>>>>>> that Greg doesn't have to deal with reverts in his "next" branch. We
>>>>>> can squash them later if we really need to rip out what's in there and
>>>>>> push it again.
>>>>>>
>>>>>> -Saravana
>>>>>>
>>>>>
>>>>> Please do not do separate clean up patches.  The series that Greg has is
>>>>> not ready for acceptance and I am going to ask him to revert it as we
>>>>> work through the needed changes.
>>>>>
>>>>> I suspect there will be at least two more versions of the series.  The
>>>>> first is to get the patches I commented in good shape.  Then I will
>>>>> look at the patches later in the series to see how they fit into the
>>>>> big picture.
>>>>>
>>>>> In the end, there should be one coherent patch series that implements
>>>>> the feature.
>>>>
>>>> Incremental patches to fix up the comments and documentation is fine, no
>>>> need to respin the whole mess.
>>>
>>> The problem is that the whole thing is a "mess" at this point.  I expect
>>> the series to go through at least two or three more versions.
>>
>> I'm confused.  All I see so far is objections about some documentation
>> in comments that can be cleaned up, and a disagreement about the name of
>> some things (naming is hard, tie goes to the submitter).
> 
> Yes naming is hard. No,tie does not go to the submitter is the naming

                                                          ^^ if

-Frank

> makes the code difficult to understand.
> 
> Naming is one of the reasons why I have found this series so difficult
> to understand.
> 
> 
>> But no logic issues, right?  Documentation and names can be fixed
>> anytime, the logic is all working properly, right?
> 
> Yes, there are logic issues.  I do not agree will all of the explanations
> in the replies.
> 
> Without going into detail about all the issues, one key is that I
> need to see an example of the edit_links() function, which Saravana
> says he will provide.  I don't want a bunch of ad hoc edit_links()
> functions that each deal with cyclic dependencies in different ways.
> 
> There is also disagreement over whether the complexity of the
> dev->has_edit_links field and driver_edit_links() are needed.
> 
> My biggest meta-issue is that this patch series is papering over the
> real problem that prompted the patches.  The real problem is that the
> boot loader has enabled a power supply, but the power subsystem is
> not aware that there is an active consumer.  I have been hopeful that
> this series can be implemented in a way that makes me comfortable
> that it is _not_ just papering over the true problem.  I still
> retain that hope.
> 
> 
>>
>> What am I missing here?
>>
>> thanks,
>>
>> greg k-h
>>
> 
> -Frank
> 

Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering

[Greg Kroah-Hartman]

On Wed, Aug 07, 2019 at 07:13:26PM -0700, Frank Rowand wrote:
> Hi Greg, Saravana,
> 
> On 8/1/19 11:37 PM, Greg Kroah-Hartman wrote:
> > On Thu, Aug 01, 2019 at 12:59:25PM -0700, Frank Rowand wrote:
> >> On 8/1/19 12:32 PM, Greg Kroah-Hartman wrote:
> >>> On Thu, Aug 01, 2019 at 12:28:13PM -0700, Frank Rowand wrote:
> >>>> Hi Greg,
> >>>>
> >>>> On 7/31/19 11:12 PM, Greg Kroah-Hartman wrote:
> >>>>> On Wed, Jul 31, 2019 at 03:17:13PM -0700, Saravana Kannan 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.
> >>>>>
> >>>>> All now queued up in my driver-core-testing branch, and if 0-day is
> >>>>> happy with this, will move it to my "real" driver-core-next branch in a
> >>>>> day or so to get included in linux-next.
> >>>>
> >>>> I have been slow in getting my review out.
> >>>>
> >>>> This patch series is not yet ready for sending to Linus, so if putting
> >>>> this in linux-next implies that it will be in your next pull request
> >>>> to Linus, please do not put it in linux-next.
> >>>
> >>> It means that it will be in my pull request for 5.4-rc1, many many
> >>> waeeks away from now.
> >>
> >> If you are willing to revert the series before the pull request _if_ I
> >> have significant review issues in the next couple of days, then I am happy
> >> to see the patches get exposure in linux-next.
> > 
> > If you have significant review issues, yes, I will be glad to revert them.
> 
> Just a heads up that I have sent review issues in reply to version 7 of this
> patch series.
> 
> We'll see what the responses are to my review comments, but I am expecting
> the changes are big enough to result in a new version (or a couple more
> versions) of the patch series.
> 
> No rush to revert version 9 since your 5.4-rc1 pull request is still not
> near, and I am glad for whatever exposure these patches are getting in
> linux-next.

Based on the further comments on this series, and the in-person we had
at ELC, I have now reverted these, and the follow-on fixes for this
series from my tree, with the hope that an updated patch set will be
sent for review soon.

thanks,

greg k-h