diff mbox

[v2,07/21] crypto: shash, caam: Make use of the new sg_map helper function

Message ID 1493144468-22493-8-git-send-email-logang@deltatee.com (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show

Commit Message

Logan Gunthorpe April 25, 2017, 6:20 p.m. UTC
Very straightforward conversion to the new function in the caam driver
and shash library.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
 crypto/shash.c                | 9 ++++++---
 drivers/crypto/caam/caamalg.c | 8 +++-----
 2 files changed, 9 insertions(+), 8 deletions(-)

Comments

Herbert Xu April 27, 2017, 3:56 a.m. UTC | #1
On Tue, Apr 25, 2017 at 12:20:54PM -0600, Logan Gunthorpe wrote:
> Very straightforward conversion to the new function in the caam driver
> and shash library.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> ---
>  crypto/shash.c                | 9 ++++++---
>  drivers/crypto/caam/caamalg.c | 8 +++-----
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/crypto/shash.c b/crypto/shash.c
> index 5e31c8d..5914881 100644
> --- a/crypto/shash.c
> +++ b/crypto/shash.c
> @@ -283,10 +283,13 @@ int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
>  	if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
>  		void *data;
>  
> -		data = kmap_atomic(sg_page(sg));
> -		err = crypto_shash_digest(desc, data + offset, nbytes,
> +		data = sg_map(sg, 0, SG_KMAP_ATOMIC);
> +		if (IS_ERR(data))
> +			return PTR_ERR(data);
> +
> +		err = crypto_shash_digest(desc, data, nbytes,
>  					  req->result);
> -		kunmap_atomic(data);
> +		sg_unmap(sg, data, 0, SG_KMAP_ATOMIC);
>  		crypto_yield(desc->flags);
>  	} else
>  		err = crypto_shash_init(desc) ?:

Nack.  This is an optimisation for the special case of a single
SG list entry.  In fact in the common case the kmap_atomic should
disappear altogether in the no-highmem case.  So replacing it
with sg_map is not acceptable.

Cheers,
Logan Gunthorpe April 27, 2017, 3:45 p.m. UTC | #2
On 26/04/17 09:56 PM, Herbert Xu wrote:
> On Tue, Apr 25, 2017 at 12:20:54PM -0600, Logan Gunthorpe wrote:
>> Very straightforward conversion to the new function in the caam driver
>> and shash library.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> ---
>>  crypto/shash.c                | 9 ++++++---
>>  drivers/crypto/caam/caamalg.c | 8 +++-----
>>  2 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/crypto/shash.c b/crypto/shash.c
>> index 5e31c8d..5914881 100644
>> --- a/crypto/shash.c
>> +++ b/crypto/shash.c
>> @@ -283,10 +283,13 @@ int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
>>  	if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
>>  		void *data;
>>  
>> -		data = kmap_atomic(sg_page(sg));
>> -		err = crypto_shash_digest(desc, data + offset, nbytes,
>> +		data = sg_map(sg, 0, SG_KMAP_ATOMIC);
>> +		if (IS_ERR(data))
>> +			return PTR_ERR(data);
>> +
>> +		err = crypto_shash_digest(desc, data, nbytes,
>>  					  req->result);
>> -		kunmap_atomic(data);
>> +		sg_unmap(sg, data, 0, SG_KMAP_ATOMIC);
>>  		crypto_yield(desc->flags);
>>  	} else
>>  		err = crypto_shash_init(desc) ?:
> 
> Nack.  This is an optimisation for the special case of a single
> SG list entry.  In fact in the common case the kmap_atomic should
> disappear altogether in the no-highmem case.  So replacing it
> with sg_map is not acceptable.

What you seem to have missed is that sg_map is just a thin wrapper
around kmap_atomic. Perhaps with a future check for a mappable page.
This change should have zero impact on performance.

Logan
Herbert Xu April 28, 2017, 6:30 a.m. UTC | #3
On Thu, Apr 27, 2017 at 09:45:57AM -0600, Logan Gunthorpe wrote:
> 
> 
> On 26/04/17 09:56 PM, Herbert Xu wrote:
> > On Tue, Apr 25, 2017 at 12:20:54PM -0600, Logan Gunthorpe wrote:
> >> Very straightforward conversion to the new function in the caam driver
> >> and shash library.
> >>
> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> >> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> >> Cc: "David S. Miller" <davem@davemloft.net>
> >> ---
> >>  crypto/shash.c                | 9 ++++++---
> >>  drivers/crypto/caam/caamalg.c | 8 +++-----
> >>  2 files changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/crypto/shash.c b/crypto/shash.c
> >> index 5e31c8d..5914881 100644
> >> --- a/crypto/shash.c
> >> +++ b/crypto/shash.c
> >> @@ -283,10 +283,13 @@ int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
> >>  	if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
> >>  		void *data;
> >>  
> >> -		data = kmap_atomic(sg_page(sg));
> >> -		err = crypto_shash_digest(desc, data + offset, nbytes,
> >> +		data = sg_map(sg, 0, SG_KMAP_ATOMIC);
> >> +		if (IS_ERR(data))
> >> +			return PTR_ERR(data);
> >> +
> >> +		err = crypto_shash_digest(desc, data, nbytes,
> >>  					  req->result);
> >> -		kunmap_atomic(data);
> >> +		sg_unmap(sg, data, 0, SG_KMAP_ATOMIC);
> >>  		crypto_yield(desc->flags);
> >>  	} else
> >>  		err = crypto_shash_init(desc) ?:
> > 
> > Nack.  This is an optimisation for the special case of a single
> > SG list entry.  In fact in the common case the kmap_atomic should
> > disappear altogether in the no-highmem case.  So replacing it
> > with sg_map is not acceptable.
> 
> What you seem to have missed is that sg_map is just a thin wrapper
> around kmap_atomic. Perhaps with a future check for a mappable page.
> This change should have zero impact on performance.

You are right.  Indeed the existing code looks buggy as they
don't take sg->offset into account when doing the kmap.  Could
you send me some patches that fix these problems first so that
they can be easily backported?

Thanks,
Logan Gunthorpe April 28, 2017, 4:53 p.m. UTC | #4
On 28/04/17 12:30 AM, Herbert Xu wrote:
> You are right.  Indeed the existing code looks buggy as they
> don't take sg->offset into account when doing the kmap.  Could
> you send me some patches that fix these problems first so that
> they can be easily backported?

Ok, I think the only buggy one in crypto is hifn_795x. Shash and caam
both do have the sg->offset accounted for. I'll send a patch for the
buggy one shortly.

Logan
Herbert Xu April 28, 2017, 5:51 p.m. UTC | #5
On Fri, Apr 28, 2017 at 10:53:45AM -0600, Logan Gunthorpe wrote:
> 
> 
> On 28/04/17 12:30 AM, Herbert Xu wrote:
> > You are right.  Indeed the existing code looks buggy as they
> > don't take sg->offset into account when doing the kmap.  Could
> > you send me some patches that fix these problems first so that
> > they can be easily backported?
> 
> Ok, I think the only buggy one in crypto is hifn_795x. Shash and caam
> both do have the sg->offset accounted for. I'll send a patch for the
> buggy one shortly.

I think they're all buggy when sg->offset is greater than PAGE_SIZE.

Thanks,
Logan Gunthorpe April 28, 2017, 7:01 p.m. UTC | #6
On 28/04/17 11:51 AM, Herbert Xu wrote:
> On Fri, Apr 28, 2017 at 10:53:45AM -0600, Logan Gunthorpe wrote:
>>
>>
>> On 28/04/17 12:30 AM, Herbert Xu wrote:
>>> You are right.  Indeed the existing code looks buggy as they
>>> don't take sg->offset into account when doing the kmap.  Could
>>> you send me some patches that fix these problems first so that
>>> they can be easily backported?
>>
>> Ok, I think the only buggy one in crypto is hifn_795x. Shash and caam
>> both do have the sg->offset accounted for. I'll send a patch for the
>> buggy one shortly.
> 
> I think they're all buggy when sg->offset is greater than PAGE_SIZE.

Yes, technically. But that's a _very_ common mistake. Pretty nearly
every case I looked at did not take that into account. I don't think
sg's that point to more than one continuous page are all that common.

Fixing all those cases without making a common function is a waste of
time IMO.

Logan
diff mbox

Patch

diff --git a/crypto/shash.c b/crypto/shash.c
index 5e31c8d..5914881 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -283,10 +283,13 @@  int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
 	if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
 		void *data;
 
-		data = kmap_atomic(sg_page(sg));
-		err = crypto_shash_digest(desc, data + offset, nbytes,
+		data = sg_map(sg, 0, SG_KMAP_ATOMIC);
+		if (IS_ERR(data))
+			return PTR_ERR(data);
+
+		err = crypto_shash_digest(desc, data, nbytes,
 					  req->result);
-		kunmap_atomic(data);
+		sg_unmap(sg, data, 0, SG_KMAP_ATOMIC);
 		crypto_yield(desc->flags);
 	} else
 		err = crypto_shash_init(desc) ?:
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 398807d..62d2f5d 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -89,7 +89,6 @@  static void dbg_dump_sg(const char *level, const char *prefix_str,
 			struct scatterlist *sg, size_t tlen, bool ascii)
 {
 	struct scatterlist *it;
-	void *it_page;
 	size_t len;
 	void *buf;
 
@@ -98,19 +97,18 @@  static void dbg_dump_sg(const char *level, const char *prefix_str,
 		 * make sure the scatterlist's page
 		 * has a valid virtual memory mapping
 		 */
-		it_page = kmap_atomic(sg_page(it));
-		if (unlikely(!it_page)) {
+		buf = sg_map(it, 0, SG_KMAP_ATOMIC);
+		if (IS_ERR(buf)) {
 			printk(KERN_ERR "dbg_dump_sg: kmap failed\n");
 			return;
 		}
 
-		buf = it_page + it->offset;
 		len = min_t(size_t, tlen, it->length);
 		print_hex_dump(level, prefix_str, prefix_type, rowsize,
 			       groupsize, buf, len, ascii);
 		tlen -= len;
 
-		kunmap_atomic(it_page);
+		sg_unmap(it, buf, 0, SG_KMAP_ATOMIC);
 	}
 }
 #endif