[dpdk-dev,v5,01/21] lib/librte_ethdev: change eth-dev-ops API to return int

Message ID 152656493702.46638.10712692446180001555.stgit@localhost.localdomain (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

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

Commit Message

Andy Green May 17, 2018, 1:48 p.m. UTC
  Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/ark/ark_ethdev_rx.c     |    4 ++--
 drivers/net/ark/ark_ethdev_rx.h     |    3 +--
 drivers/net/avf/avf_rxtx.c          |    4 ++--
 drivers/net/avf/avf_rxtx.h          |    2 +-
 drivers/net/bnxt/bnxt_ethdev.c      |    5 +++--
 drivers/net/dpaa/dpaa_ethdev.c      |    4 ++--
 drivers/net/dpaa2/dpaa2_ethdev.c    |    6 +++---
 drivers/net/e1000/e1000_ethdev.h    |    6 ++----
 drivers/net/e1000/em_rxtx.c         |    4 ++--
 drivers/net/e1000/igb_rxtx.c        |    4 ++--
 drivers/net/enic/enic_ethdev.c      |    9 +++------
 drivers/net/i40e/i40e_rxtx.c        |    4 ++--
 drivers/net/i40e/i40e_rxtx.h        |    3 +--
 drivers/net/ixgbe/ixgbe_ethdev.h    |    3 +--
 drivers/net/ixgbe/ixgbe_rxtx.c      |    4 ++--
 drivers/net/nfp/nfp_net.c           |    9 ++++-----
 drivers/net/sfc/sfc_ethdev.c        |    4 ++--
 drivers/net/thunderx/nicvf_ethdev.c |    2 +-
 drivers/net/thunderx/nicvf_rxtx.c   |    4 ++--
 drivers/net/thunderx/nicvf_rxtx.h   |    2 +-
 drivers/net/vhost/rte_eth_vhost.c   |    4 ++--
 examples/l3fwd-power/main.c         |    2 +-
 lib/librte_ethdev/rte_ethdev_core.h |    4 ++--
 23 files changed, 44 insertions(+), 52 deletions(-)
  

Comments

Bruce Richardson May 17, 2018, 1:55 p.m. UTC | #1
On Thu, May 17, 2018 at 09:48:57PM +0800, Andy Green wrote:
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---

What's the reason for this change of type?
  
Andy Green May 17, 2018, 1:59 p.m. UTC | #2
On 05/17/2018 09:55 PM, Bruce Richardson wrote:
> On Thu, May 17, 2018 at 09:48:57PM +0800, Andy Green wrote:
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
> 
> What's the reason for this change of type?

I was asked to do it on the list... the original patch I sent was 
triggered by gcc8 warnings and just casted it away.  But the comment was 
that as an int, it could return negative error codes, so better to 
change them all.

It thereby became something in the way of a refactor rather than a gcc 8 
fix any more.

-Andy
  
Shreyansh Jain May 18, 2018, 10:59 a.m. UTC | #3
> -----Original Message-----

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green

> Sent: Thursday, May 17, 2018 7:19 PM

> To: dev@dpdk.org

> Subject: [dpdk-dev] [PATCH v5 01/21] lib/librte_ethdev: change eth-dev-

> ops API to return int

> 

> Signed-off-by: Andy Green <andy@warmcat.com>

> ---

>  drivers/net/ark/ark_ethdev_rx.c     |    4 ++--

>  drivers/net/ark/ark_ethdev_rx.h     |    3 +--

>  drivers/net/avf/avf_rxtx.c          |    4 ++--

>  drivers/net/avf/avf_rxtx.h          |    2 +-

>  drivers/net/bnxt/bnxt_ethdev.c      |    5 +++--

>  drivers/net/dpaa/dpaa_ethdev.c      |    4 ++--

>  drivers/net/dpaa2/dpaa2_ethdev.c    |    6 +++---

>  drivers/net/e1000/e1000_ethdev.h    |    6 ++----

>  drivers/net/e1000/em_rxtx.c         |    4 ++--

>  drivers/net/e1000/igb_rxtx.c        |    4 ++--

>  drivers/net/enic/enic_ethdev.c      |    9 +++------

>  drivers/net/i40e/i40e_rxtx.c        |    4 ++--

>  drivers/net/i40e/i40e_rxtx.h        |    3 +--

>  drivers/net/ixgbe/ixgbe_ethdev.h    |    3 +--

>  drivers/net/ixgbe/ixgbe_rxtx.c      |    4 ++--

>  drivers/net/nfp/nfp_net.c           |    9 ++++-----

>  drivers/net/sfc/sfc_ethdev.c        |    4 ++--

>  drivers/net/thunderx/nicvf_ethdev.c |    2 +-

>  drivers/net/thunderx/nicvf_rxtx.c   |    4 ++--

>  drivers/net/thunderx/nicvf_rxtx.h   |    2 +-

>  drivers/net/vhost/rte_eth_vhost.c   |    4 ++--

>  examples/l3fwd-power/main.c         |    2 +-

>  lib/librte_ethdev/rte_ethdev_core.h |    4 ++--

>  23 files changed, 44 insertions(+), 52 deletions(-)

> 


[...]

>  	rxq = dev->data->rx_queues[rx_queue_id];

> diff --git a/drivers/net/dpaa/dpaa_ethdev.c

> b/drivers/net/dpaa/dpaa_ethdev.c

> index d014a11aa..70a5b4851 100644

> --- a/drivers/net/dpaa/dpaa_ethdev.c

> +++ b/drivers/net/dpaa/dpaa_ethdev.c

> @@ -725,7 +725,7 @@ static void dpaa_eth_tx_queue_release(void *txq

> __rte_unused)

>  	PMD_INIT_FUNC_TRACE();

>  }

> 

> -static uint32_t

> +static int

>  dpaa_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)

>  {

>  	struct dpaa_if *dpaa_intf = dev->data->dev_private;

> @@ -738,7 +738,7 @@ dpaa_dev_rx_queue_count(struct rte_eth_dev *dev,

> uint16_t rx_queue_id)

>  		RTE_LOG(DEBUG, PMD, "RX frame count for q(%d) is %u\n",

>  			rx_queue_id, frm_cnt);

>  	}

> -	return frm_cnt;

> +	return (int)frm_cnt;

>  }

> 

>  static int dpaa_link_down(struct rte_eth_dev *dev)

> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c

> b/drivers/net/dpaa2/dpaa2_ethdev.c

> index 9297725d9..eb6245b83 100644

> --- a/drivers/net/dpaa2/dpaa2_ethdev.c

> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c

> @@ -604,7 +604,7 @@ dpaa2_dev_tx_queue_release(void *q __rte_unused)

>  	PMD_INIT_FUNC_TRACE();

>  }

> 

> -static uint32_t

> +static int

>  dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t

> rx_queue_id)

>  {

>  	int32_t ret;

> @@ -612,7 +612,7 @@ dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev,

> uint16_t rx_queue_id)

>  	struct dpaa2_queue *dpaa2_q;

>  	struct qbman_swp *swp;

>  	struct qbman_fq_query_np_rslt state;

> -	uint32_t frame_cnt = 0;

> +	int frame_cnt = 0;

> 

>  	PMD_INIT_FUNC_TRACE();

> 

> @@ -628,7 +628,7 @@ dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev,

> uint16_t rx_queue_id)

>  	dpaa2_q = (struct dpaa2_queue *)priv->rx_vq[rx_queue_id];

> 

>  	if (qbman_fq_query_state(swp, dpaa2_q->fqid, &state) == 0) {

> -		frame_cnt = qbman_fq_state_frame_count(&state);

> +		frame_cnt = (int)qbman_fq_state_frame_count(&state);

>  		DPAA2_PMD_DEBUG("RX frame count for q(%d) is %u",

>  				rx_queue_id, frame_cnt);

>  	}


This doesn't feel correct. A counter, especially the number of descriptors in a queue, doesn't have a negative value. So, 1) this is an unnatural return and 2) we litter the code with unnecessary typecast.

In fact, even in the above change, the debug messages continue to print unsigned values. So, another typecast would be required there.

I don't agree with this change - at least not until some strong gcc 8 warning reason is triggering this. Can you please point me to some conversation on mailing list which enforces this?
  
Andy Green May 18, 2018, 11:12 a.m. UTC | #4
On 05/18/2018 06:59 PM, Shreyansh Jain wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Thursday, May 17, 2018 7:19 PM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v5 01/21] lib/librte_ethdev: change eth-dev-
>> ops API to return int
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   drivers/net/ark/ark_ethdev_rx.c     |    4 ++--
>>   drivers/net/ark/ark_ethdev_rx.h     |    3 +--
>>   drivers/net/avf/avf_rxtx.c          |    4 ++--
>>   drivers/net/avf/avf_rxtx.h          |    2 +-
>>   drivers/net/bnxt/bnxt_ethdev.c      |    5 +++--
>>   drivers/net/dpaa/dpaa_ethdev.c      |    4 ++--
>>   drivers/net/dpaa2/dpaa2_ethdev.c    |    6 +++---
>>   drivers/net/e1000/e1000_ethdev.h    |    6 ++----
>>   drivers/net/e1000/em_rxtx.c         |    4 ++--
>>   drivers/net/e1000/igb_rxtx.c        |    4 ++--
>>   drivers/net/enic/enic_ethdev.c      |    9 +++------
>>   drivers/net/i40e/i40e_rxtx.c        |    4 ++--
>>   drivers/net/i40e/i40e_rxtx.h        |    3 +--
>>   drivers/net/ixgbe/ixgbe_ethdev.h    |    3 +--
>>   drivers/net/ixgbe/ixgbe_rxtx.c      |    4 ++--
>>   drivers/net/nfp/nfp_net.c           |    9 ++++-----
>>   drivers/net/sfc/sfc_ethdev.c        |    4 ++--
>>   drivers/net/thunderx/nicvf_ethdev.c |    2 +-
>>   drivers/net/thunderx/nicvf_rxtx.c   |    4 ++--
>>   drivers/net/thunderx/nicvf_rxtx.h   |    2 +-
>>   drivers/net/vhost/rte_eth_vhost.c   |    4 ++--
>>   examples/l3fwd-power/main.c         |    2 +-
>>   lib/librte_ethdev/rte_ethdev_core.h |    4 ++--
>>   23 files changed, 44 insertions(+), 52 deletions(-)
>>
> 
> [...]
> 
>>   	rxq = dev->data->rx_queues[rx_queue_id];
>> diff --git a/drivers/net/dpaa/dpaa_ethdev.c
>> b/drivers/net/dpaa/dpaa_ethdev.c
>> index d014a11aa..70a5b4851 100644
>> --- a/drivers/net/dpaa/dpaa_ethdev.c
>> +++ b/drivers/net/dpaa/dpaa_ethdev.c
>> @@ -725,7 +725,7 @@ static void dpaa_eth_tx_queue_release(void *txq
>> __rte_unused)
>>   	PMD_INIT_FUNC_TRACE();
>>   }
>>
>> -static uint32_t
>> +static int
>>   dpaa_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
>>   {
>>   	struct dpaa_if *dpaa_intf = dev->data->dev_private;
>> @@ -738,7 +738,7 @@ dpaa_dev_rx_queue_count(struct rte_eth_dev *dev,
>> uint16_t rx_queue_id)
>>   		RTE_LOG(DEBUG, PMD, "RX frame count for q(%d) is %u\n",
>>   			rx_queue_id, frm_cnt);
>>   	}
>> -	return frm_cnt;
>> +	return (int)frm_cnt;
>>   }
>>
>>   static int dpaa_link_down(struct rte_eth_dev *dev)
>> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c
>> b/drivers/net/dpaa2/dpaa2_ethdev.c
>> index 9297725d9..eb6245b83 100644
>> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
>> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
>> @@ -604,7 +604,7 @@ dpaa2_dev_tx_queue_release(void *q __rte_unused)
>>   	PMD_INIT_FUNC_TRACE();
>>   }
>>
>> -static uint32_t
>> +static int
>>   dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t
>> rx_queue_id)
>>   {
>>   	int32_t ret;
>> @@ -612,7 +612,7 @@ dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev,
>> uint16_t rx_queue_id)
>>   	struct dpaa2_queue *dpaa2_q;
>>   	struct qbman_swp *swp;
>>   	struct qbman_fq_query_np_rslt state;
>> -	uint32_t frame_cnt = 0;
>> +	int frame_cnt = 0;
>>
>>   	PMD_INIT_FUNC_TRACE();
>>
>> @@ -628,7 +628,7 @@ dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev,
>> uint16_t rx_queue_id)
>>   	dpaa2_q = (struct dpaa2_queue *)priv->rx_vq[rx_queue_id];
>>
>>   	if (qbman_fq_query_state(swp, dpaa2_q->fqid, &state) == 0) {
>> -		frame_cnt = qbman_fq_state_frame_count(&state);
>> +		frame_cnt = (int)qbman_fq_state_frame_count(&state);
>>   		DPAA2_PMD_DEBUG("RX frame count for q(%d) is %u",
>>   				rx_queue_id, frame_cnt);
>>   	}
> 
> This doesn't feel correct. A counter, especially the number of descriptors in a queue, doesn't have a negative value. So, 1) this is an unnatural return and 2) we litter the code with unnecessary typecast.
> 
> In fact, even in the above change, the debug messages continue to print unsigned values. So, another typecast would be required there.
> 
> I don't agree with this change - at least not until some strong gcc 8 warning reason is triggering this. Can you please point me to some conversation on mailing list which enforces this?
> 

