[dpdk-stable] [ PATCH 17.11 1/2] ethdev: clear ethdev data upon detach

[dpdk-stable] [ PATCH 17.11 1/2] ethdev: clear ethdev data upon detach

[Reshma Pattan]

When port is detached its relevant rte_eth_dev_data[port_id]
has to be zeroed, otherwise the next port creations
could get wrong port_id.

Fixes: 92d94d3744 ("ethdev: attach or detach port")
Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 096b35faf..7ba9bc564 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -443,6 +443,7 @@ rte_eth_dev_detach(uint16_t port_id, char *name)
 		goto err;
 
 	rte_eth_devices[port_id].state = RTE_ETH_DEV_UNUSED;
+	memset(&rte_eth_dev_data[port_id], 0, sizeof(struct rte_eth_dev_data));
 	return 0;
 
 err:
-- 
2.17.1

[dpdk-stable] [ PATCH 17.11 2/2] app/pdump: remove created vdevs

[Reshma Pattan]

Virtual devices added in pdump application
should be removed explicitly while exiting the pdump application,
otherwise the subsequent run of the pdump application can cause
undefined behaviour due to stale devices still exist in
rte_eth_dev_data[].

Fixes: caa7028276 ("app/pdump: add tool for packet capturing")
Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
 app/pdump/main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 8e42b3647..c6950da75 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -548,6 +548,7 @@ cleanup_pdump_resources(void)
 {
 	int i;
 	struct pdump_tuples *pt;
+	char name[RTE_ETH_NAME_MAX_LEN];
 
 	/* disable pdump and free the pdump_tuple resources */
 	for (i = 0; i < num_tuples; i++) {
@@ -564,6 +565,17 @@ cleanup_pdump_resources(void)
 			free_ring_data(pt->rx_ring, pt->rx_vdev_id, &pt->stats);
 		if (pt->dir & RTE_PDUMP_FLAG_TX)
 			free_ring_data(pt->tx_ring, pt->tx_vdev_id, &pt->stats);
+
+		/* Remove the vdev(s) created */
+		if (pt->dir & RTE_PDUMP_FLAG_RX)
+			rte_eth_dev_detach(pt->rx_vdev_id, name);
+
+		if (pt->single_pdump_dev)
+			continue;
+
+		if (pt->dir & RTE_PDUMP_FLAG_TX)
+			rte_eth_dev_detach(pt->tx_vdev_id, name);
+
 	}
 	cleanup_rings();
 }
-- 
2.17.1

Re: [dpdk-stable] [ PATCH 17.11 1/2] ethdev: clear ethdev data upon detach

[Yongseok Koh]

> On Mar 1, 2019, at 3:53 AM, Reshma Pattan <reshma.pattan@intel.com> wrote:
> 
> When port is detached its relevant rte_eth_dev_data[port_id]
> has to be zeroed, otherwise the next port creations
> could get wrong port_id.
> 
> Fixes: 92d94d3744 ("ethdev: attach or detach port")
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> ---
> lib/librte_ether/rte_ethdev.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 096b35faf..7ba9bc564 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -443,6 +443,7 @@ rte_eth_dev_detach(uint16_t port_id, char *name)
> 		goto err;
> 
> 	rte_eth_devices[port_id].state = RTE_ETH_DEV_UNUSED;
> +	memset(&rte_eth_dev_data[port_id], 0, sizeof(struct rte_eth_dev_data));

Hi,

I believe such code is good to have. Before I merge it, I have a question.
I wonder what the problem of the current code is.
If the state turns into RTE_ETH_DEV_UNUSED,
then will the eth_dev be re-initialized next time anyway?


Thanks,
Yongseok


> 	return 0;
> 
> err:
> -- 
> 2.17.1
> 

Re: [dpdk-stable] [ PATCH 17.11 1/2] ethdev: clear ethdev data upon detach

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: Yongseok Koh [mailto:yskoh@mellanox.com]
> Sent: Monday, March 4, 2019 7:57 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>
> Cc: stable@dpdk.org
> Subject: Re: [dpdk-stable] [ PATCH 17.11 1/2] ethdev: clear ethdev data upon
> detach
> 
> 
> > On Mar 1, 2019, at 3:53 AM, Reshma Pattan <reshma.pattan@intel.com>
> wrote:
> >
> > When port is detached its relevant rte_eth_dev_data[port_id] has to be
> > zeroed, otherwise the next port creations could get wrong port_id.
> >
> > Fixes: 92d94d3744 ("ethdev: attach or detach port")
> > Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> > ---
> > lib/librte_ether/rte_ethdev.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index 096b35faf..7ba9bc564 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -443,6 +443,7 @@ rte_eth_dev_detach(uint16_t port_id, char *name)
> > 		goto err;
> >
> > 	rte_eth_devices[port_id].state = RTE_ETH_DEV_UNUSED;
> > +	memset(&rte_eth_dev_data[port_id], 0, sizeof(struct
> > +rte_eth_dev_data));
> 
> Hi,
> 
> I believe such code is good to have. Before I merge it, I have a question.
> I wonder what the problem of the current code is.
> If the state turns into RTE_ETH_DEV_UNUSED, then will the eth_dev be re-
> initialized next time anyway?
> 
> 

Yes to reuse the UNUSED port next time , its relevant rte_eth_dev_data[] also has to be 0 (which is missing in current code), because attach operation not only checks for UNUSED but also for its relevant data to be  0.
If state UNUSED, but data still not 0, the attach operation will assume port is under use, so it will assign the next new available port id  which is wrong. So we are now fixing this by zeroing dev data upon detach operation.



Thanks,
Reshma

Re: [dpdk-stable] [ PATCH 17.11 1/2] ethdev: clear ethdev data upon detach

[Yongseok Koh]

> On Mar 12, 2019, at 2:31 AM, Pattan, Reshma <reshma.pattan@intel.com> wrote:
> 
> Hi,
> 
>> -----Original Message-----
>> From: Yongseok Koh [mailto:yskoh@mellanox.com]
>> Sent: Monday, March 4, 2019 7:57 PM
>> To: Pattan, Reshma <reshma.pattan@intel.com>
>> Cc: stable@dpdk.org
>> Subject: Re: [dpdk-stable] [ PATCH 17.11 1/2] ethdev: clear ethdev data upon
>> detach
>> 
>> 
>>> On Mar 1, 2019, at 3:53 AM, Reshma Pattan <reshma.pattan@intel.com>
>> wrote:
>>> 
>>> When port is detached its relevant rte_eth_dev_data[port_id] has to be
>>> zeroed, otherwise the next port creations could get wrong port_id.
>>> 
>>> Fixes: 92d94d3744 ("ethdev: attach or detach port")
>>> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
>>> ---
>>> lib/librte_ether/rte_ethdev.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/lib/librte_ether/rte_ethdev.c
>>> b/lib/librte_ether/rte_ethdev.c index 096b35faf..7ba9bc564 100644
>>> --- a/lib/librte_ether/rte_ethdev.c
>>> +++ b/lib/librte_ether/rte_ethdev.c
>>> @@ -443,6 +443,7 @@ rte_eth_dev_detach(uint16_t port_id, char *name)
>>> 		goto err;
>>> 
>>> 	rte_eth_devices[port_id].state = RTE_ETH_DEV_UNUSED;
>>> +	memset(&rte_eth_dev_data[port_id], 0, sizeof(struct
>>> +rte_eth_dev_data));
>> 
>> Hi,
>> 
>> I believe such code is good to have. Before I merge it, I have a question.
>> I wonder what the problem of the current code is.
>> If the state turns into RTE_ETH_DEV_UNUSED, then will the eth_dev be re-
>> initialized next time anyway?
>> 
>> 
> 
> Yes to reuse the UNUSED port next time , its relevant rte_eth_dev_data[] also has to be 0 (which is missing in current code), because attach operation not only checks for UNUSED but also for its relevant data to be  0.
> If state UNUSED, but data still not 0, the attach operation will assume port is under use, so it will assign the next new available port id  which is wrong. So we are now fixing this by zeroing dev data upon detach operation.

applied to stable/17.11

Thanks,
Yongseok