Re: [PATCH net-next 4/5] cteontx2-af: npc: cn20k: dynamically allocate and free default MCAM entries

[Simon Horman <horms@kernel.org>]

On Mon, Mar 02, 2026 at 02:28:02PM +0530, Ratheesh Kannoth wrote:

...

> @@ -4220,21 +4242,46 @@ void npc_cn20k_dft_rules_free(struct rvu *rvu, u16 pcifunc)
>  		index = NPC_DFT_RULE_ID_MK(pcifunc, i);
>  		map = xa_erase(&npc_priv.xa_pf2dfl_rmap, index);
>  		if (!map)
> -			dev_dbg(rvu->dev,
> +			dev_err(rvu->dev,
>  				"%s: Err from delete %s mcam idx from xarray (pcifunc=%#x\n",
>  				__func__, npc_dft_rule_name[i],
>  				pcifunc);
>  	}
>  
>  free_rules:
> +	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
> +	if (blkaddr < 0)
> +		return;
>  
> -	free_req.hdr.pcifunc = pcifunc;
> -	free_req.all = 1;
> -	rc = rvu_mbox_handler_npc_mcam_free_entry(rvu, &free_req, &rsp);
> -	if (rc)
> -		dev_err(rvu->dev,
> -			"%s: Error deleting default entries (pcifunc=%#x\n",
> -			__func__, pcifunc);
> +	for (int i = 0; i < 4; i++) {
> +		if (ptr[i] == USHRT_MAX)
> +			continue;
> +
> +		mutex_lock(&mcam->lock);
> +		npc_mcam_clear_bit(mcam, ptr[i]);
> +		mcam->entry2pfvf_map[ptr[i]] = NPC_MCAM_INVALID_MAP;
> +		npc_cn20k_enable_mcam_entry(rvu, blkaddr, ptr[i], false);
> +		mcam->entry2target_pffunc[ptr[i]] = 0x0;
> +		mutex_unlock(&mcam->lock);
> +
> +		rc = npc_cn20k_idx_free(rvu, &ptr[i], 1);
> +		if (rc)
> +			dev_err(rvu->dev,
> +				"%s:%d Error deleting default entries (pcifunc=%#x) mcam_idx=%u\n",
> +				__func__, __LINE__, pcifunc, ptr[i]);
> +	}
> +
> +	mutex_lock(&mcam->lock);
> +	list_for_each_entry_safe(rule, tmp, &mcam->mcam_rules, list) {
> +		for (int i = 0; i < 4; i++) {
> +			if (ptr[i] != rule->entry)
> +				continue;

Hi Ratheesh,

I assume that the condition above should match exactly once
for each iteration of the for loop. But if not rule may
either not be freed or freed twice.

Would it make sense to move freeing rule to outside the for loop?
I think this would address both problems. But perhaps it leads
to rule being freed when it should not be.

Alternatively, should there be a break after the call to kfree()?
I think that would address only the (theoretical possibility of) a double
free.

Flagged by Smatch.

> +
> +			list_del(&rule->list);
> +			kfree(rule);
> +		}
> +	}
> +	mutex_unlock(&mcam->lock);
>  }

...