diff mbox series

[v2] net/ring: fix eth_dev device pointer on allocation

Message ID 61746fd5771ae6d359a02638ee6b5d027b43a981.1588788335.git.grive@u256.net (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] net/ring: fix eth_dev device pointer on allocation |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/travis-robot warning Travis build: failed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation fail Compilation issues
ci/iol-testing success Testing PASS

Commit Message

Gaëtan Rivet May 6, 2020, 6:09 p.m. UTC 
  When a net_ring device is allocated, its device pointer is not set
before calling rte_eth_dev_probing_finish, which is incorrect.

The following:
  commit: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API")
  commit: a6992e961050 ("net/ring: set ethernet device field")

already fixed the same issue in 17.08, which was fine at the time.
Adding the hook rte_eth_dev_probing_finish() however created this bug,
as the eth_dev exposed when this hook is executed is expected to be
complete.

Remove the prior attempts to fix the issue in rte_pmd_ring_probe() and
write the pointer properly in do_eth_dev_ring_create().

Cc: stable@dpdk.org
Fixes: fbe90cdd776c ("ethdev: add probing finish function")
Signed-off-by: Gaetan Rivet <grive@u256.net>
---
 drivers/net/ring/rte_eth_ring.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

Comments

Ferruh Yigit May 8, 2020, 11 a.m. UTC | #1 
On 5/6/2020 7:09 PM, Gaetan Rivet wrote:
> When a net_ring device is allocated, its device pointer is not set
> before calling rte_eth_dev_probing_finish, which is incorrect.
> 
> The following:
>   commit: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API")
>   commit: a6992e961050 ("net/ring: set ethernet device field")
> 
> already fixed the same issue in 17.08, which was fine at the time.
> Adding the hook rte_eth_dev_probing_finish() however created this bug,
> as the eth_dev exposed when this hook is executed is expected to be
> complete.
> 
> Remove the prior attempts to fix the issue in rte_pmd_ring_probe() and
> write the pointer properly in do_eth_dev_ring_create().
> 
> Cc: stable@dpdk.org
> Fixes: fbe90cdd776c ("ethdev: add probing finish function")
> Signed-off-by: Gaetan Rivet <grive@u256.net>

I would prefer moving the assignment up in the stack where 'device' is
available, instead of moving the variable down in the stack to assign it, but
both does the work ...

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Ferruh Yigit May 11, 2020, 4:54 p.m. UTC | #2 
On 5/8/2020 12:00 PM, Ferruh Yigit wrote:
> On 5/6/2020 7:09 PM, Gaetan Rivet wrote:
>> When a net_ring device is allocated, its device pointer is not set
>> before calling rte_eth_dev_probing_finish, which is incorrect.
>>
>> The following:
>>   commit: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API")
>>   commit: a6992e961050 ("net/ring: set ethernet device field")
>>
>> already fixed the same issue in 17.08, which was fine at the time.
>> Adding the hook rte_eth_dev_probing_finish() however created this bug,
>> as the eth_dev exposed when this hook is executed is expected to be
>> complete.
>>
>> Remove the prior attempts to fix the issue in rte_pmd_ring_probe() and
>> write the pointer properly in do_eth_dev_ring_create().
>>
>> Cc: stable@dpdk.org
>> Fixes: fbe90cdd776c ("ethdev: add probing finish function")
>> Signed-off-by: Gaetan Rivet <grive@u256.net>
> 
> I would prefer moving the assignment up in the stack where 'device' is
> available, instead of moving the variable down in the stack to assign it, but
> both does the work ...
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

Applied to dpdk-next-net/master, thanks.
diff mbox series

Patch

diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index 41acbc513..f0fafa0c0 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -246,6 +246,7 @@  static const struct eth_dev_ops ops = {
 
 static int
 do_eth_dev_ring_create(const char *name,
+		struct rte_vdev_device *vdev,
 		struct rte_ring * const rx_queues[],
 		const unsigned int nb_rx_queues,
 		struct rte_ring *const tx_queues[],
@@ -291,12 +292,15 @@  do_eth_dev_ring_create(const char *name,
 	}
 
 	/* now put it all together
+	 * - store EAL device in eth_dev,
 	 * - store queue data in internals,
 	 * - store numa_node info in eth_dev_data
 	 * - point eth_dev_data to internals
 	 * - and point eth_dev structure to new eth_dev_data structure
 	 */
 
+	eth_dev->device = &vdev->device;
+
 	data = eth_dev->data;
 	data->rx_queues = rx_queues_local;
 	data->tx_queues = tx_queues_local;
@@ -408,7 +412,9 @@  rte_eth_from_ring(struct rte_ring *r)
 }
 
 static int
-eth_dev_ring_create(const char *name, const unsigned int numa_node,
+eth_dev_ring_create(const char *name,
+		struct rte_vdev_device *vdev,
+		const unsigned int numa_node,
 		enum dev_action action, struct rte_eth_dev **eth_dev)
 {
 	/* rx and tx are so-called from point of view of first port.
@@ -438,7 +444,7 @@  eth_dev_ring_create(const char *name, const unsigned int numa_node,
 			return -1;
 	}
 
-	if (do_eth_dev_ring_create(name, rxtx, num_rings, rxtx, num_rings,
+	if (do_eth_dev_ring_create(name, vdev, rxtx, num_rings, rxtx, num_rings,
 		numa_node, action, eth_dev) < 0)
 		return -1;
 
@@ -560,12 +566,12 @@  rte_pmd_ring_probe(struct rte_vdev_device *dev)
 	PMD_LOG(INFO, "Initializing pmd_ring for %s", name);
 
 	if (params == NULL || params[0] == '\0') {
-		ret = eth_dev_ring_create(name, rte_socket_id(), DEV_CREATE,
+		ret = eth_dev_ring_create(name, dev, rte_socket_id(), DEV_CREATE,
 				&eth_dev);
 		if (ret == -1) {
 			PMD_LOG(INFO,
 				"Attach to pmd_ring for %s", name);
-			ret = eth_dev_ring_create(name, rte_socket_id(),
+			ret = eth_dev_ring_create(name, dev, rte_socket_id(),
 						  DEV_ATTACH, &eth_dev);
 		}
 	} else {
@@ -574,19 +580,16 @@  rte_pmd_ring_probe(struct rte_vdev_device *dev)
 		if (!kvlist) {
 			PMD_LOG(INFO,
 				"Ignoring unsupported parameters when creatingrings-backed ethernet device");
-			ret = eth_dev_ring_create(name, rte_socket_id(),
+			ret = eth_dev_ring_create(name, dev, rte_socket_id(),
 						  DEV_CREATE, &eth_dev);
 			if (ret == -1) {
 				PMD_LOG(INFO,
 					"Attach to pmd_ring for %s",
 					name);
-				ret = eth_dev_ring_create(name, rte_socket_id(),
+				ret = eth_dev_ring_create(name, dev, rte_socket_id(),
 							  DEV_ATTACH, &eth_dev);
 			}
 
-			if (eth_dev)
-				eth_dev->device = &dev->device;
-
 			return ret;
 		}
 
@@ -597,7 +600,7 @@  rte_pmd_ring_probe(struct rte_vdev_device *dev)
 			if (ret < 0)
 				goto out_free;
 
-			ret = do_eth_dev_ring_create(name,
+			ret = do_eth_dev_ring_create(name, dev,
 				internal_args->rx_queues,
 				internal_args->nb_rx_queues,
 				internal_args->tx_queues,
@@ -627,6 +630,7 @@  rte_pmd_ring_probe(struct rte_vdev_device *dev)
 
 			for (info->count = 0; info->count < info->total; info->count++) {
 				ret = eth_dev_ring_create(info->list[info->count].name,
+							  dev,
 							  info->list[info->count].node,
 							  info->list[info->count].action,
 							  &eth_dev);
@@ -635,7 +639,7 @@  rte_pmd_ring_probe(struct rte_vdev_device *dev)
 					PMD_LOG(INFO,
 						"Attach to pmd_ring for %s",
 						name);
-					ret = eth_dev_ring_create(name,
+					ret = eth_dev_ring_create(name, dev,
 							info->list[info->count].node,
 							DEV_ATTACH,
 							&eth_dev);
@@ -644,9 +648,6 @@  rte_pmd_ring_probe(struct rte_vdev_device *dev)
 		}
 	}
 
-	if (eth_dev)
-		eth_dev->device = &dev->device;
-
 out_free:
 	rte_kvargs_free(kvlist);
 	rte_free(info);