| Message ID | 20240416134432.9527-1-ast@fiberby.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] sfc: use flow_rule_no_unsupp_control_flags() | expand |
On 16/04/2024 14:44, Asbjørn Sloth Tønnesen wrote: > Adopt nfp-style *_FLOWER_SUPPORTED_CTLFLAGS define. > > Change the check for unsupported control flags, to use the new helper > flow_rule_is_supp_control_flags(). > > Since the helper was based on sfc, then nothing really changes. > > Compile-tested, and compiled objects are identical. > > Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> Subject line doesn't match the patch (I guess because the helper got renamed). > --- > drivers/net/ethernet/sfc/tc.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c > index 82e8891a619a..5f73f1dea524 100644 > --- a/drivers/net/ethernet/sfc/tc.c > +++ b/drivers/net/ethernet/sfc/tc.c > @@ -21,6 +21,10 @@ > #include "ef100_rep.h" > #include "efx.h" > > +#define SFC_FLOWER_SUPPORTED_CTLFLAGS \ > + (FLOW_DIS_IS_FRAGMENT | \ > + FLOW_DIS_FIRST_FRAG) I'd rather keep the flags in-line, next to where they're actually used. I.e. we have if (flags & FRAGMENT) blah; if (flags & FIRST_FRAG) foo; if (!blah_supported(FRAGMENT | FIRST_FRAG)) return -EEK; and it's very clear that anyone changing one of those parts also needs to change the other. Whereas with your #define it's not immediately obvious to someone reading the code where that set of supported flags comes from conceptually. -ed
Hi Ed, On 4/16/24 1:57 PM, Edward Cree wrote: > On 16/04/2024 14:44, Asbjørn Sloth Tønnesen wrote: >> Adopt nfp-style *_FLOWER_SUPPORTED_CTLFLAGS define. >> >> Change the check for unsupported control flags, to use the new helper >> flow_rule_is_supp_control_flags(). >> >> Since the helper was based on sfc, then nothing really changes. >> >> Compile-tested, and compiled objects are identical. >> >> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> > > Subject line doesn't match the patch (I guess because the helper > got renamed). Correct, through I had fixed it everywhere. Apparently I missed one. >> --- >> drivers/net/ethernet/sfc/tc.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c >> index 82e8891a619a..5f73f1dea524 100644 >> --- a/drivers/net/ethernet/sfc/tc.c >> +++ b/drivers/net/ethernet/sfc/tc.c >> @@ -21,6 +21,10 @@ >> #include "ef100_rep.h" >> #include "efx.h" >> >> +#define SFC_FLOWER_SUPPORTED_CTLFLAGS \ >> + (FLOW_DIS_IS_FRAGMENT | \ >> + FLOW_DIS_FIRST_FRAG) > > I'd rather keep the flags in-line, next to where they're actually > used. I.e. we have > if (flags & FRAGMENT) > blah; > if (flags & FIRST_FRAG) > foo; > if (!blah_supported(FRAGMENT | FIRST_FRAG)) > return -EEK; > and it's very clear that anyone changing one of those parts also > needs to change the other. Whereas with your #define it's not > immediately obvious to someone reading the code where that set > of supported flags comes from conceptually. Ok, I liked the NFP-style #define, but will drop trying to expand that. pw-bot: changes-requested
diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c index 82e8891a619a..5f73f1dea524 100644 --- a/drivers/net/ethernet/sfc/tc.c +++ b/drivers/net/ethernet/sfc/tc.c @@ -21,6 +21,10 @@ #include "ef100_rep.h" #include "efx.h" +#define SFC_FLOWER_SUPPORTED_CTLFLAGS \ + (FLOW_DIS_IS_FRAGMENT | \ + FLOW_DIS_FIRST_FRAG) + enum efx_encap_type efx_tc_indr_netdev_type(struct net_device *net_dev) { if (netif_is_vxlan(net_dev)) @@ -273,11 +277,9 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx, match->value.ip_firstfrag = fm.key->flags & FLOW_DIS_FIRST_FRAG; match->mask.ip_firstfrag = true; } - if (fm.mask->flags & ~(FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) { - NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported match on control.flags %#x", - fm.mask->flags); + if (!flow_rule_is_supp_control_flags(SFC_FLOWER_SUPPORTED_CTLFLAGS, + fm.mask->flags, extack)) return -EOPNOTSUPP; - } } if (dissector->used_keys & ~(BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL) |
Adopt nfp-style *_FLOWER_SUPPORTED_CTLFLAGS define. Change the check for unsupported control flags, to use the new helper flow_rule_is_supp_control_flags(). Since the helper was based on sfc, then nothing really changes. Compile-tested, and compiled objects are identical. Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> --- drivers/net/ethernet/sfc/tc.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)