[PATCH] mac80211: cancel scan_work in ieee80211_unregister_hw

[PATCH] mac80211: cancel scan_work in ieee80211_unregister_hw

[Rajkumar Manoharan]

scan_work is never stopped if the device was unplugged during
s/w scan.

Signed-off-by: Rajkumar Manoharan <rmanoharan@atheros.com>
---
 net/mac80211/main.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 30e6a68..d2aa0be 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -964,6 +964,8 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 	unregister_inetaddr_notifier(&local->ifa_notifier);
 #endif
 
+	cancel_delayed_work_sync(&local->scan_work);
+
 	rtnl_lock();
 
 	/*
-- 
1.7.5.1

Re: [PATCH] mac80211: cancel scan_work in ieee80211_unregister_hw

[Johannes Berg]

On Thu, 2011-05-12 at 22:39 +0530, Rajkumar Manoharan wrote:
> scan_work is never stopped if the device was unplugged during
> s/w scan.

How did you come to that conclusion? ieee80211_do_stop() does stop the
scan.

johannes

Re: [PATCH] mac80211: cancel scan_work in ieee80211_unregister_hw

[Rajkumar Manoharan]

On Thu, May 12, 2011 at 10:55:21PM +0530, Johannes Berg wrote:
> On Thu, 2011-05-12 at 22:39 +0530, Rajkumar Manoharan wrote:
> > scan_work is never stopped if the device was unplugged during
> > s/w scan.
> 
> How did you come to that conclusion? ieee80211_do_stop() does stop the
> scan.
>
True. But Sometimes ieee80211_scan_cancel is waiting for mutex where
the lock was already aquired by scan work and scan_work never releases
the lock till scan completion whenever next_delay == 0.
This scenario was observed while unplug the card during scan.

--
Rajkumar

Re: [PATCH] mac80211: cancel scan_work in ieee80211_unregister_hw

[Stanislaw Gruszka]

On Fri, May 13, 2011 at 11:05:34AM +0530, Rajkumar Manoharan wrote:
> On Thu, May 12, 2011 at 10:55:21PM +0530, Johannes Berg wrote:
> > On Thu, 2011-05-12 at 22:39 +0530, Rajkumar Manoharan wrote:
> > > scan_work is never stopped if the device was unplugged during
> > > s/w scan.
> > 
> > How did you come to that conclusion? ieee80211_do_stop() does stop the
> > scan.
> >
> True. But Sometimes ieee80211_scan_cancel is waiting for mutex where
> the lock was already aquired by scan work and scan_work never releases
> the lock till scan completion whenever next_delay == 0.
> This scenario was observed while unplug the card during scan.

So that looks like a bug somwhere in ieee80211_scan_state_* functions,
which should be fixed there, no?

Stanislaw

Re: [PATCH] mac80211: cancel scan_work in ieee80211_unregister_hw

[Johannes Berg]

On Fri, 2011-05-13 at 11:05 +0530, Rajkumar Manoharan wrote:

> > How did you come to that conclusion? ieee80211_do_stop() does stop the
> > scan.
> >
> True. But Sometimes ieee80211_scan_cancel is waiting for mutex where
> the lock was already aquired by scan work and scan_work never releases
> the lock till scan completion whenever next_delay == 0.

But that shouldn't take a long time. And if it's "stuck" there, then
cancel_delayed_work_sync() will also wait for it.

> This scenario was observed while unplug the card during scan.

If this happens then mac80211 will still make calls to the driver, I
don't think we can avoid this and drivers will need to handle it. Maybe
the driver is repeatedly timing out somewhere?

johannes

Re: [PATCH] mac80211: cancel scan_work in ieee80211_unregister_hw

[Rajkumar Manoharan]

On Fri, May 13, 2011 at 09:13:36PM +0530, Johannes Berg wrote:
> On Fri, 2011-05-13 at 11:05 +0530, Rajkumar Manoharan wrote:
> 
> > > How did you come to that conclusion? ieee80211_do_stop() does stop the
> > > scan.
> > >
> > True. But Sometimes ieee80211_scan_cancel is waiting for mutex where
> > the lock was already aquired by scan work and scan_work never releases
> > the lock till scan completion whenever next_delay == 0.
> 
> But that shouldn't take a long time. And if it's "stuck" there, then
> cancel_delayed_work_sync() will also wait for it.
>
Agree. cancel_delayed_work_sync does not help to abort scan when
scan_work is in next_scan_state loop.

> > This scenario was observed while unplug the card during scan.
> 
> If this happens then mac80211 will still make calls to the driver, I
> don't think we can avoid this and drivers will need to handle it. Maybe
> the driver is repeatedly timing out somewhere?
>
These unneccesary driver calls has to be avoided. How about the following
change to abort scan during scan loop.

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 4054399..9d18c43 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -384,11 +384,11 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
        int i;
        enum nl80211_channel_type orig_ct;
 
+       clear_bit(SDATA_STATE_RUNNING, &sdata->state);
+
        if (local->scan_sdata == sdata)
                ieee80211_scan_cancel(local);
 
-       clear_bit(SDATA_STATE_RUNNING, &sdata->state);
-
        /*
         * Stop TX on this interface first.
         */
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index ea44a8e..6353f93 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -719,6 +719,11 @@ void ieee80211_scan_work(struct work_struct *work)
         * without scheduling a new work
         */
        do {
+               if (!test_bit(SDATA_STATE_RUNNING, &sdata->state)) {
+                       aborted = true;
+                       goto out_complete;
+               }
+
                switch (local->next_scan_state) {
                case SCAN_DECISION:
                        /* if no more bands/channels left, complete scan */

--
Rajkumar

Re: [PATCH] mac80211: cancel scan_work in ieee80211_unregister_hw

[Johannes Berg]

On Fri, 2011-05-13 at 22:48 +0530, Rajkumar Manoharan wrote:

> These unneccesary driver calls has to be avoided. How about the following
> change to abort scan during scan loop.
> 
> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> index 4054399..9d18c43 100644
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -384,11 +384,11 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>         int i;
>         enum nl80211_channel_type orig_ct;
>  
> +       clear_bit(SDATA_STATE_RUNNING, &sdata->state);
> +
>         if (local->scan_sdata == sdata)
>                 ieee80211_scan_cancel(local);
>  
> -       clear_bit(SDATA_STATE_RUNNING, &sdata->state);
> -
>         /*
>          * Stop TX on this interface first.
>          */
> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
> index ea44a8e..6353f93 100644
> --- a/net/mac80211/scan.c
> +++ b/net/mac80211/scan.c
> @@ -719,6 +719,11 @@ void ieee80211_scan_work(struct work_struct *work)
>          * without scheduling a new work
>          */
>         do {
> +               if (!test_bit(SDATA_STATE_RUNNING, &sdata->state)) {
> +                       aborted = true;
> +                       goto out_complete;
> +               }
> +

Seems reasonable, but you should use ieee80211_sdata_running().

johannes