2008-11-19 23:31:56

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 0/7] iwlwifi driver updates

This series contains mostly fixes for iwlwifi. Patch 7 is a fix to a bug
that is tracked as a regression from 2.6.27
(http://bugzilla.kernel.org/show_bug.cgi?id=12040 ). The patch does apply
to Linus's tree also - could you please push this patch there?

[PATCH 1/7] iwl3945 : Fix ad-hoc mode for 3945
[PATCH 2/7] iwlwifi: move iwl_clear_stations_table to iwl-sta.c
[PATCH 3/7] iwlwifi: enable base band calibration in 5000 HW
[PATCH 4/7] iwlwifi: 4965 define firmware file name once
[PATCH 5/7] iwlwifi: TX setup fix confusion between TX queue and TX DMA channel
[PATCH 6/7] iwlwifi: TX update chicken bits
[PATCH 7/7] iwlwifi: prevent double key removal

Thank you

Reinette


2008-11-19 23:31:57

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 7/7] iwlwifi: prevent double key removal

From: Zhu Yi <[email protected]>

Do not remove the same key twice.

This patch also fixes a memory corruption problem reported in
http://marc.info/?l=linux-wireless&m=122641417231586&w=2 and tracked in
http://bugzilla.kernel.org/show_bug.cgi?id=12040.

When the key is removed a second time the offset is set to 255 - this index
is not valid for the ucode_key_table and corrupts the eeprom pointer (which
is 255 bits from ucode_key_table).

Signed-off-by: Zhu Yi <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Tested-by: Carlos R. Mafra <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
John, Could you please push this patch up to 2.6.28 also?

drivers/net/wireless/iwlwifi/iwl-sta.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-sta.c b/drivers/net/wireless/iwlwifi/iwl-sta.c
index 0222ef8..27f7108 100644
--- a/drivers/net/wireless/iwlwifi/iwl-sta.c
+++ b/drivers/net/wireless/iwlwifi/iwl-sta.c
@@ -809,6 +809,12 @@ int iwl_remove_dynamic_key(struct iwl_priv *priv,
return 0;
}

+ if (WARN(priv->stations[sta_id].sta.key.key_offset == WEP_INVALID_OFFSET,
+ "Removing wrong key %d 0x%x\n", keyconf->keyidx, key_flags)) {
+ spin_unlock_irqrestore(&priv->sta_lock, flags);
+ return 0;
+ }
+
if (!test_and_clear_bit(priv->stations[sta_id].sta.key.key_offset,
&priv->ucode_key_table))
IWL_ERROR("index %d not used in uCode key table.\n",
--
1.5.4.3


2008-11-19 23:31:57

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 1/7] iwl3945 : Fix ad-hoc mode for 3945

From: Abhijeet Kolekar <[email protected]>

Patch fixes the ad-hoc mode by
1) Removing redundant clear_stations_table which prevented generation of
beacons.
2) Setting assoc_id to 1. It was never set so preventing tx flow
in iwl3945_tx_skb.

Signed-off-by: Abhijeet Kolekar <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl3945-base.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index c465bb4..104a0b7 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -6333,10 +6333,7 @@ static void iwl3945_post_associate(struct iwl3945_priv *priv)

case NL80211_IFTYPE_ADHOC:

- /* clear out the station table */
- iwl3945_clear_stations_table(priv);
-
- iwl3945_add_station(priv, iwl3945_broadcast_addr, 0, 0);
+ priv->assoc_id = 1;
iwl3945_add_station(priv, priv->bssid, 0, 0);
iwl3945_sync_sta(priv, IWL_STA_ID,
(priv->band == IEEE80211_BAND_5GHZ) ?
--
1.5.4.3


2008-11-19 23:31:57

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 5/7] iwlwifi: TX setup fix confusion between TX queue and TX DMA channel

From: Winkler, Tomas <[email protected]>

This patch configures correctly TX DMA channel. It is not
the same as TX queue.

