Add iwlwifi wireless drivers

Hi,

This is the second version of the patch (still against 2.6.23-rc3) adds the mac80211 based wireless drivers for the Intel PRO/Wireless 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN (4965) adapters. You can find it from:
http://intellinuxwireless.org/iwlwifi/v5-Add-iwlwifi-wireless-drivers.patch

I list the changes against the first version I submitted to the list on 8/17 in case you have reviewed the previous version and only want to see the diff. You can find the overall patch here:
http://intellinuxwireless.org/iwlwifi/iwlwifi-v4_to_v5.patch
and the broken out patches here:
http://intellinuxwireless.org/iwlwifi/patches/iwlwifi-v5-v4-patches/

Ben M Cahill (5):
iwlwifi: remove unused 3945-only struct from 4965 build
iwlwifi: move all uCode API structs into iwl-command.h
iwlwifi: replace iwl_scanstart_notification with
iwl_scanreq_notification
iwlwifi: add struct iwl_card_state_cmd as documentation
iwlwifi: add BSM_DRAM_INST_LOAD definition

Johannes Berg (1):
iwlwifi: remove atheros turbo modes

Johannes Engel (1):

      iwlwifi: fix iwl-3945-rs.c typo

Mohamed Abbas (3):
iwlwifi: fix rs_get_best_rate to pick up the right rate
iwlwifi: fix 11n connection problem
iwlwifi: fix aggregation problem

Tomas Winkler (11):
iwlwifi: adding queues_num module parameter
iwlwifi: iwl_get_sta_id AP mode fix
iwlwifi: EEPROM band 5 endianity fix
iwlwifi: AP mode beacon update
iwlwifi: kill iwl_rate union
iwlwifi: simplifying reclaim flow
iwlwifi: removing while loop from iwl_print_hex_dump
iwlwifi: typo fix for CONFIG_IWLWIFI_SPECTRUM_MEASUREMENY
iwlwifi: remove unused code
iwlwifi: rate scaling - up scaling fix
iwlwifi: rate scale quality command fix

Zhu Yi (10):
iwlwifi: replace lock_flags into simpler flags
iwlwifi: split priv->hcmd_lock from priv->lock to protect hcmd
queue
iwlwifi: iwl_mac_set_key cleanup
iwlwifi: fix priv->hcmd_lock uninitialized problem
iwlwifi: Update version iwl-base.c stamp to 0.1.9
iwlwifi: fix iwl_grab_restricted_access return code
iwlwifi: Update version iwl-base.c stamp to 0.1.10
iwlwifi: add a parameter to functions
iwl_rate_control_[un]register
iwlwifi: Update version iwl-base.c stamp to 0.1.11
iwlwifi: fix iwl_send_cmd_async return value bug

Btw, strong NACK from me until you sort out the mess with the common files. Including C files with cpp symbols defined or not is not how we do driver development in Linux. Please split out really common code into common files, and build driver specific code into files of it's own. Having a slight amount of duplicated code is much better than having such a mess for maintaince.

It's not a "slight amount" of duplicated code.

$ expr `grep -e '^{$' iwl-base.c |wc -l` - ` sed -e '/^{$/,/^}$/{/#if
IWL == /,/^}$/d}' iwl-base.c |grep -e '^}$' |wc -l`
34

Additional 34 functions have to be splited out from iwl-base.c into their own hardware specific files. If you look at the code, most of these functions are only with little difference which are caused by the difference of the hardware. In the future, when we add new hardware support for iwlwifi, it's more maintainable to extend the current functions with new hardware difference than copy all the 34 functions into a new file and only make slight changes to each of them.

I know the method is not common used in the Linux drivers, but this is decided by the hardware layout. For 3945 and 4965, 90% of the hardware/firmware layout are the same and only 10% are different. This makes it possible to support two slightly differs hardwares in one driver. If the hardwares differ a lot (i.e 2100 and 2200), I won't even think about to do it in this way.

Thanks,
-yi

To be fair, that's not what you do, you're building two drivers from one source rather than have a single driver that supports both. I'm fairly certain Christoph wouldn't object to the latter.

johannes

Because of the difference of the two hardwares (for example, in iwl-command.h, iwl_rxon_assoc_cmd differs between 3945 and 4965), runtime supporting is difficult if not possible (hw structures differs). So we select to do it statically by generating two drivers from one source code.

