| Message ID | 040eb6ae-bb21-2d27-14e0-b291cb4cc1c9@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] r8169: perform reset synchronously in __rtl8169_resume | expand |
Heiner Kallweit <hkallweit1@gmail.com> : [...] > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > index 75dfac024..1eb4f625a 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c > @@ -7327,9 +7327,9 @@ static void __rtl8169_resume(struct net_device *dev) > rtl_lock_work(tp); > napi_enable(&tp->napi); > set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags); > + if (netif_running(dev)) > + rtl_reset_work(tp); > rtl_unlock_work(tp); > - > - rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING); > } > > static int rtl8169_resume(struct device *device) netif_running() returns true before rtl_open(): issuing rtl_reset_work() synchronously against uninitialized rings right at the start of rtl_open() won't work as expected. pm_runtime_get_sync() could be issued later in rtl_open() (but I would not mind something different with runtime_{resume/suspend} in open/close).
Am 21.05.2018 um 23:24 schrieb Francois Romieu: > Heiner Kallweit <hkallweit1@gmail.com> : > [...] >> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c >> index 75dfac024..1eb4f625a 100644 >> --- a/drivers/net/ethernet/realtek/r8169.c >> +++ b/drivers/net/ethernet/realtek/r8169.c >> @@ -7327,9 +7327,9 @@ static void __rtl8169_resume(struct net_device *dev) >> rtl_lock_work(tp); >> napi_enable(&tp->napi); >> set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags); >> + if (netif_running(dev)) >> + rtl_reset_work(tp); >> rtl_unlock_work(tp); >> - >> - rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING); >> } >> >> static int rtl8169_resume(struct device *device) > > netif_running() returns true before rtl_open(): issuing rtl_reset_work() > synchronously against uninitialized rings right at the start of rtl_open() > won't work as expected. > rtl8169_runtime_resume() has a check for tp->TxDescArray at the beginning. Therefore it will return immediately before even calling __rtl8169_resume(), because tp->TxDescArray is set in rtl_open() only. So I don't think the described issue exists. However I have to admit that I wasn't aware that netif_running() is true already when entering rtl_open(), so thanks for the hint. > pm_runtime_get_sync() could be issued later in rtl_open() (but I would > not mind something different with runtime_{resume/suspend} in open/close). >
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Mon, 21 May 2018 19:01:19 +0200 > The driver uses pm_runtime_get_sync() in few places and relies on the > device being fully runtime-resumed after this call. So far however > the runtime resume callback triggers an asynchronous reset. > Avoid this and perform the reset synchronously. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/ethernet/realtek/r8169.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > index 75dfac024..1eb4f625a 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c > @@ -7327,9 +7327,9 @@ static void __rtl8169_resume(struct net_device *dev) > rtl_lock_work(tp); > napi_enable(&tp->napi); > set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags); > + if (netif_running(dev)) > + rtl_reset_work(tp); > rtl_unlock_work(tp); > - > - rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING); > } > Given what we know about ->ndo_open() and the checks by the callers of __rtl8169_resume(), the netif_running() test seems superfluous or wrong. Either way you need to resolve this somehow.
David Miller <davem@davemloft.net> : > From: Heiner Kallweit <hkallweit1@gmail.com> > Date: Mon, 21 May 2018 19:01:19 +0200 > > > The driver uses pm_runtime_get_sync() in few places and relies on the > > device being fully runtime-resumed after this call. So far however > > the runtime resume callback triggers an asynchronous reset. > > Avoid this and perform the reset synchronously. [...] > Given what we know about ->ndo_open() and the checks by the callers of > __rtl8169_resume(), the netif_running() test seems superfluous or > wrong. > > Either way you need to resolve this somehow. Actually I do not see why the asynchronous reset would be a problem. Aforementioned places that use pm_runtime_get_sync() are rtl_{open/close} 1. I understand that rtl_open needs to reach synchronously something like a D0 state but it does not need anything else, whence the current no-op in the driver runtime suspend/resume handlers (that I should have remembered btw). 2. rtl_close() needs the same D0 state to perform the hw counters update but it should neither need nor care about rtl_reset_work. rtl_close even disables itself the deferred work queue right after the hw counter update. If it's also worth making the code more palatable, more explicit symmetry between rtl8169_net_suspend and __rtl8169_resume would be welcome.
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 75dfac024..1eb4f625a 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -7327,9 +7327,9 @@ static void __rtl8169_resume(struct net_device *dev) rtl_lock_work(tp); napi_enable(&tp->napi); set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags); + if (netif_running(dev)) + rtl_reset_work(tp); rtl_unlock_work(tp); - - rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING); } static int rtl8169_resume(struct device *device)
The driver uses pm_runtime_get_sync() in few places and relies on the device being fully runtime-resumed after this call. So far however the runtime resume callback triggers an asynchronous reset. Avoid this and perform the reset synchronously. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)