Re: [PATCH net-next v6 2/7] net: phy: phylink: add helper to modify pause

[Maxime Chevallier]

Hi,

On 7/9/26 12:02, javen wrote:
> From: Javen Xu <javen_xu@xxxxxxxxxxxxxx>
> 
> For Realtek nics, when we enable jumbo, pause are not supported. So we
> must check the pause capabilities from ourself and lp.
> 
> Signed-off-by: Javen Xu <javen_xu@xxxxxxxxxxxxxx>
> ---
> Changes in v5:
>  - no changes, new file
> 
> Changes in v6:
>  - rename phylink_update_mac_pause_capabilities(), this function only
>    changes mac pause capability
>  - set asym pause and pause according to config->pause tx and rx
>  - add phydev->lock when change pl->phydev->advertising
> ---
>  drivers/net/phy/phylink.c | 87 +++++++++++++++++++++++++++++++++++++++
>  include/linux/phylink.h   |  2 +
>  2 files changed, 89 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 59dfe35afa54..9e9cd79301d6 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1828,6 +1828,93 @@ int phylink_set_fixed_link(struct phylink *pl,
>  }
>  EXPORT_SYMBOL_GPL(phylink_set_fixed_link);
>  
> +/**
> + * phylink_update_mac_pause_capabilities() - Dynamically update MAC pause
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + * @mac_pause: the new MAC pause capabilities mask
> + *
> + * This function allows a MAC driver to dynamically change its pause state,
> + * such as losing/gaining Pause frame support based on MTU size.
> + * It recalculates supported link modes and triggers renegotiation if needed.
> + */
> +void phylink_update_mac_pause_capabilities(struct phylink *pl, unsigned long mac_pause)
> +{
> +	struct phylink_link_state *config = &pl->link_config;
> +	unsigned long old_pause, caps_added, caps_removed;
> +	bool pause_adv, asym_adv;
> +
> +	ASSERT_RTNL();
> +
> +	if (mac_pause & ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE)) {
> +		phylink_err(pl, "Attempted to dynamically change non-pause MAC capabilities\n");
> +		return;
> +	}
> +
> +	old_pause = pl->config->mac_capabilities & (MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
> +	caps_added = mac_pause & ~old_pause;
> +	caps_removed = old_pause & ~mac_pause;
> +
> +	if (!caps_added && !caps_removed)
> +		return;
> +
> +	mutex_lock(&pl->state_mutex);
> +
> +	pl->config->mac_capabilities &= ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
> +	pl->config->mac_capabilities |= mac_pause;
> +
> +	if (caps_removed & MAC_SYM_PAUSE)
> +		linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, pl->supported);
> +	if (caps_removed & MAC_ASYM_PAUSE)
> +		linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, pl->supported);
> +
> +	linkmode_and(config->advertising, config->advertising, pl->supported);
> +
> +	if (caps_added & MAC_SYM_PAUSE) {
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, pl->supported);
> +		if (pl->phydev && !phylink_test(pl->phydev->supported, Pause))
> +			linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, pl->supported);

Why look at what the PHY can do here ? You shouldn't need to.

> +	}
> +	if (caps_added & MAC_ASYM_PAUSE) {
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, pl->supported);
> +		if (pl->phydev && !phylink_test(pl->phydev->supported, Asym_Pause))
> +			linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, pl->supported);
> +	}
> +
> +	linkmode_and(config->advertising, config->advertising, pl->supported);
> +
> +	if (config->pause & MLO_PAUSE_AN) {
> +		if (phylink_test(pl->supported, Pause) &&
> +		    (config->pause & MLO_PAUSE_RX) && (config->pause & MLO_PAUSE_TX))
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, config->advertising);

This isn't correct, Pause is set if RX is set, regardless of the TX value

> +
> +		if (phylink_test(pl->supported, Asym_Pause) &&
> +		    ((config->pause & MLO_PAUSE_RX) ^ (config->pause & MLO_PAUSE_TX)))
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, config->advertising);

to get this right, use :

linkmode_set_pause(adv, config->pause & MLO_PAUSE_TX, config->pause & MLO_PAUSE_RX)

This will build the supported bitfield for you.

I also think we should update pl->link_config.pause, as phylink_ethtool_get_pauseparam will
report wrong values otherwise.

I'm wondering wether it's worth keeping track of the user-requested pause settings when
changing the MTU, we have a true HW capability change here, so I think we should change it.

> +
> +	if (!pl->phydev)
> +		phylink_change_inband_advert(pl);
> +
> +	mutex_unlock(&pl->state_mutex);
> +
> +	if (pl->phydev) {
> +		pause_adv = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +					      config->advertising);
> +		asym_adv = linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +					     config->advertising);
> +
> +		mutex_lock(&pl->phydev->lock);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				 pl->phydev->advertising, pause_adv);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +				 pl->phydev->advertising, asym_adv);


This is too much manual interaction with all the supported/advertising in phylink/phylib, it's
hard to read, understand and debug :(

We have helpers in phylib such as phy_set_asym_pause() to change pause settings
in phy_devices, don't set these manually like this.

Let's take a step back, it seems you're following sashiko too literally here.

There's a change in the MAC's ability to support Pause, so we should :

 - Recompute the pl->supported field. Update the config.mac_capabilities with the
   new pause settings, calling phylink_validate() should do the trick I think, this
   will rebuild the capability list:

   phylink_validate(pl, pl->supported, &pl->link_config);
   
 - Then update the pl->link_config.pause,

 - Then update the pause advertising, like done in phylink_setpauseparam
   ( I think, everything that comes after pl->state_mutex gets released in
    phylink_ethtool_set_pauseparam)

Ideally, the logic to update the advertising and re-trigger a negociation
should be factored out in a private helper, then reused from both this
path (MAC updates pause support) and the phylink_ethtool_set_pauseparam path.

> +		mutex_unlock(&pl->phydev->lock);

Why take phydev->lock here ? Sashiko's comment on a possible race with ethtool -s
isn't right, you go through phylink_ethtool_ksettings_set() first, which is
serialised with this current function through RTNL.

Maxime