[PATCH 1/3] ath9k_hw: fix division by zero in the ANI monitor code

[PATCH 1/3] ath9k_hw: fix division by zero in the ANI monitor code

[Felix Fietkau]

The commit "ath9k_hw: remove code duplication in phy error counter handling"
split off some duplicate code into a separate function, but did not have a
return code for aborting ANI processing based on counter values.
This introduced a divide by zero issue.
This patch adds the missing return code check in ath9k_hw_ani_monitor

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/wireless/ath/ath9k/ani.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ani.c b/drivers/net/wireless/ath/ath9k/ani.c
index 3aa8fb1..9297f57 100644
--- a/drivers/net/wireless/ath/ath9k/ani.c
+++ b/drivers/net/wireless/ath/ath9k/ani.c
@@ -633,7 +633,7 @@ void ath9k_ani_reset(struct ath_hw *ah, bool is_scanning)
 	REGWRITE_BUFFER_FLUSH(ah);
 }
 
-static void ath9k_hw_ani_read_counters(struct ath_hw *ah)
+static bool ath9k_hw_ani_read_counters(struct ath_hw *ah)
 {
 	struct ath_common *common = ath9k_hw_common(ah);
 	struct ar5416AniState *aniState = &ah->curchan->ani;
@@ -646,10 +646,10 @@ static void ath9k_hw_ani_read_counters(struct ath_hw *ah)
 	ath_hw_cycle_counters_update(common);
 	listenTime = ath_hw_get_listen_time(common);
 
-	if (listenTime < 0) {
+	if (listenTime <= 0) {
 		ah->stats.ast_ani_lneg++;
 		ath9k_ani_restart(ah);
-		return;
+		return false;
 	}
 
 	if (!use_new_ani(ah)) {
@@ -683,7 +683,7 @@ static void ath9k_hw_ani_read_counters(struct ath_hw *ah)
 			REG_WRITE(ah, AR_PHY_ERR_MASK_2,
 				  AR_PHY_ERR_CCK_TIMING);
 		}
-		return;
+		return false;
 	}
 
 	ofdmPhyErrCnt = phyCnt1 - ofdm_base;
@@ -695,7 +695,7 @@ static void ath9k_hw_ani_read_counters(struct ath_hw *ah)
 	ah->stats.ast_ani_cckerrs +=
 		cckPhyErrCnt - aniState->cckPhyErrCount;
 	aniState->cckPhyErrCount = cckPhyErrCnt;
-
+	return true;
 }
 
 void ath9k_hw_ani_monitor(struct ath_hw *ah, struct ath9k_channel *chan)
@@ -711,7 +711,8 @@ void ath9k_hw_ani_monitor(struct ath_hw *ah, struct ath9k_channel *chan)
 	if (WARN_ON(!aniState))
 		return;
 
-	ath9k_hw_ani_read_counters(ah);
+	if (!ath9k_hw_ani_read_counters(ah))
+		return;
 
 	ofdmPhyErrRate = aniState->ofdmPhyErrCount * 1000 /
 			 aniState->listenTime;
-- 
1.7.2.2

[PATCH 2/3] ath9k_hw: fix PHY counter overflow handling in ANI v1

[Felix Fietkau]

PHY counter overflows need to be checked for the old ANI version,
because of its use of interrupt based counter overflow reports when
the counters exceed the configured thresholds.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/wireless/ath/ath9k/ani.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ani.c b/drivers/net/wireless/ath/ath9k/ani.c
index 9297f57..63ccb39 100644
--- a/drivers/net/wireless/ath/ath9k/ani.c
+++ b/drivers/net/wireless/ath/ath9k/ani.c
@@ -664,7 +664,7 @@ static bool ath9k_hw_ani_read_counters(struct ath_hw *ah)
 	phyCnt1 = REG_READ(ah, AR_PHY_ERR_1);
 	phyCnt2 = REG_READ(ah, AR_PHY_ERR_2);
 
