[dpdk-dev] [PATCH] drivers/net: do not redefine bool

[dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Thomas Monjalon]

When trying to include stdbool.h in DPDK base headers, there are a lot
of conflicts with drivers which redefine bool/true/false
in their compatibility layer.

It is fixed by including stdbool.h in these drivers.
Some errors with usage of bool type are also fixed in some drivers.

Note: the driver qede has a surprising mix of bool and int:
	(~p_iov->b_pre_fp_hsi & ETH_HSI_VER_MINOR)
where the first variable is boolean and the version is a number.
It is replaced by
	!p_iov->b_pre_fp_hsi

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/cxgbe/cxgbe_compat.h         |  2 +-
 drivers/net/e1000/base/e1000_osdep.h     |  5 +----
 drivers/net/fm10k/base/fm10k_osdep.h     |  8 +-------
 drivers/net/fm10k/fm10k_ethdev.c         |  4 ++--
 drivers/net/ixgbe/base/ixgbe_osdep.h     |  6 +-----
 drivers/net/ixgbe/ixgbe_ethdev.c         | 16 +++++++++-------
 drivers/net/ixgbe/ixgbe_rxtx.c           |  2 +-
 drivers/net/qede/base/bcm_osal.h         |  6 ++----
 drivers/net/qede/base/ecore_vf.c         |  3 +--
 drivers/net/qede/qede_ethdev.c           |  2 +-
 drivers/net/vmxnet3/base/vmxnet3_osdep.h |  3 ++-
 11 files changed, 22 insertions(+), 35 deletions(-)

diff --git a/drivers/net/cxgbe/cxgbe_compat.h b/drivers/net/cxgbe/cxgbe_compat.h
index 5d47c5f3d..2650ffcab 100644
--- a/drivers/net/cxgbe/cxgbe_compat.h
+++ b/drivers/net/cxgbe/cxgbe_compat.h
@@ -7,6 +7,7 @@
 #define _CXGBE_COMPAT_H_
 
 #include <string.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdarg.h>
@@ -106,7 +107,6 @@ typedef uint16_t  u16;
 typedef uint32_t  u32;
 typedef int32_t   s32;
 typedef uint64_t  u64;
-typedef int       bool;
 typedef uint64_t  dma_addr_t;
 
 #ifndef __le16
diff --git a/drivers/net/e1000/base/e1000_osdep.h b/drivers/net/e1000/base/e1000_osdep.h
index b8868049f..556ed1742 100644
--- a/drivers/net/e1000/base/e1000_osdep.h
+++ b/drivers/net/e1000/base/e1000_osdep.h
@@ -35,6 +35,7 @@
 #ifndef _E1000_OSDEP_H_
 #define _E1000_OSDEP_H_
 
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdarg.h>
@@ -87,7 +88,6 @@ typedef int64_t		s64;
 typedef int32_t		s32;
 typedef int16_t		s16;
 typedef int8_t		s8;
-typedef int		bool;
 
 #define __le16		u16
 #define __le32		u32
@@ -192,7 +192,4 @@ static inline uint16_t e1000_read_addr16(volatile void *addr)
 #define ETH_ADDR_LEN                  6
 #endif
 
-#define false                         FALSE
-#define true                          TRUE
-
 #endif /* _E1000_OSDEP_H_ */
diff --git a/drivers/net/fm10k/base/fm10k_osdep.h b/drivers/net/fm10k/base/fm10k_osdep.h
index 199ebd8ea..9665239fd 100644
--- a/drivers/net/fm10k/base/fm10k_osdep.h
+++ b/drivers/net/fm10k/base/fm10k_osdep.h
@@ -34,6 +34,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #ifndef _FM10K_OSDEP_H_
 #define _FM10K_OSDEP_H_
 
+#include <stdbool.h>
 #include <stdint.h>
 #include <string.h>
 #include <rte_atomic.h>
@@ -61,12 +62,6 @@ POSSIBILITY OF SUCH DAMAGE.
 
 #define FALSE      0
 #define TRUE       1
-#ifndef false
-#define false      FALSE
-#endif
-#ifndef true
-#define true       TRUE
-#endif
 
 typedef uint8_t    u8;
 typedef int8_t     s8;
@@ -76,7 +71,6 @@ typedef uint32_t   u32;
 typedef int32_t    s32;
 typedef int64_t    s64;
 typedef uint64_t   u64;