Do you think if it is a good idea to split them into two drivers and make each of them "#include iwl-base.c"? Please don't hesitate to share if you have better ideas.

Thanks,
-yi

We used to be able to even runtime-select transmit headers in bcm43xx.

I thought that's what you do now?

I haven't looked into what the differences are.

Some things I plain don't understand, like this:
#if IWL == 4965
#ifdef CONFIG_IWLWIFI_SENSITIVITY

why not just make the latter depend on 4965 in Kconfig? Same with HT.

johannes

Absolutely not. The right way to do it is to have a least a common driver core, and if the device-specific parts get too big they can but don't have to be separate modules.

With s"light amount of duplicated code" I meant possibly duplicated bits if the split between common and device-specific bits doesn't work out perfectly because e.g. both device types do the same kind of operation on differently layed out data structures, which AFAIK is the case for some operations in this driver.

---end quoted text---

As far as I can speak for next generation of Intel Linux Wifi drivers the #if IWL == will be killed.
3945 will be probably dropped to it's own module as it is abg NIC unlike other 11n NICs. It differs quite a lot in data structures and mostly in flows. This wasn't clear at the begin when 4965 was build on top of 3945 so we ended up with IWL == which was much more efficient at the time. We had the library approach before if you remember.
The mess stared somewhere at the point where HT was dropped from submission to mainstream and we rushed to create something that can be submitted. To much code was jungled in the air and still is... Hope we settle all this now to the right path

Tomas

A few comments below -- many of them are nits or somewhat minor. I think the ieee80211_rate.h one needs to be resolved for sure. Also, splitting iwl-base.c is definitely worth considering -- building two object from one source is a bit ugly.

Please respond to the questions/comments, and indicate if/when you will split iwl-base.c. Also, let's figure-out what really needs to be exportd to drivers from ieee80211_rate.h and get that done.

Thanks for the post!

John

--------------------------------------------------------------------------------

Big plus: both drivers work fairly well, and have received a lot of testing in Fedora (F7 and rawhide).

--------------------------------------------------------------------------------

As others have pointed-out, building two drivers from a single source is a bit ugly. I hacked-up a couple of awk scripts to separate this into two files (btw, let me know if you want them). It seems the two files differ by 5-10% (depending on how you score it). I'm not sure what the cut-off should be for requiring a split...?

$ wc iwl-base.c
  9565  29226 265696 iwl-base.c

$ wc iwl3945-base.c
  8701  26834 242294 iwl3945-base.c

$ wc iwl4965-base.c
  9197  28016 256601 iwl4965-base.c

FWIW, it looks like some of the header files also use the IWL defintion for "two objects from one source" compilation.

--------------------------------------------------------------------------------

Nit: does iwl_hw_valid_rtc_data_addr need to be inline?

--------------------------------------------------------------------------------

IWL_DEBUG_RATE("enter\n")
IWL_DEBUG_RATE("leave\n")
IWL_DEBUG_RATE("NOP\n");

Multiples of each of these, particularly in the rate scaling algorithms. Are these necessary? Some seem to like them, others don't...

--------------------------------------------------------------------------------

iwl-4965-rs.c:

#include "../net/mac80211/ieee80211_rate.h"

I think this is unacceptable. Let's figure-out what needs to be exported to drivers and get it moved to an appopriate header, rather than just pulling a header from a random location out of the tree.

--------------------------------------------------------------------------------

Same file line 586:

#define IWL_LEGACY_SWITCH_ANTENNA      0
#define IWL_LECACY_SWITCH_SISO         1
#define IWL_LEGACY_SWITCH_MIMO         2

#define IWL_RS_GOOD_RATIO 12800

#define IWL_ACTION_LIMIT               3
#define IWL_LEGACY_FAILURE_LIMIT       160
#define IWL_LEGACY_SUCCESS_LIMIT       480
#define IWL_LEGACY_TABLE_COUNT         160

#define IWL_NONE_LEGACY_FAILURE_LIMIT  400
#define IWL_NONE_LEGACY_SUCCESS_LIMIT  4500
#define IWL_NONE_LEGACY_TABLE_COUNT    1500

