2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Keith Packard]

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


-rc2 was causing resume failures on my X201s -- the video mode would get
reset, but the system would then lock up. I bisected the failure down to:

commit b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Thu Mar 24 20:16:42 2011 +0100

    Bluetooth: Fix warning with hci_cmd_timer

Reverting this on top of -rc2 makes resume reliable again.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Thomas Gleixner]

On Wed, 6 Apr 2011, Keith Packard wrote:
> -rc2 was causing resume failures on my X201s -- the video mode would get
> reset, but the system would then lock up. I bisected the failure down to:
> 
> commit b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716
> Author: Thomas Gleixner <tglx@linutronix.de>
> Date:   Thu Mar 24 20:16:42 2011 +0100
> 
>     Bluetooth: Fix warning with hci_cmd_timer
> 
> Reverting this on top of -rc2 makes resume reliable again.

Huch. I really have a hard time to connect this. Can you revert the
two hunks seperately?

Thanks,

	tglx

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Thomas Gleixner]

On Thu, 7 Apr 2011, Thomas Gleixner wrote:

> On Wed, 6 Apr 2011, Keith Packard wrote:
> > -rc2 was causing resume failures on my X201s -- the video mode would get
> > reset, but the system would then lock up. I bisected the failure down to:
> > 
> > commit b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716
> > Author: Thomas Gleixner <tglx@linutronix.de>
> > Date:   Thu Mar 24 20:16:42 2011 +0100
> > 
> >     Bluetooth: Fix warning with hci_cmd_timer
> > 
> > Reverting this on top of -rc2 makes resume reliable again.
> 
> Huch. I really have a hard time to connect this. Can you revert the
> two hunks seperately?

Keith confirmed that reverting the second hunk fixes the
problem. Heck, so that timer is rearmed somewhere in the teardown
path?

Can the bluetooth folks please have a look at that ASAP? The obvious
fast fix for Linus tree is to revert the second hunk for now, but this
needs to be fixed proper.

Thanks,

	tglx

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Keith Packard]

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

On Fri, 8 Apr 2011 23:44:51 +0200 (CEST), Thomas Gleixner <tglx@linutronix.de> wrote:

> Can the bluetooth folks please have a look at that ASAP? The obvious
> fast fix for Linus tree is to revert the second hunk for now, but this
> needs to be fixed proper.

Who will submit this patch? I'd rather have your name on it so that
people come complain at you...

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Vinicius Costa Gomes]

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

Hi Keith,

On 16:13 Fri 08 Apr, Keith Packard wrote:
> On Fri, 8 Apr 2011 23:44:51 +0200 (CEST), Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > Can the bluetooth folks please have a look at that ASAP? The obvious
> > fast fix for Linus tree is to revert the second hunk for now, but this
> > needs to be fixed proper.
> 
> Who will submit this patch? I'd rather have your name on it so that
> people come complain at you...

I took a shot at it and just sent a patch (also attached for convenience) 
that should solve the problem.

> 
> -- 
> keith.packard@intel.com


Cheers,
-- 
Vinicius

[-- Attachment #2: 0001-Bluetooth-Fix-keeping-the-command-timer-running.patch --]
[-- Type: text/plain, Size: 1335 bytes --]

>From 1a617e2dde807ae932531ddd9b451011f236546c Mon Sep 17 00:00:00 2001
From: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
Date: Fri, 8 Apr 2011 20:33:17 -0300
Subject: [PATCH] Bluetooth: Fix keeping the command timer running

In the teardown path the reset command is sent to the controller,
this event causes the command timer to be reactivated.

Now we remove the timer after the reset command is sent.

Reported-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
 net/bluetooth/hci_core.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index a80bc1c..7467eaa 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 	hci_req_cancel(hdev, ENODEV);
 	hci_req_lock(hdev);
 
-	/* Stop timer, it might be running */
-	del_timer_sync(&hdev->cmd_timer);
-
 	if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
 		hci_req_unlock(hdev);
 		return 0;
@@ -618,6 +615,9 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 		clear_bit(HCI_INIT, &hdev->flags);
 	}
 
+	/* Stop timer, it might be running */
+	del_timer_sync(&hdev->cmd_timer);
+
 	/* Kill cmd task */
 	tasklet_kill(&hdev->cmd_task);
 
-- 
1.7.4.3

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Keith Packard]

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

On Fri, 8 Apr 2011 21:08:57 -0300, Vinicius Costa Gomes <vinicius.gomes@openbossa.org> wrote:

> I took a shot at it and just sent a patch (also attached for convenience) 
> that should solve the problem.

Thanks much! I applied this to -rc2 and it appears to work fine in some
brief testing (a dozen suspend/resume cycles)

> Reported-by: Keith Packard <keithp@keithp.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>

Tested-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Gustavo F. Padovan]

* Keith Packard <keithp@keithp.com> [2011-04-08 19:03:25 -0700]:

> On Fri, 8 Apr 2011 21:08:57 -0300, Vinicius Costa Gomes <vinicius.gomes@openbossa.org> wrote:
> 
> > I took a shot at it and just sent a patch (also attached for convenience) 
> > that should solve the problem.
> 
> Thanks much! I applied this to -rc2 and it appears to work fine in some
> brief testing (a dozen suspend/resume cycles)
> 
> > Reported-by: Keith Packard <keithp@keithp.com>
> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> 
> Tested-by: Keith Packard <keithp@keithp.com>

I applied the patch to bluetooth-2.6. Thank you all.


-- 
Gustavo F. Padovan
http://profusion.mobi

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Thomas Gleixner]

On Fri, 8 Apr 2011, Vinicius Costa Gomes wrote:

> Hi Keith,
> 
> On 16:13 Fri 08 Apr, Keith Packard wrote:
> > On Fri, 8 Apr 2011 23:44:51 +0200 (CEST), Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > Can the bluetooth folks please have a look at that ASAP? The obvious
> > > fast fix for Linus tree is to revert the second hunk for now, but this
> > > needs to be fixed proper.
> > 
> > Who will submit this patch? I'd rather have your name on it so that
> > people come complain at you...
> 
> I took a shot at it and just sent a patch (also attached for convenience) 
> that should solve the problem.

Aaarg. No. That patch reverts both hunks.

--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 	hci_req_cancel(hdev, ENODEV);
 	hci_req_lock(hdev);
 
-	/* Stop timer, it might be running */
-	del_timer_sync(&hdev->cmd_timer);
-
 	if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
 		hci_req_unlock(hdev);
 		return 0;

As I said before you need that first hunk to stay for the case where
there is no device up and you return via the !HCI_UP check. You just
moved back to the state before as the stupid timer is active for
whatever reason even when HCI_UP is not set.

@@ -618,6 +615,9 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 		clear_bit(HCI_INIT, &hdev->flags);
 	}
 
+	/* Stop timer, it might be running */
+	del_timer_sync(&hdev->cmd_timer);
+
 	/* Kill cmd task */
 	tasklet_kill(&hdev->cmd_task);

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Thomas Gleixner]

On Mon, 11 Apr 2011, Gustavo F. Padovan wrote:

> * Keith Packard <keithp@keithp.com> [2011-04-08 19:03:25 -0700]:
> 
> > On Fri, 8 Apr 2011 21:08:57 -0300, Vinicius Costa Gomes <vinicius.gomes@openbossa.org> wrote:
> > 
> > > I took a shot at it and just sent a patch (also attached for convenience) 
> > > that should solve the problem.
> > 
> > Thanks much! I applied this to -rc2 and it appears to work fine in some
> > brief testing (a dozen suspend/resume cycles)
> > 
> > > Reported-by: Keith Packard <keithp@keithp.com>
> > > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> > 
> > Tested-by: Keith Packard <keithp@keithp.com>
> 
> I applied the patch to bluetooth-2.6. Thank you all.

Sigh. Is actually anyone trying to read what I wrote ?

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Alan Cox]

> I took a shot at it and just sent a patch (also attached for convenience) 
> that should solve the problem.

Well its reverting too much but its also looking pretty bogus to me

-	/* Stop timer, it might be running */
-	del_timer_sync(&hdev->cmd_timer);
-
 	if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
 		hci_req_unlock(hdev);
 		return 0;

