Re: [PATCH 4.4 009/115] Bluetooth: btusb: driver to enable the usb-wakeup feature

[Brian Norris]

Hi Greg,

On Mon, Dec 18, 2017 at 04:47:58PM +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.

I'm sorry, but I already objected to this one during the discussion
here:

https://patchwork.kernel.org/patch/10065483/
[PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

in which we pointed out a regression. The $subject patch does NOT
actually resolve the previous regression, though it might help to mask
it. The proper approach to resolve the above regression was to revert
the patch, not to backport the $subject patch.

Regarding this patch, IIUC this is not a bugfix -- it's a feature
addition (e.g., for helping with BLE mouse wakeup), and it has already
been proven to break some user space (we have an internal bug tracking
this, but suffice it to say that we've already tried and reverted this
patch [1]). This patch massively increases the surface in which spurious
bluetooth activity can wake the system, and in some cases we never can
suspend the system at all.

Unfortunately, Matthias was on vacation when you sent the review
request, so our team wasn't alerted properly. Can you please back this
out of all -stable branches?

Or alternatively, if those I've added on CC disagree and are happy to
deal with the fallout of this patch...well, then that's fine. We can
revert this patch in our downstream kernels and reapply if/when we can
account for it properly :)

Thanks,
Brian

[1]
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/750073

> ------------------
> 
> From: Sukumar Ghorai <sukumar.ghorai@xxxxxxxxx>
> 
> commit a0085f2510e8976614ad8f766b209448b385492f upstream.
> 
> 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.
> 
> Signed-off-by: Sukumar Ghorai <sukumar.ghorai@xxxxxxxxx>
> Signed-off-by: Amit K Bag <amit.k.bag@xxxxxxxxx>
> Acked-by: Oliver Neukum <oneukum@xxxxxxxx>
> Signed-off-by: Marcel Holtmann <marcel@xxxxxxxxxxxx>
> Cc: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/bluetooth/btusb.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1050,6 +1050,10 @@ static int btusb_open(struct hci_dev *hd
>  		return err;
>  
>  	data->intf->needs_remote_wakeup = 1;
> +	/* device specific wakeup source enabled and required for USB
> +	 * remote wakeup while host is suspended
> +	 */
> +	device_wakeup_enable(&data->udev->dev);
>  
>  	if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags))
>  		goto done;
> @@ -1113,6 +1117,7 @@ static int btusb_close(struct hci_dev *h
>  		goto failed;
>  
>  	data->intf->needs_remote_wakeup = 0;
> +	device_wakeup_disable(&data->udev->dev);
>  	usb_autopm_put_interface(data->intf);
>  
>  failed: