[PATCH v2 00/17] Refactor fw_devlink to significantly improve boot time

[PATCH v2 00/17] Refactor fw_devlink to significantly improve boot time

[Saravana Kannan]

The current implementation of fw_devlink is very inefficient because it
tries to get away without creating fwnode links in the name of saving
memory usage. Past attempts to optimize runtime at the cost of memory
usage were blocked with request for data showing that the optimization
made significant improvement for real world scenarios.

We have those scenarios now. There have been several reports of boot
time increase in the order of seconds in this thread [1]. Several OEMs
and SoC manufacturers have also privately reported significant
(350-400ms) increase in boot time due to all the parsing done by
fw_devlink.

So this patch series refactors fw_devlink to be more efficient. The key
difference now is the addition of support for fwnode links -- just a few
simple APIs. This also allows most of the code to be moved out of
firmware specific (DT mostly) code into driver core.

This brings the following benefits:
- Instead of parsing the device tree multiple times (complexity was
  close to O(N^3) where N in the number of properties) during bootup,
  fw_devlink parses each fwnode node/property only once and creates
  fwnode links. The rest of the fw_devlink code then just looks at these
  fwnode links to do rest of the work.

- Makes it much easier to debug probe issue due to fw_devlink in the
  future. fw_devlink=on blocks the probing of devices if they depend on
  a device that hasn't been added yet. With this refactor, it'll be very
  easy to tell what that device is because we now have a reference to
  the fwnode of the device.

- Much easier to add fw_devlink support to ACPI and other firmware
  types. A refactor to move the common bits from DT specific code to
  driver core was in my TODO list as a prerequisite to adding ACPI
  support to fw_devlink. This series gets that done.

Laurent and Grygorii tested the v1 series and they saw boot time
improvment of about 12 seconds and 3 seconds, respectively.

Thanks,
Saravana

[1] - https://lore.kernel.org/linux-pm/CAGETcx-aiW251dhEXT1GNb9bi6YcX8W=jLBrro5CnPuEjGL09g@mail.gmail.com/

v1 -> v2:
Patches 1-6:
- Added the "why" to explain the reverts.
Patch 7:
- Fixed white space comment.
Patch 8:
- Reworded commit text and some function doc.
Patch 11:
- Fixed the build warning this patch would cause by removing a "const".
Patch 12:
- Added/updated documentation.
- Changed flags from u32 to u8.
Patch 13:
- Squashed with Patch 10. Will use v1 patch number for the rest of the diff
  descriptions.
Patch 15:
- Removed an unnecessary unlikely()
Patch 17:
- Refactored fw_devlink_create_devlink() to flip the error handling vs
  successful paths.
Patch 18:
- Squashed into Patch 17 as requested by Greg.
- Added Tested-by: tags from Laurent and Grygorii.
New Patch 17:
- New patch to delete useless input to add_links()

Saravana Kannan (17):
  Revert "driver core: Avoid deferred probe due to
    fw_devlink_pause/resume()"
  Revert "driver core: Rename dev_links_info.defer_sync to defer_hook"
  Revert "driver core: Don't do deferred probe in parallel with
    kernel_init thread"
  Revert "driver core: Remove check in
    driver_deferred_probe_force_trigger()"
  Revert "of: platform: Batch fwnode parsing when adding all top level
    devices"
  Revert "driver core: fw_devlink: Add support for batching fwnode
    parsing"
  driver core: Add fwnode_init()
  driver core: Add fwnode link support
  driver core: Allow only unprobed consumers for SYNC_STATE_ONLY device
    links
  device property: Add fwnode_is_ancestor_of() and
    fwnode_get_next_parent_dev()
  driver core: Redefine the meaning of fwnode_operations.add_links()
  driver core: Add fw_devlink_parse_fwtree()
  driver core: Use device's fwnode to check if it is waiting for
    suppliers
  of: property: Update implementation of add_links() to create fwnode
    links
  efi: Update implementation of add_links() to create fwnode links
  driver core: Refactor fw_devlink feature
  driver core: Delete pointless parameter in fwnode_operations.add_links

 drivers/acpi/property.c         |   2 +-
 drivers/acpi/scan.c             |   2 +-
 drivers/base/core.c             | 555 ++++++++++++++++++++------------
 drivers/base/property.c         |  52 +++
 drivers/base/swnode.c           |   2 +-
 drivers/firmware/efi/efi-init.c |  32 +-
 drivers/of/dynamic.c            |   1 +
 drivers/of/platform.c           |   2 -
 drivers/of/property.c           | 149 +++------
 include/linux/device.h          |  10 +-
 include/linux/fwnode.h          |  73 ++---
 include/linux/of.h              |   2 +-
 include/linux/property.h        |   3 +
 kernel/irq/irqdomain.c          |   2 +-
 14 files changed, 495 insertions(+), 392 deletions(-)

-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 01/17] Revert "driver core: Avoid deferred probe due to fw_devlink_pause/resume()"

[Saravana Kannan]

This reverts commit 2451e746478a6a6e981cfa66b62b791ca93b90c8.

fw_devlink_pause/resume() was an incomplete attempt at boot time
optimization. That's going to get replaced by a much better optimization
at the end of the series. Since fw_devlink_pause/resume() is going away,
changes made for that can also go away.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 21 ---------------------
 include/linux/device.h |  3 +--
 2 files changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d661ada1518f..b202fe54f46a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -51,7 +51,6 @@ static DEFINE_MUTEX(wfs_lock);
 static LIST_HEAD(deferred_sync);
 static unsigned int defer_sync_state_count = 1;
 static unsigned int defer_fw_devlink_count;
-static LIST_HEAD(deferred_fw_devlink);
 static DEFINE_MUTEX(defer_fw_devlink_lock);
 static bool fw_devlink_is_permissive(void);
 
@@ -1470,12 +1469,6 @@ static void fw_devlink_link_device(struct device *dev)
 			fw_ret = -EAGAIN;
 	} else {
 		fw_ret = -ENODEV;
-		/*
-		 * defer_hook is not used to add device to deferred_sync list
-		 * until device is bound. Since deferred fw devlink also blocks
-		 * probing, same list hook can be used for deferred_fw_devlink.
-		 */
-		list_add_tail(&dev->links.defer_hook, &deferred_fw_devlink);
 	}
 
 	if (fw_ret == -ENODEV)
@@ -1544,9 +1537,6 @@ void fw_devlink_pause(void)
  */
 void fw_devlink_resume(void)
 {
-	struct device *dev, *tmp;
-	LIST_HEAD(probe_list);
-
 	mutex_lock(&defer_fw_devlink_lock);
 	if (!defer_fw_devlink_count) {
 		WARN(true, "Unmatched fw_devlink pause/resume!");
@@ -1558,19 +1548,8 @@ void fw_devlink_resume(void)
 		goto out;
 
 	device_link_add_missing_supplier_links();
-	list_splice_tail_init(&deferred_fw_devlink, &probe_list);
 out:
 	mutex_unlock(&defer_fw_devlink_lock);
-
-	/*
-	 * bus_probe_device() can cause new devices to get added and they'll
-	 * try to grab defer_fw_devlink_lock. So, this needs to be done outside
-	 * the defer_fw_devlink_lock.
-	 */
-	list_for_each_entry_safe(dev, tmp, &probe_list, links.defer_hook) {
-		list_del_init(&dev->links.defer_hook);
-		bus_probe_device(dev);
-	}
 }
 /* Device links support end. */
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 5ed101be7b2e..da00f8e449bb 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -352,8 +352,7 @@ 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_hook: Hook to global list of devices that have deferred sync_state or
- *		deferred fw_devlink.
+ * @defer_hook: Hook to global list of devices that have deferred sync_state.
  * @need_for_probe: If needs_suppliers is on a list, this indicates if the
  *		    suppliers are needed for probe or not.
  * @status: Driver status information.
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 02/17] Revert "driver core: Rename dev_links_info.defer_sync to defer_hook"

[Saravana Kannan]

This reverts commit ec7bd78498f29680f536451fbdf9464e851273ed.

This field rename was done to reuse defer_syc list head for multiple
lists. That's not needed anymore and this list head will only be used
for defer sync. So revert this patch to avoid conflicts with the other
reverts coming after this.

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index b202fe54f46a..c91ff2546247 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -959,11 +959,11 @@ static void __device_links_queue_sync_state(struct device *dev,
 	 */
 	dev->state_synced = true;
 
-	if (WARN_ON(!list_empty(&dev->links.defer_hook)))
+	if (WARN_ON(!list_empty(&dev->links.defer_sync)))
 		return;
 
 	get_device(dev);
-	list_add_tail(&dev->links.defer_hook, list);
+	list_add_tail(&dev->links.defer_sync, list);
 }
 
 /**
@@ -981,8 +981,8 @@ static void device_links_flush_sync_list(struct list_head *list,
 {
 	struct device *dev, *tmp;
 
-	list_for_each_entry_safe(dev, tmp, list, links.defer_hook) {
-		list_del_init(&dev->links.defer_hook);
+	list_for_each_entry_safe(dev, tmp, list, links.defer_sync) {
+		list_del_init(&dev->links.defer_sync);
 
 		if (dev != dont_lock_dev)
 			device_lock(dev);
@@ -1020,12 +1020,12 @@ void device_links_supplier_sync_state_resume(void)
 	if (defer_sync_state_count)
 		goto out;
 
-	list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_hook) {
+	list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_sync) {
 		/*
 		 * Delete from deferred_sync list before queuing it to
-		 * sync_list because defer_hook is used for both lists.
+		 * sync_list because defer_sync is used for both lists.
 		 */
-		list_del_init(&dev->links.defer_hook);
+		list_del_init(&dev->links.defer_sync);
 		__device_links_queue_sync_state(dev, &sync_list);
 	}
 out:
@@ -1043,8 +1043,8 @@ late_initcall(sync_state_resume_initcall);
 
 static void __device_links_supplier_defer_sync(struct device *sup)
 {
-	if (list_empty(&sup->links.defer_hook) && dev_has_sync_state(sup))
-		list_add_tail(&sup->links.defer_hook, &deferred_sync);
+	if (list_empty(&sup->links.defer_sync) && dev_has_sync_state(sup))
+		list_add_tail(&sup->links.defer_sync, &deferred_sync);
 }
 
 static void device_link_drop_managed(struct device_link *link)
@@ -1274,7 +1274,7 @@ void device_links_driver_cleanup(struct device *dev)
 		WRITE_ONCE(link->status, DL_STATE_DORMANT);
 	}
 
-	list_del_init(&dev->links.defer_hook);
+	list_del_init(&dev->links.defer_sync);
 	__device_links_no_driver(dev);
 
 	device_links_write_unlock();
@@ -2407,7 +2407,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_hook);
+	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 da00f8e449bb..1e771ea4dca6 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -352,7 +352,7 @@ 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_hook: Hook to global list of devices that have deferred sync_state.
+ * @defer_sync: Hook to global list of devices that have deferred sync_state.
  * @need_for_probe: If needs_suppliers is on a list, this indicates if the
  *		    suppliers are needed for probe or not.
  * @status: Driver status information.
@@ -361,7 +361,7 @@ struct dev_links_info {
 	struct list_head suppliers;
 	struct list_head consumers;
 	struct list_head needs_suppliers;
-	struct list_head defer_hook;
+	struct list_head defer_sync;
 	bool need_for_probe;
 	enum dl_dev_state status;
 };
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 03/17] Revert "driver core: Don't do deferred probe in parallel with kernel_init thread"

[Saravana Kannan]

This reverts commit cec72f3efc6272420c2c2c699607f03d09b93e41.

Commit cec72f3efc62 ("driver core: Don't do deferred probe in parallel
with kernel_init thread") was fixing a commit 716a7a259690 ("driver
core: fw_devlink: Add support for batching fwnode parsing"). Since the
commit being fixed itself is going to be reverted, the fix can also be
reverted.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/base.h | 1 +
 drivers/base/core.c | 1 +
 drivers/base/dd.c   | 5 +++++
 3 files changed, 7 insertions(+)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 91cfb8405abd..c3562adf4789 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -156,6 +156,7 @@ extern char *make_class_name(const char *name, struct kobject *kobj);
 extern int devres_release_all(struct device *dev);
 extern void device_block_probing(void);
 extern void device_unblock_probing(void);
+extern void driver_deferred_probe_force_trigger(void);
 
 /* /sys/devices directory */
 extern struct kset *devices_kset;
diff --git a/drivers/base/core.c b/drivers/base/core.c
index c91ff2546247..e6bb4bf4f19d 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1548,6 +1548,7 @@ void fw_devlink_resume(void)
 		goto out;
 
 	device_link_add_missing_supplier_links();
+	driver_deferred_probe_force_trigger();
 out:
 	mutex_unlock(&defer_fw_devlink_lock);
 }
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 148e81969e04..001caa0b447c 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -167,6 +167,11 @@ static void driver_deferred_probe_trigger(void)
 	if (!driver_deferred_probe_enable)
 		return;
 
+	driver_deferred_probe_force_trigger();
+}
+
+void driver_deferred_probe_force_trigger(void)
+{
 	/*
 	 * A successful probe means that all the devices in the pending list
 	 * should be triggered to be reprobed.  Move all the deferred devices
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 04/17] Revert "driver core: Remove check in driver_deferred_probe_force_trigger()"

[Saravana Kannan]

This reverts commit fefcfc968723caf93318613a08e1f3ad07a6154f.

The reverted commit is fixing commit 716a7a259690 ("driver core:
fw_devlink: Add support for batching fwnode parsing"). Since the
original commit will be reverted, the fix can be reverted too.

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

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 001caa0b447c..b4be35fa7fda 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -172,6 +172,9 @@ static void driver_deferred_probe_trigger(void)
 
 void driver_deferred_probe_force_trigger(void)
 {
+	if (!driver_deferred_probe_enable)
+		return;
+
 	/*
 	 * A successful probe means that all the devices in the pending list
 	 * should be triggered to be reprobed.  Move all the deferred devices
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 05/17] Revert "of: platform: Batch fwnode parsing when adding all top level devices"

[Saravana Kannan]

This reverts commit 93d2e4322aa74c1ad1e8c2160608eb9a960d69ff.

The fw_devlink_pause/resume() optimization attempt is getting replaced
with a much more robust optimization by the end of this series. So, stop
using those APIs.

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

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b557a0fcd4ba..79bd5f5a1bf1 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -538,9 +538,7 @@ static int __init of_platform_default_populate_init(void)
 	}
 
 	/* Populate everything else. */
-	fw_devlink_pause();
 	of_platform_default_populate(NULL, NULL, NULL);
-	fw_devlink_resume();
 
 	return 0;
 }
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 06/17] Revert "driver core: fw_devlink: Add support for batching fwnode parsing"

[Saravana Kannan]

This reverts commit 716a7a25969003d82ab738179c3f1068a120ed11.

The fw_devlink_pause/resume() APIs added by the commit being reverted
were a first cut attempt at optimizing boot time. But these APIs don't
fully solve the problem and are very fragile (can only be used for the
top level devices being added). This series replaces them with a much
better optimization that works for all device additions and also has the
benefit of reducing the complexity of the firmware (DT, EFI) specific
code and abstracting out common code to driver core.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/base.h    |   1 -
 drivers/base/core.c    | 116 +++--------------------------------------
 drivers/base/dd.c      |   8 ---
 include/linux/fwnode.h |   2 -
 4 files changed, 7 insertions(+), 120 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index c3562adf4789..91cfb8405abd 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -156,7 +156,6 @@ extern char *make_class_name(const char *name, struct kobject *kobj);
 extern int devres_release_all(struct device *dev);
 extern void device_block_probing(void);
 extern void device_unblock_probing(void);
-extern void driver_deferred_probe_force_trigger(void);
 
 /* /sys/devices directory */
 extern struct kset *devices_kset;
diff --git a/drivers/base/core.c b/drivers/base/core.c
index e6bb4bf4f19d..401fa7e3505c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -50,9 +50,6 @@ static LIST_HEAD(wait_for_suppliers);
 static DEFINE_MUTEX(wfs_lock);
 static LIST_HEAD(deferred_sync);
 static unsigned int defer_sync_state_count = 1;
-static unsigned int defer_fw_devlink_count;
-static DEFINE_MUTEX(defer_fw_devlink_lock);
-static bool fw_devlink_is_permissive(void);
 
 #ifdef CONFIG_SRCU
 static DEFINE_MUTEX(device_links_lock);
@@ -758,7 +755,7 @@ static void device_link_add_missing_supplier_links(void)
 		int ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
 		if (!ret)
 			list_del_init(&dev->links.needs_suppliers);
-		else if (ret != -ENODEV || fw_devlink_is_permissive())
+		else if (ret != -ENODEV)
 			dev->links.need_for_probe = false;
 	}
 	mutex_unlock(&wfs_lock);
@@ -1442,116 +1439,17 @@ static void fw_devlink_link_device(struct device *dev)
 {
 	int fw_ret;
 
-	if (!fw_devlink_flags)
-		return;
-
-	mutex_lock(&defer_fw_devlink_lock);
-	if (!defer_fw_devlink_count)
-		device_link_add_missing_supplier_links();
-
-	/*
-	 * The device's fwnode not having add_links() doesn't affect if other
-	 * consumers can find this device as a supplier.  So, this check is
-	 * intentionally placed after device_link_add_missing_supplier_links().
-	 */
-	if (!fwnode_has_op(dev->fwnode, add_links))
-		goto out;
+	device_link_add_missing_supplier_links();
 
-	/*
-	 * If fw_devlink is being deferred, assume all devices have mandatory
-	 * suppliers they need to link to later. Then, when the fw_devlink is
-	 * resumed, all these devices will get a chance to try and link to any
-	 * suppliers they have.
-	 */
-	if (!defer_fw_devlink_count) {
+	if (fw_devlink_flags && fwnode_has_op(dev->fwnode, add_links)) {
 		fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
-		if (fw_ret == -ENODEV && fw_devlink_is_permissive())
-			fw_ret = -EAGAIN;
-	} else {
-		fw_ret = -ENODEV;
+		if (fw_ret == -ENODEV && !fw_devlink_is_permissive())
+			device_link_wait_for_mandatory_supplier(dev);
+		else if (fw_ret)
+			device_link_wait_for_optional_supplier(dev);
 	}
-
-	if (fw_ret == -ENODEV)
-		device_link_wait_for_mandatory_supplier(dev);
-	else if (fw_ret)
-		device_link_wait_for_optional_supplier(dev);
-
-out:
-	mutex_unlock(&defer_fw_devlink_lock);
 }
 
-/**
- * fw_devlink_pause - Pause parsing of fwnode to create device links
- *
- * Calling this function defers any fwnode parsing to create device links until
- * fw_devlink_resume() is called. Both these functions are ref counted and the
- * caller needs to match the calls.
- *
- * While fw_devlink is paused:
- * - Any device that is added won't have its fwnode parsed to create device
- *   links.
- * - The probe of the device will also be deferred during this period.
- * - Any devices that were already added, but waiting for suppliers won't be
- *   able to link to newly added devices.
- *
- * Once fw_devlink_resume():
- * - All the fwnodes that was not parsed will be parsed.
- * - All the devices that were deferred probing will be reattempted if they
- *   aren't waiting for any more suppliers.
- *
- * This pair of functions, is mainly meant to optimize the parsing of fwnodes
- * when a lot of devices that need to link to each other are added in a short
- * interval of time. For example, adding all the top level devices in a system.
- *
- * For example, if N devices are added and:
- * - All the consumers are added before their suppliers
- * - All the suppliers of the N devices are part of the N devices
- *
- * Then:
- *
- * - With the use of fw_devlink_pause() and fw_devlink_resume(), each device
- *   will only need one parsing of its fwnode because it is guaranteed to find
- *   all the supplier devices already registered and ready to link to. It won't
- *   have to do another pass later to find one or more suppliers it couldn't
- *   find in the first parse of the fwnode. So, we'll only need O(N) fwnode
- *   parses.
- *
- * - Without the use of fw_devlink_pause() and fw_devlink_resume(), we would
- *   end up doing O(N^2) parses of fwnodes because every device that's added is
- *   guaranteed to trigger a parse of the fwnode of every device added before
- *   it. This O(N^2) parse is made worse by the fact that when a fwnode of a
- *   device is parsed, all it descendant devices might need to have their
- *   fwnodes parsed too (even if the devices themselves aren't added).
- */
-void fw_devlink_pause(void)
-{
-	mutex_lock(&defer_fw_devlink_lock);
-	defer_fw_devlink_count++;
-	mutex_unlock(&defer_fw_devlink_lock);
-}
-
-/** fw_devlink_resume - Resume parsing of fwnode to create device links
- *
- * This function is used in conjunction with fw_devlink_pause() and is ref
- * counted. See documentation for fw_devlink_pause() for more details.
- */
-void fw_devlink_resume(void)
-{
-	mutex_lock(&defer_fw_devlink_lock);
-	if (!defer_fw_devlink_count) {
-		WARN(true, "Unmatched fw_devlink pause/resume!");
-		goto out;
-	}
-
-	defer_fw_devlink_count--;
-	if (defer_fw_devlink_count)
-		goto out;
-
-	device_link_add_missing_supplier_links();
-	driver_deferred_probe_force_trigger();
-out:
-	mutex_unlock(&defer_fw_devlink_lock);
-}
 /* Device links support end. */
 
 int (*platform_notify)(struct device *dev) = NULL;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index b4be35fa7fda..148e81969e04 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -163,14 +163,6 @@ static bool driver_deferred_probe_enable = false;
  * again.
  */
 static void driver_deferred_probe_trigger(void)
-{
-	if (!driver_deferred_probe_enable)
-		return;
-
-	driver_deferred_probe_force_trigger();
-}
-
-void driver_deferred_probe_force_trigger(void)
 {
 	if (!driver_deferred_probe_enable)
 		return;
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 9506f8ec0974..e0abafbb17f8 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -171,7 +171,5 @@ struct fwnode_operations {
 #define get_dev_from_fwnode(fwnode)	get_device((fwnode)->dev)
 
 extern u32 fw_devlink_get_flags(void);
-void fw_devlink_pause(void);
-void fw_devlink_resume(void);
 
 #endif
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 07/17] driver core: Add fwnode_init()

[Saravana Kannan]

There are multiple locations in the kernel where a struct fwnode_handle
is initialized. Add fwnode_init() so that we have one way of
initializing a fwnode_handle.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/acpi/property.c         | 2 +-
 drivers/acpi/scan.c             | 2 +-
 drivers/base/swnode.c           | 2 +-
 drivers/firmware/efi/efi-init.c | 8 ++++----
 include/linux/fwnode.h          | 6 ++++++
 include/linux/of.h              | 2 +-
 kernel/irq/irqdomain.c          | 2 +-
 7 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index d04de10a63e4..24e87b630573 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -76,7 +76,7 @@ static bool acpi_nondev_subnode_extract(const union acpi_object *desc,
 		return false;
 
 	dn->name = link->package.elements[0].string.pointer;
-	dn->fwnode.ops = &acpi_data_fwnode_ops;
+	fwnode_init(&dn->fwnode, &acpi_data_fwnode_ops);
 	dn->parent = parent;
 	INIT_LIST_HEAD(&dn->data.properties);
 	INIT_LIST_HEAD(&dn->data.subnodes);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index bc6a79e33220..519963bcc047 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1589,7 +1589,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	device->device_type = type;
 	device->handle = handle;
 	device->parent = acpi_bus_get_parent(handle);
-	device->fwnode.ops = &acpi_device_fwnode_ops;
+	fwnode_init(&device->fwnode, &acpi_device_fwnode_ops);
 	acpi_set_device_status(device, sta);
 	acpi_device_get_busid(device);
 	acpi_set_pnp_ids(handle, &device->pnp, type);
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 010828fc785b..4a4b2008fbc2 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -653,7 +653,7 @@ swnode_register(const struct software_node *node, struct swnode *parent,
 	swnode->parent = parent;
 	swnode->allocated = allocated;
 	swnode->kobj.kset = swnode_kset;
-	swnode->fwnode.ops = &software_node_ops;
+	fwnode_init(&swnode->fwnode, &software_node_ops);
 
 	ida_init(&swnode->child_ids);
 	INIT_LIST_HEAD(&swnode->entry);
diff --git a/drivers/firmware/efi/efi-init.c b/drivers/firmware/efi/efi-init.c
index f55a92ff12c0..b148f1459fb3 100644
--- a/drivers/firmware/efi/efi-init.c
+++ b/drivers/firmware/efi/efi-init.c
@@ -359,9 +359,7 @@ static const struct fwnode_operations efifb_fwnode_ops = {
 	.add_links = efifb_add_links,
 };
 
-static struct fwnode_handle efifb_fwnode = {
-	.ops = &efifb_fwnode_ops,
-};
+static struct fwnode_handle efifb_fwnode;
 
 static int __init register_gop_device(void)
 {
@@ -375,8 +373,10 @@ static int __init register_gop_device(void)
 	if (!pd)
 		return -ENOMEM;
 
-	if (IS_ENABLED(CONFIG_PCI))
+	if (IS_ENABLED(CONFIG_PCI)) {
+		fwnode_init(&efifb_fwnode, &efifb_fwnode_ops);
 		pd->dev.fwnode = &efifb_fwnode;
+	}
 
 	err = platform_device_add_data(pd, &screen_info, sizeof(screen_info));
 	if (err)
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index e0abafbb17f8..5589799708b5 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -170,6 +170,12 @@ struct fwnode_operations {
 	} while (false)
 #define get_dev_from_fwnode(fwnode)	get_device((fwnode)->dev)
 
+static inline void fwnode_init(struct fwnode_handle *fwnode,
+			       const struct fwnode_operations *ops)
+{
+	fwnode->ops = ops;
+}
+
 extern u32 fw_devlink_get_flags(void);
 
 #endif
diff --git a/include/linux/of.h b/include/linux/of.h
index 5d51891cbf1a..27fba2472eee 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -108,7 +108,7 @@ static inline void of_node_init(struct device_node *node)
 #if defined(CONFIG_OF_KOBJ)
 	kobject_init(&node->kobj, &of_node_ktype);
 #endif
-	node->fwnode.ops = &of_fwnode_ops;
+	fwnode_init(&node->fwnode, &of_fwnode_ops);
 }
 
 #if defined(CONFIG_OF_KOBJ)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index cf8b374b892d..06fce7e39033 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -91,7 +91,7 @@ struct fwnode_handle *__irq_domain_alloc_fwnode(unsigned int type, int id,
 	fwid->type = type;
 	fwid->name = n;
 	fwid->pa = pa;
-	fwid->fwnode.ops = &irqchip_fwnode_ops;
+	fwnode_init(&fwid->fwnode, &irqchip_fwnode_ops);
 	return &fwid->fwnode;
 }
 EXPORT_SYMBOL_GPL(__irq_domain_alloc_fwnode);
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 08/17] driver core: Add fwnode link support

[Saravana Kannan]

Add support for creating supplier-consumer links between fwnodes.  It is
intended for internal use the driver core and generic firmware support
code (eg. Device Tree, ACPI), so it is simple by design and the API
provided is limited.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 98 ++++++++++++++++++++++++++++++++++++++++++
 drivers/of/dynamic.c   |  1 +
 include/linux/fwnode.h | 14 ++++++
 3 files changed, 113 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 401fa7e3505c..e2b246a44d1a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -50,6 +50,104 @@ static LIST_HEAD(wait_for_suppliers);
 static DEFINE_MUTEX(wfs_lock);
 static LIST_HEAD(deferred_sync);
 static unsigned int defer_sync_state_count = 1;
+static DEFINE_MUTEX(fwnode_link_lock);
+
+/**
+ * fwnode_link_add - Create a link between two fwnode_handles.
+ * @con: Consumer end of the link.
+ * @sup: Supplier end of the link.
+ *
+ * Create a fwnode link between fwnode handles @con and @sup. The fwnode link
+ * represents the detail that the firmware lists @sup fwnode as supplying a
+ * resource to @con.
+ *
+ * The driver core will use the fwnode link to create a device link between the
+ * two device objects corresponding to @con and @sup when they are created. The
+ * driver core will automatically delete the fwnode link between @con and @sup
+ * after doing that.
+ *
+ * Attempts to create duplicate links between the same pair of fwnode handles
+ * are ignored and there is no reference counting.
+ */
+int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)
+{
+	struct fwnode_link *link;
+	int ret = 0;
+
+	mutex_lock(&fwnode_link_lock);
+
+	list_for_each_entry(link, &sup->consumers, s_hook)
+		if (link->consumer == con)
+			goto out;
+
+	link = kzalloc(sizeof(*link), GFP_KERNEL);
+	if (!link) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	link->supplier = sup;
+	INIT_LIST_HEAD(&link->s_hook);
+	link->consumer = con;
+	INIT_LIST_HEAD(&link->c_hook);
+
+	list_add(&link->s_hook, &sup->consumers);
+	list_add(&link->c_hook, &con->suppliers);
+out:
+	mutex_unlock(&fwnode_link_lock);
+
+	return ret;
+}
+
+/**
+ * fwnode_links_purge_suppliers - Delete all supplier links of fwnode_handle.
+ * @fwnode: fwnode whose supplier links need to be deleted
+ *
+ * Deletes all supplier links connecting directly to @fwnode.
+ */
+static void fwnode_links_purge_suppliers(struct fwnode_handle *fwnode)
+{
+	struct fwnode_link *link, *tmp;
+
+	mutex_lock(&fwnode_link_lock);
+	list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) {
+		list_del(&link->s_hook);
+		list_del(&link->c_hook);
+		kfree(link);
+	}
+	mutex_unlock(&fwnode_link_lock);
+}
+
+/**
+ * fwnode_links_purge_consumers - Delete all consumer links of fwnode_handle.
+ * @fwnode: fwnode whose consumer links need to be deleted
+ *
+ * Deletes all consumer links connecting directly to @fwnode.
+ */
+static void fwnode_links_purge_consumers(struct fwnode_handle *fwnode)
+{
+	struct fwnode_link *link, *tmp;
+
+	mutex_lock(&fwnode_link_lock);
+	list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) {
+		list_del(&link->s_hook);
+		list_del(&link->c_hook);
+		kfree(link);
+	}
+	mutex_unlock(&fwnode_link_lock);
+}
+
+/**
+ * fwnode_links_purge - Delete all links connected to a fwnode_handle.
+ * @fwnode: fwnode whose links needs to be deleted
+ *
+ * Deletes all links connecting directly to a fwnode.
+ */
+void fwnode_links_purge(struct fwnode_handle *fwnode)
+{
+	fwnode_links_purge_suppliers(fwnode);
+	fwnode_links_purge_consumers(fwnode);
+}
 
 #ifdef CONFIG_SRCU
 static DEFINE_MUTEX(device_links_lock);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index fe64430b438a..9a824decf61f 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -356,6 +356,7 @@ void of_node_release(struct kobject *kobj)
 
 	property_list_free(node->properties);
 	property_list_free(node->deadprops);
