[PATCH 0/2] net-ath9k_htc: Fine-tuning for two function implementations

[PATCH 0/2] net-ath9k_htc: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 19:16:05 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Delete an unnecessary variable initialisation in ath9k_hif_usb_rx_stream()
  Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

 drivers/net/wireless/ath/ath9k/hif_usb.c      | 2 +-
 drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-----
 2 files changed, 3 insertions(+), 6 deletions(-)

-- 
2.6.3

[PATCH 1/2] net-ath9k_htc: Delete an unnecessary variable initialisation in ath9k_hif_usb_rx_stream()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 19:00:53 +0100

Omit explicit initialisation at the beginning for one local variable
that is redefined before its first use.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/ath/ath9k/hif_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index 165dd20..51bd61b 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -525,7 +525,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
 				    struct sk_buff *skb)
 {
 	struct sk_buff *nskb, *skb_pool[MAX_PKT_NUM_IN_TRANSFER];
-	int index = 0, i = 0, len = skb->len;
+	int index = 0, i, len = skb->len;
 	int rx_remain_len, rx_pkt_len;
 	u16 pool_index = 0;
 	u8 *ptr;
-- 
2.6.3

[PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 19:09:32 +0100

Replace an explicit initialisation for one local variable at the beginning
by a conditional assignment.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index a680a97..30bd59e 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -246,7 +246,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv *priv,
 	struct ieee80211_conf *conf = &common->hw->conf;
 	bool fastcc;
 	struct ieee80211_channel *channel = hw->conf.chandef.chan;
-	struct ath9k_hw_cal_data *caldata = NULL;
+	struct ath9k_hw_cal_data *caldata;
 	enum htc_phymode mode;
 	__be16 htc_mode;
 	u8 cmd_rsp;
@@ -274,10 +274,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv *priv,
 		priv->ah->curchan->channel,
 		channel->center_freq, conf_is_ht(conf), conf_is_ht40(conf),
 		fastcc);
-
-	if (!fastcc)
-		caldata = &priv->caldata;
-
+	caldata = fastcc ? NULL : &priv->caldata;
 	ret = ath9k_hw_reset(ah, hchan, caldata, fastcc);
 	if (ret) {
 		ath_err(common,
-- 
2.6.3

Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

Am 01.01.2016 um 19:25 schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 19:09:32 +0100
> 
> Replace an explicit initialisation for one local variable at the beginning
> by a conditional assignment.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> index a680a97..30bd59e 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> @@ -246,7 +246,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv *priv,
>  	struct ieee80211_conf *conf = &common->hw->conf;
>  	bool fastcc;
>  	struct ieee80211_channel *channel = hw->conf.chandef.chan;
> -	struct ath9k_hw_cal_data *caldata = NULL;
> +	struct ath9k_hw_cal_data *caldata;
>  	enum htc_phymode mode;
>  	__be16 htc_mode;
>  	u8 cmd_rsp;
> @@ -274,10 +274,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv *priv,
>  		priv->ah->curchan->channel,
>  		channel->center_freq, conf_is_ht(conf), conf_is_ht40(conf),
>  		fastcc);
> -
> -	if (!fastcc)
> -		caldata = &priv->caldata;
> -
> +	caldata = fastcc ? NULL : &priv->caldata;
>  	ret = ath9k_hw_reset(ah, hchan, caldata, fastcc);
>  	if (ret) {
>  		ath_err(common,
> 

Reviewed-by: Oleksij Rempel <linux@rempel-privat.de>

-- 
Regards,
Oleksij


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH 1/2] net-ath9k_htc: Delete an unnecessary variable initialisation in ath9k_hif_usb_rx_stream()

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]

Am 01.01.2016 um 19:23 schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 19:00:53 +0100
> 
> Omit explicit initialisation at the beginning for one local variable
> that is redefined before its first use.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/ath/ath9k/hif_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
> index 165dd20..51bd61b 100644
> --- a/drivers/net/wireless/ath/ath9k/hif_usb.c
> +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
> @@ -525,7 +525,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
>  				    struct sk_buff *skb)
>  {
>  	struct sk_buff *nskb, *skb_pool[MAX_PKT_NUM_IN_TRANSFER];
> -	int index = 0, i = 0, len = skb->len;
> +	int index = 0, i, len = skb->len;
>  	int rx_remain_len, rx_pkt_len;
>  	u16 pool_index = 0;
>  	u8 *ptr;
> 


Reviewed-by: Oleksij Rempel <linux@rempel-privat.de>

-- 
Regards,
Oleksij


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

[PATCH] net-brcmfmac: Delete an unnecessary variable initialisation in brcmf_sdio_download_firmware()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 20:20:15 +0100

Omit explicit initialisation at the beginning for one local variable
that is redefined before its first use.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index ceb2a75..c21eeb1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3260,7 +3260,7 @@ static int brcmf_sdio_download_firmware(struct brcmf_sdio *bus,
 					const struct firmware *fw,
 					void *nvram, u32 nvlen)
 {
-	int bcmerror = -EFAULT;
+	int bcmerror;
 	u32 rstvec;
 
 	sdio_claim_host(bus->sdiodev->func[1]);
-- 
2.6.3

[PATCH 0/3] net-iwlegacy: Fine-tuning for il_eeprom_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 21:25:43 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Refactoring
  One check less after error detection
  Another refactoring

 drivers/net/wireless/intel/iwlegacy/common.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

-- 
2.6.3

[PATCH 1/3] net-iwlegacy: Refactoring for il_eeprom_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 20:54:25 +0100

Return directly if a memory allocation failed at the beginning.

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

diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index eb5cb60..c3afaf7 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -723,10 +723,9 @@ il_eeprom_init(struct il_priv *il)
 	sz = il->cfg->eeprom_size;
 	D_EEPROM("NVM size = %d\n", sz);
 	il->eeprom = kzalloc(sz, GFP_KERNEL);
-	if (!il->eeprom) {
-		ret = -ENOMEM;
-		goto alloc_err;
-	}
+	if (!il->eeprom)
+		return -ENOMEM;
+
 	e = (__le16 *) il->eeprom;
 
 	il->ops->apm_init(il);
@@ -778,7 +777,6 @@ err:
 		il_eeprom_free(il);
 	/* Reset chip to save power until we load uCode during "up". */
 	il_apm_stop(il);
-alloc_err:
 	return ret;
 }
 EXPORT_SYMBOL(il_eeprom_init);
-- 
2.6.3

[PATCH 2/3] net-iwlegacy: One check less in il_eeprom_init() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 21:12:29 +0100

This issue was detected by using the Coccinelle software.

Adjust a jump target to avoid a check repetition before the function
call "il_eeprom_free".

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

diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index c3afaf7..ae45fd3 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -734,7 +734,7 @@ il_eeprom_init(struct il_priv *il)
 	if (ret < 0) {
 		IL_ERR("EEPROM not found, EEPROM_GP=0x%08x\n", gp);
 		ret = -ENOENT;
-		goto err;
+		goto free_eeprom;
 	}
 
 	/* Make sure driver (instead of uCode) is allowed to read EEPROM */
@@ -742,7 +742,7 @@ il_eeprom_init(struct il_priv *il)
 	if (ret < 0) {
 		IL_ERR("Failed to acquire EEPROM semaphore.\n");
 		ret = -ENOENT;
-		goto err;
+		goto free_eeprom;
 	}
 
 	/* eeprom is an array of 16bit values */
@@ -772,9 +772,11 @@ il_eeprom_init(struct il_priv *il)
 done:
 	il->ops->eeprom_release_semaphore(il);
 
-err:
-	if (ret)
+	if (ret) {
+free_eeprom:
 		il_eeprom_free(il);
+	}
+
 	/* Reset chip to save power until we load uCode during "up". */
 	il_apm_stop(il);
 	return ret;
-- 
2.6.3

[PATCH 3/3] net-iwlegacy: Another refactoring for il_eeprom_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 21:16:01 +0100

Rename a jump label according to the current Linux coding style convention.

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

diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index ae45fd3..660ab2b 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -759,7 +759,7 @@ il_eeprom_init(struct il_priv *il)
 				 IL_EEPROM_ACCESS_TIMEOUT);
 		if (ret < 0) {
 			IL_ERR("Time out reading EEPROM[%d]\n", addr);
-			goto done;
+			goto release_semaphore;
 		}
 		r = _il_rd(il, CSR_EEPROM_REG);
 		e[addr / 2] = cpu_to_le16(r >> 16);
@@ -769,7 +769,7 @@ il_eeprom_init(struct il_priv *il)
 		 il_eeprom_query16(il, EEPROM_VERSION));
 
 	ret = 0;
-done:
+release_semaphore:
 	il->ops->eeprom_release_semaphore(il);
 
 	if (ret) {
-- 
2.6.3

[PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 22:27:20 +0100

This issue was detected by using the Coccinelle software.

Move the jump label directly before the desired log statement
so that the variable "err" will not be checked once more
after it was determined that a function call failed.
Use the identifier "report_failure" instead of the label "err".

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

diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c b/drivers/net/wireless/marvell/libertas/if_spi.c
index 82c0796..c9ae27e 100644
--- a/drivers/net/wireless/marvell/libertas/if_spi.c
+++ b/drivers/net/wireless/marvell/libertas/if_spi.c
@@ -880,18 +880,18 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
 				&hiStatus);
 	if (err) {
 		netdev_err(priv->dev, "I/O error\n");
-		goto err;
+		goto report_failure;
 	}
 
 	if (hiStatus & IF_SPI_HIST_CMD_UPLOAD_RDY) {
 		err = if_spi_c2h_cmd(card);
 		if (err)
-			goto err;
+			goto report_failure;
 	}
 	if (hiStatus & IF_SPI_HIST_RX_UPLOAD_RDY) {
 		err = if_spi_c2h_data(card);
 		if (err)
-			goto err;
+			goto report_failure;
 	}
 
 	/*
@@ -940,9 +940,10 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
 	if (hiStatus & IF_SPI_HIST_CARD_EVENT)
 		if_spi_e2h(card);
 
-err:
-	if (err)
+	if (err) {
+report_failure:
 		netdev_err(priv->dev, "%s: got error %d\n", __func__, err);
+	}
 
 	lbs_deb_leave(LBS_DEB_SPI);
 }
-- 
2.6.3

Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Julia Lawall]

On Fri, 1 Jan 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 22:27:20 +0100
> 
> This issue was detected by using the Coccinelle software.
> 
> Move the jump label directly before the desired log statement
> so that the variable "err" will not be checked once more
> after it was determined that a function call failed.

Putting a label inside an if is ugly.

julia

> Use the identifier "report_failure" instead of the label "err".
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/marvell/libertas/if_spi.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c b/drivers/net/wireless/marvell/libertas/if_spi.c
> index 82c0796..c9ae27e 100644
> --- a/drivers/net/wireless/marvell/libertas/if_spi.c
> +++ b/drivers/net/wireless/marvell/libertas/if_spi.c
> @@ -880,18 +880,18 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
>  				&hiStatus);
>  	if (err) {
>  		netdev_err(priv->dev, "I/O error\n");
> -		goto err;
> +		goto report_failure;
>  	}
>  
>  	if (hiStatus & IF_SPI_HIST_CMD_UPLOAD_RDY) {
>  		err = if_spi_c2h_cmd(card);
>  		if (err)
> -			goto err;
> +			goto report_failure;
>  	}
>  	if (hiStatus & IF_SPI_HIST_RX_UPLOAD_RDY) {
>  		err = if_spi_c2h_data(card);
>  		if (err)
> -			goto err;
> +			goto report_failure;
>  	}
>  
>  	/*
> @@ -940,9 +940,10 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
>  	if (hiStatus & IF_SPI_HIST_CARD_EVENT)
>  		if_spi_e2h(card);
>  
> -err:
> -	if (err)
> +	if (err) {
> +report_failure:
>  		netdev_err(priv->dev, "%s: got error %d\n", __func__, err);
> +	}
>  
>  	lbs_deb_leave(LBS_DEB_SPI);
>  }
> -- 
> 2.6.3
> 
> --
> 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 2/3] net-iwlegacy: One check less in il_eeprom_init() after error detection

[Sergei Shtylyov]

Hello.

On 1/1/2016 11:31 PM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 21:12:29 +0100
>
> This issue was detected by using the Coccinelle software.
>
> Adjust a jump target to avoid a check repetition before the function
> call "il_eeprom_free".

    One question: why?

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/net/wireless/intel/iwlegacy/common.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
> index c3afaf7..ae45fd3 100644
> --- a/drivers/net/wireless/intel/iwlegacy/common.c
> +++ b/drivers/net/wireless/intel/iwlegacy/common.c
[...]
> @@ -772,9 +772,11 @@ il_eeprom_init(struct il_priv *il)
>   done:
>   	il->ops->eeprom_release_semaphore(il);
>
> -err:
> -	if (ret)
> +	if (ret) {
> +free_eeprom:

    This is ugly, I'd say.

[...]

MBR, Sergei

Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Sergei Shtylyov]

Hello

On 1/2/2016 12:33 AM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 22:27:20 +0100
>
> This issue was detected by using the Coccinelle software.
>
> Move the jump label directly before the desired log statement
> so that the variable "err" will not be checked once more
> after it was determined that a function call failed.
> Use the identifier "report_failure" instead of the label "err".

    Why? The code was smart enough and you're making it uglier that it needs 
to be.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

MBR, Sergei

Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[SF Markus Elfring]

>> Move the jump label directly before the desired log statement
>> so that the variable "err" will not be checked once more
>> after it was determined that a function call failed.
>> Use the identifier "report_failure" instead of the label "err".
> 
>    Why?

I suggest to reconsider the places with which such a jump label
is connected.


> The code was smart enough

Which action should really be performed after a failure was detected
and handled a bit already?

* Another condition check

* Just additional error logging


> and you're making it uglier that it needs to be.

I assume that a software development taste can evolve, can't it?

Regards,
Markus

Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Julia Lawall]

On Sat, 2 Jan 2016, SF Markus Elfring wrote:

> >> Move the jump label directly before the desired log statement
> >> so that the variable "err" will not be checked once more
> >> after it was determined that a function call failed.
> >> Use the identifier "report_failure" instead of the label "err".
> > 
> >    Why?
> 
> I suggest to reconsider the places with which such a jump label
> is connected.
> 
> 
> > The code was smart enough
> 
> Which action should really be performed after a failure was detected
> and handled a bit already?
> 
> * Another condition check
> 
> * Just additional error logging
> 
> 
> > and you're making it uglier that it needs to be.
> 
> I assume that a software development taste can evolve, can't it?

So far, you have gotten several down votes for this kind of change, and no 
enthusiasm.

Admittedly, this is a trivial case, because there are no local variables, 
but do you actually know the semantics in C of a jump into a block?  And 
if you do know, do you think that this semantics is common knowledge?  And 
do you really think that introducing poorly understandable code is really 
worth saving an if test of a single variable on a non-critical path?

Most of the kernel code is not performance critical at the level of a 
single if test.  So the goal should be for the code to be easy to 
understand and robust to change.  The code that is performance critical, 
you should probably not touch, ever.  The people who wrote it knew what 
was important and what was not.

julia

Re: [PATCH] net-brcmfmac: Delete an unnecessary variable initialisation in brcmf_sdio_download_firmware()

[Arend van Spriel]

On 01/01/2016 08:26 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 20:20:15 +0100

I think it has been said over and over, but please use driver name only 
as prefix. I don't see value to prepend it with 'net-'.

> Omit explicit initialisation at the beginning for one local variable
> that is redefined before its first use.
>

That being said here is my....

Acked-by: Arend van Spriel <arend@broadcom.com>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index ceb2a75..c21eeb1 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -3260,7 +3260,7 @@ static int brcmf_sdio_download_firmware(struct brcmf_sdio *bus,
>   					const struct firmware *fw,
>   					void *nvram, u32 nvlen)
>   {
> -	int bcmerror = -EFAULT;
> +	int bcmerror;
>   	u32 rstvec;
>
>   	sdio_claim_host(bus->sdiodev->func[1]);
>

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[SF Markus Elfring]

>> I assume that a software development taste can evolve, can't it?
> 
> So far, you have gotten several down votes for this kind of change,

I am curious when more contributors will share corresponding opinions.


> and no enthusiasm.

How many software designers and developers can become enthusiastic
about better exception handling to some degree?


> The code that is performance critical, you should probably not touch, ever.

I imagine that technical evolution will result in further considerations
so that "unchangeable" components can be adjusted once more.


> The people who wrote it knew what was important and what was not.

I might come along at some places where the affected knowledge will also evolve.

Regards,
Markus

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Arend van Spriel]

On 02-01-16 10:08, SF Markus Elfring wrote:
>>> I assume that a software development taste can evolve, can't it?
>>
>> So far, you have gotten several down votes for this kind of change,
> 
> I am curious when more contributors will share corresponding opinions.

Let's burn some cycles on this while the holidays give me time to do so.
"software development taste" is another term for "coding style". In
every project battles are fought over this between friends and foes. I
have never seen much evolution going on in this area.

>> and no enthusiasm.
> 
> How many software designers and developers can become enthusiastic
> about better exception handling to some degree?

I had to  take a look at this particular patch and I have to say that I
don't see, using your favorite term, evolution at work. It looks more
like the result of inbred. What the patch tries to do is avoid the extra
'if (err)'. Setting coding style aside, the question is whether there is
a good metric for the patch. So does it really safe processing time? Did
you look at the resulting assembly code for different target architectures?

You got pushed back on the change so you have to come up with solid
arguments for your change instead of spewing ideas about evolution in
software development. Running Coccinelle is one thing, but understanding
the results and what you are ultimately proposing to be changed is more
important.

Regards,
Arend

>> The code that is performance critical, you should probably not touch, ever.
> 
> I imagine that technical evolution will result in further considerations
> so that "unchangeable" components can be adjusted once more.
> 
> 
>> The people who wrote it knew what was important and what was not.
> 
> I might come along at some places where the affected knowledge will also evolve.
> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[SF Markus Elfring]

> I have never seen much evolution going on in this area.

I can get an other impression from a specific document for example.
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/log/Documentation/CodingStyle


> What the patch tries to do is avoid the extra 'if (err)'.

Yes. - I propose to look at related consequences together with the usage
of a popular short jump label once more.


> Setting coding style aside, the question is whether there is
> a good metric for the patch.

A software development challenge is to accept changes also around a topic
like "error handling". My update suggestion for the source file
"drivers/net/wireless/marvell/libertas/if_spi.c" should only improve
exception handling. (I came along other source files where more improvements
will eventually be possible.)

When will the run-time behaviour matter also for exceptional situations?


> Did you look at the resulting assembly code for different target architectures?

Not yet. - Which execution system variants would you recommend for
further comparisons?

Regards,
Markus

[PATCH 0/3] net-rsi: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 15:36:25 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()
  Delete unnecessary variable initialisations in rsi_send_data_pkt()
  Replace variable initialisations by assignments in rsi_send_data_pkt()

 drivers/net/wireless/rsi/rsi_91x_pkt.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

-- 
2.6.3

[PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 14:54:30 +0100

Omit explicit initialisation at the beginning for five local variables
which are redefined before their first use.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/rsi/rsi_91x_pkt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 702593f..ee98f5b 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -123,15 +123,15 @@ int rsi_send_mgmt_pkt(struct rsi_common *common,
 		      struct sk_buff *skb)
 {
 	struct rsi_hw *adapter = common->priv;
-	struct ieee80211_hdr *wh = NULL;
+	struct ieee80211_hdr *wh;
 	struct ieee80211_tx_info *info;
-	struct ieee80211_bss_conf *bss = NULL;
+	struct ieee80211_bss_conf *bss;
 	struct ieee80211_hw *hw = adapter->hw;
 	struct ieee80211_conf *conf = &hw->conf;
 	struct skb_info *tx_params;
-	int status = -E2BIG;
-	__le16 *msg = NULL;
-	u8 extnd_size = 0;
+	int status;
+	__le16 *msg;
+	u8 extnd_size;
 	u8 vap_id = 0;
 
 	info = IEEE80211_SKB_CB(skb);
-- 
2.6.3

[PATCH 2/3] rsi: Delete unnecessary variable initialisations in rsi_send_data_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 15:15:12 +0100

Omit explicit initialisation at the beginning for four local variables
which are redefined before their first use.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/rsi/rsi_91x_pkt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index ee98f5b..ec65e1c 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -27,15 +27,15 @@
 int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 {
 	struct rsi_hw *adapter = common->priv;
-	struct ieee80211_hdr *tmp_hdr = NULL;
+	struct ieee80211_hdr *tmp_hdr;
 	struct ieee80211_tx_info *info;
 	struct skb_info *tx_params;
-	struct ieee80211_bss_conf *bss = NULL;
+	struct ieee80211_bss_conf *bss;
 	int status = -EINVAL;
 	u8 ieee80211_size = MIN_802_11_HDR_LEN;
-	u8 extnd_size = 0;
+	u8 extnd_size;
 	__le16 *frame_desc;
-	u16 seq_num = 0;
+	u16 seq_num;
 
 	info = IEEE80211_SKB_CB(skb);
 	bss = &info->control.vif->bss_conf;
-- 
2.6.3

[PATCH 3/3] rsi: Replace variable initialisations by assignments in rsi_send_data_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 15:25:34 +0100

Replace explicit initialisation for two local variables at the beginning
by assignments.

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

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index ec65e1c..fe36e7d 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -26,12 +26,12 @@
  */
 int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 {
-	struct rsi_hw *adapter = common->priv;
+	struct rsi_hw *adapter;
 	struct ieee80211_hdr *tmp_hdr;
 	struct ieee80211_tx_info *info;
 	struct skb_info *tx_params;
 	struct ieee80211_bss_conf *bss;
-	int status = -EINVAL;
+	int status;
 	u8 ieee80211_size = MIN_802_11_HDR_LEN;
 	u8 extnd_size;
 	__le16 *frame_desc;
@@ -41,8 +41,10 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 	bss = &info->control.vif->bss_conf;
 	tx_params = (struct skb_info *)info->driver_data;
 
-	if (!bss->assoc)
+	if (!bss->assoc) {
+		status = -EINVAL;
 		goto err;
+	}
 
 	tmp_hdr = (struct ieee80211_hdr *)&skb->data[0];
 	seq_num = (le16_to_cpu(tmp_hdr->seq_ctrl) >> 4);
@@ -97,7 +99,7 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 	frame_desc[7] = cpu_to_le16(((tx_params->tid & 0xf) << 4) |
 				    (skb->priority & 0xf) |
 				    (tx_params->sta_id << 8));
-
+	adapter = common->priv;
 	status = adapter->host_intf_write_pkt(common->priv,
 					      skb->data,
 					      skb->len);
-- 
2.6.3

Re: [PATCH 3/3] rsi: Replace variable initialisations by assignments in rsi_send_data_pkt()

[Francois Romieu]

SF Markus Elfring <elfring@users.sourceforge.net> :
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 2 Jan 2016 15:25:34 +0100
> 
> Replace explicit initialisation for two local variables at the beginning
> by assignments.

It makes no sense for the 'adapter' variable.

-- 
Ueimor

net-rsi: Reconsider usage of variable "vap_id" in rsi_send_mgmt_pkt()

[SF Markus Elfring]

Hello,

I have taken another look at the implementation of the function "rsi_send_mgmt_pkt".
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/net/wireless/rsi/rsi_91x_pkt.c?id=e8c58e7a5a106c3d557fccd01cd4d1128f9bab38#n114

I find the following statement combination interesting there.

…
	u8 vap_id = 0;
…
	msg[7] |= cpu_to_le16(vap_id << 8);
…

I would appreciate a further clarification.
Does a shift operation for a variable which contains zero indicate an open issue?

Regards,
Markus

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2786 bytes --]

Hi Markus,

[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on v4.4-rc7 next-20151231]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/net-rsi-Fine-tuning-for-two-function-implementations/20160102-224740
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: x86_64-randconfig-s2-01030012 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/net/wireless/rsi/rsi_91x_pkt.c: In function 'rsi_send_mgmt_pkt':
>> drivers/net/wireless/rsi/rsi_91x_pkt.c:211:2: warning: 'status' may be used uninitialized in this function [-Wmaybe-uninitialized]
     rsi_indicate_tx_status(common->priv, skb, status);
     ^

vim +/status +211 drivers/net/wireless/rsi/rsi_91x_pkt.c

dad0d04f Fariya Fatima 2014-03-16  195  	/* Indicate to firmware to give cfm */
dad0d04f Fariya Fatima 2014-03-16  196  	if ((skb->data[16] == IEEE80211_STYPE_PROBE_REQ) && (!bss->assoc)) {
dad0d04f Fariya Fatima 2014-03-16  197  		msg[1] |= cpu_to_le16(BIT(10));
dad0d04f Fariya Fatima 2014-03-16  198  		msg[7] = cpu_to_le16(PROBEREQ_CONFIRM);
dad0d04f Fariya Fatima 2014-03-16  199  		common->mgmt_q_block = true;
dad0d04f Fariya Fatima 2014-03-16  200  	}
dad0d04f Fariya Fatima 2014-03-16  201  
dad0d04f Fariya Fatima 2014-03-16  202  	msg[7] |= cpu_to_le16(vap_id << 8);
dad0d04f Fariya Fatima 2014-03-16  203  
dad0d04f Fariya Fatima 2014-03-16  204  	status = adapter->host_intf_write_pkt(common->priv,
dad0d04f Fariya Fatima 2014-03-16  205  					      (u8 *)msg,
dad0d04f Fariya Fatima 2014-03-16  206  					      skb->len);
dad0d04f Fariya Fatima 2014-03-16  207  	if (status)
dad0d04f Fariya Fatima 2014-03-16  208  		rsi_dbg(ERR_ZONE, "%s: Failed to write the packet\n", __func__);
dad0d04f Fariya Fatima 2014-03-16  209  
dad0d04f Fariya Fatima 2014-03-16  210  err:
dad0d04f Fariya Fatima 2014-03-16 @211  	rsi_indicate_tx_status(common->priv, skb, status);
dad0d04f Fariya Fatima 2014-03-16  212  	return status;
dad0d04f Fariya Fatima 2014-03-16  213  }

