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

[Simon Horman <horms@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-pf: cn20k: Use unified Halo context

Use unified Halo context present in CN20K hardware for
octeontx2 netdevs instead of aura and pool contexts.

> 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

[ ... ]

> +int cn20k_npa_alloc_dpc(struct otx2_nic *nic)
> +{
> +	struct npa_cn20k_dpc_alloc_req *req;
> +	struct npa_cn20k_dpc_alloc_rsp *rsp;
> +	int err;
> +
> +	req = otx2_mbox_alloc_msg_npa_cn20k_dpc_alloc(&nic->mbox);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	/* Count successful ALLOC requests only */
> +	req->dpc_conf = 1ULL << 4;
> +
> +	err = otx2_sync_mbox_msg(&nic->mbox);
> +	if (err)
> +		return err;
> +
> +	rsp = (struct npa_cn20k_dpc_alloc_rsp *)otx2_mbox_get_rsp(&nic->mbox.mbox,
> +								  0, &req->hdr);
> +	if (IS_ERR(rsp))
> +		return PTR_ERR(rsp);
> +
> +	nic->npa_dpc = rsp->cntr_id;
> +
> +	return 0;
> +}

[ ... ]

> 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);
                ^^^^^^^^^^^^^^^^^^^^^

Should the return value of cn20k_npa_alloc_dpc() be checked here?

Looking at cn20k_npa_alloc_dpc(), it can return -ENOMEM or errors from
otx2_sync_mbox_msg() or PTR_ERR() from otx2_mbox_get_rsp(). If the DPC
counter allocation fails, pf->npa_dpc remains 0 (from struct
zero-initialization).

Then cn20k_halo_aq_init() configures all NPA auras with
aq->halo.op_dpc_set = pfvf->npa_dpc (= 0), referencing an unallocated
DPC performance counter. On teardown, cn20k_npa_free_dpc() sends a mbox
message to free counter ID 0, which was never allocated.

Every other error-returning mbox operation in this function checks the
return value and jumps to an error label on failure. Should this follow
the same pattern?

> +
>  	/* Init Auras and pools used by NIX RQ, for free buffer ptrs */
>  	err = otx2_rq_aura_pool_init(pf);
>  	if (err) {

[ ... ]