Re: [PATCH v3 5/6] net/dpaa2: support Rx queue interrupts

[Hemant Agrawal <hemant.agrawal@oss.nxp.com>]

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


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


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

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