iwlwifi: rewrite iwl-scan.c to avoid race conditions

PATCH In review Stanislaw Gruszka 2 participants 19 messages Aug 31, 2010 – Sep 2, 2010 Networking drivers (wireless)

Share & tools

Stanislaw Gruszka

author

2010-08-31 15:03 UTC #0 lore raw

PATCH 1/1 iwlwifi: rewrite iwl-scan.c to avoid race conditions

In this patch I try to avoid hardware scanning race conditions that may lead to not call ieee80211_scan_completed() (what in consequences gives "WARNING: at net/wireless/core.c:614 wdev_cleanup_work+0xb7/0xf0"), or call iee80211_scan_completed() more then once (what gives " WARNING: at net/mac80211/scan.c:312 ieee80211_scan_completed+0x5f/0x1f1").

First problem (warning in wdev_cleanup_work) make any further scan request from cfg80211 are ignored by mac80211 with EBUSY error, hence NetworkManager can not perform successful scan and not allow to establish a new connection. So after suspend/resume (but maybe not only then) user is not able to connect to wireless network again.

We can not rely on that the commands (start and abort scan) are successful. Even if they are successfully send to the hardware, we can not get back notification from firmware (i.e. firmware hung or was reseted), or we can get notification when we actually perform abort scan in driver code or after that.

To assure we call ieee80211_scan_completed() only once when scan was started we use SCAN_SCANNING bit. Code path, which first clear STATUS_SCANNING bit will call ieee80211_scan_completed(). We do this in many cases, in scan complete notification, scan abort, device down, etc. Each time we check SCANNING bit.

Cancelling scan when down device is a bit tricky, because we do not want to drop priv->mutex while holding it. First we wait for hardware finish scan (STATUS_SCAN_HW bit clear), then proceed device down. After that wait for all scan workqueues finish (STATUS_SCANNING bit clear).

Scan works have now custom workqueue to allow scan functions to run in parallel with other iwlwifi works, which could wait for scan abort finish.

Signed-off-by Stanislaw Gruszka <[email protected]>

---
 drivers/net/wireless/iwlwifi/iwl-3945.h     |    2 +-
 drivers/net/wireless/iwlwifi/iwl-agn-lib.c  |   71 +----
 drivers/net/wireless/iwlwifi/iwl-agn.c      |   58 ++--
 drivers/net/wireless/iwlwifi/iwl-agn.h      |    2 +-
 drivers/net/wireless/iwlwifi/iwl-core.c     |   23 +-
 drivers/net/wireless/iwlwifi/iwl-core.h     |   19 +-
 drivers/net/wireless/iwlwifi/iwl-dev.h      |   18 +-
 drivers/net/wireless/iwlwifi/iwl-scan.c     |  510 ++++++++++++++++-----------
 drivers/net/wireless/iwlwifi/iwl-sta.c      |    8 +-
 drivers/net/wireless/iwlwifi/iwl3945-base.c |  134 ++------
 10 files changed, 406 insertions(+), 439 deletions(-)

drivers/net/wireless/iwlwifi/iwl-3945.h +1 -1

index bb2aeeb..98509c5 100644
@@ -295,7 +295,7 @@ extern const struct iwl_channel_info *iwl3945_get_channel_info(
 extern int iwl3945_rs_next_rate(struct iwl_priv *priv, int rate);
 
 /* scanning */
-void iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif);
+int iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif);
 
 /* Requires full declaration of iwl_priv before including */
 #include "iwl-io.h"

drivers/net/wireless/iwlwifi/iwl-agn-lib.c +9 ~~-62~~

108 lines not on this page · raw message

drivers/net/wireless/iwlwifi/iwl-agn.c +21 ~~-37~~

133 lines not on this page · raw message

drivers/net/wireless/iwlwifi/iwl-agn.h +1 -1

index 1a7f70f..4691563 100644
@@ -217,7 +217,7 @@ void iwl_reply_statistics(struct iwl_priv *priv,
 			  struct iwl_rx_mem_buffer *rxb);
 
 /* scan */
-void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif);
+int  iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif);
 
 /* station mgmt */
 int iwlagn_manage_ibss_station(struct iwl_priv *priv,

drivers/net/wireless/iwlwifi/iwl-core.c +8 ~~-15~~

index c43124c..8dc5d3b 100644
@@ -1726,8 +1726,8 @@ void iwl_bss_info_changed(struct ieee80211_hw *hw,
 		 * background then we need to cancel it else the RXON
 		 * below/in post_associate will fail.
 		 */
-		if (iwl_scan_cancel_timeout(priv, 100)) {
-			IWL_WARN(priv, "Aborted scan still in progress after 100ms\n");
+		if (iwl_scan_cancel_sleep(priv)) {
+			IWL_WARN(priv, "Aborted scan still in progress\n");
 			IWL_DEBUG_MAC80211(priv, "leaving - scan abort failed.\n");
 			mutex_unlock(&priv->mutex);
 			return;

@@ -1875,7 +1875,8 @@ int iwl_mac_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 
 	mutex_lock(&priv->mutex);
 
-	if (WARN_ON(!iwl_is_ready_rf(priv))) {
+	if (!iwl_is_ready_rf(priv)) {
+		IWL_WARN(priv, "Can not add vif - device not ready\n");
 		err = -EINVAL;
 		goto out;
 	}

@@ -1920,24 +1921,20 @@ void iwl_mac_remove_interface(struct ieee80211_hw *hw,
 			      struct ieee80211_vif *vif)
 {
 	struct iwl_priv *priv = hw->priv;
-	bool scan_completed = false;
 
 	IWL_DEBUG_MAC80211(priv, "enter\n");
 
 	mutex_lock(&priv->mutex);
 
+	if (priv->scan_vif == vif)
+		iwl_scan_cancel_sleep(priv);
+
 	if (iwl_is_ready_rf(priv)) {
-		iwl_scan_cancel_timeout(priv, 100);
 		priv->staging_rxon.filter_flags &= ~RXON_FILTER_ASSOC_MSK;
 		iwlcore_commit_rxon(priv);
 	}
 	if (priv->vif == vif) {
 		priv->vif = NULL;
-		if (priv->scan_vif == vif) {
-			scan_completed = true;
-			priv->scan_vif = NULL;
-			priv->scan_request = NULL;
-		}
 		memset(priv->bssid, 0, ETH_ALEN);
 	}

@@ -1953,11 +1950,7 @@ void iwl_mac_remove_interface(struct ieee80211_hw *hw,
 
 	mutex_unlock(&priv->mutex);
 
-	if (scan_completed)
-		ieee80211_scan_completed(priv->hw, true);
-
 	IWL_DEBUG_MAC80211(priv, "leave\n");
-
 }
 EXPORT_SYMBOL(iwl_mac_remove_interface);

@@ -2121,6 +2114,7 @@ void iwl_mac_reset_tsf(struct ieee80211_hw *hw)
 
 	spin_unlock_irqrestore(&priv->lock, flags);
 
+	iwl_scan_cancel_sleep(priv);
 	if (!iwl_is_ready_rf(priv)) {
 		IWL_DEBUG_MAC80211(priv, "leave - not ready\n");
 		mutex_unlock(&priv->mutex);

@@ -2130,7 +2124,6 @@ void iwl_mac_reset_tsf(struct ieee80211_hw *hw)
 	/* we are restarting association process
 	 * clear RXON_FILTER_ASSOC_MSK bit
 	 */
-	iwl_scan_cancel_timeout(priv, 100);
 	priv->staging_rxon.filter_flags &= ~RXON_FILTER_ASSOC_MSK;
 	iwlcore_commit_rxon(priv);

drivers/net/wireless/iwlwifi/iwl-core.h +13 -6

index de2e39f..e9969df 100644
@@ -109,7 +109,7 @@ struct iwl_hcmd_utils_ops {
 				  __le16 fc, __le32 *tx_flags);
 	int  (*calc_rssi)(struct iwl_priv *priv,
 			  struct iwl_rx_phy_res *rx_resp);
-	void (*request_scan)(struct iwl_priv *priv, struct ieee80211_vif *vif);
+	int (*request_scan)(struct iwl_priv *priv, struct ieee80211_vif *vif);
 };
 
 struct iwl_apm_ops {

@@ -544,8 +544,9 @@ static inline __le32 iwl_hw_set_rate_n_flags(u8 rate, u32 flags)
  * Scanning
  ******************************************************************************/
 void iwl_init_scan_params(struct iwl_priv *priv);
-int iwl_scan_cancel(struct iwl_priv *priv);
-int iwl_scan_cancel_timeout(struct iwl_priv *priv, unsigned long ms);
+void iwl_scan_cancel(struct iwl_priv *priv);
+int iwl_scan_cancel_sleep(struct iwl_priv *priv);
+void iwl_wait_for_scan_end(struct iwl_priv *priv);
 int iwl_mac_hw_scan(struct ieee80211_hw *hw,
 		    struct ieee80211_vif *vif,
 		    struct cfg80211_scan_request *req);

@@ -560,7 +561,8 @@ u16 iwl_get_active_dwell_time(struct iwl_priv *priv,
 u16 iwl_get_passive_dwell_time(struct iwl_priv *priv,
 			       enum ieee80211_band band,
 			       struct ieee80211_vif *vif);
-void iwl_setup_scan_deferred_work(struct iwl_priv *priv);
+void iwl_setup_scan_deferred_work(struct iwl_priv *priv, const char *name);
+void iwl_cancel_scan_deferred_work(struct iwl_priv *priv);
 
 /* For faster active scanning, scan will move to the next channel if fewer than
  * PLCP_QUIET_THRESH packets are heard on this channel within

@@ -571,8 +573,6 @@ void iwl_setup_scan_deferred_work(struct iwl_priv *priv);
 #define IWL_ACTIVE_QUIET_TIME       cpu_to_le16(10)  /* msec */
 #define IWL_PLCP_QUIET_THRESH       cpu_to_le16(1)  /* packets */
 
-#define IWL_SCAN_CHECK_WATCHDOG		(HZ * 7)
-
 /*******************************************************************************
  * Calibrations - implemented in iwl-calib.c
  ******************************************************************************/

@@ -667,6 +667,13 @@ void iwlcore_free_geos(struct iwl_priv *priv);
 #define STATUS_POWER_PMI	16
 #define STATUS_FW_ERROR		17
 
+#define IWL_PRESERVE_STATUS_MASK \
+		(BIT(STATUS_RF_KILL_HW) | \
+		 BIT(STATUS_GEO_CONFIGURED) | \
+		 BIT(STATUS_FW_ERROR) | \
+		 BIT(STATUS_EXIT_PENDING) | \
+		 BIT(STATUS_SCANNING) | \
+		 BIT(STATUS_SCAN_ABORTING))
 
 static inline int iwl_is_ready(struct iwl_priv *priv)
 {

drivers/net/wireless/iwlwifi/iwl-dev.h +14 -4

index 8d5201a..d20d223 100644
@@ -1392,23 +1392,33 @@ struct iwl_priv {
 	struct workqueue_struct *workqueue;
 
 	struct work_struct restart;
-	struct work_struct scan_completed;
 	struct work_struct rx_replenish;
-	struct work_struct abort_scan;
 	struct work_struct beacon_update;
 	struct work_struct tt_work;
 	struct work_struct ct_enter;
 	struct work_struct ct_exit;
-	struct work_struct start_internal_scan;
 	struct work_struct tx_flush;
 	struct work_struct bt_full_concurrency;
 	struct work_struct bt_runtime_config;
 
+	/* Use separate workqueue for scanning, to allow scan work
+	 * functions run in parallel with other work functions, which
+	 * can call iwl_scan_cancel_sleep() (particularly restart).
+	 * Otherwise scan will not be able to finish and cleanup,
+	 * because we can not run two works in the same workqueue
+	 * at the same time. */
+	struct workqueue_struct *scan_workqueue;
+
+	struct work_struct start_internal_scan;
+	struct work_struct scan_completed;
+	struct work_struct abort_scan;
+	struct delayed_work scan_timeout;
+	struct delayed_work abort_timeout;
+
 	struct tasklet_struct irq_tasklet;
 
 	struct delayed_work init_alive_start;
 	struct delayed_work alive_start;
-	struct delayed_work scan_check;
 
 	/* TX Power */
 	s8 tx_power_user_lmt;

drivers/net/wireless/iwlwifi/iwl-scan.c +306 ~~-204~~

637 lines not on this page · raw message

drivers/net/wireless/iwlwifi/iwl-sta.c +5 -3

index d5e8db3..c7c39ae 100644
@@ -989,11 +989,13 @@ void iwl_update_tkip_key(struct iwl_priv *priv,
 	unsigned long flags;
 	int i;
 
-	if (iwl_scan_cancel(priv)) {
-		/* cancel scan failed, just live w/ bad key and rely
-		   briefly on SW decryption */
+	if (test_bit(STATUS_SCAN_HW, &priv->status)) {
+		/* just live w/ bad key and rely briefly on SW decryption */
 		return;
 	}
+	/* XXX: possible race condition: nothing prevents to start HW scanning
+	 *      right now, however it is not clear if we really can not send
+	 *      update key command to firmware while performing scanning */
 
 	sta_id = iwl_sta_id_or_broadcast(priv, sta);
 	if (sta_id == IWL_INVALID_STATION)

drivers/net/wireless/iwlwifi/iwl3945-base.c +28 ~~-106~~

246 lines not on this page · raw message

Signature · 1 line

1.7.1

Johannes Berg

maintainer

2010-09-01 10:59 UTC #1 lore raw

Quoted 3 lines · Stanislaw Gruszka

Scan works have now custom workqueue to allow scan functions to run in parallel with other iwlwifi works, which could wait for scan abort finish.

Do we really need a complete workqueue? It seems like the relevant work structs that really do need to be waited for from the workqueue can be queued with schedule_work? Shouldn't that only be a few of them anyway?

johannes

Stanislaw Gruszka

author

2010-09-01 12:01 UTC #2 lore raw

Quoted 9 lines · Johannes Berg

Quoted 3 lines · Stanislaw Gruszka
Scan works have now custom workqueue to allow scan functions to run in parallel with other iwlwifi works, which could wait for scan abort finish.
Do we really need a complete workqueue? It seems like the relevant work structs that really do need to be waited for from the workqueue can be queued with schedule_work?

Custom workqueue is not strictly needed, but it assure all works will run short after schedule. Common workqueue can not give us such guarantees, as other driver/subsystem can schedule own work, possibly slow, which can block start of our work for long time.

Quoted 1 line

Shouldn't that only be a few of them anyway?

For sure abort_scan and abort_timeout works have to be scheduled on something other than priv->workqueue. I'm queuing all scan works on priv->scan_workqueue for consistency.

Stanislaw

Johannes Berg

maintainer

2010-09-01 12:42 UTC #3 lore raw

Quoted 4 lines · Stanislaw Gruszka

Custom workqueue is not strictly needed, but it assure all works will run short after schedule. Common workqueue can not give us such guarantees, as other driver/subsystem can schedule own work, possibly slow, which can block start of our work for long time.

I believe that's no longer true, with Tejun's workqueue rewrite that just got into mainline.

Quoted 5 lines

Quoted 1 line
Shouldn't that only be a few of them anyway?
For sure abort_scan and abort_timeout works have to be scheduled on something other than priv->workqueue. I'm queuing all scan works on priv->scan_workqueue for consistency.

Remind me: The reason is that we need to cancel them from within the workqueue? But if we're on the same workqueue, it seems like they couldn't be running already, so cancel_work_sync() would always cancel them? Do we get lockdep errors for that? Or are there actual locks involved?

johannes

Stanislaw Gruszka

author

2010-09-01 14:20 UTC #4 lore raw

Quoted 7 lines · Johannes Berg

Quoted 4 lines
Custom workqueue is not strictly needed, but it assure all works will run short after schedule. Common workqueue can not give us such guarantees, as other driver/subsystem can schedule own work, possibly slow, which can block start of our work for long time.
I believe that's no longer true, with Tejun's workqueue rewrite that just got into mainline.

Good. However I still think having separate workqueue for scanning is clean and consistent solution, I prefer it over schedule_work() ... and want that patch backport to RHEL6 2.6.32 where we do not have such goodies :-)

Quoted 10 lines

Quoted 5 lines
Quoted 1 line
Shouldn't that only be a few of them anyway?
For sure abort_scan and abort_timeout works have to be scheduled on something other than priv->workqueue. I'm queuing all scan works on priv->scan_workqueue for consistency.
Remind me: The reason is that we need to cancel them from within the workqueue? But if we're on the same workqueue, it seems like they couldn't be running already, so cancel_work_sync() would always cancel them?

Problem is not canceling, but exactly that we can not run new work when old one does not finish. For example, if queued to priv->workqueue abort_timeout will not be able run when we are performing iwl_bg_restart. Will run after iwl_bg_restart finish, we don't want that.

Quoted 2 lines

Do we get lockdep errors for that? Or are there actual locks involved?

No, priv->mutex used to protect critical sections of functions, but it's not keep all the time in works (i.e: iwl_bg_restart), so give scan code chance to complete.

Stanislaw

Johannes Berg

maintainer

2010-09-01 14:26 UTC #5 lore raw

Quoted 6 lines · Stanislaw Gruszka

Quoted 5 lines · Johannes Berg
Quoted 4 lines
Custom workqueue is not strictly needed, but it assure all works will run short after schedule. Common workqueue can not give us such guarantees, as other driver/subsystem can schedule own work, possibly slow, which can block start of our work for long time.

Quoted 6 lines

Quoted 2 lines
I believe that's no longer true, with Tejun's workqueue rewrite that just got into mainline.
Good. However I still think having separate workqueue for scanning is clean and consistent solution, I prefer it over schedule_work() ... and want that patch backport to RHEL6 2.6.32 where we do not have such goodies :-)

Ah, but Tejun will kill us if we add more workqueues to the current kernel :-)

Quoted 9 lines

Quoted 8 lines
Quoted 3 lines
For sure abort_scan and abort_timeout works have to be scheduled on something other than priv->workqueue. I'm queuing all scan works on priv->scan_workqueue for consistency.
Remind me: The reason is that we need to cancel them from within the workqueue? But if we're on the same workqueue, it seems like they couldn't be running already, so cancel_work_sync() would always cancel them?

Quoted 4 lines

Problem is not canceling, but exactly that we can not run new work when old one does not finish. For example, if queued to priv->workqueue abort_timeout will not be able run when we are performing iwl_bg_restart. Will run after iwl_bg_restart finish, we don't want that.

Ok ... Not sure I understand. Why do we care about abort_timeout work coming after it? We'd cancel it anyway, when we kill the scan from bg_restart, no?

johannes

Stanislaw Gruszka

author

2010-09-01 15:27 UTC #6 lore raw

Quoted 2 lines · Johannes Berg

Ah, but Tejun will kill us if we add more workqueues to the current kernel :-)

Ok, I wanna live ...

Quoted 9 lines

Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
For sure abort_scan and abort_timeout works have to be scheduled on something other than priv->workqueue. I'm queuing all scan works on priv->scan_workqueue for consistency.
Remind me: The reason is that we need to cancel them from within the workqueue? But if we're on the same workqueue, it seems like they couldn't be running already, so cancel_work_sync() would always cancel them?

Quoted 8 lines

Quoted 4 lines
Problem is not canceling, but exactly that we can not run new work when old one does not finish. For example, if queued to priv->workqueue abort_timeout will not be able run when we are performing iwl_bg_restart. Will run after iwl_bg_restart finish, we don't want that.
Ok ... Not sure I understand. Why do we care about abort_timeout work coming after it? We'd cancel it anyway, when we kill the scan from bg_restart, no?

I wanted abort_timeout finish to avoid warning I putted in iwl_scan_cancel_sleep, but I could just remove the warning ...

Cancel at the end of iwl_cancel_scan_deferred_work() is for case when we still get notification from f/w and clear STATUS_SCAN_HW bit in iwl_rx_scan_complete_notif. This can happen when firmware is in some sane state but we do restart (for example: when debugfs force_reset file was used).

For the same reason we need have to iwl_bg_scan_completed allow to run in parallel with iwl_bg_restart. To complete scanning when performing restart, but f/w is responsible.

Stanislaw

Johannes Berg

maintainer

2010-09-01 16:04 UTC #7 lore raw

Quoted 6 lines · Stanislaw Gruszka

Quoted 3 lines
Ok ... Not sure I understand. Why do we care about abort_timeout work coming after it? We'd cancel it anyway, when we kill the scan from bg_restart, no?
I wanted abort_timeout finish to avoid warning I putted in iwl_scan_cancel_sleep, but I could just remove the warning ...

Ok.

Quoted 9 lines

Cancel at the end of iwl_cancel_scan_deferred_work() is for case when we still get notification from f/w and clear STATUS_SCAN_HW bit in iwl_rx_scan_complete_notif. This can happen when firmware is in some sane state but we do restart (for example: when debugfs force_reset file was used).
For the same reason we need have to iwl_bg_scan_completed allow to run in parallel with iwl_bg_restart. To complete scanning when performing restart, but f/w is responsible.

I think you mean "responsive"? Otherwise what you're saying doesn't make much sense to me?

But they all take the mutex, so they can't run in parallel anyway. So it only becomes an ordering issue, no?

johannes

Stanislaw Gruszka

author

2010-09-02 08:50 UTC #8 lore raw

Quoted 6 lines · Johannes Berg

Quoted 6 lines
Cancel at the end of iwl_cancel_scan_deferred_work() is for case when we still get notification from f/w and clear STATUS_SCAN_HW bit in iwl_rx_scan_complete_notif. This can happen when firmware is in some sane state but we do restart (for example: when debugfs force_reset file was used).

Quoted 6 lines

Quoted 3 lines
For the same reason we need have to iwl_bg_scan_completed allow to run in parallel with iwl_bg_restart. To complete scanning when performing restart, but f/w is responsible.
I think you mean "responsive"? Otherwise what you're saying doesn't make much sense to me?

Ehh, yes responsive.

Quoted 2 lines

But they all take the mutex, so they can't run in parallel anyway. So it only becomes an ordering issue, no?

Mutex is not taken all the time in iwl_bg_restart.

Stanislaw

Johannes Berg

maintainer

2010-09-02 09:21 UTC #9 lore raw

Quoted 4 lines · Stanislaw Gruszka

Quoted 2 lines
But they all take the mutex, so they can't run in parallel anyway. So it only becomes an ordering issue, no?
Mutex is not taken all the time in iwl_bg_restart.

Oh, true, but still. In a restart, why do we need to worry about having aborted scan_completed? We abort the scan at that point, so if later scan_completed actually runs we don't really care all that much, since it won't do anything, no?

johannes

Stanislaw Gruszka

author

2010-09-02 10:55 UTC #10 lore raw

Quoted 11 lines · Johannes Berg

Quoted 4 lines · Stanislaw Gruszka
Quoted 2 lines
But they all take the mutex, so they can't run in parallel anyway. So it only becomes an ordering issue, no?
Mutex is not taken all the time in iwl_bg_restart.
Oh, true, but still. In a restart, why do we need to worry about having aborted scan_completed? We abort the scan at that point, so if later scan_completed actually runs we don't really care all that much, since it won't do anything, no?

In patch in current form this is needed to avoid warnings in iwl_wait_for_scan_end() and iwl_cancel_scan_deferred_work(). But I think you have right, patch can be rewritten to not have parallel scan works requirement.

Stanislaw

Johannes Berg

maintainer

2010-09-01 12:52 UTC #11 lore raw

In reply to Stanislaw Gruszka

Some comments on the patch:

drivers/net/wireless/iwlwifi/iwl-scan.c line 76

Quoted 9 lines

static int iwl_send_scan_abort(struct iwl_priv *priv) {
drivers/net/wireless/iwlwifi/iwl-scan.c +1 -1
-	int ret = 0;
+	int ret;
 	struct iwl_rx_packet *pkt;
 	struct iwl_host_cmd cmd = {
 		.id = REPLY_SCAN_ABORT_CMD,
 		.flags = CMD_WANT_SKB,
 	};

Since you're going through, and probably know where what lock is needed, could you annotate the places with, e.g.

	lockdep_assert_held(&priv->mutex)?

If you're not sure, that's fine, but if you know already that'd probably make things easier in the future.

Quoted 1 line

+static inline void iwl_complete_scan(struct iwl_priv *priv, bool aborted)

These "inline" annotations seem wrong, either the function is used once, then gcc will inline it, or it is used multiple times, then we shouldn't inline it.

drivers/net/wireless/iwlwifi/iwl-scan.c EXPORT_SYMBOL(iwl_mac_hw_scan);

Quoted 3 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +2 -0

+static void iwl_do_scan_abort(struct iwl_priv *priv)
 
+	lockdep_assert_held(&priv->mutex);

:-)

Quoted 9 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +9 -0

+/**
+ * iwl_scan_cancel_sleep - Cancel any currently executing HW scan,
+ *			   wait for hardware/firmware! completion
+ */
+int iwl_scan_cancel_sleep(struct iwl_priv *priv)
+{
+	int ret;
+	unsigned long timeout = jiffies + IWL_SCAN_ABORT_SLEEP;
+

drivers/net/wireless/iwlwifi/iwl-scan.c EXPORT_SYMBOL(iwl_mac_hw_scan); · line 479

Quoted 10 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +10 -0

+	IWL_DEBUG_SCAN(priv, "Scan cancel wait\n");
+
+	cancel_delayed_work(&priv->scan_timeout);
+	iwl_do_scan_abort(priv);
+
+	while (time_before_eq(jiffies, timeout)) {
+		if (!test_bit(STATUS_SCAN_HW, &priv->status))
+			break;
+		msleep(20);
+	}

This, and

drivers/net/wireless/iwlwifi/iwl-scan.c EXPORT_SYMBOL(iwl_mac_hw_scan); · line 495

Quoted 9 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +9 -0

+void iwl_wait_for_scan_end(struct iwl_priv *priv)
+{
+	unsigned long timeout = jiffies + IWL_SCAN_WAIT_END;
+
+	while (time_before_eq(jiffies, timeout)) {
+		if (!test_bit(STATUS_SCANNING, &priv->status))
+			break;
+		msleep(20);
+	}

this seems like it could use a completion?

johannes

Stanislaw Gruszka

author

2010-09-01 14:20 UTC #12 lore raw

Quoted 13 lines · Johannes Berg

drivers/net/wireless/iwlwifi/iwl-scan.c line 76
Quoted 9 lines
static int iwl_send_scan_abort(struct iwl_priv *priv) {
drivers/net/wireless/iwlwifi/iwl-scan.c +1 -1
-	int ret = 0;
+	int ret;
 	struct iwl_rx_packet *pkt;
 	struct iwl_host_cmd cmd = {
 		.id = REPLY_SCAN_ABORT_CMD,
 		.flags = CMD_WANT_SKB,
 	};
Since you're going through, and probably know where what lock is needed, could you annotate the places with, e.g.

Quoted 1 line

	lockdep_assert_held(&priv->mutex)?

Quoted 3 lines

These "inline" annotations seem wrong, either the function is used once, then gcc will inline it, or it is used multiple times, then we shouldn't inline it.

Quoted 18 lines

Quoted 5 lines
drivers/net/wireless/iwlwifi/iwl-scan.c +5 -0
+int iwl_scan_cancel_sleep(struct iwl_priv *priv)
+{
+	int ret;
+	unsigned long timeout = jiffies + IWL_SCAN_ABORT_SLEEP;
+
drivers/net/wireless/iwlwifi/iwl-scan.c EXPORT_SYMBOL(iwl_mac_hw_scan); · line 479
Quoted 10 lines
drivers/net/wireless/iwlwifi/iwl-scan.c +10 -0
+	IWL_DEBUG_SCAN(priv, "Scan cancel wait\n");
+
+	cancel_delayed_work(&priv->scan_timeout);
+	iwl_do_scan_abort(priv);
+
+	while (time_before_eq(jiffies, timeout)) {
+		if (!test_bit(STATUS_SCAN_HW, &priv->status))
+			break;
+		msleep(20);
+	}
This, and

Quoted 11 lines

drivers/net/wireless/iwlwifi/iwl-scan.c EXPORT_SYMBOL(iwl_mac_hw_scan); · line 495
Quoted 9 lines
drivers/net/wireless/iwlwifi/iwl-scan.c +9 -0
+void iwl_wait_for_scan_end(struct iwl_priv *priv)
+{
+	unsigned long timeout = jiffies + IWL_SCAN_WAIT_END;
+
+	while (time_before_eq(jiffies, timeout)) {
+		if (!test_bit(STATUS_SCANNING, &priv->status))
+			break;
+		msleep(20);
+	}
this seems like it could use a completion?

We don't know is scanning is currently performed or not, so we nobody could ever call complete(). Above code works fine if no scan is running.

Stanislaw

Johannes Berg

maintainer

2010-09-01 14:25 UTC #13 lore raw

Quoted 12 lines · Stanislaw Gruszka

Quoted 11 lines
drivers/net/wireless/iwlwifi/iwl-scan.c EXPORT_SYMBOL(iwl_mac_hw_scan); · line 495
Quoted 9 lines
drivers/net/wireless/iwlwifi/iwl-scan.c +9 -0
+void iwl_wait_for_scan_end(struct iwl_priv *priv)
+{
+	unsigned long timeout = jiffies + IWL_SCAN_WAIT_END;
+
+	while (time_before_eq(jiffies, timeout)) {
+		if (!test_bit(STATUS_SCANNING, &priv->status))
+			break;
+		msleep(20);
+	}
this seems like it could use a completion?

Quoted 3 lines

We don't know is scanning is currently performed or not, so we nobody could ever call complete(). Above code works fine if no scan is running.

Then I guess it should be a waitqueue?

johannes

Johannes Berg

maintainer

2010-09-02 09:36 UTC #14 lore raw

In reply to Stanislaw Gruszka

Review

Quoted 1 line · Stanislaw Gruszka

(quoted diff) +1 -0

+	priv->status &= IWL_PRESERVE_STATUS_MASK;

Is that completely save here? No other functions can interfere?

I just ported your patch to the current tree:
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/NNN-iwl-scan.patch

but I'm going to split it up into multiple now.

johannes

Stanislaw Gruszka

author

2010-09-02 11:10 UTC #15 lore raw

Quoted 5 lines · Johannes Berg

Review
Quoted 1 line · Stanislaw Gruszka
(quoted diff) +1 -0
+	priv->status &= IWL_PRESERVE_STATUS_MASK;
Is that completely save here? No other functions can interfere?

I don't know, I think is not safe, but seems that is not worse compeered to what we have now.

Quoted 4 lines

I just ported your patch to the current tree:
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/NNN-iwl-scan.patch
but I'm going to split it up into multiple now.

Ok, if you want to. I could to this as well, actually I should doing that way from the beginning.

Stanislaw

Johannes Berg

maintainer

2010-09-02 11:37 UTC #16 lore raw

Quoted 2 lines · Stanislaw Gruszka

Ok, if you want to. I could to this as well, actually I should doing that way from the beginning.

I got started:
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/011-iwl-remove-unused-conf.patch
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/012-iwl-unify-scan-checks.patch
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/013-iwl-fix-scan-queued-bug.patch
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/014-iwl-move-scan-completed.patch

but haven't really started addressing the actual problem. Want to take it from here again? (oh and I also haven't tested these patches)

johannes

Stanislaw Gruszka

author

2010-09-02 11:46 UTC #17 lore raw

Quoted 5 lines · Johannes Berg

Quoted 2 lines · Stanislaw Gruszka
Ok, if you want to. I could to this as well, actually I should doing that way from the beginning.

Quoted 8 lines

I got started:
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/011-iwl-remove-unused-conf.patch
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/012-iwl-unify-scan-checks.patch
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/013-iwl-fix-scan-queued-bug.patch
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/014-iwl-move-scan-completed.patch
but haven't really started addressing the actual problem. Want to take it from here again? (oh and I also haven't tested these patches)

Yes, I will continue work of splitting patch, fix remaining issues, test and post.

Stanislaw

Johannes Berg

maintainer

2010-09-02 10:17 UTC #18 lore raw

In reply to Stanislaw Gruszka

Stanislaw,

What do you think about the two attached (untested) patches? I think your patch was doing too many things at a time, is this a good start?

johannes

Subject: iwlwifi: remove unused conf variables

From: Johannes Berg <[email protected]>

There are a number of conf variables that are
unused, remove them.

Signed-off-by Johannes Berg <[email protected]>

---
 drivers/net/wireless/iwlwifi/iwl-agn-lib.c  |    3 ---
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    6 ------
 2 files changed, 9 deletions(-)

drivers/net/wireless/iwlwifi/iwl-agn-lib.c +0 -3

@@ -1162,7 +1162,6 @@ void iwlagn_request_scan(struct iwl_priv
 		.flags = CMD_SIZE_HUGE,
 	};
 	struct iwl_scan_cmd *scan;
-	struct ieee80211_conf *conf = NULL;
 	struct iwl_rxon_context *ctx = &priv->contexts[IWL_RXON_CTX_BSS];
 	u32 rate_flags = 0;
 	u16 cmd_len;

@@ -1179,8 +1178,6 @@ void iwlagn_request_scan(struct iwl_priv
 	if (vif)
 		ctx = iwl_rxon_ctx_from_vif(vif);
 
-	conf = ieee80211_get_hw_conf(priv->hw);
-
 	cancel_delayed_work(&priv->scan_check);
 
 	if (!iwl_is_ready(priv)) {

drivers/net/wireless/iwlwifi/iwl3945-base.c +0 -6

@@ -2569,12 +2569,9 @@ static void __iwl3945_down(struct iwl_pr
 {
 	unsigned long flags;
 	int exit_pending = test_bit(STATUS_EXIT_PENDING, &priv->status);
-	struct ieee80211_conf *conf = NULL;
 
 	IWL_DEBUG_INFO(priv, DRV_NAME " is going down\n");
 
-	conf = ieee80211_get_hw_conf(priv->hw);
-
 	if (!exit_pending)
 		set_bit(STATUS_EXIT_PENDING, &priv->status);

@@ -2828,13 +2825,10 @@ void iwl3945_request_scan(struct iwl_pri
 		.flags = CMD_SIZE_HUGE,
 	};
 	struct iwl3945_scan_cmd *scan;
-	struct ieee80211_conf *conf = NULL;
 	u8 n_probes = 0;
 	enum ieee80211_band band;
 	bool is_active = false;
 
-	conf = ieee80211_get_hw_conf(priv->hw);
-
 	cancel_delayed_work(&priv->scan_check);
 
 	if (!iwl_is_ready(priv)) {

Subject: iwlwifi: unify scan start checks

From: Johannes Berg <[email protected]>

Rather than duplicating all the checks and even
in case of errors accepting the scan request
from mac80211, we can push the checks to the
caller and in all error cases reject the scan
request right away (rather than accepting and
then saying it was aborted).

Signed-off-by Johannes Berg <[email protected]>

---
 drivers/net/wireless/iwlwifi/iwl-3945.h     |    2 
 drivers/net/wireless/iwlwifi/iwl-agn-lib.c  |   89 ++++----------------
 drivers/net/wireless/iwlwifi/iwl-agn.h      |    2 
 drivers/net/wireless/iwlwifi/iwl-core.h     |    2 
 drivers/net/wireless/iwlwifi/iwl-scan.c     |  124 ++++++++++++++++------------
 drivers/net/wireless/iwlwifi/iwl3945-base.c |   74 ++--------------
 6 files changed, 107 insertions(+), 186 deletions(-)

drivers/net/wireless/iwlwifi/iwl-3945.h +1 -1

@@ -295,7 +295,7 @@ extern const struct iwl_channel_info *iw
 extern int iwl3945_rs_next_rate(struct iwl_priv *priv, int rate);
 
 /* scanning */
-void iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif);
+int iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif);
 
 /* Requires full declaration of iwl_priv before including */
 #include "iwl-io.h"

drivers/net/wireless/iwlwifi/iwl-agn-lib.c +22 ~~-67~~

129 lines not on this page · raw message

drivers/net/wireless/iwlwifi/iwl-agn.h +1 -1

@@ -217,7 +217,7 @@ void iwl_reply_statistics(struct iwl_pri
 			  struct iwl_rx_mem_buffer *rxb);
 
 /* scan */
-void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif);
+int iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif);
 
 /* station mgmt */
 int iwlagn_manage_ibss_station(struct iwl_priv *priv,

drivers/net/wireless/iwlwifi/iwl-core.h +1 -1

@@ -111,7 +111,7 @@ struct iwl_hcmd_utils_ops {
 				  __le16 fc, __le32 *tx_flags);
 	int  (*calc_rssi)(struct iwl_priv *priv,
 			  struct iwl_rx_phy_res *rx_resp);
-	void (*request_scan)(struct iwl_priv *priv, struct ieee80211_vif *vif);
+	int (*request_scan)(struct iwl_priv *priv, struct ieee80211_vif *vif);
 };
 
 struct iwl_apm_ops {

drivers/net/wireless/iwlwifi/iwl-scan.c +72 ~~-52~~

192 lines not on this page · raw message

drivers/net/wireless/iwlwifi/iwl3945-base.c +10 ~~-64~~

111 lines not on this page · raw message