Re: [PATCH RFC 06/10] crypto: engine: introduce ct

[Iuliana Prodan]

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@xxxxxxxxx>
> ---
>   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;
>   };
>   
>   /*
>