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

[Andrew Lunn <andrew@lunn.ch>]

> +/**
> + * 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;
> +	int pause_state;
> +
> +	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);
> +	if (old_pause == mac_pause)
> +		return;
> +
> +	mutex_lock(&pl->state_mutex);
> +
> +	pl->config->mac_capabilities &= ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
> +	pl->config->mac_capabilities |= mac_pause;
> +
> +	phylink_set(pl->supported, Pause);
> +	phylink_set(pl->supported, Asym_Pause);

I don't know the phylink code very well. Could you explain why these
are unconditional? It is not obvious to me.

> +	if (pl->phydev)
> +		linkmode_and(pl->supported, pl->supported, pl->phydev->supported);
> +	else if (pl->sfp_bus)
> +		linkmode_and(pl->supported, pl->supported, pl->sfp_support);

These lines are masking out pause if the PHY or SFP does out support
pause. But where is pause masked out if the MAC does not support
pause? That is what i'm missing here.

	Andrew