| Message ID | 1493144468-22493-8-git-send-email-logang@deltatee.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
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,
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
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,
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,
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 --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
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(-)