[PATCH 1/1] net: brcm80211: Deletion of unnecessary checks before two function calls

[PATCH 1/1] net: brcm80211: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 20 Nov 2014 16:42:51 +0100

The functions brcmu_pkt_buf_free_skb() and release_firmware() test whether
their argument is NULL and then return 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>
---
 drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c | 3 +--
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 3 +--
 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c   | 3 +--
 drivers/net/wireless/brcm80211/brcmsmac/main.c     | 3 +--
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
index f55f625..8ff7037 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
@@ -2539,8 +2539,7 @@ static void brcmf_sdio_bus_stop(struct device *dev)
 	brcmu_pktq_flush(&bus->txq, true, NULL, NULL);
 
 	/* Clear any held glomming stuff */
-	if (bus->glomd)
-		brcmu_pkt_buf_free_skb(bus->glomd);
+	brcmu_pkt_buf_free_skb(bus->glomd);
 	brcmf_sdio_free_glom(bus);
 
 	/* Clear rx control and wake any waiters */
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 8ea9f28..3a2d014 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -262,8 +262,7 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 
 fail:
 	brcmf_dbg(TRACE, "failed: dev=%s\n", dev_name(fwctx->dev));
-	if (fwctx->code)
-		release_firmware(fwctx->code);
+	release_firmware(fwctx->code);
 	device_release_driver(fwctx->dev);
 	kfree(fwctx);
 }
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
index 8f8b937..0cb00dc 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
@@ -506,8 +506,7 @@ static int brcmf_msgbuf_query_dcmd(struct brcmf_pub *drvr, int ifidx,
 		memcpy(buf, skb->data, (len < msgbuf->ioctl_resp_ret_len) ?
 				       len : msgbuf->ioctl_resp_ret_len);
 	}
-	if (skb)
-		brcmu_pkt_buf_free_skb(skb);
+	brcmu_pkt_buf_free_skb(skb);
 
 	return msgbuf->ioctl_resp_status;
 }
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
index 1b47482..ce538a1 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
@@ -1009,8 +1009,7 @@ brcms_c_dotxstatus(struct brcms_c_info *wlc, struct tx_status *txs)
 		if (txh)
 			trace_brcms_txdesc(&wlc->hw->d11core->dev, txh,
 					   sizeof(*txh));
