2012-04-02 11:22:05

by Stanislaw Gruszka

[permalink] [raw]
Subject: [PATCH 1/2] rt2x00: configure different txdesc parameters for non HT channel

This is needed when we are concted to non 11n AP.

Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/rt2x00/rt2x00.h | 1 +
drivers/net/wireless/rt2x00/rt2x00config.c | 5 +++++
drivers/net/wireless/rt2x00/rt2x00queue.c | 14 ++++++++++++++
3 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
index 471f87c..8de9bfc 100644
--- a/drivers/net/wireless/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/rt2x00/rt2x00.h
@@ -692,6 +692,7 @@ enum rt2x00_state_flags {
*/
CONFIG_CHANNEL_HT40,
CONFIG_POWERSAVING,
+ CONFIG_HT_DISABLED,

/*
* Mark we currently are sequentially reading TX_STA_FIFO register
diff --git a/drivers/net/wireless/rt2x00/rt2x00config.c b/drivers/net/wireless/rt2x00/rt2x00config.c
index 293676b..e7361d9 100644
--- a/drivers/net/wireless/rt2x00/rt2x00config.c
+++ b/drivers/net/wireless/rt2x00/rt2x00config.c
@@ -217,6 +217,11 @@ void rt2x00lib_config(struct rt2x00_dev *rt2x00dev,
libconf.conf = conf;

if (ieee80211_flags & IEEE80211_CONF_CHANGE_CHANNEL) {
+ if (!conf_is_ht(conf))
+ set_bit(CONFIG_HT_DISABLED, &rt2x00dev->flags);
+ else
+ clear_bit(CONFIG_HT_DISABLED, &rt2x00dev->flags);
+
if (conf_is_ht40(conf)) {
set_bit(CONFIG_CHANNEL_HT40, &rt2x00dev->flags);
hw_value = rt2x00ht_center_channel(rt2x00dev, conf);
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
index 9b1b2b7..f7403cf 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.c
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
@@ -320,6 +320,20 @@ static void rt2x00queue_create_tx_descriptor_ht(struct rt2x00_dev *rt2x00dev,
txdesc->u.ht.wcid = sta_priv->wcid;
}

+ if (test_bit(CONFIG_HT_DISABLED, &rt2x00dev->flags)) {
+ if (ieee80211_is_beacon(hdr->frame_control))
+ txdesc->u.ht.txop = TXOP_PIFS;
+ else if (!(tx_info->flags & IEEE80211_TX_CTL_FIRST_FRAGMENT))
+ txdesc->u.ht.txop = TXOP_SIFS;
+ else
+ txdesc->u.ht.txop = TXOP_BACKOFF;
+
+ txdesc->u.ht.mcs = txrate->idx;
+
+ /* Left zero on all other settings. */
+ return;
+ }
+
txdesc->u.ht.ba_size = 7; /* FIXME: What value is needed? */

