[PATCH 0/2] CW1200: Deletion of an unnecessary check

STALE4279d

4 messages, 2 authors, 2015-02-06 · open the first message on its own page

[PATCH 0/2] CW1200: Deletion of an unnecessary check

From: SF Markus Elfring <hidden>
Date: 2015-02-04 16:46:04

From: Markus Elfring <redacted>
Date: Wed, 4 Feb 2015 17:31:00 +0100

Another update suggestion was taken into account after a patch was applied
from static source code analysis.

Markus Elfring (2):
  Delete an unnecessary check before the function call "release_firmware"
  Less function calls in cw1200_load_firmware_cw1200() after error detection

 drivers/net/wireless/cw1200/fwio.c | 40 +++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 16 deletions(-)

-- 
2.2.2

[PATCH 1/2] CW1200: Delete an unnecessary check before the function call "release_firmware"

From: SF Markus Elfring <hidden>
Date: 2015-02-04 16:47:39

From: Markus Elfring <redacted>
Date: Wed, 4 Feb 2015 16:32:15 +0100

The release_firmware() 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 <redacted>
---
 drivers/net/wireless/cw1200/fwio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/cw1200/fwio.c b/drivers/net/wireless/cw1200/fwio.c
index 6f1b9aa..581dfde 100644
--- a/drivers/net/wireless/cw1200/fwio.c
+++ b/drivers/net/wireless/cw1200/fwio.c
@@ -246,8 +246,7 @@ static int cw1200_load_firmware_cw1200(struct cw1200_common *priv)
 
 error:
 	kfree(buf);
-	if (firmware)
-		release_firmware(firmware);
+	release_firmware(firmware);
 	return ret;
 
 #undef APB_WRITE
-- 
2.2.2

[PATCH 2/2] CW1200: Less function calls in cw1200_load_firmware_cw1200() after error detection

From: SF Markus Elfring <hidden>
Date: 2015-02-04 16:48:52

From: Markus Elfring <redacted>
Date: Wed, 4 Feb 2015 17:28:41 +0100

The functions kfree() and release_firmware() were called in a few cases
by the cw1200_load_firmware_cw1200() function during error handling even if
the passed variables contained still a null pointer.

