Re: [PATCH net-next v2 3/3] nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer

[Yinjun Zhang <yinjun.zhang@corigine.com>]

On Thu, Sep 29, 2022 at 11:26:24AM +0300, Leon Romanovsky wrote:
> On Tue, Sep 27, 2022 at 12:27:07PM +0200, Simon Horman wrote:
> 
> > +	mutex_lock(&ipd->lock);
> > +
> > +	if (ipd->sa_free_cnt == 0) {
> > +		nn_err(nn, "No space for xfrm offload\n");
> > +		err = -ENOSPC;
> 
> Why don't you return EOPNOTSUPP?
> 

Here means no available sa. I think ENOSPC is more appropriate than
EOPNOTSUPP, and it looks like xfrm will fall back to software mode
when driver returns EOPNOTSUPP.

> > +static void xfrm_invalidate(struct nfp_net *nn, unsigned int saidx, int is_del)
> > +{
> > +	struct nfp_net_ipsec_data *ipd = nn->ipsec_data;
> > +	struct nfp_net_ipsec_sa_data *sa_data;
> > +	struct nfp_ipsec_cfg_mssg msg;
> > +	int err;
> > +
> > +	sa_data = &ipd->sa_entries[saidx];
> > +	if (!sa_data->invalidated) {
> > +		err = nfp_ipsec_cfg_cmd_issue(nn, NFP_IPSEC_CFG_MSSG_INV_SA, saidx, &msg);
> > +		if (err)
> > +			nn_warn(nn, "Failed to invalidate SA in hardware\n");
> > +		sa_data->invalidated = 1;
> > +	} else if (is_del) {
> > +		nn_warn(nn, "Unexpected invalidate state for offloaded saidx %d\n", saidx);
> 
> You definitely need to clean all these not-possible flows.
> 

Do you mean clean those sa entries? We clean them by invalidating them.
You can see `xfrm_invalidate` is called in `nfp_net_xfrm_del_state`.

> > +	}
> >  }
> >  
> >  static void nfp_net_xfrm_del_state(struct xfrm_state *x)
> >  {
> > +	struct net_device *netdev = x->xso.dev;
> > +	struct nfp_net_ipsec_data *ipd;
> > +	struct nfp_net *nn;
> > +
> > +	nn = netdev_priv(netdev);
> > +	ipd = nn->ipsec_data;
> > +
> > +	nn_dbg(nn, "XFRM del state!\n");
> > +
> > +	if (x->xso.offload_handle == OFFLOAD_HANDLE_ERROR) {
> > +		nn_err(nn, "Invalid xfrm offload handle\n");
> > +		return;
> > +	}
> > +
> > +	mutex_lock(&ipd->lock);
> > +	xfrm_invalidate(nn, x->xso.offload_handle - 1, 1);
> > +	mutex_unlock(&ipd->lock);
> >  }
> >  
> >  static void nfp_net_xfrm_free_state(struct xfrm_state *x)
> >  {
> > +	struct net_device *netdev = x->xso.dev;
> > +	struct nfp_net_ipsec_data *ipd;
> > +	struct nfp_net *nn;
> > +	int saidx;
> > +
> > +	nn = netdev_priv(netdev);
> > +	ipd = nn->ipsec_data;
> > +
> > +	nn_dbg(nn, "XFRM free state!\n");
> > +
> > +	if (x->xso.offload_handle == OFFLOAD_HANDLE_ERROR) {
> > +		nn_err(nn, "Invalid xfrm offload handle\n");
> > +		return;
> > +	}
> > +
> > +	mutex_lock(&ipd->lock);
> > +	saidx = x->xso.offload_handle - 1;
> > +	xfrm_invalidate(nn, saidx, 0);
> > +	ipd->sa_entries[saidx].x = NULL;
> > +	/* Return saidx to free list */
> > +	ipd->sa_free_stack[ipd->sa_free_cnt] = saidx;
> > +	ipd->sa_free_cnt++;
> > +
> > +	mutex_unlock(&ipd->lock);
> >  }