Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Brian Norris]

Hi,

On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote:
> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
> 
> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
> has several issues (see the original commit message). An added advantage
> of moving over to the USB-core reset-resume handling is that it also
> disables autosuspend for these devices, which is similarly broken on these.

Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't
think so -- I'm using one now. Thus, this is a poor solution, which
negatively affects my systems. However, I see that this patch was
applied regardless...

What justifications was found for this anyway? AIUI, this is a platform
bug, and not entirely a chipset bug. Why should all users of this
chipset be penalized?

Brian

> But there are 2 issues with this approach:
> 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek
>    devices.
> 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been
>    added to usb/core/quirks.c and if we fix the Realtek case the same way
>    we need to add an additional 14 entries. So in essence we need to
>    duplicate a large part of the usb_device_id table in btusb.c in
>    usb/core/quirks.c and manually keep them in sync.
> 
> This commit instead restores setting a reset-resume quirk for QCA devices
> in the btusb.c code, avoiding the duplicate usb_device_id table problem.
> 
> This commit avoids the problems with the original DIY BTUSB_RESET_RESUME
> code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the
> usb_device.
> 
> This commit also moves the BTUSB_REALTEK case over to directly setting the
> USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused
> BTUSB_RESET_RESUME code.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
> Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
> Cc: stable@vger.kernel.org
> Cc: Leif Liddy <leif.linux@gmail.com>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Daniel Drake <drake@endlessm.com>
> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note:
> 1) Once this has been merged, the 2 commits adding QCA device entries to
> drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next.
> 2) I don't have any of the affected devices, please test
> ---
>  drivers/bluetooth/btusb.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 4764100a5888..c4689f03220f 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -23,6 +23,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/usb.h>
> +#include <linux/usb/quirks.h>
>  #include <linux/firmware.h>
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
> @@ -388,9 +389,8 @@ static const struct usb_device_id blacklist_table[] = {
>  #define BTUSB_FIRMWARE_LOADED	7
>  #define BTUSB_FIRMWARE_FAILED	8
>  #define BTUSB_BOOTING		9
> -#define BTUSB_RESET_RESUME	10
> -#define BTUSB_DIAG_RUNNING	11
> -#define BTUSB_OOB_WAKE_ENABLED	12
> +#define BTUSB_DIAG_RUNNING	10
> +#define BTUSB_OOB_WAKE_ENABLED	11
>  
>  struct btusb_data {
>  	struct hci_dev       *hdev;
> @@ -3118,6 +3118,12 @@ static int btusb_probe(struct usb_interface *intf,
>  	if (id->driver_info & BTUSB_QCA_ROME) {
>  		data->setup_on_usb = btusb_setup_qca;
>  		hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
> +
> +		/* QCA Rome devices lose their updated firmware over suspend,
> +		 * but the USB hub doesn't notice any status change.
> +		 * explicitly request a device reset on resume.
> +		 */
> +		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
>  	}
>  
>  #ifdef CONFIG_BT_HCIBTUSB_RTL
> @@ -3128,7 +3134,7 @@ static int btusb_probe(struct usb_interface *intf,
>  		 * but the USB hub doesn't notice any status change.
>  		 * Explicitly request a device reset on resume.
>  		 */
> -		set_bit(BTUSB_RESET_RESUME, &data->flags);
> +		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
>  	}
>  #endif
>  
> @@ -3297,14 +3303,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>  		enable_irq(data->oob_wake_irq);
>  	}
>  
> -	/* Optionally request a device reset on resume, but only when
> -	 * wakeups are disabled. If wakeups are enabled we assume the
> -	 * device will stay powered up throughout suspend.
> -	 */
> -	if (test_bit(BTUSB_RESET_RESUME, &data->flags) &&
> -	    !device_may_wakeup(&data->udev->dev))
> -		data->udev->reset_resume = 1;
> -
>  	return 0;
>  }
>  
> -- 
> 2.14.3
> 

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Hans de Goede]

Hi,

On 13-02-18 03:24, Brian Norris wrote:
> Hi,
> 
> On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote:
>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>>
>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
>> has several issues (see the original commit message). An added advantage
>> of moving over to the USB-core reset-resume handling is that it also
>> disables autosuspend for these devices, which is similarly broken on these.
> 
> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't
> think so -- I'm using one now.

And have you manually enabled USB autosuspend for it, or are you
running something which might have done so, e.g. powertop --auto-tune ?

Because if you did not do that then you're already not using autosuspend
for your QCA devices and this patch will change nothing.

> Thus, this is a poor solution, which
> negatively affects my systems. However, I see that this patch was
> applied regardless...

Note that there already is a quirk to handle broken suspend/resume
behavior on ALL QCA devices in older kernels. Also note that the
patches series which this commit builds on top of was already
setting USB_QUIRK_RESET_RESUME for some devices in
usb/core/quirks.c.

All my commit does is instead of duplicating all the QCA USB-ids in
usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME
to btusb.c so that we don't need to duplicate the USB-id tables.

The result of the combination of these patches is that the custom
DIY reset on resume handling btusb.c was doing is now replaced
by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk.

As a (desirable) side effect this also disables USB autosuspend
for QCA devices since the USB-core does not allow USB autosuspend
on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown
this to be necessary on at least some QCA devices and given that
these devices tend to loose there firmware on a suspend, it seems
sensible to not allow autosuspend on them.

 > What justifications was found for this anyway? AIUI, this is a platform
 > bug, and not entirely a chipset bug.

No this is believed to be a chipset issue, hence also the quirk in
older kernels to always reset these devices after a normal suspend/resume.

Regards,

Hans




> 
> Brian
> 
>> But there are 2 issues with this approach:
>> 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek
>>     devices.
>> 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been
>>     added to usb/core/quirks.c and if we fix the Realtek case the same way
>>     we need to add an additional 14 entries. So in essence we need to
>>     duplicate a large part of the usb_device_id table in btusb.c in
>>     usb/core/quirks.c and manually keep them in sync.
>>
>> This commit instead restores setting a reset-resume quirk for QCA devices
>> in the btusb.c code, avoiding the duplicate usb_device_id table problem.
>>
>> This commit avoids the problems with the original DIY BTUSB_RESET_RESUME
>> code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the
>> usb_device.
>>
>> This commit also moves the BTUSB_REALTEK case over to directly setting the
>> USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused
>> BTUSB_RESET_RESUME code.
>>
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
>> Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
>> Cc: stable@vger.kernel.org
>> Cc: Leif Liddy <leif.linux@gmail.com>
>> Cc: Matthias Kaehlcke <mka@chromium.org>
>> Cc: Brian Norris <briannorris@chromium.org>
>> Cc: Daniel Drake <drake@endlessm.com>
>> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note:
>> 1) Once this has been merged, the 2 commits adding QCA device entries to
>> drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next.
>> 2) I don't have any of the affected devices, please test
>> ---
>>   drivers/bluetooth/btusb.c | 22 ++++++++++------------
>>   1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 4764100a5888..c4689f03220f 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -23,6 +23,7 @@
>>   
>>   #include <linux/module.h>
>>   #include <linux/usb.h>
>> +#include <linux/usb/quirks.h>
>>   #include <linux/firmware.h>
>>   #include <linux/of_device.h>
>>   #include <linux/of_irq.h>
>> @@ -388,9 +389,8 @@ static const struct usb_device_id blacklist_table[] = {
>>   #define BTUSB_FIRMWARE_LOADED	7
>>   #define BTUSB_FIRMWARE_FAILED	8
>>   #define BTUSB_BOOTING		9
>> -#define BTUSB_RESET_RESUME	10
>> -#define BTUSB_DIAG_RUNNING	11
>> -#define BTUSB_OOB_WAKE_ENABLED	12
>> +#define BTUSB_DIAG_RUNNING	10
>> +#define BTUSB_OOB_WAKE_ENABLED	11
>>   
>>   struct btusb_data {
>>   	struct hci_dev       *hdev;
>> @@ -3118,6 +3118,12 @@ static int btusb_probe(struct usb_interface *intf,
>>   	if (id->driver_info & BTUSB_QCA_ROME) {
>>   		data->setup_on_usb = btusb_setup_qca;
>>   		hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
>> +
>> +		/* QCA Rome devices lose their updated firmware over suspend,
>> +		 * but the USB hub doesn't notice any status change.
>> +		 * explicitly request a device reset on resume.
>> +		 */
>> +		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
>>   	}
>>   
>>   #ifdef CONFIG_BT_HCIBTUSB_RTL
>> @@ -3128,7 +3134,7 @@ static int btusb_probe(struct usb_interface *intf,
>>   		 * but the USB hub doesn't notice any status change.
>>   		 * Explicitly request a device reset on resume.
>>   		 */
>> -		set_bit(BTUSB_RESET_RESUME, &data->flags);
>> +		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
>>   	}
>>   #endif
>>   
>> @@ -3297,14 +3303,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>>   		enable_irq(data->oob_wake_irq);
>>   	}
>>   
>> -	/* Optionally request a device reset on resume, but only when
>> -	 * wakeups are disabled. If wakeups are enabled we assume the
>> -	 * device will stay powered up throughout suspend.
>> -	 */
>> -	if (test_bit(BTUSB_RESET_RESUME, &data->flags) &&
>> -	    !device_may_wakeup(&data->udev->dev))
>> -		data->udev->reset_resume = 1;
>> -
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.14.3
>>

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Brian Norris]

Hi Hans,

On Tue, Feb 13, 2018 at 12:25:55PM +0100, Hans de Goede wrote:
> On 13-02-18 03:24, Brian Norris wrote:
> > On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote:
> > > Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
> > > removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
> > > instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
> > > 
> > > This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
> > > has several issues (see the original commit message). An added advantage
> > > of moving over to the USB-core reset-resume handling is that it also
> > > disables autosuspend for these devices, which is similarly broken on these.
> > 
> > Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't
> > think so -- I'm using one now.
> 
> And have you manually enabled USB autosuspend for it, or are you
> running something which might have done so, e.g. powertop --auto-tune ?
> 
> Because if you did not do that then you're already not using autosuspend
> for your QCA devices and this patch will change nothing.

I use a set of udev rules that manually whitelist devices for
autosuspend. You can see it here:

https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83

You'll find at least one Rome chip in there.

> > Thus, this is a poor solution, which
> > negatively affects my systems. However, I see that this patch was
> > applied regardless...
> 
> Note that there already is a quirk to handle broken suspend/resume
> behavior on ALL QCA devices in older kernels. Also note that the

Yes, and the quirk was broken, and I made sure it got reverted when it
broke my devices ;)

> patches series which this commit builds on top of was already
> setting USB_QUIRK_RESET_RESUME for some devices in
> usb/core/quirks.c.
> 
> All my commit does is instead of duplicating all the QCA USB-ids in
> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME
> to btusb.c so that we don't need to duplicate the USB-id tables.

I was slightly more OK with marking specific IDs as broken, because
those IDs didn't happen to be ones that I knew were currently working.
Now you're breaking my systems again. But this time, it's more subtle
because bluetooth will still work, but we just suck more power leaving
our USB port active all the time.

> The result of the combination of these patches is that the custom
> DIY reset on resume handling btusb.c was doing is now replaced
> by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk.
> 
> As a (desirable) side effect this also disables USB autosuspend
> for QCA devices since the USB-core does not allow USB autosuspend
> on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown
> this to be necessary on at least some QCA devices and given that
> these devices tend to loose there firmware on a suspend, it seems
> sensible to not allow autosuspend on them.

But you're not accurately targeting the "some". AFAICT, you're wasting
power on my system.

> > What justifications was found for this anyway? AIUI, this is a platform
> > bug, and not entirely a chipset bug.
> 
> No this is believed to be a chipset issue, hence also the quirk in
> older kernels to always reset these devices after a normal suspend/resume.

I have Qualcomm telling me this is a platform issue. I haven't noticed
problems with autosuspend nor system suspend/resume on my platform. Do
you have any more detail on this issue? Have you consulsted QCA folks?

Unfortunately, Greg is already queueing this patch up to all the -stable
trees, so I'm going to have to revert it yet again in Chromium
kernels...

Brian

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Guenter Roeck]

On 02/15/2018 06:27 PM, Brian Norris wrote:
> Hi Hans,
> 
> On Tue, Feb 13, 2018 at 12:25:55PM +0100, Hans de Goede wrote:
>> On 13-02-18 03:24, Brian Norris wrote:
>>> On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote:
>>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
>>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
>>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>>>>
>>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
>>>> has several issues (see the original commit message). An added advantage
>>>> of moving over to the USB-core reset-resume handling is that it also
>>>> disables autosuspend for these devices, which is similarly broken on these.
>>>
>>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't
>>> think so -- I'm using one now.
>>
>> And have you manually enabled USB autosuspend for it, or are you
>> running something which might have done so, e.g. powertop --auto-tune ?
>>
>> Because if you did not do that then you're already not using autosuspend
>> for your QCA devices and this patch will change nothing.
> 
> I use a set of udev rules that manually whitelist devices for
> autosuspend. You can see it here:
> 
> https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83
> 
> You'll find at least one Rome chip in there.
> 
>>> Thus, this is a poor solution, which
>>> negatively affects my systems. However, I see that this patch was
>>> applied regardless...
>>
>> Note that there already is a quirk to handle broken suspend/resume
>> behavior on ALL QCA devices in older kernels. Also note that the
> 
> Yes, and the quirk was broken, and I made sure it got reverted when it
> broke my devices ;)
> 
>> patches series which this commit builds on top of was already
>> setting USB_QUIRK_RESET_RESUME for some devices in
>> usb/core/quirks.c.
>>
>> All my commit does is instead of duplicating all the QCA USB-ids in
>> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME
>> to btusb.c so that we don't need to duplicate the USB-id tables.
> 
> I was slightly more OK with marking specific IDs as broken, because
> those IDs didn't happen to be ones that I knew were currently working.
> Now you're breaking my systems again. But this time, it's more subtle
> because bluetooth will still work, but we just suck more power leaving
> our USB port active all the time.
> 
>> The result of the combination of these patches is that the custom
>> DIY reset on resume handling btusb.c was doing is now replaced
>> by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk.
>>
>> As a (desirable) side effect this also disables USB autosuspend
>> for QCA devices since the USB-core does not allow USB autosuspend
>> on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown
>> this to be necessary on at least some QCA devices and given that
>> these devices tend to loose there firmware on a suspend, it seems
>> sensible to not allow autosuspend on them.
> 
> But you're not accurately targeting the "some". AFAICT, you're wasting
> power on my system.
> 
>>> What justifications was found for this anyway? AIUI, this is a platform
>>> bug, and not entirely a chipset bug.
>>
>> No this is believed to be a chipset issue, hence also the quirk in
>> older kernels to always reset these devices after a normal suspend/resume.
> 
> I have Qualcomm telling me this is a platform issue. I haven't noticed
> problems with autosuspend nor system suspend/resume on my platform. Do
> you have any more detail on this issue? Have you consulsted QCA folks?
> 
> Unfortunately, Greg is already queueing this patch up to all the -stable
> trees, so I'm going to have to revert it yet again in Chromium
> kernels...
> 

Grumble Grumble Grumble. I'll try to remember to revert this patch when I merge
v4.14.20 into chromeos-4.14. Please remind me if I forget for some reason.

I don't see it queued in v4.4.y - is that still coming ? Hope I won't miss it.

Guenter

udev USB autosupend whitelist (was Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version)

[Hans de Goede]

Hi,

Going a bit off-topic here, so changed the subject.
I will reply on topic in another mail.

On 16-02-18 03:27, Brian Norris wrote:
> Hi Hans,
> 
> On Tue, Feb 13, 2018 at 12:25:55PM +0100, Hans de Goede wrote:
>> On 13-02-18 03:24, Brian Norris wrote:
>>> On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote:
>>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
>>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
>>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>>>>
>>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
>>>> has several issues (see the original commit message). An added advantage
>>>> of moving over to the USB-core reset-resume handling is that it also
>>>> disables autosuspend for these devices, which is similarly broken on these.
>>>
>>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't
>>> think so -- I'm using one now.
>>
>> And have you manually enabled USB autosuspend for it, or are you
>> running something which might have done so, e.g. powertop --auto-tune ?
>>
>> Because if you did not do that then you're already not using autosuspend
>> for your QCA devices and this patch will change nothing.
> 
> I use a set of udev rules that manually whitelist devices for
> autosuspend. You can see it here:
> 
> https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83
> 
> You'll find at least one Rome chip in there.

Oh, that is a very interesting link for the work I've been doing to
improve Linux power-consumption in general:

https://fedoraproject.org/wiki/Changes/ImprovedLaptopBatteryLife

I was actually planning on at least doing such a list for WWAN modems,
for btusb my approach has been to just enable it everywhere
(except for QCA devices as I got bugreports for those).

Note that I plan to eventually submit this whitelist to the
udev rules which are part of systemd upstream, so if chromeos
is using systemd too, this is something to be aware of for you.

Question, is the white-listing of the root and rate-limiting
hubs really necessary? I thought these have this enabled by default?

Also any caveats here I should be aware of?

Regards,

Hans

Re: udev USB autosupend whitelist (was Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version)

[Brian Norris]

+ Benson (and there are probably others that know better answers)

Hi,

On Fri, Feb 16, 2018 at 09:26:37AM +0100, Hans de Goede wrote:
> Going a bit off-topic here, so changed the subject.
> I will reply on topic in another mail.
> 
> On 16-02-18 03:27, Brian Norris wrote:
> > I use a set of udev rules that manually whitelist devices for
> > autosuspend. You can see it here:
> > 
> > https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83
> > 
> > You'll find at least one Rome chip in there.
> 
> Oh, that is a very interesting link for the work I've been doing to
> improve Linux power-consumption in general:
> 
> https://fedoraproject.org/wiki/Changes/ImprovedLaptopBatteryLife
> 
> I was actually planning on at least doing such a list for WWAN modems,
> for btusb my approach has been to just enable it everywhere
> (except for QCA devices as I got bugreports for those).
> 
> Note that I plan to eventually submit this whitelist to the
> udev rules which are part of systemd upstream, so if chromeos
> is using systemd too, this is something to be aware of for you.

Chrome OS does not currently use systemd, but thanks for the heads up.

> Question, is the white-listing of the root and rate-limiting
> hubs really necessary? I thought these have this enabled by default?

This list is old and maintained by several of my team, originating from
quite a ways back (i.e., much older kernels). It's quite possible that
some of it is redundant today.

> Also any caveats here I should be aware of?

That it's only maintained for the express purpose of Chrome{device}s?
There's no guarantee that there aren't platform issues with other
systems, for instance :)

I'm not really aware of any particular caveats otherwise.

Brian

Re: udev USB autosupend whitelist (was Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version)

[Hans de Goede]

Hi,

On 16-02-18 17:49, Brian Norris wrote:
> + Benson (and there are probably others that know better answers)
> 
> Hi,
> 
> On Fri, Feb 16, 2018 at 09:26:37AM +0100, Hans de Goede wrote:
>> Going a bit off-topic here, so changed the subject.
>> I will reply on topic in another mail.
>>
>> On 16-02-18 03:27, Brian Norris wrote:
>>> I use a set of udev rules that manually whitelist devices for
>>> autosuspend. You can see it here:
>>>
>>> https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83
>>>
>>> You'll find at least one Rome chip in there.
>>
>> Oh, that is a very interesting link for the work I've been doing to
>> improve Linux power-consumption in general:
>>
>> https://fedoraproject.org/wiki/Changes/ImprovedLaptopBatteryLife
>>
>> I was actually planning on at least doing such a list for WWAN modems,
>> for btusb my approach has been to just enable it everywhere
>> (except for QCA devices as I got bugreports for those).
>>
>> Note that I plan to eventually submit this whitelist to the
>> udev rules which are part of systemd upstream, so if chromeos
>> is using systemd too, this is something to be aware of for you.
> 
> Chrome OS does not currently use systemd, but thanks for the heads up.
> 
>> Question, is the white-listing of the root and rate-limiting
>> hubs really necessary? I thought these have this enabled by default?
> 
> This list is old and maintained by several of my team, originating from
> quite a ways back (i.e., much older kernels). It's quite possible that
> some of it is redundant today.

Ok, I double checked and it seems that explicitly setting power/control
to auto for any USB hub is not necessary as they all default to auto
now.

So FWIW you may want to consider removing this.

Regards,

Hans

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Marcel Holtmann]

Hi Brian,

>>> On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote:
>>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
>>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
>>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>>>> 
>>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
>>>> has several issues (see the original commit message). An added advantage
>>>> of moving over to the USB-core reset-resume handling is that it also
>>>> disables autosuspend for these devices, which is similarly broken on these.
>>> 
>>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't
>>> think so -- I'm using one now.
>> 
>> And have you manually enabled USB autosuspend for it, or are you
>> running something which might have done so, e.g. powertop --auto-tune ?
>> 
>> Because if you did not do that then you're already not using autosuspend
>> for your QCA devices and this patch will change nothing.
> 
> I use a set of udev rules that manually whitelist devices for
> autosuspend. You can see it here:
> 
> https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83
> 
> You'll find at least one Rome chip in there.
> 
>>> Thus, this is a poor solution, which
>>> negatively affects my systems. However, I see that this patch was
>>> applied regardless...
>> 
>> Note that there already is a quirk to handle broken suspend/resume
>> behavior on ALL QCA devices in older kernels. Also note that the
> 
> Yes, and the quirk was broken, and I made sure it got reverted when it
> broke my devices ;)
> 
>> patches series which this commit builds on top of was already
>> setting USB_QUIRK_RESET_RESUME for some devices in
>> usb/core/quirks.c.
>> 
>> All my commit does is instead of duplicating all the QCA USB-ids in
>> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME
>> to btusb.c so that we don't need to duplicate the USB-id tables.
> 
> I was slightly more OK with marking specific IDs as broken, because
> those IDs didn't happen to be ones that I knew were currently working.
> Now you're breaking my systems again. But this time, it's more subtle
> because bluetooth will still work, but we just suck more power leaving
> our USB port active all the time.
> 
>> The result of the combination of these patches is that the custom
>> DIY reset on resume handling btusb.c was doing is now replaced
>> by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk.
>> 
>> As a (desirable) side effect this also disables USB autosuspend
>> for QCA devices since the USB-core does not allow USB autosuspend
>> on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown
>> this to be necessary on at least some QCA devices and given that
>> these devices tend to loose there firmware on a suspend, it seems
>> sensible to not allow autosuspend on them.
> 
> But you're not accurately targeting the "some". AFAICT, you're wasting
> power on my system.
> 
>>> What justifications was found for this anyway? AIUI, this is a platform
>>> bug, and not entirely a chipset bug.
>> 
>> No this is believed to be a chipset issue, hence also the quirk in
>> older kernels to always reset these devices after a normal suspend/resume.
> 
> I have Qualcomm telling me this is a platform issue. I haven't noticed
> problems with autosuspend nor system suspend/resume on my platform. Do
> you have any more detail on this issue? Have you consulsted QCA folks?
> 
> Unfortunately, Greg is already queueing this patch up to all the -stable
> trees, so I'm going to have to revert it yet again in Chromium
> kernels...

if this is a platform issue, then doing this based on VID:PID pairs is wrong no matter what. Then again, if we have really dedicated VID:PID combination for certain platforms, we could also just whitelist them in the driver. The btusb.c already contains an extra blacklist check. It is easy to add another USB ID table for a QCA whitelist.

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Hans de Goede]

Hi,

On 16-02-18 03:27, Brian Norris wrote:
> Hi Hans,
> 
> On Tue, Feb 13, 2018 at 12:25:55PM +0100, Hans de Goede wrote:
>> On 13-02-18 03:24, Brian Norris wrote:
>>> On Mon, Jan 08, 2018 at 10:44:16AM +0100, Hans de Goede wrote:
>>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
>>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
>>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>>>>
>>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
>>>> has several issues (see the original commit message). An added advantage
>>>> of moving over to the USB-core reset-resume handling is that it also
>>>> disables autosuspend for these devices, which is similarly broken on these.
>>>
>>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't
>>> think so -- I'm using one now.
>>
>> And have you manually enabled USB autosuspend for it, or are you
>> running something which might have done so, e.g. powertop --auto-tune ?
>>
>> Because if you did not do that then you're already not using autosuspend
>> for your QCA devices and this patch will change nothing.
> 
> I use a set of udev rules that manually whitelist devices for
> autosuspend. You can see it here:
> 
> https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83
> 
> You'll find at least one Rome chip in there.
 >
>>> Thus, this is a poor solution, which
>>> negatively affects my systems. However, I see that this patch was
>>> applied regardless...
>>
>> Note that there already is a quirk to handle broken suspend/resume
>> behavior on ALL QCA devices in older kernels. Also note that the
> 
> Yes, and the quirk was broken, and I made sure it got reverted when it
> broke my devices ;)

Note that the revert for this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d06d5895c15

Says: "This commit causes a regression on some QCA ROME chips. The USB device reset happens
in btusb_open(), hence firmware loading gets interrupted."

And says:

"If we really want to reset the USB device, we need to do it before btusb_open(). Let's
handle it in drivers/usb/core/quirks.c."

It does not mention that the quirk is not necessary on some devices only
that the implementation of it we had before had issues.

And the original commit of the quirk:

https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56

Says: "There's been numerous reported instances where BTUSB_QCA_ROME
bluetooth controllers stop functioning upon resume from suspend."

So it may be platform specific but it is not just limited to 1 or
2 platforms I think.

Note I'm not saying that I don't believe you that the quirk is not
necessary on your devices.

>> patches series which this commit builds on top of was already
>> setting USB_QUIRK_RESET_RESUME for some devices in
>> usb/core/quirks.c.
>>
>> All my commit does is instead of duplicating all the QCA USB-ids in
>> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME
>> to btusb.c so that we don't need to duplicate the USB-id tables.
> 
> I was slightly more OK with marking specific IDs as broken, because
> those IDs didn't happen to be ones that I knew were currently working.
> Now you're breaking my systems again. But this time, it's more subtle
> because bluetooth will still work, but we just suck more power leaving
> our USB port active all the time.

I see, sorry about that. Ok, so we are going to need to make the
reset-on-resume quirking more fine grained. I can see 3 ways to do this:

1) Make it a separate per usb-id BTUSB_RESET_RESUME flag in the
usb_device_id table inside btusb.c (I still believe duplicating most
ids to usb/core/quirks.c is a bad idea).

2) Use dmi based whitelisting to opt out of reset-resume behavior on
QCA btusb devices.

3) Use dmi based blacklisting which enables reset-resume behavior.

In retrospect I guess 3 would have been best, but if we do that now
it will cause regressions.

I guess we should go with 1. adding the BTUSB_RESET_RESUME to all the
QCA usb-ids except for the ones where you know things work and which
only seem to be used in working devices (based on you not having
objections against the additions of the quirk for some ids to
drivers/usb/core/quirks.c).

And if then those usb-ids do turn out to have broken suspend on
on some devices too I guess we need to move to 2.

>> The result of the combination of these patches is that the custom
>> DIY reset on resume handling btusb.c was doing is now replaced
>> by setting the standard USB-core USB_QUIRK_RESET_RESUME quirk.
>>
>> As a (desirable) side effect this also disables USB autosuspend
>> for QCA devices since the USB-core does not allow USB autosuspend
>> on devices with the USB_QUIRK_RESET_RESUME quirk. Testing has shown
>> this to be necessary on at least some QCA devices and given that
>> these devices tend to loose there firmware on a suspend, it seems
>> sensible to not allow autosuspend on them.
> 
> But you're not accurately targeting the "some". AFAICT, you're wasting
> power on my system.

OK, so it turns out that this is a platform problem, which was not
known to those involved until now. As mentioned before solution 3.
from my above list of solutions would then be ideal, but I don't see
how we can get there now without causing regressions where bluetooth
stops working completely for people.

Regards,

Hans

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Marcel Holtmann]

Hi Hans,

>>>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
>>>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
>>>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>>>>> 
>>>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
>>>>> has several issues (see the original commit message). An added advantage
>>>>> of moving over to the USB-core reset-resume handling is that it also
>>>>> disables autosuspend for these devices, which is similarly broken on these.
>>>> 
>>>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't
>>>> think so -- I'm using one now.
>>> 
>>> And have you manually enabled USB autosuspend for it, or are you
>>> running something which might have done so, e.g. powertop --auto-tune ?
>>> 
>>> Because if you did not do that then you're already not using autosuspend
>>> for your QCA devices and this patch will change nothing.
>> I use a set of udev rules that manually whitelist devices for
>> autosuspend. You can see it here:
>> https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83
>> You'll find at least one Rome chip in there.
> >
>>>> Thus, this is a poor solution, which
>>>> negatively affects my systems. However, I see that this patch was
>>>> applied regardless...
>>> 
>>> Note that there already is a quirk to handle broken suspend/resume
>>> behavior on ALL QCA devices in older kernels. Also note that the
>> Yes, and the quirk was broken, and I made sure it got reverted when it
>> broke my devices ;)
> 
> Note that the revert for this:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d06d5895c15
> 
> Says: "This commit causes a regression on some QCA ROME chips. The USB device reset happens
> in btusb_open(), hence firmware loading gets interrupted."
> 
> And says:
> 
> "If we really want to reset the USB device, we need to do it before btusb_open(). Let's
> handle it in drivers/usb/core/quirks.c."
> 
> It does not mention that the quirk is not necessary on some devices only
> that the implementation of it we had before had issues.
> 
> And the original commit of the quirk:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56
> 
> Says: "There's been numerous reported instances where BTUSB_QCA_ROME
> bluetooth controllers stop functioning upon resume from suspend."
> 
> So it may be platform specific but it is not just limited to 1 or
> 2 platforms I think.
> 
> Note I'm not saying that I don't believe you that the quirk is not
> necessary on your devices.
> 
>>> patches series which this commit builds on top of was already
>>> setting USB_QUIRK_RESET_RESUME for some devices in
>>> usb/core/quirks.c.
>>> 
>>> All my commit does is instead of duplicating all the QCA USB-ids in
>>> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME
>>> to btusb.c so that we don't need to duplicate the USB-id tables.
>> I was slightly more OK with marking specific IDs as broken, because
>> those IDs didn't happen to be ones that I knew were currently working.
>> Now you're breaking my systems again. But this time, it's more subtle
>> because bluetooth will still work, but we just suck more power leaving
>> our USB port active all the time.
> 
> I see, sorry about that. Ok, so we are going to need to make the
> reset-on-resume quirking more fine grained. I can see 3 ways to do this:
> 
> 1) Make it a separate per usb-id BTUSB_RESET_RESUME flag in the
> usb_device_id table inside btusb.c (I still believe duplicating most
> ids to usb/core/quirks.c is a bad idea).
> 
> 2) Use dmi based whitelisting to opt out of reset-resume behavior on
> QCA btusb devices.
> 
> 3) Use dmi based blacklisting which enables reset-resume behavior.
> 
> In retrospect I guess 3 would have been best, but if we do that now
> it will cause regressions.
> 
> I guess we should go with 1. adding the BTUSB_RESET_RESUME to all the
> QCA usb-ids except for the ones where you know things work and which
> only seem to be used in working devices (based on you not having
> objections against the additions of the quirk for some ids to
> drivers/usb/core/quirks.c).
> 
> And if then those usb-ids do turn out to have broken suspend on
> on some devices too I guess we need to move to 2.

actually if this is really platform related as Qualcomm is indicating, then we should just go with 3) and the two platforms that previously added quirks to usb/core/quirks.c and blacklist these. I am all for figuring out what is going on here. So lets blacklist these and see how this goes. Maybe there are only two bad platforms out there and we are making too much fuzz about this. Before we added quirks in the USB core these platforms were just plain broken as well. So not much different situation than before. We need to push the DMI blacklisting back into -stable as well and that means any impact of a 3rd broken platform briefly working and then be broken again is slim and also fixable via -stable.

I addition we can add a module option to btusb.ko that allows us to force this flag to be set. That way testing this is easy on a vendor kernel. A bunch of the btusb quirks (and also core quirks) can be tested via module options or debugfs.

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Hans de Goede]

Hi,

On 16-02-18 12:45, Marcel Holtmann wrote:
> Hi Hans,
> 
>>>>>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
>>>>>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
>>>>>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>>>>>>
>>>>>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
>>>>>> has several issues (see the original commit message). An added advantage
>>>>>> of moving over to the USB-core reset-resume handling is that it also
>>>>>> disables autosuspend for these devices, which is similarly broken on these.
>>>>>
>>>>> Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't
>>>>> think so -- I'm using one now.
>>>>
>>>> And have you manually enabled USB autosuspend for it, or are you
>>>> running something which might have done so, e.g. powertop --auto-tune ?
>>>>
>>>> Because if you did not do that then you're already not using autosuspend
>>>> for your QCA devices and this patch will change nothing.
>>> I use a set of udev rules that manually whitelist devices for
>>> autosuspend. You can see it here:
>>> https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83
>>> You'll find at least one Rome chip in there.
>>>
>>>>> Thus, this is a poor solution, which
>>>>> negatively affects my systems. However, I see that this patch was
>>>>> applied regardless...
>>>>
>>>> Note that there already is a quirk to handle broken suspend/resume
>>>> behavior on ALL QCA devices in older kernels. Also note that the
>>> Yes, and the quirk was broken, and I made sure it got reverted when it
>>> broke my devices ;)
>>
>> Note that the revert for this:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d06d5895c15
>>
>> Says: "This commit causes a regression on some QCA ROME chips. The USB device reset happens
>> in btusb_open(), hence firmware loading gets interrupted."
>>
>> And says:
>>
>> "If we really want to reset the USB device, we need to do it before btusb_open(). Let's
>> handle it in drivers/usb/core/quirks.c."
>>
>> It does not mention that the quirk is not necessary on some devices only
>> that the implementation of it we had before had issues.
>>
>> And the original commit of the quirk:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56
>>
>> Says: "There's been numerous reported instances where BTUSB_QCA_ROME
>> bluetooth controllers stop functioning upon resume from suspend."
>>
>> So it may be platform specific but it is not just limited to 1 or
>> 2 platforms I think.
>>
>> Note I'm not saying that I don't believe you that the quirk is not
>> necessary on your devices.
>>
>>>> patches series which this commit builds on top of was already
>>>> setting USB_QUIRK_RESET_RESUME for some devices in
>>>> usb/core/quirks.c.
>>>>
>>>> All my commit does is instead of duplicating all the QCA USB-ids in
>>>> usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME
>>>> to btusb.c so that we don't need to duplicate the USB-id tables.
>>> I was slightly more OK with marking specific IDs as broken, because
>>> those IDs didn't happen to be ones that I knew were currently working.
>>> Now you're breaking my systems again. But this time, it's more subtle
>>> because bluetooth will still work, but we just suck more power leaving
>>> our USB port active all the time.
>>
>> I see, sorry about that. Ok, so we are going to need to make the
>> reset-on-resume quirking more fine grained. I can see 3 ways to do this:
>>
>> 1) Make it a separate per usb-id BTUSB_RESET_RESUME flag in the
>> usb_device_id table inside btusb.c (I still believe duplicating most
>> ids to usb/core/quirks.c is a bad idea).
>>
>> 2) Use dmi based whitelisting to opt out of reset-resume behavior on
>> QCA btusb devices.
>>
>> 3) Use dmi based blacklisting which enables reset-resume behavior.
>>
>> In retrospect I guess 3 would have been best, but if we do that now
>> it will cause regressions.
>>
>> I guess we should go with 1. adding the BTUSB_RESET_RESUME to all the
>> QCA usb-ids except for the ones where you know things work and which
>> only seem to be used in working devices (based on you not having
>> objections against the additions of the quirk for some ids to
>> drivers/usb/core/quirks.c).
>>
>> And if then those usb-ids do turn out to have broken suspend on
>> on some devices too I guess we need to move to 2.
> 
> actually if this is really platform related as Qualcomm is indicating, then we should just go with 3) and the two platforms that previously added quirks to usb/core/quirks.c and blacklist these. I am all for figuring out what is going on here. So lets blacklist these and see how this goes. Maybe there are only two bad platforms out there and we are making too much fuzz about this. Before we added quirks in the USB core these platforms were just plain broken as well. So not much different situation than before. We need to push the DMI blacklisting back into -stable as well and that means any impact of a 3rd broken platform briefly working and then be broken again is slim and also fixable via -stable.

Ok, I've asked the reporter of:

https://bugzilla.redhat.com/show_bug.cgi?id=1514836

Which is how I got involved in this to provide DMI data and
I will whip up a patch for his machine and ask him to test
and once that is done submit upstream. This is the only machine
I'm aware of though and looking at the history of this I think
there will be others.

> I addition we can add a module option to btusb.ko that allows us to force this flag to be set. That way testing this is easy on a vendor kernel. A bunch of the btusb quirks (and also core quirks) can be tested via module options or debugfs.

Yes a module option for this is a good idea.

Regards,

Hans

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Brian Norris]

Hi,

On Fri, Feb 16, 2018 at 01:10:20PM +0100, Hans de Goede wrote:
> On 16-02-18 12:45, Marcel Holtmann wrote:
> > Hi Hans,
> > 
> > > > > > > Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
> > > > > > > removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
> > > > > > > instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
> > > > > > > 
> > > > > > > This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
> > > > > > > has several issues (see the original commit message). An added advantage
> > > > > > > of moving over to the USB-core reset-resume handling is that it also
> > > > > > > disables autosuspend for these devices, which is similarly broken on these.
> > > > > > 
> > > > > > Wait, is autosuspend actually broken for all QCA Rome chipsets? I don't
> > > > > > think so -- I'm using one now.
> > > > > 
> > > > > And have you manually enabled USB autosuspend for it, or are you
> > > > > running something which might have done so, e.g. powertop --auto-tune ?
> > > > > 
> > > > > Because if you did not do that then you're already not using autosuspend
> > > > > for your QCA devices and this patch will change nothing.
> > > > I use a set of udev rules that manually whitelist devices for
> > > > autosuspend. You can see it here:
> > > > https://chromium.googlesource.com/chromiumos/platform2/+/43728a93f6de137006c6b92fbb2a7cc4f353c9bf/power_manager/udev/gen_autosuspend_rules.py#83
> > > > You'll find at least one Rome chip in there.
> > > > 
> > > > > > Thus, this is a poor solution, which
> > > > > > negatively affects my systems. However, I see that this patch was
> > > > > > applied regardless...
> > > > > 
> > > > > Note that there already is a quirk to handle broken suspend/resume
> > > > > behavior on ALL QCA devices in older kernels. Also note that the
> > > > Yes, and the quirk was broken, and I made sure it got reverted when it
> > > > broke my devices ;)
> > > 
> > > Note that the revert for this:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d06d5895c15
> > > 
> > > Says: "This commit causes a regression on some QCA ROME chips. The USB device reset happens
> > > in btusb_open(), hence firmware loading gets interrupted."
> > > 
> > > And says:
> > > 
> > > "If we really want to reset the USB device, we need to do it before btusb_open(). Let's
> > > handle it in drivers/usb/core/quirks.c."

Just a note: I didn't write that part ;) I also didn't have evidence to
say that *no* QCA chipset was broken. I just knew that on my platform
with my QCA ROME chipset, there was no problem.

> > > It does not mention that the quirk is not necessary on some devices only
> > > that the implementation of it we had before had issues.
> > > 
> > > And the original commit of the quirk:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56
> > > 
> > > Says: "There's been numerous reported instances where BTUSB_QCA_ROME
> > > bluetooth controllers stop functioning upon resume from suspend."
> > > 
> > > So it may be platform specific but it is not just limited to 1 or
> > > 2 platforms I think.

Understood for most of the above. But I'll stick a "citation needed" on
the "numerous reported instances" claim. As I note below, I believe that
is the crux of the problem: understanding what platforms actually
reported this problem. All attempts to "fix" it have broken things in
one way or another, which have just caused noise since then.

> > > Note I'm not saying that I don't believe you that the quirk is not
> > > necessary on your devices.
> > > 
> > > > > patches series which this commit builds on top of was already
> > > > > setting USB_QUIRK_RESET_RESUME for some devices in
> > > > > usb/core/quirks.c.
> > > > > 
> > > > > All my commit does is instead of duplicating all the QCA USB-ids in
> > > > > usb/core/quirks.c, move the setting of USB_QUIRK_RESET_RESUME
> > > > > to btusb.c so that we don't need to duplicate the USB-id tables.
> > > > I was slightly more OK with marking specific IDs as broken, because
> > > > those IDs didn't happen to be ones that I knew were currently working.
> > > > Now you're breaking my systems again. But this time, it's more subtle
> > > > because bluetooth will still work, but we just suck more power leaving
> > > > our USB port active all the time.
> > > 
> > > I see, sorry about that. Ok, so we are going to need to make the
> > > reset-on-resume quirking more fine grained. I can see 3 ways to do this:
> > > 
> > > 1) Make it a separate per usb-id BTUSB_RESET_RESUME flag in the
> > > usb_device_id table inside btusb.c (I still believe duplicating most
> > > ids to usb/core/quirks.c is a bad idea).
> > > 
> > > 2) Use dmi based whitelisting to opt out of reset-resume behavior on
> > > QCA btusb devices.
> > > 
> > > 3) Use dmi based blacklisting which enables reset-resume behavior.
> > > 
> > > In retrospect I guess 3 would have been best, but if we do that now
> > > it will cause regressions.
> > > 
> > > I guess we should go with 1. adding the BTUSB_RESET_RESUME to all the
> > > QCA usb-ids except for the ones where you know things work and which
> > > only seem to be used in working devices (based on you not having
> > > objections against the additions of the quirk for some ids to
> > > drivers/usb/core/quirks.c).
> > > 
> > > And if then those usb-ids do turn out to have broken suspend on
> > > on some devices too I guess we need to move to 2.
> > 
> > actually if this is really platform related as Qualcomm is indicating, then we should just go with 3) and the two platforms that previously added quirks to usb/core/quirks.c and blacklist these. I am all for figuring out what is going on here. So lets blacklist these and see how this goes. Maybe there are only two bad platforms out there and we are making too much fuzz about this. Before we added quirks in the USB core these platforms were just plain broken as well. So not much different situation than before. We need to push the DMI blacklisting back into -stable as well and that means any impact of a 3rd broken platform briefly working and then be broken again is slim and also fixable via -stable.
> 
> Ok, I've asked the reporter of:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1514836

Are you even sure that this reporter is seeing the original symptom at
all (BT loses power, and therefore firmware)? Their report shows them
running 4.15, which had this commit:

fd865802c66b Bluetooth: btusb: fix QCA Rome suspend/resume

which is admittedly completely broken. It breaks even perfectly working
BT/USB devices, like mine. That's where I first complained, and we got
this into 4.16-rc1:

7d06d5895c15 Revert "Bluetooth: btusb: fix QCA Rome suspend/resume"

Isn't it possible your reporter has no further problem, and none if this
is actually important to them? I'd just caution you to be careful before
assuming you need to add blacklist info for their DMI...

As I read it, you need to investigate who are the "numerous reported
instances" that generated commit fd865802c66b in the first place. That's
where this mess started, IIUC.

But otherwise, yes, option 3 sounds OK. FWIW, my systems are ARM based
and don't have DMI data, so option 2 wouldn't work.

> Which is how I got involved in this to provide DMI data and
> I will whip up a patch for his machine and ask him to test
> and once that is done submit upstream. This is the only machine
> I'm aware of though and looking at the history of this I think
> there will be others.
> 
> > I addition we can add a module option to btusb.ko that allows us to force this flag to be set. That way testing this is easy on a vendor kernel. A bunch of the btusb quirks (and also core quirks) can be tested via module options or debugfs.
> 
> Yes a module option for this is a good idea.

That too. Or, would be nice to be able to force it either way. So if
someone erroneously sticks the flag in this driver (yet again), we can
easily undo that.

Brian

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Hans de Goede]

HI,

On 16-02-18 18:59, Brian Norris wrote:
> Hi,
> 
> On Fri, Feb 16, 2018 at 01:10:20PM +0100, Hans de Goede wrote:
>> On 16-02-18 12:45, Marcel Holtmann wrote:

<snip>

>>> actually if this is really platform related as Qualcomm is indicating, then we should just go with 3) and the two platforms that previously added quirks to usb/core/quirks.c and blacklist these. I am all for figuring out what is going on here. So lets blacklist these and see how this goes. Maybe there are only two bad platforms out there and we are making too much fuzz about this. Before we added quirks in the USB core these platforms were just plain broken as well. So not much different situation than before. We need to push the DMI blacklisting back into -stable as well and that means any impact of a 3rd broken platform briefly working and then be broken again is slim and also fixable via -stable.
>>
>> Ok, I've asked the reporter of:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1514836
> 
> Are you even sure that this reporter is seeing the original symptom at
> all (BT loses power, and therefore firmware)? Their report shows them
> running 4.15, which had this commit:
> 
> fd865802c66b Bluetooth: btusb: fix QCA Rome suspend/resume
> 
> which is admittedly completely broken. It breaks even perfectly working
> BT/USB devices, like mine. That's where I first complained, and we got
> this into 4.16-rc1:
> 
> 7d06d5895c15 Revert "Bluetooth: btusb: fix QCA Rome suspend/resume"
> 
> Isn't it possible your reporter has no further problem, and none if this
> is actually important to them? I'd just caution you to be careful before
> assuming you need to add blacklist info for their DMI...

Thanks, that is a good question. His problems only started when I
enabled usb-autosuspend by default for btusb devices and he got things
working by adding "btusb.enable_autosuspend=n" on the kernel commandline,
so he was not hitting the firmware loading race introduced by
fd865802c66b and runtime suspend/resume is really broken for him.

> As I read it, you need to investigate who are the "numerous reported
> instances" that generated commit fd865802c66b in the first place. That's
> where this mess started, IIUC. >
> But otherwise, yes, option 3 sounds OK. FWIW, my systems are ARM based
> and don't have DMI data, so option 2 wouldn't work.

Right I think we all agree that the new plan now is to go back to
QCA behaving normally wrt (runtime) suspend/resume and then set the
USB-core RESET_RESUME quirk (which does not have the firmware
loading race) based on a DMI blacklist.

I only have the one report for which I will write a patch implementing
this new policy soonish. And Kai-Heng Feng has another report which
might even be the machine. I certainly would not be surprised if it
is another Lenovo machine.

Regards,

Hans

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Brian Norris]

Hi Hans,

Sorry if I'm a little slow to follow up here. This hasn't been my
top priority...

On Mon, Feb 19, 2018 at 11:17:24AM +0100, Hans de Goede wrote:
> On 16-02-18 18:59, Brian Norris wrote:
> > On Fri, Feb 16, 2018 at 01:10:20PM +0100, Hans de Goede wrote:
> > > Ok, I've asked the reporter of:
> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1514836
> > 
> > Are you even sure that this reporter is seeing the original symptom at
> > all (BT loses power, and therefore firmware)? Their report shows them
> > running 4.15, which had this commit:
> > 
> > fd865802c66b Bluetooth: btusb: fix QCA Rome suspend/resume
> > 
> > which is admittedly completely broken. It breaks even perfectly working
> > BT/USB devices, like mine. That's where I first complained, and we got
> > this into 4.16-rc1:
> > 
> > 7d06d5895c15 Revert "Bluetooth: btusb: fix QCA Rome suspend/resume"
> > 
> > Isn't it possible your reporter has no further problem, and none if this
> > is actually important to them? I'd just caution you to be careful before
> > assuming you need to add blacklist info for their DMI...
> 
> Thanks, that is a good question. His problems only started when I
> enabled usb-autosuspend by default for btusb devices and he got things
> working by adding "btusb.enable_autosuspend=n" on the kernel commandline,
> so he was not hitting the firmware loading race introduced by
> fd865802c66b and runtime suspend/resume is really broken for him.

Hmm? I'm not sure I completely follow here when you say "he was not
hitting the firmware loading race". If things were functioning fine with
system suspend (but not with autosuspend), then he's not seeing the
controller (quoting commit fd865802c66b) "losing power during suspend".

So, that would suggest he could only be seeing the race (as I was), and
that his machine does not deserve a RESET_RESUME quirk?

Or maybe I'm really misunderstanding.

> > As I read it, you need to investigate who are the "numerous reported
> > instances" that generated commit fd865802c66b in the first place. That's
> > where this mess started, IIUC. >
> > But otherwise, yes, option 3 sounds OK. FWIW, my systems are ARM based
> > and don't have DMI data, so option 2 wouldn't work.
> 
> Right I think we all agree that the new plan now is to go back to
> QCA behaving normally wrt (runtime) suspend/resume and then set the
> USB-core RESET_RESUME quirk (which does not have the firmware
> loading race) based on a DMI blacklist.
> 
> I only have the one report for which I will write a patch implementing
> this new policy soonish. And Kai-Heng Feng has another report which
> might even be the machine. I certainly would not be surprised if it
> is another Lenovo machine.

It seems like you folks moved forward on that one. Thanks.

Brian

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Hans de Goede]

Hi,

On 23-02-18 04:12, Brian Norris wrote:
> Hi Hans,
> 
> Sorry if I'm a little slow to follow up here. This hasn't been my
> top priority...
> 
> On Mon, Feb 19, 2018 at 11:17:24AM +0100, Hans de Goede wrote:
>> On 16-02-18 18:59, Brian Norris wrote:
>>> On Fri, Feb 16, 2018 at 01:10:20PM +0100, Hans de Goede wrote:
>>>> Ok, I've asked the reporter of:
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1514836
>>>
>>> Are you even sure that this reporter is seeing the original symptom at
>>> all (BT loses power, and therefore firmware)? Their report shows them
>>> running 4.15, which had this commit:
>>>
>>> fd865802c66b Bluetooth: btusb: fix QCA Rome suspend/resume
>>>
>>> which is admittedly completely broken. It breaks even perfectly working
>>> BT/USB devices, like mine. That's where I first complained, and we got
>>> this into 4.16-rc1:
>>>
>>> 7d06d5895c15 Revert "Bluetooth: btusb: fix QCA Rome suspend/resume"
>>>
>>> Isn't it possible your reporter has no further problem, and none if this
>>> is actually important to them? I'd just caution you to be careful before
>>> assuming you need to add blacklist info for their DMI...
>>
>> Thanks, that is a good question. His problems only started when I
>> enabled usb-autosuspend by default for btusb devices and he got things
>> working by adding "btusb.enable_autosuspend=n" on the kernel commandline,
>> so he was not hitting the firmware loading race introduced by
>> fd865802c66b and runtime suspend/resume is really broken for him.
> 
> Hmm? I'm not sure I completely follow here when you say "he was not
> hitting the firmware loading race". If things were functioning fine with
> system suspend (but not with autosuspend), then he's not seeing the
> controller (quoting commit fd865802c66b) "losing power during suspend".

He was running a kernel with the original "fd865802c66b Bluetooth: btusb:
fix QCA Rome suspend/resume" commit, which fixes regular suspend for
devices which are "losing power during suspend", but does nothing for
runtime-suspend.

He ran tests both with and without runtime-pm enabled with that same kernel
and he needed to disable runtime-pm to get working bluetooth.

> So, that would suggest he could only be seeing the race (as I was), and
> that his machine does not deserve a RESET_RESUME quirk?

I hope my above answer helps to clarify why I believe the quirk is
necessary on his machine.

Regards,

Hans


