v3: - Correct commit message (Greg) - Add fixes (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(-)
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. As a result usb probe completes without phy.
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)
On Thu, Nov 17, 2022 at 09:54:10PM +0100, Ferry Toth wrote:
Since commit 0f010171 Dual Role support on Intel Merrifield platform broke due to rearranging the call to dwc3_get_extcon().
Not sure why format is broken, you may add into your ~/.gitconfig
[core] abbrev = 12
[alias] one = show -s --pretty='format:%h ("%s")'
and run
git one 0f010171
with the result
0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present")
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.
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() 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.
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.
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; }
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.' Subject: [PATCH v3 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Link: https://lore.kernel.org/stable/20221117205411.11489-3-ftoth%40exalondelft.nl
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
On Thu, Nov 17, 2022, Ferry Toth wrote:
Since commit 0f010171
I don't your update as noted in the v3 change (ie. Greg's suggestions).
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.
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.
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);
I'm not sure if you saw my previous response. I wanted to check to see if we need to do soft-reset once before ulpi init as part of its initialization.
I'm just curious why Andrey Smirnov test doesn't require this change. If it's a quirk for this specific configuration, then the change here is fine. If it's required for all ULPI setup, then we can unconditionally do that for all ULPI setup during initialization.
ret = -EPROBE_DEFER;} goto err0;}2.37.2
Thanks, Thinh
On 18-11-2022 03:11, Thinh Nguyen wrote:
On Thu, Nov 17, 2022, Ferry Toth wrote:
Since commit 0f010171
I don't your update as noted in the v3 change (ie. Greg's suggestions).
Indeed. I seem to have sent in the wrong sha. Sorry about this.
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.
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.
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);I'm not sure if you saw my previous response. I wanted to check to see if we need to do soft-reset once before ulpi init as part of its initialization.
I missed it but found it now. I will try your suggestion and answer.
I'm just curious why Andrey Smirnov test doesn't require this change. If it's a quirk for this specific configuration, then the change here is fine. If it's required for all ULPI setup, then we can unconditionally do that for all ULPI setup during initialization.
Yes me too. I can reproduce that when I build kernel and rootfs with buildroot there is no issue. But as soon as I add ftrace / bootconfig the issue appears and then I can't analyze the flow. It's seems some kind of timing / race.
However, I have tried deferring without dwc3_core_soft_reset() (to verify if it is just a matter of time) and that doesn't work. dwc3 is deferred about 10x and then gives up without phy. So, it's not just the time and not just a matter of retrying the test write.
ret = -EPROBE_DEFER;} goto err0;}2.37.2
Thanks, Thinh
On Fri, Nov 18, 2022, Ferry Toth wrote:
On 18-11-2022 03:11, Thinh Nguyen wrote:
On Thu, Nov 17, 2022, Ferry Toth wrote:
Since commit 0f010171
I don't your update as noted in the v3 change (ie. Greg's suggestions).
Indeed. I seem to have sent in the wrong sha. Sorry about this.
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.
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.
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);I'm not sure if you saw my previous response. I wanted to check to see if we need to do soft-reset once before ulpi init as part of its initialization.
I missed it but found it now. I will try your suggestion and answer.
I think it may not be needed now from your experiments noted below.
I'm just curious why Andrey Smirnov test doesn't require this change. If it's a quirk for this specific configuration, then the change here is fine. If it's required for all ULPI setup, then we can unconditionally do that for all ULPI setup during initialization.
Yes me too. I can reproduce that when I build kernel and rootfs with buildroot there is no issue. But as soon as I add ftrace / bootconfig the issue appears and then I can't analyze the flow. It's seems some kind of timing / race.
I think this is very useful info that should go into the commit message.
However, I have tried deferring without dwc3_core_soft_reset() (to verify if it is just a matter of time) and that doesn't work. dwc3 is deferred about 10x and then gives up without phy. So, it's not just the time and not just a matter of retrying the test write.
Even though we can't root cause the problem, this fix should be fine.
ret = -EPROBE_DEFER;} goto err0;}-- 2.37.2
Thanks, Thinh
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
Ferry Toth -
Ferry Toth -
kernel test robot -
Thinh Nguyen