Very straightforward conversion to the new function in the caam driver
and shash library.
Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/[email protected]>
---
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) ?:
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
--
2.1.4
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 <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> ---
> 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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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-OTvnGxWRz7hWk0Htik3J/[email protected]>
>> Cc: Herbert Xu <[email protected]>
>> Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/[email protected]>
>> ---
>> 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
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-OTvnGxWRz7hWk0Htik3J/[email protected]>
> >> Cc: Herbert Xu <[email protected]>
> >> Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/[email protected]>
> >> ---
> >> 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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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
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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx