Re: [PATCH v1] driver core: Update device link status correctly for SYNC_STATE_ONLY links

[Rafael J. Wysocki]

On Tue, May 26, 2020 at 9:05 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> When SYNC_STATE_ONLY support was added in commit 05ef983e0d65 ("driver
> core: Add device link support for SYNC_STATE_ONLY flag"),
> SYNC_STATE_ONLY links were treated similar to STATELESS links in terms
> of not blocking consumer probe if the supplier hasn't probed yet.
>
> That caused a SYNC_STATE_ONLY device link's status to not get updated.
> Since SYNC_STATE_ONLY device link is no longer useful once the
> consumer probes, commit 21c27f06587d ("driver core: Fix
> SYNC_STATE_ONLY device link implementation") addresses the status
> update issue by deleting the SYNC_STATE_ONLY device link instead of
> complicating the status update code.
>
> However, there are still some cases where we need to update the status
> of a SYNC_STATE_ONLY device link.  A SYNC_STATE_ONLY device link can
> later get converted into a normal MANAGED device link when a normal
> MANAGED device link is created between a supplier and consumer that
> already have a SYNC_STATE_ONLY device link between them. If a
> SYNC_STATE_ONLY device link's status isn't maintained correctly till
> it's converted to a normal MANAGED device link, then the normal
> MANAGED device link will end up with a wrong link status. This can
> cause a warning stack trace[1] when the consumer device probes.
>
> This commit fixes the SYNC_STATE_ONLY device link status update issue
> where it wouldn't transition correctly from DL_STATE_AVAILABLE to
> DL_STATE_CONSUMER_PROBE.
>
> [1] - https://lore.kernel.org/lkml/20200522204120.3b3c9ed6@apollo/
> Fixes: 05ef983e0d65 ("driver core: Add device link support for SYNC_STATE_ONLY flag")
> Fixes: 21c27f06587d ("driver core: Fix SYNC_STATE_ONLY device link implementation")
> Reported-by: Michael Walle <michael@xxxxxxxx>
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>

Tentatively

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

->

> ---
> Greg,
>
> I think this is the issue Michael ran into. I'd like him to test the fix
> before it's pulled in.
>
> Michael,
>
> If you can test this on the branch you saw the issue in and give a
> Tested-by if it works, that'd be great.

-> provided that the patch works.

Thanks!

>  drivers/base/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 791b7530599f..9511be3f9a32 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -687,11 +687,11 @@ int device_links_check_suppliers(struct device *dev)
>         device_links_write_lock();
>
>         list_for_each_entry(link, &dev->links.suppliers, c_node) {
> -               if (!(link->flags & DL_FLAG_MANAGED) ||
> -                   link->flags & DL_FLAG_SYNC_STATE_ONLY)
> +               if (!(link->flags & DL_FLAG_MANAGED))
>                         continue;
>
> -               if (link->status != DL_STATE_AVAILABLE) {
> +               if (link->status != DL_STATE_AVAILABLE &&
> +                   !(link->flags & DL_FLAG_SYNC_STATE_ONLY)) {
>                         device_links_missing_supplier(dev);
>                         ret = -EPROBE_DEFER;
>                         break;
> --
> 2.27.0.rc0.183.gde8f92d652-goog
>