Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Iuliana Prodan <iuliana.prodan@nxp.com>
To:     Corentin Labbe <clabbe.montjoie@gmail.com>,
        "alexandre.torgue@st.com" <alexandre.torgue@st.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
        "mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>,
        "mripard@kernel.org" <mripard@kernel.org>,
        "wens@csie.org" <wens@csie.org>,
        Horia Geanta <horia.geanta@nxp.com>,
        Aymen Sghaier <aymen.sghaier@nxp.com>
CC:     "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-stm32@st-md-mailman.stormreply.com" 
        <linux-stm32@st-md-mailman.stormreply.com>,
        "linux-sunxi@googlegroups.com" <linux-sunxi@googlegroups.com>
Subject: Re: [PATCH RFC 06/10] crypto: engine: introduce ct
Thread-Topic: [PATCH RFC 06/10] crypto: engine: introduce ct
Thread-Index: AQHVyuLpwfJViqB/TkiZAIjgROs28Q==
Date:   Thu, 16 Jan 2020 11:34:19 +0000
Message-ID: <VI1PR04MB44455F7F7830159B6ED336648C360@VI1PR04MB4445.eurprd04.prod.outlook.com>
References: <20200114135936.32422-1-clabbe.montjoie@gmail.com>
 <20200114135936.32422-7-clabbe.montjoie@gmail.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 951b7343-9be3-449e-05a6-08d79a7806e4
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Jan 2020 11:34:19.7385
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: XQ5imzjKGCbotMlIwqAkYw8oVvujyTv5ed/SwmjOwKcv5lA6Lbtvw60HNPot6i0Y1Ca8xZ4T18bfa6QrhhID+g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB7184
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On 1/14/2020 4:00 PM, Corentin Labbe wrote:=0A=
> We will store the number of request in a batch in engine->ct.=0A=
> This patch adds all loop to unprepare all requests of a batch.=0A=
> =0A=
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>=0A=
> ---=0A=
>   crypto/crypto_engine.c  | 30 ++++++++++++++++++------------=0A=
>   include/crypto/engine.h |  2 ++=0A=
>   2 files changed, 20 insertions(+), 12 deletions(-)=0A=
> =0A=
> diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c=0A=
> index b72873550587..591dea5ddeec 100644=0A=
> --- a/crypto/crypto_engine.c=0A=
> +++ b/crypto/crypto_engine.c=0A=
> @@ -28,6 +28,7 @@ static void crypto_finalize_request(struct crypto_engin=
e *engine,=0A=
>   	bool finalize_cur_req =3D false;=0A=
>   	int ret;=0A=
>   	struct crypto_engine_ctx *enginectx;=0A=
> +	int i =3D 0;=0A=
>   =0A=
>   	spin_lock_irqsave(&engine->queue_lock, flags);=0A=
>   	if (engine->cur_reqs[0].req =3D=3D req)=0A=
You're checking here just the first request, but do the completion for =0A=
all? Why? Shouldn't we check for each request if it was done by hw or not?=
=0A=
=0A=
I've also seen that the do_one_request is called only on the first =0A=
request, from the batch.=0A=
=0A=
In your driver you do the prepare/unprepare for the whole batch at once, =
=0A=
but not all drivers, who uses crypto-engine, are doing this (see virtio, =
=0A=
amlogic, stm32). And I don't know if they can...=0A=
=0A=
> @@ -35,17 +36,20 @@ static void crypto_finalize_request(struct crypto_eng=
ine *engine,=0A=
>   	spin_unlock_irqrestore(&engine->queue_lock, flags);=0A=
>   =0A=
>   	if (finalize_cur_req) {=0A=
> -		enginectx =3D crypto_tfm_ctx(engine->cur_reqs[0].req->tfm);=0A=
> -		if (engine->cur_reqs[0].prepared &&=0A=
> -		    enginectx->op.unprepare_request) {=0A=
> -			ret =3D enginectx->op.unprepare_request(engine, engine->cur_reqs[0].r=
eq);=0A=
> -			if (ret)=0A=
> -				dev_err(engine->dev, "failed to unprepare request\n");=0A=
> -		}=0A=
> -		engine->cur_reqs[0].req->complete(engine->cur_reqs[0].req, err);=0A=
> +		do {=0A=
> +			enginectx =3D crypto_tfm_ctx(engine->cur_reqs[i].req->tfm);=0A=
> +			if (engine->cur_reqs[i].prepared &&=0A=
> +			    enginectx->op.unprepare_request) {=0A=
> +				ret =3D enginectx->op.unprepare_request(engine, engine->cur_reqs[i].=
req);=0A=
> +				if (ret)=0A=
> +					dev_err(engine->dev, "failed to unprepare request\n");=0A=
> +			}=0A=
> +			engine->cur_reqs[i].prepared =3D false;=0A=
> +			engine->cur_reqs[i].req->complete(engine->cur_reqs[i].req, err);=0A=
> +		} while (++i < engine->ct);=0A=
>   		spin_lock_irqsave(&engine->queue_lock, flags);=0A=
> -		engine->cur_reqs[0].prepared =3D false;=0A=
> -		engine->cur_reqs[0].req =3D NULL;=0A=
> +		while (engine->ct-- > 0)=0A=
> +			engine->cur_reqs[engine->ct].req =3D NULL;=0A=
>   		spin_unlock_irqrestore(&engine->queue_lock, flags);=0A=
>   	} else {=0A=
>   		req->complete(req, err);=0A=
> @@ -109,13 +113,14 @@ static void crypto_pump_requests(struct crypto_engi=
ne *engine,=0A=
>   		goto out;=0A=
>   	}=0A=
>   =0A=
> +	engine->ct =3D 0;=0A=
>   	/* Get the fist request from the engine queue to handle */=0A=
>   	backlog =3D crypto_get_backlog(&engine->queue);=0A=
>   	async_req =3D crypto_dequeue_request(&engine->queue);=0A=
>   	if (!async_req)=0A=
>   		goto out;=0A=
>   =0A=
> -	engine->cur_reqs[0].req =3D async_req;=0A=
> +	engine->cur_reqs[engine->ct].req =3D async_req;=0A=
>   	if (backlog)=0A=
>   		backlog->complete(backlog, -EINPROGRESS);=0A=
>   =0A=
> @@ -144,8 +149,9 @@ static void crypto_pump_requests(struct crypto_engine=
 *engine,=0A=
>   				ret);=0A=
>   			goto req_err;=0A=
>   		}=0A=
> -		engine->cur_reqs[0].prepared =3D true;=0A=
> +		engine->cur_reqs[engine->ct].prepared =3D true;=0A=
>   	}=0A=
> +	engine->ct++;=0A=
>   	if (!enginectx->op.do_one_request) {=0A=
>   		dev_err(engine->dev, "failed to do request\n");=0A=
>   		ret =3D -EINVAL;=0A=
> diff --git a/include/crypto/engine.h b/include/crypto/engine.h=0A=
> index 362134e226f4..021136a47b93 100644=0A=
> --- a/include/crypto/engine.h=0A=
> +++ b/include/crypto/engine.h=0A=
> @@ -50,6 +50,7 @@ struct cur_req {=0A=
>    * @priv_data: the engine private data=0A=
>    * @rmax:	The number of request which can be processed in one batch=0A=
>    * @cur_reqs: 	A list for requests to be processed=0A=
> + * @ct:		How many requests will be handled in one batch=0A=
>    */=0A=
>   struct crypto_engine {=0A=
>   	char			name[ENGINE_NAME_LEN];=0A=
> @@ -73,6 +74,7 @@ struct crypto_engine {=0A=
>   	void				*priv_data;=0A=
>   	int 				rmax;=0A=
>   	struct cur_req 			*cur_reqs;=0A=
> +	int ct;=0A=
>   };=0A=
>   =0A=
>   /*=0A=
> =0A=
=0A=