* [PATCH] usb: core: Set port link to RxDetect if port is not enabled after resume
@ 2020-03-05 7:28 Kai-Heng Feng
2020-03-05 19:02 ` Alan Stern
0 siblings, 1 reply; 3+ messages in thread
From: Kai-Heng Feng @ 2020-03-05 7:28 UTC (permalink / raw)
To: gregkh; +Cc: mathias.nyman, stern, linux-usb, linux-kernel, Kai-Heng Feng
On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
then connects to ASMedia xHCI's root hub:
/: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
|__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
|__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
The port is disabled after resume:
xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
According to xHCI 4.19.1.2.1, we should set link to RxDetect to transit
it from disabled state to disconnected state, which allows the port to
be set to U0 and completes the resume process.
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
P.S. This is and differnt (and should be more correct) approach to solve
https://lore.kernel.org/lkml/20191129174115.31683-1-kai.heng.feng@canonical.com/
drivers/usb/core/hub.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 54cd8ef795ec..352e33c84d6a 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3537,12 +3537,24 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, &portstatus, &portchange);
- if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (status == 0 && !port_is_suspended(hub, portstatus)
+ && (portstatus & USB_PORT_STAT_ENABLE)) {
if (portchange & USB_PORT_STAT_C_SUSPEND)
pm_wakeup_event(&udev->dev, 0);
goto SuspendCleared;
}
+ /* xHCI 4.19.1.2.1 */
+ if (hub_is_superspeed(hub->hdev)) {
+ if (!(portstatus & USB_PORT_STAT_ENABLE))
+ status = hub_set_port_link_state(hub, port1,
+ USB_SS_PORT_LS_RX_DETECT);
+
+ if (status)
+ dev_dbg(&port_dev->dev,
+ "can't set to RxDetect, status %d\n", status);
+ }
+
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U0);
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] usb: core: Set port link to RxDetect if port is not enabled after resume
2020-03-05 7:28 [PATCH] usb: core: Set port link to RxDetect if port is not enabled after resume Kai-Heng Feng
@ 2020-03-05 19:02 ` Alan Stern
2020-03-06 8:38 ` Kai-Heng Feng
0 siblings, 1 reply; 3+ messages in thread
From: Alan Stern @ 2020-03-05 19:02 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: gregkh, mathias.nyman, linux-usb, linux-kernel
On Thu, 5 Mar 2020, Kai-Heng Feng wrote:
> On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
> then connects to ASMedia xHCI's root hub:
>
> /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
> |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
> |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
>
> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
> Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
>
> The port is disabled after resume:
> xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
>
> According to xHCI 4.19.1.2.1, we should set link to RxDetect to transit
> it from disabled state to disconnected state, which allows the port to
> be set to U0 and completes the resume process.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>
> P.S. This is and differnt (and should be more correct) approach to solve
> https://lore.kernel.org/lkml/20191129174115.31683-1-kai.heng.feng@canonical.com/
>
> drivers/usb/core/hub.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 54cd8ef795ec..352e33c84d6a 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3537,12 +3537,24 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>
> /* Skip the initial Clear-Suspend step for a remote wakeup */
> status = hub_port_status(hub, port1, &portstatus, &portchange);
> - if (status == 0 && !port_is_suspended(hub, portstatus)) {
> + if (status == 0 && !port_is_suspended(hub, portstatus)
> + && (portstatus & USB_PORT_STAT_ENABLE)) {
This doesn't look right. We want to skip the step the clears the
port's suspend feature whenever the feature is already clear,
regardless of whether the port is enabled.
Besides, isn't this new test racy? Suppose the port was enabled when
we called hub_port_status above, but it got disabled in the nanoseconds
since then?
> if (portchange & USB_PORT_STAT_C_SUSPEND)
> pm_wakeup_event(&udev->dev, 0);
> goto SuspendCleared;
> }
>
> + /* xHCI 4.19.1.2.1 */
> + if (hub_is_superspeed(hub->hdev)) {
> + if (!(portstatus & USB_PORT_STAT_ENABLE))
> + status = hub_set_port_link_state(hub, port1,
> + USB_SS_PORT_LS_RX_DETECT);
> +
> + if (status)
> + dev_dbg(&port_dev->dev,
> + "can't set to RxDetect, status %d\n", status);
> + }
> +
So maybe this part belongs later in the routine. Or maybe it shouldn't
be in the hub driver at all -- since it is xHCI-specific, shouldn't it
go into the xhci-hcd driver?
Alan Stern
> /* see 7.1.7.7; affects power usage, but not budgeting */
> if (hub_is_superspeed(hub->hdev))
> status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U0);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] usb: core: Set port link to RxDetect if port is not enabled after resume
2020-03-05 19:02 ` Alan Stern
@ 2020-03-06 8:38 ` Kai-Heng Feng
0 siblings, 0 replies; 3+ messages in thread
From: Kai-Heng Feng @ 2020-03-06 8:38 UTC (permalink / raw)
To: Alan Stern; +Cc: gregkh, mathias.nyman, linux-usb, linux-kernel
> On Mar 6, 2020, at 03:02, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, 5 Mar 2020, Kai-Heng Feng wrote:
>
>> On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
>> then connects to ASMedia xHCI's root hub:
>>
>> /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
>> |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
>> |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
>>
>> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
>> Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
>> Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
>>
>> The port is disabled after resume:
>> xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
>>
>> According to xHCI 4.19.1.2.1, we should set link to RxDetect to transit
>> it from disabled state to disconnected state, which allows the port to
>> be set to U0 and completes the resume process.
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>>
>> P.S. This is and differnt (and should be more correct) approach to solve
>> https://lore.kernel.org/lkml/20191129174115.31683-1-kai.heng.feng@canonical.com/
>>
>> drivers/usb/core/hub.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 54cd8ef795ec..352e33c84d6a 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -3537,12 +3537,24 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>>
>> /* Skip the initial Clear-Suspend step for a remote wakeup */
>> status = hub_port_status(hub, port1, &portstatus, &portchange);
>> - if (status == 0 && !port_is_suspended(hub, portstatus)) {
>> + if (status == 0 && !port_is_suspended(hub, portstatus)
>> + && (portstatus & USB_PORT_STAT_ENABLE)) {
>
> This doesn't look right. We want to skip the step the clears the
> port's suspend feature whenever the feature is already clear,
> regardless of whether the port is enabled.
>
> Besides, isn't this new test racy? Suppose the port was enabled when
> we called hub_port_status above, but it got disabled in the nanoseconds
> since then?
>
>> if (portchange & USB_PORT_STAT_C_SUSPEND)
>> pm_wakeup_event(&udev->dev, 0);
>> goto SuspendCleared;
>> }
>>
>> + /* xHCI 4.19.1.2.1 */
>> + if (hub_is_superspeed(hub->hdev)) {
>> + if (!(portstatus & USB_PORT_STAT_ENABLE))
>> + status = hub_set_port_link_state(hub, port1,
>> + USB_SS_PORT_LS_RX_DETECT);
>> +
>> + if (status)
>> + dev_dbg(&port_dev->dev,
>> + "can't set to RxDetect, status %d\n", status);
>> + }
>> +
>
> So maybe this part belongs later in the routine. Or maybe it shouldn't
> be in the hub driver at all -- since it is xHCI-specific, shouldn't it
> go into the xhci-hcd driver?
Ok, I'll put this change into xhci instead. It should also avoid the race
you mentioned above.
Kai-Heng
>
> Alan Stern
>
>> /* see 7.1.7.7; affects power usage, but not budgeting */
>> if (hub_is_superspeed(hub->hdev))
>> status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U0);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-06 8:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-05 7:28 [PATCH] usb: core: Set port link to RxDetect if port is not enabled after resume Kai-Heng Feng
2020-03-05 19:02 ` Alan Stern
2020-03-06 8:38 ` Kai-Heng Feng
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).