Received-SPF: pass (google.com: domain of linux-kernel+bounces-136493-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 9 Apr 2024 10:40:51 +0200
From: Louis Peens <louis.peens@corigine.com>
To: =?iso-8859-1?Q?Asbj=F8rn_Sloth_T=F8nnesen?= <ast@fiberby.net>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Taras Chornyi <taras.chornyi@plvision.eu>,
	Woojung Huh <woojung.huh@microchip.com>,
	UNGLinuxDriver@microchip.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, Yanguo Li <yanguo.li@corigine.com>,
	oss-drivers@corigine.com, Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vladimir Oltean <olteanv@gmail.com>,
	Edward Cree <ecree.xilinx@gmail.com>,
	Jamal Hadi Salim <jhs@mojatatu.com>,
	Cong Wang <xiyou.wangcong@gmail.com>, Jiri Pirko <jiri@resnulli.us>
Subject: Re: [PATCH net-next 1/6] flow_offload: add
 flow_rule_no_unsupp_control_flags()
Message-ID: <ZhT/E1qDsMmMxGwb@LouisNoVo>
References: <20240408130927.78594-1-ast@fiberby.net>
 <20240408130927.78594-2-ast@fiberby.net>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240408130927.78594-2-ast@fiberby.net>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?iso-8859-1?Q?DzxvpVj+UJmKsxy0bOHNVomFuf7p5PtMqPHl4LzgtKD+sLWhaNgfHrcBi/?=
 =?iso-8859-1?Q?KKhuFPelwz95WVYQ+Aqcgq9KfiF+tqgXJd1P6DdixpaalpOwjgNoAEfai5?=
 =?iso-8859-1?Q?UZqQ4HOlzCpWHgJ3L6Doh/GcTx7ERhf4a/PzjfF39dTS04rY/arl11WYP/?=
 =?iso-8859-1?Q?Z9ZtPT25jshfxh9xGeIsV6FCu00azJBXWHlGZJ5KCyJiFF3Wak/kywv/Oq?=
 =?iso-8859-1?Q?3pCbQw6gnd9jypywaz087fQ1lYWLdGAESjPb1CXlH3um/U8Bb10LHdmvzi?=
 =?iso-8859-1?Q?5Uy0ATxiq1QblYZ7J3Pww6jgzuRbgoVmcW1z6h2jIO9u26QynLbVojk2Iq?=
 =?iso-8859-1?Q?aAE0amRWZdvJ+L5qEs+yUaFSHpkm0WfYraD21IaKBXpdGqBwcRoY4W7zvQ?=
 =?iso-8859-1?Q?KtHgZoVswFSMWlPYHb9Uq+PFzhsePyGyE0vBlM7B0RQffICcB79pgcCTMg?=
 =?iso-8859-1?Q?kJbPHf5e0d7cnJTdWQP2vRX3SL4Db2WJ4HsCX+UfAXwcTyCRyQF9Bhy7zM?=
 =?iso-8859-1?Q?O9UaIMF6o3p82twETFPtPwcWcGZdGKPkIaR5Lp/+wmd6c3HGBS389lL62I?=
 =?iso-8859-1?Q?YAZhZDFalZ8PIwAhX6VxV+hbs848AgHfvu1rmTtM3crwcCr25W5JrBTcr8?=
 =?iso-8859-1?Q?F0NwToz1tveNaecMQwFRK8apaZHdeJBKVIKP9uQTDmGMcOA0DWiW94namV?=
 =?iso-8859-1?Q?W7dLCkMJifdh2uujy2OfsacTxLJHV9oj6DRL7ooCUdqPAxP9tLmjhviio2?=
 =?iso-8859-1?Q?XRRFZMJbkXshRvqnvqOSaF1Zld4XT8GFOJRk3ezZ8grAcxvJ314qCmryot?=
 =?iso-8859-1?Q?lclFGO6K9Dqiglwf83E5bDFIhZdkupqUy7YjMwdEYobO4McXbDcjVrsKFO?=
 =?iso-8859-1?Q?VZ/xwPpVVS7M4jJMgMdMGhQMmiY4KzayLxi7NSm3Ekt5dck57PRz5DWfN2?=
 =?iso-8859-1?Q?DY0RnbTmADXU0Fi1rApKO56ZL5DoxbTFuj6xunrJGMGWlhJfbvq8zL1mck?=
 =?iso-8859-1?Q?/gvJZU/8ThO1Y8dfpOc7lfqB/tvqvtXJ2z/M6xoZlQeX7jTsooLUq60PhB?=
 =?iso-8859-1?Q?aby5TOHk8rCnO+ATSV1oIuD6K0/rExtzdGfdr/W0jRqyvVCt/D/r9ObZ7u?=
 =?iso-8859-1?Q?rk2vCA1OJfIiLhQL/HOTTJSGax4Ng1SrYJBZD1DLB4ilh/j61KsZDdo9kb?=
 =?iso-8859-1?Q?2wXByUZfAjgLOKU3YwZESMIbX8YukaOAvQ/1prthxB9nY7S4yEhxp7vfEN?=
 =?iso-8859-1?Q?79JOVOVwlSnx7K7HPXqcBUmABwj+alitxaZImRRJyJapScDbH8VNAZ2ntH?=
 =?iso-8859-1?Q?85uO/ObwcacAxaoJyWq8thWj6vR4VZKqJYfRqq2MHEerfb9TJ3BDmpWo1L?=
 =?iso-8859-1?Q?e6/a9nPFEF6bvJByuxIldpfgqftksKnEV7B/IdN3Uq/cdbDFnF0zjPGylp?=
 =?iso-8859-1?Q?wwRRhHue7e9y0Q+z773XUQ9CJIbx/n0okkVZiLO0rRBmur3lqtYC3/S78F?=
 =?iso-8859-1?Q?Z0ew5+3RrYKceI3p0zIlfcwikaUZ9jG4xVE8TDcbPZ7b3Np3gtXAc56odA?=
 =?iso-8859-1?Q?ITkzH8l94LXVAZBEwIBQs77yEl4OG/z/S0DaoLOomilIa4ldi5hDPW9jBE?=
 =?iso-8859-1?Q?mycsPXiNA1PDqP64kIYtRXtMvRvSXyUjHpQh3/ZpNoALBfN/oegOoHLQ?=
 =?iso-8859-1?Q?=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0c95ae35-76a0-4835-6909-08dc5870ee50
X-MS-Exchange-CrossTenant-AuthSource: BL0PR13MB4403.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Apr 2024 08:42:03.9691
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ApbcjJJEtWaT7vHVHA0LbgGkNF3FQtbTOZQu7gSGUkFA2L+LkNqQ5XaYYcFZ1/eEtKkmasSgS2tyoCMBI3wQF30ft8eFhGL4JA6NVMybV6I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR13MB5474

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.