diff mbox series

[v2,2/3] net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()

Message ID 20231113170605.408281-3-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series use static_assertion for build errors |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Stephen Hemminger Nov. 13, 2023, 5:06 p.m. UTC 
  The macro RTE_MIN has some hidden assignments to provide type
safety which means the statement can not be fully evaluted in
first pass of compiler. Replace RTE_MIN() with equivalent macro.

This will cause errors from checkpatch about multiple evaluations
of same expression in macro but it is ok in this case.

Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
Cc: ivan.malov@oktetlabs.ru
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/sfc/sfc_ef100_tx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Tyler Retzlaff Nov. 13, 2023, 5:55 p.m. UTC | #1 
On Mon, Nov 13, 2023 at 09:06:04AM -0800, Stephen Hemminger wrote:
> The macro RTE_MIN has some hidden assignments to provide type
> safety which means the statement can not be fully evaluted in
> first pass of compiler. Replace RTE_MIN() with equivalent macro.
> 
> This will cause errors from checkpatch about multiple evaluations
> of same expression in macro but it is ok in this case.
> 
> Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> Cc: ivan.malov@oktetlabs.ru
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Stephen Hemminger Nov. 13, 2023, 10:13 p.m. UTC | #2 
On Mon, 13 Nov 2023 09:06:04 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

> The macro RTE_MIN has some hidden assignments to provide type
> safety which means the statement can not be fully evaluted in
> first pass of compiler. Replace RTE_MIN() with equivalent macro.
> 
> This will cause errors from checkpatch about multiple evaluations
> of same expression in macro but it is ok in this case.
> 
> Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> Cc: ivan.malov@oktetlabs.ru
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Building with clang finds another issue.
../drivers/net/sfc/sfc_rx.c:158:3: error: expected expression
                RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0);
yet
lib/mbuf/rte_mbuf_core.h:#define RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN 0
Tyler Retzlaff Nov. 13, 2023, 10:28 p.m. UTC | #3 
On Mon, Nov 13, 2023 at 02:13:26PM -0800, Stephen Hemminger wrote:
> On Mon, 13 Nov 2023 09:06:04 -0800
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > The macro RTE_MIN has some hidden assignments to provide type
> > safety which means the statement can not be fully evaluted in
> > first pass of compiler. Replace RTE_MIN() with equivalent macro.
> > 
> > This will cause errors from checkpatch about multiple evaluations
> > of same expression in macro but it is ok in this case.
> > 
> > Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> > Cc: ivan.malov@oktetlabs.ru
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> Building with clang finds another issue.
> ../drivers/net/sfc/sfc_rx.c:158:3: error: expected expression
>                 RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0);
> yet
> lib/mbuf/rte_mbuf_core.h:#define RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN 0

curious. do you have the gcc -E / clang -E preprocessed output for the
expansion? wonder what it looks like.
Stephen Hemminger Nov. 14, 2023, midnight UTC | #4 
On Mon, 13 Nov 2023 14:28:55 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Mon, Nov 13, 2023 at 02:13:26PM -0800, Stephen Hemminger wrote:
> > On Mon, 13 Nov 2023 09:06:04 -0800
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> >   
> > > The macro RTE_MIN has some hidden assignments to provide type
> > > safety which means the statement can not be fully evaluted in
> > > first pass of compiler. Replace RTE_MIN() with equivalent macro.
> > > 
> > > This will cause errors from checkpatch about multiple evaluations
> > > of same expression in macro but it is ok in this case.
> > > 
> > > Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> > > Cc: ivan.malov@oktetlabs.ru
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> > 
> > Building with clang finds another issue.
> > ../drivers/net/sfc/sfc_rx.c:158:3: error: expected expression
> >                 RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0);
> > yet
> > lib/mbuf/rte_mbuf_core.h:#define RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN 0  
> 
> curious. do you have the gcc -E / clang -E preprocessed output for the
> expansion? wonder what it looks like.

yet another one from clang.
../drivers/net/sfc/sfc_ef10_rx_ev.h:142:4: error: expected expression
                 RTE_BUILD_BUG_ON(ESE_FZ_L4_CLASS_TCP != ESE_DE_L4_CLASS_TCP);

