diff mbox series

[net,v2,2/5] r8152: Add RTL8152_INACCESSIBLE checks to more loops

Message ID 20231128133811.net.v2.2.I79c8a6c8cafd89979af5407d77a6eda589833dca@changeid
State New
Headers
Series [net,v2,1/5] r8152: Hold the rtnl_lock for all of reset |

Commit Message

Doug Anderson Nov. 28, 2023, 9:38 p.m. UTC 
  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(+)

Comments

Hayes Wang Nov. 29, 2023, 12:47 p.m. UTC | #1 
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
Doug Anderson Nov. 29, 2023, 9:26 p.m. UTC | #2 
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 mbox series

Patch

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;