| Message ID | 20231128133811.net.v2.2.I79c8a6c8cafd89979af5407d77a6eda589833dca@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2,1/5] r8152: Hold the rtnl_lock for all of reset | expand |
Douglas Anderson <dianders@chromium.org> > Sent: Wednesday, November 29, 2023 5:38 AM [...] > @@ -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; I think you have to replace return with break. Otherwise, rtl_stop_rx() wouldn't be called. And, you may free the memory which is using, when rtl8152_close () is called. > 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; Same as above. > 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; > -- Best Regards, Hayes
Hi, On Wed, Nov 29, 2023 at 4:47 AM Hayes Wang <hayeswang@realtek.com> wrote: > > Douglas Anderson <dianders@chromium.org> > > Sent: Wednesday, November 29, 2023 5:38 AM > [...] > > @@ -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; > > I think you have to replace return with break. > Otherwise, rtl_stop_rx() wouldn't be called. > And, you may free the memory which is using, when rtl8152_close () is called. > > > 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; > > Same as above. Good catch, thanks! I've changed all of the `return` statements in this patch to `break` for consistency, though for the other ones it doesn't really matter. For patch #3 I also changed the `return` to a `break`, but for patches #4 and #5 the return was better so I left those. v3 posted now with this fix. -Doug
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index d6edf0254599..5a9c5b790508 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;
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. All the loops that are part of this commit existed in some form or another since the r8152 driver was first introduced, though RTL8152_INACCESSIBLE was known as RTL8152_UNPLUG before commit 715f67f33af4 ("r8152: Rename RTL8152_UNPLUG to RTL8152_INACCESSIBLE") Fixes: ac718b69301c ("net/usb: new driver for RTL8152") Reviewed-by: Grant Grundler <grundler@chromium.org> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: - Added Fixes tag to RTL8152_INACCESSIBLE patches. - Split RTL8152_INACCESSIBLE patches by the commit the loop came from. drivers/net/usb/r8152.c | 8 ++++++++ 1 file changed, 8 insertions(+)