diff mbox series

[v2,05/11] driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links

Message ID 20230127001141.407071-6-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
fw_devlink uses DL_FLAG_SYNC_STATE_ONLY device link flag for two
purposes:

1. To allow a parent device to proxy its child device's dependency on a
   supplier so that the supplier doesn't get its sync_state() callback
   before the child device/consumer can be added and probed. In this
   usage scenario, we need to ignore cycles for ensure correctness of
   sync_state() callbacks.

2. When there are dependency cycles in firmware, we don't know which of
   those dependencies are valid. So, we have to ignore them all wrt
   probe ordering while still making sure the sync_state() callbacks
   come correctly.

However, when detecting dependency cycles, there can be multiple
dependency cycles between two devices that we need to detect. For
example:

A -> B -> A and A -> C -> B -> A.

To detect multiple cycles correct, we need to be able to differentiate
DL_FLAG_SYNC_STATE_ONLY device links used for (1) vs (2) above.

To allow this differentiation, add a DL_FLAG_CYCLE that can be use to
mark use case (2). We can then use the DL_FLAG_CYCLE to decide which
DL_FLAG_SYNC_STATE_ONLY device links to follow when looking for
dependency cycles.

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

Comments

Andy Shevchenko Jan. 27, 2023, 9:29 a.m. UTC | #1
On Thu, Jan 26, 2023 at 04:11:32PM -0800, Saravana Kannan wrote:
> fw_devlink uses DL_FLAG_SYNC_STATE_ONLY device link flag for two
> purposes:
> 
> 1. To allow a parent device to proxy its child device's dependency on a
>    supplier so that the supplier doesn't get its sync_state() callback
>    before the child device/consumer can be added and probed. In this
>    usage scenario, we need to ignore cycles for ensure correctness of
>    sync_state() callbacks.
> 
> 2. When there are dependency cycles in firmware, we don't know which of
>    those dependencies are valid. So, we have to ignore them all wrt
>    probe ordering while still making sure the sync_state() callbacks
>    come correctly.
> 
> However, when detecting dependency cycles, there can be multiple
> dependency cycles between two devices that we need to detect. For
> example:
> 
> A -> B -> A and A -> C -> B -> A.
> 
> To detect multiple cycles correct, we need to be able to differentiate
> DL_FLAG_SYNC_STATE_ONLY device links used for (1) vs (2) above.
> 
> To allow this differentiation, add a DL_FLAG_CYCLE that can be use to
> mark use case (2). We can then use the DL_FLAG_CYCLE to decide which
> DL_FLAG_SYNC_STATE_ONLY device links to follow when looking for
> dependency cycles.

...

> +static inline bool device_link_flag_is_sync_state_only(u32 flags)
> +{
> +	return (flags & ~(DL_FLAG_INFERRED | DL_FLAG_CYCLE))
> +		== (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED);

Weird indentation, why not

	return (flags & ~(DL_FLAG_INFERRED | DL_FLAG_CYCLE)) ==
	       (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED);

?

> +}

...

>  			       DL_FLAG_AUTOREMOVE_SUPPLIER | \
>  			       DL_FLAG_AUTOPROBE_CONSUMER  | \
>  			       DL_FLAG_SYNC_STATE_ONLY | \
> -			       DL_FLAG_INFERRED)
> +			       DL_FLAG_INFERRED | \
> +			       DL_FLAG_CYCLE)

You can make less churn by squeezing the new one above the last one.
Andy Shevchenko Jan. 27, 2023, 9:30 a.m. UTC | #2
On Fri, Jan 27, 2023 at 11:29:43AM +0200, Andy Shevchenko wrote:
> On Thu, Jan 26, 2023 at 04:11:32PM -0800, Saravana Kannan wrote:

...

> >  			       DL_FLAG_AUTOREMOVE_SUPPLIER | \
> >  			       DL_FLAG_AUTOPROBE_CONSUMER  | \
> >  			       DL_FLAG_SYNC_STATE_ONLY | \
> > -			       DL_FLAG_INFERRED)
> > +			       DL_FLAG_INFERRED | \
> > +			       DL_FLAG_CYCLE)
> 
> You can make less churn by squeezing the new one above the last one.

Or even define a mask with all necessary bits in the header and use it.
Geert Uytterhoeven Jan. 27, 2023, 9:55 a.m. UTC | #3
On Fri, Jan 27, 2023 at 10:30 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, Jan 26, 2023 at 04:11:32PM -0800, Saravana Kannan wrote:

> >                              DL_FLAG_AUTOREMOVE_SUPPLIER | \
> >                              DL_FLAG_AUTOPROBE_CONSUMER  | \
> >                              DL_FLAG_SYNC_STATE_ONLY | \
> > -                            DL_FLAG_INFERRED)
> > +                            DL_FLAG_INFERRED | \
> > +                            DL_FLAG_CYCLE)
>
> You can make less churn by squeezing the new one above the last one.

And avoiding some future churn by introducing alphabetical order.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Saravana Kannan Jan. 28, 2023, 7:34 a.m. UTC | #4
On Fri, Jan 27, 2023 at 1:30 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jan 26, 2023 at 04:11:32PM -0800, Saravana Kannan wrote:
> > fw_devlink uses DL_FLAG_SYNC_STATE_ONLY device link flag for two
> > purposes:
> >
> > 1. To allow a parent device to proxy its child device's dependency on a
> >    supplier so that the supplier doesn't get its sync_state() callback
> >    before the child device/consumer can be added and probed. In this
> >    usage scenario, we need to ignore cycles for ensure correctness of
> >    sync_state() callbacks.
> >
> > 2. When there are dependency cycles in firmware, we don't know which of
> >    those dependencies are valid. So, we have to ignore them all wrt
> >    probe ordering while still making sure the sync_state() callbacks
> >    come correctly.
> >
> > However, when detecting dependency cycles, there can be multiple
> > dependency cycles between two devices that we need to detect. For
> > example:
> >
> > A -> B -> A and A -> C -> B -> A.
> >
> > To detect multiple cycles correct, we need to be able to differentiate
> > DL_FLAG_SYNC_STATE_ONLY device links used for (1) vs (2) above.
> >
> > To allow this differentiation, add a DL_FLAG_CYCLE that can be use to
> > mark use case (2). We can then use the DL_FLAG_CYCLE to decide which
> > DL_FLAG_SYNC_STATE_ONLY device links to follow when looking for
> > dependency cycles.
>
> ...
>
> > +static inline bool device_link_flag_is_sync_state_only(u32 flags)
> > +{
> > +     return (flags & ~(DL_FLAG_INFERRED | DL_FLAG_CYCLE))
> > +             == (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED);
>
> Weird indentation, why not
>
>         return (flags & ~(DL_FLAG_INFERRED | DL_FLAG_CYCLE)) ==
>                (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED);
>
> ?

Ack. Will fix in v3.

>
> > +}
>
> ...
>
> >                              DL_FLAG_AUTOREMOVE_SUPPLIER | \
> >                              DL_FLAG_AUTOPROBE_CONSUMER  | \
> >                              DL_FLAG_SYNC_STATE_ONLY | \
> > -                            DL_FLAG_INFERRED)
> > +                            DL_FLAG_INFERRED | \
> > +                            DL_FLAG_CYCLE)
>
> You can make less churn by squeezing the new one above the last one.

I feel like this part is getting bike shedded. I'm going to leave it
as is. It's done in the order it's defined in the header and keeping
it that way makes it way more easier to read than worry about a single
line churn.


-Saravana

>
> --
> With Best Regards,
> Andy Shevchenko
>
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>
Andy Shevchenko Jan. 30, 2023, 12:08 p.m. UTC | #5
On Fri, Jan 27, 2023 at 11:34:11PM -0800, Saravana Kannan wrote:
> On Fri, Jan 27, 2023 at 1:30 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Jan 26, 2023 at 04:11:32PM -0800, Saravana Kannan wrote:

...

> > >                              DL_FLAG_AUTOREMOVE_SUPPLIER | \
> > >                              DL_FLAG_AUTOPROBE_CONSUMER  | \
> > >                              DL_FLAG_SYNC_STATE_ONLY | \
> > > -                            DL_FLAG_INFERRED)
> > > +                            DL_FLAG_INFERRED | \
> > > +                            DL_FLAG_CYCLE)
> >
> > You can make less churn by squeezing the new one above the last one.
> 
> I feel like this part is getting bike shedded. I'm going to leave it
> as is. It's done in the order it's defined in the header and keeping
> it that way makes it way more easier to read than worry about a single
> line churn.

Not at all. What you are showing here is the additional churn for the sake of
the churn. With a no-cost trick you may avoid that.

