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]

Date:   Fri, 5 Apr 2019 22:32:12 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        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 05.04.2019 22:27, Andrew Lunn wrote:
> On Fri, Apr 05, 2019 at 10:04:22PM +0200, Heiner Kallweit wrote:
>> On 05.04.2019 21:48, Andrew Lunn wrote:
>>> On Fri, Apr 05, 2019 at 09:23:13PM +0200, Heiner Kallweit wrote:
>>>> genphy_read_status() so far checks phydev->supported, not the actual
>>>> PHY capabilities. This can make a difference if the supported speeds
>>>> have been limited by of_set_phy_supported() or phy_set_max_speed().
>>>>
>>>> It seems that this issue only affects the link partner advertisements
>>>> as displayed by ethtool. Also this patch wouldn't apply to older
>>>> kernels because linkmode bitmaps have been introduced recently.
>>>> Therefore net-next.
>>>
>>> Hi Heiner
>>>
>> Hi Andrew,
>>
>>> Are you saying, if the local PHY/MAC does not support 1000Base-T, we
>>> should not check if the peer is advertising 1000Base-T? That seems
>>> wrong. We should report everything it offers. The fact we cannot make
>>> use of 1000Base-T should not matter, and resolving of the auto-get
>>> should do the right thing when presented with modes it cannot use.
>>>
>> Link partner gigabit advertisement is reported in register MII_STAT1000.
>> If we have a 100MBit PHY w/o this register, then we will never know
>> whether the link partner supports gigabit.
> 
> Hi Heiner
> 
> There seems to be two uses cases:
> 
> A Fast MAC connected to a Fast PHY. In that case, MII_STAT1000
> probably does not exist, so we should not read it. Hopefully .features
> says BASIC, or if we ask the PHY what is it, it reports it is a Fast
> PHY.
> 
Right. BMSR_ESTATEN should not be set on a Fast PHY. Handling of this
case didn't change.

> A Fast MAC connected to a Giga PHY. The MAC driver will of used
> phy_set_max_speed() to indicate its limits. In that case, MII_STAT1000
> does exist and we should report what the peer is advertising.
> 
That's what we're doing now with this patch.

>       Andrew
> 
Heiner