:::::: The code at line 211 was first introduced by commit
:::::: dad0d04fa7ba41ce603a01e8e64967650303e9a2 rsi: Add RS9113 wireless driver

:::::: TO: Fariya Fatima <fariyaf@gmail.com>
:::::: CC: John W. Linville <linville@tuxdriver.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 27399 bytes --]

Re: [PATCH 3/3] net-iwlegacy: Another refactoring for il_eeprom_init()

[Souptick Joarder]

On Sat, Jan 2, 2016 at 2:02 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 21:16:01 +0100
>
> Rename a jump label according to the current Linux coding style convention.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/intel/iwlegacy/common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
> index ae45fd3..660ab2b 100644
> --- a/drivers/net/wireless/intel/iwlegacy/common.c
> +++ b/drivers/net/wireless/intel/iwlegacy/common.c
> @@ -759,7 +759,7 @@ il_eeprom_init(struct il_priv *il)
>                                  IL_EEPROM_ACCESS_TIMEOUT);
>                 if (ret < 0) {
>                         IL_ERR("Time out reading EEPROM[%d]\n", addr);
> -                       goto done;
> +                       goto release_semaphore;

Current code looks good.
>                 }
>                 r = _il_rd(il, CSR_EEPROM_REG);
>                 e[addr / 2] = cpu_to_le16(r >> 16);
> @@ -769,7 +769,7 @@ il_eeprom_init(struct il_priv *il)
>                  il_eeprom_query16(il, EEPROM_VERSION));
>
>         ret = 0;
> -done:
> +release_semaphore:
>         il->ops->eeprom_release_semaphore(il);
>
>         if (ret) {
> --
> 2.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-Souptick

[PATCH v2 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 19:22:36 +0100

Omit explicit initialisation at the beginning for four local variables
which are redefined before their first use.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/rsi/rsi_91x_pkt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 702593f..571eaba 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -123,15 +123,15 @@ int rsi_send_mgmt_pkt(struct rsi_common *common,
 		      struct sk_buff *skb)
 {
 	struct rsi_hw *adapter = common->priv;
-	struct ieee80211_hdr *wh = NULL;
+	struct ieee80211_hdr *wh;
 	struct ieee80211_tx_info *info;
-	struct ieee80211_bss_conf *bss = NULL;
+	struct ieee80211_bss_conf *bss;
 	struct ieee80211_hw *hw = adapter->hw;
 	struct ieee80211_conf *conf = &hw->conf;
 	struct skb_info *tx_params;
 	int status = -E2BIG;
-	__le16 *msg = NULL;
-	u8 extnd_size = 0;
+	__le16 *msg;
+	u8 extnd_size;
 	u8 vap_id = 0;
 
 	info = IEEE80211_SKB_CB(skb);
-- 
2.6.3

[PATCH 0/3] NFC-mei_phy: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 21:47:30 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Refactoring for mei_nfc_connect()
  Refactoring for mei_nfc_if_version()
  Delete an unnecessary variable initialisation in mei_nfc_if_version()

 drivers/nfc/mei_phy.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

-- 
2.6.3

[PATCH 1/3] NFC-mei_phy: Refactoring for mei_nfc_connect()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 21:21:24 +0100

This issue was detected by using the Coccinelle software.

Adjust jump targets according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/nfc/mei_phy.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/nfc/mei_phy.c b/drivers/nfc/mei_phy.c
index 83deda4..8e3a69f 100644
--- a/drivers/nfc/mei_phy.c
+++ b/drivers/nfc/mei_phy.c
@@ -173,8 +173,8 @@ static int mei_nfc_connect(struct nfc_mei_phy *phy)
 
 	reply = kzalloc(connect_resp_length, GFP_KERNEL);
 	if (!reply) {
-		kfree(cmd);
-		return -ENOMEM;
+		r = -ENOMEM;
+		goto free_cmd;
 	}
 
 	connect_resp = (struct mei_nfc_connect_resp *)reply->data;
@@ -189,7 +189,7 @@ static int mei_nfc_connect(struct nfc_mei_phy *phy)
 	r = mei_cldev_send(phy->cldev, (u8 *)cmd, connect_length);
 	if (r < 0) {
 		pr_err("Could not send connect cmd %d\n", r);
-		goto err;
+		goto free_reply;
 	}
 
 	bytes_recv = mei_cldev_recv(phy->cldev, (u8 *)reply,
@@ -197,7 +197,7 @@ static int mei_nfc_connect(struct nfc_mei_phy *phy)
 	if (bytes_recv < 0) {
 		r = bytes_recv;
 		pr_err("Could not read connect response %d\n", r);
-		goto err;
+		goto free_reply;
 	}
 
 	MEI_DUMP_NFC_HDR("connect reply", &reply->hdr);
@@ -210,11 +210,10 @@ static int mei_nfc_connect(struct nfc_mei_phy *phy)
 		connect_resp->me_hotfix, connect_resp->me_build);
 
 	r = 0;
-
-err:
+free_reply:
 	kfree(reply);
+free_cmd:
 	kfree(cmd);
-
 	return r;
 }
 
-- 
2.6.3

[PATCH 2/3] NFC-mei_phy: Refactoring for mei_nfc_if_version()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 21:33:04 +0100

Rename a jump label according to the current Linux coding style convention.

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

diff --git a/drivers/nfc/mei_phy.c b/drivers/nfc/mei_phy.c
index 8e3a69f..3c74028 100644
--- a/drivers/nfc/mei_phy.c
+++ b/drivers/nfc/mei_phy.c
@@ -136,7 +136,7 @@ static int mei_nfc_if_version(struct nfc_mei_phy *phy)
 	if (bytes_recv < 0 || bytes_recv < sizeof(struct mei_nfc_reply)) {
 		pr_err("Could not read IF version\n");
 		r = -EIO;
-		goto err;
+		goto free_reply;
 	}
 
 	version = (struct mei_nfc_if_version *)reply->data;
@@ -144,8 +144,7 @@ static int mei_nfc_if_version(struct nfc_mei_phy *phy)
 	phy->fw_ivn = version->fw_ivn;
 	phy->vendor_id = version->vendor_id;
 	phy->radio_type = version->radio_type;
-
-err:
+free_reply:
 	kfree(reply);
 	return r;
 }
-- 
2.6.3

[PATCH 3/3] NFC-mei_phy: Delete an unnecessary variable initialisation in mei_nfc_if_version()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 21:40:10 +0100

Omit explicit initialisation at the beginning for one local variable
that is redefined before its first use.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/nfc/mei_phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nfc/mei_phy.c b/drivers/nfc/mei_phy.c
index 3c74028..99fd87d 100644
--- a/drivers/nfc/mei_phy.c
+++ b/drivers/nfc/mei_phy.c
@@ -105,7 +105,7 @@ static int mei_nfc_if_version(struct nfc_mei_phy *phy)
 {
 
 	struct mei_nfc_cmd cmd;
-	struct mei_nfc_reply *reply = NULL;
+	struct mei_nfc_reply *reply;
 	struct mei_nfc_if_version *version;
 	size_t if_version_length;
 	int bytes_recv, r;
-- 
2.6.3

Re: [PATCH 1/3] NFC-mei_phy: Refactoring for mei_nfc_connect()

[Julian Calaby]

Hi Markus,

On Sun, Jan 3, 2016 at 7:54 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 2 Jan 2016 21:21:24 +0100
>
> This issue was detected by using the Coccinelle software.
>
> Adjust jump targets according to the current Linux coding style convention.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/nfc/mei_phy.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nfc/mei_phy.c b/drivers/nfc/mei_phy.c
> index 83deda4..8e3a69f 100644
> --- a/drivers/nfc/mei_phy.c
> +++ b/drivers/nfc/mei_phy.c
> @@ -173,8 +173,8 @@ static int mei_nfc_connect(struct nfc_mei_phy *phy)
>
>         reply = kzalloc(connect_resp_length, GFP_KERNEL);
>         if (!reply) {
> -               kfree(cmd);
> -               return -ENOMEM;
> +               r = -ENOMEM;
> +               goto free_cmd;
>         }
>
>         connect_resp = (struct mei_nfc_connect_resp *)reply->data;
> @@ -189,7 +189,7 @@ static int mei_nfc_connect(struct nfc_mei_phy *phy)
>         r = mei_cldev_send(phy->cldev, (u8 *)cmd, connect_length);
>         if (r < 0) {
>                 pr_err("Could not send connect cmd %d\n", r);
> -               goto err;
> +               goto free_reply;
>         }
>
>         bytes_recv = mei_cldev_recv(phy->cldev, (u8 *)reply,
> @@ -197,7 +197,7 @@ static int mei_nfc_connect(struct nfc_mei_phy *phy)
>         if (bytes_recv < 0) {
>                 r = bytes_recv;
>                 pr_err("Could not read connect response %d\n", r);
> -               goto err;
> +               goto free_reply;
>         }
>
>         MEI_DUMP_NFC_HDR("connect reply", &reply->hdr);
> @@ -210,11 +210,10 @@ static int mei_nfc_connect(struct nfc_mei_phy *phy)
>                 connect_resp->me_hotfix, connect_resp->me_build);
>
>         r = 0;
> -
> -err:
> +free_reply:
>         kfree(reply);
> +free_cmd:
>         kfree(cmd);
> -

Why are you deleting the two blank lines here?

Thanks,

-- 
Julian Calaby

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

Re: [PATCH 1/3] NFC-mei_phy: Refactoring for mei_nfc_connect()

[SF Markus Elfring]

>>         r = 0;
>> -
>> -err:
>> +free_reply:
>>         kfree(reply);
>> +free_cmd:
>>         kfree(cmd);
>> -
> 
> Why are you deleting the two blank lines here?

Can they be unnecessary at this source code place
according to the Linux coding style convention?

Regards,
Markus

Re: [PATCH 1/3] NFC-mei_phy: Refactoring for mei_nfc_connect()

[Joe Perches]

On Sun, 2016-01-03 at 08:00 +0100, SF Markus Elfring wrote:
> > >         r = 0;
> > > -
> > > -err:
> > > +free_reply:
> > >         kfree(reply);
> > > +free_cmd:
> > >         kfree(cmd);
> > > -
> > 
> > Why are you deleting the two blank lines here?
> 
> Can they be unnecessary at this source code place
> according to the Linux coding style convention?

As far as I know, there's no linux specific accepted
convention for blank lines preceding labels.

My personal preference is for a blank line before a
new block, but not before the second and subsequent
labels in an error handling block.

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Arend van Spriel]

On 02-01-16 12:21, SF Markus Elfring wrote:
>> I have never seen much evolution going on in this area.
> 
> I can get an other impression from a specific document for example.
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/log/Documentation/CodingStyle
> 
> 
>> What the patch tries to do is avoid the extra 'if (err)'.
> 
> Yes. - I propose to look at related consequences together with the usage
> of a popular short jump label once more.

When I read a subject saying "Better exception handling" it sounds like
a functional improvement. Your change does not change anything
functionally and may or may not save a bit of execution time depending
on how smart the compiler is. What you change does is confuse people
reading the code. So please explain why your update improves exception
handling here. I don't see it. The code is not making the driver more
robust against failures in this function, which is what I think of
reading "better exception handling".

>> Setting coding style aside, the question is whether there is
>> a good metric for the patch.
> 
> A software development challenge is to accept changes also around a topic
> like "error handling". My update suggestion for the source file
> "drivers/net/wireless/marvell/libertas/if_spi.c" should only improve
> exception handling. (I came along other source files where more improvements
> will eventually be possible.)
> 
> When will the run-time behaviour matter also for exceptional situations?
> 
> 
>> Did you look at the resulting assembly code for different target architectures?
> 
> Not yet. - Which execution system variants would you recommend for
> further comparisons?

Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.

Regards,
Arend

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[SF Markus Elfring]

>>> What the patch tries to do is avoid the extra 'if (err)'.
>>
>> Yes. - I propose to look at related consequences together with the usage
>> of a popular short jump label once more.
> 
> When I read a subject saying "Better exception handling" it sounds like
> a functional improvement. Your change does not change anything
> functionally and may or may not save a bit of execution time depending
> on how smart the compiler is.

Can it eventually matter to skip another condition check in three cases?


> What you change does is confuse people reading the code.

A few software developers might find this proposal unusual.


> So please explain why your update improves exception handling here.
> I don't see it.

How does this feedback fit to the mentioned check avoidance?


> The code is not making the driver more robust against failures

That's true for this update suggestion.


> in this function, which is what I think of reading "better exception handling".

Other implementation details are affected by the shown fine-tuning.

Regards,
Markus

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Rafał Miłecki]

On 3 January 2016 at 10:36, Arend van Spriel <aspriel@gmail.com> wrote:
> On 02-01-16 12:21, SF Markus Elfring wrote:
>>> Did you look at the resulting assembly code for different target architectures?
>>
>> Not yet. - Which execution system variants would you recommend for
>> further comparisons?
>
> Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.

Oh, don't forget about MIPS with its fancy branches handling. You know
about it, don't you?

I'm against this patch as well.

-- 
Rafał

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Dan Carpenter]

These patches are labour intensive to review because you can't just do
it in the email client.  Also you were not able to review it properly
yourself and introduced a bug.

I am often remove initializers but it's normally because I am changing
something else which makes it worthwhile.  This patch is the correct
thing but it's not "worthwhile".  It is not a good use of my time.

Please stop sending cleanup patches, Markus.  Just send fixes.

regards,
dan carpenter

Re: [PATCH 1/3] net-iwlegacy: Refactoring for il_eeprom_init()

[Stanislaw Gruszka]

On Fri, Jan 01, 2016 at 09:30:10PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 20:54:25 +0100
> 
> Return directly if a memory allocation failed at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Dan Carpenter]

Btw, GCC misses a lot of uninitialized variable bugs.  I have a Smatch
check which sometimes catches the bugs that GCC misses but you should
not rely on the tools here.  These patches need to be reviewed manually.

