linux-kernel - Re: [PATCH] usb: dwc3: of-simple: reset host controller at suspend/resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFqH_51EvuJmU2rxcndWdyYj6CAhzDv_o6YPnUHJb6ctM+k8VQ@mail.gmail.com>
Date:   Fri, 13 Jul 2018 10:14:59 +0200
From:   Enric Balletbo Serra <eballetbo@...il.com>
To:     ezequiel@...labora.com
Cc:     Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Brian Norris <briannorris@...omium.org>, kernel@...labora.com,
        Heiko Stübner <heiko@...ech.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, Felipe Balbi <balbi@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: of-simple: reset host controller at suspend/resume

Hi,
Missatge de Ezequiel Garcia <ezequiel@...labora.com> del dia dt., 10
de jul. 2018 a les 0:10:
>
> Sigh, now actually Cc devicetree folks
>
> On Mon, 2018-07-09 at 19:04 -0300, Ezequiel Garcia wrote:
> > (Cc devicetree folks)
> >
> > Hi Enric,
> >
> > Thanks for the patch.
> >
> > On Mon, 2018-07-09 at 17:08 +0200, Enric Balletbo i Serra wrote:
> > > If we power off the SoC logic rail in S3, we can find that the
> > > Type-C
> > > PHY can't initialize correctly after system resume. We need to
> > > toggle
> > > the USB3-OTG reset before trying to initialize the PHY, or else it
> > > times out.
> > >
> > >     phy phy-ff800000.phy.9: phy poweron failed --> -110
> > >     dwc3 fe900000.dwc3: failed to initialize core
> > >     dwc3: probe of fe900000.dwc3 failed with error -110
> > >
> > > Note that the RK3399 TRM suggests that we should keep the whole
> > > usb3
> > > controller in reset for the duration of the Type-C PHY
> > > initialization.
> > > However, it's hard to assert the reset in the current framework of
> > > reset. We're still skeptical about that, and we haven't yet found a
> > > case where this seems to have mattered. This approach is much
> > > easier,
> > > it
> > > simply holds the USB3-OTG reset while device is supended.
> > >
> > > The dwc3 core is going to reinitialize the controller at
> > > suspend/resume
> > > anyway (including a "soft reset"), so it should be safe to do this.
> > >
> > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com
> > > >
> > > ---
> > > Dear all,
> > >
> > > Now that the usb3-phy otg port support for rk3399 has been merged
> > > [1]
> > > we
> > > found that suspend/resume is broken. The problem is well known for
> > > ChromeOS kernels, they solved it in a similar way adding a reset
> > > pulse on
> > > resume in the specific usb glue layer (dwc3-rockchip). In mainline,
> > > though, we use the dwc3-of-simple glue layer instead of a specific
> > > layer
> > > for rockchip. The patch is based on the Brian Norris work but
> > > slightly
> > > different, it holds the reset while device is suspended. It was
> > > tested
> > > on a Samsung Chromebook Plus with usbc docking station attached by
> > > doing
> > > different suspend/resume cycles and checking no usb devices has
> > > been
> > > lost.
> > >
> > > I am not sure this is the better way to solve this but I did not
> > > find
> > > any other way, and, as I am not sure this can be generic, the reset
> > > is only
> > > done on rockchip platforms.
> > >
> >
> > I don't really understand why there are per-platform hacks in glue
> > drivers, instead of having per-platform glue drivers, or some other
> > pluggable hooks.
> >
> > > Best regards,
> > >  Enric
> > >
> > > [1] bfdca1736ea76345071bbc5607d18928e54909ac ('arm64: dts:
> > > rockchip:
> > > add
> > >     usb3-phy otg-port support for rk3399')
> > >
> > >  drivers/usb/dwc3/dwc3-of-simple.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > >
> > > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
> > > b/drivers/usb/dwc3/dwc3-of-simple.c
> > > index dbeff5e6ad14..1d1ece99ed94 100644
> > > --- a/drivers/usb/dwc3/dwc3-of-simple.c
> > > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> > > @@ -201,9 +201,30 @@ static int
> > > dwc3_of_simple_runtime_resume(struct
> > > device *dev)
> > >
> > >     return 0;
> > >  }
> > > +
> > > +static int dwc3_of_simple_suspend(struct device *dev)
> > > +{
> > > +   struct dwc3_of_simple *simple = dev_get_drvdata(dev);
> > > +
> > > +   if (of_device_is_compatible(dev->of_node,
> > > "rockchip,rk3399-
> > > dwc3"))
> > >
> >
> > Instead of calling of_device_is_compatible in each suspend/resume,
> > which seems quite expensive, how about having a per-device boolean
> > 'needs_reset' or something like that?
> >
Yep, not sure how much, but probably use a boolean will be faster. I
am also wondering if we can remove the of_device_is_compatible call
and do the reset on all platforms. This will need lots of tests on
different platforms, of course. i'd like to hear maintainers feedback
here.

> > Or maybe just have a boolean devicetree property for this?

I don't think moving the "hack" to the devicetree will be acceptable
for the DT maintainers, this is not really a hardware property.

Thanks,
  Enric.

> >
> > Regards,
> > Eze
> >