Re: RTL8402 stops working after hibernate/resume

[Heiner Kallweit <hkallweit1@gmail.com>]

On 30.09.2020 20:00, Petr Tesarik wrote:
> Hi Heiner again,
> 
> On Wed, 30 Sep 2020 19:32:59 +0200
> Petr Tesarik <ptesarik@suse.cz> wrote:
> 
>> Hi Heiner,
>>
>> On Wed, 30 Sep 2020 18:41:24 +0200
>> Petr Tesarik <ptesarik@suse.cz> wrote:
>>
>>> HI Heiner,
>>>
>>> On Wed, 30 Sep 2020 17:47:15 +0200
>>> Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>> [...]  
>>>> Petr,
>>>> in the following I send two patches. First one is supposed to fix the freeze.
>>>> It also fixes another issue that existed before my ether_clk change:
>>>> ether_clk was disabled on suspend even if WoL is enabled. And the network
>>>> chip most likely needs the clock to check for WoL packets.    
>>>
>>> Should I also check whether WoL works? Plus, it would be probably good
>>> if I could check whether it indeed didn't work before the change. ;-)
>>>   
>>>> Please let me know whether it fixes the freeze, then I'll add your
>>>>   Tested-by.    
>>>
>>> Will do.  
>>
>> Here are the results:
>>
>> - WoL does not work without your patch; this was tested with 5.8.4,
>>   because master freezes hard on load.
>>
>> - I got a kernel crash when I woke up the laptop through keyboard; I am
>>   not sure if it is related, although I could spend some time looking
>>   at the resulting crash dump if you think it's worth the time.
>>

This may be caused by the following code in the resume path in 5.8.
The chip is accessed before the clock gets enabled.

rtl_rar_set(tp, tp->dev->dev_addr);
clk_prepare_enable(tp->clk);


>> - After applying your first patch on top of v5.9-rc6, the driver can be
>>   loaded, but stops working properly on suspend/resume.
>>
>> - WoL still does not work, but I'm no longer getting a kernel crash at
>>   least. ;-)
>>
>> Tested-by: Petr Tesarik <ptesarik@suse.com>
>>
>>>> Second patch is a re-send of the one I sent before, it should fix
>>>> the rx issues after resume from suspend for you.    
>>>
>>> All right, going to rebuild the kernel and see how it goes.  
>>
>> This second patch does not fix suspend/resume for me, unfortunately. The
>> receive is still erratic, but the behaviour is now different: some
>> packets are truncated, other packets are delayed by approx. 1024 ms.
>> Yes, 1024 may ring a bell, but I've no idea which.
> 
> I'm sorry, I added some debugging message, and as I was wondering why
> the resume path is not run, I found out I was not properly reloading the
> modified module. So, after fixing my build, your patch also fixes the
> Rx queue on resume! Both with and without NetworkManager.
> 
> So, you can also add to the second patch:
> 
> Tested-by: Petr Tesarik <ptesarik@suse.com>
> 
Thanks a lot for all your efforts!

> WoL still does not work on my laptop, but this might be an unrelated
> issue, and I can even imagine the BIOS is buggy in this regard.
> 
A simple further check you could do:
After sending the WoL packet (that doesn't wake the system) you wake
the system by e.g. a keystroke. Then check in /proc/interrupts for
a PCIe PME interrupt. If there's a PME interrupt, then the network
chip successfully detected the WoL packet, and it seems we have to
blame the BIOS.

> Petr T
> 
>> HTH,
>> Petr T
>>
>>> Stay tuned,
>>> Petr T
>>>
>>>   
>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>>>>   b/drivers/net/ethernet/realtek/r8169_main.c index
>>>>   6c7c004c2..72351c5b0 100644 ---
>>>>   a/drivers/net/ethernet/realtek/r8169_main.c +++
>>>>   b/drivers/net/ethernet/realtek/r8169_main.c @@ -2236,14 +2236,10
>>>>   @@ static void rtl_pll_power_down(struct rtl8169_private *tp)
>>>>   default: break;
>>>>  	}
>>>> -
>>>> -	clk_disable_unprepare(tp->clk);
>>>>  }
>>>>  
>>>>  static void rtl_pll_power_up(struct rtl8169_private *tp)
>>>>  {
>>>> -	clk_prepare_enable(tp->clk);
>>>> -
>>>>  	switch (tp->mac_version) {
>>>>  	case RTL_GIGA_MAC_VER_25 ... RTL_GIGA_MAC_VER_33:
>>>>  	case RTL_GIGA_MAC_VER_37:
>>>> @@ -4820,29 +4816,39 @@ static void rtl8169_net_suspend(struct
>>>>   rtl8169_private *tp) 
>>>>  #ifdef CONFIG_PM
>>>>  
>>>> +static int rtl8169_net_resume(struct rtl8169_private *tp)
>>>> +{
>>>> +	rtl_rar_set(tp, tp->dev->dev_addr);
>>>> +
>>>> +	if (tp->TxDescArray)
>>>> +		rtl8169_up(tp);
>>>> +
>>>> +	netif_device_attach(tp->dev);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static int __maybe_unused rtl8169_suspend(struct device *device)
>>>>  {
>>>>  	struct rtl8169_private *tp = dev_get_drvdata(device);
>>>>  
>>>>  	rtnl_lock();
>>>>  	rtl8169_net_suspend(tp);
>>>> +	if (!device_may_wakeup(tp_to_dev(tp)))
>>>> +		clk_disable_unprepare(tp->clk);
>>>>  	rtnl_unlock();
>>>>  
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static int rtl8169_resume(struct device *device)
>>>> +static int __maybe_unused rtl8169_resume(struct device *device)
>>>>  {
>>>>  	struct rtl8169_private *tp = dev_get_drvdata(device);
>>>>  
>>>> -	rtl_rar_set(tp, tp->dev->dev_addr);
>>>> -
>>>> -	if (tp->TxDescArray)
>>>> -		rtl8169_up(tp);
>>>> +	if (!device_may_wakeup(tp_to_dev(tp)))
>>>> +		clk_prepare_enable(tp->clk);
>>>>  
>>>> -	netif_device_attach(tp->dev);
>>>> -
>>>> -	return 0;
>>>> +	return rtl8169_net_resume(tp);
>>>>  }
>>>>  
>>>>  static int rtl8169_runtime_suspend(struct device *device)
>>>> @@ -4868,7 +4874,7 @@ static int rtl8169_runtime_resume(struct
>>>>   device *device) 
>>>>  	__rtl8169_set_wol(tp, tp->saved_wolopts);
>>>>  
>>>> -	return rtl8169_resume(device);
>>>> +	return rtl8169_net_resume(tp);
>>>>  }
>>>>  
>>>>  static int rtl8169_runtime_idle(struct device *device)    
>>>   
>>
> 
Heiner