* [PATCH] usb: core: hub: Improved device recognition on remote wakeup
@ 2020-01-09 5:14 Keiya Nobuta
2020-01-09 15:22 ` Alan Stern
2020-01-09 15:46 ` Greg KH
0 siblings, 2 replies; 4+ messages in thread
From: Keiya Nobuta @ 2020-01-09 5:14 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Keiya Nobuta
If hub_activate() is called before D+ has stabilized after remote
wakeup, the following situation might occur:
__ ___________________
/ \ /
D+ __/ \__/
Hub _______________________________
| ^ ^ ^
| | | |
Host _____v__|___|___________|______
| | | |
| | | \-- Interrupt Transfer (*3)
| | \-- ClearPortFeature (*2)
| \-- GetPortStatus (*1)
\-- Host detects remote wakeup
- D+ goes high, Host starts running by remote wakeup
- D+ is not stable, goes low
- Host requests GetPortStatus at (*1) and gets the following hub status:
- Current Connect Status bit is 0
- Connect Status Change bit is 1
- D+ stabilizes, goes high
- Host requests ClearPortFeature and thus Connect Status Change bit is
cleared at (*2)
- After waiting 100 ms, Host starts the Interrupt Transfer at (*3)
- Since the Connect Status Change bit is 0, Hub returns NAK.
In this case, port_event() is not called in hub_event() and Host cannot
recognize device. To solve this issue, flag change_bits even if only
Connect Status Change bit is 1 when got in the first GetPortStatus.
This issue occurs rarely because it only if D+ changes during a very
short time between GetPortStatus and ClearPortFeature. However, it is
fatal if it occurs in embedded system.
Signed-off-by: Keiya Nobuta <nobuta.keiya@fujitsu.com>
---
drivers/usb/core/hub.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f229ad6..77f8eb1 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1192,6 +1192,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
* PORT_OVER_CURRENT is not. So check for any of them.
*/
if (udev || (portstatus & USB_PORT_STAT_CONNECTION) ||
+ (portchange & USB_PORT_STAT_C_CONNECTION) ||
(portstatus & USB_PORT_STAT_OVERCURRENT) ||
(portchange & USB_PORT_STAT_C_OVERCURRENT))
set_bit(port1, hub->change_bits);
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: core: hub: Improved device recognition on remote wakeup
2020-01-09 5:14 [PATCH] usb: core: hub: Improved device recognition on remote wakeup Keiya Nobuta
@ 2020-01-09 15:22 ` Alan Stern
2020-01-09 15:46 ` Greg KH
1 sibling, 0 replies; 4+ messages in thread
From: Alan Stern @ 2020-01-09 15:22 UTC (permalink / raw)
To: Keiya Nobuta; +Cc: gregkh, linux-usb
On Thu, 9 Jan 2020, Keiya Nobuta wrote:
> If hub_activate() is called before D+ has stabilized after remote
> wakeup, the following situation might occur:
>
> __ ___________________
> / \ /
> D+ __/ \__/
>
> Hub _______________________________
> | ^ ^ ^
> | | | |
> Host _____v__|___|___________|______
> | | | |
> | | | \-- Interrupt Transfer (*3)
> | | \-- ClearPortFeature (*2)
> | \-- GetPortStatus (*1)
> \-- Host detects remote wakeup
>
> - D+ goes high, Host starts running by remote wakeup
> - D+ is not stable, goes low
> - Host requests GetPortStatus at (*1) and gets the following hub status:
> - Current Connect Status bit is 0
> - Connect Status Change bit is 1
> - D+ stabilizes, goes high
> - Host requests ClearPortFeature and thus Connect Status Change bit is
> cleared at (*2)
> - After waiting 100 ms, Host starts the Interrupt Transfer at (*3)
> - Since the Connect Status Change bit is 0, Hub returns NAK.
>
> In this case, port_event() is not called in hub_event() and Host cannot
> recognize device. To solve this issue, flag change_bits even if only
> Connect Status Change bit is 1 when got in the first GetPortStatus.
>
> This issue occurs rarely because it only if D+ changes during a very
> short time between GetPortStatus and ClearPortFeature. However, it is
> fatal if it occurs in embedded system.
>
> Signed-off-by: Keiya Nobuta <nobuta.keiya@fujitsu.com>
> ---
> drivers/usb/core/hub.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index f229ad6..77f8eb1 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1192,6 +1192,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
> * PORT_OVER_CURRENT is not. So check for any of them.
> */
> if (udev || (portstatus & USB_PORT_STAT_CONNECTION) ||
> + (portchange & USB_PORT_STAT_C_CONNECTION) ||
> (portstatus & USB_PORT_STAT_OVERCURRENT) ||
> (portchange & USB_PORT_STAT_C_OVERCURRENT))
> set_bit(port1, hub->change_bits);
I would have added the new test one line lower, so that all the
portstatus checks were adjacent and all the portchange checks were
adjacent. But that's a very small thing...
Acked-by: Alan Stern <stern@rowland.harvard.edu>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: core: hub: Improved device recognition on remote wakeup
2020-01-09 5:14 [PATCH] usb: core: hub: Improved device recognition on remote wakeup Keiya Nobuta
2020-01-09 15:22 ` Alan Stern
@ 2020-01-09 15:46 ` Greg KH
2020-01-10 0:48 ` nobuta.keiya
1 sibling, 1 reply; 4+ messages in thread
From: Greg KH @ 2020-01-09 15:46 UTC (permalink / raw)
To: Keiya Nobuta; +Cc: linux-usb
On Thu, Jan 09, 2020 at 02:14:48PM +0900, Keiya Nobuta wrote:
> If hub_activate() is called before D+ has stabilized after remote
> wakeup, the following situation might occur:
>
> __ ___________________
> / \ /
> D+ __/ \__/
>
> Hub _______________________________
> | ^ ^ ^
> | | | |
> Host _____v__|___|___________|______
> | | | |
> | | | \-- Interrupt Transfer (*3)
> | | \-- ClearPortFeature (*2)
> | \-- GetPortStatus (*1)
> \-- Host detects remote wakeup
>
> - D+ goes high, Host starts running by remote wakeup
> - D+ is not stable, goes low
> - Host requests GetPortStatus at (*1) and gets the following hub status:
> - Current Connect Status bit is 0
> - Connect Status Change bit is 1
> - D+ stabilizes, goes high
> - Host requests ClearPortFeature and thus Connect Status Change bit is
> cleared at (*2)
> - After waiting 100 ms, Host starts the Interrupt Transfer at (*3)
> - Since the Connect Status Change bit is 0, Hub returns NAK.
>
> In this case, port_event() is not called in hub_event() and Host cannot
> recognize device. To solve this issue, flag change_bits even if only
> Connect Status Change bit is 1 when got in the first GetPortStatus.
>
> This issue occurs rarely because it only if D+ changes during a very
> short time between GetPortStatus and ClearPortFeature. However, it is
> fatal if it occurs in embedded system.
>
> Signed-off-by: Keiya Nobuta <nobuta.keiya@fujitsu.com>
> ---
> drivers/usb/core/hub.c | 1 +
> 1 file changed, 1 insertion(+)
Is this something that should go into 5.5-final and the stable trees?
Or is it ok for 5.6-rc1?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-01-10 0:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-09 5:14 [PATCH] usb: core: hub: Improved device recognition on remote wakeup Keiya Nobuta
2020-01-09 15:22 ` Alan Stern
2020-01-09 15:46 ` Greg KH
2020-01-10 0:48 ` nobuta.keiya
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).