And the "goto err" before the initialization makes everything more
complicated (that's actually what caused the bug in this patch, in fact).

regards,
dan carpenter

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Arend van Spriel]

On 03-01-16 16:18, Rafał Miłecki wrote:
> On 3 January 2016 at 10:36, Arend van Spriel <aspriel@gmail.com> wrote:
>> On 02-01-16 12:21, SF Markus Elfring wrote:
>>>> Did you look at the resulting assembly code for different target architectures?
>>>
>>> Not yet. - Which execution system variants would you recommend for
>>> further comparisons?
>>
>> Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.
> 
> Oh, don't forget about MIPS with its fancy branches handling. You know
> about it, don't you?

You are asking me, right ;-) ? I have come across my share of MIPS
platforms here at Broadcom, but I still try to avoid them as much as
possible.

> I'm against this patch as well.

and counting... :-p

Regards,
Arend

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

> These patches are labour intensive to review because you can't just do
> it in the email client.

Thanks for your general interest.


> Also you were not able to review it properly yourself and introduced
> a bug.

I admit that it can happen during my software development that I overlook
implementation details somehow.


> I am often remove initializers but it's normally because I am changing
> something else which makes it worthwhile.

It is nice to hear that you are also occasionally looking for similar
update candidates.


> This patch is the correct thing but it's not "worthwhile".

I find this view interesting.


> Please stop sending cleanup patches, Markus.  Just send fixes.

How often will source code clean-up fix something?


May I resend a consistent patch series for the source file
"drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future?

Regards,
Markus

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Rafał Miłecki]

On 4 January 2016 at 11:05, Arend van Spriel <aspriel@gmail.com> wrote:
> On 03-01-16 16:18, Rafał Miłecki wrote:
>> On 3 January 2016 at 10:36, Arend van Spriel <aspriel@gmail.com> wrote:
>>> On 02-01-16 12:21, SF Markus Elfring wrote:
>>>>> Did you look at the resulting assembly code for different target architectures?
>>>>
>>>> Not yet. - Which execution system variants would you recommend for
>>>> further comparisons?
>>>
>>> Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.
>>
>> Oh, don't forget about MIPS with its fancy branches handling. You know
>> about it, don't you?
>
> You are asking me, right ;-) ? I have come across my share of MIPS
> platforms here at Broadcom, but I still try to avoid them as much as
> possible.

I was more thinking about author on this patch. But it's indeed an
interesting thing, just to know, how MIPS CPU handles branches ;)

-- 
Rafał

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Dan Carpenter]

On Mon, Jan 04, 2016 at 11:44:15AM +0100, SF Markus Elfring wrote:
> > Please stop sending cleanup patches, Markus.  Just send fixes.
> 
> How often will source code clean-up fix something?
> 
> 
> May I resend a consistent patch series for the source file
> "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future?

If you were sending checkpatch.pl fixes that would be easier to deal
with but you are sending hundreds of "controversial" cleanups.  They are
controversial in the sense that they don't fix anything against official
kernel style and they go against the author's original intention.  I
tend to agree that useless initializers are bad and disable GCCs
uninitialized variable warnings but just because I agree with you
doesn't make it official kernel style.  It's slightly rude to go against
the author's intention.

regards,
dan carpenter

Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

>> May I resend a consistent patch series for the source file
>> "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future?
> 
> If you were sending checkpatch.pl fixes that would be easier to deal with

Does this feedback mean that you would accept any more suggestions around
source code updates which are derived from recommendations of this script?


> but you are sending hundreds of "controversial" cleanups.

It depends on the time range you look at for my proposals.


> They are controversial in the sense that they don't fix anything
> against official kernel style

I find that I suggested also few changes that fit to this aspect.


> and they go against the author's original intention.

Can it occasionally help to reconsider the "first approach"?


> I tend to agree that useless initializers are bad

Would any more software developers like to share their opinions on this detail?


> and disable GCCs uninitialized variable warnings

I hope that this software area can be also improved.


> but just because I agree with you doesn't make it official kernel style.

That is fine. - Will it become useful to clarify any extensions
to a document like "CodingStyle"?


> It's slightly rude to go against the author's intention.

I just dare to propose further special changes.

Regards,
Markus

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Bjørn Mork]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> Please stop sending cleanup patches, Markus.  Just send fixes.

Thanks for your continued but unwarranted belief in AI.

Do you mind if I remind you of https://lkml.org/lkml/2014/11/3/162 ?
I am sure there are lots and lots of other examples.  There is no reason
to believe this will ever stop.  He just goes into Eliza mode.


Bjørn

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Dan Carpenter]

On Mon, Jan 04, 2016 at 02:17:40PM +0100, Bjørn Mork wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > Please stop sending cleanup patches, Markus.  Just send fixes.
> 
> Thanks for your continued but unwarranted belief in AI.
> 

I always tell people that I am very mechanical and you can rely on me to
send predictable responses...

> Do you mind if I remind you of https://lkml.org/lkml/2014/11/3/162 ?
> I am sure there are lots and lots of other examples.  There is no reason
> to believe this will ever stop.  He just goes into Eliza mode.

Yup.

I feel some sense of responsibility for any patches where kernel-janitors
is on the CC but I'm having a hard time dealing with all of Markus's
patches.  Normally you just respond to the first patch and people change
the later patches but, as you put it, Markus just goes into ELIZA mode.

regards,
dan carpenter

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Mon, 4 Jan 2016 12:28:57 +0300

> Please stop sending cleanup patches, Markus.  Just send fixes.

+1

Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Julian Calaby]

Hi Markus,

On Mon, Jan 4, 2016 at 11:33 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> May I resend a consistent patch series for the source file
>>> "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future?
>>
>> If you were sending checkpatch.pl fixes that would be easier to deal with
>
> Does this feedback mean that you would accept any more suggestions around
> source code updates which are derived from recommendations of this script?

A good rule of thumb here would be that if people start complaining
about a particular type of change, stop sending them.

Another good rule of thumb is to try to "rock the boat" on coding
style and conventions as little as possible. Just because it's
possible doesn't mean that people want to do it.

That said, if you figure out some change that produces significant
reductions in code or binary size on multiple architectures without
making things more complicated, less readable or making the code or
binary size larger, then by all means propose it. "This makes things
smaller" carries much more weight than "I think this is better".
Almost all of the changes you've proposed that have seen any
discussion whatsoever fall into the latter category.

