by Stanimir Varbanov

[permalink] [raw]

Subject: Re: [PATCH] crypto: qce: dma_map_sg can handle chained SG

On 11/03/2015 02:36 PM, LABBE Corentin wrote:
> On Tue, Nov 03, 2015 at 12:39:57PM +0200, Stanimir Varbanov wrote:
>> Hi,
>>
>> I know that this patch has been queued up, but ...
>>
>> On 10/02/2015 09:01 AM, LABBE Corentin wrote:
>>> The qce driver use two dma_map_sg path according to SG are chained
>>> or not.
>>> Since dma_map_sg can handle both case, clean the code with all
>>> references to sg chained.
>>>
>>> Thus removing qce_mapsg, qce_unmapsg and qce_countsg functions.
>>>
>>> Signed-off-by: LABBE Corentin <[email protected]>
>>> ---
>>> drivers/crypto/qce/ablkcipher.c | 30 ++++++++----------------
>>> drivers/crypto/qce/cipher.h | 4 ----
>>> drivers/crypto/qce/dma.c | 52 -----------------------------------------
>>> drivers/crypto/qce/dma.h | 5 ----
>>> drivers/crypto/qce/sha.c | 18 ++++++--------
>>> drivers/crypto/qce/sha.h | 2 --
>>> 6 files changed, 17 insertions(+), 94 deletions(-)
>>>
>>
>> <snip>
>>
>>> diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c
>>> index be2f504..0c9973e 100644
>>> --- a/drivers/crypto/qce/sha.c
>>> +++ b/drivers/crypto/qce/sha.c
>>> @@ -51,9 +51,8 @@ static void qce_ahash_done(void *data)
>>> if (error)
>>> dev_dbg(qce->dev, "ahash dma termination error (%d)\n", error);
>>>
>>> - qce_unmapsg(qce->dev, req->src, rctx->src_nents, DMA_TO_DEVICE,
>>> - rctx->src_chained);
>>> - qce_unmapsg(qce->dev, &rctx->result_sg, 1, DMA_FROM_DEVICE, 0);
>>> + dma_unmap_sg(qce->dev, req->src, rctx->src_nents, DMA_TO_DEVICE);
>>> + dma_unmap_sg(qce->dev, &rctx->result_sg, 1, DMA_FROM_DEVICE);
>>>
>>> memcpy(rctx->digest, result->auth_iv, digestsize);
>>> if (req->result)
>>> @@ -92,16 +91,14 @@ static int qce_ahash_async_req_handle(struct crypto_async_request *async_req)
>>> rctx->authklen = AES_KEYSIZE_128;
>>> }
>>>
>>> - rctx->src_nents = qce_countsg(req->src, req->nbytes,
>>> - &rctx->src_chained);
>>> - ret = qce_mapsg(qce->dev, req->src, rctx->src_nents, DMA_TO_DEVICE,
>>> - rctx->src_chained);
>>> + rctx->src_nents = sg_nents_for_len(req->src, req->nbytes);
>>
>> sg_nents_for_len can return -EINVAL, and this error should be handled.
>>
>> After added a check for the error I'm observing below:
>>
>> #insmod tcrypt.ko mode=403 (test_ahash_speed("sha1"))
>>
>> test 21 ( 8192 byte blocks, 8192 bytes per update, 1 updates):
>> qcrypto 73a000.crypto: sg_nents_for_len failed (-22) (nbytes:8192,
>> sg_len:4096)
>> hashing failed ret=-22
>>
>> It seems that something is wrong with the test case? Looking further in
>> test_hash_sg_init() we can see:
>>
>> #define TVMEMSIZE 4
>>
>> sg_init_table(sg, TVMEMSIZE);
>> for (i = 0; i < TVMEMSIZE; i++) {
>> sg_set_buf(sg + i, tvmem[i], PAGE_SIZE);
>> memset(tvmem[i], 0xff, PAGE_SIZE);
>> }
>>
>> so we have 4 SGs with sg->length = 4096, thus sg_nents_for_len() should
>> return 2 entries with 4096 bytes each.
>>
>> What is wrong here?
>>
>
> Hello
>
> It is a shame that I forgot to check the return value.

I guess that you expected dma_map_sg() to handle the error, and infact
it does - trow out BUG_ON :))

More interesting thing to me is how the above test case is expected to
work without sg chaining.

--
regards,
Stan

2015-11-04 05:02:29

by Herbert Xu

[permalink] [raw]

Subject: Re: [PATCH] crypto: qce: dma_map_sg can handle chained SG

On Tue, Nov 03, 2015 at 01:36:55PM +0100, LABBE Corentin wrote:
>
> Herbert, do you prefer to drop the patch series and I send an updated one, or does I will send a new patch series for checking the return value of sg_nents_for_len() ?

Your patch has already been applied so please send any fixes on
top of it.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt