2010-04-08 17:19:48

by Jeff Chua

[permalink] [raw]
Subject: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless


Linus,

I'm having problem with the latest git pull 2 days ago with the 4965AGN
wireless.

Starting the wireless with WPA2 resulted in system hard freeze. Reverting
the commit below solves the problem.


Thanks,
Jeff


commit be6b38bcb175613f239e0b302607db346472c6b6
Author: Wey-Yi Guy <[email protected]>
Date: Thu Mar 18 09:05:00 2010 -0700

iwlwifi: counting number of tfds can be free for 4965

Forget one hunk in 4965 during "iwlwifi: error checking for number of tfds
in queue" patch.

Reported-by: Shanyu Zhao <[email protected]>
Signed-off-by: Wey-Yi Guy <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
CC: [email protected]

diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index 1bd2cd8..83c52a6 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -2041,16 +2041,14 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
tx_resp->failure_frame);

freed = iwl_tx_queue_reclaim(priv, txq_id, index);
- if (qc && likely(sta_id != IWL_INVALID_STATION))
- priv->stations[sta_id].tid[tid].tfds_in_queue -= freed;
+ iwl_free_tfds_in_queue(priv, sta_id, tid, freed);

if (priv->mac80211_registered &&
(iwl_queue_space(&txq->q) > txq->q.low_mark))
iwl_wake_queue(priv, txq_id);
}

- if (qc && likely(sta_id != IWL_INVALID_STATION))
- iwl_txq_check_empty(priv, sta_id, tid, txq_id);
+ iwl_txq_check_empty(priv, sta_id, tid, txq_id);

if (iwl_check_bits(status, TX_ABORT_REQUIRED_MSK))
IWL_ERR(priv, "TODO: Implement Tx ABORT REQUIRED!!!\n");


2010-04-08 17:23:24

by Linus Torvalds

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless



On Fri, 9 Apr 2010, Jeff Chua wrote:
>
> I'm having problem with the latest git pull 2 days ago with the 4965AGN
> wireless.
>
> Starting the wireless with WPA2 resulted in system hard freeze. Reverting the
> commit below solves the problem.

I don't mind reverting patches, but I hate doing it blind and without
_any_ chance for the guilty party to try to fix it first.

Can you post the oops that your subject implies happened? Maybe the driver
authors can find the proper fix without a revert?

I'll revert if if I hear no updates in a couple of days - please remind
me.

Linus

2010-04-08 17:53:52

by Wey-Yi Guy

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

Hi Jeff,

On Thu, 2010-04-08 at 10:16 -0700, Jeff Chua wrote:
> Linus,
>
> I'm having problem with the latest git pull 2 days ago with the 4965AGN
> wireless.
>
> Starting the wireless with WPA2 resulted in system hard freeze. Reverting
> the commit below solves the problem.
>
>
> Thanks,
> Jeff
>
>
Sorry for the problem caused by this patch, it is my mistake using the
similar approach for 4965 like the newer devices. could you try the
attach patch to see if this fix your system freeze problem.

Regards
Wey


Attachments:
0001-iwlwifi-need-check-for-valid-qos-packet-before-free.patch (1.61 kB)

2010-04-08 18:13:52

by Al Viro

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

On Fri, Apr 09, 2010 at 01:16:43AM +0800, Jeff Chua wrote:
>
> index 1bd2cd8..83c52a6 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-4965.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
> @@ -2041,16 +2041,14 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
> tx_resp->failure_frame);
>
> freed = iwl_tx_queue_reclaim(priv, txq_id, index);
> - if (qc && likely(sta_id != IWL_INVALID_STATION))
> - priv->stations[sta_id].tid[tid].tfds_in_queue -= freed;
> + iwl_free_tfds_in_queue(priv, sta_id, tid, freed);

So what happens if we hit sta_id == IWL_INVALID_STATION and !txq->sched_retry?

AFAICS, IWL_INVALID_STATION is 255 and priv->stations[] has only 32 elements.
And code around that place is
if (txq->sched_retry && unlikely(sta_id == IWL_INVALID_STATION)) {
IWL_ERR(priv, "Station not known\n");
return;
}
if (txq->sched_retry) {
....
} else {
....
the code modified in that chunk
....
}
so this removal of check for sta_id doesn't look apriori safe...

I'm not familiar with that code and I don't have the hardware, so this is
just from RTFS, but... might make sense to replace that call of
iwl_free_tfds_in_queue with

if (sta_id == IWL_INVALID_STATION)
printk(KERN_ERR "buggered");
else
iwl_free_tfds_in_queue(priv, sta_id, tid, freed);

and see if that helps and if printk gets triggered.

2010-04-08 19:13:04

by Wey-Yi Guy

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless


On Thu, 2010-04-08 at 11:13 -0700, Al Viro wrote:
> On Fri, Apr 09, 2010 at 01:16:43AM +0800, Jeff Chua wrote:
> >
> > index 1bd2cd8..83c52a6 100644
> > --- a/drivers/net/wireless/iwlwifi/iwl-4965.c
> > +++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
> > @@ -2041,16 +2041,14 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
> > tx_resp->failure_frame);
> >
> > freed = iwl_tx_queue_reclaim(priv, txq_id, index);
> > - if (qc && likely(sta_id != IWL_INVALID_STATION))
> > - priv->stations[sta_id].tid[tid].tfds_in_queue -= freed;
> > + iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
>
> So what happens if we hit sta_id == IWL_INVALID_STATION and !txq->sched_retry?
>
> AFAICS, IWL_INVALID_STATION is 255 and priv->stations[] has only 32 elements.
> And code around that place is
> if (txq->sched_retry && unlikely(sta_id == IWL_INVALID_STATION)) {
> IWL_ERR(priv, "Station not known\n");
> return;
> }
> if (txq->sched_retry) {
> ....
> } else {
> ....
> the code modified in that chunk
> ....
> }
> so this removal of check for sta_id doesn't look apriori safe...
>
> I'm not familiar with that code and I don't have the hardware, so this is
> just from RTFS, but... might make sense to replace that call of
> iwl_free_tfds_in_queue with
>
> if (sta_id == IWL_INVALID_STATION)
> printk(KERN_ERR "buggered");
> else
> iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
>
> and see if that helps and if printk gets triggered.

