[PATCH 0/4] Introduce bulk mode for crypto engine framework

[PATCH 0/4] Introduce bulk mode for crypto engine framework

[Baolin Wang]

Now some cipher hardware engines prefer to handle bulk block by merging requests
to increase the block size and thus increase the hardware engine processing speed.

This patchset introduces request bulk mode to help the crypto hardware drivers
improve in efficiency.

Baolin Wang (4):
  scatterlist: Introduce some helper functions
  crypto: Introduce some helper functions to help to merge requests
  crypto: Introduce the bulk mode for crypto engine framework
  md: dm-crypt: Initialize the sector number for one request

 crypto/Kconfig               |    1 +
 crypto/ablk_helper.c         |  135 ++++++++++++++++++++++++++++++++++++++++++
 crypto/crypto_engine.c       |  122 +++++++++++++++++++++++++++++++++++++-
 drivers/crypto/omap-aes.c    |    2 +-
 drivers/md/dm-crypt.c        |    1 +
 include/crypto/ablk_helper.h |    3 +
 include/crypto/algapi.h      |   23 ++++++-
 include/linux/crypto.h       |    5 ++
 include/linux/scatterlist.h  |   33 +++++++++++
 lib/scatterlist.c            |   59 ++++++++++++++++++
 10 files changed, 379 insertions(+), 5 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] scatterlist: Introduce some helper functions

[Baolin Wang]

In crypto engine framework, one request can combine other requests'
scatterlists into its sg table to improve engine efficency with
handling bulk block. Thus we need some helper functions to manage
dynamic scattertables.

This patch introduces 'sg_is_contiguous()' function to check if two
scatterlists are contiguous, 'sg_alloc_empty_table()' function to
allocate one empty sg table, 'sg_add_sg_to_table()' function to add
one scatterlist into sg table and 'sg_table_is_empty' function to
check if the sg table is empty.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 include/linux/scatterlist.h |   33 ++++++++++++++++++++++++
 lib/scatterlist.c           |   59 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 556ec1e..ae9aee6 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -212,6 +212,37 @@ static inline void sg_unmark_end(struct scatterlist *sg)
 }
 
 /**
+ * sg_is_contiguous - Check if the scatterlists are contiguous
+ * @sga: SG entry
+ * @sgb: SG entry
+ *
+ * Description:
+ *   If the sga scatterlist is contiguous with the sgb scatterlist,
+ *   that means they can be merged together.
+ *
+ **/
+static inline bool sg_is_contiguous(struct scatterlist *sga,
+				    struct scatterlist *sgb)
+{
+	return ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
+		(sgb->page_link & ~0x3UL));
+}
+
+/**
+ * sg_table_is_empty - Check if the sg table is empty
+ * @sgt: sg table
+ *
+ * Description:
+ *   The 'orig_nents' member of one sg table is used to indicate how many
+ *   scatterlists in the sg table.
+ *
+ **/
+static inline bool sg_table_is_empty(struct sg_table *sgt)
+{
+	return !sgt->orig_nents;
+}
+
+/**
  * sg_phys - Return physical address of an sg entry
  * @sg:	     SG entry
  *
@@ -261,6 +292,8 @@ void sg_free_table(struct sg_table *);
 int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int,
 		     struct scatterlist *, gfp_t, sg_alloc_fn *);
 int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
+int sg_alloc_empty_table(struct sg_table *, unsigned int, gfp_t);
+int sg_add_sg_to_table(struct sg_table *, struct scatterlist *);
 int sg_alloc_table_from_pages(struct sg_table *sgt,
 	struct page **pages, unsigned int n_pages,
 	unsigned long offset, unsigned long size,
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 004fc70..e9e5d5a 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
 EXPORT_SYMBOL(sg_alloc_table);
 
 /**
+ * sg_add_sg_to_table - Add one scatterlist into sg table
+ * @sgt:	The sg table header to use
+ * @src:	The sg need to be added into sg table
+ *
+ * Description:
+ *   The 'nents' member indicates how many scatterlists added in the sg table.
+ *   Copy the @src@ scatterlist into sg table and increase 'nents' member.
+ *
+ **/
+int sg_add_sg_to_table(struct sg_table *sgt, struct scatterlist *src)
+{
+	unsigned int i = 0, orig_nents = sgt->orig_nents;
+	struct scatterlist *sgl = sgt->sgl;
+	struct scatterlist *sg;
+
+	/* Check if there are enough space for the new sg to be added */
+	if (sgt->nents >= sgt->orig_nents)
+		return -EINVAL;
+
+	for_each_sg(sgl, sg, orig_nents, i) {
+		if (sgt->nents > 0 && i == (sgt->nents - 1)) {
+			sg_unmark_end(sg);
+		} else if (i == sgt->nents) {
+			memcpy(sg, src, sizeof(struct scatterlist));
+			sg_mark_end(sg);
+			sgt->nents++;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * sg_alloc_empty_table - Allocate one empty sg table
+ * @sgt:	The sg table header to use
+ * @nents:	Number of entries in sg list
+ * @gfp_mask:	GFP allocation mask
+ *
+ *  Description:
+ *    Allocate and initialize an sg table. The 'nents' member of sg_table
+ *    indicates how many scatterlists added in the sg table. It should set
+ *    0 which means there are no scatterlists added in this sg table now.
+ *
+ **/
+int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
+			 gfp_t gfp_mask)
+{
+	int ret;
+
+	ret = sg_alloc_table(sgt, nents, gfp_mask);
+	if (ret)
+		return ret;
+
+	sgt->nents = 0;
+	return 0;
+}
+
+/**
  * sg_alloc_table_from_pages - Allocate and initialize an sg table from
  *			       an array of pages
  * @sgt:	The sg table header to use
-- 
1.7.9.5

[PATCH 2/4] crypto: Introduce some helper functions to help to merge requests

[Baolin Wang]

Usually the dm-crypt subsystem will send encryption/descryption requests to
the crypto layer one block at a time, making each request 512 bytes long,
which is a much smaller size for hardware engine, that means the hardware
engine can not play its best performance.

Now some cipher hardware engines prefer to handle bulk block rather than one
sector (512 bytes) created by dm-crypt, cause these cipher engines can handle
the intermediate values (IV) by themselves in one bulk block. This means we
can increase the size of the request by merging request rather than always 512
bytes and thus increase the hardware engine processing speed.

This patch introduces some helper functions to help to merge requests to improve
hardware engine efficiency.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 crypto/ablk_helper.c         |  135 ++++++++++++++++++++++++++++++++++++++++++
 include/crypto/ablk_helper.h |    3 +
 include/linux/crypto.h       |    5 ++
 3 files changed, 143 insertions(+)

diff --git a/crypto/ablk_helper.c b/crypto/ablk_helper.c
index e1fcf53..3cf15cb 100644
--- a/crypto/ablk_helper.c
+++ b/crypto/ablk_helper.c
@@ -26,6 +26,7 @@
 
 #include <linux/kernel.h>
 #include <linux/crypto.h>
+#include <linux/device-mapper.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/hardirq.h>
@@ -34,6 +35,140 @@
 #include <crypto/ablk_helper.h>
 #include <asm/simd.h>
 
+/**
+ * ablk_link_request_if_contigous - try to link one request into previous one
+ * if the page address is contiguous.
+ * @list_req: the request from queue list
+ * @req: the new request need to be merged
+ *
+ * If the listed request and new request's pages of the scatterlists are
+ * contiguous, then merge the scatterlists of new request into the listed one.
+ *
+ * Return true on success, others means failed.
+ */
+static bool ablk_link_request_if_contigous(struct ablkcipher_request *list_req,
+					   struct ablkcipher_request *req)
+{
+	struct scatterlist *last_src_sg =
+		sg_last(list_req->sgt_src.sgl, list_req->sgt_src.nents);
+	struct scatterlist *last_dst_sg =
+		sg_last(list_req->sgt_dst.sgl, list_req->sgt_dst.nents);
+	struct scatterlist *req_src_sg = req->src;
+	struct scatterlist *req_dst_sg = req->dst;
+
+	if (!last_src_sg || !last_dst_sg)
+		return false;
+
+	/* Check if the src/dst scatterlists are contiguous */
+	if (!sg_is_contiguous(last_src_sg, req_src_sg) ||
+	    !sg_is_contiguous(last_dst_sg, req_dst_sg))
+		return false;
+
+	/*
+	 * If the request can be merged into the listed request after the
+	 * checking, then expand the listed request scatterlists' length.
+	 */
+	last_src_sg->length += req_src_sg->length;
+	last_dst_sg->length += req_dst_sg->length;
+	list_req->nbytes += req->nbytes;
+
+	return true;
+}
+
+/**
+ * ablk_merge_request - try to merge one request into previous one
+ * @list_req: the request from queue list
+ * @req: the request need to be merged
+ *
+ * This function will create a dynamic scatterlist table for both source
+ * and destination if the request is the first coming in.
+ *
+ * Return true on success, others means failed.
+ */
+static bool ablk_merge_request(struct ablkcipher_request *list_req,
+			       struct ablkcipher_request *req)
+{
+	struct sg_table *sgt_src = &list_req->sgt_src;
+	struct sg_table *sgt_dst = &list_req->sgt_dst;
+	unsigned int nents = SG_MAX_SINGLE_ALLOC;
+
+	if (sg_table_is_empty(sgt_src)) {
+		if (sg_alloc_empty_table(sgt_src, nents, GFP_ATOMIC))
+			return false;
+
+		if (sg_add_sg_to_table(sgt_src, list_req->src))
+			return false;
+	}
+
+	if (sg_table_is_empty(sgt_dst)) {
+		if (sg_alloc_empty_table(sgt_dst, nents, GFP_ATOMIC))
+			return false;
+
+		if (sg_add_sg_to_table(sgt_dst, list_req->dst))
+			return false;
+	}
+
+	/*
+	 * Check if the new request is contiguous for the listed request,
+	 * if it is contiguous then merge the new request into the listed one.
+	 */
+	if (ablk_link_request_if_contigous(list_req, req))
+		return true;
+
+	if (sg_add_sg_to_table(sgt_src, req->src))
+		return false;
+
+	if (sg_add_sg_to_table(sgt_dst, req->dst))
+		return false;
+
+	list_req->nbytes += req->nbytes;
+	return true;
+}
+
+/**
+ * ablk_try_merge - try to merge one request into previous one
+ * @queue: the crypto queue list
+ * @req: the request need to be merged
+ *
+ * Note: The merging action should be under the spinlock or mutex protection.
+ *
+ * Return 0 on success and others are failed.
+ */
+int ablk_try_merge(struct crypto_queue *queue,
+		   struct ablkcipher_request *req)
+{
+	struct ablkcipher_request *list_req;
+	struct crypto_async_request *async_req;
+
+	list_for_each_entry(async_req, &queue->list, list) {
+		list_req = ablkcipher_request_cast(async_req);
+
+		if (list_req->base.flags != req->base.flags)
+			continue;
+
+		/* Check that the request adds up to an even number of sectors */
+		if (!IS_ALIGNED(list_req->nbytes, (1U << SECTOR_SHIFT)))
+			continue;
+
+		if (list_req->nbytes + req->nbytes > UINT_MAX)
+			continue;
+
+		/*
+		 * We first check that the sectors are adjacent so we don't
+		 * mistadly coalesce something that is contigous in memory but
+		 * not contigous on disk.
+		 */
+		if (list_req->sector + list_req->nbytes /
+		    (1U << SECTOR_SHIFT) == req->sector) {
+			if (ablk_merge_request(list_req, req))
+				return 0;
+		}
+	}
+
+	return 1;
+}
+EXPORT_SYMBOL_GPL(ablk_try_merge);
+
 int ablk_set_key(struct crypto_ablkcipher *tfm, const u8 *key,
 		 unsigned int key_len)
 {
diff --git a/include/crypto/ablk_helper.h b/include/crypto/ablk_helper.h
index 4f93df5..12ae00d 100644
--- a/include/crypto/ablk_helper.h
+++ b/include/crypto/ablk_helper.h
@@ -8,6 +8,7 @@
 #include <linux/crypto.h>
 #include <linux/kernel.h>
 #include <crypto/cryptd.h>
+#include <crypto/algapi.h>
 
 struct async_helper_ctx {
 	struct cryptd_ablkcipher *cryptd_tfm;
@@ -28,4 +29,6 @@ extern int ablk_init_common(struct crypto_tfm *tfm, const char *drv_name);
 
 extern int ablk_init(struct crypto_tfm *tfm);
 
+extern int ablk_try_merge(struct crypto_queue *queue,
+			  struct ablkcipher_request *req);
 #endif /* _CRYPTO_ABLK_HELPER_H */
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index e71cb70..f878bb1 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -21,6 +21,7 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/bug.h>
+#include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/uaccess.h>
@@ -170,6 +171,10 @@ struct ablkcipher_request {
 	struct scatterlist *src;
 	struct scatterlist *dst;
 
+	struct sg_table sgt_src;
+	struct sg_table sgt_dst;
+	sector_t	sector;
+
 	void *__ctx[] CRYPTO_MINALIGN_ATTR;
 };
 
-- 
1.7.9.5

[PATCH 3/4] crypto: Introduce the bulk mode for crypto engine framework

[Baolin Wang]

Now some cipher hardware engines prefer to handle bulk block by merging
requests to increase the block size and thus increase the hardware engine
processing speed.

This patch introduces request bulk mode to help the crypto hardware drivers
improve in efficiency, and chooses the suitable mode (SECTOR_MODE) for
initializing aes engine.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 crypto/Kconfig            |    1 +
 crypto/crypto_engine.c    |  122 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/crypto/omap-aes.c |    2 +-
 include/crypto/algapi.h   |   23 ++++++++-
 4 files changed, 143 insertions(+), 5 deletions(-)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index c844227..6a2f9a6 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -229,6 +229,7 @@ config CRYPTO_GLUE_HELPER_X86
 
 config CRYPTO_ENGINE
 	tristate
+	select CRYPTO_ABLK_HELPER
 
 comment "Authenticated Encryption with Associated Data"
 
diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
index a55c82d..0de5829 100644
--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -14,6 +14,7 @@
 
 #include <linux/err.h>
 #include <linux/delay.h>
+#include <crypto/ablk_helper.h>
 #include "internal.h"
 
 #define CRYPTO_ENGINE_MAX_QLEN 10
@@ -84,6 +85,17 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 
 	req = ablkcipher_request_cast(async_req);
 
+	/*
+	 * If the engine supports the bulk mode and the request is allocated the
+	 * sg table to expand scatterlists entries, then it need to point the
+	 * scatterlists from the sg table.
+	 */
+	if (engine->mode == SECTOR_BULK_MODE && req->sgt_src.orig_nents &&
+	    req->sgt_dst.orig_nents) {
+		req->src = req->sgt_src.sgl;
+		req->dst = req->sgt_dst.sgl;
+	}
+
 	engine->cur_req = req;
 	if (backlog)
 		backlog->complete(backlog, -EINPROGRESS);
@@ -137,9 +149,46 @@ static void crypto_pump_work(struct kthread_work *work)
 }
 
 /**
+ * crypto_merge_request_to_engine - try to merge one request into previous one
+ * @engine: the hardware engine
+ * @req: the request need to be merged
+ *
+ * If the crypto engine supports bulk mode, then try to merge the new request
+ * into the listed one from engine queue to handle them together.
+ *
+ * Return 0 on success and others are failed.
+ */
+static bool crypto_merge_request_to_engine(struct crypto_engine *engine,
+					   struct ablkcipher_request *req)
+{
+	/*
+	 * The request is allocated from memory pool in dm-crypt, here need to
+	 * do initialization for sg table in case some random values.
+	 */
+	req->sgt_src.orig_nents = 0;
+	req->sgt_dst.orig_nents = 0;
+
+	/*
+	 * If the hardware engine can not support the bulk mode encryption,
+	 * just return 1 means merging failed.
+	 */
+	if (engine->mode != SECTOR_BULK_MODE)
+		return 1;
+
+	return ablk_try_merge(&engine->queue, req);
+}
+
+/**
  * crypto_transfer_request - transfer the new request into the engine queue
  * @engine: the hardware engine
  * @req: the request need to be listed into the engine queue
+ *
+ * Firstly it will check if the new request can be merged into previous one
+ * if their secotr numbers are continuous, if not should list it into engine
+ * queue.
+ *
+ * If the new request can be merged into the previous request, then just
+ * finalize the new request.
  */
 int crypto_transfer_request(struct crypto_engine *engine,
 			    struct ablkcipher_request *req, bool need_pump)
@@ -154,6 +203,26 @@ int crypto_transfer_request(struct crypto_engine *engine,
 		return -ESHUTDOWN;
 	}
 
+	/*
+	 * Here need to check if the request can be merged into previous
+	 * request. If the hardware engine can support encryption with
+	 * bulk block, we can merge the new request into previous request
+	 * if their secotr numbers are continuous, which can be handled
+	 * together by engine to improve the encryption efficiency.
+	 * Return -EINPROGRESS means it has been merged into previous request,
+	 * so just end up this request.
+	 */
+	ret = crypto_merge_request_to_engine(engine, req);
+	if (!ret) {
+		spin_unlock_irqrestore(&engine->queue_lock, flags);
+		crypto_finalize_request(engine, req, 0);
+		return -EINPROGRESS;
+	}
+
+	/*
+	 * If the request can not be merged into previous request, then list it
+	 * into the queue of engine, and will be handled by kthread worker.
+	 */
 	ret = ablkcipher_enqueue_request(&engine->queue, req);
 
 	if (!engine->busy && need_pump)
@@ -178,7 +247,8 @@ int crypto_transfer_request_to_engine(struct crypto_engine *engine,
 EXPORT_SYMBOL_GPL(crypto_transfer_request_to_engine);
 
 /**
- * crypto_finalize_request - finalize one request if the request is done
+ * crypto_finalize_request - finalize one request if the request is done or
+ * merged into previous request
  * @engine: the hardware engine
  * @req: the request need to be finalized
  * @err: error number
@@ -208,9 +278,18 @@ void crypto_finalize_request(struct crypto_engine *engine,
 		spin_unlock_irqrestore(&engine->queue_lock, flags);
 	}
 
+	sg_free_table(&req->sgt_src);
+	sg_free_table(&req->sgt_dst);
 	req->base.complete(&req->base, err);
 
-	queue_kthread_work(&engine->kworker, &engine->pump_requests);
+	/*
+	 * If the request is finalized by merging into the previous request from
+	 * the engine queue, then it is no need to queue the kthread work.
+	 * Cause now maybe there are other requests need to be merged into the
+	 * listed request from one block, just wait for merging action.
+	 */
+	if (finalize_cur_req)
+		queue_kthread_work(&engine->kworker, &engine->pump_requests);
 }
 EXPORT_SYMBOL_GPL(crypto_finalize_request);
 
@@ -279,15 +358,45 @@ int crypto_engine_stop(struct crypto_engine *engine)
 EXPORT_SYMBOL_GPL(crypto_engine_stop);
 
 /**
+ * crypto_engine_change_mode - Change the mode for hardware engine
+ * @engine: the hardware engine
+ * @mode: engine mode to be set
+ *
+ * This function can change the hardware engine mode when the engine is running.
+ * Return 0 on success, else on fail.
+ */
+int crypto_engine_change_mode(struct crypto_engine *engine,
+			      enum engine_mode mode)
+{
+	unsigned long flags;
+	int ret;
+
+	ret = crypto_engine_stop(engine);
+	if (ret) {
+		pr_warn("could not change engine mode now\n");
+		return ret;
+	}
+
+	spin_lock_irqsave(&engine->queue_lock, flags);
+	engine->mode = mode;
+	spin_unlock_irqrestore(&engine->queue_lock, flags);
+
+	return crypto_engine_start(engine);
+}
+EXPORT_SYMBOL_GPL(crypto_engine_change_mode);
+
+/**
  * crypto_engine_alloc_init - allocate crypto hardware engine structure and
  * initialize it.
  * @dev: the device attached with one hardware engine
+ * @mode: crypto engine mode
  * @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)
+struct crypto_engine *crypto_engine_alloc_init(struct device *dev,
+					       enum engine_mode mode, bool rt)
 {
 	struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
 	struct crypto_engine *engine;
@@ -299,6 +408,13 @@ struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt)
 	if (!engine)
 		return NULL;
 
+	/*
+	 * If the hardware engine can handle the IV by itself, that means it
+	 * just need one initial IV for multiple requests from one block. So
+	 * we can merge requests from one block into one request to handle,
+	 * which can improve the hardware engine efficiency.
+	 */
+	engine->mode = mode;
 	engine->rt = rt;
 	engine->running = false;
 	engine->busy = false;
diff --git a/drivers/crypto/omap-aes.c b/drivers/crypto/omap-aes.c
index d420ec7..946f11f 100644
--- a/drivers/crypto/omap-aes.c
+++ b/drivers/crypto/omap-aes.c
@@ -1230,7 +1230,7 @@ static int omap_aes_probe(struct platform_device *pdev)
 	}
 
 	/* Initialize crypto engine */
-	dd->engine = crypto_engine_alloc_init(dev, 1);
+	dd->engine = crypto_engine_alloc_init(dev, SECTOR_MODE, 1);
 	if (!dd->engine)
 		goto err_algs;
 
diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
index b09d43f..69fb43e 100644
--- a/include/crypto/algapi.h
+++ b/include/crypto/algapi.h
@@ -130,6 +130,22 @@ struct ablkcipher_walk {
 };
 
 #define ENGINE_NAME_LEN	30
+
+/*
+ * enum engine_mode - crypto engine mode
+ * @SECTOR_MODE: should do encryption/decryption one request by one (one
+ * request length is one sector size), and it will not coalesce requests.
+ * @SECTOR_BULK_MODE: it can coalesce the contiguous requests (one request
+ * length is one sector size) together to be one bulk request, which can be
+ * handled by crypto engine at one time.
+ * @MAX_MODE: engine mode numbers
+ */
+enum engine_mode {
+	SECTOR_MODE,
+	SECTOR_BULK_MODE,
+	MAX_MODE,
+};
+
 /*
  * struct crypto_engine - crypto hardware engine
  * @name: the engine name
@@ -140,6 +156,7 @@ struct ablkcipher_walk {
  * @list: link with the global crypto engine list
  * @queue_lock: spinlock to syncronise access to request queue
  * @queue: the crypto queue of the engine
+ * @mode: crypto engine mode
  * @rt: whether this queue is set to run as a realtime task
  * @prepare_crypt_hardware: a request will soon arrive from the queue
  * so the subsystem requests the driver to prepare the hardware
@@ -167,6 +184,7 @@ struct crypto_engine {
 	spinlock_t		queue_lock;
 	struct crypto_queue	queue;
 
+	enum engine_mode	mode;
 	bool			rt;
 
 	int (*prepare_crypt_hardware)(struct crypto_engine *engine);
@@ -195,7 +213,10 @@ void crypto_finalize_request(struct crypto_engine *engine,
 			     struct ablkcipher_request *req, int err);
 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);
+int crypto_engine_change_mode(struct crypto_engine *engine,
+			      enum engine_mode mode);
+struct crypto_engine *crypto_engine_alloc_init(struct device *dev,
+					       enum engine_mode mode, bool rt);
 int crypto_engine_exit(struct crypto_engine *engine);
 
 extern const struct crypto_type crypto_ablkcipher_type;
-- 
1.7.9.5

[PATCH 4/4] md: dm-crypt: Initialize the sector number for one request

[Baolin Wang]

If the crypto engine can support the bulk mode, that means the contiguous
requests from one block can be merged into one request to be handled by
crypto engine. If so, the crypto engine need the sector number of one request
to do merging action.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/md/dm-crypt.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3147c8d..9e2dbfd 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -866,6 +866,7 @@ static int crypt_convert_block(struct crypt_config *cc,
 			return r;
 	}
 
+	req->sector = ctx->cc_sector;
 	ablkcipher_request_set_crypt(req, &dmreq->sg_in, &dmreq->sg_out,
 				     1 << SECTOR_SHIFT, iv);
 
-- 
1.7.9.5

Re: [PATCH 1/4] scatterlist: Introduce some helper functions

[Robert Jarzmik]

Baolin Wang <baolin.wang@linaro.org> writes:

> @@ -212,6 +212,37 @@ static inline void sg_unmark_end(struct scatterlist *sg)
>  }
>  
>  /**
> + * sg_is_contiguous - Check if the scatterlists are contiguous
> + * @sga: SG entry
> + * @sgb: SG entry
> + *
> + * Description:
> + *   If the sga scatterlist is contiguous with the sgb scatterlist,
> + *   that means they can be merged together.
> + *
> + **/
> +static inline bool sg_is_contiguous(struct scatterlist *sga,
> +				    struct scatterlist *sgb)
> +{
> +	return ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
> +		(sgb->page_link & ~0x3UL));
> +}
I don't understand that one.
sga->page_link is a pointer to a "struct page *". How can it be added to an
offset within a page ???

> @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int
> nents, gfp_t gfp_mask)
...
>  /**
> + * sg_add_sg_to_table - Add one scatterlist into sg table
> + * @sgt:	The sg table header to use
> + * @src:	The sg need to be added into sg table
> + *
> + * Description:
> + *   The 'nents' member indicates how many scatterlists added in the sg table.
> + *   Copy the @src@ scatterlist into sg table and increase 'nents' member.
> + *
> + **/
> +int sg_add_sg_to_table(struct sg_table *sgt, struct scatterlist *src)
> +{
> +	unsigned int i = 0, orig_nents = sgt->orig_nents;
> +	struct scatterlist *sgl = sgt->sgl;
> +	struct scatterlist *sg;
> +
> +	/* Check if there are enough space for the new sg to be added */
> +	if (sgt->nents >= sgt->orig_nents)
> +		return -EINVAL;
I must admit I don't understand that one either : how do comparing the number of
"mapped" entries against the number of "allocated" entries determines if there
is enough room ?

> +/**
> + * sg_alloc_empty_table - Allocate one empty sg table
> + * @sgt:	The sg table header to use
> + * @nents:	Number of entries in sg list
> + * @gfp_mask:	GFP allocation mask
> + *
> + *  Description:
> + *    Allocate and initialize an sg table. The 'nents' member of sg_table
> + *    indicates how many scatterlists added in the sg table. It should set
> + *    0 which means there are no scatterlists added in this sg table now.
> + *
> + **/
> +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
> +			 gfp_t gfp_mask)
As for this one, there has to be a purpose for it I fail to see. From far away
it looks exactly like sg_alloc_table(), excepting it "works around" the nents >
0 protection of __sg_alloc_table().
What is exactly the need for this one, and if it's usefull why not simply
changing the __sg_alloc_table() "nents > 0" test and see what the outcome of the
review will be ?

