[PATCH v1 00/18] Refactor fw_devlink to significantly improve boot time

[PATCH v1 00/18] 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.

Tomi/Laurent/Grygorii,

If you can test this series, that'd be great!

Thanks,
Saravana

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

Saravana Kannan (18):
  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()
  driver core: Redefine the meaning of fwnode_operations.add_links()
  driver core: Add fw_devlink_parse_fwtree()
  driver core: Add fwnode_get_next_parent_dev() helper function
  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: Add helper functions to convert fwnode links to device links
  driver core: Refactor fw_devlink feature

 drivers/acpi/property.c         |   2 +-
 drivers/acpi/scan.c             |   2 +-
 drivers/base/core.c             | 584 +++++++++++++++++++++-----------
 drivers/base/property.c         |  27 ++
 drivers/base/swnode.c           |   2 +-
 drivers/firmware/efi/efi-init.c |  31 +-
 drivers/of/dynamic.c            |   1 +
 drivers/of/platform.c           |   2 -
 drivers/of/property.c           | 150 +++-----
 include/linux/device.h          |  10 +-
 include/linux/fwnode.h          |  66 ++--
 include/linux/of.h              |   2 +-
 include/linux/property.h        |   2 +
 kernel/irq/irqdomain.c          |   2 +-
 14 files changed, 490 insertions(+), 393 deletions(-)

-- 
2.29.1.341.ge80a0c044ae-goog

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

[Saravana Kannan]

This reverts commit 2451e746478a6a6e981cfa66b62b791ca93b90c8.

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 78114ddac755..a56601e68a8c 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);
 
@@ -1472,12 +1471,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)
@@ -1546,9 +1539,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!");
@@ -1560,19 +1550,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.1.341.ge80a0c044ae-goog

Re: [PATCH v1 01/18] Revert "driver core: Avoid deferred probe due to fw_devlink_pause/resume()"

[Greg Kroah-Hartman]

On Wed, Nov 04, 2020 at 03:23:38PM -0800, Saravana Kannan wrote:
> This reverts commit 2451e746478a6a6e981cfa66b62b791ca93b90c8.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>

You need to say _why_ you are doing this, it's obvious _what_ you are
doing :)

Same for the other reverts in this series.

thanks,

greg k-h

Re: [PATCH v1 01/18] Revert "driver core: Avoid deferred probe due to fw_devlink_pause/resume()"

[Saravana Kannan]

On Thu, Nov 5, 2020 at 1:33 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Nov 04, 2020 at 03:23:38PM -0800, Saravana Kannan wrote:
> > This reverts commit 2451e746478a6a6e981cfa66b62b791ca93b90c8.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> You need to say _why_ you are doing this, it's obvious _what_ you are
> doing :)
>
> Same for the other reverts in this series.

Sorry, forgot about this. Will fix it in v2.

-Saravana

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

[Saravana Kannan]

This reverts commit ec7bd78498f29680f536451fbdf9464e851273ed.

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 a56601e68a8c..2328c8951695 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -961,11 +961,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);
 }
 
 /**
@@ -983,8 +983,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);
@@ -1022,12 +1022,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:
@@ -1045,8 +1045,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)
@@ -1276,7 +1276,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();
@@ -2409,7 +2409,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.1.341.ge80a0c044ae-goog

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

[Saravana Kannan]

This reverts commit cec72f3efc6272420c2c2c699607f03d09b93e41.

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 2328c8951695..6745375a8bb9 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1550,6 +1550,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 b42229b74fd6..008724f8edf1 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.1.341.ge80a0c044ae-goog

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

[Saravana Kannan]

This reverts commit fefcfc968723caf93318613a08e1f3ad07a6154f.

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 008724f8edf1..ec67b5ffa06d 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.1.341.ge80a0c044ae-goog

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

[Saravana Kannan]

This reverts commit 93d2e4322aa74c1ad1e8c2160608eb9a960d69ff.

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.1.341.ge80a0c044ae-goog

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

[Saravana Kannan]

This reverts commit 716a7a25969003d82ab738179c3f1068a120ed11.

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 6745375a8bb9..31a76159f118 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);
@@ -1444,116 +1441,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 ec67b5ffa06d..b42229b74fd6 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.1.341.ge80a0c044ae-goog

[PATCH v1 07/18] 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          | 5 +++++
 include/linux/of.h              | 2 +-
 kernel/irq/irqdomain.c          | 2 +-
 7 files changed, 14 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 a896e5e87c93..0ac19f9274b8 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..593fb8e58f21 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -169,6 +169,11 @@ struct fwnode_operations {
 			(fwnode)->ops->op(fwnode, ## __VA_ARGS__);	\
 	} 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);
 
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.1.341.ge80a0c044ae-goog

Re: [PATCH v1 07/18] driver core: Add fwnode_init()

[Greg Kroah-Hartman]

On Wed, Nov 04, 2020 at 03:23:44PM -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          | 5 +++++
>  include/linux/of.h              | 2 +-
>  kernel/irq/irqdomain.c          | 2 +-
>  7 files changed, 14 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 a896e5e87c93..0ac19f9274b8 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..593fb8e58f21 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -169,6 +169,11 @@ struct fwnode_operations {
>  			(fwnode)->ops->op(fwnode, ## __VA_ARGS__);	\
>  	} 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;
> +}
>  

A blank line before a new inline function is always nice to have :)

thanks,

greg k-h

Re: [PATCH v1 07/18] driver core: Add fwnode_init()

[Saravana Kannan]

On Thu, Nov 5, 2020 at 1:35 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Nov 04, 2020 at 03:23:44PM -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          | 5 +++++
> >  include/linux/of.h              | 2 +-
> >  kernel/irq/irqdomain.c          | 2 +-
> >  7 files changed, 14 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 a896e5e87c93..0ac19f9274b8 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..593fb8e58f21 100644
> > --- a/include/linux/fwnode.h
> > +++ b/include/linux/fwnode.h
> > @@ -169,6 +169,11 @@ struct fwnode_operations {
> >                       (fwnode)->ops->op(fwnode, ## __VA_ARGS__);      \
> >       } 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;
> > +}
> >
>
> A blank line before a new inline function is always nice to have :)
>

Ack

-Saravana

[PATCH v1 08/18] driver core: Add fwnode link support

[Saravana Kannan]

This patch adds support for creating supplier-consumer links between
fwnode. It is intentionally kept simple and with limited APIs as it is
meant to be used only by driver core and firmware code (Eg: device tree,
ACPI, etc).

We can expand the APIs later if there is ever a need for
drivers/frameworks to start using them.

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 31a76159f118..1a1d9a55645c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -50,6 +50,101 @@ 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.
+ *
+ * Creates a fwnode link between two fwnode_handles. These fwnode links are
+ * used by the driver core to automatically generate device links. Attempts to
+ * create duplicate links are simply ignored and there is no refcounting.
+ *
+ * These links are automatically deleted once they are converted to device
+ * links or when the fwnode_handles (or their corresponding devices) are
+ * deleted.
+ */
+int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)
+{
+	struct fwnode_link *link;
+	int ret = 0;
+
+	mutex_lock(&fwnode_link_lock);
+
+	/* Duplicate requests are intentionally not refcounted. */
+	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 needs to be deleted
+ *
+ * Deletes all supplier links connecting directly to a 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 needs to be deleted
+ *
+ * Deletes all consumer links connecting directly to a 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 593fb8e58f21..afde643f37a2 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;
 };
 
 /**
@@ -173,8 +183,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.1.341.ge80a0c044ae-goog

Re: [PATCH v1 08/18] driver core: Add fwnode link support

[Rafael J. Wysocki]

On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
>
> This patch adds support for creating supplier-consumer links between

Generally speaking the "This patch" part is redundant.  It is
sufficient to simply say "Add ...".

> fwnode.

fwnodes (plural)?

> It is intentionally kept simple and with limited APIs as it is
> meant to be used only by driver core and firmware code (Eg: device tree,
> ACPI, etc).

I'd say "It is intended for internal use in the driver core and
generic firmware support code (eg. Device Tree, ACPI), so it is simple
by design and the API provided by it is limited."

>
> We can expand the APIs later if there is ever a need for
> drivers/frameworks to start using them.

The above is totally redundant IMO.

>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c    | 95 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/of/dynamic.c   |  1 +
>  include/linux/fwnode.h | 14 +++++++
>  3 files changed, 110 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 31a76159f118..1a1d9a55645c 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -50,6 +50,101 @@ 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.
> + *
> + * Creates a fwnode link between two fwnode_handles. These fwnode links are

Why don't you refer to the arguments here, that is "Create a link
between fwnode handles @con and @sup ..."

> + * used by the driver core to automatically generate device links. Attempts to
> + * create duplicate links are simply ignored and there is no refcounting.

And I'd generally write it this way:

"Create a link between fwnode handles @con and @sup representing a
pair of devices the first of which uses certain resources provided by
the second one, respectively.

The driver core will use that link to create a device link between the
two device objects corresponding to @con and @sup when they are
created and it will automatically delete the link between @con and
@sup after doing that.

Attempts to create a duplicate link between the same pair of fwnode
handles are ignored and there is no reference counting."

> + *
> + * These links are automatically deleted once they are converted to device
> + * links or when the fwnode_handles (or their corresponding devices) are
> + * deleted.
> + */
> +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)

