[PATCH 1/2] wl1271: Fix scan failure detection

[PATCH 1/2] wl1271: Fix scan failure detection

[juuso.oikarinen]

From: Juuso Oikarinen <juuso.oikarinen@nokia.com>

In scan_complete_work, because the mutex is released before accessing the
scan->failed flag, it is possible for unfounded hardware recovery rounds
to be executed.

Fix this.

Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
---
 drivers/net/wireless/wl12xx/wl1271_main.c |   17 ++++++++++++++---
 drivers/net/wireless/wl12xx/wl1271_scan.c |    5 +++--
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index 63036b5..20027f4 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -1056,6 +1056,7 @@ static void __wl1271_op_remove_interface(struct wl1271 *wl)
 		wl->scan.state = WL1271_SCAN_STATE_IDLE;
 		kfree(wl->scan.scanned_ch);
 		wl->scan.scanned_ch = NULL;
+		wl->scan.req = NULL;
 		ieee80211_scan_completed(wl->hw, true);
 	}
 
@@ -1676,6 +1677,16 @@ static int wl1271_op_hw_scan(struct ieee80211_hw *hw,
 
 	mutex_lock(&wl->mutex);
 
+	if (wl->state == WL1271_STATE_OFF) {
+		/*
+		 * We cannot return -EBUSY here because mac80211 will expect
+		 * a call to ieee80211_scan_completed if we do - in this case
+		 * there won't be any call.
+		 */
+		ret = -EAGAIN;
+		goto out;
+	}
+
 	ret = wl1271_ps_elp_wakeup(wl, false);
 	if (ret < 0)
 		goto out;
@@ -2093,14 +2104,14 @@ static int wl1271_op_get_survey(struct ieee80211_hw *hw, int idx,
 {
 	struct wl1271 *wl = hw->priv;
 	struct ieee80211_conf *conf = &hw->conf;
- 
+
 	if (idx != 0)
 		return -ENOENT;
- 
+
 	survey->channel = conf->channel;
 	survey->filled = SURVEY_INFO_NOISE_DBM;
 	survey->noise = wl->noise;
- 
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/wl12xx/wl1271_scan.c b/drivers/net/wireless/wl12xx/wl1271_scan.c
index 909bb47..e0661a5 100644
--- a/drivers/net/wireless/wl12xx/wl1271_scan.c
+++ b/drivers/net/wireless/wl12xx/wl1271_scan.c
@@ -48,14 +48,15 @@ void wl1271_scan_complete_work(struct work_struct *work)
 	wl->scan.state = WL1271_SCAN_STATE_IDLE;
 	kfree(wl->scan.scanned_ch);
 	wl->scan.scanned_ch = NULL;
-	mutex_unlock(&wl->mutex);
-
+	wl->scan.req = NULL;
 	ieee80211_scan_completed(wl->hw, false);
 
 	if (wl->scan.failed) {
 		wl1271_info("Scan completed due to error.");
 		ieee80211_queue_work(wl->hw, &wl->recovery_work);
 	}
+	mutex_unlock(&wl->mutex);
+
 }
 
 
-- 
1.7.0.4

[PATCH 2/2] wl1271: Check interface state in op_* functions

[juuso.oikarinen]

From: Juuso Oikarinen <juuso.oikarinen@nokia.com>

Check the state of the interface on op_* function so we don't try to access
the hardware in when its off.

The mac80211 may call these in some corner cases related, for instance, to
the hardware recovery procedure. These accesses cause a kernel crash on at
least some SDIO devices, because the bus is not properly claimed in that
scenario.

Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
---
 drivers/net/wireless/wl12xx/wl1271_main.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index 20027f4..8d56ae9 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -1344,8 +1344,10 @@ static int wl1271_op_config(struct ieee80211_hw *hw, u32 changed)
 
 	mutex_lock(&wl->mutex);
 
-	if (unlikely(wl->state == WL1271_STATE_OFF))
+	if (unlikely(wl->state == WL1271_STATE_OFF)) {
+		ret = -EAGAIN;
 		goto out;
+	}
 
 	ret = wl1271_ps_elp_wakeup(wl, false);
 	if (ret < 0)
@@ -1568,6 +1570,11 @@ static int wl1271_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 
 	mutex_lock(&wl->mutex);
 
+	if (unlikely(wl->state == WL1271_STATE_OFF)) {
+		ret = -EAGAIN;
+		goto out_unlock;
+	}
+
 	ret = wl1271_ps_elp_wakeup(wl, false);
 	if (ret < 0)
 		goto out_unlock;
@@ -1708,8 +1715,10 @@ static int wl1271_op_set_rts_threshold(struct ieee80211_hw *hw, u32 value)
 
 	mutex_lock(&wl->mutex);
 
-	if (unlikely(wl->state == WL1271_STATE_OFF))
+	if (unlikely(wl->state == WL1271_STATE_OFF)) {
+		ret = -EAGAIN;
 		goto out;
+	}
 
 	ret = wl1271_ps_elp_wakeup(wl, false);
 	if (ret < 0)
@@ -1760,6 +1769,9 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
 
 	mutex_lock(&wl->mutex);
 
+	if (unlikely(wl->state == WL1271_STATE_OFF))
+		goto out;
+
 	ret = wl1271_ps_elp_wakeup(wl, false);
 	if (ret < 0)
 		goto out;
@@ -2040,6 +2052,11 @@ static int wl1271_op_conf_tx(struct ieee80211_hw *hw, u16 queue,
 
 	wl1271_debug(DEBUG_MAC80211, "mac80211 conf tx %d", queue);
 
+	if (unlikely(wl->state == WL1271_STATE_OFF)) {
+		ret = -EAGAIN;
+		goto out;
+	}
+
 	ret = wl1271_ps_elp_wakeup(wl, false);
 	if (ret < 0)
 		goto out;
@@ -2083,6 +2100,9 @@ static u64 wl1271_op_get_tsf(struct ieee80211_hw *hw)
 
 	mutex_lock(&wl->mutex);
 
+	if (unlikely(wl->state == WL1271_STATE_OFF))
+		goto out;
+
 	ret = wl1271_ps_elp_wakeup(wl, false);
 	if (ret < 0)
 		goto out;
-- 
1.7.0.4

Re: [PATCH 2/2] wl1271: Check interface state in op_* functions

[Tuomas Katila]

On Tue, 2010-10-26 at 13:24 +0200, ext juuso.oikarinen@nokia.com wrote:
> From: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> 
> Check the state of the interface on op_* function so we don't try to access
> the hardware in when its off.
> 
> The mac80211 may call these in some corner cases related, for instance, to
> the hardware recovery procedure. These accesses cause a kernel crash on at
> least some SDIO devices, because the bus is not properly claimed in that
> scenario.
> 
> Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> ---

Tested-by: Tuomas Katila <ext-tuomas.2.katila@nokia.com>

-Tuomas

Re: [PATCH 2/2] wl1271: Check interface state in op_* functions

[Luciano Coelho]

On Tue, 2010-10-26 at 13:24 +0200, juuso.oikarinen@nokia.com wrote:
> From: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> 
> Check the state of the interface on op_* function so we don't try to access
> the hardware in when its off.
> 
> The mac80211 may call these in some corner cases related, for instance, to
> the hardware recovery procedure. These accesses cause a kernel crash on at
> least some SDIO devices, because the bus is not properly claimed in that
> scenario.
> 
> Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> ---

Reviewed-by: Luciano Coelho <luciano.coelho@nokia.com>

Thanks! applied to wl12xx.

-- 
Cheers,
Luca.

Re: [PATCH 1/2] wl1271: Fix scan failure detection

[Johannes Berg]

On Tue, 2010-10-26 at 14:24 +0300, juuso.oikarinen@nokia.com wrote:

>  	mutex_lock(&wl->mutex);
>  
> +	if (wl->state == WL1271_STATE_OFF) {
> +		/*
> +		 * We cannot return -EBUSY here because mac80211 will expect
> +		 * a call to ieee80211_scan_completed if we do - in this case
> +		 * there won't be any call.
> +		 */

???

johannes

Re: [PATCH 1/2] wl1271: Fix scan failure detection

[Juuso Oikarinen]

On Tue, 2010-10-26 at 13:40 +0200, ext Johannes Berg wrote:
> On Tue, 2010-10-26 at 14:24 +0300, juuso.oikarinen@nokia.com wrote:
> 
> >  	mutex_lock(&wl->mutex);
> >  
> > +	if (wl->state == WL1271_STATE_OFF) {
> > +		/*
> > +		 * We cannot return -EBUSY here because mac80211 will expect
> > +		 * a call to ieee80211_scan_completed if we do - in this case
> > +		 * there won't be any call.
> > +		 */
> 
> ???
> 
> johannes
> 

I'll try explain what this is about. In net/wireless/sme.c you'll find
this:


                } else {
                        /* otherwise we'll need to scan for the AP first
*/
                        err = cfg80211_conn_scan(wdev);
                        /*
                         * If we can't scan right now, then we need to
scan again
                         * after the current scan finished, since the
parameters
                         * changed (unless we find a good AP anyway).
                         */
                        if (err == -EBUSY) {
                                err = 0;
                                wdev->conn->state =
CFG80211_CONN_SCAN_AGAIN;
                        }
                }

In this code, if the driver ends up returning -EBUSY, the cfg80211 ends
up waiting for a ieee80211_scan_complete call from the driver to proceed
in the connection setting.  If the scan is invoked while the driver is
not ready, returning -EBUSY in this scenario will cause the cfg80211 to
wait forever.

Obviously, cfg80211 would normally never call scan while the driver is
not initialized. This patch is about a corner case - if the wl1271
hardware crashes just before user-space requests a connection, hardware
recovery can be running while the connection is set up. To recover the
hardware, it is powered down and then mac80211 is asked to reconfigure
it.

This is the window in which the connection setup may stall. Returning an
error code other than -EBUSY for this corner case causes the error code
to be retruned to user-space, and user-space can retry the connection.

-Juuso

Re: [PATCH 1/2] wl1271: Fix scan failure detection

[Johannes Berg]

On Tue, 2010-10-26 at 15:01 +0300, Juuso Oikarinen wrote:
> On Tue, 2010-10-26 at 13:40 +0200, ext Johannes Berg wrote:

> > > +	if (wl->state == WL1271_STATE_OFF) {
> > > +		/*
> > > +		 * We cannot return -EBUSY here because mac80211 will expect
> > > +		 * a call to ieee80211_scan_completed if we do - in this case
> > > +		 * there won't be any call.
> > > +		 */
> > 
> > ???

> I'll try explain what this is about. In net/wireless/sme.c you'll find
> this:

[snip]

so it's really about cfg80211, that explains my confusion :)

johannes

Re: [PATCH 1/2] wl1271: Fix scan failure detection

[Juuso Oikarinen]

On Tue, 2010-10-26 at 14:18 +0200, ext Johannes Berg wrote:
> On Tue, 2010-10-26 at 15:01 +0300, Juuso Oikarinen wrote:
> > On Tue, 2010-10-26 at 13:40 +0200, ext Johannes Berg wrote:
> 
> > > > +	if (wl->state == WL1271_STATE_OFF) {
> > > > +		/*
> > > > +		 * We cannot return -EBUSY here because mac80211 will expect
> > > > +		 * a call to ieee80211_scan_completed if we do - in this case
> > > > +		 * there won't be any call.
> > > > +		 */
> > > 
> > > ???
> 
> > I'll try explain what this is about. In net/wireless/sme.c you'll find
> > this:
> 
> [snip]
> 
> so it's really about cfg80211, that explains my confusion :)

Yeah, it seems it's my confusion :/

-Juuso

> johannes
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] wl1271: Fix scan failure detection

[Luciano Coelho]

On Tue, 2010-10-26 at 14:22 +0200, Juuso Oikarinen wrote:
> On Tue, 2010-10-26 at 14:18 +0200, ext Johannes Berg wrote:
> > On Tue, 2010-10-26 at 15:01 +0300, Juuso Oikarinen wrote:
> > > On Tue, 2010-10-26 at 13:40 +0200, ext Johannes Berg wrote:
> > 
> > > > > +	if (wl->state == WL1271_STATE_OFF) {
> > > > > +		/*
> > > > > +		 * We cannot return -EBUSY here because mac80211 will expect
> > > > > +		 * a call to ieee80211_scan_completed if we do - in this case
> > > > > +		 * there won't be any call.
> > > > > +		 */
> > > > 
> > > > ???
> > 
> > > I'll try explain what this is about. In net/wireless/sme.c you'll find
> > > this:
> > 
> > [snip]
> > 
> > so it's really about cfg80211, that explains my confusion :)
> 
> Yeah, it seems it's my confusion :/

Thanks for the patch, Juuso.  Thanks for the comment, Johannes.

I have reviewed this patch, changed the comment to cfg80211 and applied
to the wl12xx tree.

Reviewed-by: Luciano Coelho <luciano.coelho@nokia.com>

-- 
Cheers,
Luca.

Re: [PATCH 1/2] wl1271: Fix scan failure detection

[Tuomas Katila]

On Tue, 2010-10-26 at 13:24 +0200, ext juuso.oikarinen@nokia.com wrote:
> From: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> 
> In scan_complete_work, because the mutex is released before accessing the
> scan->failed flag, it is possible for unfounded hardware recovery rounds
> to be executed.
> 
> Fix this.
> 
> Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> ---

Tested-by: Tuomas Katila <ext-tuomas.2.katila@nokia.com>

Good work. I couldn't reproduce the error with this patch.

-Tuomas