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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> [2]
> https://lore.kernel.org/lkml/20200122144134.axqpwx65j7xysyy3@xxxxxxxxxxxxxxxxxxx/
