From 3995bd9332a51b626237d6671cfeb7235e6c1305 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Fri, 24 Jul 2009 11:13:14 -0700 Subject: iwlwifi: fix TX queue race I had a problem on 4965 hardware (well, probably other hardware too, but others don't survive my stress testing right now, unfortunately) where the driver was sending invalid commands to the device, but no such thing could be seen from the driver's point of view. I could reproduce this fairly easily by sending multiple TCP streams with iperf on different TIDs, though sometimes a single iperf stream was sufficient. It even happened with a single core, but I have forced preemption turned on. The culprit was a queue overrun, where we advanced the queue's write pointer over the read pointer. After careful analysis I've come to the conclusion that the cause is a race condition between iwlwifi and mac80211. mac80211, of course, checks whether the queue is stopped, before transmitting a frame. This effectively looks like this: lock(queues) if (stopped(queue)) { unlock(queues) return busy; } unlock(queues) ... <-- this place will be important there is some more code here drv_tx(frame) The driver, on the other hand, can stop and start queues, which does lock(queues) mark_running/stopped(queue) unlock(queues) [if marked running: wake up tasklet to send pending frames] Now, however, once the driver starts the queue, mac80211 can see that and end up at the marked place above, at which point for some reason the driver seems to stop the queue again (I don't understand that) and then we end up transmitting while the queue is actually full. Now, this shouldn't actually matter much, but for some reason I've seen it happen multiple times in a row and the queue actually overflows, at which point the queue bites itself in the tail and things go completely wrong. This patch fixes this by just dropping the packet should this have happened, and making the lock in iwlwifi cover everything so iwlwifi can't race against itself (dropping the lock there might make it more likely, but it did seem to happen without that too). Since we can't hold the lock across drv_tx() above, I see no way to fix this in mac80211, but I also don't understand why I haven't seen this before -- maybe I just never stress tested it this badly. With this patch, the device has survived many minutes of simultanously sending two iperf streams on different TIDs with combined throughput of about 60 Mbps. Signed-off-by: Johannes Berg Signed-off-by: Reinette Chatre Signed-off-by: John W. Linville --- drivers/net/wireless/iwlwifi/iwl-tx.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'drivers/net/wireless/iwlwifi') diff --git a/drivers/net/wireless/iwlwifi/iwl-tx.c b/drivers/net/wireless/iwlwifi/iwl-tx.c index 9bbeec9427f..5febb318636 100644 --- a/drivers/net/wireless/iwlwifi/iwl-tx.c +++ b/drivers/net/wireless/iwlwifi/iwl-tx.c @@ -720,8 +720,6 @@ int iwl_tx_skb(struct iwl_priv *priv, struct sk_buff *skb) goto drop_unlock; } - spin_unlock_irqrestore(&priv->lock, flags); - hdr_len = ieee80211_hdrlen(fc); /* Find (or create) index into station table for destination station */ @@ -729,7 +727,7 @@ int iwl_tx_skb(struct iwl_priv *priv, struct sk_buff *skb) if (sta_id == IWL_INVALID_STATION) { IWL_DEBUG_DROP(priv, "Dropping - INVALID STATION: %pM\n", hdr->addr1); - goto drop; + goto drop_unlock; } IWL_DEBUG_TX(priv, "station Id %d\n", sta_id); @@ -750,14 +748,17 @@ int iwl_tx_skb(struct iwl_priv *priv, struct sk_buff *skb) txq_id = priv->stations[sta_id].tid[tid].agg.txq_id; swq_id = iwl_virtual_agg_queue_num(swq_id, txq_id); } - priv->stations[sta_id].tid[tid].tfds_in_queue++; } txq = &priv->txq[txq_id]; q = &txq->q; txq->swq_id = swq_id; - spin_lock_irqsave(&priv->lock, flags); + if (unlikely(iwl_queue_space(q) < q->high_mark)) + goto drop_unlock; + + if (ieee80211_is_data_qos(fc)) + priv->stations[sta_id].tid[tid].tfds_in_queue++; /* Set up driver data for this TFD */ memset(&(txq->txb[q->write_ptr]), 0, sizeof(struct iwl_tx_info)); @@ -902,7 +903,6 @@ int iwl_tx_skb(struct iwl_priv *priv, struct sk_buff *skb) drop_unlock: spin_unlock_irqrestore(&priv->lock, flags); -drop: return -1; } EXPORT_SYMBOL(iwl_tx_skb); -- cgit v1.2.3 From 45f5fa32b130b2a59f9b726be45ce7fa73fb834c Mon Sep 17 00:00:00 2001 From: reinette chatre Date: Tue, 21 Jul 2009 09:29:07 -0700 Subject: iwlagn: fix minimum number of queues setting We need to provide a reasonable minimum that will result in a working setup if used. Set minimum to be 10 to provide for 4 standard TX queues + 1 command queue + 2 (unused) HCCA queues + 4 HT queues (one per AC). We allow the user to change the number of queues used via a module parameter and use this minimum value to check if it is valid. Without this patch a user can select a value for the number of queues that will result in a failing setup. Signed-off-by: Reinette Chatre Reviewed-by: Tomas Winkler Acked-by: Tomas Winkler Signed-off-by: John W. Linville --- drivers/net/wireless/iwlwifi/iwl-3945.h | 2 +- drivers/net/wireless/iwlwifi/iwl-dev.h | 6 ++++-- drivers/net/wireless/iwlwifi/iwl3945-base.c | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) (limited to 'drivers/net/wireless/iwlwifi') diff --git a/drivers/net/wireless/iwlwifi/iwl-3945.h b/drivers/net/wireless/iwlwifi/iwl-3945.h index fbb3a573463..2de6471d4be 100644 --- a/drivers/net/wireless/iwlwifi/iwl-3945.h +++ b/drivers/net/wireless/iwlwifi/iwl-3945.h @@ -112,7 +112,7 @@ enum iwl3945_antenna { #define IWL_TX_FIFO_NONE 7 /* Minimum number of queues. MAX_NUM is defined in hw specific files */ -#define IWL_MIN_NUM_QUEUES 4 +#define IWL39_MIN_NUM_QUEUES 4 #define IEEE80211_DATA_LEN 2304 #define IEEE80211_4ADDR_LEN 30 diff --git a/drivers/net/wireless/iwlwifi/iwl-dev.h b/drivers/net/wireless/iwlwifi/iwl-dev.h index e2d620f0b6e..650e20af20f 100644 --- a/drivers/net/wireless/iwlwifi/iwl-dev.h +++ b/drivers/net/wireless/iwlwifi/iwl-dev.h @@ -258,8 +258,10 @@ struct iwl_channel_info { #define IWL_TX_FIFO_HCCA_2 6 #define IWL_TX_FIFO_NONE 7 -/* Minimum number of queues. MAX_NUM is defined in hw specific files */ -#define IWL_MIN_NUM_QUEUES 4 +/* Minimum number of queues. MAX_NUM is defined in hw specific files. + * Set the minimum to accommodate the 4 standard TX queues, 1 command + * queue, 2 (unused) HCCA queues, and 4 HT queues (one for each AC) */ +#define IWL_MIN_NUM_QUEUES 10 /* Power management (not Tx power) structures */ diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c index 956798f2c80..2f50ab60bfd 100644 --- a/drivers/net/wireless/iwlwifi/iwl3945-base.c +++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c @@ -4018,10 +4018,10 @@ static int iwl3945_pci_probe(struct pci_dev *pdev, const struct pci_device_id *e SET_IEEE80211_DEV(hw, &pdev->dev); if ((iwl3945_mod_params.num_of_queues > IWL39_MAX_NUM_QUEUES) || - (iwl3945_mod_params.num_of_queues < IWL_MIN_NUM_QUEUES)) { + (iwl3945_mod_params.num_of_queues < IWL39_MIN_NUM_QUEUES)) { IWL_ERR(priv, "invalid queues_num, should be between %d and %d\n", - IWL_MIN_NUM_QUEUES, IWL39_MAX_NUM_QUEUES); + IWL39_MIN_NUM_QUEUES, IWL39_MAX_NUM_QUEUES); err = -EINVAL; goto out_ieee80211_free_hw; } -- cgit v1.2.3 From 082e708acc50a5b625b9bde0bb1af90dfdbd1942 Mon Sep 17 00:00:00 2001 From: Roel Kluin Date: Sat, 25 Jul 2009 23:34:31 +0200 Subject: iwlwifi: Read outside array bounds tid is bounded (above) by the size of default_tid_to_tx_fifo (17 elements), but the size of priv->stations[].tid[] is MAX_TID_COUNT (9) elements. Signed-off-by: Roel Kluin Signed-off-by: John W. Linville --- drivers/net/wireless/iwlwifi/iwl-tx.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/net/wireless/iwlwifi') diff --git a/drivers/net/wireless/iwlwifi/iwl-tx.c b/drivers/net/wireless/iwlwifi/iwl-tx.c index 5febb318636..2e89040e63b 100644 --- a/drivers/net/wireless/iwlwifi/iwl-tx.c +++ b/drivers/net/wireless/iwlwifi/iwl-tx.c @@ -1171,6 +1171,8 @@ int iwl_tx_agg_start(struct iwl_priv *priv, const u8 *ra, u16 tid, u16 *ssn) IWL_ERR(priv, "Start AGG on invalid station\n"); return -ENXIO; } + if (unlikely(tid >= MAX_TID_COUNT)) + return -EINVAL; if (priv->stations[sta_id].tid[tid].agg.state != IWL_AGG_OFF) { IWL_ERR(priv, "Start AGG when state is not IWL_AGG_OFF !\n"); -- cgit v1.2.3 From 371842448c05b42d11a4be1c8e4e81d62ecc7534 Mon Sep 17 00:00:00 2001 From: "Luis R. Rodriguez" Date: Thu, 30 Jul 2009 17:43:48 -0700 Subject: cfg80211: fix regression on beacon world roaming feature A regression was added through patch a4ed90d6: "cfg80211: respect API on orig_flags on channel for beacon hint" We did indeed respect _orig flags but the intention was not clearly stated in the commit log. This patch fixes firmware issues picked up by iwlwifi when we lift passive scan of beaconing restrictions on channels its EEPROM has been configured to always enable. By doing so though we also disallowed beacon hints on devices registering their wiphy with custom world regulatory domains enabled, this happens to be currently ath5k, ath9k and ar9170. The passive scan and beacon restrictions on those devices would never be lifted even if we did find a beacon and the hardware did support such enhancements when world roaming. Since Johannes indicates iwlwifi firmware cannot be changed to allow beacon hinting we set up a flag now to specifically allow drivers to disable beacon hints for devices which cannot use them. We enable the flag on iwlwifi to disable beacon hints and by default enable it for all other drivers. It should be noted beacon hints lift passive scan flags and beacon restrictions when we receive a beacon from an AP on any 5 GHz non-DFS channels, and channels 12-14 on the 2.4 GHz band. We don't bother with channels 1-11 as those channels are allowed world wide. This should fix world roaming for ath5k, ath9k and ar9170, thereby improving scan time when we receive the first beacon from any AP, and also enabling beaconing operation (AP/IBSS/Mesh) on cards which would otherwise not be allowed to do so. Drivers not using custom regulatory stuff (wiphy_apply_custom_regulatory()) were not affected by this as the orig_flags for the channels would have been cleared upon wiphy registration. I tested this with a world roaming ath5k card. Cc: Jouni Malinen Signed-off-by: Luis R. Rodriguez Reviewed-by: Johannes Berg Signed-off-by: John W. Linville --- drivers/net/wireless/iwlwifi/iwl-core.c | 3 +++ drivers/net/wireless/iwlwifi/iwl3945-base.c | 3 +++ 2 files changed, 6 insertions(+) (limited to 'drivers/net/wireless/iwlwifi') diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c index 6ab07165ea2..18b135f510e 100644 --- a/drivers/net/wireless/iwlwifi/iwl-core.c +++ b/drivers/net/wireless/iwlwifi/iwl-core.c @@ -1332,6 +1332,9 @@ int iwl_setup_mac(struct iwl_priv *priv) hw->wiphy->custom_regulatory = true; + /* Firmware does not support this */ + hw->wiphy->disable_beacon_hints = true; + hw->wiphy->max_scan_ssids = PROBE_OPTION_MAX; /* we create the 802.11 header and a zero-length SSID element */ hw->wiphy->max_scan_ie_len = IWL_MAX_PROBE_REQUEST - 24 - 2; diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c index 2f50ab60bfd..523843369ca 100644 --- a/drivers/net/wireless/iwlwifi/iwl3945-base.c +++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c @@ -3968,6 +3968,9 @@ static int iwl3945_setup_mac(struct iwl_priv *priv) hw->wiphy->custom_regulatory = true; + /* Firmware does not support this */ + hw->wiphy->disable_beacon_hints = true; + hw->wiphy->max_scan_ssids = PROBE_OPTION_MAX_3945; /* we create the 802.11 header and a zero-length SSID element */ hw->wiphy->max_scan_ie_len = IWL_MAX_PROBE_REQUEST - 24 - 2; -- cgit v1.2.3 From c37457e69ffd7d3c94cbfcc1c39be9a45dd7ad21 Mon Sep 17 00:00:00 2001 From: Julia Lawall Date: Mon, 3 Aug 2009 11:11:45 +0200 Subject: drivers/net/wireless/iwlwifi: introduce missing kfree Move orthogonal error handling code up before a kzalloc, so that it doesn't have to free the allocated data. The semantic match that finds the problem is as follows: (http://www.emn.fr/x-info/coccinelle/) // @r exists@ local idexpression x; statement S; expression E; identifier f,f1,l; position p1,p2; expression *ptr != NULL; @@ x@p1 = \(kmalloc\|kzalloc\|kcalloc\)(...); ... if (x == NULL) S <... when != x when != if (...) { <+...x...+> } ( x->f1 = E | (x->f1 == NULL || ...) | f(...,x->f1,...) ) ...> ( return \(0\|<+...x...+>\|ptr\); | return@p2 ...; ) @script:python@ p1 << r.p1; p2 << r.p2; @@ print "* file: %s kmalloc %s return %s" % (p1[0].file,p1[0].line,p2[0].line) // Signed-off-by: Julia Lawall Acked-by: Zhu Yi Signed-off-by: John W. Linville --- drivers/net/wireless/iwlwifi/iwl-debugfs.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'drivers/net/wireless/iwlwifi') diff --git a/drivers/net/wireless/iwlwifi/iwl-debugfs.c b/drivers/net/wireless/iwlwifi/iwl-debugfs.c index 11e08c06891..ca00cc8ad4c 100644 --- a/drivers/net/wireless/iwlwifi/iwl-debugfs.c +++ b/drivers/net/wireless/iwlwifi/iwl-debugfs.c @@ -308,18 +308,18 @@ static ssize_t iwl_dbgfs_nvm_read(struct file *file, return -ENODATA; } + ptr = priv->eeprom; + if (!ptr) { + IWL_ERR(priv, "Invalid EEPROM/OTP memory\n"); + return -ENOMEM; + } + /* 4 characters for byte 0xYY */ buf = kzalloc(buf_size, GFP_KERNEL); if (!buf) { IWL_ERR(priv, "Can not allocate Buffer\n"); return -ENOMEM; } - - ptr = priv->eeprom; - if (!ptr) { - IWL_ERR(priv, "Invalid EEPROM/OTP memory\n"); - return -ENOMEM; - } pos += scnprintf(buf + pos, buf_size - pos, "NVM Type: %s\n", (priv->nvm_device_type == NVM_DEVICE_TYPE_OTP) ? "OTP" : "EEPROM"); -- cgit v1.2.3 From 99f1b01562b7dcae75b043114f76163fbf84fcab Mon Sep 17 00:00:00 2001 From: Reinette Chatre Date: Mon, 3 Aug 2009 12:10:16 -0700 Subject: iwlagn: do not send key clear commands when rfkill enabled Do all key clearing except sending sommands to device when rfkill enabled. When rfkill enabled the interface is brought down and will be brought back up correctly after rfkill is enabled again. Same change is not needed for iwl3945 as it ignores return code when sending key clearing command to device. This fixes http://bugzilla.kernel.org/show_bug.cgi?id=13742 Signed-off-by: Reinette Chatre Tested-by: Frans Pop Signed-off-by: John W. Linville --- drivers/net/wireless/iwlwifi/iwl-sta.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'drivers/net/wireless/iwlwifi') diff --git a/drivers/net/wireless/iwlwifi/iwl-sta.c b/drivers/net/wireless/iwlwifi/iwl-sta.c index 2addf735b19..ffd5c61a755 100644 --- a/drivers/net/wireless/iwlwifi/iwl-sta.c +++ b/drivers/net/wireless/iwlwifi/iwl-sta.c @@ -566,6 +566,8 @@ int iwl_remove_default_wep_key(struct iwl_priv *priv, unsigned long flags; spin_lock_irqsave(&priv->sta_lock, flags); + IWL_DEBUG_WEP(priv, "Removing default WEP key: idx=%d\n", + keyconf->keyidx); if (!test_and_clear_bit(keyconf->keyidx, &priv->ucode_key_table)) IWL_ERR(priv, "index %d not used in uCode key table.\n", @@ -573,6 +575,11 @@ int iwl_remove_default_wep_key(struct iwl_priv *priv, priv->default_wep_key--; memset(&priv->wep_keys[keyconf->keyidx], 0, sizeof(priv->wep_keys[0])); + if (iwl_is_rfkill(priv)) { + IWL_DEBUG_WEP(priv, "Not sending REPLY_WEPKEY command due to RFKILL.\n"); + spin_unlock_irqrestore(&priv->sta_lock, flags); + return 0; + } ret = iwl_send_static_wepkey_cmd(priv, 1); IWL_DEBUG_WEP(priv, "Remove default WEP key: idx=%d ret=%d\n", keyconf->keyidx, ret); @@ -853,6 +860,11 @@ int iwl_remove_dynamic_key(struct iwl_priv *priv, priv->stations[sta_id].sta.sta.modify_mask = STA_MODIFY_KEY_MASK; priv->stations[sta_id].sta.mode = STA_CONTROL_MODIFY_MSK; + if (iwl_is_rfkill(priv)) { + IWL_DEBUG_WEP(priv, "Not sending REPLY_ADD_STA command because RFKILL enabled. \n"); + spin_unlock_irqrestore(&priv->sta_lock, flags); + return 0; + } ret = iwl_send_add_sta(priv, &priv->stations[sta_id].sta, CMD_ASYNC); spin_unlock_irqrestore(&priv->sta_lock, flags); return ret; -- cgit v1.2.3