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

[陆朱伟 <alex_lu@realsil.com.cn>]

Hi Kai-Heng,

> On September 25, 2020 at 15:56, Kai-Heng Feng wrote:
> 
> 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?
 
It depends on your desire on power consumption during global suspend.
The BT controller takes less power if firmware was lost during global suspend.

> 
> >
> >> 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.