* [PATCH 0/3] ath9k: Fix smatch warnings
@ 2012-06-21 18:31 Rajkumar Manoharan
2012-06-21 18:31 ` [PATCH 1/3] ath9k_hw: fix buffer overflow smatch warning Rajkumar Manoharan
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Rajkumar Manoharan @ 2012-06-21 18:31 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, 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
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 1/3] ath9k_hw: fix buffer overflow smatch warning 2012-06-21 18:31 [PATCH 0/3] ath9k: Fix smatch warnings Rajkumar Manoharan @ 2012-06-21 18:31 ` Rajkumar Manoharan 2012-06-21 18:31 ` [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate Rajkumar Manoharan 2012-06-21 18:31 ` [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning Rajkumar Manoharan 2 siblings, 0 replies; 17+ messages in thread From: Rajkumar Manoharan @ 2012-06-21 18:31 UTC (permalink / raw) To: linville; +Cc: linux-wireless, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate 2012-06-21 18:31 [PATCH 0/3] ath9k: Fix smatch warnings Rajkumar Manoharan 2012-06-21 18:31 ` [PATCH 1/3] ath9k_hw: fix buffer overflow smatch warning Rajkumar Manoharan @ 2012-06-21 18:31 ` Rajkumar Manoharan 2012-06-22 7:23 ` Kalle Valo 2012-06-21 18:31 ` [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning Rajkumar Manoharan 2 siblings, 1 reply; 17+ messages in thread From: Rajkumar Manoharan @ 2012-06-21 18:31 UTC (permalink / raw) To: linville; +Cc: linux-wireless, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate 2012-06-21 18:31 ` [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate Rajkumar Manoharan @ 2012-06-22 7:23 ` Kalle Valo 2012-06-22 9:22 ` Rajkumar Manoharan 0 siblings, 1 reply; 17+ messages in thread From: Kalle Valo @ 2012-06-22 7:23 UTC (permalink / raw) To: Rajkumar Manoharan; +Cc: linville, linux-wireless, dan.carpenter 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate 2012-06-22 7:23 ` Kalle Valo @ 2012-06-22 9:22 ` Rajkumar Manoharan 2012-06-22 11:44 ` Kalle Valo 0 siblings, 1 reply; 17+ messages in thread From: Rajkumar Manoharan @ 2012-06-22 9:22 UTC (permalink / raw) To: Kalle Valo; +Cc: linville, linux-wireless, dan.carpenter 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate 2012-06-22 9:22 ` Rajkumar Manoharan @ 2012-06-22 11:44 ` Kalle Valo 2012-06-22 11:57 ` Dan Carpenter 0 siblings, 1 reply; 17+ messages in thread From: Kalle Valo @ 2012-06-22 11:44 UTC (permalink / raw) To: Rajkumar Manoharan; +Cc: linville, linux-wireless, dan.carpenter 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate 2012-06-22 11:44 ` Kalle Valo @ 2012-06-22 11:57 ` Dan Carpenter 2012-06-22 12:42 ` Rajkumar Manoharan 0 siblings, 1 reply; 17+ messages in thread From: Dan Carpenter @ 2012-06-22 11:57 UTC (permalink / raw) To: Kalle Valo; +Cc: Rajkumar Manoharan, linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate 2012-06-22 11:57 ` Dan Carpenter @ 2012-06-22 12:42 ` Rajkumar Manoharan 0 siblings, 0 replies; 17+ messages in thread From: Rajkumar Manoharan @ 2012-06-22 12:42 UTC (permalink / raw) To: Dan Carpenter; +Cc: Kalle Valo, linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning 2012-06-21 18:31 [PATCH 0/3] ath9k: Fix smatch warnings Rajkumar Manoharan 2012-06-21 18:31 ` [PATCH 1/3] ath9k_hw: fix buffer overflow smatch warning Rajkumar Manoharan 2012-06-21 18:31 ` [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate Rajkumar Manoharan @ 2012-06-21 18:31 ` Rajkumar Manoharan 2012-06-21 18:42 ` Ben Greear 2012-06-22 7:17 ` Kalle Valo 2 siblings, 2 replies; 17+ messages in thread From: Rajkumar Manoharan @ 2012-06-21 18:31 UTC (permalink / raw) To: linville; +Cc: linux-wireless, Rajkumar Manoharan, Ben Greear 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning 2012-06-21 18:31 ` [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning Rajkumar Manoharan @ 2012-06-21 18:42 ` Ben Greear 2012-06-21 19:07 ` Rajkumar Manoharan 2012-06-22 7:17 ` Kalle Valo 1 sibling, 1 reply; 17+ messages in thread From: Ben Greear @ 2012-06-21 18:42 UTC (permalink / raw) To: Rajkumar Manoharan; +Cc: linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning 2012-06-21 18:42 ` Ben Greear @ 2012-06-21 19:07 ` Rajkumar Manoharan 2012-06-21 19:15 ` Ben Greear 0 siblings, 1 reply; 17+ messages in thread From: Rajkumar Manoharan @ 2012-06-21 19:07 UTC (permalink / raw) To: Ben Greear; +Cc: linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning 2012-06-21 19:07 ` Rajkumar Manoharan @ 2012-06-21 19:15 ` Ben Greear 2012-06-21 19:20 ` Rajkumar Manoharan 0 siblings, 1 reply; 17+ messages in thread From: Ben Greear @ 2012-06-21 19:15 UTC (permalink / raw) To: Rajkumar Manoharan; +Cc: linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning 2012-06-21 19:15 ` Ben Greear @ 2012-06-21 19:20 ` Rajkumar Manoharan 0 siblings, 0 replies; 17+ messages in thread From: Rajkumar Manoharan @ 2012-06-21 19:20 UTC (permalink / raw) To: Ben Greear; +Cc: linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning 2012-06-21 18:31 ` [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning Rajkumar Manoharan 2012-06-21 18:42 ` Ben Greear @ 2012-06-22 7:17 ` Kalle Valo 2012-06-22 11:55 ` Dan Carpenter 1 sibling, 1 reply; 17+ messages in thread From: Kalle Valo @ 2012-06-22 7:17 UTC (permalink / raw) To: Rajkumar Manoharan; +Cc: linville, linux-wireless, Ben Greear, dan.carpenter 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning 2012-06-22 7:17 ` Kalle Valo @ 2012-06-22 11:55 ` Dan Carpenter 2012-06-22 14:24 ` Ben Greear 0 siblings, 1 reply; 17+ messages in thread From: Dan Carpenter @ 2012-06-22 11:55 UTC (permalink / raw) To: Kalle Valo; +Cc: Rajkumar Manoharan, linville, linux-wireless, Ben Greear 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning 2012-06-22 11:55 ` Dan Carpenter @ 2012-06-22 14:24 ` Ben Greear 2012-06-22 14:32 ` Dan Carpenter 0 siblings, 1 reply; 17+ messages in thread From: Ben Greear @ 2012-06-22 14:24 UTC (permalink / raw) To: Dan Carpenter; +Cc: Kalle Valo, Rajkumar Manoharan, linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning 2012-06-22 14:24 ` Ben Greear @ 2012-06-22 14:32 ` Dan Carpenter 0 siblings, 0 replies; 17+ messages in thread From: Dan Carpenter @ 2012-06-22 14:32 UTC (permalink / raw) To: Ben Greear; +Cc: Kalle Valo, Rajkumar Manoharan, linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-06-22 14:32 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-06-21 18:31 [PATCH 0/3] ath9k: Fix smatch warnings Rajkumar Manoharan 2012-06-21 18:31 ` [PATCH 1/3] ath9k_hw: fix buffer overflow smatch warning Rajkumar Manoharan 2012-06-21 18:31 ` [PATCH 2/3] ath9k_hw: fix smatch warnings in spur_mitigate Rajkumar Manoharan 2012-06-22 7:23 ` Kalle Valo 2012-06-22 9:22 ` Rajkumar Manoharan 2012-06-22 11:44 ` Kalle Valo 2012-06-22 11:57 ` Dan Carpenter 2012-06-22 12:42 ` Rajkumar Manoharan 2012-06-21 18:31 ` [PATCH 3/3] ath9k: fix 'side effect in macro' smatch warning Rajkumar Manoharan 2012-06-21 18:42 ` Ben Greear 2012-06-21 19:07 ` Rajkumar Manoharan 2012-06-21 19:15 ` Ben Greear 2012-06-21 19:20 ` Rajkumar Manoharan 2012-06-22 7:17 ` Kalle Valo 2012-06-22 11:55 ` Dan Carpenter 2012-06-22 14:24 ` Ben Greear 2012-06-22 14:32 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).