[PATCH v1 0/5] Improve of_devlink to handle "proxy cycles"

[PATCH v1 0/5] Improve of_devlink to handle "proxy cycles"

[Saravana Kannan]

As described in [1], parent devices needed to create device links to
suppliers of their child devices to make sure the suppliers of the child
devices don't get a sync_state() before the child devices are added and
probed.

In these illustrations, -> denotes DT references and indentation
represents child status.

Example 1:
==========
Device node A
	Device node B -> C

Device node C

In this example, everything works as intended.
1. Device A is added
   a. Device A is added to wait_for_suppliers list
2. Device C is added
   a. Device link is created from A to C
   b. Device A is removed from wait_for_suppliers list
3. Device C probes
4. During late_initcall_sync() Device C doesn't get sync_state() call
   because Device A hasn't probed yet
4. Device A's driver is loaded as a module
5. Device A probes
   a. Device B is added
      i. Device link is created from B to C
6. Device B probes
7. Device C get sync_state() call since A and B have probed

Example 2:
==========
Device node A
	Device node B -> C

Device node C
	Device node D -> A

In this example, none of the devices probe:
1. Device A is added
   a. Device A is added to wait_for_suppliers list
2. Device C is added
   a. Device link is created from A to C
   b. Device A is removed from wait_for_suppliers list
   c. Device link is attempted from C to A, but fails since it would
      create a cycle.
   d. Device C is added to wait_for_suppliers list
3. Device C doesn't probe because it's on wait_for_suppliers list
4. Device A's driver is loaded as a module
5. Device A doesn't probe because its supplier Device C hasn't probed

Cycles in device links aren't allowed because device links represent
functional dependency and cause the devices to be reordered in the
dpm_list to make sure the consumer suspends/goes idle before the
supplier. If there's a cycle in the supplier/consumer dependencies,
there's no logical way to sort them in the dpm_list.

This patch series addresses this problem.

Patch 1 adds a SYNC_STATE_ONLY device link flag that states that the
device link should only affect the sync_state() call behavior and
nothing else. Since a SYNC_STATE_ONLY device link doesn't affect
dpm_list or probing, we can allow cycles within SYNC_STATE_ONLY device
links. What this means is that, all the devices in the SYNC_STATE_ONLY
device link cycle will get their sync_state() calls only after all of
them have probed successfully.

Patch 2 improves wait_for_suppliers semantics by allowing a device on
the list to state (device.links.need_for_probe) if the suppliers it's
waiting on (to make a device link to) are needed for it to probe or not.
If the device is not waiting on suppliers that are needed for probing,
the device is no longer blocked from attempting to probe even if it's on
the wait_for_suppliers list.

Patch 3 allows fwnode_operations.add_links() return error codes to
differentiate between being unable to find mandatory suppliers (-ENODEV)
vs optional suppliers (-EAGAIN). If add_links() is only unable to find
optional suppliers, then the corresponding device is marked as waiting
on suppliers that are not needed for probing.

Patch 4 changes device tree fwnode add_links() implementation so that
all device links created from a device to the suppliers of its child
devices use the SYNC_STATE_ONLY flag. It is also changed to return
-ENODEV only for errors with the suppliers of the device and return
-EAGAIN when the errors are limited to the suppliers of its child
devices.

Patch 5 is an unrelated improvement that touches the same bit of code,
so I'm grouping it with this series to avoid rebases. That commit should
be self descriptive.

With these changes, Example 2 works as follows:
1. Device A is added
   a. Device A is added to wait_for_suppliers list
   b. Device A is marked as waiting for optional suppliers
2. Device C is added
   a. SYNC_STATE_ONLY device link is created from A to C
   b. Device A is removed from wait_for_suppliers list
   c. SYNC_STATE_ONLY device link is created from C to A
3. Device C probes
   a. Device D is added.
      i. Device link is created from D to A
4. During late_initcall_sync() Device C doesn't get sync_state() call
   because Device A hasn't probed yet
5. Device A's driver is loaded as a module
6. Device A probes
   a. Device B is added
      i. Device link is created from B to C
7. Device A doesn't get sync_stat() call because Device D hasn't probed
8. Device D probes
9. Device A get sync_state() call since C and D have probed
10.Device B probes
11.Device C get sync_state() call since A and B have probed

Thanks,
Saravana

[1] - commit d4387cd117414ba80230f27a514be5ca4a09cfcc
      ("of: property: Create device links for all child-supplier depencencies")
      or https://lore.kernel.org/linux-acpi/20190904211126.47518-7-saravanak@google.com/

Saravana Kannan (5):
  driver core: Add device link support for SYNC_STATE_ONLY flag
  driver core: Allow a device to wait on optional suppliers
  driver core: Allow fwnode_operations.add_links to differentiate errors
  of: property: Make sure child dependencies don't block probing of
    parent
  of: property: Skip adding device links to suppliers that aren't
    devices

 drivers/base/core.c    | 88 +++++++++++++++++++++++++++++++++++-------
 drivers/of/property.c  | 21 +++++++---
 include/linux/device.h |  5 +++
 include/linux/fwnode.h | 13 +++++--
 4 files changed, 102 insertions(+), 25 deletions(-)

-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH v1 1/5] driver core: Add device link support for SYNC_STATE_ONLY flag

[Saravana Kannan]

Parent devices might need to create "proxy" device links from themselves
to supplier devices to make sure the supplier devices don't get a
sync_state() before the child consumer devices get a chance to add
device links to the supplier devices.

However, the parent device has no real dependency on the supplier device
and probing, suspend/resume or runtime PM don't need to be affected by
the supplier device.  To capture these cases, create a SYNC_STATE_ONLY
device link flag that only affects sync_state() behavior and doesn't
affect probing, suspend/resume or runtime PM.

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7ea665a97da2..17ed054c4132 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -131,6 +131,9 @@ static int device_is_dependent(struct device *dev, void *target)
 		return ret;
 
 	list_for_each_entry(link, &dev->links.consumers, s_node) {
+		if (link->flags == (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED))
+			continue;
+
 		if (link->consumer == target)
 			return 1;
 
@@ -200,8 +203,11 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
 		device_pm_move_last(dev);
 
 	device_for_each_child(dev, NULL, device_reorder_to_tail);
-	list_for_each_entry(link, &dev->links.consumers, s_node)
+	list_for_each_entry(link, &dev->links.consumers, s_node) {
+		if (link->flags == (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED))
+			continue;
 		device_reorder_to_tail(link->consumer, NULL);
+	}
 
 	return 0;
 }
@@ -228,7 +234,8 @@ void device_pm_move_to_tail(struct device *dev)
 
 #define DL_MANAGED_LINK_FLAGS (DL_FLAG_AUTOREMOVE_CONSUMER | \
 			       DL_FLAG_AUTOREMOVE_SUPPLIER | \
-			       DL_FLAG_AUTOPROBE_CONSUMER)
+			       DL_FLAG_AUTOPROBE_CONSUMER  | \
+			       DL_FLAG_SYNC_STATE_ONLY)
 
 #define DL_ADD_VALID_FLAGS (DL_MANAGED_LINK_FLAGS | DL_FLAG_STATELESS | \
 			    DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE)
@@ -296,6 +303,8 @@ struct device_link *device_link_add(struct device *consumer,
 
 	if (!consumer || !supplier || flags & ~DL_ADD_VALID_FLAGS ||
 	    (flags & DL_FLAG_STATELESS && flags & DL_MANAGED_LINK_FLAGS) ||
+	    (flags & DL_FLAG_SYNC_STATE_ONLY &&
+	     flags != DL_FLAG_SYNC_STATE_ONLY) ||
 	    (flags & DL_FLAG_AUTOPROBE_CONSUMER &&
 	     flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
 		      DL_FLAG_AUTOREMOVE_SUPPLIER)))
@@ -316,11 +325,14 @@ struct device_link *device_link_add(struct device *consumer,
 
 	/*
 	 * If the supplier has not been fully registered yet or there is a
-	 * reverse dependency between the consumer and the supplier already in
-	 * the graph, return NULL.
+	 * reverse (non-SYNC_STATE_ONLY) dependency between the consumer and
+	 * the supplier already in the graph, return NULL. If the link is a
+	 * SYNC_STATE_ONLY link, we don't check for reverse dependencies
+	 * because it only affects sync_state() callbacks.
 	 */
 	if (!device_pm_initialized(supplier)
-	    || device_is_dependent(consumer, supplier)) {
+	    || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
+		  device_is_dependent(consumer, supplier))) {
 		link = NULL;
 		goto out;
 	}
