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

[Paolo Abeni <pabeni@redhat.com>]

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

[ ... ]