| 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
| ||
|
Message-ID: <20221111013048.p6ahttrpbgzpavid@synopsys.com>
Date: Fri, 11 Nov 2022 01:31:02 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Ferry Toth <fntoth@...il.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Sean Anderson <sean.anderson@...o.com>,
Liu Shixin <liushixin2@...wei.com>,
Andrey Smirnov <andrew.smirnov@...il.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id
timeout
On Thu, Nov 10, 2022, Ferry Toth wrote:
> Hi
>
> Op 10-11-2022 om 13:45 schreef Ferry Toth:
> > (sorry sent html with previous attempt)
> >
> > On 10-11-2022 01:06, Thinh Nguyen wrote:
> > > Hi Ferry,
> > >
> > > On Wed, Nov 09, 2022, Ferry Toth wrote:
> > > > Since commit 0f010171
> > > > Dual Role support on Intel Merrifield platform broke due to rearranging
> > > > the call to dwc3_get_extcon().
> > > >
> > > > It appears to be caused by ulpi_read_id() on the first test
> > > > write failing
> > > > with -ETIMEDOUT. Currently ulpi_read_id() expects to discover
> > > > the phy via
> > > > DT when the test write fails and returns 0 in that case even if
> > > > DT does not
> > > > provide the phy. Due to the timeout being masked dwc3 probe continues by
> > > > calling dwc3_core_soft_reset() followed by dwc3_get_extcon()
> > > > which happens
> > > > to return -EPROBE_DEFER. On deferred probe ulpi_read_id()
> > > > finally succeeds.
> > > >
> > > > This patch changes ulpi_read_id() to return -ETIMEDOUT when it
> > > > occurs and
> > > > catches the error in dwc3_core_init(). It handles the error by calling
> > > > dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On
> > > > deferred
> > > > probe ulpi_read_id() again succeeds.
> > > >
> > > > Signed-off-by: Ferry Toth<ftoth@...londelft.nl>
> > > > ---
> > > > drivers/usb/common/ulpi.c | 5 +++--
> > > > drivers/usb/dwc3/core.c | 5 ++++-
> > > > 2 files changed, 7 insertions(+), 3 deletions(-)
> > > >
> > > Can you split the dwc3 change and ulpi change to separate patches?
> >
> > Thanks for your comments.
> >
> > I will send v2
> >
> > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > > index d7c8461976ce..d8f22bc2f9d0 100644
> > > > --- a/drivers/usb/common/ulpi.c
> > > > +++ b/drivers/usb/common/ulpi.c
> > > > @@ -206,8 +206,9 @@ static int ulpi_read_id(struct ulpi *ulpi)
> > > > /* Test the interface */
> > > > ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
> > > > - if (ret < 0)
> > > > - goto err;
> > > > + if (ret < 0) {
> > > > + return ret;
> > > > + }
> > > > ret = ulpi_read(ulpi, ULPI_SCRATCH);
> > > > if (ret < 0)
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 648f1c570021..e293ef70039b 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -1106,8 +1106,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
> > > > if (!dwc->ulpi_ready) {
> > > > ret = dwc3_core_ulpi_init(dwc);
> > > > - if (ret)
> > > > + if (ret) {
> > > > + dwc3_core_soft_reset(dwc);
> > > We shouldn't need to do soft reset here. The controller shouldn't be at
> > > a bad/incorrect state at this point to warrant a soft-reset. There will
> > > be a soft-reset when it goes through the initialization again.
> >
> > It doesn't go through the initialization again unless we set
> > -EPROBE_DEFER. And when we make ulpi_read_id() return -EPROBE_DEFER it
> > will goto err0 here, so skips dwc3_core_soft_reset.
> >
> > Do you mean you prefer something like:
> >
> > if (ret) {
> >
> > if (ret == -ETIMEDOUT) ret = -EPROBE_DEFER;
> >
> > else goto err0;
Why "else"? But I saw you remove that in the new patch.
> >
> > }
>
> I just tested, and calling dwc3_core_soft_reset() proves to be necessary as
> we need to goto err0 directly after. Else ret is overwritten and
> -EPROBE_DEFER lost.
Looks like there's a strange dependency problem here.
* The setup needs a soft-reset before ulpi registration
* The ulpi registration needs to go before the phy initialization
* The soft-reset should be called after the phy initialization
I can't explain the actual issue here, and we can't debug further
because to look into it further would require looking at internal
signals.
This soft-reset and -EPROBE_DEFER seems to be a workaround to this
dependency problem. Instead of using -EPROBE_DEFER, can you do this:
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 2f0a9679686f..5a1aaf3741ec 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1097,6 +1097,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
goto err0;
if (!dwc->ulpi_ready) {
+ /* Add comment */
+ dwc3_core_soft_reset(dwc);
ret = dwc3_core_ulpi_init(dwc);
if (ret)
goto err0;
>
> > > > + ret = -EPROBE_DEFER;
> > > We shouldn't automatically set every error status to correspond to
> > > -EPROBE_DEFER. Check only the approapriate error codes (-ETIMEDOUT +
> > > any other?).
> > Other could be -ENOMEM. I think no need to do any new handling for that.
There's also -EINVAL and some others too. Regardless, if it's -ENOMEM,
then it should not return -EPROBE_DEFER.
> > > > goto err0;
> > > > + }
> > > > dwc->ulpi_ready = true;
> > > > }
> > > >
Thanks,
Thinh
Powered by blists - more mailing lists