Patchwork mac80211: fix deferred hardware scan requests

login
register
mail settings
Submitter Chase Douglas
Date April 16, 2010, 11:18 p.m.
Message ID <1271459898-31305-1-git-send-email-chase.douglas@canonical.com>
Download mbox | patch
Permalink /patch/50359/
State Awaiting Upstream
Delegated to: Andy Whitcroft
Headers show

Comments

Chase Douglas - April 16, 2010, 11:18 p.m.
From: Johannes Berg <johannes@sipsolutions.net>

Reinette found the reason for the warnings that
happened occasionally when a hw-offloaded scan
finished; her description of the problem:

  mac80211 will defer the handling of scan requests if it is
  busy with management work at the time. The scan requests
  are deferred and run after the work has completed. When
  this occurs there are currently two problems.

  * The scan request for hardware scan is not fully populated
    with the band and channels to scan not initialized.

  * When the scan is queued the state is not correctly updated
    to reflect that a scan is in progress. The problem here is
    that when the driver completes the scan and calls
    ieee80211_scan_completed() a warning will be triggered
    since mac80211 was not aware that a scan was in progress.

The reason is that the queued scan work will start
the hw scan right away when the hw_scan_req struct
has already been allocated. However, in the first
pass it will not have been filled, which happens
at the same time as setting the bits. To fix this,
simply move the allocation after the pending work
test as well, so that the first iteration of the
scan work will call __ieee80211_start_scan() even
in the hardware scan case.

Bug-identified-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
(cherry picked from commit c0ce77b8323c1a0d4eeef97caf16c0ea971222a9)

BugLink: http://bugs.launchpad.net/bugs/528688

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 net/mac80211/scan.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)
Stefan Bader - April 16, 2010, 11:29 p.m.
Despite of likely knowing the intention I need to play dumb: what is the
intention of this? There is not much indication which release you target,
neither much help about the impact of the bug or the regression potential of the
fix. What is the use case to trigger it.
As we basically are locked down with the release kernel, we are now doing all
fixes in SRU mode which requires a bit more explanation on things (see Leann's
recent submissions).
If this only triggers a warning but still leaves things workable, this should
hopefully get back via stable. As normal SRU updates are in general limited to
more serious issues.

Stefan

