[PATCH 0/3] ath9k: Fix smatch warnings

[PATCH 0/3] ath9k: Fix smatch warnings

[Rajkumar Manoharan]

Hi,

Here is the another set of smatch warning fixes.
Now ath9k is smatch warning free code :)

Rajkumar Manoharan (3):
  ath9k_hw: fix buffer overflow smatch warning
  ath9k_hw: fix smatch warnings in spur_mitigate
  ath9k: fix 'side effect in macro' smatch warning

 drivers/net/wireless/ath/ath9k/ar5008_phy.c   | 7 ++++---
 drivers/net/wireless/ath/ath9k/ar9002_phy.c   | 9 +++------
 drivers/net/wireless/ath/ath9k/ar9003_calib.c | 2 +-
 drivers/net/wireless/ath/ath9k/btcoex.c       | 2 +-
 drivers/net/wireless/ath/ath9k/main.c         | 9 +++++----
 5 files changed, 14 insertions(+), 15 deletions(-)

-- 
1.7.11

[PATCH 1/3] ath9k_hw: fix buffer overflow smatch warning

[Rajkumar Manoharan]

drivers/net/wireless/ath/ath9k/btcoex.c:93
  ath9k_hw_init_btcoex_hw() error: buffer overflow
  'ah->hw_gen_timers.gen_timer_index' 32 <= 2009813776

Signed-off-by: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/btcoex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/btcoex.c b/drivers/net/wireless/ath/ath9k/btcoex.c
index acd4373..f06477a 100644
--- a/drivers/net/wireless/ath/ath9k/btcoex.c
+++ b/drivers/net/wireless/ath/ath9k/btcoex.c
@@ -89,7 +89,7 @@ void ath9k_hw_init_btcoex_hw(struct ath_hw *ah, int qnum)
 		AR_BT_DISABLE_BT_ANT;
 
 	for (i = 0; i < 32; i++) {
-		idx = (debruijn32 << i) >> 27;
+		idx = ((debruijn32 << i) >> 27) % 32;
 		ah->hw_gen_timers.gen_timer_index[idx] = i;
 	}
 }
-- 
1.7.11

[PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate

[Rajkumar Manoharan]

This patch fixes below smatch warnings

drivers/net/wireless/ath/ath9k/ar9002_phy.c:275
ar9002_hw_spur_mitigate() Error invalid range 576717 to 471859
drivers/net/wireless/ath/ath9k/ar5008_phy.c:323
ar5008_hw_spur_mitigate() Error invalid range 555746 to 492830
drivers/net/wireless/ath/ath9k/ar5008_phy.c:326
ar5008_hw_spur_mitigate() Error invalid range 587 to 481
drivers/net/wireless/ath/ath9k/ar9003_calib.c:272
ar9003_hw_iqcalibrate() Error invalid range 65 to 63

Signed-off-by: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/ar5008_phy.c   | 7 ++++---
 drivers/net/wireless/ath/ath9k/ar9002_phy.c   | 9 +++------
 drivers/net/wireless/ath/ath9k/ar9003_calib.c | 2 +-
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar5008_phy.c b/drivers/net/wireless/ath/ath9k/ar5008_phy.c
index 874186b..4130035 100644
--- a/drivers/net/wireless/ath/ath9k/ar5008_phy.c
+++ b/drivers/net/wireless/ath/ath9k/ar5008_phy.c
@@ -319,11 +319,12 @@ static void ar5008_hw_spur_mitigate(struct ath_hw *ah,
 	       SM(SPUR_RSSI_THRESH, AR_PHY_SPUR_REG_SPUR_RSSI_THRESH));
 	REG_WRITE(ah, AR_PHY_SPUR_REG, new);
 
-	spur_delta_phase = ((bb_spur * 524288) / 100) &
-		AR_PHY_TIMING11_SPUR_DELTA_PHASE;
+	spur_delta_phase = (bb_spur * 524288) / 100;
+	spur_delta_phase &= AR_PHY_TIMING11_SPUR_DELTA_PHASE;
 
 	denominator = IS_CHAN_2GHZ(chan) ? 440 : 400;
-	spur_freq_sd = ((bb_spur * 2048) / denominator) & 0x3ff;
+	spur_freq_sd = (bb_spur * 2048) / denominator;
+	spur_freq_sd &= 0x3ff;
 
 	new = (AR_PHY_TIMING11_USE_SPUR_IN_AGC |
 	       SM(spur_freq_sd, AR_PHY_TIMING11_SPUR_FREQ_SD) |
diff --git a/drivers/net/wireless/ath/ath9k/ar9002_phy.c b/drivers/net/wireless/ath/ath9k/ar9002_phy.c
index 846dd79..58b9ca0 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_phy.c
+++ b/drivers/net/wireless/ath/ath9k/ar9002_phy.c
@@ -270,13 +270,10 @@ static void ar9002_hw_spur_mitigate(struct ath_hw *ah,
 	}
 
 	if (IS_CHAN_HT40(chan))
-		spur_delta_phase =
-			((bb_spur * 262144) /
-			 10) & AR_PHY_TIMING11_SPUR_DELTA_PHASE;
+		spur_delta_phase = (bb_spur * 262144) / 10;
 	else
-		spur_delta_phase =
-			((bb_spur * 524288) /
-			 10) & AR_PHY_TIMING11_SPUR_DELTA_PHASE;
+		spur_delta_phase = (bb_spur * 524288) / 10;
+	spur_delta_phase &= AR_PHY_TIMING11_SPUR_DELTA_PHASE;
 
 	denominator = IS_CHAN_2GHZ(chan) ? 44 : 40;
 	spur_freq_sd = ((bb_spur_off * 2048) / denominator) & 0x3ff;
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_calib.c b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
index d7deb8c..8bee934 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_calib.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
@@ -269,7 +269,7 @@ static void ar9003_hw_iqcalibrate(struct ath_hw *ah, u8 numChains)
 				qCoff = -63;
 
 			iCoff = iCoff & 0x7f;
-			qCoff = qCoff & 0x7f;
+			qCoff &= 0x7f;
 
 			ath_dbg(common, CALIBRATE,
 				"Chn %d : iCoff = 0x%x  qCoff = 0x%x\n",
-- 
1.7.11

[PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning

[Rajkumar Manoharan]

ath9k_get_et_stats() warn: side effect in macro
'AWDATA' doing 'i++'

Cc: Ben Greear <greearb@candelatech.com>
Signed-off-by: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 85f9ab4..32474b0 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -2003,10 +2003,11 @@ static int ath9k_get_et_sset_count(struct ieee80211_hw *hw,
 #define PR_QNUM(_n) (sc->tx.txq_map[_n]->axq_qnum)
 #define AWDATA(elem)							\
 	do {								\
-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
+		data[i+0] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
+		data[i+1] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
+		data[i+2] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
+		data[i+3] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
+		i += 4; \
 	} while (0)
 
 #define AWDATA_RX(elem)						\
-- 
1.7.11

Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning

[Ben Greear]

On 06/21/2012 11:31 AM, Rajkumar Manoharan wrote:
> ath9k_get_et_stats() warn: side effect in macro
> 'AWDATA' doing 'i++'
>
> Cc: Ben Greear<greearb@candelatech.com>
> Signed-off-by: Rajkumar Manoharan<rmanohar@qca.qualcomm.com>
> ---
>   drivers/net/wireless/ath/ath9k/main.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> index 85f9ab4..32474b0 100644
> --- a/drivers/net/wireless/ath/ath9k/main.c
> +++ b/drivers/net/wireless/ath/ath9k/main.c
> @@ -2003,10 +2003,11 @@ static int ath9k_get_et_sset_count(struct ieee80211_hw *hw,
>   #define PR_QNUM(_n) (sc->tx.txq_map[_n]->axq_qnum)
>   #define AWDATA(elem)							\
>   	do {								\
> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> +		data[i+0] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> +		data[i+1] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> +		data[i+2] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> +		data[i+3] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> +		i += 4; \
>   	} while (0)

The macro is still changing i.  So, whatever smatch is, seems it
should still warn, or it's broken :P

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning

[Rajkumar Manoharan]

On Thu, Jun 21, 2012 at 11:42:53AM -0700, Ben Greear wrote:
> On 06/21/2012 11:31 AM, Rajkumar Manoharan wrote:
> >ath9k_get_et_stats() warn: side effect in macro
> >'AWDATA' doing 'i++'
> >
> >Cc: Ben Greear<greearb@candelatech.com>
> >Signed-off-by: Rajkumar Manoharan<rmanohar@qca.qualcomm.com>
> >---
> >  drivers/net/wireless/ath/ath9k/main.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> >index 85f9ab4..32474b0 100644
> >--- a/drivers/net/wireless/ath/ath9k/main.c
> >+++ b/drivers/net/wireless/ath/ath9k/main.c
> >@@ -2003,10 +2003,11 @@ static int ath9k_get_et_sset_count(struct ieee80211_hw *hw,
> >  #define PR_QNUM(_n) (sc->tx.txq_map[_n]->axq_qnum)
> >  #define AWDATA(elem)							\
> >  	do {								\
> >-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> >-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> >-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> >-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> >+		data[i+0] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> >+		data[i+1] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> >+		data[i+2] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> >+		data[i+3] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> >+		i += 4; \
> >  	} while (0)
> 
> The macro is still changing i.  So, whatever smatch is, seems it
> should still warn, or it's broken :P
>
No it is not. The warning message is a hint. The smatch assumes that replacing
the macro 'i++' might cause unexpected behaviour like 5++ in each statement.

-Rajkumar

Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning

[Ben Greear]

On 06/21/2012 12:07 PM, Rajkumar Manoharan wrote:
> On Thu, Jun 21, 2012 at 11:42:53AM -0700, Ben Greear wrote:
>> On 06/21/2012 11:31 AM, Rajkumar Manoharan wrote:
>>> ath9k_get_et_stats() warn: side effect in macro
>>> 'AWDATA' doing 'i++'
>>>
>>> Cc: Ben Greear<greearb@candelatech.com>
>>> Signed-off-by: Rajkumar Manoharan<rmanohar@qca.qualcomm.com>
>>> ---
>>>   drivers/net/wireless/ath/ath9k/main.c | 9 +++++----
>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
>>> index 85f9ab4..32474b0 100644
>>> --- a/drivers/net/wireless/ath/ath9k/main.c
>>> +++ b/drivers/net/wireless/ath/ath9k/main.c
>>> @@ -2003,10 +2003,11 @@ static int ath9k_get_et_sset_count(struct ieee80211_hw *hw,
>>>   #define PR_QNUM(_n) (sc->tx.txq_map[_n]->axq_qnum)
>>>   #define AWDATA(elem)							\
>>>   	do {								\
>>> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
>>> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
>>> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
>>> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
>>> +		data[i+0] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
>>> +		data[i+1] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
>>> +		data[i+2] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
>>> +		data[i+3] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
>>> +		i += 4; \
>>>   	} while (0)
>>
>> The macro is still changing i.  So, whatever smatch is, seems it
>> should still warn, or it's broken :P
>>
> No it is not. The warning message is a hint. The smatch assumes that replacing
> the macro 'i++' might cause unexpected behaviour like 5++ in each statement.

Well, my opinion is that your patch only adds un-needed code and that
smatch is either currently giving false-positives, or that it is
missing a warning when you add your patch.

But, not a big deal either way.

Thanks,
Ben

>
> -Rajkumar


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning

[Rajkumar Manoharan]

On Thu, Jun 21, 2012 at 12:15:26PM -0700, Ben Greear wrote:
> On 06/21/2012 12:07 PM, Rajkumar Manoharan wrote:
> >On Thu, Jun 21, 2012 at 11:42:53AM -0700, Ben Greear wrote:
> >>On 06/21/2012 11:31 AM, Rajkumar Manoharan wrote:
> >>>ath9k_get_et_stats() warn: side effect in macro
> >>>'AWDATA' doing 'i++'
> >>>
> >>>Cc: Ben Greear<greearb@candelatech.com>
> >>>Signed-off-by: Rajkumar Manoharan<rmanohar@qca.qualcomm.com>
> >>>---
> >>>  drivers/net/wireless/ath/ath9k/main.c | 9 +++++----
> >>>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> >>>index 85f9ab4..32474b0 100644
> >>>--- a/drivers/net/wireless/ath/ath9k/main.c
> >>>+++ b/drivers/net/wireless/ath/ath9k/main.c
> >>>@@ -2003,10 +2003,11 @@ static int ath9k_get_et_sset_count(struct ieee80211_hw *hw,
> >>>  #define PR_QNUM(_n) (sc->tx.txq_map[_n]->axq_qnum)
> >>>  #define AWDATA(elem)							\
> >>>  	do {								\
> >>>-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> >>>-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> >>>-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> >>>-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> >>>+		data[i+0] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> >>>+		data[i+1] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> >>>+		data[i+2] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> >>>+		data[i+3] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> >>>+		i += 4; \
> >>>  	} while (0)
> >>
> >>The macro is still changing i.  So, whatever smatch is, seems it
> >>should still warn, or it's broken :P
> >>
> >No it is not. The warning message is a hint. The smatch assumes that replacing
> >the macro 'i++' might cause unexpected behaviour like 5++ in each statement.
> 
> Well, my opinion is that your patch only adds un-needed code and that
> smatch is either currently giving false-positives, or that it is
> missing a warning when you add your patch.
> 
> But, not a big deal either way.
> 
Thanks. Most of my smatch fixes could address false-positives. Anyway i prefer
warning free code :)

-Rajkumar

Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning

[Kalle Valo]

Rajkumar Manoharan <rmanohar@qca.qualcomm.com> writes:

> ath9k_get_et_stats() warn: side effect in macro
> 'AWDATA' doing 'i++'
>
> Cc: Ben Greear <greearb@candelatech.com>
> Signed-off-by: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>

[...]

>  	do {								\
> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> +		data[i+0] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> +		data[i+1] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> +		data[i+2] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> +		data[i+3] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> +		i += 4; \
>  	} while (0)

I agree with Ben, this is a useless change as the end result is still
the same (the side effect is that i is increased with four). You are
just hiding that from smatch and once smatch is fixed it will warn again
about the same thing.

I recommend fixing this properly so that the macro doesn't have any side
effects.

-- 
Kalle Valo

Re: [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate

[Kalle Valo]

Rajkumar Manoharan <rmanohar@qca.qualcomm.com> writes:

> This patch fixes below smatch warnings
>
> drivers/net/wireless/ath/ath9k/ar9002_phy.c:275
> ar9002_hw_spur_mitigate() Error invalid range 576717 to 471859
> drivers/net/wireless/ath/ath9k/ar5008_phy.c:323
> ar5008_hw_spur_mitigate() Error invalid range 555746 to 492830
> drivers/net/wireless/ath/ath9k/ar5008_phy.c:326
> ar5008_hw_spur_mitigate() Error invalid range 587 to 481
> drivers/net/wireless/ath/ath9k/ar9003_calib.c:272
> ar9003_hw_iqcalibrate() Error invalid range 65 to 63
>
> Signed-off-by: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>

[...]

> -			qCoff = qCoff & 0x7f;
> +			qCoff &= 0x7f;

I'm curious, how does a change like this fix anything? To me it just
looks same functionality, just a different operator is used. Am I
missing something?

-- 
Kalle Valo

Re: [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate

[Rajkumar Manoharan]

On Fri, Jun 22, 2012 at 10:23:44AM +0300, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qca.qualcomm.com> writes:
> 
> > This patch fixes below smatch warnings
> >
> > drivers/net/wireless/ath/ath9k/ar9003_calib.c:272
> > ar9003_hw_iqcalibrate() Error invalid range 65 to 63
> >
> > Signed-off-by: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>
> 
> [...]
> 
> > -			qCoff = qCoff & 0x7f;
> > +			qCoff &= 0x7f;
> 
> I'm curious, how does a change like this fix anything? To me it just
> looks same functionality, just a different operator is used. Am I
> missing something?

Though it doesn't make a difference, somehow it fixes smatch warning.
I agree it is not the right fix.

I just found the actual issue here that the min and max limitation
are checked before masking out the value.

   if (qCoff >= 63)
           qCoff = 63;
   else if (qCoff <= -63)
           qCoff = -63;

Later it is masked out with 0x7f.

-63 & 0x7f = 65 which is greater than the max limit 63.

Thats why smatch is throwing warning "Error invalid range 65 to 63".

-Rajkumar 

Re: [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate

[Kalle Valo]

Rajkumar Manoharan <rmanohar@qca.qualcomm.com> writes:

>> > -			qCoff = qCoff & 0x7f;
>> > +			qCoff &= 0x7f;
>> 
>> I'm curious, how does a change like this fix anything? To me it just
>> looks same functionality, just a different operator is used. Am I
>> missing something?
>
> Though it doesn't make a difference, somehow it fixes smatch warning.
> I agree it is not the right fix.

Ok, maybe it's just that smatch isn't clever enough with the &= operator.
Dan?

-- 
Kalle Valo

Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning

[Dan Carpenter]

On Fri, Jun 22, 2012 at 10:17:23AM +0300, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qca.qualcomm.com> writes:
> 
> > ath9k_get_et_stats() warn: side effect in macro
> > 'AWDATA' doing 'i++'
> >
> > Cc: Ben Greear <greearb@candelatech.com>
> > Signed-off-by: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>
> 
> [...]
> 
> >  	do {								\
> > -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> > -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> > -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> > -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> > +		data[i+0] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> > +		data[i+1] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> > +		data[i+2] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> > +		data[i+3] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> > +		i += 4; \
> >  	} while (0)
> 
> I agree with Ben, this is a useless change as the end result is still
> the same (the side effect is that i is increased with four). You are
> just hiding that from smatch and once smatch is fixed it will warn again
> about the same thing.
> 
> I recommend fixing this properly so that the macro doesn't have any side
> effects.

Uh.  Sorry, also I thought I had pushed a fix to add this to the
ignored macro list, but I forgot.  I've pushed it now.

I don't think this check has ever found a bug.  These bugs are very
rare and we are careful about macro side effects in the kernel.  I
have disabled the check by default and it's only enabled when you
pass --spammy.  I told the Ruby people I was going to do this
earlier in the week but I've been out of the office and delayed.

regards,
dan carpenter

Re: [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate

[Dan Carpenter]

On Fri, Jun 22, 2012 at 02:44:14PM +0300, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qca.qualcomm.com> writes:
> 
> >> > -			qCoff = qCoff & 0x7f;
> >> > +			qCoff &= 0x7f;
> >> 
> >> I'm curious, how does a change like this fix anything? To me it just
> >> looks same functionality, just a different operator is used. Am I
> >> missing something?
> >
> > Though it doesn't make a difference, somehow it fixes smatch warning.
> > I agree it is not the right fix.
> 
> Ok, maybe it's just that smatch isn't clever enough with the &= operator.
> Dan?

Ugh...  I think it's debugging code that leaked into the wild.  I
will remove that.

regards,
dan carpenter

Re: [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate

[Rajkumar Manoharan]

On Fri, Jun 22, 2012 at 02:57:47PM +0300, Dan Carpenter wrote:
> On Fri, Jun 22, 2012 at 02:44:14PM +0300, Kalle Valo wrote:
> > Rajkumar Manoharan <rmanohar@qca.qualcomm.com> writes:
> > 
> > >> > -			qCoff = qCoff & 0x7f;
> > >> > +			qCoff &= 0x7f;
> > >> 
> > >> I'm curious, how does a change like this fix anything? To me it just
> > >> looks same functionality, just a different operator is used. Am I
> > >> missing something?
> > >
> > > Though it doesn't make a difference, somehow it fixes smatch warning.
> > > I agree it is not the right fix.
> > 
> > Ok, maybe it's just that smatch isn't clever enough with the &= operator.
> > Dan?
> 
> Ugh...  I think it's debugging code that leaked into the wild.  I
> will remove that.
>
Thanks Dan. I'll rerun smatch once you pushed the changes.

-Rajkumar

Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning

[Ben Greear]

On 06/22/2012 04:55 AM, Dan Carpenter wrote:
> On Fri, Jun 22, 2012 at 10:17:23AM +0300, Kalle Valo wrote:
>> Rajkumar Manoharan<rmanohar@qca.qualcomm.com>  writes:
>>
>>> ath9k_get_et_stats() warn: side effect in macro
>>> 'AWDATA' doing 'i++'
>>>
>>> Cc: Ben Greear<greearb@candelatech.com>
>>> Signed-off-by: Rajkumar Manoharan<rmanohar@qca.qualcomm.com>
>>
>> [...]
>>
>>>   	do {								\
>>> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
>>> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
>>> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
>>> -		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
>>> +		data[i+0] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
>>> +		data[i+1] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
>>> +		data[i+2] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
>>> +		data[i+3] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
>>> +		i += 4; \
>>>   	} while (0)
>>
>> I agree with Ben, this is a useless change as the end result is still
>> the same (the side effect is that i is increased with four). You are
>> just hiding that from smatch and once smatch is fixed it will warn again
>> about the same thing.
>>
>> I recommend fixing this properly so that the macro doesn't have any side
>> effects.
>
> Uh.  Sorry, also I thought I had pushed a fix to add this to the
> ignored macro list, but I forgot.  I've pushed it now.

The whole point of the macro is to have an affect.  This is not a general
purpose macro..it's very specific to this local logic.

I don't think folks should be so dogmatic about these sorts of things.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning

[Dan Carpenter]

On Fri, Jun 22, 2012 at 07:24:54AM -0700, Ben Greear wrote:
> On 06/22/2012 04:55 AM, Dan Carpenter wrote:
> >On Fri, Jun 22, 2012 at 10:17:23AM +0300, Kalle Valo wrote:
> >>Rajkumar Manoharan<rmanohar@qca.qualcomm.com>  writes:
> >>
> >>>ath9k_get_et_stats() warn: side effect in macro
> >>>'AWDATA' doing 'i++'
> >>>
> >>>Cc: Ben Greear<greearb@candelatech.com>
> >>>Signed-off-by: Rajkumar Manoharan<rmanohar@qca.qualcomm.com>
> >>
> >>[...]
> >>
> >>>  	do {								\
> >>>-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> >>>-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> >>>-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> >>>-		data[i++] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> >>>+		data[i+0] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BE)].elem; \
> >>>+		data[i+1] = sc->debug.stats.txstats[PR_QNUM(WME_AC_BK)].elem; \
> >>>+		data[i+2] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VI)].elem; \
> >>>+		data[i+3] = sc->debug.stats.txstats[PR_QNUM(WME_AC_VO)].elem; \
> >>>+		i += 4; \
> >>>  	} while (0)
> >>
> >>I agree with Ben, this is a useless change as the end result is still
> >>the same (the side effect is that i is increased with four). You are
> >>just hiding that from smatch and once smatch is fixed it will warn again
> >>about the same thing.
> >>
> >>I recommend fixing this properly so that the macro doesn't have any side
> >>effects.
> >
> >Uh.  Sorry, also I thought I had pushed a fix to add this to the
> >ignored macro list, but I forgot.  I've pushed it now.
> 
> The whole point of the macro is to have an affect.  This is not a general
> purpose macro..it's very specific to this local logic.
> 
> I don't think folks should be so dogmatic about these sorts of things.
> 

Right.  I have a list of macros which are expected to have side
effects but this one was only merged into linux-next recently and
had not been added to my list.

regards,
dan carpenter