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 <[email protected]>
Signed-off-by: wangyangxin <[email protected]>
Signed-off-by: Gonglei <[email protected]>
---
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);
}
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
Gonglei Arei <[email protected]> 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 <[email protected]>
> Signed-off-by: wangyangxin <[email protected]>
> Signed-off-by: Gonglei <[email protected]>
> ---
> drivers/crypto/virtio/virtio_crypto_common.h | 2 ++
> drivers/crypto/virtio/virtio_crypto_core.c | 23 +++++++++++++----------
> 2 files changed, 15 insertions(+), 10 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
> Signed-off-by: wangyangxin <[email protected]>
> Signed-off-by: Gonglei <[email protected]>
> ---
> 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?
> 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
>
>
> -----Original Message-----
> From: Michael S. Tsirkin [mailto:[email protected]]
> Sent: Friday, December 1, 2023 10:51 PM
> To: Gonglei (Arei) <[email protected]>
> Cc: Herbert Xu <[email protected]>; [email protected];
> Halil Pasic <[email protected]>; Jason Wang <[email protected]>;
> [email protected]; [email protected];
> wangyangxin <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: wangyangxin <[email protected]>
> > Signed-off-by: Gonglei <[email protected]>
> > ---
> > 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
> >
> >
>
Hi Herbert,
> -----Original Message-----
> From: Herbert Xu [mailto:[email protected]]
> Sent: Friday, December 1, 2023 6:11 PM
> To: Gonglei (Arei) <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; wangyangxin <[email protected]>;
> Gonglei (Arei) <[email protected]>
> Subject: Re: [PATCH] crypto: virtio-crypto: Handle dataq logic with tasklet
>
> Gonglei Arei <[email protected]> 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 <[email protected]>
> > Signed-off-by: wangyangxin <[email protected]>
> > Signed-off-by: Gonglei <[email protected]>
> > ---
> > drivers/crypto/virtio/virtio_crypto_common.h | 2 ++
> > drivers/crypto/virtio/virtio_crypto_core.c | 23 +++++++++++++----------
> > 2 files changed, 15 insertions(+), 10 deletions(-)
>
> Patch applied. Thanks.
Sorry, pls apply version 2 if possible.
Regards,
-Gonglei
> --
> Email: Herbert Xu <[email protected]> Home Page:
> http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt