Re: [PATCH 5/9] crypto: engine: add enqueue_request/can_do_more

From: Corentin Labbe <clabbe.montjoie@gmail.com>
To: Iuliana Prodan <iuliana.prodan@nxp.com>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
	"herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
	"mripard@kernel.org" <mripard@kernel.org>,
	"wens@csie.org" <wens@csie.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-sunxi@googlegroups.com" <linux-sunxi@googlegroups.com>
Subject: Re: [PATCH 5/9] crypto: engine: add enqueue_request/can_do_more
Date: Tue, 28 Jan 2020 09:40:41 +0100	[thread overview]
Message-ID: <20200128084041.GA10493@Red> (raw)
In-Reply-To: <AM0PR04MB717155300E3575C07D31E1D08C0B0@AM0PR04MB7171.eurprd04.prod.outlook.com>

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@gmail.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/