[PATCH] ath5k: Fix 64 bits TSF reading.

[PATCH] ath5k: Fix 64 bits TSF reading.

[Benoit Papillault]

According to tests, both TSF lower and upper registers kept counting, so
a rollover of the lower part can happen after the upper 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 reading the upper part once again in such
case. 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.

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

diff --git a/drivers/net/wireless/ath/ath5k/pcu.c b/drivers/net/wireless/ath/ath5k/pcu.c
index aefe84f..4b24c15 100644
--- a/drivers/net/wireless/ath/ath5k/pcu.c
+++ b/drivers/net/wireless/ath/ath5k/pcu.c
@@ -593,10 +593,27 @@ u32 ath5k_hw_get_tsf32(struct ath5k_hw *ah)
  */
 u64 ath5k_hw_get_tsf64(struct ath5k_hw *ah)
 {
-	u64 tsf = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
+	u32 tsf_lower, tsf_upper;
+
+	/*
+	 * While reading TSF upper and then lower part, the clock is still
+	 * counting so the lower part can rollover just after reading the
+	 * upper part. In this case, we expect the lower part to be quite
+	 * small (let's say less than 100us) and we would just need to read
+	 * the upper part again to get the correct value.
+	 *
+	 * Tested on AR2425 (AR5001)
+	 */
+
+	tsf_upper = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
+	tsf_lower = ath5k_hw_reg_read(ah, AR5K_TSF_L32);
+
+	if (tsf_lower < 100)
+		tsf_upper = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
+
 	ATH5K_TRACE(ah->ah_sc);
 
-	return ath5k_hw_reg_read(ah, AR5K_TSF_L32) | (tsf << 32);
+	return (((u64)tsf_upper << 32) | tsf_lower);
 }
 
 /**
-- 
1.5.6.5

Re: [ath5k-devel] [PATCH] ath5k: Fix 64 bits TSF reading.

[Derek Smithies]

Hi,
  No, canot support this.

What happens if there is an interrupt immediately after the first 
two reads - before the test on tsf_lower? - and thisinterrupt lasts longer 
than 100us ? Most often, this is ok - until the interrupt happens at the 
wrong time.

OR, there is a TSF merge event after the tsf_upper has been read, but 
before the tsf_lower has been read? The tsf merge (which is done by the 
hardware), adjusted both the high and low registers.

Derek.
On Sun, 28 Feb 2010, Benoit Papillault wrote:

> +
> +	tsf_upper = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
> +	tsf_lower = ath5k_hw_reg_read(ah, AR5K_TSF_L32);
> +
> +	if (tsf_lower < 100)
> +		tsf_upper = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
> +
> 	ATH5K_TRACE(ah->ah_sc);
>
> -	return ath5k_hw_reg_read(ah, AR5K_TSF_L32) | (tsf << 32);
> +	return (((u64)tsf_upper << 32) | tsf_lower);
> }
>
> /**
>

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

"The only thing IE should be used for is to download Fire Fox"

"My favorite language is call STAR. It's extremely concise. It has
  exactly one verb '*', which does exactly what I want at the moment."
     --Larry Wall

Re: [ath5k-devel] [PATCH] ath5k: Fix 64 bits TSF reading.

[Benoit PAPILLAULT]

Derek Smithies a écrit :
>
> Hi,
>  No, canot support this.
>
> What happens if there is an interrupt immediately after the first two 
> reads - before the test on tsf_lower? - and thisinterrupt lasts longer 
> than 100us ? Most often, this is ok - until the interrupt happens at 
> the wrong time.
>
> OR, there is a TSF merge event after the tsf_upper has been read, but 
> before the tsf_lower has been read? The tsf merge (which is done by 
> the hardware), adjusted both the high and low registers.
>
> Derek.

Hi Derek,

You are indeed right. This was a first step, but like you mentioned, it  
does not handle every cases, so let's forget it.

Regards,
Benoit

> On Sun, 28 Feb 2010, Benoit Papillault wrote:
>
>> +
>> +    tsf_upper = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
>> +    tsf_lower = ath5k_hw_reg_read(ah, AR5K_TSF_L32);
>> +
>> +    if (tsf_lower < 100)
>> +        tsf_upper = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
>> +
>>     ATH5K_TRACE(ah->ah_sc);
>>
>> -    return ath5k_hw_reg_read(ah, AR5K_TSF_L32) | (tsf << 32);
>> +    return (((u64)tsf_upper << 32) | tsf_lower);
>> }
>>
>> /**
>>
>

Re: [PATCH] ath5k: Fix 64 bits TSF reading.

[Johannes Berg]

On Sun, 2010-02-28 at 23:08 +0100, Benoit Papillault wrote:

> -	u64 tsf = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
> +	u32 tsf_lower, tsf_upper;
> +
> +	/*
> +	 * While reading TSF upper and then lower part, the clock is still
> +	 * counting so the lower part can rollover just after reading the
> +	 * upper part. In this case, we expect the lower part to be quite
> +	 * small (let's say less than 100us) and we would just need to read
> +	 * the upper part again to get the correct value.
> +	 *
> +	 * Tested on AR2425 (AR5001)
> +	 */
> +
> +	tsf_upper = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
> +	tsf_lower = ath5k_hw_reg_read(ah, AR5K_TSF_L32);
> +
> +	if (tsf_lower < 100)
> +		tsf_upper = ath5k_hw_reg_read(ah, AR5K_TSF_U32);

You would typically do

do {
   read upper 1
   read lower
   read upper 2
} while (upper 1 != upper 2)

or so but that obviously incurs another read in most cases.

johannes

Re: [PATCH] ath5k: Fix 64 bits TSF reading.

[Benoit PAPILLAULT]

Johannes Berg a écrit :
> On Sun, 2010-02-28 at 23:08 +0100, Benoit Papillault wrote:
>
>   
>> -	u64 tsf = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
>> +	u32 tsf_lower, tsf_upper;
>> +
>> +	/*
>> +	 * While reading TSF upper and then lower part, the clock is still
>> +	 * counting so the lower part can rollover just after reading the
>> +	 * upper part. In this case, we expect the lower part to be quite
>> +	 * small (let's say less than 100us) and we would just need to read
>> +	 * the upper part again to get the correct value.
>> +	 *
>> +	 * Tested on AR2425 (AR5001)
>> +	 */
>> +
>> +	tsf_upper = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
>> +	tsf_lower = ath5k_hw_reg_read(ah, AR5K_TSF_L32);
>> +
>> +	if (tsf_lower < 100)
>> +		tsf_upper = ath5k_hw_reg_read(ah, AR5K_TSF_U32);
>>     
>
> You would typically do
>
> do {
>    read upper 1
>    read lower
>    read upper 2
> } while (upper 1 != upper 2)
>
> or so but that obviously incurs another read in most cases.
>
> johannes
>
>   
Indeed. I'll redo the patch. Derek has convinced me that accuracy is 
sometimes more important than few register reads. So forget this patch.

Regards,
Benoit