* 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
* [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: [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
* 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
* 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
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).