So you've now got a path where you leave the timer running and didn't
before - not it appears one that is a good idea.

Certainly not the kind of change that should be considered for a
regression fix for an rc kernel. It's far too big a behavioural change to
be safe.

Alan

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Vinicius Gomes]

Hi Gustavo,

On Mon, Apr 11, 2011 at 1:34 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> I took a shot at it and just sent a patch (also attached for convenience)
>> that should solve the problem.
>
> Well its reverting too much but its also looking pretty bogus to me
>
> -       /* Stop timer, it might be running */
> -       del_timer_sync(&hdev->cmd_timer);
> -
>        if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
>                hci_req_unlock(hdev);
>                return 0;
>
> So you've now got a path where you leave the timer running and didn't
> before - not it appears one that is a good idea.
>
> Certainly not the kind of change that should be considered for a
> regression fix for an rc kernel. It's far too big a behavioural change to
> be safe.

Considering what was said and that this patch didn't hit your public tree yet,
I guess that this patch should be ignored.

Will send a proper fix soon. Thanks all.

>
> Alan
>


Cheers,
-- 
Vinicius

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Marcel Holtmann]

Hi Thomas,

> > > > Can the bluetooth folks please have a look at that ASAP? The obvious
> > > > fast fix for Linus tree is to revert the second hunk for now, but this
> > > > needs to be fixed proper.
> > > 
> > > Who will submit this patch? I'd rather have your name on it so that
> > > people come complain at you...
> > 
> > I took a shot at it and just sent a patch (also attached for convenience) 
> > that should solve the problem.
> 
> Aaarg. No. That patch reverts both hunks.
> 
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
>  	hci_req_cancel(hdev, ENODEV);
>  	hci_req_lock(hdev);
>  
> -	/* Stop timer, it might be running */
> -	del_timer_sync(&hdev->cmd_timer);
> -
>  	if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
>  		hci_req_unlock(hdev);
>  		return 0;
> 
> As I said before you need that first hunk to stay for the case where
> there is no device up and you return via the !HCI_UP check. You just
> moved back to the state before as the stupid timer is active for
> whatever reason even when HCI_UP is not set.

if I read this right then we have the case that we arm this timer for no
real reason. A device in !HCI_UP should have nothing running. Certainly
not the cmd_timer since it will never process any commands.

According to Gustavo, the problem is really in the hci_reset logic were
we arm the timer even when shutting down the device.

Regards

Marcel

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Thomas Gleixner]

On Mon, 11 Apr 2011, Marcel Holtmann wrote:

> Hi Thomas,
> 
> > > > > Can the bluetooth folks please have a look at that ASAP? The obvious
> > > > > fast fix for Linus tree is to revert the second hunk for now, but this
> > > > > needs to be fixed proper.
> > > > 
> > > > Who will submit this patch? I'd rather have your name on it so that
> > > > people come complain at you...
> > > 
> > > I took a shot at it and just sent a patch (also attached for convenience) 
> > > that should solve the problem.
> > 
> > Aaarg. No. That patch reverts both hunks.
> > 
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> >  	hci_req_cancel(hdev, ENODEV);
> >  	hci_req_lock(hdev);
> >  
> > -	/* Stop timer, it might be running */
> > -	del_timer_sync(&hdev->cmd_timer);
> > -
> >  	if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> >  		hci_req_unlock(hdev);
> >  		return 0;
> > 
> > As I said before you need that first hunk to stay for the case where
> > there is no device up and you return via the !HCI_UP check. You just
> > moved back to the state before as the stupid timer is active for
> > whatever reason even when HCI_UP is not set.
> 
> if I read this right then we have the case that we arm this timer for no
> real reason. A device in !HCI_UP should have nothing running. Certainly
> not the cmd_timer since it will never process any commands.
> 
> According to Gustavo, the problem is really in the hci_reset logic were
> we arm the timer even when shutting down the device.

The reason why the original patch was sent is, that the timer was
running when the thing went out via the !HCI_UP path, which caused the
whole thing to explode in the first place. I had no time to figure out
why, but moving the del_timer_sync above the
if (!test_and_clear_bit(HCI_UP, &hdev->flags)) solved it.

Thanks,

	tglx

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Thomas Gleixner]

