linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] crypto: hifn_795x: Fix a handful of minor bugs spotted by eye
@ 2017-04-28 17:02 Logan Gunthorpe
  2017-05-18  3:25 ` Herbert Xu
  0 siblings, 1 reply; 5+ messages in thread
From: Logan Gunthorpe @ 2017-04-28 17:02 UTC (permalink / raw)
  To: linux-crypto, linux-kernel; +Cc: Logan Gunthorpe, Herbert Xu, David S. Miller

* Cleaned up the formatting of ablkcipher_get arguments so it complies
  with kernel style
* The offset in ablkcipher_get sould be added to the source, not the
  destination. We rename it to soffset for clarity.
* dst++ should be dst=sg_next(dst)
* We call kunmap_atomic earlier so we only have to do it once.
* If ablkcipher_get fails, we should probably ensure the request
  completes with an error.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
 drivers/crypto/hifn_795x.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index e09d405..4c19e4a 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -1619,8 +1619,9 @@ static int hifn_start_device(struct hifn_device *dev)
 	return 0;
 }
 
-static int ablkcipher_get(void *saddr, unsigned int *srestp, unsigned int offset,
-		struct scatterlist *dst, unsigned int size, unsigned int *nbytesp)
+static int ablkcipher_get(void *saddr, unsigned int *srestp,
+			  unsigned int soffset, struct scatterlist *dst,
+			  unsigned int size, unsigned int *nbytesp)
 {
 	unsigned int srest = *srestp, nbytes = *nbytesp, copy;
 	void *daddr;
@@ -1633,19 +1634,19 @@ static int ablkcipher_get(void *saddr, unsigned int *srestp, unsigned int offset
 		copy = min3(srest, dst->length, size);
 
 		daddr = kmap_atomic(sg_page(dst));
-		memcpy(daddr + dst->offset + offset, saddr, copy);
+		memcpy(daddr + dst->offset, saddr + soffset, copy);
 		kunmap_atomic(daddr);
 
 		nbytes -= copy;
 		size -= copy;
 		srest -= copy;
 		saddr += copy;
-		offset = 0;
+		soffset = 0;
 
 		pr_debug("%s: copy: %u, size: %u, srest: %u, nbytes: %u.\n",
 			 __func__, copy, size, srest, nbytes);
 
-		dst++;
+		dst = sg_next(dst);
 		idx++;
 	}
 
@@ -1699,13 +1700,15 @@ static void hifn_process_ready(struct ablkcipher_request *req, int error)
 
 			err = ablkcipher_get(saddr, &t->length, t->offset,
 					dst, nbytes, &nbytes);
+			kunmap_atomic(saddr);
+
 			if (err < 0) {
-				kunmap_atomic(saddr);
+				if (!error)
+					error = err;
 				break;
 			}
 
 			idx += err;
-			kunmap_atomic(saddr);
 		}
 
 		hifn_cipher_walk_exit(&rctx->walk);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] crypto: hifn_795x: Fix a handful of minor bugs spotted by eye
  2017-04-28 17:02 [PATCH] crypto: hifn_795x: Fix a handful of minor bugs spotted by eye Logan Gunthorpe
@ 2017-05-18  3:25 ` Herbert Xu
  2017-05-18  3:26   ` Herbert Xu
  0 siblings, 1 reply; 5+ messages in thread
From: Herbert Xu @ 2017-05-18  3:25 UTC (permalink / raw)
  To: Logan Gunthorpe; +Cc: linux-crypto, linux-kernel, David S. Miller

On Fri, Apr 28, 2017 at 11:02:40AM -0600, Logan Gunthorpe wrote:
> * Cleaned up the formatting of ablkcipher_get arguments so it complies
>   with kernel style
> * The offset in ablkcipher_get sould be added to the source, not the
>   destination. We rename it to soffset for clarity.
> * dst++ should be dst=sg_next(dst)
> * We call kunmap_atomic earlier so we only have to do it once.
> * If ablkcipher_get fails, we should probably ensure the request
>   completes with an error.

Please don't mix unrelated changes in the same patch.

Also was this tested on actual hardware? This is an old driver so
it's probably best to just let it be.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] crypto: hifn_795x: Fix a handful of minor bugs spotted by eye
  2017-05-18  3:25 ` Herbert Xu
@ 2017-05-18  3:26   ` Herbert Xu
  2017-05-18  3:33     ` Logan Gunthorpe
  0 siblings, 1 reply; 5+ messages in thread
