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

[Subbaraya Sundeep]

Hi Paolo,

On 2026-04-02 at 16:50:11, Paolo Abeni (pabeni@xxxxxxxxxx) wrote:
> 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):

For now we want backpressure to be disabled since we do not have setup
to test it at the moment. I will write a comment saying disable backpressure
in Halo context and revisit later?
> 
>     /* 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?
>
Nice catch. Will fix this.

Thanks,
Sundeep
 
> >  	mutex_lock(&mbox->lock);
> >  	free_req = otx2_mbox_alloc_msg_nix_lf_free(mbox);
> >  	if (free_req) {
> 
> [ ... ]
>