@@ -347,9 +359,14 @@ struct device_link *device_link_add(struct device *consumer,
 		}
 
 		if (flags & DL_FLAG_STATELESS) {
-			link->flags |= DL_FLAG_STATELESS;
 			kref_get(&link->kref);
-			goto out;
+			if (link->flags & DL_FLAG_SYNC_STATE_ONLY &&
+			    !(link->flags & DL_FLAG_STATELESS)) {
+				link->flags |= DL_FLAG_STATELESS;
+				goto reorder;
+			} else {
+				goto out;
+			}
 		}
 
 		/*
@@ -371,6 +388,12 @@ struct device_link *device_link_add(struct device *consumer,
 			link->flags |= DL_FLAG_MANAGED;
 			device_link_init_status(link, consumer, supplier);
 		}
+		if (link->flags & DL_FLAG_SYNC_STATE_ONLY &&
+		    !(flags & DL_FLAG_SYNC_STATE_ONLY)) {
+			link->flags &= ~DL_FLAG_SYNC_STATE_ONLY;
+			goto reorder;
+		}
+
 		goto out;
 	}
 
@@ -410,6 +433,13 @@ struct device_link *device_link_add(struct device *consumer,
 	    flags & DL_FLAG_PM_RUNTIME)
 		pm_runtime_resume(supplier);
 
+	if (flags & DL_FLAG_SYNC_STATE_ONLY) {
+		dev_dbg(consumer,
+			"Linked as a sync state only consumer to %s\n",
+			dev_name(supplier));
+		goto out;
+	}
+reorder:
 	/*
 	 * Move the consumer and all of the devices depending on it to the end
 	 * of dpm_list and the devices_kset list.
@@ -635,7 +665,8 @@ int device_links_check_suppliers(struct device *dev)
 	device_links_write_lock();
 
 	list_for_each_entry(link, &dev->links.suppliers, c_node) {
-		if (!(link->flags & DL_FLAG_MANAGED))
+		if (!(link->flags & DL_FLAG_MANAGED) ||
+		    link->flags & DL_FLAG_SYNC_STATE_ONLY)
 			continue;
 
 		if (link->status != DL_STATE_AVAILABLE) {
@@ -949,7 +980,8 @@ void device_links_unbind_consumers(struct device *dev)
 	list_for_each_entry(link, &dev->links.consumers, s_node) {
 		enum device_link_state status;
 
-		if (!(link->flags & DL_FLAG_MANAGED))
+		if (!(link->flags & DL_FLAG_MANAGED) ||
+		    link->flags & DL_FLAG_SYNC_STATE_ONLY)
 			continue;
 
 		status = link->status;
diff --git a/include/linux/device.h b/include/linux/device.h
index 9f2f2e169f95..f1f2aa0b19da 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1097,6 +1097,7 @@ enum device_link_state {
  * AUTOREMOVE_SUPPLIER: Remove the link automatically on supplier driver unbind.
  * AUTOPROBE_CONSUMER: Probe consumer driver automatically after supplier binds.
  * MANAGED: The core tracks presence of supplier/consumer drivers (internal).
+ * SYNC_STATE_ONLY: Link only affects sync_state() behavior.
  */
 #define DL_FLAG_STATELESS		BIT(0)
 #define DL_FLAG_AUTOREMOVE_CONSUMER	BIT(1)
@@ -1105,6 +1106,7 @@ enum device_link_state {
 #define DL_FLAG_AUTOREMOVE_SUPPLIER	BIT(4)
 #define DL_FLAG_AUTOPROBE_CONSUMER	BIT(5)
 #define DL_FLAG_MANAGED			BIT(6)
+#define DL_FLAG_SYNC_STATE_ONLY		BIT(7)
 
 /**
  * struct device_link - Device link representation.
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH v1 2/5] driver core: Allow a device to wait on optional suppliers

[Saravana Kannan]

Before this change, if a device is waiting on suppliers, it's assumed
that all those suppliers are needed for the device to probe
successfully. This change allows marking a devices as waiting only on
optional suppliers. This allows a device to wait on suppliers (and link
to them as soon as they are available) without preventing the device
from being probed.

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 17ed054c4132..48cd43a91ce6 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -480,13 +480,25 @@ EXPORT_SYMBOL_GPL(device_link_add);
  * 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)
+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
@@ -656,7 +668,8 @@ int device_links_check_suppliers(struct device *dev)
 	 * probe.
 	 */
 	mutex_lock(&wfs_lock);
-	if (!list_empty(&dev->links.needs_suppliers)) {
+	if (!list_empty(&dev->links.needs_suppliers) &&
+	    dev->links.need_for_probe) {
 		mutex_unlock(&wfs_lock);
 		return -EPROBE_DEFER;
 	}
@@ -760,6 +773,15 @@ void device_links_driver_bound(struct device *dev)
 {
 	struct device_link *link;
 
+	/*
+	 * If a device probes successfully, it's expected to have created all
+	 * 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);
+
 	device_links_write_lock();
 
 	list_for_each_entry(link, &dev->links.consumers, s_node) {
@@ -2393,7 +2415,7 @@ int device_add(struct device *dev)
 
 	if (fwnode_has_op(dev->fwnode, add_links)
 	    && fwnode_call_int_op(dev->fwnode, add_links, dev))
-		device_link_wait_for_supplier(dev);
+		device_link_wait_for_mandatory_supplier(dev, true);
 
 	bus_probe_device(dev);
 	if (parent)
diff --git a/include/linux/device.h b/include/linux/device.h
index f1f2aa0b19da..4fd33da9a848 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1156,6 +1156,8 @@ enum dl_dev_state {
  * @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 {
@@ -1163,6 +1165,7 @@ struct dev_links_info {
 	struct list_head consumers;
 	struct list_head needs_suppliers;
 	struct list_head defer_sync;
+	bool need_for_probe;
 	enum dl_dev_state status;
 };
 
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH v1 3/5] driver core: Allow fwnode_operations.add_links to differentiate errors

[Saravana Kannan]

When add_links() still has suppliers that it needs to link to in the
future, this patch allows it to differentiate between suppliers that are
needed for probing vs suppliers that are needed for sync_state()
correctness.

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 48cd43a91ce6..e6d3e6d485da 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2297,7 +2297,7 @@ int device_add(struct device *dev)
 	struct device *parent;
 	struct kobject *kobj;
 	struct class_interface *class_intf;
-	int error = -EINVAL;
+	int error = -EINVAL, fw_ret;
 	struct kobject *glue_dir = NULL;
 
 	dev = get_device(dev);
@@ -2413,9 +2413,13 @@ int device_add(struct device *dev)
 	 */
 	device_link_add_missing_supplier_links();
 
-	if (fwnode_has_op(dev->fwnode, add_links)
-	    && fwnode_call_int_op(dev->fwnode, add_links, dev))
-		device_link_wait_for_mandatory_supplier(dev, true);
+	if (fwnode_has_op(dev->fwnode, add_links)) {
+		fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
+		if (fw_ret == -ENODEV)
+			device_link_wait_for_mandatory_supplier(dev);
+		else if (fw_ret)
+			device_link_wait_for_optional_supplier(dev);
+	}
 
 	bus_probe_device(dev);
 	if (parent)
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 25bb81f8ded8..a19134eae5a5 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -96,10 +96,15 @@ struct fwnode_reference_args {
  *		available suppliers.
  *
  *		Return 0 if device links have been successfully created to all
- *		the suppliers of this device or if the supplier information is
- *		not known. Return an error if and only if the supplier
- *		information is known but some of the suppliers are not yet
- *		available to create device links to.
+ *		the suppliers this device needs to create device links to or if
+ *		the supplier information is not known.
+ *
+ *		Return -ENODEV if and only if the suppliers needed for probing
+ *		the device are not yet available to create device links to.
+ *
+ *		Return -EAGAIN if there are suppliers that need to be linked to
+ *		that are not yet available but none of those suppliers are
+ *		necessary for probing this device.
  */
 struct fwnode_operations {
 	struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH v1 4/5] of: property: Make sure child dependencies don't block probing of parent

[Saravana Kannan]

When creating device links to proxy the sync_state() needs of child
dependencies, create SYNC_STATE_ONLY device links so that children
dependencies don't block probing of the parent.

Also, differentiate between missing suppliers of parent device vs
missing suppliers of child devices so that driver core doesn't block
parent device probing when only child supplier dependencies are missing.

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

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 2808832b2e86..f16f85597ccc 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1032,10 +1032,10 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
  * - -EINVAL if the supplier link is invalid and should not be created
  * - -ENODEV if there is no device that corresponds to the supplier phandle
  */
-static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
+static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
+			      u32 dl_flags)
 {
 	struct device *sup_dev;
-	u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
 	int ret = 0;
 	struct device_node *tmp_np = sup_np;
 
@@ -1195,13 +1195,20 @@ static int of_link_property(struct device *dev, struct device_node *con_np,
 	unsigned int i = 0;
 	bool matched = false;
 	int ret = 0;
+	u32 dl_flags;
+
+	if (dev->of_node == con_np)
+		dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
+	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) == -EAGAIN)
+			if (of_link_to_phandle(dev, phandle, dl_flags)
+								== -EAGAIN)
 				ret = -EAGAIN;
 			of_node_put(phandle);
 		}
@@ -1219,10 +1226,10 @@ static int of_link_to_suppliers(struct device *dev,
 
 	for_each_property_of_node(con_np, p)
 		if (of_link_property(dev, con_np, p->name))
-			ret = -EAGAIN;
+			ret = -ENODEV;
 
 	for_each_child_of_node(con_np, child)
-		if (of_link_to_suppliers(dev, child))
+		if (of_link_to_suppliers(dev, child) && !ret)
 			ret = -EAGAIN;
 
 	return ret;
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH v1 5/5] of: property: Skip adding device links to suppliers that aren't devices

[Saravana Kannan]

Some devices need to be initialized really early and can't wait for
driver core or drivers to be functional.  These devices are typically
initialized without creating a struct device for their device nodes.

If a supplier ends up being one of these devices, skip trying to add
device links to them.

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

diff --git a/drivers/of/property.c b/drivers/of/property.c
index f16f85597ccc..21c9d251318a 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1038,6 +1038,7 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 	struct device *sup_dev;
 	int ret = 0;
 	struct device_node *tmp_np = sup_np;
+	int is_populated;
 
 	of_node_get(sup_np);
 	/*
@@ -1062,9 +1063,10 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 		return -EINVAL;
 	}
 	sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
+	is_populated = of_node_check_flag(sup_np, OF_POPULATED);
 	of_node_put(sup_np);
 	if (!sup_dev)
-		return -EAGAIN;
+		return is_populated ? 0 : -EAGAIN;
 	if (!device_link_add(dev, sup_dev, dl_flags))
 		ret = -EAGAIN;
 	put_device(sup_dev);
-- 
2.24.0.rc0.303.g954a862665-goog

Re: [PATCH v1 5/5] of: property: Skip adding device links to suppliers that aren't devices

[Rob Herring]

On Mon, Oct 28, 2019 at 5:00 PM Saravana Kannan <saravanak@google.com> wrote:
>
> Some devices need to be initialized really early and can't wait for
> driver core or drivers to be functional.  These devices are typically
> initialized without creating a struct device for their device nodes.
>
> If a supplier ends up being one of these devices, skip trying to add
> device links to them.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/property.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index f16f85597ccc..21c9d251318a 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1038,6 +1038,7 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
>         struct device *sup_dev;
>         int ret = 0;
>         struct device_node *tmp_np = sup_np;
> +       int is_populated;
>
>         of_node_get(sup_np);
>         /*
> @@ -1062,9 +1063,10 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
>                 return -EINVAL;
>         }
>         sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
> +       is_populated = of_node_check_flag(sup_np, OF_POPULATED);
>         of_node_put(sup_np);
>         if (!sup_dev)
> -               return -EAGAIN;
> +               return is_populated ? 0 : -EAGAIN;

You're only using the flag in one spot and a comment would be good
here, so I'd just do:

if (of_node_check_flag(sup_np, OF_POPULATED))
        return 0; /* Early device without a struct device */

>         if (!device_link_add(dev, sup_dev, dl_flags))
>                 ret = -EAGAIN;
>         put_device(sup_dev);
> --
> 2.24.0.rc0.303.g954a862665-goog
>

Re: [PATCH v1 4/5] of: property: Make sure child dependencies don't block probing of parent

[Rob Herring]

On Mon, Oct 28, 2019 at 5:00 PM Saravana Kannan <saravanak@google.com> wrote:
>
> When creating device links to proxy the sync_state() needs of child
> dependencies, create SYNC_STATE_ONLY device links so that children
> dependencies don't block probing of the parent.
>
> Also, differentiate between missing suppliers of parent device vs
> missing suppliers of child devices so that driver core doesn't block
> parent device probing when only child supplier dependencies are missing.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/property.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)

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

One nit below:

>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 2808832b2e86..f16f85597ccc 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1032,10 +1032,10 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
>   * - -EINVAL if the supplier link is invalid and should not be created
>   * - -ENODEV if there is no device that corresponds to the supplier phandle
>   */
> -static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> +                             u32 dl_flags)
>  {
>         struct device *sup_dev;
> -       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
>         int ret = 0;
>         struct device_node *tmp_np = sup_np;
>
> @@ -1195,13 +1195,20 @@ static int of_link_property(struct device *dev, struct device_node *con_np,
>         unsigned int i = 0;
>         bool matched = false;
>         int ret = 0;
> +       u32 dl_flags;
> +
> +       if (dev->of_node == con_np)
> +               dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> +       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) == -EAGAIN)
> +                       if (of_link_to_phandle(dev, phandle, dl_flags)
> +                                                               == -EAGAIN)

nit: I'd just keep this one line or at least move '==' up.

>                                 ret = -EAGAIN;
>                         of_node_put(phandle);
>                 }
> @@ -1219,10 +1226,10 @@ static int of_link_to_suppliers(struct device *dev,
>
>         for_each_property_of_node(con_np, p)
>                 if (of_link_property(dev, con_np, p->name))
> -                       ret = -EAGAIN;
> +                       ret = -ENODEV;
>
>         for_each_child_of_node(con_np, child)
> -               if (of_link_to_suppliers(dev, child))
> +               if (of_link_to_suppliers(dev, child) && !ret)
>                         ret = -EAGAIN;
>
>         return ret;
> --
> 2.24.0.rc0.303.g954a862665-goog
>

Re: [PATCH v1 5/5] of: property: Skip adding device links to suppliers that aren't devices

[Saravana Kannan]

On Mon, Nov 4, 2019 at 7:18 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Oct 28, 2019 at 5:00 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Some devices need to be initialized really early and can't wait for
> > driver core or drivers to be functional.  These devices are typically
> > initialized without creating a struct device for their device nodes.
> >
> > If a supplier ends up being one of these devices, skip trying to add
> > device links to them.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/of/property.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index f16f85597ccc..21c9d251318a 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1038,6 +1038,7 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> >         struct device *sup_dev;
> >         int ret = 0;
> >         struct device_node *tmp_np = sup_np;
> > +       int is_populated;
> >
> >         of_node_get(sup_np);
> >         /*
> > @@ -1062,9 +1063,10 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> >                 return -EINVAL;
> >         }
> >         sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
> > +       is_populated = of_node_check_flag(sup_np, OF_POPULATED);
> >         of_node_put(sup_np);
> >         if (!sup_dev)
> > -               return -EAGAIN;
> > +               return is_populated ? 0 : -EAGAIN;
>
> You're only using the flag in one spot and a comment would be good
> here, so I'd just do:
>
> if (of_node_check_flag(sup_np, OF_POPULATED))
>         return 0; /* Early device without a struct device */

Hi Rob,

Thanks for the review.

I'm using the flag to keep the error handling code simple/cleaner. I
can't do the check like that after I do a put on the sup_np.

Yeah, I was actually planning to add a dev_dbg() message when this
happens and returning a -EINVAL (that'll be ignored by the caller)
instead of -EAGAIN (that's NOT ignored by the caller).

Looks like these changes go pulled into driver-core-next. So I'll send
a delta patch to add the dbg message and also address you nit on the
other patch.

Thanks,
Saravana

Re: [PATCH v1 4/5] of: property: Make sure child dependencies don't block probing of parent

[Saravana Kannan]

On Mon, Nov 4, 2019 at 9:01 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Oct 28, 2019 at 5:00 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > When creating device links to proxy the sync_state() needs of child
> > dependencies, create SYNC_STATE_ONLY device links so that children
> > dependencies don't block probing of the parent.
> >
> > Also, differentiate between missing suppliers of parent device vs
> > missing suppliers of child devices so that driver core doesn't block
> > parent device probing when only child supplier dependencies are missing.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/of/property.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
>
> Reviewed-by: Rob Herring <robh@kernel.org>

Thank you!

> One nit below:
>
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 2808832b2e86..f16f85597ccc 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1032,10 +1032,10 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
> >   * - -EINVAL if the supplier link is invalid and should not be created
> >   * - -ENODEV if there is no device that corresponds to the supplier phandle
> >   */
> > -static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> > +                             u32 dl_flags)
> >  {
> >         struct device *sup_dev;
> > -       u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> >         int ret = 0;
> >         struct device_node *tmp_np = sup_np;
> >
> > @@ -1195,13 +1195,20 @@ static int of_link_property(struct device *dev, struct device_node *con_np,
> >         unsigned int i = 0;
> >         bool matched = false;
> >         int ret = 0;
> > +       u32 dl_flags;
> > +
> > +       if (dev->of_node == con_np)
> > +               dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > +       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) == -EAGAIN)
> > +                       if (of_link_to_phandle(dev, phandle, dl_flags)
> > +                                                               == -EAGAIN)
>
> nit: I'd just keep this one line or at least move '==' up.

You'd keep it one line even if it's > 80 cols? Ok, I'll move the "==" up.


-Saravana

Re: [PATCH v1 5/5] of: property: Skip adding device links to suppliers that aren't devices

[Rob Herring]

On Mon, Nov 4, 2019 at 1:02 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Nov 4, 2019 at 7:18 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Mon, Oct 28, 2019 at 5:00 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > Some devices need to be initialized really early and can't wait for
> > > driver core or drivers to be functional.  These devices are typically
> > > initialized without creating a struct device for their device nodes.
> > >
> > > If a supplier ends up being one of these devices, skip trying to add
> > > device links to them.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/of/property.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index f16f85597ccc..21c9d251318a 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1038,6 +1038,7 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> > >         struct device *sup_dev;
> > >         int ret = 0;
> > >         struct device_node *tmp_np = sup_np;
> > > +       int is_populated;
> > >
> > >         of_node_get(sup_np);
> > >         /*
> > > @@ -1062,9 +1063,10 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> > >                 return -EINVAL;
> > >         }
> > >         sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
> > > +       is_populated = of_node_check_flag(sup_np, OF_POPULATED);
> > >         of_node_put(sup_np);
> > >         if (!sup_dev)
> > > -               return -EAGAIN;
> > > +               return is_populated ? 0 : -EAGAIN;
> >
> > You're only using the flag in one spot and a comment would be good
> > here, so I'd just do:
> >
> > if (of_node_check_flag(sup_np, OF_POPULATED))
> >         return 0; /* Early device without a struct device */
>
> Hi Rob,
>
> Thanks for the review.
>
> I'm using the flag to keep the error handling code simple/cleaner. I
> can't do the check like that after I do a put on the sup_np.

Ah, right. Nevermind.

> Yeah, I was actually planning to add a dev_dbg() message when this
> happens and returning a -EINVAL (that'll be ignored by the caller)
> instead of -EAGAIN (that's NOT ignored by the caller).
>
> Looks like these changes go pulled into driver-core-next. So I'll send
> a delta patch to add the dbg message and also address you nit on the
> other patch.

I didn't notice Greg applied already. Don't worry about the nit then.

Rob

Re: [PATCH v1 2/5] driver core: Allow a device to wait on optional suppliers

[Rafael J. Wysocki]

On Monday, October 28, 2019 11:00:23 PM CET Saravana Kannan wrote:
> Before this change, if a device is waiting on suppliers, it's assumed
> that all those suppliers are needed for the device to probe
> successfully. This change allows marking a devices as waiting only on
> optional suppliers. This allows a device to wait on suppliers (and link
> to them as soon as they are available) without preventing the device
> from being probed.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c    | 28 +++++++++++++++++++++++++---
>  include/linux/device.h |  3 +++
>  2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 17ed054c4132..48cd43a91ce6 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -480,13 +480,25 @@ EXPORT_SYMBOL_GPL(device_link_add);
>   * 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)
> +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
> @@ -656,7 +668,8 @@ int device_links_check_suppliers(struct device *dev)
>  	 * probe.
>  	 */
>  	mutex_lock(&wfs_lock);
> -	if (!list_empty(&dev->links.needs_suppliers)) {
> +	if (!list_empty(&dev->links.needs_suppliers) &&
> +	    dev->links.need_for_probe) {
>  		mutex_unlock(&wfs_lock);
>  		return -EPROBE_DEFER;
>  	}
> @@ -760,6 +773,15 @@ void device_links_driver_bound(struct device *dev)
>  {
>  	struct device_link *link;
>  
> +	/*
> +	 * If a device probes successfully, it's expected to have created all
> +	 * 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);
> +
>  	device_links_write_lock();
>  
>  	list_for_each_entry(link, &dev->links.consumers, s_node) {
> @@ -2393,7 +2415,7 @@ int device_add(struct device *dev)
>  
>  	if (fwnode_has_op(dev->fwnode, add_links)
>  	    && fwnode_call_int_op(dev->fwnode, add_links, dev))
> -		device_link_wait_for_supplier(dev);
> +		device_link_wait_for_mandatory_supplier(dev, true);

Does this compile even?

The function takes one argument according to the definition above ...

>  	bus_probe_device(dev);
>  	if (parent)
> diff --git a/include/linux/device.h b/include/linux/device.h
> index f1f2aa0b19da..4fd33da9a848 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1156,6 +1156,8 @@ enum dl_dev_state {
>   * @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 {
> @@ -1163,6 +1165,7 @@ struct dev_links_info {
>  	struct list_head consumers;
>  	struct list_head needs_suppliers;
>  	struct list_head defer_sync;
> +	bool need_for_probe;
>  	enum dl_dev_state status;
>  };
>  
> 

Re: [PATCH v1 2/5] driver core: Allow a device to wait on optional suppliers

[Saravana Kannan]

Looks like I squashed/rebased a bit incorrectly. It's fixed in the
next patch in the series.

-Saravana

On Tue, Nov 5, 2019 at 2:29 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Monday, October 28, 2019 11:00:23 PM CET Saravana Kannan wrote:
> > Before this change, if a device is waiting on suppliers, it's assumed
> > that all those suppliers are needed for the device to probe
> > successfully. This change allows marking a devices as waiting only on
> > optional suppliers. This allows a device to wait on suppliers (and link
> > to them as soon as they are available) without preventing the device
> > from being probed.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c    | 28 +++++++++++++++++++++++++---
> >  include/linux/device.h |  3 +++
> >  2 files changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 17ed054c4132..48cd43a91ce6 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -480,13 +480,25 @@ EXPORT_SYMBOL_GPL(device_link_add);
> >   * 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)
> > +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
> > @@ -656,7 +668,8 @@ int device_links_check_suppliers(struct device *dev)
> >        * probe.
> >        */
> >       mutex_lock(&wfs_lock);
> > -     if (!list_empty(&dev->links.needs_suppliers)) {
> > +     if (!list_empty(&dev->links.needs_suppliers) &&
> > +         dev->links.need_for_probe) {
> >               mutex_unlock(&wfs_lock);
> >               return -EPROBE_DEFER;
> >       }
> > @@ -760,6 +773,15 @@ void device_links_driver_bound(struct device *dev)
> >  {
> >       struct device_link *link;
> >
> > +     /*
> > +      * If a device probes successfully, it's expected to have created all
> > +      * 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);
> > +
> >       device_links_write_lock();
> >
> >       list_for_each_entry(link, &dev->links.consumers, s_node) {
> > @@ -2393,7 +2415,7 @@ int device_add(struct device *dev)
> >
> >       if (fwnode_has_op(dev->fwnode, add_links)
> >           && fwnode_call_int_op(dev->fwnode, add_links, dev))
> > -             device_link_wait_for_supplier(dev);
> > +             device_link_wait_for_mandatory_supplier(dev, true);
>
> Does this compile even?
>
> The function takes one argument according to the definition above ...
>
> >       bus_probe_device(dev);
> >       if (parent)
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index f1f2aa0b19da..4fd33da9a848 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -1156,6 +1156,8 @@ enum dl_dev_state {
> >   * @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 {
> > @@ -1163,6 +1165,7 @@ struct dev_links_info {
> >       struct list_head consumers;
> >       struct list_head needs_suppliers;
> >       struct list_head defer_sync;
> > +     bool need_for_probe;
> >       enum dl_dev_state status;
> >  };
> >
> >
>
>
>
>

Re: [PATCH v1 3/5] driver core: Allow fwnode_operations.add_links to differentiate errors

[Rafael J. Wysocki]

On Monday, October 28, 2019 11:00:24 PM CET Saravana Kannan wrote:
> When add_links() still has suppliers that it needs to link to in the
> future, this patch allows it to differentiate between suppliers that are
> needed for probing vs suppliers that are needed for sync_state()
> correctness.

I guess you mean that it will return different error codes in the different
cases.

> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c    | 12 ++++++++----
>  include/linux/fwnode.h | 13 +++++++++----
>  2 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 48cd43a91ce6..e6d3e6d485da 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2297,7 +2297,7 @@ int device_add(struct device *dev)
>  	struct device *parent;
>  	struct kobject *kobj;
>  	struct class_interface *class_intf;
> -	int error = -EINVAL;
> +	int error = -EINVAL, fw_ret;
>  	struct kobject *glue_dir = NULL;
>  
>  	dev = get_device(dev);
> @@ -2413,9 +2413,13 @@ int device_add(struct device *dev)
>  	 */
>  	device_link_add_missing_supplier_links();
>  
> -	if (fwnode_has_op(dev->fwnode, add_links)
> -	    && fwnode_call_int_op(dev->fwnode, add_links, dev))
> -		device_link_wait_for_mandatory_supplier(dev, true);
> +	if (fwnode_has_op(dev->fwnode, add_links)) {

fw_ret can be defined here and I'd just call it "ret".

> +		fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
> +		if (fw_ret == -ENODEV)
> +			device_link_wait_for_mandatory_supplier(dev);
> +		else if (fw_ret)
> +			device_link_wait_for_optional_supplier(dev);
> +	}
>  
>  	bus_probe_device(dev);
>  	if (parent)
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 25bb81f8ded8..a19134eae5a5 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -96,10 +96,15 @@ struct fwnode_reference_args {
>   *		available suppliers.
>   *
>   *		Return 0 if device links have been successfully created to all
> - *		the suppliers of this device or if the supplier information is
> - *		not known. Return an error if and only if the supplier
> - *		information is known but some of the suppliers are not yet
> - *		available to create device links to.
> + *		the suppliers this device needs to create device links to or if
> + *		the supplier information is not known.

"the known suppliers of this device or if the supplier information is not known."

> + *
> + *		Return -ENODEV if and only if the suppliers needed for probing
> + *		the device are not yet available to create device links to.

It would be more precise to say something like this:

"Return -ENODEV if an attempt to create a device link to one of the device's
suppliers needed for probing it fails."

> + *
> + *		Return -EAGAIN if there are suppliers that need to be linked to
> + *		that are not yet available but none of those suppliers are
> + *		necessary for probing this device.

"Return -EAGAIN if attempts to create device links to some of the device's
suppliers have failed, but those suppliers are not necessary for probing the
device."

>   */
>  struct fwnode_operations {
>  	struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
> 

Re: [PATCH v1 3/5] driver core: Allow fwnode_operations.add_links to differentiate errors

[Saravana Kannan]

Hi Rafael,

Thanks for the review.

On Tue, Nov 5, 2019 at 2:43 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Monday, October 28, 2019 11:00:24 PM CET Saravana Kannan wrote:
> > When add_links() still has suppliers that it needs to link to in the
> > future, this patch allows it to differentiate between suppliers that are
> > needed for probing vs suppliers that are needed for sync_state()
> > correctness.
>
> I guess you mean that it will return different error codes in the different
> cases.

Yes.

>
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c    | 12 ++++++++----
> >  include/linux/fwnode.h | 13 +++++++++----
> >  2 files changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 48cd43a91ce6..e6d3e6d485da 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2297,7 +2297,7 @@ int device_add(struct device *dev)
> >       struct device *parent;
> >       struct kobject *kobj;
> >       struct class_interface *class_intf;
> > -     int error = -EINVAL;
> > +     int error = -EINVAL, fw_ret;
> >       struct kobject *glue_dir = NULL;
> >
> >       dev = get_device(dev);
> > @@ -2413,9 +2413,13 @@ int device_add(struct device *dev)
> >        */
> >       device_link_add_missing_supplier_links();
> >
> > -     if (fwnode_has_op(dev->fwnode, add_links)
> > -         && fwnode_call_int_op(dev->fwnode, add_links, dev))
> > -             device_link_wait_for_mandatory_supplier(dev, true);
> > +     if (fwnode_has_op(dev->fwnode, add_links)) {
>
> fw_ret can be defined here and I'd just call it "ret".

I thought that style of variable declaration is frowned up in the
kernel coding style.

>
> > +             fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
> > +             if (fw_ret == -ENODEV)
> > +                     device_link_wait_for_mandatory_supplier(dev);
> > +             else if (fw_ret)
> > +                     device_link_wait_for_optional_supplier(dev);
> > +     }
> >
> >       bus_probe_device(dev);
> >       if (parent)
> > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > index 25bb81f8ded8..a19134eae5a5 100644
> > --- a/include/linux/fwnode.h
> > +++ b/include/linux/fwnode.h
> > @@ -96,10 +96,15 @@ struct fwnode_reference_args {
> >   *           available suppliers.
> >   *
> >   *           Return 0 if device links have been successfully created to all
> > - *           the suppliers of this device or if the supplier information is
> > - *           not known. Return an error if and only if the supplier
> > - *           information is known but some of the suppliers are not yet
> > - *           available to create device links to.
> > + *           the suppliers this device needs to create device links to or if
> > + *           the supplier information is not known.
>
> "the known suppliers of this device or if the supplier information is not known."

"suppliers it needs to create device links to" is a subset of known
suppliers. There's no requirement that fw needs to create links to ALL
known suppliers. Just a minor distinction.

> > + *
> > + *           Return -ENODEV if and only if the suppliers needed for probing
> > + *           the device are not yet available to create device links to.
>
> It would be more precise to say something like this:
>
> "Return -ENODEV if an attempt to create a device link to one of the device's
> suppliers needed for probing it fails."

"attempt to create a device link to one of the device's suppliers
needed for probing it fails" to me means device_link_add() fails.
But I'm trying to say that it should return an error if the struct
device isn't even there yet.

> > + *
> > + *           Return -EAGAIN if there are suppliers that need to be linked to
> > + *           that are not yet available but none of those suppliers are
> > + *           necessary for probing this device.
>
> "Return -EAGAIN if attempts to create device links to some of the device's
> suppliers have failed, but those suppliers are not necessary for probing the
> device."

Same comment as before. The distinction I'm making here is that
-EAGAIN is needed when the struct device itself isn't there.

Btw, Greg already pulled these into driver-core-next. Let me know if
you want me to send a delta patch to fix any of these comments.

Thanks,
Saravana

Re: [PATCH v1 3/5] driver core: Allow fwnode_operations.add_links to differentiate errors

[Rafael J. Wysocki]

On Tue, Nov 5, 2019 at 11:52 PM Saravana Kannan <saravanak@google.com> wrote:
>
> Hi Rafael,
>
> Thanks for the review.
>
> On Tue, Nov 5, 2019 at 2:43 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Monday, October 28, 2019 11:00:24 PM CET Saravana Kannan wrote:
> > > When add_links() still has suppliers that it needs to link to in the
> > > future, this patch allows it to differentiate between suppliers that are
> > > needed for probing vs suppliers that are needed for sync_state()
> > > correctness.
> >
> > I guess you mean that it will return different error codes in the different
> > cases.
>
> Yes.
>
> >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/base/core.c    | 12 ++++++++----
> > >  include/linux/fwnode.h | 13 +++++++++----
> > >  2 files changed, 17 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 48cd43a91ce6..e6d3e6d485da 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -2297,7 +2297,7 @@ int device_add(struct device *dev)
> > >       struct device *parent;
> > >       struct kobject *kobj;
> > >       struct class_interface *class_intf;
> > > -     int error = -EINVAL;
> > > +     int error = -EINVAL, fw_ret;
> > >       struct kobject *glue_dir = NULL;
> > >
> > >       dev = get_device(dev);
> > > @@ -2413,9 +2413,13 @@ int device_add(struct device *dev)
> > >        */
> > >       device_link_add_missing_supplier_links();
> > >
> > > -     if (fwnode_has_op(dev->fwnode, add_links)
> > > -         && fwnode_call_int_op(dev->fwnode, add_links, dev))
> > > -             device_link_wait_for_mandatory_supplier(dev, true);
> > > +     if (fwnode_has_op(dev->fwnode, add_links)) {
> >
> > fw_ret can be defined here and I'd just call it "ret".
>
> I thought that style of variable declaration is frowned up in the
> kernel coding style.

Well, I'm not aware of that. :-)

> >
> > > +             fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
> > > +             if (fw_ret == -ENODEV)
> > > +                     device_link_wait_for_mandatory_supplier(dev);
> > > +             else if (fw_ret)
> > > +                     device_link_wait_for_optional_supplier(dev);
> > > +     }
> > >
> > >       bus_probe_device(dev);
> > >       if (parent)
> > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > > index 25bb81f8ded8..a19134eae5a5 100644
> > > --- a/include/linux/fwnode.h
> > > +++ b/include/linux/fwnode.h
> > > @@ -96,10 +96,15 @@ struct fwnode_reference_args {
> > >   *           available suppliers.
> > >   *
> > >   *           Return 0 if device links have been successfully created to all
> > > - *           the suppliers of this device or if the supplier information is
> > > - *           not known. Return an error if and only if the supplier
> > > - *           information is known but some of the suppliers are not yet
> > > - *           available to create device links to.
> > > + *           the suppliers this device needs to create device links to or if
> > > + *           the supplier information is not known.
> >
> > "the known suppliers of this device or if the supplier information is not known."
>
> "suppliers it needs to create device links to" is a subset of known
> suppliers. There's no requirement that fw needs to create links to ALL
> known suppliers. Just a minor distinction.

That depends on what exactly you mean by "known suppliers".  The
suppliers that are not listed by the firmware are not known at this
point.

> > > + *
> > > + *           Return -ENODEV if and only if the suppliers needed for probing
> > > + *           the device are not yet available to create device links to.
> >
> > It would be more precise to say something like this:
> >
> > "Return -ENODEV if an attempt to create a device link to one of the device's
> > suppliers needed for probing it fails."
>
> "attempt to create a device link to one of the device's suppliers
> needed for probing it fails" to me means device_link_add() fails.
> But I'm trying to say that it should return an error if the struct
> device isn't even there yet.

OK, so it should be something like "if the supplier device has not
been registered yet".

My point is that "not yet available" is kind of ambiguous.

> > > + *
> > > + *           Return -EAGAIN if there are suppliers that need to be linked to
> > > + *           that are not yet available but none of those suppliers are
> > > + *           necessary for probing this device.
> >
> > "Return -EAGAIN if attempts to create device links to some of the device's
> > suppliers have failed, but those suppliers are not necessary for probing the
> > device."
>
> Same comment as before. The distinction I'm making here is that
> -EAGAIN is needed when the struct device itself isn't there.
>
> Btw, Greg already pulled these into driver-core-next. Let me know if
> you want me to send a delta patch to fix any of these comments.

Well, it's a Greg's call if he has taken the patches, but it also
depends on you (if you agree with the comments, it would be prudent to
send updates).

Re: [PATCH v1 3/5] driver core: Allow fwnode_operations.add_links to differentiate errors

[Saravana Kannan]

On Tue, Nov 5, 2019 at 3:07 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Nov 5, 2019 at 11:52 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Hi Rafael,
> >
> > Thanks for the review.
> >
> > On Tue, Nov 5, 2019 at 2:43 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > On Monday, October 28, 2019 11:00:24 PM CET Saravana Kannan wrote:
> > > > When add_links() still has suppliers that it needs to link to in the
> > > > future, this patch allows it to differentiate between suppliers that are
> > > > needed for probing vs suppliers that are needed for sync_state()
> > > > correctness.
> > >
> > > I guess you mean that it will return different error codes in the different
> > > cases.
> >
> > Yes.
> >
> > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/base/core.c    | 12 ++++++++----
> > > >  include/linux/fwnode.h | 13 +++++++++----
> > > >  2 files changed, 17 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index 48cd43a91ce6..e6d3e6d485da 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -2297,7 +2297,7 @@ int device_add(struct device *dev)
> > > >       struct device *parent;
> > > >       struct kobject *kobj;
> > > >       struct class_interface *class_intf;
> > > > -     int error = -EINVAL;
> > > > +     int error = -EINVAL, fw_ret;
> > > >       struct kobject *glue_dir = NULL;
> > > >
> > > >       dev = get_device(dev);
> > > > @@ -2413,9 +2413,13 @@ int device_add(struct device *dev)
> > > >        */
> > > >       device_link_add_missing_supplier_links();
> > > >
> > > > -     if (fwnode_has_op(dev->fwnode, add_links)
> > > > -         && fwnode_call_int_op(dev->fwnode, add_links, dev))
> > > > -             device_link_wait_for_mandatory_supplier(dev, true);
> > > > +     if (fwnode_has_op(dev->fwnode, add_links)) {
> > >
> > > fw_ret can be defined here and I'd just call it "ret".
> >
> > I thought that style of variable declaration is frowned up in the
> > kernel coding style.
>
> Well, I'm not aware of that. :-)

I've definitely seen such comments before. So I'll leave fw_ret as is.
If you and Greg both want to change it to the way you mentioned, I'm
happy to do it.

> > >
> > > > +             fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
> > > > +             if (fw_ret == -ENODEV)
> > > > +                     device_link_wait_for_mandatory_supplier(dev);
> > > > +             else if (fw_ret)
> > > > +                     device_link_wait_for_optional_supplier(dev);
> > > > +     }
> > > >
> > > >       bus_probe_device(dev);
> > > >       if (parent)
> > > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > > > index 25bb81f8ded8..a19134eae5a5 100644
> > > > --- a/include/linux/fwnode.h
> > > > +++ b/include/linux/fwnode.h
> > > > @@ -96,10 +96,15 @@ struct fwnode_reference_args {
> > > >   *           available suppliers.
> > > >   *
> > > >   *           Return 0 if device links have been successfully created to all
> > > > - *           the suppliers of this device or if the supplier information is
> > > > - *           not known. Return an error if and only if the supplier
> > > > - *           information is known but some of the suppliers are not yet
> > > > - *           available to create device links to.
> > > > + *           the suppliers this device needs to create device links to or if
> > > > + *           the supplier information is not known.
> > >
> > > "the known suppliers of this device or if the supplier information is not known."
> >
> > "suppliers it needs to create device links to" is a subset of known
> > suppliers. There's no requirement that fw needs to create links to ALL
> > known suppliers. Just a minor distinction.
>
> That depends on what exactly you mean by "known suppliers".  The
> suppliers that are not listed by the firmware are not known at this
> point.

Ok, I'll rephrase my comment:
"suppliers it needs to create device links to" is a subset of listed
suppliers. There's no requirement that fw needs to create links to ALL
listed suppliers. For example, I can't think of any reason for
sync_state() to be necessary for an interrupt controller driver. So,
fw doesn't need to create device links from consumer to interrupt
supplier. So I'm being more explicit and saying "the suppliers this
device needs to create device links to" instead of "the listed
suppliers of this device".

Long story short, I wrote the comment this way intentionally and
changing it to what you suggest makes it inaccurate IMHO. But I'm open
to other wording suggestions to improve the clarity of this comment.

>
> > > > + *
> > > > + *           Return -ENODEV if and only if the suppliers needed for probing
> > > > + *           the device are not yet available to create device links to.
> > >
> > > It would be more precise to say something like this:
> > >
> > > "Return -ENODEV if an attempt to create a device link to one of the device's
> > > suppliers needed for probing it fails."
> >
> > "attempt to create a device link to one of the device's suppliers
> > needed for probing it fails" to me means device_link_add() fails.
> > But I'm trying to say that it should return an error if the struct
> > device isn't even there yet.
>
> OK, so it should be something like "if the supplier device has not
> been registered yet".
>
> My point is that "not yet available" is kind of ambiguous.

Agree, the latest suggestion sounds better.

> > > > + *
> > > > + *           Return -EAGAIN if there are suppliers that need to be linked to
> > > > + *           that are not yet available but none of those suppliers are
> > > > + *           necessary for probing this device.
> > >
> > > "Return -EAGAIN if attempts to create device links to some of the device's
> > > suppliers have failed, but those suppliers are not necessary for probing the
> > > device."
> >
> > Same comment as before. The distinction I'm making here is that
> > -EAGAIN is needed when the struct device itself isn't there.
> >
> > Btw, Greg already pulled these into driver-core-next. Let me know if
> > you want me to send a delta patch to fix any of these comments.
>
> Well, it's a Greg's call if he has taken the patches, but it also
> depends on you (if you agree with the comments, it would be prudent to
> send updates).

I don't mind sending updates at all. Just trying to make sure I follow
the maintainers' preference in case they don't want trivial (because
my current ones aren't terrible :)) comment update patches.

Once we agree on all the discussion here, I can send an update patch.

Thanks again for your review.

-Saravana

Re: [PATCH v1 2/5] driver core: Allow a device to wait on optional suppliers

[Rafael J. Wysocki]

On Tuesday, November 5, 2019 11:35:28 PM CET Saravana Kannan wrote:
> Looks like I squashed/rebased a bit incorrectly. It's fixed in the
> next patch in the series.

Well, that still is somewhat bisection-unfriendly.

> On Tue, Nov 5, 2019 at 2:29 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Monday, October 28, 2019 11:00:23 PM CET Saravana Kannan wrote:
> > > Before this change, if a device is waiting on suppliers, it's assumed
> > > that all those suppliers are needed for the device to probe
> > > successfully. This change allows marking a devices as waiting only on
> > > optional suppliers. This allows a device to wait on suppliers (and link
> > > to them as soon as they are available) without preventing the device
> > > from being probed.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/base/core.c    | 28 +++++++++++++++++++++++++---
> > >  include/linux/device.h |  3 +++
> > >  2 files changed, 28 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 17ed054c4132..48cd43a91ce6 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -480,13 +480,25 @@ EXPORT_SYMBOL_GPL(device_link_add);
> > >   * 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)
> > > +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
> > > @@ -656,7 +668,8 @@ int device_links_check_suppliers(struct device *dev)
> > >        * probe.
> > >        */
> > >       mutex_lock(&wfs_lock);
> > > -     if (!list_empty(&dev->links.needs_suppliers)) {
> > > +     if (!list_empty(&dev->links.needs_suppliers) &&
> > > +         dev->links.need_for_probe) {
> > >               mutex_unlock(&wfs_lock);
> > >               return -EPROBE_DEFER;
> > >       }
> > > @@ -760,6 +773,15 @@ void device_links_driver_bound(struct device *dev)
> > >  {
> > >       struct device_link *link;
> > >
> > > +     /*
> > > +      * If a device probes successfully, it's expected to have created all
> > > +      * 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);
> > > +
> > >       device_links_write_lock();
> > >
> > >       list_for_each_entry(link, &dev->links.consumers, s_node) {
> > > @@ -2393,7 +2415,7 @@ int device_add(struct device *dev)
> > >
> > >       if (fwnode_has_op(dev->fwnode, add_links)
> > >           && fwnode_call_int_op(dev->fwnode, add_links, dev))
> > > -             device_link_wait_for_supplier(dev);
> > > +             device_link_wait_for_mandatory_supplier(dev, true);
> >
> > Does this compile even?
> >
> > The function takes one argument according to the definition above ...
> >
> > >       bus_probe_device(dev);
> > >       if (parent)
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index f1f2aa0b19da..4fd33da9a848 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -1156,6 +1156,8 @@ enum dl_dev_state {
> > >   * @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 {
> > > @@ -1163,6 +1165,7 @@ struct dev_links_info {
> > >       struct list_head consumers;
> > >       struct list_head needs_suppliers;
> > >       struct list_head defer_sync;
> > > +     bool need_for_probe;
> > >       enum dl_dev_state status;
> > >  };
> > >
> > >
> >
> >
> >
> >
> 

Re: [PATCH v1 2/5] driver core: Allow a device to wait on optional suppliers

[Saravana Kannan]

On Thu, Nov 7, 2019 at 4:05 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Tuesday, November 5, 2019 11:35:28 PM CET Saravana Kannan wrote:
> > Looks like I squashed/rebased a bit incorrectly. It's fixed in the
> > next patch in the series.
>
> Well, that still is somewhat bisection-unfriendly.

Agreed. I was just answering why it compiled fine. Sorry about the
screw up. Any sorry about the accidental top posting last time.

-Saravana

>
>
> > On Tue, Nov 5, 2019 at 2:29 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > On Monday, October 28, 2019 11:00:23 PM CET Saravana Kannan wrote:
> > > > Before this change, if a device is waiting on suppliers, it's assumed
> > > > that all those suppliers are needed for the device to probe
> > > > successfully. This change allows marking a devices as waiting only on
> > > > optional suppliers. This allows a device to wait on suppliers (and link
> > > > to them as soon as they are available) without preventing the device
> > > > from being probed.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/base/core.c    | 28 +++++++++++++++++++++++++---
> > > >  include/linux/device.h |  3 +++
> > > >  2 files changed, 28 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index 17ed054c4132..48cd43a91ce6 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -480,13 +480,25 @@ EXPORT_SYMBOL_GPL(device_link_add);
> > > >   * 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)
> > > > +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
> > > > @@ -656,7 +668,8 @@ int device_links_check_suppliers(struct device *dev)
> > > >        * probe.
> > > >        */
> > > >       mutex_lock(&wfs_lock);
> > > > -     if (!list_empty(&dev->links.needs_suppliers)) {
> > > > +     if (!list_empty(&dev->links.needs_suppliers) &&
> > > > +         dev->links.need_for_probe) {
> > > >               mutex_unlock(&wfs_lock);
> > > >               return -EPROBE_DEFER;
> > > >       }
> > > > @@ -760,6 +773,15 @@ void device_links_driver_bound(struct device *dev)
> > > >  {
> > > >       struct device_link *link;
> > > >
> > > > +     /*
> > > > +      * If a device probes successfully, it's expected to have created all
> > > > +      * 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);
> > > > +
> > > >       device_links_write_lock();
> > > >
> > > >       list_for_each_entry(link, &dev->links.consumers, s_node) {
> > > > @@ -2393,7 +2415,7 @@ int device_add(struct device *dev)
> > > >
> > > >       if (fwnode_has_op(dev->fwnode, add_links)
> > > >           && fwnode_call_int_op(dev->fwnode, add_links, dev))
> > > > -             device_link_wait_for_supplier(dev);
> > > > +             device_link_wait_for_mandatory_supplier(dev, true);
> > >
> > > Does this compile even?
> > >
> > > The function takes one argument according to the definition above ...
> > >
> > > >       bus_probe_device(dev);
> > > >       if (parent)
> > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > index f1f2aa0b19da..4fd33da9a848 100644
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -1156,6 +1156,8 @@ enum dl_dev_state {
> > > >   * @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 {
> > > > @@ -1163,6 +1165,7 @@ struct dev_links_info {
> > > >       struct list_head consumers;
> > > >       struct list_head needs_suppliers;
> > > >       struct list_head defer_sync;
> > > > +     bool need_for_probe;
> > > >       enum dl_dev_state status;
> > > >  };
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
>
>
>
>

Re: [PATCH v1 3/5] driver core: Allow fwnode_operations.add_links to differentiate errors

[Rafael J. Wysocki]

On Wednesday, November 6, 2019 1:00:18 AM CET Saravana Kannan wrote:
> On Tue, Nov 5, 2019 at 3:07 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Nov 5, 2019 at 11:52 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > Hi Rafael,
> > >
> > > Thanks for the review.
> > >
> > > On Tue, Nov 5, 2019 at 2:43 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > On Monday, October 28, 2019 11:00:24 PM CET Saravana Kannan wrote:
> > > > > When add_links() still has suppliers that it needs to link to in the
> > > > > future, this patch allows it to differentiate between suppliers that are
> > > > > needed for probing vs suppliers that are needed for sync_state()
> > > > > correctness.
> > > >
> > > > I guess you mean that it will return different error codes in the different
> > > > cases.
> > >
> > > Yes.
> > >
> > > >
> > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > ---
> > > > >  drivers/base/core.c    | 12 ++++++++----
> > > > >  include/linux/fwnode.h | 13 +++++++++----
> > > > >  2 files changed, 17 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > index 48cd43a91ce6..e6d3e6d485da 100644
> > > > > --- a/drivers/base/core.c
> > > > > +++ b/drivers/base/core.c
> > > > > @@ -2297,7 +2297,7 @@ int device_add(struct device *dev)
> > > > >       struct device *parent;
> > > > >       struct kobject *kobj;
> > > > >       struct class_interface *class_intf;
> > > > > -     int error = -EINVAL;
> > > > > +     int error = -EINVAL, fw_ret;
> > > > >       struct kobject *glue_dir = NULL;
> > > > >
> > > > >       dev = get_device(dev);
> > > > > @@ -2413,9 +2413,13 @@ int device_add(struct device *dev)
> > > > >        */
> > > > >       device_link_add_missing_supplier_links();
> > > > >
> > > > > -     if (fwnode_has_op(dev->fwnode, add_links)
> > > > > -         && fwnode_call_int_op(dev->fwnode, add_links, dev))
> > > > > -             device_link_wait_for_mandatory_supplier(dev, true);
> > > > > +     if (fwnode_has_op(dev->fwnode, add_links)) {
> > > >
> > > > fw_ret can be defined here and I'd just call it "ret".
> > >
> > > I thought that style of variable declaration is frowned up in the
> > > kernel coding style.
> >
> > Well, I'm not aware of that. :-)
> 
> I've definitely seen such comments before. So I'll leave fw_ret as is.
> If you and Greg both want to change it to the way you mentioned, I'm
> happy to do it.

If this has been committed the way it is, there's not so much of a difference,
but I generally like variables to not be seen out of the scope in which they
are used, as that allows bugs to be caught at compile time sometimes.

> > > >
> > > > > +             fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
> > > > > +             if (fw_ret == -ENODEV)
> > > > > +                     device_link_wait_for_mandatory_supplier(dev);
> > > > > +             else if (fw_ret)
> > > > > +                     device_link_wait_for_optional_supplier(dev);
> > > > > +     }
> > > > >
> > > > >       bus_probe_device(dev);
> > > > >       if (parent)
> > > > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > > > > index 25bb81f8ded8..a19134eae5a5 100644
> > > > > --- a/include/linux/fwnode.h
> > > > > +++ b/include/linux/fwnode.h
> > > > > @@ -96,10 +96,15 @@ struct fwnode_reference_args {
> > > > >   *           available suppliers.
> > > > >   *
> > > > >   *           Return 0 if device links have been successfully created to all
> > > > > - *           the suppliers of this device or if the supplier information is
> > > > > - *           not known. Return an error if and only if the supplier
> > > > > - *           information is known but some of the suppliers are not yet
> > > > > - *           available to create device links to.
> > > > > + *           the suppliers this device needs to create device links to or if
> > > > > + *           the supplier information is not known.
> > > >
> > > > "the known suppliers of this device or if the supplier information is not known."
> > >
> > > "suppliers it needs to create device links to" is a subset of known
> > > suppliers. There's no requirement that fw needs to create links to ALL
> > > known suppliers. Just a minor distinction.
> >
> > That depends on what exactly you mean by "known suppliers".  The
> > suppliers that are not listed by the firmware are not known at this
> > point.
> 
> Ok, I'll rephrase my comment:
> "suppliers it needs to create device links to" is a subset of listed
> suppliers. There's no requirement that fw needs to create links to ALL
> listed suppliers. For example, I can't think of any reason for
> sync_state() to be necessary for an interrupt controller driver.

A sync_state() may not be, but it may be a good idea to create device links
to the controller device from all devices that rely on it, so as to ensure
the right system suspend/resume ordering if nothing else.

> So, fw doesn't need to create device links from consumer to interrupt
> supplier. So I'm being more explicit and saying "the suppliers this
> device needs to create device links to" instead of "the listed
> suppliers of this device".

This gives me the feeling of splitting hairs to be honest. :-)

In fact, the FW indicates to the OS that there are some dependencies (either
hard or soft) between devices and adding device links is a way to act on that
information.

The "device link" notion is not actually defined at the FW level.  What it
knows about is a "probe dependency" or an "ordering constraint" which then
is represented by a device link at the OS level.

> Long story short, I wrote the comment this way intentionally and
> changing it to what you suggest makes it inaccurate IMHO. But I'm open
> to other wording suggestions to improve the clarity of this comment.

My point basically is that the way you phrased it may lead to some confusion
(regardless of whether or not it is intentional).

> >
> > > > > + *
> > > > > + *           Return -ENODEV if and only if the suppliers needed for probing
> > > > > + *           the device are not yet available to create device links to.
> > > >
> > > > It would be more precise to say something like this:
> > > >
> > > > "Return -ENODEV if an attempt to create a device link to one of the device's
> > > > suppliers needed for probing it fails."
> > >
> > > "attempt to create a device link to one of the device's suppliers
> > > needed for probing it fails" to me means device_link_add() fails.
> > > But I'm trying to say that it should return an error if the struct
> > > device isn't even there yet.
> >
> > OK, so it should be something like "if the supplier device has not
> > been registered yet".
> >
> > My point is that "not yet available" is kind of ambiguous.
> 
> Agree, the latest suggestion sounds better.
> 
> > > > > + *
> > > > > + *           Return -EAGAIN if there are suppliers that need to be linked to
> > > > > + *           that are not yet available but none of those suppliers are
> > > > > + *           necessary for probing this device.
> > > >
> > > > "Return -EAGAIN if attempts to create device links to some of the device's
> > > > suppliers have failed, but those suppliers are not necessary for probing the
> > > > device."
> > >
> > > Same comment as before. The distinction I'm making here is that
> > > -EAGAIN is needed when the struct device itself isn't there.
> > >
> > > Btw, Greg already pulled these into driver-core-next. Let me know if
> > > you want me to send a delta patch to fix any of these comments.
> >
> > Well, it's a Greg's call if he has taken the patches, but it also
> > depends on you (if you agree with the comments, it would be prudent to
> > send updates).
> 
> I don't mind sending updates at all. Just trying to make sure I follow
> the maintainers' preference in case they don't want trivial (because
> my current ones aren't terrible :)) comment update patches.

If it can be improved, then improve it.  Worst case you can hear from the
maintainers that they don't agree with the proposed changes.

Re: [PATCH v1 3/5] driver core: Allow fwnode_operations.add_links to differentiate errors

[Saravana Kannan]

On Thu, Nov 7, 2019 at 4:35 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, November 6, 2019 1:00:18 AM CET Saravana Kannan wrote:
> > On Tue, Nov 5, 2019 at 3:07 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Tue, Nov 5, 2019 at 11:52 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > Hi Rafael,
> > > >
> > > > Thanks for the review.
> > > >
> > > > On Tue, Nov 5, 2019 at 2:43 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > >
> > > > > On Monday, October 28, 2019 11:00:24 PM CET Saravana Kannan wrote:
> > > > > > When add_links() still has suppliers that it needs to link to in the
> > > > > > future, this patch allows it to differentiate between suppliers that are
> > > > > > needed for probing vs suppliers that are needed for sync_state()
> > > > > > correctness.
> > > > >
> > > > > I guess you mean that it will return different error codes in the different
> > > > > cases.
> > > >
> > > > Yes.
> > > >
> > > > >
> > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > ---
> > > > > >  drivers/base/core.c    | 12 ++++++++----
> > > > > >  include/linux/fwnode.h | 13 +++++++++----
> > > > > >  2 files changed, 17 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > > index 48cd43a91ce6..e6d3e6d485da 100644
> > > > > > --- a/drivers/base/core.c
> > > > > > +++ b/drivers/base/core.c
> > > > > > @@ -2297,7 +2297,7 @@ int device_add(struct device *dev)
> > > > > >       struct device *parent;
> > > > > >       struct kobject *kobj;
> > > > > >       struct class_interface *class_intf;
> > > > > > -     int error = -EINVAL;
> > > > > > +     int error = -EINVAL, fw_ret;
> > > > > >       struct kobject *glue_dir = NULL;
> > > > > >
> > > > > >       dev = get_device(dev);
> > > > > > @@ -2413,9 +2413,13 @@ int device_add(struct device *dev)
> > > > > >        */
> > > > > >       device_link_add_missing_supplier_links();
> > > > > >
> > > > > > -     if (fwnode_has_op(dev->fwnode, add_links)
> > > > > > -         && fwnode_call_int_op(dev->fwnode, add_links, dev))
> > > > > > -             device_link_wait_for_mandatory_supplier(dev, true);
> > > > > > +     if (fwnode_has_op(dev->fwnode, add_links)) {
> > > > >
> > > > > fw_ret can be defined here and I'd just call it "ret".
> > > >
> > > > I thought that style of variable declaration is frowned up in the
> > > > kernel coding style.
> > >
> > > Well, I'm not aware of that. :-)
> >
> > I've definitely seen such comments before. So I'll leave fw_ret as is.
> > If you and Greg both want to change it to the way you mentioned, I'm
> > happy to do it.
>
> If this has been committed the way it is, there's not so much of a difference,
> but I generally like variables to not be seen out of the scope in which they
> are used, as that allows bugs to be caught at compile time sometimes.

Ok, I don't have a strong preference either way. My understanding is
that Greg doesn't like declaring variables "in the middle". So I'm
going to leave it as is unless I'm corrected.

>
> > > > >
> > > > > > +             fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
> > > > > > +             if (fw_ret == -ENODEV)
> > > > > > +                     device_link_wait_for_mandatory_supplier(dev);
> > > > > > +             else if (fw_ret)
> > > > > > +                     device_link_wait_for_optional_supplier(dev);
> > > > > > +     }
> > > > > >
> > > > > >       bus_probe_device(dev);
> > > > > >       if (parent)
> > > > > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > > > > > index 25bb81f8ded8..a19134eae5a5 100644
> > > > > > --- a/include/linux/fwnode.h
> > > > > > +++ b/include/linux/fwnode.h
> > > > > > @@ -96,10 +96,15 @@ struct fwnode_reference_args {
> > > > > >   *           available suppliers.
> > > > > >   *
> > > > > >   *           Return 0 if device links have been successfully created to all
> > > > > > - *           the suppliers of this device or if the supplier information is
> > > > > > - *           not known. Return an error if and only if the supplier
> > > > > > - *           information is known but some of the suppliers are not yet
> > > > > > - *           available to create device links to.
> > > > > > + *           the suppliers this device needs to create device links to or if
> > > > > > + *           the supplier information is not known.
> > > > >
> > > > > "the known suppliers of this device or if the supplier information is not known."
> > > >
> > > > "suppliers it needs to create device links to" is a subset of known
> > > > suppliers. There's no requirement that fw needs to create links to ALL
> > > > known suppliers. Just a minor distinction.
> > >
> > > That depends on what exactly you mean by "known suppliers".  The
> > > suppliers that are not listed by the firmware are not known at this
> > > point.
> >
> > Ok, I'll rephrase my comment:
> > "suppliers it needs to create device links to" is a subset of listed
> > suppliers. There's no requirement that fw needs to create links to ALL
> > listed suppliers. For example, I can't think of any reason for
> > sync_state() to be necessary for an interrupt controller driver.
>
> A sync_state() may not be, but it may be a good idea to create device links
> to the controller device from all devices that rely on it, so as to ensure
> the right system suspend/resume ordering if nothing else.

True. Frameworks are starting to add device links when consumers make
calls to "get()" the resource. But if firmware can add it ahead of
time, it doesn't hurt I suppose and it'll also cover for frameworks
without device links support yet.

> > So, fw doesn't need to create device links from consumer to interrupt
> > supplier. So I'm being more explicit and saying "the suppliers this
> > device needs to create device links to" instead of "the listed
> > suppliers of this device".
>
> This gives me the feeling of splitting hairs to be honest. :-)

Maybe, but it's coming from a point of goodwill :) I just want to
capture the intent as narrowly and accurately as possible.

> In fact, the FW indicates to the OS that there are some dependencies (either
> hard or soft) between devices and adding device links is a way to act on that
> information.
>
> The "device link" notion is not actually defined at the FW level.  What it
> knows about is a "probe dependency" or an "ordering constraint" which then
> is represented by a device link at the OS level.

And you've convinced me why it doesn't have to be as narrow as I intended.

> > Long story short, I wrote the comment this way intentionally and
> > changing it to what you suggest makes it inaccurate IMHO. But I'm open
> > to other wording suggestions to improve the clarity of this comment.
>
> My point basically is that the way you phrased it may lead to some confusion
> (regardless of whether or not it is intentional).

Ok, I think we are on the same page now. I'll send an update soon.

>
> > >
> > > > > > + *
> > > > > > + *           Return -ENODEV if and only if the suppliers needed for probing
> > > > > > + *           the device are not yet available to create device links to.
> > > > >
> > > > > It would be more precise to say something like this:
> > > > >
> > > > > "Return -ENODEV if an attempt to create a device link to one of the device's
> > > > > suppliers needed for probing it fails."
> > > >
> > > > "attempt to create a device link to one of the device's suppliers
> > > > needed for probing it fails" to me means device_link_add() fails.
> > > > But I'm trying to say that it should return an error if the struct
> > > > device isn't even there yet.
> > >
> > > OK, so it should be something like "if the supplier device has not
> > > been registered yet".
> > >
> > > My point is that "not yet available" is kind of ambiguous.
> >
> > Agree, the latest suggestion sounds better.
> >
> > > > > > + *
> > > > > > + *           Return -EAGAIN if there are suppliers that need to be linked to
> > > > > > + *           that are not yet available but none of those suppliers are
> > > > > > + *           necessary for probing this device.
> > > > >
> > > > > "Return -EAGAIN if attempts to create device links to some of the device's
> > > > > suppliers have failed, but those suppliers are not necessary for probing the
> > > > > device."
> > > >
> > > > Same comment as before. The distinction I'm making here is that
> > > > -EAGAIN is needed when the struct device itself isn't there.
> > > >
> > > > Btw, Greg already pulled these into driver-core-next. Let me know if
> > > > you want me to send a delta patch to fix any of these comments.
> > >
> > > Well, it's a Greg's call if he has taken the patches, but it also
> > > depends on you (if you agree with the comments, it would be prudent to
> > > send updates).
> >
> > I don't mind sending updates at all. Just trying to make sure I follow
> > the maintainers' preference in case they don't want trivial (because
> > my current ones aren't terrible :)) comment update patches.
>
> If it can be improved, then improve it.  Worst case you can hear from the
> maintainers that they don't agree with the proposed changes.

Eh, I've been yelled at a couple of times for sending patches too soon
or too many. So just playing it safe.

-Saravana