| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250304154330.6e00961b@fedora.home>
Date: Tue, 4 Mar 2025 15:43:30 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: davem@...emloft.net, Andrew Lunn <andrew@...n.ch>, Jakub Kicinski
<kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
<pabeni@...hat.com>, Russell King <linux@...linux.org.uk>, Heiner Kallweit
<hkallweit1@...il.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
thomas.petazzoni@...tlin.com, linux-arm-kernel@...ts.infradead.org,
Christophe Leroy <christophe.leroy@...roup.eu>, Herve Codina
<herve.codina@...tlin.com>, Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <vladimir.oltean@....com>, Köry Maincent
<kory.maincent@...tlin.com>, Oleksij Rempel <o.rempel@...gutronix.de>,
Simon Horman <horms@...nel.org>, Romain Gantois
<romain.gantois@...tlin.com>
Subject: Re: [PATCH net-next v4 09/13] net: phylink: Use phy_caps_lookup for
fixed-link configuration
Hi,
On Mon, 3 Mar 2025 10:03:15 +0100
Maxime Chevallier <maxime.chevallier@...tlin.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@...tlin.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;
Powered by blists - more mailing lists