From: Herbert Xu @ 2017-05-18  3:26 UTC (permalink / raw)
  To: Logan Gunthorpe; +Cc: linux-crypto, linux-kernel, David S. Miller

On Thu, May 18, 2017 at 11:25:06AM +0800, Herbert Xu wrote:
> On Fri, Apr 28, 2017 at 11:02:40AM -0600, Logan Gunthorpe wrote:
> > * Cleaned up the formatting of ablkcipher_get arguments so it complies
> >   with kernel style
> > * The offset in ablkcipher_get sould be added to the source, not the
> >   destination. We rename it to soffset for clarity.
> > * dst++ should be dst=sg_next(dst)
> > * We call kunmap_atomic earlier so we only have to do it once.
> > * If ablkcipher_get fails, we should probably ensure the request
> >   completes with an error.
> 
> Please don't mix unrelated changes in the same patch.
> 
> Also was this tested on actual hardware? This is an old driver so
> it's probably best to just let it be.

Of course we do want fix the offset issue so could you please send
a new patch with just that fix?

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] crypto: hifn_795x: Fix a handful of minor bugs spotted by eye
  2017-05-18  3:26   ` Herbert Xu
@ 2017-05-18  3:33     ` Logan Gunthorpe
  2017-05-18  4:02       ` David Miller
  0 siblings, 1 reply; 5+ messages in thread
From: Logan Gunthorpe @ 2017-05-18  3:33 UTC (permalink / raw)
  To: Herbert Xu; +Cc: linux-crypto, linux-kernel, David S. Miller



On 17/05/17 09:26 PM, Herbert Xu wrote:
> On Thu, May 18, 2017 at 11:25:06AM +0800, Herbert Xu wrote:
>> On Fri, Apr 28, 2017 at 11:02:40AM -0600, Logan Gunthorpe wrote:
>>> * Cleaned up the formatting of ablkcipher_get arguments so it complies
>>>   with kernel style
>>> * The offset in ablkcipher_get sould be added to the source, not the
>>>   destination. We rename it to soffset for clarity.
>>> * dst++ should be dst=sg_next(dst)
>>> * We call kunmap_atomic earlier so we only have to do it once.
>>> * If ablkcipher_get fails, we should probably ensure the request
>>>   completes with an error.

>> Please don't mix unrelated changes in the same patch.

Seriously? The patch is tiny and those are all incredibly minor changes.

>> Also was this tested on actual hardware? This is an old driver so
>> it's probably best to just let it be.

No, I don't have that hardware. I sent this patch because you requested it.

> Of course we do want fix the offset issue so could you please send
> a new patch with just that fix?

Sorry, no, I honestly don't see that as worth my time. Feel free to drop
the patch or roll your own.

Logan

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] crypto: hifn_795x: Fix a handful of minor bugs spotted by eye
  2017-05-18  3:33     ` Logan Gunthorpe
@ 2017-05-18  4:02       ` David Miller
  0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2017-05-18  4:02 UTC (permalink / raw)
  To: logang; +Cc: herbert, linux-crypto, linux-kernel

From: Logan Gunthorpe <logang@deltatee.com>
Date: Wed, 17 May 2017 21:33:22 -0600

> 
> 
> On 17/05/17 09:26 PM, Herbert Xu wrote:
>> On Thu, May 18, 2017 at 11:25:06AM +0800, Herbert Xu wrote:
>>> On Fri, Apr 28, 2017 at 11:02:40AM -0600, Logan Gunthorpe wrote:
>>>> * Cleaned up the formatting of ablkcipher_get arguments so it complies
>>>>   with kernel style
>>>> * The offset in ablkcipher_get sould be added to the source, not the
>>>>   destination. We rename it to soffset for clarity.
>>>> * dst++ should be dst=sg_next(dst)
>>>> * We call kunmap_atomic earlier so we only have to do it once.
>>>> * If ablkcipher_get fails, we should probably ensure the request
>>>>   completes with an error.
> 
>>> Please don't mix unrelated changes in the same patch.
> 
> Seriously? The patch is tiny and those are all incredibly minor changes.

Yes, seriously.

It is the only way to make your changes easy to review and audit.

Please do as Herbert is asking.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-05-18  4:02 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-28 17:02 [PATCH] crypto: hifn_795x: Fix a handful of minor bugs spotted by eye Logan Gunthorpe
2017-05-18  3:25 ` Herbert Xu
2017-05-18  3:26   ` Herbert Xu
2017-05-18  3:33     ` Logan Gunthorpe
2017-05-18  4:02       ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).