Message-ID: <bbe1c983-788f-0561-a897-53f2ab4508df@gmail.com>
Date:   Mon, 6 Dec 2021 09:06:53 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Tom Lendacky <thomas.lendacky@....com>
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        George McCollister <george.mccollister@...il.com>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to
 phylink_generic_validate()

On 12/4/21 6:52 AM, Russell King (Oracle) wrote:
> On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
>> On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote:
>>> Now, with respect to the fixed link ports reporting 1000baseKX/Full this is
>>> introduced by switching to your patch, it works before and it "breaks"
>>> after.
>>>
>>> The first part that is a bit weird is that we seem to be calling
>>> phylink_generic_validate() twice in a row from the same call site.
>>>
>>> For fixed link ports, instead of masking with what the fixed link actually
>>> supports, we seem to be using a supported mask which is all 1s which seems a
>>> bit excessive for a fixed link.
>>>
>>> This is an excerpt with the internal PHY:
>>>
>>> [    4.210890] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized):
>>> Calling phylink_generic_validate
>>> [    4.220063] before phylink_get_linkmodes: 0000000,00000000,00010fc0
>>> [    4.226357] phylink_get_linkmodes: caps: 0xffffffff mac_capabilities:
>>> 0xff
>>> [    4.233258] after phylink_get_linkmodes: c000018,00000200,00036fff
>>> [    4.239463] before anding supported with mask: 0000000,00000000,000062ff
>>> [    4.246189] after anding supported with mask: 0000000,00000000,000062ff
>>> [    4.252829] before anding advertising with mask:
>>> c000018,00000200,00036fff
>>> [    4.259729] after anding advertising with mask: c000018,00000200,00036fff
>>> [    4.266546] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): PHY
>>> [f0b403c0.mdio--1:05] driver [Broadcom BCM7445] (irq=POLL)
>>>
>>> and this is what a fixed link port looks like:
>>>
>>> [    4.430765] brcm-sf2 f0b00000.ethernet_switch rgmii_2 (uninitialized):
>>> Calling phylink_generic_validate
>>> [    4.440205] before phylink_get_linkmodes: 0000000,00000000,00010fc0
>>> [    4.446500] phylink_get_linkmodes: caps: 0xff mac_capabilities: 0xff
>>> [    4.452880] after phylink_get_linkmodes: c000018,00000200,00036fff
>>> [    4.459085] before anding supported with mask: fffffff,ffffffff,ffffffff
>>> [    4.465811] after anding supported with mask: c000018,00000200,00036fff
>>> [    4.472450] before anding advertising with mask:
>>> c000018,00000200,00036fff
>>> [    4.479349] after anding advertising with mask: c000018,00000200,00036fff
>>>
>>> or maybe the problem is with phylink_get_ksettings... ran out of time
>>> tonight to look further into it.
>>
>> It will be:
>>
>>         s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
>>                                pl->supported, true);
>>         linkmode_zero(pl->supported);
>>         phylink_set(pl->supported, MII);
>>         phylink_set(pl->supported, Pause);
>>         phylink_set(pl->supported, Asym_Pause);
>>         phylink_set(pl->supported, Autoneg);
>>         if (s) {
>>                 __set_bit(s->bit, pl->supported);
>>                 __set_bit(s->bit, pl->link_config.lp_advertising);
>>
>> Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting()
>> returns the first entry it finds in the supported table:
>>
>>         /* 1G */
>>         PHY_SETTING(   1000, FULL,   1000baseKX_Full            ),
>>         PHY_SETTING(   1000, FULL,   1000baseT_Full             ),
>>         PHY_SETTING(   1000, HALF,   1000baseT_Half             ),
>>         PHY_SETTING(   1000, FULL,   1000baseT1_Full            ),
>>         PHY_SETTING(   1000, FULL,   1000baseX_Full             ),
>>
>> Consequently, 1000baseKX_Full is preferred over 1000baseT_Full.
>>
>> Fixed links don't specify their underlying technology, only the speed
>> and duplex, so going from speed and duplex to an ethtool link mode is
>> not easy. I suppose we could drop 1000baseKX_Full from the supported
>> bitmap in phylink_parse_fixedlink() before the first phylink_validate()
>> call. Alternatively, the table could be re-ordered. It was supposed to
>> be grouped by speed and sorted in descending match priority as specified
>> by the comment above the table. Does it really make sense that
>> 1000baseKX_Full is supposed to be preferred over all the other 1G
>> speeds? I suppose that's a question for Tom Lendacky
>> <thomas.lendacky@....com>, who introduced this in 3e7077067e80
>> ("phy: Expand phy speed/duplex settings array") back in 2014.
> 
> Here's a patch for one of my suggestions above. Tom, I'd appreciate
> if you could look at this please. Thanks.

I don't have objections on the patch per-se, but I am still wary that
this is going to break another driver in terms of what its fixed link
ports are supposed to report, so maybe the generic validation approach
needs to be provided some additional hints as to what port link modes
are supported, or rather, not supported.

So I would suggest we have bcm_sf2 continue to implement
ds->ops->validate which does call phylink_generic_validate() but also
prunes unsupported link modes for its fixed link ports, what do you think?
-- 
Florian

Powered by blists - more mailing lists