| Message ID | 20201223144533.4145-3-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | MRP without hardware offload? | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: ivecera@redhat.com jiri@resnulli.us |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 41 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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 --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
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(-)