Re: [PATCH net-next v6 2/7] net: phy: phylink: add helper to modify pause
From: Maxime Chevallier <maxime.chevallier@bootlin.com>
Date: 2026-07-10 10:05:17
Also in:
lkml
Hi, On 7/9/26 12:02, javen wrote:
quoted hunk ↗ jump to hunk
From: Javen Xu <redacted> 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 <redacted> --- 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