d80211: make sleeping in hw->config possible #2

PATCH In review Michael Buesch 4 participants 10 messages Jul 10, 2006 – Jul 18, 2006 1 message not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Michael Buesch

author

2006-07-10 22:55 UTC #0 lore raw

PATCH 1/1 d80211: make sleeping in hw->config possible #2

Hi John,

Please apply this to wireless-dev.
Note that this is the second try to submit this patch.
The first try contained a little bug. I'm sorry for that.
If you already applied the first one, I can provide an incremental patch.

Note2 that this patch depends on the
[PATCH] cancel_rearming_delayed_work infinite loop fix I just sent out to the lists and akpm.

This patch makes sleeping in the hw->config callback possible by removing the only atomic caller. The atomic caller was a timer and is replaced by a workqueue.

In general, allowing to sleep in the config callback is a good thing. bcm43xx must be able to sleep here, as it is required to lock a mutex.
But there are other good reasons to sleep here. We might want to sleep for a grace period on channel switch, for example.

Signed-off-by Michael Buesch <[email protected]>

net/d80211/ieee80211.c +2 -2

Index: wireless-dev-dscapeports/net/d80211/ieee80211.c
===================================================================
@@ -4327,8 +4327,8 @@
 		del_timer_sync(&local->rate_limit_timer);
 	if (local->stat_time)
 		del_timer_sync(&local->stat_timer);
-	if (local->scan_timer.data)
-		del_timer_sync(&local->scan_timer);
+	if (local->scan_work.data)
+		cancel_rearming_delayed_work(&local->scan_work);
 	ieee80211_rx_bss_list_deinit(dev);
 
 	rtnl_lock();

net/d80211/ieee80211_i.h +2 -1

Index: wireless-dev-dscapeports/net/d80211/ieee80211_i.h
===================================================================
@@ -17,6 +17,7 @@
 #include <linux/list.h>
 #include <linux/netdevice.h>
 #include <linux/skbuff.h>
+#include <linux/workqueue.h>
 #include "ieee80211_key.h"
 #include "sta_info.h"

@@ -407,7 +408,7 @@
 	int scan_channel_idx;
 	enum { SCAN_SET_CHANNEL, SCAN_SEND_PROBE } scan_state;
 	unsigned long last_scan_completed;
-	struct timer_list scan_timer;
+	struct work_struct scan_work;
 	int scan_oper_channel;
 	int scan_oper_channel_val;
 	int scan_oper_power_level;

net/d80211/ieee80211_iface.c +2 -2

Index: wireless-dev-dscapeports/net/d80211/ieee80211_iface.c
===================================================================
@@ -271,8 +271,8 @@
 	case IEEE80211_IF_TYPE_STA:
 	case IEEE80211_IF_TYPE_IBSS:
 		del_timer_sync(&sdata->u.sta.timer);
-		if (local->scan_timer.data == (unsigned long) sdata->dev)
-			del_timer_sync(&local->scan_timer);
+		if (local->scan_work.data == sdata->dev)
+			cancel_rearming_delayed_work(&local->scan_work);
 		kfree(sdata->u.sta.extra_ie);
 		sdata->u.sta.extra_ie = NULL;
 		kfree(sdata->u.sta.assocreq_ies);

net/d80211/ieee80211_sta.c +14 ~~-18~~

Index: wireless-dev-dscapeports/net/d80211/ieee80211_sta.c
===================================================================
@@ -2422,15 +2422,16 @@
 }
 
 
-static void ieee80211_sta_scan_timer(unsigned long ptr)
+static void ieee80211_sta_scan_work(void *_data)
 {
-	struct net_device *dev = (struct net_device *) ptr;
+	struct net_device *dev = _data;
 	struct ieee80211_local *local = dev->ieee80211_ptr;
         struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	struct ieee80211_hw_modes *mode;
 	struct ieee80211_channel *chan;
 	int skip;
 	union iwreq_data wrqu;
+	unsigned long next_delay = 0;
 
 	if (!local->sta_scanning)
 		return;

@@ -2498,29 +2499,27 @@
 			local->scan_channel_idx = 0;
 		}
 
