[PATCH V2] Add iwlwifi wireless drivers

[PATCH V2] Add iwlwifi wireless drivers

[Zhu Yi]

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

Re: [PATCH V2] Add iwlwifi wireless drivers

[Christoph Hellwig]

On Mon, Aug 27, 2007 at 01:20:12PM +0800, Zhu Yi wrote:
> 
> 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/

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.

Re: [PATCH V2] Add iwlwifi wireless drivers

[Zhu Yi]

On Mon, 2007-08-27 at 14:10 +0100, Christoph Hellwig wrote:
> 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

Re: [PATCH V2] Add iwlwifi wireless drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

On Tue, 2007-08-28 at 15:25 +0800, Zhu Yi wrote:

> 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.

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH V2] Add iwlwifi wireless drivers

[Zhu Yi]

On Tue, 2007-08-28 at 10:50 +0200, Johannes Berg wrote:
> 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. 

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

Re: [PATCH V2] Add iwlwifi wireless drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

On Tue, 2007-08-28 at 17:07 +0800, Zhu Yi wrote:

> 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.

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

> 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.

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH V2] Add iwlwifi wireless drivers

[Christoph Hellwig]

On Tue, Aug 28, 2007 at 10:50:15AM +0200, Johannes Berg wrote:
> On Tue, 2007-08-28 at 15:25 +0800, Zhu Yi wrote:
> 
> > 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.
> 
> 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.

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.

> 
> johannes


---end quoted text---

Re: [PATCH V2] Add iwlwifi wireless drivers

[Tomas Winkler]

On 8/28/07, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Aug 28, 2007 at 10:50:15AM +0200, Johannes Berg wrote:
> > On Tue, 2007-08-28 at 15:25 +0800, Zhu Yi wrote:
> >
> > > 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.
> >
> > 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.
>
> 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.
>
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


> >
> > johannes
>
>
> ---end quoted text---
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH V2] Add iwlwifi wireless drivers

[John W. Linville]

On Mon, Aug 27, 2007 at 01:20:12PM +0800, Zhu Yi wrote:

> 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

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
linville@tuxdriver.com

Re: [PATCH V2] Add iwlwifi wireless drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

On Fri, 2007-08-31 at 16:55 -0400, John W. Linville wrote:

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

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH V2] Add iwlwifi wireless drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 823 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH V2] Add iwlwifi wireless drivers

[Zhu Yi]

On Fri, 2007-08-31 at 16:55 -0400, John W. Linville wrote:
> 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.

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

> Also, let's figure-out what really needs to
> be exportd to drivers from ieee80211_rate.h and get that done.

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

Re: [PATCH V2] Add iwlwifi wireless drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]

On Tue, 2007-09-04 at 10:25 +0800, Zhu Yi wrote:

> Since they are device specific rate scale algorithm, I don't think they
> will help to increase performance for other devices.

What exactly is device specific?

> 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)

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

> 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?

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH V2] Add iwlwifi wireless drivers

[Ian Schram]

Johannes Berg wrote:
> On Tue, 2007-09-04 at 10:25 +0800, Zhu Yi wrote:
> 
>> Since they are device specific rate scale algorithm, I don't think they
>> will help to increase performance for other devices.
> 
> What exactly is device specific?
> 

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

> johannes

Re: [PATCH V2] Add iwlwifi wireless drivers

[Larry Finger]

Ian Schram wrote:
> 
> Johannes Berg wrote:
>> On Tue, 2007-09-04 at 10:25 +0800, Zhu Yi wrote:
>>
>>> Since they are device specific rate scale algorithm, I don't think they
>>> will help to increase performance for other devices.
>> What exactly is device specific?
>>
> 
> 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?

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

Re: [PATCH V2] Add iwlwifi wireless drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

On Thu, 2007-09-06 at 03:32 +0200, Ian Schram wrote:

> 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.

Thanks, I appreciate 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

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.

> - 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.

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

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

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

> 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?

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH V2] Add iwlwifi wireless drivers

[Tomas Winkler]

On 9/6/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2007-09-06 at 03:32 +0200, Ian Schram wrote:
>
> > 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.
>
> Thanks, I appreciate 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
>
> 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.

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.

> > - 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.
>
> Seems to me like something the driver should be doing and reporting to
> the rate control algorithm via some defined interface.
>
Agree

> > -some synchronization of the station list with the device ucode happens here
>
> This is totally wrong. Please extend mac80211 instead, maybe the
> sta_table_notification callback.
>
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
:)

> > 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?
>
> 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.
>
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





> johannes
>
>

Re: [PATCH V2] Add iwlwifi wireless drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2777 bytes --]

On Thu, 2007-09-06 at 17:04 +0300, Tomas Winkler wrote:

> 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.

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

> 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.

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

> 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.

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.

> 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.

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

> 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)

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

> 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

What sort of statistics would it bring, and when?

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

That's what we have now afaict.

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

Where would it be called?

> 4.  rs_get_rate - for regular TX path setting of rates

Right.

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

This needs more parameters, no?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH V2] Add iwlwifi wireless drivers

[Tomas Winkler]

On 9/6/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2007-09-06 at 17:04 +0300, Tomas Winkler wrote:
>
> > 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.
>
> Yeah, I agree, we need to improve this. "minstrel" really wants more
> retry rates too than just two.
>
> > 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.
>
> Right. Maybe we can orthogonalize this into a rate control interface
> for .11n and one for "legacy"?
>
11n should cover also legacy rate scaling, and legacy mode in general.

> > 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.
>
> 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.
>
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
:)

> > 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.
>
> It seemed like the rate was attached to each station. This is a bit and
> I don't really know how to handle it.
???

> > 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)
>
> Right. I think you mentioned aggregation as number two already :)
>
Number 2 is the rate scaling update path this is rate scaling decision path

> > 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
>
> What sort of statistics would it bring, and when?
>
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.

> > 2. rs_add_sta/rs_init(sta)  - create new instance of rate scaling for
> > added station including AP in STA mode
>
> That's what we have now afaict.
> Yes

> > 3. rs_compute(sta/sta_addr) - actual computation of rate scaling
> > algorithm possible also from RX path (tx reclaim)
>
> Where would it be called?

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

> > 4.  rs_get_rate - for regular TX path setting of rates
>
> Right.
>
> > 5. void (*rs_apply_rates)(sta_addr)  - driver supplied handler for
> > applying rate not through TX path.
>
> This needs more parameters, no?
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
>

Re: [PATCH V2] Add iwlwifi wireless drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2996 bytes --]

On Thu, 2007-09-06 at 17:31 +0300, Tomas Winkler wrote:

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

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)

> 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
> :)

Sounds good :)

> > > 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.
> >
> > It seemed like the rate was attached to each station. This is a bit and
> > I don't really know how to handle it.

> ???

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.

> > > 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)
> >
> > Right. I think you mentioned aggregation as number two already :)
> >
> Number 2 is the rate scaling update path this is rate scaling decision path

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.

> 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.

Right.

> The whole aggregation is sent in one rate. Theoreticaly multi rate
> aggregation might be sent, but no vendor creates such radios yet.

We can add it when we need it then.

> > > 3. rs_compute(sta/sta_addr) - actual computation of rate scaling
> > > algorithm possible also from RX path (tx reclaim)
> >
> > Where would it be called?
> 
> This can be run in reclaim path (RX) as now  but for 4965 I would
> create period work

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?

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

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]