2017-05-19 18:55 GMT+02:00 Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx>: > On 5/19/17 7:25 PM, Ivan Vecera wrote: >> >> 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. >> 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. >> >> 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. >> 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
