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

[Andy Green]

On 05/20/2018 10:43 AM, Shreyansh Jain wrote:
>> -----Original Message-----
>> From: Andy Green [mailto:andy at warmcat.com]
>> Sent: Friday, May 18, 2018 4:42 PM
>> To: Shreyansh Jain <shreyansh.jain at nxp.com>; dev at 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 at dpdk.org] On Behalf Of Andy Green
>>>> Sent: Thursday, May 17, 2018 7:19 PM
>>>> To: dev at 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 at 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