Why doesn't it return a pointer to the new link or NULL?

That would be consistent with device_link_add().

> +{
> +       struct fwnode_link *link;
> +       int ret = 0;
> +
> +       mutex_lock(&fwnode_link_lock);
> +
> +       /* Duplicate requests are intentionally not refcounted. */

Is this comment really necessary?

> +       list_for_each_entry(link, &sup->consumers, s_hook)
> +               if (link->consumer == con)
> +                       goto out;

It is also necessary to look the other way around AFAICS, that is if
there is a link between the two fwnode handles in the other direction
already, the creation of a new one should fail.

> +
> +       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 needs to be deleted

s/needs/need/

> + *
> + * Deletes all supplier links connecting directly to a fwnode.

I'd say "Delete all supplier links connecting directly to @fwnode."
and analogously below.

> + */
> +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 needs to be deleted
> + *
> + * Deletes all consumer links connecting directly to a 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);

I'd avoid the code duplication, even though it doesn't appear to be
significant ATM.

> +       }
> +       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);

Dropping the lock here may turn out to be problematic at one point
going forward.  IMO it is better to hold it throughout the entire
operation.

> +       fwnode_links_purge_consumers(fwnode);

I'd get rid of the two functions above, add something like
fwnode_link_del() and walk the lists directly here calling it for
every link on the way.

> +}
>
>  #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 593fb8e58f21..afde643f37a2 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;
>  };
>
>  /**
> @@ -173,8 +183,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.1.341.ge80a0c044ae-goog
>

Re: [PATCH v1 08/18] driver core: Add fwnode link support

[Saravana Kannan]

On Mon, Nov 16, 2020 at 7:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > This patch adds support for creating supplier-consumer links between
>
> Generally speaking the "This patch" part is redundant.  It is
> sufficient to simply say "Add ...".
>
> > fwnode.
>
> fwnodes (plural)?
>
> > It is intentionally kept simple and with limited APIs as it is
> > meant to be used only by driver core and firmware code (Eg: device tree,
> > ACPI, etc).
>
> I'd say "It is intended for internal use in the driver core and
> generic firmware support code (eg. Device Tree, ACPI), so it is simple
> by design and the API provided by it is limited."
>
> >
> > We can expand the APIs later if there is ever a need for
> > drivers/frameworks to start using them.
>
> The above is totally redundant IMO.
>
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c    | 95 ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/of/dynamic.c   |  1 +
> >  include/linux/fwnode.h | 14 +++++++
> >  3 files changed, 110 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 31a76159f118..1a1d9a55645c 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -50,6 +50,101 @@ 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.
> > + *
> > + * Creates a fwnode link between two fwnode_handles. These fwnode links are
>
> Why don't you refer to the arguments here, that is "Create a link
> between fwnode handles @con and @sup ..."

Ack/done to everything above.

>
> > + * used by the driver core to automatically generate device links. Attempts to
> > + * create duplicate links are simply ignored and there is no refcounting.
>
> And I'd generally write it this way:
>
> "Create a link between fwnode handles @con and @sup representing a
> pair of devices the first of which uses certain resources provided by
> the second one, respectively.
>
> The driver core will use that link to create a device link between the
> two device objects corresponding to @con and @sup when they are
> created and it will automatically delete the link between @con and
> @sup after doing that.
>
> Attempts to create a duplicate link between the same pair of fwnode
> handles are ignored and there is no reference counting."

Took most of this as is with some minor rewording.

>
> > + *
> > + * These links are automatically deleted once they are converted to device
> > + * links or when the fwnode_handles (or their corresponding devices) are
> > + * deleted.
> > + */
> > +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)
>
> Why doesn't it return a pointer to the new link or NULL?
>
> That would be consistent with device_link_add().

However, as opposed to device_link_add(), I don't want the caller
holding any reference to the allocated link. So I don't want to return
any pointer to them.

> > +{
> > +       struct fwnode_link *link;
> > +       int ret = 0;
> > +
> > +       mutex_lock(&fwnode_link_lock);
> > +
> > +       /* Duplicate requests are intentionally not refcounted. */
>
> Is this comment really necessary?

I guess with the function comment explicitly stating "no ref
counting", this is kind of redundant. I can remove this.

>
> > +       list_for_each_entry(link, &sup->consumers, s_hook)
> > +               if (link->consumer == con)
> > +                       goto out;
>
> It is also necessary to look the other way around AFAICS, that is if
> there is a link between the two fwnode handles in the other direction
> already, the creation of a new one should fail.

No, fwnode links can have cycles. At this state, we can't tell which
ones are invali.d When we create device links out of this, we have
more info at that point and we make sure not to add any device links
that can cause cycles. There are a bunch of corner cases where we
can't tell which one is the invalid fwnode link in the links that make
up the cycle and in those cases, we have to make the device links as
SYNC_STATE_ONLY device links. Long story short, cycles are allowed.

>
> > +
> > +       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 needs to be deleted
>
> s/needs/need/

Ack

>
> > + *
> > + * Deletes all supplier links connecting directly to a fwnode.
>
> I'd say "Delete all supplier links connecting directly to @fwnode."
> and analogously below.

Ack

>
> > + */
> > +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 needs to be deleted
> > + *
> > + * Deletes all consumer links connecting directly to a 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);
>
> I'd avoid the code duplication, even though it doesn't appear to be
> significant ATM.

I'd like to leave this as is for now if that's okay.

> > +       }
> > +       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);
>
> Dropping the lock here may turn out to be problematic at one point
> going forward.  IMO it is better to hold it throughout the entire
> operation.

It's not really a problem as there's nothing that can happen in
between these two calls that can cause a problem but won't be a
problem if it happens after these two calls. I was trying to avoid
repeating the purge supplier/consumer code here again. Can we leave
this as is for now?

>
> > +       fwnode_links_purge_consumers(fwnode);
>
> I'd get rid of the two functions above, add something like
> fwnode_link_del() and walk the lists directly here calling it for
> every link on the way.

I need a fwnode_links_purge_suppliers() (as in, not purging consumer
links) though. I used it later in the series. So instead of repeating
that code for fwnode_links_purge(), I created
fwnode_links_purge_consumers() and called both functions from here.
Can we leave this as is?

-Saravana

