[REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

[REVERT] be6b38bcb175613f239e0b302607db346472c6b6. v2.6.34-rc3-406 oops with 4965AGN wireless

[Jeff Chua]

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 <wey-yi.w.guy@intel.com>
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 <shanyu.zhao@intel.com>
     Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
     Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
     CC: stable@kernel.org

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

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

[Linus Torvalds]

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

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

[Al Viro]

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.

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

[Guy, Wey-Yi]

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

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

[-- Attachment #2: 0001-iwlwifi-need-check-for-valid-qos-packet-before-free.patch --]
[-- Type: text/x-patch, Size: 1647 bytes --]

>From 0c441b5c684709152cd322e91ce59914159ea513 Mon Sep 17 00:00:00 2001
From: Wey-Yi Guy <wey-yi.w.guy@intel.com>
Date: Thu, 8 Apr 2010 11:41:24 -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 <wey-yi.w.guy@intel.com>
---
 drivers/net/wireless/iwlwifi/iwl-4965.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index c710b26..4113909 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -2014,7 +2014,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 = iwlagn_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);
 
 			if (priv->mac80211_registered &&
 			    (iwl_queue_space(&txq->q) > txq->q.low_mark) &&
@@ -2046,8 +2048,8 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
 		    (iwl_queue_space(&txq->q) > txq->q.low_mark))
 			iwl_wake_queue(priv, txq_id);
 	}
-
-	iwlagn_txq_check_empty(priv, sta_id, tid, txq_id);
+	if (qc && likely(sta_id != IWL_INVALID_STATION))
+		iwlagn_txq_check_empty(priv, sta_id, tid, txq_id);
 
 	iwl_check_abort_status(priv, tx_resp->frame_count, status);
 }
-- 
1.5.6.3

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

[Peter Zijlstra]

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.

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

[Guy, Wey-Yi]

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

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

[Guy, Wey-Yi]

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

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

[-- Attachment #2: 0001-iwlwifi-need-check-for-valid-qos-packet-before-free.patch --]
[-- Type: text/x-patch, Size: 1998 bytes --]

>From f3a5f691e32c0edfbc42a1e8687392dc3efe396e Mon Sep 17 00:00:00 2001
From: Wey-Yi Guy <wey-yi.w.guy@intel.com>
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 <wey-yi.w.guy@intel.com>
---
 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 c710b26..2e3cda7 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -2014,7 +2014,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 = iwlagn_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) &&
@@ -2040,14 +2042,17 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
 				   tx_resp->failure_frame);
 
 		freed = iwlagn_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);
 	}
-
-	iwlagn_txq_check_empty(priv, sta_id, tid, txq_id);
+	if (qc && likely(sta_id != IWL_INVALID_STATION))
+		iwlagn_txq_check_empty(priv, sta_id, tid, txq_id);
 
 	iwl_check_abort_status(priv, tx_resp->frame_count, status);
 }
-- 
1.5.6.3

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

[Guy, Wey-Yi]

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 <wey-yi.w.guy@intel.com>
> 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

> 
> 
> 

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

[Guy, Wey-Yi]

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 <wey-yi.w.guy@intel.com>
> > 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 <wey-yi.w.guy@intel.com>
> 
> 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...

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

[Guy, Wey-Yi]

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

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 <wey-yi.w.guy@intel.com> 
> 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



[-- Attachment #2: 0001-iwlwifi-need-check-for-valid-qos-packet-before-free.patch --]
[-- Type: text/x-patch, Size: 1998 bytes --]

>From f3a5f691e32c0edfbc42a1e8687392dc3efe396e Mon Sep 17 00:00:00 2001
From: Wey-Yi Guy <wey-yi.w.guy@intel.com>
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 <wey-yi.w.guy@intel.com>
---
 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 c710b26..2e3cda7 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -2014,7 +2014,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 = iwlagn_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) &&
@@ -2040,14 +2042,17 @@ static void iwl4965_rx_reply_tx(struct iwl_priv *priv,
 				   tx_resp->failure_frame);
 
 		freed = iwlagn_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);
 	}
-
-	iwlagn_txq_check_empty(priv, sta_id, tid, txq_id);
+	if (qc && likely(sta_id != IWL_INVALID_STATION))
+		iwlagn_txq_check_empty(priv, sta_id, tid, txq_id);
 
 	iwl_check_abort_status(priv, tx_resp->frame_count, status);
 }
-- 
1.5.6.3

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

[John W. Linville]

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
linville@tuxdriver.com			might be all we have.  Be ready.

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

[John W. Linville]

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 <wey-yi.w.guy@intel.com>
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 <wey-yi.w.guy@intel.com>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 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
linville@tuxdriver.com			might be all we have.  Be ready.

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

[John W. Linville]

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 <wey-yi.w.guy@intel.com>
> 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 <wey-yi.w.guy@intel.com>

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
linville@tuxdriver.com			might be all we have.  Be ready.

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

[Jeff Chua]

On Fri, Apr 9, 2010 at 4:24 AM, Guy, Wey-Yi <wey-yi.w.guy@intel.com> 
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");

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

[Jeff Chua]

On Fri, Apr 9, 2010 at 1:19 AM, Linus Torvalds 
<torvalds@linux-foundation.org> 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 <wey-yi.w.guy@intel.com> 
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 <viro@zeniv.linux.org.uk> 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 <wey-yi.w.guy@intel.com> 
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");