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

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

[Hayes Wang]

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

[PATCH net-next 1/2] r8152: adjust set_carrier

[Hayes Wang]

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 <hayeswang@realtek.com>
---
 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

[PATCH net-next 2/2] r8152: check the status before submitting rx

[Hayes Wang]

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

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 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

Re: [PATCH net-next 2/2] r8152: check the status before submitting rx

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Fri, 19 Dec 2014 16:56:00 +0800

> Don't submit the rx if the device is unplugged, linking down,
> or stopped.
 ...
> @@ -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;
> +

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.

RE: [PATCH net-next 2/2] r8152: check the status before submittingrx

[Hayes Wang]

 David Miller [mailto:davem@davemloft.net] 
> Sent: Saturday, December 20, 2014 4:44 AM
[...]
> > Don't submit the rx if the device is unplugged, linking down,
> > or stopped.
>  ...
> > @@ -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;
> > +
> 
> 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

Re: [PATCH net-next 2/2] r8152: check the status before submittingrx

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Mon, 22 Dec 2014 02:53:42 +0000

>  David Miller [mailto:davem@davemloft.net] 
>> Sent: Saturday, December 20, 2014 4:44 AM
> [...]
>> > Don't submit the rx if the device is unplugged, linking down,
>> > or stopped.
>>  ...
>> > @@ -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;
>> > +
>> 
>> 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.

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

[Hayes Wang]

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

[PATCH net-next v2 1/2] r8152: call rtl_start_rx after netif_carrier_on

[Hayes Wang]

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

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 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

[PATCH net-next v2 2/2] r8152: check the status before submitting rx

[Hayes Wang]

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

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 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