Signed-off-by: Tomas Winkler <[email protected]>
Acked-by: Zhu Yi <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-4965.c | 10 ++++++++--
drivers/net/wireless/iwlwifi/iwl-5000.c | 9 ++++++++-
drivers/net/wireless/iwlwifi/iwl-tx.c | 5 -----
3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index b96f553..60769b1 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -692,9 +692,9 @@ static const u16 default_queue_to_tx_fifo[] = {
static int iwl4965_alive_notify(struct iwl_priv *priv)
{
u32 a;
- int i = 0;
unsigned long flags;
int ret;
+ int i, chan;

spin_lock_irqsave(&priv->lock, flags);

@@ -718,6 +718,12 @@ static int iwl4965_alive_notify(struct iwl_priv *priv)
iwl_write_prph(priv, IWL49_SCD_DRAM_BASE_ADDR,
priv->scd_bc_tbls.dma >> 10);

+ /* Enable DMA channel */
+ for (chan = 0; chan < FH49_TCSR_CHNL_NUM ; chan++)
+ iwl_write_direct32(priv, FH_TCSR_CHNL_TX_CONFIG_REG(chan),
+ FH_TCSR_TX_CONFIG_REG_VAL_DMA_CHNL_ENABLE |
+ FH_TCSR_TX_CONFIG_REG_VAL_DMA_CREDIT_ENABLE);
+
/* Disable chain mode for all queues */
iwl_write_prph(priv, IWL49_SCD_QUEUECHAIN_SEL, 0);

@@ -748,7 +754,7 @@ static int iwl4965_alive_notify(struct iwl_priv *priv)
(1 << priv->hw_params.max_txq_num) - 1);

/* Activate all Tx DMA/FIFO channels */
- priv->cfg->ops->lib->txq_set_sched(priv, IWL_MASK(0, 7));
+ priv->cfg->ops->lib->txq_set_sched(priv, IWL_MASK(0, 6));

iwl4965_set_wr_ptrs(priv, IWL_CMD_QUEUE_NUM, 0);

diff --git a/drivers/net/wireless/iwlwifi/iwl-5000.c b/drivers/net/wireless/iwlwifi/iwl-5000.c
index 478c5c3..d73760c 100644
--- a/drivers/net/wireless/iwlwifi/iwl-5000.c
+++ b/drivers/net/wireless/iwlwifi/iwl-5000.c
@@ -700,9 +700,9 @@ static int iwl5000_send_wimax_coex(struct iwl_priv *priv)
static int iwl5000_alive_notify(struct iwl_priv *priv)
{
u32 a;
- int i = 0;
unsigned long flags;
int ret;
+ int i, chan;

spin_lock_irqsave(&priv->lock, flags);

@@ -725,6 +725,13 @@ static int iwl5000_alive_notify(struct iwl_priv *priv)

iwl_write_prph(priv, IWL50_SCD_DRAM_BASE_ADDR,
priv->scd_bc_tbls.dma >> 10);
+
+ /* Enable DMA channel */
+ for (chan = 0; chan < FH50_TCSR_CHNL_NUM ; chan++)
+ iwl_write_direct32(priv, FH_TCSR_CHNL_TX_CONFIG_REG(chan),
+ FH_TCSR_TX_CONFIG_REG_VAL_DMA_CHNL_ENABLE |
+ FH_TCSR_TX_CONFIG_REG_VAL_DMA_CREDIT_ENABLE);
+
iwl_write_prph(priv, IWL50_SCD_QUEUECHAIN_SEL,
IWL50_SCD_QUEUECHAIN_SEL_ALL(priv->hw_params.max_txq_num));
iwl_write_prph(priv, IWL50_SCD_AGGR_SEL, 0);
diff --git a/drivers/net/wireless/iwlwifi/iwl-tx.c b/drivers/net/wireless/iwlwifi/iwl-tx.c
index 6db11af..9e638c2 100644
--- a/drivers/net/wireless/iwlwifi/iwl-tx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-tx.c
@@ -449,11 +449,6 @@ static int iwl_hw_tx_queue_init(struct iwl_priv *priv,
iwl_write_direct32(priv, FH_MEM_CBBC_QUEUE(txq_id),
txq->q.dma_addr >> 8);

- /* Enable DMA channel, using same id as for TFD queue */
- iwl_write_direct32(priv, FH_TCSR_CHNL_TX_CONFIG_REG(txq_id),
- FH_TCSR_TX_CONFIG_REG_VAL_DMA_CHNL_ENABLE |
- FH_TCSR_TX_CONFIG_REG_VAL_DMA_CREDIT_ENABLE);
-
iwl_release_nic_access(priv);
spin_unlock_irqrestore(&priv->lock, flags);

--
1.5.4.3


2008-11-19 23:31:57

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 2/7] iwlwifi: move iwl_clear_stations_table to iwl-sta.c

From: Winkler, Tomas <[email protected]>

This patch moves iwl_clear_stations_table into iwl-sta.c

Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-core.c | 23 +----------------------
drivers/net/wireless/iwlwifi/iwl-core.h | 1 -
drivers/net/wireless/iwlwifi/iwl-sta.c | 23 +++++++++++++++++++++++
drivers/net/wireless/iwlwifi/iwl-sta.h | 1 +
4 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index 8bd4d08..9a2e964 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -37,6 +37,7 @@
#include "iwl-io.h"
#include "iwl-rfkill.h"
#include "iwl-power.h"
+#include "iwl-sta.h"


MODULE_DESCRIPTION("iwl core");
@@ -237,28 +238,6 @@ int iwl_hw_nic_init(struct iwl_priv *priv)
}
EXPORT_SYMBOL(iwl_hw_nic_init);

-/**
- * iwl_clear_stations_table - Clear the driver's station table
- *
- * NOTE: This does not clear or otherwise alter the device's station table.
- */
-void iwl_clear_stations_table(struct iwl_priv *priv)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&priv->sta_lock, flags);
-
- if (iwl_is_alive(priv) &&
- !test_bit(STATUS_EXIT_PENDING, &priv->status) &&
- iwl_send_cmd_pdu_async(priv, REPLY_REMOVE_ALL_STA, 0, NULL, NULL))
- IWL_ERROR("Couldn't clear the station table\n");
-
- priv->num_stations = 0;
- memset(priv->stations, 0, sizeof(priv->stations));
-
- spin_unlock_irqrestore(&priv->sta_lock, flags);
-}
-EXPORT_SYMBOL(iwl_clear_stations_table);