-		if (p)
-			brcmu_pkt_buf_free_skb(p);
+		brcmu_pkt_buf_free_skb(p);
 	}
 
 	if (dma && queue < NFIFO) {
-- 
2.1.3

Re: [PATCH 1/1] net: brcm80211: Deletion of unnecessary checks before two function calls

[Arend van Spriel]

On 11/20/14 16:50, SF Markus Elfring wrote:
> From: Markus Elfring<elfring@users.sourceforge.net>
> Date: Thu, 20 Nov 2014 16:42:51 +0100
>
> The functions brcmu_pkt_buf_free_skb() and release_firmware() test whether
> their argument is NULL and then return immediately. Thus the test around
> the call is not needed.
>
> This issue was detected by using the Coccinelle software.

Goodo for coccinelle and you for running it.

Acked-by: Arend van Spriel <arend@broadcom.com>
> Signed-off-by: Markus Elfring<elfring@users.sourceforge.net>
> ---
>   drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c | 3 +--
>   drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 3 +--
>   drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c   | 3 +--
>   drivers/net/wireless/brcm80211/brcmsmac/main.c     | 3 +--
>   4 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
> index f55f625..8ff7037 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
> @@ -2539,8 +2539,7 @@ static void brcmf_sdio_bus_stop(struct device *dev)
>   	brcmu_pktq_flush(&bus->txq, true, NULL, NULL);
>
>   	/* Clear any held glomming stuff */
> -	if (bus->glomd)
> -		brcmu_pkt_buf_free_skb(bus->glomd);
> +	brcmu_pkt_buf_free_skb(bus->glomd);
>   	brcmf_sdio_free_glom(bus);
>
>   	/* Clear rx control and wake any waiters */
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> index 8ea9f28..3a2d014 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> @@ -262,8 +262,7 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>
>   fail:
>   	brcmf_dbg(TRACE, "failed: dev=%s\n", dev_name(fwctx->dev));
> -	if (fwctx->code)
> -		release_firmware(fwctx->code);
> +	release_firmware(fwctx->code);
>   	device_release_driver(fwctx->dev);
>   	kfree(fwctx);
>   }
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> index 8f8b937..0cb00dc 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> @@ -506,8 +506,7 @@ static int brcmf_msgbuf_query_dcmd(struct brcmf_pub *drvr, int ifidx,
>   		memcpy(buf, skb->data, (len<  msgbuf->ioctl_resp_ret_len) ?
>   				       len : msgbuf->ioctl_resp_ret_len);
>   	}
> -	if (skb)
> -		brcmu_pkt_buf_free_skb(skb);
> +	brcmu_pkt_buf_free_skb(skb);
>
>   	return msgbuf->ioctl_resp_status;
>   }
> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> index 1b47482..ce538a1 100644
> --- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
> +++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> @@ -1009,8 +1009,7 @@ brcms_c_dotxstatus(struct brcms_c_info *wlc, struct tx_status *txs)
>   		if (txh)
>   			trace_brcms_txdesc(&wlc->hw->d11core->dev, txh,
>   					   sizeof(*txh));
> -		if (p)
> -			brcmu_pkt_buf_free_skb(p);
> +		brcmu_pkt_buf_free_skb(p);
>   	}
>
>   	if (dma&&  queue<  NFIFO) {

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

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[PATCH] ath9k: Delete an unnecessary check before the function call "relay_close"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 4 Feb 2015 18:48:28 +0100

The relay_close() 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>
---
 drivers/net/wireless/ath/ath9k/common-spectral.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/common-spectral.c b/drivers/net/wireless/ath/ath9k/common-spectral.c
index ec93ddf..5cee231 100644
--- a/drivers/net/wireless/ath/ath9k/common-spectral.c
+++ b/drivers/net/wireless/ath/ath9k/common-spectral.c
@@ -582,7 +582,7 @@ static struct rchan_callbacks rfs_spec_scan_cb = {
 
 void ath9k_cmn_spectral_deinit_debug(struct ath_spec_scan_priv *spec_priv)
 {
-	if (config_enabled(CONFIG_ATH9K_DEBUGFS) && spec_priv->rfs_chan_spec_scan) {
+	if (config_enabled(CONFIG_ATH9K_DEBUGFS)) {
 		relay_close(spec_priv->rfs_chan_spec_scan);
 		spec_priv->rfs_chan_spec_scan = NULL;
 	}
-- 
2.2.2

[PATCH] ath10k: Delete unnecessary checks before the function call "release_firmware"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 4 Feb 2015 19:30:23 +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 <elfring@users.sourceforge.net>
---
 drivers/net/wireless/ath/ath10k/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 2d0671e..45171ad 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -393,16 +393,16 @@ static int ath10k_download_fw(struct ath10k *ar, enum ath10k_firmware_mode mode)
 
 static void ath10k_core_free_firmware_files(struct ath10k *ar)
 {
-	if (ar->board && !IS_ERR(ar->board))
+	if (!IS_ERR(ar->board))
 		release_firmware(ar->board);
 
-	if (ar->otp && !IS_ERR(ar->otp))
+	if (!IS_ERR(ar->otp))
 		release_firmware(ar->otp);
 
-	if (ar->firmware && !IS_ERR(ar->firmware))
+	if (!IS_ERR(ar->firmware))
 		release_firmware(ar->firmware);
 
-	if (ar->cal_file && !IS_ERR(ar->cal_file))
+	if (!IS_ERR(ar->cal_file))
 		release_firmware(ar->cal_file);
 
 	ar->board = NULL;
-- 
2.2.2

[PATCH] orinoco: Delete an unnecessary check before the function call "kfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 4 Feb 2015 19:53:11 +0100

The kfree() 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>
---
 drivers/net/wireless/orinoco/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/orinoco/main.c b/drivers/net/wireless/orinoco/main.c
index 38ec8d1..c410180 100644
--- a/drivers/net/wireless/orinoco/main.c
+++ b/drivers/net/wireless/orinoco/main.c
@@ -2342,7 +2342,7 @@ void free_orinocodev(struct orinoco_private *priv)
 	list_for_each_entry_safe(sd, sdtemp, &priv->scan_list, list) {
 		list_del(&sd->list);
 
-		if ((sd->len > 0) && sd->buf)
+		if (sd->len > 0)
 			kfree(sd->buf);
 		kfree(sd);
 	}
-- 
2.2.2

[PATCH] HostAP: Delete an unnecessary check before the function call "kfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 4 Feb 2015 20:06:39 +0100

The kfree() 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>
---
 drivers/net/wireless/hostap/hostap_ap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/hostap/hostap_ap.c b/drivers/net/wireless/hostap/hostap_ap.c
index 5965255..fd8d83d 100644
--- a/drivers/net/wireless/hostap/hostap_ap.c
+++ b/drivers/net/wireless/hostap/hostap_ap.c
@@ -145,7 +145,7 @@ static void ap_free_sta(struct ap_data *ap, struct sta_info *sta)
 	if (sta->aid > 0)
 		ap->sta_aid[sta->aid - 1] = NULL;
 
-	if (!sta->ap && sta->u.sta.challenge)
+	if (!sta->ap)
 		kfree(sta->u.sta.challenge);
 	del_timer_sync(&sta->timer);
 #endif /* PRISM2_NO_KERNEL_IEEE80211_MGMT */
-- 
2.2.2

[PATCH] net: brcm80211: Delete unnecessary checks before two function calls

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 4 Feb 2015 20:28:49 +0100

The functions brcmu_pkt_buf_free_skb() and usb_free_urb() test whether
their argument is NULL and then return immediately. Thus the test around
the call is not needed.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/brcm80211/brcmfmac/sdio.c | 3 +--
 drivers/net/wireless/brcm80211/brcmfmac/usb.c  | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
index 99a3776..5b5520b 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
@@ -2543,8 +2543,7 @@ static void brcmf_sdio_bus_stop(struct device *dev)
 	brcmu_pktq_flush(&bus->txq, true, NULL, NULL);
 
 	/* Clear any held glomming stuff */
-	if (bus->glomd)
-		brcmu_pkt_buf_free_skb(bus->glomd);
+	brcmu_pkt_buf_free_skb(bus->glomd);
 	brcmf_sdio_free_glom(bus);
 
 	/* Clear rx control and wake any waiters */
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
index 4572def..10c684c 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
@@ -421,7 +421,7 @@ fail:
 	brcmf_err("fail!\n");
 	while (!list_empty(q)) {
 		req = list_entry(q->next, struct brcmf_usbreq, list);
-		if (req && req->urb)
+		if (req)
 			usb_free_urb(req->urb);
 		list_del(q->next);
 	}
-- 
2.2.2

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

[Kalle Valo]

> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>

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

Re: ath9k: Delete an unnecessary check before the function call"relay_close"

[Kalle Valo]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 4 Feb 2015 18:48:28 +0100
> 
> The relay_close() 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>

Thanks, applied to wireless-drivers-next.git.

Kalle Valo

Re: orinoco: Delete an unnecessary check before the function call "kfree"

[Kalle Valo]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 4 Feb 2015 19:53:11 +0100
> 
> The kfree() 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>

Thanks, applied to wireless-drivers-next.git.

Kalle Valo

Re: hostap: Delete an unnecessary check before the function call "kfree"

[Kalle Valo]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 4 Feb 2015 20:06:39 +0100
> 
> The kfree() 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>

Thanks, applied to wireless-drivers-next.git.

Kalle Valo

Re: brcm80211: Delete unnecessary checks before two function calls

[Kalle Valo]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 4 Feb 2015 20:28:49 +0100
> 
> The functions brcmu_pkt_buf_free_skb() and usb_free_urb() test whether
> their argument is NULL and then return immediately. Thus the test around
> the call is not needed.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Thanks, applied to wireless-drivers-next.git.

Kalle Valo

Re: [PATCH] ath10k: Delete unnecessary checks before the function call "release_firmware"

[Kalle Valo]

SF Markus Elfring <elfring@users.sourceforge.net> writes:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 4 Feb 2015 19:30:23 +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 <elfring@users.sourceforge.net>

Thanks, applied to ath.git.

-- 
Kalle Valo

[PATCH 0/2] staging: wilc1000: Deletion of two unnecessary checks

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>

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

Markus Elfring (2):
  Delete unnecessary checks before two function calls
  One function call less in mac_ioctl() after error detection

 drivers/staging/wilc1000/linux_wlan.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

-- 
2.4.4

[PATCH 1/2] staging: wilc1000: Delete unnecessary checks before two function calls

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 27 Jun 2015 15:56:57 +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/staging/wilc1000/linux_wlan.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index b352c50..2aa8d9b 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -2421,11 +2421,7 @@ int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 	}
 
 done:
-
-	if (buff != NULL) {
-		kfree(buff);
-	}
-
+	kfree(buff);
 	return s32Error;
 }
 
@@ -2707,8 +2703,7 @@ static void __exit exit_wilc_driver(void)
 		}
 	}
 
-
-	if ((g_linux_wlan != NULL) && g_linux_wlan->wilc_firmware != NULL) {
+	if (g_linux_wlan) {
 		release_firmware(g_linux_wlan->wilc_firmware);
 		g_linux_wlan->wilc_firmware = NULL;
 	}
-- 
2.4.4

[PATCH 2/2] staging: wilc1000: One function call less in mac_ioctl() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 27 Jun 2015 16:00:59 +0200

The kfree() function was called in two cases by the mac_ioctl() function
during error handling even if the passed variable did not contain a pointer
for a valid data item.

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

* Drop an unnecessary initialisation for the variable "buff" then.

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

diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 2aa8d9b..f492310 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -2351,16 +2351,13 @@ int mac_close(struct net_device *ndev)
 
 int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 {
-
-	u8 *buff = NULL;
+	u8 *buff;
 	s8 rssi;
 	u32 size = 0, length = 0;
 	perInterface_wlan_t *nic;
 	struct WILC_WFI_priv *priv;
 	s32 s32Error = WILC_SUCCESS;
 
-
-
 	/* struct iwreq *wrq = (struct iwreq *) req;	// tony moved to case SIOCSIWPRIV */
 	#ifdef USE_WIRELESS
 	nic = netdev_priv(ndev);
@@ -2405,7 +2402,7 @@ int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 				if (copy_to_user(wrq->u.data.pointer, buff, size)) {
 					PRINT_ER("%s: failed to copy data to user buffer\n", __func__);
 					s32Error = -EFAULT;
-					goto done;
+					goto free_buffer;
 				}
 			}
 		}
@@ -2420,8 +2417,9 @@ int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 	}
 	}
 
-done:
+free_buffer:
 	kfree(buff);
+done:
 	return s32Error;
 }
 
-- 
2.4.4

Re: [PATCH 2/2] staging: wilc1000: One function call less in mac_ioctl() after error detection

[Julia Lawall]

On Sat, 27 Jun 2015, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 27 Jun 2015 16:00:59 +0200
>
> The kfree() function was called in two cases by the mac_ioctl() function
> during error handling even if the passed variable did not contain a pointer
> for a valid data item.
>
> * This implementation detail could be improved by the introduction
>   of another jump label.
>
> * Drop an unnecessary initialisation for the variable "buff" then.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/staging/wilc1000/linux_wlan.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
> index 2aa8d9b..f492310 100644
> --- a/drivers/staging/wilc1000/linux_wlan.c
> +++ b/drivers/staging/wilc1000/linux_wlan.c
> @@ -2351,16 +2351,13 @@ int mac_close(struct net_device *ndev)
>
>  int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
>  {
> -
> -	u8 *buff = NULL;
> +	u8 *buff;
>  	s8 rssi;
>  	u32 size = 0, length = 0;
>  	perInterface_wlan_t *nic;
>  	struct WILC_WFI_priv *priv;
>  	s32 s32Error = WILC_SUCCESS;
>
> -
> -

Removing these blank lines seems to have nothing to do with the topic of
the patch.

julia

>  	/* struct iwreq *wrq = (struct iwreq *) req;	// tony moved to case SIOCSIWPRIV */
>  	#ifdef USE_WIRELESS
>  	nic = netdev_priv(ndev);
> @@ -2405,7 +2402,7 @@ int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
>  				if (copy_to_user(wrq->u.data.pointer, buff, size)) {
>  					PRINT_ER("%s: failed to copy data to user buffer\n", __func__);
>  					s32Error = -EFAULT;
> -					goto done;
> +					goto free_buffer;
>  				}
>  			}
>  		}
> @@ -2420,8 +2417,9 @@ int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
>  	}
>  	}
>
> -done:
> +free_buffer:
>  	kfree(buff);
> +done:
>  	return s32Error;
>  }
>
> --
> 2.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/2] staging: wilc1000: Delete unnecessary checks before two function calls

[Greg Kroah-Hartman]

On Sat, Jun 27, 2015 at 04:36:14PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 27 Jun 2015 15:56:57 +0200

Why is this in the body of the email?  Please fix your email client or
just use git send-email properly.

Re: [PATCH 2/2] staging: wilc1000: One function call less in mac_ioctl() after error detection

[Greg Kroah-Hartman]

On Sat, Jun 27, 2015 at 04:37:24PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 27 Jun 2015 16:00:59 +0200

Again, please fix.