+	fwnode_links_purge(of_fwnode_handle(node));
 
 	kfree(node->full_name);
 	kfree(node->data);
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 5589799708b5..b88365187347 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -10,6 +10,7 @@
 #define _LINUX_FWNODE_H_
 
 #include <linux/types.h>
+#include <linux/list.h>
 
 struct fwnode_operations;
 struct device;
@@ -18,6 +19,15 @@ struct fwnode_handle {
 	struct fwnode_handle *secondary;
 	const struct fwnode_operations *ops;
 	struct device *dev;
+	struct list_head suppliers;
+	struct list_head consumers;
+};
+
+struct fwnode_link {
+	struct fwnode_handle *supplier;
+	struct list_head s_hook;
+	struct fwnode_handle *consumer;
+	struct list_head c_hook;
 };
 
 /**
@@ -174,8 +184,12 @@ static inline void fwnode_init(struct fwnode_handle *fwnode,
 			       const struct fwnode_operations *ops)
 {
 	fwnode->ops = ops;
+	INIT_LIST_HEAD(&fwnode->consumers);
+	INIT_LIST_HEAD(&fwnode->suppliers);
 }
 
 extern u32 fw_devlink_get_flags(void);
+int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup);
+void fwnode_links_purge(struct fwnode_handle *fwnode);
 
 #endif
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 09/17] driver core: Allow only unprobed consumers for SYNC_STATE_ONLY device links

[Saravana Kannan]

SYNC_STATE_ONLY device links only affect the behavior of sync_state()
callbacks. Specifically, they prevent sync_state() only callbacks from
being called on a device if one or more of its consumers haven't probed.

So, creating a SYNC_STATE_ONLY device link from an already probed
consumer is useless. So, don't allow creating such device links.

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index e2b246a44d1a..215ce9e72790 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -649,6 +649,17 @@ struct device_link *device_link_add(struct device *consumer,
 		goto out;
 	}
 
+	/*
+	 * SYNC_STATE_ONLY links are useless once a consumer device has probed.
+	 * So, only create it if the consumer hasn't probed yet.
+	 */
+	if (flags & DL_FLAG_SYNC_STATE_ONLY &&
+	    consumer->links.status != DL_DEV_NO_DRIVER &&
+	    consumer->links.status != DL_DEV_PROBING) {
+		link = NULL;
+		goto out;
+	}
+
 	/*
 	 * DL_FLAG_AUTOREMOVE_SUPPLIER indicates that the link will be needed
 	 * longer than for DL_FLAG_AUTOREMOVE_CONSUMER and setting them both
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 10/17] device property: Add fwnode_is_ancestor_of() and fwnode_get_next_parent_dev()

[Saravana Kannan]

Add fwnode_is_ancestor_of() helper function to check if a fwnode is an
ancestor of another fwnode.

Add fwnode_get_next_parent_dev() helper function that take as input a
fwnode and finds the closest ancestor fwnode that has a corresponding
struct device and returns that struct device.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/property.c  | 52 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 55 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 4c43d30145c6..35b95c6ac0c6 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -614,6 +614,31 @@ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
 
+/**
+ * fwnode_get_next_parent_dev - Find device of closest ancestor fwnode
+ * @fwnode: firmware node
+ *
+ * Given a firmware node (@fwnode), this function finds its closest ancestor
+ * firmware node that has a corresponding struct device and returns that struct
+ * device.
+ *
+ * The caller of this function is expected to call put_device() on the returned
+ * device when they are done.
+ */
+struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
+{
+	struct device *dev = NULL;
+
+	fwnode_handle_get(fwnode);
+	do {
+		fwnode = fwnode_get_next_parent(fwnode);
+		if (fwnode)
+			dev = get_dev_from_fwnode(fwnode);
+	} while (fwnode && !dev);
+	fwnode_handle_put(fwnode);
+	return dev;
+}
+
 /**
  * fwnode_count_parents - Return the number of parents a node has
  * @fwnode: The node the parents of which are to be counted
@@ -660,6 +685,33 @@ struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
 
+/**
+ * fwnode_is_ancestor_of - Test if @test_ancestor is ancestor of @test_child
+ * @test_ancestor: Firmware which is tested for being an ancestor
+ * @test_child: Firmware which is tested for being the child
+ *
+ * A node is considered an ancestor of itself too.
+ *
+ * Returns true if @test_ancestor is an ancestor of @test_child.
+ * Otherwise, returns false.
+ */
+bool fwnode_is_ancestor_of(struct fwnode_handle *test_ancestor,
+				  struct fwnode_handle *test_child)
+{
+	if (!test_ancestor)
+		return false;
+
+	fwnode_handle_get(test_child);
+	while (test_child) {
+		if (test_child == test_ancestor) {
+			fwnode_handle_put(test_child);
+			return true;
+		}
+		test_child = fwnode_get_next_parent(test_child);
+	}
+	return false;
+}
+
 /**
  * fwnode_get_next_child_node - Return the next child node handle for a node
  * @fwnode: Firmware node to find the next child node for.
diff --git a/include/linux/property.h b/include/linux/property.h
index 2d4542629d80..0a9001fe7aea 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -85,9 +85,12 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);
+struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode);
 unsigned int fwnode_count_parents(const struct fwnode_handle *fwn);
 struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
 					    unsigned int depth);
+bool fwnode_is_ancestor_of(struct fwnode_handle *test_ancestor,
+				  struct fwnode_handle *test_child);
 struct fwnode_handle *fwnode_get_next_child_node(
 	const struct fwnode_handle *fwnode, struct fwnode_handle *child);
 struct fwnode_handle *fwnode_get_next_available_child_node(
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 11/17] driver core: Redefine the meaning of fwnode_operations.add_links()

[Saravana Kannan]

Change the meaning of fwnode_operations.add_links() to just create
fwnode links by parsing the properties of a given fwnode.

This patch doesn't actually make any code changes. To keeps things more
digestable, the actual functional changes come in later patches in this
series.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/firmware/efi/efi-init.c |  2 +-
 include/linux/fwnode.h          | 42 +++------------------------------
 2 files changed, 4 insertions(+), 40 deletions(-)

diff --git a/drivers/firmware/efi/efi-init.c b/drivers/firmware/efi/efi-init.c
index b148f1459fb3..65bb97c391b0 100644
--- a/drivers/firmware/efi/efi-init.c
+++ b/drivers/firmware/efi/efi-init.c
@@ -316,7 +316,7 @@ static struct device_node *find_pci_overlap_node(void)
  * resource reservation conflict on the memory window that the efifb
  * framebuffer steals from the PCIe host bridge.
  */
-static int efifb_add_links(const struct fwnode_handle *fwnode,
+static int efifb_add_links(struct fwnode_handle *fwnode,
 			   struct device *dev)
 {
 	struct device_node *sup_np;
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index b88365187347..942a6bb18201 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -78,44 +78,8 @@ struct fwnode_reference_args {
  *			       endpoint node.
  * @graph_get_port_parent: Return the parent node of a port node.
  * @graph_parse_endpoint: Parse endpoint for port and endpoint id.
- * @add_links:	Called after the device corresponding to the fwnode is added
- *		using device_add(). The function is expected to create device
- *		links to all the suppliers of the device that are available at
- *		the time this function is called.  The function must NOT stop
- *		at the first failed device link if other unlinked supplier
- *		devices are present in the system.  This is necessary for the
- *		driver/bus sync_state() callbacks to work correctly.
- *
- *		For example, say Device-C depends on suppliers Device-S1 and
- *		Device-S2 and the dependency is listed in that order in the
- *		firmware.  Say, S1 gets populated from the firmware after
- *		late_initcall_sync().  Say S2 is populated and probed way
- *		before that in device_initcall(). When C is populated, if this
- *		add_links() function doesn't continue past a "failed linking to
- *		S1" and continue linking C to S2, then S2 will get a
- *		sync_state() callback before C is probed. This is because from
- *		the perspective of S2, C was never a consumer when its
- *		sync_state() evaluation is done. To avoid this, the add_links()
- *		function has to go through all available suppliers of the
- *		device (that corresponds to this fwnode) and link to them
- *		before returning.
- *
- *		If some suppliers are not yet available (indicated by an error
- *		return value), this function will be called again when other
- *		devices are added to allow creating device links to any newly
- *		available suppliers.
- *
- *		Return 0 if device links have been successfully created to all
- *		the known suppliers of this device or if the supplier
- *		information is not known.
- *
- *		Return -ENODEV if the suppliers needed for probing this device
- *		have not been registered yet (because device links can only be
- *		created to devices registered with the driver core).
- *
- *		Return -EAGAIN if some of the suppliers of this device have not
- *		been registered yet, but none of those suppliers are necessary
- *		for probing the device.
+ * @add_links:	Create fwnode links to all the suppliers of the fwnode. Return
+ *		zero on success, a negative error code otherwise.
  */
 struct fwnode_operations {
 	struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
@@ -155,7 +119,7 @@ struct fwnode_operations {
 	(*graph_get_port_parent)(struct fwnode_handle *fwnode);
 	int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode,
 				    struct fwnode_endpoint *endpoint);
-	int (*add_links)(const struct fwnode_handle *fwnode,
+	int (*add_links)(struct fwnode_handle *fwnode,
 			 struct device *dev);
 };
 
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 12/17] driver core: Add fw_devlink_parse_fwtree()

[Saravana Kannan]

This function is a wrapper around fwnode_operations.add_links().

This function parses each node in a fwnode tree and create fwnode links
for each of those nodes. The information for creating the fwnode links
(the supplier and consumer fwnode) is obtained by parsing the properties
in each of the fwnodes.

This function also ensures that no fwnode is parsed more than once by
marking the fwnodes as parsed.

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 215ce9e72790..395dece1c83a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1544,6 +1544,25 @@ static bool fw_devlink_is_permissive(void)
 	return fw_devlink_flags == DL_FLAG_SYNC_STATE_ONLY;
 }
 
+static void fw_devlink_parse_fwnode(struct fwnode_handle *fwnode)
+{
+	if (fwnode->flags & FWNODE_FLAG_LINKS_ADDED)
+		return;
+
+	fwnode_call_int_op(fwnode, add_links, NULL);
+	fwnode->flags |= FWNODE_FLAG_LINKS_ADDED;
+}
+
+static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *child = NULL;
+
+	fw_devlink_parse_fwnode(fwnode);
+
+	while ((child = fwnode_get_next_available_child_node(fwnode, child)))
+		fw_devlink_parse_fwtree(child);
+}
+
 static void fw_devlink_link_device(struct device *dev)
 {
 	int fw_ret;
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 942a6bb18201..ffa9129182a6 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -15,12 +15,20 @@
 struct fwnode_operations;
 struct device;
 
+/*
+ * fwnode link flags
+ *
+ * LINKS_ADDED: The fwnode has already be parsed to add fwnode links.
+ */
+#define FWNODE_FLAG_LINKS_ADDED		BIT(0)
+
 struct fwnode_handle {
 	struct fwnode_handle *secondary;
 	const struct fwnode_operations *ops;
 	struct device *dev;
 	struct list_head suppliers;
 	struct list_head consumers;
+	u8 flags;
 };
 
 struct fwnode_link {
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 13/17] driver core: Use device's fwnode to check if it is waiting for suppliers

[Saravana Kannan]

To check if a device is still waiting for its supplier devices to be
added, we used to check if the devices is in a global
waiting_for_suppliers list. Since the global list will be deleted in
subsequent patches, this patch stops using this check.

Instead, this patch uses a more device specific check. It checks if the
device's fwnode has any fwnode links that haven't been converted to
device links yet.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 395dece1c83a..1873cecb0cc4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -51,6 +51,7 @@ static DEFINE_MUTEX(wfs_lock);
 static LIST_HEAD(deferred_sync);
 static unsigned int defer_sync_state_count = 1;
 static DEFINE_MUTEX(fwnode_link_lock);
+static bool fw_devlink_is_permissive(void);
 
 /**
  * fwnode_link_add - Create a link between two fwnode_handles.
@@ -995,13 +996,13 @@ int device_links_check_suppliers(struct device *dev)
 	 * Device waiting for supplier to become available is not allowed to
 	 * probe.
 	 */
-	mutex_lock(&wfs_lock);
-	if (!list_empty(&dev->links.needs_suppliers) &&
-	    dev->links.need_for_probe) {
-		mutex_unlock(&wfs_lock);
+	mutex_lock(&fwnode_link_lock);
+	if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
+	    !fw_devlink_is_permissive()) {
+		mutex_unlock(&fwnode_link_lock);
 		return -EPROBE_DEFER;
 	}
-	mutex_unlock(&wfs_lock);
+	mutex_unlock(&fwnode_link_lock);
 
 	device_links_write_lock();
 
@@ -1167,10 +1168,7 @@ static ssize_t waiting_for_supplier_show(struct device *dev,
 	bool val;
 
 	device_lock(dev);
-	mutex_lock(&wfs_lock);
-	val = !list_empty(&dev->links.needs_suppliers)
-	      && dev->links.need_for_probe;
-	mutex_unlock(&wfs_lock);
+	val = !list_empty(&dev->fwnode->suppliers);
 	device_unlock(dev);
 	return sysfs_emit(buf, "%u\n", val);
 }
@@ -2202,7 +2200,7 @@ static int device_add_attrs(struct device *dev)
 			goto err_remove_dev_groups;
 	}
 
-	if (fw_devlink_flags && !fw_devlink_is_permissive()) {
+	if (fw_devlink_flags && !fw_devlink_is_permissive() && dev->fwnode) {
 		error = device_create_file(dev, &dev_attr_waiting_for_supplier);
 		if (error)
 			goto err_remove_dev_online;
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 14/17] of: property: Update implementation of add_links() to create fwnode links

[Saravana Kannan]

The semantics of add_links() has changed from creating device link
between devices to creating fwnode links between fwnodes. So, update the
implementation of add_links() to match the new semantics.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 150 ++++++++++++------------------------------
 1 file changed, 41 insertions(+), 109 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 408a7b5f06a9..620d29fdace8 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1038,33 +1038,9 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
 }
 
 /**
- * of_get_next_parent_dev - Add device link to supplier from supplier phandle
- * @np: device tree node
- *
- * Given a device tree node (@np), this function finds its closest ancestor
- * device tree node that has a corresponding struct device.
- *
- * The caller of this function is expected to call put_device() on the returned
- * device when they are done.
- */
-static struct device *of_get_next_parent_dev(struct device_node *np)
-{
-	struct device *dev = NULL;
-
-	of_node_get(np);
-	do {
-		np = of_get_next_parent(np);
-		if (np)
-			dev = get_dev_from_fwnode(&np->fwnode);
-	} while (np && !dev);
-	of_node_put(np);
-	return dev;
-}
-
-/**
- * of_link_to_phandle - Add device link to supplier from supplier phandle
- * @dev: consumer device
- * @sup_np: phandle to supplier device tree node
+ * of_link_to_phandle - Add fwnode link to supplier from supplier phandle
+ * @con_np: consumer device tree node
+ * @sup_np: supplier device tree node
  *
  * Given a phandle to a supplier device tree node (@sup_np), this function
  * finds the device that owns the supplier device tree node and creates a
@@ -1074,16 +1050,14 @@ static struct device *of_get_next_parent_dev(struct device_node *np)
  * cases, it returns an error.
  *
  * Returns:
- * - 0 if link successfully created to supplier
- * - -EAGAIN if linking to the supplier should be reattempted
+ * - 0 if fwnode link successfully created to supplier
  * - -EINVAL if the supplier link is invalid and should not be created
- * - -ENODEV if there is no device that corresponds to the supplier phandle
+ * - -ENODEV if struct device will never be create for supplier
  */
-static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
-			      u32 dl_flags)
+static int of_link_to_phandle(struct device_node *con_np,
+			      struct device_node *sup_np)
 {
-	struct device *sup_dev, *sup_par_dev;
-	int ret = 0;
+	struct device *sup_dev;
 	struct device_node *tmp_np = sup_np;
 
 	of_node_get(sup_np);
@@ -1106,7 +1080,8 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 	}
 
 	if (!sup_np) {
-		dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
+		pr_debug("Not linking %pOFP to %pOFP - No device\n",
+			 con_np, tmp_np);
 		return -ENODEV;
 	}
 
@@ -1115,53 +1090,30 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 	 * descendant nodes. By definition, a child node can't be a functional
 	 * dependency for the parent node.
 	 */
-	if (of_is_ancestor_of(dev->of_node, sup_np)) {
-		dev_dbg(dev, "Not linking to %pOFP - is descendant\n", sup_np);
+	if (of_is_ancestor_of(con_np, sup_np)) {
+		pr_debug("Not linking %pOFP to %pOFP - is descendant\n",
+			 con_np, sup_np);
 		of_node_put(sup_np);
 		return -EINVAL;
 	}
+
+	/*
+	 * Don't create links to "early devices" that won't have struct devices
+	 * created for them.
+	 */
 	sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
 	if (!sup_dev && of_node_check_flag(sup_np, OF_POPULATED)) {
-		/* Early device without struct device. */
-		dev_dbg(dev, "Not linking to %pOFP - No struct device\n",
-			sup_np);
+		pr_debug("Not linking %pOFP to %pOFP - No struct device\n",
+			 con_np, sup_np);
 		of_node_put(sup_np);
 		return -ENODEV;
-	} else if (!sup_dev) {
-		/*
-		 * DL_FLAG_SYNC_STATE_ONLY doesn't block probing and supports
-		 * cycles. So cycle detection isn't necessary and shouldn't be
-		 * done.
-		 */
-		if (dl_flags & DL_FLAG_SYNC_STATE_ONLY) {
-			of_node_put(sup_np);
-			return -EAGAIN;
-		}
-
-		sup_par_dev = of_get_next_parent_dev(sup_np);
-
-		if (sup_par_dev && device_is_dependent(dev, sup_par_dev)) {
-			/* Cyclic dependency detected, don't try to link */
-			dev_dbg(dev, "Not linking to %pOFP - cycle detected\n",
-				sup_np);
-			ret = -EINVAL;
-		} else {
-			/*
-			 * Can't check for cycles or no cycles. So let's try
-			 * again later.
-			 */
-			ret = -EAGAIN;
-		}
-
-		of_node_put(sup_np);
-		put_device(sup_par_dev);
-		return ret;
 	}
-	of_node_put(sup_np);
-	if (!device_link_add(dev, sup_dev, dl_flags))
-		ret = -EINVAL;
 	put_device(sup_dev);
-	return ret;
+
+	fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
+	of_node_put(sup_np);
+
+	return 0;
 }
 
 /**
@@ -1361,37 +1313,29 @@ static const struct supplier_bindings of_supplier_bindings[] = {
  * that list phandles to suppliers. If @prop_name isn't one, this function
  * doesn't do anything.
  *
- * If @prop_name is one, this function attempts to create device links from the
- * consumer device @dev to all the devices of the suppliers listed in
- * @prop_name.
+ * If @prop_name is one, this function attempts to create fwnode links from the
+ * consumer device tree node @con_np to all the suppliers device tree nodes
+ * listed in @prop_name.
  *
- * Any failed attempt to create a device link will NOT result in an immediate
+ * Any failed attempt to create a fwnode link will NOT result in an immediate
  * return.  of_link_property() must create links to all the available supplier
- * devices even when attempts to create a link to one or more suppliers fail.
+ * device tree nodes even when attempts to create a link to one or more
+ * suppliers fail.
  */
-static int of_link_property(struct device *dev, struct device_node *con_np,
-			     const char *prop_name)
+static int of_link_property(struct device_node *con_np, const char *prop_name)
 {
 	struct device_node *phandle;
 	const struct supplier_bindings *s = of_supplier_bindings;
 	unsigned int i = 0;
 	bool matched = false;
 	int ret = 0;
-	u32 dl_flags;
-
-	if (dev->of_node == con_np)
-		dl_flags = fw_devlink_get_flags();
-	else
-		dl_flags = DL_FLAG_SYNC_STATE_ONLY;
 
 	/* Do not stop at first failed link, link all available suppliers. */
 	while (!matched && s->parse_prop) {
 		while ((phandle = s->parse_prop(con_np, prop_name, i))) {
 			matched = true;
 			i++;
-			if (of_link_to_phandle(dev, phandle, dl_flags)
-								== -EAGAIN)
-				ret = -EAGAIN;
+			of_link_to_phandle(con_np, phandle);
 			of_node_put(phandle);
 		}
 		s++;
@@ -1399,31 +1343,19 @@ static int of_link_property(struct device *dev, struct device_node *con_np,
 	return ret;
 }
 
-static int of_link_to_suppliers(struct device *dev,
-				  struct device_node *con_np)
+static int of_fwnode_add_links(struct fwnode_handle *fwnode,
+			       struct device *dev)
 {
-	struct device_node *child;
 	struct property *p;
-	int ret = 0;
+	struct device_node *con_np = to_of_node(fwnode);
 
-	for_each_property_of_node(con_np, p)
-		if (of_link_property(dev, con_np, p->name))
-			ret = -ENODEV;
-
-	for_each_available_child_of_node(con_np, child)
-		if (of_link_to_suppliers(dev, child) && !ret)
-			ret = -EAGAIN;
-
-	return ret;
-}
+	if (!con_np)
+		return -EINVAL;
 
-static int of_fwnode_add_links(const struct fwnode_handle *fwnode,
-			       struct device *dev)
-{
-	if (unlikely(!is_of_node(fwnode)))
-		return 0;
+	for_each_property_of_node(con_np, p)
+		of_link_property(con_np, p->name);
 
-	return of_link_to_suppliers(dev, to_of_node(fwnode));
+	return 0;
 }
 
 const struct fwnode_operations of_fwnode_ops = {
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 15/17] efi: Update implementation of add_links() to create fwnode links

[Saravana Kannan]

The semantics of add_links() has changed from creating device link
between devices to creating fwnode links between fwnodes. So, update the
implementation of add_links() to match the new semantics.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/firmware/efi/efi-init.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/firmware/efi/efi-init.c b/drivers/firmware/efi/efi-init.c
index 65bb97c391b0..c0c3d4c3837a 100644
--- a/drivers/firmware/efi/efi-init.c
+++ b/drivers/firmware/efi/efi-init.c
@@ -320,7 +320,6 @@ static int efifb_add_links(struct fwnode_handle *fwnode,
 			   struct device *dev)
 {
 	struct device_node *sup_np;
-	struct device *sup_dev;
 
 	sup_np = find_pci_overlap_node();
 
@@ -331,27 +330,9 @@ static int efifb_add_links(struct fwnode_handle *fwnode,
 	if (!sup_np)
 		return 0;
 
-	sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
+	fwnode_link_add(fwnode, of_fwnode_handle(sup_np));
 	of_node_put(sup_np);
 
-	/*
-	 * Return -ENODEV if the PCI graphics controller device hasn't been
-	 * registered yet.  This ensures that efifb isn't allowed to probe
-	 * and this function is retried again when new devices are
-	 * registered.
-	 */
-	if (!sup_dev)
-		return -ENODEV;
-
-	/*
-	 * If this fails, retrying this function at a later point won't
-	 * change anything. So, don't return an error after this.
-	 */
-	if (!device_link_add(dev, sup_dev, fw_devlink_get_flags()))
-		dev_warn(dev, "device_link_add() failed\n");
-
-	put_device(sup_dev);
-
 	return 0;
 }
 
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Saravana Kannan]

The current implementation of fw_devlink is very inefficient because it
tries to get away without creating fwnode links in the name of saving
memory usage. Past attempts to optimize runtime at the cost of memory
usage were blocked with request for data showing that the optimization
made significant improvement for real world scenarios.

We have those scenarios now. There have been several reports of boot
time increase in the order of seconds in this thread [1]. Several OEMs
and SoC manufacturers have also privately reported significant
(350-400ms) increase in boot time due to all the parsing done by
fw_devlink.

So this patch uses all the setup done by the previous patches in this
series to refactor fw_devlink to be more efficient. Most of the code has
been moved out of firmware specific (DT mostly) code into driver core.

This brings the following benefits:
- Instead of parsing the device tree multiple times during bootup,
  fw_devlink parses each fwnode node/property only once and creates
  fwnode links. The rest of the fw_devlink code then just looks at these
  fwnode links to do rest of the work.

- Makes it much easier to debug probe issue due to fw_devlink in the
  future. fw_devlink=on blocks the probing of devices if they depend on
  a device that hasn't been added yet. With this refactor, it'll be very
  easy to tell what that device is because we now have a reference to
  the fwnode of the device.

- Much easier to add fw_devlink support to ACPI and other firmware
  types. A refactor to move the common bits from DT specific code to
  driver core was in my TODO list as a prerequisite to adding ACPI
  support to fw_devlink. This series gets that done.

[1] - https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/
Signed-off-by: Saravana Kannan <saravanak@google.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/base/core.c    | 325 ++++++++++++++++++++++++++++++-----------
 include/linux/device.h |   5 -
 2 files changed, 238 insertions(+), 92 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 1873cecb0cc4..9edf9084fc98 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -46,8 +46,6 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
 #endif
 
 /* Device links support. */
-static LIST_HEAD(wait_for_suppliers);
-static DEFINE_MUTEX(wfs_lock);
 static LIST_HEAD(deferred_sync);
 static unsigned int defer_sync_state_count = 1;
 static DEFINE_MUTEX(fwnode_link_lock);
@@ -803,74 +801,6 @@ struct device_link *device_link_add(struct device *consumer,
 }
 EXPORT_SYMBOL_GPL(device_link_add);
 
-/**
- * device_link_wait_for_supplier - Add device to wait_for_suppliers list
- * @consumer: Consumer device
- *
- * Marks the @consumer device as waiting for suppliers to become available by
- * adding it to the wait_for_suppliers list. The consumer device will never be
- * probed until it's removed from the wait_for_suppliers list.
- *
- * The caller is responsible for adding the links to the supplier devices once
- * they are available and removing the @consumer device from the
- * wait_for_suppliers list once links to all the suppliers have been created.
- *
- * 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,
-					  bool need_for_probe)
-{
-	mutex_lock(&wfs_lock);
-	list_add_tail(&consumer->links.needs_suppliers, &wait_for_suppliers);
-	consumer->links.need_for_probe = need_for_probe;
-	mutex_unlock(&wfs_lock);
-}
-
-static void device_link_wait_for_mandatory_supplier(struct device *consumer)
-{
-	device_link_wait_for_supplier(consumer, true);
-}
-
-static void device_link_wait_for_optional_supplier(struct device *consumer)
-{
-	device_link_wait_for_supplier(consumer, false);
-}
-
-/**
- * device_link_add_missing_supplier_links - Add links from consumer devices to
- *					    supplier devices, leaving any
- *					    consumer with inactive suppliers on
- *					    the wait_for_suppliers list
- *
- * Loops through all consumers waiting on suppliers and tries to add all their
- * supplier links. If that succeeds, the consumer device is removed from
- * wait_for_suppliers list. Otherwise, they are left in the wait_for_suppliers
- * list.  Devices left on the wait_for_suppliers list will not be probed.
- *
- * The fwnode add_links 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_add_missing_supplier_links(void)
-{
-	struct device *dev, *tmp;
-
-	mutex_lock(&wfs_lock);
-	list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
-				 links.needs_suppliers) {
-		int ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
-		if (!ret)
-			list_del_init(&dev->links.needs_suppliers);
-		else if (ret != -ENODEV)
-			dev->links.need_for_probe = false;
-	}
-	mutex_unlock(&wfs_lock);
-}
-
 #ifdef CONFIG_SRCU
 static void __device_link_del(struct kref *kref)
 {
@@ -1195,9 +1125,8 @@ void device_links_driver_bound(struct device *dev)
 	 * the device links it needs to or make new device links as it needs
 	 * them. So, it no longer needs to wait on any suppliers.
 	 */
-	mutex_lock(&wfs_lock);
-	list_del_init(&dev->links.needs_suppliers);
-	mutex_unlock(&wfs_lock);
+	if (dev->fwnode && dev->fwnode->dev == dev)
+		fwnode_links_purge_suppliers(dev->fwnode);
 	device_remove_file(dev, &dev_attr_waiting_for_supplier);
 
 	device_links_write_lock();
@@ -1488,10 +1417,6 @@ static void device_links_purge(struct device *dev)
 	if (dev->class == &devlink_class)
 		return;
 
-	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).
@@ -1561,19 +1486,246 @@ static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode)
 		fw_devlink_parse_fwtree(child);
 }
 
