[PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Xin Long]

Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
kernel hello and hold timers"), bridge would not start hello_timer if
stp_enabled is not KERNEL_STP when br_dev_open.

The problem is even if users set stp_enabled with KERNEL_STP later,
the timer will still not be started. It causes that KERNEL_STP can
not really work. Users have to re-ifup the bridge to avoid this.

This patch is to fix it by starting br->hello_timer when enabling
KERNEL_STP in br_stp_start.

As an improvement, it's also to start hello_timer again only when
br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
no reason to start the timer again when it's NO_STP.

Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel hello and hold timers")
Reported-by: Haidong Li <haili@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/bridge/br_stp_if.c    | 1 +
 net/bridge/br_stp_timer.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 08341d2..0db8102 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -179,6 +179,7 @@ static void br_stp_start(struct net_bridge *br)
 		br_debug(br, "using kernel STP\n");
 
 		/* To start timers on any ports left in blocking */
+		mod_timer(&br->hello_timer, jiffies + br->hello_time);
 		br_port_state_selection(br);
 	}
 
diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
index c98b3e5..60b6fe2 100644
--- a/net/bridge/br_stp_timer.c
+++ b/net/bridge/br_stp_timer.c
@@ -40,7 +40,7 @@ static void br_hello_timer_expired(unsigned long arg)
 	if (br->dev->flags & IFF_UP) {
 		br_config_bpdu_generation(br);
 
-		if (br->stp_enabled != BR_USER_STP)
+		if (br->stp_enabled == BR_KERNEL_STP)
 			mod_timer(&br->hello_timer,
 				  round_jiffies(jiffies + br->hello_time));
 	}
-- 
2.1.0

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Nikolay Aleksandrov]

On 5/19/17 5:20 PM, Xin Long wrote:
> Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
> kernel hello and hold timers"), bridge would not start hello_timer if
> stp_enabled is not KERNEL_STP when br_dev_open.
> 
> The problem is even if users set stp_enabled with KERNEL_STP later,
> the timer will still not be started. It causes that KERNEL_STP can
> not really work. Users have to re-ifup the bridge to avoid this.
> 
> This patch is to fix it by starting br->hello_timer when enabling
> KERNEL_STP in br_stp_start.
> 
> As an improvement, it's also to start hello_timer again only when
> br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
> no reason to start the timer again when it's NO_STP.
> 
> Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel hello and hold timers")
> Reported-by: Haidong Li <haili@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>   net/bridge/br_stp_if.c    | 1 +
>   net/bridge/br_stp_timer.c | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 

This doesn't make much sense to me, how do you change from USER_STP to
KERNEL_STP without first going through NO_STP ?

If you go through NO_STP then all will be fine because br_stp_stop will restart
the timers if the previous val was USER_STP.

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Florian Westphal]

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> On 5/19/17 5:20 PM, Xin Long wrote:
> >Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
> >kernel hello and hold timers"), bridge would not start hello_timer if
> >stp_enabled is not KERNEL_STP when br_dev_open.
> >
> >The problem is even if users set stp_enabled with KERNEL_STP later,
> >the timer will still not be started. It causes that KERNEL_STP can
> >not really work. Users have to re-ifup the bridge to avoid this.
> >
> >This patch is to fix it by starting br->hello_timer when enabling
> >KERNEL_STP in br_stp_start.
> >
> >As an improvement, it's also to start hello_timer again only when
> >br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
> >no reason to start the timer again when it's NO_STP.
> >
> >Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel hello and hold timers")
> >Reported-by: Haidong Li <haili@redhat.com>
> >Signed-off-by: Xin Long <lucien.xin@gmail.com>
> >---
> >  net/bridge/br_stp_if.c    | 1 +
> >  net/bridge/br_stp_timer.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> 
> This doesn't make much sense to me, how do you change from USER_STP to
> KERNEL_STP without first going through NO_STP ?

This is easily rerpoduceable via:

ip link add vethin1 type veth peer name vethout1
ip link add vethin2 type veth peer name vethout2

