[PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage()

[PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage()

[Asbjørn Sloth Tønnesen]

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;
 	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

[PATCH net-next 2/2] net: sparx5: flower: check for unsupported control flags

[Asbjørn Sloth Tønnesen]

Use flow_rule_is_supp_control_flags() to reject filters with
unsupported control flags.

In case any unsupported control flags are masked,
flow_rule_is_supp_control_flags() sets a NL extended
error message, and we return -EOPNOTSUPP.

Only compile-tested.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
index d846edd77a01..f81d89f8f620 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
@@ -197,6 +197,11 @@ sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st)
 		}
 	}
 
+	if (!flow_rule_is_supp_control_flags(FLOW_DIS_IS_FRAGMENT |
+					     FLOW_DIS_FIRST_FRAG,
+					     mt.mask->flags, extack))
+		return -EOPNOTSUPP;
+
 	st->used_keys |= BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL);
 
 	return err;
-- 
2.43.0

Re: [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage()

[Daniel Machon]

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

Re: [PATCH net-next 2/2] net: sparx5: flower: check for unsupported control flags

[Daniel Machon]

> Use flow_rule_is_supp_control_flags() to reject filters with
> unsupported control flags.
> 
> In case any unsupported control flags are masked,
> flow_rule_is_supp_control_flags() sets a NL extended
> error message, and we return -EOPNOTSUPP.
> 
> Only compile-tested.
> 
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
>  drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> index d846edd77a01..f81d89f8f620 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> @@ -197,6 +197,11 @@ sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st)
>                 }
>         }
> 
> +       if (!flow_rule_is_supp_control_flags(FLOW_DIS_IS_FRAGMENT |
> +                                            FLOW_DIS_FIRST_FRAG,
> +                                            mt.mask->flags, extack))
> +               return -EOPNOTSUPP;
> +
>         st->used_keys |= BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL);
> 
>         return err;
> --
> 2.43.0

As mentioned in patch #1, use supp_flags here. Otherwise looks
good.

/Daniel

Re: [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage()

[Jiri Pirko]

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
>
>

Re: [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage()

[Asbjørn Sloth Tønnesen]

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?

-- 
Best regards
Asbjørn Sloth Tønnesen
Network Engineer
Fiberby - AS42541

Re: [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage()

[Daniel Machon]

> > > -       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