| Message ID | 20250303090321.805785-10-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: Rework linkmodes handling in a dedicated file | expand |
Hi, On Mon, 3 Mar 2025 10:03:15 +0100 Maxime Chevallier <maxime.chevallier@bootlin.com> 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> > --- Maybe before anything goes further with this patch, I'd like to get some feedback from it on a particular point. This changes the linkmodes that are reported on fixed-link interfaces. Instead of reporting one single mode, we report all modes supported by the fixed-link' speed and duplex settings. The following example is a before/after of the "ethtool ethX" output on a 1G fixed link : Before this patch : Settings for eth0: Supported ports: [ MII ] Supported link modes: 1000baseT/Full Supported pause frame use: Symmetric Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: Not reported Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Speed: Unknown! Duplex: Half Port: MII PHYAD: 0 Transceiver: internal Auto-negotiation: off Supports Wake-on: d Wake-on: d Link detected: no After : Supported ports: [ MII ] Supported link modes: 1000baseT/Full 1000baseKX/Full 1000baseX/Full 1000baseT1/Full Supported pause frame use: Symmetric Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: Not reported Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Speed: Unknown! Duplex: Half Port: MII PHYAD: 0 Transceiver: internal Auto-negotiation: off Supports Wake-on: d Wake-on: d Link detected: no The fixed-link in question is for the CPU port of a DSA switch. In my opinion, this is OK as the linkmodes expressed here don't match physical linkmodes on an actual wire, but as this is a user visible change, I'd like to make sure this is OK. Any comment here is more than welcome. Maxime > V4: Remove unnecessary linklmode_zero in phylink_set_fixed_link(), > follwing Russell's comment > > drivers/net/phy/phylink.c | 27 +++++++++++++++------------ > 1 file changed, 15 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 6c67d5c9b787..0b9585cb508e 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -805,9 +805,10 @@ static int phylink_validate(struct phylink *pl, unsigned long *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; > @@ -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); > > linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask); > linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask); > @@ -889,9 +892,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 +1883,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;
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? Thanks! Paolo
On Thu, 6 Mar 2025 09:56:32 +0100 Paolo Abeni <pabeni@redhat.com> 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); + 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); That way we should have a consistent behaviour with what we currently have, and to me it's more explicit what will the fixed-link linkmodes be. I'd like to make sure Russell is OK with that though :) Thanks you for the review Paolo ! Maxime
On Tue, Mar 04, 2025 at 03:43:30PM +0100, Maxime Chevallier wrote: > On Mon, 3 Mar 2025 10:03:15 +0100 > Maxime Chevallier <maxime.chevallier@bootlin.com> 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> > > --- > > Maybe before anything goes further with this patch, I'd like to get > some feedback from it on a particular point. This changes the linkmodes > that are reported on fixed-link interfaces. Instead of reporting one > single mode, we report all modes supported by the fixed-link' speed and > duplex settings. This is a good question. We have historically only used the baseT link modes because the software PHY implementation was based around clause 22 baseT PHYs (although that doesn't support >1G of course.) The real question is... does it matter, to which I'd say I don't know. One can argue that it shouldn't matter, and I think userspace would be unlikely to break, but userspace tends to do weird stuff all the time so there's never any guarantee. > The fixed-link in question is for the CPU port of a DSA switch. > > In my opinion, this is OK as the linkmodes expressed here don't match > physical linkmodes on an actual wire, but as this is a user visible > change, I'd like to make sure this is OK. Any comment here is more than > welcome. Maybe Andrew has an opinion, but I suspect like me, it's really a case that "we don't know".
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@redhat.com> 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?
On Thu, 6 Mar 2025 12:51:16 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> 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@redhat.com> 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@bootlin.com/ 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
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 6c67d5c9b787..0b9585cb508e 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -805,9 +805,10 @@ static int phylink_validate(struct phylink *pl, unsigned long *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; @@ -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); linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask); linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask); @@ -889,9 +892,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 +1883,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;
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> --- V4: Remove unnecessary linklmode_zero in phylink_set_fixed_link(), follwing Russell's comment drivers/net/phy/phylink.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)