From: Michael Wu <[email protected]>
This patch makes mac80211 monitor interfaces use radiotap headers. It also
provides a bit to let a driver specify a frame has a radiotap header and
another bit to let the driver know if adding a radiotap header would be
helpful.
Thanks to Andy Green <[email protected]> for testing earlier versions of
this patch.
Signed-off-by: Michael Wu <[email protected]>
---
include/net/mac80211.h | 8 +++-
net/mac80211/ieee80211.c | 75 ++++++++++++++++++++++++++++++++++------
net/mac80211/ieee80211_iface.c | 2 +
3 files changed, 70 insertions(+), 15 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 1fb4bd9..6392c44 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -216,9 +216,6 @@ struct ieee80211_tx_control {
int ifindex; /* internal */
};
-#define RX_FLAG_MMIC_ERROR 0x1
-#define RX_FLAG_DECRYPTED 0x2
-
/* Receive status. The low-level driver should provide this information
* (the subset supported by hardware) to the 802.11 code with each received
* frame. */
@@ -232,6 +229,9 @@ struct ieee80211_rx_status {
int noise;
int antenna;
int rate;
+#define RX_FLAG_MMIC_ERROR (1<<0)
+#define RX_FLAG_DECRYPTED (1<<1)
+#define RX_FLAG_RADIOTAP (1<<2)
int flag;
};
@@ -278,6 +278,8 @@ struct ieee80211_conf {
#define IEEE80211_CONF_SHORT_SLOT_TIME (1<<0) /* use IEEE 802.11g Short Slot
* Time */
#define IEEE80211_CONF_SSID_HIDDEN (1<<1) /* do not broadcast the ssid */
+#define IEEE80211_CONF_RADIOTAP (1<<2) /* use radiotap if supported
+ check this bit at RX time */
u32 flags; /* configuration flags defined above */
u8 power_level; /* transmit power limit for current
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index ab51a74..51ad624 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -8,6 +8,7 @@
*/
#include <net/mac80211.h>
+#include <net/ieee80211_radiotap.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/netdevice.h>
@@ -291,6 +292,14 @@ int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
}
EXPORT_SYMBOL(ieee80211_get_hdrlen_from_skb);
+static int ieee80211_get_radiotap_len(struct sk_buff *skb)
+{
+ struct ieee80211_radiotap_header *hdr =
+ (struct ieee80211_radiotap_header *) skb->data;
+
+ return le16_to_cpu(hdr->it_len);
+}
+
#ifdef CONFIG_MAC80211_LOWTX_FRAME_DUMP
static void ieee80211_dump_frame(const char *ifname, const char *title,
const struct sk_buff *skb)
@@ -2355,6 +2364,7 @@ static int ieee80211_open(struct net_device *dev)
/* run the interface in a "soft monitor" mode */
local->monitors++;
local->open_count++;
+ local->hw.conf.flags |= IEEE80211_CONF_RADIOTAP;
return 0;
}
ieee80211_start_soft_monitor(local);
@@ -2403,9 +2413,10 @@ static int ieee80211_open(struct net_device *dev)
}
local->open_count++;
- if (sdata->type == IEEE80211_IF_TYPE_MNTR)
+ if (sdata->type == IEEE80211_IF_TYPE_MNTR) {
local->monitors++;
- else
+ local->hw.conf.flags |= IEEE80211_CONF_RADIOTAP;
+ } else
ieee80211_if_config(dev);
if (sdata->type == IEEE80211_IF_TYPE_STA &&
@@ -2430,13 +2441,18 @@ static int ieee80211_stop(struct net_device *dev)
/* remove "soft monitor" interface */
local->open_count--;
local->monitors--;
+ if (!local->monitors)
+ local->hw.conf.flags &= ~IEEE80211_CONF_RADIOTAP;
return 0;
}
netif_stop_queue(dev);
- if (sdata->type == IEEE80211_IF_TYPE_MNTR)
+ if (sdata->type == IEEE80211_IF_TYPE_MNTR) {
local->monitors--;
+ if (!local->monitors)
+ local->hw.conf.flags &= ~IEEE80211_CONF_RADIOTAP;
+ }
local->open_count--;
if (local->open_count == 0) {
@@ -2782,26 +2798,53 @@ ieee80211_rx_monitor(struct net_device *dev, struct sk_buff *skb,
struct ieee80211_rx_status *status)
{
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
- struct ieee80211_frame_info *fi;
struct ieee80211_sub_if_data *sdata;
- const size_t hlen = sizeof(struct ieee80211_frame_info)
- - sizeof(fi->msg_type);
+ struct ieee80211_rate *rate;
+ struct ieee80211_rtap_hdr {
+ struct ieee80211_radiotap_header hdr;
+ u8 flags;
+ u8 rate;
+ __le16 chan_freq;
+ __le16 chan_flags;
+ u8 antsignal;
+ } __attribute__ ((packed)) *rthdr;
skb->dev = dev;
sdata = IEEE80211_DEV_TO_SUB_IF(dev);
- if (skb_headroom(skb) < hlen) {
+ if (status->flag & RX_FLAG_RADIOTAP)
+ goto out;
+
+ if (skb_headroom(skb) < sizeof(*rthdr)) {
I802_DEBUG_INC(local->rx_expand_skb_head);
- if (pskb_expand_head(skb, hlen, 0, GFP_ATOMIC)) {
+ if (pskb_expand_head(skb, sizeof(*rthdr), 0, GFP_ATOMIC)) {
dev_kfree_skb(skb);
return;
}
}
- fi = (struct ieee80211_frame_info *) skb_push(skb, hlen);
+ rthdr = (struct ieee80211_rtap_hdr *) skb_push(skb, sizeof(*rthdr));
+ memset(rthdr, 0, sizeof(*rthdr));
+ rthdr->hdr.it_len = cpu_to_le16(sizeof(*rthdr));
+ rthdr->hdr.it_present =
+ cpu_to_le32((1 << IEEE80211_RADIOTAP_FLAGS) |
+ (1 << IEEE80211_RADIOTAP_RATE) |
+ (1 << IEEE80211_RADIOTAP_CHANNEL) |
+ (1 << IEEE80211_RADIOTAP_DB_ANTSIGNAL));
+ rthdr->flags = local->hw.flags & IEEE80211_HW_RX_INCLUDES_FCS ?
+ IEEE80211_RADIOTAP_F_FCS : 0;
+ rate = ieee80211_get_rate(local, status->phymode, status->rate);
+ if (rate)
+ rthdr->rate = rate->rate / 5;
+ rthdr->chan_freq = cpu_to_le16(status->freq);
+ rthdr->chan_flags =
+ status->phymode == MODE_IEEE80211A ?
+ cpu_to_le16(IEEE80211_CHAN_OFDM | IEEE80211_CHAN_5GHZ) :
+ cpu_to_le16(IEEE80211_CHAN_DYN | IEEE80211_CHAN_2GHZ);
+ rthdr->antsignal = status->ssi;
- ieee80211_fill_frame_info(local, fi, status);
+ out:
sdata->stats.rx_packets++;
sdata->stats.rx_bytes += skb->len;
@@ -3397,6 +3440,9 @@ ieee80211_rx_h_sta_process(struct ieee80211_txrx_data *rx)
return TXRX_QUEUED;
}
+ if (rx->u.rx.status->flag & RX_FLAG_RADIOTAP)
+ skb_pull(rx->skb, ieee80211_get_radiotap_len(rx->skb));
+
return TXRX_CONTINUE;
}
@@ -3772,6 +3818,12 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
struct ieee80211_txrx_data rx;
u16 type;
int multicast;
+ int radiotap_len = 0;
+
+ if (status->flag & RX_FLAG_RADIOTAP) {
+ radiotap_len = ieee80211_get_radiotap_len(skb);
+ skb_pull(skb, radiotap_len);
+ }
hdr = (struct ieee80211_hdr *) skb->data;
memset(&rx, 0, sizeof(rx));
@@ -3808,6 +3860,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
goto end;
skb = rx.skb;
+ skb_push(skb, radiotap_len);
if (sta && !sta->assoc_ap && !(sta->flags & WLAN_STA_WDS) &&
!local->iff_promiscs && !multicast) {
rx.u.rx.ra_match = 1;
@@ -3816,7 +3869,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
} else {
struct ieee80211_sub_if_data *prev = NULL;
struct sk_buff *skb_new;
- u8 *bssid = ieee80211_get_bssid(hdr, skb->len);
+ u8 *bssid = ieee80211_get_bssid(hdr, skb->len - radiotap_len);
list_for_each_entry(sdata, &local->sub_if_list, list) {
rx.u.rx.ra_match = 1;
diff --git a/net/mac80211/ieee80211_iface.c b/net/mac80211/ieee80211_iface.c
index b1b20ba..495177b 100644
--- a/net/mac80211/ieee80211_iface.c
+++ b/net/mac80211/ieee80211_iface.c
@@ -192,7 +192,7 @@ void ieee80211_if_set_type(struct net_device *dev, int type)
break;
}
case IEEE80211_IF_TYPE_MNTR:
- dev->type = ARPHRD_IEEE80211_PRISM;
+ dev->type = ARPHRD_IEEE80211_RADIOTAP;
break;
default:
printk(KERN_WARNING "%s: %s: Unknown interface type 0x%x",
Michael Wu wrote:
> How fast is hardware scanning?
I just hooked the hw scanning back up in iwlwifi (as of iwlwifi-0.0.14) The driver wasn't calling ieee80211_scan_complete() before to tell the stack the scan was done, so no results were being passing up.
Anyway, I ran a quick test of using hw_scan enabled and disabled on a system with 15 active and 17 passive channels.
hw scan: 2.2s
sw scan: 4.7s
So the SW scan wasn't the 10s that I stated before (although it sure felt like 10s before I ran it with 'time') But anyway... the hw scan is currently beating out sw scanning by ~54%, at least here.
James
On Tuesday 24 April 2007 12:20, Johannes Berg wrote:
> Nah. Is that missing from some previous patch?
>
Opps. Yeah, that was suppose to be part of patch 7.
-Michael Wu
Jiri Benc wrote:
> On Fri, 27 Apr 2007 16:30:02 +0100, Andy Green wrote:
>>> fixes the problem. Now, let's find something that fixes the problem as
>>> well but doesn't remove the functionality. James already proposed a
>>> solution that could work if a support for user space MLME is added. Do
>>> you have an idea how to add it?
>> Jiri, what functionality? The ability to change frequency periodically?
>> Isn't that something mac80211 can do already?
>
> Please see other mails in this thread. The opinions differ.
*shrug* if somebody has the opinion the sky isn't blue, the sky doesn't
care much.
> As long as it is implemented cleanly I have no objections against it.
Well then, that is how it will be.
> Btw, the main argument for hw_scan was power consumption. I'd still
> like to see numbers :-)
"firmware scan" I think you mean.
-Andy
On Fri, 2007-04-27 at 20:10 +0200, Jiri Benc wrote:
> On Fri, 27 Apr 2007 10:02:33 -0700, James Ketrenos wrote:
> > Jiri Benc wrote:
> > > The proposed solution (removing of the hw_scan callback) obviously
> > > fixes the problem.
> >
> > No, it doesn't fix the problem--it removes the functionality. Deleting
> > automatic AP selection "fixes the problem" too.
>
> Yes. But if these two features conflict with each other and I have to
> decide which one should be kept... well.. that's easy.
I don't see how they could possibly conflict methodically. They are two
paths to the same result. Or do you mean currently conflict technically
in the code?
dan
> Of course I'd prefer a correct fix.
>
> Btw, automatic AP selection works quite well for me (at least with
> bcm43xx).
>
> Jiri
>
Michael Wu wrote:
> On Monday 23 April 2007 16:58, Andy Green wrote:
>> Couldn't get this to apply on current wireless-dev, and there is a
>> trailing whitespace. Maybe it's a problem with a script I concocted to
>> extract the patch from saved Thunderbird 2.0 mails... ? Is it just that
>> wireless-dev changed inbetweentimes?
>>
> This patch series depends on the recent cfg80211/mac80211 rtnl locking
> patches.
Thanks for that. I added
cfg80211_nl80211-use-rtnl.patch
mac80211-update-for-cfg80211-rtnl-locking.patch
(these names are made from the originally posted subject line) before
your series and they went in fine. And the injection patches went in
fine on top of that, and continue to work.
However I saw this on reinsertion of zd1211rw-mac80211 module:
=============================================
[ INFO: possible recursive locking detected ]
2.6.21-rc7 #11
---------------------------------------------
ip/2600 is trying to acquire lock:
(&dev->_xmit_lock){-+..}, at: [<e09199c5>]
ieee80211_set_multicast_list+0x46/0x167 [mac80211]
but task is already holding lock:
(&dev->_xmit_lock){-+..}, at: [<c05abc05>] dev_mc_upload+0x14/0x3c
other info that might help us debug this:
2 locks held by ip/2600:
#0: (rtnl_mutex){--..}, at: [<c0609593>] mutex_lock+0x21/0x24
#1: (&dev->_xmit_lock){-+..}, at: [<c05abc05>] dev_mc_upload+0x14/0x3c
stack backtrace:
[<c0405ea6>] show_trace_log_lvl+0x1a/0x2f
[<c0406456>] show_trace+0x12/0x14
[<c04064da>] dump_stack+0x16/0x18
[<c04428c7>] __lock_acquire+0x116/0xb46
[<c04436b8>] lock_acquire+0x56/0x6f
[<c060a7d1>] _spin_lock+0x2b/0x38
[<e09199c5>] ieee80211_set_multicast_list+0x46/0x167 [mac80211]
[<c05abad8>] __dev_mc_upload+0x20/0x22
[<c05abc18>] dev_mc_upload+0x27/0x3c
[<c05a9e79>] dev_open+0x47/0x66
[<c05a87b9>] dev_change_flags+0x51/0xf1
[<c05e60c4>] devinet_ioctl+0x235/0x53b
[<c05e6684>] inet_ioctl+0x73/0x91
[<c059f531>] sock_ioctl+0x1ac/0x1c9
[<c048464a>] do_ioctl+0x22/0x67
[<c04848e1>] vfs_ioctl+0x252/0x265
[<c048493d>] sys_ioctl+0x49/0x64
[<c0404eec>] syscall_call+0x7/0xb
-Andy
On Fri, 2007-04-27 at 16:42 +0200, Jiri Benc wrote:
> On Fri, 27 Apr 2007 10:28:52 -0400, Dan Williams wrote:
> > We shouldn't be ignoring discrete hardware functionality just because
> > it's in the firmware and also may be in the stack. Hardware crypto like
> > somebody mentioned. TCP offload. etc. Not allowing a driver writer to
> > take advantage of hardware functionality is quite short-sighted. Having
> > a pure host-CPU software stack is not utopia; it's entirely unrealistic
> > for a variety of reasons, some of which are below [1].
>
> I'm not saying no (see my other mail in this thread), I see some
> reasons myself, but I don't think your reasons are valid.
My point is that things are not black and white. It's not fullmac vs.
softmac, there's a spectrum of parts in between. I'm not advocating
making mac80211 work with every part under the sun, but it seems that
we're not being open enough to functionality that might be in hardware.
Holding a 100% software line with mac80211 is just IMHO wrong and
short-sighted. The stack needs to be flexible WRT to the hardware
capabilities of the parts that we expect to use it. Saying no to
hardware scanning just because it can also be done in software too is
wrong.
We're not trying to limit the capabilities of hardware artificially, but
at the same time we're not going to allow crackpot hardware stuff to
clutter up the stack. Hardware scan is far from crackpot hardware
stuff; it's a simple, discrete function that shouldn't be hard work
with. It's already in, right? Ripping it out for a 100% software
agenda is wrong. Let's take out crypto offload then too if we're going
to take a consistently 100% software line. Again, it's not either
software or hardware; it's a spectrum of capabilities and we shouldn't
be making the stack _less_ flexible.
Dan
> > 1) power-critical situations like embedded devices where some pieces
> > must be offloaded to the wireless part
>
> Such solutions will use fullmac. See e.g. OLPC.
>
> > 2) lower-speed devices that may not have cycles to burn on functions
> > that the hardware can also do, even if most of the stack is software
>
> Such solutions have no other way than using fullmac.
>
> > 3) timing critical functions
>
> Such as? Scanning? That's not timing critical at all. Or something
> other?
>
> > 4) hybrid parts that are mostly softmac (ipw2100, ipw2200)
>
> Supporting of halfmacs in a softmac stack is hardly possible. You can
> write something like a "halfsoftmac" but you need to do that for each
> such chipset again. Because (by definition) every halfmac chipset needs
> a different set of functionality from the stack.
>
> > 5) we don't make hardware
>
> But we are also not required to support every obscure feature of the
> hardware.
>
> Thanks,
>
> Jiri
>
James Ketrenos wrote:
>> Sure there is. Smaller firmware means firmware that's less likely to
>> malfunction, which seems to be an issue.
>
> I haven't seen that as an issue; I've seen bugs in the driver that are
> misconfiguring the hardware or with the driver/stack trying to
> reconfigure things while a hardware scan is already occurring.
It seems that by definition this class of bugs would disappear if what
we should perhaps call "firmware scan" went away.
>> I don't see what you're saying about added complexity on the driver side.
>
> Right now the scanning in mac80211 is slow. I haven't really dug into
> it to see if it will transition from passive to active if it detects RF
> activity on an otherwise passive channel, etc. but I have seen that if
> we turn on hw scanning we can get results back in a few seconds vs. 10
> or more.
> The more passive channels you add (most of 802.11a) the slower the
> scanning gets.
Well just a datapoint on that (zd1211rw, presumably mac80211 scanned)
$ time iwlist mon0 scan
...
real 0m0.857s
user 0m0.001s
sys 0m0.004s
$
(I saw your other post also)
>> Eliminating the hw_scan callback reduces driver complexity.
>
> If done right, the stack would set up the list of channels to scan,
> whether to scan the channel active or passive, and the template for the
> probe request to use.
I think one can reasonably say that's a lot of stuff to support what is
a vendor-specific feature that only optimizes what can already be done.
> In order to do it all as fast as possible on the host you need access to
> all channel activity during the scan, especially on passive channels.
> If traffic is detected a probe request can be sent and then that channel
> can be given up on faster if it had to sit around for an arbitrary
> amount of time (110ms perhaps to ensure you catch beacons being
> transmitted at the vendor default interval of 100ms) Or if there is no
> traffic at all for some interval you could assume nothing is on that
> channel. Every 10 channels that's a full second you have to sit around...
>
> Unless there is a mechanism to quickly and easily toggle between filter
> and don't filter, you'll end up turning off hw/uCode filtering of
> packets all the time.
Maybe a simpler, more granular way to come at that is in the firmware to
allow selection of "beacon only" filtering, or maybe a count of valid
CRC packets that got filtered so you can assess if you should look
closer at that channel.
> Which means the hardware would be set to full promiscuous mode and every
> packet Rx'd would get tossed to the host to then either process or
> discard--which keeps the host CPU awake (which isn't good on laptops or
> anywhere else folks care about power consumption) And scanning is one
> of those things that happens more frequently when you are not plugged in
> and are moving around the home, school, office, etc.
Well a major thing for power saving is the duty cycle of the thing you
try to optimize. If scans take up 0.1% of the usage of the device even
(dubious), then making the system consume zero power during the scan
will make an extremely hard to measure difference. I don't think it
will be accepted by the Guardians Of The Code here to run the stack
always in hardware promisc as part of its standard operation, so have no
fear about that ;-)
> Not without an impact to power use. When 802.11n and 802.11e gets added
> to the mix, I'm not sure if it can realistically be eliminated through a
> host implementation without impacting performance.
I don't know anything about these new standards, but if they support the
powersave mode at the AP then Michael's fake powersave method (while we
tune the radio elsewhere for a while) would presumably handle it.
> Latency on getting scan results (especially as more passive channels are
> added) and platform power utilization while mobile.
>
>>> In the short term, I would rather we leave hw_scan in the code and
>>> have the
>>> users that currently rely on hw_scan just have to manually configure
>>> the AP
>>> selection until such time as the in-kernel-AP-selection-policy code
>>> works
>>> with hw offloaded scan.
>>>
>> What incentive does that give to fix the code?
>
> I want userspace to be able to fully configure the set of channels and
> parameters to pass to the hardware. But overhauling scan component to
> better support hardware scanning will likely take time.
Ultimately if it is possible to leverage the fake powersave mode to
allow monitor mode virtual interfaces to run on different frequencies on
the same hardware, then it will be possible to throw out the whole
concept of scanning from the stack. I just took a look at iwlist and
surely the combination of the code in there and the code in the stack to
package the results would be made radically simpler removing the stack
code entirely and bringing up a monitor mode virtual interface and
moving it around channels from usermode, filtering on beacon packets
using the pcap BPF stuff, and injecting probes if you want. The device
need only be in hardware promisc during the scan action.
For result latency, at the moment the monolithic scan is atomic, you sit
there with nothing and get your results all at once. With a usermode
driven scan, you will get results as you scan a channel, improving the
perception of latency.
>> Leaving broken things in for the sake of out-of-tree drivers does not
>> appeal to me.
>
> Nor to me--and we plan on getting iwlwifi in-tree. We also plan on
> using hw_scan. Keeping it in the stack for now helps me out. Removing
> it will make my life harder.
It seems it won't make your life that much harder, mac80211-based
scanning appears to work for you already, and we talk about 2 seconds
instead of 1 for results now and again.
-Andy
From: Michael Wu <[email protected]>
flush_scheduled_work in ieee80211_if_shutdown is called with rtnl held,
which can lead to deadlocks. This patch eliminates that by adding a new
single thread workqueue which can be safely flushed. It is also available
for driver use.
Signed-off-by: Michael Wu <[email protected]>
---
include/net/mac80211.h | 4 ++++
net/mac80211/ieee80211.c | 30 +++++++++++++-----------------
net/mac80211/ieee80211_iface.c | 2 +-
net/mac80211/ieee80211_sta.c | 24 ++++++++++++++++--------
net/mac80211/sta_info.c | 4 ++--
5 files changed, 36 insertions(+), 28 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 6392c44..b4b6619 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -465,6 +465,10 @@ struct ieee80211_hw {
/* assigned by mac80211, don't write */
struct ieee80211_conf conf;
+ /* Single thread workqueue available for driver use
+ * Allocated by mac80211 on registration */
+ struct workqueue_struct *workqueue;
+
/* Pointer to the private area that was
* allocated with this struct for you. */
void *priv;
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 8a3c749..25ea011 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -2234,18 +2234,13 @@ static void ieee80211_if_shutdown(struct net_device *dev)
case IEEE80211_IF_TYPE_IBSS:
sdata->u.sta.state = IEEE80211_DISABLED;
del_timer_sync(&sdata->u.sta.timer);
- flush_scheduled_work();
skb_queue_purge(&sdata->u.sta.skb_queue);
if (!local->ops->hw_scan &&
local->scan_dev == sdata->dev) {
local->sta_scanning = 0;
cancel_delayed_work(&local->scan_work);
- flush_scheduled_work();
- /* see comment in ieee80211_unregister_hw to
- * understand why this works */
- local->scan_dev = NULL;
- } else
- flush_scheduled_work();
+ }
+ flush_workqueue(local->hw.workqueue);
break;
}
}
@@ -2460,6 +2455,7 @@ static int ieee80211_stop(struct net_device *dev)
}
netif_stop_queue(dev);
+ ieee80211_if_shutdown(dev);
if (sdata->type == IEEE80211_IF_TYPE_MNTR) {
local->monitors--;
@@ -2486,7 +2482,6 @@ static int ieee80211_stop(struct net_device *dev)
conf.mac_addr = dev->dev_addr;
local->ops->remove_interface(local_to_hw(local), &conf);
}
- ieee80211_if_shutdown(dev);
ieee80211_start_hard_monitor(local);
@@ -4726,12 +4721,20 @@ EXPORT_SYMBOL(ieee80211_alloc_hw);
int ieee80211_register_hw(struct ieee80211_hw *hw)
{
struct ieee80211_local *local = hw_to_local(hw);
+ const char *name;
int result;
result = wiphy_register(local->hw.wiphy);
if (result < 0)
return result;
+ name = wiphy_dev(local->hw.wiphy)->driver->name;
+ local->hw.workqueue = create_singlethread_workqueue(name);
+ if (!local->hw.workqueue) {
+ result = -ENOMEM;
+ goto fail_workqueue;
+ }
+
debugfs_hw_add(local);
local->hw.conf.beacon_int = 1000;
@@ -4806,6 +4809,7 @@ fail_dev:
sta_info_stop(local);
fail_sta_info:
debugfs_hw_del(local);
+fail_workqueue:
wiphy_unregister(local->hw.wiphy);
return result;
}
@@ -4872,15 +4876,6 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
if (local->stat_time)
del_timer_sync(&local->stat_timer);
- if (!local->ops->hw_scan && local->scan_dev) {
- local->sta_scanning = 0;
- cancel_delayed_work(&local->scan_work);
- flush_scheduled_work();
- /* The scan_work is guaranteed not to be called at this
- * point. It is not scheduled and not running now. It can be
- * scheduled again only by sta_work (stopped by now) or under
- * rtnl lock. */
- }
ieee80211_rx_bss_list_deinit(local->mdev);
ieee80211_clear_tx_pending(local);
@@ -4900,6 +4895,7 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
skb_queue_purge(&local->skb_queue);
skb_queue_purge(&local->skb_queue_unreliable);
+ destroy_workqueue(local->hw.workqueue);
wiphy_unregister(local->hw.wiphy);
ieee80211_wep_free(local);
ieee80211_led_exit(local);
diff --git a/net/mac80211/ieee80211_iface.c b/net/mac80211/ieee80211_iface.c
index 719bc21..cf0f32e 100644
--- a/net/mac80211/ieee80211_iface.c
+++ b/net/mac80211/ieee80211_iface.c
@@ -179,7 +179,7 @@ void ieee80211_if_set_type(struct net_device *dev, int type)
ifsta = &sdata->u.sta;
INIT_WORK(&ifsta->work, ieee80211_sta_work);
setup_timer(&ifsta->timer, ieee80211_sta_timer,
- (unsigned long) ifsta);
+ (unsigned long) sdata);
skb_queue_head_init(&ifsta->skb_queue);
ifsta->capab = WLAN_CAPABILITY_ESS;
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 3d16ed8..0e9e4f9 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -1730,6 +1730,7 @@ static void ieee80211_rx_mgmt_probe_req(struct net_device *dev,
void ieee80211_sta_rx_mgmt(struct net_device *dev, struct sk_buff *skb,
struct ieee80211_rx_status *rx_status)
{
+ struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
struct ieee80211_sub_if_data *sdata;
struct ieee80211_if_sta *ifsta;
struct ieee80211_mgmt *mgmt;
@@ -1755,7 +1756,7 @@ void ieee80211_sta_rx_mgmt(struct net_device *dev, struct sk_buff *skb,
case IEEE80211_STYPE_DEAUTH:
case IEEE80211_STYPE_DISASSOC:
skb_queue_tail(&ifsta->skb_queue, skb);
- schedule_work(&ifsta->work);
+ queue_work(local->hw.workqueue, &ifsta->work);
return;
default:
printk(KERN_DEBUG "%s: received unknown management frame - "
@@ -1900,9 +1901,13 @@ static void ieee80211_sta_merge_ibss(struct net_device *dev,
void ieee80211_sta_timer(unsigned long data)
{
- struct ieee80211_if_sta *ifsta = (struct ieee80211_if_sta *) data;
+ struct ieee80211_sub_if_data *sdata =
+ (struct ieee80211_sub_if_data *) data;
+ struct ieee80211_if_sta *ifsta = &sdata->u.sta;
+ struct ieee80211_local *local = wdev_priv(&sdata->wdev);
+
set_bit(IEEE80211_STA_REQ_RUN, &ifsta->request);
- schedule_work(&ifsta->work);
+ queue_work(local->hw.workqueue, &ifsta->work);
}
@@ -2013,6 +2018,7 @@ static void ieee80211_sta_reset_auth(struct net_device *dev,
void ieee80211_sta_req_auth(struct net_device *dev,
struct ieee80211_if_sta *ifsta)
{
+ struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
if (sdata->type != IEEE80211_IF_TYPE_STA)
@@ -2021,7 +2027,7 @@ void ieee80211_sta_req_auth(struct net_device *dev,
if ((ifsta->bssid_set || ifsta->auto_bssid_sel) &&
(ifsta->ssid_set || ifsta->auto_ssid_sel)) {
set_bit(IEEE80211_STA_REQ_AUTH, &ifsta->request);
- schedule_work(&ifsta->work);
+ queue_work(local->hw.workqueue, &ifsta->work);
}
}
@@ -2574,7 +2580,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
if (sdata->type == IEEE80211_IF_TYPE_STA) {
if (sdata->u.sta.associated)
ieee80211_send_nullfunc(local, sdata, 0);
- ieee80211_sta_timer((unsigned long)&sdata->u.sta);
+ ieee80211_sta_timer((unsigned long)sdata);
}
netif_wake_queue(sdata->dev);
}
@@ -2666,7 +2672,8 @@ void ieee80211_sta_scan_work(struct work_struct *work)
}
if (local->sta_scanning)
- schedule_delayed_work(&local->scan_work, next_delay);
+ queue_delayed_work(local->hw.workqueue, &local->scan_work,
+ next_delay);
}
@@ -2735,7 +2742,8 @@ static int ieee80211_sta_start_scan(struct net_device *dev,
local->scan_channel_idx = 0;
local->scan_dev = dev;
/* TODO: start scan as soon as all nullfunc frames are ACKed */
- schedule_delayed_work(&local->scan_work, IEEE80211_CHANNEL_TIME);
+ queue_delayed_work(local->hw.workqueue, &local->scan_work,
+ IEEE80211_CHANNEL_TIME);
return 0;
}
@@ -2757,7 +2765,7 @@ int ieee80211_sta_req_scan(struct net_device *dev, u8 *ssid, size_t ssid_len)
}
set_bit(IEEE80211_STA_REQ_SCAN, &ifsta->request);
- schedule_work(&ifsta->work);
+ queue_work(local->hw.workqueue, &ifsta->work);
return 0;
}
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 4e341f6..62f90b4 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -184,7 +184,7 @@ struct sta_info * sta_info_add(struct ieee80211_local *local,
/* procfs entry adding might sleep, so schedule process context
* task for adding proc entry for STAs that do not yet have
* one. */
- schedule_work(&local->sta_proc_add);
+ queue_work(local->hw.workqueue, &local->sta_proc_add);
}
return sta;
@@ -275,7 +275,7 @@ void sta_info_free(struct sta_info *sta, int locked)
if (in_atomic()) {
list_add(&sta->list, &local->deleted_sta_list);
- schedule_work(&local->sta_proc_add);
+ queue_work(local->hw.workqueue, &local->sta_proc_add);
} else
finish_sta_info_free(local, sta);
}
On Fri, 27 Apr 2007 16:30:02 +0100, Andy Green wrote:
> > fixes the problem. Now, let's find something that fixes the problem as
> > well but doesn't remove the functionality. James already proposed a
> > solution that could work if a support for user space MLME is added. Do
> > you have an idea how to add it?
>
> Jiri, what functionality? The ability to change frequency periodically?
> Isn't that something mac80211 can do already?
Please see other mails in this thread. The opinions differ.
As long as it is implemented cleanly I have no objections against it.
Btw, the main argument for hw_scan was power consumption. I'd still
like to see numbers :-)
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
From: Michael Wu <[email protected]>
This prevents userspace and the in-kernel MLME from configuring
channel/BSSID/SSID at the same time when the in-kernel MLME is in the midst
of automatic AP selection. This is done by holding the RTNL lock.
Signed-off-by: Michael Wu <[email protected]>
---
net/mac80211/ieee80211_sta.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 0e9e4f9..04a63a7 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -2068,9 +2068,12 @@ static int ieee80211_sta_config_auth(struct net_device *dev,
struct ieee80211_sta_bss *bss, *selected = NULL;
int top_rssi = 0, freq;
+ rtnl_lock();
+
if (!ifsta->auto_channel_sel && !ifsta->auto_bssid_sel &&
!ifsta->auto_ssid_sel) {
ifsta->state = IEEE80211_AUTHENTICATE;
+ rtnl_unlock();
ieee80211_sta_reset_auth(dev, ifsta);
return 0;
}
@@ -2113,6 +2116,7 @@ static int ieee80211_sta_config_auth(struct net_device *dev,
ieee80211_sta_set_bssid(dev, selected->bssid);
ieee80211_rx_bss_put(dev, selected);
ifsta->state = IEEE80211_AUTHENTICATE;
+ rtnl_unlock();
ieee80211_sta_reset_auth(dev, ifsta);
return 0;
} else {
@@ -2123,6 +2127,7 @@ static int ieee80211_sta_config_auth(struct net_device *dev,
} else
ifsta->state = IEEE80211_DISABLED;
}
+ rtnl_unlock();
return -1;
}
James Ketrenos wrote:
> Andy Green wrote:
>> James Ketrenos wrote:
>>> The more passive channels you add (most of 802.11a) the slower the
>>> scanning gets.
>>
>> Well just a datapoint on that (zd1211rw, presumably mac80211 scanned)
>>
>> $ time iwlist mon0 scan
> ...
>> real 0m0.857s
>> user 0m0.001s
>> sys 0m0.004s
>
> How many channels are being scanned, and how many of those channels are
> passive only? And can you clarify on 'presumably'?
It's a BG stick, so 13. 'presumably' means I grepped the
zd1211rw-mac80211 dir looking for scan and didn't find anything, so I
"presume" the scan action is driven by mac80211.
>>>> Eliminating the hw_scan callback reduces driver complexity.
>>> If done right, the stack would set up the list of channels to scan,
>>> whether to scan the channel active or passive, and the template for the
>>> probe request to use.
>>
>> I think one can reasonably say that's a lot of stuff to support what is
>> a vendor-specific feature that only optimizes what can already be done.
>
> I think I would say that's a lot of stuff to make software scanning try
> and work as well as hardware scanning does now. hw_scan() is there now,
> it works, and its twice as fast as software scanning. The work should
> be to make software scanning better for hardware that can't do hardware
> scan, not to remove the ability for hardware scanning.
>
> ...
>> Maybe a simpler, more granular way to come at that is in the firmware to
>> allow selection of "beacon only" filtering, or maybe a count of valid
>> CRC packets that got filtered so you can assess if you should look
>> closer at that channel.
>
> Or maybe just let the firmware do the scan? :)
Well, let me put it this way... hiding the scan sequencing action even
in the stack seems like a bad thing to me, so hiding it further back
into a modal device-specific firmware action seems really wrong to me.
If scan is moved completely out of kernel mode, and instead the existing
primitive actions (change channel, set monitor mode, use libpcap BPF
filtering) enhanced and exploited to deliver better functionality (see
the latency subject below), the result is leaner kernel code, simpler
and all-in-one-place usermode code AND the architecture of empowering
the simpler already-exposed primitives allow re-use to do things that
aren't thought of yet.
Let's not forget that the bulk of the time this firmware assisted scan
action is doing nothing but letting the radio dwell on the frequency it
selected, for tens of ms. I'm not sure why including mac80211 in the
loop should make a tremendous difference.
> ...
>> For result latency, at the moment the monolithic scan is atomic, you sit
>> there with nothing and get your results all at once. With a usermode
>> driven scan, you will get results as you scan a channel, improving the
>> perception of latency.
>
> The action for finding your particular AP will always be atomic. If the
> AP you want is on a passive channel and you happen to miss the beacon in
> one pass, now the 4.7s "find the AP and associate via software" sequence
> is 9.4s... or 13.
> There is no 'perception of latency' when it comes to the question of
> "How long after I click the button to turn on wireless do I have to wait
> before I can hit refresh on my browser?"
I was talking about iwlist scan or populating a list of channels in a
GUI, but let me point out that in the case of the usermode
wpa_supplicant hunting an AP, if it is in control of the scan behaviour
because it drives the whole process from usermode as I propose it should
eventually, it becomes quite easy to prioritize the channel hunt order
on probability of where it has been seen before, and to modulate the
dwell time on particular channels as well, all of that done in usermode
according to wpa_supplicant's needs. Delivering much better performance
in the typical case. For all mac80211 drivers at once.
>> It seems it won't make your life that much harder
>
> Actually it will because we'll end up having to have our users use the
> external mac80211 subsystem for as long as hardware scanning is not
> supported by the kernel. Accelerated scanning is something end users
> like. I'm not going to give that up.
Well I don't have any right to tell you to give up on anything, but as
someone whose whole project is completely stalled by his code not
getting into mac80211, I can confidently assert you need to bring
mac80211 along with you in that case.
>> mac80211-based scanning appears to work for you already
>
> mac80211-based scanning worked as a stop gap. It is definitely not
> suitable as a final solution.
Hum well, it works, it's not that unsuitable. It's slower. It's still
suitable.
>> and we talk about 2 seconds instead of 1 for results now and again.
>
> It isn't 2s vs 1s -- we're talking 5s vs. 2s... and that's if it finds
> the AP on the first scan, which it doesn't always do. "now and again"
> is a lot more common than you'd like and when you're using a laptop and
> changing environments, roaming, AP hopping, etc.
Fine: we talk about 5s vs 2s "now and again" and when roaming and AP
hopping. And there is at least a competing concept to enhance mac80211
to reduce that down below 2s for many typical cases without special
vendor-specific tweaks so that all drivers would benefit.
-Andy
On Fri, 27 Apr 2007 10:56:33 -0400, Dan Williams wrote:
> My point is that things are not black and white. It's not fullmac vs.
> softmac, there's a spectrum of parts in between. I'm not advocating
> making mac80211 work with every part under the sun, but it seems that
> we're not being open enough to functionality that might be in hardware.
> Holding a 100% software line with mac80211 is just IMHO wrong and
> short-sighted. The stack needs to be flexible WRT to the hardware
> capabilities of the parts that we expect to use it. Saying no to
> hardware scanning just because it can also be done in software too is
> wrong.
Agreed.
> We're not trying to limit the capabilities of hardware artificially, but
> at the same time we're not going to allow crackpot hardware stuff to
> clutter up the stack. Hardware scan is far from crackpot hardware
> stuff; it's a simple, discrete function that shouldn't be hard work
> with. It's already in, right? Ripping it out for a 100% software
> agenda is wrong. Let's take out crypto offload then too if we're going
> to take a consistently 100% software line. Again, it's not either
> software or hardware; it's a spectrum of capabilities and we shouldn't
> be making the stack _less_ flexible.
The problem is that Michael during fixing of the stack for mainline
inclusion encountered a problem with the current implementation of the
hw scanning. We need to fix that problem.
The proposed solution (removing of the hw_scan callback) obviously
fixes the problem. Now, let's find something that fixes the problem as
well but doesn't remove the functionality. James already proposed a
solution that could work if a support for user space MLME is added. Do
you have an idea how to add it?
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
On Friday 27 April 2007 00:14, James Ketrenos wrote:
> If done right, the stack would set up the list of channels to scan, whether
> to scan the channel active or passive, and the template for the probe
> request to use.
>
> For sw based scans, the stack would then have a mechanism for executing
> that scan through a series of tunes, transmits, etc. For hw scans, it
> would pass that structure to the driver which would package it for the
> hardware and pass it down.
>
> Upon completion of the scan (sw or hw) the notification would come back to
> the stack to let it know scanning is complete.
>
Yes, this addresses many of the deficiencies in the current hw_scan api, but
that wasn't what I was concerned about.
> > What's the big bottleneck that justifies moving this to
> > hardware?
>
> With the 3945 its mainly power consumption.
>
Ok. Since you did not provide any numbers to back up that assertion, let's try
to find the upper bound for the cost of using software scanning.
With NetworkManager, userspace requests a scan about every 2 minutes when
connected. Assume that each scan takes 3 seconds. Assume that things are
ridiculously inefficient and software scanning keeps the cpu on for an
additional 20% of the scanning time. This keeps the CPU on unnecessarily for
18 seconds every hour, assuming nothing else is running. With an additional
assumption that all the additional CPU time that goes into software scan
directly subtracts from the battery life, this results in a loss of about 2
minutes and a half of battery life.. for a laptop that can last 8 hours.
An upper bound of 18 seconds of battery life lost per hour sounds okay to me
especially when it assumes the user is just letting the laptop idle. In
practice, I think that the difference is going to be negligible.. but what I
think is going to happen and my guess at the upper bound doesn't matter much.
If you can provide numbers, this discussion becomes significantly more
productive.
-Michael Wu
Jiri Benc wrote:
> fixes the problem. Now, let's find something that fixes the problem as
> well but doesn't remove the functionality. James already proposed a
> solution that could work if a support for user space MLME is added. Do
> you have an idea how to add it?
Jiri, what functionality? The ability to change frequency periodically?
Isn't that something mac80211 can do already?
What is the special sauce that just HAS to be in vendor firmware here?
-Andy
On Mon, 2007-04-23 at 14:48 -0400, Michael Wu wrote:
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -502,6 +502,7 @@ struct ieee80211_local {
> struct ieee80211_hw_mode *oper_hw_mode, *scan_hw_mode;
> u8 scan_ssid[IEEE80211_MAX_SSID_LEN];
> size_t scan_ssid_len;
> + u8 scan_bssid[ETH_ALEN];
> struct list_head sta_bss_list;
> struct ieee80211_sta_bss *sta_bss_hash[STA_HASH_SIZE];
> spinlock_t sta_bss_lock;
Do you see any use for setting scan_bssid to anything other than the
broadcast address? It doesn't seem useful to add this field here at this
time.
johannes
On Mon, 2007-04-23 at 14:48 -0400, Michael Wu wrote:
> --- a/net/mac80211/ieee80211_sta.c
> +++ b/net/mac80211/ieee80211_sta.c
> @@ -23,6 +23,7 @@
> #include <linux/wireless.h>
> #include <linux/random.h>
> #include <linux/etherdevice.h>
> +#include <linux/rtnetlink.h>
> #include <net/iw_handler.h>
> #include <asm/types.h>
> #include <asm/delay.h>
Nah. Is that missing from some previous patch?
johannes
On Mon, 23 Apr 2007 14:48:13 -0400, Michael Wu wrote:
> This prevents all tasklets from running when the device is down.
>
> [...]
> --- a/net/mac80211/ieee80211.c
> +++ b/net/mac80211/ieee80211.c
> @@ -2276,7 +2276,6 @@ static int ieee80211_master_open(struct net_device *dev)
> list_for_each_entry(sdata, &local->sub_if_list, list) {
> if (sdata->dev != dev && netif_running(sdata->dev)) {
> res = 0;
> - tasklet_enable(&local->tx_pending_tasklet);
> break;
> }
> }
> @@ -2289,7 +2288,6 @@ static int ieee80211_master_stop(struct net_device *dev)
> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> struct ieee80211_sub_if_data *sdata;
>
> - tasklet_disable(&local->tx_pending_tasklet);
> read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list) {
> if (sdata->dev != dev && netif_running(sdata->dev)) {
> @@ -2401,6 +2399,8 @@ static int ieee80211_open(struct net_device *dev)
>
> if (local->open_count == 0) {
> res = 0;
> + tasklet_enable(&local->tx_pending_tasklet);
> + tasklet_enable(&local->tasklet);
> if (local->ops->open)
> res = local->ops->open(local_to_hw(local));
> if (res == 0) {
> @@ -2475,6 +2475,8 @@ static int ieee80211_stop(struct net_device *dev)
> dev_close(local->apdev);
> if (local->ops->stop)
> local->ops->stop(local_to_hw(local));
> + tasklet_disable(&local->tx_pending_tasklet);
> + tasklet_disable(&local->tasklet);
This seems suspicious to me. Will it really work when somebody take the
master interface down by hand while some other interface is still
active?
Thanks,
Jiri
> }
> if (local->ops->remove_interface) {
> struct ieee80211_if_init_conf conf;
> @@ -4713,6 +4715,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
> tasklet_init(&local->tasklet,
> ieee80211_tasklet_handler,
> (unsigned long) local);
> + tasklet_disable(&local->tasklet);
> +
> skb_queue_head_init(&local->skb_queue);
> skb_queue_head_init(&local->skb_queue_unreliable);
>
>
--
Jiri Benc
SUSE Labs
Michael Wu wrote:
> This is fine, though it would be nice if we can shut the warning off.
> Virtual
> interfaces can try to take the xmit lock of the master device, but the
> master
> device will never try to take the xmit lock of the virtual interfaces and
> especially not while holding its own xmit lock.
You can shut the warning off by explicitly labeling the master lock as
belonging to a different class or something. Bridging code has similar
things IIRC, maybe check there.
johannes
On Fri, 2007-04-27 at 16:16 +0100, Andy Green wrote:
> Yeah but this isn't "in hardware" -- it's in firmware: software that
> runs of a different, vendor-specific, CPU.
[...]
Guys, this discussion is deteriorating rapidly!
The diff to rip out the hw_scan callback is tiny, and since it'd need to
be worked on anyway to add notification when scanning is done, IMHO it
is little more work to remove the scan now and add it back in one patch
including the notification. That just pushes the problem back to those
people who want/need it and then they can decide if it's worth the
effort and if they can show that it brings enough advantages.
The current cfg80211 scanning API is pretty much modeled on the 802.11
MLME interface and as such should be easy to push through to the
firmware if necessary, even through mac80211.
johannes
Dan Williams wrote:
>>> we're not being open enough to functionality that might be in hardware.
>>> Holding a 100% software line with mac80211 is just IMHO wrong and
>>> short-sighted. The stack needs to be flexible WRT to the hardware
>> Yeah but this isn't "in hardware" -- it's in firmware: software that
>
> I know that. But to the driver, it might as well be in hardware. The
> driver and stack doesn't care where the functionality lives (silicon or
> firmware) as long as the firmware does what's needed.
That's right, it "might as well be hardware" because there are no
sources for the firmware, despite it is as much software as anything
else. But architecturally, when we have the freedom to make a choice
where to place our bets, who CHOOSES to place it in closed source
firmware out of our control? Did you read the LICENSE file that came
with the iwlwifi firmware?
''Redistribution. Redistribution and use in binary form, without
modification, are permitted provided that the following conditions are
met:
* Redistributions must reproduce the above copyright notice and the
following disclaimer in the documentation and/or other materials
provided with the distribution.
* Neither the name of Intel Corporation nor the names of its suppliers
may be used to endorse or promote products derived from this software
without specific prior written permission.
* No reverse engineering, decompilation, or disassembly of this software
is permitted.''
Yeah that sounds like why we are all here, doesn't it.
>> runs of a different, vendor-specific, CPU. Let's not talk about magic
>> and impressive "hardware" when the truth is we only talk about the same
>> software action on another CPU. The problems with this particular
>> offload to firmware:
>>
>> - it is vendor-specific
>
> So is everything a driver does because the vendors make the hardware and
> firmware. We never get anywhere by being vendor-hostile. We also never
> get anywhere by kissing vendor ass. We need to toe a line in between.
Yes, it sounds like Jiri said, there are "opinions", since it is widely
known that the validity of all opinions are equal, no need therefore for
any actual project management decision based on, well, you know, logic
and philosophy.
>> - it is not time-critical. In fact it just sits there for tens of ms.
>> The extra us lost going through mac80211 when it wants to change the
>> frequency should not be measurable. It's not like it is some special
>> DSP in there that is computing PI faster than the main CPU in the box
>> can. It is just changing the frequency now and then, which can
>> perfectly well be done in the stack
>
> I'm using hw_scan as a vehicle for my larger point. But the point is
> still valid even if hw_scan is not time critical.
What larger point? We have to kiss vendor ass because you must never be
"hostile" towards people even if they are trying to "burn" you? Well
never mind.
>> - it is not exploitable in other ways. If we want to talk about
>> "short-sighted", let's talk about managing what can be very generic RF
>> hardware in a way that can never do anything but IEEE802.11a/b/g actions
>
> We can't and shouldn't try to coerce every piece of hardware into a 100%
> userspace-controllable SDR. While that would pretty much solve all
> _our_ problems (but not everyone else's), it's a pipe-dream and
> unrealistic.
Sorry, why is it a "pipe-dream and unrealistic" to support the common
denominator of the hardware in a flexible way? mac80211 already has a
concept of a bitmap of driver low level features. Well never mind.
>>> capabilities of the parts that we expect to use it. Saying no to
>>> hardware scanning just because it can also be done in software too is
>>> wrong.
>> To be clear, this isn't a "hardware" action, just an opaque software API
>> specific to that chipset, and that runs on a CPU in the chipset.
>>
>>> with. It's already in, right? Ripping it out for a 100% software
>>> agenda is wrong. Let's take out crypto offload then too if we're going
>> The iwlwifi proposal is for an opaque vendor-specific firmware API, that
>> is also "100% software". Don't get confused by the bogus magic alleged
>> "hardware" nomenclature.
>
> I've been burned by wifi firmware/hardware a lot in the past. All
> firmware is a black box API. But the reality of life is that we're not
> going to open all vendor firmware overnight, and I can't actually think
> of any vendor that provides the source and build tools for their
> firmware. That's a reality we have to deal with. We can and should
> encourage openness; but hostility gets us nowhere.
"The reality of life" is that Intel's closed firmware does not need to
have any special place with the scanning action. At this moment it can
safely be denied, repudiated and told to go fsck itself with no
functional deficit.
> Just because it's a black box _doesn't_ mean we make it impossible to
> utilize specific features that some hardware has, like crypto offload or
> hwscan. _That's_ the short-sighted part. We need to keep flexibility
> in the stack to make this sort of thing possible and let people
> experiment with it.
Yep *INTEL* will have a lot of fun "experimenting" with their
closed-source firmware. Nobody else or you can get sued. We are
talking about closed source *FIRMWARE* here, not hardware.
mac80211 can be an enabler of moving stuff behind the closed barrier or
can discourage it, and that is a philosophical issue controlled by the
project leaders. The project is BY NO MEANS A POWERLESS BYSTANDER in
this case.
-Andy
Jiri Benc wrote:
> The problem is that Michael during fixing of the stack for mainline
> inclusion encountered a problem with the current implementation of the
> hw scanning. We need to fix that problem.
All his patch does is delete the hw_scan callback. It doesn't *fix* anything.
> The proposed solution (removing of the hw_scan callback) obviously
> fixes the problem.
No, it doesn't fix the problem--it removes the functionality. Deleting automatic AP selection "fixes the problem" too.
> Now, let's find something that fixes the problem as
> well but doesn't remove the functionality. James already proposed a
> solution that could work if a support for user space MLME is added. Do
> you have an idea how to add it?
userspace MLME needs to have a scan command if you really want to have efficient scanning controlled by userspace. Tuning, waiting, tuning, waiting, tx, listening, tuning, ... introduces a *lot* of userspace/kernel/hardware bouncing.
The userspace MLME can evolve over time.
User space can try and do sw scanning even if the hw_scan callback is setup. Nothing about adding hw_scan prevents userspace from thrashing between the kernel, the drivers, and the host to chew up extra CPU cycles.
James
Updated patch attached. I did not add documentation about set_multicast_list
since documenting what conditions callbacks are called with is an issue
separate from this patch. I don't think I changed it much in this patch. Also
did not remove the dead code since it never compiled in the first place so I
didn't make it any more broken. That can go in another patch. The random RTNL
changes were removed, one trailing space identified by Andy Green removed, an
unnecessary dev_hold/dev_put was removed, and the nested locking in
ieee80211_ioctl.c was removed.
Thanks,
-Michael Wu
From: Michael Wu <[email protected]>
This prevents all tasklets from running when the device is down.
Signed-off-by: Michael Wu <[email protected]>
---
net/mac80211/ieee80211.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index bc89ac9..7743ca0 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -2276,7 +2276,6 @@ static int ieee80211_master_open(struct net_device *dev)
list_for_each_entry(sdata, &local->sub_if_list, list) {
if (sdata->dev != dev && netif_running(sdata->dev)) {
res = 0;
- tasklet_enable(&local->tx_pending_tasklet);
break;
}
}
@@ -2289,7 +2288,6 @@ static int ieee80211_master_stop(struct net_device *dev)
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
struct ieee80211_sub_if_data *sdata;
- tasklet_disable(&local->tx_pending_tasklet);
read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
if (sdata->dev != dev && netif_running(sdata->dev)) {
@@ -2401,6 +2399,8 @@ static int ieee80211_open(struct net_device *dev)
if (local->open_count == 0) {
res = 0;
+ tasklet_enable(&local->tx_pending_tasklet);
+ tasklet_enable(&local->tasklet);
if (local->ops->open)
res = local->ops->open(local_to_hw(local));
if (res == 0) {
@@ -2475,6 +2475,8 @@ static int ieee80211_stop(struct net_device *dev)
dev_close(local->apdev);
if (local->ops->stop)
local->ops->stop(local_to_hw(local));
+ tasklet_disable(&local->tx_pending_tasklet);
+ tasklet_disable(&local->tasklet);
}
if (local->ops->remove_interface) {
struct ieee80211_if_init_conf conf;
@@ -4713,6 +4715,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
tasklet_init(&local->tasklet,
ieee80211_tasklet_handler,
(unsigned long) local);
+ tasklet_disable(&local->tasklet);
+
skb_queue_head_init(&local->skb_queue);
skb_queue_head_init(&local->skb_queue_unreliable);
From: Michael Wu <[email protected]>
Update the radiotap header with what's in wireless-2.6. Also remove the
comment about IEEE80211_RADIOTAP_FCS.
Signed-off-by: Michael Wu <[email protected]>
---
include/net/ieee80211_radiotap.h | 77 ++++++++++++++++++++++++++------------
1 files changed, 52 insertions(+), 25 deletions(-)
diff --git a/include/net/ieee80211_radiotap.h b/include/net/ieee80211_radiotap.h
index 429b738..a0c2b41 100644
--- a/include/net/ieee80211_radiotap.h
+++ b/include/net/ieee80211_radiotap.h
@@ -66,7 +66,9 @@
*/
#define IEEE80211_RADIOTAP_HDRLEN 64
-/* The radio capture header precedes the 802.11 header. */
+/* The radio capture header precedes the 802.11 header.
+ * All data in the header is little endian on all platforms.
+ */
struct ieee80211_radiotap_header {
u8 it_version; /* Version 0. Only increases
* for drastic changes,
@@ -74,12 +76,12 @@ struct ieee80211_radiotap_header {
* new fields does not count.
*/
u8 it_pad;
- u16 it_len; /* length of the whole
+ __le16 it_len; /* length of the whole
* header in bytes, including
* it_version, it_pad,
* it_len, and data fields.
*/
- u32 it_present; /* A bitmap telling which
+ __le32 it_present; /* A bitmap telling which
* fields are present. Set bit 31
* (0x80000000) to extend the
* bitmap by another 32 bits.
@@ -88,89 +90,102 @@ struct ieee80211_radiotap_header {
*/
};
-/* Name Data type Units
- * ---- --------- -----
+/* Name Data type Units
+ * ---- --------- -----
*
- * IEEE80211_RADIOTAP_TSFT u64 microseconds
+ * IEEE80211_RADIOTAP_TSFT __le64 microseconds
*
* Value in microseconds of the MAC's 64-bit 802.11 Time
* Synchronization Function timer when the first bit of the
* MPDU arrived at the MAC. For received frames, only.
*
- * IEEE80211_RADIOTAP_CHANNEL 2 x u16 MHz, bitmap
+ * IEEE80211_RADIOTAP_CHANNEL 2 x __le16 MHz, bitmap
*
* Tx/Rx frequency in MHz, followed by flags (see below).
*
- * IEEE80211_RADIOTAP_FHSS u16 see below
+ * IEEE80211_RADIOTAP_FHSS __le16 see below
*
* For frequency-hopping radios, the hop set (first byte)
* and pattern (second byte).
*
- * IEEE80211_RADIOTAP_RATE u8 500kb/s
+ * IEEE80211_RADIOTAP_RATE u8 500kb/s
*
* Tx/Rx data rate
*
- * IEEE80211_RADIOTAP_DBM_ANTSIGNAL int8_t decibels from
- * one milliwatt (dBm)
+ * IEEE80211_RADIOTAP_DBM_ANTSIGNAL s8 decibels from
+ * one milliwatt (dBm)
*
* RF signal power at the antenna, decibel difference from
* one milliwatt.
*
- * IEEE80211_RADIOTAP_DBM_ANTNOISE int8_t decibels from
- * one milliwatt (dBm)
+ * IEEE80211_RADIOTAP_DBM_ANTNOISE s8 decibels from
+ * one milliwatt (dBm)
*
* RF noise power at the antenna, decibel difference from one
* milliwatt.
*
- * IEEE80211_RADIOTAP_DB_ANTSIGNAL u8 decibel (dB)
+ * IEEE80211_RADIOTAP_DB_ANTSIGNAL u8 decibel (dB)
*
* RF signal power at the antenna, decibel difference from an
* arbitrary, fixed reference.
*
- * IEEE80211_RADIOTAP_DB_ANTNOISE u8 decibel (dB)
+ * IEEE80211_RADIOTAP_DB_ANTNOISE u8 decibel (dB)
*
* RF noise power at the antenna, decibel difference from an
* arbitrary, fixed reference point.
*
- * IEEE80211_RADIOTAP_LOCK_QUALITY u16 unitless
+ * IEEE80211_RADIOTAP_LOCK_QUALITY __le16 unitless
*
* Quality of Barker code lock. Unitless. Monotonically
* nondecreasing with "better" lock strength. Called "Signal
* Quality" in datasheets. (Is there a standard way to measure
* this?)
*
- * IEEE80211_RADIOTAP_TX_ATTENUATION u16 unitless
+ * IEEE80211_RADIOTAP_TX_ATTENUATION __le16 unitless
*
* Transmit power expressed as unitless distance from max
* power set at factory calibration. 0 is max power.
* Monotonically nondecreasing with lower power levels.
*
- * IEEE80211_RADIOTAP_DB_TX_ATTENUATION u16 decibels (dB)
+ * IEEE80211_RADIOTAP_DB_TX_ATTENUATION __le16 decibels (dB)
*
* Transmit power expressed as decibel distance from max power
* set at factory calibration. 0 is max power. Monotonically
* nondecreasing with lower power levels.
*
- * IEEE80211_RADIOTAP_DBM_TX_POWER int8_t decibels from
- * one milliwatt (dBm)
+ * IEEE80211_RADIOTAP_DBM_TX_POWER s8 decibels from
+ * one milliwatt (dBm)
*
* Transmit power expressed as dBm (decibels from a 1 milliwatt
* reference). This is the absolute power level measured at
* the antenna port.
*
- * IEEE80211_RADIOTAP_FLAGS u8 bitmap
+ * IEEE80211_RADIOTAP_FLAGS u8 bitmap
*
* Properties of transmitted and received frames. See flags
* defined below.
*
- * IEEE80211_RADIOTAP_ANTENNA u8 antenna index
+ * IEEE80211_RADIOTAP_ANTENNA u8 antenna index
*
* Unitless indication of the Rx/Tx antenna for this packet.
* The first antenna is antenna 0.
*
- * IEEE80211_RADIOTAP_FCS u32 data
+ * IEEE80211_RADIOTAP_RX_FLAGS __le16 bitmap
+ *
+ * Properties of received frames. See flags defined below.
+ *
+ * IEEE80211_RADIOTAP_TX_FLAGS __le16 bitmap
+ *
+ * Properties of transmitted frames. See flags defined below.
+ *
+ * IEEE80211_RADIOTAP_RTS_RETRIES u8 data
+ *
+ * Number of rts retries a transmitted frame used.
+ *
+ * IEEE80211_RADIOTAP_DATA_RETRIES u8 data
+ *
+ * Number of unicast retries a transmitted frame used.
*
- * FCS from frame in network byte order.
*/
enum ieee80211_radiotap_type {
IEEE80211_RADIOTAP_TSFT = 0,
@@ -187,7 +202,11 @@ enum ieee80211_radiotap_type {
IEEE80211_RADIOTAP_ANTENNA = 11,
IEEE80211_RADIOTAP_DB_ANTSIGNAL = 12,
IEEE80211_RADIOTAP_DB_ANTNOISE = 13,
- IEEE80211_RADIOTAP_EXT = 31,
+ IEEE80211_RADIOTAP_RX_FLAGS = 14,
+ IEEE80211_RADIOTAP_TX_FLAGS = 15,
+ IEEE80211_RADIOTAP_RTS_RETRIES = 16,
+ IEEE80211_RADIOTAP_DATA_RETRIES = 17,
+ IEEE80211_RADIOTAP_EXT = 31
};
/* Channel flags. */
@@ -219,6 +238,14 @@ enum ieee80211_radiotap_type {
* 802.11 header and payload
* (to 32-bit boundary)
*/
+/* For IEEE80211_RADIOTAP_RX_FLAGS */
+#define IEEE80211_RADIOTAP_F_RX_BADFCS 0x0001 /* frame failed crc check */
+
+/* For IEEE80211_RADIOTAP_TX_FLAGS */
+#define IEEE80211_RADIOTAP_F_TX_FAIL 0x0001 /* failed due to excessive
+ * retries */
+#define IEEE80211_RADIOTAP_F_TX_CTS 0x0002 /* used cts 'protection' */
+#define IEEE80211_RADIOTAP_F_TX_RTS 0x0004 /* used rts/cts handshake */
/* Ugly macro to convert literal channel numbers into their mhz equivalents
* There are certianly some conditions that will break this (like feeding it '30')
On Fri, 2007-04-27 at 16:16 +0100, Andy Green wrote:
> Dan Williams wrote:
>
> > we're not being open enough to functionality that might be in hardware.
> > Holding a 100% software line with mac80211 is just IMHO wrong and
> > short-sighted. The stack needs to be flexible WRT to the hardware
>
> Yeah but this isn't "in hardware" -- it's in firmware: software that
I know that. But to the driver, it might as well be in hardware. The
driver and stack doesn't care where the functionality lives (silicon or
firmware) as long as the firmware does what's needed.
> runs of a different, vendor-specific, CPU. Let's not talk about magic
> and impressive "hardware" when the truth is we only talk about the same
> software action on another CPU. The problems with this particular
> offload to firmware:
>
> - it is vendor-specific
So is everything a driver does because the vendors make the hardware and
firmware. We never get anywhere by being vendor-hostile. We also never
get anywhere by kissing vendor ass. We need to toe a line in between.
> - it is not time-critical. In fact it just sits there for tens of ms.
> The extra us lost going through mac80211 when it wants to change the
> frequency should not be measurable. It's not like it is some special
> DSP in there that is computing PI faster than the main CPU in the box
> can. It is just changing the frequency now and then, which can
> perfectly well be done in the stack
I'm using hw_scan as a vehicle for my larger point. But the point is
still valid even if hw_scan is not time critical.
> - it is not exploitable in other ways. If we want to talk about
> "short-sighted", let's talk about managing what can be very generic RF
> hardware in a way that can never do anything but IEEE802.11a/b/g actions
We can't and shouldn't try to coerce every piece of hardware into a 100%
userspace-controllable SDR. While that would pretty much solve all
_our_ problems (but not everyone else's), it's a pipe-dream and
unrealistic.
> > capabilities of the parts that we expect to use it. Saying no to
> > hardware scanning just because it can also be done in software too is
> > wrong.
>
> To be clear, this isn't a "hardware" action, just an opaque software API
> specific to that chipset, and that runs on a CPU in the chipset.
>
> > with. It's already in, right? Ripping it out for a 100% software
> > agenda is wrong. Let's take out crypto offload then too if we're going
>
> The iwlwifi proposal is for an opaque vendor-specific firmware API, that
> is also "100% software". Don't get confused by the bogus magic alleged
> "hardware" nomenclature.
I've been burned by wifi firmware/hardware a lot in the past. All
firmware is a black box API. But the reality of life is that we're not
going to open all vendor firmware overnight, and I can't actually think
of any vendor that provides the source and build tools for their
firmware. That's a reality we have to deal with. We can and should
encourage openness; but hostility gets us nowhere.
Just because it's a black box _doesn't_ mean we make it impossible to
utilize specific features that some hardware has, like crypto offload or
hwscan. _That's_ the short-sighted part. We need to keep flexibility
in the stack to make this sort of thing possible and let people
experiment with it.
Dan
> > to take a consistently 100% software line. Again, it's not either
> > software or hardware; it's a spectrum of capabilities and we shouldn't
> > be making the stack _less_ flexible.
>
> Yeah let's make the stack completely flexible and either keep control of
> the scan action in the stack, or eventually move it to usermode, in both
> cases doing the scan action well ONCE for ALL drivers.
>
> -Andy
On Tue, Apr 24, 2007 at 10:03:03PM -0700, James Ketrenos wrote:
> (I would like to see hw_scan remain since iwlwifi uses it -- which we hope
> to submit as soon as the code restructuring is complete)
I was going to mention this in response to Michael's patch, but then
I checked your current iwlwifi git tree. There your hw_scan routine
is commented-out, so I figured you didn't want/need it anymore...?
John
--
John W. Linville
[email protected]
On Friday 27 April 2007 17:18, Michael Wu wrote:
> On Friday 27 April 2007 15:49, Michael Wu wrote:
> > On Friday 27 April 2007 13:40, Jiri Benc wrote:
> > > I agree.
> > >
> > > And I'd like to see some way to disable this feature for sane cards.
> >
> > Here you go.
> >
> > Thanks,
> > -Michael Wu
>
> And here's one that *really* works.
>
> Thanks,
> -Michael Wu
And this is the one that even looks good.
Thanks,
-Michael Wu
On Thu, 2007-04-26 at 20:23 -0400, Michael Wu wrote:
> On Thursday 26 April 2007 17:57, James Ketrenos wrote:
> > With a penalty to battery life and an increase in the amount of time a scan
> > takes. There are improvements that can be made to make the software
> > scanning faster, but at a penalty of added complexity on both the driver
> > and the stack side -- for no *real* gain for users that have cards that can
> > offload the scan.
> >
> Sure there is. Smaller firmware means firmware that's less likely to
> malfunction, which seems to be an issue. I don't see what you're saying about
> added complexity on the driver side. Eliminating the hw_scan callback reduces
> driver complexity. The increase in complexity on the stack side is offset by
> the fact that no driver/firmware/hardware needs to implement their own clever
> scanning algorithm.
If it's done right, it doesn't increase stack/driver complexity at all.
Obviously not everything can/should be done in software on the host CPU.
We all disliked Intel's binary regulatory daemon, and now that it's back
in firmware we're all happy. That's reality.
We shouldn't be ignoring discrete hardware functionality just because
it's in the firmware and also may be in the stack. Hardware crypto like
somebody mentioned. TCP offload. etc. Not allowing a driver writer to
take advantage of hardware functionality is quite short-sighted. Having
a pure host-CPU software stack is not utopia; it's entirely unrealistic
for a variety of reasons, some of which are below [1].
Obviously somebody needs to figure out why software scan is so slow.
Scan time matters; it should be as fast as possible without sacrificing
correctness or regulatory safety. But that doesn't mean dismissing
hardware scan just because it can also be done in the stack.
Let's be clear. Is mac80211 (a) _the_ Linux wireless stack that
non-fullmac vendors should target so Linux has a consistent wireless
story? Or is it (b) a pure software stack for only thin parts like
Atheros and everyone who doesn't have hardware exactly like Atheros can
just go away and play somewhere else? If it's (a), then we shouldn't be
saying no to James. If it's (b), then why aren't we doing (a) instead?
Dan
1) power-critical situations like embedded devices where some pieces
must be offloaded to the wireless part
2) lower-speed devices that may not have cycles to burn on functions
that the hardware can also do, even if most of the stack is software
3) timing critical functions
4) hybrid parts that are mostly softmac (ipw2100, ipw2200)
5) we don't make hardware
> > > the use of the hw_scan callback breaks the AP autoconfiguration code
> > > in ieee80211_sta.c due to its inadequate design.
> >
> > Is it breaking it just due to the auto-configuration code not knowing when
> > to configure things?
> >
> Yes, because ..
>
> > > Calling hw_scan starts a
> > > hardware scan, but there is no way to know when the scan is completed.
> >
> > That needs to be improved.
> >
> This is exactly the issue.
>
> > > Even if that problem were addressed, I still wouldn't like it as the
> > > design of the hw_scan callback is deficient in a number of other ways and
> > > is completely inapplicable to all other hardware/firmware softmac designs
> > > that I know.
> >
> > Given that there are things we can do as a result of the hw_scan that we
> > can't do on the host without imposing greater system load and slower scan
> > results, I really don't want to lose the ability to support hardware
> > offloading of scanning.
> >
> How fast is hardware scanning? What part of software scanning is slowing
> things down? What's the big bottleneck that justifies moving this to
> hardware? Are you sure it cannot be eliminated? Why haven't any other vendors
> chosen to offload scanning like this?
>
> > Could you voice some of the "other ways" the current hw_scan callback is
> > deficient?
> >
> Sure. In addition to not being able to find out when the scan is completed..
>
> - There is no way to specify what channels to scan.
> - There is no way to specify a passive scan.
> - The probe frame that is used is fixed with the exception of the SSID.
> - There is no corresponding interface for the userspace MLME.
>
> This isn't very important, however. I am more interested in what problems in
> software scanning need to be solved by moving to hardware scanning.
>
> > I'm fine with us overhauling how hw_scan works and integrates with the
> > stack, but an all out ban on hardware scan offload just doesn't make sense.
> > The host can do all RC4 and AES encrypt/decrypt too but certainly we would
> > prefer the hardware to do that for us, right?
> >
> The host can do TCP/IP too but certainly we would prefer the hardware to do
> that for us, right?
>
> Well, referring to TOE is unfair, but it makes the point that only some things
> make sense in hardware. Softmac is the result of removing the things that
> don't.
>
> > In the short term, I would rather we leave hw_scan in the code and have the
> > users that currently rely on hw_scan just have to manually configure the AP
> > selection until such time as the in-kernel-AP-selection-policy code works
> > with hw offloaded scan.
> >
> What incentive does that give to fix the code? Leaving broken things in for
> the sake of out-of-tree drivers does not appeal to me.
>
> -Michael Wu
On Fri, Apr 27, 2007 at 05:20:04PM +0200, Jiri Benc wrote:
> On Fri, 27 Apr 2007 10:56:33 -0400, Dan Williams wrote:
> > My point is that things are not black and white. It's not fullmac vs.
> > softmac, there's a spectrum of parts in between. I'm not advocating
> > making mac80211 work with every part under the sun, but it seems that
> > we're not being open enough to functionality that might be in hardware.
> > Holding a 100% software line with mac80211 is just IMHO wrong and
> > short-sighted. The stack needs to be flexible WRT to the hardware
> > capabilities of the parts that we expect to use it. Saying no to
> > hardware scanning just because it can also be done in software too is
> > wrong.
>
> Agreed.
Ditto.
> > We're not trying to limit the capabilities of hardware artificially, but
> > at the same time we're not going to allow crackpot hardware stuff to
> > clutter up the stack. Hardware scan is far from crackpot hardware
> > stuff; it's a simple, discrete function that shouldn't be hard work
> > with. It's already in, right? Ripping it out for a 100% software
> > agenda is wrong. Let's take out crypto offload then too if we're going
> > to take a consistently 100% software line. Again, it's not either
> > software or hardware; it's a spectrum of capabilities and we shouldn't
> > be making the stack _less_ flexible.
>
> The problem is that Michael during fixing of the stack for mainline
> inclusion encountered a problem with the current implementation of the
> hw scanning. We need to fix that problem.
>
> The proposed solution (removing of the hw_scan callback) obviously
> fixes the problem. Now, let's find something that fixes the problem as
> well but doesn't remove the functionality. James already proposed a
> solution that could work if a support for user space MLME is added. Do
> you have an idea how to add it?
I just want to chime-in here in concurrence with Dan and Jiri. I think
it makes sense to allow the stack to take advantage of reasonable
features offered by the hardware (or firmware). If supporting
such features requires the addition and support of reams of code or
awkward algorithms, then that support will be rejected or will at
least require strong justification backed by real numbers. If such
support requires minimal changes, then we will rely on our judgment
and whatever data is readily available -- the same as always.
I am definitely _not_ interested in conducting some witch hunt to
enforce a "software only" design purity. And it troubles me that
Intel seems to have been singled-out as some sort of bad actor in this
thread. I hope that I am misreading some of those remarks.
John
--
John W. Linville
[email protected]
On Mon, 23 Apr 2007 14:48:15 -0400, Michael Wu wrote:
> The master device cannot actually veto taking a device down. This patch
> makes all virtual devices go down if the master device goes down.
That fixes my objections for patch #4 :-)
Jiri
--
Jiri Benc
SUSE Labs
On Friday 27 April 2007 02:54, James Ketrenos wrote:
> Anyway, I ran a quick test of using hw_scan enabled and disabled on a
> system with 15 active and 17 passive channels.
>
> hw scan: 2.2s
> sw scan: 4.7s
>
> So the SW scan wasn't the 10s that I stated before (although it sure felt
> like 10s before I ran it with 'time') But anyway... the hw scan is
> currently beating out sw scanning by ~54%, at least here.
>
Ok. Let's try interpreting these numbers.
The current settings in mac80211 give each active scan channel 1/33rd of a
second to send a probe request and another 1/33rd of a second to listen for
any replies. That means, at the very minimum, an active scan will take a bit
over 60 ms to complete per channel. For passive scans, each channel gets
1/5th of a second, so that's 200 ms right there. With all this, we get:
.06 (s/channel) * 15 channels + .2 (s/channel) * 17 channels = 4.3 s
So, 91% of the time that software scan takes is just.. waiting. HW scan
simply "wins" by default because it doesn't wait as long. These numbers say
nothing about the actual overhead of using SW vs. HW scanning. Can you
provide more details on the delays that iwlwifi uses so we can do a proper
comparison?
-Michael Wu
On Fri, 27 Apr 2007 10:02:33 -0700, James Ketrenos wrote:
> Jiri Benc wrote:
> > The proposed solution (removing of the hw_scan callback) obviously
> > fixes the problem.
>
> No, it doesn't fix the problem--it removes the functionality. Deleting
> automatic AP selection "fixes the problem" too.
Yes. But if these two features conflict with each other and I have to
decide which one should be kept... well.. that's easy.
Of course I'd prefer a correct fix.
Btw, automatic AP selection works quite well for me (at least with
bcm43xx).
Jiri
--
Jiri Benc
SUSE Labs
Andy Green wrote:
> James Ketrenos wrote:
>> The more passive channels you add (most of 802.11a) the slower the
>> scanning gets.
>
> Well just a datapoint on that (zd1211rw, presumably mac80211 scanned)
>
> $ time iwlist mon0 scan
...
> real 0m0.857s
> user 0m0.001s
> sys 0m0.004s
How many channels are being scanned, and how many of those channels are passive only? And can you clarify on 'presumably'?
>>> Eliminating the hw_scan callback reduces driver complexity.
>> If done right, the stack would set up the list of channels to scan,
>> whether to scan the channel active or passive, and the template for the
>> probe request to use.
>
> I think one can reasonably say that's a lot of stuff to support what is
> a vendor-specific feature that only optimizes what can already be done.
I think I would say that's a lot of stuff to make software scanning try and work as well as hardware scanning does now. hw_scan() is there now, it works, and its twice as fast as software scanning. The work should be to make software scanning better for hardware that can't do hardware scan, not to remove the ability for hardware scanning.
...
> Maybe a simpler, more granular way to come at that is in the firmware to
> allow selection of "beacon only" filtering, or maybe a count of valid
> CRC packets that got filtered so you can assess if you should look
> closer at that channel.
Or maybe just let the firmware do the scan? :)
...
> For result latency, at the moment the monolithic scan is atomic, you sit
> there with nothing and get your results all at once. With a usermode
> driven scan, you will get results as you scan a channel, improving the
> perception of latency.
The action for finding your particular AP will always be atomic. If the AP you want is on a passive channel and you happen to miss the beacon in one pass, now the 4.7s "find the AP and associate via software" sequence is 9.4s... or 13.
There is no 'perception of latency' when it comes to the question of "How long after I click the button to turn on wireless do I have to wait before I can hit refresh on my browser?"
> It seems it won't make your life that much harder
Actually it will because we'll end up having to have our users use the external mac80211 subsystem for as long as hardware scanning is not supported by the kernel. Accelerated scanning is something end users like. I'm not going to give that up.
> mac80211-based scanning appears to work for you already
mac80211-based scanning worked as a stop gap. It is definitely not suitable as a final solution.
> and we talk about 2 seconds instead of 1 for results now and again.
It isn't 2s vs 1s -- we're talking 5s vs. 2s... and that's if it finds the AP on the first scan, which it doesn't always do. "now and again" is a lot more common than you'd like and when you're using a laptop and changing environments, roaming, AP hopping, etc.
James
On Thu, 26 Apr 2007 21:14:00 -0700, James Ketrenos wrote:
> Right now the scanning in mac80211 is slow. I haven't really dug
> into it to see if it will transition from passive to active if it
> detects RF activity on an otherwise passive channel, etc. but I have
> seen that if we turn on hw scanning we can get results back in a few
> seconds vs. 10 or more.
The scanning in mac80211 needs to be improved. I'd consider this as a
job for user space MLME, though.
> The more passive channels you add (most of 802.11a) the slower the
> scanning gets.
This seems to be a common, not a softmac specific problem. At least, if
done properly, I don't see how there could be a difference in softmac
vs. firmware scanning wrt. time.
> If done right, the stack would set up the list of channels to scan,
> whether to scan the channel active or passive, and the template for
> the probe request to use.
>
> For sw based scans, the stack would then have a mechanism for
> executing that scan through a series of tunes, transmits, etc. For
> hw scans, it would pass that structure to the driver which would
> package it for the hardware and pass it down.
>
> Upon completion of the scan (sw or hw) the notification would come
> back to the stack to let it know scanning is complete.
>
> The sw scan engine would be tied into the rest of the transmit
> infrastructure to manage off channel time, etc. (which you don't have
> to do with hw scanning since the hardware/uCode does it all for you)
This sounds reasonable and I think it addresses most of Michael's
objections.
Unfortunately, I don't see how user space MLME fits into this. Any idea
how to solve this remaining issue? (But no ugly hacks, please.)
> Unless there is a mechanism to quickly and easily toggle between
> filter and don't filter, you'll end up turning off hw/uCode
> filtering of packets all the time.
I'm not sure I understand. What prevents you from turning off the
filtering just during the scanning?
> Which means the hardware would be set to full promiscuous mode and
> every packet Rx'd would get tossed to the host to then either process
> or discard--which keeps the host CPU awake (which isn't good on
> laptops or anywhere else folks care about power consumption) And
> scanning is one of those things that happens more frequently when you
> are not plugged in and are moving around the home, school, office,
> etc.
Yes, power consumption is a good argument. However, how much time you
spend in scanning (compared to the total time you use your wifi card)?
It would be helpful to see some numbers.
> > What's the big bottleneck that justifies moving this to
> > hardware?
>
> With the 3945 its mainly power consumption.
Do you have some numbers? Personally, I have no idea how much power you
can save by offloading the scan. It could be interesting.
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
On Monday 23 April 2007 16:58, Andy Green wrote:
> Couldn't get this to apply on current wireless-dev, and there is a
> trailing whitespace. Maybe it's a problem with a script I concocted to
> extract the patch from saved Thunderbird 2.0 mails... ? Is it just that
> wireless-dev changed inbetweentimes?
>
This patch series depends on the recent cfg80211/mac80211 rtnl locking
patches.
-Michael Wu
Patches 1, 3-8, 10-13 applied to my tree.
Thanks!
Jiri
--
Jiri Benc
SUSE Labs
On Wednesday 25 April 2007 14:16, John W. Linville wrote:
> On Tue, Apr 24, 2007 at 10:03:03PM -0700, James Ketrenos wrote:
> > (I would like to see hw_scan remain since iwlwifi uses it -- which we
> > hope to submit as soon as the code restructuring is complete)
>
> I was going to mention this in response to Michael's patch, but then
> I checked your current iwlwifi git tree. There your hw_scan routine
> is commented-out, so I figured you didn't want/need it anymore...?
>
It was never needed in the first place except as an iwlwifi specific hack to
minimize any disruption to the current connection. (which often seems to be
commented out due to firmware issues) mac80211 now does its own PS mode
scanning trick along with some frame queuing as a generic means to minimize
disruptions to the current connection during scans. It still needs some
improvements in order to quickly empty the current hardware TX queue before
commencing a scan and possibly time/segment the scanning such that beacons
are not lost, but it can certainly be done.
Besides the fact that what hw_scan does can be done by mac80211 for all
drivers, the use of the hw_scan callback breaks the AP autoconfiguration code
in ieee80211_sta.c due to its inadequate design. Calling hw_scan starts a
hardware scan, but there is no way to know when the scan is completed. This
means the code has no way of knowing when it should look through its scan
results and configure itself to connect to an AP if a scan was started by
mac80211 for the purpose of finding an AP to connect to.
Even if that problem were addressed, I still wouldn't like it as the design of
the hw_scan callback is deficient in a number of other ways and is completely
inapplicable to all other hardware/firmware softmac designs that I know.
-Michael Wu
Michael,
1) Saving 18 seconds of busy system will provide you much more time of
idle system. So the benefit is much more than 18 seconds per hour. The
calculation that you gave below is just incorrect.
2) CPU utilization/Power is a big deal for handheld devices. We can't
ignore it. Linux is gaining momentum in small factor devices and we
need to welcome any HW acceleration. The point that it is implemented
in FW doesn't change the fact that this is a HW offloading from the
host.
3) Sending Tune command for every channel from the stack when a HW is
capable of doing so, doesn't make sense. The latency and overhead is
redundant and will impact user experience (e.g. a [longer] glitch in
Video streaming, shorter battery life).
Guy.
On 4/27/07, Michael Wu <[email protected]> wrote:
> Ok. Since you did not provide any numbers to back up that assertion, let's try
> to find the upper bound for the cost of using software scanning.
>
> With NetworkManager, userspace requests a scan about every 2 minutes when
> connected. Assume that each scan takes 3 seconds. Assume that things are
> ridiculously inefficient and software scanning keeps the cpu on for an
> additional 20% of the scanning time. This keeps the CPU on unnecessarily for
> 18 seconds every hour, assuming nothing else is running. With an additional
> assumption that all the additional CPU time that goes into software scan
> directly subtracts from the battery life, this results in a loss of about 2
> minutes and a half of battery life.. for a laptop that can last 8 hours.
>
> An upper bound of 18 seconds of battery life lost per hour sounds okay to me
> especially when it assumes the user is just letting the laptop idle. In
> practice, I think that the difference is going to be negligible.. but what I
> think is going to happen and my guess at the upper bound doesn't matter much.
> If you can provide numbers, this discussion becomes significantly more
> productive.
>
> -Michael Wu
>
>
> This prevents userspace and the in-kernel MLME from configuring
> channel/BSSID/SSID at the same time when the in-kernel MLME is in the midst
> of automatic AP selection. This is done by holding the RTNL lock.
Once cfg80211 drops the rtnl we'll have to find a better way of doing
this, for now I agree. Maybe in cfg80211 we'll have to allow drivers to
access the per-cfg80211-instance mutex allocated for them.
johannes
Johannes Berg wrote:
> The diff to rip out the hw_scan callback is tiny, and since it'd need to
> be worked on anyway to add notification when scanning is done,
> IMHO it is little more work to remove the scan now and add it back in one patch
> including the notification.
The driver calls ieee80211_scan_completed(), which is in stack and works so let's not rip out hw_scan.
To be fair, I still don't know what was being fixed by taking hw_scan out... auto-AP selection has never worked here whether I use hw_scan or mac80211's sw scan; I have to configure everything from userspace including channel, essid, channel, and bssid (essid might be optional; I haven't tried not including it)
James
From: Michael Wu <[email protected]>
This patch sets the BSSID to broadcast before scanning to ensure that the
hardware filter does not filter probe responses.
Signed-off-by: Michael Wu <[email protected]>
---
net/mac80211/ieee80211.c | 6 +++++-
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/ieee80211_sta.c | 10 ++++++++++
3 files changed, 16 insertions(+), 1 deletions(-)
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index ddb8153..17c6fde 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -2023,7 +2023,11 @@ static int __ieee80211_if_config(struct net_device *dev,
conf.type = sdata->type;
if (sdata->type == IEEE80211_IF_TYPE_STA ||
sdata->type == IEEE80211_IF_TYPE_IBSS) {
- conf.bssid = sdata->u.sta.bssid;
+ if (local->sta_scanning &&
+ local->scan_dev == dev)
+ conf.bssid = local->scan_bssid;
+ else
+ conf.bssid = sdata->u.sta.bssid;
conf.ssid = sdata->u.sta.ssid;
conf.ssid_len = sdata->u.sta.ssid_len;
conf.generic_elem = sdata->u.sta.extra_ie;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 6b3e1c5..fe638a9 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -502,6 +502,7 @@ struct ieee80211_local {
struct ieee80211_hw_mode *oper_hw_mode, *scan_hw_mode;
u8 scan_ssid[IEEE80211_MAX_SSID_LEN];
size_t scan_ssid_len;
+ u8 scan_bssid[ETH_ALEN];
struct list_head sta_bss_list;
struct ieee80211_sta_bss *sta_bss_hash[STA_HASH_SIZE];
spinlock_t sta_bss_lock;
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 7cb2dd3..20bd77b 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -2577,6 +2577,10 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
printk(KERN_DEBUG "%s: failed to restore operational"
"channel after scan\n", dev->name);
+ if (ieee80211_if_config(dev))
+ printk(KERN_DEBUG "%s: failed to restore operational"
+ "BSSID after scan\n", dev->name);
+
memset(&wrqu, 0, sizeof(wrqu));
wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL);
@@ -2730,12 +2734,18 @@ static int ieee80211_sta_start_scan(struct net_device *dev,
memcpy(local->scan_ssid, ssid, ssid_len);
} else
local->scan_ssid_len = 0;
+ memset(local->scan_bssid, ~0, ETH_ALEN);
local->scan_state = SCAN_SET_CHANNEL;
local->scan_hw_mode = list_entry(local->modes_list.next,
struct ieee80211_hw_mode,
list);
local->scan_channel_idx = 0;
local->scan_dev = dev;
+
+ if (ieee80211_if_config(dev))
+ printk(KERN_DEBUG "%s: failed to set BSSID for scan\n",
+ dev->name);
+
/* TODO: start scan as soon as all nullfunc frames are ACKed */
queue_delayed_work(local->hw.workqueue, &local->scan_work,
IEEE80211_CHANNEL_TIME);
On Monday 23 April 2007 16:41, Jiri Benc wrote:
> > + netif_tx_lock(local->mdev);
>
> Shouldn't it be netif_tx_lock_bh?
>
Softirqs should already be disabled by the caller.
> > if (((dev->flags & IFF_ALLMULTI) != 0) ^ (sdata->allmulti != 0)) {
> > if (sdata->allmulti) {
> > sdata->allmulti = 0;
> > @@ -2177,9 +2178,12 @@ static void ieee80211_set_multicast_list(struct
> > net_device *dev) flags |= IFF_ALLMULTI;
> > if (local->iff_promiscs)
> > flags |= IFF_PROMISC;
> > + read_lock(&local->sub_if_lock);
> > local->ops->set_multicast_list(local_to_hw(local), flags,
> > local->mc_count);
>
> Would be nice to have documented that set_multicast_list is called in
> atomic context.
>
Sure, but this is no different from other network drivers.
> > [...]
> > @@ -3871,6 +3884,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct
> > sk_buff *skb, struct sk_buff *skb_new;
> > u8 *bssid = ieee80211_get_bssid(hdr, skb->len - radiotap_len);
> >
> > + read_lock(&local->sub_if_lock);
> > list_for_each_entry(sdata, &local->sub_if_list, list) {
> > rx.u.rx.ra_match = 1;
> > switch (sdata->type) {
> > @@ -3906,10 +3920,9 @@ void __ieee80211_rx(struct ieee80211_hw *hw,
> > struct sk_buff *skb, rx.u.rx.ra_match = 0;
> > } else if (!sta)
> > sta = rx.sta =
> > - ieee80211_ibss_add_sta(local->mdev,
> > + ieee80211_ibss_add_sta(sdata->dev,
> > skb, bssid,
> > hdr->addr2);
> > - /* FIXME: call with sdata->dev */
> > break;
> > case IEEE80211_IF_TYPE_AP:
> > if (!bssid) {
> > @@ -3964,6 +3977,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct
> > sk_buff *skb, &rx, sta);
> > } else
> > dev_kfree_skb(skb);
> > + read_unlock(&local->sub_if_lock);
>
> Documenting that sub_if_lock cannot be taken under sta_bss_lock nor
> under sta_lock would be nice.
>
> local->ops->conf_tx and ->set_time are called under spinlock, are we ok
> with that?
>
Since conf_tx is being called from ieee80211_sta.c, I'd rather have it run
from the workqueue. As far set_time.. would be nice to call it outside of the
tasklet but that can be addressed in another patch. Calling it under readlock
is okay since writes are so rare and the callback isn't allowed to sleep
anyway.
> > return res;
> > case HOSTAP_IF_VLAN:
> > if (left < sizeof(struct hostapd_if_vlan))
> > return -EPROTO;
> >
> > - res = ieee80211_if_add(dev, param->u.if_info.name, 0, &new_dev);
> > + res = ieee80211_if_add(dev, param->u.if_info.name, NULL,
> > + IEEE80211_IF_TYPE_VLAN);
> > if (res)
> > return res;
> > - ieee80211_if_set_type(new_dev, IEEE80211_IF_TYPE_VLAN);
> > #if 0
> > res = ieee80211_if_update_vlan(new_dev, vlan->id);
> > if (res)
>
> I know that the next few lines are commented out but please fix them as
> well or remove the #ifed out code completely, otherwise we may be in
> trouble later.
>
It doesn't even compile now since ieee80211_if_update_vlan doesn't exist
anymore.
> > [...]
> > @@ -1093,36 +1098,34 @@ static int ieee80211_ioctl_update_if(struct
> > net_device *dev, struct ieee80211_local *local =
> > wdev_priv(dev->ieee80211_ptr); struct net_device *wds_dev = NULL;
> > struct ieee80211_sub_if_data *sdata;
> > + int ret;
> >
> > if (left < sizeof(struct ieee80211_if_wds))
> > return -EPROTO;
> >
> > + read_lock(&local->sub_if_lock);
> > list_for_each_entry(sdata, &local->sub_if_list, list) {
> > if (strcmp(param->u.if_info.name,
> > sdata->dev->name) == 0) {
> > wds_dev = sdata->dev;
> > + dev_hold(wds_dev);
> > break;
> > }
> > }
> > + read_unlock(&local->sub_if_lock);
> >
> > if (!wds_dev || sdata->type != IEEE80211_IF_TYPE_WDS)
> > return -ENODEV;
> >
> > - return ieee80211_if_update_wds(wds_dev, wds->remote_addr);
> > + ret = ieee80211_if_update_wds(wds_dev, wds->remote_addr);
> > + dev_put(wds_dev);
>
> This was the place with unnecessary dev_hold/dev_put you talked about,
> right? Just wanted to show I'm paying attention :-)
>
Yup.
> > [...]
> > @@ -2239,6 +2239,7 @@ static int ieee80211_ioctl_clear_keys(struct
> > net_device *dev) struct sta_info *sta;
> >
> > memset(addr, 0xff, ETH_ALEN);
> > + read_lock(&local->sub_if_lock);
> > list_for_each_entry(sdata, &local->sub_if_list, list) {
> > for (i = 0; i < NUM_DEFAULT_KEYS; i++) {
> > keyconf = NULL;
> > @@ -2256,6 +2257,7 @@ static int ieee80211_ioctl_clear_keys(struct
> > net_device *dev) }
> > sdata->default_key = NULL;
> > }
> > + read_unlock(&local->sub_if_lock);
>
> Again, local->ops->set_key called under spinlock.
>
The good thing about this rwlock is that writers are extremely rare. (and not
going to happen in this case since we're holding rtnl which is a prerequisite
to adding/removing virtual interfaces)
> Recursively taken lock. Ok, it's rwlock locked for reading, but I'd
> still prefer not doing that.
>
Opps. Good catch.
I'll post a new patch in an hour or so.
Thanks,
-Michael Wu
Michael Wu wrote:
Hi Michael -
Couldn't get this to apply on current wireless-dev, and there is a
trailing whitespace. Maybe it's a problem with a script I concocted to
extract the patch from saved Thunderbird 2.0 mails... ? Is it just that
wireless-dev changed inbetweentimes?
$ git pull
Already up-to-date.
$ guilt push
Applying patch..mac80211-Add-radiotap-support.patch
Patch applied.
$ guilt push
Applying patch..sync-with-radiotap-header-in-wireless-2.6.patch
Patch applied.
$ guilt push
Applying patch..mac80211-fix-virtual-interface-related-locking.patch
Adds trailing whitespace.
Context reduced to (1/1) to apply fragment at 30
error: patch failed: net/mac80211/ieee80211_cfg.c:45
error: net/mac80211/ieee80211_cfg.c: patch does not apply
To force apply this patch, use 'guilt push -f'
$
Failing chunk:
> diff --git a/net/mac80211/ieee80211_cfg.c b/net/mac80211/ieee80211_cfg.c
> index e370b4b..0069826 100644
> --- a/net/mac80211/ieee80211_cfg.c
> +++ b/net/mac80211/ieee80211_cfg.c
...
> @@ -45,16 +44,12 @@ static int ieee80211_add_iface(struct wiphy *wiphy, char *name,
> return -EINVAL;
> }
>
> - res = ieee80211_if_add(local->mdev, name, 0, &new_dev);
> - if (res == 0)
> - ieee80211_if_set_type(new_dev, itype);
> - return res;
> + return ieee80211_if_add(local->mdev, name, NULL, itype);
> }
...
> diff --git a/net/mac80211/ieee80211_ioctl.c b/net/mac80211/ieee80211_ioctl.c
> index 2ff762d..502010e 100644
> --- a/net/mac80211/ieee80211_ioctl.c
> +++ b/net/mac80211/ieee80211_ioctl.c
> @@ -1003,23 +1003,30 @@ static int ieee80211_ioctl_add_if(struct net_device *dev,
> if (left < sizeof(struct hostapd_if_wds))
> return -EPROTO;
>
> - res = ieee80211_if_add(dev, param->u.if_info.name, 0, &new_dev);
> + res = ieee80211_if_add(dev, param->u.if_info.name, &new_dev,
> + IEEE80211_IF_TYPE_WDS);
> if (res)
> return res;
> - ieee80211_if_set_type(new_dev, IEEE80211_IF_TYPE_WDS);
> res = ieee80211_if_update_wds(new_dev, wds->remote_addr);
> - if (res)
> - __ieee80211_if_del(wdev_priv(dev->ieee80211_ptr),
> - IEEE80211_DEV_TO_SUB_IF(new_dev));
> + if (unlikely(res)) {
> + struct ieee80211_local *local =
> + wdev_priv(dev->ieee80211_ptr);
> + struct ieee80211_sub_if_data *sdata = <<<<<<----- trailing space
-Andy
On Fri, 2007-04-27 at 17:22 +0200, Johannes Berg wrote:
> On Fri, 2007-04-27 at 16:16 +0100, Andy Green wrote:
>
> > Yeah but this isn't "in hardware" -- it's in firmware: software that
> > runs of a different, vendor-specific, CPU.
>
> [...]
>
> Guys, this discussion is deteriorating rapidly!
>
> The diff to rip out the hw_scan callback is tiny, and since it'd need to
> be worked on anyway to add notification when scanning is done, IMHO it
> is little more work to remove the scan now and add it back in one patch
> including the notification. That just pushes the problem back to those
> people who want/need it and then they can decide if it's worth the
> effort and if they can show that it brings enough advantages.
This discussion has made it pretty clear that if hw_scan ever goes out,
it's going to be really difficult to get it back _in_ at all, even if it
works.
Dan
> The current cfg80211 scanning API is pretty much modeled on the 802.11
> MLME interface and as such should be easy to push through to the
> firmware if necessary, even through mac80211.
>
> johannes
Jiri Benc wrote:
> On Fri, 27 Apr 2007 16:30:02 +0100, Andy Green wrote:
>>> fixes the problem. Now, let's find something that fixes the problem as
>>> well but doesn't remove the functionality. James already proposed a
>>> solution that could work if a support for user space MLME is added. Do
>>> you have an idea how to add it?
>> Jiri, what functionality? The ability to change frequency periodically?
>> Isn't that something mac80211 can do already?
>
> Please see other mails in this thread. The opinions differ.
>
> As long as it is implemented cleanly I have no objections against it.
>
> Btw, the main argument for hw_scan was power consumption. I'd still
> like to see numbers :-)
No, my main argument for hw_scan was latency to associate.
I also think power consumption and overall system overhead is important; opinions vary on this (I think even the 18 seconds per hour that Michael asserts is worth it given that it doesn't impose any negative impact the end users flexibility)
If mac80211 can get scanning to consistently work as fast as hardware scanning, and can show that it doesn't negatively impact battery life, CPU load, or throughput (esp. while associated)--great! Once that goal is reached then I can't think of a reason why we would want hardware scan offloading.
I don't have numbers to show you how much power a scan costs when done in SW vs. HW. I do have numbers that show that if I don't use hw_scan, the scan takes more than twice as long. A code profiler can show that there is a lot more processing going on in the host during a software scan than a hardware scan.
There are also features enabled through Intel's wireless products that mac80211 currently doesn't support that you just can't do as efficiently in a 100% host based softmac solution. I would love to enable those as well. One example is concurrent scanning for multiple SSIDs in a single scan without having to send multiple probe requests from the [userspace->]mac80211->driver->hardware.
James
From: Michael Wu <[email protected]>
This eliminates some unnecessary code in ieee80211_sta.c.
Signed-off-by: Michael Wu <[email protected]>
---
net/mac80211/ieee80211_sta.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 04a63a7..fe5c762 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -1926,7 +1926,6 @@ void ieee80211_sta_work(struct work_struct *work)
if (local->sta_scanning)
return;
- sdata = IEEE80211_DEV_TO_SUB_IF(dev);
if (sdata->type != IEEE80211_IF_TYPE_STA &&
sdata->type != IEEE80211_IF_TYPE_IBSS) {
printk(KERN_DEBUG "%s: ieee80211_sta_work: non-STA interface "
@@ -2121,7 +2120,7 @@ static int ieee80211_sta_config_auth(struct net_device *dev,
return 0;
} else {
if (ifsta->state != IEEE80211_AUTHENTICATE) {
- ieee80211_sta_start_scan(dev, NULL, 0);;
+ ieee80211_sta_start_scan(dev, NULL, 0);
ifsta->state = IEEE80211_AUTHENTICATE;
set_bit(IEEE80211_STA_REQ_AUTH, &ifsta->request);
} else
From: Michael Wu <[email protected]>
The master device cannot actually veto taking a device down. This patch
makes all virtual devices go down if the master device goes down.
Signed-off-by: Michael Wu <[email protected]>
---
net/mac80211/ieee80211.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index e876e04..6002f2d 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -2293,13 +2293,11 @@ static int ieee80211_master_stop(struct net_device *dev)
struct ieee80211_sub_if_data *sdata;
read_lock(&local->sub_if_lock);
- list_for_each_entry(sdata, &local->sub_if_list, list) {
- if (sdata->dev != dev && netif_running(sdata->dev)) {
- read_unlock(&local->sub_if_lock);
- return -EOPNOTSUPP;
- }
- }
+ list_for_each_entry(sdata, &local->sub_if_list, list)
+ if (sdata->dev != dev && netif_running(sdata->dev))
+ dev_close(sdata->dev);
read_unlock(&local->sub_if_lock);
+
return 0;
}
@@ -2475,7 +2473,8 @@ static int ieee80211_stop(struct net_device *dev)
local->open_count--;
if (local->open_count == 0) {
ieee80211_stop_scan(local);
- dev_close(local->mdev);
+ if (netif_running(local->mdev))
+ dev_close(local->mdev);
if (local->apdev)
dev_close(local->apdev);
if (local->ops->stop)
From: Michael Wu <[email protected]>
This patch ensures that the master device cannot be opened without the
ieee80211 qdisc installed.
Signed-off-by: Michael Wu <[email protected]>
---
net/mac80211/ieee80211.c | 6 ++++++
net/mac80211/wme.c | 6 ++++++
net/mac80211/wme.h | 1 +
3 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 17c6fde..e876e04 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -2270,6 +2270,12 @@ static int ieee80211_master_open(struct net_device *dev)
struct ieee80211_sub_if_data *sdata;
int res = -EOPNOTSUPP;
+ if (!ieee80211_qdisc_installed(dev)) {
+ printk(KERN_ERR "%s: ieee80211 qdisc not installed\n",
+ dev->name);
+ return res;
+ }
+
read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
if (sdata->dev != dev && netif_running(sdata->dev)) {
diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index 79b4305..1cb6358 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -655,6 +655,12 @@ void ieee80211_install_qdisc(struct net_device *dev)
}
+int ieee80211_qdisc_installed(struct net_device *dev)
+{
+ return dev->qdisc_sleeping->ops == &wme_qdisc_ops;
+}
+
+
int ieee80211_wme_register(void)
{
int err = 0;
diff --git a/net/mac80211/wme.h b/net/mac80211/wme.h
index 90add6e..2a79262 100644
--- a/net/mac80211/wme.h
+++ b/net/mac80211/wme.h
@@ -31,6 +31,7 @@ ieee80211_txrx_result
ieee80211_rx_h_remove_qos_control(struct ieee80211_txrx_data *rx);
void ieee80211_install_qdisc(struct net_device *dev);
+int ieee80211_qdisc_installed(struct net_device *dev);
int ieee80211_wme_register(void);
void ieee80211_wme_unregister(void);
On Tue, Apr 24, 2007 at 02:24:56PM -0400, Michael Wu wrote:
> On Tuesday 24 April 2007 14:09, Andy Green wrote:
> > =============================================
> > [ INFO: possible recursive locking detected ]
> > 2.6.21-rc7 #11
> > ---------------------------------------------
> > ip/2600 is trying to acquire lock:
> > (&dev->_xmit_lock){-+..}, at: [<e09199c5>]
> > ieee80211_set_multicast_list+0x46/0x167 [mac80211]
> >
> > but task is already holding lock:
> > (&dev->_xmit_lock){-+..}, at: [<c05abc05>] dev_mc_upload+0x14/0x3c
> >
> This is fine, though it would be nice if we can shut the warning off. Virtual
> interfaces can try to take the xmit lock of the master device, but the master
> device will never try to take the xmit lock of the virtual interfaces and
> especially not while holding its own xmit lock.
Hmmm...there is probably some lockdep annotation foo to take care of
this, but I don't know it.
Anyone?
John
--
John W. Linville
[email protected]
On Tue, 24 Apr 2007 18:24:11 +0200, Johannes Berg wrote:
> On Mon, 2007-04-23 at 14:48 -0400, Michael Wu wrote:
>
> > --- a/net/mac80211/ieee80211_i.h
> > +++ b/net/mac80211/ieee80211_i.h
> > @@ -502,6 +502,7 @@ struct ieee80211_local {
> > struct ieee80211_hw_mode *oper_hw_mode, *scan_hw_mode;
> > u8 scan_ssid[IEEE80211_MAX_SSID_LEN];
> > size_t scan_ssid_len;
> > + u8 scan_bssid[ETH_ALEN];
> > struct list_head sta_bss_list;
> > struct ieee80211_sta_bss *sta_bss_hash[STA_HASH_SIZE];
> > spinlock_t sta_bss_lock;
>
> Do you see any use for setting scan_bssid to anything other than the
> broadcast address? It doesn't seem useful to add this field here at this
> time.
I agree.
And I'd like to see some way to disable this feature for sane cards.
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
From: Michael Wu <[email protected]>
This patch fixes two issues found by Patrick McHardy:
1. wme_qdiscop_enqueue doesn't increment q.qlen for packets queued to
q->requeued[], which might cause upper layer code to stop dequeueing if
q.qlen reaches zero.
2. wme_discop_destroy leaks classifier module references and memory when
destroying classifiers, it should use tcf_destroy()
It also removes some dead code.
Signed-off-by: Michael Wu <[email protected]>
---
net/mac80211/wme.c | 13 +++++--------
1 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index 243da1f..79b4305 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -220,6 +220,7 @@ static int wme_qdiscop_enqueue(struct sk_buff *skb, struct Qdisc* qd)
if (pkt_data->requeue) {
skb_queue_tail(&q->requeued[pkt_data->queue], skb);
+ qd->q.qlen++;
return 0;
}
@@ -313,8 +314,10 @@ static struct sk_buff *wme_qdiscop_dequeue(struct Qdisc* qd)
/* there is space - try and get a frame */
skb = skb_dequeue(&q->requeued[queue]);
- if (skb)
+ if (skb) {
+ qd->q.qlen--;
return skb;
+ }
qdisc = q->queues[queue];
skb = qdisc->dequeue(qdisc);
@@ -356,7 +359,7 @@ static void wme_qdiscop_destroy(struct Qdisc* qd)
while ((tp = q->filter_list) != NULL) {
q->filter_list = tp->next;
- tp->ops->destroy(tp);
+ tcf_destroy(tp);
}
for (queue=0; queue < hw->queues; queue++) {
@@ -503,7 +506,6 @@ static unsigned long wme_classop_bind(struct Qdisc *qd, unsigned long parent,
static void wme_classop_put(struct Qdisc *q, unsigned long cl)
{
- /* printk(KERN_DEBUG "entering %s\n", __func__); */
}
@@ -513,7 +515,6 @@ static int wme_classop_change(struct Qdisc *qd, u32 handle, u32 parent,
unsigned long cl = *arg;
struct ieee80211_local *local = wdev_priv(qd->dev->ieee80211_ptr);
struct ieee80211_hw *hw = &local->hw;
- /* printk(KERN_DEBUG "entering %s\n", __func__); */
if (cl - 1 > hw->queues)
return -ENOENT;
@@ -531,7 +532,6 @@ static int wme_classop_delete(struct Qdisc *qd, unsigned long cl)
{
struct ieee80211_local *local = wdev_priv(qd->dev->ieee80211_ptr);
struct ieee80211_hw *hw = &local->hw;
- /* printk(KERN_DEBUG "entering %s\n", __func__); */
if (cl - 1 > hw->queues)
return -ENOENT;
@@ -545,7 +545,6 @@ static int wme_classop_dump_class(struct Qdisc *qd, unsigned long cl,
struct ieee80211_sched_data *q = qdisc_priv(qd);
struct ieee80211_local *local = wdev_priv(qd->dev->ieee80211_ptr);
struct ieee80211_hw *hw = &local->hw;
- /* printk(KERN_DEBUG "entering %s\n", __func__); */
if (cl - 1 > hw->queues)
return -ENOENT;
@@ -561,7 +560,6 @@ static void wme_classop_walk(struct Qdisc *qd, struct qdisc_walker *arg)
struct ieee80211_local *local = wdev_priv(qd->dev->ieee80211_ptr);
struct ieee80211_hw *hw = &local->hw;
int queue;
- /* printk(KERN_DEBUG "entering %s\n", __func__); */
if (arg->stop)
return;
@@ -586,7 +584,6 @@ static struct tcf_proto ** wme_classop_find_tcf(struct Qdisc *qd,
unsigned long cl)
{
struct ieee80211_sched_data *q = qdisc_priv(qd);
- /* printk("entering %s\n", __func__); */
if (cl)
return NULL;
On Friday 27 April 2007 15:49, Michael Wu wrote:
> On Friday 27 April 2007 13:40, Jiri Benc wrote:
> > I agree.
> >
> > And I'd like to see some way to disable this feature for sane cards.
>
> Here you go.
>
> Thanks,
> -Michael Wu
And here's one that *really* works.
Thanks,
-Michael Wu
> -#define RX_FLAG_MMIC_ERROR 0x1
> -#define RX_FLAG_DECRYPTED 0x2
> -
> /* Receive status. The low-level driver should provide this information
> * (the subset supported by hardware) to the 802.11 code with each received
> * frame. */
> @@ -232,6 +229,9 @@ struct ieee80211_rx_status {
> int noise;
> int antenna;
> int rate;
> +#define RX_FLAG_MMIC_ERROR (1<<0)
> +#define RX_FLAG_DECRYPTED (1<<1)
> +#define RX_FLAG_RADIOTAP (1<<2)
> int flag;
> };
Doesn't matter right now, but for the future: kernel-doc hates having
constants inlined into a struct like that.
johannes
On Fri, 27 Apr 2007 15:42:55 -0400, Dan Williams wrote:
> I don't see how they could possibly conflict methodically. They are two
> paths to the same result. Or do you mean currently conflict technically
> in the code?
The latter.
Jiri
--
Jiri Benc
SUSE Labs
From: Michael Wu <[email protected]>
Statistics were never updated properly for the master device, so don't
bother reporting them.
Signed-off-by: Michael Wu <[email protected]>
---
net/mac80211/ieee80211.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 7743ca0..8a3c749 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -4694,7 +4694,6 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
sta_info_init(local);
mdev->hard_start_xmit = ieee80211_master_start_xmit;
- mdev->get_stats = ieee80211_get_stats;
mdev->open = ieee80211_master_open;
mdev->stop = ieee80211_master_stop;
mdev->type = ARPHRD_IEEE80211;
From: Michael Wu <[email protected]>
This converts sub_if_lock to a rw lock and makes all code touching
sub_if_list use it, grabs mdev's tx lock in set_multicast_list to
synchronize multicast configuration, and simplifies some related code.
Signed-off-by: Michael Wu <[email protected]>
---
net/mac80211/ieee80211.c | 52 +++++++++++++++++++---------
net/mac80211/ieee80211_cfg.c | 15 +++-----
net/mac80211/ieee80211_i.h | 7 +---
net/mac80211/ieee80211_iface.c | 75 +++++++++++++++++-----------------------
net/mac80211/ieee80211_ioctl.c | 62 ++++++++++++++++++---------------
net/mac80211/ieee80211_sta.c | 24 +++----------
6 files changed, 113 insertions(+), 122 deletions(-)
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 51ad624..bc89ac9 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -953,7 +953,7 @@ static void purge_old_ps_buffers(struct ieee80211_local *local)
struct ieee80211_sub_if_data *sdata;
struct sta_info *sta;
- spin_lock_bh(&local->sub_if_lock);
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
struct ieee80211_if_ap *ap;
if (sdata->dev == local->mdev ||
@@ -967,7 +967,7 @@ static void purge_old_ps_buffers(struct ieee80211_local *local)
}
total += skb_queue_len(&ap->ps_bc_buf);
}
- spin_unlock_bh(&local->sub_if_lock);
+ read_unlock(&local->sub_if_lock);
spin_lock_bh(&local->sta_lock);
list_for_each_entry(sta, &local->sta_list, list) {
@@ -2148,6 +2148,7 @@ static void ieee80211_set_multicast_list(struct net_device *dev)
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
unsigned short flags;
+ netif_tx_lock(local->mdev);
if (((dev->flags & IFF_ALLMULTI) != 0) ^ (sdata->allmulti != 0)) {
if (sdata->allmulti) {
sdata->allmulti = 0;
@@ -2177,9 +2178,12 @@ static void ieee80211_set_multicast_list(struct net_device *dev)
flags |= IFF_ALLMULTI;
if (local->iff_promiscs)
flags |= IFF_PROMISC;
+ read_lock(&local->sub_if_lock);
local->ops->set_multicast_list(local_to_hw(local), flags,
local->mc_count);
+ read_unlock(&local->sub_if_lock);
}
+ netif_tx_unlock(local->mdev);
}
struct dev_mc_list *ieee80211_get_mc_list_item(struct ieee80211_hw *hw,
@@ -2220,12 +2224,11 @@ static struct net_device_stats *ieee80211_get_stats(struct net_device *dev)
return &(sdata->stats);
}
-void ieee80211_if_shutdown(struct net_device *dev)
+static void ieee80211_if_shutdown(struct net_device *dev)
{
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
- ASSERT_RTNL();
switch (sdata->type) {
case IEEE80211_IF_TYPE_STA:
case IEEE80211_IF_TYPE_IBSS:
@@ -2269,6 +2272,7 @@ static int ieee80211_master_open(struct net_device *dev)
struct ieee80211_sub_if_data *sdata;
int res = -EOPNOTSUPP;
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
if (sdata->dev != dev && netif_running(sdata->dev)) {
res = 0;
@@ -2276,6 +2280,7 @@ static int ieee80211_master_open(struct net_device *dev)
break;
}
}
+ read_unlock(&local->sub_if_lock);
return res;
}
@@ -2285,10 +2290,14 @@ static int ieee80211_master_stop(struct net_device *dev)
struct ieee80211_sub_if_data *sdata;
tasklet_disable(&local->tx_pending_tasklet);
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
- if (sdata->dev != dev && netif_running(sdata->dev))
+ if (sdata->dev != dev && netif_running(sdata->dev)) {
+ read_unlock(&local->sub_if_lock);
return -EOPNOTSUPP;
+ }
}
+ read_unlock(&local->sub_if_lock);
return 0;
}
@@ -2346,14 +2355,18 @@ static int ieee80211_open(struct net_device *dev)
int res;
sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ read_lock(&local->sub_if_lock);
list_for_each_entry(nsdata, &local->sub_if_list, list) {
struct net_device *ndev = nsdata->dev;
if (ndev != dev && ndev != local->mdev && netif_running(ndev) &&
compare_ether_addr(dev->dev_addr, ndev->dev_addr) == 0 &&
- !identical_mac_addr_allowed(sdata->type, nsdata->type))
+ !identical_mac_addr_allowed(sdata->type, nsdata->type)) {
+ read_unlock(&local->sub_if_lock);
return -ENOTUNIQ;
+ }
}
+ read_unlock(&local->sub_if_lock);
if (sdata->type == IEEE80211_IF_TYPE_WDS &&
is_zero_ether_addr(sdata->u.wds.remote_addr))
@@ -3871,6 +3884,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
struct sk_buff *skb_new;
u8 *bssid = ieee80211_get_bssid(hdr, skb->len - radiotap_len);
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
rx.u.rx.ra_match = 1;
switch (sdata->type) {
@@ -3906,10 +3920,9 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
rx.u.rx.ra_match = 0;
} else if (!sta)
sta = rx.sta =
- ieee80211_ibss_add_sta(local->mdev,
+ ieee80211_ibss_add_sta(sdata->dev,
skb, bssid,
hdr->addr2);
- /* FIXME: call with sdata->dev */
break;
case IEEE80211_IF_TYPE_AP:
if (!bssid) {
@@ -3964,6 +3977,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
&rx, sta);
} else
dev_kfree_skb(skb);
+ read_unlock(&local->sub_if_lock);
}
end:
@@ -4108,11 +4122,13 @@ static void ieee80211_stat_refresh(unsigned long data)
spin_unlock_bh(&local->sta_lock);
/* go through all subinterfaces */
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
sdata->channel_use = (sdata->channel_use_raw /
local->stat_time) / CHAN_UTIL_PER_10MS;
sdata->channel_use_raw = 0;
}
+ read_unlock(&local->sub_if_lock);
/* hardware interface */
local->channel_use = (local->channel_use_raw /
@@ -4549,7 +4565,6 @@ int ieee80211_init_rate_ctrl_alg(struct ieee80211_local *local,
{
struct rate_control_ref *ref, *old;
- ASSERT_RTNL();
if (local->open_count || netif_running(local->mdev) ||
(local->apdev && netif_running(local->apdev)))
return -EBUSY;
@@ -4665,7 +4680,7 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
INIT_LIST_HEAD(&local->modes_list);
- spin_lock_init(&local->sub_if_lock);
+ rwlock_init(&local->sub_if_lock);
INIT_LIST_HEAD(&local->sub_if_list);
INIT_DELAYED_WORK(&local->scan_work, ieee80211_sta_scan_work);
@@ -4708,7 +4723,6 @@ EXPORT_SYMBOL(ieee80211_alloc_hw);
int ieee80211_register_hw(struct ieee80211_hw *hw)
{
struct ieee80211_local *local = hw_to_local(hw);
- struct net_device *sta_dev;
int result;
result = wiphy_register(local->hw.wiphy);
@@ -4763,13 +4777,14 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
goto fail_wep;
}
- /* TODO: add rtnl locking around device creation and qdisc install */
ieee80211_install_qdisc(local->mdev);
/* add one default STA interface */
- result = ieee80211_if_add(local->mdev, "wlan%d", 1, &sta_dev);
- if (result == 0)
- ieee80211_if_set_type(sta_dev, IEEE80211_IF_TYPE_STA);
+ result = ieee80211_if_add(local->mdev, "wlan%d", NULL,
+ IEEE80211_IF_TYPE_STA);
+ if (result)
+ printk(KERN_WARNING "%s: Failed to add default virtual iface\n",
+ local->mdev->name);
local->reg_state = IEEE80211_DEV_REGISTERED;
rtnl_unlock();
@@ -4829,6 +4844,7 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
{
struct ieee80211_local *local = hw_to_local(hw);
struct ieee80211_sub_if_data *sdata, *tmp;
+ struct list_head tmp_list;
int i;
tasklet_kill(&local->tx_pending_tasklet);
@@ -4842,7 +4858,11 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
if (local->apdev)
ieee80211_if_del_mgmt(local);
- list_for_each_entry_safe(sdata, tmp, &local->sub_if_list, list)
+ write_lock_bh(&local->sub_if_lock);
+ list_replace_init(&local->sub_if_list, &tmp_list);
+ write_unlock_bh(&local->sub_if_lock);
+
+ list_for_each_entry_safe(sdata, tmp, &tmp_list, list)
__ieee80211_if_del(local, sdata);
rtnl_unlock();
diff --git a/net/mac80211/ieee80211_cfg.c b/net/mac80211/ieee80211_cfg.c
index e370b4b..0069826 100644
--- a/net/mac80211/ieee80211_cfg.c
+++ b/net/mac80211/ieee80211_cfg.c
@@ -16,8 +16,7 @@ static int ieee80211_add_iface(struct wiphy *wiphy, char *name,
unsigned int type)
{
struct ieee80211_local *local = wiphy_priv(wiphy);
- struct net_device *new_dev;
- int res, itype;
+ int itype;
if (unlikely(local->reg_state != IEEE80211_DEV_REGISTERED))
return -ENODEV;
@@ -45,16 +44,12 @@ static int ieee80211_add_iface(struct wiphy *wiphy, char *name,
return -EINVAL;
}
- res = ieee80211_if_add(local->mdev, name, 0, &new_dev);
- if (res == 0)
- ieee80211_if_set_type(new_dev, itype);
- return res;
+ return ieee80211_if_add(local->mdev, name, NULL, itype);
}
static int ieee80211_del_iface(struct wiphy *wiphy, int ifindex)
{
struct ieee80211_local *local = wiphy_priv(wiphy);
- int res;
struct net_device *dev;
char *name;
@@ -62,11 +57,13 @@ static int ieee80211_del_iface(struct wiphy *wiphy, int ifindex)
return -ENODEV;
dev = dev_get_by_index(ifindex);
+ if (!dev)
+ return 0;
+
name = dev->name;
dev_put(dev);
- res = ieee80211_if_remove(local->mdev, name, -1);
- return res;
+ return ieee80211_if_remove(local->mdev, name, -1);
}
struct cfg80211_ops mac80211_config_ops = {
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index f6d389b..6b3e1c5 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -490,7 +490,7 @@ struct ieee80211_local {
ieee80211_rx_handler *rx_handlers;
ieee80211_tx_handler *tx_handlers;
- spinlock_t sub_if_lock; /* mutex for STA data structures */
+ rwlock_t sub_if_lock; /* protects sub_if_list */
struct list_head sub_if_list;
int sta_scanning;
int scan_channel_idx;
@@ -753,7 +753,6 @@ void ieee80211_tx_set_iswep(struct ieee80211_txrx_data *tx);
int ieee80211_if_update_wds(struct net_device *dev, u8 *remote_addr);
void ieee80211_if_setup(struct net_device *dev);
void ieee80211_if_mgmt_setup(struct net_device *dev);
-void ieee80211_if_shutdown(struct net_device *dev);
int ieee80211_init_rate_ctrl_alg(struct ieee80211_local *local,
const char *name);
struct net_device_stats *ieee80211_dev_stats(struct net_device *dev);
@@ -830,15 +829,13 @@ int ieee80211_sta_disassociate(struct net_device *dev, u16 reason);
/* ieee80211_iface.c */
int ieee80211_if_add(struct net_device *dev, const char *name,
- int format, struct net_device **new_dev);
+ struct net_device **new_dev, int type);
void ieee80211_if_set_type(struct net_device *dev, int type);
void ieee80211_if_reinit(struct net_device *dev);
void __ieee80211_if_del(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata);
-void ieee80211_if_del(struct net_device *dev);
int ieee80211_if_remove(struct net_device *dev, const char *name, int id);
void ieee80211_if_free(struct net_device *dev);
-void ieee80211_if_flush(struct net_device *dev);
void ieee80211_if_sdata_init(struct ieee80211_sub_if_data *sdata);
int ieee80211_if_add_mgmt(struct ieee80211_local *local);
void ieee80211_if_del_mgmt(struct ieee80211_local *local);
diff --git a/net/mac80211/ieee80211_iface.c b/net/mac80211/ieee80211_iface.c
index 495177b..719bc21 100644
--- a/net/mac80211/ieee80211_iface.c
+++ b/net/mac80211/ieee80211_iface.c
@@ -38,7 +38,7 @@ static void ieee80211_if_sdata_deinit(struct ieee80211_sub_if_data *sdata)
/* Must be called with rtnl lock held. */
int ieee80211_if_add(struct net_device *dev, const char *name,
- int format, struct net_device **new_dev)
+ struct net_device **new_dev, int type)
{
struct net_device *ndev;
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
@@ -46,22 +46,14 @@ int ieee80211_if_add(struct net_device *dev, const char *name,
int ret;
ASSERT_RTNL();
- ndev = *new_dev = alloc_netdev(sizeof(struct ieee80211_sub_if_data),
- "", ieee80211_if_setup);
+ ndev = alloc_netdev(sizeof(struct ieee80211_sub_if_data),
+ name, ieee80211_if_setup);
if (!ndev)
return -ENOMEM;
- if (*name == '\0') {
- snprintf(ndev->name, IFNAMSIZ, "%s.%%d", dev->name);
- format = 1;
- }
-
- if (format) {
- ret = dev_alloc_name(ndev, name);
- if (ret < 0)
- goto fail;
- } else
- snprintf(ndev->name, IFNAMSIZ, "%s", name);
+ ret = dev_alloc_name(ndev, ndev->name);
+ if (ret < 0)
+ goto fail;
memcpy(ndev->dev_addr, local->hw.wiphy->perm_addr, ETH_ALEN);
ndev->base_addr = dev->base_addr;
@@ -83,14 +75,25 @@ int ieee80211_if_add(struct net_device *dev, const char *name,
goto fail;
ieee80211_debugfs_add_netdev(sdata);
+ ieee80211_if_set_type(ndev, type);
+
+ write_lock_bh(&local->sub_if_lock);
+ if (unlikely(local->reg_state == IEEE80211_DEV_UNREGISTERED)) {
+ write_unlock_bh(&local->sub_if_lock);
+ __ieee80211_if_del(local, sdata);
+ return -ENODEV;
+ }
list_add(&sdata->list, &local->sub_if_list);
+ if (new_dev)
+ *new_dev = ndev;
+ write_unlock_bh(&local->sub_if_lock);
+
ieee80211_update_default_wep_only(local);
return 0;
fail:
free_netdev(ndev);
- *new_dev = NULL;
return ret;
}
@@ -102,11 +105,11 @@ int ieee80211_if_add_mgmt(struct ieee80211_local *local)
ASSERT_RTNL();
- ndev = alloc_netdev(sizeof(struct ieee80211_sub_if_data), "",
+ ndev = alloc_netdev(sizeof(struct ieee80211_sub_if_data), "wmgmt%d",
ieee80211_if_mgmt_setup);
if (!ndev)
return -ENOMEM;
- ret = dev_alloc_name(ndev, "wmgmt%d");
+ ret = dev_alloc_name(ndev, ndev->name);
if (ret < 0)
goto fail;
@@ -232,16 +235,22 @@ void ieee80211_if_reinit(struct net_device *dev)
/* Remove all virtual interfaces that use this BSS
* as their sdata->bss */
struct ieee80211_sub_if_data *tsdata, *n;
+ LIST_HEAD(tmp_list);
+ write_lock_bh(&local->sub_if_lock);
list_for_each_entry_safe(tsdata, n, &local->sub_if_list, list) {
if (tsdata != sdata && tsdata->bss == &sdata->u.ap) {
printk(KERN_DEBUG "%s: removing virtual "
"interface %s because its BSS interface"
" is being removed\n",
sdata->dev->name, tsdata->dev->name);
- __ieee80211_if_del(local, tsdata);
+ list_move_tail(&tsdata->list, &tmp_list);
}
}
+ write_unlock_bh(&local->sub_if_lock);
+
+ list_for_each_entry_safe(tsdata, n, &tmp_list, list)
+ __ieee80211_if_del(local, tsdata);
kfree(sdata->u.ap.beacon_head);
kfree(sdata->u.ap.beacon_tail);
@@ -301,7 +310,6 @@ void __ieee80211_if_del(struct ieee80211_local *local,
{
struct net_device *dev = sdata->dev;
- list_del(&sdata->list);
ieee80211_debugfs_remove_netdev(sdata);
unregister_netdevice(dev);
/* Except master interface, the net_device will be freed by
@@ -316,15 +324,19 @@ int ieee80211_if_remove(struct net_device *dev, const char *name, int id)
ASSERT_RTNL();
+ write_lock_bh(&local->sub_if_lock);
list_for_each_entry_safe(sdata, n, &local->sub_if_list, list) {
if ((sdata->type == id || id == -1) &&
strcmp(name, sdata->dev->name) == 0 &&
sdata->dev != local->mdev) {
+ list_del(&sdata->list);
+ write_unlock_bh(&local->sub_if_lock);
__ieee80211_if_del(local, sdata);
ieee80211_update_default_wep_only(local);
return 0;
}
}
+ write_unlock_bh(&local->sub_if_lock);
return -ENODEV;
}
@@ -338,28 +350,3 @@ void ieee80211_if_free(struct net_device *dev)
ieee80211_if_sdata_deinit(sdata);
free_netdev(dev);
}
-
-/* Must be called with rtnl lock held. */
-void ieee80211_if_flush(struct net_device *dev)
-{
- struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
- struct ieee80211_sub_if_data *sdata, *n;
-
- ASSERT_RTNL();
- list_for_each_entry_safe(sdata, n, &local->sub_if_list, list) {
- __ieee80211_if_del(local, sdata);
- }
-}
-
-void ieee80211_if_del(struct net_device *dev)
-{
- struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
- struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
-
- rtnl_lock();
- if (sdata->type == IEEE80211_IF_TYPE_MGMT)
- ieee80211_if_del_mgmt(local);
- else
- __ieee80211_if_del(local, sdata);
- rtnl_unlock();
-}
diff --git a/net/mac80211/ieee80211_ioctl.c b/net/mac80211/ieee80211_ioctl.c
index 2ff762d..502010e 100644
--- a/net/mac80211/ieee80211_ioctl.c
+++ b/net/mac80211/ieee80211_ioctl.c
@@ -1003,23 +1003,30 @@ static int ieee80211_ioctl_add_if(struct net_device *dev,
if (left < sizeof(struct hostapd_if_wds))
return -EPROTO;
- res = ieee80211_if_add(dev, param->u.if_info.name, 0, &new_dev);
+ res = ieee80211_if_add(dev, param->u.if_info.name, &new_dev,
+ IEEE80211_IF_TYPE_WDS);
if (res)
return res;
- ieee80211_if_set_type(new_dev, IEEE80211_IF_TYPE_WDS);
res = ieee80211_if_update_wds(new_dev, wds->remote_addr);
- if (res)
- __ieee80211_if_del(wdev_priv(dev->ieee80211_ptr),
- IEEE80211_DEV_TO_SUB_IF(new_dev));
+ if (unlikely(res)) {
+ struct ieee80211_local *local =
+ wdev_priv(dev->ieee80211_ptr);
+ struct ieee80211_sub_if_data *sdata =
+ IEEE80211_DEV_TO_SUB_IF(new_dev);
+ write_lock_bh(&local->sub_if_lock);
+ list_del(&sdata->list);
+ write_unlock_bh(&local->sub_if_lock);
+ __ieee80211_if_del(local, sdata);
+ }
return res;
case HOSTAP_IF_VLAN:
if (left < sizeof(struct hostapd_if_vlan))
return -EPROTO;
- res = ieee80211_if_add(dev, param->u.if_info.name, 0, &new_dev);
+ res = ieee80211_if_add(dev, param->u.if_info.name, NULL,
+ IEEE80211_IF_TYPE_VLAN);
if (res)
return res;
- ieee80211_if_set_type(new_dev, IEEE80211_IF_TYPE_VLAN);
#if 0
res = ieee80211_if_update_vlan(new_dev, vlan->id);
if (res)
@@ -1033,21 +1040,19 @@ static int ieee80211_ioctl_add_if(struct net_device *dev,
if (left < sizeof(struct hostapd_if_bss))
return -EPROTO;
- res = ieee80211_if_add(dev, param->u.if_info.name, 0, &new_dev);
+ res = ieee80211_if_add(dev, param->u.if_info.name, &new_dev,
+ IEEE80211_IF_TYPE_AP);
if (res)
return res;
- ieee80211_if_set_type(new_dev, IEEE80211_IF_TYPE_AP);
memcpy(new_dev->dev_addr, bss->bssid, ETH_ALEN);
return 0;
case HOSTAP_IF_STA:
if (left < sizeof(struct hostapd_if_sta))
return -EPROTO;
- res = ieee80211_if_add(dev, param->u.if_info.name, 0, &new_dev);
- if (res)
- return res;
- ieee80211_if_set_type(new_dev, IEEE80211_IF_TYPE_STA);
- return 0;
+ res = ieee80211_if_add(dev, param->u.if_info.name, NULL,
+ IEEE80211_IF_TYPE_STA);
+ return res;
default:
return -EINVAL;
}
@@ -1093,36 +1098,34 @@ static int ieee80211_ioctl_update_if(struct net_device *dev,
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
struct net_device *wds_dev = NULL;
struct ieee80211_sub_if_data *sdata;
+ int ret;
if (left < sizeof(struct ieee80211_if_wds))
return -EPROTO;
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
if (strcmp(param->u.if_info.name,
sdata->dev->name) == 0) {
wds_dev = sdata->dev;
+ dev_hold(wds_dev);
break;
}
}
+ read_unlock(&local->sub_if_lock);
if (!wds_dev || sdata->type != IEEE80211_IF_TYPE_WDS)
return -ENODEV;
- return ieee80211_if_update_wds(wds_dev, wds->remote_addr);
+ ret = ieee80211_if_update_wds(wds_dev, wds->remote_addr);
+ dev_put(wds_dev);
+ return ret;
} else {
return -EOPNOTSUPP;
}
}
-static int ieee80211_ioctl_flush_ifs(struct net_device *dev,
- struct prism2_hostapd_param *param)
-{
- ieee80211_if_flush(dev);
- return 0;
-}
-
-
static int ieee80211_ioctl_scan_req(struct net_device *dev,
struct prism2_hostapd_param *param,
int param_len)
@@ -1512,9 +1515,6 @@ static int ieee80211_ioctl_priv_hostapd(struct net_device *dev,
case PRISM2_HOSTAPD_MLME:
ret = ieee80211_ioctl_mlme(dev, param);
break;
- case PRISM2_HOSTAPD_FLUSH_IFS:
- ret = ieee80211_ioctl_flush_ifs(dev, param);
- break;
case PRISM2_HOSTAPD_SET_RADAR_PARAMS:
ret = ieee80211_ioctl_set_radar_params(dev, param);
break;
@@ -2239,6 +2239,7 @@ static int ieee80211_ioctl_clear_keys(struct net_device *dev)
struct sta_info *sta;
memset(addr, 0xff, ETH_ALEN);
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
for (i = 0; i < NUM_DEFAULT_KEYS; i++) {
keyconf = NULL;
@@ -2256,6 +2257,7 @@ static int ieee80211_ioctl_clear_keys(struct net_device *dev)
}
sdata->default_key = NULL;
}
+ read_unlock(&local->sub_if_lock);
spin_lock_bh(&local->sta_lock);
list_for_each_entry(sta, &local->sta_list, list) {
@@ -2388,6 +2390,7 @@ static int ieee80211_ioctl_default_wep_only(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata;
local->default_wep_only = value;
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list)
for (i = 0; i < NUM_DEFAULT_KEYS; i++)
if (value)
@@ -2396,6 +2399,7 @@ static int ieee80211_ioctl_default_wep_only(struct ieee80211_local *local,
else
ieee80211_key_disable_hwaccel(local,
sdata->keys[i]);
+ read_unlock(&local->sub_if_lock);
return 0;
}
@@ -2406,7 +2410,7 @@ void ieee80211_update_default_wep_only(struct ieee80211_local *local)
int i = 0;
struct ieee80211_sub_if_data *sdata;
- spin_lock_bh(&local->sub_if_lock);
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
if (sdata->dev == local->mdev)
@@ -2415,7 +2419,7 @@ void ieee80211_update_default_wep_only(struct ieee80211_local *local)
/* If there is an AP interface then depend on userspace to
set default_wep_only correctly. */
if (sdata->type == IEEE80211_IF_TYPE_AP) {
- spin_unlock_bh(&local->sub_if_lock);
+ read_unlock(&local->sub_if_lock);
return;
}
@@ -2427,7 +2431,7 @@ void ieee80211_update_default_wep_only(struct ieee80211_local *local)
else
ieee80211_ioctl_default_wep_only(local, 0);
- spin_unlock_bh(&local->sub_if_lock);
+ read_unlock(&local->sub_if_lock);
}
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 0b01800..3d16ed8 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -2569,7 +2569,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
memset(&wrqu, 0, sizeof(wrqu));
wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL);
- spin_lock_bh(&local->sub_if_lock);
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
if (sdata->type == IEEE80211_IF_TYPE_STA) {
if (sdata->u.sta.associated)
@@ -2578,7 +2578,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
}
netif_wake_queue(sdata->dev);
}
- spin_unlock_bh(&local->sub_if_lock);
+ read_unlock(&local->sub_if_lock);
sdata = IEEE80211_DEV_TO_SUB_IF(dev);
if (sdata->type == IEEE80211_IF_TYPE_IBSS) {
@@ -2714,14 +2714,14 @@ static int ieee80211_sta_start_scan(struct net_device *dev,
local->sta_scanning = 1;
- spin_lock_bh(&local->sub_if_lock);
+ read_lock(&local->sub_if_lock);
list_for_each_entry(sdata, &local->sub_if_list, list) {
netif_stop_queue(sdata->dev);
if (sdata->type == IEEE80211_IF_TYPE_STA &&
sdata->u.sta.associated)
ieee80211_send_nullfunc(local, sdata, 1);
}
- spin_unlock_bh(&local->sub_if_lock);
+ read_unlock(&local->sub_if_lock);
if (ssid) {
local->scan_ssid_len = ssid_len;
@@ -2965,7 +2965,6 @@ struct sta_info * ieee80211_ibss_add_sta(struct net_device *dev,
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
struct sta_info *sta;
struct ieee80211_sub_if_data *sdata = NULL;
- struct net_device *sta_dev = NULL;
/* TODO: Could consider removing the least recently used entry and
* allow new one to be added. */
@@ -2977,26 +2976,13 @@ struct sta_info * ieee80211_ibss_add_sta(struct net_device *dev,
return NULL;
}
- spin_lock_bh(&local->sub_if_lock);
- list_for_each_entry(sdata, &local->sub_if_list, list)
- if (sdata->type == IEEE80211_IF_TYPE_IBSS &&
- memcmp(bssid, sdata->u.sta.bssid, ETH_ALEN) == 0) {
- sta_dev = sdata->dev;
- break;
- }
- spin_unlock_bh(&local->sub_if_lock);
-
- if (!sta_dev)
- return NULL;
-
printk(KERN_DEBUG "%s: Adding new IBSS station " MAC_FMT " (dev=%s)\n",
- dev->name, MAC_ARG(addr), sta_dev->name);
+ local->mdev->name, MAC_ARG(addr), dev->name);
sta = sta_info_add(local, dev, addr, GFP_ATOMIC);
if (!sta)
return NULL;
- sta->dev = sta_dev;
sta->supp_rates = sdata->u.sta.supp_rates_bits;
rate_control_rate_init(sta, local);
Dan Williams wrote:
> we're not being open enough to functionality that might be in hardware.
> Holding a 100% software line with mac80211 is just IMHO wrong and
> short-sighted. The stack needs to be flexible WRT to the hardware
Yeah but this isn't "in hardware" -- it's in firmware: software that
runs of a different, vendor-specific, CPU. Let's not talk about magic
and impressive "hardware" when the truth is we only talk about the same
software action on another CPU. The problems with this particular
offload to firmware:
- it is vendor-specific
- it is not time-critical. In fact it just sits there for tens of ms.
The extra us lost going through mac80211 when it wants to change the
frequency should not be measurable. It's not like it is some special
DSP in there that is computing PI faster than the main CPU in the box
can. It is just changing the frequency now and then, which can
perfectly well be done in the stack
- it is not exploitable in other ways. If we want to talk about
"short-sighted", let's talk about managing what can be very generic RF
hardware in a way that can never do anything but IEEE802.11a/b/g actions
> capabilities of the parts that we expect to use it. Saying no to
> hardware scanning just because it can also be done in software too is
> wrong.
To be clear, this isn't a "hardware" action, just an opaque software API
specific to that chipset, and that runs on a CPU in the chipset.
> with. It's already in, right? Ripping it out for a 100% software
> agenda is wrong. Let's take out crypto offload then too if we're going
The iwlwifi proposal is for an opaque vendor-specific firmware API, that
is also "100% software". Don't get confused by the bogus magic alleged
"hardware" nomenclature.
> to take a consistently 100% software line. Again, it's not either
> software or hardware; it's a spectrum of capabilities and we shouldn't
> be making the stack _less_ flexible.
Yeah let's make the stack completely flexible and either keep control of
the scan action in the stack, or eventually move it to usermode, in both
cases doing the scan action well ONCE for ALL drivers.
-Andy
Michael Wu wrote:
> On Thursday 26 April 2007 17:57, James Ketrenos wrote:
>> With a penalty to battery life and an increase in the amount of time a scan
>> takes. There are improvements that can be made to make the software
>> scanning faster, but at a penalty of added complexity on both the driver
>> and the stack side -- for no *real* gain for users that have cards that can
>> offload the scan.
>>
> Sure there is. Smaller firmware means firmware that's less likely to
> malfunction, which seems to be an issue.
I haven't seen that as an issue; I've seen bugs in the driver that are misconfiguring the hardware or with the driver/stack trying to reconfigure things while a hardware scan is already occurring.
> I don't see what you're saying about
> added complexity on the driver side.
Right now the scanning in mac80211 is slow. I haven't really dug into it to see if it will transition from passive to active if it detects RF activity on an otherwise passive channel, etc. but I have seen that if we turn on hw scanning we can get results back in a few seconds vs. 10 or more.
The more passive channels you add (most of 802.11a) the slower the scanning gets.
>Eliminating the hw_scan callback reduces driver complexity.
If done right, the stack would set up the list of channels to scan, whether to scan the channel active or passive, and the template for the probe request to use.
For sw based scans, the stack would then have a mechanism for executing that scan through a series of tunes, transmits, etc. For hw scans, it would pass that structure to the driver which would package it for the hardware and pass it down.
Upon completion of the scan (sw or hw) the notification would come back to the stack to let it know scanning is complete.
The sw scan engine would be tied into the rest of the transmit infrastructure to manage off channel time, etc. (which you don't have to do with hw scanning since the hardware/uCode does it all for you)
> The increase in complexity on the stack side is offset by
> the fact that no driver/firmware/hardware needs to implement their own clever
> scanning algorithm.
Maybe yes and maybe no.
In order to do it all as fast as possible on the host you need access to all channel activity during the scan, especially on passive channels. If traffic is detected a probe request can be sent and then that channel can be given up on faster if it had to sit around for an arbitrary amount of time (110ms perhaps to ensure you catch beacons being transmitted at the vendor default interval of 100ms) Or if there is no traffic at all for some interval you could assume nothing is on that channel. Every 10 channels that's a full second you have to sit around...
Unless there is a mechanism to quickly and easily toggle between filter and don't filter, you'll end up turning off hw/uCode filtering of packets all the time.
Which means the hardware would be set to full promiscuous mode and every packet Rx'd would get tossed to the host to then either process or discard--which keeps the host CPU awake (which isn't good on laptops or anywhere else folks care about power consumption) And scanning is one of those things that happens more frequently when you are not plugged in and are moving around the home, school, office, etc.
>>> Calling hw_scan starts a
>>> hardware scan, but there is no way to know when the scan is completed.
>> That needs to be improved.
>>
> This is exactly the issue.
>
>>> Even if that problem were addressed, I still wouldn't like it as the
>>> design of the hw_scan callback is deficient in a number of other ways and
>>> is completely inapplicable to all other hardware/firmware softmac designs
>>> that I know.
>> Given that there are things we can do as a result of the hw_scan that we
>> can't do on the host without imposing greater system load and slower scan
>> results, I really don't want to lose the ability to support hardware
>> offloading of scanning.
>>
> How fast is hardware scanning? What part of software scanning is slowing
> things down?
Passive scanning is the worst. 802.11a takes *forever* if you have to sit around on a channel waiting for beacons without issuing a probe request -- and you can't issue a probe request until other traffic is seen.
> What's the big bottleneck that justifies moving this to
> hardware?
With the 3945 its mainly power consumption.
> Are you sure it cannot be eliminated?
Not without an impact to power use. When 802.11n and 802.11e gets added to the mix, I'm not sure if it can realistically be eliminated through a host implementation without impacting performance.
> Why haven't any other vendors chosen to offload scanning like this?
I don't know how other vendors justify a lot of what they do and don't do.
>> Could you voice some of the "other ways" the current hw_scan callback is
>> deficient?
>>
> Sure. In addition to not being able to find out when the scan is completed..
>
> - There is no way to specify what channels to scan.
> - There is no way to specify a passive scan.
> - The probe frame that is used is fixed with the exception of the SSID.
> - There is no corresponding interface for the userspace MLME.
The interface to the uCode supports configuring all of the above elements--I'll look at how mac80211 currently sets up its scanning algorithm and see if there is a clean spot to have it push that data to the driver if hw scan is supported. That way the user MLME *could* customize it.
> This isn't very important, however. I am more interested in what problems in
> software scanning need to be solved by moving to hardware scanning.
Latency on getting scan results (especially as more passive channels are added) and platform power utilization while mobile.
>> In the short term, I would rather we leave hw_scan in the code and have the
>> users that currently rely on hw_scan just have to manually configure the AP
>> selection until such time as the in-kernel-AP-selection-policy code works
>> with hw offloaded scan.
>>
> What incentive does that give to fix the code?
I want userspace to be able to fully configure the set of channels and parameters to pass to the hardware. But overhauling scan component to better support hardware scanning will likely take time.
> Leaving broken things in for
> the sake of out-of-tree drivers does not appeal to me.
Nor to me--and we plan on getting iwlwifi in-tree. We also plan on using hw_scan. Keeping it in the stack for now helps me out. Removing it will make my life harder.
James
On Tuesday 24 April 2007 14:09, Andy Green wrote:
> =============================================
> [ INFO: possible recursive locking detected ]
> 2.6.21-rc7 #11
> ---------------------------------------------
> ip/2600 is trying to acquire lock:
> (&dev->_xmit_lock){-+..}, at: [<e09199c5>]
> ieee80211_set_multicast_list+0x46/0x167 [mac80211]
>
> but task is already holding lock:
> (&dev->_xmit_lock){-+..}, at: [<c05abc05>] dev_mc_upload+0x14/0x3c
>
This is fine, though it would be nice if we can shut the warning off. Virtual
interfaces can try to take the xmit lock of the master device, but the master
device will never try to take the xmit lock of the virtual interfaces and
especially not while holding its own xmit lock.
-Michael Wu
Michael Wu wrote:
> From: Michael Wu <[email protected]>
>
> The hw_scan callback breaks the auto AP selection code in ieee80211_sta.c.
> No in-tree drivers use it either. This patch removes it.
What is the specific problem that is being addressed by pulling hw offload scan support?
(I would like to see hw_scan remain since iwlwifi uses it -- which we hope to submit as soon as the code restructuring is complete)
James
Michael Wu wrote:
> On Wednesday 25 April 2007 14:16, John W. Linville wrote:
>> On Tue, Apr 24, 2007 at 10:03:03PM -0700, James Ketrenos wrote:
>>> (I would like to see hw_scan remain since iwlwifi uses it -- which we
>>> hope to submit as soon as the code restructuring is complete)
>> I was going to mention this in response to Michael's patch, but then
>> I checked your current iwlwifi git tree. There your hw_scan routine
>> is commented-out, so I figured you didn't want/need it anymore...?
>>
...
> Besides the fact that what hw_scan does can be done by mac80211 for all
> drivers,
With a penalty to battery life and an increase in the amount of time a scan takes. There are improvements that can be made to make the software scanning faster, but at a penalty of added complexity on both the driver and the stack side -- for no *real* gain for users that have cards that can offload the scan.
> the use of the hw_scan callback breaks the AP autoconfiguration code
> in ieee80211_sta.c due to its inadequate design.
Is it breaking it just due to the auto-configuration code not knowing when to configure things?
> Calling hw_scan starts a
> hardware scan, but there is no way to know when the scan is completed.
That needs to be improved.
> Even if that problem were addressed, I still wouldn't like it as the design of
> the hw_scan callback is deficient in a number of other ways and is completely
> inapplicable to all other hardware/firmware softmac designs that I know.
Given that there are things we can do as a result of the hw_scan that we can't do on the host without imposing greater system load and slower scan results, I really don't want to lose the ability to support hardware offloading of scanning.
Could you voice some of the "other ways" the current hw_scan callback is deficient?
I'm fine with us overhauling how hw_scan works and integrates with the stack, but an all out ban on hardware scan offload just doesn't make sense. The host can do all RC4 and AES encrypt/decrypt too but certainly we would prefer the hardware to do that for us, right?
In the short term, I would rather we leave hw_scan in the code and have the users that currently rely on hw_scan just have to manually configure the AP selection until such time as the in-kernel-AP-selection-policy code works with hw offloaded scan.
James
From: Michael Wu <[email protected]>
The hw_scan callback breaks the auto AP selection code in ieee80211_sta.c.
No in-tree drivers use it either. This patch removes it.
Signed-off-by: Michael Wu <[email protected]>
---
include/net/mac80211.h | 4 ----
net/mac80211/ieee80211.c | 3 +--
net/mac80211/ieee80211_sta.c | 11 +----------
3 files changed, 2 insertions(+), 16 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index b4b6619..cade956 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -668,10 +668,6 @@ struct ieee80211_ops {
int (*passive_scan)(struct ieee80211_hw *hw, int state,
struct ieee80211_scan_conf *conf);
- /* Ask the hardware to service the scan request, no need to start
- * the scan state machine in stack. */
- int (*hw_scan)(struct ieee80211_hw *hw, u8 *ssid, size_t len);
-
/* return low-level statistics */
int (*get_stats)(struct ieee80211_hw *hw,
struct ieee80211_low_level_stats *stats);
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 25ea011..ddb8153 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -2235,8 +2235,7 @@ static void ieee80211_if_shutdown(struct net_device *dev)
sdata->u.sta.state = IEEE80211_DISABLED;
del_timer_sync(&sdata->u.sta.timer);
skb_queue_purge(&sdata->u.sta.skb_queue);
- if (!local->ops->hw_scan &&
- local->scan_dev == sdata->dev) {
+ if (local->scan_dev == sdata->dev) {
local->sta_scanning = 0;
cancel_delayed_work(&local->scan_work);
}
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index fe5c762..7cb2dd3 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -23,6 +23,7 @@
#include <linux/wireless.h>
#include <linux/random.h>
#include <linux/etherdevice.h>
+#include <linux/rtnetlink.h>
#include <net/iw_handler.h>
#include <asm/types.h>
#include <asm/delay.h>
@@ -2713,16 +2714,6 @@ static int ieee80211_sta_start_scan(struct net_device *dev,
return -EBUSY;
}
- if (local->ops->hw_scan) {
- int rc = local->ops->hw_scan(local_to_hw(local),
- ssid, ssid_len);
- if (!rc) {
- local->sta_scanning = 1;
- local->scan_dev = dev;
- }
- return rc;
- }
-
local->sta_scanning = 1;
read_lock(&local->sub_if_lock);
On Fri, 27 Apr 2007 10:28:52 -0400, Dan Williams wrote:
> We shouldn't be ignoring discrete hardware functionality just because
> it's in the firmware and also may be in the stack. Hardware crypto like
> somebody mentioned. TCP offload. etc. Not allowing a driver writer to
> take advantage of hardware functionality is quite short-sighted. Having
> a pure host-CPU software stack is not utopia; it's entirely unrealistic
> for a variety of reasons, some of which are below [1].
I'm not saying no (see my other mail in this thread), I see some
reasons myself, but I don't think your reasons are valid.
> 1) power-critical situations like embedded devices where some pieces
> must be offloaded to the wireless part
Such solutions will use fullmac. See e.g. OLPC.
> 2) lower-speed devices that may not have cycles to burn on functions
> that the hardware can also do, even if most of the stack is software
Such solutions have no other way than using fullmac.
> 3) timing critical functions
Such as? Scanning? That's not timing critical at all. Or something
other?
> 4) hybrid parts that are mostly softmac (ipw2100, ipw2200)
Supporting of halfmacs in a softmac stack is hardly possible. You can
write something like a "halfsoftmac" but you need to do that for each
such chipset again. Because (by definition) every halfmac chipset needs
a different set of functionality from the stack.
> 5) we don't make hardware
But we are also not required to support every obscure feature of the
hardware.
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
On Fri, 27 Apr 2007 13:47:07 -0700, James Ketrenos wrote:
> [PATCH] Fix ieee80211_sta_config_auth to select networks if rssi < 0
>
> The rssi check was against the top_rssi being less than the network
> rssi. However, 0 is higher than all negative rssi, so if an adapter
> returned negative rssi values, auto AP selection would never work.
>
> This patch changes behavior such if no network is currently selected it
> ignores the rssi comparison and picks the network that matches all other criteria.
Applied, thanks!
Jiri
--
Jiri Benc
SUSE Labs
Michael Wu wrote:
> From: Michael Wu <[email protected]>
>
> The hw_scan callback breaks the auto AP selection code in ieee80211_sta.c.
> No in-tree drivers use it either. This patch removes it.
The auto AP selection code is broken because ieee80211_sta_config_auth wasn't selecting a network if the RSSI was negative.)
Thanks,
James
----
[PATCH] Fix ieee80211_sta_config_auth to select networks if rssi < 0
The rssi check was against the top_rssi being less than the network
rssi. However, 0 is higher than all negative rssi, so if an adapter
returned negative rssi values, auto AP selection would never work.
This patch changes behavior such if no network is currently selected it
ignores the rssi comparison and picks the network that matches all other criteria.
Signed-off-by: James Ketrenos <[email protected]>
---
net/mac80211/ieee80211_sta.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 92e575a..6889e79 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -2249,7 +2249,7 @@ static int ieee80211_sta_config_auth(struct net_device *dev,
!ieee80211_sta_match_ssid(ifsta, bss->ssid, bss->ssid_len))
continue;
- if (top_rssi < bss->rssi) {
+ if (!selected || top_rssi < bss->rssi) {
selected = bss;
top_rssi = bss->rssi;
}
--
1.5.0.6
On Mon, 23 Apr 2007 14:48:13 -0400, Michael Wu wrote:
> This converts sub_if_lock to a rw lock and makes all code touching
> sub_if_list use it, grabs mdev's tx lock in set_multicast_list to
> synchronize multicast configuration, and simplifies some related code.
>
> [...]
> --- a/net/mac80211/ieee80211.c
> +++ b/net/mac80211/ieee80211.c
> [...]
> @@ -2148,6 +2148,7 @@ static void ieee80211_set_multicast_list(struct net_device *dev)
> struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> unsigned short flags;
>
> + netif_tx_lock(local->mdev);
Shouldn't it be netif_tx_lock_bh?
> if (((dev->flags & IFF_ALLMULTI) != 0) ^ (sdata->allmulti != 0)) {
> if (sdata->allmulti) {
> sdata->allmulti = 0;
> @@ -2177,9 +2178,12 @@ static void ieee80211_set_multicast_list(struct net_device *dev)
> flags |= IFF_ALLMULTI;
> if (local->iff_promiscs)
> flags |= IFF_PROMISC;
> + read_lock(&local->sub_if_lock);
> local->ops->set_multicast_list(local_to_hw(local), flags,
> local->mc_count);
Would be nice to have documented that set_multicast_list is called in
atomic context.
> + read_unlock(&local->sub_if_lock);
> }
> + netif_tx_unlock(local->mdev);
> }
>
> struct dev_mc_list *ieee80211_get_mc_list_item(struct ieee80211_hw *hw,
> @@ -2220,12 +2224,11 @@ static struct net_device_stats *ieee80211_get_stats(struct net_device *dev)
> return &(sdata->stats);
> }
>
> -void ieee80211_if_shutdown(struct net_device *dev)
> +static void ieee80211_if_shutdown(struct net_device *dev)
> {
> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>
> - ASSERT_RTNL();
As you already said, this is not necessary.
> [...]
> @@ -3871,6 +3884,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
> struct sk_buff *skb_new;
> u8 *bssid = ieee80211_get_bssid(hdr, skb->len - radiotap_len);
>
> + read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list) {
> rx.u.rx.ra_match = 1;
> switch (sdata->type) {
> @@ -3906,10 +3920,9 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
> rx.u.rx.ra_match = 0;
> } else if (!sta)
> sta = rx.sta =
> - ieee80211_ibss_add_sta(local->mdev,
> + ieee80211_ibss_add_sta(sdata->dev,
> skb, bssid,
> hdr->addr2);
> - /* FIXME: call with sdata->dev */
> break;
> case IEEE80211_IF_TYPE_AP:
> if (!bssid) {
> @@ -3964,6 +3977,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
> &rx, sta);
> } else
> dev_kfree_skb(skb);
> + read_unlock(&local->sub_if_lock);
Documenting that sub_if_lock cannot be taken under sta_bss_lock nor
under sta_lock would be nice.
local->ops->conf_tx and ->set_time are called under spinlock, are we ok with that?
> [...]
> --- a/net/mac80211/ieee80211_ioctl.c
> +++ b/net/mac80211/ieee80211_ioctl.c
> @@ -1003,23 +1003,30 @@ static int ieee80211_ioctl_add_if(struct net_device *dev,
> if (left < sizeof(struct hostapd_if_wds))
> return -EPROTO;
>
> - res = ieee80211_if_add(dev, param->u.if_info.name, 0, &new_dev);
> + res = ieee80211_if_add(dev, param->u.if_info.name, &new_dev,
> + IEEE80211_IF_TYPE_WDS);
> if (res)
> return res;
> - ieee80211_if_set_type(new_dev, IEEE80211_IF_TYPE_WDS);
> res = ieee80211_if_update_wds(new_dev, wds->remote_addr);
> - if (res)
> - __ieee80211_if_del(wdev_priv(dev->ieee80211_ptr),
> - IEEE80211_DEV_TO_SUB_IF(new_dev));
> + if (unlikely(res)) {
> + struct ieee80211_local *local =
> + wdev_priv(dev->ieee80211_ptr);
> + struct ieee80211_sub_if_data *sdata =
> + IEEE80211_DEV_TO_SUB_IF(new_dev);
> + write_lock_bh(&local->sub_if_lock);
> + list_del(&sdata->list);
> + write_unlock_bh(&local->sub_if_lock);
> + __ieee80211_if_del(local, sdata);
> + }
A good candidate for a separate function :-) But it can be done later
when this is needed at some other place.
> return res;
> case HOSTAP_IF_VLAN:
> if (left < sizeof(struct hostapd_if_vlan))
> return -EPROTO;
>
> - res = ieee80211_if_add(dev, param->u.if_info.name, 0, &new_dev);
> + res = ieee80211_if_add(dev, param->u.if_info.name, NULL,
> + IEEE80211_IF_TYPE_VLAN);
> if (res)
> return res;
> - ieee80211_if_set_type(new_dev, IEEE80211_IF_TYPE_VLAN);
> #if 0
> res = ieee80211_if_update_vlan(new_dev, vlan->id);
> if (res)
I know that the next few lines are commented out but please fix them as
well or remove the #ifed out code completely, otherwise we may be in
trouble later.
> [...]
> @@ -1093,36 +1098,34 @@ static int ieee80211_ioctl_update_if(struct net_device *dev,
> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> struct net_device *wds_dev = NULL;
> struct ieee80211_sub_if_data *sdata;
> + int ret;
>
> if (left < sizeof(struct ieee80211_if_wds))
> return -EPROTO;
>
> + read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list) {
> if (strcmp(param->u.if_info.name,
> sdata->dev->name) == 0) {
> wds_dev = sdata->dev;
> + dev_hold(wds_dev);
> break;
> }
> }
> + read_unlock(&local->sub_if_lock);
>
> if (!wds_dev || sdata->type != IEEE80211_IF_TYPE_WDS)
> return -ENODEV;
>
> - return ieee80211_if_update_wds(wds_dev, wds->remote_addr);
> + ret = ieee80211_if_update_wds(wds_dev, wds->remote_addr);
> + dev_put(wds_dev);
This was the place with unnecessary dev_hold/dev_put you talked about,
right? Just wanted to show I'm paying attention :-)
> [...]
> @@ -2239,6 +2239,7 @@ static int ieee80211_ioctl_clear_keys(struct net_device *dev)
> struct sta_info *sta;
>
> memset(addr, 0xff, ETH_ALEN);
> + read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list) {
> for (i = 0; i < NUM_DEFAULT_KEYS; i++) {
> keyconf = NULL;
> @@ -2256,6 +2257,7 @@ static int ieee80211_ioctl_clear_keys(struct net_device *dev)
> }
> sdata->default_key = NULL;
> }
> + read_unlock(&local->sub_if_lock);
Again, local->ops->set_key called under spinlock.
>
> spin_lock_bh(&local->sta_lock);
> list_for_each_entry(sta, &local->sta_list, list) {
> @@ -2388,6 +2390,7 @@ static int ieee80211_ioctl_default_wep_only(struct ieee80211_local *local,
> struct ieee80211_sub_if_data *sdata;
>
> local->default_wep_only = value;
> + read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list)
> for (i = 0; i < NUM_DEFAULT_KEYS; i++)
> if (value)
> @@ -2396,6 +2399,7 @@ static int ieee80211_ioctl_default_wep_only(struct ieee80211_local *local,
> else
> ieee80211_key_disable_hwaccel(local,
> sdata->keys[i]);
> + read_unlock(&local->sub_if_lock);
>
> return 0;
> }
> @@ -2406,7 +2410,7 @@ void ieee80211_update_default_wep_only(struct ieee80211_local *local)
> int i = 0;
> struct ieee80211_sub_if_data *sdata;
>
> - spin_lock_bh(&local->sub_if_lock);
> + read_lock(&local->sub_if_lock);
> list_for_each_entry(sdata, &local->sub_if_list, list) {
>
> if (sdata->dev == local->mdev)
> @@ -2415,7 +2419,7 @@ void ieee80211_update_default_wep_only(struct ieee80211_local *local)
> /* If there is an AP interface then depend on userspace to
> set default_wep_only correctly. */
> if (sdata->type == IEEE80211_IF_TYPE_AP) {
> - spin_unlock_bh(&local->sub_if_lock);
> + read_unlock(&local->sub_if_lock);
> return;
> }
>
> @@ -2427,7 +2431,7 @@ void ieee80211_update_default_wep_only(struct ieee80211_local *local)
> else
> ieee80211_ioctl_default_wep_only(local, 0);
Recursively taken lock. Ok, it's rwlock locked for reading, but I'd
still prefer not doing that.
>
> - spin_unlock_bh(&local->sub_if_lock);
> + read_unlock(&local->sub_if_lock);
> }
> [...]
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
On Friday 27 April 2007 13:40, Jiri Benc wrote:
> I agree.
>
> And I'd like to see some way to disable this feature for sane cards.
>
Here you go.
Thanks,
-Michael Wu
John W. Linville wrote:
> On Tue, Apr 24, 2007 at 10:03:03PM -0700, James Ketrenos wrote:
>
>> (I would like to see hw_scan remain since iwlwifi uses it -- which we hope
>> to submit as soon as the code restructuring is complete)
>
> I was going to mention this in response to Michael's patch, but then
> I checked your current iwlwifi git tree. There your hw_scan routine
> is commented-out, so I figured you didn't want/need it anymore...?
One of the changes in iwlwifi caused the hwscan to start locking the system and at the time it was easier to comment it out and focus on other issues (rate scaling was broken as well)
When the hw scanning does work, its returns scan results a lot faster to the user without as much processing overhead, so we'll definitely want to switch back to using the hwscan for iwlwifi.
James
On Thursday 26 April 2007 17:57, James Ketrenos wrote:
> With a penalty to battery life and an increase in the amount of time a scan
> takes. There are improvements that can be made to make the software
> scanning faster, but at a penalty of added complexity on both the driver
> and the stack side -- for no *real* gain for users that have cards that can
> offload the scan.
>
Sure there is. Smaller firmware means firmware that's less likely to
malfunction, which seems to be an issue. I don't see what you're saying about
added complexity on the driver side. Eliminating the hw_scan callback reduces
driver complexity. The increase in complexity on the stack side is offset by
the fact that no driver/firmware/hardware needs to implement their own clever
scanning algorithm.
> > the use of the hw_scan callback breaks the AP autoconfiguration code
> > in ieee80211_sta.c due to its inadequate design.
>
> Is it breaking it just due to the auto-configuration code not knowing when
> to configure things?
>
Yes, because ..
> > Calling hw_scan starts a
> > hardware scan, but there is no way to know when the scan is completed.
>
> That needs to be improved.
>
This is exactly the issue.
> > Even if that problem were addressed, I still wouldn't like it as the
> > design of the hw_scan callback is deficient in a number of other ways and
> > is completely inapplicable to all other hardware/firmware softmac designs
> > that I know.
>
> Given that there are things we can do as a result of the hw_scan that we
> can't do on the host without imposing greater system load and slower scan
> results, I really don't want to lose the ability to support hardware
> offloading of scanning.
>
How fast is hardware scanning? What part of software scanning is slowing
things down? What's the big bottleneck that justifies moving this to
hardware? Are you sure it cannot be eliminated? Why haven't any other vendors
chosen to offload scanning like this?
> Could you voice some of the "other ways" the current hw_scan callback is
> deficient?
>
Sure. In addition to not being able to find out when the scan is completed..
- There is no way to specify what channels to scan.
- There is no way to specify a passive scan.
- The probe frame that is used is fixed with the exception of the SSID.
- There is no corresponding interface for the userspace MLME.
This isn't very important, however. I am more interested in what problems in
software scanning need to be solved by moving to hardware scanning.
> I'm fine with us overhauling how hw_scan works and integrates with the
> stack, but an all out ban on hardware scan offload just doesn't make sense.
> The host can do all RC4 and AES encrypt/decrypt too but certainly we would
> prefer the hardware to do that for us, right?
>
The host can do TCP/IP too but certainly we would prefer the hardware to do
that for us, right?
Well, referring to TOE is unfair, but it makes the point that only some things
make sense in hardware. Softmac is the result of removing the things that
don't.
> In the short term, I would rather we leave hw_scan in the code and have the
> users that currently rely on hw_scan just have to manually configure the AP
> selection until such time as the in-kernel-AP-selection-policy code works
> with hw offloaded scan.
>
What incentive does that give to fix the code? Leaving broken things in for
the sake of out-of-tree drivers does not appeal to me.
-Michael Wu
On Friday 27 April 2007 02:54, James Ketrenos wrote:
> hw scan: 2.2s
> sw scan: 4.7s
>
Alright, so I put together a patch to optimize software scanning a little and
tune the delays to be a bit closer to what the iwlwifi firmware uses. I don't
actually have a ipw3945 card, so I asked Andy Green to test it. Here are his
results:
options iwlwifi disable_hw_scan=0 debug=0x800
real 0m2.974s
real 0m2.855s
real 0m2.853s
real 0m2.753s
options iwlwifi disable_hw_scan=1 debug=0x800
real 0m2.754s
real 0m2.753s
real 0m2.757s
real 0m2.754s
The hw scans settle around 2.753s so there's basically no difference in speed
between sw and hw scan for this particular case. This is with 11 active, 2
passive 2.4ghz channels and 4 active, 16 passive 5.4ghz channels. For people
not in Europe, software scanning should be roughly 20ms slower due to some
(fixable) dumbness in the ieee80211_sta scanning code.
Patch containing the optimizations and delay adjustments attached.
IEEE80211_HW_NO_PROBE_FILTER also needs to be set in dev->flags (or
the "[PATCH 10/13] mac80211: set bssid to broadcast before scan" patch backed
out) in order for software scanning to work with iwlwifi.
-Michael Wu