[PATCH] ath5k/ath9k: Fix 64 bits TSF reads

[PATCH] ath5k/ath9k: Fix 64 bits TSF reads

[Benoit Papillault]

According to tests, both TSF lower and upper registers kept counting, so
the higher part could have been updated after the lower part has been
read, as shown in the following log where the upper part is read first
and the lower part next.

tsf = {00000003-fffffffd}
tsf = {00000003-00000001}
tsf = {00000004-0000000b}

This patch corrects this by checking that the upper part has not been
changed while the lower part was read. It has been tested in an IBSS
network where artifical IBSS merges have been done in order to trigger
hundreds of rollover for the TSF lower part.

It follows the logic mentionned by Derek, with only 2 register reads
needed at each additional steps instead of 3 (the minimum number of
register reads is still 3).

Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>
---
 drivers/net/wireless/ath/ath5k/pcu.c |   31 +++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath9k/hw.c  |   19 +++++++++++++++----
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/pcu.c b/drivers/net/wireless/ath/ath5k/pcu.c
index 710870e..35ac13e 100644
--- a/drivers/net/wireless/ath/ath5k/pcu.c
+++ b/drivers/net/wireless/ath/ath5k/pcu.c
@@ -496,6 +496,8 @@ void ath5k_hw_set_rx_filter(struct ath5k_hw *ah, u32 filter)
 * Beacon control *
 \****************/
 
