2007-11-02 20:46:52

by Luis R. Rodriguez

[permalink] [raw]
Subject: [PATCH] ath5k: Set ACK to user lower bit rates

Sorry, forgot this last hunk in my patch series... This applies on top
of my series.

This sets the ACK bitrate to the lower rates. Without this I get
about 70% packet loss when using the 11M rate. Not sure exactly what rates
this is setting the HW to send the ACKs in but it sure does help.

I'll be poking more with this and trying to fix rates for g. We'll figure
this out ;)

Note: our higher rates are still pretty unusable, something is still
wrong with those.

Changes to base.c
Changes-licensed-under: 3-clause-BSD

Changes to ath5k.h, hw.c
Changes-licensed-under: ISC

Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath5k/ath5k.h | 2 ++
drivers/net/wireless/ath5k/base.c | 2 ++
drivers/net/wireless/ath5k/hw.c | 19 +++++++++++++++++++
3 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath5k/ath5k.h b/drivers/net/wireless/ath5k/ath5k.h
index 72c24bc..b6f5364 100644
--- a/drivers/net/wireless/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath5k/ath5k.h
@@ -1061,6 +1061,8 @@ extern int ath5k_hw_set_beacon_timers(struct ath5k_hw *ah, const struct ath5k_be
extern void ath5k_hw_reset_beacon(struct ath5k_hw *ah);
extern int ath5k_hw_wait_for_beacon(struct ath5k_hw *ah, unsigned long phys_addr);
extern void ath5k_hw_update_mib_counters(struct ath5k_hw *ah, struct ath5k_mib_stats *statistics);
+/* ACK bit rate */
+void ath5k_hw_set_ack_bitrate_high(struct ath5k_hw *ah, bool high);
/* ACK/CTS Timeouts */
extern int ath5k_hw_set_ack_timeout(struct ath5k_hw *ah, unsigned int timeout);
extern unsigned int ath5k_hw_get_ack_timeout(struct ath5k_hw *ah);
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 15ae868..8173feb 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -2193,6 +2193,8 @@ ath5k_init(struct ath5k_softc *sc)
AR5K_INT_RXORN | AR5K_INT_FATAL | AR5K_INT_GLOBAL;

ath5k_hw_set_intr(sc->ah, sc->imask);
+ /* Set ack to be sent at low bit-rates */
+ ath5k_hw_set_ack_bitrate_high(sc->ah, false);

mod_timer(&sc->calib_tim, round_jiffies(jiffies +
msecs_to_jiffies(ath5k_calinterval * 1000)));
diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
index f1ba863..f597502 100644
--- a/drivers/net/wireless/ath5k/hw.c
+++ b/drivers/net/wireless/ath5k/hw.c
@@ -2852,6 +2852,25 @@ void ath5k_hw_update_mib_counters(struct ath5k_hw *ah,
}
}

+/** ath5k_hw_set_ack_bitrate - set bitrate for ACKs
+ *
+ * @ah: the &struct ath5k_hw
+ * @high: determines if to use low bit rate or now
+ */
+void ath5k_hw_set_ack_bitrate_high(struct ath5k_hw *ah, bool high)
+{
+ if (ah->ah_version != AR5K_AR5212)
+ return;
+ else {
+ u32 val = AR5K_STA_ID1_BASE_RATE_11B | AR5K_STA_ID1_ACKCTS_6MB;
+ if (high)
+ AR5K_REG_ENABLE_BITS(ah, AR5K_STA_ID1, val);
+ else
+ AR5K_REG_DISABLE_BITS(ah, AR5K_STA_ID1, val);
+ }
+}
+
+
/*
* ACK/CTS Timeouts
*/


2007-11-02 21:13:47

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Set ACK to user lower bit rates

2007/11/2, Luis R. Rodriguez <[email protected]>:
> Sorry, forgot this last hunk in my patch series... This applies on top
> of my series.
>
> This sets the ACK bitrate to the lower rates. Without this I get
> about 70% packet loss when using the 11M rate. Not sure exactly what rates
> this is setting the HW to send the ACKs in but it sure does help.
>

6Mbits for OFDM (have checked it out with various sniffers), if not
set i usualy get 24Mbits for acks when transmiting on 54Mbit.

Haven't checked it for CCK but i guess it's 1 or 2 Mbits, have a look
at my comments inside reg.h...

#define AR5K_STA_ID1_ACKCTS_6MB 0x01000000 /* Use 6Mbit/s
for ACK/CTS (?) */
#define AR5K_STA_ID1_BASE_RATE_11B 0x02000000 /* Use 11b
base rate (for ACK/CTS ?) [5211+] */

