* 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
* Re: [PATCHv3 05/18] mac80211: fix timing for 5 MHz and 10 MHz channels
From: Johannes Berg @ 2013-06-18 14:21 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Mathias Kretschmer, Simon Wunderlich
In-Reply-To: <1368702045-27598-6-git-send-email-siwu@hrz.tu-chemnitz.de>
> +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.
Also, you're disabling HT so why bother changing minstrel_ht?
johannes
^ permalink raw reply
* Re: [RFC 0/4] add master channel switch announcement support
From: Johannes Berg @ 2013-06-18 14:01 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Mathias Kretschmer, Simon Wunderlich
In-Reply-To: <20130611141811.GA9490@pandem0nium>
On Tue, 2013-06-11 at 16:18 +0200, Simon Wunderlich wrote:
> > > I was thinking about adding another callback function or option for that for drivers
> > > who do the channel switch internally. Then we would only need mac80211 to adapt.
> > >
> > > Would that help, or what would be problematic?
> >
> > I don't really know. If you look at what we do in managed mode now, some
> > drivers will do the switching and report back when done.
>
> OK, maybe we should leave that open to people who implement that feature for
> their respective drivers.
We just have to be aware of it and make the API "offloading-friendly"
> What I'm concered about is to initiate a channel switch from userspace. This will be
> required for IBSS-DFS: when a radar is detected and this event is sent to userspace, it
> must react by initiating a channel switch.
Why would that not happen in the kernel directly?
johannes
^ permalink raw reply
* Re: [PATCH v3] mac80211: update mesh beacon on workqueue
From: Johannes Berg @ 2013-06-18 13:58 UTC (permalink / raw)
To: Thomas Pedersen; +Cc: open80211s, linux-wireless
In-Reply-To: <1371164081-5516-1-git-send-email-thomas@cozybit.com>
On Thu, 2013-06-13 at 15:54 -0700, Thomas Pedersen wrote:
> * @MESH_WORK_ROOT: the mesh root station needs to send a frame
> * @MESH_WORK_DRIFT_ADJUST: time to compensate for clock drift relative to other
> + * @MESH_WORK_MBSS_CHANGED: rebuild beacon and notify driver of BSS changes
> * mesh nodes
That seems to have gone wrong :)
I fixed it and applied this.
johannes
^ permalink raw reply
* Re: [PATCHv2 1/5] nl80211: use attributes to parse beacons
From: Johannes Berg @ 2013-06-18 13:56 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Simon Wunderlich, Mathias Kretschmer
In-Reply-To: <1371212124-26264-2-git-send-email-siwu@hrz.tu-chemnitz.de>
Taking the easy stuff first ...
On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
> only the attributes are required and not the whole netlink info, as the
> function accesses the attributes only anyway. This makes it easier to
> parse nested beacon IEs later.
I applied this.
johannes
^ permalink raw reply
* Re: pull-request: iwlwifi-fixes 2013-06-14
From: Johannes Berg @ 2013-06-18 13:53 UTC (permalink / raw)
To: John W. Linville; +Cc: linux-wireless
In-Reply-To: <20130614173419.GB28497@tuxdriver.com>
On Fri, 2013-06-14 at 13:34 -0400, John W. Linville wrote:
> It isn't obvious to me that these need to be included in 3.10. I can
> pull this into wireless-next once the current stuff in wireless makes
> it to davem's tree.
Please do, unless you changed your mind based on Emmanuel's reply.
johannes
^ permalink raw reply
* Re: Lots of confusion on bss refcounting.
From: Johannes Berg @ 2013-06-18 12:49 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <51BFAA34.1020407@candelatech.com>
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
johannes
^ permalink raw reply
* Re: [PATCH] nl80211: allow to register for mgmt frames without a matching pattern
From: Antonio Quartulli @ 2013-06-18 12:42 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Antonio Quartulli
In-Reply-To: <1371558772.8318.11.camel@jlt4.sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 812 bytes --]
On Tue, Jun 18, 2013 at 02:32:52PM +0200, Johannes Berg wrote:
> On Tue, 2013-06-18 at 14:29 +0200, Antonio Quartulli wrote:
> > From: Antonio Quartulli <antonio@open-mesh.com>
> >
> > It may be the case that a user wants to register for any
> > frame of a particular type.
> >
> > Allow this operation by passing null as matching pattern and
> > 0 as pattern len when registering for the frame.
> >
> > This is useful to wpa_supplicant when it wants to receive
> > any AUTH frame.
>
> We can (and do) pass a 0-length pattern, any problem with that?
Hello Johannes,
I did not know that netlink supports 0-length attribute.
Thanks for the hint. At this point this patch can be dropped.
Regards,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] nl80211: allow to register for mgmt frames without a matching pattern
From: Johannes Berg @ 2013-06-18 12:32 UTC (permalink / raw)
To: Antonio Quartulli; +Cc: linux-wireless, Antonio Quartulli
In-Reply-To: <1371558554-367-1-git-send-email-ordex@autistici.org>
On Tue, 2013-06-18 at 14:29 +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@open-mesh.com>
>
> It may be the case that a user wants to register for any
> frame of a particular type.
>
> Allow this operation by passing null as matching pattern and
> 0 as pattern len when registering for the frame.
>
> This is useful to wpa_supplicant when it wants to receive
> any AUTH frame.
We can (and do) pass a 0-length pattern, any problem with that?
johannes
^ permalink raw reply
* [PATCH] nl80211: allow to register for mgmt frames without a matching pattern
From: Antonio Quartulli @ 2013-06-18 12:29 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Antonio Quartulli
From: Antonio Quartulli <antonio@open-mesh.com>
It may be the case that a user wants to register for any
frame of a particular type.
Allow this operation by passing null as matching pattern and
0 as pattern len when registering for the frame.
This is useful to wpa_supplicant when it wants to receive
any AUTH frame.
Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
---
net/wireless/nl80211.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index e402819..878d346 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -7099,9 +7099,11 @@ static int nl80211_register_mgmt(struct sk_buff *skb, struct genl_info *info)
struct cfg80211_registered_device *rdev = info->user_ptr[0];
struct wireless_dev *wdev = info->user_ptr[1];
u16 frame_type = IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_ACTION;
+ void *data = nla_data(info->attrs[NL80211_ATTR_FRAME_MATCH]);
+ int len = 0;
- if (!info->attrs[NL80211_ATTR_FRAME_MATCH])
- return -EINVAL;
+ if (data)
+ len = nla_len(data);
if (info->attrs[NL80211_ATTR_FRAME_TYPE])
frame_type = nla_get_u16(info->attrs[NL80211_ATTR_FRAME_TYPE]);
@@ -7125,8 +7127,7 @@ static int nl80211_register_mgmt(struct sk_buff *skb, struct genl_info *info)
return -EOPNOTSUPP;
return cfg80211_mlme_register_mgmt(wdev, info->snd_portid, frame_type,
- nla_data(info->attrs[NL80211_ATTR_FRAME_MATCH]),
- nla_len(info->attrs[NL80211_ATTR_FRAME_MATCH]));
+ data, len);
}
static int nl80211_tx_mgmt(struct sk_buff *skb, struct genl_info *info)
--
1.8.1.5
^ permalink raw reply related
* [PATCHv2] Revert "mac80211: in IBSS use the Auth frame to trigger STA reinsertion"
From: Antonio Quartulli @ 2013-06-18 12:20 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Antonio Quartulli
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.
Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
---
v2:
- remove auth_frame_registrations member in ieee80211_if_ibss
Cheers,
net/mac80211/cfg.c | 10 ----------
net/mac80211/ibss.c | 41 +++++++----------------------------------
net/mac80211/ieee80211_i.h | 1 -
3 files changed, 7 insertions(+), 45 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 64cf294..98c323d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2927,16 +2927,6 @@ static void ieee80211_mgmt_frame_register(struct wiphy *wiphy,
struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev);
switch (frame_type) {
- case IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_AUTH:
- if (sdata->vif.type == NL80211_IFTYPE_ADHOC) {
- struct ieee80211_if_ibss *ifibss = &sdata->u.ibss;
-
- if (reg)
- ifibss->auth_frame_registrations++;
- else
- ifibss->auth_frame_registrations--;
- }
- break;
case IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_PROBE_REQ:
if (reg)
local->probe_req_reg++;
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index caa4b4f..6c9d21a 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -298,8 +298,7 @@ static void ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
tsf, false);
}
-static struct sta_info *ieee80211_ibss_finish_sta(struct sta_info *sta,
- bool auth)
+static struct sta_info *ieee80211_ibss_finish_sta(struct sta_info *sta)
__acquires(RCU)
{
struct ieee80211_sub_if_data *sdata = sta->sdata;
@@ -321,20 +320,12 @@ static struct sta_info *ieee80211_ibss_finish_sta(struct sta_info *sta,
/* If it fails, maybe we raced another insertion? */
if (sta_info_insert_rcu(sta))
return sta_info_get(sdata, addr);
- if (auth && !sdata->u.ibss.auth_frame_registrations) {
- ibss_dbg(sdata,
- "TX Auth SA=%pM DA=%pM BSSID=%pM (auth_transaction=1)\n",
- sdata->vif.addr, addr, sdata->u.ibss.bssid);
- ieee80211_send_auth(sdata, 1, WLAN_AUTH_OPEN, 0, NULL, 0,
- addr, sdata->u.ibss.bssid, NULL, 0, 0, 0);
- }
return sta;
}
static struct sta_info *
-ieee80211_ibss_add_sta(struct ieee80211_sub_if_data *sdata,
- const u8 *bssid, const u8 *addr,
- u32 supp_rates, bool auth)
+ieee80211_ibss_add_sta(struct ieee80211_sub_if_data *sdata, const u8 *bssid,
+ const u8 *addr, u32 supp_rates)
__acquires(RCU)
{
struct ieee80211_if_ibss *ifibss = &sdata->u.ibss;
@@ -385,7 +376,7 @@ ieee80211_ibss_add_sta(struct ieee80211_sub_if_data *sdata,
sta->sta.supp_rates[band] = supp_rates |
ieee80211_mandatory_rates(sband);
- return ieee80211_ibss_finish_sta(sta, auth);
+ return ieee80211_ibss_finish_sta(sta);
}
static void ieee80211_rx_mgmt_deauth_ibss(struct ieee80211_sub_if_data *sdata,
@@ -407,8 +398,6 @@ static void ieee80211_rx_mgmt_auth_ibss(struct ieee80211_sub_if_data *sdata,
size_t len)
{
u16 auth_alg, auth_transaction;
- struct sta_info *sta;
- u8 deauth_frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
sdata_assert_lock(sdata);
@@ -425,22 +414,6 @@ static void ieee80211_rx_mgmt_auth_ibss(struct ieee80211_sub_if_data *sdata,
if (auth_alg != WLAN_AUTH_OPEN || auth_transaction != 1)
return;
- sta_info_destroy_addr(sdata, mgmt->sa);
- sta = ieee80211_ibss_add_sta(sdata, mgmt->bssid, mgmt->sa, 0, false);
- rcu_read_unlock();
-
- /*
- * if we have any problem in allocating the new station, we reply with a
- * DEAUTH frame to tell the other end that we had a problem
- */
- if (!sta) {
- ieee80211_send_deauth_disassoc(sdata, sdata->u.ibss.bssid,
- IEEE80211_STYPE_DEAUTH,
- WLAN_REASON_UNSPECIFIED, true,
- deauth_frame_buf);
- return;
- }
-
/*
* IEEE 802.11 standard does not require authentication in IBSS
* networks and most implementations do not seem to use it.
@@ -506,7 +479,7 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
} else {
rcu_read_unlock();
sta = ieee80211_ibss_add_sta(sdata, mgmt->bssid,
- mgmt->sa, supp_rates, true);
+ mgmt->sa, supp_rates);
}
}
@@ -610,7 +583,7 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
ieee80211_sta_join_ibss(sdata, bss);
supp_rates = ieee80211_sta_get_rates(local, elems, band, NULL);
ieee80211_ibss_add_sta(sdata, mgmt->bssid, mgmt->sa,
- supp_rates, true);
+ supp_rates);
rcu_read_unlock();
}
@@ -982,7 +955,7 @@ void ieee80211_ibss_work(struct ieee80211_sub_if_data *sdata)
list_del(&sta->list);
spin_unlock_bh(&ifibss->incomplete_lock);
- ieee80211_ibss_finish_sta(sta, true);
+ ieee80211_ibss_finish_sta(sta);
rcu_read_unlock();
spin_lock_bh(&ifibss->incomplete_lock);
}
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 7a6f1a0..1d1aac6 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -497,7 +497,6 @@ struct ieee80211_if_ibss {
bool privacy;
bool control_port;
- unsigned int auth_frame_registrations;
u8 bssid[ETH_ALEN] __aligned(2);
u8 ssid[IEEE80211_MAX_SSID_LEN];
--
1.8.1.5
^ permalink raw reply related
* Re: [RFC 2/2] wireless: Make sure __cfg80211_connect_result always puts bss.
From: Johannes Berg @ 2013-06-18 12:19 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless
In-Reply-To: <1371515281-26879-2-git-send-email-greearb@candelatech.com>
On Mon, 2013-06-17 at 17:28 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> Otherwise, we can leak a bss reference.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> net/wireless/sme.c | 11 +++++++++--
> 1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/net/wireless/sme.c b/net/wireless/sme.c
> index 6066720..ea2ce33 100644
> --- a/net/wireless/sme.c
> +++ b/net/wireless/sme.c
> @@ -420,6 +420,7 @@ void cfg80211_sme_failed_assoc(struct wireless_dev *wdev)
> schedule_work(&rdev->conn_work);
> }
>
> +/** This method must consume bss one way or another */
> void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
> const u8 *req_ie, size_t req_ie_len,
> const u8 *resp_ie, size_t resp_ie_len,
> @@ -435,11 +436,17 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
> ASSERT_WDEV_LOCK(wdev);
>
> if (WARN_ON(wdev->iftype != NL80211_IFTYPE_STATION &&
> - wdev->iftype != NL80211_IFTYPE_P2P_CLIENT))
> + wdev->iftype != NL80211_IFTYPE_P2P_CLIENT)) {
> + if (bss)
> + cfg80211_put_bss(wdev->wiphy, bss);
> return;
> + }
This is reasonable, though it'd be stupid to call it in this case, I'm
not worried about leaking when the warning triggers.
> - if (wdev->sme_state != CFG80211_SME_CONNECTING)
> + if (wdev->sme_state != CFG80211_SME_CONNECTING) {
> + if (bss)
> + cfg80211_put_bss(wdev->wiphy, bss);
> return;
> + }
This code doesn't exist any more.
johannes
^ permalink raw reply
* Re: [RFC 1/2] mac80211: Stop timer before deleting data structures.
From: Johannes Berg @ 2013-06-18 12:14 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless
In-Reply-To: <1371515281-26879-1-git-send-email-greearb@candelatech.com>
On Mon, 2013-06-17 at 17:28 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> Not sure this matters, but it seems cleaner.
It really doesn't make a difference at all since it's all locked and the
timer just fires off the work struct.
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: track AP's beacon rate and give it to the driver
From: Johannes Berg @ 2013-06-18 12:06 UTC (permalink / raw)
To: linux-wireless; +Cc: Alexander Bondar
In-Reply-To: <1370974911-6074-1-git-send-email-johannes@sipsolutions.net>
On Tue, 2013-06-11 at 20:21 +0200, Johannes Berg wrote:
> From: Alexander Bondar <alexander.bondar@intel.com>
>
> Track the AP's beacon rate in the scan BSS data and in the
> interface configuration to let the drivers know which rate
> the AP is using. This information may be used by drivers,
> in our case to let the firmware optimise beacon RX.
Applied.
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