Re: [PATCH v2] ethdev: add Tx queue flow matching item

[Thomas Monjalon <thomas@monjalon.net>]

08/06/2023 21:59, Thomas Monjalon:
> 08/05/2023 15:49, kirankumark@marvell.com:
> > From: Kiran Kumar K <kirankumark@marvell.com>
> > 
> > Adding support for Tx queue flow matching item.
> > This item is valid only for egress rules.
> > An example use case would be that application can
> > set different vlan insert rules with different PCP values
> > based on Tx queue number.
> > 
> > Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> > ---
> >  app/test-pmd/cmdline_flow.c                 | 28 +++++++++++++++++++++
> >  doc/guides/prog_guide/rte_flow.rst          |  7 ++++++
> >  doc/guides/rel_notes/release_23_07.rst      |  5 ++++
> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
> >  lib/ethdev/rte_flow.c                       |  1 +
> >  lib/ethdev/rte_flow.h                       | 26 +++++++++++++++++++
> >  6 files changed, 71 insertions(+)
> 
> That's only 71 lines but I could make 10 comments.
> I'm fixing spacing, alignment, sorting, etc while pulling next-net,
> but it would have been more confortable if more attention was paid
> by the author and the reviewers.
> 
> 
> I have a question though (it could be fixed in a later patch):
> 
> > +struct rte_flow_item_tx_queue {
> > +	/** Tx queue number that packet is being transmitted */
> > +	uint16_t tx_queue;
> > +};
> 
> This field is used with host endianness, right?
> 
> > +static const struct rte_flow_item_tx_queue rte_flow_item_tx_queue_mask = {
> > +	.tx_queue = RTE_BE16(0xffff),
> > +};
> 
> So why using RTE_BE16 to set a field which is not specifically big endian?
> (not talking about the fact that 0xffff is the same no matter the endianness,
> and not talking about the fact that it is better to use UINT16_MAX).

Repeating again:
This patch has bad quality, bad review,
I've noticed something wrong,
and there are 0 comments or reply!

Now David has sent a fix for this:
https://patches.dpdk.org/project/dpdk/patch/20230629135839.974700-1-david.marchand@redhat.com/

Don't expect next patches to be merged quickly.