> I'll be poking more with this and trying to fix rates for g. We'll figure
> this out ;)
>
> Note: our higher rates are still pretty unusable, something is still
> wrong with those.
>
> Changes to base.c
> Changes-licensed-under: 3-clause-BSD
>
> Changes to ath5k.h, hw.c
> Changes-licensed-under: ISC
>
> Signed-off-by: Luis R. Rodriguez <[email protected]>
> ---
> drivers/net/wireless/ath5k/ath5k.h | 2 ++
> drivers/net/wireless/ath5k/base.c | 2 ++
> drivers/net/wireless/ath5k/hw.c | 19 +++++++++++++++++++
> 3 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/ath5k/ath5k.h b/drivers/net/wireless/ath5k/ath5k.h
> index 72c24bc..b6f5364 100644
> --- a/drivers/net/wireless/ath5k/ath5k.h
> +++ b/drivers/net/wireless/ath5k/ath5k.h
> @@ -1061,6 +1061,8 @@ extern int ath5k_hw_set_beacon_timers(struct ath5k_hw *ah, const struct ath5k_be
> extern void ath5k_hw_reset_beacon(struct ath5k_hw *ah);
> extern int ath5k_hw_wait_for_beacon(struct ath5k_hw *ah, unsigned long phys_addr);
> extern void ath5k_hw_update_mib_counters(struct ath5k_hw *ah, struct ath5k_mib_stats *statistics);
> +/* ACK bit rate */
> +void ath5k_hw_set_ack_bitrate_high(struct ath5k_hw *ah, bool high);
> /* ACK/CTS Timeouts */
> extern int ath5k_hw_set_ack_timeout(struct ath5k_hw *ah, unsigned int timeout);
> extern unsigned int ath5k_hw_get_ack_timeout(struct ath5k_hw *ah);
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index 15ae868..8173feb 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
> @@ -2193,6 +2193,8 @@ ath5k_init(struct ath5k_softc *sc)
> AR5K_INT_RXORN | AR5K_INT_FATAL | AR5K_INT_GLOBAL;
>
> ath5k_hw_set_intr(sc->ah, sc->imask);
> + /* Set ack to be sent at low bit-rates */
> + ath5k_hw_set_ack_bitrate_high(sc->ah, false);
>
> mod_timer(&sc->calib_tim, round_jiffies(jiffies +
> msecs_to_jiffies(ath5k_calinterval * 1000)));
> diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
> index f1ba863..f597502 100644
> --- a/drivers/net/wireless/ath5k/hw.c
> +++ b/drivers/net/wireless/ath5k/hw.c
> @@ -2852,6 +2852,25 @@ void ath5k_hw_update_mib_counters(struct ath5k_hw *ah,
> }
> }
>
> +/** ath5k_hw_set_ack_bitrate - set bitrate for ACKs
> + *
> + * @ah: the &struct ath5k_hw
> + * @high: determines if to use low bit rate or now
> + */
> +void ath5k_hw_set_ack_bitrate_high(struct ath5k_hw *ah, bool high)
> +{
> + if (ah->ah_version != AR5K_AR5212)
> + return;
> + else {
> + u32 val = AR5K_STA_ID1_BASE_RATE_11B | AR5K_STA_ID1_ACKCTS_6MB;
> + if (high)
> + AR5K_REG_ENABLE_BITS(ah, AR5K_STA_ID1, val);
> + else
> + AR5K_REG_DISABLE_BITS(ah, AR5K_STA_ID1, val);
> + }
> +}
> +
> +
> /*
> * ACK/CTS Timeouts
> */
>


--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

2007-11-02 21:25:39

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Set ACK to user lower bit rates

2007/11/2, Nick Kossifidis <[email protected]>:
> 2007/11/2, Luis R. Rodriguez <[email protected]>:
> > Sorry, forgot this last hunk in my patch series... This applies on top
> > of my series.
> >
> > This sets the ACK bitrate to the lower rates. Without this I get
> > about 70% packet loss when using the 11M rate. Not sure exactly what rates
> > this is setting the HW to send the ACKs in but it sure does help.
> >
>
> 6Mbits for OFDM (have checked it out with various sniffers), if not
> set i usualy get 24Mbits for acks when transmiting on 54Mbit.
>
> Haven't checked it for CCK but i guess it's 1 or 2 Mbits, have a look
> at my comments inside reg.h...
>
> #define AR5K_STA_ID1_ACKCTS_6MB 0x01000000 /* Use 6Mbit/s
> for ACK/CTS (?) */
> #define AR5K_STA_ID1_BASE_RATE_11B 0x02000000 /* Use 11b
> base rate (for ACK/CTS ?) [5211+] */
>

