When phylink creates a fixed-link configuration, it finds a matching
linkmode to set as the advertised, lp_advertising and supported modes
based on the speed and duplex of the fixed link.
Use the newly introduced phy_caps_lookup to get these modes instead of
phy_lookup_settings(). This has the side effect that the matched
settings and configured linkmodes may now contain several linkmodes (the
intersection of supported linkmodes from the phylink settings and the
linkmodes that match speed/duplex) instead of the one from
phy_lookup_settings().
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
1 file changed, 31 insertions(+), 13 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index cf9f019382ad..8e2b7d647a92 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
return phylink_validate_mac_and_pcs(pl, supported, state);
}
+static void phylink_fill_fixedlink_supported(unsigned long *supported)
+{
+ linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
+}
+
static int phylink_parse_fixedlink(struct phylink *pl,
const struct fwnode_handle *fwnode)
{
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(match) = { 0, };
__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+ const struct link_capabilities *c;
struct fwnode_handle *fixed_node;
- const struct phy_setting *s;
struct gpio_desc *desc;
u32 speed;
int ret;
@@ -875,12 +889,16 @@ 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_fill(pl->supported);
+ linkmode_zero(pl->supported);
+ phylink_fill_fixedlink_supported(pl->supported);
+
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);
linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
@@ -889,9 +907,10 @@ static int phylink_parse_fixedlink(struct phylink *pl,
phylink_set(pl->supported, MII);
- if (s) {
- __set_bit(s->bit, pl->supported);
- __set_bit(s->bit, pl->link_config.lp_advertising);
+ if (c) {
+ linkmode_or(pl->supported, pl->supported, match);
+ linkmode_or(pl->link_config.lp_advertising,
+ pl->link_config.lp_advertising, match);
} else {
phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
@@ -1879,21 +1898,20 @@ static int phylink_register_sfp(struct phylink *pl,
int phylink_set_fixed_link(struct phylink *pl,
const struct phylink_link_state *state)
{
- const struct phy_setting *s;
+ const struct link_capabilities *c;
unsigned long *adv;
if (pl->cfg_link_an_mode != MLO_AN_PHY || !state ||
!test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
return -EINVAL;
- s = phy_lookup_setting(state->speed, state->duplex,
- pl->supported, true);
- if (!s)
+ c = phy_caps_lookup(state->speed, state->duplex,
+ pl->supported, true);
+ if (!c)
return -EINVAL;
adv = pl->link_config.advertising;
- linkmode_zero(adv);
- linkmode_set_bit(s->bit, adv);
+ linkmode_and(adv, pl->supported, c->linkmodes);
linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, adv);
pl->link_config.speed = state->speed;
--
2.48.1On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> When phylink creates a fixed-link configuration, it finds a matching
> linkmode to set as the advertised, lp_advertising and supported modes
> based on the speed and duplex of the fixed link.
>
> Use the newly introduced phy_caps_lookup to get these modes instead of
> phy_lookup_settings(). This has the side effect that the matched
> settings and configured linkmodes may now contain several linkmodes (the
> intersection of supported linkmodes from the phylink settings and the
> linkmodes that match speed/duplex) instead of the one from
> phy_lookup_settings().
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> 1 file changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index cf9f019382ad..8e2b7d647a92 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> return phylink_validate_mac_and_pcs(pl, supported, state);
> }
>
> +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> +{
> + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> +}
> +
Any chance we can go with a different route here than just locking
fixed mode to being only for BaseT configurations?
I am currently working on getting the fbnic driver up and running and I
am using fixed-link mode as a crutch until I can finish up enabling
QSFP module support for phylink and this just knocked out the supported
link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
Seems like this should really be something handled by some sort of
validation function rather than just forcing all devices using fixed
link to assume that they are BaseT. I know in our direct attached
copper case we aren't running autoneg so that plan was to use fixed
link until we can add more flexibility by getting QSFP support going.
On Thu, Mar 27, 2025 at 06:16:00PM -0700, Alexander H Duyck wrote:
> On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > When phylink creates a fixed-link configuration, it finds a matching
> > linkmode to set as the advertised, lp_advertising and supported modes
> > based on the speed and duplex of the fixed link.
> >
> > Use the newly introduced phy_caps_lookup to get these modes instead of
> > phy_lookup_settings(). This has the side effect that the matched
> > settings and configured linkmodes may now contain several linkmodes (the
> > intersection of supported linkmodes from the phylink settings and the
> > linkmodes that match speed/duplex) instead of the one from
> > phy_lookup_settings().
> >
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> > drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > 1 file changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index cf9f019382ad..8e2b7d647a92 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > return phylink_validate_mac_and_pcs(pl, supported, state);
> > }
> >
> > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > +{
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > +}
> > +
>
> Any chance we can go with a different route here than just locking
> fixed mode to being only for BaseT configurations?
>
> I am currently working on getting the fbnic driver up and running and I
> am using fixed-link mode as a crutch until I can finish up enabling
> QSFP module support for phylink and this just knocked out the supported
> link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
>
> Seems like this should really be something handled by some sort of
> validation function rather than just forcing all devices using fixed
> link to assume that they are BaseT. I know in our direct attached
> copper case we aren't running autoneg so that plan was to use fixed
> link until we can add more flexibility by getting QSFP support going.
Please look back at phylink's historical behaviour. Phylink used to
use phy_lookup_setting() to locate the linkmode for the speed and
duplex. That is the behaviour that we should be aiming to preserve.
We were getting:
speed duplex linkmode
10M Half 10baseT_Half
10M Full 10baseT_Full
100M Half 100baseT_Half
100M Full 100baseT_Full
1G Half 1000baseT_Half
1G Full 1000baseT_Full (this changed over time)
2.5G Full 2500baseT_Full
5G Full 5000baseT_Full
At this point, things get weird because of the way linkmodes were
added, as we return the _first_ match. Before commit 3c6b59d6f07c
("net: phy: Add more link modes to the settings table"):
10G Full 10000baseKR_Full
Faster speeds not supported
After the commit:
10G Full 10000baseCR_Full
20G Full 20000baseKR2_Full
25G Full 25000baseCR_Full
40G Full 40000baseCR4_Full
50G Full 50000baseCR2_Full
56G Full 56000baseCR4_Full
100G Full 100000baseCR4_Full
It's all a bit random. :(
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Mar 31, 2025 at 5:51 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Mar 27, 2025 at 06:16:00PM -0700, Alexander H Duyck wrote:
> > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > > When phylink creates a fixed-link configuration, it finds a matching
> > > linkmode to set as the advertised, lp_advertising and supported modes
> > > based on the speed and duplex of the fixed link.
> > >
> > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > phy_lookup_settings(). This has the side effect that the matched
> > > settings and configured linkmodes may now contain several linkmodes (the
> > > intersection of supported linkmodes from the phylink settings and the
> > > linkmodes that match speed/duplex) instead of the one from
> > > phy_lookup_settings().
> > >
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > ---
> > > drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > > 1 file changed, 31 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index cf9f019382ad..8e2b7d647a92 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > > return phylink_validate_mac_and_pcs(pl, supported, state);
> > > }
> > >
> > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > +{
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > +}
> > > +
> >
> > Any chance we can go with a different route here than just locking
> > fixed mode to being only for BaseT configurations?
> >
> > I am currently working on getting the fbnic driver up and running and I
> > am using fixed-link mode as a crutch until I can finish up enabling
> > QSFP module support for phylink and this just knocked out the supported
> > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> >
> > Seems like this should really be something handled by some sort of
> > validation function rather than just forcing all devices using fixed
> > link to assume that they are BaseT. I know in our direct attached
> > copper case we aren't running autoneg so that plan was to use fixed
> > link until we can add more flexibility by getting QSFP support going.
>
> Please look back at phylink's historical behaviour. Phylink used to
> use phy_lookup_setting() to locate the linkmode for the speed and
> duplex. That is the behaviour that we should be aiming to preserve.
>
> We were getting:
>
> speed duplex linkmode
> 10M Half 10baseT_Half
> 10M Full 10baseT_Full
> 100M Half 100baseT_Half
> 100M Full 100baseT_Full
> 1G Half 1000baseT_Half
> 1G Full 1000baseT_Full (this changed over time)
> 2.5G Full 2500baseT_Full
> 5G Full 5000baseT_Full
>
> At this point, things get weird because of the way linkmodes were
> added, as we return the _first_ match. Before commit 3c6b59d6f07c
> ("net: phy: Add more link modes to the settings table"):
>
> 10G Full 10000baseKR_Full
> Faster speeds not supported
>
> After the commit:
> 10G Full 10000baseCR_Full
> 20G Full 20000baseKR2_Full
> 25G Full 25000baseCR_Full
> 40G Full 40000baseCR4_Full
> 50G Full 50000baseCR2_Full
> 56G Full 56000baseCR4_Full
> 100G Full 100000baseCR4_Full
>
> It's all a bit random. :(
I agree. I was using pcs_validate to overcome some of that by limiting
myself to *mostly* one link type at each speed. The only spot that got
a bit iffy was the 50G as I support 50GAUI and LAUI-2. I was
overcoming that by tracking the number of lanes expected and filtering
for one or the other.
One thought I had proposed in another email was to look at adding more
data to the fields. In the case of duplex we could overload it to also
be the number of lanes as they represent full duplex lanes anyway.
With that you could sort out some of the CR vs CR2 noise.
In addition I wonder if we shouldn't also look at including port type
in the data. Using that you could essentially go through the
post-validated supported values and OR in the types that belong to the
various link modes for TP, FIBER, DA, ect. That would sort out some of
the KR vs CR vs SR vs LR noise.
On Thu, 27 Mar 2025 18:16:00 -0700
Alexander H Duyck <alexander.duyck@gmail.com> wrote:
> On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > When phylink creates a fixed-link configuration, it finds a matching
> > linkmode to set as the advertised, lp_advertising and supported modes
> > based on the speed and duplex of the fixed link.
> >
> > Use the newly introduced phy_caps_lookup to get these modes instead of
> > phy_lookup_settings(). This has the side effect that the matched
> > settings and configured linkmodes may now contain several linkmodes (the
> > intersection of supported linkmodes from the phylink settings and the
> > linkmodes that match speed/duplex) instead of the one from
> > phy_lookup_settings().
> >
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> > drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > 1 file changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index cf9f019382ad..8e2b7d647a92 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > return phylink_validate_mac_and_pcs(pl, supported, state);
> > }
> >
> > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > +{
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > +}
> > +
>
> Any chance we can go with a different route here than just locking
> fixed mode to being only for BaseT configurations?
>
> I am currently working on getting the fbnic driver up and running and I
> am using fixed-link mode as a crutch until I can finish up enabling
> QSFP module support for phylink and this just knocked out the supported
> link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
>
> Seems like this should really be something handled by some sort of
> validation function rather than just forcing all devices using fixed
> link to assume that they are BaseT. I know in our direct attached
> copper case we aren't running autoneg so that plan was to use fixed
> link until we can add more flexibility by getting QSFP support going.
These baseT mode were for compatibility with the previous way to deal
with fixed links, but we can extend what's being report above 10G with
other modes. Indeed the above code unnecessarily restricts the
linkmodes. Can you tell me if the following patch works for you ?
Maxime
-----------
From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
From: Maxime Chevallier <maxime.chevallier@bootlin.com>
Date: Fri, 28 Mar 2025 08:53:00 +0100
Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G
The blamed commit introduced a helper to keep the linkmodes reported by
fixed links identical to what they were before phy_caps. This means
filtering linkmodes only to report BaseT modes.
Doing so, it also filtered out any linkmode above 10G. Reinstate the
reporting of linkmodes above 10G, where we report any linkmodes that
exist at these speeds.
Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/phy/phylink.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 69ca765485db..de90fed9c207 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -715,16 +715,25 @@ 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);
+ phylink_fill_fixedlink_supported(match);
+
c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
pl->supported, true);
- if (c)
- linkmode_and(match, pl->supported, c->linkmodes);
+ if (c) {
+ /* Compatbility with the legacy behaviour : Report one single
+ * BaseT mode for fixed-link speeds under or at 10G, or all
+ * linkmodes at the speed/duplex for speeds over 10G
+ */
+ if (linkmode_intersects(match, c->linkmodes))
+ linkmode_and(match, match, c->linkmodes);
+ else
+ linkmode_copy(match, c->linkmodes);
+ }
linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
--
2.48.1
On Fri, Mar 28, 2025 at 1:06 AM Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> On Thu, 27 Mar 2025 18:16:00 -0700
> Alexander H Duyck <alexander.duyck@gmail.com> wrote:
>
> > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > > When phylink creates a fixed-link configuration, it finds a matching
> > > linkmode to set as the advertised, lp_advertising and supported modes
> > > based on the speed and duplex of the fixed link.
> > >
> > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > phy_lookup_settings(). This has the side effect that the matched
> > > settings and configured linkmodes may now contain several linkmodes (the
> > > intersection of supported linkmodes from the phylink settings and the
> > > linkmodes that match speed/duplex) instead of the one from
> > > phy_lookup_settings().
> > >
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > ---
> > > drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > > 1 file changed, 31 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index cf9f019382ad..8e2b7d647a92 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > > return phylink_validate_mac_and_pcs(pl, supported, state);
> > > }
> > >
> > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > +{
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > +}
> > > +
> >
> > Any chance we can go with a different route here than just locking
> > fixed mode to being only for BaseT configurations?
> >
> > I am currently working on getting the fbnic driver up and running and I
> > am using fixed-link mode as a crutch until I can finish up enabling
> > QSFP module support for phylink and this just knocked out the supported
> > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> >
> > Seems like this should really be something handled by some sort of
> > validation function rather than just forcing all devices using fixed
> > link to assume that they are BaseT. I know in our direct attached
> > copper case we aren't running autoneg so that plan was to use fixed
> > link until we can add more flexibility by getting QSFP support going.
>
> These baseT mode were for compatibility with the previous way to deal
> with fixed links, but we can extend what's being report above 10G with
> other modes. Indeed the above code unnecessarily restricts the
> linkmodes. Can you tell me if the following patch works for you ?
>
> Maxime
>
> -----------
>
> From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
> From: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Date: Fri, 28 Mar 2025 08:53:00 +0100
> Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G
>
> The blamed commit introduced a helper to keep the linkmodes reported by
> fixed links identical to what they were before phy_caps. This means
> filtering linkmodes only to report BaseT modes.
>
> Doing so, it also filtered out any linkmode above 10G. Reinstate the
> reporting of linkmodes above 10G, where we report any linkmodes that
> exist at these speeds.
>
> Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
> Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
> Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> drivers/net/phy/phylink.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 69ca765485db..de90fed9c207 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -715,16 +715,25 @@ 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);
>
> + phylink_fill_fixedlink_supported(match);
> +
I worry that this might put you back into the same problem again with
the older drivers. In addition it is filling in modes that shouldn't
be present after the validation.
One thought on this. You might look at checking to see if the TP bit
is set in the supported modes after validation and then use your
fixedlink_supported as a mask if it is supporting twisted pair
connections. One possibility would be to go through and filter the
modes based on the media type where you could basically filter out TP,
Fiber, and Backplane connection types and use that as a second pass
based on the settings by basically doing a set of AND and OR
operations.
Also I am not sure it makes sense to say we can't support multiple
modes on a fixed connection. For example in the case of SerDes links
and the like it isn't unusual to see support for CR/KR advertised at
the same speed on the same link and use the exact same configuration
so a fixed config could support both and advertise both at the same
time if I am not mistaken.
On Fri, 28 Mar 2025 14:03:54 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:
> On Fri, Mar 28, 2025 at 1:06 AM Maxime Chevallier
> <maxime.chevallier@bootlin.com> wrote:
> >
> > On Thu, 27 Mar 2025 18:16:00 -0700
> > Alexander H Duyck <alexander.duyck@gmail.com> wrote:
> >
> > > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > > > When phylink creates a fixed-link configuration, it finds a matching
> > > > linkmode to set as the advertised, lp_advertising and supported modes
> > > > based on the speed and duplex of the fixed link.
> > > >
> > > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > > phy_lookup_settings(). This has the side effect that the matched
> > > > settings and configured linkmodes may now contain several linkmodes (the
> > > > intersection of supported linkmodes from the phylink settings and the
> > > > linkmodes that match speed/duplex) instead of the one from
> > > > phy_lookup_settings().
> > > >
> > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > ---
> > > > drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > > > 1 file changed, 31 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index cf9f019382ad..8e2b7d647a92 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > > > return phylink_validate_mac_and_pcs(pl, supported, state);
> > > > }
> > > >
> > > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > > +{
> > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > > +}
> > > > +
> > >
> > > Any chance we can go with a different route here than just locking
> > > fixed mode to being only for BaseT configurations?
> > >
> > > I am currently working on getting the fbnic driver up and running and I
> > > am using fixed-link mode as a crutch until I can finish up enabling
> > > QSFP module support for phylink and this just knocked out the supported
> > > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> > >
> > > Seems like this should really be something handled by some sort of
> > > validation function rather than just forcing all devices using fixed
> > > link to assume that they are BaseT. I know in our direct attached
> > > copper case we aren't running autoneg so that plan was to use fixed
> > > link until we can add more flexibility by getting QSFP support going.
> >
> > These baseT mode were for compatibility with the previous way to deal
> > with fixed links, but we can extend what's being report above 10G with
> > other modes. Indeed the above code unnecessarily restricts the
> > linkmodes. Can you tell me if the following patch works for you ?
> >
> > Maxime
> >
> > -----------
> >
> > From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
> > From: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > Date: Fri, 28 Mar 2025 08:53:00 +0100
> > Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G
> >
> > The blamed commit introduced a helper to keep the linkmodes reported by
> > fixed links identical to what they were before phy_caps. This means
> > filtering linkmodes only to report BaseT modes.
> >
> > Doing so, it also filtered out any linkmode above 10G. Reinstate the
> > reporting of linkmodes above 10G, where we report any linkmodes that
> > exist at these speeds.
> >
> > Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
> > Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
> > Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> > drivers/net/phy/phylink.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 69ca765485db..de90fed9c207 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -715,16 +715,25 @@ 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);
> >
> > + phylink_fill_fixedlink_supported(match);
> > +
>
> I worry that this might put you back into the same problem again with
> the older drivers. In addition it is filling in modes that shouldn't
> be present after the validation.
Note that I'm not directly filling pl->supported here.
[...]
c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
pl->supported, true);
- if (c)
- linkmode_and(match, pl->supported, c->linkmodes);
+ if (c) {
+ /* Compatbility with the legacy behaviour : Report one single
+ * BaseT mode for fixed-link speeds under or at 10G, or all
+ * linkmodes at the speed/duplex for speeds over 10G
+ */
+ if (linkmode_intersects(match, c->linkmodes))
+ linkmode_and(match, match, c->linkmodes);
+ else
+ linkmode_copy(match, c->linkmodes);
+ }
[...]
if (c) {
linkmode_or(pl->supported, pl->supported, match);
linkmode_or(pl->link_config.lp_advertising,
pl->link_config.lp_advertising, match);
}
For speeds above 10G, you will get all the modes for the requested
speed, so it should solve the issue in the next steps of your code
where you need matching linkmodes for your settings ? Did you give it a
try ?
You may end up with too many linkmodes, but for fixed-link we don't
really expect these modes to precisely represent any real linkmodes
> One thought on this. You might look at checking to see if the TP bit
> is set in the supported modes after validation and then use your
> fixedlink_supported as a mask if it is supporting twisted pair
> connections.
But the TP bit doesn't really mean much here, at least as of today.
What matters at that point really is the supported phy_interface_t and
your requested speed/duplex
> One possibility would be to go through and filter the
> modes based on the media type where you could basically filter out TP,
> Fiber, and Backplane connection types and use that as a second pass
> based on the settings by basically doing a set of AND and OR
> operations.
At that point, the linkmodes aren't very relevant, even if you look at
the old version of that same function, the linkmodes are built only
based on speed/duplex and do not represent any physical mode.
The media-specific filtering will come in a next series, I sent a few
iterations in last cycle to be able to filter out based on medium, I'll
CC you for the next round
> Also I am not sure it makes sense to say we can't support multiple
> modes on a fixed connection. For example in the case of SerDes links
> and the like it isn't unusual to see support for CR/KR advertised at
> the same speed on the same link and use the exact same configuration
> so a fixed config could support both and advertise both at the same
> time if I am not mistaken.
The use-cases for fixed links have mostly been about describing a link
that we know exists, but can't really control or query. In that case,
we don't even know what's the physical medium so we report pretty much
bogus values (as Andrew says, that used to be done by emulating a
copper PHY). As use-cases are evolving, I think we can consider indeed
a better coverage of your use-case :)
Maxime
On Mon, 2025-03-31 at 09:14 +0200, Maxime Chevallier wrote:
> On Fri, 28 Mar 2025 14:03:54 -0700
> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
> > On Fri, Mar 28, 2025 at 1:06 AM Maxime Chevallier
> > <maxime.chevallier@bootlin.com> wrote:
> > >
> > > On Thu, 27 Mar 2025 18:16:00 -0700
> > > Alexander H Duyck <alexander.duyck@gmail.com> wrote:
> > >
> > > > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > > > > When phylink creates a fixed-link configuration, it finds a matching
> > > > > linkmode to set as the advertised, lp_advertising and supported modes
> > > > > based on the speed and duplex of the fixed link.
> > > > >
> > > > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > > > phy_lookup_settings(). This has the side effect that the matched
> > > > > settings and configured linkmodes may now contain several linkmodes (the
> > > > > intersection of supported linkmodes from the phylink settings and the
> > > > > linkmodes that match speed/duplex) instead of the one from
> > > > > phy_lookup_settings().
> > > > >
> > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > > ---
> > > > > drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > > > > 1 file changed, 31 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > index cf9f019382ad..8e2b7d647a92 100644
> > > > > --- a/drivers/net/phy/phylink.c
> > > > > +++ b/drivers/net/phy/phylink.c
> > > > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > > > > return phylink_validate_mac_and_pcs(pl, supported, state);
> > > > > }
> > > > >
> > > > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > > > +{
> > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > > > +}
> > > > > +
> > > >
> > > > Any chance we can go with a different route here than just locking
> > > > fixed mode to being only for BaseT configurations?
> > > >
> > > > I am currently working on getting the fbnic driver up and running and I
> > > > am using fixed-link mode as a crutch until I can finish up enabling
> > > > QSFP module support for phylink and this just knocked out the supported
> > > > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> > > >
> > > > Seems like this should really be something handled by some sort of
> > > > validation function rather than just forcing all devices using fixed
> > > > link to assume that they are BaseT. I know in our direct attached
> > > > copper case we aren't running autoneg so that plan was to use fixed
> > > > link until we can add more flexibility by getting QSFP support going.
> > >
> > > These baseT mode were for compatibility with the previous way to deal
> > > with fixed links, but we can extend what's being report above 10G with
> > > other modes. Indeed the above code unnecessarily restricts the
> > > linkmodes. Can you tell me if the following patch works for you ?
> > >
> > > Maxime
> > >
> > > -----------
> > >
> > > From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
> > > From: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > Date: Fri, 28 Mar 2025 08:53:00 +0100
> > > Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G
> > >
> > > The blamed commit introduced a helper to keep the linkmodes reported by
> > > fixed links identical to what they were before phy_caps. This means
> > > filtering linkmodes only to report BaseT modes.
> > >
> > > Doing so, it also filtered out any linkmode above 10G. Reinstate the
> > > reporting of linkmodes above 10G, where we report any linkmodes that
> > > exist at these speeds.
> > >
> > > Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
> > > Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
> > > Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > ---
> > > drivers/net/phy/phylink.c | 17 +++++++++++++----
> > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index 69ca765485db..de90fed9c207 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -715,16 +715,25 @@ 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);
> > >
> > > + phylink_fill_fixedlink_supported(match);
> > > +
> >
> > I worry that this might put you back into the same problem again with
> > the older drivers. In addition it is filling in modes that shouldn't
> > be present after the validation.
>
> Note that I'm not directly filling pl->supported here.
>
> [...]
>
> c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
> pl->supported, true);
> - if (c)
> - linkmode_and(match, pl->supported, c->linkmodes);
> + if (c) {
> + /* Compatbility with the legacy behaviour : Report one single
> + * BaseT mode for fixed-link speeds under or at 10G, or all
> + * linkmodes at the speed/duplex for speeds over 10G
> + */
> + if (linkmode_intersects(match, c->linkmodes))
> + linkmode_and(match, match, c->linkmodes);
> + else
> + linkmode_copy(match, c->linkmodes);
> + }
>
> [...]
>
> if (c) {
> linkmode_or(pl->supported, pl->supported, match);
> linkmode_or(pl->link_config.lp_advertising,
> pl->link_config.lp_advertising, match);
> }
>
> For speeds above 10G, you will get all the modes for the requested
> speed, so it should solve the issue in the next steps of your code
> where you need matching linkmodes for your settings ? Did you give it a
> try ?
>
> You may end up with too many linkmodes, but for fixed-link we don't
> really expect these modes to precisely represent any real linkmodes
Here is more the quick-n-dirty approach that I think does what you were
trying to do without breaking stuff:
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 16a1f31f0091..380e51c5bdaa 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -713,17 +713,24 @@ 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);
c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
pl->supported, true);
- if (c)
+ if (c) {
linkmode_and(match, pl->supported, c->linkmodes);
+ /* Compatbility with the legacy behaviour:
+ * Report one single BaseT mode.
+ */
+ phylink_fill_fixedlink_supported(mask);
+ if (linkmode_intersects(match, mask))
+ linkmode_and(match, match, mask);
+ linkmode_zero(mask);
+ }
+
linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);
Basically we still need the value to be screened by the pl->supported.
The one change is that we have to run the extra screening on the
intersect instead of skipping the screening, or doing it before we even
start providing bits.
With this approach we will even allow people to use non twisted pair
setups regardless of speed as long as they don't provide any twisted
pair modes in the standard set.
I will try to get this tested today and if it works out I will submit
it for net. I just need to test this and an SFP ksettings_set issue I
found when we aren't using autoneg.
On Tue, Apr 01, 2025 at 08:28:29AM -0700, Alexander H Duyck wrote:
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 16a1f31f0091..380e51c5bdaa 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -713,17 +713,24 @@ 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);
>
> c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
> pl->supported, true);
> - if (c)
> + if (c) {
> linkmode_and(match, pl->supported, c->linkmodes);
>
> + /* Compatbility with the legacy behaviour:
> + * Report one single BaseT mode.
> + */
> + phylink_fill_fixedlink_supported(mask);
> + if (linkmode_intersects(match, mask))
> + linkmode_and(match, match, mask);
> + linkmode_zero(mask);
> + }
> +
> linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
> linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
> linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);
>
> Basically we still need the value to be screened by the pl->supported.
> The one change is that we have to run the extra screening on the
> intersect instead of skipping the screening, or doing it before we even
> start providing bits.
>
> With this approach we will even allow people to use non twisted pair
> setups regardless of speed as long as they don't provide any twisted
> pair modes in the standard set.
>
> I will try to get this tested today and if it works out I will submit
> it for net. I just need to test this and an SFP ksettings_set issue I
> found when we aren't using autoneg.
This code used to be so simple... and that makes me wonder whether
Maxime's work is really the best approach. It seems that the old way
was better precisely because it was more simple.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> > Basically we still need the value to be screened by the pl->supported. > > The one change is that we have to run the extra screening on the > > intersect instead of skipping the screening, or doing it before we even > > start providing bits. > > > > With this approach we will even allow people to use non twisted pair > > setups regardless of speed as long as they don't provide any twisted > > pair modes in the standard set. > > > > I will try to get this tested today and if it works out I will submit > > it for net. I just need to test this and an SFP ksettings_set issue I > > found when we aren't using autoneg. > > This code used to be so simple... and that makes me wonder whether > Maxime's work is really the best approach. It seems that the old way > was better precisely because it was more simple. Sorry to hear you say that. Fixed-link was the main pain point with this work, I've stressed it out. I agree that for fixed-link, it ends-up not looking too good, hopefully the rest of the series compensate for that. Maxime
On Tue, 01 Apr 2025 08:28:29 -0700
Alexander H Duyck <alexander.duyck@gmail.com> wrote:
> On Mon, 2025-03-31 at 09:14 +0200, Maxime Chevallier wrote:
> > On Fri, 28 Mar 2025 14:03:54 -0700
> > Alexander Duyck <alexander.duyck@gmail.com> wrote:
> >
> > > On Fri, Mar 28, 2025 at 1:06 AM Maxime Chevallier
> > > <maxime.chevallier@bootlin.com> wrote:
> > > >
> > > > On Thu, 27 Mar 2025 18:16:00 -0700
> > > > Alexander H Duyck <alexander.duyck@gmail.com> wrote:
> > > >
> > > > > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > > > > > When phylink creates a fixed-link configuration, it finds a matching
> > > > > > linkmode to set as the advertised, lp_advertising and supported modes
> > > > > > based on the speed and duplex of the fixed link.
> > > > > >
> > > > > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > > > > phy_lookup_settings(). This has the side effect that the matched
> > > > > > settings and configured linkmodes may now contain several linkmodes (the
> > > > > > intersection of supported linkmodes from the phylink settings and the
> > > > > > linkmodes that match speed/duplex) instead of the one from
> > > > > > phy_lookup_settings().
> > > > > >
> > > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > > > ---
> > > > > > drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > > > > > 1 file changed, 31 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > > index cf9f019382ad..8e2b7d647a92 100644
> > > > > > --- a/drivers/net/phy/phylink.c
> > > > > > +++ b/drivers/net/phy/phylink.c
> > > > > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > > > > > return phylink_validate_mac_and_pcs(pl, supported, state);
> > > > > > }
> > > > > >
> > > > > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > > > > +{
> > > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > > > > +}
> > > > > > +
> > > > >
> > > > > Any chance we can go with a different route here than just locking
> > > > > fixed mode to being only for BaseT configurations?
> > > > >
> > > > > I am currently working on getting the fbnic driver up and running and I
> > > > > am using fixed-link mode as a crutch until I can finish up enabling
> > > > > QSFP module support for phylink and this just knocked out the supported
> > > > > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> > > > >
> > > > > Seems like this should really be something handled by some sort of
> > > > > validation function rather than just forcing all devices using fixed
> > > > > link to assume that they are BaseT. I know in our direct attached
> > > > > copper case we aren't running autoneg so that plan was to use fixed
> > > > > link until we can add more flexibility by getting QSFP support going.
> > > >
> > > > These baseT mode were for compatibility with the previous way to deal
> > > > with fixed links, but we can extend what's being report above 10G with
> > > > other modes. Indeed the above code unnecessarily restricts the
> > > > linkmodes. Can you tell me if the following patch works for you ?
> > > >
> > > > Maxime
> > > >
> > > > -----------
> > > >
> > > > From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
> > > > From: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > Date: Fri, 28 Mar 2025 08:53:00 +0100
> > > > Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G
> > > >
> > > > The blamed commit introduced a helper to keep the linkmodes reported by
> > > > fixed links identical to what they were before phy_caps. This means
> > > > filtering linkmodes only to report BaseT modes.
> > > >
> > > > Doing so, it also filtered out any linkmode above 10G. Reinstate the
> > > > reporting of linkmodes above 10G, where we report any linkmodes that
> > > > exist at these speeds.
> > > >
> > > > Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
> > > > Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
> > > > Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > ---
> > > > drivers/net/phy/phylink.c | 17 +++++++++++++----
> > > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index 69ca765485db..de90fed9c207 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -715,16 +715,25 @@ 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);
> > > >
> > > > + phylink_fill_fixedlink_supported(match);
> > > > +
> > >
> > > I worry that this might put you back into the same problem again with
> > > the older drivers. In addition it is filling in modes that shouldn't
> > > be present after the validation.
> >
> > Note that I'm not directly filling pl->supported here.
> >
> > [...]
> >
> > c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
> > pl->supported, true);
> > - if (c)
> > - linkmode_and(match, pl->supported, c->linkmodes);
> > + if (c) {
> > + /* Compatbility with the legacy behaviour : Report one single
> > + * BaseT mode for fixed-link speeds under or at 10G, or all
> > + * linkmodes at the speed/duplex for speeds over 10G
> > + */
> > + if (linkmode_intersects(match, c->linkmodes))
> > + linkmode_and(match, match, c->linkmodes);
> > + else
> > + linkmode_copy(match, c->linkmodes);
> > + }
> >
> > [...]
> >
> > if (c) {
> > linkmode_or(pl->supported, pl->supported, match);
> > linkmode_or(pl->link_config.lp_advertising,
> > pl->link_config.lp_advertising, match);
> > }
> >
> > For speeds above 10G, you will get all the modes for the requested
> > speed, so it should solve the issue in the next steps of your code
> > where you need matching linkmodes for your settings ? Did you give it a
> > try ?
> >
> > You may end up with too many linkmodes, but for fixed-link we don't
> > really expect these modes to precisely represent any real linkmodes
>
> Here is more the quick-n-dirty approach that I think does what you were
> trying to do without breaking stuff:
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 16a1f31f0091..380e51c5bdaa 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -713,17 +713,24 @@ 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);
>
> c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
> pl->supported, true);
> - if (c)
> + if (c) {
> linkmode_and(match, pl->supported, c->linkmodes);
>
> + /* Compatbility with the legacy behaviour:
> + * Report one single BaseT mode.
> + */
> + phylink_fill_fixedlink_supported(mask);
> + if (linkmode_intersects(match, mask))
> + linkmode_and(match, match, mask);
> + linkmode_zero(mask);
> + }
> +
> linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
> linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
> linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);
>
> Basically we still need the value to be screened by the pl->supported.
> The one change is that we have to run the extra screening on the
> intersect instead of skipping the screening, or doing it before we even
> start providing bits.
>
> With this approach we will even allow people to use non twisted pair
> setups regardless of speed as long as they don't provide any twisted
> pair modes in the standard set.
>
> I will try to get this tested today and if it works out I will submit
> it for net. I just need to test this and an SFP ksettings_set issue I
> found when we aren't using autoneg.
That looks correct and indeed better than my patch, thanks for that :)
Feel free to send it indeed, I'll give it a try on the setups I have
here as well
Maxime
> Also I am not sure it makes sense to say we can't support multiple > modes on a fixed connection. For example in the case of SerDes links > and the like it isn't unusual to see support for CR/KR advertised at > the same speed on the same link and use the exact same configuration > so a fixed config could support both and advertise both at the same > time if I am not mistaken. Traditionally, fixed link has only supported one mode. The combination of speed and duplex fully describes a base-T link. Even more traditionally, it was implemented as an emulated C22 PHY, using the genphy driver, so limited to just 1G. With multigige PHY we needed to be a bit more flexible, so phylink gained its own fixed link implementation which did not emulate a PHY, just the results of talking to a multigige PHY. But i don't think you are actually talking about a PHY. I think you mean the PCS advertises CR/KR, and you want to emulate a fixed-link PCS? That is a different beast. Andrew
On Fri, Mar 28, 2025 at 2:45 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > Also I am not sure it makes sense to say we can't support multiple > > modes on a fixed connection. For example in the case of SerDes links > > and the like it isn't unusual to see support for CR/KR advertised at > > the same speed on the same link and use the exact same configuration > > so a fixed config could support both and advertise both at the same > > time if I am not mistaken. > > Traditionally, fixed link has only supported one mode. The combination > of speed and duplex fully describes a base-T link. Even more > traditionally, it was implemented as an emulated C22 PHY, using the > genphy driver, so limited to just 1G. With multigige PHY we needed to > be a bit more flexible, so phylink gained its own fixed link > implementation which did not emulate a PHY, just the results of > talking to a multigige PHY. > > But i don't think you are actually talking about a PHY. I think you > mean the PCS advertises CR/KR, and you want to emulate a fixed-link > PCS? That is a different beast. > > Andrew A serdes PHY is part of it, but not a traditional twisted pair PHY as we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I agree it is a different beast, but are we saying that the fixed-link is supposed to be a twisted pair PHY only? That is the part I am confused with as there are multiple scenarios where you might end up with a fixed link configuration at a specific speed for something not twisted pair. For example in our case the firmware provides us with the fixed modulation/lanes/fec configuration so we can essentially take that and treat it as a fixed-link configuration. In addition one advantage is that it makes it possible to support speeds that don't yet have a type in the phy_interface_t, so as I was enabling things it allowed some backwards compatibility with older kernels. In the case of fbnic I was planning to use pcs_validate to strip down the supported features and essentially limit things based on the bitrate per lane and the number of lanes. We were only using CR so for us the result should only be 1 regardless based on the speed match. The general idea I had in mind for upstreaming the support for fbnic was to initially bring it up as a fixed-link setup using PHY_INTERFACE_MODE_INTERNAL as that is essentially what we have now and I can get rid of the extraneous 40G stuff that we aren't using. Then we pivot into enabling additional PHY interface modes and QSFP+/28 support in the kernel. Then I would add a mailbox based i2c and gpio to enable SFP/QSPF on fbnic. After that we could switch fbnic back to the inband setup with support for the higher speed interfaces. One option I would be open to is to have me take on addressing the issue in net-next instead of net since I would need to sort it out to enable my patches anyway. I was mostly bringing it up as I was concerned that I may have not been the only one impacted. I was using the fixed-link/internal combination to basically signal to the phylink interface that we were locked in and weren't going to change it, as such the only impact for me is it seemed to result in a warning message about the link configuration not being recognized and the supported/advertised modes being empty.
On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote: > On Fri, Mar 28, 2025 at 2:45 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > Also I am not sure it makes sense to say we can't support multiple > > > modes on a fixed connection. For example in the case of SerDes links > > > and the like it isn't unusual to see support for CR/KR advertised at > > > the same speed on the same link and use the exact same configuration > > > so a fixed config could support both and advertise both at the same > > > time if I am not mistaken. > > > > Traditionally, fixed link has only supported one mode. The combination > > of speed and duplex fully describes a base-T link. Even more > > traditionally, it was implemented as an emulated C22 PHY, using the > > genphy driver, so limited to just 1G. With multigige PHY we needed to > > be a bit more flexible, so phylink gained its own fixed link > > implementation which did not emulate a PHY, just the results of > > talking to a multigige PHY. > > > > But i don't think you are actually talking about a PHY. I think you > > mean the PCS advertises CR/KR, and you want to emulate a fixed-link > > PCS? That is a different beast. > > > > Andrew > > A serdes PHY is part of it, but not a traditional twisted pair PHY as > we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I > agree it is a different beast, but are we saying that the fixed-link > is supposed to be a twisted pair PHY only? With phylink, the PCS enumerates its capabilities, the PHY enumerates its capabilities, and the MAC enumerates it capabilities. phylink then finds the subset which all support. As i said, historically, fixed_link was used in place of a PHY, since it emulated a PHY. phylinks implementation of fixed_link is however different. Can it be used in place of both a PCS and a PHY? I don't know. You are pushing the envelope here, and maybe we need to take a step back and consider what is a fixed link, how does it fit into the MAC, PCS, PHY model of enumeration? Maybe fixed link should only represent the PHY and we need a second sort of fixed_link object to represent the PCS? I don't know? > In addition one advantage is that it makes it possible to support > speeds that don't yet have a type in the phy_interface_t, so as I was > enabling things it allowed some backwards compatibility with older > kernels. I don't like the sound of that. I would simply not support the older kernels, rather than do some hacks. Andrew
On Mon, Mar 31, 2025 at 04:17:02PM +0200, Andrew Lunn wrote: > On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote: > > A serdes PHY is part of it, but not a traditional twisted pair PHY as > > we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I > > agree it is a different beast, but are we saying that the fixed-link > > is supposed to be a twisted pair PHY only? > > With phylink, the PCS enumerates its capabilities, the PHY enumerates > its capabilities, and the MAC enumerates it capabilities. phylink then > finds the subset which all support. > > As i said, historically, fixed_link was used in place of a PHY, since > it emulated a PHY. phylinks implementation of fixed_link is however > different. Can it be used in place of both a PCS and a PHY? I don't > know. In fixed-link mode, phylink will use a PCS if the MAC driver says there is one, but it will not look for a PHY. > You are pushing the envelope here, and maybe we need to take a step > back and consider what is a fixed link, how does it fit into the MAC, > PCS, PHY model of enumeration? Maybe fixed link should only represent > the PHY and we need a second sort of fixed_link object to represent > the PCS? I don't know? As I previously wrote today in response to an earlier email, the link modes that phylink used were the first-match from the old settings[] array in phylib which is now gone. This would only ever return _one_ link mode, which invariably was a baseT link mode for the slower speeds. Maxime's first approach at adapting this to his new system was to set every single link mode that corresponded with the speed. I objected to that, because it quickly gets rediculous when we end up with lots of link modes being indicated for e.g. 10, 100M, 1G but the emulated PHY for these speeds only indicates baseT. That's just back-compatibility but... in principle changing the link modes that are reported to userspace for a fixed link is something we should not be doing - we don't know if userspace tooling has come to rely on that. Yes, it's a bit weird to be reporting 1000baseT for a 1000BASE-X interface mode, but that's what we've always done in the past and phylink was coded to maintain that (following the principle that we shouldn't do gratuitous changes to the information exposed to userspace.) Maxime's replacement approach is to just expose baseT, which means that for the speeds which do not have a baseT mode, we go from supporting it but with a weird link mode (mostly baseCR*) based on first-match in the settings[] table, to not supporting the speed. > > In addition one advantage is that it makes it possible to support > > speeds that don't yet have a type in the phy_interface_t, so as I was > > enabling things it allowed some backwards compatibility with older > > kernels. > > I don't like the sound of that. I would simply not support the older > kernels, rather than do some hacks. I think Alexander is referring to having a PHY interface modes that supports the speed - for example, if we didn't have a PHY interface mode that supported 100G speeds, then 100G speed would not be supported. Phylink has already restricted this and has done for quite some time. We only allow speeds that the selected interface mode can support, with the exception of PHY_INTERFACE_MODE_INTERNAL which supports everything. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, 31 Mar 2025 15:54:20 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Mon, Mar 31, 2025 at 04:17:02PM +0200, Andrew Lunn wrote: > > On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote: > > > A serdes PHY is part of it, but not a traditional twisted pair PHY as > > > we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I > > > agree it is a different beast, but are we saying that the fixed-link > > > is supposed to be a twisted pair PHY only? > > > > With phylink, the PCS enumerates its capabilities, the PHY enumerates > > its capabilities, and the MAC enumerates it capabilities. phylink then > > finds the subset which all support. > > > > As i said, historically, fixed_link was used in place of a PHY, since > > it emulated a PHY. phylinks implementation of fixed_link is however > > different. Can it be used in place of both a PCS and a PHY? I don't > > know. > > In fixed-link mode, phylink will use a PCS if the MAC driver says there > is one, but it will not look for a PHY. > > > You are pushing the envelope here, and maybe we need to take a step > > back and consider what is a fixed link, how does it fit into the MAC, > > PCS, PHY model of enumeration? Maybe fixed link should only represent > > the PHY and we need a second sort of fixed_link object to represent > > the PCS? I don't know? > > As I previously wrote today in response to an earlier email, the > link modes that phylink used were the first-match from the old > settings[] array in phylib which is now gone. This would only ever > return _one_ link mode, which invariably was a baseT link mode for > the slower speeds. > > Maxime's first approach at adapting this to his new system was to > set every single link mode that corresponded with the speed. I > objected to that, because it quickly gets rediculous when we end > up with lots of link modes being indicated for e.g. 10, 100M, 1G > but the emulated PHY for these speeds only indicates baseT. That's > just back-compatibility but... in principle changing the link modes > that are reported to userspace for a fixed link is something we > should not be doing - we don't know if userspace tooling has come > to rely on that. > > Yes, it's a bit weird to be reporting 1000baseT for a 1000BASE-X > interface mode, but that's what we've always done in the past and > phylink was coded to maintain that (following the principle that > we shouldn't do gratuitous changes to the information exposed to > userspace.) > > Maxime's replacement approach is to just expose baseT, which > means that for the speeds which do not have a baseT mode, we go > from supporting it but with a weird link mode (mostly baseCR*) > based on first-match in the settings[] table, to not supporting the > speed. I very wrongfully considered that there was no >10G fixed-link users, I plan to fix that with something like the proposed patch in the discussion, that reports all linkmodes for speeds above 10G (looks less like a randomly selected mode, you can kind-of see what's going on as you get all the linkmodes) but is a change in what we expose to userspace. Or maybe simpler, I could extend the list of compat fixed-link linkmodes to all speeds with the previous arbitrary values that Russell listed in the other mail (that way, no user-visible changes :) ) 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
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
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);
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.
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
On Mon, Mar 31, 2025 at 9:20 AM Maxime Chevallier <maxime.chevallier@bootlin.com> wrote: > > On Mon, 31 Mar 2025 15:54:20 +0100 > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > On Mon, Mar 31, 2025 at 04:17:02PM +0200, Andrew Lunn wrote: > > > On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote: > > > > A serdes PHY is part of it, but not a traditional twisted pair PHY as > > > > we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I > > > > agree it is a different beast, but are we saying that the fixed-link > > > > is supposed to be a twisted pair PHY only? > > > > > > With phylink, the PCS enumerates its capabilities, the PHY enumerates > > > its capabilities, and the MAC enumerates it capabilities. phylink then > > > finds the subset which all support. > > > > > > As i said, historically, fixed_link was used in place of a PHY, since > > > it emulated a PHY. phylinks implementation of fixed_link is however > > > different. Can it be used in place of both a PCS and a PHY? I don't > > > know. > > > > In fixed-link mode, phylink will use a PCS if the MAC driver says there > > is one, but it will not look for a PHY. Admittedly the documentation does reference much lower speeds as being the use case. I was a bit of an eager beaver and started assembling things without really reading the directions. I just kind of assumed what I could or couldn't get away with within the interface. > > > You are pushing the envelope here, and maybe we need to take a step > > > back and consider what is a fixed link, how does it fit into the MAC, > > > PCS, PHY model of enumeration? Maybe fixed link should only represent > > > the PHY and we need a second sort of fixed_link object to represent > > > the PCS? I don't know? > > > > As I previously wrote today in response to an earlier email, the > > link modes that phylink used were the first-match from the old > > settings[] array in phylib which is now gone. This would only ever > > return _one_ link mode, which invariably was a baseT link mode for > > the slower speeds. > > > > Maxime's first approach at adapting this to his new system was to > > set every single link mode that corresponded with the speed. I > > objected to that, because it quickly gets rediculous when we end > > up with lots of link modes being indicated for e.g. 10, 100M, 1G > > but the emulated PHY for these speeds only indicates baseT. That's > > just back-compatibility but... in principle changing the link modes > > that are reported to userspace for a fixed link is something we > > should not be doing - we don't know if userspace tooling has come > > to rely on that. > > > > Yes, it's a bit weird to be reporting 1000baseT for a 1000BASE-X > > interface mode, but that's what we've always done in the past and > > phylink was coded to maintain that (following the principle that > > we shouldn't do gratuitous changes to the information exposed to > > userspace.) > > > > Maxime's replacement approach is to just expose baseT, which > > means that for the speeds which do not have a baseT mode, we go > > from supporting it but with a weird link mode (mostly baseCR*) > > based on first-match in the settings[] table, to not supporting the > > speed. > > I very wrongfully considered that there was no >10G fixed-link users, I > plan to fix that with something like the proposed patch in the > discussion, that reports all linkmodes for speeds above 10G (looks less > like a randomly selected mode, you can kind-of see what's going on as > you get all the linkmodes) but is a change in what we expose to > userspace. I am not sure if there are any >10G users. I haven't landed anything in the kernel yet and like I said what I was doing was more of a hack to enable backwards compatibility on older kernels w/ the correct supported and advertised modes. If I have to patch one kernel to make it work for me that would be manageable. One thing I was thinking about that it looks like this code might prevent would be reinterpreting the meaning of duplex. Currently we only have 3 values for it 0 (half), 1 (Full), and ~0 (Unknown). One thought I had is that once we are over 1G we don't really care about that anymore as everything is Full duplex and instead care about lanes. As it turns out the duplex values currently used would work well to be extended out to lanes. Essentially 0 would still be half, 1 would be 1 lane full duplex, 2-8 could be the number of full duplex lanes the interface is using, and unknown lane count would still be ~0 since it is unlikely we will end up with anything other than a power of 2 number of lanes anyway. With that you could greatly sort out a number of modes in your setup. We would then have to do some cleanups here and there to do something like "duplex == DUPLEX_UNKNOWN ? duplex : !!duplex" to clean up any cases where the legacy values are expected. Likewise if you were to look at adding the port type that might allow for further division and cleanup. With that someone could specify the speed, duplex, and port type and they would be able to pretty precisely pick out a specific fixed mode. > Or maybe simpler, I could extend the list of compat fixed-link linkmodes > to all speeds with the previous arbitrary values that Russell listed in > the other mail (that way, no user-visible changes :) ) > > 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. I can try to get to it, but I have a number of meetings today so I may not be able to get to it until tomorrow morning. Also I suspect this may have an impact outside of just the fixed link setup. I will have to try some other spots to see if I see anything odd pop up as I suspect that I will have issues with 50R2/50R running over top of each other after these changes. Thanks, - Alex
On Mon, 31 Mar 2025 09:38:34 -0700 Alexander Duyck <alexander.duyck@gmail.com> wrote: > On Mon, Mar 31, 2025 at 9:20 AM Maxime Chevallier > <maxime.chevallier@bootlin.com> wrote: > > > > On Mon, 31 Mar 2025 15:54:20 +0100 > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > > > On Mon, Mar 31, 2025 at 04:17:02PM +0200, Andrew Lunn wrote: > > > > On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote: > > > > > A serdes PHY is part of it, but not a traditional twisted pair PHY as > > > > > we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I > > > > > agree it is a different beast, but are we saying that the fixed-link > > > > > is supposed to be a twisted pair PHY only? > > > > > > > > With phylink, the PCS enumerates its capabilities, the PHY enumerates > > > > its capabilities, and the MAC enumerates it capabilities. phylink then > > > > finds the subset which all support. > > > > > > > > As i said, historically, fixed_link was used in place of a PHY, since > > > > it emulated a PHY. phylinks implementation of fixed_link is however > > > > different. Can it be used in place of both a PCS and a PHY? I don't > > > > know. > > > > > > In fixed-link mode, phylink will use a PCS if the MAC driver says there > > > is one, but it will not look for a PHY. > > Admittedly the documentation does reference much lower speeds as being > the use case. I was a bit of an eager beaver and started assembling > things without really reading the directions. I just kind of assumed > what I could or couldn't get away with within the interface. > > > > > You are pushing the envelope here, and maybe we need to take a step > > > > back and consider what is a fixed link, how does it fit into the MAC, > > > > PCS, PHY model of enumeration? Maybe fixed link should only represent > > > > the PHY and we need a second sort of fixed_link object to represent > > > > the PCS? I don't know? > > > > > > As I previously wrote today in response to an earlier email, the > > > link modes that phylink used were the first-match from the old > > > settings[] array in phylib which is now gone. This would only ever > > > return _one_ link mode, which invariably was a baseT link mode for > > > the slower speeds. > > > > > > Maxime's first approach at adapting this to his new system was to > > > set every single link mode that corresponded with the speed. I > > > objected to that, because it quickly gets rediculous when we end > > > up with lots of link modes being indicated for e.g. 10, 100M, 1G > > > but the emulated PHY for these speeds only indicates baseT. That's > > > just back-compatibility but... in principle changing the link modes > > > that are reported to userspace for a fixed link is something we > > > should not be doing - we don't know if userspace tooling has come > > > to rely on that. > > > > > > Yes, it's a bit weird to be reporting 1000baseT for a 1000BASE-X > > > interface mode, but that's what we've always done in the past and > > > phylink was coded to maintain that (following the principle that > > > we shouldn't do gratuitous changes to the information exposed to > > > userspace.) > > > > > > Maxime's replacement approach is to just expose baseT, which > > > means that for the speeds which do not have a baseT mode, we go > > > from supporting it but with a weird link mode (mostly baseCR*) > > > based on first-match in the settings[] table, to not supporting the > > > speed. > > > > I very wrongfully considered that there was no >10G fixed-link users, I > > plan to fix that with something like the proposed patch in the > > discussion, that reports all linkmodes for speeds above 10G (looks less > > like a randomly selected mode, you can kind-of see what's going on as > > you get all the linkmodes) but is a change in what we expose to > > userspace. > > I am not sure if there are any >10G users. I haven't landed anything > in the kernel yet and like I said what I was doing was more of a hack > to enable backwards compatibility on older kernels w/ the correct > supported and advertised modes. If I have to patch one kernel to make > it work for me that would be manageable. > > One thing I was thinking about that it looks like this code might > prevent would be reinterpreting the meaning of duplex. Currently we > only have 3 values for it 0 (half), 1 (Full), and ~0 (Unknown). One > thought I had is that once we are over 1G we don't really care about > that anymore as everything is Full duplex and instead care about > lanes. As it turns out the duplex values currently used would work > well to be extended out to lanes. Essentially 0 would still be half, 1 > would be 1 lane full duplex, 2-8 could be the number of full duplex > lanes the interface is using, and unknown lane count would still be ~0 > since it is unlikely we will end up with anything other than a power > of 2 number of lanes anyway. With that you could greatly sort out a > number of modes in your setup. We would then have to do some cleanups > here and there to do something like "duplex == DUPLEX_UNKNOWN ? duplex > : !!duplex" to clean up any cases where the legacy values are > expected. Funny you say that, the phy_port work I was referring to with the mediums introduction also tracks the lanes for a given port previous outdated iteration here : https://lore.kernel.org/netdev/20250213101606.1154014-4-maxime.chevallier@bootlin.com/ The idea is to represent physical interfaces to a NIC, as NICs can have multiple ports for different types of connectors. Ports would be driven by either a PHY, a NIC directly or an SFP. The port contains, among other things, speed, duplex, n_lanes, and the medium (with more granularity than just TP/Backplane/Fiber, as we list the 802.3 media : BaseS / BaseC / BaseK / BaseT ...) We already have information about whickh linkmode uses how many lanes, but we don't do much with that information besides reporting it to userspace. I'll revamp the whole thing for the next iteration, however I really only have a deep-ish understanding of embedded usecases, and I lack insights on other classes of devices such as what your working on. I'll be very happy to get your feedback on it, I plan to send that when net-next reopens. Maxime
On Fri, 28 Mar 2025 16:26:04 -0700 Alexander Duyck <alexander.duyck@gmail.com> wrote: > The general idea I had in mind for upstreaming the support for fbnic > was to initially bring it up as a fixed-link setup using > PHY_INTERFACE_MODE_INTERNAL as that is essentially what we have now > and I can get rid of the extraneous 40G stuff that we aren't using. > Then we pivot into enabling additional PHY interface modes and > QSFP+/28 support in the kernel. Then I would add a mailbox based i2c > and gpio to enable SFP/QSPF on fbnic. After that we could switch fbnic > back to the inband setup with support for the higher speed interfaces. > > One option I would be open to is to have me take on addressing the > issue in net-next instead of net since I would need to sort it out to > enable my patches anyway. If that's OK for you and the proposed patch fixes your problem, I'd still like for the patch to get in, as right now fixed links will not work at all for >10G links. Then we can get better linkmodes reporting for your usecase ? Maxime
© 2016 - 2026 Red Hat, Inc.