[PATCH 0/6] atheros-wireless: fix sparse warnings ho ho ho

[PATCH 0/6] atheros-wireless: fix sparse warnings ho ho ho

[Luis R. Rodriguez]

From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>

Here's a slew of sparse warning fixes for all Atheros wireless drivers.
These have been itching me for a while now. I ask all patch submitters
and driver maintainers to please run their patches with these two tests:

make C=1 M=drivers/net/wireless/ath/

And then if all is happy try:

make C=2 CF="-D__CHECK_ENDIAN__" M=drivers/net/wireless/ath/

Luis R. Rodriguez (6):
  ath6kl: fix sparse warning on init.c
  ath9k: make ath_mci_duty_cycle static
  ath9k_hw: fix sparse warnings on ar9003_rtt.c
  ath9k: fix tx queue sparse complaint
  ath5k: avoid sparse warnings on tracing
  ath9k_hw: fix sparse complaint on ar9003_switch_com_spdt_get()

 drivers/net/wireless/ath/ath5k/trace.h         |    5 +++--
 drivers/net/wireless/ath/ath6kl/init.c         |    5 ++++-
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c |    4 ++--
 drivers/net/wireless/ath/ath9k/ar9003_rtt.c    |    1 +
 drivers/net/wireless/ath/ath9k/mci.c           |    2 +-
 drivers/net/wireless/ath/ath9k/xmit.c          |    3 +++
 6 files changed, 14 insertions(+), 6 deletions(-)

-- 
1.7.4.15.g7811d

[PATCH 1/6] ath6kl: fix sparse warning on init.c

[Luis R. Rodriguez]

From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>

This fixes this sparse warning:

  CC [M]  drivers/net/wireless/ath/ath6kl/init.o
drivers/net/wireless/ath/ath6kl/init.c: In function ‘ath6kl_init_hw_params’:
drivers/net/wireless/ath/ath6kl/init.c:1377:26: warning: ‘hw’ may be used uninitialized in this function

Signed-off-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/init.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/init.c b/drivers/net/wireless/ath/ath6kl/init.c
index 368ecbd..ef3641b 100644
--- a/drivers/net/wireless/ath/ath6kl/init.c
+++ b/drivers/net/wireless/ath/ath6kl/init.c
@@ -1374,7 +1374,7 @@ static int ath6kl_init_upload(struct ath6kl *ar)
 
 static int ath6kl_init_hw_params(struct ath6kl *ar)
 {
-	const struct ath6kl_hw *hw;
+	const struct ath6kl_hw *hw = NULL;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
@@ -1390,6 +1390,9 @@ static int ath6kl_init_hw_params(struct ath6kl *ar)
 		return -EINVAL;
 	}
 
+	if (!hw)
+		return -EINVAL;
+
 	ar->hw = *hw;
 
 	ath6kl_dbg(ATH6KL_DBG_BOOT,
-- 
1.7.4.15.g7811d

[PATCH 2/6] ath9k: make ath_mci_duty_cycle static

[Luis R. Rodriguez]

From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>

This fixes this sparse warning:

  CHECK   drivers/net/wireless/ath/ath9k/mci.c
drivers/net/wireless/ath/ath9k/mci.c:23:4: warning: symbol 'ath_mci_duty_cycle' was not declared. Should it be static?

Signed-off-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/mci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/mci.c b/drivers/net/wireless/ath/ath9k/mci.c
index fee8c6f..15629f7 100644
--- a/drivers/net/wireless/ath/ath9k/mci.c
+++ b/drivers/net/wireless/ath/ath9k/mci.c
@@ -20,7 +20,7 @@
 #include "ath9k.h"
 #include "mci.h"
 
-u8 ath_mci_duty_cycle[] = { 0, 50, 60, 70, 80, 85, 90, 95, 98 };
+static u8 ath_mci_duty_cycle[] = { 0, 50, 60, 70, 80, 85, 90, 95, 98 };
 
 static struct ath_mci_profile_info*
 ath_mci_find_profile(struct ath_mci_profile *mci,
-- 
1.7.4.15.g7811d

[PATCH 3/6] ath9k_hw: fix sparse warnings on ar9003_rtt.c

[Luis R. Rodriguez]

From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>

This fixes these sparse warnings:

  CHECK   drivers/net/wireless/ath/ath9k/ar9003_rtt.c
drivers/net/wireless/ath/ath9k/ar9003_rtt.c:36:6: warning: symbol 'ar9003_hw_rtt_enable' was not declared. Should it be static?
drivers/net/wireless/ath/ath9k/ar9003_rtt.c:41:6: warning: symbol 'ar9003_hw_rtt_disable' was not declared. Should it be static?
drivers/net/wireless/ath/ath9k/ar9003_rtt.c:46:6: warning: symbol 'ar9003_hw_rtt_set_mask' was not declared. Should it be static?
drivers/net/wireless/ath/ath9k/ar9003_rtt.c:52:6: warning: symbol 'ar9003_hw_rtt_force_restore' was not declared. Should it be static?
drivers/net/wireless/ath/ath9k/ar9003_rtt.c:102:6: warning: symbol 'ar9003_hw_rtt_load_hist' was not declared. Should it be static?
drivers/net/wireless/ath/ath9k/ar9003_rtt.c:135:6: warning: symbol 'ar9003_hw_rtt_fill_hist' was not declared. Should it be static?
drivers/net/wireless/ath/ath9k/ar9003_rtt.c:143:6: warning: symbol 'ar9003_hw_rtt_clear_hist' was not declared. Should it be stati

Signed-off-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/ar9003_rtt.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9003_rtt.c b/drivers/net/wireless/ath/ath9k/ar9003_rtt.c
index 48803ee..458bedf 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_rtt.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_rtt.c
@@ -16,6 +16,7 @@
 
 #include "hw.h"
 #include "ar9003_phy.h"
+#include "ar9003_rtt.h"
 
 #define RTT_RESTORE_TIMEOUT          1000
 #define RTT_ACCESS_TIMEOUT           100
-- 
1.7.4.15.g7811d

[PATCH 4/6] ath9k: fix tx queue sparse complaint

[Luis R. Rodriguez]

From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>

This fixes this rant from sparse:

  CHECK   drivers/net/wireless/ath/ath9k/xmit.c
drivers/net/wireless/ath/ath9k/xmit.c:107:13: warning: context imbalance in 'ath_txq_lock' - wrong count at exit
drivers/net/wireless/ath/ath9k/xmit.c:112:13: warning: context imbalance in 'ath_txq_unlock' - unexpected unlock
drivers/net/wireless/ath/ath9k/xmit.c:123:30: warning: context imbalance in 'ath_txq_unlock_complete' - unexpected unlock
  CC [M]  drivers/net/wireless/ath/ath9k/xmit.

Signed-off-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/xmit.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index b092523..c8fc180 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -105,16 +105,19 @@ static int ath_max_4ms_framelen[4][32] = {
 /*********************/
 
 static void ath_txq_lock(struct ath_softc *sc, struct ath_txq *txq)
+	__acquires(&txq->axq_lock)
 {
 	spin_lock_bh(&txq->axq_lock);
 }
 
 static void ath_txq_unlock(struct ath_softc *sc, struct ath_txq *txq)
+	__releases(&txq->axq_lock)
 {
 	spin_unlock_bh(&txq->axq_lock);
 }
 
 static void ath_txq_unlock_complete(struct ath_softc *sc, struct ath_txq *txq)
+	__releases(&txq->axq_lock)
 {
 	struct sk_buff_head q;
 	struct sk_buff *skb;
-- 
1.7.4.15.g7811d

[PATCH 5/6] ath5k: avoid sparse warnings on tracing

[Luis R. Rodriguez]

From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>

Just skip the sparse checks on tracing.

  CHECK   drivers/net/wireless/ath/ath5k/base.c
include/trace/../../drivers/net/wireless/ath/ath5k/trace.h:19:1: error: incompatible types for operation (<)
include/trace/../../drivers/net/wireless/ath/ath5k/trace.h:19:1:    left side has type struct ath5k_hw *<noident>
include/trace/../../drivers/net/wireless/ath/ath5k/trace.h:19:1:    right side has type int
include/trace/../../drivers/net/wireless/ath/ath5k/trace.h:37:1: error: incompatible types for operation (<)
include/trace/../../drivers/net/wireless/ath/ath5k/trace.h:37:1:    left side has type struct ath5k_hw *<noident>
include/trace/../../drivers/net/wireless/ath/ath5k/trace.h:37:1:    right side has type int
include/trace/../../drivers/net/wireless/ath/ath5k/trace.h:63:1: error: incompatible types for operation (<)
include/trace/../../drivers/net/wireless/ath/ath5k/trace.h:63:1:    left side has type struct ath5k_hw *<noident>
include/trace/../../drivers/net/wireless/ath/ath5k/trace.h:63:1:    right side has type int
/home/mcgrof/wireless-testing/arch/x86/include/asm/jump_label.h:16:9: error: bad asm output
/home/mcgrof/wireless-testing/arch/x86/include/asm/jump_label.h:16:9: error: bad asm output
/home/mcgrof/wireless-testing/arch/x86/include/asm/jump_label.h:16:9: error: bad asm output
/home/mcgrof/wireless-testing/arch/x86/include/asm/jump_label.h:16:9: error: bad asm output
  CC [M]  drivers/net/wireless/ath/ath5k/base.o

Signed-off-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath5k/trace.h |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/trace.h b/drivers/net/wireless/ath/ath5k/trace.h
index 39f002e..00f0158 100644
--- a/drivers/net/wireless/ath/ath5k/trace.h
+++ b/drivers/net/wireless/ath/ath5k/trace.h
@@ -3,7 +3,8 @@
 
 #include <linux/tracepoint.h>
 
-#ifndef CONFIG_ATH5K_TRACER
+
+#if !defined(CONFIG_ATH5K_TRACER) || defined(__CHECKER__)
 #undef TRACE_EVENT
 #define TRACE_EVENT(name, proto, ...) \
 static inline void trace_ ## name(proto) {}
@@ -93,7 +94,7 @@ TRACE_EVENT(ath5k_tx_complete,
 
 #endif /* __TRACE_ATH5K_H */
 
-#ifdef CONFIG_ATH5K_TRACER
+#if defined(CONFIG_ATH5K_TRACER) && !defined(__CHECKER__)
 
 #undef TRACE_INCLUDE_PATH
 #define TRACE_INCLUDE_PATH ../../drivers/net/wireless/ath/ath5k
-- 
1.7.4.15.g7811d

[PATCH 6/6] ath9k_hw: fix sparse complaint on ar9003_switch_com_spdt_get()

[Luis R. Rodriguez]

From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>

This fixes this sparse complaint:

make C=2 CF="-D__CHECK_ENDIAN__" M=drivers/net/wireless/ath/

  CHECK   drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
drivers/net/wireless/ath/ath9k/ar9003_eeprom.c:3544:21: warning: incorrect type in assignment (different base types)
drivers/net/wireless/ath/ath9k/ar9003_eeprom.c:3544:21:    expected restricted __le32 [usertype] val
drivers/net/wireless/ath/ath9k/ar9003_eeprom.c:3544:21:    got restricted __le16 [usertype] switchcomspdt
drivers/net/wireless/ath/ath9k/ar9003_eeprom.c:3546:21: warning: incorrect type in assignment (different base types)
drivers/net/wireless/ath/ath9k/ar9003_eeprom.c:3546:21:    expected restricted __le32 [usertype] val
drivers/net/wireless/ath/ath9k/ar9003_eeprom.c:3546:21:    got restricted __le16 [usertype] switchcomspdt

The eep->modalHeader5G.switchcomspdt is a le16 and we return u16,
so just return le16_to_cpu().

Cc: Felix Fietkau <nbd@openwrt.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
index 391def9..9fbcbdd 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
@@ -3538,13 +3538,13 @@ static void ar9003_hw_xpa_bias_level_apply(struct ath_hw *ah, bool is2ghz)
 static u16 ar9003_switch_com_spdt_get(struct ath_hw *ah, bool is_2ghz)
 {
 	struct ar9300_eeprom *eep = &ah->eeprom.ar9300_eep;
-	__le32 val;
+	__le16 val;
 
 	if (is_2ghz)
 		val = eep->modalHeader2G.switchcomspdt;
 	else
 		val = eep->modalHeader5G.switchcomspdt;
-	return le32_to_cpu(val);
+	return le16_to_cpu(val);
 }
 
 
-- 
1.7.4.15.g7811d

Re: [PATCH 1/6] ath6kl: fix sparse warning on init.c

[Luis R. Rodriguez]

On Tue, Dec 20, 2011 at 10:46 AM, Luis R. Rodriguez
<rodrigue@qca.qualcomm.com> wrote:
> From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
>
> This fixes this sparse warning:
>
>  CC [M]  drivers/net/wireless/ath/ath6kl/init.o
> drivers/net/wireless/ath/ath6kl/init.c: In function ‘ath6kl_init_hw_params’:
> drivers/net/wireless/ath/ath6kl/init.c:1377:26: warning: ‘hw’ may be used uninitialized in this function
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>

Not sure if Kalle will suck this in or if John will.

  Luis

Re: [PATCH 0/6] atheros-wireless: fix sparse warnings ho ho ho

[Larry Finger]

On 12/20/2011 12:46 PM, Luis R. Rodriguez wrote:
> From: Luis R. Rodriguez<mcgrof@qca.qualcomm.com>
>
> Here's a slew of sparse warning fixes for all Atheros wireless drivers.
> These have been itching me for a while now. I ask all patch submitters
> and driver maintainers to please run their patches with these two tests:
>
> make C=1 M=drivers/net/wireless/ath/
>
> And then if all is happy try:
>
> make C=2 CF="-D__CHECK_ENDIAN__" M=drivers/net/wireless/ath/

Why not follow a suggestion that Johannes made to me? Add the line

ccflags-y += -D__CHECK_ENDIAN__

To the bottom of the make file(s). That way they only need one run. Then enforce 
the rule by not ACKing any patches with too many of the endian warnings, and do 
not allow any of the others.

Larry

Re: [PATCH 0/6] atheros-wireless: fix sparse warnings ho ho ho

[Luis R. Rodriguez]

On Tue, Dec 20, 2011 at 01:01:36PM -0600, Larry Finger wrote:
> On 12/20/2011 12:46 PM, Luis R. Rodriguez wrote:
> >From: Luis R. Rodriguez<mcgrof@qca.qualcomm.com>
> >
> >Here's a slew of sparse warning fixes for all Atheros wireless drivers.
> >These have been itching me for a while now. I ask all patch submitters
> >and driver maintainers to please run their patches with these two tests:
> >
> >make C=1 M=drivers/net/wireless/ath/
> >
> >And then if all is happy try:
> >
> >make C=2 CF="-D__CHECK_ENDIAN__" M=drivers/net/wireless/ath/
> 
> Why not follow a suggestion that Johannes made to me? Add the line
> 
> ccflags-y += -D__CHECK_ENDIAN__
> 
> To the bottom of the make file(s). That way they only need one run.
> Then enforce the rule by not ACKing any patches with too many of the
> endian warnings, and do not allow any of the others.

Awesome idea.

  Luis

Re: [PATCH 1/6] ath6kl: fix sparse warning on init.c

[Kalle Valo]

On 12/20/2011 08:46 PM, Luis R. Rodriguez wrote:
> From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
> 
> This fixes this sparse warning:
> 
>   CC [M]  drivers/net/wireless/ath/ath6kl/init.o
> drivers/net/wireless/ath/ath6kl/init.c: In function ‘ath6kl_init_hw_params’:
> drivers/net/wireless/ath/ath6kl/init.c:1377:26: warning: ‘hw’ may be used uninitialized in this function

That's a compiler warning.

>  static int ath6kl_init_hw_params(struct ath6kl *ar)
>  {
> -	const struct ath6kl_hw *hw;
> +	const struct ath6kl_hw *hw = NULL;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> @@ -1390,6 +1390,9 @@ static int ath6kl_init_hw_params(struct ath6kl *ar)
>  		return -EINVAL;
>  	}
>  
> +	if (!hw)
> +		return -EINVAL;

I can't see how hw can be uninitialised here (looking at the version in
ath6kl.git). I copy the full code here:

static int ath6kl_init_hw_params(struct ath6kl *ar)
{
	const struct ath6kl_hw *hw;
	int i;

	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
		hw = &hw_list[i];

		if (hw->id == ar->version.target_ver)
			break;
	}

	if (i == ARRAY_SIZE(hw_list)) {
		ath6kl_err("Unsupported hardware version: 0x%x\n",
			   ar->version.target_ver);
		return -EINVAL;
	}

	ar->hw = *hw;

I always check for both compiler and sparse warnings and I have never
seen this. What version of compiler do you have?

I have:

gcc-4.5.real (Ubuntu/Linaro 4.5.2-8ubuntu4) 4.5.2

Kalle

Re: [PATCH 1/6] ath6kl: fix sparse warning on init.c

[Luis R. Rodriguez]

On Tue, Dec 20, 2011 at 09:40:19PM +0200, Kalle Valo wrote:
> On 12/20/2011 08:46 PM, Luis R. Rodriguez wrote:
> > From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
> > 
> > This fixes this sparse warning:
> > 
> >   CC [M]  drivers/net/wireless/ath/ath6kl/init.o
> > drivers/net/wireless/ath/ath6kl/init.c: In function ‘ath6kl_init_hw_params’:
> > drivers/net/wireless/ath/ath6kl/init.c:1377:26: warning: ‘hw’ may be used uninitialized in this function
> 
> That's a compiler warning.
> 
> >  static int ath6kl_init_hw_params(struct ath6kl *ar)
> >  {
> > -	const struct ath6kl_hw *hw;
> > +	const struct ath6kl_hw *hw = NULL;
> >  	int i;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> > @@ -1390,6 +1390,9 @@ static int ath6kl_init_hw_params(struct ath6kl *ar)
> >  		return -EINVAL;
> >  	}
> >  
> > +	if (!hw)
> > +		return -EINVAL;
> 
> I can't see how hw can be uninitialised here (looking at the version in
> ath6kl.git). I copy the full code here:
> 
> static int ath6kl_init_hw_params(struct ath6kl *ar)
> {
> 	const struct ath6kl_hw *hw;
> 	int i;
> 
> 	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> 		hw = &hw_list[i];
> 
> 		if (hw->id == ar->version.target_ver)
> 			break;
> 	}
> 
> 	if (i == ARRAY_SIZE(hw_list)) {
> 		ath6kl_err("Unsupported hardware version: 0x%x\n",
> 			   ar->version.target_ver);
> 		return -EINVAL;
> 	}
> 
> 	ar->hw = *hw;
> 
> I always check for both compiler and sparse warnings and I have never
> seen this. What version of compiler do you have?
> 
> I have:
> 
> gcc-4.5.real (Ubuntu/Linaro 4.5.2-8ubuntu4) 4.5.2

Same.

mcgrof@tux ~ $ gcc --version
gcc-4.5.real (Ubuntu/Linaro 4.5.2-8ubuntu4) 4.5.2
Copyright (C) 2010 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE

  Luis

Re: [PATCH 1/6] ath6kl: fix sparse warning on init.c

[Kalle Valo]

On 12/20/2011 09:46 PM, Luis R. Rodriguez wrote:
> On Tue, Dec 20, 2011 at 09:40:19PM +0200, Kalle Valo wrote:

>> I always check for both compiler and sparse warnings and I have never
>> seen this. What version of compiler do you have?
>>
>> I have:
>>
>> gcc-4.5.real (Ubuntu/Linaro 4.5.2-8ubuntu4) 4.5.2
> 
> Same.
> 
> mcgrof@tux ~ $ gcc --version
> gcc-4.5.real (Ubuntu/Linaro 4.5.2-8ubuntu4) 4.5.2

This is odd as I don't see it, even with latest wireless-testing:

$ make M=drivers/net/wireless/ath/ath6kl
  LD      drivers/net/wireless/ath/ath6kl/built-in.o
  CC [M]  drivers/net/wireless/ath/ath6kl/debug.o
  CC [M]  drivers/net/wireless/ath/ath6kl/hif.o
  CC [M]  drivers/net/wireless/ath/ath6kl/htc.o
  CC [M]  drivers/net/wireless/ath/ath6kl/bmi.o
  CC [M]  drivers/net/wireless/ath/ath6kl/cfg80211.o
  CC [M]  drivers/net/wireless/ath/ath6kl/init.o
  CC [M]  drivers/net/wireless/ath/ath6kl/main.o
  CC [M]  drivers/net/wireless/ath/ath6kl/txrx.o
  CC [M]  drivers/net/wireless/ath/ath6kl/wmi.o
  CC [M]  drivers/net/wireless/ath/ath6kl/sdio.o
  CC [M]  drivers/net/wireless/ath/ath6kl/testmode.o
  CC [M]  drivers/net/wireless/ath/ath6kl/usb.o
  LD [M]  drivers/net/wireless/ath/ath6kl/ath6kl_sdio.o
  LD [M]  drivers/net/wireless/ath/ath6kl/ath6kl_usb.o
  Building modules, stage 2.
  MODPOST 2 modules
  CC      drivers/net/wireless/ath/ath6kl/ath6kl_sdio.mod.o
  LD [M]  drivers/net/wireless/ath/ath6kl/ath6kl_sdio.ko
  CC      drivers/net/wireless/ath/ath6kl/ath6kl_usb.mod.o
  LD [M]  drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko

I really would like to understand what's happening here...

But anyway, I think your test is not needed (unless I'm blind again).
Maybe uninitialized_var() is a better choise?

Kalle

Re: [PATCH 1/6] ath6kl: fix sparse warning on init.c

[Joe Perches]

On Tue, 2011-12-20 at 21:40 +0200, Kalle Valo wrote:
> On 12/20/2011 08:46 PM, Luis R. Rodriguez wrote:
> > From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
> > This fixes this sparse warning:
> >   CC [M]  drivers/net/wireless/ath/ath6kl/init.o
> > drivers/net/wireless/ath/ath6kl/init.c: In function ‘ath6kl_init_hw_params’:
> > drivers/net/wireless/ath/ath6kl/init.c:1377:26: warning: ‘hw’ may be used uninitialized in this function
> That's a compiler warning.
[]
> I can't see how hw can be uninitialised here (looking at the version in
> ath6kl.git). I copy the full code here:
> static int ath6kl_init_hw_params(struct ath6kl *ar)
> {
> 	const struct ath6kl_hw *hw;
> 	int i;
> 
> 	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> 		hw = &hw_list[i];
> 
> 		if (hw->id == ar->version.target_ver)
> 			break;
> 	}
> 
> 	if (i == ARRAY_SIZE(hw_list)) {
> 		ath6kl_err("Unsupported hardware version: 0x%x\n",
> 			   ar->version.target_ver);
> 		return -EINVAL;
> 	}
> 
> 	ar->hw = *hw;
> 
> I always check for both compiler and sparse warnings and I have never
> seen this. What version of compiler do you have?

Is the intent here really to allow multiple ids
in the list and match the last one?

If not, perhaps something like this is simpler?

static int ath6kl_init_hw_params(struct ath6kl *ar)
{
	int i;
	const struct ath6kl_hw *hw = hw_list;

	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
		if (hw->id == ar->version.target_ver) {
			ar->hw = *hw;

			ath6kl_dbg(ATH6KL_DBG_BOOT,
				   "target_ver 0x%x target_type 0x%x dataset_patch 0x%x app_load_addr 0x%x\n",
				   ar->version.target_ver, ar->target_type,
				   ar->hw.dataset_patch_addr,
				   ar->hw.app_load_addr);
			ath6kl_dbg(ATH6KL_DBG_BOOT,
				   "app_start_override_addr 0x%x board_ext_data_addr 0x%x reserved_ram_size 0x%x\n",
				   ar->hw.app_start_override_addr,
				   ar->hw.board_ext_data_addr,
				   ar->hw.reserved_ram_size);
			ath6kl_dbg(ATH6KL_DBG_BOOT,
				   "refclk_hz %d uarttx_pin %d\n",
				   ar->hw.refclk_hz, ar->hw.uarttx_pin);

			return 0;
		}
		hw++;
	}

	ath6kl_err("Unsupported hardware version: 0x%x\n",
		   ar->version.target_ver);
	return -EINVAL;
}

btw: there are missing terminating newlines in the
current ath6kl_dbg uses in this routine.

Re: [PATCH 1/6] ath6kl: fix sparse warning on init.c

[Joe Perches]

On Tue, 2011-12-20 at 21:40 +0200, Kalle Valo wrote:
> On 12/20/2011 08:46 PM, Luis R. Rodriguez wrote:
> > From: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
> > This fixes this sparse warning:
> >   CC [M]  drivers/net/wireless/ath/ath6kl/init.o
> > drivers/net/wireless/ath/ath6kl/init.c: In function ‘ath6kl_init_hw_params’:
> > drivers/net/wireless/ath/ath6kl/init.c:1377:26: warning: ‘hw’ may be used uninitialized in this function
> That's a compiler warning.
[]
> I can't see how hw can be uninitialised here (looking at the version in
> ath6kl.git). I copy the full code here:
> static int ath6kl_init_hw_params(struct ath6kl *ar)
> {
> 	const struct ath6kl_hw *hw;
> 	int i;
> 
> 	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> 		hw = &hw_list[i];
> 
> 		if (hw->id == ar->version.target_ver)
> 			break;
> 	}
> 
> 	if (i == ARRAY_SIZE(hw_list)) {
> 		ath6kl_err("Unsupported hardware version: 0x%x\n",
> 			   ar->version.target_ver);
> 		return -EINVAL;
> 	}
> 
> 	ar->hw = *hw;
> 
> I always check for both compiler and sparse warnings and I have never
> seen this. What version of compiler do you have?

Is the intent here really to allow multiple ids
in the list and match the last one?

If not, perhaps this is simpler?

static int ath6kl_init_hw_params(struct ath6kl *ar)
{
	int i;
	const struct ath6kl_hw *hw = hw_list;

	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
		if (hw->id == ar->version.target_ver) {
			ar->hw = *hw;

			ath6kl_dbg(ATH6KL_DBG_BOOT,
				   "target_ver 0x%x target_type 0x%x dataset_patch 0x%x app_load_addr 0x%x\n",
				   ar->version.target_ver, ar->target_type,
				   ar->hw.dataset_patch_addr,
				   ar->hw.app_load_addr);
			ath6kl_dbg(ATH6KL_DBG_BOOT,
				   "app_start_override_addr 0x%x board_ext_data_addr 0x%x reserved_ram_size 0x%x\n",
				   ar->hw.app_start_override_addr,
				   ar->hw.board_ext_data_addr,
				   ar->hw.reserved_ram_size);
			ath6kl_dbg(ATH6KL_DBG_BOOT,
				   "refclk_hz %d uarttx_pin %d\n",
				   ar->hw.refclk_hz, ar->hw.uarttx_pin);

			return 0;
		}
		hw++;
	}

	ath6kl_err("Unsupported hardware version: 0x%x\n",
		   ar->version.target_ver);
	return -EINVAL;
}

