The 01/25/2021 19:06, Willem de Bruijn wrote:
> On Sat, Jan 23, 2021 at 11:23 AM Horatiu Vultur
> <horatiu.vultur@xxxxxxxxxxxxx> wrote:
Hi Willem,
> >
> > This patch extends the br_mrp_switchdev functions to be able to have a
> > better understanding what cause the issue and if the SW needs to be used
> > as a backup.
> >
> > There are the following cases:
> > - when the code is compiled without CONFIG_NET_SWITCHDEV. In this case
> > return success so the SW can continue with the protocol. Depending on
> > the function it returns 0 or BR_MRP_SW.
> > - when code is compiled with CONFIG_NET_SWITCHDEV and the driver doesn't
> > implement any MRP callbacks, then the HW can't run MRP so it just
> > returns -EOPNOTSUPP. So the SW will stop further to configure the
> > node.
> > - when code is compiled with CONFIG_NET_SWITCHDEV and the driver fully
> > supports any MRP functionality then the SW doesn't need to do
> > anything. The functions will return 0 or BR_MRP_HW.
> > - when code is compiled with CONFIG_NET_SWITCHDEV and the HW can't run
> > completely the protocol but it can help the SW to run it. For
> > example, the HW can't support completely MRM role(can't detect when it
> > stops receiving MRP Test frames) but it can redirect these frames to
> > CPU. In this case it is possible to have a SW fallback. The SW will
> > try initially to call the driver with sw_backup set to false, meaning
> > that the HW can implement completely the role. If the driver returns
> > -EOPNOTSUPP, the SW will try again with sw_backup set to false,
> > meaning that the SW will detect when it stops receiving the frames. In
> > case the driver returns 0 then the SW will continue to configure the
> > node accordingly.
> >
> > In this way is more clear when the SW needs to stop configuring the
> > node, or when the SW is used as a backup or the HW can implement the
> > functionality.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
>
>
> > -int br_mrp_switchdev_set_ring_role(struct net_bridge *br,
> > - struct br_mrp *mrp,
> > - enum br_mrp_ring_role_type role)
> > +enum br_mrp_hw_support
> > +br_mrp_switchdev_set_ring_role(struct net_bridge *br, struct br_mrp *mrp,
> > + enum br_mrp_ring_role_type role)
> > {
> > struct switchdev_obj_ring_role_mrp mrp_role = {
> > .obj.orig_dev = br->dev,
> > .obj.id = SWITCHDEV_OBJ_ID_RING_ROLE_MRP,
> > .ring_role = role,
> > .ring_id = mrp->ring_id,
> > + .sw_backup = false,
> > };
> > int err;
> >
> > + /* If switchdev is not enabled then just run in SW */
> > + if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
> > + return BR_MRP_SW;
> > +
> > + /* First try to see if HW can implement comptletly the role in HW */
>
> typo: completely
>
> > if (role == BR_MRP_RING_ROLE_DISABLED)
> > err = switchdev_port_obj_del(br->dev, &mrp_role.obj);
> > else
> > err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);
> >
> > - return err;
> > + /* In case of success then just return and notify the SW that doesn't
> > + * need to do anything
> > + */
> > + if (!err)
> > + return BR_MRP_HW;
> > +
> > + /* There was some issue then is not possible at all to have this role so
> > + * just return failire
>
> typo: failure
>
> > + */
> > + if (err != -EOPNOTSUPP)
> > + return BR_MRP_NONE;
> > +
> > + /* In case the HW can't run complety in HW the protocol, we try again
>
> typo: completely. Please proofread your comments closely. I saw at
> least one typo in the commit messages too.
Sorry for that. I will fix those in the next version.
>
> More in general comments that say what the code does can generally be eschewed.
>
> > + * and this time to allow the SW to help, but the HW needs to redirect
> > + * the frames to CPU.
> > + */
> > + mrp_role.sw_backup = true;
> > + err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);
>
> This calls the same function. I did not see code that changes behavior
> based on sw_backup. Will this not give the same result?
No, because is the driver responsibility to check that flag and
implement what it can support. If the sw_backup is false, then it is
expected the driver to implement completely the functionality in HW. If
sw_backup is true it just needs to help the SW to run. So the driver can
check this flag and decide what to do.
>
> Also, this lacks the role test (add or del). Is that because if
> falling back onto SW mode during add, this code does not get called at
> all on delete?
Good catch!. It should have the check.
>
> > +
> > + /* In case of success then notify the SW that it needs to help with the
> > + * protocol
> > + */
> > + if (!err)
> > + return BR_MRP_SW;
> > +
> > + return BR_MRP_NONE;
> > }
> >
> > -int br_mrp_switchdev_send_ring_test(struct net_bridge *br,
> > - struct br_mrp *mrp, u32 interval,
> > - u8 max_miss, u32 period,
> > - bool monitor)
> > +enum br_mrp_hw_support
> > +br_mrp_switchdev_send_ring_test(struct net_bridge *br, struct br_mrp *mrp,
> > + u32 interval, u8 max_miss, u32 period,
> > + bool monitor)
> > {
> > struct switchdev_obj_ring_test_mrp test = {
> > .obj.orig_dev = br->dev,
> > @@ -79,12 +106,29 @@ int br_mrp_switchdev_send_ring_test(struct net_bridge *br,
> > };
> > int err;
> >
> > + /* If switchdev is not enabled then just run in SW */
> > + if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
> > + return BR_MRP_SW;
> > +
> > if (interval == 0)
> > err = switchdev_port_obj_del(br->dev, &test.obj);
> > else
> > err = switchdev_port_obj_add(br->dev, &test.obj, NULL);
> >
> > - return err;
> > + /* If everything succeed then the HW can send this frames, so the SW
> > + * doesn't need to generate them
> > + */
> > + if (!err)
> > + return BR_MRP_HW;
> > +
> > + /* There was an error when the HW was configured so the SW return
> > + * failure
> > + */
> > + if (err != -EOPNOTSUPP)
> > + return BR_MRP_NONE;
> > +
> > + /* So the HW can't generate these frames so allow the SW to do that */
> > + return BR_MRP_SW;
>
> This is the same ternary test as above. It recurs a few times. Perhaps
> it can use a helper.
Yes, I will try to have a look.
--
/Horatiu
