On 11/27/25 3:06 PM, Hangbin Liu wrote:
> On Thu, Nov 27, 2025 at 11:36:43AM +0100, Paolo Abeni wrote:
>> On 11/24/25 5:33 AM, Hangbin Liu wrote:
>>> static void ad_churn_machine(struct port *port)
>>> {
>>> - if (port->sm_vars & AD_PORT_CHURNED) {
>>> + bool partner_synced = port->partner_oper.port_state &
>>> LACP_STATE_SYNCHRONIZATION;
>>> + bool actor_synced = port->actor_oper_port_state &
>>> LACP_STATE_SYNCHRONIZATION;
>>> + bool partner_churned = port->sm_vars & AD_PORT_PARTNER_CHURN;
>>> + bool actor_churned = port->sm_vars & AD_PORT_ACTOR_CHURN;
>>> +
>>> + /* ---- 1. begin or port not enabled ---- */
>>> + if ((port->sm_vars & AD_PORT_BEGIN) || !port->is_enabled) {
>>> port->sm_vars &= ~AD_PORT_CHURNED;
>>> +
>>> port->sm_churn_actor_state = AD_CHURN_MONITOR;
>>> port->sm_churn_partner_state = AD_CHURN_MONITOR;
>>> +
>>> port->sm_churn_actor_timer_counter =
>>> __ad_timer_to_ticks(AD_ACTOR_CHURN_TIMER, 0);
>>> port->sm_churn_partner_timer_counter =
>>> - __ad_timer_to_ticks(AD_PARTNER_CHURN_TIMER, 0);
>>> + __ad_timer_to_ticks(AD_PARTNER_CHURN_TIMER, 0);
>>> +
>>
>> Please avoid white-space changes only, or if you are going to target
>> net-next, move them to a pre-req patch.
>
> OK, what's pre-req patch?
I mean: a separate patch, earlier in the series, to keep cosmetic and
functional changes separated and more easily reviewable.
>>> + if (actor_synced) {
>>> + port->sm_vars &= ~AD_PORT_ACTOR_CHURN;
>>> port->sm_churn_actor_state = AD_NO_CHURN;
>>> - } else {
>>> - port->churn_actor_count++;
>>> - port->sm_churn_actor_state = AD_CHURN;
>>> + actor_churned = false;
>>> }
>>
>> I think this part is not described by the state diagram above?!?
>
> This part is about path (3), port in monitor or churn, and actor is in sync.
> Then move to state no_churn.
>
> Do you mean port->sm_vars &= ~AD_PORT_ACTOR_CHURN is not described?
> Hmm, maybe we don't need this after re-organise.
I mean the state change in the else part, I can't map them in the state
machine diagram.
>>> }
>>> +
>>> + /* ---- 4. NO_CHURN + !sync -> MONITOR ---- */
>>> + if (port->sm_churn_actor_state == AD_NO_CHURN && !actor_churned &&
>>> !actor_synced) {
>>> + port->sm_churn_actor_state = AD_CHURN_MONITOR;
>>> + port->sm_churn_actor_timer_counter =
>>> + __ad_timer_to_ticks(AD_ACTOR_CHURN_TIMER, 0);
>>
>> Should this clear sm_vars & AD_PORT_ACTOR_CHURN, too?
>
> Yes, or we can just remove AD_PORT_ACTOR_CHURN as I said above.
Generally speaking less state sounds simpler and better.
/P
