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@xxxxxxxxxx>
>>>> ---
>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