void iwl_reset_qos(struct iwl_priv *priv)
{
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.h b/drivers/net/wireless/iwlwifi/iwl-core.h
index 5d4e2e2..82bf263 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.h
+++ b/drivers/net/wireless/iwlwifi/iwl-core.h
@@ -182,7 +182,6 @@ struct iwl_cfg {
struct ieee80211_hw *iwl_alloc_all(struct iwl_cfg *cfg,
struct ieee80211_ops *hw_ops);
void iwl_hw_detect(struct iwl_priv *priv);
-void iwl_clear_stations_table(struct iwl_priv *priv);
void iwl_reset_qos(struct iwl_priv *priv);
void iwl_set_rxon_chain(struct iwl_priv *priv);
int iwl_set_rxon_channel(struct iwl_priv *priv, struct ieee80211_channel *ch);
diff --git a/drivers/net/wireless/iwlwifi/iwl-sta.c b/drivers/net/wireless/iwlwifi/iwl-sta.c
index 2529c23..0222ef8 100644
--- a/drivers/net/wireless/iwlwifi/iwl-sta.c
+++ b/drivers/net/wireless/iwlwifi/iwl-sta.c
@@ -470,6 +470,29 @@ out:
}
EXPORT_SYMBOL(iwl_remove_station);

+/**
+ * iwl_clear_stations_table - Clear the driver's station table
+ *
+ * NOTE: This does not clear or otherwise alter the device's station table.
+ */
+void iwl_clear_stations_table(struct iwl_priv *priv)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->sta_lock, flags);
+
+ if (iwl_is_alive(priv) &&
+ !test_bit(STATUS_EXIT_PENDING, &priv->status) &&
+ iwl_send_cmd_pdu_async(priv, REPLY_REMOVE_ALL_STA, 0, NULL, NULL))
+ IWL_ERROR("Couldn't clear the station table\n");
+
+ priv->num_stations = 0;
+ memset(priv->stations, 0, sizeof(priv->stations));
+
+ spin_unlock_irqrestore(&priv->sta_lock, flags);
+}
+EXPORT_SYMBOL(iwl_clear_stations_table);
+
static int iwl_get_free_ucode_key_index(struct iwl_priv *priv)
{
int i;
diff --git a/drivers/net/wireless/iwlwifi/iwl-sta.h b/drivers/net/wireless/iwlwifi/iwl-sta.h
index 61eede5..7b98ea4 100644
--- a/drivers/net/wireless/iwlwifi/iwl-sta.h
+++ b/drivers/net/wireless/iwlwifi/iwl-sta.h
@@ -53,6 +53,7 @@ void iwl_update_tkip_key(struct iwl_priv *priv,

int iwl_rxon_add_station(struct iwl_priv *priv, const u8 *addr, int is_ap);
int iwl_remove_station(struct iwl_priv *priv, const u8 *addr, int is_ap);
+void iwl_clear_stations_table(struct iwl_priv *priv);
int iwl_get_sta_id(struct iwl_priv *priv, struct ieee80211_hdr *hdr);
int iwl_get_ra_sta_id(struct iwl_priv *priv, struct ieee80211_hdr *hdr);
u8 iwl_add_station_flags(struct iwl_priv *priv, const u8 *addr,
--
1.5.4.3


2008-11-19 23:31:56

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 4/7] iwlwifi: 4965 define firmware file name once

From: Winkler, Tomas <[email protected]>

Apply same idiom as in 5000 introduced by
'iwlwifi: define firmware file name once'

Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-4965.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index 9007ff2..b96f553 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -53,6 +53,7 @@ static int iwl4965_hw_get_temperature(const struct iwl_priv *priv);
* is not compatible with earlier drivers.
* This number will also appear in << 8 position of 1st dword of uCode file */
#define IWL4965_UCODE_API "-2"
+#define IWL4965_MODULE_FIRMWARE "iwlwifi-4965" IWL4965_UCODE_API ".ucode"


/* module parameters */
@@ -2322,7 +2323,7 @@ static struct iwl_ops iwl4965_ops = {

struct iwl_cfg iwl4965_agn_cfg = {
.name = "4965AGN",
- .fw_name = "iwlwifi-4965" IWL4965_UCODE_API ".ucode",
+ .fw_name = IWL4965_MODULE_FIRMWARE,
.sku = IWL_SKU_A|IWL_SKU_G|IWL_SKU_N,
.eeprom_size = IWL4965_EEPROM_IMG_SIZE,
.eeprom_ver = EEPROM_4965_EEPROM_VERSION,
@@ -2332,7 +2333,7 @@ struct iwl_cfg iwl4965_agn_cfg = {
};

/* Module firmware */
-MODULE_FIRMWARE("iwlwifi-4965" IWL4965_UCODE_API ".ucode");
+MODULE_FIRMWARE(IWL4965_MODULE_FIRMWARE);

module_param_named(antenna, iwl4965_mod_params.antenna, int, 0444);
MODULE_PARM_DESC(antenna, "select antenna (1=Main, 2=Aux, default 0 [both])");
--
1.5.4.3


2008-11-20 08:46:47

by Tomas Winkler

[permalink] [raw]
Subject: Re: [ipw3945-devel] [PATCH 7/7] iwlwifi: prevent double key removal

On Thu, Nov 20, 2008 at 8:56 AM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2008-11-19 at 15:32 -0800, Reinette Chatre wrote:
>
>
>> When the key is removed a second time the offset is set to 255 - this index
>> is not valid for the ucode_key_table and corrupts the eeprom pointer (which
>> is 255 bits from ucode_key_table).
>
>> + if (WARN(priv->stations[sta_id].sta.key.key_offset == WEP_INVALID_OFFSET,
>> + "Removing wrong key %d 0x%x\n", keyconf->keyidx, key_flags)) {
>> + spin_unlock_irqrestore(&priv->sta_lock, flags);
>> + return 0;
>> + }
>
> So, since _this_ patch has been tested to fix the problem, the WARN_ON
> must be triggering.

It fix the immediate problem of crashing the kernel but not the
problem why the key is removed twice.
I'm analyzing the log I've got from Carlos to figure this out you can
have a look as well.
It should be probably considered as a test patch. The purpose is not
to hide the bug buy fixing a symptom.
I suspect there is some problem in mac80211 is that this is happening
only on suspend/resumes not in regular flow

> What are you doing to address the actual bug that causes it to trigger?

If the flow of double removal of a key is okay from mac80211
perspective we just catch it internally which is always good otherwise
we need to fix also mac80211.
Thanks
Tomas

2008-11-19 23:31:55

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 3/7] iwlwifi: enable base band calibration in 5000 HW

From: Tomas Winkler <[email protected]>

This patch adds base band calibration support.

Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-5000.c | 8 ++++++--
drivers/net/wireless/iwlwifi/iwl-dev.h | 1 +
2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-5000.c b/drivers/net/wireless/iwlwifi/iwl-5000.c
index aeb69ee..478c5c3 100644
--- a/drivers/net/wireless/iwlwifi/iwl-5000.c
+++ b/drivers/net/wireless/iwlwifi/iwl-5000.c
@@ -475,6 +475,9 @@ static void iwl5000_rx_calib_result(struct iwl_priv *priv,
case IWL_PHY_CALIBRATE_TX_IQ_PERD_CMD:
index = IWL_CALIB_TX_IQ_PERD;
break;
+ case IWL_PHY_CALIBRATE_BASE_BAND_CMD:
+ index = IWL_CALIB_BASE_BAND;
+ break;
default:
IWL_ERROR("Unknown calibration notification %d\n",
hdr->op_code);
@@ -841,8 +844,9 @@ static int iwl5000_hw_set_hw_params(struct iwl_priv *priv)
priv->hw_params.calib_init_cfg =
BIT(IWL_CALIB_XTAL) |
BIT(IWL_CALIB_LO) |
- BIT(IWL_CALIB_TX_IQ) |
- BIT(IWL_CALIB_TX_IQ_PERD);
+ BIT(IWL_CALIB_TX_IQ) |
+ BIT(IWL_CALIB_TX_IQ_PERD) |
+ BIT(IWL_CALIB_BASE_BAND);
break;
case CSR_HW_REV_TYPE_5150:
priv->hw_params.calib_init_cfg = 0;
diff --git a/drivers/net/wireless/iwlwifi/iwl-dev.h b/drivers/net/wireless/iwlwifi/iwl-dev.h
index 1b305d8..4da988e 100644
--- a/drivers/net/wireless/iwlwifi/iwl-dev.h
+++ b/drivers/net/wireless/iwlwifi/iwl-dev.h
@@ -695,6 +695,7 @@ enum iwl_calib {
IWL_CALIB_LO,
IWL_CALIB_TX_IQ,
IWL_CALIB_TX_IQ_PERD,
+ IWL_CALIB_BASE_BAND,
IWL_CALIB_MAX
};

--
1.5.4.3


2008-11-20 19:15:00

by Tomas Winkler

[permalink] [raw]
Subject: Re: [ipw3945-devel] [PATCH 7/7] iwlwifi: prevent double key removal

On Thu, Nov 20, 2008 at 7:56 PM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2008-11-20 at 10:46 +0200, Tomas Winkler wrote:
>> On Thu, Nov 20, 2008 at 8:56 AM, Johannes Berg
>> <[email protected]> wrote:
>> > On Wed, 2008-11-19 at 15:32 -0800, Reinette Chatre wrote:
>> >
>> >
>> >> When the key is removed a second time the offset is set to 255 - this index
>> >> is not valid for the ucode_key_table and corrupts the eeprom pointer (which
>> >> is 255 bits from ucode_key_table).
>> >
>> >> + if (WARN(priv->stations[sta_id].sta.key.key_offset == WEP_INVALID_OFFSET,
>> >> + "Removing wrong key %d 0x%x\n", keyconf->keyidx, key_flags)) {
>> >> + spin_unlock_irqrestore(&priv->sta_lock, flags);
>> >> + return 0;
>> >> + }
>> >
>> > So, since _this_ patch has been tested to fix the problem, the WARN_ON
>> > must be triggering.
>>
>> It fix the immediate problem of crashing the kernel but not the
>> problem why the key is removed twice.
>> I'm analyzing the log I've got from Carlos to figure this out you can
>> have a look as well.
>> It should be probably considered as a test patch. The purpose is not
>> to hide the bug buy fixing a symptom.
>> I suspect there is some problem in mac80211 is that this is happening
>> only on suspend/resumes not in regular flow
>
> Hmm. Since we don't have suspend/resume that seems a bit odd. Maybe
> because we don't have suspend/resume in mac80211? Maybe you've already
> killed all the keys at suspend, but mac80211 doesn't know about that and
> removes them after resume when we disassoc?

Okay, thanks for the hint.

>> > What are you doing to address the actual bug that causes it to trigger?
>>
>> If the flow of double removal of a key is okay from mac80211
>> perspective we just catch it internally which is always good otherwise
>> we need to fix also mac80211.
>
> Do we really want the WARN() in this patch though? We already know the
> information from Carlos's log, so do we need to bother users?

Agree, will cook something else
Thanks
Tomas

2008-11-20 17:56:19

by Johannes Berg

[permalink] [raw]
Subject: Re: [ipw3945-devel] [PATCH 7/7] iwlwifi: prevent double key removal

On Thu, 2008-11-20 at 10:46 +0200, Tomas Winkler wrote:
> On Thu, Nov 20, 2008 at 8:56 AM, Johannes Berg
> <[email protected]> wrote:
> > On Wed, 2008-11-19 at 15:32 -0800, Reinette Chatre wrote:
> >
> >
> >> When the key is removed a second time the offset is set to 255 - this index
> >> is not valid for the ucode_key_table and corrupts the eeprom pointer (which
> >> is 255 bits from ucode_key_table).
> >
> >> + if (WARN(priv->stations[sta_id].sta.key.key_offset == WEP_INVALID_OFFSET,
> >> + "Removing wrong key %d 0x%x\n", keyconf->keyidx, key_flags)) {
> >> + spin_unlock_irqrestore(&priv->sta_lock, flags);
> >> + return 0;
> >> + }
> >
> > So, since _this_ patch has been tested to fix the problem, the WARN_ON
> > must be triggering.
>
> It fix the immediate problem of crashing the kernel but not the
> problem why the key is removed twice.
> I'm analyzing the log I've got from Carlos to figure this out you can
> have a look as well.
> It should be probably considered as a test patch. The purpose is not
> to hide the bug buy fixing a symptom.
> I suspect there is some problem in mac80211 is that this is happening
> only on suspend/resumes not in regular flow

Hmm. Since we don't have suspend/resume that seems a bit odd. Maybe
because we don't have suspend/resume in mac80211? Maybe you've already
killed all the keys at suspend, but mac80211 doesn't know about that and
removes them after resume when we disassoc?

> > What are you doing to address the actual bug that causes it to trigger?
>
> If the flow of double removal of a key is okay from mac80211
> perspective we just catch it internally which is always good otherwise
> we need to fix also mac80211.

Do we really want the WARN() in this patch though? We already know the
information from Carlos's log, so do we need to bother users?

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-11-20 06:56:43

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 7/7] iwlwifi: prevent double key removal

On Wed, 2008-11-19 at 15:32 -0800, Reinette Chatre wrote:


> When the key is removed a second time the offset is set to 255 - this index
> is not valid for the ucode_key_table and corrupts the eeprom pointer (which
> is 255 bits from ucode_key_table).

> + if (WARN(priv->stations[sta_id].sta.key.key_offset == WEP_INVALID_OFFSET,
> + "Removing wrong key %d 0x%x\n", keyconf->keyidx, key_flags)) {
> + spin_unlock_irqrestore(&priv->sta_lock, flags);
> + return 0;
> + }

So, since _this_ patch has been tested to fix the problem, the WARN_ON
must be triggering.

What are you doing to address the actual bug that causes it to trigger?

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-11-19 23:31:58

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 6/7] iwlwifi: TX update chicken bits

From: Winkler, Tomas <[email protected]>

This instructs FH to increment the retry count of a packet when
it is brought from the memory to TX-FIFO to save transactions
during aggregation flow.

Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-4965.c | 6 ++++++
drivers/net/wireless/iwlwifi/iwl-5000.c | 6 ++++++
drivers/net/wireless/iwlwifi/iwl-fh.h | 5 +++++
3 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index 60769b1..ab0b405 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -695,6 +695,7 @@ static int iwl4965_alive_notify(struct iwl_priv *priv)
unsigned long flags;
int ret;
int i, chan;
+ u32 reg_val;

spin_lock_irqsave(&priv->lock, flags);

@@ -724,6 +725,11 @@ static int iwl4965_alive_notify(struct iwl_priv *priv)
FH_TCSR_TX_CONFIG_REG_VAL_DMA_CHNL_ENABLE |
FH_TCSR_TX_CONFIG_REG_VAL_DMA_CREDIT_ENABLE);

+ /* Update FH chicken bits */
+ reg_val = iwl_read_direct32(priv, FH_TX_CHICKEN_BITS_REG);
+ iwl_write_direct32(priv, FH_TX_CHICKEN_BITS_REG,
+ reg_val | FH_TX_CHICKEN_BITS_SCD_AUTO_RETRY_EN);
+
/* Disable chain mode for all queues */
iwl_write_prph(priv, IWL49_SCD_QUEUECHAIN_SEL, 0);

diff --git a/drivers/net/wireless/iwlwifi/iwl-5000.c b/drivers/net/wireless/iwlwifi/iwl-5000.c
index d73760c..a738886 100644
--- a/drivers/net/wireless/iwlwifi/iwl-5000.c
+++ b/drivers/net/wireless/iwlwifi/iwl-5000.c
@@ -703,6 +703,7 @@ static int iwl5000_alive_notify(struct iwl_priv *priv)
unsigned long flags;
int ret;
int i, chan;
+ u32 reg_val;

spin_lock_irqsave(&priv->lock, flags);

@@ -732,6 +733,11 @@ static int iwl5000_alive_notify(struct iwl_priv *priv)
FH_TCSR_TX_CONFIG_REG_VAL_DMA_CHNL_ENABLE |
FH_TCSR_TX_CONFIG_REG_VAL_DMA_CREDIT_ENABLE);

