btusb suspend/resume bug...

btusb suspend/resume bug...

[David Miller]

Marcel, others, please bring some kind of closure to this
regression list entry:

Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=11442
Subject		: btusb hibernation/suspend breakage in current -git
Submitter	: Rafael J. Wysocki <rjw@sisk.pl>
Date		: 2008-08-25 11:37 (19 days old)
References	: http://marc.info/?l=linux-bluetooth&m=121966402012074&w=4
Handled-By	: Oliver Neukum <oliver@neukum.org>
Patch		: http://marc.info/?l=linux-bluetooth&m=121967226027323&w=4

There is a patch, it is tested, so the only course of action at
this point is to merge the fix or declare that this really isn't
a regression.

My impression is the later, because the driver btusb is replacing
doesn't handle suspend/resume either.  Isn't that right?

Re: btusb suspend/resume bug...

[Marcel Holtmann]

Hi Dave,

> Marcel, others, please bring some kind of closure to this
> regression list entry:
> 
> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=11442
> Subject		: btusb hibernation/suspend breakage in current -git
> Submitter	: Rafael J. Wysocki <rjw@sisk.pl>
> Date		: 2008-08-25 11:37 (19 days old)
> References	: http://marc.info/?l=linux-bluetooth&m=121966402012074&w=4
> Handled-By	: Oliver Neukum <oliver@neukum.org>
> Patch		: http://marc.info/?l=linux-bluetooth&m=121967226027323&w=4
> 
> There is a patch, it is tested, so the only course of action at
> this point is to merge the fix or declare that this really isn't
> a regression.
> 
> My impression is the later, because the driver btusb is replacing
> doesn't handle suspend/resume either.  Isn't that right?

the original patch that I had was expecting changes in the USB subsystem
that I deemed to much at this point. However Oliver got a patch that
would make it work without the USB changes. I am still testing it.

Let me see if I get some free minutes during the PlumbersConf to get
this fully tested.

Actually hci_usb is handling suspend/resume to some degree. So we could
count this as regression.

Regards

Marcel

Re: btusb suspend/resume bug...

[Marcel Holtmann]

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

Hi Dave,

> > Marcel, others, please bring some kind of closure to this
> > regression list entry:
> > 
> > Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=11442
> > Subject		: btusb hibernation/suspend breakage in current -git
> > Submitter	: Rafael J. Wysocki <rjw@sisk.pl>
> > Date		: 2008-08-25 11:37 (19 days old)
> > References	: http://marc.info/?l=linux-bluetooth&m=121966402012074&w=4
> > Handled-By	: Oliver Neukum <oliver@neukum.org>
> > Patch		: http://marc.info/?l=linux-bluetooth&m=121967226027323&w=4
> > 
> > There is a patch, it is tested, so the only course of action at
> > this point is to merge the fix or declare that this really isn't
> > a regression.
> > 
> > My impression is the later, because the driver btusb is replacing
> > doesn't handle suspend/resume either.  Isn't that right?
> 
> the original patch that I had was expecting changes in the USB subsystem
> that I deemed to much at this point. However Oliver got a patch that
> would make it work without the USB changes. I am still testing it.
> 
> Let me see if I get some free minutes during the PlumbersConf to get
> this fully tested.

so I took the patch apart and actually found a few more issues. I am not
sure if they should be applied this late in -rc phase.

Rafael, can you pull from my tree and test the changes:

git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-2.6.git

It would be interesting if these fixes are enough. Or if you need the
attached suspend/resume patch.

Regards

Marcel



[-- Attachment #2: patch-btusb-suspend --]
[-- Type: text/x-patch, Size: 2026 bytes --]

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 6e455ac..0e9451b 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -193,6 +193,7 @@ struct btusb_data {
 	struct usb_endpoint_descriptor *isoc_rx_ep;
 
 	int isoc_altsetting;
+	int suspend_count;
 };
 
 static void btusb_intr_complete(struct urb *urb)
@@ -946,10 +947,71 @@ static void btusb_disconnect(struct usb_interface *intf)
 	hci_free_dev(hdev);
 }
 
