[PATCH 0/9] crypto: engine: permit to handle multiple requests

[PATCH 0/9] crypto: engine: permit to handle multiple requests

[Corentin Labbe]

Hello

The sun8i-ce hardware can work on multiple requests in one batch.
For this it use a task descriptor, and chain them.
For the moment, the driver does not use this mechanism and do requests
one at a time and issue an irq for each.

Using the chaining will permit to issue less interrupts, and increase
thoughput.

But the crypto/engine can enqueue lots of requests but can ran them only
one by one.

This serie introduce a way to batch requests in crypto/engine by adding
a new function can_queue_more() that a driver can implement to tell
crypto_engine if it can handle more request.

For testing the serie, the selftest are not enough, since it issue
request one at a time.
I have used LUKS for testing it.
Tested on sun8i-ce (with/without batching).
And tested for non-regression on caam, amlogic and sun8i-ss drivers.

The 4 first patchs are cleanup necessary for permit crypto_engine to
handle more requests.
The 5th patch introduce the new wrappers for handle multiple requests.
Lasts patchs are for enabling batching in sun8i-ce.

Regards

Corentin Labbe (9):
  crypto: engine: workqueue can only be processed one by one
  crypto: engine: get rid of cur_req_prepared
  crypto: engine: get rid of cur_req
  crypto: engine: permit to choose queue length
  crypto: engine: add enqueue_request/can_do_more
  crypto: sun8i-ce: move iv data to request context
  crypto: sun8i-ce: increase task list size
  crypto: sun8i-ce: split into prepare/run/unprepare
  crypto: sun8i-ce: permit to batch requests

 crypto/crypto_engine.c                        |  99 +++++++-----
 .../allwinner/sun8i-ce/sun8i-ce-cipher.c      | 153 ++++++++++++++----
 .../crypto/allwinner/sun8i-ce/sun8i-ce-core.c |  19 ++-
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h  |  20 ++-
 include/crypto/engine.h                       |  19 +--
 5 files changed, 213 insertions(+), 97 deletions(-)

-- 
2.24.1

[PATCH 1/9] crypto: engine: workqueue can only be processed one by one

[Corentin Labbe]

Some bykeshedding are unnecessary since a workqueue can only be executed
one by one.
This behaviour is documented in:
- kernel/kthread.c: comment of kthread_worker_fn()
- Documentation/core-api/workqueue.rst: the functions associated with the work items one after the other

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 crypto/crypto_engine.c  | 13 -------------
 include/crypto/engine.h |  2 --
 2 files changed, 15 deletions(-)

diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
index eb029ff1e05a..feb0d82dd454 100644
--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -73,16 +73,6 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 
 	spin_lock_irqsave(&engine->queue_lock, flags);
 
-	/* Make sure we are not already running a request */
-	if (engine->cur_req)
-		goto out;
-
-	/* If another context is idling then defer */
-	if (engine->idling) {
-		kthread_queue_work(engine->kworker, &engine->pump_requests);
-		goto out;
-	}
-
 	/* Check if the engine queue is idle */
 	if (!crypto_queue_len(&engine->queue) || !engine->running) {
 		if (!engine->busy)
@@ -96,7 +86,6 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 		}
 
 		engine->busy = false;
-		engine->idling = true;
 		spin_unlock_irqrestore(&engine->queue_lock, flags);
 
 		if (engine->unprepare_crypt_hardware &&
@@ -104,7 +93,6 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 			dev_err(engine->dev, "failed to unprepare crypt hardware\n");
 
 		spin_lock_irqsave(&engine->queue_lock, flags);
-		engine->idling = false;
 		goto out;
 	}
 
@@ -410,7 +398,6 @@ struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt)
 	engine->rt = rt;
 	engine->running = false;
 	engine->busy = false;
-	engine->idling = false;
 	engine->cur_req_prepared = false;
 	engine->priv_data = dev;
 	snprintf(engine->name, sizeof(engine->name),
diff --git a/include/crypto/engine.h b/include/crypto/engine.h
index e29cd67f93c7..7e7cbd9ca3b5 100644
--- a/include/crypto/engine.h
+++ b/include/crypto/engine.h
@@ -21,7 +21,6 @@
 /*
  * struct crypto_engine - crypto hardware engine
  * @name: the engine name
- * @idling: the engine is entering idle state
  * @busy: request pump is busy
  * @running: the engine is on working
  * @cur_req_prepared: current request is prepared
@@ -42,7 +41,6 @@
  */
 struct crypto_engine {
 	char			name[ENGINE_NAME_LEN];
-	bool			idling;
 	bool			busy;
 	bool			running;
 	bool			cur_req_prepared;
-- 
2.24.1

[PATCH 2/9] crypto: engine: get rid of cur_req_prepared

[Corentin Labbe]

Since we want cryptoengine to support handling more than one request, we
need to remove cur_req_prepared.
It is used only to choose if we need to unprepare() in
crypto_finalize_request().
This choice is needed only in case of error, so let's handle error
without crypto_finalize_request.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 crypto/crypto_engine.c  | 17 ++++++++++-------
 include/crypto/engine.h |  2 --
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
index feb0d82dd454..dfcb00e92e09 100644
--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -36,7 +36,7 @@ static void crypto_finalize_request(struct crypto_engine *engine,
 
 	if (finalize_cur_req) {
 		enginectx = crypto_tfm_ctx(req->tfm);
-		if (engine->cur_req_prepared &&
+		if (enginectx->op.prepare_request &&
 		    enginectx->op.unprepare_request) {
 			ret = enginectx->op.unprepare_request(engine, req);
 			if (ret)
@@ -44,7 +44,6 @@ static void crypto_finalize_request(struct crypto_engine *engine,
 		}
 		spin_lock_irqsave(&engine->queue_lock, flags);
 		engine->cur_req = NULL;
-		engine->cur_req_prepared = false;
 		spin_unlock_irqrestore(&engine->queue_lock, flags);
 	}
 
@@ -118,7 +117,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 		ret = engine->prepare_crypt_hardware(engine);
 		if (ret) {
 			dev_err(engine->dev, "failed to prepare crypt hardware\n");
-			goto req_err;
+			goto req_err2;
 		}
 	}
 
@@ -129,9 +128,8 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 		if (ret) {
 			dev_err(engine->dev, "failed to prepare request: %d\n",
 				ret);
-			goto req_err;
+			goto req_err2;
 		}
-		engine->cur_req_prepared = true;
 	}
 	if (!enginectx->op.do_one_request) {
 		dev_err(engine->dev, "failed to do request\n");
@@ -146,7 +144,13 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 	return;
 
 req_err:
-	crypto_finalize_request(engine, async_req, ret);
+	if (enginectx->op.unprepare_request) {
+		ret = enginectx->op.unprepare_request(engine, async_req);
+		if (ret)
+			dev_err(engine->dev, "failed to unprepare request\n");
+	}
+req_err2:
+	async_req->complete(async_req, ret);
 	return;
 
 out:
@@ -398,7 +402,6 @@ struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt)
 	engine->rt = rt;
 	engine->running = false;
 	engine->busy = false;
-	engine->cur_req_prepared = false;
 	engine->priv_data = dev;
 	snprintf(engine->name, sizeof(engine->name),
 		 "%s-engine", dev_name(dev));
diff --git a/include/crypto/engine.h b/include/crypto/engine.h
index 7e7cbd9ca3b5..4d8a2602c9ed 100644
--- a/include/crypto/engine.h
+++ b/include/crypto/engine.h
@@ -23,7 +23,6 @@
  * @name: the engine name
  * @busy: request pump is busy
  * @running: the engine is on working
- * @cur_req_prepared: current request is prepared
  * @list: link with the global crypto engine list
  * @queue_lock: spinlock to syncronise access to request queue
  * @queue: the crypto queue of the engine
@@ -43,7 +42,6 @@ struct crypto_engine {
 	char			name[ENGINE_NAME_LEN];
 	bool			busy;
 	bool			running;
-	bool			cur_req_prepared;
 
 	struct list_head	list;
 	spinlock_t		queue_lock;
-- 
2.24.1

[PATCH 3/9] crypto: engine: get rid of cur_req

[Corentin Labbe]

cur_req was used as a sign of usage of the crypto_engine, now this
behaviour has gone, its usage remains for detecting if we finalize the
cur_req.
But testing if we finalize the cur_req prevent to do more request at a
time and is unnecessary.
It is unnecessary since crypto_finalize_request() is only used for
cryptoengine and so the request finalized will be always the current
request.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 crypto/crypto_engine.c  | 25 ++++++-------------------
 include/crypto/engine.h |  2 --
 2 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
index dfcb00e92e09..c21867106aa4 100644
--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -24,27 +24,15 @@
 static void crypto_finalize_request(struct crypto_engine *engine,
 			     struct crypto_async_request *req, int err)
 {
-	unsigned long flags;
-	bool finalize_cur_req = false;
 	int ret;
 	struct crypto_engine_ctx *enginectx;
 
-	spin_lock_irqsave(&engine->queue_lock, flags);
-	if (engine->cur_req == req)
-		finalize_cur_req = true;
-	spin_unlock_irqrestore(&engine->queue_lock, flags);
-
-	if (finalize_cur_req) {
-		enginectx = crypto_tfm_ctx(req->tfm);
-		if (enginectx->op.prepare_request &&
-		    enginectx->op.unprepare_request) {
-			ret = enginectx->op.unprepare_request(engine, req);
-			if (ret)
-				dev_err(engine->dev, "failed to unprepare request\n");
-		}
-		spin_lock_irqsave(&engine->queue_lock, flags);
-		engine->cur_req = NULL;
-		spin_unlock_irqrestore(&engine->queue_lock, flags);
+	enginectx = crypto_tfm_ctx(req->tfm);
+	if (enginectx->op.prepare_request &&
+			enginectx->op.unprepare_request) {
+		ret = enginectx->op.unprepare_request(engine, req);
+		if (ret)
+			dev_err(engine->dev, "failed to unprepare request\n");
 	}
 
 	req->complete(req, err);
@@ -101,7 +89,6 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 	if (!async_req)
 		goto out;
 
-	engine->cur_req = async_req;
 	if (backlog)
 		backlog->complete(backlog, -EINPROGRESS);
 
diff --git a/include/crypto/engine.h b/include/crypto/engine.h
index 4d8a2602c9ed..d77080227beb 100644
--- a/include/crypto/engine.h
+++ b/include/crypto/engine.h
@@ -36,7 +36,6 @@
  * @kworker: kthread worker struct for request pump
  * @pump_requests: work struct for scheduling work to the request pump
  * @priv_data: the engine private data
- * @cur_req: the current request which is on processing
  */
 struct crypto_engine {
 	char			name[ENGINE_NAME_LEN];
@@ -57,7 +56,6 @@ struct crypto_engine {
 	struct kthread_work             pump_requests;
 
 	void				*priv_data;
-	struct crypto_async_request	*cur_req;
 };
 
 /*
-- 
2.24.1

[PATCH 4/9] crypto: engine: permit to choose queue length

[Corentin Labbe]

This patch adds a new function which permit to choose the crypto engine
queue length.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 crypto/crypto_engine.c  | 23 ++++++++++++++++++++---
 include/crypto/engine.h |  2 ++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
index c21867106aa4..5bcb1e740fd9 100644
--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -365,15 +365,17 @@ int crypto_engine_stop(struct crypto_engine *engine)
 EXPORT_SYMBOL_GPL(crypto_engine_stop);
 
 /**
- * crypto_engine_alloc_init - allocate crypto hardware engine structure and
+ * crypto_engine_alloc_init2 - allocate crypto hardware engine structure and
  * initialize it.
  * @dev: the device attached with one hardware engine
  * @rt: whether this queue is set to run as a realtime task
+ * @qlen: The size of the crypto queue
  *
  * This must be called from context that can sleep.
  * Return: the crypto engine structure on success, else NULL.
  */
-struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt)
+struct crypto_engine *crypto_engine_alloc_init2(struct device *dev, bool rt,
+						int qlen)
 {
 	struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
 	struct crypto_engine *engine;
@@ -393,7 +395,7 @@ struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt)
 	snprintf(engine->name, sizeof(engine->name),
 		 "%s-engine", dev_name(dev));
 
-	crypto_init_queue(&engine->queue, CRYPTO_ENGINE_MAX_QLEN);
+	crypto_init_queue(&engine->queue, qlen);
 	spin_lock_init(&engine->queue_lock);
 
 	engine->kworker = kthread_create_worker(0, "%s", engine->name);
@@ -410,6 +412,21 @@ struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt)
 
 	return engine;
 }
+EXPORT_SYMBOL_GPL(crypto_engine_alloc_init2);
+
+/**
+ * crypto_engine_alloc_init - allocate crypto hardware engine structure and
+ * initialize it.
+ * @dev: the device attached with one hardware engine
+ * @rt: whether this queue is set to run as a realtime task
+ *
+ * This must be called from context that can sleep.
+ * Return: the crypto engine structure on success, else NULL.
+ */
+struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt)
+{
+	return crypto_engine_alloc_init2(dev, rt, CRYPTO_ENGINE_MAX_QLEN);
+}
 EXPORT_SYMBOL_GPL(crypto_engine_alloc_init);
 
 /**
diff --git a/include/crypto/engine.h b/include/crypto/engine.h
index d77080227beb..03d9f9ec1cea 100644
--- a/include/crypto/engine.h
+++ b/include/crypto/engine.h
@@ -96,6 +96,8 @@ void crypto_finalize_skcipher_request(struct crypto_engine *engine,
 int crypto_engine_start(struct crypto_engine *engine);
 int crypto_engine_stop(struct crypto_engine *engine);
 struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt);
+struct crypto_engine *crypto_engine_alloc_init2(struct device *dev, bool rt,
+						int qlen);
 int crypto_engine_exit(struct crypto_engine *engine);
 
 #endif /* _CRYPTO_ENGINE_H */
-- 
2.24.1

[PATCH 5/9] crypto: engine: add enqueue_request/can_do_more

[Corentin Labbe]

This patchs adds two new function wrapper in crypto_engine.
- enqueue_request() for drivers enqueuing request to hardware.
- can_queue_more() for letting drivers to tell if they can
enqueue/prepare more.

Since some drivers (like caam) only enqueue request without "doing"
them, do_one_request() is now optional.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 crypto/crypto_engine.c  | 25 ++++++++++++++++++++++---
 include/crypto/engine.h | 14 ++++++++------
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
index 5bcb1e740fd9..4a28548c49aa 100644
--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -83,6 +83,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 		goto out;
 	}
 
+retry:
 	/* Get the fist request from the engine queue to handle */
 	backlog = crypto_get_backlog(&engine->queue);
 	async_req = crypto_dequeue_request(&engine->queue);
@@ -118,10 +119,28 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 			goto req_err2;
 		}
 	}
