From: Daehwan Jung <[email protected]>
xhci_reset() is called with interrupts disabled. Waiting 10 seconds for
controller reset and controller ready operations can be fatal to the
system when controller is timed out. Reduce the timeout to 1 second
and print a error message when the time out happens.
Fixes: 22ceac191211 ("xhci: Increase reset timeout for Renesas 720201 host.")
Signed-off-by: Daehwan Jung <[email protected]>
Signed-off-by: Pavankumar Kondeti <[email protected]>
---
v2:
- Add error print statements in the code that change log refers to
drivers/usb/host/xhci.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index dc357ca..bb9ea3f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -196,9 +196,11 @@ int xhci_reset(struct xhci_hcd *xhci)
udelay(1000);
ret = xhci_handshake(&xhci->op_regs->command,
- CMD_RESET, 0, 10 * 1000 * 1000);
- if (ret)
+ CMD_RESET, 0, 1 * 1000 * 1000);
+ if (ret) {
+ xhci_err(xhci, "Host controller reset timed out\n");
return ret;
+ }
if (xhci->quirks & XHCI_ASMEDIA_MODIFY_FLOWCONTROL)
usb_asmedia_modifyflowcontrol(to_pci_dev(xhci_to_hcd(xhci)->self.controller));
@@ -210,7 +212,11 @@ int xhci_reset(struct xhci_hcd *xhci)
* than status until the "Controller Not Ready" flag is cleared.
*/
ret = xhci_handshake(&xhci->op_regs->status,
- STS_CNR, 0, 10 * 1000 * 1000);
+ STS_CNR, 0, 1 * 1000 * 1000);
+ if (ret) {
+ xhci_err(xhci, "Host controller is not ready within timeout\n");
+ return ret;
+ }
xhci->usb2_rhub.bus_state.port_c_suspend = 0;
xhci->usb2_rhub.bus_state.suspended_ports = 0;
--
2.7.4
On 14.2.2022 14.20, Pavankumar Kondeti wrote:
> From: Daehwan Jung <[email protected]>
>
> xhci_reset() is called with interrupts disabled. Waiting 10 seconds for
> controller reset and controller ready operations can be fatal to the
> system when controller is timed out. Reduce the timeout to 1 second
> and print a error message when the time out happens.
>
> Fixes: 22ceac191211 ("xhci: Increase reset timeout for Renesas 720201 host.")
The commit 22ceac191211 ("xhci: Increase reset timeout for Renesas 720201 host.")
intentionally increased the timeout to 10 seconds as that host might take 9
seconds to complete reset. This was done almost 10 years ago so I don't know
if it really is an issue anymore.
Anyways, your patch might break Renesas 72021 instead of fixing it.
I agree that busylooping up to 10 seconds with interrupts disabled doesn't make sense.
Lets see if there is another solution for your case.
- Does a "guard interval" after writing the reset help?
For example Intel xHCI needs 1ms before touching xHC after writing the reset bit
- Is it the CNR bit or the RESET bit that fails? could be just stuck CNR bit?
- we only disable local interrupts when xhci_reset() is called from xhci_stop(),
and sometimes from xhci_shutdown() and xhci_resume() if some conditions are met.
Have you identified which one is the problematic case?
I think we halt the host in the above case first, meaning there should be no
xHC interrupts when xhci_reset() is called. So if we could guarantee xhci interrupt
isn't handled on this cpu, maybe we could somehow enable local interrupt after
halting the host?
haven't really thought this true yet, but something like this could e investigated:
spin_lock_irqsave()
xhci_halt()
< enable interrupts, magically turn spin_lock_irqsave() to just keeping spin lock>
xhci_reset()
spin_unlock()
-Mathias
On 15.02.22 09:51, Jung Daehwan wrote:
>
> I also think it doesn't make sense 10 secs timeout with irqs disabled.
> It could cause critical system problem as Pavan said. How about adding
> new quirk for different timeout value?
>
Hi,
I am afraid this amounts to putting bandages onto a fundamental
flaw. We need a version of xhci_reset() that can sleep and if
you need interrupts to be off to reset the HC, you are doing a bad design.
Regards
Oliver
On 15.2.2022 19.07, Pavan Kondeti wrote:
>>>>
>>>> The crash reports I have seen are pointing to
>>>>
>>>> usb_remove_hcd()->xhci_stop()->xhci_reset()
>>>
>>> Ok, so xhci_stop() and xhci_shutdown() both may call xhci_reset() with interrupts
>>> disabled and spinlock held. In both these cases we're not that interested in the
>>> outcome of xhci_reset().
>>>
>>> But during probe we call xhci_reset() with interrupts enabled without spinlock,
>>> and here we really care about it succeeding.
>>> I'm also guessing reset could take a longer time during probe due to possible recent
>>> BIOS handover, or firmware loading etc.
>>>
>>> So how about passing a timeout value to xhci_reset()?
>>> Give it 10 seconds during probe, and 250ms in the other cases.
>>>
>>
>> Thanks for this suggestion.
>>
>> This sounds better compared to the quirks approach. xhci_resume() also seems
>> to be calling xhci_reset() in the hibernation path, I believe we should treat
>> this like probe()/startup case and give larger timeout.
>>
> I will test the below patch as per Mathias suggestion.
>
> Thanks,
> Pavan
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index df3522d..031fe90 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -762,7 +762,7 @@ static int xhci_exit_test_mode(struct xhci_hcd *xhci)
> }
> pm_runtime_allow(xhci_to_hcd(xhci)->self.controller);
> xhci->test_mode = 0;
> - return xhci_reset(xhci);
> + return xhci_reset(xhci, false);
Maybe just pass the timeout value directly to xhci_reset().
Looks like readl_poll_timeout_atomic() uses u64 for timeout_us,
makes sense to use the same.
Sergey also pointed out xhci_handshake() incorrectly uses a signed integer for timeouts.
This could be changed to u64 as well.
I'll write a patch that does all above
-Mathias
On 2/16/22 6:58 PM, Mathias Nyman wrote:
>>>>> The crash reports I have seen are pointing to
>>>>>
>>>>> usb_remove_hcd()->xhci_stop()->xhci_reset()
>>>>
>>>> Ok, so xhci_stop() and xhci_shutdown() both may call xhci_reset() with interrupts
>>>> disabled and spinlock held. In both these cases we're not that interested in the
>>>> outcome of xhci_reset().
>>>>
>>>> But during probe we call xhci_reset() with interrupts enabled without spinlock,
>>>> and here we really care about it succeeding.
>>>> I'm also guessing reset could take a longer time during probe due to possible recent
>>>> BIOS handover, or firmware loading etc.
>>>>
>>>> So how about passing a timeout value to xhci_reset()?
>>>> Give it 10 seconds during probe, and 250ms in the other cases.
>>>>
>>>
>>> Thanks for this suggestion.
>>>
>>> This sounds better compared to the quirks approach. xhci_resume() also seems
>>> to be calling xhci_reset() in the hibernation path, I believe we should treat
>>> this like probe()/startup case and give larger timeout.
>>>
>> I will test the below patch as per Mathias suggestion.
>>
>> Thanks,
>> Pavan
>>
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index df3522d..031fe90 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -762,7 +762,7 @@ static int xhci_exit_test_mode(struct xhci_hcd *xhci)
>> }
>> pm_runtime_allow(xhci_to_hcd(xhci)->self.controller);
>> xhci->test_mode = 0;
>> - return xhci_reset(xhci);
>> + return xhci_reset(xhci, false);
>
> Maybe just pass the timeout value directly to xhci_reset().
> Looks like readl_poll_timeout_atomic() uses u64 for timeout_us,
> makes sense to use the same.
>
> Sergey also pointed out xhci_handshake() incorrectly uses a signed integer for timeouts.
> This could be changed to u64 as well.
>
> I'll write a patch that does all above
You mean I don't need to respin my xhci_handshake() patch?
I'm happy to do that if that's a prevailing opinion. :-)
> -Mathias
MBR, Sergey
Hi Mathias,
On Wed, Feb 16, 2022 at 05:58:15PM +0200, Mathias Nyman wrote:
> On 15.2.2022 19.07, Pavan Kondeti wrote:
> >>>>
> >>>> The crash reports I have seen are pointing to
> >>>>
> >>>> usb_remove_hcd()->xhci_stop()->xhci_reset()
> >>>
> >>> Ok, so xhci_stop() and xhci_shutdown() both may call xhci_reset() with interrupts
> >>> disabled and spinlock held. In both these cases we're not that interested in the
> >>> outcome of xhci_reset().
> >>>
> >>> But during probe we call xhci_reset() with interrupts enabled without spinlock,
> >>> and here we really care about it succeeding.
> >>> I'm also guessing reset could take a longer time during probe due to possible recent
> >>> BIOS handover, or firmware loading etc.
> >>>
> >>> So how about passing a timeout value to xhci_reset()?
> >>> Give it 10 seconds during probe, and 250ms in the other cases.
> >>>
> >>
> >> Thanks for this suggestion.
> >>
> >> This sounds better compared to the quirks approach. xhci_resume() also seems
> >> to be calling xhci_reset() in the hibernation path, I believe we should treat
> >> this like probe()/startup case and give larger timeout.
> >>
> > I will test the below patch as per Mathias suggestion.
> >
> > Thanks,
> > Pavan
> >
> > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> > index df3522d..031fe90 100644
> > --- a/drivers/usb/host/xhci-hub.c
> > +++ b/drivers/usb/host/xhci-hub.c
> > @@ -762,7 +762,7 @@ static int xhci_exit_test_mode(struct xhci_hcd *xhci)
> > }
> > pm_runtime_allow(xhci_to_hcd(xhci)->self.controller);
> > xhci->test_mode = 0;
> > - return xhci_reset(xhci);
> > + return xhci_reset(xhci, false);
>
> Maybe just pass the timeout value directly to xhci_reset().
> Looks like readl_poll_timeout_atomic() uses u64 for timeout_us,
> makes sense to use the same.
>
> Sergey also pointed out xhci_handshake() incorrectly uses a signed integer for timeouts.
> This could be changed to u64 as well.
>
> I'll write a patch that does all above
>
Thank you. I will look forward to your patch and provide the test results with
it.
Thanks,
Pavan