patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH v1 2/3] net/ring: fix eth_dev device pointer on allocation
       [not found] <cover.1588705694.git.grive@u256.net>
@ 2020-05-05 19:10 ` Gaetan Rivet
  2020-05-06 11:48   ` Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: Gaetan Rivet @ 2020-05-05 19:10 UTC (permalink / raw)
  To: dev; +Cc: stable, ferruh.yigit, thomas

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 attempted to fix this 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")
Cc: ferruh.yigit@intel.com
Cc: thomas@monjalon.net
Signed-off-by: Gaetan Rivet <grive@u256.net>
---
 drivers/net/ring/rte_eth_ring.c | 36 ++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index 41acbc513..ad27f9d89 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -244,6 +244,26 @@ static const struct eth_dev_ops ops = {
 	.mac_addr_add = eth_mac_addr_add,
 };
 
+static int
+dev_name_cmp(const struct rte_device *dev, const void *_name)
+{
+	const char *name = _name;
+
+	return strcmp(dev->name, name);
+}
+
+static struct rte_device *
+ring_device_from_name(const char *name)
+{
+	struct rte_bus *vdev_bus;
+
+	vdev_bus = rte_bus_find_by_name("vdev");
+	if (vdev_bus == NULL)
+		return NULL;
+
+	return vdev_bus->find_device(NULL, dev_name_cmp, name);
+}
+
 static int
 do_eth_dev_ring_create(const char *name,
 		struct rte_ring * const rx_queues[],
@@ -294,6 +314,7 @@ do_eth_dev_ring_create(const char *name,
 	 * - store queue data in internals,
 	 * - store numa_node info in eth_dev_data
 	 * - point eth_dev_data to internals
+	 * - store EAL device in eth_dev,
 	 * - and point eth_dev structure to new eth_dev_data structure
 	 */
 
@@ -325,10 +346,17 @@ do_eth_dev_ring_create(const char *name,
 	data->kdrv = RTE_KDRV_NONE;
 	data->numa_node = numa_node;
 
-	/* finally assign rx and tx ops */
+	/* assign rx and tx ops */
 	eth_dev->rx_pkt_burst = eth_ring_rx;
 	eth_dev->tx_pkt_burst = eth_ring_tx;
 
+	/* finally set the rte_device pointer in eth_dev. */
+	eth_dev->device = ring_device_from_name(name);
+	if (eth_dev->device == NULL) {
+		rte_errno = ENODEV;
+		goto error;
+	}
+
 	rte_eth_dev_probing_finish(eth_dev);
 	*eth_dev_p = eth_dev;
 
@@ -584,9 +612,6 @@ rte_pmd_ring_probe(struct rte_vdev_device *dev)
 							  DEV_ATTACH, &eth_dev);
 			}
 
-			if (eth_dev)
-				eth_dev->device = &dev->device;
-
 			return ret;
 		}
 
@@ -644,9 +669,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);
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [PATCH v1 2/3] net/ring: fix eth_dev device pointer on allocation
  2020-05-05 19:10 ` [dpdk-stable] [PATCH v1 2/3] net/ring: fix eth_dev device pointer on allocation Gaetan Rivet
@ 2020-05-06 11:48   ` Ferruh Yigit
  2020-05-06 12:33     ` Gaëtan Rivet
  0 siblings, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-06 11:48 UTC (permalink / raw)
  To: Gaetan Rivet, dev; +Cc: stable, thomas

On 5/5/2020 8:10 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 attempted to fix this 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")
> Cc: ferruh.yigit@intel.com
> Cc: thomas@monjalon.net
> Signed-off-by: Gaetan Rivet <grive@u256.net>

<...>

> @@ -325,10 +346,17 @@ do_eth_dev_ring_create(const char *name,
>  	data->kdrv = RTE_KDRV_NONE;
>  	data->numa_node = numa_node;
>  
> -	/* finally assign rx and tx ops */
> +	/* assign rx and tx ops */
>  	eth_dev->rx_pkt_burst = eth_ring_rx;
>  	eth_dev->tx_pkt_burst = eth_ring_tx;
>  
> +	/* finally set the rte_device pointer in eth_dev. */
> +	eth_dev->device = ring_device_from_name(name);
> +	if (eth_dev->device == NULL) {
> +		rte_errno = ENODEV;
> +		goto error;
> +	}
> +
>  	rte_eth_dev_probing_finish(eth_dev);
>  	*eth_dev_p = eth_dev;

Why not move the 'rte_eth_dev_probing_finish()' to the 'rte_pmd_ring_probe()',
below where 'eth_dev->device' set?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [PATCH v1 2/3] net/ring: fix eth_dev device pointer on allocation
  2020-05-06 11:48   ` Ferruh Yigit
