| Message ID | 1271459898-31305-1-git-send-email-chase.douglas@canonical.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Andy Whitcroft |
| Headers | show |
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
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
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
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
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
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
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
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
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
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