On Tue, 12 Apr 2011, Thomas Gleixner wrote:
> On Mon, 11 Apr 2011, Marcel Holtmann wrote:
> 
> > Hi Thomas,
> > 
> > > > > > Can the bluetooth folks please have a look at that ASAP? The obvious
> > > > > > fast fix for Linus tree is to revert the second hunk for now, but this
> > > > > > needs to be fixed proper.
> > > > > 
> > > > > Who will submit this patch? I'd rather have your name on it so that
> > > > > people come complain at you...
> > > > 
> > > > I took a shot at it and just sent a patch (also attached for convenience) 
> > > > that should solve the problem.
> > > 
> > > Aaarg. No. That patch reverts both hunks.
> > > 
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> > >  	hci_req_cancel(hdev, ENODEV);
> > >  	hci_req_lock(hdev);
> > >  
> > > -	/* Stop timer, it might be running */
> > > -	del_timer_sync(&hdev->cmd_timer);
> > > -
> > >  	if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> > >  		hci_req_unlock(hdev);
> > >  		return 0;
> > > 
> > > As I said before you need that first hunk to stay for the case where
> > > there is no device up and you return via the !HCI_UP check. You just
> > > moved back to the state before as the stupid timer is active for
> > > whatever reason even when HCI_UP is not set.
> > 
> > if I read this right then we have the case that we arm this timer for no
> > real reason. A device in !HCI_UP should have nothing running. Certainly
> > not the cmd_timer since it will never process any commands.
> > 
> > According to Gustavo, the problem is really in the hci_reset logic were
> > we arm the timer even when shutting down the device.
> 
> The reason why the original patch was sent is, that the timer was
> running when the thing went out via the !HCI_UP path, which caused the
> whole thing to explode in the first place. I had no time to figure out
> why, but moving the del_timer_sync above the
> if (!test_and_clear_bit(HCI_UP, &hdev->flags)) solved it.

Oops. Hit send too fast.

Then it broke the resume on Keith machine and reverting just the hunk
which disarms the timer in the 

        if (hdev->sent_cmd) {

path made both scenarios working. So there are two problems:

     1) Why do we need the del_timer_sync() above the !HCI_UP check

     2) Why gets the timer rearmed after that

Thanks,

	tglx

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Gustavo F. Padovan]

* Thomas Gleixner <tglx@linutronix.de> [2011-04-12 00:19:32 +0200]:

> On Tue, 12 Apr 2011, Thomas Gleixner wrote:
> > On Mon, 11 Apr 2011, Marcel Holtmann wrote:
> > 
> > > Hi Thomas,
> > > 
> > > > > > > Can the bluetooth folks please have a look at that ASAP? The obvious
> > > > > > > fast fix for Linus tree is to revert the second hunk for now, but this
> > > > > > > needs to be fixed proper.
> > > > > > 
> > > > > > Who will submit this patch? I'd rather have your name on it so that
> > > > > > people come complain at you...
> > > > > 
> > > > > I took a shot at it and just sent a patch (also attached for convenience) 
> > > > > that should solve the problem.
> > > > 
> > > > Aaarg. No. That patch reverts both hunks.
> > > > 
> > > > --- a/net/bluetooth/hci_core.c
> > > > +++ b/net/bluetooth/hci_core.c
> > > > @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> > > >  	hci_req_cancel(hdev, ENODEV);
> > > >  	hci_req_lock(hdev);
> > > >  
> > > > -	/* Stop timer, it might be running */
> > > > -	del_timer_sync(&hdev->cmd_timer);
> > > > -
> > > >  	if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> > > >  		hci_req_unlock(hdev);
> > > >  		return 0;
> > > > 
> > > > As I said before you need that first hunk to stay for the case where
> > > > there is no device up and you return via the !HCI_UP check. You just
> > > > moved back to the state before as the stupid timer is active for
> > > > whatever reason even when HCI_UP is not set.
> > > 
> > > if I read this right then we have the case that we arm this timer for no
> > > real reason. A device in !HCI_UP should have nothing running. Certainly
> > > not the cmd_timer since it will never process any commands.
> > > 
> > > According to Gustavo, the problem is really in the hci_reset logic were
> > > we arm the timer even when shutting down the device.
> > 
> > The reason why the original patch was sent is, that the timer was
> > running when the thing went out via the !HCI_UP path, which caused the
> > whole thing to explode in the first place. I had no time to figure out
> > why, but moving the del_timer_sync above the
> > if (!test_and_clear_bit(HCI_UP, &hdev->flags)) solved it.
> 
> Oops. Hit send too fast.
> 
> Then it broke the resume on Keith machine and reverting just the hunk
> which disarms the timer in the 
> 
>         if (hdev->sent_cmd) {
> 
> path made both scenarios working. So there are two problems:
> 
>      1) Why do we need the del_timer_sync() above the !HCI_UP check

That is still a mysterious to me, the real bug the hiding here. I'm trying to
track this down but no luck yet.

> 
>      2) Why gets the timer rearmed after that

It is armed at each HCI command we send. In the close path we send out an HCI
RESET command that rearms it.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Gustavo F. Padovan]

* Gustavo F. Padovan <padovan@profusion.mobi> [2011-04-11 19:25:04 -0300]:

> * Thomas Gleixner <tglx@linutronix.de> [2011-04-12 00:19:32 +0200]:
> 
> > On Tue, 12 Apr 2011, Thomas Gleixner wrote:
> > > On Mon, 11 Apr 2011, Marcel Holtmann wrote:
> > > 
> > > > Hi Thomas,
> > > > 
> > > > > > > > Can the bluetooth folks please have a look at that ASAP? The obvious
> > > > > > > > fast fix for Linus tree is to revert the second hunk for now, but this
> > > > > > > > needs to be fixed proper.
> > > > > > > 
> > > > > > > Who will submit this patch? I'd rather have your name on it so that
> > > > > > > people come complain at you...
> > > > > > 
> > > > > > I took a shot at it and just sent a patch (also attached for convenience) 
> > > > > > that should solve the problem.
> > > > > 
> > > > > Aaarg. No. That patch reverts both hunks.
> > > > > 
> > > > > --- a/net/bluetooth/hci_core.c
> > > > > +++ b/net/bluetooth/hci_core.c
> > > > > @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> > > > >  	hci_req_cancel(hdev, ENODEV);
> > > > >  	hci_req_lock(hdev);
> > > > >  
> > > > > -	/* Stop timer, it might be running */
> > > > > -	del_timer_sync(&hdev->cmd_timer);
> > > > > -
> > > > >  	if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> > > > >  		hci_req_unlock(hdev);
> > > > >  		return 0;
> > > > > 
> > > > > As I said before you need that first hunk to stay for the case where
> > > > > there is no device up and you return via the !HCI_UP check. You just
> > > > > moved back to the state before as the stupid timer is active for
> > > > > whatever reason even when HCI_UP is not set.
> > > > 
> > > > if I read this right then we have the case that we arm this timer for no
> > > > real reason. A device in !HCI_UP should have nothing running. Certainly
> > > > not the cmd_timer since it will never process any commands.
> > > > 
> > > > According to Gustavo, the problem is really in the hci_reset logic were
> > > > we arm the timer even when shutting down the device.
> > > 
> > > The reason why the original patch was sent is, that the timer was
> > > running when the thing went out via the !HCI_UP path, which caused the
> > > whole thing to explode in the first place. I had no time to figure out
> > > why, but moving the del_timer_sync above the
> > > if (!test_and_clear_bit(HCI_UP, &hdev->flags)) solved it.
> > 
> > Oops. Hit send too fast.
> > 
> > Then it broke the resume on Keith machine and reverting just the hunk
> > which disarms the timer in the 
> > 
> >         if (hdev->sent_cmd) {
> > 
> > path made both scenarios working. So there are two problems:
> > 
> >      1) Why do we need the del_timer_sync() above the !HCI_UP check
> 
> That is still a mysterious to me, the real bug the hiding here. I'm trying to
> track this down but no luck yet.
> 
> > 
> >      2) Why gets the timer rearmed after that
> 
> It is armed at each HCI command we send. In the close path we send out an HCI
> RESET command that rearms it.

