| Message ID | 20240408130927.78594-2-ast@fiberby.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | flower: validate control flags | expand |
On April 8, 2024 9:09 PM, Asbjørn wrote: >This helper can be used by drivers to check for the presence of unsupported >control flags. > >It mirrors the existing check done in sfc: > drivers/net/ethernet/sfc/tc.c +276 > >This is aimed at drivers, which implements some control flags. > >This should also be used by drivers that implement all current flags, so that >future flags will be unsupported by default. > >Only compile-tested. > >Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> >--- > include/net/flow_offload.h | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > >diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index >314087a5e1818..c1317b14da08c 100644 >--- a/include/net/flow_offload.h >+++ b/include/net/flow_offload.h >@@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct >flow_rule *rule, > return dissector_uses_key(rule->match.dissector, key); } > >+/** >+ * flow_rule_no_unsupp_control_flags() - check for unsupported control >+flags >+ * @supp_flags: flags supported by driver >+ * @flags: flags present in rule >+ * @extack: The netlink extended ACK for reporting errors. >+ * >+ * Returns true if only supported control flags are set, false otherwise. >+ */ >+static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags, >+ const u32 flags, >+ Hi Asbjørn, thanks for your work, it makes sense for driver check. Will it better to name flags as "ctrl_flags" to make it more clear since it indicates the ctrl_flags in rule and you name it as control.flags in the following print message. struct >+netlink_ext_ack *extack) { >+ if (likely((flags & ~supp_flags) == 0)) >+ return true; >+ >+ NL_SET_ERR_MSG_FMT_MOD(extack, >+ "Unsupported match on control.flags %#x", >+ flags); >+ >+ return false; >+} >+ > struct flow_stats { > u64 pkts; > u64 bytes; >-- This should not be included in this patch. >2.43.0
On April 8, 2024 9:09 PM, Asbjørn wrote: >flow_rule_no_unsupp_control_flags() > >[Some people who received this message don't often get email from >ast@fiberby.net. Learn why this is important at >https://aka.ms/LearnAboutSenderIdentification ] > >This helper can be used by drivers to check for the presence of unsupported >control flags. > >It mirrors the existing check done in sfc: > drivers/net/ethernet/sfc/tc.c +276 > >This is aimed at drivers, which implements some control flags. > >This should also be used by drivers that implement all current flags, so that >future flags will be unsupported by default. > >Only compile-tested. > >Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> >--- > include/net/flow_offload.h | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > >diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index >314087a5e1818..c1317b14da08c 100644 >--- a/include/net/flow_offload.h >+++ b/include/net/flow_offload.h >@@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct >flow_rule *rule, > return dissector_uses_key(rule->match.dissector, key); } > >+/** >+ * flow_rule_no_unsupp_control_flags() - check for unsupported control >+flags >+ * @supp_flags: flags supported by driver >+ * @flags: flags present in rule >+ * @extack: The netlink extended ACK for reporting errors. >+ * >+ * Returns true if only supported control flags are set, false otherwise. >+ */ >+static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags, >+ const u32 flags, >+ struct >+netlink_ext_ack *extack) { >+ if (likely((flags & ~supp_flags) == 0)) >+ return true; >+ >+ NL_SET_ERR_MSG_FMT_MOD(extack, >+ "Unsupported match on control.flags %#x", >+ flags); >+ >+ return false; >+} How about to squash this change with series I patch since they have similar functions for driver to use. >+ > struct flow_stats { > u64 pkts; > u64 bytes; >-- >2.43.0
On Mon, Apr 08, 2024 at 01:09:19PM +0000, Asbjørn Sloth Tønnesen wrote: > [Some people who received this message don't often get email from ast@fiberby.net. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > This helper can be used by drivers to check for the > presence of unsupported control flags. > > It mirrors the existing check done in sfc: > drivers/net/ethernet/sfc/tc.c +276 > > This is aimed at drivers, which implements some control flags. > > This should also be used by drivers that implement all > current flags, so that future flags will be unsupported > by default. > > Only compile-tested. > > Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> > --- > include/net/flow_offload.h | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > index 314087a5e1818..c1317b14da08c 100644 > --- a/include/net/flow_offload.h > +++ b/include/net/flow_offload.h > @@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule, > return dissector_uses_key(rule->match.dissector, key); > } > > +/** > + * flow_rule_no_unsupp_control_flags() - check for unsupported control flags > + * @supp_flags: flags supported by driver > + * @flags: flags present in rule > + * @extack: The netlink extended ACK for reporting errors. > + * > + * Returns true if only supported control flags are set, false otherwise. > + */ > +static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags, > + const u32 flags, > + struct netlink_ext_ack *extack) Thanks for the change Asbjørn, I like the series in general. I do have some nitpicking with the naming of this function, the double negative makes it a bit hard to read. Especially where it gets used, where it then reads as: 'if not no unsupported' I think something like: 'if not supported' or 'if unsupported' will read much better - personally I think the first option is the best, otherwise you might end up with 'if not unsupported', which is also weird. Some possible suggestions I can think of: flow_rule_control_flags_is_supp() flow_rule_is_supp_control_flags() flow_rule_check_supp_control_flags() or perhaps some even better variant of this. I hope it's not just me, if that's the case please feel free to ignore.
Hi Louis, On 4/9/24 8:40 AM, Louis Peens wrote: > On Mon, Apr 08, 2024 at 01:09:19PM +0000, Asbjørn Sloth Tønnesen wrote: >> [Some people who received this message don't often get email from ast@fiberby.net. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] >> >> This helper can be used by drivers to check for the >> presence of unsupported control flags. >> >> It mirrors the existing check done in sfc: >> drivers/net/ethernet/sfc/tc.c +276 >> >> This is aimed at drivers, which implements some control flags. >> >> This should also be used by drivers that implement all >> current flags, so that future flags will be unsupported >> by default. >> >> Only compile-tested. >> >> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> >> --- >> include/net/flow_offload.h | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h >> index 314087a5e1818..c1317b14da08c 100644 >> --- a/include/net/flow_offload.h >> +++ b/include/net/flow_offload.h >> @@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule, >> return dissector_uses_key(rule->match.dissector, key); >> } >> >> +/** >> + * flow_rule_no_unsupp_control_flags() - check for unsupported control flags >> + * @supp_flags: flags supported by driver >> + * @flags: flags present in rule >> + * @extack: The netlink extended ACK for reporting errors. >> + * >> + * Returns true if only supported control flags are set, false otherwise. >> + */ >> +static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags, >> + const u32 flags, >> + struct netlink_ext_ack *extack) > Thanks for the change Asbjørn, I like the series in general. I do have > some nitpicking with the naming of this function, the double negative > makes it a bit hard to read. Especially where it gets used, where it > then reads as: > 'if not no unsupported' > > I think something like: > 'if not supported' > or > 'if unsupported' > > will read much better - personally I think the first option is the best, > otherwise you might end up with 'if not unsupported', which is also > weird. > > Some possible suggestions I can think of: > flow_rule_control_flags_is_supp() > flow_rule_is_supp_control_flags() > flow_rule_check_supp_control_flags() > > or perhaps some even better variant of this. I hope it's not just me, if > that's the case please feel free to ignore. I agree, I will update the naming in v2: flow_rule_no_unsupp_control_flags => flow_rule_is_supp_control_flags flow_rule_no_control_flags + s/no/has/ => flow_rule_has_control_flags flow_rule_match_no_control_flags + s/no/has/ => flow_rule_match_has_control_flags
Hi Baowen, On 4/9/24 1:56 AM, Baowen Zheng wrote: > On April 8, 2024 9:09 PM, Asbjørn wrote: > >> This helper can be used by drivers to check for the presence of unsupported >> control flags. >> >> It mirrors the existing check done in sfc: >> drivers/net/ethernet/sfc/tc.c +276 >> >> This is aimed at drivers, which implements some control flags. >> >> This should also be used by drivers that implement all current flags, so that >> future flags will be unsupported by default. >> >> Only compile-tested. >> >> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> >> --- >> include/net/flow_offload.h | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index >> 314087a5e1818..c1317b14da08c 100644 >> --- a/include/net/flow_offload.h >> +++ b/include/net/flow_offload.h >> @@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct >> flow_rule *rule, >> return dissector_uses_key(rule->match.dissector, key); } >> >> +/** >> + * flow_rule_no_unsupp_control_flags() - check for unsupported control >> +flags >> + * @supp_flags: flags supported by driver >> + * @flags: flags present in rule >> + * @extack: The netlink extended ACK for reporting errors. >> + * >> + * Returns true if only supported control flags are set, false otherwise. >> + */ >> +static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags, >> + const u32 flags, >> + > Hi Asbjørn, thanks for your work, it makes sense for driver check. Will it better to name flags as "ctrl_flags" to make it more clear since it indicates the ctrl_flags in rule and you name it as control.flags in the following print message. Good point, I will rename that argument in v2. I copied the error message verbatim from sfc. > struct >> +netlink_ext_ack *extack) { >> + if (likely((flags & ~supp_flags) == 0)) >> + return true; >> + >> + NL_SET_ERR_MSG_FMT_MOD(extack, >> + "Unsupported match on control.flags %#x", >> + flags); >> + >> + return false; >> +} >> + >> struct flow_stats { >> u64 pkts; >> u64 bytes; >> -- > This should not be included in this patch. It's the default 3 lines of context, as is default in git format-patch.
On Tue, Apr 09, 2024 at 11:13:22AM +0000, Asbjørn Sloth Tønnesen wrote: > [Some people who received this message don't often get email from ast@fiberby.net. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > > +static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags, > > > + const u32 flags, > > > + struct netlink_ext_ack *extack) > > Thanks for the change Asbjørn, I like the series in general. I do have > > some nitpicking with the naming of this function, the double negative > > makes it a bit hard to read. Especially where it gets used, where it > > then reads as: > > 'if not no unsupported' > > > > I think something like: > > 'if not supported' > > or > > 'if unsupported' > > > > will read much better - personally I think the first option is the best, > > otherwise you might end up with 'if not unsupported', which is also > > weird. > > > > Some possible suggestions I can think of: > > flow_rule_control_flags_is_supp() > > flow_rule_is_supp_control_flags() > > flow_rule_check_supp_control_flags() > > > > or perhaps some even better variant of this. I hope it's not just me, if > > that's the case please feel free to ignore. > I agree, I will update the naming in v2: > > flow_rule_no_unsupp_control_flags => flow_rule_is_supp_control_flags > flow_rule_no_control_flags + s/no/has/ => flow_rule_has_control_flags > flow_rule_match_no_control_flags + s/no/has/ => flow_rule_match_has_control_flags Hi Asbjørn. I like these, I think it will follow much easier, thanks. Regards Louis > > -- > Best regards > Asbjørn Sloth Tønnesen > Network Engineer > Fiberby - AS42541
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index 314087a5e1818..c1317b14da08c 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule, return dissector_uses_key(rule->match.dissector, key); } +/** + * flow_rule_no_unsupp_control_flags() - check for unsupported control flags + * @supp_flags: flags supported by driver + * @flags: flags present in rule + * @extack: The netlink extended ACK for reporting errors. + * + * Returns true if only supported control flags are set, false otherwise. + */ +static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags, + const u32 flags, + struct netlink_ext_ack *extack) +{ + if (likely((flags & ~supp_flags) == 0)) + return true; + + NL_SET_ERR_MSG_FMT_MOD(extack, + "Unsupported match on control.flags %#x", + flags); + + return false; +} + struct flow_stats { u64 pkts; u64 bytes;
This helper can be used by drivers to check for the presence of unsupported control flags. It mirrors the existing check done in sfc: drivers/net/ethernet/sfc/tc.c +276 This is aimed at drivers, which implements some control flags. This should also be used by drivers that implement all current flags, so that future flags will be unsupported by default. Only compile-tested. Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> --- include/net/flow_offload.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)