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 1ADA4A09EE; Tue, 15 Dec 2020 14:42:24 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 151E4CA28; Tue, 15 Dec 2020 14:42:22 +0100 (CET) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id E89CFCA0E for ; Tue, 15 Dec 2020 14:42:18 +0100 (CET) IronPort-SDR: OsLwlmvXT0EbkiR0BlEf8XFpj/BhNGAnTMJyvjyWwVNeTdWgOhtJnLRwYxH60L+NcJ/ttOGgIx ygcmaUdeW7Iw== X-IronPort-AV: E=McAfee;i="6000,8403,9835"; a="154682964" X-IronPort-AV: E=Sophos;i="5.78,421,1599548400"; d="scan'208";a="154682964" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Dec 2020 05:42:16 -0800 IronPort-SDR: TokORnZUMF4FPyG0M6z9dLRUgqePzYn0uKUIIbWbuRIYCYWXxjVTXc1UAxnGVhk2mLEBdwf0+p GyX6/UXPny6g== X-IronPort-AV: E=Sophos;i="5.78,421,1599548400"; d="scan'208";a="411963101" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.201.43]) ([10.213.201.43]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Dec 2020 05:42:15 -0800 To: Andrew Boyer , dev@dpdk.org Cc: Alfredo Cardigliano References: <20201210142231.63209-1-aboyer@pensando.io> <20201210142231.63209-4-aboyer@pensando.io> From: Ferruh Yigit Message-ID: Date: Tue, 15 Dec 2020 13:42:11 +0000 MIME-Version: 1.0 In-Reply-To: <20201210142231.63209-4-aboyer@pensando.io> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 3/6] net/ionic: fully implement remove-on-close 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 12/10/2020 2:22 PM, Andrew Boyer wrote: > This is now required behavior for a PMD. > Remove the UNMAINTAINED flag. > > Signed-off-by: Andrew Boyer > --- > MAINTAINERS | 2 +- > drivers/net/ionic/ionic_ethdev.c | 41 ++++++++++++++++---------------- > drivers/net/ionic/ionic_lif.c | 15 ++++++++++++ > drivers/net/ionic/ionic_lif.h | 1 + > 4 files changed, 38 insertions(+), 21 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 7bc0010f2..fc1b09923 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -840,7 +840,7 @@ F: doc/guides/nics/pfe.rst > F: drivers/net/pfe/ > F: doc/guides/nics/features/pfe.ini > > -Pensando ionic - UNMAINTAINED > +Pensando ionic > M: Andrew Boyer > F: drivers/net/ionic/ > F: doc/guides/nics/ionic.rst > diff --git a/drivers/net/ionic/ionic_ethdev.c b/drivers/net/ionic/ionic_ethdev.c > index 5a360ac08..629d7068b 100644 > --- a/drivers/net/ionic/ionic_ethdev.c > +++ b/drivers/net/ionic/ionic_ethdev.c > @@ -958,6 +958,8 @@ ionic_dev_stop(struct rte_eth_dev *eth_dev) > return err; > } > > +static void ionic_unconfigure_intr(struct ionic_adapter *adapter); > + > /* > * Reset and stop device. > */ > @@ -965,6 +967,8 @@ static int > ionic_dev_close(struct rte_eth_dev *eth_dev) > { > struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev); > + struct ionic_adapter *adapter = lif->adapter; > + uint32_t i; > int err; > > IONIC_PRINT_CALL(); > @@ -977,12 +981,21 @@ ionic_dev_close(struct rte_eth_dev *eth_dev) > return -1; > } > > - err = eth_ionic_dev_uninit(eth_dev); > - if (err) { > - IONIC_PRINT(ERR, "Cannot destroy LIF: %d", err); > - return -1; > + ionic_lif_free_queues(lif); > + > + IONIC_PRINT(NOTICE, "Removing device %s", eth_dev->device->name); > + ionic_unconfigure_intr(adapter); > + > + for (i = 0; i < adapter->nlifs; i++) { > + lif = adapter->lifs[i]; > + rte_eth_dev_destroy(lif->eth_dev, eth_ionic_dev_uninit); Should 'ionic_lif_stop()' & 'ionic_lif_free_queues()' be called per 'lif' here? > } > > + ionic_port_reset(adapter); > + ionic_reset(adapter); > + > + rte_free(adapter); > + In ionic, an ethdev is created per 'lif' during probe and when one of the ethdev closed, all 'lif' destroyed and 'adapter' freed, so all ethdev should be unusable at this stage. 1) First better to document this behavior in the commit log, and as overall can you please prefer more descriptive commit logs. 2) What happens to the ethdev statuses, 'lif' destroyed and ethdev are not usable but ethdev status not updated or freed? What happens if the application tries to access to ethdev of another 'lif' after 'ionic_dev_close()'? > return 0; > } > > @@ -1280,10 +1293,7 @@ static int > eth_ionic_pci_remove(struct rte_pci_device *pci_dev __rte_unused) > { > char name[RTE_ETH_NAME_MAX_LEN]; > - struct ionic_adapter *adapter = NULL; > struct rte_eth_dev *eth_dev; > - struct ionic_lif *lif; > - uint32_t i; > > /* Adapter lookup is using (the first) eth_dev name */ > snprintf(name, sizeof(name), "net_%s_lif_0", > @@ -1291,19 +1301,10 @@ eth_ionic_pci_remove(struct rte_pci_device *pci_dev __rte_unused) > Should remove '__rte_unused' > eth_dev = rte_eth_dev_allocated(name); > if (eth_dev) { > - lif = IONIC_ETH_DEV_TO_LIF(eth_dev); > - adapter = lif->adapter; > - } > - > - if (adapter) { > - ionic_unconfigure_intr(adapter); > - > - for (i = 0; i < adapter->nlifs; i++) { > - lif = adapter->lifs[i]; > - rte_eth_dev_destroy(lif->eth_dev, eth_ionic_dev_uninit); > - } > - > - rte_free(adapter); > + ionic_dev_close(eth_dev); > + } else { > + IONIC_PRINT(WARNING, "Cannot find device %s", > + pci_dev->device.name); Not sure if this warning is correct, if there is no allocated ethdev this is not an error, this means there is nothing to remove and function can continue with 'return 0' > } > > return 0; > diff --git a/drivers/net/ionic/ionic_lif.c b/drivers/net/ionic/ionic_lif.c > index 875c7e585..e213597ee 100644 > --- a/drivers/net/ionic/ionic_lif.c > +++ b/drivers/net/ionic/ionic_lif.c > @@ -927,6 +927,21 @@ ionic_lif_free(struct ionic_lif *lif) > } > } > > +void > +ionic_lif_free_queues(struct ionic_lif *lif) > +{ > + uint32_t i; > + > + for (i = 0; i < lif->ntxqcqs; i++) { > + ionic_dev_tx_queue_release(lif->eth_dev->data->tx_queues[i]); > + lif->eth_dev->data->tx_queues[i] = NULL; > + } > + for (i = 0; i < lif->nrxqcqs; i++) { > + ionic_dev_rx_queue_release(lif->eth_dev->data->rx_queues[i]); > + lif->eth_dev->data->rx_queues[i] = NULL; > + } > +} > + > int > ionic_lif_rss_config(struct ionic_lif *lif, > const uint16_t types, const uint8_t *key, const uint32_t *indir) > diff --git a/drivers/net/ionic/ionic_lif.h b/drivers/net/ionic/ionic_lif.h > index b80931c61..bf010716e 100644 > --- a/drivers/net/ionic/ionic_lif.h > +++ b/drivers/net/ionic/ionic_lif.h > @@ -122,6 +122,7 @@ int ionic_lifs_size(struct ionic_adapter *ionic); > > int ionic_lif_alloc(struct ionic_lif *lif); > void ionic_lif_free(struct ionic_lif *lif); > +void ionic_lif_free_queues(struct ionic_lif *lif); > > int ionic_lif_init(struct ionic_lif *lif); > void ionic_lif_deinit(struct ionic_lif *lif); >