hmmmmm.... no, it's not my idea.

If you don't like it, don't do it, I don't mind either way.  I sent a 
patch that just solved the compiler error only already, and was told on 
the list it would be cooler if these things returned an int instead.

There's no point challenging me about the wisdom of it, although it 
seems reasonable to me.  I sent a patch, list guy $1 says do X instead, 
I do X and then list guy $2 says EXPLAIN YOURSELF.

I really don't care, $1 should argue with $2 and leave me out of it.

If I don't hear anything more I'll reissue with just the minimal change 
later.

-Andy
  
Shreyansh Jain May 20, 2018, 2:43 a.m. UTC | #5
> -----Original Message-----

> From: Andy Green [mailto:andy@warmcat.com]

> Sent: Friday, May 18, 2018 4:42 PM

> To: Shreyansh Jain <shreyansh.jain@nxp.com>; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v5 01/21] lib/librte_ethdev: change eth-

> dev-ops API to return int

> 

> 

> 

> On 05/18/2018 06:59 PM, Shreyansh Jain wrote:

> >> -----Original Message-----

> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green

> >> Sent: Thursday, May 17, 2018 7:19 PM

> >> To: dev@dpdk.org

> >> Subject: [dpdk-dev] [PATCH v5 01/21] lib/librte_ethdev: change eth-

> dev-

> >> ops API to return int

> >>

> >> Signed-off-by: Andy Green <andy@warmcat.com>

> >> ---

> >>   drivers/net/ark/ark_ethdev_rx.c     |    4 ++--

> >>   drivers/net/ark/ark_ethdev_rx.h     |    3 +--

> >>   drivers/net/avf/avf_rxtx.c          |    4 ++--

> >>   drivers/net/avf/avf_rxtx.h          |    2 +-

> >>   drivers/net/bnxt/bnxt_ethdev.c      |    5 +++--

> >>   drivers/net/dpaa/dpaa_ethdev.c      |    4 ++--

> >>   drivers/net/dpaa2/dpaa2_ethdev.c    |    6 +++---

> >>   drivers/net/e1000/e1000_ethdev.h    |    6 ++----

> >>   drivers/net/e1000/em_rxtx.c         |    4 ++--

> >>   drivers/net/e1000/igb_rxtx.c        |    4 ++--

> >>   drivers/net/enic/enic_ethdev.c      |    9 +++------

> >>   drivers/net/i40e/i40e_rxtx.c        |    4 ++--

> >>   drivers/net/i40e/i40e_rxtx.h        |    3 +--

> >>   drivers/net/ixgbe/ixgbe_ethdev.h    |    3 +--

> >>   drivers/net/ixgbe/ixgbe_rxtx.c      |    4 ++--

> >>   drivers/net/nfp/nfp_net.c           |    9 ++++-----

> >>   drivers/net/sfc/sfc_ethdev.c        |    4 ++--

> >>   drivers/net/thunderx/nicvf_ethdev.c |    2 +-

> >>   drivers/net/thunderx/nicvf_rxtx.c   |    4 ++--

> >>   drivers/net/thunderx/nicvf_rxtx.h   |    2 +-

> >>   drivers/net/vhost/rte_eth_vhost.c   |    4 ++--

> >>   examples/l3fwd-power/main.c         |    2 +-

> >>   lib/librte_ethdev/rte_ethdev_core.h |    4 ++--

> >>   23 files changed, 44 insertions(+), 52 deletions(-)

> >>

> >

> > [...]

> >

> >>   	rxq = dev->data->rx_queues[rx_queue_id];

> >> diff --git a/drivers/net/dpaa/dpaa_ethdev.c

> >> b/drivers/net/dpaa/dpaa_ethdev.c

> >> index d014a11aa..70a5b4851 100644

> >> --- a/drivers/net/dpaa/dpaa_ethdev.c

> >> +++ b/drivers/net/dpaa/dpaa_ethdev.c

> >> @@ -725,7 +725,7 @@ static void dpaa_eth_tx_queue_release(void *txq

> >> __rte_unused)

> >>   	PMD_INIT_FUNC_TRACE();

> >>   }

> >>

> >> -static uint32_t

> >> +static int

> >>   dpaa_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t

> rx_queue_id)

> >>   {

> >>   	struct dpaa_if *dpaa_intf = dev->data->dev_private;

> >> @@ -738,7 +738,7 @@ dpaa_dev_rx_queue_count(struct rte_eth_dev *dev,

> >> uint16_t rx_queue_id)

> >>   		RTE_LOG(DEBUG, PMD, "RX frame count for q(%d) is %u\n",

> >>   			rx_queue_id, frm_cnt);

> >>   	}

> >> -	return frm_cnt;

> >> +	return (int)frm_cnt;

> >>   }

> >>

> >>   static int dpaa_link_down(struct rte_eth_dev *dev)

> >> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c

> >> b/drivers/net/dpaa2/dpaa2_ethdev.c

> >> index 9297725d9..eb6245b83 100644

> >> --- a/drivers/net/dpaa2/dpaa2_ethdev.c

> >> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c

> >> @@ -604,7 +604,7 @@ dpaa2_dev_tx_queue_release(void *q __rte_unused)

> >>   	PMD_INIT_FUNC_TRACE();

> >>   }

> >>

> >> -static uint32_t

> >> +static int

> >>   dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t

> >> rx_queue_id)

> >>   {

> >>   	int32_t ret;

> >> @@ -612,7 +612,7 @@ dpaa2_dev_rx_queue_count(struct rte_eth_dev

> *dev,

> >> uint16_t rx_queue_id)

> >>   	struct dpaa2_queue *dpaa2_q;

> >>   	struct qbman_swp *swp;

> >>   	struct qbman_fq_query_np_rslt state;

> >> -	uint32_t frame_cnt = 0;

> >> +	int frame_cnt = 0;

> >>

> >>   	PMD_INIT_FUNC_TRACE();

> >>

> >> @@ -628,7 +628,7 @@ dpaa2_dev_rx_queue_count(struct rte_eth_dev

> *dev,

> >> uint16_t rx_queue_id)

> >>   	dpaa2_q = (struct dpaa2_queue *)priv->rx_vq[rx_queue_id];

> >>

> >>   	if (qbman_fq_query_state(swp, dpaa2_q->fqid, &state) == 0) {

> >> -		frame_cnt = qbman_fq_state_frame_count(&state);

> >> +		frame_cnt = (int)qbman_fq_state_frame_count(&state);

> >>   		DPAA2_PMD_DEBUG("RX frame count for q(%d) is %u",

> >>   				rx_queue_id, frame_cnt);

> >>   	}

> >

> > This doesn't feel correct. A counter, especially the number of

> descriptors in a queue, doesn't have a negative value. So, 1) this is

> an unnatural return and 2) we litter the code with unnecessary

> typecast.

> >

> > In fact, even in the above change, the debug messages continue to

> print unsigned values. So, another typecast would be required there.

> >

> > I don't agree with this change - at least not until some strong gcc 8

> warning reason is triggering this. Can you please point me to some

> conversation on mailing list which enforces this?

> >

> 

> hmmmmm.... no, it's not my idea.

> 

> If you don't like it, don't do it, I don't mind either way.  I sent a

> patch that just solved the compiler error only already, and was told on

> the list it would be cooler if these things returned an int instead.

> 

> There's no point challenging me about the wisdom of it, although it

> seems reasonable to me.  I sent a patch, list guy $1 says do X instead,

> I do X and then list guy $2 says EXPLAIN YOURSELF.


That is what a community is. Consensus has to be built, not expected automagically. If you touch a line, you are responsible for it (also, because in future git blame would point *you* out for a change).

> 

> I really don't care, $1 should argue with $2 and leave me out of it.


I was expecting that you point me out to where the $1 conversation was - so that I could have understood reason for your change. You didn’t do that and rather went into a tangential conversation.

I will not be searching through your previous patches and conversations to understand why $1 said something and you *agreed* to go ahead and make changes.

> 

> If I don't hear anything more I'll reissue with just the minimal change

> later.

> 

> -Andy
  
Andy Green May 20, 2018, 4:11 a.m. UTC | #6
On 05/20/2018 10:43 AM, Shreyansh Jain wrote:
>> -----Original Message-----
>> From: Andy Green [mailto:andy@warmcat.com]
>> Sent: Friday, May 18, 2018 4:42 PM
>> To: Shreyansh Jain <shreyansh.jain@nxp.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v5 01/21] lib/librte_ethdev: change eth-
>> dev-ops API to return int
>>
>>
>>
>> On 05/18/2018 06:59 PM, Shreyansh Jain wrote:
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>>>> Sent: Thursday, May 17, 2018 7:19 PM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH v5 01/21] lib/librte_ethdev: change eth-
>> dev-
>>>> ops API to return int
>>>>
>>>> Signed-off-by: Andy Green <andy@warmcat.com>
>>>> ---
>>>>    drivers/net/ark/ark_ethdev_rx.c     |    4 ++--
>>>>    drivers/net/ark/ark_ethdev_rx.h     |    3 +--
>>>>    drivers/net/avf/avf_rxtx.c          |    4 ++--
>>>>    drivers/net/avf/avf_rxtx.h          |    2 +-
>>>>    drivers/net/bnxt/bnxt_ethdev.c      |    5 +++--
>>>>    drivers/net/dpaa/dpaa_ethdev.c      |    4 ++--
>>>>    drivers/net/dpaa2/dpaa2_ethdev.c    |    6 +++---
>>>>    drivers/net/e1000/e1000_ethdev.h    |    6 ++----
>>>>    drivers/net/e1000/em_rxtx.c         |    4 ++--
>>>>    drivers/net/e1000/igb_rxtx.c        |    4 ++--
>>>>    drivers/net/enic/enic_ethdev.c      |    9 +++------
>>>>    drivers/net/i40e/i40e_rxtx.c        |    4 ++--
>>>>    drivers/net/i40e/i40e_rxtx.h        |    3 +--
>>>>    drivers/net/ixgbe/ixgbe_ethdev.h    |    3 +--
>>>>    drivers/net/ixgbe/ixgbe_rxtx.c      |    4 ++--
>>>>    drivers/net/nfp/nfp_net.c           |    9 ++++-----
>>>>    drivers/net/sfc/sfc_ethdev.c        |    4 ++--
>>>>    drivers/net/thunderx/nicvf_ethdev.c |    2 +-
>>>>    drivers/net/thunderx/nicvf_rxtx.c   |    4 ++--
>>>>    drivers/net/thunderx/nicvf_rxtx.h   |    2 +-
>>>>    drivers/net/vhost/rte_eth_vhost.c   |    4 ++--
>>>>    examples/l3fwd-power/main.c         |    2 +-
>>>>    lib/librte_ethdev/rte_ethdev_core.h |    4 ++--
>>>>    23 files changed, 44 insertions(+), 52 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>>    	rxq = dev->data->rx_queues[rx_queue_id];
>>>> diff --git a/drivers/net/dpaa/dpaa_ethdev.c
>>>> b/drivers/net/dpaa/dpaa_ethdev.c
>>>> index d014a11aa..70a5b4851 100644
>>>> --- a/drivers/net/dpaa/dpaa_ethdev.c
>>>> +++ b/drivers/net/dpaa/dpaa_ethdev.c
>>>> @@ -725,7 +725,7 @@ static void dpaa_eth_tx_queue_release(void *txq
>>>> __rte_unused)
>>>>    	PMD_INIT_FUNC_TRACE();
>>>>    }
>>>>
>>>> -static uint32_t
>>>> +static int
>>>>    dpaa_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t
>> rx_queue_id)
>>>>    {
>>>>    	struct dpaa_if *dpaa_intf = dev->data->dev_private;
>>>> @@ -738,7 +738,7 @@ dpaa_dev_rx_queue_count(struct rte_eth_dev *dev,
>>>> uint16_t rx_queue_id)
>>>>    		RTE_LOG(DEBUG, PMD, "RX frame count for q(%d) is %u\n",
>>>>    			rx_queue_id, frm_cnt);
>>>>    	}
>>>> -	return frm_cnt;
>>>> +	return (int)frm_cnt;
>>>>    }
>>>>
>>>>    static int dpaa_link_down(struct rte_eth_dev *dev)
>>>> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c
>>>> b/drivers/net/dpaa2/dpaa2_ethdev.c
>>>> index 9297725d9..eb6245b83 100644
>>>> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
>>>> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
>>>> @@ -604,7 +604,7 @@ dpaa2_dev_tx_queue_release(void *q __rte_unused)
>>>>    	PMD_INIT_FUNC_TRACE();
>>>>    }
>>>>
>>>> -static uint32_t
>>>> +static int
>>>>    dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t
>>>> rx_queue_id)
>>>>    {
>>>>    	int32_t ret;
>>>> @@ -612,7 +612,7 @@ dpaa2_dev_rx_queue_count(struct rte_eth_dev
>> *dev,
>>>> uint16_t rx_queue_id)
>>>>    	struct dpaa2_queue *dpaa2_q;
>>>>    	struct qbman_swp *swp;
>>>>    	struct qbman_fq_query_np_rslt state;
>>>> -	uint32_t frame_cnt = 0;
>>>> +	int frame_cnt = 0;
>>>>
>>>>    	PMD_INIT_FUNC_TRACE();
>>>>
>>>> @@ -628,7 +628,7 @@ dpaa2_dev_rx_queue_count(struct rte_eth_dev
>> *dev,
>>>> uint16_t rx_queue_id)
>>>>    	dpaa2_q = (struct dpaa2_queue *)priv->rx_vq[rx_queue_id];
>>>>
>>>>    	if (qbman_fq_query_state(swp, dpaa2_q->fqid, &state) == 0) {
>>>> -		frame_cnt = qbman_fq_state_frame_count(&state);
>>>> +		frame_cnt = (int)qbman_fq_state_frame_count(&state);
>>>>    		DPAA2_PMD_DEBUG("RX frame count for q(%d) is %u",
>>>>    				rx_queue_id, frame_cnt);
>>>>    	}
>>>
>>> This doesn't feel correct. A counter, especially the number of
>> descriptors in a queue, doesn't have a negative value. So, 1) this is
>> an unnatural return and 2) we litter the code with unnecessary
>> typecast.
>>>
>>> In fact, even in the above change, the debug messages continue to
>> print unsigned values. So, another typecast would be required there.
>>>
>>> I don't agree with this change - at least not until some strong gcc 8
>> warning reason is triggering this. Can you please point me to some
>> conversation on mailing list which enforces this?
>>>
>>
>> hmmmmm.... no, it's not my idea.
>>
>> If you don't like it, don't do it, I don't mind either way.  I sent a
>> patch that just solved the compiler error only already, and was told on
>> the list it would be cooler if these things returned an int instead.
>>
>> There's no point challenging me about the wisdom of it, although it
>> seems reasonable to me.  I sent a patch, list guy $1 says do X instead,
>> I do X and then list guy $2 says EXPLAIN YOURSELF.
> 
> That is what a community is. Consensus has to be built, not expected automagically. If you touch a line, you are responsible for it (also, because in future git blame would point *you* out for a change).

That makes a lot of sense if you work for Intel.

>> I really don't care, $1 should argue with $2 and leave me out of it.
> 
> I was expecting that you point me out to where the $1 conversation was - so that I could have understood reason for your change. You didn’t do that and rather went into a tangential conversation.
> 
> I will not be searching through your previous patches and conversations to understand why $1 said something and you *agreed* to go ahead and make changes.

As I said I don't care to waste more time digging it up and arguing 
about it either.  It wasn't my idea.  The dude just suggested the 
change, as someone passing by I can't tell if the project is asking me 
to do some "community service" to get my patches in (as is common on the 
kernel) or some random guy is just explaining his prejudices.

Friday I sent a patch here without the return type change at all, 
instead it's just a one-liner fixing the original compile problem, which 
is what I originally sent to the list.  So already you can just pick 
which version you like and move on.

-Andy

>>
>> If I don't hear anything more I'll reissue with just the minimal change
>> later.
>>
>> -Andy
  
Stephen Hemminger May 21, 2018, 6:52 a.m. UTC | #7
On Sun, 20 May 2018 02:43:58 +0000
Shreyansh Jain <shreyansh.jain@nxp.com> wrote:

> > > This doesn't feel correct. A counter, especially the number of  
> > descriptors in a queue, doesn't have a negative value. So, 1) this is
> > an unnatural return and 2) we litter the code with unnecessary
> > typecast.  
> > >
> > > In fact, even in the above change, the debug messages continue to  
> > print unsigned values. So, another typecast would be required there.  
> > >
> > > I don't agree with this change - at least not until some strong gcc 8  
> > warning reason is triggering this. Can you please point me to some
> > conversation on mailing list which enforces this?  
> > >  
> > 
> > hmmmmm.... no, it's not my idea.
> > 
> > If you don't like it, don't do it, I don't mind either way.  I sent a
> > patch that just solved the compiler error only already, and was told on
> > the list it would be cooler if these things returned an int instead.
> > 
> > There's no point challenging me about the wisdom of it, although it
> > seems reasonable to me.  I sent a patch, list guy $1 says do X instead,
> > I do X and then list guy $2 says EXPLAIN YOURSELF.  
> 
> That is what a community is. Consensus has to be built, not expected automagically. If you touch a line, you are responsible for it (also, because in future git blame would point *you* out for a change).


My comment was a suggestion, not a "you must do it this way".
The reason was it was cleaner change for Gcc fix
and it allowed for possibility that some driver might not detect an error
(for example if device was removed by hot plug).
  
Andy Green May 21, 2018, 7:25 a.m. UTC | #8
On 05/21/2018 02:52 PM, Stephen Hemminger wrote:
> On Sun, 20 May 2018 02:43:58 +0000
> Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> 
>>>> This doesn't feel correct. A counter, especially the number of
>>> descriptors in a queue, doesn't have a negative value. So, 1) this is
>>> an unnatural return and 2) we litter the code with unnecessary
>>> typecast.
>>>>
>>>> In fact, even in the above change, the debug messages continue to
>>> print unsigned values. So, another typecast would be required there.
>>>>
>>>> I don't agree with this change - at least not until some strong gcc 8
>>> warning reason is triggering this. Can you please point me to some
>>> conversation on mailing list which enforces this?
>>>>   
>>>
>>> hmmmmm.... no, it's not my idea.
>>>
>>> If you don't like it, don't do it, I don't mind either way.  I sent a
>>> patch that just solved the compiler error only already, and was told on
>>> the list it would be cooler if these things returned an int instead.
>>>
>>> There's no point challenging me about the wisdom of it, although it
>>> seems reasonable to me.  I sent a patch, list guy $1 says do X instead,
>>> I do X and then list guy $2 says EXPLAIN YOURSELF.
>>
>> That is what a community is. Consensus has to be built, not expected automagically. If you touch a line, you are responsible for it (also, because in future git blame would point *you* out for a change).
> 
> 
> My comment was a suggestion, not a "you must do it this way".

