Re: [PATCH 5.15 330/567] driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 7, 2023 at 11:02 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> From: Saravana Kannan <saravanak@xxxxxxxxxx>
>
> [ Upstream commit 67cad5c67019c38126b749621665b6723d3ae7e6 ]
>
> 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@xxxxxxxxxx>
> Tested-by: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> Tested-by: Sudeep Holla <sudeep.holla@xxxxxxx>
> Tested-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Tested-by: Luca Weiss <luca.weiss@xxxxxxxxxxxxx> # qcom/sm7225-fairphone-fp4
> Link: https://lore.kernel.org/r/20230207014207.1678715-6-saravanak@xxxxxxxxxx
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---

These fw_devlink patches weren't tested with 5.15. It's also an
extensive refactor. So I'm a little worried if it'll be finicky and
break things in a kernel that's a year old.

Unless someone specifically wants these patches in 5.15, I'd prefer we
don't pick it up in 5.15.

-Saravana

>  drivers/base/core.c    | 28 ++++++++++++++++++----------
>  include/linux/device.h |  1 +
>  2 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index adf003a7e8d6a..178a21e985197 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -269,6 +269,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.
> @@ -295,8 +301,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)
> @@ -369,8 +374,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);
>         }
> @@ -621,7 +625,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)
> @@ -690,8 +695,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)))
> @@ -707,6 +710,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();
>
> @@ -1627,7 +1634,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);
> @@ -1695,8 +1702,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))
> @@ -1705,6 +1712,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 e270cb740b9e7..636ef7caa021d 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -326,6 +326,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.
> --
> 2.39.2
>
>
>




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux