Return-path: Received: from mail-ig0-f182.google.com ([209.85.213.182]:36258 "EHLO mail-ig0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752606AbbFVOB5 (ORCPT ); Mon, 22 Jun 2015 10:01:57 -0400 Received: by igbiq7 with SMTP id iq7so53806341igb.1 for ; Mon, 22 Jun 2015 07:01:57 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1434973385-15865-1-git-send-email-janusz.dziedzic@tieto.com> <1434973385-15865-2-git-send-email-janusz.dziedzic@tieto.com> From: Krishna Chaitanya Date: Mon, 22 Jun 2015 19:31:37 +0530 Message-ID: (sfid-20150622_160201_130624_8D4C802E) Subject: Re: [PATCH 1/3] ath9k: handle RoC abort correctly To: Michal Kazior Cc: Janusz Dziedzic , linux-wireless , ath9k-devel , Felix Fietkau , Sujith Manoharan Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On Mon, Jun 22, 2015 at 6:32 PM, Michal Kazior wrote: > On 22 June 2015 at 13:56, Krishna Chaitanya wrote: >> On Mon, Jun 22, 2015 at 5:13 PM, Janusz Dziedzic >> 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 >>> --- >>> 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