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 <[email protected]>
---
.../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 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 <[email protected]>
> ---
> .../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, [email protected] 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 <[email protected]>
>---
> .../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 <[email protected]>
>> ---
>> .../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
> > > - 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