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 49C87A09F0; Wed, 16 Dec 2020 20:52:42 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id AE1BDC9C8; Wed, 16 Dec 2020 20:52:40 +0100 (CET) Received: from mail-pg1-f169.google.com (mail-pg1-f169.google.com [209.85.215.169]) by dpdk.org (Postfix) with ESMTP id 82671C9BC for ; Wed, 16 Dec 2020 20:52:39 +0100 (CET) Received: by mail-pg1-f169.google.com with SMTP id t37so18373141pga.7 for ; Wed, 16 Dec 2020 11:52:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pensando.io; s=google; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=xE5FYIUKgP0QymViuaTpszS339sXz1P6ARs9oQukaG0=; b=PLikRbmNf7mShBPzfLlNrt1XWB6+2dTuYmtzbRy9rcH/V2UzK7vQ4SY/iQrZWypx8d XPT33iqBvip9o/cxioyeTZ+aqsoBtDJkz5EYfkh3R/JdB09maZNRlyWAOt94Kfrs73k5 fUoI7SFsCn+zcnCusx83xcwGVXOOe0DY+dR2+G+jxuO14In73C2jhMpffgEfQIGgy891 cbVdQNpJ2/wcPlLBx0OoU4JeqaN5veGLtNdcRAeAf575PUfuAvSbcB1B5DDE0PoGhaMx L/iIqDqv3031gd8hlbS9o5nt2e39q/n7LVCnXGLf65mSUf9WZfYoHrFzoGMGusbPaTLc T+JA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=xE5FYIUKgP0QymViuaTpszS339sXz1P6ARs9oQukaG0=; b=jIxbAqOUsMv5azAEATrgxbzb8rQFKRD6V4uxR22byubFzeJRbBHt87EHYI25AzsAD/ d8aTB+IRYo21euVgcb3HCKYCx1qXdz5UroNfve6OcJncFRTPYQoYPJyRSODW8mU2WJkR ZfQDELljAoiZvJ/coMWu5PG9REMHpDjUxBO+53CmxCswpoc85M7+LE37WFUx2hyA4ta0 MGtCXOzGnvbVO85yBLHTMcVX1Z3S0rbug5gbg3XOCwF6RV+5M2uJogaoMiUatatB+5Ni itq2m1D15JxvJZxt9emYELa4WAXQesbZ2+BZ37eENKh1elvPehVXf3mI1ulzxWaxWsrC yN9g== X-Gm-Message-State: AOAM531n4Z6sl9G0XMlBLDXvunNThq6LLa+csH4b+ULimW7cf2upxMPx xuoE7Ql/BeXa+8f+KwVp7Wj44g== X-Google-Smtp-Source: ABdhPJxSiq475AE34dMzTNld8LEg31WRhygW9XKzj4AkHl56dv5VL7EfPvWVXQcZwdKZW1OOtJBCnA== X-Received: by 2002:a05:6a00:14cf:b029:18c:959d:929e with SMTP id w15-20020a056a0014cfb029018c959d929emr33676575pfu.53.1608148357565; Wed, 16 Dec 2020 11:52:37 -0800 (PST) Received: from ?IPv6:2600:1700:6b0:fde0:4ca3:81ff:f654:57db? ([2600:1700:6b0:fde0:4ca3:81ff:f654:57db]) by smtp.gmail.com with ESMTPSA id p127sm3340855pfp.93.2020.12.16.11.52.36 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 16 Dec 2020 11:52:37 -0800 (PST) From: Andrew Boyer Message-Id: <475A40F1-ED76-4690-B2FF-0AEE0D2FE681@pensando.io> Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Date: Wed, 16 Dec 2020 14:52:34 -0500 In-Reply-To: Cc: dev@dpdk.org, Alfredo Cardigliano To: Ferruh Yigit References: <20201210142231.63209-1-aboyer@pensando.io> <20201210142231.63209-4-aboyer@pensando.io> X-Mailer: Apple Mail (2.3608.120.23.2.4) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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" Hello Ferruh, > On Dec 15, 2020, at 8:42 AM, Ferruh Yigit = wrote: >=20 > 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 =3D IONIC_ETH_DEV_TO_LIF(eth_dev); >> + struct ionic_adapter *adapter =3D 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 =3D 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 =3D 0; i < adapter->nlifs; i++) { >> + lif =3D adapter->lifs[i]; >> + rte_eth_dev_destroy(lif->eth_dev, eth_ionic_dev_uninit); >=20 > Should 'ionic_lif_stop()' & 'ionic_lif_free_queues()' be called per = 'lif' here? You are correct. A future patch removes multi-lif support - it is = unused. So this is a result of how I broke up the changes. See below. >> } >> + ionic_port_reset(adapter); >> + ionic_reset(adapter); >> + >> + rte_free(adapter); >> + >=20 > 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. Sure, I will add more detail to 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()'? I see what you=E2=80=99re saying, we get here from eth_dev_ops.dev_close = -> ionic_dev_close(), so the first ethdev to get closed brings down the = adapter etc. In reality we are going to have one adapter <-> one lif <-> one ethdev, = so closing the ethdev will be the end of everything. (Just for this = PF/VF; they are independent.) Rather than doing any design work to fix this I will reorder the patches = to take out multi-lif support first. -Andrew >=20 >> 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 =3D 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) >> =20 >=20 > Should remove '__rte_unused' OK >=20 >> eth_dev =3D rte_eth_dev_allocated(name); >> if (eth_dev) { >> - lif =3D IONIC_ETH_DEV_TO_LIF(eth_dev); >> - adapter =3D lif->adapter; >> - } >> - >> - if (adapter) { >> - ionic_unconfigure_intr(adapter); >> - >> - for (i =3D 0; i < adapter->nlifs; i++) { >> - lif =3D 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); >=20 > 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' OK, I will reduce it to DEBUG level. >> } >> 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 =3D 0; i < lif->ntxqcqs; i++) { >> + = ionic_dev_tx_queue_release(lif->eth_dev->data->tx_queues[i]); >> + lif->eth_dev->data->tx_queues[i] =3D NULL; >> + } >> + for (i =3D 0; i < lif->nrxqcqs; i++) { >> + = ionic_dev_rx_queue_release(lif->eth_dev->data->rx_queues[i]); >> + lif->eth_dev->data->rx_queues[i] =3D 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);