[PATCH net 0/4] r8152: fix scheduling napi

[PATCH net 0/4] r8152: fix scheduling napi

[Hayes Wang]

Scheduling the napi during the following periods would let it be ignored.
And the events wouldn't be handled until next napi_schedule() is called.

1. after napi_disable and before napi_enable().
2. after all actions of napi function is completed and before calling
   napi_complete().

If no next napi_schedule() is called, tx or rx would stop working.

In order to avoid these situations, the followings solutions are applied.

1. prevent start_xmit() from calling napi_schedule() during runtime suspend
   or after napi_disable().
2. re-schedule the napi for tx if it is necessary.
3. check if any rx is finished or not after napi_enable().

Hayes Wang (4):
  r8152: avoid start_xmit to call napi_schedule during autosuspend
  r8152: avoid start_xmit to schedule napi when napi is disabled
  r8152: re-schedule napi for tx
  r8152: check rx after napi is enabled

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

-- 
2.7.4

[PATCH net 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend

[Hayes Wang]

Adjust the setting of the flag of SELECTIVE_SUSPEND to prevent start_xmit()
from calling napi_schedule() directly during runtime suspend.

After calling napi_disable() or clearing the flag of WORK_ENABLE,
scheduling the napi is useless.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index e1466b4..27b0b44 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3585,10 +3585,13 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
 	struct net_device *netdev = tp->netdev;
 	int ret = 0;
 
+	set_bit(SELECTIVE_SUSPEND, &tp->flags);
+
 	if (netif_running(netdev) && test_bit(WORK_ENABLE, &tp->flags)) {
 		u32 rcr = 0;
 
 		if (delay_autosuspend(tp)) {
+			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			ret = -EBUSY;
 			goto out1;
 		}
@@ -3605,6 +3608,7 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
 			if (!(ocp_data & RXFIFO_EMPTY)) {
 				rxdy_gated_en(tp, false);
 				ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, rcr);
+				clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 				ret = -EBUSY;
 				goto out1;
 			}
@@ -3624,8 +3628,6 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
 		}
 	}
 
-	set_bit(SELECTIVE_SUSPEND, &tp->flags);
-
 out1:
 	return ret;
 }
@@ -3681,12 +3683,12 @@ static int rtl8152_resume(struct usb_interface *intf)
 	if (netif_running(tp->netdev) && tp->netdev->flags & IFF_UP) {
 		if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
 			tp->rtl_ops.autosuspend_en(tp, false);
-			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			napi_disable(&tp->napi);
 			set_bit(WORK_ENABLE, &tp->flags);
 			if (netif_carrier_ok(tp->netdev))
 				rtl_start_rx(tp);
 			napi_enable(&tp->napi);
+			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 		} else {
 			tp->rtl_ops.up(tp);
 			netif_carrier_off(tp->netdev);
-- 
2.7.4

[PATCH net 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled

[Hayes Wang]

Stop the tx when the napi is disabled to prevent napi_schedule() is
called.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 27b0b44..3454238 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3155,10 +3155,13 @@ static void set_carrier(struct r8152 *tp)
 		if (!netif_carrier_ok(netdev)) {
 			tp->rtl_ops.enable(tp);
 			set_bit(RTL8152_SET_RX_MODE, &tp->flags);
+			netif_stop_queue(netdev);
 			napi_disable(&tp->napi);
 			netif_carrier_on(netdev);
 			rtl_start_rx(tp);
 			napi_enable(&tp->napi);
+			netif_wake_queue(netdev);
+			netif_info(tp, link, netdev, "carrier on\n");
 		}
 	} else {
 		if (netif_carrier_ok(netdev)) {
@@ -3166,6 +3169,7 @@ static void set_carrier(struct r8152 *tp)
 			napi_disable(&tp->napi);
 			tp->rtl_ops.disable(tp);
 			napi_enable(&tp->napi);
+			netif_info(tp, link, netdev, "carrier off\n");
 		}
 	}
 }
@@ -3515,12 +3519,12 @@ static int rtl8152_pre_reset(struct usb_interface *intf)
 	if (!netif_running(netdev))
 		return 0;
 
