Re: [RFC] mac80211: fix resuming when device is gone

[Stanislaw Gruszka <sgruszka@redhat.com>]

On Tue, Aug 09, 2011 at 01:47:43PM +0200, Johannes Berg wrote:
> On Tue, 2011-08-09 at 13:46 +0200, Johannes Berg wrote:
> > On Tue, 2011-08-09 at 13:45 +0200, Johannes Berg wrote:
> > > On Tue, 2011-08-09 at 13:43 +0200, Johannes Berg wrote:
> > > 
> > > > > Yes, but __ieee80211_resume() calls directly ieee80211_reconfig().
> > > > 
> > > > Sure. Just trying to make sure I understand where it's coming from. So
> > > > wiphy_unregister() is still running, before device_del(), but then we
> > > > get the resume from the core...
> > > > 
> > > > Shouldn't we implement this logic you just did in cfg80211 instead?
> > > 
> > > Although I guess until wiphy_unregister() returns, drivers must still
> > > expect callbacks, which clearly mac80211 didn't handle correctly here
> > > with the timer...
> > 
> > Actually...
> > 
> > Doesn't that just mean we need to reorder the code in _free_hw()?
> 
> I mean ieee80211_unregister_hw().
> 
> >  If we
> > do the timer stuff after wiphy_unregister(), we should be OK, and
> > prevent other, similar problems in the future too.

There are some debugfs dependencies, so moving wiphy_unregister() before
del_timer_sync(&local->work_timer); in ieee80211_unregister_hw() gave me
a crash. I moved sta_info_stop(local); after
wiphy_unregister(local->hw.wiphy); but this alone do not solve the
problem, ->resume callback can be called after wiphy_unregister().
Below patch fixes the problem (it also protect ->suspend, in case device
gone during suspend process):

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 866f269..acb4423 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1012,7 +1012,6 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 	cancel_work_sync(&local->reconfig_filter);
 
 	ieee80211_clear_tx_pending(local);
-	sta_info_stop(local);
 	rate_control_deinitialize(local);
 
 	if (skb_queue_len(&local->skb_queue) ||
@@ -1024,6 +1023,7 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 
 	destroy_workqueue(local->workqueue);
 	wiphy_unregister(local->hw.wiphy);
+	sta_info_stop(local);
 	ieee80211_wep_free(local);
 	ieee80211_led_exit(local);
 	kfree(local->int_scan_req);
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 645437c..453fcc6 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -616,6 +616,9 @@ int wiphy_register(struct wiphy *wiphy)
 	if (res)
 		goto out_rm_dev;
 
+	rtnl_lock();
+	rdev->wiphy.registered = true;
+	rtnl_unlock();
 	return 0;
 
 out_rm_dev:
@@ -647,6 +650,10 @@ void wiphy_unregister(struct wiphy *wiphy)
 {
 	struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
 
+	rtnl_lock();
+	rdev->wiphy.registered = false;
+	rtnl_unlock();
+
 	rfkill_unregister(rdev->rfkill);
 
 	/* protect the device list */
@@ -690,6 +697,7 @@ void wiphy_unregister(struct wiphy *wiphy)
 	reg_device_remove(wiphy);
 
 	cfg80211_rdev_list_generation++;
+	rdev->wiphy.registered = false;
 	device_del(&rdev->wiphy.dev);
 
 	mutex_unlock(&cfg80211_mutex);
diff --git a/net/wireless/sysfs.c b/net/wireless/sysfs.c
index c6e4ca6..107516b 100644
--- a/net/wireless/sysfs.c
+++ b/net/wireless/sysfs.c
@@ -93,7 +93,10 @@ static int wiphy_suspend(struct device *dev, pm_message_t state)
 
 	if (rdev->ops->suspend) {
 		rtnl_lock();
-		ret = rdev->ops->suspend(&rdev->wiphy, rdev->wowlan);
+		if (rdev->wiphy.registered)
+			ret = rdev->ops->suspend(&rdev->wiphy, rdev->wowlan);
+		else
+			ret = -ENODEV;
 		rtnl_unlock();
 	}
 
@@ -112,7 +115,10 @@ static int wiphy_resume(struct device *dev)
 
 	if (rdev->ops->resume) {
 		rtnl_lock();
-		ret = rdev->ops->resume(&rdev->wiphy);
+		if (rdev->wiphy.registered)
+			ret = rdev->ops->resume(&rdev->wiphy);
+		else
+			ret = -ENODEV;
 		rtnl_unlock();
 	}