-typedef int        bool;
 
 #ifndef __le16
 #define __le16     u16
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index 3359df3c8..243066e08 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -3132,7 +3132,7 @@ eth_fm10k_dev_init(struct rte_eth_dev *dev)
 
 	/* Make sure Switch Manager is ready before going forward. */
 	if (hw->mac.type == fm10k_mac_pf) {
-		int switch_ready = 0;
+		bool switch_ready = false;
 
 		for (i = 0; i < MAX_QUERY_SWITCH_STATE_TIMES; i++) {
 			fm10k_mbx_lock(hw);
@@ -3144,7 +3144,7 @@ eth_fm10k_dev_init(struct rte_eth_dev *dev)
 			rte_delay_us(WAIT_SWITCH_MSG_US);
 		}
 
-		if (switch_ready == 0) {
+		if (!switch_ready) {
 			PMD_INIT_LOG(ERR, "switch is not ready");
 			return -1;
 		}
diff --git a/drivers/net/ixgbe/base/ixgbe_osdep.h b/drivers/net/ixgbe/base/ixgbe_osdep.h
index bb5dfd2af..39e9118aa 100644
--- a/drivers/net/ixgbe/base/ixgbe_osdep.h
+++ b/drivers/net/ixgbe/base/ixgbe_osdep.h
@@ -36,6 +36,7 @@
 #define _IXGBE_OS_H_
 
 #include <string.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdarg.h>
@@ -70,8 +71,6 @@
 #define FALSE               0
 #define TRUE                1
 
-#define false               0
-#define true                1
 #define min(a,b)	RTE_MIN(a,b) 
 
 #define EWARN(hw, S, args...)     DEBUGOUT1(S, ##args)
@@ -112,9 +111,6 @@ typedef int16_t		s16;
 typedef uint32_t	u32;
 typedef int32_t		s32;
 typedef uint64_t	u64;
-#ifndef __cplusplus
-typedef int		bool;
-#endif
 
 #define mb()	rte_mb()
 #define wmb()	rte_wmb()
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index cee886754..c272a4112 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2527,7 +2527,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
 	uint32_t intr_vector = 0;
-	int err, link_up = 0, negotiate = 0;
+	int err;
+	bool negotiate = false;
+	bool link_up = false;
 	uint32_t speed = 0;
 	uint32_t allowed_speeds = 0;
 	int mask = 0;
@@ -2669,7 +2671,7 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 		ixgbe_enable_tx_laser(hw);
 	}
 
-	err = ixgbe_check_link(hw, &speed, &link_up, 0);
+	err = ixgbe_check_link(hw, &speed, &link_up, false);
 	if (err)
 		goto error;
 	dev->data->dev_link.link_status = link_up;
@@ -3870,7 +3872,7 @@ ixgbevf_dev_info_get(struct rte_eth_dev *dev,
 
 static int
 ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
-		   int *link_up, int wait_to_complete)
+		   bool *link_up, int wait_to_complete)
 {
 	/**
 	 * for a quick link status checking, wait_to_compelet == 0,
@@ -3984,10 +3986,10 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 	ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN;
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
-	int link_up;
+	bool link_up;
 	int diag;
 	u32 speed = 0;
-	int wait = 1;
+	bool wait = true;
 	bool autoneg = false;
 
 	memset(&link, 0, sizeof(link));
@@ -4008,7 +4010,7 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 
 	/* check if it needs to wait to complete, if lsc interrupt is enabled */
 	if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0)
-		wait = 0;
+		wait = false;
 
 	if (vf)
 		diag = ixgbevf_check_link(hw, &link_speed, &link_up, wait);
@@ -4021,7 +4023,7 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 		return rte_eth_linkstatus_set(dev, &link);
 	}
 
-	if (link_up == 0) {
+	if (!link_up) {
 		intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
 		return rte_eth_linkstatus_set(dev, &link);
 	}
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index ae21f04a1..2dc14c47f 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2025,7 +2025,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
 		struct ixgbe_rx_entry *next_rxe = NULL;
 		struct rte_mbuf *first_seg;
 		struct rte_mbuf *rxm;
-		struct rte_mbuf *nmb;
+		struct rte_mbuf *nmb = NULL;
 		union ixgbe_adv_rx_desc rxd;
 		uint16_t data_len;
 		uint16_t next_id;
diff --git a/drivers/net/qede/base/bcm_osal.h b/drivers/net/qede/base/bcm_osal.h
index 630867fad..060ef8e1d 100644
--- a/drivers/net/qede/base/bcm_osal.h
+++ b/drivers/net/qede/base/bcm_osal.h
@@ -7,6 +7,8 @@
 #ifndef __BCM_OSAL_H
 #define __BCM_OSAL_H
 
+#include <stdbool.h>
+
 #include <rte_byteorder.h>
 #include <rte_spinlock.h>
 #include <rte_malloc.h>
@@ -71,10 +73,6 @@ typedef size_t osal_size_t;
 
 typedef intptr_t osal_int_ptr_t;
 
-typedef int bool;
-#define true 1
-#define false 0
-
 #define nothing do {} while (0)
 
 /* Delays */
diff --git a/drivers/net/qede/base/ecore_vf.c b/drivers/net/qede/base/ecore_vf.c
index d2213f793..f5deb2916 100644
--- a/drivers/net/qede/base/ecore_vf.c
+++ b/drivers/net/qede/base/ecore_vf.c
@@ -445,8 +445,7 @@ static enum _ecore_status_t ecore_vf_pf_acquire(struct ecore_hwfn *p_hwfn)
 	}
 
 	/* @DPDK */