> 
> The kfree() function was called in two cases by the mac_ioctl() function
> during error handling even if the passed variable did not contain a pointer
> for a valid data item.
> 
> * This implementation detail could be improved by the introduction
>   of another jump label.
> 
> * Drop an unnecessary initialisation for the variable "buff" then.

That's a different issue, please fix it in a separate patch.

thanks,

greg k-h

Re: Clarification for the use of additional fields in the message body

[SF Markus Elfring]

>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Sat, 27 Jun 2015 15:56:57 +0200
> 
> Why is this in the body of the email?

Does the canonical patch format support to preserve
specific details about a shown commit by specification
of fields like "Date" and "From" in the message body?

Regards,
Markus

Re: Clarification for the use of additional fields in the message body

[Frans Klaver]

On Tue, Jul 7, 2015 at 8:21 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>> Date: Sat, 27 Jun 2015 15:56:57 +0200
>>
>> Why is this in the body of the email?
>
> Does the canonical patch format support to preserve
> specific details about a shown commit by specification
> of fields like "Date" and "From" in the message body?

Using "From:" in the body can be used to provide your properly spelled
name, or the name/e-mail of the actual author, if that happens not to
be the sender. git-send-email will do that automagically for you. If
"From:" is not specified in the message body, the e-mails sender will
be used as author.

The date, as far as I know, is ignored. It is the commit date, not the
authoring date, and once your patch is applied by a maintainer (i.e.
committed), the date gets reset anyway. No need to try and preserve
it.

Frans

Re: Clarification for the use of additional fields in the message body

[SF Markus Elfring]

> The date, as far as I know, is ignored. It is the commit date,
> not the authoring date, and once your patch is applied by a maintainer
> (i.e. committed), the date gets reset anyway.

Thanks for your feedback.


> No need to try and preserve it.

I find that it might occasionally help to share and keep the record
on timestamps about the evolution for an original update suggestion.

Regards,
Markus

Re: Clarification for the use of additional fields in the message body

[Frans Klaver]

On Tue, Jul 7, 2015 at 9:54 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:

>> No need to try and preserve it.
>
> I find that it might occasionally help to share and keep the record
> on timestamps about the evolution for an original update suggestion.

I think that as far as these kernel mailing lists are concerned, the
date of the update suggestion is the date on which you submitted the
patch, rather than the date you originally committed it to your local
tree. If you wish to keep track of this evolution for yourself, or
wish to share it, you're better off stashing it somewhere in a
(public) git repo that you control. If you wish, you can always make
mention of that repo below the ---, just above the diffstat. If you
insist on placing the date somewhere, you can also put the date there
if you wish. It'll be ignored by git when applied.

Cheers,
Frans

Re: Clarification for the use of additional fields in the message body

[SF Markus Elfring]

> I think that as far as these kernel mailing lists are concerned,
> the date of the update suggestion is the date on which you submitted the patch,
> rather than the date you originally committed it to your local tree.

I imagine that there are committers who would like to keep
corresponding software development history a bit more accurate.


> If you wish to keep track of this evolution for yourself, or
> wish to share it, you're better off stashing it somewhere in a
> (public) git repo that you control.

Would it be nicer to preserve such data directly also
by the usual mail interface?


> If you insist on placing the date somewhere, you can also put the date
> there if you wish. It'll be ignored by git when applied.

This content management tool provides the capability to store
the discussed information by the parameters "--author=" and "--date=",
doesn't it?
Is the environment variable "GIT_AUTHOR_DATE" also interesting occasionally?

How often do you take extra care for passing appropriate data there?

Regards,
Markus

Re: Clarification for the use of additional fields in the message body

[Frans Klaver]

On Tue, Jul 7, 2015 at 1:53 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> I think that as far as these kernel mailing lists are concerned,
>> the date of the update suggestion is the date on which you submitted the patch,
>> rather than the date you originally committed it to your local tree.
>
> I imagine that there are committers who would like to keep
> corresponding software development history a bit more accurate.

I guess it depends on what your view on accurate is.


>> If you wish to keep track of this evolution for yourself, or
>> wish to share it, you're better off stashing it somewhere in a
>> (public) git repo that you control.
>
> Would it be nicer to preserve such data directly also
> by the usual mail interface?
>
>
>> If you insist on placing the date somewhere, you can also put the date
>> there if you wish. It'll be ignored by git when applied.
>
> This content management tool provides the capability to store
> the discussed information by the parameters "--author=" and "--date=",
> doesn't it?
> Is the environment variable "GIT_AUTHOR_DATE" also interesting occasionally?
>
> How often do you take extra care for passing appropriate data there?

I can't remember ever changing or explicitly preserving the commit
date. I don't think I care enough. I did change the author on botched
patches, but that's an exception. Remembering the author separately
from the committer is something git does by design anyway.

Frans

Re: Clarification for the use of additional fields in the message body

[SF Markus Elfring]

> I can't remember ever changing or explicitly preserving the commit date.
> I don't think I care enough.

Would any more software developers and maintainers like to share
their experiences around such details?

When do commit timestamps become relevant as a documentation item
for contribution authorship?


> Remembering the author separately from the committer is something
> git does by design anyway.

Do you usually just reuse a procedure from a well-known command
for which a description is provided like the following?
http://git-scm.com/docs/git-am
'…
"From: " and "Subject: " lines starting the body override
the respective commit author name and title values
taken from the headers.
…'

Will further fields be eventually mentioned there?

Regards,
Markus

Re: Clarification for the use of additional fields in the message body

[Julian Calaby]

Hi Markus,

On Wed, Jul 8, 2015 at 2:15 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> I can't remember ever changing or explicitly preserving the commit date.
>> I don't think I care enough.
>
> Would any more software developers and maintainers like to share
> their experiences around such details?
>
> When do commit timestamps become relevant as a documentation item
> for contribution authorship?

They are never relevant.

"When" a commit happened is never relevant except in relation to other
things, at which point the actual date and time is almost completely
irrelevant.

