Re: [PATCH 5/9] crypto: engine: add enqueue_request/can_do_more

[Iuliana Prodan]

On 1/28/2020 10:40 AM, Corentin Labbe wrote:
> On Mon, Jan 27, 2020 at 10:58:36PM +0000, Iuliana Prodan wrote:
>> On 1/22/2020 12:45 PM, Corentin Labbe wrote:
>>> This patchs adds two new function wrapper in crypto_engine.
>>> - enqueue_request() for drivers enqueuing request to hardware.
>>> - can_queue_more() for letting drivers to tell if they can
>>> enqueue/prepare more.
>>>
>>> Since some drivers (like caam) only enqueue request without "doing"
>>> them, do_one_request() is now optional.
>>>
>>> Signed-off-by: Corentin Labbe <clabbe.montjoie@xxxxxxxxx>
>>> ---
>>>    crypto/crypto_engine.c  | 25 ++++++++++++++++++++++---
>>>    include/crypto/engine.h | 14 ++++++++------
>>>    2 files changed, 30 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
>>> index 5bcb1e740fd9..4a28548c49aa 100644
>>> --- a/crypto/crypto_engine.c
>>> +++ b/crypto/crypto_engine.c
>>> @@ -83,6 +83,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>>>    		goto out;
>>>    	}
>>>    
>>> +retry:
>>>    	/* Get the fist request from the engine queue to handle */
>>>    	backlog = crypto_get_backlog(&engine->queue);
>>>    	async_req = crypto_dequeue_request(&engine->queue);
>>> @@ -118,10 +119,28 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>>>    			goto req_err2;
>>>    		}
>>>    	}
>>> +
>>> +	if (enginectx->op.enqueue_request) {
>>> +		ret = enginectx->op.enqueue_request(engine, async_req);
>>> +		if (ret) {
>>> +			dev_err(engine->dev, "failed to enqueue request: %d\n",
>>> +				ret);
>>> +			goto req_err;
>>> +		}
>>> +	}
>>> +	if (enginectx->op.can_queue_more && engine->queue.qlen > 0) {
>>> +		ret = enginectx->op.can_queue_more(engine, async_req);
>>> +		if (ret > 0) {
>>> +			spin_lock_irqsave(&engine->queue_lock, flags);
>>> +			goto retry;
>>> +		}
>>> +		if (ret < 0) {
>>> +			dev_err(engine->dev, "failed to call can_queue_more\n");
>>> +			/* TODO */
>>> +		}
>>> +	}
>>>    	if (!enginectx->op.do_one_request) {
>>> -		dev_err(engine->dev, "failed to do request\n");
>>> -		ret = -EINVAL;
>>> -		goto req_err;
>>> +		return;
>>>    	}
>>>    	ret = enginectx->op.do_one_request(engine, async_req);
>>>    	if (ret) {
>>> diff --git a/include/crypto/engine.h b/include/crypto/engine.h
>>> index 03d9f9ec1cea..8ab9d26e30fe 100644
>>> --- a/include/crypto/engine.h
>>> +++ b/include/crypto/engine.h
>>> @@ -63,14 +63,16 @@ struct crypto_engine {
>>>     * @prepare__request: do some prepare if need before handle the current request
>>>     * @unprepare_request: undo any work done by prepare_request()
>>>     * @do_one_request: do encryption for current request
>>> + * @enqueue_request:	Enqueue the request in the hardware
>>> + * @can_queue_more:	if this function return > 0, it will tell the crypto
>>> + * 	engine that more space are availlable for prepare/enqueue request
>>>     */
>>>    struct crypto_engine_op {
>>> -	int (*prepare_request)(struct crypto_engine *engine,
>>> -			       void *areq);
>>> -	int (*unprepare_request)(struct crypto_engine *engine,
>>> -				 void *areq);
>>> -	int (*do_one_request)(struct crypto_engine *engine,
>>> -			      void *areq);
>>> +	int (*prepare_request)(struct crypto_engine *engine, void *areq);
>>> +	int (*unprepare_request)(struct crypto_engine *engine, void *areq);
>>> +	int (*do_one_request)(struct crypto_engine *engine, void *areq);
>>> +	int (*enqueue_request)(struct crypto_engine *engine, void *areq);
>>> +	int (*can_queue_more)(struct crypto_engine *engine, void *areq);
>>>    };
>>
>> As I mentioned in another thread [1], these crypto-engine patches (#1 -
>> #5) imply modifications in all the drivers that use crypto-engine.
>> It's not backwards compatible.
> 
> This is wrong. This is false.
> AS I HAVE ALREADY SAID, I have tested and didnt see any behavour change in the current user of crypto engine.
> I have tested my serie with omap, virtio, amlogic, sun8i-ss, sun8i-ce and didnt see any change in behavour WITHOUT CHANGING them.
> I resaid, I didnt touch omap, virtio, etc...
> Only stm32 is not tested because simply there are not board with this driver enabled.
> 

I'm not saying that doesn't compile or anything, is just that you change 
the API and those drivers will not comply on this new API.
I believe that "it works" is not sufficient, should work properly!

> I have also tested your serie which adds support for crypto engine to caam, and the crash is the same with/without my serie.
> So no behavour change.
> 
Thanks for testing this. I'll look into it!

>> Your changes imply that do_one_request executes the request & waits for
>> completion and enqueue_request sends it to hardware. That means that all
>> the other drivers need to be modify, to implement enqueue_request,
>> instead of do_one_request. They need to be compliant with the new
>> changes, new API. Otherwise, they are not using crypto-engine right,
>> don't you think?
>>
> 
> My change imply nothing, current user work the same.
> But if they want, they COULD switch to enqueue_request().
> 
>> Also, do_one_request it shouldn’t be blocking. We got this confirmation
>> from Herbert [2].
> 
> Re-read what Herbert said, "It certainly shouldn't be blocking in the general case." But that means it could.
> But this wont change my patch since both behavour are supported.
> 

Since your driver is the one being different (implements do_one_request 
as blocking), it's not fair to change the other drivers just for you 
special case when we update the crypto-engine. It should be the other 
way around.
Add a special case for you and let the other drivers unchanged.
The updated crypto-engine API should be consistent (same semantics) with 
the old one. Your proposal doesn't extend the current API, but 
reshuffles the callbacks changing their meaning.
We should agree on how we should update crypto-engine to accommodate all 
the scenarios, but maintaining backwards compatibility.

Thanks,
Iulia

>>
>> [1]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FVI1PR04MB44455343230CBA7400D21C998C0C0%40VI1PR04MB4445.eurprd04.prod.outlook.com%2F&amp;data=02%7C01%7Ciuliana.prodan%40nxp.com%7C238e3e9a8e5f4d934cf308d7a3cdc3da%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637157976462463995&amp;sdata=rgzYhX0g9hrzlYcHs7aUWVNFYs6mj86gDu7YIowy0Nk%3D&amp;reserved=0
>> [2]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20200122144134.axqpwx65j7xysyy3%40gondor.apana.org.au%2F&amp;data=02%7C01%7Ciuliana.prodan%40nxp.com%7C238e3e9a8e5f4d934cf308d7a3cdc3da%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637157976462463995&amp;sdata=Jdp0Q8xPnn5uXtcv6hrk3sFbeC5PgzfwRys2itmL09w%3D&amp;reserved=0
>