Re: [net-next,v8,08/13] octeontx2-af: npc: cn20k: Add new mailboxes for CN20K silicon

[Jakub Kicinski <kuba@kernel.org>]

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-af: npc: cn20k: Add new mailboxes for CN20K silicon

This patch adds new mailboxes for CN20K silicon to support an updated
struct mcam_entry with 8 keywords instead of 7, maintaining backward
compatibility by using separate structures and mailbox handlers.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h

[ ... ]

>     Due to new requirements in CN20K, the existing `struct mcam_entry` needed
>     to be updated. Previously, it contained two arrays, `kw` and `kw_mask`,
>     each of size 7 (keyword size). To support CN20K requirements, the size of
>     these arrays has been increased from 7 to 8.

This isn't a bug, but the commit message description appears inaccurate. The
code shows that struct mcam_entry retains NPC_MAX_KWS_IN_KEY = 7, while a new
struct cn20k_mcam_entry was created with NPC_CN20K_MAX_KWS_IN_KEY = 8. Could
the commit message be updated to reflect that a new structure was created
rather than the existing one being resized?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c

[ ... ]

> @@ -706,6 +707,21 @@ static int rvu_setup_msix_resources(struct rvu *rvu)
>  	rvu->msix_base_iova = iova;
>  	rvu->msixtr_base_phy = phy_addr;
>
> +	if (is_rvu_otx2(rvu) || is_cn20k(rvu->pdev))
> +		return 0;
> +
> +	altaf_intr_data = &rvu->fwdata->altaf_intr_info;

Is rvu->fwdata guaranteed to be non-NULL here? Looking at rvu_fwdata_init(),
it can fail in several cases (cgx_get_fwdata_base() failure, ioremap_wc()
failure, or is_rvu_fwdata_valid() explicitly setting fwdata to NULL), and
rvu_setup_hw_resources() does not check its return value before calling
rvu_setup_msix_resources().

The platform check (is_rvu_otx2 || is_cn20k) does not guarantee fwdata is
valid since fwdata initialization depends on firmware availability, not
silicon version.

> +	if (altaf_intr_data->gint_paddr) {
> +		iova = dma_map_resource(rvu->dev, altaf_intr_data->gint_paddr,
> +					PCI_MSIX_ENTRY_SIZE,
> +					DMA_BIDIRECTIONAL, 0);
> +
> +		if (dma_mapping_error(rvu->dev, iova))
> +			return -ENOMEM;
> +
> +		altaf_intr_data->gint_iova_addr = iova;

Does altaf_intr_data->gint_iova_addr get unmapped anywhere? Looking at
rvu_free_hw_resources(), only rvu->msix_base_iova is unmapped. If this second
DMA mapping fails, does the first mapping (stored at rvu->msix_base_iova)
also leak?

[ ... ]

> +static void rvu_notify_altaf(struct rvu *rvu, u16 pcifunc, u64 op)
> +{
> +	int pf, vf;
> +
> +	if (!rvu->fwdata)
> +		return;
> +
> +	if (op == ALTAF_FLR) {
> +		pf = rvu_get_pf(rvu->pdev, pcifunc);
> +		set_bit(pf, rvu->fwdata->altaf_intr_info.flr_pf_bmap);
> +		if (pcifunc & RVU_PFVF_FUNC_MASK) {
> +			vf = pcifunc & RVU_PFVF_FUNC_MASK;
> +			set_bit(vf, rvu->fwdata->altaf_intr_info.flr_vf_bmap);

Can this overflow flr_vf_bmap[]? RVU_PFVF_FUNC_MASK is 0x3FF allowing vf
values 0-1023, but flr_vf_bmap is declared as unsigned long flr_vf_bmap[2]
which provides only 128 bits on 64-bit systems. If vf >= 128, set_bit() will
write beyond the array bounds.

[ ... ]

> @@ -3269,6 +3316,13 @@ static int rvu_register_interrupts(struct rvu *rvu)
>  	if (ret)
>  		goto fail;
>
> +	for (i = 0; i < rvu->num_vec; i++) {
> +		if (strstr(&rvu->irq_name[i * NAME_SIZE], "Mbox") ||
> +		    strstr(&rvu->irq_name[i * NAME_SIZE], "FLR"))

Is it safe to call strstr() on all entries in irq_name[]? The array is
allocated with devm_kmalloc_array() which does not zero-initialize memory.
IRQ names are only set via sprintf() for specific vector indices, so unnamed
vectors will contain uninitialized data without a null terminator. Could this
cause strstr() to read past the buffer?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c

[ ... ]

> @@ -3111,7 +3115,7 @@ int rvu_mbox_handler_npc_mcam_write_entry(struct rvu *rvu,
>  	if (rc)
>  		goto exit;
>
> -	if (req->set_cntr &&
> +	if (!is_cn20k(rvu->pdev) && req->set_cntr &&
>  	    npc_mcam_verify_counter(mcam, pcifunc, req->cntr)) {
>  		rc = NPC_MCAM_INVALID_REQ;
>  		goto exit;

Should this explicitly reject req->set_cntr for CN20K? Other handlers
(rvu_mbox_handler_npc_mcam_alloc_counter, free_counter, unmap_counter)
return NPC_MCAM_INVALID_REQ for CN20K, but this function skips validation
and proceeds. Would it be more consistent to return an error if
is_cn20k(rvu->pdev) && req->set_cntr?

[ ... ]

> @@ -3482,12 +3498,20 @@ int rvu_mbox_handler_npc_mcam_clear_counter(struct rvu *rvu,
>  		struct npc_mcam_oper_counter_req *req, struct msg_rsp *rsp)
>  {
>  	struct npc_mcam *mcam = &rvu->hw->mcam;
> -	int blkaddr, err;
> +	int blkaddr, err, index, bank;
>
>  	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
>  	if (blkaddr < 0)
>  		return NPC_MCAM_INVALID_REQ;
>
> +	if (is_cn20k(rvu->pdev)) {
> +		index = req->cntr & (mcam->banksize - 1);
> +		bank = npc_get_bank(mcam, req->cntr);
> +		rvu_write64(rvu, blkaddr,
> +			    NPC_AF_CN20K_MCAMEX_BANKX_STAT_EXT(index, bank), 0);
> +		return 0;
> +	}

On CN20K, req->cntr is treated as an entry index since statistics are
per-entry. Should npc_mcam_verify_entry() be called to verify ownership
before clearing? Without validation, any VF could clear statistics for
entries owned by other VFs or the PF.

[ ... ]

> @@ -3504,12 +3528,23 @@ int rvu_mbox_handler_npc_mcam_counter_stats(struct rvu *rvu,
>  			struct npc_mcam_oper_counter_rsp *rsp)
>  {
>  	struct npc_mcam *mcam = &rvu->hw->mcam;
> -	int blkaddr, err;
> +	int blkaddr, err, index, bank;
> +	u64 regval;
>
>  	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
>  	if (blkaddr < 0)
>  		return NPC_MCAM_INVALID_REQ;
>
> +	if (is_cn20k(rvu->pdev)) {
> +		index = req->cntr & (mcam->banksize - 1);
> +		bank = npc_get_bank(mcam, req->cntr);
> +		regval = rvu_read64(rvu, blkaddr,
> +				    NPC_AF_CN20K_MCAMEX_BANKX_STAT_EXT(index,
> +								       bank));
> +		rsp->stat = regval;
> +		return 0;
> +	}

Similar to the clear function above, the CN20K path reads per-entry
statistics without ownership validation. Should npc_mcam_verify_entry()
be called before returning statistics? This could allow a VF to read
traffic statistics for entries owned by other VFs.