Message ID | 1392318594-32540-1-git-send-email-greearb@candelatech.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
greearb@candelatech.com writes: > From: Ben Greear <greearb@candelatech.com> > > Properly clean up driver state in case firmware fails > to start scan for some reason. > > Signed-off-by: Ben Greear <greearb@candelatech.com> Why just RFC? And "ath10k:" prefix missing. > --- a/drivers/net/wireless/ath/ath10k/wmi.c > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -770,7 +770,25 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb) > ath10k_dbg(ATH10K_DBG_WMI, "WMI_SCAN_EVENT_PREEMPTED\n"); > break; > case WMI_SCAN_EVENT_START_FAILED: > - ath10k_dbg(ATH10K_DBG_WMI, "WMI_SCAN_EVENT_START_FAILED\n"); > + ath10k_warn("WMI_SCAN_EVENT_START_FAILED, reason: %i\n", reason); "scan failed to start: %i\n" > + ar->scan_channel = NULL; > + if (!ar->scan.in_progress) { > + ath10k_warn("scan-start-failed: no scan requested, ignoring\n"); "scan failed to start but no scan requested, ignoring\n"
On 13 February 2014 20:09, <greearb@candelatech.com> wrote: > From: Ben Greear <greearb@candelatech.com> > > Properly clean up driver state in case firmware fails > to start scan for some reason. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > drivers/net/wireless/ath/ath10k/wmi.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > index 20f7c79..a5be0d3 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi.c > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -770,7 +770,25 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb) > ath10k_dbg(ATH10K_DBG_WMI, "WMI_SCAN_EVENT_PREEMPTED\n"); > break; > case WMI_SCAN_EVENT_START_FAILED: > - ath10k_dbg(ATH10K_DBG_WMI, "WMI_SCAN_EVENT_START_FAILED\n"); > + ath10k_warn("WMI_SCAN_EVENT_START_FAILED, reason: %i\n", reason); > + > + ar->scan_channel = NULL; > + if (!ar->scan.in_progress) { > + ath10k_warn("scan-start-failed: no scan requested, ignoring\n"); > + break; > + } > + > + if (ar->scan.is_roc) { > + ath10k_offchan_tx_purge(ar); > + > + if (!ar->scan.aborting) > + ieee80211_remain_on_channel_expired(ar->hw); > + } else { > + ieee80211_scan_completed(ar->hw, ar->scan.aborting); > + } > + > + del_timer(&ar->scan.timeout); > + ar->scan.in_progress = false; > break; > default: > break; We already wait for EVENT_STARTED in mac.c (see ath10k_start_scan) and clean up stuff (ath10k_abort_scan). Why not add the missing bits in there? Or is it possible to get EVENT_START_FAILED *after* EVENT_STARTED? Or am I missing something else here? Micha? -- 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 02/13/2014 10:47 PM, Michal Kazior wrote: > On 13 February 2014 20:09, <greearb@candelatech.com> wrote: >> From: Ben Greear <greearb@candelatech.com> >> >> Properly clean up driver state in case firmware fails >> to start scan for some reason. >> >> Signed-off-by: Ben Greear <greearb@candelatech.com> >> --- >> drivers/net/wireless/ath/ath10k/wmi.c | 20 +++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c >> index 20f7c79..a5be0d3 100644 >> --- a/drivers/net/wireless/ath/ath10k/wmi.c >> +++ b/drivers/net/wireless/ath/ath10k/wmi.c >> @@ -770,7 +770,25 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb) >> ath10k_dbg(ATH10K_DBG_WMI, "WMI_SCAN_EVENT_PREEMPTED\n"); >> break; >> case WMI_SCAN_EVENT_START_FAILED: >> - ath10k_dbg(ATH10K_DBG_WMI, "WMI_SCAN_EVENT_START_FAILED\n"); >> + ath10k_warn("WMI_SCAN_EVENT_START_FAILED, reason: %i\n", reason); >> + >> + ar->scan_channel = NULL; >> + if (!ar->scan.in_progress) { >> + ath10k_warn("scan-start-failed: no scan requested, ignoring\n"); >> + break; >> + } >> + >> + if (ar->scan.is_roc) { >> + ath10k_offchan_tx_purge(ar); >> + >> + if (!ar->scan.aborting) >> + ieee80211_remain_on_channel_expired(ar->hw); >> + } else { >> + ieee80211_scan_completed(ar->hw, ar->scan.aborting); >> + } >> + >> + del_timer(&ar->scan.timeout); >> + ar->scan.in_progress = false; >> break; >> default: >> break; > > We already wait for EVENT_STARTED in mac.c (see ath10k_start_scan) and > clean up stuff (ath10k_abort_scan). Why not add the missing bits in > there? Or is it possible to get EVENT_START_FAILED *after* > EVENT_STARTED? Or am I missing something else here? I think a lot of this would be firmware dependent, and might change between various versions of the firmware. It seems to me we should handle this case and do cleanup just to be safe, but maybe cleanup is needed in failure case of ath10k_start_scan as well? Thanks, Ben
On 14 February 2014 17:07, Ben Greear <greearb@candelatech.com> wrote: > On 02/13/2014 10:47 PM, Michal Kazior wrote: >> >> On 13 February 2014 20:09, <greearb@candelatech.com> wrote: >>> >>> From: Ben Greear <greearb@candelatech.com> >>> >>> Properly clean up driver state in case firmware fails >>> to start scan for some reason. >>> >>> Signed-off-by: Ben Greear <greearb@candelatech.com> >>> --- >>> drivers/net/wireless/ath/ath10k/wmi.c | 20 +++++++++++++++++++- >>> 1 file changed, 19 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c >>> b/drivers/net/wireless/ath/ath10k/wmi.c >>> index 20f7c79..a5be0d3 100644 >>> --- a/drivers/net/wireless/ath/ath10k/wmi.c >>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c >>> @@ -770,7 +770,25 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, >>> struct sk_buff *skb) >>> ath10k_dbg(ATH10K_DBG_WMI, >>> "WMI_SCAN_EVENT_PREEMPTED\n"); >>> break; >>> case WMI_SCAN_EVENT_START_FAILED: >>> - ath10k_dbg(ATH10K_DBG_WMI, >>> "WMI_SCAN_EVENT_START_FAILED\n"); >>> + ath10k_warn("WMI_SCAN_EVENT_START_FAILED, reason: %i\n", >>> reason); >>> + >>> + ar->scan_channel = NULL; >>> + if (!ar->scan.in_progress) { >>> + ath10k_warn("scan-start-failed: no scan >>> requested, ignoring\n"); >>> + break; >>> + } >>> + >>> + if (ar->scan.is_roc) { >>> + ath10k_offchan_tx_purge(ar); >>> + >>> + if (!ar->scan.aborting) >>> + >>> ieee80211_remain_on_channel_expired(ar->hw); >>> + } else { >>> + ieee80211_scan_completed(ar->hw, >>> ar->scan.aborting); >>> + } >>> + >>> + del_timer(&ar->scan.timeout); >>> + ar->scan.in_progress = false; >>> break; >>> default: >>> break; >> >> >> We already wait for EVENT_STARTED in mac.c (see ath10k_start_scan) and >> clean up stuff (ath10k_abort_scan). Why not add the missing bits in >> there? Or is it possible to get EVENT_START_FAILED *after* >> EVENT_STARTED? Or am I missing something else here? > > > I think a lot of this would be firmware dependent, and might change between > various versions of the firmware. It doesn't make any sense. That would suggest a really ugly firmware bug. Did you see this (i.e. START_FAILED after STARTED) on 636 or 10.1.467? > It seems to me we should handle this case and do cleanup just to be safe, > but maybe cleanup is needed in failure case of ath10k_start_scan as well? If you really get START_FAILED then you shouldn't have received STARTED before that. ath10k_start_scan() already waits for the STARTED event with a timeout and if it fails it triggers a cleanup. If it doesn't work for you then what perhaps needs to be fixed is the current cleanup code? Micha? -- 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 02/16/2014 11:42 PM, Michal Kazior wrote: > On 14 February 2014 17:07, Ben Greear <greearb@candelatech.com> wrote: >>> We already wait for EVENT_STARTED in mac.c (see ath10k_start_scan) and >>> clean up stuff (ath10k_abort_scan). Why not add the missing bits in >>> there? Or is it possible to get EVENT_START_FAILED *after* >>> EVENT_STARTED? Or am I missing something else here? >> >> >> I think a lot of this would be firmware dependent, and might change between >> various versions of the firmware. > > It doesn't make any sense. That would suggest a really ugly firmware > bug. Did you see this (i.e. START_FAILED after STARTED) on 636 or > 10.1.467? I am working on a 10.1.389 release currently...will move forward when I can get access to newer source code. >> It seems to me we should handle this case and do cleanup just to be safe, >> but maybe cleanup is needed in failure case of ath10k_start_scan as well? > > If you really get START_FAILED then you shouldn't have received > STARTED before that. ath10k_start_scan() already waits for the STARTED > event with a timeout and if it fails it triggers a cleanup. If it > doesn't work for you then what perhaps needs to be fixed is the > current cleanup code? I am not certain the patch is needed. I was looking at my firmware and it appeared that I could hit the START_FAILED case, but perhaps it was not really possible, and maybe newer firmware keeps it from happening entirely. Thanks, Ben
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c index 20f7c79..a5be0d3 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.c +++ b/drivers/net/wireless/ath/ath10k/wmi.c @@ -770,7 +770,25 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb) ath10k_dbg(ATH10K_DBG_WMI, "WMI_SCAN_EVENT_PREEMPTED\n"); break; case WMI_SCAN_EVENT_START_FAILED: - ath10k_dbg(ATH10K_DBG_WMI, "WMI_SCAN_EVENT_START_FAILED\n"); + ath10k_warn("WMI_SCAN_EVENT_START_FAILED, reason: %i\n", reason); + + ar->scan_channel = NULL; + if (!ar->scan.in_progress) { + ath10k_warn("scan-start-failed: no scan requested, ignoring\n"); + break; + } + + if (ar->scan.is_roc) { + ath10k_offchan_tx_purge(ar); + + if (!ar->scan.aborting) + ieee80211_remain_on_channel_expired(ar->hw); + } else { + ieee80211_scan_completed(ar->hw, ar->scan.aborting); + } + + del_timer(&ar->scan.timeout); + ar->scan.in_progress = false; break; default: break;