[PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume

[PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume

[Heiner Kallweit]

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

Re: [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume

[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.

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

Re: [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume

[Heiner Kallweit]

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).
> 

Re: [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume

[David Miller]

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.

Re: [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume

[Francois Romieu]

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