-static void fw_devlink_link_device(struct device *dev)
+/**
+ * fw_devlink_create_devlink - Create a device link from a consumer to fwnode
+ * @con - Consumer device for the device link
+ * @sup_handle - fwnode handle of supplier
+ *
+ * This function will try to create a device link between the consumer device
+ * @con and the supplier device represented by @sup_handle.
+ *
+ * The supplier has to be provided as a fwnode because incorrect cycles in
+ * fwnode links can sometimes cause the supplier device to never be created.
+ * This function detects such cases and returns an error if it cannot create a
+ * device link from the consumer to a missing supplier.
+ *
+ * Returns,
+ * 0 on successfully creating a device link
+ * -EINVAL if the device link cannot be created as expected
+ * -EAGAIN if the device link cannot be created right now, but it may be
+ *  possible to do that in the future
+ */
+static int fw_devlink_create_devlink(struct device *con,
+				     struct fwnode_handle *sup_handle, u32 flags)
+{
+	struct device *sup_dev;
+	int ret = 0;
+
+	sup_dev = get_dev_from_fwnode(sup_handle);
+	if (sup_dev) {
+		/*
+		 * If this fails, it is due to cycles in device links.  Just
+		 * give up on this link and treat it as invalid.
+		 */
+		if (!device_link_add(con, sup_dev, flags))
+			ret = -EINVAL;
+
+		goto out;
+	}
+
+	/*
+	 * DL_FLAG_SYNC_STATE_ONLY doesn't block probing and supports
+	 * cycles. So cycle detection isn't necessary and shouldn't be
+	 * done.
+	 */
+	if (flags & DL_FLAG_SYNC_STATE_ONLY)
+		return -EAGAIN;
+
+	/*
+	 * If we can't find the supplier device from its fwnode, it might be
+	 * due to a cyclic dependency between fwnodes. Some of these cycles can
+	 * be broken by applying logic. Check for these types of cycles and
+	 * break them so that devices in the cycle probe properly.
+	 *
+	 * If the supplier's parent is dependent on the consumer, then
+	 * the consumer-supplier dependency is a false dependency. So,
+	 * treat it as an invalid link.
+	 */
+	sup_dev = fwnode_get_next_parent_dev(sup_handle);
+	if (sup_dev && device_is_dependent(con, sup_dev)) {
+		dev_dbg(con, "Not linking to %pfwP - False link\n",
+			sup_handle);
+		ret = -EINVAL;
+	} else {
+		/*
+		 * Can't check for cycles or no cycles. So let's try
+		 * again later.
+		 */
+		ret = -EAGAIN;
+	}
+
+out:
+	put_device(sup_dev);
+	return ret;
+}
+
+/**
+ * __fw_devlink_link_to_consumers - Create device links to consumers of a device
+ * @dev - Device that needs to be linked to its consumers
+ *
+ * This function looks at all the consumer fwnodes of @dev and creates device
+ * links between the consumer device and @dev (supplier).
+ *
+ * If the consumer device has not been added yet, then this function creates a
+ * SYNC_STATE_ONLY link between @dev (supplier) and the closest ancestor device
+ * of the consumer fwnode. This is necessary to make sure @dev doesn't get a
+ * sync_state() callback before the real consumer device gets to be added and
+ * then probed.
+ *
+ * Once device links are created from the real consumer to @dev (supplier), the
+ * fwnode links are deleted.
+ */
+static void __fw_devlink_link_to_consumers(struct device *dev)
+{
+	struct fwnode_handle *fwnode = dev->fwnode;
+	struct fwnode_link *link, *tmp;
+
+	list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) {
+		u32 dl_flags = fw_devlink_get_flags();
+		struct device *con_dev;
+		bool own_link = true;
+		int ret;
+
+		con_dev = get_dev_from_fwnode(link->consumer);
+		/*
+		 * If consumer device is not available yet, make a "proxy"
+		 * SYNC_STATE_ONLY link from the consumer's parent device to
+		 * the supplier device. This is necessary to make sure the
+		 * supplier doesn't get a sync_state() callback before the real
+		 * consumer can create a device link to the supplier.
+		 *
+		 * This proxy link step is needed to handle the case where the
+		 * consumer's parent device is added before the supplier.
+		 */
+		if (!con_dev) {
+			con_dev = fwnode_get_next_parent_dev(link->consumer);
+			/*
+			 * However, if the consumer's parent device is also the
+			 * parent of the supplier, don't create a
+			 * consumer-supplier link from the parent to its child
+			 * device. Such a dependency is impossible.
+			 */
+			if (con_dev &&
+			    fwnode_is_ancestor_of(con_dev->fwnode, fwnode)) {
+				put_device(con_dev);
+				con_dev = NULL;
+			} else {
+				own_link = false;
+				dl_flags = DL_FLAG_SYNC_STATE_ONLY;
+			}
+		}
+
+		if (!con_dev)
+			continue;
+
+		ret = fw_devlink_create_devlink(con_dev, fwnode, dl_flags);
+		put_device(con_dev);
+		if (!own_link || ret == -EAGAIN)
+			continue;
+
+		list_del(&link->s_hook);
+		list_del(&link->c_hook);
+		kfree(link);
+	}
+}
+
+/**
+ * __fw_devlink_link_to_suppliers - Create device links to suppliers of a device
+ * @dev - The consumer device that needs to be linked to its suppliers
+ * @fwnode - Root of the fwnode tree that is used to create device links
+ *
+ * This function looks at all the supplier fwnodes of fwnode tree rooted at
+ * @fwnode and creates device links between @dev (consumer) and all the
+ * supplier devices of the entire fwnode tree at @fwnode.
+ *
+ * The function creates normal (non-SYNC_STATE_ONLY) device links between @dev
+ * and the real suppliers of @dev. Once these device links are created, the
+ * fwnode links are deleted. When such device links are successfully created,
+ * this function is called recursively on those supplier devices. This is
+ * needed to detect and break some invalid cycles in fwnode links.  See
+ * fw_devlink_create_devlink() for more details.
+ *
+ * In addition, it also looks at all the suppliers of the entire fwnode tree
+ * because some of the child devices of @dev that have not been added yet
+ * (because @dev hasn't probed) might already have their suppliers added to
+ * driver core. So, this function creates SYNC_STATE_ONLY device links between
+ * @dev (consumer) and these suppliers to make sure they don't execute their
+ * sync_state() callbacks before these child devices have a chance to create
+ * their device links. The fwnode links that correspond to the child devices
+ * aren't delete because they are needed later to create the device links
+ * between the real consumer and supplier devices.
+ */
+static void __fw_devlink_link_to_suppliers(struct device *dev,
+					   struct fwnode_handle *fwnode)
 {
-	int fw_ret;
+	bool own_link = (dev->fwnode == fwnode);
+	struct fwnode_link *link, *tmp;
+	struct fwnode_handle *child = NULL;
+	u32 dl_flags;
+
+	if (own_link)
+		dl_flags = fw_devlink_get_flags();
+	else
+		dl_flags = DL_FLAG_SYNC_STATE_ONLY;
 
-	device_link_add_missing_supplier_links();
+	list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) {
+		int ret;
+		struct device *sup_dev;
+		struct fwnode_handle *sup = link->supplier;
+
+		ret = fw_devlink_create_devlink(dev, sup, dl_flags);
+		if (!own_link || ret == -EAGAIN)
+			continue;
 
-	if (fw_devlink_flags && fwnode_has_op(dev->fwnode, add_links)) {
-		fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
-		if (fw_ret == -ENODEV && !fw_devlink_is_permissive())
-			device_link_wait_for_mandatory_supplier(dev);
-		else if (fw_ret)
-			device_link_wait_for_optional_supplier(dev);
+		list_del(&link->s_hook);
+		list_del(&link->c_hook);
+		kfree(link);
+
+		/* If no device link was created, nothing more to do. */
+		if (ret)
+			continue;
+
+		/*
+		 * If a device link was successfully created to a supplier, we
+		 * now need to try and link the supplier to all its suppliers.
+		 *
+		 * This is needed to detect and delete false dependencies in
+		 * fwnode links that haven't been converted to a device link
+		 * yet. See comments in fw_devlink_create_devlink() for more
+		 * details on the false dependency.
+		 *
+		 * Without deleting these false dependencies, some devices will
+		 * never probe because they'll keep waiting for their false
+		 * dependency fwnode links to be converted to device links.
+		 */
+		sup_dev = get_dev_from_fwnode(sup);
+		__fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode);
+		put_device(sup_dev);
 	}
+
+	/*
+	 * Make "proxy" SYNC_STATE_ONLY device links to represent the needs of
+	 * all the descendants. This proxy link step is needed to handle the
+	 * case where the supplier is added before the consumer's parent device
+	 * (@dev).
+	 */
+	while ((child = fwnode_get_next_available_child_node(fwnode, child)))
+		__fw_devlink_link_to_suppliers(dev, child);
+}
+
+static void fw_devlink_link_device(struct device *dev)
+{
+	struct fwnode_handle *fwnode = dev->fwnode;
+
+	if (!fw_devlink_flags)
+		return;
+
+	fw_devlink_parse_fwtree(fwnode);
+
+	mutex_lock(&fwnode_link_lock);
+	__fw_devlink_link_to_consumers(dev);
+	__fw_devlink_link_to_suppliers(dev, fwnode);
+	mutex_unlock(&fwnode_link_lock);
 }
 
 /* Device links support end. */
@@ -2431,7 +2583,6 @@ 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);
 	INIT_LIST_HEAD(&dev->links.defer_sync);
 	dev->links.status = DL_DEV_NO_DRIVER;
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index 1e771ea4dca6..89bb8b84173e 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -351,18 +351,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.
  * @defer_sync: Hook to global list of devices that have deferred sync_state.
- * @need_for_probe: If needs_suppliers is on a list, this indicates if the
- *		    suppliers are needed for probe or not.
  * @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;
-	bool need_for_probe;
 	enum dl_dev_state status;
 };
 
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 17/17] driver core: Delete pointless parameter in fwnode_operations.add_links

[Saravana Kannan]

The struct device input to add_links() is not used for anything. So
delete it.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c             | 2 +-
 drivers/firmware/efi/efi-init.c | 3 +--
 drivers/of/property.c           | 3 +--
 include/linux/fwnode.h          | 3 +--
 4 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 9edf9084fc98..63edb8bd9d7d 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1472,7 +1472,7 @@ static void fw_devlink_parse_fwnode(struct fwnode_handle *fwnode)
 	if (fwnode->flags & FWNODE_FLAG_LINKS_ADDED)
 		return;
 
-	fwnode_call_int_op(fwnode, add_links, NULL);
+	fwnode_call_int_op(fwnode, add_links);
 	fwnode->flags |= FWNODE_FLAG_LINKS_ADDED;
 }
 
diff --git a/drivers/firmware/efi/efi-init.c b/drivers/firmware/efi/efi-init.c
index c0c3d4c3837a..a552a08a1741 100644
--- a/drivers/firmware/efi/efi-init.c
+++ b/drivers/firmware/efi/efi-init.c
@@ -316,8 +316,7 @@ static struct device_node *find_pci_overlap_node(void)
  * resource reservation conflict on the memory window that the efifb
  * framebuffer steals from the PCIe host bridge.
  */
-static int efifb_add_links(struct fwnode_handle *fwnode,
-			   struct device *dev)
+static int efifb_add_links(struct fwnode_handle *fwnode)
 {
 	struct device_node *sup_np;
 
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 620d29fdace8..5f9eed79a8aa 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1343,8 +1343,7 @@ static int of_link_property(struct device_node *con_np, const char *prop_name)
 	return ret;
 }
 
-static int of_fwnode_add_links(struct fwnode_handle *fwnode,
-			       struct device *dev)
+static int of_fwnode_add_links(struct fwnode_handle *fwnode)
 {
 	struct property *p;
 	struct device_node *con_np = to_of_node(fwnode);
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index ffa9129182a6..fde4ad97564c 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -127,8 +127,7 @@ struct fwnode_operations {
 	(*graph_get_port_parent)(struct fwnode_handle *fwnode);
 	int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode,
 				    struct fwnode_endpoint *endpoint);
-	int (*add_links)(struct fwnode_handle *fwnode,
-			 struct device *dev);
+	int (*add_links)(struct fwnode_handle *fwnode);
 };
 
 #define fwnode_has_op(fwnode, op)				\
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v2 00/17] Refactor fw_devlink to significantly improve boot time

[Tomi Valkeinen]

Hi,

On 21/11/2020 04:02, Saravana Kannan wrote:
> The current implementation of fw_devlink is very inefficient because it
> tries to get away without creating fwnode links in the name of saving
> memory usage. Past attempts to optimize runtime at the cost of memory
> usage were blocked with request for data showing that the optimization
> made significant improvement for real world scenarios.
> 
> We have those scenarios now. There have been several reports of boot
> time increase in the order of seconds in this thread [1]. Several OEMs
> and SoC manufacturers have also privately reported significant
> (350-400ms) increase in boot time due to all the parsing done by
> fw_devlink.
> 
> So this patch series refactors fw_devlink to be more efficient. The key
> difference now is the addition of support for fwnode links -- just a few
> simple APIs. This also allows most of the code to be moved out of
> firmware specific (DT mostly) code into driver core.
> 
> This brings the following benefits:
> - Instead of parsing the device tree multiple times (complexity was
>   close to O(N^3) where N in the number of properties) during bootup,
>   fw_devlink parses each fwnode node/property only once and creates
>   fwnode links. The rest of the fw_devlink code then just looks at these
>   fwnode links to do rest of the work.
> 
> - Makes it much easier to debug probe issue due to fw_devlink in the
>   future. fw_devlink=on blocks the probing of devices if they depend on
>   a device that hasn't been added yet. With this refactor, it'll be very
>   easy to tell what that device is because we now have a reference to
>   the fwnode of the device.
> 
> - Much easier to add fw_devlink support to ACPI and other firmware
>   types. A refactor to move the common bits from DT specific code to
>   driver core was in my TODO list as a prerequisite to adding ACPI
>   support to fw_devlink. This series gets that done.
> 
> Laurent and Grygorii tested the v1 series and they saw boot time
> improvment of about 12 seconds and 3 seconds, respectively.

Tested v2 on OMAP4 SDP. With my particular config, boot time to starting init went from 18.5 seconds
to 12.5 seconds.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 00/17] Refactor fw_devlink to significantly improve boot time

[Saravana Kannan]

On Tue, Nov 24, 2020 at 12:29 AM 'Tomi Valkeinen' via kernel-team
<kernel-team@android.com> wrote:
>
> Hi,
>
> On 21/11/2020 04:02, Saravana Kannan wrote:
> > The current implementation of fw_devlink is very inefficient because it
> > tries to get away without creating fwnode links in the name of saving
> > memory usage. Past attempts to optimize runtime at the cost of memory
> > usage were blocked with request for data showing that the optimization
> > made significant improvement for real world scenarios.
> >
> > We have those scenarios now. There have been several reports of boot
> > time increase in the order of seconds in this thread [1]. Several OEMs
> > and SoC manufacturers have also privately reported significant
> > (350-400ms) increase in boot time due to all the parsing done by
> > fw_devlink.
> >
> > So this patch series refactors fw_devlink to be more efficient. The key
> > difference now is the addition of support for fwnode links -- just a few
> > simple APIs. This also allows most of the code to be moved out of
> > firmware specific (DT mostly) code into driver core.
> >
> > This brings the following benefits:
> > - Instead of parsing the device tree multiple times (complexity was
> >   close to O(N^3) where N in the number of properties) during bootup,
> >   fw_devlink parses each fwnode node/property only once and creates
> >   fwnode links. The rest of the fw_devlink code then just looks at these
> >   fwnode links to do rest of the work.
> >
> > - Makes it much easier to debug probe issue due to fw_devlink in the
> >   future. fw_devlink=on blocks the probing of devices if they depend on
> >   a device that hasn't been added yet. With this refactor, it'll be very
> >   easy to tell what that device is because we now have a reference to
> >   the fwnode of the device.
> >
> > - Much easier to add fw_devlink support to ACPI and other firmware
> >   types. A refactor to move the common bits from DT specific code to
> >   driver core was in my TODO list as a prerequisite to adding ACPI
> >   support to fw_devlink. This series gets that done.
> >
> > Laurent and Grygorii tested the v1 series and they saw boot time
> > improvment of about 12 seconds and 3 seconds, respectively.
>
> Tested v2 on OMAP4 SDP. With my particular config, boot time to starting init went from 18.5 seconds
> to 12.5 seconds.

Thanks for testing Tomi!

-Saravana

Re: [PATCH v2 00/17] Refactor fw_devlink to significantly improve boot time

[Saravana Kannan]

On Tue, Nov 24, 2020 at 12:29 AM 'Tomi Valkeinen' via kernel-team
<kernel-team@android.com> wrote:
>
> Hi,
>
> On 21/11/2020 04:02, Saravana Kannan wrote:
> > The current implementation of fw_devlink is very inefficient because it
> > tries to get away without creating fwnode links in the name of saving
> > memory usage. Past attempts to optimize runtime at the cost of memory
> > usage were blocked with request for data showing that the optimization
> > made significant improvement for real world scenarios.
> >
> > We have those scenarios now. There have been several reports of boot
> > time increase in the order of seconds in this thread [1]. Several OEMs
> > and SoC manufacturers have also privately reported significant
> > (350-400ms) increase in boot time due to all the parsing done by
> > fw_devlink.
> >
> > So this patch series refactors fw_devlink to be more efficient. The key
> > difference now is the addition of support for fwnode links -- just a few
> > simple APIs. This also allows most of the code to be moved out of
> > firmware specific (DT mostly) code into driver core.
> >
> > This brings the following benefits:
> > - Instead of parsing the device tree multiple times (complexity was
> >   close to O(N^3) where N in the number of properties) during bootup,
> >   fw_devlink parses each fwnode node/property only once and creates
> >   fwnode links. The rest of the fw_devlink code then just looks at these
> >   fwnode links to do rest of the work.
> >
> > - Makes it much easier to debug probe issue due to fw_devlink in the
> >   future. fw_devlink=on blocks the probing of devices if they depend on
> >   a device that hasn't been added yet. With this refactor, it'll be very
> >   easy to tell what that device is because we now have a reference to
> >   the fwnode of the device.
> >
> > - Much easier to add fw_devlink support to ACPI and other firmware
> >   types. A refactor to move the common bits from DT specific code to
> >   driver core was in my TODO list as a prerequisite to adding ACPI
> >   support to fw_devlink. This series gets that done.
> >
> > Laurent and Grygorii tested the v1 series and they saw boot time
> > improvment of about 12 seconds and 3 seconds, respectively.
>
> Tested v2 on OMAP4 SDP. With my particular config, boot time to starting init went from 18.5 seconds
> to 12.5 seconds.
>
>  Tomi

Rafael,

Friendly reminder for a review.

-Saravana

Re: [PATCH v2 07/17] driver core: Add fwnode_init()

[Leon Romanovsky]

On Fri, Nov 20, 2020 at 06:02:22PM -0800, Saravana Kannan wrote:
> There are multiple locations in the kernel where a struct fwnode_handle
> is initialized. Add fwnode_init() so that we have one way of
> initializing a fwnode_handle.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/acpi/property.c         | 2 +-
>  drivers/acpi/scan.c             | 2 +-
>  drivers/base/swnode.c           | 2 +-
>  drivers/firmware/efi/efi-init.c | 8 ++++----
>  include/linux/fwnode.h          | 6 ++++++
>  include/linux/of.h              | 2 +-
>  kernel/irq/irqdomain.c          | 2 +-
>  7 files changed, 15 insertions(+), 9 deletions(-)

In this series, I didn't find any extension of fwnode_init() to be it more
than simple assignment. This change looks to me like unnecessary churn and
obfuscation rather than improvement.

"...ops = &...;" is pretty standard in the kernel to initialize ops
structures.

Thanks

Re: [PATCH v2 08/17] driver core: Add fwnode link support

[Leon Romanovsky]

