| Message ID | 20231117130836.1.I77097aa9ec01aeca1b3c75fde4ba5007a17fdf76@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] r8152: Hold the rtnl_lock for all of reset | expand |
On Fri, Nov 17, 2023 at 1:10 PM Douglas Anderson <dianders@chromium.org> wrote: > > Previous commits added checks for RTL8152_INACCESSIBLE in the loops in > the driver. There are still a few more that keep tripping the driver > up in error cases and make things take longer than they should. Add > those in. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Grant Grundler <grundler@chromium.org> I've checked all the return paths and believe these changes don't break any of them. cheers, grant > --- > > drivers/net/usb/r8152.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index d6edf0254599..aca7dd7b4090 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -3000,6 +3000,8 @@ static void rtl8152_nic_reset(struct r8152 *tp) > ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CR, CR_RST); > > for (i = 0; i < 1000; i++) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > + return; > if (!(ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CR) & CR_RST)) > break; > usleep_range(100, 400); > @@ -3329,6 +3331,8 @@ static void rtl_disable(struct r8152 *tp) > rxdy_gated_en(tp, true); > > for (i = 0; i < 1000; i++) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > + return; > ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL); > if ((ocp_data & FIFO_EMPTY) == FIFO_EMPTY) > break; > @@ -3336,6 +3340,8 @@ static void rtl_disable(struct r8152 *tp) > } > > for (i = 0; i < 1000; i++) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > + return; > if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_TCR0) & TCR0_TX_EMPTY) > break; > usleep_range(1000, 2000); > @@ -5499,6 +5505,8 @@ static void wait_oob_link_list_ready(struct r8152 *tp) > int i; > > for (i = 0; i < 1000; i++) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > + return; > ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL); > if (ocp_data & LINK_LIST_READY) > break; > @@ -5513,6 +5521,8 @@ static void r8156b_wait_loading_flash(struct r8152 *tp) > int i; > > for (i = 0; i < 100; i++) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > + return; > if (ocp_read_word(tp, MCU_TYPE_USB, USB_GPHY_CTRL) & GPHY_PATCH_DONE) > break; > usleep_range(1000, 2000); > @@ -5635,6 +5645,8 @@ static int r8153_pre_firmware_1(struct r8152 *tp) > for (i = 0; i < 104; i++) { > u32 ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_WDT1_CTRL); > > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > + return -ENODEV; > if (!(ocp_data & WTD1_EN)) > break; > usleep_range(1000, 2000); > @@ -5791,6 +5803,8 @@ static void r8153_aldps_en(struct r8152 *tp, bool enable) > data &= ~EN_ALDPS; > ocp_reg_write(tp, OCP_POWER_CFG, data); > for (i = 0; i < 20; i++) { > + if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) > + return; > usleep_range(1000, 2000); > if (ocp_read_word(tp, MCU_TYPE_PLA, 0xe000) & 0x0100) > break; > -- > 2.43.0.rc0.421.g78406f8d94-goog >
On Fri, 2023-11-17 at 13:08 -0800, Douglas Anderson wrote: > Previous commits added checks for RTL8152_INACCESSIBLE in the loops in > the driver. There are still a few more that keep tripping the driver > up in error cases and make things take longer than they should. Add > those in. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> I think this deserves a 'Fixes' tag. Please add it. Additionally please insert the target tree in the subj prefix when re- postin (in this case 'net') You can retain the already collected reviewed-by tags. Thanks, Paolo
Hi, On Tue, Nov 21, 2023 at 2:25 AM Paolo Abeni <pabeni@redhat.com> wrote: > > On Fri, 2023-11-17 at 13:08 -0800, Douglas Anderson wrote: > > As of commit d9962b0d4202 ("r8152: Block future register access if > > register access fails") there is a race condition that can happen > > between the USB device reset thread and napi_enable() (not) getting > > called during rtl8152_open(). Specifically: > > * While rtl8152_open() is running we get a register access error > > that's _not_ -ENODEV and queue up a USB reset. > > * rtl8152_open() exits before calling napi_enable() due to any reason > > (including usb_submit_urb() returning an error). > > > > In that case: > > * Since the USB reset is perform in a separate thread asynchronously, > > it can run at anytime USB device lock is not held - even before > > rtl8152_open() has exited with an error and caused __dev_open() to > > clear the __LINK_STATE_START bit. > > * The rtl8152_pre_reset() will notice that the netif_running() returns > > true (since __LINK_STATE_START wasn't cleared) so it won't exit > > early. > > * rtl8152_pre_reset() will then hang in napi_disable() because > > napi_enable() was never called. > > > > We can fix the race by making sure that the r8152 reset routines don't > > run at the same time as we're opening the device. Specifically we need > > the reset routines in their entirety rely on the return value of > > netif_running(). The only way to reliably depend on that is for them > > to hold the rntl_lock() mutex for the duration of reset. > > Acquiring the rtnl_lock in a callback and releasing it in a different > one, with the latter called depending on the configuration, looks > fragile and possibly prone to deadlock issues. Yeah, I debated this as well. I looked through the USB code and I couldn't find any reason that it wouldn't work to hold the lock for the duration. I agree that it's a little more fragile in one sense, but I think it avoids potential races too and that makes it less fragile in a different sense. ;-) > Have you tested your patch with lockdep enabled? Yes, lockdep reported no problems with my patch. Indeed lockdep hints are how I ended up with the current solution. When I originally tried to lock the device in rtl8152_open() then lockdep yelled at me about the AB BA issues between the device lock and the rtnl_lock() mutex which made me realize that grabbing the rtnl_lock() in the reset code was the right solution here. > Can you instead acquire the rtnl lock only for pre_reset/post_rest and > in rtl8152_open() do something alike: > > for (i = 0; i < MAX_WAIT; ++i) { > if (usb_lock_device_for_reset(udev, NULL)) > goto error; > > wait_again = udev->reset_in_progress; > usb_unlock_device(udev); > if (!wait_again) > break; > > usleep(1); > } > if (i == MAX_WAIT) > goto error; > > which should be more polite to other locks? Right, I could add a call to usb_lock_device_for_reset() here. That shouldn't trigger AB BA lockdep splats since it has a timeout. I'm not 100% convinced that it's right, though. ...and I'm fairly certain that if we call it we don't want to call it in a loop. I don't think we should have a loop because usb_lock_device_for_reset() already has a loop in it and I don't think an extra loop will help. I'd imagine that usb_lock_device_for_reset() would usually timeout only if USB reset is currently running and somehow blocked. If pre_reset or post_reset are currently running then they've already got the USB lock (from their caller) and may be blocked waiting for the rtnl_lock. We've already got the rtnl_lock (from our caller) and now we're waiting for the USB lock. In neither case do I think it's a good idea to drop the locks that our caller grabbed for us, so about the best we can do in that case is return an error from r8152_open() after the first timeout. Let's step back and think about why we might want to get the USB lock in the first place. This would only be necessary if we dropped the lock between pre_reset and post_reset, right? ...so we're trying to make sure that we're not trying to open a device while the USB reset code is half executed. I guess the expected order of operations we're trying to protect against would be: 1. rtl8152_close() is called and has a transfer error that queues up a reset. 2. USB reset starts and pre-reset runs. It should be a no-op because netif_running() would return false. 3. rl8152_open() is called and opens the device successfully 4. USB reset runs post-reset, which is no longer the inverse of pre-reset because netif_running() would return true. This would end up with, among other things, an unbalanced napi_enable() count. That feels relatively unlikely to actually hit but it does seem conceivably possible. Thus if we do drop the rtnl_lock between pre-reset and post-reset then I agree we should call usb_lock_device_for_reset(). Probably we need to do that for _both_ rtl8152_open() and rtl8152_close()? We also probably don't need to hold the lock for the whole duration of rtl8152_open() / rtl8152_close(). We can just grab it and release it to make sure that we're not midway through a reset. I guess one sorta odd thing here is that it means that rtl8152_close() could now fail if someone called it at just the right time and we were unable to grab the USB lock. Though it does have an error return, that's not a failure that I'd expect most users to be able to handle terribly well. I guess conceivably we could return -EAGAIN or -EDEADLOCK in this case, but ick... Hopefully the above makes sense. I'd be interested to hear your further thoughts on the issue. I'd still lean towards leaving the code as-is and holding the rtnl_lock across the whole reset, but for all practical purposes I think it would be fine to split it and add usb_lock_device_for_reset() to the rtl8152_open() / rtl8152_close(), since the issues I talk about above seem like they'd need extremely rare timing conditions to hit. -Doug
Hi, On Tue, Nov 21, 2023 at 2:28 AM Paolo Abeni <pabeni@redhat.com> wrote: > > On Fri, 2023-11-17 at 13:08 -0800, Douglas Anderson wrote: > > Previous commits added checks for RTL8152_INACCESSIBLE in the loops in > > the driver. There are still a few more that keep tripping the driver > > up in error cases and make things take longer than they should. Add > > those in. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > I think this deserves a 'Fixes' tag. Please add it. Sure, I can add it. It didn't feel worth it to me since there's no real functional issue--just that it takes a little longer for these loops to exit out, but it shouldn't hurt. I guess that means breaking this commit into several depending on when the offending loop was added. > Additionally please insert the target tree in the subj prefix when re- > postin (in this case 'net') Funny, I just followed the tags for other commits to this file and the "net:" prefix isn't common. I guess this should be "net: usb: r8152". I can add it when I post v2.
On Tue, Nov 21, 2023 at 09:55:46AM -0800, Doug Anderson wrote: > Hi, > > On Tue, Nov 21, 2023 at 2:28 AM Paolo Abeni <pabeni@redhat.com> wrote: > > > > On Fri, 2023-11-17 at 13:08 -0800, Douglas Anderson wrote: > > > Previous commits added checks for RTL8152_INACCESSIBLE in the loops in > > > the driver. There are still a few more that keep tripping the driver > > > up in error cases and make things take longer than they should. Add > > > those in. > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > I think this deserves a 'Fixes' tag. Please add it. > > Sure, I can add it. It didn't feel worth it to me since there's no > real functional issue--just that it takes a little longer for these > loops to exit out, but it shouldn't hurt. I guess that means breaking > this commit into several depending on when the offending loop was > added. > > > > Additionally please insert the target tree in the subj prefix when re- > > postin (in this case 'net') > > Funny, I just followed the tags for other commits to this file and the > "net:" prefix isn't common. I guess this should be "net: usb: r8152". > I can add it when I post v2. Hi Doug, unfortunately prefix can have more than one meaning here. The target tree, often either net or net-next, should go in the [] part of the subject. In this case I think what you want is: [PATCH net n/m v2] Add RTL8152_INACCESSIBLE checks to more loops
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 2c5c1e91ded6..d6edf0254599 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -8397,6 +8397,8 @@ static int rtl8152_pre_reset(struct usb_interface *intf) struct r8152 *tp = usb_get_intfdata(intf); struct net_device *netdev; + rtnl_lock(); + if (!tp || !test_bit(PROBED_WITH_NO_ERRORS, &tp->flags)) return 0; @@ -8428,20 +8430,17 @@ static int rtl8152_post_reset(struct usb_interface *intf) struct sockaddr sa; if (!tp || !test_bit(PROBED_WITH_NO_ERRORS, &tp->flags)) - return 0; + goto exit; rtl_set_accessible(tp); /* reset the MAC address in case of policy change */ - if (determine_ethernet_addr(tp, &sa) >= 0) { - rtnl_lock(); + if (determine_ethernet_addr(tp, &sa) >= 0) dev_set_mac_address (tp->netdev, &sa, NULL); - rtnl_unlock(); - } netdev = tp->netdev; if (!netif_running(netdev)) - return 0; + goto exit; set_bit(WORK_ENABLE, &tp->flags); if (netif_carrier_ok(netdev)) { @@ -8460,6 +8459,8 @@ static int rtl8152_post_reset(struct usb_interface *intf) if (!list_empty(&tp->rx_done)) napi_schedule(&tp->napi); +exit: + rtnl_unlock(); return 0; }
As of commit d9962b0d4202 ("r8152: Block future register access if register access fails") there is a race condition that can happen between the USB device reset thread and napi_enable() (not) getting called during rtl8152_open(). Specifically: * While rtl8152_open() is running we get a register access error that's _not_ -ENODEV and queue up a USB reset. * rtl8152_open() exits before calling napi_enable() due to any reason (including usb_submit_urb() returning an error). In that case: * Since the USB reset is perform in a separate thread asynchronously, it can run at anytime USB device lock is not held - even before rtl8152_open() has exited with an error and caused __dev_open() to clear the __LINK_STATE_START bit. * The rtl8152_pre_reset() will notice that the netif_running() returns true (since __LINK_STATE_START wasn't cleared) so it won't exit early. * rtl8152_pre_reset() will then hang in napi_disable() because napi_enable() was never called. We can fix the race by making sure that the r8152 reset routines don't run at the same time as we're opening the device. Specifically we need the reset routines in their entirety rely on the return value of netif_running(). The only way to reliably depend on that is for them to hold the rntl_lock() mutex for the duration of reset. Grabbing the rntl_lock() mutex for the duration of reset seems like a long time, but reset is not expected to be common and the rtnl_lock() mutex is already held for long durations since the core grabs it around the open/close calls. Fixes: d9962b0d4202 ("r8152: Block future register access if register access fails") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/net/usb/r8152.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)