libertas: make association work again

PATCH In review Dan Williams 4 participants 15 messages Jul 30, 2010 – Aug 5, 2010 Networking drivers (wireless)

Share & tools

Dan Williams

author

2010-07-30 04:05 UTC #0 lore raw

cover libertas: make association work again

This series makes libertas actually associate with APs after the cfg80211 conversion. There was a pretty big TODO where if cfg80211 didn't pass in the BSSID of the AP to associate with, then libertas would fail because the firmware really, really, really wants a BSSID. So lets do a scan to find that BSSID. The other notable change is a fix to make sure all supported rates are included in the firmware's association request, lack of which caused some APs to deny the attempt.

Dan Williams (5):
libertas: get the right # of scanned BSSes
libertas: better scan response debugging
libertas: better association request debugging
libertas: fix association with some APs by using extended rates
libertas: scan before assocation if no BSSID was given

 drivers/net/wireless/libertas/cfg.c  |  275 ++++++++++++++++++++++++++--------
 drivers/net/wireless/libertas/dev.h  |    5 +
 drivers/net/wireless/libertas/main.c |    1 +
 3 files changed, 219 insertions(+), 62 deletions(-)

Dan Williams

author

2010-07-30 04:10 UTC #1 lore raw

PATCH 1/5 libertas: get the right # of scanned BSSes

Let's actually check the right field in the command response; and if there aren't any reported BSSes, exit early with success.

Signed-off-by Dan Williams <[email protected]>

---
 drivers/net/wireless/libertas/cfg.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

drivers/net/wireless/libertas/cfg.c +9 -1

