When a device driver is unloaded local->interfaces
list is cleared. If there was more than 1
interface running and connected (bound to a
chanctx) then chantype recalc was called and it
ended up with compat being NULL causing a call
trace warning.
Warn if compat becomes NULL as a result of
incompatible bss_conf.chandef of interfaces bound
to a given channel context only.
The call trace looked like this:
WARNING: CPU: 2 PID: 2594 at /devel/src/linux/net/mac80211/chan.c:557 ieee80211_recalc_chanctx_chantype+0x2cd/0x2e0()
Modules linked in: ath10k_pci(-) ath10k_core ath
CPU: 2 PID: 2594 Comm: rmmod Tainted: G W 3.16.0-rc1+ #150
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
0000000000000009 ffff88001ea279c0 ffffffff818dfa93 0000000000000000
ffff88001ea279f8 ffffffff810514a8 ffff88001ce09cd0 ffff88001e03cc58
0000000000000000 ffff88001ce08840 ffff88001ce09cd0 ffff88001ea27a08
Call Trace:
[<ffffffff818dfa93>] dump_stack+0x4d/0x66
[<ffffffff810514a8>] warn_slowpath_common+0x78/0xa0
[<ffffffff81051585>] warn_slowpath_null+0x15/0x20
[<ffffffff818a407d>] ieee80211_recalc_chanctx_chantype+0x2cd/0x2e0
[<ffffffff818a3dda>] ? ieee80211_recalc_chanctx_chantype+0x2a/0x2e0
[<ffffffff818a4919>] ieee80211_assign_vif_chanctx+0x1a9/0x770
[<ffffffff818a6220>] __ieee80211_vif_release_channel+0x70/0x130
[<ffffffff818a6dd3>] ieee80211_vif_release_channel+0x43/0xb0
[<ffffffff81885f4e>] ieee80211_stop_ap+0x21e/0x5a0
[<ffffffff8184b9b5>] __cfg80211_stop_ap+0x85/0x520
[<ffffffff8181c188>] __cfg80211_leave+0x68/0x120
[<ffffffff8181c268>] cfg80211_leave+0x28/0x40
[<ffffffff8181c5f3>] cfg80211_netdev_notifier_call+0x373/0x6b0
[<ffffffff8107f965>] notifier_call_chain+0x55/0x110
[<ffffffff8107fa41>] raw_notifier_call_chain+0x11/0x20
[<ffffffff816a8dc0>] call_netdevice_notifiers_info+0x30/0x60
[<ffffffff816a8eb9>] __dev_close_many+0x59/0xf0
[<ffffffff816a9021>] dev_close_many+0x81/0x120
[<ffffffff816aa1c5>] rollback_registered_many+0x115/0x2a0
[<ffffffff816aa3a6>] unregister_netdevice_many+0x16/0xa0
[<ffffffff8187d841>] ieee80211_remove_interfaces+0x121/0x1b0
[<ffffffff8185e0e6>] ieee80211_unregister_hw+0x56/0x110
[<ffffffffa0011ac4>] ath10k_mac_unregister+0x14/0x60 [ath10k_core]
[<ffffffffa0014fe7>] ath10k_core_unregister+0x27/0x40 [ath10k_core]
[<ffffffffa003b1f4>] ath10k_pci_remove+0x44/0xa0 [ath10k_pci]
[<ffffffff81373138>] pci_device_remove+0x28/0x60
[<ffffffff814cb534>] __device_release_driver+0x64/0xd0
[<ffffffff814cbcc8>] driver_detach+0xb8/0xc0
[<ffffffff814cb23a>] bus_remove_driver+0x4a/0xb0
[<ffffffff814cc697>] driver_unregister+0x27/0x50
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/chan.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index f3317fa..d810b2e 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -549,12 +549,12 @@ static void ieee80211_recalc_chanctx_chantype(struct ieee80211_local *local,
compat = cfg80211_chandef_compatible(
&sdata->vif.bss_conf.chandef, compat);
- if (!compat)
+ if (WARN_ON_ONCE(!compat))
break;
}
rcu_read_unlock();
- if (WARN_ON_ONCE(!compat))
+ if (!compat)
return;
ieee80211_change_chanctx(local, ctx, compat);
--
1.8.5.3
On Mon, 2014-07-28 at 15:16 +0200, Michal Kazior wrote:
> When a device driver is unloaded local->interfaces
> list is cleared. If there was more than 1
> interface running and connected (bound to a
> chanctx) then chantype recalc was called and it
> ended up with compat being NULL causing a call
> trace warning.
>
> Warn if compat becomes NULL as a result of
> incompatible bss_conf.chandef of interfaces bound
> to a given channel context only.
Err, this slipped my radar - where does this need to go?
Also - is this really the right thing? Why does this happen in this
situation? Shouldn't they all be not connected or not have a chanctx or
be compatible?
johannes
On 11 August 2014 17:05, Johannes Berg <[email protected]> wrote:
> On Mon, 2014-07-28 at 15:16 +0200, Michal Kazior wrote:
>> When a device driver is unloaded local->interfaces
>> list is cleared. If there was more than 1
>> interface running and connected (bound to a
>> chanctx) then chantype recalc was called and it
>> ended up with compat being NULL causing a call
>> trace warning.
>>
>> Warn if compat becomes NULL as a result of
>> incompatible bss_conf.chandef of interfaces bound
>> to a given channel context only.
>
> Err, this slipped my radar - where does this need to go?
No big deal. The warning is just a small nuisance with multi-vif. You
won't see this on a typical system.
>
> Also - is this really the right thing? Why does this happen in this
> situation? Shouldn't they all be not connected or not have a chanctx or
> be compatible?
If you eject, e.g. plug a usb wifi device out, its driver will call
ieee80211_unregister_hw() which calls ieee80211_remove_interfaces().
If there are still running interfaces that have a netdev (probably to
avoid a deadlock on iflist_mtx?) each interface is *moved* to a
temporary list for a later call to unregister_netdevice_many(). While
unregister_netdevice_many() executes the local->interfaces is empty.
ieee80211_recalc_chanctx_chantype() is called whenever a chanctx is
unassigned from a vif as long as there are still references to that
chanctx. If there is just 1 interface when device is ejected, then
ieee80211_recalc_chanctx_chantype() is not called. If there is more
than 1 running interfaces then the removal of each but the last one
calls to ieee80211_recalc_chanctx_chantype(). Since local->interfaces
is empty compat pointer stays NULL and this triggers WARN_ON_ONCE (but
only the first one effectively dumps a trace).
Another way to fix this would probably be to change
ieee80211_remove_interfaces() to remove all interfaces one-by-one
(instead of in bulk). It seems iflist_mtx can be temporarily released
while iterating over interfaces for the sake of calling
unregister_netdevice() for each one because RTNL is held all the time
guaranteeing no new interfaces are added in the meantime. I'm not sure
if it's perfectly safe to replace unregister_netdevice_many() just
like that though. I can look more into it.
Michał
On Mon, 2014-08-18 at 15:07 +0200, Michal Kazior wrote:
> If you eject, e.g. plug a usb wifi device out, its driver will call
> ieee80211_unregister_hw() which calls ieee80211_remove_interfaces().
> If there are still running interfaces that have a netdev (probably to
> avoid a deadlock on iflist_mtx?) each interface is *moved* to a
> temporary list for a later call to unregister_netdevice_many(). While
> unregister_netdevice_many() executes the local->interfaces is empty.
> ieee80211_recalc_chanctx_chantype() is called whenever a chanctx is
> unassigned from a vif as long as there are still references to that
> chanctx. If there is just 1 interface when device is ejected, then
> ieee80211_recalc_chanctx_chantype() is not called. If there is more
> than 1 running interfaces then the removal of each but the last one
> calls to ieee80211_recalc_chanctx_chantype(). Since local->interfaces
> is empty compat pointer stays NULL and this triggers WARN_ON_ONCE (but
> only the first one effectively dumps a trace).
Ah ok.
> Another way to fix this would probably be to change
> ieee80211_remove_interfaces() to remove all interfaces one-by-one
> (instead of in bulk). It seems iflist_mtx can be temporarily released
> while iterating over interfaces for the sake of calling
> unregister_netdevice() for each one because RTNL is held all the time
> guaranteeing no new interfaces are added in the meantime. I'm not sure
> if it's perfectly safe to replace unregister_netdevice_many() just
> like that though. I can look more into it.
Well, it's safe, but it's more efficient to call the _many() because
otherwise you require rcu_synchronize() for each one.
I'll apply this, to -next for now, later we can decide if we want to
send it to stable or so?
johannes
On 29 August 2014 13:05, Johannes Berg <[email protected]> wrote:
> On Mon, 2014-08-18 at 15:07 +0200, Michal Kazior wrote:
[...]
>> Another way to fix this would probably be to change
>> ieee80211_remove_interfaces() to remove all interfaces one-by-one
>> (instead of in bulk). It seems iflist_mtx can be temporarily released
>> while iterating over interfaces for the sake of calling
>> unregister_netdevice() for each one because RTNL is held all the time
>> guaranteeing no new interfaces are added in the meantime. I'm not sure
>> if it's perfectly safe to replace unregister_netdevice_many() just
>> like that though. I can look more into it.
>
> Well, it's safe, but it's more efficient to call the _many() because
> otherwise you require rcu_synchronize() for each one.
>
> I'll apply this, to -next for now, later we can decide if we want to
> send it to stable or so?
Thanks. As far as I'm aware the splat was harmless so I'm unsure if it
qualifies for stable.
Michał