Re: [net-next,v4,4/4] octeontx2-pf: cn20k: Use unified Halo context

Paolo Abeni
2026-04-02T11:20:25Z

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
octeontx2-pf: cn20k: Use unified Halo context

This commit replaces the separate aura and pool contexts with the unified
Halo context available in CN20K hardware for managing network device pools
and buffer allocation.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c

[ ... ]

> @@ -516,85 +507,8 @@ int cn20k_tc_alloc_entry(struct otx2_nic *nic,
>
> return 0;
> }
> -
> -static int cn20k_aura_aq_init(struct otx2_nic *pfvf, int aura_id,
> - int pool_id, int numptrs)
> -{
> - struct npa_cn20k_aq_enq_req *aq;
> - struct otx2_pool *pool;
> - u8 bpid_idx;
> - int err;
> -
> - pool = &pfvf->qset.pool[pool_id];
> -
> - /* Allocate memory for HW to update Aura count.
> - * Alloc one cache line, so that it fits all FC_STYPE modes.
> - */
> - if (!pool->fc_addr) {
> - err = qmem_alloc(pfvf->dev, &pool->fc_addr, 1, OTX2_ALIGN);
> - if (err)
> - return err;
> - }
> -
> - /* Initialize this aura's context via AF */
> - aq = otx2_mbox_alloc_msg_npa_cn20k_aq_enq(&pfvf->mbox);
> - if (!aq) {
> - /* Shared mbox memory buffer is full, flush it and retry */
> - err = otx2_sync_mbox_msg(&pfvf->mbox);
> - if (err)
> - return err;
> - aq = otx2_mbox_alloc_msg_npa_cn20k_aq_enq(&pfvf->mbox);
> - if (!aq)
> - return -ENOMEM;
> - }
> -
> - aq->aura_id = aura_id;
> -
> - /* Will be filled by AF with correct pool context address */
> - aq->aura.pool_addr = pool_id;
> - aq->aura.pool_caching = 1;
> - aq->aura.shift = ilog2(numptrs) - 8;
> - aq->aura.count = numptrs;
> - aq->aura.limit = numptrs;
> - aq->aura.avg_level = 255;
> - aq->aura.ena = 1;
> - aq->aura.fc_ena = 1;
> - aq->aura.fc_addr = pool->fc_addr->iova;
> - aq->aura.fc_hyst_bits = 0; /* Store count on all updates */
>
> - /* Enable backpressure for RQ aura */
> - if (aura_id < pfvf->hw.rqpool_cnt && !is_otx2_lbkvf(pfvf->pdev)) {
> - aq->aura.bp_ena = 0;
> - /* If NIX1 LF is attached then specify NIX1_RX.
> - *
> - * Below NPA_AURA_S[BP_ENA] is set according to the
> - * NPA_BPINTF_E enumeration given as:
> - * 0x0 + a*0x1 where 'a' is 0 for NIX0_RX and 1 for NIX1_RX so
> - * NIX0_RX is 0x0 + 0*0x1 = 0
> - * NIX1_RX is 0x0 + 1*0x1 = 1
> - * But in HRM it is given that
> - * "NPA_AURA_S[BP_ENA](w1[33:32]) - Enable aura backpressure to
> - * NIX-RX based on [BP] level. One bit per NIX-RX; index
> - * enumerated by NPA_BPINTF_E."
> - */
> - if (pfvf->nix_blkaddr == BLKADDR_NIX1)
> - aq->aura.bp_ena = 1;
> -
> - bpid_idx = cn20k_aura_bpid_idx(pfvf, aura_id);
> - aq->aura.bpid = pfvf->bpid[bpid_idx];
> -
> - /* Set backpressure level for RQ's Aura */
> - aq->aura.bp = RQ_BP_LVL_AURA;
> - }
> -
> - /* Fill AQ info */
> - aq->ctype = NPA_AQ_CTYPE_AURA;
> - aq->op = NPA_AQ_INSTOP_INIT;
> -
> - return 0;
> -}
> -
> -static int cn20k_pool_aq_init(struct otx2_nic *pfvf, u16 pool_id,
> +static int cn20k_halo_aq_init(struct otx2_nic *pfvf, u16 pool_id,
> int stack_pages, int numptrs, int buf_size,
> int type)
> {
> @@ -610,36 +524,57 @@ struct page_pool_params pp_params = { 0 };
> if (err)
> return err;
>
> + /* Allocate memory for HW to update Aura count.
> + * Alloc one cache line, so that it fits all FC_STYPE modes.
> + */
> + if (!pool->fc_addr) {
> + err = qmem_alloc(pfvf->dev, &pool->fc_addr, 1, OTX2_ALIGN);
> + if (err) {
> + qmem_free(pfvf->dev, pool->stack);
> + return err;
> + }
> + }
> +
> pool->rbsize = buf_size;
>
> - /* Initialize this pool's context via AF */
> + /* Initialize this aura's context via AF */
> aq = otx2_mbox_alloc_msg_npa_cn20k_aq_enq(&pfvf->mbox);
> if (!aq) {
> /* Shared mbox memory buffer is full, flush it and retry */
> err = otx2_sync_mbox_msg(&pfvf->mbox);
> - if (err) {
> - qmem_free(pfvf->dev, pool->stack);
> - return err;
> - }
> + if (err)
> + goto free_mem;
> aq = otx2_mbox_alloc_msg_npa_cn20k_aq_enq(&pfvf->mbox);
> if (!aq) {
> - qmem_free(pfvf->dev, pool->stack);
> - return -ENOMEM;
> + err = -ENOMEM;
> + goto free_mem;
> }
> }
>
> aq->aura_id = pool_id;
> - aq->pool.stack_base = pool->stack->iova;
> - aq->pool.stack_caching = 1;
> - aq->pool.ena = 1;
> - aq->pool.buf_size = buf_size / 128;
> - aq->pool.stack_max_pages = stack_pages;
> - aq->pool.shift = ilog2(numptrs) - 8;
> - aq->pool.ptr_start = 0;
> - aq->pool.ptr_end = ~0ULL;
>
> + aq->halo.stack_base = pool->stack->iova;
> + aq->halo.stack_caching = 1;
> + aq->halo.ena = 1;
> + aq->halo.buf_size = buf_size / 128;
> + aq->halo.stack_max_pages = stack_pages;
> + aq->halo.shift = ilog2(numptrs) - 8;
> + aq->halo.ptr_start = 0;
> + aq->halo.ptr_end = ~0ULL;
> +
> + aq->halo.avg_level = 255;
> + aq->halo.fc_ena = 1;
> + aq->halo.fc_addr = pool->fc_addr->iova;
> + aq->halo.fc_hyst_bits = 0; /* Store count on all updates */
> +
> + if (pfvf->npa_dpc_valid) {
> + aq->halo.op_dpc_ena = 1;
> + aq->halo.op_dpc_set = pfvf->npa_dpc;
> + }
> + aq->halo.unified_ctx = 1;
^^^^

Does this code properly configure backpressure for receive queues? The old
cn20k_aura_aq_init configured NPA-level backpressure by setting bp_ena,
bpid, and bp fields in the aura context. This caused the hardware to signal
backpressure to NIX-RX when receive buffers filled to 85% (RQ_BP_LVL_AURA):

/* Enable backpressure for RQ aura */
if (aura_id < pfvf->hw.rqpool_cnt && !is_otx2_lbkvf(pfvf->pdev)) {
aq->aura.bp_ena = 0;
if (pfvf->nix_blkaddr == BLKADDR_NIX1)
aq->aura.bp_ena = 1;

bpid_idx = cn20k_aura_bpid_idx(pfvf, aura_id);
aq->aura.bpid = pfvf->bpid[bpid_idx];

/* Set backpressure level for RQ's Aura */
aq->aura.bp = RQ_BP_LVL_AURA;
}

The new cn20k_halo_aq_init does not set the equivalent Halo context fields
(bp_ena_0, bpid_0, bp_0). Looking at struct npa_cn20k_halo_s in the AF
headers, these fields exist in the Halo context.

The AF side rvu_npa_aq_enq_inst() does not auto-configure backpressure
during INIT operations. The AF teardown code rvu_npa_halo_hwctx_disable()
explicitly clears bp_ena_0 through bp_ena_7, confirming these fields are
expected to be set during initialization.

Without NPA-level backpressure configuration, wouldn't CN20K devices fail
to trigger backpressure when aura buffers fill up, leading to packet drops
under heavy traffic load?

>
> /* Fill AQ info */
> - aq->ctype = NPA_AQ_CTYPE_POOL;
> + aq->ctype = NPA_AQ_CTYPE_HALO;
> aq->op = NPA_AQ_INSTOP_INIT;

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> @@ -1651,6 +1651,9 @@ int otx2_init_hw_resources(struct otx2_nic *pf)
> if (!is_otx2_lbkvf(pf->pdev))
> otx2_nix_config_bp(pf, true);
>
> + if (is_cn20k(pf->pdev))
> + cn20k_npa_alloc_dpc(pf);
> +
> /* Init Auras and pools used by NIX RQ, for free buffer ptrs */
> err = otx2_rq_aura_pool_init(pf);
> if (err) {
> @@ -1726,6 +1729,8 @@
> otx2_ctx_disable(mbox, NPA_AQ_CTYPE_AURA, true);
> otx2_aura_pool_free(pf);
> err_free_nix_lf:
> + if (pf->npa_dpc_valid)
> + cn20k_npa_free_dpc(pf);

Is there a potential double-free of the NPA DPC counter via the
rvu_rep_rsrc_init error path?

cn20k_npa_free_dpc() does not clear nic->npa_dpc_valid after successfully
freeing the DPC counter. If otx2_init_hw_resources() fails after
cn20k_npa_alloc_dpc() succeeds (e.g., otx2_rq_aura_pool_init() returns an
error), the error cleanup at err_free_nix_lf calls cn20k_npa_free_dpc(pf)
which frees the counter but leaves npa_dpc_valid=true.

The caller rvu_rep_rsrc_init() then calls otx2_free_hw_resources(priv) at
its err_free_rsrc label:

rvu_rep_rsrc_init()
otx2_init_hw_resources(priv)
cn20k_npa_alloc_dpc(pf) [succeeds]
otx2_rq_aura_pool_init(pf) [fails]
goto err_free_nix_lf
cn20k_npa_free_dpc(pf) [frees counter, npa_dpc_valid still true]
goto err_free_rsrc
otx2_free_hw_resources(priv)
cn20k_npa_free_dpc(pf) [frees same counter again]

Looking at cn20k_npa_free_dpc() in cn20k.c:

int cn20k_npa_free_dpc(struct otx2_nic *nic)
{
struct npa_cn20k_dpc_free_req *req;
int err;

if (!nic->npa_dpc_valid)
return 0;

mutex_lock(&nic->mbox.lock);

req = otx2_mbox_alloc_msg_npa_cn20k_dpc_free(&nic->mbox);
if (!req) {
mutex_unlock(&nic->mbox.lock);
return -ENOMEM;
}

req->cntr_id = nic->npa_dpc;

err = otx2_sync_mbox_msg(&nic->mbox);

mutex_unlock(&nic->mbox.lock);

return err;
}

Since npa_dpc_valid is still true, the second call passes the guard check
and sends another mbox message to free the same counter ID. Shouldn't
cn20k_npa_free_dpc() clear nic->npa_dpc_valid after the successful
otx2_sync_mbox_msg() call to prevent this double-free?

> mutex_lock(&mbox->lock);
> free_req = otx2_mbox_alloc_msg_nix_lf_free(mbox);
> if (free_req) {

[ ... ]