[PATCH v1 0/2] Revert commit 6ccb83d6c497 to fix DWC3 dual-role regression

[PATCH v1 0/2] Revert commit 6ccb83d6c497 to fix DWC3 dual-role regression

[Roy Luo]

Commit 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state()
helper") was introduced to workaround watchdog timeout issues on some
platforms, allowing xhci_reset() to bail out early without waiting
for the reset to complete.

This behavior causes a regression on SNPS DWC3 USB controller with
dual-role capability. When the DWC3 controller exits host mode and
removes xhci while a reset is still in progress, and then tries to
configure its hardware for device mode, the ongoing reset leads to
register access issues; specifically, all register reads returns 0.
These issues extend beyond the xhci register space (which is expected
during a reset) and affect the entire DWC3 IP block, causing the DWC3
device mode to malfunction.

To fix this regression without reintroducing the watchdog timeout issue,
the first patchset "usb: xhci: Skip xhci_reset in xhci_resume if
xhci is being removed" skips xhci_reset() in xhci_resume() reinit
path when xhci is being removed, which should address the watchdog
timeout issue. Then we can safely revert commit 6ccb83d6c497 ("usb:
xhci: Implement xhci_handshake_check_state() helper").

---
Changes in v1:
- Link to previous discussion: https://lore.kernel.org/r/20250517043942.372315-1-royluo@google.com/
---

Roy Luo (2):
  usb: xhci: Skip xhci_reset in xhci_resume if xhci is being removed
  Revert "usb: xhci: Implement xhci_handshake_check_state() helper"

 drivers/usb/host/xhci-ring.c |  5 ++---
 drivers/usb/host/xhci.c      | 31 +++++--------------------------
 drivers/usb/host/xhci.h      |  2 --
 3 files changed, 7 insertions(+), 31 deletions(-)


base-commit: 4a95bc121ccdaee04c4d72f84dbfa6b880a514b6
-- 
2.49.0.1204.g71687c7c1d-goog

[PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci_resume if xhci is being removed

[Roy Luo]

xhci_reset() currently returns -ENODEV if XHCI_STATE_REMOVING is
set, without completing the xhci handshake, unless the reset completes
exceptionally quickly. This behavior causes a regression on Synopsys
DWC3 USB controllers with dual-role capabilities.

Specifically, when a DWC3 controller exits host mode and removes xhci
while a reset is still in progress, and then attempts to configure its
hardware for device mode, the ongoing, incomplete reset leads to
critical register access issues. All register reads return zero, not
just within the xHCI register space (which might be expected during a
reset), but across the entire DWC3 IP block.

This patch addresses the issue by preventing xhci_reset() from being
called in xhci_resume() and bailing out early in the reinit flow when
XHCI_STATE_REMOVING is set.

Cc: stable@vger.kernel.org
Fixes: 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state() helper")
Suggested-by: Mathias Nyman <mathias.nyman@intel.com>
Signed-off-by: Roy Luo <royluo@google.com>
---
 drivers/usb/host/xhci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 90eb491267b5..244b12eafd95 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1084,7 +1084,10 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
 		xhci_dbg(xhci, "Stop HCD\n");
 		xhci_halt(xhci);
 		xhci_zero_64b_regs(xhci);
-		retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
+		if (xhci->xhc_state & XHCI_STATE_REMOVING)
+			retval = -ENODEV;
+		else
+			retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
 		spin_unlock_irq(&xhci->lock);
 		if (retval)
 			return retval;
-- 
2.49.0.1204.g71687c7c1d-goog

[PATCH v1 2/2] Revert "usb: xhci: Implement xhci_handshake_check_state() helper"

[Roy Luo]

This reverts commit 6ccb83d6c4972ebe6ae49de5eba051de3638362c.

Commit 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state()
helper") was introduced to workaround watchdog timeout issues on some
platforms, allowing xhci_reset() to bail out early without waiting
for the reset to complete.

Skipping the xhci handshake during a reset is a dangerous move. The
xhci specification explicitly states that certain registers cannot
be accessed during reset in section 5.4.1 USB Command Register (USBCMD),
Host Controller Reset (HCRST) field:
"This bit is cleared to '0' by the Host Controller when the reset
process is complete. Software cannot terminate the reset process
early by writinga '0' to this bit and shall not write any xHC
Operational or Runtime registers until while HCRST is '1'."

This behavior causes a regression on SNPS DWC3 USB controller with
dual-role capability. When the DWC3 controller exits host mode and
removes xhci while a reset is still in progress, and then tries to
configure its hardware for device mode, the ongoing reset leads to
register access issues; specifically, all register reads returns 0.
These issues extend beyond the xhci register space (which is expected
during a reset) and affect the entire DWC3 IP block, causing the DWC3
device mode to malfunction.

Cc: stable@vger.kernel.org
Fixes: 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state() helper")
Signed-off-by: Roy Luo <royluo@google.com>
---
 drivers/usb/host/xhci-ring.c |  5 ++---
 drivers/usb/host/xhci.c      | 26 +-------------------------
 drivers/usb/host/xhci.h      |  2 --
 3 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 423bf3649570..b720e04ce7d8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -518,9 +518,8 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
 	 * In the future we should distinguish between -ENODEV and -ETIMEDOUT
 	 * and try to recover a -ETIMEDOUT with a host controller reset.
 	 */
-	ret = xhci_handshake_check_state(xhci, &xhci->op_regs->cmd_ring,
-			CMD_RING_RUNNING, 0, 5 * 1000 * 1000,
-			XHCI_STATE_REMOVING);
+	ret = xhci_handshake(&xhci->op_regs->cmd_ring,
+			CMD_RING_RUNNING, 0, 5 * 1000 * 1000);
 	if (ret < 0) {
 		xhci_err(xhci, "Abort failed to stop command ring: %d\n", ret);
 		xhci_halt(xhci);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 244b12eafd95..cb9f35acb1f9 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -83,29 +83,6 @@ int xhci_handshake(void __iomem *ptr, u32 mask, u32 done, u64 timeout_us)
 	return ret;
 }
 
-/*
- * xhci_handshake_check_state - same as xhci_handshake but takes an additional
- * exit_state parameter, and bails out with an error immediately when xhc_state
- * has exit_state flag set.
- */
-int xhci_handshake_check_state(struct xhci_hcd *xhci, void __iomem *ptr,
-		u32 mask, u32 done, int usec, unsigned int exit_state)
-{
-	u32	result;
-	int	ret;
-
-	ret = readl_poll_timeout_atomic(ptr, result,
-				(result & mask) == done ||
-				result == U32_MAX ||
-				xhci->xhc_state & exit_state,
-				1, usec);
-
-	if (result == U32_MAX || xhci->xhc_state & exit_state)
-		return -ENODEV;
-
-	return ret;
-}
-
 /*
  * Disable interrupts and begin the xHCI halting process.
  */
@@ -226,8 +203,7 @@ int xhci_reset(struct xhci_hcd *xhci, u64 timeout_us)
 	if (xhci->quirks & XHCI_INTEL_HOST)
 		udelay(1000);
 
-	ret = xhci_handshake_check_state(xhci, &xhci->op_regs->command,
-				CMD_RESET, 0, timeout_us, XHCI_STATE_REMOVING);
+	ret = xhci_handshake(&xhci->op_regs->command, CMD_RESET, 0, timeout_us);
 	if (ret)
 		return ret;
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 242ab9fbc8ae..5e698561b96d 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1855,8 +1855,6 @@ void xhci_remove_secondary_interrupter(struct usb_hcd
 /* xHCI host controller glue */
 typedef void (*xhci_get_quirks_t)(struct device *, struct xhci_hcd *);
 int xhci_handshake(void __iomem *ptr, u32 mask, u32 done, u64 timeout_us);
-int xhci_handshake_check_state(struct xhci_hcd *xhci, void __iomem *ptr,
-		u32 mask, u32 done, int usec, unsigned int exit_state);
 void xhci_quiesce(struct xhci_hcd *xhci);
 int xhci_halt(struct xhci_hcd *xhci);
 int xhci_start(struct xhci_hcd *xhci);
-- 
2.49.0.1204.g71687c7c1d-goog

Re: [PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci_resume if xhci is being removed

[Thinh Nguyen]

Hi Mathias, Roy,

On Thu, May 22, 2025, Roy Luo wrote:
> xhci_reset() currently returns -ENODEV if XHCI_STATE_REMOVING is
> set, without completing the xhci handshake, unless the reset completes
> exceptionally quickly. This behavior causes a regression on Synopsys
> DWC3 USB controllers with dual-role capabilities.
> 
> Specifically, when a DWC3 controller exits host mode and removes xhci
> while a reset is still in progress, and then attempts to configure its
> hardware for device mode, the ongoing, incomplete reset leads to
> critical register access issues. All register reads return zero, not
> just within the xHCI register space (which might be expected during a
> reset), but across the entire DWC3 IP block.
> 
> This patch addresses the issue by preventing xhci_reset() from being
> called in xhci_resume() and bailing out early in the reinit flow when
> XHCI_STATE_REMOVING is set.
> 
> Cc: stable@vger.kernel.org
> Fixes: 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state() helper")
> Suggested-by: Mathias Nyman <mathias.nyman@intel.com>
> Signed-off-by: Roy Luo <royluo@google.com>
> ---
>  drivers/usb/host/xhci.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 90eb491267b5..244b12eafd95 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1084,7 +1084,10 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  		xhci_dbg(xhci, "Stop HCD\n");
>  		xhci_halt(xhci);
>  		xhci_zero_64b_regs(xhci);
> -		retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
> +		if (xhci->xhc_state & XHCI_STATE_REMOVING)
> +			retval = -ENODEV;
> +		else
> +			retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);

How can this prevent the xhc_state from changing while in reset? There's
no locking in xhci-plat.

I would suggest to simply revert the commit 6ccb83d6c497 that causes
regression first. We can investigate and look into a solution to the
specific Qcom issue afterward.

Note that this commit may impact role-switching flow for all DRD dwc3
(and perhaps others), which may also impact other Qcom DRD platforms.

Thanks,
Thinh

>  		spin_unlock_irq(&xhci->lock);
>  		if (retval)
>  			return retval;
> -- 
> 2.49.0.1204.g71687c7c1d-goog
> 

Re: [PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci_resume if xhci is being removed

[Mathias Nyman]

On 24.5.2025 2.06, Thinh Nguyen wrote:
> Hi Mathias, Roy,
> 
> On Thu, May 22, 2025, Roy Luo wrote:
>> xhci_reset() currently returns -ENODEV if XHCI_STATE_REMOVING is
>> set, without completing the xhci handshake, unless the reset completes
>> exceptionally quickly. This behavior causes a regression on Synopsys
>> DWC3 USB controllers with dual-role capabilities.
>>
>> Specifically, when a DWC3 controller exits host mode and removes xhci
>> while a reset is still in progress, and then attempts to configure its
>> hardware for device mode, the ongoing, incomplete reset leads to
>> critical register access issues. All register reads return zero, not
>> just within the xHCI register space (which might be expected during a
>> reset), but across the entire DWC3 IP block.
>>
>> This patch addresses the issue by preventing xhci_reset() from being
>> called in xhci_resume() and bailing out early in the reinit flow when
>> XHCI_STATE_REMOVING is set.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state() helper")
>> Suggested-by: Mathias Nyman <mathias.nyman@intel.com>
>> Signed-off-by: Roy Luo <royluo@google.com>
>> ---
>>   drivers/usb/host/xhci.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 90eb491267b5..244b12eafd95 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -1084,7 +1084,10 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>>   		xhci_dbg(xhci, "Stop HCD\n");
>>   		xhci_halt(xhci);
>>   		xhci_zero_64b_regs(xhci);
>> -		retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
>> +		if (xhci->xhc_state & XHCI_STATE_REMOVING)
>> +			retval = -ENODEV;
>> +		else
>> +			retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
> 
> How can this prevent the xhc_state from changing while in reset? There's
> no locking in xhci-plat.

Patch 2/2, which is the revert of 6ccb83d6c497 prevents xhci_reset() from
aborting due to xhc_state flags change.

This patch makes sure xHC is not reset twice if xhci is resuming due to
remove being called. (XHCI_STATE_REMOVING is set).
The Qcom platform has watchdog issues with the 10 second XHCI_RESET_LONG_USEC
timeout reset during resume at remove.

> 
> I would suggest to simply revert the commit 6ccb83d6c497 that causes
> regression first. We can investigate and look into a solution to the
> specific Qcom issue afterward.

Why intentionally bring back the Qcom watchdog issue by only reverting
6ccb83d6c497 ?. Can't we solve both in one go?

> 
> Note that this commit may impact role-switching flow for all DRD dwc3
> (and perhaps others), which may also impact other Qcom DRD platforms.

Could you expand on this, I'm not sure I follow.

xHC will be reset later in remove path:

xhci_plat_remove()
   usb_remove_hcd()
     hcd->driver->stop(hcd) -> xhci_stop()
       xhci_reset(xhci, XHCI_RESET_SHORT_USEC);

Thanks
Mathias

Re: [PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci_resume if xhci is being removed

[Thinh Nguyen]

On Mon, May 26, 2025, Mathias Nyman wrote:
> On 24.5.2025 2.06, Thinh Nguyen wrote:
> > Hi Mathias, Roy,
> > 
> > On Thu, May 22, 2025, Roy Luo wrote:
> > > xhci_reset() currently returns -ENODEV if XHCI_STATE_REMOVING is
> > > set, without completing the xhci handshake, unless the reset completes
> > > exceptionally quickly. This behavior causes a regression on Synopsys
> > > DWC3 USB controllers with dual-role capabilities.
> > > 
> > > Specifically, when a DWC3 controller exits host mode and removes xhci
> > > while a reset is still in progress, and then attempts to configure its
> > > hardware for device mode, the ongoing, incomplete reset leads to
> > > critical register access issues. All register reads return zero, not
> > > just within the xHCI register space (which might be expected during a
> > > reset), but across the entire DWC3 IP block.
> > > 
> > > This patch addresses the issue by preventing xhci_reset() from being
> > > called in xhci_resume() and bailing out early in the reinit flow when
> > > XHCI_STATE_REMOVING is set.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state() helper")
> > > Suggested-by: Mathias Nyman <mathias.nyman@intel.com>
> > > Signed-off-by: Roy Luo <royluo@google.com>
> > > ---
> > >   drivers/usb/host/xhci.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > index 90eb491267b5..244b12eafd95 100644
> > > --- a/drivers/usb/host/xhci.c
> > > +++ b/drivers/usb/host/xhci.c
> > > @@ -1084,7 +1084,10 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
> > >   		xhci_dbg(xhci, "Stop HCD\n");
> > >   		xhci_halt(xhci);
> > >   		xhci_zero_64b_regs(xhci);
> > > -		retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
> > > +		if (xhci->xhc_state & XHCI_STATE_REMOVING)
> > > +			retval = -ENODEV;
> > > +		else
> > > +			retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
> > 
> > How can this prevent the xhc_state from changing while in reset? There's
> > no locking in xhci-plat.
> 
> Patch 2/2, which is the revert of 6ccb83d6c497 prevents xhci_reset() from
> aborting due to xhc_state flags change.
> 
> This patch makes sure xHC is not reset twice if xhci is resuming due to
> remove being called. (XHCI_STATE_REMOVING is set).

Wouldn't it still be possible for xhci to be removed in the middle of
reset on resume? The watchdog may still timeout afterward if there's an
issue with reset right?

> The Qcom platform has watchdog issues with the 10 second XHCI_RESET_LONG_USEC
> timeout reset during resume at remove.

> 
> > 
> > I would suggest to simply revert the commit 6ccb83d6c497 that causes
> > regression first. We can investigate and look into a solution to the
> > specific Qcom issue afterward.
> 
> Why intentionally bring back the Qcom watchdog issue by only reverting
> 6ccb83d6c497 ?. Can't we solve both in one go?

I feel that the fix is doesn't cover all the scenarios, that's why I
suggest the revert for now and wait until the fix is properly tested
before applying it to stable? 

> 
> > 
> > Note that this commit may impact role-switching flow for all DRD dwc3
> > (and perhaps others), which may also impact other Qcom DRD platforms.
> 
> Could you expand on this, I'm not sure I follow.
> 
> xHC will be reset later in remove path:
> 
> xhci_plat_remove()
>   usb_remove_hcd()
>     hcd->driver->stop(hcd) -> xhci_stop()
>       xhci_reset(xhci, XHCI_RESET_SHORT_USEC);
> 

I'm referring to the offending commit 6ccb83d6c497, that it should be
prioritized and pushed out first.

BR,
Thinh

Re: [PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci_resume if xhci is being removed

[Mathias Nyman]

On 29.5.2025 4.17, Thinh Nguyen wrote:
> On Mon, May 26, 2025, Mathias Nyman wrote:
>> On 24.5.2025 2.06, Thinh Nguyen wrote:
>>> Hi Mathias, Roy,
>>>
>>> On Thu, May 22, 2025, Roy Luo wrote:
>>>> xhci_reset() currently returns -ENODEV if XHCI_STATE_REMOVING is
>>>> set, without completing the xhci handshake, unless the reset completes
>>>> exceptionally quickly. This behavior causes a regression on Synopsys
>>>> DWC3 USB controllers with dual-role capabilities.
>>>>
>>>> Specifically, when a DWC3 controller exits host mode and removes xhci
>>>> while a reset is still in progress, and then attempts to configure its
>>>> hardware for device mode, the ongoing, incomplete reset leads to
>>>> critical register access issues. All register reads return zero, not
>>>> just within the xHCI register space (which might be expected during a
>>>> reset), but across the entire DWC3 IP block.
>>>>
>>>> This patch addresses the issue by preventing xhci_reset() from being
>>>> called in xhci_resume() and bailing out early in the reinit flow when
>>>> XHCI_STATE_REMOVING is set.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state() helper")
>>>> Suggested-by: Mathias Nyman <mathias.nyman@intel.com>
>>>> Signed-off-by: Roy Luo <royluo@google.com>
>>>> ---
>>>>    drivers/usb/host/xhci.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>>> index 90eb491267b5..244b12eafd95 100644
>>>> --- a/drivers/usb/host/xhci.c
>>>> +++ b/drivers/usb/host/xhci.c
>>>> @@ -1084,7 +1084,10 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>>>>    		xhci_dbg(xhci, "Stop HCD\n");
>>>>    		xhci_halt(xhci);
>>>>    		xhci_zero_64b_regs(xhci);
>>>> -		retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
>>>> +		if (xhci->xhc_state & XHCI_STATE_REMOVING)
>>>> +			retval = -ENODEV;
>>>> +		else
>>>> +			retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
>>>
>>> How can this prevent the xhc_state from changing while in reset? There's
>>> no locking in xhci-plat.
>>
>> Patch 2/2, which is the revert of 6ccb83d6c497 prevents xhci_reset() from
>> aborting due to xhc_state flags change.
>>
>> This patch makes sure xHC is not reset twice if xhci is resuming due to
>> remove being called. (XHCI_STATE_REMOVING is set).
> 
> Wouldn't it still be possible for xhci to be removed in the middle of
> reset on resume? The watchdog may still timeout afterward if there's an
> issue with reset right?
> 

Probably yes, but that problem is the same if we only revert 6ccb83d6c497.

>> Why intentionally bring back the Qcom watchdog issue by only reverting
>> 6ccb83d6c497 ?. Can't we solve both in one go?
> 
> I feel that the fix is doesn't cover all the scenarios, that's why I
> suggest the revert for now and wait until the fix is properly tested
> before applying it to stable?

Ok, we have different views on this.

I think we should avoid causing as much known regression as possible even
if the patch  might not cover all scenarios.

By reverting 6ccb83d6c497 we fix a SNPS DWC3 regression, but at the same
time bring back the Qcom issue, so cause another regression.

We can avoid the main part or the Qcom regression by adding this patch as
issue is with (long) xhci reset during resume if xhci is being removed, and
driver always resumes xhci during ->remove callback.

If we discover the patch is not perfect then we fix it

Thanks
Mathias

Re: [PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci_resume if xhci is being removed

[Thinh Nguyen]

On Wed, Jun 04, 2025, Mathias Nyman wrote:
> On 29.5.2025 4.17, Thinh Nguyen wrote:
> > On Mon, May 26, 2025, Mathias Nyman wrote:
> > > On 24.5.2025 2.06, Thinh Nguyen wrote:
> > > > Hi Mathias, Roy,
> > > > 
> > > > On Thu, May 22, 2025, Roy Luo wrote:
> > > > > xhci_reset() currently returns -ENODEV if XHCI_STATE_REMOVING is
> > > > > set, without completing the xhci handshake, unless the reset completes
> > > > > exceptionally quickly. This behavior causes a regression on Synopsys
> > > > > DWC3 USB controllers with dual-role capabilities.
> > > > > 
> > > > > Specifically, when a DWC3 controller exits host mode and removes xhci
> > > > > while a reset is still in progress, and then attempts to configure its
> > > > > hardware for device mode, the ongoing, incomplete reset leads to
> > > > > critical register access issues. All register reads return zero, not
> > > > > just within the xHCI register space (which might be expected during a
> > > > > reset), but across the entire DWC3 IP block.
> > > > > 
> > > > > This patch addresses the issue by preventing xhci_reset() from being
> > > > > called in xhci_resume() and bailing out early in the reinit flow when
> > > > > XHCI_STATE_REMOVING is set.
> > > > > 
> > > > > Cc: stable@vger.kernel.org
> > > > > Fixes: 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state() helper")
> > > > > Suggested-by: Mathias Nyman <mathias.nyman@intel.com>
> > > > > Signed-off-by: Roy Luo <royluo@google.com>
> > > > > ---
> > > > >    drivers/usb/host/xhci.c | 5 ++++-
> > > > >    1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > > > index 90eb491267b5..244b12eafd95 100644
> > > > > --- a/drivers/usb/host/xhci.c
> > > > > +++ b/drivers/usb/host/xhci.c
> > > > > @@ -1084,7 +1084,10 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
> > > > >    		xhci_dbg(xhci, "Stop HCD\n");
> > > > >    		xhci_halt(xhci);
> > > > >    		xhci_zero_64b_regs(xhci);
> > > > > -		retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
> > > > > +		if (xhci->xhc_state & XHCI_STATE_REMOVING)
> > > > > +			retval = -ENODEV;
> > > > > +		else
> > > > > +			retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
> > > > 
> > > > How can this prevent the xhc_state from changing while in reset? There's
> > > > no locking in xhci-plat.
> > > 
> > > Patch 2/2, which is the revert of 6ccb83d6c497 prevents xhci_reset() from
> > > aborting due to xhc_state flags change.
> > > 
> > > This patch makes sure xHC is not reset twice if xhci is resuming due to
> > > remove being called. (XHCI_STATE_REMOVING is set).
> > 
> > Wouldn't it still be possible for xhci to be removed in the middle of
> > reset on resume? The watchdog may still timeout afterward if there's an
> > issue with reset right?
> > 
> 
> Probably yes, but that problem is the same if we only revert 6ccb83d6c497.
> 
> > > Why intentionally bring back the Qcom watchdog issue by only reverting
> > > 6ccb83d6c497 ?. Can't we solve both in one go?
> > 
> > I feel that the fix is doesn't cover all the scenarios, that's why I
> > suggest the revert for now and wait until the fix is properly tested
> > before applying it to stable?
> 
> Ok, we have different views on this.
> 
> I think we should avoid causing as much known regression as possible even
> if the patch  might not cover all scenarios.
> 
> By reverting 6ccb83d6c497 we fix a SNPS DWC3 regression, but at the same
> time bring back the Qcom issue, so cause another regression.
> 
> We can avoid the main part or the Qcom regression by adding this patch as
> issue is with (long) xhci reset during resume if xhci is being removed, and
> driver always resumes xhci during ->remove callback.
> 
> If we discover the patch is not perfect then we fix it
> 

Ok. Fair enough.

BR,
Thinh

Re: [PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci_resume if xhci is being removed

[Roy Luo]

On Wed, Jun 4, 2025 at 5:18 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Wed, Jun 04, 2025, Mathias Nyman wrote:
> > On 29.5.2025 4.17, Thinh Nguyen wrote:
> > > On Mon, May 26, 2025, Mathias Nyman wrote:
> > > > On 24.5.2025 2.06, Thinh Nguyen wrote:
> > > > > Hi Mathias, Roy,
> > > > >
> > > > > On Thu, May 22, 2025, Roy Luo wrote:
> > > > > > xhci_reset() currently returns -ENODEV if XHCI_STATE_REMOVING is
> > > > > > set, without completing the xhci handshake, unless the reset completes
> > > > > > exceptionally quickly. This behavior causes a regression on Synopsys
> > > > > > DWC3 USB controllers with dual-role capabilities.
> > > > > >
> > > > > > Specifically, when a DWC3 controller exits host mode and removes xhci
> > > > > > while a reset is still in progress, and then attempts to configure its
> > > > > > hardware for device mode, the ongoing, incomplete reset leads to
> > > > > > critical register access issues. All register reads return zero, not
> > > > > > just within the xHCI register space (which might be expected during a
> > > > > > reset), but across the entire DWC3 IP block.
> > > > > >
> > > > > > This patch addresses the issue by preventing xhci_reset() from being
> > > > > > called in xhci_resume() and bailing out early in the reinit flow when
> > > > > > XHCI_STATE_REMOVING is set.
> > > > > >
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Fixes: 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state() helper")
> > > > > > Suggested-by: Mathias Nyman <mathias.nyman@intel.com>
> > > > > > Signed-off-by: Roy Luo <royluo@google.com>
> > > > > > ---
> > > > > >    drivers/usb/host/xhci.c | 5 ++++-
> > > > > >    1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > > > > index 90eb491267b5..244b12eafd95 100644
> > > > > > --- a/drivers/usb/host/xhci.c
> > > > > > +++ b/drivers/usb/host/xhci.c
> > > > > > @@ -1084,7 +1084,10 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
> > > > > >               xhci_dbg(xhci, "Stop HCD\n");
> > > > > >               xhci_halt(xhci);
> > > > > >               xhci_zero_64b_regs(xhci);
> > > > > > -             retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
> > > > > > +             if (xhci->xhc_state & XHCI_STATE_REMOVING)
> > > > > > +                     retval = -ENODEV;
> > > > > > +             else
> > > > > > +                     retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
> > > > >
> > > > > How can this prevent the xhc_state from changing while in reset? There's
> > > > > no locking in xhci-plat.
> > > >
> > > > Patch 2/2, which is the revert of 6ccb83d6c497 prevents xhci_reset() from
> > > > aborting due to xhc_state flags change.
> > > >
> > > > This patch makes sure xHC is not reset twice if xhci is resuming due to
> > > > remove being called. (XHCI_STATE_REMOVING is set).
> > >
> > > Wouldn't it still be possible for xhci to be removed in the middle of
> > > reset on resume? The watchdog may still timeout afterward if there's an
> > > issue with reset right?
> > >
> >
> > Probably yes, but that problem is the same if we only revert 6ccb83d6c497.
> >
> > > > Why intentionally bring back the Qcom watchdog issue by only reverting
> > > > 6ccb83d6c497 ?. Can't we solve both in one go?
> > >
> > > I feel that the fix is doesn't cover all the scenarios, that's why I
> > > suggest the revert for now and wait until the fix is properly tested
> > > before applying it to stable?
> >
> > Ok, we have different views on this.
> >
> > I think we should avoid causing as much known regression as possible even
> > if the patch  might not cover all scenarios.
> >
> > By reverting 6ccb83d6c497 we fix a SNPS DWC3 regression, but at the same
> > time bring back the Qcom issue, so cause another regression.
> >
> > We can avoid the main part or the Qcom regression by adding this patch as
> > issue is with (long) xhci reset during resume if xhci is being removed, and
> > driver always resumes xhci during ->remove callback.
> >
> > If we discover the patch is not perfect then we fix it
> >
>
> Ok. Fair enough.
>
> BR,
> Thinh

Thanks Thinh and Mathias for the review.
Please let me know if any further changes are needed before these
patches can be accepted.
I just want to make sure they're still on your radar.

Thanks,
Roy

Re: [PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci_resume if xhci is being removed

[Mathias Nyman]

> Thanks Thinh and Mathias for the review.
> Please let me know if any further changes are needed before these
> patches can be accepted.
> I just want to make sure they're still on your radar.
> 
> Thanks,
> Roy
> 

I think Greg just picked up these two.

-Mathias

Re: [PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci_resume if xhci is being removed

[gregkh]

On Thu, Jun 19, 2025 at 03:44:25PM +0300, Mathias Nyman wrote:
> > Thanks Thinh and Mathias for the review.
> > Please let me know if any further changes are needed before these
> > patches can be accepted.
> > I just want to make sure they're still on your radar.
> > 
> > Thanks,
> > Roy
> > 
> 
> I think Greg just picked up these two.

I did, if I need to revert them, please let me know.

thanks,

greg k-h