drivers/common/sfc_efx/base/efx_regs_ef10.h:#define     ESE_FZ_L4_CLASS_TCP 1
drivers/common/sfc_efx/base/efx_regs_ef10.h:#define     ESE_DE_L4_CLASS_TCP 1
Stephen Hemminger Nov. 14, 2023, 12:16 a.m. UTC | #5 
On Mon, 13 Nov 2023 14:28:55 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Mon, Nov 13, 2023 at 02:13:26PM -0800, Stephen Hemminger wrote:
> > On Mon, 13 Nov 2023 09:06:04 -0800
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> >   
> > > The macro RTE_MIN has some hidden assignments to provide type
> > > safety which means the statement can not be fully evaluted in
> > > first pass of compiler. Replace RTE_MIN() with equivalent macro.
> > > 
> > > This will cause errors from checkpatch about multiple evaluations
> > > of same expression in macro but it is ok in this case.
> > > 
> > > Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> > > Cc: ivan.malov@oktetlabs.ru
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> > 
> > Building with clang finds another issue.
> > ../drivers/net/sfc/sfc_rx.c:158:3: error: expected expression
> >                 RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0);
> > yet
> > lib/mbuf/rte_mbuf_core.h:#define RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN 0  
> 
> curious. do you have the gcc -E / clang -E preprocessed output for the
> expansion? wonder what it looks like.

Building with clang and -E.

This code:
	switch (desc_flags & (EFX_PKT_IPV4 | EFX_CKSUM_IPV4)) {
	case (EFX_PKT_IPV4 | EFX_CKSUM_IPV4):
		mbuf_flags |= RTE_MBUF_F_RX_IP_CKSUM_GOOD;
		break;
	case EFX_PKT_IPV4:
		mbuf_flags |= RTE_MBUF_F_RX_IP_CKSUM_BAD;
		break;
	default:
		RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0);
		SFC_ASSERT((mbuf_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) ==
			   RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN);
		break;
	}

Becomes:
 switch (desc_flags & (0x0800 | 0x0040)) {
 case (0x0800 | 0x0040):
  mbuf_flags |= (1ULL << 7);
  break;
 case 0x0800:
  mbuf_flags |= (1ULL << 4);
  break;
 default:
  _Static_assert(!(0 != 0), "RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0");
  do {} while (0);

  break;
 }

Doing same with Gcc:
 switch (desc_flags & (0x0800 | 0x0040)) {
 case (0x0800 | 0x0040):
  mbuf_flags |= (1ULL << 7);
  break;
 case 0x0800:
  mbuf_flags |= (1ULL << 4);
  break;
 default:
  
# 158 "./drivers/net/sfc/sfc_rx.c" 3 4
 _Static_assert
# 158 "./drivers/net/sfc/sfc_rx.c"
 (!(0 != 0), "RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0");
  do {} while (0)
                                     ;
Stephen Hemminger Nov. 14, 2023, 12:22 a.m. UTC | #6 
On Mon, 13 Nov 2023 16:16:35 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

>   _Static_assert(!(0 != 0), "RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0");

Looks like a clang bug, or something about the other compiler flags
because compiling just this part is fine.
Morten Brørup Nov. 14, 2023, 5:50 a.m. UTC | #7 
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, 14 November 2023 01.22
> 
> On Mon, 13 Nov 2023 16:16:35 -0800
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> >   _Static_assert(!(0 != 0), "RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0");
> 
> Looks like a clang bug, or something about the other compiler flags
> because compiling just this part is fine.

Ideas:

Perhaps this file is not compiled as C11, so _Static_assert is not recognized. Does that part compile in the same file?
Or perhaps the C version or some compiler flags are modified (by a #pragma or similar) in the file.
diff mbox series

Patch

diff --git a/drivers/net/sfc/sfc_ef100_tx.c b/drivers/net/sfc/sfc_ef100_tx.c
index 1b6374775f07..f4bcadc1e8e0 100644
--- a/drivers/net/sfc/sfc_ef100_tx.c
+++ b/drivers/net/sfc/sfc_ef100_tx.c
@@ -26,6 +26,10 @@ 
 #include "sfc_ef100.h"
 #include "sfc_nic_dma_dp.h"
 
+#ifndef MIN
+/* not typesafe but is a constant */
+#define MIN(x, y) ((x) < (y) ? (x) : (y))
+#endif
 
 #define sfc_ef100_tx_err(_txq, ...) \
 	SFC_DP_LOG(SFC_KVARG_DATAPATH_EF100, ERR, &(_txq)->dp.dpq, __VA_ARGS__)
@@ -563,8 +567,7 @@  sfc_ef100_tx_pkt_descs_max(const struct rte_mbuf *m)
 		 * (split into many Tx descriptors).
 		 */
 		RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX <
-				 RTE_MIN((unsigned int)EFX_MAC_PDU_MAX,
-				 SFC_MBUF_SEG_LEN_MAX));
+				 MIN((unsigned int)EFX_MAC_PDU_MAX, SFC_MBUF_SEG_LEN_MAX));
 	}
 
 	if (m->ol_flags & sfc_dp_mport_override) {