On Fri, Nov 20, 2020 at 06:02:23PM -0800, Saravana Kannan wrote:
> Add support for creating supplier-consumer links between fwnodes.  It is
> intended for internal use the driver core and generic firmware support
> code (eg. Device Tree, ACPI), so it is simple by design and the API
> provided is limited.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c    | 98 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/of/dynamic.c   |  1 +
>  include/linux/fwnode.h | 14 ++++++
>  3 files changed, 113 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 401fa7e3505c..e2b246a44d1a 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -50,6 +50,104 @@ static LIST_HEAD(wait_for_suppliers);
>  static DEFINE_MUTEX(wfs_lock);
>  static LIST_HEAD(deferred_sync);
>  static unsigned int defer_sync_state_count = 1;
> +static DEFINE_MUTEX(fwnode_link_lock);
> +
> +/**
> + * fwnode_link_add - Create a link between two fwnode_handles.
> + * @con: Consumer end of the link.
> + * @sup: Supplier end of the link.
> + *
> + * Create a fwnode link between fwnode handles @con and @sup. The fwnode link
> + * represents the detail that the firmware lists @sup fwnode as supplying a
> + * resource to @con.
> + *
> + * The driver core will use the fwnode link to create a device link between the
> + * two device objects corresponding to @con and @sup when they are created. The
> + * driver core will automatically delete the fwnode link between @con and @sup
> + * after doing that.
> + *
> + * Attempts to create duplicate links between the same pair of fwnode handles
> + * are ignored and there is no reference counting.

Sorry to ask, but why is that?
Isn't this a programmer error?

Thanks

Re: [PATCH v2 08/17] driver core: Add fwnode link support

[Saravana Kannan]

On Sat, Dec 5, 2020 at 11:48 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Fri, Nov 20, 2020 at 06:02:23PM -0800, Saravana Kannan wrote:
> > Add support for creating supplier-consumer links between fwnodes.  It is
> > intended for internal use the driver core and generic firmware support
> > code (eg. Device Tree, ACPI), so it is simple by design and the API
> > provided is limited.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c    | 98 ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/of/dynamic.c   |  1 +
> >  include/linux/fwnode.h | 14 ++++++
> >  3 files changed, 113 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 401fa7e3505c..e2b246a44d1a 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -50,6 +50,104 @@ static LIST_HEAD(wait_for_suppliers);
> >  static DEFINE_MUTEX(wfs_lock);
> >  static LIST_HEAD(deferred_sync);
> >  static unsigned int defer_sync_state_count = 1;
> > +static DEFINE_MUTEX(fwnode_link_lock);
> > +
> > +/**
> > + * fwnode_link_add - Create a link between two fwnode_handles.
> > + * @con: Consumer end of the link.
> > + * @sup: Supplier end of the link.
> > + *
> > + * Create a fwnode link between fwnode handles @con and @sup. The fwnode link
> > + * represents the detail that the firmware lists @sup fwnode as supplying a
> > + * resource to @con.
> > + *
> > + * The driver core will use the fwnode link to create a device link between the
> > + * two device objects corresponding to @con and @sup when they are created. The
> > + * driver core will automatically delete the fwnode link between @con and @sup
> > + * after doing that.
> > + *
> > + * Attempts to create duplicate links between the same pair of fwnode handles
> > + * are ignored and there is no reference counting.
>
> Sorry to ask, but why is that?
> Isn't this a programmer error?

No, not a programmer error.

One firmware node can point to the same supplier many times. For
example, the consumer can be using multiple clocks from the same
supplier clock controller. In the context of fw_devlink, there's no
reason to keep track of each clock dependency separately because we'll
be creating only one device link from fwnode link. So multiple fwnode
link attempts between the same two devices are just treated as one
instance of dependency. I hope that clarifies things.

-Saravana

Re: [PATCH v2 07/17] driver core: Add fwnode_init()

[Saravana Kannan]

On Sat, Dec 5, 2020 at 11:26 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Fri, Nov 20, 2020 at 06:02:22PM -0800, Saravana Kannan wrote:
> > There are multiple locations in the kernel where a struct fwnode_handle
> > is initialized. Add fwnode_init() so that we have one way of
> > initializing a fwnode_handle.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/acpi/property.c         | 2 +-
> >  drivers/acpi/scan.c             | 2 +-
> >  drivers/base/swnode.c           | 2 +-
> >  drivers/firmware/efi/efi-init.c | 8 ++++----
> >  include/linux/fwnode.h          | 6 ++++++
> >  include/linux/of.h              | 2 +-
> >  kernel/irq/irqdomain.c          | 2 +-
> >  7 files changed, 15 insertions(+), 9 deletions(-)
>
> In this series, I didn't find any extension of fwnode_init() to be it more
> than simple assignment. This change looks to me like unnecessary churn and
> obfuscation rather than improvement.
>
> "...ops = &...;" is pretty standard in the kernel to initialize ops
> structures.

Subsequent patches make fwnode_init() do more stuff.

-Saravana

Re: [PATCH v2 07/17] driver core: Add fwnode_init()

[Leon Romanovsky]

On Mon, Dec 07, 2020 at 11:25:15AM -0800, Saravana Kannan wrote:
> On Sat, Dec 5, 2020 at 11:26 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Fri, Nov 20, 2020 at 06:02:22PM -0800, Saravana Kannan wrote:
> > > There are multiple locations in the kernel where a struct fwnode_handle
> > > is initialized. Add fwnode_init() so that we have one way of
> > > initializing a fwnode_handle.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/acpi/property.c         | 2 +-
> > >  drivers/acpi/scan.c             | 2 +-
> > >  drivers/base/swnode.c           | 2 +-
> > >  drivers/firmware/efi/efi-init.c | 8 ++++----
> > >  include/linux/fwnode.h          | 6 ++++++
> > >  include/linux/of.h              | 2 +-
> > >  kernel/irq/irqdomain.c          | 2 +-
> > >  7 files changed, 15 insertions(+), 9 deletions(-)
> >
> > In this series, I didn't find any extension of fwnode_init() to be it more
> > than simple assignment. This change looks to me like unnecessary churn and
> > obfuscation rather than improvement.
> >
> > "...ops = &...;" is pretty standard in the kernel to initialize ops
> > structures.
>
> Subsequent patches make fwnode_init() do more stuff.

But not in this series, right?

Thanks

>
> -Saravana

Re: [PATCH v2 08/17] driver core: Add fwnode link support

[Leon Romanovsky]

On Mon, Dec 07, 2020 at 11:25:03AM -0800, Saravana Kannan wrote:
> On Sat, Dec 5, 2020 at 11:48 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Fri, Nov 20, 2020 at 06:02:23PM -0800, Saravana Kannan wrote:
> > > Add support for creating supplier-consumer links between fwnodes.  It is
> > > intended for internal use the driver core and generic firmware support
> > > code (eg. Device Tree, ACPI), so it is simple by design and the API
> > > provided is limited.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/base/core.c    | 98 ++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/of/dynamic.c   |  1 +
> > >  include/linux/fwnode.h | 14 ++++++
> > >  3 files changed, 113 insertions(+)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 401fa7e3505c..e2b246a44d1a 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -50,6 +50,104 @@ static LIST_HEAD(wait_for_suppliers);
> > >  static DEFINE_MUTEX(wfs_lock);
> > >  static LIST_HEAD(deferred_sync);
> > >  static unsigned int defer_sync_state_count = 1;
> > > +static DEFINE_MUTEX(fwnode_link_lock);
> > > +
> > > +/**
> > > + * fwnode_link_add - Create a link between two fwnode_handles.
> > > + * @con: Consumer end of the link.
> > > + * @sup: Supplier end of the link.
> > > + *
> > > + * Create a fwnode link between fwnode handles @con and @sup. The fwnode link
> > > + * represents the detail that the firmware lists @sup fwnode as supplying a
> > > + * resource to @con.
> > > + *
> > > + * The driver core will use the fwnode link to create a device link between the
> > > + * two device objects corresponding to @con and @sup when they are created. The
> > > + * driver core will automatically delete the fwnode link between @con and @sup
> > > + * after doing that.
> > > + *
> > > + * Attempts to create duplicate links between the same pair of fwnode handles
> > > + * are ignored and there is no reference counting.
> >
> > Sorry to ask, but why is that?
> > Isn't this a programmer error?
>
> No, not a programmer error.
>
> One firmware node can point to the same supplier many times. For
> example, the consumer can be using multiple clocks from the same
> supplier clock controller. In the context of fw_devlink, there's no
> reason to keep track of each clock dependency separately because we'll
> be creating only one device link from fwnode link. So multiple fwnode
> link attempts between the same two devices are just treated as one
> instance of dependency. I hope that clarifies things.

Yes, thanks.

>
> -Saravana

Re: [PATCH v2 07/17] driver core: Add fwnode_init()

[Saravana Kannan]

On Mon, Dec 7, 2020 at 11:54 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, Dec 07, 2020 at 11:25:15AM -0800, Saravana Kannan wrote:
> > On Sat, Dec 5, 2020 at 11:26 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Fri, Nov 20, 2020 at 06:02:22PM -0800, Saravana Kannan wrote:
> > > > There are multiple locations in the kernel where a struct fwnode_handle
> > > > is initialized. Add fwnode_init() so that we have one way of
> > > > initializing a fwnode_handle.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/acpi/property.c         | 2 +-
> > > >  drivers/acpi/scan.c             | 2 +-
> > > >  drivers/base/swnode.c           | 2 +-
> > > >  drivers/firmware/efi/efi-init.c | 8 ++++----
> > > >  include/linux/fwnode.h          | 6 ++++++
> > > >  include/linux/of.h              | 2 +-
> > > >  kernel/irq/irqdomain.c          | 2 +-
> > > >  7 files changed, 15 insertions(+), 9 deletions(-)
> > >
> > > In this series, I didn't find any extension of fwnode_init() to be it more
> > > than simple assignment. This change looks to me like unnecessary churn and
> > > obfuscation rather than improvement.
> > >
> > > "...ops = &...;" is pretty standard in the kernel to initialize ops
> > > structures.
> >
> > Subsequent patches make fwnode_init() do more stuff.
>
> But not in this series, right?

In this series. The very next patch - Patch 8/17 :)

-Saravana

Re: [PATCH v2 05/17] Revert "of: platform: Batch fwnode parsing when adding all top level devices"

[Rob Herring]

On Fri, 20 Nov 2020 18:02:20 -0800, Saravana Kannan wrote:
> This reverts commit 93d2e4322aa74c1ad1e8c2160608eb9a960d69ff.
> 
> The fw_devlink_pause/resume() optimization attempt is getting replaced
> with a much more robust optimization by the end of this series. So, stop
> using those APIs.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/platform.c | 2 --
>  1 file changed, 2 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 07/17] driver core: Add fwnode_init()

[Rob Herring]

On Fri, 20 Nov 2020 18:02:22 -0800, Saravana Kannan wrote:
> There are multiple locations in the kernel where a struct fwnode_handle
> is initialized. Add fwnode_init() so that we have one way of
> initializing a fwnode_handle.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/acpi/property.c         | 2 +-
>  drivers/acpi/scan.c             | 2 +-
>  drivers/base/swnode.c           | 2 +-
>  drivers/firmware/efi/efi-init.c | 8 ++++----
>  include/linux/fwnode.h          | 6 ++++++
>  include/linux/of.h              | 2 +-
>  kernel/irq/irqdomain.c          | 2 +-
>  7 files changed, 15 insertions(+), 9 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 08/17] driver core: Add fwnode link support

[Rob Herring]

On Fri, 20 Nov 2020 18:02:23 -0800, Saravana Kannan wrote:
> Add support for creating supplier-consumer links between fwnodes.  It is
> intended for internal use the driver core and generic firmware support
> code (eg. Device Tree, ACPI), so it is simple by design and the API
> provided is limited.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c    | 98 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/of/dynamic.c   |  1 +
>  include/linux/fwnode.h | 14 ++++++
>  3 files changed, 113 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 14/17] of: property: Update implementation of add_links() to create fwnode links

[Rob Herring]

On Fri, 20 Nov 2020 18:02:29 -0800, Saravana Kannan wrote:
> The semantics of add_links() has changed from creating device link
> between devices to creating fwnode links between fwnodes. So, update the
> implementation of add_links() to match the new semantics.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/property.c | 150 ++++++++++++------------------------------
>  1 file changed, 41 insertions(+), 109 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 17/17] driver core: Delete pointless parameter in fwnode_operations.add_links

[Rob Herring]

On Fri, 20 Nov 2020 18:02:32 -0800, Saravana Kannan wrote:
> The struct device input to add_links() is not used for anything. So
> delete it.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c             | 2 +-
>  drivers/firmware/efi/efi-init.c | 3 +--
>  drivers/of/property.c           | 3 +--
>  include/linux/fwnode.h          | 3 +--
>  4 files changed, 4 insertions(+), 7 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 07/17] driver core: Add fwnode_init()

[Leon Romanovsky]

On Mon, Dec 07, 2020 at 12:36:43PM -0800, Saravana Kannan wrote:
> On Mon, Dec 7, 2020 at 11:54 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Mon, Dec 07, 2020 at 11:25:15AM -0800, Saravana Kannan wrote:
> > > On Sat, Dec 5, 2020 at 11:26 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Fri, Nov 20, 2020 at 06:02:22PM -0800, Saravana Kannan wrote:
> > > > > There are multiple locations in the kernel where a struct fwnode_handle
> > > > > is initialized. Add fwnode_init() so that we have one way of
> > > > > initializing a fwnode_handle.
> > > > >
> > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > ---
> > > > >  drivers/acpi/property.c         | 2 +-
> > > > >  drivers/acpi/scan.c             | 2 +-
> > > > >  drivers/base/swnode.c           | 2 +-
> > > > >  drivers/firmware/efi/efi-init.c | 8 ++++----
> > > > >  include/linux/fwnode.h          | 6 ++++++
> > > > >  include/linux/of.h              | 2 +-
> > > > >  kernel/irq/irqdomain.c          | 2 +-
> > > > >  7 files changed, 15 insertions(+), 9 deletions(-)
> > > >
> > > > In this series, I didn't find any extension of fwnode_init() to be it more
> > > > than simple assignment. This change looks to me like unnecessary churn and
> > > > obfuscation rather than improvement.
> > > >
> > > > "...ops = &...;" is pretty standard in the kernel to initialize ops
> > > > structures.
> > >
> > > Subsequent patches make fwnode_init() do more stuff.
> >
> > But not in this series, right?
>
> In this series. The very next patch - Patch 8/17 :)

Thanks, sorry for the noise.

>
> -Saravana

Re: [PATCH v2 00/17] Refactor fw_devlink to significantly improve boot time

[Greg Kroah-Hartman]

On Fri, Nov 20, 2020 at 06:02:15PM -0800, Saravana Kannan wrote:
> The current implementation of fw_devlink is very inefficient because it
> tries to get away without creating fwnode links in the name of saving
> memory usage. Past attempts to optimize runtime at the cost of memory
> usage were blocked with request for data showing that the optimization
> made significant improvement for real world scenarios.
> 
> We have those scenarios now. There have been several reports of boot
> time increase in the order of seconds in this thread [1]. Several OEMs
> and SoC manufacturers have also privately reported significant
> (350-400ms) increase in boot time due to all the parsing done by
> fw_devlink.
> 
> So this patch series refactors fw_devlink to be more efficient. The key
> difference now is the addition of support for fwnode links -- just a few
> simple APIs. This also allows most of the code to be moved out of
> firmware specific (DT mostly) code into driver core.
> 
> This brings the following benefits:
> - Instead of parsing the device tree multiple times (complexity was
>   close to O(N^3) where N in the number of properties) during bootup,
>   fw_devlink parses each fwnode node/property only once and creates
>   fwnode links. The rest of the fw_devlink code then just looks at these
>   fwnode links to do rest of the work.
> 
> - Makes it much easier to debug probe issue due to fw_devlink in the
>   future. fw_devlink=on blocks the probing of devices if they depend on
>   a device that hasn't been added yet. With this refactor, it'll be very
>   easy to tell what that device is because we now have a reference to
>   the fwnode of the device.
> 
> - Much easier to add fw_devlink support to ACPI and other firmware
>   types. A refactor to move the common bits from DT specific code to
>   driver core was in my TODO list as a prerequisite to adding ACPI
>   support to fw_devlink. This series gets that done.
> 
> Laurent and Grygorii tested the v1 series and they saw boot time
> improvment of about 12 seconds and 3 seconds, respectively.

Now queued up to my tree.  Note, I had to hand-apply patches 13 and 16
due to some reason (for 13, I have no idea, for 16 it was due to a
previous patch applied to my tree that I cc:ed you on.)

Verifying I got it all correct would be great :)

thanks,

greg k-h

Re: [PATCH v2 00/17] Refactor fw_devlink to significantly improve boot time

[Saravana Kannan]

On Wed, Dec 9, 2020 at 10:15 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Nov 20, 2020 at 06:02:15PM -0800, Saravana Kannan wrote:
> > The current implementation of fw_devlink is very inefficient because it
> > tries to get away without creating fwnode links in the name of saving
> > memory usage. Past attempts to optimize runtime at the cost of memory
> > usage were blocked with request for data showing that the optimization
> > made significant improvement for real world scenarios.
> >
> > We have those scenarios now. There have been several reports of boot
> > time increase in the order of seconds in this thread [1]. Several OEMs
> > and SoC manufacturers have also privately reported significant
> > (350-400ms) increase in boot time due to all the parsing done by
> > fw_devlink.
> >
> > So this patch series refactors fw_devlink to be more efficient. The key
> > difference now is the addition of support for fwnode links -- just a few
> > simple APIs. This also allows most of the code to be moved out of
> > firmware specific (DT mostly) code into driver core.
> >
> > This brings the following benefits:
> > - Instead of parsing the device tree multiple times (complexity was
> >   close to O(N^3) where N in the number of properties) during bootup,
> >   fw_devlink parses each fwnode node/property only once and creates
> >   fwnode links. The rest of the fw_devlink code then just looks at these
> >   fwnode links to do rest of the work.
> >
> > - Makes it much easier to debug probe issue due to fw_devlink in the
> >   future. fw_devlink=on blocks the probing of devices if they depend on
> >   a device that hasn't been added yet. With this refactor, it'll be very
> >   easy to tell what that device is because we now have a reference to
> >   the fwnode of the device.
> >
> > - Much easier to add fw_devlink support to ACPI and other firmware
> >   types. A refactor to move the common bits from DT specific code to
> >   driver core was in my TODO list as a prerequisite to adding ACPI
> >   support to fw_devlink. This series gets that done.
> >
> > Laurent and Grygorii tested the v1 series and they saw boot time
> > improvment of about 12 seconds and 3 seconds, respectively.
>
> Now queued up to my tree.  Note, I had to hand-apply patches 13 and 16
> due to some reason (for 13, I have no idea, for 16 it was due to a
> previous patch applied to my tree that I cc:ed you on.)
>
> Verifying I got it all correct would be great :)

A quick diff of drivers/base/core.c between driver-core-testing and my
local tree doesn't show any major diff (only some unrelated comment
fixes). So, it looks fine.

The patch 13 conflict is probably due to having to rebase the v2
series on top of this:
https://lore.kernel.org/lkml/20201104205431.3795207-1-saravanak@google.com/

And looks like Patch 16 was handled fine.

Thanks for applying the series.

-Saravana

Re: [PATCH v2 00/17] Refactor fw_devlink to significantly improve boot time

[Greg Kroah-Hartman]

On Wed, Dec 09, 2020 at 12:24:32PM -0800, Saravana Kannan wrote:
> On Wed, Dec 9, 2020 at 10:15 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Nov 20, 2020 at 06:02:15PM -0800, Saravana Kannan wrote:
> > > The current implementation of fw_devlink is very inefficient because it
> > > tries to get away without creating fwnode links in the name of saving
> > > memory usage. Past attempts to optimize runtime at the cost of memory
> > > usage were blocked with request for data showing that the optimization
> > > made significant improvement for real world scenarios.
> > >
> > > We have those scenarios now. There have been several reports of boot
> > > time increase in the order of seconds in this thread [1]. Several OEMs
> > > and SoC manufacturers have also privately reported significant
> > > (350-400ms) increase in boot time due to all the parsing done by
> > > fw_devlink.
> > >
> > > So this patch series refactors fw_devlink to be more efficient. The key
> > > difference now is the addition of support for fwnode links -- just a few
> > > simple APIs. This also allows most of the code to be moved out of
> > > firmware specific (DT mostly) code into driver core.
> > >
> > > This brings the following benefits:
> > > - Instead of parsing the device tree multiple times (complexity was
> > >   close to O(N^3) where N in the number of properties) during bootup,
> > >   fw_devlink parses each fwnode node/property only once and creates
> > >   fwnode links. The rest of the fw_devlink code then just looks at these
> > >   fwnode links to do rest of the work.
> > >
> > > - Makes it much easier to debug probe issue due to fw_devlink in the
> > >   future. fw_devlink=on blocks the probing of devices if they depend on
> > >   a device that hasn't been added yet. With this refactor, it'll be very
> > >   easy to tell what that device is because we now have a reference to
> > >   the fwnode of the device.
> > >
> > > - Much easier to add fw_devlink support to ACPI and other firmware
> > >   types. A refactor to move the common bits from DT specific code to
> > >   driver core was in my TODO list as a prerequisite to adding ACPI
> > >   support to fw_devlink. This series gets that done.
> > >
> > > Laurent and Grygorii tested the v1 series and they saw boot time
> > > improvment of about 12 seconds and 3 seconds, respectively.
> >
> > Now queued up to my tree.  Note, I had to hand-apply patches 13 and 16
> > due to some reason (for 13, I have no idea, for 16 it was due to a
> > previous patch applied to my tree that I cc:ed you on.)
> >
> > Verifying I got it all correct would be great :)
> 
> A quick diff of drivers/base/core.c between driver-core-testing and my
> local tree doesn't show any major diff (only some unrelated comment
> fixes). So, it looks fine.
> 
> The patch 13 conflict is probably due to having to rebase the v2
> series on top of this:
> https://lore.kernel.org/lkml/20201104205431.3795207-1-saravanak@google.com/
> 
> And looks like Patch 16 was handled fine.

Great, thanks for verifying!

greg k-h

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Qian Cai]

On Fri, 2020-11-20 at 18:02 -0800, Saravana Kannan wrote:
> The current implementation of fw_devlink is very inefficient because it
> tries to get away without creating fwnode links in the name of saving
> memory usage. Past attempts to optimize runtime at the cost of memory
> usage were blocked with request for data showing that the optimization
> made significant improvement for real world scenarios.
> 
> We have those scenarios now. There have been several reports of boot
> time increase in the order of seconds in this thread [1]. Several OEMs
> and SoC manufacturers have also privately reported significant
> (350-400ms) increase in boot time due to all the parsing done by
> fw_devlink.
> 
> So this patch uses all the setup done by the previous patches in this
> series to refactor fw_devlink to be more efficient. Most of the code has
> been moved out of firmware specific (DT mostly) code into driver core.
> 
> This brings the following benefits:
> - Instead of parsing the device tree multiple times during bootup,
>   fw_devlink parses each fwnode node/property only once and creates
>   fwnode links. The rest of the fw_devlink code then just looks at these
>   fwnode links to do rest of the work.
> 
> - Makes it much easier to debug probe issue due to fw_devlink in the
>   future. fw_devlink=on blocks the probing of devices if they depend on
>   a device that hasn't been added yet. With this refactor, it'll be very
>   easy to tell what that device is because we now have a reference to
>   the fwnode of the device.
> 
> - Much easier to add fw_devlink support to ACPI and other firmware
>   types. A refactor to move the common bits from DT specific code to
>   driver core was in my TODO list as a prerequisite to adding ACPI
>   support to fw_devlink. This series gets that done.
> 
> [1] - https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>

Reverting this commit and its dependency:

2d09e6eb4a6f driver core: Delete pointless parameter in fwnode_operations.add_links

from today's linux-next fixed a boot crash on an arm64 Thunder X2 server.

.config: https://cailca.coding.net/public/linux/mm/git/files/master/arm64.config

[   57.413929][    T1] ACPI: 5 ACPI AML tables successfully acquired and loaded
[   60.571643][    T1] ACPI: Interpreter enabled
[   60.576104][    T1] ACPI: Using GIC for interrupt routing
[   60.582474][    T1] ACPI: MCFG table detected, 1 entries
[   60.588051][    T1] ACPI: IORT: SMMU-v3[402300000] Mapped to Proximity domain 0
[   60.601374][    T1] Unable to handle kernel paging request at virtual address dfff800000000000
[   60.610146][    T1] Mem abort info:
[   60.613694][    T1]   ESR = 0x96000004
[   60.617496][    T1]   EC = 0x25: DABT (current EL), IL = 32 bits
[   60.623616][    T1]   SET = 0, FnV = 0
[   60.627420][    T1]   EA = 0, S1PTW = 0
[   60.631304][    T1] Data abort info:
[   60.634957][    T1]   ISV = 0, ISS = 0x00000004
[   60.639546][    T1]   CM = 0, WnR = 0
[   60.643255][    T1] [dfff800000000000] address between user and kernel address ranges
[   60.651226][    T1] Internal error: Oops: 96000004 [#1] SMP
[   60.656864][    T1] Modules linked in:
[   60.660658][    T1] CPU: 38 PID: 1 Comm: swapper/0 Tainted: G        W         5.10.0-rc7-next-20201211 #2
[   60.670424][    T1] Hardware name: HPE Apollo 70             /C01_APACHE_MB         , BIOS L50_5.13_1.16 07/29/2020
[   60.680979][    T1] pstate: 10400009 (nzcV daif +PAN -UAO -TCO BTYPE=--)
[   60.687757][    T1] pc : device_add+0xf60/0x16b0
[   60.692430][    T1] lr : device_add+0xf08/0x16b0
[   60.697098][    T1] sp : ffff0000063bf7d0
[   60.701147][    T1] x29: ffff0000063bf7d0 x28: ffff00001f760810 
[   60.707226][    T1] x27: fffffffffffffff8 x26: ffff00001f760858 
[   60.713304][    T1] x25: ffff00001f760c58 x24: fffffffffffffff8 
[   60.719381][    T1] x23: 1fffe00003eec10b x22: ffff8000190d0260 
[   60.725458][    T1] x21: ffff800011dba708 x20: 0000000000000000 
[   60.731535][    T1] x19: 1fffe00000c77f10 x18: 1fffe001cf0d53ed 
[   60.737616][    T1] x17: 0000000000000000 x16: 1ffff000033676f1 
[   60.743709][    T1] x15: 0000000000000000 x14: ffff800011731e34 
[   60.749786][    T1] x13: ffff700003217fb9 x12: 1ffff00003217fb8 
[   60.755864][    T1] x11: 1ffff00003217fb8 x10: ffff700003217fb8 
[   60.761940][    T1] x9 : dfff800000000000 x8 : ffff8000190bfdc7 
[   60.768017][    T1] x7 : 0000000000000001 x6 : ffff700003217fb9 
[   60.774094][    T1] x5 : ffff700003217fb9 x4 : ffff000006324a80 
[   60.780170][    T1] x3 : 1fffe00000c64951 x2 : 0000000000000000 
[   60.786247][    T1] x1 : 0000000000000000 x0 : dfff800000000000 
[   60.792324][    T1] Call trace:
[   60.795495][    T1]  device_add+0xf60/0x16b0
__fw_devlink_link_to_consumers at drivers/base/core.c:1583
(inlined by) fw_devlink_link_device at drivers/base/core.c:1726
(inlined by) device_add at drivers/base/core.c:3088
[   60.799813][    T1]  platform_device_add+0x274/0x628
[   60.804833][    T1]  acpi_iort_init+0x9d8/0xc50
[   60.809415][    T1]  acpi_init+0x45c/0x4e8
[   60.813556][    T1]  do_one_initcall+0x170/0xb70
[   60.818224][    T1]  kernel_init_freeable+0x6a8/0x734
[   60.823332][    T1]  kernel_init+0x18/0x12c
[   60.827566][    T1]  ret_from_fork+0x10/0x1c
[   60.831890][    T1] Code: f2fbffe0 d100205b d343fc41 aa1b03f8 (38e06820) 
[   60.838756][    T1] ---[ end trace fa5c8ce17a226d83 ]---
[   60.844127][    T1] Kernel panic - not syncing: Oops: Fatal exception
[   60.850881][    T1] SMP: stopping secondary CPUs
[   60.855733][    T1] ---[ end Kernel panic - not syncing: Oops: Fatal exception ]---
> ---
>  drivers/base/core.c    | 325 ++++++++++++++++++++++++++++++-----------
>  include/linux/device.h |   5 -
>  2 files changed, 238 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 1873cecb0cc4..9edf9084fc98 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -46,8 +46,6 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
>  #endif
>  
>  /* Device links support. */
> -static LIST_HEAD(wait_for_suppliers);
> -static DEFINE_MUTEX(wfs_lock);
>  static LIST_HEAD(deferred_sync);
>  static unsigned int defer_sync_state_count = 1;
>  static DEFINE_MUTEX(fwnode_link_lock);
> @@ -803,74 +801,6 @@ struct device_link *device_link_add(struct device *consumer,
>  }
>  EXPORT_SYMBOL_GPL(device_link_add);
>  
> -/**
> - * device_link_wait_for_supplier - Add device to wait_for_suppliers list
> - * @consumer: Consumer device
> - *
> - * Marks the @consumer device as waiting for suppliers to become available by
> - * adding it to the wait_for_suppliers list. The consumer device will never be
> - * probed until it's removed from the wait_for_suppliers list.
> - *
> - * The caller is responsible for adding the links to the supplier devices once
> - * they are available and removing the @consumer device from the
> - * wait_for_suppliers list once links to all the suppliers have been created.
> - *
> - * 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,
> -					  bool need_for_probe)
> -{
> -	mutex_lock(&wfs_lock);
> -	list_add_tail(&consumer->links.needs_suppliers, &wait_for_suppliers);
> -	consumer->links.need_for_probe = need_for_probe;
> -	mutex_unlock(&wfs_lock);
> -}
> -
> -static void device_link_wait_for_mandatory_supplier(struct device *consumer)
> -{
> -	device_link_wait_for_supplier(consumer, true);
> -}
> -
> -static void device_link_wait_for_optional_supplier(struct device *consumer)
> -{
> -	device_link_wait_for_supplier(consumer, false);
> -}
> -
> -/**
> - * device_link_add_missing_supplier_links - Add links from consumer devices to
> - *					    supplier devices, leaving any
> - *					    consumer with inactive suppliers on
> - *					    the wait_for_suppliers list
> - *
> - * Loops through all consumers waiting on suppliers and tries to add all their
> - * supplier links. If that succeeds, the consumer device is removed from
> - * wait_for_suppliers list. Otherwise, they are left in the wait_for_suppliers
> - * list.  Devices left on the wait_for_suppliers list will not be probed.
> - *
> - * The fwnode add_links 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_add_missing_supplier_links(void)
> -{
> -	struct device *dev, *tmp;
> -
> -	mutex_lock(&wfs_lock);
> -	list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
> -				 links.needs_suppliers) {
> -		int ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
> -		if (!ret)
> -			list_del_init(&dev->links.needs_suppliers);
> -		else if (ret != -ENODEV)
> -			dev->links.need_for_probe = false;
> -	}
> -	mutex_unlock(&wfs_lock);
> -}
> -
>  #ifdef CONFIG_SRCU
>  static void __device_link_del(struct kref *kref)
>  {
> @@ -1195,9 +1125,8 @@ void device_links_driver_bound(struct device *dev)
>  	 * the device links it needs to or make new device links as it needs
>  	 * them. So, it no longer needs to wait on any suppliers.
>  	 */
> -	mutex_lock(&wfs_lock);
> -	list_del_init(&dev->links.needs_suppliers);
> -	mutex_unlock(&wfs_lock);
> +	if (dev->fwnode && dev->fwnode->dev == dev)
> +		fwnode_links_purge_suppliers(dev->fwnode);
>  	device_remove_file(dev, &dev_attr_waiting_for_supplier);
>  
>  	device_links_write_lock();
> @@ -1488,10 +1417,6 @@ static void device_links_purge(struct device *dev)
>  	if (dev->class == &devlink_class)
>  		return;
>  
> -	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).
> @@ -1561,19 +1486,246 @@ static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode)
>  		fw_devlink_parse_fwtree(child);
>  }
>  
> -static void fw_devlink_link_device(struct device *dev)
> +/**
> + * fw_devlink_create_devlink - Create a device link from a consumer to fwnode
> + * @con - Consumer device for the device link
> + * @sup_handle - fwnode handle of supplier
> + *
> + * This function will try to create a device link between the consumer device
> + * @con and the supplier device represented by @sup_handle.
> + *
> + * The supplier has to be provided as a fwnode because incorrect cycles in
> + * fwnode links can sometimes cause the supplier device to never be created.
> + * This function detects such cases and returns an error if it cannot create a
> + * device link from the consumer to a missing supplier.
> + *
> + * Returns,
> + * 0 on successfully creating a device link
> + * -EINVAL if the device link cannot be created as expected
> + * -EAGAIN if the device link cannot be created right now, but it may be
> + *  possible to do that in the future
> + */
> +static int fw_devlink_create_devlink(struct device *con,
> +				     struct fwnode_handle *sup_handle, u32 flags)
> +{
> +	struct device *sup_dev;
> +	int ret = 0;
> +
> +	sup_dev = get_dev_from_fwnode(sup_handle);
> +	if (sup_dev) {
> +		/*
> +		 * If this fails, it is due to cycles in device links.  Just
> +		 * give up on this link and treat it as invalid.
> +		 */
> +		if (!device_link_add(con, sup_dev, flags))
> +			ret = -EINVAL;
> +
> +		goto out;
> +	}
> +
> +	/*
> +	 * DL_FLAG_SYNC_STATE_ONLY doesn't block probing and supports
> +	 * cycles. So cycle detection isn't necessary and shouldn't be
> +	 * done.
> +	 */
> +	if (flags & DL_FLAG_SYNC_STATE_ONLY)
> +		return -EAGAIN;
> +
> +	/*
> +	 * If we can't find the supplier device from its fwnode, it might be
> +	 * due to a cyclic dependency between fwnodes. Some of these cycles can
> +	 * be broken by applying logic. Check for these types of cycles and
> +	 * break them so that devices in the cycle probe properly.
> +	 *
> +	 * If the supplier's parent is dependent on the consumer, then
> +	 * the consumer-supplier dependency is a false dependency. So,
> +	 * treat it as an invalid link.
> +	 */
> +	sup_dev = fwnode_get_next_parent_dev(sup_handle);
> +	if (sup_dev && device_is_dependent(con, sup_dev)) {
> +		dev_dbg(con, "Not linking to %pfwP - False link\n",
> +			sup_handle);
> +		ret = -EINVAL;
> +	} else {
> +		/*
> +		 * Can't check for cycles or no cycles. So let's try
> +		 * again later.
> +		 */
> +		ret = -EAGAIN;
> +	}
> +
> +out:
> +	put_device(sup_dev);
> +	return ret;
> +}
> +
> +/**
> + * __fw_devlink_link_to_consumers - Create device links to consumers of a device
> + * @dev - Device that needs to be linked to its consumers
> + *
> + * This function looks at all the consumer fwnodes of @dev and creates device
> + * links between the consumer device and @dev (supplier).
> + *
> + * If the consumer device has not been added yet, then this function creates a
> + * SYNC_STATE_ONLY link between @dev (supplier) and the closest ancestor device
> + * of the consumer fwnode. This is necessary to make sure @dev doesn't get a
> + * sync_state() callback before the real consumer device gets to be added and
> + * then probed.
> + *
> + * Once device links are created from the real consumer to @dev (supplier), the
> + * fwnode links are deleted.
> + */
> +static void __fw_devlink_link_to_consumers(struct device *dev)
> +{
> +	struct fwnode_handle *fwnode = dev->fwnode;
> +	struct fwnode_link *link, *tmp;
> +
> +	list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) {
> +		u32 dl_flags = fw_devlink_get_flags();
> +		struct device *con_dev;
> +		bool own_link = true;
> +		int ret;
> +
> +		con_dev = get_dev_from_fwnode(link->consumer);
> +		/*
> +		 * If consumer device is not available yet, make a "proxy"
> +		 * SYNC_STATE_ONLY link from the consumer's parent device to
> +		 * the supplier device. This is necessary to make sure the
> +		 * supplier doesn't get a sync_state() callback before the real
> +		 * consumer can create a device link to the supplier.
> +		 *
> +		 * This proxy link step is needed to handle the case where the
> +		 * consumer's parent device is added before the supplier.
> +		 */
> +		if (!con_dev) {
> +			con_dev = fwnode_get_next_parent_dev(link->consumer);
> +			/*
> +			 * However, if the consumer's parent device is also the
> +			 * parent of the supplier, don't create a
> +			 * consumer-supplier link from the parent to its child
> +			 * device. Such a dependency is impossible.
> +			 */
> +			if (con_dev &&
> +			    fwnode_is_ancestor_of(con_dev->fwnode, fwnode)) {
> +				put_device(con_dev);
> +				con_dev = NULL;
> +			} else {
> +				own_link = false;
> +				dl_flags = DL_FLAG_SYNC_STATE_ONLY;
> +			}
> +		}
> +
> +		if (!con_dev)
> +			continue;
> +
> +		ret = fw_devlink_create_devlink(con_dev, fwnode, dl_flags);
> +		put_device(con_dev);
> +		if (!own_link || ret == -EAGAIN)
> +			continue;
> +
> +		list_del(&link->s_hook);
> +		list_del(&link->c_hook);
> +		kfree(link);
> +	}
> +}
> +
> +/**
> + * __fw_devlink_link_to_suppliers - Create device links to suppliers of a device
> + * @dev - The consumer device that needs to be linked to its suppliers
> + * @fwnode - Root of the fwnode tree that is used to create device links
> + *
> + * This function looks at all the supplier fwnodes of fwnode tree rooted at
> + * @fwnode and creates device links between @dev (consumer) and all the
> + * supplier devices of the entire fwnode tree at @fwnode.
> + *
> + * The function creates normal (non-SYNC_STATE_ONLY) device links between @dev
> + * and the real suppliers of @dev. Once these device links are created, the
> + * fwnode links are deleted. When such device links are successfully created,
> + * this function is called recursively on those supplier devices. This is
> + * needed to detect and break some invalid cycles in fwnode links.  See
> + * fw_devlink_create_devlink() for more details.
> + *
> + * In addition, it also looks at all the suppliers of the entire fwnode tree
> + * because some of the child devices of @dev that have not been added yet
> + * (because @dev hasn't probed) might already have their suppliers added to
> + * driver core. So, this function creates SYNC_STATE_ONLY device links between
> + * @dev (consumer) and these suppliers to make sure they don't execute their
> + * sync_state() callbacks before these child devices have a chance to create
> + * their device links. The fwnode links that correspond to the child devices
> + * aren't delete because they are needed later to create the device links
> + * between the real consumer and supplier devices.
> + */
> +static void __fw_devlink_link_to_suppliers(struct device *dev,
> +					   struct fwnode_handle *fwnode)
>  {
> -	int fw_ret;
> +	bool own_link = (dev->fwnode == fwnode);
> +	struct fwnode_link *link, *tmp;
> +	struct fwnode_handle *child = NULL;
> +	u32 dl_flags;
> +
> +	if (own_link)
> +		dl_flags = fw_devlink_get_flags();
> +	else
> +		dl_flags = DL_FLAG_SYNC_STATE_ONLY;
>  
> -	device_link_add_missing_supplier_links();
> +	list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) {
> +		int ret;
> +		struct device *sup_dev;
> +		struct fwnode_handle *sup = link->supplier;
> +
> +		ret = fw_devlink_create_devlink(dev, sup, dl_flags);
> +		if (!own_link || ret == -EAGAIN)
> +			continue;
>  
> -	if (fw_devlink_flags && fwnode_has_op(dev->fwnode, add_links)) {
> -		fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
> -		if (fw_ret == -ENODEV && !fw_devlink_is_permissive())
> -			device_link_wait_for_mandatory_supplier(dev);
> -		else if (fw_ret)
> -			device_link_wait_for_optional_supplier(dev);
> +		list_del(&link->s_hook);
> +		list_del(&link->c_hook);
> +		kfree(link);
> +
> +		/* If no device link was created, nothing more to do. */
> +		if (ret)
> +			continue;
> +
> +		/*
> +		 * If a device link was successfully created to a supplier, we
> +		 * now need to try and link the supplier to all its suppliers.
> +		 *
> +		 * This is needed to detect and delete false dependencies in
> +		 * fwnode links that haven't been converted to a device link
> +		 * yet. See comments in fw_devlink_create_devlink() for more
> +		 * details on the false dependency.
> +		 *
> +		 * Without deleting these false dependencies, some devices will
> +		 * never probe because they'll keep waiting for their false
> +		 * dependency fwnode links to be converted to device links.
> +		 */
> +		sup_dev = get_dev_from_fwnode(sup);
> +		__fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode);
> +		put_device(sup_dev);
>  	}
> +
> +	/*
> +	 * Make "proxy" SYNC_STATE_ONLY device links to represent the needs of
> +	 * all the descendants. This proxy link step is needed to handle the
> +	 * case where the supplier is added before the consumer's parent device
> +	 * (@dev).
> +	 */
> +	while ((child = fwnode_get_next_available_child_node(fwnode, child)))
> +		__fw_devlink_link_to_suppliers(dev, child);
> +}
> +
> +static void fw_devlink_link_device(struct device *dev)
> +{
> +	struct fwnode_handle *fwnode = dev->fwnode;
> +
> +	if (!fw_devlink_flags)
> +		return;
> +
> +	fw_devlink_parse_fwtree(fwnode);
> +
> +	mutex_lock(&fwnode_link_lock);
> +	__fw_devlink_link_to_consumers(dev);
> +	__fw_devlink_link_to_suppliers(dev, fwnode);
> +	mutex_unlock(&fwnode_link_lock);
>  }
>  
>  /* Device links support end. */
> @@ -2431,7 +2583,6 @@ 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);
>  	INIT_LIST_HEAD(&dev->links.defer_sync);
>  	dev->links.status = DL_DEV_NO_DRIVER;
>  }
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1e771ea4dca6..89bb8b84173e 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -351,18 +351,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.
>   * @defer_sync: Hook to global list of devices that have deferred sync_state.
> - * @need_for_probe: If needs_suppliers is on a list, this indicates if the
> - *		    suppliers are needed for probe or not.
>   * @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;
> -	bool need_for_probe;
>  	enum dl_dev_state status;
>  };
>  

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Robin Murphy]

