[PATCH v2 0/2] crypto: engine - Permit to enqueue skcipher request

[PATCH v2 0/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

The crypto engine could actually only enqueue hash and ablkcipher request.
This patch serie permit it to enqueue skcipher requets by adding all necessary
functions.

Changes since v1
- Aligned to column struct *dev in include
- Added missing mutex_unlock in crypto_engine_start()

Corentin Labbe (2):
  crypto: engine - replace pr_xxx by dev_xxx
  crypto: engine - Permit to enqueue skcipher request

 crypto/crypto_engine.c  | 157 ++++++++++++++++++++++++++++++++++++++++++------
 include/crypto/engine.h |  15 +++++
 2 files changed, 153 insertions(+), 19 deletions(-)

-- 
2.13.0

[PATCH v2 1/2] crypto: engine - replace pr_xxx by dev_xxx

[Corentin Labbe]

By adding a struct device *dev to struct engine, we could store the
device used at register time and so use all dev_xxx functions instead of
pr_xxx.

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

diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
index 727bd5c3569e..61e7c4e02fd2 100644
--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -70,7 +70,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 
 		if (engine->unprepare_crypt_hardware &&
 		    engine->unprepare_crypt_hardware(engine))
-			pr_err("failed to unprepare crypt hardware\n");
+			dev_err(engine->dev, "failed to unprepare crypt hardware\n");
 
 		spin_lock_irqsave(&engine->queue_lock, flags);
 		engine->idling = false;
@@ -99,7 +99,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 	if (!was_busy && engine->prepare_crypt_hardware) {
 		ret = engine->prepare_crypt_hardware(engine);
 		if (ret) {
-			pr_err("failed to prepare crypt hardware\n");
+			dev_err(engine->dev, "failed to prepare crypt hardware\n");
 			goto req_err;
 		}
 	}
@@ -110,14 +110,15 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 		if (engine->prepare_hash_request) {
 			ret = engine->prepare_hash_request(engine, hreq);
 			if (ret) {
-				pr_err("failed to prepare request: %d\n", ret);
+				dev_err(engine->dev, "failed to prepare request: %d\n",
+					ret);
 				goto req_err;
 			}
 			engine->cur_req_prepared = true;
 		}
 		ret = engine->hash_one_request(engine, hreq);
 		if (ret) {
-			pr_err("failed to hash one request from queue\n");
+			dev_err(engine->dev, "failed to hash one request from queue\n");
 			goto req_err;
 		}
 		return;
@@ -126,19 +127,20 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 		if (engine->prepare_cipher_request) {
 			ret = engine->prepare_cipher_request(engine, breq);
 			if (ret) {
-				pr_err("failed to prepare request: %d\n", ret);
+				dev_err(engine->dev, "failed to prepare request: %d\n",
+					ret);
 				goto req_err;
 			}
 			engine->cur_req_prepared = true;
 		}
 		ret = engine->cipher_one_request(engine, breq);
 		if (ret) {
-			pr_err("failed to cipher one request from queue\n");
+			dev_err(engine->dev, "failed to cipher one request from queue\n");
 			goto req_err;
 		}
 		return;
 	default:
-		pr_err("failed to prepare request of unknown type\n");
+		dev_err(engine->dev, "failed to prepare request of unknown type\n");
 		return;
 	}
 
@@ -275,7 +277,7 @@ void crypto_finalize_cipher_request(struct crypto_engine *engine,
 		    engine->unprepare_cipher_request) {
 			ret = engine->unprepare_cipher_request(engine, req);
 			if (ret)
-				pr_err("failed to unprepare request\n");
+				dev_err(engine->dev, "failed to unprepare request\n");
 		}
 		spin_lock_irqsave(&engine->queue_lock, flags);
 		engine->cur_req = NULL;
@@ -312,7 +314,7 @@ void crypto_finalize_hash_request(struct crypto_engine *engine,
 		    engine->unprepare_hash_request) {
 			ret = engine->unprepare_hash_request(engine, req);
 			if (ret)
-				pr_err("failed to unprepare request\n");
+				dev_err(engine->dev, "failed to unprepare request\n");
 		}
 		spin_lock_irqsave(&engine->queue_lock, flags);
 		engine->cur_req = NULL;
@@ -384,7 +386,7 @@ int crypto_engine_stop(struct crypto_engine *engine)
 	spin_unlock_irqrestore(&engine->queue_lock, flags);
 
 	if (ret)
