Messages in this thread | | | Date | Tue, 23 Apr 2024 16:25:53 +0000 | Subject | Re: [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage() | From | 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
| |