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(-)
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;
--
2.48.1On 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 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? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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
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 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". -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
© 2016 - 2026 Red Hat, Inc.