ip link set vethin1 up
ip link set vethin2 up

ip link set vethout1 up
ip link set vethout2 up

brctl addbr br0
brctl addbr br1

brctl stp br0 on
brctl stp br1 on

brctl addif br0 vethin1
brctl addif br0 vethin2

brctl addif br1 vethout1
brctl addif br1 vethout2

ip link set br0 up
ip link set br1 up

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Ivan Vecera]

2017-05-19 16:45 GMT+02:00 Nikolay Aleksandrov <nikolay@cumulusnetworks.com>:
> On 5/19/17 5:20 PM, Xin Long wrote:
>>
>> Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
>> kernel hello and hold timers"), bridge would not start hello_timer if
>> stp_enabled is not KERNEL_STP when br_dev_open.
>>
>> The problem is even if users set stp_enabled with KERNEL_STP later,
>> the timer will still not be started. It causes that KERNEL_STP can
>> not really work. Users have to re-ifup the bridge to avoid this.
>>
>> This patch is to fix it by starting br->hello_timer when enabling
>> KERNEL_STP in br_stp_start.
>>
>> As an improvement, it's also to start hello_timer again only when
>> br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
>> no reason to start the timer again when it's NO_STP.
>>
>> Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel
>> hello and hold timers")
>> Reported-by: Haidong Li <haili@redhat.com>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>>   net/bridge/br_stp_if.c    | 1 +
>>   net/bridge/br_stp_timer.c | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>
> This doesn't make much sense to me, how do you change from USER_STP to
> KERNEL_STP without first going through NO_STP ?
>
> If you go through NO_STP then all will be fine because br_stp_stop will
> restart
> the timers if the previous val was USER_STP.
>
The problem occurs when KERNEL_STP is enabled if the bridge itself is already
up. Then the hello_timer is not started. If the hello and hold timers
should run only
when KERNEL_STP is used then there are another problematic places
(will send follow-up).

Ivan

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Nikolay Aleksandrov]

On 5/19/17 5:51 PM, Ivan Vecera wrote:
> 2017-05-19 16:45 GMT+02:00 Nikolay Aleksandrov <nikolay@cumulusnetworks.com>:
>> On 5/19/17 5:20 PM, Xin Long wrote:
>>>
>>> Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
>>> kernel hello and hold timers"), bridge would not start hello_timer if
>>> stp_enabled is not KERNEL_STP when br_dev_open.
>>>
>>> The problem is even if users set stp_enabled with KERNEL_STP later,
>>> the timer will still not be started. It causes that KERNEL_STP can
>>> not really work. Users have to re-ifup the bridge to avoid this.
>>>
>>> This patch is to fix it by starting br->hello_timer when enabling
>>> KERNEL_STP in br_stp_start.
>>>
>>> As an improvement, it's also to start hello_timer again only when
>>> br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
>>> no reason to start the timer again when it's NO_STP.
>>>
>>> Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel
>>> hello and hold timers")
>>> Reported-by: Haidong Li <haili@redhat.com>
>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>> ---
>>>    net/bridge/br_stp_if.c    | 1 +
>>>    net/bridge/br_stp_timer.c | 2 +-
>>>    2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>
>> This doesn't make much sense to me, how do you change from USER_STP to
>> KERNEL_STP without first going through NO_STP ?
>>
>> If you go through NO_STP then all will be fine because br_stp_stop will
>> restart
>> the timers if the previous val was USER_STP.
>>
> The problem occurs when KERNEL_STP is enabled if the bridge itself is already
> up. Then the hello_timer is not started. If the hello and hold timers
> should run only
> when KERNEL_STP is used then there are another problematic places
> (will send follow-up).
> 
> Ivan
> 

Oh, the problem seems to be rather going from NO_STP -> KERNEL_STP only
then, because you cannot do direct USER_STP -> KERNEL_STP.

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Nikolay Aleksandrov]

On 5/19/17 5:48 PM, Florian Westphal wrote:
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>> On 5/19/17 5:20 PM, Xin Long wrote:
>>> Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
>>> kernel hello and hold timers"), bridge would not start hello_timer if
>>> stp_enabled is not KERNEL_STP when br_dev_open.
>>>
>>> The problem is even if users set stp_enabled with KERNEL_STP later,
>>> the timer will still not be started. It causes that KERNEL_STP can
>>> not really work. Users have to re-ifup the bridge to avoid this.
>>>
>>> This patch is to fix it by starting br->hello_timer when enabling
>>> KERNEL_STP in br_stp_start.
>>>
>>> As an improvement, it's also to start hello_timer again only when
>>> br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
>>> no reason to start the timer again when it's NO_STP.
>>>
>>> Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel hello and hold timers")
>>> Reported-by: Haidong Li <haili@redhat.com>
>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>> ---
>>>   net/bridge/br_stp_if.c    | 1 +
>>>   net/bridge/br_stp_timer.c | 2 +-
>>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>
>> This doesn't make much sense to me, how do you change from USER_STP to
>> KERNEL_STP without first going through NO_STP ?
> 
> This is easily rerpoduceable via:
> 
> ip link add vethin1 type veth peer name vethout1
> ip link add vethin2 type veth peer name vethout2
> 
> ip link set vethin1 up
> ip link set vethin2 up
> 
> ip link set vethout1 up
> ip link set vethout2 up
> 
> brctl addbr br0
> brctl addbr br1
> 
> brctl stp br0 on
> brctl stp br1 on

I think this step with moving NO_STP -> KERNEL_STP should be last, then
I can see how the timer won't be started.

> 
> brctl addif br0 vethin1
> brctl addif br0 vethin2
> 
> brctl addif br1 vethout1
> brctl addif br1 vethout2
> 
> ip link set br0 up
> ip link set br1 up
> 

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Ivan Vecera]

2017-05-19 16:57 GMT+02:00 Nikolay Aleksandrov <nikolay@cumulusnetworks.com>:
> On 5/19/17 5:51 PM, Ivan Vecera wrote:
>>
>> 2017-05-19 16:45 GMT+02:00 Nikolay Aleksandrov
>> <nikolay@cumulusnetworks.com>:
>>>
>>> On 5/19/17 5:20 PM, Xin Long wrote:
>>>>
>>>>
>>>> Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
>>>> kernel hello and hold timers"), bridge would not start hello_timer if
>>>> stp_enabled is not KERNEL_STP when br_dev_open.
>>>>
>>>> The problem is even if users set stp_enabled with KERNEL_STP later,
>>>> the timer will still not be started. It causes that KERNEL_STP can
>>>> not really work. Users have to re-ifup the bridge to avoid this.
>>>>
>>>> This patch is to fix it by starting br->hello_timer when enabling
>>>> KERNEL_STP in br_stp_start.
>>>>
>>>> As an improvement, it's also to start hello_timer again only when
>>>> br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
>>>> no reason to start the timer again when it's NO_STP.
>>>>
>>>> Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel
>>>> hello and hold timers")
>>>> Reported-by: Haidong Li <haili@redhat.com>
>>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>>> ---
>>>>    net/bridge/br_stp_if.c    | 1 +
>>>>    net/bridge/br_stp_timer.c | 2 +-
>>>>    2 files changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>
>>> This doesn't make much sense to me, how do you change from USER_STP to
>>> KERNEL_STP without first going through NO_STP ?
>>>
>>> If you go through NO_STP then all will be fine because br_stp_stop will
>>> restart
>>> the timers if the previous val was USER_STP.
>>>
>> The problem occurs when KERNEL_STP is enabled if the bridge itself is
>> already
>> up. Then the hello_timer is not started. If the hello and hold timers
>> should run only
>> when KERNEL_STP is used then there are another problematic places
>> (will send follow-up).
>>
>> Ivan
>>
>
> Oh, the problem seems to be rather going from NO_STP -> KERNEL_STP only
> then, because you cannot do direct USER_STP -> KERNEL_STP.
>
No only NO_STP->KERNEL_STP but KERNEL_STP->NO_STP as well as USER_STP->NO_STP:

