[PATCH] libertas: before sleeping, check for a command result

[PATCH] libertas: before sleeping, check for a command result

[Holger Schurig]

If we don't check for a command response early, but rather sleep,
then we might sleep despite an already-received command response.
This will lead to a command-timeout.

Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>

Index: wireless-testing/drivers/net/wireless/libertas/main.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/libertas/main.c	2008-05-23 14:25:16.000000000 +0200
+++ wireless-testing/drivers/net/wireless/libertas/main.c	2008-05-23 14:25:51.000000000 +0200
@@ -722,14 +722,14 @@ static int lbs_thread(void *data)
 			shouldsleep = 1;	/* Something is en route to the device already */
 		else if (priv->tx_pending_len > 0)
 			shouldsleep = 0;	/* We've a packet to send */
+		else if (priv->resp_len[priv->resp_idx])
+			shouldsleep = 0;	/* We have a command response */
 		else if (priv->cur_cmd)
 			shouldsleep = 1;	/* Can't send a command; one already running */
 		else if (!list_empty(&priv->cmdpendingq))
 			shouldsleep = 0;	/* We have a command to send */
 		else if (__kfifo_len(priv->event_fifo))
 			shouldsleep = 0;	/* We have an event to process */
-		else if (priv->resp_len[priv->resp_idx])
-			shouldsleep = 0;	/* We have a command response */
 		else
 			shouldsleep = 1;	/* No command */
 

RE: [PATCH] libertas: before sleeping, check for a command result

[Johan Adolfsson]

> -----Original Message-----
> From: libertas-dev-bounces@lists.infradead.org 
> [mailto:libertas-dev-bounces@lists.infradead.org] On Behalf 
> Of Holger Schurig
> Sent: den 23 maj 2008 16:04
> To: libertas-dev@lists.infradead.org; Dan Williams; 
> linux-wireless@vger.kernel.org; John W. Linville
> Subject: [PATCH] libertas: before sleeping, check for a command result
> 
> 
> If we don't check for a command response early, but rather sleep,
> then we might sleep despite an already-received command response.
> This will lead to a command-timeout.
> 
> Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>
> 
> Index: wireless-testing/drivers/net/wireless/libertas/main.c
> ===================================================================
> --- 
> wireless-testing.orig/drivers/net/wireless/libertas/main.c	
> 2008-05-23 14:25:16.000000000 +0200
> +++ wireless-testing/drivers/net/wireless/libertas/main.c	
> 2008-05-23 14:25:51.000000000 +0200
> @@ -722,14 +722,14 @@ static int lbs_thread(void *data)
>  			shouldsleep = 1;	/* Something is 
> en route to the device already */
>  		else if (priv->tx_pending_len > 0)
>  			shouldsleep = 0;	/* We've a 
> packet to send */
> +		else if (priv->resp_len[priv->resp_idx])
> +			shouldsleep = 0;	/* We have a 
> command response */
>  		else if (priv->cur_cmd)
>  			shouldsleep = 1;	/* Can't send a 
> command; one already running */
>  		else if (!list_empty(&priv->cmdpendingq))
>  			shouldsleep = 0;	/* We have a 
> command to send */
>  		else if (__kfifo_len(priv->event_fifo))
>  			shouldsleep = 0;	/* We have an 
> event to process */


Hi,
I have experience a couple of timeout and resends and after 
sending another commands I get the response from the previous one, 
and the patch seems to solve a couple of those for me - 
still se some problem fetching packet from  firmware with ret = -22 though.
And it could be dependant on how much debug I have turned on..

But, shouldn't the above priv->event_fifo check move before
cur_cmd  as well?

I have no clue what kind of events we are talking about here and
if those can be processed independent of if a cmd is running or not,
just trigged on the comment.

Best regards
/Johan

RE: [PATCH] libertas: before sleeping, check for a command result

[Dan Williams]

