On 05/16/2018, 02:07 PM, Hayes Wang wrote:
> Oliver Neukum [mailto:[email protected]]
>> Sent: Wednesday, May 16, 2018 6:10 PM
> [...]
>>> Besides, I find a similar issue as following.
>>> https://www.spinics.net/lists/netdev/msg493512.html
>>
>> Well, if we have an imbalance in NAPI it should strike whereever
>> it is used, not just in suspend(). Is there debugging for NAPI
>> we could activate?
>
> No. The driver doesn't embed such debugging about it.
Despite of that, Oliver, I have a kernel with a debug patch (attached)
at (suse-only link):
https://build.suse.de/project/show/home:jirislaby:stable-drm
> And I don't find an imbalance between napi_disable() and napi_enable().
There is none, apparently (the warns never triggered). BUt still the
driver sucks wrt both power mgmt and dock plug/unplug. Since I am using
the patch (it upper-bounds the napi_disable loop count) and the udev
rule below, I can really use the nic.
$ cat /etc/udev/rules.d/10-disable-r8152-pm.rules
ACTION=="add", SUBSYSTEM=="usb", ATTR{idProduct}=="8153",
ATTR{idVendor}=="0bda", TEST=="power/control", ATTR{power/control}="on"
thanks,
--
js
suse labs
---
drivers/net/usb/r8152.c | 62 +++++++++++++++++++++++++++++++-----------------
net/core/dev.c | 14 +++++++++-
2 files changed, 53 insertions(+), 23 deletions(-)
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -704,6 +704,8 @@ struct r8152 {
unsigned long flags;
struct usb_device *udev;
struct napi_struct napi;
+ int napi_stat;
+ void *napi_last_en, *napi_last_dis;
struct usb_interface *intf;
struct net_device *netdev;
struct urb *intr_urb;
@@ -775,6 +777,31 @@ static unsigned int agg_buf_sz = 16384;
#define RTL_LIMITED_TSO_SIZE (agg_buf_sz - sizeof(struct tx_desc) - \
VLAN_ETH_HLEN - ETH_FCS_LEN)
+static void my_napi_enable(struct r8152 *tp)
+{
+ if (tp->napi_stat == 0) {
+ napi_enable(&tp->napi);
+ tp->napi_stat++;
+ tp->napi_last_en = (void *)_RET_IP_;
+ return;
+ }
+
+ WARN(1, "napi_stat=%d\n", tp->napi_stat);
+}
+
+static void my_napi_disable(struct r8152 *tp)
+{
+ if (tp->napi_stat == 1) {
+ napi_disable(&tp->napi);
+ tp->napi_stat--;
+ tp->napi_last_dis = (void *)_RET_IP_;
+ return;
+ }
+
+ WARN(1, "napi_stat=%d last_dis=%pF last_en=%pF\n",
+ tp->napi_stat, tp->napi_last_en, tp->napi_last_dis);
+}
+
static
int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
{
@@ -3787,7 +3814,6 @@ static bool rtl8153_in_nway(struct r8152
static void set_carrier(struct r8152 *tp)
{
struct net_device *netdev = tp->netdev;
- struct napi_struct *napi = &tp->napi;
u8 speed;
speed = rtl8152_get_speed(tp);
@@ -3796,12 +3822,12 @@ static void set_carrier(struct r8152 *tp
if (!netif_carrier_ok(netdev)) {
tp->rtl_ops.enable(tp);
netif_stop_queue(netdev);
- napi_disable(napi);
+ my_napi_disable(tp);
netif_carrier_on(netdev);
rtl_start_rx(tp);
clear_bit(RTL8152_SET_RX_MODE, &tp->flags);
_rtl8152_set_rx_mode(netdev);
- napi_enable(&tp->napi);
+ my_napi_enable(tp);
netif_wake_queue(netdev);
netif_info(tp, link, netdev, "carrier on\n");
} else if (netif_queue_stopped(netdev) &&
@@ -3811,9 +3837,9 @@ static void set_carrier(struct r8152 *tp
} else {
if (netif_carrier_ok(netdev)) {
netif_carrier_off(netdev);
- napi_disable(napi);
+ my_napi_disable(tp);
tp->rtl_ops.disable(tp);
- napi_enable(napi);
+ my_napi_enable(tp);
netif_info(tp, link, netdev, "carrier off\n");
}
}
@@ -3934,7 +3960,7 @@ static int rtl8152_open(struct net_devic
res);
goto out_unlock;
}
- napi_enable(&tp->napi);
+ my_napi_enable(tp);
mutex_unlock(&tp->control);
@@ -3962,7 +3988,7 @@ static int rtl8152_close(struct net_devi
#ifdef CONFIG_PM_SLEEP
unregister_pm_notifier(&tp->pm_notifier);
#endif
- napi_disable(&tp->napi);
+ my_napi_disable(tp);
clear_bit(WORK_ENABLE, &tp->flags);
usb_kill_urb(tp->intr_urb);
cancel_delayed_work_sync(&tp->schedule);
@@ -4230,7 +4256,7 @@ static int rtl8152_pre_reset(struct usb_
return 0;
netif_stop_queue(netdev);
- napi_disable(&tp->napi);
+ my_napi_disable(tp);
clear_bit(WORK_ENABLE, &tp->flags);
usb_kill_urb(tp->intr_urb);
cancel_delayed_work_sync(&tp->schedule);
@@ -4264,7 +4290,7 @@ static int rtl8152_post_reset(struct usb
mutex_unlock(&tp->control);
}
- napi_enable(&tp->napi);
+ my_napi_enable(tp);
netif_wake_queue(netdev);
usb_submit_urb(tp->intr_urb, GFP_KERNEL);
@@ -4302,10 +4328,8 @@ static int rtl8152_runtime_resume(struct
struct net_device *netdev = tp->netdev;
if (netif_running(netdev) && netdev->flags & IFF_UP) {
- struct napi_struct *napi = &tp->napi;
-
tp->rtl_ops.autosuspend_en(tp, false);
- napi_disable(napi);
+ my_napi_disable(tp);
set_bit(WORK_ENABLE, &tp->flags);
if (netif_carrier_ok(netdev)) {
@@ -4318,7 +4342,7 @@ static int rtl8152_runtime_resume(struct
}
}
- napi_enable(napi);
+ my_napi_enable(tp);
clear_bit(SELECTIVE_SUSPEND, &tp->flags);
smp_mb__after_atomic();
@@ -4388,13 +4412,11 @@ static int rtl8152_runtime_suspend(struc
tp->rtl_ops.autosuspend_en(tp, true);
if (netif_carrier_ok(netdev)) {
- struct napi_struct *napi = &tp->napi;
-
- napi_disable(napi);
+ my_napi_disable(tp);
rtl_stop_rx(tp);
rxdy_gated_en(tp, false);
ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, rcr);
- napi_enable(napi);
+ my_napi_enable(tp);
}
if (delay_autosuspend(tp)) {
@@ -4415,14 +4437,12 @@ static int rtl8152_system_suspend(struct
netif_device_detach(netdev);
if (netif_running(netdev) && test_bit(WORK_ENABLE, &tp->flags)) {
- struct napi_struct *napi = &tp->napi;
-
clear_bit(WORK_ENABLE, &tp->flags);
usb_kill_urb(tp->intr_urb);
- napi_disable(napi);
+ my_napi_disable(tp);
cancel_delayed_work_sync(&tp->schedule);
tp->rtl_ops.down(tp);
- napi_enable(napi);
+ my_napi_enable(tp);
}
return ret;
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5654,13 +5654,23 @@ EXPORT_SYMBOL(netif_napi_add);
void napi_disable(struct napi_struct *n)
{
+ int a;
+
might_sleep();
set_bit(NAPI_STATE_DISABLE, &n->state);
- while (test_and_set_bit(NAPI_STATE_SCHED, &n->state))
+ a = 0;
+ while (test_and_set_bit(NAPI_STATE_SCHED, &n->state)) {
msleep(1);
- while (test_and_set_bit(NAPI_STATE_NPSVC, &n->state))
+ if (WARN(a++ > 20 * 1000, "NAPI_STATE_SCHED never cleared"))
+ break;
+ }
+ a = 0;
+ while (test_and_set_bit(NAPI_STATE_NPSVC, &n->state)) {
msleep(1);
+ if (WARN(a++ > 20 * 1000, "NAPI_STATE_NPSVC never cleared"))
+ break;
+ }
hrtimer_cancel(&n->timer);