On 2020-12-11 14:11, Qian Cai wrote:
> On Fri, 2020-11-20 at 18:02 -0800, Saravana Kannan wrote:
>> The current implementation of fw_devlink is very inefficient because it
>> tries to get away without creating fwnode links in the name of saving
>> memory usage. Past attempts to optimize runtime at the cost of memory
>> usage were blocked with request for data showing that the optimization
>> made significant improvement for real world scenarios.
>>
>> We have those scenarios now. There have been several reports of boot
>> time increase in the order of seconds in this thread [1]. Several OEMs
>> and SoC manufacturers have also privately reported significant
>> (350-400ms) increase in boot time due to all the parsing done by
>> fw_devlink.
>>
>> So this patch uses all the setup done by the previous patches in this
>> series to refactor fw_devlink to be more efficient. Most of the code has
>> been moved out of firmware specific (DT mostly) code into driver core.
>>
>> This brings the following benefits:
>> - Instead of parsing the device tree multiple times during bootup,
>>    fw_devlink parses each fwnode node/property only once and creates
>>    fwnode links. The rest of the fw_devlink code then just looks at these
>>    fwnode links to do rest of the work.
>>
>> - Makes it much easier to debug probe issue due to fw_devlink in the
>>    future. fw_devlink=on blocks the probing of devices if they depend on
>>    a device that hasn't been added yet. With this refactor, it'll be very
>>    easy to tell what that device is because we now have a reference to
>>    the fwnode of the device.
>>
>> - Much easier to add fw_devlink support to ACPI and other firmware
>>    types. A refactor to move the common bits from DT specific code to
>>    driver core was in my TODO list as a prerequisite to adding ACPI
>>    support to fw_devlink. This series gets that done.
>>
>> [1] - https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/
>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> Reverting this commit and its dependency:
> 
> 2d09e6eb4a6f driver core: Delete pointless parameter in fwnode_operations.add_links
> 
> from today's linux-next fixed a boot crash on an arm64 Thunder X2 server.

Since the call stack implicates the platform-device-wrangling we do in 
IORT code I took a quick look; AFAICS my guess would be it's blowing up 
trying to walk a zeroed list head since "driver core: Add fwnode_init()" 
missed acpi_alloc_fwnode_static().

Robin.

> .config: https://cailca.coding.net/public/linux/mm/git/files/master/arm64.config
> 
> [   57.413929][    T1] ACPI: 5 ACPI AML tables successfully acquired and loaded
> [   60.571643][    T1] ACPI: Interpreter enabled
> [   60.576104][    T1] ACPI: Using GIC for interrupt routing
> [   60.582474][    T1] ACPI: MCFG table detected, 1 entries
> [   60.588051][    T1] ACPI: IORT: SMMU-v3[402300000] Mapped to Proximity domain 0
> [   60.601374][    T1] Unable to handle kernel paging request at virtual address dfff800000000000
> [   60.610146][    T1] Mem abort info:
> [   60.613694][    T1]   ESR = 0x96000004
> [   60.617496][    T1]   EC = 0x25: DABT (current EL), IL = 32 bits
> [   60.623616][    T1]   SET = 0, FnV = 0
> [   60.627420][    T1]   EA = 0, S1PTW = 0
> [   60.631304][    T1] Data abort info:
> [   60.634957][    T1]   ISV = 0, ISS = 0x00000004
> [   60.639546][    T1]   CM = 0, WnR = 0
> [   60.643255][    T1] [dfff800000000000] address between user and kernel address ranges
> [   60.651226][    T1] Internal error: Oops: 96000004 [#1] SMP
> [   60.656864][    T1] Modules linked in:
> [   60.660658][    T1] CPU: 38 PID: 1 Comm: swapper/0 Tainted: G        W         5.10.0-rc7-next-20201211 #2
> [   60.670424][    T1] Hardware name: HPE Apollo 70             /C01_APACHE_MB         , BIOS L50_5.13_1.16 07/29/2020
> [   60.680979][    T1] pstate: 10400009 (nzcV daif +PAN -UAO -TCO BTYPE=--)
> [   60.687757][    T1] pc : device_add+0xf60/0x16b0
> [   60.692430][    T1] lr : device_add+0xf08/0x16b0
> [   60.697098][    T1] sp : ffff0000063bf7d0
> [   60.701147][    T1] x29: ffff0000063bf7d0 x28: ffff00001f760810
> [   60.707226][    T1] x27: fffffffffffffff8 x26: ffff00001f760858
> [   60.713304][    T1] x25: ffff00001f760c58 x24: fffffffffffffff8
> [   60.719381][    T1] x23: 1fffe00003eec10b x22: ffff8000190d0260
> [   60.725458][    T1] x21: ffff800011dba708 x20: 0000000000000000
> [   60.731535][    T1] x19: 1fffe00000c77f10 x18: 1fffe001cf0d53ed
> [   60.737616][    T1] x17: 0000000000000000 x16: 1ffff000033676f1
> [   60.743709][    T1] x15: 0000000000000000 x14: ffff800011731e34
> [   60.749786][    T1] x13: ffff700003217fb9 x12: 1ffff00003217fb8
> [   60.755864][    T1] x11: 1ffff00003217fb8 x10: ffff700003217fb8
> [   60.761940][    T1] x9 : dfff800000000000 x8 : ffff8000190bfdc7
> [   60.768017][    T1] x7 : 0000000000000001 x6 : ffff700003217fb9
> [   60.774094][    T1] x5 : ffff700003217fb9 x4 : ffff000006324a80
> [   60.780170][    T1] x3 : 1fffe00000c64951 x2 : 0000000000000000
> [   60.786247][    T1] x1 : 0000000000000000 x0 : dfff800000000000
> [   60.792324][    T1] Call trace:
> [   60.795495][    T1]  device_add+0xf60/0x16b0
> __fw_devlink_link_to_consumers at drivers/base/core.c:1583
> (inlined by) fw_devlink_link_device at drivers/base/core.c:1726
> (inlined by) device_add at drivers/base/core.c:3088
> [   60.799813][    T1]  platform_device_add+0x274/0x628
> [   60.804833][    T1]  acpi_iort_init+0x9d8/0xc50
> [   60.809415][    T1]  acpi_init+0x45c/0x4e8
> [   60.813556][    T1]  do_one_initcall+0x170/0xb70
> [   60.818224][    T1]  kernel_init_freeable+0x6a8/0x734
> [   60.823332][    T1]  kernel_init+0x18/0x12c
> [   60.827566][    T1]  ret_from_fork+0x10/0x1c
> [   60.831890][    T1] Code: f2fbffe0 d100205b d343fc41 aa1b03f8 (38e06820)
> [   60.838756][    T1] ---[ end trace fa5c8ce17a226d83 ]---
> [   60.844127][    T1] Kernel panic - not syncing: Oops: Fatal exception
> [   60.850881][    T1] SMP: stopping secondary CPUs
> [   60.855733][    T1] ---[ end Kernel panic - not syncing: Oops: Fatal exception ]---
>> ---
>>   drivers/base/core.c    | 325 ++++++++++++++++++++++++++++++-----------
>>   include/linux/device.h |   5 -
>>   2 files changed, 238 insertions(+), 92 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 1873cecb0cc4..9edf9084fc98 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -46,8 +46,6 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
>>   #endif
>>   
>>   /* Device links support. */
>> -static LIST_HEAD(wait_for_suppliers);
>> -static DEFINE_MUTEX(wfs_lock);
>>   static LIST_HEAD(deferred_sync);
>>   static unsigned int defer_sync_state_count = 1;
>>   static DEFINE_MUTEX(fwnode_link_lock);
>> @@ -803,74 +801,6 @@ struct device_link *device_link_add(struct device *consumer,
>>   }
>>   EXPORT_SYMBOL_GPL(device_link_add);
>>   
>> -/**
>> - * device_link_wait_for_supplier - Add device to wait_for_suppliers list
>> - * @consumer: Consumer device
>> - *
>> - * Marks the @consumer device as waiting for suppliers to become available by
>> - * adding it to the wait_for_suppliers list. The consumer device will never be
>> - * probed until it's removed from the wait_for_suppliers list.
>> - *
>> - * The caller is responsible for adding the links to the supplier devices once
>> - * they are available and removing the @consumer device from the
>> - * wait_for_suppliers list once links to all the suppliers have been created.
>> - *
>> - * 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,
>> -					  bool need_for_probe)
>> -{
>> -	mutex_lock(&wfs_lock);
>> -	list_add_tail(&consumer->links.needs_suppliers, &wait_for_suppliers);
>> -	consumer->links.need_for_probe = need_for_probe;
>> -	mutex_unlock(&wfs_lock);
>> -}
>> -
>> -static void device_link_wait_for_mandatory_supplier(struct device *consumer)
>> -{
>> -	device_link_wait_for_supplier(consumer, true);
>> -}
>> -
>> -static void device_link_wait_for_optional_supplier(struct device *consumer)
>> -{
>> -	device_link_wait_for_supplier(consumer, false);
>> -}
>> -
>> -/**
>> - * device_link_add_missing_supplier_links - Add links from consumer devices to
>> - *					    supplier devices, leaving any
>> - *					    consumer with inactive suppliers on
>> - *					    the wait_for_suppliers list
>> - *
>> - * Loops through all consumers waiting on suppliers and tries to add all their
>> - * supplier links. If that succeeds, the consumer device is removed from
>> - * wait_for_suppliers list. Otherwise, they are left in the wait_for_suppliers
>> - * list.  Devices left on the wait_for_suppliers list will not be probed.
>> - *
>> - * The fwnode add_links 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_add_missing_supplier_links(void)
>> -{
>> -	struct device *dev, *tmp;
>> -
>> -	mutex_lock(&wfs_lock);
>> -	list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
>> -				 links.needs_suppliers) {
>> -		int ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
>> -		if (!ret)
>> -			list_del_init(&dev->links.needs_suppliers);
>> -		else if (ret != -ENODEV)
>> -			dev->links.need_for_probe = false;
>> -	}
>> -	mutex_unlock(&wfs_lock);
>> -}
>> -
>>   #ifdef CONFIG_SRCU
>>   static void __device_link_del(struct kref *kref)
>>   {
>> @@ -1195,9 +1125,8 @@ void device_links_driver_bound(struct device *dev)
>>   	 * the device links it needs to or make new device links as it needs
>>   	 * them. So, it no longer needs to wait on any suppliers.
>>   	 */
>> -	mutex_lock(&wfs_lock);
>> -	list_del_init(&dev->links.needs_suppliers);
>> -	mutex_unlock(&wfs_lock);
>> +	if (dev->fwnode && dev->fwnode->dev == dev)
>> +		fwnode_links_purge_suppliers(dev->fwnode);
>>   	device_remove_file(dev, &dev_attr_waiting_for_supplier);
>>   
>>   	device_links_write_lock();
>> @@ -1488,10 +1417,6 @@ static void device_links_purge(struct device *dev)
>>   	if (dev->class == &devlink_class)
>>   		return;
>>   
>> -	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).
>> @@ -1561,19 +1486,246 @@ static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode)
>>   		fw_devlink_parse_fwtree(child);
>>   }
>>   
>> -static void fw_devlink_link_device(struct device *dev)
>> +/**
>> + * fw_devlink_create_devlink - Create a device link from a consumer to fwnode
>> + * @con - Consumer device for the device link
>> + * @sup_handle - fwnode handle of supplier
>> + *
>> + * This function will try to create a device link between the consumer device
>> + * @con and the supplier device represented by @sup_handle.
>> + *
>> + * The supplier has to be provided as a fwnode because incorrect cycles in
>> + * fwnode links can sometimes cause the supplier device to never be created.
>> + * This function detects such cases and returns an error if it cannot create a
>> + * device link from the consumer to a missing supplier.
>> + *
>> + * Returns,
>> + * 0 on successfully creating a device link
>> + * -EINVAL if the device link cannot be created as expected
>> + * -EAGAIN if the device link cannot be created right now, but it may be
>> + *  possible to do that in the future
>> + */
>> +static int fw_devlink_create_devlink(struct device *con,
>> +				     struct fwnode_handle *sup_handle, u32 flags)
>> +{
>> +	struct device *sup_dev;
>> +	int ret = 0;
>> +
>> +	sup_dev = get_dev_from_fwnode(sup_handle);
>> +	if (sup_dev) {
>> +		/*
>> +		 * If this fails, it is due to cycles in device links.  Just
>> +		 * give up on this link and treat it as invalid.
>> +		 */
>> +		if (!device_link_add(con, sup_dev, flags))
>> +			ret = -EINVAL;
>> +
>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * DL_FLAG_SYNC_STATE_ONLY doesn't block probing and supports
>> +	 * cycles. So cycle detection isn't necessary and shouldn't be
>> +	 * done.
>> +	 */
>> +	if (flags & DL_FLAG_SYNC_STATE_ONLY)
>> +		return -EAGAIN;
>> +
>> +	/*
>> +	 * If we can't find the supplier device from its fwnode, it might be
>> +	 * due to a cyclic dependency between fwnodes. Some of these cycles can
>> +	 * be broken by applying logic. Check for these types of cycles and
>> +	 * break them so that devices in the cycle probe properly.
>> +	 *
>> +	 * If the supplier's parent is dependent on the consumer, then
>> +	 * the consumer-supplier dependency is a false dependency. So,
>> +	 * treat it as an invalid link.
>> +	 */
>> +	sup_dev = fwnode_get_next_parent_dev(sup_handle);
>> +	if (sup_dev && device_is_dependent(con, sup_dev)) {
>> +		dev_dbg(con, "Not linking to %pfwP - False link\n",
>> +			sup_handle);
>> +		ret = -EINVAL;
>> +	} else {
>> +		/*
>> +		 * Can't check for cycles or no cycles. So let's try
>> +		 * again later.
>> +		 */
>> +		ret = -EAGAIN;
>> +	}
>> +
>> +out:
>> +	put_device(sup_dev);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * __fw_devlink_link_to_consumers - Create device links to consumers of a device
>> + * @dev - Device that needs to be linked to its consumers
>> + *
>> + * This function looks at all the consumer fwnodes of @dev and creates device
>> + * links between the consumer device and @dev (supplier).
>> + *
>> + * If the consumer device has not been added yet, then this function creates a
>> + * SYNC_STATE_ONLY link between @dev (supplier) and the closest ancestor device
>> + * of the consumer fwnode. This is necessary to make sure @dev doesn't get a
>> + * sync_state() callback before the real consumer device gets to be added and
>> + * then probed.
>> + *
>> + * Once device links are created from the real consumer to @dev (supplier), the
>> + * fwnode links are deleted.
>> + */
>> +static void __fw_devlink_link_to_consumers(struct device *dev)
>> +{
>> +	struct fwnode_handle *fwnode = dev->fwnode;
>> +	struct fwnode_link *link, *tmp;
>> +
>> +	list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) {
>> +		u32 dl_flags = fw_devlink_get_flags();
>> +		struct device *con_dev;
>> +		bool own_link = true;
>> +		int ret;
>> +
>> +		con_dev = get_dev_from_fwnode(link->consumer);
>> +		/*
>> +		 * If consumer device is not available yet, make a "proxy"
>> +		 * SYNC_STATE_ONLY link from the consumer's parent device to
>> +		 * the supplier device. This is necessary to make sure the
>> +		 * supplier doesn't get a sync_state() callback before the real
>> +		 * consumer can create a device link to the supplier.
>> +		 *
>> +		 * This proxy link step is needed to handle the case where the
>> +		 * consumer's parent device is added before the supplier.
>> +		 */
>> +		if (!con_dev) {
>> +			con_dev = fwnode_get_next_parent_dev(link->consumer);
>> +			/*
>> +			 * However, if the consumer's parent device is also the
>> +			 * parent of the supplier, don't create a
>> +			 * consumer-supplier link from the parent to its child
>> +			 * device. Such a dependency is impossible.
>> +			 */
>> +			if (con_dev &&
>> +			    fwnode_is_ancestor_of(con_dev->fwnode, fwnode)) {
>> +				put_device(con_dev);
>> +				con_dev = NULL;
>> +			} else {
>> +				own_link = false;
>> +				dl_flags = DL_FLAG_SYNC_STATE_ONLY;
>> +			}
>> +		}
>> +
>> +		if (!con_dev)
>> +			continue;
>> +
>> +		ret = fw_devlink_create_devlink(con_dev, fwnode, dl_flags);
>> +		put_device(con_dev);
>> +		if (!own_link || ret == -EAGAIN)
>> +			continue;
>> +
>> +		list_del(&link->s_hook);
>> +		list_del(&link->c_hook);
>> +		kfree(link);
>> +	}
>> +}
>> +
>> +/**
>> + * __fw_devlink_link_to_suppliers - Create device links to suppliers of a device
>> + * @dev - The consumer device that needs to be linked to its suppliers
>> + * @fwnode - Root of the fwnode tree that is used to create device links
>> + *
>> + * This function looks at all the supplier fwnodes of fwnode tree rooted at
>> + * @fwnode and creates device links between @dev (consumer) and all the
>> + * supplier devices of the entire fwnode tree at @fwnode.
>> + *
>> + * The function creates normal (non-SYNC_STATE_ONLY) device links between @dev
>> + * and the real suppliers of @dev. Once these device links are created, the
>> + * fwnode links are deleted. When such device links are successfully created,
>> + * this function is called recursively on those supplier devices. This is
>> + * needed to detect and break some invalid cycles in fwnode links.  See
>> + * fw_devlink_create_devlink() for more details.
>> + *
>> + * In addition, it also looks at all the suppliers of the entire fwnode tree
>> + * because some of the child devices of @dev that have not been added yet
>> + * (because @dev hasn't probed) might already have their suppliers added to
>> + * driver core. So, this function creates SYNC_STATE_ONLY device links between
>> + * @dev (consumer) and these suppliers to make sure they don't execute their
>> + * sync_state() callbacks before these child devices have a chance to create
>> + * their device links. The fwnode links that correspond to the child devices
>> + * aren't delete because they are needed later to create the device links
>> + * between the real consumer and supplier devices.
>> + */
>> +static void __fw_devlink_link_to_suppliers(struct device *dev,
>> +					   struct fwnode_handle *fwnode)
>>   {
>> -	int fw_ret;
>> +	bool own_link = (dev->fwnode == fwnode);
>> +	struct fwnode_link *link, *tmp;
>> +	struct fwnode_handle *child = NULL;
>> +	u32 dl_flags;
>> +
>> +	if (own_link)
>> +		dl_flags = fw_devlink_get_flags();
>> +	else
>> +		dl_flags = DL_FLAG_SYNC_STATE_ONLY;
>>   
>> -	device_link_add_missing_supplier_links();
>> +	list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) {
>> +		int ret;
>> +		struct device *sup_dev;
>> +		struct fwnode_handle *sup = link->supplier;
>> +
>> +		ret = fw_devlink_create_devlink(dev, sup, dl_flags);
>> +		if (!own_link || ret == -EAGAIN)
>> +			continue;
>>   
>> -	if (fw_devlink_flags && fwnode_has_op(dev->fwnode, add_links)) {
>> -		fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
>> -		if (fw_ret == -ENODEV && !fw_devlink_is_permissive())
>> -			device_link_wait_for_mandatory_supplier(dev);
>> -		else if (fw_ret)
>> -			device_link_wait_for_optional_supplier(dev);
>> +		list_del(&link->s_hook);
>> +		list_del(&link->c_hook);
>> +		kfree(link);
>> +
>> +		/* If no device link was created, nothing more to do. */
>> +		if (ret)
>> +			continue;
>> +
>> +		/*
>> +		 * If a device link was successfully created to a supplier, we
>> +		 * now need to try and link the supplier to all its suppliers.
>> +		 *
>> +		 * This is needed to detect and delete false dependencies in
>> +		 * fwnode links that haven't been converted to a device link
>> +		 * yet. See comments in fw_devlink_create_devlink() for more
>> +		 * details on the false dependency.
>> +		 *
>> +		 * Without deleting these false dependencies, some devices will
>> +		 * never probe because they'll keep waiting for their false
>> +		 * dependency fwnode links to be converted to device links.
>> +		 */
>> +		sup_dev = get_dev_from_fwnode(sup);
>> +		__fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode);
>> +		put_device(sup_dev);
>>   	}
>> +
>> +	/*
>> +	 * Make "proxy" SYNC_STATE_ONLY device links to represent the needs of
>> +	 * all the descendants. This proxy link step is needed to handle the
>> +	 * case where the supplier is added before the consumer's parent device
>> +	 * (@dev).
>> +	 */
>> +	while ((child = fwnode_get_next_available_child_node(fwnode, child)))
>> +		__fw_devlink_link_to_suppliers(dev, child);
>> +}
>> +
>> +static void fw_devlink_link_device(struct device *dev)
>> +{
>> +	struct fwnode_handle *fwnode = dev->fwnode;
>> +
>> +	if (!fw_devlink_flags)
>> +		return;
>> +
>> +	fw_devlink_parse_fwtree(fwnode);
>> +
>> +	mutex_lock(&fwnode_link_lock);
>> +	__fw_devlink_link_to_consumers(dev);
>> +	__fw_devlink_link_to_suppliers(dev, fwnode);
>> +	mutex_unlock(&fwnode_link_lock);
>>   }
>>   
>>   /* Device links support end. */
>> @@ -2431,7 +2583,6 @@ 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);
>>   	INIT_LIST_HEAD(&dev->links.defer_sync);
>>   	dev->links.status = DL_DEV_NO_DRIVER;
>>   }
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 1e771ea4dca6..89bb8b84173e 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -351,18 +351,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.
>>    * @defer_sync: Hook to global list of devices that have deferred sync_state.
>> - * @need_for_probe: If needs_suppliers is on a list, this indicates if the
>> - *		    suppliers are needed for probe or not.
>>    * @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;
>> -	bool need_for_probe;
>>   	enum dl_dev_state status;
>>   };
>>   
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Saravana Kannan]

