Re: [net-next PATCH 2/6] octeontx2-pf: AF_XDP zero copy receive support

[Simon Horman <horms@kernel.org>]

On Tue, Jan 07, 2025 at 04:16:24PM +0530, Suman Ghosh wrote:
> This patch adds support to AF_XDP zero copy support for CN10K.
> This patch specifically adds receive side support. In this approach once
> a xdp program with zero copy support on a specific rx queue is enabled,
> then that receive queue is disabled/detached from the existing kernel
> queue and re-assigned to the umem memory.
> 
> Signed-off-by: Suman Ghosh <sumang@marvell.com>

Hi Suman,

I'd like to bring to your attention a number of minor issues
flagged by Smatch.

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c

...

> @@ -1298,6 +1311,7 @@ void otx2_free_aura_ptr(struct otx2_nic *pfvf, int type)
>  	int pool_id, pool_start = 0, pool_end = 0, size = 0;
>  	struct otx2_pool *pool;
>  	u64 iova;
> +	int idx;
>  
>  	if (type == AURA_NIX_SQ) {
>  		pool_start = otx2_get_pool_idx(pfvf, type, 0);
> @@ -1312,8 +1326,8 @@ void otx2_free_aura_ptr(struct otx2_nic *pfvf, int type)
>  
>  	/* Free SQB and RQB pointers from the aura pool */
>  	for (pool_id = pool_start; pool_id < pool_end; pool_id++) {
> -		iova = otx2_aura_allocptr(pfvf, pool_id);
>  		pool = &pfvf->qset.pool[pool_id];
> +		iova = otx2_aura_allocptr(pfvf, pool_id);
>  		while (iova) {
>  			if (type == AURA_NIX_RQ)
>  				iova -= OTX2_HEAD_ROOM;
> @@ -1323,6 +1337,13 @@ void otx2_free_aura_ptr(struct otx2_nic *pfvf, int type)
>  			iova = otx2_aura_allocptr(pfvf, pool_id);
>  		}
>  	}
> +
> +	for (idx = 0 ; idx < pool->xdp_cnt; idx++) {
> +		if (!pool->xdp[idx])
> +			continue;
> +
> +		xsk_buff_free(pool->xdp[idx]);
> +	}

Looking over otx2_pool_init(), I am wondering if the loop above run should
over all (non AURA_NIX_RQ) pools, rather than the last pool covered by the
preceding loop.

This was flagged by Smatch, because it is concerned that pool may be used
unset above, presumably if the preceding loop iterates zero times (I'm
unsure if that can happen in practice).

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c

...

> @@ -3204,6 +3199,10 @@ static int otx2_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	/* Enable link notifications */
>  	otx2_cgx_config_linkevents(pf, true);
>  
> +	pf->af_xdp_zc_qidx = bitmap_zalloc(qcount, GFP_KERNEL);
> +	if (!pf->af_xdp_zc_qidx)
> +		goto err_pf_sriov_init;
> +

This goto will result in the function returning err.
However, here err is 0. Should it be set to a negative error value instead?

>  #ifdef CONFIG_DCB
>  	err = otx2_dcbnl_set_ops(netdev);
>  	if (err)
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c

...

> @@ -571,6 +574,7 @@ int otx2_napi_handler(struct napi_struct *napi, int budget)
>  		if (pfvf->flags & OTX2_FLAG_ADPTV_INT_COAL_ENABLED)
>  			otx2_adjust_adaptive_coalese(pfvf, cq_poll);
>  
> +		pool = &pfvf->qset.pool[cq->cq_idx];

I am also unsure if this can happen in practice, but Smatch is concerned
that cq may be used uninitialised here. It does seem that could
theoretically be the case if, in the for loop towards the top of this
function, cq_poll->cq_ids[i] is always CINT_INVALID_CQ.

>  		if (unlikely(!filled_cnt)) {
>  			struct refill_work *work;
>  			struct delayed_work *dwork;

...

> @@ -1426,13 +1440,24 @@ static bool otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
>  	unsigned char *hard_start;
>  	struct otx2_pool *pool;
>  	int qidx = cq->cq_idx;
> -	struct xdp_buff xdp;
> +	struct xdp_buff xdp, *xsk_buff = NULL;
>  	struct page *page;
>  	u64 iova, pa;
>  	u32 act;
>  	int err;
>  
>  	pool = &pfvf->qset.pool[qidx];
> +
> +	if (pool->xsk_pool) {
> +		xsk_buff = pool->xdp[--cq->rbpool->xdp_top];
> +		if (!xsk_buff)
> +			return false;
> +
> +		xsk_buff->data_end = xsk_buff->data + cqe->sg.seg_size;
> +		act = bpf_prog_run_xdp(prog, xsk_buff);
> +		goto handle_xdp_verdict;

iova is not initialised until a few lines further down,
which does not occur in the case of this condition.

> +	}
> +
>  	iova = cqe->sg.seg_addr - OTX2_HEAD_ROOM;
>  	pa = otx2_iova_to_phys(pfvf->iommu_domain, iova);
>  	page = virt_to_page(phys_to_virt(pa));
> @@ -1445,6 +1470,7 @@ static bool otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
>  
>  	act = bpf_prog_run_xdp(prog, &xdp);
>  
> +handle_xdp_verdict:
>  	switch (act) {
>  	case XDP_PASS:
>  		break;

The lines lines of this function, between the hunk above and the one below
looks like this:

	case XDP_TX:
		qidx += pfvf->hw.tx_queues;
		cq->pool_ptrs++;
		return otx2_xdp_sq_append_pkt(pfvf, iova,

The above uses iova, but in the case that we got here
via goto handle_xdp_verdict it is uninitialised.

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> index e926c6ce96cf..9bb7e5c3e227 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> @@ -722,6 +722,10 @@ static int otx2vf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (err)
>  		goto err_shutdown_tc;
>  
> +	vf->af_xdp_zc_qidx = bitmap_zalloc(qcount, GFP_KERNEL);
> +	if (!vf->af_xdp_zc_qidx)
> +		goto err_shutdown_tc;

Along the same lines of my comment on otx2_probe():
should err be set to a negative error value here?

...

-- 
pw-bot: changes-requested