+static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct btusb_data *data = usb_get_intfdata(intf);
+
+	BT_DBG("intf %p", intf);
+
+	if (data->suspend_count++)
+		return 0;
+
+	cancel_work_sync(&data->work);
+
+	usb_kill_anchored_urbs(&data->tx_anchor);
+
+	usb_kill_anchored_urbs(&data->isoc_anchor);
+	usb_kill_anchored_urbs(&data->bulk_anchor);
+	usb_kill_anchored_urbs(&data->intr_anchor);
+
+	return 0;
+}
+
+static int btusb_resume(struct usb_interface *intf)
+{
+	struct btusb_data *data = usb_get_intfdata(intf);
+	struct hci_dev *hdev = data->hdev;
+	int err;
+
+	BT_DBG("intf %p", intf);
+
+	if (--data->suspend_count)
+		return 0;
+
+	if (!test_bit(HCI_RUNNING, &hdev->flags))
+		return 0;
+
+	if (test_bit(BTUSB_INTR_RUNNING, &data->flags)) {
+		err = btusb_submit_intr_urb(hdev, GFP_NOIO);
+		if (err < 0) {
+			clear_bit(BTUSB_INTR_RUNNING, &data->flags);
+			return err;
+		}
+	}
+
+	if (test_bit(BTUSB_BULK_RUNNING, &data->flags)) {
+		if (btusb_submit_bulk_urb(hdev, GFP_NOIO) < 0)
+			clear_bit(BTUSB_BULK_RUNNING, &data->flags);
+		else
+			btusb_submit_bulk_urb(hdev, GFP_NOIO);
+	}
+
+	if (test_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
+		if (btusb_submit_isoc_urb(hdev, GFP_NOIO) < 0)
+			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+		else
+			btusb_submit_isoc_urb(hdev, GFP_NOIO);
+	}
+
+	return 0;
+}
+
 static struct usb_driver btusb_driver = {
 	.name		= "btusb",
 	.probe		= btusb_probe,
 	.disconnect	= btusb_disconnect,
+	.suspend	= btusb_suspend,
+	.resume		= btusb_resume,
 	.id_table	= btusb_table,
 };
 

Re: btusb suspend/resume bug...

[Rafael J. Wysocki]

On Monday, 22 of September 2008, Marcel Holtmann wrote:
> Hi Dave,
> 
> > > Marcel, others, please bring some kind of closure to this
> > > regression list entry:
> > > 
> > > Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=11442
> > > Subject		: btusb hibernation/suspend breakage in current -git
> > > Submitter	: Rafael J. Wysocki <rjw@sisk.pl>
> > > Date		: 2008-08-25 11:37 (19 days old)
> > > References	: http://marc.info/?l=linux-bluetooth&m=121966402012074&w=4
> > > Handled-By	: Oliver Neukum <oliver@neukum.org>
> > > Patch		: http://marc.info/?l=linux-bluetooth&m=121967226027323&w=4
> > > 
> > > There is a patch, it is tested, so the only course of action at
> > > this point is to merge the fix or declare that this really isn't
> > > a regression.
> > > 
> > > My impression is the later, because the driver btusb is replacing
> > > doesn't handle suspend/resume either.  Isn't that right?
> > 
> > the original patch that I had was expecting changes in the USB subsystem
> > that I deemed to much at this point. However Oliver got a patch that
> > would make it work without the USB changes. I am still testing it.
> > 
> > Let me see if I get some free minutes during the PlumbersConf to get
> > this fully tested.
> 
> so I took the patch apart and actually found a few more issues. I am not
> sure if they should be applied this late in -rc phase.
> 
> Rafael, can you pull from my tree and test the changes:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-2.6.git
> 
> It would be interesting if these fixes are enough.

They appear to be enough.  I haven't had any suspend/resume failures with them
applied.

Thanks,
Rafael

Re: btusb suspend/resume bug...

[Marcel Holtmann]

Hi Rafael,

>>>> Marcel, others, please bring some kind of closure to this
>>>> regression list entry:
>>>>
>>>> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=11442
>>>> Subject		: btusb hibernation/suspend breakage in current -git
>>>> Submitter	: Rafael J. Wysocki <rjw@sisk.pl>
>>>> Date		: 2008-08-25 11:37 (19 days old)
>>>> References	: http://marc.info/?l=linux-bluetooth&m=121966402012074&w=4
>>>> Handled-By	: Oliver Neukum <oliver@neukum.org>
>>>> Patch		: http://marc.info/?l=linux-bluetooth&m=121967226027323&w=4
>>>>
>>>> There is a patch, it is tested, so the only course of action at
>>>> this point is to merge the fix or declare that this really isn't
>>>> a regression.
>>>>
>>>> My impression is the later, because the driver btusb is replacing
>>>> doesn't handle suspend/resume either.  Isn't that right?
>>>
>>> the original patch that I had was expecting changes in the USB  
>>> subsystem
>>> that I deemed to much at this point. However Oliver got a patch that
>>> would make it work without the USB changes. I am still testing it.
>>>
>>> Let me see if I get some free minutes during the PlumbersConf to get
>>> this fully tested.
>>
>> so I took the patch apart and actually found a few more issues. I  
>> am not
>> sure if they should be applied this late in -rc phase.
>>
>> Rafael, can you pull from my tree and test the changes:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/ 
>> bluetooth-2.6.git
>>
>> It would be interesting if these fixes are enough.
>
> They appear to be enough.  I haven't had any suspend/resume failures  
> with them
> applied.

so it works _without_ applying patch-btusb-suspend. That would be  
great news and mean that there is no bug inside the USB and/or  
Bluetooth subsystem. It means we just have a  bug when handling the  
multiple interfaces. And that makes actually sense.

Dave, I will prepare a minimal patch set for you today or tomorrow.

Regards

Marcel

Re: btusb suspend/resume bug...

[David Miller]

From: Marcel Holtmann <marcel@holtmann.org>
Date: Tue, 23 Sep 2008 01:32:05 +0200

> Dave, I will prepare a minimal patch set for you today or tomorrow.

Thanks.

Re: [Bug 11442] btusb suspend/resume bug...

[Rafael J. Wysocki]

On Tuesday, 23 of September 2008, Marcel Holtmann wrote:
> Hi Rafael,
> 
> >>>> Marcel, others, please bring some kind of closure to this
> >>>> regression list entry:
> >>>>
> >>>> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=11442
> >>>> Subject		: btusb hibernation/suspend breakage in current -git
> >>>> Submitter	: Rafael J. Wysocki <rjw@sisk.pl>
> >>>> Date		: 2008-08-25 11:37 (19 days old)
> >>>> References	: http://marc.info/?l=linux-bluetooth&m=121966402012074&w=4
> >>>> Handled-By	: Oliver Neukum <oliver@neukum.org>
> >>>> Patch		: http://marc.info/?l=linux-bluetooth&m=121967226027323&w=4
> >>>>
> >>>> There is a patch, it is tested, so the only course of action at
> >>>> this point is to merge the fix or declare that this really isn't
> >>>> a regression.
> >>>>
> >>>> My impression is the later, because the driver btusb is replacing
> >>>> doesn't handle suspend/resume either.  Isn't that right?
> >>>
> >>> the original patch that I had was expecting changes in the USB  
> >>> subsystem
> >>> that I deemed to much at this point. However Oliver got a patch that
> >>> would make it work without the USB changes. I am still testing it.
> >>>
> >>> Let me see if I get some free minutes during the PlumbersConf to get
> >>> this fully tested.
> >>
> >> so I took the patch apart and actually found a few more issues. I  
> >> am not
> >> sure if they should be applied this late in -rc phase.
> >>
> >> Rafael, can you pull from my tree and test the changes:
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/ 
> >> bluetooth-2.6.git
> >>
> >> It would be interesting if these fixes are enough.
> >
> > They appear to be enough.  I haven't had any suspend/resume failures  
> > with them
> > applied.
> 
> so it works _without_ applying patch-btusb-suspend.

Well, unfortunately I spoke too soon.

I'm still seeing post-hibernation crashes triggered by the bluetooth user land
trying to use the device handled by btusb.  They happen every second
hibernation, more or less, and apparently they are oopses in various code
paths not directly related to bluetooth, like ext3 (memory corruption or
what?).

