On Mon, Jun 22, 2015 at 6:32 PM, Michal Kazior <michal.kazior@xxxxxxxxx> wrote: > On 22 June 2015 at 13:56, Krishna Chaitanya <chaitanya.mgit@xxxxxxxxx> wrote: >> On Mon, Jun 22, 2015 at 5:13 PM, Janusz Dziedzic >> <janusz.dziedzic@xxxxxxxxx> 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 <janusz.dziedzic@xxxxxxxxx> >>> --- >>> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