#define IWL_RATE_SCALE_SWITCH (10880)

Shouldn't these be in a header file, or at least at the top of this file?

--------------------------------------------------------------------------------

Same file line 1115:

#define IWL_SISO_SWITCH_ANTENNA  0
#define IWL_SISO_SWITCH_MIMO     1
#define IWL_SISO_SWITCH_GI       2

Ditto?

--------------------------------------------------------------------------------

Same file line 1210:

#define IWL_MIMO_SWITCH_ANTENNA_A      0
#define IWL_MIMO_SWITCH_ANTENNA_B      1
#define IWL_MIMO_SWITCH_GI             2

Ditto?

--------------------------------------------------------------------------------

Does iwl_rate_index_from_plcp need to be inline? It seems a bit long, and called from several places. (And defined in multiple places...)

--------------------------------------------------------------------------------

iwl-4965.c line 1815:

#define TX_POWER_IWL_ILLEGAL_VDET -100000
#define TX_POWER_IWL_ILLEGAL_VOLTAGE -10000 #define TX_POWER_IWL_CLOSED_LOOP_MIN_POWER 18 #define TX_POWER_IWL_CLOSED_LOOP_MAX_POWER 34 #define TX_POWER_IWL_VDET_SLOPE_BELOW_NOMINAL 17 #define TX_POWER_IWL_VDET_SLOPE_ABOVE_NOMINAL 20
#define TX_POWER_IWL_NOMINAL_POWER 26
#define TX_POWER_IWL_CLOSED_LOOP_ITERATION_LIMIT 1
#define TX_POWER_IWL_VOLTAGE_CODES_PER_03V 7
#define TX_POWER_IWL_DEGREES_PER_VDET_CODE 11
#define IWL_TX_POWER_MAX_NUM_PA_MEASUREMENTS 1 #define IWL_TX_POWER_CCK_COMPENSATION_B_STEP (9) #define IWL_TX_POWER_CCK_COMPENSATION_C_STEP (5)

Same as previous "#define in middle of file" comments...

--------------------------------------------------------------------------------

iwl-4965.c line 2800:

#define IWL4965_LEGACY_SWITCH_ANTENNA 0
#define IWL4965_LECACY_SWITCH_SISO 1
#define IWL4965_LEGACY_SWITCH_MIMO 2

#define IWL4965_GOOD_RATIO 12800

#define IWL_ACTION_LIMIT               3
#define IWL4965_LEGACY_FAILURE_LIMIT   160
#define IWL4965_LEGACY_SUCCESS_LIMIT   480

#define IWL4965_LEGACY_TABLE_COUNT 160

#define IWL4965_NONE_LEGACY_FAILURE_LIMIT      400
#define IWL4965_NONE_LEGACY_SUCCESS_LIMIT      4500
#define IWL4965_NONE_LEGACY_TABLE_COUNT        1500

#define IWL4965_RATE_SCALE_SWITCH (10880)

Ditto?

--------------------------------------------------------------------------------

There are several more examples like the above -- I'm not going to keep documenting them...

--------------------------------------------------------------------------------

Otherwise, it mostly seems acceptable. I think the mac80211 guys have a few issues -- hopefully they will chime-in here now?

John W. Linville
[email protected]

Few things off the top of my head:
* mac80211 will be very noisy with the recent key changes -- the iwlwifi drivers don't allow disabling keys which is sort of required.

* oopses when you enabled a monitor mode interface and have iwlwifi debug enabled

* doesn't honour mac address setting, always uses eeprom mac address

johannes

Here's another thing:

iwlwifi tries to guess the state of the associate state machine in the stack by looking at what frames it sends. This will sooner or later break, please fix it by adding notification calls to the stack about the things iwlwifi cares about (I think it's mostly powersave mode).

Also, it drops frames to not associated stations. This is something mac80211 already does.

Then there's some code manipulating the QoS field: that's something you should be contributing to the stack instead of hiding in iwlwifi. It also seems to do aggregation. Why the hell are you adding deaggregation to the stack while having aggregation here. Hate.

Also, there are some comments I don't understand related to fragmentation ("copy frags header").

Clearly, there's lots of work to do.

johannes

Will split it in the next version as well as addressing all your comments below.

Since they are device specific rate scale algorithm, I don't think they will help to increase performance for other devices. So make them stay together with the driver is a better choice (vs. with mac80211). If so, the rate scale interface has to be abstracted from internal mac80211. The current dependencies are as below:

iwl3945-rs -> rate_control_ops (ieee80211_rate.h) rate_control_ops -> sta_info (sta_info.h) rate_control_ops -> ieee80211_local (ieee80211_i.h) ieee80211_local -> ieee80211_key (ieee80211_key.h)

If we want to avoid a big restructure of the mac80211 headers, the easiest way is to move four header files from net/mac80211 to include/net/ and rename them with mac80211_ prefix (especially for sta_info.h). This way, for any mac80211 based wireless drivers want to create their own rate scale algorithm, they can smiply include <net/mac80211_rate.h> and implement all the callback functions. Comments?

If you all agree with this change, especially the mac80211 folks, I'll come up with a patch for it.

Thanks,
-yi

What exactly is device specific?

I never liked exporting ieee80211_local to the rate control algorithms. That just means the interface is badly defined. And sta_info I'm not too sure about either, on the one hand the rate control algorithms need access to some flags but on the other hand some things if they need to store per-sta information then it can't really be in the sta_info struct since that info varies per algorithm

I don't like this, especially not ieee80211_i.h. I'm ok with _rate.h, but let's get rid of the ieee80211_local dependency in rate control algorithms.

johannes

I thought I'd try and answer this question to the best of my ability, since it has been asked before. And even though it's open source and now has been submitted to this list, leaving this unanswered feels like a creepy way of potential time bombs and frustration. That said I'm probably not the best person to do it.

iwl3945-rs:

- the device can retry at different rates, and hence is able to deduct from the total number of retries a packet needed at which rates it failed/ succeeded

- tables of expected tpt (throughput?) which are used in the the throughput calculation are probably not very universal?
there aren't identical for 3945 and 4965.

-some synchronization of the station list with the device ucode happens here

addidtionally in iwl4965-rs:

- there is additionally the use of the "link quality" command which for example gets issued when there isn't enough of other throughput data available.

Might be other things that I have missed, and
parts of the algorithm might be tested/fine tuned for the intended devices.

So that's that. Some questionable implementation details, but it does use device specific logic/capabilities in order to decide which rate to use. Now what do we do?

ian

For the first time since this thread started, I think I begin to understand. What is wanted is not a new/exotic rate algorithm as much as a way to override the algorithm that mac80211 is using and specify the rate you want. If this is correct, such a change would be easy. Such an override would be valid only for a STA, and the logic is already there to lock the STA rate in the WEXT interface (see routine ieee80211_ioctl_siwrate in net/mac80211/ieee80211_ioctl.c). There are two quantities that index the rates in the mode->rates array, sdata->bss->max_ratectrl_rateidx, and sdata->bss->force_unicast_rateidx. They are interpreted using the following logic:

if (ratectl_rateidx == -1)
any legal rate is allowed
else
the maximum rate is that specified in ratectl_index

if (unicast_rateidx == -1)
allow any rate up to the maximum above
else
perform unicast operations at the specified rate

The resulting call will need to supply a net_device and a rate. After verifying that the STA exists, it should find the rate in the rate tables and set the two above quantities to the index of that rate, just as ieee80211_ioctl_siwrate does.

Certainly, as long as one allows a WEXT entry to set a fixed rate and I would strenuously object if someone tried to remove it, a driver should be allowed to do so as well.

Larry

Thanks, I appreciate it.

We recently discussed this capability, atheros hardware as it as well, so we need a generic way to tell mac80211 how many retry rates the hardware can support so that better rate control algorithms can be written. I don't see this as device specific but rather as something the mac80211 driver interface is currently lacking. Cf. the "minstrel" rate control algorithm.

Seems to me like something the driver should be doing and reporting to the rate control algorithm via some defined interface.

This is totally wrong. Please extend mac80211 instead, maybe the sta_table_notification callback.

As a first step, I think we (i.e. mostly you because only few other people have an interest right now) should work on defining an interface between the drivers and mac80211 that allows the drivers to export these capabilities like "how many different retry rates can I give you with one packet" in order to allow different rate control algorithms to take advantage of that. Oh and then don't just implement the interface and push it onto us as a "ok done" thing but discuss the interface first.

johannes

There is nothing NOT general in 3945 rate scaling algorithm just the current interfaces of rate scaling algorithm is a bit awkward, not extensible. The struct ieee80211_tx_status is tight to mac80211 and not to rate scale algorithm so if you create your own crazy rate scale algorithm you cannot decide what parameters you need.

Agree

True we have some RFC patches with that unfortunately need some time to catchup on current wireless-dev changes. Looks like students were on vacation and spent more time behind computers then on the sea shore :)