+
+	if (enginectx->op.enqueue_request) {
+		ret = enginectx->op.enqueue_request(engine, async_req);
+		if (ret) {
+			dev_err(engine->dev, "failed to enqueue request: %d\n",
+				ret);
+			goto req_err;
+		}
+	}
+	if (enginectx->op.can_queue_more && engine->queue.qlen > 0) {
+		ret = enginectx->op.can_queue_more(engine, async_req);
+		if (ret > 0) {
+			spin_lock_irqsave(&engine->queue_lock, flags);
+			goto retry;
+		}
+		if (ret < 0) {
+			dev_err(engine->dev, "failed to call can_queue_more\n");
+			/* TODO */
+		}
+	}
 	if (!enginectx->op.do_one_request) {
-		dev_err(engine->dev, "failed to do request\n");
-		ret = -EINVAL;
-		goto req_err;
+		return;
 	}
 	ret = enginectx->op.do_one_request(engine, async_req);
 	if (ret) {
diff --git a/include/crypto/engine.h b/include/crypto/engine.h
index 03d9f9ec1cea..8ab9d26e30fe 100644
--- a/include/crypto/engine.h
+++ b/include/crypto/engine.h
@@ -63,14 +63,16 @@ struct crypto_engine {
  * @prepare__request: do some prepare if need before handle the current request
  * @unprepare_request: undo any work done by prepare_request()
  * @do_one_request: do encryption for current request
+ * @enqueue_request:	Enqueue the request in the hardware
+ * @can_queue_more:	if this function return > 0, it will tell the crypto
+ * 	engine that more space are availlable for prepare/enqueue request
  */
 struct crypto_engine_op {
-	int (*prepare_request)(struct crypto_engine *engine,
-			       void *areq);
-	int (*unprepare_request)(struct crypto_engine *engine,
-				 void *areq);
-	int (*do_one_request)(struct crypto_engine *engine,
-			      void *areq);
+	int (*prepare_request)(struct crypto_engine *engine, void *areq);
+	int (*unprepare_request)(struct crypto_engine *engine, void *areq);
+	int (*do_one_request)(struct crypto_engine *engine, void *areq);
+	int (*enqueue_request)(struct crypto_engine *engine, void *areq);
+	int (*can_queue_more)(struct crypto_engine *engine, void *areq);
 };
 
 struct crypto_engine_ctx {
-- 
2.24.1

[PATCH 6/9] crypto: sun8i-ce: move iv data to request context

[Corentin Labbe]

Instead of storing IV data in the channel context, store them in the
request context.
Storing them in the channel structure was conceptualy wrong since they
are per request related.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 .../allwinner/sun8i-ce/sun8i-ce-cipher.c      | 27 +++++++++----------
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h  | 10 ++++---
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index a5fd8975f3d3..7fd19667bceb 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -91,7 +91,6 @@ static int sun8i_ce_cipher(struct skcipher_request *areq)
 	struct scatterlist *sg;
 	unsigned int todo, len, offset, ivsize;
 	dma_addr_t addr_iv = 0, addr_key = 0;
-	void *backup_iv = NULL;
 	u32 common, sym;
 	int flow, i;
 	int nr_sgs = 0;
@@ -154,24 +153,24 @@ static int sun8i_ce_cipher(struct skcipher_request *areq)
 
 	ivsize = crypto_skcipher_ivsize(tfm);
 	if (areq->iv && crypto_skcipher_ivsize(tfm) > 0) {
-		chan->ivlen = ivsize;
-		chan->bounce_iv = kzalloc(ivsize, GFP_KERNEL | GFP_DMA);
-		if (!chan->bounce_iv) {
+		rctx->ivlen = ivsize;
+		rctx->bounce_iv = kzalloc(ivsize, GFP_KERNEL | GFP_DMA);
+		if (!rctx->bounce_iv) {
 			err = -ENOMEM;
 			goto theend_key;
 		}
 		if (rctx->op_dir & CE_DECRYPTION) {
-			backup_iv = kzalloc(ivsize, GFP_KERNEL);
-			if (!backup_iv) {
+			rctx->backup_iv = kzalloc(ivsize, GFP_KERNEL);
+			if (!rctx->backup_iv) {
 				err = -ENOMEM;
 				goto theend_key;
 			}
 			offset = areq->cryptlen - ivsize;
-			scatterwalk_map_and_copy(backup_iv, areq->src, offset,
-						 ivsize, 0);
+			scatterwalk_map_and_copy(rctx->backup_iv, areq->src,
+						 offset, ivsize, 0);
 		}
-		memcpy(chan->bounce_iv, areq->iv, ivsize);
-		addr_iv = dma_map_single(ce->dev, chan->bounce_iv, chan->ivlen,
+		memcpy(rctx->bounce_iv, areq->iv, ivsize);
+		addr_iv = dma_map_single(ce->dev, rctx->bounce_iv, rctx->ivlen,
 					 DMA_TO_DEVICE);
 		cet->t_iv = cpu_to_le32(addr_iv);
 		if (dma_mapping_error(ce->dev, addr_iv)) {
@@ -252,17 +251,17 @@ static int sun8i_ce_cipher(struct skcipher_request *areq)
 theend_iv:
 	if (areq->iv && ivsize > 0) {
 		if (addr_iv)
-			dma_unmap_single(ce->dev, addr_iv, chan->ivlen,
+			dma_unmap_single(ce->dev, addr_iv, rctx->ivlen,
 					 DMA_TO_DEVICE);
 		offset = areq->cryptlen - ivsize;
 		if (rctx->op_dir & CE_DECRYPTION) {
-			memcpy(areq->iv, backup_iv, ivsize);
-			kzfree(backup_iv);
+			memcpy(areq->iv, rctx->backup_iv, ivsize);
+			kzfree(rctx->backup_iv);
 		} else {
 			scatterwalk_map_and_copy(areq->iv, areq->dst, offset,
 						 ivsize, 0);
 		}
-		kfree(chan->bounce_iv);
+		kfree(rctx->bounce_iv);
 	}
 
 theend_key:
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
index 8f8404c84a4d..49507ef2ec63 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
@@ -129,8 +129,6 @@ struct ce_task {
 /*
  * struct sun8i_ce_flow - Information used by each flow
  * @engine:	ptr to the crypto_engine for this flow
- * @bounce_iv:	buffer which contain the IV
- * @ivlen:	size of bounce_iv
  * @complete:	completion for the current task on this flow
  * @status:	set to 1 by interrupt if task is done
  * @t_phy:	Physical address of task
@@ -139,8 +137,6 @@ struct ce_task {
  */
 struct sun8i_ce_flow {
 	struct crypto_engine *engine;
-	void *bounce_iv;
-	unsigned int ivlen;
 	struct completion complete;
 	int status;
 	dma_addr_t t_phy;
@@ -183,10 +179,16 @@ struct sun8i_ce_dev {
  * struct sun8i_cipher_req_ctx - context for a skcipher request
  * @op_dir:	direction (encrypt vs decrypt) for this request
  * @flow:	the flow to use for this request
+ * @backup_iv:	buffer which contain the next IV to store
+ * @bounce_iv:	buffer which contain a copy of IV
+ * @ivlen:	size of bounce_iv
  */
 struct sun8i_cipher_req_ctx {
 	u32 op_dir;
 	int flow;
+	void *backup_iv;
+	void *bounce_iv;
+	unsigned int ivlen;
 };
 
 /*
-- 
2.24.1

[PATCH 7/9] crypto: sun8i-ce: increase task list size

[Corentin Labbe]

The CE can handle more than 1 task at once, so this patch increase the size of
the task list up to 8.
For the moment I did not see more gain beyong this value.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c | 4 ++--
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h      | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
index f72346a44e69..e8bf7bf31061 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
@@ -321,7 +321,7 @@ static void sun8i_ce_free_chanlist(struct sun8i_ce_dev *ce, int i)
 	while (i >= 0) {
 		crypto_engine_exit(ce->chanlist[i].engine);
 		if (ce->chanlist[i].tl)
-			dma_free_coherent(ce->dev, sizeof(struct ce_task),
+			dma_free_coherent(ce->dev, sizeof(struct ce_task) * MAXTASK,
 					  ce->chanlist[i].tl,
 					  ce->chanlist[i].t_phy);
 		i--;
@@ -356,7 +356,7 @@ static int sun8i_ce_allocate_chanlist(struct sun8i_ce_dev *ce)
 			goto error_engine;
 		}
 		ce->chanlist[i].tl = dma_alloc_coherent(ce->dev,
-							sizeof(struct ce_task),
+							sizeof(struct ce_task) * MAXTASK,
 							&ce->chanlist[i].t_phy,
 							GFP_KERNEL);
 		if (!ce->chanlist[i].tl) {
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
index 49507ef2ec63..049b3175d755 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
@@ -73,6 +73,7 @@
 #define CE_MAX_CLOCKS 3
 
 #define MAXFLOW 4
+#define MAXTASK 8
 
 /*
  * struct ce_clock - Describe clocks used by sun8i-ce
-- 
2.24.1

[PATCH 8/9] crypto: sun8i-ce: split into prepare/run/unprepare

[Corentin Labbe]

This patch split the do_one_request into three.
Prepare will handle all DMA mapping and initialisation of the task
structure.
Unprepare will clean all DMA mapping.
And the do_one_request will be limited to just excuting the task.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 .../allwinner/sun8i-ce/sun8i-ce-cipher.c      | 70 ++++++++++++++++---
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h  |  4 ++
 2 files changed, 66 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index 7fd19667bceb..fc0a2299c701 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -78,8 +78,9 @@ static int sun8i_ce_cipher_fallback(struct skcipher_request *areq)
 	return err;
 }
 
-static int sun8i_ce_cipher(struct skcipher_request *areq)
+static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req)
 {
+	struct skcipher_request *areq = container_of(async_req, struct skcipher_request, base);
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq);
 	struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm);
 	struct sun8i_ce_dev *ce = op->ce;
@@ -237,7 +238,9 @@ static int sun8i_ce_cipher(struct skcipher_request *areq)
 	}
 
 	chan->timeout = areq->cryptlen;
-	err = sun8i_ce_run_task(ce, flow, crypto_tfm_alg_name(areq->base.tfm));
+	rctx->nr_sgs = nr_sgs;
+	rctx->nr_sgd = nr_sgd;
+	return 0;
 
 theend_sgs:
 	if (areq->src == areq->dst) {
@@ -271,13 +274,64 @@ static int sun8i_ce_cipher(struct skcipher_request *areq)
 	return err;
 }
 
-static int sun8i_ce_handle_cipher_request(struct crypto_engine *engine, void *areq)
+int sun8i_ce_cipher_run(struct crypto_engine *engine, void *areq)
 {
-	int err;
 	struct skcipher_request *breq = container_of(areq, struct skcipher_request, base);
+	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(breq);
+	struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm);
+	struct sun8i_ce_dev *ce = op->ce;
+	struct sun8i_cipher_req_ctx *rctx = skcipher_request_ctx(breq);
+	int flow, err;
 
-	err = sun8i_ce_cipher(breq);
+	flow = rctx->flow;
+	err = sun8i_ce_run_task(ce, flow, crypto_tfm_alg_name(breq->base.tfm));
 	crypto_finalize_skcipher_request(engine, breq, err);
+	return 0;
+}
+
+static int sun8i_ce_cipher_unprepare(struct crypto_engine *engine, void *async_req)
+{
+	struct skcipher_request *areq = container_of(async_req, struct skcipher_request, base);
+	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq);
+	struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm);
+	struct sun8i_ce_dev *ce = op->ce;
+	struct sun8i_cipher_req_ctx *rctx = skcipher_request_ctx(areq);
+	struct sun8i_ce_flow *chan;
+	struct ce_task *cet;
+	unsigned int ivsize, offset;
+	int nr_sgs = rctx->nr_sgs;
+	int nr_sgd = rctx->nr_sgd;
+	int flow;
+
+	flow = rctx->flow;
+	chan = &ce->chanlist[flow];
+	cet = chan->tl;
+	ivsize = crypto_skcipher_ivsize(tfm);
+
+	if (areq->src == areq->dst) {
+		dma_unmap_sg(ce->dev, areq->src, nr_sgs, DMA_BIDIRECTIONAL);
+	} else {
+		if (nr_sgs > 0)
+			dma_unmap_sg(ce->dev, areq->src, nr_sgs, DMA_TO_DEVICE);
+		dma_unmap_sg(ce->dev, areq->dst, nr_sgd, DMA_FROM_DEVICE);
+	}
+
+	if (areq->iv && ivsize > 0) {
+		if (cet->t_iv)
+			dma_unmap_single(ce->dev, cet->t_iv, rctx->ivlen,
+					 DMA_TO_DEVICE);
+		offset = areq->cryptlen - ivsize;
+		if (rctx->op_dir & CE_DECRYPTION) {
+			memcpy(areq->iv, rctx->backup_iv, ivsize);
+			kzfree(rctx->backup_iv);
+		} else {
+			scatterwalk_map_and_copy(areq->iv, areq->dst, offset,
+						 ivsize, 0);
+		}
+		kfree(rctx->bounce_iv);
+	}
+
+	dma_unmap_single(ce->dev, cet->t_key, op->keylen, DMA_TO_DEVICE);
 
 	return 0;
 }
@@ -347,9 +401,9 @@ int sun8i_ce_cipher_init(struct crypto_tfm *tfm)
 		 crypto_tfm_alg_driver_name(&sktfm->base),
 		 crypto_tfm_alg_driver_name(crypto_skcipher_tfm(&op->fallback_tfm->base)));
 
-	op->enginectx.op.do_one_request = sun8i_ce_handle_cipher_request;
-	op->enginectx.op.prepare_request = NULL;
-	op->enginectx.op.unprepare_request = NULL;
+	op->enginectx.op.do_one_request = sun8i_ce_cipher_run;
+	op->enginectx.op.prepare_request = sun8i_ce_cipher_prepare;
+	op->enginectx.op.unprepare_request = sun8i_ce_cipher_unprepare;
 
 	err = pm_runtime_get_sync(op->ce->dev);
 	if (err < 0)
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
index 049b3175d755..2d3325a13bf1 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
@@ -183,6 +183,8 @@ struct sun8i_ce_dev {
  * @backup_iv:	buffer which contain the next IV to store
  * @bounce_iv:	buffer which contain a copy of IV
  * @ivlen:	size of bounce_iv
+ * @nr_sgs:	The number of source SG (as given by dma_map_sg())
+ * @nr_sgd:	The number of destination SG (as given by dma_map_sg())
  */
 struct sun8i_cipher_req_ctx {
 	u32 op_dir;
@@ -190,6 +192,8 @@ struct sun8i_cipher_req_ctx {
 	void *backup_iv;
 	void *bounce_iv;
 	unsigned int ivlen;
+	int nr_sgs;
+	int nr_sgd;
 };
 
 /*
-- 
2.24.1

[PATCH 9/9] crypto: sun8i-ce: permit to batch requests

[Corentin Labbe]

This patch permit to batch request.
This imply:
- linking two task via next
- set interrupt flag just before running the batch in the last task.
- storing all requests for finalizing them later

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 .../allwinner/sun8i-ce/sun8i-ce-cipher.c      | 60 +++++++++++++++----
 .../crypto/allwinner/sun8i-ce/sun8i-ce-core.c | 15 +++--
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h  |  6 ++
 3 files changed, 66 insertions(+), 15 deletions(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index fc0a2299c701..832fb4a51da9 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -96,31 +96,38 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
 	int flow, i;
 	int nr_sgs = 0;
 	int nr_sgd = 0;
+	int slot = 0;
 	int err = 0;
 
 	algt = container_of(alg, struct sun8i_ce_alg_template, alg.skcipher);
 
-	dev_dbg(ce->dev, "%s %s %u %x IV(%p %u) key=%u\n", __func__,
+	dev_dbg(ce->dev, "%s %s %u %x IV(%p %u) key=%u slot=%d\n", __func__,
 		crypto_tfm_alg_name(areq->base.tfm),
 		areq->cryptlen,
 		rctx->op_dir, areq->iv, crypto_skcipher_ivsize(tfm),
-		op->keylen);
-
-#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
-	algt->stat_req++;
-#endif
+		op->keylen, slot);
 
 	flow = rctx->flow;
 
 	chan = &ce->chanlist[flow];
+	slot = chan->ct;
+
+#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
+	algt->stat_req++;
+	if (chan->ct + 1 > chan->tmax)
+		chan->tmax = chan->ct + 1;
+#endif
 
-	cet = chan->tl;
+	cet = &chan->tl[slot];
 	memset(cet, 0, sizeof(struct ce_task));
 
 	cet->t_id = cpu_to_le32(flow);
 	common = ce->variant->alg_cipher[algt->ce_algo_id];
-	common |= rctx->op_dir | CE_COMM_INT;
+	common |= rctx->op_dir;
 	cet->t_common_ctl = cpu_to_le32(common);
+	if (slot > 0)
+		chan->tl[slot - 1].next = cpu_to_le32(chan->t_phy + 176 * slot);
+
 	/* CTS and recent CE (H6) need length in bytes, in word otherwise */
 	if (ce->variant->has_t_dlen_in_bytes)
 		cet->t_dlen = cpu_to_le32(areq->cryptlen);
@@ -240,6 +247,9 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
 	chan->timeout = areq->cryptlen;
 	rctx->nr_sgs = nr_sgs;
 	rctx->nr_sgd = nr_sgd;
+	rctx->slot = slot;
+	chan->lreq[chan->ct] = &areq->base;
+	chan->ct++;
 	return 0;
 
 theend_sgs:
@@ -281,14 +291,41 @@ int sun8i_ce_cipher_run(struct crypto_engine *engine, void *areq)
 	struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm);
 	struct sun8i_ce_dev *ce = op->ce;
 	struct sun8i_cipher_req_ctx *rctx = skcipher_request_ctx(breq);
+	struct sun8i_ce_flow *chan;
 	int flow, err;
+	int i;
 
 	flow = rctx->flow;
+	chan = &ce->chanlist[flow];
 	err = sun8i_ce_run_task(ce, flow, crypto_tfm_alg_name(breq->base.tfm));
-	crypto_finalize_skcipher_request(engine, breq, err);
+	for (i = 0; i < chan->ct; i++) {
+		if (!chan->lreq[i]) {
+			dev_err(ce->dev, "Missing request at slot %d\n", i);
+			continue;
+		}
+		breq = container_of(chan->lreq[i], struct skcipher_request, base);
+		crypto_finalize_skcipher_request(engine, breq, err);
+		chan->lreq[i] = NULL;
+	}
+	chan->ct = 0;
 	return 0;
 }
 
+static int sun8i_ce_qmore(struct crypto_engine *engine, void *async_req)
+{
+	struct skcipher_request *areq = container_of(async_req, struct skcipher_request, base);
+	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq);
+	struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm);
+	struct sun8i_ce_dev *ce = op->ce;
+	struct sun8i_cipher_req_ctx *rctx = skcipher_request_ctx(areq);
+	struct sun8i_ce_flow *chan;
+	int flow;
+
+	flow = rctx->flow;
+	chan = &ce->chanlist[flow];
+	return MAXTASK - chan->ct;
+}
+
 static int sun8i_ce_cipher_unprepare(struct crypto_engine *engine, void *async_req)
 {
 	struct skcipher_request *areq = container_of(async_req, struct skcipher_request, base);
@@ -301,11 +338,13 @@ static int sun8i_ce_cipher_unprepare(struct crypto_engine *engine, void *async_r
 	unsigned int ivsize, offset;
 	int nr_sgs = rctx->nr_sgs;
 	int nr_sgd = rctx->nr_sgd;
+	int slot = rctx->slot;
 	int flow;
 
 	flow = rctx->flow;
 	chan = &ce->chanlist[flow];
-	cet = chan->tl;
+
+	cet = &chan->tl[slot];
 	ivsize = crypto_skcipher_ivsize(tfm);
 
 	if (areq->src == areq->dst) {
@@ -404,6 +443,7 @@ int sun8i_ce_cipher_init(struct crypto_tfm *tfm)
 	op->enginectx.op.do_one_request = sun8i_ce_cipher_run;
 	op->enginectx.op.prepare_request = sun8i_ce_cipher_prepare;
 	op->enginectx.op.unprepare_request = sun8i_ce_cipher_unprepare;
+	op->enginectx.op.can_queue_more = sun8i_ce_qmore;
 
 	err = pm_runtime_get_sync(op->ce->dev);
 	if (err < 0)
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
index e8bf7bf31061..348d3927344b 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
@@ -104,8 +104,10 @@ int sun8i_ce_run_task(struct sun8i_ce_dev *ce, int flow, const char *name)
 	int err = 0;
 
 #ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
-	ce->chanlist[flow].stat_req++;
+	ce->chanlist[flow].stat_req += ce->chanlist[flow].ct;
 #endif
+	/* mark last one */
+	ce->chanlist[flow].tl[ce->chanlist[flow].ct - 1].t_common_ctl |= cpu_to_le32(CE_COMM_INT);
 
 	mutex_lock(&ce->mlock);
 
@@ -120,7 +122,7 @@ int sun8i_ce_run_task(struct sun8i_ce_dev *ce, int flow, const char *name)
 	/* Be sure all data is written before enabling the task */
 	wmb();
 
-	v = 1 | (ce->chanlist[flow].tl->t_common_ctl & 0x7F) << 8;
+	v = 1 | (ce->chanlist[flow].tl[0].t_common_ctl & 0x7F) << 8;
 	writel(v, ce->base + CE_TLR);
 	mutex_unlock(&ce->mlock);
 
@@ -128,7 +130,7 @@ int sun8i_ce_run_task(struct sun8i_ce_dev *ce, int flow, const char *name)
 			msecs_to_jiffies(ce->chanlist[flow].timeout));
 
 	if (ce->chanlist[flow].status == 0) {
-		dev_err(ce->dev, "DMA timeout for %s\n", name);
+		dev_err(ce->dev, "DMA timeout for %s on flow %d (batch=%d)\n", name, flow, ce->chanlist[flow].ct);
 		err = -EFAULT;
 	}
 	/* No need to lock for this read, the channel is locked so
@@ -285,7 +287,10 @@ static int sun8i_ce_dbgfs_read(struct seq_file *seq, void *v)
 	int i;
 
 	for (i = 0; i < MAXFLOW; i++)
-		seq_printf(seq, "Channel %d: nreq %lu\n", i, ce->chanlist[i].stat_req);
+		seq_printf(seq, "Channel %d: nreq %lu tmax %d eqlen=%d/%d\n", i,
+			   ce->chanlist[i].stat_req, ce->chanlist[i].tmax,
+			   ce->chanlist[i].engine->queue.qlen,
+			   ce->chanlist[i].engine->queue.max_qlen);
 
 	for (i = 0; i < ARRAY_SIZE(ce_algs); i++) {
 		if (!ce_algs[i].ce)
@@ -343,7 +348,7 @@ static int sun8i_ce_allocate_chanlist(struct sun8i_ce_dev *ce)
 	for (i = 0; i < MAXFLOW; i++) {
 		init_completion(&ce->chanlist[i].complete);
 
-		ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true);
+		ce->chanlist[i].engine = crypto_engine_alloc_init2(ce->dev, true, MAXTASK * 2);
 		if (!ce->chanlist[i].engine) {
 			dev_err(ce->dev, "Cannot allocate engine\n");
 			i--;
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
index 2d3325a13bf1..59e9985fc6c8 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
@@ -135,6 +135,7 @@ struct ce_task {
  * @t_phy:	Physical address of task
  * @tl:		pointer to the current ce_task for this flow
  * @stat_req:	number of request done by this flow
+ * @tmax:	The maximum number of tasks done in one batch
  */
 struct sun8i_ce_flow {
 	struct crypto_engine *engine;
@@ -143,8 +144,11 @@ struct sun8i_ce_flow {
 	dma_addr_t t_phy;
 	int timeout;
 	struct ce_task *tl;
+	struct crypto_async_request     *lreq[MAXTASK];
+	int ct;
 #ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
 	unsigned long stat_req;
+	int tmax;
 #endif
 };
 
@@ -185,6 +189,7 @@ struct sun8i_ce_dev {
  * @ivlen:	size of bounce_iv
  * @nr_sgs:	The number of source SG (as given by dma_map_sg())
  * @nr_sgd:	The number of destination SG (as given by dma_map_sg())
+ * @slot:	The slot in the tasklist used for this requests
  */
 struct sun8i_cipher_req_ctx {
 	u32 op_dir;
@@ -194,6 +199,7 @@ struct sun8i_cipher_req_ctx {
 	unsigned int ivlen;
 	int nr_sgs;
 	int nr_sgd;
+	int slot;
 };
 
 /*
-- 
2.24.1

Re: [PATCH 5/9] crypto: engine: add enqueue_request/can_do_more

[Iuliana Prodan]

On 1/22/2020 12:45 PM, Corentin Labbe wrote:
> This patchs adds two new function wrapper in crypto_engine.
> - enqueue_request() for drivers enqueuing request to hardware.
> - can_queue_more() for letting drivers to tell if they can
> enqueue/prepare more.
> 
> Since some drivers (like caam) only enqueue request without "doing"
> them, do_one_request() is now optional.
> 
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> ---
>   crypto/crypto_engine.c  | 25 ++++++++++++++++++++++---
>   include/crypto/engine.h | 14 ++++++++------
>   2 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
> index 5bcb1e740fd9..4a28548c49aa 100644
> --- a/crypto/crypto_engine.c
> +++ b/crypto/crypto_engine.c
> @@ -83,6 +83,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>   		goto out;
>   	}
>   
> +retry:
>   	/* Get the fist request from the engine queue to handle */
>   	backlog = crypto_get_backlog(&engine->queue);
>   	async_req = crypto_dequeue_request(&engine->queue);
> @@ -118,10 +119,28 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>   			goto req_err2;
>   		}
>   	}
> +
> +	if (enginectx->op.enqueue_request) {
> +		ret = enginectx->op.enqueue_request(engine, async_req);
> +		if (ret) {
> +			dev_err(engine->dev, "failed to enqueue request: %d\n",
> +				ret);
> +			goto req_err;
> +		}
> +	}
> +	if (enginectx->op.can_queue_more && engine->queue.qlen > 0) {
> +		ret = enginectx->op.can_queue_more(engine, async_req);
> +		if (ret > 0) {
> +			spin_lock_irqsave(&engine->queue_lock, flags);
> +			goto retry;
> +		}
> +		if (ret < 0) {
> +			dev_err(engine->dev, "failed to call can_queue_more\n");
> +			/* TODO */
> +		}
> +	}
>   	if (!enginectx->op.do_one_request) {
> -		dev_err(engine->dev, "failed to do request\n");
> -		ret = -EINVAL;
> -		goto req_err;
> +		return;
>   	}
>   	ret = enginectx->op.do_one_request(engine, async_req);
>   	if (ret) {
> diff --git a/include/crypto/engine.h b/include/crypto/engine.h
> index 03d9f9ec1cea..8ab9d26e30fe 100644
> --- a/include/crypto/engine.h
> +++ b/include/crypto/engine.h
> @@ -63,14 +63,16 @@ struct crypto_engine {
>    * @prepare__request: do some prepare if need before handle the current request
>    * @unprepare_request: undo any work done by prepare_request()
>    * @do_one_request: do encryption for current request
> + * @enqueue_request:	Enqueue the request in the hardware
> + * @can_queue_more:	if this function return > 0, it will tell the crypto
> + * 	engine that more space are availlable for prepare/enqueue request
>    */
>   struct crypto_engine_op {
> -	int (*prepare_request)(struct crypto_engine *engine,
> -			       void *areq);
> -	int (*unprepare_request)(struct crypto_engine *engine,
> -				 void *areq);
> -	int (*do_one_request)(struct crypto_engine *engine,
> -			      void *areq);
> +	int (*prepare_request)(struct crypto_engine *engine, void *areq);
> +	int (*unprepare_request)(struct crypto_engine *engine, void *areq);
> +	int (*do_one_request)(struct crypto_engine *engine, void *areq);
> +	int (*enqueue_request)(struct crypto_engine *engine, void *areq);
> +	int (*can_queue_more)(struct crypto_engine *engine, void *areq);
>   };

As I mentioned in another thread [1], these crypto-engine patches (#1 - 
#5) imply modifications in all the drivers that use crypto-engine.
It's not backwards compatible.
Your changes imply that do_one_request executes the request & waits for 
completion and enqueue_request sends it to hardware. That means that all 
the other drivers need to be modify, to implement enqueue_request, 
instead of do_one_request. They need to be compliant with the new 
changes, new API. Otherwise, they are not using crypto-engine right, 
don't you think?

Also, do_one_request it shouldn’t be blocking. We got this confirmation 
from Herbert [2].

[1] 
https://lore.kernel.org/lkml/VI1PR04MB44455343230CBA7400D21C998C0C0@VI1PR04MB4445.eurprd04.prod.outlook.com/
[2] 
https://lore.kernel.org/lkml/20200122144134.axqpwx65j7xysyy3@gondor.apana.org.au/

Re: [PATCH 5/9] crypto: engine: add enqueue_request/can_do_more

[Corentin Labbe]

On Mon, Jan 27, 2020 at 10:58:36PM +0000, Iuliana Prodan wrote:
> On 1/22/2020 12:45 PM, Corentin Labbe wrote:
> > This patchs adds two new function wrapper in crypto_engine.
> > - enqueue_request() for drivers enqueuing request to hardware.
> > - can_queue_more() for letting drivers to tell if they can
> > enqueue/prepare more.
> > 
> > Since some drivers (like caam) only enqueue request without "doing"
> > them, do_one_request() is now optional.
> > 
> > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> > ---
> >   crypto/crypto_engine.c  | 25 ++++++++++++++++++++++---
> >   include/crypto/engine.h | 14 ++++++++------
> >   2 files changed, 30 insertions(+), 9 deletions(-)
> > 
> > diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
> > index 5bcb1e740fd9..4a28548c49aa 100644
> > --- a/crypto/crypto_engine.c
> > +++ b/crypto/crypto_engine.c
> > @@ -83,6 +83,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
> >   		goto out;
> >   	}
> >   
> > +retry:
> >   	/* Get the fist request from the engine queue to handle */
> >   	backlog = crypto_get_backlog(&engine->queue);
> >   	async_req = crypto_dequeue_request(&engine->queue);
> > @@ -118,10 +119,28 @@ static void crypto_pump_requests(struct crypto_engine *engine,
> >   			goto req_err2;
> >   		}
> >   	}
> > +
> > +	if (enginectx->op.enqueue_request) {
> > +		ret = enginectx->op.enqueue_request(engine, async_req);
> > +		if (ret) {
> > +			dev_err(engine->dev, "failed to enqueue request: %d\n",
> > +				ret);
> > +			goto req_err;
> > +		}
> > +	}
> > +	if (enginectx->op.can_queue_more && engine->queue.qlen > 0) {
> > +		ret = enginectx->op.can_queue_more(engine, async_req);
> > +		if (ret > 0) {
> > +			spin_lock_irqsave(&engine->queue_lock, flags);
> > +			goto retry;
> > +		}
> > +		if (ret < 0) {
> > +			dev_err(engine->dev, "failed to call can_queue_more\n");
> > +			/* TODO */
> > +		}
> > +	}
> >   	if (!enginectx->op.do_one_request) {
> > -		dev_err(engine->dev, "failed to do request\n");
> > -		ret = -EINVAL;
> > -		goto req_err;
> > +		return;
> >   	}
> >   	ret = enginectx->op.do_one_request(engine, async_req);
> >   	if (ret) {
> > diff --git a/include/crypto/engine.h b/include/crypto/engine.h
> > index 03d9f9ec1cea..8ab9d26e30fe 100644
> > --- a/include/crypto/engine.h
> > +++ b/include/crypto/engine.h
> > @@ -63,14 +63,16 @@ struct crypto_engine {
> >    * @prepare__request: do some prepare if need before handle the current request
> >    * @unprepare_request: undo any work done by prepare_request()
> >    * @do_one_request: do encryption for current request
> > + * @enqueue_request:	Enqueue the request in the hardware
> > + * @can_queue_more:	if this function return > 0, it will tell the crypto
> > + * 	engine that more space are availlable for prepare/enqueue request
> >    */
> >   struct crypto_engine_op {
> > -	int (*prepare_request)(struct crypto_engine *engine,
> > -			       void *areq);
> > -	int (*unprepare_request)(struct crypto_engine *engine,
> > -				 void *areq);
> > -	int (*do_one_request)(struct crypto_engine *engine,
> > -			      void *areq);
> > +	int (*prepare_request)(struct crypto_engine *engine, void *areq);
> > +	int (*unprepare_request)(struct crypto_engine *engine, void *areq);
> > +	int (*do_one_request)(struct crypto_engine *engine, void *areq);
> > +	int (*enqueue_request)(struct crypto_engine *engine, void *areq);
> > +	int (*can_queue_more)(struct crypto_engine *engine, void *areq);
> >   };
> 
> As I mentioned in another thread [1], these crypto-engine patches (#1 - 
> #5) imply modifications in all the drivers that use crypto-engine.
> It's not backwards compatible.

This is wrong. This is false.
AS I HAVE ALREADY SAID, I have tested and didnt see any behavour change in the current user of crypto engine.
I have tested my serie with omap, virtio, amlogic, sun8i-ss, sun8i-ce and didnt see any change in behavour WITHOUT CHANGING them.
I resaid, I didnt touch omap, virtio, etc...
Only stm32 is not tested because simply there are not board with this driver enabled.

I have also tested your serie which adds support for crypto engine to caam, and the crash is the same with/without my serie.
So no behavour change.

> Your changes imply that do_one_request executes the request & waits for 
> completion and enqueue_request sends it to hardware. That means that all 
> the other drivers need to be modify, to implement enqueue_request, 
> instead of do_one_request. They need to be compliant with the new 
> changes, new API. Otherwise, they are not using crypto-engine right, 
> don't you think?
> 

My change imply nothing, current user work the same.
But if they want, they COULD switch to enqueue_request().

> Also, do_one_request it shouldn’t be blocking. We got this confirmation 
> from Herbert [2].

Re-read what Herbert said, "It certainly shouldn't be blocking in the general case." But that means it could.
But this wont change my patch since both behavour are supported.

> 
> [1] 
> https://lore.kernel.org/lkml/VI1PR04MB44455343230CBA7400D21C998C0C0@VI1PR04MB4445.eurprd04.prod.outlook.com/
> [2] 
> https://lore.kernel.org/lkml/20200122144134.axqpwx65j7xysyy3@gondor.apana.org.au/

Re: [PATCH 5/9] crypto: engine: add enqueue_request/can_do_more

[Iuliana Prodan]

On 1/28/2020 10:40 AM, Corentin Labbe wrote:
> On Mon, Jan 27, 2020 at 10:58:36PM +0000, Iuliana Prodan wrote:
>> On 1/22/2020 12:45 PM, Corentin Labbe wrote:
>>> This patchs adds two new function wrapper in crypto_engine.
>>> - enqueue_request() for drivers enqueuing request to hardware.
>>> - can_queue_more() for letting drivers to tell if they can
>>> enqueue/prepare more.
>>>
>>> Since some drivers (like caam) only enqueue request without "doing"
>>> them, do_one_request() is now optional.
>>>
>>> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
>>> ---
>>>    crypto/crypto_engine.c  | 25 ++++++++++++++++++++++---
>>>    include/crypto/engine.h | 14 ++++++++------
>>>    2 files changed, 30 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
>>> index 5bcb1e740fd9..4a28548c49aa 100644
>>> --- a/crypto/crypto_engine.c
>>> +++ b/crypto/crypto_engine.c
>>> @@ -83,6 +83,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>>>    		goto out;
>>>    	}
>>>    
>>> +retry:
>>>    	/* Get the fist request from the engine queue to handle */
>>>    	backlog = crypto_get_backlog(&engine->queue);
>>>    	async_req = crypto_dequeue_request(&engine->queue);
>>> @@ -118,10 +119,28 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>>>    			goto req_err2;
>>>    		}
>>>    	}
>>> +
>>> +	if (enginectx->op.enqueue_request) {
>>> +		ret = enginectx->op.enqueue_request(engine, async_req);
>>> +		if (ret) {
>>> +			dev_err(engine->dev, "failed to enqueue request: %d\n",
>>> +				ret);
>>> +			goto req_err;
>>> +		}
>>> +	}
>>> +	if (enginectx->op.can_queue_more && engine->queue.qlen > 0) {
>>> +		ret = enginectx->op.can_queue_more(engine, async_req);
>>> +		if (ret > 0) {
>>> +			spin_lock_irqsave(&engine->queue_lock, flags);
>>> +			goto retry;
>>> +		}
>>> +		if (ret < 0) {
>>> +			dev_err(engine->dev, "failed to call can_queue_more\n");
>>> +			/* TODO */
>>> +		}
>>> +	}
>>>    	if (!enginectx->op.do_one_request) {
>>> -		dev_err(engine->dev, "failed to do request\n");
>>> -		ret = -EINVAL;
>>> -		goto req_err;
>>> +		return;
>>>    	}
>>>    	ret = enginectx->op.do_one_request(engine, async_req);
>>>    	if (ret) {
>>> diff --git a/include/crypto/engine.h b/include/crypto/engine.h
>>> index 03d9f9ec1cea..8ab9d26e30fe 100644
>>> --- a/include/crypto/engine.h
>>> +++ b/include/crypto/engine.h
>>> @@ -63,14 +63,16 @@ struct crypto_engine {
>>>     * @prepare__request: do some prepare if need before handle the current request
>>>     * @unprepare_request: undo any work done by prepare_request()
>>>     * @do_one_request: do encryption for current request
>>> + * @enqueue_request:	Enqueue the request in the hardware
>>> + * @can_queue_more:	if this function return > 0, it will tell the crypto
>>> + * 	engine that more space are availlable for prepare/enqueue request
>>>     */
>>>    struct crypto_engine_op {
>>> -	int (*prepare_request)(struct crypto_engine *engine,
>>> -			       void *areq);
>>> -	int (*unprepare_request)(struct crypto_engine *engine,
>>> -				 void *areq);
>>> -	int (*do_one_request)(struct crypto_engine *engine,
>>> -			      void *areq);
>>> +	int (*prepare_request)(struct crypto_engine *engine, void *areq);
>>> +	int (*unprepare_request)(struct crypto_engine *engine, void *areq);
>>> +	int (*do_one_request)(struct crypto_engine *engine, void *areq);
>>> +	int (*enqueue_request)(struct crypto_engine *engine, void *areq);
>>> +	int (*can_queue_more)(struct crypto_engine *engine, void *areq);
>>>    };
>>
>> As I mentioned in another thread [1], these crypto-engine patches (#1 -
>> #5) imply modifications in all the drivers that use crypto-engine.
>> It's not backwards compatible.
> 
> This is wrong. This is false.
> AS I HAVE ALREADY SAID, I have tested and didnt see any behavour change in the current user of crypto engine.
> I have tested my serie with omap, virtio, amlogic, sun8i-ss, sun8i-ce and didnt see any change in behavour WITHOUT CHANGING them.
> I resaid, I didnt touch omap, virtio, etc...
> Only stm32 is not tested because simply there are not board with this driver enabled.
> 

I'm not saying that doesn't compile or anything, is just that you change 
the API and those drivers will not comply on this new API.
I believe that "it works" is not sufficient, should work properly!

> I have also tested your serie which adds support for crypto engine to caam, and the crash is the same with/without my serie.
> So no behavour change.
> 
Thanks for testing this. I'll look into it!

>> Your changes imply that do_one_request executes the request & waits for
>> completion and enqueue_request sends it to hardware. That means that all
>> the other drivers need to be modify, to implement enqueue_request,
>> instead of do_one_request. They need to be compliant with the new
>> changes, new API. Otherwise, they are not using crypto-engine right,
>> don't you think?
>>
> 
> My change imply nothing, current user work the same.
> But if they want, they COULD switch to enqueue_request().
> 
>> Also, do_one_request it shouldn’t be blocking. We got this confirmation
>> from Herbert [2].
> 
> Re-read what Herbert said, "It certainly shouldn't be blocking in the general case." But that means it could.
> But this wont change my patch since both behavour are supported.
> 

Since your driver is the one being different (implements do_one_request 
as blocking), it's not fair to change the other drivers just for you 
special case when we update the crypto-engine. It should be the other 
way around.
Add a special case for you and let the other drivers unchanged.
The updated crypto-engine API should be consistent (same semantics) with 
the old one. Your proposal doesn't extend the current API, but 
reshuffles the callbacks changing their meaning.
We should agree on how we should update crypto-engine to accommodate all 
the scenarios, but maintaining backwards compatibility.

Thanks,
Iulia

>>
>> [1]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FVI1PR04MB44455343230CBA7400D21C998C0C0%40VI1PR04MB4445.eurprd04.prod.outlook.com%2F&amp;data=02%7C01%7Ciuliana.prodan%40nxp.com%7C238e3e9a8e5f4d934cf308d7a3cdc3da%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637157976462463995&amp;sdata=rgzYhX0g9hrzlYcHs7aUWVNFYs6mj86gDu7YIowy0Nk%3D&amp;reserved=0
>> [2]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20200122144134.axqpwx65j7xysyy3%40gondor.apana.org.au%2F&amp;data=02%7C01%7Ciuliana.prodan%40nxp.com%7C238e3e9a8e5f4d934cf308d7a3cdc3da%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637157976462463995&amp;sdata=Jdp0Q8xPnn5uXtcv6hrk3sFbeC5PgzfwRys2itmL09w%3D&amp;reserved=0
> 

Re: [PATCH 1/9] crypto: engine: workqueue can only be processed one by one

[Horia Geanta]

On 1/22/2020 12:46 PM, Corentin Labbe wrote:
> Some bykeshedding are unnecessary since a workqueue can only be executed
> one by one.
> This behaviour is documented in:
> - kernel/kthread.c: comment of kthread_worker_fn()
> - Documentation/core-api/workqueue.rst: the functions associated with the work items one after the other
[...]
> @@ -73,16 +73,6 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>  
>  	spin_lock_irqsave(&engine->queue_lock, flags);
>  
> -	/* Make sure we are not already running a request */
> -	if (engine->cur_req)
> -		goto out;
> -
This check is here for a good reason, namely because crypto engine
cannot currently handle multiple crypto requests being in "flight"
in parallel.

More exactly, if this check is removed the following sequence could occur:
crypto_pump_work() -> crypto_pump_requests() -> .do_one_request(areq1)
crypto_pump_work() -> crypto_pump_requests() -> .do_one_request(areq2)
crypto_finalize_request(areq1)
crypto_finalize_request(areq2)

This isn't correctly handled in crypto_finalize_request(),
since .unprepare_request will be called only for areq2.

/**
 * crypto_finalize_request - finalize one request if the request is done
 * @engine: the hardware engine
 * @req: the request need to be finalized
 * @err: error number
 */
static void crypto_finalize_request(struct crypto_engine *engine,
			     struct crypto_async_request *req, int err)
{
	unsigned long flags;
	bool finalize_cur_req = false;
	int ret;
	struct crypto_engine_ctx *enginectx;

	spin_lock_irqsave(&engine->queue_lock, flags);
	if (engine->cur_req == req)
		finalize_cur_req = true;
	spin_unlock_irqrestore(&engine->queue_lock, flags);

	if (finalize_cur_req) {
		enginectx = crypto_tfm_ctx(req->tfm);
		if (engine->cur_req_prepared &&
		    enginectx->op.unprepare_request) {
			ret = enginectx->op.unprepare_request(engine, req);
			if (ret)
				dev_err(engine->dev, "failed to unprepare request\n");
		}
		spin_lock_irqsave(&engine->queue_lock, flags);
		engine->cur_req = NULL;
		engine->cur_req_prepared = false;
		spin_unlock_irqrestore(&engine->queue_lock, flags);
	}

	req->complete(req, err);

	kthread_queue_work(engine->kworker, &engine->pump_requests);
}

Horia

Re: [PATCH 1/9] crypto: engine: workqueue can only be processed one by one

[Corentin Labbe]

On Tue, Jan 28, 2020 at 03:50:14PM +0000, Horia Geanta wrote:
> On 1/22/2020 12:46 PM, Corentin Labbe wrote:
> > Some bykeshedding are unnecessary since a workqueue can only be executed
> > one by one.
> > This behaviour is documented in:
> > - kernel/kthread.c: comment of kthread_worker_fn()
> > - Documentation/core-api/workqueue.rst: the functions associated with the work items one after the other
> [...]
> > @@ -73,16 +73,6 @@ static void crypto_pump_requests(struct crypto_engine *engine,
> >  
> >  	spin_lock_irqsave(&engine->queue_lock, flags);
> >  
> > -	/* Make sure we are not already running a request */
> > -	if (engine->cur_req)
> > -		goto out;
> > -
> This check is here for a good reason, namely because crypto engine
> cannot currently handle multiple crypto requests being in "flight"
> in parallel.
> 
> More exactly, if this check is removed the following sequence could occur:
> crypto_pump_work() -> crypto_pump_requests() -> .do_one_request(areq1)
> crypto_pump_work() -> crypto_pump_requests() -> .do_one_request(areq2)
> crypto_finalize_request(areq1)
> crypto_finalize_request(areq2)
> 

As explained in the commitlog, crypto_pump_work() cannot be ran twice.

Regards

Re: [PATCH 1/9] crypto: engine: workqueue can only be processed one by one

[Corentin Labbe]

On Tue, Jan 28, 2020 at 04:58:00PM +0100, Corentin Labbe wrote:
> On Tue, Jan 28, 2020 at 03:50:14PM +0000, Horia Geanta wrote:
> > On 1/22/2020 12:46 PM, Corentin Labbe wrote:
> > > Some bykeshedding are unnecessary since a workqueue can only be executed
> > > one by one.
> > > This behaviour is documented in:
> > > - kernel/kthread.c: comment of kthread_worker_fn()
> > > - Documentation/core-api/workqueue.rst: the functions associated with the work items one after the other
> > [...]
> > > @@ -73,16 +73,6 @@ static void crypto_pump_requests(struct crypto_engine *engine,
> > >  
> > >  	spin_lock_irqsave(&engine->queue_lock, flags);
> > >  
> > > -	/* Make sure we are not already running a request */
> > > -	if (engine->cur_req)
> > > -		goto out;
> > > -
> > This check is here for a good reason, namely because crypto engine
> > cannot currently handle multiple crypto requests being in "flight"
> > in parallel.
> > 
> > More exactly, if this check is removed the following sequence could occur:
> > crypto_pump_work() -> crypto_pump_requests() -> .do_one_request(areq1)
> > crypto_pump_work() -> crypto_pump_requests() -> .do_one_request(areq2)
> > crypto_finalize_request(areq1)
> > crypto_finalize_request(areq2)
> > 
> 
> As explained in the commitlog, crypto_pump_work() cannot be ran twice.
> 

Sorry, I have misunderstood and wrongly answered.

Right since some driver does not block on do_one_request(), crypto_pump_work() can be ran one after one and so launch two request.

So this patch is bad.

Regards