+	netif_stop_queue(netdev);
 	napi_disable(&tp->napi);
 	clear_bit(WORK_ENABLE, &tp->flags);
 	usb_kill_urb(tp->intr_urb);
 	cancel_delayed_work_sync(&tp->schedule);
 	if (netif_carrier_ok(netdev)) {
-		netif_stop_queue(netdev);
 		mutex_lock(&tp->control);
 		tp->rtl_ops.disable(tp);
 		mutex_unlock(&tp->control);
@@ -3548,10 +3552,10 @@ static int rtl8152_post_reset(struct usb_interface *intf)
 		rtl_start_rx(tp);
 		rtl8152_set_rx_mode(netdev);
 		mutex_unlock(&tp->control);
-		netif_wake_queue(netdev);
 	}
 
 	napi_enable(&tp->napi);
+	netif_wake_queue(netdev);
 	usb_submit_urb(tp->intr_urb, GFP_KERNEL);
 
 	return 0;
-- 
2.7.4

[PATCH net 3/4] r8152: re-schedule napi for tx

[Hayes Wang]

Re-schedule napi after napi_complete() for tx, if it is necessay.

In r8152_poll(), if the tx is completed after tx_bottom() and before
napi_complete(), the scheduling of napi would be lost. Then, no
one handles the next tx until the next napi_schedule() is called.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3454238..f65109b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1936,6 +1936,9 @@ static int r8152_poll(struct napi_struct *napi, int budget)
 		napi_complete(napi);
 		if (!list_empty(&tp->rx_done))
 			napi_schedule(napi);
+		else if (!skb_queue_empty(&tp->tx_queue) &&
+			 !list_empty(&tp->tx_free))
+			napi_schedule(&tp->napi);
 	}
 
 	return work_done;
-- 
2.7.4

[PATCH net 4/4] r8152: check rx after napi is enabled

[Hayes Wang]

Schedule the napi after napi_enable() for rx, if it is necessary.

If the rx is completed when napi is disabled, the sheduling of napi
would be lost. Then, no one handles the rx packet until next napi
is scheduled.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f65109b..f0f55b3 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -32,7 +32,7 @@
 #define NETNEXT_VERSION		"08"
 
 /* Information for net */
-#define NET_VERSION		"7"
+#define NET_VERSION		"8"
 
 #define DRIVER_VERSION		"v1." NETNEXT_VERSION "." NET_VERSION
 #define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_swsd@realtek.com>"
@@ -3561,6 +3561,9 @@ static int rtl8152_post_reset(struct usb_interface *intf)
 	netif_wake_queue(netdev);
 	usb_submit_urb(tp->intr_urb, GFP_KERNEL);
 
+	if (!list_empty(&tp->rx_done))
+		napi_schedule(&tp->napi);
+
 	return 0;
 }
 
@@ -3696,6 +3699,8 @@ static int rtl8152_resume(struct usb_interface *intf)
 				rtl_start_rx(tp);
 			napi_enable(&tp->napi);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+			if (!list_empty(&tp->rx_done))
+				napi_schedule(&tp->napi);
 		} else {
 			tp->rtl_ops.up(tp);
 			netif_carrier_off(tp->netdev);
-- 
2.7.4

Re: [PATCH net 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend

[Stephen Hemminger]

On Wed, 25 Jan 2017 10:50:51 +0800
Hayes Wang <hayeswang@realtek.com> wrote:

> Adjust the setting of the flag of SELECTIVE_SUSPEND to prevent start_xmit()
> from calling napi_schedule() directly during runtime suspend.
> 
> After calling napi_disable() or clearing the flag of WORK_ENABLE,
> scheduling the napi is useless.
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
>  drivers/net/usb/r8152.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index e1466b4..27b0b44 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -3585,10 +3585,13 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
>  	struct net_device *netdev = tp->netdev;
>  	int ret = 0;
>  
> +	set_bit(SELECTIVE_SUSPEND, &tp->flags);
> +
>  	if (netif_running(netdev) && test_bit(WORK_ENABLE, &tp->flags)) {
>  		u32 rcr = 0;
>  
>  		if (delay_autosuspend(tp)) {
> +			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
>  			ret = -EBUSY;
>  			goto out1;
>  		}
> @@ -3605,6 +3608,7 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
>  			if (!(ocp_data & RXFIFO_EMPTY)) {
>  				rxdy_gated_en(tp, false);
>  				ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, rcr);
> +				clear_bit(SELECTIVE_SUSPEND, &tp->flags);
>  				ret = -EBUSY;

If you are going to start using bit operations then you may need smp_mb_before/after_atomic.

[PATCH net v2 0/4] r8152: fix scheduling napi

[Hayes Wang]

v2:
Add smp_mb__after_atomic() for patch #1.

v1:
Scheduling the napi during the following periods would let it be ignored.
And the events wouldn't be handled until next napi_schedule() is called.

1. after napi_disable and before napi_enable().
2. after all actions of napi function is completed and before calling
   napi_complete().

If no next napi_schedule() is called, tx or rx would stop working.

In order to avoid these situations, the followings solutions are applied.

1. prevent start_xmit() from calling napi_schedule() during runtime suspend
   or after napi_disable().
2. re-schedule the napi for tx if it is necessary.
3. check if any rx is finished or not after napi_enable().

Hayes Wang (4):
  r8152: avoid start_xmit to call napi_schedule during autosuspend
  r8152: avoid start_xmit to schedule napi when napi is disabled
  r8152: re-schedule napi for tx
  r8152: check rx after napi is enabled

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

-- 
2.7.4

[PATCH net v2 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend

[Hayes Wang]

Adjust the setting of the flag of SELECTIVE_SUSPEND to prevent start_xmit()
from calling napi_schedule() directly during runtime suspend.

After calling napi_disable() or clearing the flag of WORK_ENABLE,
scheduling the napi is useless.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index e1466b4..23bef8e 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3585,10 +3585,15 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
 	struct net_device *netdev = tp->netdev;
 	int ret = 0;
 
+	set_bit(SELECTIVE_SUSPEND, &tp->flags);
+	smp_mb__after_atomic();
+
 	if (netif_running(netdev) && test_bit(WORK_ENABLE, &tp->flags)) {
 		u32 rcr = 0;
 
 		if (delay_autosuspend(tp)) {
+			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+			smp_mb__after_atomic();
 			ret = -EBUSY;
 			goto out1;
 		}
@@ -3605,6 +3610,8 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
 			if (!(ocp_data & RXFIFO_EMPTY)) {
 				rxdy_gated_en(tp, false);
 				ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, rcr);
+				clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+				smp_mb__after_atomic();
 				ret = -EBUSY;
 				goto out1;
 			}
@@ -3624,8 +3631,6 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
 		}
 	}
 
-	set_bit(SELECTIVE_SUSPEND, &tp->flags);
-
 out1:
 	return ret;
 }
@@ -3681,12 +3686,13 @@ static int rtl8152_resume(struct usb_interface *intf)
 	if (netif_running(tp->netdev) && tp->netdev->flags & IFF_UP) {
 		if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
 			tp->rtl_ops.autosuspend_en(tp, false);
-			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			napi_disable(&tp->napi);
 			set_bit(WORK_ENABLE, &tp->flags);
 			if (netif_carrier_ok(tp->netdev))
 				rtl_start_rx(tp);
 			napi_enable(&tp->napi);
+			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+			smp_mb__after_atomic();
 		} else {
 			tp->rtl_ops.up(tp);
 			netif_carrier_off(tp->netdev);
-- 
2.7.4

[PATCH net v2 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled

[Hayes Wang]

Stop the tx when the napi is disabled to prevent napi_schedule() is
called.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 23bef8e..ec882be 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3155,10 +3155,13 @@ static void set_carrier(struct r8152 *tp)
 		if (!netif_carrier_ok(netdev)) {
 			tp->rtl_ops.enable(tp);
 			set_bit(RTL8152_SET_RX_MODE, &tp->flags);
+			netif_stop_queue(netdev);
 			napi_disable(&tp->napi);
 			netif_carrier_on(netdev);
 			rtl_start_rx(tp);
 			napi_enable(&tp->napi);
+			netif_wake_queue(netdev);
+			netif_info(tp, link, netdev, "carrier on\n");
 		}
 	} else {
 		if (netif_carrier_ok(netdev)) {
@@ -3166,6 +3169,7 @@ static void set_carrier(struct r8152 *tp)
 			napi_disable(&tp->napi);
 			tp->rtl_ops.disable(tp);
 			napi_enable(&tp->napi);
+			netif_info(tp, link, netdev, "carrier off\n");
 		}
 	}
 }
@@ -3515,12 +3519,12 @@ static int rtl8152_pre_reset(struct usb_interface *intf)
 	if (!netif_running(netdev))
 		return 0;
 
+	netif_stop_queue(netdev);
 	napi_disable(&tp->napi);
 	clear_bit(WORK_ENABLE, &tp->flags);
 	usb_kill_urb(tp->intr_urb);
 	cancel_delayed_work_sync(&tp->schedule);
 	if (netif_carrier_ok(netdev)) {
-		netif_stop_queue(netdev);
 		mutex_lock(&tp->control);
 		tp->rtl_ops.disable(tp);
 		mutex_unlock(&tp->control);
@@ -3548,10 +3552,10 @@ static int rtl8152_post_reset(struct usb_interface *intf)
 		rtl_start_rx(tp);
 		rtl8152_set_rx_mode(netdev);
 		mutex_unlock(&tp->control);
-		netif_wake_queue(netdev);
 	}
 
 	napi_enable(&tp->napi);
+	netif_wake_queue(netdev);
 	usb_submit_urb(tp->intr_urb, GFP_KERNEL);
 
 	return 0;
-- 
2.7.4

[PATCH net v2 3/4] r8152: re-schedule napi for tx

[Hayes Wang]

Re-schedule napi after napi_complete() for tx, if it is necessay.

In r8152_poll(), if the tx is completed after tx_bottom() and before
napi_complete(), the scheduling of napi would be lost. Then, no
one handles the next tx until the next napi_schedule() is called.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ec882be..45d168e 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1936,6 +1936,9 @@ static int r8152_poll(struct napi_struct *napi, int budget)
 		napi_complete(napi);
 		if (!list_empty(&tp->rx_done))
 			napi_schedule(napi);
+		else if (!skb_queue_empty(&tp->tx_queue) &&
+			 !list_empty(&tp->tx_free))
+			napi_schedule(&tp->napi);
 	}
 
 	return work_done;
