| Message ID | 20210121234317.65936-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [resend,net] net: switchdev: don't set port_obj_info->handled true when -EOPNOTSUPP | expand |
The 01/22/2021 00:43, 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. Just a small correction to what you have said regarding MRP. Is not the option mrp->ring_role_offload that determines if the MRP Test frames are generated by the HW but is the return value of the function 'br_mrp_switchdev_send_ring_test' called from function 'br_mrp_start_test'. But everything else looks good. I have also started to work on a patch series where I try to improve the way the return values of switchdev calls are handled in MRP. I should be able to send these patches by the end of week. > > 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. > > Fixes: f30f0601eb93 ("switchdev: Add helpers to aid traversal through lower devices") > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > Resending with more folks on cc and a tentative fixes tag. > > net/switchdev/switchdev.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > 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 > -- > 2.23.0 > -- /Horatiu
On 22/01/2021 10.05, Horatiu Vultur wrote: > The 01/22/2021 00:43, 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. > > Just a small correction to what you have said regarding MRP. Is not the > option mrp->ring_role_offload that determines if the MRP Test frames are > generated by the HW but is the return value of the function > 'br_mrp_switchdev_send_ring_test' called from function > 'br_mrp_start_test'. Hm, you're right, but the underlying problem is the same: br_mrp_switchdev_set_ring_role() (whose return value is what is used to determine ->ing_role_offloaded) and br_mrp_switchdev_send_ring_test() both make use of switchdev_port_obj_add(SWITCHDEV_OBJ_ID_*MRP), and that function is currently unable to return -EOPNOTSUPP, which it must in order for the MRP logic to work. Rasmus
Rasmus Villemoes <rasmus.villemoes@prevas.dk> writes: > 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. > > Fixes: f30f0601eb93 ("switchdev: Add helpers to aid traversal through lower devices") > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Looks good. Reviewed-by: Petr Machata <petrm@nvidia.com> Thanks!
On Fri, 22 Jan 2021 14:36:08 +0100 Rasmus Villemoes wrote: > On 22/01/2021 10.05, Horatiu Vultur wrote: > > The 01/22/2021 00:43, 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. > > > > Just a small correction to what you have said regarding MRP. Is not the > > option mrp->ring_role_offload that determines if the MRP Test frames are > > generated by the HW but is the return value of the function > > 'br_mrp_switchdev_send_ring_test' called from function > > 'br_mrp_start_test'. > > Hm, you're right, but the underlying problem is the same: > br_mrp_switchdev_set_ring_role() (whose return value is what is used to > determine ->ing_role_offloaded) and br_mrp_switchdev_send_ring_test() > both make use of switchdev_port_obj_add(SWITCHDEV_OBJ_ID_*MRP), and that > function is currently unable to return -EOPNOTSUPP, which it must in > order for the MRP logic to work. Could you reword the commit message to avoid confusion for folks reading git history and repost? You can keep Petr's tag.
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. Fixes: f30f0601eb93 ("switchdev: Add helpers to aid traversal through lower devices") Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- Resending with more folks on cc and a tentative fixes tag. net/switchdev/switchdev.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)