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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!