Skip to contentSkip to conversation

net/mlx5: fix verification of device context

PATCH In review Shachar Beiser 5 participants 23 messages Jul 26, 2017 – Aug 8, 2017 Network drivers

lore
Share & tools
lore

Shachar Beiser

author
#0 lore raw

PATCH 1/1 net/mlx5: fix verification of device context

Get interface name function lacks verification of device context. It might lead to segmentation fault when trying to query the name after the device is closed.fixing it by adding the missing verification

Fixes cd89f22a1e9770 ("net/mlx5: remove unused interface name query")
Cc [email protected]
Signed-off-by Shachar Beiser <[email protected]>
---
 drivers/net/mlx5/mlx5_ethdev.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/net/mlx5/mlx5_ethdev.c +4 -0
index b70b7b9..6e67461 100644
@@ -173,6 +173,10 @@ struct priv *
 	char match[IF_NAMESIZE] = "";
 
 	{
+		if (priv->ctx == NULL) {
+			DEBUG("The device is closed, cannot query interface name ");
+			return -1;
+		}
 		MKSTR(path, "%s/device/net", priv->ctx->device->ibdev_path);
 
 		dir = opendir(path);
Signature · 1 line
1.8.3.1
#1 lore raw

Hi Shachar,

Quoted 4 lines · Shachar Beiser

Get interface name function lacks verification of device context. It might lead to segmentation fault when trying to query the name after the device is closed.fixing it by adding the missing verification

Thanks, however if by "close" you mean it may occur when applications use ethdev callbacks after a call to rte_eth_dev_close(), I do not think PMDs have to protect themselves against bad application behavior, otherwise there is no end to such fixes.

The reverse of rte_eth_dev_close() is not rte_eth_dev_configure() nor any other ethdev callback (see documentation), but a bus probe operation.

Perhaps I've missed something, so in case a crash occurs *while* calling rte_eth_dev_close() I guess this patch is fine, but then please describe the reason.

Quoted 1 line
Fixes cd89f22a1e9770 ("net/mlx5: remove unused interface name query")

This commit doesn't look like the root cause of that issue?

Quoted 12 lines
Cc [email protected]
Signed-off-by Shachar Beiser <[email protected]>
---
 drivers/net/mlx5/mlx5_ethdev.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/net/mlx5/mlx5_ethdev.c +0 -0
index b70b7b9..6e67461 100644

drivers/net/mlx5/mlx5_ethdev.c struct priv * · line 179

Quoted 7 lines
drivers/net/mlx5/mlx5_ethdev.c +4 -0
@@ -173,6 +173,10 @@ struct priv *
 	char match[IF_NAMESIZE] = "";
 
 	{
+		if (priv->ctx == NULL) {
+			DEBUG("The device is closed, cannot query interface name ");
+			return -1;
+		}

MKSTR() is at the beginning of this block because it defines a new variable (path). For coding style consistency you should not put any code before variable declarations, or at least insert an empty line between them. Otherwise you could move this check to the parent block.

Quoted 6 lines
drivers/net/mlx5/mlx5_ethdev.c +0 -0
 		MKSTR(path, "%s/device/net", priv->ctx->device->ibdev_path);
 
 		dir = opendir(path);
Signature · 1 line
1.8.3.1

I think this patch is not necessary unless proved otherwise, have you actually observed a crash addressed by it?

Signature · 2 lines
Adrien Mazarguil
6WIND
author
#2 lore raw

Hi ,

       When I say close I mean : " mlx5_dev_close" . This function set the priv->ctx to NULL.
       We think this patch is required because we have an open bug of seg fault while accessing priv->ctx == NULL.

                         -Shachar Beiser.

-----Original Message-----
From: Adrien Mazarguil [mailto:[email protected]]
Sent: Wednesday, July 26, 2017 12:06 PM
To: Shachar Beiser <[email protected]>
Cc: [email protected]; Nélio Laranjeiro <[email protected]>; [email protected]
Subject: Re: [PATCH] net/mlx5: fix verification of device context

Hi Shachar,

Quoted 5 lines · Shachar Beiser

Get interface name function lacks verification of device context. It might lead to segmentation fault when trying to query the name after the device is closed.fixing it by adding the missing verification

Thanks, however if by "close" you mean it may occur when applications use ethdev callbacks after a call to rte_eth_dev_close(), I do not think PMDs have to protect themselves against bad application behavior, otherwise there is no end to such fixes.

The reverse of rte_eth_dev_close() is not rte_eth_dev_configure() nor any other ethdev callback (see documentation), but a bus probe operation.

Perhaps I've missed something, so in case a crash occurs *while* calling rte_eth_dev_close() I guess this patch is fine, but then please describe the reason.

Quoted 1 line
Fixes cd89f22a1e9770 ("net/mlx5: remove unused interface name query")

This commit doesn't look like the root cause of that issue?

Quoted 12 lines
Cc [email protected]
Signed-off-by Shachar Beiser <[email protected]>
---
 drivers/net/mlx5/mlx5_ethdev.c | 4 ++++
 1 file changed, 4 insertions(+)
(quoted diff) +0 -0

b/drivers/net/mlx5/mlx5_ethdev.c index b70b7b9..6e67461 100644

drivers/net/mlx5/mlx5_ethdev.c struct priv * · line 179

Quoted 7 lines
drivers/net/mlx5/mlx5_ethdev.c +4 -0
@@ -173,6 +173,10 @@ struct priv *
 	char match[IF_NAMESIZE] = "";
 
 	{
+		if (priv->ctx == NULL) {
+			DEBUG("The device is closed, cannot query interface name ");
+			return -1;
+		}

MKSTR() is at the beginning of this block because it defines a new variable (path). For coding style consistency you should not put any code before variable declarations, or at least insert an empty line between them. Otherwise you could move this check to the parent block.

Quoted 6 lines
drivers/net/mlx5/mlx5_ethdev.c +0 -1
 		MKSTR(path, "%s/device/net", priv->ctx->device->ibdev_path);
 
 		dir = opendir(path);
--

1.8.3.1

I think this patch is not necessary unless proved otherwise, have you actually observed a crash addressed by it?

Signature · 2 lines
Adrien Mazarguil
6WIND
#3 lore raw

Hi Shachar,

Quoted 7 lines · Shachar Beiser

Hi ,

       When I say close I mean : " mlx5_dev_close" . This function set the priv->ctx to NULL.
       We think this patch is required because we have an open bug of seg fault while accessing priv->ctx == NULL.

                         -Shachar Beiser.

This patch does not fix the root cause of the issue.
It is a bug in the ether layer, and missing flags within MLX5 PMD. So NACK on this patch, I will send shortly the proper fix.

Best regards,

Quoted 63 lines

-----Original Message-----
From: Adrien Mazarguil [mailto:[email protected]]
Sent: Wednesday, July 26, 2017 12:06 PM
To: Shachar Beiser <[email protected]>
Cc: [email protected]; Nélio Laranjeiro <[email protected]>; [email protected]
Subject: Re: [PATCH] net/mlx5: fix verification of device context

Hi Shachar,

Quoted 5 lines · Shachar Beiser

Get interface name function lacks verification of device context. It might lead to segmentation fault when trying to query the name after the device is closed.fixing it by adding the missing verification

Thanks, however if by "close" you mean it may occur when applications use ethdev callbacks after a call to rte_eth_dev_close(), I do not think PMDs have to protect themselves against bad application behavior, otherwise there is no end to such fixes.

The reverse of rte_eth_dev_close() is not rte_eth_dev_configure() nor any other ethdev callback (see documentation), but a bus probe operation.

Perhaps I've missed something, so in case a crash occurs *while* calling rte_eth_dev_close() I guess this patch is fine, but then please describe the reason.

Quoted 1 line
Fixes cd89f22a1e9770 ("net/mlx5: remove unused interface name query")

This commit doesn't look like the root cause of that issue?

Quoted 12 lines
Cc [email protected]
Signed-off-by Shachar Beiser <[email protected]>
---
 drivers/net/mlx5/mlx5_ethdev.c | 4 ++++
 1 file changed, 4 insertions(+)
(quoted diff) +0 -0

b/drivers/net/mlx5/mlx5_ethdev.c index b70b7b9..6e67461 100644

drivers/net/mlx5/mlx5_ethdev.c struct priv * · line 179

Quoted 7 lines
drivers/net/mlx5/mlx5_ethdev.c +4 -0
@@ -173,6 +173,10 @@ struct priv *
 	char match[IF_NAMESIZE] = "";
 
 	{
+		if (priv->ctx == NULL) {
+			DEBUG("The device is closed, cannot query interface name ");
+			return -1;
+		}

MKSTR() is at the beginning of this block because it defines a new variable (path). For coding style consistency you should not put any code before variable declarations, or at least insert an empty line between them. Otherwise you could move this check to the parent block.

Quoted 6 lines
drivers/net/mlx5/mlx5_ethdev.c +0 -1
 		MKSTR(path, "%s/device/net", priv->ctx->device->ibdev_path);
 
 		dir = opendir(path);
--

1.8.3.1

I think this patch is not necessary unless proved otherwise, have you actually observed a crash addressed by it?

Signature · 2 lines
Adrien Mazarguil
6WIND
Signature · 2 lines
Gaëtan Rivet
6WIND
#4 lore raw

cover fix ethdev device detach

In reply to Shachar Beiser

Device detach in librte_ether is rough right now.

- Device hotplug capability is not properly checked
- Device state should be set after a successful detach
- MLX drivers are lacking the relevant flag
- And this flag should actually be removed, thus occuring an API change for v17.11. An announce follows.

Without this series on an MLX4 port:

testpmd> port close 0
Closing ports...
Port 0 is now not stopped
Done
testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done
testpmd> port close 0
Closing ports...
Done
testpmd> port detach 0
Detaching a port...
testpmd> show port info 0
Segmentation fault (core dumped)

With this series:

testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done
testpmd> port detach 0
Detaching a port...
Please close port first
testpmd> port close 0
Closing ports...
Done
testpmd> port detach 0
Detaching a port...
Port '00:03.0' is detached. Now total ports is 0 Done
testpmd> show port info 0
Invalid port 0
Valid port range is [0]

Gaetan Rivet (6):
ethdev: fix device state on detach
ethdev: properly check detach capability net/mlx4: advertize the detach capability net/mlx5: advertize the detach capability app/testpmd: let the user know device detach failed
doc: announce ethdev API change for detach flag

 app/test-pmd/testpmd.c               |   9 ++++++---
 core                                 | Bin 0 -> 114331648 bytes
 doc/guides/rel_notes/deprecation.rst |   6 ++++++
 drivers/net/mlx4/mlx4.c              |   1 +
 drivers/net/mlx5/mlx5.c              |   1 +
 lib/librte_ether/rte_ethdev.c        |  11 +----------
 6 files changed, 15 insertions(+), 13 deletions(-)
 create mode 100644 core
Signature · 1 line
2.1.4
#5 lore raw

PATCH 1/6 ethdev: fix device state on detach

The device state should be handled by the ether layer when possible. Applications should not have to do it.

Not setting the state to UNUSED will make the port_id of the device valid for all ether API functions, usually resulting in segfault.

Fixes 284c908cc588 ("app/testpmd: request device removal interrupt")
Cc [email protected]
Signed-off-by Gaetan Rivet <[email protected]>
---
 app/test-pmd/testpmd.c        | 1 -
 lib/librte_ether/rte_ethdev.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)
app/test-pmd/testpmd.c +0 -1
index e754d12..9142218 100644
@@ -1836,7 +1836,6 @@ rmv_event_callback(void *arg)
 	close_port(port_id);
 	printf("removing device %s\n", dev->device->name);
 	rte_eal_dev_detach(dev->device);
-	dev->state = RTE_ETH_DEV_UNUSED;
 }
 
 /* This function is used by the interrupt thread */
lib/librte_ether/rte_ethdev.c +1 -0
index d4ebb1b..8c365ed 100644
@@ -453,6 +453,7 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 	if (ret < 0)
 		goto err;
 
+	rte_eth_devices[port_id].state = RTE_ETH_DEV_UNUSED;
 	return 0;
 
 err:
Signature · 1 line
2.1.4
#6 lore raw

PATCH 3/6 net/mlx4: advertize the detach capability

In reply to Gaetan Rivet

This PMD supports hotplug, it is able to be detached.

Signed-off-by Gaetan Rivet <[email protected]>
---
 drivers/net/mlx4/mlx4.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/mlx4/mlx4.c +1 -0
index d41552b..8451f5b 100644
@@ -6376,6 +6376,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 
 		priv->dev = eth_dev;
 		eth_dev->dev_ops = &mlx4_dev_ops;
+		eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 		/* Bring Ethernet device up. */
 		DEBUG("forcing Ethernet interface up");
Signature · 1 line
2.1.4
#7 lore raw

PATCH 4/6 net/mlx5: advertize the detach capability

In reply to Gaetan Rivet
Signed-off-by Gaetan Rivet <[email protected]>
---
 drivers/net/mlx5/mlx5.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/mlx5/mlx5.c +1 -0
index 49d4dba..5fd0e76 100644
@@ -788,6 +788,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 
 		eth_dev->device = &pci_dev->device;
 		rte_eth_copy_pci_info(eth_dev, pci_dev);
+		eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 		eth_dev->device->driver = &mlx5_driver.driver;
 		priv->dev = eth_dev;
 		eth_dev->dev_ops = &mlx5_dev_ops;
Signature · 1 line
2.1.4
#8 lore raw

PATCH 5/6 app/testpmd: let the user know device detach failed

In reply to Gaetan Rivet
Signed-off-by Gaetan Rivet <[email protected]>
---
 app/test-pmd/testpmd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
app/test-pmd/testpmd.c +6 -2
index 9142218..9a36e66 100644
@@ -1728,8 +1728,10 @@ detach_port(uint8_t port_id)
 	if (ports[port_id].flow_list)
 		port_flow_flush(port_id);
 
-	if (rte_eth_dev_detach(port_id, name))
+	if (rte_eth_dev_detach(port_id, name)) {
+		RTE_LOG(ERR, USER1, "Failed to detach port '%s'\n", name);
 		return;
+	}
 
 	nb_ports = rte_eth_dev_count();
 

@@ -1835,7 +1837,9 @@ rmv_event_callback(void *arg)
 	stop_port(port_id);
 	close_port(port_id);
 	printf("removing device %s\n", dev->device->name);
-	rte_eal_dev_detach(dev->device);
+	if (rte_eal_dev_detach(dev->device))
+		RTE_LOG(ERR, USER1, "Failed to detach device %s\n",
+			dev->device->name);
 }
 
 /* This function is used by the interrupt thread */
Signature · 1 line
2.1.4
#9 lore raw

PATCH 6/6 doc: announce ethdev API change for detach flag

In reply to Gaetan Rivet

The flag RTE_ETH_DEV_DETACHABLE will disappear.

This flag is not needed anymore following the hotplug work done for v17.08. It can be removed, its intent is now implicitly made available by the relevant EAL and rte_bus implementations.

Signed-off-by Gaetan Rivet <[email protected]>
---
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 1 file changed, 6 insertions(+)
doc/guides/rel_notes/deprecation.rst +6 -0
index 257dcba..9cc2299 100644
@@ -64,3 +64,9 @@ Deprecation Notices
   be removed in 17.11:
 
   - ``rte_eal_parse_devargs_str``, replaced by ``rte_eal_devargs_parse``
+
+* ethdev: The device flag advertizing hotplug capability
+  ``RTE_ETH_DEV_DETACHABLE`` is not needed anymore and will be removed in
+  v17.11. Drivers must fullfill any condition set by their bus and the latter
+  will be able to recognize this capability by checking those upon a detach
+  attempt.
Signature · 1 line
2.1.4
#10 lore raw

v2 cover fix ethdev device detach

In reply to Gaetan Rivet

Device detach in librte_ether is rough right now.

- Device hotplug capability is not properly checked
- Device state should be set after a successful detach
- MLX drivers are lacking the relevant flag
- And this flag should actually be removed, thus occuring an API change for v17.11. An announce follows.

Without this series on an MLX4 port:

testpmd> port close 0
Closing ports...
Port 0 is now not stopped
Done
testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done
testpmd> port close 0
Closing ports...
Done
testpmd> port detach 0
Detaching a port...
testpmd> show port info 0
Segmentation fault (core dumped)

With this series:

testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done
testpmd> port detach 0
Detaching a port...
Please close port first
testpmd> port close 0
Closing ports...
Done
testpmd> port detach 0
Detaching a port...
Port '00:03.0' is detached. Now total ports is 0 Done
testpmd> show port info 0
Invalid port 0
Valid port range is [0]

v2:

- remove coredump from patchset

Gaetan Rivet (6):
ethdev: fix device state on detach
ethdev: properly check detach capability net/mlx4: advertize the detach capability net/mlx5: advertize the detach capability app/testpmd: let the user know device detach failed
doc: announce ethdev API change for detach flag

 app/test-pmd/testpmd.c               |  9 ++++++---
 doc/guides/rel_notes/deprecation.rst |  6 ++++++
 drivers/net/mlx4/mlx4.c              |  1 +
 drivers/net/mlx5/mlx5.c              |  1 +
 lib/librte_ether/rte_ethdev.c        | 11 +----------
 5 files changed, 15 insertions(+), 13 deletions(-)
Signature · 1 line
2.1.4
#11 lore raw

PATCH v2 1/6 ethdev: fix device state on detach

The device state should be handled by the ether layer when possible. Applications should not have to do it.

Not setting the state to UNUSED will make the port_id of the device valid for all ether API functions, usually resulting in segfault.

Fixes 284c908cc588 ("app/testpmd: request device removal interrupt")
Cc [email protected]
Signed-off-by Gaetan Rivet <[email protected]>
---
 app/test-pmd/testpmd.c        | 1 -
 lib/librte_ether/rte_ethdev.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)
app/test-pmd/testpmd.c +0 -1
index e754d12..9142218 100644
@@ -1836,7 +1836,6 @@ rmv_event_callback(void *arg)
 	close_port(port_id);
 	printf("removing device %s\n", dev->device->name);
 	rte_eal_dev_detach(dev->device);
-	dev->state = RTE_ETH_DEV_UNUSED;
 }
 
 /* This function is used by the interrupt thread */
lib/librte_ether/rte_ethdev.c +1 -0
index d4ebb1b..8c365ed 100644
@@ -453,6 +453,7 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 	if (ret < 0)
 		goto err;
 
+	rte_eth_devices[port_id].state = RTE_ETH_DEV_UNUSED;
 	return 0;
 
 err:
Signature · 1 line
2.1.4
#12 lore raw

PATCH v2 2/6 ethdev: properly check detach capability

In reply to Gaetan Rivet

This capability is not bound to which driver is handling the device, but whether the bus is able to unplug it.

This check is already performed in rte_eal_dev_detach, there is no need to do it in the ether layer.

Signed-off-by Gaetan Rivet <[email protected]>
---
 lib/librte_ether/rte_ethdev.c | 10 ----------
 1 file changed, 10 deletions(-)
lib/librte_ether/rte_ethdev.c +0 -10
index 8c365ed..805ef63 100644
@@ -367,16 +367,6 @@ rte_eth_dev_is_detachable(uint8_t port_id)
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
-	switch (rte_eth_devices[port_id].data->kdrv) {
-	case RTE_KDRV_IGB_UIO:
-	case RTE_KDRV_UIO_GENERIC:
-	case RTE_KDRV_NIC_UIO:
-	case RTE_KDRV_NONE:
-	case RTE_KDRV_VFIO:
-		break;
-	default:
-		return -ENOTSUP;
-	}
 	dev_flags = rte_eth_devices[port_id].data->dev_flags;
 	if ((dev_flags & RTE_ETH_DEV_DETACHABLE) &&
 		(!(dev_flags & RTE_ETH_DEV_BONDED_SLAVE)))
