[RFC] mac80211: remove ieee80211_notify_mac

[RFC] mac80211: remove ieee80211_notify_mac

[Johannes Berg]

Before ieee80211_notify_mac() was added, it was presented with the
use case of using it to tell mac80211 that the association may
have been lost because the firmware crashed/reset.

Since then, it has also been used by iwlwifi to (slightly) speed
up re-association after resume, a workaround around the fact that
mac80211 has no suspend/resume handling yet. It is also not used
by any other drivers, so clearly it cannot be necessary for "good
enough" suspend/resume.

Unfortunately, the callback suffers from a severe problem: It only
works for station mode. If suspend/resume happens while in IBSS or
any other mode (but station), then the callback is pointless.

Recently, it has created a number of locking issues, first because
it required rtnl locking rather than RCU due to calling sleeping
functions within the critical section, and now because it's called
by iwlwifi from the mac80211 workqueue that may not use the rtnl
because it is flushed under rtnl.
(cf. http://bugzilla.kernel.org/show_bug.cgi?id=12046)

I think, therefore, that we should take a step back, remove it
entirely for now and add the small feature it provided properly.
For suspend and resume we will need to introduce new hooks, and for
the case where the firmware was reset the driver will probably
simply just pretend it has done a suspend/resume cycle to get
mac80211 to reprogram the hardware completely, not just try to
connect to the current AP again in station mode. When doing so, we
will need to take into account locking issues and possibly defer
to schedule_work from within mac80211 for the resume operation,
while the suspend operation must be done directly.

Proper suspend/resume should also not necessarily try to reconnect
to the current AP, the time spent in suspend may have been short
enough to not be disconnected from the AP, mac80211 will detect
that the AP went out of range quickly if it did, and if the
association is lost then the AP will disassoc as soon as a data
frame is sent. We might also take into account WWOL then, and
have mac80211 program the hardware into such a mode where it is
available and requested.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/net/wireless/iwlwifi/iwl-agn.c      |    2 --
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    2 --
 include/net/mac80211.h                      |   20 --------------------
 net/mac80211/mlme.c                         |   22 ----------------------
 4 files changed, 46 deletions(-)

--- everything.orig/drivers/net/wireless/iwlwifi/iwl-agn.c	2008-11-17 03:16:07.000000000 +0100
+++ everything/drivers/net/wireless/iwlwifi/iwl-agn.c	2008-11-17 03:16:55.000000000 +0100
@@ -2192,7 +2192,6 @@ static void iwl_bg_alive_start(struct wo
 	mutex_lock(&priv->mutex);
 	iwl_alive_start(priv);
 	mutex_unlock(&priv->mutex);
-	ieee80211_notify_mac(priv->hw, IEEE80211_NOTIFY_RE_ASSOC);
 }
 
 static void iwl_bg_rf_kill(struct work_struct *work)
@@ -2248,7 +2247,6 @@ static void iwl_bg_set_monitor(struct wo
 	}
 
 	mutex_unlock(&priv->mutex);
-	ieee80211_notify_mac(priv->hw, IEEE80211_NOTIFY_RE_ASSOC);
 }
 
 static void iwl_bg_run_time_calib_work(struct work_struct *work)
--- everything.orig/drivers/net/wireless/iwlwifi/iwl3945-base.c	2008-11-17 03:16:07.000000000 +0100
+++ everything/drivers/net/wireless/iwlwifi/iwl3945-base.c	2008-11-17 03:16:18.000000000 +0100
@@ -5972,7 +5972,6 @@ static void iwl3945_bg_alive_start(struc
 	mutex_lock(&priv->mutex);
 	iwl3945_alive_start(priv);
 	mutex_unlock(&priv->mutex);
-	ieee80211_notify_mac(priv->hw, IEEE80211_NOTIFY_RE_ASSOC);
 }
 
 static void iwl3945_bg_rf_kill(struct work_struct *work)
@@ -6023,7 +6022,6 @@ static void iwl3945_bg_set_monitor(struc
 			IWL_ERROR("iwl3945_set_mode() failed\n");
 
 	mutex_unlock(&priv->mutex);
-	ieee80211_notify_mac(priv->hw, IEEE80211_NOTIFY_RE_ASSOC);
 }
 
 #define IWL_SCAN_CHECK_WATCHDOG (7 * HZ)
--- everything.orig/include/net/mac80211.h	2008-11-17 03:15:20.000000000 +0100
+++ everything/include/net/mac80211.h	2008-11-17 03:15:44.000000000 +0100
@@ -74,14 +74,6 @@
  */
 
 /**
- * enum ieee80211_notification_type - Low level driver notification
- * @IEEE80211_NOTIFY_RE_ASSOC: start the re-association sequence
- */
-enum ieee80211_notification_types {
-	IEEE80211_NOTIFY_RE_ASSOC,
-};
-
-/**
  * enum ieee80211_max_queues - maximum number of queues
  *
  * @IEEE80211_MAX_QUEUES: Maximum number of regular device queues.
@@ -1853,18 +1845,6 @@ void ieee80211_stop_tx_ba_cb_irqsafe(str
 				     u16 tid);
 
 /**
- * ieee80211_notify_mac - low level driver notification
- * @hw: pointer as obtained from ieee80211_alloc_hw().
- * @notif_type: enum ieee80211_notification_types
- *
- * This function must be called by low level driver to inform mac80211 of
- * low level driver status change or force mac80211 to re-assoc for low
- * level driver internal error that require re-assoc.
- */
-void ieee80211_notify_mac(struct ieee80211_hw *hw,
-			  enum ieee80211_notification_types  notif_type);
-
-/**
  * ieee80211_find_sta - find a station
  *
  * @hw: pointer as obtained from ieee80211_alloc_hw()
--- everything.orig/net/mac80211/mlme.c	2008-11-17 03:15:56.000000000 +0100
+++ everything/net/mac80211/mlme.c	2008-11-17 03:16:00.000000000 +0100
@@ -2586,25 +2586,3 @@ void ieee80211_mlme_notify_scan_complete
 		ieee80211_restart_sta_timer(sdata);
 	rcu_read_unlock();
 }
-
-/* driver notification call */
-void ieee80211_notify_mac(struct ieee80211_hw *hw,
-			  enum ieee80211_notification_types  notif_type)
-{
-	struct ieee80211_local *local = hw_to_local(hw);
-	struct ieee80211_sub_if_data *sdata;
-
-	switch (notif_type) {
-	case IEEE80211_NOTIFY_RE_ASSOC:
-		rtnl_lock();
-		list_for_each_entry(sdata, &local->interfaces, list) {
-			if (sdata->vif.type != NL80211_IFTYPE_STATION)
-				continue;
-
-			ieee80211_sta_req_auth(sdata, &sdata->u.sta);
-		}
-		rtnl_unlock();
-		break;
-	}
-}
-EXPORT_SYMBOL(ieee80211_notify_mac);

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Tomas Winkler]

