[PATCH] Add iwlwifi wireless drivers

[PATCH] Add iwlwifi wireless drivers

[James Ketrenos]

This patch adds the iwlwifi project directory and sources needed to 
build the mac80211 based wireless drivers for the Intel PRO/Wireless 
3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.

Signed-off-by: James Ketrenos <jketreno@linux.intel.com>
---
 drivers/net/wireless/mac80211/Kconfig              |    1 +
 drivers/net/wireless/mac80211/Makefile             |    1 +
 drivers/net/wireless/mac80211/iwlwifi/Kconfig      |   96 +
 drivers/net/wireless/mac80211/iwlwifi/Makefile     |   27 +
 drivers/net/wireless/mac80211/iwlwifi/base.c       | 8146 ++++++++++++++++++++
 .../net/wireless/mac80211/iwlwifi/iwl-3945-hw.h    |   88 +
 .../net/wireless/mac80211/iwlwifi/iwl-3945-rs.h    |  175 +
 drivers/net/wireless/mac80211/iwlwifi/iwl-3945.c   | 2389 ++++++
 drivers/net/wireless/mac80211/iwlwifi/iwl-3945.h   |   54 +
 .../net/wireless/mac80211/iwlwifi/iwl-4965-hw.h    |  909 +++
 drivers/net/wireless/mac80211/iwlwifi/iwl-4965.c   | 3637 +++++++++
 drivers/net/wireless/mac80211/iwlwifi/iwl-4965.h   |  177 +
 .../net/wireless/mac80211/iwlwifi/iwl-channel.h    |  164 +
 .../net/wireless/mac80211/iwlwifi/iwl-commands.h   |  664 ++
 drivers/net/wireless/mac80211/iwlwifi/iwl-debug.h  |  141 +
 drivers/net/wireless/mac80211/iwlwifi/iwl-eeprom.h |  325 +
 .../net/wireless/mac80211/iwlwifi/iwl-helpers.h    |  250 +
 drivers/net/wireless/mac80211/iwlwifi/iwl-hw.h     | 1490 ++++
 drivers/net/wireless/mac80211/iwlwifi/iwl-io.h     |  454 ++
 drivers/net/wireless/mac80211/iwlwifi/iwl-priv.h   |  344 +
 .../net/wireless/mac80211/iwlwifi/iwl-spectrum.h   |   91 +
 drivers/net/wireless/mac80211/iwlwifi/iwlwifi.h    |  665 ++
 22 files changed, 20288 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/Kconfig
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/Makefile
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/base.c
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-3945-hw.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-3945-rs.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-3945.c
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-3945.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-4965-hw.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-4965.c
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-4965.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-channel.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-commands.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-debug.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-eeprom.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-helpers.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-hw.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-io.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-priv.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwl-spectrum.h
 create mode 100644 drivers/net/wireless/mac80211/iwlwifi/iwlwifi.h

NOTE:  The patch is 597k and can be found at:

	http://intellinuxwireless.org/iwlwifi/0001-Add-iwlwifi-wireless-drivers.patch

Patch is against wireless-dev commit-id be8662897~

Re: [PATCH] Add iwlwifi wireless drivers

[Randy Dunlap]

On Wed, 16 May 2007 14:45:32 -0700 James Ketrenos wrote:

> This patch adds the iwlwifi project directory and sources needed to 
> build the mac80211 based wireless drivers for the Intel PRO/Wireless 
> 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.
> 
> Signed-off-by: James Ketrenos <jketreno@linux.intel.com>
> ---
> 
> NOTE:  The patch is 597k and can be found at:
> 
> 	http://intellinuxwireless.org/iwlwifi/0001-Add-iwlwifi-wireless-drivers.patch
> 
> Patch is against wireless-dev commit-id be8662897~


Some comments/review, mostly not-code-related:

1.  Indent Kconfig help text with one tab + 2 spaces (not 8 spaces).

2.  Convert function comment blocks into kernel-doc.  E.g., this one:

+/**
+ * Deallocate DMA queue.
+ *
+ * Empty queue by removing and destroying all BD's.
+ * Free all buffers.
+ *
+ * @param dev
+ * @param q

(These param names are not correct.)

+ */

would be:

/**
 * iwl_tx_queue_free - Deallocate DMA queue
 * @priv:  describe this param
 * @txq:  describe this param
 *
 * Empty queue by removing and destroying all BD's.
 * Free all buffers.
 */

Preferably at least all non-static functions will be documented
with kernel-doc.

3.  Indent labels: with 0 or 1 space, not with 7 spaces.
Using 7 spaces hides them, makes them too difficult to see/find.

4.  Please don't use /** to begin comment blocks that are not
in kernel-doc format.

5.  Some inline functions may be too large.  E.g., this one:
+static inline int full_rxon_required(struct iwl_priv *priv)

6.  What does 0x40 mean here?  (don't use magic values)
+	if (res->hdr.flags & 0x40) {

7.  This test can be shortened (lots of instances):

+		if (is_broadcast_ether_addr(header->addr1) ||
+		    is_multicast_ether_addr(header->addr1))
+			return !compare_ether_addr(header->addr3, priv->bssid);

since (quoting include/linux/etherdevic.h):
 * By definition the broadcast address is also a multicast address.

8.  Multi-line comment format has a '*' on each line:
+/*
+  handle build REPLY_TX command notification.
+*/

so add '*' in lots of places.

9.  in is_duplicate_packet(), the
+	case IEEE80211_IF_TYPE_IBSS:{
block is indented one extra tab.

10. typo:
+ * When FW adwances 'R' index, all entries between old and
            advances

11. Don't indent #endif like this:

+#if IWL == 3945
+	u8 rate = beacon->beacon_notify_hdr.rate;
+#elif IWL == 4965
+	u8 rate = beacon->beacon_notify_hdr.rate.rate;
+	#endif

12.  36 lines end with trailing whitespace.  :(

13.  Need blank line before and after functions (several of these):

+#define ERROR_START_OFFSET  (1 * sizeof(u32))
+#define ERROR_ELEM_SIZE     (7 * sizeof(u32))
+static void iwl_dump_nic_error_log(struct iwl_priv *priv)
+{

14.  sysfs files should contain only 1 value.  Compare

+static ssize_t show_tune(struct device *d,
+			 struct device_attribute *attr, char *buf)
+{
+	struct iwl_priv *priv = (struct iwl_priv *)d->driver_data;
+
+	return sprintf(buf, "%d %d\n",
+		       priv->phymode, priv->active_rxon.channel);
+}

and show_channels()  [probably others].

15.  Limit source lines to <= 80 columns (this patch contains
over 200 lines that are > 80 columns).

16.  Don't use such obfuscated wrappers like these:

+#define IWL_NOP {}
+#define IWL_NOP_RET(x) { return x; }

just open-code them.


OK, my eyes are glazing over.  I'll check the rest of it later.

I hope that some of the wireless developers also review it (since
I'm not one).

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] Add iwlwifi wireless drivers

[Michael Wu]

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

On Wednesday 16 May 2007 17:45, James Ketrenos wrote:
> This patch adds the iwlwifi project directory and sources needed to
> build the mac80211 based wireless drivers for the Intel PRO/Wireless
> 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.
>
Looks much better than when I last checked. A few comments:

- The enter/leave stuff should be removed.
- Many of the in_interrupt() checks are unnecessary. Jiri recently documented 
which callbacks need to be atomic - the ones that don't need to be atomic 
definitely won't be called in an interrupt.
- Get rid of the stub functions for callbacks, like set_multicast_list and 
reset. mac80211 checks if those callbacks are defined before calling them.
- CONFIG_IWLWIFI_QOS doesn't appear to be defined.
- Read and set the MAC address before calling ieee80211_register_hw.
- flush_scheduled_work in d_stop can lead to deadlocks since RTNL is held. It 
looks like everything is running on a dedicated workqueue so that doesn't 
really even do anything. priv->workqueue should be flushed instead if needed.
- mac80211 (very recently) sets up a workqueue for the driver to use so you 
don't need to create your own anymore.
- param_auto_create looks very suspicious. mac80211 already creates an adhoc 
station if it can't join one.. what's this for?
- The suspend and resume callback setting in struct pci_driver iwl_driver 
needs to be put on two lines.
- Why are probe requests being dropped in adhoc mode? (assuming hardware 
handles it.. but the message output for that doesn't sound like it)
- The bitwise ORs shouldn't be getting a line to itself in iwl3945_rx_init and 
iwl3945_tx_reset.
- Mixed case variable name in iwl_hw_txq_ctx_stop: queueId.
- CONFIG_IWLWIFI_MONITOR and CONFIG_IEEE80211_RADIOTAP doesn't look right. 
- RX_FLAG_RADIOTAP should be set in the ieee80211_rx_status flag when a 
radiotap header is added to a frame. (otherwise, mac80211 will add its own 
radiotap header)
- If you don't want to add a radiotap header all the time, you should check 
the IEEE80211_CONF_RADIOTAP flag to see if a radiotap header is desired by 
the stack. (not whether or not we're configured as IEEE80211_IF_TYPE_MNTR)
- Splitting rx_start->phy_flags over two lines in iwl4965_rx_reply_rx doesn't 
look good.
- The IEEE80211_FTYPE_MGMT case in the switch statement inside 
iwl4965_rx_reply_rx doesn't need that extra set of braces. Also, the && 
and || don't need their own lines.
- param_channel looks suspicious too.
- ieee80211chan2mhz is provided by ieee80211_radiotap.h.

Haven't seen anything severe enough to be a blocker for wireless-dev, however.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add iwlwifi wireless drivers

[Jeff Garzik]

Michael Wu wrote:
> On Wednesday 16 May 2007 17:45, James Ketrenos wrote:
>> This patch adds the iwlwifi project directory and sources needed to
>> build the mac80211 based wireless drivers for the Intel PRO/Wireless
>> 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.
>>
> Looks much better than when I last checked. A few comments:
> 
> - The enter/leave stuff should be removed.

This is the maintainer's personal preference.  I put them in all my 
drivers, and they look perfectly fine here.

	Jeff

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Randy Dunlap]

On Wed, 16 May 2007 14:45:32 -0700 James Ketrenos wrote:

> This patch adds the iwlwifi project directory and sources needed to 
> build the mac80211 based wireless drivers for the Intel PRO/Wireless 
> 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.
> 
> Signed-off-by: James Ketrenos <jketreno@linux.intel.com>
> 
> NOTE:  The patch is 597k and can be found at:
> 
> 	http://intellinuxwireless.org/iwlwifi/0001-Add-iwlwifi-wireless-drivers.patch
> 
> Patch is against wireless-dev commit-id be8662897~


1.  Can't this:

+	if (sizeof(priv->eeprom) != 1024) {
+		IWL_ERROR("EEPROM structure size incorrect!\n");
+		return -EINVAL;
+	}

use this instead of 1024 ?
+#define IWL4965_EEPROM_IMAGE_SIZE  (0x200 * sizeof(u16))

2.  Use of bitfields:  bitfields are not (guaranteed to be) portable
across a wire interface to some other CPU architecture.
Are all bitfield uses always local to the running driver?
They should be.

3.  What are all of those "volatile" C keywords doing in struct
iwl_shared?
See http://lwn.net/Articles/232961/ : "The trouble with volatile"

4.  Don't list the parameters like this:

+static const struct iwl_channel_info *iwl4965_get_current_txpower_info(struct
+								       iwl_priv
+								       *priv,
+								       u8 *
+								       band,
+								       u8 *
+								       channel,
+								       u8 *
+								       is_fat,
+								       u8 *
+								       ctrl_chan_high)
+{

Just do more like this:

+static const struct iwl_channel_info *iwl4965_get_current_txpower_info(
+						struct iwl_priv *priv,
+						u8 *band,
+						u8 *channel,
+						u8 *is_fat,
+						u8 *ctrl_chan_high)
+{

5.  This function:

+static inline u8 iwl4965_get_dma_hi_address(dma_addr_t addr)
+{
+#ifdef _X86_64_TYPES_H
+	return addr >> 32;
+#else
+#ifdef _I386_TYPES_H
+	return 0;
+#else
+#error "unsupported architecture"
+#endif
+#endif
+}

a.  For i386, dma_addr_t can still be 64 bits.  See asm-i386/types.h:

#ifdef CONFIG_HIGHMEM64G
typedef u64 dma_addr_t;
#else
typedef u32 dma_addr_t;
#endif

b.  Should use CONFIG_64BIT instead of checking _X64_64_TYPES_H or
_I386_TYPES_H.

c.  can just shift addr 32 bits right (in 2 shifts) in all cases:

	return (addr >> 16) >> 16;

6.  No need to init rc:

+int iwl_hw_setup_bootstrap(struct iwl_priv *priv)
+{
+	int rc = 0;
+
+	/* Load bootstrap instructions into bootstrap state machine, for
+	 *   restoring after suspend/resume and rfkill power-downs */
+	rc = iwl4965_load_bsm(priv, priv->ucode_boot.v_addr,
+			      priv->ucode_boot.len);

7.  No need to init hwVersion:

+void iwl_hw_card_show_info(struct iwl_priv *priv)
+{
+	u16 hwVersion = 0;
+
+	hwVersion = priv->eeprom.board_revision_4965;

8.  Too many BUG_ON()s.  Try to recover or return -ERRORs instead.

9.  printk() calls should use KERN_* levels.

10. Don't use C99-style // comments.



and as Andrew's -mm announcements and my sig say, use
Documentation/SubmitChecklist to see what else needs to be done.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] Add iwlwifi wireless drivers

[Michael Wu]

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

On Wednesday 16 May 2007 22:56, Jeff Garzik wrote:
> This is the maintainer's personal preference.  I put them in all my
> drivers, and they look perfectly fine here.
>
I consider it generally ugly and to be avoided. These seem to be placed only 
in the mac80211 callbacks so it's more of a tool to figure out when mac80211 
uses callbacks.. doesn't really belong here IMHO.

But.. if James likes them, fine. I haven't seen anything better using the gcc 
mcount stuff Christoph Hellwig was talking about so this is the only option.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add iwlwifi wireless drivers

[James Ketrenos]

Randy Dunlap wrote:
> On Wed, 16 May 2007 14:45:32 -0700 James Ketrenos wrote:
> 
>> This patch adds the iwlwifi project directory and sources needed to 
>> build the mac80211 based wireless drivers for the Intel PRO/Wireless 
>> 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.
>>
>> Signed-off-by: James Ketrenos <jketreno@linux.intel.com>
>> ---
>>
>> NOTE:  The patch is 597k and can be found at:
>>
>> 	http://intellinuxwireless.org/iwlwifi/0001-Add-iwlwifi-wireless-drivers.patch
>>
>> Patch is against wireless-dev commit-id be8662897~
> 
> 
> Some comments/review, mostly not-code-related:
...
> 3.  Indent labels: with 0 or 1 space, not with 7 spaces.
> Using 7 spaces hides them, makes them too difficult to see/find.

I can correct this with:

	sed -i -e 's,^\(.*\)default:,foo\1default:,g' \
		-e 's,^[[:space:]]*\([[:alpha:]]\+[[:alnum:]]*:\)$, \1,g' \
		-e 's,^foo\(.*\)default:,\1default:,g' origin/*.[ch]

However scripts/Lindent undoes it.  Is there a way to keep Lindent from indenting labels?  When I looked at indent's man page it only talked about case labels.

...
> 12.  36 lines end with trailing whitespace.   :( 

I must have forgotten to run Lindent on the final code.  I'll run it (or just a sed to remove the trailing whitespace) on the code before I submit the version that addresses all the comments.

...
> 15.  Limit source lines to <= 80 columns (this patch contains
> over 200 lines that are > 80 columns).
> 

Confirmed 239 instances of > 80 columns by:

	for i in *.[ch]; do 
		echo $i
		sed -e 's,\t,        ,g' $i | 
			grep -n '^.\{80\}.\+$'
	done

I thought Lindent would handle that prettily, but I guess not.  

It seems to have enforced 80-column on some lines (sticking a bunch of right justified parameters) and on others it has made things a lot worse.   

Lindent seems to have converted space-justified column alignment into tab-justified column alignment, which frequently doesn't fit in 80 columns.  For example:

          1         2         3         4         5         6         7
01234567890123456789012345678901234567890123456789012345678901234567890123456789
int param_antenna = 0;<-10spc->/* def: 0 = both antennas (use diversity) */

was changed to:

         1         2         3         4         5         6         7
01234567890123456789012345678901234567890123456789012345678901234567890123456789
int param_antenna = 0;<-  two tabs  ->/* def: 0 = both antennas (use diversity) */

Is there another tool people use besides Lindent to enforce style?
 
Thanks,
James

Re: [PATCH] Add iwlwifi wireless drivers

[Jeff Garzik]

Lindent is only the first step.

You then have to go through the code and undo the damage it does.

For instance,

> +               rc = wait_event_interruptible_timeout(priv->
> +                                                     wait_command_queue,
> +                                                     !(priv->
> +                                                       status &
> +                                                       STATUS_HCMD_ACTIVE),
> +                                                     HOST_COMPLETE_TIMEOUT);

you SHOULD (using IETF RFC language) violate the 80-column limit, when 
the alternative is word-wrapping at silly boundaries like the middle of 
a de-ref.

Or,

> +       iwl_write_restricted(priv, FH_RCSR_CONFIG(0),
> +                            ALM_FH_RCSR_RX_CONFIG_REG_VAL_DMA_CHNL_EN_ENABLE
> +                            |
> +                            ALM_FH_RCSR_RX_CONFIG_REG_VAL_RDRBD_EN_ENABLE
> +                            |
> +                            ALM_FH_RCSR_RX_CONFIG_REG_BIT_WR_STTS_EN
> +                            |
> +                            ALM_FH_RCSR_RX_CONFIG_REG_VAL_MAX_FRAG_SIZE_128
> +                            | (RX_QUEUE_SIZE_LOG <<
> +                               ALM_FH_RCSR_RX_CONFIG_REG_POS_RBDC_SIZE)
> +                            |
> +                            ALM_FH_RCSR_RX_CONFIG_REG_VAL_IRQ_DEST_INT_HOST
> +                            | (1 << ALM_FH_RCSR_RX_CONFIG_REG_POS_IRQ_RBTH)
> +                            | ALM_FH_RCSR_RX_CONFIG_REG_VAL_MSG_MODE_FH);
> +

you SHOULD NOT waste an entire line on a single '|'.

As an aside, remove the redundant "_REG_BIT_" and "_REG_VAL_" stuff.  It 
makes too-long symbols even longer.

As an aside #2,

> +#if IWL == 4965
> +           (rxon1->ofdm_ht_single_stream_basic_rates ==
> +            rxon2->ofdm_ht_single_stream_basic_rates) &&
> +           (rxon1->ofdm_ht_dual_stream_basic_rates ==
> +            rxon2->ofdm_ht_dual_stream_basic_rates) &&
> +           (rxon1->rx_chain == rxon2->rx_chain) &&
> +#endif


those #if's should go away.

	Jeff

Re: [PATCH] Add iwlwifi wireless drivers

[Christoph Hellwig]

On Wed, May 16, 2007 at 09:51:28PM -0400, Michael Wu wrote:
> On Wednesday 16 May 2007 17:45, James Ketrenos wrote:
> > This patch adds the iwlwifi project directory and sources needed to
> > build the mac80211 based wireless drivers for the Intel PRO/Wireless
> > 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.
> >
> Looks much better than when I last checked. A few comments:
> 
> - The enter/leave stuff should be removed.
> - Many of the in_interrupt() checks are unnecessary. Jiri recently documented 
> which callbacks need to be atomic - the ones that don't need to be atomic 
> definitely won't be called in an interrupt.

Actually not some but all.  Drivers should never use in_interrupt().

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Stephen Hemminger]

On Wed, 16 May 2007 20:35:05 -0700
Randy Dunlap <randy.dunlap@oracle.com> wrote:

> On Wed, 16 May 2007 14:45:32 -0700 James Ketrenos wrote:
> 
> > This patch adds the iwlwifi project directory and sources needed to 
> > build the mac80211 based wireless drivers for the Intel PRO/Wireless 
> > 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.
> > 
> > Signed-off-by: James Ketrenos <jketreno@linux.intel.com>
> > 
> > NOTE:  The patch is 597k and can be found at:
> > 
> > 	http://intellinuxwireless.org/iwlwifi/0001-Add-iwlwifi-wireless-drivers.patch
> > 
> > Patch is against wireless-dev commit-id be8662897~
> 
> 
> 1.  Can't this:
> 
> +	if (sizeof(priv->eeprom) != 1024) {
> +		IWL_ERROR("EEPROM structure size incorrect!\n");
> +		return -EINVAL;
> +	}
> 
> use this instead of 1024 ?
> +#define IWL4965_EEPROM_IMAGE_SIZE  (0x200 * sizeof(u16))

Why not BUILD_BUG_ON()?


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Stephen Hemminger]

On Wed, 16 May 2007 20:35:05 -0700
Randy Dunlap <randy.dunlap@oracle.com> wrote:


> 
> 5.  This function:
> 
> +static inline u8 iwl4965_get_dma_hi_address(dma_addr_t addr)
> +{
> +#ifdef _X86_64_TYPES_H
> +	return addr >> 32;
> +#else
> +#ifdef _I386_TYPES_H
> +	return 0;
> +#else
> +#error "unsupported architecture"
> +#endif
> +#endif
> +}
> 
> a.  For i386, dma_addr_t can still be 64 bits.  See asm-i386/types.h:
> 
> #ifdef CONFIG_HIGHMEM64G
> typedef u64 dma_addr_t;
> #else
> typedef u32 dma_addr_t;
> #endif
> 
> b.  Should use CONFIG_64BIT instead of checking _X64_64_TYPES_H or
> _I386_TYPES_H.
> 
> c.  can just shift addr 32 bits right (in 2 shifts) in all cases:
> 
> 	return (addr >> 16) >> 16;
>

I did this in sky2 driver:

/* Return high part of DMA address (could be 32 or 64 bit) */
static inline u32 high32(dma_addr_t a)
{
	return sizeof(a) > sizeof(u32) ? (a >> 16) >> 16 : 0;
}



-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Johannes Berg]

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

On Thu, 2007-05-17 at 08:05 -0700, Stephen Hemminger wrote:

> I did this in sky2 driver:
> 
> /* Return high part of DMA address (could be 32 or 64 bit) */
> static inline u32 high32(dma_addr_t a)
> {
> 	return sizeof(a) > sizeof(u32) ? (a >> 16) >> 16 : 0;
> }

Note that akpm recently added a macro/inline for this to the tree which
IIRC went to Linus.

johannes

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

Re: [PATCH] Add iwlwifi wireless drivers

[James Ketrenos]

Randy Dunlap wrote:
>> 	http://intellinuxwireless.org/iwlwifi/0001-Add-iwlwifi-wireless-drivers.patch
> Some comments/review, mostly not-code-related:
> 

Here is the current status (everything not described below has been fixed in GIT, and is available in the patch at http://intellinuxwireless.org/iwlwifi/0001-v2-Add-iwlwifi-wireless-drivers.patch:

> 2.  Convert function comment blocks into kernel-doc.  E.g., this one:
Fixed this part.
...
> Preferably at least all non-static functions will be documented
> with kernel-doc.

I updated and added some of these.  Improving the kernel-doc (and comments in general) over time is on the list of things to do.

> 14.  sysfs files should contain only 1 value.  Compare
> 
> +static ssize_t show_tune(struct device *d,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct iwl_priv *priv = (struct iwl_priv *)d->driver_data;
> +
> +	return sprintf(buf, "%d %d\n",
> +		       priv->phymode, priv->active_rxon.channel);
> +}
> 
> and show_channels()  [probably others].

I've modified show/store_tune to take and return a single value encoding the band and channel (since they are locked together and can't be set separately without introducing temporary state logic; tuning to channel 6 could be done in the 2.4GHz band or the 5.2GHz band)

I've added fixing show_channels to the list of things to change; the approach currently used has been *very* useful, without any additional tools or scripts, to be able to just cat a file and see the supported channel map by the driver.  Having to do an ls on a directory to see the bands, and within each band to do an ls to see the channels, and within each channel to then have a single flags attribute seems painful from a system overhead as well as usability standpoint (although making a graphical tool on top of it would be easier...)

We also need to look at cleaning up / fixing how the measurement requests can be performed, etc.  

The power_level sysfs attribute currently has a single parameter for setting it, and in showing it it provides back the single parameter and a text translation with additional information about the setting.  Ex (with a slight change I committed vs. what was in the original patch):

% echo 3 > /sys/bus/pci/drivers/iwl3945/*/power_level
% cat /sys/bus/pci/drivers/iwl3945/*/power_level
3 (Timeout 75ms, Period 1000ms)

> 15.  Limit source lines to <= 80 columns (this patch contains
> over 200 lines that are > 80 columns).

Fixed in the .c files.  Still have 155 instances in the .h files (most due to mid-line tabs)

---

Are you OK with iwlwifi addressing the above post-merge into wireless-dev?

James

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[James Ketrenos]

Randy Dunlap wrote:
> On Wed, 16 May 2007 14:45:32 -0700 James Ketrenos wrote:
> 
>> This patch adds the iwlwifi project directory and sources needed to 
>> build the mac80211 based wireless drivers for the Intel PRO/Wireless 
>> 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.
>>
>> Signed-off-by: James Ketrenos <jketreno@linux.intel.com>
>>
>> NOTE:  The patch is 597k and can be found at:
>>
>> 	http://intellinuxwireless.org/iwlwifi/0001-Add-iwlwifi-wireless-drivers.patch
>>
>> Patch is against wireless-dev commit-id be8662897~
> 
> 
> 1.  Can't this:
> +	if (sizeof(priv->eeprom) != 1024) {
...
> use this instead of 1024 ?
> +#define IWL4965_EEPROM_IMAGE_SIZE  (0x200 * sizeof(u16))

Changed to BUILD_BUG_ON(sizeof(priv->eeprom) != IWL_EEPROM_IMAGE_SIZE) and defined IWL_EEPROM_IMAGE_SIZE in iwl-eeprom.h

> 2.  Use of bitfields:  bitfields are not (guaranteed to be) portable
> across a wire interface to some other CPU architecture.
> Are all bitfield uses always local to the running driver?
> They should be.

The bitfields are between the hw/ucode (__le) and the driver.  Are you saying the bitfields won't be compatible if the driver is compiled and running on big endian?  Shouldn't le16_to_cpu() fix that?  I just noticed the types in iwl-4965-hw.h weren't endian specific; I've updated that (u16, u32, u64 -> __le16, __le32, __le64)

> 3.  What are all of those "volatile" C keywords doing in struct
> iwl_shared?
> See http://lwn.net/Articles/232961/ : "The trouble with volatile"

If a reference shared through memory mapped IO with a device that can change the value at any time, doesn't it need to be volatile?  'struct iwl_shared' is allocated with pci_alloc_consistent and the physical address is provided to the hardware for it to update whenever it sees fit.

> 4.  Don't list the parameters like this:

Lindent == bad.  I've fixed the instances of this munging I've come across.

> 5.  This function:
> 
> +static inline u8 iwl4965_get_dma_hi_address(dma_addr_t addr)

Fixed to do what Stephen did in sky2.

> 6.  No need to init rc:

Fixed those and several others.  I'm sure there are more we'll have to weed out over time (unless you know a sparse option that will say 'initializing variable unnecessarily')

> 8.  Too many BUG_ON()s.  Try to recover or return -ERRORs instead.

I pulled out two that shouldn't have been there.

The rest are currently there map to bugs which are easily introduced through code changes but which may not cause an immediately visible bug at run-time.  Typically they are placed where an eventual printk() at that location during debugging of a user bug resulted in the critical "ah-ha!" that led to fixing it.  I've added comments to the BUG_ON's that didn't have comments already.

I suppose we could try and introduce a new driver recovery mechanism that, upon detection, throws a stack trace and then shuts down the driver (vs. throwing a kernel OOPs) requiring user space to unload and reload the driver.  That would still be a sucky situation, but at least it wouldn't cause a system failure requiring a reboot...  does such a mechanism already exist?

> 
> 9.  printk() calls should use KERN_* levels.
Fixed.

> 10. Don't use C99-style // comments.
Fixed.

> and as Andrew's -mm announcements and my sig say, use
> Documentation/SubmitChecklist to see what else needs to be done.

Will do.

Above changes in GIT (http://intellinuxwireless.org/repos/iwlwifi.git/)

Thanks,
James

Re: [PATCH] Add iwlwifi wireless drivers

[James Ketrenos]

Jeff Garzik wrote:
> James Ketrenos wrote:
>> Randy Dunlap wrote:
>>> 15.  Limit source lines to <= 80 columns (this patch contains
>>> over 200 lines that are > 80 columns).
>>
>> Fixed in the .c files.  Still have 155 instances in the .h files (most 
>> due to mid-line tabs)
> 
> See my email on this subject.  This is NOT a hard and fast rule.  If you 
> have to do something stupid to the formatting to get it to fit, then you 
> are moving in the wrong direction.

The bulk of the problems in the .h files are caused by Lindent injecting tabs instead of spaces between the C language item and the comment describing it, blowing the comments over 80.

That said, some of the define names are horrendously long... over time we can get those cleaned up w/ some sed scripts.

> As an aside, sometimes long lines are an indication that a function 
> needs to be broken up into smaller sub-functions.

Nod.

James

Re: [PATCH] Add iwlwifi wireless drivers

[Jeff Garzik]

James Ketrenos wrote:
> Randy Dunlap wrote:
>> 15.  Limit source lines to <= 80 columns (this patch contains
>> over 200 lines that are > 80 columns).
> 
> Fixed in the .c files.  Still have 155 instances in the .h files (most 
> due to mid-line tabs)


See my email on this subject.  This is NOT a hard and fast rule.  If you 
have to do something stupid to the formatting to get it to fit, then you 
are moving in the wrong direction.

As an aside, sometimes long lines are an indication that a function 
needs to be broken up into smaller sub-functions.

	Jeff

Re: [PATCH] Add iwlwifi wireless drivers

[Randy Dunlap]

On Fri, 18 May 2007 12:01:16 -0700 James Ketrenos wrote:

> Randy Dunlap wrote:
> >> 	http://intellinuxwireless.org/iwlwifi/0001-Add-iwlwifi-wireless-drivers.patch
> > Some comments/review, mostly not-code-related:
> > 
> 
> Here is the current status (everything not described below has been fixed in GIT, and is available in the patch at http://intellinuxwireless.org/iwlwifi/0001-v2-Add-iwlwifi-wireless-drivers.patch:
> 

[specific updates clipped]

> Are you OK with iwlwifi addressing the above post-merge into wireless-dev?

/me points to Mr. Linville.  :)

There are still some review items that need quick attention IMO,
like the 32/64-bit dma_addr_t issue.

However, I'm in favor of merging into wireless-dev if that's what will
speed up general intel wireless merging and maintainence on a more
timely basis instead of seeing giant patches every few months.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[James Ketrenos]

Jeff Garzik wrote:
> James Ketrenos wrote:
>> Randy Dunlap wrote:
>>> 2.  Use of bitfields:  bitfields are not (guaranteed to be) portable
>>> across a wire interface to some other CPU architecture.
>>> Are all bitfield uses always local to the running driver?
>>> They should be.
>>
>> The bitfields are between the hw/ucode (__le) and the driver.  Are you 
>> saying the bitfields won't be compatible if the driver is compiled and 
>> running on big endian?  Shouldn't le16_to_cpu() fix that?  I just 
>> noticed the types in iwl-4965-hw.h weren't endian specific; I've 
>> updated that (u16, u32, u64 -> __le16, __le32, __le64)
> 
> Use of bitfields, as defined in the C standard, should be avoided at all 
> costs.  It requires you to define structures twice -- once for LE, and 
> once for BE.  It generates demonstrably sub-optimal code on all known 
> compilers.  And in general, people tend to screw them up.
> 
> Drivers should be using bitwise integer math rather than bitfields.
> 
> As for u8/u16/u32/u64 integer mixes, you must still pay attention to 
> structure layout and compiler padding, in addition to endian issues 
> (which are lessened once bitfields are gone).

Alright; I'll add replacing the bitfields with bitwise access to the todo list.

Are you OK with us marking the driver as not working on big endian for now and getting this one fixed over time?

>>> 3.  What are all of those "volatile" C keywords doing in struct
>>> iwl_shared?
>>> See http://lwn.net/Articles/232961/ : "The trouble with volatile"
>>
>> If a reference shared through memory mapped IO with a device that can 
>> change the value at any time, doesn't it need to be volatile?  'struct 
>> iwl_shared' is allocated with pci_alloc_consistent and the physical 
>> address is provided to the hardware for it to update whenever it sees 
>> fit.
> 
> volatile should not be in your driver at all.
> 
> See the innumerable threads on LKML, including one current one.

I tried reading through the thread.  From what I gathered, it sounds like if I want to read a value from a memory location where the adapter could be writing to it I would just call rmb() before I access the memory?  And when writing to a value, call wmb() immediately after I write to it?  And expunge 'volatile' from my vocabulary.

> 
>>> 8.  Too many BUG_ON()s.  Try to recover or return -ERRORs instead.
>>
>> I pulled out two that shouldn't have been there.
>>
>> The rest are currently there map to bugs which are easily introduced 
>> through code changes but which may not cause an immediately visible 
>> bug at run-time.  Typically they are placed where an eventual printk() 
>> at that location during debugging of a user bug resulted in the 
>> critical "ah-ha!" that led to fixing it.  I've added comments to the 
>> BUG_ON's that didn't have comments already.
>>
>> I suppose we could try and introduce a new driver recovery mechanism 
>> that, upon detection, throws a stack trace and then shuts down the 
>> driver (vs. throwing a kernel OOPs) requiring user space to unload and 
>> reload the driver.  That would still be a sucky situation, but at 
>> least it wouldn't cause a system failure requiring a reboot...  does 
>> such a mechanism already exist?
> 
> In general, you should not be checking for programmer mistakes at every 
> level.  Only use BUG_ON() for mistakes that have happened in the past in 
> this driver, AND are likely to occur in the future, AND do not cause 
> problems easily detectable simply by allowing the code to screw up.

That has been the general goal in where the BUG_ONs are.  

There were a couple that weren't doing that (which were yanked).  

I've updated iwl_send_cmd which was using BUG_ON but could use printk and return a failure code.

James

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Jeff Garzik]

James Ketrenos wrote:
> Randy Dunlap wrote:
>> 2.  Use of bitfields:  bitfields are not (guaranteed to be) portable
>> across a wire interface to some other CPU architecture.
>> Are all bitfield uses always local to the running driver?
>> They should be.
> 
> The bitfields are between the hw/ucode (__le) and the driver.  Are you 
> saying the bitfields won't be compatible if the driver is compiled and 
> running on big endian?  Shouldn't le16_to_cpu() fix that?  I just 
> noticed the types in iwl-4965-hw.h weren't endian specific; I've updated 
> that (u16, u32, u64 -> __le16, __le32, __le64)

Use of bitfields, as defined in the C standard, should be avoided at all 
costs.  It requires you to define structures twice -- once for LE, and 
once for BE.  It generates demonstrably sub-optimal code on all known 
compilers.  And in general, people tend to screw them up.

Drivers should be using bitwise integer math rather than bitfields.

As for u8/u16/u32/u64 integer mixes, you must still pay attention to 
structure layout and compiler padding, in addition to endian issues 
(which are lessened once bitfields are gone).


>> 3.  What are all of those "volatile" C keywords doing in struct
>> iwl_shared?
>> See http://lwn.net/Articles/232961/ : "The trouble with volatile"
> 
> If a reference shared through memory mapped IO with a device that can 
> change the value at any time, doesn't it need to be volatile?  'struct 
> iwl_shared' is allocated with pci_alloc_consistent and the physical 
> address is provided to the hardware for it to update whenever it sees fit.

volatile should not be in your driver at all.

See the innumerable threads on LKML, including one current one.


>> 8.  Too many BUG_ON()s.  Try to recover or return -ERRORs instead.
> 
> I pulled out two that shouldn't have been there.
> 
> The rest are currently there map to bugs which are easily introduced 
> through code changes but which may not cause an immediately visible bug 
> at run-time.  Typically they are placed where an eventual printk() at 
> that location during debugging of a user bug resulted in the critical 
> "ah-ha!" that led to fixing it.  I've added comments to the BUG_ON's 
> that didn't have comments already.
> 
> I suppose we could try and introduce a new driver recovery mechanism 
> that, upon detection, throws a stack trace and then shuts down the 
> driver (vs. throwing a kernel OOPs) requiring user space to unload and 
> reload the driver.  That would still be a sucky situation, but at least 
> it wouldn't cause a system failure requiring a reboot...  does such a 
> mechanism already exist?

In general, you should not be checking for programmer mistakes at every 
level.  Only use BUG_ON() for mistakes that have happened in the past in 
this driver, AND are likely to occur in the future, AND do not cause 
problems easily detectable simply by allowing the code to screw up.


>> 9.  printk() calls should use KERN_* levels.
> Fixed.

dev_xxx stuff is preferred over printk(), actually :)

	Jeff

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Randy Dunlap]

On Fri, 18 May 2007 13:33:57 -0700 James Ketrenos wrote:

> > 2.  Use of bitfields:  bitfields are not (guaranteed to be) portable
> > across a wire interface to some other CPU architecture.
> > Are all bitfield uses always local to the running driver?
> > They should be.
> 
> The bitfields are between the hw/ucode (__le) and the driver.  Are you saying the bitfields won't be compatible if the driver is compiled and running on big endian?  Shouldn't le16_to_cpu() fix that?  I just noticed the types in iwl-4965-hw.h weren't endian specific; I've updated that (u16, u32, u64 -> __le16, __le32, __le64)


I'm just saying that something like
	struct bits {
		u32	priority :4;
		u32	flags :12;
		u32	direction :1;
		u32	status :15;
	};
when built on x86[-64] and then sent over a network connection
to a big-endian machine won't necessarily be looked at correctly
on the receiving machine.  There is no guarantee of bitfield order
from one platform to another.

> > 3.  What are all of those "volatile" C keywords doing in struct
> > iwl_shared?
> > See http://lwn.net/Articles/232961/ : "The trouble with volatile"
> 
> If a reference shared through memory mapped IO with a device that can change the value at any time, doesn't it need to be volatile?  'struct iwl_shared' is allocated with pci_alloc_consistent and the physical address is provided to the hardware for it to update whenever it sees fit.

That's a good question.  You probably know as much about it as I do.

Does the driver access those mmIO regions by something direct like
	*mmio =
or does the driver use some kind of access routines to read/write
the mmIO regions, such as readb()/writeb()?

If the former, then I expect that those should be changed/fixed,
but I'd prefer to have Jeff G. answer that if he would.


> > 4.  Don't list the parameters like this:
> 
> Lindent == bad.  I've fixed the instances of this munging I've come across.

Indeed.  Too bad.

> > 8.  Too many BUG_ON()s.  Try to recover or return -ERRORs instead.
> 
> I pulled out two that shouldn't have been there.
> 
> The rest are currently there map to bugs which are easily introduced through code changes but which may not cause an immediately visible bug at run-time.  Typically they are placed where an eventual printk() at that location during debugging of a user bug resulted in the critical "ah-ha!" that led to fixing it.  I've added comments to the BUG_ON's that didn't have comments already.
> 
> I suppose we could try and introduce a new driver recovery mechanism that, upon detection, throws a stack trace and then shuts down the driver (vs. throwing a kernel OOPs) requiring user space to unload and reload the driver.  That would still be a sucky situation, but at least it wouldn't cause a system failure requiring a reboot...  does such a mechanism already exist?

You can call dump_stack() or use WARN_ON(), but you'll have roll your
own shut-down-the-driver methods AFAIK.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Jeff Garzik]

James Ketrenos wrote:
> Are you OK with us marking the driver as not working on big endian for 
> now and getting this one fixed over time?

No.

Getting this right is a key merge requirement for all drivers.

Getting this right has also, in the past, been a proven way to find 
hidden bugs in your driver that wanted fixing anyway.


> I tried reading through the thread.  From what I gathered, it sounds 
> like if I want to read a value from a memory location where the adapter 
> could be writing to it I would just call rmb() before I access the 
> memory?  And when writing to a value, call wmb() immediately after I 
> write to it?  And expunge 'volatile' from my vocabulary.

MMIO - no.  Accessors handle that for you in most cases.  For rare cases 
you might need mmiowb() or whatnot, but that is unlikely.

DMA, generally that's what people do, but it's not perfect.

	Jeff

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Christoph Hellwig]

On Fri, May 18, 2007 at 02:31:54PM -0700, James Ketrenos wrote:
> Alright; I'll add replacing the bitfields with bitwise access to the todo 
> list.
> 
> Are you OK with us marking the driver as not working on big endian for now 
> and getting this one fixed over time?

Given that Intel still hasn't fixed the older ipw2X00 drivers for big
endian despite clong-time onstant prodding I'm dead set against this.
It just won't happen if you don't do it now.

Re: [PATCH] Add iwlwifi wireless drivers

[Christoph Hellwig]

On Wed, May 16, 2007 at 02:45:32PM -0700, James Ketrenos wrote:
> drivers/net/wireless/mac80211/Kconfig              |    1 +
> drivers/net/wireless/mac80211/Makefile             |    1 +
> drivers/net/wireless/mac80211/iwlwifi/Kconfig      |   96 +
> drivers/net/wireless/mac80211/iwlwifi/Makefile     |   27 +
> drivers/net/wireless/mac80211/iwlwifi/base.c       | 8146 

You're not reading linux-wireless a lot, are you?  The last
driver submission got a pretty clear answer that there should not
be a drivers/net/wireless/mac80211/ subdirectory.

Re: [PATCH] Add iwlwifi wireless drivers

[Michael Wu]

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

On Friday 18 May 2007 16:05, Christoph Hellwig wrote:
> On Wed, May 16, 2007 at 02:45:32PM -0700, James Ketrenos wrote:
> > drivers/net/wireless/mac80211/Kconfig              |    1 +
> > drivers/net/wireless/mac80211/Makefile             |    1 +
> > drivers/net/wireless/mac80211/iwlwifi/Kconfig      |   96 +
> > drivers/net/wireless/mac80211/iwlwifi/Makefile     |   27 +
> > drivers/net/wireless/mac80211/iwlwifi/base.c       | 8146
>
> You're not reading linux-wireless a lot, are you?  The last
> driver submission got a pretty clear answer that there should not
> be a drivers/net/wireless/mac80211/ subdirectory.
>
This is just for wireless-dev. When it's being pushed for wireless-2.6 and 
upstream it should go away then.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[James Ketrenos]

Christoph Hellwig wrote:
> On Fri, May 18, 2007 at 02:31:54PM -0700, James Ketrenos wrote:
>> Alright; I'll add replacing the bitfields with bitwise access to the todo 
>> list.
>>
>> Are you OK with us marking the driver as not working on big endian for now 
>> and getting this one fixed over time?
> 
> Given that Intel still hasn't fixed the older ipw2X00 drivers for big
> endian despite clong-time onstant prodding I'm dead set against this.

Really?  Long time constant prodding?

I'll admit I'm not CC:'d on every email regarding the ipw2100 and ipw2200, so if I missed a long-time and constant thread, please forward it my way.  

Searching Google for 'ipw endian' (and several other searches) found a reference to a fix for an endian issue when it was raised for the ipw2200.  For the ipw2100 I see a post back in 2004 by one person asking if anyone had tried the ipw2100 on a big endian system.

> It just won't happen if you don't do it now.

Well, if you have evidence to this then great.  If not, please don't spread FUD.

Historical footnote #1 -- the ipw2200 driver was added to the kernel w/out big endian support:
	commit: 43f66a6ce8da299344cf1bc2ac2311889cc88555

Historical footnote #2 -- we fixed big endian problems with the driver in August 2005
	commit: a613bffd3aac89bb0a8c9b7afa72af9b0ae30f0a (and a few other commits as users reported issues)

For the ipw2100, aside from one post in 2004 referencing it, I don't see anything.

James

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Christoph Hellwig]

On Mon, May 21, 2007 at 07:56:10AM -0700, James Ketrenos wrote:
> Christoph Hellwig wrote:
> >On Fri, May 18, 2007 at 02:31:54PM -0700, James Ketrenos wrote:
> >>Alright; I'll add replacing the bitfields with bitwise access to the todo 
> >>list.
> >>
> >>Are you OK with us marking the driver as not working on big endian for 
> >>now and getting this one fixed over time?
> >
> >Given that Intel still hasn't fixed the older ipw2X00 drivers for big
> >endian despite clong-time onstant prodding I'm dead set against this.
> 
> Really?  Long time constant prodding?
> 
> I'll admit I'm not CC:'d on every email regarding the ipw2100 and ipw2200, 
> so if I missed a long-time and constant thread, please forward it my way.  
> Searching Google for 'ipw endian' (and several other searches) found a 
> reference to a fix for an endian issue when it was raised for the ipw2200.  

Just from my outbox I have a thread in August 2006 where I tell Zhu Yi
to fix the endianess problems instead of masking them, and even offer
testing on big endian when Intel sends me hardware.  Despite that this
got completely ignored and resubmitted in december, where I replied again.
And probably quite a few more I haven't caught.

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[James Ketrenos]

Christoph Hellwig wrote:
> On Mon, May 21, 2007 at 07:56:10AM -0700, James Ketrenos wrote:
>> Christoph Hellwig wrote:
>>> On Fri, May 18, 2007 at 02:31:54PM -0700, James Ketrenos wrote:
>>>> Alright; I'll add replacing the bitfields with bitwise access to the todo 
>>>> list.
>>>>
>>>> Are you OK with us marking the driver as not working on big endian for 
>>>> now and getting this one fixed over time?
>>> Given that Intel still hasn't fixed the older ipw2X00 drivers for big
>>> endian despite clong-time onstant prodding I'm dead set against this.
>> Really?  Long time constant prodding?
>>
>> I'll admit I'm not CC:'d on every email regarding the ipw2100 and ipw2200, 
>> so if I missed a long-time and constant thread, please forward it my way.  
>> Searching Google for 'ipw endian' (and several other searches) found a 
>> reference to a fix for an endian issue when it was raised for the ipw2200.  
> 
> Just from my outbox I have a thread in August 2006 where I tell Zhu Yi
> to fix the endianess problems instead of masking them, and even offer
> testing on big endian when Intel sends me hardware.  Despite that this
> got completely ignored and resubmitted in december, where I replied again.
> And probably quite a few more I haven't caught.

Ok, first, you said 'ipw2x00'  Which is false.  The ipw2200 does work on big endian.  We have worked to enable it.  You also said merging w/out fixing big endian leads to big endian never being fixed.  Again, false (see prior email with specific references demonstrating that). 
So, Please don't generalize and make false accusations.

Second, regarding your prior thread, you more or less demanded that the driver maintainer send you hardware.  That isn't fair or reasonable.

If you have a bug, and users that can help resolve the bug, we'd love to work with them fix the issues.  But demanding that Yi send you hardware and then making stuff up about him and the ipw2100, ipw2200, and iwlwifi projects when he doesn't is really not appropriate.

James 

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Christoph Hellwig]

On Mon, May 21, 2007 at 09:48:30AM -0700, James Ketrenos wrote:
> Second, regarding your prior thread, you more or less demanded that the 
> driver maintainer send you hardware.  That isn't fair or reasonable.

No, that's entirely false.  I'd more than happy to have it fixed without
me beeing involved.  I offered to test it in case it's really that
difficult to get a foreign endian machine inside intel.

RE: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Cohen, Guy]

Hi,

Support for big-endian platforms (as well as AP mode) for iwlwifi is
currently *planned* to be added by Intel in the near future for several
purposes.

However, the vast majority of iwlwifi users at this point run it over
little-endian platforms. _Delaying_ the inclusion of iwlwifi driver will
not assist iwlwifi-over-big-endian-platform users in any way, but the
impact on the rest of the users is clear.

We got many comments from the community and we thank you for that. We do
our best to fix all of them in timely manner for the benefit of
everyone. If something falls between the cracks it is only because of
limited resources, priority issues, human mistakes or alike and not
because of flippancy or anything like that. I hope you understand that.

Guy.

PS
I marked *planned* with asterisks so no one mixes it by mistake with
*committed* and attack me if Intel stops making HW and SW, etc :)


On 5/21/07, Christoph Hellwig <hch@infradead.org> wrote:
>On Mon, May 21, 2007 at 09:48:30AM -0700, James Ketrenos wrote:
>> Second, regarding your prior thread, you more or less demanded that
the
>> driver maintainer send you hardware.  That isn't fair or reasonable.
>
> No, that's entirely false.  I'd more than happy to have it fixed
without
> me beeing involved.  I offered to test it in case it's really that
> difficult to get a foreign endian machine inside intel.

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Jeff Garzik]

Cohen, Guy wrote:
> Support for big-endian platforms (as well as AP mode) for iwlwifi is
> currently *planned* to be added by Intel in the near future for several
> purposes.

> However, the vast majority of iwlwifi users at this point run it over
> little-endian platforms. _Delaying_ the inclusion of iwlwifi driver will
> not assist iwlwifi-over-big-endian-platform users in any way, but the
> impact on the rest of the users is clear.


All Linux drivers that use the proper APIs are transparently portable to 
either little endian or big endian, 32-bit or 64-bit machines.

If your driver is not portable, then it is not using the proper APIs, 
and needs updating.

Next time, write the driver using the correct APIs and you will not run 
into these sort of problems...

	Jeff

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Randy Dunlap]

Jeff Garzik wrote:
> Cohen, Guy wrote:
>> Support for big-endian platforms (as well as AP mode) for iwlwifi is
>> currently *planned* to be added by Intel in the near future for several
>> purposes.
> 
>> However, the vast majority of iwlwifi users at this point run it over
>> little-endian platforms. _Delaying_ the inclusion of iwlwifi driver will
>> not assist iwlwifi-over-big-endian-platform users in any way, but the
>> impact on the rest of the users is clear.
> 
> 
> All Linux drivers that use the proper APIs are transparently portable to 
> either little endian or big endian, 32-bit or 64-bit machines.
> 
> If your driver is not portable, then it is not using the proper APIs, 
> and needs updating.
> 
> Next time, write the driver using the correct APIs and you will not run 
> into these sort of problems...

Ack.  Basically same idea for coding style issues also.
Do it right the first time, don't have to redo it.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Joerg Mayer]

On Mon, May 21, 2007 at 11:12:17PM +0300, Cohen, Guy wrote:
> However, the vast majority of iwlwifi users at this point run it over
> little-endian platforms.

That would be 100% as it doesn't yet support big-endian platforms?

 ciao
     Joerg

-- 
Joerg Mayer                                           <jmayer@loplof.de>
We are stuck with technology when what we really want is just stuff that
works. Some say that should read Microsoft instead of technology.

RE: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Cohen, Guy]

From: Randy Dunlap [mailto:randy.dunlap@oracle.com]
>Jeff Garzik wrote:
>> Cohen, Guy wrote:
>>> Support for big-endian platforms (as well as AP mode) for iwlwifi is
>>> currently *planned* to be added by Intel in the near future for
several
>>> purposes.
>>
>>> However, the vast majority of iwlwifi users at this point run it
over
>>> little-endian platforms. _Delaying_ the inclusion of iwlwifi driver
will
>>> not assist iwlwifi-over-big-endian-platform users in any way, but
the
>>> impact on the rest of the users is clear.
>>
>>
>> All Linux drivers that use the proper APIs are transparently portable
to
>> either little endian or big endian, 32-bit or 64-bit machines.
>>
>> If your driver is not portable, then it is not using the proper APIs,
>> and needs updating.
>>
>> Next time, write the driver using the correct APIs and you will not
run
>> into these sort of problems...
>
>Ack.  Basically same idea for coding style issues also.
>Do it right the first time, don't have to redo it.
>

You two are right. Any other reply from my side will be dumb... We will
learn the lesson...

But at this point (we can't change the past nor the present), we would
like to focus on more urgent issues with iwlwifi in the immediate term
and delay a bit the big-endian support. Is this acceptable?

>--
>~Randy
>*** Remember to use Documentation/SubmitChecklist when testing your
code
>***

RE: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Cohen, Guy]

From: Joerg Mayer [mailto:jmayer@loplof.de]
>On Mon, May 21, 2007 at 11:12:17PM +0300, Cohen, Guy wrote:
>> However, the vast majority of iwlwifi users at this point run it over
>> little-endian platforms.
>
>That would be 100% as it doesn't yet support big-endian platforms?
>

:)

I meant potential users...

> ciao
>     Joerg

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Jeff Garzik]

Cohen, Guy wrote:
> But at this point (we can't change the past nor the present), we would
> like to focus on more urgent issues with iwlwifi in the immediate term
> and delay a bit the big-endian support. Is this acceptable?


Your internal schedule is your internal schedule.

But as I previously stated, getting rid of the bitfields and using the 
proper (portable) Linux driver API is a hard merge requirement.

	Jeff

Re: [PATCH] Add iwlwifi wireless drivers

[James Ketrenos]

John W. Linville wrote:
> On Fri, May 18, 2007 at 02:06:19PM -0700, Randy Dunlap wrote:
>> On Fri, 18 May 2007 12:01:16 -0700 James Ketrenos wrote:
>>
>>> Randy Dunlap wrote:
>>>>> 	http://intellinuxwireless.org/iwlwifi/0001-Add-iwlwifi-wireless-drivers.patch
>>>> Some comments/review, mostly not-code-related:
>>>>
>>> Here is the current status (everything not described below has been fixed in GIT, and is available in the patch at http://intellinuxwireless.org/iwlwifi/0001-v2-Add-iwlwifi-wireless-drivers.patch:
>>>
>> [specific updates clipped]
>>
>>> Are you OK with iwlwifi addressing the above post-merge into wireless-dev?
>> /me points to Mr. Linville.  :)
> 
> I'm happy to take it into wireless-dev at this point.
> 
> James, will you be posting an updated patch?

Yes.  I'm testing the code w/ the bitfield usage removed and finishing up a few of the items from Michael's list.  By end of day today I hope to have an updated patch for you.

Thanks,
James

Re: [PATCH] Add iwlwifi wireless drivers

[John W. Linville]

On Fri, May 18, 2007 at 02:06:19PM -0700, Randy Dunlap wrote:
> On Fri, 18 May 2007 12:01:16 -0700 James Ketrenos wrote:
> 
> > Randy Dunlap wrote:
> > >> 	http://intellinuxwireless.org/iwlwifi/0001-Add-iwlwifi-wireless-drivers.patch
> > > Some comments/review, mostly not-code-related:
> > > 
> > 
> > Here is the current status (everything not described below has been fixed in GIT, and is available in the patch at http://intellinuxwireless.org/iwlwifi/0001-v2-Add-iwlwifi-wireless-drivers.patch:
> > 
> 
> [specific updates clipped]
> 
> > Are you OK with iwlwifi addressing the above post-merge into wireless-dev?
> 
> /me points to Mr. Linville.  :)

I'm happy to take it into wireless-dev at this point.

James, will you be posting an updated patch?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] Add iwlwifi wireless drivers

[James Ketrenos]

Michael Wu wrote:
> On Wednesday 16 May 2007 17:45, James Ketrenos wrote:
>> This patch adds the iwlwifi project directory and sources needed to
>> build the mac80211 based wireless drivers for the Intel PRO/Wireless
>> 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.
>>
> Looks much better than when I last checked. A few comments:

I've attempted to address everything not discussed below in the updated code.  That said --

> - Read and set the MAC address before calling ieee80211_register_hw.

We're calling SET_IEEE80211_PERM_ADDR before we call ieee80211_register_hw.  Is there something else we should be doing?

> - mac80211 (very recently) sets up a workqueue for the driver to use so you 
> don't need to create your own anymore.

In addition to the workqueue created when you call ieee80211_register_hw?  We need to have a workqueue for initializing the adapter and uCode before we call ieee80211_register_hw.  If ieee80211_alloc_hw created the workqueue, that would work for us (assuming there aren't going to be any side effects from us flushing the workqueue before we call ieee80211_free_hw/ieee80211_unregister_hw)

That said -- if the driver can execute in parallel to the stack for some operations, shouldn't they remain on their own workqueues so the work can be divided up vs. having *everything* routed through one singlethread workqueue?

> - Why are probe requests being dropped in adhoc mode? (assuming hardware 
> handles it.. but the message output for that doesn't sound like it)

It only drops them if the adapter is not in the associated state (the card is configured with the RXON_FLAG_ASSOC_MSK bit clear)

> - If you don't want to add a radiotap header all the time, you should check 
> the IEEE80211_CONF_RADIOTAP flag to see if a radiotap header is desired by 
> the stack. (not whether or not we're configured as IEEE80211_IF_TYPE_MNTR)

I have this on the todo list.  

Thanks for the time you spent looking over the code.

James

Re: [PATCH] Add iwlwifi wireless drivers

[James Ketrenos]

Jeff Garzik wrote:

> As an aside #2,
> 
>> +#if IWL == 4965
>> +           (rxon1->ofdm_ht_single_stream_basic_rates ==
>> +            rxon2->ofdm_ht_single_stream_basic_rates) &&
>> +           (rxon1->ofdm_ht_dual_stream_basic_rates ==
>> +            rxon2->ofdm_ht_dual_stream_basic_rates) &&
>> +           (rxon1->rx_chain == rxon2->rx_chain) &&
>> +#endif
> 
> 
> those #if's should go away.

Agreed.  We'll continue working to remove those over time.

Originally the intent was to have a single driver that supported the 4965 and 3945 via run-time switching.  As development moved forward, switching to a build-time approach made more sense.  base.c was then isolated from the hardware specific code and the specific HW functionality was moved into iwl-3945 and iwl-4965.  The remaining chunks are small code pieces where that work hasn't yet completed.

James

> 
>     Jeff
> 
> 
> -
> 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] Add iwlwifi wireless drivers

[James Ketrenos]

Jeff Garzik wrote:
> James Ketrenos wrote:
>> That said -- if the driver can execute in parallel to the stack for 
>> some operations, shouldn't they remain on their own workqueues so the 
>> work can be divided up vs. having *everything* routed through one 
>> singlethread workqueue?
> 
> Just because it -can-, does not mean it should.
> 
> Unless there is a -proven- need for the operations to be parallel, you 
> should avoid the burden of such complexity.

There is no additional complexity by having the driver create its own workqueue; it just calls create_workqueue during probe and destroy_workqueue during remove.

James

Re: [PATCH v3] Add iwlwifi wireless drivers

[James Ketrenos]

An updated full patch (v3) to add the driver is available at:
	
	http://intellinuxwireless.org/iwlwifi/0001-v3-Add-iwlwifi-wireless-drivers.patch

And if interested, here is the diff between the first version and this version:

	http://intellinuxwireless.org/iwlwifi/iwlwifi-v1_to_v3.patch

The short log since the first submission:

Hong Liu (8):
      remove param_channel module parameter
      rename queueId to queue in iwl_hw_txq_ctx_stop
      remove unused callback reset set_multicast_list
      remove CONFIG_IWLWIFI_MONITOR and CONFIG_IWLWIFI_RADIOTAP in iwl4965_handle_data_packet
      set RX_FLAG_RADIOTAP in iwl_handle_data_packet_monitor
      ieee80211chan2mhz is defined in ieee80211_radiotap.h
      reformat iwl4965_rx_reply_rx
      remove unnecessary in_interrupt() checks according to the ieee80211_ops comments

James Ketrenos (38):
      Fixed Kconfig help indentation to be {tab}{sp}{sp}
      Fixed indentation of non-case labels
      Updated some function kernel-doc info; removed usage of /** for non kdoc
      Fixed up some usage of magic numbers to #defines, etc.
      Removed is_broadast_ether_addr redundancy with is_multicast_ether_addr
      Updated block comments to conform to kernel style
      Removed // style comments
      Fixed indentation in is_duplicate_packet
      Comment typo adwances -> advances
      Whitespace, some 80-column, kernel doc, and other non-functional changes
      Trailing whitespace cleanup
      Fix > 80 column problems in .c files (still have to do it in .h files)
      Removed usage of IWL_NOP and IWL_NOP_RET
      Converted existing comment blocks into kernel-doc format
      Add missing linebreak in pci_driver declrataion for suspend/resume
      Removed inline on non-trivial (>~3 lines) functions
      Modified tune sysfs attribute to show/store a single value.
      Removed unused static function iwl4965_toggle_antennta from iwl-4965.c
      Change show_power_level to not prefix text before the power_level attr.
      Fixed trailing whitespace (again)
      Set check to BUILD_BUG_ON(sizeof(priv->eeprom) != IWL_EEPROM_IMAGE_SIZE)
      u16, u32, u64 -> __le16, __le32, __le64 in iwl-4965-hw.h (hardware/uCode API
      Moved struct iwl_txpower_comp_entry out of iwl-4965-hw.h
      Fixed iwl4965_get_dma_hi_address to work on all architectures
      Whitespace fixups.
      Fixed printk() w/out KERN_ level
      Removed several unnecessary variable initializers.
      Whitespace and added line to todo
      Added comments to BUG_ON code, and removed two BUG_ONs
      Changed iwl_send_cmd BUG_ONs to printk errors and return -EINVAL.
      Fixed build for CONFIG_IWL4965=m
      Removed all direct bitfield usage
      Removed several deprecated CFG_ and CAP_ declarations and use
      Removed CONFIG_IWLWIFI_QOS blocks until they are buildable.
      Replace flush_scheduled_work in d_stop with flush_workqueue
      Removed Tx cmd message spam on iwl4965
      Fixed a couple > 80 column width lines
      Set CONFIG_IWL4965 to default to =m.  Disabled =y configurations.

Yi Zhu (1):
      Fix volatile usage

Re: [PATCH] Add iwlwifi wireless drivers

[James Ketrenos]

Jeff Garzik wrote:
> James Ketrenos wrote:
>> Jeff Garzik wrote:
>>> James Ketrenos wrote:
>>>> That said -- if the driver can execute in parallel to the stack for 
>>>> some operations, shouldn't they remain on their own workqueues so 
>>>> the work can be divided up vs. having *everything* routed through 
>>>> one singlethread workqueue?
>>>
>>> Just because it -can-, does not mean it should.
>>>
>>> Unless there is a -proven- need for the operations to be parallel, 
>>> you should avoid the burden of such complexity.
>>
>> There is no additional complexity by having the driver create its own 
>> workqueue; it just calls create_workqueue during probe and 
>> destroy_workqueue during remove.
> 
> That is obviously false.  Ignoring the additional code and memory usage, 
> you must additionally evaluate the potential for deadlocks and races.

So you're saying that by using mac80211's workqueue a driver no longer has to worry about any deadlocks or race conditions?  That is obviously false.

> And then there is the Linus mantra:  do what you must, and no more.  No 
> need for additional workqueues has been demonstrated.  (which I noticed 
> your response completely skipped -- an implicit admission of lack of need)

Funny.  I noticed you completely deleted the part of my original email that listed one reason why we can't just use mac80211's workqueue currently:

"We need to have a workqueue for initializing the adapter and uCode before we call ieee80211_register_hw.  If ieee80211_alloc_hw created the workqueue, that would work for us (assuming there aren't going to be any side effects from us flushing the workqueue before we call ieee80211_free_hw/ieee80211_unregister_hw) "

I didn't say we can't use mac80211's workqueue if it was created/destroyed during alloc/free vs. hw register.  

I asked a question about if work can execute in parallel, shouldn't we let it -- and you came back saying I have to prove a need to execute in parallel.  I get your point -- Linux isn't about innovation, discussion, or putting code out and having it evolve and stand the test of time.  Its about doing exhaustive experimentation in house, putting together a thesis, and then submitting it to the mailing list.  I'll do my best to follow that model moving forward.

Thanks,
James

> 
>     Jeff
> 

Re: [PATCH] Add iwlwifi wireless drivers

[Jeff Garzik]

James Ketrenos wrote:
> That said -- if the driver can execute in parallel to the stack for some 
> operations, shouldn't they remain on their own workqueues so the work 
> can be divided up vs. having *everything* routed through one 
> singlethread workqueue?

Just because it -can-, does not mean it should.

Unless there is a -proven- need for the operations to be parallel, you 
should avoid the burden of such complexity.

	Jeff

Re: [PATCH] Add iwlwifi wireless drivers

[Jeff Garzik]

James Ketrenos wrote:
> Jeff Garzik wrote:
>> James Ketrenos wrote:
>>> That said -- if the driver can execute in parallel to the stack for 
>>> some operations, shouldn't they remain on their own workqueues so the 
>>> work can be divided up vs. having *everything* routed through one 
>>> singlethread workqueue?
>>
>> Just because it -can-, does not mean it should.
>>
>> Unless there is a -proven- need for the operations to be parallel, you 
>> should avoid the burden of such complexity.
> 
> There is no additional complexity by having the driver create its own 
> workqueue; it just calls create_workqueue during probe and 
> destroy_workqueue during remove.

That is obviously false.  Ignoring the additional code and memory usage, 
you must additionally evaluate the potential for deadlocks and races.

And then there is the Linus mantra:  do what you must, and no more.  No 
need for additional workqueues has been demonstrated.  (which I noticed 
your response completely skipped -- an implicit admission of lack of need)

	Jeff

Re: [PATCH] Add iwlwifi wireless drivers

[Jeff Garzik]

James Ketrenos wrote:
> Jeff Garzik wrote:
>> James Ketrenos wrote:
>>> Jeff Garzik wrote:
>>>> James Ketrenos wrote:
>>>>> That said -- if the driver can execute in parallel to the stack for 
>>>>> some operations, shouldn't they remain on their own workqueues so 
>>>>> the work can be divided up vs. having *everything* routed through 
>>>>> one singlethread workqueue?
>>>>
>>>> Just because it -can-, does not mean it should.
>>>>
>>>> Unless there is a -proven- need for the operations to be parallel, 
>>>> you should avoid the burden of such complexity.
>>>
>>> There is no additional complexity by having the driver create its own 
>>> workqueue; it just calls create_workqueue during probe and 
>>> destroy_workqueue during remove.
>>
>> That is obviously false.  Ignoring the additional code and memory 
>> usage, you must additionally evaluate the potential for deadlocks and 
>> races.
> 
> So you're saying that by using mac80211's workqueue a driver no longer 
> has to worry about any deadlocks or race conditions?  That is obviously 
> false.

Don't be dense.  If two operations are guaranteed to go down the same 
pipeline, then there is a clear sequence guarantee that is not present 
if operations are [needlessly] parallel.



> I didn't say we can't use mac80211's workqueue if it was 
> created/destroyed during alloc/free vs. hw register. 
> I asked a question about if work can execute in parallel, shouldn't we 
> let it

Without justification for the additional complexity, the answer is no.

	Jeff

Re: [PATCH] Add iwlwifi wireless drivers

[Michael Wu]

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

On Tuesday 22 May 2007 12:16, James Ketrenos wrote:
> > - Read and set the MAC address before calling ieee80211_register_hw.
>
> We're calling SET_IEEE80211_PERM_ADDR before we call ieee80211_register_hw.
>  Is there something else we should be doing?
>
Well, after jumping through a few hoops to find ieee80211_register_hw, it 
appears you're right.

However, I don't like it. Bringing the hardware up should be deferred as much 
as possible to when the user actually brings the interface up. ipw2200 can 
defer firmware loading to interface open time while still reading the mac 
address from the eeprom.. why not ipw3945? (the ipw2200 driver doesn't 
actually do this, but it should. I know it can.)

Also, the code doesn't appear to support changing the mac address at all. The 
mac address given by conf->mac_addr should be the address that your driver 
uses.

> That said -- if the driver can execute in parallel to the stack for some
> operations, shouldn't they remain on their own workqueues so the work can
> be divided up vs. having *everything* routed through one singlethread
> workqueue?
>
Your choice. mac80211 needs a workqueue regardless so it's made available to 
drivers. Many callbacks are called on this workqueue so drivers don't need to 
defer work to their own workqueue (and can actually return error values).

> > - Why are probe requests being dropped in adhoc mode? (assuming hardware
> > handles it.. but the message output for that doesn't sound like it)
>
> It only drops them if the adapter is not in the associated state (the card
> is configured with the RXON_FLAG_ASSOC_MSK bit clear)
>
I think this decision making belongs in mac80211.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH v3] Add iwlwifi wireless drivers

[Jeff Garzik]

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

James Ketrenos wrote:
> An updated full patch (v3) to add the driver is available at:
>     
>     http://intellinuxwireless.org/iwlwifi/0001-v3-Add-iwlwifi-wireless-drivers.patch 

Quick review attached.

	Jeff




[-- Attachment #2: notes.txt --]
[-- Type: text/plain, Size: 8042 bytes --]


1) iwl_queue_inc_wrap and iwl_queue_dec_wrap can usually avoid a branch,
if you use power-of-two queue size.

2) Using '%' to calculate index is more expensive than a mask

3) delete all casts to/from void pointers, such as

	txq->bd = (u8 *)
		pci_alloc_consistent(dev,

4) the difference in the TX queue size calculations is very error-prone:

	len = (sizeof(struct iwl_cmd) * count) + IWL_MAX_SCAN_SIZE;
		and
	len = (sizeof(txq->cmd[0]) * q->n_window) + IWL_MAX_SCAN_SIZE;

5) more Lindent damage to undo, in iwl_remove_station():

+               for (i = IWL_STA_ID; i < (priv->num_stations + IWL_STA_ID);
+                    i++) {
+                       if ((priv->stations[i].used)
+                           &&
+                           (!compare_ether_addr(
+                                   priv->stations[i].sta.sta.addr, bssid))) {
+                               index = i;
+                               priv->stations[index].used = 0;
+                               break;
+                       }
+               }

6) iwl_delete_stations_table() and iwl_clear_stations_table()
   are exactly the same implementation.  remove one.

7) ditto #5 in iwl_add_station()

8) obtain a ptr rather than repeatedly referencing priv->stations[i],
   in iwl_add_station()

9) de-indent the primary code block in iwl_sync_station(),
   by returning immediately if the sta_id is invalid

10) remove a lot of the 'inline' markers.  let the compiler decide that.

11) eliminate this compile-the-code-twice scheme

12) delete the myriad checks in iwl_send_cmd() that are never
    hit in real life.  Mark others WARN_ON() or whatever.

    That is just way too many needless checks in a hot path.

13) delete the conditional locking in iwl_send_cmd().  spinlocking
    should not be conditional.  it's error prone and usually wrong.  the
    spinlocking in this routine is an excellent example of that.
    [reads further]
    Yuck, it is -even worse- than I thought.

14) you don't need a prototype right before the function def:
+static int iwl_send_cmd_u32(struct iwl_priv *priv, u8 id, u32 val)
+    __attribute__ ((warn_unused_result));
+static int iwl_send_cmd_u32(struct iwl_priv *priv, u8 id, u32 val)
+{

15) more Lindent damage in iwl_check_rxon_cmd():
+       error |=
+           ((rxon->
+             flags & (RXON_FLG_AUTO_DETECT_MSK |

16) This whole CMD_ASYNC paradigm is problematic.  Use the standard
Linux style:  Make EVERYTHING async, and then have separate helpers that
allow the code to wait for completion.

17) more Lindent damage:
+       if (priv->frames_count) {
+               IWL_WARNING
+                   ("%d frames still in use.  Did we lose one?\n",
+                    priv->frames_count);

18) remove nonsense tests:
+       if (list_empty(&priv->free_frames)) {
...
+               if (priv->frames_count >= FREE_FRAME_THRESHOLD) {
+                       IWL_WARNING("%d frames allocated.  "
+                                   "Are we losing them?\n",
+                                   priv->frames_count);

19) iwl_free_frame() does not pay attention to FREE_FRAME_THRESHOLD

20) Lindent damage:
+               IWL_ERROR
+                   ("Coult not obtain free frame buffer for beacon "
+                    "command.\n");
+               return -ENOMEM;
+       }
+
+       if (!(priv->staging_rxon.flags & RXON_FLG_BAND_24G_MSK)) {
+               rate =
+                   iwl_rate_get_lowest_plcp(priv->active_rate_basic & 0xFF0);

21) poorly named function:
+static void eeprom_parse_mac(struct iwl_priv *priv, u8 * mac)
+{
+       memcpy(mac, priv->eeprom.mac_address, 6);
+}

22) kill magic numbers in iwl_eeprom_init()

23) bogus comment and messy declarations in iwl_report_frame():
+       /* these are declared without "=" to avoid compiler warnings if
we
+        *   don't use them in the debug messages below */
+       u16 frame_ctl;
+       u16 seq_ctl;
+       u16 channel;

    CLEARLY you will get 'unused variable' warnings, if you don't use them

24) as akpm said, don't break up variables, it makes decls look like
code blocks:
+       u8 agc;
+       u16 sig_avg;
+       u16 noise_diff;
+
+       struct iwl_rx_frame_stats *rx_stats = IWL_RX_STATS(pkt);
+       struct iwl_rx_frame_hdr *rx_hdr = IWL_RX_HDR(pkt);
+       struct iwl_rx_frame_end *rx_end = IWL_RX_END(pkt);
+       u8 *data = IWL_RX_DATA(pkt);


25) this loop needs some sort of fencing:
+               mutex_unlock(&priv->mutex);
+               if (ms)
+                       while (!time_after(jiffies,
+                                          now + msecs_to_jiffies(ms)) &&
+                              priv->status & STATUS_SCANNING)
+                               msleep(1);
+
+               mutex_lock(&priv->mutex);