OK.

> The reason was it was cleaner change for Gcc fix
> and it allowed for possibility that some driver might not detect an error
> (for example if device was removed by hot plug).

Yes, I also thought it was reasonable.  Even if it was somehow 
unreasonable, it's selfcontained enough in terms of what it does that it 
shouldn't blow anything up.

But it only took me five minutes.  Binning that and just directly fixing 
the compiler warning is also 100% fine from my side and no worries.

The main thing is 18.05 should be usable to build with things that want 
to bind to dpdk using contemporary tools like gcc8.1.  If we can manage 
that, we can build on it for helping lagopus get ahead too.

-Andy
  

Patch

diff --git a/drivers/net/ark/ark_ethdev_rx.c b/drivers/net/ark/ark_ethdev_rx.c
index 987d085e2..7f0a6fc52 100644
--- a/drivers/net/ark/ark_ethdev_rx.c
+++ b/drivers/net/ark/ark_ethdev_rx.c
@@ -407,13 +407,13 @@  eth_ark_rx_queue_drain(struct ark_rx_queue *queue)
 	}
 }
 
-uint32_t
+int
 eth_ark_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t queue_id)
 {
 	struct ark_rx_queue *queue;
 
 	queue = dev->data->rx_queues[queue_id];
-	return (queue->prod_index - queue->cons_index);	/* mod arith */
+	return (int)(queue->prod_index - queue->cons_index);	/* mod arith */
 }
 
 /* ************************************************************************* */
diff --git a/drivers/net/ark/ark_ethdev_rx.h b/drivers/net/ark/ark_ethdev_rx.h
index 146787112..c3f56be3b 100644
--- a/drivers/net/ark/ark_ethdev_rx.h
+++ b/drivers/net/ark/ark_ethdev_rx.h
@@ -47,8 +47,7 @@  int eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev,
 			       unsigned int socket_id,
 			       const struct rte_eth_rxconf *rx_conf,
 			       struct rte_mempool *mp);
-uint32_t eth_ark_dev_rx_queue_count(struct rte_eth_dev *dev,
-				    uint16_t rx_queue_id);
+int eth_ark_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id);
 int eth_ark_rx_stop_queue(struct rte_eth_dev *dev, uint16_t queue_id);
 int eth_ark_rx_start_queue(struct rte_eth_dev *dev, uint16_t queue_id);
 uint16_t eth_ark_recv_pkts_noop(void *rx_queue, struct rte_mbuf **rx_pkts,
diff --git a/drivers/net/avf/avf_rxtx.c b/drivers/net/avf/avf_rxtx.c
index e03a136fc..abfca0e85 100644
--- a/drivers/net/avf/avf_rxtx.c
+++ b/drivers/net/avf/avf_rxtx.c
@@ -1839,13 +1839,13 @@  avf_dev_txq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
 }
 
 /* Get the number of used descriptors of a rx queue */
-uint32_t
+int
 avf_dev_rxq_count(struct rte_eth_dev *dev, uint16_t queue_id)
 {
 #define AVF_RXQ_SCAN_INTERVAL 4
 	volatile union avf_rx_desc *rxdp;
 	struct avf_rx_queue *rxq;
-	uint16_t desc = 0;
+	int desc = 0;
 
 	rxq = dev->data->rx_queues[queue_id];
 	rxdp = &rxq->rx_ring[rxq->rx_tail];
diff --git a/drivers/net/avf/avf_rxtx.h b/drivers/net/avf/avf_rxtx.h
index 297d0776d..fa287a003 100644
--- a/drivers/net/avf/avf_rxtx.h
+++ b/drivers/net/avf/avf_rxtx.h
@@ -185,7 +185,7 @@  void avf_dev_rxq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
 			  struct rte_eth_rxq_info *qinfo);
 void avf_dev_txq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
 			  struct rte_eth_txq_info *qinfo);
-uint32_t avf_dev_rxq_count(struct rte_eth_dev *dev, uint16_t queue_id);
+int avf_dev_rxq_count(struct rte_eth_dev *dev, uint16_t queue_id);
 int avf_dev_rx_desc_status(void *rx_queue, uint16_t offset);
 int avf_dev_tx_desc_status(void *tx_queue, uint16_t offset);
 
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 9edcc7b7d..edee42bb2 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -1610,15 +1610,16 @@  bnxt_dev_led_off_op(struct rte_eth_dev *dev)
 	return bnxt_hwrm_port_led_cfg(bp, false);
 }
 
-static uint32_t
+static int
 bnxt_rx_queue_count_op(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
-	uint32_t desc = 0, raw_cons = 0, cons;
 	struct bnxt_cp_ring_info *cpr;
+	uint32_t raw_cons = 0, cons;
 	struct bnxt_rx_queue *rxq;
 	struct rx_pkt_cmpl *rxcmp;
 	uint16_t cmp_type;
 	uint8_t cmp = 1;
+	int desc = 0;
 	bool valid;
 
 	rxq = dev->data->rx_queues[rx_queue_id];
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index d014a11aa..70a5b4851 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -725,7 +725,7 @@  static void dpaa_eth_tx_queue_release(void *txq __rte_unused)
 	PMD_INIT_FUNC_TRACE();
 }
 
-static uint32_t
+static int
 dpaa_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
 	struct dpaa_if *dpaa_intf = dev->data->dev_private;
@@ -738,7 +738,7 @@  dpaa_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 		RTE_LOG(DEBUG, PMD, "RX frame count for q(%d) is %u\n",
 			rx_queue_id, frm_cnt);
 	}
-	return frm_cnt;
+	return (int)frm_cnt;
 }
 
 static int dpaa_link_down(struct rte_eth_dev *dev)
diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 9297725d9..eb6245b83 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -604,7 +604,7 @@  dpaa2_dev_tx_queue_release(void *q __rte_unused)
 	PMD_INIT_FUNC_TRACE();
 }
 
-static uint32_t
+static int
 dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
 	int32_t ret;
@@ -612,7 +612,7 @@  dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 	struct dpaa2_queue *dpaa2_q;
 	struct qbman_swp *swp;
 	struct qbman_fq_query_np_rslt state;
