> -----Original Message-----
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Friday, December 1, 2023 10:51 PM
> To: Gonglei (Arei) <arei.gong...@huawei.com>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>; linux-crypto@vger.kernel.org;
> Halil Pasic <pa...@linux.ibm.com>; Jason Wang <jasow...@redhat.com>;
> virtualizat...@lists.linux-foundation.org; linux-ker...@vger.kernel.org;
> wangyangxin <wangyangx...@huawei.com>
> Subject: Re: [PATCH] crypto: virtio-crypto: Handle dataq logic with tasklet
> 
> On Mon, Nov 20, 2023 at 11:49:45AM +0000, Gonglei (Arei) wrote:
> > Doing ipsec produces a spinlock recursion warning.
> > This is due to crypto_finalize_request() being called in the upper half.
> > Move virtual data queue processing of virtio-crypto driver to tasklet.
> >
> > Fixes: dbaf0624ffa57 ("crypto: add virtio-crypto driver")
> > Reported-by: Halil Pasic <pa...@linux.ibm.com>
> > Signed-off-by: wangyangxin <wangyangx...@huawei.com>
> > Signed-off-by: Gonglei <arei.gong...@huawei.com>
> > ---
> >  drivers/crypto/virtio/virtio_crypto_common.h |  2 ++
> >  drivers/crypto/virtio/virtio_crypto_core.c   | 23 +++++++++++++----------
> >  2 files changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/crypto/virtio/virtio_crypto_common.h
> > b/drivers/crypto/virtio/virtio_crypto_common.h
> > index 59a4c02..5c17c6e 100644
> > --- a/drivers/crypto/virtio/virtio_crypto_common.h
> > +++ b/drivers/crypto/virtio/virtio_crypto_common.h
> > @@ -10,6 +10,7 @@
> >  #include <linux/virtio.h>
> >  #include <linux/crypto.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/interrupt.h>
> >  #include <crypto/aead.h>
> >  #include <crypto/aes.h>
> >  #include <crypto/engine.h>
> > @@ -28,6 +29,7 @@ struct data_queue {
> >     char name[32];
> >
> >     struct crypto_engine *engine;
> > +   struct tasklet_struct done_task;
> >  };
> >
> >  struct virtio_crypto {
> > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> > b/drivers/crypto/virtio/virtio_crypto_core.c
> > index 1198bd3..e747f4f 100644
> > --- a/drivers/crypto/virtio/virtio_crypto_core.c
> > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> > @@ -72,27 +72,28 @@ int virtio_crypto_ctrl_vq_request(struct virtio_crypto
> *vcrypto, struct scatterl
> >     return 0;
> >  }
> >
> > -static void virtcrypto_dataq_callback(struct virtqueue *vq)
> > +static void virtcrypto_done_task(unsigned long data)
> >  {
> > -   struct virtio_crypto *vcrypto = vq->vdev->priv;
> > +   struct data_queue *data_vq = (struct data_queue *)data;
> > +   struct virtqueue *vq = data_vq->vq;
> >     struct virtio_crypto_request *vc_req;
> > -   unsigned long flags;
> >     unsigned int len;
> > -   unsigned int qid = vq->index;
> >
> > -   spin_lock_irqsave(&vcrypto->data_vq[qid].lock, flags);
> >     do {
> >             virtqueue_disable_cb(vq);
> >             while ((vc_req = virtqueue_get_buf(vq, &len)) != NULL) {
> > -                   spin_unlock_irqrestore(
> > -                           &vcrypto->data_vq[qid].lock, flags);
> >                     if (vc_req->alg_cb)
> >                             vc_req->alg_cb(vc_req, len);
> > -                   spin_lock_irqsave(
> > -                           &vcrypto->data_vq[qid].lock, flags);
> >             }
> >     } while (!virtqueue_enable_cb(vq));
> > -   spin_unlock_irqrestore(&vcrypto->data_vq[qid].lock, flags);
> > +}
> > +
> > +static void virtcrypto_dataq_callback(struct virtqueue *vq) {
> > +   struct virtio_crypto *vcrypto = vq->vdev->priv;
> > +   struct data_queue *dq = &vcrypto->data_vq[vq->index];
> > +
> > +   tasklet_schedule(&dq->done_task);
> >  }
> >
> 
> Don't we then need to wait for tasklet to complete on device remove?
> 
> 
Yes, we should do it. V2 will be sent soon.

Regards,
-Gonglei

> >  static int virtcrypto_find_vqs(struct virtio_crypto *vi) @@ -150,6
> > +151,8 @@ static int virtcrypto_find_vqs(struct virtio_crypto *vi)
> >                     ret = -ENOMEM;
> >                     goto err_engine;
> >             }
> > +           tasklet_init(&vi->data_vq[i].done_task, virtcrypto_done_task,
> > +                           (unsigned long)&vi->data_vq[i]);
> >     }
> >
> >     kfree(names);
> > --
> > 1.8.3.1
> >
> >
> 


Reply via email to