drivers/net: do not redefine bool

Message ID 20180920001853.23454-1-thomas@monjalon.net
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • drivers/net: do not redefine bool
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

Thomas Monjalon Sept. 20, 2018, 12:18 a.m.
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(-)

Comments

Shaikh, Shahed Sept. 20, 2018, 5:48 p.m. | #1
> -----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>
Ferruh Yigit Sept. 21, 2018, 1:47 p.m. | #2
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?
Thomas Monjalon Sept. 21, 2018, 2:49 p.m. | #3
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.
Ferruh Yigit Sept. 24, 2018, 2:43 p.m. | #4
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.
Ferruh Yigit Sept. 24, 2018, 3:06 p.m. | #5
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?
Thomas Monjalon Sept. 24, 2018, 4:59 p.m. | #6
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?
Ferruh Yigit Sept. 25, 2018, 8:03 a.m. | #7
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.
Thomas Monjalon Sept. 25, 2018, 9:04 a.m. | #8
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.
Ferruh Yigit Oct. 3, 2018, 2:11 p.m. | #9
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)
Thomas Monjalon Oct. 3, 2018, 7:16 p.m. | #10
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)

Patch

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