Cheers.

-- 
Robert

Re: [PATCH 1/4] scatterlist: Introduce some helper functions

[Baolin Wang]

Hi Robert,

On 4 March 2016 at 03:15, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Baolin Wang <baolin.wang@linaro.org> writes:
>
>> @@ -212,6 +212,37 @@ static inline void sg_unmark_end(struct scatterlist *sg)
>>  }
>>
>>  /**
>> + * sg_is_contiguous - Check if the scatterlists are contiguous
>> + * @sga: SG entry
>> + * @sgb: SG entry
>> + *
>> + * Description:
>> + *   If the sga scatterlist is contiguous with the sgb scatterlist,
>> + *   that means they can be merged together.
>> + *
>> + **/
>> +static inline bool sg_is_contiguous(struct scatterlist *sga,
>> +                                 struct scatterlist *sgb)
>> +{
>> +     return ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
>> +             (sgb->page_link & ~0x3UL));
>> +}
> I don't understand that one.
> sga->page_link is a pointer to a "struct page *". How can it be added to an
> offset within a page ???


Ah, sorry that's a mistake. It should check as below:
static inline bool sg_is_contiguous(struct scatterlist *sga, struct
scatterlist *sgb)
{
    return (unsigned int)sg_virt(sga) + sga->length == (unsigned
int)sg_virt(sgb);
}

