| Message ID | 20210123161812.1043345-1-horatiu.vultur@microchip.com |
|---|---|
| Headers | show |
| Series | bridge: mrp: Extend br_mrp_switchdev_* | expand |
On Sat, Jan 23, 2021 at 11:23 AM Horatiu Vultur <horatiu.vultur@microchip.com> wrote: > > 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@microchip.com> > -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. 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? 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? > + > + /* 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.
The 01/25/2021 19:06, Willem de Bruijn wrote: > On Sat, Jan 23, 2021 at 11:23 AM Horatiu Vultur > <horatiu.vultur@microchip.com> 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@microchip.com> > > > > -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
This patch series extends MRP switchdev to allow the SW to have a better understanding if the HW can implment the MRP functionality or it needs to help the HW to run it. There are 3 cases: - when HW can't implement at all the functionality. - when HW can implement a part of the functionality but needs the SW implement the rest. For example if it can't detect when it stops receiving MRP Test frames but it can copy the MRP frames to CPU to allow the SW to determin this. Another example is generating the MRP Test frames. If HW can't do that then the SW is used as backup. - when HW can implement completely the functionality. So, initially the SW tries to offload the entire functionality in HW, if that fails it tries offload parts of the functionality in HW and use the SW as helper and if also this fails then MRP can't run on this HW. Horatiu Vultur (4): switchdev: mrp: Extend ring_role_mrp and in_role_mrp bridge: mrp: Add 'enum br_mrp_hw_support' bridge: mrp: Extend br_mrp_switchdev to detect better the errors bridge: mrp: Update br_mrp to use new return values of br_mrp_switchdev include/net/switchdev.h | 2 + net/bridge/br_mrp.c | 43 +++++--- net/bridge/br_mrp_switchdev.c | 189 +++++++++++++++++++++++++--------- net/bridge/br_private_mrp.h | 38 +++++-- 4 files changed, 195 insertions(+), 77 deletions(-)