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