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>
CC:     Herbert Xu <herbert@gondor.apana.org.au>,
        Baolin Wang <baolin.wang@linaro.org>,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        Horia Geanta <horia.geanta@nxp.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Alexandre Torgue <alexandre.torgue@st.com>,
        Maxime Ripard <mripard@kernel.org>,
        Aymen Sghaier <aymen.sghaier@nxp.com>,
        "David S. Miller" <davem@davemloft.net>,
        Silvano Di Ninno <silvano.dininno@nxp.com>,
        Franck Lenormand <franck.lenormand@nxp.com>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        dl-linux-imx <linux-imx@nxp.com>
Subject: Re: [PATCH v2 1/2] crypto: engine - support for parallel requests
Thread-Topic: [PATCH v2 1/2] crypto: engine - support for parallel requests
Thread-Index: AQHV21d9ovH+nULTjUSN5UBY10erRw==
Date:   Fri, 7 Feb 2020 11:26:38 +0000
Message-ID: <AM0PR04MB71711FEFD0738893772BE3198C1C0@AM0PR04MB7171.eurprd04.prod.outlook.com>
References: <1580819660-30211-1-git-send-email-iuliana.prodan@nxp.com>
 <1580819660-30211-2-git-send-email-iuliana.prodan@nxp.com>
 <20200205191128.GA32606@Red>
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: fda9f352-b5a6-40be-ef3b-08d7abc09953
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Feb 2020 11:26:38.9783
 (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: RLlB6TidNyd43O+dAL0kgKsUw3kILkoNLegGYoRYH5uR9+X1txZqZg/Pm8i5+lDybNPVfZfo3mE0aGgef0smug==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB4964
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 2/5/2020 9:11 PM, Corentin Labbe wrote:=0A=
> On Tue, Feb 04, 2020 at 02:34:19PM +0200, Iuliana Prodan wrote:=0A=
>> Added support for executing multiple requests, in parallel,=0A=
>> for crypto engine.=0A=
>> A new callback is added, can_enqueue_more, which asks the=0A=
>> driver if the hardware has free space, to enqueue a new request.=0A=
>> The new crypto_engine_alloc_init_and_set function, initialize=0A=
>> crypto-engine, sets the maximum size for crypto-engine software=0A=
>> queue (not hardcoded anymore) and the can_enqueue_more callback.=0A=
>> On crypto_pump_requests, if can_enqueue_more callback returns true,=0A=
>> a new request is send to hardware, until there is no space and the=0A=
>> callback returns false.=0A=
>>=0A=
>> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>=0A=
>> ---=0A=
>>   crypto/crypto_engine.c  | 106 ++++++++++++++++++++++++++++++----------=
--------=0A=
>>   include/crypto/engine.h |  10 +++--=0A=
>>   2 files changed, 72 insertions(+), 44 deletions(-)=0A=
>>=0A=
>> diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c=0A=
>> index eb029ff..aba934f 100644=0A=
>> --- a/crypto/crypto_engine.c=0A=
>> +++ b/crypto/crypto_engine.c=0A=
>> @@ -22,32 +22,18 @@=0A=
>>    * @err: error number=0A=
>>    */=0A=
>>   static void crypto_finalize_request(struct crypto_engine *engine,=0A=
>> -			     struct crypto_async_request *req, int err)=0A=
>> +				    struct crypto_async_request *req, int err)=0A=
>>   {=0A=
>> -	unsigned long flags;=0A=
>> -	bool finalize_cur_req =3D false;=0A=
>>   	int ret;=0A=
>>   	struct crypto_engine_ctx *enginectx;=0A=
>>   =0A=
>> -	spin_lock_irqsave(&engine->queue_lock, flags);=0A=
>> -	if (engine->cur_req =3D=3D req)=0A=
>> -		finalize_cur_req =3D true;=0A=
>> -	spin_unlock_irqrestore(&engine->queue_lock, flags);=0A=
>> -=0A=
>> -	if (finalize_cur_req) {=0A=
>> -		enginectx =3D crypto_tfm_ctx(req->tfm);=0A=
>> -		if (engine->cur_req_prepared &&=0A=
>> -		    enginectx->op.unprepare_request) {=0A=
>> -			ret =3D enginectx->op.unprepare_request(engine, req);=0A=
>> -			if (ret)=0A=
>> -				dev_err(engine->dev, "failed to unprepare request\n");=0A=
>> -		}=0A=
>> -		spin_lock_irqsave(&engine->queue_lock, flags);=0A=
>> -		engine->cur_req =3D NULL;=0A=
>> -		engine->cur_req_prepared =3D false;=0A=
>> -		spin_unlock_irqrestore(&engine->queue_lock, flags);=0A=
>> +	enginectx =3D crypto_tfm_ctx(req->tfm);=0A=
>> +	if (enginectx->op.prepare_request &&=0A=
>> +	    enginectx->op.unprepare_request) {=0A=
>> +		ret =3D enginectx->op.unprepare_request(engine, req);=0A=
>> +		if (ret)=0A=
>> +			dev_err(engine->dev, "failed to unprepare request\n");=0A=
>>   	}=0A=
>> -=0A=
>>   	req->complete(req, err);=0A=
>>   =0A=
>>   	kthread_queue_work(engine->kworker, &engine->pump_requests);=0A=
>> @@ -73,10 +59,6 @@ static void crypto_pump_requests(struct crypto_engine=
 *engine,=0A=
>>   =0A=
>>   	spin_lock_irqsave(&engine->queue_lock, flags);=0A=
>>   =0A=
>> -	/* Make sure we are not already running a request */=0A=
>> -	if (engine->cur_req)=0A=
>> -		goto out;=0A=
>> -=0A=
> =0A=
> Hello=0A=
> =0A=
> Your patch has the same problem than mine reported by Horia.=0A=
> If a queue has more than one request, a first crypto_pump_requests() will=
 send a request and for drivers which do not block on do_one_request() cryp=
to_pump_requests() will end.=0A=
> Then another crypto_pump_requests() will fire sending a second request wh=
ile the driver does not support that.=0A=
=0A=
> So we need to replace engine->cur_req by another locking mechanism.=0A=
> Perhaps the cleaner is to add a "request count" (increased when do_one_re=
quest, decreased in crypto_finalize_request)=0A=
> I know that the early version have that and it was removed, but I do not =
see any better way.=0A=
> =0A=
=0A=
The "request count" I've change it to can_enqueue_more, so the hw can =0A=
"answer" if it can enqueue or not.=0A=
=0A=
I'll (re)add the cur_req in crypto-engine.=0A=
If the new callback, can_enqueue_more, is not implemented the crypto- =0A=
engine will work as before - will send requests to hardware, one-by-one, =
=0A=
on crypto_pump_requests, and complete it, on crypto_finalize_request, =0A=
and so on.=0A=
=0A=
Thanks,=0A=
Iulia=0A=
=0A=