>
> > +}
> >
> >  #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 593fb8e58f21..afde643f37a2 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;
> >  };
> >
> >  /**
> > @@ -173,8 +183,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.1.341.ge80a0c044ae-goog
> >

[PATCH v1 09/18] 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 1a1d9a55645c..4a0907574646 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -646,6 +646,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.1.341.ge80a0c044ae-goog

Re: [PATCH v1 09/18] driver core: Allow only unprobed consumers for SYNC_STATE_ONLY device links

[Rafael J. Wysocki]

On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
>
> 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.

I'm wondering why this needs to be part of the series?

It looks like it could go in separately, couldn't it?

>
> 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 1a1d9a55645c..4a0907574646 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -646,6 +646,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;
> +       }

Returning NULL at this point may be confusing if there is a link
between these devices already.

> +
>         /*
>          * DL_FLAG_AUTOREMOVE_SUPPLIER indicates that the link will be needed
>          * longer than for DL_FLAG_AUTOREMOVE_CONSUMER and setting them both
> --
> 2.29.1.341.ge80a0c044ae-goog
>

Re: [PATCH v1 09/18] driver core: Allow only unprobed consumers for SYNC_STATE_ONLY device links

[Saravana Kannan]

On Mon, Nov 16, 2020 at 7:57 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > 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.
>
> I'm wondering why this needs to be part of the series?
>
> It looks like it could go in separately, couldn't it?

Right, I just wrote this as part of the series as I noticed this gap
in the error checking as I wrote this series. It can go in separately.

>
> >
> > 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 1a1d9a55645c..4a0907574646 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -646,6 +646,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;
> > +       }
>
> Returning NULL at this point may be confusing if there is a link
> between these devices already.

But the request is for a SYNC_STATE_ONLY link that can't be created
when this condition is met. I see it similar to the error check above.

I think returning the existing non-SYNC_STATE_ONLY link gives the
wrong impression that the link was created successfully. Also, if I
find the existing link and return it, then I need to refcount it
(conditional on STATELESS?) and
the caller who shouldn't be trying to create this link should now need
to keep track of this and release it too. I think it's cleaner and
simpler to just return NULL.


-Saravana

[PATCH v1 10/18] device property: Add fwnode_is_ancestor_of()

[Saravana Kannan]

Add a helper function to check if a fwnode is an ancestor of another
fwnode. This will be useful for fw_devlink feature.

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

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 4c43d30145c6..2569ebd52e6b 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -660,6 +660,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..2e5eed3ddf1c 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -88,6 +88,8 @@ struct fwnode_handle *fwnode_get_next_parent(
 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.1.341.ge80a0c044ae-goog

[PATCH v1 11/18] 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>
---
 include/linux/fwnode.h | 42 +++---------------------------------------
 1 file changed, 3 insertions(+), 39 deletions(-)

diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index afde643f37a2..ec02e1e939cc 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.1.341.ge80a0c044ae-goog

Re: [PATCH v1 11/18] driver core: Redefine the meaning of fwnode_operations.add_links()

[Rafael J. Wysocki]

On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
>
> 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>
> ---
>  include/linux/fwnode.h | 42 +++---------------------------------------
>  1 file changed, 3 insertions(+), 39 deletions(-)
>
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index afde643f37a2..ec02e1e939cc 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.

I'd say something like "Create fwnode links to all nodes that
represent devices supplying resources to the device represented by the
current fwnode.  Return ..., or a negative ... on failure."

>   */
>  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.1.341.ge80a0c044ae-goog
>

Re: [PATCH v1 11/18] driver core: Redefine the meaning of fwnode_operations.add_links()

[Saravana Kannan]

On Mon, Nov 16, 2020 at 8:16 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > 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>
> > ---
> >  include/linux/fwnode.h | 42 +++---------------------------------------
> >  1 file changed, 3 insertions(+), 39 deletions(-)
> >
> > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > index afde643f37a2..ec02e1e939cc 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.
>
> I'd say something like "Create fwnode links to all nodes that
> represent devices supplying resources to the device represented by the
> current fwnode.  Return ..., or a negative ... on failure."

I don't have a strong opinion about this, but want to clarify that I'm
intentionally choosing not to say "device" because not all fwnodes
will have devices created for them. Do you still want me to make this
change?


>
> >   */
> >  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.1.341.ge80a0c044ae-goog
> >

[PATCH v1 12/18] 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 |  3 +++
 2 files changed, 22 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4a0907574646..ee28d8c7ee85 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1543,6 +1543,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 ec02e1e939cc..9aaf9e4f3994 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -15,12 +15,15 @@
 struct fwnode_operations;
 struct device;
 
+#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;
+	u32 flags;
 };
 
 struct fwnode_link {
-- 
2.29.1.341.ge80a0c044ae-goog

Re: [PATCH v1 12/18] driver core: Add fw_devlink_parse_fwtree()

[Rafael J. Wysocki]

On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
>
> 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 |  3 +++
>  2 files changed, 22 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4a0907574646..ee28d8c7ee85 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1543,6 +1543,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;

Why is the flag needed?

Duplicate links won't be created anyway and it doesn't cause the tree
walk to be terminated.

> +
> +       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)))

I'd prefer

for (child = NULL; child; 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 ec02e1e939cc..9aaf9e4f3994 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -15,12 +15,15 @@
>  struct fwnode_operations;
>  struct device;
>

Description here, please.

> +#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;
> +       u32 flags;

That's a bit wasteful.  Maybe u8 would suffice for the time being?

>  };
>
>  struct fwnode_link {
> --
> 2.29.1.341.ge80a0c044ae-goog
>

Re: [PATCH v1 12/18] driver core: Add fw_devlink_parse_fwtree()

[Saravana Kannan]

On Mon, Nov 16, 2020 at 8:25 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > 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 |  3 +++
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 4a0907574646..ee28d8c7ee85 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1543,6 +1543,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;
>
> Why is the flag needed?
>
> Duplicate links won't be created anyway and it doesn't cause the tree
> walk to be terminated.

To avoid parsing a fwnode more than once. The cumulative impact of the
repeated parsing is actually quite high.

And I intentionally didn't do this check at the tree walk level
because DT overlay can add/remove/change individual fwnodes and I want
to reparse those when they are added while avoiding parsing other
nodes that have already been parsed and not changed by DT overlay.

>
> > +
> > +       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)))
>
> I'd prefer
>
> for (child = NULL; child; child =
> fwnode_get_next_available_child_node(fwnode, child))

I was about to change to this and then realized it won't work. It
would have to be

for (child = fwnode_get_next_available_child_node(fwnode, NULL));
       child;
       child = fwnode_get_next_available_child_node(fwnode, child))

Is that really better? The while() seems a lot more readable to me. I
don't have a strong opinion, so I'll go with whatever you say after
reading this.

>
> > +               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 ec02e1e939cc..9aaf9e4f3994 100644
> > --- a/include/linux/fwnode.h
> > +++ b/include/linux/fwnode.h
> > @@ -15,12 +15,15 @@
> >  struct fwnode_operations;
> >  struct device;
> >
>
> Description here, please.

Ack

>
> > +#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;
> > +       u32 flags;
>
> That's a bit wasteful.  Maybe u8 would suffice for the time being?

Ack.


-Saravana

[PATCH v1 13/18] driver core: Add fwnode_get_next_parent_dev() helper function

[Saravana Kannan]

Given a fwnode, this function finds the closest ancestor fwnode that has
a corresponding struct device. The function returns this struct device.
This function will be used in a subsequent patch in this series.

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index ee28d8c7ee85..4ae5f2885ac5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1562,6 +1562,31 @@ static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode)
 		fw_devlink_parse_fwtree(child);
 }
 
