From: Shani Peretz <shperetz@nvidia.com>
To: Gagandeep Singh <g.singh@nxp.com>, "stable@dpdk.org" <stable@dpdk.org>
Cc: "hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>
Subject: RE: [PATCH 23.11] bus/dpaa: improve cleanup
Date: Tue, 23 Dec 2025 07:47:47 +0000 [thread overview]
Message-ID: <MW4PR12MB74841C527B81D052035AB44BBFB5A@MW4PR12MB7484.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20251222061607.2685380-1-g.singh@nxp.com>
> -----Original Message-----
> From: Gagandeep Singh <g.singh@nxp.com>
> Sent: Monday, 22 December 2025 8:16
> To: stable@dpdk.org
> Cc: hemant.agrawal@nxp.com; Gagandeep Singh <g.singh@nxp.com>
> Subject: [PATCH 23.11] bus/dpaa: improve cleanup
>
> External email: Use caution opening links or attachments
>
>
> [ upstream commit 78ea4b4fcb52f786aeb1c470c730ea3e54e239d5 ]
>
> This patch addresses DPAA driver issues with the introduction of
> rte_eal_cleanup, which caused driver-specific destructors to fail due to
> memory cleanup.
> To resolve this, we remove the driver destructor and relocate the code to the
> bus cleanup function.
>
> So, this patch also implements DPAA bus cleanup.
>
> Fixes: ff9e112d7870 ("net/dpaa: add NXP DPAA PMD driver skeleton")
> Cc: stable@dpdk.org
>
> Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> ---
> drivers/bus/dpaa/base/qbman/qman_driver.c | 2 -
> drivers/bus/dpaa/dpaa_bus.c | 58 ++++++
> drivers/net/dpaa/dpaa_ethdev.c | 234 +++++++++++++++-------
> 3 files changed, 219 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/bus/dpaa/base/qbman/qman_driver.c
> b/drivers/bus/dpaa/base/qbman/qman_driver.c
> index 7a129a2d86..cdce6b777b 100644
> --- a/drivers/bus/dpaa/base/qbman/qman_driver.c
> +++ b/drivers/bus/dpaa/base/qbman/qman_driver.c
> @@ -228,8 +228,6 @@ int fsl_qman_fq_portal_destroy(struct qman_portal
> *qp)
> if (ret)
> pr_err("qman_free_global_portal() (%d)\n", ret);
>
> - kfree(qp);
> -
> process_portal_irq_unmap(cfg->irq);
>
> addr.cena = cfg->addr_virt[DPAA_PORTAL_CE]; diff --git
> a/drivers/bus/dpaa/dpaa_bus.c b/drivers/bus/dpaa/dpaa_bus.c index
> aaf2a5f43e..2f718ad7f1 100644
> --- a/drivers/bus/dpaa/dpaa_bus.c
> +++ b/drivers/bus/dpaa/dpaa_bus.c
> @@ -43,6 +43,7 @@
> #include <fsl_bman.h>
> #include <netcfg.h>
>
> +#define RTE_PRIORITY_102 102
> struct rte_dpaa_bus {
> struct rte_bus bus;
> TAILQ_HEAD(, rte_dpaa_device) device_list; @@ -56,6 +57,9 @@ struct
> netcfg_info *dpaa_netcfg;
>
> /* define a variable to hold the portal_key, once created.*/ static
> pthread_key_t dpaa_portal_key;
> +/* dpaa lcore specific portals */
> +struct dpaa_portal *dpaa_portals[RTE_MAX_LCORE] = {NULL}; static int
> +dpaa_bus_global_init;
>
> unsigned int dpaa_svr_family;
>
> @@ -377,6 +381,7 @@ int rte_dpaa_portal_init(void *arg)
>
> return ret;
> }
> + dpaa_portals[lcore] = DPAA_PER_LCORE_PORTAL;
>
> DPAA_BUS_LOG(DEBUG, "QMAN thread initialized");
>
> @@ -434,6 +439,8 @@ dpaa_portal_finish(void *arg)
> rte_free(dpaa_io_portal);
> dpaa_io_portal = NULL;
> DPAA_PER_LCORE_PORTAL = NULL;
> + dpaa_portals[rte_lcore_id()] = NULL;
> + DPAA_BUS_DEBUG("Portal cleanup done for lcore = %d",
> + rte_lcore_id());
> }
>
> static int
> @@ -712,6 +719,7 @@ rte_dpaa_bus_probe(void)
> break;
> }
> }
> + dpaa_bus_global_init = 1;
>
> return 0;
> }
> @@ -819,6 +827,55 @@ dpaa_bus_dev_iterate(const void *start, const char
> *str,
> return NULL;
> }
>
> +static int
> +dpaa_bus_cleanup(void)
> +{
> + struct rte_dpaa_device *dev, *tmp_dev;
> +
> + BUS_INIT_FUNC_TRACE();
> + RTE_TAILQ_FOREACH_SAFE(dev, &rte_dpaa_bus.device_list, next,
> tmp_dev) {
> + struct rte_dpaa_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!rte_dev_is_probed(&dev->device))
> + continue;
> + if (!drv || !drv->remove)
> + continue;
> + ret = drv->remove(dev);
> + if (ret < 0) {
> + rte_errno = errno;
> + return -1;
> + }
> + dev->driver = NULL;
> + dev->device.driver = NULL;
> + }
> + dpaa_portal_finish((void *)DPAA_PER_LCORE_PORTAL);
> + dpaa_bus_global_init = 0;
> + DPAA_BUS_DEBUG("Bus cleanup done");
> +
> + return 0;
> +}
> +
> +/* Adding destructor for double check in case non-gracefully
> + * exit.
> + */
> +RTE_FINI_PRIO(dpaa_cleanup, 102)
> +{
> + unsigned int lcore_id;
> +
> + if (!dpaa_bus_global_init)
> + return;
> +
> + /* cleanup portals in case non-graceful exit */
> + RTE_LCORE_FOREACH_WORKER(lcore_id) {
> + /* Check for non zero id */
> + dpaa_portal_finish((void *)dpaa_portals[lcore_id]);
> + }
> + dpaa_portal_finish((void *)DPAA_PER_LCORE_PORTAL);
> + dpaa_bus_global_init = 0;
> + DPAA_BUS_DEBUG("Worker thread clean up done"); }
> +
> static struct rte_dpaa_bus rte_dpaa_bus = {
> .bus = {
> .scan = rte_dpaa_bus_scan, @@ -829,6 +886,7 @@ static struct
> rte_dpaa_bus rte_dpaa_bus = {
> .plug = dpaa_bus_plug,
> .unplug = dpaa_bus_unplug,
> .dev_iterate = dpaa_bus_dev_iterate,
> + .cleanup = dpaa_bus_cleanup,
> },
> .device_list = TAILQ_HEAD_INITIALIZER(rte_dpaa_bus.device_list),
> .driver_list = TAILQ_HEAD_INITIALIZER(rte_dpaa_bus.driver_list),
> diff --git a/drivers/net/dpaa/dpaa_ethdev.c
> b/drivers/net/dpaa/dpaa_ethdev.c index 6fdbe80334..64f51b3517 100644
> --- a/drivers/net/dpaa/dpaa_ethdev.c
> +++ b/drivers/net/dpaa/dpaa_ethdev.c
> @@ -52,6 +52,7 @@
>
> #define CHECK_INTERVAL 100 /* 100ms */
> #define MAX_REPEAT_TIME 90 /* 9s (90 * 100ms) in total */
> +#define RTE_PRIORITY_103 103
>
> /* Supported Rx offloads */
> static uint64_t dev_rx_offloads_sup =
> @@ -287,10 +288,11 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
> }
>
> if (!(default_q || fmc_q)) {
> - if (dpaa_fm_config(dev,
> - eth_conf->rx_adv_conf.rss_conf.rss_hf)) {
> + ret = dpaa_fm_config(dev,
> + eth_conf->rx_adv_conf.rss_conf.rss_hf);
> + if (ret) {
> dpaa_write_fm_config_to_file();
> - DPAA_PMD_ERR("FM port configuration: Failed\n");
> + DPAA_PMD_ERR("FM port configuration:
> + Failed(%d)", ret);
> return -1;
> }
> dpaa_write_fm_config_to_file(); @@ -481,6 +483,7 @@ static int
> dpaa_eth_dev_close(struct rte_eth_dev *dev)
> struct rte_intr_handle *intr_handle;
> struct rte_eth_link *link = &dev->data->dev_link;
> struct dpaa_if *dpaa_intf = dev->data->dev_private;
> + struct qman_fq *fq;
> int loop;
> int ret;
>
> @@ -490,14 +493,17 @@ static int dpaa_eth_dev_close(struct rte_eth_dev
> *dev)
> return 0;
>
> if (!dpaa_intf) {
> - DPAA_PMD_WARN("Already closed or not started");
> - return -1;
> + DPAA_PMD_DEBUG("Already closed or not started");
> + return -ENOENT;
> }
>
> /* DPAA FM deconfig */
> if (!(default_q || fmc_q)) {
> - if (dpaa_fm_deconfig(dpaa_intf, dev->process_private))
> - DPAA_PMD_WARN("DPAA FM deconfig failed\n");
> + ret = dpaa_fm_deconfig(dpaa_intf, dev->process_private);
> + if (ret) {
> + DPAA_PMD_WARN("%s: FM deconfig failed(%d)",
> + dev->data->name, ret);
> + }
> }
>
> dpaa_dev = container_of(rdev, struct rte_dpaa_device, device); @@ -
> 505,17 +511,33 @@ static int dpaa_eth_dev_close(struct rte_eth_dev *dev)
> __fif = container_of(fif, struct __fman_if, __if);
>
> ret = dpaa_eth_dev_stop(dev);
> + if (ret) {
> + DPAA_PMD_WARN("%s: stop device failed(%d)",
> + dev->data->name, ret);
> + }
>
> /* Reset link to autoneg */
> - if (link->link_status && !link->link_autoneg)
> - dpaa_restart_link_autoneg(__fif->node_name);
> + if (link->link_status && !link->link_autoneg) {
> + ret = dpaa_restart_link_autoneg(__fif->node_name);
> + if (ret) {
> + DPAA_PMD_WARN("%s: restart link failed(%d)",
> + dev->data->name, ret);
> + }
> + }
>
> if (intr_handle && rte_intr_fd_get(intr_handle) &&
> dev->data->dev_conf.intr_conf.lsc != 0) {
> - dpaa_intr_disable(__fif->node_name);
> - rte_intr_callback_unregister(intr_handle,
> - dpaa_interrupt_handler,
> - (void *)dev);
> + ret = dpaa_intr_disable(__fif->node_name);
> + if (ret) {
> + DPAA_PMD_WARN("%s: disable interrupt failed(%d)",
> + dev->data->name, ret);
> + }
> + ret = rte_intr_callback_unregister(intr_handle,
> + dpaa_interrupt_handler, (void *)dev);
> + if (ret) {
> + DPAA_PMD_WARN("%s: unregister interrupt failed(%d)",
> + dev->data->name, ret);
> + }
> }
>
> /* release configuration memory */ @@ -523,26 +545,63 @@ static int
> dpaa_eth_dev_close(struct rte_eth_dev *dev)
>
> /* Release RX congestion Groups */
> if (dpaa_intf->cgr_rx) {
> - for (loop = 0; loop < dpaa_intf->nb_rx_queues; loop++)
> - qman_delete_cgr(&dpaa_intf->cgr_rx[loop]);
> + for (loop = 0; loop < dpaa_intf->nb_rx_queues; loop++) {
> + ret = qman_delete_cgr(&dpaa_intf->cgr_rx[loop]);
> + if (ret) {
> + DPAA_PMD_WARN("%s: delete rxq%d's cgr err(%d)",
> + dev->data->name, loop, ret);
> + }
> + }
> }
>
> rte_free(dpaa_intf->cgr_rx);
> dpaa_intf->cgr_rx = NULL;
> /* Release TX congestion Groups */
> if (dpaa_intf->cgr_tx) {
> - for (loop = 0; loop < MAX_DPAA_CORES; loop++)
> - qman_delete_cgr(&dpaa_intf->cgr_tx[loop]);
> + for (loop = 0; loop < MAX_DPAA_CORES; loop++) {
> + ret = qman_delete_cgr(&dpaa_intf->cgr_tx[loop]);
> + if (ret) {
> + DPAA_PMD_WARN("%s: delete txq%d's cgr err(%d)",
> + dev->data->name, loop, ret);
> + }
> + }
> rte_free(dpaa_intf->cgr_tx);
> dpaa_intf->cgr_tx = NULL;
> }
>
> + /* Freeing queue specific portals */
> + for (loop = 0; loop < dpaa_intf->nb_rx_queues; loop++) {
> + if (!dpaa_intf->rx_queues)
> + break;
> +
> + fq = &dpaa_intf->rx_queues[loop];
> + if (fq->qp_initialized) {
> + rte_dpaa_portal_fq_close(fq);
> + fq->qp_initialized = 0;
> + }
> + }
> +
> rte_free(dpaa_intf->rx_queues);
> dpaa_intf->rx_queues = NULL;
>
> rte_free(dpaa_intf->tx_queues);
> dpaa_intf->tx_queues = NULL;
>
> + if (dpaa_intf->port_handle) {
> + ret = dpaa_fm_deconfig(dpaa_intf, fif);
> + if (ret) {
> + DPAA_PMD_WARN("%s: FM deconfig failed(%d)",
> + dev->data->name, ret);
> + }
> + }
> + if (fif->num_profiles) {
> + ret = dpaa_port_vsp_cleanup(dpaa_intf, fif);
> + if (ret) {
> + DPAA_PMD_WARN("%s: cleanup VSP failed(%d)",
> + dev->data->name, ret);
> + }
> + }
> +
> return ret;
> }
>
> @@ -1363,6 +1422,8 @@ dpaa_flow_ctrl_set(struct rte_eth_dev *dev, {
> struct dpaa_if *dpaa_intf = dev->data->dev_private;
> struct rte_eth_fc_conf *net_fc;
> + struct fman_if *fm_if = dev->process_private;
> + int ret;
>
> PMD_INIT_FUNC_TRACE();
>
> @@ -1381,19 +1442,31 @@ dpaa_flow_ctrl_set(struct rte_eth_dev *dev,
> return -EINVAL;
> }
>
> - if (fc_conf->mode == RTE_ETH_FC_NONE) {
> + if (fc_conf->mode == RTE_ETH_FC_NONE)
> return 0;
> - } else if (fc_conf->mode == RTE_ETH_FC_TX_PAUSE ||
> - fc_conf->mode == RTE_ETH_FC_FULL) {
> - fman_if_set_fc_threshold(dev->process_private,
> +
> + if (fc_conf->mode != RTE_ETH_FC_TX_PAUSE &&
> + fc_conf->mode != RTE_ETH_FC_FULL)
> + goto save_fc;
> +
> + ret = fman_if_set_fc_threshold(fm_if,
> fc_conf->high_water,
> fc_conf->low_water,
> dpaa_intf->bp_info->bpid);
> - if (fc_conf->pause_time)
> - fman_if_set_fc_quanta(dev->process_private,
> - fc_conf->pause_time);
> + if (ret) {
> + DPAA_PMD_ERR("Set %s's fc on bpid(%d) err(%d)",
> + dev->data->name, dpaa_intf->bp_info->bpid,
> + ret);
> + }
> + if (fc_conf->pause_time) {
> + ret = fman_if_set_fc_quanta(fm_if, fc_conf->pause_time);
> + if (ret) {
> + DPAA_PMD_ERR("Set %s's fc pause time err(%d)",
> + dev->data->name, ret);
> + }
> }
>
> +save_fc:
> /* Save the information in dpaa device */
> net_fc->pause_time = fc_conf->pause_time;
> net_fc->high_water = fc_conf->high_water; @@ -1486,13 +1559,15 @@
> dpaa_dev_rss_hash_update(struct rte_eth_dev *dev, {
> struct rte_eth_dev_data *data = dev->data;
> struct rte_eth_conf *eth_conf = &data->dev_conf;
> + int ret;
>
> PMD_INIT_FUNC_TRACE();
>
> if (!(default_q || fmc_q)) {
> - if (dpaa_fm_config(dev, rss_conf->rss_hf)) {
> - DPAA_PMD_ERR("FM port configuration: Failed\n");
> - return -1;
> + ret = dpaa_fm_config(dev, rss_conf->rss_hf);
> + if (ret) {
> + DPAA_PMD_ERR("FM port configuration: Failed(%d)", ret);
> + return ret;
> }
> eth_conf->rx_adv_conf.rss_conf.rss_hf = rss_conf->rss_hf;
> } else {
> @@ -1954,8 +2029,8 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
> /* Each device can not have more than DPAA_MAX_NUM_PCD_QUEUES
> RX
> * queues.
> */
> - if (num_rx_fqs < 0 || num_rx_fqs > DPAA_MAX_NUM_PCD_QUEUES) {
> - DPAA_PMD_ERR("Invalid number of RX queues\n");
> + if (num_rx_fqs > DPAA_MAX_NUM_PCD_QUEUES) {
> + DPAA_PMD_ERR("Invalid number of RX queues(%d)",
> + num_rx_fqs);
> return -EINVAL;
> }
>
> @@ -2197,8 +2272,8 @@ rte_dpaa_probe(struct rte_dpaa_driver *dpaa_drv,
> eth_dev->dev_ops = &dpaa_devops;
>
> ret = dpaa_dev_init_secondary(eth_dev);
> - if (ret != 0) {
> - RTE_LOG(ERR, PMD, "secondary dev init failed\n");
> + if (ret) {
> + DPAA_PMD_ERR("secondary dev init failed(%d)",
> + ret);
> return ret;
> }
>
> @@ -2213,9 +2288,10 @@ rte_dpaa_probe(struct rte_dpaa_driver
> *dpaa_drv,
> }
>
> if (!(default_q || fmc_q)) {
> - if (dpaa_fm_init()) {
> - DPAA_PMD_ERR("FM init failed\n");
> - return -1;
> + ret = dpaa_fm_init();
> + if (ret) {
> + DPAA_PMD_ERR("FM init failed(%d)", ret);
> + return ret;
> }
> }
>
> @@ -2290,59 +2366,71 @@ rte_dpaa_probe(struct rte_dpaa_driver
> *dpaa_drv,
> return diag;
> }
>
> -static int
> -rte_dpaa_remove(struct rte_dpaa_device *dpaa_dev)
> +/* Adding destructor for double check in case non-gracefully
> + * exit.
> + */
> +RTE_FINI_PRIO(dpaa_finish, 103)
> {
> - struct rte_eth_dev *eth_dev;
> - int ret;
> + struct dpaa_if *dpaa_intf;
> + int loop;
> + struct qman_fq *fq;
> + uint16_t portid;
> + struct rte_eth_dev *dev;
>
> PMD_INIT_FUNC_TRACE();
>
> - eth_dev = dpaa_dev->eth_dev;
> - dpaa_eth_dev_close(eth_dev);
> - dpaa_valid_dev--;
> - if (!dpaa_valid_dev)
> - rte_mempool_free(dpaa_tx_sg_pool);
> - ret = rte_eth_dev_release_port(eth_dev);
> -
> - return ret;
> -}
> -
> -static void __attribute__((destructor(102))) dpaa_finish(void) -{
> /* For secondary, primary will do all the cleanup */
> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> return;
>
> + if (!is_global_init)
> + return;
> +
> if (!(default_q || fmc_q)) {
> - unsigned int i;
> -
> - for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> - if (rte_eth_devices[i].dev_ops == &dpaa_devops) {
> - struct rte_eth_dev *dev = &rte_eth_devices[i];
> - struct dpaa_if *dpaa_intf =
> - dev->data->dev_private;
> - struct fman_if *fif =
> - dev->process_private;
> - if (dpaa_intf->port_handle)
> - if (dpaa_fm_deconfig(dpaa_intf, fif))
> - DPAA_PMD_WARN("DPAA FM "
> - "deconfig failed\n");
> - if (fif->num_profiles) {
> - if (dpaa_port_vsp_cleanup(dpaa_intf,
> - fif))
> - DPAA_PMD_WARN("DPAA FM vsp cleanup failed\n");
> - }
> + if (dpaa_fm_term())
> + DPAA_PMD_WARN("DPAA FM term failed");
> +
> + DPAA_PMD_INFO("DPAA fman cleaned up");
> + }
> +
> + RTE_ETH_FOREACH_DEV(portid) {
> + dev = &rte_eth_devices[portid];
> + if (strcmp(dev->device->driver->name,
> + rte_dpaa_pmd.driver.name))
> + continue;
> + dpaa_intf = dev->data->dev_private;
> + /* Freeing queue specific portals */
> + for (loop = 0; loop < dpaa_intf->nb_rx_queues; loop++) {
> + if (!dpaa_intf->rx_queues)
> + break;
> +
> + fq = &dpaa_intf->rx_queues[loop];
> + if (fq->qp_initialized) {
> + rte_dpaa_portal_fq_close(fq);
> + fq->qp_initialized = 0;
> }
> }
> - if (is_global_init)
> - if (dpaa_fm_term())
> - DPAA_PMD_WARN("DPAA FM term failed\n");
> + }
> + is_global_init = 0;
> +}
> +
> +static int
> +rte_dpaa_remove(struct rte_dpaa_device *dpaa_dev) {
> + struct rte_eth_dev *eth_dev;
> + int ret;
>
> - is_global_init = 0;
> + PMD_INIT_FUNC_TRACE();
>
> - DPAA_PMD_INFO("DPAA fman cleaned up");
> + eth_dev = dpaa_dev->eth_dev;
> + dpaa_eth_dev_close(eth_dev);
> + ret = rte_eth_dev_release_port(eth_dev);
> + dpaa_valid_dev--;
> + if (!dpaa_valid_dev) {
> + rte_mempool_free(dpaa_tx_sg_pool);
> + dpaa_finish();
> }
> + return ret;
> }
>
> static struct rte_dpaa_driver rte_dpaa_pmd = {
> --
> 2.25.1
Thanks Gagandeep, I applied the patch to 23.11 branch
prev parent reply other threads:[~2025-12-23 7:47 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-22 6:16 Gagandeep Singh
2025-12-23 7:47 ` Shani Peretz [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MW4PR12MB74841C527B81D052035AB44BBFB5A@MW4PR12MB7484.namprd12.prod.outlook.com \
--to=shperetz@nvidia.com \
--cc=g.singh@nxp.com \
--cc=hemant.agrawal@nxp.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).