Re: [PATCH] phy: core: Add consumer device link support

[Saravana Kannan <saravanak@google.com>]

On Mon, Feb 10, 2020 at 4:14 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi,
>
> On 10/02/20 5:00 PM, Alexandre Torgue wrote:
> > Hi Kishon,
> >
> > On 2/10/20 9:08 AM, Kishon Vijay Abraham I wrote:
> >> Hi Alexandre,
> >>
> >> On 07/02/20 12:27 PM, youling 257 wrote:
> >>> test this diff, dwc3 work for my device, thanks.
> >>>
> >>> 2020-02-07 13:16 GMT+08:00, Kishon Vijay Abraham I <kishon@ti.com>:
> >>>> Hi,
> >>>>
> >>>> On 06/02/20 7:09 PM, youling257 wrote:
> >>>>> This patch cause "dwc3 dwc3.3.auto: failed to create device link to
> >>>>> dwc3.3.auto.ulpi" problem.
> >>>>> https://bugzilla.kernel.org/show_bug.cgi?id=206435
> >>>>
> >>>> I'm suspecting there is some sort of reverse dependency with dwc3 ULPI.
> >>>> Can you try the following diff?
> >>>>
> >>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> >>>> index 2eb28cc2d2dc..397311dcb116 100644
> >>>> --- a/drivers/phy/phy-core.c
> >>>> +++ b/drivers/phy/phy-core.c
> >>>> @@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const char
> >>>> *string)
> >>>>
> >>>>          get_device(&phy->dev);
> >>>>
> >>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> >>>> +       link = device_link_add(dev, &phy->dev,
> >>>> DL_FLAG_SYNC_STATE_ONLY);
> >>>>          if (!link) {
> >>>>                  dev_err(dev, "failed to create device link to %s\n",
> >>>>                          dev_name(phy->dev.parent));
> >>>> @@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev,
> >>>> struct device_node *np,
> >>>>                  return phy;
> >>>>          }
> >>>>
> >>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> >>>> +       link = device_link_add(dev, &phy->dev,
> >>>> DL_FLAG_SYNC_STATE_ONLY);

Definitely don't use SYNC_STATE_ONLY.

To give some context, drivers themselves are only expected to use
STATELESS links. Only the driver core is supposed to use MANAGED (if
you don't use STATELESS, it's MANAGED by default) links. And
SYNC_STATE_ONLY makes sense only for MANAGED links.

Also, as the SYNC_STATE_ONLY documentation says, it only affect
sync_state() behavior and doesn't affect suspend/resume in any way.

> >>>>          if (!link) {
> >>>>                  dev_err(dev, "failed to create device link to %s\n",
> >>>>                          dev_name(phy->dev.parent));
> >>>> @@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct device
> >>>> *dev, struct device_node *np,
> >>>>          *ptr = phy;
> >>>>          devres_add(dev, ptr);
> >>>>
> >>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> >>>> +       link = device_link_add(dev, &phy->dev,
> >>>> DL_FLAG_SYNC_STATE_ONLY);
> >>>>          if (!link) {
> >>>>                  dev_err(dev, "failed to create device link to %s\n",
> >>>>                          dev_name(phy->dev.parent));Parent
> >>
> >> Can you check if this doesn't affect the suspend/resume ordering?
> >
> > With this fix, suspend/resume ordering is broken on my side. What do you
> > think to keep the STATELESS flag and to only display a warn if
> > "device_link_add" returns an error ? It's not "smart" but it could
> > solved our issue.
>
> yeah, that sounds reasonable for now. Can you find out the dependencies
> between dwc3 and ulpi and what exactly breaks. That would help Saravana
> to suggest a fix?

Yup, I don't have enough context of the dependencies here to suggest a
good fix. But if device_link_add() is failing with STATELESS and not
failing with SYNC_STATE_ONLY, I'm pretty sure you have a cyclic
dependency between these devices when you create this link. Keep in
mind that it can be a cycle involving more than 2 devices -- A -> B ->
C -> A. And cycles don't make sense when you are trying to order
suspend/resume. Looks like the new link is wrong or an existing link
is wrong. If the dependencies are actually correct in hardware, then
maybe your hardware device needs to be split into multiple devices so
that you don't have cycles and can suspend in a meaningful way?

Hope that helps.

Thanks,
Saravana