| Message ID | 20210216214205.32385-6-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cd605d455a445837edb3372addbdd9a9e38df23b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bridge: mrp: Extend br_mrp_switchdev_* | expand |
| 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-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| 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: 0 this patch: 0 |
| 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, 104 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Feb 16, 2021 at 10:42:02PM +0100, Horatiu Vultur wrote: > diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c > index 01c67ed727a9..12487f6fe9b4 100644 > --- a/net/bridge/br_mrp.c > +++ b/net/bridge/br_mrp.c > @@ -639,7 +639,7 @@ int br_mrp_set_ring_role(struct net_bridge *br, > struct br_mrp_ring_role *role) > { > struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id); > - int err; > + enum br_mrp_hw_support support; > > if (!mrp) > return -EINVAL; > @@ -647,9 +647,9 @@ int br_mrp_set_ring_role(struct net_bridge *br, > mrp->ring_role = role->ring_role; > > /* If there is an error just bailed out */ > - err = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role); > - if (err && err != -EOPNOTSUPP) > - return err; > + support = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role); > + if (support == BR_MRP_NONE) > + return -EOPNOTSUPP; It is broken to update the return type and value of a function in one patch, and check for the updated return value in another patch. > > /* Now detect if the HW actually applied the role or not. If the HW > * applied the role it means that the SW will not to do those operations > @@ -657,7 +657,7 @@ int br_mrp_set_ring_role(struct net_bridge *br, > * SW when ring is open, but if the is not pushed to the HW the SW will > * need to detect when the ring is open > */ > - mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1; > + mrp->ring_role_offloaded = support == BR_MRP_SW ? 0 : 1; > > return 0; > }
The 02/17/2021 10:59, Vladimir Oltean wrote: > > On Tue, Feb 16, 2021 at 10:42:02PM +0100, Horatiu Vultur wrote: > > diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c > > index 01c67ed727a9..12487f6fe9b4 100644 > > --- a/net/bridge/br_mrp.c > > +++ b/net/bridge/br_mrp.c > > @@ -639,7 +639,7 @@ int br_mrp_set_ring_role(struct net_bridge *br, > > struct br_mrp_ring_role *role) > > { > > struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id); > > - int err; > > + enum br_mrp_hw_support support; > > > > if (!mrp) > > return -EINVAL; > > @@ -647,9 +647,9 @@ int br_mrp_set_ring_role(struct net_bridge *br, > > mrp->ring_role = role->ring_role; > > > > /* If there is an error just bailed out */ > > - err = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role); > > - if (err && err != -EOPNOTSUPP) > > - return err; > > + support = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role); > > + if (support == BR_MRP_NONE) > > + return -EOPNOTSUPP; > > It is broken to update the return type and value of a function in one > patch, and check for the updated return value in another patch. > Yes, I will be more careful next time. I have tried to compile between the patches and I have not see any issues here so I though that everything is good. > > > > /* Now detect if the HW actually applied the role or not. If the HW > > * applied the role it means that the SW will not to do those operations > > @@ -657,7 +657,7 @@ int br_mrp_set_ring_role(struct net_bridge *br, > > * SW when ring is open, but if the is not pushed to the HW the SW will > > * need to detect when the ring is open > > */ > > - mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1; > > + mrp->ring_role_offloaded = support == BR_MRP_SW ? 0 : 1; > > > > return 0; > > }
diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c index 01c67ed727a9..12487f6fe9b4 100644 --- a/net/bridge/br_mrp.c +++ b/net/bridge/br_mrp.c @@ -639,7 +639,7 @@ int br_mrp_set_ring_role(struct net_bridge *br, struct br_mrp_ring_role *role) { struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id); - int err; + enum br_mrp_hw_support support; if (!mrp) return -EINVAL; @@ -647,9 +647,9 @@ int br_mrp_set_ring_role(struct net_bridge *br, mrp->ring_role = role->ring_role; /* If there is an error just bailed out */ - err = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role); - if (err && err != -EOPNOTSUPP) - return err; + support = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role); + if (support == BR_MRP_NONE) + return -EOPNOTSUPP; /* Now detect if the HW actually applied the role or not. If the HW * applied the role it means that the SW will not to do those operations @@ -657,7 +657,7 @@ int br_mrp_set_ring_role(struct net_bridge *br, * SW when ring is open, but if the is not pushed to the HW the SW will * need to detect when the ring is open */ - mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1; + mrp->ring_role_offloaded = support == BR_MRP_SW ? 0 : 1; return 0; } @@ -670,6 +670,7 @@ int br_mrp_start_test(struct net_bridge *br, struct br_mrp_start_test *test) { struct br_mrp *mrp = br_mrp_find_id(br, test->ring_id); + enum br_mrp_hw_support support; if (!mrp) return -EINVAL; @@ -677,9 +678,13 @@ int br_mrp_start_test(struct net_bridge *br, /* Try to push it to the HW and if it fails then continue with SW * implementation and if that also fails then return error. */ - if (!br_mrp_switchdev_send_ring_test(br, mrp, test->interval, - test->max_miss, test->period, - test->monitor)) + support = br_mrp_switchdev_send_ring_test(br, mrp, test->interval, + test->max_miss, test->period, + test->monitor); + if (support == BR_MRP_NONE) + return -EOPNOTSUPP; + + if (support == BR_MRP_HW) return 0; mrp->test_interval = test->interval; @@ -721,8 +726,8 @@ int br_mrp_set_in_state(struct net_bridge *br, struct br_mrp_in_state *state) int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role) { struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id); + enum br_mrp_hw_support support; struct net_bridge_port *p; - int err; if (!mrp) return -EINVAL; @@ -780,10 +785,10 @@ int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role) mrp->in_id = role->in_id; /* If there is an error just bailed out */ - err = br_mrp_switchdev_set_in_role(br, mrp, role->in_id, - role->ring_id, role->in_role); - if (err && err != -EOPNOTSUPP) - return err; + support = br_mrp_switchdev_set_in_role(br, mrp, role->in_id, + role->ring_id, role->in_role); + if (support == BR_MRP_NONE) + return -EOPNOTSUPP; /* Now detect if the HW actually applied the role or not. If the HW * applied the role it means that the SW will not to do those operations @@ -791,7 +796,7 @@ int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role) * SW when interconnect ring is open, but if the is not pushed to the HW * the SW will need to detect when the interconnect ring is open. */ - mrp->in_role_offloaded = err == -EOPNOTSUPP ? 0 : 1; + mrp->in_role_offloaded = support == BR_MRP_SW ? 0 : 1; return 0; } @@ -804,6 +809,7 @@ int br_mrp_start_in_test(struct net_bridge *br, struct br_mrp_start_in_test *in_test) { struct br_mrp *mrp = br_mrp_find_in_id(br, in_test->in_id); + enum br_mrp_hw_support support; if (!mrp) return -EINVAL; @@ -814,8 +820,13 @@ int br_mrp_start_in_test(struct net_bridge *br, /* Try to push it to the HW and if it fails then continue with SW * implementation and if that also fails then return error. */ - if (!br_mrp_switchdev_send_in_test(br, mrp, in_test->interval, - in_test->max_miss, in_test->period)) + support = br_mrp_switchdev_send_in_test(br, mrp, in_test->interval, + in_test->max_miss, + in_test->period); + if (support == BR_MRP_NONE) + return -EOPNOTSUPP; + + if (support == BR_MRP_HW) return 0; mrp->in_test_interval = in_test->interval;
Check the return values of the br_mrp_switchdev function. In case of: - BR_MRP_NONE, return the error to userspace, - BR_MRP_SW, continue with SW implementation, - BR_MRP_HW, continue without SW implementation, Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> --- net/bridge/br_mrp.c | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-)