26) overall, priv->status accesses look racy in many instances

27) ditch private workqueue

28) weird indentation:
+       IWL_DEBUG_RATE("station Id %d\n", sta_id);
+
+       qc = ieee80211_get_qos_ctrl(hdr);
+        if (qc) {
+               u8 tid = (u8)(*qc & 0xf);

29) are some uses of control_flags in iwl_tx_skb() endian-unsafe?

30) replace "todoG" with something standard to Linux like FIXME or TODO

31) iwl_rx_reply_scan() does nothing

32) racy priv->status manipulation in iwl_rx_scan_complete_notif()

33) kill braces around single C statements:
+       if (rxq->free_count <= RX_LOW_WATERMARK) {
+               queue_work(priv->workqueue, &priv->rx_replenish);
+       }

34) dont do this in your interrupt handler:
	* mask events
	* handle events
	* unmask events
    delete the mask/unmask stuff

35) your irq tasklet is unneeded.  just do it in the irq handler,
    and eliminate the races.

36) way too many allocations are GFP_ATOMIC.  that tells me you are
    often doing initialization in the wrong context

37) looks like iwl_read_ucode() needs endian accessors?

38) after disabling interrupts you need to do a read, to ensure your
request is flushed to the hardware

39) rename d_xxx functions.  those are ambiguous in a trace

40) no need to zero stuff in iwl_pci_probe(), priv should already be
zeroed for you

41) use pci_iomap() rather than ioremap_nocache()

42) you queue stuff to a workqueue, when it is obvious from context
(e.g. iwl_pci_probe) it can sleep

43) naming your module parameters param_xxx, and making them global
symbols, pollutes the global namespace and potentially introduces
conflicts

44) don't use reg_xxx as a function prefix, that is ambiguous in a trace

45) fix long function names:
	reg_txpower_compensate_for_temperature_dif()

46) ditto:
	iwl_write_restricted(), iwl_release_restricted_access()

47) kill magic numbers in pci_{read,write}_config_xxx calls

48) shorten too-long constant names:
	CSR_GP_CNTRL_REG_MSK_POWER_SAVE_TYPE
	CSR_RESET_REG_FLAG_MASTER_DISABLED,

49) PCI posting bug?
+
+       iwl_set_bit(priv, CSR_RESET, CSR_RESET_REG_FLAG_SW_RESET);
+
+       udelay(10);
+
+       iwl_set_bit(priv, CSR_GP_CNTRL, CSR_GP_CNTRL_REG_FLAG_INIT_DONE);

50) this is just silly:
+
+       spin_unlock_irqrestore(&priv->lock, flags);
+
+       spin_lock_irqsave(&priv->lock, flags);
+

51) fix long struct names:
	struct iwl_eeprom_calib_measurement

52) and struct member names:
	ch_i1 = priv->eeprom.calib_info.band_info_tbl[s].ch1.ch_num;

53) etc.
	iwl4965_get_txatten_group_from_channel()

54) magic number
+       if (-40 > current_temp) {
+               IWL_WARNING("Invalid temperature %d, can't calculate "

55) no need for all this casting and masking:
+static inline u32 iwl4965_get_dma_lo_address(dma_addr_t addr)
+{
+       return (u32) (addr & 0xffffffff);
+}

56) endian-ness in iwl4965_load_bsm()

57) no need to lock around a single MMIO write:
+       spin_lock_irqsave(&priv->lock, flags);
+
+       iwl_write32(priv, CSR_RESET, 0);
+
+       spin_unlock_irqrestore(&priv->lock, flags);

58) separate declarations and code:
+       };
+       u8 network_packet;
+       if ((unlikely(rx_start->cfg_mib_cnt > 20))) {

59) too long:
	iwl_write_restricted_reg_buffer()

Re: [PATCH] Add iwlwifi wireless drivers

[Jeff Garzik]

Michael Wu wrote:
> However, I don't like it. Bringing the hardware up should be deferred as much 
> as possible to when the user actually brings the interface up. ipw2200 can 
> defer firmware loading to interface open time while still reading the mac 
> address from the eeprom.. why not ipw3945? (the ipw2200 driver doesn't 
> actually do this, but it should. I know it can.)

VERY good point.  This is the standard for ethernet drivers:

probe:
	basic hardware init, read MAC address from EEPROM
interface up:
	full hardware power-up, reset and init
	write MAC address to hardware
	request irq
	net stack up
interface down:
	net stack down
	hardware, power & irq down

And I would hope that wireless drivers would follow a similar pattern.

	Jeff

Re: [PATCH v3] Add iwlwifi wireless drivers

[James Ketrenos]

Jeff Garzik wrote:
> James Ketrenos wrote:
>> An updated full patch (v3) to add the driver is available at:
>>         
>> http://intellinuxwireless.org/iwlwifi/0001-v3-Add-iwlwifi-wireless-drivers.patch 
> 
> 
> Quick review attached.
> 
>     Jeff

Thanks for the review.  

Hopefully once in wireless-dev, we (more than just me, myself, and I) can work to get these knocked off in iterative small commits vs. having to paste a monolithic driver add patch that makes git-bisect near useless.  Some of your review comments impact code which can have significant ripple effects -- I would like to be able to manage those changes here vs. solely with the iwlwifi community of users.

James

Re: [PATCH] Add iwlwifi wireless drivers

[James Ketrenos]

Jeff Garzik wrote:
> James Ketrenos wrote:
>> Jeff Garzik wrote:
>>> James Ketrenos wrote:
>>>> Jeff Garzik wrote:
>>>>> James Ketrenos wrote:
>>>>>> That said -- if the driver can execute in parallel to the stack 
>>>>>> for some operations, shouldn't they remain on their own workqueues 
>>>>>> so the work can be divided up vs. having *everything* routed 
>>>>>> through one singlethread workqueue?
>>>>>
>>>>> Just because it -can-, does not mean it should.
>>>>>
>>>>> Unless there is a -proven- need for the operations to be parallel, 
>>>>> you should avoid the burden of such complexity.
>>>>
>>>> There is no additional complexity by having the driver create its 
>>>> own workqueue; it just calls create_workqueue during probe and 
>>>> destroy_workqueue during remove.
>>>
>>> That is obviously false.  Ignoring the additional code and memory 
>>> usage, you must additionally evaluate the potential for deadlocks and 
>>> races.
>>
>> So you're saying that by using mac80211's workqueue a driver no longer 
>> has to worry about any deadlocks or race conditions?  That is 
>> obviously false.
> 
> Don't be dense.  If two operations are guaranteed to go down the same 
> pipeline, then there is a clear sequence guarantee that is not present 
> if operations are [needlessly] parallel.

Being on ifsta->work doesn't guarantee any sequence--it just means ieee80211_sta_work can't process management frames at the same time the driver is doing things on that workqueue.  ieee80211_sta_work does make some driver callbacks, however those callbacks are also be made from ioctls and cfg80211.  Additionally there is other work that ieee80211_sta_work does that does not make any callbacks or state transitions into the driver.

There is no guarantee that driver callbacks from mac80211 will not occur in parallel or in different contexts. 

The driver has to protect access to memory shared across those contexts regardless of which workqueue it happens to use. 

>> I didn't say we can't use mac80211's workqueue if it was 
>> created/destroyed during alloc/free vs. hw register. I asked a 
>> question about if work can execute in parallel, shouldn't we let it
> 
> Without justification for the additional complexity, the answer is no.

Can you show me how the complexity is reduced?

    "Ignoring the additional code and memory usage, you must additionally evaluate the potential for deadlocks and races."

Given the API mac80211 exposes, and how the stack operates, I don't see how using ifsta->work as the workqueue allows the driver to no longer evaluate the potential for deadlocks and races.

The second argument you made:

    "And then there is the Linus mantra:  do what you must, and no more.  No need for additional workqueues has been demonstrated."

To make the most use of the processing and power resources available, you must execute in parallel.

I haven't seen a need, or benefit, to serialize unrelated work onto a single workqueue.

James

Re: [PATCH] Add iwlwifi wireless drivers

[James Ketrenos]

Jeff Garzik wrote:
> James Ketrenos wrote:
>> Given the API mac80211 exposes, and how the stack operates, I don't 
>> see how using ifsta->work as the workqueue allows the driver to no 
>> longer evaluate the potential for deadlocks and races.
> 
> For the operations in the workqueue, they are obviously serialized.

And the inverse is also true -- operations not on the workqueue are not serialized.  The driver must handle both.

>> To make the most use of the processing and power resources available, 
>> you must execute in parallel.
> 
> That's some impressive English parsing, but clearly wrong on the face of 
> it.

If you have N cores on a CPU and more than one is up, but only one gets to do anything because you've serialized everything onto a single workqueue, you are wasting CPU resources.  When you waste CPU resources you decrease system efficiency and increase power consumption.  

If you have N cores and only one is up, then the scheduler can decide to run serially if it wants, or wake up a core, or do whatever it wants.  The driver certainly shouldn't dictate policy and force everything to run serially.

> The Linus mantra cited is about MINIMALIZATION.  When you say "to 
> make the most use of..." you are MAXIMIZING something that need not be 
> maximized.

I'm really not following what your point is here.  In an attempt to twist my words, you're now saying we shouldn't maximize system efficiency.

> As repeated many times, there is no NEED to execute this slow path stuff 
> in parallel.  

I must have missed the thread where you said that many times, let alone where you proved that there is no benefit or need to  running "this slow path stuff" (however you are defining that to be) in parallel.

A driver should not be written in a way which suppresses the kernel's ability to optimize execution by forcing serialization of disjointed operations through a single workqueue.

James

Re: [PATCH] Add iwlwifi wireless drivers

[Jeff Garzik]

James Ketrenos wrote:
> Given the API mac80211 exposes, and how the stack operates, I don't see 
> how using ifsta->work as the workqueue allows the driver to no longer 
> evaluate the potential for deadlocks and races.

For the operations in the workqueue, they are obviously serialized.


> The second argument you made:
> 
>    "And then there is the Linus mantra:  do what you must, and no more.  
> No need for additional workqueues has been demonstrated."
> 
> To make the most use of the processing and power resources available, 
> you must execute in parallel.

That's some impressive English parsing, but clearly wrong on the face of 
it.  The Linus mantra cited is about MINIMALIZATION.  When you say "to 
make the most use of..." you are MAXIMIZING something that need not be 
maximized.

As repeated many times, there is no NEED to execute this slow path stuff 
in parallel.  Though yes, I agree parallel execution maximized power drain.

	Jeff

[PATCH V3] Add iwlwifi wireless drivers

[Zhu Yi]

Hi,

This is the third version of the patch (against 2.6.23-rc4) 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/v6-Add-iwlwifi-wireless-drivers.patch

I list the changes against the last version I submitted to the list last
week 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-v5_to_v6.patch

In this version, we replace the "comiple iwl-base.c twice for two
drivers" scheme with separating the two drivers into two code base. In
the future, we will abstract a common layer for all the iwl based
drivers and make it a separate module for better maintenance.

Note, the #include "../net/mac80211/ieee80211_rate.h" still existed in
this version of the driver. We are discussing how to abstract the
mac80211 rate scaling interface in another thread in this ML.


Signed-off-by: Zhu Yi <yi.zhu@intel.com>

Adrian Bunk (1):
      iwl-base.c bugfixes

Ben Cahill (3):
      iwlwifi: unify definitions (3945/4965) of struct
iwl_txpowertable_cmd
      iwlwifi: remove unused definitions in iwl-4965-hw.h
      iwlwifi: move 4965 Rx API stuff into iwl-commands.h

Mohamed Abbas (2):
      iwlwifi: fix assert when calling LinkQuality command
      iwlwifi: fix aggregation tx status

Tomas Winkler (14):
      iwlwifi: kill legacy fields from iwl_cmd_meta
      iwlwifi: kill __iwl_send_cmd
      iwlwifi: add file iwl-prph.h
      iwlwifi: CSR registers cleanup
      iwlwifi: add Makefile sparse target
      iwlwifi: iwl_mac_conf_tx remove casting to restricted
      iwlwifi: setting RAxTID endianity fix
      iwlwifi: AP setup qos and tx_conf bug fix
      iwlwifi: move defines from iwl-4965.c to iwl-commands.h
      iwlwifi: remove unnecessary condition check
      iwlwifi: fixing LQ command changes in AP mode
      iwlwifi: FAT channel bug fix for zero relevant bits
      iwlwifi: Control channel fix for FAT channel
      iwlwifi: move APMG registers to Periphery section

Zhu Yi (4):
      iwlwifi: make "ipw going down" debug message depends on
IWL_DL_INFO
      iwlwifi: fix IBSS connection problem caused by LinkQuality cmd
change
      iwlwifi: Update version iwl-base.c stamp to 0.1.14
      iwlwifi: move macros defines from *.c to *.h
      
ian (1):
      avoid kernel oops in monitor mode with debug enabled

Re: [PATCH V3] Add iwlwifi wireless drivers

[Johannes Berg]

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

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

> Note, the #include "../net/mac80211/ieee80211_rate.h" still existed in
> this version of the driver. We are discussing how to abstract the
> mac80211 rate scaling interface in another thread in this ML.

