netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink_create()
@ 2023-12-14 17:06 Vladimir Oltean
  2023-12-14 17:18 ` Russell King (Oracle)
  0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Oltean @ 2023-12-14 17:06 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Andrew Lunn, Heiner Kallweit, Russell King

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@nxp.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


^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-01-02 16:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-14 17:06 [PATCH net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink_create() Vladimir Oltean
2023-12-14 17:18 ` Russell King (Oracle)
2024-01-02 14:24   ` Russell King (Oracle)
2024-01-02 16:26     ` Vladimir Oltean

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).