We can check for IWL_INVALID_STATION and print log, but need to check
for qc to make sure it is valid; maybe something like
if (qc && likely(sta_id != IWL_INVALID_STATION))
iwl_free_tfds_in_queue(priv, sta_id,
tid, freed);
else {
if (sta_id == IWL_INVALID_STATION)
IWL_ERR(priv, "invalid station"");
}

Wey


2010-04-08 19:28:30

by Jeff Chua

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless



On Fri, Apr 9, 2010 at 1:19 AM, Linus Torvalds
<[email protected]> wrote:

> I don't mind reverting patches, but I hate doing it blind and without
> _any_ chance for the guilty party to try to fix it first.

> Can you post the oops that your subject implies happened? Maybe the
> driver authors can find the proper fix without a revert?

The screen is so fast and never stops so I don't know how to capture that.
I just tried the patch from Wey and had to modify it slightly to make it
work.



On Fri, Apr 9, 2010 at 2:50 AM, Guy, Wey-Yi <[email protected]>
wrote:

> Sorry for the problem caused by this patch, it is my mistake using the
similar approach for 4965 like the newer devices. could you try the
attach patch to see if this fix your system freeze problem.

Wey, the patch doesn't apply cleanly. I believe you meant "iwl_" instead
of "iwlagn_".

Also, the checking for IWL_INVALID_STATION should be done after the "}
else {" as in the original code prior to your patch. I just verified this
with the patch below and it no longer oops.



On Fri, Apr 9, 2010 at 2:13 AM, Al Viro <[email protected]> wrote:

> So what happens if we hit sta_id == IWL_INVALID_STATION and
> !txq->sched_retry?

> I'm not familiar with that code and I don't have the hardware, so this
> is just from RTFS, but... might make sense to replace that call of
> iwl_free_tfds_in_queue with
> if (sta_id == IWL_INVALID_STATION)
> printk(KERN_ERR "buggered");
> else
> iwl_free_tfds_in_queue(priv, sta_id, tid, freed);


I just tried that, but not seeing any invalid station messages. The
KERN_ERR has been added below.




On Fri, Apr 9, 2010 at 4:09 AM, Guy, Wey-Yi <[email protected]>
wrote:

> We can check for IWL_INVALID_STATION and print log, but need to check
> for qc to make sure it is valid; maybe something like


Ok, added below.



Thanks,
Jeff

--- drivers/net/wireless/iwlwifi/iwl-4965.c.org 2010-04-09 02:11:45.000000000 +0800
+++ drivers/net/wireless/iwlwifi/iwl-4965.c 2010-04-09 03:21:57.000000000 +0800
@@ -2012,10 +2012,18 @@

if (txq->q.read_ptr != (scd_ssn & 0xff)) {
index = iwl_queue_dec_wrap(scd_ssn & 0xff, txq->q.n_bd);
- IWL_DEBUG_TX_REPLY(priv, "Retry scheduler reclaim scd_ssn "
- "%d index %d\n", scd_ssn , index);
+ IWL_DEBUG_TX_REPLY(priv,
+ "Retry scheduler reclaim scd_ssn "
+ "%d index %d\n", scd_ssn , index);
freed = iwl_tx_queue_reclaim(priv, txq_id, index);
iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ if (qc && likely(sta_id != IWL_INVALID_STATION))
+ iwl_free_tfds_in_queue(priv, sta_id, tid,
+ freed);
+ else {
+ if (sta_id == IWL_INVALID_STATION)
+ IWL_ERR(priv, "invalid station");
+ }

if (priv->mac80211_registered &&
(iwl_queue_space(&txq->q) > txq->q.low_mark) &&
@@ -2041,14 +2049,20 @@
tx_resp->failure_frame);

freed = iwl_tx_queue_reclaim(priv, txq_id, index);
- iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ if (qc && likely(sta_id != IWL_INVALID_STATION))
+ iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ else {
+ if (sta_id == IWL_INVALID_STATION)
+ IWL_ERR(priv, "invalid station");
+ }

if (priv->mac80211_registered &&
(iwl_queue_space(&txq->q) > txq->q.low_mark))
iwl_wake_queue(priv, txq_id);
}

- iwl_txq_check_empty(priv, sta_id, tid, txq_id);
+ if (qc && likely(sta_id != IWL_INVALID_STATION))
+ iwl_txq_check_empty(priv, sta_id, tid, txq_id);

if (iwl_check_bits(status, TX_ABORT_REQUIRED_MSK))
IWL_ERR(priv, "TODO: Implement Tx ABORT REQUIRED!!!\n");

2010-04-08 19:28:32

by Wey-Yi Guy

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

Hi Viro and Jeff,

On Thu, 2010-04-08 at 11:13 -0700, Al Viro wrote:
> On Fri, Apr 09, 2010 at 01:16:43AM +0800, Jeff Chua wrote:
> >
> > index 1bd2cd8..83c52a6 100644
> > --- a/drivers/net/wireless/iwlwifi/iwl-4965.c
> > +++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
> > @@ -2041,16 +2041,14 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
> > tx_resp->failure_frame);
> >
> > freed = iwl_tx_queue_reclaim(priv, txq_id, index);
> > - if (qc && likely(sta_id != IWL_INVALID_STATION))
> > - priv->stations[sta_id].tid[tid].tfds_in_queue -= freed;
> > + iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
>
> So what happens if we hit sta_id == IWL_INVALID_STATION and !txq->sched_retry?
>
> AFAICS, IWL_INVALID_STATION is 255 and priv->stations[] has only 32 elements.
> And code around that place is
> if (txq->sched_retry && unlikely(sta_id == IWL_INVALID_STATION)) {
> IWL_ERR(priv, "Station not known\n");
> return;
> }
> if (txq->sched_retry) {
> ....
> } else {
> ....
> the code modified in that chunk
> ....
> }
> so this removal of check for sta_id doesn't look apriori safe...
>
> I'm not familiar with that code and I don't have the hardware, so this is
> just from RTFS, but... might make sense to replace that call of
> iwl_free_tfds_in_queue with
>
> if (sta_id == IWL_INVALID_STATION)
> printk(KERN_ERR "buggered");
> else
> iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
>
> and see if that helps and if printk gets triggered.

Maybe this patch looks better, if sched_rety and sta_id ==
IWL_INVALID_ID_STATION, this function already return before reach
iwl_free_tfds_in_queue, so do not have to check for sta_id ==
IWL_INVALID_ID_STATION. the other case, print log if sta_id ==
IWL_INVALID_ID_STATION.

Wey


Attachments:
0001-iwlwifi-need-check-for-valid-qos-packet-before-free.patch (1.95 kB)

2010-04-08 19:43:15

by Jeff Chua

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless



On Fri, Apr 9, 2010 at 4:24 AM, Guy, Wey-Yi <[email protected]>
wrote:

> Maybe this patch looks better, if sched_rety and sta_id ==

Wey,

I've updated your patch and tested it. Wireless seems ok now.

Please verify it and inform Linus once you sign-off on it.


Thanks,
Jeff

--- a/drivers/net/wireless/iwlwifi/iwl-4965.c.org 2010-04-09 02:11:45.000000000 +0800
+++ a/drivers/net/wireless/iwlwifi/iwl-4965.c 2010-04-09 03:33:43.000000000 +0800
@@ -2012,10 +2012,15 @@

if (txq->q.read_ptr != (scd_ssn & 0xff)) {
index = iwl_queue_dec_wrap(scd_ssn & 0xff, txq->q.n_bd);
- IWL_DEBUG_TX_REPLY(priv, "Retry scheduler reclaim scd_ssn "
- "%d index %d\n", scd_ssn , index);
+ IWL_DEBUG_TX_REPLY(priv,
+ "Retry scheduler reclaim scd_ssn "
+ "%d index %d\n", scd_ssn , index);
freed = iwl_tx_queue_reclaim(priv, txq_id, index);
- iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ if(qc)
+ iwl_free_tfds_in_queue(priv, sta_id, tid,
+ freed);
+ else if (sta_id == IWL_INVALID_STATION)
+ IWL_ERR(priv, "invalid station");

if (priv->mac80211_registered &&
(iwl_queue_space(&txq->q) > txq->q.low_mark) &&
@@ -2041,14 +2046,18 @@
tx_resp->failure_frame);

freed = iwl_tx_queue_reclaim(priv, txq_id, index);
- iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ if (qc && likely(sta_id != IWL_INVALID_STATION))
+ iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ else if (sta_id == IWL_INVALID_STATION)
+ IWL_ERR(priv, "invalid station");

if (priv->mac80211_registered &&
(iwl_queue_space(&txq->q) > txq->q.low_mark))
iwl_wake_queue(priv, txq_id);
}

- iwl_txq_check_empty(priv, sta_id, tid, txq_id);
+ if (qc && likely(sta_id != IWL_INVALID_STATION))
+ iwl_txq_check_empty(priv, sta_id, tid, txq_id);

if (iwl_check_bits(status, TX_ABORT_REQUIRED_MSK))
IWL_ERR(priv, "TODO: Implement Tx ABORT REQUIRED!!!\n");

2010-04-08 19:47:18

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

On Thu, 2010-04-08 at 10:19 -0700, Linus Torvalds wrote:
>
> On Fri, 9 Apr 2010, Jeff Chua wrote:
> >
> > I'm having problem with the latest git pull 2 days ago with the 4965AGN
> > wireless.
> >
> > Starting the wireless with WPA2 resulted in system hard freeze. Reverting the
> > commit below solves the problem.
>
> I don't mind reverting patches, but I hate doing it blind and without
> _any_ chance for the guilty party to try to fix it first.
>
> Can you post the oops that your subject implies happened? Maybe the driver
> authors can find the proper fix without a revert?
>
> I'll revert if if I hear no updates in a couple of days - please remind
> me.

Does remind me of the iwlagn trouble I had, see
https://bugzilla.kernel.org/show_bug.cgi?id=15667 and
https://bugzilla.kernel.org/attachment.cgi?id=25881 for a patch that
seems to have cured my laptop.

2010-04-08 19:53:36

by Wey-Yi Guy

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

Hi Jeff,


On Thu, 2010-04-08 at 12:42 -0700, Jeff Chua wrote:
>
> On Fri, Apr 9, 2010 at 4:24 AM, Guy, Wey-Yi <[email protected]>
> wrote:
>
> > Maybe this patch looks better, if sched_rety and sta_id ==
>
> Wey,
>
> I've updated your patch and tested it. Wireless seems ok now.
>
> Please verify it and inform Linus once you sign-off on it.
>
>
> Thanks,
> Jeff
>
> --- a/drivers/net/wireless/iwlwifi/iwl-4965.c.org 2010-04-09 02:11:45.000000000 +0800
> +++ a/drivers/net/wireless/iwlwifi/iwl-4965.c 2010-04-09 03:33:43.000000000 +0800
> @@ -2012,10 +2012,15 @@
>
> if (txq->q.read_ptr != (scd_ssn & 0xff)) {
> index = iwl_queue_dec_wrap(scd_ssn & 0xff, txq->q.n_bd);
> - IWL_DEBUG_TX_REPLY(priv, "Retry scheduler reclaim scd_ssn "
> - "%d index %d\n", scd_ssn , index);
> + IWL_DEBUG_TX_REPLY(priv,
> + "Retry scheduler reclaim scd_ssn "
> + "%d index %d\n", scd_ssn , index);
> freed = iwl_tx_queue_reclaim(priv, txq_id, index);
> - iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
> + if(qc)
> + iwl_free_tfds_in_queue(priv, sta_id, tid,
> + freed);
> + else if (sta_id == IWL_INVALID_STATION)
> + IWL_ERR(priv, "invalid station");
>
do not need to check sta_id == IWL_INVALID_STATION here since already
check before.
could you try my revised version of patch I send after my first attempt.
to make sure it works.

I attach here again in case you did not get it.

Thanks
Wey



Attachments:
0001-iwlwifi-need-check-for-valid-qos-packet-before-free.patch (1.95 kB)

2010-04-08 20:15:30

by John W. Linville

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

On Thu, Apr 08, 2010 at 01:50:00PM -0700, Guy, Wey-Yi wrote:

> I attach here again in case you did not get it.
>
> Thanks
> Wey
>
>

> From f3a5f691e32c0edfbc42a1e8687392dc3efe396e Mon Sep 17 00:00:00 2001
> From: Wey-Yi Guy <[email protected]>
> Date: Thu, 8 Apr 2010 13:17:37 -0700
> Subject: [PATCH 1/1] iwlwifi: need check for valid qos packet before free
>
> For 4965, need to check it is valid qos frame before free, only valid
> QoS frame has the tid used to free the packets.
>
> Signed-off-by: Wey-Yi Guy <[email protected]>

I would prefer to take this through the normal wireless-2.6 route if
you don't mind. Are you satisifed with this version?

Thanks,

John

P.S. In the future, I'm also quite open to revert requests as needed.
It makes my life easier for things (even reverts) under wireless to
come through my tree rather than trying to figure-out how something
happened later...
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.

2010-04-08 20:30:28

by John W. Linville

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

On Fri, Apr 09, 2010 at 03:27:53AM +0800, Jeff Chua wrote:

> Wey, the patch doesn't apply cleanly. I believe you meant "iwl_" instead
> of "iwlagn_".

He based his patch on wireless-testing (or something similar), where
iwlagn_ is the proper prefix for the functions in question.

John

P.S. Cc'ing linux-wireless...

P.P.S. You might try this version of his later patch...

>From ece6444c2fe80dab679beb5f0d58b091f1933b00 Mon Sep 17 00:00:00 2001
From: Wey-Yi Guy <[email protected]>
Date: Thu, 8 Apr 2010 13:17:37 -0700
Subject: [PATCH] iwlwifi: need check for valid qos packet before free

For 4965, need to check it is valid qos frame before free, only valid
QoS frame has the tid used to free the packets.

Signed-off-by: Wey-Yi Guy <[email protected]>
Signed-off-by: John W. Linville <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-4965.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index 83c52a6..8972166 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -2015,7 +2015,9 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
IWL_DEBUG_TX_REPLY(priv, "Retry scheduler reclaim scd_ssn "
"%d index %d\n", scd_ssn , index);
freed = iwl_tx_queue_reclaim(priv, txq_id, index);
- iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ if (qc)
+ iwl_free_tfds_in_queue(priv, sta_id,
+ tid, freed);

if (priv->mac80211_registered &&
(iwl_queue_space(&txq->q) > txq->q.low_mark) &&
@@ -2041,14 +2043,17 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
tx_resp->failure_frame);

freed = iwl_tx_queue_reclaim(priv, txq_id, index);
- iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ if (qc && likely(sta_id != IWL_INVALID_STATION))
+ iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ else if (sta_id == IWL_INVALID_STATION)
+ IWL_DEBUG_TX_REPLY(priv, "Station not known\n");

if (priv->mac80211_registered &&
(iwl_queue_space(&txq->q) > txq->q.low_mark))
iwl_wake_queue(priv, txq_id);
}
-
- iwl_txq_check_empty(priv, sta_id, tid, txq_id);
+ if (qc && likely(sta_id != IWL_INVALID_STATION))
+ iwl_txq_check_empty(priv, sta_id, tid, txq_id);

if (iwl_check_bits(status, TX_ABORT_REQUIRED_MSK))
IWL_ERR(priv, "TODO: Implement Tx ABORT REQUIRED!!!\n");
--
1.6.2.5

--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.

2010-04-08 20:32:16

by Wey-Yi Guy

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

Hi John,

On Thu, 2010-04-08 at 13:02 -0700, John W. Linville wrote:
> On Thu, Apr 08, 2010 at 01:50:00PM -0700, Guy, Wey-Yi wrote:
>
> > I attach here again in case you did not get it.
> >
> > Thanks
> > Wey
> >
> >
>
> > From f3a5f691e32c0edfbc42a1e8687392dc3efe396e Mon Sep 17 00:00:00 2001
> > From: Wey-Yi Guy <[email protected]>
> > Date: Thu, 8 Apr 2010 13:17:37 -0700
> > Subject: [PATCH 1/1] iwlwifi: need check for valid qos packet before free
> >
> > For 4965, need to check it is valid qos frame before free, only valid
> > QoS frame has the tid used to free the packets.
> >
> > Signed-off-by: Wey-Yi Guy <[email protected]>
>
> I would prefer to take this through the normal wireless-2.6 route if
> you don't mind. Are you satisifed with this version?
>

Thanks for the help, the finial version should take care of the problem
and ready for upstream .

Regards
Wey
>
> P.S. In the future, I'm also quite open to revert requests as needed.
> It makes my life easier for things (even reverts) under wireless to
> come through my tree rather than trying to figure-out how something
> happened later...

2010-04-08 20:45:21

by John W. Linville

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

On Thu, Apr 08, 2010 at 04:19:53PM -0400, John W. Linville wrote:
> On Fri, Apr 09, 2010 at 03:27:53AM +0800, Jeff Chua wrote:
>
> > Wey, the patch doesn't apply cleanly. I believe you meant "iwl_" instead
> > of "iwlagn_".
>
> He based his patch on wireless-testing (or something similar), where
> iwlagn_ is the proper prefix for the functions in question.

"She based her patch..." -- my apologies!

John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.

2010-04-09 02:18:34

by Wey-Yi Guy

[permalink] [raw]
Subject: Re: [REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

Hi Jeff,

On Thu, 2010-04-08 at 17:36 -0700, Jeff Chua wrote:
>
>
> On Fri, Apr 9, 2010 at 4:50 AM, Guy, Wey-Yi <[email protected]>
> wrote:
> do not need to check sta_id == IWL_INVALID_STATION here since
> already
> check before.
> could you try my revised version of patch I send after my
> first attempt.
> to make sure it works.
>
> I attach here again in case you did not get it.
>
> Wey,
>
> One small touch-up. Again, I've to rename "iwlagn_" to "iwl_" so that
> it can apply to Linus's base code.
>
> Tested and working fine. Here's the updated patch.
>
Thanks for verify the patch, you are correct, since I am using the
wireless-testing version of the kernel, you will need to make the
changes in order to get the patch apply. Sorry for the trouble.

Regards
Wey

>
>
>

2010-04-09 15:45:22

by John W. Linville

[permalink] [raw]
Subject: pull request: wireless-2.6 2010-04-09

Dave,

This fix is intended for 2.6.34. It resolves an issue involving an
Oops on boxes w/ iwl4965 hardware, apparently introduced by another
recent patch. The thread describing the issue and the resolution
is here:

http://marc.info/?l=linux-kernel&m=127074721531649&w=2

In order to avoid reverting that patch, please accept this fix instead.
As usual, please let me know if there are problems!

Thanks,

John

---

The following changes since commit 2626419ad5be1a054d350786b684b41d23de1538:
David S. Miller (1):
tcp: Set CHECKSUM_UNNECESSARY in tcp_init_nondata_skb

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6.git master

Wey-Yi Guy (1):
iwlwifi: need check for valid qos packet before free

drivers/net/wireless/iwlwifi/iwl-4965.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index 83c52a6..8972166 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -2015,7 +2015,9 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
IWL_DEBUG_TX_REPLY(priv, "Retry scheduler reclaim scd_ssn "
"%d index %d\n", scd_ssn , index);
freed = iwl_tx_queue_reclaim(priv, txq_id, index);
- iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ if (qc)
+ iwl_free_tfds_in_queue(priv, sta_id,
+ tid, freed);

if (priv->mac80211_registered &&
(iwl_queue_space(&txq->q) > txq->q.low_mark) &&
@@ -2041,14 +2043,17 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
tx_resp->failure_frame);

freed = iwl_tx_queue_reclaim(priv, txq_id, index);
- iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ if (qc && likely(sta_id != IWL_INVALID_STATION))
+ iwl_free_tfds_in_queue(priv, sta_id, tid, freed);
+ else if (sta_id == IWL_INVALID_STATION)
+ IWL_DEBUG_TX_REPLY(priv, "Station not known\n");

if (priv->mac80211_registered &&
(iwl_queue_space(&txq->q) > txq->q.low_mark))
iwl_wake_queue(priv, txq_id);
}
-
- iwl_txq_check_empty(priv, sta_id, tid, txq_id);
+ if (qc && likely(sta_id != IWL_INVALID_STATION))
+ iwl_txq_check_empty(priv, sta_id, tid, txq_id);

if (iwl_check_bits(status, TX_ABORT_REQUIRED_MSK))
IWL_ERR(priv, "TODO: Implement Tx ABORT REQUIRED!!!\n");
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.

2010-04-09 17:03:40

by David Miller

[permalink] [raw]
Subject: Re: pull request: wireless-2.6 2010-04-09

From: "John W. Linville" <[email protected]>
Date: Fri, 9 Apr 2010 11:38:07 -0400

> This fix is intended for 2.6.34. It resolves an issue involving an
> Oops on boxes w/ iwl4965 hardware, apparently introduced by another
> recent patch. The thread describing the issue and the resolution
> is here:
>
> http://marc.info/?l=linux-kernel&m=127074721531649&w=2
>
> In order to avoid reverting that patch, please accept this fix instead.
> As usual, please let me know if there are problems!

Pulled, thanks John.