[PATCH 1/1] crypto-drbg: Deletion of unnecessary checks before the function call "kzfree"

[PATCH 1/1] crypto-drbg: Deletion of unnecessary checks before the function call "kzfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 19 Nov 2014 10:11:04 +0100

The kzfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 crypto/drbg.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index a53ee09..b6f22d4 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1151,8 +1151,7 @@ static int drbg_seed(struct drbg_state *drbg, struct drbg_string *pers,
 	drbg->reseed_ctr = 1;
 
 out:
-	if (entropy)
-		kzfree(entropy);
+	kzfree(entropy);
 	return ret;
 }
 
@@ -1161,19 +1160,15 @@ static inline void drbg_dealloc_state(struct drbg_state *drbg)
 {
 	if (!drbg)
 		return;
-	if (drbg->V)
-		kzfree(drbg->V);
+	kzfree(drbg->V);
 	drbg->V = NULL;
-	if (drbg->C)
-		kzfree(drbg->C);
+	kzfree(drbg->C);
 	drbg->C = NULL;
-	if (drbg->scratchpad)
-		kzfree(drbg->scratchpad);
+	kzfree(drbg->scratchpad);
 	drbg->scratchpad = NULL;
 	drbg->reseed_ctr = 0;
 #ifdef CONFIG_CRYPTO_FIPS
-	if (drbg->prev)
-		kzfree(drbg->prev);
+	kzfree(drbg->prev);
 	drbg->prev = NULL;
 	drbg->fips_primed = false;
 #endif
@@ -1293,8 +1288,7 @@ static int drbg_make_shadow(struct drbg_state *drbg, struct drbg_state **shadow)
 	return 0;
 
 err:
-	if (tmp)
-		kzfree(tmp);
+	kzfree(tmp);
 	return ret;
 }
 
-- 
2.1.3

Re: [PATCH 1/1] crypto-drbg: Deletion of unnecessary checks before the function call "kzfree"

[Herbert Xu]

On Wed, Nov 19, 2014 at 10:20:32AM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 19 Nov 2014 10:11:04 +0100
> 
> The kzfree() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Sorry but you're too late as someone else has already fixed this :)

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

Re: crypto-drbg: Deletion of unnecessary checks before the function call "kzfree"

[SF Markus Elfring]

> Sorry but you're too late as someone else has already fixed this :)

Thanks for your feedback.

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/crypto/drbg.c?id=46f64f6ef978dc1f36ebaa50ed79c7c8386711ee

Regards,
Markus

[PATCH] crypto-jitterentropy: Delete unnecessary checks before the function call "kzfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 23 Jun 2015 22:30:21 +0200

The kzfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 crypto/jitterentropy.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/crypto/jitterentropy.c b/crypto/jitterentropy.c
index d3c3045..22ded3e 100644
--- a/crypto/jitterentropy.c
+++ b/crypto/jitterentropy.c
@@ -698,11 +698,9 @@ static struct rand_data *jent_entropy_collector_alloc(unsigned int osr,
 
 static void jent_entropy_collector_free(struct rand_data *entropy_collector)
 {
-	if (entropy_collector->mem)
-		kzfree(entropy_collector->mem);
+	kzfree(entropy_collector->mem);
 	entropy_collector->mem = NULL;
-	if (entropy_collector)
-		kzfree(entropy_collector);
+	kzfree(entropy_collector);
 	entropy_collector = NULL;
 }
 
-- 
2.4.4

Re: [PATCH] crypto-jitterentropy: Delete unnecessary checks before the function call "kzfree"

[Stephan Mueller]

Am Dienstag, 23. Juni 2015, 23:20:21 schrieb SF Markus Elfring:

Hi Markus,

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 23 Jun 2015 22:30:21 +0200
> 
> The kzfree() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.

Ack: the patch is correct and I was overzealous.

Nack: I just proposed a patch earlier today to avoid compiler warnings [1]. 
That patch clashes with yours. May I ask you to wait briefly to see whether 
that patch will be accepted? If yes, it would be great if you could repost the 
patch that applies to the changed code.

PS: when you are at it, maybe even the last line in the function can be 
removed: setting the entropy_collector to NULL at that the end of the function 
is moot as well.

Thanks.

[1] http://lkml.iu.edu/hypermail/linux/kernel/1506.2/05196.html
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  crypto/jitterentropy.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/crypto/jitterentropy.c b/crypto/jitterentropy.c
> index d3c3045..22ded3e 100644
> --- a/crypto/jitterentropy.c
> +++ b/crypto/jitterentropy.c
> @@ -698,11 +698,9 @@ static struct rand_data
> *jent_entropy_collector_alloc(unsigned int osr,
> 
>  static void jent_entropy_collector_free(struct rand_data
> *entropy_collector) {
> -	if (entropy_collector->mem)
> -		kzfree(entropy_collector->mem);
> +	kzfree(entropy_collector->mem);
>  	entropy_collector->mem = NULL;
> -	if (entropy_collector)
> -		kzfree(entropy_collector);
> +	kzfree(entropy_collector);
>  	entropy_collector = NULL;
>  }


-- 
Ciao
Stephan

Re: [PATCH] crypto-jitterentropy: Delete unnecessary checks before the function call "kzfree"

[Dan Carpenter]

The other NULL assignment isn't useful either.  Also If
"entropy_collector" is NULL then we would oops calling
kzfree(entropy_collector->mem);
       ^^^^^^^^^^^^^^^^^^^^^^
Dereference.

I don't understand the patch that you sent.  We shouldn't be introducing
jent_zalloc() or jent_zfree().  Why are you adding abstractions there?
Quite bad ones as well.

regards,
dan carpenter

Re: [PATCH] crypto-jitterentropy: Delete unnecessary checks before the function call "kzfree"

[Stephan Mueller]

Am Mittwoch, 24. Juni 2015, 10:48:19 schrieb Dan Carpenter:

Hi Dan,

>The other NULL assignment isn't useful either.  Also If
>"entropy_collector" is NULL then we would oops calling
>kzfree(entropy_collector->mem);
>       ^^^^^^^^^^^^^^^^^^^^^^
>Dereference.
>
>I don't understand the patch that you sent.  We shouldn't be introducing
>jent_zalloc() or jent_zfree().  Why are you adding abstractions there?

The one C file should have no dependencies on any kernel header files. Thus, 
all links to kernel functions are wrapped.

>Quite bad ones as well.

What would you suggest to change?

Thanks.

Ciao
Stephan

Re: [PATCH] crypto-jitterentropy: Delete unnecessary checks before the function call "kzfree"

[Dan Carpenter]

Ah.  I should have read the whole thread.  I sort of assumed we were
just adding abstractions for the fun of it (because I have worked in
staging for too long).

regards,
dan carpenter

Re: [PATCH] crypto-jitterentropy: Delete unnecessary checks before the function call "kzfree"

[Herbert Xu]

On Tue, Jun 23, 2015 at 11:20:21PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 23 Jun 2015 22:30:21 +0200
> 
> The kzfree() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied.
-- 
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

[PATCH] crypto-qat: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 26 Jun 2015 20:30:11 +0200

The functions kfree() and release_firmware() test whether their argument
is NULL and then return immediately.
Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/crypto/qat/qat_common/adf_accel_engine.c | 5 +----
 drivers/crypto/qat/qat_common/adf_transport.c    | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/adf_accel_engine.c b/drivers/crypto/qat/qat_common/adf_accel_engine.c
index 7f8b66c..fdda8e7 100644
--- a/drivers/crypto/qat/qat_common/adf_accel_engine.c
+++ b/drivers/crypto/qat/qat_common/adf_accel_engine.c
@@ -88,10 +88,7 @@ void adf_ae_fw_release(struct adf_accel_dev *accel_dev)
 
 	qat_uclo_del_uof_obj(loader_data->fw_loader);
 	qat_hal_deinit(loader_data->fw_loader);
-
-	if (loader_data->uof_fw)
-		release_firmware(loader_data->uof_fw);
-
+	release_firmware(loader_data->uof_fw);
 	loader_data->uof_fw = NULL;
 	loader_data->fw_loader = NULL;
 }
diff --git a/drivers/crypto/qat/qat_common/adf_transport.c b/drivers/crypto/qat/qat_common/adf_transport.c
index ccec3274..db2926b 100644
--- a/drivers/crypto/qat/qat_common/adf_transport.c
+++ b/drivers/crypto/qat/qat_common/adf_transport.c
@@ -449,7 +449,7 @@ static int adf_init_bank(struct adf_accel_dev *accel_dev,
 err:
 	for (i = 0; i < ADF_ETR_MAX_RINGS_PER_BANK; i++) {
 		ring = &bank->rings[i];
-		if (hw_data->tx_rings_mask & (1 << i) && ring->inflights)
+		if (hw_data->tx_rings_mask & (1 << i))
 			kfree(ring->inflights);
 	}
 	return -ENOMEM;
-- 
2.4.4

Re: [PATCH] crypto-qat: Deletion of unnecessary checks before two function calls

[Herbert Xu]

On Fri, Jun 26, 2015 at 08:45:28PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 26 Jun 2015 20:30:11 +0200
> 
> The functions kfree() and release_firmware() test whether their argument
> is NULL and then return immediately.
> Thus the test around the calls is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied.
-- 
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

[PATCH 0/4] crypto-ixp4xx: Deletion of a few unnecessary checks

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Nov 2015 19:39:00 +0100

Further update suggestions were taken into account after a patch
was applied from static source code analysis.

Markus Elfring (4):
  Delete unnecessary checks before the function call "dma_pool_destroy"
  Two function calls less in init_ixp_crypto() after error detection
  Reduce assignment for a variable in init_ixp_crypto()
  Less function calls in init_ixp_crypto() after error detection

 drivers/crypto/ixp4xx_crypto.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

-- 
2.6.2

[PATCH 1/4] crypto-ixp4xx: Delete unnecessary checks before the function call "dma_pool_destroy"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Nov 2015 16:51:21 +0100

The dma_pool_destroy() function tests whether its argument is NULL
and then returns immediately. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/crypto/ixp4xx_crypto.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 8f27903..e52496a 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -510,10 +510,8 @@ npe_error:
 	printk(KERN_ERR "%s not responding\n", npe_name(npe_c));
 	ret = -EIO;
 err:
-	if (ctx_pool)
-		dma_pool_destroy(ctx_pool);
-	if (buffer_pool)
-		dma_pool_destroy(buffer_pool);
+	dma_pool_destroy(ctx_pool);
+	dma_pool_destroy(buffer_pool);
 	npe_release(npe_c);
 	return ret;
 }
-- 
2.6.2

Re: [PATCH 1/4] crypto-ixp4xx: Delete unnecessary checks before the function call "dma_pool_destroy"

[Herbert Xu]

On Sun, Nov 15, 2015 at 07:49:16PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Nov 2015 16:51:21 +0100
> 
> The dma_pool_destroy() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the calls is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Patch applied.  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

[PATCH 2/4] crypto-ixp4xx: Two function calls less in init_ixp_crypto() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Nov 2015 18:28:39 +0100

The dma_pool_destroy() function was called twice with a null pointer
if a "npe_error" was reported.

This implementation detail could be improved by the introduction
of another jump label.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/crypto/ixp4xx_crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index e52496a..efe0eca 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -509,9 +509,11 @@ static int init_ixp_crypto(struct device *dev)
 npe_error:
 	printk(KERN_ERR "%s not responding\n", npe_name(npe_c));
 	ret = -EIO;
+	goto release_npe;
 err:
 	dma_pool_destroy(ctx_pool);
 	dma_pool_destroy(buffer_pool);
+release_npe:
 	npe_release(npe_c);
 	return ret;
 }
-- 
2.6.2

Re: [PATCH 2/4] crypto-ixp4xx: Two function calls less in init_ixp_crypto() after error detection

[Herbert Xu]

On Sun, Nov 15, 2015 at 07:50:51PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Nov 2015 18:28:39 +0100
> 
> The dma_pool_destroy() function was called twice with a null pointer
> if a "npe_error" was reported.
> 
> This implementation detail could be improved by the introduction
> of another jump label.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/crypto/ixp4xx_crypto.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
> index e52496a..efe0eca 100644
> --- a/drivers/crypto/ixp4xx_crypto.c
> +++ b/drivers/crypto/ixp4xx_crypto.c
> @@ -509,9 +509,11 @@ static int init_ixp_crypto(struct device *dev)
>  npe_error:
>  	printk(KERN_ERR "%s not responding\n", npe_name(npe_c));
>  	ret = -EIO;
> +	goto release_npe;
>  err:
>  	dma_pool_destroy(ctx_pool);
>  	dma_pool_destroy(buffer_pool);
> +release_npe:
>  	npe_release(npe_c);
>  	return ret;

I think this patch is making the code worse so I'm not applying it.
-- 
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

[PATCH 3/4] crypto-ixp4xx: Reduce assignment for a variable in init_ixp_crypto()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Nov 2015 19:06:44 +0100

The variable "ret" was set more often than necessary by the
init_ixp_crypto() function.

* Omit its initialisation at the beginning.

* Use an error return code in two cases directly.

* Improve compliance with the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/crypto/ixp4xx_crypto.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index efe0eca..97da36a 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -433,17 +433,17 @@ static void crypto_done_action(unsigned long arg)
 
 static int init_ixp_crypto(struct device *dev)
 {
-	int ret = -ENODEV;
+	int ret;
 	u32 msg[2] = { 0, 0 };
 
 	if (! ( ~(*IXP4XX_EXP_CFG2) & (IXP4XX_FEATURE_HASH |
 				IXP4XX_FEATURE_AES | IXP4XX_FEATURE_DES))) {
 		printk(KERN_ERR "ixp_crypto: No HW crypto available\n");
-		return ret;
+		return -ENODEV;
 	}
 	npe_c = npe_request(NPE_ID);
 	if (!npe_c)
-		return ret;
+		return -ENODEV;
 
 	if (!npe_running(npe_c)) {
 		ret = npe_load_firmware(npe_c, npe_name(npe_c), dev);
@@ -481,13 +481,14 @@ static int init_ixp_crypto(struct device *dev)
 	BUILD_BUG_ON(SHA1_DIGEST_SIZE > sizeof(struct buffer_desc));
 	buffer_pool = dma_pool_create("buffer", dev,
 			sizeof(struct buffer_desc), 32, 0);
-	ret = -ENOMEM;
 	if (!buffer_pool) {
+		ret = -ENOMEM;
 		goto err;
 	}
 	ctx_pool = dma_pool_create("context", dev,
 			NPE_CTX_LEN, 16, 0);
 	if (!ctx_pool) {
+		ret = -ENOMEM;
 		goto err;
 	}
 	ret = qmgr_request_queue(SEND_QID, NPE_QLEN_TOTAL, 0, 0,
-- 
2.6.2

Re: [PATCH 3/4] crypto-ixp4xx: Reduce assignment for a variable in init_ixp_crypto()

[Herbert Xu]

On Sun, Nov 15, 2015 at 07:52:22PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Nov 2015 19:06:44 +0100
> 
> The variable "ret" was set more often than necessary by the
> init_ixp_crypto() function.
> 
> * Omit its initialisation at the beginning.
> 
> * Use an error return code in two cases directly.
> 
> * Improve compliance with the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Again I don't see how this is making the code better so nack.
-- 
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

[PATCH 4/4] crypto-ixp4xx: Less function calls in init_ixp_crypto() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Nov 2015 19:23:55 +0100

The dma_pool_destroy() function was called in up to two cases by the
init_ixp_crypto() function during error handling even if a call of
the dma_pool_create() function failed.

This implementation detail could be improved by the adjustment
of jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/crypto/ixp4xx_crypto.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 97da36a..a2c3155 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -483,23 +483,23 @@ static int init_ixp_crypto(struct device *dev)
 			sizeof(struct buffer_desc), 32, 0);
 	if (!buffer_pool) {
 		ret = -ENOMEM;
-		goto err;
+		goto release_npe;
 	}
 	ctx_pool = dma_pool_create("context", dev,
 			NPE_CTX_LEN, 16, 0);
 	if (!ctx_pool) {
 		ret = -ENOMEM;
-		goto err;
+		goto destroy_buffer_pool;
 	}
 	ret = qmgr_request_queue(SEND_QID, NPE_QLEN_TOTAL, 0, 0,
 				 "ixp_crypto:out", NULL);
 	if (ret)
-		goto err;
+		goto destroy_ctx_pool;
 	ret = qmgr_request_queue(RECV_QID, NPE_QLEN, 0, 0,
 				 "ixp_crypto:in", NULL);
 	if (ret) {
 		qmgr_release_queue(SEND_QID);
-		goto err;
+		goto destroy_ctx_pool;
 	}
 	qmgr_set_irq(RECV_QID, QUEUE_IRQ_SRC_NOT_EMPTY, irqhandler, NULL);
 	tasklet_init(&crypto_done_tasklet, crypto_done_action, 0);
@@ -511,8 +511,9 @@ npe_error:
 	printk(KERN_ERR "%s not responding\n", npe_name(npe_c));
 	ret = -EIO;
 	goto release_npe;
-err:
+destroy_ctx_pool:
 	dma_pool_destroy(ctx_pool);
+destroy_buffer_pool:
 	dma_pool_destroy(buffer_pool);
 release_npe:
 	npe_release(npe_c);
-- 
2.6.2

[PATCH v2 0/3] crypto-ixp4xx: Deletion of a few unnecessary checks

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 17 Nov 2015 16:26:01 +0100
Subject: [PATCH 0/3] crypto-ixp4xx: Deletion of a few unnecessary checks

Further update suggestions were taken into account after a patch
was applied from static source code analysis.

Markus Elfring (3):
  Delete unnecessary checks before the function call "dma_pool_destroy"
  Reduce assignment for a variable in init_ixp_crypto()
  crypto-ixp4xx: Less function calls in init_ixp_crypto() after error detection

 drivers/crypto/ixp4xx_crypto.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

-- 
2.6.2

[PATCH v2 1/3] crypto-ixp4xx: Delete unnecessary checks before the function call "dma_pool_destroy"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Nov 2015 16:51:21 +0100

The dma_pool_destroy() function tests whether its argument is NULL
and then returns immediately. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/crypto/ixp4xx_crypto.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 8f27903..e52496a 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -510,10 +510,8 @@ npe_error:
 	printk(KERN_ERR "%s not responding\n", npe_name(npe_c));
 	ret = -EIO;
 err:
-	if (ctx_pool)
-		dma_pool_destroy(ctx_pool);
-	if (buffer_pool)
-		dma_pool_destroy(buffer_pool);
+	dma_pool_destroy(ctx_pool);
+	dma_pool_destroy(buffer_pool);
 	npe_release(npe_c);
 	return ret;
 }
-- 
2.6.2

[PATCH v2 2/3] crypto-ixp4xx: Reduce assignment for a variable in init_ixp_crypto()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 17 Nov 2015 15:45:32 +0100

The variable "ret" was set more often than necessary by the
init_ixp_crypto() function.

* Omit its initialisation at the beginning.

* Use an error return code in two cases directly.

* Improve compliance with the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/crypto/ixp4xx_crypto.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index e52496a..79b6958 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -433,17 +433,17 @@ static void crypto_done_action(unsigned long arg)
 
 static int init_ixp_crypto(struct device *dev)
 {
-	int ret = -ENODEV;
+	int ret;
 	u32 msg[2] = { 0, 0 };
 
 	if (! ( ~(*IXP4XX_EXP_CFG2) & (IXP4XX_FEATURE_HASH |
 				IXP4XX_FEATURE_AES | IXP4XX_FEATURE_DES))) {
 		printk(KERN_ERR "ixp_crypto: No HW crypto available\n");
-		return ret;
+		return -ENODEV;
 	}
 	npe_c = npe_request(NPE_ID);
 	if (!npe_c)
-		return ret;
+		return -ENODEV;
 
 	if (!npe_running(npe_c)) {
 		ret = npe_load_firmware(npe_c, npe_name(npe_c), dev);
@@ -481,13 +481,14 @@ static int init_ixp_crypto(struct device *dev)
 	BUILD_BUG_ON(SHA1_DIGEST_SIZE > sizeof(struct buffer_desc));
 	buffer_pool = dma_pool_create("buffer", dev,
 			sizeof(struct buffer_desc), 32, 0);
-	ret = -ENOMEM;
 	if (!buffer_pool) {
+		ret = -ENOMEM;
 		goto err;
 	}
 	ctx_pool = dma_pool_create("context", dev,
 			NPE_CTX_LEN, 16, 0);
 	if (!ctx_pool) {
+		ret = -ENOMEM;
 		goto err;
 	}
 	ret = qmgr_request_queue(SEND_QID, NPE_QLEN_TOTAL, 0, 0,
-- 
2.6.2

[PATCH v2 3/3] crypto-ixp4xx: Less function calls in init_ixp_crypto() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 17 Nov 2015 16:15:21 +0100

The dma_pool_destroy() function was called in up to two cases by the
init_ixp_crypto() function during error handling even if a call of
the dma_pool_create() function failed.

This implementation detail could be improved by the adjustment
of jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/crypto/ixp4xx_crypto.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 79b6958..0a5969c 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -483,23 +483,23 @@ static int init_ixp_crypto(struct device *dev)
 			sizeof(struct buffer_desc), 32, 0);
 	if (!buffer_pool) {
 		ret = -ENOMEM;
-		goto err;
+		goto release_npe;
 	}
 	ctx_pool = dma_pool_create("context", dev,
 			NPE_CTX_LEN, 16, 0);
 	if (!ctx_pool) {
 		ret = -ENOMEM;
-		goto err;
+		goto destroy_buffer_pool;
 	}
 	ret = qmgr_request_queue(SEND_QID, NPE_QLEN_TOTAL, 0, 0,
 				 "ixp_crypto:out", NULL);
 	if (ret)
-		goto err;
+		goto destroy_ctx_pool;
 	ret = qmgr_request_queue(RECV_QID, NPE_QLEN, 0, 0,
 				 "ixp_crypto:in", NULL);
 	if (ret) {
 		qmgr_release_queue(SEND_QID);
-		goto err;
+		goto destroy_ctx_pool;
 	}
 	qmgr_set_irq(RECV_QID, QUEUE_IRQ_SRC_NOT_EMPTY, irqhandler, NULL);
 	tasklet_init(&crypto_done_tasklet, crypto_done_action, 0);
@@ -510,9 +510,11 @@ static int init_ixp_crypto(struct device *dev)
 npe_error:
 	printk(KERN_ERR "%s not responding\n", npe_name(npe_c));
 	ret = -EIO;
-err:
+destroy_ctx_pool:
 	dma_pool_destroy(ctx_pool);
+destroy_buffer_pool:
 	dma_pool_destroy(buffer_pool);
+release_npe:
 	npe_release(npe_c);
 	return ret;
 }
-- 
2.6.2

Re: [PATCH v2 0/3] crypto-ixp4xx: Deletion of a few unnecessary checks

[Herbert Xu]

On Tue, Nov 17, 2015 at 04:32:47PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 17 Nov 2015 16:26:01 +0100
> Subject: [PATCH 0/3] crypto-ixp4xx: Deletion of a few unnecessary checks
> 
> Further update suggestions were taken into account after a patch
> was applied from static source code analysis.
> 
> Markus Elfring (3):
>   Delete unnecessary checks before the function call "dma_pool_destroy"
>   Reduce assignment for a variable in init_ixp_crypto()
>   crypto-ixp4xx: Less function calls in init_ixp_crypto() after error detection

You completely ignored my comments.  Nack.
-- 
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

Re: crypto-ixp4xx: Deletion of a few unnecessary checks

[SF Markus Elfring]

>>   Delete unnecessary checks before the function call "dma_pool_destroy"

It seems that you accepted this specific update suggestion, didn't you?
https://lkml.org/lkml/2015/11/17/391
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1021951.html


>>   Reduce assignment for a variable in init_ixp_crypto()
>>   crypto-ixp4xx: Less function calls in init_ixp_crypto() after error detection
> 
> You completely ignored my comments.  Nack.

Will it be useful to clarify the proposed changes a bit more?

Regards,
Markus