RE: [RFC/PATCH 6/13] d80211: remove obsolete stuff
Yes, fully agreed - and the hardware's pre-beacon interrupt would cause the beacon function to create a beacon frame and put it into the queue (dev_queue_xmit on the master device). The beacon frame would the be passed to the hardware through the normal run_queue that follows. Simon -Original Message- From: Jouni Malinen Sent: Wednesday, March 15, 2006 4:48 PM To: Simon Barber Cc: Jiri Benc; netdev@vger.kernel.org Subject: Re: [RFC/PATCH 6/13] d80211: remove obsolete stuff On Wed, Mar 15, 2006 at 04:41:56PM -0800, Simon Barber wrote: > The more natural way for beacons to flow from the 80211.o to the low > level driver would be for beacons to be passed down just like any > other > 802.11 frame is passed down - rather than having a special case for > beacons and buffered MC data, where they are pulled. I would suggest > making the qdisc aware of beacons, and then there is no special > interface for passing beacons down - they are passed down just like > other frames, with a special queue ID reserved for beacons and > buffered multicast. > > This would simplify the 80211.o/low level interface. Sure, but it would also require good synchronization for sending the beacons just before they are needed for transmission.. If the wlan hardware implementation provides support for interrupts that request beacons at proper times, being able to use them for this is quite convenient. -- Jouni MalinenPGP id EFC895FA - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 6/13] d80211: remove obsolete stuff
On Thu, Mar 16, 2006 at 05:45:48PM +0100, Jiri Benc wrote: > On Wed, 15 Mar 2006 16:36:16 -0800, Jouni Malinen wrote: > See the patch below. Is it viable? I'll test this with our low-level driver. > > This and similar change for ieee80211_get_buffered_bc() add more > > requirements for the low-level driver. It used to be enough to just know > > that the low-level code should ask for up to N beacon frames. However, > > with this change, the low-level driver would need to maintain a list of > > ifindexes for the virtual interfaces. This is somewhat against the > > original design of hiding all the virtual interfaces from low-level > > code. > > I like the approach, but I'm afraid it's not generic enough. The new > code should cover all possibilities (even such hypothetical case as a > card capable of multiple APs in its firmware with host-buffering of > frames for STAs in PS mode). Your proposed change does not cover at least one case.. > In a typical case of only-one-AP capable card the code will be nearly > the same. In your case it indeed means one extra list. Agreed. > Yes, I wanted to add ifindex into ieee80211_if_conf but apparently > forgot to. See patch below. OK. I'll test this. > This patch fixes some problems in interface configuration. > > - Pass interface ID to add_interface and remove_interface callbacks. This ID > is used by a driver when calling ieee80211_beacon_get and > ieee80211_get_buffered_bc functions. There is one more corner case that I did not remember yesterday: Atheros XR. It is using two BSSIDs and two different beacons per network interface (or at least our implementation of it is doing this; in theory, it could be possible to do this with two interfaces and bridging, but it gets somewhat complex for couple of cases).. With the current d80211 implementation, this can be handled by just multiplying bss_count with two. With per-ifindex call, this is more difficult. One option would be to modify the interface to return more than one skb in this case.. This might actually be useful way to handle all indexes if the beacons are to be sent at the same time. However, if beacons are to be sent at different times for different BSSes, it may be easier to just maintain the current mechanism (one call per virtual AP) and allow XR to return two beacon frames. -- Jouni MalinenPGP id EFC895FA - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 6/13] d80211: remove obsolete stuff
On Wed, 15 Mar 2006 16:36:16 -0800, Jouni Malinen wrote:
> In theory, the low-level driver can determine the needed mask itself.
> However, it would need to be somehow notified of allowed BSSID values.
> By removing this entry, this information would need to fetched from
> somewhere else before interfaces are added.
>
> Most hardware implementations have storage for a single MAC address in
> EEPROM (or something similar) and in some cases, no addresses are stored
> with the card and some external store is needed for this. We have been
> using this mechanism of passing the information from user space to avoid
> problems in figuring out board specific mechanisms for storing extra
> data. Do you have any ideas on what would be the best of getting this
> information configured after this change?
See the patch below. Is it viable?
> This and similar change for ieee80211_get_buffered_bc() add more
> requirements for the low-level driver. It used to be enough to just know
> that the low-level code should ask for up to N beacon frames. However,
> with this change, the low-level driver would need to maintain a list of
> ifindexes for the virtual interfaces. This is somewhat against the
> original design of hiding all the virtual interfaces from low-level
> code.
I like the approach, but I'm afraid it's not generic enough. The new
code should cover all possibilities (even such hypothetical case as a
card capable of multiple APs in its firmware with host-buffering of
frames for STAs in PS mode).
In a typical case of only-one-AP capable card the code will be nearly
the same. In your case it indeed means one extra list.
> I think the ifindex values could be made available from add/remove
> interface calls that you added. Was that what you had in mind or is
> there another mechanism for getting the needed ifindexes down?
Yes, I wanted to add ifindex into ieee80211_if_conf but apparently
forgot to. See patch below.
>8
This patch fixes some problems in interface configuration.
- Pass interface ID to add_interface and remove_interface callbacks. This ID
is used by a driver when calling ieee80211_beacon_get and
ieee80211_get_buffered_bc functions.
- New configuration callback, config_interface, is introduced.
- Allow BSSID to be set per-interface.
Signed-off-by: Jiri Benc <[EMAIL PROTECTED]>
Index: dscape/include/net/d80211.h
===
--- dscape.orig/include/net/d80211.h2006-03-06 16:23:57.0 +0100
+++ dscape/include/net/d80211.h 2006-03-16 16:10:08.0 +0100
@@ -286,8 +286,6 @@ struct ieee80211_conf {
int atheros_xr;
- u8 client_bssid[ETH_ALEN];
-
/* Following five fields are used for IEEE 802.11H */
unsigned int radar_detect;
unsigned int spect_mgmt;
@@ -310,11 +308,18 @@ struct ieee80211_conf {
#define IEEE80211_SUB_IF_TYPE_WDS 0x5A580211
#define IEEE80211_SUB_IF_TYPE_VLAN 0x00080211
-struct ieee80211_if_conf {
+struct ieee80211_if_init_conf {
+ int if_id; /* not valid for monitor interface */
int type;
void *mac_addr;
};
+struct ieee80211_if_conf {
+ int type; /* just for convenience; doesn't change during
+* the live of the interface */
+ u8 *bssid;
+};
+
typedef enum { ALG_NONE, ALG_WEP, ALG_TKIP, ALG_CCMP, ALG_NULL }
ieee80211_key_alg;
@@ -476,17 +481,22 @@ struct ieee80211_hw {
* of open() and add_interface() handler has to be non-NULL. If
* add_interface() is NULL, one STA interface is permitted only. */
int (*add_interface)(struct net_device *dev,
-struct ieee80211_if_conf *conf);
+struct ieee80211_if_init_conf *conf);
/* Notify a driver that interface is going down. The stop() handler
* is called prior to this if this is a last interface. */
void (*remove_interface)(struct net_device *dev,
-struct ieee80211_if_conf *conf);
+struct ieee80211_if_init_conf *conf);
/* Handler for configuration requests. IEEE 802.11 code calls this
* function to change hardware configuration, e.g., channel. */
int (*config)(struct net_device *dev, struct ieee80211_conf *conf);
+ /* Handler for configuration requests related to interfaces (e.g.
+* BSSID). */
+ int (*config_interface)(struct net_device *dev, int if_id,
+ struct ieee80211_if_conf *conf);
+
/* ieee80211 drivers should use this and not the function in
* net_device. dev->flags, dev->mc_count and dev->mc_list must not
* be used; use passed parameters and ieee80211_get_mc_list_item
@@ -674,7 +684,7 @@ void ieee80211_tx_status_irqsafe(struct
* low-level is responsible for calling this function before beacon data is
* needed (e.g., based on hardware interrup
Re: [RFC/PATCH 6/13] d80211: remove obsolete stuff
On Wed, Mar 15, 2006 at 04:41:56PM -0800, Simon Barber wrote: > The more natural way for beacons to flow from the 80211.o to the low > level driver would be for beacons to be passed down just like any other > 802.11 frame is passed down - rather than having a special case for > beacons and buffered MC data, where they are pulled. I would suggest > making the qdisc aware of beacons, and then there is no special > interface for passing beacons down - they are passed down just like > other frames, with a special queue ID reserved for beacons and buffered > multicast. > > This would simplify the 80211.o/low level interface. Sure, but it would also require good synchronization for sending the beacons just before they are needed for transmission.. If the wlan hardware implementation provides support for interrupts that request beacons at proper times, being able to use them for this is quite convenient. -- Jouni MalinenPGP id EFC895FA - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC/PATCH 6/13] d80211: remove obsolete stuff
The more natural way for beacons to flow from the 80211.o to the low
level driver would be for beacons to be passed down just like any other
802.11 frame is passed down - rather than having a special case for
beacons and buffered MC data, where they are pulled. I would suggest
making the qdisc aware of beacons, and then there is no special
interface for passing beacons down - they are passed down just like
other frames, with a special queue ID reserved for beacons and buffered
multicast.
This would simplify the 80211.o/low level interface.
Simon
-Original Message-
From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]
On Behalf Of Jouni Malinen
Sent: Wednesday, March 15, 2006 4:36 PM
To: Jiri Benc
Cc: netdev@vger.kernel.org
Subject: Re: [RFC/PATCH 6/13] d80211: remove obsolete stuff
On Mon, Mar 06, 2006 at 04:44:26PM +0100, Jiri Benc wrote:
> Because any number of interfaces may be added, bss_devs and sta_devs
> arrays cannot be fixed-size arrays. We can make them linked lists, but
> they are needed for optimalization only (and even that is questionable
> with subsequent patches). Let's remove them; we will probably want
> something similar later to speed up packet receiving, but let's not
> bother ourselves now.
> @@ -277,9 +277,6 @@ struct ieee80211_conf {
> int antenna_def;
> int antenna_mode;
>
> - u8 bssid_mask[ETH_ALEN];/* ff:ff:ff:ff:ff:ff = 1 BSSID
*/
> - int bss_count;
In theory, the low-level driver can determine the needed mask itself.
However, it would need to be somehow notified of allowed BSSID values.
By removing this entry, this information would need to fetched from
somewhere else before interfaces are added.
Most hardware implementations have storage for a single MAC address in
EEPROM (or something similar) and in some cases, no addresses are stored
with the card and some external store is needed for this. We have been
using this mechanism of passing the information from user space to avoid
problems in figuring out board specific mechanisms for storing extra
data. Do you have any ideas on what would be the best of getting this
information configured after this change?
> --- dscape.orig/net/d80211/ieee80211.c2006-03-06
14:10:18.0 +0100
> +++ dscape/net/d80211/ieee80211.c 2006-03-06 14:10:22.0
+0100
> @@ -1569,17 +1569,14 @@ struct sk_buff * ieee80211_beacon_get(st
> u8 *b_head, *b_tail;
> int bh_len, bt_len;
>
> - spin_lock_bh(&local->sub_if_lock);
> - if (bss_idx < 0 || bss_idx >= local->bss_dev_count)
> - bdev = NULL;
> - else {
> - bdev = local->bss_devs[bss_idx];
> + bdev = dev_get_by_index(bss_idx);
This and similar change for ieee80211_get_buffered_bc() add more
requirements for the low-level driver. It used to be enough to just know
that the low-level code should ask for up to N beacon frames. However,
with this change, the low-level driver would need to maintain a list of
ifindexes for the virtual interfaces. This is somewhat against the
original design of hiding all the virtual interfaces from low-level
code.
I think the ifindex values could be made available from add/remove
interface calls that you added. Was that what you had in mind or is
there another mechanism for getting the needed ifindexes down? I need to
understand this bit better in order to be able to modify the low-level
driver to handle this kind of change. At the moment, this change does
not look very good to me because of the extra requirement added for the
low-level code as far as virtual interfaces are concerned.
--
Jouni MalinenPGP id EFC895FA
-
To unsubscribe from this list: send the line "unsubscribe netdev" in the
body of a message to [EMAIL PROTECTED] More majordomo info at
http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 6/13] d80211: remove obsolete stuff
On Mon, Mar 06, 2006 at 04:44:26PM +0100, Jiri Benc wrote:
> Because any number of interfaces may be added, bss_devs and sta_devs arrays
> cannot be fixed-size arrays. We can make them linked lists, but they are
> needed for optimalization only (and even that is questionable with
> subsequent patches). Let's remove them; we will probably want something
> similar later to speed up packet receiving, but let's not bother ourselves
> now.
> @@ -277,9 +277,6 @@ struct ieee80211_conf {
> int antenna_def;
> int antenna_mode;
>
> - u8 bssid_mask[ETH_ALEN];/* ff:ff:ff:ff:ff:ff = 1 BSSID */
> - int bss_count;
In theory, the low-level driver can determine the needed mask itself.
However, it would need to be somehow notified of allowed BSSID values.
By removing this entry, this information would need to fetched from
somewhere else before interfaces are added.
Most hardware implementations have storage for a single MAC address in
EEPROM (or something similar) and in some cases, no addresses are stored
with the card and some external store is needed for this. We have been
using this mechanism of passing the information from user space to avoid
problems in figuring out board specific mechanisms for storing extra
data. Do you have any ideas on what would be the best of getting this
information configured after this change?
> --- dscape.orig/net/d80211/ieee80211.c2006-03-06 14:10:18.0
> +0100
> +++ dscape/net/d80211/ieee80211.c 2006-03-06 14:10:22.0 +0100
> @@ -1569,17 +1569,14 @@ struct sk_buff * ieee80211_beacon_get(st
> u8 *b_head, *b_tail;
> int bh_len, bt_len;
>
> - spin_lock_bh(&local->sub_if_lock);
> - if (bss_idx < 0 || bss_idx >= local->bss_dev_count)
> - bdev = NULL;
> - else {
> - bdev = local->bss_devs[bss_idx];
> + bdev = dev_get_by_index(bss_idx);
This and similar change for ieee80211_get_buffered_bc() add more
requirements for the low-level driver. It used to be enough to just know
that the low-level code should ask for up to N beacon frames. However,
with this change, the low-level driver would need to maintain a list of
ifindexes for the virtual interfaces. This is somewhat against the
original design of hiding all the virtual interfaces from low-level
code.
I think the ifindex values could be made available from add/remove
interface calls that you added. Was that what you had in mind or is
there another mechanism for getting the needed ifindexes down? I need to
understand this bit better in order to be able to modify the low-level
driver to handle this kind of change. At the moment, this change does
not look very good to me because of the extra requirement added for the
low-level code as far as virtual interfaces are concerned.
--
Jouni MalinenPGP id EFC895FA
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 6/13] d80211: remove obsolete stuff
On Mon, Mar 06, 2006 at 08:07:26PM +0100, Jiri Benc wrote: > On Mon, 6 Mar 2006 10:49:46 -0800, Jouni Malinen wrote: > > The reason for this optimization was in even high-end CPUs starting to > > run out of resources when running one radio with 2007 "virtual STAs", > Yes, I'm aware of that. But I'm afraid that hard-wired need for STA > interfaces to have incremental MAC addresses is not acceptable - and > this fact alone means degrading of performance in case of 2000+ STAs. Since this is not needed for most normal use cases, this is probably fine. However, being able to test with large number of STAs is very useful test case for AP functionality (number of APs crash if you try to associate that many STAs..) and performance. One approach could be to add a hash table for address to netdev mapping. If that is not easily doable, a (hopefully) small patch could of course be maintained separately, but I would prefer to see this operation mode as something that is supported in the stack by default. -- Jouni MalinenPGP id EFC895FA - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 6/13] d80211: remove obsolete stuff
On Mon, 6 Mar 2006 10:49:46 -0800, Jouni Malinen wrote: > The reason for this optimization was in even high-end CPUs starting to > run out of resources when running one radio with 2007 "virtual STAs", > i.e., when testing AP capabilities.. In other words, unless some of your > other patches improved netdev lookup, more work is likely needed here to > allow such a test mode to be used. Yes, I'm aware of that. But I'm afraid that hard-wired need for STA interfaces to have incremental MAC addresses is not acceptable - and this fact alone means degrading of performance in case of 2000+ STAs. -- Jiri Benc SUSE Labs - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 6/13] d80211: remove obsolete stuff
On Mon, Mar 06, 2006 at 04:44:26PM +0100, Jiri Benc wrote: > Because any number of interfaces may be added, bss_devs and sta_devs arrays > cannot be fixed-size arrays. We can make them linked lists, but they are > needed for optimalization only (and even that is questionable with > subsequent patches). Let's remove them; we will probably want something > similar later to speed up packet receiving, but let's not bother ourselves > now. The reason for this optimization was in even high-end CPUs starting to run out of resources when running one radio with 2007 "virtual STAs", i.e., when testing AP capabilities.. In other words, unless some of your other patches improved netdev lookup, more work is likely needed here to allow such a test mode to be used. With 2000 netdevs, there were other performance issues, at least with Linux 2.4.x. If I remember correctly, it took close to half an hour to just add and remove those netdevs.. ;-) Anyway, after the netdevs were up, these optimizations were enough to keep CPU load reasonable in a test case that sent data packets through all the STAs. I have not tested this with 2.6 kernels, so the area of adding/deleting netdevs may have already been improved. -- Jouni MalinenPGP id EFC895FA - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 6/13] d80211: remove obsolete stuff
Because any number of interfaces may be added, bss_devs and sta_devs arrays
cannot be fixed-size arrays. We can make them linked lists, but they are
needed for optimalization only (and even that is questionable with
subsequent patches). Let's remove them; we will probably want something
similar later to speed up packet receiving, but let's not bother ourselves
now.
Also, ieee80211_addr_inc is removed. Choosing of MAC address of a new STA
should be matter of userspace. It's responsibility of the stack not to allow
two STAs with the same MAC address to be up - this feature is introduced in
one of subsequent patches.
Signed-off-by: Jiri Benc <[EMAIL PROTECTED]>
Index: dscape/include/net/d80211.h
===
--- dscape.orig/include/net/d80211.h2006-03-06 14:10:18.0 +0100
+++ dscape/include/net/d80211.h 2006-03-06 14:10:22.0 +0100
@@ -277,9 +277,6 @@ struct ieee80211_conf {
int antenna_def;
int antenna_mode;
- u8 bssid_mask[ETH_ALEN];/* ff:ff:ff:ff:ff:ff = 1 BSSID */
- int bss_count;
-
int atheros_super_ag_compression;
int atheros_super_ag_fast_frame;
int atheros_super_ag_burst;
Index: dscape/net/d80211/ieee80211.c
===
--- dscape.orig/net/d80211/ieee80211.c 2006-03-06 14:10:18.0 +0100
+++ dscape/net/d80211/ieee80211.c 2006-03-06 14:10:22.0 +0100
@@ -1569,17 +1569,14 @@ struct sk_buff * ieee80211_beacon_get(st
u8 *b_head, *b_tail;
int bh_len, bt_len;
- spin_lock_bh(&local->sub_if_lock);
- if (bss_idx < 0 || bss_idx >= local->bss_dev_count)
- bdev = NULL;
- else {
- bdev = local->bss_devs[bss_idx];
+ bdev = dev_get_by_index(bss_idx);
+ if (bdev) {
sdata = IEEE80211_DEV_TO_SUB_IF(bdev);
ap = &sdata->u.ap;
+ dev_put(bdev);
}
- spin_unlock_bh(&local->sub_if_lock);
- if (bdev == NULL || ap == NULL || ap->beacon_head == NULL) {
+ if (ap == NULL || ap->beacon_head == NULL) {
#ifdef CONFIG_D80211_VERBOSE_DEBUG
if (net_ratelimit())
printk(KERN_DEBUG "no beacon data avail for idx=%d "
@@ -1646,19 +1643,15 @@ ieee80211_get_buffered_bc(struct net_dev
ieee80211_txrx_result res = TXRX_DROP;
struct net_device *bdev;
struct ieee80211_sub_if_data *sdata;
- struct ieee80211_if_ap *bss;
+ struct ieee80211_if_ap *bss = NULL;
- spin_lock_bh(&local->sub_if_lock);
- if (bss_idx < 0 || bss_idx >= local->bss_dev_count) {
- bdev = NULL;
- bss = NULL;
- } else {
- bdev = local->bss_devs[bss_idx];
+ bdev = dev_get_by_index(bss_idx);
+ if (bdev) {
sdata = IEEE80211_DEV_TO_SUB_IF(bdev);
bss = &sdata->u.ap;
+ dev_put(bdev);
}
- spin_unlock_bh(&local->sub_if_lock);
- if (bdev == NULL || bss == NULL || bss->beacon_head == NULL)
+ if (bss == NULL || bss->beacon_head == NULL)
return NULL;
if (bss->dtim_count != 0)
@@ -1911,14 +1904,14 @@ ieee80211_get_wds_dev(struct ieee80211_l
static struct net_device * ieee80211_own_bssid(struct ieee80211_local *local,
u8 *addr)
{
- int i;
struct net_device *dev = NULL;
+ struct ieee80211_sub_if_data *sdata;
spin_lock_bh(&local->sub_if_lock);
- for (i = 0; i < local->bss_dev_count; i++) {
- if ((memcmp(local->bss_devs[i]->dev_addr, addr, ETH_ALEN) == 0)
- ) {
- dev = local->bss_devs[i];
+ list_for_each_entry(sdata, &local->sub_if_list, list) {
+ if (sdata->type == IEEE80211_SUB_IF_TYPE_AP &&
+ memcmp(addr, sdata->dev->dev_addr, ETH_ALEN) == 0) {
+ dev = sdata->dev;
break;
}
}
@@ -1934,31 +1927,9 @@ static struct net_device * ieee80211_sta
{
struct list_head *ptr;
int multicast;
- u8 *own_addr = local->mdev->dev_addr;
multicast = a1[0] & 0x01;
- /* Try O(1) lookup for a common case of only one AP being used. */
- if (own_addr[0] == a1[0] && own_addr[1] == a1[1] &&
- own_addr[2] == a1[2]) {
- int index = (((int) a1[3] << 16) | ((int) a1[4] << 8) | a1[5])
- - (((int) own_addr[3] << 16) |
- ((int) own_addr[4] << 8) | own_addr[5]);
- if (index >= 0 && index < local->conf.bss_count &&
- local->sta_devs[index]) {
- struct net_device *dev = local->sta_devs[index];
- struct ieee80211_sub_if_data *sdata;
- sdata = IEEE80211_DEV_TO_SUB_IF(dev);
-
