* Re: 3.7.8/amd64 full interrupt hangs due to iwlwifi under big nfs copies out
From: Eric Dumazet @ 2013-06-18 16:52 UTC (permalink / raw)
To: Johannes Berg
Cc: Marc MERLIN, David Miller, Larry.Finger, bhutchings,
linux-wireless, netdev
In-Reply-To: <1361912099.8440.21.camel@jlt4.sipsolutions.net>
On Tue, 2013-02-26 at 21:54 +0100, Johannes Berg wrote:
> On Fri, 2013-02-22 at 22:14 -0800, Marc MERLIN wrote:
> > On Wed, Feb 20, 2013 at 10:15:03AM +0100, Johannes Berg wrote:
> > > On Wed, 2013-02-20 at 10:12 +0100, Johannes Berg wrote:
> > > > On Tue, 2013-02-19 at 08:21 -0800, Eric Dumazet wrote:
> > > > > On Tue, 2013-02-19 at 11:03 +0100, Johannes Berg wrote:
> > > > > > On Mon, 2013-02-18 at 21:17 -0800, Eric Dumazet wrote:
> > > > > >
> > > > > > > > chrome: page allocation failure: order:1, mode:0x4020
> > > > > > > > Pid: 8730, comm: chrome Tainted: G O 3.7.8-amd64-preempt-20121226-fixwd #1
> > > > > > > > Call Trace:
> > > > > > > > <IRQ> [<ffffffff810d5f38>] warn_alloc_failed+0x117/0x12c
> > > > > >
> > > > > > > You could try to load iwlwifi with amsdu_size_8K set to 0 (disable)
> > > > > > >
> > > > > > > It should hopefully use order-0 pages
> > > > > >
> > > > > > It will, do that then, unfortunately it can't switch at runtime because
> > > > > > it advertised this support to the access point or clients.
> > > > >
> > > > > What are the drawbacks of setting amsdu_size_8K to 0 by default ?
> > > >
> > > > We're discussing this now, the only downside would be that we couldn't
> > > > receive 8k A-MSDUs. Thing is, practically nobody uses A-MSDU anyway, and
> > > > even when I suspect the difference between 4k and 8k won't be huge.
> > >
> > > OTOH, this affects the protocol, and when you really can't allocate any
> > > order-1 pages you pointed out yourself that many other things also won't
> > > work, so I'm not really sure it makes a big difference if we change the
> > > driver?
> >
> > That as an unscientific test, but when I did the NFS eats all my pages
> > test using ethernet, my system didn't hang like it did with iwlagn.
> >
> > So while the NFS code is definitely doing something wrong when it uses
> > its default huge buffers, the e1000e code deals with it without hanging
> > my system.
> >
> > So thanks for trying to improve the iwlagn code to avoid those system
> > lockups.
>
> We'll be submitting a patch to make single pages default.
Do you think the same change would be possible for
drivers/net/wireless/iwlegacy/4965-mac.c ?
Thanks !
^ permalink raw reply
* ath9k_htc: station unable to authenticate
From: Ignacy Gawedzki @ 2013-06-18 16:48 UTC (permalink / raw)
To: linux-wireless
Hi,
After a lot of git-bisecting and other testing, it appears that commit
382a103b2b528a3085cde4ac56fc69d92a828b72 is the culprit.
To reproduce the problem, use backports-20130607, and the ath9k defconfig
(with no change otherwise).
The station is unable to authenticate with WPA2 to the AP. Interestingly
enough, if a monitor vif is created and upped beforehand, the authentication
succeeds.
After reverting the commit, authentication succeeds very quickly (without the
need to up a monitor vif), just as expected.
Note that I have kernel 3.8.0 (as per Ubuntu 13.04) and that original
ath9k_htc driver works as expected. This has been also tested on Debian with
driver from original kernel 3.9 (i.e. without backports).
For the moment I can live with the commit reverted, but I suppose it's been
there to fix something else, so this should probably be looked into by someone
more knowledgeable than me.
Ignacy
--
A person is shit's way of making more shit.
-- S. Barnett, anthropologist.
^ permalink raw reply
* Re: [RFC 2/2] mac80211: Fix bss ref leak.
From: Ben Greear @ 2013-06-18 16:33 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1371569205.8318.47.camel@jlt4.sipsolutions.net>
On 06/18/2013 08:26 AM, Johannes Berg wrote:
> On Mon, 2013-06-17 at 14:32 -0700, greearb@candelatech.com wrote:
>
>> static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
>> - bool assoc)
>> + bool assoc, bool put_bss)
>
> Do we _really_ need another argument? Shouldn't it always be put in the
> non-assoc case anyway, at least if non-NULL?
I don't think so. Check out the ieee80211_rx_mgmt_assoc_resp method.
if (status_code != WLAN_STATUS_SUCCESS) {
sdata_info(sdata, "%pM denied association (code=%d)\n",
mgmt->sa, status_code);
ieee80211_destroy_assoc_data(sdata, false, false);
This passes in false as 'assoc', but we should not free bss here because
it is being passed back to the calling method, and the return
code of RX_MGMT_CFG80211_RX_ASSOC means bss should eventually
be consumed by the cfg80211 logic.
Of course, this is all 'as far as I can tell'.
I sort of like the second boolean because it forces the caller to
think about whether bss should be freed or not...
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCHv3 05/18] mac80211: fix timing for 5 MHz and 10 MHz channels
From: Simon Wunderlich @ 2013-06-18 16:27 UTC (permalink / raw)
To: Johannes Berg
Cc: Simon Wunderlich, linux-wireless, Mathias Kretschmer,
Simon Wunderlich
In-Reply-To: <1371565283.8318.21.camel@jlt4.sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 791 bytes --]
On Tue, Jun 18, 2013 at 04:21:23PM +0200, Johannes Berg wrote:
>
> > +static inline int
> > +ieee80211_hw_get_divisor(struct ieee80211_hw *hw)
> > +{
> > + switch (hw->conf.chandef.width) {
> > + case NL80211_CHAN_WIDTH_5:
> > + return 4;
> > + case NL80211_CHAN_WIDTH_10:
> > + return 2;
> > + default:
> > + return 1;
> > + }
> > +}
>
> Using this function will break with drivers that use channel contexts,
> you really shouldn't do that.
Hmm ... yeah, you are right. I've used this a lot in the following patches, argh.
Will find another solution.
>
> Also, you're disabling HT so why bother changing minstrel_ht?
Mhm, yeah that is probably not neccesary. I'll remove that, however we remember
it once HT support should be added. ;)
Thanks,
Simon
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: Lots of confusion on bss refcounting.
From: Ben Greear @ 2013-06-18 15:59 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <1371570723.22256.0.camel@jlt4.sipsolutions.net>
On 06/18/2013 08:52 AM, Johannes Berg wrote:
>
>>> You mean ->current_bss? That should be handled in all the callbacks in
>>> sme.c or so
>>
>> Looks like much of the action happens on work-queues. I'm wondering if
>> we managed to delete wdev objects before we have completely cleaned up
>> in some cases...
>
> Don't we flush work structs appropriately?
Looks like it, from core.c in the netdev event handler:
/*
* Ensure that all events have been processed and
* freed.
*/
cfg80211_process_wdev_events(wdev);
/* I just added this to see if it helps... */
if (WARN_ON(wdev->current_bss)) {
cfg80211_unhold_bss(wdev->current_bss);
cfg80211_put_bss(wdev->wiphy, &wdev->current_bss->pub);
SET_BSS(wdev, NULL);
}
break;
Some of the unregister and similar sme.c calls that should be cleaning up
the current_bss have some early returns if state does not match expected
value. If the warning above hits, then probably we are hitting those
somehow.
If not, then I'll keep looking :)
Thanks,
Ben
>
> johannes
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCHv3 07/18] mac80211: add radiotap flag and handling for 5/10 MHz
From: Johannes Berg @ 2013-06-18 15:53 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Mathias Kretschmer, Simon Wunderlich
In-Reply-To: <20130618155051.GD27492@pandem0nium>
On Tue, 2013-06-18 at 17:50 +0200, Simon Wunderlich wrote:
> Hmm, if this [1] is the official standard, then no, it is not defined. Don't
> know when this was added in wireshark, and it was only added implicitly in
> the code (no header files etc). But since it was there, I figured it would
> be a good idea to re-use it (unaware that there was some "standard").
>
> How shall we proceed, propose that on some radiotap mailing list officially?
Yes, see http://www.radiotap.org/Standardisation
> > > + RX_FLAG_10MHZ = BIT(26),
> > > + RX_FLAG_5MHZ = BIT(27),
> >
> > Does that make sense? We know what kind of channel we're on? Though it
> > might be easier for the driver, so it may make sense I guess.
>
> Yeah, it's a little easier for the driver, and I thought it would be
> cleaner to get this reported through a flag.
Yeah I tend to agree.
johannes
^ permalink raw reply
* Re: Lots of confusion on bss refcounting.
From: Johannes Berg @ 2013-06-18 15:52 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <51C08119.3000407@candelatech.com>
> > You mean ->current_bss? That should be handled in all the callbacks in
> > sme.c or so
>
> Looks like much of the action happens on work-queues. I'm wondering if
> we managed to delete wdev objects before we have completely cleaned up
> in some cases...
Don't we flush work structs appropriately?
johannes
^ permalink raw reply
* Re: [PATCHv3 07/18] mac80211: add radiotap flag and handling for 5/10 MHz
From: Simon Wunderlich @ 2013-06-18 15:50 UTC (permalink / raw)
To: Johannes Berg
Cc: Simon Wunderlich, linux-wireless, Mathias Kretschmer,
Simon Wunderlich
In-Reply-To: <1371565626.8318.26.camel@jlt4.sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]
On Tue, Jun 18, 2013 at 04:27:06PM +0200, Johannes Berg wrote:
> On Thu, 2013-05-16 at 13:00 +0200, Simon Wunderlich wrote:
>
> > Wireshark
>
> doesn't really matter all that much, but I guess the radiotap standard
> also defines it.
>
Hmm, if this [1] is the official standard, then no, it is not defined. Don't
know when this was added in wireshark, and it was only added implicitly in
the code (no header files etc). But since it was there, I figured it would
be a good idea to re-use it (unaware that there was some "standard").
How shall we proceed, propose that on some radiotap mailing list officially?
>
> > + RX_FLAG_10MHZ = BIT(26),
> > + RX_FLAG_5MHZ = BIT(27),
>
> Does that make sense? We know what kind of channel we're on? Though it
> might be easier for the driver, so it may make sense I guess.
Yeah, it's a little easier for the driver, and I thought it would be
cleaner to get this reported through a flag.
Cheers,
Simon
[1] http://www.radiotap.org/defined-fields/Channel
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: Lots of confusion on bss refcounting.
From: Ben Greear @ 2013-06-18 15:47 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <1371559771.8318.12.camel@jlt4.sipsolutions.net>
On 06/18/2013 05:49 AM, Johannes Berg wrote:
> On Mon, 2013-06-17 at 17:30 -0700, Ben Greear wrote:
>> On 06/17/2013 02:31 PM, Ben Greear wrote:
>>> On 06/17/2013 12:09 PM, Ben Greear wrote:
>>>> On 06/17/2013 12:02 PM, Johannes Berg wrote:
>>>
>>>> The bss reference is passed back, and through luck or careful programming,
>>>> it *seems* that all paths related to calling ieee80211_rx_mgmt_assoc_resp
>>>> managed to consume the bss.
>>>>
>>>> I haven't figured out yet why this is not an erroneous put since I didn't
>>>> find the reference taken in the first place.
>>>>
>>>> I'm going to work on making some changes to the ref counting scheme
>>>> a bit. I'd rather have the code perhaps take and put a few refs
>>>> it might otherwise skip to keep the ownership cleaner and make
>>>> the code easier to debug and understand...
>>>>
>>>> I'll post some for RFC when I make some progress.
>>>
>>> I think I found at least some of the leaks.
>>>
>>> In places like ieee80211_mgd_stop, we were calling ieee80211_destroy_assoc_data,
>>> but it was not putting the bss reference.
>>>
>>> I'll post some RFC patches in a minute or two...first is debugging
>>> logic, second attempts to fix bss ref counting. This needs more
>>> testing before it is applied...we will continue testing it....
>>
>> It seems that the wdev objects (struct wireless_dev) can also
>> hold a reference to the bss.
>>
>> Do you happen to know what code is responsible for destructing
>> those objects? I want to check to make sure it properly puts
>> its reference.
>
> You mean ->current_bss? That should be handled in all the callbacks in
> sme.c or so
Looks like much of the action happens on work-queues. I'm wondering if
we managed to delete wdev objects before we have completely cleaned up
in some cases...
I was planning to add code to explicitly clean up current_bss if it
is not NULL in whatever code actually does the final cleanup for
wdev objects.
I didn't see an obvious cleanup method in sme.c, but will go look around
some more...
Thanks,
Ben
>
> johannes
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [RFC 2/2] mac80211: Fix bss ref leak.
From: Ben Greear @ 2013-06-18 15:38 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1371569205.8318.47.camel@jlt4.sipsolutions.net>
On 06/18/2013 08:26 AM, Johannes Berg wrote:
> On Mon, 2013-06-17 at 14:32 -0700, greearb@candelatech.com wrote:
>
>> static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
>> - bool assoc)
>> + bool assoc, bool put_bss)
>
> Do we _really_ need another argument? Shouldn't it always be put in the
> non-assoc case anyway, at least if non-NULL?
I can check if that is the case...
>
>> @@ -2415,6 +2415,10 @@ static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
>> ieee80211_vif_release_channel(sdata);
>> }
>>
>> + if (put_bss)
>> + cfg80211_put_bss(sdata->local->hw.wiphy, assoc_data->bss);
>> +
>> +
>
> just one blank line
>
>> + /** bss will be passed back to calling code, and that code must
>> + * deal with properly putting the reference.
>> + */
>
> /** is for kernel-doc only
>
>> return RX_MGMT_CFG80211_RX_ASSOC;
>
> You're working on some pretty old code here ... If you want this to be
> in stable the patch really needs to be smaller, I think. And for
> mac80211-next this can't apply.
I'm working on 3.9.6. When I get the problems fixed here, I can help
port this to whatever is the upstream kernel.
I'm not certain this is worth bothering with for stable anyway.
It seems the leaks are not too bad in general use cases,
and piddling around with code this tricky code could
introduce worse bugs that we may not immediately notice.
Thanks,
Ben
>
> johannes
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [RFC 2/2] mac80211: Fix bss ref leak.
From: Johannes Berg @ 2013-06-18 15:26 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless
In-Reply-To: <1371504746-8476-2-git-send-email-greearb@candelatech.com>
On Mon, 2013-06-17 at 14:32 -0700, greearb@candelatech.com wrote:
> static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
> - bool assoc)
> + bool assoc, bool put_bss)
Do we _really_ need another argument? Shouldn't it always be put in the
non-assoc case anyway, at least if non-NULL?
> @@ -2415,6 +2415,10 @@ static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
> ieee80211_vif_release_channel(sdata);
> }
>
> + if (put_bss)
> + cfg80211_put_bss(sdata->local->hw.wiphy, assoc_data->bss);
> +
> +
just one blank line
> + /** bss will be passed back to calling code, and that code must
> + * deal with properly putting the reference.
> + */
/** is for kernel-doc only
> return RX_MGMT_CFG80211_RX_ASSOC;
You're working on some pretty old code here ... If you want this to be
in stable the patch really needs to be smaller, I think. And for
mac80211-next this can't apply.
johannes
^ permalink raw reply
* Re: Connection not established with Realtek RTL8188CUS based USB device (EDIMAX)
From: Larry Finger @ 2013-06-18 15:26 UTC (permalink / raw)
To: Paul Menzel; +Cc: linux-wireless
In-Reply-To: <1371542085.7163.15.camel@mattotaupa>
On 06/18/2013 02:54 AM, Paul Menzel wrote:
> Three more questions regarding this patch.
>
> 1. Could you add `CC: stable@vger.kernel.org` to the patch so it gets
> backported to the stable Linux kernel releases.
I thought I had done that. In fact, the patch was Cc'd to stable, but I missed
the entry in the commit message. I have written to Stable to see what caqn be done.
> 2. Does this fix a regression or did it never work with WEP/WPA(1)
> networks before?
It does fix a regression, but I have no idea when it happened.
> 3. If wpa_supplicant only prints WPA to `/var/log/syslog`, can I be sure
> this is *no* WPA2 network? Because I always thought this is a WPA2
> network.
The data in an "iwlist scan" will state the version of WPA in use. If it is
WPA2, you get a line with
IE: IEEE 802.11i/WPA2 Version 1
For WPA1, it is
IE: WPA Version 1
Larry
^ permalink raw reply
* Re: [PATCHv2 2/5] nl80211/cfg80211: add channel switch command
From: Johannes Berg @ 2013-06-18 15:21 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Simon Wunderlich, Mathias Kretschmer
In-Reply-To: <20130618151424.GA27492@pandem0nium>
> > If you're going to use nested IEs, shouldn't you define a separate
> > namespace to be used within the nesting?
>
> The idea was to recycle the nl80211_parse_beacon() function for the CSA IEs,
> but if you prefer I can define an own namespace and create a separate function
> to check.
Hmm, ok. Not sure. This just seems like an awful lot of stack space,
increasing with each new attribute ...
johannes
^ permalink raw reply
* Re: [PATCHv2 2/5] nl80211/cfg80211: add channel switch command
From: Johannes Berg @ 2013-06-18 15:20 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Simon Wunderlich, Mathias Kretschmer
In-Reply-To: <20130618151806.GB27492@pandem0nium>
On Tue, 2013-06-18 at 17:18 +0200, Simon Wunderlich wrote:
> On Tue, Jun 18, 2013 at 04:50:30PM +0200, Johannes Berg wrote:
> >
> > > + * @NL80211_CMD_CHANNEL_SWITCH: Perform a channel switch by announcing the
> > > + * the new channel information (Channel Switch Announcement - CSA)
> > > + * in the beacon for some time (as defined in the
> > > + * %NL80211_ATTR_CH_SWITCH_COUNT parameter) and then change to the
> > > + * new channel. Userspace provides the new channel information (using
> > > + * %NL80211_ATTR_WIPHY_FREQ and the attributes determining channel
> > > + * width). %NL80211_ATTR_CH_SWITCH_BLOCK_TX may be supplied to inform
> > > + * other station that transmission must be blocked until the channel
> > > + * switch is complete.
> >
> > We probably need a flag to indicate that the command is available, since
> > mac80211 might implement it but not all drivers will?
>
> Yeah ... shall we announce it as CMD() in nl80211_send_wiphy()? Or feature
> flag?
CMD() works (make sure to only put it if split), but need a wiphy flag
since mac80211 always implements it, I think.
johannes
^ permalink raw reply
* Re: [PATCHv2 3/5] mac80211: add functions to duplicate a cfg80211_beacon
From: Simon Wunderlich @ 2013-06-18 15:20 UTC (permalink / raw)
To: Johannes Berg
Cc: Simon Wunderlich, linux-wireless, Simon Wunderlich,
Mathias Kretschmer
In-Reply-To: <1371566972.8318.33.camel@jlt4.sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 544 bytes --]
On Tue, Jun 18, 2013 at 04:49:32PM +0200, Johannes Berg wrote:
> On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
>
> > +static struct cfg80211_beacon_data *
> > +cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
> > +{
> > + struct cfg80211_beacon_data *new_beacon;
> > + new_beacon = kzalloc(sizeof(*new_beacon), GFP_KERNEL);
> > + if (!beacon)
> > + return NULL;
>
> Why not allocate one bigger block and use pointers into it? There's no
> (reasonable) way that it'd get really big ...
Yeah, can do that.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCHv2 2/5] nl80211/cfg80211: add channel switch command
From: Simon Wunderlich @ 2013-06-18 15:18 UTC (permalink / raw)
To: Johannes Berg
Cc: Simon Wunderlich, linux-wireless, Simon Wunderlich,
Mathias Kretschmer
In-Reply-To: <1371567030.8318.34.camel@jlt4.sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 921 bytes --]
On Tue, Jun 18, 2013 at 04:50:30PM +0200, Johannes Berg wrote:
>
> > + * @NL80211_CMD_CHANNEL_SWITCH: Perform a channel switch by announcing the
> > + * the new channel information (Channel Switch Announcement - CSA)
> > + * in the beacon for some time (as defined in the
> > + * %NL80211_ATTR_CH_SWITCH_COUNT parameter) and then change to the
> > + * new channel. Userspace provides the new channel information (using
> > + * %NL80211_ATTR_WIPHY_FREQ and the attributes determining channel
> > + * width). %NL80211_ATTR_CH_SWITCH_BLOCK_TX may be supplied to inform
> > + * other station that transmission must be blocked until the channel
> > + * switch is complete.
>
> We probably need a flag to indicate that the command is available, since
> mac80211 might implement it but not all drivers will?
Yeah ... shall we announce it as CMD() in nl80211_send_wiphy()? Or feature
flag?
Cheers,
Simon
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCHv2 2/5] nl80211/cfg80211: add channel switch command
From: Simon Wunderlich @ 2013-06-18 15:14 UTC (permalink / raw)
To: Johannes Berg
Cc: Simon Wunderlich, linux-wireless, Simon Wunderlich,
Mathias Kretschmer
In-Reply-To: <1371566890.8318.32.camel@jlt4.sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 1671 bytes --]
On Tue, Jun 18, 2013 at 04:48:10PM +0200, Johannes Berg wrote:
>
> > +/* struct cfg80211_csa_settings - channel switch settings
> > + *
>
> /**
> * struct ...
> * ...
>
OK
> > + * @NL80211_ATTR_CH_SWITCH_COUNT: u32 attribute specifying the number of TBTT's
> > + * until the channel switch event.
> > + * @NL80211_ATTR_CH_SWITCH_BLOCK_TX: flag attribute specifying that transmission
> > + * must be blocked on the current channel (before the channel switch
> > + * operation).
> > + * @NL80211_ATTR_CSA_IES: Nested set of attributes containing the IE information
> > + * for the time while performing a channel switch.
> > + * @NL80211_ATTR_CSA_C_OFF_BEACON: Offset of the channel switch counter
> > + * field in the beacons tail (%NL80211_ATTR_BEACON_TAIL).
> > + * @NL80211_ATTR_CSA_C_OFF_PRESP: Offset of the channel switch counter
> > + * field in the probe response (%NL80211_ATTR_PROBE_RESP).
>
> Shouldn't the offset be into the CSA_IES?
>
Hmm ... yeah you are right, that would be better. Will fix that.
> > + struct nlattr *csa_ies[NL80211_ATTR_MAX+1];
>
> Hmm, this doesn't seem right.
>
Why? If it's about namespace, see below. :)
> > + err = nla_parse_nested(csa_ies, NL80211_ATTR_MAX,
> > + info->attrs[NL80211_ATTR_CSA_IES],
> > + nl80211_policy);
>
> (Indentation)
oops...
>
> If you're going to use nested IEs, shouldn't you define a separate
> namespace to be used within the nesting?
The idea was to recycle the nl80211_parse_beacon() function for the CSA IEs,
but if you prefer I can define an own namespace and create a separate function
to check.
Cheers,
Simon
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCHv2 4/5] mac80211: add channel switch command and beacon callbacks
From: Johannes Berg @ 2013-06-18 15:00 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Simon Wunderlich, Mathias Kretschmer
In-Reply-To: <1371212124-26264-5-git-send-email-siwu@hrz.tu-chemnitz.de>
On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
> + * @channel_switch_beacon: Starts a channel switch for the to a new channel.
for the ?
> + * Beacons are modified to include CSA or ECSA IEs before calling this
> + * function. The corresponding count fields in these IEs must be
> + * decremented, and when they reach zero the driver must call
> + * ieee80211_csa_finish(). Drivers which use ieee80211_beacon_get()
> + * get the csa counter decremented by mac80211, but must check if it is
> + * zero using ieee80211_csa_is_complete() and then call
> + * ieee80211_csa_finish().
... when the beacon was transmitted, or something like that, right?
> @@ -2818,6 +2830,8 @@ struct ieee80211_ops {
> struct ieee80211_vif *vif,
> struct inet6_dev *idev);
> #endif
> + void (*channel_switch_beacon)(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif);
What about channel contexts? Actually I don't really understand this?
Shouldn't it say which channel to switch to?
> /**
> + * ieee80211_csa_finish - notify mac80211 about channel switch
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + *
> + * After a channel switch announcement was scheduled and the counter in this
> + * announcement hit zero, this function must be called by the driver to
> + * notify mac80211 that the channel can be changed.
> + */
> +void ieee80211_csa_finish(struct ieee80211_vif *vif);
If there are multiple interfaces, should it be called multiple times?
etc. Maybe it should be on a channel context instead?
> +/**
> + * ieee80211_csa_is_complete - find out if counters reached zero
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + *
> + * This function checks the registered beacon if the counters reached zero.
> + */
> +int ieee80211_csa_is_complete(struct ieee80211_vif *vif);
Huh? Return value? Maybe it should be bool?
> + netif_carrier_off(sdata->dev);
> + err = ieee80211_vif_use_channel(sdata, &local->csa_chandef,
> + IEEE80211_CHANCTX_SHARED);
> + netif_carrier_on(sdata->dev);
That seems like a really bad idea, deleting a channel context might tear
down all kinds of device state and might require deleting the interface
first ... I think the chan context API needs to be extended to switch
instead.
> + if (WARN_ON(err < 0))
> + return;
This can fail _easily_ too, e.g. if some other vif stays on the channel
and you're now using too many channel contexts.
> + /* don't handle if chanctx is used */
> + if (local->use_chanctx)
> + return -EBUSY;
Still don't really like the way you've implemented it :-)
> + rcu_read_lock();
> + mutex_lock(&local->chanctx_mtx);
Yeah right.
> +static inline void
> +drv_channel_switch_beacon(struct ieee80211_sub_if_data *sdata)
> +{
> + struct ieee80211_local *local = sdata->local;
> + if (local->ops->channel_switch_beacon) {
blank line between these two please
> + trace_drv_channel_switch_beacon(sdata);
> + local->ops->channel_switch_beacon(&local->hw, &sdata->vif);
> + }
> +}
> +
> +
steal one from here ;-)
> void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
> +/* channel switch handling */
I'd prefer a blank line before the comment
> +TRACE_EVENT(drv_channel_switch_beacon,
> + TP_PROTO(struct ieee80211_sub_if_data *sdata),
Use local_sdata_evt I think.
> +void ieee80211_csa_finish(struct ieee80211_vif *vif)
> +{
> + struct ieee80211_sub_if_data *sdata = NULL;
> + sdata = vif_to_sdata(vif);
ahem.
> + vif->csa_active = 0;
is that a counter, or should it be a bool?
> +static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
> + struct beacon_data *beacon)
> +{
> + struct probe_resp *resp;
> + int counter_beacon = sdata->csa_counter_offset_beacon;
> + int counter_presp = sdata->csa_counter_offset_presp;
> +
> + if (WARN_ON(counter_beacon > beacon->tail_len))
> + return;
> +
> + if (WARN_ON(((u8 *)beacon->tail)[counter_beacon] == 0))
> + return;
How can these happen?
> + ((u8 *)beacon->tail)[counter_beacon]--;
> +
> + if (counter_presp && sdata->vif.type == NL80211_IFTYPE_AP) {
> + resp = rcu_dereference(sdata->u.ap.probe_resp);
Who guarantees RCU protection?
> + if (WARN_ON(!resp))
> + return;
That can legimitately happen, no? At least userspace is allowed to not
set probe_resp now, if you want to change that ...
> + if (WARN_ON(counter_presp > resp->len))
> + return;
?
I guess counter_presp should be called "counter_offset_presp" or so
> + ((u8 *)resp->data)[counter_presp]--;
resp->data is already a u8 *, no?
> +int ieee80211_csa_is_complete(struct ieee80211_vif *vif)
> +{
> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> + struct beacon_data *beacon = NULL;
> + int counter_beacon = sdata->csa_counter_offset_beacon;
> +
> + if (!sdata || !ieee80211_sdata_running(sdata))
> + return 0;
yeah right ... container_of returned NULL?
> + if (vif->type == NL80211_IFTYPE_AP) {
> + struct ieee80211_if_ap *ap = &sdata->u.ap;
> + beacon = rcu_dereference(ap->beacon);
> +
swap those last two lines :)
johannes
^ permalink raw reply
* Re: [PATCHv2 2/5] nl80211/cfg80211: add channel switch command
From: Johannes Berg @ 2013-06-18 14:50 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Simon Wunderlich, Mathias Kretschmer
In-Reply-To: <1371212124-26264-3-git-send-email-siwu@hrz.tu-chemnitz.de>
> + * @NL80211_CMD_CHANNEL_SWITCH: Perform a channel switch by announcing the
> + * the new channel information (Channel Switch Announcement - CSA)
> + * in the beacon for some time (as defined in the
> + * %NL80211_ATTR_CH_SWITCH_COUNT parameter) and then change to the
> + * new channel. Userspace provides the new channel information (using
> + * %NL80211_ATTR_WIPHY_FREQ and the attributes determining channel
> + * width). %NL80211_ATTR_CH_SWITCH_BLOCK_TX may be supplied to inform
> + * other station that transmission must be blocked until the channel
> + * switch is complete.
We probably need a flag to indicate that the command is available, since
mac80211 might implement it but not all drivers will?
johannes
^ permalink raw reply
* Re: [PATCHv2 3/5] mac80211: add functions to duplicate a cfg80211_beacon
From: Johannes Berg @ 2013-06-18 14:49 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Simon Wunderlich, Mathias Kretschmer
In-Reply-To: <1371212124-26264-4-git-send-email-siwu@hrz.tu-chemnitz.de>
On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
> +static struct cfg80211_beacon_data *
> +cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
> +{
> + struct cfg80211_beacon_data *new_beacon;
> + new_beacon = kzalloc(sizeof(*new_beacon), GFP_KERNEL);
> + if (!beacon)
> + return NULL;
Why not allocate one bigger block and use pointers into it? There's no
(reasonable) way that it'd get really big ...
johannes
^ permalink raw reply
* Re: [PATCHv2 2/5] nl80211/cfg80211: add channel switch command
From: Johannes Berg @ 2013-06-18 14:48 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Simon Wunderlich, Mathias Kretschmer
In-Reply-To: <1371212124-26264-3-git-send-email-siwu@hrz.tu-chemnitz.de>
> +/* struct cfg80211_csa_settings - channel switch settings
> + *
/**
* struct ...
* ...
> + * @NL80211_ATTR_CH_SWITCH_COUNT: u32 attribute specifying the number of TBTT's
> + * until the channel switch event.
> + * @NL80211_ATTR_CH_SWITCH_BLOCK_TX: flag attribute specifying that transmission
> + * must be blocked on the current channel (before the channel switch
> + * operation).
> + * @NL80211_ATTR_CSA_IES: Nested set of attributes containing the IE information
> + * for the time while performing a channel switch.
> + * @NL80211_ATTR_CSA_C_OFF_BEACON: Offset of the channel switch counter
> + * field in the beacons tail (%NL80211_ATTR_BEACON_TAIL).
> + * @NL80211_ATTR_CSA_C_OFF_PRESP: Offset of the channel switch counter
> + * field in the probe response (%NL80211_ATTR_PROBE_RESP).
Shouldn't the offset be into the CSA_IES?
> + struct nlattr *csa_ies[NL80211_ATTR_MAX+1];
Hmm, this doesn't seem right.
> + err = nla_parse_nested(csa_ies, NL80211_ATTR_MAX,
> + info->attrs[NL80211_ATTR_CSA_IES],
> + nl80211_policy);
(Indentation)
If you're going to use nested IEs, shouldn't you define a separate
namespace to be used within the nesting?
johannes
^ permalink raw reply
* Re: [PATCHv2] Revert "mac80211: in IBSS use the Auth frame to trigger STA reinsertion"
From: Johannes Berg @ 2013-06-18 14:36 UTC (permalink / raw)
To: Antonio Quartulli; +Cc: linux-wireless, Antonio Quartulli
In-Reply-To: <1371558040-2821-1-git-send-email-ordex@autistici.org>
On Tue, 2013-06-18 at 14:20 +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@open-mesh.com>
>
> This reverts commit 6d810f10325522cfcf498dc6d64b9f96e1f5153f
>
> In this way an IBSS station will not use the AUTH messages
> to trigger a state reinitialisation anymore.
>
> The behaviour was racy and was not working properly.
> It has been introduced to help wpa_supplicant to support
> IBSS/RSN, however all the logic is now getting moved into
> wpa_s itself which will also be in charge of handling the
> AUTH messages thanks to the mgmt frame registration.
>
> If userspace does not register for receiving AUTH frames
> then mac80211 will still reply by itself.
>
> At the same time, the auth frame registration counter can be
> removed since it is not needed anymore.
Applied.
johannes
^ permalink raw reply
* Re: [PATCHv3 09/18] mac80211: change IBSS channel state to chandef
From: Johannes Berg @ 2013-06-18 14:28 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Mathias Kretschmer, Simon Wunderlich
In-Reply-To: <1368702045-27598-10-git-send-email-siwu@hrz.tu-chemnitz.de>
On Thu, 2013-05-16 at 13:00 +0200, Simon Wunderlich wrote:
> This should make some parts cleaner and is also required for handling
> 5/10 MHz properly.
Applied.
johannes
^ permalink raw reply
* Re: [PATCHv3 07/18] mac80211: add radiotap flag and handling for 5/10 MHz
From: Johannes Berg @ 2013-06-18 14:27 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Mathias Kretschmer, Simon Wunderlich
In-Reply-To: <1368702045-27598-8-git-send-email-siwu@hrz.tu-chemnitz.de>
On Thu, 2013-05-16 at 13:00 +0200, Simon Wunderlich wrote:
> Wireshark
doesn't really matter all that much, but I guess the radiotap standard
also defines it.
> + RX_FLAG_10MHZ = BIT(26),
> + RX_FLAG_5MHZ = BIT(27),
Does that make sense? We know what kind of channel we're on? Though it
might be easier for the driver, so it may make sense I guess.
johannes
^ permalink raw reply
* Re: [PATCHv3 06/18] mac80211: select and adjust bitrates according for channel mode
From: Johannes Berg @ 2013-06-18 14:25 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Mathias Kretschmer, Simon Wunderlich
In-Reply-To: <1368702045-27598-7-git-send-email-siwu@hrz.tu-chemnitz.de>
On Thu, 2013-05-16 at 13:00 +0200, Simon Wunderlich wrote:
> The various components accessing the bitrates table must use consider
> the used channel bandwidth to select only available rates or calculate
> the bitrate correctly.
>
> There are some rates in reduced bandwidth modes which can't be
> represented as multiples of 500kbps, like 2.25 MBit/s in 5 MHz mode. The
> standard suggests to round up to the next multiple of 500kbps, just do
> that in mac80211 as well.
"suggests", heh ...
> @@ -4238,7 +4238,6 @@ rate_lowest_index(struct ieee80211_supported_band *sband,
> struct ieee80211_sta *sta)
> {
> int i;
> -
> for (i = 0; i < sband->n_bitrates; i++)
please don't do that.
> for (i = 0; i < params->supported_rates_len; i++) {
> int rate = (params->supported_rates[i] & 0x7f) * 5;
> + int brate;
> for (j = 0; j < sband->n_bitrates; j++) {
> - if (sband->bitrates[j].bitrate == rate)
> + brate = sband->bitrates[j].bitrate;
> + brate = DIV_ROUND_UP(brate, divisor);
> + if ((rate_flags & sband->bitrates[i].flags)
> + != rate_flags)
> + continue;
> +
> + if (brate == rate)
> rates |= BIT(j);
> }
some refactoring might be useful ... having != at the start of a line is
a really good sign for that ;-)
johannes
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox