Skip to contentSkip to conversation

carl9170: updates

PATCH In review Christian Lamparter 2 participants 9 messages Sep 27, 2010 – Sep 30, 2010 CARL9170 Linux community wireless driver

lore
Share & tools
lore

Christian Lamparter

author maintainer
#0 lore raw

cover carl9170: updates

A few more random changes.
Note: I do have some problems with -rc5, so this series might need some external testing.

Christian Lamparter (6):
carl9170: remove non-standard ba session teardown
carl9170: angle brackets for wiki link
carl9170: remove lost-frame workaround
carl9170: fix tx_ampdu_upload counter
carl9170: fix WARN_ON triggered by Broadcom HT STAs
carl9170: revamp carl9170_tx_prepare

 drivers/net/wireless/ath/carl9170/Kconfig    |    2 +-
 drivers/net/wireless/ath/carl9170/carl9170.h |    1 -
 drivers/net/wireless/ath/carl9170/debug.c    |    4 -
 drivers/net/wireless/ath/carl9170/main.c     |   20 ++-
 drivers/net/wireless/ath/carl9170/tx.c       |  192 ++++++++++---------------
 5 files changed, 93 insertions(+), 126 deletions(-)
author maintainer
#1 lore raw

PATCH 1/6 carl9170: remove non-standard ba session teardown

802.11n-2009 demands in 11.2.1: "
When a STA enters normal (non-APSD) PS mode, any downlink Block ACK agreement without an associated schedule is suspended for the duration of this PS mode."

The operative word is "suspended" and not terminated.

Signed-off-by Christian Lamparter <[email protected]>
---
 drivers/net/wireless/ath/carl9170/main.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)
drivers/net/wireless/ath/carl9170/main.c +0 -3
index 43de9df..d28b4ff 100644
@@ -1453,9 +1453,6 @@ static void carl9170_op_sta_notify(struct ieee80211_hw *hw,
 				while ((skb = __skb_dequeue(&tid_info->queue)))
 					__skb_queue_tail(&free, skb);
 				spin_unlock_bh(&tid_info->lock);
-
-				ieee80211_stop_tx_ba_session(sta,
-					tid_info->tid);
 			}
 			rcu_read_unlock();
 		}
Signature · 1 line
1.7.1
author maintainer
#2 lore raw

PATCH 2/6 carl9170: angle brackets for wiki link

In reply to Christian Lamparter

"The convention seems to be angle brackets around URLS in Kconfig."
-- Finn Thain (to update web addresses in the kernel)

Signed-off-by Christian Lamparter <[email protected]>
---
 drivers/net/wireless/ath/carl9170/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/wireless/ath/carl9170/Kconfig +1 -1
index c5d3a3f..2d1b821 100644
@@ -10,7 +10,7 @@ config CARL9170
 	  but it needs a special firmware (carl9170-1.fw) to do that.
 
 	  The firmware can be downloaded from our wiki here:
-	  http://wireless.kernel.org/en/users/Drivers/carl9170
+	  <http://wireless.kernel.org/en/users/Drivers/carl9170>
 
 	  If you choose to build a module, it'll be called carl9170.
Signature · 1 line
1.7.1
author maintainer
#3 lore raw

PATCH 3/6 carl9170: remove lost-frame workaround

In reply to Christian Lamparter

This patch removes some cruft, which survived the RFC review phase.

Originally, carl9170_tx_ampdu_queue erroneously dropped a lot of frames. As a result the ampdu scheduler bogged down quite frequently and the affected BA session timed out.

However this bug has been fixed and the WA and its debugfs counter is no longer useful.

Signed-off-by Christian Lamparter <[email protected]>
---
 drivers/net/wireless/ath/carl9170/carl9170.h |    1 -
 drivers/net/wireless/ath/carl9170/debug.c    |    4 ---
 drivers/net/wireless/ath/carl9170/tx.c       |   37 +------------------------
 3 files changed, 2 insertions(+), 40 deletions(-)
drivers/net/wireless/ath/carl9170/carl9170.h +0 -1
index d7c5482..20f2a77 100644
@@ -364,7 +364,6 @@ struct ar9170 {
 	unsigned int tx_dropped;
 	unsigned int tx_ack_failures;
 	unsigned int tx_fcs_errors;
-	unsigned int tx_ampdu_timeout;
 	unsigned int rx_dropped;
 
 	/* EEPROM */
drivers/net/wireless/ath/carl9170/debug.c +0 -4
index dbb35c3..19f45e9 100644
@@ -799,8 +799,6 @@ DEBUGFS_READONLY_FILE(tx_total_queued, 20, "%d",
 		      atomic_read(&ar->tx_total_queued));
 DEBUGFS_READONLY_FILE(tx_ampdu_scheduler, 20, "%d",
 		      atomic_read(&ar->tx_ampdu_scheduler));
-DEBUGFS_READONLY_FILE(tx_ampdu_timeout, 20, "%d",
-		      ar->tx_ampdu_timeout);
 
 DEBUGFS_READONLY_FILE(tx_total_pending, 20, "%d",
 		      atomic_read(&ar->tx_total_pending));

@@ -873,8 +871,6 @@ void carl9170_debugfs_register(struct ar9170 *ar)
 	DEBUGFS_ADD(ampdu_density);
 	DEBUGFS_ADD(ampdu_factor);
 
-	DEBUGFS_ADD(tx_ampdu_timeout);
-
 	DEBUGFS_ADD(tx_janitor_last_run);
 
 	DEBUGFS_ADD(tx_status_0);
drivers/net/wireless/ath/carl9170/tx.c +2 -35
index e0d2374..e6be5e6 100644
@@ -1042,41 +1042,8 @@ retry:
 		queue = TID_TO_WME_AC(tid_info->tid);
 
 		spin_lock_bh(&tid_info->lock);
-		if (tid_info->state != CARL9170_TID_STATE_XMIT) {
-			first = skb_peek(&tid_info->queue);
-			if (first) {
-				struct ieee80211_tx_info *txinfo;
-				struct carl9170_tx_info *arinfo;
-
-				txinfo = IEEE80211_SKB_CB(first);
-				arinfo = (void *) txinfo->rate_driver_data;
-
-				if (time_is_after_jiffies(arinfo->timeout +
-				    msecs_to_jiffies(CARL9170_QUEUE_TIMEOUT))
-				    == true)
-					goto processed;
-
-				/*
-				 * We've been waiting for the frame which
-				 * matches "snx" (start sequence of the
-				 * next aggregate) for some time now.
-				 *
-				 * But it never arrived. Therefore
-				 * jump to the next available frame
-				 * and kick-start the transmission.
-				 *
-				 * Note: This might induce odd latency
-				 * spikes because the receiver will be
-				 * waiting for the lost frame too.
-				 */
-				ar->tx_ampdu_timeout++;
-
-				tid_info->snx = carl9170_get_seq(first);
-				tid_info->state = CARL9170_TID_STATE_XMIT;
-			} else {
-				goto processed;
-			}
-		}
+		if (tid_info->state != CARL9170_TID_STATE_XMIT)
+			goto processed;
 
 		tid_info->counter++;
 		first = skb_peek(&tid_info->queue);
Signature · 1 line
1.7.1
author maintainer
#4 lore raw

PATCH 4/6 carl9170: fix tx_ampdu_upload counter

In reply to Christian Lamparter

tx_ampdu_upload was not decreased when an a-MPDU frame had to be kicked out from the tx_pending queues.

This broke ampdu aggregation, because the scheduler waits until tx_ampdu_upload drops to zero, before making the next aggregate.

Signed-off-by Christian Lamparter <[email protected]>
---
 drivers/net/wireless/ath/carl9170/main.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)
drivers/net/wireless/ath/carl9170/main.c +14 -1
index d28b4ff..a8b0cec 100644
@@ -230,8 +230,15 @@ static void carl9170_flush(struct ar9170 *ar, bool drop_queued)
 		for (i = 0; i < ar->hw->queues; i++) {
 			struct sk_buff *skb;
 
-			while ((skb = skb_dequeue(&ar->tx_pending[i])))
+			while ((skb = skb_dequeue(&ar->tx_pending[i]))) {
+				struct ieee80211_tx_info *info;
+
+				info = IEEE80211_SKB_CB(skb);
+				if (info->flags & IEEE80211_TX_CTL_AMPDU)
+					atomic_dec(&ar->tx_ampdu_upload);
+
 				carl9170_tx_status(ar, skb, false);
+			}
 		}
 	}
 

@@ -1462,6 +1469,7 @@ static void carl9170_op_sta_notify(struct ieee80211_hw *hw,
 			skb_queue_walk_safe(&ar->tx_pending[i], skb, tmp) {
 				struct _carl9170_tx_superframe *super;
 				struct ieee80211_hdr *hdr;
+				struct ieee80211_tx_info *info;
 
 				super = (void *) skb->data;
 				hdr = (void *) super->frame_data;

@@ -1470,6 +1478,11 @@ static void carl9170_op_sta_notify(struct ieee80211_hw *hw,
 					continue;
 
 				__skb_unlink(skb, &ar->tx_pending[i]);
+
+				info = IEEE80211_SKB_CB(skb);
+				if (info->flags & IEEE80211_TX_CTL_AMPDU)
+					atomic_dec(&ar->tx_ampdu_upload);
+
 				carl9170_tx_status(ar, skb, false);
 			}
 			spin_unlock_bh(&ar->tx_pending[i].lock);
Signature · 1 line
1.7.1
author maintainer
#5 lore raw

PATCH 5/6 carl9170: fix WARN_ON triggered by Broadcom HT STAs

In reply to Christian Lamparter

Broadcom's Windows driver for the 4313 advertises an ampdu density of 7 => 16 us. The AR9170 MAC on the other hand only supports densities up to 8 us.

This patch removes the noisy WARN_ON, because there is nothing we can do about it.

Signed-off-by Christian Lamparter <[email protected]>
---
 drivers/net/wireless/ath/carl9170/main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/wireless/ath/carl9170/main.c +1 -1
index a8b0cec..84bd38e 100644
@@ -1248,7 +1248,7 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 
 	switch (action) {
 	case IEEE80211_AMPDU_TX_START:
-		if (WARN_ON_ONCE(!sta_info->ht_sta))
+		if (!sta_info->ht_sta)
 			return -EOPNOTSUPP;
 
 		rcu_read_lock();
Signature · 1 line
1.7.1
author maintainer
#6 lore raw

PATCH 6/6 carl9170: revamp carl9170_tx_prepare

In reply to Christian Lamparter

David Miller complained about the driver's excessive use of variables in __packed structs. While I did not fully agree with his sole "performance" argument on all accounts. I do see some room for improvement in hot-paths on architectures without an efficient access to unaligned elements.

This first patch (dare I say?) optimizes an important tx hot-path in the driver: carl9170_tx_prepare.

Signed-off-by Christian Lamparter <[email protected]>
---
 drivers/net/wireless/ath/carl9170/tx.c |  155 +++++++++++++++----------------
 1 files changed, 75 insertions(+), 80 deletions(-)
drivers/net/wireless/ath/carl9170/tx.c +75 -80
index e6be5e6..b575c86 100644
@@ -760,8 +760,8 @@ static int carl9170_tx_prepare(struct ar9170 *ar, struct sk_buff *skb)
 	struct carl9170_tx_info *arinfo;
 	unsigned int hw_queue;
 	int i;
-	u16 keytype = 0;
-	u16 len, icv = 0;
+	__le16 mac_tmp;
+	u16 len;
 	bool ampdu, no_ack;
 
 	BUILD_BUG_ON(sizeof(*arinfo) > sizeof(info->rate_driver_data));

@@ -773,6 +773,10 @@ static int carl9170_tx_prepare(struct ar9170 *ar, struct sk_buff *skb)
 
 	BUILD_BUG_ON(IEEE80211_TX_MAX_RATES < CARL9170_TX_MAX_RATES);
 
+	BUILD_BUG_ON(AR9170_MAX_VIRTUAL_MAC >
+		((CARL9170_TX_SUPER_MISC_VIF_ID >>
+		 CARL9170_TX_SUPER_MISC_VIF_ID_S) + 1));
+
 	hw_queue = ar9170_qmap[carl9170_get_queue(ar, skb)];
 
 	hdr = (void *)skb->data;

@@ -793,20 +797,37 @@ static int carl9170_tx_prepare(struct ar9170 *ar, struct sk_buff *skb)
 	txc = (void *)skb_push(skb, sizeof(*txc));
 	memset(txc, 0, sizeof(*txc));
 
-	ampdu = !!(info->flags & IEEE80211_TX_CTL_AMPDU);
+	SET_VAL(CARL9170_TX_SUPER_MISC_QUEUE, txc->s.misc, hw_queue);
+
+	if (likely(cvif))
+		SET_VAL(CARL9170_TX_SUPER_MISC_VIF_ID, txc->s.misc, cvif->id);
+
+	if (unlikely(info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM))
+		txc->s.misc |= CARL9170_TX_SUPER_MISC_CAB;
+
+	if (unlikely(ieee80211_is_probe_resp(hdr->frame_control)))
+		txc->s.misc |= CARL9170_TX_SUPER_MISC_FILL_IN_TSF;
+
+	mac_tmp = cpu_to_le16(AR9170_TX_MAC_HW_DURATION |
+			      AR9170_TX_MAC_BACKOFF);
+	mac_tmp |= cpu_to_le16((hw_queue << AR9170_TX_MAC_QOS_S) &&
+			       AR9170_TX_MAC_QOS);
+
 	no_ack = !!(info->flags & IEEE80211_TX_CTL_NO_ACK);
+	if (unlikely(no_ack))
+		mac_tmp |= cpu_to_le16(AR9170_TX_MAC_NO_ACK);
 
 	if (info->control.hw_key) {
-		icv = info->control.hw_key->icv_len;
+		len += info->control.hw_key->icv_len;
 
 		switch (info->control.hw_key->cipher) {
 		case WLAN_CIPHER_SUITE_WEP40:
 		case WLAN_CIPHER_SUITE_WEP104:
 		case WLAN_CIPHER_SUITE_TKIP:
-			keytype = AR9170_TX_MAC_ENCR_RC4;
+			mac_tmp |= cpu_to_le16(AR9170_TX_MAC_ENCR_RC4);
 			break;
 		case WLAN_CIPHER_SUITE_CCMP:
-			keytype = AR9170_TX_MAC_ENCR_AES;
+			mac_tmp |= cpu_to_le16(AR9170_TX_MAC_ENCR_AES);
 			break;
 		default:
 			WARN_ON(1);

@@ -814,48 +835,58 @@ static int carl9170_tx_prepare(struct ar9170 *ar, struct sk_buff *skb)
 		}
 	}
 
-	BUILD_BUG_ON(AR9170_MAX_VIRTUAL_MAC >
-		((CARL9170_TX_SUPER_MISC_VIF_ID >>
-		 CARL9170_TX_SUPER_MISC_VIF_ID_S) + 1));
-
-	txc->s.len = cpu_to_le16(len + sizeof(*txc));
-	txc->f.length = cpu_to_le16(len + icv + 4);
-	SET_VAL(CARL9170_TX_SUPER_MISC_VIF_ID, txc->s.misc,
-		cvif ? cvif->id : 0);
+	ampdu = !!(info->flags & IEEE80211_TX_CTL_AMPDU);
+	if (ampdu) {
+		unsigned int density, factor;
 
-	txc->f.mac_control = cpu_to_le16(AR9170_TX_MAC_HW_DURATION |
-					 AR9170_TX_MAC_BACKOFF);
+		if (unlikely(!sta || !cvif))
+			goto err_out;
 
-	SET_VAL(CARL9170_TX_SUPER_MISC_QUEUE, txc->s.misc, hw_queue);
+		factor = min_t(unsigned int, 1u,
+			 info->control.sta->ht_cap.ampdu_factor);
 
-	txc->f.mac_control |= cpu_to_le16(hw_queue << AR9170_TX_MAC_QOS_S);
-	txc->f.mac_control |= cpu_to_le16(keytype);
-	txc->f.phy_control = cpu_to_le32(0);
+		density = info->control.sta->ht_cap.ampdu_density;
 
-	if (no_ack)
-		txc->f.mac_control |= cpu_to_le16(AR9170_TX_MAC_NO_ACK);
+		if (density) {
+			/*
+			 * Watch out!
+			 *
+			 * Otus uses slightly different density values than
+			 * those from the 802.11n spec.
+			 */
 
-	if (info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM)
-		txc->s.misc |= CARL9170_TX_SUPER_MISC_CAB;
+			density = max_t(unsigned int, density + 1, 7u);
+		}
 
-	txrate = &info->control.rates[0];
-	if (carl9170_tx_rts_check(ar, txrate, ampdu, no_ack))
-		txc->f.mac_control |= cpu_to_le16(AR9170_TX_MAC_PROT_RTS);
-	else if (carl9170_tx_cts_check(ar, txrate))
-		txc->f.mac_control |= cpu_to_le16(AR9170_TX_MAC_PROT_CTS);
+		SET_VAL(CARL9170_TX_SUPER_AMPDU_DENSITY,
+			txc->s.ampdu_settings, density);
 
-	SET_VAL(CARL9170_TX_SUPER_RI_TRIES, txc->s.ri[0], txrate->count);
-	txc->f.phy_control |= carl9170_tx_physet(ar, info, txrate);
+		SET_VAL(CARL9170_TX_SUPER_AMPDU_FACTOR,
+			txc->s.ampdu_settings, factor);
 
-	if (info->flags & IEEE80211_TX_CTL_AMPDU) {
-		for (i = 1; i < CARL9170_TX_MAX_RATES; i++) {
+		for (i = 0; i < CARL9170_TX_MAX_RATES; i++) {
 			txrate = &info->control.rates[i];
-			if (txrate->idx >= 0)
+			if (txrate->idx >= 0) {
+				txc->s.ri[i] =
+					CARL9170_TX_SUPER_RI_AMPDU;
+
+				if (WARN_ON(!(txrate->flags &
+					      IEEE80211_TX_RC_MCS))) {
+					/*
+					 * Not sure if it's even possible
+					 * to aggregate non-ht rates with
+					 * this HW.
+					 */
+					goto err_out;
+				}
 				continue;
+			}
 
 			txrate->idx = 0;
 			txrate->count = ar->hw->max_rate_tries;
 		}
+
+		mac_tmp |= cpu_to_le16(AR9170_TX_MAC_AGGR);
 	}
 
 	/*

@@ -878,57 +909,21 @@ static int carl9170_tx_prepare(struct ar9170 *ar, struct sk_buff *skb)
 			txc->s.ri[i] |= (AR9170_TX_MAC_PROT_CTS <<
 				CARL9170_TX_SUPER_RI_ERP_PROT_S);
 
-		/*
-		 * unaggregated fallback, in case aggregation
-		 * proves to be unsuccessful and unreliable.
-		 */
-		if (ampdu && i < 3)
-			txc->s.ri[i] |= CARL9170_TX_SUPER_RI_AMPDU;
-
 		txc->s.rr[i - 1] = carl9170_tx_physet(ar, info, txrate);
 	}
 
-	if (ieee80211_is_probe_resp(hdr->frame_control))
-		txc->s.misc |= CARL9170_TX_SUPER_MISC_FILL_IN_TSF;
-
-	if (ampdu) {
-		unsigned int density, factor;
-
-		if (unlikely(!sta || !cvif))
-			goto err_out;
-
-		density = info->control.sta->ht_cap.ampdu_density;
-		factor = info->control.sta->ht_cap.ampdu_factor;
-
-		if (density) {
-			/*
-			 * Watch out!
-			 *
-			 * Otus uses slightly different density values than
-			 * those from the 802.11n spec.
-			 */
-
-			density = max_t(unsigned int, density + 1, 7u);
-		}
-
-		factor = min_t(unsigned int, 1u, factor);
-
-		SET_VAL(CARL9170_TX_SUPER_AMPDU_DENSITY,
-			txc->s.ampdu_settings, density);
+	txrate = &info->control.rates[0];
+	SET_VAL(CARL9170_TX_SUPER_RI_TRIES, txc->s.ri[0], txrate->count);
 
-		SET_VAL(CARL9170_TX_SUPER_AMPDU_FACTOR,
-			txc->s.ampdu_settings, factor);
+	if (carl9170_tx_rts_check(ar, txrate, ampdu, no_ack))
+		mac_tmp |= cpu_to_le16(AR9170_TX_MAC_PROT_RTS);
+	else if (carl9170_tx_cts_check(ar, txrate))
+		mac_tmp |= cpu_to_le16(AR9170_TX_MAC_PROT_CTS);
 
-		if (info->control.rates[0].flags & IEEE80211_TX_RC_MCS) {
-			txc->f.mac_control |= cpu_to_le16(AR9170_TX_MAC_AGGR);
-		} else {
-			/*
-			 * Not sure if it's even possible to aggregate
-			 * non-ht rates with this HW.
-			 */
-			WARN_ON_ONCE(1);
-		}
-	}
+	txc->s.len = cpu_to_le16(skb->len);
+	txc->f.length = cpu_to_le16(len + FCS_LEN);
+	txc->f.mac_control = mac_tmp;
+	txc->f.phy_control = carl9170_tx_physet(ar, info, txrate);
 
 	arinfo = (void *)info->rate_driver_data;
 	arinfo->timeout = jiffies;
Signature · 1 line
1.7.1
#7 lore raw
In reply to Christian Lamparter
Quoted 11 lines · Christian Lamparter

A few more random changes.
Note: I do have some problems with -rc5, so this series might need some external testing.

Christian Lamparter (6):
carl9170: remove non-standard ba session teardown
carl9170: angle brackets for wiki link
carl9170: remove lost-frame workaround
carl9170: fix tx_ampdu_upload counter
carl9170: fix WARN_ON triggered by Broadcom HT STAs
carl9170: revamp carl9170_tx_prepare

Nothing to address davem's _packed comments?

Signature · 2 lines
John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.
#8 lore raw
Quoted 13 lines · John W. Linville
Quoted 11 lines · Christian Lamparter

A few more random changes.
Note: I do have some problems with -rc5, so this series might need some external testing.

Christian Lamparter (6):
carl9170: remove non-standard ba session teardown
carl9170: angle brackets for wiki link
carl9170: remove lost-frame workaround
carl9170: fix tx_ampdu_upload counter
carl9170: fix WARN_ON triggered by Broadcom HT STAs
carl9170: revamp carl9170_tx_prepare

Quoted 1 line

Nothing to address davem's _packed comments?

Please ignore this, I found them. :-)

Signature · 2 lines
John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.