> 
> Or maybe I'm really misunderstanding.
> 
>>> As I read it, you need to investigate who are the "numerous reported
>>> instances" that generated commit fd865802c66b in the first place. That's
>>> where this mess started, IIUC. >
>>> But otherwise, yes, option 3 sounds OK. FWIW, my systems are ARM based
>>> and don't have DMI data, so option 2 wouldn't work.
>>
>> Right I think we all agree that the new plan now is to go back to
>> QCA behaving normally wrt (runtime) suspend/resume and then set the
>> USB-core RESET_RESUME quirk (which does not have the firmware
>> loading race) based on a DMI blacklist.
>>
>> I only have the one report for which I will write a patch implementing
>> this new policy soonish. And Kai-Heng Feng has another report which
>> might even be the machine. I certainly would not be surprised if it
>> is another Lenovo machine.
> 
> It seems like you folks moved forward on that one. Thanks.
> 
> Brian
> 

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Brian Norris]

On Thu, Feb 22, 2018 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> On 23-02-18 04:12, Brian Norris wrote:
>> Hmm? I'm not sure I completely follow here when you say "he was not
>> hitting the firmware loading race". If things were functioning fine with
>> system suspend (but not with autosuspend), then he's not seeing the
>> controller (quoting commit fd865802c66b) "losing power during suspend".
>
>
> He was running a kernel with the original "fd865802c66b Bluetooth: btusb:
> fix QCA Rome suspend/resume" commit, which fixes regular suspend for
> devices which are "losing power during suspend", but does nothing for
> runtime-suspend.
>
> He ran tests both with and without runtime-pm enabled with that same kernel
> and he needed to disable runtime-pm to get working bluetooth.

Did he ever test with commit fd865802c66b reverted?

My symptoms were exactly the same as you described. BT was broken as
of v4.14 if I had runtime suspend enabled. Things were fine if I
either (a) reverted the patch or (b) disabled runtime suspend. I
obviously preferred (a), which is why I continued to complain :)

Did your tester ever try (a)? If not, then I don't think you've really
ensured that he really needed a "fixed" version; he may not have
needed the patch at all.

Or an alternative question: did that system work on an older Fedora
release (and presumably an older kernel)? If so, then he probably also
did not need that patch.

>> So, that would suggest he could only be seeing the race (as I was), and
>> that his machine does not deserve a RESET_RESUME quirk?
>
>
> I hope my above answer helps to clarify why I believe the quirk is
> necessary on his machine.

I'm sorry, but no it doesn't. If anything, it suggests to me even more
that it may not have been necessary.

But hey, I'm not personally going to notice if random Lenovo laptops
are wasting power.

Brian

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Hans de Goede]

Hi,

On 27-02-18 03:29, Brian Norris wrote:
> On Thu, Feb 22, 2018 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> On 23-02-18 04:12, Brian Norris wrote:
>>> Hmm? I'm not sure I completely follow here when you say "he was not
>>> hitting the firmware loading race". If things were functioning fine with
>>> system suspend (but not with autosuspend), then he's not seeing the
>>> controller (quoting commit fd865802c66b) "losing power during suspend".
>>
>>
>> He was running a kernel with the original "fd865802c66b Bluetooth: btusb:
>> fix QCA Rome suspend/resume" commit, which fixes regular suspend for
>> devices which are "losing power during suspend", but does nothing for
>> runtime-suspend.
>>
>> He ran tests both with and without runtime-pm enabled with that same kernel
>> and he needed to disable runtime-pm to get working bluetooth.
> 
> Did he ever test with commit fd865802c66b reverted?
> 
> My symptoms were exactly the same as you described. BT was broken as
> of v4.14 if I had runtime suspend enabled. Things were fine if I
> either (a) reverted the patch or (b) disabled runtime suspend. I
> obviously preferred (a), which is why I continued to complain :)
> 
> Did your tester ever try (a)? If not, then I don't think you've really
> ensured that he really needed a "fixed" version; he may not have
> needed the patch at all.
> 
> Or an alternative question: did that system work on an older Fedora
> release (and presumably an older kernel)? If so, then he probably also
> did not need that patch.
> 
>>> So, that would suggest he could only be seeing the race (as I was), and
>>> that his machine does not deserve a RESET_RESUME quirk?
>>
>>
>> I hope my above answer helps to clarify why I believe the quirk is
>> necessary on his machine.
> 
> I'm sorry, but no it doesn't. If anything, it suggests to me even more
> that it may not have been necessary.

Ok, I've started another test-kernel build for the reporter this time
without any quirks at all and I've asked him to test.

Regards,

Hans

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Hans de Goede]

Hi,

On 27-02-18 15:07, Hans de Goede wrote:
> Hi,
> 
> On 27-02-18 03:29, Brian Norris wrote:
>> On Thu, Feb 22, 2018 at 11:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> On 23-02-18 04:12, Brian Norris wrote:
>>>> Hmm? I'm not sure I completely follow here when you say "he was not
>>>> hitting the firmware loading race". If things were functioning fine with
>>>> system suspend (but not with autosuspend), then he's not seeing the
>>>> controller (quoting commit fd865802c66b) "losing power during suspend".
>>>
>>>
>>> He was running a kernel with the original "fd865802c66b Bluetooth: btusb:
>>> fix QCA Rome suspend/resume" commit, which fixes regular suspend for
>>> devices which are "losing power during suspend", but does nothing for
>>> runtime-suspend.
>>>
>>> He ran tests both with and without runtime-pm enabled with that same kernel
>>> and he needed to disable runtime-pm to get working bluetooth.
>>
>> Did he ever test with commit fd865802c66b reverted?
>>
>> My symptoms were exactly the same as you described. BT was broken as
>> of v4.14 if I had runtime suspend enabled. Things were fine if I
>> either (a) reverted the patch or (b) disabled runtime suspend. I
>> obviously preferred (a), which is why I continued to complain :)
>>
>> Did your tester ever try (a)? If not, then I don't think you've really
>> ensured that he really needed a "fixed" version; he may not have
>> needed the patch at all.
>>
>> Or an alternative question: did that system work on an older Fedora
>> release (and presumably an older kernel)? If so, then he probably also
>> did not need that patch.
>>
>>>> So, that would suggest he could only be seeing the race (as I was), and
>>>> that his machine does not deserve a RESET_RESUME quirk?
>>>
>>>
>>> I hope my above answer helps to clarify why I believe the quirk is
>>> necessary on his machine.
>>
>> I'm sorry, but no it doesn't. If anything, it suggests to me even more
>> that it may not have been necessary.
> 
> Ok, I've started another test-kernel build for the reporter this time
> without any quirks at all and I've asked him to test.

You were right, the Yoga 920 works fine without any quirks, thank you
for being persistent on getting this tested properly.

I will submit a patch dropping the Yoga 920 from the
btusb_needs_reset_resume_table.

Regards,

Hans

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Leif Liddy]

Sorry for being a bit slow to respond to this.

I'm the original author of commit:
https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56

> Says: "There's been numerous reported instances where BTUSB_QCA_ROME
> bluetooth controllers stop functioning upon resume from suspend."

> So it may be platform specific but it is not just limited to 1 or
>2 platforms I think.

This issue might very well be limited to just a few platforms.
Here's a link to the original bug report I submitted.

https://bugzilla.kernel.org/show_bug.cgi?id=193571

There was a number of people who reported they were having similar
issues with various QCA Rome bluetooth devices.
The userspace fix that seemed to work for everyone was resetting the usb device.

ie

#!/bin/bash
echo 0 > /sys/bus/usb/devices/1-4/authorized
echo 1 > /sys/bus/usb/devices/1-4/authorized

or

#!/usr/bin/python
from usb.core import find as finddev
dev = finddev(idVendor=0x0cf3, idProduct=0xe300)
dev.reset()

It's difficult to ascertain what the underlying issue was for each
person (and associated device) who commented. I can only speak
authoritatively for my own device.
It's a Samsung ATIV Book 9 12.2 (2015) laptop that contains an
integrated bluetooth controller that's attached to the internal usb
bus.

# lsusb
Bus 001 Device 005: ID 0cf3:e300 Atheros Communications, Inc.

After suspending the laptop for an extended length of time (sometimes
it would take an hour or two before issue occurred), and resuming
--the bluetooth controller would not come back on-line. After reading
through various forums, I suspected the issue I was experiencing was
due to the bluetooth device losing it's firmware during suspend.
The original patch I created did fix the issue. (I do realize that
targeting all QCA Rome chipset's was the wrong decision). Hans de
Goede's rewritten version (commit: 61f5acea8737) also worked for my
device. Since, some people think that the underlying issue may been
fixed elsewhere in the kernel. I'm going to remove Hans' commit and
see if the issue persists (with kernel 4.15.6). I'll report back
tomorrow with the results.


-Leif

On Wed, Feb 28, 2018 at 11:54 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 27-02-18 15:07, Hans de Goede wrote:
>>
>> Hi,
>>
>> On 27-02-18 03:29, Brian Norris wrote:
>>>
>>> On Thu, Feb 22, 2018 at 11:14 PM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> On 23-02-18 04:12, Brian Norris wrote:
>>>>>
>>>>> Hmm? I'm not sure I completely follow here when you say "he was not
>>>>> hitting the firmware loading race". If things were functioning fine
>>>>> with
>>>>> system suspend (but not with autosuspend), then he's not seeing the
>>>>> controller (quoting commit fd865802c66b) "losing power during suspend".
>>>>
>>>>
>>>>
>>>> He was running a kernel with the original "fd865802c66b Bluetooth:
>>>> btusb:
>>>> fix QCA Rome suspend/resume" commit, which fixes regular suspend for
>>>> devices which are "losing power during suspend", but does nothing for
>>>> runtime-suspend.
>>>>
>>>> He ran tests both with and without runtime-pm enabled with that same
>>>> kernel
>>>> and he needed to disable runtime-pm to get working bluetooth.
>>>
>>>
>>> Did he ever test with commit fd865802c66b reverted?
>>>
>>> My symptoms were exactly the same as you described. BT was broken as
>>> of v4.14 if I had runtime suspend enabled. Things were fine if I
>>> either (a) reverted the patch or (b) disabled runtime suspend. I
>>> obviously preferred (a), which is why I continued to complain :)
>>>
>>> Did your tester ever try (a)? If not, then I don't think you've really
>>> ensured that he really needed a "fixed" version; he may not have
>>> needed the patch at all.
>>>
>>> Or an alternative question: did that system work on an older Fedora
>>> release (and presumably an older kernel)? If so, then he probably also
>>> did not need that patch.
>>>
>>>>> So, that would suggest he could only be seeing the race (as I was), and
>>>>> that his machine does not deserve a RESET_RESUME quirk?
>>>>
>>>>
>>>>
>>>> I hope my above answer helps to clarify why I believe the quirk is
>>>> necessary on his machine.
>>>
>>>
>>> I'm sorry, but no it doesn't. If anything, it suggests to me even more
>>> that it may not have been necessary.
>>
>>
>> Ok, I've started another test-kernel build for the reporter this time
>> without any quirks at all and I've asked him to test.
>
>
> You were right, the Yoga 920 works fine without any quirks, thank you
> for being persistent on getting this tested properly.
>
> I will submit a patch dropping the Yoga 920 from the
> btusb_needs_reset_resume_table.
>
> Regards,
>
> Hans

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Leif Liddy]

Hans,

Everything is now working perfectly fine without commit 61f5acea8737.
I suspended the laptop for hours at a time, the bluetooth controller
comes up fine every time. Whatever the underlying issue was, it's been
resolved elsewhere in the kernel.  There's no longer any need to
implement any sort of RESET_RESUME logic for this device (0cf3:e300).
I appreciate all of the work that you and other developers do. Thanks
for staying on top of this issue!

-Leif


On Fri, Mar 9, 2018 at 1:56 AM, Leif Liddy <leif.linux@gmail.com> wrote:
> Sorry for being a bit slow to respond to this.
>
> I'm the original author of commit:
> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/drivers/bluetooth/btusb.c?id=fd865802c66bc451dc515ed89360f84376ce1a56
>
>> Says: "There's been numerous reported instances where BTUSB_QCA_ROME
>> bluetooth controllers stop functioning upon resume from suspend."
>
>> So it may be platform specific but it is not just limited to 1 or
>>2 platforms I think.
>
> This issue might very well be limited to just a few platforms.
> Here's a link to the original bug report I submitted.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=193571
>
> There was a number of people who reported they were having similar
> issues with various QCA Rome bluetooth devices.
> The userspace fix that seemed to work for everyone was resetting the usb device.
>
> ie
>
> #!/bin/bash
> echo 0 > /sys/bus/usb/devices/1-4/authorized
> echo 1 > /sys/bus/usb/devices/1-4/authorized
>
> or
>
> #!/usr/bin/python
> from usb.core import find as finddev
> dev = finddev(idVendor=0x0cf3, idProduct=0xe300)
> dev.reset()
>
> It's difficult to ascertain what the underlying issue was for each
> person (and associated device) who commented. I can only speak
> authoritatively for my own device.
> It's a Samsung ATIV Book 9 12.2 (2015) laptop that contains an
> integrated bluetooth controller that's attached to the internal usb
> bus.
>
> # lsusb
> Bus 001 Device 005: ID 0cf3:e300 Atheros Communications, Inc.
>
> After suspending the laptop for an extended length of time (sometimes
> it would take an hour or two before issue occurred), and resuming
> --the bluetooth controller would not come back on-line. After reading
> through various forums, I suspected the issue I was experiencing was
> due to the bluetooth device losing it's firmware during suspend.
> The original patch I created did fix the issue. (I do realize that
> targeting all QCA Rome chipset's was the wrong decision). Hans de
> Goede's rewritten version (commit: 61f5acea8737) also worked for my
> device. Since, some people think that the underlying issue may been
> fixed elsewhere in the kernel. I'm going to remove Hans' commit and
> see if the issue persists (with kernel 4.15.6). I'll report back
> tomorrow with the results.
>
>
> -Leif
>
> On Wed, Feb 28, 2018 at 11:54 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 27-02-18 15:07, Hans de Goede wrote:
>>>
>>> Hi,
>>>
>>> On 27-02-18 03:29, Brian Norris wrote:
>>>>
>>>> On Thu, Feb 22, 2018 at 11:14 PM, Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>>>>
>>>>> On 23-02-18 04:12, Brian Norris wrote:
>>>>>>
>>>>>> Hmm? I'm not sure I completely follow here when you say "he was not
>>>>>> hitting the firmware loading race". If things were functioning fine
>>>>>> with
>>>>>> system suspend (but not with autosuspend), then he's not seeing the
>>>>>> controller (quoting commit fd865802c66b) "losing power during suspend".
>>>>>
>>>>>
>>>>>
>>>>> He was running a kernel with the original "fd865802c66b Bluetooth:
>>>>> btusb:
>>>>> fix QCA Rome suspend/resume" commit, which fixes regular suspend for
>>>>> devices which are "losing power during suspend", but does nothing for
>>>>> runtime-suspend.
>>>>>
>>>>> He ran tests both with and without runtime-pm enabled with that same
>>>>> kernel
>>>>> and he needed to disable runtime-pm to get working bluetooth.
>>>>
>>>>
>>>> Did he ever test with commit fd865802c66b reverted?
>>>>
>>>> My symptoms were exactly the same as you described. BT was broken as
>>>> of v4.14 if I had runtime suspend enabled. Things were fine if I
>>>> either (a) reverted the patch or (b) disabled runtime suspend. I
>>>> obviously preferred (a), which is why I continued to complain :)
>>>>
>>>> Did your tester ever try (a)? If not, then I don't think you've really
>>>> ensured that he really needed a "fixed" version; he may not have
>>>> needed the patch at all.
>>>>
>>>> Or an alternative question: did that system work on an older Fedora
>>>> release (and presumably an older kernel)? If so, then he probably also
>>>> did not need that patch.
>>>>
>>>>>> So, that would suggest he could only be seeing the race (as I was), and
>>>>>> that his machine does not deserve a RESET_RESUME quirk?
>>>>>
>>>>>
>>>>>
>>>>> I hope my above answer helps to clarify why I believe the quirk is
>>>>> necessary on his machine.
>>>>
>>>>
>>>> I'm sorry, but no it doesn't. If anything, it suggests to me even more
>>>> that it may not have been necessary.
>>>
>>>
>>> Ok, I've started another test-kernel build for the reporter this time
>>> without any quirks at all and I've asked him to test.
>>
>>
>> You were right, the Yoga 920 works fine without any quirks, thank you
>> for being persistent on getting this tested properly.
>>
>> I will submit a patch dropping the Yoga 920 from the
>> btusb_needs_reset_resume_table.
>>
>> Regards,
>>
>> Hans

Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version

[Kai-Heng Feng]

> On 16 Feb 2018, at 8:10 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
> 
> Ok, I've asked the reporter of:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1514836
> 
> Which is how I got involved in this to provide DMI data and
> I will whip up a patch for his machine and ask him to test
> and once that is done submit upstream. This is the only machine
> I'm aware of though and looking at the history of this I think
> there will be others.

I have one machine needs the quirk. I’ll also send a patch to
match its DMI.

Kai-Heng

> 
>> I addition we can add a module option to btusb.ko that allows us to force this flag to be set. That way testing this is easy on a vendor kernel. A bunch of the btusb quirks (and also core quirks) can be tested via module options or debugfs.
> 
> Yes a module option for this is a good idea.
> 
> Regards,
> 
> Hans
> 
>