[PATCH] Regression fix revert: "Bluetooth: Add missing reset_resume dev_pm_ops"

[PATCH] Regression fix revert: "Bluetooth: Add missing reset_resume dev_pm_ops"

[Hans de Goede]

Many btusb devices have 2 modes, a hid mode and a bluetooth hci mode. These
devices default to hid mode for BIOS use. This means that after having been
reset they will revert to HID mode, and are no longer usable as a HCI.

Therefor it is a very bad idea to just blindly make reset_resume point to
the regular resume handler. Note that the btusb driver has no clue how to
switch these devices from hid to hci mode, this is done in userspace through
udev rules, so the proper way to deal with this is to not have a reset-resume
handler and instead let the usb-system re-enumerate the device, and re-run
the udev rules.

I must also note, that the commit message for the commit causing this
problem has a very weak motivation for the change:

"Add missing reset_resume dev_pm_ops. Missing reset_resume results in the
following message after power management device test. This change sets
reset_resume to btusb_resume().

[ 2506.936134] btusb 1-1.5:1.0: no reset_resume for driver btusb?
[ 2506.936137] btusb 1-1.5:1.1: no reset_resume for driver btusb?"

Making a change solely to silence a warning while also changing important
behavior (normal resume handling versus re-enumeration) requires a commit
message with a proper explanation why it is safe to do so, which clearly lacks
here, and unsurprisingly it turns out to not be safe to make this change.

Reverting the commit in question fixes bt no longer working on my Dell
E6430 after a suspend/resume, and I believe it likely also fixes the
following bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=988481
https://bugzilla.redhat.com/show_bug.cgi?id=1010649
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1213239

This reverts commit 502f769662978a2fe99d0caed5e53e3006107381.

