[v5,3/6] net/axgbe: use common rte bit operation APIs instead
Checks
Commit Message
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(-)
Comments
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
02/12/2019 07:09, Gavin Hu (Arm Technology China):
> 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.
Please could you be more specific? What is the exact error?
> 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?
Which unsigned long?
If it is in the (not merged) bit API, it can still be changed no?
Thomas,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, December 2, 2019 10:12 AM
>
> 02/12/2019 07:09, Gavin Hu (Arm Technology China):
> > 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.
>
> Please could you be more specific? What is the exact error?
The PMD has a private structure with an unsigned long field.
The patch for the PMD uses the 64 bit operations on this field. The patch fails to compile for a 32 bit target, because the struct field is only 32 bit there.
>
> > 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?
>
> Which unsigned long?
> If it is in the (not merged) bit API, it can still be changed no?
>
The patch for the PMD can be changed to use the 64 or 32 bit operations depending on whether it is being compiled for a 64 or 32 bit target.
However, the question seems to be if we want to either 1) do something like that, or 2) drop support for 32 bit targets, or 3) make these target dependent fields obsolete (i.e. ban the use of unsigned long) and require explicit sizes, e.g. uint32_t.
02/12/2019 10:24, Morten Brørup:
> Thomas,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Monday, December 2, 2019 10:12 AM
> >
> > 02/12/2019 07:09, Gavin Hu (Arm Technology China):
> > > 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.
> >
> > Please could you be more specific? What is the exact error?
>
> The PMD has a private structure with an unsigned long field.
>
> The patch for the PMD uses the 64 bit operations on this field. The patch fails to compile for a 32 bit target, because the struct field is only 32 bit there.
>
> >
> > > 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?
> >
> > Which unsigned long?
> > If it is in the (not merged) bit API, it can still be changed no?
> >
>
> The patch for the PMD can be changed to use the 64 or 32 bit operations depending on whether it is being compiled for a 64 or 32 bit target.
>
> However, the question seems to be if we want to either 1) do something like that, or 2) drop support for 32 bit targets, or 3) make these target dependent fields obsolete (i.e. ban the use of unsigned long) and require explicit sizes, e.g. uint32_t.
We should support both,
and use the appropriate instruction.
But I wonder why this field has not a fixed size.
It would be probably better to change the field to uint32_t or uint64_t.
On Mon, 2 Dec 2019 10:24:32 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:
> Thomas,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Monday, December 2, 2019 10:12 AM
> >
> > 02/12/2019 07:09, Gavin Hu (Arm Technology China):
> > > 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.
> >
> > Please could you be more specific? What is the exact error?
>
> The PMD has a private structure with an unsigned long field.
>
> The patch for the PMD uses the 64 bit operations on this field. The patch fails to compile for a 32 bit target, because the struct field is only 32 bit there.
>
> >
> > > 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?
> >
> > Which unsigned long?
> > If it is in the (not merged) bit API, it can still be changed no?
> >
>
> The patch for the PMD can be changed to use the 64 or 32 bit operations depending on whether it is being compiled for a 64 or 32 bit target.
>
> However, the question seems to be if we want to either 1) do something like that, or 2) drop support for 32 bit targets, or 3) make these target dependent fields obsolete (i.e. ban the use of unsigned long) and require explicit sizes, e.g. uint32_t.
The bitop library should not assume sizeof(unsigned long) == 32 bit.
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, December 3, 2019 12:53 AM
> To: Morten Brørup <mb@smartsharesystems.com>
> Cc: thomas@monjalon.net; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>; Joyce Kong (Arm Technology China)
> <Joyce.Kong@arm.com>; dev@dpdk.org; jerinj@marvell.com; Bruce
> Richardson <bruce.richardson@intel.com>; nd <nd@arm.com>;
> david.marchand@redhat.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; ravi1.kumar@amd.com;
> rmody@marvell.com; shshaikh@marvell.com; xuanziyang2@huawei.com;
> cloud.wangxiaoyun@huawei.com; zhouguoyang@huawei.com
> Subject: Re: [dpdk-dev] [PATCH v5 3/6] net/axgbe: use common rte
> bitoperation APIs instead
>
> On Mon, 2 Dec 2019 10:24:32 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > Thomas,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> Monjalon
> > > Sent: Monday, December 2, 2019 10:12 AM
> > >
> > > 02/12/2019 07:09, Gavin Hu (Arm Technology China):
> > > > 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.
> > >
> > > Please could you be more specific? What is the exact error?
> >
> > The PMD has a private structure with an unsigned long field.
> >
> > The patch for the PMD uses the 64 bit operations on this field. The patch
> fails to compile for a 32 bit target, because the struct field is only 32 bit there.
> >
> > >
> > > > 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?
> > >
> > > Which unsigned long?
> > > If it is in the (not merged) bit API, it can still be changed no?
> > >
> >
> > The patch for the PMD can be changed to use the 64 or 32 bit operations
> depending on whether it is being compiled for a 64 or 32 bit target.
> >
> > However, the question seems to be if we want to either 1) do something like
> that, or 2) drop support for 32 bit targets, or 3) make these target dependent
> fields obsolete (i.e. ban the use of unsigned long) and require explicit sizes, e.g.
> uint32_t.
>
> The bitop library should not assume sizeof(unsigned long) == 32 bit.
As discussed, both 32-bit and 64-bit OSes should be supported, and their sizes of "unsigned long" are not fixed.
Taking all these into considerations, we will use "unsigned int" or uint32_t instead of "unsigned long" in the PMDs to be compatible across 32- or 64-bit OSes.
/Gavin
@@ -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);
@@ -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);
@@ -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)