Also note that setting these bits is safe and cause a more stable
behaviour. If these bits are not set then control_rate is being used
(check out rate tables -remember these tables are hardcoded in hal,
that's why they have put control_rate there, to let us know what rate
is being used for ack/cts frames by hw-).

Here it is...
(transmision rate) -> (ack/cts rate)

OFDM
6 - 12 -> 6
12 - 24 -> 12
24 - 54 -> 24

CCK
1 - 2 -> 1
2 - 11 -> 2

So it still doesn't make sense that you had 70% loss, i mean any rate
between 2 and 11Mbits would cause the same ack rate (2Mbits), even if
you didn't set that bit.

--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

2007-11-02 21:39:58

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Set ACK to user lower bit rates

On 11/2/07, Nick Kossifidis <[email protected]> wrote:
> 2007/11/2, Nick Kossifidis <[email protected]>:
> > 2007/11/2, Luis R. Rodriguez <[email protected]>:
> > > Sorry, forgot this last hunk in my patch series... This applies on top
> > > of my series.
> > >
> > > This sets the ACK bitrate to the lower rates. Without this I get
> > > about 70% packet loss when using the 11M rate. Not sure exactly what rates
> > > this is setting the HW to send the ACKs in but it sure does help.
> > >
> >
> > 6Mbits for OFDM (have checked it out with various sniffers), if not
> > set i usualy get 24Mbits for acks when transmiting on 54Mbit.
> >
> > Haven't checked it for CCK but i guess it's 1 or 2 Mbits, have a look
> > at my comments inside reg.h...
> >
> > #define AR5K_STA_ID1_ACKCTS_6MB 0x01000000 /* Use 6Mbit/s
> > for ACK/CTS (?) */
> > #define AR5K_STA_ID1_BASE_RATE_11B 0x02000000 /* Use 11b
> > base rate (for ACK/CTS ?) [5211+] */
> >
>
> Also note that setting these bits is safe and cause a more stable
> behaviour. If these bits are not set then control_rate is being used

Why do you say that? How do you know?

> (check out rate tables -remember these tables are hardcoded in hal,
> that's why they have put control_rate there, to let us know what rate
> is being used for ack/cts frames by hw-).
>
> Here it is...
> (transmision rate) -> (ack/cts rate)
>
> OFDM
> 6 - 12 -> 6
> 12 - 24 -> 12
> 24 - 54 -> 24
>
> CCK
> 1 - 2 -> 1
> 2 - 11 -> 2

This is not a concoction by the HAL or driver, this is what the spec
yields. Let me quote the relevant section, which is in tx.c on
mac80211:

IEEE 802.11, 9.6:
- control response frame (CTS or ACK) shall be transmitted using the
same rate as the immediately previous frame in the frame exchange
sequence, if this rate belongs to the PHY mandatory rates, or else
at the highest possible rate belonging to the PHY rates in the
BSSBasicRateSet

Also I think it might be possible to change the ACK rate per used TX
rate. I am not sure where this can be set though. In my latest
approach I just removed the control_rate all together as we should be
following the spec. I left the control rate though just to gradually
shift in the right direction and as I'm not yet 100% sure this is just
for the ACK timeout.

> So it still doesn't make sense that you had 70% loss i mean any rate
> between 2 and 11Mbits would cause the same ack rate (2Mbits), even if
> you didn't set that bit.

Yeah, it seemed strange to me too.

Luis

2007-11-03 02:48:22

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Set ACK to user lower bit rates