You also haven't addressed any of the other points I raised wrt.
encryption [please try running your driver on the latest
mac80211/net-2.6.24 tree, it'll complain very loudly], looking into
frames, using the configured MAC address instead of the programmed one,
oopsing if pure monitor mode is requested, doing frame aggregation and
all the others.

I think this repost is mostly useless for review, I applaud that you're
working on things but don't see why we'd be interested in this step.

johannes

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

Re: [PATCH V3] Add iwlwifi wireless drivers

[Christoph Hellwig]

On Tue, Sep 04, 2007 at 11:04:17AM +0800, Zhu Yi wrote:
> In this version, we replace the "comiple iwl-base.c twice for two
> drivers" scheme with separating the two drivers into two code base. In
> the future, we will abstract a common layer for all the iwl based
> drivers and make it a separate module for better maintenance.

Nice.  But please also get rid of the Makefile hacks added for this now
that they're not required anymore.

Re: [PATCH V3] Add iwlwifi wireless drivers

[Johannes Berg]

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

Some more things.

+	case IEEE80211_FTYPE_DATA:
+		if (unlikely(is_duplicate_packet(priv, header)))
+			IWL_DEBUG_DROP("Dropping (dup): " MAC_FMT ", "
+				       MAC_FMT ", " MAC_FMT "\n",
+				       MAC_ARG(header->addr1),
+				       MAC_ARG(header->addr2),
+				       MAC_ARG(header->addr3));

mac80211 does duplicate detection.

+	if (priv->iw_mode == IEEE80211_IF_TYPE_MNTR) {
+		if (iwl_param_hwcrypto)
+			iwl_set_decrypted_flag(priv, rxb->skb,
+					       ampdu_status, stats);
+		iwl_handle_data_packet_monitor(priv, rxb, hdr, len, stats, 0);
+		return;
+	}

why handle monitor mode differently? mac80211 does all that for you.


How much does the hardware do when you're in AP mode and have stations
in PS mode? You seem to do some things but it's not clear why.


+	/* FIXME: not sure why this doesn't work in AP mode */
+	if (priv->iw_mode != IEEE80211_IF_TYPE_AP)
+		iwl_sequence_reset(priv);

Now, I'd understand that in a reverse engineered driver....

+static int iwl_mac_stop(struct ieee80211_hw *hw)
+{
+	struct iwl_priv *priv = hw->priv;
+
+	IWL_DEBUG_MAC80211("enter\n");
+	priv->is_open = 0;
+	/*netif_stop_queue(dev); */

eh? dead code in comments isn't nice especially if it's wrong.

+	if (priv->iw_mode == IEEE80211_IF_TYPE_MNTR) {
+		IWL_DEBUG_MAC80211("leave - monitor\n");
+		return -1;
+	}

Andy will hate you for that. And I see no point.

+	if (priv->interface_id) {
+		IWL_DEBUG_MAC80211("leave - interface_id != 0\n");
+		return 0;
+	}

I don't think you should return 0 for an error case, when userspace
tries to add multiple you just accept them?

+ * We ignore conf->flags & IEEE80211_CONF_SHORT_SLOT_TIME since it seems to
+ * be set inappropriately and the driver currently sets the hardware up to
+ * use it whenever needed.

would be nice to figure out if there is a bug and then fix it. I haven't
noticed one.

+	/* TODO: Figure out how to get ieee80211_local->sta_scanning w/ only
+	 * what is exposed through include/ declrations */
+	if (unlikely(!iwl_param_disable_hw_scan &&
+		     test_bit(STATUS_SCANNING, &priv->status))) {
+		IWL_DEBUG_MAC80211("leave - scanning\n");
+		mutex_unlock(&priv->mutex);
+		return 0;
+	}

Uh huh? Is that more workarounds for the hardware scanning?

+		iwl_set_rxon_hwcrypto(priv, 1);
+		iwl_commit_rxon(priv);
+		key->flags &= ~IEEE80211_KEY_FORCE_SW_ENCRYPT;
+		key->hw_key_idx = sta_id;
+		IWL_DEBUG_MAC80211("set_key success, using hwcrypto\n");

Since you're not going to be able to push the driver into .23 and .24
will include mac80211 updates, how about making it compile with those
updates? Hint: take patches from wireless-dev.

+ * The following adds a new attribute to the sysfs representation
+ * of this device driver (i.e. a new file in /sys/bus/pci/drivers/ipw/)
+ * used for controlling the debug level.
+ *
+ * See the level definitions in ipw for details.

That interface should likely live in debugfs.

+static ssize_t show_rf_kill(struct device *d,
+			    struct device_attribute *attr, char *buf)
+{
+	/*
+	 * 0 - RF kill not enabled
+	 * 1 - SW based RF kill active (sysfs)
+	 * 2 - HW based RF kill active
+	 * 3 - Both HW and SW based RF kill active

that as well, along with all the other sysfs bits. Also, how about using
the generic rfkill infrastructure Ivo did?

+	/* The following it a temporary work around due to the
+	 * suspend / resume not fully initializing the NIC correctly.

"temporary" until when?

+		if (unlikely(!network_packet))
+			IWL_DEBUG_DROP("Dropping (non network): "
+				       MAC_FMT ", " MAC_FMT ", "
+				       MAC_FMT "\n",
+				       MAC_ARG(header->addr1),
+				       MAC_ARG(header->addr2),
+				       MAC_ARG(header->addr3));

Andy will hate that as well. I wonder if his stuff actually works?

Uh. Enough for now.

johannes

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

Re: [PATCH V3] Add iwlwifi wireless drivers

[dragoran]

>+static ssize_t show_rf_kill(struct device *d,
>+			    struct device_attribute *attr, char *buf)
>+{
>+	/*
>+	 * 0 - RF kill not enabled
>+	 * 1 - SW based RF kill active (sysfs)
>+	 * 2 - HW based RF kill active
>+	 * 3 - Both HW and SW based RF kill active
>
>that as well, along with all the other sysfs bits. Also, how about using
>the generic rfkill infrastructure Ivo did?

is the generic rfkill interface already stable and merged into the linus tree?
if not please leave this for now to not break userspace (hal).
hal currently is using this on all ipw* and iwl* drivers to get and
set the rfkill status. And NM uses this interface to set/get rfkill
status.

So please don't remove this yet until the proper interface is merged
too (which should be better anyway because this one requires polling)

Re: [PATCH V3] Add iwlwifi wireless drivers

[Tomas Winkler]

>

> Uh huh? Is that more workarounds for the hardware scanning?
>
> +               iwl_set_rxon_hwcrypto(priv, 1);
> +               iwl_commit_rxon(priv);
> +               key->flags &= ~IEEE80211_KEY_FORCE_SW_ENCRYPT;
> +               key->hw_key_idx = sta_id;
> +               IWL_DEBUG_MAC80211("set_key success, using hwcrypto\n");
>
> Since you're not going to be able to push the driver into .23 and .24
> will include mac80211 updates, how about making it compile with those
> updates? Hint: take patches from wireless-dev.
>
> + * The following adds a new attribute to the sysfs representation
> + * of this device driver (i.e. a new file in /sys/bus/pci/drivers/ipw/)
> + * used for controlling the debug level.
> + *
> + * See the level definitions in ipw for details.
>

> That interface should likely live in debugfs.
>
On the way as well

>
> Uh. Enough for now.
>
> johannes
>
>

Re: [PATCH V3] Add iwlwifi wireless drivers

[Ivo van Doorn]

On Tuesday 04 September 2007, dragoran wrote:
> >+static ssize_t show_rf_kill(struct device *d,
> >+			    struct device_attribute *attr, char *buf)
> >+{
> >+	/*
> >+	 * 0 - RF kill not enabled
> >+	 * 1 - SW based RF kill active (sysfs)
> >+	 * 2 - HW based RF kill active
> >+	 * 3 - Both HW and SW based RF kill active
> >
> >that as well, along with all the other sysfs bits. Also, how about using
> >the generic rfkill infrastructure Ivo did?
> 
> is the generic rfkill interface already stable and merged into the linus tree?

Yes. It currently is only missing users.

> if not please leave this for now to not break userspace (hal).
> hal currently is using this on all ipw* and iwl* drivers to get and
> set the rfkill status. And NM uses this interface to set/get rfkill
> status.
> 
> So please don't remove this yet until the proper interface is merged
> too (which should be better anyway because this one requires polling)

The input-polldev interface could be used for polling if it is required.

Ivo

Re: [PATCH V3] Add iwlwifi wireless drivers

[dragoran]

On 9/4/07, Ivo van Doorn <ivdoorn@gmail.com> wrote:
> On Tuesday 04 September 2007, dragoran wrote:
> > >+static ssize_t show_rf_kill(struct device *d,
> > >+                        struct device_attribute *attr, char *buf)
> > >+{
> > >+    /*
> > >+     * 0 - RF kill not enabled
> > >+     * 1 - SW based RF kill active (sysfs)
> > >+     * 2 - HW based RF kill active
> > >+     * 3 - Both HW and SW based RF kill active
> > >
> > >that as well, along with all the other sysfs bits. Also, how about using
> > >the generic rfkill infrastructure Ivo did?
> >
> > is the generic rfkill interface already stable and merged into the linus tree?
>
> Yes. It currently is only missing users.

ok thats great ;) is the (userspace) interface defined somewhere? or
should I read the code to understand how it works? (would like to add
support to hal)

> > if not please leave this for now to not break userspace (hal).
> > hal currently is using this on all ipw* and iwl* drivers to get and
> > set the rfkill status. And NM uses this interface to set/get rfkill
> > status.
> >
> > So please don't remove this yet until the proper interface is merged
> > too (which should be better anyway because this one requires polling)
>
> The input-polldev interface could be used for polling if it is required.

shouldn't the new interface send events so that polling isn't required?

Re: [PATCH V3] Add iwlwifi wireless drivers

[Ivo van Doorn]

On Tuesday 04 September 2007, dragoran wrote:
> On 9/4/07, Ivo van Doorn <ivdoorn@gmail.com> wrote:
> > On Tuesday 04 September 2007, dragoran wrote:
> > > >+static ssize_t show_rf_kill(struct device *d,
> > > >+                        struct device_attribute *attr, char *buf)
> > > >+{
> > > >+    /*
> > > >+     * 0 - RF kill not enabled
> > > >+     * 1 - SW based RF kill active (sysfs)
> > > >+     * 2 - HW based RF kill active
> > > >+     * 3 - Both HW and SW based RF kill active
> > > >
> > > >that as well, along with all the other sysfs bits. Also, how about using
> > > >the generic rfkill infrastructure Ivo did?
> > >
> > > is the generic rfkill interface already stable and merged into the linus tree?
> >
> > Yes. It currently is only missing users.
> 
> ok thats great ;) is the (userspace) interface defined somewhere? or
> should I read the code to understand how it works? (would like to add
> support to hal)

There isn't a documentation file for it, so best thing to do would be looking at the code.
basically hal only needs to check the sysfs files:

	name -> Name of device/interface
	type -> wlan, bluetooth, irda
	state -> Current device state. 0: Off, 1: On
	claim -> 0: Kernel handles events, 1: Userspace handles events

"name" and "type" are read-only
"claim" and "state" are read/writable

Note that there is a bug in 2.6.22 which causes the "state" file to be read-only,
this has been fixed in 2.6.23-rc.

> > > if not please leave this for now to not break userspace (hal).
> > > hal currently is using this on all ipw* and iwl* drivers to get and
> > > set the rfkill status. And NM uses this interface to set/get rfkill
> > > status.
> > >
> > > So please don't remove this yet until the proper interface is merged
> > > too (which should be better anyway because this one requires polling)
> >
> > The input-polldev interface could be used for polling if it is required.
> 
> shouldn't the new interface send events so that polling isn't required?

Not really, rfkill was intended for a generic interface, which would make
the rfkill buttons work with or without intervention of userspace. So polling
is required unless the hardware generates interrupts when the button is called.

Ivo

Re: [PATCH V3] Add iwlwifi wireless drivers

[dragoran]

On 9/4/07, Ivo van Doorn <ivdoorn@gmail.com> wrote:
> On Tuesday 04 September 2007, dragoran wrote:
> > On 9/4/07, Ivo van Doorn <ivdoorn@gmail.com> wrote:
> > > On Tuesday 04 September 2007, dragoran wrote:
> > > > >+static ssize_t show_rf_kill(struct device *d,
> > > > >+                        struct device_attribute *attr, char *buf)
> > > > >+{
> > > > >+    /*
> > > > >+     * 0 - RF kill not enabled
> > > > >+     * 1 - SW based RF kill active (sysfs)
> > > > >+     * 2 - HW based RF kill active
> > > > >+     * 3 - Both HW and SW based RF kill active
> > > > >
> > > > >that as well, along with all the other sysfs bits. Also, how about using
> > > > >the generic rfkill infrastructure Ivo did?
> > > >
> > > > is the generic rfkill interface already stable and merged into the linus tree?
> > >
> > > Yes. It currently is only missing users.
> >
> > ok thats great ;) is the (userspace) interface defined somewhere? or
> > should I read the code to understand how it works? (would like to add
> > support to hal)
>
> There isn't a documentation file for it, so best thing to do would be looking at the code.
> basically hal only needs to check the sysfs files:
>
>         name -> Name of device/interface
>         type -> wlan, bluetooth, irda
>         state -> Current device state. 0: Off, 1: On
>         claim -> 0: Kernel handles events, 1: Userspace handles events
>
> "name" and "type" are read-only
> "claim" and "state" are read/writable
>
> Note that there is a bug in 2.6.22 which causes the "state" file to be read-only,
> this has been fixed in 2.6.23-rc.

ok that isn't complicated what is the claim used for?
does It has to be set to userspace to be able to toggle the status via software?


> > shouldn't the new interface send events so that polling isn't required?
>
> Not really, rfkill was intended for a generic interface, which would make
> the rfkill buttons work with or without intervention of userspace. So polling
> is required unless the hardware generates interrupts when the button is called.

ok

Re: [PATCH V3] Add iwlwifi wireless drivers

[Ivo van Doorn]

On Tuesday 04 September 2007, dragoran wrote:
> On 9/4/07, Ivo van Doorn <ivdoorn@gmail.com> wrote:
> > On Tuesday 04 September 2007, dragoran wrote:
> > > On 9/4/07, Ivo van Doorn <ivdoorn@gmail.com> wrote:
> > > > On Tuesday 04 September 2007, dragoran wrote:
> > > > > >+static ssize_t show_rf_kill(struct device *d,
> > > > > >+                        struct device_attribute *attr, char *buf)
> > > > > >+{
> > > > > >+    /*
> > > > > >+     * 0 - RF kill not enabled
> > > > > >+     * 1 - SW based RF kill active (sysfs)
> > > > > >+     * 2 - HW based RF kill active
> > > > > >+     * 3 - Both HW and SW based RF kill active
> > > > > >
> > > > > >that as well, along with all the other sysfs bits. Also, how about using
> > > > > >the generic rfkill infrastructure Ivo did?
> > > > >
> > > > > is the generic rfkill interface already stable and merged into the linus tree?
> > > >
> > > > Yes. It currently is only missing users.
> > >
> > > ok thats great ;) is the (userspace) interface defined somewhere? or
> > > should I read the code to understand how it works? (would like to add
> > > support to hal)
> >
> > There isn't a documentation file for it, so best thing to do would be looking at the code.
> > basically hal only needs to check the sysfs files:
> >
> >         name -> Name of device/interface
> >         type -> wlan, bluetooth, irda
> >         state -> Current device state. 0: Off, 1: On
> >         claim -> 0: Kernel handles events, 1: Userspace handles events
> >
> > "name" and "type" are read-only
> > "claim" and "state" are read/writable
> >
> > Note that there is a bug in 2.6.22 which causes the "state" file to be read-only,
> > this has been fixed in 2.6.23-rc.
> 
> ok that isn't complicated what is the claim used for?
> does It has to be set to userspace to be able to toggle the status via software?

By default the kernel will act when the rfkill button is pressed, it will loop
through all registered buttons of the same type and change the state.
Userspace can read the "state" sysfs file to see the current status,
but if a userspace tools wants to take control of taking action when the button is pressed,
or at least doesn't want the kernel to do anything, 1 should be written to "claim" to
tell the kernel that it should back off and ignore all events.

Ivo

Re: [PATCH V3] Add iwlwifi wireless drivers

[Inaky Perez-Gonzalez]

On Tuesday 04 September 2007, Ivo van Doorn wrote:
> On Tuesday 04 September 2007, dragoran wrote:
> > On 9/4/07, Ivo van Doorn <ivdoorn@gmail.com> wrote:
> > > On Tuesday 04 September 2007, dragoran wrote:
> > > > >+static ssize_t show_rf_kill(struct device *d,
> > >
> > > Yes. It currently is only missing users.
> > 
> > ok thats great ;) is the (userspace) interface defined somewhere? or
> > should I read the code to understand how it works? (would like to add
> > support to hal)
> 
> There isn't a documentation file for it, so best thing to do would be looking at the code.
> basically hal only needs to check the sysfs files:
> 
> 	name -> Name of device/interface
> 	type -> wlan, bluetooth, irda

Can you add 'uwb' [for ultrawideband radios]? That way I could start to use it. A
Doc file would help enourmously also :)

-- Inaky

Re: [PATCH V3] Add iwlwifi wireless drivers

[Zhu Yi]

On Tue, Sep 04, 2007 at 04:04:11PM +0200, Johannes Berg wrote:
> On Tue, 2007-09-04 at 11:04 +0800, Zhu Yi wrote:
> 
> > Note, the #include "../net/mac80211/ieee80211_rate.h" still existed in
> > this version of the driver. We are discussing how to abstract the
> > mac80211 rate scaling interface in another thread in this ML.
> 
> You also haven't addressed any of the other points I raised wrt.
> encryption [please try running your driver on the latest
> mac80211/net-2.6.24 tree, it'll complain very loudly], looking into
> frames, using the configured MAC address instead of the programmed one,
> oopsing if pure monitor mode is requested,

Do you mean this?

> ian (1):
>      avoid kernel oops in monitor mode with debug enabled

For your other comments, I agree they are valid and we will address
them. But I don't think any of them are a block issue for .24, right?
The driver works well for most users, I think we can push it into
mainline for better testing and fix them slowly (keep stability) by
.25 time.

Thanks,
-yi

Re: [PATCH V3] Add iwlwifi wireless drivers

[Johannes Berg]

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

On Wed, 2007-09-05 at 09:38 +0800, Zhu Yi wrote:

> For your other comments, I agree they are valid and we will address
> them. But I don't think any of them are a block issue for .24, right?

Actually, at least the crypto thing is an issue for you: it'll be a
support nightmare. Please do what I've been saying in every second mail
now: run this driver against the mac80211 in net-2.6.24. It *will*
trigger a WARN_ON() in the key handling code.

johannes

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

Re: [PATCH V3] Add iwlwifi wireless drivers

[Johannes Berg]

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

I was just looking through the iwlwifi sources to find out whether it
can encrypt with a key even if the packet isn't unicast... Anybody know?
I myself removed the ability to upload multicast/broadcast keys because
it was so unclear, but it seems that maybe the firmware knows about some
sort of "virtual broadcast/multicast" station? How about multiple
broadcast keys for APs with VLANs?

In any case, I found this comment:

                /* XXX: ACK flag must be set for CCMP even if it
                 * is a multicast/broadcast packet, because CCMP
                 * group communication encrypted by GTK is
                 * actually done by the AP. */
                cmd->cmd.tx.tx_flags |= TX_CMD_FLG_ACK_MSK;

and it doesn't make any sense at all. Care to explain? If we're a
station associated to some other AP, then mac80211 will *obviously* set
the "expect ack" bit on a "multicast data" packet that's being sent
since it's actually unicast in 802.11 terms to the AP before it resends
it using the group key... And if we're an AP, this is plain wrong
(pending a positive answer to the question whether iwlwifi can offload
broadcast/multicast encryption...) I suggest you remove the comment and
the tx flags manipulation.

The same comment is present in the #if 0'ed TKIP code and still talks
about CCMP. Please!

And all the crypto code does:

                cmd->cmd.tx.hdr[0].frame_control |=
                    cpu_to_le16(IEEE80211_FCTL_PROTECTED);

This is a no-op. Remove it. And while I'm at it, let me say this once
more: Do *NOT* work around the 802.11 implementation in mac80211 in the
driver. Fix bugs where there are any, and otherwise don't do work
mac80211 has done before.

And then I see things like
                cmd->cmd.tx.sec_ctl = 1 |       /* WEP */
                    (ctl->key_idx & 0x3) << 6;

Please use proper constants for the shifts and masks like everybody
else. Can't be too hard can it?

Generally, the driver needs *LOTS* more comments. While hacking on
mac80211 it is sometimes important to know what the hardware can handle,
and I'm unable to answer my initial question using the iwlwifi code. We
know a lot more about all the chipsets we've reverse engineered and are
able to judge their capabilities MUCH better than we'll ever be able to
by looking at the iwlwifi code :(

I can only guess this is intentional. While I applaud Intel's efforts in
wireless especially in the light of how some other vendors behave, this
is suboptimal and basically requires reverse engineering too to figure
out how things work.

Oh don't get me wrong. There are some comments explaining how things
work, for example the one about DMA queues. Except that it's not even
correct:

 * The four transmit queues allow for performing quality of service (qos)
 * transmissions as per the 802.11 protocol.  Currently Linux does not
 * provide a mechanism to the user for utilizing prioritized queues, so
 * we only utilize the first data transmit queue (queue1).

No? Why do we have an 802.11 qdisc then and all that stuff?

Further comments below while I'm at it.

 * mac80211 should also be examined to determine if sta_info is duplicating
 * the functionality provided here

absolutely. It also needs a good description of how the firmware manages
stations. And is this for AP case or just for remote APs?

In fact, come to think of it, the only thing it does to the hardware is
iwl_send_add_station so what you really want is a notification from
mac80211 when a sta_info is added/removed. And for that you build a huge
amount of station management code into the driver? That all needs to go
and new features like HT must be folded into mac80211 instead.

 * NOTE: This initializes the table for a single retry per data rate
 * which is not optimal.  Setting up an intelligent retry per rate  
 * requires feedback from transmission, which isn't exposed through 
 * rc80211_simple which is what this driver is currently using.

cute. No comment really, speaks for itself.

iwl_is_fat_tx_allowed and similar stuff needs to be in mac80211.

static struct ieee80211_ops iwl_hw_ops = {
        .ht_tx_agg_start = iwl_mac_ht_tx_agg_start,
        .ht_tx_agg_stop = iwl_mac_ht_tx_agg_stop,
        .ht_rx_agg_start = iwl_mac_ht_rx_agg_start,
        .ht_rx_agg_stop = iwl_mac_ht_rx_agg_stop,

That doesn't seem to be in my mac80211? Did you actually start
implementing HT features in mac80211? Why are they not available? Why
clutter the driver with #idef CONFIG_..._HT_AGG when that cannot
possibly be defined?

        /* Hard coded to start at the highest retry fallback position
         * until the 4965 specific rate control algorithm is tied in */
        tx->initial_rate_index = LINK_QUAL_MAX_RETRY_NUM - 1;

        /* Alternate between antenna A and B for successive frames */
        if (priv->use_ant_b_for_management_frame) {
                priv->use_ant_b_for_management_frame = 0;
                rate_flags = RATE_MCS_ANT_B_MSK;
        } else {
                priv->use_ant_b_for_management_frame = 1;
                rate_flags = RATE_MCS_ANT_A_MSK;
        }

Again, you were telling us that you absolutely need the rate control
algorithm tied in with things etc., yet you're not doing it.

iwl4965_sign_extend can be implemented a lot better like such:

static inline s32 sign_extend(u32 value, u8 bits)
{
	u8 shift = 32 - bits;

	return (s32)(value << shift) >> shift;
}

Note the renamed variables.

        struct ieee80211_rx_status stats = {
                .mactime = le32_to_cpu(rx_start->beacon_time_stamp),

That beacon_time_start variable looks rather misnamed. Or it shouldn't
be used here.

Enough for now again. I suggest when you run into problems like the rate
control algorithm not working well or HT mode missing, you start by
describing the hardware functionality and the resulting software problem
to linux-wireless so we can give advice on how to best implement things,
instead of stuffing tons of code into the driver and then pushing it out
as "hey this works can it be merged?"

Thanks,
johannes

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

Re: [PATCH V3] Add iwlwifi wireless drivers

[Ivo van Doorn]

On Wednesday 05 September 2007, Inaky Perez-Gonzalez wrote:
> On Tuesday 04 September 2007, Ivo van Doorn wrote:
> > On Tuesday 04 September 2007, dragoran wrote:
> > > On 9/4/07, Ivo van Doorn <ivdoorn@gmail.com> wrote:
> > > > On Tuesday 04 September 2007, dragoran wrote:
> > > > > >+static ssize_t show_rf_kill(struct device *d,
> > > >
> > > > Yes. It currently is only missing users.
> > > 
> > > ok thats great ;) is the (userspace) interface defined somewhere? or
> > > should I read the code to understand how it works? (would like to add
> > > support to hal)
> > 
> > There isn't a documentation file for it, so best thing to do would be looking at the code.
> > basically hal only needs to check the sysfs files:
> > 
> > 	name -> Name of device/interface
> > 	type -> wlan, bluetooth, irda
> 
> Can you add 'uwb' [for ultrawideband radios]? That way I could start to use it. A
> Doc file would help enourmously also :)

I'll create a doc file this weekend which will explain the sysfs files
and some instructions for drivers that want to implement it.

I'm not familiar with ultrawideband radios, don't they fall under the same
catagory as wireless lan?

Ivo

Re: [PATCH V3] Add iwlwifi wireless drivers

[Inaky Perez-Gonzalez]

On Thursday 06 September 2007, Ivo van Doorn wrote:
> >
> > Can you add 'uwb' [for ultrawideband radios]? That way I could start to use it. A
> > Doc file would help enourmously also :)
> 
> I'll create a doc file this weekend which will explain the sysfs files
> and some instructions for drivers that want to implement it.

Thanks,

> I'm not familiar with ultrawideband radios, don't they fall under the same
> catagory as wireless lan?

Nope, totally different beast; see linuxuwb.org. UWB is kind of like bluetooth 
on esteroids, for PAN applications. Wireless USB (for eaxmple) uses a UWB radio.

-- Inaky

Re: [PATCH V3] Add iwlwifi wireless drivers

[Ivo van Doorn]

On Thursday 06 September 2007, Inaky Perez-Gonzalez wrote:
> On Thursday 06 September 2007, Ivo van Doorn wrote:
> > >
> > > Can you add 'uwb' [for ultrawideband radios]? That way I could start to use it. A
> > > Doc file would help enourmously also :)
> > 
> > I'll create a doc file this weekend which will explain the sysfs files
> > and some instructions for drivers that want to implement it.
> 
> Thanks,
> 
> > I'm not familiar with ultrawideband radios, don't they fall under the same
> > catagory as wireless lan?
> 
> Nope, totally different beast; see linuxuwb.org. UWB is kind of like bluetooth 
> on esteroids, for PAN applications. Wireless USB (for eaxmple) uses a UWB radio.

Ah ok.
Well if there will be users, I'll create a patch to add the UWB to the list.
Patch will, just like the doc file, follow this weekend.

Ivo

Re: [PATCH V3] Add iwlwifi wireless drivers

[Zhu Yi]

On Wed, 2007-09-05 at 13:28 +0200, Johannes Berg wrote:
> I've been saying in every second mail
> now: run this driver against the mac80211 in net-2.6.24. It *will*
> trigger a WARN_ON() in the key handling code. 

Where is the WARN_ON? I ran the iwl4965 driver with net-2.6.24 commit-id
3259203776dc4c3f99b9fe3b303e3e76b13f934d, tried WEP, CCMP, no problem,
no WARN_ON. 

Since the mac80211 in net-2.6.24 has some structure change, I modified
the iwlwifi a little bit. You can find the iwlwifi driver for davem's
net-2.6.24 from:
http://intellinuxwireless.org/iwlwifi/v6-iwlwifi-net-2.6.24.patch

Thanks,
-yi

Re: [PATCH V3] Add iwlwifi wireless drivers

[Zhu Yi]

On Thu, 2007-09-06 at 13:00 +0200, Johannes Berg wrote:
> I was just looking through the iwlwifi sources to find out whether it
> can encrypt with a key even if the packet isn't unicast... Anybody kn=
ow?
> I myself removed the ability to upload multicast/broadcast keys becau=
se
> it was so unclear, but it seems that maybe the firmware knows about s=
ome
> sort of "virtual broadcast/multicast" station? How about multiple
> broadcast keys for APs with VLANs?
>=20
> In any case, I found this comment:
>=20
>                 /* XXX: ACK flag must be set for CCMP even if it
>                  * is a multicast/broadcast packet, because CCMP
>                  * group communication encrypted by GTK is
>                  * actually done by the AP. */
>                 cmd->cmd.tx.tx_flags |=3D TX_CMD_FLG_ACK_MSK;
>=20
> and it doesn't make any sense at all. Care to explain? If we're a
> station associated to some other AP, then mac80211 will *obviously* s=
et
> the "expect ack" bit on a "multicast data" packet that's being sent
> since it's actually unicast in 802.11 terms to the AP before it resen=
ds
> it using the group key...=20

This is what exactly the above comment tells you. The comment speaks fo=
r
the non-AP STA mode (the AP code was not there at that time). The
tx_flags is used by the firmware, so whatever mac80211 set its flag, it
has to be translated to the firmware's language -- because you are usin=
g
hardware crypto.

> And if we're an AP, this is plain wrong
> (pending a positive answer to the question whether iwlwifi can offloa=
d
> broadcast/multicast encryption...) I suggest you remove the comment a=
nd
> the tx flags manipulation.

The comment is not for AP mode.

> The same comment is present in the #if 0'ed TKIP code and still talks
> about CCMP. Please!

iwlwifi hardware/firmware only supports partial TKIP (ie. it can do [en=
|
de]crypt, but cannot build michael_mic), so we comment out the TKIP
hwcrypto code temporarily at this time. I agree the comment should run =
a
's/CCMP/TKIP/g' though.

> And all the crypto code does:
>=20
>                 cmd->cmd.tx.hdr[0].frame_control |=3D
>                     cpu_to_le16(IEEE80211_FCTL_PROTECTED);
>=20
> This is a no-op. Remove it. And while I'm at it, let me say this once
> more: Do *NOT* work around the 802.11 implementation in mac80211 in t=
he
> driver. Fix bugs where there are any, and otherwise don't do work
> mac80211 has done before.

It might be removed, but let me do some test (especially for EAP
packets) first.

> And then I see things like
>                 cmd->cmd.tx.sec_ctl =3D 1 |       /* WEP */
>                     (ctl->key_idx & 0x3) << 6;
>=20
> Please use proper constants for the shifts and masks like everybody
> else. Can't be too hard can it?

OK.

> Generally, the driver needs *LOTS* more comments. While hacking on
> mac80211 it is sometimes important to know what the hardware can hand=
le,
> and I'm unable to answer my initial question using the iwlwifi code. =
We
> know a lot more about all the chipsets we've reverse engineered and a=
re
> able to judge their capabilities MUCH better than we'll ever be able =
to
> by looking at the iwlwifi code :(

It is obviously clear, both in the comment and the code: when you get a
GTK, you have already got a PTK anyway. In the non-AP STA mode, you
encypt your packets with PTK (whatever for unicast, broadcast), the AP
receives it and rencrypts them with GTK. Your GTK is used to decrypt fo=
r
multicast, broadcast packets.

> I can only guess this is intentional. While I applaud Intel's efforts=
 in
> wireless especially in the light of how some other vendors behave, th=
is
> is suboptimal and basically requires reverse engineering too to figur=
e
> out how things work.

Intentional for what? When you are not able to follow a piece of code,
you think the author intentional mislead you, instead of blaming your
own capability? Arrogant!

> Oh don't get me wrong. There are some comments explaining how things
> work, for example the one about DMA queues. Except that it's not even
> correct:
>=20
>  * The four transmit queues allow for performing quality of service (=
qos)
>  * transmissions as per the 802.11 protocol.  Currently Linux does no=
t
>  * provide a mechanism to the user for utilizing prioritized queues, =
so
>  * we only utilize the first data transmit queue (queue1).
>=20
> No? Why do we have an 802.11 qdisc then and all that stuff?

iwlwifi _does_ honor the ieee80211_tx_control->queue field, I'll delete
the old comment. BTW=A3=AC the whole 802.11 qdisc hack will be removed =
with
the multiqueue supported in .24. =20

> Further comments below while I'm at it.
>=20
>  * mac80211 should also be examined to determine if sta_info is dupli=
cating
>  * the functionality provided here
>=20
> absolutely. It also needs a good description of how the firmware mana=
ges
> stations. And is this for AP case or just for remote APs?
>=20
> In fact, come to think of it, the only thing it does to the hardware =
is
> iwl_send_add_station so what you really want is a notification from
> mac80211 when a sta_info is added/removed. And for that you build a h=
uge
> amount of station management code into the driver? That all needs to =
go
> and new features like HT must be folded into mac80211 instead.
>=20
>  * NOTE: This initializes the table for a single retry per data rate
>  * which is not optimal.  Setting up an intelligent retry per rate =20
>  * requires feedback from transmission, which isn't exposed through=20
>  * rc80211_simple which is what this driver is currently using.
>=20
> cute. No comment really, speaks for itself.
>=20
> iwl_is_fat_tx_allowed and similar stuff needs to be in mac80211.
>=20
> static struct ieee80211_ops iwl_hw_ops =3D {
>         .ht_tx_agg_start =3D iwl_mac_ht_tx_agg_start,
>         .ht_tx_agg_stop =3D iwl_mac_ht_tx_agg_stop,
>         .ht_rx_agg_start =3D iwl_mac_ht_rx_agg_start,
>         .ht_rx_agg_stop =3D iwl_mac_ht_rx_agg_stop,
>=20
> That doesn't seem to be in my mac80211? Did you actually start
> implementing HT features in mac80211? Why are they not available? Why
> clutter the driver with #idef CONFIG_..._HT_AGG when that cannot
> possibly be defined?

We have the HT AGG support in our mac80211. Will submit the patch for
review when we are ready.

>         /* Hard coded to start at the highest retry fallback position
>          * until the 4965 specific rate control algorithm is tied in =
*/
>         tx->initial_rate_index =3D LINK_QUAL_MAX_RETRY_NUM - 1;
>=20
>         /* Alternate between antenna A and B for successive frames */
>         if (priv->use_ant_b_for_management_frame) {
>                 priv->use_ant_b_for_management_frame =3D 0;
>                 rate_flags =3D RATE_MCS_ANT_B_MSK;
>         } else {
>                 priv->use_ant_b_for_management_frame =3D 1;
>                 rate_flags =3D RATE_MCS_ANT_A_MSK;
>         }
>=20
> Again, you were telling us that you absolutely need the rate control
> algorithm tied in with things etc., yet you're not doing it.

I have sent a proposal to reorganize the header files so that we can
export a rate scale interface to the drivers. It is not perfect but it
works. Then we don't like it here and there. If you have a better idea,
why don't you speak it out?

> iwl4965_sign_extend can be implemented a lot better like such:
>=20
> static inline s32 sign_extend(u32 value, u8 bits)
> {
> 	u8 shift =3D 32 - bits;
>=20
> 	return (s32)(value << shift) >> shift;
> }

Really? Are you sure this work?

> Note the renamed variables.
>=20
>         struct ieee80211_rx_status stats =3D {
>                 .mactime =3D le32_to_cpu(rx_start->beacon_time_stamp)=
,
>=20
> That beacon_time_start variable looks rather misnamed. Or it shouldn'=
t
> be used here.

OK. I assuem you mentioned the beacon_time_stamp, it should be
->timestamp.

> Enough for now again.=20

Please don't. We have given you enough time for commenting the code.
Please do it in a batch like other people did. Don't squeeze a little i=
n
-rc2 and another little in -rc4, ... You are wasting people's time.

> I suggest when you run into problems like the rate
> control algorithm not working well or HT mode missing, you start by
> describing the hardware functionality and the resulting software prob=
lem
> to linux-wireless so we can give advice on how to best implement thin=
gs,
> instead of stuffing tons of code into the driver and then pushing it =
out

James had done this several months ago. We did everything the mac80211
people suggested, but the patch is still not merged. Do you think we'd
really like to workaound the mac80211 issues in the driver, or even hav=
e
our own mac80211 package? Absolutely not, if you mac80211 guys are easy
to work with. Without creating our own mac80211 package, today, the
users are still no better rate scale algorithm selection, no advanced
wireless QoS, no 11N support, not even mention about link aggregation.

> as "hey this works can it be merged?"

Something working is always better than something not working whatever
how beautiful the code looks like. And I believe the current iwlwifi
driver has reached the quality to be merged mainline (both it looks and
it works).

Thanks,
-yi
-
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] Add iwlwifi wireless drivers

[Johannes Berg]

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

On Fri, 2007-09-07 at 14:31 +0800, Zhu Yi wrote:

> >                 /* XXX: ACK flag must be set for CCMP even if it
> >                  * is a multicast/broadcast packet, because CCMP
> >                  * group communication encrypted by GTK is
> >                  * actually done by the AP. */
> >                 cmd->cmd.tx.tx_flags |= TX_CMD_FLG_ACK_MSK;
> > 
> > and it doesn't make any sense at all. Care to explain? If we're a
> > station associated to some other AP, then mac80211 will *obviously* set
> > the "expect ack" bit on a "multicast data" packet that's being sent
> > since it's actually unicast in 802.11 terms to the AP before it resends
> > it using the group key... 
> 
> This is what exactly the above comment tells you. The comment speaks for
> the non-AP STA mode (the AP code was not there at that time). The
> tx_flags is used by the firmware, so whatever mac80211 set its flag, it
> has to be translated to the firmware's language -- because you are using
> hardware crypto.

But you need to have another place where this flag is set based on the
equivalent mac80211 flag, so this is not necessary.

> BTW， the whole 802.11 qdisc hack will be removed with
> the multiqueue supported in .24.  

Yeah, is anybody working on that? I have to admit I didn't look into
that yet.

> We have the HT AGG support in our mac80211. Will submit the patch for
> review when we are ready.

Yeah I gathered that much from what Tomas said, but why do you keep
pushing for including this driver when you know it's not the right thing
to do?

> > iwl4965_sign_extend can be implemented a lot better like such:
> > 
> > static inline s32 sign_extend(u32 value, u8 bits)
> > {
> > 	u8 shift = 32 - bits;
> > 
> > 	return (s32)(value << shift) >> shift;
> > }
> 
> Really? Are you sure this work?

Hmm? Of course, shift it up and then do a signed shift down.

> > Enough for now again. 
> 
> Please don't. We have given you enough time for commenting the code.
> Please do it in a batch like other people did. Don't squeeze a little in
> -rc2 and another little in -rc4, ... You are wasting people's time.

I just don't have enough time/energy to read through all this at once.

> James had done this several months ago. We did everything the mac80211
> people suggested, but the patch is still not merged. Do you think we'd
> really like to workaound the mac80211 issues in the driver, or even have
> our own mac80211 package? Absolutely not, if you mac80211 guys are easy
> to work with. Without creating our own mac80211 package, today, the
> users are still no better rate scale algorithm selection, no advanced
> wireless QoS, no 11N support, not even mention about link aggregation.

You still haven't fixed the issues we pointed out in the 11N
deaggregation code. And maybe we missed some things too. Also, I
personally only recently started working on mac80211 and Jiri seems to
have vanished completely.

> Something working is always better than something not working whatever
> how beautiful the code looks like. And I believe the current iwlwifi
> driver has reached the quality to be merged mainline (both it looks and
> it works).

Well, I disagree. I see no incentive for you to help improving mac80211
when we have a driver with lots of stuff that works around mac80211.

In any case, let's get back to more productive things. I apologise for
anything I might have said that offended you, I didn't mean to. All the
code that superficially looks like workarounds around mac80211 stuff
made me not like the code and I suppose that showed.

Can you post patches against the iwlwifi currently included in
wireless-dev? Currently, it seems that I have to either dig in your
repository or through your patches to figure out the current state of
things I commented on, which is suboptimal because I don't even get to
see the fixes to things I pointed out. I'd like to have iwlwifi in
wireless-dev be essentially the same as is merged into mainline, modulo
the bits that depend on things like HT deaggregation that are not in
mainline yet.

Incidentally, I currently have just about as much a hard time getting
things changed in mac80211 as you do because nobody wants to review that
code. Hence, it would even help if you could review the patches I posted
and maybe look if that is easy/possible to support with iwlwifi and
possibly even adapt iwlwifi and test so that John feels more confident
in merging patches. Conversely, I promise I'll review and test any
patches you make to mac80211 as time permits.

johannes

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

Re: [PATCH V3] Add iwlwifi wireless drivers

[Johannes Berg]

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

On Fri, 2007-09-07 at 10:28 +0800, Zhu Yi wrote:

> Where is the WARN_ON? I ran the iwl4965 driver with net-2.6.24 commit-id
> 3259203776dc4c3f99b9fe3b303e3e76b13f934d, tried WEP, CCMP, no problem,
> no WARN_ON. 

Huh, ok, I looked again, it seems I changed the WARN_ON later to be less
severe, but there's this code in key.c:
        ret = key->local->ops->set_key(local_to_hw(key->local), DISABLE_KEY,
                                       key->sdata->dev->dev_addr, addr,
                                       &key->conf);

        if (ret)
                printk(KERN_ERR "mac80211-%s: failed to remove key "
                       "(%d, " MAC_FMT ") from hardware (%d)\n",
                       wiphy_name(key->local->hw.wiphy),
                       key->conf.keyidx, MAC_ARG(addr), ret);

which as far as I can tell will trigger with iwlwifi when a key is
removed from the hardware acceleration. Maybe my iwlwifi is not new
enough though.

> Since the mac80211 in net-2.6.24 has some structure change, I modified
> the iwlwifi a little bit. You can find the iwlwifi driver for davem's
> net-2.6.24 from:
> http://intellinuxwireless.org/iwlwifi/v6-iwlwifi-net-2.6.24.patch

Thanks. Since wireless-dev is mostly equivalent, can you post patches
against that? I know it's suboptimal to be essentially maintaining two
drivers..

johannes

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

Re: [PATCH V3] Add iwlwifi wireless drivers

[Zhu Yi]

On Fri, 2007-09-07 at 15:40 +0200, Johannes Berg wrote:
> But you need to have another place where this flag is set based on th=
e
> equivalent mac80211 flag, so this is not necessary.

OK. Will use mac80211's indication if mac80211 already takes care about
the "trick".

> > BTW=A3=AC the whole 802.11 qdisc hack will be removed with
> > the multiqueue supported in .24. =20
>=20
> Yeah, is anybody working on that? I have to admit I didn't look into
> that yet.

If nobody will work on that, we Intel will do. I remembered Tomas
promised John in this OLS ;)

> > We have the HT AGG support in our mac80211. Will submit the patch f=
or
> > review when we are ready.
>=20
> Yeah I gathered that much from what Tomas said, but why do you keep
> pushing for including this driver when you know it's not the right th=
ing
> to do?
>=20
> > > iwl4965_sign_extend can be implemented a lot better like such:
> > >=20
> > > static inline s32 sign_extend(u32 value, u8 bits)
> > > {
> > > 	u8 shift =3D 32 - bits;
> > >=20
> > > 	return (s32)(value << shift) >> shift;
> > > }
> >=20
> > Really? Are you sure this work?
>=20
> Hmm? Of course, shift it up and then do a signed shift down.

It should be 31, not 32 (shift =3D 31 - bits). Will use your suggestion=
=2E

> You still haven't fixed the issues we pointed out in the 11N
> deaggregation code. And maybe we missed some things too. Also, I
> personally only recently started working on mac80211 and Jiri seems t=
o
> have vanished completely.

OK. Let me remove the 11N stuff in iwlwifi for .24 and then we do
another round of mac80211 11N patches submission/review later. I believ=
e
you can do a better job than Jiri did.

> In any case, let's get back to more productive things. I apologise fo=
r
> anything I might have said that offended you, I didn't mean to. All t=
he
> code that superficially looks like workarounds around mac80211 stuff
> made me not like the code and I suppose that showed.
>=20
> Can you post patches against the iwlwifi currently included in
> wireless-dev? Currently, it seems that I have to either dig in your
> repository or through your patches to figure out the current state of
> things I commented on, which is suboptimal because I don't even get t=
o
> see the fixes to things I pointed out. I'd like to have iwlwifi in
> wireless-dev be essentially the same as is merged into mainline, modu=
lo
> the bits that depend on things like HT deaggregation that are not in
> mainline yet.

I thought John would take my .24 iwlwifi patch to wireless-dev GIT
directly since he just rebased the tree (at that time) and all the
commits for iwlwifi were merged into one big patch. Anyway, I will
repost it explicitly for wireless-dev next time: iwlwifi driver + HT
features for the iwlwifi (or everything) branch; and iwlwifi driver
without HT features for pending-upstream branch. We can work on iwlwifi
driver to make it in .24 first and sort out HT things in .25.

> Incidentally, I currently have just about as much a hard time getting
> things changed in mac80211 as you do because nobody wants to review t=
hat
> code. Hence, it would even help if you could review the patches I pos=
ted
> and maybe look if that is easy/possible to support with iwlwifi and
> possibly even adapt iwlwifi and test so that John feels more confiden=
t
> in merging patches. Conversely, I promise I'll review and test any
> patches you make to mac80211 as time permits.

I totally agree with you.

Thanks,
-yi
-
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] Add iwlwifi wireless drivers

[Johannes Berg]

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

On Mon, 2007-09-10 at 10:09 +0800, Zhu Yi wrote:
> On Fri, 2007-09-07 at 15:40 +0200, Johannes Berg wrote:
> > But you need to have another place where this flag is set based on the
> > equivalent mac80211 flag, so this is not necessary.
> 
> OK. Will use mac80211's indication if mac80211 already takes care about
> the "trick".

If not I'd consider it a bug. But I don't see a "trick" there anyhow.
What's non-standard in this behaviour? Almost every unicast packet is
expected to be acknowledged.

> > > BTW， the whole 802.11 qdisc hack will be removed with
> > > the multiqueue supported in .24.  

> If nobody will work on that, we Intel will do. I remembered Tomas
> promised John in this OLS ;)

:)
Sounds good. Do you guys have contact to whoever wrote it? ISTR it came
from Intel in the first place.

> It should be 31, not 32 (shift = 31 - bits). Will use your suggestion.

Huh. /me double-checks. Am I doing this bit-calculation totally wrong? I
thought you had a "bits"-bit number that included the sign in the bit
"bits-1" (if you number 0 through "bits-1"). Then you have to shift up
by "32 - bits" so that the "bits" bits including the sign live in the
upper "bits" bits of the 32-bit number, and then shift down again by the
same amount to do the sign extension. I wonder if there's a good way to
ask gcc to do this :)

