Re: [net-next PATCH v8 5/8] cn10k-ipsec: Add SA add/del support for outb ipsec crypto offload

[Bharat Bhushan]

On Tue, Sep 3, 2024 at 3:08 PM Sabrina Dubroca <sd@xxxxxxxxxxxxxxx> wrote:
>
> 2024-09-03, 10:29:34 +0530, Bharat Bhushan wrote:
> > +static int cn10k_ipsec_validate_state(struct xfrm_state *x)
> > +{
> > +     struct net_device *netdev = x->xso.dev;
> > +
> > +     if (x->props.aalgo != SADB_AALG_NONE) {
> > +             netdev_err(netdev, "Cannot offload authenticated xfrm states\n");
>
> This should use extack, to return this information directly to the
> application that's creating the invalid config. You can propagate it
> from cn10k_ipsec_add_state down to this function, and then:
>
>     NL_SET_ERR_MSG_MOD(extack, "Cannot offload authenticated xfrm states");

ack, this and other places
>
>
> > +static int cn10k_ipsec_inb_add_state(struct xfrm_state *x)
> > +{
> > +     struct net_device *netdev = x->xso.dev;
> > +
> > +     netdev_err(netdev, "xfrm inbound offload not supported\n");
>
> Here too, extack.
>
> > +     return -EOPNOTSUPP;
> > +}
> > +
> > +static int cn10k_ipsec_outb_add_state(struct xfrm_state *x)
> > +{
> > +     struct net_device *netdev = x->xso.dev;
> > +     struct cn10k_tx_sa_s *sa_entry;
> > +     struct cpt_ctx_info_s *sa_info;
> > +     struct otx2_nic *pf;
> > +     int err;
> > +
> > +     err = cn10k_ipsec_validate_state(x);
> > +     if (err)
> > +             return err;
> > +
> > +     pf = netdev_priv(netdev);
> > +     if (!mutex_trylock(&pf->ipsec.lock)) {
>
> Why not wait until we can take the lock? Failing to offload the state
> because this lock is temporarily busy isn't nice to users.
>
> > +             netdev_err(netdev, "IPSEC device is busy\n");
> > +             return -EBUSY;
> > +     }
> > +
> > +     if (!(pf->flags & OTX2_FLAG_IPSEC_OFFLOAD_ENABLED)) {
> > +             netdev_err(netdev, "IPSEC not enabled/supported on device\n");
>
> You should also use extack in this function.
>
>
> [...]
> > +static void cn10k_ipsec_del_state(struct xfrm_state *x)
> > +{
> > +     struct net_device *netdev = x->xso.dev;
> > +     struct cn10k_tx_sa_s *sa_entry;
> > +     struct cpt_ctx_info_s *sa_info;
> > +     struct otx2_nic *pf;
> > +     int sa_index;
> > +
> > +     if (x->xso.dir == XFRM_DEV_OFFLOAD_IN)
> > +             return;
> > +
> > +     pf = netdev_priv(netdev);
> > +     if (!mutex_trylock(&pf->ipsec.lock)) {
> > +             netdev_err(netdev, "IPSEC device is busy\n");
> > +             return;
>
> If we can't take the lock, we leave the state installed on the device
> and leak some memory? That's not good. I assume we're going to reach
> HW limits if this happens a bunch of times, and then we can't offload
> ipsec at all anymore?
>
> I think it would be better to wait until we can take the lock.

This is atomic context (in_atomic() is true). So we need to call the
trylock variant.

Thanks
-Bharat

>
> --
> Sabrina
>