Re: [net-next PATCH v6 3/5] octeontx2-af: npa: cn20k: Add DPC support

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
octeontx2-af: npa: cn20k: Add DPC support

This adds NPA_CN20K_DPC_ALLOC/FREE mbox handlers so each PF/VF can request
one of 32 shared diagnostic and performance counters with a configuration
written to NPA_AF_DPCX_CFG, with LF-to-counter mappings tracked in
NPA_AF_DPC_PERMITX. Counter mappings owned by a pcifunc are released on LF
free and teardown.

A few questions below.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npa.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npa.c
> index fe8f926c8b75..7c9cd28de3ee 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npa.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npa.c

[ ... ]

> +static int npa_cn20k_dpc_alloc(struct rvu *rvu,
> +			       struct npa_cn20k_dpc_alloc_req *req,
> +			       struct npa_cn20k_dpc_alloc_rsp *rsp)
> +{
> +	struct rvu_hwinfo *hw = rvu->hw;
> +	u16 pcifunc = req->hdr.pcifunc;
> +	int cntr, lf, blkaddr, ridx;
> +	struct rvu_block *block;
> +	struct rvu_pfvf *pfvf;
> +	u64 val, lfmask;
> +
> +	pfvf = rvu_get_pfvf(rvu, pcifunc);
> +
> +	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPA, 0);
> +	if (!pfvf->npalf || blkaddr < 0)
> +		return NPA_AF_ERR_AF_LF_INVALID;
> +
> +	mutex_lock(&rvu->rsrc_lock);
> +
> +	block = &hw->block[blkaddr];
> +	lf = rvu_get_lf(rvu, block, pcifunc, 0);

[Critical]
Can this self-deadlock?  rvu_get_lf() in rvu.c does:

	int rvu_get_lf(struct rvu *rvu, struct rvu_block *block, u16 pcifunc, u16 slot)
	{
		...
		mutex_lock(&rvu->rsrc_lock);
		for (lf = 0; lf < block->lf.max; lf++) {
		...

and rvu->rsrc_lock is a plain non-recursive struct mutex.  The sibling
npa_cn20k_dpc_free() calls rvu_get_lf() before taking rsrc_lock, which
appears to be the intended order.  Should the rvu_get_lf() call here move
above the mutex_lock() too?

> +	if (lf < 0) {
> +		mutex_unlock(&rvu->rsrc_lock);
> +		return NPA_AF_ERR_AF_LF_INVALID;
> +	}
> +
> +	/* allocate a new counter */
> +	cntr = rvu_alloc_rsrc(&rvu->npa_dpc);
> +	if (cntr < 0) {
> +		mutex_unlock(&rvu->rsrc_lock);
> +		return cntr;

[Low]
Other failure paths in this file return NPA_AF_ERR_* values
(NPA_AF_ERR_AF_LF_INVALID, NPA_AF_ERR_PARAM, ...), but on bitmap
exhaustion this returns the raw -ENOSPC/-EINVAL from rvu_alloc_rsrc().
Should this be normalised to an NPA_AF_ERR_* code for consistency with the
rest of the AF mbox surface?

[Medium]
Is there a reason a single pcifunc cannot consume all 32 DPC counters?
rvu_alloc_rsrc(&rvu->npa_dpc) is called with no per-pcifunc accounting or
quota, ownership is only encoded in the NPA_AF_DPC_PERMITX register.  Any
PF/VF that loops NPA_CN20K_DPC_ALLOC can drain the pool and starve every
other LF.  Other shared AF resources (MCAM counters, nix_ipolicer
profilers) keep a per-pcifunc count for exactly this reason.  Would a
similar per-pcifunc cap be appropriate here?

> +	}
> +
> +	rsp->cntr_id = cntr;
> +
> +	/* Consumers can configure to count ALLOC/FREE requests only */
> +	rvu_write64(rvu, blkaddr, NPA_AF_DPCX_CFG(cntr), req->dpc_conf);

[High]
The comment says consumers can configure to count ALLOC/FREE requests
only, but req->dpc_conf is a u16 supplied by an arbitrary PF/VF (including
guest VFs through the AF mbox path) and is written verbatim into the CSR
with no validation or masking.  Should the AF mask req->dpc_conf to the
documented event-select bits, or reject illegal values with
NPA_AF_ERR_PARAM, rather than forwarding an opaque value to the register?

> +
> +	/* 0 to 63 lfs -> idx 0, 64 - 127 lfs -> idx 1 */
> +	ridx = lf >> 6;
> +	lfmask = BIT_ULL(ridx ? lf - NPA_DPC_LFS_PER_REG : lf);
> +
> +	ridx = 2 * cntr + ridx;

[Medium]
What bounds lf to [0, 127] here?  block->lf.max for NPA comes from a
12-bit HW field, and rvu_get_lf() can return any LF up to that maximum.
For lf >= 128:

	lfmask = BIT_ULL(lf - 64);

becomes a shift count >= 64 on a u64, which is undefined in C, and:

	ridx = 2 * cntr + (lf >> 6);

walks past the documented NPA_AF_DPC_PERMITX array, doing AF MMIO against
whatever register sits at the computed offset.  The same arithmetic
appears in npa_cn20k_dpc_free(), so a misbehaving caller could end up
clearing permission bits and freeing a counter slot that belongs to
another LF.  Should there be a guard like
if (lf >= 2 * NPA_DPC_LFS_PER_REG) return NPA_AF_ERR_PARAM; in both
paths?

> +	/* Give permission for LF access */
> +	val = rvu_read64(rvu, blkaddr, NPA_AF_DPC_PERMITX(ridx));
> +	val |= lfmask;
> +	rvu_write64(rvu, blkaddr, NPA_AF_DPC_PERMITX(ridx), val);
> +
> +	mutex_unlock(&rvu->rsrc_lock);
> +
> +	return 0;
> +}

[ ... ]

> +static int npa_cn20k_dpc_free(struct rvu *rvu,
> +			      struct npa_cn20k_dpc_free_req *req)
> +{

[ ... ]

> +	/* Revert permission */
> +	val &= ~lfmask;
> +	rvu_write64(rvu, blkaddr, NPA_AF_DPC_PERMITX(ridx), val);
> +
> +	/* Free this counter */
> +	rvu_free_rsrc(&rvu->npa_dpc, req->cntr_id);

[Medium]
On free, the LF bit in NPA_AF_DPC_PERMITX is cleared and the bitmap slot
is returned, but NPA_AF_DPCX_CFG(cntr) and the counter value itself are
left as-is.  When the slot is later reallocated, the previous owner's
configuration and accumulated count remain in HW until the new owner's
write to dpc_conf takes effect, and the first reads of the counter return
the prior accumulation.

The commit message says:

    Also ensure all the LF to DPC counter mappings are cleared
    at the time of LF free/teardown.

Should the free path also zero NPA_AF_DPCX_CFG(cntr) and the counter so a
recycled slot starts clean?  And should rvu_npa_freemem() quiesce any
still-armed counters before the bitmap is freed?

> +
> +	mutex_unlock(&rvu->rsrc_lock);
> +
> +	return 0;
> +}
-- 
This is an AI-generated review.