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;
Date:   Tue, 28 Jan 2020 17:55:34 +0100
From:   Corentin Labbe <clabbe.montjoie@gmail.com>
To:     Horia Geanta <horia.geanta@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>,
        Iuliana Prodan <iuliana.prodan@nxp.com>,
        "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
Message-ID: <20200128165534.GA11610@Red>
References: <20200122104528.30084-1-clabbe.montjoie@gmail.com>
 <20200122104528.30084-2-clabbe.montjoie@gmail.com>
 <VI1PR0402MB3485B787EA6BCDD5A5600BAA980A0@VI1PR0402MB3485.eurprd04.prod.outlook.com>
 <20200128155800.GB17295@Red>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200128155800.GB17295@Red>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Tue, Jan 28, 2020 at 04:58:00PM +0100, Corentin Labbe wrote:
> On Tue, Jan 28, 2020 at 03:50:14PM +0000, Horia Geanta wrote:
> > On 1/22/2020 12:46 PM, Corentin Labbe wrote:
> > > Some bykeshedding are unnecessary since a workqueue can only be executed
> > > one by one.
> > > This behaviour is documented in:
> > > - kernel/kthread.c: comment of kthread_worker_fn()
> > > - Documentation/core-api/workqueue.rst: the functions associated with the work items one after the other
> > [...]
> > > @@ -73,16 +73,6 @@ static void crypto_pump_requests(struct crypto_engine *engine,
> > >  
> > >  	spin_lock_irqsave(&engine->queue_lock, flags);
> > >  
> > > -	/* Make sure we are not already running a request */
> > > -	if (engine->cur_req)
> > > -		goto out;
> > > -
> > This check is here for a good reason, namely because crypto engine
> > cannot currently handle multiple crypto requests being in "flight"
> > in parallel.
> > 
> > More exactly, if this check is removed the following sequence could occur:
> > crypto_pump_work() -> crypto_pump_requests() -> .do_one_request(areq1)
> > crypto_pump_work() -> crypto_pump_requests() -> .do_one_request(areq2)
> > crypto_finalize_request(areq1)
> > crypto_finalize_request(areq2)
> > 
> 
> As explained in the commitlog, crypto_pump_work() cannot be ran twice.
> 

Sorry, I have misunderstood and wrongly answered.

Right since some driver does not block on do_one_request(), crypto_pump_work() can be ran one after one and so launch two request.

So this patch is bad.

Regards