Re: [dpdk-dev] [PATCH v5 3/6] net/axgbe: use common rte bit operation APIs instead

["Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com>]

Hi Bruce, Thomas,

This series of patches was reported a compilation issue[1] on 32bit Ubuntu. 
On mainstream 64-bit OS,  "unsigned long" is 64-bit in size and we uses the 64-bit variant of APIs. But the 32-bit OS expect 32-bit 'unsigned long' arguments. 
This is where the error happens. 

My question is how 32-bit OSes shall we support, put another way, can we ignore this compilation issue? 
If we still need to care, how about making 'obsolete' of 'unsigned long' and use 'uint32' instead to be multi-OS friendly? 

*Meson Build Failed #1:
OS: UB1604-32
Target:build-gcc-static
[1] http://mails.dpdk.org/archives/test-report/2019-November/109515.html 

> -----Original Message-----
> From: Joyce Kong <joyce.kong@arm.com>
> Sent: Thursday, November 28, 2019 2:44 PM
> To: dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> stephen@networkplumber.org; mb@smartsharesystems.com;
> david.marchand@redhat.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>; ravi1.kumar@amd.com; rmody@marvell.com;
> shshaikh@marvell.com; xuanziyang2@huawei.com;
> cloud.wangxiaoyun@huawei.com; zhouguoyang@huawei.com
> Subject: [PATCH v5 3/6] net/axgbe: use common rte bit operation APIs
> instead
> 
> Remove its own bit operation APIs and use the common one,
> this can reduce the code duplication largely.
> 
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
>  drivers/net/axgbe/axgbe_common.h | 29 +----------------------------
>  drivers/net/axgbe/axgbe_ethdev.c | 14 +++++++-------
>  drivers/net/axgbe/axgbe_mdio.c   | 14 +++++++-------
>  3 files changed, 15 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/axgbe/axgbe_common.h
> b/drivers/net/axgbe/axgbe_common.h
> index 34f60f1..9cabda8 100644
> --- a/drivers/net/axgbe/axgbe_common.h
> +++ b/drivers/net/axgbe/axgbe_common.h
> @@ -22,6 +22,7 @@
>  #include <pthread.h>
> 
>  #include <rte_byteorder.h>
> +#include <rte_bitops.h>
>  #include <rte_memory.h>
>  #include <rte_malloc.h>
>  #include <rte_hexdump.h>
> @@ -1674,34 +1675,6 @@ do {
> 			\
>  #define time_after_eq(a, b)     ((long)((a) - (b)) >= 0)
>  #define time_before_eq(a, b)	time_after_eq(b, a)
> 
> -/*---bitmap support apis---*/
> -static inline int axgbe_test_bit(int nr, volatile unsigned long *addr)
> -{
> -	int res;
> -
> -	rte_mb();
> -	res = ((*addr) & (1UL << nr)) != 0;
> -	rte_mb();
> -	return res;
> -}
> -
> -static inline void axgbe_set_bit(unsigned int nr, volatile unsigned long *addr)
> -{
> -	__sync_fetch_and_or(addr, (1UL << nr));
> -}
> -
> -static inline void axgbe_clear_bit(int nr, volatile unsigned long *addr)
> -{
> -	__sync_fetch_and_and(addr, ~(1UL << nr));
> -}
> -
> -static inline int axgbe_test_and_clear_bit(int nr, volatile unsigned long
> *addr)
> -{
> -	unsigned long mask = (1UL << nr);
> -
> -	return __sync_fetch_and_and(addr, ~mask) & mask;
> -}
> -
>  static inline unsigned long msecs_to_timer_cycles(unsigned int m)
>  {
>  	return rte_get_timer_hz() * (m / 1000);
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c
> b/drivers/net/axgbe/axgbe_ethdev.c
> index d1f160e..fa597f3 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -201,8 +201,8 @@ axgbe_dev_start(struct rte_eth_dev *dev)
>  	axgbe_dev_enable_tx(dev);
>  	axgbe_dev_enable_rx(dev);
> 
> -	axgbe_clear_bit(AXGBE_STOPPED, &pdata->dev_state);
> -	axgbe_clear_bit(AXGBE_DOWN, &pdata->dev_state);
> +	rte_clear_bit64(AXGBE_STOPPED, &pdata->dev_state);
> +	rte_clear_bit64(AXGBE_DOWN, &pdata->dev_state);
>  	return 0;
>  }
> 
> @@ -216,17 +216,17 @@ axgbe_dev_stop(struct rte_eth_dev *dev)
> 
>  	rte_intr_disable(&pdata->pci_dev->intr_handle);
> 
> -	if (axgbe_test_bit(AXGBE_STOPPED, &pdata->dev_state))
> +	if (rte_get_bit64(AXGBE_STOPPED, &pdata->dev_state))
>  		return;
> 
> -	axgbe_set_bit(AXGBE_STOPPED, &pdata->dev_state);
> +	rte_set_bit64(AXGBE_STOPPED, &pdata->dev_state);
>  	axgbe_dev_disable_tx(dev);
>  	axgbe_dev_disable_rx(dev);
> 
>  	pdata->phy_if.phy_stop(pdata);
>  	pdata->hw_if.exit(pdata);
>  	memset(&dev->data->dev_link, 0, sizeof(struct rte_eth_link));
> -	axgbe_set_bit(AXGBE_DOWN, &pdata->dev_state);
> +	rte_set_bit64(AXGBE_DOWN, &pdata->dev_state);
>  }
> 
>  /* Clear all resources like TX/RX queues. */
> @@ -598,8 +598,8 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
> 
>  	pdata = eth_dev->data->dev_private;
>  	/* initial state */
> -	axgbe_set_bit(AXGBE_DOWN, &pdata->dev_state);
> -	axgbe_set_bit(AXGBE_STOPPED, &pdata->dev_state);
> +	rte_set_bit64(AXGBE_DOWN, &pdata->dev_state);
> +	rte_set_bit64(AXGBE_STOPPED, &pdata->dev_state);
>  	pdata->eth_dev = eth_dev;
> 
>  	pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
> diff --git a/drivers/net/axgbe/axgbe_mdio.c
> b/drivers/net/axgbe/axgbe_mdio.c
> index 2721e5c..00394a7 100644
> --- a/drivers/net/axgbe/axgbe_mdio.c
> +++ b/drivers/net/axgbe/axgbe_mdio.c
> @@ -743,7 +743,7 @@ static int __axgbe_phy_config_aneg(struct
> axgbe_port *pdata)
>  {
>  	int ret;
> 
> -	axgbe_set_bit(AXGBE_LINK_INIT, &pdata->dev_state);
> +	rte_set_bit64(AXGBE_LINK_INIT, &pdata->dev_state);
>  	pdata->link_check = rte_get_timer_cycles();
> 
>  	ret = pdata->phy_if.phy_impl.an_config(pdata);
> @@ -807,9 +807,9 @@ static int axgbe_phy_config_aneg(struct axgbe_port
> *pdata)
> 
>  	ret = __axgbe_phy_config_aneg(pdata);
>  	if (ret)
> -		axgbe_set_bit(AXGBE_LINK_ERR, &pdata->dev_state);
> +		rte_set_bit64(AXGBE_LINK_ERR, &pdata->dev_state);
>  	else
> -		axgbe_clear_bit(AXGBE_LINK_ERR, &pdata->dev_state);
> +		rte_clear_bit64(AXGBE_LINK_ERR, &pdata->dev_state);
> 
>  	pthread_mutex_unlock(&pdata->an_mutex);
> 
> @@ -880,7 +880,7 @@ static void axgbe_phy_status(struct axgbe_port
> *pdata)
>  	unsigned int link_aneg;
>  	int an_restart;
> 
> -	if (axgbe_test_bit(AXGBE_LINK_ERR, &pdata->dev_state)) {
> +	if (rte_get_bit64(AXGBE_LINK_ERR, &pdata->dev_state)) {
>  		pdata->phy.link = 0;
>  		goto adjust_link;
>  	}
> @@ -900,10 +900,10 @@ static void axgbe_phy_status(struct axgbe_port
> *pdata)
>  			return;
>  		}
>  		axgbe_phy_status_result(pdata);
> -		if (axgbe_test_bit(AXGBE_LINK_INIT, &pdata->dev_state))
> -			axgbe_clear_bit(AXGBE_LINK_INIT, &pdata-
> >dev_state);
> +		if (rte_get_bit64(AXGBE_LINK_INIT, &pdata->dev_state))
> +			rte_clear_bit64(AXGBE_LINK_INIT, &pdata-
> >dev_state);
>  	} else {
> -		if (axgbe_test_bit(AXGBE_LINK_INIT, &pdata->dev_state)) {
> +		if (rte_get_bit64(AXGBE_LINK_INIT, &pdata->dev_state)) {
>  			axgbe_check_link_timeout(pdata);
> 
>  			if (link_aneg)
> --
> 2.7.4