Received-SPF: pass (google.com: domain of linux-kernel+bounces-136732-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <2c7bc566-c975-4dd8-a17c-10c7b9ff3928@fiberby.net>
Date: Tue, 9 Apr 2024 11:13:22 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 1/6] flow_offload: add
 flow_rule_no_unsupp_control_flags()
To: Louis Peens <louis.peens@corigine.com>
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>
References: <20240408130927.78594-1-ast@fiberby.net>
 <20240408130927.78594-2-ast@fiberby.net> <ZhT/E1qDsMmMxGwb@LouisNoVo>
Content-Language: en-US
From: =?UTF-8?Q?Asbj=C3=B8rn_Sloth_T=C3=B8nnesen?= <ast@fiberby.net>
In-Reply-To: <ZhT/E1qDsMmMxGwb@LouisNoVo>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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

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