| Message ID | 1452821009-1156-2-git-send-email-miaoqing@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
<miaoqing@codeaurora.org> writes: > From: Miaoqing Pan <miaoqing@codeaurora.org> > > ath9k_ani_restart() is always be invoked even if the trigger > condition is false. > > Signed-off-by: Miaoqing Pan <miaoqing@codeaurora.org> Why? The commit log should always answer that.
The commit 54da20d83f0e7fe87b75aec44bc2b1448d119320 ("ath9k_hw: improve ANI processing and rx desensitizing parameters") first introduced this issue, ath9k_ani_restart() is always be invoked is wrong.
Thanks,
Miaoqing
"Pan, Miaoqing" <miaoqing@qti.qualcomm.com> writes: > The commit 54da20d83f0e7fe87b75aec44bc2b1448d119320 ("ath9k_hw: > improve ANI processing and rx desensitizing parameters") first > introduced this issue, ath9k_ani_restart() is always be invoked is > wrong. Then you should add this to the commit log: Fixes: 54da20d83f0e ("ath9k_hw: improve ANI processing and rx desensitizing parameters") Also you need to explain more, especially why you think it's wrong. How did you notice this? What kind of user visible issue this fixes?
On 2016-01-15 02:23, miaoqing@codeaurora.org wrote: > From: Miaoqing Pan <miaoqing@codeaurora.org> > > ath9k_ani_restart() is always be invoked even if the trigger > condition is false. > > Signed-off-by: Miaoqing Pan <miaoqing@codeaurora.org> > --- > drivers/net/wireless/ath/ath9k/ani.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath9k/ani.c b/drivers/net/wireless/ath/ath9k/ani.c > index 25e45e4..0f74d59 100644 > --- a/drivers/net/wireless/ath/ath9k/ani.c > +++ b/drivers/net/wireless/ath/ath9k/ani.c > @@ -444,14 +444,16 @@ void ath9k_hw_ani_monitor(struct ath_hw *ah, struct ath9k_channel *chan) > ofdmPhyErrRate < ah->config.ofdm_trig_low) { > ath9k_hw_ani_lower_immunity(ah); > aniState->ofdmsTurn = !aniState->ofdmsTurn; > + ath9k_ani_restart(ah); > } else if (ofdmPhyErrRate > ah->config.ofdm_trig_high) { > ath9k_hw_ani_ofdm_err_trigger(ah); > aniState->ofdmsTurn = false; > + ath9k_ani_restart(ah); > } else if (cckPhyErrRate > ah->config.cck_trig_high) { > ath9k_hw_ani_cck_err_trigger(ah); > aniState->ofdmsTurn = true; > + ath9k_ani_restart(ah); > } How about adding an 'else return' here instead of duplicating the ath9k_ani_restart lines? - Felix -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/ath9k/ani.c b/drivers/net/wireless/ath/ath9k/ani.c index 25e45e4..0f74d59 100644 --- a/drivers/net/wireless/ath/ath9k/ani.c +++ b/drivers/net/wireless/ath/ath9k/ani.c @@ -444,14 +444,16 @@ void ath9k_hw_ani_monitor(struct ath_hw *ah, struct ath9k_channel *chan) ofdmPhyErrRate < ah->config.ofdm_trig_low) { ath9k_hw_ani_lower_immunity(ah); aniState->ofdmsTurn = !aniState->ofdmsTurn; + ath9k_ani_restart(ah); } else if (ofdmPhyErrRate > ah->config.ofdm_trig_high) { ath9k_hw_ani_ofdm_err_trigger(ah); aniState->ofdmsTurn = false; + ath9k_ani_restart(ah); } else if (cckPhyErrRate > ah->config.cck_trig_high) { ath9k_hw_ani_cck_err_trigger(ah); aniState->ofdmsTurn = true; + ath9k_ani_restart(ah); } - ath9k_ani_restart(ah); } } EXPORT_SYMBOL(ath9k_hw_ani_monitor);