Re: [PATCH] Bluetooth: btusb: Avoid unnecessary reset upon system resume

[Kai-Heng Feng <kai.heng.feng@canonical.com>]

Hi Alex,

> On Sep 25, 2020, at 15:42, 陆朱伟 <alex_lu@realsil.com.cn> wrote:
> 
> Hi Kai-Heng,
> 
>> On 25 September 2020 at 15:14, Kai-Heng Feng wrote:
>> 
>> Hi Alex,

[snipped]

>> Apparently for my case, RTL8821CE, firmware was kept without setting
>> remote wakeup.
> 
> So you got the btusb disconnect and reprobe sequence after resume, and " Bluetooth: hci0: command 0x1001 tx timeout " before firmware downloading ?

USB power wasn't lost, but it got USB warm reset because btusb driver explicitly flagged "reset_resume = 1".
Then the issue appeared as "Bluetooth: hci0: command 0x1001 tx timeout", before downloading firmware.

> 
>> Is it okay to also set remote wakeup for global suspend to retain the
>> firmware?
> 
> Yes, it's ok.

Abhishek, does setting remote wakeup during global suspend works for you?

> 
>> If firmware was retained, does USB warm reset affect BT controller in
>> anyway?
> 
> USB warm reset shouldn't affect BT controller.
> But hci device will not work after resume, because btrtl will find "unknown IC info, lmp subvert ..." and return error when hci device setup is called.
> Tips: The lmp subver in controller changes after firmware downloading. And driver will find " unknown IC info, lmp subver  ..." when setup is called with firmware retained.

This should already be fixed by "Bluetooth: btrtl: Restore old logic to assume firmware is already loaded".

Kai-Heng

> 
>> 
>> Kai-Heng
>> 
>>> 
>>>> 
>>>> Kai-Heng
>>>> 
>>>>> 
>>>>> @Alex -- What is the common behavior for Realtek controllers? Should
>>>>> we set BTUSB_WAKEUP_DISABLE only on RTL8822CE or should we unset
>> it
>>>>> only on RTL8821CE?
>>>>> 
>>>>>>> 
>>>>>>> I would prefer this doesn't get accepted in its current state.
>>>>>> 
>>>>>> Of course.
>>>>>> I think we need to find the root cause for your case before applying this
>>>> one.
>>>>>> 
>>>>>> Kai-Heng
>>>>>> 
>>>>>>> 
>>>>>>> Abhishek
>>>>>>> 
>>>>>>> On Wed, Sep 23, 2020 at 10:56 AM Kai-Heng Feng
>>>>>>> <kai.heng.feng@canonical.com> wrote:
>>>>>>>> 
>>>>>>>> Realtek bluetooth controller may fail to work after system sleep:
>>>>>>>> [ 1272.707670] Bluetooth: hci0: command 0x1001 tx timeout
>>>>>>>> [ 1280.835712] Bluetooth: hci0: RTL: HCI_OP_READ_LOCAL_VERSION
>>>> failed (-110)
>>>>>>>> 
>>>>>>>> If platform firmware doesn't cut power off during suspend, the
>>>> firmware
>>>>>>>> is considered retained in controller but the driver is still asking USB
>>>>>>>> core to perform a reset-resume. This can make bluetooth controller
>>>>>>>> unusable.
>>>>>>>> 
>>>>>>>> So avoid unnecessary reset to resolve the issue.
>>>>>>>> 
>>>>>>>> For devices that really lose power during suspend, USB core will
>> detect
>>>>>>>> and handle reset-resume correctly.
>>>>>>>> 
>>>>>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>>>>>> ---
>>>>>>>> drivers/bluetooth/btusb.c | 8 +++-----
>>>>>>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>>>> 
>>>>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>>>>> index 8d2608ddfd08..de86ef4388f9 100644
>>>>>>>> --- a/drivers/bluetooth/btusb.c
>>>>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>>>>> @@ -4255,17 +4255,15 @@ static int btusb_suspend(struct
>>>> usb_interface *intf, pm_message_t message)
>>>>>>>>             enable_irq(data->oob_wake_irq);
>>>>>>>>     }
>>>>>>>> 
>>>>>>>> -       /* For global suspend, Realtek devices lose the loaded fw
>>>>>>>> -        * in them. But for autosuspend, firmware should remain.
>>>>>>>> -        * Actually, it depends on whether the usb host sends
>>>>>>>> +       /* For global suspend, Realtek devices lose the loaded fw in
>> them
>>>> if
>>>>>>>> +        * platform firmware cut power off. But for autosuspend,
>>>> firmware
>>>>>>>> +        * should remain.  Actually, it depends on whether the usb host
>>>> sends
>>>>>>>>      * set feature (enable wakeup) or not.
>>>>>>>>      */
>>>>>>>>     if (test_bit(BTUSB_WAKEUP_DISABLE, &data->flags)) {
>>>>>>>>             if (PMSG_IS_AUTO(message) &&
>>>>>>>>                 device_can_wakeup(&data->udev->dev))
>>>>>>>>                     data->udev->do_remote_wakeup = 1;
>>>>>>>> -               else if (!PMSG_IS_AUTO(message))
>>>>>>>> -                       data->udev->reset_resume = 1;
>>>>>>>>     }
>>>>>>>> 
>>>>>>>>     return 0;
>>>>>>>> --
>>>>>>>> 2.17.1
>>>> 
>>>> 
>>>> ------Please consider the environment before printing this e-mail.