[PATCH] Bluetooth: btusb: Disable autosuspend on QCA Rome devices

[PATCH] Bluetooth: btusb: Disable autosuspend on QCA Rome devices

[Hans de Goede]

Commit fd865802c66b ("Bluetooth: btusb: fix QCA Rome suspend/resume") fixes
a suspend/resume problem on QCA devices by doing a full reset on resume,
reloading the firmware.

A similar problem happens when using runtime-pm / autosuspend, when this is
enabled by the user the QCA Rome device stops working. Reloading the
firmware after a runtime suspend is not really an option since the latency
caused by this is unacceptable.

To fix the runtime-pm issues, this commit disables runtime-pm on QCA Rome
HCIs, by getting (and not releasing) an usb autopm reference on the btusb
interface.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/btusb.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 808c249845db..6ed3a0e5b8f6 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3122,8 +3122,15 @@ static int btusb_probe(struct usb_interface *intf,
 		/* 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.
+		 * And disable runtime pm by getting a pm reference, the USB
+		 * core will drop our reference on disconnect.
 		 */
 		set_bit(BTUSB_RESET_RESUME, &data->flags);
+		err = usb_autopm_get_interface(data->intf);
+		if (err < 0) {
+			BT_ERR("failed to get pm reference %d", err);
+			goto out_free_dev;
+		}
 	}
 
 #ifdef CONFIG_BT_HCIBTUSB_RTL
-- 
2.14.3

Re: [PATCH] Bluetooth: btusb: Disable autosuspend on QCA Rome devices

[Marcel Holtmann]

Hi Hans,

> Commit fd865802c66b ("Bluetooth: btusb: fix QCA Rome suspend/resume") fixes
> a suspend/resume problem on QCA devices by doing a full reset on resume,
> reloading the firmware.
> 
> A similar problem happens when using runtime-pm / autosuspend, when this is
> enabled by the user the QCA Rome device stops working. Reloading the
> firmware after a runtime suspend is not really an option since the latency
> caused by this is unacceptable.
> 
> To fix the runtime-pm issues, this commit disables runtime-pm on QCA Rome
> HCIs, by getting (and not releasing) an usb autopm reference on the btusb
> interface.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/bluetooth/btusb.c | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 808c249845db..6ed3a0e5b8f6 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3122,8 +3122,15 @@ static int btusb_probe(struct usb_interface *intf,
> 		/* 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.
> +		 * And disable runtime pm by getting a pm reference, the USB
> +		 * core will drop our reference on disconnect.
> 		 */
> 		set_bit(BTUSB_RESET_RESUME, &data->flags);
> +		err = usb_autopm_get_interface(data->intf);
> +		if (err < 0) {
> +			BT_ERR("failed to get pm reference %d", err);
> +			goto out_free_dev;
> +		}
> 	}

is this still needed after commit 7d06d5895c159f6 that reverts this entry and adds a core USB quirk? See bluetooth-next tree for reference.

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: Disable autosuspend on QCA Rome devices

[Hans de Goede]

Hi,

On 05-01-18 15:31, Marcel Holtmann wrote:
> Hi Hans,
> 
>> Commit fd865802c66b ("Bluetooth: btusb: fix QCA Rome suspend/resume") fixes
>> a suspend/resume problem on QCA devices by doing a full reset on resume,
>> reloading the firmware.
>>
>> A similar problem happens when using runtime-pm / autosuspend, when this is
>> enabled by the user the QCA Rome device stops working. Reloading the
>> firmware after a runtime suspend is not really an option since the latency
>> caused by this is unacceptable.
>>
>> To fix the runtime-pm issues, this commit disables runtime-pm on QCA Rome
>> HCIs, by getting (and not releasing) an usb autopm reference on the btusb
>> interface.
>>
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/bluetooth/btusb.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 808c249845db..6ed3a0e5b8f6 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -3122,8 +3122,15 @@ static int btusb_probe(struct usb_interface *intf,
>> 		/* 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.
>> +		 * And disable runtime pm by getting a pm reference, the USB
>> +		 * core will drop our reference on disconnect.
>> 		 */
>> 		set_bit(BTUSB_RESET_RESUME, &data->flags);
>> +		err = usb_autopm_get_interface(data->intf);
>> +		if (err < 0) {
>> +			BT_ERR("failed to get pm reference %d", err);
>> +			goto out_free_dev;
>> +		}
>> 	}
> 
> is this still needed after commit 7d06d5895c159f6 that reverts this entry and adds a core USB quirk? See bluetooth-next tree for reference.