On Fri, Dec 11, 2020 at 8:34 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-12-11 14:11, Qian Cai wrote:
> > On Fri, 2020-11-20 at 18:02 -0800, Saravana Kannan wrote:
> >> The current implementation of fw_devlink is very inefficient because it
> >> tries to get away without creating fwnode links in the name of saving
> >> memory usage. Past attempts to optimize runtime at the cost of memory
> >> usage were blocked with request for data showing that the optimization
> >> made significant improvement for real world scenarios.
> >>
> >> We have those scenarios now. There have been several reports of boot
> >> time increase in the order of seconds in this thread [1]. Several OEMs
> >> and SoC manufacturers have also privately reported significant
> >> (350-400ms) increase in boot time due to all the parsing done by
> >> fw_devlink.
> >>
> >> So this patch uses all the setup done by the previous patches in this
> >> series to refactor fw_devlink to be more efficient. Most of the code has
> >> been moved out of firmware specific (DT mostly) code into driver core.
> >>
> >> This brings the following benefits:
> >> - Instead of parsing the device tree multiple times during bootup,
> >>    fw_devlink parses each fwnode node/property only once and creates
> >>    fwnode links. The rest of the fw_devlink code then just looks at these
> >>    fwnode links to do rest of the work.
> >>
> >> - Makes it much easier to debug probe issue due to fw_devlink in the
> >>    future. fw_devlink=on blocks the probing of devices if they depend on
> >>    a device that hasn't been added yet. With this refactor, it'll be very
> >>    easy to tell what that device is because we now have a reference to
> >>    the fwnode of the device.
> >>
> >> - Much easier to add fw_devlink support to ACPI and other firmware
> >>    types. A refactor to move the common bits from DT specific code to
> >>    driver core was in my TODO list as a prerequisite to adding ACPI
> >>    support to fw_devlink. This series gets that done.
> >>
> >> [1] - https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/
> >> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >
> > Reverting this commit and its dependency:
> >
> > 2d09e6eb4a6f driver core: Delete pointless parameter in fwnode_operations.add_links
> >
> > from today's linux-next fixed a boot crash on an arm64 Thunder X2 server.
>
> Since the call stack implicates the platform-device-wrangling we do in
> IORT code I took a quick look; AFAICS my guess would be it's blowing up
> trying to walk a zeroed list head since "driver core: Add fwnode_init()"
> missed acpi_alloc_fwnode_static().

Thanks Robin. I'm pretty sure this is the reason. I thought I fixed
all ACPI cases, but clearly I missed this one. I'll send out a patch
for this today. If you think there are any other places I missed
please let me know. I'll try some git grep foo to see if I missed any
other instances of fwnode ops being set.

-Saravana

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Marc Zyngier]

On 2020-12-11 17:51, Saravana Kannan wrote:
> On Fri, Dec 11, 2020 at 8:34 AM Robin Murphy <robin.murphy@arm.com> 
> wrote:
>> 
>> On 2020-12-11 14:11, Qian Cai wrote:
>> > On Fri, 2020-11-20 at 18:02 -0800, Saravana Kannan wrote:
>> >> The current implementation of fw_devlink is very inefficient because it
>> >> tries to get away without creating fwnode links in the name of saving
>> >> memory usage. Past attempts to optimize runtime at the cost of memory
>> >> usage were blocked with request for data showing that the optimization
>> >> made significant improvement for real world scenarios.
>> >>
>> >> We have those scenarios now. There have been several reports of boot
>> >> time increase in the order of seconds in this thread [1]. Several OEMs
>> >> and SoC manufacturers have also privately reported significant
>> >> (350-400ms) increase in boot time due to all the parsing done by
>> >> fw_devlink.
>> >>
>> >> So this patch uses all the setup done by the previous patches in this
>> >> series to refactor fw_devlink to be more efficient. Most of the code has
>> >> been moved out of firmware specific (DT mostly) code into driver core.
>> >>
>> >> This brings the following benefits:
>> >> - Instead of parsing the device tree multiple times during bootup,
>> >>    fw_devlink parses each fwnode node/property only once and creates
>> >>    fwnode links. The rest of the fw_devlink code then just looks at these
>> >>    fwnode links to do rest of the work.
>> >>
>> >> - Makes it much easier to debug probe issue due to fw_devlink in the
>> >>    future. fw_devlink=on blocks the probing of devices if they depend on
>> >>    a device that hasn't been added yet. With this refactor, it'll be very
>> >>    easy to tell what that device is because we now have a reference to
>> >>    the fwnode of the device.
>> >>
>> >> - Much easier to add fw_devlink support to ACPI and other firmware
>> >>    types. A refactor to move the common bits from DT specific code to
>> >>    driver core was in my TODO list as a prerequisite to adding ACPI
>> >>    support to fw_devlink. This series gets that done.
>> >>
>> >> [1] - https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/
>> >> Signed-off-by: Saravana Kannan <saravanak@google.com>
>> >> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> >> Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> >
>> > Reverting this commit and its dependency:
>> >
>> > 2d09e6eb4a6f driver core: Delete pointless parameter in fwnode_operations.add_links
>> >
>> > from today's linux-next fixed a boot crash on an arm64 Thunder X2 server.
>> 
>> Since the call stack implicates the platform-device-wrangling we do in
>> IORT code I took a quick look; AFAICS my guess would be it's blowing 
>> up
>> trying to walk a zeroed list head since "driver core: Add 
>> fwnode_init()"
>> missed acpi_alloc_fwnode_static().
> 
> Thanks Robin. I'm pretty sure this is the reason. I thought I fixed
> all ACPI cases, but clearly I missed this one. I'll send out a patch
> for this today. If you think there are any other places I missed
> please let me know. I'll try some git grep foo to see if I missed any
> other instances of fwnode ops being set.

Yup, that fixed it here (QDF2400).

Thanks,

         M.

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 39263c6b52e1..2630c2e953f7 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -55,7 +55,7 @@ static inline struct fwnode_handle 
*acpi_alloc_fwnode_static(void)
  	if (!fwnode)
  		return NULL;

-	fwnode->ops = &acpi_static_fwnode_ops;
+	fwnode_init(fwnode, &acpi_static_fwnode_ops);

  	return fwnode;
  }



-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Saravana Kannan]

On Fri, Dec 11, 2020 at 10:03 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-12-11 17:51, Saravana Kannan wrote:
> > On Fri, Dec 11, 2020 at 8:34 AM Robin Murphy <robin.murphy@arm.com>
> > wrote:
> >>
> >> On 2020-12-11 14:11, Qian Cai wrote:
> >> > On Fri, 2020-11-20 at 18:02 -0800, Saravana Kannan wrote:
> >> >> The current implementation of fw_devlink is very inefficient because it
> >> >> tries to get away without creating fwnode links in the name of saving
> >> >> memory usage. Past attempts to optimize runtime at the cost of memory
> >> >> usage were blocked with request for data showing that the optimization
> >> >> made significant improvement for real world scenarios.
> >> >>
> >> >> We have those scenarios now. There have been several reports of boot
> >> >> time increase in the order of seconds in this thread [1]. Several OEMs
> >> >> and SoC manufacturers have also privately reported significant
> >> >> (350-400ms) increase in boot time due to all the parsing done by
> >> >> fw_devlink.
> >> >>
> >> >> So this patch uses all the setup done by the previous patches in this
> >> >> series to refactor fw_devlink to be more efficient. Most of the code has
> >> >> been moved out of firmware specific (DT mostly) code into driver core.
> >> >>
> >> >> This brings the following benefits:
> >> >> - Instead of parsing the device tree multiple times during bootup,
> >> >>    fw_devlink parses each fwnode node/property only once and creates
> >> >>    fwnode links. The rest of the fw_devlink code then just looks at these
> >> >>    fwnode links to do rest of the work.
> >> >>
> >> >> - Makes it much easier to debug probe issue due to fw_devlink in the
> >> >>    future. fw_devlink=on blocks the probing of devices if they depend on
> >> >>    a device that hasn't been added yet. With this refactor, it'll be very
> >> >>    easy to tell what that device is because we now have a reference to
> >> >>    the fwnode of the device.
> >> >>
> >> >> - Much easier to add fw_devlink support to ACPI and other firmware
> >> >>    types. A refactor to move the common bits from DT specific code to
> >> >>    driver core was in my TODO list as a prerequisite to adding ACPI
> >> >>    support to fw_devlink. This series gets that done.
> >> >>
> >> >> [1] - https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/
> >> >> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >> >> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> >> Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >> >
> >> > Reverting this commit and its dependency:
> >> >
> >> > 2d09e6eb4a6f driver core: Delete pointless parameter in fwnode_operations.add_links
> >> >
> >> > from today's linux-next fixed a boot crash on an arm64 Thunder X2 server.
> >>
> >> Since the call stack implicates the platform-device-wrangling we do in
> >> IORT code I took a quick look; AFAICS my guess would be it's blowing
> >> up
> >> trying to walk a zeroed list head since "driver core: Add
> >> fwnode_init()"
> >> missed acpi_alloc_fwnode_static().
> >
> > Thanks Robin. I'm pretty sure this is the reason. I thought I fixed
> > all ACPI cases, but clearly I missed this one. I'll send out a patch
> > for this today. If you think there are any other places I missed
> > please let me know. I'll try some git grep foo to see if I missed any
> > other instances of fwnode ops being set.
>
> Yup, that fixed it here (QDF2400).
>
> Thanks,
>
>          M.
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 39263c6b52e1..2630c2e953f7 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -55,7 +55,7 @@ static inline struct fwnode_handle
> *acpi_alloc_fwnode_static(void)
>         if (!fwnode)
>                 return NULL;
>
> -       fwnode->ops = &acpi_static_fwnode_ops;
> +       fwnode_init(fwnode, &acpi_static_fwnode_ops);
>
>         return fwnode;
>   }
>

Lol, my only contribution to the patch will be the commit text. I'll
send them with reported-by, suggested-by and tested-by if no one less
beats me to it.

-Saravana

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Marc Zyngier]

On 2020-12-11 18:20, Saravana Kannan wrote:

> Lol, my only contribution to the patch will be the commit text. I'll
> send them with reported-by, suggested-by and tested-by if no one less
> beats me to it.

Teamwork!

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Saravana Kannan]

On Fri, Dec 11, 2020 at 11:07 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-12-11 18:20, Saravana Kannan wrote:
>
> > Lol, my only contribution to the patch will be the commit text. I'll
> > send them with reported-by, suggested-by and tested-by if no one less
> > beats me to it.
>
> Teamwork!
>
>          M.

Forgot to CC most of the folks/lists here, but patch has been sent.

https://lore.kernel.org/lkml/20201211202629.2164655-1-saravanak@google.com/

-Saravana

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Michael Walle]

> The current implementation of fw_devlink is very inefficient because it
> tries to get away without creating fwnode links in the name of saving
> memory usage. Past attempts to optimize runtime at the cost of memory
> usage were blocked with request for data showing that the optimization
> made significant improvement for real world scenarios.
> 
> We have those scenarios now. There have been several reports of boot
> time increase in the order of seconds in this thread [1]. Several OEMs
> and SoC manufacturers have also privately reported significant
> (350-400ms) increase in boot time due to all the parsing done by
> fw_devlink.
> 
> So this patch uses all the setup done by the previous patches in this
> series to refactor fw_devlink to be more efficient. Most of the code has
> been moved out of firmware specific (DT mostly) code into driver core.
> 
> This brings the following benefits:
> - Instead of parsing the device tree multiple times during bootup,
>   fw_devlink parses each fwnode node/property only once and creates
>   fwnode links. The rest of the fw_devlink code then just looks at these
>   fwnode links to do rest of the work.
> 
> - Makes it much easier to debug probe issue due to fw_devlink in the
>   future. fw_devlink=on blocks the probing of devices if they depend on
>   a device that hasn't been added yet. With this refactor, it'll be very
>   easy to tell what that device is because we now have a reference to
>   the fwnode of the device.
> 
> - Much easier to add fw_devlink support to ACPI and other firmware
>   types. A refactor to move the common bits from DT specific code to
>   driver core was in my TODO list as a prerequisite to adding ACPI
>   support to fw_devlink. This series gets that done.
> 
> [1] - https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>

git bisect show that this commit broke my board in 5.11-rc1:

[    2.294375] sysfs: cannot create duplicate filename '/devices/virtual/devlink/0000:00:00.1--0000:00:00.1'
[    2.303999] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.11.0-rc1-00016-ga0fb284b267 #267
[    2.312125] Hardware name: Kontron SMARC-sAL28 (4 Lane) (DT)
[    2.317804] Call trace:
[    2.320253]  dump_backtrace+0x0/0x1b8
[    2.323936]  show_stack+0x20/0x70
[    2.327263]  dump_stack+0xd8/0x134
[    2.330677]  sysfs_warn_dup+0x6c/0x88
[    2.334351]  sysfs_create_dir_ns+0xe8/0x100
[    2.338547]  kobject_add_internal+0x9c/0x290
[    2.342833]  kobject_add+0xa0/0x108
[    2.346331]  device_add+0xfc/0x798
[    2.349746]  device_link_add+0x454/0x5e0
[    2.353682]  fw_devlink_create_devlink+0xb8/0xc8
[    2.358316]  __fw_devlink_link_to_suppliers+0x84/0x180
[    2.363474]  __fw_devlink_link_to_suppliers+0x134/0x180
[    2.368718]  device_add+0x778/0x798
[    2.372217]  device_register+0x28/0x38
[    2.375979]  __mdiobus_register+0x94/0x340
[    2.380089]  of_mdiobus_register+0xb4/0x380
[    2.384285]  enetc_pf_probe+0x73c/0xb10
[    2.388132]  local_pci_probe+0x48/0xb8
[    2.391896]  pci_device_probe+0x120/0x1c0
[    2.395920]  really_probe+0xec/0x3c0
[    2.399505]  driver_probe_device+0x60/0xc0
[    2.403614]  device_driver_attach+0x7c/0x88
[    2.407810]  __driver_attach+0x60/0xe8
[    2.411570]  bus_for_each_dev+0x7c/0xd0
[    2.415419]  driver_attach+0x2c/0x38
[    2.419004]  bus_add_driver+0x194/0x1f8
[    2.422851]  driver_register+0x6c/0x128
[    2.426698]  __pci_register_driver+0x4c/0x58
[    2.430983]  enetc_pf_driver_init+0x2c/0x38
[    2.435181]  do_one_initcall+0x54/0x2d8
[    2.439029]  kernel_init_freeable+0x1fc/0x268
[    2.443403]  kernel_init+0x1c/0x120
[    2.446904]  ret_from_fork+0x10/0x30
[    2.450502] kobject_add_internal failed for 0000:00:00.1--0000:00:00.1 with -EEXIST, don't try to register things with the same name in the same directory.

Looks like it will generate that link twice? Let me know if I can help
testing.

See also: https://lavalab.kontron.com/scheduler/job/3894#L831

-michael

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Saravana Kannan]

On Mon, Dec 28, 2020 at 7:35 PM Michael Walle <michael@walle.cc> wrote:
>
> > The current implementation of fw_devlink is very inefficient because it
> > tries to get away without creating fwnode links in the name of saving
> > memory usage. Past attempts to optimize runtime at the cost of memory
> > usage were blocked with request for data showing that the optimization
> > made significant improvement for real world scenarios.
> >
> > We have those scenarios now. There have been several reports of boot
> > time increase in the order of seconds in this thread [1]. Several OEMs
> > and SoC manufacturers have also privately reported significant
> > (350-400ms) increase in boot time due to all the parsing done by
> > fw_devlink.
> >
> > So this patch uses all the setup done by the previous patches in this
> > series to refactor fw_devlink to be more efficient. Most of the code has
> > been moved out of firmware specific (DT mostly) code into driver core.
> >
> > This brings the following benefits:
> > - Instead of parsing the device tree multiple times during bootup,
> >   fw_devlink parses each fwnode node/property only once and creates
> >   fwnode links. The rest of the fw_devlink code then just looks at these
> >   fwnode links to do rest of the work.
> >
> > - Makes it much easier to debug probe issue due to fw_devlink in the
> >   future. fw_devlink=on blocks the probing of devices if they depend on
> >   a device that hasn't been added yet. With this refactor, it'll be very
> >   easy to tell what that device is because we now have a reference to
> >   the fwnode of the device.
> >
> > - Much easier to add fw_devlink support to ACPI and other firmware
> >   types. A refactor to move the common bits from DT specific code to
> >   driver core was in my TODO list as a prerequisite to adding ACPI
> >   support to fw_devlink. This series gets that done.
> >
> > [1] - https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
>
> git bisect show that this commit broke my board in 5.11-rc1:
>
> [    2.294375] sysfs: cannot create duplicate filename '/devices/virtual/devlink/0000:00:00.1--0000:00:00.1'
> [    2.303999] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.11.0-rc1-00016-ga0fb284b267 #267
> [    2.312125] Hardware name: Kontron SMARC-sAL28 (4 Lane) (DT)
> [    2.317804] Call trace:
> [    2.320253]  dump_backtrace+0x0/0x1b8
> [    2.323936]  show_stack+0x20/0x70
> [    2.327263]  dump_stack+0xd8/0x134
> [    2.330677]  sysfs_warn_dup+0x6c/0x88
> [    2.334351]  sysfs_create_dir_ns+0xe8/0x100
> [    2.338547]  kobject_add_internal+0x9c/0x290
> [    2.342833]  kobject_add+0xa0/0x108
> [    2.346331]  device_add+0xfc/0x798
> [    2.349746]  device_link_add+0x454/0x5e0
> [    2.353682]  fw_devlink_create_devlink+0xb8/0xc8
> [    2.358316]  __fw_devlink_link_to_suppliers+0x84/0x180
> [    2.363474]  __fw_devlink_link_to_suppliers+0x134/0x180
> [    2.368718]  device_add+0x778/0x798
> [    2.372217]  device_register+0x28/0x38
> [    2.375979]  __mdiobus_register+0x94/0x340
> [    2.380089]  of_mdiobus_register+0xb4/0x380
> [    2.384285]  enetc_pf_probe+0x73c/0xb10
> [    2.388132]  local_pci_probe+0x48/0xb8
> [    2.391896]  pci_device_probe+0x120/0x1c0
> [    2.395920]  really_probe+0xec/0x3c0
> [    2.399505]  driver_probe_device+0x60/0xc0
> [    2.403614]  device_driver_attach+0x7c/0x88
> [    2.407810]  __driver_attach+0x60/0xe8
> [    2.411570]  bus_for_each_dev+0x7c/0xd0
> [    2.415419]  driver_attach+0x2c/0x38
> [    2.419004]  bus_add_driver+0x194/0x1f8
> [    2.422851]  driver_register+0x6c/0x128
> [    2.426698]  __pci_register_driver+0x4c/0x58
> [    2.430983]  enetc_pf_driver_init+0x2c/0x38
> [    2.435181]  do_one_initcall+0x54/0x2d8
> [    2.439029]  kernel_init_freeable+0x1fc/0x268
> [    2.443403]  kernel_init+0x1c/0x120
> [    2.446904]  ret_from_fork+0x10/0x30
> [    2.450502] kobject_add_internal failed for 0000:00:00.1--0000:00:00.1 with -EEXIST, don't try to register things with the same name in the same directory.
>
> Looks like it will generate that link twice? Let me know if I can help
> testing.
>
> See also: https://lavalab.kontron.com/scheduler/job/3894#L831

I'll look into this this week. Is the DT for this board in upstream?
If so, can you point me to the DT file(s)?

Also, can you give me the output of this?
find /sys/devices -type d | grep "0000:00:00.1"

Thanks,
Saravana

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Michael Walle]

Am 2021-01-05 20:00, schrieb Saravana Kannan:
> On Mon, Dec 28, 2020 at 7:35 PM Michael Walle <michael@walle.cc> wrote:
>> 
>> > The current implementation of fw_devlink is very inefficient because it
>> > tries to get away without creating fwnode links in the name of saving
>> > memory usage. Past attempts to optimize runtime at the cost of memory
>> > usage were blocked with request for data showing that the optimization
>> > made significant improvement for real world scenarios.
>> >
>> > We have those scenarios now. There have been several reports of boot
>> > time increase in the order of seconds in this thread [1]. Several OEMs
>> > and SoC manufacturers have also privately reported significant
>> > (350-400ms) increase in boot time due to all the parsing done by
>> > fw_devlink.
>> >
>> > So this patch uses all the setup done by the previous patches in this
>> > series to refactor fw_devlink to be more efficient. Most of the code has
>> > been moved out of firmware specific (DT mostly) code into driver core.
>> >
>> > This brings the following benefits:
>> > - Instead of parsing the device tree multiple times during bootup,
>> >   fw_devlink parses each fwnode node/property only once and creates
>> >   fwnode links. The rest of the fw_devlink code then just looks at these
>> >   fwnode links to do rest of the work.
>> >
>> > - Makes it much easier to debug probe issue due to fw_devlink in the
>> >   future. fw_devlink=on blocks the probing of devices if they depend on
>> >   a device that hasn't been added yet. With this refactor, it'll be very
>> >   easy to tell what that device is because we now have a reference to
>> >   the fwnode of the device.
>> >
>> > - Much easier to add fw_devlink support to ACPI and other firmware
>> >   types. A refactor to move the common bits from DT specific code to
>> >   driver core was in my TODO list as a prerequisite to adding ACPI
>> >   support to fw_devlink. This series gets that done.
>> >
>> > [1] - https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/
>> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>> > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> 
>> git bisect show that this commit broke my board in 5.11-rc1:
>> 
>> [    2.294375] sysfs: cannot create duplicate filename 
>> '/devices/virtual/devlink/0000:00:00.1--0000:00:00.1'
>> [    2.303999] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 
>> 5.11.0-rc1-00016-ga0fb284b267 #267
>> [    2.312125] Hardware name: Kontron SMARC-sAL28 (4 Lane) (DT)
>> [    2.317804] Call trace:
>> [    2.320253]  dump_backtrace+0x0/0x1b8
>> [    2.323936]  show_stack+0x20/0x70
>> [    2.327263]  dump_stack+0xd8/0x134
>> [    2.330677]  sysfs_warn_dup+0x6c/0x88
>> [    2.334351]  sysfs_create_dir_ns+0xe8/0x100
>> [    2.338547]  kobject_add_internal+0x9c/0x290
>> [    2.342833]  kobject_add+0xa0/0x108
>> [    2.346331]  device_add+0xfc/0x798
>> [    2.349746]  device_link_add+0x454/0x5e0
>> [    2.353682]  fw_devlink_create_devlink+0xb8/0xc8
>> [    2.358316]  __fw_devlink_link_to_suppliers+0x84/0x180
>> [    2.363474]  __fw_devlink_link_to_suppliers+0x134/0x180
>> [    2.368718]  device_add+0x778/0x798
>> [    2.372217]  device_register+0x28/0x38
>> [    2.375979]  __mdiobus_register+0x94/0x340
>> [    2.380089]  of_mdiobus_register+0xb4/0x380
>> [    2.384285]  enetc_pf_probe+0x73c/0xb10
>> [    2.388132]  local_pci_probe+0x48/0xb8
>> [    2.391896]  pci_device_probe+0x120/0x1c0
>> [    2.395920]  really_probe+0xec/0x3c0
>> [    2.399505]  driver_probe_device+0x60/0xc0
>> [    2.403614]  device_driver_attach+0x7c/0x88
>> [    2.407810]  __driver_attach+0x60/0xe8
>> [    2.411570]  bus_for_each_dev+0x7c/0xd0
>> [    2.415419]  driver_attach+0x2c/0x38
>> [    2.419004]  bus_add_driver+0x194/0x1f8
>> [    2.422851]  driver_register+0x6c/0x128
>> [    2.426698]  __pci_register_driver+0x4c/0x58
>> [    2.430983]  enetc_pf_driver_init+0x2c/0x38
>> [    2.435181]  do_one_initcall+0x54/0x2d8
>> [    2.439029]  kernel_init_freeable+0x1fc/0x268
>> [    2.443403]  kernel_init+0x1c/0x120
>> [    2.446904]  ret_from_fork+0x10/0x30
>> [    2.450502] kobject_add_internal failed for 
>> 0000:00:00.1--0000:00:00.1 with -EEXIST, don't try to register things 
>> with the same name in the same directory.
>> 
>> Looks like it will generate that link twice? Let me know if I can help
>> testing.
>> 
>> See also: https://lavalab.kontron.com/scheduler/job/3894#L831
> 
> I'll look into this this week. Is the DT for this board in upstream?
> If so, can you point me to the DT file(s)?

arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts

> Also, can you give me the output of this?
> find /sys/devices -type d | grep "0000:00:00.1"

# uname -a
Linux buildroot 5.11.0-rc1-next-20210104 #298 SMP PREEMPT Tue Jan 5 
21:55:23 CET 2021 aarch64 GNU/Linux
# find /sys/devices -type d | grep "0000:00:00.1"
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/power
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/statistics
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/power
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-6
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-6/byte_queue_limits
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-4
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-4/byte_queue_limits
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-7
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-2
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-2/byte_queue_limits
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-5
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-0
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-0/byte_queue_limits
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-3
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-1
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-7
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-7/byte_queue_limits
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-5
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-5/byte_queue_limits
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-3
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-3/byte_queue_limits
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-6
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-1
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-1/byte_queue_limits
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-4
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-2
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-0
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/statistics
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/power
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/statistics
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/regulator
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/regulator/regulator.3
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/regulator/regulator.3/power
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/regulator/regulator.4
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/regulator/regulator.4/power
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/power
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/msi_irqs
/sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/link
/sys/devices/virtual/devlink/5000000.iommu--0000:00:00.1

HTH,
-michael

Re: [PATCH v2 16/17] driver core: Refactor fw_devlink feature

[Saravana Kannan]