+ /* Update FH chicken bits */
+ reg_val = iwl_read_direct32(priv, FH_TX_CHICKEN_BITS_REG);
+ iwl_write_direct32(priv, FH_TX_CHICKEN_BITS_REG,
+ reg_val | FH_TX_CHICKEN_BITS_SCD_AUTO_RETRY_EN);
+
iwl_write_prph(priv, IWL50_SCD_QUEUECHAIN_SEL,
IWL50_SCD_QUEUECHAIN_SEL_ALL(priv->hw_params.max_txq_num));
iwl_write_prph(priv, IWL50_SCD_AGGR_SEL, 0);
diff --git a/drivers/net/wireless/iwlwifi/iwl-fh.h b/drivers/net/wireless/iwlwifi/iwl-fh.h
index bc20acb..c3dadb0 100644
--- a/drivers/net/wireless/iwlwifi/iwl-fh.h
+++ b/drivers/net/wireless/iwlwifi/iwl-fh.h
@@ -396,6 +396,11 @@
#define FH_SRVC_CHNL_SRAM_ADDR_REG(_chnl) \
(FH_SRVC_LOWER_BOUND + ((_chnl) - 9) * 0x4)

+#define FH_TX_CHICKEN_BITS_REG (FH_MEM_LOWER_BOUND + 0xE98)
+/* Instruct FH to increment the retry count of a packet when
+ * it is brought from the memory to TX-FIFO
+ */
+#define FH_TX_CHICKEN_BITS_SCD_AUTO_RETRY_EN (0x00000002)

/**
* struct iwl_rb_status - reseve buffer status
--
1.5.4.3