Signature · 1 line
2.1.4
#13 lore raw

PATCH v2 3/6 net/mlx4: advertize the detach capability

In reply to Gaetan Rivet

This PMD supports hotplug, it is able to be detached.

Signed-off-by Gaetan Rivet <[email protected]>
---
 drivers/net/mlx4/mlx4.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/mlx4/mlx4.c +1 -0
index d41552b..8451f5b 100644
@@ -6376,6 +6376,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 
 		priv->dev = eth_dev;
 		eth_dev->dev_ops = &mlx4_dev_ops;
+		eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 		/* Bring Ethernet device up. */
 		DEBUG("forcing Ethernet interface up");
Signature · 1 line
2.1.4
#14 lore raw

PATCH v2 4/6 net/mlx5: advertize the detach capability

In reply to Gaetan Rivet
Signed-off-by Gaetan Rivet <[email protected]>
---
 drivers/net/mlx5/mlx5.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/mlx5/mlx5.c +1 -0
index 49d4dba..5fd0e76 100644
@@ -788,6 +788,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 
 		eth_dev->device = &pci_dev->device;
 		rte_eth_copy_pci_info(eth_dev, pci_dev);
+		eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 		eth_dev->device->driver = &mlx5_driver.driver;
 		priv->dev = eth_dev;
 		eth_dev->dev_ops = &mlx5_dev_ops;
Signature · 1 line
2.1.4
#15 lore raw

PATCH v2 5/6 app/testpmd: let the user know device detach failed

In reply to Gaetan Rivet
Signed-off-by Gaetan Rivet <[email protected]>
---
 app/test-pmd/testpmd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
app/test-pmd/testpmd.c +6 -2
index 9142218..9a36e66 100644
@@ -1728,8 +1728,10 @@ detach_port(uint8_t port_id)
 	if (ports[port_id].flow_list)
 		port_flow_flush(port_id);
 
-	if (rte_eth_dev_detach(port_id, name))
+	if (rte_eth_dev_detach(port_id, name)) {
+		RTE_LOG(ERR, USER1, "Failed to detach port '%s'\n", name);
 		return;
+	}
 
 	nb_ports = rte_eth_dev_count();
 

@@ -1835,7 +1837,9 @@ rmv_event_callback(void *arg)
 	stop_port(port_id);
 	close_port(port_id);
 	printf("removing device %s\n", dev->device->name);
-	rte_eal_dev_detach(dev->device);
+	if (rte_eal_dev_detach(dev->device))
+		RTE_LOG(ERR, USER1, "Failed to detach device %s\n",
+			dev->device->name);
 }
 
 /* This function is used by the interrupt thread */
Signature · 1 line
2.1.4
#16 lore raw

