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

[Maxime Chevallier]

Hi Javen,

On 7/21/26 11:52, Javen wrote:
> Hi Maxime,
> 
>>
>> Hi Javen,
>>
>> On 7/13/26 09:51, Javen wrote:
>>> Hi,
>>>
>>>> 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.
>>>>
>>>> Maxime
>>>
>>> Thanks for review and helpful suggestions.
>>>
>>> I agree with your suggestion to factor out the logic into a private helper and
>> reuse it for both phylink_ethtool_set_pauseparam() and
>> phylink_update_mac_pause_capabilities().
>>>
>>> Here is the refactored logic. I want to share this specific part with you for a
>> quick check before I submit v7 patch.
>>
>> I'm currently attending the netdev conference, It'll take a few days for me to
>> look at this, sorry about that :/
>>
>> Maxime
> 
> Just a gentle ping on this thread. Could you please take a quick look at this refactored logic whenever you are free? Any suggestion would be greatly appreciated.

Thanks for the ping, I just looked at it from a high level and this is the
thing I had in mind yes :)

I think you can post a new revision then.

Thanks for that work,

Maxime