diff mbox

rndis_wlan: wait association to complete

Message ID 20090806181746.31308.15687.stgit@fate.lan (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Jussi Kivilinna Aug. 6, 2009, 6:17 p.m. UTC 
Fix WPA authentication problems by waiting to association to complete.
Otherwise userspace (wpa_supplicant) receives authentication packets before
association events from driver.

Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
---

 drivers/net/wireless/rndis_wlan.c |   47 +++++++++++++++++++++++++++++++++----
 1 files changed, 42 insertions(+), 5 deletions(-)


--
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

Comments

Johannes Berg Aug. 6, 2009, 6:46 p.m. UTC | #1 
On Thu, 2009-08-06 at 21:17 +0300, Jussi Kivilinna wrote:
> Fix WPA authentication problems by waiting to association to complete.
> Otherwise userspace (wpa_supplicant) receives authentication packets before
> association events from driver.

> +	/* If we return now, userspace would get association events too late
> +	 * (after receiving first packets from access point). This causes
> +	 * WPA authentication to fail.

This is a bit weird, shouldn't you just send the event later then?

johannes
Jussi Kivilinna Aug. 6, 2009, 7:34 p.m. UTC | #2 
Quoting "Johannes Berg" <johannes@sipsolutions.net>:

> On Thu, 2009-08-06 at 21:17 +0300, Jussi Kivilinna wrote:
>> Fix WPA authentication problems by waiting to association to complete.
>> Otherwise userspace (wpa_supplicant) receives authentication packets before
>> association events from driver.
>
>> +	/* If we return now, userspace would get association events too late
>> +	 * (after receiving first packets from access point). This causes
>> +	 * WPA authentication to fail.
>
> This is a bit weird, shouldn't you just send the event later then?
>

rndis_wlan bases on rndis_host/usbnet. Problem is that rndis_host uses  
polling for rndis_command() results to avoid more complex interrupt  
based solution. Because rndis_host and therefore rndis_wlan doesn't  
handle interrupts on command channel, rndis_wlan knows about completed  
association too late. 'Too late' means here that device starts  
receiving packets from AP, which end up ignored because userspace  
thinks device isn't assosiated yet (I think).

So this patch adds waiting in set_essid() and active polling for  
association completion in worker.

Real fix would probably be to hold on data channel until association  
is complete, but that would mean touching usbnet/rndis_host little bit  
too much than I'd like to.

--
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
Dan Williams Aug. 7, 2009, 9:31 p.m. UTC | #3 
On Thu, 2009-08-06 at 22:34 +0300, Jussi Kivilinna wrote:
> Quoting "Johannes Berg" <johannes@sipsolutions.net>:
> 
> > On Thu, 2009-08-06 at 21:17 +0300, Jussi Kivilinna wrote:
> >> Fix WPA authentication problems by waiting to association to complete.
> >> Otherwise userspace (wpa_supplicant) receives authentication packets before
> >> association events from driver.
> >
> >> +	/* If we return now, userspace would get association events too late
> >> +	 * (after receiving first packets from access point). This causes
> >> +	 * WPA authentication to fail.
> >
> > This is a bit weird, shouldn't you just send the event later then?
> >
> 
> rndis_wlan bases on rndis_host/usbnet. Problem is that rndis_host uses  
> polling for rndis_command() results to avoid more complex interrupt  
> based solution. Because rndis_host and therefore rndis_wlan doesn't  
> handle interrupts on command channel, rndis_wlan knows about completed  
> association too late. 'Too late' means here that device starts  
> receiving packets from AP, which end up ignored because userspace  
> thinks device isn't assosiated yet (I think).
> 
> So this patch adds waiting in set_essid() and active polling for  
> association completion in worker.
> 
> Real fix would probably be to hold on data channel until association  
> is complete, but that would mean touching usbnet/rndis_host little bit  
> too much than I'd like to.

If this patch ever blocks SIOSIWESSID, then a huge NAK.  Setting the
ESSID is an asynchronous operation, it should *not* block for operation
to complete.  Completion is signaled only when an IWESSID event is sent
to userspace.  What you should be doing is dropping packets from the AP
on the floor until the firmware things the association is complete.

Dan


--
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
Jussi Kivilinna Aug. 8, 2009, 8:33 a.m. UTC | #4 
Quoting "Dan Williams" <dcbw@redhat.com>:

> On Thu, 2009-08-06 at 22:34 +0300, Jussi Kivilinna wrote:
>> Quoting "Johannes Berg" <johannes@sipsolutions.net>:
>>
>> > On Thu, 2009-08-06 at 21:17 +0300, Jussi Kivilinna wrote:
>> >> Fix WPA authentication problems by waiting to association to complete.
>> >> Otherwise userspace (wpa_supplicant) receives authentication  
>> packets before
>> >> association events from driver.
>> >
>> >> +	/* If we return now, userspace would get association events too late
>> >> +	 * (after receiving first packets from access point). This causes
>> >> +	 * WPA authentication to fail.
>> >
>> > This is a bit weird, shouldn't you just send the event later then?
>> >
>>
>> rndis_wlan bases on rndis_host/usbnet. Problem is that rndis_host uses
>> polling for rndis_command() results to avoid more complex interrupt
>> based solution. Because rndis_host and therefore rndis_wlan doesn't
>> handle interrupts on command channel, rndis_wlan knows about completed
>> association too late. 'Too late' means here that device starts
>> receiving packets from AP, which end up ignored because userspace
>> thinks device isn't assosiated yet (I think).
>>
>> So this patch adds waiting in set_essid() and active polling for
>> association completion in worker.
>>
>> Real fix would probably be to hold on data channel until association
>> is complete, but that would mean touching usbnet/rndis_host little bit
>> too much than I'd like to.
>
> If this patch ever blocks SIOSIWESSID, then a huge NAK.  Setting the
> ESSID is an asynchronous operation, it should *not* block for operation
> to complete.  Completion is signaled only when an IWESSID event is sent
> to userspace.  What you should be doing is dropping packets from the AP
> on the floor until the firmware things the association is complete.
>

Ok, I noticed that I need to do this 'right way' anyway for  
cfg80211-connect conversion. So I'm taking this one back and work out  
better fix for/with connect patches.

-Jussi

--
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
diff mbox

Patch

diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
index 828dc18..f2e7505 100644
--- a/drivers/net/wireless/rndis_wlan.c
+++ b/drivers/net/wireless/rndis_wlan.c
@@ -378,6 +378,7 @@  enum wpa_key_mgmt { KEY_MGMT_802_1X, KEY_MGMT_PSK, KEY_MGMT_NONE,
 #define WORK_LINK_UP		(1<<0)
 #define WORK_LINK_DOWN		(1<<1)
 #define WORK_SET_MULTICAST_LIST	(1<<2)
+#define WORK_ASSOC_WAIT		(1<<3)
 
 #define COMMAND_BUFFER_SIZE	(CONTROL_BUFFER_SIZE + sizeof(struct rndis_set))
 
@@ -436,6 +437,7 @@  struct rndis_wlan_private {
 	struct work_struct work;
 	struct mutex command_lock;
 	spinlock_t stats_lock;
+	struct completion assoc_wait;
 	unsigned long work_pending;
 
 	struct ieee80211_supported_band band;
@@ -936,7 +938,9 @@  static int get_essid(struct usbnet *usbdev, struct ndis_80211_ssid *ssid)
 }
 
 
-static int set_essid(struct usbnet *usbdev, struct ndis_80211_ssid *ssid)
+#define ASSOC_TIMEOUT_JIFFIES (HZ*10)
+static int set_essid(struct usbnet *usbdev, struct ndis_80211_ssid *ssid,
+								bool wait)
 {
 	struct rndis_wlan_private *priv = get_rndis_wlan_priv(usbdev);
 	int ret;
@@ -948,6 +952,21 @@  static int set_essid(struct usbnet *usbdev, struct ndis_80211_ssid *ssid)
 		devdbg(usbdev, "set_essid: radio_on = 1");
 	}
 
+	if (!wait)
+		return ret;
+
+	/* If we return now, userspace would get association events too late
+	 * (after receiving first packets from access point). This causes
+	 * WPA authentication to fail.
+	 */
+	set_bit(WORK_ASSOC_WAIT, &priv->work_pending);
+	queue_work(priv->workqueue, &priv->work);
+
+	wait_for_completion_interruptible_timeout(&priv->assoc_wait,
+							ASSOC_TIMEOUT_JIFFIES);
+
+	clear_bit(WORK_ASSOC_WAIT, &priv->work_pending);
+
 	return ret;
 }
 
