* [PATCH 01/13] mac80211: Add radiotap support
@ 2007-04-23 18:48 Michael Wu
2007-04-23 18:48 ` [PATCH 02/13] sync with radiotap header in wireless-2.6 Michael Wu
` (13 more replies)
0 siblings, 14 replies; 71+ messages in thread
From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw)
To: Jiri Benc; +Cc: linux-wireless, John Linville
From: Michael Wu <flamingice@sourmilk.net>
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 <andy@warmcat.com> for testing earlier versions of
this patch.
Signed-off-by: Michael Wu <flamingice@sourmilk.net>
---
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",
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH 02/13] sync with radiotap header in wireless-2.6 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-23 18:48 ` [PATCH 03/13] mac80211: fix virtual interface related locking Michael Wu ` (12 subsequent siblings) 13 siblings, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> Update the radiotap header with what's in wireless-2.6. Also remove the comment about IEEE80211_RADIOTAP_FCS. Signed-off-by: Michael Wu <flamingice@sourmilk.net> --- 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') ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 03/13] mac80211: fix virtual interface related locking 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu 2007-04-23 18:48 ` [PATCH 02/13] sync with radiotap header in wireless-2.6 Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-23 20:41 ` Jiri Benc 2007-04-23 20:58 ` Andy Green 2007-04-23 18:48 ` [PATCH 05/13] mac80211: remove statistics callback for master device Michael Wu ` (11 subsequent siblings) 13 siblings, 2 replies; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- 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); ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 03/13] mac80211: fix virtual interface related locking 2007-04-23 18:48 ` [PATCH 03/13] mac80211: fix virtual interface related locking Michael Wu @ 2007-04-23 20:41 ` Jiri Benc 2007-04-23 20:55 ` Michael Wu 2007-04-23 20:58 ` Andy Green 1 sibling, 1 reply; 71+ messages in thread From: Jiri Benc @ 2007-04-23 20:41 UTC (permalink / raw) To: Michael Wu; +Cc: linux-wireless, John Linville 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 03/13] mac80211: fix virtual interface related locking 2007-04-23 20:41 ` Jiri Benc @ 2007-04-23 20:55 ` Michael Wu 2007-04-23 22:20 ` Michael Wu 0 siblings, 1 reply; 71+ messages in thread From: Michael Wu @ 2007-04-23 20:55 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville [-- Attachment #1: Type: text/plain, Size: 5178 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 03/13] mac80211: fix virtual interface related locking 2007-04-23 20:55 ` Michael Wu @ 2007-04-23 22:20 ` Michael Wu 0 siblings, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-04-23 22:20 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville [-- Attachment #1.1: Type: text/plain, Size: 598 bytes --] 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 [-- Attachment #1.2: 06-fix-subif-locking.diff --] [-- Type: text/x-diff, Size: 24774 bytes --] mac80211: fix virtual interface related locking From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- net/mac80211/ieee80211.c | 50 +++++++++++++++++++-------- net/mac80211/ieee80211_cfg.c | 15 +++----- net/mac80211/ieee80211_i.h | 7 +--- net/mac80211/ieee80211_iface.c | 75 +++++++++++++++++----------------------- net/mac80211/ieee80211_ioctl.c | 58 +++++++++++++++---------------- net/mac80211/ieee80211_sta.c | 24 +++---------- 6 files changed, 109 insertions(+), 120 deletions(-) diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c index 51ad624..934ad38 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,7 +2224,7 @@ 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); @@ -2269,6 +2273,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 +2281,7 @@ static int ieee80211_master_open(struct net_device *dev) break; } } + read_unlock(&local->sub_if_lock); return res; } @@ -2285,10 +2291,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 +2356,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 +3885,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 +3921,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 +3978,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 +4123,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 / @@ -4665,7 +4682,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 +4725,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 +4779,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 +4846,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 +4860,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..91bc633 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; } @@ -1097,6 +1102,7 @@ static int ieee80211_ioctl_update_if(struct net_device *dev, 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) { @@ -1104,6 +1110,7 @@ static int ieee80211_ioctl_update_if(struct net_device *dev, break; } } + read_unlock(&local->sub_if_lock); if (!wds_dev || sdata->type != IEEE80211_IF_TYPE_WDS) return -ENODEV; @@ -1115,14 +1122,6 @@ static int ieee80211_ioctl_update_if(struct net_device *dev, } -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 +1511,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 +2235,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 +2253,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 +2386,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 +2395,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 +2406,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,19 +2415,19 @@ 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; } i++; } + read_unlock(&local->sub_if_lock); + if (i <= 1) ieee80211_ioctl_default_wep_only(local, 1); else ieee80211_ioctl_default_wep_only(local, 0); - - spin_unlock_bh(&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); [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 03/13] mac80211: fix virtual interface related locking 2007-04-23 18:48 ` [PATCH 03/13] mac80211: fix virtual interface related locking Michael Wu 2007-04-23 20:41 ` Jiri Benc @ 2007-04-23 20:58 ` Andy Green 2007-04-23 21:21 ` Michael Wu 1 sibling, 1 reply; 71+ messages in thread From: Andy Green @ 2007-04-23 20:58 UTC (permalink / raw) To: Michael Wu; +Cc: linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 03/13] mac80211: fix virtual interface related locking 2007-04-23 20:58 ` Andy Green @ 2007-04-23 21:21 ` Michael Wu 2007-04-24 18:09 ` Andy Green 0 siblings, 1 reply; 71+ messages in thread From: Michael Wu @ 2007-04-23 21:21 UTC (permalink / raw) To: Andy Green; +Cc: linux-wireless [-- Attachment #1: Type: text/plain, Size: 412 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 03/13] mac80211: fix virtual interface related locking 2007-04-23 21:21 ` Michael Wu @ 2007-04-24 18:09 ` Andy Green 2007-04-24 18:24 ` Michael Wu 0 siblings, 1 reply; 71+ messages in thread From: Andy Green @ 2007-04-24 18:09 UTC (permalink / raw) To: Michael Wu; +Cc: linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 03/13] mac80211: fix virtual interface related locking 2007-04-24 18:09 ` Andy Green @ 2007-04-24 18:24 ` Michael Wu 2007-04-24 18:59 ` John W. Linville 2007-04-25 12:09 ` Johannes Berg 0 siblings, 2 replies; 71+ messages in thread From: Michael Wu @ 2007-04-24 18:24 UTC (permalink / raw) To: Andy Green; +Cc: linux-wireless [-- Attachment #1: Type: text/plain, Size: 771 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 03/13] mac80211: fix virtual interface related locking 2007-04-24 18:24 ` Michael Wu @ 2007-04-24 18:59 ` John W. Linville 2007-04-25 12:09 ` Johannes Berg 1 sibling, 0 replies; 71+ messages in thread From: John W. Linville @ 2007-04-24 18:59 UTC (permalink / raw) To: Michael Wu; +Cc: Andy Green, linux-wireless 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 linville@tuxdriver.com ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 03/13] mac80211: fix virtual interface related locking 2007-04-24 18:24 ` Michael Wu 2007-04-24 18:59 ` John W. Linville @ 2007-04-25 12:09 ` Johannes Berg 1 sibling, 0 replies; 71+ messages in thread From: Johannes Berg @ 2007-04-25 12:09 UTC (permalink / raw) To: Michael Wu; +Cc: Andy Green, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 05/13] mac80211: remove statistics callback for master device 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu 2007-04-23 18:48 ` [PATCH 02/13] sync with radiotap header in wireless-2.6 Michael Wu 2007-04-23 18:48 ` [PATCH 03/13] mac80211: fix virtual interface related locking Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-23 18:48 ` [PATCH 04/13] mac80211: disable tasklets on close Michael Wu ` (10 subsequent siblings) 13 siblings, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> Statistics were never updated properly for the master device, so don't bother reporting them. Signed-off-by: Michael Wu <flamingice@sourmilk.net> --- 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; ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 04/13] mac80211: disable tasklets on close 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (2 preceding siblings ...) 2007-04-23 18:48 ` [PATCH 05/13] mac80211: remove statistics callback for master device Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-23 20:53 ` Jiri Benc 2007-04-23 18:48 ` [PATCH 07/13] mac80211: fix configuration concurrency issues in ieee80211_sta.c Michael Wu ` (9 subsequent siblings) 13 siblings, 1 reply; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> This prevents all tasklets from running when the device is down. Signed-off-by: Michael Wu <flamingice@sourmilk.net> --- 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); ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 04/13] mac80211: disable tasklets on close 2007-04-23 18:48 ` [PATCH 04/13] mac80211: disable tasklets on close Michael Wu @ 2007-04-23 20:53 ` Jiri Benc 0 siblings, 0 replies; 71+ messages in thread From: Jiri Benc @ 2007-04-23 20:53 UTC (permalink / raw) To: Michael Wu; +Cc: linux-wireless, John Linville 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 07/13] mac80211: fix configuration concurrency issues in ieee80211_sta.c 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (3 preceding siblings ...) 2007-04-23 18:48 ` [PATCH 04/13] mac80211: disable tasklets on close Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-24 16:19 ` Johannes Berg 2007-04-23 18:48 ` [PATCH 06/13] mac80211: avoid flush_scheduled_work Michael Wu ` (8 subsequent siblings) 13 siblings, 1 reply; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- 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; } ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 07/13] mac80211: fix configuration concurrency issues in ieee80211_sta.c 2007-04-23 18:48 ` [PATCH 07/13] mac80211: fix configuration concurrency issues in ieee80211_sta.c Michael Wu @ 2007-04-24 16:19 ` Johannes Berg 0 siblings, 0 replies; 71+ messages in thread From: Johannes Berg @ 2007-04-24 16:19 UTC (permalink / raw) To: Michael Wu; +Cc: Jiri Benc, linux-wireless, John Linville [-- Attachment #1: Type: text/plain, Size: 435 bytes --] > 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 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 190 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 06/13] mac80211: avoid flush_scheduled_work 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (4 preceding siblings ...) 2007-04-23 18:48 ` [PATCH 07/13] mac80211: fix configuration concurrency issues in ieee80211_sta.c Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-23 18:48 ` [PATCH 08/13] mac80211: misc cleanups in ieee80211_sta.c Michael Wu ` (7 subsequent siblings) 13 siblings, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- 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); } ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 08/13] mac80211: misc cleanups in ieee80211_sta.c 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (5 preceding siblings ...) 2007-04-23 18:48 ` [PATCH 06/13] mac80211: avoid flush_scheduled_work Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-23 18:48 ` [PATCH 09/13] mac80211: remove hw_scan callback Michael Wu ` (6 subsequent siblings) 13 siblings, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> This eliminates some unnecessary code in ieee80211_sta.c. Signed-off-by: Michael Wu <flamingice@sourmilk.net> --- 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 ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (6 preceding siblings ...) 2007-04-23 18:48 ` [PATCH 08/13] mac80211: misc cleanups in ieee80211_sta.c Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-24 16:20 ` Johannes Berg ` (2 more replies) 2007-04-23 18:48 ` [PATCH 11/13] mac80211: fix issues in ieee80211 qdisc Michael Wu ` (5 subsequent siblings) 13 siblings, 3 replies; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- 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); ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-23 18:48 ` [PATCH 09/13] mac80211: remove hw_scan callback Michael Wu @ 2007-04-24 16:20 ` Johannes Berg 2007-04-26 12:48 ` Michael Wu 2007-04-25 5:03 ` James Ketrenos 2007-04-27 20:47 ` James Ketrenos 2 siblings, 1 reply; 71+ messages in thread From: Johannes Berg @ 2007-04-24 16:20 UTC (permalink / raw) To: Michael Wu; +Cc: Jiri Benc, linux-wireless, John Linville [-- Attachment #1: Type: text/plain, Size: 431 bytes --] 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 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 190 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-24 16:20 ` Johannes Berg @ 2007-04-26 12:48 ` Michael Wu 0 siblings, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-04-26 12:48 UTC (permalink / raw) To: Johannes Berg; +Cc: Jiri Benc, linux-wireless, John Linville [-- Attachment #1: Type: text/plain, Size: 169 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-23 18:48 ` [PATCH 09/13] mac80211: remove hw_scan callback Michael Wu 2007-04-24 16:20 ` Johannes Berg @ 2007-04-25 5:03 ` James Ketrenos 2007-04-25 18:16 ` John W. Linville 2007-04-27 20:47 ` James Ketrenos 2 siblings, 1 reply; 71+ messages in thread From: James Ketrenos @ 2007-04-25 5:03 UTC (permalink / raw) To: Michael Wu; +Cc: Jiri Benc, linux-wireless, John Linville Michael Wu wrote: > From: Michael Wu <flamingice@sourmilk.net> > > 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-25 5:03 ` James Ketrenos @ 2007-04-25 18:16 ` John W. Linville 2007-04-25 20:34 ` Michael Wu 2007-04-26 3:03 ` James Ketrenos 0 siblings, 2 replies; 71+ messages in thread From: John W. Linville @ 2007-04-25 18:16 UTC (permalink / raw) To: James Ketrenos; +Cc: Michael Wu, Jiri Benc, linux-wireless 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 linville@tuxdriver.com ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-25 18:16 ` John W. Linville @ 2007-04-25 20:34 ` Michael Wu 2007-04-26 21:57 ` James Ketrenos 2007-04-26 3:03 ` James Ketrenos 1 sibling, 1 reply; 71+ messages in thread From: Michael Wu @ 2007-04-25 20:34 UTC (permalink / raw) To: John W. Linville; +Cc: James Ketrenos, Jiri Benc, linux-wireless [-- Attachment #1: Type: text/plain, Size: 1843 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-25 20:34 ` Michael Wu @ 2007-04-26 21:57 ` James Ketrenos 2007-04-27 0:23 ` Michael Wu 0 siblings, 1 reply; 71+ messages in thread From: James Ketrenos @ 2007-04-26 21:57 UTC (permalink / raw) To: Michael Wu; +Cc: John W. Linville, Jiri Benc, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-26 21:57 ` James Ketrenos @ 2007-04-27 0:23 ` Michael Wu 2007-04-27 4:14 ` James Ketrenos ` (2 more replies) 0 siblings, 3 replies; 71+ messages in thread From: Michael Wu @ 2007-04-27 0:23 UTC (permalink / raw) To: James Ketrenos; +Cc: John W. Linville, Jiri Benc, linux-wireless [-- Attachment #1: Type: text/plain, Size: 3527 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 0:23 ` Michael Wu @ 2007-04-27 4:14 ` James Ketrenos 2007-04-27 7:44 ` Andy Green ` (2 more replies) 2007-04-27 6:54 ` James Ketrenos 2007-04-27 14:28 ` Dan Williams 2 siblings, 3 replies; 71+ messages in thread From: James Ketrenos @ 2007-04-27 4:14 UTC (permalink / raw) To: Michael Wu; +Cc: John W. Linville, Jiri Benc, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 4:14 ` James Ketrenos @ 2007-04-27 7:44 ` Andy Green 2007-04-27 8:06 ` James Ketrenos 2007-04-27 9:00 ` Jiri Benc 2007-04-27 15:32 ` Michael Wu 2 siblings, 1 reply; 71+ messages in thread From: Andy Green @ 2007-04-27 7:44 UTC (permalink / raw) To: James Ketrenos; +Cc: Michael Wu, John W. Linville, Jiri Benc, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 7:44 ` Andy Green @ 2007-04-27 8:06 ` James Ketrenos 2007-04-27 8:54 ` Andy Green 0 siblings, 1 reply; 71+ messages in thread From: James Ketrenos @ 2007-04-27 8:06 UTC (permalink / raw) To: Andy Green; +Cc: Michael Wu, John W. Linville, Jiri Benc, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 8:06 ` James Ketrenos @ 2007-04-27 8:54 ` Andy Green 0 siblings, 0 replies; 71+ messages in thread From: Andy Green @ 2007-04-27 8:54 UTC (permalink / raw) To: James Ketrenos; +Cc: Michael Wu, John W. Linville, Jiri Benc, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 4:14 ` James Ketrenos 2007-04-27 7:44 ` Andy Green @ 2007-04-27 9:00 ` Jiri Benc 2007-04-27 15:32 ` Michael Wu 2 siblings, 0 replies; 71+ messages in thread From: Jiri Benc @ 2007-04-27 9:00 UTC (permalink / raw) To: James Ketrenos; +Cc: Michael Wu, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 4:14 ` James Ketrenos 2007-04-27 7:44 ` Andy Green 2007-04-27 9:00 ` Jiri Benc @ 2007-04-27 15:32 ` Michael Wu 2007-04-29 11:55 ` Guy Cohen 2 siblings, 1 reply; 71+ messages in thread From: Michael Wu @ 2007-04-27 15:32 UTC (permalink / raw) To: James Ketrenos; +Cc: John W. Linville, Jiri Benc, linux-wireless [-- Attachment #1: Type: text/plain, Size: 2038 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:32 ` Michael Wu @ 2007-04-29 11:55 ` Guy Cohen 0 siblings, 0 replies; 71+ messages in thread From: Guy Cohen @ 2007-04-29 11:55 UTC (permalink / raw) To: Michael Wu; +Cc: James Ketrenos, John W. Linville, Jiri Benc, linux-wireless 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 <flamingice@sourmilk.net> 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 > > ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 0:23 ` Michael Wu 2007-04-27 4:14 ` James Ketrenos @ 2007-04-27 6:54 ` James Ketrenos 2007-04-27 14:27 ` Michael Wu 2007-05-08 17:08 ` Michael Wu 2007-04-27 14:28 ` Dan Williams 2 siblings, 2 replies; 71+ messages in thread From: James Ketrenos @ 2007-04-27 6:54 UTC (permalink / raw) To: Michael Wu; +Cc: John W. Linville, Jiri Benc, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 6:54 ` James Ketrenos @ 2007-04-27 14:27 ` Michael Wu 2007-05-08 17:08 ` Michael Wu 1 sibling, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-04-27 14:27 UTC (permalink / raw) To: James Ketrenos; +Cc: John W. Linville, Jiri Benc, linux-wireless [-- Attachment #1: Type: text/plain, Size: 1262 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 6:54 ` James Ketrenos 2007-04-27 14:27 ` Michael Wu @ 2007-05-08 17:08 ` Michael Wu 1 sibling, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-05-08 17:08 UTC (permalink / raw) To: James Ketrenos; +Cc: John W. Linville, Jiri Benc, linux-wireless, Andy Green [-- Attachment #1.1: Type: text/plain, Size: 1254 bytes --] 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 [-- Attachment #1.2: 26-optimize-scanning.diff --] [-- Type: text/x-diff, Size: 1657 bytes --] mac80211: optimize scanning code From: Michael Wu <flamingice@sourmilk.net> --- net/mac80211/ieee80211_sta.c | 13 +++++++------ 1 files changed, 7 insertions(+), 6 deletions(-) diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c index d0a3aa0..f3017a1 100644 --- a/net/mac80211/ieee80211_sta.c +++ b/net/mac80211/ieee80211_sta.c @@ -45,9 +45,8 @@ #define IEEE80211_SCAN_INTERVAL_SLOW (15 * HZ) #define IEEE80211_IBSS_JOIN_TIMEOUT (20 * HZ) -#define IEEE80211_PROBE_DELAY (HZ / 33) -#define IEEE80211_CHANNEL_TIME (HZ / 33) -#define IEEE80211_PASSIVE_CHANNEL_TIME (HZ / 5) +#define IEEE80211_CHANNEL_TIME (HZ / 50) +#define IEEE80211_PASSIVE_CHANNEL_TIME (HZ / 9) #define IEEE80211_SCAN_RESULT_EXPIRE (10 * HZ) #define IEEE80211_IBSS_MERGE_INTERVAL (30 * HZ) #define IEEE80211_IBSS_INACTIVITY_LIMIT (60 * HZ) @@ -2669,15 +2668,17 @@ void ieee80211_sta_scan_work(struct work_struct *work) if (skip) break; - next_delay = IEEE80211_PROBE_DELAY + - usecs_to_jiffies(local->hw.channel_change_time); + next_delay = usecs_to_jiffies(local->hw.channel_change_time); local->scan_state = SCAN_SEND_PROBE; - break; + if (local->hw.channel_change_time) + break; case SCAN_SEND_PROBE: if (local->scan_channel->flag & IEEE80211_CHAN_W_ACTIVE_SCAN) { ieee80211_send_probe_req(dev, NULL, local->scan_ssid, local->scan_ssid_len); next_delay = IEEE80211_CHANNEL_TIME; + if (local->scan_hw_mode->mode == MODE_IEEE80211A) + next_delay >>= 1; } else next_delay = IEEE80211_PASSIVE_CHANNEL_TIME; local->scan_state = SCAN_SET_CHANNEL; [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 0:23 ` Michael Wu 2007-04-27 4:14 ` James Ketrenos 2007-04-27 6:54 ` James Ketrenos @ 2007-04-27 14:28 ` Dan Williams 2007-04-27 14:42 ` Jiri Benc 2 siblings, 1 reply; 71+ messages in thread From: Dan Williams @ 2007-04-27 14:28 UTC (permalink / raw) To: Michael Wu; +Cc: James Ketrenos, John W. Linville, Jiri Benc, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 14:28 ` Dan Williams @ 2007-04-27 14:42 ` Jiri Benc 2007-04-27 14:56 ` Dan Williams 0 siblings, 1 reply; 71+ messages in thread From: Jiri Benc @ 2007-04-27 14:42 UTC (permalink / raw) To: Dan Williams; +Cc: Michael Wu, James Ketrenos, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 14:42 ` Jiri Benc @ 2007-04-27 14:56 ` Dan Williams 2007-04-27 15:16 ` Andy Green 2007-04-27 15:20 ` Jiri Benc 0 siblings, 2 replies; 71+ messages in thread From: Dan Williams @ 2007-04-27 14:56 UTC (permalink / raw) To: Jiri Benc; +Cc: Michael Wu, James Ketrenos, John W. Linville, linux-wireless 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 > ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 14:56 ` Dan Williams @ 2007-04-27 15:16 ` Andy Green 2007-04-27 15:22 ` Johannes Berg 2007-04-27 18:09 ` Dan Williams 2007-04-27 15:20 ` Jiri Benc 1 sibling, 2 replies; 71+ messages in thread From: Andy Green @ 2007-04-27 15:16 UTC (permalink / raw) To: Dan Williams Cc: Jiri Benc, Michael Wu, James Ketrenos, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:16 ` Andy Green @ 2007-04-27 15:22 ` Johannes Berg 2007-04-27 17:17 ` James Ketrenos 2007-04-27 17:49 ` Dan Williams 2007-04-27 18:09 ` Dan Williams 1 sibling, 2 replies; 71+ messages in thread From: Johannes Berg @ 2007-04-27 15:22 UTC (permalink / raw) To: Andy Green Cc: Dan Williams, Jiri Benc, Michael Wu, James Ketrenos, John W. Linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 857 bytes --] 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 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 190 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:22 ` Johannes Berg @ 2007-04-27 17:17 ` James Ketrenos 2007-04-27 17:49 ` Dan Williams 1 sibling, 0 replies; 71+ messages in thread From: James Ketrenos @ 2007-04-27 17:17 UTC (permalink / raw) To: Johannes Berg Cc: Andy Green, Dan Williams, Jiri Benc, Michael Wu, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:22 ` Johannes Berg 2007-04-27 17:17 ` James Ketrenos @ 2007-04-27 17:49 ` Dan Williams 1 sibling, 0 replies; 71+ messages in thread From: Dan Williams @ 2007-04-27 17:49 UTC (permalink / raw) To: Johannes Berg Cc: Andy Green, Jiri Benc, Michael Wu, James Ketrenos, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:16 ` Andy Green 2007-04-27 15:22 ` Johannes Berg @ 2007-04-27 18:09 ` Dan Williams 2007-04-27 18:52 ` Andy Green 1 sibling, 1 reply; 71+ messages in thread From: Dan Williams @ 2007-04-27 18:09 UTC (permalink / raw) To: Andy Green Cc: Jiri Benc, Michael Wu, James Ketrenos, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 18:09 ` Dan Williams @ 2007-04-27 18:52 ` Andy Green 0 siblings, 0 replies; 71+ messages in thread From: Andy Green @ 2007-04-27 18:52 UTC (permalink / raw) To: Dan Williams Cc: Jiri Benc, Michael Wu, James Ketrenos, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 14:56 ` Dan Williams 2007-04-27 15:16 ` Andy Green @ 2007-04-27 15:20 ` Jiri Benc 2007-04-27 15:30 ` Andy Green ` (2 more replies) 1 sibling, 3 replies; 71+ messages in thread From: Jiri Benc @ 2007-04-27 15:20 UTC (permalink / raw) To: Dan Williams; +Cc: Michael Wu, James Ketrenos, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:20 ` Jiri Benc @ 2007-04-27 15:30 ` Andy Green 2007-04-27 15:36 ` Jiri Benc 2007-04-27 17:02 ` James Ketrenos 2007-04-27 19:52 ` John W. Linville 2 siblings, 1 reply; 71+ messages in thread From: Andy Green @ 2007-04-27 15:30 UTC (permalink / raw) To: Jiri Benc Cc: Dan Williams, Michael Wu, James Ketrenos, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:30 ` Andy Green @ 2007-04-27 15:36 ` Jiri Benc 2007-04-27 15:52 ` Andy Green 2007-04-27 17:44 ` James Ketrenos 0 siblings, 2 replies; 71+ messages in thread From: Jiri Benc @ 2007-04-27 15:36 UTC (permalink / raw) To: Andy Green Cc: Dan Williams, Michael Wu, James Ketrenos, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:36 ` Jiri Benc @ 2007-04-27 15:52 ` Andy Green 2007-04-27 17:44 ` James Ketrenos 1 sibling, 0 replies; 71+ messages in thread From: Andy Green @ 2007-04-27 15:52 UTC (permalink / raw) To: Jiri Benc Cc: Dan Williams, Michael Wu, James Ketrenos, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:36 ` Jiri Benc 2007-04-27 15:52 ` Andy Green @ 2007-04-27 17:44 ` James Ketrenos 1 sibling, 0 replies; 71+ messages in thread From: James Ketrenos @ 2007-04-27 17:44 UTC (permalink / raw) To: Jiri Benc Cc: Andy Green, Dan Williams, Michael Wu, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:20 ` Jiri Benc 2007-04-27 15:30 ` Andy Green @ 2007-04-27 17:02 ` James Ketrenos 2007-04-27 18:10 ` Jiri Benc 2007-04-27 19:52 ` John W. Linville 2 siblings, 1 reply; 71+ messages in thread From: James Ketrenos @ 2007-04-27 17:02 UTC (permalink / raw) To: Jiri Benc; +Cc: Dan Williams, Michael Wu, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 17:02 ` James Ketrenos @ 2007-04-27 18:10 ` Jiri Benc 2007-04-27 19:42 ` Dan Williams 0 siblings, 1 reply; 71+ messages in thread From: Jiri Benc @ 2007-04-27 18:10 UTC (permalink / raw) To: James Ketrenos; +Cc: Dan Williams, Michael Wu, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 18:10 ` Jiri Benc @ 2007-04-27 19:42 ` Dan Williams 2007-04-27 19:47 ` Jiri Benc 0 siblings, 1 reply; 71+ messages in thread From: Dan Williams @ 2007-04-27 19:42 UTC (permalink / raw) To: Jiri Benc; +Cc: James Ketrenos, Michael Wu, John W. Linville, linux-wireless 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 > ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 19:42 ` Dan Williams @ 2007-04-27 19:47 ` Jiri Benc 0 siblings, 0 replies; 71+ messages in thread From: Jiri Benc @ 2007-04-27 19:47 UTC (permalink / raw) To: Dan Williams; +Cc: James Ketrenos, Michael Wu, John W. Linville, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 15:20 ` Jiri Benc 2007-04-27 15:30 ` Andy Green 2007-04-27 17:02 ` James Ketrenos @ 2007-04-27 19:52 ` John W. Linville 2 siblings, 0 replies; 71+ messages in thread From: John W. Linville @ 2007-04-27 19:52 UTC (permalink / raw) To: Jiri Benc; +Cc: Dan Williams, Michael Wu, James Ketrenos, linux-wireless 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 linville@tuxdriver.com ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-25 18:16 ` John W. Linville 2007-04-25 20:34 ` Michael Wu @ 2007-04-26 3:03 ` James Ketrenos 1 sibling, 0 replies; 71+ messages in thread From: James Ketrenos @ 2007-04-26 3:03 UTC (permalink / raw) To: John W. Linville; +Cc: Michael Wu, Jiri Benc, linux-wireless 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-23 18:48 ` [PATCH 09/13] mac80211: remove hw_scan callback Michael Wu 2007-04-24 16:20 ` Johannes Berg 2007-04-25 5:03 ` James Ketrenos @ 2007-04-27 20:47 ` James Ketrenos 2007-04-28 13:25 ` Jiri Benc 2 siblings, 1 reply; 71+ messages in thread From: James Ketrenos @ 2007-04-27 20:47 UTC (permalink / raw) To: Michael Wu; +Cc: Jiri Benc, linux-wireless, John Linville Michael Wu wrote: > From: Michael Wu <flamingice@sourmilk.net> > > 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 <jketreno@linux.intel.com> --- 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 ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 09/13] mac80211: remove hw_scan callback 2007-04-27 20:47 ` James Ketrenos @ 2007-04-28 13:25 ` Jiri Benc 0 siblings, 0 replies; 71+ messages in thread From: Jiri Benc @ 2007-04-28 13:25 UTC (permalink / raw) To: James Ketrenos; +Cc: Michael Wu, linux-wireless, John Linville 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 11/13] mac80211: fix issues in ieee80211 qdisc 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (7 preceding siblings ...) 2007-04-23 18:48 ` [PATCH 09/13] mac80211: remove hw_scan callback Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-23 18:48 ` [PATCH 10/13] mac80211: set bssid to broadcast before scan Michael Wu ` (4 subsequent siblings) 13 siblings, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- 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; ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 10/13] mac80211: set bssid to broadcast before scan 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (8 preceding siblings ...) 2007-04-23 18:48 ` [PATCH 11/13] mac80211: fix issues in ieee80211 qdisc Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-24 16:24 ` Johannes Berg 2007-04-23 18:48 ` [PATCH 12/13] mac80211: prevent master device from going up without ieee80211 qdisc Michael Wu ` (3 subsequent siblings) 13 siblings, 1 reply; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- 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); ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 10/13] mac80211: set bssid to broadcast before scan 2007-04-23 18:48 ` [PATCH 10/13] mac80211: set bssid to broadcast before scan Michael Wu @ 2007-04-24 16:24 ` Johannes Berg 2007-04-27 17:40 ` Jiri Benc 0 siblings, 1 reply; 71+ messages in thread From: Johannes Berg @ 2007-04-24 16:24 UTC (permalink / raw) To: Michael Wu; +Cc: Jiri Benc, linux-wireless, John Linville [-- Attachment #1: Type: text/plain, Size: 627 bytes --] 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 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 190 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 10/13] mac80211: set bssid to broadcast before scan 2007-04-24 16:24 ` Johannes Berg @ 2007-04-27 17:40 ` Jiri Benc 2007-04-27 19:49 ` Michael Wu 0 siblings, 1 reply; 71+ messages in thread From: Jiri Benc @ 2007-04-27 17:40 UTC (permalink / raw) To: Johannes Berg; +Cc: Michael Wu, linux-wireless, John Linville 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 10/13] mac80211: set bssid to broadcast before scan 2007-04-27 17:40 ` Jiri Benc @ 2007-04-27 19:49 ` Michael Wu 2007-04-27 21:18 ` Michael Wu 0 siblings, 1 reply; 71+ messages in thread From: Michael Wu @ 2007-04-27 19:49 UTC (permalink / raw) To: Jiri Benc; +Cc: Johannes Berg, linux-wireless, John Linville [-- Attachment #1.1: Type: text/plain, Size: 168 bytes --] 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 [-- Attachment #1.2: 11-reset-bssid-before-scan.diff --] [-- Type: text/x-diff, Size: 3270 bytes --] mac80211: set bssid to broadcast before scan From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- include/net/mac80211.h | 4 ++++ net/mac80211/ieee80211.c | 7 ++++++- net/mac80211/ieee80211_sta.c | 11 +++++++++++ 3 files changed, 21 insertions(+), 1 deletions(-) diff --git a/include/net/mac80211.h b/include/net/mac80211.h index cade956..2da0d31 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -524,6 +524,10 @@ struct ieee80211_hw { * normal operation. */ #define IEEE80211_HW_MONITOR_DURING_OPER (1<<9) + /* Device does not need BSSID filter set to broadcast in order to + * receive all probe responses while scanning */ +#define IEEE80211_HW_NO_PROBE_FILTERING (1<<10) + /* please fill this gap when adding new flags */ /* calculate Michael MIC for an MSDU when doing hwcrypto */ diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c index f6a71ce..4976493 100644 --- a/net/mac80211/ieee80211.c +++ b/net/mac80211/ieee80211.c @@ -2015,6 +2015,7 @@ static int __ieee80211_if_config(struct net_device *dev, struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev); struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr); struct ieee80211_if_conf conf; + static const u8 scan_bssid[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; if (!local->ops->config_interface || !netif_running(dev)) return 0; @@ -2023,7 +2024,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 = 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_sta.c b/net/mac80211/ieee80211_sta.c index 7cb2dd3..d996412 100644 --- a/net/mac80211/ieee80211_sta.c +++ b/net/mac80211/ieee80211_sta.c @@ -2577,6 +2577,11 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw) printk(KERN_DEBUG "%s: failed to restore operational" "channel after scan\n", dev->name); + if (local->hw.flags & IEEE80211_HW_NO_PROBE_FILTERING || + 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); @@ -2736,6 +2741,12 @@ static int ieee80211_sta_start_scan(struct net_device *dev, list); local->scan_channel_idx = 0; local->scan_dev = dev; + + if (local->hw.flags & IEEE80211_HW_NO_PROBE_FILTERING || + 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); [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 10/13] mac80211: set bssid to broadcast before scan 2007-04-27 19:49 ` Michael Wu @ 2007-04-27 21:18 ` Michael Wu 2007-04-27 21:29 ` Michael Wu 0 siblings, 1 reply; 71+ messages in thread From: Michael Wu @ 2007-04-27 21:18 UTC (permalink / raw) To: Jiri Benc; +Cc: Johannes Berg, linux-wireless, John Linville [-- Attachment #1.1: Type: text/plain, Size: 289 bytes --] 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 [-- Attachment #1.2: 11-reset-bssid-before-scan.diff --] [-- Type: text/x-diff, Size: 3278 bytes --] mac80211: set bssid to broadcast before scan From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- include/net/mac80211.h | 4 ++++ net/mac80211/ieee80211.c | 7 ++++++- net/mac80211/ieee80211_sta.c | 11 +++++++++++ 3 files changed, 21 insertions(+), 1 deletions(-) diff --git a/include/net/mac80211.h b/include/net/mac80211.h index cade956..2da0d31 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -524,6 +524,10 @@ struct ieee80211_hw { * normal operation. */ #define IEEE80211_HW_MONITOR_DURING_OPER (1<<9) + /* Device does not need BSSID filter set to broadcast in order to + * receive all probe responses while scanning */ +#define IEEE80211_HW_NO_PROBE_FILTERING (1<<10) + /* please fill this gap when adding new flags */ /* calculate Michael MIC for an MSDU when doing hwcrypto */ diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c index f6a71ce..4976493 100644 --- a/net/mac80211/ieee80211.c +++ b/net/mac80211/ieee80211.c @@ -2015,6 +2015,7 @@ static int __ieee80211_if_config(struct net_device *dev, struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev); struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr); struct ieee80211_if_conf conf; + static const u8 scan_bssid[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; if (!local->ops->config_interface || !netif_running(dev)) return 0; @@ -2023,7 +2024,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 = 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_sta.c b/net/mac80211/ieee80211_sta.c index 7cb2dd3..7b12d7d 100644 --- a/net/mac80211/ieee80211_sta.c +++ b/net/mac80211/ieee80211_sta.c @@ -2577,6 +2577,11 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw) printk(KERN_DEBUG "%s: failed to restore operational" "channel after scan\n", dev->name); + if (!(local->hw.flags & IEEE80211_HW_NO_PROBE_FILTERING)) + 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); @@ -2736,6 +2741,12 @@ static int ieee80211_sta_start_scan(struct net_device *dev, list); local->scan_channel_idx = 0; local->scan_dev = dev; + + if (!(local->hw.flags & IEEE80211_HW_NO_PROBE_FILTERING)) + 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); [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 10/13] mac80211: set bssid to broadcast before scan 2007-04-27 21:18 ` Michael Wu @ 2007-04-27 21:29 ` Michael Wu 0 siblings, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-04-27 21:29 UTC (permalink / raw) To: Jiri Benc; +Cc: Johannes Berg, linux-wireless, John Linville [-- Attachment #1.1: Type: text/plain, Size: 430 bytes --] 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 [-- Attachment #1.2: 11-reset-bssid-before-scan.diff --] [-- Type: text/x-diff, Size: 3276 bytes --] mac80211: set bssid to broadcast before scan From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- include/net/mac80211.h | 4 ++++ net/mac80211/ieee80211.c | 7 ++++++- net/mac80211/ieee80211_sta.c | 11 +++++++++++ 3 files changed, 21 insertions(+), 1 deletions(-) diff --git a/include/net/mac80211.h b/include/net/mac80211.h index cade956..2da0d31 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -524,6 +524,10 @@ struct ieee80211_hw { * normal operation. */ #define IEEE80211_HW_MONITOR_DURING_OPER (1<<9) + /* Device does not need BSSID filter set to broadcast in order to + * receive all probe responses while scanning */ +#define IEEE80211_HW_NO_PROBE_FILTERING (1<<10) + /* please fill this gap when adding new flags */ /* calculate Michael MIC for an MSDU when doing hwcrypto */ diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c index f6a71ce..4976493 100644 --- a/net/mac80211/ieee80211.c +++ b/net/mac80211/ieee80211.c @@ -2015,6 +2015,7 @@ static int __ieee80211_if_config(struct net_device *dev, struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev); struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr); struct ieee80211_if_conf conf; + static const u8 scan_bssid[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; if (!local->ops->config_interface || !netif_running(dev)) return 0; @@ -2023,7 +2024,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 = 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_sta.c b/net/mac80211/ieee80211_sta.c index 7cb2dd3..d1376f1 100644 --- a/net/mac80211/ieee80211_sta.c +++ b/net/mac80211/ieee80211_sta.c @@ -2577,6 +2577,11 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw) printk(KERN_DEBUG "%s: failed to restore operational" "channel after scan\n", dev->name); + if (!(local->hw.flags & IEEE80211_HW_NO_PROBE_FILTERING) && + 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); @@ -2736,6 +2741,12 @@ static int ieee80211_sta_start_scan(struct net_device *dev, list); local->scan_channel_idx = 0; local->scan_dev = dev; + + if (!(local->hw.flags & IEEE80211_HW_NO_PROBE_FILTERING) && + 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); [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 12/13] mac80211: prevent master device from going up without ieee80211 qdisc 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (9 preceding siblings ...) 2007-04-23 18:48 ` [PATCH 10/13] mac80211: set bssid to broadcast before scan Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-23 18:48 ` [PATCH 13/13] mac80211: stop all virtual interfaces when master device goes down Michael Wu ` (2 subsequent siblings) 13 siblings, 0 replies; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> This patch ensures that the master device cannot be opened without the ieee80211 qdisc installed. Signed-off-by: Michael Wu <flamingice@sourmilk.net> --- 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); ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 13/13] mac80211: stop all virtual interfaces when master device goes down 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (10 preceding siblings ...) 2007-04-23 18:48 ` [PATCH 12/13] mac80211: prevent master device from going up without ieee80211 qdisc Michael Wu @ 2007-04-23 18:48 ` Michael Wu 2007-04-23 20:58 ` Jiri Benc 2007-04-24 16:16 ` [PATCH 01/13] mac80211: Add radiotap support Johannes Berg 2007-04-28 13:18 ` Jiri Benc 13 siblings, 1 reply; 71+ messages in thread From: Michael Wu @ 2007-04-23 18:48 UTC (permalink / raw) To: Jiri Benc; +Cc: linux-wireless, John Linville From: Michael Wu <flamingice@sourmilk.net> 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 <flamingice@sourmilk.net> --- 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) ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 13/13] mac80211: stop all virtual interfaces when master device goes down 2007-04-23 18:48 ` [PATCH 13/13] mac80211: stop all virtual interfaces when master device goes down Michael Wu @ 2007-04-23 20:58 ` Jiri Benc 0 siblings, 0 replies; 71+ messages in thread From: Jiri Benc @ 2007-04-23 20:58 UTC (permalink / raw) To: Michael Wu; +Cc: linux-wireless, John Linville 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 01/13] mac80211: Add radiotap support 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (11 preceding siblings ...) 2007-04-23 18:48 ` [PATCH 13/13] mac80211: stop all virtual interfaces when master device goes down Michael Wu @ 2007-04-24 16:16 ` Johannes Berg 2007-04-28 13:18 ` Jiri Benc 13 siblings, 0 replies; 71+ messages in thread From: Johannes Berg @ 2007-04-24 16:16 UTC (permalink / raw) To: Michael Wu; +Cc: Jiri Benc, linux-wireless, John Linville [-- Attachment #1: Type: text/plain, Size: 619 bytes --] > -#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 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 190 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 01/13] mac80211: Add radiotap support 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu ` (12 preceding siblings ...) 2007-04-24 16:16 ` [PATCH 01/13] mac80211: Add radiotap support Johannes Berg @ 2007-04-28 13:18 ` Jiri Benc 13 siblings, 0 replies; 71+ messages in thread From: Jiri Benc @ 2007-04-28 13:18 UTC (permalink / raw) To: Michael Wu; +Cc: linux-wireless, John Linville Patches 1, 3-8, 10-13 applied to my tree. Thanks! Jiri -- Jiri Benc SUSE Labs ^ permalink raw reply [flat|nested] 71+ messages in thread
end of thread, other threads:[~2007-05-08 17:09 UTC | newest] Thread overview: 71+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-04-23 18:48 [PATCH 01/13] mac80211: Add radiotap support Michael Wu 2007-04-23 18:48 ` [PATCH 02/13] sync with radiotap header in wireless-2.6 Michael Wu 2007-04-23 18:48 ` [PATCH 03/13] mac80211: fix virtual interface related locking Michael Wu 2007-04-23 20:41 ` Jiri Benc 2007-04-23 20:55 ` Michael Wu 2007-04-23 22:20 ` Michael Wu 2007-04-23 20:58 ` Andy Green 2007-04-23 21:21 ` Michael Wu 2007-04-24 18:09 ` Andy Green 2007-04-24 18:24 ` Michael Wu 2007-04-24 18:59 ` John W. Linville 2007-04-25 12:09 ` Johannes Berg 2007-04-23 18:48 ` [PATCH 05/13] mac80211: remove statistics callback for master device Michael Wu 2007-04-23 18:48 ` [PATCH 04/13] mac80211: disable tasklets on close Michael Wu 2007-04-23 20:53 ` Jiri Benc 2007-04-23 18:48 ` [PATCH 07/13] mac80211: fix configuration concurrency issues in ieee80211_sta.c Michael Wu 2007-04-24 16:19 ` Johannes Berg 2007-04-23 18:48 ` [PATCH 06/13] mac80211: avoid flush_scheduled_work Michael Wu 2007-04-23 18:48 ` [PATCH 08/13] mac80211: misc cleanups in ieee80211_sta.c Michael Wu 2007-04-23 18:48 ` [PATCH 09/13] mac80211: remove hw_scan callback Michael Wu 2007-04-24 16:20 ` Johannes Berg 2007-04-26 12:48 ` Michael Wu 2007-04-25 5:03 ` James Ketrenos 2007-04-25 18:16 ` John W. Linville 2007-04-25 20:34 ` Michael Wu 2007-04-26 21:57 ` James Ketrenos 2007-04-27 0:23 ` Michael Wu 2007-04-27 4:14 ` James Ketrenos 2007-04-27 7:44 ` Andy Green 2007-04-27 8:06 ` James Ketrenos 2007-04-27 8:54 ` Andy Green 2007-04-27 9:00 ` Jiri Benc 2007-04-27 15:32 ` Michael Wu 2007-04-29 11:55 ` Guy Cohen 2007-04-27 6:54 ` James Ketrenos 2007-04-27 14:27 ` Michael Wu 2007-05-08 17:08 ` Michael Wu 2007-04-27 14:28 ` Dan Williams 2007-04-27 14:42 ` Jiri Benc 2007-04-27 14:56 ` Dan Williams 2007-04-27 15:16 ` Andy Green 2007-04-27 15:22 ` Johannes Berg 2007-04-27 17:17 ` James Ketrenos 2007-04-27 17:49 ` Dan Williams 2007-04-27 18:09 ` Dan Williams 2007-04-27 18:52 ` Andy Green 2007-04-27 15:20 ` Jiri Benc 2007-04-27 15:30 ` Andy Green 2007-04-27 15:36 ` Jiri Benc 2007-04-27 15:52 ` Andy Green 2007-04-27 17:44 ` James Ketrenos 2007-04-27 17:02 ` James Ketrenos 2007-04-27 18:10 ` Jiri Benc 2007-04-27 19:42 ` Dan Williams 2007-04-27 19:47 ` Jiri Benc 2007-04-27 19:52 ` John W. Linville 2007-04-26 3:03 ` James Ketrenos 2007-04-27 20:47 ` James Ketrenos 2007-04-28 13:25 ` Jiri Benc 2007-04-23 18:48 ` [PATCH 11/13] mac80211: fix issues in ieee80211 qdisc Michael Wu 2007-04-23 18:48 ` [PATCH 10/13] mac80211: set bssid to broadcast before scan Michael Wu 2007-04-24 16:24 ` Johannes Berg 2007-04-27 17:40 ` Jiri Benc 2007-04-27 19:49 ` Michael Wu 2007-04-27 21:18 ` Michael Wu 2007-04-27 21:29 ` Michael Wu 2007-04-23 18:48 ` [PATCH 12/13] mac80211: prevent master device from going up without ieee80211 qdisc Michael Wu 2007-04-23 18:48 ` [PATCH 13/13] mac80211: stop all virtual interfaces when master device goes down Michael Wu 2007-04-23 20:58 ` Jiri Benc 2007-04-24 16:16 ` [PATCH 01/13] mac80211: Add radiotap support Johannes Berg 2007-04-28 13:18 ` Jiri Benc
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).