diff mbox series

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

Message ID 20210121234317.65936-1-rasmus.villemoes@prevas.dk (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [resend,net] net: switchdev: don't set port_obj_info->handled true when -EOPNOTSUPP | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: 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

Commit Message

Rasmus Villemoes Jan. 21, 2021, 11:43 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.

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(-)

Comments

Horatiu Vultur Jan. 22, 2021, 9:05 a.m. UTC | #1
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
>
Rasmus Villemoes Jan. 22, 2021, 1:36 p.m. UTC | #2
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
Petr Machata Jan. 22, 2021, 1:42 p.m. UTC | #3
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!
Jakub Kicinski Jan. 23, 2021, 8:44 p.m. UTC | #4
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 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