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