iwl3945's scan_completed calls into the mac80211 stack which triggers a
warn on if there is no scan outstanding.
This can be avoided by not calling scan_completed but abort_scan in
iwl3945_request_scan in the done: branch of the function which is used
as an error out.
The done: branch seems to be an error-out branch, as, for example, if
iwl_is_ready(priv) returns false the done: branch is executed.
NOTE:
I'm not familiar with the driver at all.
I just quickly scanned as a reaction to
https://bugzilla.kernel.org/show_bug.cgi?id=17722
the users of scan_completed in the iwl3945 driver and noted the odd
discrepancy between the comment above this instance and the comment in
mac80211 scan_completed function.
Signed-off-by: Florian Mickler <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn-lib.c | 2 +-
drivers/net/wireless/iwlwifi/iwl3945-base.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
index 9dd9e64..8fd00a6 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
@@ -1411,7 +1411,7 @@ void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
clear_bit(STATUS_SCAN_HW, &priv->status);
clear_bit(STATUS_SCANNING, &priv->status);
/* inform mac80211 scan aborted */
- queue_work(priv->workqueue, &priv->scan_completed);
+ queue_work(priv->workqueue, &priv->abort_scan);
}
int iwlagn_manage_ibss_station(struct iwl_priv *priv,
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 59a308b..d31661c 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -3018,7 +3018,7 @@ void iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
clear_bit(STATUS_SCANNING, &priv->status);
/* inform mac80211 scan aborted */
- queue_work(priv->workqueue, &priv->scan_completed);
+ queue_work(priv->workqueue, &priv->abort_scan);
}
static void iwl3945_bg_restart(struct work_struct *data)
--
1.7.3
On Fri, 2010-09-24 at 09:22 -0700, Florian Mickler wrote:
> iwl3945's scan_completed calls into the mac80211 stack which triggers a
> warn on if there is no scan outstanding.
>
> This can be avoided by not calling scan_completed but abort_scan in
> iwl3945_request_scan in the done: branch of the function which is used
> as an error out.
>
> The done: branch seems to be an error-out branch, as, for example, if
> iwl_is_ready(priv) returns false the done: branch is executed.
>
> NOTE:
> I'm not familiar with the driver at all.
> I just quickly scanned as a reaction to
>
> https://bugzilla.kernel.org/show_bug.cgi?id=17722
>
> the users of scan_completed in the iwl3945 driver and noted the odd
> discrepancy between the comment above this instance and the comment in
> mac80211 scan_completed function.
> Signed-off-by: Florian Mickler <[email protected]>
Acked-by: Wey-Yi Guy <[email protected]>
> ---
go into wireless-2.6 and stable only, scan fix already in
wireless-next-2.6
Thanks
Wey
> drivers/net/wireless/iwlwifi/iwl-agn-lib.c | 2 +-
> drivers/net/wireless/iwlwifi/iwl3945-base.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> index 9dd9e64..8fd00a6 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> @@ -1411,7 +1411,7 @@ void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
> clear_bit(STATUS_SCAN_HW, &priv->status);
> clear_bit(STATUS_SCANNING, &priv->status);
> /* inform mac80211 scan aborted */
> - queue_work(priv->workqueue, &priv->scan_completed);
> + queue_work(priv->workqueue, &priv->abort_scan);
> }
>
> int iwlagn_manage_ibss_station(struct iwl_priv *priv,
> diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
> index 59a308b..d31661c 100644
> --- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
> +++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
> @@ -3018,7 +3018,7 @@ void iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
> clear_bit(STATUS_SCANNING, &priv->status);
>
> /* inform mac80211 scan aborted */
> - queue_work(priv->workqueue, &priv->scan_completed);
> + queue_work(priv->workqueue, &priv->abort_scan);
> }
>
> static void iwl3945_bg_restart(struct work_struct *data)
On Fri, 24 Sep 2010 12:51:08 -0700
"Guy, Wey-Yi" <[email protected]> wrote:
> Hi Florian,
>
> Thanks for sending the patch, the scan operation in iwlwifi just being
> modified and the changes you have no longer apply.please check the
> latest kernel.
>
> Thanks
> Wey
>
Ok, I checked out
git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-2.6.git
and it definitely looks better in there. This patch shouldn't be
necessary for that.
Do you plan on submitting this for 2.6.36 or .37?
I think my patch should fix the instance reported in the bug in a less
invasive manner.
Only remaining question is why it did only show up now. But that's
probably due to the workqueue changes.
Regards,
Flo
Hi Florian,
On Fri, 2010-09-24 at 09:22 -0700, Florian Mickler wrote:
> iwl3945's scan_completed calls into the mac80211 stack which triggers a
> warn on if there is no scan outstanding.
>
> This can be avoided by not calling scan_completed but abort_scan in
> iwl3945_request_scan in the done: branch of the function which is used
> as an error out.
>
> The done: branch seems to be an error-out branch, as, for example, if
> iwl_is_ready(priv) returns false the done: branch is executed.
>
> NOTE:
> I'm not familiar with the driver at all.
> I just quickly scanned as a reaction to
>
> https://bugzilla.kernel.org/show_bug.cgi?id=17722
>
> the users of scan_completed in the iwl3945 driver and noted the odd
> discrepancy between the comment above this instance and the comment in
> mac80211 scan_completed function.
>
> Signed-off-by: Florian Mickler <[email protected]>
> ---
> drivers/net/wireless/iwlwifi/iwl-agn-lib.c | 2 +-
> drivers/net/wireless/iwlwifi/iwl3945-base.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> index 9dd9e64..8fd00a6 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> @@ -1411,7 +1411,7 @@ void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
> clear_bit(STATUS_SCAN_HW, &priv->status);
> clear_bit(STATUS_SCANNING, &priv->status);
> /* inform mac80211 scan aborted */
> - queue_work(priv->workqueue, &priv->scan_completed);
> + queue_work(priv->workqueue, &priv->abort_scan);
> }
>
> int iwlagn_manage_ibss_station(struct iwl_priv *priv,
> diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
> index 59a308b..d31661c 100644
> --- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
> +++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
> @@ -3018,7 +3018,7 @@ void iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
> clear_bit(STATUS_SCANNING, &priv->status);
>
> /* inform mac80211 scan aborted */
> - queue_work(priv->workqueue, &priv->scan_completed);
> + queue_work(priv->workqueue, &priv->abort_scan);
> }
>
> static void iwl3945_bg_restart(struct work_struct *data)
Thanks for sending the patch, the scan operation in iwlwifi just being
modified and the changes you have no longer apply.please check the
latest kernel.
Thanks
Wey
Hi Florian,
Agree the patch is needed for .35 and .36.
Thanks
Wey
-----Original Message-----
From: Florian Mickler [mailto:[email protected]]
Sent: Sunday, September 26, 2010 11:14 PM
To: Guy, Wey-Yi W
Cc: [email protected]; Chatre, Reinette; Intel Linux Wireless; John W. Linville; Berg, Johannes; Zhu Yi; Cahill, Ben M; [email protected]; [email protected]
Subject: Re: [PATCH] iwl3945: queue the right work if the scan needs to be aborted
On Fri, 24 Sep 2010 12:51:08 -0700
"Guy, Wey-Yi" <[email protected]> wrote:
> Hi Florian,
>
> Thanks for sending the patch, the scan operation in iwlwifi just being
> modified and the changes you have no longer apply.please check the
> latest kernel.
>
> Thanks
> Wey
>
Ok, I checked out
git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-2.6.git
and it definitely looks better in there. This patch shouldn't be
necessary for that.
Do you plan on submitting this for 2.6.36 or .37?
I think my patch should fix the instance reported in the bug in a less
invasive manner.
Only remaining question is why it did only show up now. But that's
probably due to the workqueue changes.
Regards,
Flo
On Tue, 5 Oct 2010 12:43:58 +0200
Stanislaw Gruszka <[email protected]> wrote:
> > >
> > > However, I do not think we should go with that to -stable (below
> > > 2.6.36). IIRC warnings showed up in current 2.6.36-rc, because of
> > > some other changes in the code.
> > >
> > > Stanislaw
Hello stable,
scrap this. I handed out a brown paper bag bug fix and if that doesn't
get into 2.6.36, it probably should go into 2.6.36.y. But let's see...
Regards,
Flo
On Tue, 5 Oct 2010 10:57:17 +0200
Stanislaw Gruszka <[email protected]> wrote:
> On Tue, Oct 05, 2010 at 08:43:05AM +0200, Florian Mickler wrote:
> > commit e7ee762cf074b0fd8eec483d0cef8fdbf0d04b81
> >
> >
> > Begin forwarded message:
> > On Fri, 2010-09-24 at 09:22 -0700, Florian Mickler wrote:
> > > iwl3945's scan_completed calls into the mac80211 stack which triggers a
> > > warn on if there is no scan outstanding.
> > >
> > > This can be avoided by not calling scan_completed but abort_scan in
> > > iwl3945_request_scan in the done: branch of the function which is used
> > > as an error out.
> > >
> > > The done: branch seems to be an error-out branch, as, for example, if
> > > iwl_is_ready(priv) returns false the done: branch is executed.
> > >
> > > NOTE:
> > > I'm not familiar with the driver at all.
> > > I just quickly scanned as a reaction to
> > >
> > > https://bugzilla.kernel.org/show_bug.cgi?id=17722
> > >
> > > the users of scan_completed in the iwl3945 driver and noted the odd
> > > discrepancy between the comment above this instance and the comment in
> > > mac80211 scan_completed function.
> > > Signed-off-by: Florian Mickler <[email protected]>
> > Acked-by: Wey-Yi Guy <[email protected]>
> > > ---
> > go into wireless-2.6 and stable only, scan fix already in
> > wireless-next-2.6
>
> >
> > Thanks
> > Wey
> >
> > > drivers/net/wireless/iwlwifi/iwl-agn-lib.c | 2 +-
> > > drivers/net/wireless/iwlwifi/iwl3945-base.c | 2 +-
> > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> > > index 9dd9e64..8fd00a6 100644
> > > --- a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> > > +++ b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> > > @@ -1411,7 +1411,7 @@ void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
> > > clear_bit(STATUS_SCAN_HW, &priv->status);
> > > clear_bit(STATUS_SCANNING, &priv->status);
> > > /* inform mac80211 scan aborted */
> > > - queue_work(priv->workqueue, &priv->scan_completed);
> > > + queue_work(priv->workqueue, &priv->abort_scan);
>
> Unfortunately this patch is not right thing to do. If you look at
> abort_scan work, it do nothing if STATUS_SCAN_ABORTING bit is not set.
> That's wrong because we have to complete scan (with abort == true).
> If STATUS_SCAN_ABORTING will be set, abort_work will send scan cancel
> commands to hardware what is wrong if scan was not started yet.
>
> What we can eventually do, except apply iwl-scan rewrite from
> wireless-testing, is something like that:
>
> iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
>
> clear_bit(STATUS_SCAN_HW, &priv->status);
> clear_bit(STATUS_SCANNING, &priv->status);
> /* inform mac80211 scan aborted */
> set_bit(STATUS_SCAN_ABORTING, &priv->status);
> queue_work(priv->workqueue, &priv->scan_completed);
>
> ieee80211_scan_completed
>
> if (!internal) {
> bool aborted = test_bit(STATUS_SCAN_ABORTING, &priv->status);
> ieee80211_scan_completed(priv->hw, aborted);
>
> }
>
> However, I do not think we should go with that to -stable (below
> 2.6.36). IIRC warnings showed up in current 2.6.36-rc, because of
> some other changes in the code.
>
> Stanislaw
Thx for looking at this. I suspect you know the code better than I do.. what is about the
first jump to :done in iwlagn_request_scan()
if (!iwl_is_ready(priv)) {
IWL_WARN(priv, "request scan called when driver not ready.\n");
goto done;
}
Does abort_scan need to do anything in that case?
I can't see where we set up the hardware for scanning in that case.
(I've gone through the codepath coming from the mac80211 hw_scan)
Regards,
Flo
On Tue, Oct 05, 2010 at 12:12:42PM +0200, Florian Mickler wrote:
> > Unfortunately this patch is not right thing to do. If you look at
> > abort_scan work, it do nothing if STATUS_SCAN_ABORTING bit is not set.
> > That's wrong because we have to complete scan (with abort == true).
> > If STATUS_SCAN_ABORTING will be set, abort_work will send scan cancel
> > commands to hardware what is wrong if scan was not started yet.
> >
> > What we can eventually do, except apply iwl-scan rewrite from
> > wireless-testing, is something like that:
> >
> > iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
> >
> > clear_bit(STATUS_SCAN_HW, &priv->status);
> > clear_bit(STATUS_SCANNING, &priv->status);
> > /* inform mac80211 scan aborted */
> > set_bit(STATUS_SCAN_ABORTING, &priv->status);
> > queue_work(priv->workqueue, &priv->scan_completed);
> >
> > ieee80211_scan_completed
> >
> > if (!internal) {
> > bool aborted = test_bit(STATUS_SCAN_ABORTING, &priv->status);
> > ieee80211_scan_completed(priv->hw, aborted);
> >
> > }
> >
> > However, I do not think we should go with that to -stable (below
> > 2.6.36). IIRC warnings showed up in current 2.6.36-rc, because of
> > some other changes in the code.
> >
> > Stanislaw
>
> Thx for looking at this. I suspect you know the code better than I do.. what is about the
> first jump to :done in iwlagn_request_scan()
>
> if (!iwl_is_ready(priv)) {
> IWL_WARN(priv, "request scan called when driver not ready.\n");
> goto done;
> }
>
> Does abort_scan need to do anything in that case?
Yes, because we do not return value to mac80211 about .hw_scan failure,
we always return 0, what is completely wrong. And yes, return error can
be done instead of queueing scan_completed to improve situation - this is
one of the thing, we do in wireless-testing.
> I can't see where we set up the hardware for scanning in that case.
> (I've gone through the codepath coming from the mac80211 hw_scan)
.hw_scan = iwl_mac_hw_scan -> iwl_scan_initiate ->
"priv->cfg->ops->utils->request_scan" = {iwl3945,iwlang}_request_scan
Stanislaw