diff mbox series

[v2,06/11] driver core: fw_devlink: Allow marking a fwnode link as being part of a cycle

Message ID 20230127001141.407071-7-saravanak@google.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series fw_devlink improvements | expand

Commit Message

Saravana Kannan Jan. 27, 2023, 12:11 a.m. UTC
To improve detection and handling of dependency cycles, we need to be
able to mark fwnode links as being part of cycles. fwnode links marked
as being part of a cycle should not block their consumers from probing.

Fixes: 2de9d8e0d2fe ("driver core: fw_devlink: Improve handling of cyclic dependencies")
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 41 +++++++++++++++++++++++++++++++++++------
 include/linux/fwnode.h | 11 ++++++++++-
 2 files changed, 45 insertions(+), 7 deletions(-)

Comments

Andy Shevchenko Jan. 27, 2023, 9:33 a.m. UTC | #1
On Thu, Jan 26, 2023 at 04:11:33PM -0800, Saravana Kannan wrote:
> To improve detection and handling of dependency cycles, we need to be
> able to mark fwnode links as being part of cycles. fwnode links marked
> as being part of a cycle should not block their consumers from probing.

...

> +	list_for_each_entry(link, &fwnode->suppliers, c_hook) {
> +		if (link->flags & FWLINK_FLAG_CYCLE)
> +			continue;
> +		return link->supplier;

Hmm...

		if (!(link->flags & FWLINK_FLAG_CYCLE))
			return link->supplier;

?

> +	}
> +
> +	return NULL;

...

> -	if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> -	    !fw_devlink_is_permissive()) {
> -		sup_fw = list_first_entry(&dev->fwnode->suppliers,
> -					  struct fwnode_link,
> -					  c_hook)->supplier;
> +	sup_fw = fwnode_links_check_suppliers(dev->fwnode);

dev_fwnode() ?

...

> -	val = !list_empty(&dev->fwnode->suppliers);
> +	mutex_lock(&fwnode_link_lock);
> +	val = !!fwnode_links_check_suppliers(dev->fwnode);

Ditto?

> +	mutex_unlock(&fwnode_link_lock);
Saravana Kannan Jan. 28, 2023, 7:34 a.m. UTC | #2
On Fri, Jan 27, 2023 at 1:33 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jan 26, 2023 at 04:11:33PM -0800, Saravana Kannan wrote:
> > To improve detection and handling of dependency cycles, we need to be
> > able to mark fwnode links as being part of cycles. fwnode links marked
> > as being part of a cycle should not block their consumers from probing.
>
> ...
>
> > +     list_for_each_entry(link, &fwnode->suppliers, c_hook) {
> > +             if (link->flags & FWLINK_FLAG_CYCLE)
> > +                     continue;
> > +             return link->supplier;
>
> Hmm...

Thanks!

>
>                 if (!(link->flags & FWLINK_FLAG_CYCLE))
>                         return link->supplier;
>
> ?
>
> > +     }
> > +
> > +     return NULL;
>
> ...
>
> > -     if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> > -         !fw_devlink_is_permissive()) {
> > -             sup_fw = list_first_entry(&dev->fwnode->suppliers,
> > -                                       struct fwnode_link,
> > -                                       c_hook)->supplier;
> > +     sup_fw = fwnode_links_check_suppliers(dev->fwnode);
>
> dev_fwnode() ?
>
> ...
>
> > -     val = !list_empty(&dev->fwnode->suppliers);
> > +     mutex_lock(&fwnode_link_lock);
> > +     val = !!fwnode_links_check_suppliers(dev->fwnode);
>
> Ditto?

Similar response as Patch 1 and Patch 4.


-Saravana
Andy Shevchenko Jan. 30, 2023, 12:09 p.m. UTC | #3
On Fri, Jan 27, 2023 at 11:34:19PM -0800, Saravana Kannan wrote:
> On Fri, Jan 27, 2023 at 1:33 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Jan 26, 2023 at 04:11:33PM -0800, Saravana Kannan wrote:

...

> > > -     if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> > > -         !fw_devlink_is_permissive()) {
> > > -             sup_fw = list_first_entry(&dev->fwnode->suppliers,
> > > -                                       struct fwnode_link,
> > > -                                       c_hook)->supplier;
> > > +     sup_fw = fwnode_links_check_suppliers(dev->fwnode);
> >
> > dev_fwnode() ?
> >
> > ...
> >
> > > -     val = !list_empty(&dev->fwnode->suppliers);
> > > +     mutex_lock(&fwnode_link_lock);
> > > +     val = !!fwnode_links_check_suppliers(dev->fwnode);
> >
> > Ditto?
> 
> Similar response as Patch 1 and Patch 4.

Same.
diff mbox series

Patch

diff --git a/drivers/base/core.c b/drivers/base/core.c
index e5390b09a02f..82b29e9070bf 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -126,6 +126,19 @@  static void __fwnode_link_del(struct fwnode_link *link)
 	kfree(link);
 }
 
