[RESEND] [PATCHv2] b43 add harware tkip

[RESEND] [PATCHv2] b43 add harware tkip

[gregor kowski]

Update : work with qos, implement dump key, fix an issue with setting
random value on tkip key clear.
PS : this depends on "b43 : remove old kidx API"

This add hardware tkip for b43.

It uncovered a bug in b43_write_probe_resp_template : it is writing at the
wrong shm offset, it is in the B43_SHM_SH_TKIPTSCTTAK zone. This patch
comments these writes.

Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>
Index: linux-2.6/drivers/net/wireless/b43/dma.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/dma.c	2009-07-25
10:58:30.000000000 +0000
+++ linux-2.6/drivers/net/wireless/b43/dma.c	2009-07-27 20:46:05.000000000 +0000
@@ -1188,7 +1188,7 @@
 	header = &(ring->txhdr_cache[(slot / TX_SLOTS_PER_FRAME) * hdrsize]);
 	cookie = generate_cookie(ring, slot);
 	err = b43_generate_txhdr(ring->dev, header,
-				 skb->data, skb->len, info, cookie);
+				 skb, info, cookie);
 	if (unlikely(err)) {
 		ring->current_slot = old_top_slot;
 		ring->used_slots = old_used_slots;
Index: linux-2.6/drivers/net/wireless/b43/main.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/main.c	2009-07-27
20:40:14.000000000 +0000
+++ linux-2.6/drivers/net/wireless/b43/main.c	2009-07-27
20:46:05.000000000 +0000
@@ -836,6 +836,64 @@
 	}
 }

+/* The ucode will use this with key to decrypt rx packets.
+ * It will first check if the iv32 match,
+ * - if they don't it returns the packet without decryption (and software
+ *   decryption can be done). That's what happen when iv16 wrap.
+ * - if they do, the rc4 key is computed with tkip phase2, and
+ *   the wep decryption is tried on the packet. Either it will
+ *   success and B43_RX_MAC_DEC is returned, either it fails
+ *   and B43_RX_MAC_DEC|B43_RX_MAC_DECERR is returned and the packet
+ *   is not usable (wrong key used on it).
+ * So in order to never have B43_RX_MAC_DECERR, we should provide
+ * a iv32 and phase1key that match. Because we drop packets in case of
+ * B43_RX_MAC_DECERR, if we have a correct iv32 but a wrong phase1key, all
+ * packets will be lost without higher layer knowing (ie no resync possible
+ * until next wrap).
+ *
+ * NOTE : this should support 50 key like RCMTA because
+ * (B43_SHM_SH_KEYIDXBLOCK - B43_SHM_SH_TKIPTSCTTAK)/14 = 50
+ */
+static void rx_tkip_phase1_write(struct b43_wldev *dev, u8 index, u32 iv32,
+		u16 *phase1key)
+{
+	unsigned int i;
+	u32 offset;
+	const u8 per_sta_keys_start = 4;
+
+	B43_WARN_ON(index < per_sta_keys_start);
+	/* We have two default TX keys and possibly two default RX keys.
+	 * Physical mac 0 is mapped to physical key 4 or 8, depending
+	 * on the firmware version.
+	 * So we must adjust the index here.
+	 */
+	index -= per_sta_keys_start;
+
+	if (b43_debug(dev, B43_DBG_KEYS))
+		b43dbg(dev->wl, "rx_tkip_phase1_write : idx 0x%x, iv32 0x%x\n",
+				index, iv32);
+	/* Write the key to the  RX tkip shared mem */
+	offset = B43_SHM_SH_TKIPTSCTTAK + index * (10 + 4);
+	for (i = 0; i < 10; i += 2) {
+		b43_shm_write16(dev, B43_SHM_SHARED, offset + i, phase1key[i/2]);
+	}
+	b43_shm_write16(dev, B43_SHM_SHARED, offset + i, iv32);
+	b43_shm_write16(dev, B43_SHM_SHARED, offset + i + 2, iv32>>16);
+}
+
+static void b43_mac_update_tkip_key(struct ieee80211_hw *hw,
+			struct ieee80211_key_conf *keyconf, const u8 *addr,
+			u32 iv32, u16 *phase1key)
+{
+	struct b43_wl *wl = hw_to_b43_wl(hw);
+	struct b43_wldev *dev = wl->current_dev;
+	int index = keyconf->hw_key_idx;
+	keymac_write(dev, index, NULL);	/* First zero out mac to avoid race */
+
+	rx_tkip_phase1_write(dev, index, iv32, phase1key);
+	keymac_write(dev, index, addr);
+}
+
 static void do_key_write(struct b43_wldev *dev,
 			 u8 index, u8 algorithm,
 			 const u8 *key, size_t key_len, const u8 *mac_addr)
@@ -848,6 +906,19 @@

 	if (index >= per_sta_keys_start)
 		keymac_write(dev, index, NULL);	/* First zero out mac. */
+	if (algorithm == B43_SEC_ALGO_TKIP) {
+		/*
+		 * We should provide an initial iv32, phase1key pair.
+		 * We could start with iv32=0 and compute the corresponding
+		 * phase1key, but this mean calling ieee80211_get_tkip_key
+		 * with a fake skb (or export other tkip function).
+		 * Because we are lazy we hope iv32 won't start with
+		 * 0xffffffff and let's b43_mac_update_tkip_key provide a
+		 * correct pair.
+		 */
+		rx_tkip_phase1_write(dev, index, 0xffffffff, (u16*)buf);
+	} else if (index >= per_sta_keys_start) /* clear it */
+		rx_tkip_phase1_write(dev, index, 0, (u16*)buf);
 	if (key)
 		memcpy(buf, key, key_len);
 	key_write(dev, index, algorithm, buf);
