linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] net: bridge: mrp: Add small fixes to MRP
@ 2020-05-20 13:09 Horatiu Vultur
  2020-05-20 13:09 ` [PATCH 1/3] bridge: mrp: Add br_mrp_unique_ifindex function Horatiu Vultur
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Horatiu Vultur @ 2020-05-20 13:09 UTC (permalink / raw)
  To: jiri, ivecera, davem, kuba, roopa, nikolay, andrew,
	UNGLinuxDriver, netdev, linux-kernel, bridge
  Cc: Horatiu Vultur

This patch series adds small fixes to MRP implementation.
The following are fixed in this patch series:
- now is not allow to add the same port to multiple MRP rings
- remove unused variable
- restore the port state according to the bridge state when the MRP instance
  is deleted

Horatiu Vultur (3):
  bridge: mrp: Add br_mrp_unique_ifindex function
  switchdev: mrp: Remove the variable mrp_ring_state
  bridge: mrp: Restore port state when deleting MRP instance

 include/net/switchdev.h |  1 -
 net/bridge/br_mrp.c     | 44 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 40 insertions(+), 5 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] bridge: mrp: Add br_mrp_unique_ifindex function
  2020-05-20 13:09 [PATCH 0/3] net: bridge: mrp: Add small fixes to MRP Horatiu Vultur
@ 2020-05-20 13:09 ` Horatiu Vultur
  2020-05-21  8:16   ` Nikolay Aleksandrov
  2020-05-20 13:09 ` [PATCH 2/3] switchdev: mrp: Remove the variable mrp_ring_state Horatiu Vultur
  2020-05-20 13:09 ` [PATCH 3/3] bridge: mrp: Restore port state when deleting MRP instance Horatiu Vultur
  2 siblings, 1 reply; 10+ messages in thread
From: Horatiu Vultur @ 2020-05-20 13:09 UTC (permalink / raw)
  To: jiri, ivecera, davem, kuba, roopa, nikolay, andrew,
	UNGLinuxDriver, netdev, linux-kernel, bridge
  Cc: Horatiu Vultur

It is not allow to have the same net bridge port part of multiple MRP
rings. Therefore add a check if the port is used already in a different
MRP. In that case return failure.

Fixes: 9a9f26e8f7ea ("bridge: mrp: Connect MRP API with the switchdev API")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_mrp.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index d7bc09de4c139..a5a3fa59c078a 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -37,6 +37,32 @@ static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
 	return res;
 }
 
+static bool br_mrp_unique_ifindex(struct net_bridge *br, u32 ifindex)
+{
+	struct br_mrp *mrp;
+	bool res = true;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(mrp, &br->mrp_list, list,
+				lockdep_rtnl_is_held()) {
+		struct net_bridge_port *p;
+
+		p = rcu_dereference(mrp->p_port);
+		if (p && p->dev->ifindex == ifindex) {
+			res = false;
+			break;
+		}
+
+		p = rcu_dereference(mrp->s_port);
+		if (p && p->dev->ifindex == ifindex) {
+			res = false;
+			break;
+		}
+	}
+	rcu_read_unlock();
+	return res;
+}
+
 static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
 				       struct net_bridge_port *p)
 {
@@ -255,6 +281,11 @@ int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
 	    !br_mrp_get_port(br, instance->s_ifindex))
 		return -EINVAL;
 
+	/* It is not possible to have the same port part of multiple rings */
+	if (!br_mrp_unique_ifindex(br, instance->p_ifindex) ||
+	    !br_mrp_unique_ifindex(br, instance->s_ifindex))
+		return -EINVAL;
+
 	mrp = kzalloc(sizeof(*mrp), GFP_KERNEL);
 	if (!mrp)
 		return -ENOMEM;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/3] switchdev: mrp: Remove the variable mrp_ring_state
  2020-05-20 13:09 [PATCH 0/3] net: bridge: mrp: Add small fixes to MRP Horatiu Vultur
  2020-05-20 13:09 ` [PATCH 1/3] bridge: mrp: Add br_mrp_unique_ifindex function Horatiu Vultur
@ 2020-05-20 13:09 ` Horatiu Vultur
  2020-05-20 15:56   ` Ivan Vecera
  2020-05-20 13:09 ` [PATCH 3/3] bridge: mrp: Restore port state when deleting MRP instance Horatiu Vultur
  2 siblings, 1 reply; 10+ messages in thread
From: Horatiu Vultur @ 2020-05-20 13:09 UTC (permalink / raw)
  To: jiri, ivecera, davem, kuba, roopa, nikolay, andrew,
	UNGLinuxDriver, netdev, linux-kernel, bridge
  Cc: Horatiu Vultur

Remove the variable mrp_ring_state from switchdev_attr because is not
used anywhere.
The ring state is set using SWITCHDEV_OBJ_ID_RING_STATE_MRP.

Fixes: c284b5459008 ("switchdev: mrp: Extend switchdev API to offload MRP")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 include/net/switchdev.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index ae7aeb0d1f9ca..db519957e134b 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -62,7 +62,6 @@ struct switchdev_attr {
 #if IS_ENABLED(CONFIG_BRIDGE_MRP)
 		u8 mrp_port_state;			/* MRP_PORT_STATE */
 		u8 mrp_port_role;			/* MRP_PORT_ROLE */
-		u8 mrp_ring_state;			/* MRP_RING_STATE */
 #endif
 	} u;
 };
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/3] bridge: mrp: Restore port state when deleting MRP instance
  2020-05-20 13:09 [PATCH 0/3] net: bridge: mrp: Add small fixes to MRP Horatiu Vultur
  2020-05-20 13:09 ` [PATCH 1/3] bridge: mrp: Add br_mrp_unique_ifindex function Horatiu Vultur
  2020-05-20 13:09 ` [PATCH 2/3] switchdev: mrp: Remove the variable mrp_ring_state Horatiu Vultur
@ 2020-05-20 13:09 ` Horatiu Vultur
  2020-05-21  8:18   ` Nikolay Aleksandrov
  2 siblings, 1 reply; 10+ messages in thread
From: Horatiu Vultur @ 2020-05-20 13:09 UTC (permalink / raw)
  To: jiri, ivecera, davem, kuba, roopa, nikolay, andrew,
	UNGLinuxDriver, netdev, linux-kernel, bridge
  Cc: Horatiu Vultur

When a MRP instance is deleted, then restore the port according to the
bridge state. If the bridge is up then the ports will be in forwarding
state otherwise will be in disabled state.

Fixes: 9a9f26e8f7ea ("bridge: mrp: Connect MRP API with the switchdev API")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_mrp.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index a5a3fa59c078a..bdd8920c15053 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -229,6 +229,7 @@ static void br_mrp_test_work_expired(struct work_struct *work)
 static void br_mrp_del_impl(struct net_bridge *br, struct br_mrp *mrp)
 {
 	struct net_bridge_port *p;
+	u8 state;
 
 	/* Stop sending MRP_Test frames */
 	cancel_delayed_work_sync(&mrp->test_work);
@@ -240,20 +241,24 @@ static void br_mrp_del_impl(struct net_bridge *br, struct br_mrp *mrp)
 	p = rtnl_dereference(mrp->p_port);
 	if (p) {
 		spin_lock_bh(&br->lock);
-		p->state = BR_STATE_FORWARDING;
+		state = netif_running(br->dev) ?
+				BR_STATE_FORWARDING : BR_STATE_DISABLED;
+		p->state = state;
 		p->flags &= ~BR_MRP_AWARE;
 		spin_unlock_bh(&br->lock);
-		br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
+		br_mrp_port_switchdev_set_state(p, state);
 		rcu_assign_pointer(mrp->p_port, NULL);
 	}
 
 	p = rtnl_dereference(mrp->s_port);
 	if (p) {
 		spin_lock_bh(&br->lock);
-		p->state = BR_STATE_FORWARDING;
+		state = netif_running(br->dev) ?
+				BR_STATE_FORWARDING : BR_STATE_DISABLED;
+		p->state = state;
 		p->flags &= ~BR_MRP_AWARE;
 		spin_unlock_bh(&br->lock);
-		br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
+		br_mrp_port_switchdev_set_state(p, state);
 		rcu_assign_pointer(mrp->s_port, NULL);
 	}
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] switchdev: mrp: Remove the variable mrp_ring_state
  2020-05-20 13:09 ` [PATCH 2/3] switchdev: mrp: Remove the variable mrp_ring_state Horatiu Vultur
@ 2020-05-20 15:56   ` Ivan Vecera
  0 siblings, 0 replies; 10+ messages in thread
From: Ivan Vecera @ 2020-05-20 15:56 UTC (permalink / raw)
  To: Horatiu Vultur
  Cc: jiri, davem, kuba, roopa, nikolay, andrew, UNGLinuxDriver,
	netdev, linux-kernel, bridge

On Wed, 20 May 2020 13:09:22 +0000
Horatiu Vultur <horatiu.vultur@microchip.com> wrote:

> Remove the variable mrp_ring_state from switchdev_attr because is not
> used anywhere.
> The ring state is set using SWITCHDEV_OBJ_ID_RING_STATE_MRP.
> 
> Fixes: c284b5459008 ("switchdev: mrp: Extend switchdev API to offload MRP")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  include/net/switchdev.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index ae7aeb0d1f9ca..db519957e134b 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -62,7 +62,6 @@ struct switchdev_attr {
>  #if IS_ENABLED(CONFIG_BRIDGE_MRP)
>  		u8 mrp_port_state;			/* MRP_PORT_STATE */
>  		u8 mrp_port_role;			/* MRP_PORT_ROLE */
> -		u8 mrp_ring_state;			/* MRP_RING_STATE */
>  #endif
>  	} u;
>  };

Acked-by: Ivan Vecera <ivecera@redhat.com>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] bridge: mrp: Add br_mrp_unique_ifindex function
  2020-05-20 13:09 ` [PATCH 1/3] bridge: mrp: Add br_mrp_unique_ifindex function Horatiu Vultur
@ 2020-05-21  8:16   ` Nikolay Aleksandrov
  2020-05-21 18:49     ` Horatiu Vultur
  0 siblings, 1 reply; 10+ messages in thread
From: Nikolay Aleksandrov @ 2020-05-21  8:16 UTC (permalink / raw)
  To: Horatiu Vultur, jiri, ivecera, davem, kuba, roopa, andrew,
	UNGLinuxDriver, netdev, linux-kernel, bridge

On 20/05/2020 16:09, Horatiu Vultur wrote:
> It is not allow to have the same net bridge port part of multiple MRP
> rings. Therefore add a check if the port is used already in a different
> MRP. In that case return failure.
> 
> Fixes: 9a9f26e8f7ea ("bridge: mrp: Connect MRP API with the switchdev API")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  net/bridge/br_mrp.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> index d7bc09de4c139..a5a3fa59c078a 100644
> --- a/net/bridge/br_mrp.c
> +++ b/net/bridge/br_mrp.c
> @@ -37,6 +37,32 @@ static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
>  	return res;
>  }
>  
> +static bool br_mrp_unique_ifindex(struct net_bridge *br, u32 ifindex)
> +{
> +	struct br_mrp *mrp;
> +	bool res = true;
> +
> +	rcu_read_lock();

Why do you need the rcu_read_lock() here when lockdep_rtnl_is_held() is used?
You should be able to just do rtnl_dereference() below as this is used only
under rtnl.

> +	list_for_each_entry_rcu(mrp, &br->mrp_list, list,
> +				lockdep_rtnl_is_held()) {
> +		struct net_bridge_port *p;
> +
> +		p = rcu_dereference(mrp->p_port);
> +		if (p && p->dev->ifindex == ifindex) {
> +			res = false;
> +			break;
> +		}
> +
> +		p = rcu_dereference(mrp->s_port);
> +		if (p && p->dev->ifindex == ifindex) {
> +			res = false;
> +			break;
> +		}
> +	}
> +	rcu_read_unlock();
> +	return res;
> +}
> +
>  static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
>  				       struct net_bridge_port *p)
>  {
> @@ -255,6 +281,11 @@ int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
>  	    !br_mrp_get_port(br, instance->s_ifindex))
>  		return -EINVAL;
>  
> +	/* It is not possible to have the same port part of multiple rings */
> +	if (!br_mrp_unique_ifindex(br, instance->p_ifindex) ||
> +	    !br_mrp_unique_ifindex(br, instance->s_ifindex))
> +		return -EINVAL;
> +
>  	mrp = kzalloc(sizeof(*mrp), GFP_KERNEL);
>  	if (!mrp)
>  		return -ENOMEM;
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] bridge: mrp: Restore port state when deleting MRP instance
  2020-05-20 13:09 ` [PATCH 3/3] bridge: mrp: Restore port state when deleting MRP instance Horatiu Vultur
@ 2020-05-21  8:18   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 10+ messages in thread
From: Nikolay Aleksandrov @ 2020-05-21  8:18 UTC (permalink / raw)
  To: Horatiu Vultur, jiri, ivecera, davem, kuba, roopa, andrew,
	UNGLinuxDriver, netdev, linux-kernel, bridge

On 20/05/2020 16:09, Horatiu Vultur wrote:
> When a MRP instance is deleted, then restore the port according to the
> bridge state. If the bridge is up then the ports will be in forwarding
> state otherwise will be in disabled state.
> 
> Fixes: 9a9f26e8f7ea ("bridge: mrp: Connect MRP API with the switchdev API")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  net/bridge/br_mrp.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 

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

> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> index a5a3fa59c078a..bdd8920c15053 100644
> --- a/net/bridge/br_mrp.c
> +++ b/net/bridge/br_mrp.c
> @@ -229,6 +229,7 @@ static void br_mrp_test_work_expired(struct work_struct *work)
>  static void br_mrp_del_impl(struct net_bridge *br, struct br_mrp *mrp)
>  {
>  	struct net_bridge_port *p;
> +	u8 state;
>  
>  	/* Stop sending MRP_Test frames */
>  	cancel_delayed_work_sync(&mrp->test_work);
> @@ -240,20 +241,24 @@ static void br_mrp_del_impl(struct net_bridge *br, struct br_mrp *mrp)
>  	p = rtnl_dereference(mrp->p_port);
>  	if (p) {
>  		spin_lock_bh(&br->lock);
> -		p->state = BR_STATE_FORWARDING;
> +		state = netif_running(br->dev) ?
> +				BR_STATE_FORWARDING : BR_STATE_DISABLED;
> +		p->state = state;
>  		p->flags &= ~BR_MRP_AWARE;
>  		spin_unlock_bh(&br->lock);
> -		br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
> +		br_mrp_port_switchdev_set_state(p, state);
>  		rcu_assign_pointer(mrp->p_port, NULL);
>  	}
>  
>  	p = rtnl_dereference(mrp->s_port);
>  	if (p) {
>  		spin_lock_bh(&br->lock);
> -		p->state = BR_STATE_FORWARDING;
> +		state = netif_running(br->dev) ?
> +				BR_STATE_FORWARDING : BR_STATE_DISABLED;
> +		p->state = state;
>  		p->flags &= ~BR_MRP_AWARE;
>  		spin_unlock_bh(&br->lock);
> -		br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
> +		br_mrp_port_switchdev_set_state(p, state);
>  		rcu_assign_pointer(mrp->s_port, NULL);
>  	}
>  
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] bridge: mrp: Add br_mrp_unique_ifindex function
  2020-05-21 18:49     ` Horatiu Vultur