Corresponding implementation details could be improved by adjustments for
jump targets.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/net/wireless/cw1200/fwio.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/drivers/net/wireless/cw1200/fwio.c b/drivers/net/wireless/cw1200/fwio.c
index 581dfde..30e7646 100644
--- a/drivers/net/wireless/cw1200/fwio.c
+++ b/drivers/net/wireless/cw1200/fwio.c
@@ -66,25 +66,31 @@ static int cw1200_load_firmware_cw1200(struct cw1200_common *priv)
 	do { \
 		ret = cw1200_apb_write_32(priv, CW1200_APB(reg), (val)); \
 		if (ret < 0) \
-			goto error; \
+			goto exit; \
+	} while (0)
+#define APB_WRITE2(reg, val) \
+	do { \
+		ret = cw1200_apb_write_32(priv, CW1200_APB(reg), (val)); \
+		if (ret < 0) \
+			goto free_buffer; \
 	} while (0)
 #define APB_READ(reg, val) \
 	do { \
 		ret = cw1200_apb_read_32(priv, CW1200_APB(reg), &(val)); \
 		if (ret < 0) \
-			goto error; \
+			goto free_buffer; \
 	} while (0)
 #define REG_WRITE(reg, val) \
 	do { \
 		ret = cw1200_reg_write_32(priv, (reg), (val)); \
 		if (ret < 0) \
-			goto error; \
+			goto exit; \
 	} while (0)
 #define REG_READ(reg, val) \
 	do { \
 		ret = cw1200_reg_read_32(priv, (reg), &(val)); \
 		if (ret < 0) \
-			goto error; \
+			goto exit; \
 	} while (0)
 
 	switch (priv->hw_revision) {
@@ -142,14 +148,14 @@ static int cw1200_load_firmware_cw1200(struct cw1200_common *priv)
 	ret = request_firmware(&firmware, fw_path, priv->pdev);
 	if (ret) {
 		pr_err("Can't load firmware file %s.\n", fw_path);
-		goto error;
+		goto exit;
 	}
 
 	buf = kmalloc(DOWNLOAD_BLOCK_SIZE, GFP_KERNEL | GFP_DMA);
 	if (!buf) {
 		pr_err("Can't allocate firmware load buffer.\n");
 		ret = -ENOMEM;
-		goto error;
+		goto firmware_release;
 	}
 
 	/* Check if the bootloader is ready */
@@ -163,7 +169,7 @@ static int cw1200_load_firmware_cw1200(struct cw1200_common *priv)
 	if (val32 != DOWNLOAD_I_AM_HERE) {
 		pr_err("Bootloader is not ready.\n");
 		ret = -ETIMEDOUT;
-		goto error;
+		goto free_buffer;
 	}
 
 	/* Calculcate number of download blocks */
@@ -171,7 +177,7 @@ static int cw1200_load_firmware_cw1200(struct cw1200_common *priv)
 
 	/* Updating the length in Download Ctrl Area */
 	val32 = firmware->size; /* Explicit cast from size_t to u32 */
-	APB_WRITE(DOWNLOAD_IMAGE_SIZE_REG, val32);
+	APB_WRITE2(DOWNLOAD_IMAGE_SIZE_REG, val32);
 
 	/* Firmware downloading loop */
 	for (block = 0; block < num_blocks; block++) {
@@ -183,7 +189,7 @@ static int cw1200_load_firmware_cw1200(struct cw1200_common *priv)
 		if (val32 != DOWNLOAD_PENDING) {
 			pr_err("Bootloader reported error %d.\n", val32);
 			ret = -EIO;
-			goto error;
+			goto free_buffer;
 		}
 
 		/* loop until put - get <= 24K */
@@ -198,7 +204,7 @@ static int cw1200_load_firmware_cw1200(struct cw1200_common *priv)
 		if ((put - get) > (DOWNLOAD_FIFO_SIZE - DOWNLOAD_BLOCK_SIZE)) {
 			pr_err("Timeout waiting for FIFO.\n");
 			ret = -ETIMEDOUT;
-			goto error;
+			goto free_buffer;
 		}
 
 		/* calculate the block size */
@@ -220,12 +226,12 @@ static int cw1200_load_firmware_cw1200(struct cw1200_common *priv)
 		if (ret < 0) {
 			pr_err("Can't write firmware block @ %d!\n",
 			       put & (DOWNLOAD_FIFO_SIZE - 1));
-			goto error;
+			goto free_buffer;
 		}
 
 		/* update the put register */
 		put += block_size;
-		APB_WRITE(DOWNLOAD_PUT_REG, put);
+		APB_WRITE2(DOWNLOAD_PUT_REG, put);
 	} /* End of firmware download loop */
 
 	/* Wait for the download completion */
@@ -238,18 +244,21 @@ static int cw1200_load_firmware_cw1200(struct cw1200_common *priv)
 	if (val32 != DOWNLOAD_SUCCESS) {
 		pr_err("Wait for download completion failed: 0x%.8X\n", val32);
 		ret = -ETIMEDOUT;
-		goto error;
+		goto free_buffer;
 	} else {
 		pr_info("Firmware download completed.\n");
 		ret = 0;
 	}
 
-error:
+free_buffer:
 	kfree(buf);
+firmware_release:
 	release_firmware(firmware);
+exit:
 	return ret;
 
 #undef APB_WRITE
+#undef APB_WRITE2
 #undef APB_READ
 #undef REG_WRITE
 #undef REG_READ
-- 
2.2.2

Re: [1/2] cw1200: Delete an unnecessary check before the function call "release_firmware"

From: Kalle Valo <hidden>
Date: 2015-02-06 06:49:04

From: Markus Elfring <redacted>
Date: Wed, 4 Feb 2015 16:32:15 +0100

The release_firmware() 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 <redacted>
Thanks, 2 patches applied to wireless-drivers-next.git:

df970d39b90e cw1200: Delete an unnecessary check before the function call "release_firmware"
ee4ddad82356 cw1200: Less function calls in cw1200_load_firmware_cw1200() after error detection

Kalle Valo
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help