netdev - Re: [PATCH net-next 2/2] r8169: use the generic EEE management functions

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: <24146e48-c498-d13a-8c12-76519455d0d4@gmail.com>
Date:   Thu, 15 Aug 2019 08:43:30 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] r8169: use the generic EEE management
 functions



On 8/15/2019 6:02 AM, Heiner Kallweit wrote:
> On 15.08.2019 14:35, Andrew Lunn wrote:
>> On Thu, Aug 15, 2019 at 11:47:33AM +0200, Heiner Kallweit wrote:
>>> Now that the Realtek PHY driver maps the vendor-specific EEE registers
>>> to the standard MMD registers, we can remove all special handling and
>>> use the generic functions phy_ethtool_get/set_eee.
>>
>> Hi Heiner
>>
> Hi Andrew,
> 
>> I think you should also add a call the phy_init_eee()?
>>
> I think it's not strictly needed. And few things regarding
> phy_init_eee are not fully clear to me:
> 
> - When is it supposed to be called? Before each call to
>   phy_ethtool_set_eee? Or once in the drivers init path?
> 
> - The name is a little bit misleading as it's mainly a
>   validity check. An actual "init" is done only if
>   parameter clk_stop_enable is set.
> 
> - It returns -EPROTONOSUPPORT if at least one link partner
>   doesn't advertise EEE for current speed/duplex. To me this
>   seems to be too restrictive. Example:
>   We're at 1Gbps/full and link partner advertises EEE for
>   100Mbps only. Then phy_init_eee returns -EPROTONOSUPPORT.
>   This keeps me from controlling 100Mbps EEE advertisement.  

That function needs a complete rework, it does not say what its name
implies, and there is an assumption that you have already locally
advertised EEE for it to work properly, that does not make any sense
since the whole purpose is to see whether EEE can/will be active with
the link partner (that's how I read it at least).

Regarding whether the clock stop enable can be turned on or off is also
a bit suspicious, because a MAC driver does not know whether the PHY
supports doing that, I had started something in that area years ago:

https://github.com/ffainelli/linux/commits/phy-eee-tx-clk
-- 
Florian