With patch-btusb-suspend applied I don't see them (actually I have to use
a slightly modified version of the patch which is appended).

Interestingly enough, suspend to RAM works without any visible problems.

---
 drivers/bluetooth/btusb.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

Index: linux-2.6/drivers/bluetooth/btusb.c
===================================================================
--- linux-2.6.orig/drivers/bluetooth/btusb.c
+++ linux-2.6/drivers/bluetooth/btusb.c
@@ -193,6 +193,7 @@ struct btusb_data {
 	struct usb_endpoint_descriptor *isoc_rx_ep;
 
 	int isoc_altsetting;
+	int suspend_count;
 };
 
 static void btusb_intr_complete(struct urb *urb)
@@ -947,10 +948,71 @@ static void btusb_disconnect(struct usb_
 	hci_free_dev(hdev);
 }
 
+static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct btusb_data *data = usb_get_intfdata(intf);
+
+	BT_DBG("intf %p", intf);
+
+	if (data->suspend_count++)
+		return 0;
+
+	cancel_work_sync(&data->work);
+
+	usb_kill_anchored_urbs(&data->tx_anchor);
+
+	usb_kill_anchored_urbs(&data->isoc_anchor);
+	usb_kill_anchored_urbs(&data->bulk_anchor);
+	usb_kill_anchored_urbs(&data->intr_anchor);
+
+	return 0;
+}
+
+static int btusb_resume(struct usb_interface *intf)
+{
+	struct btusb_data *data = usb_get_intfdata(intf);
+	struct hci_dev *hdev = data->hdev;
+	int err;
+
+	BT_DBG("intf %p", intf);
+
+	if (--data->suspend_count)
+		return 0;
+
+	if (!test_bit(HCI_RUNNING, &hdev->flags))
+		return 0;
+
+	if (test_bit(BTUSB_INTR_RUNNING, &data->flags)) {
+		err = btusb_submit_intr_urb(hdev);
+		if (err < 0) {
+			clear_bit(BTUSB_INTR_RUNNING, &data->flags);
+			return err;
+		}
+	}
+
+	if (test_bit(BTUSB_BULK_RUNNING, &data->flags)) {
+		if (btusb_submit_bulk_urb(hdev) < 0)
+			clear_bit(BTUSB_BULK_RUNNING, &data->flags);
+		else
+			btusb_submit_bulk_urb(hdev);
+	}
+
+	if (test_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
+		if (btusb_submit_isoc_urb(hdev) < 0)
+			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+		else
+			btusb_submit_isoc_urb(hdev);
+	}
+
+	return 0;
+}
+
 static struct usb_driver btusb_driver = {
 	.name		= "btusb",
 	.probe		= btusb_probe,
 	.disconnect	= btusb_disconnect,
+	.suspend	= btusb_suspend,
+	.resume		= btusb_resume,
 	.id_table	= btusb_table,
 };
 

Re: [Bug 11442] btusb suspend/resume bug...

[Marcel Holtmann]

Hi Rafael,

> > >> Rafael, can you pull from my tree and test the changes:
> > >>
> > >> git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/ 
> > >> bluetooth-2.6.git
> > >>
> > >> It would be interesting if these fixes are enough.
> > >
> > > They appear to be enough.  I haven't had any suspend/resume failures  
> > > with them
> > > applied.
> > 
> > so it works _without_ applying patch-btusb-suspend.
> 
> Well, unfortunately I spoke too soon.
> 
> I'm still seeing post-hibernation crashes triggered by the bluetooth user land
> trying to use the device handled by btusb.  They happen every second
> hibernation, more or less, and apparently they are oopses in various code
> paths not directly related to bluetooth, like ext3 (memory corruption or
> what?).

I pushed two extra patches to my bluetooth-2.6 repository:

git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-2.6.git

One is fixing a double-free in the error path. This error path can be
triggered during suspend/resume if the USB core just disconnects the
device. Please check if that fixes it for you.

> With patch-btusb-suspend applied I don't see them (actually I have to use
> a slightly modified version of the patch which is appended).
> 
> Interestingly enough, suspend to RAM works without any visible problems.

