diff mbox series

[net-next,v4,5/8] bridge: mrp: Update br_mrp to use new return values of br_mrp_switchdev

Message ID 20210216214205.32385-6-horatiu.vultur@microchip.com
State New
Headers show
Series bridge: mrp: Extend br_mrp_switchdev_* | expand

Commit Message

Horatiu Vultur Feb. 16, 2021, 9:42 p.m. UTC 
Check the return values of the br_mrp_switchdev function.
In case of:
- BR_MRP_NONE, return the error to userspace,
- BR_MRP_SW, continue with SW implementation,
- BR_MRP_HW, continue without SW implementation,

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_mrp.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

Comments

Vladimir Oltean Feb. 17, 2021, 10:59 a.m. UTC | #1 
On Tue, Feb 16, 2021 at 10:42:02PM +0100, Horatiu Vultur wrote:
> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c

> index 01c67ed727a9..12487f6fe9b4 100644

> --- a/net/bridge/br_mrp.c

> +++ b/net/bridge/br_mrp.c

> @@ -639,7 +639,7 @@ int br_mrp_set_ring_role(struct net_bridge *br,

>  			 struct br_mrp_ring_role *role)

>  {

>  	struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id);

> -	int err;

> +	enum br_mrp_hw_support support;

>  

>  	if (!mrp)

>  		return -EINVAL;

> @@ -647,9 +647,9 @@ int br_mrp_set_ring_role(struct net_bridge *br,

>  	mrp->ring_role = role->ring_role;

>  

>  	/* If there is an error just bailed out */

> -	err = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);

> -	if (err && err != -EOPNOTSUPP)

> -		return err;

> +	support = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);

> +	if (support == BR_MRP_NONE)

> +		return -EOPNOTSUPP;


It is broken to update the return type and value of a function in one
patch, and check for the updated return value in another patch.

>  

>  	/* Now detect if the HW actually applied the role or not. If the HW

>  	 * applied the role it means that the SW will not to do those operations

> @@ -657,7 +657,7 @@ int br_mrp_set_ring_role(struct net_bridge *br,

>  	 * SW when ring is open, but if the is not pushed to the HW the SW will

>  	 * need to detect when the ring is open

>  	 */

> -	mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;

> +	mrp->ring_role_offloaded = support == BR_MRP_SW ? 0 : 1;

>  

>  	return 0;

>  }
Horatiu Vultur Feb. 17, 2021, 4:18 p.m. UTC | #2 
The 02/17/2021 10:59, Vladimir Oltean wrote:
> 

> On Tue, Feb 16, 2021 at 10:42:02PM +0100, Horatiu Vultur wrote:

> > diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c

> > index 01c67ed727a9..12487f6fe9b4 100644

> > --- a/net/bridge/br_mrp.c

> > +++ b/net/bridge/br_mrp.c

> > @@ -639,7 +639,7 @@ int br_mrp_set_ring_role(struct net_bridge *br,

> >                        struct br_mrp_ring_role *role)

> >  {

> >       struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id);

> > -     int err;

> > +     enum br_mrp_hw_support support;

> >

> >       if (!mrp)

> >               return -EINVAL;

> > @@ -647,9 +647,9 @@ int br_mrp_set_ring_role(struct net_bridge *br,

> >       mrp->ring_role = role->ring_role;

> >

> >       /* If there is an error just bailed out */

> > -     err = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);

> > -     if (err && err != -EOPNOTSUPP)

> > -             return err;

> > +     support = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);

> > +     if (support == BR_MRP_NONE)

> > +             return -EOPNOTSUPP;

> 

> It is broken to update the return type and value of a function in one

> patch, and check for the updated return value in another patch.

> 


Yes, I will be more careful next time. I have tried to compile between
the patches and I have not see any issues here so I though that
everything is good.

> >

> >       /* Now detect if the HW actually applied the role or not. If the HW

> >        * applied the role it means that the SW will not to do those operations

> > @@ -657,7 +657,7 @@ int br_mrp_set_ring_role(struct net_bridge *br,

> >        * SW when ring is open, but if the is not pushed to the HW the SW will

> >        * need to detect when the ring is open

> >        */

> > -     mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;

> > +     mrp->ring_role_offloaded = support == BR_MRP_SW ? 0 : 1;

> >

> >       return 0;

> >  }


-- 
/Horatiu
diff mbox series

Patch

diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index 01c67ed727a9..12487f6fe9b4 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -639,7 +639,7 @@  int br_mrp_set_ring_role(struct net_bridge *br,
 			 struct br_mrp_ring_role *role)
 {
 	struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id);
-	int err;
+	enum br_mrp_hw_support support;
 
 	if (!mrp)
 		return -EINVAL;
@@ -647,9 +647,9 @@  int br_mrp_set_ring_role(struct net_bridge *br,
 	mrp->ring_role = role->ring_role;
 
 	/* If there is an error just bailed out */
-	err = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	support = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);
+	if (support == BR_MRP_NONE)
+		return -EOPNOTSUPP;
 
 	/* Now detect if the HW actually applied the role or not. If the HW
 	 * applied the role it means that the SW will not to do those operations
@@ -657,7 +657,7 @@  int br_mrp_set_ring_role(struct net_bridge *br,
 	 * SW when ring is open, but if the is not pushed to the HW the SW will
 	 * need to detect when the ring is open
 	 */
-	mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
+	mrp->ring_role_offloaded = support == BR_MRP_SW ? 0 : 1;
 
 	return 0;
 }
@@ -670,6 +670,7 @@  int br_mrp_start_test(struct net_bridge *br,
 		      struct br_mrp_start_test *test)
 {
 	struct br_mrp *mrp = br_mrp_find_id(br, test->ring_id);
+	enum br_mrp_hw_support support;
 
 	if (!mrp)
 		return -EINVAL;
@@ -677,9 +678,13 @@  int br_mrp_start_test(struct net_bridge *br,
 	/* Try to push it to the HW and if it fails then continue with SW
 	 * implementation and if that also fails then return error.
 	 */
-	if (!br_mrp_switchdev_send_ring_test(br, mrp, test->interval,
-					     test->max_miss, test->period,
-					     test->monitor))
+	support = br_mrp_switchdev_send_ring_test(br, mrp, test->interval,
+						  test->max_miss, test->period,
+						  test->monitor);
+	if (support == BR_MRP_NONE)
+		return -EOPNOTSUPP;
+
+	if (support == BR_MRP_HW)
 		return 0;
 
 	mrp->test_interval = test->interval;
@@ -721,8 +726,8 @@  int br_mrp_set_in_state(struct net_bridge *br, struct br_mrp_in_state *state)
 int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role)
 {
 	struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id);
+	enum br_mrp_hw_support support;
 	struct net_bridge_port *p;
-	int err;
 
 	if (!mrp)
 		return -EINVAL;
@@ -780,10 +785,10 @@  int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role)
 	mrp->in_id = role->in_id;
 
 	/* If there is an error just bailed out */
-	err = br_mrp_switchdev_set_in_role(br, mrp, role->in_id,
-					   role->ring_id, role->in_role);
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	support = br_mrp_switchdev_set_in_role(br, mrp, role->in_id,
+					       role->ring_id, role->in_role);
+	if (support == BR_MRP_NONE)
+		return -EOPNOTSUPP;
 
 	/* Now detect if the HW actually applied the role or not. If the HW
 	 * applied the role it means that the SW will not to do those operations
@@ -791,7 +796,7 @@  int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role)
 	 * SW when interconnect ring is open, but if the is not pushed to the HW
 	 * the SW will need to detect when the interconnect ring is open.
 	 */
-	mrp->in_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
+	mrp->in_role_offloaded = support == BR_MRP_SW ? 0 : 1;
 
 	return 0;
 }
@@ -804,6 +809,7 @@  int br_mrp_start_in_test(struct net_bridge *br,
 			 struct br_mrp_start_in_test *in_test)
 {
 	struct br_mrp *mrp = br_mrp_find_in_id(br, in_test->in_id);
+	enum br_mrp_hw_support support;
 
 	if (!mrp)
 		return -EINVAL;
@@ -814,8 +820,13 @@  int br_mrp_start_in_test(struct net_bridge *br,
 	/* Try to push it to the HW and if it fails then continue with SW
 	 * implementation and if that also fails then return error.
 	 */
-	if (!br_mrp_switchdev_send_in_test(br, mrp, in_test->interval,
-					   in_test->max_miss, in_test->period))
+	support =  br_mrp_switchdev_send_in_test(br, mrp, in_test->interval,
+						 in_test->max_miss,
+						 in_test->period);
+	if (support == BR_MRP_NONE)
+		return -EOPNOTSUPP;
+
+	if (support == BR_MRP_HW)
 		return 0;
 
 	mrp->in_test_interval = in_test->interval;