Re: [net-next PATCH v1 10/15] octeontx2-pf: ipsec: Setup NIX HW resources for inbound flows

Simon Horman
2025-05-07T13:46:28Z

On Fri, May 02, 2025 at 06:49:51PM +0530, Tanmay Jagdale wrote:
> A incoming encrypted IPsec packet in the RVU NIX hardware needs
> to be classified for inline fastpath processing and then assinged

nit: assigned

checkpatch.pl --codespell is your friend

> a RQ and Aura pool before sending to CPT for decryption.
>
> Create a dedicated RQ, Aura and Pool with the following setup
> specifically for IPsec flows:
> - Set ipsech_en, ipsecd_drop_en in RQ context to enable hardware
> fastpath processing for IPsec flows.
> - Configure the dedicated Aura to raise an interrupt when
> it's buffer count drops below a threshold value so that the
> buffers can be replenished from the CPU.
>
> The RQ, Aura and Pool contexts are initialized only when esp-hw-offload
> feature is enabled via ethtool.
>
> Also, move some of the RQ context macro definitions to otx2_common.h
> so that they can be used in the IPsec driver as well.
>
> Signed-off-by: Tanmay Jagdale <tanmay@marvell.com>

...

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

...

> +static int cn10k_ipsec_setup_nix_rx_hw_resources(struct otx2_nic *pfvf)
> +{
> + struct otx2_hw *hw = &pfvf->hw;
> + int stack_pages, pool_id;
> + struct otx2_pool *pool;
> + int err, ptr, num_ptrs;
> + dma_addr_t bufptr;
> +
> + num_ptrs = 256;
> + pool_id = pfvf->ipsec.inb_ipsec_pool;
> + stack_pages = (num_ptrs + hw->stack_pg_ptrs - 1) / hw->stack_pg_ptrs;
> +
> + mutex_lock(&pfvf->mbox.lock);
> +
> + /* Initialize aura context */
> + err = cn10k_ipsec_ingress_aura_init(pfvf, pool_id, pool_id, num_ptrs);
> + if (err)
> + goto fail;
> +
> + /* Initialize pool */
> + err = otx2_pool_init(pfvf, pool_id, stack_pages, num_ptrs, pfvf->rbsize, AURA_NIX_RQ);
> + if (err)

This appears to leak pool->fc_addr.

> + goto fail;
> +
> + /* Flush accumulated messages */
> + err = otx2_sync_mbox_msg(&pfvf->mbox);
> + if (err)
> + goto pool_fail;
> +
> + /* Allocate pointers and free them to aura/pool */
> + pool = &pfvf->qset.pool[pool_id];
> + for (ptr = 0; ptr < num_ptrs; ptr++) {
> + err = otx2_alloc_rbuf(pfvf, pool, &bufptr, pool_id, ptr);
> + if (err) {
> + err = -ENOMEM;
> + goto pool_fail;
> + }
> + pfvf->hw_ops->aura_freeptr(pfvf, pool_id, bufptr + OTX2_HEAD_ROOM);
> + }
> +
> + /* Initialize RQ and map buffers from pool_id */
> + err = cn10k_ipsec_ingress_rq_init(pfvf, pfvf->ipsec.inb_ipsec_rq, pool_id);
> + if (err)
> + goto pool_fail;
> +
> + mutex_unlock(&pfvf->mbox.lock);
> + return 0;
> +
> +pool_fail:
> + mutex_unlock(&pfvf->mbox.lock);
> + qmem_free(pfvf->dev, pool->stack);
> + qmem_free(pfvf->dev, pool->fc_addr);
> + page_pool_destroy(pool->page_pool);
> + devm_kfree(pfvf->dev, pool->xdp);

It is not clear to me why devm_kfree() is being called here.
I didn't look deeply. But I think it is likely that
either pool->xdp should be freed when the device is released.
Or pool->xdp should not be allocated (and freed) using devm functions.

> + pool->xsk_pool = NULL;

The clean-up of pool->stack, pool->page_pool), pool->xdp, and
pool->xsk_pool, all seem to unwind initialisation performed by
otx2_pool_init(). And appear to be duplicated elsewhere.
I would suggest adding a helper for that.

> +fail:
> + otx2_mbox_reset(&pfvf->mbox.mbox, 0);
> + return err;
> +}

...