PATCH v2 6/6 doc: announce ethdev API change for detach flag

In reply to Gaetan Rivet

The flag RTE_ETH_DEV_DETACHABLE will disappear.

This flag is not needed anymore following the hotplug work done for v17.08. It can be removed, its intent is now implicitly made available by the relevant EAL and rte_bus implementations.

Signed-off-by Gaetan Rivet <[email protected]>
---
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 1 file changed, 6 insertions(+)
doc/guides/rel_notes/deprecation.rst +6 -0
index 257dcba..9cc2299 100644
@@ -64,3 +64,9 @@ Deprecation Notices
   be removed in 17.11:
 
   - ``rte_eal_parse_devargs_str``, replaced by ``rte_eal_devargs_parse``
+
+* ethdev: The device flag advertizing hotplug capability
+  ``RTE_ETH_DEV_DETACHABLE`` is not needed anymore and will be removed in
+  v17.11. Drivers must fullfill any condition set by their bus and the latter
+  will be able to recognize this capability by checking those upon a detach
+  attempt.
Signature · 1 line
2.1.4
Acked-by on v2 6/6
#17 lore raw

Re: [PATCH v2 6/6] doc: announce ethdev API change for detach flag

Quoted 10 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Gaetan Rivet
Sent: Wednesday, July 26, 2017 2:36 PM
To: [email protected]
Cc: Gaetan Rivet <[email protected]>
Subject: [dpdk-dev] [PATCH v2 6/6] doc: announce ethdev API change for detach flag

