On 30.09.2020 18:41, Petr Tesarik wrote:
> HI Heiner,
>
> On Wed, 30 Sep 2020 17:47:15 +0200
> Heiner Kallweit <hkallwe...@gmail.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)
>
