| Message ID | 1361793008-2857-2-git-send-email-sgruszka@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, 2013-02-25 at 12:50 +0100, Stanislaw Gruszka wrote: > Is possible that we close interface while we are suspended, that > results in warning like below (and some others similar): > > WARNING: at net/mac80211/driver-ops.h:12 ieee80211_do_stop+0x62e/0x670 [mac80211]() > wlan0: Failed check-sdata-in-driver check, flags: 0x4 > @@ -834,16 +835,18 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, > > skb_queue_purge(&sdata->skb_queue); > > - /* > - * Free all remaining keys, there shouldn't be any, > - * except maybe group keys in AP more or WDS? > - */ > - ieee80211_free_keys(sdata); > - > drv_remove_interface_debugfs(local, sdata); > > - if (going_down) > - drv_remove_interface(local, sdata); > + if (!local->suspended) { > + /* > + * There shouldn't be any kays, sice we disconnected > + * before suspend. > + */ > + WARN_ON(!list_empty(&sdata->key_list)); This is wrong -- you're now leaking the keys after a warning; freeing the keys had nothing to do with suspend originally. You shouldn't change this, in fact you should just move the comment & code I think. More generally, does this really make much sense? There are other things here, e.g. ieee80211_configure_filter(), ieee80211_recalc_ps(), ieee80211_hw_config() and probably more that can be executed in this function -- I don't think protecting these two calls is really sufficient? I think it'd be smarter to delay the down until resumed, or so. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 26, 2013 at 09:44:00PM +0100, Johannes Berg wrote: > More generally, does this really make much sense? There are other things > here, e.g. ieee80211_configure_filter(), ieee80211_recalc_ps(), > ieee80211_hw_config() and probably more that can be executed in this > function -- I don't think protecting these two calls is really > sufficient? Seems all other drv calls like those on ieee80211_confgure_fitler() do not require sdata. So this is most likely sufficient. I'm able to reporduce warnings on rt2x00 usb with commit 761ce8c41ed20ee3af77f2df527edc3f92e6f3bf reverted. This patch make them gone. Stanislaw -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-02-27 at 11:42 +0100, Stanislaw Gruszka wrote: > On Tue, Feb 26, 2013 at 09:44:00PM +0100, Johannes Berg wrote: > > More generally, does this really make much sense? There are other things > > here, e.g. ieee80211_configure_filter(), ieee80211_recalc_ps(), > > ieee80211_hw_config() and probably more that can be executed in this > > function -- I don't think protecting these two calls is really > > sufficient? > > Seems all other drv calls like those on ieee80211_confgure_fitler() do > not require sdata. So this is most likely sufficient. I'm able to > reporduce warnings on rt2x00 usb with commit > 761ce8c41ed20ee3af77f2df527edc3f92e6f3bf reverted. This patch make > them gone. Well, we're talking about different things. You're concerned about warnings, while I'm saying that semantically this shouldn't be called while the device is stopped as the driver might not expect it. That might not cause a warning today, but that's only because we didn't put in a warning like WARN_ON(!local->device_started); johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 26, 2013 at 09:44:00PM +0100, Johannes Berg wrote: > More generally, does this really make much sense? There are other things > here, e.g. ieee80211_configure_filter(), ieee80211_recalc_ps(), > ieee80211_hw_config() and probably more that can be executed in this > function -- I don't think protecting these two calls is really > sufficient? > > I think it'd be smarter to delay the down until resumed, or so. I'm thinking how to do this nicely. For now I'll skip this change from my patch set. Stanislaw -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 2c059e5..370b86b 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -419,7 +419,8 @@ static void ieee80211_del_virtual_monitor(struct ieee80211_local *local) ieee80211_vif_release_channel(sdata); - drv_remove_interface(local, sdata); + if (!local->suspended) + drv_remove_interface(local, sdata); kfree(sdata); out_unlock: @@ -834,16 +835,18 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, skb_queue_purge(&sdata->skb_queue); - /* - * Free all remaining keys, there shouldn't be any, - * except maybe group keys in AP more or WDS? - */ - ieee80211_free_keys(sdata); - drv_remove_interface_debugfs(local, sdata); - if (going_down) - drv_remove_interface(local, sdata); + if (!local->suspended) { + /* + * There shouldn't be any kays, sice we disconnected + * before suspend. + */ + WARN_ON(!list_empty(&sdata->key_list)); + + if (going_down) + drv_remove_interface(local, sdata); + } } sdata->bss = NULL;
Is possible that we close interface while we are suspended, that results in warning like below (and some others similar): WARNING: at net/mac80211/driver-ops.h:12 ieee80211_do_stop+0x62e/0x670 [mac80211]() wlan0: Failed check-sdata-in-driver check, flags: 0x4 Call Trace: [<ffffffff8105c9d6>] warn_slowpath_fmt+0x46/0x50 [<ffffffffa045d46e>] ieee80211_do_stop+0x62e/0x670 [mac80211] [<ffffffffa045d4ca>] ieee80211_stop+0x1a/0x20 [mac80211] [<ffffffff815122ed>] __dev_close_many+0x7d/0xc0 [<ffffffff81513af8>] dev_close_many+0x88/0x100 [<ffffffff81513f2a>] dev_close+0x3a/0x50 [<ffffffffa03c90ae>] cfg80211_rfkill_set_block+0x6e/0xa0 [cfg80211] [<ffffffffa03c9106>] cfg80211_rfkill_sync_work+0x26/0x30 [cfg80211] Check for local->suspended in ieee80211_do_stop() before perform any action that require operation on interface that was removed from the driver. All keys should be freed, so remove ieee80211_free_keys() and add corresponding WARN_ON(). Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> --- net/mac80211/iface.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)