On Fri, 2008-05-23 at 16:26 +0200, Johan Adolfsson wrote:
> > -----Original Message-----
> > From: libertas-dev-bounces@lists.infradead.org 
> > [mailto:libertas-dev-bounces@lists.infradead.org] On Behalf 
> > Of Holger Schurig
> > Sent: den 23 maj 2008 16:04
> > To: libertas-dev@lists.infradead.org; Dan Williams; 
> > linux-wireless@vger.kernel.org; John W. Linville
> > Subject: [PATCH] libertas: before sleeping, check for a command result
> > 
> > 
> > If we don't check for a command response early, but rather sleep,
> > then we might sleep despite an already-received command response.
> > This will lead to a command-timeout.
> > 
> > Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>
> > 
> > Index: wireless-testing/drivers/net/wireless/libertas/main.c
> > ===================================================================
> > --- 
> > wireless-testing.orig/drivers/net/wireless/libertas/main.c	
> > 2008-05-23 14:25:16.000000000 +0200
> > +++ wireless-testing/drivers/net/wireless/libertas/main.c	
> > 2008-05-23 14:25:51.000000000 +0200
> > @@ -722,14 +722,14 @@ static int lbs_thread(void *data)
> >  			shouldsleep = 1;	/* Something is 
> > en route to the device already */
> >  		else if (priv->tx_pending_len > 0)
> >  			shouldsleep = 0;	/* We've a 
> > packet to send */
> > +		else if (priv->resp_len[priv->resp_idx])
> > +			shouldsleep = 0;	/* We have a 
> > command response */
> >  		else if (priv->cur_cmd)
> >  			shouldsleep = 1;	/* Can't send a 
> > command; one already running */
> >  		else if (!list_empty(&priv->cmdpendingq))
> >  			shouldsleep = 0;	/* We have a 
> > command to send */
> >  		else if (__kfifo_len(priv->event_fifo))
> >  			shouldsleep = 0;	/* We have an 
> > event to process */
> 
> 
> Hi,
> I have experience a couple of timeout and resends and after 
> sending another commands I get the response from the previous one, 
> and the patch seems to solve a couple of those for me - 
> still se some problem fetching packet from  firmware with ret = -22 though.
> And it could be dependant on how much debug I have turned on..
> 
> But, shouldn't the above priv->event_fifo check move before
> cur_cmd  as well?

Most likely, yes...

> I have no clue what kind of events we are talking about here and
> if those can be processed independent of if a cmd is running or not,
> just trigged on the comment.

Anything that might be handled in lbs_process_event().  Link loss, MIC
failure, etc.

Dan

> Best regards
> /Johan
> 
> 

Re: [PATCH] libertas: before sleeping, check for a command result

[Holger Schurig]

> I have experience a couple of timeout and resends and after
> sending another commands I get the response from the previous
> one, and the patch seems to solve a couple of those for me -
> still se some problem fetching packet from  firmware with ret
> = -22 though. And it could be dependant on how much debug I
> have turned on..

Keep in mind that maybe the firmware actually really takes ages 
to complete a command. David Woodhouse reported this.

In my case, because I had the proper debugging statements in my 
interrupt handler, I knew for sure that I already received the 
response. Can you verify that, for the comment timeouts you 
noticed, if this did happen or not?


> But, shouldn't the above priv->event_fifo check move before
> cur_cmd  as well?

Maybe, my change to the thread was just a shot into the dark, I 
don't claim that I fully understand it. Maybe Dan or David can 
help you with this.

> I have no clue what kind of events we are talking about here
> and if those can be processed independent of if a cmd is
> running or not, just trigged on the comment.

The kfifo contains hostevents (e.g. link lost, deauthenticated) 
and command responses. Hmm, not sure if the latter is still 
true.

Re: [PATCH] libertas: before sleeping, check for a command result

[Holger Schurig]

The thing is: now I don't have the issue that I reported
in the first message of this thread anymore. Libertas
works as expected without the patch.

I'm on wireless-testing v2.6.26-rc3-8056-gc2c4044 and have the
following patches applied:

libertas-fix-lock.patch
libertas-fix-printk.patch
libertas-if-cs-speed-firmware.patch
libertas-if-cs-int.patch


Compared to friday, I also turned on the following kernel debug
options:

+CONFIG_DEBUG_KERNEL=y
+CONFIG_DETECT_SOFTLOCKUP=y
+CONFIG_DEBUG_PREEMPT=y
+CONFIG_DEBUG_RT_MUTEXES=y
+CONFIG_DEBUG_PI_LIST=y
+CONFIG_DEBUG_SPINLOCK=y
+CONFIG_DEBUG_MUTEXES=y
+CONFIG_DEBUG_LOCK_ALLOC=y
+CONFIG_PROVE_LOCKING=y
+CONFIG_LOCKDEP=y
+CONFIG_LOCK_STAT=y
+CONFIG_TRACE_IRQFLAGS=y
+CONFIG_DEBUG_SPINLOCK_SLEEP=y

Yet associating and executing of commands works fine. I
did run "while true; do iwconfig eth1; sleep 0.2; done"
quite long (up to sequence number 8804) and did not
have any hickup.

On friday, I even saw the hickup on the first commands/response ...

Re: [PATCH] libertas: before sleeping, check for a command result

[Johan Adolfsson]

----- Original Message ----- 
From: "Holger Schurig" <hs4233@mail.mn-solutions.de>
To: "Johan Adolfsson" <johana@axis.com>
Cc: <libertas-dev@lists.infradead.org>; "'Dan Williams'" <dcbw@redhat.com>; 
<linux-wireless@vger.kernel.org>; "'John W. Linville'" 
<linville@tuxdriver.com>
Sent: Friday, May 23, 2008 4:57 PM
Subject: Re: [PATCH] libertas: before sleeping, check for a command result


>> I have experience a couple of timeout and resends and after
>> sending another commands I get the response from the previous
>> one, and the patch seems to solve a couple of those for me -
>> still se some problem fetching packet from  firmware with ret
>> = -22 though. And it could be dependant on how much debug I
>> have turned on..
>
> Keep in mind that maybe the firmware actually really takes ages
> to complete a command. David Woodhouse reported this.
>
> In my case, because I had the proper debugging statements in my
> interrupt handler, I knew for sure that I already received the
> response. Can you verify that, for the comment timeouts you
> noticed, if this did happen or not?

While trying to find out what went wrong I believe I stumbled
on a cache issue since our sdio driver didn't invalidate the cache
and the DMA is not cachecoherent.

Possible something for others to check in their SDIO drivers
as well.
Ran into some weird problems when trying to verify if it solved my problem 
though, so I will get back with more info when I get it up and running 
again.

Best regards
/Johan

Re: [PATCH] libertas: before sleeping, check for a command result

[Dan Williams]

On Fri, 2008-05-23 at 16:04 +0200, Holger Schurig wrote:
> If we don't check for a command response early, but rather sleep,
> then we might sleep despite an already-received command response.
> This will lead to a command-timeout.
> 
> Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>

Acked-by: Dan Williams <dcbw@redhat.com>

> Index: wireless-testing/drivers/net/wireless/libertas/main.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/libertas/main.c	2008-05-23 14:25:16.000000000 +0200
> +++ wireless-testing/drivers/net/wireless/libertas/main.c	2008-05-23 14:25:51.000000000 +0200
> @@ -722,14 +722,14 @@ static int lbs_thread(void *data)
>  			shouldsleep = 1;	/* Something is en route to the device already */
>  		else if (priv->tx_pending_len > 0)
>  			shouldsleep = 0;	/* We've a packet to send */
> +		else if (priv->resp_len[priv->resp_idx])
> +			shouldsleep = 0;	/* We have a command response */
>  		else if (priv->cur_cmd)
>  			shouldsleep = 1;	/* Can't send a command; one already running */
>  		else if (!list_empty(&priv->cmdpendingq))
>  			shouldsleep = 0;	/* We have a command to send */
>  		else if (__kfifo_len(priv->event_fifo))
>  			shouldsleep = 0;	/* We have an event to process */
> -		else if (priv->resp_len[priv->resp_idx])
> -			shouldsleep = 0;	/* We have a command response */
>  		else
>  			shouldsleep = 1;	/* No command */
>