On Mon, Nov 17, 2008 at 11:59 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Before ieee80211_notify_mac() was added, it was presented with the
> use case of using it to tell mac80211 that the association may
> have been lost because the firmware crashed/reset.
>
> Since then, it has also been used by iwlwifi to (slightly) speed
> up re-association after resume, a workaround around the fact that
> mac80211 has no suspend/resume handling yet. It is also not used
> by any other drivers, so clearly it cannot be necessary for "good
> enough" suspend/resume.
>
> Unfortunately, the callback suffers from a severe problem: It only
> works for station mode. If suspend/resume happens while in IBSS or
> any other mode (but station), then the callback is pointless.
>
> Recently, it has created a number of locking issues, first because
> it required rtnl locking rather than RCU due to calling sleeping
> functions within the critical section, and now because it's called
> by iwlwifi from the mac80211 workqueue that may not use the rtnl
> because it is flushed under rtnl.
> (cf. http://bugzilla.kernel.org/show_bug.cgi?id=12046)
>
> I think, therefore, that we should take a step back, remove it
> entirely for now and add the small feature it provided properly.
> For suspend and resume we will need to introduce new hooks, and for
> the case where the firmware was reset the driver will probably
> simply just pretend it has done a suspend/resume cycle to get
> mac80211 to reprogram the hardware completely, not just try to
> connect to the current AP again in station mode. When doing so, we
> will need to take into account locking issues and possibly defer
> to schedule_work from within mac80211 for the resume operation,
> while the suspend operation must be done directly.
>
> Proper suspend/resume should also not necessarily try to reconnect
> to the current AP, the time spent in suspend may have been short
> enough to not be disconnected from the AP, mac80211 will detect
> that the AP went out of range quickly if it did, and if the
> association is lost then the AP will disassoc as soon as a data
> frame is sent. We might also take into account WWOL then, and
> have mac80211 program the hardware into such a mode where it is
> available and requested.

IMHO Not the concept but the implementation of this function is wrong.
It should be no implementation difference between mac notification and
reception of one RX frames that triggers oneof the connection step
such as association response. If this would be handled in this context
and there won't be any locking issue.
If driver crashes internally and lost association info it can generate
"local disassociation frame" and mac will try to reapply association
in a regular flow.

I agree that resume/suspend shell be handled properly in the mac80211
regardless of this issue.


> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
>  drivers/net/wireless/iwlwifi/iwl-agn.c      |    2 --
>  drivers/net/wireless/iwlwifi/iwl3945-base.c |    2 --
>  include/net/mac80211.h                      |   20 --------------------
>  net/mac80211/mlme.c                         |   22 ----------------------
>  4 files changed, 46 deletions(-)
>
> --- everything.orig/drivers/net/wireless/iwlwifi/iwl-agn.c      2008-11-17 03:16:07.000000000 +0100
> +++ everything/drivers/net/wireless/iwlwifi/iwl-agn.c   2008-11-17 03:16:55.000000000 +0100
> @@ -2192,7 +2192,6 @@ static void iwl_bg_alive_start(struct wo
>        mutex_lock(&priv->mutex);
>        iwl_alive_start(priv);
>        mutex_unlock(&priv->mutex);
> -       ieee80211_notify_mac(priv->hw, IEEE80211_NOTIFY_RE_ASSOC);
>  }
>
>  static void iwl_bg_rf_kill(struct work_struct *work)
> @@ -2248,7 +2247,6 @@ static void iwl_bg_set_monitor(struct wo
>        }
>
>        mutex_unlock(&priv->mutex);
> -       ieee80211_notify_mac(priv->hw, IEEE80211_NOTIFY_RE_ASSOC);
>  }
>
>  static void iwl_bg_run_time_calib_work(struct work_struct *work)
> --- everything.orig/drivers/net/wireless/iwlwifi/iwl3945-base.c 2008-11-17 03:16:07.000000000 +0100
> +++ everything/drivers/net/wireless/iwlwifi/iwl3945-base.c      2008-11-17 03:16:18.000000000 +0100
> @@ -5972,7 +5972,6 @@ static void iwl3945_bg_alive_start(struc
>        mutex_lock(&priv->mutex);
>        iwl3945_alive_start(priv);
>        mutex_unlock(&priv->mutex);
> -       ieee80211_notify_mac(priv->hw, IEEE80211_NOTIFY_RE_ASSOC);
>  }
>
>  static void iwl3945_bg_rf_kill(struct work_struct *work)
> @@ -6023,7 +6022,6 @@ static void iwl3945_bg_set_monitor(struc
>                        IWL_ERROR("iwl3945_set_mode() failed\n");
>
>        mutex_unlock(&priv->mutex);
> -       ieee80211_notify_mac(priv->hw, IEEE80211_NOTIFY_RE_ASSOC);
>  }
>
>  #define IWL_SCAN_CHECK_WATCHDOG (7 * HZ)
> --- everything.orig/include/net/mac80211.h      2008-11-17 03:15:20.000000000 +0100
> +++ everything/include/net/mac80211.h   2008-11-17 03:15:44.000000000 +0100
> @@ -74,14 +74,6 @@
>  */
>
>  /**
> - * enum ieee80211_notification_type - Low level driver notification
> - * @IEEE80211_NOTIFY_RE_ASSOC: start the re-association sequence
> - */
> -enum ieee80211_notification_types {
> -       IEEE80211_NOTIFY_RE_ASSOC,
> -};
> -
> -/**
>  * enum ieee80211_max_queues - maximum number of queues
>  *
>  * @IEEE80211_MAX_QUEUES: Maximum number of regular device queues.
> @@ -1853,18 +1845,6 @@ void ieee80211_stop_tx_ba_cb_irqsafe(str
>                                     u16 tid);
>
>  /**
> - * ieee80211_notify_mac - low level driver notification
> - * @hw: pointer as obtained from ieee80211_alloc_hw().
> - * @notif_type: enum ieee80211_notification_types
> - *
> - * This function must be called by low level driver to inform mac80211 of
> - * low level driver status change or force mac80211 to re-assoc for low
> - * level driver internal error that require re-assoc.
> - */
> -void ieee80211_notify_mac(struct ieee80211_hw *hw,
> -                         enum ieee80211_notification_types  notif_type);
> -
> -/**
>  * ieee80211_find_sta - find a station
>  *
>  * @hw: pointer as obtained from ieee80211_alloc_hw()
> --- everything.orig/net/mac80211/mlme.c 2008-11-17 03:15:56.000000000 +0100
> +++ everything/net/mac80211/mlme.c      2008-11-17 03:16:00.000000000 +0100
> @@ -2586,25 +2586,3 @@ void ieee80211_mlme_notify_scan_complete
>                ieee80211_restart_sta_timer(sdata);
>        rcu_read_unlock();
>  }
> -
> -/* driver notification call */
> -void ieee80211_notify_mac(struct ieee80211_hw *hw,
> -                         enum ieee80211_notification_types  notif_type)
> -{
> -       struct ieee80211_local *local = hw_to_local(hw);
> -       struct ieee80211_sub_if_data *sdata;
> -
> -       switch (notif_type) {
> -       case IEEE80211_NOTIFY_RE_ASSOC:
> -               rtnl_lock();
> -               list_for_each_entry(sdata, &local->interfaces, list) {
> -                       if (sdata->vif.type != NL80211_IFTYPE_STATION)
> -                               continue;
> -
> -                       ieee80211_sta_req_auth(sdata, &sdata->u.sta);
> -               }
> -               rtnl_unlock();
> -               break;
> -       }
> -}
> -EXPORT_SYMBOL(ieee80211_notify_mac);
>
>
>

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