btw: there are missing terminating newlines in the
existing ath6kl_dbg uses in this routine.

Re: [PATCH 2/6] ath9k: make ath_mci_duty_cycle static

[Joe Perches]

On Tue, 2011-12-20 at 10:46 -0800, Luis R. Rodriguez wrote:
> diff --git a/drivers/net/wireless/ath/ath9k/mci.c b/drivers/net/wireless/ath/ath9k/mci.c
[]
> @@ -20,7 +20,7 @@
>  #include "ath9k.h"
>  #include "mci.h"
>  
> -u8 ath_mci_duty_cycle[] = { 0, 50, 60, 70, 80, 85, 90, 95, 98 };
> +static u8 ath_mci_duty_cycle[] = { 0, 50, 60, 70, 80, 85, 90, 95, 98 };

static const would be better.

Re: [PATCH 5/6] ath5k: avoid sparse warnings on tracing

[Johannes Berg]

On Tue, 2011-12-20 at 10:46 -0800, Luis R. Rodriguez wrote:

> Just skip the sparse checks on tracing.

I seem to remember removing that elsewhere since newer sparse or newer
tracing code worked fine together.

johannes

Re: [PATCH 5/6] ath5k: avoid sparse warnings on tracing

[Luis R. Rodriguez]

On Wed, Dec 21, 2011 at 7:15 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2011-12-20 at 10:46 -0800, Luis R. Rodriguez wrote:
>
>> Just skip the sparse checks on tracing.
>
> I seem to remember removing that elsewhere since newer sparse or newer
> tracing code worked fine together.