-	if ((~p_iov->b_pre_fp_hsi &
-	    ETH_HSI_VER_MINOR) &&
+	if (!p_iov->b_pre_fp_hsi &&
 	    (resp->pfdev_info.minor_fp_hsi < ETH_HSI_VER_MINOR))
 		DP_INFO(p_hwfn,
 			"PF is using older fastpath HSI;"
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index 7bb52b157..53a767b3e 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -534,7 +534,7 @@ int qede_activate_vport(struct rte_eth_dev *eth_dev, bool flg)
 	params.update_vport_active_tx_flg = 1;
 	params.vport_active_rx_flg = flg;
 	params.vport_active_tx_flg = flg;
-	if (~qdev->enable_tx_switching & flg) {
+	if (!qdev->enable_tx_switching && flg) {
 		params.update_tx_switching_flg = 1;
 		params.tx_switching_flg = !flg;
 	}
diff --git a/drivers/net/vmxnet3/base/vmxnet3_osdep.h b/drivers/net/vmxnet3/base/vmxnet3_osdep.h
index c9b92b049..12b390acb 100644
--- a/drivers/net/vmxnet3/base/vmxnet3_osdep.h
+++ b/drivers/net/vmxnet3/base/vmxnet3_osdep.h
@@ -5,11 +5,12 @@
 #ifndef _VMXNET3_OSDEP_H
 #define _VMXNET3_OSDEP_H
 
+#include <stdbool.h>
+
 typedef uint64_t	uint64;
 typedef uint32_t	uint32;
 typedef uint16_t	uint16;
 typedef uint8_t		uint8;
-typedef int		bool;
 typedef char		Bool;
 
 #ifndef UNLIKELY
-- 
2.19.0

Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Shaikh, Shahed]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, September 20, 2018 5:49 AM
> To: Ferruh Yigit <ferruh.yigit@intel.com>; Rahul Lakkireddy
> <rahul.lakkireddy@chelsio.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>; Qi
> Zhang <qi.z.zhang@intel.com>; Xiao Wang <xiao.w.wang@intel.com>;
> Konstantin Ananyev <konstantin.ananyev@intel.com>; Mody, Rasesh
> <Rasesh.Mody@cavium.com>; Patil, Harish <Harish.Patil@cavium.com>; Shaikh,
> Shahed <Shahed.Shaikh@cavium.com>; Yong Wang <yongwang@vmware.com>
> Cc: dev@dpdk.org
> Subject: [PATCH] drivers/net: do not redefine bool
> 
> External Email
> 
> When trying to include stdbool.h in DPDK base headers, there are a lot
> of conflicts with drivers which redefine bool/true/false
> in their compatibility layer.
> 
> It is fixed by including stdbool.h in these drivers.
> Some errors with usage of bool type are also fixed in some drivers.
> 
> Note: the driver qede has a surprising mix of bool and int:
>         (~p_iov->b_pre_fp_hsi & ETH_HSI_VER_MINOR)
> where the first variable is boolean and the version is a number.
> It is replaced by
>         !p_iov->b_pre_fp_hsi
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  drivers/net/cxgbe/cxgbe_compat.h         |  2 +-
>  drivers/net/e1000/base/e1000_osdep.h     |  5 +----
>  drivers/net/fm10k/base/fm10k_osdep.h     |  8 +-------
>  drivers/net/fm10k/fm10k_ethdev.c         |  4 ++--
>  drivers/net/ixgbe/base/ixgbe_osdep.h     |  6 +-----
>  drivers/net/ixgbe/ixgbe_ethdev.c         | 16 +++++++++-------
>  drivers/net/ixgbe/ixgbe_rxtx.c           |  2 +-
>  drivers/net/qede/base/bcm_osal.h         |  6 ++----
>  drivers/net/qede/base/ecore_vf.c         |  3 +--
>  drivers/net/qede/qede_ethdev.c           |  2 +-
>  drivers/net/vmxnet3/base/vmxnet3_osdep.h |  3 ++-
>  11 files changed, 22 insertions(+), 35 deletions(-)
> 
 ...
> 
>  /* Delays */
> diff --git a/drivers/net/qede/base/ecore_vf.c
> b/drivers/net/qede/base/ecore_vf.c
> index d2213f793..f5deb2916 100644
> --- a/drivers/net/qede/base/ecore_vf.c
> +++ b/drivers/net/qede/base/ecore_vf.c
> @@ -445,8 +445,7 @@ static enum _ecore_status_t ecore_vf_pf_acquire(struct
> ecore_hwfn *p_hwfn)
>         }
> 
>         /* @DPDK */
> -       if ((~p_iov->b_pre_fp_hsi &
> -           ETH_HSI_VER_MINOR) &&
> +       if (!p_iov->b_pre_fp_hsi &&
>             (resp->pfdev_info.minor_fp_hsi < ETH_HSI_VER_MINOR))
>                 DP_INFO(p_hwfn,
>                         "PF is using older fastpath HSI;"
> diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
> index 7bb52b157..53a767b3e 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -534,7 +534,7 @@ int qede_activate_vport(struct rte_eth_dev *eth_dev,
> bool flg)
>         params.update_vport_active_tx_flg = 1;
>         params.vport_active_rx_flg = flg;
>         params.vport_active_tx_flg = flg;
> -       if (~qdev->enable_tx_switching & flg) {
> +       if (!qdev->enable_tx_switching && flg) {
>                 params.update_tx_switching_flg = 1;
>                 params.tx_switching_flg = !flg;
>         }

For qede changes -
Acked-by: Shahed Shaikh <shahed.shaikh@cavium.com>

Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Ferruh Yigit]

On 9/20/2018 1:18 AM, Thomas Monjalon wrote:
> When trying to include stdbool.h in DPDK base headers, there are a lot
> of conflicts with drivers which redefine bool/true/false
> in their compatibility layer.
> 
> It is fixed by including stdbool.h in these drivers.
> Some errors with usage of bool type are also fixed in some drivers.
> 
> Note: the driver qede has a surprising mix of bool and int:
> 	(~p_iov->b_pre_fp_hsi & ETH_HSI_VER_MINOR)
> where the first variable is boolean and the version is a number.
> It is replaced by
> 	!p_iov->b_pre_fp_hsi
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  drivers/net/cxgbe/cxgbe_compat.h         |  2 +-
>  drivers/net/e1000/base/e1000_osdep.h     |  5 +----
>  drivers/net/fm10k/base/fm10k_osdep.h     |  8 +-------
>  drivers/net/fm10k/fm10k_ethdev.c         |  4 ++--
>  drivers/net/ixgbe/base/ixgbe_osdep.h     |  6 +-----
>  drivers/net/ixgbe/ixgbe_ethdev.c         | 16 +++++++++-------
>  drivers/net/ixgbe/ixgbe_rxtx.c           |  2 +-
>  drivers/net/qede/base/bcm_osal.h         |  6 ++----
>  drivers/net/qede/base/ecore_vf.c         |  3 +--
>  drivers/net/qede/qede_ethdev.c           |  2 +-
>  drivers/net/vmxnet3/base/vmxnet3_osdep.h |  3 ++-
>  11 files changed, 22 insertions(+), 35 deletions(-)

<...>

> @@ -35,6 +35,7 @@
>  #ifndef _E1000_OSDEP_H_
>  #define _E1000_OSDEP_H_
>  
> +#include <stdbool.h>
>  #include <stdint.h>
>  #include <stdio.h>
>  #include <stdarg.h>
> @@ -87,7 +88,6 @@ typedef int64_t		s64;
>  typedef int32_t		s32;
>  typedef int16_t		s16;
>  typedef int8_t		s8;
> -typedef int		bool;
>  
>  #define __le16		u16
>  #define __le32		u32
> @@ -192,7 +192,4 @@ static inline uint16_t e1000_read_addr16(volatile void *addr)
>  #define ETH_ADDR_LEN                  6
>  #endif
>  
> -#define false                         FALSE
> -#define true                          TRUE
> -

It is too much hassle to update Intel base driver code. What would happen if not
include stdbool and keep define for base code updates? Will it break build for
applications?

Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Thomas Monjalon]

21/09/2018 15:47, Ferruh Yigit:
> On 9/20/2018 1:18 AM, Thomas Monjalon wrote:
> > When trying to include stdbool.h in DPDK base headers, there are a lot
> > of conflicts with drivers which redefine bool/true/false
> > in their compatibility layer.
> > 
> > It is fixed by including stdbool.h in these drivers.
> > Some errors with usage of bool type are also fixed in some drivers.
> > 
> > Note: the driver qede has a surprising mix of bool and int:
> > 	(~p_iov->b_pre_fp_hsi & ETH_HSI_VER_MINOR)
> > where the first variable is boolean and the version is a number.
> > It is replaced by
> > 	!p_iov->b_pre_fp_hsi
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >  drivers/net/cxgbe/cxgbe_compat.h         |  2 +-
> >  drivers/net/e1000/base/e1000_osdep.h     |  5 +----
> >  drivers/net/fm10k/base/fm10k_osdep.h     |  8 +-------
> >  drivers/net/fm10k/fm10k_ethdev.c         |  4 ++--
> >  drivers/net/ixgbe/base/ixgbe_osdep.h     |  6 +-----
> >  drivers/net/ixgbe/ixgbe_ethdev.c         | 16 +++++++++-------
> >  drivers/net/ixgbe/ixgbe_rxtx.c           |  2 +-
> >  drivers/net/qede/base/bcm_osal.h         |  6 ++----
> >  drivers/net/qede/base/ecore_vf.c         |  3 +--
> >  drivers/net/qede/qede_ethdev.c           |  2 +-
> >  drivers/net/vmxnet3/base/vmxnet3_osdep.h |  3 ++-
> >  11 files changed, 22 insertions(+), 35 deletions(-)
> 
> <...>
> 
> > @@ -35,6 +35,7 @@
> >  #ifndef _E1000_OSDEP_H_
> >  #define _E1000_OSDEP_H_
> >  
> > +#include <stdbool.h>
> >  #include <stdint.h>
> >  #include <stdio.h>
> >  #include <stdarg.h>
> > @@ -87,7 +88,6 @@ typedef int64_t		s64;
> >  typedef int32_t		s32;
> >  typedef int16_t		s16;
> >  typedef int8_t		s8;
> > -typedef int		bool;
> >  
> >  #define __le16		u16
> >  #define __le32		u32
> > @@ -192,7 +192,4 @@ static inline uint16_t e1000_read_addr16(volatile void *addr)
> >  #define ETH_ADDR_LEN                  6
> >  #endif
> >  
> > -#define false                         FALSE
> > -#define true                          TRUE
> > -
> 
> It is too much hassle to update Intel base driver code.

It is not really base driver code.
It was agreed that *_osdep.h can be modified:
	http://git.dpdk.org/dpdk/tree/drivers/net/ixgbe/base/README#n56

> What would happen if not
> include stdbool and keep define for base code updates? Will it break build for
> applications?

The problem is not applications, but using stdbool in DPDK headers.

Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Ferruh Yigit]

On 9/21/2018 3:49 PM, Thomas Monjalon wrote:
> 21/09/2018 15:47, Ferruh Yigit:
>> On 9/20/2018 1:18 AM, Thomas Monjalon wrote:
>>> When trying to include stdbool.h in DPDK base headers, there are a lot
>>> of conflicts with drivers which redefine bool/true/false
>>> in their compatibility layer.
>>>
>>> It is fixed by including stdbool.h in these drivers.
>>> Some errors with usage of bool type are also fixed in some drivers.
>>>
>>> Note: the driver qede has a surprising mix of bool and int:
>>> 	(~p_iov->b_pre_fp_hsi & ETH_HSI_VER_MINOR)
>>> where the first variable is boolean and the version is a number.
>>> It is replaced by
>>> 	!p_iov->b_pre_fp_hsi
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>> ---
>>>  drivers/net/cxgbe/cxgbe_compat.h         |  2 +-
>>>  drivers/net/e1000/base/e1000_osdep.h     |  5 +----
>>>  drivers/net/fm10k/base/fm10k_osdep.h     |  8 +-------
>>>  drivers/net/fm10k/fm10k_ethdev.c         |  4 ++--
>>>  drivers/net/ixgbe/base/ixgbe_osdep.h     |  6 +-----
>>>  drivers/net/ixgbe/ixgbe_ethdev.c         | 16 +++++++++-------
>>>  drivers/net/ixgbe/ixgbe_rxtx.c           |  2 +-
>>>  drivers/net/qede/base/bcm_osal.h         |  6 ++----
>>>  drivers/net/qede/base/ecore_vf.c         |  3 +--
>>>  drivers/net/qede/qede_ethdev.c           |  2 +-
>>>  drivers/net/vmxnet3/base/vmxnet3_osdep.h |  3 ++-
>>>  11 files changed, 22 insertions(+), 35 deletions(-)
>>
>> <...>
>>
>>> @@ -35,6 +35,7 @@
>>>  #ifndef _E1000_OSDEP_H_
>>>  #define _E1000_OSDEP_H_
>>>  
>>> +#include <stdbool.h>
>>>  #include <stdint.h>
>>>  #include <stdio.h>
>>>  #include <stdarg.h>
>>> @@ -87,7 +88,6 @@ typedef int64_t		s64;
>>>  typedef int32_t		s32;
>>>  typedef int16_t		s16;
>>>  typedef int8_t		s8;
>>> -typedef int		bool;
>>>  
>>>  #define __le16		u16
>>>  #define __le32		u32
>>> @@ -192,7 +192,4 @@ static inline uint16_t e1000_read_addr16(volatile void *addr)
>>>  #define ETH_ADDR_LEN                  6
>>>  #endif
>>>  
>>> -#define false                         FALSE
>>> -#define true                          TRUE
>>> -
>>
>> It is too much hassle to update Intel base driver code.
> 
> It is not really base driver code.
> It was agreed that *_osdep.h can be modified:
> 	http://git.dpdk.org/dpdk/tree/drivers/net/ixgbe/base/README#n56

Right.

> 
>> What would happen if not
>> include stdbool and keep define for base code updates? Will it break build for
>> applications?
> 
> The problem is not applications, but using stdbool in DPDK headers.

I see.

Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Ferruh Yigit]

On 9/20/2018 1:18 AM, Thomas Monjalon wrote:
> When trying to include stdbool.h in DPDK base headers, there are a lot
> of conflicts with drivers which redefine bool/true/false
> in their compatibility layer.
> 
> It is fixed by including stdbool.h in these drivers.
> Some errors with usage of bool type are also fixed in some drivers.
> 
> Note: the driver qede has a surprising mix of bool and int:
> 	(~p_iov->b_pre_fp_hsi & ETH_HSI_VER_MINOR)
> where the first variable is boolean and the version is a number.
> It is replaced by
> 	!p_iov->b_pre_fp_hsi
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

<...>

> diff --git a/drivers/net/e1000/base/e1000_osdep.h b/drivers/net/e1000/base/e1000_osdep.h
> index b8868049f..556ed1742 100644
> --- a/drivers/net/e1000/base/e1000_osdep.h
> +++ b/drivers/net/e1000/base/e1000_osdep.h
> @@ -35,6 +35,7 @@
>  #ifndef _E1000_OSDEP_H_
>  #define _E1000_OSDEP_H_
>  
> +#include <stdbool.h>
>  #include <stdint.h>
>  #include <stdio.h>
>  #include <stdarg.h>
> @@ -87,7 +88,6 @@ typedef int64_t		s64;
>  typedef int32_t		s32;
>  typedef int16_t		s16;
>  typedef int8_t		s8;
> -typedef int		bool;
>  
>  #define __le16		u16
>  #define __le32		u32
> @@ -192,7 +192,4 @@ static inline uint16_t e1000_read_addr16(volatile void *addr)
>  #define ETH_ADDR_LEN                  6
>  #endif
>  
> -#define false                         FALSE
> -#define true                          TRUE

TRUE and FALSE also defined in this patch, can we remove them too?

> -
>  #endif /* _E1000_OSDEP_H_ */
> diff --git a/drivers/net/fm10k/base/fm10k_osdep.h b/drivers/net/fm10k/base/fm10k_osdep.h
> index 199ebd8ea..9665239fd 100644
> --- a/drivers/net/fm10k/base/fm10k_osdep.h
> +++ b/drivers/net/fm10k/base/fm10k_osdep.h
> @@ -34,6 +34,7 @@ POSSIBILITY OF SUCH DAMAGE.
>  #ifndef _FM10K_OSDEP_H_
>  #define _FM10K_OSDEP_H_
>  
> +#include <stdbool.h>
>  #include <stdint.h>
>  #include <string.h>
>  #include <rte_atomic.h>
> @@ -61,12 +62,6 @@ POSSIBILITY OF SUCH DAMAGE.
>  
>  #define FALSE      0
>  #define TRUE       1
> -#ifndef false
> -#define false      FALSE
> -#endif
> -#ifndef true
> -#define true       TRUE
> -#endif

Same here, TRUE and FALSE defined in this header and used in .c files one or two
places, what about remove them and convert usage to "true" and "false"

<...>

> diff --git a/drivers/net/ixgbe/base/ixgbe_osdep.h b/drivers/net/ixgbe/base/ixgbe_osdep.h
> index bb5dfd2af..39e9118aa 100644
> --- a/drivers/net/ixgbe/base/ixgbe_osdep.h
> +++ b/drivers/net/ixgbe/base/ixgbe_osdep.h
> @@ -36,6 +36,7 @@
>  #define _IXGBE_OS_H_
>  
>  #include <string.h>
> +#include <stdbool.h>
>  #include <stdint.h>
>  #include <stdio.h>
>  #include <stdarg.h>
> @@ -70,8 +71,6 @@
>  #define FALSE               0
>  #define TRUE                1

Same again, can we remove TRUE and FALSE

<...>

> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index cee886754..c272a4112 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2527,7 +2527,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
>  	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
>  	struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
>  	uint32_t intr_vector = 0;
> -	int err, link_up = 0, negotiate = 0;
> +	int err;
> +	bool negotiate = false;
> +	bool link_up = false;

"link_up" is used in assignment to a single bit in uint16_t:
dev->data->dev_link.link_status = link_up;

When "link_up" is bool, should we change that line to:
if (link_up)
	dev->data->dev_link.link_status = 1;
else
	dev->data->dev_link.link_status = 0;

<...>

> @@ -3870,7 +3872,7 @@ ixgbevf_dev_info_get(struct rte_eth_dev *dev,
>  
>  static int
>  ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
> -		   int *link_up, int wait_to_complete)
> +		   bool *link_up, int wait_to_complete)

Also need to change "wait_to_complete" to bool because below changes start
sending bool type to this function.

<...>

> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index ae21f04a1..2dc14c47f 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -2025,7 +2025,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
>  		struct ixgbe_rx_entry *next_rxe = NULL;
>  		struct rte_mbuf *first_seg;
>  		struct rte_mbuf *rxm;
> -		struct rte_mbuf *nmb;
> +		struct rte_mbuf *nmb = NULL;

This change is unrelated. Can we separate this one?

Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Thomas Monjalon]

24/09/2018 17:06, Ferruh Yigit:
> On 9/20/2018 1:18 AM, Thomas Monjalon wrote:
> > -#define false                         FALSE
> > -#define true                          TRUE
> 
> TRUE and FALSE also defined in this patch, can we remove them too?

I don't see the need to remove TRUE and FALSE.
The base drivers use them on other platforms, and it is convenient to not
change the base drivers.

[...]
> >  static int
> >  ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
> > -		   int *link_up, int wait_to_complete)
> > +		   bool *link_up, int wait_to_complete)
> 
> Also need to change "wait_to_complete" to bool because below changes start
> sending bool type to this function.

[...]
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > @@ -2025,7 +2025,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
> >  		struct ixgbe_rx_entry *next_rxe = NULL;
> >  		struct rte_mbuf *first_seg;
> >  		struct rte_mbuf *rxm;
> > -		struct rte_mbuf *nmb;
> > +		struct rte_mbuf *nmb = NULL;
> 
> This change is unrelated. Can we separate this one?

Yes it looks unrelated but it becomes necessary when including stdbool.h.
I don't know the root cause, but yes, it may deserve a separate commit.
Maybe an ixgbe maintainer can take care of it?

Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Ferruh Yigit]

On 9/24/2018 5:59 PM, Thomas Monjalon wrote:
> 24/09/2018 17:06, Ferruh Yigit:
>> On 9/20/2018 1:18 AM, Thomas Monjalon wrote:
>>> -#define false                         FALSE
>>> -#define true                          TRUE
>>
>> TRUE and FALSE also defined in this patch, can we remove them too?
> 
> I don't see the need to remove TRUE and FALSE.
> The base drivers use them on other platforms, and it is convenient to not
> change the base drivers.

Not needed, but previously it was only TRUE & FALSE, and true & false was define
to them.

Now there are TRUE & FALSE from header files and true & false from stdbool and
these pairs used interchangeably, I thought it can better to unify the usage to
stdbool ones.

> 
> [...]
>>>  static int
>>>  ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
>>> -		   int *link_up, int wait_to_complete)
>>> +		   bool *link_up, int wait_to_complete)
>>
>> Also need to change "wait_to_complete" to bool because below changes start
>> sending bool type to this function.
> 
> [...]
>>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
>>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
>>> @@ -2025,7 +2025,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
>>>  		struct ixgbe_rx_entry *next_rxe = NULL;
>>>  		struct rte_mbuf *first_seg;
>>>  		struct rte_mbuf *rxm;
>>> -		struct rte_mbuf *nmb;
>>> +		struct rte_mbuf *nmb = NULL;
>>
>> This change is unrelated. Can we separate this one?
> 
> Yes it looks unrelated but it becomes necessary when including stdbool.h.
> I don't know the root cause, but yes, it may deserve a separate commit.
> Maybe an ixgbe maintainer can take care of it?

Why becomes necessary? Does it give a build warning etc?
My concern is this is in data path, one extra assignment, it would be better to
confirm it is really needed.

Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Thomas Monjalon]

