DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close
@ 2025-11-06 16:38 Hemant Agrawal
  2025-11-06 16:38 ` [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
                   ` (4 more replies)
  0 siblings, 5 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-06 16:38 UTC (permalink / raw)
  To: dev; +Cc: stephen, Hemant Agrawal, sachin.saxena, stable

When rte_eth_dev_close() is called, it performs the following actions:

Calls dev->dev_ops->dev_close(), which in this case is dpaa2_dev_close().
Then calls rte_eth_dev_release_port(), which releases all device data
and sets dev->data to NULL.

Later, when rte_dev_remove() is called, the FSLMC bus invokes
dev->remove() — that is, rte_dpaa2_remove().
However, rte_dpaa2_remove() calls dpaa2_dev_close() again. Since dev->data
was already set to NULL by the previous call, this second invocation
causes a crash.

Fixes: 5964d36a2904 ("net/dpaa2: release port upon close")
Cc: sachin.saxena@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 7da32ce856..f3db7982a4 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -3347,14 +3347,17 @@ static int
 rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
 {
 	struct rte_eth_dev *eth_dev;
-	int ret;
+	int ret = 0;
 
 	eth_dev = dpaa2_dev->eth_dev;
-	dpaa2_dev_close(eth_dev);
+	if (eth_dev->data) {
+		ret = dpaa2_dev_close(eth_dev);
+		if (!ret)
+			ret = rte_eth_dev_release_port(eth_dev);
+	}
 	dpaa2_valid_dev--;
 	if (!dpaa2_valid_dev)
 		rte_mempool_free(dpaa2_tx_sg_pool);
-	ret = rte_eth_dev_release_port(eth_dev);
 
 	return ret;
 }
-- 
2.25.1


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