I got the complaint from sparse v0.4.3 and saw that iwlwifi, mac80211
used the same technique.

  Luis

Re: [PATCH 2/6] ath9k: make ath_mci_duty_cycle static

[Luis R. Rodriguez]

On Tue, Dec 20, 2011 at 8:41 PM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2011-12-20 at 10:46 -0800, Luis R. Rodriguez wrote:
>> diff --git a/drivers/net/wireless/ath/ath9k/mci.c b/drivers/net/wireless/ath/ath9k/mci.c
> []
>> @@ -20,7 +20,7 @@
>>  #include "ath9k.h"
>>  #include "mci.h"
>>
>> -u8 ath_mci_duty_cycle[] = { 0, 50, 60, 70, 80, 85, 90, 95, 98 };
>> +static u8 ath_mci_duty_cycle[] = { 0, 50, 60, 70, 80, 85, 90, 95, 98 };
>
> static const would be better.

Good idea, v2 coming up.

  Luis

Re: [PATCH 1/6] ath6kl: fix sparse warning on init.c

[Kalle Valo]

On 12/21/2011 03:42 AM, Joe Perches wrote:
> On Tue, 2011-12-20 at 21:40 +0200, Kalle Valo wrote:
>> On 12/20/2011 08:46 PM, Luis R. Rodriguez wrote:
>>
>> 	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
>> 		hw = &hw_list[i];
>>
>> 		if (hw->id == ar->version.target_ver)
>> 			break;
>> 	}
>>
>> 	if (i == ARRAY_SIZE(hw_list)) {
>> 		ath6kl_err("Unsupported hardware version: 0x%x\n",
>> 			   ar->version.target_ver);
>> 		return -EINVAL;
>> 	}
>>
>> 	ar->hw = *hw;
>>
>> I always check for both compiler and sparse warnings and I have never
>> seen this. What version of compiler do you have?
> 
> Is the intent here really to allow multiple ids
> in the list and match the last one?

