Re: [PATCH net-next v4 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Maxime Chevallier]

On Thu, 6 Mar 2025 12:51:16 +0000
"Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx> wrote:

> On Thu, Mar 06, 2025 at 11:12:20AM +0100, Maxime Chevallier wrote:
> > On Thu, 6 Mar 2025 09:56:32 +0100
> > Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
> >   
> > > On 3/3/25 10:03 AM, Maxime Chevallier wrote:  
> > > > @@ -879,8 +880,10 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> > > >  	linkmode_copy(pl->link_config.advertising, pl->supported);
> > > >  	phylink_validate(pl, pl->supported, &pl->link_config);
> > > >  
> > > > -	s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
> > > > -			       pl->supported, true);
> > > > +	c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
> > > > +			    pl->supported, true);
> > > > +	if (c)
> > > > +		linkmode_and(match, pl->supported, c->linkmodes);    
> > > 
> > > How about using only the first bit from `c->linkmodes`, to avoid
> > > behavior changes?  
> > 
> > If what we want is to keep the exact same behaviour, then we need to
> > go one step further and make sure we keep the same one as before, and
> > it's not guaranteed that the first bit in c->linkmodes is this one.
> > 
> > We could however have a default supported mask for fixed-link in phylink
> > that contains all the linkmodes we allow for fixed links, then filter
> > with the lookup, something like :
> > 
> > 
> > -       linkmode_fill(pl->supported);
> > +       /* (in a dedicated helper) Linkmodes reported for fixed links below
> > +        * 10G */
> > +       linkmode_zero(pl->supported);
> > +
> > +       linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, pl->supported);
> > +       linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, pl->supported);
> > +       linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, pl->supported);
> > +       linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, pl->supported);
> > +       linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, pl->supported);
> > +       linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, pl->supported);
> > +       linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, pl->supported);
> > +       linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, pl->supported);
> > +       linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, pl->supported);  
> 
> Good idea, but do we have some way to automatically generate the baseT
> link modes?

I think we could with some of the preliminary phy_port patches I had
sent before going into that phy_caps series :

https://lore.kernel.org/netdev/20250213101606.1154014-2-maxime.chevallier@xxxxxxxxxxx/

It adds the information about medium, maybe we could adapt that, making
sure we filter out BaseT1 for example, but that would be a generic way
of generating that list indeed.

I don't necessarily mean to add this "mediums" thing into this series
right now, we could for now set that list of all BaseT modes in an
internal helper, then later on convert it to the mediums-based
linkmodes listing. I'll go back to phy_ports after phy_caps :)

Thanks,

Maxime