* r8151: check disconnect status after long sleep
@ 2020-02-24 7:15 You-Sheng Yang
2020-02-24 22:47 ` David Miller
2020-02-24 23:02 ` r8151: " Prashant Malani
0 siblings, 2 replies; 6+ messages in thread
From: You-Sheng Yang @ 2020-02-24 7:15 UTC (permalink / raw)
To: David S . Miller, Hayes Wang, Jakub Kicinski, Prashant Malani,
Kai-Heng Feng
Cc: Grant Grundler, You-Sheng Yang, linux-usb, netdev, linux-kernel
Dell USB Type C docking WD19/WD19DC attaches additional peripherals as:
/: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 5000M
|__ Port 1: Dev 11, If 0, Class=Hub, Driver=hub/4p, 5000M
|__ Port 3: Dev 12, If 0, Class=Hub, Driver=hub/4p, 5000M
|__ Port 4: Dev 13, If 0, Class=Vendor Specific Class,
Driver=r8152, 5000M
where usb 2-1-3 is a hub connecting all USB Type-A/C ports on the dock.
When hotplugging such dock with additional usb devices already attached on
it, the probing process may reset usb 2.1 port, therefore r8152 ethernet
device is also reset. However, during r8152 device init there are several
for-loops that, when it's unable to retrieve hardware registers due to
being discconected from USB, may take up to 14 seconds each in practice,
and that has to be completed before USB may re-enumerate devices on the
bus. As a result, devices attached to the dock will only be available
after nearly 1 minute after the dock was plugged in:
[ 216.388290] [250] r8152 2-1.4:1.0: usb_probe_interface
[ 216.388292] [250] r8152 2-1.4:1.0: usb_probe_interface - got id
[ 258.830410] r8152 2-1.4:1.0 (unnamed net_device) (uninitialized): PHY not ready
[ 258.830460] r8152 2-1.4:1.0 (unnamed net_device) (uninitialized): Invalid header when reading pass-thru MAC addr
[ 258.830464] r8152 2-1.4:1.0 (unnamed net_device) (uninitialized): Get ether addr fail
This can be reproduced on all kernel versions up to latest v5.6-rc2, but
after v5.5-rc7 the reproduce rate is dramatically lower to 1/30 or so
while it was around 1/2.
The time consuming for-loops are at:
https://elixir.bootlin.com/linux/v5.5/source/drivers/net/usb/r8152.c#L3206
https://elixir.bootlin.com/linux/v5.5/source/drivers/net/usb/r8152.c#L5400
https://elixir.bootlin.com/linux/v5.5/source/drivers/net/usb/r8152.c#L5537
Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com>
---
drivers/net/usb/r8152.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 78ddbaf6401b..95b19ce96513 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3221,6 +3221,8 @@ static u16 r8153_phy_status(struct r8152 *tp, u16 desired)
}
msleep(20);
+ if (test_bit(RTL8152_UNPLUG, &tp->flags))
+ break;
}
return data;
@@ -5402,7 +5404,10 @@ static void r8153_init(struct r8152 *tp)
if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
AUTOLOAD_DONE)
break;
+
msleep(20);
+ if (test_bit(RTL8152_UNPLUG, &tp->flags))
+ break;
}
data = r8153_phy_status(tp, 0);
@@ -5539,7 +5544,10 @@ static void r8153b_init(struct r8152 *tp)
if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
AUTOLOAD_DONE)
break;
+
msleep(20);
+ if (test_bit(RTL8152_UNPLUG, &tp->flags))
+ break;
}
data = r8153_phy_status(tp, 0);
--
2.25.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: r8151: check disconnect status after long sleep
2020-02-24 7:15 r8151: check disconnect status after long sleep You-Sheng Yang
@ 2020-02-24 22:47 ` David Miller
2020-02-26 15:37 ` [PATCH v2] r8152: " You-Sheng Yang
2020-02-24 23:02 ` r8151: " Prashant Malani
1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2020-02-24 22:47 UTC (permalink / raw)
To: vicamo.yang
Cc: hayeswang, kuba, pmalani, kai.heng.feng, grundler, vicamo,
linux-usb, netdev, linux-kernel
From: You-Sheng Yang <vicamo.yang@canonical.com>
Date: Mon, 24 Feb 2020 15:15:41 +0800
> When hotplugging such dock with additional usb devices already attached on
> it, the probing process may reset usb 2.1 port, therefore r8152 ethernet
> device is also reset. However, during r8152 device init there are several
> for-loops that, when it's unable to retrieve hardware registers due to
> being discconected from USB, may take up to 14 seconds each in practice,
> and that has to be completed before USB may re-enumerate devices on the
> bus. As a result, devices attached to the dock will only be available
> after nearly 1 minute after the dock was plugged in:
Your description of the problem and exactly what is happening is great,
but you are not explaining how exactly you are solving the problem and
why you are solving it in that way.
Please enhance your commit message the explain those things.
Thank you.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: r8151: check disconnect status after long sleep
2020-02-24 7:15 r8151: check disconnect status after long sleep You-Sheng Yang
2020-02-24 22:47 ` David Miller
@ 2020-02-24 23:02 ` Prashant Malani
2020-02-26 15:40 ` You-Sheng Yang
1 sibling, 1 reply; 6+ messages in thread
From: Prashant Malani @ 2020-02-24 23:02 UTC (permalink / raw)
To: You-Sheng Yang
Cc: David S . Miller, Hayes Wang, Jakub Kicinski, Kai-Heng Feng,
Grant Grundler, You-Sheng Yang, linux-usb, netdev, linux-kernel
nit: The commit message title tag should be "r8152", instead of "r8151".
On Mon, Feb 24, 2020 at 03:15:41PM +0800, You-Sheng Yang wrote:
> Dell USB Type C docking WD19/WD19DC attaches additional peripherals as:
>
> /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 5000M
> |__ Port 1: Dev 11, If 0, Class=Hub, Driver=hub/4p, 5000M
> |__ Port 3: Dev 12, If 0, Class=Hub, Driver=hub/4p, 5000M
> |__ Port 4: Dev 13, If 0, Class=Vendor Specific Class,
> Driver=r8152, 5000M
>
> where usb 2-1-3 is a hub connecting all USB Type-A/C ports on the dock.
>
> When hotplugging such dock with additional usb devices already attached on
> it, the probing process may reset usb 2.1 port, therefore r8152 ethernet
> device is also reset. However, during r8152 device init there are several
> for-loops that, when it's unable to retrieve hardware registers due to
> being discconected from USB, may take up to 14 seconds each in practice,
> and that has to be completed before USB may re-enumerate devices on the
> bus. As a result, devices attached to the dock will only be available
> after nearly 1 minute after the dock was plugged in:
>
> [ 216.388290] [250] r8152 2-1.4:1.0: usb_probe_interface
> [ 216.388292] [250] r8152 2-1.4:1.0: usb_probe_interface - got id
> [ 258.830410] r8152 2-1.4:1.0 (unnamed net_device) (uninitialized): PHY not ready
> [ 258.830460] r8152 2-1.4:1.0 (unnamed net_device) (uninitialized): Invalid header when reading pass-thru MAC addr
> [ 258.830464] r8152 2-1.4:1.0 (unnamed net_device) (uninitialized): Get ether addr fail
>
> This can be reproduced on all kernel versions up to latest v5.6-rc2, but
> after v5.5-rc7 the reproduce rate is dramatically lower to 1/30 or so
> while it was around 1/2.
>
> The time consuming for-loops are at:
> https://elixir.bootlin.com/linux/v5.5/source/drivers/net/usb/r8152.c#L3206
> https://elixir.bootlin.com/linux/v5.5/source/drivers/net/usb/r8152.c#L5400
> https://elixir.bootlin.com/linux/v5.5/source/drivers/net/usb/r8152.c#L5537
>
> Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com>
> ---
> drivers/net/usb/r8152.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 78ddbaf6401b..95b19ce96513 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -3221,6 +3221,8 @@ static u16 r8153_phy_status(struct r8152 *tp, u16 desired)
> }
>
> msleep(20);
> + if (test_bit(RTL8152_UNPLUG, &tp->flags))
> + break;
> }
>
> return data;
> @@ -5402,7 +5404,10 @@ static void r8153_init(struct r8152 *tp)
> if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
> AUTOLOAD_DONE)
> break;
> +
> msleep(20);
> + if (test_bit(RTL8152_UNPLUG, &tp->flags))
> + break;
> }
>
> data = r8153_phy_status(tp, 0);
> @@ -5539,7 +5544,10 @@ static void r8153b_init(struct r8152 *tp)
> if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
> AUTOLOAD_DONE)
> break;
> +
> msleep(20);
> + if (test_bit(RTL8152_UNPLUG, &tp->flags))
> + break;
> }
>
> data = r8153_phy_status(tp, 0);
> --
> 2.25.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] r8152: check disconnect status after long sleep
2020-02-24 22:47 ` David Miller
@ 2020-02-26 15:37 ` You-Sheng Yang
2020-03-01 5:20 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: You-Sheng Yang @ 2020-02-26 15:37 UTC (permalink / raw)
To: davem
Cc: grundler, hayeswang, kai.heng.feng, kuba, linux-kernel,
linux-usb, netdev, pmalani, vicamo.yang, vicamo
From: You-Sheng Yang <vicamo.yang@canonical.com>
Dell USB Type C docking WD19/WD19DC attaches additional peripherals as:
/: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 5000M
|__ Port 1: Dev 11, If 0, Class=Hub, Driver=hub/4p, 5000M
|__ Port 3: Dev 12, If 0, Class=Hub, Driver=hub/4p, 5000M
|__ Port 4: Dev 13, If 0, Class=Vendor Specific Class,
Driver=r8152, 5000M
where usb 2-1-3 is a hub connecting all USB Type-A/C ports on the dock.
When hotplugging such dock with additional usb devices already attached on
it, the probing process may reset usb 2.1 port, therefore r8152 ethernet
device is also reset. However, during r8152 device init there are several
for-loops that, when it's unable to retrieve hardware registers due to
being disconnected from USB, may take up to 14 seconds each in practice,
and that has to be completed before USB may re-enumerate devices on the
bus. As a result, devices attached to the dock will only be available
after nearly 1 minute after the dock was plugged in:
[ 216.388290] [250] r8152 2-1.4:1.0: usb_probe_interface
[ 216.388292] [250] r8152 2-1.4:1.0: usb_probe_interface - got id
[ 258.830410] r8152 2-1.4:1.0 (unnamed net_device) (uninitialized): PHY not ready
[ 258.830460] r8152 2-1.4:1.0 (unnamed net_device) (uninitialized): Invalid header when reading pass-thru MAC addr
[ 258.830464] r8152 2-1.4:1.0 (unnamed net_device) (uninitialized): Get ether addr fail
This happens in, for example, r8153_init:
static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
void *data, u16 type)
{
if (test_bit(RTL8152_UNPLUG, &tp->flags))
return -ENODEV;
...
}
static u16 ocp_read_word(struct r8152 *tp, u16 type, u16 index)
{
u32 data;
...
generic_ocp_read(tp, index, sizeof(tmp), &tmp, type | byen);
data = __le32_to_cpu(tmp);
...
return (u16)data;
}
static void r8153_init(struct r8152 *tp)
{
...
if (test_bit(RTL8152_UNPLUG, &tp->flags))
return;
for (i = 0; i < 500; i++) {
if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
AUTOLOAD_DONE)
break;
msleep(20);
}
...
}
Since ocp_read_word() doesn't check the return status of
generic_ocp_read(), and the only exit condition for the loop is to have
a match in the returned value, such loops will only ends after exceeding
its maximum runs when the device has been marked as disconnected, which
takes 500 * 20ms = 10 seconds in theory, 14 in practice.
To solve this long latency another test to RTL8152_UNPLUG flag should be
added after those 20ms sleep to skip unnecessary loops, so that the device
probe can complete early and proceed to parent port reset/reprobe process.
This can be reproduced on all kernel versions up to latest v5.6-rc2, but
after v5.5-rc7 the reproduce rate is dramatically lowered to 1/30 or less
while it was around 1/2.
Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com>
---
Changes since v1:
- update patch title, commit messages.
---
drivers/net/usb/r8152.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 78ddbaf6401b..95b19ce96513 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3221,6 +3221,8 @@ static u16 r8153_phy_status(struct r8152 *tp, u16 desired)
}
msleep(20);
+ if (test_bit(RTL8152_UNPLUG, &tp->flags))
+ break;
}
return data;
@@ -5402,7 +5404,10 @@ static void r8153_init(struct r8152 *tp)
if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
AUTOLOAD_DONE)
break;
+
msleep(20);
+ if (test_bit(RTL8152_UNPLUG, &tp->flags))
+ break;
}
data = r8153_phy_status(tp, 0);
@@ -5539,7 +5544,10 @@ static void r8153b_init(struct r8152 *tp)
if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
AUTOLOAD_DONE)
break;
+
msleep(20);
+ if (test_bit(RTL8152_UNPLUG, &tp->flags))
+ break;
}
data = r8153_phy_status(tp, 0);
--
2.25.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: r8151: check disconnect status after long sleep
2020-02-24 23:02 ` r8151: " Prashant Malani
@ 2020-02-26 15:40 ` You-Sheng Yang
0 siblings, 0 replies; 6+ messages in thread
From: You-Sheng Yang @ 2020-02-26 15:40 UTC (permalink / raw)
To: Prashant Malani
Cc: David S . Miller, Hayes Wang, Jakub Kicinski, Kai-Heng Feng,
Grant Grundler, You-Sheng Yang, linux-usb, netdev, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 189 bytes --]
On 2020-02-25 07:02, Prashant Malani wrote:
> nit: The commit message title tag should be "r8152", instead of "r8151".
Thank you for reviewing. Also updated in v2.
You-Sheng Yang
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] r8152: check disconnect status after long sleep
2020-02-26 15:37 ` [PATCH v2] r8152: " You-Sheng Yang
@ 2020-03-01 5:20 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2020-03-01 5:20 UTC (permalink / raw)
To: vicamo
Cc: grundler, hayeswang, kai.heng.feng, kuba, linux-kernel,
linux-usb, netdev, pmalani, vicamo.yang
From: You-Sheng Yang <vicamo@gmail.com>
Date: Wed, 26 Feb 2020 23:37:10 +0800
> From: You-Sheng Yang <vicamo.yang@canonical.com>
>
> Dell USB Type C docking WD19/WD19DC attaches additional peripherals as:
>
> /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 5000M
> |__ Port 1: Dev 11, If 0, Class=Hub, Driver=hub/4p, 5000M
> |__ Port 3: Dev 12, If 0, Class=Hub, Driver=hub/4p, 5000M
> |__ Port 4: Dev 13, If 0, Class=Vendor Specific Class,
> Driver=r8152, 5000M
>
> where usb 2-1-3 is a hub connecting all USB Type-A/C ports on the dock.
>
> When hotplugging such dock with additional usb devices already attached on
> it, the probing process may reset usb 2.1 port, therefore r8152 ethernet
> device is also reset. However, during r8152 device init there are several
> for-loops that, when it's unable to retrieve hardware registers due to
> being disconnected from USB, may take up to 14 seconds each in practice,
> and that has to be completed before USB may re-enumerate devices on the
> bus. As a result, devices attached to the dock will only be available
> after nearly 1 minute after the dock was plugged in:
...
> To solve this long latency another test to RTL8152_UNPLUG flag should be
> added after those 20ms sleep to skip unnecessary loops, so that the device
> probe can complete early and proceed to parent port reset/reprobe process.
>
> This can be reproduced on all kernel versions up to latest v5.6-rc2, but
> after v5.5-rc7 the reproduce rate is dramatically lowered to 1/30 or less
> while it was around 1/2.
>
> Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com>
Applied, thank you.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-01 5:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-24 7:15 r8151: check disconnect status after long sleep You-Sheng Yang
2020-02-24 22:47 ` David Miller
2020-02-26 15:37 ` [PATCH v2] r8152: " You-Sheng Yang
2020-03-01 5:20 ` David Miller
2020-02-24 23:02 ` r8151: " Prashant Malani
2020-02-26 15:40 ` You-Sheng Yang
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).