linux-kernel - Re: [PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci

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: <b982ff0e-1ae8-429d-aa11-c3e81a9c14e5@linux.intel.com>
Date: Mon, 26 May 2025 10:39:47 +0300
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
 "mathias.nyman@...el.com" <mathias.nyman@...el.com>,
 Roy Luo <royluo@...gle.com>
Cc: "quic_ugoswami@...cinc.com" <quic_ugoswami@...cinc.com>,
 "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
 "michal.pecio@...il.com" <michal.pecio@...il.com>,
 "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v1 1/2] usb: xhci: Skip xhci_reset in xhci_resume if xhci
 is being removed

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@...r.kernel.org
>> Fixes: 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state() helper")
>> Suggested-by: Mathias Nyman <mathias.nyman@...el.com>
>> Signed-off-by: Roy Luo <royluo@...gle.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