Date:   Wed, 30 Sep 2020 19:13:57 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Petr Tesarik <ptesarik@...e.cz>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        netdev@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: RTL8402 stops working after hibernate/resume

On 30.09.2020 18:41, Petr Tesarik wrote:
> HI Heiner,
> 
> On Wed, 30 Sep 2020 17:47:15 +0200
> Heiner Kallweit <hkallweit1@...il.com> wrote:
> 
>> On 30.09.2020 11:04, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 9/29/20 10:35 PM, Heiner Kallweit wrote:  
>>>> On 29.09.2020 22:08, Hans de Goede wrote:  
>>>
>>> <snip>
>>>   
>>>>> Also some remarks about this while I'm being a bit grumpy about
>>>>> all this anyways (sorry):
>>>>>
>>>>> 1. 9f0b54cd167219 ("r8169: move switching optional clock on/off
>>>>> to pll power functions") commit's message does not seem to really
>>>>> explain why this change was made...
>>>>>
>>>>> 2. If a git blame would have been done to find the commit adding
>>>>> the clk support: commit c2f6f3ee7f22 ("r8169: Get and enable optional ether_clk clock")
>>>>> then you could have known that the clk in question is an external
>>>>> clock for the entire chip, the commit message pretty clearly states
>>>>> this (although "the entire" part is implied only) :
>>>>>
>>>>> "On some boards a platform clock is used as clock for the r8169 chip,
>>>>> this commit adds support for getting and enabling this clock (assuming
>>>>> it has an "ether_clk" alias set on it).
>>>>>  
>>>> Even if the missing clock would stop the network chip completely,
>>>> this shouldn't freeze the system as described by Petr.
>>>> In some old RTL8169S spec an external 25MHz clock is mentioned,
>>>> what matches the MII bus frequency. Therefore I'm not 100% convinced
>>>> that the clock is needed for basic chip operation, but due to
>>>> Realtek not releasing datasheets I can't verify this.  
>>>
>>> Well if that 25 MHz is the only clock the chip has, then it basically
>>> has to be on all the time since all clocked digital ASICs cannot work
>>> without a clock.  Now pci-e is a packet-switched point-to-point bus,
>>> so the ethernet chip not working should not freeze the entire system,
>>> but I'm not really surprised that even though it should not do that,
>>> that it still does.
>>>   
>>>> But yes, if reverting this change avoids the issue on Petr's system,
>>>> then we should do it. A simple mechanical revert wouldn't work because
>>>> source file structure has changed since then, so I'll prepare a patch
>>>> that effectively reverts the change.  
>>>
>>> Great, thank you.
>>>
>>> Regards,
>>>
>>> Hans
>>>   
>>
>> 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. ;-)
> 
This would be perfect and much appreciated!


>> Please let me know whether it fixes the freeze, then I'll add your Tested-by.
> 
> Will do.
> 
>> 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.
> 
> 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)
> 

Powered by blists - more mailing lists