-	uint32_t frame_cnt = 0;
+	int frame_cnt = 0;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -628,7 +628,7 @@  dpaa2_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 	dpaa2_q = (struct dpaa2_queue *)priv->rx_vq[rx_queue_id];
 
 	if (qbman_fq_query_state(swp, dpaa2_q->fqid, &state) == 0) {
-		frame_cnt = qbman_fq_state_frame_count(&state);
+		frame_cnt = (int)qbman_fq_state_frame_count(&state);
 		DPAA2_PMD_DEBUG("RX frame count for q(%d) is %u",
 				rx_queue_id, frame_cnt);
 	}
diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
index 902001f36..aeaa90048 100644
--- a/drivers/net/e1000/e1000_ethdev.h
+++ b/drivers/net/e1000/e1000_ethdev.h
@@ -370,8 +370,7 @@  int eth_igb_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
 		const struct rte_eth_rxconf *rx_conf,
 		struct rte_mempool *mb_pool);
 
-uint32_t eth_igb_rx_queue_count(struct rte_eth_dev *dev,
-		uint16_t rx_queue_id);
+int eth_igb_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id);
 
 int eth_igb_rx_descriptor_done(void *rx_queue, uint16_t offset);
 
@@ -447,8 +446,7 @@  int eth_em_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
 		const struct rte_eth_rxconf *rx_conf,
 		struct rte_mempool *mb_pool);
 
-uint32_t eth_em_rx_queue_count(struct rte_eth_dev *dev,
-		uint16_t rx_queue_id);
+int eth_em_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id);
 
 int eth_em_rx_descriptor_done(void *rx_queue, uint16_t offset);
 
diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c
index a6b3e92a6..827bf1f7d 100644
--- a/drivers/net/e1000/em_rxtx.c
+++ b/drivers/net/e1000/em_rxtx.c
@@ -1476,7 +1476,7 @@  eth_em_rx_queue_setup(struct rte_eth_dev *dev,
 	return 0;
 }
 
-uint32_t
+int
 eth_em_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
 #define EM_RXQ_SCAN_INTERVAL 4
@@ -1496,7 +1496,7 @@  eth_em_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 				desc - rxq->nb_rx_desc]);
 	}
 
-	return desc;
+	return (int)desc;
 }
 
 int
diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
index 5f729f271..753f1f291 100644
--- a/drivers/net/e1000/igb_rxtx.c
+++ b/drivers/net/e1000/igb_rxtx.c
@@ -1757,7 +1757,7 @@  eth_igb_rx_queue_setup(struct rte_eth_dev *dev,
 	return 0;
 }
 
-uint32_t
+int
 eth_igb_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
 #define IGB_RXQ_SCAN_INTERVAL 4
@@ -1777,7 +1777,7 @@  eth_igb_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 				desc - rxq->nb_rx_desc]);
 	}
 
-	return desc;
+	return (int)desc;
 }
 
 int
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 286308924..14dbe9f91 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -269,11 +269,10 @@  static void enicpmd_dev_rx_queue_release(void *rxq)
 	enic_free_rq(rxq);
 }
 
-static uint32_t enicpmd_dev_rx_queue_count(struct rte_eth_dev *dev,
-					   uint16_t rx_queue_id)
+static int enicpmd_dev_rx_queue_count(struct rte_eth_dev *dev,
+				      uint16_t rx_queue_id)
 {
 	struct enic *enic = pmd_priv(dev);
-	uint32_t queue_count = 0;
 	struct vnic_cq *cq;
 	uint32_t cq_tail;
 	uint16_t cq_idx;
@@ -288,9 +287,7 @@  static uint32_t enicpmd_dev_rx_queue_count(struct rte_eth_dev *dev,
 	if (cq_tail < cq_idx)
 		cq_tail += cq->ring.desc_count;
 
-	queue_count = cq_tail - cq_idx;
-
-	return queue_count;
+	return (int)(cq_tail - cq_idx);
 }
 
 static int enicpmd_dev_rx_queue_setup(struct rte_eth_dev *eth_dev,
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 6032d5541..895ecede8 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1938,7 +1938,7 @@  i40e_dev_rx_queue_release(void *rxq)
 	rte_free(q);
 }
 
-uint32_t
+int
 i40e_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
 #define I40E_RXQ_SCAN_INTERVAL 4
@@ -1964,7 +1964,7 @@  i40e_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 					desc - rxq->nb_rx_desc]);
 	}
 
-	return desc;
+	return (int)desc;
 }
 
 int
diff --git a/drivers/net/i40e/i40e_rxtx.h b/drivers/net/i40e/i40e_rxtx.h
index ea73a8a1b..669377099 100644
--- a/drivers/net/i40e/i40e_rxtx.h
+++ b/drivers/net/i40e/i40e_rxtx.h
@@ -205,8 +205,7 @@  void i40e_tx_queue_release_mbufs(struct i40e_tx_queue *txq);
 int i40e_alloc_rx_queue_mbufs(struct i40e_rx_queue *rxq);
 void i40e_rx_queue_release_mbufs(struct i40e_rx_queue *rxq);
 
-uint32_t i40e_dev_rx_queue_count(struct rte_eth_dev *dev,
-				 uint16_t rx_queue_id);
+int i40e_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id);
 int i40e_dev_rx_descriptor_done(void *rx_queue, uint16_t offset);
 int i40e_dev_rx_descriptor_status(void *rx_queue, uint16_t offset);
 int i40e_dev_tx_descriptor_status(void *tx_queue, uint16_t offset);
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index e42ec30d3..236efd9e9 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -567,8 +567,7 @@  int  ixgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
 		uint16_t nb_tx_desc, unsigned int socket_id,
 		const struct rte_eth_txconf *tx_conf);
 
-uint32_t ixgbe_dev_rx_queue_count(struct rte_eth_dev *dev,
-		uint16_t rx_queue_id);
+int ixgbe_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id);
 
 int ixgbe_dev_rx_descriptor_done(void *rx_queue, uint16_t offset);
 
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 3e13d26ae..429d4329d 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -3063,7 +3063,7 @@  ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	return 0;
 }
 
-uint32_t
+int
 ixgbe_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
 #define IXGBE_RXQ_SCAN_INTERVAL 4
@@ -3084,7 +3084,7 @@  ixgbe_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 				desc - rxq->nb_rx_desc]);
 	}
 
-	return desc;
+	return (int)desc;
 }
 
 int
diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 80dc2731c..287796bd0 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -79,8 +79,7 @@  static int nfp_net_link_update(struct rte_eth_dev *dev, int wait_to_complete);
 static void nfp_net_promisc_enable(struct rte_eth_dev *dev);
 static void nfp_net_promisc_disable(struct rte_eth_dev *dev);
 static int nfp_net_rx_fill_freelist(struct nfp_net_rxq *rxq);
-static uint32_t nfp_net_rx_queue_count(struct rte_eth_dev *dev,
-				       uint16_t queue_idx);
+static int nfp_net_rx_queue_count(struct rte_eth_dev *dev, uint16_t queue_idx);
 static uint16_t nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 				  uint16_t nb_pkts);
 static void nfp_net_rx_queue_release(void *rxq);
@@ -1232,13 +1231,13 @@  nfp_net_supported_ptypes_get(struct rte_eth_dev *dev)
 	return NULL;
 }
 
-static uint32_t
+static int
 nfp_net_rx_queue_count(struct rte_eth_dev *dev, uint16_t queue_idx)
 {
 	struct nfp_net_rxq *rxq;
 	struct nfp_net_rx_desc *rxds;
 	uint32_t idx;
-	uint32_t count;
+	int count;
 
 	rxq = (struct nfp_net_rxq *)dev->data->rx_queues[queue_idx];
 
@@ -1263,7 +1262,7 @@  nfp_net_rx_queue_count(struct rte_eth_dev *dev, uint16_t queue_idx)
 		idx++;
 
 		/* Wrapping? */
-		if ((idx) == rxq->rx_count)
+		if (idx == rxq->rx_count)
 			idx = 0;
 	}
 
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 1b6499f85..7bbfbb823 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -1098,14 +1098,14 @@  sfc_tx_queue_info_get(struct rte_eth_dev *dev, uint16_t tx_queue_id,
 	sfc_adapter_unlock(sa);
 }
 
-static uint32_t
+static int
 sfc_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
 	struct sfc_adapter *sa = dev->data->dev_private;
 
 	sfc_log_init(sa, "RxQ=%u", rx_queue_id);
 
-	return sfc_rx_qdesc_npending(sa, rx_queue_id);
+	return (int)sfc_rx_qdesc_npending(sa, rx_queue_id);
 }
 
 static int
diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
index 99fcd516b..21ad1df35 100644
--- a/drivers/net/thunderx/nicvf_ethdev.c
+++ b/drivers/net/thunderx/nicvf_ethdev.c
@@ -1044,7 +1044,7 @@  nicvf_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t qidx,
 static inline void
 nicvf_rx_queue_release_mbufs(struct rte_eth_dev *dev, struct nicvf_rxq *rxq)
 {
-	uint32_t rxq_cnt;
+	int rxq_cnt;
 	uint32_t nb_pkts, released_pkts = 0;
 	uint32_t refill_cnt = 0;
 	struct rte_mbuf *rx_pkts[NICVF_MAX_RX_FREE_THRESH];
diff --git a/drivers/net/thunderx/nicvf_rxtx.c b/drivers/net/thunderx/nicvf_rxtx.c
index 72305d9d2..133b8ba27 100644
--- a/drivers/net/thunderx/nicvf_rxtx.c
+++ b/drivers/net/thunderx/nicvf_rxtx.c
@@ -535,13 +535,13 @@  nicvf_recv_pkts_multiseg(void *rx_queue, struct rte_mbuf **rx_pkts,
 	return to_process;
 }
 
-uint32_t
+int
 nicvf_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t queue_idx)
 {
 	struct nicvf_rxq *rxq;
 
 	rxq = dev->data->rx_queues[queue_idx];
-	return nicvf_addr_read(rxq->cq_status) & NICVF_CQ_CQE_COUNT_MASK;
+	return (int)(nicvf_addr_read(rxq->cq_status) & NICVF_CQ_CQE_COUNT_MASK);
 }
 
 uint32_t
diff --git a/drivers/net/thunderx/nicvf_rxtx.h b/drivers/net/thunderx/nicvf_rxtx.h
index 8bdd582ed..792beb85a 100644
--- a/drivers/net/thunderx/nicvf_rxtx.h
+++ b/drivers/net/thunderx/nicvf_rxtx.h
@@ -83,7 +83,7 @@  nicvf_mbuff_init_mseg_update(struct rte_mbuf *pkt, const uint64_t mbuf_init,
 	*(uint64_t *)(&pkt->rearm_data) = init.value;
 }
 
-uint32_t nicvf_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t queue_idx);
+int nicvf_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t queue_idx);
 uint32_t nicvf_dev_rbdr_refill(struct rte_eth_dev *dev, uint16_t queue_idx);
 
 uint16_t nicvf_recv_pkts(void *rxq, struct rte_mbuf **rx_pkts, uint16_t pkts);
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 0d000c71c..bdf504bac 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1163,7 +1163,7 @@  eth_link_update(struct rte_eth_dev *dev __rte_unused,
 	return 0;
 }
 
-static uint32_t
+static int
 eth_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
 	struct vhost_queue *vq;
@@ -1172,7 +1172,7 @@  eth_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 	if (vq == NULL)
 		return 0;
 
-	return rte_vhost_rx_queue_count(vq->vid, vq->virtqueue_id);
+	return (int)rte_vhost_rx_queue_count(vq->vid, vq->virtqueue_id);
 }
 
 static const struct eth_dev_ops ops = {
diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 596d64548..22dd7006f 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -722,7 +722,7 @@  power_freq_scaleup_heuristic(unsigned lcore_id,
 			     uint16_t port_id,
 			     uint16_t queue_id)
 {
-	uint32_t rxq_count = rte_eth_rx_queue_count(port_id, queue_id);
+	int rxq_count = rte_eth_rx_queue_count(port_id, queue_id);
 /**
  * HW Rx queue size is 128 by default, Rx burst read at maximum 32 entries
  * per iteration
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 33d12b3a2..5e77555d3 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -144,8 +144,8 @@  typedef int (*eth_rx_disable_intr_t)(struct rte_eth_dev *dev,
 typedef void (*eth_queue_release_t)(void *queue);
 /**< @internal Release memory resources allocated by given RX/TX queue. */
 
-typedef uint32_t (*eth_rx_queue_count_t)(struct rte_eth_dev *dev,
-					 uint16_t rx_queue_id);
+typedef int (*eth_rx_queue_count_t)(struct rte_eth_dev *dev,
+				    uint16_t rx_queue_id);
 /**< @internal Get number of used descriptors on a receive queue. */
 
 typedef int (*eth_rx_descriptor_done_t)(void *rxq, uint16_t offset);