Just submit your patches using git-format-patch or git-send-email and
friends. There's a file in the documentation directory of the kernel
tree describing submitting patches and email client setup. Read them
both, do what they say without anything extra.

>> Remembering the author separately from the committer is something
>> git does by design anyway.
>
> Do you usually just reuse a procedure from a well-known command
> for which a description is provided like the following?
> http://git-scm.com/docs/git-am
> '…
> "From: " and "Subject: " lines starting the body override
> the respective commit author name and title values
> taken from the headers.
> …'
>
> Will further fields be eventually mentioned there?

Why? Just do what is described in SubmittingPatches. Your attempts to
"improve" on the system are unnecessary and annoying people. The
instructions there are the recommended way to do things for a reason.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: Clarification for the use of additional fields in the message body

[SF Markus Elfring]

> There's a file in the documentation directory of the kernel
> tree describing submitting patches and email client setup.
> Read them both,

I read this information several times.


> do what they say without anything extra.

Do you see any special consequences if a bit of "extra" functionality
is already provided by the Git software for a while?


> Your attempts to "improve" on the system are unnecessary

It seems that my approach does not need improvements for the current
command "git am".
Would a few extensions for the available documentation help to clarify
the situation?


Do items like "commit mail address" and "commit timestamp"
belong together for the data structure "author" by design
in this content management system?


> and annoying people.

I understand that various update suggestions can be surprising.
It is also usual that corresponding acceptance might take
a bit longer than what some contributors would prefer.

Regards,
Markus

Re: Clarification for the use of additional fields in the message body

[Julian Calaby]

Hi Markus,

On Wed, Jul 8, 2015 at 5:09 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> There's a file in the documentation directory of the kernel
>> tree describing submitting patches and email client setup.
>> Read them both,
>
> I read this information several times.
>
>
>> do what they say without anything extra.
>
> Do you see any special consequences if a bit of "extra" functionality
> is already provided by the Git software for a while?

If it's harmless, then no, but in this case, people are questioning
why you're adding it as it adds no value to anyone and makes it look
like you don't know what you're doing.

>> Your attempts to "improve" on the system are unnecessary
>
> It seems that my approach does not need improvements for the current
> command "git am".
> Would a few extensions for the available documentation help to clarify
> the situation?

The issue is that the headers you're adding, From: and Date: are unnecessary.

The From: header you add is unnecessary as your email's From: header
has the exact same information. The reason it's there is because
sometimes people forward patches on from other people, e.g. if I were
to resend one of your patches, I'd add a From: header to the body of
the email so it'd be credited to you.

The Date: header you add is unnecessary as git-format-patch sets the
date header in the email it produces to the author date stored in the
commit. (see below) So if you're sending your patches in emails
produced by git-format-patch, there's absolutely no reason to include
it.

> Do items like "commit mail address" and "commit timestamp"
> belong together for the data structure "author" by design
> in this content management system?

The information stored for a commit is:

= = = = = =

tree 09496defc9eb793c665a7b80aa22f24c7bd5f204
parent 63c07589832bfe5ec49f2523ddb0e94a20af0f31
author Julian Calaby <julian.calaby@gmail.com> 1435196810 +1000
committer Julian Calaby <julian.calaby@gmail.com> 1436322540 +1000

= = = = = =

Then the subject and commit message.

The numbers after the email addresses are the timestamps. They are
both almost completely irrelevant for most workflows as people are
less interested in when a commit was made and more interested in what
release it's in, how it was merged, etc. All of which should be
determined without using the timestamp.

To be honest, I've only ever used that timestamp for reporting
purposes at work, and I'd be surprised if anyone was doing anything
other than that with them.

In short, nobody cares, and nobody's going to be upset if the actual
time you authored a patch is different to the time recorded upstream.

>> and annoying people.
>
> I understand that various update suggestions can be surprising.
> It is also usual that corresponding acceptance might take
> a bit longer than what some contributors would prefer.

How would you feel if someone came in to your place of work and told
you to change how you do the job you've been doing for years without a
good reason?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: staging: wilc1000: One function call less in mac_ioctl() after error detection

[SF Markus Elfring]

>> The kfree() function was called in two cases by the mac_ioctl() function
>> during error handling even if the passed variable did not contain a pointer
>> for a valid data item.
>>
>> * This implementation detail could be improved by the introduction
>>   of another jump label.
>>
>> * Drop an unnecessary initialisation for the variable "buff" then.
> 
> That's a different issue, please fix it in a separate patch.

I have got an other opinion here.
This update suggestion depends on a small source code clean-up
from the previous patch, doesn't it?

Do you want that I split the second suggestion from this series
into more steps?

Regards,
Markus

Re: Clarification for the use of additional fields in the message body

[SF Markus Elfring]

> If it's harmless, then no, but in this case, people are questioning
> why you're adding it as it adds no value

Some Git software developers care to keep the information complete
for the author commit.


> to anyone and makes it look like you don't know what you're doing.

I specify message field overrides in my update suggestions intentionally.


> The issue is that the headers you're adding, From: and Date: are unnecessary.

We have got different opinions about the purpose.


> The From: header you add is unnecessary as your email's From: header
> has the exact same information.

I would like to point out that there is a slight difference in my use case.


> The reason it's there is because sometimes people forward patches on
> from other people, e.g. if I were to resend one of your patches,
> I'd add a From: header to the body of the email so it'd be credited to you.

I am also interested in such an use case.


> The Date: header you add is unnecessary as git-format-patch sets the
> date header in the email it produces to the author date stored in the commit.

How do you think about my extra patch preparation for the mentioned
mail forwarding?


> So if you're sending your patches in emails produced by git-format-patch,
> there's absolutely no reason to include it.

I disagree here to some degree.

The difference in suggested commit timestamps of a few minutes might look
negligible for some patches. There are few occasions where the delay between
a concrete commit and its publishing by an interface like email
can become days.


> They are both almost completely irrelevant for most workflows as people
> are less interested in when a commit was made and more interested in what
> release it's in, how it was merged, etc. All of which should be
> determined without using the timestamp.

