[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZXs467Epke85f0Im@shell.armlinux.org.uk>
Date: Thu, 14 Dec 2023 17:18:35 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, "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>
Subject: Re: [PATCH net-next] net: phylink: avoid one unnecessary
phylink_validate() call during phylink_create()
I'll say this for the benefit of the netdev developers - my time is
currently being spent elsewhere (e.g. ARM64 VCPU hotplug) so I don't
have a lot of time to review netdev patches. With only a few days
left to Christmas vacations and my intention not to work over that
period, this means that I'm unlikely to be able to review changes
such as this - and they do need review.
If I get some spare time, then I will review them.
However, probably best to wait until the new year before sending
patches that touch phylink - which will involve adding stress on my
side.
Thanks.
On Thu, Dec 14, 2023 at 07:06:59PM +0200, Vladimir Oltean wrote:
> 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
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists