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

[Javen]

>
>> +/**
>> + * 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.

Thanks for review.
The unconditional phylink_set() is required precisely because of how phylink_validate() works. It acts as a necessary "reset" to prevent the permanent loss of capabilities in pl->supported across MTU changes.
Consider the following scenario when MTU changes:
1. MTU 1500 -> 9000: The MAC loses Pause support. mac_pause becomes 0. When phylink_validate() is called, it correctly clears the Pause and Asym_Pause bits in pl->supported. This happens in the function call trace phylink_validate()=>phylink_validate_mac_and_pcs()=>phylink_validate_mask_caps()=>linkmode_and(supported, supported, mask), where mask represent the capability.
2. MTU 9000 -> 1500: The MAC regains Pause support. We update mac_pause back to 1. But if we simply call phylink_validate() at this point without unconditionally setting the bits first, it will take the current pl->supported (where Pause is 0 from step 1) as its input. The Pause capability will be permanently masked out and never restored.
By unconditionally setting them here, we restore the baseline to 1, allowing phylink_validate() to correctly decide whether to keep them or clear them based on the current PHY/MAC capabilities.

>
>> +     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.

If the MAC does not support pause (e.g., mac_pause == 0), those unconditionally set bits are correctly masked out during the phylink_validate() execution chain. Inside phylink_validate_mask_caps(), the mac_capabilities are converted into an ethtool linkmode mask, and the actual filtering happens via linkmode_and(state->advertising, state->advertising, mask). And mac_capabilities is from pl->config->mac_capabilities, which we set at the beginning of this patch.

BRs,
Javen

>
>        Andrew