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:   Horia Geanta <horia.geanta@nxp.com>
To:     Corentin Labbe <clabbe.montjoie@gmail.com>,
        "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>,
        Iuliana Prodan <iuliana.prodan@nxp.com>
CC:     "linux-sunxi@googlegroups.com" <linux-sunxi@googlegroups.com>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/9] crypto: engine: workqueue can only be processed one
 by one
Thread-Topic: [PATCH 1/9] crypto: engine: workqueue can only be processed one
 by one
Thread-Index: AQHV0RErU4XkhL83r0qysobyMlLYdA==
Date:   Tue, 28 Jan 2020 15:50:14 +0000
Message-ID: <VI1PR0402MB3485B787EA6BCDD5A5600BAA980A0@VI1PR0402MB3485.eurprd04.prod.outlook.com>
References: <20200122104528.30084-1-clabbe.montjoie@gmail.com>
 <20200122104528.30084-2-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: f11a6f60-1b53-48d3-4620-08d7a409c41d
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Jan 2020 15:50:14.6441
 (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: +kNzlJIgiV16h0AQ1A94J101UrsS34r1J0/QYd5oOQ47SUGR89DSHG7vRNFc5hgL4I+GIhzAIqLwu2coHHW0/A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB3758
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/22/2020 12:46 PM, Corentin Labbe wrote:=0A=
> Some bykeshedding are unnecessary since a workqueue can only be executed=
=0A=
> one by one.=0A=
> This behaviour is documented in:=0A=
> - kernel/kthread.c: comment of kthread_worker_fn()=0A=
> - Documentation/core-api/workqueue.rst: the functions associated with the=
 work items one after the other=0A=
[...]=0A=
> @@ -73,16 +73,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=
This check is here for a good reason, namely because crypto engine=0A=
cannot currently handle multiple crypto requests being in "flight"=0A=
in parallel.=0A=
=0A=
More exactly, if this check is removed the following sequence could occur:=
=0A=
crypto_pump_work() -> crypto_pump_requests() -> .do_one_request(areq1)=0A=
crypto_pump_work() -> crypto_pump_requests() -> .do_one_request(areq2)=0A=
crypto_finalize_request(areq1)=0A=
crypto_finalize_request(areq2)=0A=
=0A=
This isn't correctly handled in crypto_finalize_request(),=0A=
since .unprepare_request will be called only for areq2.=0A=
=0A=
/**=0A=
 * crypto_finalize_request - finalize one request if the request is done=0A=
 * @engine: the hardware engine=0A=
 * @req: the request need to be finalized=0A=
 * @err: error number=0A=
 */=0A=
static void crypto_finalize_request(struct crypto_engine *engine,=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=
	}=0A=
=0A=
	req->complete(req, err);=0A=
=0A=
	kthread_queue_work(engine->kworker, &engine->pump_requests);=0A=
}=0A=
=0A=
Horia=0A=