This was introduced in 54da20d83f "ath9k_hw: improve ANI processing and
rx desensitizing parameters". It triggers a Smatch complaint because
the "if (!x != y) { ..." formation is a common precedence error.
In this case, maybe the code was intended to be as it is. The
"->ofdmWeakSigDetectOff" and "->ofdm_weak_signal_on" variables are both
boolean.
Or perhaps based we could change the != to an == and remove the negate?
But normally when I see this formation it is a precedence bug so that's
the patch I've sent.
Signed-off-by: Dan Carpenter <[email protected]>
diff --git a/drivers/net/wireless/ath/ath9k/ani.c b/drivers/net/wireless/ath/ath9k/ani.c
index 47a9fb4..8304e27 100644
--- a/drivers/net/wireless/ath/ath9k/ani.c
+++ b/drivers/net/wireless/ath/ath9k/ani.c
@@ -291,7 +291,7 @@ static void ath9k_hw_set_ofdm_nil(struct ath_hw *ah, u8 immunityLevel)
entry_ofdm->fir_step_level);
if ((aniState->noiseFloor >= aniState->rssiThrHigh) &&
- (!aniState->ofdmWeakSigDetectOff !=
+ (aniState->ofdmWeakSigDetectOff !=
entry_ofdm->ofdm_weak_signal_on)) {
ath9k_hw_ani_control(ah,
ATH9K_ANI_OFDM_WEAK_SIGNAL_DETECTION,
On Tue, Apr 10, 2012 at 01:57:25PM +0300, Dan Carpenter wrote:
> On Tue, Apr 10, 2012 at 11:58:26AM +0200, Felix Fietkau wrote:
> > > if ((aniState->noiseFloor >= aniState->rssiThrHigh) &&
> > > - (!aniState->ofdmWeakSigDetectOff !=
> > > + (aniState->ofdmWeakSigDetectOff !=
> > Looking at other Atheros code, I think this patch is wrong, it should
> > be: aniState->ofdmWeakSigDetectOff == entry_ofdm->ofdm_weak_signal_on
> >
> > While a bit confusing, the behavior of the original code was correct,
> > aniState->ofdmWeakSigDetectOff is used as a boolean.
> >
> > This code badly needs a cleanup, the whole mess with *on vs *off
> > variables is quite confusing.
> >
>
> Yep. It's bad to name variables xxx_off because it leads triple
> negatives like we see here.
>
> I guess that "if (!off != on) { ..." might be more readable than
> "if (off == on) { ...". There are a couple ways to silence this
> warning in Smatch.
>
> The first way would be to add parenthesis like,
> "((!off) != on) {...". I think it helps because it makes the negate
> stand out and you don't have to think about how the precedence
> rules works. Other people think it's a needless parenthesis for
> something that is obvious.
>
> The other way would be to make either ->ofdmWeakSigDetectOff or
> ->ofdm_weak_signal_on be type bool. I think this would make the
> code clearer.
>
Declare them as bool type and rename one of them to common (either *_off or *_on).
-Rajkumar
On 2012-04-10 11:04 AM, Dan Carpenter wrote:
> This was introduced in 54da20d83f "ath9k_hw: improve ANI processing and
> rx desensitizing parameters". It triggers a Smatch complaint because
> the "if (!x != y) { ..." formation is a common precedence error.
>
> In this case, maybe the code was intended to be as it is. The
> "->ofdmWeakSigDetectOff" and "->ofdm_weak_signal_on" variables are both
> boolean.
>
> Or perhaps based we could change the != to an == and remove the negate?
>
> But normally when I see this formation it is a precedence bug so that's
> the patch I've sent.
>
> Signed-off-by: Dan Carpenter <[email protected]>
>
> diff --git a/drivers/net/wireless/ath/ath9k/ani.c b/drivers/net/wireless/ath/ath9k/ani.c
> index 47a9fb4..8304e27 100644
> --- a/drivers/net/wireless/ath/ath9k/ani.c
> +++ b/drivers/net/wireless/ath/ath9k/ani.c
> @@ -291,7 +291,7 @@ static void ath9k_hw_set_ofdm_nil(struct ath_hw *ah, u8 immunityLevel)
> entry_ofdm->fir_step_level);
>
> if ((aniState->noiseFloor >= aniState->rssiThrHigh) &&
> - (!aniState->ofdmWeakSigDetectOff !=
> + (aniState->ofdmWeakSigDetectOff !=
Looking at other Atheros code, I think this patch is wrong, it should
be: aniState->ofdmWeakSigDetectOff == entry_ofdm->ofdm_weak_signal_on
While a bit confusing, the behavior of the original code was correct,
aniState->ofdmWeakSigDetectOff is used as a boolean.
This code badly needs a cleanup, the whole mess with *on vs *off
variables is quite confusing.
- Felix
On Tue, Apr 10, 2012 at 11:58:26AM +0200, Felix Fietkau wrote:
> > if ((aniState->noiseFloor >= aniState->rssiThrHigh) &&
> > - (!aniState->ofdmWeakSigDetectOff !=
> > + (aniState->ofdmWeakSigDetectOff !=
> Looking at other Atheros code, I think this patch is wrong, it should
> be: aniState->ofdmWeakSigDetectOff == entry_ofdm->ofdm_weak_signal_on
>
> While a bit confusing, the behavior of the original code was correct,
> aniState->ofdmWeakSigDetectOff is used as a boolean.
>
> This code badly needs a cleanup, the whole mess with *on vs *off
> variables is quite confusing.
>
Yep. It's bad to name variables xxx_off because it leads triple
negatives like we see here.
I guess that "if (!off != on) { ..." might be more readable than
"if (off == on) { ...". There are a couple ways to silence this
warning in Smatch.
The first way would be to add parenthesis like,
"((!off) != on) {...". I think it helps because it makes the negate
stand out and you don't have to think about how the precedence
rules works. Other people think it's a needless parenthesis for
something that is obvious.
The other way would be to make either ->ofdmWeakSigDetectOff or
->ofdm_weak_signal_on be type bool. I think this would make the
code clearer.
regards,
dan carpenter