Hi Hemant,
On Fri, Jul 3, 2026 at 7:21 PM Hemant Agrawal
<[email protected]> wrote:
>
>
> On 30-06-2026 20:13, Maxime Leroy wrote:
>
> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c
> b/drivers/net/dpaa2/dpaa2_ethdev.c
> index a68404ee5e..36f8669644 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -5,6 +5,8 @@
>
> #include <time.h>
> #include <net/if.h>
> +#include <unistd.h>
> +#include <errno.h>
>
> #include <eal_export.h>
> #include <rte_mbuf.h>
> @@ -25,6 +27,7 @@
> #include <dpaa2_hw_mempool.h>
> #include <dpaa2_hw_dpio.h>
> #include <mc/fsl_dpmng.h>
> +#include <mc/fsl_dpcon.h>
> #include "dpaa2_ethdev.h"
> #include "dpaa2_sparser.h"
> #include <fsl_qbman_debug.h>
> @@ -658,6 +661,8 @@ dpaa2_clear_queue_active_dps(struct dpaa2_queue *q, int
> num_lcores)
> }
> }
>
> +static void dpaa2_dev_rx_queue_intr_unbind(struct dpaa2_queue *dpaa2_q);
> +
> static void
> dpaa2_free_rx_tx_queues(struct rte_eth_dev *dev)
> {
> @@ -675,6 +680,12 @@ 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];
> + if (dpaa2_q->napi_dpcon) { /* release the rx-intr channel */
> + dpaa2_dev_rx_queue_intr_unbind(dpaa2_q);
> + rte_dpaa2_free_dpcon_dev(dpaa2_q->napi_dpcon);
> + dpaa2_q->napi_dpcon = NULL;
> + dpaa2_q->napi_sub_dpio = NULL;
> + }
> dpaa2_clear_queue_active_dps(dpaa2_q,
> RTE_MAX_LCORE);
> dpaa2_queue_storage_free(dpaa2_q,
> @@ -880,6 +891,26 @@ dpaa2_eth_dev_configure(struct rte_eth_dev *dev)
> }
> }
>
> + if (dev->data->dev_conf.intr_conf.rxq) {
> + if (!dev->intr_handle)
> + dev->intr_handle = rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_PRIVATE);
> + if (!dev->intr_handle ||
> + rte_intr_vec_list_alloc(dev->intr_handle, "rxq_intr",
> + dev->data->nb_rx_queues) ||
> + rte_intr_nb_efd_set(dev->intr_handle, dev->data->nb_rx_queues) ||
> + rte_intr_type_set(dev->intr_handle, RTE_INTR_HANDLE_EXT)) {
> + DPAA2_PMD_ERR("Failed to set up rx-queue interrupts");
> + /* capture the error before cleanup may clobber rte_errno */
> + ret = rte_errno ? -rte_errno : -EIO;
> + if (dev->intr_handle) {
> + rte_intr_vec_list_free(dev->intr_handle);
> + rte_intr_instance_free(dev->intr_handle);
> + dev->intr_handle = NULL;
> + }
> + return ret;
> + }
> + }
> +
>
> If dev_configure() is called a second time (e.g. after changing
> nb_rx_queues), dev->intr_handle is already non-NULL. The code skips
> rte_intr_instance_alloc() but calls rte_intr_vec_list_alloc() on the existing
> handle without first freeing the old vector list. This leaks the previously
> allocated vector list and may also cause a size mismatch if nb_rx_queues
> changed between calls.
>
> Should not we free the old vector list before reallocating:
>
> if (dev->intr_handle)
>
> rte_intr_vec_list_free(dev->intr_handle);
>
> else
>
> dev->intr_handle = rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_PRIVATE);
>
> /* then always call rte_intr_vec_list_alloc() */
>
real bug, but not a leak. rte_intr_vec_list_alloc() returns early when
intr_vec != NULL, so a second configure() allocates nothing. The
opposite happens: the vector list keeps its old size, and if
nb_rx_queues grew
(stop/reconfigure/start, no close, which the API allows), arming the
new queues fails with -ERANGE.
Freeing only the vector list isn't enough either, it leaves
nb_efd/the eventfd array stale. The fix is to rebuild the whole handle
on re-entry:
if (dev->intr_handle) {
rte_intr_vec_list_free(dev->intr_handle);
rte_intr_instance_free(dev->intr_handle);
dev->intr_handle = NULL;
}
dev->intr_handle = rte_intr_instance_alloc(...);
I'll fix this in v4.
>
> dpaa2_tm_init(dev);
>
> return 0;
> @@ -898,6 +929,7 @@ dpaa2_dev_rx_queue_setup(struct rte_eth_dev *dev,
> {
> struct dpaa2_dev_priv *priv = dev->data->dev_private;
> struct fsl_mc_io *dpni = dev->process_private;
> + bool dpcon_allocated = false;
> struct dpaa2_queue *dpaa2_q;
> struct dpni_queue cfg;
> uint8_t options = 0;
> @@ -938,6 +970,25 @@ dpaa2_dev_rx_queue_setup(struct rte_eth_dev *dev,
> dpaa2_q->bp_array = rte_dpaa2_bpid_info;
> dpaa2_q->offloads = rx_conf->offloads;
>
> + /* NAPI: grab a DPCON channel for dev_start to bind this FQ statically */
> + dpaa2_q->napi_sub_dpio = NULL;
>
>
> dpaa2_q->napi_sub_dpio is declared as RTE_ATOMIC(struct dpaa2_dpio_dev *) in
> dpaa2_hw_pvt.h, but here it is assigned with a plain = NULL instead of
> rte_atomic_store_explicit(...). This is inconsistent with the atomic stores
> used elsewhere (e.g. in dpaa2_dev_rx_queue_intr_unbind) and may cause
> memory-ordering issues on weakly-ordered architectures (ARM/aarch64).
>
> Please use:
>
> rte_atomic_store_explicit(&dpaa2_q->napi_sub_dpio, NULL,
> rte_memory_order_release);
I'll switch to rte_atomic_store_explicit() for consistency and MSVC
(where RTE_ATOMIC(x) is a plain type). But it's not an ordering bug:
the field is _Atomic (plain store is already seq_cst), and this is
rx_queue_setup, before dev_start, with no reader yet. So this is
cleanup, not a correctness fix, I'll change it in v4
Thanks,
Maxime
