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

Hi Simon,

quoted

@@ 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)

quoted

 			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.

[Suman] Yes, you are right. Thanks for catching this, will update in v2.

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).

[Suman] Will update the logic in v2.

...

quoted

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

b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c

...

quoted

@@ -3204,6 +3199,10 @@ static int otx2_probe(struct pci_dev *pdev,

const struct pci_device_id *id)

quoted

 	/* 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?

[Suman] Yes, will update in v2

quoted

 #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

...

quoted

@@ -571,6 +574,7 @@ int otx2_napi_handler(struct napi_struct *napi,

int budget)

quoted

 		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.

quoted

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

...

quoted

@@ -1426,13 +1440,24 @@ static bool otx2_xdp_rcv_pkt_handler(struct

otx2_nic *pfvf,

quoted

 	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.	

[Suman] I think the patch ordering is wrong. 
"patch#4: octeontx2-pf: Don't unmap page pool buffer used by XDP" is fixing this. I will update the patch order.

quoted

+	}
+
 	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:

[Suman] replied above.

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.

[Suman] same as above.

...

quoted

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)

quoted

 	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?

[Suman] yes, will update in v2

...

--
pw-bot: changes-requested