Thanks,

-- 
Julian Calaby

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

Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

> That said, if you figure out some change that produces significant
> reductions in code or binary size on multiple architectures without
> making things more complicated, less readable or making the code or
> binary size larger, then by all means propose it.

Are you looking also for "a proof" that such changes are worthwhile?


> "This makes things smaller" carries much more weight than
> "I think this is better".

Can the discussed implementation of a function like "rsi_send_mgmt_pkt"
become a bit smaller by the deletion of extra variable initialisations


> Almost all of the changes you've proposed that have seen any
> discussion whatsoever fall into the latter category.

Thanks for your interesting feedback.

Can a further constructive dialogue evolve from the presented information?

Regards,
Markus

Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Julian Calaby]

Hi Markus,

On Tue, Jan 5, 2016 at 7:29 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> That said, if you figure out some change that produces significant
>> reductions in code or binary size on multiple architectures without
>> making things more complicated, less readable or making the code or
>> binary size larger, then by all means propose it.
>
> Are you looking also for "a proof" that such changes are worthwhile?

It'd be better than "I think doing things this way is better", which
is the hallmark of most of your patch sets. (Admittedly not this one,
but this one is where the discussion is now, so that's where we're
discussing it.)

>> "This makes things smaller" carries much more weight than
>> "I think this is better".
>
> Can the discussed implementation of a function like "rsi_send_mgmt_pkt"
> become a bit smaller by the deletion of extra variable initialisations

I'm talking in general.

In this case you're asking people to review a patch which requires a
lot of careful review for a fairly minor improvement. I must also note
that you haven't CC'd the people who wrote this driver, so it's
possible that the only people who have reviewed it aren't experts in
the code.

The patches you sent recently which moved labels into if statements
were a clear case of "I think this is better" where any actual benefit
from the changes was eclipsed by the style and readability issues they
introduced.

>> Almost all of the changes you've proposed that have seen any
>> discussion whatsoever fall into the latter category.
>
> Thanks for your interesting feedback.

No problem.

> Can a further constructive dialogue evolve from the presented information?

Part of the issue here is that you don't seem to be listening to the
discussion of your patches, or if you are, you're not significantly
changing your approach or attitude in response.

Every time you send a set of patches, there are legitimate issues
which people raise, and every time they are discussed, you assert that
your patches improve things and seem to ignore the concerns people
raise.

I've seen this same pattern of discussion here with these patches,
with your patches to move labels into if statements, with the patches
you sent late June last year, your patches to remove conditions before
kfree() and friends, etc.

You need to change you attitude: just because you can see some benefit
from your patches doesn't mean others do and it doesn't mean that
they're willing to accept them.

Thanks,

-- 
Julian Calaby

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

Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

> Every time you send a set of patches,

I suggested some updates for Linux source files since October 2014.


> there are legitimate issues which people raise,

There was usual feedback.


> and every time they are discussed,

The discussion results were mixed between acceptance
and usual disagreement.


> you assert that your patches improve things

I guess that should be the default intention of every patch, shouldn't it?


> and seem to ignore the concerns people raise.

I hope not. - But I can imagine that you might understand some responses
from contributors in this way.
Are you waiting for another clarification on a specific issue?


> I've seen this same pattern of discussion here with these patches,
> with your patches to move labels into if statements, with the patches
> you sent late June last year, your patches to remove conditions before
> kfree() and friends, etc.

It seems that communication difficulties come partly from the fact
that I chose search patterns from static source code analysis so far
which belong to an error category that gets a lower priority.


> You need to change you attitude: just because you can see some benefit
> from your patches doesn't mean others do and it doesn't mean that
> they're willing to accept them.

I understand your advice.

Further update suggestions with higher importance might follow for various
software areas in the future.

Regards,
Markus

Re: [PATCH] net-brcmfmac: Delete an unnecessary variable initialisation in brcmf_sdio_download_firmware()

[Kalle Valo]

Arend van Spriel <arend@broadcom.com> writes:

> On 01/01/2016 08:26 PM, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Fri, 1 Jan 2016 20:20:15 +0100
>
> I think it has been said over and over, but please use driver name
> only as prefix. I don't see value to prepend it with 'net-'.

Yes, please use existing naming schemes. This time I can fix it before
I commit the patch, but in the future please use correct prefixes. It's
easy to check what has been used previously:

$ git log --oneline --no-merges --follow drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | head -10
63ce3d5db093 brcmfmac: use msecs_to_jiffies() in macro definitions
4011fc499690 brcmfmac: change brcmf_sdio_wd_timer() prototype
a7decc44a002 brcmfmac: fix waitqueue_active without memory barrier in brcmfmac driver
46d703a77539 brcmfmac: Unify methods to define and map firmware files.
64d66c30c37e brcmfmac: no retries on rxglom superframe errors
6866a64a0f9b brcmfmac: constify brcmf_bus_ops structures
05491d2ccf20 brcm80211: move under broadcom vendor directory
ff4445a8502c brcmfmac: expose device memory to devcoredump subsystem
a32be0177252 brcmfmac: include linux/atomic.h
9d6c1dc4f913 brcmfmac: add dedicated debug level for firmware console logging
$

-- 
Kalle Valo

[PATCH v3 0/3] net-rsi: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 15 Jan 2016 13:54:43 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()
  Delete unnecessary variable initialisations in rsi_send_data_pkt()
  Replace variable initialisations by assignments in rsi_send_data_pkt()

 drivers/net/wireless/rsi/rsi_91x_pkt.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

---

v3: Rebase proposed changes on the source files for the software
    "Linux next-20160114".
    
v2: Unfortunately, the first update step from this series contained
    an inappropriate suggestion.
    Thus fix that.

-- 
2.6.3