+/**
+ * __fwnode_link_cycle - Mark a fwnode link as being part of a cycle.
+ * @link: the fwnode_link to be marked
+ *
+ * The fwnode_link_lock needs to be held when this function is called.
+ */
+static void __fwnode_link_cycle(struct fwnode_link *link)
+{
+	pr_debug("%pfwf: Relaxing link with %pfwf\n",
+		 link->consumer, link->supplier);
+	link->flags |= FWLINK_FLAG_CYCLE;
+}
+
 /**
  * fwnode_links_purge_suppliers - Delete all supplier links of fwnode_handle.
  * @fwnode: fwnode whose supplier links need to be deleted
@@ -1041,6 +1054,23 @@  static bool dev_is_best_effort(struct device *dev)
 		(dev->fwnode && (dev->fwnode->flags & FWNODE_FLAG_BEST_EFFORT));
 }
 
+static struct fwnode_handle *fwnode_links_check_suppliers(
+						struct fwnode_handle *fwnode)
+{
+	struct fwnode_link *link;
+
+	if (!fwnode || fw_devlink_is_permissive())
+		return NULL;
+
+	list_for_each_entry(link, &fwnode->suppliers, c_hook) {
+		if (link->flags & FWLINK_FLAG_CYCLE)
+			continue;
+		return link->supplier;
+	}
+
+	return NULL;
+}
+
 /**
  * device_links_check_suppliers - Check presence of supplier drivers.
  * @dev: Consumer device.
@@ -1068,11 +1098,8 @@  int device_links_check_suppliers(struct device *dev)
 	 * probe.
 	 */
 	mutex_lock(&fwnode_link_lock);
-	if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
-	    !fw_devlink_is_permissive()) {
-		sup_fw = list_first_entry(&dev->fwnode->suppliers,
-					  struct fwnode_link,
-					  c_hook)->supplier;
+	sup_fw = fwnode_links_check_suppliers(dev->fwnode);
+	if (sup_fw) {
 		if (!dev_is_best_effort(dev)) {
 			fwnode_ret = -EPROBE_DEFER;
 			dev_err_probe(dev, -EPROBE_DEFER,
@@ -1261,7 +1288,9 @@  static ssize_t waiting_for_supplier_show(struct device *dev,
 	bool val;
 
 	device_lock(dev);
-	val = !list_empty(&dev->fwnode->suppliers);
+	mutex_lock(&fwnode_link_lock);
+	val = !!fwnode_links_check_suppliers(dev->fwnode);
+	mutex_unlock(&fwnode_link_lock);
 	device_unlock(dev);
 	return sysfs_emit(buf, "%u\n", val);
 }
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 89b9bdfca925..fdf2ee0285b7 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -18,7 +18,7 @@  struct fwnode_operations;
 struct device;
 
 /*
- * fwnode link flags
+ * fwnode flags
  *
  * LINKS_ADDED:	The fwnode has already be parsed to add fwnode links.
  * NOT_DEVICE:	The fwnode will never be populated as a struct device.
@@ -36,6 +36,7 @@  struct device;
 #define FWNODE_FLAG_INITIALIZED			BIT(2)
 #define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD	BIT(3)
 #define FWNODE_FLAG_BEST_EFFORT			BIT(4)
+#define FWNODE_FLAG_VISITED			BIT(5)
 
 struct fwnode_handle {
 	struct fwnode_handle *secondary;
@@ -46,11 +47,19 @@  struct fwnode_handle {
 	u8 flags;
 };
 
+/*
+ * fwnode link flags
+ *
+ * CYCLE:	The fwnode link is part of a cycle. Don't defer probe.
+ */
+#define FWLINK_FLAG_CYCLE			BIT(0)
+
 struct fwnode_link {
 	struct fwnode_handle *supplier;
 	struct list_head s_hook;
 	struct fwnode_handle *consumer;
 	struct list_head c_hook;
+	u8 flags;
 };
 
 /**