On 30.09.2020 20:00, Petr Tesarik wrote:
> Hi Heiner again,
>
> On Wed, 30 Sep 2020 19:32:59 +0200
> Petr Tesarik <ptesa...@suse.cz> wrote:
>
>> Hi Heiner,
>>
>> On Wed, 30 Sep 2020 18:41:24 +0200
>> Petr Tesarik <ptesa...@suse.cz> wrote:
>>
>>> HI Heiner,
>>>
>>> On Wed, 30 Sep 2020 17:47:15 +0200
>>> Heiner Kallweit <hkallwe...@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 <ptesa...@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 <ptesa...@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