Chase Douglas wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> 
> Reinette found the reason for the warnings that
> happened occasionally when a hw-offloaded scan
> finished; her description of the problem:
> 
>   mac80211 will defer the handling of scan requests if it is
>   busy with management work at the time. The scan requests
>   are deferred and run after the work has completed. When
>   this occurs there are currently two problems.
> 
>   * The scan request for hardware scan is not fully populated
>     with the band and channels to scan not initialized.
> 
>   * When the scan is queued the state is not correctly updated
>     to reflect that a scan is in progress. The problem here is
>     that when the driver completes the scan and calls
>     ieee80211_scan_completed() a warning will be triggered
>     since mac80211 was not aware that a scan was in progress.
> 
> The reason is that the queued scan work will start
> the hw scan right away when the hw_scan_req struct
> has already been allocated. However, in the first
> pass it will not have been filled, which happens
> at the same time as setting the bits. To fix this,
> simply move the allocation after the pending work
> test as well, so that the first iteration of the
> scan work will call __ieee80211_start_scan() even
> in the hardware scan case.
> 
> Bug-identified-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> (cherry picked from commit c0ce77b8323c1a0d4eeef97caf16c0ea971222a9)
> 
> BugLink: http://bugs.launchpad.net/bugs/528688
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  net/mac80211/scan.c |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
> index 1a41909..fd6411d 100644
> --- a/net/mac80211/scan.c
> +++ b/net/mac80211/scan.c
> @@ -408,6 +408,16 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
>  	if (local->scan_req)
>  		return -EBUSY;
>  
> +	if (req != local->int_scan_req &&
> +	    sdata->vif.type == NL80211_IFTYPE_STATION &&
> +	    !list_empty(&ifmgd->work_list)) {
> +		/* actually wait for the work it's doing to finish/time out */
> +		set_bit(IEEE80211_STA_REQ_SCAN, &ifmgd->request);
> +		local->scan_req = req;
> +		local->scan_sdata = sdata;
> +		return 0;
> +	}
> +
>  	if (local->ops->hw_scan) {
>  		u8 *ies;
>  		int ielen;
> @@ -428,14 +438,6 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
>  	local->scan_req = req;
>  	local->scan_sdata = sdata;
>  
> -	if (req != local->int_scan_req &&
> -	    sdata->vif.type == NL80211_IFTYPE_STATION &&
> -	    !list_empty(&ifmgd->work_list)) {
> -		/* actually wait for the work it's doing to finish/time out */
> -		set_bit(IEEE80211_STA_REQ_SCAN, &ifmgd->request);
> -		return 0;
> -	}
> -
>  	if (local->ops->hw_scan)
>  		__set_bit(SCAN_HW_SCANNING, &local->scanning);
>  	else
Chase Douglas - April 16, 2010, 11:38 p.m.
On Fri, Apr 16, 2010 at 4:29 PM, Stefan Bader
<stefan.bader@canonical.com> wrote:
> Despite of likely knowing the intention I need to play dumb: what is the
> intention of this? There is not much indication which release you target,
> neither much help about the impact of the bug or the regression potential of the
> fix. What is the use case to trigger it.
> As we basically are locked down with the release kernel, we are now doing all
> fixes in SRU mode which requires a bit more explanation on things (see Leann's
> recent submissions).
> If this only triggers a warning but still leaves things workable, this should
> hopefully get back via stable. As normal SRU updates are in general limited to
> more serious issues.

Ok. I saw Leann with the SRU and forgot we are in that mode now. So do
we prefer the SRU justification in the patch email like Leann
provided, or in the bug report. The wiki page indicates that it should
be done in the bug report, but maybe we do things different in the
kernel?

I think the issue doesn't take down the system, so once apport is
disabled for the release this probably won't be a big deal. I think we
should probably just wait for it to come from -stable assuming it
isn't rejected.

-- Chase
Stefan Bader - April 16, 2010, 11:42 p.m.
Chase Douglas wrote:
> On Fri, Apr 16, 2010 at 4:29 PM, Stefan Bader
> <stefan.bader@canonical.com> wrote:
>> Despite of likely knowing the intention I need to play dumb: what is the
>> intention of this? There is not much indication which release you target,
>> neither much help about the impact of the bug or the regression potential of the
>> fix. What is the use case to trigger it.
>> As we basically are locked down with the release kernel, we are now doing all
>> fixes in SRU mode which requires a bit more explanation on things (see Leann's
>> recent submissions).
>> If this only triggers a warning but still leaves things workable, this should
>> hopefully get back via stable. As normal SRU updates are in general limited to
>> more serious issues.
> 
> Ok. I saw Leann with the SRU and forgot we are in that mode now. So do
> we prefer the SRU justification in the patch email like Leann
> provided, or in the bug report. The wiki page indicates that it should
> be done in the bug report, but maybe we do things different in the
> kernel?

In the bug report primarily (best by modifying the description so it is right at
the top). But I found it good to be in the mail as well (usually right copied
from the bug report) as it saves one from needing to go to launchpad to
understand the reasoning.

> I think the issue doesn't take down the system, so once apport is
> disabled for the release this probably won't be a big deal. I think we
> should probably just wait for it to come from -stable assuming it
> isn't rejected.
> 
> -- Chase

Ok thanks. Stefan
Luis Rodriguez - April 20, 2010, 7:28 p.m.
On Fri, Apr 16, 2010 at 4:42 PM, Stefan Bader
<stefan.bader@canonical.com> wrote:
> Chase Douglas wrote:
>> On Fri, Apr 16, 2010 at 4:29 PM, Stefan Bader
>> <stefan.bader@canonical.com> wrote:
>>> Despite of likely knowing the intention I need to play dumb: what is the
>>> intention of this? There is not much indication which release you target,
>>> neither much help about the impact of the bug or the regression potential of the
>>> fix. What is the use case to trigger it.
>>> As we basically are locked down with the release kernel, we are now doing all
>>> fixes in SRU mode which requires a bit more explanation on things (see Leann's
>>> recent submissions).
>>> If this only triggers a warning but still leaves things workable, this should
>>> hopefully get back via stable. As normal SRU updates are in general limited to
>>> more serious issues.
>>
>> Ok. I saw Leann with the SRU and forgot we are in that mode now. So do
>> we prefer the SRU justification in the patch email like Leann
>> provided, or in the bug report. The wiki page indicates that it should
>> be done in the bug report, but maybe we do things different in the
>> kernel?
>
> In the bug report primarily (best by modifying the description so it is right at
> the top). But I found it good to be in the mail as well (usually right copied
> from the bug report) as it saves one from needing to go to launchpad to
> understand the reasoning.
>
>> I think the issue doesn't take down the system, so once apport is
>> disabled for the release this probably won't be a big deal. I think we
>> should probably just wait for it to come from -stable assuming it
>> isn't rejected.
>>
>> -- Chase
>
> Ok thanks. Stefan

If there are good reasons to push this into a distribution for stable
kernel inclusion might as well send the same request to
stable@kernel.org and cc linux-wireless for the request. You should
provide the details for your justification. This would then trickle
down to Ubuntu from the stable kernel series.

If this issues is causing to fill up your logs that is good enough
justification for a stable fix propagation.

  Luis
Chase Douglas - April 20, 2010, 7:43 p.m.
On Tue, Apr 20, 2010 at 3:28 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> If there are good reasons to push this into a distribution for stable
> kernel inclusion might as well send the same request to
> stable@kernel.org and cc linux-wireless for the request. You should
> provide the details for your justification. This would then trickle
> down to Ubuntu from the stable kernel series.
>
> If this issues is causing to fill up your logs that is good enough
> justification for a stable fix propagation.

Already done :). I sent it to stable on Friday, and we'll wait for it
to come from -stable instead of applying it directly to Ubuntu.

Thanks

-- Chase
Luis Rodriguez - April 22, 2010, 4:33 a.m.
On Tue, Apr 20, 2010 at 12:43 PM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On Tue, Apr 20, 2010 at 3:28 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>> If there are good reasons to push this into a distribution for stable
>> kernel inclusion might as well send the same request to
>> stable@kernel.org and cc linux-wireless for the request. You should
>> provide the details for your justification. This would then trickle
>> down to Ubuntu from the stable kernel series.
>>
>> If this issues is causing to fill up your logs that is good enough
>> justification for a stable fix propagation.
>
> Already done :). I sent it to stable on Friday, and we'll wait for it
> to come from -stable instead of applying it directly to Ubuntu.

Did you cc linux-wireless by chance?

  Luis
Chase Douglas - April 22, 2010, 1:35 p.m.
On Thu, Apr 22, 2010 at 12:33 AM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Tue, Apr 20, 2010 at 12:43 PM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>> On Tue, Apr 20, 2010 at 3:28 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>>> If there are good reasons to push this into a distribution for stable
>>> kernel inclusion might as well send the same request to
>>> stable@kernel.org and cc linux-wireless for the request. You should
>>> provide the details for your justification. This would then trickle
>>> down to Ubuntu from the stable kernel series.
>>>
>>> If this issues is causing to fill up your logs that is good enough
>>> justification for a stable fix propagation.
>>
>> Already done :). I sent it to stable on Friday, and we'll wait for it
>> to come from -stable instead of applying it directly to Ubuntu.
>
> Did you cc linux-wireless by chance?

No, it has been CC'd to John Linville, Johannes Berg, and Reinette
Chatre. I can forward it to linux-wireless, but I don't think that's
how most stable commits are handled (I could be wrong, I'm new to
this). It's been queued up for the next stable drop.