How often will it matter who made and published a change first?


> To be honest, I've only ever used that timestamp for reporting
> purposes at work, and I'd be surprised if anyone was doing anything
> other than that with them.

Thanks for your detailed feedback.


> How would you feel if someone came in to your place of work
> and told you to change how you do the job you've been doing for years
> without a good reason?

You might feel uncomfortable for a moment if you would interpret
such a suggestion as a personal attack.

I guess that I point only a few technical details out which can change
the popularity of existing functionality from the Git software.

Regards,
Markus

Re: Clarification for the use of additional fields in the message body

[Julian Calaby]

Hi Markus,

On Wed, Jul 8, 2015 at 7:28 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> If it's harmless, then no, but in this case, people are questioning
>> why you're adding it as it adds no value
>
> Some Git software developers care to keep the information complete
> for the author commit.
>
>
>> to anyone and makes it look like you don't know what you're doing.
>
> I specify message field overrides in my update suggestions intentionally.
>
>
>> The issue is that the headers you're adding, From: and Date: are unnecessary.
>
> We have got different opinions about the purpose.

Let me rephrase that: they _should_ be unnecessary.

>> The From: header you add is unnecessary as your email's From: header
>> has the exact same information.
>
> I would like to point out that there is a slight difference in my use case.

Then fix your email client or patch submission process.

>> The reason it's there is because sometimes people forward patches on
>> from other people, e.g. if I were to resend one of your patches,
>> I'd add a From: header to the body of the email so it'd be credited to you.
>
> I am also interested in such an use case.

This happens automatically if you use git-format-patch on a commit you
didn't author. Nobody is adding this manually.

>> The Date: header you add is unnecessary as git-format-patch sets the
>> date header in the email it produces to the author date stored in the commit.
>
> How do you think about my extra patch preparation for the mentioned
> mail forwarding?

It sounds like you're using what could only be described as a
Rube-Goldberg process to send email. Is there truly no way to simplify
that process? (Also, are the servers you send it through re-writing
the original headers?)

You should be sending the patches directly with SMTP using
git-send-email, if you're not, then you're making things overly
complicated for yourself.

>> So if you're sending your patches in emails produced by git-format-patch,
>> there's absolutely no reason to include it.
>
> I disagree here to some degree.
>
> The difference in suggested commit timestamps of a few minutes might look
> negligible for some patches. There are few occasions where the delay between
> a concrete commit and its publishing by an interface like email
> can become days.

I made a commit a month ago, it got rebased today, in fact, I sent
it's metadata in my previous email. When I ran git-format-patch on it,
the timestamp in the headers of the email produced was the date from a
month ago.

If I then sent that email, I believe it'd make it to the list here
with that date intact. If it hadn't, I'd be trying to figure out why
and either fix it or find a different path to get my patch here.

>> They are both almost completely irrelevant for most workflows as people
>> are less interested in when a commit was made and more interested in what
>> release it's in, how it was merged, etc. All of which should be
>> determined without using the timestamp.
>
> How often will it matter who made and published a change first?

If multiple people are submitting identical changes, then the one that
is applied is the one the maintainer sees first, which will most
likely be determined by which one hit their inbox / list first. Nobody
is going to look at timestamps in emails to determine which one will
be applied.

If you're worried about which one of several versions of a patch will
be applied, change the subject to [PATCH v2] ..... instead of [PATCH]
.... for the second version.

>> To be honest, I've only ever used that timestamp for reporting
>> purposes at work, and I'd be surprised if anyone was doing anything
>> other than that with them.
>
> Thanks for your detailed feedback.
>
>
>> How would you feel if someone came in to your place of work
>> and told you to change how you do the job you've been doing for years
>> without a good reason?
>
> You might feel uncomfortable for a moment if you would interpret
> such a suggestion as a personal attack.

I'm not interpreting this as a personal attack.

> I guess that I point only a few technical details out which can change
> the popularity of existing functionality from the Git software.

Git was originally written to manage the Linux kernel. This feature
was probably added by either Linus himself or one of the original
developers who I believe were kernel developers. The patch submission
process has been around for a long time, if this feature isn't used,
it's for a good reason.

Having a feature doesn't mean that it should be used.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: Clarification for the use of additional fields in the message body

[SF Markus Elfring]

> Is there truly no way to simplify that process?

I see some software development possibilities which could improve
the communication with high volume mailing lists.


> You should be sending the patches directly with SMTP using git-send-email,

This tool is also fine for the publishing of a lot of patches.


> if you're not, then you're making things overly complicated for yourself.

But I prefer a graphical user interface for my mail handling so far.


> Having a feature doesn't mean that it should be used.

Does any of the "questionable functionality" get occasionally overlooked
a bit too often?

Regards,
Markus

Re: Clarification for the use of additional fields in the message body

