[ath5k] Incorrect value for ACK_TIMEOUT

[ath5k] Incorrect value for ACK_TIMEOUT

[Jonathan Guerin]

According to the 802.11-2007 spec document, the ACKTimeout value is
(Section 9.2.8 ACK procedure):
ACKTimeout = aSIFSTime + aSlotTime + aPHY-RX-START-Delay

>From Table 17-15—OFDM PHY characteristics, the values are:
aSIFSTime = 16
aSlotTime = 9
aPHY-RX-START-Delay = 25

Therefore, ACKTimeout = 50

In the driver source, this appears to be initialised to:

$$ ath5k.h
#define AR5K_INIT_ACK_CTS_TIMEOUT		1024

$$ reg.h
/*
 * ACK/CTS timeout register
 */
#define AR5K_TIME_OUT		0x8014			/* Register Address */
#define AR5K_TIME_OUT_ACK	0x00001fff	/* ACK timeout mask */
#define AR5K_TIME_OUT_ACK_S	0
#define AR5K_TIME_OUT_CTS	0x1fff0000	/* CTS timeout mask */
#define AR5K_TIME_OUT_CTS_S	16

Taking the value of the register (1024), masking it with the ACK
Timeout Mask (0x1fff) and dividing it by the clock rate (40), we get a
much smaller ACK Timeout value:
1024 & 0x1fff = 1024
1024/40 = 25us


Am I understanding this right? Is the driver actually setting the ACK
Timeout to be much smaller than even a DIFS, let alone matching the
spec?



Thanks,

--
Jonathan Guerin

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Bruno Randolf]

On Wed September 15 2010 10:56:32 Jonathan Guerin wrote:
> According to the 802.11-2007 spec document, the ACKTimeout value is
> (Section 9.2.8 ACK procedure):
> ACKTimeout = aSIFSTime + aSlotTime + aPHY-RX-START-Delay
> 
> >From Table 17-15—OFDM PHY characteristics, the values are:
> aSIFSTime = 16
> aSlotTime = 9
> aPHY-RX-START-Delay = 25
> 
> Therefore, ACKTimeout = 50
> 
> In the driver source, this appears to be initialised to:
> 
> $$ ath5k.h
> #define AR5K_INIT_ACK_CTS_TIMEOUT		1024
> 
> $$ reg.h
> /*
>  * ACK/CTS timeout register
>  */
> #define AR5K_TIME_OUT		0x8014			/* Register Address */
> #define AR5K_TIME_OUT_ACK	0x00001fff	/* ACK timeout mask */
> #define AR5K_TIME_OUT_ACK_S	0
> #define AR5K_TIME_OUT_CTS	0x1fff0000	/* CTS timeout mask */
> #define AR5K_TIME_OUT_CTS_S	16
> 
> Taking the value of the register (1024), masking it with the ACK
> Timeout Mask (0x1fff) and dividing it by the clock rate (40), we get a
> much smaller ACK Timeout value:
> 1024 & 0x1fff = 1024
> 1024/40 = 25us
> 
> 
> Am I understanding this right? Is the driver actually setting the ACK
> Timeout to be much smaller than even a DIFS, let alone matching the
> spec?
> 

thanks for looking into this! i think you are right in that there is a mistake 
here, but AR5K_INIT_ACK_CTS_TIMEOUT is just used for AR5210 chips for setting 
the AR5K_SLOT_TIME. so this is wrong, but it probably does not affect you (or 
many people). 5211 and 5212 only use initvals:

       /*	  a	    aTurbo	  b	  g (OFDM)    */
5211:
	{ AR5K_TIME_OUT,
	   { 0x04000400, 0x08000800, 0x20003000, 0x04000400 } },

5212:
	{ AR5K_TIME_OUT,
	   { 0x03e803e8, 0x06e006e0, 0x04200420, 0x08400840, 0x06e006e0 } },

so on 5211 this is 1024 in A and G mode (i dont care about B and turbo), so 
same problem you described.

on 5212 this is 1000 in A mode and 1760 in G mode, which would result in a 
timeout of 25 and 40.

