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 <[email protected]>
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 */
> 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.
> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf
> Of Holger Schurig
> Sent: den 23 maj 2008 16:04
> To: [email protected]; Dan Williams;
> [email protected]; 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 <[email protected]>
>
> 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
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 ...
----- Original Message -----
From: "Holger Schurig" <[email protected]>
To: "Johan Adolfsson" <[email protected]>
Cc: <[email protected]>; "'Dan Williams'" <[email protected]>;
<[email protected]>; "'John W. Linville'"
<[email protected]>
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
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 <[email protected]>
Acked-by: Dan Williams <[email protected]>
> 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 */
>
On Fri, 2008-05-23 at 16:26 +0200, Johan Adolfsson wrote:
> > -----Original Message-----
> > From: [email protected]
> > [mailto:[email protected]] On Behalf
> > Of Holger Schurig
> > Sent: den 23 maj 2008 16:04
> > To: [email protected]; Dan Williams;
> > [email protected]; 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 <[email protected]>
> >
> > 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
>
>