rt2x00: Fix lock dependency errror

PATCH In review Ivo van Doorn 2 participants 4 messages Jun 25, 2008 Networking drivers (wireless)

Share & tools

Ivo van Doorn

author

2008-06-25 19:25 UTC #0 lore raw

PATCH 1/1 rt2x00: Fix lock dependency errror

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 <[email protected]>

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

drivers/net/wireless/rt2x00/rt2x00.h +1 -0

index 611d983..73dacbb 100644
@@ -821,6 +822,7 @@ struct rt2x00_dev {
 	/*
 	 * Scheduled work.
 	 */
+	struct workqueue_struct *workqueue;
 	struct work_struct intf_work;
 	struct work_struct filter_work;

drivers/net/wireless/rt2x00/rt2x00dev.c +25 ~~-13~~

index 2673d56..f836de1 100644
@@ -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);

drivers/net/wireless/rt2x00/rt2x00mac.c +2 -2

index 87e280a..48c7ce3 100644
@@ -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);
 }

Johannes Berg

maintainer

2008-06-25 19:34 UTC #1 lore raw

drivers/net/wireless/rt2x00/rt2x00.h struct rt2x00_dev { · line 825

Quoted 10 lines

drivers/net/wireless/rt2x00/rt2x00.h +1 -0

index 611d983..73dacbb 100644
@@ -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

Ivo Van Doorn

author

2008-06-25 20:18 UTC #2 lore raw

Quoted 5 lines

Quoted 5 lines
drivers/net/wireless/rt2x00/rt2x00.h +0 -0
index 611d983..73dacbb 100644
@@ -821,6 +822,7 @@ struct rt2x00_dev {

Quoted 8 lines

drivers/net/wireless/rt2x00/rt2x00.h struct rt2x00_dev { · line 825
Quoted 4 lines
drivers/net/wireless/rt2x00/rt2x00.h +1 -0
      /*
       * 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

Johannes Berg

maintainer

2008-06-25 20:21 UTC #3 lore raw

Quoted 14 lines · Ivo Van Doorn

Quoted 5 lines
Quoted 5 lines
drivers/net/wireless/rt2x00/rt2x00.h +0 -0
index 611d983..73dacbb 100644
@@ -821,6 +822,7 @@ struct rt2x00_dev {
Quoted 8 lines
drivers/net/wireless/rt2x00/rt2x00.h struct rt2x00_dev { · line 825
Quoted 4 lines
drivers/net/wireless/rt2x00/rt2x00.h +1 -0
      /*
       * 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?

Quoted 1 line

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