[PATCH v4 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[PATCH v4 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[Ferry Toth]

v4:
- Sent in correct version (Andy, Thinh)
- Add details on reproducing on Intel Merrifield (Thinh)

v3:
- Correct commit message (Greg)
- Add Fixes: and Cc: stable (Greg)

v2:
- Split into separate commits (Thinh)
- Only defer probe on -ETIMEDOUT (Thinh)
- Loose curly brackets (Heikki)

Ferry Toth (2):
  usb: ulpi: defer ulpi_register on ulpi_read_id timeout
  usb: dwc3: core: defer probe on ulpi_read_id timeout

 drivers/usb/common/ulpi.c | 2 +-
 drivers/usb/dwc3/core.c   | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.37.2

[PATCH v4 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout

[Ferry Toth]

Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present")
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. As a result usb probe completes without phy.

On Intel Merrifield the issue is reproducible but difficult to find the
root cause. Using an ordinary kernel and rootfs with buildroot ulpi_read_id()
succeeds. As soon as adding ftrace / bootconfig to find out why,
ulpi_read_id() fails and we can't analyze the flow. Using another rootfs
ulpi_read_id() fails even without adding ftrace. Appearantly the issue is
some kind of timing / race, but merely retrying ulpi_read_id() does not
resolve the issue.

This patch makes ulpi_read_id() return -ETIMEDOUT to its user if the first
test write fails. The user should then handle it appropriately. A follow up
patch will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out.

Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT")
Cc: stable@vger.kernel.org

Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
---
 drivers/usb/common/ulpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index d7c8461976ce..60e8174686a1 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -207,7 +207,7 @@ static int ulpi_read_id(struct ulpi *ulpi)
 	/* Test the interface */
 	ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
 	if (ret < 0)
-		goto err;
+		return ret;
 
 	ret = ulpi_read(ulpi, ULPI_SCRATCH);
 	if (ret < 0)
-- 
2.37.2

Re: [PATCH v4 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout

[Andy Shevchenko]

Sorry, Ferry, but a few style issues in the commit message.

On Sun, Nov 20, 2022 at 04:37:03PM +0100, Ferry Toth wrote:
> Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present")

It's fine to wrap this long line when it's in the commit message main body.

> 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. As a result usb probe completes without phy.
> 
> On Intel Merrifield the issue is reproducible but difficult to find the
> root cause. Using an ordinary kernel and rootfs with buildroot ulpi_read_id()
> succeeds. As soon as adding ftrace / bootconfig to find out why,
> ulpi_read_id() fails and we can't analyze the flow. Using another rootfs
> ulpi_read_id() fails even without adding ftrace. Appearantly the issue is
> some kind of timing / race, but merely retrying ulpi_read_id() does not
> resolve the issue.

> This patch makes ulpi_read_id() return -ETIMEDOUT to its user if the first

s/This patch makes/Make/  (as per Submitting Patches: use imperative form)

> test write fails. The user should then handle it appropriately. A follow up
> patch will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out.
> 
> Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT")
> Cc: stable@vger.kernel.org

> 

This line breaks the tag block, meaning that Fixes won't be recognized as a tag
by some scripts.

> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
> ---
>  drivers/usb/common/ulpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index d7c8461976ce..60e8174686a1 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -207,7 +207,7 @@ static int ulpi_read_id(struct ulpi *ulpi)
>  	/* Test the interface */
>  	ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
>  	if (ret < 0)
> -		goto err;
> +		return ret;
>  
>  	ret = ulpi_read(ulpi, ULPI_SCRATCH);
>  	if (ret < 0)
> -- 
> 2.37.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout

[Thinh Nguyen]

On Sun, Nov 20, 2022, Ferry Toth wrote:
> Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present")
> 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. As a result usb probe completes without phy.
> 
> On Intel Merrifield the issue is reproducible but difficult to find the
> root cause. Using an ordinary kernel and rootfs with buildroot ulpi_read_id()

ordinary = mainline?

> succeeds. As soon as adding ftrace / bootconfig to find out why,
> ulpi_read_id() fails and we can't analyze the flow. Using another rootfs
> ulpi_read_id() fails even without adding ftrace. Appearantly the issue is

This info fits more for the dwc3 patch, and can we use another word for
"apparently" when we still don't know the real cause? Maybe "we
suspect?"

Thanks,
Thinh

> some kind of timing / race, but merely retrying ulpi_read_id() does not
> resolve the issue.
> 
> This patch makes ulpi_read_id() return -ETIMEDOUT to its user if the first
> test write fails. The user should then handle it appropriately. A follow up
> patch will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out.
> 
> Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT")
> Cc: stable@vger.kernel.org
> 
> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
> ---
>  drivers/usb/common/ulpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index d7c8461976ce..60e8174686a1 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -207,7 +207,7 @@ static int ulpi_read_id(struct ulpi *ulpi)
>  	/* Test the interface */
>  	ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
>  	if (ret < 0)
> -		goto err;
> +		return ret;
>  
>  	ret = ulpi_read(ulpi, ULPI_SCRATCH);
>  	if (ret < 0)
> -- 
> 2.37.2
> 

[PATCH v4 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[Ferry Toth]

Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present"),
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() masking the timeout on the first
test write. In the past dwc3 probe continued by calling dwc3_core_soft_reset()
followed by dwc3_get_extcon() which happend to return -EPROBE_DEFER.
On deferred probe ulpi_read_id() finally succeeded. Due to above mentioned
rearranging -EPROBE_DEFER is not returned and probe completes without phy.

As we now changed ulpi_read_id() to return -ETIMEDOUT in this case, we
need to handle the error by calling dwc3_core_soft_reset() and request
-EPROBE_DEFER. On deferred probe ulpi_read_id() is retried and succeeds.

Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT")
Cc: stable@vger.kernel.org

Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
---
 drivers/usb/dwc3/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 648f1c570021..2779f17bffaf 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1106,8 +1106,13 @@ static int dwc3_core_init(struct dwc3 *dwc)
 
 	if (!dwc->ulpi_ready) {
 		ret = dwc3_core_ulpi_init(dwc);
-		if (ret)
+		if (ret) {
+			if (ret == -ETIMEDOUT) {
+				dwc3_core_soft_reset(dwc);
+				ret = -EPROBE_DEFER;
+			}
 			goto err0;
+		}
 		dwc->ulpi_ready = true;
 	}
 
-- 
2.37.2

Re: [PATCH v4 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[Andy Shevchenko]

On Sun, Nov 20, 2022 at 04:37:04PM +0100, Ferry Toth wrote:
> Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present"),
> 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() masking the timeout on the first
> test write. In the past dwc3 probe continued by calling dwc3_core_soft_reset()
> followed by dwc3_get_extcon() which happend to return -EPROBE_DEFER.
> On deferred probe ulpi_read_id() finally succeeded. Due to above mentioned
> rearranging -EPROBE_DEFER is not returned and probe completes without phy.
> 
> As we now changed ulpi_read_id() to return -ETIMEDOUT in this case, we
> need to handle the error by calling dwc3_core_soft_reset() and request
> -EPROBE_DEFER. On deferred probe ulpi_read_id() is retried and succeeds.
> 
> Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT")
> Cc: stable@vger.kernel.org
> 
> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>

Same comments as per patch 1.

But before sending v5, let give a chance to others to review the code and other
aspects of the changes.

-- 
With Best Regards,
Andy Shevchenko