On Tue, Jan 5, 2021 at 1:04 PM Michael Walle <michael@walle.cc> wrote:
>
> Am 2021-01-05 20:00, schrieb Saravana Kannan:
> > On Mon, Dec 28, 2020 at 7:35 PM Michael Walle <michael@walle.cc> wrote:
> >>
> >> > The current implementation of fw_devlink is very inefficient because it
> >> > tries to get away without creating fwnode links in the name of saving
> >> > memory usage. Past attempts to optimize runtime at the cost of memory
> >> > usage were blocked with request for data showing that the optimization
> >> > made significant improvement for real world scenarios.
> >> >
> >> > We have those scenarios now. There have been several reports of boot
> >> > time increase in the order of seconds in this thread [1]. Several OEMs
> >> > and SoC manufacturers have also privately reported significant
> >> > (350-400ms) increase in boot time due to all the parsing done by
> >> > fw_devlink.
> >> >
> >> > So this patch uses all the setup done by the previous patches in this
> >> > series to refactor fw_devlink to be more efficient. Most of the code has
> >> > been moved out of firmware specific (DT mostly) code into driver core.
> >> >
> >> > This brings the following benefits:
> >> > - Instead of parsing the device tree multiple times during bootup,
> >> >   fw_devlink parses each fwnode node/property only once and creates
> >> >   fwnode links. The rest of the fw_devlink code then just looks at these
> >> >   fwnode links to do rest of the work.
> >> >
> >> > - Makes it much easier to debug probe issue due to fw_devlink in the
> >> >   future. fw_devlink=on blocks the probing of devices if they depend on
> >> >   a device that hasn't been added yet. With this refactor, it'll be very
> >> >   easy to tell what that device is because we now have a reference to
> >> >   the fwnode of the device.
> >> >
> >> > - Much easier to add fw_devlink support to ACPI and other firmware
> >> >   types. A refactor to move the common bits from DT specific code to
> >> >   driver core was in my TODO list as a prerequisite to adding ACPI
> >> >   support to fw_devlink. This series gets that done.
> >> >
> >> > [1] - https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/
> >> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> >> > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> > Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >>
> >> git bisect show that this commit broke my board in 5.11-rc1:
> >>
> >> [    2.294375] sysfs: cannot create duplicate filename
> >> '/devices/virtual/devlink/0000:00:00.1--0000:00:00.1'
> >> [    2.303999] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
> >> 5.11.0-rc1-00016-ga0fb284b267 #267
> >> [    2.312125] Hardware name: Kontron SMARC-sAL28 (4 Lane) (DT)
> >> [    2.317804] Call trace:
> >> [    2.320253]  dump_backtrace+0x0/0x1b8
> >> [    2.323936]  show_stack+0x20/0x70
> >> [    2.327263]  dump_stack+0xd8/0x134
> >> [    2.330677]  sysfs_warn_dup+0x6c/0x88
> >> [    2.334351]  sysfs_create_dir_ns+0xe8/0x100
> >> [    2.338547]  kobject_add_internal+0x9c/0x290
> >> [    2.342833]  kobject_add+0xa0/0x108
> >> [    2.346331]  device_add+0xfc/0x798
> >> [    2.349746]  device_link_add+0x454/0x5e0
> >> [    2.353682]  fw_devlink_create_devlink+0xb8/0xc8
> >> [    2.358316]  __fw_devlink_link_to_suppliers+0x84/0x180
> >> [    2.363474]  __fw_devlink_link_to_suppliers+0x134/0x180
> >> [    2.368718]  device_add+0x778/0x798
> >> [    2.372217]  device_register+0x28/0x38
> >> [    2.375979]  __mdiobus_register+0x94/0x340
> >> [    2.380089]  of_mdiobus_register+0xb4/0x380
> >> [    2.384285]  enetc_pf_probe+0x73c/0xb10
> >> [    2.388132]  local_pci_probe+0x48/0xb8
> >> [    2.391896]  pci_device_probe+0x120/0x1c0
> >> [    2.395920]  really_probe+0xec/0x3c0
> >> [    2.399505]  driver_probe_device+0x60/0xc0
> >> [    2.403614]  device_driver_attach+0x7c/0x88
> >> [    2.407810]  __driver_attach+0x60/0xe8
> >> [    2.411570]  bus_for_each_dev+0x7c/0xd0
> >> [    2.415419]  driver_attach+0x2c/0x38
> >> [    2.419004]  bus_add_driver+0x194/0x1f8
> >> [    2.422851]  driver_register+0x6c/0x128
> >> [    2.426698]  __pci_register_driver+0x4c/0x58
> >> [    2.430983]  enetc_pf_driver_init+0x2c/0x38
> >> [    2.435181]  do_one_initcall+0x54/0x2d8
> >> [    2.439029]  kernel_init_freeable+0x1fc/0x268
> >> [    2.443403]  kernel_init+0x1c/0x120
> >> [    2.446904]  ret_from_fork+0x10/0x30
> >> [    2.450502] kobject_add_internal failed for
> >> 0000:00:00.1--0000:00:00.1 with -EEXIST, don't try to register things
> >> with the same name in the same directory.
> >>
> >> Looks like it will generate that link twice? Let me know if I can help
> >> testing.
> >>
> >> See also: https://lavalab.kontron.com/scheduler/job/3894#L831
> >
> > I'll look into this this week. Is the DT for this board in upstream?
> > If so, can you point me to the DT file(s)?
>
> arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
>
> > Also, can you give me the output of this?
> > find /sys/devices -type d | grep "0000:00:00.1"
>
> # uname -a
> Linux buildroot 5.11.0-rc1-next-20210104 #298 SMP PREEMPT Tue Jan 5
> 21:55:23 CET 2021 aarch64 GNU/Linux
> # find /sys/devices -type d | grep "0000:00:00.1"
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/power
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/statistics
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/power
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-6
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-6/byte_queue_limits
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-4
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-4/byte_queue_limits
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-7
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-2
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-2/byte_queue_limits
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-5
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-0
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-0/byte_queue_limits
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-3
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-1
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-7
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-7/byte_queue_limits
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-5
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-5/byte_queue_limits
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-3
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-3/byte_queue_limits
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-6
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-1
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/tx-1/byte_queue_limits
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-4
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-2
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/net/eno1/queues/rx-0
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/statistics
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/power
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/statistics
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/regulator
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/regulator/regulator.3
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/regulator/regulator.3/power
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/regulator/regulator.4
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/regulator/regulator.4/power
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/mdio_bus/0000:00:00.1/0000:00:00.1:04/power
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/msi_irqs
> /sys/devices/platform/soc/1f0000000.pcie/pci0000:00/0000:00:00.1/link
> /sys/devices/virtual/devlink/5000000.iommu--0000:00:00.1


Sent a fix. Any further discussion will most likely continue in that thread.
https://lore.kernel.org/lkml/20210106232641.459081-1-saravanak@google.com/T/#u

-Saravana

Re: [PATCH v2 13/17] driver core: Use device's fwnode to check if it is waiting for suppliers

[Abel Vesa]

