| Message ID | 20200114135936.32422-7-clabbe.montjoie@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | crypto: engine: permit to batch requests | expand |
On 1/14/2020 4:00 PM, Corentin Labbe wrote: > We will store the number of request in a batch in engine->ct. > This patch adds all loop to unprepare all requests of a batch. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > crypto/crypto_engine.c | 30 ++++++++++++++++++------------ > include/crypto/engine.h | 2 ++ > 2 files changed, 20 insertions(+), 12 deletions(-) > > diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c > index b72873550587..591dea5ddeec 100644 > --- a/crypto/crypto_engine.c > +++ b/crypto/crypto_engine.c > @@ -28,6 +28,7 @@ static void crypto_finalize_request(struct crypto_engine *engine, > bool finalize_cur_req = false; > int ret; > struct crypto_engine_ctx *enginectx; > + int i = 0; > > spin_lock_irqsave(&engine->queue_lock, flags); > if (engine->cur_reqs[0].req == req) You're checking here just the first request, but do the completion for all? Why? Shouldn't we check for each request if it was done by hw or not? I've also seen that the do_one_request is called only on the first request, from the batch. In your driver you do the prepare/unprepare for the whole batch at once, but not all drivers, who uses crypto-engine, are doing this (see virtio, amlogic, stm32). And I don't know if they can... > @@ -35,17 +36,20 @@ static void crypto_finalize_request(struct crypto_engine *engine, > spin_unlock_irqrestore(&engine->queue_lock, flags); > > if (finalize_cur_req) { > - enginectx = crypto_tfm_ctx(engine->cur_reqs[0].req->tfm); > - if (engine->cur_reqs[0].prepared && > - enginectx->op.unprepare_request) { > - ret = enginectx->op.unprepare_request(engine, engine->cur_reqs[0].req); > - if (ret) > - dev_err(engine->dev, "failed to unprepare request\n"); > - } > - engine->cur_reqs[0].req->complete(engine->cur_reqs[0].req, err); > + do { > + enginectx = crypto_tfm_ctx(engine->cur_reqs[i].req->tfm); > + if (engine->cur_reqs[i].prepared && > + enginectx->op.unprepare_request) { > + ret = enginectx->op.unprepare_request(engine, engine->cur_reqs[i].req); > + if (ret) > + dev_err(engine->dev, "failed to unprepare request\n"); > + } > + engine->cur_reqs[i].prepared = false; > + engine->cur_reqs[i].req->complete(engine->cur_reqs[i].req, err); > + } while (++i < engine->ct); > spin_lock_irqsave(&engine->queue_lock, flags); > - engine->cur_reqs[0].prepared = false; > - engine->cur_reqs[0].req = NULL; > + while (engine->ct-- > 0) > + engine->cur_reqs[engine->ct].req = NULL; > spin_unlock_irqrestore(&engine->queue_lock, flags); > } else { > req->complete(req, err); > @@ -109,13 +113,14 @@ static void crypto_pump_requests(struct crypto_engine *engine, > goto out; > } > > + engine->ct = 0; > /* Get the fist request from the engine queue to handle */ > backlog = crypto_get_backlog(&engine->queue); > async_req = crypto_dequeue_request(&engine->queue); > if (!async_req) > goto out; > > - engine->cur_reqs[0].req = async_req; > + engine->cur_reqs[engine->ct].req = async_req; > if (backlog) > backlog->complete(backlog, -EINPROGRESS); > > @@ -144,8 +149,9 @@ static void crypto_pump_requests(struct crypto_engine *engine, > ret); > goto req_err; > } > - engine->cur_reqs[0].prepared = true; > + engine->cur_reqs[engine->ct].prepared = true; > } > + engine->ct++; > if (!enginectx->op.do_one_request) { > dev_err(engine->dev, "failed to do request\n"); > ret = -EINVAL; > diff --git a/include/crypto/engine.h b/include/crypto/engine.h > index 362134e226f4..021136a47b93 100644 > --- a/include/crypto/engine.h > +++ b/include/crypto/engine.h > @@ -50,6 +50,7 @@ struct cur_req { > * @priv_data: the engine private data > * @rmax: The number of request which can be processed in one batch > * @cur_reqs: A list for requests to be processed > + * @ct: How many requests will be handled in one batch > */ > struct crypto_engine { > char name[ENGINE_NAME_LEN]; > @@ -73,6 +74,7 @@ struct crypto_engine { > void *priv_data; > int rmax; > struct cur_req *cur_reqs; > + int ct; > }; > > /* >
On Thu, Jan 16, 2020 at 11:34:19AM +0000, Iuliana Prodan wrote: > On 1/14/2020 4:00 PM, Corentin Labbe wrote: > > We will store the number of request in a batch in engine->ct. > > This patch adds all loop to unprepare all requests of a batch. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > crypto/crypto_engine.c | 30 ++++++++++++++++++------------ > > include/crypto/engine.h | 2 ++ > > 2 files changed, 20 insertions(+), 12 deletions(-) > > > > diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c > > index b72873550587..591dea5ddeec 100644 > > --- a/crypto/crypto_engine.c > > +++ b/crypto/crypto_engine.c > > @@ -28,6 +28,7 @@ static void crypto_finalize_request(struct crypto_engine *engine, > > bool finalize_cur_req = false; > > int ret; > > struct crypto_engine_ctx *enginectx; > > + int i = 0; > > > > spin_lock_irqsave(&engine->queue_lock, flags); > > if (engine->cur_reqs[0].req == req) > You're checking here just the first request, but do the completion for > all? Why? Shouldn't we check for each request if it was done by hw or not? The first request is a sort of key for the whole batch. > > I've also seen that the do_one_request is called only on the first > request, from the batch. Since the request are linked, this is not a problem. But I miss this explanaition in the code. > > In your driver you do the prepare/unprepare for the whole batch at once, > but not all drivers, who uses crypto-engine, are doing this (see virtio, > amlogic, stm32). And I don't know if they can... prepare is optionnal, and unprepare is optional even if prepare is done. Furthermore, doing prepare/unprepare is optional per request. I have tested this serie on sun8i-ss and amlogic which dont use prepare/unprepare.
diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c index b72873550587..591dea5ddeec 100644 --- a/crypto/crypto_engine.c +++ b/crypto/crypto_engine.c @@ -28,6 +28,7 @@ static void crypto_finalize_request(struct crypto_engine *engine, bool finalize_cur_req = false; int ret; struct crypto_engine_ctx *enginectx; + int i = 0; spin_lock_irqsave(&engine->queue_lock, flags); if (engine->cur_reqs[0].req == req) @@ -35,17 +36,20 @@ static void crypto_finalize_request(struct crypto_engine *engine, spin_unlock_irqrestore(&engine->queue_lock, flags); if (finalize_cur_req) { - enginectx = crypto_tfm_ctx(engine->cur_reqs[0].req->tfm); - if (engine->cur_reqs[0].prepared && - enginectx->op.unprepare_request) { - ret = enginectx->op.unprepare_request(engine, engine->cur_reqs[0].req); - if (ret) - dev_err(engine->dev, "failed to unprepare request\n"); - } - engine->cur_reqs[0].req->complete(engine->cur_reqs[0].req, err); + do { + enginectx = crypto_tfm_ctx(engine->cur_reqs[i].req->tfm); + if (engine->cur_reqs[i].prepared && + enginectx->op.unprepare_request) { + ret = enginectx->op.unprepare_request(engine, engine->cur_reqs[i].req); + if (ret) + dev_err(engine->dev, "failed to unprepare request\n"); + } + engine->cur_reqs[i].prepared = false; + engine->cur_reqs[i].req->complete(engine->cur_reqs[i].req, err); + } while (++i < engine->ct); spin_lock_irqsave(&engine->queue_lock, flags); - engine->cur_reqs[0].prepared = false; - engine->cur_reqs[0].req = NULL; + while (engine->ct-- > 0) + engine->cur_reqs[engine->ct].req = NULL; spin_unlock_irqrestore(&engine->queue_lock, flags); } else { req->complete(req, err); @@ -109,13 +113,14 @@ static void crypto_pump_requests(struct crypto_engine *engine, goto out; } + engine->ct = 0; /* Get the fist request from the engine queue to handle */ backlog = crypto_get_backlog(&engine->queue); async_req = crypto_dequeue_request(&engine->queue); if (!async_req) goto out; - engine->cur_reqs[0].req = async_req; + engine->cur_reqs[engine->ct].req = async_req; if (backlog) backlog->complete(backlog, -EINPROGRESS); @@ -144,8 +149,9 @@ static void crypto_pump_requests(struct crypto_engine *engine, ret); goto req_err; } - engine->cur_reqs[0].prepared = true; + engine->cur_reqs[engine->ct].prepared = true; } + engine->ct++; if (!enginectx->op.do_one_request) { dev_err(engine->dev, "failed to do request\n"); ret = -EINVAL; diff --git a/include/crypto/engine.h b/include/crypto/engine.h index 362134e226f4..021136a47b93 100644 --- a/include/crypto/engine.h +++ b/include/crypto/engine.h @@ -50,6 +50,7 @@ struct cur_req { * @priv_data: the engine private data * @rmax: The number of request which can be processed in one batch * @cur_reqs: A list for requests to be processed + * @ct: How many requests will be handled in one batch */ struct crypto_engine { char name[ENGINE_NAME_LEN]; @@ -73,6 +74,7 @@ struct crypto_engine { void *priv_data; int rmax; struct cur_req *cur_reqs; + int ct; }; /*
We will store the number of request in a batch in engine->ct. This patch adds all loop to unprepare all requests of a batch. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- crypto/crypto_engine.c | 30 ++++++++++++++++++------------ include/crypto/engine.h | 2 ++ 2 files changed, 20 insertions(+), 12 deletions(-)