Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1261380 > unrolled thread

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

Started byStanimir Varbanov <svarbanov@mm-sol.com>
First post2015-11-03 11:50 +0100
Last post2015-11-04 06:10 +0100
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] crypto: qce: dma_map_sg can handle chained SG Stanimir Varbanov <svarbanov@mm-sol.com> - 2015-11-03 11:50 +0100
    Re: [PATCH] crypto: qce: dma_map_sg can handle chained SG LABBE Corentin <clabbe.montjoie@gmail.com> - 2015-11-03 13:40 +0100
      Re: [PATCH] crypto: qce: dma_map_sg can handle chained SG Stanimir Varbanov <svarbanov@mm-sol.com> - 2015-11-03 14:40 +0100
      Re: [PATCH] crypto: qce: dma_map_sg can handle chained SG Herbert Xu <herbert@gondor.apana.org.au> - 2015-11-04 06:10 +0100

#1261380 — Re: [PATCH] crypto: qce: dma_map_sg can handle chained SG

FromStanimir Varbanov <svarbanov@mm-sol.com>
Date2015-11-03 11:50 +0100
SubjectRe: [PATCH] crypto: qce: dma_map_sg can handle chained SG
Message-ID<qqHk6-1z5-9@gated-at.bofh.it>
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 <clabbe.montjoie@gmail.com>
> ---
>  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?

> +	ret = dma_map_sg(qce->dev, req->src, rctx->src_nents, DMA_TO_DEVICE);
>  	if (ret < 0)
>  		return ret;
>  
>  	sg_init_one(&rctx->result_sg, qce->dma.result_buf, QCE_RESULT_BUF_SZ);
>  
> -	ret = qce_mapsg(qce->dev, &rctx->result_sg, 1, DMA_FROM_DEVICE, 0);
> +	ret = dma_map_sg(qce->dev, &rctx->result_sg, 1, DMA_FROM_DEVICE);
>  	if (ret < 0)
>  		goto error_unmap_src;
>  

<snip>


-- 
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1261451

FromLABBE Corentin <clabbe.montjoie@gmail.com>
Date2015-11-03 13:40 +0100
Message-ID<qqJ2x-2Hl-1@gated-at.bofh.it>
In reply to#1261380
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 <clabbe.montjoie@gmail.com>
> > ---
> >  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.
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() ?

Regards

LABBE Corentin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261502

FromStanimir Varbanov <svarbanov@mm-sol.com>
Date2015-11-03 14:40 +0100
Message-ID<qqJYC-3iE-19@gated-at.bofh.it>
In reply to#1261451
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 <clabbe.montjoie@gmail.com>
>>> ---
>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262074

FromHerbert Xu <herbert@gondor.apana.org.au>
Date2015-11-04 06:10 +0100
Message-ID<qqYuC-4tu-11@gated-at.bofh.it>
In reply to#1261451
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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web