Re: [PATCH v2 1/5] bus/dpaa: add FQ shutdown and improve logging

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

On 08-10-2025 10:05, Gagandeep Singh wrote:
> adding a FQ shutdown functionality to ensure proper cleanup of
> frame queues during queue setup. This helps reset the
> queues reliably and prevents potential resource leaks or
> stale state issues.
>
> Additionally, update logging to use DPAA_BUS_ERR instead
> of pr_err for better consistency and clarity in error
> reporting within the DPAA bus subsystem.
Can you break the changes into two patches? logging  and cleanup patch?
> These changes enhance maintainability and improve
> debugging experience.
>
> Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> ---
>   drivers/bus/dpaa/base/qbman/qman.c  | 394 +++++++++++++++++++++++++---
>   drivers/bus/dpaa/include/fsl_qman.h |   3 +
>   drivers/net/dpaa/dpaa_ethdev.c      |   3 +
>   3 files changed, 369 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/bus/dpaa/base/qbman/qman.c b/drivers/bus/dpaa/base/qbman/qman.c
> index 60087c55a1..6ce3690366 100644
> --- a/drivers/bus/dpaa/base/qbman/qman.c
> +++ b/drivers/bus/dpaa/base/qbman/qman.c
> @@ -10,7 +10,8 @@
>   #include <bus_dpaa_driver.h>
>   #include <rte_eventdev.h>
>   #include <rte_byteorder.h>
> -
> +#include <rte_dpaa_logs.h>
> +#include <eal_export.h>
>   #include <dpaa_bits.h>
>   
>   /* Compilation constants */
> @@ -137,7 +138,7 @@ static inline int table_push_fq(struct qman_portal *p, struct qman_fq *fq)
>   	int ret = fqtree_push(&p->retire_table, fq);
>   
>   	if (ret)
> -		pr_err("ERROR: double FQ-retirement %d\n", fq->fqid);
> +		DPAA_BUS_ERR("ERROR: double FQ-retirement %d", fq->fqid);
DPAA_BUS_ERR already adds ERR, why add ERROR again?
>   	return ret;
>   }
>   
> @@ -161,7 +162,7 @@ int qman_setup_fq_lookup_table(size_t num_entries)
>   	/* Allocate 1 more entry since the first entry is not used */
>   	qman_fq_lookup_table = vmalloc((num_entries * sizeof(void *)));
>   	if (!qman_fq_lookup_table) {
> -		pr_err("QMan: Could not allocate fq lookup table\n");
> +		DPAA_BUS_ERR("QMan: Could not allocate fq lookup table");
>   		return -ENOMEM;
>   	}
>   	memset(qman_fq_lookup_table, 0, num_entries * sizeof(void *));
> @@ -349,7 +350,8 @@ static int drain_mr_fqrni(struct qm_portal *p)
>   	}
>   	if ((msg->ern.verb & QM_MR_VERB_TYPE_MASK) != QM_MR_VERB_FQRNI) {
>   		/* We aren't draining anything but FQRNIs */
> -		pr_err("Found verb 0x%x in MR\n", msg->ern.verb);
> +		DPAA_BUS_ERR("Found verb 0x%x and after mask = 0x%x in MR",
> +			msg->ern.verb, msg->ern.verb & QM_MR_VERB_TYPE_MASK);
>   		return -1;
>   	}
>   	qm_mr_next(p);
> @@ -423,11 +425,11 @@ static inline void qm_eqcr_finish(struct qm_portal *portal)
>   	DPAA_ASSERT(!eqcr->busy);
>   #endif
>   	if (pi != EQCR_PTR2IDX(eqcr->cursor))
> -		pr_crit("losing uncommitted EQCR entries\n");
> +		DPAA_BUS_ERR("losing uncommitted EQCR entries");
>   	if (ci != eqcr->ci)
> -		pr_crit("missing existing EQCR completions\n");
> +		DPAA_BUS_ERR("missing existing EQCR completions");
>   	if (eqcr->ci != EQCR_PTR2IDX(eqcr->cursor))
> -		pr_crit("EQCR destroyed unquiesced\n");
> +		DPAA_BUS_ERR("EQCR destroyed unquiesced");
>   }
>   
>   static inline int qm_dqrr_init(struct qm_portal *portal,
> @@ -515,6 +517,7 @@ qman_init_portal(struct qman_portal *portal,
>   	int ret;
>   	u32 isdr;
>   
> +
>   	p = &portal->p;
>   
>   	if (!c)
> @@ -540,30 +543,68 @@ qman_init_portal(struct qman_portal *portal,
>   	 */
>   	if (qm_eqcr_init(p, qm_eqcr_pvb,
>   			 portal->use_eqcr_ci_stashing, 1)) {
> -		pr_err("Qman EQCR initialisation failed\n");
> +		DPAA_BUS_ERR("Qman EQCR initialisation failed");
> +		goto fail_eqcr;
> +	}
> +	if (qm_dqrr_init(p, c, qm_dqrr_dpush, qm_dqrr_pvb,
> +			 qm_dqrr_cdc, DQRR_MAXFILL)) {
> +		DPAA_BUS_ERR("Qman DQRR initialisation failed");
> +		goto fail_dqrr;
> +	}
> +	if (qm_mr_init(p, qm_mr_pvb, qm_mr_cci)) {
> +		DPAA_BUS_ERR("Qman MR initialisation failed");
> +		goto fail_mr;
> +	}
> +	if (qm_mc_init(p)) {
> +		DPAA_BUS_ERR("Qman MC initialisation failed");
> +		goto fail_mc;
> +	}
> +
So you are doing init two times?  Init, reset and init ?