I applied v2 patch from Vinícius that fix all the symptoms. Now we have more time
to find the real cause of this bug. However I still have no idea, I'm not able
to reproduce it.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Gustavo F. Padovan]

* Gustavo F. Padovan <padovan@profusion.mobi> [2011-04-11 19:25:04 -0300]:

> * Thomas Gleixner <tglx@linutronix.de> [2011-04-12 00:19:32 +0200]:
>=20
> > On Tue, 12 Apr 2011, Thomas Gleixner wrote:
> > > On Mon, 11 Apr 2011, Marcel Holtmann wrote:
> > >=20
> > > > Hi Thomas,
> > > >=20
> > > > > > > > Can the bluetooth folks please have a look at that ASAP? Th=
e obvious
> > > > > > > > fast fix for Linus tree is to revert the second hunk for no=
w, but this
> > > > > > > > needs to be fixed proper.
> > > > > > >=20
> > > > > > > Who will submit this patch? I'd rather have your name on it s=
o that
> > > > > > > people come complain at you...
> > > > > >=20
> > > > > > I took a shot at it and just sent a patch (also attached for co=
nvenience)=20
> > > > > > that should solve the problem.
> > > > >=20
> > > > > Aaarg. No. That patch reverts both hunks.
> > > > >=20
> > > > > --- a/net/bluetooth/hci_core.c
> > > > > +++ b/net/bluetooth/hci_core.c
> > > > > @@ -586,9 +586,6 @@ static int hci_dev_do_close(struct hci_dev *h=
dev)
> > > > >  	hci_req_cancel(hdev, ENODEV);
> > > > >  	hci_req_lock(hdev);
> > > > > =20
> > > > > -	/* Stop timer, it might be running */
> > > > > -	del_timer_sync(&hdev->cmd_timer);
> > > > > -
> > > > >  	if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> > > > >  		hci_req_unlock(hdev);
> > > > >  		return 0;
> > > > >=20
> > > > > As I said before you need that first hunk to stay for the case wh=
ere
> > > > > there is no device up and you return via the !HCI_UP check. You j=
ust
> > > > > moved back to the state before as the stupid timer is active for
> > > > > whatever reason even when HCI_UP is not set.
> > > >=20
> > > > if I read this right then we have the case that we arm this timer f=
or no
> > > > real reason. A device in !HCI_UP should have nothing running. Certa=
inly
> > > > not the cmd_timer since it will never process any commands.
> > > >=20
> > > > According to Gustavo, the problem is really in the hci_reset logic =
were
> > > > we arm the timer even when shutting down the device.
> > >=20
> > > The reason why the original patch was sent is, that the timer was
> > > running when the thing went out via the !HCI_UP path, which caused the
> > > whole thing to explode in the first place. I had no time to figure out
> > > why, but moving the del_timer_sync above the
> > > if (!test_and_clear_bit(HCI_UP, &hdev->flags)) solved it.
> >=20
> > Oops. Hit send too fast.
> >=20
> > Then it broke the resume on Keith machine and reverting just the hunk
> > which disarms the timer in the=20
> >=20
> >         if (hdev->sent_cmd) {
> >=20
> > path made both scenarios working. So there are two problems:
> >=20
> >      1) Why do we need the del_timer_sync() above the !HCI_UP check
>=20
> That is still a mysterious to me, the real bug the hiding here. I'm tryin=
g to
> track this down but no luck yet.
>=20
> >=20
> >      2) Why gets the timer rearmed after that
>=20
> It is armed at each HCI command we send. In the close path we send out an=
 HCI
> RESET command that rearms it.

I applied v2 patch from Vin=EDcius that fix all the symptoms. Now we have m=
ore time
to find the real cause of this bug. However I still have no idea, I'm not a=
ble
to reproduce it.

--=20
Gustavo F. Padovan
http://profusion.mobi

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 557 bytes --]

On Tue, 12 Apr 2011, Gustavo F. Padovan wrote:
> * Gustavo F. Padovan <padovan@profusion.mobi> [2011-04-11 19:25:04 -0300]:
> > * Thomas Gleixner <tglx@linutronix.de> [2011-04-12 00:19:32 +0200]:
> > It is armed at each HCI command we send. In the close path we send out an HCI
> > RESET command that rearms it.
> 
> I applied v2 patch from Vinícius that fix all the symptoms. Now we have more time
> to find the real cause of this bug. However I still have no idea, I'm not able
> to reproduce it.

