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

[Heiner Kallweit <hkallweit1@gmail.com>]

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