| Message ID | 20231020210751.3415723-1-dianders@chromium.org |
|---|---|
| Headers | show |
| Series | r8152: Avoid writing garbage to the adapter's registers | expand |
On Fri, Oct 20, 2023 at 2:08 PM Douglas Anderson <dianders@chromium.org> wrote: > > According to the comment next to USB_CTRL_GET_TIMEOUT and > USB_CTRL_SET_TIMEOUT, although sending/receiving control messages is > usually quite fast, the spec allows them to take up to 5 seconds. > Let's increase the timeout in the Realtek driver from 500ms to 5000ms > (using the #defines) to account for this. > > This is not just a theoretical change. The need for the longer timeout > was seen in testing. Specifically, if you drop a sc7180-trogdor based > Chromebook into the kdb debugger and then "go" again after sitting in > the debugger for a while, the next USB control message takes a long > time. Out of ~40 tests the slowest USB control message was 4.5 > seconds. > > While dropping into kdb is not exactly an end-user scenario, the above > is similar to what could happen due to an temporary interrupt storm, > what could happen if there was a host controller (HW or SW) issue, or > what could happen if the Realtek device got into a confused state and > needed time to recover. > > This change is fairly critical since the r8152 driver in Linux doesn't > expect register reads/writes (which are backed by USB control > messages) to fail. > > Fixes: ac718b69301c ("net/usb: new driver for RTL8152") > Suggested-by: Hayes Wang <hayeswang@realtek.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Grant Grundler <grundler@chromium.org> > --- > > (no changes since v1) > > drivers/net/usb/r8152.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index 0c13d9950cd8..482957beae66 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -1212,7 +1212,7 @@ int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) > > ret = usb_control_msg(tp->udev, tp->pipe_ctrl_in, > RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, > - value, index, tmp, size, 500); > + value, index, tmp, size, USB_CTRL_GET_TIMEOUT); > if (ret < 0) > memset(data, 0xff, size); > else > @@ -1235,7 +1235,7 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) > > ret = usb_control_msg(tp->udev, tp->pipe_ctrl_out, > RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE, > - value, index, tmp, size, 500); > + value, index, tmp, size, USB_CTRL_SET_TIMEOUT); > > kfree(tmp); > > @@ -9494,7 +9494,8 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev) > > ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), > RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, > - PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500); > + PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), > + USB_CTRL_GET_TIMEOUT); > if (ret > 0) > ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK; > > -- > 2.42.0.758.gaed0368e0e-goog >
On Fri, Oct 20, 2023 at 2:08 PM Douglas Anderson <dianders@chromium.org> wrote: > > The error handling in rtl8152_probe() is missing a call to cancel the > hw_phy_work. Add it in to match what's in the cleanup code in > rtl8152_disconnect(). Sounds like there is a future opportunity for someone (not Doug) to refactor code. > Fixes: a028a9e003f2 ("r8152: move the settings of PHY to a work queue") > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Grant Grundler <grundler@chromium.org> > --- > > Changes in v5: > - ("Cancel hw_phy_work if we have an error in probe") new for v5. > > drivers/net/usb/r8152.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index 201c688e3e3f..d10b0886b652 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -9783,6 +9783,7 @@ static int rtl8152_probe(struct usb_interface *intf, > > out1: > tasklet_kill(&tp->tx_tl); > + cancel_delayed_work_sync(&tp->hw_phy_work); > if (tp->rtl_ops.unload) > tp->rtl_ops.unload(tp); > usb_set_intfdata(intf, NULL); > -- > 2.42.0.758.gaed0368e0e-goog >
On Fri, Oct 20, 2023 at 2:08 PM Douglas Anderson <dianders@chromium.org> wrote: > > If the adapter is unplugged while we're looping in > rtl_phy_patch_request() we could end up looping for 10 seconds (2 ms * > 5000 loops). Add code similar to what's done in other places in the > driver to check for unplug and bail. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Grant Grundler <grundler@chromium.org> > --- > > (no changes since v2) > > Changes in v2: > - ("Check for unplug in rtl_phy_patch_request()") new for v2. > > drivers/net/usb/r8152.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index 656fe90734fc..9888bc43e903 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -4046,6 +4046,9 @@ static int rtl_phy_patch_request(struct r8152 *tp, bool request, bool wait) > for (i = 0; wait && i < 5000; i++) { > u32 ocp_data; > > + if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + break; > + > usleep_range(1000, 2000); > ocp_data = ocp_reg_read(tp, OCP_PHY_PATCH_STAT); > if ((ocp_data & PATCH_READY) ^ check) > -- > 2.42.0.758.gaed0368e0e-goog >
On Fri, Oct 20, 2023 at 2:08 PM Douglas Anderson <dianders@chromium.org> wrote: > > Whenever the RTL8152_UNPLUG is set that just tells the driver that all > accesses will fail and we should just immediately bail. A future patch > will use this same concept at a time when the driver hasn't actually > been unplugged but is about to be reset. Rename the flag in > preparation for the future patch. > > This is a no-op change and just a search and replace. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Grant Grundler <grundler@chromium.org> > --- > > (no changes since v2) > > Changes in v2: > - ("Rename RTL8152_UNPLUG to RTL8152_INACCESSIBLE") new for v2. > > drivers/net/usb/r8152.c | 96 ++++++++++++++++++++--------------------- > 1 file changed, 48 insertions(+), 48 deletions(-) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index 982f9ca03e7a..65232848b31d 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -764,7 +764,7 @@ enum rtl_register_content { > > /* rtl8152 flags */ > enum rtl8152_flags { > - RTL8152_UNPLUG = 0, > + RTL8152_INACCESSIBLE = 0, > RTL8152_SET_RX_MODE, > WORK_ENABLE, > RTL8152_LINK_CHG, > @@ -1245,7 +1245,7 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) > static void rtl_set_unplug(struct r8152 *tp) > { > if (tp->udev->state == USB_STATE_NOTATTACHED) { > - set_bit(RTL8152_UNPLUG, &tp->flags); > + set_bit(RTL8152_INACCESSIBLE, &tp->flags); > smp_mb__after_atomic(); > } > } > @@ -1256,7 +1256,7 @@ static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size, > u16 limit = 64; > int ret = 0; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return -ENODEV; > > /* both size and indix must be 4 bytes align */ > @@ -1300,7 +1300,7 @@ static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen, > u16 byteen_start, byteen_end, byen; > u16 limit = 512; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return -ENODEV; > > /* both size and indix must be 4 bytes align */ > @@ -1537,7 +1537,7 @@ static int read_mii_word(struct net_device *netdev, int phy_id, int reg) > struct r8152 *tp = netdev_priv(netdev); > int ret; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return -ENODEV; > > if (phy_id != R8152_PHY_ID) > @@ -1553,7 +1553,7 @@ void write_mii_word(struct net_device *netdev, int phy_id, int reg, int val) > { > struct r8152 *tp = netdev_priv(netdev); > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > if (phy_id != R8152_PHY_ID) > @@ -1758,7 +1758,7 @@ static void read_bulk_callback(struct urb *urb) > if (!tp) > return; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > if (!test_bit(WORK_ENABLE, &tp->flags)) > @@ -1850,7 +1850,7 @@ static void write_bulk_callback(struct urb *urb) > if (!test_bit(WORK_ENABLE, &tp->flags)) > return; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > if (!skb_queue_empty(&tp->tx_queue)) > @@ -1871,7 +1871,7 @@ static void intr_callback(struct urb *urb) > if (!test_bit(WORK_ENABLE, &tp->flags)) > return; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > switch (status) { > @@ -2615,7 +2615,7 @@ static void bottom_half(struct tasklet_struct *t) > { > struct r8152 *tp = from_tasklet(tp, t, tx_tl); > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > if (!test_bit(WORK_ENABLE, &tp->flags)) > @@ -2658,7 +2658,7 @@ int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags) > int ret; > > /* The rx would be stopped, so skip submitting */ > - if (test_bit(RTL8152_UNPLUG, &tp->flags) || > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags) || > !test_bit(WORK_ENABLE, &tp->flags) || !netif_carrier_ok(tp->netdev)) > return 0; > > @@ -3058,7 +3058,7 @@ static int rtl_enable(struct r8152 *tp) > > static int rtl8152_enable(struct r8152 *tp) > { > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return -ENODEV; > > set_tx_qlen(tp); > @@ -3145,7 +3145,7 @@ static int rtl8153_enable(struct r8152 *tp) > { > u32 ocp_data; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return -ENODEV; > > set_tx_qlen(tp); > @@ -3177,7 +3177,7 @@ static void rtl_disable(struct r8152 *tp) > u32 ocp_data; > int i; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) { > rtl_drop_queued_tx(tp); > return; > } > @@ -3631,7 +3631,7 @@ static u16 r8153_phy_status(struct r8152 *tp, u16 desired) > } > > msleep(20); > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > break; > } > > @@ -3663,7 +3663,7 @@ static void r8153b_ups_en(struct r8152 *tp, bool enable) > int i; > > for (i = 0; i < 500; i++) { > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) & > AUTOLOAD_DONE) > @@ -3705,7 +3705,7 @@ static void r8153c_ups_en(struct r8152 *tp, bool enable) > int i; > > for (i = 0; i < 500; i++) { > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) & > AUTOLOAD_DONE) > @@ -4050,8 +4050,8 @@ static int rtl_phy_patch_request(struct r8152 *tp, bool request, bool wait) > for (i = 0; wait && i < 5000; i++) { > u32 ocp_data; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > - break; > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > + return -ENODEV; > > usleep_range(1000, 2000); > ocp_data = ocp_reg_read(tp, OCP_PHY_PATCH_STAT); > @@ -6009,7 +6009,7 @@ static int rtl8156_enable(struct r8152 *tp) > u32 ocp_data; > u16 speed; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return -ENODEV; > > r8156_fc_parameter(tp); > @@ -6067,7 +6067,7 @@ static int rtl8156b_enable(struct r8152 *tp) > u32 ocp_data; > u16 speed; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return -ENODEV; > > set_tx_qlen(tp); > @@ -6253,7 +6253,7 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex, > > static void rtl8152_up(struct r8152 *tp) > { > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > r8152_aldps_en(tp, false); > @@ -6263,7 +6263,7 @@ static void rtl8152_up(struct r8152 *tp) > > static void rtl8152_down(struct r8152 *tp) > { > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) { > rtl_drop_queued_tx(tp); > return; > } > @@ -6278,7 +6278,7 @@ static void rtl8153_up(struct r8152 *tp) > { > u32 ocp_data; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > r8153_u1u2en(tp, false); > @@ -6318,7 +6318,7 @@ static void rtl8153_down(struct r8152 *tp) > { > u32 ocp_data; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) { > rtl_drop_queued_tx(tp); > return; > } > @@ -6339,7 +6339,7 @@ static void rtl8153b_up(struct r8152 *tp) > { > u32 ocp_data; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > r8153b_u1u2en(tp, false); > @@ -6363,7 +6363,7 @@ static void rtl8153b_down(struct r8152 *tp) > { > u32 ocp_data; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) { > rtl_drop_queued_tx(tp); > return; > } > @@ -6400,7 +6400,7 @@ static void rtl8153c_up(struct r8152 *tp) > { > u32 ocp_data; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > r8153b_u1u2en(tp, false); > @@ -6481,7 +6481,7 @@ static void rtl8156_up(struct r8152 *tp) > { > u32 ocp_data; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > r8153b_u1u2en(tp, false); > @@ -6554,7 +6554,7 @@ static void rtl8156_down(struct r8152 *tp) > { > u32 ocp_data; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) { > rtl_drop_queued_tx(tp); > return; > } > @@ -6692,7 +6692,7 @@ static void rtl_work_func_t(struct work_struct *work) > /* If the device is unplugged or !netif_running(), the workqueue > * doesn't need to wake the device, and could return directly. > */ > - if (test_bit(RTL8152_UNPLUG, &tp->flags) || !netif_running(tp->netdev)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags) || !netif_running(tp->netdev)) > return; > > if (usb_autopm_get_interface(tp->intf) < 0) > @@ -6731,7 +6731,7 @@ static void rtl_hw_phy_work_func_t(struct work_struct *work) > { > struct r8152 *tp = container_of(work, struct r8152, hw_phy_work.work); > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > if (usb_autopm_get_interface(tp->intf) < 0) > @@ -6858,7 +6858,7 @@ static int rtl8152_close(struct net_device *netdev) > netif_stop_queue(netdev); > > res = usb_autopm_get_interface(tp->intf); > - if (res < 0 || test_bit(RTL8152_UNPLUG, &tp->flags)) { > + if (res < 0 || test_bit(RTL8152_INACCESSIBLE, &tp->flags)) { > rtl_drop_queued_tx(tp); > rtl_stop_rx(tp); > } else { > @@ -6891,7 +6891,7 @@ static void r8152b_init(struct r8152 *tp) > u32 ocp_data; > u16 data; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > data = r8152_mdio_read(tp, MII_BMCR); > @@ -6935,7 +6935,7 @@ static void r8153_init(struct r8152 *tp) > u16 data; > int i; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > r8153_u1u2en(tp, false); > @@ -6946,7 +6946,7 @@ static void r8153_init(struct r8152 *tp) > break; > > msleep(20); > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > break; > } > > @@ -7075,7 +7075,7 @@ static void r8153b_init(struct r8152 *tp) > u16 data; > int i; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > r8153b_u1u2en(tp, false); > @@ -7086,7 +7086,7 @@ static void r8153b_init(struct r8152 *tp) > break; > > msleep(20); > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > break; > } > > @@ -7157,7 +7157,7 @@ static void r8153c_init(struct r8152 *tp) > u16 data; > int i; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > r8153b_u1u2en(tp, false); > @@ -7177,7 +7177,7 @@ static void r8153c_init(struct r8152 *tp) > break; > > msleep(20); > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > } > > @@ -8006,7 +8006,7 @@ static void r8156_init(struct r8152 *tp) > u16 data; > int i; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_ECM_OP); > @@ -8027,7 +8027,7 @@ static void r8156_init(struct r8152 *tp) > break; > > msleep(20); > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > } > > @@ -8102,7 +8102,7 @@ static void r8156b_init(struct r8152 *tp) > u16 data; > int i; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_ECM_OP); > @@ -8136,7 +8136,7 @@ static void r8156b_init(struct r8152 *tp) > break; > > msleep(20); > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > } > > @@ -9165,7 +9165,7 @@ static int rtl8152_ioctl(struct net_device *netdev, struct ifreq *rq, int cmd) > struct mii_ioctl_data *data = if_mii(rq); > int res; > > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return -ENODEV; > > res = usb_autopm_get_interface(tp->intf); > @@ -9267,7 +9267,7 @@ static const struct net_device_ops rtl8152_netdev_ops = { > > static void rtl8152_unload(struct r8152 *tp) > { > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > if (tp->version != RTL_VER_01) > @@ -9276,7 +9276,7 @@ static void rtl8152_unload(struct r8152 *tp) > > static void rtl8153_unload(struct r8152 *tp) > { > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > r8153_power_cut_en(tp, false); > @@ -9284,7 +9284,7 @@ static void rtl8153_unload(struct r8152 *tp) > > static void rtl8153b_unload(struct r8152 *tp) > { > - if (test_bit(RTL8152_UNPLUG, &tp->flags)) > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > return; > > r8153b_power_cut_en(tp, false); > -- > 2.42.0.758.gaed0368e0e-goog >
Hello: This series was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Fri, 20 Oct 2023 14:06:51 -0700 you wrote: > This series is the result of a cooperative debug effort between > Realtek and the ChromeOS team. On ChromeOS, we've noticed that Realtek > Ethernet adapters can sometimes get so wedged that even a reboot of > the host can't get them to enumerate again, assuming that the adapter > was on a powered hub and din't lose power when the host rebooted. This > is sometimes seen in the ChromeOS automated testing lab. The only way > to recover adapters in this state is to manually power cycle them. > > [...] Here is the summary with links: - [v5,1/8] r8152: Increase USB control msg timeout to 5000ms as per spec https://git.kernel.org/netdev/net/c/a5feba71ec9c - [v5,2/8] r8152: Run the unload routine if we have errors during probe https://git.kernel.org/netdev/net/c/5dd176895269 - [v5,3/8] r8152: Cancel hw_phy_work if we have an error in probe https://git.kernel.org/netdev/net/c/bb8adff9123e - [v5,4/8] r8152: Release firmware if we have an error in probe https://git.kernel.org/netdev/net/c/b8d35024d405 - [v5,5/8] r8152: Check for unplug in rtl_phy_patch_request() https://git.kernel.org/netdev/net/c/dc90ba37a8c3 - [v5,6/8] r8152: Check for unplug in r8153b_ups_en() / r8153c_ups_en() https://git.kernel.org/netdev/net/c/bc65cc42af73 - [v5,7/8] r8152: Rename RTL8152_UNPLUG to RTL8152_INACCESSIBLE https://git.kernel.org/netdev/net/c/715f67f33af4 - [v5,8/8] r8152: Block future register access if register access fails https://git.kernel.org/netdev/net/c/d9962b0d4202 You are awesome, thank you!
On 10/22/2023 3:50 AM, patchwork-bot+netdevbpf@kernel.org wrote: > Hello: > > This series was applied to netdev/net.git (main) > by David S. Miller <davem@davemloft.net>: > > On Fri, 20 Oct 2023 14:06:51 -0700 you wrote: >> This series is the result of a cooperative debug effort between >> Realtek and the ChromeOS team. On ChromeOS, we've noticed that Realtek >> Ethernet adapters can sometimes get so wedged that even a reboot of >> the host can't get them to enumerate again, assuming that the adapter >> was on a powered hub and din't lose power when the host rebooted. This >> is sometimes seen in the ChromeOS automated testing lab. The only way >> to recover adapters in this state is to manually power cycle them. >> >> [...] Oh well, late to the party, but this looks great, thanks!
This series is the result of a cooperative debug effort between Realtek and the ChromeOS team. On ChromeOS, we've noticed that Realtek Ethernet adapters can sometimes get so wedged that even a reboot of the host can't get them to enumerate again, assuming that the adapter was on a powered hub and din't lose power when the host rebooted. This is sometimes seen in the ChromeOS automated testing lab. The only way to recover adapters in this state is to manually power cycle them. I managed to reproduce one instance of this wedging (unknown if this is truly related to what the test lab sees) by doing this: 1. Start a flood ping from a host to the device. 2. Drop the device into kdb. 3. Wait 90 seconds. 4. Resume from kdb (the "g" command). 5. Wait another 45 seconds. Upon analysis, Realtek realized this was happening: 1. The Linux driver was getting a "Tx timeout" after resuming from kdb and then trying to reset itself. 2. As part of the reset, the Linux driver was attempting to do a read-modify-write of the adapter's registers. 3. The read would fail (due to a timeout) and the driver pretended that the register contained all 0xFFs. See commit f53a7ad18959 ("r8152: Set memory to all 0xFFs on failed reg reads") 4. The driver would take this value of all 0xFFs, modify it, and attempt to write it back to the adapter. 5. By this time the USB channel seemed to recover and thus we'd successfully write a value that was mostly 0xFFs to the adpater. 6. The adapter didn't like this and would wedge itself. Another Engineer also managed to reproduce wedging of the Realtek Ethernet adpater during a reboot test on an AMD Chromebook. In that case he was sometimes seeing -EPIPE returned from the control transfers. This patch series fixes both issues. Changes in v5: - ("Run the unload routine if we have errors during probe") new for v5. - ("Cancel hw_phy_work if we have an error in probe") new for v5. - ("Release firmware if we have an error in probe") new for v5. - Removed extra mutex_unlock() left over in v4. - Fixed minor typos. - Don't do queue an unbind/bind reset if probe fails; just retry probe. Changes in v4: - Took out some unnecessary locks/unlocks of the control mutex. - Added comment about reading version causing probe fail if 3 fails. - Added text to commit msg about the potential unbind/bind loop. Changes in v3: - Fixed v2 changelog ending up in the commit message. - farmework -> framework in comments. Changes in v2: - ("Check for unplug in rtl_phy_patch_request()") new for v2. - ("Check for unplug in r8153b_ups_en() / r8153c_ups_en()") new for v2. - ("Rename RTL8152_UNPLUG to RTL8152_INACCESSIBLE") new for v2. - Reset patch no longer based on retry patch, since that was dropped. - Reset patch should be robust even if failures happen in probe. - Switched booleans to bits in the "flags" variable. - Check for -ENODEV instead of "udev->state == USB_STATE_NOTATTACHED" Douglas Anderson (8): r8152: Increase USB control msg timeout to 5000ms as per spec r8152: Run the unload routine if we have errors during probe r8152: Cancel hw_phy_work if we have an error in probe r8152: Release firmware if we have an error in probe r8152: Check for unplug in rtl_phy_patch_request() r8152: Check for unplug in r8153b_ups_en() / r8153c_ups_en() r8152: Rename RTL8152_UNPLUG to RTL8152_INACCESSIBLE r8152: Block future register access if register access fails drivers/net/usb/r8152.c | 303 ++++++++++++++++++++++++++++++---------- 1 file changed, 230 insertions(+), 73 deletions(-)