+#define ATH5K_MAX_TSF_READ 10
+
 /**
  * ath5k_hw_get_tsf64 - Get the full 64bit TSF
  *
@@ -505,10 +507,35 @@ void ath5k_hw_set_rx_filter(struct ath5k_hw *ah, u32 filter)
  */
 u64 ath5k_hw_get_tsf64(struct ath5k_hw *ah)
 {
-	u64 tsf = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
+	u32 tsf_lower, tsf_upper1, tsf_upper2;
+	int i;
+
+	/*
+	 * While reading TSF upper and then lower part, the clock is still
+	 * counting (or jumping in case of IBSS merge) so we might get
+	 * inconsistent values. To avoid this, we read the upper part again
+	 * and check it has not been changed. We make the hypothesis that a
+	 * maximum of 3 changes can happens in a row (we use 10 as a safe
+	 * value).
+	 * 
+	 * Impact on performance is pretty small, since in most cases, only
+	 * 3 register reads are needed.
+	 */
+
+	tsf_upper1 = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
+	for (i = 0; i < ATH5K_MAX_TSF_READ; i++) {
+		tsf_lower = ath5k_hw_reg_read(ah, AR5K_TSF_L32);
+		tsf_upper2 = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
+		if (tsf_upper2 == tsf_upper1)
+			break;
+		tsf_upper1 = tsf_upper2;
+	}
+
+	WARN_ON( i == ATH5K_MAX_TSF_READ );
+
 	ATH5K_TRACE(ah->ah_sc);
 
-	return ath5k_hw_reg_read(ah, AR5K_TSF_L32) | (tsf << 32);
+	return (((u64)tsf_upper1 << 32) | tsf_lower);
 }
 
 /**
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 0945452..370e5ed 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -3606,14 +3606,25 @@ void ath9k_hw_write_associd(struct ath_hw *ah)
 }
 EXPORT_SYMBOL(ath9k_hw_write_associd);
 
+#define ATH9K_MAX_TSF_READ 10
+
 u64 ath9k_hw_gettsf64(struct ath_hw *ah)
 {
-	u64 tsf;
+	u32 tsf_lower, tsf_upper1, tsf_upper2;
+	int i;
+
+	tsf_upper1 = REG_READ(ah, AR_TSF_U32);
+	for (i = 0; i < ATH9K_MAX_TSF_READ; i++) {
+		tsf_lower = REG_READ(ah, AR_TSF_L32);
+		tsf_upper2 = REG_READ(ah, AR_TSF_U32);
+		if (tsf_upper2 == tsf_upper1)
+			break;
+		tsf_upper1 = tsf_upper2;
+	}
 
-	tsf = REG_READ(ah, AR_TSF_U32);
-	tsf = (tsf << 32) | REG_READ(ah, AR_TSF_L32);
+	WARN_ON( i == ATH9K_MAX_TSF_READ );
 
-	return tsf;
+	return (((u64)tsf_upper1 << 32) | tsf_lower);
 }
 EXPORT_SYMBOL(ath9k_hw_gettsf64);
 
-- 
1.5.6.5

Re: [ath5k-devel] [PATCH] ath5k/ath9k: Fix 64 bits TSF reads

[Pavel Roskin]

On Fri, 2010-04-16 at 00:07 +0200, Benoit Papillault wrote:

> It follows the logic mentionned by Derek, with only 2 register reads
> needed at each additional steps instead of 3 (the minimum number of
> register reads is still 3).

I would prefer an approach whereas tsf_upper2 or tsf_upper1 is chosen
based on whether tsf_lower is more or less than 0x80000000 if
(tsf_upper2 - tsf_upper1) is 1.  If the difference is not 0 or 1, either
the hardware is broken or the kernel was stuck for so long (71 minutes!)
that getting the exact tsf should be the least worry.  That's when
WARN_ON would be appropriate.

The problem with overengineered code is that it doesn't break when it's
better to break and expose the problem :-)

But it's just a suggestion, not a NACK.  It's better to have some fix
than no fix at all.
 
-- 
Regards,
Pavel Roskin

Re: [ath5k-devel] [PATCH] ath5k/ath9k: Fix 64 bits TSF reads

[Benoit PAPILLAULT]

Pavel Roskin a écrit :
> On Fri, 2010-04-16 at 00:07 +0200, Benoit Papillault wrote:
>
>   
>> It follows the logic mentionned by Derek, with only 2 register reads
>> needed at each additional steps instead of 3 (the minimum number of
>> register reads is still 3).
>>     
>
> I would prefer an approach whereas tsf_upper2 or tsf_upper1 is chosen
> based on whether tsf_lower is more or less than 0x80000000 if
> (tsf_upper2 - tsf_upper1) is 1.  If the difference is not 0 or 1, either
> the hardware is broken or the kernel was stuck for so long (71 minutes!)
> that getting the exact tsf should be the least worry.  That's when
> WARN_ON would be appropriate.
>
> The problem with overengineered code is that it doesn't break when it's
> better to break and expose the problem :-)
>
> But it's just a suggestion, not a NACK.  It's better to have some fix
> than no fix at all.
>  
>   
Hello Pavel,

You are considering rollover here, but the TSF can make big jumps as 
well (in case of IBSS merges). This later case can only be handled by 
the above code, to my knowledge. I am seeking consistency first and 
optimization next, not the opposite.

We can even consider the case where only the lower TSF has made a small 
jump before reading the higher part and the lower part. However, such 
case cannot be distinguished from a normal case and the resulting value 
will be consistent anyway (since the higher part has not changed).

In the case where tsf_upper2 = tsf_upper1 + 1, this can happen when a 
rollover occurs. It could also happens in case of IBSS merge, in which 
case, your logic won't work. Let's take an example:

real TSF 0x00000001-ffffffc0 => tsf_upper1 = 1
--- rollover ---
real TSF 0x00000001-ffffffc8 => tsf_lower = 0xffffffc8
real TSF 0x00000002-00000008 => tsf_upper2 = 2

In this case, we assume that TSF = 0x00000001-ffffffc8 (since 0xffffffc8 
 > 0x80000000).

real TSF 0x00000001-10000009 => tsf_upper1 = 1
--- HW IBSS merge ---
real TSF 0x00000002-ffffffc8 => tsf_lower = 0xffffffc8
real TSF 0x00000002-ffffffd0 => tsf_upper2 = 2

In this case, we assume that TSF = 0x00000001-ffffffc8 ... which is wrong!

Did I miss something?

Regards,
Benoit

Re: [ath5k-devel] [PATCH] ath5k/ath9k: Fix 64 bits TSF reads

[Pavel Roskin]

Quoting Benoit PAPILLAULT <benoit.papillault@free.fr>:

> You are considering rollover here, but the TSF can make big jumps as
> well (in case of IBSS merges). This later case can only be handled by
> the above code, to my knowledge. I am seeking consistency first and
> optimization next, not the opposite.

OK, I didn't realize that.  Then I'm fine with your patch.  Sorry for  
the noise.

-- 
Regards,
Pavel Roskin

Re: [ath5k-devel] [PATCH] ath5k/ath9k: Fix 64 bits TSF reads

[Derek Smithies]

Hi,
  The original code was wrong, and there must have been occasions when the 
TSF was read incorrectly. Those occasions were infrequent, and would have 
show up in large networks that were operational on the month timescale.

Benoit's code tries 10 times to read the TSF, looking for when two 
consecutive upper TSF values that are the same. I can think of no physical 
scenario that would cause the 10 consecutive reads to not terminate.

If this fails to happen, then the TSF counter on the radio board is 
busted. If the TSF counter (or the reading of the TSF counter) is busted, 
then you have a bad situation, and something seriously wrong is happening.
We need to know about this - so the kernel warnings are good.

> The problem with overengineered code is that it doesn't break when it's
> better to break and expose the problem :-)
Yes, but the problem with underengineered code is that it doesn't break, 
and the users of the code are blissfully unaware of serious problems.

I would not call this overengineered. I would call this the appropriate 
level of peer review to get stable code that is acceptably reliable.

Benoit's patch is good.
ACK.

Derek.
========================================================================
On Fri, 16 Apr 2010, Pavel Roskin wrote:

> On Fri, 2010-04-16 at 00:07 +0200, Benoit Papillault wrote:
>
>> It follows the logic mentionned by Derek, with only 2 register reads
>> needed at each additional steps instead of 3 (the minimum number of
>> register reads is still 3).
>
> I would prefer an approach whereas tsf_upper2 or tsf_upper1 is chosen
> based on whether tsf_lower is more or less than 0x80000000 if
> (tsf_upper2 - tsf_upper1) is 1.  If the difference is not 0 or 1, either
> the hardware is broken or the kernel was stuck for so long (71 minutes!)
> that getting the exact tsf should be the least worry.  That's when
> WARN_ON would be appropriate.
>
> The problem with overengineered code is that it doesn't break when it's
> better to break and expose the problem :-)
>
> But it's just a suggestion, not a NACK.  It's better to have some fix
> than no fix at all.
>
>

-- 
Derek Smithies Ph.D.
IndraNet Technologies Ltd.
Email: derek@indranet.co.nz
ph +64 3 365 6485
Web: http://www.indranet-technologies.com/