On 20-11-20 18:02:28, Saravana Kannan wrote:
> To check if a device is still waiting for its supplier devices to be
> added, we used to check if the devices is in a global
> waiting_for_suppliers list. Since the global list will be deleted in
> subsequent patches, this patch stops using this check.
>
> Instead, this patch uses a more device specific check. It checks if the
> device's fwnode has any fwnode links that haven't been converted to
> device links yet.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 395dece1c83a..1873cecb0cc4 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -51,6 +51,7 @@ static DEFINE_MUTEX(wfs_lock);
>  static LIST_HEAD(deferred_sync);
>  static unsigned int defer_sync_state_count = 1;
>  static DEFINE_MUTEX(fwnode_link_lock);
> +static bool fw_devlink_is_permissive(void);
>
>  /**
>   * fwnode_link_add - Create a link between two fwnode_handles.
> @@ -995,13 +996,13 @@ int device_links_check_suppliers(struct device *dev)
>  	 * Device waiting for supplier to become available is not allowed to
>  	 * probe.
>  	 */
> -	mutex_lock(&wfs_lock);
> -	if (!list_empty(&dev->links.needs_suppliers) &&
> -	    dev->links.need_for_probe) {
> -		mutex_unlock(&wfs_lock);
> +	mutex_lock(&fwnode_link_lock);
> +	if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> +	    !fw_devlink_is_permissive()) {
> +		mutex_unlock(&fwnode_link_lock);

Hi Saravana,

First of, sorry for going back to this.

There is a scenario where this check will not work and probably should
work. It goes like this:

A clock controller is not allowed to probe because it uses a clock from a child device of a
consumer, like so:

	dispcc: clock-controller@af00000 {
        	clocks = <&dsi0_phy 0>;
	};

	mdss: mdss@ae00000 {
		clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;

		dsi0_phy: dsi-phy@ae94400 {
        		clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
		};
	};

This is a real scenario actually, but I stripped it down to the essentials.

So, the dsi0_phy will be "device_add'ed" (through of_platform_populate) by the mdss probe.
The mdss will probe defer waiting for the DISP_CC_MDSS_MDP_CLK, while
the dispcc will probe defer waiting for the dsi0_phy (supplier).

Basically, this 'supplier availability check' does not work when a supplier might
be populated by a consumer of the device that is currently trying to probe.


Abel


>  		return -EPROBE_DEFER;
>  	}
> -	mutex_unlock(&wfs_lock);
> +	mutex_unlock(&fwnode_link_lock);
>
>  	device_links_write_lock();
>
> @@ -1167,10 +1168,7 @@ static ssize_t waiting_for_supplier_show(struct device *dev,
>  	bool val;
>
>  	device_lock(dev);
> -	mutex_lock(&wfs_lock);
> -	val = !list_empty(&dev->links.needs_suppliers)
> -	      && dev->links.need_for_probe;
> -	mutex_unlock(&wfs_lock);
> +	val = !list_empty(&dev->fwnode->suppliers);
>  	device_unlock(dev);
>  	return sysfs_emit(buf, "%u\n", val);
>  }
> @@ -2202,7 +2200,7 @@ static int device_add_attrs(struct device *dev)
>  			goto err_remove_dev_groups;
>  	}
>
> -	if (fw_devlink_flags && !fw_devlink_is_permissive()) {
> +	if (fw_devlink_flags && !fw_devlink_is_permissive() && dev->fwnode) {
>  		error = device_create_file(dev, &dev_attr_waiting_for_supplier);
>  		if (error)
>  			goto err_remove_dev_online;
> --
> 2.29.2.454.gaff20da3a2-goog
>
>

Re: [PATCH v2 13/17] driver core: Use device's fwnode to check if it is waiting for suppliers

[Saravana Kannan]

On Mon, Jun 27, 2022 at 4:42 AM Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 20-11-20 18:02:28, Saravana Kannan wrote:
> > To check if a device is still waiting for its supplier devices to be
> > added, we used to check if the devices is in a global
> > waiting_for_suppliers list. Since the global list will be deleted in
> > subsequent patches, this patch stops using this check.
> >
> > Instead, this patch uses a more device specific check. It checks if the
> > device's fwnode has any fwnode links that haven't been converted to
> > device links yet.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 395dece1c83a..1873cecb0cc4 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -51,6 +51,7 @@ static DEFINE_MUTEX(wfs_lock);
> >  static LIST_HEAD(deferred_sync);
> >  static unsigned int defer_sync_state_count = 1;
> >  static DEFINE_MUTEX(fwnode_link_lock);
> > +static bool fw_devlink_is_permissive(void);
> >
> >  /**
> >   * fwnode_link_add - Create a link between two fwnode_handles.
> > @@ -995,13 +996,13 @@ int device_links_check_suppliers(struct device *dev)
> >        * Device waiting for supplier to become available is not allowed to
> >        * probe.
> >        */
> > -     mutex_lock(&wfs_lock);
> > -     if (!list_empty(&dev->links.needs_suppliers) &&
> > -         dev->links.need_for_probe) {
> > -             mutex_unlock(&wfs_lock);
> > +     mutex_lock(&fwnode_link_lock);
> > +     if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> > +         !fw_devlink_is_permissive()) {
> > +             mutex_unlock(&fwnode_link_lock);
>
> Hi Saravana,
>
> First of, sorry for going back to this.

No worries at all. If there's an issue with fw_devlink, I want to have it fixed.

> There is a scenario where this check will not work and probably should
> work. It goes like this:
>
> A clock controller is not allowed to probe because it uses a clock from a child device of a
> consumer, like so:
>
>         dispcc: clock-controller@af00000 {
>                 clocks = <&dsi0_phy 0>;
>         };
>
>         mdss: mdss@ae00000 {
>                 clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
>
>                 dsi0_phy: dsi-phy@ae94400 {
>                         clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>                 };
>         };
>
> This is a real scenario actually, but I stripped it down to the essentials.

I'm well aware of this scenario and explicitly wrote code to address this :)

See this comment in fw_devlink_create_devlink()

       /*
         * If we can't find the supplier device from its fwnode, it might be
         * due to a cyclic dependency between fwnodes. Some of these cycles can
         * be broken by applying logic. Check for these types of cycles and
         * break them so that devices in the cycle probe properly.
         *
         * If the supplier's parent is dependent on the consumer, then the
         * consumer and supplier have a cyclic dependency. Since fw_devlink
         * can't tell which of the inferred dependencies are incorrect, don't
         * enforce probe ordering between any of the devices in this cyclic
         * dependency. Do this by relaxing all the fw_devlink device links in
         * this cycle and by treating the fwnode link between the consumer and
         * the supplier as an invalid dependency.
         */

Applying this comment to your example, dispcc is the "consumer",
dsi0_phy is the "supplier" and mdss is the "supplier's parent".

And because we can't guarantee the order of addition of these top
level devices is why I also have this piece of recursive call inside
__fw_devlink_link_to_suppliers():

                /*
                 * If a device link was successfully created to a supplier, we
                 * now need to try and link the supplier to all its suppliers.
                 *
                 * This is needed to detect and delete false dependencies in
                 * fwnode links that haven't been converted to a device link
                 * yet. See comments in fw_devlink_create_devlink() for more
                 * details on the false dependency.
                 *
                 * Without deleting these false dependencies, some devices will
                 * never probe because they'll keep waiting for their false
                 * dependency fwnode links to be converted to device links.
                 */
                sup_dev = get_dev_from_fwnode(sup);
                __fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode);
                put_device(sup_dev);

So when mdss gets added, we'll link it to dispcc and then check if
dispcc has any suppliers it needs to link to. And that's when the
logic will catch the cycle and fix it.

Can you tell me why this wouldn't unblock the probing of dispcc? Are
you actually hitting this on a device? If so, can you please check why
this logic isn't sufficient to catch and undo the cycle?

Thanks,
Saravana

> So, the dsi0_phy will be "device_add'ed" (through of_platform_populate) by the mdss probe.
> The mdss will probe defer waiting for the DISP_CC_MDSS_MDP_CLK, while
> the dispcc will probe defer waiting for the dsi0_phy (supplier).
>
> Basically, this 'supplier availability check' does not work when a supplier might
> be populated by a consumer of the device that is currently trying to probe.
>
>
> Abel
>
>
> >               return -EPROBE_DEFER;
> >       }
> > -     mutex_unlock(&wfs_lock);
> > +     mutex_unlock(&fwnode_link_lock);
> >
> >       device_links_write_lock();
> >
> > @@ -1167,10 +1168,7 @@ static ssize_t waiting_for_supplier_show(struct device *dev,
> >       bool val;
> >
> >       device_lock(dev);
> > -     mutex_lock(&wfs_lock);
> > -     val = !list_empty(&dev->links.needs_suppliers)
> > -           && dev->links.need_for_probe;
> > -     mutex_unlock(&wfs_lock);
> > +     val = !list_empty(&dev->fwnode->suppliers);
> >       device_unlock(dev);
> >       return sysfs_emit(buf, "%u\n", val);
> >  }
> > @@ -2202,7 +2200,7 @@ static int device_add_attrs(struct device *dev)
> >                       goto err_remove_dev_groups;
> >       }
> >
> > -     if (fw_devlink_flags && !fw_devlink_is_permissive()) {
> > +     if (fw_devlink_flags && !fw_devlink_is_permissive() && dev->fwnode) {
> >               error = device_create_file(dev, &dev_attr_waiting_for_supplier);
> >               if (error)
> >                       goto err_remove_dev_online;
> > --
> > 2.29.2.454.gaff20da3a2-goog
> >
> >

Re: [PATCH v2 13/17] driver core: Use device's fwnode to check if it is waiting for suppliers

[Abel Vesa]

On 22-06-27 15:30:25, Saravana Kannan wrote:
> On Mon, Jun 27, 2022 at 4:42 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > On 20-11-20 18:02:28, Saravana Kannan wrote:
> > > To check if a device is still waiting for its supplier devices to be
> > > added, we used to check if the devices is in a global
> > > waiting_for_suppliers list. Since the global list will be deleted in
> > > subsequent patches, this patch stops using this check.
> > >
> > > Instead, this patch uses a more device specific check. It checks if the
> > > device's fwnode has any fwnode links that haven't been converted to
> > > device links yet.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/base/core.c | 18 ++++++++----------
> > >  1 file changed, 8 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 395dece1c83a..1873cecb0cc4 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -51,6 +51,7 @@ static DEFINE_MUTEX(wfs_lock);
> > >  static LIST_HEAD(deferred_sync);
> > >  static unsigned int defer_sync_state_count = 1;
> > >  static DEFINE_MUTEX(fwnode_link_lock);
> > > +static bool fw_devlink_is_permissive(void);
> > >
> > >  /**
> > >   * fwnode_link_add - Create a link between two fwnode_handles.
> > > @@ -995,13 +996,13 @@ int device_links_check_suppliers(struct device *dev)
> > >        * Device waiting for supplier to become available is not allowed to
> > >        * probe.
> > >        */
> > > -     mutex_lock(&wfs_lock);
> > > -     if (!list_empty(&dev->links.needs_suppliers) &&
> > > -         dev->links.need_for_probe) {
> > > -             mutex_unlock(&wfs_lock);
> > > +     mutex_lock(&fwnode_link_lock);
> > > +     if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> > > +         !fw_devlink_is_permissive()) {
> > > +             mutex_unlock(&fwnode_link_lock);
> >
> > Hi Saravana,
> >
> > First of, sorry for going back to this.
>
> No worries at all. If there's an issue with fw_devlink, I want to have it fixed.
>
> > There is a scenario where this check will not work and probably should
> > work. It goes like this:
> >
> > A clock controller is not allowed to probe because it uses a clock from a child device of a
> > consumer, like so:
> >
> >         dispcc: clock-controller@af00000 {
> >                 clocks = <&dsi0_phy 0>;
> >         };
> >
> >         mdss: mdss@ae00000 {
> >                 clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
> >
> >                 dsi0_phy: dsi-phy@ae94400 {
> >                         clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> >                 };
> >         };
> >
> > This is a real scenario actually, but I stripped it down to the essentials.
>
> I'm well aware of this scenario and explicitly wrote code to address this :)
>

Actually, the problem seems to be when you have two dsi phys.
Like so:

         dispcc: clock-controller@af00000 {
                 clocks = <&dsi0_phy 0>;
                 clocks = <&dsi1_phy 0>;
         };

         mdss: mdss@ae00000 {
                 clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;

                 dsi0_phy: dsi-phy@ae94400 {
                         clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
                 };

		 dsi1_phy: dsi-phy@ae64400 {
                         clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
                 };
         };

And from what I've seen happening so far is that the device_is_dependent
check for the parent of the supplier (if it also a consumer) seems to return
false on second pass of the same link due to the DL_FLAG_SYNC_STATE_ONLY
being set this time around.

> See this comment in fw_devlink_create_devlink()
>
>        /*
>          * If we can't find the supplier device from its fwnode, it might be
>          * due to a cyclic dependency between fwnodes. Some of these cycles can
>          * be broken by applying logic. Check for these types of cycles and
>          * break them so that devices in the cycle probe properly.
>          *
>          * If the supplier's parent is dependent on the consumer, then the
>          * consumer and supplier have a cyclic dependency. Since fw_devlink
>          * can't tell which of the inferred dependencies are incorrect, don't
>          * enforce probe ordering between any of the devices in this cyclic
>          * dependency. Do this by relaxing all the fw_devlink device links in
>          * this cycle and by treating the fwnode link between the consumer and
>          * the supplier as an invalid dependency.
>          */
>

So when this thing you mentioned above is happening for the second dsi
phy (order doesn't matter), since the dsi phy itself cannot be found,
the device_is_dependent is run for the same link: dispcc -> mdss
(supplier -> consumer), but again, since it has the
DL_FLAG_SYNC_STATE_ONLY this time around, it will skip that specific
link.

> Applying this comment to your example, dispcc is the "consumer",
> dsi0_phy is the "supplier" and mdss is the "supplier's parent".
>
> And because we can't guarantee the order of addition of these top
> level devices is why I also have this piece of recursive call inside
> __fw_devlink_link_to_suppliers():
>
>                 /*
>                  * If a device link was successfully created to a supplier, we
>                  * now need to try and link the supplier to all its suppliers.
>                  *
>                  * This is needed to detect and delete false dependencies in
>                  * fwnode links that haven't been converted to a device link
>                  * yet. See comments in fw_devlink_create_devlink() for more
>                  * details on the false dependency.
>                  *
>                  * Without deleting these false dependencies, some devices will
>                  * never probe because they'll keep waiting for their false
>                  * dependency fwnode links to be converted to device links.
>                  */
>                 sup_dev = get_dev_from_fwnode(sup);
>                 __fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode);
>                 put_device(sup_dev);
>
> So when mdss gets added, we'll link it to dispcc and then check if
> dispcc has any suppliers it needs to link to. And that's when the
> logic will catch the cycle and fix it.
>
> Can you tell me why this wouldn't unblock the probing of dispcc? Are
> you actually hitting this on a device? If so, can you please check why
> this logic isn't sufficient to catch and undo the cycle?
>

This is happening on Qualcomm SDM845 with Linus's tree.

> Thanks,
> Saravana
>
> > So, the dsi0_phy will be "device_add'ed" (through of_platform_populate) by the mdss probe.
> > The mdss will probe defer waiting for the DISP_CC_MDSS_MDP_CLK, while
> > the dispcc will probe defer waiting for the dsi0_phy (supplier).
> >
> > Basically, this 'supplier availability check' does not work when a supplier might
> > be populated by a consumer of the device that is currently trying to probe.
> >
> >
> > Abel
> >
> >
> > >               return -EPROBE_DEFER;
> > >       }
> > > -     mutex_unlock(&wfs_lock);
> > > +     mutex_unlock(&fwnode_link_lock);
> > >
> > >       device_links_write_lock();
> > >
> > > @@ -1167,10 +1168,7 @@ static ssize_t waiting_for_supplier_show(struct device *dev,
> > >       bool val;
> > >
> > >       device_lock(dev);
> > > -     mutex_lock(&wfs_lock);
> > > -     val = !list_empty(&dev->links.needs_suppliers)
> > > -           && dev->links.need_for_probe;
> > > -     mutex_unlock(&wfs_lock);
> > > +     val = !list_empty(&dev->fwnode->suppliers);
> > >       device_unlock(dev);
> > >       return sysfs_emit(buf, "%u\n", val);
> > >  }
> > > @@ -2202,7 +2200,7 @@ static int device_add_attrs(struct device *dev)
> > >                       goto err_remove_dev_groups;
> > >       }
> > >
> > > -     if (fw_devlink_flags && !fw_devlink_is_permissive()) {
> > > +     if (fw_devlink_flags && !fw_devlink_is_permissive() && dev->fwnode) {
> > >               error = device_create_file(dev, &dev_attr_waiting_for_supplier);
> > >               if (error)
> > >                       goto err_remove_dev_online;
> > > --
> > > 2.29.2.454.gaff20da3a2-goog
> > >
> > >
>

Re: [PATCH v2 13/17] driver core: Use device's fwnode to check if it is waiting for suppliers

[Abel Vesa]

On 22-06-28 18:24:29, Abel Vesa wrote:
> On 22-06-27 15:30:25, Saravana Kannan wrote:
> > On Mon, Jun 27, 2022 at 4:42 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> > >

Oups, forget this reply since it not to the right message-id.

Will do it properly right now.


> > > On 20-11-20 18:02:28, Saravana Kannan wrote:
> > > > To check if a device is still waiting for its supplier devices to be
> > > > added, we used to check if the devices is in a global
> > > > waiting_for_suppliers list. Since the global list will be deleted in
> > > > subsequent patches, this patch stops using this check.
> > > >
> > > > Instead, this patch uses a more device specific check. It checks if the
> > > > device's fwnode has any fwnode links that haven't been converted to
> > > > device links yet.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/base/core.c | 18 ++++++++----------
> > > >  1 file changed, 8 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index 395dece1c83a..1873cecb0cc4 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -51,6 +51,7 @@ static DEFINE_MUTEX(wfs_lock);
> > > >  static LIST_HEAD(deferred_sync);
> > > >  static unsigned int defer_sync_state_count = 1;
> > > >  static DEFINE_MUTEX(fwnode_link_lock);
> > > > +static bool fw_devlink_is_permissive(void);
> > > >
> > > >  /**
> > > >   * fwnode_link_add - Create a link between two fwnode_handles.
> > > > @@ -995,13 +996,13 @@ int device_links_check_suppliers(struct device *dev)
> > > >        * Device waiting for supplier to become available is not allowed to
> > > >        * probe.
> > > >        */
> > > > -     mutex_lock(&wfs_lock);
> > > > -     if (!list_empty(&dev->links.needs_suppliers) &&
> > > > -         dev->links.need_for_probe) {
> > > > -             mutex_unlock(&wfs_lock);
> > > > +     mutex_lock(&fwnode_link_lock);
> > > > +     if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> > > > +         !fw_devlink_is_permissive()) {
> > > > +             mutex_unlock(&fwnode_link_lock);
> > >
> > > Hi Saravana,
> > >
> > > First of, sorry for going back to this.
> >
> > No worries at all. If there's an issue with fw_devlink, I want to have it fixed.
> >
> > > There is a scenario where this check will not work and probably should
> > > work. It goes like this:
> > >
> > > A clock controller is not allowed to probe because it uses a clock from a child device of a
> > > consumer, like so:
> > >
> > >         dispcc: clock-controller@af00000 {
> > >                 clocks = <&dsi0_phy 0>;
> > >         };
> > >
> > >         mdss: mdss@ae00000 {
> > >                 clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
> > >
> > >                 dsi0_phy: dsi-phy@ae94400 {
> > >                         clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> > >                 };
> > >         };
> > >
> > > This is a real scenario actually, but I stripped it down to the essentials.
> >
> > I'm well aware of this scenario and explicitly wrote code to address this :)
> >
>
> Actually, the problem seems to be when you have two dsi phys.
> Like so:
>
>          dispcc: clock-controller@af00000 {
>                  clocks = <&dsi0_phy 0>;
>                  clocks = <&dsi1_phy 0>;
>          };
>
>          mdss: mdss@ae00000 {
>                  clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
>
>                  dsi0_phy: dsi-phy@ae94400 {
>                          clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>                  };
>
> 		 dsi1_phy: dsi-phy@ae64400 {
>                          clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>                  };
>          };
>
> And from what I've seen happening so far is that the device_is_dependent
> check for the parent of the supplier (if it also a consumer) seems to return
> false on second pass of the same link due to the DL_FLAG_SYNC_STATE_ONLY
> being set this time around.
>
> > See this comment in fw_devlink_create_devlink()
> >
> >        /*
> >          * If we can't find the supplier device from its fwnode, it might be
> >          * due to a cyclic dependency between fwnodes. Some of these cycles can
> >          * be broken by applying logic. Check for these types of cycles and
> >          * break them so that devices in the cycle probe properly.
> >          *
> >          * If the supplier's parent is dependent on the consumer, then the
> >          * consumer and supplier have a cyclic dependency. Since fw_devlink
> >          * can't tell which of the inferred dependencies are incorrect, don't
> >          * enforce probe ordering between any of the devices in this cyclic
> >          * dependency. Do this by relaxing all the fw_devlink device links in
> >          * this cycle and by treating the fwnode link between the consumer and
> >          * the supplier as an invalid dependency.
> >          */
> >
>
> So when this thing you mentioned above is happening for the second dsi
> phy (order doesn't matter), since the dsi phy itself cannot be found,
> the device_is_dependent is run for the same link: dispcc -> mdss
> (supplier -> consumer), but again, since it has the
> DL_FLAG_SYNC_STATE_ONLY this time around, it will skip that specific
> link.
>
> > Applying this comment to your example, dispcc is the "consumer",
> > dsi0_phy is the "supplier" and mdss is the "supplier's parent".
> >
> > And because we can't guarantee the order of addition of these top
> > level devices is why I also have this piece of recursive call inside
> > __fw_devlink_link_to_suppliers():
> >
> >                 /*
> >                  * If a device link was successfully created to a supplier, we
> >                  * now need to try and link the supplier to all its suppliers.
> >                  *
> >                  * This is needed to detect and delete false dependencies in
> >                  * fwnode links that haven't been converted to a device link
> >                  * yet. See comments in fw_devlink_create_devlink() for more
> >                  * details on the false dependency.
> >                  *
> >                  * Without deleting these false dependencies, some devices will
> >                  * never probe because they'll keep waiting for their false
> >                  * dependency fwnode links to be converted to device links.
> >                  */
> >                 sup_dev = get_dev_from_fwnode(sup);
> >                 __fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode);
> >                 put_device(sup_dev);
> >
> > So when mdss gets added, we'll link it to dispcc and then check if
> > dispcc has any suppliers it needs to link to. And that's when the
> > logic will catch the cycle and fix it.
> >
> > Can you tell me why this wouldn't unblock the probing of dispcc? Are
> > you actually hitting this on a device? If so, can you please check why
> > this logic isn't sufficient to catch and undo the cycle?
> >
>
> This is happening on Qualcomm SDM845 with Linus's tree.
>
> > Thanks,
> > Saravana
> >
> > > So, the dsi0_phy will be "device_add'ed" (through of_platform_populate) by the mdss probe.
> > > The mdss will probe defer waiting for the DISP_CC_MDSS_MDP_CLK, while
> > > the dispcc will probe defer waiting for the dsi0_phy (supplier).
> > >
> > > Basically, this 'supplier availability check' does not work when a supplier might
> > > be populated by a consumer of the device that is currently trying to probe.
> > >
> > >
> > > Abel
> > >
> > >
> > > >               return -EPROBE_DEFER;
> > > >       }
> > > > -     mutex_unlock(&wfs_lock);
> > > > +     mutex_unlock(&fwnode_link_lock);
> > > >
> > > >       device_links_write_lock();
> > > >
> > > > @@ -1167,10 +1168,7 @@ static ssize_t waiting_for_supplier_show(struct device *dev,
> > > >       bool val;
> > > >
> > > >       device_lock(dev);
> > > > -     mutex_lock(&wfs_lock);
> > > > -     val = !list_empty(&dev->links.needs_suppliers)
> > > > -           && dev->links.need_for_probe;
> > > > -     mutex_unlock(&wfs_lock);
> > > > +     val = !list_empty(&dev->fwnode->suppliers);
> > > >       device_unlock(dev);
> > > >       return sysfs_emit(buf, "%u\n", val);
> > > >  }
> > > > @@ -2202,7 +2200,7 @@ static int device_add_attrs(struct device *dev)
> > > >                       goto err_remove_dev_groups;
> > > >       }
> > > >
> > > > -     if (fw_devlink_flags && !fw_devlink_is_permissive()) {
> > > > +     if (fw_devlink_flags && !fw_devlink_is_permissive() && dev->fwnode) {
> > > >               error = device_create_file(dev, &dev_attr_waiting_for_supplier);
> > > >               if (error)
> > > >                       goto err_remove_dev_online;
> > > > --
> > > > 2.29.2.454.gaff20da3a2-goog
> > > >
> > > >
> >

Re: [PATCH v2 13/17] driver core: Use device's fwnode to check if it is waiting for suppliers

[Abel Vesa]

On 22-06-27 15:30:25, Saravana Kannan wrote:
> On Mon, Jun 27, 2022 at 4:42 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > On 20-11-20 18:02:28, Saravana Kannan wrote:
> > > To check if a device is still waiting for its supplier devices to be
> > > added, we used to check if the devices is in a global
> > > waiting_for_suppliers list. Since the global list will be deleted in
> > > subsequent patches, this patch stops using this check.
> > >
> > > Instead, this patch uses a more device specific check. It checks if the
> > > device's fwnode has any fwnode links that haven't been converted to
> > > device links yet.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/base/core.c | 18 ++++++++----------
> > >  1 file changed, 8 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 395dece1c83a..1873cecb0cc4 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -51,6 +51,7 @@ static DEFINE_MUTEX(wfs_lock);
> > >  static LIST_HEAD(deferred_sync);
> > >  static unsigned int defer_sync_state_count = 1;
> > >  static DEFINE_MUTEX(fwnode_link_lock);
> > > +static bool fw_devlink_is_permissive(void);
> > >
> > >  /**
> > >   * fwnode_link_add - Create a link between two fwnode_handles.
> > > @@ -995,13 +996,13 @@ int device_links_check_suppliers(struct device *dev)
> > >        * Device waiting for supplier to become available is not allowed to
> > >        * probe.
> > >        */
> > > -     mutex_lock(&wfs_lock);
> > > -     if (!list_empty(&dev->links.needs_suppliers) &&
> > > -         dev->links.need_for_probe) {
> > > -             mutex_unlock(&wfs_lock);
> > > +     mutex_lock(&fwnode_link_lock);
> > > +     if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> > > +         !fw_devlink_is_permissive()) {
> > > +             mutex_unlock(&fwnode_link_lock);
> >
> > Hi Saravana,
> >
> > First of, sorry for going back to this.
>
> No worries at all. If there's an issue with fw_devlink, I want to have it fixed.
>
> > There is a scenario where this check will not work and probably should
> > work. It goes like this:
> >
> > A clock controller is not allowed to probe because it uses a clock from a child device of a
> > consumer, like so:
> >
> >         dispcc: clock-controller@af00000 {
> >                 clocks = <&dsi0_phy 0>;
> >         };
> >
> >         mdss: mdss@ae00000 {
> >                 clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
> >
> >                 dsi0_phy: dsi-phy@ae94400 {
> >                         clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> >                 };
> >         };
> >
> > This is a real scenario actually, but I stripped it down to the essentials.
>
> I'm well aware of this scenario and explicitly wrote code to address this :)
>

Actually, the problem seems to be when you have two dsi phys.
Like so:

         dispcc: clock-controller@af00000 {
                 clocks = <&dsi0_phy 0>;
                 clocks = <&dsi1_phy 0>;
         };

         mdss: mdss@ae00000 {
                 clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;

                 dsi0_phy: dsi-phy@ae94400 {
                         clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
                 };

		 dsi1_phy: dsi-phy@ae64400 {
                         clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
                 };
         };

And from what I've seen happening so far is that the device_is_dependent
check for the parent of the supplier (if it also a consumer) seems to return
false on second pass of the same link due to the DL_FLAG_SYNC_STATE_ONLY
being set this time around.

> See this comment in fw_devlink_create_devlink()
>
>        /*
>          * If we can't find the supplier device from its fwnode, it might be
>          * due to a cyclic dependency between fwnodes. Some of these cycles can
>          * be broken by applying logic. Check for these types of cycles and
>          * break them so that devices in the cycle probe properly.
>          *
>          * If the supplier's parent is dependent on the consumer, then the
>          * consumer and supplier have a cyclic dependency. Since fw_devlink
>          * can't tell which of the inferred dependencies are incorrect, don't
>          * enforce probe ordering between any of the devices in this cyclic
>          * dependency. Do this by relaxing all the fw_devlink device links in
>          * this cycle and by treating the fwnode link between the consumer and
>          * the supplier as an invalid dependency.
>          */
>

So when this thing you mentioned above is happening for the second dsi
phy (order doesn't matter), since the dsi phy itself cannot be found,
the device_is_dependent is run for the same link: dispcc -> mdss
(supplier -> consumer), but again, since it has the
DL_FLAG_SYNC_STATE_ONLY this time around, it will skip that specific
link.

> Applying this comment to your example, dispcc is the "consumer",
> dsi0_phy is the "supplier" and mdss is the "supplier's parent".
>
> And because we can't guarantee the order of addition of these top
> level devices is why I also have this piece of recursive call inside
> __fw_devlink_link_to_suppliers():
>
>                 /*
>                  * If a device link was successfully created to a supplier, we
>                  * now need to try and link the supplier to all its suppliers.
>                  *
>                  * This is needed to detect and delete false dependencies in
>                  * fwnode links that haven't been converted to a device link
>                  * yet. See comments in fw_devlink_create_devlink() for more
>                  * details on the false dependency.
>                  *
>                  * Without deleting these false dependencies, some devices will
>                  * never probe because they'll keep waiting for their false
>                  * dependency fwnode links to be converted to device links.
>                  */
>                 sup_dev = get_dev_from_fwnode(sup);
>                 __fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode);
>                 put_device(sup_dev);
>
> So when mdss gets added, we'll link it to dispcc and then check if
> dispcc has any suppliers it needs to link to. And that's when the
> logic will catch the cycle and fix it.
>
> Can you tell me why this wouldn't unblock the probing of dispcc? Are
> you actually hitting this on a device? If so, can you please check why
> this logic isn't sufficient to catch and undo the cycle?
>

This is happening on Qualcomm SDM845 with Linus's tree.

> Thanks,
> Saravana
>
> > So, the dsi0_phy will be "device_add'ed" (through of_platform_populate) by the mdss probe.
> > The mdss will probe defer waiting for the DISP_CC_MDSS_MDP_CLK, while
> > the dispcc will probe defer waiting for the dsi0_phy (supplier).
> >
> > Basically, this 'supplier availability check' does not work when a supplier might
> > be populated by a consumer of the device that is currently trying to probe.
> >
> >
> > Abel
> >
> >
> > >               return -EPROBE_DEFER;
> > >       }
> > > -     mutex_unlock(&wfs_lock);
> > > +     mutex_unlock(&fwnode_link_lock);
> > >
> > >       device_links_write_lock();
> > >
> > > @@ -1167,10 +1168,7 @@ static ssize_t waiting_for_supplier_show(struct device *dev,
> > >       bool val;
> > >
> > >       device_lock(dev);
> > > -     mutex_lock(&wfs_lock);
> > > -     val = !list_empty(&dev->links.needs_suppliers)
> > > -           && dev->links.need_for_probe;
> > > -     mutex_unlock(&wfs_lock);
> > > +     val = !list_empty(&dev->fwnode->suppliers);
> > >       device_unlock(dev);
> > >       return sysfs_emit(buf, "%u\n", val);
> > >  }
> > > @@ -2202,7 +2200,7 @@ static int device_add_attrs(struct device *dev)
> > >                       goto err_remove_dev_groups;
> > >       }
> > >
> > > -     if (fw_devlink_flags && !fw_devlink_is_permissive()) {
> > > +     if (fw_devlink_flags && !fw_devlink_is_permissive() && dev->fwnode) {
> > >               error = device_create_file(dev, &dev_attr_waiting_for_supplier);
> > >               if (error)
> > >                       goto err_remove_dev_online;
> > > --
> > > 2.29.2.454.gaff20da3a2-goog
> > >
> > >
>

Re: [PATCH v2 13/17] driver core: Use device's fwnode to check if it is waiting for suppliers

[Saravana Kannan]

On Tue, Jun 28, 2022 at 8:55 AM Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 22-06-27 15:30:25, Saravana Kannan wrote:
> > On Mon, Jun 27, 2022 at 4:42 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> > >
> > > On 20-11-20 18:02:28, Saravana Kannan wrote:
> > > > To check if a device is still waiting for its supplier devices to be
> > > > added, we used to check if the devices is in a global
> > > > waiting_for_suppliers list. Since the global list will be deleted in
> > > > subsequent patches, this patch stops using this check.
> > > >
> > > > Instead, this patch uses a more device specific check. It checks if the
> > > > device's fwnode has any fwnode links that haven't been converted to
> > > > device links yet.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/base/core.c | 18 ++++++++----------
> > > >  1 file changed, 8 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index 395dece1c83a..1873cecb0cc4 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -51,6 +51,7 @@ static DEFINE_MUTEX(wfs_lock);
> > > >  static LIST_HEAD(deferred_sync);
> > > >  static unsigned int defer_sync_state_count = 1;
> > > >  static DEFINE_MUTEX(fwnode_link_lock);
> > > > +static bool fw_devlink_is_permissive(void);
> > > >
> > > >  /**
> > > >   * fwnode_link_add - Create a link between two fwnode_handles.
> > > > @@ -995,13 +996,13 @@ int device_links_check_suppliers(struct device *dev)
> > > >        * Device waiting for supplier to become available is not allowed to
> > > >        * probe.
> > > >        */
> > > > -     mutex_lock(&wfs_lock);
> > > > -     if (!list_empty(&dev->links.needs_suppliers) &&
> > > > -         dev->links.need_for_probe) {
> > > > -             mutex_unlock(&wfs_lock);
> > > > +     mutex_lock(&fwnode_link_lock);
> > > > +     if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> > > > +         !fw_devlink_is_permissive()) {
> > > > +             mutex_unlock(&fwnode_link_lock);
> > >
> > > Hi Saravana,
> > >
> > > First of, sorry for going back to this.
> >
> > No worries at all. If there's an issue with fw_devlink, I want to have it fixed.
> >
> > > There is a scenario where this check will not work and probably should
> > > work. It goes like this:
> > >
> > > A clock controller is not allowed to probe because it uses a clock from a child device of a
> > > consumer, like so:
> > >
> > >         dispcc: clock-controller@af00000 {
> > >                 clocks = <&dsi0_phy 0>;
> > >         };
> > >
> > >         mdss: mdss@ae00000 {
> > >                 clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
> > >
> > >                 dsi0_phy: dsi-phy@ae94400 {
> > >                         clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> > >                 };
> > >         };
> > >
> > > This is a real scenario actually, but I stripped it down to the essentials.
> >
> > I'm well aware of this scenario and explicitly wrote code to address this :)
> >
>
> Actually, the problem seems to be when you have two dsi phys.
> Like so:
>
>          dispcc: clock-controller@af00000 {
>                  clocks = <&dsi0_phy 0>;
>                  clocks = <&dsi1_phy 0>;
>          };
>
>          mdss: mdss@ae00000 {
>                  clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
>
>                  dsi0_phy: dsi-phy@ae94400 {
>                          clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>                  };
>
>                  dsi1_phy: dsi-phy@ae64400 {
>                          clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>                  };
>          };
>
> And from what I've seen happening so far is that the device_is_dependent
> check for the parent of the supplier (if it also a consumer) seems to return
> false on second pass of the same link due to the DL_FLAG_SYNC_STATE_ONLY
> being set this time around.
>
> > See this comment in fw_devlink_create_devlink()
> >
> >        /*
> >          * If we can't find the supplier device from its fwnode, it might be
> >          * due to a cyclic dependency between fwnodes. Some of these cycles can
> >          * be broken by applying logic. Check for these types of cycles and
> >          * break them so that devices in the cycle probe properly.
> >          *
> >          * If the supplier's parent is dependent on the consumer, then the
> >          * consumer and supplier have a cyclic dependency. Since fw_devlink
> >          * can't tell which of the inferred dependencies are incorrect, don't
> >          * enforce probe ordering between any of the devices in this cyclic
> >          * dependency. Do this by relaxing all the fw_devlink device links in
> >          * this cycle and by treating the fwnode link between the consumer and
> >          * the supplier as an invalid dependency.
> >          */
> >
>
> So when this thing you mentioned above is happening for the second dsi
> phy (order doesn't matter), since the dsi phy itself cannot be found,
> the device_is_dependent is run for the same link: dispcc -> mdss
> (supplier -> consumer), but again, since it has the
> DL_FLAG_SYNC_STATE_ONLY this time around, it will skip that specific
> link.

Ugh... I knew there was this gap, but didn't expect it to be a real world issue.

There are different ways of addressing this and they all fall
somewhere within a spectrum of:
"stop enforcing very specific edges of the dependency graph when you
find a cycles"
To
"just don't enforce any dependency for devices in a cycle and let the
drivers figure out when to -EPROBE_DEFER".

And each of those are of varying complexity. Ideally I'd prefer to
relax specific edges, but I need to balance it out with the code
complexity. Let me soak this for a few weeks to decide on what option
to take.

Thanks for the report.

-Saravana

>
> > Applying this comment to your example, dispcc is the "consumer",
> > dsi0_phy is the "supplier" and mdss is the "supplier's parent".
> >
> > And because we can't guarantee the order of addition of these top
> > level devices is why I also have this piece of recursive call inside
> > __fw_devlink_link_to_suppliers():
> >
> >                 /*
> >                  * If a device link was successfully created to a supplier, we
> >                  * now need to try and link the supplier to all its suppliers.
> >                  *
> >                  * This is needed to detect and delete false dependencies in
> >                  * fwnode links that haven't been converted to a device link
> >                  * yet. See comments in fw_devlink_create_devlink() for more
> >                  * details on the false dependency.
> >                  *
> >                  * Without deleting these false dependencies, some devices will
> >                  * never probe because they'll keep waiting for their false
> >                  * dependency fwnode links to be converted to device links.
> >                  */
> >                 sup_dev = get_dev_from_fwnode(sup);
> >                 __fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode);
> >                 put_device(sup_dev);
> >
> > So when mdss gets added, we'll link it to dispcc and then check if
> > dispcc has any suppliers it needs to link to. And that's when the
> > logic will catch the cycle and fix it.
> >
> > Can you tell me why this wouldn't unblock the probing of dispcc? Are
> > you actually hitting this on a device? If so, can you please check why
> > this logic isn't sufficient to catch and undo the cycle?
> >
>
> This is happening on Qualcomm SDM845 with Linus's tree.
>
> > Thanks,
> > Saravana
> >
> > > So, the dsi0_phy will be "device_add'ed" (through of_platform_populate) by the mdss probe.
> > > The mdss will probe defer waiting for the DISP_CC_MDSS_MDP_CLK, while
> > > the dispcc will probe defer waiting for the dsi0_phy (supplier).
> > >
> > > Basically, this 'supplier availability check' does not work when a supplier might
> > > be populated by a consumer of the device that is currently trying to probe.
> > >
> > >
> > > Abel
> > >
> > >
> > > >               return -EPROBE_DEFER;
> > > >       }
> > > > -     mutex_unlock(&wfs_lock);
> > > > +     mutex_unlock(&fwnode_link_lock);
> > > >
> > > >       device_links_write_lock();
> > > >
> > > > @@ -1167,10 +1168,7 @@ static ssize_t waiting_for_supplier_show(struct device *dev,
> > > >       bool val;
> > > >
> > > >       device_lock(dev);
> > > > -     mutex_lock(&wfs_lock);
> > > > -     val = !list_empty(&dev->links.needs_suppliers)
> > > > -           && dev->links.need_for_probe;
> > > > -     mutex_unlock(&wfs_lock);
> > > > +     val = !list_empty(&dev->fwnode->suppliers);
> > > >       device_unlock(dev);
> > > >       return sysfs_emit(buf, "%u\n", val);
> > > >  }
> > > > @@ -2202,7 +2200,7 @@ static int device_add_attrs(struct device *dev)
> > > >                       goto err_remove_dev_groups;
> > > >       }
> > > >
> > > > -     if (fw_devlink_flags && !fw_devlink_is_permissive()) {
> > > > +     if (fw_devlink_flags && !fw_devlink_is_permissive() && dev->fwnode) {
> > > >               error = device_create_file(dev, &dev_attr_waiting_for_supplier);
> > > >               if (error)
> > > >                       goto err_remove_dev_online;
> > > > --
> > > > 2.29.2.454.gaff20da3a2-goog
> > > >
> > > >
> >

Re: [PATCH v2 13/17] driver core: Use device's fwnode to check if it is waiting for suppliers

[Dmitry Baryshkov]

Hi,

On 28/06/2022 21:09, Saravana Kannan wrote:
> On Tue, Jun 28, 2022 at 8:55 AM Abel Vesa <abel.vesa@linaro.org> wrote:
>>
>> On 22-06-27 15:30:25, Saravana Kannan wrote:
>>> On Mon, Jun 27, 2022 at 4:42 AM Abel Vesa <abel.vesa@linaro.org> wrote:
>>>>
>>>> On 20-11-20 18:02:28, Saravana Kannan wrote:
>>>>> To check if a device is still waiting for its supplier devices to be
>>>>> added, we used to check if the devices is in a global
>>>>> waiting_for_suppliers list. Since the global list will be deleted in
>>>>> subsequent patches, this patch stops using this check.
>>>>>
>>>>> Instead, this patch uses a more device specific check. It checks if the
>>>>> device's fwnode has any fwnode links that haven't been converted to
>>>>> device links yet.
>>>>>
>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>>> ---
>>>>>   drivers/base/core.c | 18 ++++++++----------
>>>>>   1 file changed, 8 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>>>> index 395dece1c83a..1873cecb0cc4 100644
>>>>> --- a/drivers/base/core.c
>>>>> +++ b/drivers/base/core.c
>>>>> @@ -51,6 +51,7 @@ static DEFINE_MUTEX(wfs_lock);
>>>>>   static LIST_HEAD(deferred_sync);
>>>>>   static unsigned int defer_sync_state_count = 1;
>>>>>   static DEFINE_MUTEX(fwnode_link_lock);
>>>>> +static bool fw_devlink_is_permissive(void);
>>>>>
>>>>>   /**
>>>>>    * fwnode_link_add - Create a link between two fwnode_handles.
>>>>> @@ -995,13 +996,13 @@ int device_links_check_suppliers(struct device *dev)
>>>>>         * Device waiting for supplier to become available is not allowed to
>>>>>         * probe.
>>>>>         */
>>>>> -     mutex_lock(&wfs_lock);
>>>>> -     if (!list_empty(&dev->links.needs_suppliers) &&
>>>>> -         dev->links.need_for_probe) {
>>>>> -             mutex_unlock(&wfs_lock);
>>>>> +     mutex_lock(&fwnode_link_lock);
>>>>> +     if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
>>>>> +         !fw_devlink_is_permissive()) {
>>>>> +             mutex_unlock(&fwnode_link_lock);
>>>>
>>>> Hi Saravana,
>>>>
>>>> First of, sorry for going back to this.
>>>
>>> No worries at all. If there's an issue with fw_devlink, I want to have it fixed.
>>>
>>>> There is a scenario where this check will not work and probably should
>>>> work. It goes like this:
>>>>
>>>> A clock controller is not allowed to probe because it uses a clock from a child device of a
>>>> consumer, like so:
>>>>
>>>>          dispcc: clock-controller@af00000 {
>>>>                  clocks = <&dsi0_phy 0>;
>>>>          };
>>>>
>>>>          mdss: mdss@ae00000 {
>>>>                  clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
>>>>
>>>>                  dsi0_phy: dsi-phy@ae94400 {
>>>>                          clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>>>>                  };
>>>>          };
>>>>
>>>> This is a real scenario actually, but I stripped it down to the essentials.
>>>
>>> I'm well aware of this scenario and explicitly wrote code to address this :)
>>>
>>
>> Actually, the problem seems to be when you have two dsi phys.
>> Like so:
>>
>>           dispcc: clock-controller@af00000 {
>>                   clocks = <&dsi0_phy 0>;
>>                   clocks = <&dsi1_phy 0>;
>>           };
>>
>>           mdss: mdss@ae00000 {
>>                   clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
>>
>>                   dsi0_phy: dsi-phy@ae94400 {
>>                           clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>>                   };
>>
>>                   dsi1_phy: dsi-phy@ae64400 {
>>                           clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>>                   };
>>           };
>>
>> And from what I've seen happening so far is that the device_is_dependent
>> check for the parent of the supplier (if it also a consumer) seems to return
>> false on second pass of the same link due to the DL_FLAG_SYNC_STATE_ONLY
>> being set this time around.
>>
>>> See this comment in fw_devlink_create_devlink()
>>>
>>>         /*
>>>           * If we can't find the supplier device from its fwnode, it might be
>>>           * due to a cyclic dependency between fwnodes. Some of these cycles can
>>>           * be broken by applying logic. Check for these types of cycles and
>>>           * break them so that devices in the cycle probe properly.
>>>           *
>>>           * If the supplier's parent is dependent on the consumer, then the
>>>           * consumer and supplier have a cyclic dependency. Since fw_devlink
>>>           * can't tell which of the inferred dependencies are incorrect, don't
>>>           * enforce probe ordering between any of the devices in this cyclic
>>>           * dependency. Do this by relaxing all the fw_devlink device links in
>>>           * this cycle and by treating the fwnode link between the consumer and
>>>           * the supplier as an invalid dependency.
>>>           */
>>>
>>
>> So when this thing you mentioned above is happening for the second dsi
>> phy (order doesn't matter), since the dsi phy itself cannot be found,
>> the device_is_dependent is run for the same link: dispcc -> mdss
>> (supplier -> consumer), but again, since it has the
>> DL_FLAG_SYNC_STATE_ONLY this time around, it will skip that specific
>> link.
> 
> Ugh... I knew there was this gap, but didn't expect it to be a real world issue.
> 
> There are different ways of addressing this and they all fall
> somewhere within a spectrum of:
> "stop enforcing very specific edges of the dependency graph when you
> find a cycles"
> To
> "just don't enforce any dependency for devices in a cycle and let the
> drivers figure out when to -EPROBE_DEFER".
> 
> And each of those are of varying complexity. Ideally I'd prefer to
> relax specific edges, but I need to balance it out with the code
> complexity. Let me soak this for a few weeks to decide on what option
> to take.

I wanted to check if there is any progress on this topic? It appears 
that few weeks turned into few months already and the issue is still 
present. If not, can we please re-consider applying [1] while Saravana 
is working on a better fix?

[1] 
https://lore.kernel.org/all/20211125183622.597177-1-dmitry.baryshkov@linaro.org/

> 
> Thanks for the report.
> 
> -Saravana
> 
>>
>>> Applying this comment to your example, dispcc is the "consumer",
>>> dsi0_phy is the "supplier" and mdss is the "supplier's parent".
>>>
>>> And because we can't guarantee the order of addition of these top
>>> level devices is why I also have this piece of recursive call inside
>>> __fw_devlink_link_to_suppliers():
>>>
>>>                  /*
>>>                   * If a device link was successfully created to a supplier, we
>>>                   * now need to try and link the supplier to all its suppliers.
>>>                   *
>>>                   * This is needed to detect and delete false dependencies in
>>>                   * fwnode links that haven't been converted to a device link
>>>                   * yet. See comments in fw_devlink_create_devlink() for more
>>>                   * details on the false dependency.
>>>                   *
>>>                   * Without deleting these false dependencies, some devices will
>>>                   * never probe because they'll keep waiting for their false
>>>                   * dependency fwnode links to be converted to device links.
>>>                   */
>>>                  sup_dev = get_dev_from_fwnode(sup);
>>>                  __fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode);
>>>                  put_device(sup_dev);
>>>
>>> So when mdss gets added, we'll link it to dispcc and then check if
>>> dispcc has any suppliers it needs to link to. And that's when the
>>> logic will catch the cycle and fix it.
>>>
>>> Can you tell me why this wouldn't unblock the probing of dispcc? Are
>>> you actually hitting this on a device? If so, can you please check why
>>> this logic isn't sufficient to catch and undo the cycle?
>>>
>>
>> This is happening on Qualcomm SDM845 with Linus's tree.
>>
>>> Thanks,
>>> Saravana
>>>
>>>> So, the dsi0_phy will be "device_add'ed" (through of_platform_populate) by the mdss probe.
>>>> The mdss will probe defer waiting for the DISP_CC_MDSS_MDP_CLK, while
>>>> the dispcc will probe defer waiting for the dsi0_phy (supplier).
>>>>
>>>> Basically, this 'supplier availability check' does not work when a supplier might
>>>> be populated by a consumer of the device that is currently trying to probe.
>>>>
>>>>
>>>> Abel
>>>>
>>>>
>>>>>                return -EPROBE_DEFER;
>>>>>        }
>>>>> -     mutex_unlock(&wfs_lock);
>>>>> +     mutex_unlock(&fwnode_link_lock);
>>>>>
>>>>>        device_links_write_lock();
>>>>>
>>>>> @@ -1167,10 +1168,7 @@ static ssize_t waiting_for_supplier_show(struct device *dev,
>>>>>        bool val;
>>>>>
>>>>>        device_lock(dev);
>>>>> -     mutex_lock(&wfs_lock);
>>>>> -     val = !list_empty(&dev->links.needs_suppliers)
>>>>> -           && dev->links.need_for_probe;
>>>>> -     mutex_unlock(&wfs_lock);
>>>>> +     val = !list_empty(&dev->fwnode->suppliers);
>>>>>        device_unlock(dev);
>>>>>        return sysfs_emit(buf, "%u\n", val);
>>>>>   }
>>>>> @@ -2202,7 +2200,7 @@ static int device_add_attrs(struct device *dev)
>>>>>                        goto err_remove_dev_groups;
>>>>>        }
>>>>>
>>>>> -     if (fw_devlink_flags && !fw_devlink_is_permissive()) {
>>>>> +     if (fw_devlink_flags && !fw_devlink_is_permissive() && dev->fwnode) {
>>>>>                error = device_create_file(dev, &dev_attr_waiting_for_supplier);
>>>>>                if (error)
>>>>>                        goto err_remove_dev_online;
>>>>> --
>>>>> 2.29.2.454.gaff20da3a2-goog
>>>>>
>>>>>
>>>

-- 
With best wishes
Dmitry