As Oliver said, the USB core should do the right thing when no suspend
and resume callbacks are provided. I looked through the code so many
times now and I am running out of ideas what can happen.

Lets try it one last time without the suspend patch, but the double free
fix and see if that works. Otherwise I really give up.

Regards

Marcel

Re: [Bug 11442] btusb suspend/resume bug...

[Rafael J. Wysocki]

On Tuesday, 30 of September 2008, Marcel Holtmann wrote:
> Hi Rafael,
> 
> > > >> Rafael, can you pull from my tree and test the changes:
> > > >>
> > > >> git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/ 
> > > >> bluetooth-2.6.git
> > > >>
> > > >> It would be interesting if these fixes are enough.
> > > >
> > > > They appear to be enough.  I haven't had any suspend/resume failures  
> > > > with them
> > > > applied.
> > > 
> > > so it works _without_ applying patch-btusb-suspend.
> > 
> > Well, unfortunately I spoke too soon.
> > 
> > I'm still seeing post-hibernation crashes triggered by the bluetooth user land
> > trying to use the device handled by btusb.  They happen every second
> > hibernation, more or less, and apparently they are oopses in various code
> > paths not directly related to bluetooth, like ext3 (memory corruption or
> > what?).
> 
> I pushed two extra patches to my bluetooth-2.6 repository:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-2.6.git
> 
> One is fixing a double-free in the error path. This error path can be
> triggered during suspend/resume if the USB core just disconnects the
> device. Please check if that fixes it for you.
> 
> > With patch-btusb-suspend applied I don't see them (actually I have to use
> > a slightly modified version of the patch which is appended).
> > 
> > Interestingly enough, suspend to RAM works without any visible problems.
> 
> As Oliver said, the USB core should do the right thing when no suspend
> and resume callbacks are provided. I looked through the code so many
> times now and I am running out of ideas what can happen.
> 
> Lets try it one last time without the suspend patch, but the double free
> fix and see if that works. Otherwise I really give up.

This time I cannot reproduce the hibernation issue without the suspend patch,
so it appears that your double-free fix works for me.

Thanks,
Rafael

Re: [Bug 11442] btusb suspend/resume bug...

[Marcel Holtmann]

Hi Rafael,

> > > > >> Rafael, can you pull from my tree and test the changes:
> > > > >>
> > > > >> git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/ 
> > > > >> bluetooth-2.6.git
> > > > >>
> > > > >> It would be interesting if these fixes are enough.
> > > > >
> > > > > They appear to be enough.  I haven't had any suspend/resume failures  
> > > > > with them
> > > > > applied.
> > > > 
> > > > so it works _without_ applying patch-btusb-suspend.
> > > 
> > > Well, unfortunately I spoke too soon.
> > > 
> > > I'm still seeing post-hibernation crashes triggered by the bluetooth user land
> > > trying to use the device handled by btusb.  They happen every second
> > > hibernation, more or less, and apparently they are oopses in various code
> > > paths not directly related to bluetooth, like ext3 (memory corruption or
> > > what?).
> > 
> > I pushed two extra patches to my bluetooth-2.6 repository:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-2.6.git
> > 
> > One is fixing a double-free in the error path. This error path can be
> > triggered during suspend/resume if the USB core just disconnects the
> > device. Please check if that fixes it for you.
> > 
> > > With patch-btusb-suspend applied I don't see them (actually I have to use
> > > a slightly modified version of the patch which is appended).
> > > 
> > > Interestingly enough, suspend to RAM works without any visible problems.
> > 
> > As Oliver said, the USB core should do the right thing when no suspend
> > and resume callbacks are provided. I looked through the code so many
> > times now and I am running out of ideas what can happen.
> > 
> > Lets try it one last time without the suspend patch, but the double free
> > fix and see if that works. Otherwise I really give up.
> 
> This time I cannot reproduce the hibernation issue without the suspend patch,
> so it appears that your double-free fix works for me.

great. So I push these for 2.6.27 and then for 2.6.28, we can add full
suspend and auto-suspend and remote-wakeup support. Thanks for testing.

Regards

Marcel