You might poke Ingo. He had a reproducer.

Thanks,

	tglx

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Keith Packard]

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

On Mon, 11 Apr 2011 13:59:23 -0300, Vinicius Gomes <vinicius.gomes@openbossa.org> wrote:

> Will send a proper fix soon. Thanks all.

This is still broken in -rc4, I'd love to see a proper patch soon --
this will affect any machine with a bluetooth device.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Gustavo F. Padovan]

Hi Keith,

* Keith Packard <keithp@keithp.com> [2011-04-21 14:50:32 -0700]:

> On Mon, 11 Apr 2011 13:59:23 -0300, Vinicius Gomes <vinicius.gomes@openbossa.org> wrote:
> 
> > Will send a proper fix soon. Thanks all.
> 
> This is still broken in -rc4, I'd love to see a proper patch soon --
> this will affect any machine with a bluetooth device.

Patch is on the way to mainline. It's currently on wireless-2.6. Next steps
are net-2.6 -> linux-2.6.

Regards,

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Keith Packard]

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


On Thu, 21 Apr 2011 19:59:53 -0300, "Gustavo F. Padovan" <padovan@profusion.mobi> wrote:

> Patch is on the way to mainline. It's currently on wireless-2.6. Next steps
> are net-2.6 -> linux-2.6.

I didn't see it on linux-kernel, so I was a bit worried. I'd love to
test it in place of the kludge that Vinicius sent me.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Gustavo F. Padovan]

* Keith Packard <keithp@keithp.com> [2011-04-21 17:04:51 -0700]:

> 
> On Thu, 21 Apr 2011 19:59:53 -0300, "Gustavo F. Padovan" <padovan@profusion.mobi> wrote:
> 
> > Patch is on the way to mainline. It's currently on wireless-2.6. Next steps
> > are net-2.6 -> linux-2.6.
> 
> I didn't see it on linux-kernel, so I was a bit worried. I'd love to
> test it in place of the kludge that Vinicius sent me.

I usually don't cc linux-kernel in my pull-requests, maybe I should.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Stefan Seyfried]

On Sat, 23 Apr 2011 13:37:59 -0300
"Gustavo F. Padovan" <padovan@profusion.mobi> wrote:

> * Keith Packard <keithp@keithp.com> [2011-04-21 17:04:51 -0700]:
> 
> > 
> > On Thu, 21 Apr 2011 19:59:53 -0300, "Gustavo F. Padovan" <padovan@profusion.mobi> wrote:
> > 
> > > Patch is on the way to mainline. It's currently on wireless-2.6. Next steps
> > > are net-2.6 -> linux-2.6.
> > 
> > I didn't see it on linux-kernel, so I was a bit worried. I'd love to
> > test it in place of the kludge that Vinicius sent me.
> 
> I usually don't cc linux-kernel in my pull-requests, maybe I should.

Did this hit mainline already? I seem to suffer from this still with -rc5
-- 
Stefan Seyfried

"Dispatch war rocket Ajax to bring back his body!"

Re: 2.6.39-rc2 regression: X201s fails to resume b77dcf8460ae57d4eb9fd3633eb4f97b8fb20716

[Keith Packard]

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

On Tue, 3 May 2011 09:27:34 +0200, Stefan Seyfried <stefan.seyfried@googlemail.com> wrote:

> Did this hit mainline already? I seem to suffer from this still with
> -rc5

It's on master, but not in -rc5:

commit b79f44c16a4e2181b1d6423afe746745d5e949ff
Author: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
Date:   Mon Apr 11 18:46:55 2011 -0300

    Bluetooth: Fix keeping the command timer running
    
    In the teardown path the reset command is sent to the controller,
    this event causes the command timer to be reactivated.
    
    So the timer is removed in two situations, when the adapter isn't
    marked as UP and when we know that some command has been sent.
    
    Reported-by: Keith Packard <keithp@keithp.com>
    Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
    Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>

I'll be testing this patch shortly.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]