On 11/2/07, Nick Kossifidis <[email protected]> wrote:
> 2007/11/2, Luis R. Rodriguez <[email protected]>:
> > On 11/2/07, Nick Kossifidis <[email protected]> wrote:
> > > 2007/11/2, Nick Kossifidis <[email protected]>:
> > > > 2007/11/2, Luis R. Rodriguez <[email protected]>:
> > > > > Sorry, forgot this last hunk in my patch series... This applies on top
> > > > > of my series.
> > > > >
> > > > > This sets the ACK bitrate to the lower rates. Without this I get
> > > > > about 70% packet loss when using the 11M rate. Not sure exactly what rates
> > > > > this is setting the HW to send the ACKs in but it sure does help.
> > > > >
> > > >
> > > > 6Mbits for OFDM (have checked it out with various sniffers), if not
> > > > set i usualy get 24Mbits for acks when transmiting on 54Mbit.
> > > >
> > > > Haven't checked it for CCK but i guess it's 1 or 2 Mbits, have a look
> > > > at my comments inside reg.h...
> > > >
> > > > #define AR5K_STA_ID1_ACKCTS_6MB 0x01000000 /* Use 6Mbit/s
> > > > for ACK/CTS (?) */
> > > > #define AR5K_STA_ID1_BASE_RATE_11B 0x02000000 /* Use 11b
> > > > base rate (for ACK/CTS ?) [5211+] */
> > > >
> > >
> > > Also note that setting these bits is safe and cause a more stable
> > > behaviour. If these bits are not set then control_rate is being used
> >
> > Why do you say that? How do you know?
> >
>
> Using sniffers/analyzers with madwifi i can see that either
> control_rate is being used or base rate (i guess HAL chooses if it
> will use control_rate or base rate). I know for sure that if i
> explicitly set these bits to use lower rates with madwifi (which also
> has this function that writes directly on the register) i only get
> 6Mbits for ack frames and if i don't i usualy get 24Mbits
> (control_rate) but nothing else (that's because i'm transmitting @ 24
> - 54Mbits).

Excellent, we just need to confirm this behavior with ath5k, but of
course we're still having problems with higher rates though.. so this
may take a bit.

> > > (check out rate tables -remember these tables are hardcoded in hal,
> > > that's why they have put control_rate there, to let us know what rate
> > > is being used for ack/cts frames by hw-).
> > >
> > > Here it is...
> > > (transmision rate) -> (ack/cts rate)
> > >
> > > OFDM
> > > 6 - 12 -> 6
> > > 12 - 24 -> 12
> > > 24 - 54 -> 24
> > >
> > > CCK
> > > 1 - 2 -> 1
> > > 2 - 11 -> 2
> >
> > This is not a concoction by the HAL or driver, this is what the spec
> > yields. Let me quote the relevant section, which is in tx.c on
> > mac80211:
> >
> > IEEE 802.11, 9.6:
> > - control response frame (CTS or ACK) shall be transmitted using the
> > same rate as the immediately previous frame in the frame exchange
> > sequence, if this rate belongs to the PHY mandatory rates, or else
> > at the highest possible rate belonging to the PHY rates in the
> > BSSBasicRateSet
> >
>
> I don't see how this is relevant, as i told you i can only see ack
> frames on the above rates (6,12,24) with madwifi regardless of the
> previously transmited frame's rate,

Its relevant as this is the rules followed to determine what rate to
TX back ACKs at. Essentially what you pasted are what the ACK rates
for each respective TX rate.

> also BSSBasicRateSet is nothing
> standard, i checked out 802.11-1999 and here is what it says...
>
> "The set of data rates (in units of 500 kbit/s) that must
> be supported by all STAs to join this BSS. The STA
> that is creating the BSS must be able to receive and
> transmit at each of the data rates listed in the set."
>
> so what we get (6,12,24 for acks) is not defined anywhere, so i guess
> is hw-related.

Notice how it mentions mandatory rates though. Mandatory rates for
IEEE 802.11g PHY are 1, 2, 5.5, 11, 6, 12, 24 Mbps. I believe that's
where the 1, 2, 6, 12 and 24 come from.

> > Also I think it might be possible to change the ACK rate per used TX
> > rate. I am not sure where this can be set though. In my latest
> > approach I just removed the control_rate all together as we should be
> > following the spec. I left the control rate though just to gradually
> > shift in the right direction and as I'm not yet 100% sure this is just
> > for the ACK timeout.
> >
>
> control_rate isn't used anywhere in ath5k (like the rest of rate
> tables),

Well it is -- its used to compute the value of the rate duration
registers. I'll do some more tests and confirm the value of the ACKs
on ath5k currently by setting this low bit rate for ACKs or not.
Depending on these results we should change the current computation.

> i'm not sure if rate control algorithms on madwifi use this.

Bleh.

> As i've said before we should remove that led blinking stuff and get
> rid of them

Sure, I'll be working on that next as well as removing the rate macros.