/*
--
1.7.1



2012-04-02 12:09:14

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 1/2] rt2x00: configure different txdesc parameters for non HT channel

Hi Stanislaw,

On Mon, Apr 2, 2012 at 1:21 PM, Stanislaw Gruszka <[email protected]> wrote:
> This is needed when we are concted to non 11n AP.
>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> ?drivers/net/wireless/rt2x00/rt2x00.h ? ? ? | ? ?1 +
> ?drivers/net/wireless/rt2x00/rt2x00config.c | ? ?5 +++++
> ?drivers/net/wireless/rt2x00/rt2x00queue.c ?| ? 14 ++++++++++++++
> ?3 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
> index 471f87c..8de9bfc 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00.h
> @@ -692,6 +692,7 @@ enum rt2x00_state_flags {
> ? ? ? ? */
> ? ? ? ?CONFIG_CHANNEL_HT40,
> ? ? ? ?CONFIG_POWERSAVING,
> + ? ? ? CONFIG_HT_DISABLED,
>
> ? ? ? ?/*
> ? ? ? ? * Mark we currently are sequentially reading TX_STA_FIFO register
> diff --git a/drivers/net/wireless/rt2x00/rt2x00config.c b/drivers/net/wireless/rt2x00/rt2x00config.c
> index 293676b..e7361d9 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00config.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00config.c
> @@ -217,6 +217,11 @@ void rt2x00lib_config(struct rt2x00_dev *rt2x00dev,
> ? ? ? ?libconf.conf = conf;
>
> ? ? ? ?if (ieee80211_flags & IEEE80211_CONF_CHANGE_CHANNEL) {
> + ? ? ? ? ? ? ? if (!conf_is_ht(conf))
> + ? ? ? ? ? ? ? ? ? ? ? set_bit(CONFIG_HT_DISABLED, &rt2x00dev->flags);
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? clear_bit(CONFIG_HT_DISABLED, &rt2x00dev->flags);
> +
> ? ? ? ? ? ? ? ?if (conf_is_ht40(conf)) {
> ? ? ? ? ? ? ? ? ? ? ? ?set_bit(CONFIG_CHANNEL_HT40, &rt2x00dev->flags);
> ? ? ? ? ? ? ? ? ? ? ? ?hw_value = rt2x00ht_center_channel(rt2x00dev, conf);
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
> index 9b1b2b7..f7403cf 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
> @@ -320,6 +320,20 @@ static void rt2x00queue_create_tx_descriptor_ht(struct rt2x00_dev *rt2x00dev,
> ? ? ? ? ? ? ? ?txdesc->u.ht.wcid = sta_priv->wcid;
> ? ? ? ?}
>
> + ? ? ? if (test_bit(CONFIG_HT_DISABLED, &rt2x00dev->flags)) {
> + ? ? ? ? ? ? ? if (ieee80211_is_beacon(hdr->frame_control))
> + ? ? ? ? ? ? ? ? ? ? ? txdesc->u.ht.txop = TXOP_PIFS;

Why should we use PIFS for the beacon? Is that what the ralink drivers
are doing?

> + ? ? ? ? ? ? ? else if (!(tx_info->flags & IEEE80211_TX_CTL_FIRST_FRAGMENT))
> + ? ? ? ? ? ? ? ? ? ? ? txdesc->u.ht.txop = TXOP_SIFS;
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? txdesc->u.ht.txop = TXOP_BACKOFF;
> +
> + ? ? ? ? ? ? ? txdesc->u.ht.mcs = txrate->idx;
> +

This lacks short preamble handling. Something like:

if (txrate->flags & IEEE80211_TX_RC_USE_SHORT_PREAMBLE)
txdesc->u.ht.mcs |= 0x08;

Actually, you could just shuffle the code a bit such that the rate setup
happens before all the HT stuff, no?

Helmut

2012-04-02 13:26:53

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 2/2] rt2x00: do not generate seqno in h/w if QOS is disabled

On Mon, Apr 2, 2012 at 1:21 PM, Stanislaw Gruszka <[email protected]> wrote:
> This is workaround H/W or F/W bug, see in code comments. Without the fix
> ping can receive duplicated ICMP frames while associated with legacy AP.
>
> Reported-by: Walter Goldens <[email protected]>
> Signed-off-by: Stanislaw Gruszka <[email protected]>

I don't have a better idea right now :) so this workaround looks suitable to me.

Acked-by: Helmut Schaa <[email protected]>

Thanks for tracking this down Stanislaw!

> ---
> ?drivers/net/wireless/rt2x00/rt2x00.h ? ? ?| ? ?1 +
> ?drivers/net/wireless/rt2x00/rt2x00mac.c ? | ? 10 ++++++++++
> ?drivers/net/wireless/rt2x00/rt2x00queue.c | ? 15 +++++++++++++--
> ?3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
> index 8de9bfc..5583214 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00.h
> @@ -693,6 +693,7 @@ enum rt2x00_state_flags {
> ? ? ? ?CONFIG_CHANNEL_HT40,
> ? ? ? ?CONFIG_POWERSAVING,
> ? ? ? ?CONFIG_HT_DISABLED,
> + ? ? ? CONFIG_QOS_DISABLED,
>
> ? ? ? ?/*
> ? ? ? ? * Mark we currently are sequentially reading TX_STA_FIFO register
> diff --git a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c
> index 2df2eb6..b49773e 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00mac.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00mac.c
> @@ -709,9 +709,19 @@ void rt2x00mac_bss_info_changed(struct ieee80211_hw *hw,
> ? ? ? ? ? ? ? ? ? ? ? ?rt2x00dev->intf_associated--;
>
> ? ? ? ? ? ? ? ?rt2x00leds_led_assoc(rt2x00dev, !!rt2x00dev->intf_associated);
> +
> + ? ? ? ? ? ? ? clear_bit(CONFIG_QOS_DISABLED, &rt2x00dev->flags);
> ? ? ? ?}
>
> ? ? ? ?/*
> + ? ? ? ?* Check for access point which do not support 802.11e . We have to
> + ? ? ? ?* generate data frames sequence number in S/W for such AP, because
> + ? ? ? ?* of H/W bug.
> + ? ? ? ?*/
> + ? ? ? if (changes & BSS_CHANGED_QOS && !bss_conf->qos)
> + ? ? ? ? ? ? ? set_bit(CONFIG_QOS_DISABLED, &rt2x00dev->flags);
> +
> + ? ? ? /*
> ? ? ? ? * When the erp information has changed, we should perform
> ? ? ? ? * additional configuration steps. For all other changes we are done.
> ? ? ? ? */
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
> index f7403cf..5019a3e 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
> @@ -213,8 +213,19 @@ static void rt2x00queue_create_tx_descriptor_seq(struct rt2x00_dev *rt2x00dev,
>
> ? ? ? ?__set_bit(ENTRY_TXD_GENERATE_SEQ, &txdesc->flags);
>
> - ? ? ? if (!test_bit(REQUIRE_SW_SEQNO, &rt2x00dev->cap_flags))
> - ? ? ? ? ? ? ? return;
> + ? ? ? if (!test_bit(REQUIRE_SW_SEQNO, &rt2x00dev->cap_flags)) {
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* rt2800 has a H/W (or F/W) bug, device incorrectly increase
> + ? ? ? ? ? ? ? ?* seqno on retransmited data (non-QOS) frames. To workaround
> + ? ? ? ? ? ? ? ?* the problem let's generate seqno in software if QOS is
> + ? ? ? ? ? ? ? ?* disabled.
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? if (test_bit(CONFIG_QOS_DISABLED, &rt2x00dev->flags))
> + ? ? ? ? ? ? ? ? ? ? ? __clear_bit(ENTRY_TXD_GENERATE_SEQ, &txdesc->flags);
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? /* H/W will generate sequence number */
> + ? ? ? ? ? ? ? ? ? ? ? return;
> + ? ? ? }
>
> ? ? ? ?/*
> ? ? ? ? * The hardware is not able to insert a sequence number. Assign a
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

2012-04-02 13:20:03

by Helmut Schaa

[permalink] [raw]
Subject: Re: [PATCH 1/2] rt2x00: configure different txdesc parameters for non HT channel

On Mon, Apr 2, 2012 at 2:35 PM, Stanislaw Gruszka <[email protected]> wrote:
> On Mon, Apr 02, 2012 at 02:09:14PM +0200, Helmut Schaa wrote:
>> Why should we use PIFS for the beacon? Is that what the ralink drivers
>> are doing?
> Hmm, seems that is my own invention. I thought that should be used for
> beacons, but currently I can only find in docs, that this is only needed
> for entering PCF.

Ok, so if we're running in non-HT mode we could just always stick to
TXOP_BACKOFF and only use SIFS for bursts? Makes sense I guess.

Thanks,
Helmut

2012-04-04 14:16:40

by Stanislaw Gruszka

[permalink] [raw]
Subject: [PATCH 1/2 v2] rt2x00: configure different txdesc parameters for non HT channel

This is needed when we are concted to non 11n AP.

v1 -> v2: set correctly txdesc->u.ht.txop and txdesc->u.ht.mcs

Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/rt2x00/rt2x00.h | 1 +
drivers/net/wireless/rt2x00/rt2x00config.c | 5 +++++
drivers/net/wireless/rt2x00/rt2x00queue.c | 26 ++++++++++++++++++--------
3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
index 471f87c..8de9bfc 100644
--- a/drivers/net/wireless/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/rt2x00/rt2x00.h
@@ -692,6 +692,7 @@ enum rt2x00_state_flags {
*/
CONFIG_CHANNEL_HT40,
CONFIG_POWERSAVING,
+ CONFIG_HT_DISABLED,

/*
* Mark we currently are sequentially reading TX_STA_FIFO register
diff --git a/drivers/net/wireless/rt2x00/rt2x00config.c b/drivers/net/wireless/rt2x00/rt2x00config.c
index 293676b..e7361d9 100644
--- a/drivers/net/wireless/rt2x00/rt2x00config.c
+++ b/drivers/net/wireless/rt2x00/rt2x00config.c
@@ -217,6 +217,11 @@ void rt2x00lib_config(struct rt2x00_dev *rt2x00dev,
libconf.conf = conf;

if (ieee80211_flags & IEEE80211_CONF_CHANGE_CHANNEL) {
+ if (!conf_is_ht(conf))
+ set_bit(CONFIG_HT_DISABLED, &rt2x00dev->flags);
+ else
+ clear_bit(CONFIG_HT_DISABLED, &rt2x00dev->flags);
+
if (conf_is_ht40(conf)) {
set_bit(CONFIG_CHANNEL_HT40, &rt2x00dev->flags);
hw_value = rt2x00ht_center_channel(rt2x00dev, conf);
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
index 9b1b2b7..acd0133 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.c
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
@@ -320,14 +320,6 @@ static void rt2x00queue_create_tx_descriptor_ht(struct rt2x00_dev *rt2x00dev,
txdesc->u.ht.wcid = sta_priv->wcid;
}

- txdesc->u.ht.ba_size = 7; /* FIXME: What value is needed? */
-
- /*
- * Only one STBC stream is supported for now.
- */
- if (tx_info->flags & IEEE80211_TX_CTL_STBC)
- txdesc->u.ht.stbc = 1;
-
/*
* If IEEE80211_TX_RC_MCS is set txrate->idx just contains the
* mcs rate to be used
@@ -351,6 +343,24 @@ static void rt2x00queue_create_tx_descriptor_ht(struct rt2x00_dev *rt2x00dev,
txdesc->u.ht.mcs |= 0x08;
}

+ if (test_bit(CONFIG_HT_DISABLED, &rt2x00dev->flags)) {
+ if (!(tx_info->flags & IEEE80211_TX_CTL_FIRST_FRAGMENT))
+ txdesc->u.ht.txop = TXOP_SIFS;
+ else
+ txdesc->u.ht.txop = TXOP_BACKOFF;
+
+ /* Left zero on all other settings. */
+ return;
+ }
+
+ txdesc->u.ht.ba_size = 7; /* FIXME: What value is needed? */
+
+ /*
+ * Only one STBC stream is supported for now.
+ */
+ if (tx_info->flags & IEEE80211_TX_CTL_STBC)
+ txdesc->u.ht.stbc = 1;
+
/*
* This frame is eligible for an AMPDU, however, don't aggregate
* frames that are intended to probe a specific tx rate.
--
1.7.1


2012-04-02 12:36:45

by Stanislaw Gruszka

[permalink] [raw]
Subject: Re: [PATCH 1/2] rt2x00: configure different txdesc parameters for non HT channel

Hello

On Mon, Apr 02, 2012 at 02:09:14PM +0200, Helmut Schaa wrote:
> Why should we use PIFS for the beacon? Is that what the ralink drivers
> are doing?
Hmm, seems that is my own invention. I thought that should be used for
beacons, but currently I can only find in docs, that this is only needed
for entering PCF.

> > + ? ? ? ? ? ? ? else if (!(tx_info->flags & IEEE80211_TX_CTL_FIRST_FRAGMENT))
> > + ? ? ? ? ? ? ? ? ? ? ? txdesc->u.ht.txop = TXOP_SIFS;
> > + ? ? ? ? ? ? ? else
> > + ? ? ? ? ? ? ? ? ? ? ? txdesc->u.ht.txop = TXOP_BACKOFF;
> > +
> > + ? ? ? ? ? ? ? txdesc->u.ht.mcs = txrate->idx;
> > +
>
> This lacks short preamble handling. Something like:
>
> if (txrate->flags & IEEE80211_TX_RC_USE_SHORT_PREAMBLE)
> txdesc->u.ht.mcs |= 0x08;
>
> Actually, you could just shuffle the code a bit such that the rate setup
> happens before all the HT stuff, no?
Make sense.

Stanislaw

2012-04-02 11:22:18

by Stanislaw Gruszka

[permalink] [raw]
Subject: [PATCH 2/2] rt2x00: do not generate seqno in h/w if QOS is disabled

This is workaround H/W or F/W bug, see in code comments. Without the fix
ping can receive duplicated ICMP frames while associated with legacy AP.

Reported-by: Walter Goldens <[email protected]>
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/rt2x00/rt2x00.h | 1 +
drivers/net/wireless/rt2x00/rt2x00mac.c | 10 ++++++++++
drivers/net/wireless/rt2x00/rt2x00queue.c | 15 +++++++++++++--
3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
index 8de9bfc..5583214 100644
--- a/drivers/net/wireless/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/rt2x00/rt2x00.h
@@ -693,6 +693,7 @@ enum rt2x00_state_flags {
CONFIG_CHANNEL_HT40,
CONFIG_POWERSAVING,
CONFIG_HT_DISABLED,
+ CONFIG_QOS_DISABLED,

/*
* Mark we currently are sequentially reading TX_STA_FIFO register
diff --git a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c
index 2df2eb6..b49773e 100644
--- a/drivers/net/wireless/rt2x00/rt2x00mac.c
+++ b/drivers/net/wireless/rt2x00/rt2x00mac.c
@@ -709,9 +709,19 @@ void rt2x00mac_bss_info_changed(struct ieee80211_hw *hw,
rt2x00dev->intf_associated--;

rt2x00leds_led_assoc(rt2x00dev, !!rt2x00dev->intf_associated);
+
+ clear_bit(CONFIG_QOS_DISABLED, &rt2x00dev->flags);
}

/*
+ * Check for access point which do not support 802.11e . We have to
+ * generate data frames sequence number in S/W for such AP, because
+ * of H/W bug.
+ */
+ if (changes & BSS_CHANGED_QOS && !bss_conf->qos)
+ set_bit(CONFIG_QOS_DISABLED, &rt2x00dev->flags);
+
+ /*
* When the erp information has changed, we should perform
* additional configuration steps. For all other changes we are done.
*/
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
index f7403cf..5019a3e 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.c
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
@@ -213,8 +213,19 @@ static void rt2x00queue_create_tx_descriptor_seq(struct rt2x00_dev *rt2x00dev,

__set_bit(ENTRY_TXD_GENERATE_SEQ, &txdesc->flags);

- if (!test_bit(REQUIRE_SW_SEQNO, &rt2x00dev->cap_flags))
- return;
+ if (!test_bit(REQUIRE_SW_SEQNO, &rt2x00dev->cap_flags)) {
+ /*
+ * rt2800 has a H/W (or F/W) bug, device incorrectly increase
+ * seqno on retransmited data (non-QOS) frames. To workaround
+ * the problem let's generate seqno in software if QOS is
+ * disabled.
+ */
+ if (test_bit(CONFIG_QOS_DISABLED, &rt2x00dev->flags))
+ __clear_bit(ENTRY_TXD_GENERATE_SEQ, &txdesc->flags);
+ else
+ /* H/W will generate sequence number */
+ return;
+ }

/*
* The hardware is not able to insert a sequence number. Assign a
--
1.7.1