Stanislaw,
I looked at this and I think we should do a bit more restructuring.
The virtual monitor is unsafe no matter how you look at it, even
with your second patch, due to channel contexts. I therefore opted
to just destroy it unconditionally on suspend, and restore it when
resuming, which has pretty much the same effect to the driver but
handles channel contexts differently. As we disconnect etc. in all
other interface types (*), channel contexts should thus be fully
destroyed on suspend. I should probably add a WARN_ON() for that
somewhere ...
I've also restructured the do_stop() function itself so it doesn't
get "if (local->suspended)" checks all over but has it at just a
single point at the end of the function before doing all driver
updates.
I think this is a good compromise as it makes it obvious that the
driver updates happen last, and only conditionally. It remains a
tricky proposition to make sure all other state (like chanctx) is
actually destroyed correctly, but right now that should indeed be
the case.
No doubt I've made some mistakes here, but as I don't own any USB
devices any more (they all broke) I'd appreciate some testing.
(*) the stupid one is WDS -- for it we still have the station and
key removal in the do_stop() code. We really should kill that
but I don't know how we could do it better, preferably in a
backward compatible way ...
johannes
From: Johannes Berg <[email protected]>
When a device is unplugged while suspended, mac80211 is
de-initialized and all interfaces are removed while no
state is actually present in the driver. This can cause
warnings and driver confusion.
Fix this by reordering the do_stop code to not call the
driver when it is suspended, i.e. when there's no state
in the driver anyway.
The previous patches removed a few corner cases in ROC
and virtual monitor interfaces so that now this is safe
to do and no state should be left over.
Reported-by: Stanislaw Gruszka <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/iface.c | 66 +++++++++++++++++++++++++++++++++++-----------------
1 file changed, 45 insertions(+), 21 deletions(-)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 5ec9c02..f54e72f 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -760,8 +760,6 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
sdata->dev->addr_len);
spin_unlock_bh(&local->filter_lock);
netif_addr_unlock_bh(sdata->dev);
-
- ieee80211_configure_filter(local);
}
del_timer_sync(&local->dynamic_ps_timer);
@@ -772,6 +770,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
if (sdata->wdev.cac_started) {
+ WARN_ON(local->suspended);
mutex_lock(&local->iflist_mtx);
ieee80211_vif_release_channel(sdata);
mutex_unlock(&local->iflist_mtx);
@@ -822,14 +821,9 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
if (local->monitors == 0) {
local->hw.conf.flags &= ~IEEE80211_CONF_MONITOR;
hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
- ieee80211_del_virtual_monitor(local);
}
ieee80211_adjust_monitor_flags(sdata, -1);
- ieee80211_configure_filter(local);
- mutex_lock(&local->mtx);
- ieee80211_recalc_idle(local);
- mutex_unlock(&local->mtx);
break;
case NL80211_IFTYPE_P2P_DEVICE:
/* relies on synchronize_rcu() below */
@@ -859,13 +853,53 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
/* fall through */
case NL80211_IFTYPE_AP:
skb_queue_purge(&sdata->skb_queue);
+ }
+
+ sdata->bss = NULL;
+
+ spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
+ for (i = 0; i < IEEE80211_MAX_QUEUES; i++) {
+ skb_queue_walk_safe(&local->pending[i], skb, tmp) {
+ struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+ if (info->control.vif == &sdata->vif) {
+ __skb_unlink(skb, &local->pending[i]);
+ ieee80211_free_txskb(&local->hw, skb);
+ }
+ }
+ }
+ spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
+
+ /*
+ * If the interface goes down while suspended, presumably because
+ * the device was unplugged and that happens before our resume,
+ * then the driver is already unconfigured and the remainder of
+ * this function isn't needed.
+ * XXX: what about WoWLAN? If the device has software state, e.g.
+ * memory allocated, it might expect teardown commands from
+ * mac80211 here?
+ */
+ if (local->suspended) {
+ WARN_ON(local->wowlan);
+ WARN_ON(rtnl_dereference(local->monitor_sdata));
+ return;
+ }
+ switch (sdata->vif.type) {
+ case NL80211_IFTYPE_AP_VLAN:
+ break;
+ case NL80211_IFTYPE_MONITOR:
+ if (local->monitors == 0)
+ ieee80211_del_virtual_monitor(local);
+
+ mutex_lock(&local->mtx);
+ ieee80211_recalc_idle(local);
+ mutex_unlock(&local->mtx);
+ break;
+ default:
if (going_down)
drv_remove_interface(local, sdata);
}
- sdata->bss = NULL;
-
ieee80211_recalc_ps(local, -1);
if (local->open_count == 0) {
@@ -877,20 +911,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
}
/* do after stop to avoid reconfiguring when we stop anyway */
- if (hw_reconf_flags)
+ if (hw_reconf_flags) {
+ ieee80211_configure_filter(local);
ieee80211_hw_config(local, hw_reconf_flags);
-
- spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
- for (i = 0; i < IEEE80211_MAX_QUEUES; i++) {
- skb_queue_walk_safe(&local->pending[i], skb, tmp) {
- struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
- if (info->control.vif == &sdata->vif) {
- __skb_unlink(skb, &local->pending[i]);
- ieee80211_free_txskb(&local->hw, skb);
- }
- }
}
- spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
if (local->monitors == local->open_count && local->monitors > 0)
ieee80211_add_virtual_monitor(local);
--
1.8.0
From: Johannes Berg <[email protected]>
It has to be removed from the driver, but completely
destroying it helps handle unplug of a device during
suspend since then the channel context handling etc.
doesn't have to happen later when it's removed.
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/ieee80211_i.h | 2 ++
net/mac80211/iface.c | 4 ++--
net/mac80211/pm.c | 6 ++----
net/mac80211/util.c | 5 +++++
4 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 84fa129..f4a65a3 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1335,6 +1335,8 @@ void ieee80211_adjust_monitor_flags(struct ieee80211_sub_if_data *sdata,
const int offset);
int ieee80211_do_open(struct wireless_dev *wdev, bool coming_up);
void ieee80211_sdata_stop(struct ieee80211_sub_if_data *sdata);
+int ieee80211_add_virtual_monitor(struct ieee80211_local *local);
+void ieee80211_del_virtual_monitor(struct ieee80211_local *local);
bool __ieee80211_recalc_txpower(struct ieee80211_sub_if_data *sdata);
void ieee80211_recalc_txpower(struct ieee80211_sub_if_data *sdata);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index aadb471..5ec9c02 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -346,7 +346,7 @@ static void ieee80211_set_default_queues(struct ieee80211_sub_if_data *sdata)
sdata->vif.cab_queue = IEEE80211_INVAL_HW_QUEUE;
}
-static int ieee80211_add_virtual_monitor(struct ieee80211_local *local)
+int ieee80211_add_virtual_monitor(struct ieee80211_local *local)
{
struct ieee80211_sub_if_data *sdata;
int ret = 0;
@@ -400,7 +400,7 @@ static int ieee80211_add_virtual_monitor(struct ieee80211_local *local)
return ret;
}
-static void ieee80211_del_virtual_monitor(struct ieee80211_local *local)
+void ieee80211_del_virtual_monitor(struct ieee80211_local *local)
{
struct ieee80211_sub_if_data *sdata;
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index b98d927..d1c021b 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -21,6 +21,8 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
ieee80211_roc_purge(local, NULL);
+ ieee80211_del_virtual_monitor(local);
+
if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
mutex_lock(&local->sta_mtx);
list_for_each_entry(sta, &local->sta_list, list) {
@@ -103,10 +105,6 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
drv_remove_interface(local, sdata);
}
- sdata = rtnl_dereference(local->monitor_sdata);
- if (sdata)
- drv_remove_interface(local, sdata);
-
/*
* We disconnected on all interfaces before suspend, all channel
* contexts should be released.
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 2a18d70..23bbfe0 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1504,6 +1504,8 @@ int ieee80211_reconfig(struct ieee80211_local *local)
/* add interfaces */
sdata = rtnl_dereference(local->monitor_sdata);
if (sdata) {
+ /* in HW restart it exists already */
+ WARN_ON(local->resuming);
res = drv_add_interface(local, sdata);
if (WARN_ON(res)) {
rcu_assign_pointer(local->monitor_sdata, NULL);
@@ -1693,6 +1695,9 @@ int ieee80211_reconfig(struct ieee80211_local *local)
local->in_reconfig = false;
barrier();
+ if (local->monitors == local->open_count && local->monitors > 0)
+ ieee80211_add_virtual_monitor(local);
+
/*
* Clear the WLAN_STA_BLOCK_BA flag so new aggregation
* sessions can be established after a resume.
--
1.8.0
On Wed, Mar 27, 2013 at 11:33:19PM +0100, Johannes Berg wrote:
> I looked at this and I think we should do a bit more restructuring.
> The virtual monitor is unsafe no matter how you look at it, even
> with your second patch, due to channel contexts. I therefore opted
> to just destroy it unconditionally on suspend, and restore it when
> resuming, which has pretty much the same effect to the driver but
> handles channel contexts differently. As we disconnect etc. in all
> other interface types (*), channel contexts should thus be fully
> destroyed on suspend. I should probably add a WARN_ON() for that
> somewhere ...
>
> I've also restructured the do_stop() function itself so it doesn't
> get "if (local->suspended)" checks all over but has it at just a
> single point at the end of the function before doing all driver
> updates.
>
> I think this is a good compromise as it makes it obvious that the
> driver updates happen last, and only conditionally. It remains a
> tricky proposition to make sure all other state (like chanctx) is
> actually destroyed correctly, but right now that should indeed be
> the case.
>
> No doubt I've made some mistakes here, but as I don't own any USB
> devices any more (they all broke) I'd appreciate some testing.
Just tested (with 3/3 v2 patch) and it fixes the test case I have,
that I used to create my patches. Test case is doing suspend with
reverted commit 761ce8c41ed20ee3af77f2df527edc3f92e6f3bf. This make
device is automatically removed/added during suspend, because lack
of .reset_resume callback.
Than I tested with physical remove device from USB slot while machine
is suspend. This gives warnings in ieee80211_reconfig() and
ieee80211_stop(). I'm not sure why, I'm going to look at that, but
I think this patch set should be applied and some more fixes provided
on top of it.
Stanislaw
From: Johannes Berg <[email protected]>
They can't really be executed while suspended and could
trigger work warnings, so abort all ROC items. When the
system resumes the notifications about this will be
delivered to userspace which can then act accordingly
(though it will assume they were canceled/finished.)
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/ieee80211_i.h | 3 ++-
net/mac80211/iface.c | 2 +-
net/mac80211/offchannel.c | 6 +++---
net/mac80211/pm.c | 2 ++
4 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 99fe4ce..84fa129 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1314,7 +1314,8 @@ void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local);
void ieee80211_offchannel_return(struct ieee80211_local *local);
void ieee80211_roc_setup(struct ieee80211_local *local);
void ieee80211_start_next_roc(struct ieee80211_local *local);
-void ieee80211_roc_purge(struct ieee80211_sub_if_data *sdata);
+void ieee80211_roc_purge(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata);
void ieee80211_roc_notify_destroy(struct ieee80211_roc_work *roc);
void ieee80211_sw_roc_work(struct work_struct *work);
void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index e995419..aadb471 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -714,7 +714,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
if (sdata->dev)
netif_tx_stop_all_queues(sdata->dev);
- ieee80211_roc_purge(sdata);
+ ieee80211_roc_purge(local, sdata);
if (sdata->vif.type == NL80211_IFTYPE_STATION)
ieee80211_mgd_stop(sdata);
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index b01eb73..e19d6cf 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -436,15 +436,15 @@ void ieee80211_roc_setup(struct ieee80211_local *local)
INIT_LIST_HEAD(&local->roc_list);
}
-void ieee80211_roc_purge(struct ieee80211_sub_if_data *sdata)
+void ieee80211_roc_purge(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata)
{
- struct ieee80211_local *local = sdata->local;
struct ieee80211_roc_work *roc, *tmp;
LIST_HEAD(tmp_list);
mutex_lock(&local->mtx);
list_for_each_entry_safe(roc, tmp, &local->roc_list, list) {
- if (roc->sdata != sdata)
+ if (sdata && roc->sdata != sdata)
continue;
if (roc->started && local->ops->remain_on_channel) {
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index 3d16f4e..b98d927 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -19,6 +19,8 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
ieee80211_dfs_cac_cancel(local);
+ ieee80211_roc_purge(local, NULL);
+
if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
mutex_lock(&local->sta_mtx);
list_for_each_entry(sta, &local->sta_list, list) {
--
1.8.0
From: Johannes Berg <[email protected]>
When a device is unplugged while suspended, mac80211 is
de-initialized and all interfaces are removed while no
state is actually present in the driver. This can cause
warnings and driver confusion.
Fix this by reordering the do_stop code to not call the
driver when it is suspended, i.e. when there's no state
in the driver anyway.
The previous patches removed a few corner cases in ROC
and virtual monitor interfaces so that now this is safe
to do and no state should be left over.
Reported-by: Stanislaw Gruszka <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/iface.c | 72 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 48 insertions(+), 24 deletions(-)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 5ec9c02..3591c4c 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -760,8 +760,6 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
sdata->dev->addr_len);
spin_unlock_bh(&local->filter_lock);
netif_addr_unlock_bh(sdata->dev);
-
- ieee80211_configure_filter(local);
}
del_timer_sync(&local->dynamic_ps_timer);
@@ -772,6 +770,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
if (sdata->wdev.cac_started) {
+ WARN_ON(local->suspended);
mutex_lock(&local->iflist_mtx);
ieee80211_vif_release_channel(sdata);
mutex_unlock(&local->iflist_mtx);
@@ -822,14 +821,9 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
if (local->monitors == 0) {
local->hw.conf.flags &= ~IEEE80211_CONF_MONITOR;
hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
- ieee80211_del_virtual_monitor(local);
}
ieee80211_adjust_monitor_flags(sdata, -1);
- ieee80211_configure_filter(local);
- mutex_lock(&local->mtx);
- ieee80211_recalc_idle(local);
- mutex_unlock(&local->mtx);
break;
case NL80211_IFTYPE_P2P_DEVICE:
/* relies on synchronize_rcu() below */
@@ -859,27 +853,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
/* fall through */
case NL80211_IFTYPE_AP:
skb_queue_purge(&sdata->skb_queue);
-
- if (going_down)
- drv_remove_interface(local, sdata);
}
sdata->bss = NULL;
- ieee80211_recalc_ps(local, -1);
-
- if (local->open_count == 0) {
- ieee80211_clear_tx_pending(local);
- ieee80211_stop_device(local);
-
- /* no reconfiguring after stop! */
- hw_reconf_flags = 0;
- }
-
- /* do after stop to avoid reconfiguring when we stop anyway */
- if (hw_reconf_flags)
- ieee80211_hw_config(local, hw_reconf_flags);
-
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
for (i = 0; i < IEEE80211_MAX_QUEUES; i++) {
skb_queue_walk_safe(&local->pending[i], skb, tmp) {
@@ -892,6 +869,53 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
}
spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
+ if (local->open_count == 0)
+ ieee80211_clear_tx_pending(local);
+
+ /*
+ * If the interface goes down while suspended, presumably because
+ * the device was unplugged and that happens before our resume,
+ * then the driver is already unconfigured and the remainder of
+ * this function isn't needed.
+ * XXX: what about WoWLAN? If the device has software state, e.g.
+ * memory allocated, it might expect teardown commands from
+ * mac80211 here?
+ */
+ if (local->suspended) {
+ WARN_ON(local->wowlan);
+ WARN_ON(rtnl_dereference(local->monitor_sdata));
+ return;
+ }
+
+ switch (sdata->vif.type) {
+ case NL80211_IFTYPE_AP_VLAN:
+ break;
+ case NL80211_IFTYPE_MONITOR:
+ if (local->monitors == 0)
+ ieee80211_del_virtual_monitor(local);
+
+ mutex_lock(&local->mtx);
+ ieee80211_recalc_idle(local);
+ mutex_unlock(&local->mtx);
+ break;
+ default:
+ if (going_down)
+ drv_remove_interface(local, sdata);
+ }
+
+ ieee80211_recalc_ps(local, -1);
+
+ if (local->open_count == 0) {
+ ieee80211_stop_device(local);
+
+ /* no reconfiguring after stop! */
+ return;
+ }
+
+ /* do after stop to avoid reconfiguring when we stop anyway */
+ ieee80211_configure_filter(local);
+ ieee80211_hw_config(local, hw_reconf_flags);
+
if (local->monitors == local->open_count && local->monitors > 0)
ieee80211_add_virtual_monitor(local);
}
--
1.8.0
On Wed, 2013-04-03 at 14:25 +0200, Johannes Berg wrote:
> Inspired by Stanislaw's patches, I took another look. He tested my
> RFC series and while there were still issues it looks better now,
> so we're going to apply this. Speak up if you don't want that! :)
Applied.
johannes
From: Johannes Berg <[email protected]>
It has to be removed from the driver, but completely
destroying it helps handle unplug of a device during
suspend since then the channel context handling etc.
doesn't have to happen later when it's removed.
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/ieee80211_i.h | 2 ++
net/mac80211/iface.c | 4 ++--
net/mac80211/pm.c | 6 ++----
net/mac80211/util.c | 5 +++++
4 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 693c181..55fb382 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1336,6 +1336,8 @@ void ieee80211_adjust_monitor_flags(struct ieee80211_sub_if_data *sdata,
const int offset);
int ieee80211_do_open(struct wireless_dev *wdev, bool coming_up);
void ieee80211_sdata_stop(struct ieee80211_sub_if_data *sdata);
+int ieee80211_add_virtual_monitor(struct ieee80211_local *local);
+void ieee80211_del_virtual_monitor(struct ieee80211_local *local);
bool __ieee80211_recalc_txpower(struct ieee80211_sub_if_data *sdata);
void ieee80211_recalc_txpower(struct ieee80211_sub_if_data *sdata);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index aadb471..5ec9c02 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -346,7 +346,7 @@ static void ieee80211_set_default_queues(struct ieee80211_sub_if_data *sdata)
sdata->vif.cab_queue = IEEE80211_INVAL_HW_QUEUE;
}
-static int ieee80211_add_virtual_monitor(struct ieee80211_local *local)
+int ieee80211_add_virtual_monitor(struct ieee80211_local *local)
{
struct ieee80211_sub_if_data *sdata;
int ret = 0;
@@ -400,7 +400,7 @@ static int ieee80211_add_virtual_monitor(struct ieee80211_local *local)
return ret;
}
-static void ieee80211_del_virtual_monitor(struct ieee80211_local *local)
+void ieee80211_del_virtual_monitor(struct ieee80211_local *local)
{
struct ieee80211_sub_if_data *sdata;
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index b98d927..d1c021b 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -21,6 +21,8 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
ieee80211_roc_purge(local, NULL);
+ ieee80211_del_virtual_monitor(local);
+
if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
mutex_lock(&local->sta_mtx);
list_for_each_entry(sta, &local->sta_list, list) {
@@ -103,10 +105,6 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
drv_remove_interface(local, sdata);
}
- sdata = rtnl_dereference(local->monitor_sdata);
- if (sdata)
- drv_remove_interface(local, sdata);
-
/*
* We disconnected on all interfaces before suspend, all channel
* contexts should be released.
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index f9581c6..43465b6 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1461,6 +1461,8 @@ int ieee80211_reconfig(struct ieee80211_local *local)
/* add interfaces */
sdata = rtnl_dereference(local->monitor_sdata);
if (sdata) {
+ /* in HW restart it exists already */
+ WARN_ON(local->resuming);
res = drv_add_interface(local, sdata);
if (WARN_ON(res)) {
rcu_assign_pointer(local->monitor_sdata, NULL);
@@ -1650,6 +1652,9 @@ int ieee80211_reconfig(struct ieee80211_local *local)
local->in_reconfig = false;
barrier();
+ if (local->monitors == local->open_count && local->monitors > 0)
+ ieee80211_add_virtual_monitor(local);
+
/*
* Clear the WLAN_STA_BLOCK_BA flag so new aggregation
* sessions can be established after a resume.
--
1.8.0
From: Johannes Berg <[email protected]>
When a device is unplugged while suspended, mac80211 is
de-initialized and all interfaces are removed while no
state is actually present in the driver. This can cause
warnings and driver confusion.
Fix this by reordering the do_stop code to not call the
driver when it is suspended, i.e. when there's no state
in the driver anyway.
The previous patches removed a few corner cases in ROC
and virtual monitor interfaces so that now this is safe
to do and no state should be left over.
Reported-by: Stanislaw Gruszka <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/iface.c | 74 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 49 insertions(+), 25 deletions(-)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 5ec9c02..b6abaaa 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -760,8 +760,6 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
sdata->dev->addr_len);
spin_unlock_bh(&local->filter_lock);
netif_addr_unlock_bh(sdata->dev);
-
- ieee80211_configure_filter(local);
}
del_timer_sync(&local->dynamic_ps_timer);
@@ -772,6 +770,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
if (sdata->wdev.cac_started) {
+ WARN_ON(local->suspended);
mutex_lock(&local->iflist_mtx);
ieee80211_vif_release_channel(sdata);
mutex_unlock(&local->iflist_mtx);
@@ -822,14 +821,9 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
if (local->monitors == 0) {
local->hw.conf.flags &= ~IEEE80211_CONF_MONITOR;
hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
- ieee80211_del_virtual_monitor(local);
}
ieee80211_adjust_monitor_flags(sdata, -1);
- ieee80211_configure_filter(local);
- mutex_lock(&local->mtx);
- ieee80211_recalc_idle(local);
- mutex_unlock(&local->mtx);
break;
case NL80211_IFTYPE_P2P_DEVICE:
/* relies on synchronize_rcu() below */
@@ -859,27 +853,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
/* fall through */
case NL80211_IFTYPE_AP:
skb_queue_purge(&sdata->skb_queue);
-
- if (going_down)
- drv_remove_interface(local, sdata);
}
sdata->bss = NULL;
- ieee80211_recalc_ps(local, -1);
-
- if (local->open_count == 0) {
- ieee80211_clear_tx_pending(local);
- ieee80211_stop_device(local);
-
- /* no reconfiguring after stop! */
- hw_reconf_flags = 0;
- }
-
- /* do after stop to avoid reconfiguring when we stop anyway */
- if (hw_reconf_flags)
- ieee80211_hw_config(local, hw_reconf_flags);
-
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
for (i = 0; i < IEEE80211_MAX_QUEUES; i++) {
skb_queue_walk_safe(&local->pending[i], skb, tmp) {
@@ -892,7 +869,54 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
}
spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
- if (local->monitors == local->open_count && local->monitors > 0)
+ if (local->open_count == 0)
+ ieee80211_clear_tx_pending(local);
+
+ /*
+ * If the interface goes down while suspended, presumably because
+ * the device was unplugged and that happens before our resume,
+ * then the driver is already unconfigured and the remainder of
+ * this function isn't needed.
+ * XXX: what about WoWLAN? If the device has software state, e.g.
+ * memory allocated, it might expect teardown commands from
+ * mac80211 here?
+ */
+ if (local->suspended) {
+ WARN_ON(local->wowlan);
+ WARN_ON(rtnl_dereference(local->monitor_sdata));
+ return;
+ }
+
+ switch (sdata->vif.type) {
+ case NL80211_IFTYPE_AP_VLAN:
+ break;
+ case NL80211_IFTYPE_MONITOR:
+ if (local->monitors == 0)
+ ieee80211_del_virtual_monitor(local);
+
+ mutex_lock(&local->mtx);
+ ieee80211_recalc_idle(local);
+ mutex_unlock(&local->mtx);
+ break;
+ default:
+ if (going_down)
+ drv_remove_interface(local, sdata);
+ }
+
+ ieee80211_recalc_ps(local, -1);
+
+ if (local->open_count == 0) {
+ ieee80211_stop_device(local);
+
+ /* no reconfiguring after stop! */
+ return;
+ }
+
+ /* do after stop to avoid reconfiguring when we stop anyway */
+ ieee80211_configure_filter(local);
+ ieee80211_hw_config(local, hw_reconf_flags);
+
+ if (local->monitors == local->open_count)
ieee80211_add_virtual_monitor(local);
}
--
1.8.0
From: Johannes Berg <[email protected]>
They can't really be executed while suspended and could
trigger work warnings, so abort all ROC items. When the
system resumes the notifications about this will be
delivered to userspace which can then act accordingly
(though it will assume they were canceled/finished.)
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/ieee80211_i.h | 3 ++-
net/mac80211/iface.c | 2 +-
net/mac80211/offchannel.c | 6 +++---
net/mac80211/pm.c | 2 ++
4 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c783e99..693c181 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1315,7 +1315,8 @@ void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local);
void ieee80211_offchannel_return(struct ieee80211_local *local);
void ieee80211_roc_setup(struct ieee80211_local *local);
void ieee80211_start_next_roc(struct ieee80211_local *local);
-void ieee80211_roc_purge(struct ieee80211_sub_if_data *sdata);
+void ieee80211_roc_purge(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata);
void ieee80211_roc_notify_destroy(struct ieee80211_roc_work *roc);
void ieee80211_sw_roc_work(struct work_struct *work);
void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index e995419..aadb471 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -714,7 +714,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
if (sdata->dev)
netif_tx_stop_all_queues(sdata->dev);
- ieee80211_roc_purge(sdata);
+ ieee80211_roc_purge(local, sdata);
if (sdata->vif.type == NL80211_IFTYPE_STATION)
ieee80211_mgd_stop(sdata);
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index b01eb73..e19d6cf 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -436,15 +436,15 @@ void ieee80211_roc_setup(struct ieee80211_local *local)
INIT_LIST_HEAD(&local->roc_list);
}
-void ieee80211_roc_purge(struct ieee80211_sub_if_data *sdata)
+void ieee80211_roc_purge(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata)
{
- struct ieee80211_local *local = sdata->local;
struct ieee80211_roc_work *roc, *tmp;
LIST_HEAD(tmp_list);
mutex_lock(&local->mtx);
list_for_each_entry_safe(roc, tmp, &local->roc_list, list) {
- if (roc->sdata != sdata)
+ if (sdata && roc->sdata != sdata)
continue;
if (roc->started && local->ops->remain_on_channel) {
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index 3d16f4e..b98d927 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -19,6 +19,8 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
ieee80211_dfs_cac_cancel(local);
+ ieee80211_roc_purge(local, NULL);
+
if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
mutex_lock(&local->sta_mtx);
list_for_each_entry(sta, &local->sta_list, list) {
--
1.8.0
On Fri, 2013-03-29 at 13:33 +0100, Stanislaw Gruszka wrote:
> Just tested (with 3/3 v2 patch) and it fixes the test case I have,
> that I used to create my patches. Test case is doing suspend with
> reverted commit 761ce8c41ed20ee3af77f2df527edc3f92e6f3bf. This make
> device is automatically removed/added during suspend, because lack
> of .reset_resume callback.
>
> Than I tested with physical remove device from USB slot while machine
> is suspend. This gives warnings in ieee80211_reconfig() and
> ieee80211_stop(). I'm not sure why, I'm going to look at that, but
> I think this patch set should be applied and some more fixes provided
> on top of it.
Sounds good. I'll send it as [PATCH] for the record.
johannes
Inspired by Stanislaw's patches, I took another look. He tested my
RFC series and while there were still issues it looks better now,
so we're going to apply this. Speak up if you don't want that! :)
johannes