On Mon, 2008-11-17 at 16:32 +0200, Tomas Winkler wrote:

> IMHO Not the concept but the implementation of this function is wrong.

I disagree.

> It should be no implementation difference between mac notification and
> reception of one RX frames that triggers oneof the connection step
> such as association response. If this would be handled in this context
> and there won't be any locking issue.
> If driver crashes internally and lost association info it can generate
> "local disassociation frame" and mac will try to reapply association
> in a regular flow.

The point is that the whole thing about disassociation is already the
wrong assumption. As I've outlined, it only works in STA mode, and thus
the function is generally not very useful.

> I agree that resume/suspend shell be handled properly in the mac80211
> regardless of this issue.

And it will handle the "firmware crashed" case perfectly too.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Tomas Winkler]

On Mon, Nov 17, 2008 at 4:45 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-11-17 at 16:32 +0200, Tomas Winkler wrote:
>
>> IMHO Not the concept but the implementation of this function is wrong.
>
> I disagree.
>
>> It should be no implementation difference between mac notification and
>> reception of one RX frames that triggers oneof the connection step
>> such as association response. If this would be handled in this context
>> and there won't be any locking issue.
>> If driver crashes internally and lost association info it can generate
>> "local disassociation frame" and mac will try to reapply association
>> in a regular flow.
>
> The point is that the whole thing about disassociation is already the
> wrong assumption. As I've outlined, it only works in STA mode, and thus
> the function is generally not very useful.
>
>> I agree that resume/suspend shell be handled properly in the mac80211
>> regardless of this issue.
>
> And it will handle the "firmware crashed" case perfectly too.

You may have a case, anyhow, please show us some RFC before you remove
of mac notify.
Tomas

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On Mon, 2008-11-17 at 17:14 +0200, Tomas Winkler wrote:

> > The point is that the whole thing about disassociation is already the
> > wrong assumption. As I've outlined, it only works in STA mode, and thus
> > the function is generally not very useful.
> >
> >> I agree that resume/suspend shell be handled properly in the mac80211
> >> regardless of this issue.
> >
> > And it will handle the "firmware crashed" case perfectly too.
> 
> You may have a case, anyhow, please show us some RFC before you remove
> of mac notify.

I don't have any, but given the current deficiencies, I don't think the
notify callback is worth keeping especially in light of the locking
nightmare it's creating. What does it fix anyway? No user ever
complained about slow re-association at resume time that I've heard, and
no driver but iwlwifi tries to speed it up.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

On Mon, 2008-11-17 at 19:34 +0200, Tomas Winkler wrote:

> There actually complains about slow reconnection, 

Ok I guess then I haven't seen them for some reason.

Either way, here's a quick summary:
 * locking issues with the callback are fixed by removing it
 * callback is incorrect when you're only suspended for a very short
   time
 * callback is incorrect when you're in non-STA modes
 * suspend/resume cannot be implemented well through this callback, at
   least not the way it is written now and needs to do a whole lot more
 * there's no "slow" issue when you actually resume in a different
   location where the AP is not around any more
 * there should be no "slow" issue when the AP properly deauthenticates
   when receiving data frames

This was an RFC. I'm convinced it should go in, but I don't make those
decisions anyway. I've outlined my reasons for it.

> Second we used the
> same mechanism to
> recover from rfkill which wasn't submitted. rfkill needs also mac80211
> treatment.

Sure does, and I've even described how I'd do it in some email. Seems
nobody actually cares enough though to invest the day or two it would
take to write it. And I don't care about killswitches at all,
fortunately, so I don't need to touch that mess.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Tomas Winkler]

On Mon, Nov 17, 2008 at 7:59 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-11-17 at 19:34 +0200, Tomas Winkler wrote:
>
>> There actually complains about slow reconnection,
>
> Ok I guess then I haven't seen them for some reason.
>
> Either way, here's a quick summary:
>  * locking issues with the callback are fixed by removing it
>  * callback is incorrect when you're only suspended for a very short
>   time
>  * callback is incorrect when you're in non-STA modes
>  * suspend/resume cannot be implemented well through this callback, at
>   least not the way it is written now and needs to do a whole lot more
>  * there's no "slow" issue when you actually resume in a different
>   location where the AP is not around any more
>  * there should be no "slow" issue when the AP properly deauthenticates
>   when receiving data frames
>
> This was an RFC. I'm convinced it should go in, but I don't make those
> decisions anyway. I've outlined my reasons for it.

My concern is what is immediate impact of this. Whether we fix mac
notify for short term
and we'll remove it when resume/suspend is available or we have
suspend/resume worked out in 2h.

>
>> Second we used the
>> same mechanism to
>> recover from rfkill which wasn't submitted. rfkill needs also mac80211
>> treatment.
>
> Sure does, and I've even described how I'd do it in some email. Seems
> nobody actually cares enough though to invest the day or two it would
> take to write it. And I don't care about killswitches at all,
> fortunately, so I don't need to touch that mess.

I've posted something based on plugging into notification chain. It
worked but wasn't accepted I hoped that who rejected will supply
alternative solution...

Tomas


> johannes
>

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

On Mon, 2008-11-17 at 20:12 +0200, Tomas Winkler wrote:

> I've posted something based on plugging into notification chain. It
> worked but wasn't accepted I hoped that who rejected will supply
> alternative solution...

Sorry, that's not how it works... Those who want the functionality are
generally expected to do the work, and if the design phase happens
behind closed doors they don't get to complain...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Tomas Winkler]

On Mon, Nov 17, 2008 at 9:46 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-11-17 at 20:12 +0200, Tomas Winkler wrote:
>
>> I've posted something based on plugging into notification chain. It
>> worked but wasn't accepted I hoped that who rejected will supply
>> alternative solution...
>
> Sorry, that's not how it works... Those who want the functionality are
> generally expected to do the work,

I didn't get the feeling I'm the only one who wants it there were
alternative design which shell be backed up by some code.

 and if the design phase happens
> behind closed doors they don't get to complain...

RFC patch is a valid design presentation. This is not valid complain of yours.
Tomas