Yes something like this is still needed, the reporter of the bug looses
bluetooth coonectivity due to runtime-pm / usb-autosuspend which is not
influenced by the RESET_RESUME quirk either way.

I assume the original commit was reverted and replaced with a usb-core
quirk because the original commit was applying the RESET_RESUME behavior
to all qca models, while only some are affected ?

I will ask the reporter for the usb-id of the QCA Rome on his laptop and
see if this is the same one as which has been added to
drivers/usb/core/quirks.c now.

Regards,

Hans

Re: [PATCH] Bluetooth: btusb: Disable autosuspend on QCA Rome devices

[Hans de Goede]

Hi,

On 06-01-18 23:23, Hans de Goede wrote:
> Hi,
> 
> On 05-01-18 15:31, Marcel Holtmann wrote:
>> Hi Hans,
>>
>>> Commit fd865802c66b ("Bluetooth: btusb: fix QCA Rome suspend/resume") fixes
>>> a suspend/resume problem on QCA devices by doing a full reset on resume,
>>> reloading the firmware.
>>>
>>> A similar problem happens when using runtime-pm / autosuspend, when this is
>>> enabled by the user the QCA Rome device stops working. Reloading the
>>> firmware after a runtime suspend is not really an option since the latency
>>> caused by this is unacceptable.
>>>
>>> To fix the runtime-pm issues, this commit disables runtime-pm on QCA Rome
>>> HCIs, by getting (and not releasing) an usb autopm reference on the btusb
>>> interface.
>>>
>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> drivers/bluetooth/btusb.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 808c249845db..6ed3a0e5b8f6 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -3122,8 +3122,15 @@ static int btusb_probe(struct usb_interface *intf,
>>>         /* 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.
>>> +         * And disable runtime pm by getting a pm reference, the USB
>>> +         * core will drop our reference on disconnect.
>>>          */
>>>         set_bit(BTUSB_RESET_RESUME, &data->flags);
>>> +        err = usb_autopm_get_interface(data->intf);
>>> +        if (err < 0) {
>>> +            BT_ERR("failed to get pm reference %d", err);
>>> +            goto out_free_dev;
>>> +        }
>>>     }
>>
>> is this still needed after commit 7d06d5895c159f6 that reverts this entry and adds a core USB quirk? See bluetooth-next tree for reference.
> 
> Yes something like this is still needed, the reporter of the bug looses
> bluetooth coonectivity due to runtime-pm / usb-autosuspend which is not
> influenced by the RESET_RESUME quirk either way.
> 
> I assume the original commit was reverted and replaced with a usb-core
> quirk because the original commit was applying the RESET_RESUME behavior
> to all qca models, while only some are affected ?
> 
> I will ask the reporter for the usb-id of the QCA Rome on his laptop and
> see if this is the same one as which has been added to
> drivers/usb/core/quirks.c now.

Ok, so looking a bit further into this, the usb-core has this:

static int autosuspend_check(struct usb_device *udev)
{
	...
                         /* Don't allow autosuspend if the device will need
                          * a reset-resume and any of its interface drivers
                          * doesn't include support or needs remote wakeup.
                          */
                         if (udev->quirks & USB_QUIRK_RESET_RESUME) {
                                 struct usb_driver *driver;

                                 driver = to_usb_driver(intf->dev.driver);
                                 if (!driver->reset_resume ||
                                                 intf->needs_remote_wakeup)
                                         return -EOPNOTSUPP;
                         }
	...

So setting the USB_QUIRK_RESET_RESUME instead of using the DIY code btusb
used to have will also disable USB autosuspend since the btusb driver
does not have a reset_resume callback.  So it may be necessary to add
more qca USB ids to drivers/usb/core/quirks.c with an USB_QUIRK_RESET_RESUME
entry, but otherwise this patch should not be necessary.

2 Questions:

1) Why was only the one USB id added to drivers/usb/core/quirks.c
where as before we were doing this for all known QCA HCIs ?

2) Since btusb.c's own RESET_RESUME handling is broken, shouldn't we
remove it completely (so also for realtek devices) and instead add all
blacklist entries which set .driver_info = BTUSB_REALTEK to
drivers/usb/core/quirks.c with an USB_QUIRK_RESET_RESUME
entry?


Hmm, this will likely boil down to adding lots of semi-duplicate entries
to drivers/usb/core/quirks.c I think that it would be better to do
the following in btusb.c:

         if (id->driver_info & BTUSB_QCA_ROME) {
		...
	         /* 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;
	}

And the same for driver_info & BTUSB_REALTEK ?

And then remove the DIY reset_resume handling completely. If you agree that
that is the best way forward, then I can submit a patch doing that.

Regards.

Hans

Re: [PATCH] Bluetooth: btusb: Disable autosuspend on QCA Rome devices

[Marcel Holtmann]

Hi Hans,

>>>> Commit fd865802c66b ("Bluetooth: btusb: fix QCA Rome suspend/resume") fixes
>>>> a suspend/resume problem on QCA devices by doing a full reset on resume,
>>>> reloading the firmware.
>>>> 
>>>> A similar problem happens when using runtime-pm / autosuspend, when this is
>>>> enabled by the user the QCA Rome device stops working. Reloading the
>>>> firmware after a runtime suspend is not really an option since the latency
>>>> caused by this is unacceptable.
>>>> 
>>>> To fix the runtime-pm issues, this commit disables runtime-pm on QCA Rome
>>>> HCIs, by getting (and not releasing) an usb autopm reference on the btusb
>>>> interface.
>>>> 
>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> drivers/bluetooth/btusb.c | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>> 
>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>> index 808c249845db..6ed3a0e5b8f6 100644
>>>> --- a/drivers/bluetooth/btusb.c
>>>> +++ b/drivers/bluetooth/btusb.c
>>>> @@ -3122,8 +3122,15 @@ static int btusb_probe(struct usb_interface *intf,
>>>>         /* 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.
>>>> +         * And disable runtime pm by getting a pm reference, the USB
>>>> +         * core will drop our reference on disconnect.
>>>>          */
>>>>         set_bit(BTUSB_RESET_RESUME, &data->flags);
>>>> +        err = usb_autopm_get_interface(data->intf);
>>>> +        if (err < 0) {
>>>> +            BT_ERR("failed to get pm reference %d", err);
>>>> +            goto out_free_dev;
>>>> +        }
>>>>     }
>>> 
>>> is this still needed after commit 7d06d5895c159f6 that reverts this entry and adds a core USB quirk? See bluetooth-next tree for reference.
>> Yes something like this is still needed, the reporter of the bug looses
>> bluetooth coonectivity due to runtime-pm / usb-autosuspend which is not
>> influenced by the RESET_RESUME quirk either way.
>> I assume the original commit was reverted and replaced with a usb-core
>> quirk because the original commit was applying the RESET_RESUME behavior
>> to all qca models, while only some are affected ?
>> I will ask the reporter for the usb-id of the QCA Rome on his laptop and
>> see if this is the same one as which has been added to
>> drivers/usb/core/quirks.c now.
> 
> Ok, so looking a bit further into this, the usb-core has this:
> 
> static int autosuspend_check(struct usb_device *udev)
> {
> 	...
>                        /* Don't allow autosuspend if the device will need
>                         * a reset-resume and any of its interface drivers
>                         * doesn't include support or needs remote wakeup.
>                         */
>                        if (udev->quirks & USB_QUIRK_RESET_RESUME) {
>                                struct usb_driver *driver;
> 
>                                driver = to_usb_driver(intf->dev.driver);
>                                if (!driver->reset_resume ||
>                                                intf->needs_remote_wakeup)
>                                        return -EOPNOTSUPP;
>                        }
> 	...
> 
> So setting the USB_QUIRK_RESET_RESUME instead of using the DIY code btusb
> used to have will also disable USB autosuspend since the btusb driver
> does not have a reset_resume callback.  So it may be necessary to add
> more qca USB ids to drivers/usb/core/quirks.c with an USB_QUIRK_RESET_RESUME
> entry, but otherwise this patch should not be necessary.
> 
> 2 Questions:
> 
> 1) Why was only the one USB id added to drivers/usb/core/quirks.c
> where as before we were doing this for all known QCA HCIs ?
> 
> 2) Since btusb.c's own RESET_RESUME handling is broken, shouldn't we
> remove it completely (so also for realtek devices) and instead add all
> blacklist entries which set .driver_info = BTUSB_REALTEK to
> drivers/usb/core/quirks.c with an USB_QUIRK_RESET_RESUME
> entry?
> 
> 
> Hmm, this will likely boil down to adding lots of semi-duplicate entries
> to drivers/usb/core/quirks.c I think that it would be better to do
> the following in btusb.c:
> 
>        if (id->driver_info & BTUSB_QCA_ROME) {
> 		...
> 	         /* 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;
> 	}
> 
> And the same for driver_info & BTUSB_REALTEK ?
> 
> And then remove the DIY reset_resume handling completely. If you agree that
> that is the best way forward, then I can submit a patch doing that.

then lets give this a try and I might have to take the two patches out of bluetooth-next which are adding USB core quirks.

Regards

Marcel