Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Kai Heng Feng]

> On 20 Dec 2017, at 7:11 AM, Brian Norris  wrote:
> 
> Hi Kai,
> 
> On Tue, Dec 19, 2017 at 12:28:17PM +0800, Kai Heng Feng wrote:
>>> On 19 Dec 2017, at 2:13 AM, Brian Norris  wrote:
>>> On Mon, Dec 18, 2017 at 12:43:48PM +0100, Greg Kroah-Hartman wrote:
 On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
> We identified the above patch as the culprit, in combination with USB
> autosuspend being enabled for the Bluetooth controller.
> 
> We found that the following (recent) upstream patch fixes the issue on
> most devices (we are still investigating one case on a specific device):
>>> 
>>> A key word to highlight here is "most". I have at least one device that
>>> is consistently having problems even with both patches included; the
>>> only way things work reliably so far is to simply revert the $subject
>>> patch in 4.4.x.
>> 
>> The problem we have is that the QCA Rome Bluetooth stops working
>> after system S3. The reset resume quirk workaround the issue on both
>> mainline and 4.4.x (at least for me).
> 
> Understood. But that's not the case for all systems, and on some, you're
> regressing functionality.

Yes. So reverting the commit is a reasonable path we should take.

> 
> Many systems keep power enabled for system suspend, and so that "fix" is
> not necessary for them. It's also not very precise, because it seems to
> act in many cases where it need not -- for one, it takes effect on *all*
> resume attempts, even resuming from autosuspend. I really doubt your
> system is losing USB power in S0 + USB autosuspend?

No, USB power is on. Not seeing my XHCI gets put to D3cold.

> 
> So, if you really need this patch for some systems, it seems like it
> should be much better targeted. You're currently adding a lot of
> unnecessary overhead and fragility.

You are right. Applying this unconditionally to all QCA Rome may be
overkill.

> 
>>> I'll try to investigate further, since this result is a bit confusing
>>> still. If there's a real problem with the patch (and I suspect there
>>> might be), it probably shouldn't be in mainline either…
>> 
>> Do you have the same issue on mainline?
> 
> Refer to my note [1] :)
> 
> But because you asked, I did the work necessary to boot mainline on this
> system, and in fact, I see the same problem. I think that's enough
> reason to revert your patch in all branches (upstream, and any -stable
> branch that already took it).
> 
> To be clear, here's the kernel logs on 4.15-rc4+, where the 55-second
> mark is around where I try to power on and scan for BT devices (power-on
> and scan both fail):
> 
> [2.323475] usb 3-1: new full-speed USB device number 2 using ohci-platform
> ...
> [2.731719] usb 3-1: New USB device found, idVendor=0cf3, idProduct=e300
> ...
> [2.867870] usb 3-1: New USB device strings: Mfr=0, Product=0, 
> SerialNumber=0
> ...(udev doesn't run until here)...
> [   35.733139] usbcore: registered new interface driver btusb
> [   35.740912] Bluetooth: hci0: using rampatch file: 
> qca/rampatch_usb_0302.bin
> [   35.749216] Bluetooth: hci0: QCA: patch rome 0x302 build 0x3e8, firmware 
> rome 0x302 build 0x111
> ...
> [   35.816104] Bluetooth: hci0: using NVM file: qca/nvm_usb_0302.bin
> ...
> [   55.073503] usb 3-1: reset full-speed USB device number 2 using 
> ohci-platform
> ...
> [   57.772513] Bluetooth: hci0: urb ec39086b failed to resubmit (113)
> [   57.780217] Bluetooth: hci0: urb 66228fda failed to resubmit (113)
> 
> Whereas reverting the $subject patch yields no reset and URB failure;
> Bluetooth seems to work fine.

Thanks for your testing on mainline!

> 
> commit a0085f2510e8976614ad8f766b209448b385492f
> Author: Sukumar Ghorai 
> Date:   Wed Aug 16 14:46:55 2017 -0700
> 
>   Bluetooth: btusb: driver to enable the usb-wakeup feature
>>> [...]
> stable branches are currently broken for BTUSB_QCA_ROME with USB
> autosuspend enabled, since the above patch is not included (I only
> checked v4.4 and v4.9), so we probably want to integrate it.
 
 Now queued up, thanks for letting me know.
>>> 
>>> I think you have almost as much information as I do at this point, and
>>> I'll try to reply back here if I figure out anything more, so I'll leave
>>> you to your decisions. But I would personally revert, not backport more
>>> patches.
>>> 
>>> Among the reasons:
>>> (a) I have at least one device that does not work better with both
>>>   patches [1]
>>> (b) So far, I haven't seen any explanation why commit a0085f2510e8
>>>   should be a dependency for $subject patch; the closest I can imagine
>>>   would be that commit a0085f2510e8 could effectively neutralize
>>>   $subject patch -- if the device is marked as a wakeup source, then
>>>   it will never try to RESET on resume -- but that's still a fuzzy
>>>   signal; just because it's marked a wakeup source 

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Kai Heng Feng]

> On 20 Dec 2017, at 7:11 AM, Brian Norris  wrote:
> 
> Hi Kai,
> 
> On Tue, Dec 19, 2017 at 12:28:17PM +0800, Kai Heng Feng wrote:
>>> On 19 Dec 2017, at 2:13 AM, Brian Norris  wrote:
>>> On Mon, Dec 18, 2017 at 12:43:48PM +0100, Greg Kroah-Hartman wrote:
 On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
> We identified the above patch as the culprit, in combination with USB
> autosuspend being enabled for the Bluetooth controller.
> 
> We found that the following (recent) upstream patch fixes the issue on
> most devices (we are still investigating one case on a specific device):
>>> 
>>> A key word to highlight here is "most". I have at least one device that
>>> is consistently having problems even with both patches included; the
>>> only way things work reliably so far is to simply revert the $subject
>>> patch in 4.4.x.
>> 
>> The problem we have is that the QCA Rome Bluetooth stops working
>> after system S3. The reset resume quirk workaround the issue on both
>> mainline and 4.4.x (at least for me).
> 
> Understood. But that's not the case for all systems, and on some, you're
> regressing functionality.

Yes. So reverting the commit is a reasonable path we should take.

> 
> Many systems keep power enabled for system suspend, and so that "fix" is
> not necessary for them. It's also not very precise, because it seems to
> act in many cases where it need not -- for one, it takes effect on *all*
> resume attempts, even resuming from autosuspend. I really doubt your
> system is losing USB power in S0 + USB autosuspend?

No, USB power is on. Not seeing my XHCI gets put to D3cold.

> 
> So, if you really need this patch for some systems, it seems like it
> should be much better targeted. You're currently adding a lot of
> unnecessary overhead and fragility.

You are right. Applying this unconditionally to all QCA Rome may be
overkill.

> 
>>> I'll try to investigate further, since this result is a bit confusing
>>> still. If there's a real problem with the patch (and I suspect there
>>> might be), it probably shouldn't be in mainline either…
>> 
>> Do you have the same issue on mainline?
> 
> Refer to my note [1] :)
> 
> But because you asked, I did the work necessary to boot mainline on this
> system, and in fact, I see the same problem. I think that's enough
> reason to revert your patch in all branches (upstream, and any -stable
> branch that already took it).
> 
> To be clear, here's the kernel logs on 4.15-rc4+, where the 55-second
> mark is around where I try to power on and scan for BT devices (power-on
> and scan both fail):
> 
> [2.323475] usb 3-1: new full-speed USB device number 2 using ohci-platform
> ...
> [2.731719] usb 3-1: New USB device found, idVendor=0cf3, idProduct=e300
> ...
> [2.867870] usb 3-1: New USB device strings: Mfr=0, Product=0, 
> SerialNumber=0
> ...(udev doesn't run until here)...
> [   35.733139] usbcore: registered new interface driver btusb
> [   35.740912] Bluetooth: hci0: using rampatch file: 
> qca/rampatch_usb_0302.bin
> [   35.749216] Bluetooth: hci0: QCA: patch rome 0x302 build 0x3e8, firmware 
> rome 0x302 build 0x111
> ...
> [   35.816104] Bluetooth: hci0: using NVM file: qca/nvm_usb_0302.bin
> ...
> [   55.073503] usb 3-1: reset full-speed USB device number 2 using 
> ohci-platform
> ...
> [   57.772513] Bluetooth: hci0: urb ec39086b failed to resubmit (113)
> [   57.780217] Bluetooth: hci0: urb 66228fda failed to resubmit (113)
> 
> Whereas reverting the $subject patch yields no reset and URB failure;
> Bluetooth seems to work fine.

Thanks for your testing on mainline!

> 
> commit a0085f2510e8976614ad8f766b209448b385492f
> Author: Sukumar Ghorai 
> Date:   Wed Aug 16 14:46:55 2017 -0700
> 
>   Bluetooth: btusb: driver to enable the usb-wakeup feature
>>> [...]
> stable branches are currently broken for BTUSB_QCA_ROME with USB
> autosuspend enabled, since the above patch is not included (I only
> checked v4.4 and v4.9), so we probably want to integrate it.
 
 Now queued up, thanks for letting me know.
>>> 
>>> I think you have almost as much information as I do at this point, and
>>> I'll try to reply back here if I figure out anything more, so I'll leave
>>> you to your decisions. But I would personally revert, not backport more
>>> patches.
>>> 
>>> Among the reasons:
>>> (a) I have at least one device that does not work better with both
>>>   patches [1]
>>> (b) So far, I haven't seen any explanation why commit a0085f2510e8
>>>   should be a dependency for $subject patch; the closest I can imagine
>>>   would be that commit a0085f2510e8 could effectively neutralize
>>>   $subject patch -- if the device is marked as a wakeup source, then
>>>   it will never try to RESET on resume -- but that's still a fuzzy
>>>   signal; just because it's marked a wakeup source sometimes doesn't
>>>   mean it always will be. We can disable it in user space 

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Brian Norris]

Hi Kai,

On Tue, Dec 19, 2017 at 12:28:17PM +0800, Kai Heng Feng wrote:
> > On 19 Dec 2017, at 2:13 AM, Brian Norris  wrote:
> > On Mon, Dec 18, 2017 at 12:43:48PM +0100, Greg Kroah-Hartman wrote:
> >> On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
> >>> We identified the above patch as the culprit, in combination with USB
> >>> autosuspend being enabled for the Bluetooth controller.
> >>> 
> >>> We found that the following (recent) upstream patch fixes the issue on
> >>> most devices (we are still investigating one case on a specific device):
> > 
> > A key word to highlight here is "most". I have at least one device that
> > is consistently having problems even with both patches included; the
> > only way things work reliably so far is to simply revert the $subject
> > patch in 4.4.x.
> 
> The problem we have is that the QCA Rome Bluetooth stops working
> after system S3. The reset resume quirk workaround the issue on both
> mainline and 4.4.x (at least for me).

Understood. But that's not the case for all systems, and on some, you're
regressing functionality.

Many systems keep power enabled for system suspend, and so that "fix" is
not necessary for them. It's also not very precise, because it seems to
act in many cases where it need not -- for one, it takes effect on *all*
resume attempts, even resuming from autosuspend. I really doubt your
system is losing USB power in S0 + USB autosuspend?

So, if you really need this patch for some systems, it seems like it
should be much better targeted. You're currently adding a lot of
unnecessary overhead and fragility.

> > I'll try to investigate further, since this result is a bit confusing
> > still. If there's a real problem with the patch (and I suspect there
> > might be), it probably shouldn't be in mainline either…
> 
> Do you have the same issue on mainline?

Refer to my note [1] :)

But because you asked, I did the work necessary to boot mainline on this
system, and in fact, I see the same problem. I think that's enough
reason to revert your patch in all branches (upstream, and any -stable
branch that already took it).

To be clear, here's the kernel logs on 4.15-rc4+, where the 55-second
mark is around where I try to power on and scan for BT devices (power-on
and scan both fail):

[2.323475] usb 3-1: new full-speed USB device number 2 using ohci-platform
...
[2.731719] usb 3-1: New USB device found, idVendor=0cf3, idProduct=e300
...
[2.867870] usb 3-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
...(udev doesn't run until here)...
[   35.733139] usbcore: registered new interface driver btusb
[   35.740912] Bluetooth: hci0: using rampatch file: 
qca/rampatch_usb_0302.bin
[   35.749216] Bluetooth: hci0: QCA: patch rome 0x302 build 0x3e8, firmware 
rome 0x302 build 0x111
...
[   35.816104] Bluetooth: hci0: using NVM file: qca/nvm_usb_0302.bin
...
[   55.073503] usb 3-1: reset full-speed USB device number 2 using ohci-platform
...
[   57.772513] Bluetooth: hci0: urb ec39086b failed to resubmit (113)
[   57.780217] Bluetooth: hci0: urb 66228fda failed to resubmit (113)

Whereas reverting the $subject patch yields no reset and URB failure;
Bluetooth seems to work fine.

> >>> commit a0085f2510e8976614ad8f766b209448b385492f
> >>> Author: Sukumar Ghorai 
> >>> Date:   Wed Aug 16 14:46:55 2017 -0700
> >>> 
> >>>Bluetooth: btusb: driver to enable the usb-wakeup feature
> > [...]
> >>> stable branches are currently broken for BTUSB_QCA_ROME with USB
> >>> autosuspend enabled, since the above patch is not included (I only
> >>> checked v4.4 and v4.9), so we probably want to integrate it.
> >> 
> >> Now queued up, thanks for letting me know.
> > 
> > I think you have almost as much information as I do at this point, and
> > I'll try to reply back here if I figure out anything more, so I'll leave
> > you to your decisions. But I would personally revert, not backport more
> > patches.
> > 
> > Among the reasons:
> > (a) I have at least one device that does not work better with both
> >patches [1]
> > (b) So far, I haven't seen any explanation why commit a0085f2510e8
> >should be a dependency for $subject patch; the closest I can imagine
> >would be that commit a0085f2510e8 could effectively neutralize
> >$subject patch -- if the device is marked as a wakeup source, then
> >it will never try to RESET on resume -- but that's still a fuzzy
> >signal; just because it's marked a wakeup source sometimes doesn't
> >mean it always will be. We can disable it in user space too.
> 
> Hi Leif,
> 
> Can you try if a0085f2510e8 breaks your $subject commit?
> If it’s a yes, then what Brian suggests is correct.
> 
> Also, can you share the output of "usb-device” for your btusb device?

FWIW, here's mine:

T:  Bus=03 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  2 Spd=12  MxCh= 0
D:  Ver= 2.01 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Brian Norris]

Hi Kai,

On Tue, Dec 19, 2017 at 12:28:17PM +0800, Kai Heng Feng wrote:
> > On 19 Dec 2017, at 2:13 AM, Brian Norris  wrote:
> > On Mon, Dec 18, 2017 at 12:43:48PM +0100, Greg Kroah-Hartman wrote:
> >> On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
> >>> We identified the above patch as the culprit, in combination with USB
> >>> autosuspend being enabled for the Bluetooth controller.
> >>> 
> >>> We found that the following (recent) upstream patch fixes the issue on
> >>> most devices (we are still investigating one case on a specific device):
> > 
> > A key word to highlight here is "most". I have at least one device that
> > is consistently having problems even with both patches included; the
> > only way things work reliably so far is to simply revert the $subject
> > patch in 4.4.x.
> 
> The problem we have is that the QCA Rome Bluetooth stops working
> after system S3. The reset resume quirk workaround the issue on both
> mainline and 4.4.x (at least for me).

Understood. But that's not the case for all systems, and on some, you're
regressing functionality.

Many systems keep power enabled for system suspend, and so that "fix" is
not necessary for them. It's also not very precise, because it seems to
act in many cases where it need not -- for one, it takes effect on *all*
resume attempts, even resuming from autosuspend. I really doubt your
system is losing USB power in S0 + USB autosuspend?

So, if you really need this patch for some systems, it seems like it
should be much better targeted. You're currently adding a lot of
unnecessary overhead and fragility.

> > I'll try to investigate further, since this result is a bit confusing
> > still. If there's a real problem with the patch (and I suspect there
> > might be), it probably shouldn't be in mainline either…
> 
> Do you have the same issue on mainline?

Refer to my note [1] :)

But because you asked, I did the work necessary to boot mainline on this
system, and in fact, I see the same problem. I think that's enough
reason to revert your patch in all branches (upstream, and any -stable
branch that already took it).

To be clear, here's the kernel logs on 4.15-rc4+, where the 55-second
mark is around where I try to power on and scan for BT devices (power-on
and scan both fail):

[2.323475] usb 3-1: new full-speed USB device number 2 using ohci-platform
...
[2.731719] usb 3-1: New USB device found, idVendor=0cf3, idProduct=e300
...
[2.867870] usb 3-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
...(udev doesn't run until here)...
[   35.733139] usbcore: registered new interface driver btusb
[   35.740912] Bluetooth: hci0: using rampatch file: 
qca/rampatch_usb_0302.bin
[   35.749216] Bluetooth: hci0: QCA: patch rome 0x302 build 0x3e8, firmware 
rome 0x302 build 0x111
...
[   35.816104] Bluetooth: hci0: using NVM file: qca/nvm_usb_0302.bin
...
[   55.073503] usb 3-1: reset full-speed USB device number 2 using ohci-platform
...
[   57.772513] Bluetooth: hci0: urb ec39086b failed to resubmit (113)
[   57.780217] Bluetooth: hci0: urb 66228fda failed to resubmit (113)

Whereas reverting the $subject patch yields no reset and URB failure;
Bluetooth seems to work fine.

> >>> commit a0085f2510e8976614ad8f766b209448b385492f
> >>> Author: Sukumar Ghorai 
> >>> Date:   Wed Aug 16 14:46:55 2017 -0700
> >>> 
> >>>Bluetooth: btusb: driver to enable the usb-wakeup feature
> > [...]
> >>> stable branches are currently broken for BTUSB_QCA_ROME with USB
> >>> autosuspend enabled, since the above patch is not included (I only
> >>> checked v4.4 and v4.9), so we probably want to integrate it.
> >> 
> >> Now queued up, thanks for letting me know.
> > 
> > I think you have almost as much information as I do at this point, and
> > I'll try to reply back here if I figure out anything more, so I'll leave
> > you to your decisions. But I would personally revert, not backport more
> > patches.
> > 
> > Among the reasons:
> > (a) I have at least one device that does not work better with both
> >patches [1]
> > (b) So far, I haven't seen any explanation why commit a0085f2510e8
> >should be a dependency for $subject patch; the closest I can imagine
> >would be that commit a0085f2510e8 could effectively neutralize
> >$subject patch -- if the device is marked as a wakeup source, then
> >it will never try to RESET on resume -- but that's still a fuzzy
> >signal; just because it's marked a wakeup source sometimes doesn't
> >mean it always will be. We can disable it in user space too.
> 
> Hi Leif,
> 
> Can you try if a0085f2510e8 breaks your $subject commit?
> If it’s a yes, then what Brian suggests is correct.
> 
> Also, can you share the output of "usb-device” for your btusb device?

FWIW, here's mine:

T:  Bus=03 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  2 Spd=12  MxCh= 0
D:  Ver= 2.01 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=0cf3 ProdID=e300 Rev=00.01
C:  #Ifs= 2 

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Kai Heng Feng]

Hi Brian,

> On 19 Dec 2017, at 2:13 AM, Brian Norris  wrote:
> 
> Hi Greg,
> 
> On Mon, Dec 18, 2017 at 12:43:48PM +0100, Greg Kroah-Hartman wrote:
>> On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
>>> We identified the above patch as the culprit, in combination with USB
>>> autosuspend being enabled for the Bluetooth controller.
>>> 
>>> We found that the following (recent) upstream patch fixes the issue on
>>> most devices (we are still investigating one case on a specific device):
> 
> A key word to highlight here is "most". I have at least one device that
> is consistently having problems even with both patches included; the
> only way things work reliably so far is to simply revert the $subject
> patch in 4.4.x.

The problem we have is that the QCA Rome Bluetooth stops working
after system S3. The reset resume quirk workaround the issue on both
mainline and 4.4.x (at least for me).

> 
> I'll try to investigate further, since this result is a bit confusing
> still. If there's a real problem with the patch (and I suspect there
> might be), it probably shouldn't be in mainline either…

Do you have the same issue on mainline?

> 
>>> commit a0085f2510e8976614ad8f766b209448b385492f
>>> Author: Sukumar Ghorai 
>>> Date:   Wed Aug 16 14:46:55 2017 -0700
>>> 
>>>Bluetooth: btusb: driver to enable the usb-wakeup feature
> [...]
>>> stable branches are currently broken for BTUSB_QCA_ROME with USB
>>> autosuspend enabled, since the above patch is not included (I only
>>> checked v4.4 and v4.9), so we probably want to integrate it.
>> 
>> Now queued up, thanks for letting me know.
> 
> I think you have almost as much information as I do at this point, and
> I'll try to reply back here if I figure out anything more, so I'll leave
> you to your decisions. But I would personally revert, not backport more
> patches.
> 
> Among the reasons:
> (a) I have at least one device that does not work better with both
>patches [1]
> (b) So far, I haven't seen any explanation why commit a0085f2510e8
>should be a dependency for $subject patch; the closest I can imagine
>would be that commit a0085f2510e8 could effectively neutralize
>$subject patch -- if the device is marked as a wakeup source, then
>it will never try to RESET on resume -- but that's still a fuzzy
>signal; just because it's marked a wakeup source sometimes doesn't
>mean it always will be. We can disable it in user space too.

Hi Leif,

Can you try if a0085f2510e8 breaks your $subject commit?
If it’s a yes, then what Brian suggests is correct.

Also, can you share the output of "usb-device” for your btusb device?


> (c) Isn't it safer to revert than to backport more? You're delving into
>feature-land, not bugfix-land…

Sounds reasonable.

We’ll need more information from Leif if we need to do ID-specific quirks.

Kai-Heng

> Brian
> 
> [1] Before you ask: this particular device is not quite fully supported
> on upstream yet, though its sibling devices are. With a bit of effort, I
> might be able to test a clean upstream. At the moment, I'm using our
> Chrom{e,ium}OS 4.4 kernel, where we regularly merge in -stable updates.

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Kai Heng Feng]

Hi Brian,

> On 19 Dec 2017, at 2:13 AM, Brian Norris  wrote:
> 
> Hi Greg,
> 
> On Mon, Dec 18, 2017 at 12:43:48PM +0100, Greg Kroah-Hartman wrote:
>> On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
>>> We identified the above patch as the culprit, in combination with USB
>>> autosuspend being enabled for the Bluetooth controller.
>>> 
>>> We found that the following (recent) upstream patch fixes the issue on
>>> most devices (we are still investigating one case on a specific device):
> 
> A key word to highlight here is "most". I have at least one device that
> is consistently having problems even with both patches included; the
> only way things work reliably so far is to simply revert the $subject
> patch in 4.4.x.

The problem we have is that the QCA Rome Bluetooth stops working
after system S3. The reset resume quirk workaround the issue on both
mainline and 4.4.x (at least for me).

> 
> I'll try to investigate further, since this result is a bit confusing
> still. If there's a real problem with the patch (and I suspect there
> might be), it probably shouldn't be in mainline either…

Do you have the same issue on mainline?

> 
>>> commit a0085f2510e8976614ad8f766b209448b385492f
>>> Author: Sukumar Ghorai 
>>> Date:   Wed Aug 16 14:46:55 2017 -0700
>>> 
>>>Bluetooth: btusb: driver to enable the usb-wakeup feature
> [...]
>>> stable branches are currently broken for BTUSB_QCA_ROME with USB
>>> autosuspend enabled, since the above patch is not included (I only
>>> checked v4.4 and v4.9), so we probably want to integrate it.
>> 
>> Now queued up, thanks for letting me know.
> 
> I think you have almost as much information as I do at this point, and
> I'll try to reply back here if I figure out anything more, so I'll leave
> you to your decisions. But I would personally revert, not backport more
> patches.
> 
> Among the reasons:
> (a) I have at least one device that does not work better with both
>patches [1]
> (b) So far, I haven't seen any explanation why commit a0085f2510e8
>should be a dependency for $subject patch; the closest I can imagine
>would be that commit a0085f2510e8 could effectively neutralize
>$subject patch -- if the device is marked as a wakeup source, then
>it will never try to RESET on resume -- but that's still a fuzzy
>signal; just because it's marked a wakeup source sometimes doesn't
>mean it always will be. We can disable it in user space too.

Hi Leif,

Can you try if a0085f2510e8 breaks your $subject commit?
If it’s a yes, then what Brian suggests is correct.

Also, can you share the output of "usb-device” for your btusb device?


> (c) Isn't it safer to revert than to backport more? You're delving into
>feature-land, not bugfix-land…

Sounds reasonable.

We’ll need more information from Leif if we need to do ID-specific quirks.

Kai-Heng

> Brian
> 
> [1] Before you ask: this particular device is not quite fully supported
> on upstream yet, though its sibling devices are. With a bit of effort, I
> might be able to test a clean upstream. At the moment, I'm using our
> Chrom{e,ium}OS 4.4 kernel, where we regularly merge in -stable updates.

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Brian Norris]

Hi Greg,

On Mon, Dec 18, 2017 at 12:43:48PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
> > We identified the above patch as the culprit, in combination with USB
> > autosuspend being enabled for the Bluetooth controller.
> > 
> > We found that the following (recent) upstream patch fixes the issue on
> > most devices (we are still investigating one case on a specific device):

A key word to highlight here is "most". I have at least one device that
is consistently having problems even with both patches included; the
only way things work reliably so far is to simply revert the $subject
patch in 4.4.x.

I'll try to investigate further, since this result is a bit confusing
still. If there's a real problem with the patch (and I suspect there
might be), it probably shouldn't be in mainline either...

> > commit a0085f2510e8976614ad8f766b209448b385492f
> > Author: Sukumar Ghorai 
> > Date:   Wed Aug 16 14:46:55 2017 -0700
> > 
> > Bluetooth: btusb: driver to enable the usb-wakeup feature
[...]
> > stable branches are currently broken for BTUSB_QCA_ROME with USB
> > autosuspend enabled, since the above patch is not included (I only
> > checked v4.4 and v4.9), so we probably want to integrate it.
> 
> Now queued up, thanks for letting me know.

I think you have almost as much information as I do at this point, and
I'll try to reply back here if I figure out anything more, so I'll leave
you to your decisions. But I would personally revert, not backport more
patches.

Among the reasons:
(a) I have at least one device that does not work better with both
patches [1]
(b) So far, I haven't seen any explanation why commit a0085f2510e8
should be a dependency for $subject patch; the closest I can imagine
would be that commit a0085f2510e8 could effectively neutralize
$subject patch -- if the device is marked as a wakeup source, then
it will never try to RESET on resume -- but that's still a fuzzy
signal; just because it's marked a wakeup source sometimes doesn't
mean it always will be. We can disable it in user space too.
(c) Isn't it safer to revert than to backport more? You're delving into
feature-land, not bugfix-land...

Brian

[1] Before you ask: this particular device is not quite fully supported
on upstream yet, though its sibling devices are. With a bit of effort, I
might be able to test a clean upstream. At the moment, I'm using our
Chrom{e,ium}OS 4.4 kernel, where we regularly merge in -stable updates.

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Brian Norris]

Hi Greg,

On Mon, Dec 18, 2017 at 12:43:48PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
> > We identified the above patch as the culprit, in combination with USB
> > autosuspend being enabled for the Bluetooth controller.
> > 
> > We found that the following (recent) upstream patch fixes the issue on
> > most devices (we are still investigating one case on a specific device):

A key word to highlight here is "most". I have at least one device that
is consistently having problems even with both patches included; the
only way things work reliably so far is to simply revert the $subject
patch in 4.4.x.

I'll try to investigate further, since this result is a bit confusing
still. If there's a real problem with the patch (and I suspect there
might be), it probably shouldn't be in mainline either...

> > commit a0085f2510e8976614ad8f766b209448b385492f
> > Author: Sukumar Ghorai 
> > Date:   Wed Aug 16 14:46:55 2017 -0700
> > 
> > Bluetooth: btusb: driver to enable the usb-wakeup feature
[...]
> > stable branches are currently broken for BTUSB_QCA_ROME with USB
> > autosuspend enabled, since the above patch is not included (I only
> > checked v4.4 and v4.9), so we probably want to integrate it.
> 
> Now queued up, thanks for letting me know.

I think you have almost as much information as I do at this point, and
I'll try to reply back here if I figure out anything more, so I'll leave
you to your decisions. But I would personally revert, not backport more
patches.

Among the reasons:
(a) I have at least one device that does not work better with both
patches [1]
(b) So far, I haven't seen any explanation why commit a0085f2510e8
should be a dependency for $subject patch; the closest I can imagine
would be that commit a0085f2510e8 could effectively neutralize
$subject patch -- if the device is marked as a wakeup source, then
it will never try to RESET on resume -- but that's still a fuzzy
signal; just because it's marked a wakeup source sometimes doesn't
mean it always will be. We can disable it in user space too.
(c) Isn't it safer to revert than to backport more? You're delving into
feature-land, not bugfix-land...

Brian

[1] Before you ask: this particular device is not quite fully supported
on upstream yet, though its sibling devices are. With a bit of effort, I
might be able to test a clean upstream. At the moment, I'm using our
Chrom{e,ium}OS 4.4 kernel, where we regularly merge in -stable updates.

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Greg Kroah-Hartman]

On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
> Hi,
> 
> El Sun, Nov 19, 2017 at 03:43:50PM +0100 Greg Kroah-Hartman ha dit:
> 
> > 4.13-stable review patch.  If anyone has any objections, please let me know.
> > 
> > --
> > 
> > From: Leif Liddy 
> > 
> > commit fd865802c66bc451dc515ed89360f84376ce1a56 upstream.
> > 
> > There's been numerous reported instances where BTUSB_QCA_ROME
> > bluetooth controllers stop functioning upon resume from suspend. These
> > devices seem to be losing power during suspend. Patch will detect a status
> > change on resume and perform a reset.
> > 
> > Signed-off-by: Leif Liddy 
> > Signed-off-by: Marcel Holtmann 
> > Cc: Kai Heng Feng 
> > Signed-off-by: Greg Kroah-Hartman 
> > 
> > ---
> >  drivers/bluetooth/btusb.c |6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -3068,6 +3068,12 @@ static int btusb_probe(struct usb_interf
> > 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.
> > +*/
> > +   set_bit(BTUSB_RESET_RESUME, >flags);
> > }
> >  
> >  #ifdef CONFIG_BT_HCIBTUSB_RTL
> > 
> > 
> 
> On Chrome OS QCA Bluetooth stopped working after a merge from v4.4-stable:
> 
> [  124.789298] usb 2-1.1: reset full-speed USB device number 3 using 
> ehci-platform
> [  126.624274] Bluetooth: hci0 command 0x2005 tx timeout
> [  128.672262] Bluetooth: hci0 command 0x200b tx timeout
> [  130.720264] Bluetooth: hci0 command 0x200c tx timeout
> 
> We identified the above patch as the culprit, in combination with USB
> autosuspend being enabled for the Bluetooth controller.
> 
> We found that the following (recent) upstream patch fixes the issue on
> most devices (we are still investigating one case on a specific device):
> 
> commit a0085f2510e8976614ad8f766b209448b385492f
> Author: Sukumar Ghorai 
> Date:   Wed Aug 16 14:46:55 2017 -0700
> 
> Bluetooth: btusb: driver to enable the usb-wakeup feature
> 
> BT-Controller connected as platform non-root-hub device and
> usb-driver initialize such device with wakeup disabled,
> Ref. usb_new_device().
> 
> At present wakeup-capability get enabled by hid-input device from usb
> function driver(e.g. BT HID device) at runtime. Again some functional
> driver does not set usb-wakeup capability(e.g LE HID device implement
> as HID-over-GATT), and can't wakeup the host on USB.
> 
> Most of the device operation (such as mass storage) initiated from host
> (except HID) and USB wakeup aligned with host resume procedure. For BT
> device, usb-wakeup capability need to enable form btusc driver as a
> generic solution for multiple profile use case and required for USB remote
> wakeup (in-bus wakeup) while host is suspended. Also usb-wakeup feature
> need to enable/disable with HCI interface up and down.
> 
> 
> stable branches are currently broken for BTUSB_QCA_ROME with USB
> autosuspend enabled, since the above patch is not included (I only
> checked v4.4 and v4.9), so we probably want to integrate it.

Now queued up, thanks for letting me know.

greg k-h

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Greg Kroah-Hartman]

On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
> Hi,
> 
> El Sun, Nov 19, 2017 at 03:43:50PM +0100 Greg Kroah-Hartman ha dit:
> 
> > 4.13-stable review patch.  If anyone has any objections, please let me know.
> > 
> > --
> > 
> > From: Leif Liddy 
> > 
> > commit fd865802c66bc451dc515ed89360f84376ce1a56 upstream.
> > 
> > There's been numerous reported instances where BTUSB_QCA_ROME
> > bluetooth controllers stop functioning upon resume from suspend. These
> > devices seem to be losing power during suspend. Patch will detect a status
> > change on resume and perform a reset.
> > 
> > Signed-off-by: Leif Liddy 
> > Signed-off-by: Marcel Holtmann 
> > Cc: Kai Heng Feng 
> > Signed-off-by: Greg Kroah-Hartman 
> > 
> > ---
> >  drivers/bluetooth/btusb.c |6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -3068,6 +3068,12 @@ static int btusb_probe(struct usb_interf
> > 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.
> > +*/
> > +   set_bit(BTUSB_RESET_RESUME, >flags);
> > }
> >  
> >  #ifdef CONFIG_BT_HCIBTUSB_RTL
> > 
> > 
> 
> On Chrome OS QCA Bluetooth stopped working after a merge from v4.4-stable:
> 
> [  124.789298] usb 2-1.1: reset full-speed USB device number 3 using 
> ehci-platform
> [  126.624274] Bluetooth: hci0 command 0x2005 tx timeout
> [  128.672262] Bluetooth: hci0 command 0x200b tx timeout
> [  130.720264] Bluetooth: hci0 command 0x200c tx timeout
> 
> We identified the above patch as the culprit, in combination with USB
> autosuspend being enabled for the Bluetooth controller.
> 
> We found that the following (recent) upstream patch fixes the issue on
> most devices (we are still investigating one case on a specific device):
> 
> commit a0085f2510e8976614ad8f766b209448b385492f
> Author: Sukumar Ghorai 
> Date:   Wed Aug 16 14:46:55 2017 -0700
> 
> Bluetooth: btusb: driver to enable the usb-wakeup feature
> 
> BT-Controller connected as platform non-root-hub device and
> usb-driver initialize such device with wakeup disabled,
> Ref. usb_new_device().
> 
> At present wakeup-capability get enabled by hid-input device from usb
> function driver(e.g. BT HID device) at runtime. Again some functional
> driver does not set usb-wakeup capability(e.g LE HID device implement
> as HID-over-GATT), and can't wakeup the host on USB.
> 
> Most of the device operation (such as mass storage) initiated from host
> (except HID) and USB wakeup aligned with host resume procedure. For BT
> device, usb-wakeup capability need to enable form btusc driver as a
> generic solution for multiple profile use case and required for USB remote
> wakeup (in-bus wakeup) while host is suspended. Also usb-wakeup feature
> need to enable/disable with HCI interface up and down.
> 
> 
> stable branches are currently broken for BTUSB_QCA_ROME with USB
> autosuspend enabled, since the above patch is not included (I only
> checked v4.4 and v4.9), so we probably want to integrate it.

Now queued up, thanks for letting me know.

greg k-h

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Kai Heng Feng]

Hi,

> On 16 Dec 2017, at 11:05 AM, Matthias Kaehlcke  wrote:
> stable branches are currently broken for BTUSB_QCA_ROME with USB
> autosuspend enabled, since the above patch is not included (I only
> checked v4.4 and v4.9), so we probably want to integrate it.

Thanks for this information.

I just found the same issue and I can confirm your finding.

I was testing with runtime suspend disabled.
Once the runtime suspend gets enabled, the additional patch is needed.

I’ll remember to enable runtime pm next time.

Thanks again.

Kai-Heng

> 
> Thanks
> 
> Matthias

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Kai Heng Feng]

Hi,

> On 16 Dec 2017, at 11:05 AM, Matthias Kaehlcke  wrote:
> stable branches are currently broken for BTUSB_QCA_ROME with USB
> autosuspend enabled, since the above patch is not included (I only
> checked v4.4 and v4.9), so we probably want to integrate it.

Thanks for this information.

I just found the same issue and I can confirm your finding.

I was testing with runtime suspend disabled.
Once the runtime suspend gets enabled, the additional patch is needed.

I’ll remember to enable runtime pm next time.

Thanks again.

Kai-Heng

> 
> Thanks
> 
> Matthias

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Matthias Kaehlcke]

Hi,

El Sun, Nov 19, 2017 at 03:43:50PM +0100 Greg Kroah-Hartman ha dit:

> 4.13-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Leif Liddy 
> 
> commit fd865802c66bc451dc515ed89360f84376ce1a56 upstream.
> 
> There's been numerous reported instances where BTUSB_QCA_ROME
> bluetooth controllers stop functioning upon resume from suspend. These
> devices seem to be losing power during suspend. Patch will detect a status
> change on resume and perform a reset.
> 
> Signed-off-by: Leif Liddy 
> Signed-off-by: Marcel Holtmann 
> Cc: Kai Heng Feng 
> Signed-off-by: Greg Kroah-Hartman 
> 
> ---
>  drivers/bluetooth/btusb.c |6 ++
>  1 file changed, 6 insertions(+)
> 
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3068,6 +3068,12 @@ static int btusb_probe(struct usb_interf
>   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.
> +  */
> + set_bit(BTUSB_RESET_RESUME, >flags);
>   }
>  
>  #ifdef CONFIG_BT_HCIBTUSB_RTL
> 
> 

On Chrome OS QCA Bluetooth stopped working after a merge from v4.4-stable:

[  124.789298] usb 2-1.1: reset full-speed USB device number 3 using 
ehci-platform
[  126.624274] Bluetooth: hci0 command 0x2005 tx timeout
[  128.672262] Bluetooth: hci0 command 0x200b tx timeout
[  130.720264] Bluetooth: hci0 command 0x200c tx timeout

We identified the above patch as the culprit, in combination with USB
autosuspend being enabled for the Bluetooth controller.

We found that the following (recent) upstream patch fixes the issue on
most devices (we are still investigating one case on a specific device):

commit a0085f2510e8976614ad8f766b209448b385492f
Author: Sukumar Ghorai 
Date:   Wed Aug 16 14:46:55 2017 -0700

Bluetooth: btusb: driver to enable the usb-wakeup feature

BT-Controller connected as platform non-root-hub device and
usb-driver initialize such device with wakeup disabled,
Ref. usb_new_device().

At present wakeup-capability get enabled by hid-input device from usb
function driver(e.g. BT HID device) at runtime. Again some functional
driver does not set usb-wakeup capability(e.g LE HID device implement
as HID-over-GATT), and can't wakeup the host on USB.

Most of the device operation (such as mass storage) initiated from host
(except HID) and USB wakeup aligned with host resume procedure. For BT
device, usb-wakeup capability need to enable form btusc driver as a
generic solution for multiple profile use case and required for USB remote
wakeup (in-bus wakeup) while host is suspended. Also usb-wakeup feature
need to enable/disable with HCI interface up and down.


stable branches are currently broken for BTUSB_QCA_ROME with USB
autosuspend enabled, since the above patch is not included (I only
checked v4.4 and v4.9), so we probably want to integrate it.

Thanks

Matthias

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Matthias Kaehlcke]

Hi,

El Sun, Nov 19, 2017 at 03:43:50PM +0100 Greg Kroah-Hartman ha dit:

> 4.13-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Leif Liddy 
> 
> commit fd865802c66bc451dc515ed89360f84376ce1a56 upstream.
> 
> There's been numerous reported instances where BTUSB_QCA_ROME
> bluetooth controllers stop functioning upon resume from suspend. These
> devices seem to be losing power during suspend. Patch will detect a status
> change on resume and perform a reset.
> 
> Signed-off-by: Leif Liddy 
> Signed-off-by: Marcel Holtmann 
> Cc: Kai Heng Feng 
> Signed-off-by: Greg Kroah-Hartman 
> 
> ---
>  drivers/bluetooth/btusb.c |6 ++
>  1 file changed, 6 insertions(+)
> 
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3068,6 +3068,12 @@ static int btusb_probe(struct usb_interf
>   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.
> +  */
> + set_bit(BTUSB_RESET_RESUME, >flags);
>   }
>  
>  #ifdef CONFIG_BT_HCIBTUSB_RTL
> 
> 

On Chrome OS QCA Bluetooth stopped working after a merge from v4.4-stable:

[  124.789298] usb 2-1.1: reset full-speed USB device number 3 using 
ehci-platform
[  126.624274] Bluetooth: hci0 command 0x2005 tx timeout
[  128.672262] Bluetooth: hci0 command 0x200b tx timeout
[  130.720264] Bluetooth: hci0 command 0x200c tx timeout

We identified the above patch as the culprit, in combination with USB
autosuspend being enabled for the Bluetooth controller.

We found that the following (recent) upstream patch fixes the issue on
most devices (we are still investigating one case on a specific device):

commit a0085f2510e8976614ad8f766b209448b385492f
Author: Sukumar Ghorai 
Date:   Wed Aug 16 14:46:55 2017 -0700

Bluetooth: btusb: driver to enable the usb-wakeup feature

BT-Controller connected as platform non-root-hub device and
usb-driver initialize such device with wakeup disabled,
Ref. usb_new_device().

At present wakeup-capability get enabled by hid-input device from usb
function driver(e.g. BT HID device) at runtime. Again some functional
driver does not set usb-wakeup capability(e.g LE HID device implement
as HID-over-GATT), and can't wakeup the host on USB.

Most of the device operation (such as mass storage) initiated from host
(except HID) and USB wakeup aligned with host resume procedure. For BT
device, usb-wakeup capability need to enable form btusc driver as a
generic solution for multiple profile use case and required for USB remote
wakeup (in-bus wakeup) while host is suspended. Also usb-wakeup feature
need to enable/disable with HCI interface up and down.


stable branches are currently broken for BTUSB_QCA_ROME with USB
autosuspend enabled, since the above patch is not included (I only
checked v4.4 and v4.9), so we probably want to integrate it.

Thanks

Matthias