Re: [RFC] mac80211: remove ieee80211_notify_mac

[John W. Linville]

On Mon, Nov 17, 2008 at 06:59:59PM +0100, Johannes Berg wrote:
> On Mon, 2008-11-17 at 19:34 +0200, Tomas Winkler wrote:
> 
> > There actually complains about slow reconnection, 
> 
> Ok I guess then I haven't seen them for some reason.
> 
> Either way, here's a quick summary:
>  * locking issues with the callback are fixed by removing it
>  * callback is incorrect when you're only suspended for a very short
>    time
>  * callback is incorrect when you're in non-STA modes
>  * suspend/resume cannot be implemented well through this callback, at
>    least not the way it is written now and needs to do a whole lot more
>  * there's no "slow" issue when you actually resume in a different
>    location where the AP is not around any more
>  * there should be no "slow" issue when the AP properly deauthenticates
>    when receiving data frames
> 
> This was an RFC. I'm convinced it should go in, but I don't make those
> decisions anyway. I've outlined my reasons for it.

I agree that it seems to solve problems, and there is little benefit
tokeeping the callback in question.  I'm going to send this upstream.

John
-- 
John W. Linville		Linux should be at the core
linville@tuxdriver.com			of your literate lifestyle.

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Tomas Winkler]

On Tue, Nov 18, 2008 at 9:46 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Mon, Nov 17, 2008 at 06:59:59PM +0100, Johannes Berg wrote:
>> On Mon, 2008-11-17 at 19:34 +0200, Tomas Winkler wrote:
>>
>> > There actually complains about slow reconnection,
>>
>> Ok I guess then I haven't seen them for some reason.
>>
>> Either way, here's a quick summary:
>>  * locking issues with the callback are fixed by removing it
>>  * callback is incorrect when you're only suspended for a very short
>>    time
>>  * callback is incorrect when you're in non-STA modes
>>  * suspend/resume cannot be implemented well through this callback, at
>>    least not the way it is written now and needs to do a whole lot more
>>  * there's no "slow" issue when you actually resume in a different
>>    location where the AP is not around any more
>>  * there should be no "slow" issue when the AP properly deauthenticates
>>    when receiving data frames
>>
>> This was an RFC. I'm convinced it should go in, but I don't make those
>> decisions anyway. I've outlined my reasons for it.
>
> I agree that it seems to solve problems, and there is little benefit
> tokeeping the callback in question.  I'm going to send this upstream.
>
I suggest to run this code before going upstream
Tomas

Re: [RFC] mac80211: remove ieee80211_notify_mac

[mohamed salim abbas]

some bugs related to coming from resume is filed caused by removing
this callback function, any body looking into alternatives to share
how to fix this problem.

On Tue, Nov 18, 2008 at 3:08 PM, Tomas Winkler <tomasw@gmail.com> wrote:
> On Tue, Nov 18, 2008 at 9:46 PM, John W. Linville
> <linville@tuxdriver.com> wrote:
>> On Mon, Nov 17, 2008 at 06:59:59PM +0100, Johannes Berg wrote:
>>> On Mon, 2008-11-17 at 19:34 +0200, Tomas Winkler wrote:
>>>
>>> > There actually complains about slow reconnection,
>>>
>>> Ok I guess then I haven't seen them for some reason.
>>>
>>> Either way, here's a quick summary:
>>>  * locking issues with the callback are fixed by removing it
>>>  * callback is incorrect when you're only suspended for a very short
>>>    time
>>>  * callback is incorrect when you're in non-STA modes
>>>  * suspend/resume cannot be implemented well through this callback, at
>>>    least not the way it is written now and needs to do a whole lot more
>>>  * there's no "slow" issue when you actually resume in a different
>>>    location where the AP is not around any more
>>>  * there should be no "slow" issue when the AP properly deauthenticates
>>>    when receiving data frames
>>>
>>> This was an RFC. I'm convinced it should go in, but I don't make those
>>> decisions anyway. I've outlined my reasons for it.
>>
>> I agree that it seems to solve problems, and there is little benefit
>> tokeeping the callback in question.  I'm going to send this upstream.
>>
> I suggest to run this code before going upstream
> Tomas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC] mac80211: remove ieee80211_notify_mac

