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] [day] [month] [year] [list]

Message-ID: <c536f0bf-d271-ec89-d219-69cd19c28c65@gmail.com>
Date:   Thu, 15 Aug 2019 18:02:32 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...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 15.08.2019 17:43, Florian Fainelli wrote:
> 
> 
> 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
> 
Not related to this patch, but to EEE support in general:

There's something in the back of my mind to create linkmodes for all
EEE modes. They could be used with the normal supported, advertising,
and lp_advertising bitmaps. Means:
- extend genphy_read_abilities to read supported EEE modes
- extend genphy_read_status to read lp-advertised EEE modes
- let genphy_config_aneg handle EEE advertising
- ..
This should help to make EEE mode handling consistent with link mode
handling.
Also still missing is support for the new C45 registers for 2.5Gbps and
5Gbps EEE (3.21, 7.62, 7.63).

Open for me is how to deal best with the scenario that older PHY's
use the C22 MMD registers for proprietary purposes. Writing to the
MMD address register then may cause misbehavior of the PHY (that was
the case for RTL8211B), and MMD reads may return random data.
Maybe we need a flag to explicitly state whether MMD is supported
or not.

Heiner