-	if (use_new_ani(ah) && (phyCnt1 < ofdm_base || phyCnt2 < cck_base)) {
+	if (!use_new_ani(ah) && (phyCnt1 < ofdm_base || phyCnt2 < cck_base)) {
 		if (phyCnt1 < ofdm_base) {
 			ath_print(common, ATH_DBG_ANI,
 				  "phyCnt1 0x%x, resetting "
-- 
1.7.2.2

[PATCH 3/3] ath9k: add missing locking around ath9k_hw_proc_mib_event

[Felix Fietkau]

ath9k_hw_proc_mib_event updates the cycle counters, so it common->cc_lock
must be acquired.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/wireless/ath/ath9k/main.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 8656491..fa9be51 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -762,7 +762,9 @@ irqreturn_t ath_isr(int irq, void *dev)
 		 * it will clear whatever condition caused
 		 * the interrupt.
 		 */
+		spin_lock(&common->cc_lock);
 		ath9k_hw_proc_mib_event(ah);
+		spin_unlock(&common->cc_lock);
 		ath9k_hw_set_interrupts(ah, ah->imask);
 	}
 
-- 
1.7.2.2

Re: [PATCH 3/3] ath9k: add missing locking around ath9k_hw_proc_mib_event

[Rajkumar Manoharan]

On Tue, Oct 12, 2010 at 07:38:03PM +0530, Felix Fietkau wrote:
> +		spin_lock(&common->cc_lock);
>  		ath9k_hw_proc_mib_event(ah);
> +		spin_unlock(&common->cc_lock);
>  		ath9k_hw_set_interrupts(ah, ah->imask);
>  	}
>
shouldn't we lock ath9k_ani_reset which also restarts ani?

- Rajkumar
> -- 
> 1.7.2.2
> 
> --
> 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

Re: [PATCH 3/3] ath9k: add missing locking around ath9k_hw_proc_mib_event

[Senthil Balasubramanian]

On Wed, Oct 13, 2010 at 10:50:31AM +0530, Rajkumar Manoharan wrote:
> On Tue, Oct 12, 2010 at 07:38:03PM +0530, Felix Fietkau wrote:
> > +		spin_lock(&common->cc_lock);
> >  		ath9k_hw_proc_mib_event(ah);
> > +		spin_unlock(&common->cc_lock);
> >  		ath9k_hw_set_interrupts(ah, ah->imask);
> >  	}
> >
> shouldn't we lock ath9k_ani_reset which also restarts ani?
Remember syncrhonizing ath9k_ani_reset() with spin_lock requires careful
review as ath9k_htc uses mutex lock somewhere in the ani reset path.
> 
> - Rajkumar
> > -- 
> > 1.7.2.2
> > 
> > --
> > 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
> --
> 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

Re: [PATCH 3/3] ath9k: add missing locking around ath9k_hw_proc_mib_event

[Felix Fietkau]

On 2010-10-13 7:20 AM, Rajkumar Manoharan wrote:
> On Tue, Oct 12, 2010 at 07:38:03PM +0530, Felix Fietkau wrote:
>> +		spin_lock(&common->cc_lock);
>>  		ath9k_hw_proc_mib_event(ah);
>> +		spin_unlock(&common->cc_lock);
>>  		ath9k_hw_set_interrupts(ah, ah->imask);
>>  	}
>>
> shouldn't we lock ath9k_ani_reset which also restarts ani?
No, cc_lock is not an ANI lock, it's only for the cycle counters, which
are not touched by ath9k_ani_reset at all.

- Felix

Re: [PATCH 3/3] ath9k: add missing locking around ath9k_hw_proc_mib_event

[Senthil Balasubramanian]

On Wed, Oct 13, 2010 at 03:31:47PM +0530, Felix Fietkau wrote:
> On 2010-10-13 7:20 AM, Rajkumar Manoharan wrote:
> > On Tue, Oct 12, 2010 at 07:38:03PM +0530, Felix Fietkau wrote:
> >> +		spin_lock(&common->cc_lock);
> >>  		ath9k_hw_proc_mib_event(ah);
> >> +		spin_unlock(&common->cc_lock);
> >>  		ath9k_hw_set_interrupts(ah, ah->imask);
> >>  	}
> >>
> > shouldn't we lock ath9k_ani_reset which also restarts ani?
> No, cc_lock is not an ANI lock, it's only for the cycle counters, which
> are not touched by ath9k_ani_reset at all.
yes. cc_lock is meant for cycle counters. the confusion is that whether
ani_reset uses cc_conter. It's obviously not. so there is no need for
for locking ani_reset with the current code.

However I am wondering where are we resetting cc counters except
ath_hw_get_listen_time() which is obviously called after
ath_hw_cycle_counters_update(). There are plenty of cases where ani_reset
is called and shouldn't we reset these counters somewhere ???.
> 
> - 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