>
>> @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int
>> nents, gfp_t gfp_mask)
> ...
>>  /**
>> + * sg_add_sg_to_table - Add one scatterlist into sg table
>> + * @sgt:     The sg table header to use
>> + * @src:     The sg need to be added into sg table
>> + *
>> + * Description:
>> + *   The 'nents' member indicates how many scatterlists added in the sg table.
>> + *   Copy the @src@ scatterlist into sg table and increase 'nents' member.
>> + *
>> + **/
>> +int sg_add_sg_to_table(struct sg_table *sgt, struct scatterlist *src)
>> +{
>> +     unsigned int i = 0, orig_nents = sgt->orig_nents;
>> +     struct scatterlist *sgl = sgt->sgl;
>> +     struct scatterlist *sg;
>> +
>> +     /* Check if there are enough space for the new sg to be added */
>> +     if (sgt->nents >= sgt->orig_nents)
>> +             return -EINVAL;
> I must admit I don't understand that one either : how do comparing the number of
> "mapped" entries against the number of "allocated" entries determines if there
> is enough room ?

That's for a dynamic sg table. If there is one sg table allocated
'orig_nents' scatterlists, and we need copy another mapped scatterlist
into the sg table if there are some requirements. So we use 'nents' to
record how many scatterlists have been copied into the sg table.

>
>> +/**
>> + * sg_alloc_empty_table - Allocate one empty sg table
>> + * @sgt:     The sg table header to use
>> + * @nents:   Number of entries in sg list
>> + * @gfp_mask:        GFP allocation mask
>> + *
>> + *  Description:
>> + *    Allocate and initialize an sg table. The 'nents' member of sg_table
>> + *    indicates how many scatterlists added in the sg table. It should set
>> + *    0 which means there are no scatterlists added in this sg table now.
>> + *
>> + **/
>> +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
>> +                      gfp_t gfp_mask)
> As for this one, there has to be a purpose for it I fail to see. From far away
> it looks exactly like sg_alloc_table(), excepting it "works around" the nents >
> 0 protection of __sg_alloc_table().
> What is exactly the need for this one, and if it's usefull why not simply
> changing the __sg_alloc_table() "nents > 0" test and see what the outcome of the
> review will be ?

Like I said above. If we want to copy some mapped scatterlists into
one sg table, we should set the 'nents' to 0 to indicates how many
scatterlists coppied in the sg table.
Thanks for your comments.

>
> Cheers.
>
> --
> Robert



-- 
Baolin.wang
Best Regards

Re: [PATCH 1/4] scatterlist: Introduce some helper functions

[Baolin Wang]

>>> + **/
>>> +static inline bool sg_is_contiguous(struct scatterlist *sga,
>>> +                                 struct scatterlist *sgb)
>>> +{
>>> +     return ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
>>> +             (sgb->page_link & ~0x3UL));
>>> +}
>> I don't understand that one.
>> sga->page_link is a pointer to a "struct page *". How can it be added to an
>> offset within a page ???
>
>
> Ah, sorry that's a mistake. It should check as below:
> static inline bool sg_is_contiguous(struct scatterlist *sga, struct
> scatterlist *sgb)
> {
>     return (unsigned int)sg_virt(sga) + sga->length == (unsigned
> int)sg_virt(sgb);
> }

sorry, it should be:
static inline bool sg_is_contiguous(struct scatterlist *sga,
   struct scatterlist *sgb)
{
    return (unsigned long)sg_virt(sga) + sga->length ==
               (unsigned long)sg_virt(sgb);
}

-- 
Baolin.wang
Best Regards

Re: [PATCH 1/4] scatterlist: Introduce some helper functions

[Robert Jarzmik]

Baolin Wang <baolin.wang@linaro.org> writes:

> Hi Robert,
>
> On 4 March 2016 at 03:15, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> +static inline bool sg_is_contiguous(struct scatterlist *sga,
>>> +                                 struct scatterlist *sgb)
>>> +{
>>> +     return ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
>>> +             (sgb->page_link & ~0x3UL));
>>> +}
>> I don't understand that one.
>> sga->page_link is a pointer to a "struct page *". How can it be added to an
>> offset within a page ???
>
>
> Ah, sorry that's a mistake. It should check as below:
> static inline bool sg_is_contiguous(struct scatterlist *sga, struct
> scatterlist *sgb)
> {
>     return (unsigned int)sg_virt(sga) + sga->length == (unsigned
> int)sg_virt(sgb);
> }
NAK.
First, I don't like the cast.
Second ask yourself: what if you compile on a 64 bit architecture ?
Third, I don't like void* arithmetics, some compilers might refuse it.

And as a last comment, is it "virtual" contiguity you're looking after, physical
contiguity, or dma_addr_t contiguity ?

>>> @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int
>>> nents, gfp_t gfp_mask)
>> ...
>>>  /**
>>> + * sg_add_sg_to_table - Add one scatterlist into sg table
>>> + * @sgt:     The sg table header to use
>>> + * @src:     The sg need to be added into sg table
>>> + *
>>> + * Description:
>>> + *   The 'nents' member indicates how many scatterlists added in the sg table.
>>> + *   Copy the @src@ scatterlist into sg table and increase 'nents' member.
>>> + *
>>> + **/
>>> +int sg_add_sg_to_table(struct sg_table *sgt, struct scatterlist *src)
>>> +{
>>> +     unsigned int i = 0, orig_nents = sgt->orig_nents;
>>> +     struct scatterlist *sgl = sgt->sgl;
>>> +     struct scatterlist *sg;
>>> +
>>> +     /* Check if there are enough space for the new sg to be added */
>>> +     if (sgt->nents >= sgt->orig_nents)
>>> +             return -EINVAL;
>> I must admit I don't understand that one either : how do comparing the number of
>> "mapped" entries against the number of "allocated" entries determines if there
>> is enough room ?
>
> That's for a dynamic sg table. If there is one sg table allocated
> 'orig_nents' scatterlists, and we need copy another mapped scatterlist
> into the sg table if there are some requirements. So we use 'nents' to
> record how many scatterlists have been copied into the sg table.
As I'm still having difficulities to understand, please explain to me :
 - what is a dynamic sg_table
 - how it works
 - if it's "struct sg_table", how the field of this structure are different when
   it's a dynamic sg_table from a "static sg_table" ?

>>> +/**
>>> + * sg_alloc_empty_table - Allocate one empty sg table
>>> + * @sgt:     The sg table header to use
>>> + * @nents:   Number of entries in sg list
>>> + * @gfp_mask:        GFP allocation mask
>>> + *
>>> + *  Description:
>>> + *    Allocate and initialize an sg table. The 'nents' member of sg_table
>>> + *    indicates how many scatterlists added in the sg table. It should set
>>> + *    0 which means there are no scatterlists added in this sg table now.
>>> + *
>>> + **/
>>> +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
>>> +                      gfp_t gfp_mask)
>> As for this one, there has to be a purpose for it I fail to see. From far away
>> it looks exactly like sg_alloc_table(), excepting it "works around" the nents >
>> 0 protection of __sg_alloc_table().
>> What is exactly the need for this one, and if it's usefull why not simply
>> changing the __sg_alloc_table() "nents > 0" test and see what the outcome of the
>> review will be ?
>
> Like I said above. If we want to copy some mapped scatterlists into
> one sg table, we should set the 'nents' to 0 to indicates how many
> scatterlists coppied in the sg table.
So how do you unmap this scatterlist once you've lost the number of mapped
entries ?