@@ -865,6 +936,8 @@
 {
 	int i;

+	if (algorithm == B43_SEC_ALGO_TKIP && key_len == 32)
+		key_len = 16;
 	if (key_len > B43_SEC_KEYSIZE)
 		return -EINVAL;
 	for (i = 0; i < dev->max_nr_keys; i++) {
@@ -946,6 +1019,14 @@
 		printk("   Algo: %04X/%02X", algo, key->algorithm);

 		if (index >= 4) {
+			if (key->algorithm == B43_SEC_ALGO_TKIP) {
+				printk("   TKIP: ");
+				offset = B43_SHM_SH_TKIPTSCTTAK + (index - 4) * (10 + 4);
+				for (i = 0; i < 14; i+=2) {
+					u16 tmp = b43_shm_read16(dev, B43_SHM_SHARED, offset + i);
+					printk("%02X%02X", (tmp & 0xFF), ((tmp >> 8) & 0xFF));
+				}
+			}
 			rcmta0 = b43_shm_read32(dev, B43_SHM_RCMTA,
 						((index - 4) * 2) + 0);
 			rcmta1 = b43_shm_read16(dev, B43_SHM_RCMTA,
@@ -1505,10 +1586,13 @@
 	/* Looks like PLCP headers plus packet timings are stored for
 	 * all possible basic rates
 	 */
+	/* FIXME this is the wrong offset : it goes in tkip rx phase1 shm */
+#if 0
 	b43_write_probe_resp_plcp(dev, 0x31A, size, &b43_b_ratetable[0]);
 	b43_write_probe_resp_plcp(dev, 0x32C, size, &b43_b_ratetable[1]);
 	b43_write_probe_resp_plcp(dev, 0x33E, size, &b43_b_ratetable[2]);
 	b43_write_probe_resp_plcp(dev, 0x350, size, &b43_b_ratetable[3]);
+#endif

 	size = min((size_t) size, 0x200 - sizeof(struct b43_plcp_hdr6));
 	b43_write_template_common(dev, probe_resp_data,
@@ -3667,8 +3751,9 @@

 	switch (cmd) {
 	case SET_KEY:
-		if (algorithm == B43_SEC_ALGO_TKIP) {
-			/* FIXME: No TKIP hardware encryption for now. */
+		if (algorithm == B43_SEC_ALGO_TKIP &&
+		    !(key->flags & IEEE80211_KEY_FLAG_PAIRWISE)) {
+			/* We support only pairwise key */
 			err = -EOPNOTSUPP;
 			goto out_unlock;
 		}
@@ -3698,6 +3783,8 @@
 				     b43_hf_read(dev) & ~B43_HF_USEDEFKEYS);
 		}
 		key->flags |= IEEE80211_KEY_FLAG_GENERATE_IV;
+		if (algorithm == B43_SEC_ALGO_TKIP)
+			key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
 		break;
 	case DISABLE_KEY: {
 		err = b43_key_clear(dev, key->hw_key_idx);
@@ -4425,6 +4512,7 @@
 	.bss_info_changed	= b43_op_bss_info_changed,
 	.configure_filter	= b43_op_configure_filter,
 	.set_key		= b43_op_set_key,
+	.update_tkip_key	= b43_mac_update_tkip_key,
 	.get_stats		= b43_op_get_stats,
 	.get_tx_stats		= b43_op_get_tx_stats,
 	.get_tsf		= b43_op_get_tsf,
Index: linux-2.6/drivers/net/wireless/b43/pio.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/pio.c	2009-07-25
10:58:30.000000000 +0000
+++ linux-2.6/drivers/net/wireless/b43/pio.c	2009-07-27 20:46:05.000000000 +0000
@@ -461,8 +461,8 @@

 	cookie = generate_cookie(q, pack);
 	hdrlen = b43_txhdr_size(q->dev);
-	err = b43_generate_txhdr(q->dev, (u8 *)&txhdr, skb->data,
-				 skb->len, info, cookie);
+	err = b43_generate_txhdr(q->dev, (u8 *)&txhdr, skb,
+				 info, cookie);
 	if (err)
 		return err;

Index: linux-2.6/drivers/net/wireless/b43/xmit.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/xmit.c	2009-07-27
20:40:14.000000000 +0000
+++ linux-2.6/drivers/net/wireless/b43/xmit.c	2009-07-27
20:46:05.000000000 +0000
@@ -180,11 +180,12 @@
 /* Generate a TX data header. */
 int b43_generate_txhdr(struct b43_wldev *dev,
 		       u8 *_txhdr,
-		       const unsigned char *fragment_data,
-		       unsigned int fragment_len,
+		       struct sk_buff *skb_frag,
 		       struct ieee80211_tx_info *info,
 		       u16 cookie)
 {
+	const unsigned char *fragment_data = skb_frag->data;
+	unsigned int fragment_len = skb_frag->len;
 	struct b43_txhdr *txhdr = (struct b43_txhdr *)_txhdr;
 	const struct b43_phy *phy = &dev->phy;
 	const struct ieee80211_hdr *wlhdr =
@@ -257,9 +258,25 @@
 		mac_ctl |= (key->algorithm << B43_TXH_MAC_KEYALG_SHIFT) &
 			   B43_TXH_MAC_KEYALG;
 		wlhdr_len = ieee80211_hdrlen(fctl);
-		iv_len = min((size_t) info->control.hw_key->iv_len,
-			     ARRAY_SIZE(txhdr->iv));
-		memcpy(txhdr->iv, ((u8 *) wlhdr) + wlhdr_len, iv_len);
+		if (key->algorithm == B43_SEC_ALGO_TKIP) {
+			u16 phase1key[5];
+			int i;
+			/* we give the phase1key and iv16 here, the key is stored in
+			 * shm. With that the hardware can do phase 2 and encryption.
+			 */
+			ieee80211_get_tkip_key(info->control.hw_key, skb_frag,
IEEE80211_TKIP_P1_KEY, (u8*)phase1key);
+			/* phase1key is in host endian */
+			for (i = 0; i < 5; i++)
+				phase1key[i] = cpu_to_le16(phase1key[i]);
+
+			memcpy(txhdr->iv, phase1key, 10);
+			/* iv16 */
+			memcpy(txhdr->iv+10, ((u8 *) wlhdr) + wlhdr_len, 3);
+		} else {
+			iv_len = min((size_t) info->control.hw_key->iv_len,
+				     ARRAY_SIZE(txhdr->iv));
+			memcpy(txhdr->iv, ((u8 *) wlhdr) + wlhdr_len, iv_len);
+		}
 	}
 	if (b43_is_old_txhdr_format(dev)) {
 		b43_generate_plcp_hdr((struct b43_plcp_hdr4 *)(&txhdr->old_format.plcp),
Index: linux-2.6/drivers/net/wireless/b43/xmit.h
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/xmit.h	2009-07-27
20:40:14.000000000 +0000
+++ linux-2.6/drivers/net/wireless/b43/xmit.h	2009-07-27
20:46:05.000000000 +0000
@@ -176,8 +176,7 @@

 int b43_generate_txhdr(struct b43_wldev *dev,
 		       u8 * txhdr,
-		       const unsigned char *fragment_data,
-		       unsigned int fragment_len,
+		       struct sk_buff *skb_frag,
 		       struct ieee80211_tx_info *txctl, u16 cookie);

 /* Transmit Status */

Re: [RESEND] [PATCHv2] b43 add harware tkip

[Michael Buesch]

On Monday 27 July 2009 22:49:17 gregor kowski wrote:
> Update : work with qos, implement dump key, fix an issue with setting
> random value on tkip key clear.
> PS : this depends on "b43 : remove old kidx API"
> 
> This add hardware tkip for b43.

First of all, I don't really want to support hw tkip on b43.
Broadcom removed the support for hw tkip from their drivers. I bet they
had a very good reason to do so.

So can we put hw tkip support under module parameter that defaults to OFF?

> Index: linux-2.6/drivers/net/wireless/b43/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/net/wireless/b43/main.c	2009-07-27
> 20:40:14.000000000 +0000
> +++ linux-2.6/drivers/net/wireless/b43/main.c	2009-07-27
> 20:46:05.000000000 +0000
> @@ -836,6 +836,64 @@
>  	}
>  }
> 
> +/* The ucode will use this with key to decrypt rx packets.
> + * It will first check if the iv32 match,
> + * - if they don't it returns the packet without decryption (and software
> + *   decryption can be done). That's what happen when iv16 wrap.
> + * - if they do, the rc4 key is computed with tkip phase2, and
> + *   the wep decryption is tried on the packet. Either it will
> + *   success and B43_RX_MAC_DEC is returned, either it fails
> + *   and B43_RX_MAC_DEC|B43_RX_MAC_DECERR is returned and the packet
> + *   is not usable (wrong key used on it).
> + * So in order to never have B43_RX_MAC_DECERR, we should provide
> + * a iv32 and phase1key that match. Because we drop packets in case of
> + * B43_RX_MAC_DECERR, if we have a correct iv32 but a wrong phase1key, all
> + * packets will be lost without higher layer knowing (ie no resync possible
> + * until next wrap).
> + *
> + * NOTE : this should support 50 key like RCMTA because
> + * (B43_SHM_SH_KEYIDXBLOCK - B43_SHM_SH_TKIPTSCTTAK)/14 = 50
> + */

It's rather hard for me to understand your comments. You should try to fix
a few spelling and grammar errors and such...

> +static void rx_tkip_phase1_write(struct b43_wldev *dev, u8 index, u32 iv32,
> +		u16 *phase1key)
> +{
> +	unsigned int i;
> +	u32 offset;
> +	const u8 per_sta_keys_start = 4;
> +
> +	B43_WARN_ON(index < per_sta_keys_start);
> +	/* We have two default TX keys and possibly two default RX keys.

That comment is wrong.
We have 4 default TX/RX keys.

> +	 * Physical mac 0 is mapped to physical key 4 or 8, depending

It's mapped to 4, because you removed the old API support.
(I'm not sure whether we want to remove that support, yet. Gimme some time on it...)

> +	 * on the firmware version.
> +	 * So we must adjust the index here.
> +	 */
> +	index -= per_sta_keys_start;
> +
> +	if (b43_debug(dev, B43_DBG_KEYS))
> +		b43dbg(dev->wl, "rx_tkip_phase1_write : idx 0x%x, iv32 0x%x\n",
> +				index, iv32);

Add curly brackets. (yes, the code is correct. But our coding style is to use brackets
on multiline-indents).

> +	/* Write the key to the  RX tkip shared mem */
> +	offset = B43_SHM_SH_TKIPTSCTTAK + index * (10 + 4);
> +	for (i = 0; i < 10; i += 2) {
> +		b43_shm_write16(dev, B43_SHM_SHARED, offset + i, phase1key[i/2]);
                                                                           ^^^
Coding style

> +	}

Remove curly brackets.

> +	b43_shm_write16(dev, B43_SHM_SHARED, offset + i, iv32);
> +	b43_shm_write16(dev, B43_SHM_SHARED, offset + i + 2, iv32>>16);
                                                             ^^^^^^^^

Coding style

> +}
> +
> +static void b43_mac_update_tkip_key(struct ieee80211_hw *hw,
> +			struct ieee80211_key_conf *keyconf, const u8 *addr,
> +			u32 iv32, u16 *phase1key)
> +{
> +	struct b43_wl *wl = hw_to_b43_wl(hw);
> +	struct b43_wldev *dev = wl->current_dev;
> +	int index = keyconf->hw_key_idx;
> +	keymac_write(dev, index, NULL);	/* First zero out mac to avoid race */
> +
> +	rx_tkip_phase1_write(dev, index, iv32, phase1key);
> +	keymac_write(dev, index, addr);
> +}

Completely lacks locking. You need to hold the mutex and check for dev validity.
See other b43_op_* for examples.

>  static void do_key_write(struct b43_wldev *dev,
>  			 u8 index, u8 algorithm,
>  			 const u8 *key, size_t key_len, const u8 *mac_addr)
> @@ -848,6 +906,19 @@
> 
>  	if (index >= per_sta_keys_start)
>  		keymac_write(dev, index, NULL);	/* First zero out mac. */
> +	if (algorithm == B43_SEC_ALGO_TKIP) {
> +		/*
> +		 * We should provide an initial iv32, phase1key pair.
> +		 * We could start with iv32=0 and compute the corresponding
> +		 * phase1key, but this mean calling ieee80211_get_tkip_key
> +		 * with a fake skb (or export other tkip function).
> +		 * Because we are lazy we hope iv32 won't start with
> +		 * 0xffffffff and let's b43_mac_update_tkip_key provide a
> +		 * correct pair.
> +		 */
> +		rx_tkip_phase1_write(dev, index, 0xffffffff, (u16*)buf);
> +	} else if (index >= per_sta_keys_start) /* clear it */
> +		rx_tkip_phase1_write(dev, index, 0, (u16*)buf);

Why do you state /* clear it */, but yet you pass the key?
Shouldn't you pass a NULL pointer and modify rx_tkip_phase1_write() to cope with
NULL pointers? (write zeros if the key is NULL).

>  	if (key)
>  		memcpy(buf, key, key_len);
>  	key_write(dev, index, algorithm, buf);
> @@ -865,6 +936,8 @@
>  {
>  	int i;
> 
> +	if (algorithm == B43_SEC_ALGO_TKIP && key_len == 32)
> +		key_len = 16;

Can you add a comment to this? It doesn't make sense without explanation.

>  	if (key_len > B43_SEC_KEYSIZE)
>  		return -EINVAL;
>  	for (i = 0; i < dev->max_nr_keys; i++) {
> @@ -946,6 +1019,14 @@
>  		printk("   Algo: %04X/%02X", algo, key->algorithm);
> 
>  		if (index >= 4) {
> +			if (key->algorithm == B43_SEC_ALGO_TKIP) {
> +				printk("   TKIP: ");
> +				offset = B43_SHM_SH_TKIPTSCTTAK + (index - 4) * (10 + 4);
> +				for (i = 0; i < 14; i+=2) {
                                                    ^^^^
Coding style.

> +					u16 tmp = b43_shm_read16(dev, B43_SHM_SHARED, offset + i);
> +					printk("%02X%02X", (tmp & 0xFF), ((tmp >> 8) & 0xFF));
> +				}
> +			}
>  			rcmta0 = b43_shm_read32(dev, B43_SHM_RCMTA,
>  						((index - 4) * 2) + 0);
>  			rcmta1 = b43_shm_read16(dev, B43_SHM_RCMTA,
> @@ -1505,10 +1586,13 @@
>  	/* Looks like PLCP headers plus packet timings are stored for
>  	 * all possible basic rates
>  	 */
> +	/* FIXME this is the wrong offset : it goes in tkip rx phase1 shm */
> +#if 0
>  	b43_write_probe_resp_plcp(dev, 0x31A, size, &b43_b_ratetable[0]);
>  	b43_write_probe_resp_plcp(dev, 0x32C, size, &b43_b_ratetable[1]);
>  	b43_write_probe_resp_plcp(dev, 0x33E, size, &b43_b_ratetable[2]);
>  	b43_write_probe_resp_plcp(dev, 0x350, size, &b43_b_ratetable[3]);
> +#endif
> 
>  	size = min((size_t) size, 0x200 - sizeof(struct b43_plcp_hdr6));
>  	b43_write_template_common(dev, probe_resp_data,

Please submit this hunk as separate patch.

> @@ -3667,8 +3751,9 @@
> 
>  	switch (cmd) {
>  	case SET_KEY:
> -		if (algorithm == B43_SEC_ALGO_TKIP) {
> -			/* FIXME: No TKIP hardware encryption for now. */
> +		if (algorithm == B43_SEC_ALGO_TKIP &&
> +		    !(key->flags & IEEE80211_KEY_FLAG_PAIRWISE)) {
> +			/* We support only pairwise key */
>  			err = -EOPNOTSUPP;
>  			goto out_unlock;
>  		}
> @@ -3698,6 +3783,8 @@
>  				     b43_hf_read(dev) & ~B43_HF_USEDEFKEYS);
>  		}
>  		key->flags |= IEEE80211_KEY_FLAG_GENERATE_IV;
> +		if (algorithm == B43_SEC_ALGO_TKIP)
> +			key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
>  		break;
>  	case DISABLE_KEY: {
>  		err = b43_key_clear(dev, key->hw_key_idx);
> @@ -4425,6 +4512,7 @@
>  	.bss_info_changed	= b43_op_bss_info_changed,
>  	.configure_filter	= b43_op_configure_filter,
>  	.set_key		= b43_op_set_key,
> +	.update_tkip_key	= b43_mac_update_tkip_key,

Rename to
b43_op_update_tkip_key

>  	.get_stats		= b43_op_get_stats,
>  	.get_tx_stats		= b43_op_get_tx_stats,
>  	.get_tsf		= b43_op_get_tsf,

> @@ -257,9 +258,25 @@
>  		mac_ctl |= (key->algorithm << B43_TXH_MAC_KEYALG_SHIFT) &
>  			   B43_TXH_MAC_KEYALG;
>  		wlhdr_len = ieee80211_hdrlen(fctl);
> -		iv_len = min((size_t) info->control.hw_key->iv_len,
> -			     ARRAY_SIZE(txhdr->iv));
> -		memcpy(txhdr->iv, ((u8 *) wlhdr) + wlhdr_len, iv_len);
> +		if (key->algorithm == B43_SEC_ALGO_TKIP) {
> +			u16 phase1key[5];
> +			int i;
> +			/* we give the phase1key and iv16 here, the key is stored in
> +			 * shm. With that the hardware can do phase 2 and encryption.
> +			 */
> +			ieee80211_get_tkip_key(info->control.hw_key, skb_frag,
> IEEE80211_TKIP_P1_KEY, (u8*)phase1key);

Patch is linewrap damaged. Fix your mail agent.

> +			/* phase1key is in host endian */
> +			for (i = 0; i < 5; i++)
> +				phase1key[i] = cpu_to_le16(phase1key[i]);
> +
> +			memcpy(txhdr->iv, phase1key, 10);
> +			/* iv16 */
> +			memcpy(txhdr->iv+10, ((u8 *) wlhdr) + wlhdr_len, 3);
                                      ^^^^^
Coding style.


> +		} else {
> +			iv_len = min((size_t) info->control.hw_key->iv_len,
> +				     ARRAY_SIZE(txhdr->iv));
> +			memcpy(txhdr->iv, ((u8 *) wlhdr) + wlhdr_len, iv_len);
> +		}
>  	}
>  	if (b43_is_old_txhdr_format(dev)) {
>  		b43_generate_plcp_hdr((struct b43_plcp_hdr4 *)(&txhdr->old_format.plcp),


-- 
Greetings, Michael.

Re: [RESEND] [PATCHv3] b43 add harware tkip

[gregor kowski]

Update v3 : add a module parameter to enable hw tkip, Coding style
fix, locking fix
Update v2 : work with qos, implement dump key, fix an issue with setting
random value on tkip key clear.
PS : this depends on "b43 : remove old kidx API" and "remove wrong
probe_resp_plcp write"

This add hardware tkip for b43.

Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>

Index: linux-2.6/drivers/net/wireless/b43/dma.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/dma.c	2009-07-31
20:51:10.000000000 +0000
+++ linux-2.6/drivers/net/wireless/b43/dma.c	2009-07-31 20:54:06.000000000 +0000
@@ -1188,7 +1188,7 @@
 	header = &(ring->txhdr_cache[(slot / TX_SLOTS_PER_FRAME) * hdrsize]);
 	cookie = generate_cookie(ring, slot);
 	err = b43_generate_txhdr(ring->dev, header,
-				 skb->data, skb->len, info, cookie);
+				 skb, info, cookie);
 	if (unlikely(err)) {
 		ring->current_slot = old_top_slot;
 		ring->used_slots = old_used_slots;
Index: linux-2.6/drivers/net/wireless/b43/main.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/main.c	2009-07-31
20:51:10.000000000 +0000
+++ linux-2.6/drivers/net/wireless/b43/main.c	2009-07-31
20:56:10.000000000 +0000
@@ -80,6 +80,10 @@
 module_param_named(nohwcrypt, modparam_nohwcrypt, int, 0444);
 MODULE_PARM_DESC(nohwcrypt, "Disable hardware encryption.");

+static int modparam_hwtkip;
+module_param_named(hwtkip, modparam_hwtkip, int, 0444);
+MODULE_PARM_DESC(hwtkip, "Enable hardware tkip.");
+
 static int modparam_qos = 1;
 module_param_named(qos, modparam_qos, int, 0444);
 MODULE_PARM_DESC(qos, "Enable QOS support (default on)");
@@ -836,6 +840,76 @@
 	}
 }

+/* The ucode will use phase1 key with TEK key to decrypt rx packets.
+ * When a packet is received, the iv32 is checked.
+ * - if it doesn't the packet is returned without modification (and software
+ *   decryption can be done). That's what happen when iv16 wrap.
+ * - if it does, the rc4 key is computed, and decryption is tried.
+ *   Either it will success and B43_RX_MAC_DEC is returned,
+ *   either it fails and B43_RX_MAC_DEC|B43_RX_MAC_DECERR is returned
+ *   and the packet is not usable (it got modified by the ucode).
+ * So in order to never have B43_RX_MAC_DECERR, we should provide
+ * a iv32 and phase1key that match. Because we drop packets in case of
+ * B43_RX_MAC_DECERR, if we have a correct iv32 but a wrong phase1key, all
+ * packets will be lost without higher layer knowing (ie no resync possible
+ * until next wrap).
+ *
+ * NOTE : this should support 50 key like RCMTA because
+ * (B43_SHM_SH_KEYIDXBLOCK - B43_SHM_SH_TKIPTSCTTAK)/14 = 50
+ */
+static void rx_tkip_phase1_write(struct b43_wldev *dev, u8 index, u32 iv32,
+		u16 *phase1key)
+{
+	unsigned int i;
+	u32 offset;
+	const u8 per_sta_keys_start = 4;
+	if (!modparam_hwtkip)
+		return;
+
+	B43_WARN_ON(index < per_sta_keys_start);
+	/* We have four default TX/ RX keys.
+	 * Physical mac 0 is mapped to physical key 4.
+	 * So we must adjust the index here.
+	 */
+	index -= per_sta_keys_start;
+
+	if (b43_debug(dev, B43_DBG_KEYS)) {
+		b43dbg(dev->wl, "rx_tkip_phase1_write : idx 0x%x, iv32 0x%x\n",
+				index, iv32);
+	}
+	/* Write the key to the  RX tkip shared mem */
+	offset = B43_SHM_SH_TKIPTSCTTAK + index * (10 + 4);
+	for (i = 0; i < 10; i += 2) {
+		b43_shm_write16(dev, B43_SHM_SHARED, offset + i,
+				phase1key ? phase1key[i / 2] : 0);
+	}
+	b43_shm_write16(dev, B43_SHM_SHARED, offset + i, iv32);
+	b43_shm_write16(dev, B43_SHM_SHARED, offset + i + 2, iv32 >> 16);
+}
+
+static void b43_op_update_tkip_key(struct ieee80211_hw *hw,
+			struct ieee80211_key_conf *keyconf, const u8 *addr,
+			u32 iv32, u16 *phase1key)
+{
+	struct b43_wl *wl = hw_to_b43_wl(hw);
+	struct b43_wldev *dev;
+	int index = keyconf->hw_key_idx;
+
+	mutex_lock(&wl->mutex);
+
+	dev = wl->current_dev;
+	if (!dev || b43_status(dev) < B43_STAT_INITIALIZED)
+		goto out_unlock;
+
+	keymac_write(dev, index, NULL);	/* First zero out mac to avoid race */
+
+	rx_tkip_phase1_write(dev, index, iv32, phase1key);
+	keymac_write(dev, index, addr);
+
+out_unlock:
+	mutex_unlock(&wl->mutex);
+}
+
 static void do_key_write(struct b43_wldev *dev,
 			 u8 index, u8 algorithm,
 			 const u8 *key, size_t key_len, const u8 *mac_addr)
@@ -848,6 +922,19 @@

 	if (index >= per_sta_keys_start)
 		keymac_write(dev, index, NULL);	/* First zero out mac. */
+	if (algorithm == B43_SEC_ALGO_TKIP) {
+		/*
+		 * We should provide an initial iv32, phase1key pair.
+		 * We could start with iv32=0 and compute the corresponding
+		 * phase1key, but this means calling ieee80211_get_tkip_key
+		 * with a fake skb (or export other tkip function).
+		 * Because we are lazy we hope iv32 won't start with
+		 * 0xffffffff and let's b43_op_update_tkip_key provide a
+		 * correct pair.
+		 */
+		rx_tkip_phase1_write(dev, index, 0xffffffff, (u16*)buf);
+	} else if (index >= per_sta_keys_start) /* clear it */
+		rx_tkip_phase1_write(dev, index, 0, NULL);
 	if (key)
 		memcpy(buf, key, key_len);
 	key_write(dev, index, algorithm, buf);
@@ -865,6 +952,15 @@
 {
 	int i;

+	/* For ALG_TKIP the key is encoded as a 256-bit (32 byte) data block:
+	 * 	- Temporal Encryption Key (128 bits)
+	 * 	- Temporal Authenticator Tx MIC Key (64 bits)
+	 * 	- Temporal Authenticator Rx MIC Key (64 bits)
+	 *
+	 * 	Hardware only store TEK
+	 */
+	if (algorithm == B43_SEC_ALGO_TKIP && key_len == 32)
+		key_len = 16;
 	if (key_len > B43_SEC_KEYSIZE)
 		return -EINVAL;
 	for (i = 0; i < dev->max_nr_keys; i++) {
@@ -946,6 +1042,14 @@
 		printk("   Algo: %04X/%02X", algo, key->algorithm);

 		if (index >= 4) {
+			if (key->algorithm == B43_SEC_ALGO_TKIP) {
+				printk("   TKIP: ");
+				offset = B43_SHM_SH_TKIPTSCTTAK + (index - 4) * (10 + 4);
+				for (i = 0; i < 14; i += 2) {
+					u16 tmp = b43_shm_read16(dev, B43_SHM_SHARED, offset + i);
+					printk("%02X%02X", (tmp & 0xFF), ((tmp >> 8) & 0xFF));
+				}
+			}
 			rcmta0 = b43_shm_read32(dev, B43_SHM_RCMTA,
 						((index - 4) * 2) + 0);
 			rcmta1 = b43_shm_read16(dev, B43_SHM_RCMTA,
@@ -3670,8 +3774,10 @@

 	switch (cmd) {
 	case SET_KEY:
-		if (algorithm == B43_SEC_ALGO_TKIP) {
-			/* FIXME: No TKIP hardware encryption for now. */
+		if (algorithm == B43_SEC_ALGO_TKIP &&
+		    (!(key->flags & IEEE80211_KEY_FLAG_PAIRWISE) ||
+		    !modparam_hwtkip)) {
+			/* We support only pairwise key */
 			err = -EOPNOTSUPP;
 			goto out_unlock;
 		}
@@ -3701,6 +3807,8 @@
 				     b43_hf_read(dev) & ~B43_HF_USEDEFKEYS);
 		}
 		key->flags |= IEEE80211_KEY_FLAG_GENERATE_IV;
+		if (algorithm == B43_SEC_ALGO_TKIP)
+			key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
 		break;
 	case DISABLE_KEY: {
 		err = b43_key_clear(dev, key->hw_key_idx);
@@ -4428,6 +4536,7 @@
 	.bss_info_changed	= b43_op_bss_info_changed,
 	.configure_filter	= b43_op_configure_filter,
 	.set_key		= b43_op_set_key,
+	.update_tkip_key	= b43_op_update_tkip_key,
 	.get_stats		= b43_op_get_stats,
 	.get_tx_stats		= b43_op_get_tx_stats,
 	.get_tsf		= b43_op_get_tsf,
Index: linux-2.6/drivers/net/wireless/b43/pio.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/pio.c	2009-07-31
20:51:10.000000000 +0000
+++ linux-2.6/drivers/net/wireless/b43/pio.c	2009-07-31 20:54:06.000000000 +0000
@@ -461,8 +461,8 @@

 	cookie = generate_cookie(q, pack);
 	hdrlen = b43_txhdr_size(q->dev);
-	err = b43_generate_txhdr(q->dev, (u8 *)&txhdr, skb->data,
-				 skb->len, info, cookie);
+	err = b43_generate_txhdr(q->dev, (u8 *)&txhdr, skb,
+				 info, cookie);
 	if (err)
 		return err;

Index: linux-2.6/drivers/net/wireless/b43/xmit.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/xmit.c	2009-07-31
20:51:10.000000000 +0000
+++ linux-2.6/drivers/net/wireless/b43/xmit.c	2009-07-31
20:54:06.000000000 +0000
@@ -180,11 +180,12 @@
 /* Generate a TX data header. */
 int b43_generate_txhdr(struct b43_wldev *dev,
 		       u8 *_txhdr,
-		       const unsigned char *fragment_data,
-		       unsigned int fragment_len,
+		       struct sk_buff *skb_frag,
 		       struct ieee80211_tx_info *info,
 		       u16 cookie)
 {
+	const unsigned char *fragment_data = skb_frag->data;
+	unsigned int fragment_len = skb_frag->len;
 	struct b43_txhdr *txhdr = (struct b43_txhdr *)_txhdr;
 	const struct b43_phy *phy = &dev->phy;
 	const struct ieee80211_hdr *wlhdr =
@@ -257,9 +258,26 @@
 		mac_ctl |= (key->algorithm << B43_TXH_MAC_KEYALG_SHIFT) &
 			   B43_TXH_MAC_KEYALG;
 		wlhdr_len = ieee80211_hdrlen(fctl);
-		iv_len = min((size_t) info->control.hw_key->iv_len,
-			     ARRAY_SIZE(txhdr->iv));
-		memcpy(txhdr->iv, ((u8 *) wlhdr) + wlhdr_len, iv_len);
+		if (key->algorithm == B43_SEC_ALGO_TKIP) {
+			u16 phase1key[5];
+			int i;
+			/* we give the phase1key and iv16 here, the key is stored in
+			 * shm. With that the hardware can do phase 2 and encryption.
+			 */
+			ieee80211_get_tkip_key(info->control.hw_key, skb_frag,
+					IEEE80211_TKIP_P1_KEY, (u8*)phase1key);
+			/* phase1key is in host endian */
+			for (i = 0; i < 5; i++)
+				phase1key[i] = cpu_to_le16(phase1key[i]);
+
+			memcpy(txhdr->iv, phase1key, 10);
+			/* iv16 */
+			memcpy(txhdr->iv + 10, ((u8 *) wlhdr) + wlhdr_len, 3);
+		} else {
+			iv_len = min((size_t) info->control.hw_key->iv_len,
+				     ARRAY_SIZE(txhdr->iv));
+			memcpy(txhdr->iv, ((u8 *) wlhdr) + wlhdr_len, iv_len);
+		}
 	}
 	if (b43_is_old_txhdr_format(dev)) {
 		b43_generate_plcp_hdr((struct b43_plcp_hdr4 *)(&txhdr->old_format.plcp),
Index: linux-2.6/drivers/net/wireless/b43/xmit.h
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/xmit.h	2009-07-31
20:51:10.000000000 +0000
+++ linux-2.6/drivers/net/wireless/b43/xmit.h	2009-07-31
20:54:06.000000000 +0000
@@ -176,8 +176,7 @@

 int b43_generate_txhdr(struct b43_wldev *dev,
 		       u8 * txhdr,
-		       const unsigned char *fragment_data,
-		       unsigned int fragment_len,
+		       struct sk_buff *skb_frag,
 		       struct ieee80211_tx_info *txctl, u16 cookie);

 /* Transmit Status */

Re: [RESEND] [PATCHv2] b43 add harware tkip

[gregor kowski]

Hi Michael,

On 7/28/09, Michael Buesch <mb@bu3sch.de> wrote:
>> This add hardware tkip for b43.
>
> First of all, I don't really want to support hw tkip on b43.
> Broadcom removed the support for hw tkip from their drivers. I bet they
> had a very good reason to do so.
Did they remove it from the firmware or only the driver ?
May be they remove it to support advanced stuff like multiple SSID ?
Which version of the driver it is ?

> It's mapped to 4, because you removed the old API support.
> (I'm not sure whether we want to remove that support, yet. Gimme some time
> on it...)
>
Does the old API still useful ?
Are there v4 firmware supporting old API ?


Regards,

Gregor

Re: [RESEND] [PATCHv2] b43 add harware tkip

[Michael Buesch]

On Friday 31 July 2009 23:04:17 gregor kowski wrote:
> Hi Michael,
> 
> On 7/28/09, Michael Buesch <mb@bu3sch.de> wrote:
> >> This add hardware tkip for b43.
> >
> > First of all, I don't really want to support hw tkip on b43.
> > Broadcom removed the support for hw tkip from their drivers. I bet they
> > had a very good reason to do so.
> Did they remove it from the firmware or only the driver ?

I'm not sure about the firmware. But they removed it from the driver.

> > It's mapped to 4, because you removed the old API support.
> > (I'm not sure whether we want to remove that support, yet. Gimme some time
> > on it...)
> >
> Does the old API still useful ?

Well, I'm not sure. Why do we remove it at all? It just avoids one single
if condition in your tkip patch, doesn't it?

> Are there v4 firmware supporting old API ?

Yes

-- 
Greetings, Michael.

Re: [RESEND] [PATCHv3] b43 add harware tkip

[Michael Buesch]

On Friday 31 July 2009 23:00:01 gregor kowski wrote:
> Update v3 : add a module parameter to enable hw tkip, Coding style
> fix, locking fix
> Update v2 : work with qos, implement dump key, fix an issue with setting
> random value on tkip key clear.
> PS : this depends on "b43 : remove old kidx API" and "remove wrong
> probe_resp_plcp write"
> 
> This add hardware tkip for b43.
> 
> Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>
> 
> Index: linux-2.6/drivers/net/wireless/b43/dma.c
> ===================================================================
> --- linux-2.6.orig/drivers/net/wireless/b43/dma.c	2009-07-31
> 20:51:10.000000000 +0000
> +++ linux-2.6/drivers/net/wireless/b43/dma.c	2009-07-31 20:54:06.000000000 +0000
> @@ -1188,7 +1188,7 @@
>  	header = &(ring->txhdr_cache[(slot / TX_SLOTS_PER_FRAME) * hdrsize]);
>  	cookie = generate_cookie(ring, slot);
>  	err = b43_generate_txhdr(ring->dev, header,
> -				 skb->data, skb->len, info, cookie);
> +				 skb, info, cookie);
>  	if (unlikely(err)) {
>  		ring->current_slot = old_top_slot;
>  		ring->used_slots = old_used_slots;
> Index: linux-2.6/drivers/net/wireless/b43/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/net/wireless/b43/main.c	2009-07-31
> 20:51:10.000000000 +0000
> +++ linux-2.6/drivers/net/wireless/b43/main.c	2009-07-31
> 20:56:10.000000000 +0000
> @@ -80,6 +80,10 @@
>  module_param_named(nohwcrypt, modparam_nohwcrypt, int, 0444);
>  MODULE_PARM_DESC(nohwcrypt, "Disable hardware encryption.");
> 
> +static int modparam_hwtkip;
> +module_param_named(hwtkip, modparam_hwtkip, int, 0444);
> +MODULE_PARM_DESC(hwtkip, "Enable hardware tkip.");
> +
>  static int modparam_qos = 1;
>  module_param_named(qos, modparam_qos, int, 0444);
>  MODULE_PARM_DESC(qos, "Enable QOS support (default on)");
> @@ -836,6 +840,76 @@
>  	}
>  }
> 
> +/* The ucode will use phase1 key with TEK key to decrypt rx packets.
> + * When a packet is received, the iv32 is checked.
> + * - if it doesn't the packet is returned without modification (and software
> + *   decryption can be done). That's what happen when iv16 wrap.
> + * - if it does, the rc4 key is computed, and decryption is tried.
> + *   Either it will success and B43_RX_MAC_DEC is returned,
> + *   either it fails and B43_RX_MAC_DEC|B43_RX_MAC_DECERR is returned
> + *   and the packet is not usable (it got modified by the ucode).
> + * So in order to never have B43_RX_MAC_DECERR, we should provide
> + * a iv32 and phase1key that match. Because we drop packets in case of
> + * B43_RX_MAC_DECERR, if we have a correct iv32 but a wrong phase1key, all
> + * packets will be lost without higher layer knowing (ie no resync possible
> + * until next wrap).
> + *
> + * NOTE : this should support 50 key like RCMTA because
> + * (B43_SHM_SH_KEYIDXBLOCK - B43_SHM_SH_TKIPTSCTTAK)/14 = 50
> + */
> +static void rx_tkip_phase1_write(struct b43_wldev *dev, u8 index, u32 iv32,
> +		u16 *phase1key)
> +{
> +	unsigned int i;
> +	u32 offset;
> +	const u8 per_sta_keys_start = 4;

Can we add an empty line here before the code begins?

> +	if (!modparam_hwtkip)
> +		return;
> +
> +	B43_WARN_ON(index < per_sta_keys_start);
> +	/* We have four default TX/ RX keys.
> +	 * Physical mac 0 is mapped to physical key 4.
> +	 * So we must adjust the index here.
> +	 */
> +	index -= per_sta_keys_start;
> +
> +	if (b43_debug(dev, B43_DBG_KEYS)) {
> +		b43dbg(dev->wl, "rx_tkip_phase1_write : idx 0x%x, iv32 0x%x\n",
> +				index, iv32);
> +	}
> +	/* Write the key to the  RX tkip shared mem */
> +	offset = B43_SHM_SH_TKIPTSCTTAK + index * (10 + 4);
> +	for (i = 0; i < 10; i += 2) {
> +		b43_shm_write16(dev, B43_SHM_SHARED, offset + i,
> +				phase1key ? phase1key[i / 2] : 0);
> +	}
> +	b43_shm_write16(dev, B43_SHM_SHARED, offset + i, iv32);
> +	b43_shm_write16(dev, B43_SHM_SHARED, offset + i + 2, iv32 >> 16);
> +}
> +
> +static void b43_op_update_tkip_key(struct ieee80211_hw *hw,
> +			struct ieee80211_key_conf *keyconf, const u8 *addr,
> +			u32 iv32, u16 *phase1key)
> +{
> +	struct b43_wl *wl = hw_to_b43_wl(hw);
> +	struct b43_wldev *dev;
> +	int index = keyconf->hw_key_idx;
> +

I'd add

	if (B43_WARN_ON(!modparam_hwtkip))
		return;

here, because it's a cheap way to detect possible mac80211 bugs.
Mac80211 should not ever call this function, if we reject all tkip keys. But if
it does (due to a bug), this cheap check will prevent keymac trashing and keep the
device working.

> +	mutex_lock(&wl->mutex);
> +
> +	dev = wl->current_dev;
> +	if (!dev || b43_status(dev) < B43_STAT_INITIALIZED)
> +		goto out_unlock;
> +
> +	keymac_write(dev, index, NULL);	/* First zero out mac to avoid race */
> +
> +	rx_tkip_phase1_write(dev, index, iv32, phase1key);
> +	keymac_write(dev, index, addr);
> +
> +out_unlock:
> +	mutex_unlock(&wl->mutex);
> +}
> +


The rest of the patch looks very good.


-- 
Greetings, Michael.

Re: [RESEND] [PATCHv2] b43 add harware tkip

[gregor kowski]

On 7/31/09, Michael Buesch <mb@bu3sch.de> wrote:
> On Friday 31 July 2009 23:04:17 gregor kowski wrote:
>> Hi Michael,
>>
>> > It's mapped to 4, because you removed the old API support.
>> > (I'm not sure whether we want to remove that support, yet. Gimme some
>> > time
>> > on it...)
>> >
>> Does the old API still useful ?
>
> Well, I'm not sure. Why do we remove it at all? It just avoids one single
> if condition in your tkip patch, doesn't it?
>
because you ask it : "We should probably remove that new_kidx_api crap
alltogether." [1] :)
There is really bugs in the current code. You could see it by enabling
B43_DBG_KEYS.
With the current code and new API, you will see that 54 pairwise keys
are dumped, not only 50.


[1]
> >> +     /* FIXME : for b43_new_kidx_api, there can be 54 key
> >> +      * instead of 50 in RCMTA and TKIPTSCTTAK.
> >> +      */
> >
> > I don't understand this comment.
> if  b43_new_kidx_api is true :
> - we set max_nr_keys to 58
> - we program B43_MMIO_RCMTA_COUNT to 50
> - in b43_key_write we can allocate key up to index 58 (4 for default,
> 54 for sta)
>
> But there is only 50 entries for TKIPTSCTTAK, and a comment on bcm-v4
> suggest there is 50 entries for RCMTA. So if there more than 50
> station we will overflow RCMTA and TKIPTSCTTAK.

Yeah well. The key handling is pretty weird.
There are 50 pairwise keys available. max_nr_keys includes pairwise
keys, group keys (4)
and another copy of the group keys (4) used with older firmware. So we
end up at 58.

To summarize it, in practice we have 50 pairwise keys and 4 group
keys. You can ignore
the additional 4 "rx keys" as they are not used in recent fw and are
just a copy of the
group keys.

> So the fix will be do to something like
> dev->max_nr_keys = (dev->dev->id.revision >= 5) ? 58 : 20;
> if (b43_new_kidx_api())
> dev->max_nr_keys -= 4;

Hm, I don't think this is correct. There are lots of weird assumptions
all over the code.
We should probably remove that new_kidx_api crap alltogether.

Re: [RESEND] [PATCHv2] b43 add harware tkip

[Michael Buesch]

On Tuesday 04 August 2009 23:54:42 gregor kowski wrote:
> On 7/31/09, Michael Buesch <mb@bu3sch.de> wrote:
> > On Friday 31 July 2009 23:04:17 gregor kowski wrote:
> >> Hi Michael,
> >>
> >> > It's mapped to 4, because you removed the old API support.
> >> > (I'm not sure whether we want to remove that support, yet. Gimme some
> >> > time
> >> > on it...)
> >> >
> >> Does the old API still useful ?
> >
> > Well, I'm not sure. Why do we remove it at all? It just avoids one single
> > if condition in your tkip patch, doesn't it?
> >
> because you ask it : "We should probably remove that new_kidx_api crap
> alltogether." [1] :)

<quote> probably </quote>

> There is really bugs in the current code.

You always talk about "bugs". What are these "bugs"? Is it just the wrong
max_nr_keys assignment? That's trivial to fix.


-- 
Greetings, Michael.

Re: [RESEND] [PATCHv2] b43 add harware tkip

[gregor kowski]

On 8/4/09, Michael Buesch <mb@bu3sch.de> wrote:
> On Tuesday 04 August 2009 23:54:42 gregor kowski wrote:
>> On 7/31/09, Michael Buesch <mb@bu3sch.de> wrote:
>> > On Friday 31 July 2009 23:04:17 gregor kowski wrote:
>> >> Hi Michael,
>
>> There is really bugs in the current code.
>
> You always talk about "bugs". What are these "bugs"? Is it just the wrong
> max_nr_keys assignment? That's trivial to fix.
>
So [1] is ok ?

Thanks,

Gregor

[1]
> So the fix will be do to something like
> dev->max_nr_keys = (dev->dev->id.revision >= 5) ? 58 : 20;
> if (b43_new_kidx_api())
> dev->max_nr_keys -= 4;

Re: [RESEND] [PATCHv2] b43 add harware tkip

[Michael Buesch]

On Wednesday 05 August 2009 00:03:11 gregor kowski wrote:
> On 8/4/09, Michael Buesch <mb@bu3sch.de> wrote:
> > On Tuesday 04 August 2009 23:54:42 gregor kowski wrote:
> >> On 7/31/09, Michael Buesch <mb@bu3sch.de> wrote:
> >> > On Friday 31 July 2009 23:04:17 gregor kowski wrote:
> >> >> Hi Michael,
> >
> >> There is really bugs in the current code.
> >
> > You always talk about "bugs". What are these "bugs"? Is it just the wrong
> > max_nr_keys assignment? That's trivial to fix.
> >
> So [1] is ok ?

Could you answer my question?



-- 
Greetings, Michael.

Re: [RESEND] [PATCHv2] b43 add harware tkip

[gregor kowski]

On 8/5/09, Michael Buesch <mb@bu3sch.de> wrote:
> On Wednesday 05 August 2009 00:03:11 gregor kowski wrote:
>> On 8/4/09, Michael Buesch <mb@bu3sch.de> wrote:
>> > On Tuesday 04 August 2009 23:54:42 gregor kowski wrote:
>> >
>> > You always talk about "bugs". What are these "bugs"? Is it just the
>> > wrong
>> > max_nr_keys assignment? That's trivial to fix.
>> >
>> So [1] is ok ?
>
> Could you answer my question?
That's [1]. But may be my description wasn't good.
I will try a new one.
we can have up to 50 pairwise keys (due to RCMTA and tkip stuff).

In the case of new API :
- max_nr_keys is set to 58
- in b43_key_clear we call do_key_write for index in [0 ... 58]
- in do_key_write we call keymac_write for index in [4 ... 58]
- in keymac_write write to RCMTA index [0 ... 54]
We write too much pairwise keys.

- max_nr_keys is set to 58
- b43_key_write generate pairwise keys in [sta_keys_start ...
max_nr_keys] = [0 ... 58] and call do_key_write
- in do_key_write we call keymac_write for index in [4 ... 58]
- in keymac_write write to RCMTA index [0 ... 54]
We write too much pairwise keys.

So max_nr_keys seems wrong in case of new API.

Gregor.
[1]
> if  b43_new_kidx_api is true :
> - we set max_nr_keys to 58
> - we program B43_MMIO_RCMTA_COUNT to 50
> - in b43_key_write we can allocate key up to index 58 (4 for default,
> 54 for sta)
>
> But there is only 50 entries for TKIPTSCTTAK, and a comment on bcm-v4
> suggest there is 50 entries for RCMTA. So if there more than 50
> station we will overflow RCMTA and TKIPTSCTTAK.

Re: [RESEND] [PATCHv2] b43 add harware tkip

[Michael Buesch]

On Wednesday 05 August 2009 00:23:23 gregor kowski wrote:
> On 8/5/09, Michael Buesch <mb@bu3sch.de> wrote:
> > On Wednesday 05 August 2009 00:03:11 gregor kowski wrote:
> >> On 8/4/09, Michael Buesch <mb@bu3sch.de> wrote:
> >> > On Tuesday 04 August 2009 23:54:42 gregor kowski wrote:
> >> >
> >> > You always talk about "bugs". What are these "bugs"? Is it just the
> >> > wrong
> >> > max_nr_keys assignment? That's trivial to fix.
> >> >
> >> So [1] is ok ?
> >
> > Could you answer my question?
> That's [1]. But may be my description wasn't good.
> I will try a new one.
> we can have up to 50 pairwise keys (due to RCMTA and tkip stuff).
> 
> In the case of new API :
> - max_nr_keys is set to 58
> - in b43_key_clear we call do_key_write for index in [0 ... 58]
> - in do_key_write we call keymac_write for index in [4 ... 58]
> - in keymac_write write to RCMTA index [0 ... 54]
> We write too much pairwise keys.
> 
> - max_nr_keys is set to 58
> - b43_key_write generate pairwise keys in [sta_keys_start ...
> max_nr_keys] = [0 ... 58] and call do_key_write
> - in do_key_write we call keymac_write for index in [4 ... 58]
> - in keymac_write write to RCMTA index [0 ... 54]
> We write too much pairwise keys.

Yeah, I do understand this bug. My question was if that is the only bug.

> So max_nr_keys seems wrong in case of new API.

It's not that simple, actually.
The max_nr_keys thing was never meant to specify which API we're on.
It was invented to do the RCMTA vs *oldcrappymechanismiforgotthenameof*.
max_nr_keys essentially became obsolete and dead code when b43 did not
support <rev5 anymore. I will submit a patch which removes it and cleans up
the code alltogether.

-- 
Greetings, Michael.

Re: [RESEND] [PATCHv2] b43 add harware tkip

[gregor kowski]

>
> Yeah, I do understand this bug. My question was if that is the only bug.
>
Sorry, yes this is the only one.

>> So max_nr_keys seems wrong in case of new API.
>
> It's not that simple, actually.
> The max_nr_keys thing was never meant to specify which API we're on.
> It was invented to do the RCMTA vs *oldcrappymechanismiforgotthenameof*.
> max_nr_keys essentially became obsolete and dead code when b43 did not
> support <rev5 anymore. I will submit a patch which removes it and cleans up
> the code alltogether.
Ok, thanks