* [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues
  2025-11-06 16:38 [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
@ 2025-11-06 16:38 ` Hemant Agrawal
  2025-11-06 19:29   ` Stephen Hemminger
  2025-11-07  8:34   ` David Marchand
  2025-11-06 16:38 ` [PATCH 3/3] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-06 16:38 UTC (permalink / raw)
  To: dev; +Cc: stephen, Maxime Leroy, jun.yang, stable

From: Maxime Leroy <maxime@leroys.fr>

When using the prefetch Rx path (dpaa2_dev_prefetch_rx), the driver keeps
track of one outstanding VDQCR command per DPIO portal in the global
rte_global_active_dqs_list[] array. Each queue_storage_info_t also stores
the active result buffer and portal index:

  qs->active_dqs
  qs->active_dpio_id

Before issuing a new pull command, dpaa2_dev_prefetch_rx() checks for an
active entry and spins on qbman_check_command_complete() until the
corresponding VDQCR completes.

On port close / hotplug remove, dpaa2_free_rx_tx_queues() frees all
per-lcore queue_storage_info_t structures and their dq_storage[] buffers,
but never clears the global rte_global_active_dqs_list[] entries. After a
detach/attach sequence (or "del/add" in grout), the prefetch Rx path
still sees an active entry for the portal and spins forever on a stale dq
buffer that has been freed and will never be completed by hardware. In
gdb, dq->dq.tok stays 0 and dpaa2_dev_prefetch_rx() loops in:

  while (!qbman_check_command_complete(get_swp_active_dqs(idx)))
      ;

Fix this by clearing the active VDQ state before freeing queue storage.
For each Rx queue and lcore, if qs->active_dqs is non-NULL, call
clear_swp_active_dqs(qs->active_dpio_id) and set qs->active_dqs to NULL.
Then dpaa2_queue_storage_free() can safely free q_storage and
dq_storage[].

After this change, a DPNI detach/attach sequence no longer leaves stale
entries in rte_global_active_dqs_list[], and the prefetch Rx loop does
not hang waiting for a completion from a previous device instance.

Reproduction:
  - grout:
      grcli interface add  port dpni.1 devargs fslmc:dpni.1
      grcli interface del  dpni.1
      grcli interface add  port dpni.1 devargs fslmc:dpni.1
    -> Rx was stuck in qbman_check_command_complete(), now works.

  - testpmd:
      dpdk-testpmd -n1 -a fslmc:dpni.65535 -- -i --forward-mode=rxonly
      testpmd> port attach fslmc:dpni.1
      testpmd> port start all
      testpmd> start
      testpmd> stop
      testpmd> port stop all
      testpmd> port detach 0
      testpmd> port attach fslmc:dpni.1
      testpmd> port start all
      testpmd> start
    -> Rx was hanging, now runs normall

Fixes: 12d98eceb8ac ("bus/fslmc: enhance QBMAN DQ storage logic")
Cc: jun.yang@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Maxime Leroy <maxime@leroys.fr>
---
 .mailmap                         |  1 +
 drivers/net/dpaa2/dpaa2_ethdev.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/.mailmap b/.mailmap
index 10c37a97a6..1f540f7f51 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1036,6 +1036,7 @@ Mauro Annarumma <mauroannarumma@hotmail.it>
 Maxime Coquelin <maxime.coquelin@redhat.com>
 Maxime Gouin <maxime.gouin@6wind.com>
 Maxime Leroy <maxime.leroy@6wind.com>
+Maxime Leroy <maxime@leroys.fr>
 Md Fahad Iqbal Polash <md.fahad.iqbal.polash@intel.com>
 Megha Ajmera <megha.ajmera@intel.com>
 Meijuan Zhao <meijuanx.zhao@intel.com>
diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index f3db7982a4..3c18d58804 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -631,6 +631,24 @@ dpaa2_alloc_rx_tx_queues(struct rte_eth_dev *dev)
 	return ret;
 }
 
+static void
+dpaa2_clear_queue_active_dps(struct dpaa2_queue *q, int num_lcores)
+{
+	int i;
+
+	for (i = 0; i < num_lcores; i++) {
+		struct queue_storage_info_t *qs = q->q_storage[i];
+
+		if (!qs)
+			continue;
+
+		if (qs->active_dqs) {
+			clear_swp_active_dqs(qs->active_dpio_id);
+			qs->active_dqs = NULL;
+		}
+	}
+}
+
 static void
 dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 {
@@ -645,6 +663,7 @@ dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 		/* cleaning up queue storage */
 		for (i = 0; i < priv->nb_rx_queues; i++) {
 			dpaa2_q = priv->rx_vq[i];
+			dpaa2_clear_queue_active_dps(dpaa2_q, RTE_MAX_LCORE);
 			dpaa2_queue_storage_free(dpaa2_q,
 				RTE_MAX_LCORE);
 		}
-- 
2.25.1


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

* [PATCH 3/3] bus/fslmc: add support for hotplug of dpni
  2025-11-06 16:38 [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
  2025-11-06 16:38 ` [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
@ 2025-11-06 16:38 ` Hemant Agrawal
  2025-11-07  8:32 ` [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close David Marchand
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-06 16:38 UTC (permalink / raw)
  To: dev; +Cc: stephen, Hemant Agrawal

This patch implements the plug and unplug function to support
attach/detach of dpni interfaces.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/fslmc/fslmc_bus.c | 56 +++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
index 49c61c9d2d..4529ec5085 100644
--- a/drivers/bus/fslmc/fslmc_bus.c
+++ b/drivers/bus/fslmc/fslmc_bus.c
@@ -589,17 +589,61 @@ rte_dpaa2_get_iommu_class(void)
 }
 
 static int
-fslmc_bus_plug(struct rte_device *dev __rte_unused)
+fslmc_bus_plug(struct rte_device *rte_dev)
 {
-	/* No operation is performed while plugging the device */
-	return 0;
+	int ret = 0;
+	struct rte_dpaa2_device *dev = container_of(rte_dev,
+			struct rte_dpaa2_device, device);
+	struct rte_dpaa2_driver *drv;
+
+	TAILQ_FOREACH(drv, &rte_fslmc_bus.driver_list, next) {
+		ret = rte_fslmc_match(drv, dev);
+		if (ret)
+			continue;
+
+		if (!drv->probe)
+			continue;
+
+		if (rte_dev_is_probed(&dev->device))
+			continue;
+
+		if (dev->device.devargs &&
+		    dev->device.devargs->policy == RTE_DEV_BLOCKED) {
+			DPAA2_BUS_DEBUG("%s Blocked, skipping",
+				      dev->device.name);
+			continue;
+		}
+
+		ret = drv->probe(drv, dev);
+		if (ret) {
+			DPAA2_BUS_ERR("Unable to probe");
+		} else {
+			dev->driver = drv;
+			dev->device.driver = &drv->driver;
+			DPAA2_BUS_INFO("%s Plugged",  dev->device.name);
+		}
+		break;
+	}
+
+	return ret;
 }
 
 static int
-fslmc_bus_unplug(struct rte_device *dev __rte_unused)
+fslmc_bus_unplug(struct rte_device *rte_dev)
 {
-	/* No operation is performed while unplugging the device */
-	return 0;
+	struct rte_dpaa2_device *dev = container_of(rte_dev,
+			struct rte_dpaa2_device, device);
+	struct rte_dpaa2_driver *drv = dev->driver;
+
+	if (drv && drv->remove) {
+		drv->remove(dev);
+		dev->driver = NULL;
+		dev->device.driver = NULL;
+		DPAA2_BUS_INFO("%s Un-Plugged",  dev->device.name);
+		return 0;
+	}
+
+	return -ENODEV;
 }
 
 static void *
-- 
2.25.1


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

* Re: [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues
  2025-11-06 16:38 ` [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
@ 2025-11-06 19:29   ` Stephen Hemminger
  2025-11-07  9:51     ` Maxime Leroy
  2025-11-07  8:34   ` David Marchand
  1 sibling, 1 reply; 28+ messages in thread
From: Stephen Hemminger @ 2025-11-06 19:29 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: dev, Maxime Leroy, jun.yang, stable

On Thu,  6 Nov 2025 22:08:06 +0530
Hemant Agrawal <hemant.agrawal@nxp.com> wrote:

> +static void
> +dpaa2_clear_queue_active_dps(struct dpaa2_queue *q, int num_lcores)
> +{
> +	int i;
> +
> +	for (i = 0; i < num_lcores; i++) {
> +		struct queue_storage_info_t *qs = q->q_storage[i];
> +
> +		if (!qs)
> +			continue;
> +
> +		if (qs->active_dqs) {
> +			clear_swp_active_dqs(qs->active_dpio_id);
> +			qs->active_dqs = NULL;
> +		}
> +	}
> +}
> +

Why not use RTE_LCORE_FOREACH() here?

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

* Re: [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close
  2025-11-06 16:38 [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
  2025-11-06 16:38 ` [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
  2025-11-06 16:38 ` [PATCH 3/3] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
@ 2025-11-07  8:32 ` David Marchand
  2025-11-08 15:35   ` David Marchand
  2025-11-12 23:06 ` Stephen Hemminger
  2025-11-13  5:29 ` [PATCH v2 1/5] " Hemant Agrawal
  4 siblings, 1 reply; 28+ messages in thread
From: David Marchand @ 2025-11-07  8:32 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: dev, stephen, sachin.saxena, stable, maxime

Hello,

On Thu, 6 Nov 2025 at 17:38, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>
> When rte_eth_dev_close() is called, it performs the following actions:
>
> Calls dev->dev_ops->dev_close(), which in this case is dpaa2_dev_close().
> Then calls rte_eth_dev_release_port(), which releases all device data
> and sets dev->data to NULL.
>
> Later, when rte_dev_remove() is called, the FSLMC bus invokes
> dev->remove() — that is, rte_dpaa2_remove().
> However, rte_dpaa2_remove() calls dpaa2_dev_close() again. Since dev->data
> was already set to NULL by the previous call, this second invocation
> causes a crash.
>
> Fixes: 5964d36a2904 ("net/dpaa2: release port upon close")
> Cc: sachin.saxena@nxp.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>  drivers/net/dpaa2/dpaa2_ethdev.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> index 7da32ce856..f3db7982a4 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -3347,14 +3347,17 @@ static int
>  rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
>  {
>         struct rte_eth_dev *eth_dev;
> -       int ret;
> +       int ret = 0;
>
>         eth_dev = dpaa2_dev->eth_dev;

Having a back reference of the "class" object in a "device" object
seems wrong to me (and there is a dev_priv->eth_dev too...).
It breaks the separation that was introduced with rte_device years ago.

I did not look in detail, but it seems strange that after closing a
first time, there would still remain a reference of the eth_dev object
in the dpaa2 device object.
At least, it would be worth double checking that the
dpaa2_dev->eth_dev is cleared in dpaa2_dev_close.


> -       dpaa2_dev_close(eth_dev);
> +       if (eth_dev->data) {
> +               ret = dpaa2_dev_close(eth_dev);
> +               if (!ret)
> +                       ret = rte_eth_dev_release_port(eth_dev);

I don't see why you would need to decrement dpaa2_valid_dev again below.
Maybe a missing return here?


> +       }
>         dpaa2_valid_dev--;
>         if (!dpaa2_valid_dev)
>                 rte_mempool_free(dpaa2_tx_sg_pool);
> -       ret = rte_eth_dev_release_port(eth_dev);
>
>         return ret;
>  }


Taking a step back, the issue this patch wants to fix is a pattern
that is resolved by other drivers by checking if a eth_dev is
allocated for a rte_device.
A simpler (untested) fix seems to be:

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 7da32ce856..6682a72341 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -3349,7 +3349,10 @@ rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
        struct rte_eth_dev *eth_dev;
        int ret;

-       eth_dev = dpaa2_dev->eth_dev;
+       eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
+       if (!eth_dev)
+               return 0;
+
        dpaa2_dev_close(eth_dev);
        dpaa2_valid_dev--;
        if (!dpaa2_valid_dev)


-- 
David Marchand


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

* Re: [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues
  2025-11-06 16:38 ` [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
  2025-11-06 19:29   ` Stephen Hemminger
@ 2025-11-07  8:34   ` David Marchand
  1 sibling, 0 replies; 28+ messages in thread
From: David Marchand @ 2025-11-07  8:34 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: dev, stephen, Maxime Leroy, jun.yang, stable

On Thu, 6 Nov 2025 at 17:38, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> diff --git a/.mailmap b/.mailmap
> index 10c37a97a6..1f540f7f51 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1036,6 +1036,7 @@ Mauro Annarumma <mauroannarumma@hotmail.it>
>  Maxime Coquelin <maxime.coquelin@redhat.com>
>  Maxime Gouin <maxime.gouin@6wind.com>
>  Maxime Leroy <maxime.leroy@6wind.com>
> +Maxime Leroy <maxime@leroys.fr>

On a single line please:
Maxime Leroy <maxime@leroys.fr> <maxime.leroy@6wind.com>

>  Md Fahad Iqbal Polash <md.fahad.iqbal.polash@intel.com>
>  Megha Ajmera <megha.ajmera@intel.com>
>  Meijuan Zhao <meijuanx.zhao@intel.com>


-- 
David Marchand


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

* Re: [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues
  2025-11-06 19:29   ` Stephen Hemminger
@ 2025-11-07  9:51     ` Maxime Leroy
  2025-11-07 10:38       ` Hemant Agrawal
  0 siblings, 1 reply; 28+ messages in thread
From: Maxime Leroy @ 2025-11-07  9:51 UTC (permalink / raw)
  To: stephen; +Cc: Hemant Agrawal, dev, jun.yang, stable

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]

Le jeu. 6 nov. 2025, 20:29, Stephen Hemminger <stephen@networkplumber.org>
a écrit :

> On Thu,  6 Nov 2025 22:08:06 +0530
> Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>
> > +static void
> > +dpaa2_clear_queue_active_dps(struct dpaa2_queue *q, int num_lcores)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < num_lcores; i++) {
> > +             struct queue_storage_info_t *qs = q->q_storage[i];
> > +
> > +             if (!qs)
> > +                     continue;
> > +
> > +             if (qs->active_dqs) {
> > +                     clear_swp_active_dqs(qs->active_dpio_id);
> > +                     qs->active_dqs = NULL;
> > +             }
> > +     }
> > +}
> > +
>
> Why not use RTE_LCORE_FOREACH() here?
>

For the loop, I did it the same way as in dpaa2_queue_storage_alloc(), to
stay aligned with the rest of the driver instead of using
RTE_LCORE_FOREACH().

Thanks Hemant for upstreaming my patch. However, you added my email in
.mailmap as a new entry — it should instead be added as a second email
under the existing Maxime Leroy entry.

>

[-- Attachment #2: Type: text/html, Size: 1950 bytes --]

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

* RE: [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues
  2025-11-07  9:51     ` Maxime Leroy
@ 2025-11-07 10:38       ` Hemant Agrawal
  0 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-07 10:38 UTC (permalink / raw)
  To: Maxime Leroy, stephen; +Cc: dev, Jun Yang, stable

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]


Le jeu. 6 nov. 2025, 20:29, Stephen Hemminger <stephen@networkplumber.org<mailto:stephen@networkplumber.org>> a écrit :
On Thu,  6 Nov 2025 22:08:06 +0530
Hemant Agrawal <hemant.agrawal@nxp.com<mailto:hemant.agrawal@nxp.com>> wrote:

> +static void
> +dpaa2_clear_queue_active_dps(struct dpaa2_queue *q, int num_lcores)
> +{
> +     int i;
> +
> +     for (i = 0; i < num_lcores; i++) {
> +             struct queue_storage_info_t *qs = q->q_storage[i];
> +
> +             if (!qs)
> +                     continue;
> +
> +             if (qs->active_dqs) {
> +                     clear_swp_active_dqs(qs->active_dpio_id);
> +                     qs->active_dqs = NULL;
> +             }
> +     }
> +}
> +

Why not use RTE_LCORE_FOREACH() here?

For the loop, I did it the same way as in dpaa2_queue_storage_alloc(), to stay aligned with the rest of the driver instead of using RTE_LCORE_FOREACH().

Thanks Hemant for upstreaming my patch. However, you added my email in .mailmap as a new entry — it should instead be added as a second email under the existing Maxime Leroy entry.
[Hemant]
Will send v2

[-- Attachment #2: Type: text/html, Size: 4290 bytes --]

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

* Re: [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close
  2025-11-07  8:32 ` [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close David Marchand
@ 2025-11-08 15:35   ` David Marchand
  0 siblings, 0 replies; 28+ messages in thread
From: David Marchand @ 2025-11-08 15:35 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: dev, stephen, sachin.saxena, stable, maxime

On Fri, 7 Nov 2025 at 09:32, David Marchand <david.marchand@redhat.com> wrote:
> > diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> > index 7da32ce856..f3db7982a4 100644
> > --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> > +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> > @@ -3347,14 +3347,17 @@ static int
> >  rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
> >  {
> >         struct rte_eth_dev *eth_dev;
> > -       int ret;
> > +       int ret = 0;
> >
> >         eth_dev = dpaa2_dev->eth_dev;
>
> Having a back reference of the "class" object in a "device" object
> seems wrong to me (and there is a dev_priv->eth_dev too...).
> It breaks the separation that was introduced with rte_device years ago.

I went and had a try:
https://patchwork.dpdk.org/project/dpdk/list/?series=36627

No promise it works, but it is cleaner this way.


-- 
David Marchand


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

* Re: [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close
  2025-11-06 16:38 [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
                   ` (2 preceding siblings ...)
  2025-11-07  8:32 ` [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close David Marchand
@ 2025-11-12 23:06 ` Stephen Hemminger
  2025-11-13  5:29 ` [PATCH v2 1/5] " Hemant Agrawal
  4 siblings, 0 replies; 28+ messages in thread
From: Stephen Hemminger @ 2025-11-12 23:06 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: dev, sachin.saxena, stable

On Thu,  6 Nov 2025 22:08:05 +0530
Hemant Agrawal <hemant.agrawal@nxp.com> wrote:

> When rte_eth_dev_close() is called, it performs the following actions:
> 
> Calls dev->dev_ops->dev_close(), which in this case is dpaa2_dev_close().
> Then calls rte_eth_dev_release_port(), which releases all device data
> and sets dev->data to NULL.
> 
> Later, when rte_dev_remove() is called, the FSLMC bus invokes
> dev->remove() — that is, rte_dpaa2_remove().
> However, rte_dpaa2_remove() calls dpaa2_dev_close() again. Since dev->data
> was already set to NULL by the previous call, this second invocation
> causes a crash.
> 
> Fixes: 5964d36a2904 ("net/dpaa2: release port upon close")
> Cc: sachin.saxena@nxp.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

Not merging this now, because of the feedback about how this driver
is interacting with bus. Either need an ACK or new version.

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

* [PATCH v2 1/5] net/dpaa2: fix duplicate calling of dpaa2 dev close
  2025-11-06 16:38 [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
                   ` (3 preceding siblings ...)
  2025-11-12 23:06 ` Stephen Hemminger
@ 2025-11-13  5:29 ` Hemant Agrawal
  2025-11-13  5:29   ` [PATCH v2 2/5] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
                     ` (4 more replies)
  4 siblings, 5 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13  5:29 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime; +Cc: sachin.saxena, stable

When rte_eth_dev_close() is called, it performs the following actions:

Calls dev->dev_ops->dev_close(), which in this case is dpaa2_dev_close().
Then calls rte_eth_dev_release_port(), which releases all device data
and sets dev->data to NULL.

Later, when rte_dev_remove() is called, the FSLMC bus invokes
dev->remove() — that is, rte_dpaa2_remove().
However, rte_dpaa2_remove() calls dpaa2_dev_close() again. Since dev->data
was already set to NULL by the previous call, this second invocation
causes a crash.

Fixes: 5964d36a2904 ("net/dpaa2: release port upon close")
Cc: sachin.saxena@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 7da32ce856..fc63cf4f09 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -3347,14 +3347,19 @@ static int
 rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
 {
 	struct rte_eth_dev *eth_dev;
-	int ret;
+	int ret = 0;
+
+	eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
+	if (eth_dev) {
+		dpaa2_dev_close(eth_dev);
+		ret = rte_eth_dev_release_port(eth_dev);
+	}
 
-	eth_dev = dpaa2_dev->eth_dev;
-	dpaa2_dev_close(eth_dev);
 	dpaa2_valid_dev--;
-	if (!dpaa2_valid_dev)
+	if (!dpaa2_valid_dev) {
 		rte_mempool_free(dpaa2_tx_sg_pool);
-	ret = rte_eth_dev_release_port(eth_dev);
+		dpaa2_tx_sg_pool = NULL;
+	}
 
 	return ret;
 }
-- 
2.25.1


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

* [PATCH v2 2/5] net/dpaa2: clear active VDQ state when freeing Rx queues
  2025-11-13  5:29 ` [PATCH v2 1/5] " Hemant Agrawal
@ 2025-11-13  5:29   ` Hemant Agrawal
  2025-11-13  5:29   ` [PATCH v2 3/5] net/dpaa2: fix queue free cleanup Hemant Agrawal
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13  5:29 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime; +Cc: jun.yang, stable

From: Maxime Leroy <maxime@leroys.fr>

When using the prefetch Rx path (dpaa2_dev_prefetch_rx), the driver keeps
track of one outstanding VDQCR command per DPIO portal in the global
rte_global_active_dqs_list[] array. Each queue_storage_info_t also stores
the active result buffer and portal index:

  qs->active_dqs
  qs->active_dpio_id

Before issuing a new pull command, dpaa2_dev_prefetch_rx() checks for an
active entry and spins on qbman_check_command_complete() until the
corresponding VDQCR completes.

On port close / hotplug remove, dpaa2_free_rx_tx_queues() frees all
per-lcore queue_storage_info_t structures and their dq_storage[] buffers,
but never clears the global rte_global_active_dqs_list[] entries. After a
detach/attach sequence (or "del/add" in grout), the prefetch Rx path
still sees an active entry for the portal and spins forever on a stale dq
buffer that has been freed and will never be completed by hardware. In
gdb, dq->dq.tok stays 0 and dpaa2_dev_prefetch_rx() loops in:

  while (!qbman_check_command_complete(get_swp_active_dqs(idx)))
      ;

Fix this by clearing the active VDQ state before freeing queue storage.
For each Rx queue and lcore, if qs->active_dqs is non-NULL, call
clear_swp_active_dqs(qs->active_dpio_id) and set qs->active_dqs to NULL.
Then dpaa2_queue_storage_free() can safely free q_storage and
dq_storage[].

After this change, a DPNI detach/attach sequence no longer leaves stale
entries in rte_global_active_dqs_list[], and the prefetch Rx loop does
not hang waiting for a completion from a previous device instance.

Reproduction:
  - grout:
      grcli interface add  port dpni.1 devargs fslmc:dpni.1
      grcli interface del  dpni.1
      grcli interface add  port dpni.1 devargs fslmc:dpni.1
    -> Rx was stuck in qbman_check_command_complete(), now works.

  - testpmd:
      dpdk-testpmd -n1 -a fslmc:dpni.65535 -- -i --forward-mode=rxonly
      testpmd> port attach fslmc:dpni.1
      testpmd> port start all
      testpmd> start
      testpmd> stop
      testpmd> port stop all
      testpmd> port detach 0
      testpmd> port attach fslmc:dpni.1
      testpmd> port start all
      testpmd> start
    -> Rx was hanging, now runs normal

Fixes: 12d98eceb8ac ("bus/fslmc: enhance QBMAN DQ storage logic")
Cc: jun.yang@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Maxime Leroy <maxime@leroys.fr>
---
 .mailmap                         |  2 +-
 drivers/net/dpaa2/dpaa2_ethdev.c | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index 10c37a97a6..d92d4fc24b 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1035,7 +1035,7 @@ Mauricio Vasquez B <mauricio.vasquez@polito.it> <mauricio.vasquezbernal@studenti
 Mauro Annarumma <mauroannarumma@hotmail.it>
 Maxime Coquelin <maxime.coquelin@redhat.com>
 Maxime Gouin <maxime.gouin@6wind.com>
-Maxime Leroy <maxime.leroy@6wind.com>
+Maxime Leroy <maxime.leroy@6wind.com> <maxime@leroys.fr>
 Md Fahad Iqbal Polash <md.fahad.iqbal.polash@intel.com>
 Megha Ajmera <megha.ajmera@intel.com>
 Meijuan Zhao <meijuanx.zhao@intel.com>
diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index fc63cf4f09..c94034104a 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -631,6 +631,27 @@ dpaa2_alloc_rx_tx_queues(struct rte_eth_dev *dev)
 	return ret;
 }
 
+static void
+dpaa2_clear_queue_active_dps(struct dpaa2_queue *q)
+{
+	int lcore_id;
+
+	RTE_LCORE_FOREACH(lcore_id) {
+		struct queue_storage_info_t *qs = q->q_storage[lcore_id];
+
+		if (!qs)
+			continue;
+
+		if (qs->active_dqs) {
+			while (!qbman_check_command_complete(qs->active_dqs))
+				/* wait */;
+
+			clear_swp_active_dqs(qs->active_dpio_id);
+			qs->active_dqs = NULL;
+		}
+	}
+}
+
 static void
 dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 {
@@ -645,6 +666,7 @@ dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 		/* cleaning up queue storage */
 		for (i = 0; i < priv->nb_rx_queues; i++) {
 			dpaa2_q = priv->rx_vq[i];
+			dpaa2_clear_queue_active_dps(dpaa2_q);
 			dpaa2_queue_storage_free(dpaa2_q,
 				RTE_MAX_LCORE);
 		}
-- 
2.25.1


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

* [PATCH v2 3/5] net/dpaa2: fix queue free cleanup
  2025-11-13  5:29 ` [PATCH v2 1/5] " Hemant Agrawal
  2025-11-13  5:29   ` [PATCH v2 2/5] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
@ 2025-11-13  5:29   ` Hemant Agrawal
  2025-11-13  5:29   ` [PATCH v2 4/5] net/dpaa2: bifurcate close into close and deinit Hemant Agrawal
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13  5:29 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime; +Cc: hemant.agrawal

The RX error queue was not being free.
Also, set the free queues pointers to NULL.

Fixes: 407ce3e5384b ("net/dpaa2: replace global variables with flags")
Cc: hemant.agrawal@nxp.com

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index c94034104a..f82c50341d 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -669,11 +669,13 @@ dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 			dpaa2_clear_queue_active_dps(dpaa2_q);
 			dpaa2_queue_storage_free(dpaa2_q,
 				RTE_MAX_LCORE);
+			priv->rx_vq[i] = NULL;
 		}
 		/* cleanup tx queue cscn */
 		for (i = 0; i < priv->nb_tx_queues; i++) {
 			dpaa2_q = priv->tx_vq[i];
 			rte_free(dpaa2_q->cscn);
+			priv->tx_vq[i] = NULL;
 		}
 		if (priv->flags & DPAA2_TX_CONF_ENABLE) {
 			/* cleanup tx conf queue storage */
@@ -681,8 +683,14 @@ dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 				dpaa2_q = priv->tx_conf_vq[i];
 				dpaa2_queue_storage_free(dpaa2_q,
 					RTE_MAX_LCORE);
+				priv->tx_conf_vq[i] = NULL;
 			}
 		}
+		if (priv->flags & DPAAX_RX_ERROR_QUEUE_FLAG) {
+			dpaa2_q = priv->rx_err_vq;
+			dpaa2_queue_storage_free(dpaa2_q, RTE_MAX_LCORE);
+		}
+
 		/*free memory for all queues (RX+TX) */
 		rte_free(priv->rx_vq[0]);
 		priv->rx_vq[0] = NULL;
-- 
2.25.1


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

* [PATCH v2 4/5] net/dpaa2: bifurcate close into close and deinit
  2025-11-13  5:29 ` [PATCH v2 1/5] " Hemant Agrawal
  2025-11-13  5:29   ` [PATCH v2 2/5] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
  2025-11-13  5:29   ` [PATCH v2 3/5] net/dpaa2: fix queue free cleanup Hemant Agrawal
@ 2025-11-13  5:29   ` Hemant Agrawal
  2025-11-13  9:48     ` Maxime Leroy
  2025-11-13  9:49     ` Maxime Leroy
  2025-11-13  5:29   ` [PATCH v2 5/5] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
  2025-11-13  9:59   ` [PATCH v3 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
  4 siblings, 2 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13  5:29 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime

The close function was overloaded to be also used as deinit function.
This can cause issues when using functions like hotplug.

This patch cleans up the code and implement separate close and deinit

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 92 ++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 41 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index f82c50341d..0cd875d715 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -1524,53 +1524,14 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)
 static int
 dpaa2_dev_close(struct rte_eth_dev *dev)
 {
-	struct dpaa2_dev_priv *priv = dev->data->dev_private;
-	struct fsl_mc_io *dpni = dev->process_private;
-	int i, ret;
-	struct rte_eth_link link;
-
 	PMD_INIT_FUNC_TRACE();
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
-	if (!dpni) {
-		DPAA2_PMD_WARN("Already closed or not started");
-		return -EINVAL;
-	}
-
 	dpaa2_tm_deinit(dev);
 	dpaa2_flow_clean(dev);
-	/* Clean the device first */
-	ret = dpni_reset(dpni, CMD_PRI_LOW, priv->token);
-	if (ret) {
-		DPAA2_PMD_ERR("Failure cleaning dpni device: err=%d", ret);
-		return ret;
-	}
-
-	memset(&link, 0, sizeof(link));
-	rte_eth_linkstatus_set(dev, &link);
-
-	/* Free private queues memory */
-	dpaa2_free_rx_tx_queues(dev);
-	/* Close the device at underlying layer*/
-	ret = dpni_close(dpni, CMD_PRI_LOW, priv->token);
-	if (ret) {
-		DPAA2_PMD_ERR("Failure closing dpni device with err code %d",
-			ret);
-	}
-
-	/* Free the allocated memory for ethernet private data and dpni*/
-	priv->hw = NULL;
-	dev->process_private = NULL;
-	rte_free(dpni);
 
-	for (i = 0; i < MAX_TCS; i++)
-		rte_free(priv->extract.tc_extract_param[i]);
-
-	rte_free(priv->extract.qos_extract_param);
-
-	DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
 	return 0;
 }
 
@@ -2891,6 +2852,55 @@ dpaa2_get_devargs(struct rte_devargs *devargs, const char *key)
 	return 1;
 }
 
+static int
+dpaa2_dev_deinit(struct rte_eth_dev *dev)
+{
+	struct dpaa2_dev_priv *priv = dev->data->dev_private;
+	struct fsl_mc_io *dpni = dev->process_private;
+	int i, ret;
+	struct rte_eth_link link;
+
+	PMD_INIT_FUNC_TRACE();
+
+	if (!dpni) {
+		DPAA2_PMD_WARN("Already closed or not started");
+		return -EINVAL;
+	}
+
+	/* Clean the device first */
+	ret = dpni_reset(dpni, CMD_PRI_LOW, priv->token);
+	if (ret) {
+		DPAA2_PMD_ERR("Failure cleaning dpni device: err=%d", ret);
+		return ret;
+	}
+
+	memset(&link, 0, sizeof(link));
+	rte_eth_linkstatus_set(dev, &link);
+
+	/* Free private queues memory */
+	dpaa2_free_rx_tx_queues(dev);
+	/* Close the device at underlying layer*/
+	ret = dpni_close(dpni, CMD_PRI_LOW, priv->token);
+	if (ret) {
+		DPAA2_PMD_ERR("Failure closing dpni device with err code %d",
+			ret);
+	}
+
+	/* Free the allocated memory for ethernet private data and dpni*/
+	priv->hw = NULL;
+	dev->process_private = NULL;
+	rte_free(dpni);
+
+	for (i = 0; i < MAX_TCS; i++)
+		rte_free(priv->extract.tc_extract_param[i]);
+
+	rte_free(priv->extract.qos_extract_param);
+
+	DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
+	return 0;
+}
+
+
 static int
 dpaa2_dev_init(struct rte_eth_dev *eth_dev)
 {
@@ -3177,7 +3187,7 @@ dpaa2_dev_init(struct rte_eth_dev *eth_dev)
 
 	return 0;
 init_err:
-	dpaa2_dev_close(eth_dev);
+	dpaa2_dev_deinit(eth_dev);
 
 	return ret;
 }
@@ -3381,7 +3391,7 @@ rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
 
 	eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
 	if (eth_dev) {
-		dpaa2_dev_close(eth_dev);
+		dpaa2_dev_deinit(eth_dev);
 		ret = rte_eth_dev_release_port(eth_dev);
 	}
 
-- 
2.25.1


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

* [PATCH v2 5/5] bus/fslmc: add support for hotplug of dpni
  2025-11-13  5:29 ` [PATCH v2 1/5] " Hemant Agrawal
                     ` (2 preceding siblings ...)
  2025-11-13  5:29   ` [PATCH v2 4/5] net/dpaa2: bifurcate close into close and deinit Hemant Agrawal
@ 2025-11-13  5:29   ` Hemant Agrawal
  2025-11-13  9:59   ` [PATCH v3 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
  4 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13  5:29 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime

This patch implements the plug and unplug function to support
attach/detach of dpni interfaces.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/fslmc/fslmc_bus.c | 56 +++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
index 49c61c9d2d..4529ec5085 100644
--- a/drivers/bus/fslmc/fslmc_bus.c
+++ b/drivers/bus/fslmc/fslmc_bus.c
@@ -589,17 +589,61 @@ rte_dpaa2_get_iommu_class(void)
 }
 
 static int
-fslmc_bus_plug(struct rte_device *dev __rte_unused)
+fslmc_bus_plug(struct rte_device *rte_dev)
 {
-	/* No operation is performed while plugging the device */
-	return 0;
+	int ret = 0;
+	struct rte_dpaa2_device *dev = container_of(rte_dev,
+			struct rte_dpaa2_device, device);
+	struct rte_dpaa2_driver *drv;
+
+	TAILQ_FOREACH(drv, &rte_fslmc_bus.driver_list, next) {
+		ret = rte_fslmc_match(drv, dev);
+		if (ret)
+			continue;
+
+		if (!drv->probe)
+			continue;
+
+		if (rte_dev_is_probed(&dev->device))
+			continue;
+
+		if (dev->device.devargs &&
+		    dev->device.devargs->policy == RTE_DEV_BLOCKED) {
+			DPAA2_BUS_DEBUG("%s Blocked, skipping",
+				      dev->device.name);
+			continue;
+		}
+
+		ret = drv->probe(drv, dev);
+		if (ret) {
+			DPAA2_BUS_ERR("Unable to probe");
+		} else {
+			dev->driver = drv;
+			dev->device.driver = &drv->driver;
+			DPAA2_BUS_INFO("%s Plugged",  dev->device.name);
+		}
+		break;
+	}
+
+	return ret;
 }
 
 static int
-fslmc_bus_unplug(struct rte_device *dev __rte_unused)
+fslmc_bus_unplug(struct rte_device *rte_dev)
 {
-	/* No operation is performed while unplugging the device */
-	return 0;
+	struct rte_dpaa2_device *dev = container_of(rte_dev,
+			struct rte_dpaa2_device, device);
+	struct rte_dpaa2_driver *drv = dev->driver;
+
+	if (drv && drv->remove) {
+		drv->remove(dev);
+		dev->driver = NULL;
+		dev->device.driver = NULL;
+		DPAA2_BUS_INFO("%s Un-Plugged",  dev->device.name);
+		return 0;
+	}
+
+	return -ENODEV;
 }
 
 static void *
-- 
2.25.1


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

* Re: [PATCH v2 4/5] net/dpaa2: bifurcate close into close and deinit
  2025-11-13  5:29   ` [PATCH v2 4/5] net/dpaa2: bifurcate close into close and deinit Hemant Agrawal
@ 2025-11-13  9:48     ` Maxime Leroy
  2025-11-13  9:49     ` Maxime Leroy
  1 sibling, 0 replies; 28+ messages in thread
From: Maxime Leroy @ 2025-11-13  9:48 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: dev, stephen, david.marchand

Hi Hemant,

Le jeu. 13 nov. 2025 à 06:30, Hemant Agrawal <hemant.agrawal@nxp.com> a écrit :
>
> The close function was overloaded to be also used as deinit function.
> This can cause issues when using functions like hotplug.
>
> This patch cleans up the code and implement separate close and deinit
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>  drivers/net/dpaa2/dpaa2_ethdev.c | 92 ++++++++++++++++++--------------
>  1 file changed, 51 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> index f82c50341d..0cd875d715 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -1524,53 +1524,14 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)
>  static int
>  dpaa2_dev_close(struct rte_eth_dev *dev)
>  {
> -       struct dpaa2_dev_priv *priv = dev->data->dev_private;
> -       struct fsl_mc_io *dpni = dev->process_private;
> -       int i, ret;
> -       struct rte_eth_link link;
> -
>         PMD_INIT_FUNC_TRACE();
>
>         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>                 return 0;
>
> -       if (!dpni) {
> -               DPAA2_PMD_WARN("Already closed or not started");
> -               return -EINVAL;
> -       }
> -
>         dpaa2_tm_deinit(dev);
>         dpaa2_flow_clean(dev);
> -       /* Clean the device first */
> -       ret = dpni_reset(dpni, CMD_PRI_LOW, priv->token);
> -       if (ret) {
> -               DPAA2_PMD_ERR("Failure cleaning dpni device: err=%d", ret);
> -               return ret;
> -       }
> -
> -       memset(&link, 0, sizeof(link));
> -       rte_eth_linkstatus_set(dev, &link);
> -
> -       /* Free private queues memory */
> -       dpaa2_free_rx_tx_queues(dev);
> -       /* Close the device at underlying layer*/
> -       ret = dpni_close(dpni, CMD_PRI_LOW, priv->token);
> -       if (ret) {
> -               DPAA2_PMD_ERR("Failure closing dpni device with err code %d",
> -                       ret);
> -       }
> -
> -       /* Free the allocated memory for ethernet private data and dpni*/
> -       priv->hw = NULL;
> -       dev->process_private = NULL;
> -       rte_free(dpni);
>
> -       for (i = 0; i < MAX_TCS; i++)
> -               rte_free(priv->extract.tc_extract_param[i]);
> -
> -       rte_free(priv->extract.qos_extract_param);
> -
> -       DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
>         return 0;
>  }
>
> @@ -2891,6 +2852,55 @@ dpaa2_get_devargs(struct rte_devargs *devargs, const char *key)
>         return 1;
>  }
>
> +static int
> +dpaa2_dev_deinit(struct rte_eth_dev *dev)
> +{
> +       struct dpaa2_dev_priv *priv = dev->data->dev_private;
> +       struct fsl_mc_io *dpni = dev->process_private;
> +       int i, ret;
> +       struct rte_eth_link link;
> +
> +       PMD_INIT_FUNC_TRACE();
> +
> +       if (!dpni) {
> +               DPAA2_PMD_WARN("Already closed or not started");
> +               return -EINVAL;
> +       }
> +
> +       /* Clean the device first */
> +       ret = dpni_reset(dpni, CMD_PRI_LOW, priv->token);
> +       if (ret) {
> +               DPAA2_PMD_ERR("Failure cleaning dpni device: err=%d", ret);
> +               return ret;
> +       }
> +
> +       memset(&link, 0, sizeof(link));
> +       rte_eth_linkstatus_set(dev, &link);
> +
> +       /* Free private queues memory */
> +       dpaa2_free_rx_tx_queues(dev);
> +       /* Close the device at underlying layer*/
> +       ret = dpni_close(dpni, CMD_PRI_LOW, priv->token);
> +       if (ret) {
> +               DPAA2_PMD_ERR("Failure closing dpni device with err code %d",
> +                       ret);
> +       }
> +
> +       /* Free the allocated memory for ethernet private data and dpni*/
> +       priv->hw = NULL;
> +       dev->process_private = NULL;
> +       rte_free(dpni);
> +
> +       for (i = 0; i < MAX_TCS; i++)
> +               rte_free(priv->extract.tc_extract_param[i]);
> +
> +       rte_free(priv->extract.qos_extract_param);
> +
> +       DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
> +       return 0;
> +}
> +
> +
>  static int
>  dpaa2_dev_init(struct rte_eth_dev *eth_dev)
>  {
> @@ -3177,7 +3187,7 @@ dpaa2_dev_init(struct rte_eth_dev *eth_dev)
>
>         return 0;
>  init_err:
> -       dpaa2_dev_close(eth_dev);
> +       dpaa2_dev_deinit(eth_dev);
>
>         return ret;
>  }
> @@ -3381,7 +3391,7 @@ rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
>
>         eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
>         if (eth_dev) {
> -               dpaa2_dev_close(eth_dev);
> +               dpaa2_dev_deinit(eth_dev);
>                 ret = rte_eth_dev_release_port(eth_dev);
>         }
>

When a DPDK application stops, dpaa2_dev_close is no longer called.
This happens because eal_cleanup() invokes rte_fslmc_close(), which
then calls the remove function (rte_dpaa2_remove) for each DPAA2
Ethernet driver. As a result, dpaa2_tm_deinit() and dpaa2_flow_clean()
are no longer executed.

Another issue is that dpaa2_dev_deinit is now called in secondary
process, which was not the case before this commit.

For comparison, other drivers like ixgbe call their close method from
the bus’s remove callback. I don’t see any issue doing the same
here—calling close during remove if the Ethernet device is still
allocated.

As far as I know, there is no counterpart to rte_eth_dev_close() in
the DPDK API (there could have been an rte_eth_dev_open()). The open
path is always triggered by the bus’s probe callback.
Because of this, it is expected that dpaa2_dev_init is called from the
plug/probe path, while the corresponding deinit logic is handled in
the close callback.

One more point: why isn’t dpaa2_tx_sg_pool_init() done directly in
dpaa2_dev_init(), with its corresponding cleanup performed in
dpaa2_dev_close()?

Maxime Leroy

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

* Re: [PATCH v2 4/5] net/dpaa2: bifurcate close into close and deinit
  2025-11-13  5:29   ` [PATCH v2 4/5] net/dpaa2: bifurcate close into close and deinit Hemant Agrawal
  2025-11-13  9:48     ` Maxime Leroy
@ 2025-11-13  9:49     ` Maxime Leroy
  2025-11-13  9:55       ` Hemant Agrawal
  1 sibling, 1 reply; 28+ messages in thread
From: Maxime Leroy @ 2025-11-13  9:49 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: dev, stephen, david.marchand

Hi Hemant,

Le jeu. 13 nov. 2025 à 06:30, Hemant Agrawal <hemant.agrawal@nxp.com> a écrit :
>
> The close function was overloaded to be also used as deinit function.
> This can cause issues when using functions like hotplug.
>
> This patch cleans up the code and implement separate close and deinit
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>  drivers/net/dpaa2/dpaa2_ethdev.c | 92 ++++++++++++++++++--------------
>  1 file changed, 51 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> index f82c50341d..0cd875d715 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -1524,53 +1524,14 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)
>  static int
>  dpaa2_dev_close(struct rte_eth_dev *dev)
>  {
> -       struct dpaa2_dev_priv *priv = dev->data->dev_private;
> -       struct fsl_mc_io *dpni = dev->process_private;
> -       int i, ret;
> -       struct rte_eth_link link;
> -
>         PMD_INIT_FUNC_TRACE();
>
>         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>                 return 0;
>
> -       if (!dpni) {
> -               DPAA2_PMD_WARN("Already closed or not started");
> -               return -EINVAL;
> -       }
> -
>         dpaa2_tm_deinit(dev);
>         dpaa2_flow_clean(dev);
> -       /* Clean the device first */
> -       ret = dpni_reset(dpni, CMD_PRI_LOW, priv->token);
> -       if (ret) {
> -               DPAA2_PMD_ERR("Failure cleaning dpni device: err=%d", ret);
> -               return ret;
> -       }
> -
> -       memset(&link, 0, sizeof(link));
> -       rte_eth_linkstatus_set(dev, &link);
> -
> -       /* Free private queues memory */
> -       dpaa2_free_rx_tx_queues(dev);
> -       /* Close the device at underlying layer*/
> -       ret = dpni_close(dpni, CMD_PRI_LOW, priv->token);
> -       if (ret) {
> -               DPAA2_PMD_ERR("Failure closing dpni device with err code %d",
> -                       ret);
> -       }
> -
> -       /* Free the allocated memory for ethernet private data and dpni*/
> -       priv->hw = NULL;
> -       dev->process_private = NULL;
> -       rte_free(dpni);
>
> -       for (i = 0; i < MAX_TCS; i++)
> -               rte_free(priv->extract.tc_extract_param[i]);
> -
> -       rte_free(priv->extract.qos_extract_param);
> -
> -       DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
>         return 0;
>  }
>
> @@ -2891,6 +2852,55 @@ dpaa2_get_devargs(struct rte_devargs *devargs, const char *key)
>         return 1;
>  }
>
> +static int
> +dpaa2_dev_deinit(struct rte_eth_dev *dev)
> +{
> +       struct dpaa2_dev_priv *priv = dev->data->dev_private;
> +       struct fsl_mc_io *dpni = dev->process_private;
> +       int i, ret;
> +       struct rte_eth_link link;
> +
> +       PMD_INIT_FUNC_TRACE();
> +
> +       if (!dpni) {
> +               DPAA2_PMD_WARN("Already closed or not started");
> +               return -EINVAL;
> +       }
> +
> +       /* Clean the device first */
> +       ret = dpni_reset(dpni, CMD_PRI_LOW, priv->token);
> +       if (ret) {
> +               DPAA2_PMD_ERR("Failure cleaning dpni device: err=%d", ret);
> +               return ret;
> +       }
> +
> +       memset(&link, 0, sizeof(link));
> +       rte_eth_linkstatus_set(dev, &link);
> +
> +       /* Free private queues memory */
> +       dpaa2_free_rx_tx_queues(dev);
> +       /* Close the device at underlying layer*/
> +       ret = dpni_close(dpni, CMD_PRI_LOW, priv->token);
> +       if (ret) {
> +               DPAA2_PMD_ERR("Failure closing dpni device with err code %d",
> +                       ret);
> +       }
> +
> +       /* Free the allocated memory for ethernet private data and dpni*/
> +       priv->hw = NULL;
> +       dev->process_private = NULL;
> +       rte_free(dpni);
> +
> +       for (i = 0; i < MAX_TCS; i++)
> +               rte_free(priv->extract.tc_extract_param[i]);
> +
> +       rte_free(priv->extract.qos_extract_param);
> +
> +       DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
> +       return 0;
> +}
> +
> +
>  static int
>  dpaa2_dev_init(struct rte_eth_dev *eth_dev)
>  {
> @@ -3177,7 +3187,7 @@ dpaa2_dev_init(struct rte_eth_dev *eth_dev)
>
>         return 0;
>  init_err:
> -       dpaa2_dev_close(eth_dev);
> +       dpaa2_dev_deinit(eth_dev);
>
>         return ret;
>  }
> @@ -3381,7 +3391,7 @@ rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
>
>         eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
>         if (eth_dev) {
> -               dpaa2_dev_close(eth_dev);
> +               dpaa2_dev_deinit(eth_dev);
>                 ret = rte_eth_dev_release_port(eth_dev);
>         }

When a DPDK application stops, dpaa2_dev_close is no longer called.
This happens because eal_cleanup() invokes rte_fslmc_close(), which
then calls the remove function (rte_dpaa2_remove) for each DPAA2
Ethernet driver. As a result, dpaa2_tm_deinit() and dpaa2_flow_clean()
are no longer executed.

Another issue is that dpaa2_dev_deinit is now called in secondary
process, which was not the case before this commit.

For comparison, other drivers like ixgbe call their close method from
the bus’s remove callback. I don’t see any issue doing the same
here—calling close during remove if the Ethernet device is still
allocated.

As far as I know, there is no counterpart to rte_eth_dev_close() in
the DPDK API (there could have been an rte_eth_dev_open()). The open
path is always triggered by the bus’s probe callback.
Because of this, it is expected that dpaa2_dev_init is called from the
plug/probe path, while the corresponding deinit logic is handled in
the close callback.

One more point: why isn’t dpaa2_tx_sg_pool_init() done directly in
dpaa2_dev_init(), with its corresponding cleanup performed in
dpaa2_dev_close()?

Maxime Leroy

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

* Re: [PATCH v2 4/5] net/dpaa2: bifurcate close into close and deinit
  2025-11-13  9:49     ` Maxime Leroy
@ 2025-11-13  9:55       ` Hemant Agrawal
  0 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13  9:55 UTC (permalink / raw)
  To: Maxime Leroy; +Cc: dev, stephen, david.marchand


On 13-11-2025 15:19, Maxime Leroy wrote:
> [Some people who received this message don't often get email from maxime@leroys.fr. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Hemant,
>
> Le jeu. 13 nov. 2025 à 06:30, Hemant Agrawal <hemant.agrawal@nxp.com> a écrit :
>> The close function was overloaded to be also used as deinit function.
>> This can cause issues when using functions like hotplug.
>>
>> This patch cleans up the code and implement separate close and deinit
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> ---
>>   drivers/net/dpaa2/dpaa2_ethdev.c | 92 ++++++++++++++++++--------------
>>   1 file changed, 51 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
>> index f82c50341d..0cd875d715 100644
>> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
>> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
>> @@ -1524,53 +1524,14 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)
>>   static int
>>   dpaa2_dev_close(struct rte_eth_dev *dev)
>>   {
>> -       struct dpaa2_dev_priv *priv = dev->data->dev_private;
>> -       struct fsl_mc_io *dpni = dev->process_private;
>> -       int i, ret;
>> -       struct rte_eth_link link;
>> -
>>          PMD_INIT_FUNC_TRACE();
>>
>>          if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>                  return 0;
>>
>> -       if (!dpni) {
>> -               DPAA2_PMD_WARN("Already closed or not started");
>> -               return -EINVAL;
>> -       }
>> -
>>          dpaa2_tm_deinit(dev);
>>          dpaa2_flow_clean(dev);
>> -       /* Clean the device first */
>> -       ret = dpni_reset(dpni, CMD_PRI_LOW, priv->token);
>> -       if (ret) {
>> -               DPAA2_PMD_ERR("Failure cleaning dpni device: err=%d", ret);
>> -               return ret;
>> -       }
>> -
>> -       memset(&link, 0, sizeof(link));
>> -       rte_eth_linkstatus_set(dev, &link);
>> -
>> -       /* Free private queues memory */
>> -       dpaa2_free_rx_tx_queues(dev);
>> -       /* Close the device at underlying layer*/
>> -       ret = dpni_close(dpni, CMD_PRI_LOW, priv->token);
>> -       if (ret) {
>> -               DPAA2_PMD_ERR("Failure closing dpni device with err code %d",
>> -                       ret);
>> -       }
>> -
>> -       /* Free the allocated memory for ethernet private data and dpni*/
>> -       priv->hw = NULL;
>> -       dev->process_private = NULL;
>> -       rte_free(dpni);
>>
>> -       for (i = 0; i < MAX_TCS; i++)
>> -               rte_free(priv->extract.tc_extract_param[i]);
>> -
>> -       rte_free(priv->extract.qos_extract_param);
>> -
>> -       DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
>>          return 0;
>>   }
>>
>> @@ -2891,6 +2852,55 @@ dpaa2_get_devargs(struct rte_devargs *devargs, const char *key)
>>          return 1;
>>   }
>>
>> +static int
>> +dpaa2_dev_deinit(struct rte_eth_dev *dev)
>> +{
>> +       struct dpaa2_dev_priv *priv = dev->data->dev_private;
>> +       struct fsl_mc_io *dpni = dev->process_private;
>> +       int i, ret;
>> +       struct rte_eth_link link;
>> +
>> +       PMD_INIT_FUNC_TRACE();
>> +
>> +       if (!dpni) {
>> +               DPAA2_PMD_WARN("Already closed or not started");
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* Clean the device first */
>> +       ret = dpni_reset(dpni, CMD_PRI_LOW, priv->token);
>> +       if (ret) {
>> +               DPAA2_PMD_ERR("Failure cleaning dpni device: err=%d", ret);
>> +               return ret;
>> +       }
>> +
>> +       memset(&link, 0, sizeof(link));
>> +       rte_eth_linkstatus_set(dev, &link);
>> +
>> +       /* Free private queues memory */
>> +       dpaa2_free_rx_tx_queues(dev);
>> +       /* Close the device at underlying layer*/
>> +       ret = dpni_close(dpni, CMD_PRI_LOW, priv->token);
>> +       if (ret) {
>> +               DPAA2_PMD_ERR("Failure closing dpni device with err code %d",
>> +                       ret);
>> +       }
>> +
>> +       /* Free the allocated memory for ethernet private data and dpni*/
>> +       priv->hw = NULL;
>> +       dev->process_private = NULL;
>> +       rte_free(dpni);
>> +
>> +       for (i = 0; i < MAX_TCS; i++)
>> +               rte_free(priv->extract.tc_extract_param[i]);
>> +
>> +       rte_free(priv->extract.qos_extract_param);
>> +
>> +       DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
>> +       return 0;
>> +}
>> +
>> +
>>   static int
>>   dpaa2_dev_init(struct rte_eth_dev *eth_dev)
>>   {
>> @@ -3177,7 +3187,7 @@ dpaa2_dev_init(struct rte_eth_dev *eth_dev)
>>
>>          return 0;
>>   init_err:
>> -       dpaa2_dev_close(eth_dev);
>> +       dpaa2_dev_deinit(eth_dev);
>>
>>          return ret;
>>   }
>> @@ -3381,7 +3391,7 @@ rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
>>
>>          eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
>>          if (eth_dev) {
>> -               dpaa2_dev_close(eth_dev);
>> +               dpaa2_dev_deinit(eth_dev);
>>                  ret = rte_eth_dev_release_port(eth_dev);
>>          }
> When a DPDK application stops, dpaa2_dev_close is no longer called.
> This happens because eal_cleanup() invokes rte_fslmc_close(), which
> then calls the remove function (rte_dpaa2_remove) for each DPAA2
> Ethernet driver. As a result, dpaa2_tm_deinit() and dpaa2_flow_clean()
> are no longer executed.
>
> Another issue is that dpaa2_dev_deinit is now called in secondary
> process, which was not the case before this commit.
>
> For comparison, other drivers like ixgbe call their close method from
> the bus’s remove callback. I don’t see any issue doing the same
> here—calling close during remove if the Ethernet device is still
> allocated.
>
> As far as I know, there is no counterpart to rte_eth_dev_close() in
> the DPDK API (there could have been an rte_eth_dev_open()). The open
> path is always triggered by the bus’s probe callback.
> Because of this, it is expected that dpaa2_dev_init is called from the
> plug/probe path, while the corresponding deinit logic is handled in
> the close callback.
ok, it looks like best approach is to drop this patch.
>
> One more point: why isn’t dpaa2_tx_sg_pool_init() done directly in
> dpaa2_dev_init(), with its corresponding cleanup performed in
> dpaa2_dev_close()?

dpaa2_tx_sg_pool is a global resource across devices; which is being 
tracked with dpaa2_valid_dev

we will be revamping this logic in next major release.

>
> Maxime Leroy

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

* [PATCH v3 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close
  2025-11-13  5:29 ` [PATCH v2 1/5] " Hemant Agrawal
                     ` (3 preceding siblings ...)
  2025-11-13  5:29   ` [PATCH v2 5/5] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
@ 2025-11-13  9:59   ` Hemant Agrawal
  2025-11-13  9:59     ` [PATCH v3 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
                       ` (3 more replies)
  4 siblings, 4 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13  9:59 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime; +Cc: sachin.saxena, stable

When rte_eth_dev_close() is called, it performs the following actions:

Calls dev->dev_ops->dev_close(), which in this case is dpaa2_dev_close().
Then calls rte_eth_dev_release_port(), which releases all device data
and sets dev->data to NULL.

Later, when rte_dev_remove() is called, the FSLMC bus invokes
dev->remove() — that is, rte_dpaa2_remove().
However, rte_dpaa2_remove() calls dpaa2_dev_close() again. Since dev->data
was already set to NULL by the previous call, this second invocation
causes a crash.

Fixes: 5964d36a2904 ("net/dpaa2: release port upon close")
Cc: sachin.saxena@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 7da32ce856..fc63cf4f09 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -3347,14 +3347,19 @@ static int
 rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
 {
 	struct rte_eth_dev *eth_dev;
-	int ret;
+	int ret = 0;
+
+	eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
+	if (eth_dev) {
+		dpaa2_dev_close(eth_dev);
+		ret = rte_eth_dev_release_port(eth_dev);
+	}
 
-	eth_dev = dpaa2_dev->eth_dev;
-	dpaa2_dev_close(eth_dev);
 	dpaa2_valid_dev--;
-	if (!dpaa2_valid_dev)
+	if (!dpaa2_valid_dev) {
 		rte_mempool_free(dpaa2_tx_sg_pool);
-	ret = rte_eth_dev_release_port(eth_dev);
+		dpaa2_tx_sg_pool = NULL;
+	}
 
 	return ret;
 }
-- 
2.25.1


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

* [PATCH v3 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues
  2025-11-13  9:59   ` [PATCH v3 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
@ 2025-11-13  9:59     ` Hemant Agrawal
  2025-11-13 11:27       ` Maxime Leroy
  2025-11-13  9:59     ` [PATCH v3 3/4] net/dpaa2: fix queue free cleanup Hemant Agrawal
                       ` (2 subsequent siblings)
  3 siblings, 1 reply; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13  9:59 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime; +Cc: jun.yang, stable

From: Maxime Leroy <maxime@leroys.fr>

When using the prefetch Rx path (dpaa2_dev_prefetch_rx), the driver keeps
track of one outstanding VDQCR command per DPIO portal in the global
rte_global_active_dqs_list[] array. Each queue_storage_info_t also stores
the active result buffer and portal index:

  qs->active_dqs
  qs->active_dpio_id

Before issuing a new pull command, dpaa2_dev_prefetch_rx() checks for an
active entry and spins on qbman_check_command_complete() until the
corresponding VDQCR completes.

On port close / hotplug remove, dpaa2_free_rx_tx_queues() frees all
per-lcore queue_storage_info_t structures and their dq_storage[] buffers,
but never clears the global rte_global_active_dqs_list[] entries. After a
detach/attach sequence (or "del/add" in grout), the prefetch Rx path
still sees an active entry for the portal and spins forever on a stale dq
buffer that has been freed and will never be completed by hardware. In
gdb, dq->dq.tok stays 0 and dpaa2_dev_prefetch_rx() loops in:

  while (!qbman_check_command_complete(get_swp_active_dqs(idx)))
      ;

Fix this by clearing the active VDQ state before freeing queue storage.
For each Rx queue and lcore, if qs->active_dqs is non-NULL, call
clear_swp_active_dqs(qs->active_dpio_id) and set qs->active_dqs to NULL.
Then dpaa2_queue_storage_free() can safely free q_storage and
dq_storage[].

After this change, a DPNI detach/attach sequence no longer leaves stale
entries in rte_global_active_dqs_list[], and the prefetch Rx loop does
not hang waiting for a completion from a previous device instance.

Reproduction:
  - grout:
      grcli interface add  port dpni.1 devargs fslmc:dpni.1
      grcli interface del  dpni.1
      grcli interface add  port dpni.1 devargs fslmc:dpni.1
    -> Rx was stuck in qbman_check_command_complete(), now works.

  - testpmd:
      dpdk-testpmd -n1 -a fslmc:dpni.65535 -- -i --forward-mode=rxonly
      testpmd> port attach fslmc:dpni.1
      testpmd> port start all
      testpmd> start
      testpmd> stop
      testpmd> port stop all
      testpmd> port detach 0
      testpmd> port attach fslmc:dpni.1
      testpmd> port start all
      testpmd> start
    -> Rx was hanging, now runs normal

Fixes: 12d98eceb8ac ("bus/fslmc: enhance QBMAN DQ storage logic")
Cc: jun.yang@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Maxime Leroy <maxime@leroys.fr>
---
 .mailmap                         |  2 +-
 drivers/net/dpaa2/dpaa2_ethdev.c | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index 10c37a97a6..d92d4fc24b 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1035,7 +1035,7 @@ Mauricio Vasquez B <mauricio.vasquez@polito.it> <mauricio.vasquezbernal@studenti
 Mauro Annarumma <mauroannarumma@hotmail.it>
 Maxime Coquelin <maxime.coquelin@redhat.com>
 Maxime Gouin <maxime.gouin@6wind.com>
-Maxime Leroy <maxime.leroy@6wind.com>
+Maxime Leroy <maxime.leroy@6wind.com> <maxime@leroys.fr>
 Md Fahad Iqbal Polash <md.fahad.iqbal.polash@intel.com>
 Megha Ajmera <megha.ajmera@intel.com>
 Meijuan Zhao <meijuanx.zhao@intel.com>
diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index fc63cf4f09..c94034104a 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -631,6 +631,27 @@ dpaa2_alloc_rx_tx_queues(struct rte_eth_dev *dev)
 	return ret;
 }
 
+static void
+dpaa2_clear_queue_active_dps(struct dpaa2_queue *q)
+{
+	int lcore_id;
+
+	RTE_LCORE_FOREACH(lcore_id) {
+		struct queue_storage_info_t *qs = q->q_storage[lcore_id];
+
+		if (!qs)
+			continue;
+
+		if (qs->active_dqs) {
+			while (!qbman_check_command_complete(qs->active_dqs))
+				/* wait */;
+
+			clear_swp_active_dqs(qs->active_dpio_id);
+			qs->active_dqs = NULL;
+		}
+	}
+}
+
 static void
 dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 {
@@ -645,6 +666,7 @@ dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 		/* cleaning up queue storage */
 		for (i = 0; i < priv->nb_rx_queues; i++) {
 			dpaa2_q = priv->rx_vq[i];
+			dpaa2_clear_queue_active_dps(dpaa2_q);
 			dpaa2_queue_storage_free(dpaa2_q,
 				RTE_MAX_LCORE);
 		}
-- 
2.25.1


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

* [PATCH v3 3/4] net/dpaa2: fix queue free cleanup
  2025-11-13  9:59   ` [PATCH v3 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
  2025-11-13  9:59     ` [PATCH v3 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
@ 2025-11-13  9:59     ` Hemant Agrawal
  2025-11-13  9:59     ` [PATCH v3 4/4] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
  2025-11-13 11:43     ` [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
  3 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13  9:59 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime; +Cc: hemant.agrawal

The RX error queue was not being free.
Also, set the free queues pointers to NULL.

Fixes: 407ce3e5384b ("net/dpaa2: replace global variables with flags")
Cc: hemant.agrawal@nxp.com

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index c94034104a..f82c50341d 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -669,11 +669,13 @@ dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 			dpaa2_clear_queue_active_dps(dpaa2_q);
 			dpaa2_queue_storage_free(dpaa2_q,
 				RTE_MAX_LCORE);
+			priv->rx_vq[i] = NULL;
 		}
 		/* cleanup tx queue cscn */
 		for (i = 0; i < priv->nb_tx_queues; i++) {
 			dpaa2_q = priv->tx_vq[i];
 			rte_free(dpaa2_q->cscn);
+			priv->tx_vq[i] = NULL;
 		}
 		if (priv->flags & DPAA2_TX_CONF_ENABLE) {
 			/* cleanup tx conf queue storage */
@@ -681,8 +683,14 @@ dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 				dpaa2_q = priv->tx_conf_vq[i];
 				dpaa2_queue_storage_free(dpaa2_q,
 					RTE_MAX_LCORE);
+				priv->tx_conf_vq[i] = NULL;
 			}
 		}
+		if (priv->flags & DPAAX_RX_ERROR_QUEUE_FLAG) {
+			dpaa2_q = priv->rx_err_vq;
+			dpaa2_queue_storage_free(dpaa2_q, RTE_MAX_LCORE);
+		}
+
 		/*free memory for all queues (RX+TX) */
 		rte_free(priv->rx_vq[0]);
 		priv->rx_vq[0] = NULL;
-- 
2.25.1


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

* [PATCH v3 4/4] bus/fslmc: add support for hotplug of dpni
  2025-11-13  9:59   ` [PATCH v3 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
  2025-11-13  9:59     ` [PATCH v3 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
  2025-11-13  9:59     ` [PATCH v3 3/4] net/dpaa2: fix queue free cleanup Hemant Agrawal
@ 2025-11-13  9:59     ` Hemant Agrawal
  2025-11-13 11:43     ` [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
  3 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13  9:59 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime

This patch implements the plug and unplug function to support
attach/detach of dpni interfaces.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/fslmc/fslmc_bus.c | 56 +++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
index 49c61c9d2d..4529ec5085 100644
--- a/drivers/bus/fslmc/fslmc_bus.c
+++ b/drivers/bus/fslmc/fslmc_bus.c
@@ -589,17 +589,61 @@ rte_dpaa2_get_iommu_class(void)
 }
 
 static int
-fslmc_bus_plug(struct rte_device *dev __rte_unused)
+fslmc_bus_plug(struct rte_device *rte_dev)
 {
-	/* No operation is performed while plugging the device */
-	return 0;
+	int ret = 0;
+	struct rte_dpaa2_device *dev = container_of(rte_dev,
+			struct rte_dpaa2_device, device);
+	struct rte_dpaa2_driver *drv;
+
+	TAILQ_FOREACH(drv, &rte_fslmc_bus.driver_list, next) {
+		ret = rte_fslmc_match(drv, dev);
+		if (ret)
+			continue;
+
+		if (!drv->probe)
+			continue;
+
+		if (rte_dev_is_probed(&dev->device))
+			continue;
+
+		if (dev->device.devargs &&
+		    dev->device.devargs->policy == RTE_DEV_BLOCKED) {
+			DPAA2_BUS_DEBUG("%s Blocked, skipping",
+				      dev->device.name);
+			continue;
+		}
+
+		ret = drv->probe(drv, dev);
+		if (ret) {
+			DPAA2_BUS_ERR("Unable to probe");
+		} else {
+			dev->driver = drv;
+			dev->device.driver = &drv->driver;
+			DPAA2_BUS_INFO("%s Plugged",  dev->device.name);
+		}
+		break;
+	}
+
+	return ret;
 }
 
 static int
-fslmc_bus_unplug(struct rte_device *dev __rte_unused)
+fslmc_bus_unplug(struct rte_device *rte_dev)
 {
-	/* No operation is performed while unplugging the device */
-	return 0;
+	struct rte_dpaa2_device *dev = container_of(rte_dev,
+			struct rte_dpaa2_device, device);
+	struct rte_dpaa2_driver *drv = dev->driver;
+
+	if (drv && drv->remove) {
+		drv->remove(dev);
+		dev->driver = NULL;
+		dev->device.driver = NULL;
+		DPAA2_BUS_INFO("%s Un-Plugged",  dev->device.name);
+		return 0;
+	}
+
+	return -ENODEV;
 }
 
 static void *
-- 
2.25.1


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

* Re: [PATCH v3 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues
  2025-11-13  9:59     ` [PATCH v3 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
@ 2025-11-13 11:27       ` Maxime Leroy
  0 siblings, 0 replies; 28+ messages in thread
From: Maxime Leroy @ 2025-11-13 11:27 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: dev, stephen, david.marchand, jun.yang, stable

Hi Hemant,

Le jeu. 13 nov. 2025 à 10:59, Hemant Agrawal <hemant.agrawal@nxp.com> a écrit :
>
> From: Maxime Leroy <maxime@leroys.fr>
>
> When using the prefetch Rx path (dpaa2_dev_prefetch_rx), the driver keeps
> track of one outstanding VDQCR command per DPIO portal in the global
> rte_global_active_dqs_list[] array. Each queue_storage_info_t also stores
> the active result buffer and portal index:
>
>   qs->active_dqs
>   qs->active_dpio_id
>
> Before issuing a new pull command, dpaa2_dev_prefetch_rx() checks for an
> active entry and spins on qbman_check_command_complete() until the
> corresponding VDQCR completes.
>
> On port close / hotplug remove, dpaa2_free_rx_tx_queues() frees all
> per-lcore queue_storage_info_t structures and their dq_storage[] buffers,
> but never clears the global rte_global_active_dqs_list[] entries. After a
> detach/attach sequence (or "del/add" in grout), the prefetch Rx path
> still sees an active entry for the portal and spins forever on a stale dq
> buffer that has been freed and will never be completed by hardware. In
> gdb, dq->dq.tok stays 0 and dpaa2_dev_prefetch_rx() loops in:
>
>   while (!qbman_check_command_complete(get_swp_active_dqs(idx)))
>       ;
>
> Fix this by clearing the active VDQ state before freeing queue storage.
> For each Rx queue and lcore, if qs->active_dqs is non-NULL, call
> clear_swp_active_dqs(qs->active_dpio_id) and set qs->active_dqs to NULL.
> Then dpaa2_queue_storage_free() can safely free q_storage and
> dq_storage[].
>
> After this change, a DPNI detach/attach sequence no longer leaves stale
> entries in rte_global_active_dqs_list[], and the prefetch Rx loop does
> not hang waiting for a completion from a previous device instance.
>
> Reproduction:
>   - grout:
>       grcli interface add  port dpni.1 devargs fslmc:dpni.1
>       grcli interface del  dpni.1
>       grcli interface add  port dpni.1 devargs fslmc:dpni.1
>     -> Rx was stuck in qbman_check_command_complete(), now works.
>
>   - testpmd:
>       dpdk-testpmd -n1 -a fslmc:dpni.65535 -- -i --forward-mode=rxonly
>       testpmd> port attach fslmc:dpni.1
>       testpmd> port start all
>       testpmd> start
>       testpmd> stop
>       testpmd> port stop all
>       testpmd> port detach 0
>       testpmd> port attach fslmc:dpni.1
>       testpmd> port start all
>       testpmd> start
>     -> Rx was hanging, now runs normal
>
> Fixes: 12d98eceb8ac ("bus/fslmc: enhance QBMAN DQ storage logic")
> Cc: jun.yang@nxp.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Leroy <maxime@leroys.fr>
> ---
>  .mailmap                         |  2 +-
>  drivers/net/dpaa2/dpaa2_ethdev.c | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/.mailmap b/.mailmap
> index 10c37a97a6..d92d4fc24b 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1035,7 +1035,7 @@ Mauricio Vasquez B <mauricio.vasquez@polito.it> <mauricio.vasquezbernal@studenti
>  Mauro Annarumma <mauroannarumma@hotmail.it>
>  Maxime Coquelin <maxime.coquelin@redhat.com>
>  Maxime Gouin <maxime.gouin@6wind.com>
> -Maxime Leroy <maxime.leroy@6wind.com>
> +Maxime Leroy <maxime.leroy@6wind.com> <maxime@leroys.fr>
>  Md Fahad Iqbal Polash <md.fahad.iqbal.polash@intel.com>
>  Megha Ajmera <megha.ajmera@intel.com>
>  Meijuan Zhao <meijuanx.zhao@intel.com>
> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> index fc63cf4f09..c94034104a 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -631,6 +631,27 @@ dpaa2_alloc_rx_tx_queues(struct rte_eth_dev *dev)
>         return ret;
>  }
>
> +static void
> +dpaa2_clear_queue_active_dps(struct dpaa2_queue *q)
> +{
> +       int lcore_id;
> +
> +       RTE_LCORE_FOREACH(lcore_id) {

RTE_LCORE_FOREACH does not iterate over NON-EAL threads. However,
Grout creates NON-EAL threads using the pthread library to poll the RX
queue.

As a result, using RTE_LCORE_FOREACH breaks the fix. We should revert
to the previous version of the fix.

Thanks,

Maxime Leroy

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

* [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close
  2025-11-13  9:59   ` [PATCH v3 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
                       ` (2 preceding siblings ...)
  2025-11-13  9:59     ` [PATCH v3 4/4] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
@ 2025-11-13 11:43     ` Hemant Agrawal
  2025-11-13 11:43       ` [PATCH v4 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
                         ` (3 more replies)
  3 siblings, 4 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13 11:43 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime; +Cc: sachin.saxena, stable

When rte_eth_dev_close() is called, it performs the following actions:

Calls dev->dev_ops->dev_close(), which in this case is dpaa2_dev_close().
Then calls rte_eth_dev_release_port(), which releases all device data
and sets dev->data to NULL.

Later, when rte_dev_remove() is called, the FSLMC bus invokes
dev->remove() — that is, rte_dpaa2_remove().
However, rte_dpaa2_remove() calls dpaa2_dev_close() again. Since dev->data
was already set to NULL by the previous call, this second invocation
causes a crash.

Fixes: 5964d36a2904 ("net/dpaa2: release port upon close")
Cc: sachin.saxena@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 7da32ce856..fc63cf4f09 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -3347,14 +3347,19 @@ static int
 rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
 {
 	struct rte_eth_dev *eth_dev;
-	int ret;
+	int ret = 0;
+
+	eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
+	if (eth_dev) {
+		dpaa2_dev_close(eth_dev);
+		ret = rte_eth_dev_release_port(eth_dev);
+	}
 
-	eth_dev = dpaa2_dev->eth_dev;
-	dpaa2_dev_close(eth_dev);
 	dpaa2_valid_dev--;
-	if (!dpaa2_valid_dev)
+	if (!dpaa2_valid_dev) {
 		rte_mempool_free(dpaa2_tx_sg_pool);
-	ret = rte_eth_dev_release_port(eth_dev);
+		dpaa2_tx_sg_pool = NULL;
+	}
 
 	return ret;
 }
-- 
2.25.1


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

* [PATCH v4 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues
  2025-11-13 11:43     ` [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
@ 2025-11-13 11:43       ` Hemant Agrawal
  2025-11-13 11:43       ` [PATCH v4 3/4] net/dpaa2: fix queue free cleanup Hemant Agrawal
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13 11:43 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime; +Cc: jun.yang, stable

From: Maxime Leroy <maxime@leroys.fr>

When using the prefetch Rx path (dpaa2_dev_prefetch_rx), the driver keeps
track of one outstanding VDQCR command per DPIO portal in the global
rte_global_active_dqs_list[] array. Each queue_storage_info_t also stores
the active result buffer and portal index:

  qs->active_dqs
  qs->active_dpio_id

Before issuing a new pull command, dpaa2_dev_prefetch_rx() checks for an
active entry and spins on qbman_check_command_complete() until the
corresponding VDQCR completes.

On port close / hotplug remove, dpaa2_free_rx_tx_queues() frees all
per-lcore queue_storage_info_t structures and their dq_storage[] buffers,
but never clears the global rte_global_active_dqs_list[] entries. After a
detach/attach sequence (or "del/add" in grout), the prefetch Rx path
still sees an active entry for the portal and spins forever on a stale dq
buffer that has been freed and will never be completed by hardware. In
gdb, dq->dq.tok stays 0 and dpaa2_dev_prefetch_rx() loops in:

  while (!qbman_check_command_complete(get_swp_active_dqs(idx)))
      ;

Fix this by clearing the active VDQ state before freeing queue storage.
For each Rx queue and lcore, if qs->active_dqs is non-NULL, call
clear_swp_active_dqs(qs->active_dpio_id) and set qs->active_dqs to NULL.
Then dpaa2_queue_storage_free() can safely free q_storage and
dq_storage[].

After this change, a DPNI detach/attach sequence no longer leaves stale
entries in rte_global_active_dqs_list[], and the prefetch Rx loop does
not hang waiting for a completion from a previous device instance.

Reproduction:
  - grout:
      grcli interface add  port dpni.1 devargs fslmc:dpni.1
      grcli interface del  dpni.1
      grcli interface add  port dpni.1 devargs fslmc:dpni.1
    -> Rx was stuck in qbman_check_command_complete(), now works.

  - testpmd:
      dpdk-testpmd -n1 -a fslmc:dpni.65535 -- -i --forward-mode=rxonly
      testpmd> port attach fslmc:dpni.1
      testpmd> port start all
      testpmd> start
      testpmd> stop
      testpmd> port stop all
      testpmd> port detach 0
      testpmd> port attach fslmc:dpni.1
      testpmd> port start all
      testpmd> start
    -> Rx was hanging, now runs normal

Fixes: 12d98eceb8ac ("bus/fslmc: enhance QBMAN DQ storage logic")
Cc: jun.yang@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Maxime Leroy <maxime@leroys.fr>
---
 .mailmap                         |  2 +-
 drivers/net/dpaa2/dpaa2_ethdev.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index 10c37a97a6..d92d4fc24b 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1035,7 +1035,7 @@ Mauricio Vasquez B <mauricio.vasquez@polito.it> <mauricio.vasquezbernal@studenti
 Mauro Annarumma <mauroannarumma@hotmail.it>
 Maxime Coquelin <maxime.coquelin@redhat.com>
 Maxime Gouin <maxime.gouin@6wind.com>
-Maxime Leroy <maxime.leroy@6wind.com>
+Maxime Leroy <maxime.leroy@6wind.com> <maxime@leroys.fr>
 Md Fahad Iqbal Polash <md.fahad.iqbal.polash@intel.com>
 Megha Ajmera <megha.ajmera@intel.com>
 Meijuan Zhao <meijuanx.zhao@intel.com>
diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index fc63cf4f09..3c3a1508fc 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -631,6 +631,27 @@ dpaa2_alloc_rx_tx_queues(struct rte_eth_dev *dev)
 	return ret;
 }
 
+static void
+dpaa2_clear_queue_active_dps(struct dpaa2_queue *q, int num_lcores)
+{
+	int i;
+
+	for (i = 0; i < num_lcores; i++) {
+		struct queue_storage_info_t *qs = q->q_storage[i];
+
+		if (!qs)
+			continue;
+
+		if (qs->active_dqs) {
+			while (!qbman_check_command_complete(qs->active_dqs))
+				/* wait */;
+
+			clear_swp_active_dqs(qs->active_dpio_id);
+			qs->active_dqs = NULL;
+		}
+	}
+}
+
 static void
 dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 {
@@ -645,6 +666,8 @@ dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 		/* cleaning up queue storage */
 		for (i = 0; i < priv->nb_rx_queues; i++) {
 			dpaa2_q = priv->rx_vq[i];
+			dpaa2_clear_queue_active_dps(dpaa2_q,
+						RTE_MAX_LCORE);
 			dpaa2_queue_storage_free(dpaa2_q,
 				RTE_MAX_LCORE);
 		}
-- 
2.25.1


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

* [PATCH v4 3/4] net/dpaa2: fix queue free cleanup
  2025-11-13 11:43     ` [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
  2025-11-13 11:43       ` [PATCH v4 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
@ 2025-11-13 11:43       ` Hemant Agrawal
  2025-11-13 11:43       ` [PATCH v4 4/4] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
  2025-11-13 15:11       ` [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Maxime Leroy
  3 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13 11:43 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime; +Cc: hemant.agrawal

The RX error queue was not being free.
Also, set the free queues pointers to NULL.

Fixes: 407ce3e5384b ("net/dpaa2: replace global variables with flags")
Cc: hemant.agrawal@nxp.com

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 3c3a1508fc..cf53ddf639 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -670,11 +670,13 @@ dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 						RTE_MAX_LCORE);
 			dpaa2_queue_storage_free(dpaa2_q,
 				RTE_MAX_LCORE);
+			priv->rx_vq[i] = NULL;
 		}
 		/* cleanup tx queue cscn */
 		for (i = 0; i < priv->nb_tx_queues; i++) {
 			dpaa2_q = priv->tx_vq[i];
 			rte_free(dpaa2_q->cscn);
+			priv->tx_vq[i] = NULL;
 		}
 		if (priv->flags & DPAA2_TX_CONF_ENABLE) {
 			/* cleanup tx conf queue storage */
@@ -682,8 +684,14 @@ dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
 				dpaa2_q = priv->tx_conf_vq[i];
 				dpaa2_queue_storage_free(dpaa2_q,
 					RTE_MAX_LCORE);
+				priv->tx_conf_vq[i] = NULL;
 			}
 		}
+		if (priv->flags & DPAAX_RX_ERROR_QUEUE_FLAG) {
+			dpaa2_q = priv->rx_err_vq;
+			dpaa2_queue_storage_free(dpaa2_q, RTE_MAX_LCORE);
+		}
+
 		/*free memory for all queues (RX+TX) */
 		rte_free(priv->rx_vq[0]);
 		priv->rx_vq[0] = NULL;
-- 
2.25.1


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

* [PATCH v4 4/4] bus/fslmc: add support for hotplug of dpni
  2025-11-13 11:43     ` [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
  2025-11-13 11:43       ` [PATCH v4 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
  2025-11-13 11:43       ` [PATCH v4 3/4] net/dpaa2: fix queue free cleanup Hemant Agrawal
@ 2025-11-13 11:43       ` Hemant Agrawal
  2025-11-13 15:11       ` [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Maxime Leroy
  3 siblings, 0 replies; 28+ messages in thread
From: Hemant Agrawal @ 2025-11-13 11:43 UTC (permalink / raw)
  To: dev, stephen, david.marchand, maxime

This patch implements the plug and unplug function to support
attach/detach of dpni interfaces.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/fslmc/fslmc_bus.c | 56 +++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
index 49c61c9d2d..4529ec5085 100644
--- a/drivers/bus/fslmc/fslmc_bus.c
+++ b/drivers/bus/fslmc/fslmc_bus.c
@@ -589,17 +589,61 @@ rte_dpaa2_get_iommu_class(void)
 }
 
 static int
-fslmc_bus_plug(struct rte_device *dev __rte_unused)
+fslmc_bus_plug(struct rte_device *rte_dev)
 {
-	/* No operation is performed while plugging the device */
-	return 0;
+	int ret = 0;
+	struct rte_dpaa2_device *dev = container_of(rte_dev,
+			struct rte_dpaa2_device, device);
+	struct rte_dpaa2_driver *drv;
+
+	TAILQ_FOREACH(drv, &rte_fslmc_bus.driver_list, next) {
+		ret = rte_fslmc_match(drv, dev);
+		if (ret)
+			continue;
+
+		if (!drv->probe)
+			continue;
+
+		if (rte_dev_is_probed(&dev->device))
+			continue;
+
+		if (dev->device.devargs &&
+		    dev->device.devargs->policy == RTE_DEV_BLOCKED) {
+			DPAA2_BUS_DEBUG("%s Blocked, skipping",
+				      dev->device.name);
+			continue;
+		}
+
+		ret = drv->probe(drv, dev);
+		if (ret) {
+			DPAA2_BUS_ERR("Unable to probe");
+		} else {
+			dev->driver = drv;
+			dev->device.driver = &drv->driver;
+			DPAA2_BUS_INFO("%s Plugged",  dev->device.name);
+		}
+		break;
+	}
+
+	return ret;
 }
 
 static int
-fslmc_bus_unplug(struct rte_device *dev __rte_unused)
+fslmc_bus_unplug(struct rte_device *rte_dev)
 {
-	/* No operation is performed while unplugging the device */
-	return 0;
+	struct rte_dpaa2_device *dev = container_of(rte_dev,
+			struct rte_dpaa2_device, device);
+	struct rte_dpaa2_driver *drv = dev->driver;
+
+	if (drv && drv->remove) {
+		drv->remove(dev);
+		dev->driver = NULL;
+		dev->device.driver = NULL;
+		DPAA2_BUS_INFO("%s Un-Plugged",  dev->device.name);
+		return 0;
+	}
+
+	return -ENODEV;
 }
 
 static void *
-- 
2.25.1


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

* Re: [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close
  2025-11-13 11:43     ` [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
                         ` (2 preceding siblings ...)
  2025-11-13 11:43       ` [PATCH v4 4/4] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
@ 2025-11-13 15:11       ` Maxime Leroy
  3 siblings, 0 replies; 28+ messages in thread
From: Maxime Leroy @ 2025-11-13 15:11 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: dev, stephen, david.marchand, sachin.saxena, stable

Hi Hemant,

Le jeu. 13 nov. 2025 à 12:44, Hemant Agrawal <hemant.agrawal@nxp.com> a écrit :
>
> When rte_eth_dev_close() is called, it performs the following actions:
>
> Calls dev->dev_ops->dev_close(), which in this case is dpaa2_dev_close().
> Then calls rte_eth_dev_release_port(), which releases all device data
> and sets dev->data to NULL.
>
> Later, when rte_dev_remove() is called, the FSLMC bus invokes
> dev->remove() — that is, rte_dpaa2_remove().
> However, rte_dpaa2_remove() calls dpaa2_dev_close() again. Since dev->data
> was already set to NULL by the previous call, this second invocation
> causes a crash.
>
> Fixes: 5964d36a2904 ("net/dpaa2: release port upon close")
> Cc: sachin.saxena@nxp.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>  drivers/net/dpaa2/dpaa2_ethdev.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> index 7da32ce856..fc63cf4f09 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -3347,14 +3347,19 @@ static int
>  rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
>  {
>         struct rte_eth_dev *eth_dev;
> -       int ret;
> +       int ret = 0;
> +
> +       eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
> +       if (eth_dev) {
> +               dpaa2_dev_close(eth_dev);
> +               ret = rte_eth_dev_release_port(eth_dev);
> +       }

dpaa2_dev_close() returns a status code, but it isn’t being checked here.

For comparison, in rte_eth_dev_pci_generic_remove(), if dev_uninit
(i.e. the device close callback) fails, rte_eth_dev_release_port() is
not called.
How should the dpaa2 driver handle this kind of error case?

By the way, if it makes sense for you, feel free to add a Tested-by
tag from me, since I’ve validated the series on my side with Grout.

Regards,

Maxime Leroy

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

end of thread, other threads:[~2025-11-13 15:11 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-11-06 16:38 [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
2025-11-06 16:38 ` [PATCH 2/3] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
2025-11-06 19:29   ` Stephen Hemminger
2025-11-07  9:51     ` Maxime Leroy
2025-11-07 10:38       ` Hemant Agrawal
2025-11-07  8:34   ` David Marchand
2025-11-06 16:38 ` [PATCH 3/3] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
2025-11-07  8:32 ` [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close David Marchand
2025-11-08 15:35   ` David Marchand
2025-11-12 23:06 ` Stephen Hemminger
2025-11-13  5:29 ` [PATCH v2 1/5] " Hemant Agrawal
2025-11-13  5:29   ` [PATCH v2 2/5] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
2025-11-13  5:29   ` [PATCH v2 3/5] net/dpaa2: fix queue free cleanup Hemant Agrawal
2025-11-13  5:29   ` [PATCH v2 4/5] net/dpaa2: bifurcate close into close and deinit Hemant Agrawal
2025-11-13  9:48     ` Maxime Leroy
2025-11-13  9:49     ` Maxime Leroy
2025-11-13  9:55       ` Hemant Agrawal
2025-11-13  5:29   ` [PATCH v2 5/5] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
2025-11-13  9:59   ` [PATCH v3 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
2025-11-13  9:59     ` [PATCH v3 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
2025-11-13 11:27       ` Maxime Leroy
2025-11-13  9:59     ` [PATCH v3 3/4] net/dpaa2: fix queue free cleanup Hemant Agrawal
2025-11-13  9:59     ` [PATCH v3 4/4] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
2025-11-13 11:43     ` [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Hemant Agrawal
2025-11-13 11:43       ` [PATCH v4 2/4] net/dpaa2: clear active VDQ state when freeing Rx queues Hemant Agrawal
2025-11-13 11:43       ` [PATCH v4 3/4] net/dpaa2: fix queue free cleanup Hemant Agrawal
2025-11-13 11:43       ` [PATCH v4 4/4] bus/fslmc: add support for hotplug of dpni Hemant Agrawal
2025-11-13 15:11       ` [PATCH v4 1/4] net/dpaa2: fix duplicate calling of dpaa2 dev close Maxime Leroy

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