this is all confusing and i think we should replace the initvals by calling 
the function ath5k_hw_set_ack_timeout().

bruno

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Jonathan Guerin]

On Wed, Sep 15, 2010 at 12:30 PM, Bruno Randolf <br1@einfach.org> wrote:
> On Wed September 15 2010 10:56:32 Jonathan Guerin wrote:
>> According to the 802.11-2007 spec document, the ACKTimeout value is
>> (Section 9.2.8 ACK procedure):
>> ACKTimeout = aSIFSTime + aSlotTime + aPHY-RX-START-Delay
>>
>> >From Table 17-15—OFDM PHY characteristics, the values are:
>> aSIFSTime = 16
>> aSlotTime = 9
>> aPHY-RX-START-Delay = 25
>>
>> Therefore, ACKTimeout = 50
>>
>> In the driver source, this appears to be initialised to:
>>
>> $$ ath5k.h
>> #define AR5K_INIT_ACK_CTS_TIMEOUT             1024
>>
>> $$ reg.h
>> /*
>>  * ACK/CTS timeout register
>>  */
>> #define AR5K_TIME_OUT         0x8014                  /* Register Address */
>> #define AR5K_TIME_OUT_ACK     0x00001fff      /* ACK timeout mask */
>> #define AR5K_TIME_OUT_ACK_S   0
>> #define AR5K_TIME_OUT_CTS     0x1fff0000      /* CTS timeout mask */
>> #define AR5K_TIME_OUT_CTS_S   16
>>
>> Taking the value of the register (1024), masking it with the ACK
>> Timeout Mask (0x1fff) and dividing it by the clock rate (40), we get a
>> much smaller ACK Timeout value:
>> 1024 & 0x1fff = 1024
>> 1024/40 = 25us
>>
>>
>> Am I understanding this right? Is the driver actually setting the ACK
>> Timeout to be much smaller than even a DIFS, let alone matching the
>> spec?
>>
>
> thanks for looking into this! i think you are right in that there is a mistake
> here, but AR5K_INIT_ACK_CTS_TIMEOUT is just used for AR5210 chips for setting
> the AR5K_SLOT_TIME. so this is wrong, but it probably does not affect you (or

I did realise after posting my message that the value was being
written into the SLOT_TIME register, rather than the TIME_OUT one. My
bad, I should've done a more thorough search...

> many people). 5211 and 5212 only use initvals:
>
>       /*         a         aTurbo        b       g (OFDM)    */
> 5211:
>        { AR5K_TIME_OUT,
>           { 0x04000400, 0x08000800, 0x20003000, 0x04000400 } },
>
> 5212:
>        { AR5K_TIME_OUT,
>           { 0x03e803e8, 0x06e006e0, 0x04200420, 0x08400840, 0x06e006e0 } },
>
> so on 5211 this is 1024 in A and G mode (i dont care about B and turbo), so
> same problem you described.
>
> on 5212 this is 1000 in A mode and 1760 in G mode, which would result in a
> timeout of 25 and 40.

We have a 5213 chip - does this default to the 5212 branch inside ath5k?

>
> this is all confusing and i think we should replace the initvals by calling
> the function ath5k_hw_set_ack_timeout().

Should all of the init be changed to use these functions, along with
#define init values?

>
> bruno
>

Thanks,

Jonathan

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Jonathan Guerin]

On Wed, Sep 15, 2010 at 1:01 PM, Jonathan Guerin <jonathan@guerin.id.au> wrote:

>>
>> on 5212 this is 1000 in A mode and 1760 in G mode, which would result in a
>> timeout of 25 and 40.
>
> We have a 5213 chip - does this default to the 5212 branch inside ath5k?
>

Never mind, I found it...
$$ attach.c
else if (srev == AR5K_SREV_AR5213A &&
		ah->ah_phy_revision == AR5K_SREV_PHY_5212B) {
			ah->ah_radio = AR5K_RF5112;
			ah->ah_single_chip = false;
			ah->ah_radio_5ghz_revision = AR5K_SREV_RAD_5112B;

> Thanks,
>
> Jonathan
>

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Bruno Randolf]

