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: <459184db-6fc6-453b-933d-299f827bdc55@linux.intel.com>
Date: Wed, 4 Jun 2025 17:17:14 +0300
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc: "mathias.nyman@...el.com" <mathias.nyman@...el.com>,
 Roy Luo <royluo@...gle.com>,
 "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 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@...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).
> 
> 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