index 25f9027..b60f661 100644
@@ -465,7 +465,15 @@ static int lbs_ret_scan(struct lbs_private *priv, unsigned long dummy,
 	lbs_deb_enter(LBS_DEB_CFG80211);
 
 	bsssize = get_unaligned_le16(&scanresp->bssdescriptsize);
-	nr_sets = le16_to_cpu(resp->size);
+	nr_sets = le16_to_cpu(scanresp->nr_sets);
+
+	lbs_deb_scan("scan response: %d BSSs (%d bytes); resp size %d bytes\n",
+			nr_sets, bsssize, le16_to_cpu(resp->size));
+
+	if (nr_sets == 0) {
+		ret = 0;
+		goto done;
+	}
 
 	/*
 	 * The general layout of the scan response is described in chapter

Signature · 1 line

1.7.2

Dan Williams

author

2010-07-30 04:11 UTC #2 lore raw

PATCH 2/5 libertas: better scan response debugging

In reply to Dan Williams

Make it a bit easier to debug scan results in the future.

Signed-off-by Dan Williams <[email protected]>

---
 drivers/net/wireless/libertas/cfg.c |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

drivers/net/wireless/libertas/cfg.c +20 -4

index b60f661..c7da6c0 100644
@@ -502,20 +502,31 @@ static int lbs_ret_scan(struct lbs_private *priv, unsigned long dummy,
 
 	pos = scanresp->bssdesc_and_tlvbuffer;
 
+	lbs_deb_hex(LBS_DEB_SCAN, "SCAN_RSP", scanresp->bssdesc_and_tlvbuffer,
+			scanresp->bssdescriptsize);
+
 	tsfdesc = pos + bsssize;
 	tsfsize = 4 + 8 * scanresp->nr_sets;
+	lbs_deb_hex(LBS_DEB_SCAN, "SCAN_TSF", (u8 *) tsfdesc, tsfsize);
 
 	/* Validity check: we expect a Marvell-Local TLV */
 	i = get_unaligned_le16(tsfdesc);
 	tsfdesc += 2;
-	if (i != TLV_TYPE_TSFTIMESTAMP)
+	if (i != TLV_TYPE_TSFTIMESTAMP) {
+		lbs_deb_scan("scan response: invalid TSF Timestamp %d\n", i);
 		goto done;
+	}
+
 	/* Validity check: the TLV holds TSF values with 8 bytes each, so
 	 * the size in the TLV must match the nr_sets value */
 	i = get_unaligned_le16(tsfdesc);
 	tsfdesc += 2;
-	if (i / 8 != scanresp->nr_sets)
+	if (i / 8 != scanresp->nr_sets) {
+		lbs_deb_scan("scan response: invalid number of TSF timestamp "
+			     "sets (expected %d got %d)\n", scanresp->nr_sets,
+			     i / 8);
 		goto done;
+	}
 
 	for (i = 0; i < scanresp->nr_sets; i++) {
 		const u8 *bssid;

@@ -557,8 +568,11 @@ static int lbs_ret_scan(struct lbs_private *priv, unsigned long dummy,
 			id = *pos++;
 			elen = *pos++;
 			left -= 2;
-			if (elen > left || elen == 0)
+			if (elen > left || elen == 0) {
+				lbs_deb_scan("scan response: invalid IE fmt\n");
 				goto done;
+			}
+
 			if (id == WLAN_EID_DS_PARAMS)
 				chan_no = *pos;
 			if (id == WLAN_EID_SSID) {

@@ -589,7 +603,9 @@ static int lbs_ret_scan(struct lbs_private *priv, unsigned long dummy,
 					capa, intvl, ie, ielen,
 					LBS_SCAN_RSSI_TO_MBM(rssi),
 					GFP_KERNEL);
-		}
+		} else
+			lbs_deb_scan("scan response: missing BSS channel IE\n");
+
 		tsfdesc += 8;
 	}
 	ret = 0;

Signature · 1 line

1.7.2

Dan Williams

author

2010-07-30 04:13 UTC #3 lore raw

PATCH 3/5 libertas: better association request debugging

In reply to Dan Williams

Bring back the comment about FW v5 status codes from the pre-cfg80211 driver, and let through status codes that aren't remapped by the firmware.

Signed-off-by Dan Williams <[email protected]>

---
 drivers/net/wireless/libertas/cfg.c |   37 +++++++++++++++++++++++++++-------
 1 files changed, 29 insertions(+), 8 deletions(-)

drivers/net/wireless/libertas/cfg.c +29 -8

index c7da6c0..a9647e6 100644
@@ -1071,7 +1071,7 @@ static int lbs_associate(struct lbs_private *priv,
 	pos += lbs_add_common_rates_tlv(pos, bss);
 
 	/* add auth type TLV */
-	if (priv->fwrelease >= 0x09000000)
+	if (MRVL_FW_MAJOR_REV(priv->fwrelease) >= 9)
 		pos += lbs_add_auth_type_tlv(pos, sme->auth_type);
 
 	/* add WPA/WPA2 TLV */

@@ -1082,6 +1082,9 @@ static int lbs_associate(struct lbs_private *priv,
 		(u16)(pos - (u8 *) &cmd->iebuf);
 	cmd->hdr.size = cpu_to_le16(len);
 
+	lbs_deb_hex(LBS_DEB_ASSOC, "ASSOC_CMD", (u8 *) cmd,
+			le16_to_cpu(cmd->hdr.size));
+
 	/* store for later use */
 	memcpy(priv->assoc_bss, bss->bssid, ETH_ALEN);

@@ -1089,14 +1092,28 @@ static int lbs_associate(struct lbs_private *priv,
 	if (ret)
 		goto done;
 
-
 	/* generate connect message to cfg80211 */
 
 	resp = (void *) cmd; /* recast for easier field access */
 	status = le16_to_cpu(resp->statuscode);
 
-	/* Convert statis code of old firmware */
-	if (priv->fwrelease < 0x09000000)
+	/* Older FW versions map the IEEE 802.11 Status Code in the association
+	 * response to the following values returned in resp->statuscode:
+	 *
+	 *    IEEE Status Code                Marvell Status Code
+	 *    0                       ->      0x0000 ASSOC_RESULT_SUCCESS
+	 *    13                      ->      0x0004 ASSOC_RESULT_AUTH_REFUSED
+	 *    14                      ->      0x0004 ASSOC_RESULT_AUTH_REFUSED
+	 *    15                      ->      0x0004 ASSOC_RESULT_AUTH_REFUSED
+	 *    16                      ->      0x0004 ASSOC_RESULT_AUTH_REFUSED
+	 *    others                  ->      0x0003 ASSOC_RESULT_REFUSED
+	 *
+	 * Other response codes:
+	 *    0x0001 -> ASSOC_RESULT_INVALID_PARAMETERS (unused)
+	 *    0x0002 -> ASSOC_RESULT_TIMEOUT (internal timer expired waiting for
+	 *                                    association response from the AP)
+	 */
+	if (MRVL_FW_MAJOR_REV(priv->fwrelease) <= 8) {
 		switch (status) {
 		case 0:
 			break;

@@ -1118,11 +1135,16 @@ static int lbs_associate(struct lbs_private *priv,
 			break;
 		default:
 			lbs_deb_assoc("association failure %d\n", status);
-			status = WLAN_STATUS_UNSPECIFIED_FAILURE;
+			/* v5 OLPC firmware does return the AP status code if
+			 * it's not one of the values above.  Let that through.
+			 */
+			break;
+		}
 	}
 
-	lbs_deb_assoc("status %d, capability 0x%04x\n", status,
-		      le16_to_cpu(resp->capability));
+	lbs_deb_assoc("status %d, statuscode 0x%04x, capability 0x%04x, "
+		      "aid 0x%04x\n", status, le16_to_cpu(resp->statuscode),
+		      le16_to_cpu(resp->capability), le16_to_cpu(resp->aid));
 
 	resp_ie_len = le16_to_cpu(resp->hdr.size)
 		- sizeof(resp->hdr)

@@ -1142,7 +1164,6 @@ static int lbs_associate(struct lbs_private *priv,
 			netif_tx_wake_all_queues(priv->dev);
 	}
 
-
 done:
 	lbs_deb_leave_args(LBS_DEB_CFG80211, "ret %d", ret);
 	return ret;

Signature · 1 line

1.7.2

Dan Williams

author

2010-07-30 04:14 UTC #4 lore raw

PATCH 4/5 libertas: fix association with some APs by using extended rates

In reply to Dan Williams

Some APs get pissy if you don't send the firmware the extended rates in the association request's rates TLV. Found this on a Linksys WRT54G v2; it denies association with status code 18 unless you add the extended rates too. The old driver did this, but it got lost in the cfg80211 conversion.

Signed-off-by Dan Williams <[email protected]>

---
 drivers/net/wireless/libertas/cfg.c |   56 +++++++++++++++++++++++++----------
 1 files changed, 40 insertions(+), 16 deletions(-)

drivers/net/wireless/libertas/cfg.c +40 ~~-16~~

index a9647e6..a8c0126 100644
@@ -257,6 +257,29 @@ static int lbs_add_supported_rates_tlv(u8 *tlv)
 	return sizeof(rate_tlv->header) + i;
 }
 
+/* Add common rates from a TLV and return the new end of the TLV */
+static u8 *
+add_ie_rates(u8 *tlv, const u8 *ie, int *nrates)
+{
+	int hw, ap, ap_max = ie[1];
+	u8 hw_rate;
+
+	/* Advance past IE header */
+	ie += 2;
+
+	lbs_deb_hex(LBS_DEB_ASSOC, "AP IE Rates", (u8 *) ie, ap_max);
+
+	for (hw = 0; hw < ARRAY_SIZE(lbs_rates); hw++) {
+		hw_rate = lbs_rates[hw].bitrate / 5;
+		for (ap = 0; ap < ap_max; ap++) {
+			if (hw_rate == (ie[ap] & 0x7f)) {
+				*tlv++ = ie[ap];
+				*nrates = *nrates + 1;
+			}
+		}
+	}
+	return tlv;
+}
 
 /*
  * Adds a TLV with all rates the hardware *and* BSS supports.

@@ -264,8 +287,11 @@ static int lbs_add_supported_rates_tlv(u8 *tlv)
 static int lbs_add_common_rates_tlv(u8 *tlv, struct cfg80211_bss *bss)
 {
 	struct mrvl_ie_rates_param_set *rate_tlv = (void *)tlv;
-	const u8 *rates_eid = ieee80211_bss_get_ie(bss, WLAN_EID_SUPP_RATES);
-	int n;
+	const u8 *rates_eid, *ext_rates_eid;
+	int n = 0;
+
+	rates_eid = ieee80211_bss_get_ie(bss, WLAN_EID_SUPP_RATES);
+	ext_rates_eid = ieee80211_bss_get_ie(bss, WLAN_EID_EXT_SUPP_RATES);
 
 	/*
 	 * 01 00                   TLV_TYPE_RATES

@@ -275,26 +301,21 @@ static int lbs_add_common_rates_tlv(u8 *tlv, struct cfg80211_bss *bss)
 	rate_tlv->header.type = cpu_to_le16(TLV_TYPE_RATES);
 	tlv += sizeof(rate_tlv->header);
 
-	if (!rates_eid) {
+	/* Add basic rates */
+	if (rates_eid) {
+		tlv = add_ie_rates(tlv, rates_eid, &n);
+
+		/* Add extended rates, if any */
+		if (ext_rates_eid)
+			tlv = add_ie_rates(tlv, ext_rates_eid, &n);
+	} else {
+		lbs_deb_assoc("assoc: bss had no basic rate IE\n");
 		/* Fallback: add basic 802.11b rates */
 		*tlv++ = 0x82;
 		*tlv++ = 0x84;
 		*tlv++ = 0x8b;
 		*tlv++ = 0x96;
 		n = 4;
-	} else {
-		int hw, ap;
-		u8 ap_max = rates_eid[1];
-		n = 0;
-		for (hw = 0; hw < ARRAY_SIZE(lbs_rates); hw++) {
-			u8 hw_rate = lbs_rates[hw].bitrate / 5;
-			for (ap = 0; ap < ap_max; ap++) {
-				if (hw_rate == (rates_eid[ap+2] & 0x7f)) {
-					*tlv++ = rates_eid[ap+2];
-					n++;
-				}
-			}
-		}
 	}
 
 	rate_tlv->header.len = cpu_to_le16(n);

@@ -1024,6 +1045,7 @@ static int lbs_associate(struct lbs_private *priv,
 	int status;
 	int ret;
 	u8 *pos = &(cmd->iebuf[0]);
+	u8 *tmp;
 
 	lbs_deb_enter(LBS_DEB_CFG80211);

@@ -1068,7 +1090,9 @@ static int lbs_associate(struct lbs_private *priv,
 	pos += lbs_add_cf_param_tlv(pos);
 
 	/* add rates TLV */
+	tmp = pos + 4; /* skip Marvell IE header */
 	pos += lbs_add_common_rates_tlv(pos, bss);
+	lbs_deb_hex(LBS_DEB_ASSOC, "Common Rates", tmp, pos - tmp);
 
 	/* add auth type TLV */
 	if (MRVL_FW_MAJOR_REV(priv->fwrelease) <= 9)

Signature · 1 line

1.7.2

Dan Williams

author

2010-07-30 04:17 UTC #5 lore raw

PATCH 5/5 libertas: scan before assocation if no BSSID was given

In reply to Dan Williams

Fix this leftover TODO from the cfg80211 conversion by doing a scan if cfg80211 didn't pass in the BSSID for us. Since the scan code uses so much of the cfg80211_scan_request structure to build up the firmware command, we just fake one when the scan request is triggered internally. But we need to make sure that internal 'fake' cfg82011 scan request does get back to cfg82011 via cfg80211_scan_done().

Signed-off-by Dan Williams <[email protected]>

---
 drivers/net/wireless/libertas/cfg.c  |  148 ++++++++++++++++++++++++++--------
 drivers/net/wireless/libertas/dev.h  |    5 +
 drivers/net/wireless/libertas/main.c |    1 +
 3 files changed, 121 insertions(+), 33 deletions(-)

drivers/net/wireless/libertas/cfg.c +115 ~~-33~~

index a8c0126..623b81e 100644
@@ -715,8 +715,13 @@ static void lbs_scan_worker(struct work_struct *work)
 
 	if (priv->scan_channel >= priv->scan_req->n_channels) {
 		/* Mark scan done */
-		cfg80211_scan_done(priv->scan_req, false);
+		if (priv->internal_scan)
+			kfree(priv->scan_req);
+		else
+			cfg80211_scan_done(priv->scan_req, false);
+
 		priv->scan_req = NULL;
+		priv->last_scan = jiffies;
 	}
 
 	/* Restart network */

@@ -727,10 +732,33 @@ static void lbs_scan_worker(struct work_struct *work)
 
 	kfree(scan_cmd);
 
+	/* Wake up anything waiting on scan completion */
+	if (priv->scan_req == NULL) {
+		lbs_deb_scan("scan: waking up waiters\n");
+		wake_up_all(&priv->scan_q);
+	}
+
  out_no_scan_cmd:
 	lbs_deb_leave(LBS_DEB_SCAN);
 }
 
+static void _internal_start_scan(struct lbs_private *priv, bool internal,
+	struct cfg80211_scan_request *request)
+{
+	lbs_deb_enter(LBS_DEB_CFG80211);
+
+	lbs_deb_scan("scan: ssids %d, channels %d, ie_len %zd\n",
+		request->n_ssids, request->n_channels, request->ie_len);
+
+	priv->scan_channel = 0;
+	queue_delayed_work(priv->work_thread, &priv->scan_work,
+		msecs_to_jiffies(50));
+
+	priv->scan_req = request;
+	priv->internal_scan = internal;
+
+	lbs_deb_leave(LBS_DEB_CFG80211);
+}
 
 static int lbs_cfg_scan(struct wiphy *wiphy,
 	struct net_device *dev,

@@ -747,18 +775,11 @@ static int lbs_cfg_scan(struct wiphy *wiphy,
 		goto out;
 	}
 
-	lbs_deb_scan("scan: ssids %d, channels %d, ie_len %zd\n",
-		request->n_ssids, request->n_channels, request->ie_len);
-
-	priv->scan_channel = 0;
-	queue_delayed_work(priv->work_thread, &priv->scan_work,
-		msecs_to_jiffies(50));
+	_internal_start_scan(priv, false, request);
 
 	if (priv->surpriseremoved)
 		ret = -EIO;
 
-	priv->scan_req = request;
-
  out:
 	lbs_deb_leave_args(LBS_DEB_CFG80211, "ret %d", ret);
 	return ret;

@@ -1193,7 +1214,62 @@ done:
 	return ret;
 }
 
+static struct cfg80211_scan_request *
+_new_connect_scan_req(struct wiphy *wiphy, struct cfg80211_connect_params *sme)
+{
+	struct cfg80211_scan_request *creq = NULL;
+	int i, n_channels = 0;
+	enum ieee80211_band band;
+
+	for (band = 0; band < IEEE80211_NUM_BANDS; band++) {
+		if (wiphy->bands[band])
+			n_channels += wiphy->bands[band]->n_channels;
+	}
+
+	creq = kzalloc(sizeof(*creq) + sizeof(struct cfg80211_ssid) +
+		       n_channels * sizeof(void *),
+		       GFP_ATOMIC);
+	if (!creq)
+		return NULL;
+
+	/* SSIDs come after channels */
+	creq->ssids = (void *)&creq->channels[n_channels];
+	creq->n_channels = n_channels;
+	creq->n_ssids = 1;
+
+	/* Scan all available channels */
+	i = 0;
+	for (band = 0; band < IEEE80211_NUM_BANDS; band++) {
+		int j;
+
+		if (!wiphy->bands[band])
+			continue;
+
+		for (j = 0; j < wiphy->bands[band]->n_channels; j++) {
+			/* ignore disabled channels */
+			if (wiphy->bands[band]->channels[j].flags &
+						IEEE80211_CHAN_DISABLED)
+				continue;
+
+			creq->channels[i] = &wiphy->bands[band]->channels[j];
+			i++;
+		}
+	}
+	if (i) {
+		/* Set real number of channels specified in creq->channels[] */
+		creq->n_channels = i;
+
+		/* Scan for the SSID we're going to connect to */
+		memcpy(creq->ssids[0].ssid, sme->ssid, sme->ssid_len);
+		creq->ssids[0].ssid_len = sme->ssid_len;
+	} else {
+		/* No channels found... */
+		kfree(creq);
+		creq = NULL;
+	}
 
+	return creq;
+}
 
 static int lbs_cfg_connect(struct wiphy *wiphy, struct net_device *dev,
 			   struct cfg80211_connect_params *sme)

@@ -1205,37 +1281,43 @@ static int lbs_cfg_connect(struct wiphy *wiphy, struct net_device *dev,
 
 	lbs_deb_enter(LBS_DEB_CFG80211);
 
-	if (sme->bssid) {
-		bss = cfg80211_get_bss(wiphy, sme->channel, sme->bssid,
-			sme->ssid, sme->ssid_len,
-			WLAN_CAPABILITY_ESS, WLAN_CAPABILITY_ESS);
-	} else {
-		/*
-		 * Here we have an impedance mismatch. The firmware command
-		 * CMD_802_11_ASSOCIATE always needs a BSSID, it cannot
-		 * connect otherwise. However, for the connect-API of
-		 * cfg80211 the bssid is purely optional. We don't get one,
-		 * except the user specifies one on the "iw" command line.
-		 *
-		 * If we don't got one, we could initiate a scan and look
-		 * for the best matching cfg80211_bss entry.
-		 *
-		 * Or, better yet, net/wireless/sme.c get's rewritten into
-		 * something more generally useful.
+	if (!sme->bssid) {
+		/* Run a scan if one isn't in-progress already and if the last
+		 * scan was done more than 2 seconds ago.
 		 */
-		lbs_pr_err("TODO: no BSS specified\n");
-		ret = -ENOTSUPP;
-		goto done;
-	}
+		if (priv->scan_req == NULL &&
+		    time_after(jiffies, priv->last_scan + (2 * HZ))) {
+			struct cfg80211_scan_request *creq;
 
+			creq = _new_connect_scan_req(wiphy, sme);
+			if (!creq) {
+				ret = -EINVAL;
+				goto done;
+			}
+
+			lbs_deb_assoc("assoc: scanning for compatible AP\n");
+			_internal_start_scan(priv, true, creq);
+		}
+
+		/* Wait for any in-progress scan to complete */
+		lbs_deb_assoc("assoc: waiting for scan to complete\n");
+		wait_event_interruptible_timeout(priv->scan_q,
+						 (priv->scan_req == NULL),
+						 (15 * HZ));
+		lbs_deb_assoc("assoc: scanning competed\n");
+	}
 
+	/* Find the BSS we want using available scan results */
+	bss = cfg80211_get_bss(wiphy, sme->channel, sme->bssid,
+		sme->ssid, sme->ssid_len,
+		WLAN_CAPABILITY_ESS, WLAN_CAPABILITY_ESS);
 	if (!bss) {
-		lbs_pr_err("assicate: bss %pM not in scan results\n",
+		lbs_pr_err("assoc: bss %pM not in scan results\n",
 			   sme->bssid);
 		ret = -ENOENT;
 		goto done;
 	}
-	lbs_deb_assoc("trying %pM", sme->bssid);
+	lbs_deb_assoc("trying %pM\n", bss->bssid);
 	lbs_deb_assoc("cipher 0x%x, key index %d, key len %d\n",
 		      sme->crypto.cipher_group,
 		      sme->key_idx, sme->key_len);

@@ -1298,7 +1380,7 @@ static int lbs_cfg_connect(struct wiphy *wiphy, struct net_device *dev,
 	lbs_set_radio(priv, preamble, 1);
 
 	/* Do the actual association */
-	lbs_associate(priv, bss, sme);
+	ret = lbs_associate(priv, bss, sme);
 
  done:
 	if (bss)

drivers/net/wireless/libertas/dev.h +5 -0

index 3c7e255..f062ed5 100644
@@ -161,6 +161,11 @@ struct lbs_private {
 	/** Scanning */
 	struct delayed_work scan_work;
 	int scan_channel;
+	/* Queue of things waiting for scan completion */
+	wait_queue_head_t scan_q;
+	/* Whether the scan was initiated internally and not by cfg80211 */
+	bool internal_scan;
+	unsigned long last_scan;
 };
 
 extern struct cmd_confirm_sleep confirm_sleep;

drivers/net/wireless/libertas/main.c +1 -0

index 2589671..24958a8 100644
@@ -719,6 +719,7 @@ static int lbs_init_adapter(struct lbs_private *priv)
 	priv->deep_sleep_required = 0;
 	priv->wakeup_dev_required = 0;
 	init_waitqueue_head(&priv->ds_awake_q);
+	init_waitqueue_head(&priv->scan_q);
 	priv->authtype_auto = 1;
 	priv->is_host_sleep_configured = 0;
 	priv->is_host_sleep_activated = 0;

Signature · 1 line

1.7.2

Johannes Berg

maintainer

2010-07-30 06:36 UTC #6 lore raw

Re: [wt PATCH 5/5] libertas: scan before assocation if no BSSID was given

Quoted 6 lines · Dan Williams

Fix this leftover TODO from the cfg80211 conversion by doing a scan if cfg80211 didn't pass in the BSSID for us. Since the scan code uses so much of the cfg80211_scan_request structure to build up the firmware command, we just fake one when the scan request is triggered internally. But we need to make sure that internal 'fake' cfg82011 scan request does get back to cfg82011 via cfg80211_scan_done().

I think you mean "does _not_ get back".

johannes

Dan Williams

author

2010-08-04 05:42 UTC #7 lore raw

PATCH v2 5/5 libertas: scan before assocation if no BSSID was given

In reply to Dan Williams

Signed-off-by Dan Williams <[email protected]>

---
 drivers/net/wireless/libertas/cfg.c  |  148 ++++++++++++++++++++++++++--------
 drivers/net/wireless/libertas/dev.h  |    5 +
 drivers/net/wireless/libertas/main.c |    1 +
 3 files changed, 121 insertions(+), 33 deletions(-)

v2: fixed commit message

drivers/net/wireless/libertas/cfg.c +115 ~~-33~~

index a8c0126..623b81e 100644
@@ -715,8 +715,13 @@ static void lbs_scan_worker(struct work_struct *work)
 
 	if (priv->scan_channel >= priv->scan_req->n_channels) {
 		/* Mark scan done */
-		cfg80211_scan_done(priv->scan_req, false);
+		if (priv->internal_scan)
+			kfree(priv->scan_req);
+		else
+			cfg80211_scan_done(priv->scan_req, false);
+
 		priv->scan_req = NULL;
+		priv->last_scan = jiffies;
 	}
 
 	/* Restart network */

@@ -727,10 +732,33 @@ static void lbs_scan_worker(struct work_struct *work)
 
 	kfree(scan_cmd);
 
+	/* Wake up anything waiting on scan completion */
+	if (priv->scan_req == NULL) {
+		lbs_deb_scan("scan: waking up waiters\n");
+		wake_up_all(&priv->scan_q);
+	}
+
  out_no_scan_cmd:
 	lbs_deb_leave(LBS_DEB_SCAN);
 }
 
+static void _internal_start_scan(struct lbs_private *priv, bool internal,
+	struct cfg80211_scan_request *request)
+{
+	lbs_deb_enter(LBS_DEB_CFG80211);
+
+	lbs_deb_scan("scan: ssids %d, channels %d, ie_len %zd\n",
+		request->n_ssids, request->n_channels, request->ie_len);
+
+	priv->scan_channel = 0;
+	queue_delayed_work(priv->work_thread, &priv->scan_work,
+		msecs_to_jiffies(50));
+
+	priv->scan_req = request;
+	priv->internal_scan = internal;
+
+	lbs_deb_leave(LBS_DEB_CFG80211);
+}
 
 static int lbs_cfg_scan(struct wiphy *wiphy,
 	struct net_device *dev,

@@ -747,18 +775,11 @@ static int lbs_cfg_scan(struct wiphy *wiphy,
 		goto out;
 	}
 
-	lbs_deb_scan("scan: ssids %d, channels %d, ie_len %zd\n",
-		request->n_ssids, request->n_channels, request->ie_len);
-
-	priv->scan_channel = 0;
-	queue_delayed_work(priv->work_thread, &priv->scan_work,
-		msecs_to_jiffies(50));
+	_internal_start_scan(priv, false, request);
 
 	if (priv->surpriseremoved)
 		ret = -EIO;
 
-	priv->scan_req = request;
-
  out:
 	lbs_deb_leave_args(LBS_DEB_CFG80211, "ret %d", ret);
 	return ret;

@@ -1193,7 +1214,62 @@ done:
 	return ret;
 }
 
+static struct cfg80211_scan_request *
+_new_connect_scan_req(struct wiphy *wiphy, struct cfg80211_connect_params *sme)
+{
+	struct cfg80211_scan_request *creq = NULL;
+	int i, n_channels = 0;
+	enum ieee80211_band band;
+
+	for (band = 0; band < IEEE80211_NUM_BANDS; band++) {
+		if (wiphy->bands[band])
+			n_channels += wiphy->bands[band]->n_channels;
+	}
+
+	creq = kzalloc(sizeof(*creq) + sizeof(struct cfg80211_ssid) +
+		       n_channels * sizeof(void *),
+		       GFP_ATOMIC);
+	if (!creq)
+		return NULL;
+
+	/* SSIDs come after channels */
+	creq->ssids = (void *)&creq->channels[n_channels];
+	creq->n_channels = n_channels;
+	creq->n_ssids = 1;
+
+	/* Scan all available channels */
+	i = 0;
+	for (band = 0; band < IEEE80211_NUM_BANDS; band++) {
+		int j;
+
+		if (!wiphy->bands[band])
+			continue;
+
+		for (j = 0; j < wiphy->bands[band]->n_channels; j++) {
+			/* ignore disabled channels */
+			if (wiphy->bands[band]->channels[j].flags &
+						IEEE80211_CHAN_DISABLED)
+				continue;
+
+			creq->channels[i] = &wiphy->bands[band]->channels[j];
+			i++;
+		}
+	}
+	if (i) {
+		/* Set real number of channels specified in creq->channels[] */
+		creq->n_channels = i;
+
+		/* Scan for the SSID we're going to connect to */
+		memcpy(creq->ssids[0].ssid, sme->ssid, sme->ssid_len);
+		creq->ssids[0].ssid_len = sme->ssid_len;
+	} else {
+		/* No channels found... */
+		kfree(creq);
+		creq = NULL;
+	}
 
+	return creq;
+}
 
 static int lbs_cfg_connect(struct wiphy *wiphy, struct net_device *dev,
 			   struct cfg80211_connect_params *sme)

@@ -1205,37 +1281,43 @@ static int lbs_cfg_connect(struct wiphy *wiphy, struct net_device *dev,
 
 	lbs_deb_enter(LBS_DEB_CFG80211);
 
-	if (sme->bssid) {
-		bss = cfg80211_get_bss(wiphy, sme->channel, sme->bssid,
-			sme->ssid, sme->ssid_len,
-			WLAN_CAPABILITY_ESS, WLAN_CAPABILITY_ESS);
-	} else {
-		/*
-		 * Here we have an impedance mismatch. The firmware command
-		 * CMD_802_11_ASSOCIATE always needs a BSSID, it cannot
-		 * connect otherwise. However, for the connect-API of
-		 * cfg80211 the bssid is purely optional. We don't get one,
-		 * except the user specifies one on the "iw" command line.
-		 *
-		 * If we don't got one, we could initiate a scan and look
-		 * for the best matching cfg80211_bss entry.
-		 *
-		 * Or, better yet, net/wireless/sme.c get's rewritten into
-		 * something more generally useful.
+	if (!sme->bssid) {
+		/* Run a scan if one isn't in-progress already and if the last
+		 * scan was done more than 2 seconds ago.
 		 */
-		lbs_pr_err("TODO: no BSS specified\n");
-		ret = -ENOTSUPP;
-		goto done;
-	}
+		if (priv->scan_req == NULL &&
+		    time_after(jiffies, priv->last_scan + (2 * HZ))) {
+			struct cfg80211_scan_request *creq;
 
+			creq = _new_connect_scan_req(wiphy, sme);
+			if (!creq) {
+				ret = -EINVAL;
+				goto done;
+			}
+
+			lbs_deb_assoc("assoc: scanning for compatible AP\n");
+			_internal_start_scan(priv, true, creq);
+		}
+
+		/* Wait for any in-progress scan to complete */
+		lbs_deb_assoc("assoc: waiting for scan to complete\n");
+		wait_event_interruptible_timeout(priv->scan_q,
+						 (priv->scan_req == NULL),
+						 (15 * HZ));
+		lbs_deb_assoc("assoc: scanning competed\n");
+	}
 
+	/* Find the BSS we want using available scan results */
+	bss = cfg80211_get_bss(wiphy, sme->channel, sme->bssid,
+		sme->ssid, sme->ssid_len,
+		WLAN_CAPABILITY_ESS, WLAN_CAPABILITY_ESS);
 	if (!bss) {
-		lbs_pr_err("assicate: bss %pM not in scan results\n",
+		lbs_pr_err("assoc: bss %pM not in scan results\n",
 			   sme->bssid);
 		ret = -ENOENT;
 		goto done;
 	}
-	lbs_deb_assoc("trying %pM", sme->bssid);
+	lbs_deb_assoc("trying %pM\n", bss->bssid);
 	lbs_deb_assoc("cipher 0x%x, key index %d, key len %d\n",
 		      sme->crypto.cipher_group,
 		      sme->key_idx, sme->key_len);

@@ -1298,7 +1380,7 @@ static int lbs_cfg_connect(struct wiphy *wiphy, struct net_device *dev,
 	lbs_set_radio(priv, preamble, 1);
 
 	/* Do the actual association */
-	lbs_associate(priv, bss, sme);
+	ret = lbs_associate(priv, bss, sme);
 
  done:
 	if (bss)

drivers/net/wireless/libertas/dev.h +5 -0

index 3c7e255..f062ed5 100644
@@ -161,6 +161,11 @@ struct lbs_private {
 	/** Scanning */
 	struct delayed_work scan_work;
 	int scan_channel;
+	/* Queue of things waiting for scan completion */
+	wait_queue_head_t scan_q;
+	/* Whether the scan was initiated internally and not by cfg80211 */
+	bool internal_scan;
+	unsigned long last_scan;
 };
 
 extern struct cmd_confirm_sleep confirm_sleep;

drivers/net/wireless/libertas/main.c +1 -0

index 2589671..24958a8 100644
@@ -719,6 +719,7 @@ static int lbs_init_adapter(struct lbs_private *priv)
 	priv->deep_sleep_required = 0;
 	priv->wakeup_dev_required = 0;
 	init_waitqueue_head(&priv->ds_awake_q);
+	init_waitqueue_head(&priv->scan_q);
 	priv->authtype_auto = 1;
 	priv->is_host_sleep_configured = 0;
 	priv->is_host_sleep_activated = 0;

Signature · 1 line

1.7.2

Johannes Berg

maintainer

2010-07-30 06:33 UTC #8 lore raw

In reply to Dan Williams

Quoted 7 lines · Dan Williams

This series makes libertas actually associate with APs after the cfg80211 conversion. There was a pretty big TODO where if cfg80211 didn't pass in the BSSID of the AP to associate with, then libertas would fail because the firmware really, really, really wants a BSSID. So lets do a scan to find that BSSID. The other notable change is a fix to make sure all supported rates are included in the firmware's association request, lack of which caused some APs to deny the attempt.

I'm a bit tempted to say we could use the connect logic in cfg80211 to provide this? cfg80211 _already_ provides this if the hardware wants auth()/assoc() rather than connect() calls, so it seems fairly simple to also (optionally) do it here?

johannes

Holger Schurig

2010-07-31 13:54 UTC #9 lore raw

Quoted 4 lines

I'm a bit tempted to say we could use the connect logic in cfg80211 to provide this? cfg80211 _already_ provides this if the hardware wants auth()/assoc() rather than connect() calls, so it seems fairly simple to also (optionally) do it here?

Some months ago I was thinking the same. I thought that the auth/assoc and connect driver APIs where quite unorthogonal. However, I did not understand all of cfg80211's state machinery to make the connect branch of the API also support a BSSID.

If that could be done this way, I'd prefer it over a libertas-internal hack.

Basically we'd need this to be controller from the driver:

do scan before connect
use connect() or auth()/assoc() to connect

That way ...

... some driver can say "no scan, use connect" (orinoco, e.g.) ... or "scan, use connect" (libertas)
... or "scan, use auth/assoc" (mac80211)

Signature · 1 line

http://www.holgerschurig.de

Dan Williams

author

2010-08-04 05:39 UTC #10 lore raw

In reply to Johannes Berg

Quoted 9 lines · Johannes Berg

Quoted 7 lines · Dan Williams
This series makes libertas actually associate with APs after the cfg80211 conversion. There was a pretty big TODO where if cfg80211 didn't pass in the BSSID of the AP to associate with, then libertas would fail because the firmware really, really, really wants a BSSID. So lets do a scan to find that BSSID. The other notable change is a fix to make sure all supported rates are included in the firmware's association request, lack of which caused some APs to deny the attempt.

Quoted 4 lines

I'm a bit tempted to say we could use the connect logic in cfg80211 to provide this? cfg80211 _already_ provides this if the hardware wants auth()/assoc() rather than connect() calls, so it seems fairly simple to also (optionally) do it here?

Yeah, I thought about that too. Do you want me to work on that now instead of letting this patch through? I can revert the relevant bits of this later if that's acceptable too.

Dan

Johannes Berg

maintainer

2010-08-04 07:45 UTC #11 lore raw

Quoted 10 lines · Dan Williams

Quoted 9 lines · Johannes Berg
Quoted 7 lines · Dan Williams
This series makes libertas actually associate with APs after the cfg80211 conversion. There was a pretty big TODO where if cfg80211 didn't pass in the BSSID of the AP to associate with, then libertas would fail because the firmware really, really, really wants a BSSID. So lets do a scan to find that BSSID. The other notable change is a fix to make sure all supported rates are included in the firmware's association request, lack of which caused some APs to deny the attempt.

Quoted 8 lines

Quoted 4 lines
I'm a bit tempted to say we could use the connect logic in cfg80211 to provide this? cfg80211 _already_ provides this if the hardware wants auth()/assoc() rather than connect() calls, so it seems fairly simple to also (optionally) do it here?
Yeah, I thought about that too. Do you want me to work on that now instead of letting this patch through? I can revert the relevant bits of this later if that's acceptable too.

Either way works for me since I don't care about any cruft in libertas ;)

johannes

Dan Williams

author

2010-08-04 19:14 UTC #12 lore raw

Quoted 16 lines · Johannes Berg

Quoted 15 lines · Dan Williams
Quoted 13 lines · Johannes Berg
Quoted 7 lines · Dan Williams
This series makes libertas actually associate with APs after the cfg80211 conversion. There was a pretty big TODO where if cfg80211 didn't pass in the BSSID of the AP to associate with, then libertas would fail because the firmware really, really, really wants a BSSID. So lets do a scan to find that BSSID. The other notable change is a fix to make sure all supported rates are included in the firmware's association request, lack of which caused some APs to deny the attempt.
I'm a bit tempted to say we could use the connect logic in cfg80211 to provide this? cfg80211 _already_ provides this if the hardware wants auth()/assoc() rather than connect() calls, so it seems fairly simple to also (optionally) do it here?

Quoted 6 lines

Quoted 3 lines
Yeah, I thought about that too. Do you want me to work on that now instead of letting this patch through? I can revert the relevant bits of this later if that's acceptable too.
Either way works for me since I don't care about any cruft in libertas ;)

John, if you don't mind applying this patch I'll work on generic scan-before-connect support in cfg80211 in a further patch series.

Dan

John W. Linville

2010-08-04 19:44 UTC #13 lore raw

Quoted 21 lines · Dan Williams

Quoted 20 lines · Johannes Berg
Quoted 18 lines · Dan Williams
Quoted 9 lines · Johannes Berg
Quoted 7 lines · Dan Williams
This series makes libertas actually associate with APs after the cfg80211 conversion. There was a pretty big TODO where if cfg80211 didn't pass in the BSSID of the AP to associate with, then libertas would fail because the firmware really, really, really wants a BSSID. So lets do a scan to find that BSSID. The other notable change is a fix to make sure all supported rates are included in the firmware's association request, lack of which caused some APs to deny the attempt.
Quoted 4 lines
I'm a bit tempted to say we could use the connect logic in cfg80211 to provide this? cfg80211 _already_ provides this if the hardware wants auth()/assoc() rather than connect() calls, so it seems fairly simple to also (optionally) do it here?
Yeah, I thought about that too. Do you want me to work on that now instead of letting this patch through? I can revert the relevant bits of this later if that's acceptable too.

Quoted 5 lines

Quoted 2 lines
Either way works for me since I don't care about any cruft in libertas ;)
John, if you don't mind applying this patch I'll work on generic scan-before-connect support in cfg80211 in a further patch series.

OK...which of these need to be in 2.6.36?

John

Signature · 2 lines

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

Dan Williams

author

2010-08-05 17:26 UTC #14 lore raw

Quoted 22 lines · John W. Linville

Quoted 21 lines · Dan Williams
Quoted 20 lines · Johannes Berg
Quoted 18 lines · Dan Williams
Quoted 9 lines · Johannes Berg
Quoted 7 lines · Dan Williams
This series makes libertas actually associate with APs after the cfg80211 conversion. There was a pretty big TODO where if cfg80211 didn't pass in the BSSID of the AP to associate with, then libertas would fail because the firmware really, really, really wants a BSSID. So lets do a scan to find that BSSID. The other notable change is a fix to make sure all supported rates are included in the firmware's association request, lack of which caused some APs to deny the attempt.
Quoted 4 lines
I'm a bit tempted to say we could use the connect logic in cfg80211 to provide this? cfg80211 _already_ provides this if the hardware wants auth()/assoc() rather than connect() calls, so it seems fairly simple to also (optionally) do it here?
Yeah, I thought about that too. Do you want me to work on that now instead of letting this patch through? I can revert the relevant bits of this later if that's acceptable too.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Either way works for me since I don't care about any cruft in libertas ;)
John, if you don't mind applying this patch I'll work on generic scan-before-connect support in cfg80211 in a further patch series.
OK...which of these need to be in 2.6.36?

They should stick with the libertas cfg80211 bits, wherever they are. If you've pushed whatever was in wireless-testing into 2.6.36, then this latest series of 5 should go there too.

Dan