+/**
+ * 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.
+ */
+static 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;
+}
+
 static void fw_devlink_link_device(struct device *dev)
 {
 	int fw_ret;
-- 
2.29.1.341.ge80a0c044ae-goog

Re: [PATCH v1 13/18] driver core: Add fwnode_get_next_parent_dev() helper function

[Rafael J. Wysocki]

On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
>
> Given a fwnode, this function finds the closest ancestor fwnode that has
> a corresponding struct device. The function returns this struct device.
> This function will be used in a subsequent patch in this series.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>

I would combine this one with patch [10/18].

> ---
>  drivers/base/core.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index ee28d8c7ee85..4ae5f2885ac5 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1562,6 +1562,31 @@ static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode)
>                 fw_devlink_parse_fwtree(child);
>  }
>
> +/**
> + * 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.
> + */
> +static 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;
> +}
> +
>  static void fw_devlink_link_device(struct device *dev)
>  {
>         int fw_ret;
> --
> 2.29.1.341.ge80a0c044ae-goog
>

Re: [PATCH v1 13/18] driver core: Add fwnode_get_next_parent_dev() helper function

[Saravana Kannan]

On Mon, Nov 16, 2020 at 8:27 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Given a fwnode, this function finds the closest ancestor fwnode that has
> > a corresponding struct device. The function returns this struct device.
> > This function will be used in a subsequent patch in this series.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> I would combine this one with patch [10/18].

Ack.

-Saravana




-Saravana

>
> > ---
> >  drivers/base/core.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index ee28d8c7ee85..4ae5f2885ac5 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1562,6 +1562,31 @@ static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode)
> >                 fw_devlink_parse_fwtree(child);
> >  }
> >
> > +/**
> > + * 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.
> > + */
> > +static 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;
> > +}
> > +
> >  static void fw_devlink_link_device(struct device *dev)
> >  {
> >         int fw_ret;
> > --
> > 2.29.1.341.ge80a0c044ae-goog
> >

[PATCH v1 14/18] 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 4ae5f2885ac5..d51dd564add1 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.
@@ -994,13 +995,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();
 
@@ -1166,10 +1167,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);
 }
@@ -2226,7 +2224,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.1.341.ge80a0c044ae-goog

Re: [PATCH v1 14/18] driver core: Use device's fwnode to check if it is waiting for suppliers

[Rafael J. Wysocki]

On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> 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.

My kind of educated guess is that you want to drop
waiting_for_suppliers and that's why you want to use supplier links
here.

>
> 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 4ae5f2885ac5..d51dd564add1 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.
> @@ -994,13 +995,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();
>
> @@ -1166,10 +1167,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);

Why isn't the lock needed any more?

Or maybe it wasn't needed previously too?

> +       val = !list_empty(&dev->fwnode->suppliers);
>         device_unlock(dev);
>         return sysfs_emit(buf, "%u\n", val);
>  }
> @@ -2226,7 +2224,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) {

And why is this change needed?

>                 error = device_create_file(dev, &dev_attr_waiting_for_supplier);
>                 if (error)
>                         goto err_remove_dev_online;
> --
> 2.29.1.341.ge80a0c044ae-goog
>

Re: [PATCH v1 14/18] driver core: Use device's fwnode to check if it is waiting for suppliers

[Saravana Kannan]

On Mon, Nov 16, 2020 at 8:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> 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.
>
> My kind of educated guess is that you want to drop
> waiting_for_suppliers and that's why you want to use supplier links
> here.

Yes, and a device would never be added waiting_for_suppliers list.

> >
> > 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 4ae5f2885ac5..d51dd564add1 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.
> > @@ -994,13 +995,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();
> >
> > @@ -1166,10 +1167,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);
>
> Why isn't the lock needed any more?
>
> Or maybe it wasn't needed previously too?

Yeah, I sent a separate patch for dropping this lock [1]. But I didn't
want to wait for that to land to write this series. The lock wasn't
needed in the first place and it was causing a lockdep warning.

>
> > +       val = !list_empty(&dev->fwnode->suppliers);
> >         device_unlock(dev);
> >         return sysfs_emit(buf, "%u\n", val);
> >  }
> > @@ -2226,7 +2224,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) {
>
> And why is this change needed?

Because if a device doesn't have a fwnode, it can't ever be waiting on
a supplier. Also, the "show" function dereferences
dev->fwnode->suppliers.

-Saravana

[1] - https://lore.kernel.org/lkml/20201104205431.3795207-1-saravanak@google.com/
Ignore the 1/2 thing. There's only 1 relevant patch.

[PATCH v1 15/18] 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..86303803f1b3 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 (unlikely(!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.1.341.ge80a0c044ae-goog

Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links

[Greg Kroah-Hartman]

On Wed, Nov 04, 2020 at 03:23:52PM -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(-)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 408a7b5f06a9..86303803f1b3 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);

Who is calling this function without a valid dev pointer?

>  		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);

Why not dev_dbg() here?  dev should be valid, correct?


>  		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);

How is dev not valid here?  sup_dev might not be, but dev should be.


>  		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 (unlikely(!con_np))
> +		return -EINVAL;

Can you prove that unlikely() results in faster code?  If not, don't use
it.

And the only way it can be NULL is if fwnode is NULL, and as you control
the callers to it, how can that be the case?

thanks,

greg k-h

Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links

[Saravana Kannan]

On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Nov 04, 2020 at 03:23:52PM -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(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 408a7b5f06a9..86303803f1b3 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);
>
> Who is calling this function without a valid dev pointer?

Sorry, I plan to delete the "dev" parameter as it's not really used
anywhere. I'm trying to do that without causing build time errors and
making the series into digestible small patches.

I can do the deletion of the parameter as a Patch 19/19. Will that work?

> >               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);
>
> Why not dev_dbg() here?  dev should be valid, correct?

Responded above.

> >               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);
>
> How is dev not valid here?  sup_dev might not be, but dev should be.

Answered above.

>
>
> >               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 (unlikely(!con_np))
> > +             return -EINVAL;
>
> Can you prove that unlikely() results in faster code?  If not, don't use
> it.

Ok will delete it.

> And the only way it can be NULL is if fwnode is NULL, and as you control
> the callers to it, how can that be the case?

fwnode represents a generic firmware node. The to_of_node() returns
NULL if fwnode is not a DT node. So con_np can be NULL if that
happens. That's why we need a NULL check here.  With the current code,
that can never happen, bit I think it doesn't hurt to check in case
there's a buggy caller. I don't have a strong opinion - so I can do it
whichever way.

-Saravana

Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links

[Saravana Kannan]

On Thu, Nov 5, 2020 at 3:25 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Nov 04, 2020 at 03:23:52PM -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(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 408a7b5f06a9..86303803f1b3 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);
> >
> > Who is calling this function without a valid dev pointer?
>
> Sorry, I plan to delete the "dev" parameter as it's not really used
> anywhere. I'm trying to do that without causing build time errors and
> making the series into digestible small patches.

*facepalm* for my earlier response. You'll notice that I've already
deleted the "dev" input param to this function. That's why I can't use
it here :)

-Saravana

Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links

[Greg Kroah-Hartman]

On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote:
> On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Nov 04, 2020 at 03:23:52PM -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(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 408a7b5f06a9..86303803f1b3 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);
> >
> > Who is calling this function without a valid dev pointer?
> 
> Sorry, I plan to delete the "dev" parameter as it's not really used
> anywhere. I'm trying to do that without causing build time errors and
> making the series into digestible small patches.
> 
> I can do the deletion of the parameter as a Patch 19/19. Will that work?

That's fine, but why get rid of dev?  The driver core works on these
things, and we want errors/messages/warnings to spit out what device is
causing those issues.  It is fine to drag around a struct device pointer
just for messages, that's to be expected, and is good.

> > And the only way it can be NULL is if fwnode is NULL, and as you control
> > the callers to it, how can that be the case?
> 
> fwnode represents a generic firmware node. The to_of_node() returns
> NULL if fwnode is not a DT node. So con_np can be NULL if that
> happens. That's why we need a NULL check here.  With the current code,
> that can never happen, bit I think it doesn't hurt to check in case
> there's a buggy caller. I don't have a strong opinion - so I can do it
> whichever way.

If it can't happen, no need to check for it :)

thanks,

greg k-h

Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links

[Saravana Kannan]

On Thu, Nov 5, 2020 at 11:22 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote:
> > On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Nov 04, 2020 at 03:23:52PM -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(-)
> > > >
> > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > index 408a7b5f06a9..86303803f1b3 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);
> > >
> > > Who is calling this function without a valid dev pointer?
> >
> > Sorry, I plan to delete the "dev" parameter as it's not really used
> > anywhere. I'm trying to do that without causing build time errors and
> > making the series into digestible small patches.
> >
> > I can do the deletion of the parameter as a Patch 19/19. Will that work?
>
> That's fine, but why get rid of dev?  The driver core works on these
> things, and we want errors/messages/warnings to spit out what device is
> causing those issues.  It is fine to drag around a struct device pointer
> just for messages, that's to be expected, and is good.

