From: John W. Linville <[email protected]>
Default to MAX_UCODE_BEACON_INTERVAL if the output of
iwl_adjust_beacon_interval would otherwise be zero. This prevents a
division by zero on my iwl5300-equipped Lenovo T400 with kernels that
include "mac80211: use cfg80211s BSS infrastructure".
This patch is a bit of a hack -- I'm not sure why iwl_setup_rxon_timing
is giving iwl_adjust_beacon_interval a zero input (which is the only way
it would output zero). I would be happy to have a better fix. But for
now, this makes my box boot...
Signed-off-by: John W. Linville <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index 50f8c7f..ad31005 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -644,6 +644,9 @@ static u16 iwl_adjust_beacon_interval(u16 beacon_val)
/ MAX_UCODE_BEACON_INTERVAL;
new_val = beacon_val / beacon_factor;
+ if (!new_val)
+ new_val = MAX_UCODE_BEACON_INTERVAL;
+
return new_val;
}
--
1.6.0.6
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Mon, Feb 23, 2009 at 06:38:13PM -0800, Johannes Berg wrote:
> Ok, starting to look into this ...
>
> Since it seems you get this all the time, could you please put a printk
> into net/wireless/scan.c in cfg80211_inform_bss_frame where it sets
> res->pub.beacon_interval, and print out
> "%p %d", &res->pub, res->pub.beacon_interval
>
> and another one in cfg80211_get_bss where you print out the return value
> (just to make sure it's returning the right thing). Maybe also put one
> into cfg80211_bss_update where it updates things.
>
> I suspect that some memory is getting overwritten or something. The
> embedded struct thing was a bit of a hack.
>
> Also please put a printk into the iwlwifi code and into
> net/mac80211/mlme.c where it assigns
> sdata->vif.bss_conf.beacon_int = bss->cbss.beacon_interval;
>
> so that I can see the order this happening.
I have applied the patch attached as "debug.patch". I booted F10,
logged-in, and allowed NetworkManager to establish a connection.
The resulting dmesg output is attached as "dmesg.txt".
Hth! Let me know if you want more...
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Fri, 2009-02-20 at 16:31 -0800, John W. Linville wrote:
> From: John W. Linville <[email protected]>
>
> Default to MAX_UCODE_BEACON_INTERVAL if the output of
> iwl_adjust_beacon_interval would otherwise be zero. This prevents a
> division by zero on my iwl5300-equipped Lenovo T400 with kernels that
> include "mac80211: use cfg80211s BSS infrastructure".
>
> This patch is a bit of a hack -- I'm not sure why iwl_setup_rxon_timing
> is giving iwl_adjust_beacon_interval a zero input (which is the only way
> it would output zero). I would be happy to have a better fix. But for
> now, this makes my box boot...
>
> Signed-off-by: John W. Linville <[email protected]>
Unfortunately I do not have a better fix, but I could shed some light
onto what may be going on and hope we can together figure out what is
the problem.
First, it appears that your problem occurs during association. The
driver obtains the value of beacon interval from mac80211 at the time it
is asked to associate (specifically, mac80211 calls bss_info_changed
with BSS_CHANGED_ASSOC and ieee80211_bss_conf->assoc is true). The
driver obtains beacon interval from iee80211_bss_conf structure at this
time. As you indicate - the driver gets a zero and this causes an oops
later.
Now, the patch you pointed out ("mac80211: use cfg80211s BSS
infrastructure") changes the way in which the beacon interval value is
obtained and stored ... but yet I cannot see how it can be different
from before and cause zero to be returned. Here I hope that Johannes can
shed some light.
Before this patch we used mac80211's BSS infrastructure and placed the
beacon interval from the beacon frame in that. Now we use cfg80211's BSS
infrastructure and place the beacon interval from the probe response in
that. Now, the function that does this (ieee80211_bss_info_update) does
not currently distinguish between beacon and probe resp when it updates
the BSS information and uses the probe resp fields (in
cfg80211_inform_bss_frame). This should not matter because the beacon
and probe resp fields are the same in this regard.
So ... if the information is obtained in the same way there could maybe
be an issue in how it is stored now? This has also changed significantly
with the move to the rbtree. I don't know.
Johannes, do you perhaps know how beacon interval was ok with the
mac80211 BSS infrastructure, but now we get zero after moving to
cfg80211 BSS infrastructure?
Thank you
Reinette
Ok, starting to look into this ...
Since it seems you get this all the time, could you please put a printk
into net/wireless/scan.c in cfg80211_inform_bss_frame where it sets
res->pub.beacon_interval, and print out
"%p %d", &res->pub, res->pub.beacon_interval
and another one in cfg80211_get_bss where you print out the return value
(just to make sure it's returning the right thing). Maybe also put one
into cfg80211_bss_update where it updates things.
I suspect that some memory is getting overwritten or something. The
embedded struct thing was a bit of a hack.
Also please put a printk into the iwlwifi code and into
net/mac80211/mlme.c where it assigns
sdata->vif.bss_conf.beacon_int = bss->cbss.beacon_interval;
so that I can see the order this happening.
Thanks,
johannes
On Sat, Feb 21, 2009 at 09:31:33AM -0800, reinette chatre wrote:
> On Fri, 2009-02-20 at 16:31 -0800, John W. Linville wrote:
> > From: John W. Linville <[email protected]>
> >
> > Default to MAX_UCODE_BEACON_INTERVAL if the output of
> > iwl_adjust_beacon_interval would otherwise be zero. This prevents a
> > division by zero on my iwl5300-equipped Lenovo T400 with kernels that
> > include "mac80211: use cfg80211s BSS infrastructure".
> >
> > This patch is a bit of a hack -- I'm not sure why iwl_setup_rxon_timing
> > is giving iwl_adjust_beacon_interval a zero input (which is the only way
> > it would output zero). I would be happy to have a better fix. But for
> > now, this makes my box boot...
> >
> > Signed-off-by: John W. Linville <[email protected]>
>
> Unfortunately I do not have a better fix, but I could shed some light
> onto what may be going on and hope we can together figure out what is
> the problem.
>
> First, it appears that your problem occurs during association. The
> driver obtains the value of beacon interval from mac80211 at the time it
> is asked to associate (specifically, mac80211 calls bss_info_changed
> with BSS_CHANGED_ASSOC and ieee80211_bss_conf->assoc is true). The
> driver obtains beacon interval from iee80211_bss_conf structure at this
> time. As you indicate - the driver gets a zero and this causes an oops
> later.
>
> Now, the patch you pointed out ("mac80211: use cfg80211s BSS
> infrastructure") changes the way in which the beacon interval value is
> obtained and stored ... but yet I cannot see how it can be different
> from before and cause zero to be returned. Here I hope that Johannes can
> shed some light.
>
> Before this patch we used mac80211's BSS infrastructure and placed the
> beacon interval from the beacon frame in that. Now we use cfg80211's BSS
> infrastructure and place the beacon interval from the probe response in
> that. Now, the function that does this (ieee80211_bss_info_update) does
> not currently distinguish between beacon and probe resp when it updates
> the BSS information and uses the probe resp fields (in
> cfg80211_inform_bss_frame). This should not matter because the beacon
> and probe resp fields are the same in this regard.
>
> So ... if the information is obtained in the same way there could maybe
> be an issue in how it is stored now? This has also changed significantly
> with the move to the rbtree. I don't know.
I'm sorry if I was too terse, but at least it had the benefit
of you reproducing my struggle and my analysis (and my perplexed
frustration). :-)
> Johannes, do you perhaps know how beacon interval was ok with the
> mac80211 BSS infrastructure, but now we get zero after moving to
> cfg80211 BSS infrastructure?
Help us Obi Wan Johannes...you're our only hope! :-)
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Sat, 2009-03-14 at 19:08 +0200, Kalle Valo wrote:
> Johannes Berg wrote:
> > Kalle, I suspect that your beacon offload will require something like
> > holding on to BSS structs too, is that correct?
>
> My current solution is just to disable beacon filtering (and power save)
> whenever we are scanning. That way beacon filtering doesn't interfere
> scanning in anyway.
That makes sense, but disabling beacons means that
iwlist wlan0 scan last
will not show _any_ BSS while associated, but it should really show the
BSS we're associated to. This is the problem I was referring to.
johannes
Johannes Berg wrote:
> On Sat, 2009-03-14 at 19:08 +0200, Kalle Valo wrote:
>> Johannes Berg wrote:
>>> Kalle, I suspect that your beacon offload will require something like
>>> holding on to BSS structs too, is that correct?
>> My current solution is just to disable beacon filtering (and power save)
>> whenever we are scanning. That way beacon filtering doesn't interfere
>> scanning in anyway.
>
> That makes sense, but disabling beacons means that
>
> iwlist wlan0 scan last
>
> will not show _any_ BSS while associated, but it should really show the
> BSS we're associated to. This is the problem I was referring to.
Oh man, I wasn't even aware of this feature. So yes, my patches
definitely need a way to hold bss structs. I need to study the
implementation more before I can comment.
(Dropping the ipw3945 list from CC, it bugs me about not being subscribed.)
Kalle
John,
> > I suspect that some memory is getting overwritten or something. The
> > embedded struct thing was a bit of a hack.
> >
> > Also please put a printk into the iwlwifi code and into
> > net/mac80211/mlme.c where it assigns
> > sdata->vif.bss_conf.beacon_int = bss->cbss.beacon_interval;
> >
> > so that I can see the order this happening.
>
> I have applied the patch attached as "debug.patch". I booted F10,
> logged-in, and allowed NetworkManager to establish a connection.
> The resulting dmesg output is attached as "dmesg.txt".
>
> Hth! Let me know if you want more...
Sorry for the delay. Reinette made me aware that I'd missed to work on
this! Unfortunately, I don't see anything going wrong.
The problem is in this sequence:
> net/wireless/scan.c 244
> ffff88006ed59de0
here we find the BSS to use
> wlan0: authenticate with AP 00:18:84:80:c6:b1
and authenticate with it
> net/wireless/scan.c 244
> (null)
but now it has expired (I can only guess where this is called from)
> wlan0: authenticated
> wlan0: associate with AP 00:18:84:80:c6:b1
we associate
> net/wireless/scan.c 244
> (null)
> net/wireless/scan.c 244
> (null)
not sure
> wlan0: RX AssocResp from 00:18:84:80:c6:b1 (capab=0x421 status=0 aid=1)
> wlan0: associated
> net/wireless/scan.c 244
> (null)
this is this code:
bss_info_changed |= BSS_CHANGED_ASSOC;
ifmgd->flags |= IEEE80211_STA_ASSOCIATED;
bss = ieee80211_rx_bss_get(local, ifmgd->bssid,
conf->channel->center_freq,
ifmgd->ssid, ifmgd->ssid_len);
if (bss) {
/* set timing information */
in ieee80211_set_associated
> drivers/net/wireless/iwlwifi/iwl-agn.c 643
> 0
because "bss" is NULL, we don't actually set the timing information, and
thus iwlwifi gets a 0 value here. For some reason now your
wpa_supplicant decides to disassoc, and on the second try it's all fine:
> net/wireless/scan.c 244
> ffff88006ed5b4b0
> wlan0: authenticate with AP 00:18:39:5b:82:ca
> net/wireless/scan.c 244
> ffff88006ed5b4b0
> wlan0: authenticated
> wlan0: associate with AP 00:18:39:5b:82:ca
> net/wireless/scan.c 244
> ffff88006ed5b4b0
> net/wireless/scan.c 244
> ffff88006ed5b4b0
> wlan0: RX AssocResp from 00:18:39:5b:82:ca (capab=0x411 status=0 aid=6)
> wlan0: associated
> net/wireless/scan.c 244
> ffff88006ed5b4b0
> net/mac80211/mlme.c 641
> 100
> drivers/net/wireless/iwlwifi/iwl-agn.c 643
> 100
note the extra "mlme.c 641 / 100" lines, these are in the "if (bss)"
part.
Now, thinking a little about why this happens...
Before using cfg80211's BSS structs, we *never* expired any BSS structs.
We just hid them from userspace. This was a bug, we would forever
accumulate BSSes and use loads of memory. However, what happened above
could never happen: that a BSS struct went away while we were trying to
use it!! Oddly, this isn't supposed to happen, since we only expire
structs after 10 seconds and we continually receive beacons this
shouldn't happen.
To make completely sure, can you, in addition to this debug patch, add a
printk into net/wireless/scan.c:cfg80211_bss_expire and print out the
pointer for any expired BSS? I suspect we'd have seen ffff88006ed59de0
there.
In any case, the proper solution here would be to internally keep a
reference in mac80211 to the "current BSS" struct, and hang on to it
(ie. increase the refcount, and never use the lookup function but the
pointer we have gotten)...
Another thing we should do is to not overwrite in cfg80211_bss_update by
replacing the node, but by reallocating the node with the new
information. This needs some per-node locking though, I suspect.
Kalle, I suspect that your beacon offload will require something like
holding on to BSS structs too, is that correct?
johannes
On Sat, 2009-03-14 at 16:59 +0100, Johannes Berg wrote:
> Sorry for the delay. Reinette made me aware that I'd missed to work on
> this! Unfortunately, I don't see anything going wrong.
Scratch that, I did see something going wrong ;)
> In any case, the proper solution here would be to internally keep a
> reference in mac80211 to the "current BSS" struct, and hang on to it
> (ie. increase the refcount, and never use the lookup function but the
> pointer we have gotten)...
Also, moving assoc/auth stuff to cfg80211 _properly_ will require doing
that in cfg80211, and as such would fix this problem too. I'm inclined
to wait for that.
johannes
Johannes Berg wrote:
> Kalle, I suspect that your beacon offload will require something like
> holding on to BSS structs too, is that correct?
My current solution is just to disable beacon filtering (and power save)
whenever we are scanning. That way beacon filtering doesn't interfere
scanning in anyway.
I'm about to send v2 of my beacon filtering patches next.
Kalle