Re: [PATCH net] bridge: fix hello and hold timers starting/stopping

2017-05-19 18:55 GMT+02:00 Nikolay Aleksandrov [off-list ref]:

On 5/19/17 7:25 PM, Ivan Vecera wrote:

quoted

Current bridge code incorrectly handles starting/stopping of hello and
hold timers during STP enable/disable.

1. Timers are stopped in br_stp_start() during NO_STP->USER_STP
    transition. This is not correct as the timers are stopped in NO_STP
    case.

This really is a noop, but ok.

Yes, stopping of stopped timers are safe but confusing.

quoted

2. Timers are started in br_stp_stop() during USER_STP->NO_STP transition.
    This is not also correct as the timers should be stopped in NO_STP
    state.

Indeed, but the actual end result is almost as them being stopped because
in the timers there are specific checks if the STP == KERNEL_STP (see
br_transmit_config()) and the hold_timers will simply expire and not rearm
in any other mode. The only real problem is the hello_timer which continues
to rearm itself, but with Xin's earlier patch that is taken care of too.

Yes, this is clean-up as well. The starting of timers are more
confusing than dangerous
but from a reader's point of view the starting of timers is non-sense
when STP is
going to be disabled.

quoted

3. Timers are NOT stopped in br_stp_stop() during KERNEL_STP->NO_STP
    transition. They should be stopped as they are running in KERNEL_STP
    state and should not run in NO_STP case.

Same comment as for point 2.

This can be removed... and leave hello_timer handler to stop itself.

quoted

The patch is a follow-up for "bridge: start hello_timer when enabling
KERNEL_STP in br_stp_start" patch from Xin Long.

I'd say this is more of a cleanup/improvement after Xin's patch and thus
would
suggest targeting net-next. The only real issue is fixed by his patch.

Agree... will send resend against net-next.

Thanks for comments,
Ivan