Re: [net-next,v6,10/13] octeontx2-af: npc: cn20k: Allocate MCAM entry for flow installation

[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: Allocate MCAM entry for flow installation

This patch extends the install_flow mailbox request message with
additional fields that allow the AF driver to determine the required
NPC MCAM entry type, allocate the entry, and install the flow in a
single step for CN20K.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> index 82afb4993346..7a48ba1db5e9 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c

[ ... ]

> @@ -1642,6 +1643,75 @@ static int npc_install_flow(struct rvu *rvu, int blkaddr, u16 target,
>  	return 0;
>  }
>
> +static int
> +rvu_npc_free_entry_for_flow_install(struct rvu *rvu, u16 pcifunc,
> +				    bool free_entry, int mcam_idx)
> +{

[ ... ]

> +static int
> +rvu_npc_alloc_entry_for_flow_install(struct rvu *rvu,
> +				     struct npc_install_flow_req *fl_req,
> +				     u16 *mcam_idx, u8 *kw_type,
> +				     bool *allocated)
> +{

[ ... ]

> +	if (!is_cn20k(rvu->pdev))
> +		return 0;
> +
> +	if (!fl_req->alloc_entry)
> +		return 0;

[ ... ]

>  int rvu_mbox_handler_npc_install_flow(struct rvu *rvu,
>  				      struct npc_install_flow_req *req,
>  				      struct npc_install_flow_rsp *rsp)
>  {

[ ... ]

> +	bool allocated = false;
>  	bool enable = true;
> +	u8 kw_type;
>  	u16 target;

[ ... ]

> +	err = rvu_npc_alloc_entry_for_flow_install(rvu, req, &req->entry,
> +						   &kw_type, &allocated);
> +	if (err) {
> +		dev_err(rvu->dev,
> +			"%s: Error to alloc mcam entry for pcifunc=%#x\n",
> +			__func__, req->hdr.pcifunc);
> +		return err;
> +	}

[ ... ]

>  	mutex_lock(&rswitch->switch_lock);
>  	err = npc_install_flow(rvu, blkaddr, target, nixlf, pfvf,
>  			       req, rsp, enable, pf_set_vfs_mac);
> +	if (err)
> +		rvu_npc_free_entry_for_flow_install(rvu, req->hdr.pcifunc,
> +						    allocated, req->entry);
> +
> +	rsp->kw_type = kw_type;
> +	rsp->entry = req->entry;
            ^^^^^^^^

Is kw_type initialized in all paths here?

When rvu_npc_alloc_entry_for_flow_install() returns early because either
is_cn20k() is false or alloc_entry is not set, kw_type is never written.
The function returns 0 without setting *kw_type, leaving the local variable
uninitialized.

The uninitialized stack value then gets copied to rsp->kw_type.

Should this be initialized to 0 at declaration?

>  	mutex_unlock(&rswitch->switch_lock);
>
>  	return err;
>  }