[PATCH net 0/2] bridge: multicast behaviour fixes

[PATCH net 0/2] bridge: multicast behaviour fixes

[Nikolay Aleksandrov]

Hi,
Patch 01 fixes a problem when a router is configured and a port goes
through a link down/up, the router configuration was not restored.
Patch 02 starts the multicast querier when using user-space STP and a
port goes to forwarding state.
These are behaviour fixes and if you think they are more appropriate for
net-next, then feel free to apply them there, I've run them with both
net and net-next. Also I've provided "fixes" tags, but since these are
behaviour changes they're the initial implementations of these functions.

Best regards,
 Nikolay Aleksandrov

Satish Ashok (2):
  bridge: multicast: restore router configuration on port link down/up
  bridge: multicast: start querier timer when running user-space stp

 net/bridge/br_multicast.c | 4 ++++
 net/bridge/br_stp.c       | 3 +++
 2 files changed, 7 insertions(+)

-- 
2.4.3

[PATCH net 1/2] bridge: multicast: restore router configuration on port link down/up

[Nikolay Aleksandrov]

From: Satish Ashok <sashok@cumulusnetworks.com>

When a port goes through a link down/up the multicast router configuration
is not restored.

Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Fixes: 0909e11758bd ("bridge: Add multicast_router sysfs entries")
---
 net/bridge/br_multicast.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index ff667e18b2d6..761fc733bf6d 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -37,6 +37,8 @@
 
 static void br_multicast_start_querier(struct net_bridge *br,
 				       struct bridge_mcast_own_query *query);
+static void br_multicast_add_router(struct net_bridge *br,
+				    struct net_bridge_port *port);
 unsigned int br_mdb_rehash_seq;
 
 static inline int br_ip_equal(const struct br_ip *a, const struct br_ip *b)
@@ -936,6 +938,8 @@ void br_multicast_enable_port(struct net_bridge_port *port)
 #if IS_ENABLED(CONFIG_IPV6)
 	br_multicast_enable(&port->ip6_own_query);
 #endif
+	if (port->multicast_router == 2 && hlist_unhashed(&port->rlist))
+		br_multicast_add_router(br, port);
 
 out:
 	spin_unlock(&br->multicast_lock);
-- 
2.4.3

Re: [PATCH net 1/2] bridge: multicast: restore router configuration on port link down/up

[Herbert Xu]

On Wed, Jun 17, 2015 at 04:28:30AM -0700, Nikolay Aleksandrov wrote:
> From: Satish Ashok <sashok@cumulusnetworks.com>
> 
> When a port goes through a link down/up the multicast router configuration
> is not restored.
> 
> Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Fixes: 0909e11758bd ("bridge: Add multicast_router sysfs entries")

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH net 2/2] bridge: multicast: start querier timer when running user-space stp

[Nikolay Aleksandrov]

From: Satish Ashok <sashok@cumulusnetworks.com>

When STP is running in user-space and querier is configured, the
querier timer is not started when a port goes to forwarding state.

Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Fixes: eb1d16414339 ("bridge: Add core IGMP snooping support")
---
 net/bridge/br_stp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index fb3ebe615513..1e2f2f1ff6b0 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -456,6 +456,9 @@ void br_port_state_selection(struct net_bridge *br)
 				p->topology_change_ack = 0;
 				br_make_blocking(p);
 			}
+		} else if (br->stp_enabled == BR_USER_STP &&
+			   p->state == BR_STATE_FORWARDING) {
+			br_multicast_enable_port(p);
 		}
 
 		if (p->state == BR_STATE_FORWARDING)
-- 
2.4.3

Re: [PATCH net 2/2] bridge: multicast: start querier timer when running user-space stp

[Herbert Xu]

On Wed, Jun 17, 2015 at 04:28:31AM -0700, Nikolay Aleksandrov wrote:
> From: Satish Ashok <sashok@cumulusnetworks.com>
> 
> When STP is running in user-space and querier is configured, the
> querier timer is not started when a port goes to forwarding state.
> 
> Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Fixes: eb1d16414339 ("bridge: Add core IGMP snooping support")
> ---
>  net/bridge/br_stp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index fb3ebe615513..1e2f2f1ff6b0 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -456,6 +456,9 @@ void br_port_state_selection(struct net_bridge *br)
>  				p->topology_change_ack = 0;
>  				br_make_blocking(p);
>  			}
> +		} else if (br->stp_enabled == BR_USER_STP &&
> +			   p->state == BR_STATE_FORWARDING) {
> +			br_multicast_enable_port(p);
>  		}

Minor nit, the stp_enabled check appears to be redundant since
you're in the else clause.

More importantly, I'm not sure about the logic.  For kernel STP,
we enable the port as soon as we get out of blocking.  IIRC enabling
the port just means that we start tracking subscriptions/queries
so it should be OK to do even while we're listening/learning.

In any case the logic should be identical whether we use kernel
STP or user-space STP.

So how about removing br_multicast_enable_port from br_make_forward
and just add it here for both kernel and user-space STP?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net 2/2] bridge: multicast: start querier timer when running user-space stp

[Nikolay Aleksandrov]

> On Jun 18, 2015, at 6:37 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
> On Wed, Jun 17, 2015 at 04:28:31AM -0700, Nikolay Aleksandrov wrote:
>> From: Satish Ashok <sashok@cumulusnetworks.com>
>> 
>> When STP is running in user-space and querier is configured, the
>> querier timer is not started when a port goes to forwarding state.
>> 
>> Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> Fixes: eb1d16414339 ("bridge: Add core IGMP snooping support")
>> ---
>> net/bridge/br_stp.c | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
>> index fb3ebe615513..1e2f2f1ff6b0 100644
>> --- a/net/bridge/br_stp.c
>> +++ b/net/bridge/br_stp.c
>> @@ -456,6 +456,9 @@ void br_port_state_selection(struct net_bridge *br)
>> 				p->topology_change_ack = 0;
>> 				br_make_blocking(p);
>> 			}
>> +		} else if (br->stp_enabled == BR_USER_STP &&
>> +			   p->state == BR_STATE_FORWARDING) {
>> +			br_multicast_enable_port(p);
>> 		}
> 
> Minor nit, the stp_enabled check appears to be redundant since
> you're in the else clause.
> 

Right you are, I’ve overlooked it.

> More importantly, I'm not sure about the logic.  For kernel STP,
> we enable the port as soon as we get out of blocking.  IIRC enabling
> the port just means that we start tracking subscriptions/queries
> so it should be OK to do even while we're listening/learning.
> 
> In any case the logic should be identical whether we use kernel
> STP or user-space STP.
> 
> So how about removing br_multicast_enable_port from br_make_forward
> and just add it here for both kernel and user-space STP?
> 
> Thanks,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Makes sense, I’ll re-spin, test and post a v2. Thank you for the suggestion.

Cheers,
 Nik

Re: [PATCH net 0/2] bridge: multicast behaviour fixes

[Nikolay Aleksandrov]

> On Jun 17, 2015, at 2:28 PM, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
> Hi,
> Patch 01 fixes a problem when a router is configured and a port goes
> through a link down/up, the router configuration was not restored.
> Patch 02 starts the multicast querier when using user-space STP and a
> port goes to forwarding state.
> These are behaviour fixes and if you think they are more appropriate for
> net-next, then feel free to apply them there, I've run them with both
> net and net-next. Also I've provided "fixes" tags, but since these are
> behaviour changes they're the initial implementations of these functions.
> 
> Best regards,
> Nikolay Aleksandrov
> 
> Satish Ashok (2):
>  bridge: multicast: restore router configuration on port link down/up
>  bridge: multicast: start querier timer when running user-space stp
> 
> net/bridge/br_multicast.c | 4 ++++
> net/bridge/br_stp.c       | 3 +++
> 2 files changed, 7 insertions(+)
> 
> -- 
> 2.4.3
> 

Dave please drop this series, I’ll post the patches separately because I have
to implement Herbert’s suggestion and would like to repurpose patch 02 for
net-next.

Thanks,
 Nik