On Wed September 15 2010 12:01:50 Jonathan Guerin wrote:
> On Wed, Sep 15, 2010 at 12:30 PM, Bruno Randolf <br1@einfach.org> wrote:
> > On Wed September 15 2010 10:56:32 Jonathan Guerin wrote:
> >> According to the 802.11-2007 spec document, the ACKTimeout value is
> >> (Section 9.2.8 ACK procedure):
> >> ACKTimeout = aSIFSTime + aSlotTime + aPHY-RX-START-Delay
> >> 
> >> >From Table 17-15—OFDM PHY characteristics, the values are:
> >> aSIFSTime = 16
> >> aSlotTime = 9
> >> aPHY-RX-START-Delay = 25
> >> 
> >> Therefore, ACKTimeout = 50
> >> 
> >> In the driver source, this appears to be initialised to:
> >> 
> >> $$ ath5k.h
> >> #define AR5K_INIT_ACK_CTS_TIMEOUT             1024
> >> 
> >> $$ reg.h
> >> /*
> >>  * ACK/CTS timeout register
> >>  */
> >> #define AR5K_TIME_OUT         0x8014                  /* Register
> >> Address */ #define AR5K_TIME_OUT_ACK     0x00001fff      /* ACK timeout
> >> mask */ #define AR5K_TIME_OUT_ACK_S   0
> >> #define AR5K_TIME_OUT_CTS     0x1fff0000      /* CTS timeout mask */
> >> #define AR5K_TIME_OUT_CTS_S   16
> >> 
> >> Taking the value of the register (1024), masking it with the ACK
> >> Timeout Mask (0x1fff) and dividing it by the clock rate (40), we get a
> >> much smaller ACK Timeout value:
> >> 1024 & 0x1fff = 1024
> >> 1024/40 = 25us
> >> 
> >> 
> >> Am I understanding this right? Is the driver actually setting the ACK
> >> Timeout to be much smaller than even a DIFS, let alone matching the
> >> spec?
> > 
> > thanks for looking into this! i think you are right in that there is a
> > mistake here, but AR5K_INIT_ACK_CTS_TIMEOUT is just used for AR5210
> > chips for setting the AR5K_SLOT_TIME. so this is wrong, but it probably
> > does not affect you (or
> 
> I did realise after posting my message that the value was being
> written into the SLOT_TIME register, rather than the TIME_OUT one. My
> bad, I should've done a more thorough search...

probably writing this to SLOT_TIME is wrong too? anyhow 5210 is kind of broken 
and not so important, imho. but it would be good to fix it too.

> We have a 5213 chip - does this default to the 5212 branch inside ath5k?

yes. anything higher than 5212 is treated as 5212, generally.

> > this is all confusing and i think we should replace the initvals by
> > calling the function ath5k_hw_set_ack_timeout().
> 
> Should all of the init be changed to use these functions, along with
> #define init values?

i think it would be best if we could remove the initvals alltoghether and use 
the function to set the ack timeout. but i'm not sure if we need to have them 
initialized to some value at the beginning. in this case can we just override 
them later using the function. or just update the initvals numerically (but no 
defines).

it's great that you review this! i think no-one has looked at this since the 
original import, so there may be really old bugs here.

thanks,
bruno

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Jonathan Guerin]

On Wed, Sep 15, 2010 at 1:16 PM, Bruno Randolf <br1@einfach.org> wrote:
> On Wed September 15 2010 12:01:50 Jonathan Guerin wrote:
>> On Wed, Sep 15, 2010 at 12:30 PM, Bruno Randolf <br1@einfach.org> wrote:
>> > On Wed September 15 2010 10:56:32 Jonathan Guerin wrote:
>> >> According to the 802.11-2007 spec document, the ACKTimeout value is
>> >> (Section 9.2.8 ACK procedure):
>> >> ACKTimeout = aSIFSTime + aSlotTime + aPHY-RX-START-Delay
>> >>
>> >> >From Table 17-15—OFDM PHY characteristics, the values are:
>> >> aSIFSTime = 16
>> >> aSlotTime = 9
>> >> aPHY-RX-START-Delay = 25
>> >>
>> >> Therefore, ACKTimeout = 50
>> >>
>> >> In the driver source, this appears to be initialised to:
>> >>
>> >> $$ ath5k.h
>> >> #define AR5K_INIT_ACK_CTS_TIMEOUT             1024
>> >>
>> >> $$ reg.h
>> >> /*
>> >>  * ACK/CTS timeout register
>> >>  */
>> >> #define AR5K_TIME_OUT         0x8014                  /* Register
>> >> Address */ #define AR5K_TIME_OUT_ACK     0x00001fff      /* ACK timeout
>> >> mask */ #define AR5K_TIME_OUT_ACK_S   0
>> >> #define AR5K_TIME_OUT_CTS     0x1fff0000      /* CTS timeout mask */
>> >> #define AR5K_TIME_OUT_CTS_S   16
>> >>
>> >> Taking the value of the register (1024), masking it with the ACK
>> >> Timeout Mask (0x1fff) and dividing it by the clock rate (40), we get a
>> >> much smaller ACK Timeout value:
>> >> 1024 & 0x1fff = 1024
>> >> 1024/40 = 25us
>> >>
>> >>
>> >> Am I understanding this right? Is the driver actually setting the ACK
>> >> Timeout to be much smaller than even a DIFS, let alone matching the
>> >> spec?
>> >
>> > thanks for looking into this! i think you are right in that there is a
>> > mistake here, but AR5K_INIT_ACK_CTS_TIMEOUT is just used for AR5210
>> > chips for setting the AR5K_SLOT_TIME. so this is wrong, but it probably
>> > does not affect you (or
>>
>> I did realise after posting my message that the value was being
>> written into the SLOT_TIME register, rather than the TIME_OUT one. My
>> bad, I should've done a more thorough search...
>
> probably writing this to SLOT_TIME is wrong too? anyhow 5210 is kind of broken
> and not so important, imho. but it would be good to fix it too.
>
>> We have a 5213 chip - does this default to the 5212 branch inside ath5k?
>
> yes. anything higher than 5212 is treated as 5212, generally.
>
>> > this is all confusing and i think we should replace the initvals by
>> > calling the function ath5k_hw_set_ack_timeout().
>>
>> Should all of the init be changed to use these functions, along with
>> #define init values?
>
> i think it would be best if we could remove the initvals alltoghether and use
> the function to set the ack timeout. but i'm not sure if we need to have them
> initialized to some value at the beginning. in this case can we just override
> them later using the function. or just update the initvals numerically (but no
> defines).

If there are no initvals, changing the mode won't init them to default
values, will it? Or are you saying that values should not be
initialised until the mode is set?

>
> it's great that you review this! i think no-one has looked at this since the
> original import, so there may be really old bugs here.

I've been a bit scared to post here until I had more information
rather than a generic "uhh guys, something's not working". It's been
causing me massive headaches with my work as I'm relying on the spec
for timing values, but finding that the driver is doing something
much, much different. I'm more than happy to post these things, but
I'm not a good enough programmer to just give you patches without
discussing the fact that I may just not be understanding what's really
happening.

Thanks for the help, it's greatly appreciated!

>
> thanks,
> bruno
>

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Bruno Randolf]

On Wed September 15 2010 12:28:36 Jonathan Guerin wrote:
> >> Should all of the init be changed to use these functions, along with
> >> #define init values?
> > 
> > i think it would be best if we could remove the initvals alltoghether and
> > use the function to set the ack timeout. but i'm not sure if we need to
> > have them initialized to some value at the beginning. in this case can
> > we just override them later using the function. or just update the
> > initvals numerically (but no defines).
> 
> If there are no initvals, changing the mode won't init them to default
> values, will it? Or are you saying that values should not be
> initialised until the mode is set?

that's what i'm not sure about... but ath5k_hw_set_ack_timeout() could be 
called from reset so it applies the changes when the mode is set. i think that 
should be fine, but it needs to be tested -- and i think the same applies to a 
lot of the initvals.

> I've been a bit scared to post here until I had more information
> rather than a generic "uhh guys, something's not working". It's been
> causing me massive headaches with my work as I'm relying on the spec
> for timing values, but finding that the driver is doing something
> much, much different. I'm more than happy to post these things, but
> I'm not a good enough programmer to just give you patches without
> discussing the fact that I may just not be understanding what's really
> happening.
> Thanks for the help, it's greatly appreciated!

same here :)

the thing is, like bob said, that we don't really know much about what the HW 
expects in some of these registers, so unless you can prove your changes to be 
correct - e.g. by measurements which fit the theoretical model better, or by 
better thruput, it's hard to say if they are correct or not. 

so before you worry about how to make a correct patch, just go ahead and 
change those initvals to what you believe they should be. if it improves 
something, let's talk about how to correctly fix it.

bruno

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Jonathan Guerin]

On Wed, Sep 15, 2010 at 1:45 PM, Bruno Randolf <br1@einfach.org> wrote:
> On Wed September 15 2010 12:28:36 Jonathan Guerin wrote:
>> >> Should all of the init be changed to use these functions, along with
>> >> #define init values?
>> >
>> > i think it would be best if we could remove the initvals alltoghether and
>> > use the function to set the ack timeout. but i'm not sure if we need to
>> > have them initialized to some value at the beginning. in this case can
>> > we just override them later using the function. or just update the
>> > initvals numerically (but no defines).
>>
>> If there are no initvals, changing the mode won't init them to default
>> values, will it? Or are you saying that values should not be
>> initialised until the mode is set?
>
> that's what i'm not sure about... but ath5k_hw_set_ack_timeout() could be
> called from reset so it applies the changes when the mode is set. i think that
> should be fine, but it needs to be tested -- and i think the same applies to a
> lot of the initvals.
>
>> I've been a bit scared to post here until I had more information
>> rather than a generic "uhh guys, something's not working". It's been
>> causing me massive headaches with my work as I'm relying on the spec
>> for timing values, but finding that the driver is doing something
>> much, much different. I'm more than happy to post these things, but
>> I'm not a good enough programmer to just give you patches without
>> discussing the fact that I may just not be understanding what's really
>> happening.
>> Thanks for the help, it's greatly appreciated!
>
> same here :)
>
> the thing is, like bob said, that we don't really know much about what the HW
> expects in some of these registers, so unless you can prove your changes to be
> correct - e.g. by measurements which fit the theoretical model better, or by
> better thruput, it's hard to say if they are correct or not.
>
> so before you worry about how to make a correct patch, just go ahead and
> change those initvals to what you believe they should be. if it improves
> something, let's talk about how to correctly fix it.

I don't mean to be a smart-ass, but wouldn't setting the correct
values here decrease the throughput? I'm not quite sure how to verify
an improvement, other than get the card to contend for the medium with
another known-working card? Does this sound reasonable?

>
> bruno
>

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Kalle Valo]

Jonathan Guerin <jonathan@guerin.id.au> writes:

>> the thing is, like bob said, that we don't really know much about what the HW
>> expects in some of these registers, so unless you can prove your changes to be
>> correct - e.g. by measurements which fit the theoretical model better, or by
>> better thruput, it's hard to say if they are correct or not.
>>
>> so before you worry about how to make a correct patch, just go ahead and
>> change those initvals to what you believe they should be. if it improves
>> something, let's talk about how to correctly fix it.
>
> I don't mean to be a smart-ass, but wouldn't setting the correct
> values here decrease the throughput? I'm not quite sure how to verify
> an improvement, other than get the card to contend for the medium with
> another known-working card? Does this sound reasonable?

Yes, it does sound reasonable. You might get throughput increase by
violating the spec.

-- 
Kalle Valo

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Jonathan Guerin]

So, we would prefer to match the spec for an out-of-the-box build,
even tho it would appear to be a worse-performing driving to users?

--
Jonathan Guerin



