[PATCH net-next 0/2] r8152: adjust r8152_submit_rx

Avoid r8152_submit_rx() from submitting rx during unexpected
moment. This could reduce the time of stopping rx.

For patch #1, the tp->speed should be updated early. Then,
the patch #2 could use it to check the current linking status.

Hayes Wang (2):
  r8152: adjust set_carrier
  r8152: check the status before submitting rx

 drivers/net/usb/r8152.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

-- 
2.1.0

Don't submit the rx if the device is unplugged, linking down,
or stopped.

Signed-off-by: Hayes Wang <redacted>
---
 drivers/net/usb/r8152.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 59b70c5..b39b2e4 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -1789,6 +1789,11 @@ 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) ||
+	    !test_bit(WORK_ENABLE, &tp->flags) || !(tp->speed & LINK_STATUS))
+		return 0;
+
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
 			  agg->head, agg_buf_sz,
 			  (usb_complete_t)read_bulk_callback, agg);

-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Update the tp->speed at the beginning of the function. Then,
the other fucntion could use it for checking linking status.

Signed-off-by: Hayes Wang <redacted>
---
 drivers/net/usb/r8152.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 2d1c77e..59b70c5 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -2848,26 +2848,25 @@ static void rtl8153_down(struct r8152 *tp)
 static void set_carrier(struct r8152 *tp)
 {
 	struct net_device *netdev = tp->netdev;
-	u8 speed;
+	u8 old_speed = tp->speed;
 
 	clear_bit(RTL8152_LINK_CHG, &tp->flags);
-	speed = rtl8152_get_speed(tp);
+	tp->speed = rtl8152_get_speed(tp);
 
-	if (speed & LINK_STATUS) {
-		if (!(tp->speed & LINK_STATUS)) {
+	if (tp->speed & LINK_STATUS) {
+		if (!(old_speed & LINK_STATUS)) {
 			tp->rtl_ops.enable(tp);
 			set_bit(RTL8152_SET_RX_MODE, &tp->flags);
 			netif_carrier_on(netdev);
 		}
 	} else {
-		if (tp->speed & LINK_STATUS) {
+		if (old_speed & LINK_STATUS) {
 			netif_carrier_off(netdev);
 			tasklet_disable(&tp->tl);
 			tp->rtl_ops.disable(tp);
 			tasklet_enable(&tp->tl);
 		}
 	}
-	tp->speed = speed;
 }
 
 static void rtl_work_func_t(struct work_struct *work)

-- 
2.1.0

From: Hayes Wang <redacted>
Date: Fri, 19 Dec 2014 16:56:00 +0800

Don't submit the rx if the device is unplugged, linking down,
or stopped.

 ...

I think netif_carrier_off() should always be true in all three of those
situations, and would be a much simpler test than what you've coded
here.

 David Miller [mailto:davem@davemloft.net] 

Sent: Saturday, December 20, 2014 4:44 AM

[...]

quoted

Don't submit the rx if the device is unplugged, linking down,
or stopped.

 ...

quoted

@@ -1789,6 +1789,11 @@ int r8152_submit_rx(struct r8152 

*tp, struct rx_agg *agg, gfp_t mem_flags)

quoted

 {
 	int ret;
 
+	/* The rx would be stopped, so skip submitting */
+	if (test_bit(RTL8152_UNPLUG, &tp->flags) ||
+	    !test_bit(WORK_ENABLE, &tp->flags) || !(tp->speed & LINK_STATUS))
+		return 0;
+

I think netif_carrier_off() should always be true in all three of those
situations, and would be a much simpler test than what you've coded
here.

When the device is unplugged or stopped, the linking status
may be true, so I add additional checks to avoid the submission.

Besides, in set_carrier() I set netif_carrier_on() after
ops.enable() to avoid any transmission before I finish
starting the tx/rx.

	tp->rtl_ops.enable(tp);
	set_bit(RTL8152_SET_RX_MODE, &tp->flags);
	netif_carrier_on(netdev);

However, the r8152_submit_rx() would be called in ops.enable(),
and the check of netif_carrier_ok() would be always false. That
is why I use tp->speed, not netif_carrier_ok(), to check the
linking stauts.
 
Best Regards,
Hayes

From: Hayes Wang <redacted>
Date: Mon, 22 Dec 2014 02:53:42 +0000

 David Miller [mailto:davem@davemloft.net] 

quoted

Sent: Saturday, December 20, 2014 4:44 AM

[...]

quoted

quoted

Don't submit the rx if the device is unplugged, linking down,
or stopped.

 ...

quoted

@@ -1789,6 +1789,11 @@ int r8152_submit_rx(struct r8152 

*tp, struct rx_agg *agg, gfp_t mem_flags)

quoted

 {
 	int ret;
 
+	/* The rx would be stopped, so skip submitting */
+	if (test_bit(RTL8152_UNPLUG, &tp->flags) ||
+	    !test_bit(WORK_ENABLE, &tp->flags) || !(tp->speed & LINK_STATUS))
+		return 0;
+

I think netif_carrier_off() should always be true in all three of those
situations, and would be a much simpler test than what you've coded
here.

When the device is unplugged or stopped, the linking status
may be true, so I add additional checks to avoid the submission.

Besides, in set_carrier() I set netif_carrier_on() after
ops.enable() to avoid any transmission before I finish
starting the tx/rx.

	tp->rtl_ops.enable(tp);
	set_bit(RTL8152_SET_RX_MODE, &tp->flags);
	netif_carrier_on(netdev);

However, the r8152_submit_rx() would be called in ops.enable(),
and the check of netif_carrier_ok() would be always false. That
is why I use tp->speed, not netif_carrier_ok(), to check the
linking stauts.

I stil think your check is way too complicated for this fast path so I
would ask that you arrange things such that the simpler
netif_carrier_off() test works.

Especially because that is what the core networking stack uses
to decide whether to send packets to us as well.

v2:
Replace the patch #1 with "call rtl_start_rx after netif_carrier_on".

For patch #2, replace checking tp->speed with netif_carrier_ok.

v1:
Avoid r8152_submit_rx() from submitting rx during unexpected
moment. This could reduce the time of stopping rx.

For patch #1, the tp->speed should be updated early. Then,
the patch #2 could use it to check the current linking status.

Hayes Wang (2):
  r8152: call rtl_start_rx after netif_carrier_on
  r8152: check the status before submitting rx

 drivers/net/usb/r8152.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.1.0

Remove rtl_start_rx() from rtl_enable() and put it after calling
netif_carrier_on().

Signed-off-by: Hayes Wang <redacted>
---
 drivers/net/usb/r8152.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 2d1c77e..cbe450c 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -2043,7 +2043,7 @@ static int rtl_enable(struct r8152 *tp)
 
 	rxdy_gated_en(tp, false);
 
-	return rtl_start_rx(tp);
+	return 0;
 }
 
 static int rtl8152_enable(struct r8152 *tp)

@@ -2858,6 +2858,7 @@ static void set_carrier(struct r8152 *tp)
 			tp->rtl_ops.enable(tp);
 			set_bit(RTL8152_SET_RX_MODE, &tp->flags);
 			netif_carrier_on(netdev);
+			rtl_start_rx(tp);
 		}
 	} else {
 		if (tp->speed & LINK_STATUS) {

-- 
2.1.0

Don't submit the rx if the device is unplugged, stopped, or
linking down.

Signed-off-by: Hayes Wang <redacted>
---
 drivers/net/usb/r8152.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index cbe450c..8ecc2df 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -1789,6 +1789,11 @@ 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) ||
+	    !test_bit(WORK_ENABLE, &tp->flags) || !netif_carrier_ok(tp->netdev))
+		return 0;
+
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
 			  agg->head, agg_buf_sz,
 			  (usb_complete_t)read_bulk_callback, agg);

-- 
2.1.0