| Message ID | 20240423102728.228765-1-ast@fiberby.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next-1 |
Hi Asbjørn, Thank you for your patch! > Define extack locally, to reduce line lengths and future users. > > Only perform fragment handling, when at least one fragment flag is set. > > Remove goto, as it's only used once, and the error message is specific > to that context. > > Only compile tested. > > Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> > --- > .../ethernet/microchip/sparx5/sparx5_tc_flower.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c > index 663571fe7b2d..d846edd77a01 100644 > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c > @@ -159,13 +159,14 @@ sparx5_tc_flower_handler_basic_usage(struct vcap_tc_flower_parse_usage *st) > static int > sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st) > { > + struct netlink_ext_ack *extack = st->fco->common.extack; Could you please update the use of extack in all places inside this function. You are missing one place. > struct flow_match_control mt; > u32 value, mask; > int err = 0; > > flow_rule_match_control(st->frule, &mt); > > - if (mt.mask->flags) { > + if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) { Since these flags are used here and in the next patch, maybe assign them to a variable: u32 supp_flags = FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG And update the use throughout. > u8 is_frag_key = !!(mt.key->flags & FLOW_DIS_IS_FRAGMENT); > u8 is_frag_mask = !!(mt.mask->flags & FLOW_DIS_IS_FRAGMENT); > u8 is_frag_idx = (is_frag_key << 1) | is_frag_mask; > @@ -190,17 +191,15 @@ sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st) > err = vcap_rule_add_key_u32(st->vrule, > VCAP_KF_L3_FRAGMENT_TYPE, > value, mask); > - if (err) > - goto out; > + if (err) { > + NL_SET_ERR_MSG_MOD(extack, "ip_frag parse error"); > + return err; > + } > } > > st->used_keys |= BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL); > > return err; > - > -out: > - NL_SET_ERR_MSG_MOD(st->fco->common.extack, "ip_frag parse error"); > - return err; > } > > static int > -- > 2.43.0 Also I think you missing a cover letter for this series. I will run the patches through our tests once the issues are addressed. /Daniel
Tue, Apr 23, 2024 at 12:27:26PM CEST, ast@fiberby.net wrote: >Define extack locally, to reduce line lengths and future users. > >Only perform fragment handling, when at least one fragment flag is set. > >Remove goto, as it's only used once, and the error message is specific >to that context. 3 changes, 3 patches? > >Only compile tested. > >Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> >--- > .../ethernet/microchip/sparx5/sparx5_tc_flower.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > >diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c >index 663571fe7b2d..d846edd77a01 100644 >--- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c >+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c >@@ -159,13 +159,14 @@ sparx5_tc_flower_handler_basic_usage(struct vcap_tc_flower_parse_usage *st) > static int > sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st) > { >+ struct netlink_ext_ack *extack = st->fco->common.extack; > struct flow_match_control mt; > u32 value, mask; > int err = 0; > > flow_rule_match_control(st->frule, &mt); > >- if (mt.mask->flags) { >+ if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) { > u8 is_frag_key = !!(mt.key->flags & FLOW_DIS_IS_FRAGMENT); > u8 is_frag_mask = !!(mt.mask->flags & FLOW_DIS_IS_FRAGMENT); > u8 is_frag_idx = (is_frag_key << 1) | is_frag_mask; >@@ -190,17 +191,15 @@ sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st) > err = vcap_rule_add_key_u32(st->vrule, > VCAP_KF_L3_FRAGMENT_TYPE, > value, mask); >- if (err) >- goto out; >+ if (err) { >+ NL_SET_ERR_MSG_MOD(extack, "ip_frag parse error"); >+ return err; >+ } > } > > st->used_keys |= BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL); > > return err; >- >-out: >- NL_SET_ERR_MSG_MOD(st->fco->common.extack, "ip_frag parse error"); >- return err; > } > > static int >-- >2.43.0 > >
Hi Daniel, Thank you for the review. On 4/23/24 11:15 AM, Daniel Machon wrote: > Hi Asbjørn, > > Thank you for your patch! > >> Define extack locally, to reduce line lengths and future users. >> >> Only perform fragment handling, when at least one fragment flag is set. >> >> Remove goto, as it's only used once, and the error message is specific >> to that context. >> >> Only compile tested. >> >> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> >> --- >> .../ethernet/microchip/sparx5/sparx5_tc_flower.c | 13 ++++++------- >> 1 file changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c >> index 663571fe7b2d..d846edd77a01 100644 >> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c >> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c >> @@ -159,13 +159,14 @@ sparx5_tc_flower_handler_basic_usage(struct vcap_tc_flower_parse_usage *st) >> static int >> sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st) >> { >> + struct netlink_ext_ack *extack = st->fco->common.extack; > > Could you please update the use of extack in all places inside this > function. You are missing one place. Good catch, sure. It must have got lost somewhere along the way. I deliberately kept it out of the net patch, since it could wait for net-next. >> struct flow_match_control mt; >> u32 value, mask; >> int err = 0; >> >> flow_rule_match_control(st->frule, &mt); >> >> - if (mt.mask->flags) { >> + if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) { > > Since these flags are used here and in the next patch, maybe assign them > to a variable: > > u32 supp_flags = FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG > > And update the use throughout. In an earlier state this patch had a #define SPARX5_FLOWER_SUPPORTED_CTLFLAGS, in the same style as nfp in drivers/net/ethernet/netronome/nfp/flower/offload.c Right now, this driver supports all currently defined flags (which are used with mask), so the point of using flow_rule_is_supp_control_flags() to this dirver, is to make it possible to introduce new flags in the future, without having to update all drivers to explicitly not support a new flag. My problem with using supp_flags in both places is: What happens when support for a new flag is introduced? u32 supp_flags = FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG | FLOW_DIS_NEW_FLAG; if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) /* handle fragment flags through lookup table */ if (mt.mask->flags & FLOW_DIS_NEW_FLAG) /* do something */ if (!flow_rule_is_supp_control_flags(supp_flags, mt.mask->flags, extack)) return -EOPNOTSUPP; The fragment lookup table code currently requires the above guarding, as [0][0] in the lookup table is FRAG_INVAL, and not FRAG_SHRUG. What do you think?
> > > - if (mt.mask->flags) { > > > + if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) { > > > > Since these flags are used here and in the next patch, maybe assign them > > to a variable: > > > > u32 supp_flags = FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG > > > > And update the use throughout. > > In an earlier state this patch had a #define SPARX5_FLOWER_SUPPORTED_CTLFLAGS, > in the same style as nfp in drivers/net/ethernet/netronome/nfp/flower/offload.c > > Right now, this driver supports all currently defined flags (which are used with mask), > so the point of using flow_rule_is_supp_control_flags() to this dirver, is to > make it possible to introduce new flags in the future, without having to update > all drivers to explicitly not support a new flag. > > My problem with using supp_flags in both places is: What happens when support > for a new flag is introduced? > > u32 supp_flags = FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG | FLOW_DIS_NEW_FLAG; > > if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) > /* handle fragment flags through lookup table */ > > if (mt.mask->flags & FLOW_DIS_NEW_FLAG) > /* do something */ > > if (!flow_rule_is_supp_control_flags(supp_flags, mt.mask->flags, extack)) > return -EOPNOTSUPP; > > The fragment lookup table code currently requires the above guarding, > as [0][0] in the lookup table is FRAG_INVAL, and not FRAG_SHRUG. > > What do you think? Yes - lets only check for fragment flags when doing the lookup. I am fine with your original impl. If you can fix the remaining issue, I will take the patches for a test spin tomorrow. Thanks! > > -- > Best regards > Asbjørn Sloth Tønnesen > Network Engineer > Fiberby - AS42541
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c index 663571fe7b2d..d846edd77a01 100644 --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c @@ -159,13 +159,14 @@ sparx5_tc_flower_handler_basic_usage(struct vcap_tc_flower_parse_usage *st) static int sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st) { + struct netlink_ext_ack *extack = st->fco->common.extack; struct flow_match_control mt; u32 value, mask; int err = 0; flow_rule_match_control(st->frule, &mt); - if (mt.mask->flags) { + if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) { u8 is_frag_key = !!(mt.key->flags & FLOW_DIS_IS_FRAGMENT); u8 is_frag_mask = !!(mt.mask->flags & FLOW_DIS_IS_FRAGMENT); u8 is_frag_idx = (is_frag_key << 1) | is_frag_mask; @@ -190,17 +191,15 @@ sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st) err = vcap_rule_add_key_u32(st->vrule, VCAP_KF_L3_FRAGMENT_TYPE, value, mask); - if (err) - goto out; + if (err) { + NL_SET_ERR_MSG_MOD(extack, "ip_frag parse error"); + return err; + } } st->used_keys |= BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL); return err; - -out: - NL_SET_ERR_MSG_MOD(st->fco->common.extack, "ip_frag parse error"); - return err; } static int
Define extack locally, to reduce line lengths and future users. Only perform fragment handling, when at least one fragment flag is set. Remove goto, as it's only used once, and the error message is specific to that context. Only compile tested. Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> --- .../ethernet/microchip/sparx5/sparx5_tc_flower.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)