On Wed, Sep 15, 2010 at 5:17 PM, Kalle Valo <kvalo@adurom.com> wrote:
> Jonathan Guerin <jonathan@guerin.id.au> writes:
>
>>> the thing is, like bob said, that we don't really know much about what the HW
>>> expects in some of these registers, so unless you can prove your changes to be
>>> correct - e.g. by measurements which fit the theoretical model better, or by
>>> better thruput, it's hard to say if they are correct or not.
>>>
>>> so before you worry about how to make a correct patch, just go ahead and
>>> change those initvals to what you believe they should be. if it improves
>>> something, let's talk about how to correctly fix it.
>>
>> I don't mean to be a smart-ass, but wouldn't setting the correct
>> values here decrease the throughput? I'm not quite sure how to verify
>> an improvement, other than get the card to contend for the medium with
>> another known-working card? Does this sound reasonable?
>
> Yes, it does sound reasonable. You might get throughput increase by
> violating the spec.
>
> --
> Kalle Valo
>

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Bruno Randolf]

On Wed September 15 2010 16:27:01 Jonathan Guerin wrote:
> So, we would prefer to match the spec for an out-of-the-box build,
> even tho it would appear to be a worse-performing driving to users?

sure, we need to comply to the specs.

bruno

> --
> Jonathan Guerin
> 
> On Wed, Sep 15, 2010 at 5:17 PM, Kalle Valo <kvalo@adurom.com> wrote:
> > Jonathan Guerin <jonathan@guerin.id.au> writes:
> >>> the thing is, like bob said, that we don't really know much about what
> >>> the HW expects in some of these registers, so unless you can prove
> >>> your changes to be correct - e.g. by measurements which fit the
> >>> theoretical model better, or by better thruput, it's hard to say if
> >>> they are correct or not.
> >>> 
> >>> so before you worry about how to make a correct patch, just go ahead
> >>> and change those initvals to what you believe they should be. if it
> >>> improves something, let's talk about how to correctly fix it.
> >> 
> >> I don't mean to be a smart-ass, but wouldn't setting the correct
> >> values here decrease the throughput? I'm not quite sure how to verify
> >> an improvement, other than get the card to contend for the medium with
> >> another known-working card? Does this sound reasonable?
> > 
> > Yes, it does sound reasonable. You might get throughput increase by
> > violating the spec.
> > 
> > --
> > Kalle Valo

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Kalle Valo]

Jonathan Guerin <jonathan@guerin.id.au> writes:

> So, we would prefer to match the spec for an out-of-the-box build,
> even tho it would appear to be a worse-performing driving to users?

If "an out-of-the-box build" means correctly working 802.11 device
then my opinion is yes, we should strive to match the spec even if the
performance decreases. Getting performance increases by violating the
spec is just stealing airtime from others.

But the challange is how to know if another 802.11 device is properly
working and following the spec? I know that Wi-Fi Alliance
certification tests contain lots of throughput tests to test for
contending the medium. So, for example, can we trust a device which
has passed Wi-Fi certification? I don't know.

-- 
Kalle Valo

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Jonathan Guerin]

Any chance we can 'acquire' those test plans? ;-)
--
Jonathan Guerin



On Wed, Sep 15, 2010 at 5:36 PM, Kalle Valo <kvalo@adurom.com> wrote:
> Jonathan Guerin <jonathan@guerin.id.au> writes:
>
>> So, we would prefer to match the spec for an out-of-the-box build,
>> even tho it would appear to be a worse-performing driving to users?
>
> If "an out-of-the-box build" means correctly working 802.11 device
> then my opinion is yes, we should strive to match the spec even if the
> performance decreases. Getting performance increases by violating the
> spec is just stealing airtime from others.
>
> But the challange is how to know if another 802.11 device is properly
> working and following the spec? I know that Wi-Fi Alliance
> certification tests contain lots of throughput tests to test for
> contending the medium. So, for example, can we trust a device which
> has passed Wi-Fi certification? I don't know.
>
> --
> Kalle Valo
>

Re: [ath5k-devel] [ath5k] Incorrect value for ACK_TIMEOUT

[Kalle Valo]

Jonathan Guerin <jonathan@guerin.id.au> writes:

> Any chance we can 'acquire' those test plans? ;-)

Most probably you need to pay kilo dollars to get access to them.

-- 
Kalle Valo