Date:   Tue, 28 Jan 2020 09:40:41 +0100
From:   Corentin Labbe <clabbe.montjoie@...il.com>
To:     Iuliana Prodan <iuliana.prodan@....com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        "mripard@...nel.org" <mripard@...nel.org>,
        "wens@...e.org" <wens@...e.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-sunxi@...glegroups.com" <linux-sunxi@...glegroups.com>
Subject: Re: [PATCH 5/9] crypto: engine: add enqueue_request/can_do_more

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@...il.com>
> > ---
> >   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@VI1PR04MB4445.eurprd04.prod.outlook.com/
> [2] 
> https://lore.kernel.org/lkml/20200122144134.axqpwx65j7xysyy3@gondor.apana.org.au/

Powered by blists - more mailing lists