From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 63526A04B6; Mon, 12 Oct 2020 17:12:46 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 752EF1D915; Mon, 12 Oct 2020 17:12:35 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id B44681D6C4 for ; Mon, 12 Oct 2020 17:12:32 +0200 (CEST) IronPort-SDR: AvLZ6mNzOBG0V4QlH+2GjEbhPKGcAE0Gk1RKy97+2H8RufEfIE8zEBs1D+XHPtUXCsA61aAROq 4sMtOtk1uYrA== X-IronPort-AV: E=McAfee;i="6000,8403,9771"; a="250448361" X-IronPort-AV: E=Sophos;i="5.77,367,1596524400"; d="scan'208";a="250448361" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2020 08:12:31 -0700 IronPort-SDR: ZqTLBqfltyt/RcR639N80tC51Ls+GNTBGwdQZk776FLH5hzMYtqStHh3V6WkKJoOK68EoUGlaQ +AJI3Lf4qzqg== X-IronPort-AV: E=Sophos;i="5.77,367,1596524400"; d="scan'208";a="520720635" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.244.119]) ([10.213.244.119]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2020 08:12:30 -0700 To: "Wei Hu (Xavier)" , dev@dpdk.org Cc: xavier.huwei@huawei.com References: <20201010071212.24086-1-huwei013@chinasoftinc.com> <20201012073244.7447-1-huwei013@chinasoftinc.com> From: Ferruh Yigit Message-ID: Date: Mon, 12 Oct 2020 16:12:24 +0100 MIME-Version: 1.0 In-Reply-To: <20201012073244.7447-1-huwei013@chinasoftinc.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3] ethdev: check if queue setupped in queue-related APIs 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 10/12/2020 8:32 AM, Wei Hu (Xavier) wrote: > From: Chengchang Tang > > This patch adds checking whether the related Tx or Rx queue has been > setupped in the queue-related API functions to avoid illegal address > access. And validity check of the queue_id is also added in the API > functions rte_eth_dev_rx_intr_enable and rte_eth_dev_rx_intr_disable. > > Signed-off-by: Chengchang Tang > Signed-off-by: Wei Hu (Xavier) > Signed-off-by: Chengwen Feng > --- > v2 -> v3: > don't break lines in format strings. > v1 -> v2: > 1. replace %"PRIu16" with %u. > 2. extact two common functions which validate RXQ/TXQ ids and > whether it has been set up or not. > --- > lib/librte_ethdev/rte_ethdev.c | 92 ++++++++++++++++++++++++++++++++++-------- > lib/librte_ethdev/rte_ethdev.h | 3 +- > 2 files changed, 78 insertions(+), 17 deletions(-) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 892c246..34eec97 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -877,10 +877,59 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues) > return 0; > } > > +static inline int > +rte_eth_dev_validate_rx_queue(uint16_t port_id, uint16_t rx_queue_id) > +{ > + struct rte_eth_dev *dev; > + > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); > + > + dev = &rte_eth_devices[port_id]; > + Since these are static (internal) functions, why not get "struct rte_eth_dev *dev" as parameter and drop the port_id validation? Also for same reason, although there is not convention around it, what do you think dropping the 'rte_' prefix from the funcitons names, to prevent them confused by ethdev APIs? <...> > @@ -4721,7 +4721,8 @@ rte_eth_rx_queue_count(uint16_t port_id, uint16_t queue_id) > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); > dev = &rte_eth_devices[port_id]; > RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_queue_count, -ENOTSUP); > - if (queue_id >= dev->data->nb_rx_queues) > + if (queue_id >= dev->data->nb_rx_queues || > + dev->data->rx_queues[queue_id] == NULL) > return -EINVAL; This will bring additional check for the datapath, but I guess this check is reasonable since this is an API.