1) NO_STP->KERNEL_STP issue
hello_timer should be started in br_stp_start() - this patch

2) KERNEL_STP->NO_STP issue
hello timer and hold timers should be stopped (deleted) in br_stp_stop()

3) USER_STP->NO_STP issue
hello timer and hold timers should NOT be started in br_stp_stop()

Ivan

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Nikolay Aleksandrov]

On 5/19/17 6:03 PM, Ivan Vecera wrote:
> 2017-05-19 16:57 GMT+02:00 Nikolay Aleksandrov <nikolay@cumulusnetworks.com>:
>> On 5/19/17 5:51 PM, Ivan Vecera wrote:
>>>
>>> 2017-05-19 16:45 GMT+02:00 Nikolay Aleksandrov
>>> <nikolay@cumulusnetworks.com>:
>>>>
>>>> On 5/19/17 5:20 PM, Xin Long wrote:
>>>>>
>>>>>
>>>>> Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
>>>>> kernel hello and hold timers"), bridge would not start hello_timer if
>>>>> stp_enabled is not KERNEL_STP when br_dev_open.
>>>>>
>>>>> The problem is even if users set stp_enabled with KERNEL_STP later,
>>>>> the timer will still not be started. It causes that KERNEL_STP can
>>>>> not really work. Users have to re-ifup the bridge to avoid this.
>>>>>
>>>>> This patch is to fix it by starting br->hello_timer when enabling
>>>>> KERNEL_STP in br_stp_start.
>>>>>
>>>>> As an improvement, it's also to start hello_timer again only when
>>>>> br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
>>>>> no reason to start the timer again when it's NO_STP.
>>>>>
>>>>> Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel
>>>>> hello and hold timers")
>>>>> Reported-by: Haidong Li <haili@redhat.com>
>>>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>>>> ---
>>>>>     net/bridge/br_stp_if.c    | 1 +
>>>>>     net/bridge/br_stp_timer.c | 2 +-
>>>>>     2 files changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>> This doesn't make much sense to me, how do you change from USER_STP to
>>>> KERNEL_STP without first going through NO_STP ?
>>>>
>>>> If you go through NO_STP then all will be fine because br_stp_stop will
>>>> restart
>>>> the timers if the previous val was USER_STP.
>>>>
>>> The problem occurs when KERNEL_STP is enabled if the bridge itself is
>>> already
>>> up. Then the hello_timer is not started. If the hello and hold timers
>>> should run only
>>> when KERNEL_STP is used then there are another problematic places
>>> (will send follow-up).
>>>
>>> Ivan
>>>
>>
>> Oh, the problem seems to be rather going from NO_STP -> KERNEL_STP only
>> then, because you cannot do direct USER_STP -> KERNEL_STP.
>>
> No only NO_STP->KERNEL_STP but KERNEL_STP->NO_STP as well as USER_STP->NO_STP:
> 
> 1) NO_STP->KERNEL_STP issue
> hello_timer should be started in br_stp_start() - this patch
> 

Right, I was talking only about this patch. By the way what about
the port hold_timers ? This patch only starts the hello_timer.

> 2) KERNEL_STP->NO_STP issue
> hello timer and hold timers should be stopped (deleted) in br_stp_stop()
> 
> 3) USER_STP->NO_STP issue
> hello timer and hold timers should NOT be started in br_stp_stop()
> 

Yep, ack.

> Ivan
> 

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Ivan Vecera]

