[PATCH] iwl3945: queue the right work if the scan needs to be aborted

[PATCH] iwl3945: queue the right work if the scan needs to be aborted

[Florian Mickler]

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 <florian-sVu6HhrpSfRAfugRpC6u6w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iwl3945: queue the right work if the scan needs to be aborted

[Guy, Wey-Yi]

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 <florian@mickler.org>
> ---
>  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

Re: [PATCH] iwl3945: queue the right work if the scan needs to be aborted

[Florian Mickler]

On Fri, 24 Sep 2010 12:51:08 -0700
"Guy, Wey-Yi" <wey-yi.w.guy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] iwl3945: queue the right work if the scan needs to be aborted

[Guy, Wey-Yi W]

Hi Florian,

Agree the patch is needed for .35 and .36.

Thanks

Wey

-----Original Message-----
From: Florian Mickler [mailto:florian@mickler.org]
Sent: Sunday, September 26, 2010 11:14 PM
To: Guy, Wey-Yi W
Cc: linux-wireless@vger.kernel.org; Chatre, Reinette; Intel Linux Wireless; John W. Linville; Berg, Johannes; Zhu Yi; Cahill, Ben M; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
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" <wey-yi.w.guy@intel.com> 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

Re: [PATCH] iwl3945: queue the right work if the scan needs to be aborted

[Guy, Wey-Yi]

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 <florian@mickler.org>
Acked-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> ---
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)

Re: [PATCH] iwl3945: queue the right work if the scan needs to be aborted

[Florian Mickler]

On Tue, 5 Oct 2010 10:57:17 +0200
Stanislaw Gruszka <sgruszka@redhat.com> 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 <florian@mickler.org>
> > Acked-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> > > ---
> > 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

Re: [PATCH] iwl3945: queue the right work if the scan needs to be aborted

[Stanislaw Gruszka]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iwl3945: queue the right work if the scan needs to be aborted

[Florian Mickler]

On Tue, 5 Oct 2010 12:43:58 +0200
Stanislaw Gruszka <sgruszka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html