Message-ID: <ea320070-a949-c737-22c4-14fd199fdc23@seco.com>
Date:   Mon, 14 Nov 2022 10:33:52 -0500
From:   Sean Anderson <sean.anderson@...o.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Tim Harvey <tharvey@...eworks.com>,
        netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: status of rate adaptation

On 11/12/22 08:15, Russell King (Oracle) wrote:
> On Fri, Nov 11, 2022 at 04:54:40PM -0500, Sean Anderson wrote:
>> > [    8.911932] mvpp2 f2000000.ethernet eth0: PHY
>> > [f212a600.mdio-mii:08] driver [Aquantia AQR113C] (irq=POLL)
>> > [    8.921577] mvpp2 f2000000.ethernet eth0: phy: 10gbase-r setting
>> > supported 00000000,00018000,000e706f advertising
>> > 00000000,00018000,000e706f
> 
>> > # ethtool eth0
>> > Settings for eth0:
>> >         Supported ports: [ ]
>> >         Supported link modes:   10baseT/Half 10baseT/Full
>> >                                 100baseT/Half 100baseT/Full
>> 
>> 10/100 half duplex aren't achievable with rate matching (and we avoid
>> turning them on), so they must be coming from somewhere else. I wonder
>> if this is because PHY_INTERFACE_MODE_SGMII is set in
>> supported_interfaces.
> 
> The reason is due to the way phylink_bringup_phy() works. This is
> being called with interface = 10GBASE-R, and the PHY is a C45 PHY,
> which means we call phy_get_rate_matching() with 
> PHY_INTERFACE_MODE_NA as we don't know whether the PHY will be
> switching its interface or not.
> 
> Looking at the Aquanta PHY driver, this will return that pause mode
> rate matching will be used, so config.rate_matching will be
> RATE_MATCH_PAUSE.
> 
> phylink_validate() will be called for PHY_INTERFACE_MODE_NA, which
> causes it to scan all supported interface modes (as again, we don't
> know which will be used by the PHY [*]) and the union of those
> results will be used.
> 
> So when we e.g. try SGMII mode, caps & mac_capabilities will allow
> the half duplex modes through.
> 
> Now for the bit marked with [*] - at this point, if rate matching is
> will be used, we in fact know which interface mode is going to be in
> operation, and it isn't going to change. So maybe we need this instead
> in phylink_bringup_phy():
> 
> -	if (phy->is_c45 &&
> +	config.rate_matching = phy_get_rate_matching(phy, interface);
> +	if (phy->is_c45 && config.rate_matching == RATE_MATCH_NONE &&
>             interface != PHY_INTERFACE_MODE_RXAUI &&
>             interface != PHY_INTERFACE_MODE_XAUI &&
>             interface != PHY_INTERFACE_MODE_USXGMII)
>                 config.interface = PHY_INTERFACE_MODE_NA;
>         else
>                 config.interface = interface;
> -	config.rate_matching = phy_get_rate_matching(phy, config.interface);
> 
>         ret = phylink_validate(pl, supported, &config);
> 
> ?

Yeah, that sounds reasonable. Actually, this was the logic I was
thinking of when I asked Tim to try USXGMII earlier. The funny thing is
that the comment above this implies that the link mode is never actually
(R)XAUI or USXGMII.

On another subject, if setting the SERDES mode field above fixes the
issue, then the Aquantia driver should be modified to set that field to
use a supported interface. Will host_interfaces work for this? It seems
to be set only when there's an SFP module.

That said, imagine if Tim was using a MAC without pause support, but
which supported SGMII and 10GBASE-R. Currently, we would just advertise
10G modes. But 1G could be supported by switching the phy interface. I
don't think this is supported right now. But if it were, we would need a
way to tell the phy to use a lower phy interface mode, instead of rate
adapting. We might also need a way to let the board tell us what
interfaces are supported (like [1]).

--Sean

[1] https://lore.kernel.org/netdev/20211117225050.18395-1-kabel@kernel.org/

Powered by blists - more mailing lists