Hi,
Thank you for the patch. Please see comments inline.
On Tue, Aug 12, 2025 at 08:46:30AM -0400, Khadem Ullah wrote:
> This patch validate a connection tracking state when matching
> 'conntrack is' in rte_flow rules. Since conntrack item flags
> is a bitmap, then any combination of RTE_FLOW_CONNTRACK_PKT_STATE_*
> flags is a valid value to match on.
>
> This patch validate the CT state item.
> Fixes: aca19061e4b9 ('net/mlx5: validate connection tracking item')
When used locally ./devtool/checkpatches.sh reports the following:
WARNING:BAD_FIXES_TAG: Please use correct Fixes: style 'Fixes: <12+
chars of sha1> ("<title line>")' - ie: 'Fixes: aca19061e4b9 ("net/mlx5:
validate connection tracking item")'
#12:
Fixes: aca19061e4b9 ('net/mlx5: validate connection tracking item')
but it's not reported in "ci/checkpatch" job in Patchwork.
This check was added in Linux 6.1:
https://github.com/torvalds/linux/commit/bd17e036b495bebbf07a5fc814c868e30e1dc131
so it appears that checkpatch.pl version used in CI is older than that.
Locally, I have a version from Linux 6.16.
Ali, Thomas, David: What do you think about updating the version of
checkpatch.pl used in community CI?
Khadem: Could you please fix the quotations marks in Fixes tag according to the
error?
For the future, you can set up a git alias, which can be used
to automatically generate a correct tag.
It is described in:
https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body
> Cc: sta...@dpdk.org
>
> Signed-off-by: Khadem Ullah <14pwcse1...@uetpeshawar.edu.pk>
> ---
> drivers/net/mlx5/mlx5_flow_dv.c | 12 +++++++++++-
> drivers/net/mlx5/mlx5_flow_hw.c | 17 ++++++++++++++++-
> 2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
> index 7b9e5018b8..19475b931f 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -3271,7 +3271,7 @@ mlx5_flow_dv_validate_item_aso_ct(struct rte_eth_dev
> *dev,
> {
> const struct rte_flow_item_conntrack *spec = item->spec;
> const struct rte_flow_item_conntrack *mask = item->mask;
> - uint32_t flags;
> + uint32_t flags, flags_all;
>
> if (*item_flags & MLX5_FLOW_LAYER_ASO_CT)
> return rte_flow_error_set(error, EINVAL,
> @@ -3289,6 +3289,16 @@ mlx5_flow_dv_validate_item_aso_ct(struct rte_eth_dev
> *dev,
> RTE_FLOW_ERROR_TYPE_ITEM,
> NULL,
> "Conflict status bits");
> + flags_all = (RTE_FLOW_CONNTRACK_PKT_STATE_VALID |
> + RTE_FLOW_CONNTRACK_PKT_STATE_CHANGED |
> + RTE_FLOW_CONNTRACK_PKT_STATE_INVALID |
> + RTE_FLOW_CONNTRACK_PKT_STATE_DISABLED |
> + RTE_FLOW_CONNTRACK_PKT_STATE_BAD);
I think it would be better to define a macro for "all possible packet state
flags", because now the definition of all flags is repeated in 2 places.
Could you please add MLX5_FLOW_CONNTRACK_PKT_STATE_ALL macro to mlx5_flow.h?
> + if (spec->flags & ~flags_all)
> + return rte_flow_error_set(error, EINVAL,
> + RTE_FLOW_ERROR_TYPE_ITEM,
> + NULL,
> + "Invalid CT item matching \n");
Please remove the new line character and preceding space from error message
here.
rte_flow_error_set() does not print the message, it only stores
the pointer to error message in rte_flow_error struct.
Users later can print it/log it however they like.
I also propose to change the message itself, so it better pinpoints
the exact error. For example: "Invalid CT item flags".
Please apply the same to the second instance of that error.
> }
> /* State change also needs to be considered. */
> *item_flags |= MLX5_FLOW_LAYER_ASO_CT;
> diff --git a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c
> index 6dc16f80d3..6dbbc44819 100644
> --- a/drivers/net/mlx5/mlx5_flow_hw.c
> +++ b/drivers/net/mlx5/mlx5_flow_hw.c
> @@ -16957,7 +16957,6 @@ flow_hw_validate_rule_pattern(struct rte_eth_dev *dev,
> {
> const struct rte_flow_pattern_template *pt;
> const struct rte_flow_item *pt_item;
> -
When you'll be sending a new version, could you reapply this empty line?
It's removal is not needed.
> /* snip rest of the diff */
Best regards,
Dariusz Sosnowski
