diff mbox series

ethdev: fix Tx queue mask endianness

Message ID 20230629135839.974700-1-david.marchand@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: fix Tx queue mask endianness |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-abi-testing success Testing PASS
ci/iol-aarch-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-unit-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS

Commit Message

David Marchand June 29, 2023, 1:58 p.m. UTC 
  Even if harmless, this endianness tag is incorrect as the tx_queue field
is declared as a host integer.
Additionally, this breaks OVS compilation with sparse.

Fixes: 41f6bdc7615a ("ethdev: add Tx queue flow matching item")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/ethdev/rte_flow.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ferruh Yigit June 29, 2023, 2:48 p.m. UTC | #1 
On 6/29/2023 2:58 PM, David Marchand wrote:
> Even if harmless, this endianness tag is incorrect as the tx_queue field
> is declared as a host integer.
> Additionally, this breaks OVS compilation with sparse.
> 
> Fixes: 41f6bdc7615a ("ethdev: add Tx queue flow matching item")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.
Thomas Monjalon June 29, 2023, 3:31 p.m. UTC | #2 
29/06/2023 15:58, David Marchand:
> Even if harmless, this endianness tag is incorrect as the tx_queue field
> is declared as a host integer.
> Additionally, this breaks OVS compilation with sparse.
> 
> Fixes: 41f6bdc7615a ("ethdev: add Tx queue flow matching item")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/ethdev/rte_flow.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index 3fe57140f9..86ed98c562 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -2307,7 +2307,7 @@ struct rte_flow_item_tx_queue {
>  /** Default mask for RTE_FLOW_ITEM_TX_QUEUE. */
>  #ifndef __cplusplus
>  static const struct rte_flow_item_tx_queue rte_flow_item_tx_queue_mask = {
> -	.tx_queue = RTE_BE16(0xffff),
> +	.tx_queue = 0xffff,

As I said in an earlier comment about the same issue,
UINT16_MAX would be better.
David Marchand June 29, 2023, 3:40 p.m. UTC | #3 
On Thu, Jun 29, 2023 at 5:31 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 29/06/2023 15:58, David Marchand:
> > Even if harmless, this endianness tag is incorrect as the tx_queue field
> > is declared as a host integer.
> > Additionally, this breaks OVS compilation with sparse.
> >
> > Fixes: 41f6bdc7615a ("ethdev: add Tx queue flow matching item")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  lib/ethdev/rte_flow.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > index 3fe57140f9..86ed98c562 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -2307,7 +2307,7 @@ struct rte_flow_item_tx_queue {
> >  /** Default mask for RTE_FLOW_ITEM_TX_QUEUE. */
> >  #ifndef __cplusplus
> >  static const struct rte_flow_item_tx_queue rte_flow_item_tx_queue_mask = {
> > -     .tx_queue = RTE_BE16(0xffff),
> > +     .tx_queue = 0xffff,
>
> As I said in an earlier comment about the same issue,
> UINT16_MAX would be better.

I don't mind updating (or maybe Ferruh can squash this directly ?) but
there are lots of uint16_t fields initialised with 0xffff in this same
file.
Thomas Monjalon June 29, 2023, 3:42 p.m. UTC | #4 
29/06/2023 17:40, David Marchand:
> On Thu, Jun 29, 2023 at 5:31 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > 29/06/2023 15:58, David Marchand:
> > > -     .tx_queue = RTE_BE16(0xffff),
> > > +     .tx_queue = 0xffff,
> >
> > As I said in an earlier comment about the same issue,
> > UINT16_MAX would be better.
> 
> I don't mind updating (or maybe Ferruh can squash this directly ?) but
> there are lots of uint16_t fields initialised with 0xffff in this same
> file.

It can be made in a separate patch for all occurences.
First I would like to get some comments, what do you prefer
between 0xffff and UINT16_MAX?
Ferruh Yigit June 29, 2023, 4:14 p.m. UTC | #5 
On 6/29/2023 4:42 PM, Thomas Monjalon wrote:
> 29/06/2023 17:40, David Marchand:
>> On Thu, Jun 29, 2023 at 5:31 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>> 29/06/2023 15:58, David Marchand:
>>>> -     .tx_queue = RTE_BE16(0xffff),
>>>> +     .tx_queue = 0xffff,
>>>
>>> As I said in an earlier comment about the same issue,
>>> UINT16_MAX would be better.
>>
>> I don't mind updating (or maybe Ferruh can squash this directly ?) but
>> there are lots of uint16_t fields initialised with 0xffff in this same
>> file.
> 
> It can be made in a separate patch for all occurences.
> First I would like to get some comments, what do you prefer
> between 0xffff and UINT16_MAX?
> 

Both works, no strong opinion, I am OK with 0xffff,

The variable we are setting is '*_mask', and main point of the value
used is to have all bits set, and 0xff.. usage highlights it.

Not for UINT16_MAX, but for wider variables, it is easier to make
mistake and put wrong number of 'f', using 'UINTxx_MAX' macro can
prevent this mistake, this is a benefit.


And I think consistency matters more, so if you prefer 'UINTxx_MAX',
lets stick to it.

I can update above in next-net, but as far as I understand we can have a
patch to fix all occurrences.
David Marchand June 30, 2023, 7 a.m. UTC | #6 
On Thu, Jun 29, 2023 at 6:14 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 6/29/2023 4:42 PM, Thomas Monjalon wrote:
> > 29/06/2023 17:40, David Marchand:
> >> On Thu, Jun 29, 2023 at 5:31 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >>> 29/06/2023 15:58, David Marchand:
> >>>> -     .tx_queue = RTE_BE16(0xffff),
> >>>> +     .tx_queue = 0xffff,
> >>>
> >>> As I said in an earlier comment about the same issue,
> >>> UINT16_MAX would be better.
> >>
> >> I don't mind updating (or maybe Ferruh can squash this directly ?) but
> >> there are lots of uint16_t fields initialised with 0xffff in this same
> >> file.
> >
> > It can be made in a separate patch for all occurences.
> > First I would like to get some comments, what do you prefer
> > between 0xffff and UINT16_MAX?
> >
>
> Both works, no strong opinion, I am OK with 0xffff,
>
> The variable we are setting is '*_mask', and main point of the value
> used is to have all bits set, and 0xff.. usage highlights it.
>
> Not for UINT16_MAX, but for wider variables, it is easier to make
> mistake and put wrong number of 'f', using 'UINTxx_MAX' macro can
> prevent this mistake, this is a benefit.
>
>
> And I think consistency matters more, so if you prefer 'UINTxx_MAX',
> lets stick to it.
>
> I can update above in next-net, but as far as I understand we can have a
> patch to fix all occurrences.

Given that we are considering unsigned integers, is there something
wrong with using (typeof(var)) -1 ?
We could define a new macro to hide this ugly detail.
David Marchand June 30, 2023, 7:13 a.m. UTC | #7 
On Fri, Jun 30, 2023 at 9:00 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Thu, Jun 29, 2023 at 6:14 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >
> > On 6/29/2023 4:42 PM, Thomas Monjalon wrote:
> > > 29/06/2023 17:40, David Marchand:
> > >> On Thu, Jun 29, 2023 at 5:31 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >>> 29/06/2023 15:58, David Marchand:
> > >>>> -     .tx_queue = RTE_BE16(0xffff),
> > >>>> +     .tx_queue = 0xffff,
> > >>>
> > >>> As I said in an earlier comment about the same issue,
> > >>> UINT16_MAX would be better.
> > >>
> > >> I don't mind updating (or maybe Ferruh can squash this directly ?) but
> > >> there are lots of uint16_t fields initialised with 0xffff in this same
> > >> file.
> > >
> > > It can be made in a separate patch for all occurences.
> > > First I would like to get some comments, what do you prefer
> > > between 0xffff and UINT16_MAX?
> > >
> >
> > Both works, no strong opinion, I am OK with 0xffff,
> >
> > The variable we are setting is '*_mask', and main point of the value
> > used is to have all bits set, and 0xff.. usage highlights it.
> >
> > Not for UINT16_MAX, but for wider variables, it is easier to make
> > mistake and put wrong number of 'f', using 'UINTxx_MAX' macro can
> > prevent this mistake, this is a benefit.
> >
> >
> > And I think consistency matters more, so if you prefer 'UINTxx_MAX',
> > lets stick to it.
> >
> > I can update above in next-net, but as far as I understand we can have a
> > patch to fix all occurrences.
>
> Given that we are considering unsigned integers, is there something
> wrong with using (typeof(var)) -1 ?

Or maybe get inspiration from what the Linux kernel does :-)
Like GENMASK().
diff mbox series

Patch

diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index 3fe57140f9..86ed98c562 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -2307,7 +2307,7 @@  struct rte_flow_item_tx_queue {
 /** Default mask for RTE_FLOW_ITEM_TX_QUEUE. */
 #ifndef __cplusplus
 static const struct rte_flow_item_tx_queue rte_flow_item_tx_queue_mask = {
-	.tx_queue = RTE_BE16(0xffff),
+	.tx_queue = 0xffff,
 };
 #endif