@ 2020-05-21 16:58       ` Nikolay Aleksandrov
  2020-05-21 19:30         ` Horatiu Vultur
  0 siblings, 1 reply; 10+ messages in thread
From: Nikolay Aleksandrov @ 2020-05-21 16:58 UTC (permalink / raw)
  To: Horatiu Vultur
  Cc: jiri, ivecera, davem, kuba, roopa, andrew, UNGLinuxDriver,
	netdev, linux-kernel, bridge

On 21/05/2020 21:49, Horatiu Vultur wrote:
> The 05/21/2020 11:16, Nikolay Aleksandrov wrote:
>> On 20/05/2020 16:09, Horatiu Vultur wrote:
>>> It is not allow to have the same net bridge port part of multiple MRP
>>> rings. Therefore add a check if the port is used already in a different
>>> MRP. In that case return failure.
>>>
>>> Fixes: 9a9f26e8f7ea ("bridge: mrp: Connect MRP API with the switchdev API")
>>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
>>> ---
>>>  net/bridge/br_mrp.c | 31 +++++++++++++++++++++++++++++++
>>>  1 file changed, 31 insertions(+)
>>>
>>> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
>>> index d7bc09de4c139..a5a3fa59c078a 100644
>>> --- a/net/bridge/br_mrp.c
>>> +++ b/net/bridge/br_mrp.c
>>> @@ -37,6 +37,32 @@ static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
>>>       return res;
>>>  }
>>>
>>> +static bool br_mrp_unique_ifindex(struct net_bridge *br, u32 ifindex)
>>> +{
>>> +     struct br_mrp *mrp;
>>> +     bool res = true;
>>> +
>>> +     rcu_read_lock();
>>
>> Why do you need the rcu_read_lock() here when lockdep_rtnl_is_held() is used?
>> You should be able to just do rtnl_dereference() below as this is used only
>> under rtnl.
> 
> Hi Nik,
> 
> Also initially I thought that is not needed, but when I enabled all the
> RCU debug configs to see if I use correctly the RCU, I got a warning
> regarding suspicious RCU usage.
> And that is the reason why I have put it.
> 

Did you try using rtnl_dereference() instead of rcu_dereference() ?

>>
>>> +     list_for_each_entry_rcu(mrp, &br->mrp_list, list,
>>> +                             lockdep_rtnl_is_held()) {
>>> +             struct net_bridge_port *p;
>>> +
>>> +             p = rcu_dereference(mrp->p_port);
>>> +             if (p && p->dev->ifindex == ifindex) {
>>> +                     res = false;
>>> +                     break;
>>> +             }
>>> +
>>> +             p = rcu_dereference(mrp->s_port);
>>> +             if (p && p->dev->ifindex == ifindex) {
>>> +                     res = false;
>>> +                     break;
>>> +             }
>>> +     }
>>> +     rcu_read_unlock();
>>> +     return res;
>>> +}
>>> +
>>>  static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
>>>                                      struct net_bridge_port *p)
>>>  {
>>> @@ -255,6 +281,11 @@ int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
>>>           !br_mrp_get_port(br, instance->s_ifindex))
>>>               return -EINVAL;
>>>
>>> +     /* It is not possible to have the same port part of multiple rings */
>>> +     if (!br_mrp_unique_ifindex(br, instance->p_ifindex) ||
>>> +         !br_mrp_unique_ifindex(br, instance->s_ifindex))
>>> +             return -EINVAL;
>>> +
>>>       mrp = kzalloc(sizeof(*mrp), GFP_KERNEL);
>>>       if (!mrp)
>>>               return -ENOMEM;
>>>
>>
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] bridge: mrp: Add br_mrp_unique_ifindex function
  2020-05-21  8:16   ` Nikolay Aleksandrov
@ 2020-05-21 18:49     ` Horatiu Vultur
  2020-05-21 16:58       ` Nikolay Aleksandrov
  0 siblings, 1 reply; 10+ messages in thread
From: Horatiu Vultur @ 2020-05-21 18:49 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: jiri, ivecera, davem, kuba, roopa, andrew, UNGLinuxDriver,
	netdev, linux-kernel, bridge

The 05/21/2020 11:16, Nikolay Aleksandrov wrote:
> On 20/05/2020 16:09, Horatiu Vultur wrote:
> > It is not allow to have the same net bridge port part of multiple MRP
> > rings. Therefore add a check if the port is used already in a different
> > MRP. In that case return failure.
> >
> > Fixes: 9a9f26e8f7ea ("bridge: mrp: Connect MRP API with the switchdev API")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  net/bridge/br_mrp.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> > index d7bc09de4c139..a5a3fa59c078a 100644
> > --- a/net/bridge/br_mrp.c
> > +++ b/net/bridge/br_mrp.c
> > @@ -37,6 +37,32 @@ static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
> >       return res;
> >  }
> >
> > +static bool br_mrp_unique_ifindex(struct net_bridge *br, u32 ifindex)
> > +{
> > +     struct br_mrp *mrp;
> > +     bool res = true;
> > +
> > +     rcu_read_lock();
> 
> Why do you need the rcu_read_lock() here when lockdep_rtnl_is_held() is used?
> You should be able to just do rtnl_dereference() below as this is used only
> under rtnl.

Hi Nik,

Also initially I thought that is not needed, but when I enabled all the
RCU debug configs to see if I use correctly the RCU, I got a warning
regarding suspicious RCU usage.
And that is the reason why I have put it.

> 
> > +     list_for_each_entry_rcu(mrp, &br->mrp_list, list,
> > +                             lockdep_rtnl_is_held()) {
> > +             struct net_bridge_port *p;
> > +
> > +             p = rcu_dereference(mrp->p_port);
> > +             if (p && p->dev->ifindex == ifindex) {
> > +                     res = false;
> > +                     break;
> > +             }
> > +
> > +             p = rcu_dereference(mrp->s_port);
> > +             if (p && p->dev->ifindex == ifindex) {
> > +                     res = false;
> > +                     break;
> > +             }
> > +     }
> > +     rcu_read_unlock();
> > +     return res;
> > +}
> > +
> >  static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
> >                                      struct net_bridge_port *p)
> >  {
> > @@ -255,6 +281,11 @@ int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
> >           !br_mrp_get_port(br, instance->s_ifindex))
> >               return -EINVAL;
> >
> > +     /* It is not possible to have the same port part of multiple rings */
> > +     if (!br_mrp_unique_ifindex(br, instance->p_ifindex) ||
> > +         !br_mrp_unique_ifindex(br, instance->s_ifindex))
> > +             return -EINVAL;
> > +
> >       mrp = kzalloc(sizeof(*mrp), GFP_KERNEL);
> >       if (!mrp)
> >               return -ENOMEM;
> >
> 

-- 
/Horatiu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] bridge: mrp: Add br_mrp_unique_ifindex function
  2020-05-21 16:58       ` Nikolay Aleksandrov
@ 2020-05-21 19:30         ` Horatiu Vultur
  0 siblings, 0 replies; 10+ messages in thread
From: Horatiu Vultur @ 2020-05-21 19:30 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: jiri, ivecera, davem, kuba, roopa, andrew, UNGLinuxDriver,
	netdev, linux-kernel, bridge

The 05/21/2020 19:58, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 21/05/2020 21:49, Horatiu Vultur wrote:
> > The 05/21/2020 11:16, Nikolay Aleksandrov wrote:
> >> On 20/05/2020 16:09, Horatiu Vultur wrote:
> >>> It is not allow to have the same net bridge port part of multiple MRP
> >>> rings. Therefore add a check if the port is used already in a different
> >>> MRP. In that case return failure.
> >>>
> >>> Fixes: 9a9f26e8f7ea ("bridge: mrp: Connect MRP API with the switchdev API")
> >>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> >>> ---
> >>>  net/bridge/br_mrp.c | 31 +++++++++++++++++++++++++++++++
> >>>  1 file changed, 31 insertions(+)
> >>>
> >>> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> >>> index d7bc09de4c139..a5a3fa59c078a 100644
> >>> --- a/net/bridge/br_mrp.c
> >>> +++ b/net/bridge/br_mrp.c
> >>> @@ -37,6 +37,32 @@ static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
> >>>       return res;
> >>>  }
> >>>
> >>> +static bool br_mrp_unique_ifindex(struct net_bridge *br, u32 ifindex)
> >>> +{
> >>> +     struct br_mrp *mrp;
> >>> +     bool res = true;
> >>> +
> >>> +     rcu_read_lock();
> >>
> >> Why do you need the rcu_read_lock() here when lockdep_rtnl_is_held() is used?
> >> You should be able to just do rtnl_dereference() below as this is used only
> >> under rtnl.
> >
> > Hi Nik,
> >
> > Also initially I thought that is not needed, but when I enabled all the
> > RCU debug configs to see if I use correctly the RCU, I got a warning
> > regarding suspicious RCU usage.
> > And that is the reason why I have put it.
> >
> 
> Did you try using rtnl_dereference() instead of rcu_dereference() ?

I have just tried it now and that seems to work fine.
I will redo the patch and send a new patch series.

> 
> >>
> >>> +     list_for_each_entry_rcu(mrp, &br->mrp_list, list,
> >>> +                             lockdep_rtnl_is_held()) {
> >>> +             struct net_bridge_port *p;
> >>> +
> >>> +             p = rcu_dereference(mrp->p_port);
> >>> +             if (p && p->dev->ifindex == ifindex) {
> >>> +                     res = false;
> >>> +                     break;
> >>> +             }
> >>> +
> >>> +             p = rcu_dereference(mrp->s_port);
> >>> +             if (p && p->dev->ifindex == ifindex) {
> >>> +                     res = false;
> >>> +                     break;
> >>> +             }
> >>> +     }
> >>> +     rcu_read_unlock();
> >>> +     return res;
> >>> +}
> >>> +
> >>>  static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
> >>>                                      struct net_bridge_port *p)
> >>>  {
> >>> @@ -255,6 +281,11 @@ int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
> >>>           !br_mrp_get_port(br, instance->s_ifindex))
> >>>               return -EINVAL;
> >>>
> >>> +     /* It is not possible to have the same port part of multiple rings */
> >>> +     if (!br_mrp_unique_ifindex(br, instance->p_ifindex) ||
> >>> +         !br_mrp_unique_ifindex(br, instance->s_ifindex))
> >>> +             return -EINVAL;
> >>> +
> >>>       mrp = kzalloc(sizeof(*mrp), GFP_KERNEL);
> >>>       if (!mrp)
> >>>               return -ENOMEM;
> >>>
> >>
> >
> 

-- 
/Horatiu

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-05-21 17:30 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 13:09 [PATCH 0/3] net: bridge: mrp: Add small fixes to MRP Horatiu Vultur
2020-05-20 13:09 ` [PATCH 1/3] bridge: mrp: Add br_mrp_unique_ifindex function Horatiu Vultur
2020-05-21  8:16   ` Nikolay Aleksandrov
2020-05-21 18:49     ` Horatiu Vultur
2020-05-21 16:58       ` Nikolay Aleksandrov
2020-05-21 19:30         ` Horatiu Vultur
2020-05-20 13:09 ` [PATCH 2/3] switchdev: mrp: Remove the variable mrp_ring_state Horatiu Vultur
2020-05-20 15:56   ` Ivan Vecera
2020-05-20 13:09 ` [PATCH 3/3] bridge: mrp: Restore port state when deleting MRP instance Horatiu Vultur
2020-05-21  8:18   ` Nikolay Aleksandrov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).