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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Alexander,

On Mon, 31 Mar 2025 15:31:23 -0700
Alexander H Duyck <alexander.duyck@gmail.com> wrote:

> On Mon, 2025-03-31 at 18:20 +0200, Maxime Chevallier wrote:
> > On Mon, 31 Mar 2025 15:54:20 +0100
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:  
> 
> ...
> 
> > I was hoping Alexander could give option 1 a try, but let me know if
> > you think we should instead adopt option 2, which is probably the safer
> > on.
> > 
> > Maxime  
> 
> So I gave it a try, but the results weren't promising. I ended up
> getting the lp_advertised spammed with all the modes:
> 
>     Link partner advertised link modes:  100000baseKR4/Full
>                                          100000baseSR4/Full
>                                          100000baseCR4/Full
>                                          100000baseLR4_ER4/Full
>                                          100000baseKR2/Full
>                                          100000baseSR2/Full
>                                          100000baseCR2/Full
>                                          100000baseLR2_ER2_FR2/Full
>                                          100000baseDR2/Full
>                                          100000baseKR/Full
>                                          100000baseSR/Full
>                                          100000baseLR_ER_FR/Full
>                                          100000baseCR/Full
>                                          100000baseDR/Full

Thanks for testing, that's the expected outcome for the patch though. Is
that really an issue ? For fixed links, what report is bogus
anyways :/ I guess here as you mentioned, the problem is that you don't
actually have a real fixed link :)

> 
> In order to resolve it I just made the following change:
> @@ -713,9 +700,7 @@ static int phylink_parse_fixedlink(struct phylink
> *pl,
>                 phylink_warn(pl, "fixed link specifies half duplex for
> %dMbps link?\n",
>                              pl->link_config.speed);
>  
> -       linkmode_zero(pl->supported);
> -       phylink_fill_fixedlink_supported(pl->supported);
> -
> +       linkmode_fill(pl->supported);
>         linkmode_copy(pl->link_config.advertising, pl->supported);
>         phylink_validate(pl, pl->supported, &pl->link_config);
> 

So this goes back to the <10G modes reporting multiple modes then ?

> 
> Basically the issue is that I am using the pcs_validate to cleanup my
> link modes. So the code below this point worked correctly for me. The
> only issue was the dropping of the other bits.
> 
> That is why I mentioned the possibility of maybe adding some sort of
> follow-on filter function that would go through the upper bits and or
> them into the filter being run after the original one.
> 
> For example there is mask which is used to filter out everything but
> the pause and autoneg bits. Perhaps we should assemble bits there
> depending on the TP, FIBER, and BACKPLANE bits to clean out everything
> but CR, KR, and TP types if those bits are set.

Yeah but where do we get these TP / Fiber / Backplane bits from ? We
build that list of linkmodes from scratch in that function, only based
on speed/duplex, we don't know anything about the physical medium at
that point.

What you are suggesting is something I'm adding in the phy_port work
actually. You'll be able to say : "This port is a BaseK port" or
"BaseT" or "it may be baseC or baseL or baseS" and there'll be some
filtering based on that, although only in what we report to userspace,
at least for now :)

Maxime