@@ -1009,7 +1028,7 @@  static int disassociate(struct usbnet *usbdev, int reset_ssid)
 		ssid.essid[1] = 0xff;
 		for (i = 2; i < sizeof(ssid.essid); i++)
 			ssid.essid[i] = 0x1 + (ssid.essid[i] * 0xfe / 0xff);
-		ret = set_essid(usbdev, &ssid);
+		ret = set_essid(usbdev, &ssid, false);
 	}
 	return ret;
 }
@@ -1765,7 +1784,7 @@  static int rndis_iw_set_essid(struct net_device *dev,
 	if (!wrqu->essid.flags || length == 0)
 		return disassociate(usbdev, 1);
 	else
-		return set_essid(usbdev, &ssid);
+		return set_essid(usbdev, &ssid, true);
 }
 
 
@@ -2006,7 +2025,7 @@  static int rndis_iw_set_encode(struct net_device *dev,
 
 	if (index == priv->encr_tx_key_index)
 		/* ndis drivers want essid to be set after setting encr */
-		set_essid(usbdev, &priv->essid);
+		set_essid(usbdev, &priv->essid, false);
 
 	return 0;
 }
@@ -2288,7 +2307,20 @@  static void rndis_wlan_worker(struct work_struct *work)
 	unsigned char bssid[ETH_ALEN];
 	struct ndis_80211_assoc_info *info;
 	int assoc_size = sizeof(*info) + IW_CUSTOM_MAX + 32;
