netdev - [PATCH net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20231214170659.868759-1-vladimir.oltean@nxp.com>
Date: Thu, 14 Dec 2023 19:06:59 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>
Subject: [PATCH net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink_create()

The direct phylink_validate() call from phylink_create() is partly
redundant, since there will be subsequent calls to phylink_validate()
issued by phylink_parse_mode() for MLO_AN_INBAND, and by
phylink_parse_fixedlink() for MLO_AN_FIXED. These will overwrite what
the initial phylink_validate() call already did.

The only exception is MLO_AN_PHY, for which phylink_validate() might be
called with a slight delay (the timing of the phylink_bringup_phy() call
is not under phylink's control). User space could issue
phylink_ethtool_ksettings_get() calls before phylink_bringup_phy(), and
could thus see an empty pl->supported, which this early phylink_validate()
call prevents.

So we can delay the direct phylink_create() -> phylink_validate() call
until after phylink_parse_mode() and phylink_parse_fixedlink(), and execute
it only for the mode where it makes any difference at all - MLO_AN_PHY.

This has the benefit that we issue one phylink_validate() call less, for
some deployments. The visible output remains unchanged in all cases.

Link: https://lore.kernel.org/netdev/20231004222523.p5t2cqaot6irstwq@skbuf/
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
The other, non-immediate benefit has to do with potential future API
extensions. With this change, pl->cfg_link_an_mode is now parsed and
available to phylink every time phylink_validate() is called. So it is
possible to pass it to pcs_validate(), if that ever becomes necessary,
for example with the introduction of a separate MLO_AN_* mode for clause
73 auto-negotiation (i.e. in-band selection of state->interface).

I don't think this extra information should go into the commit message,
since these plans may or may not materialize. They are just extra
information to give reviewers context. The change is useful even if the
plans do not materialize.

 drivers/net/phy/phylink.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 4adf8ff3ac31..65bff93b1bd8 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1620,10 +1620,6 @@ struct phylink *phylink_create(struct phylink_config *config,
 	__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
 	timer_setup(&pl->link_poll, phylink_fixed_poll, 0);

-	linkmode_fill(pl->supported);
-	linkmode_copy(pl->link_config.advertising, pl->supported);
-	phylink_validate(pl, pl->supported, &pl->link_config);
-
 	ret = phylink_parse_mode(pl, fwnode);
 	if (ret < 0) {
 		kfree(pl);
@@ -1636,6 +1632,17 @@ struct phylink *phylink_create(struct phylink_config *config,
 			kfree(pl);
 			return ERR_PTR(ret);
 		}
+	} else if (pl->cfg_link_an_mode == MLO_AN_PHY) {
+		/* phylink_bringup_phy() will recalculate pl->supported with
+		 * information from the PHY, but it may take a while until it
+		 * is called, and we should report something to user space
+		 * until then rather than nothing at all, to avoid issues.
+		 * Just report all link modes supportable by the current
+		 * phy_interface_t and the MAC capabilities.
+		 */
+		linkmode_fill(pl->supported);
+		linkmode_copy(pl->link_config.advertising, pl->supported);
+		phylink_validate(pl, pl->supported, &pl->link_config);
 	}

 	pl->cur_link_an_mode = pl->cfg_link_an_mode;
-- 
2.34.1