Also, it will compress better the Git index and save some mW here and there
and in the size of the world and amount of Git copies of the Linux kernel
this may save the planet (I'm serious, no jokes).
diff mbox series

Patch

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 919728e784e8..e5390b09a02f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -322,6 +322,12 @@  static bool device_is_ancestor(struct device *dev, struct device *target)
 	return false;
 }
 
+static inline bool device_link_flag_is_sync_state_only(u32 flags)
+{
+	return (flags & ~(DL_FLAG_INFERRED | DL_FLAG_CYCLE))
+		== (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED);
+}
+
 /**
  * device_is_dependent - Check if one device depends on another one
  * @dev: Device to check dependencies for.
@@ -348,8 +354,7 @@  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_INFERRED) ==
-		    (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED))
+		if (device_link_flag_is_sync_state_only(link->flags))
 			continue;
 
 		if (link->consumer == target)
@@ -422,8 +427,7 @@  static int device_reorder_to_tail(struct device *dev, void *not_used)
 
 	device_for_each_child(dev, NULL, device_reorder_to_tail);
 	list_for_each_entry(link, &dev->links.consumers, s_node) {
-		if ((link->flags & ~DL_FLAG_INFERRED) ==
-		    (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED))
+		if (device_link_flag_is_sync_state_only(link->flags))
 			continue;
 		device_reorder_to_tail(link->consumer, NULL);
 	}
@@ -684,7 +688,8 @@  postcore_initcall(devlink_class_init);
 			       DL_FLAG_AUTOREMOVE_SUPPLIER | \
 			       DL_FLAG_AUTOPROBE_CONSUMER  | \
 			       DL_FLAG_SYNC_STATE_ONLY | \
-			       DL_FLAG_INFERRED)
+			       DL_FLAG_INFERRED | \
+			       DL_FLAG_CYCLE)
 
 #define DL_ADD_VALID_FLAGS (DL_MANAGED_LINK_FLAGS | DL_FLAG_STATELESS | \
 			    DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE)
@@ -753,8 +758,6 @@  struct device_link *device_link_add(struct device *consumer,
 	if (!consumer || !supplier || 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_INFERRED) != DL_FLAG_SYNC_STATE_ONLY) ||
 	    (flags & DL_FLAG_AUTOPROBE_CONSUMER &&
 	     flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
 		      DL_FLAG_AUTOREMOVE_SUPPLIER)))
@@ -770,6 +773,10 @@  struct device_link *device_link_add(struct device *consumer,
 	if (!(flags & DL_FLAG_STATELESS))
 		flags |= DL_FLAG_MANAGED;
 
+	if (flags & DL_FLAG_SYNC_STATE_ONLY &&
+	    !device_link_flag_is_sync_state_only(flags))
+		return NULL;
+
 	device_links_write_lock();
 	device_pm_lock();
 
@@ -1729,7 +1736,7 @@  static void fw_devlink_relax_link(struct device_link *link)
 	if (!(link->flags & DL_FLAG_INFERRED))
 		return;
 
-	if (link->flags == (DL_FLAG_MANAGED | FW_DEVLINK_FLAGS_PERMISSIVE))
+	if (device_link_flag_is_sync_state_only(link->flags))
 		return;
 
 	pm_runtime_drop_link(link);
@@ -1853,8 +1860,8 @@  static int fw_devlink_relax_cycle(struct device *con, void *sup)
 		return ret;
 
 	list_for_each_entry(link, &con->links.consumers, s_node) {
-		if ((link->flags & ~DL_FLAG_INFERRED) ==
-		    (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED))
+		if (!(link->flags & DL_FLAG_CYCLE) &&
+		    device_link_flag_is_sync_state_only(link->flags))
 			continue;
 
 		if (!fw_devlink_relax_cycle(link->consumer, sup))
@@ -1863,6 +1870,7 @@  static int fw_devlink_relax_cycle(struct device *con, void *sup)
 		ret = 1;
 
 		fw_devlink_relax_link(link);
+		link->flags |= DL_FLAG_CYCLE;
 	}
 	return ret;
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index 44e3acae7b36..f4d20655d2d7 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -328,6 +328,7 @@  enum device_link_state {
 #define DL_FLAG_MANAGED			BIT(6)
 #define DL_FLAG_SYNC_STATE_ONLY		BIT(7)
 #define DL_FLAG_INFERRED		BIT(8)
+#define DL_FLAG_CYCLE			BIT(9)
 
 /**
  * enum dl_dev_state - Device driver presence tracking information.