In general I agree. If the fwnode being parsed is related to the dev,
it's nice to have the dev name in the logs.

But in this instance I feel it's analogous to printing the PID that's
parsing the fwnode -- kinda irrelevant. The device in question can
appear very random and it'll just cause more confusion than be of help
if it shows up in the logs.

For example it can be something like:
<gpio device name>: linking <wifi fwnode> to <iommu fwnode>

If the device was actually that of the wifi fwnode of the iommu
fwnode, I agree it'll be useful to carry around the dev even if it's
just for logs.

Hope that makes sense.

> > > And the only way it can be NULL is if fwnode is NULL, and as you control
> > > the callers to it, how can that be the case?
> >
> > fwnode represents a generic firmware node. The to_of_node() returns
> > NULL if fwnode is not a DT node. So con_np can be NULL if that
> > happens. That's why we need a NULL check here.  With the current code,
> > that can never happen, bit I think it doesn't hurt to check in case
> > there's a buggy caller. I don't have a strong opinion - so I can do it
> > whichever way.
>
> If it can't happen, no need to check for it :)

I don't have a strong opinion, so I can delete it if you insist.

-Saravana

Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links

[Greg Kroah-Hartman]

On Thu, Nov 05, 2020 at 11:41:20PM -0800, Saravana Kannan wrote:
> On Thu, Nov 5, 2020 at 11:22 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote:
> > > On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Nov 04, 2020 at 03:23:52PM -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(-)
> > > > >
> > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > > index 408a7b5f06a9..86303803f1b3 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);
> > > >
> > > > Who is calling this function without a valid dev pointer?
> > >
> > > Sorry, I plan to delete the "dev" parameter as it's not really used
> > > anywhere. I'm trying to do that without causing build time errors and
> > > making the series into digestible small patches.
> > >
> > > I can do the deletion of the parameter as a Patch 19/19. Will that work?
> >
> > That's fine, but why get rid of dev?  The driver core works on these
> > things, and we want errors/messages/warnings to spit out what device is
> > causing those issues.  It is fine to drag around a struct device pointer
> > just for messages, that's to be expected, and is good.
> 
> In general I agree. If the fwnode being parsed is related to the dev,
> it's nice to have the dev name in the logs.
> 
> But in this instance I feel it's analogous to printing the PID that's
> parsing the fwnode -- kinda irrelevant. The device in question can
> appear very random and it'll just cause more confusion than be of help
> if it shows up in the logs.
> 
> For example it can be something like:
> <gpio device name>: linking <wifi fwnode> to <iommu fwnode>
> 
> If the device was actually that of the wifi fwnode of the iommu
> fwnode, I agree it'll be useful to carry around the dev even if it's
> just for logs.
> 
> Hope that makes sense.

Not really, as the device here should be the one that is doing the
linking, so why doesn't that matter?  It shouldn't be confusing as this
is what the DT asks to have happen, so reflecting that in the log if an
error, or debugging, wants it should be helpful, not confusing.

thanks,

greg k-h

Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links

[Saravana Kannan]

On Thu, Nov 5, 2020 at 11:51 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Nov 05, 2020 at 11:41:20PM -0800, Saravana Kannan wrote:
> > On Thu, Nov 5, 2020 at 11:22 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote:
> > > > On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Wed, Nov 04, 2020 at 03:23:52PM -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(-)
> > > > > >
> > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > > > index 408a7b5f06a9..86303803f1b3 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);
> > > > >
> > > > > Who is calling this function without a valid dev pointer?
> > > >
> > > > Sorry, I plan to delete the "dev" parameter as it's not really used
> > > > anywhere. I'm trying to do that without causing build time errors and
> > > > making the series into digestible small patches.
> > > >
> > > > I can do the deletion of the parameter as a Patch 19/19. Will that work?
> > >
> > > That's fine, but why get rid of dev?  The driver core works on these
> > > things, and we want errors/messages/warnings to spit out what device is
> > > causing those issues.  It is fine to drag around a struct device pointer
> > > just for messages, that's to be expected, and is good.
> >
> > In general I agree. If the fwnode being parsed is related to the dev,
> > it's nice to have the dev name in the logs.
> >
> > But in this instance I feel it's analogous to printing the PID that's
> > parsing the fwnode -- kinda irrelevant. The device in question can
> > appear very random and it'll just cause more confusion than be of help
> > if it shows up in the logs.
> >
> > For example it can be something like:
> > <gpio device name>: linking <wifi fwnode> to <iommu fwnode>
> >
> > If the device was actually that of the wifi fwnode of the iommu
> > fwnode, I agree it'll be useful to carry around the dev even if it's
> > just for logs.
> >
> > Hope that makes sense.
>
> Not really, as the device here should be the one that is doing the
> linking, so why doesn't that matter?

No, the links being created here are fwnode links. Also, when a fwnode
is parsed (because the device corresponding to it is added), the whole
fwnode tree under it is also parsed. So the device in question won't
be either the consumer or the supplier when the log is printed. So the
device will just be some random ancestor up the parent chain.

-Saravana

[PATCH v1 16/18] 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 | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/firmware/efi/efi-init.c b/drivers/firmware/efi/efi-init.c
index b148f1459fb3..c0c3d4c3837a 100644
--- a/drivers/firmware/efi/efi-init.c
+++ b/drivers/firmware/efi/efi-init.c
@@ -316,11 +316,10 @@ 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;
-	struct device *sup_dev;
 
 	sup_np = find_pci_overlap_node();
 
@@ -331,27 +330,9 @@ static int efifb_add_links(const 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.1.341.ge80a0c044ae-goog

Re: [PATCH v1 16/18] efi: Update implementation of add_links() to create fwnode links

[Greg Kroah-Hartman]

On Wed, Nov 04, 2020 at 03:23:53PM -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/firmware/efi/efi-init.c | 23 ++---------------------
>  1 file changed, 2 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi-init.c b/drivers/firmware/efi/efi-init.c
> index b148f1459fb3..c0c3d4c3837a 100644
> --- a/drivers/firmware/efi/efi-init.c
> +++ b/drivers/firmware/efi/efi-init.c
> @@ -316,11 +316,10 @@ 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)

So you are fixing the build warning you added a few patches ago here?
Please fix up the function signatures when you made that change, not
here later on.

thanks,

greg k-h

Re: [PATCH v1 16/18] efi: Update implementation of add_links() to create fwnode links

[Saravana Kannan]

On Thu, Nov 5, 2020 at 1:42 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Nov 04, 2020 at 03:23:53PM -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/firmware/efi/efi-init.c | 23 ++---------------------
> >  1 file changed, 2 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi-init.c b/drivers/firmware/efi/efi-init.c
> > index b148f1459fb3..c0c3d4c3837a 100644
> > --- a/drivers/firmware/efi/efi-init.c
> > +++ b/drivers/firmware/efi/efi-init.c
> > @@ -316,11 +316,10 @@ 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)
>
> So you are fixing the build warning you added a few patches ago here?
> Please fix up the function signatures when you made that change, not
> here later on.

I'm trying not to have a mega patcht that changes a lot of code.

I guess I can drop this "const" diff from this patch and then merge it
with the earlier patch that removes the const. But still leave the
rest of the changes in this patch as is. Does that work for you?

-Saravana

Re: [PATCH v1 16/18] efi: Update implementation of add_links() to create fwnode links

[Greg Kroah-Hartman]

On Thu, Nov 05, 2020 at 03:27:52PM -0800, Saravana Kannan wrote:
> On Thu, Nov 5, 2020 at 1:42 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Nov 04, 2020 at 03:23:53PM -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/firmware/efi/efi-init.c | 23 ++---------------------
> > >  1 file changed, 2 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/efi-init.c b/drivers/firmware/efi/efi-init.c
> > > index b148f1459fb3..c0c3d4c3837a 100644
> > > --- a/drivers/firmware/efi/efi-init.c
> > > +++ b/drivers/firmware/efi/efi-init.c
> > > @@ -316,11 +316,10 @@ 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)
> >
> > So you are fixing the build warning you added a few patches ago here?
> > Please fix up the function signatures when you made that change, not
> > here later on.
> 
> I'm trying not to have a mega patcht that changes a lot of code.
> 
> I guess I can drop this "const" diff from this patch and then merge it
> with the earlier patch that removes the const. But still leave the
> rest of the changes in this patch as is. Does that work for you?

Yes, that's fine, you just can't add build warnings along the way.

thanks,

greg k-h

[PATCH v1 17/18] driver core: Add helper functions to convert fwnode links to device links

[Saravana Kannan]

Add helper functions __fw_devlink_link_to_consumers() and
__fw_devlink_link_to_suppliers() that convert fwnode links to device
links.

__fw_devlink_link_to_consumers() is for creating:
- Device links between a newly added device and all its consumer devices
  that have been added to driver core.
- Proxy SYNC_STATE_ONLY device links between the newly added device and
  the parent devices of all its consumers that have not been added to
  driver core yet.

__fw_devlink_link_to_suppliers() is for creating:
- Device links between a newly added device and all its supplier devices
- Proxy SYNC_STATE_ONLY device links between the newly added device and
  all the supplier devices of its child device nodes.

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d51dd564add1..0c87ff949d81 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1585,6 +1585,234 @@ static struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
 	return dev;
 }
 
+/**
+ * fw_devlink_create_devlink - Create a device link from a consumer to fwnode
+ * @con - Consumer device for the device link
+ * @sup - fwnode handle of supplier
+ *
+ * This function will try to create a device link between the consumer and the
+ * supplier devices.
+ *
+ * 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 a device link being
+ * created in invalid.
+ *
+ * Returns,
+ * 0 on successfully creating a device link
+ * -EINVAL if the device link being attempted is invalid
+ * -EAGAIN if the device link needs to be attempted again in the future
+ */
+static int fw_devlink_create_devlink(struct device *con,
+				     struct fwnode_handle *sup, u32 flags)
+{
+	struct device *sup_dev, *sup_par_dev;
+	int ret = 0;
+
+	sup_dev = get_dev_from_fwnode(sup);
+	/*
+	 * If we can't find the supplier device from its fwnode, it might be
+	 * due to a cyclic dependcy 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 (!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 (flags & DL_FLAG_SYNC_STATE_ONLY)
+			return -EAGAIN;
+
+		sup_par_dev = fwnode_get_next_parent_dev(sup);
+
+		/*
+		 * 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.
+		 */
+		if (sup_par_dev && device_is_dependent(con, sup_par_dev)) {
+			dev_dbg(con, "Not linking to %pfwP - False link\n",
+				sup);
+			ret = -EINVAL;
+		} else {
+			/*
+			 * Can't check for cycles or no cycles. So let's try
+			 * again later.
+			 */
+			ret = -EAGAIN;
+		}
+
+		put_device(sup_par_dev);
+		return ret;
+	}
+
+	/*
+	 * If we get this far and fail, this 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;
+	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. See
+ * fw_devlink_create_devlink() for more details.
+ *
+ * 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.
+ *
+ * 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)
+{
+	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;
+
+	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;
+
+		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)
 {
 	int fw_ret;
-- 
2.29.1.341.ge80a0c044ae-goog

Re: [PATCH v1 17/18] driver core: Add helper functions to convert fwnode links to device links

[Greg Kroah-Hartman]

On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote:
> Add helper functions __fw_devlink_link_to_consumers() and
> __fw_devlink_link_to_suppliers() that convert fwnode links to device
> links.
> 
> __fw_devlink_link_to_consumers() is for creating:
> - Device links between a newly added device and all its consumer devices
>   that have been added to driver core.
> - Proxy SYNC_STATE_ONLY device links between the newly added device and
>   the parent devices of all its consumers that have not been added to
>   driver core yet.
> 
> __fw_devlink_link_to_suppliers() is for creating:
> - Device links between a newly added device and all its supplier devices
> - Proxy SYNC_STATE_ONLY device links between the newly added device and
>   all the supplier devices of its child device nodes.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>

Did you just add build warnings with these static functions that no one
calls?

thanks,

greg k-h

Re: [PATCH v1 17/18] driver core: Add helper functions to convert fwnode links to device links

[Saravana Kannan]

On Thu, Nov 5, 2020 at 1:43 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote:
> > Add helper functions __fw_devlink_link_to_consumers() and
> > __fw_devlink_link_to_suppliers() that convert fwnode links to device
> > links.
> >
> > __fw_devlink_link_to_consumers() is for creating:
> > - Device links between a newly added device and all its consumer devices
> >   that have been added to driver core.
> > - Proxy SYNC_STATE_ONLY device links between the newly added device and
> >   the parent devices of all its consumers that have not been added to
> >   driver core yet.
> >
> > __fw_devlink_link_to_suppliers() is for creating:
> > - Device links between a newly added device and all its supplier devices
> > - Proxy SYNC_STATE_ONLY device links between the newly added device and
> >   all the supplier devices of its child device nodes.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> Did you just add build warnings with these static functions that no one
> calls?

The next patch in this series uses it. I'm just splitting it up into a
separate patch so that it's digestible and I can provide more details
in the commit text.

Couple of options:
1. Drop the static in this patch and add it back when it's used in patch 18/18.
2. Drop the commit text and squash this with 18/18 if you think the
function documentation is clear enough and it won't make patch 18/18
too hard to review.

Please let me know which one you'd prefer or if you have other
options. I don't have a strong opinion on how the patches are split up
as long as it's easy for the reviewers and future folks who look at
git log to understand stuff.

-Saravana

Re: [PATCH v1 17/18] driver core: Add helper functions to convert fwnode links to device links

[Greg Kroah-Hartman]

On Thu, Nov 05, 2020 at 03:32:05PM -0800, Saravana Kannan wrote:
> On Thu, Nov 5, 2020 at 1:43 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote:
> > > Add helper functions __fw_devlink_link_to_consumers() and
> > > __fw_devlink_link_to_suppliers() that convert fwnode links to device
> > > links.
> > >
> > > __fw_devlink_link_to_consumers() is for creating:
> > > - Device links between a newly added device and all its consumer devices
> > >   that have been added to driver core.
> > > - Proxy SYNC_STATE_ONLY device links between the newly added device and
> > >   the parent devices of all its consumers that have not been added to
> > >   driver core yet.
> > >
> > > __fw_devlink_link_to_suppliers() is for creating:
> > > - Device links between a newly added device and all its supplier devices
> > > - Proxy SYNC_STATE_ONLY device links between the newly added device and
> > >   all the supplier devices of its child device nodes.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> >
> > Did you just add build warnings with these static functions that no one
> > calls?
> 
> The next patch in this series uses it. I'm just splitting it up into a
> separate patch so that it's digestible and I can provide more details
> in the commit text.

But you can not add build warnings, you know this :)

> Couple of options:
> 1. Drop the static in this patch and add it back when it's used in patch 18/18.
> 2. Drop the commit text and squash this with 18/18 if you think the
> function documentation is clear enough and it won't make patch 18/18
> too hard to review.

It is hard to review new functions when you do not see them being used,
otherwise you have to flip back and forth between patches, which is
difficult.

Add the functions, and use them, in the same patch.  Otherwise we have
no idea _HOW_ you are using them, or even if you end up using them at
all.

thanks,

greg k-h

Re: [PATCH v1 17/18] driver core: Add helper functions to convert fwnode links to device links

[Saravana Kannan]

On Thu, Nov 5, 2020 at 11:23 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Nov 05, 2020 at 03:32:05PM -0800, Saravana Kannan wrote:
> > On Thu, Nov 5, 2020 at 1:43 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote:
> > > > Add helper functions __fw_devlink_link_to_consumers() and
> > > > __fw_devlink_link_to_suppliers() that convert fwnode links to device
> > > > links.
> > > >
> > > > __fw_devlink_link_to_consumers() is for creating:
> > > > - Device links between a newly added device and all its consumer devices
> > > >   that have been added to driver core.
> > > > - Proxy SYNC_STATE_ONLY device links between the newly added device and
> > > >   the parent devices of all its consumers that have not been added to
> > > >   driver core yet.
> > > >
> > > > __fw_devlink_link_to_suppliers() is for creating:
> > > > - Device links between a newly added device and all its supplier devices
> > > > - Proxy SYNC_STATE_ONLY device links between the newly added device and
> > > >   all the supplier devices of its child device nodes.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > >
> > > Did you just add build warnings with these static functions that no one
> > > calls?
> >
> > The next patch in this series uses it. I'm just splitting it up into a
> > separate patch so that it's digestible and I can provide more details
> > in the commit text.
>
> But you can not add build warnings, you know this :)

I know I can't break the build because that'll screw git bisect. But I
thought warning that's fixed later in the series might be okay if it
helps readability. I know now :)
>
> > Couple of options:
> > 1. Drop the static in this patch and add it back when it's used in patch 18/18.
> > 2. Drop the commit text and squash this with 18/18 if you think the
> > function documentation is clear enough and it won't make patch 18/18
> > too hard to review.
>
> It is hard to review new functions when you do not see them being used,
> otherwise you have to flip back and forth between patches, which is
> difficult.
>
> Add the functions, and use them, in the same patch.  Otherwise we have
> no idea _HOW_ you are using them, or even if you end up using them at
> all.

Sounds good. I'll squash them.

-Saravana

Re: [PATCH v1 17/18] driver core: Add helper functions to convert fwnode links to device links

[Rafael J. Wysocki]

On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
>
> Add helper functions __fw_devlink_link_to_consumers() and
> __fw_devlink_link_to_suppliers() that convert fwnode links to device
> links.
>
> __fw_devlink_link_to_consumers() is for creating:
> - Device links between a newly added device and all its consumer devices
>   that have been added to driver core.
> - Proxy SYNC_STATE_ONLY device links between the newly added device and
>   the parent devices of all its consumers that have not been added to
>   driver core yet.
>
> __fw_devlink_link_to_suppliers() is for creating:
> - Device links between a newly added device and all its supplier devices
> - Proxy SYNC_STATE_ONLY device links between the newly added device and
>   all the supplier devices of its child device nodes.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c | 228 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 228 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d51dd564add1..0c87ff949d81 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1585,6 +1585,234 @@ static struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
>         return dev;
>  }
>
> +/**
> + * fw_devlink_create_devlink - Create a device link from a consumer to fwnode

Have you considered renaming "fw_devlink" to "fwnode_link"?

That would be much less confusing IMO and the name of this function
would be clearer.

> + * @con - Consumer device for the device link
> + * @sup - fwnode handle of supplier
> + *
> + * This function will try to create a device link between the consumer and the
> + * supplier devices.

"... between consumer device @con and the supplier device represented
by @sup" (and see below).

> + *
> + * 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 a device link being
> + * created in invalid.

"... 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 being attempted is invalid

"if the device link cannot be created as expected"

> + * -EAGAIN if the device link needs to be attempted again in the future

"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, u32 flags)

I'd call the second arg sup_handle to indicate that it is not a struct device.

> +{
> +       struct device *sup_dev, *sup_par_dev;
> +       int ret = 0;
> +
> +       sup_dev = get_dev_from_fwnode(sup);
> +       /*
> +        * If we can't find the supplier device from its fwnode, it might be
> +        * due to a cyclic dependcy between fwnodes. Some of these cycles can

dependency

> +        * be broken by applying logic. Check for these types of cycles and
> +        * break them so that devices in the cycle probe properly.
> +        */