Quoted 7 lines

The flag RTE_ETH_DEV_DETACHABLE will disappear.

This flag is not needed anymore following the hotplug work done for v17.08. It can be removed, its intent is now implicitly made available by the relevant EAL and rte_bus implementations.

Signed-off-by Gaetan Rivet <[email protected]>
Acked-by John McNamara <[email protected]>
#18 lore raw

PATCH v3 1/1 doc: announce ethdev API change for detach flag

In reply to Gaetan Rivet

The flag RTE_ETH_DEV_DETACHABLE will disappear.

This flag is not needed anymore following the hotplug work done for v17.08. It can be removed, its function is now implicitly made available by the relevant EAL and rte_bus implementations.

Signed-off-by Gaetan Rivet <[email protected]>
Acked-by John McNamara <[email protected]>
Acked-by Adrien Mazarguil <[email protected]>

---

v3:

- Sending this patch separately from the rest of the series.
- Clarify the intent and rationale of this API change.

 doc/guides/rel_notes/deprecation.rst | 8 ++++++++
 1 file changed, 8 insertions(+)
doc/guides/rel_notes/deprecation.rst +8 -0
index 3c687b1..76da437 100644
@@ -69,3 +69,11 @@ Deprecation Notices
   be removed in 17.11:
 
   - ``rte_cryptodev_create_vdev``