4965 is 11n card and rate scale algorithm is totally different. One aspect is that rate scaling works with much more parameters. MIMO rates are rather 2 dimensional table, therefore the algorithm is essentially different, but so far no interface change is needed. Actually it covers also legacy rate scaling.

The second aspect of HT is the aggregation. Bunch of packets at once and they are also ACKed at once so regular TX status path of updating rate scale algorithm is not so natural. Mostly because it is combined with reclaiming the packet.

Third part and this is 4965 specific is that TX rate is not attached to packet but rate table is updated when need on it's own path. This is device specific.

Forth is that the aggregation. In general this involves some handshake on protocol level i.e. MLME but efficiency of aggregation is kind of rate scaling decision in addition it might be enabled only for MIMO rates (different plcp header)

I would suggest to change rate scaling interface algorithm in following way

1. rs_update_event(sta, struct rs_data *) - function that brings rate scaling statistics not connected to packet reclaim path struct rs_data is specific to rate scaling and opaque to mac80211

2. rs_add_sta/rs_init(sta) - create new instance of rate scaling for added station including AP in STA mode

3. rs_compute(sta/sta_addr) - actual computation of rate scaling algorithm possible also from RX path (tx reclaim)

4. rs_get_rate - for regular TX path setting of rates

5. void (*rs_apply_rates)(sta_addr) - driver supplied handler for applying rate not through TX path.

Tomas

Yeah, I agree, we need to improve this. "minstrel" really wants more retry rates too than just two.

Right. Maybe we can orthogonalize this into a rate control interface for .11n and one for "legacy"?

This is something you need to work on in mac80211 anyway. I don't like how your driver does all the decisions about when to aggregate etc. This should be in the per-sta code in mac80211.

It seemed like the rate was attached to each station. This is a bit and I don't really know how to handle it.

Right. I think you mentioned aggregation as number two already :)

What sort of statistics would it bring, and when?

That's what we have now afaict.

Where would it be called?

Right.

This needs more parameters, no?

johannes

11n should cover also legacy rate scaling, and legacy mode in general.

We have solved that it just as I said it is not rebased against latest wireless-dev so you don't see this code. I will probably release some RFC patches first so nobody will accuse me of selling FUD :)

???

Number 2 is the rate scaling update path this is rate scaling decision path

In legacy TX it will be the same as current implementation, actual rate, number of retries, etc
For aggregation it need rate and number of packets ACKed and NACked. The whole aggregation is sent in one rate. Theoreticaly multi rate aggregation might be sent, but no vendor creates such radios yet.

This can be run in reclaim path (RX) as now but for 4965 I would create period work

Sure, the rates themselves. Currently I have in vision only 4965 rate table. Need to find something more general probably.

Thanks for your comments
Tomas

Ok so we just extend the rate control to include .11n, fine with me too. Have you thought about .11e stuff too? This seems related (aggregation etc)

Sounds good :)

Well some comments in the code say that the rate retry stuff is attached to the notion of station the firmware has. If so, this will most likely require some different interaction between rate control/firmware rather than just the rate control passing you the rates with each frame.

Oh, ok. I think you need to elaborate on the split into these paths though, I don't think I've fully understood it yet.

Right.

We can add it when we need it then.

Basically we have three paths:
(1) tx
(2) rx
(3) tx status (tx reclaim)

I suppose you're thinking of TX reclaim here? Which is sort of like RX-ack but I think of it as related to TX. If we can handle the decision about aggregation in higher layers shouldn't the periodic work also be in mac80211?

Yeah I'm not aware what it does there. If you can describe what your hardware does I can possibly compare a bit to Broadcom (partially started to reverse engineer it) and then see if we can come up with something more generic.

johannes