I would do

if (sup_dev) {
        if (!device_link_add(con, sup_dev, flags))
                ret = -EINVAL;

        put_device(sup_dev);
        return ret;
}

here and the cycle detection (error code path) below, possibly using
the same dev pointer.

> +       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 (flags & DL_FLAG_SYNC_STATE_ONLY)
> +                       return -EAGAIN;
> +
> +               sup_par_dev = fwnode_get_next_parent_dev(sup);
> +
> +               /*
> +                * 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.
> +                */
> +               if (sup_par_dev && device_is_dependent(con, sup_par_dev)) {
> +                       dev_dbg(con, "Not linking to %pfwP - False link\n",
> +                               sup);
> +                       ret = -EINVAL;
> +               } else {
> +                       /*
> +                        * Can't check for cycles or no cycles. So let's try
> +                        * again later.
> +                        */
> +                       ret = -EAGAIN;
> +               }
> +
> +               put_device(sup_par_dev);
> +               return ret;
> +       }
> +
> +       /*
> +        * If we get this far and fail, this 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;
> +       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. See
> + * fw_devlink_create_devlink() for more details.
> + *
> + * 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.
> + *
> + * 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)
> +{
> +       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;
> +
> +       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;
> +
> +               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)
>  {
>         int fw_ret;
> --
> 2.29.1.341.ge80a0c044ae-goog
>

Re: [PATCH v1 17/18] driver core: Add helper functions to convert fwnode links to device links

[Saravana Kannan]

On Mon, Nov 16, 2020 at 8:57 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Add helper functions __fw_devlink_link_to_consumers() and
> > __fw_devlink_link_to_suppliers() that convert fwnode links to device
> > links.
> >
> > __fw_devlink_link_to_consumers() is for creating:
> > - Device links between a newly added device and all its consumer devices
> >   that have been added to driver core.
> > - Proxy SYNC_STATE_ONLY device links between the newly added device and
> >   the parent devices of all its consumers that have not been added to
> >   driver core yet.
> >
> > __fw_devlink_link_to_suppliers() is for creating:
> > - Device links between a newly added device and all its supplier devices
> > - Proxy SYNC_STATE_ONLY device links between the newly added device and
> >   all the supplier devices of its child device nodes.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c | 228 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 228 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index d51dd564add1..0c87ff949d81 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1585,6 +1585,234 @@ static struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
> >         return dev;
> >  }
> >
> > +/**
> > + * fw_devlink_create_devlink - Create a device link from a consumer to fwnode
>
> Have you considered renaming "fw_devlink" to "fwnode_link"?

I avoided using fwnode_link prefix because it's not related to the
fwnode link APIs. This is a fw_devlink feature related code and hence
the fw_devlink prefix.

I could just call it fw_devlink_create() or fw_devlink_create_links()
or fwnode_to_dev_link() if that's less confusing. Any preferences?

> That would be much less confusing IMO and the name of this function
> would be clearer.
>
> > + * @con - Consumer device for the device link
> > + * @sup - fwnode handle of supplier
> > + *
> > + * This function will try to create a device link between the consumer and the
> > + * supplier devices.
>
> "... between consumer device @con and the supplier device represented
> by @sup" (and see below).
>
> > + *
> > + * 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 a device link being
> > + * created in invalid.
>
> "... 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 being attempted is invalid
>
> "if the device link cannot be created as expected"
>
> > + * -EAGAIN if the device link needs to be attempted again in the future
>
> "if the device link cannot be created right now, but it may be
> possible to do that in the future."

Ack to all the documentation comments above.

>
> > + */
> > +static int fw_devlink_create_devlink(struct device *con,
> > +                                    struct fwnode_handle *sup, u32 flags)
>
> I'd call the second arg sup_handle to indicate that it is not a struct device.

Ack

>
> > +{
> > +       struct device *sup_dev, *sup_par_dev;
> > +       int ret = 0;
> > +
> > +       sup_dev = get_dev_from_fwnode(sup);
> > +       /*
> > +        * If we can't find the supplier device from its fwnode, it might be
> > +        * due to a cyclic dependcy between fwnodes. Some of these cycles can
>
> dependency
>

Ack

> > +        * be broken by applying logic. Check for these types of cycles and
> > +        * break them so that devices in the cycle probe properly.
> > +        */
>
> I would do
>
> if (sup_dev) {
>         if (!device_link_add(con, sup_dev, flags))
>                 ret = -EINVAL;
>
>         put_device(sup_dev);
>         return ret;
> }
>
> here and the cycle detection (error code path) below, possibly using
> the same dev pointer.

Nice suggestion, thanks!


-Saravana

[PATCH v1 18/18] 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>
---
 drivers/base/core.c    | 98 +++++-------------------------------------
 include/linux/device.h |  5 ---
 2 files changed, 11 insertions(+), 92 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0c87ff949d81..d76ca59252fb 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);