[John W. Linville]

On Fri, Dec 05, 2008 at 03:53:18PM -0800, mohamed salim abbas wrote:
> some bugs related to coming from resume is filed caused by removing
> this callback function, any body looking into alternatives to share
> how to fix this problem.

Perhaps you could give us a link to the reported bugs?

-- 
John W. Linville		Linux should be at the core
linville@tuxdriver.com			of your literate lifestyle.

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

On Fri, 2008-12-05 at 15:53 -0800, mohamed salim abbas wrote:
> some bugs related to coming from resume is filed caused by removing
> this callback function, any body looking into alternatives to share
> how to fix this problem.

Yes, in fact, somebody _is_ looking into implementing _proper_
suspend/resume support. You'll either have to wait or help out.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] mac80211: remove ieee80211_notify_mac

[mohamed salim abbas]

I will be happy to help if extra help need it. below is the link to the bug.
http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=1845

On Sat, Dec 6, 2008 at 1:05 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2008-12-05 at 15:53 -0800, mohamed salim abbas wrote:
>> some bugs related to coming from resume is filed caused by removing
>> this callback function, any body looking into alternatives to share
>> how to fix this problem.
>
> Yes, in fact, somebody _is_ looking into implementing _proper_
> suspend/resume support. You'll either have to wait or help out.
>
> johannes
>

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Bob Copeland]

On Sun, Dec 7, 2008 at 12:16 AM, mohamed salim abbas
<mabbaswireless@gmail.com> wrote:
> I will be happy to help if extra help need it. below is the link to the bug.
> http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=1845

Thanks Mohamed, I'd appreciate any help.  Give me a couple of days to
square away my current changes, then I'll post the full series.  My
last changeset was at:

    http://bobcopeland.com/kernel/wl/

-- 
Bob Copeland %% www.bobcopeland.com

Re: [RFC] mac80211: remove ieee80211_notify_mac

[mohamed salim abbas]

I will review these patches and try them out.

On Sun, Dec 7, 2008 at 6:19 AM, Bob Copeland <me@bobcopeland.com> wrote:
> On Sun, Dec 7, 2008 at 12:16 AM, mohamed salim abbas
> <mabbaswireless@gmail.com> wrote:
>> I will be happy to help if extra help need it. below is the link to the bug.
>> http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=1845
>
> Thanks Mohamed, I'd appreciate any help.  Give me a couple of days to
> square away my current changes, then I'll post the full series.  My
> last changeset was at:
>
>    http://bobcopeland.com/kernel/wl/
>
> --
> Bob Copeland %% www.bobcopeland.com
>

Re: [RFC] mac80211: remove ieee80211_notify_mac

[mohamed salim abbas]

I did try these patches out, I have a problem, sometime, when the
card resume from suspend that I am currently tracing but still this
patch does not bring me to the same state before the suspend. If I am
currently pinging and I suspend then resumed the ping program should
resume ping as it was before suspend. I know you still working on this
but I am not sure if this what will see in final solution. The
ieee80211_notify_mac callback function was used to make sure we return
to exact state right when driver resume or driver assert and recovered
or coming from rfkill.

On Sun, Dec 7, 2008 at 9:55 PM, mohamed salim abbas
<mabbaswireless@gmail.com> wrote:
> I will review these patches and try them out.
>
> On Sun, Dec 7, 2008 at 6:19 AM, Bob Copeland <me@bobcopeland.com> wrote:
>> On Sun, Dec 7, 2008 at 12:16 AM, mohamed salim abbas
>> <mabbaswireless@gmail.com> wrote:
>>> I will be happy to help if extra help need it. below is the link to the bug.
>>> http://www.intellinuxwireless.org/bugzilla/show_bug.cgi?id=1845
>>
>> Thanks Mohamed, I'd appreciate any help.  Give me a couple of days to
>> square away my current changes, then I'll post the full series.  My
>> last changeset was at:
>>
>>    http://bobcopeland.com/kernel/wl/
>>
>> --
>> Bob Copeland %% www.bobcopeland.com
>>
>

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Bob Copeland]

On Thu, Dec 11, 2008 at 6:00 PM, mohamed salim abbas
<mabbaswireless@gmail.com> wrote:
> I did try these patches out, I have a problem, sometime, when the
> card resume from suspend that I am currently tracing but still this
> patch does not bring me to the same state before the suspend.

Thanks for testing.  Yeah, it's still a work in progress.  I plan to
spend the weekend hacking on this and getting it into shape.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [RFC] mac80211: remove ieee80211_notify_mac

[mohamed salim abbas]

Great let me know once you want me try anything or any help needed.
Mohamed

On Fri, Dec 12, 2008 at 9:37 AM, Bob Copeland <me@bobcopeland.com> wrote:
> On Thu, Dec 11, 2008 at 6:00 PM, mohamed salim abbas
> <mabbaswireless@gmail.com> wrote:
>> I did try these patches out, I have a problem, sometime, when the
>> card resume from suspend that I am currently tracing but still this
>> patch does not bring me to the same state before the suspend.
>
> Thanks for testing.  Yeah, it's still a work in progress.  I plan to
> spend the weekend hacking on this and getting it into shape.
>
> --
> Bob Copeland %% www.bobcopeland.com
>

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Marcel Holtmann]

Hi Johannes,

>>> The point is that the whole thing about disassociation is already the
>>> wrong assumption. As I've outlined, it only works in STA mode, and thus
>>> the function is generally not very useful.
>>>
>>>> I agree that resume/suspend shell be handled properly in the mac80211
>>>> regardless of this issue.
>>> And it will handle the "firmware crashed" case perfectly too.
>> You may have a case, anyhow, please show us some RFC before you remove
>> of mac notify.
> 
> I don't have any, but given the current deficiencies, I don't think the
> notify callback is worth keeping especially in light of the locking
> nightmare it's creating. What does it fix anyway? No user ever
> complained about slow re-association at resume time that I've heard, and
> no driver but iwlwifi tries to speed it up.

until we see hard numbers here, I am fine with treating every driver the 
same and just not do any kind of speed up at all. With all my testing 
during resume and re-association, the timing has always been good enough 
so that no real impact for the user happened.

The question here is what are the real benefits and for that we need a 
more detailed measurement that is realistic. In the desktop case for 
example we have enough time since the users still has to still enter 
their password to unlock the screensaver. For the embedded type devices 
like phones etc., the impact is also not present. At least I've never 
seen it. There are other bottlenecks like updating the DHCP lease etc.

Regards

Marcel

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Bob Copeland]

On Mon, Nov 17, 2008 at 4:59 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> I think, therefore, that we should take a step back, remove it
> entirely for now and add the small feature it provided properly.
> For suspend and resume we will need to introduce new hooks, and for
> the case where the firmware was reset the driver will probably
> simply just pretend it has done a suspend/resume cycle to get
> mac80211 to reprogram the hardware completely, not just try to
> connect to the current AP again in station mode. When doing so, we
> will need to take into account locking issues and possibly defer
> to schedule_work from within mac80211 for the resume operation,
> while the suspend operation must be done directly.

I have proto-patches for suspend/resume that I still need to post -- I
haven't done so yet because, well, it's still a long way from working,
and I back-burnered it a bit.  I'll try to pull them together and post
them just for science.

In them, ieee80211_notify_mac is a minor implementation detail so any hook
back into the mac layer (ieee80211_suspend/ieee80211_resume e.g.) would be
fine with me.  So I think this is OK.

Deferring stuff in resume until later sounds like a good plan, there can
be lots of surprising stuff there (e.g. I didn't see this yet in any
drivers, but if someone happens to request_firmware in ->start() and
there is no userspace yet, uh oh...)

-- 
Bob Copeland %% www.bobcopeland.com

Re: [RFC] mac80211: remove ieee80211_notify_mac

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 356 bytes --]

On Mon, 2008-11-17 at 12:08 -0500, Bob Copeland wrote:

> Deferring stuff in resume until later sounds like a good plan, there can
> be lots of surprising stuff there (e.g. I didn't see this yet in any
> drivers, but if someone happens to request_firmware in ->start() and
> there is no userspace yet, uh oh...)

That happens for sure.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]