From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.warmcat.com (mail.warmcat.com [163.172.24.82]) by dpdk.org (Postfix) with ESMTP id C8F25AAB3 for ; Fri, 18 May 2018 13:12:40 +0200 (CEST) To: Shreyansh Jain , "dev@dpdk.org" References: <152656480225.46638.3271983577765861155.stgit@localhost.localdomain> <152656493702.46638.10712692446180001555.stgit@localhost.localdomain> From: Andy Green Message-ID: <70fc4570-7447-a9fa-dda6-a41f220c35ca@warmcat.com> Date: Fri, 18 May 2018 19:12:26 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v5 01/21] lib/librte_ethdev: change eth-dev-ops API to return int X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 May 2018 11:12:41 -0000 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 >> --- >> 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