> (btw shouldn't we register gpio led with led class ?)

Haven't looked at the led stuff yet, but feel free!

> > > So it still doesn't make sense that you had 70% loss i mean any rate
> > > between 2 and 11Mbits would cause the same ack rate (2Mbits), even if
> > > you didn't set that bit.
> >
> > Yeah, it seemed strange to me too.

BTW I was a bit far away from the access point, I just moved and get
0% packet loss now without this patch. It may have been environmental
noise.. not sure what the problem was..

Luis

2007-11-02 22:19:54

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Set ACK to user lower bit rates

2007/11/2, Luis R. Rodriguez <[email protected]>:
> On 11/2/07, Nick Kossifidis <[email protected]> wrote:
> > 2007/11/2, Nick Kossifidis <[email protected]>:
> > > 2007/11/2, Luis R. Rodriguez <[email protected]>:
> > > > Sorry, forgot this last hunk in my patch series... This applies on top
> > > > of my series.
> > > >
> > > > This sets the ACK bitrate to the lower rates. Without this I get
> > > > about 70% packet loss when using the 11M rate. Not sure exactly what rates
> > > > this is setting the HW to send the ACKs in but it sure does help.
> > > >
> > >
> > > 6Mbits for OFDM (have checked it out with various sniffers), if not
> > > set i usualy get 24Mbits for acks when transmiting on 54Mbit.
> > >
> > > Haven't checked it for CCK but i guess it's 1 or 2 Mbits, have a look
> > > at my comments inside reg.h...
> > >
> > > #define AR5K_STA_ID1_ACKCTS_6MB 0x01000000 /* Use 6Mbit/s
> > > for ACK/CTS (?) */
> > > #define AR5K_STA_ID1_BASE_RATE_11B 0x02000000 /* Use 11b
> > > base rate (for ACK/CTS ?) [5211+] */
> > >
> >
> > Also note that setting these bits is safe and cause a more stable
> > behaviour. If these bits are not set then control_rate is being used
>
> Why do you say that? How do you know?
>

Using sniffers/analyzers with madwifi i can see that either
control_rate is being used or base rate (i guess HAL chooses if it
will use control_rate or base rate). I know for sure that if i
explicitly set these bits to use lower rates with madwifi (which also
has this function that writes directly on the register) i only get
6Mbits for ack frames and if i don't i usualy get 24Mbits
(control_rate) but nothing else (that's because i'm transmitting @ 24
- 54Mbits).

> > (check out rate tables -remember these tables are hardcoded in hal,
> > that's why they have put control_rate there, to let us know what rate
> > is being used for ack/cts frames by hw-).
> >
> > Here it is...
> > (transmision rate) -> (ack/cts rate)
> >
> > OFDM
> > 6 - 12 -> 6
> > 12 - 24 -> 12
> > 24 - 54 -> 24
> >
> > CCK
> > 1 - 2 -> 1
> > 2 - 11 -> 2
>
> This is not a concoction by the HAL or driver, this is what the spec
> yields. Let me quote the relevant section, which is in tx.c on
> mac80211:
>
> IEEE 802.11, 9.6:
> - control response frame (CTS or ACK) shall be transmitted using the
> same rate as the immediately previous frame in the frame exchange
> sequence, if this rate belongs to the PHY mandatory rates, or else
> at the highest possible rate belonging to the PHY rates in the
> BSSBasicRateSet
>

I don't see how this is relevant, as i told you i can only see ack
frames on the above rates (6,12,24) with madwifi regardless of the
previously transmited frame's rate, also BSSBasicRateSet is nothing
standard, i checked out 802.11-1999 and here is what it says...

"The set of data rates (in units of 500 kbit/s) that must
be supported by all STAs to join this BSS. The STA
that is creating the BSS must be able to receive and
transmit at each of the data rates listed in the set."

so what we get (6,12,24 for acks) is not defined anywhere, so i guess
is hw-related.

> Also I think it might be possible to change the ACK rate per used TX
> rate. I am not sure where this can be set though. In my latest
> approach I just removed the control_rate all together as we should be
> following the spec. I left the control rate though just to gradually
> shift in the right direction and as I'm not yet 100% sure this is just
> for the ACK timeout.
>

control_rate isn't used anywhere in ath5k (like the rest of rate
tables), i'm not sure if rate control algorithms on madwifi use this.
As i've said before we should remove that led blinking stuff and get
rid of them (btw shouldn't we register gpio led with led class ?)

> > So it still doesn't make sense that you had 70% loss i mean any rate
> > between 2 and 11Mbits would cause the same ack rate (2Mbits), even if
> > you didn't set that bit.
>
> Yeah, it seemed strange to me too.
>
> Luis
>


--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick