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