* [PATCH net 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend
2017-01-25 2:50 [PATCH net 0/4] r8152: fix scheduling napi Hayes Wang
@ 2017-01-25 2:50 ` Hayes Wang
2017-01-25 5:02 ` Stephen Hemminger
2017-01-25 2:50 ` [PATCH net 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled Hayes Wang
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Hayes Wang @ 2017-01-25 2:50 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH net 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend
2017-01-25 2:50 ` [PATCH net 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend Hayes Wang
@ 2017-01-25 5:02 ` Stephen Hemminger
0 siblings, 0 replies; 23+ messages in thread
From: Stephen Hemminger @ 2017-01-25 5:02 UTC (permalink / raw)
To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH net 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled
2017-01-25 2:50 [PATCH net 0/4] r8152: fix scheduling napi Hayes Wang
2017-01-25 2:50 ` [PATCH net 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend Hayes Wang
@ 2017-01-25 2:50 ` Hayes Wang
2017-01-25 2:50 ` [PATCH net 3/4] r8152: re-schedule napi for tx Hayes Wang
` (3 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-25 2:50 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net 3/4] r8152: re-schedule napi for tx
2017-01-25 2:50 [PATCH net 0/4] r8152: fix scheduling napi Hayes Wang
2017-01-25 2:50 ` [PATCH net 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend Hayes Wang
2017-01-25 2:50 ` [PATCH net 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled Hayes Wang
@ 2017-01-25 2:50 ` Hayes Wang
2017-01-25 2:50 ` [PATCH net 4/4] r8152: check rx after napi is enabled Hayes Wang
` (2 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-25 2:50 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net 4/4] r8152: check rx after napi is enabled
2017-01-25 2:50 [PATCH net 0/4] r8152: fix scheduling napi Hayes Wang
` (2 preceding siblings ...)
2017-01-25 2:50 ` [PATCH net 3/4] r8152: re-schedule napi for tx Hayes Wang
@ 2017-01-25 2:50 ` Hayes Wang
2017-01-25 8:13 ` [PATCH net v2 0/4] r8152: fix scheduling napi Hayes Wang
2017-01-26 1:38 ` [PATCH net v3 " Hayes Wang
5 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-25 2:50 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net v2 0/4] r8152: fix scheduling napi
2017-01-25 2:50 [PATCH net 0/4] r8152: fix scheduling napi Hayes Wang
` (3 preceding siblings ...)
2017-01-25 2:50 ` [PATCH net 4/4] r8152: check rx after napi is enabled Hayes Wang
@ 2017-01-25 8:13 ` Hayes Wang
2017-01-25 8:13 ` [PATCH net v2 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend Hayes Wang
` (4 more replies)
2017-01-26 1:38 ` [PATCH net v3 " Hayes Wang
5 siblings, 5 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-25 8:13 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH net v2 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend
2017-01-25 8:13 ` [PATCH net v2 0/4] r8152: fix scheduling napi Hayes Wang
@ 2017-01-25 8:13 ` Hayes Wang
2017-01-25 8:13 ` [PATCH net v2 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled Hayes Wang
` (3 subsequent siblings)
4 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-25 8:13 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net v2 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled
2017-01-25 8:13 ` [PATCH net v2 0/4] r8152: fix scheduling napi Hayes Wang
2017-01-25 8:13 ` [PATCH net v2 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend Hayes Wang
@ 2017-01-25 8:13 ` Hayes Wang
2017-01-25 8:13 ` [PATCH net v2 3/4] r8152: re-schedule napi for tx Hayes Wang
` (2 subsequent siblings)
4 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-25 8:13 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net v2 3/4] r8152: re-schedule napi for tx
2017-01-25 8:13 ` [PATCH net v2 0/4] r8152: fix scheduling napi Hayes Wang
2017-01-25 8:13 ` [PATCH net v2 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend Hayes Wang
2017-01-25 8:13 ` [PATCH net v2 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled Hayes Wang
@ 2017-01-25 8:13 ` Hayes Wang
2017-01-25 13:57 ` Eric Dumazet
2017-01-25 8:13 ` [PATCH net v2 4/4] r8152: check rx after napi is enabled Hayes Wang
2017-01-25 19:31 ` [PATCH net v2 0/4] r8152: fix scheduling napi David Miller
4 siblings, 1 reply; 23+ messages in thread
From: Hayes Wang @ 2017-01-25 8:13 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH net v2 3/4] r8152: re-schedule napi for tx
2017-01-25 8:13 ` [PATCH net v2 3/4] r8152: re-schedule napi for tx Hayes Wang
@ 2017-01-25 13:57 ` Eric Dumazet
2017-01-26 1:22 ` Hayes Wang
0 siblings, 1 reply; 23+ messages in thread
From: Eric Dumazet @ 2017-01-25 13:57 UTC (permalink / raw)
To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
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 ?
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH net v2 3/4] r8152: re-schedule napi for tx
2017-01-25 13:57 ` Eric Dumazet
@ 2017-01-26 1:22 ` Hayes Wang
0 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-26 1:22 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH net v2 4/4] r8152: check rx after napi is enabled
2017-01-25 8:13 ` [PATCH net v2 0/4] r8152: fix scheduling napi Hayes Wang
` (2 preceding siblings ...)
2017-01-25 8:13 ` [PATCH net v2 3/4] r8152: re-schedule napi for tx Hayes Wang
@ 2017-01-25 8:13 ` Hayes Wang
2017-01-25 19:31 ` [PATCH net v2 0/4] r8152: fix scheduling napi David Miller
4 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-25 8:13 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH net v2 0/4] r8152: fix scheduling napi
2017-01-25 8:13 ` [PATCH net v2 0/4] r8152: fix scheduling napi Hayes Wang
` (3 preceding siblings ...)
2017-01-25 8:13 ` [PATCH net v2 4/4] r8152: check rx after napi is enabled Hayes Wang
@ 2017-01-25 19:31 ` David Miller
2017-01-26 3:04 ` Hayes Wang
4 siblings, 1 reply; 23+ messages in thread
From: David Miller @ 2017-01-25 19:31 UTC (permalink / raw)
To: hayeswang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
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.
^ permalink raw reply [flat|nested] 23+ messages in thread* RE: [PATCH net v2 0/4] r8152: fix scheduling napi
2017-01-25 19:31 ` [PATCH net v2 0/4] r8152: fix scheduling napi David Miller
@ 2017-01-26 3:04 ` Hayes Wang
2017-01-26 3:46 ` David Miller
0 siblings, 1 reply; 23+ messages in thread
From: Hayes Wang @ 2017-01-26 3:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH net v3 0/4] r8152: fix scheduling napi
2017-01-25 2:50 [PATCH net 0/4] r8152: fix scheduling napi Hayes Wang
` (4 preceding siblings ...)
2017-01-25 8:13 ` [PATCH net v2 0/4] r8152: fix scheduling napi Hayes Wang
@ 2017-01-26 1:38 ` Hayes Wang
2017-01-26 1:38 ` [PATCH net v3 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend Hayes Wang
` (4 more replies)
5 siblings, 5 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-26 1:38 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH net v3 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend
2017-01-26 1:38 ` [PATCH net v3 " Hayes Wang
@ 2017-01-26 1:38 ` Hayes Wang
2017-01-26 1:38 ` [PATCH net v3 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled Hayes Wang
` (3 subsequent siblings)
4 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-26 1:38 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net v3 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled
2017-01-26 1:38 ` [PATCH net v3 " Hayes Wang
2017-01-26 1:38 ` [PATCH net v3 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend Hayes Wang
@ 2017-01-26 1:38 ` Hayes Wang
2017-01-26 1:38 ` [PATCH net v3 3/4] r8152: re-schedule napi for tx Hayes Wang
` (2 subsequent siblings)
4 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-26 1:38 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net v3 3/4] r8152: re-schedule napi for tx
2017-01-26 1:38 ` [PATCH net v3 " Hayes Wang
2017-01-26 1:38 ` [PATCH net v3 1/4] r8152: avoid start_xmit to call napi_schedule during autosuspend Hayes Wang
2017-01-26 1:38 ` [PATCH net v3 2/4] r8152: avoid start_xmit to schedule napi when napi is disabled Hayes Wang
@ 2017-01-26 1:38 ` Hayes Wang
2017-01-26 1:38 ` [PATCH net v3 4/4] r8152: check rx after napi is enabled Hayes Wang
2017-01-26 3:47 ` [PATCH net v3 0/4] r8152: fix scheduling napi David Miller
4 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-26 1:38 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH net v3 4/4] r8152: check rx after napi is enabled
2017-01-26 1:38 ` [PATCH net v3 " Hayes Wang
` (2 preceding siblings ...)
2017-01-26 1:38 ` [PATCH net v3 3/4] r8152: re-schedule napi for tx Hayes Wang
@ 2017-01-26 1:38 ` Hayes Wang
2017-01-26 3:47 ` [PATCH net v3 0/4] r8152: fix scheduling napi David Miller
4 siblings, 0 replies; 23+ messages in thread
From: Hayes Wang @ 2017-01-26 1:38 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH net v3 0/4] r8152: fix scheduling napi
2017-01-26 1:38 ` [PATCH net v3 " Hayes Wang
` (3 preceding siblings ...)
2017-01-26 1:38 ` [PATCH net v3 4/4] r8152: check rx after napi is enabled Hayes Wang
@ 2017-01-26 3:47 ` David Miller
2017-01-26 3:52 ` Hayes Wang
4 siblings, 1 reply; 23+ messages in thread
From: David Miller @ 2017-01-26 3:47 UTC (permalink / raw)
To: hayeswang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
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.
^ permalink raw reply [flat|nested] 23+ messages in thread