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 <[email protected]>
---
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
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
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
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