* [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume
@ 2018-05-21 17:01 Heiner Kallweit
2018-05-21 21:24 ` Francois Romieu
2018-05-22 17:59 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Heiner Kallweit @ 2018-05-21 17:01 UTC (permalink / raw)
To: David Miller, Realtek linux nic maintainers; +Cc: netdev
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);
}
static int rtl8169_resume(struct device *device)
--
2.17.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume
2018-05-21 17:01 [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume Heiner Kallweit
@ 2018-05-21 21:24 ` Francois Romieu
2018-05-21 22:38 ` Heiner Kallweit
2018-05-22 17:59 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Francois Romieu @ 2018-05-21 21:24 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: David Miller, Realtek linux nic maintainers, netdev
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).
--
Ueimor
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume
2018-05-21 21:24 ` Francois Romieu
@ 2018-05-21 22:38 ` Heiner Kallweit
0 siblings, 0 replies; 5+ messages in thread
From: Heiner Kallweit @ 2018-05-21 22:38 UTC (permalink / raw)
To: Francois Romieu; +Cc: David Miller, Realtek linux nic maintainers, netdev
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).
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume
2018-05-21 17:01 [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume Heiner Kallweit
2018-05-21 21:24 ` Francois Romieu
@ 2018-05-22 17:59 ` David Miller
2018-05-22 22:51 ` Francois Romieu
1 sibling, 1 reply; 5+ messages in thread
From: David Miller @ 2018-05-22 17:59 UTC (permalink / raw)
To: hkallweit1; +Cc: nic_swsd, netdev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume
2018-05-22 17:59 ` David Miller
@ 2018-05-22 22:51 ` Francois Romieu
0 siblings, 0 replies; 5+ messages in thread
From: Francois Romieu @ 2018-05-22 22:51 UTC (permalink / raw)
To: hkallweit1; +Cc: David Miller, nic_swsd, netdev
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.
--
Ueimor
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-05-22 22:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-21 17:01 [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume Heiner Kallweit
2018-05-21 21:24 ` Francois Romieu
2018-05-21 22:38 ` Heiner Kallweit
2018-05-22 17:59 ` David Miller
2018-05-22 22:51 ` Francois Romieu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.