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

On Fri, 22 Jan 2021 14:36:08 +0100 Rasmus Villemoes wrote:

On 22/01/2021 10.05, Horatiu Vultur wrote:

quoted

The 01/22/2021 00:43, Rasmus Villemoes wrote:  

quoted

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.