-- 
2.7.4

[PATCH net v2 4/4] r8152: check rx after napi is enabled

[Hayes Wang]

Schedule the napi after napi_enable() for rx, if it is necessary.

If the rx is completed when napi is disabled, the sheduling of napi
would be lost. Then, no one handles the rx packet until next napi
is scheduled.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 45d168e..8924520 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -32,7 +32,7 @@
 #define NETNEXT_VERSION		"08"
 
 /* Information for net */
-#define NET_VERSION		"7"
+#define NET_VERSION		"8"
 
 #define DRIVER_VERSION		"v1." NETNEXT_VERSION "." NET_VERSION
 #define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_swsd@realtek.com>"
@@ -3561,6 +3561,9 @@ static int rtl8152_post_reset(struct usb_interface *intf)
 	netif_wake_queue(netdev);
 	usb_submit_urb(tp->intr_urb, GFP_KERNEL);
 
+	if (!list_empty(&tp->rx_done))
+		napi_schedule(&tp->napi);
+
 	return 0;
 }
 
@@ -3700,6 +3703,8 @@ static int rtl8152_resume(struct usb_interface *intf)
 			napi_enable(&tp->napi);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			smp_mb__after_atomic();
+			if (!list_empty(&tp->rx_done))
+				napi_schedule(&tp->napi);
 		} else {
 			tp->rtl_ops.up(tp);
 			netif_carrier_off(tp->netdev);
-- 
2.7.4

Re: [PATCH net v2 3/4] r8152: re-schedule napi for tx

[Eric Dumazet]

On Wed, 2017-01-25 at 16:13 +0800, Hayes Wang wrote:
> Re-schedule napi after napi_complete() for tx, if it is necessay.
> 
> In r8152_poll(), if the tx is completed after tx_bottom() and before
> napi_complete(), the scheduling of napi would be lost. Then, no
> one handles the next tx until the next napi_schedule() is called.
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
>  drivers/net/usb/r8152.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index ec882be..45d168e 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1936,6 +1936,9 @@ static int r8152_poll(struct napi_struct *napi, int budget)
>  		napi_complete(napi);
>  		if (!list_empty(&tp->rx_done))
>  			napi_schedule(napi);
> +		else if (!skb_queue_empty(&tp->tx_queue) &&
> +			 !list_empty(&tp->tx_free))
> +			napi_schedule(&tp->napi);

Why using &tp->napi instead of napi here, as done 3 lines above ?

Re: [PATCH net v2 0/4] r8152: fix scheduling napi

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Wed, 25 Jan 2017 16:13:17 +0800

> v2:
> Add smp_mb__after_atomic() for patch #1.
> 
> v1:
> Scheduling the napi during the following periods would let it be ignored.
> And the events wouldn't be handled until next napi_schedule() is called.
> 
> 1. after napi_disable and before napi_enable().
> 2. after all actions of napi function is completed and before calling
>    napi_complete().
> 
> If no next napi_schedule() is called, tx or rx would stop working.
> 
> In order to avoid these situations, the followings solutions are applied.
> 
> 1. prevent start_xmit() from calling napi_schedule() during runtime suspend
>    or after napi_disable().
> 2. re-schedule the napi for tx if it is necessary.
> 3. check if any rx is finished or not after napi_enable().

I think the fundamental issue is that since you can't stop URBs from
queueing up, you cannot properly synchronize NAPI and schedule polling
properly.

>From my perspective what happened here is you want GRO support, but it
comes at the expense of this extremely racey NAPI support which does
not at all achieve one of the main advantages of NAPI which is
interrupt mitigation.

It would have been so much better to implement a generic way for
drivers to get GRO support without NAPI, especially if their packet
feeding engine works the way that the USB networking drivers's do.

RE: [PATCH net v2 3/4] r8152: re-schedule napi for tx

[Hayes Wang]

Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Wednesday, January 25, 2017 9:57 PM
[...]
> >  		napi_complete(napi);
> >  		if (!list_empty(&tp->rx_done))
> >  			napi_schedule(napi);
> > +		else if (!skb_queue_empty(&tp->tx_queue) &&
> > +			 !list_empty(&tp->tx_free))
> > +			napi_schedule(&tp->napi);
> 
> Why using &tp->napi instead of napi here, as done 3 lines above ?

Oops. I would fix it. Thanks.

Best Regards,
Hayes

[PATCH net v3 0/4] r8152: fix scheduling napi

[Hayes Wang]

v3:
simply the argument for patch #3. Replace &tp->napi with napi.

v2:
Add smp_mb__after_atomic() for patch #1.

v1:
Scheduling the napi during the following periods would let it be ignored.
And the events wouldn't be handled until next napi_schedule() is called.

1. after napi_disable and before napi_enable().
2. after all actions of napi function is completed and before calling
   napi_complete().

If no next napi_schedule() is called, tx or rx would stop working.

In order to avoid these situations, the followings solutions are applied.

1. prevent start_xmit() from calling napi_schedule() during runtime suspend
   or after napi_disable().
2. re-schedule the napi for tx if it is necessary.
3. check if any rx is finished or not after napi_enable().

Hayes Wang (4):
  r8152: avoid start_xmit to call napi_schedule during autosuspend
  r8152: avoid start_xmit to schedule napi when napi is disabled
  r8152: re-schedule napi for tx
  r8152: check rx after napi is enabled

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

-- 
2.7.4

[PATCH net v3 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend

[Hayes Wang]

Adjust the setting of the flag of SELECTIVE_SUSPEND to prevent start_xmit()
from calling napi_schedule() directly during runtime suspend.

After calling napi_disable() or clearing the flag of WORK_ENABLE,
scheduling the napi is useless.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index e1466b4..23bef8e 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3585,10 +3585,15 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
 	struct net_device *netdev = tp->netdev;
 	int ret = 0;
 
+	set_bit(SELECTIVE_SUSPEND, &tp->flags);
+	smp_mb__after_atomic();
+
 	if (netif_running(netdev) && test_bit(WORK_ENABLE, &tp->flags)) {
 		u32 rcr = 0;
 
 		if (delay_autosuspend(tp)) {
+			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+			smp_mb__after_atomic();
 			ret = -EBUSY;
 			goto out1;
 		}
@@ -3605,6 +3610,8 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
 			if (!(ocp_data & RXFIFO_EMPTY)) {
 				rxdy_gated_en(tp, false);
 				ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, rcr);
+				clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+				smp_mb__after_atomic();
 				ret = -EBUSY;
 				goto out1;
 			}
@@ -3624,8 +3631,6 @@ static int rtl8152_rumtime_suspend(struct r8152 *tp)
 		}
 	}
 
-	set_bit(SELECTIVE_SUSPEND, &tp->flags);
-
 out1:
 	return ret;
 }
@@ -3681,12 +3686,13 @@ static int rtl8152_resume(struct usb_interface *intf)
 	if (netif_running(tp->netdev) && tp->netdev->flags & IFF_UP) {
 		if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
 			tp->rtl_ops.autosuspend_en(tp, false);
-			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			napi_disable(&tp->napi);
 			set_bit(WORK_ENABLE, &tp->flags);
 			if (netif_carrier_ok(tp->netdev))
 				rtl_start_rx(tp);
 			napi_enable(&tp->napi);
+			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+			smp_mb__after_atomic();
 		} else {
 			tp->rtl_ops.up(tp);
 			netif_carrier_off(tp->netdev);
-- 
2.7.4

[PATCH net v3 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled

[Hayes Wang]

Stop the tx when the napi is disabled to prevent napi_schedule() is
called.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 23bef8e..ec882be 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3155,10 +3155,13 @@ static void set_carrier(struct r8152 *tp)
 		if (!netif_carrier_ok(netdev)) {
 			tp->rtl_ops.enable(tp);
 			set_bit(RTL8152_SET_RX_MODE, &tp->flags);
+			netif_stop_queue(netdev);
 			napi_disable(&tp->napi);
 			netif_carrier_on(netdev);
 			rtl_start_rx(tp);
 			napi_enable(&tp->napi);
+			netif_wake_queue(netdev);
+			netif_info(tp, link, netdev, "carrier on\n");
 		}
 	} else {
 		if (netif_carrier_ok(netdev)) {
@@ -3166,6 +3169,7 @@ static void set_carrier(struct r8152 *tp)
 			napi_disable(&tp->napi);
 			tp->rtl_ops.disable(tp);
 			napi_enable(&tp->napi);
+			netif_info(tp, link, netdev, "carrier off\n");
 		}
 	}
 }
@@ -3515,12 +3519,12 @@ static int rtl8152_pre_reset(struct usb_interface *intf)
 	if (!netif_running(netdev))
 		return 0;
 
+	netif_stop_queue(netdev);
 	napi_disable(&tp->napi);
 	clear_bit(WORK_ENABLE, &tp->flags);
 	usb_kill_urb(tp->intr_urb);
 	cancel_delayed_work_sync(&tp->schedule);
 	if (netif_carrier_ok(netdev)) {
-		netif_stop_queue(netdev);
 		mutex_lock(&tp->control);
 		tp->rtl_ops.disable(tp);
 		mutex_unlock(&tp->control);
@@ -3548,10 +3552,10 @@ static int rtl8152_post_reset(struct usb_interface *intf)
 		rtl_start_rx(tp);
 		rtl8152_set_rx_mode(netdev);
 		mutex_unlock(&tp->control);
-		netif_wake_queue(netdev);
 	}
 
 	napi_enable(&tp->napi);
+	netif_wake_queue(netdev);
 	usb_submit_urb(tp->intr_urb, GFP_KERNEL);
 
 	return 0;
-- 
2.7.4

[PATCH net v3 3/4] r8152: re-schedule napi for tx

[Hayes Wang]

Re-schedule napi after napi_complete() for tx, if it is necessay.

In r8152_poll(), if the tx is completed after tx_bottom() and before
napi_complete(), the scheduling of napi would be lost. Then, no
one handles the next tx until the next napi_schedule() is called.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ec882be..4785d2b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1936,6 +1936,9 @@ static int r8152_poll(struct napi_struct *napi, int budget)
 		napi_complete(napi);
 		if (!list_empty(&tp->rx_done))
 			napi_schedule(napi);
+		else if (!skb_queue_empty(&tp->tx_queue) &&
+			 !list_empty(&tp->tx_free))
+			napi_schedule(napi);
 	}
 
 	return work_done;
-- 
2.7.4

[PATCH net v3 4/4] r8152: check rx after napi is enabled

[Hayes Wang]

Schedule the napi after napi_enable() for rx, if it is necessary.

If the rx is completed when napi is disabled, the sheduling of napi
would be lost. Then, no one handles the rx packet until next napi
is scheduled.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 4785d2b..ad42295 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -32,7 +32,7 @@
 #define NETNEXT_VERSION		"08"
 
 /* Information for net */
-#define NET_VERSION		"7"
+#define NET_VERSION		"8"
 
 #define DRIVER_VERSION		"v1." NETNEXT_VERSION "." NET_VERSION
 #define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_swsd@realtek.com>"
@@ -3561,6 +3561,9 @@ static int rtl8152_post_reset(struct usb_interface *intf)
 	netif_wake_queue(netdev);
 	usb_submit_urb(tp->intr_urb, GFP_KERNEL);
 
+	if (!list_empty(&tp->rx_done))
+		napi_schedule(&tp->napi);
+
 	return 0;
 }
 
@@ -3700,6 +3703,8 @@ static int rtl8152_resume(struct usb_interface *intf)
 			napi_enable(&tp->napi);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			smp_mb__after_atomic();
+			if (!list_empty(&tp->rx_done))
+				napi_schedule(&tp->napi);
 		} else {
 			tp->rtl_ops.up(tp);
 			netif_carrier_off(tp->netdev);
-- 
2.7.4

RE: [PATCH net v2 0/4] r8152: fix scheduling napi

[Hayes Wang]

David Miller [mailto:davem@davemloft.net]
> Sent: Thursday, January 26, 2017 3:31 AM
[...]
> I think the fundamental issue is that since you can't stop URBs from
> queueing up, you cannot properly synchronize NAPI and schedule polling
> properly.
> 
> From my perspective what happened here is you want GRO support, but it
> comes at the expense of this extremely racey NAPI support which does
> not at all achieve one of the main advantages of NAPI which is
> interrupt mitigation.

May you apply these patches first, until I find another way to replace
current one? The driver uses NAPI now and I have no idea to find better
way to replace current one. I think it would take me long time to find
out the solution. And the issue is still there until I finish this work.
If now I give up the NAPI and its advantages except for the interrupt
mitigation, some things would become worse.

Our hw supports packet aggregation. The purpose is interrupt mitigation,
too. I wouldn't say it is better than what the NAPI does. However, I
could say we try to improve it. If the interrupt could be disabled, I
would be happy to do it. However, it is the limitation of USB devices.
That is why I still use NAPI even though the interrupt cannot be disabled
for USB devices. Because one of the advantages of NAPI couldn't be
satisfied, I must not use the NAPI. Doesn't it seem too strict?

Best Regards,
Hayes

Re: [PATCH net v2 0/4] r8152: fix scheduling napi

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Thu, 26 Jan 2017 03:04:45 +0000

> May you apply these patches first, until I find another way to replace
> current one?

Yes, I will.

Re: [PATCH net v3 0/4] r8152: fix scheduling napi

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Thu, 26 Jan 2017 09:38:30 +0800

> v3:
> simply the argument for patch #3. Replace &tp->napi with napi.
> 
> v2:
> Add smp_mb__after_atomic() for patch #1.
> 
> v1:
> Scheduling the napi during the following periods would let it be ignored.
> And the events wouldn't be handled until next napi_schedule() is called.
> 
> 1. after napi_disable and before napi_enable().
> 2. after all actions of napi function is completed and before calling
>    napi_complete().
> 
> If no next napi_schedule() is called, tx or rx would stop working.
> 
> In order to avoid these situations, the followings solutions are applied.
> 
> 1. prevent start_xmit() from calling napi_schedule() during runtime suspend
>    or after napi_disable().
> 2. re-schedule the napi for tx if it is necessary.
> 3. check if any rx is finished or not after napi_enable().

Series applied.

RE: [PATCH net v3 0/4] r8152: fix scheduling napi

[Hayes Wang]

David Miller [mailto:davem@davemloft.net]
> Sent: Thursday, January 26, 2017 11:48 AM
[...] 
> Series applied.

Thank you very much. I would try to find better way, too.

Best Regards,
Hayes