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
Rajkumar Manoharan <[email protected]> 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 <[email protected]>
[...]
> - 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
On Fri, Jun 22, 2012 at 02:44:14PM +0300, Kalle Valo wrote:
> Rajkumar Manoharan <[email protected]> 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
Rajkumar Manoharan <[email protected]> writes:
> ath9k_get_et_stats() warn: side effect in macro
> 'AWDATA' doing 'i++'
>
> Cc: Ben Greear <[email protected]>
> Signed-off-by: Rajkumar Manoharan <[email protected]>
[...]
> 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
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<[email protected]>
> Signed-off-by: Rajkumar Manoharan<[email protected]>
> ---
> 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 <[email protected]>
Candela Technologies Inc http://www.candelatech.com
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 <[email protected]>
---
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
On Fri, Jun 22, 2012 at 10:17:23AM +0300, Kalle Valo wrote:
> Rajkumar Manoharan <[email protected]> writes:
>
> > ath9k_get_et_stats() warn: side effect in macro
> > 'AWDATA' doing 'i++'
> >
> > Cc: Ben Greear <[email protected]>
> > Signed-off-by: Rajkumar Manoharan <[email protected]>
>
> [...]
>
> > 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
On Fri, Jun 22, 2012 at 10:23:44AM +0300, Kalle Valo wrote:
> Rajkumar Manoharan <[email protected]> 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 <[email protected]>
>
> [...]
>
> > - 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
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<[email protected]>
> >>>Signed-off-by: Rajkumar Manoharan<[email protected]>
> >>>---
> >>> 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
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<[email protected]>
> >Signed-off-by: Rajkumar Manoharan<[email protected]>
> >---
> > 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
ath9k_get_et_stats() warn: side effect in macro
'AWDATA' doing 'i++'
Cc: Ben Greear <[email protected]>
Signed-off-by: Rajkumar Manoharan <[email protected]>
---
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
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 <[email protected]> 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
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<[email protected]>
>>> Signed-off-by: Rajkumar Manoharan<[email protected]>
>>> ---
>>> 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 <[email protected]>
Candela Technologies Inc http://www.candelatech.com
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<[email protected]> writes:
> >>
> >>>ath9k_get_et_stats() warn: side effect in macro
> >>>'AWDATA' doing 'i++'
> >>>
> >>>Cc: Ben Greear<[email protected]>
> >>>Signed-off-by: Rajkumar Manoharan<[email protected]>
> >>
> >>[...]
> >>
> >>> 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
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<[email protected]> writes:
>>
>>> ath9k_get_et_stats() warn: side effect in macro
>>> 'AWDATA' doing 'i++'
>>>
>>> Cc: Ben Greear<[email protected]>
>>> Signed-off-by: Rajkumar Manoharan<[email protected]>
>>
>> [...]
>>
>>> 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 <[email protected]>
Candela Technologies Inc http://www.candelatech.com
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 <[email protected]>
---
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
Rajkumar Manoharan <[email protected]> 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