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

[Tanmay Jagdale <tanmay@marvell.com>]

Hi Simon,

On 2025-05-07 at 19:16:20, Simon Horman (horms@kernel.org) wrote:
> 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
> 
ACK.

> > 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.
Okay, let me look into this.

> 
> > +		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.
Good catch. We aren't used pool->xdp for inbound IPsec yet, so I'll
drop this.

> 
> > +	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.
Okay I'll look into reusing common code.

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