I think we'll come back to this once I understand how a "dynamic" sg_table is
different from a "static" one.

Cheers.

-- 
Robert

Re: [PATCH 1/4] scatterlist: Introduce some helper functions

[Baolin Wang]

On 10 March 2016 at 17:42, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>>
>>
>> Ah, sorry that's a mistake. It should check as below:
>> static inline bool sg_is_contiguous(struct scatterlist *sga, struct
>> scatterlist *sgb)
>> {
>>     return (unsigned int)sg_virt(sga) + sga->length == (unsigned
>> int)sg_virt(sgb);
>> }
> NAK.
> First, I don't like the cast.
> Second ask yourself: what if you compile on a 64 bit architecture ?

Sorry, it's a mistake. I've resend one email to correct that with
'unsigned long'.

> Third, I don't like void* arithmetics, some compilers might refuse it.

OK. I will modify that.

>
> And as a last comment, is it "virtual" contiguity you're looking after, physical
> contiguity, or dma_addr_t contiguity ?

Yes, I need check the virtual contiguity.

>
>>>> @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int
>>>> nents, gfp_t gfp_mask)
>>> ...
>>>>  /**
>>>> + * sg_add_sg_to_table - Add one scatterlist into sg table
>>>> + * @sgt:     The sg table header to use
>>>> + * @src:     The sg need to be added into sg table
>>>> + *
>>>> + * Description:
>>>> + *   The 'nents' member indicates how many scatterlists added in the sg table.
>>>> + *   Copy the @src@ scatterlist into sg table and increase 'nents' member.
>>>> + *
>>>> + **/
>>>> +int sg_add_sg_to_table(struct sg_table *sgt, struct scatterlist *src)
>>>> +{
>>>> +     unsigned int i = 0, orig_nents = sgt->orig_nents;
>>>> +     struct scatterlist *sgl = sgt->sgl;
>>>> +     struct scatterlist *sg;
>>>> +
>>>> +     /* Check if there are enough space for the new sg to be added */
>>>> +     if (sgt->nents >= sgt->orig_nents)
>>>> +             return -EINVAL;
>>> I must admit I don't understand that one either : how do comparing the number of
>>> "mapped" entries against the number of "allocated" entries determines if there
>>> is enough room ?
>>
>> That's for a dynamic sg table. If there is one sg table allocated
>> 'orig_nents' scatterlists, and we need copy another mapped scatterlist
>> into the sg table if there are some requirements. So we use 'nents' to
>> record how many scatterlists have been copied into the sg table.
> As I'm still having difficulities to understand, please explain to me :
>  - what is a dynamic sg_table

OK. That's for some special usage. For example, the dm-crypt will send
one request mapped one scatterlist to engine level to handle the
request at one time. But we want to collect some requests together to
handle together at one time to improve engine efficiency. So in crypto
engine layer we need to copy the mapped scatterlists into one sg
table, which has been allocated some sg memory.

>  - how it works

The 'orig_nents' means how many scatterlists the sg table can hold.
The 'nents' means how many scatterlists have been copied into sg
table. So the 'nents' will be not equal as 'orig_nents' unless the sg
table is full.

>  - if it's "struct sg_table", how the field of this structure are different when
>    it's a dynamic sg_table from a "static sg_table" ?

>
>>>> +/**
>>>> + * sg_alloc_empty_table - Allocate one empty sg table
>>>> + * @sgt:     The sg table header to use
>>>> + * @nents:   Number of entries in sg list
>>>> + * @gfp_mask:        GFP allocation mask
>>>> + *
>>>> + *  Description:
>>>> + *    Allocate and initialize an sg table. The 'nents' member of sg_table
>>>> + *    indicates how many scatterlists added in the sg table. It should set
>>>> + *    0 which means there are no scatterlists added in this sg table now.
>>>> + *
>>>> + **/
>>>> +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
>>>> +                      gfp_t gfp_mask)
>>> As for this one, there has to be a purpose for it I fail to see. From far away
>>> it looks exactly like sg_alloc_table(), excepting it "works around" the nents >
>>> 0 protection of __sg_alloc_table().
>>> What is exactly the need for this one, and if it's usefull why not simply
>>> changing the __sg_alloc_table() "nents > 0" test and see what the outcome of the
>>> review will be ?
>>
>> Like I said above. If we want to copy some mapped scatterlists into
>> one sg table, we should set the 'nents' to 0 to indicates how many
>> scatterlists coppied in the sg table.
> So how do you unmap this scatterlist once you've lost the number of mapped
> entries ?

I will not lost the number of mapped in sg table,  'nents' member
means how many mapped scatterlists in the dynamic table. Every time
one mapped sg copied into the dynamic table, the 'nents' will increase
1.

>
> I think we'll come back to this once I understand how a "dynamic" sg_table is
> different from a "static" one.
>
> Cheers.
>
> --
> Robert



-- 
Baolin.wang
Best Regards