> OK. Let me remove the 11N stuff in iwlwifi for .24 and then we do
> another round of mac80211 11N patches submission/review later.

That would be much appreciated. We really also should have rate control
be able to influence that generically and lots more things that have to
be figured out but that are quite hard to work with when the 802.11n
drafts are not available to many.

> I believe
> you can do a better job than Jiri did.

Thanks. I'm not sure I can live up to that though; for example I'll have
to mostly disappear for the next 10 days or so to prepare for an exam.
I'll be around, but not patching quite as much ;)

> I thought John would take my .24 iwlwifi patch to wireless-dev GIT
> directly since he just rebased the tree (at that time) and all the
> commits for iwlwifi were merged into one big patch.

Ah ok.

> Anyway, I will
> repost it explicitly for wireless-dev next time: iwlwifi driver + HT
> features for the iwlwifi (or everything) branch; and iwlwifi driver
> without HT features for pending-upstream branch. We can work on iwlwifi
> driver to make it in .24 first and sort out HT things in .25.

Sounds good. I'll take another look then.

Thanks,
johannes

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

Re: [PATCH V3] Add iwlwifi wireless drivers

[Tomas Winkler]

On 9/10/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2007-09-10 at 10:09 +0800, Zhu Yi wrote:
> > On Fri, 2007-09-07 at 15:40 +0200, Johannes Berg wrote:
> > > But you need to have another place where this flag is set based on the
> > > equivalent mac80211 flag, so this is not necessary.
> >
> > OK. Will use mac80211's indication if mac80211 already takes care about
> > the "trick".
>
> If not I'd consider it a bug. But I don't see a "trick" there anyhow.
> What's non-standard in this behaviour? Almost every unicast packet is
> expected to be acknowledged.
>
> > > > BTW， the whole 802.11 qdisc hack will be removed with
> > > > the multiqueue supported in .24.
>
> > If nobody will work on that, we Intel will do. I remembered Tomas
> > promised John in this OLS ;)
>
> :)
> Sounds good. Do you guys have contact to whoever wrote it? ISTR it came
> from Intel in the first place.
>
It is my best intention yet first we need native interface which make
in turn problems in eb tables. So it is a long shot.

> > It should be 31, not 32 (shift = 31 - bits). Will use your suggestion.
>
> Huh. /me double-checks. Am I doing this bit-calculation totally wrong? I
> thought you had a "bits"-bit number that included the sign in the bit
> "bits-1" (if you number 0 through "bits-1"). Then you have to shift up
> by "32 - bits" so that the "bits" bits including the sign live in the
> upper "bits" bits of the 32-bit number, and then shift down again by the
> same amount to do the sign extension. I wonder if there's a good way to
> ask gcc to do this :)
>
> > OK. Let me remove the 11N stuff in iwlwifi for .24 and then we do
> > another round of mac80211 11N patches submission/review later.
>
> That would be much appreciated. We really also should have rate control
> be able to influence that generically and lots more things that have to
> be figured out but that are quite hard to work with when the 802.11n
> drafts are not available to many.

I wouldn't appreciate this at all. 11n is major feature of our NIC.
Major obstacle in finally pushing 11n is constant  code base change of
iwlwifi. This is already 4th  code base. The latest was because the
driver didn't look nice enough, what an engineering   reason! In the
bottom line we are hunting our own tail for wrong reasons

> > I believe
> > you can do a better job than Jiri did.
>
> Thanks. I'm not sure I can live up to that though; for example I'll have
> to mostly disappear for the next 10 days or so to prepare for an exam.
> I'll be around, but not patching quite as much ;)
>
> > I thought John would take my .24 iwlwifi patch to wireless-dev GIT
> > directly since he just rebased the tree (at that time) and all the
> > commits for iwlwifi were merged into one big patch.
>
> Ah ok.
>
> > Anyway, I will
> > repost it explicitly for wireless-dev next time: iwlwifi driver + HT
> > features for the iwlwifi (or everything) branch; and iwlwifi driver
> > without HT features for pending-upstream branch. We can work on iwlwifi
> > driver to make it in .24 first and sort out HT things in .25.
>
> Sounds good. I'll take another look then.
>
> Thanks,
> johannes
>
>

Re: [PATCH V3] Add iwlwifi wireless drivers

[Johannes Berg]

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

On Mon, 2007-09-10 at 17:20 +0300, Tomas Winkler wrote:

> It is my best intention yet first we need native interface which make
> in turn problems in eb tables. So it is a long shot.

Ok. We've managed long enough without it so I guess we can wait :)

> I wouldn't appreciate this at all. 11n is major feature of our NIC.
> Major obstacle in finally pushing 11n is constant  code base change of
> iwlwifi. This is already 4th  code base. The latest was because the
> driver didn't look nice enough, what an engineering   reason! In the
> bottom line we are hunting our own tail for wrong reasons

Well, I believe that there are bad layering violations in your current
driver, namely looking at the packets mac80211 sends, doing 11N
manipulations and everything in the driver and duplicating the sta_info
stuff because mac80211 happens to be missing a few hooks. If you think
those are "wrong reasons" that's fine with me.

Personally, I'm just raising these points and marking them down as
"against merging as-is". If others don't care about them, that's ok with
me.

johannes

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

Re: [PATCH V3] Add iwlwifi wireless drivers

[Tomas Winkler]

On 9/11/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2007-09-10 at 17:20 +0300, Tomas Winkler wrote:
>
> > It is my best intention yet first we need native interface which make
> > in turn problems in eb tables. So it is a long shot.
>
> Ok. We've managed long enough without it so I guess we can wait :)
>
> > I wouldn't appreciate this at all. 11n is major feature of our NIC.
> > Major obstacle in finally pushing 11n is constant  code base change of
> > iwlwifi. This is already 4th  code base. The latest was because the
> > driver didn't look nice enough, what an engineering   reason! In the
> > bottom line we are hunting our own tail for wrong reasons
>
> Well, I believe that there are bad layering violations in your current
> driver, namely looking at the packets mac80211 sends, doing 11N
> manipulations and everything in the driver and duplicating the sta_info
> stuff because mac80211 happens to be missing a few hooks. If you think
> those are "wrong reasons" that's fine with me.

There is no doubt that what you currently see is ugly, we reworked the
code and added these few hooks the problem is that they don't apply
anymore  to recent code, since I'm stabilizing basic flows after each
code base in the middle of development.
I know I'm not 'release often' complained but I really test the code
that I'm publishing, it takes time.
I prefer to have stable driver and fix it step by step then releasing
30 patches that nobody have time to consume and review in addition it
just kills the functionality. Don't know maybe I'm too old for Linux
:)

>
> Personally, I'm just raising these points and marking them down as
> "against merging as-is". If others don't care about them, that's ok with
> me.
>
After all it is functional driver. It gets ~60Mps in TCP even more.

> johannes
>
>

Re: [PATCH V3] Add iwlwifi wireless drivers

[Michael Buesch]

On Tuesday 11 September 2007 19:37:40 Tomas Winkler wrote:
> On 9/11/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Mon, 2007-09-10 at 17:20 +0300, Tomas Winkler wrote:
> >
> > > It is my best intention yet first we need native interface which make
> > > in turn problems in eb tables. So it is a long shot.
> >
> > Ok. We've managed long enough without it so I guess we can wait :)
> >
> > > I wouldn't appreciate this at all. 11n is major feature of our NIC.
> > > Major obstacle in finally pushing 11n is constant  code base change of
> > > iwlwifi. This is already 4th  code base. The latest was because the
> > > driver didn't look nice enough, what an engineering   reason! In the
> > > bottom line we are hunting our own tail for wrong reasons
> >
> > Well, I believe that there are bad layering violations in your current
> > driver, namely looking at the packets mac80211 sends, doing 11N
> > manipulations and everything in the driver and duplicating the sta_info
> > stuff because mac80211 happens to be missing a few hooks. If you think
> > those are "wrong reasons" that's fine with me.
> 
> There is no doubt that what you currently see is ugly, we reworked the
> code and added these few hooks the problem is that they don't apply
> anymore  to recent code, since I'm stabilizing basic flows after each
> code base in the middle of development.
> I know I'm not 'release often' complained but I really test the code
> that I'm publishing, it takes time.
> I prefer to have stable driver and fix it step by step then releasing
> 30 patches that nobody have time to consume and review in addition it
> just kills the functionality. Don't know maybe I'm too old for Linux
> :)

Well, if you post the patches to get included into wireless-dev,
people will start testing them. So they test them im parallel
with you (you will do your testing of course as well).
Of course you do some basic testing before releasing the patches,
such as "does it compile" or "does the card basically still work".
But with this you get _more_ review and testing. That's the whole
point of "release early and often".
Testing early-released patches is SMP, while testing alone is UP. ;)

> > Personally, I'm just raising these points and marking them down as
> > "against merging as-is". If others don't care about them, that's ok with
> > me.
> >
> After all it is functional driver. It gets ~60Mps in TCP even more.

Well, lots of other functional stuff was rejected, because it was
simply ugly and a maintanance hell. (I'm not saying that your driver
is such a thing. I didn't look at it, yet).

-- 
Greetings Michael.