netdev - Re: [PATCH net-next] net: phy: improve link partner capability detection

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: <36b1ecfb-4cde-9c48-3d1b-6304418991f8@gmail.com>
Date:   Fri, 5 Apr 2019 14:54:37 -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] net: phy: improve link partner capability
 detection

On 4/5/19 2:52 PM, Heiner Kallweit wrote:
> On 05.04.2019 23:38, Heiner Kallweit wrote:
>> On 05.04.2019 23:27, Andrew Lunn wrote:
>>>> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
>>>> +			      phydev->supported))
>>>> +		phydev->is_gigabit_capable = 1;
>>>> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
>>>> +			      phydev->supported))
>>>> +		phydev->is_gigabit_capable = 1;
>>>> +
>>>
>>> What i'm trying to get at is, why do we need this bit of the patch?
>>> Why do we need this flag? The hardware should tell us if it can do
>>> gigabit.
>>>
>> The code to query BMSR_ESTATEN and MII_ESTATUS is in genphy_read_status.
>> However we also have to cover the case that this function isn't used.
>> Therefore I query phydev->supported before the speed could be limited.
>> (relying on the PHY driver not lying about gigabit capability)
>> This part of the patch is directly before of_set_phy_supported().
>>
>> I just see that we can re-use is_gigabit_capable also in
>> genphy_config_advert.
>>
>> Of course I can read in every place the hardware for gigabit support.
>> But IMO this creates unnecessary code duplication.
>>
> I just see that we can reuse is_gigabit_capable also in
> genphy_config_advert().
> 
> And when checking occurrences of BMSR_ESTATEN there seems to be more
> work waiting: In swphy BMSR_ESTATEN is set, but MII_ESTATUS isn't
> configured. It just works by chance becausing reading this register
> returns the default 0xffff.

It is not clear to me what we are trying to optimize for here, given
everything is pretty much a slow path anyway. I am concerned though
about mis-behaving PHYs where caching of BMSR_ESTATEN could result in
incorrect behaviors, I don't have any data to back that claim, but
reading the registers should always be the safest way to determine what
HW is capable of (famous last words). Not feeling strongly about either
direction though...
-- 
Florian