netdev
[Top] [All Lists]

Re: [PATCH] ieee80211 subsystem

To: James Ketrenos <jketreno@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH] ieee80211 subsystem
From: "Randy.Dunlap" <rddunlap@xxxxxxxx>
Date: Fri, 04 Feb 2005 22:07:56 -0800
Cc: netdev@xxxxxxxxxxx
In-reply-to: <4203C32A.70402@xxxxxxxxxxxxxxx>
References: <4203C32A.70402@xxxxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla Thunderbird 0.9 (X11/20041103)
James Ketrenos wrote:
Attached is the patch against 2.6.11-rc3-mm1 that adds the ieee80211 subsystem used by the ipw2100 and ipw2200 projects.

I'll be sending out the patches for ipw2100-1.0.0 and ipw2200-1.0.0 that use thist stack to the list on Monday.

In terms of what the stack currently does:

* HW independent -- it only knows about 802.11 data and structures
* Performs an 802.3 <-> 802.11 transform for data Tx/Rx
* Host based support for fragmentation, WEP, and WPA using the kernel's crypto functions
* Beacon and probe response collection and parsing
* Default implementation of some of the WE handlers that can be managed without hardware knowledge

We are working to merge in Dave Miller's p80211 code into the ieee80211 subsystem so that it hooks into the kernel as a true network layer as opposed to a mutated offspring of ethernet. Once that is done, hopefully the skb to txb code can be reworked and 802.11 fragments can be treated either as normal skbs, or skbs can be modified to directly support them (ideally so that encrypted 802.11 frames in support of IP packets can be cached by the stack instead of having to be re-encrypted on TCP retries)

Support for HW/FW crypto and fragmentation offload, in a HW independent fashion, is also on the short-term list.

When you look through the patch you'll likely notice the #ifdef NOTYET/#endif sequences surrounding portions of code from the hostap project. Portions of this subsystem were based on an earlier version of the hostap project. Those areas that weren't directly supported by the ipw* projects weren't ported to be completely hardware independent (since I don't have the hardware to test it), and so are still wrapped in the ifdefs. These sections mainly cover support for MASTER and WDS modes.

Anyway, please let me know what you think. Hopefully I built the patch right...

It applies fine.

A few comments:

1.  config IEEE80211_DEBUG
        bool "Enable full debugging output"
        depends on IEEE80211

+         /proc/net/ieee80211/debug_level
+
+         For example:
+
+         % echo 0x00000FFO > /sys/bus/pci/drivers/ipw2200/debug_level

I get the impression that one of these file names is incorrect:
/proc vs. /sys.....

+         % . idvals
+       
+         From within drivers/net/wireless/ipw2200

Is there some good place for this other than in
drivers/net/wireless?  I don't know.

+         If you are not trying to debug or develop the IPW2200 driver,
+         you
+         most likely want to say N here.

Combine those last 2 lines.

2.      This can be compiled as a modules and it will be called
+       "ieee80211_crypt.ko".

Don't use the ".ko" suffix at all (anywhere).

3.  +static int __init ieee80211_crypto_init(void)
+       (void) ieee80211_register_crypto_ops(&ieee80211_crypt_null);

That register call can fail, so I'd rather see it checked, as in all
of the other similar register calls.

4.  +static void * ieee80211_ccmp_init(int key_idx)
+       if (priv == NULL) {
+               goto fail;
+       }

Lose the braces.  Same for:
ieee80211_ccmp_init()

5.  +static void ccmp_init_blocks()

+       /* CCM Initial Block:
+        * Flag (Include authentication header, M=3 (8-octet MIC),
+        *       L=1 (2-octet Dlen))
+        * Nonce: 0x00 | A2 | PN
+        * Dlen */

Kernel long comment style begins with "/*" by itself and
ends with "*/" by itself.

6.  +struct ieee80211_ccmp_data {

+       u32 dot11RSNAStatsCCMPFormatErrors;
+       u32 dot11RSNAStatsCCMPReplays;
+       u32 dot11RSNAStatsCCMPDecryptErrors;

Are these MIB-mandated names?  otherwise the studlyCaps should go.

7.  +static int ieee80211_ccmp_decrypt()

Does anything care about the various negative/error case return
values?

8.  What calls the .print_stats functions?
static char * ieee80211_ccmp_print_stats()

Is it possible to use seq_file there instead of sprintf(),
or is this in /sysfs (so seq_file is not possible)?
Are there any overflow possibilities?


Enough for tonight...

--
~Randy

<Prev in Thread] Current Thread [Next in Thread>