diff mbox series

[net,2/2] net: switchdev: don't set port_obj_info->handled true when -EOPNOTSUPP

Message ID 20201223144533.4145-3-rasmus.villemoes@prevas.dk
State Superseded
Headers show
Series MRP without hardware offload? | expand

Commit Message

Rasmus Villemoes Dec. 23, 2020, 2:45 p.m. UTC
It's not true that switchdev_port_obj_notify() only inspects the
->handled field of "struct switchdev_notifier_port_obj_info" if
call_switchdev_blocking_notifiers() returns 0 - there's a WARN_ON()
triggering for a non-zero return combined with ->handled not being
true. But the real problem here is that -EOPNOTSUPP is not being
properly handled.

The wrapper functions switchdev_handle_port_obj_add() et al change a
return value of -EOPNOTSUPP to 0, and the treatment of ->handled in
switchdev_port_obj_notify() seems to be designed to change that back
to -EOPNOTSUPP in case nobody actually acted on the notifier (i.e.,
everybody returned -EOPNOTSUPP).

Currently, as soon as some device down the stack passes the check_cb()
check, ->handled gets set to true, which means that
switchdev_port_obj_notify() cannot actually ever return -EOPNOTSUPP.

This, for example, means that the detection of hardware offload
support in the MRP code is broken - br_mrp_set_ring_role() always ends
up setting mrp->ring_role_offloaded to 1, despite not a single
mainline driver implementing any of the SWITCHDEV_OBJ_ID*_MRP. So
since the MRP code thinks the generation of MRP test frames has been
offloaded, no such frames are actually put on the wire.

So, continue to set ->handled true if any callback returns success or
any error distinct from -EOPNOTSUPP. But if all the callbacks return
-EOPNOTSUPP, make sure that ->handled stays false, so the logic in
switchdev_port_obj_notify() can propagate that information.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 net/switchdev/switchdev.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Comments

Jakub Kicinski Dec. 28, 2020, 10:26 p.m. UTC | #1
On Wed, 23 Dec 2020 15:45:33 +0100 Rasmus Villemoes wrote:
> It's not true that switchdev_port_obj_notify() only inspects the

> ->handled field of "struct switchdev_notifier_port_obj_info" if  

> call_switchdev_blocking_notifiers() returns 0 - there's a WARN_ON()

> triggering for a non-zero return combined with ->handled not being

> true. But the real problem here is that -EOPNOTSUPP is not being

> properly handled.

> 

> The wrapper functions switchdev_handle_port_obj_add() et al change a

> return value of -EOPNOTSUPP to 0, and the treatment of ->handled in

> switchdev_port_obj_notify() seems to be designed to change that back

> to -EOPNOTSUPP in case nobody actually acted on the notifier (i.e.,

> everybody returned -EOPNOTSUPP).

> 

> Currently, as soon as some device down the stack passes the check_cb()

> check, ->handled gets set to true, which means that

> switchdev_port_obj_notify() cannot actually ever return -EOPNOTSUPP.

> 

> This, for example, means that the detection of hardware offload

> support in the MRP code is broken - br_mrp_set_ring_role() always ends

> up setting mrp->ring_role_offloaded to 1, despite not a single

> mainline driver implementing any of the SWITCHDEV_OBJ_ID*_MRP. So

> since the MRP code thinks the generation of MRP test frames has been

> offloaded, no such frames are actually put on the wire.

> 

> So, continue to set ->handled true if any callback returns success or

> any error distinct from -EOPNOTSUPP. But if all the callbacks return

> -EOPNOTSUPP, make sure that ->handled stays false, so the logic in

> switchdev_port_obj_notify() can propagate that information.

> 

> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>


Please make sure you CC the folks who may have something to say about
this - Jiri, Ivan, Ido, Florian, etc.
diff mbox series

Patch

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 23d868545362..2c1ffc9ba2eb 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -460,10 +460,11 @@  static int __switchdev_handle_port_obj_add(struct net_device *dev,
 	extack = switchdev_notifier_info_to_extack(&port_obj_info->info);
 
 	if (check_cb(dev)) {
-		/* This flag is only checked if the return value is success. */
-		port_obj_info->handled = true;
-		return add_cb(dev, port_obj_info->obj, port_obj_info->trans,
-			      extack);
+		err = add_cb(dev, port_obj_info->obj, port_obj_info->trans,
+			     extack);
+		if (err != -EOPNOTSUPP)
+			port_obj_info->handled = true;
+		return err;
 	}
 
 	/* Switch ports might be stacked under e.g. a LAG. Ignore the
@@ -515,9 +516,10 @@  static int __switchdev_handle_port_obj_del(struct net_device *dev,
 	int err = -EOPNOTSUPP;
 
 	if (check_cb(dev)) {
-		/* This flag is only checked if the return value is success. */
-		port_obj_info->handled = true;
-		return del_cb(dev, port_obj_info->obj);
+		err = del_cb(dev, port_obj_info->obj);
+		if (err != -EOPNOTSUPP)
+			port_obj_info->handled = true;
+		return err;
 	}
 
 	/* Switch ports might be stacked under e.g. a LAG. Ignore the
@@ -568,9 +570,10 @@  static int __switchdev_handle_port_attr_set(struct net_device *dev,
 	int err = -EOPNOTSUPP;
 
 	if (check_cb(dev)) {
-		port_attr_info->handled = true;
-		return set_cb(dev, port_attr_info->attr,
-			      port_attr_info->trans);
+		err = set_cb(dev, port_attr_info->attr, port_attr_info->trans);
+		if (err != -EOPNOTSUPP)
+			port_attr_info->handled = true;
+		return err;
 	}
 
 	/* Switch ports might be stacked under e.g. a LAG. Ignore the