@@ -800,74 +798,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)
 {
@@ -1194,9 +1124,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();
@@ -1487,10 +1416,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).
@@ -1815,17 +1740,17 @@ static void __fw_devlink_link_to_suppliers(struct device *dev,
 
 static void fw_devlink_link_device(struct device *dev)
 {
-	int fw_ret;
+	struct fwnode_handle *fwnode = dev->fwnode;
+
+	if (!fw_devlink_flags)
+		return;
 
-	device_link_add_missing_supplier_links();
+	fw_devlink_parse_fwtree(fwnode);
 
-	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);
-	}
+	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. */
@@ -2683,7 +2608,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.1.341.ge80a0c044ae-goog

Re: [PATCH v1 00/18] Refactor fw_devlink to significantly improve boot time

[Saravana Kannan]

On Wed, Nov 4, 2020 at 3:23 PM Saravana Kannan <saravanak@google.com> 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.
>
> Tomi/Laurent/Grygorii,
>
> If you can test this series, that'd be great!
>
> Thanks,
> Saravana
>
> [1] - https://lore.kernel.org/linux-pm/CAGETcx-aiW251dhEXT1GNb9bi6YcX8W=jLBrro5CnPuEjGL09g@mail.gmail.com/

Forgot the update this link in the cover letter. Here's a much better
link to the thread that discusses boot time regression:
https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/


-Saravana

Re: [PATCH v1 00/18] Refactor fw_devlink to significantly improve boot time

[Laurent Pinchart]

Hi Saravana,

Thank you for working on this !

On Wed, Nov 04, 2020 at 03:23:37PM -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.
> 
> Tomi/Laurent/Grygorii,
> 
> If you can test this series, that'd be great!

I gave it a try, rebasing my branch from v5.9 to v5.10-rc2 first. On
v5.10-rc2 the kernel dies when booting due to a deadlock (reported by
lockdep, so hopefully not too hard to debug). *sigh*. Fortunately, it
dies after the fw_devlink initialization, so I can still report results.

Before your series:

[    0.743065] cpuidle: using governor menu
[   13.350259] No ATAGs?

With your series applied:

[    0.722670] cpuidle: using governor menu
[    1.135859] No ATAGs?

That's a very clear improvement :-)

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> [1] - https://lore.kernel.org/linux-pm/CAGETcx-aiW251dhEXT1GNb9bi6YcX8W=jLBrro5CnPuEjGL09g@mail.gmail.com/
> 
> Saravana Kannan (18):
>   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()
>   driver core: Redefine the meaning of fwnode_operations.add_links()
>   driver core: Add fw_devlink_parse_fwtree()
>   driver core: Add fwnode_get_next_parent_dev() helper function
>   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: Add helper functions to convert fwnode links to device links
>   driver core: Refactor fw_devlink feature
> 
>  drivers/acpi/property.c         |   2 +-
>  drivers/acpi/scan.c             |   2 +-
>  drivers/base/core.c             | 584 +++++++++++++++++++++-----------
>  drivers/base/property.c         |  27 ++
>  drivers/base/swnode.c           |   2 +-
>  drivers/firmware/efi/efi-init.c |  31 +-
>  drivers/of/dynamic.c            |   1 +
>  drivers/of/platform.c           |   2 -
>  drivers/of/property.c           | 150 +++-----
>  include/linux/device.h          |  10 +-
>  include/linux/fwnode.h          |  66 ++--
>  include/linux/of.h              |   2 +-
>  include/linux/property.h        |   2 +
>  kernel/irq/irqdomain.c          |   2 +-
>  14 files changed, 490 insertions(+), 393 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 00/18] Refactor fw_devlink to significantly improve boot time

[Saravana Kannan]

On Thu, Nov 5, 2020 at 9:09 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Saravana,
>
> Thank you for working on this !
>
> On Wed, Nov 04, 2020 at 03:23:37PM -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.
> >
> > Tomi/Laurent/Grygorii,
> >
> > If you can test this series, that'd be great!
>
> I gave it a try, rebasing my branch from v5.9 to v5.10-rc2 first. On
> v5.10-rc2 the kernel dies when booting due to a deadlock (reported by
> lockdep, so hopefully not too hard to debug). *sigh*. Fortunately, it
> dies after the fw_devlink initialization, so I can still report results.

Phew! For a sec I thought you said fw_devlink was causing a deadlock.

>
> Before your series:
>
> [    0.743065] cpuidle: using governor menu
> [   13.350259] No ATAGs?
>
> With your series applied:
>
> [    0.722670] cpuidle: using governor menu
> [    1.135859] No ATAGs?
>
> That's a very clear improvement :-)

Thanks for testing. Great to hear it's helping!

> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'll add it to my v2 series.

-Saravana

Re: [PATCH v1 00/18] Refactor fw_devlink to significantly improve boot time

[Grygorii Strashko]

On 06/11/2020 10:36, Saravana Kannan wrote:
> On Thu, Nov 5, 2020 at 9:09 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Saravana,
>>
>> Thank you for working on this !
>>
>> On Wed, Nov 04, 2020 at 03:23:37PM -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.
>>>
>>> Tomi/Laurent/Grygorii,
>>>
>>> If you can test this series, that'd be great!
>>
>> I gave it a try, rebasing my branch from v5.9 to v5.10-rc2 first. On
>> v5.10-rc2 the kernel dies when booting due to a deadlock (reported by
>> lockdep, so hopefully not too hard to debug). *sigh*. Fortunately, it
>> dies after the fw_devlink initialization, so I can still report results.
> 
> Phew! For a sec I thought you said fw_devlink was causing a deadlock.
> 
>>
>> Before your series:
>>
>> [    0.743065] cpuidle: using governor menu
>> [   13.350259] No ATAGs?
>>
>> With your series applied:
>>
>> [    0.722670] cpuidle: using governor menu
>> [    1.135859] No ATAGs?
>>
>> That's a very clear improvement :-)
> 
> Thanks for testing. Great to hear it's helping!
> 
>> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I'll add it to my v2 series.

I've tried your series on top of
521b619acdc8 Merge tag 'linux-kselftest-kunit-fixes-5.10-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest
on am571x-idk

Before:
[    0.049395] cpuidle: using governor menu
[    1.654766] audit: type=2000 audit(0.040:1): state=initialized audit_enabled=0 res=1
[    2.315266] No ATAGs?
[    2.315317] hw-breakpoint: found 5 (+1 reserved) breakpoint and 4 watchpoint registers.
[    2.315327] hw-breakpoint: maximum watchpoint size is 8 bytes.
...
[    6.549595] EXT4-fs (mmcblk0p2): mounted filesystem with ordered data mode. Opts: (null)
[    6.557794] VFS: Mounted root (ext4 filesystem) on device 179:26.
[    6.574103] devtmpfs: mounted
[    6.577749] Freeing unused kernel memory: 1024K
[    6.582433] Run /sbin/init as init process


after:
[    0.049223] cpuidle: using governor menu
[    0.095893] audit: type=2000 audit(0.040:1): state=initialized audit_enabled=0 res=1
[    0.102958] No ATAGs?
[    0.103010] hw-breakpoint: found 5 (+1 reserved) breakpoint and 4 watchpoint registers.
[    0.103020] hw-breakpoint: maximum watchpoint size is 8 bytes.
...
[    3.518623] EXT4-fs (mmcblk0p2): mounted filesystem with ordered data mode. Opts: (null)
[    3.526822] VFS: Mounted root (ext4 filesystem) on device 179:26.
[    3.543128] devtmpfs: mounted
[    3.546781] Freeing unused kernel memory: 1024K
[    3.551463] Run /sbin/init as init process

So, it's much better. Thank you.
Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>

-- 
Best regards,
grygorii