[PATCH] rt2x00: Fix lock dependency errror

[PATCH] rt2x00: Fix lock dependency errror

[Ivo van Doorn]

This fixes a circular locking dependency in the workqueue handling.
The interface work task uses the mac80211 function
ieee80211_iterate_active_interfaces() which grabs the RTNL lock.

However when the interface is brough down, this happens under the RTNL
lock as well, this causes problems because mac80211 will flush the workqueue
during the ifdown event. This causes mac80211 to wait until the driver has
completed all work which can't finish because it is waiting on the RTNL lock.

This is fixed by moving rt2x00 workqueue tasks on a different workqueue,
this workqueue can be flushed when the ieee80211_hw structure is removed
by the driver (when the driver is unloaded) which does not happen under the
RTNL lock.

Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>
---
John, please queue for 2.6.26 this fixes the locking dependency as mentioned
in Kernel bug report: http://bugzilla.kernel.org/show_bug.cgi?id=10821

diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
index 611d983..73dacbb 100644
--- a/drivers/net/wireless/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/rt2x00/rt2x00.h
@@ -821,6 +822,7 @@ struct rt2x00_dev {
 	/*
 	 * Scheduled work.
 	 */
+	struct workqueue_struct *workqueue;
 	struct work_struct intf_work;
 	struct work_struct filter_work;
 
diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index 2673d56..f836de1 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -75,7 +75,7 @@ static void rt2x00lib_start_link_tuner(struct rt2x00_dev *rt2x00dev)
 
 	rt2x00lib_reset_link_tuner(rt2x00dev);
 
-	queue_delayed_work(rt2x00dev->hw->workqueue,
+	queue_delayed_work(rt2x00dev->workqueue,
 			   &rt2x00dev->link.work, LINK_TUNE_INTERVAL);
 }
 
@@ -137,14 +137,6 @@ void rt2x00lib_disable_radio(struct rt2x00_dev *rt2x00dev)
 		return;
 
 	/*
-	 * Stop all scheduled work.
-	 */
-	if (work_pending(&rt2x00dev->intf_work))
-		cancel_work_sync(&rt2x00dev->intf_work);
-	if (work_pending(&rt2x00dev->filter_work))
-		cancel_work_sync(&rt2x00dev->filter_work);
-
-	/*
 	 * Stop the TX queues.
 	 */
 	ieee80211_stop_queues(rt2x00dev->hw);
@@ -398,8 +390,8 @@ static void rt2x00lib_link_tuner(struct work_struct *work)
 	 * Increase tuner counter, and reschedule the next link tuner run.
 	 */
 	rt2x00dev->link.count++;
-	queue_delayed_work(rt2x00dev->hw->workqueue, &rt2x00dev->link.work,
-			   LINK_TUNE_INTERVAL);
+	queue_delayed_work(rt2x00dev->workqueue,
+			   &rt2x00dev->link.work, LINK_TUNE_INTERVAL);
 }
 
 static void rt2x00lib_packetfilter_scheduled(struct work_struct *work)
@@ -433,6 +425,15 @@ static void rt2x00lib_intf_scheduled_iter(void *data, u8 *mac,
 
 	spin_unlock(&intf->lock);
 
+	/*
+	 * It is possible the radio was disabled while the work had been
+	 * scheduled. If that happens we should return here immediately,
+	 * note that in the spinlock protected area above the delayed_flags
+	 * have been cleared correctly.
+	 */
+	if (!test_bit(DEVICE_ENABLED_RADIO, &rt2x00dev->flags))
+		return;
+
 	if (delayed_flags & DELAYED_UPDATE_BEACON) {
 		skb = ieee80211_beacon_get(rt2x00dev->hw, vif, &control);
 		if (skb && rt2x00dev->ops->hw->beacon_update(rt2x00dev->hw,
@@ -441,7 +442,7 @@ static void rt2x00lib_intf_scheduled_iter(void *data, u8 *mac,
 	}
 
 	if (delayed_flags & DELAYED_CONFIG_ERP)
-		rt2x00lib_config_erp(rt2x00dev, intf, &intf->conf);
+		rt2x00lib_config_erp(rt2x00dev, intf, &conf);
 
 	if (delayed_flags & DELAYED_LED_ASSOC)
 		rt2x00leds_led_assoc(rt2x00dev, !!rt2x00dev->intf_associated);
@@ -487,7 +488,7 @@ void rt2x00lib_beacondone(struct rt2x00_dev *rt2x00dev)
 						   rt2x00lib_beacondone_iter,
 						   rt2x00dev);
 
-	queue_work(rt2x00dev->hw->workqueue, &rt2x00dev->intf_work);
+	queue_work(rt2x00dev->workqueue, &rt2x00dev->intf_work);
 }
 EXPORT_SYMBOL_GPL(rt2x00lib_beacondone);
 
@@ -1130,6 +1136,10 @@ int rt2x00lib_probe_dev(struct rt2x00_dev *rt2x00dev)
 	/*
 	 * Initialize configuration work.
 	 */
+	rt2x00dev->workqueue = create_singlethread_workqueue("rt2x00lib");
+	if (!rt2x00dev->workqueue)
+		goto exit;
+
 	INIT_WORK(&rt2x00dev->intf_work, rt2x00lib_intf_scheduled);
 	INIT_WORK(&rt2x00dev->filter_work, rt2x00lib_packetfilter_scheduled);
 	INIT_DELAYED_WORK(&rt2x00dev->link.work, rt2x00lib_link_tuner);
@@ -1190,6 +1200,13 @@ void rt2x00lib_remove_dev(struct rt2x00_dev *rt2x00dev)
 	rt2x00leds_unregister(rt2x00dev);
 
 	/*
+	 * Stop all queued work. Note that most tasks will already be halted
+	 * during rt2x00lib_disable_radio() and rt2x00lib_uninitialize().
+	 */
+	flush_workqueue(rt2x00dev->workqueue);
+	destroy_workqueue(rt2x00dev->workqueue);
+
+	/*
 	 * Free ieee80211_hw memory.
 	 */
 	rt2x00lib_remove_hw(rt2x00dev);
diff --git a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c
index 87e280a..48c7ce3 100644
--- a/drivers/net/wireless/rt2x00/rt2x00mac.c
+++ b/drivers/net/wireless/rt2x00/rt2x00mac.c
@@ -428,7 +428,7 @@ void rt2x00mac_configure_filter(struct ieee80211_hw *hw,
 	if (!test_bit(DRIVER_REQUIRE_SCHEDULED, &rt2x00dev->flags))
 		rt2x00dev->ops->lib->config_filter(rt2x00dev, *total_flags);
 	else
-		queue_work(rt2x00dev->hw->workqueue, &rt2x00dev->filter_work);
+		queue_work(rt2x00dev->workqueue, &rt2x00dev->filter_work);
 }
 EXPORT_SYMBOL_GPL(rt2x00mac_configure_filter);
 
@@ -509,7 +509,7 @@ void rt2x00mac_bss_info_changed(struct ieee80211_hw *hw,
 	memcpy(&intf->conf, bss_conf, sizeof(*bss_conf));
 	if (delayed) {
 		intf->delayed_flags |= delayed;
-		queue_work(rt2x00dev->hw->workqueue, &rt2x00dev->intf_work);
+		queue_work(rt2x00dev->workqueue, &rt2x00dev->intf_work);
 	}
 	spin_unlock(&intf->lock);
 }

Re: [PATCH] rt2x00: Fix lock dependency errror

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]


> diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
> index 611d983..73dacbb 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00.h
> @@ -821,6 +822,7 @@ struct rt2x00_dev {
>  	/*
>  	 * Scheduled work.
>  	 */
> +	struct workqueue_struct *workqueue;

Do you really need a whole workqueue (kernel thread) per device? I would
think that using schedule_work / cancel_work_sync for this task and keep
using the mac80211 wq would be sufficient?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] rt2x00: Fix lock dependency errror

[Ivo Van Doorn]

>> diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
>> index 611d983..73dacbb 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00.h
>> +++ b/drivers/net/wireless/rt2x00/rt2x00.h
>> @@ -821,6 +822,7 @@ struct rt2x00_dev {
>>       /*
>>        * Scheduled work.
>>        */
>> +     struct workqueue_struct *workqueue;
>
> Do you really need a whole workqueue (kernel thread) per device? I would
> think that using schedule_work / cancel_work_sync for this task and keep
> using the mac80211 wq would be sufficient?

True, but this is a quick patch to resolve the bug for sure. I am going to look
into the workqueue handling more closely to see what the impact on splitting
the different work structure over different workqueues is later. I *think* I at
one time made use the assumption that work structures on the same queue
don't run concurrently and I really need to look into that first to see if there
isn't some other locking problem that will appear when I split the work over
2 different queues.

At the moment I am also concerned about the link tuner who runs in the
workqueue and if there isn't any negative side effect regarding the early
flush_workqueue() in mac80211.

Ivo

Re: [PATCH] rt2x00: Fix lock dependency errror

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

On Wed, 2008-06-25 at 22:18 +0200, Ivo Van Doorn wrote:
> >> diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
> >> index 611d983..73dacbb 100644
> >> --- a/drivers/net/wireless/rt2x00/rt2x00.h
> >> +++ b/drivers/net/wireless/rt2x00/rt2x00.h
> >> @@ -821,6 +822,7 @@ struct rt2x00_dev {
> >>       /*
> >>        * Scheduled work.
> >>        */
> >> +     struct workqueue_struct *workqueue;
> >
> > Do you really need a whole workqueue (kernel thread) per device? I would
> > think that using schedule_work / cancel_work_sync for this task and keep
> > using the mac80211 wq would be sufficient?
> 
> True, but this is a quick patch to resolve the bug for sure.

Fair enough, this is a fix for 2.6.26 after all.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]