-		if (skip) {
-			local->scan_timer.expires = jiffies;
+		if (skip)
 			break;
-		}
 
-		local->scan_timer.expires = jiffies + IEEE80211_PROBE_DELAY;
+		next_delay = IEEE80211_PROBE_DELAY;
 		local->scan_state = SCAN_SEND_PROBE;
 		break;
 	case SCAN_SEND_PROBE:
 		if (ieee80211_active_scan(local)) {
 			ieee80211_send_probe_req(dev, NULL, local->scan_ssid,
 						 local->scan_ssid_len);
-			local->scan_timer.expires =
-				jiffies + IEEE80211_CHANNEL_TIME;
-		} else {
-			local->scan_timer.expires =
-				jiffies + IEEE80211_PASSIVE_CHANNEL_TIME;
-		}
+			next_delay = IEEE80211_CHANNEL_TIME;
+		} else
+			next_delay = IEEE80211_PASSIVE_CHANNEL_TIME;
 		local->scan_state = SCAN_SET_CHANNEL;
 		break;
 	}
 
-	add_timer(&local->scan_timer);
+	if (next_delay)
+		schedule_delayed_work(&local->scan_work, next_delay);
+	else
+		schedule_work(&local->scan_work);
 }

@@ -2569,11 +2568,8 @@
 	local->scan_state = SCAN_SET_CHANNEL;
 	local->scan_hw_mode_idx = 0;
 	local->scan_channel_idx = 0;
-	init_timer(&local->scan_timer);
-	local->scan_timer.data = (unsigned long) dev;
-	local->scan_timer.function = ieee80211_sta_scan_timer;
-	local->scan_timer.expires = jiffies + 1;
-	add_timer(&local->scan_timer);
+	INIT_WORK(&local->scan_work, ieee80211_sta_scan_work, dev);
+	schedule_work(&local->scan_work);
 
 	return 0;
 }

Signature · 1 line

Greetings Michael.

Andrew Morton

2006-07-11 04:26 UTC #1 lore raw

Quoted 8 lines · On Tue, 11 Jul 2006 00:54:33 +0200 Michael Buesch <[email protected]> wrote:

Please apply this to wireless-dev.
Note that this is the second try to submit this patch.
The first try contained a little bug. I'm sorry for that.
If you already applied the first one, I can provide an incremental patch.
Note2 that this patch depends on the
[PATCH] cancel_rearming_delayed_work infinite loop fix I just sent out to the lists and akpm.

Am still scratching my head over that. I wouldn't really call it a "fix". More an enhcancement to cover unanticipated (and arguably strange) usage.

It's odd to call cancel_rearming_delayed_work() against a rearming workqueue which isn't actually running. It tends to indicate that the caller has lost track of what it's up to.

But as a convenience thing I guess it's an OK thing to do. I need to stare at the implementation for a bit longer - that stuff's tricky.

Michael Buesch

author

2006-07-11 09:09 UTC #2 lore raw

Quoted 12 lines · you

Quoted 8 lines · On Tue, 11 Jul 2006 00:54:33 +0200 Michael Buesch <[email protected]> wrote:
Please apply this to wireless-dev.
Note that this is the second try to submit this patch.
The first try contained a little bug. I'm sorry for that.
If you already applied the first one, I can provide an incremental patch.
Note2 that this patch depends on the
[PATCH] cancel_rearming_delayed_work infinite loop fix I just sent out to the lists and akpm.

Quoted 6 lines

Am still scratching my head over that. I wouldn't really call it a "fix". More an enhcancement to cover unanticipated (and arguably strange) usage.
It's odd to call cancel_rearming_delayed_work() against a rearming workqueue which isn't actually running. It tends to indicate that the caller has lost track of what it's up to.

No, I don't think so.
Let's say we have the following scenario:
A wq reschedules itself x times after it was scheduled once from outside. After these x times, it does not reschedule anymore. That's what happens in d80211. And I don't see a solution to sync this, other than modifying the function, because we may call it after the x reschedule times. Or do you think we should really do a statemachine to workaround it?

I am not the first one to hit this (I call it) bug.
It is _very_ confusing to see this sync function blocking forever. I saw several people complaining about it. Also on #kernelnewbies.

Quoted 2 lines

But as a convenience thing I guess it's an OK thing to do. I need to stare at the implementation for a bit longer - that stuff's tricky.

Actually, I think there's still a little race.
I will send a more complex fix for this, if you agree to change the function. If you say "no, we don't fix this. Insert a statemachine or something in your code instead", I can use the time for better things. :)

But I think the following is also broken in the old code:
A wq is not pending anymore, but just executing (before it reschedules itself). I think that would also loop forever. I don't think that's what we want. Because we can't really keep track of _this_.

Signature · 1 line

Greetings Michael.

Andrew Morton

2006-07-11 09:31 UTC #3 lore raw

Quoted 4 lines · On Tue, 11 Jul 2006 11:11:27 +0200 Michael Buesch <[email protected]> wrote:

But I think the following is also broken in the old code:
A wq is not pending anymore, but just executing (before it reschedules itself). I think that would also loop forever. I don't think that's what we want. Because we can't really keep track of _this_.

The present implementation assumes that the handler will re-arm itself.

I agree that extending that makes sense. But beware that it's easy to leave subtle holes in this logic. Needs careful thought to get right.

Michael Buesch

author

2006-07-11 10:11 UTC #4 lore raw

Quoted 8 lines · you

Quoted 4 lines · On Tue, 11 Jul 2006 11:11:27 +0200 Michael Buesch <[email protected]> wrote:
But I think the following is also broken in the old code:
A wq is not pending anymore, but just executing (before it reschedules itself). I think that would also loop forever. I don't think that's what we want. Because we can't really keep track of _this_.

Quoted 4 lines

The present implementation assumes that the handler will re-arm itself.
I agree that extending that makes sense. But beware that it's easy to leave subtle holes in this logic. Needs careful thought to get right.

Yeah, as I said. There is still a race.
Should I redo the patch to fix it?

Signature · 1 line

Greetings Michael.

Jiri Benc

2006-07-12 16:53 UTC #5 lore raw

In reply to Michael Buesch

Quoted 8 lines · Michael Buesch

Please apply this to wireless-dev.
Note that this is the second try to submit this patch.
The first try contained a little bug. I'm sorry for that.
If you already applied the first one, I can provide an incremental patch.
Note2 that this patch depends on the
[PATCH] cancel_rearming_delayed_work infinite loop fix I just sent out to the lists and akpm.

I'm still digging through this. But I think it is not necessary to use cancel_rearming_delayed_work at all; will try to make a patch.

Thanks,

Jiri

Signature · 2 lines

Jiri Benc
SUSE Labs

Michael Buesch

author

2006-07-12 20:32 UTC #6 lore raw

Quoted 6 lines · Jiri Benc

Quoted 5 lines · Michael Buesch
Please apply this to wireless-dev.
Note that this is the second try to submit this patch.
The first try contained a little bug. I'm sorry for that.
If you already applied the first one, I can provide an incremental patch.

Quoted 6 lines

Quoted 3 lines
Note2 that this patch depends on the
[PATCH] cancel_rearming_delayed_work infinite loop fix I just sent out to the lists and akpm.
I'm still digging through this. But I think it is not necessary to use cancel_rearming_delayed_work at all; will try to make a patch.

Jiri, so you care to get that patch upstream?
Would be cool, thanks.

John, do you manage to apply the bcm43xx init fix patch, which depends on this one, to the tree after this one got in? Or should I resend it?

Signature · 1 line

Greetings Michael.

Jiri Benc

2006-07-18 16:57 UTC #7 lore raw

In reply to Michael Buesch

Quoted 3 lines · Michael Buesch

This patch makes sleeping in the hw->config callback possible by removing the only atomic caller. The atomic caller was a timer and is replaced by a workqueue.

This is a modified version of the patch that doesn't use cancel_rearming_delayed_work.

Signed-off-by Jiri Benc <[email protected]>

---
 net/d80211/ieee80211.c       |   23 +++++++++++++++--------
 net/d80211/ieee80211_i.h     |    3 ++-
 net/d80211/ieee80211_iface.c |   10 ++++++++--
 net/d80211/ieee80211_sta.c   |   37 +++++++++++++++++--------------------
 4 files changed, 42 insertions(+), 31 deletions(-)

net/d80211/ieee80211.c +15 -8

@@ -4552,14 +4552,6 @@ void ieee80211_unregister_hw(struct net_
         tasklet_disable(&local->tasklet);
         /* TODO: skb_queue should be empty here, no need to do anything? */
 
-	if (local->rate_limit)
-		del_timer_sync(&local->rate_limit_timer);
-	if (local->stat_time)
-		del_timer_sync(&local->stat_timer);
-	if (local->scan_timer.data)
-		del_timer_sync(&local->scan_timer);
-	ieee80211_rx_bss_list_deinit(dev);
-
 	rtnl_lock();
 	local->reg_state = IEEE80211_DEV_UNREGISTERED;
 	if (local->apdev)

@@ -4572,6 +4564,21 @@ void ieee80211_unregister_hw(struct net_
 	}
 	rtnl_unlock();
 
+	if (local->rate_limit)
+		del_timer_sync(&local->rate_limit_timer);
+	if (local->stat_time)
+		del_timer_sync(&local->stat_timer);
+	if (local->scan_work.data) {
+		local->sta_scanning = 0;
+		cancel_delayed_work(&local->scan_work);
+		flush_scheduled_work();
+		/* The scan_work is guaranteed not to be called at this
+		 * point. It is not scheduled and not running now. It can be
+		 * scheduled again only by some sta_timer (all of them are
+		 * stopped by now) or under rtnl lock. */
+	}
+
+	ieee80211_rx_bss_list_deinit(dev);
 	ieee80211_clear_tx_pending(local);
 	sta_info_stop(local);
 	rate_control_remove_attrs(local, local->rate_ctrl_priv,

net/d80211/ieee80211_i.h +2 -1

@@ -17,6 +17,7 @@
 #include <linux/list.h>
 #include <linux/netdevice.h>
 #include <linux/skbuff.h>
+#include <linux/workqueue.h>
 #include "ieee80211_key.h"
 #include "sta_info.h"

@@ -430,7 +431,7 @@ struct ieee80211_local {
 	int scan_channel_idx;
 	enum { SCAN_SET_CHANNEL, SCAN_SEND_PROBE } scan_state;
 	unsigned long last_scan_completed;
-	struct timer_list scan_timer;
+	struct work_struct scan_work;
 	int scan_oper_channel;
 	int scan_oper_channel_val;
 	int scan_oper_power_level;

net/d80211/ieee80211_iface.c +8 -2

@@ -287,8 +287,14 @@ void ieee80211_if_reinit(struct net_devi
 	case IEEE80211_IF_TYPE_STA:
 	case IEEE80211_IF_TYPE_IBSS:
 		del_timer_sync(&sdata->u.sta.timer);
-		if (local->scan_timer.data == (unsigned long) sdata->dev)
-			del_timer_sync(&local->scan_timer);
+		if (local->scan_work.data == sdata->dev) {
+			local->sta_scanning = 0;
+			cancel_delayed_work(&local->scan_work);
+			flush_scheduled_work();
+			/* see comment in ieee80211_unregister_hw to
+			 * understand why this works */
+			local->scan_work.data = NULL;
+		}
 		kfree(sdata->u.sta.extra_ie);
 		sdata->u.sta.extra_ie = NULL;
 		kfree(sdata->u.sta.assocreq_ies);

net/d80211/ieee80211_sta.c +17 ~~-20~~

@@ -2417,15 +2417,16 @@ static int ieee80211_active_scan(struct 
 }
 
 
-static void ieee80211_sta_scan_timer(unsigned long ptr)
+static void ieee80211_sta_scan_work(void *ptr)
 {
-	struct net_device *dev = (struct net_device *) ptr;
+	struct net_device *dev = ptr;
 	struct ieee80211_local *local = dev->ieee80211_ptr;
         struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	struct ieee80211_hw_modes *mode;
 	struct ieee80211_channel *chan;
 	int skip;
 	union iwreq_data wrqu;
+	unsigned long next_delay = 0;
 
 	if (!local->sta_scanning)
 		return;

@@ -2493,31 +2494,30 @@ static void ieee80211_sta_scan_timer(uns
 			local->scan_channel_idx = 0;
 		}
 
-		if (skip) {
-			local->scan_timer.expires = jiffies;
+		if (skip)
 			break;
-		}
 
-		local->scan_timer.expires =
-			jiffies + IEEE80211_PROBE_DELAY +
-			usecs_to_jiffies(local->hw->channel_change_time);
+		next_delay = IEEE80211_PROBE_DELAY +
+			     usecs_to_jiffies(local->hw->channel_change_time);
 		local->scan_state = SCAN_SEND_PROBE;
 		break;
 	case SCAN_SEND_PROBE:
 		if (ieee80211_active_scan(local)) {
 			ieee80211_send_probe_req(dev, NULL, local->scan_ssid,
 						 local->scan_ssid_len);
-			local->scan_timer.expires =
-				jiffies + IEEE80211_CHANNEL_TIME;
-		} else {
-			local->scan_timer.expires =
-				jiffies + IEEE80211_PASSIVE_CHANNEL_TIME;
-		}
+			next_delay = IEEE80211_CHANNEL_TIME;
+		} else
+			next_delay = IEEE80211_PASSIVE_CHANNEL_TIME;
 		local->scan_state = SCAN_SET_CHANNEL;
 		break;
 	}
 
-	add_timer(&local->scan_timer);
+	if (local->sta_scanning) {
+		if (next_delay)
+			schedule_delayed_work(&local->scan_work, next_delay);
+		else
+			schedule_work(&local->scan_work);
+	}
 }

@@ -2566,11 +2566,8 @@ int ieee80211_sta_req_scan(struct net_de
 	local->scan_state = SCAN_SET_CHANNEL;
 	local->scan_hw_mode_idx = 0;
 	local->scan_channel_idx = 0;
-	init_timer(&local->scan_timer);
-	local->scan_timer.data = (unsigned long) dev;
-	local->scan_timer.function = ieee80211_sta_scan_timer;
-	local->scan_timer.expires = jiffies + 1;
-	add_timer(&local->scan_timer);
+	INIT_WORK(&local->scan_work, ieee80211_sta_scan_work, dev);
+	schedule_work(&local->scan_work);
 
 	return 0;
 }

Signature · 2 lines

Jiri Benc
SUSE Labs

A message that is not in the archive (sent off-list or not yet received): [email protected]

Michael Buesch

2006-07-18 17:36 UTC #9 lore raw

Quoted 38 lines · Jiri Benc

Quoted 3 lines · Michael Buesch

This patch makes sleeping in the hw->config callback possible by removing the only atomic caller. The atomic caller was a timer and is replaced by a workqueue.

This is a modified version of the patch that doesn't use cancel_rearming_delayed_work.

Signed-off-by Jiri Benc <[email protected]>

---
 net/d80211/ieee80211.c       |   23 +++++++++++++++--------
 net/d80211/ieee80211_i.h     |    3 ++-
 net/d80211/ieee80211_iface.c |   10 ++++++++--
 net/d80211/ieee80211_sta.c   |   37 +++++++++++++++++--------------------
 4 files changed, 42 insertions(+), 31 deletions(-)

net/d80211/ieee80211.c +0 -8

@@ -4552,14 +4552,6 @@ void ieee80211_unregister_hw(struct net_
         tasklet_disable(&local->tasklet);
         /* TODO: skb_queue should be empty here, no need to do anything? */
 
-	if (local->rate_limit)
-		del_timer_sync(&local->rate_limit_timer);
-	if (local->stat_time)
-		del_timer_sync(&local->stat_timer);
-	if (local->scan_timer.data)
-		del_timer_sync(&local->scan_timer);
-	ieee80211_rx_bss_list_deinit(dev);
-
 	rtnl_lock();
 	local->reg_state = IEEE80211_DEV_UNREGISTERED;
 	if (local->apdev)

@@ -4572,6 +4564,21 @@ void ieee80211_unregister_hw(struct net_
 	}
 	rtnl_unlock();

net/d80211/ieee80211.c void ieee80211_unregister_hw(struct net_ · line 4576

Quoted 10 lines

net/d80211/ieee80211.c +10 -0

+	if (local->rate_limit)
+		del_timer_sync(&local->rate_limit_timer);
+	if (local->stat_time)
+		del_timer_sync(&local->stat_timer);
+	if (local->scan_work.data) {
+		local->sta_scanning = 0;
+		cancel_delayed_work(&local->scan_work);
+		flush_scheduled_work();
+		/* The scan_work is guaranteed not to be called at this
+		 * point. It is not scheduled and not running now. It can be

If it is guaranteed to be not to be called, the cancel_delayed_work(&local->scan_work);
is unnecessary.
If it is possible to be scheduled and delayed here, it's a bug.

So, if the first is true, we should remove it and if the second is true, well :)

The flush_scheduled_work() call is necessary, though.

Signature · 1 line

Greetings Michael.

Michael Buesch

author

2006-07-18 17:59 UTC #10 lore raw

Quoted 56 lines · Michael Buesch

Quoted 38 lines · Jiri Benc
Quoted 3 lines · Michael Buesch
This patch makes sleeping in the hw->config callback possible by removing the only atomic caller. The atomic caller was a timer and is replaced by a workqueue.
This is a modified version of the patch that doesn't use cancel_rearming_delayed_work.
Signed-off-by Jiri Benc <[email protected]>
---
 net/d80211/ieee80211.c       |   23 +++++++++++++++--------
 net/d80211/ieee80211_i.h     |    3 ++-
 net/d80211/ieee80211_iface.c |   10 ++++++++--
 net/d80211/ieee80211_sta.c   |   37 +++++++++++++++++--------------------
 4 files changed, 42 insertions(+), 31 deletions(-)
net/d80211/ieee80211.c +0 -8
@@ -4552,14 +4552,6 @@ void ieee80211_unregister_hw(struct net_
         tasklet_disable(&local->tasklet);
         /* TODO: skb_queue should be empty here, no need to do anything? */
 
-	if (local->rate_limit)
-		del_timer_sync(&local->rate_limit_timer);
-	if (local->stat_time)
-		del_timer_sync(&local->stat_timer);
-	if (local->scan_timer.data)
-		del_timer_sync(&local->scan_timer);
-	ieee80211_rx_bss_list_deinit(dev);
-
 	rtnl_lock();
 	local->reg_state = IEEE80211_DEV_UNREGISTERED;
 	if (local->apdev)
@@ -4572,6 +4564,21 @@ void ieee80211_unregister_hw(struct net_
 	}
 	rtnl_unlock();
 
net/d80211/ieee80211.c void ieee80211_unregister_hw(struct net_ · line 4576
Quoted 10 lines
net/d80211/ieee80211.c +10 -0
+	if (local->rate_limit)
+		del_timer_sync(&local->rate_limit_timer);
+	if (local->stat_time)
+		del_timer_sync(&local->stat_timer);
+	if (local->scan_work.data) {
+		local->sta_scanning = 0;
+		cancel_delayed_work(&local->scan_work);
+		flush_scheduled_work();
+		/* The scan_work is guaranteed not to be called at this
+		 * point. It is not scheduled and not running now. It can be
If it is guaranteed to be not to be called, the cancel_delayed_work(&local->scan_work);
is unnecessary.
If it is possible to be scheduled and delayed here, it's a bug.

Quoted 4 lines

So, if the first is true, we should remove it and if the second is true, well :)
The flush_scheduled_work() call is necessary, though.

Oh, no. I misread the code.
I think it's ok. Sorry for the noise :(

Signature · 1 line

Greetings Michael.