-	int ret, offset;
+	int ret, offset, len;
+	__le32 tmp = 0;
+
+	if (test_bit(WORK_ASSOC_WAIT, &priv->work_pending)) {
+		/* dummy OID to poll device */
+		len = sizeof(tmp);
+		rndis_query_oid(usbdev, OID_GEN_XMIT_ERROR, &tmp, &len);
+
+		/* requeue work if no link up response from device */
+		if (!test_bit(WORK_LINK_UP, &priv->work_pending)) {
+			msleep(10);
+			queue_work(priv->workqueue, &priv->work);
+		}
+	}
 
 	if (test_and_clear_bit(WORK_LINK_UP, &priv->work_pending)) {
 		netif_carrier_on(usbdev->net);
@@ -2328,6 +2360,8 @@  get_bssid:
 			memcpy(evt.ap_addr.sa_data, bssid, ETH_ALEN);
 			wireless_send_event(usbdev->net, SIOCGIWAP, &evt, NULL);
 		}
+
+		complete_all(&priv->assoc_wait);
 	}
 
 	if (test_and_clear_bit(WORK_LINK_DOWN, &priv->work_pending)) {
@@ -2830,6 +2864,7 @@  static int rndis_wlan_bind(struct usbnet *usbdev, struct usb_interface *intf)
 
 	mutex_init(&priv->command_lock);
 	spin_lock_init(&priv->stats_lock);
+	init_completion(&priv->assoc_wait);
 
 	/* because rndis_command() sleeps we need to use workqueue */
 	priv->workqueue = create_singlethread_workqueue("rndis_wlan");
@@ -2932,6 +2967,7 @@  static void rndis_wlan_unbind(struct usbnet *usbdev, struct usb_interface *intf)
 	/* turn radio off */
 	disassociate(usbdev, 0);
 
+	complete_all(&priv->assoc_wait);
 	cancel_delayed_work_sync(&priv->stats_work);
 	cancel_delayed_work_sync(&priv->scan_work);
 	cancel_work_sync(&priv->work);
@@ -2981,6 +3017,7 @@  static int rndis_wlan_stop(struct usbnet *usbdev)
 	retval = disassociate(usbdev, 0);
 
 	priv->work_pending = 0;
+	complete_all(&priv->assoc_wait);
 	cancel_delayed_work_sync(&priv->stats_work);
 	cancel_delayed_work_sync(&priv->scan_work);
 	cancel_work_sync(&priv->work);