-- Chase
Luis Rodriguez - April 23, 2010, 10:37 p.m.
On Thu, Apr 22, 2010 at 6:35 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On Thu, Apr 22, 2010 at 12:33 AM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>> On Tue, Apr 20, 2010 at 12:43 PM, Chase Douglas
>> <chase.douglas@canonical.com> wrote:
>>> On Tue, Apr 20, 2010 at 3:28 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>>>> If there are good reasons to push this into a distribution for stable
>>>> kernel inclusion might as well send the same request to
>>>> stable@kernel.org and cc linux-wireless for the request. You should
>>>> provide the details for your justification. This would then trickle
>>>> down to Ubuntu from the stable kernel series.
>>>>
>>>> If this issues is causing to fill up your logs that is good enough
>>>> justification for a stable fix propagation.
>>>
>>> Already done :). I sent it to stable on Friday, and we'll wait for it
>>> to come from -stable instead of applying it directly to Ubuntu.
>>
>> Did you cc linux-wireless by chance?
>
> No, it has been CC'd to John Linville, Johannes Berg, and Reinette
> Chatre. I can forward it to linux-wireless, but I don't think that's
> how most stable commits are handled (I could be wrong, I'm new to
> this). It's been queued up for the next stable drop.

Please cc linux-wireless for other stable requests for 802.11.

  Luis
Chase Douglas - April 23, 2010, 11:51 p.m.
On Fri, Apr 23, 2010 at 6:37 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Thu, Apr 22, 2010 at 6:35 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>> On Thu, Apr 22, 2010 at 12:33 AM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>>> On Tue, Apr 20, 2010 at 12:43 PM, Chase Douglas
>>> <chase.douglas@canonical.com> wrote:
>>>> On Tue, Apr 20, 2010 at 3:28 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>>>>> If there are good reasons to push this into a distribution for stable
>>>>> kernel inclusion might as well send the same request to
>>>>> stable@kernel.org and cc linux-wireless for the request. You should
>>>>> provide the details for your justification. This would then trickle
>>>>> down to Ubuntu from the stable kernel series.
>>>>>
>>>>> If this issues is causing to fill up your logs that is good enough
>>>>> justification for a stable fix propagation.
>>>>
>>>> Already done :). I sent it to stable on Friday, and we'll wait for it
>>>> to come from -stable instead of applying it directly to Ubuntu.
>>>
>>> Did you cc linux-wireless by chance?
>>
>> No, it has been CC'd to John Linville, Johannes Berg, and Reinette
>> Chatre. I can forward it to linux-wireless, but I don't think that's
>> how most stable commits are handled (I could be wrong, I'm new to
>> this). It's been queued up for the next stable drop.
>
> Please cc linux-wireless for other stable requests for 802.11.

Sorry, I should have realized you were one of the maintainers of the
wireless cards. Certainly you're more up to date on the correct
procedures for these things :).

I've forwarded the review email to the linux-wireless mailing list,
and will cc linux-wireless from the start in the future.

Thanks!

-- Chase

Patch

diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 1a41909..fd6411d 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -408,6 +408,16 @@  static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	if (local->scan_req)
 		return -EBUSY;
 
+	if (req != local->int_scan_req &&
+	    sdata->vif.type == NL80211_IFTYPE_STATION &&
+	    !list_empty(&ifmgd->work_list)) {
+		/* actually wait for the work it's doing to finish/time out */
+		set_bit(IEEE80211_STA_REQ_SCAN, &ifmgd->request);
+		local->scan_req = req;
+		local->scan_sdata = sdata;
+		return 0;
+	}
+
 	if (local->ops->hw_scan) {
 		u8 *ies;
 		int ielen;
@@ -428,14 +438,6 @@  static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	local->scan_req = req;
 	local->scan_sdata = sdata;
 
-	if (req != local->int_scan_req &&
-	    sdata->vif.type == NL80211_IFTYPE_STATION &&
-	    !list_empty(&ifmgd->work_list)) {
-		/* actually wait for the work it's doing to finish/time out */
-		set_bit(IEEE80211_STA_REQ_SCAN, &ifmgd->request);
-		return 0;
-	}
-
 	if (local->ops->hw_scan)
 		__set_bit(SCAN_HW_SCANNING, &local->scanning);
 	else