[PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Linus Lüssing]

Network managers like netifd (used in OpenWRT for instance) try to
configure interface options after creation but before setting the
interface up.

Unfortunately the sysfs / bridge currently only allows to configure the
hash_max and multicast_router options when the bridge interface is up.
But since br_multicast_init() doesn't start any timers and only sets
default values and initializes timers it should be save to reconfigure
the default values after that, before things actually get active after
the bridge is set up.

With this patch hash_max and multicast_router attributes can be
changed even if the according bridge (port) is down, just like other
other bridge (port) attributes allow too.

Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
Changelog:
* [RFC PATCH net-next] -> [PATCH net-next]


 net/bridge/br_multicast.c |   15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2d69d5c..066199e 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1775,8 +1775,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
 	int err = -ENOENT;
 
 	spin_lock_bh(&br->multicast_lock);
-	if (!netif_running(br->dev))
-		goto unlock;
 
 	switch (val) {
 	case 0:
@@ -1793,7 +1791,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
 		break;
 	}
 
-unlock:
 	spin_unlock_bh(&br->multicast_lock);
 
 	return err;
@@ -1802,18 +1799,15 @@ unlock:
 int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
 {
 	struct net_bridge *br = p->br;
-	int err = -ENOENT;
+	int err = 0;
 
 	spin_lock(&br->multicast_lock);
-	if (!netif_running(br->dev) || p->state == BR_STATE_DISABLED)
-		goto unlock;
 
 	switch (val) {
 	case 0:
 	case 1:
 	case 2:
 		p->multicast_router = val;
-		err = 0;
 
 		if (val < 2 && !hlist_unhashed(&p->rlist))
 			hlist_del_init_rcu(&p->rlist);
@@ -1834,7 +1828,6 @@ int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
 		break;
 	}
 
-unlock:
 	spin_unlock(&br->multicast_lock);
 
 	return err;
@@ -1939,15 +1932,11 @@ unlock:
 
 int br_multicast_set_hash_max(struct net_bridge *br, unsigned long val)
 {
-	int err = -ENOENT;
+	int err = -EINVAL;
 	u32 old;
 	struct net_bridge_mdb_htable *mdb;
 
 	spin_lock_bh(&br->multicast_lock);
-	if (!netif_running(br->dev))
-		goto unlock;
-
-	err = -EINVAL;
 	if (!is_power_of_2(val))
 		goto unlock;
 
-- 
1.7.10.4

[Bridge] [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Linus Lüssing]

Network managers like netifd (used in OpenWRT for instance) try to
configure interface options after creation but before setting the
interface up.

Unfortunately the sysfs / bridge currently only allows to configure the
hash_max and multicast_router options when the bridge interface is up.
But since br_multicast_init() doesn't start any timers and only sets
default values and initializes timers it should be save to reconfigure
the default values after that, before things actually get active after
the bridge is set up.

With this patch hash_max and multicast_router attributes can be
changed even if the according bridge (port) is down, just like other
other bridge (port) attributes allow too.

Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
Changelog:
* [RFC PATCH net-next] -> [PATCH net-next]


 net/bridge/br_multicast.c |   15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2d69d5c..066199e 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1775,8 +1775,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
 	int err = -ENOENT;
 
 	spin_lock_bh(&br->multicast_lock);
-	if (!netif_running(br->dev))
-		goto unlock;
 
 	switch (val) {
 	case 0:
@@ -1793,7 +1791,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
 		break;
 	}
 
-unlock:
 	spin_unlock_bh(&br->multicast_lock);
 
 	return err;
@@ -1802,18 +1799,15 @@ unlock:
 int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
 {
 	struct net_bridge *br = p->br;
-	int err = -ENOENT;
+	int err = 0;
 
 	spin_lock(&br->multicast_lock);
-	if (!netif_running(br->dev) || p->state == BR_STATE_DISABLED)
-		goto unlock;
 
 	switch (val) {
 	case 0:
 	case 1:
 	case 2:
 		p->multicast_router = val;
-		err = 0;
 
 		if (val < 2 && !hlist_unhashed(&p->rlist))
 			hlist_del_init_rcu(&p->rlist);
@@ -1834,7 +1828,6 @@ int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
 		break;
 	}
 
-unlock:
 	spin_unlock(&br->multicast_lock);
 
 	return err;
@@ -1939,15 +1932,11 @@ unlock:
 
 int br_multicast_set_hash_max(struct net_bridge *br, unsigned long val)
 {
-	int err = -ENOENT;
+	int err = -EINVAL;
 	u32 old;
 	struct net_bridge_mdb_htable *mdb;
 
 	spin_lock_bh(&br->multicast_lock);
-	if (!netif_running(br->dev))
-		goto unlock;
-
-	err = -EINVAL;
 	if (!is_power_of_2(val))
 		goto unlock;
 
-- 
1.7.10.4

Re: [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[roopa]

On 5/21/15, 5:56 PM, Linus Lüssing wrote:
> Network managers like netifd (used in OpenWRT for instance) try to
> configure interface options after creation but before setting the
> interface up.
>
> Unfortunately the sysfs / bridge currently only allows to configure the
> hash_max and multicast_router options when the bridge interface is up.
> But since br_multicast_init() doesn't start any timers and only sets
> default values and initializes timers it should be save to reconfigure
> the default values after that, before things actually get active after
> the bridge is set up.
>
> With this patch hash_max and multicast_router attributes can be
> changed even if the according bridge (port) is down, just like other
> other bridge (port) attributes allow too.
>
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---
> Changelog:
> * [RFC PATCH net-next] -> [PATCH net-next]
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>

we have a similar patch in-house waiting to be up-streamed.

thanks.

Re: [Bridge] [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[roopa]

On 5/21/15, 5:56 PM, Linus Lüssing wrote:
> Network managers like netifd (used in OpenWRT for instance) try to
> configure interface options after creation but before setting the
> interface up.
>
> Unfortunately the sysfs / bridge currently only allows to configure the
> hash_max and multicast_router options when the bridge interface is up.
> But since br_multicast_init() doesn't start any timers and only sets
> default values and initializes timers it should be save to reconfigure
> the default values after that, before things actually get active after
> the bridge is set up.
>
> With this patch hash_max and multicast_router attributes can be
> changed even if the according bridge (port) is down, just like other
> other bridge (port) attributes allow too.
>
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---
> Changelog:
> * [RFC PATCH net-next] -> [PATCH net-next]
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>

we have a similar patch in-house waiting to be up-streamed.

thanks.

Re: [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Cong Wang]

On Thu, May 21, 2015 at 5:56 PM, Linus Lüssing <linus.luessing@c0d3.blue> wrote:
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 2d69d5c..066199e 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1775,8 +1775,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>         int err = -ENOENT;
>

Nit: err can be initialized to -EINVAL now.

>         spin_lock_bh(&br->multicast_lock);
> -       if (!netif_running(br->dev))
> -               goto unlock;
>
>         switch (val) {
>         case 0:
> @@ -1793,7 +1791,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>                 break;
>         }
>
> -unlock:
>         spin_unlock_bh(&br->multicast_lock);
>
>         return err;
> @@ -1802,18 +1799,15 @@ unlock:
>  int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
>  {
>         struct net_bridge *br = p->br;
> -       int err = -ENOENT;
> +       int err = 0;
>
>         spin_lock(&br->multicast_lock);


Not related with your patch, but why we don't need to disable bh here?

For me it looks like we do use p->rlist in BH context, but I could easily
miss something here.

> -       if (!netif_running(br->dev) || p->state == BR_STATE_DISABLED)
> -               goto unlock;
>
>         switch (val) {
>         case 0:
>         case 1:
>         case 2:
>                 p->multicast_router = val;
> -               err = 0;
>
>                 if (val < 2 && !hlist_unhashed(&p->rlist))
>                         hlist_del_init_rcu(&p->rlist);
> @@ -1834,7 +1828,6 @@ int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
>                 break;
>         }
>
> -unlock:
>         spin_unlock(&br->multicast_lock);
>
>         return err;


Thanks!

Re: [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Cong Wang]

On Thu, May 21, 2015 at 5:56 PM, Linus Lüssing <linus.luessing@c0d3.blue> wrote:
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 2d69d5c..066199e 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1775,8 +1775,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>         int err = -ENOENT;
>

Nit: err can be initialized to -EINVAL now.

>         spin_lock_bh(&br->multicast_lock);
> -       if (!netif_running(br->dev))
> -               goto unlock;
>
>         switch (val) {
>         case 0:
> @@ -1793,7 +1791,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>                 break;
>         }
>
> -unlock:
>         spin_unlock_bh(&br->multicast_lock);
>
>         return err;
> @@ -1802,18 +1799,15 @@ unlock:
>  int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
>  {
>         struct net_bridge *br = p->br;
> -       int err = -ENOENT;
> +       int err = 0;
>
>         spin_lock(&br->multicast_lock);


Not related with your patch, but why we don't need to disable bh here?

For me it looks like we do use p->rlist in BH context, but I could easily
miss something here.

> -       if (!netif_running(br->dev) || p->state == BR_STATE_DISABLED)
> -               goto unlock;
>
>         switch (val) {
>         case 0:
>         case 1:
>         case 2:
>                 p->multicast_router = val;
> -               err = 0;
>
>                 if (val < 2 && !hlist_unhashed(&p->rlist))
>                         hlist_del_init_rcu(&p->rlist);
> @@ -1834,7 +1828,6 @@ int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
>                 break;
>         }
>
> -unlock:
>         spin_unlock(&br->multicast_lock);
>
>         return err;


Thanks!

Re: [Bridge] [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Cong Wang]

On Thu, May 21, 2015 at 5:56 PM, Linus Lüssing <linus.luessing@c0d3.blue> wrote:
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 2d69d5c..066199e 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1775,8 +1775,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>         int err = -ENOENT;
>

Nit: err can be initialized to -EINVAL now.

>         spin_lock_bh(&br->multicast_lock);
> -       if (!netif_running(br->dev))
> -               goto unlock;
>
>         switch (val) {
>         case 0:
> @@ -1793,7 +1791,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>                 break;
>         }
>
> -unlock:
>         spin_unlock_bh(&br->multicast_lock);
>
>         return err;
> @@ -1802,18 +1799,15 @@ unlock:
>  int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
>  {
>         struct net_bridge *br = p->br;
> -       int err = -ENOENT;
> +       int err = 0;
>
>         spin_lock(&br->multicast_lock);


Not related with your patch, but why we don't need to disable bh here?

For me it looks like we do use p->rlist in BH context, but I could easily
miss something here.

> -       if (!netif_running(br->dev) || p->state == BR_STATE_DISABLED)
> -               goto unlock;
>
>         switch (val) {
>         case 0:
>         case 1:
>         case 2:
>                 p->multicast_router = val;
> -               err = 0;
>
>                 if (val < 2 && !hlist_unhashed(&p->rlist))
>                         hlist_del_init_rcu(&p->rlist);
> @@ -1834,7 +1828,6 @@ int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
>                 break;
>         }
>
> -unlock:
>         spin_unlock(&br->multicast_lock);
>
>         return err;


Thanks!

Re: [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Herbert Xu]

On Fri, May 22, 2015 at 02:56:01AM +0200, Linus Lüssing wrote:
> Network managers like netifd (used in OpenWRT for instance) try to
> configure interface options after creation but before setting the
> interface up.
> 
> Unfortunately the sysfs / bridge currently only allows to configure the
> hash_max and multicast_router options when the bridge interface is up.
> But since br_multicast_init() doesn't start any timers and only sets
> default values and initializes timers it should be save to reconfigure
> the default values after that, before things actually get active after
> the bridge is set up.
> 
> With this patch hash_max and multicast_router attributes can be
> changed even if the according bridge (port) is down, just like other
> other bridge (port) attributes allow too.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>

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

Re: [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Herbert Xu]

On Fri, May 22, 2015 at 02:56:01AM +0200, Linus Lüssing wrote:
> Network managers like netifd (used in OpenWRT for instance) try to
> configure interface options after creation but before setting the
> interface up.
> 
> Unfortunately the sysfs / bridge currently only allows to configure the
> hash_max and multicast_router options when the bridge interface is up.
> But since br_multicast_init() doesn't start any timers and only sets
> default values and initializes timers it should be save to reconfigure
> the default values after that, before things actually get active after
> the bridge is set up.
> 
> With this patch hash_max and multicast_router attributes can be
> changed even if the according bridge (port) is down, just like other
> other bridge (port) attributes allow too.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>

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

Re: [Bridge] [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Herbert Xu]

On Fri, May 22, 2015 at 02:56:01AM +0200, Linus Lüssing wrote:
> Network managers like netifd (used in OpenWRT for instance) try to
> configure interface options after creation but before setting the
> interface up.
> 
> Unfortunately the sysfs / bridge currently only allows to configure the
> hash_max and multicast_router options when the bridge interface is up.
> But since br_multicast_init() doesn't start any timers and only sets
> default values and initializes timers it should be save to reconfigure
> the default values after that, before things actually get active after
> the bridge is set up.
> 
> With this patch hash_max and multicast_router attributes can be
> changed even if the according bridge (port) is down, just like other
> other bridge (port) attributes allow too.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>

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

Re: [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Herbert Xu]

On Thu, May 21, 2015 at 08:11:32PM -0700, Cong Wang wrote:
>
> For me it looks like we do use p->rlist in BH context, but I could easily
> miss something here.

Because the caller disables BH for us.

Cheers,
-- 
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: [Bridge] [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Herbert Xu]

On Thu, May 21, 2015 at 08:11:32PM -0700, Cong Wang wrote:
>
> For me it looks like we do use p->rlist in BH context, but I could easily
> miss something here.

Because the caller disables BH for us.

Cheers,
-- 
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-next] bridge: allow setting hash_max + multicast_router if interface is down

[Cong Wang]

On Thu, May 21, 2015 at 8:17 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, May 21, 2015 at 08:11:32PM -0700, Cong Wang wrote:
>>
>> For me it looks like we do use p->rlist in BH context, but I could easily
>> miss something here.
>
> Because the caller disables BH for us.
>


Right, I thought port attr setting uses the same way with br attr setting.

Re: [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Cong Wang]

On Thu, May 21, 2015 at 8:17 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, May 21, 2015 at 08:11:32PM -0700, Cong Wang wrote:
>>
>> For me it looks like we do use p->rlist in BH context, but I could easily
>> miss something here.
>
> Because the caller disables BH for us.
>


Right, I thought port attr setting uses the same way with br attr setting.

Re: [Bridge] [PATCH net-next] bridge: allow setting hash_max + multicast_router if interface is down

[Cong Wang]

On Thu, May 21, 2015 at 8:17 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, May 21, 2015 at 08:11:32PM -0700, Cong Wang wrote:
>>
>> For me it looks like we do use p->rlist in BH context, but I could easily
>> miss something here.
>
> Because the caller disables BH for us.
>


Right, I thought port attr setting uses the same way with br attr setting.