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 <[email protected]>
---
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);
On Mon, Nov 17, 2008 at 11:59 AM, Johannes Berg
<[email protected]> 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 <[email protected]>
> ---
> 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);
>
>
>
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
On Mon, Nov 17, 2008 at 4:45 PM, Johannes Berg
<[email protected]> 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
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
On Mon, Nov 17, 2008 at 4:59 AM, Johannes Berg
<[email protected]> 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 %% http://www.bobcopeland.com
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
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
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 <[email protected]> wrote:
> On Thu, Dec 11, 2008 at 6:00 PM, mohamed salim abbas
> <[email protected]> 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 %% http://www.bobcopeland.com
>
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 <[email protected]> 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
>
On Thu, Dec 11, 2008 at 6:00 PM, mohamed salim abbas
<[email protected]> 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 %% http://www.bobcopeland.com
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
<[email protected]> wrote:
> I will review these patches and try them out.
>
> On Sun, Dec 7, 2008 at 6:19 AM, Bob Copeland <[email protected]> wrote:
>> On Sun, Dec 7, 2008 at 12:16 AM, mohamed salim abbas
>> <[email protected]> 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 %% http://www.bobcopeland.com
>>
>
I will review these patches and try them out.
On Sun, Dec 7, 2008 at 6:19 AM, Bob Copeland <[email protected]> wrote:
> On Sun, Dec 7, 2008 at 12:16 AM, mohamed salim abbas
> <[email protected]> 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 %% http://www.bobcopeland.com
>
On Sun, Dec 7, 2008 at 12:16 AM, mohamed salim abbas
<[email protected]> 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 %% http://www.bobcopeland.com