[PATCH v3 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 15 Jan 2016 13:30:39 +0100

Omit explicit initialisation at the beginning for four local variables
which are redefined before their first use.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/rsi/rsi_91x_pkt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 702593f..571eaba 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -123,15 +123,15 @@ int rsi_send_mgmt_pkt(struct rsi_common *common,
 		      struct sk_buff *skb)
 {
 	struct rsi_hw *adapter = common->priv;
-	struct ieee80211_hdr *wh = NULL;
+	struct ieee80211_hdr *wh;
 	struct ieee80211_tx_info *info;
-	struct ieee80211_bss_conf *bss = NULL;
+	struct ieee80211_bss_conf *bss;
 	struct ieee80211_hw *hw = adapter->hw;
 	struct ieee80211_conf *conf = &hw->conf;
 	struct skb_info *tx_params;
 	int status = -E2BIG;
-	__le16 *msg = NULL;
-	u8 extnd_size = 0;
+	__le16 *msg;
+	u8 extnd_size;
 	u8 vap_id = 0;
 
 	info = IEEE80211_SKB_CB(skb);
-- 
2.6.3

[PATCH v3 2/3] rsi: Delete unnecessary variable initialisations in rsi_send_data_pkt()

[SF Markus Elfring]

>From 017d1bb49f46266ffeb33178ddd3022d6b341d71 Mon Sep 17 00:00:00 2001
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 15 Jan 2016 13:35:47 +0100
Subject: [PATCH 2/3] rsi: Delete unnecessary variable initialisations in
 rsi_send_data_pkt()

Omit explicit initialisation at the beginning for four local variables
which are redefined before their first use.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/rsi/rsi_91x_pkt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 571eaba..4322df1 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -27,15 +27,15 @@
 int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 {
 	struct rsi_hw *adapter = common->priv;
-	struct ieee80211_hdr *tmp_hdr = NULL;
+	struct ieee80211_hdr *tmp_hdr;
 	struct ieee80211_tx_info *info;
 	struct skb_info *tx_params;
-	struct ieee80211_bss_conf *bss = NULL;
+	struct ieee80211_bss_conf *bss;
 	int status = -EINVAL;
 	u8 ieee80211_size = MIN_802_11_HDR_LEN;
-	u8 extnd_size = 0;
+	u8 extnd_size;
 	__le16 *frame_desc;
-	u16 seq_num = 0;
+	u16 seq_num;
 
 	info = IEEE80211_SKB_CB(skb);
 	bss = &info->control.vif->bss_conf;
-- 
2.6.3

[PATCH v3 3/3] rsi: Replace variable initialisations by assignments in rsi_send_data_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 15 Jan 2016 13:40:22 +0100

Replace explicit initialisation for two local variables at the beginning
by assignments.

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

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 4322df1..2c18c01 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -26,12 +26,12 @@
  */
 int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 {
-	struct rsi_hw *adapter = common->priv;
+	struct rsi_hw *adapter;
 	struct ieee80211_hdr *tmp_hdr;
 	struct ieee80211_tx_info *info;
 	struct skb_info *tx_params;
 	struct ieee80211_bss_conf *bss;
-	int status = -EINVAL;
+	int status;
 	u8 ieee80211_size = MIN_802_11_HDR_LEN;
 	u8 extnd_size;
 	__le16 *frame_desc;
@@ -41,8 +41,10 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 	bss = &info->control.vif->bss_conf;
 	tx_params = (struct skb_info *)info->driver_data;
 
-	if (!bss->assoc)
+	if (!bss->assoc) {
+		status = -EINVAL;
 		goto err;
+	}
 
 	tmp_hdr = (struct ieee80211_hdr *)&skb->data[0];
 	seq_num = (le16_to_cpu(tmp_hdr->seq_ctrl) >> 4);
@@ -97,7 +99,7 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 	frame_desc[7] = cpu_to_le16(((tx_params->tid & 0xf) << 4) |
 				    (skb->priority & 0xf) |
 				    (tx_params->sta_id << 8));
-
+	adapter = common->priv;
 	status = adapter->host_intf_write_pkt(common->priv,
 					      skb->data,
 					      skb->len);
-- 
2.6.3

Re: [PATCH v3 3/3] rsi: Replace variable initialisations by assignments in rsi_send_data_pkt()

[Dan Carpenter]

Still makes no sense for adapter like Francois Romieu said.

regards,
dan carpenter

Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Kalle Valo]

Julia Lawall <julia.lawall@lip6.fr> writes:

> On Sat, 2 Jan 2016, SF Markus Elfring wrote:
>
>> >> Move the jump label directly before the desired log statement
>> >> so that the variable "err" will not be checked once more
>> >> after it was determined that a function call failed.
>> >> Use the identifier "report_failure" instead of the label "err".
>> > 
>> >    Why?
>> 
>> I suggest to reconsider the places with which such a jump label
>> is connected.
>> 
>> 
>> > The code was smart enough
>> 
>> Which action should really be performed after a failure was detected
>> and handled a bit already?
>> 
>> * Another condition check
>> 
>> * Just additional error logging
>> 
>> 
>> > and you're making it uglier that it needs to be.
>> 
>> I assume that a software development taste can evolve, can't it?
>
> So far, you have gotten several down votes for this kind of change, and no 
> enthusiasm.
>
> Admittedly, this is a trivial case, because there are no local variables, 
> but do you actually know the semantics in C of a jump into a block?  And 
> if you do know, do you think that this semantics is common knowledge?  And 
> do you really think that introducing poorly understandable code is really 
> worth saving an if test of a single variable on a non-critical path?
>
> Most of the kernel code is not performance critical at the level of a 
> single if test.  So the goal should be for the code to be easy to 
> understand and robust to change.  The code that is performance critical, 
> you should probably not touch, ever.  The people who wrote it knew what 
> was important and what was not.

Very well said! Only optimise something you can measure.

I'm dropping this patch.

-- 
Kalle Valo

Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[Julian Calaby]

Hi Kalle,

On Sat, Jan 2, 2016 at 5:25 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 19:09:32 +0100
>
> Replace an explicit initialisation for one local variable at the beginning
> by a conditional assignment.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

This looks sane to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

Thanks,

Julian Calaby

> ---
>  drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> index a680a97..30bd59e 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> @@ -246,7 +246,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv *priv,
>         struct ieee80211_conf *conf = &common->hw->conf;
>         bool fastcc;
>         struct ieee80211_channel *channel = hw->conf.chandef.chan;
> -       struct ath9k_hw_cal_data *caldata = NULL;
> +       struct ath9k_hw_cal_data *caldata;
>         enum htc_phymode mode;
>         __be16 htc_mode;
>         u8 cmd_rsp;
> @@ -274,10 +274,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv *priv,
>                 priv->ah->curchan->channel,
>                 channel->center_freq, conf_is_ht(conf), conf_is_ht40(conf),
>                 fastcc);
> -
> -       if (!fastcc)
> -               caldata = &priv->caldata;
> -
> +       caldata = fastcc ? NULL : &priv->caldata;
>         ret = ath9k_hw_reset(ah, hchan, caldata, fastcc);
>         if (ret) {
>                 ath_err(common,
> --
> 2.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Julian Calaby

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

Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[Kalle Valo]

Julian Calaby <julian.calaby@gmail.com> writes:

> Hi Kalle,
>
> On Sat, Jan 2, 2016 at 5:25 AM, SF Markus Elfring
> <elfring@users.sourceforge.net> wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Fri, 1 Jan 2016 19:09:32 +0100
>>
>> Replace an explicit initialisation for one local variable at the beginning
>> by a conditional assignment.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>
> This looks sane to me.
>
> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

Before I commit I'll just change the commit title to:

ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

-- 
Kalle Valo

Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[Julian Calaby]

Hi Kalle,

On Fri, Apr 15, 2016 at 10:09 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Julian Calaby <julian.calaby@gmail.com> writes:
>
>> Hi Kalle,
>>
>> On Sat, Jan 2, 2016 at 5:25 AM, SF Markus Elfring
>> <elfring@users.sourceforge.net> wrote:
>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>> Date: Fri, 1 Jan 2016 19:09:32 +0100
>>>
>>> Replace an explicit initialisation for one local variable at the beginning
>>> by a conditional assignment.
>>>
>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>
>> This looks sane to me.
>>
>> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>
>
> Before I commit I'll just change the commit title to:
>
> ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

Sounds good to me.

Thanks,

-- 
Julian Calaby

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

Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[Kalle Valo]

Julian Calaby <julian.calaby@gmail.com> writes:

> On Sat, Jan 2, 2016 at 5:25 AM, SF Markus Elfring
> <elfring@users.sourceforge.net> wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Fri, 1 Jan 2016 19:09:32 +0100
>>
>> Replace an explicit initialisation for one local variable at the beginning
>> by a conditional assignment.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>
> This looks sane to me.
>
> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

Applied, thanks.

-- 
Kalle Valo

[PATCH 0/3] hostap: Fine-tuning for a few functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:35:43 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Use memdup_user()
  Delete an unnecessary jump label
  Delete unnecessary variable initialisations

 .../net/wireless/intersil/hostap/hostap_ioctl.c    | 36 ++++++++--------------
 1 file changed, 12 insertions(+), 24 deletions(-)

-- 
2.9.3

[PATCH 1/3] hostap: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:19:43 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 .../net/wireless/intersil/hostap/hostap_ioctl.c    | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 3e5fa78..4e271f9 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -3041,14 +3041,9 @@ static int prism2_ioctl_priv_download(local_info_t *local, struct iw_point *p)
 	    p->length > 1024 || !p->pointer)
 		return -EINVAL;
 
-	param = kmalloc(p->length, GFP_KERNEL);
-	if (param == NULL)
-		return -ENOMEM;
-
-	if (copy_from_user(param, p->pointer, p->length)) {
-		ret = -EFAULT;
-		goto out;
-	}
+	param = memdup_user(p->pointer, p->length);
+	if (IS_ERR(param))
+		return PTR_ERR(param);
 
 	if (p->length < sizeof(struct prism2_download_param) +
 	    param->num_areas * sizeof(struct prism2_download_area)) {
@@ -3803,14 +3798,9 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
 	    p->length > PRISM2_HOSTAPD_MAX_BUF_SIZE || !p->pointer)
 		return -EINVAL;
 
-	param = kmalloc(p->length, GFP_KERNEL);
-	if (param == NULL)
-		return -ENOMEM;
-
-	if (copy_from_user(param, p->pointer, p->length)) {
-		ret = -EFAULT;
-		goto out;
-	}
+	param = memdup_user(p->pointer, p->length);
+	if (IS_ERR(param))
+		return PTR_ERR(param);
 
 	switch (param->cmd) {
 	case PRISM2_SET_ENCRYPTION:
-- 
2.9.3

[PATCH 2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:21:29 +0200

Remove a jump label which is unneeded in this function at the end.

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

diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 4e271f9..5942917 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -3835,14 +3835,12 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
 	}
 
 	if (ret == 1 || !ap_ioctl) {
-		if (copy_to_user(p->pointer, param, p->length)) {
+		if (copy_to_user(p->pointer, param, p->length))
 			ret = -EFAULT;
-			goto out;
-		} else if (ap_ioctl)
+		else if (ap_ioctl)
 			ret = 0;
 	}
 
- out:
 	kfree(param);
 	return ret;
 }
-- 
2.9.3

[PATCH 3/3] hostap: Delete unnecessary initialisations for the variable "ret"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:23:14 +0200

The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning of four functions.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 5942917..c37b0bb 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -2895,7 +2895,7 @@ static int prism2_ioctl_priv_monitor(struct net_device *dev, int *i)
 {
 	struct hostap_interface *iface;
 	local_info_t *local;
-	int ret = 0;
+	int ret;
 	u32 mode;
 
 	iface = netdev_priv(dev);
@@ -3035,7 +3035,7 @@ static int ap_mac_cmd_ioctl(local_info_t *local, int *cmd)
 static int prism2_ioctl_priv_download(local_info_t *local, struct iw_point *p)
 {
 	struct prism2_download_param *param;
-	int ret = 0;
+	int ret;
 
 	if (p->length < sizeof(struct prism2_download_param) ||
 	    p->length > 1024 || !p->pointer)
@@ -3791,7 +3791,7 @@ static int prism2_ioctl_scan_req(local_info_t *local,
 static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
 {
 	struct prism2_hostapd_param *param;
-	int ret = 0;
+	int ret;
 	int ap_ioctl = 0;
 
 	if (p->length < sizeof(struct prism2_hostapd_param) ||
@@ -3954,7 +3954,7 @@ int hostap_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	struct iwreq *wrq = (struct iwreq *) ifr;
 	struct hostap_interface *iface;
 	local_info_t *local;
-	int ret = 0;
+	int ret;
 
 	iface = netdev_priv(dev);
 	local = iface->local;
-- 
2.9.3

Re: [PATCH 0/3] hostap: Fine-tuning for a few functions

[Arend van Spriel]

On 20-08-16 18:43, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 18:35:43 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.

Is it worth touching this old stuff especially when you are not making
any functional changes.

Regards,
Arend

> Markus Elfring (3):
>   Use memdup_user()
>   Delete an unnecessary jump label
>   Delete unnecessary variable initialisations
> 
>  .../net/wireless/intersil/hostap/hostap_ioctl.c    | 36 ++++++++--------------
>  1 file changed, 12 insertions(+), 24 deletions(-)
> 

Re: [PATCH 2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Julian Calaby]

Hi Marcus,

On Sun, Aug 21, 2016 at 2:46 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 18:21:29 +0200
>
> Remove a jump label which is unneeded in this function at the end.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> index 4e271f9..5942917 100644
> --- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> +++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> @@ -3835,14 +3835,12 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
>         }
>
>         if (ret == 1 || !ap_ioctl) {
> -               if (copy_to_user(p->pointer, param, p->length)) {
> +               if (copy_to_user(p->pointer, param, p->length))
>                         ret = -EFAULT;
> -                       goto out;
> -               } else if (ap_ioctl)
> +               else if (ap_ioctl)
>                         ret = 0;
>         }
>
> - out:

Does this change make any difference to the compiled code?

Thanks,

-- 
Julian Calaby

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

Re: [PATCH 0/3] hostap: Fine-tuning for a few functions

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 20-08-16 18:43, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Sat, 20 Aug 2016 18:35:43 +0200
>> 
>> A few update suggestions were taken into account
>> from static source code analysis.
>
> Is it worth touching this old stuff especially when you are not making
> any functional changes.

On the other hand if these patches break something this might be a good
way to get feedback if someone is really using this driver ;)

But yeah, not really sure what to do with these obsolete drivers like
hostap, ray_cs and wl3501.

-- 
Kalle Valo

Re: [PATCH 0/3] hostap: Fine-tuning for a few functions

[Joe Perches]

On Mon, 2016-08-22 at 18:49 +0300, Kalle Valo wrote:
> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
[]
> But yeah, not really sure what to do with these obsolete drivers like
> hostap, ray_cs and wl3501.

Maybe marking sections obsolete in MAINTAINERS could
flag some "shouldn't touch this" warning for old code
in checkpatch.pl and/or get_maintainer.pl

[PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS

[Joe Perches]

Use get_maintainer to check the status of individual files.
If "obsolete", suggest leaving the files alone.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4de3cc4..df5e9d9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -704,6 +704,16 @@ sub seed_camelcase_file {
 	}
 }
 
+sub is_maintained_obsolete {
+	my ($filename) = @_;
+
+	return 0 if (!(-e "$root/scripts/get_maintainer.pl"));
+
+	my $status = `perl $root/scripts/get_maintainer.pl --status --nom --nol --nogit --nogit-fallback $filename 2>&1`;
+
+	return $status =~ /obsolete/i;
+}
+
 my $camelcase_seeded = 0;
 sub seed_camelcase_includes {
 	return if ($camelcase_seeded);
@@ -2289,6 +2299,10 @@ sub process {
 		}
 
 		if ($found_file) {
+			if (is_maintained_obsolete($realfile)) {
+				WARN("OBSOLETE",
+				     "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy.  No unnecessary modifications please.\n");
+			}
 			if ($realfile =~ m@^(?:drivers/net/|net/|drivers/staging/)@) {
 				$check = 1;
 			} else {
-- 
2.8.0.rc4.16.g56331f8

Re: [PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS

[SF Markus Elfring]

> @@ -2289,6 +2299,10 @@ sub process {
>  		}
>  
>  		if ($found_file) {
> +			if (is_maintained_obsolete($realfile)) {
> +				WARN("OBSOLETE",
> +				     "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy.  No unnecessary modifications please.\n");
> +			}

How do you think about to avoid a double negation in such a warning message?

Would a wording like "… Only really necessary modifications please.\n"
be more useful here?

Regards,
Markus

Re: [PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS

[Joe Perches]

On Mon, 2016-08-22 at 22:50 +0200, SF Markus Elfring wrote:
> > @@ -2289,6 +2299,10 @@ sub process {
> >  		}
> >  
> >  		if ($found_file) {
> > +			if (is_maintained_obsolete($realfile)) {
> > +				WARN("OBSOLETE",
> > +				     "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy.  No unnecessary modifications please.\n");
> > +			}
> How do you think about to avoid a double negation in such a warning message?
> 
> Would a wording like "… Only really necessary modifications please.\n"
> be more useful here?

No, probably not.

Re: checkpatch: See if modified files are marked obsolete in MAINTAINERS

[SF Markus Elfring]

> Use get_maintainer to check the status of individual files.
> If "obsolete", suggest leaving the files alone.

Will another software system like the "kbuild test robot"
need any more fine-tuning for this change?

Regards,
Markus

Re: checkpatch: See if modified files are marked obsolete in MAINTAINERS

[Julia Lawall]

On Tue, 23 Aug 2016, SF Markus Elfring wrote:

> > Use get_maintainer to check the status of individual files.
> > If "obsolete", suggest leaving the files alone.
>
> Will another software system like the "kbuild test robot"
> need any more fine-tuning for this change?

It only works on files in which there have been commits, thus by
definition not obsolete.

julia


>
> Regards,
> Markus
> --
> 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: [1/3] hostap: Use memdup_user() rather than duplicating its implementation

[Kalle Valo]

SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 18:19:43 +0200
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Patch set to Rejected.

[1/3] hostap: Use memdup_user() rather than duplicating i... 2016-08-20 SF Markus El Rejected

Reason: A similar patch is already applied.

Applying: hostap: Use memdup_user() rather than duplicating its implementation
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging drivers/net/wireless/intersil/hostap/hostap_ioctl.c
CONFLICT (content): Merge conflict in drivers/net/wireless/intersil/hostap/hostap_ioctl.c
Failed to merge in the changes.
Patch failed at 0001 hostap: Use memdup_user() rather than duplicating its implementation

-- 
https://patchwork.kernel.org/patch/9306999/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Kalle Valo]

SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 18:21:29 +0200
> 
> Remove a jump label which is unneeded in this function at the end.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

2 patches set to Rejected.

9291771 [2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()
9291775 [3/3] hostap: Delete unnecessary initialisations for the variable "ret"

Reason: The benefit is not clear.

-- 
https://patchwork.kernel.org/patch/9291771/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[SF Markus Elfring]

> 9291771 [2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()
> 9291775 [3/3] hostap: Delete unnecessary initialisations for the variable "ret"
> 
> Reason: The benefit is not clear.

How do you think about to reduce the source code a bit at these places?

Regards,
Markus

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Kalle Valo]

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

>> 9291771 [2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()
>> 9291775 [3/3] hostap: Delete unnecessary initialisations for the variable "ret"
>> 
>> Reason: The benefit is not clear.
>
> How do you think about to reduce the source code a bit at these places?

hostap is an obsolete driver, it's waste of time doing style fixes to it
as nobody maintains it anymore.

-- 
Kalle Valo

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[SF Markus Elfring]

> hostap is an obsolete driver, it's waste of time doing style fixes to it
> as nobody maintains it anymore.

Thanks for another bit of your software development attention and this information.

Is it easier to understand than the previous response "Reason: The benefit is not clear."?

Regards,
Markus

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Joe Perches]

On Mon, 2016-09-26 at 21:01 +0300, Kalle Valo wrote:
> hostap is an obsolete driver, it's waste of time doing style fixes to it
> as nobody maintains it anymore.

Dunno know if Jouni is still maintaining this at all
but maybe a MAINTAINERS update to mark it obsolete so
checkpatch warns on unnecessary changes.
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6e0a912c3b13..ff293e70fae6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5722,7 +5722,7 @@ M:	Jouni Malinen 
 L:	hostap@shmoo.com (subscribers-only)
 L:	linux-wireless@vger.kernel.org
 W:	http://hostap.epitest.fi/
-S:	Maintained
+S:	Maintained / Obsolete
 F:	drivers/net/wireless/intersil/hostap/
 
 HP COMPAQ TC1100 TABLET WMI EXTRAS DRIVER

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Kalle Valo]

Joe Perches <joe@perches.com> writes:

> On Mon, 2016-09-26 at 21:01 +0300, Kalle Valo wrote:
>> hostap is an obsolete driver, it's waste of time doing style fixes to it
>> as nobody maintains it anymore.
>
> Dunno know if Jouni is still maintaining this at all
> but maybe a MAINTAINERS update to mark it obsolete so
> checkpatch warns on unnecessary changes.
> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6e0a912c3b13..ff293e70fae6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5722,7 +5722,7 @@ M:	Jouni Malinen 
>  L:	hostap@shmoo.com (subscribers-only)
>  L:	linux-wireless@vger.kernel.org
>  W:	http://hostap.epitest.fi/
> -S:	Maintained
> +S:	Maintained / Obsolete
>  F:	drivers/net/wireless/intersil/hostap/

I talked with Jouni and we concluded marking this fully obsolete is the
best (so removing the "Maintained" part completely). Also the shmoo list
is not used anymore, that can be removed.

-- 
Kalle Valo

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Joe Perches]

On Mon, 2016-09-26 at 21:37 +0300, Kalle Valo wrote:
> I talked with Jouni and we concluded marking this fully obsolete is the
> best (so removing the "Maintained" part completelo the shmoo list
> is not used anymore, that can be removed.

Well, it would be best if Jouni submitted something.