25/09/2018 10:03, Ferruh Yigit:
> On 9/24/2018 5:59 PM, Thomas Monjalon wrote:
> >>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> >>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> >>> @@ -2025,7 +2025,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
> >>>  		struct ixgbe_rx_entry *next_rxe = NULL;
> >>>  		struct rte_mbuf *first_seg;
> >>>  		struct rte_mbuf *rxm;
> >>> -		struct rte_mbuf *nmb;
> >>> +		struct rte_mbuf *nmb = NULL;
> >>
> >> This change is unrelated. Can we separate this one?
> > 
> > Yes it looks unrelated but it becomes necessary when including stdbool.h.
> > I don't know the root cause, but yes, it may deserve a separate commit.
> > Maybe an ixgbe maintainer can take care of it?
> 
> Why becomes necessary? Does it give a build warning etc?
> My concern is this is in data path, one extra assignment, it would be better to
> confirm it is really needed.

Yes I had a compilation error.
If you cannot reproduce it, I will try to re-run my compilation tests.

Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Ferruh Yigit]

On 9/25/2018 10:04 AM, Thomas Monjalon wrote:
> 25/09/2018 10:03, Ferruh Yigit:
>> On 9/24/2018 5:59 PM, Thomas Monjalon wrote:
>>>>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
>>>>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
>>>>> @@ -2025,7 +2025,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
>>>>>  		struct ixgbe_rx_entry *next_rxe = NULL;
>>>>>  		struct rte_mbuf *first_seg;
>>>>>  		struct rte_mbuf *rxm;
>>>>> -		struct rte_mbuf *nmb;
>>>>> +		struct rte_mbuf *nmb = NULL;
>>>>
>>>> This change is unrelated. Can we separate this one?
>>>
>>> Yes it looks unrelated but it becomes necessary when including stdbool.h.
>>> I don't know the root cause, but yes, it may deserve a separate commit.
>>> Maybe an ixgbe maintainer can take care of it?
>>
>> Why becomes necessary? Does it give a build warning etc?
>> My concern is this is in data path, one extra assignment, it would be better to
>> confirm it is really needed.
> 
> Yes I had a compilation error.
> If you cannot reproduce it, I will try to re-run my compilation tests.

I got the error with gcc [1] but it seems false positive and only generated when
<stdbool.h> included in ixgbe_rxtx.c, so this is an odd one, I am not able to
find root cause.

But since it is false positive, what do you think adding compiler option to
disable this warning for this file?


[1]
.../drivers/net/ixgbe/ixgbe_rxtx.c:2139:14: error: ‘nmb’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
    rxe->mbuf = nmb;
    ~~~~~~~~~~^~~~~

$ gcc --version
gcc (GCC) 8.1.1 20180712 (Red Hat 8.1.1-5)

Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool

[Thomas Monjalon]

03/10/2018 16:11, Ferruh Yigit:
> On 9/25/2018 10:04 AM, Thomas Monjalon wrote:
> > 25/09/2018 10:03, Ferruh Yigit:
> >> On 9/24/2018 5:59 PM, Thomas Monjalon wrote:
> >>>>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> >>>>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> >>>>> @@ -2025,7 +2025,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
> >>>>>  		struct ixgbe_rx_entry *next_rxe = NULL;
> >>>>>  		struct rte_mbuf *first_seg;
> >>>>>  		struct rte_mbuf *rxm;
> >>>>> -		struct rte_mbuf *nmb;
> >>>>> +		struct rte_mbuf *nmb = NULL;
> >>>>
> >>>> This change is unrelated. Can we separate this one?
> >>>
> >>> Yes it looks unrelated but it becomes necessary when including stdbool.h.
> >>> I don't know the root cause, but yes, it may deserve a separate commit.
> >>> Maybe an ixgbe maintainer can take care of it?
> >>
> >> Why becomes necessary? Does it give a build warning etc?
> >> My concern is this is in data path, one extra assignment, it would be better to
> >> confirm it is really needed.
> > 
> > Yes I had a compilation error.
> > If you cannot reproduce it, I will try to re-run my compilation tests.
> 
> I got the error with gcc [1] but it seems false positive and only generated when
> <stdbool.h> included in ixgbe_rxtx.c, so this is an odd one, I am not able to
> find root cause.
> 
> But since it is false positive, what do you think adding compiler option to
> disable this warning for this file?

I don't like disabling warnings on files.
We can take time to work on this patch. It is not required for 18.11.


> [1]
> .../drivers/net/ixgbe/ixgbe_rxtx.c:2139:14: error: ‘nmb’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>     rxe->mbuf = nmb;
>     ~~~~~~~~~~^~~~~
> 
> $ gcc --version
> gcc (GCC) 8.1.1 20180712 (Red Hat 8.1.1-5)