Patches for problems I hit during P2P tests when
multichannel used (driver loaded with use_chanctx=1).
Janusz Dziedzic (3):
ath9k: handle RoC abort correctly
ath9k: make rxfilter per HW
ath9k: advertise p2p dev support when chanctx
drivers/net/wireless/ath/ath9k/ath9k.h | 3 ++-
drivers/net/wireless/ath/ath9k/channel.c | 3 ++-
drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
drivers/net/wireless/ath/ath9k/main.c | 2 +-
drivers/net/wireless/ath/ath9k/recv.c | 12 ++++++------
5 files changed, 16 insertions(+), 10 deletions(-)
--
1.9.1
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBKb2hhbm5lcyBCZXJnIFttYWls
dG86am9oYW5uZXNAc2lwc29sdXRpb25zLm5ldF0NCj4gU2VudDogTW9uZGF5LCBKdW5lIDIyLCAy
MDE1IDE1OjAwDQo+IFRvOiBKYW51c3ogRHppZWR6aWMNCj4gQ2M6IGxpbnV4LXdpcmVsZXNzQHZn
ZXIua2VybmVsLm9yZzsgYXRoOWstZGV2ZWxAdmVuZW1hLmg0Y2tyLm5ldDsNCj4gbmJkQG9wZW53
cnQub3JnOyBzdWppdGhAbXN1aml0aC5vcmc7IE90Y2hlcmV0aWFuc2tpLCBBbmRyZWkNCj4gU3Vi
amVjdDogUmU6IFtQQVRDSCAyLzNdIGF0aDlrOiBtYWtlIHJ4ZmlsdGVyIHBlciBIVw0KPiANCj4g
T24gTW9uLCAyMDE1LTA2LTIyIGF0IDEzOjU4ICswMjAwLCBKb2hhbm5lcyBCZXJnIHdyb3RlOg0K
PiA+IE9uIE1vbiwgMjAxNS0wNi0yMiBhdCAxMzo0MyArMDIwMCwgSmFudXN6IER6aWVkemljIHdy
b3RlOg0KPiA+ID4gbWFjODAyMTEgY29uZmlndXJlIHJ4ZmlsdGVyIHBlciBIVywNCj4gPiA+IHNv
IHdlIGRvbid0IG5lZWQgdGhpcyBwZXIgY2hhbm5lbC4NCj4gPg0KPiA+IEFzIEkgc2FpZCBiZWZv
cmUsIEkgdGhpbmsgdGhlcmUncyB2YWx1ZSBpbiBtYWM4MDIxMSBkb2luZyBpdCBwZXINCj4gPiBj
aGFuY3R4IG9yIGV2ZW4gcGVyIHZpZiwgYW5kIGl0IHNob3VsZCBiZSBtb3JlIGVmZmljaWVudCB0
byBkbyBzby4NCj4gPg0KPiA+IEl0J3MgdGVtcHRpbmcgdG8gZG8gaXQgcGVyIHZpZiBhbmQgbGVh
dmUgdGhlIGNoYW5jdHggd29yayB1cCB0byB0aGUNCj4gPiBkcml2ZXIsIGJ1dCBwZXJoYXBzIHdp
dGggQ1NBIGFuZCBhbGwgdGhhdCBpdCBnZXRzIGNvbXBsaWNhdGVkIGVub3VnaA0KPiA+IHRoYXQg
ZG9pbmcgaXQgcGVyIGNoYW5jdHggaW4gbWFjODAyMTEgd291bGQgbWFrZSBzZW5zZT8NCg0KSSBk
b24ndCB0aGluayB0aGF0IGl0IG1ha2VzIHNlbnNlIHRvIGRvIGl0IHBlciBjaGFuY3R4IGluIG1h
YzgwMjExLg0KVGhlIGNmZzgwMjExIEFQSSB0byBjb25maWd1cmUgZmlsdGVyIGlzbid0IGF3YXJl
IGFib3V0IGNoYW5jdHgncyBhbmQgYWxzbw0KdmlmcyB0aGF0IHNoYXJlIHNhbWUgY2hhbmN0eCBt
YXkgaGF2ZSBkaWZmZXJlbnQgZmlsdGVycyBldGMuLg0KDQpBbmRyZWkNCj4gDQo+IE9uIHRoZSBv
dGhlciBoYW5kLCBJIHRoaW5rIG91ciBkZXZpY2UgcmVxdWlyZXMgaXQgcGVyIHZpZiwgc28gd2Un
ZCBwcm9iYWJseQ0KPiBoYXZlIHRvIGRvIGJvdGguDQo+IA0KPiArQW5kcmVpLCB3aG8gd2FzIGxv
b2tpbmcgaW50byB0aGlzLg0KPiANCj4gam9oYW5uZXMNCg0K
On Mon, 2015-06-22 at 13:43 +0200, Janusz Dziedzic wrote:
> mac80211 configure rxfilter per HW,
> so we don't need this per channel.
As I said before, I think there's value in mac80211 doing it per chanctx
or even per vif, and it should be more efficient to do so.
It's tempting to do it per vif and leave the chanctx work up to the
driver, but perhaps with CSA and all that it gets complicated enough
that doing it per chanctx in mac80211 would make sense?
johannes
On Mon, Jun 22, 2015 at 5:13 PM, Janusz Dziedzic
<[email protected]> wrote:
> In case we will get ROC abort we should not call
> ieee80211_remain_on_channel_expired().
>
> In other case I hit such warning on MIPS and
> p2p negotiation failed (tested with use_chanctx=1).
>
> ath: phy0: Starting RoC period
> ath: phy0: Channel definition created: 2412 MHz
> ath: phy0: Assigned next_chan to 2412 MHz
> ath: phy0: Offchannel duration for chan 2412 MHz : 506632
> ath: phy0: ath_chanctx_set_next: current: 2412 MHz, next: 2412 MHz
> ath: phy0: Stopping current chanctx: 2412
> ath: phy0: Flush timeout: 200
> ath: phy0: ath_chanctx_set_next: Set channel 2412 MHz
> ath: phy0: Set channel: 2412 MHz width: 0
> ath: phy0: Reset to 2412 MHz, HT40: 0 fastcc: 0
> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_TSF_TIMER, state: ATH_CHANCTX_STATE_IDLE
> ath: phy0: ath_offchannel_channel_change: offchannel state: ATH_OFFCHANNEL_ROC_START
> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_SWITCH, state: ATH_CHANCTX_STATE_IDLE
> ath: phy0: Cancel RoC
> ath: phy0: RoC aborted
> ath: phy0: RoC request on vif: 00:03:7f:4e:a0:cd, type: 1 duration: 500
> ath: phy0: Starting RoC period
> ath: phy0: Channel definition created: 2412 MHz
> ath: phy0: Assigned next_chan to 2412 MHz
> ath: phy0: Offchannel duration for chan 2412 MHz : 506705
> ath: phy0: ath_chanctx_set_next: current: 2412 MHz, next: 2412 MHz
> ath: phy0: ath_offchannel_channel_change: offchannel state: ATH_OFFCHANNEL_ROC_START
> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_SWITCH, state: ATH_CHANCTX_STATE_IDLE
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 3312 at drivers/net/wireless/ath/ath9k/main.c:2319
> Modules linked in: ath9k ath9k_common ath9k_hw ath mac80211 cfg80211
>
> Signed-off-by: Janusz Dziedzic <[email protected]>
> ---
> drivers/net/wireless/ath/ath9k/channel.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
> index 2066650..e211325 100644
> --- a/drivers/net/wireless/ath/ath9k/channel.c
> +++ b/drivers/net/wireless/ath/ath9k/channel.c
> @@ -926,7 +926,8 @@ void ath_roc_complete(struct ath_softc *sc, bool abort)
>
> sc->offchannel.roc_vif = NULL;
> sc->offchannel.roc_chan = NULL;
> - ieee80211_remain_on_channel_expired(sc->hw);
> + if (!abort)
> + ieee80211_remain_on_channel_expired(sc->hw);
> ath_offchannel_next(sc);
> ath9k_ps_restore(sc);
> }
If HW aborts RoC in middle, should not we inform mac80211
that RoC is expired?
Also the we are clearing roc_vif independent of abort, so the warning
indicates that roc_complete has not come from FW, may be we should
understand that first?
On 22 June 2015 at 13:56, Krishna Chaitanya <[email protected]> wrote:
> On Mon, Jun 22, 2015 at 5:13 PM, Janusz Dziedzic
> <[email protected]> wrote:
>> In case we will get ROC abort we should not call
>> ieee80211_remain_on_channel_expired().
>>
>> In other case I hit such warning on MIPS and
>> p2p negotiation failed (tested with use_chanctx=1).
>>
>> ath: phy0: Starting RoC period
>> ath: phy0: Channel definition created: 2412 MHz
>> ath: phy0: Assigned next_chan to 2412 MHz
>> ath: phy0: Offchannel duration for chan 2412 MHz : 506632
>> ath: phy0: ath_chanctx_set_next: current: 2412 MHz, next: 2412 MHz
>> ath: phy0: Stopping current chanctx: 2412
>> ath: phy0: Flush timeout: 200
>> ath: phy0: ath_chanctx_set_next: Set channel 2412 MHz
>> ath: phy0: Set channel: 2412 MHz width: 0
>> ath: phy0: Reset to 2412 MHz, HT40: 0 fastcc: 0
>> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_TSF_TIMER, state: ATH_CHANCTX_STATE_IDLE
>> ath: phy0: ath_offchannel_channel_change: offchannel state: ATH_OFFCHANNEL_ROC_START
>> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_SWITCH, state: ATH_CHANCTX_STATE_IDLE
>> ath: phy0: Cancel RoC
>> ath: phy0: RoC aborted
>> ath: phy0: RoC request on vif: 00:03:7f:4e:a0:cd, type: 1 duration: 500
>> ath: phy0: Starting RoC period
>> ath: phy0: Channel definition created: 2412 MHz
>> ath: phy0: Assigned next_chan to 2412 MHz
>> ath: phy0: Offchannel duration for chan 2412 MHz : 506705
>> ath: phy0: ath_chanctx_set_next: current: 2412 MHz, next: 2412 MHz
>> ath: phy0: ath_offchannel_channel_change: offchannel state: ATH_OFFCHANNEL_ROC_START
>> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_SWITCH, state: ATH_CHANCTX_STATE_IDLE
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 3312 at drivers/net/wireless/ath/ath9k/main.c:2319
>> Modules linked in: ath9k ath9k_common ath9k_hw ath mac80211 cfg80211
>>
>> Signed-off-by: Janusz Dziedzic <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath9k/channel.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
>> index 2066650..e211325 100644
>> --- a/drivers/net/wireless/ath/ath9k/channel.c
>> +++ b/drivers/net/wireless/ath/ath9k/channel.c
>> @@ -926,7 +926,8 @@ void ath_roc_complete(struct ath_softc *sc, bool abort)
>>
>> sc->offchannel.roc_vif = NULL;
>> sc->offchannel.roc_chan = NULL;
>> - ieee80211_remain_on_channel_expired(sc->hw);
>> + if (!abort)
>> + ieee80211_remain_on_channel_expired(sc->hw);
>> ath_offchannel_next(sc);
>> ath9k_ps_restore(sc);
>> }
> If HW aborts RoC in middle, should not we inform mac80211
> that RoC is expired?
Good point. The ath_roc_complete() can be called with abort=true from
ath9k_cancel_pending_offchannel() as well. I guess ath_roc_complete()
needs a "reason" argument (instead of "abort") with: expired, aborted,
cancelled values. ieee80211_remain_on_channel_expired() should be
called whenever reason != cancelled.
By the way - is ath_roc_complete() lock protected properly? It looks
like it isn't from a quick glance. Neither sdata lock nor local->mtx
can be implied in all contexts and sc->mutex isn't always held while
it's called, hmm.. or am I missing something?
> Also the we are clearing roc_vif independent of abort, so the warning
> indicates that roc_complete has not come from FW, may be we should
> understand that first?
There's no FW in ath9k.
The problem is the following sequence:
1. mac80211 requests roc A
2. mac80211 cancels roc A
a. ath9k calls expired() and hw_roc_done work is scheduled
3. mac80211 requests roc B
4. mac80211 starts to process the scheduled hw_roc_done
5. mac80211 thinks roc B has expired
6. mac80211 requests roc C
7. ath9k WARN_ON is hit
There's a race between (3) and (4). Depending on circumstances (3) and
(4) may be reordered so the current code doesn't fail all the time.
Michał
On Mon, 2015-06-22 at 13:58 +0200, Johannes Berg wrote:
> On Mon, 2015-06-22 at 13:43 +0200, Janusz Dziedzic wrote:
> > mac80211 configure rxfilter per HW,
> > so we don't need this per channel.
>
> As I said before, I think there's value in mac80211 doing it per chanctx
> or even per vif, and it should be more efficient to do so.
>
> It's tempting to do it per vif and leave the chanctx work up to the
> driver, but perhaps with CSA and all that it gets complicated enough
> that doing it per chanctx in mac80211 would make sense?
On the other hand, I think our device requires it per vif, so we'd
probably have to do both.
+Andrei, who was looking into this.
johannes
On 22 June 2015 at 16:01, Krishna Chaitanya <[email protected]> wrote:
> On Mon, Jun 22, 2015 at 6:32 PM, Michal Kazior <[email protected]> wrote:
>> On 22 June 2015 at 13:56, Krishna Chaitanya <[email protected]> wrote:
>>> On Mon, Jun 22, 2015 at 5:13 PM, Janusz Dziedzic
>>> <[email protected]> wrote:
>>>> In case we will get ROC abort we should not call
>>>> ieee80211_remain_on_channel_expired().
>>>>
>>>> In other case I hit such warning on MIPS and
>>>> p2p negotiation failed (tested with use_chanctx=1).
>>>>
>>>> ath: phy0: Starting RoC period
>>>> ath: phy0: Channel definition created: 2412 MHz
>>>> ath: phy0: Assigned next_chan to 2412 MHz
>>>> ath: phy0: Offchannel duration for chan 2412 MHz : 506632
>>>> ath: phy0: ath_chanctx_set_next: current: 2412 MHz, next: 2412 MHz
>>>> ath: phy0: Stopping current chanctx: 2412
>>>> ath: phy0: Flush timeout: 200
>>>> ath: phy0: ath_chanctx_set_next: Set channel 2412 MHz
>>>> ath: phy0: Set channel: 2412 MHz width: 0
>>>> ath: phy0: Reset to 2412 MHz, HT40: 0 fastcc: 0
>>>> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_TSF_TIMER, state: ATH_CHANCTX_STATE_IDLE
>>>> ath: phy0: ath_offchannel_channel_change: offchannel state: ATH_OFFCHANNEL_ROC_START
>>>> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_SWITCH, state: ATH_CHANCTX_STATE_IDLE
>>>> ath: phy0: Cancel RoC
>>>> ath: phy0: RoC aborted
>>>> ath: phy0: RoC request on vif: 00:03:7f:4e:a0:cd, type: 1 duration: 500
>>>> ath: phy0: Starting RoC period
>>>> ath: phy0: Channel definition created: 2412 MHz
>>>> ath: phy0: Assigned next_chan to 2412 MHz
>>>> ath: phy0: Offchannel duration for chan 2412 MHz : 506705
>>>> ath: phy0: ath_chanctx_set_next: current: 2412 MHz, next: 2412 MHz
>>>> ath: phy0: ath_offchannel_channel_change: offchannel state: ATH_OFFCHANNEL_ROC_START
>>>> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_SWITCH, state: ATH_CHANCTX_STATE_IDLE
>>>> ------------[ cut here ]------------
>>>> WARNING: CPU: 0 PID: 3312 at drivers/net/wireless/ath/ath9k/main.c:2319
>>>> Modules linked in: ath9k ath9k_common ath9k_hw ath mac80211 cfg80211
>>>>
>>>> Signed-off-by: Janusz Dziedzic <[email protected]>
>>>> ---
>>>> drivers/net/wireless/ath/ath9k/channel.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
>>>> index 2066650..e211325 100644
>>>> --- a/drivers/net/wireless/ath/ath9k/channel.c
>>>> +++ b/drivers/net/wireless/ath/ath9k/channel.c
>>>> @@ -926,7 +926,8 @@ void ath_roc_complete(struct ath_softc *sc, bool abort)
>>>>
>>>> sc->offchannel.roc_vif = NULL;
>>>> sc->offchannel.roc_chan = NULL;
>>>> - ieee80211_remain_on_channel_expired(sc->hw);
>>>> + if (!abort)
>>>> + ieee80211_remain_on_channel_expired(sc->hw);
>>>> ath_offchannel_next(sc);
>>>> ath9k_ps_restore(sc);
>>>> }
>>> If HW aborts RoC in middle, should not we inform mac80211
>>> that RoC is expired?
>>
>> Good point. The ath_roc_complete() can be called with abort=true from
>> ath9k_cancel_pending_offchannel() as well. I guess ath_roc_complete()
>> needs a "reason" argument (instead of "abort") with: expired, aborted,
>> cancelled values. ieee80211_remain_on_channel_expired() should be
>> called whenever reason != cancelled.
> Agree, make sense.
>> By the way - is ath_roc_complete() lock protected properly? It looks
>> like it isn't from a quick glance. Neither sdata lock nor local->mtx
>> can be implied in all contexts and sc->mutex isn't always held while
>> it's called, hmm.. or am I missing something?
>>
>>> Also the we are clearing roc_vif independent of abort, so the warning
>>> indicates that roc_complete has not come from FW, may be we should
>>> understand that first?
>>
>> There's no FW in ath9k.
>>
>> The problem is the following sequence:
>> 1. mac80211 requests roc A
>> 2. mac80211 cancels roc A
>> a. ath9k calls expired() and hw_roc_done work is scheduled
>> 3. mac80211 requests roc B
>> 4. mac80211 starts to process the scheduled hw_roc_done
>> 5. mac80211 thinks roc B has expired
>> 6. mac80211 requests roc C
>> 7. ath9k WARN_ON is hit
>>
>> There's a race between (3) and (4). Depending on circumstances (3) and
>> (4) may be reordered so the current code doesn't fail all the time.
> Ok i understand, but if we get roc_complete for B before 6, then it works
> fine at least at ath9k level, C will be unblocked.
>
> Anyways, handling the cancel case should resolve it along with proper locking.
Thanks for comments, will send v2
BR
Janusz
Advertise p2p device support when ath9k loaded with
use_chanctx=1.
This will fix problem, when first interface is an AP
and next we would like to run p2p_find.
Before p2p find (scan phase) failed with EOPNOTSUPP.
Signed-off-by: Janusz Dziedzic <[email protected]>
---
drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index f8d11ef..7da1a17 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -736,13 +736,14 @@ static const struct ieee80211_iface_limit if_limits_multi[] = {
BIT(NL80211_IFTYPE_P2P_CLIENT) |
BIT(NL80211_IFTYPE_P2P_GO) },
{ .max = 1, .types = BIT(NL80211_IFTYPE_ADHOC) },
+ { .max = 1, .types = BIT(NL80211_IFTYPE_P2P_DEVICE) },
};
static const struct ieee80211_iface_combination if_comb_multi[] = {
{
.limits = if_limits_multi,
.n_limits = ARRAY_SIZE(if_limits_multi),
- .max_interfaces = 2,
+ .max_interfaces = 3,
.num_different_channels = 2,
.beacon_int_infra_match = true,
},
@@ -855,6 +856,9 @@ static void ath9k_set_hw_capab(struct ath_softc *sc, struct ieee80211_hw *hw)
BIT(NL80211_IFTYPE_MESH_POINT) |
BIT(NL80211_IFTYPE_WDS);
+ if (ath9k_is_chanctx_enabled())
+ hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_P2P_DEVICE);
+
hw->wiphy->iface_combinations = if_comb;
hw->wiphy->n_iface_combinations = ARRAY_SIZE(if_comb);
}
--
1.9.1
On 2015-06-22 13:59, Johannes Berg wrote:
> On Mon, 2015-06-22 at 13:58 +0200, Johannes Berg wrote:
>> On Mon, 2015-06-22 at 13:43 +0200, Janusz Dziedzic wrote:
>> > mac80211 configure rxfilter per HW,
>> > so we don't need this per channel.
>>
>> As I said before, I think there's value in mac80211 doing it per chanctx
>> or even per vif, and it should be more efficient to do so.
>>
>> It's tempting to do it per vif and leave the chanctx work up to the
>> driver, but perhaps with CSA and all that it gets complicated enough
>> that doing it per chanctx in mac80211 would make sense?
>
> On the other hand, I think our device requires it per vif, so we'd
> probably have to do both.
>
> +Andrei, who was looking into this.
There's value in it, but I think it makes more sense to fix this bug
now, and rework the code again later when mac80211 has support for
per-vif or per-chanctx rxfilter.
- Felix
On Mon, Jun 22, 2015 at 6:32 PM, Michal Kazior <[email protected]> wrote:
> On 22 June 2015 at 13:56, Krishna Chaitanya <[email protected]> wrote:
>> On Mon, Jun 22, 2015 at 5:13 PM, Janusz Dziedzic
>> <[email protected]> wrote:
>>> In case we will get ROC abort we should not call
>>> ieee80211_remain_on_channel_expired().
>>>
>>> In other case I hit such warning on MIPS and
>>> p2p negotiation failed (tested with use_chanctx=1).
>>>
>>> ath: phy0: Starting RoC period
>>> ath: phy0: Channel definition created: 2412 MHz
>>> ath: phy0: Assigned next_chan to 2412 MHz
>>> ath: phy0: Offchannel duration for chan 2412 MHz : 506632
>>> ath: phy0: ath_chanctx_set_next: current: 2412 MHz, next: 2412 MHz
>>> ath: phy0: Stopping current chanctx: 2412
>>> ath: phy0: Flush timeout: 200
>>> ath: phy0: ath_chanctx_set_next: Set channel 2412 MHz
>>> ath: phy0: Set channel: 2412 MHz width: 0
>>> ath: phy0: Reset to 2412 MHz, HT40: 0 fastcc: 0
>>> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_TSF_TIMER, state: ATH_CHANCTX_STATE_IDLE
>>> ath: phy0: ath_offchannel_channel_change: offchannel state: ATH_OFFCHANNEL_ROC_START
>>> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_SWITCH, state: ATH_CHANCTX_STATE_IDLE
>>> ath: phy0: Cancel RoC
>>> ath: phy0: RoC aborted
>>> ath: phy0: RoC request on vif: 00:03:7f:4e:a0:cd, type: 1 duration: 500
>>> ath: phy0: Starting RoC period
>>> ath: phy0: Channel definition created: 2412 MHz
>>> ath: phy0: Assigned next_chan to 2412 MHz
>>> ath: phy0: Offchannel duration for chan 2412 MHz : 506705
>>> ath: phy0: ath_chanctx_set_next: current: 2412 MHz, next: 2412 MHz
>>> ath: phy0: ath_offchannel_channel_change: offchannel state: ATH_OFFCHANNEL_ROC_START
>>> ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_SWITCH, state: ATH_CHANCTX_STATE_IDLE
>>> ------------[ cut here ]------------
>>> WARNING: CPU: 0 PID: 3312 at drivers/net/wireless/ath/ath9k/main.c:2319
>>> Modules linked in: ath9k ath9k_common ath9k_hw ath mac80211 cfg80211
>>>
>>> Signed-off-by: Janusz Dziedzic <[email protected]>
>>> ---
>>> drivers/net/wireless/ath/ath9k/channel.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
>>> index 2066650..e211325 100644
>>> --- a/drivers/net/wireless/ath/ath9k/channel.c
>>> +++ b/drivers/net/wireless/ath/ath9k/channel.c
>>> @@ -926,7 +926,8 @@ void ath_roc_complete(struct ath_softc *sc, bool abort)
>>>
>>> sc->offchannel.roc_vif = NULL;
>>> sc->offchannel.roc_chan = NULL;
>>> - ieee80211_remain_on_channel_expired(sc->hw);
>>> + if (!abort)
>>> + ieee80211_remain_on_channel_expired(sc->hw);
>>> ath_offchannel_next(sc);
>>> ath9k_ps_restore(sc);
>>> }
>> If HW aborts RoC in middle, should not we inform mac80211
>> that RoC is expired?
>
> Good point. The ath_roc_complete() can be called with abort=true from
> ath9k_cancel_pending_offchannel() as well. I guess ath_roc_complete()
> needs a "reason" argument (instead of "abort") with: expired, aborted,
> cancelled values. ieee80211_remain_on_channel_expired() should be
> called whenever reason != cancelled.
Agree, make sense.
> By the way - is ath_roc_complete() lock protected properly? It looks
> like it isn't from a quick glance. Neither sdata lock nor local->mtx
> can be implied in all contexts and sc->mutex isn't always held while
> it's called, hmm.. or am I missing something?
>
>> Also the we are clearing roc_vif independent of abort, so the warning
>> indicates that roc_complete has not come from FW, may be we should
>> understand that first?
>
> There's no FW in ath9k.
>
> The problem is the following sequence:
> 1. mac80211 requests roc A
> 2. mac80211 cancels roc A
> a. ath9k calls expired() and hw_roc_done work is scheduled
> 3. mac80211 requests roc B
> 4. mac80211 starts to process the scheduled hw_roc_done
> 5. mac80211 thinks roc B has expired
> 6. mac80211 requests roc C
> 7. ath9k WARN_ON is hit
>
> There's a race between (3) and (4). Depending on circumstances (3) and
> (4) may be reordered so the current code doesn't fail all the time.
Ok i understand, but if we get roc_complete for B before 6, then it works
fine at least at ath9k level, C will be unblocked.
Anyways, handling the cancel case should resolve it along with proper locking.
mac80211 configure rxfilter per HW,
so we don't need this per channel.
This fix problem when chanctx used and ath9k
allocate new internal ath_chanctx (eg. when
offchannel) and we loose rxfilter configuration.
Eg. when p2p_find (with use_chanctx=1), during
remain on channel, driver create new ath_chanctx
with incorrect rxfilter. Then we didn't receive
probe requests and fail p2p_find.
Signed-off-by: Janusz Dziedzic <[email protected]>
---
drivers/net/wireless/ath/ath9k/ath9k.h | 3 ++-
drivers/net/wireless/ath/ath9k/main.c | 2 +-
drivers/net/wireless/ath/ath9k/recv.c | 12 ++++++------
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index a7a81b3..030fd0f 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -356,7 +356,6 @@ struct ath_chanctx {
short nvifs;
short nvifs_assigned;
- unsigned int rxfilter;
};
enum ath_chanctx_event {
@@ -1001,8 +1000,10 @@ struct ath_softc {
struct cfg80211_chan_def cur_chandef;
struct ath_chanctx chanctx[ATH9K_NUM_CHANCTX];
struct ath_chanctx *cur_chan;
+ unsigned int rxfilter;
spinlock_t chan_lock;
+
#ifdef CONFIG_MAC80211_LEDS
bool led_registered;
char led_name[32];
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index d285e3a..945f002 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1463,7 +1463,7 @@ static void ath9k_configure_filter(struct ieee80211_hw *hw,
*total_flags &= SUPPORTED_FILTERS;
spin_lock_bh(&sc->chan_lock);
- sc->cur_chan->rxfilter = *total_flags;
+ sc->rxfilter = *total_flags;
spin_unlock_bh(&sc->chan_lock);
ath9k_ps_wakeup(sc);
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 6c75fb1..5f72c65 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -389,31 +389,31 @@ u32 ath_calcrxfilter(struct ath_softc *sc)
spin_lock_bh(&sc->chan_lock);
- if (sc->cur_chan->rxfilter & FIF_PROBE_REQ)
+ if (sc->rxfilter & FIF_PROBE_REQ)
rfilt |= ATH9K_RX_FILTER_PROBEREQ;
if (sc->sc_ah->is_monitoring)
rfilt |= ATH9K_RX_FILTER_PROM;
- if ((sc->cur_chan->rxfilter & FIF_CONTROL) ||
+ if ((sc->rxfilter & FIF_CONTROL) ||
sc->sc_ah->dynack.enabled)
rfilt |= ATH9K_RX_FILTER_CONTROL;
if ((sc->sc_ah->opmode == NL80211_IFTYPE_STATION) &&
(sc->cur_chan->nvifs <= 1) &&
- !(sc->cur_chan->rxfilter & FIF_BCN_PRBRESP_PROMISC))
+ !(sc->rxfilter & FIF_BCN_PRBRESP_PROMISC))
rfilt |= ATH9K_RX_FILTER_MYBEACON;
else
rfilt |= ATH9K_RX_FILTER_BEACON;
if ((sc->sc_ah->opmode == NL80211_IFTYPE_AP) ||
- (sc->cur_chan->rxfilter & FIF_PSPOLL))
+ (sc->rxfilter & FIF_PSPOLL))
rfilt |= ATH9K_RX_FILTER_PSPOLL;
if (sc->cur_chandef.width != NL80211_CHAN_WIDTH_20_NOHT)
rfilt |= ATH9K_RX_FILTER_COMP_BAR;
- if (sc->cur_chan->nvifs > 1 || (sc->cur_chan->rxfilter & FIF_OTHER_BSS)) {
+ if (sc->cur_chan->nvifs > 1 || (sc->rxfilter & FIF_OTHER_BSS)) {
/* This is needed for older chips */
if (sc->sc_ah->hw_version.macVersion <= AR_SREV_VERSION_9160)
rfilt |= ATH9K_RX_FILTER_PROM;
@@ -878,7 +878,7 @@ static int ath9k_rx_skb_preprocess(struct ath_softc *sc,
*/
spin_lock_bh(&sc->chan_lock);
if (!ath9k_cmn_rx_accept(common, hdr, rx_status, rx_stats, decrypt_error,
- sc->cur_chan->rxfilter)) {
+ sc->rxfilter)) {
spin_unlock_bh(&sc->chan_lock);
return -EINVAL;
}
--
1.9.1
In case we will get ROC abort we should not call
ieee80211_remain_on_channel_expired().
In other case I hit such warning on MIPS and
p2p negotiation failed (tested with use_chanctx=1).
ath: phy0: Starting RoC period
ath: phy0: Channel definition created: 2412 MHz
ath: phy0: Assigned next_chan to 2412 MHz
ath: phy0: Offchannel duration for chan 2412 MHz : 506632
ath: phy0: ath_chanctx_set_next: current: 2412 MHz, next: 2412 MHz
ath: phy0: Stopping current chanctx: 2412
ath: phy0: Flush timeout: 200
ath: phy0: ath_chanctx_set_next: Set channel 2412 MHz
ath: phy0: Set channel: 2412 MHz width: 0
ath: phy0: Reset to 2412 MHz, HT40: 0 fastcc: 0
ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_TSF_TIMER, state: ATH_CHANCTX_STATE_IDLE
ath: phy0: ath_offchannel_channel_change: offchannel state: ATH_OFFCHANNEL_ROC_START
ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_SWITCH, state: ATH_CHANCTX_STATE_IDLE
ath: phy0: Cancel RoC
ath: phy0: RoC aborted
ath: phy0: RoC request on vif: 00:03:7f:4e:a0:cd, type: 1 duration: 500
ath: phy0: Starting RoC period
ath: phy0: Channel definition created: 2412 MHz
ath: phy0: Assigned next_chan to 2412 MHz
ath: phy0: Offchannel duration for chan 2412 MHz : 506705
ath: phy0: ath_chanctx_set_next: current: 2412 MHz, next: 2412 MHz
ath: phy0: ath_offchannel_channel_change: offchannel state: ATH_OFFCHANNEL_ROC_START
ath: phy0: cur_chan: 2412 MHz, event: ATH_CHANCTX_EVENT_SWITCH, state: ATH_CHANCTX_STATE_IDLE
------------[ cut here ]------------
WARNING: CPU: 0 PID: 3312 at drivers/net/wireless/ath/ath9k/main.c:2319
Modules linked in: ath9k ath9k_common ath9k_hw ath mac80211 cfg80211
Signed-off-by: Janusz Dziedzic <[email protected]>
---
drivers/net/wireless/ath/ath9k/channel.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
index 2066650..e211325 100644
--- a/drivers/net/wireless/ath/ath9k/channel.c
+++ b/drivers/net/wireless/ath/ath9k/channel.c
@@ -926,7 +926,8 @@ void ath_roc_complete(struct ath_softc *sc, bool abort)
sc->offchannel.roc_vif = NULL;
sc->offchannel.roc_chan = NULL;
- ieee80211_remain_on_channel_expired(sc->hw);
+ if (!abort)
+ ieee80211_remain_on_channel_expired(sc->hw);
ath_offchannel_next(sc);
ath9k_ps_restore(sc);
}
--
1.9.1