netdev - Re: RTL8402 stops working after hibernate/resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3132f91a-505d-6e56-be97-334593f8ca12@gmail.com>
Date:   Tue, 29 Sep 2020 21:41:31 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Petr Tesarik <ptesarik@...e.cz>,
        Hans de Goede <hdegoede@...hat.com>
Cc:     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 29.09.2020 21:07, Petr Tesarik wrote:
> Hi Heiner (and now also Hans)!
> 
> @Hans: I'm adding you to this conversation, because you're the author
> of commit b1e3454d39f99, which seems to break the r8169 driver on a
> laptop of mine.
> 
> On Fri, 25 Sep 2020 16:47:54 +0200
> Heiner Kallweit <hkallweit1@...il.com> wrote:
> 
>> On 25.09.2020 14:56, Petr Tesarik wrote:
>>> On Fri, 25 Sep 2020 11:52:41 +0200
>>> Petr Tesarik <ptesarik@...e.cz> wrote:
>>>   
>>>> On Fri, 25 Sep 2020 11:44:09 +0200
>>>> Heiner Kallweit <hkallweit1@...il.com> wrote:
>>>>  
>>>>> On 25.09.2020 10:54, Petr Tesarik wrote:    
>>>> [...]  
>>>>>> Does it make sense to bisect the change that broke the driver for me, or should I rather dispose of this waste^Wlaptop in an environmentally friendly manner? I mean, would you eventually accept a workaround for a few machines with a broken BIOS?
>>>>>>       
>>>>> If the workaround is small and there's little chance to break other stuff: then usually yes.
>>>>> If you can spend the effort to bisect the issue, this would be appreciated.    
>>>>
>>>> OK, then I'm going to give it a try.  
>>>
>>> Done. The system freezes when this commit is applied:
>>>
>>> commit 9f0b54cd167219266bd3864570ae8f4987b57520
>>> Author: Heiner Kallweit <hkallweit1@...il.com>
>>> Date:   Wed Jun 17 22:55:40 2020 +0200
>>>
>>>     r8169: move switching optional clock on/off to pll power functions
>>>   
>> This sounds weird. On your system tp->clk should be NULL, making
>> clk_prepare_enable() et al no-ops. Please check whether tp->clk
>> is NULL after the call to rtl_get_ether_clk().
> 
> This might be part of the issue. On my system tp->clk is definitely not
> NULL:
> 
OK, interesting. The referenced patch changes driver behavior in a way
that ether_clk is disabled again in probe(), and only re-enabled
in ndo_open(). This should be helpful from a power-saving perspective
because before that we enabled the clock even if the network device
wasn't used.
It seems however that disabling ether_clk has (at least on your system)
side effects causing a system freeze. That's a best guess from my side
however, and not really something I can comment on.

> crash> *rtl8169_private.clk 0xffff9277aca58940
>   clk = 0xffff9277ac2c82a0
> 
> crash> *clk 0xffff9277ac2c82a0
> struct clk {
>   core = 0xffff9277aef65d00, 
>   dev = 0xffff9277aed000b0, 
>   dev_id = 0xffff9277aec60c00 "0000:03:00.2", 
>   con_id = 0xffff9277ad04b080 "ether_clk", 
>   min_rate = 0, 
>   max_rate = 18446744073709551615, 
>   exclusive_count = 0, 
>   clks_node = {
>     next = 0xffff9277ad2428d8, 
>     pprev = 0xffff9277aef65dc8
>   }
> }
> 
> The dev_id corresponds to the Ethernet controller:
> 
> 03:00.2 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL810xE PCI Express Fast Ethernet controller (rev 06)
> 
> Looking at clk_find(), it matches this entry in clocks:
> 
> struct clk_lookup {
>   node = {
>     next = 0xffffffffbc702f40, 
>     prev = 0xffff9277bf7190c0
>   }, 
>   dev_id = 0x0, 
>   con_id = 0xffff9277bf719524 "ether_clk", 
>   clk = 0x0, 
>   clk_hw = 0xffff9277ad2427f8
> }
> 
> That's because this kernel is built with CONFIG_PMC_ATOM=y, and looking
> at the platform initialization code, the "ether_clk" alias is created
> unconditionally. Hans already added.
> 
> Petr T
>