Bug 11442

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,
 };
 

Patch : http://bugzilla.kernel.org/show_bug.cgi?id=11442#c1

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

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

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

Fixed in 2.6.27 final.