Cc: Shuah Khan <shuah.kh@samsung.com>
Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/btusb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 8e16f0a..d0b3d90 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1623,7 +1623,6 @@ static struct usb_driver btusb_driver = {
 #ifdef CONFIG_PM
 	.suspend	= btusb_suspend,
 	.resume		= btusb_resume,
-	.reset_resume	= btusb_resume,
 #endif
 	.id_table	= btusb_table,
 	.supports_autosuspend = 1,
-- 
1.8.3.1

Re: [PATCH] Regression fix revert: "Bluetooth: Add missing reset_resume dev_pm_ops"

[Gustavo Padovan]

Hi Hans,

2013-09-28 Hans de Goede <hdegoede@redhat.com>:

> Many btusb devices have 2 modes, a hid mode and a bluetooth hci mode. These
> devices default to hid mode for BIOS use. This means that after having been
> reset they will revert to HID mode, and are no longer usable as a HCI.
> 
> Therefor it is a very bad idea to just blindly make reset_resume point to
> the regular resume handler. Note that the btusb driver has no clue how to
> switch these devices from hid to hci mode, this is done in userspace through
> udev rules, so the proper way to deal with this is to not have a reset-resume
> handler and instead let the usb-system re-enumerate the device, and re-run
> the udev rules.
> 
> I must also note, that the commit message for the commit causing this
> problem has a very weak motivation for the change:
> 
> "Add missing reset_resume dev_pm_ops. Missing reset_resume results in the
> following message after power management device test. This change sets
> reset_resume to btusb_resume().
> 
> [ 2506.936134] btusb 1-1.5:1.0: no reset_resume for driver btusb?
> [ 2506.936137] btusb 1-1.5:1.1: no reset_resume for driver btusb?"
> 
> Making a change solely to silence a warning while also changing important
> behavior (normal resume handling versus re-enumeration) requires a commit
> message with a proper explanation why it is safe to do so, which clearly lacks
> here, and unsurprisingly it turns out to not be safe to make this change.
> 
> Reverting the commit in question fixes bt no longer working on my Dell
> E6430 after a suspend/resume, and I believe it likely also fixes the
> following bugs:
> https://bugzilla.redhat.com/show_bug.cgi?id=988481
> https://bugzilla.redhat.com/show_bug.cgi?id=1010649
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1213239
> 
> This reverts commit 502f769662978a2fe99d0caed5e53e3006107381.
> 
> Cc: Shuah Khan <shuah.kh@samsung.com>
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/bluetooth/btusb.c | 1 -
>  1 file changed, 1 deletion(-)

Patch has been applied to bluetooth.git. Thanks.

	Gustavo

Re: [PATCH] Regression fix revert: "Bluetooth: Add missing reset_resume dev_pm_ops"

[Marcel Holtmann]

Hi Gustavo,

>> Many btusb devices have 2 modes, a hid mode and a bluetooth hci mode. These
>> devices default to hid mode for BIOS use. This means that after having been
>> reset they will revert to HID mode, and are no longer usable as a HCI.
>> 
>> Therefor it is a very bad idea to just blindly make reset_resume point to
>> the regular resume handler. Note that the btusb driver has no clue how to
>> switch these devices from hid to hci mode, this is done in userspace through
>> udev rules, so the proper way to deal with this is to not have a reset-resume
>> handler and instead let the usb-system re-enumerate the device, and re-run
>> the udev rules.
>> 
>> I must also note, that the commit message for the commit causing this
>> problem has a very weak motivation for the change:
>> 
>> "Add missing reset_resume dev_pm_ops. Missing reset_resume results in the
>> following message after power management device test. This change sets
>> reset_resume to btusb_resume().
>> 
>> [ 2506.936134] btusb 1-1.5:1.0: no reset_resume for driver btusb?
>> [ 2506.936137] btusb 1-1.5:1.1: no reset_resume for driver btusb?"
>> 
>> Making a change solely to silence a warning while also changing important
>> behavior (normal resume handling versus re-enumeration) requires a commit
>> message with a proper explanation why it is safe to do so, which clearly lacks
>> here, and unsurprisingly it turns out to not be safe to make this change.
>> 
>> Reverting the commit in question fixes bt no longer working on my Dell
>> E6430 after a suspend/resume, and I believe it likely also fixes the
>> following bugs:
>> https://bugzilla.redhat.com/show_bug.cgi?id=988481
>> https://bugzilla.redhat.com/show_bug.cgi?id=1010649
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1213239
>> 
>> This reverts commit 502f769662978a2fe99d0caed5e53e3006107381.
>> 
>> Cc: Shuah Khan <shuah.kh@samsung.com>
>> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/bluetooth/btusb.c | 1 -
>> 1 file changed, 1 deletion(-)
> 
> Patch has been applied to bluetooth.git. Thanks.

why? Because we have one broken Dell Bluetooth dongle. Do we actually know how this affects other chips. The dell HID Proxy thing has always been special case and that is Dell's fault. Look at the extra code that we have in hid2hci tool and its udev rules for Dell hardware. Is anybody actually willing to investigate this one properly.

Regards

Marcel

Re: [PATCH] Regression fix revert: "Bluetooth: Add missing reset_resume dev_pm_ops"

[Hans de Goede]

Hi,

On 10/03/2013 05:39 AM, Marcel Holtmann wrote:
> Hi Gustavo,
>
>>> Many btusb devices have 2 modes, a hid mode and a bluetooth hci mode. These
>>> devices default to hid mode for BIOS use. This means that after having been
>>> reset they will revert to HID mode, and are no longer usable as a HCI.
>>>
>>> Therefor it is a very bad idea to just blindly make reset_resume point to
>>> the regular resume handler. Note that the btusb driver has no clue how to
>>> switch these devices from hid to hci mode, this is done in userspace through
>>> udev rules, so the proper way to deal with this is to not have a reset-resume
>>> handler and instead let the usb-system re-enumerate the device, and re-run
>>> the udev rules.
>>>
>>> I must also note, that the commit message for the commit causing this
>>> problem has a very weak motivation for the change:
>>>
>>> "Add missing reset_resume dev_pm_ops. Missing reset_resume results in the
>>> following message after power management device test. This change sets
>>> reset_resume to btusb_resume().
>>>
>>> [ 2506.936134] btusb 1-1.5:1.0: no reset_resume for driver btusb?
>>> [ 2506.936137] btusb 1-1.5:1.1: no reset_resume for driver btusb?"
>>>
>>> Making a change solely to silence a warning while also changing important
>>> behavior (normal resume handling versus re-enumeration) requires a commit
>>> message with a proper explanation why it is safe to do so, which clearly lacks
>>> here, and unsurprisingly it turns out to not be safe to make this change.
>>>
>>> Reverting the commit in question fixes bt no longer working on my Dell
>>> E6430 after a suspend/resume, and I believe it likely also fixes the
>>> following bugs:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=988481
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1010649
>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1213239
>>>
>>> This reverts commit 502f769662978a2fe99d0caed5e53e3006107381.
>>>
>>> Cc: Shuah Khan <shuah.kh@samsung.com>
>>> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> drivers/bluetooth/btusb.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>
>> Patch has been applied to bluetooth.git. Thanks.
>
> why? Because we have one broken Dell Bluetooth dongle.

Well for one, because we have a no regressions policy.

For another because the original patch does not actually fix anything
(according to the original commit message), it just silences a warning
(with a way too big hammer).

> Do we actually know how this affects other chips.

In the mean time I have confirmation that the reset_resume patch also
breaks intel hci's (embedded in their wlan cards, 8087:07da) after
suspend/resume, and that reverting it also fixes this. To be precise
I've confirmation that downgrading to 3.10 which lacks this patch fixes
the intel hci issues, see:
https://bugzilla.redhat.com/show_bug.cgi?id=988481

> The dell HID Proxy thing has always been special case and that is Dell's fault.

And the Logitech HID proxy, and various devices with broadcom chips ...

> Look at the extra code that we have in hid2hci tool and its udev rules for Dell hardware. Is anybody actually willing to investigate this one properly.

If people really want a reset_resume handler, then a lot more works need
to be done then just making reset_resume point to the current resume handler.

reset_resume gets called when on resume the usb subsys sees that the device
has lost power, iow the device has had a hard reset, and all device state is
gone. The current btusb resume handler was never written with the assumption
that all device state is gone. All it does is resubmit a bunch of urbs, it
does not restore any device state, therefor it is unsuitable as a
reset_resume handler.

Regards,

Hans

Re: [PATCH] Regression fix revert: "Bluetooth: Add missing reset_resume dev_pm_ops"

[Shuah Khan]

On 10/02/2013 09:39 PM, Marcel Holtmann wrote:
> Hi Gustavo,
>
>>> Many btusb devices have 2 modes, a hid mode and a bluetooth hci mode. These
>>> devices default to hid mode for BIOS use. This means that after having been
>>> reset they will revert to HID mode, and are no longer usable as a HCI.
>>>
>>> Therefor it is a very bad idea to just blindly make reset_resume point to
>>> the regular resume handler. Note that the btusb driver has no clue how to
>>> switch these devices from hid to hci mode, this is done in userspace through
>>> udev rules, so the proper way to deal with this is to not have a reset-resume
>>> handler and instead let the usb-system re-enumerate the device, and re-run
>>> the udev rules.
>>>
>>> I must also note, that the commit message for the commit causing this
>>> problem has a very weak motivation for the change:
>>>
>>> "Add missing reset_resume dev_pm_ops. Missing reset_resume results in the
>>> following message after power management device test. This change sets
>>> reset_resume to btusb_resume().
>>>
>>> [ 2506.936134] btusb 1-1.5:1.0: no reset_resume for driver btusb?
>>> [ 2506.936137] btusb 1-1.5:1.1: no reset_resume for driver btusb?"
>>>
>>> Making a change solely to silence a warning while also changing important
>>> behavior (normal resume handling versus re-enumeration) requires a commit
>>> message with a proper explanation why it is safe to do so, which clearly lacks
>>> here, and unsurprisingly it turns out to not be safe to make this change.
>>>
>>> Reverting the commit in question fixes bt no longer working on my Dell
>>> E6430 after a suspend/resume, and I believe it likely also fixes the
>>> following bugs:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=988481
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1010649
>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1213239
>>>
>>> This reverts commit 502f769662978a2fe99d0caed5e53e3006107381.
>>>
>>> Cc: Shuah Khan <shuah.kh@samsung.com>
>>> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> drivers/bluetooth/btusb.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>
>> Patch has been applied to bluetooth.git. Thanks.
>
> why? Because we have one broken Dell Bluetooth dongle. Do we actually know how this affects other chips. The dell HID Proxy thing has always been special case and that is Dell's fault. Look at the extra code that we have in hid2hci tool and its udev rules for Dell hardware. Is anybody actually willing to investigate this one properly.
>
> Regards
>
> Marcel
>

Sorry for the late reply on this. Sorry for the regression on Dell. I 
didn't see any problems when I tested it on the bluetooth mouse I have. 
However, I understand what Hans is saying. My change did cause a 
regression. I will investigate this further.

The warning message itself is very misleading in the sense that it gives 
the impression that the resume routine that should have been installed 
is missing. Which is what caused me to fix things in the first place.

thanks,
-- Shuah

-- 
Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
shuah.kh@samsung.com | (970) 672-0658

Re: [PATCH] Regression fix revert: "Bluetooth: Add missing reset_resume dev_pm_ops"

[Hans de Goede]

Hi Gustavo,

> Patch has been applied to bluetooth.git. Thanks.

Hmm, I'm not seeing this in 3.12 yet, not only should this be added
to 3.12 asap, it should also really get added to 3.11.x soon.

The regression this patch fixes is breaking btusb for lots of users, see:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1213239
https://bugzilla.redhat.com/show_bug.cgi?id=988481

Where lots of people are adding me too comments (and some are building
their own kernels with the revert and confirming it fixes things for
them).

Regards,

Hans