[Theodore Ts'o]

On Wed, Jul 08, 2015 at 09:05:53PM +1000, Julian Calaby wrote:
> If multiple people are submitting identical changes, then the one that
> is applied is the one the maintainer sees first, which will most
> likely be determined by which one hit their inbox / list first. Nobody
> is going to look at timestamps in emails to determine which one will
> be applied.

And some maintainers may choose *not* to act on a patch first, even if
they see it first.  They might be focused on bug fix patches, and not
act on cleanup or feature patches until -rc3 or rc4.  Or maybe they
will use separate branches for "urgent_for_linus" patches, so two
different patchs may end up in completely different git flows.

> If you're worried about which one of several versions of a patch will
> be applied, change the subject to [PATCH v2] ..... instead of [PATCH]
> .... for the second version.

*Please* do this.  In fact, this is the one thing I wish git
send-email would do automatically, along with having a place to edit
and track the 0/N summary patch.

> >> To be honest, I've only ever used that timestamp for reporting
> >> purposes at work, and I'd be surprised if anyone was doing anything
> >> other than that with them.
> >
> > Thanks for your detailed feedback.

Note also that some maintainers have work flow that deliberately smash
the date (i.e., because they are using a system such as guilt), so if
you are depending on the submitted timestamp, it's going to break on
you.

							- Ted

Re: Clarification for the use of additional fields in the message body

[SF Markus Elfring]

> Note also that some maintainers have work flow that deliberately smash
> the date (i.e., because they are using a system such as guilt),
> so if you are depending on the submitted timestamp, it's going to
> break on you.

Thanks for your hint.

I am just trying to offer the possibility for the reuse of a more
precise commit timestamp together with an appropriate author mail
address for my update suggestions.
Do you reject any more such message field overrides?

Regards,
Markus

Re: Clarification for the use of additional fields in the message body

[Julian Calaby]

Hi Markus,

On Wed, Jul 8, 2015 at 11:46 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> Is there truly no way to simplify that process?
>
> I see some software development possibilities which could improve
> the communication with high volume mailing lists.

You shouldn't need any software development, most people's processes are:

1. Make changes
2. git add <files>
3. git commit
4. Repeat 1-3 for all changes they want to make
5. git-format-patch
6. git-send-email through some SMTP server to the list and appropriate
other people

>From what I've read, you appear to have some semi-automatic tool for
steps 1 through 4. I'd strongly recommend changing your patch
submission process to use git-format-patch and git-send-email. If
Sourceforge's email system doesn't let you send emails with SMTP
directly, then you might want to try sending emails through your ISP's
mail server.

Maintainers use a very similar process, however they collect and
review patches in some personal repository in addition to making
changes and committing them. Tools like patchwork are simply fancy
methods of accumulating patches into git trees. Most people are using
git-format-patch and git-send-email, possibly with some scripting
around them, to format and send patches.

>> You should be sending the patches directly with SMTP using git-send-email,
>
> This tool is also fine for the publishing of a lot of patches.

git-send-email or some other tool?

>> if you're not, then you're making things overly complicated for yourself.
>
> But I prefer a graphical user interface for my mail handling so far.

I send my patches using git-send-email through gmail's servers then
deal with literally everything else through gmail's web interface or
Android app. Using git-send-email doesn't mean you can't use a
graphical mail interface.

I used to send patches through a carefully configured instance of
Thunderbird, however this required too many steps and it loved to
mangle patches in ways that annoyed people.

>> Having a feature doesn't mean that it should be used.
>
> Does any of the "questionable functionality" get occasionally overlooked
> a bit too often?

It's not "questionable functionality", it's functionality we don't see
a need for.

If I wanted to, I could insert a patch at any point in the history of
Linux in my own repository, however any attempt to push that upstream
would cause enormous amounts of pain and annoyance, to everyone who
attempted to deal with it, so I don't.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: Clarification for the use of additional fields in the message body

[Theodore Ts'o]

On Wed, Jul 08, 2015 at 05:27:38PM +0200, SF Markus Elfring wrote:
> > Note also that some maintainers have work flow that deliberately smash
> > the date (i.e., because they are using a system such as guilt),
> > so if you are depending on the submitted timestamp, it's going to
> > break on you.
> 
> Thanks for your hint.
> 
> I am just trying to offer the possibility for the reuse of a more
> precise commit timestamp together with an appropriate author mail
> address for my update suggestions.
> Do you reject any more such message field overrides?

Well, I won't hold it against you, since I also often need to fix
patches or git commit descriptions anyway.  But at the same time, my
workflow (which you have no right to dictate) **will** destroy your
timestamp.  Given that you haven't explained why you want to do this,
I'm not going have much sympathy if you complain.

Personally, given that you're going through some extremely baroque
e-mail/patchsubmission scheme, I don't understand why you can't also
arrange to override the Date field in the e-mail header.  But I don't
really care all that much, because I can ignore your timestamp no
matter where you put it.

Regards,

					- Ted

[PATCH] net: brcm80211: Delete an unnecessary check before the function call "release_firmware"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 6 Nov 2015 08:48:23 +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 <elfring@users.sourceforge.net>
---
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 4248f3c..33afb9a 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -449,8 +449,7 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 
 	if (raw_nvram)
 		bcm47xx_nvram_release_contents(data);
-	if (fw)
-		release_firmware(fw);
+	release_firmware(fw);
 	if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
 		goto fail;
 
-- 
2.6.2

Re: [PATCH] net: brcm80211: Delete an unnecessary check before the function call "release_firmware"

[Arend van Spriel]

On 11/06/2015 08:58 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 6 Nov 2015 08:48:23 +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.

Acked-by: Arend van Spriel <arend@broadcom.com>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> index 4248f3c..33afb9a 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> @@ -449,8 +449,7 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>
>   	if (raw_nvram)
>   		bcm47xx_nvram_release_contents(data);
> -	if (fw)
> -		release_firmware(fw);
> +	release_firmware(fw);
>   	if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>   		goto fail;
>
>

[PATCH] NFC-nci: Delete unnecessary checks before the function call "kfree_skb"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 14 Nov 2015 22:42:48 +0100

The kfree_skb() 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>
---
 net/nfc/nci/uart.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
index 21d8875..106ecc1 100644
--- a/net/nfc/nci/uart.c
+++ b/net/nfc/nci/uart.c
@@ -199,11 +199,8 @@ static void nci_uart_tty_close(struct tty_struct *tty)
 	if (!nu)
 		return;
 
-	if (nu->tx_skb)
-		kfree_skb(nu->tx_skb);
-	if (nu->rx_skb)
-		kfree_skb(nu->rx_skb);
-
+	kfree_skb(nu->tx_skb);
+	kfree_skb(nu->rx_skb);
 	skb_queue_purge(&nu->tx_q);
 
 	nu->ops.close(nu);
-- 
2.6.2

[PATCH] rtlwifi: Delete unnecessary checks before the function call "kfree_skb"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 16 Nov 2015 13:12:25 +0100

The kfree_skb() 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/net/wireless/realtek/rtlwifi/core.c                 | 3 +--
 drivers/net/wireless/realtek/rtlwifi/rtl8723com/fw_common.c | 4 +---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c
index c925a4d..4ae421e 100644
--- a/drivers/net/wireless/realtek/rtlwifi/core.c
+++ b/drivers/net/wireless/realtek/rtlwifi/core.c
@@ -1833,8 +1833,7 @@ bool rtl_cmd_send_packet(struct ieee80211_hw *hw, struct sk_buff *skb)
 
 	spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
 	pskb = __skb_dequeue(&ring->queue);
-	if (pskb)
-		kfree_skb(pskb);
+	kfree_skb(pskb);
 
 	/*this is wrong, fill_tx_cmddesc needs update*/
 	pdesc = &ring->desc[0];
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723com/fw_common.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723com/fw_common.c
index a2f5e89..6e51862 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723com/fw_common.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723com/fw_common.c
@@ -318,9 +318,7 @@ bool rtl8723_cmd_send_packet(struct ieee80211_hw *hw,
 	ring = &rtlpci->tx_ring[BEACON_QUEUE];
 
 	pskb = __skb_dequeue(&ring->queue);
-	if (pskb)
-		kfree_skb(pskb);
-
+	kfree_skb(pskb);
 	spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
 
 	pdesc = &ring->desc[0];
-- 
2.6.2

Re: brcm80211: Delete an unnecessary check before the function call"release_firmware"

[Kalle Valo]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 6 Nov 2015 08:48:23 +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 <elfring@users.sourceforge.net>
> Acked-by: Arend van Spriel <arend@broadcom.com>

Thanks, applied to wireless-drivers-next.git.

Kalle Valo

Re: rtlwifi: Delete unnecessary checks before the function call"kfree_skb"

[Kalle Valo]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 16 Nov 2015 13:12:25 +0100
> 
> The kfree_skb() 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>

Thanks, applied to wireless-drivers-next.git.

Kalle Valo

[PATCH 0/3] staging: wilc1000: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>

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

Markus Elfring (3):
  Delete an unnecessary check before the function call "release_firmware"
  One function call less in mac_ioctl() after error detection
  Reduce scope for a few variables in mac_ioctl()

 drivers/staging/wilc1000/linux_wlan.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

-- 
2.9.2

[PATCH 1/3] staging: wilc1000: Delete an unnecessary check before the function call "release_firmware"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 24 Jul 2016 21:00:20 +0200

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 <elfring@users.sourceforge.net>
---
 drivers/staging/wilc1000/linux_wlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 3a66255..cdef645 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1223,7 +1223,7 @@ void wilc_netdev_cleanup(struct wilc *wilc)
 			vif[i] = netdev_priv(wilc->vif[i]->ndev);
 	}
 
-	if (wilc && wilc->firmware) {
+	if (wilc) {
 		release_firmware(wilc->firmware);
 		wilc->firmware = NULL;
 	}
-- 
2.9.2

[PATCH 2/3] staging: wilc1000: One function call less in mac_ioctl() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 24 Jul 2016 21:15:23 +0200

The kfree() function was called in two cases by the mac_ioctl() function
during error handling even if the passed variable did not contain a pointer
for a valid data item.

Improve this implementation detail by the introduction of another
jump label.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/wilc1000/linux_wlan.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index cdef645..7b1ebcc 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1130,7 +1130,7 @@ static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 				if (copy_to_user(wrq->u.data.pointer, buff, size)) {
 					netdev_err(ndev, "failed to copy\n");
 					ret = -EFAULT;
-					goto done;
+					goto free_buffer;
 				}
 			}
 		}
@@ -1144,11 +1144,9 @@ static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 		goto done;
 	}
 	}