+
+* ethdev: The device flag advertizing hotplug capability
+  ``RTE_ETH_DEV_DETACHABLE`` is not needed anymore and will be removed in
+  v17.11.
+  This capability is verified upon calling the relevant hotplug functions in EAL
+  by checking that the ``unplug`` ops is set in the bus. This verification is
+  done by the EAL and not by the ``ethdev`` layer anymore. Users relying on this
+  flag being present only have to remove their checks to follow the change.
Signature · 1 line
2.1.4
Acked-by on v3 1/1 Applied
#19 lore raw

Re: [PATCH v3] doc: announce ethdev API change for detach flag

31/07/2017 11:40, Gaetan Rivet:

Quoted 9 lines

The flag RTE_ETH_DEV_DETACHABLE will disappear.

This flag is not needed anymore following the hotplug work done for v17.08. It can be removed, its function is now implicitly made available by the relevant EAL and rte_bus implementations.

Signed-off-by Gaetan Rivet <[email protected]>
Acked-by John McNamara <[email protected]>
Acked-by Adrien Mazarguil <[email protected]>
Acked-by Thomas Monjalon <[email protected]>

Applied, thanks

author Tested-by on the v2 series
#20 lore raw

Re: [PATCH v2 0/6] fix ethdev device detach

In reply to Gaetan Rivet
Tested-by Shachar Beiser <[email protected]>

The bug is fixed and now there is no crash:

testpmd> port stop all
Stopping ports...
Done
testpmd> port close all
Closing ports...
Done
testpmd> port detach 0
Detaching a port...
Invalid port 0
Please close port first
testpmd> show port info 0
Invalid port 0
Valid port range is [0]
testpmd>

-----Original Message-----
From: Gaetan Rivet [mailto:[email protected]]
Sent: Wednesday, July 26, 2017 4:36 PM
To: [email protected]
Cc: Gaetan Rivet <[email protected]>; Thomas Monjalon <[email protected]>; Shachar Beiser <[email protected]>; Adrien Mazarguil <[email protected]>; Nélio Laranjeiro <[email protected]>
Subject: [PATCH v2 0/6] fix ethdev device detach

Device detach in librte_ether is rough right now.

- Device hotplug capability is not properly checked
- Device state should be set after a successful detach
- MLX drivers are lacking the relevant flag
- And this flag should actually be removed, thus occuring an API change for v17.11. An announce follows.

Without this series on an MLX4 port:

testpmd> port close 0
Closing ports...
Port 0 is now not stopped
Done
testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done
testpmd> port close 0
Closing ports...
Done
testpmd> port detach 0
Detaching a port...
testpmd> show port info 0
Segmentation fault (core dumped)

With this series:

testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done
testpmd> port detach 0
Detaching a port...
Please close port first
testpmd> port close 0
Closing ports...
Done
testpmd> port detach 0
Detaching a port...
Port '00:03.0' is detached. Now total ports is 0 Done
testpmd> show port info 0
Invalid port 0
Valid port range is [0]

v2:

- remove coredump from patchset

Gaetan Rivet (6):
ethdev: fix device state on detach
ethdev: properly check detach capability net/mlx4: advertize the detach capability net/mlx5: advertize the detach capability app/testpmd: let the user know device detach failed
doc: announce ethdev API change for detach flag

 app/test-pmd/testpmd.c               |  9 ++++++---
 doc/guides/rel_notes/deprecation.rst |  6 ++++++
 drivers/net/mlx4/mlx4.c              |  1 +
 drivers/net/mlx5/mlx5.c              |  1 +
 lib/librte_ether/rte_ethdev.c        | 11 +----------
 5 files changed, 15 insertions(+), 13 deletions(-)
Signature · 1 line
2.1.4
Acked-by on the v2 series
#21 lore raw

Re: [PATCH v2 0/6] fix ethdev device detach

In reply to Gaetan Rivet
Quoted 58 lines · Gaetan Rivet

Device detach in librte_ether is rough right now.

- Device hotplug capability is not properly checked
- Device state should be set after a successful detach
- MLX drivers are lacking the relevant flag
- And this flag should actually be removed, thus occuring an API change for v17.11. An announce follows.

Without this series on an MLX4 port:

testpmd> port close 0
Closing ports...
Port 0 is now not stopped
Done
testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done
testpmd> port close 0
Closing ports...
Done
testpmd> port detach 0
Detaching a port...
testpmd> show port info 0
Segmentation fault (core dumped)

With this series:

testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done
testpmd> port detach 0
Detaching a port...
Please close port first
testpmd> port close 0
Closing ports...
Done
testpmd> port detach 0
Detaching a port...
Port '00:03.0' is detached. Now total ports is 0 Done
testpmd> show port info 0
Invalid port 0
Valid port range is [0]

v2:

- remove coredump from patchset

Gaetan Rivet (6):
ethdev: fix device state on detach
ethdev: properly check detach capability net/mlx4: advertize the detach capability net/mlx5: advertize the detach capability app/testpmd: let the user know device detach failed
doc: announce ethdev API change for detach flag

Quoted 10 lines
 app/test-pmd/testpmd.c               |  9 ++++++---
 doc/guides/rel_notes/deprecation.rst |  6 ++++++
 drivers/net/mlx4/mlx4.c              |  1 +
 drivers/net/mlx5/mlx5.c              |  1 +
 lib/librte_ether/rte_ethdev.c        | 11 +----------
 5 files changed, 15 insertions(+), 13 deletions(-)
Signature · 1 line
2.1.4
Acked-by Adrien Mazarguil <[email protected]>
Signature · 2 lines
Adrien Mazarguil
6WIND
#22 lore raw

Re: [PATCH v2 0/6] fix ethdev device detach

Quoted 9 lines
Quoted 7 lines

Gaetan Rivet (6):
ethdev: fix device state on detach
ethdev: properly check detach capability net/mlx4: advertize the detach capability net/mlx5: advertize the detach capability app/testpmd: let the user know device detach failed
doc: announce ethdev API change for detach flag

Acked-by Adrien Mazarguil <[email protected]>

Applied without the last patch for deprecation notice, thanks

This deprecation notice requires more time to be reviewed.