@ 2020-05-06 12:33     ` Gaëtan Rivet
  2020-05-06 13:43       ` Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: Gaëtan Rivet @ 2020-05-06 12:33 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, stable, thomas

On 06/05/20 12:48 +0100, Ferruh Yigit wrote:
> On 5/5/2020 8:10 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 attempted to fix this 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")
> > Cc: ferruh.yigit@intel.com
> > Cc: thomas@monjalon.net
> > Signed-off-by: Gaetan Rivet <grive@u256.net>
> 
> <...>
> 
> > @@ -325,10 +346,17 @@ do_eth_dev_ring_create(const char *name,
> >  	data->kdrv = RTE_KDRV_NONE;
> >  	data->numa_node = numa_node;
> >  
> > -	/* finally assign rx and tx ops */
> > +	/* assign rx and tx ops */
> >  	eth_dev->rx_pkt_burst = eth_ring_rx;
> >  	eth_dev->tx_pkt_burst = eth_ring_tx;
> >  
> > +	/* finally set the rte_device pointer in eth_dev. */
> > +	eth_dev->device = ring_device_from_name(name);
> > +	if (eth_dev->device == NULL) {
> > +		rte_errno = ENODEV;
> > +		goto error;
> > +	}
> > +
> >  	rte_eth_dev_probing_finish(eth_dev);
> >  	*eth_dev_p = eth_dev;
> 
> Why not move the 'rte_eth_dev_probing_finish()' to the 'rte_pmd_ring_probe()',
> below where 'eth_dev->device' set?

Hi Ferruh,

Sure it would work. The reason I did it this way is two-fold:

  * I disliked having two places where eth_dev->device was conditionally
    set. It makes it harder to read rte_pmd_ring_probe.

  * I was actually thinking, doing this patch, that we should modify
    rte_eth_dev_allocate() to take an rte_device as parameter, as all
    eth_dev are meant to be backed by an rte_device. Keeping this in
    mind, I meant to move writing the pointer closer to the
    rte_eth_dev_allocate() call.

But you are right that it is needlessly verbose, using
vdev_bus->find_device() to do this stuff. I'm ok with changing it as you
described if you prefer.

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [PATCH v1 2/3] net/ring: fix eth_dev device pointer on allocation
  2020-05-06 12:33     ` Gaëtan Rivet
@ 2020-05-06 13:43       ` Ferruh Yigit
  2020-05-06 17:32         ` Gaëtan Rivet
  0 siblings, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-06 13:43 UTC (permalink / raw)
  To: Gaëtan Rivet; +Cc: dev, stable, thomas

On 5/6/2020 1:33 PM, Gaëtan Rivet wrote:
> On 06/05/20 12:48 +0100, Ferruh Yigit wrote:
>> On 5/5/2020 8:10 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 attempted to fix this 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")
>>> Cc: ferruh.yigit@intel.com
>>> Cc: thomas@monjalon.net
>>> Signed-off-by: Gaetan Rivet <grive@u256.net>
>>
>> <...>
>>
>>> @@ -325,10 +346,17 @@ do_eth_dev_ring_create(const char *name,
>>>  	data->kdrv = RTE_KDRV_NONE;
>>>  	data->numa_node = numa_node;
>>>  
>>> -	/* finally assign rx and tx ops */
>>> +	/* assign rx and tx ops */
>>>  	eth_dev->rx_pkt_burst = eth_ring_rx;
>>>  	eth_dev->tx_pkt_burst = eth_ring_tx;
>>>  
>>> +	/* finally set the rte_device pointer in eth_dev. */
>>> +	eth_dev->device = ring_device_from_name(name);
>>> +	if (eth_dev->device == NULL) {
>>> +		rte_errno = ENODEV;
>>> +		goto error;
>>> +	}
>>> +
>>>  	rte_eth_dev_probing_finish(eth_dev);
>>>  	*eth_dev_p = eth_dev;
>>
>> Why not move the 'rte_eth_dev_probing_finish()' to the 'rte_pmd_ring_probe()',
>> below where 'eth_dev->device' set?
> 
> Hi Ferruh,
> 
> Sure it would work. The reason I did it this way is two-fold:
> 
>   * I disliked having two places where eth_dev->device was conditionally
>     set. It makes it harder to read rte_pmd_ring_probe.

Agree, what about using a 'goto' to have the assignment and
'rte_eth_dev_probing_finish()' in a single place?
But check seems needed since creation may failed at that stage, if you think
better check can be done on 'ret' instead of 'eth_dev'...

> 
>   * I was actually thinking, doing this patch, that we should modify
>     rte_eth_dev_allocate() to take an rte_device as parameter, as all
>     eth_dev are meant to be backed by an rte_device. Keeping this in
>     mind, I meant to move writing the pointer closer to the
>     rte_eth_dev_allocate() call.

That is a bigger change, may affect many (if not all) PMDs, so I think this can
be considered when that change is available.

And although that change may fix the issues that 'eth_dev->device' is not set,
which we had a few times before, not sure it worth to change all PMDs and ethdev
API directly couple with rte_device, instead of PMD being the glue. Can be
discussed more on its own patch.

> 
> But you are right that it is needlessly verbose, using
> vdev_bus->find_device() to do this stuff. I'm ok with changing it as you
> described if you prefer.
> 

That was the concern, that is too much code to take a value which is already
available a few level up the stack.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [PATCH v1 2/3] net/ring: fix eth_dev device pointer on allocation
  2020-05-06 13:43       ` Ferruh Yigit
@ 2020-05-06 17:32         ` Gaëtan Rivet
  2020-05-06 18:09           ` [dpdk-stable] [PATCH v2] " Gaetan Rivet
  0 siblings, 1 reply; 8+ messages in thread
From: Gaëtan Rivet @ 2020-05-06 17:32 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, stable, thomas

On 06/05/20 14:43 +0100, Ferruh Yigit wrote:
> On 5/6/2020 1:33 PM, Gaëtan Rivet wrote:
> > On 06/05/20 12:48 +0100, Ferruh Yigit wrote:
> >> On 5/5/2020 8:10 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 attempted to fix this 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")
> >>> Cc: ferruh.yigit@intel.com
> >>> Cc: thomas@monjalon.net
> >>> Signed-off-by: Gaetan Rivet <grive@u256.net>
> >>
> >> <...>
> >>
> >>> @@ -325,10 +346,17 @@ do_eth_dev_ring_create(const char *name,
> >>>  	data->kdrv = RTE_KDRV_NONE;
> >>>  	data->numa_node = numa_node;
> >>>  
> >>> -	/* finally assign rx and tx ops */
> >>> +	/* assign rx and tx ops */
> >>>  	eth_dev->rx_pkt_burst = eth_ring_rx;
> >>>  	eth_dev->tx_pkt_burst = eth_ring_tx;
> >>>  
> >>> +	/* finally set the rte_device pointer in eth_dev. */
> >>> +	eth_dev->device = ring_device_from_name(name);
> >>> +	if (eth_dev->device == NULL) {
> >>> +		rte_errno = ENODEV;
> >>> +		goto error;
> >>> +	}
> >>> +
> >>>  	rte_eth_dev_probing_finish(eth_dev);
> >>>  	*eth_dev_p = eth_dev;
> >>
> >> Why not move the 'rte_eth_dev_probing_finish()' to the 'rte_pmd_ring_probe()',
> >> below where 'eth_dev->device' set?
> > 
> > Hi Ferruh,
> > 
> > Sure it would work. The reason I did it this way is two-fold:
> > 
> >   * I disliked having two places where eth_dev->device was conditionally
> >     set. It makes it harder to read rte_pmd_ring_probe.
> 
> Agree, what about using a 'goto' to have the assignment and
> 'rte_eth_dev_probing_finish()' in a single place?
> But check seems needed since creation may failed at that stage, if you think
> better check can be done on 'ret' instead of 'eth_dev'...
> 
> > 
> >   * I was actually thinking, doing this patch, that we should modify
> >     rte_eth_dev_allocate() to take an rte_device as parameter, as all
> >     eth_dev are meant to be backed by an rte_device. Keeping this in
> >     mind, I meant to move writing the pointer closer to the
> >     rte_eth_dev_allocate() call.
> 
> That is a bigger change, may affect many (if not all) PMDs, so I think this can
> be considered when that change is available.
> 
> And although that change may fix the issues that 'eth_dev->device' is not set,
> which we had a few times before, not sure it worth to change all PMDs and ethdev
> API directly couple with rte_device, instead of PMD being the glue. Can be
> discussed more on its own patch.
> 
> > 
> > But you are right that it is needlessly verbose, using
> > vdev_bus->find_device() to do this stuff. I'm ok with changing it as you
> > described if you prefer.
> > 
> 
> That was the concern, that is too much code to take a value which is already
> available a few level up the stack.

Ok, future-proofing is a bad habit so let's not do it.

I'm not a fan of the goto for the 'happy path' though. Another simple
way would be to bring the vdev pointer with me as we go down the stack.

I will send a v2 shortly, if it's too ugly to move the pointer down I'll
use the goto, or if you tell me you'd prefer it.

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-stable] [PATCH v2] net/ring: fix eth_dev device pointer on allocation
  2020-05-06 17:32         ` Gaëtan Rivet
@ 2020-05-06 18:09           ` Gaetan Rivet
  2020-05-08 11:00             ` Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: Gaetan Rivet @ 2020-05-06 18:09 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: dev, stable

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(-)

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);
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/ring: fix eth_dev device pointer on allocation
  2020-05-06 18:09           ` [dpdk-stable] [PATCH v2] " Gaetan Rivet
@ 2020-05-08 11:00             ` Ferruh Yigit
  2020-05-11 16:54               ` [dpdk-stable] [dpdk-dev] " Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-08 11:00 UTC (permalink / raw)
  To: Gaetan Rivet; +Cc: dev, stable, Bruce Richardson

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>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/ring: fix eth_dev device pointer on allocation
  2020-05-08 11:00             ` Ferruh Yigit
@ 2020-05-11 16:54               ` Ferruh Yigit
  0 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-11 16:54 UTC (permalink / raw)
  To: Gaetan Rivet; +Cc: dev, stable, Bruce Richardson

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-05-11 16:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1588705694.git.grive@u256.net>
2020-05-05 19:10 ` [dpdk-stable] [PATCH v1 2/3] net/ring: fix eth_dev device pointer on allocation Gaetan Rivet
2020-05-06 11:48   ` Ferruh Yigit
2020-05-06 12:33     ` Gaëtan Rivet
2020-05-06 13:43       ` Ferruh Yigit
2020-05-06 17:32         ` Gaëtan Rivet
2020-05-06 18:09           ` [dpdk-stable] [PATCH v2] " Gaetan Rivet
2020-05-08 11:00             ` Ferruh Yigit
2020-05-11 16:54               ` [dpdk-stable] [dpdk-dev] " Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).