-		pr_warn("could not stop engine\n");
+		dev_warn(engine->dev, "could not stop engine\n");
 
 	return ret;
 }
@@ -411,6 +413,7 @@ struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt)
 	if (!engine)
 		return NULL;
 
+	engine->dev = dev;
 	engine->rt = rt;
 	engine->running = false;
 	engine->busy = false;
diff --git a/include/crypto/engine.h b/include/crypto/engine.h
index 1bf600fc99f7..dd04c1699b51 100644
--- a/include/crypto/engine.h
+++ b/include/crypto/engine.h
@@ -58,6 +58,7 @@ struct crypto_engine {
 	struct list_head	list;
 	spinlock_t		queue_lock;
 	struct crypto_queue	queue;
+	struct device		*dev;
 
 	bool			rt;
 
-- 
2.13.0

Re: [PATCH v2 1/2] crypto: engine - replace pr_xxx by dev_xxx

[Herbert Xu]

On Tue, Jun 06, 2017 at 03:44:16PM +0200, Corentin Labbe wrote:
> By adding a struct device *dev to struct engine, we could store the
> device used at register time and so use all dev_xxx functions instead of
> pr_xxx.
> 
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

The crypto engine could actually only enqueue hash and ablkcipher request.
This patch permit it to enqueue skcipher requets by adding all necessary
functions.
The only problem is that ablkcipher and skcipher id are the same, so
only one cipher type is usable on the same crypto engine.

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

diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
index 61e7c4e02fd2..fc0f58b6299c 100644
--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -36,6 +36,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 	struct crypto_async_request *async_req, *backlog;
 	struct ahash_request *hreq;
 	struct ablkcipher_request *breq;
+	struct skcipher_request *skreq;
 	unsigned long flags;
 	bool was_busy = false;
 	int ret, rtype;
@@ -123,17 +124,34 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 		}
 		return;
 	case CRYPTO_ALG_TYPE_ABLKCIPHER:
-		breq = ablkcipher_request_cast(engine->cur_req);
-		if (engine->prepare_cipher_request) {
-			ret = engine->prepare_cipher_request(engine, breq);
-			if (ret) {
-				dev_err(engine->dev, "failed to prepare request: %d\n",
-					ret);
-				goto req_err;
+	/* since CRYPTO_ALG_TYPE_ABLKCIPHER == CRYPTO_ALG_TYPE_SKCIPHER
+	 * differenciate both with the presence of cipher_one_request
+	 */
+		if (engine->cipher_one_request) {
+			breq = ablkcipher_request_cast(engine->cur_req);
+			if (engine->prepare_cipher_request) {
+				ret = engine->prepare_cipher_request(engine, breq);
+				if (ret) {
+					dev_err(engine->dev, "failed to prepare request: %d\n",
+						ret);
+					goto req_err;
+				}
+				engine->cur_req_prepared = true;
 			}
-			engine->cur_req_prepared = true;
+			ret = engine->cipher_one_request(engine, breq);
+		} else {
+			skreq = skcipher_request_cast(engine->cur_req);
+			if (engine->prepare_skcipher_request) {
+				ret = engine->prepare_skcipher_request(engine, skreq);
+				if (ret) {
+					dev_err(engine->dev, "failed to prepare request: %d\n",
+						ret);
+					goto req_err;
+				}
+				engine->cur_req_prepared = true;
+		}
+		ret = engine->skcipher_one_request(engine, skreq);
 		}
-		ret = engine->cipher_one_request(engine, breq);
 		if (ret) {
 			dev_err(engine->dev, "failed to cipher one request from queue\n");
 			goto req_err;
@@ -151,8 +169,13 @@ static void crypto_pump_requests(struct crypto_engine *engine,
 		crypto_finalize_hash_request(engine, hreq, ret);
 		break;
 	case CRYPTO_ALG_TYPE_ABLKCIPHER:
-		breq = ablkcipher_request_cast(engine->cur_req);
-		crypto_finalize_cipher_request(engine, breq, ret);
+		if (engine->cipher_one_request) {
+			breq = ablkcipher_request_cast(engine->cur_req);
+			crypto_finalize_cipher_request(engine, breq, ret);
+		} else {
+			skreq = skcipher_request_cast(engine->cur_req);
+			crypto_finalize_skcipher_request(engine, skreq, ret);
+		}
 		break;
 	}
 	return;
@@ -213,6 +236,49 @@ int crypto_transfer_cipher_request_to_engine(struct crypto_engine *engine,
 EXPORT_SYMBOL_GPL(crypto_transfer_cipher_request_to_engine);
 
 /**
+ * crypto_transfer_skcipher_request - transfer the new request into the
+ * enginequeue
+ * @engine: the hardware engine
+ * @req: the request need to be listed into the engine queue
+ */
+int crypto_transfer_skcipher_request(struct crypto_engine *engine,
+				     struct skcipher_request *req,
+				     bool need_pump)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&engine->queue_lock, flags);
+
+	if (!engine->running) {
+		spin_unlock_irqrestore(&engine->queue_lock, flags);
+		return -ESHUTDOWN;
+	}
+
+	ret = crypto_enqueue_request(&engine->queue, &req->base);
+
+	if (!engine->busy && need_pump)
+		kthread_queue_work(engine->kworker, &engine->pump_requests);
+
+	spin_unlock_irqrestore(&engine->queue_lock, flags);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(crypto_transfer_skcipher_request);
+
+/**
+ * crypto_transfer_skcipher_request_to_engine - transfer one request to list
+ * into the engine queue
+ * @engine: the hardware engine
+ * @req: the request need to be listed into the engine queue
+ */
+int crypto_transfer_skcipher_request_to_engine(struct crypto_engine *engine,
+					       struct skcipher_request *req)
+{
+	return crypto_transfer_skcipher_request(engine, req, true);
+}
+EXPORT_SYMBOL_GPL(crypto_transfer_skcipher_request_to_engine);
+
+/**
  * crypto_transfer_hash_request - transfer the new request into the
  * enginequeue
  * @engine: the hardware engine
@@ -292,6 +358,43 @@ void crypto_finalize_cipher_request(struct crypto_engine *engine,
 EXPORT_SYMBOL_GPL(crypto_finalize_cipher_request);
 
 /**
+ * crypto_finalize_skcipher_request - finalize one request if the request is done
+ * @engine: the hardware engine
+ * @req: the request need to be finalized
+ * @err: error number
+ */
+void crypto_finalize_skcipher_request(struct crypto_engine *engine,
+				      struct skcipher_request *req, int err)
+{
+	unsigned long flags;
+	bool finalize_cur_req = false;
+	int ret;
+
+	spin_lock_irqsave(&engine->queue_lock, flags);
+	if (engine->cur_req == &req->base)
+		finalize_cur_req = true;
+	spin_unlock_irqrestore(&engine->queue_lock, flags);
+
+	if (finalize_cur_req) {
+		if (engine->cur_req_prepared &&
+		    engine->unprepare_skcipher_request) {
+			ret = engine->unprepare_skcipher_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->base.complete(&req->base, err);
+
+	kthread_queue_work(engine->kworker, &engine->pump_requests);
+}
+EXPORT_SYMBOL_GPL(crypto_finalize_skcipher_request);
+
+/**
  * crypto_finalize_hash_request - finalize one request if the request is done
  * @engine: the hardware engine
  * @req: the request need to be finalized
@@ -345,6 +448,19 @@ int crypto_engine_start(struct crypto_engine *engine)
 		return -EBUSY;
 	}
 
+	if (!engine->skcipher_one_request && !engine->cipher_one_request &&
+	    !engine->hash_one_request) {
+		spin_unlock_irqrestore(&engine->queue_lock, flags);
+		dev_err(engine->dev, "need at least one request type\n");
+		return -EINVAL;
+	}
+
+	if (engine->skcipher_one_request && engine->cipher_one_request) {
+		spin_unlock_irqrestore(&engine->queue_lock, flags);
+		dev_err(engine->dev, "Cannot use both skcipher and ablkcipher\n");
+		return -EINVAL;
+	}
+
 	engine->running = true;
 	spin_unlock_irqrestore(&engine->queue_lock, flags);
 
diff --git a/include/crypto/engine.h b/include/crypto/engine.h
index dd04c1699b51..a8f6e6ed377b 100644
--- a/include/crypto/engine.h
+++ b/include/crypto/engine.h
@@ -18,6 +18,7 @@
 #include <linux/kthread.h>
 #include <crypto/algapi.h>
 #include <crypto/hash.h>
+#include <crypto/skcipher.h>
 
 #define ENGINE_NAME_LEN	30
 /*
@@ -69,12 +70,18 @@ struct crypto_engine {
 				      struct ablkcipher_request *req);
 	int (*unprepare_cipher_request)(struct crypto_engine *engine,
 					struct ablkcipher_request *req);
+	int (*prepare_skcipher_request)(struct crypto_engine *engine,
+					struct skcipher_request *req);
+	int (*unprepare_skcipher_request)(struct crypto_engine *engine,
+					  struct skcipher_request *req);
 	int (*prepare_hash_request)(struct crypto_engine *engine,
 				    struct ahash_request *req);
 	int (*unprepare_hash_request)(struct crypto_engine *engine,
 				      struct ahash_request *req);
 	int (*cipher_one_request)(struct crypto_engine *engine,
 				  struct ablkcipher_request *req);
+	int (*skcipher_one_request)(struct crypto_engine *engine,
+				    struct skcipher_request *req);
 	int (*hash_one_request)(struct crypto_engine *engine,
 				struct ahash_request *req);
 
@@ -90,12 +97,19 @@ int crypto_transfer_cipher_request(struct crypto_engine *engine,
 				   bool need_pump);
 int crypto_transfer_cipher_request_to_engine(struct crypto_engine *engine,
 					     struct ablkcipher_request *req);
+int crypto_transfer_skcipher_request(struct crypto_engine *engine,
+				     struct skcipher_request *req,
+				     bool need_pump);
+int crypto_transfer_skcipher_request_to_engine(struct crypto_engine *engine,
+					       struct skcipher_request *req);
 int crypto_transfer_hash_request(struct crypto_engine *engine,
 				 struct ahash_request *req, bool need_pump);
 int crypto_transfer_hash_request_to_engine(struct crypto_engine *engine,
 					   struct ahash_request *req);
 void crypto_finalize_cipher_request(struct crypto_engine *engine,
 				    struct ablkcipher_request *req, int err);
+void crypto_finalize_skcipher_request(struct crypto_engine *engine,
+				      struct skcipher_request *req, int err);
 void crypto_finalize_hash_request(struct crypto_engine *engine,
 				  struct ahash_request *req, int err);
 int crypto_engine_start(struct crypto_engine *engine);
-- 
2.13.0

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Stephan Müller]

Am Dienstag, 6. Juni 2017, 15:44:17 CEST schrieb Corentin Labbe:

Hi Corentin,

> The crypto engine could actually only enqueue hash and ablkcipher request.
> This patch permit it to enqueue skcipher requets by adding all necessary
> functions.
> The only problem is that ablkcipher and skcipher id are the same, so
> only one cipher type is usable on the same crypto engine.

Why do you think this is needed? I thought that skcipher is the successor to 
ablkcipher AND blkcipher. I.e. ablkcipher and blkcipher should be phased out. 
Thus, I would infer that any ablkcipher support should or could be dropped.

Ciao
Stephan

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Tue, Jun 06, 2017 at 04:07:25PM +0200, Stephan Müller wrote:
> Am Dienstag, 6. Juni 2017, 15:44:17 CEST schrieb Corentin Labbe:
> 
> Hi Corentin,
> 
> > The crypto engine could actually only enqueue hash and ablkcipher request.
> > This patch permit it to enqueue skcipher requets by adding all necessary
> > functions.
> > The only problem is that ablkcipher and skcipher id are the same, so
> > only one cipher type is usable on the same crypto engine.
> 
> Why do you think this is needed? I thought that skcipher is the successor to 
> ablkcipher AND blkcipher. I.e. ablkcipher and blkcipher should be phased out. 
> Thus, I would infer that any ablkcipher support should or could be dropped.
> 

There are two user of it, crypto/omap and crypto/virtio.
And for crypto/omap, I have no way of testing any convertion to skcipher (no hardware).

Regards
Corentin Labbe

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Herbert Xu]

On Tue, Jun 06, 2017 at 03:44:17PM +0200, Corentin Labbe wrote:
> The crypto engine could actually only enqueue hash and ablkcipher request.
> This patch permit it to enqueue skcipher requets by adding all necessary
> functions.
> The only problem is that ablkcipher and skcipher id are the same, so
> only one cipher type is usable on the same crypto engine.
> 
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>

I think this should be done as part of the skcipher conversion rather
than as a standalone patch.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Mon, Jun 19, 2017 at 01:27:08PM +0800, Herbert Xu wrote:
> On Tue, Jun 06, 2017 at 03:44:17PM +0200, Corentin Labbe wrote:
> > The crypto engine could actually only enqueue hash and ablkcipher request.
> > This patch permit it to enqueue skcipher requets by adding all necessary
> > functions.
> > The only problem is that ablkcipher and skcipher id are the same, so
> > only one cipher type is usable on the same crypto engine.
> > 
> > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> 
> I think this should be done as part of the skcipher conversion rather
> than as a standalone patch.
> 

Since there are two different user of "crypto engine + ablkcipher", it will be not easy to convert them in one serie. (I could do it, but I simply could not test it for OMAP (lack of hw))
And any new user which want to use crypto engine+skcipher (like me with the sun8i-ce driver) are simply stuck.

Regards

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Herbert Xu]

On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
>
> Since there are two different user of "crypto engine + ablkcipher", it will be not easy to convert them in one serie. (I could do it, but I simply could not test it for OMAP (lack of hw))
> And any new user which want to use crypto engine+skcipher (like me with the sun8i-ce driver) are simply stuck.

You're right.  We'll need to do this in a backwards-compatible way.  In fact
we already do something similar in skcipher.c itself.  Simply look at the
cra_type field and if it matches blkcipher/ablkcipher/givcipher then it's
legacy ablkcipher, otherwise it's skcipher.

Also the way crypto_engine looks at the request type in the data-path is
suboptimal.  This should really be built into the cra_type object.  For
example, we can have cra_type->engine->prepare_request which would just
do the right thing.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Fri, Jun 23, 2017 at 02:48:37PM +0800, Herbert Xu wrote:
> On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
> >
> > Since there are two different user of "crypto engine + ablkcipher", it will be not easy to convert them in one serie. (I could do it, but I simply could not test it for OMAP (lack of hw))
> > And any new user which want to use crypto engine+skcipher (like me with the sun8i-ce driver) are simply stuck.
> 
> You're right.  We'll need to do this in a backwards-compatible way.  In fact
> we already do something similar in skcipher.c itself.  Simply look at the
> cra_type field and if it matches blkcipher/ablkcipher/givcipher then it's
> legacy ablkcipher, otherwise it's skcipher.
> 
> Also the way crypto_engine looks at the request type in the data-path is
> suboptimal.  This should really be built into the cra_type object.  For
> example, we can have cra_type->engine->prepare_request which would just
> do the right thing.
> 

Not sure to have well understand what you want.
You want that I switch on cra_type instead of crypto_tfm_alg_type() ?

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Herbert Xu]

On Fri, Jul 14, 2017 at 01:15:36PM +0200, Corentin Labbe wrote:
> On Fri, Jun 23, 2017 at 02:48:37PM +0800, Herbert Xu wrote:
> > On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
> > >
> > > Since there are two different user of "crypto engine + ablkcipher", it will be not easy to convert them in one serie. (I could do it, but I simply could not test it for OMAP (lack of hw))
> > > And any new user which want to use crypto engine+skcipher (like me with the sun8i-ce driver) are simply stuck.
> > 
> > You're right.  We'll need to do this in a backwards-compatible way.  In fact
> > we already do something similar in skcipher.c itself.  Simply look at the
> > cra_type field and if it matches blkcipher/ablkcipher/givcipher then it's
> > legacy ablkcipher, otherwise it's skcipher.
> > 
> > Also the way crypto_engine looks at the request type in the data-path is
> > suboptimal.  This should really be built into the cra_type object.  For
> > example, we can have cra_type->engine->prepare_request which would just
> > do the right thing.
> > 
> 
> Not sure to have well understand what you want.
> You want that I switch on cra_type instead of crypto_tfm_alg_type() ?

No I mean that we should have an engine hooks object registered
under cra_type so that you simply call

	cra_type->engine->prepare_request()

regardless of what type you're using.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Fri, Jul 28, 2017 at 09:52:57PM +0800, Herbert Xu wrote:
> On Fri, Jul 14, 2017 at 01:15:36PM +0200, Corentin Labbe wrote:
> > On Fri, Jun 23, 2017 at 02:48:37PM +0800, Herbert Xu wrote:
> > > On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
> > > >
> > > > Since there are two different user of "crypto engine + ablkcipher", it will be not easy to convert them in one serie. (I could do it, but I simply could not test it for OMAP (lack of hw))
> > > > And any new user which want to use crypto engine+skcipher (like me with the sun8i-ce driver) are simply stuck.
> > > 
> > > You're right.  We'll need to do this in a backwards-compatible way.  In fact
> > > we already do something similar in skcipher.c itself.  Simply look at the
> > > cra_type field and if it matches blkcipher/ablkcipher/givcipher then it's
> > > legacy ablkcipher, otherwise it's skcipher.
> > > 
> > > Also the way crypto_engine looks at the request type in the data-path is
> > > suboptimal.  This should really be built into the cra_type object.  For
> > > example, we can have cra_type->engine->prepare_request which would just
> > > do the right thing.
> > > 
> > 
> > Not sure to have well understand what you want.
> > You want that I switch on cra_type instead of crypto_tfm_alg_type() ?
> 
> No I mean that we should have an engine hooks object registered
> under cra_type so that you simply call
> 
> 	cra_type->engine->prepare_request()
> 
> regardless of what type you're using.
> 

I am sorry, I didnt see how to do that.

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Corentin Labbe]

On Fri, Jul 28, 2017 at 05:01:19PM +0200, Corentin Labbe wrote:
> On Fri, Jul 28, 2017 at 09:52:57PM +0800, Herbert Xu wrote:
> > On Fri, Jul 14, 2017 at 01:15:36PM +0200, Corentin Labbe wrote:
> > > On Fri, Jun 23, 2017 at 02:48:37PM +0800, Herbert Xu wrote:
> > > > On Mon, Jun 19, 2017 at 09:55:24AM +0200, Corentin Labbe wrote:
> > > > >
> > > > > Since there are two different user of "crypto engine + ablkcipher", it will be not easy to convert them in one serie. (I could do it, but I simply could not test it for OMAP (lack of hw))
> > > > > And any new user which want to use crypto engine+skcipher (like me with the sun8i-ce driver) are simply stuck.
> > > > 
> > > > You're right.  We'll need to do this in a backwards-compatible way.  In fact
> > > > we already do something similar in skcipher.c itself.  Simply look at the
> > > > cra_type field and if it matches blkcipher/ablkcipher/givcipher then it's
> > > > legacy ablkcipher, otherwise it's skcipher.
> > > > 
> > > > Also the way crypto_engine looks at the request type in the data-path is
> > > > suboptimal.  This should really be built into the cra_type object.  For
> > > > example, we can have cra_type->engine->prepare_request which would just
> > > > do the right thing.
> > > > 
> > > 
> > > Not sure to have well understand what you want.
> > > You want that I switch on cra_type instead of crypto_tfm_alg_type() ?
> > 
> > No I mean that we should have an engine hooks object registered
> > under cra_type so that you simply call
> > 
> > 	cra_type->engine->prepare_request()
> > 
> > regardless of what type you're using.
> > 
> 
> I am sorry, I didnt see how to do that.

Hello

I really didnt see how to do that since cra_type is const.
Anyway, I think it cannot be possible since we could have two different engine with two different prepare_request().

I will really appreciate any advice on what you want exactly.

Regards

Re: [PATCH v2 2/2] crypto: engine - Permit to enqueue skcipher request

[Herbert Xu]

On Wed, Aug 09, 2017 at 11:40:12AM +0200, Corentin Labbe wrote:
>
> I really didnt see how to do that since cra_type is const.
> Anyway, I think it cannot be possible since we could have two different engine with two different prepare_request().
> 
> I will really appreciate any advice on what you want exactly.

The issue is that we have these handlers such as prepare_request
that gives us a level of indirection into the driver.  However,
on top of those we're adding another level of indirection based
on the type of request.

I'd like to see these two combined into just a single function
pointer.

My idea of using cra_type is indeed broken as these functions
ultimately live in the driver and not crypto API.

So how about something like this:

struct crypto_engine_op {
	int (*prepare_request)(struct crypto_engine *engine,
			       struct crypto_async_request *req);
	int (*unprepare_request)(struct crypto_engine *engine,
				 struct crypto_async_request *req);
	int (*do_one_request)(struct crypto_engine *engine,
			      struct crypto_async_request *req);
};

struct crypto_engine_reqctx {
	struct crypto_engine_op *op;
};	

struct crypto_engine {
	struct crypto_engine_op aead;
	struct crypto_engine_op hash;
	struct crypto_engine_op cipher;

	...
};

Then in the request_to_engine functions you would store the right
op pointer inside the request context area.  Obviously the users
of crypto_engine would need to allocate space for the struct
crypto_engine_reqctx in their reqctx structure.

The only wart with this scheme is that the drivers end up seeing
struct crypto_async_request and will need to convert that to the
respective request types but I couldn't really find a better way.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt