| Message ID | 1618017645-12259-1-git-send-email-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] usb: core: reduce power-on-good delay time of root hub | expand |
Three month ago, My USB 3.1 device cannot be detected after upgrade kernel from version 5.10.41 to 5.10.42. And I found some other people ran into the same issue: https://forum.manjaro.org/t/kernel-5-10-59-doesnt-detect-a-connected-usb-external-disk/79320 https://forum.manjaro.org/t/how-to-make-active-usb-hub-work-in-manjaro/84548 Two days ago, I try to fix the issue myself, and I found out that it was cause by this patch. After revert this patch, my USB 3.1 device can be detect again. I have already file a bug on here: https://bugzilla.kernel.org/show_bug.cgi?id=214875.
On Sat, Oct 30, 2021 at 12:49:37PM +0800, Walt Jr. Brake wrote: > This patch make USB 3.1 device cannot be detected, and I report the bug [1] > to archlinux three month ago. Yesterday I try to fix it myself, and after I > revert this patch, compile the kernel and test, it works. > > [1] https://bugs.archlinux.org/task/71660?project=1&pagenum=2 > > > diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h > index 22ea1f4f2d66..73f4482d833a 100644 > --- a/drivers/usb/core/hub.h > +++ b/drivers/usb/core/hub.h > @@ -148,10 +148,8 @@ static inline unsigned hub_power_on_good_delay(struct > usb_hub *hub) > { > unsigned delay = hub->descriptor->bPwrOn2PwrGood * 2; > > - if (!hub->hdev->parent) /* root hub */ > - return delay; > - else /* Wait at least 100 msec for power to become stable */ > - return max(delay, 100U); > + /* Wait at least 100 msec for power to become stable */ > + return max(delay, 100U); > } Mathias: It looks like the bPwrOn2PwrGood value in xhci-hcd's hub descriptor is too small for some USB 3.1 devices. Can you look into this? Alan Stern > On 10/4/2021 09:20, Chunfeng Yun wrote: > > Return the exactly delay time given by root hub descriptor, > > this helps to reduce resume time etc. > > > > Due to the root hub descriptor is usually provided by the host > > controller driver, if there is compatibility for a root hub, > > we can fix it easily without affect other root hub > > > > Acked-by: Alan Stern<stern@rowland.harvard.edu> > > Signed-off-by: Chunfeng Yun<chunfeng.yun@mediatek.com> > > --- > > v2: remove RFC tag, and add acked-by Alan > > --- > > drivers/usb/core/hub.h | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h > > index 73f4482d833a..22ea1f4f2d66 100644 > > --- a/drivers/usb/core/hub.h > > +++ b/drivers/usb/core/hub.h > > @@ -148,8 +148,10 @@ static inline unsigned hub_power_on_good_delay(struct usb_hub *hub) > > { > > unsigned delay = hub->descriptor->bPwrOn2PwrGood * 2; > > - /* Wait at least 100 msec for power to become stable */ > > - return max(delay, 100U); > > + if (!hub->hdev->parent) /* root hub */ > > + return delay; > > + else /* Wait at least 100 msec for power to become stable */ > > + return max(delay, 100U); > > } > > static inline int hub_port_debounce_be_connected(struct usb_hub *hub,
On 1.11.2021 16.06, Alan Stern wrote: > On Sat, Oct 30, 2021 at 12:49:37PM +0800, Walt Jr. Brake wrote: >> This patch make USB 3.1 device cannot be detected, and I report the bug [1] >> to archlinux three month ago. Yesterday I try to fix it myself, and after I >> revert this patch, compile the kernel and test, it works. >> >> [1] https://bugs.archlinux.org/task/71660?project=1&pagenum=2 >> >> >> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h >> index 22ea1f4f2d66..73f4482d833a 100644 >> --- a/drivers/usb/core/hub.h >> +++ b/drivers/usb/core/hub.h >> @@ -148,10 +148,8 @@ static inline unsigned hub_power_on_good_delay(struct >> usb_hub *hub) >> { >> unsigned delay = hub->descriptor->bPwrOn2PwrGood * 2; >> >> - if (!hub->hdev->parent) /* root hub */ >> - return delay; >> - else /* Wait at least 100 msec for power to become stable */ >> - return max(delay, 100U); >> + /* Wait at least 100 msec for power to become stable */ >> + return max(delay, 100U); >> } > > Mathias: > > It looks like the bPwrOn2PwrGood value in xhci-hcd's hub descriptor is > too small for some USB 3.1 devices. > > Can you look into this? > > Alan Stern > At first glance the xhci roothub bPwrOn2PwrGood value looks ok. xhci spec 5.4.8 states software should wait 20ms after asserting PP, before attempting to change the state of the port. xhci driver sets desc->bPwrOn2PwrGood = 10; (2ms interval, so equals 20ms ) We should probably get this working immediately, so maybe revert that patch while looking into the rootcause. Walt Jr. Brake, instead of reverting that patch, could you test if changing the xhci roothub bPwrOn2PwrGood value helps. diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index a3f875eea751..756231a55602 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -257,7 +257,7 @@ static void xhci_common_hub_descriptor(struct xhci_hcd *xhci, { u16 temp; - desc->bPwrOn2PwrGood = 10; /* xhci section 5.4.9 says 20ms max */ + desc->bPwrOn2PwrGood = 50; /* The 20ms in xhci 5.4.8 isn't enough for USB 3.1 */ desc->bHubContrCurrent = 0; desc->bNbrPorts = ports; Thanks -Mathias
On 2/11/2021 17:05, Mathias Nyman wrote: > On 1.11.2021 16.06, Alan Stern wrote: >> On Sat, Oct 30, 2021 at 12:49:37PM +0800, Walt Jr. Brake wrote: >>> This patch make USB 3.1 device cannot be detected, and I report the bug [1] >>> to archlinux three month ago. Yesterday I try to fix it myself, and after I >>> revert this patch, compile the kernel and test, it works. >>> >>> [1] https://bugs.archlinux.org/task/71660?project=1&pagenum=2 >>> >>> >>> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h >>> index 22ea1f4f2d66..73f4482d833a 100644 >>> --- a/drivers/usb/core/hub.h >>> +++ b/drivers/usb/core/hub.h >>> @@ -148,10 +148,8 @@ static inline unsigned hub_power_on_good_delay(struct >>> usb_hub *hub) >>> { >>> unsigned delay = hub->descriptor->bPwrOn2PwrGood * 2; >>> >>> - if (!hub->hdev->parent) /* root hub */ >>> - return delay; >>> - else /* Wait at least 100 msec for power to become stable */ >>> - return max(delay, 100U); >>> + /* Wait at least 100 msec for power to become stable */ >>> + return max(delay, 100U); >>> } >> Mathias: >> >> It looks like the bPwrOn2PwrGood value in xhci-hcd's hub descriptor is >> too small for some USB 3.1 devices. >> >> Can you look into this? >> >> Alan Stern >> > At first glance the xhci roothub bPwrOn2PwrGood value looks ok. > xhci spec 5.4.8 states software should wait 20ms after asserting PP, before > attempting to change the state of the port. > > xhci driver sets desc->bPwrOn2PwrGood = 10; (2ms interval, so equals 20ms ) > > We should probably get this working immediately, so maybe revert that patch > while looking into the rootcause. > > Walt Jr. Brake, instead of reverting that patch, could you test if changing the > xhci roothub bPwrOn2PwrGood value helps. > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > index a3f875eea751..756231a55602 100644 > --- a/drivers/usb/host/xhci-hub.c > +++ b/drivers/usb/host/xhci-hub.c > @@ -257,7 +257,7 @@ static void xhci_common_hub_descriptor(struct xhci_hcd *xhci, > { > u16 temp; > > - desc->bPwrOn2PwrGood = 10; /* xhci section 5.4.9 says 20ms max */ > + desc->bPwrOn2PwrGood = 50; /* The 20ms in xhci 5.4.8 isn't enough for USB 3.1 */ > desc->bHubContrCurrent = 0; > > desc->bNbrPorts = ports; > > Thanks > -Mathias Mathias: Sorry to reply lately. I test with your patch, it works. I also test with setting bPwrOn2PwrGood to 45, and it not work. Seems that the minimal value should be 50 for this case.
On 2.11.2021 22.29, Walt Jr. Brake wrote: > On 2/11/2021 17:05, Mathias Nyman wrote: >> On 1.11.2021 16.06, Alan Stern wrote: >>> On Sat, Oct 30, 2021 at 12:49:37PM +0800, Walt Jr. Brake wrote: >>>> This patch make USB 3.1 device cannot be detected, and I report the bug [1] >>>> to archlinux three month ago. Yesterday I try to fix it myself, and after I >>>> revert this patch, compile the kernel and test, it works. >>>> >>>> [1] https://bugs.archlinux.org/task/71660?project=1&pagenum=2 >>>> >>>> >>>> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h >>>> index 22ea1f4f2d66..73f4482d833a 100644 >>>> --- a/drivers/usb/core/hub.h >>>> +++ b/drivers/usb/core/hub.h >>>> @@ -148,10 +148,8 @@ static inline unsigned hub_power_on_good_delay(struct >>>> usb_hub *hub) >>>> { >>>> unsigned delay = hub->descriptor->bPwrOn2PwrGood * 2; >>>> >>>> - if (!hub->hdev->parent) /* root hub */ >>>> - return delay; >>>> - else /* Wait at least 100 msec for power to become stable */ >>>> - return max(delay, 100U); >>>> + /* Wait at least 100 msec for power to become stable */ >>>> + return max(delay, 100U); >>>> } >>> Mathias: >>> >>> It looks like the bPwrOn2PwrGood value in xhci-hcd's hub descriptor is >>> too small for some USB 3.1 devices. >>> >>> Can you look into this? >>> >>> Alan Stern >>> >> At first glance the xhci roothub bPwrOn2PwrGood value looks ok. >> xhci spec 5.4.8 states software should wait 20ms after asserting PP, before >> attempting to change the state of the port. >> >> xhci driver sets desc->bPwrOn2PwrGood = 10; (2ms interval, so equals 20ms ) >> >> We should probably get this working immediately, so maybe revert that patch >> while looking into the rootcause. >> >> Walt Jr. Brake, instead of reverting that patch, could you test if changing the >> xhci roothub bPwrOn2PwrGood value helps. >> >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c >> index a3f875eea751..756231a55602 100644 >> --- a/drivers/usb/host/xhci-hub.c >> +++ b/drivers/usb/host/xhci-hub.c >> @@ -257,7 +257,7 @@ static void xhci_common_hub_descriptor(struct xhci_hcd *xhci, >> { >> u16 temp; >> - desc->bPwrOn2PwrGood = 10; /* xhci section 5.4.9 says 20ms max */ >> + desc->bPwrOn2PwrGood = 50; /* The 20ms in xhci 5.4.8 isn't enough for USB 3.1 */ >> desc->bHubContrCurrent = 0; >> desc->bNbrPorts = ports; >> >> Thanks >> -Mathias > > Mathias: > > Sorry to reply lately. I test with your patch, it works. > > I also test with setting bPwrOn2PwrGood to 45, and it not work. > > Seems that the minimal value should be 50 for this case. > Thanks for testing, and for checking that 90ms wait isn't enough -Mathias
On Wed, Nov 03, 2021 at 10:37:33PM +0200, Mathias Nyman wrote: > On 2.11.2021 22.29, Walt Jr. Brake wrote: > > On 2/11/2021 17:05, Mathias Nyman wrote: > >> On 1.11.2021 16.06, Alan Stern wrote: > >>> On Sat, Oct 30, 2021 at 12:49:37PM +0800, Walt Jr. Brake wrote: > >>>> This patch make USB 3.1 device cannot be detected, and I report the bug [1] > >>>> to archlinux three month ago. Yesterday I try to fix it myself, and after I > >>>> revert this patch, compile the kernel and test, it works. > >>>> > >>>> [1] https://bugs.archlinux.org/task/71660?project=1&pagenum=2 > >>>> > >>>> > >>>> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h > >>>> index 22ea1f4f2d66..73f4482d833a 100644 > >>>> --- a/drivers/usb/core/hub.h > >>>> +++ b/drivers/usb/core/hub.h > >>>> @@ -148,10 +148,8 @@ static inline unsigned hub_power_on_good_delay(struct > >>>> usb_hub *hub) > >>>> { > >>>> unsigned delay = hub->descriptor->bPwrOn2PwrGood * 2; > >>>> > >>>> - if (!hub->hdev->parent) /* root hub */ > >>>> - return delay; > >>>> - else /* Wait at least 100 msec for power to become stable */ > >>>> - return max(delay, 100U); > >>>> + /* Wait at least 100 msec for power to become stable */ > >>>> + return max(delay, 100U); > >>>> } > >>> Mathias: > >>> > >>> It looks like the bPwrOn2PwrGood value in xhci-hcd's hub descriptor is > >>> too small for some USB 3.1 devices. > >>> > >>> Can you look into this? > >>> > >>> Alan Stern > >>> > >> At first glance the xhci roothub bPwrOn2PwrGood value looks ok. > >> xhci spec 5.4.8 states software should wait 20ms after asserting PP, before > >> attempting to change the state of the port. > >> > >> xhci driver sets desc->bPwrOn2PwrGood = 10; (2ms interval, so equals 20ms ) > >> > >> We should probably get this working immediately, so maybe revert that patch > >> while looking into the rootcause. > >> > >> Walt Jr. Brake, instead of reverting that patch, could you test if changing the > >> xhci roothub bPwrOn2PwrGood value helps. > >> > >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > >> index a3f875eea751..756231a55602 100644 > >> --- a/drivers/usb/host/xhci-hub.c > >> +++ b/drivers/usb/host/xhci-hub.c > >> @@ -257,7 +257,7 @@ static void xhci_common_hub_descriptor(struct xhci_hcd *xhci, > >> { > >> u16 temp; > >> - desc->bPwrOn2PwrGood = 10; /* xhci section 5.4.9 says 20ms max */ > >> + desc->bPwrOn2PwrGood = 50; /* The 20ms in xhci 5.4.8 isn't enough for USB 3.1 */ > >> desc->bHubContrCurrent = 0; > >> desc->bNbrPorts = ports; > >> > >> Thanks > >> -Mathias > > > > Mathias: > > > > Sorry to reply lately. I test with your patch, it works. > > > > I also test with setting bPwrOn2PwrGood to 45, and it not work. > > > > Seems that the minimal value should be 50 for this case. > > > > Thanks for testing, and for checking that 90ms wait isn't enough Can you send a "real" patch for this so I can get it into the tree soon to resolve the regression? thanks, greg k-h
On Fri, Nov 05, 2021 at 11:00:31AM +0100, Greg Kroah-Hartman wrote: > On Wed, Nov 03, 2021 at 10:37:33PM +0200, Mathias Nyman wrote: > > On 2.11.2021 22.29, Walt Jr. Brake wrote: > > > On 2/11/2021 17:05, Mathias Nyman wrote: > > >> On 1.11.2021 16.06, Alan Stern wrote: > > >>> On Sat, Oct 30, 2021 at 12:49:37PM +0800, Walt Jr. Brake wrote: > > >>>> This patch make USB 3.1 device cannot be detected, and I report the bug [1] > > >>>> to archlinux three month ago. Yesterday I try to fix it myself, and after I > > >>>> revert this patch, compile the kernel and test, it works. > > >>>> > > >>>> [1] https://bugs.archlinux.org/task/71660?project=1&pagenum=2 > > >>>> > > >>>> > > >>>> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h > > >>>> index 22ea1f4f2d66..73f4482d833a 100644 > > >>>> --- a/drivers/usb/core/hub.h > > >>>> +++ b/drivers/usb/core/hub.h > > >>>> @@ -148,10 +148,8 @@ static inline unsigned hub_power_on_good_delay(struct > > >>>> usb_hub *hub) > > >>>> { > > >>>> unsigned delay = hub->descriptor->bPwrOn2PwrGood * 2; > > >>>> > > >>>> - if (!hub->hdev->parent) /* root hub */ > > >>>> - return delay; > > >>>> - else /* Wait at least 100 msec for power to become stable */ > > >>>> - return max(delay, 100U); > > >>>> + /* Wait at least 100 msec for power to become stable */ > > >>>> + return max(delay, 100U); > > >>>> } > > >>> Mathias: > > >>> > > >>> It looks like the bPwrOn2PwrGood value in xhci-hcd's hub descriptor is > > >>> too small for some USB 3.1 devices. > > >>> > > >>> Can you look into this? > > >>> > > >>> Alan Stern > > >>> > > >> At first glance the xhci roothub bPwrOn2PwrGood value looks ok. > > >> xhci spec 5.4.8 states software should wait 20ms after asserting PP, before > > >> attempting to change the state of the port. > > >> > > >> xhci driver sets desc->bPwrOn2PwrGood = 10; (2ms interval, so equals 20ms ) > > >> > > >> We should probably get this working immediately, so maybe revert that patch > > >> while looking into the rootcause. > > >> > > >> Walt Jr. Brake, instead of reverting that patch, could you test if changing the > > >> xhci roothub bPwrOn2PwrGood value helps. > > >> > > >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > > >> index a3f875eea751..756231a55602 100644 > > >> --- a/drivers/usb/host/xhci-hub.c > > >> +++ b/drivers/usb/host/xhci-hub.c > > >> @@ -257,7 +257,7 @@ static void xhci_common_hub_descriptor(struct xhci_hcd *xhci, > > >> { > > >> u16 temp; > > >> - desc->bPwrOn2PwrGood = 10; /* xhci section 5.4.9 says 20ms max */ > > >> + desc->bPwrOn2PwrGood = 50; /* The 20ms in xhci 5.4.8 isn't enough for USB 3.1 */ > > >> desc->bHubContrCurrent = 0; > > >> desc->bNbrPorts = ports; > > >> > > >> Thanks > > >> -Mathias > > > > > > Mathias: > > > > > > Sorry to reply lately. I test with your patch, it works. > > > > > > I also test with setting bPwrOn2PwrGood to 45, and it not work. > > > > > > Seems that the minimal value should be 50 for this case. > > > > > > > Thanks for testing, and for checking that 90ms wait isn't enough > > Can you send a "real" patch for this so I can get it into the tree soon > to resolve the regression? Also, it might make sense to move the desc->bPwrOn2PwrGood setting from xhci_common_hub_descriptor into the speed-specific routines. For the USB-2 root hub the value can remain set to 10; only the USB-3 root hub needs to be changed. Alan Stern
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h index 73f4482d833a..22ea1f4f2d66 100644 --- a/drivers/usb/core/hub.h +++ b/drivers/usb/core/hub.h @@ -148,8 +148,10 @@ static inline unsigned hub_power_on_good_delay(struct usb_hub *hub) { unsigned delay = hub->descriptor->bPwrOn2PwrGood * 2; - /* Wait at least 100 msec for power to become stable */ - return max(delay, 100U); + if (!hub->hdev->parent) /* root hub */ + return delay; + else /* Wait at least 100 msec for power to become stable */ + return max(delay, 100U); } static inline int hub_port_debounce_be_connected(struct usb_hub *hub,