From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Rybchenko <arybchenko@solarflare.com>
Subject: Re: [PATCH v5 6/6] ethdev: complete closing of port
Date: Thu, 18 Oct 2018 11:33:40 +0300
Message-ID: <97e9f9fa-067e-57bf-6ade-3745cd65c703@solarflare.com>
References: <20180907233929.21950-1-thomas@monjalon.net>
 <20181018012402.1240-1-thomas@monjalon.net>
 <20181018012402.1240-7-thomas@monjalon.net>
Mime-Version: 1.0
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Cc: <dev@dpdk.org>, <ophirmu@mellanox.com>, <bernard.iremonger@intel.com>,
 <rahul.lakkireddy@chelsio.com>
To: Thomas Monjalon <thomas@monjalon.net>, <ferruh.yigit@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com
 [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 4CFB85F4A
 for <dev@dpdk.org>; Thu, 18 Oct 2018 10:34:29 +0200 (CEST)
In-Reply-To: <20181018012402.1240-7-thomas@monjalon.net>
Content-Language: en-GB
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 10/18/18 4:24 AM, Thomas Monjalon wrote:
> After closing a port, it cannot be restarted.
> So there is no reason to not free all associated resources.
>
> The last step was done with rte_eth_dev_detach() which is deprecated.
> Instead of blindly removing the associated rte_device, the driver should
> check if no more port (ethdev, cryptodev, etc) is open for the device.
>
> The last ethdev freeing which were done by rte_eth_dev_detach(),
> are now done at the end of rte_eth_dev_close().
>
> Some drivers does not allocate MAC addresses dynamically or separately.
> In those cases, the pointer is set to NULL, in order to avoid wrongly
> freeing them in rte_eth_dev_release_port().
>
> A closed port will have the state RTE_ETH_DEV_UNUSED which is
> considered as invalid by rte_eth_dev_is_valid_port().
> So validity is not checked anymore for closed ports in testpmd.
>
> If the driver is trying to free the port again, the function
> rte_eth_dev_release_port() will abort with -ENODEV error.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

I've tested the patch series together with [1].

As I expected it makes problems and resource leaks if
rte_eth_dev_close() is used.

Everything is OK if I do port stop and detach (with net/sfc
patch which does close from uninit).

If I do port stop, close and detach, the last one returns
error since the device already released and net/sfc uninit
is never called.

Basically it should be one function which is called in both
cases: dev_close or pci_device remove. Similar changes
should be done in many PCI drivers.

If I drop the patch, everything seems to be work fine
from the first sight.
May be it should be removed from the patchset and
considered separately.

[1] http://patches.dpdk.org/project/dpdk/list/?series=1966