2017-05-19 17:05 GMT+02:00 Nikolay Aleksandrov <nikolay@cumulusnetworks.com>:
> On 5/19/17 6:03 PM, Ivan Vecera wrote:
>>
>> 2017-05-19 16:57 GMT+02:00 Nikolay Aleksandrov
>> <nikolay@cumulusnetworks.com>:
>>>
>>> On 5/19/17 5:51 PM, Ivan Vecera wrote:
>>>>
>>>>
>>>> 2017-05-19 16:45 GMT+02:00 Nikolay Aleksandrov
>>>> <nikolay@cumulusnetworks.com>:
>>>>>
>>>>>
>>>>> On 5/19/17 5:20 PM, Xin Long wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
>>>>>> kernel hello and hold timers"), bridge would not start hello_timer if
>>>>>> stp_enabled is not KERNEL_STP when br_dev_open.
>>>>>>
>>>>>> The problem is even if users set stp_enabled with KERNEL_STP later,
>>>>>> the timer will still not be started. It causes that KERNEL_STP can
>>>>>> not really work. Users have to re-ifup the bridge to avoid this.
>>>>>>
>>>>>> This patch is to fix it by starting br->hello_timer when enabling
>>>>>> KERNEL_STP in br_stp_start.
>>>>>>
>>>>>> As an improvement, it's also to start hello_timer again only when
>>>>>> br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
>>>>>> no reason to start the timer again when it's NO_STP.
>>>>>>
>>>>>> Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop
>>>>>> kernel
>>>>>> hello and hold timers")
>>>>>> Reported-by: Haidong Li <haili@redhat.com>
>>>>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>>>>> ---
>>>>>>     net/bridge/br_stp_if.c    | 1 +
>>>>>>     net/bridge/br_stp_timer.c | 2 +-
>>>>>>     2 files changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>
>>>>> This doesn't make much sense to me, how do you change from USER_STP to
>>>>> KERNEL_STP without first going through NO_STP ?
>>>>>
>>>>> If you go through NO_STP then all will be fine because br_stp_stop will
>>>>> restart
>>>>> the timers if the previous val was USER_STP.
>>>>>
>>>> The problem occurs when KERNEL_STP is enabled if the bridge itself is
>>>> already
>>>> up. Then the hello_timer is not started. If the hello and hold timers
>>>> should run only
>>>> when KERNEL_STP is used then there are another problematic places
>>>> (will send follow-up).
>>>>
>>>> Ivan
>>>>
>>>
>>> Oh, the problem seems to be rather going from NO_STP -> KERNEL_STP only
>>> then, because you cannot do direct USER_STP -> KERNEL_STP.
>>>
>> No only NO_STP->KERNEL_STP but KERNEL_STP->NO_STP as well as
>> USER_STP->NO_STP:
>>
>> 1) NO_STP->KERNEL_STP issue
>> hello_timer should be started in br_stp_start() - this patch
>>
>
> Right, I was talking only about this patch. By the way what about
> the port hold_timers ? This patch only starts the hello_timer.

The hold_timers should be started indirectly from br_transmit_config()
called from
br_config_bpdu_generation() called from hello_timer_expired() handler.

I.

>> 2) KERNEL_STP->NO_STP issue
>> hello timer and hold timers should be stopped (deleted) in br_stp_stop()
>>
>> 3) USER_STP->NO_STP issue
>> hello timer and hold timers should NOT be started in br_stp_stop()
>>
>
> Yep, ack.
>
>> Ivan
>>
>

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Nikolay Aleksandrov]

On 5/19/17 5:20 PM, Xin Long wrote:
> Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
> kernel hello and hold timers"), bridge would not start hello_timer if
> stp_enabled is not KERNEL_STP when br_dev_open.
> 
> The problem is even if users set stp_enabled with KERNEL_STP later,
> the timer will still not be started. It causes that KERNEL_STP can
> not really work. Users have to re-ifup the bridge to avoid this.
> 
> This patch is to fix it by starting br->hello_timer when enabling
> KERNEL_STP in br_stp_start.
> 
> As an improvement, it's also to start hello_timer again only when
> br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
> no reason to start the timer again when it's NO_STP.
> 
> Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel hello and hold timers")
> Reported-by: Haidong Li <haili@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>   net/bridge/br_stp_if.c    | 1 +
>   net/bridge/br_stp_timer.c | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index 08341d2..0db8102 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -179,6 +179,7 @@ static void br_stp_start(struct net_bridge *br)
>   		br_debug(br, "using kernel STP\n");
>   
>   		/* To start timers on any ports left in blocking */
> +		mod_timer(&br->hello_timer, jiffies + br->hello_time);
>   		br_port_state_selection(br);
>   	}
>   
> diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
> index c98b3e5..60b6fe2 100644
> --- a/net/bridge/br_stp_timer.c
> +++ b/net/bridge/br_stp_timer.c
> @@ -40,7 +40,7 @@ static void br_hello_timer_expired(unsigned long arg)
>   	if (br->dev->flags & IFF_UP) {
>   		br_config_bpdu_generation(br);
>   
> -		if (br->stp_enabled != BR_USER_STP)
> +		if (br->stp_enabled == BR_KERNEL_STP)
>   			mod_timer(&br->hello_timer,
>   				  round_jiffies(jiffies + br->hello_time));
>   	}
> 

+CC Bridge maintainers & fixes commit author

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[Ivan Vecera]

2017-05-19 16:20 GMT+02:00 Xin Long <lucien.xin@gmail.com>:
> Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
> kernel hello and hold timers"), bridge would not start hello_timer if
> stp_enabled is not KERNEL_STP when br_dev_open.
>
> The problem is even if users set stp_enabled with KERNEL_STP later,
> the timer will still not be started. It causes that KERNEL_STP can
> not really work. Users have to re-ifup the bridge to avoid this.
>
> This patch is to fix it by starting br->hello_timer when enabling
> KERNEL_STP in br_stp_start.
>
> As an improvement, it's also to start hello_timer again only when
> br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
> no reason to start the timer again when it's NO_STP.
>
> Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel hello and hold timers")
> Reported-by: Haidong Li <haili@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/bridge/br_stp_if.c    | 1 +
>  net/bridge/br_stp_timer.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index 08341d2..0db8102 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -179,6 +179,7 @@ static void br_stp_start(struct net_bridge *br)
>                 br_debug(br, "using kernel STP\n");
>
>                 /* To start timers on any ports left in blocking */
> +               mod_timer(&br->hello_timer, jiffies + br->hello_time);
>                 br_port_state_selection(br);
>         }
>
> diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
> index c98b3e5..60b6fe2 100644
> --- a/net/bridge/br_stp_timer.c
> +++ b/net/bridge/br_stp_timer.c
> @@ -40,7 +40,7 @@ static void br_hello_timer_expired(unsigned long arg)
>         if (br->dev->flags & IFF_UP) {
>                 br_config_bpdu_generation(br);
>
> -               if (br->stp_enabled != BR_USER_STP)
> +               if (br->stp_enabled == BR_KERNEL_STP)
>                         mod_timer(&br->hello_timer,
>                                   round_jiffies(jiffies + br->hello_time));
>         }
> --
> 2.1.0
>
Reviewed-by: Ivan Vecera <cera@cera.cz>

Re: [PATCH net] bridge: start hello_timer when enabling KERNEL_STP in br_stp_start

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 19 May 2017 22:20:29 +0800

> Since commit 76b91c32dd86 ("bridge: stp: when using userspace stp stop
> kernel hello and hold timers"), bridge would not start hello_timer if
> stp_enabled is not KERNEL_STP when br_dev_open.
> 
> The problem is even if users set stp_enabled with KERNEL_STP later,
> the timer will still not be started. It causes that KERNEL_STP can
> not really work. Users have to re-ifup the bridge to avoid this.
> 
> This patch is to fix it by starting br->hello_timer when enabling
> KERNEL_STP in br_stp_start.
> 
> As an improvement, it's also to start hello_timer again only when
> br->stp_enabled is KERNEL_STP in br_hello_timer_expired, there is
> no reason to start the timer again when it's NO_STP.
> 
> Fixes: 76b91c32dd86 ("bridge: stp: when using userspace stp stop kernel hello and hold timers")
> Reported-by: Haidong Li <haili@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Applied and queued up for -stable, thanks.