-
-done:
-
+free_buffer:
 	kfree(buff);
-
+done:
 	return ret;
 }
 
-- 
2.9.2

[PATCH 3/3] staging: wilc1000: Reduce scope for a few variables in mac_ioctl()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 24 Jul 2016 21:45:37 +0200

Three local variables were used only within a single case branch.

* Thus move the data type definition for "rssi" and "size" into the
  corresponding code block.

* The variable "length" was not modified after its initialisation.
  Thus pass a constant value in the affected function call instead.

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

diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 7b1ebcc..173be16 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1094,8 +1094,6 @@ int wilc_mac_close(struct net_device *ndev)
 static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 {
 	u8 *buff = NULL;
-	s8 rssi;
-	u32 size = 0, length = 0;
 	struct wilc_vif *vif;
 	s32 ret = 0;
 	struct wilc *wilc;
@@ -1110,8 +1108,7 @@ static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 	case SIOCSIWPRIV:
 	{
 		struct iwreq *wrq = (struct iwreq *)req;
-
-		size = wrq->u.data.length;
+		u32 size = wrq->u.data.length;
 
 		if (size && wrq->u.data.pointer) {
 			buff = memdup_user(wrq->u.data.pointer,
@@ -1119,7 +1116,9 @@ static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 			if (IS_ERR(buff))
 				return PTR_ERR(buff);
 
-			if (strncasecmp(buff, "RSSI", length) == 0) {
+			if (strncasecmp(buff, "RSSI", 0) == 0) {
+				s8 rssi;
+
 				ret = wilc_get_rssi(vif, &rssi);
 				netdev_info(ndev, "RSSI :%d\n", rssi);
 
-- 
2.9.2

Re: [PATCH 2/3] staging: wilc1000: One function call less in mac_ioctl() after error detection

[Julian Calaby]

Hi Marcus,

On Mon, Jul 25, 2016 at 6:22 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 24 Jul 2016 21:15:23 +0200
>
> The kfree() function was called in two cases by the mac_ioctl() function
> during error handling even if the passed variable did not contain a pointer
> for a valid data item.
>
> Improve this implementation detail by the introduction of another
> jump label.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

This is pointless micro-optimisation: kfree(NULL) is perfectly valid
and buff is either malloc'd memory or NULL when it's called.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/