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