The idea is that there should be just one hw entry per id. But if there
would be multiple entires, the code above would take the first one.

> If not, perhaps this is simpler?
> 
> static int ath6kl_init_hw_params(struct ath6kl *ar)
> {
> 	int i;
> 	const struct ath6kl_hw *hw = hw_list;
> 
> 	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> 		if (hw->id == ar->version.target_ver) {
> 			ar->hw = *hw;
> 
> 			ath6kl_dbg(ATH6KL_DBG_BOOT,
> 				   "target_ver 0x%x target_type 0x%x dataset_patch 0x%x app_load_addr 0x%x\n",
> 				   ar->version.target_ver, ar->target_type,
> 				   ar->hw.dataset_patch_addr,
> 				   ar->hw.app_load_addr);
> 			ath6kl_dbg(ATH6KL_DBG_BOOT,
> 				   "app_start_override_addr 0x%x board_ext_data_addr 0x%x reserved_ram_size 0x%x\n",
> 				   ar->hw.app_start_override_addr,
> 				   ar->hw.board_ext_data_addr,
> 				   ar->hw.reserved_ram_size);
> 			ath6kl_dbg(ATH6KL_DBG_BOOT,
> 				   "refclk_hz %d uarttx_pin %d\n",
> 				   ar->hw.refclk_hz, ar->hw.uarttx_pin);
> 
> 			return 0;
> 		}
> 		hw++;
> 	}
> 
> 	ath6kl_err("Unsupported hardware version: 0x%x\n",
> 		   ar->version.target_ver);
> 	return -EINVAL;
> }

There's a lot of indentation in that form which I don't like. And I
would prefer to have the "main" (non-error) code path unindented. Easier
to read that way.

> btw: there are missing terminating newlines in the
> existing ath6kl_dbg uses in this routine.

Thanks, I didn't notice that. That needs to be fixed.

Kalle