Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\))
Subject: Re: [PATCH] Bluetooth: btusb: Avoid unnecessary reset upon system
 resume
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
In-Reply-To: <30ce4ee1eede47c09c3e7f277c26918a@realsil.com.cn>
Date:   Fri, 25 Sep 2020 19:51:06 +0800
Cc:     Abhishek Pandit-Subedi <abhishekpandit@chromium.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        "open list:BLUETOOTH DRIVERS" <linux-bluetooth@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:USB XHCI DRIVER" <linux-usb@vger.kernel.org>
Content-Transfer-Encoding: 8BIT
Message-Id: <E2E04FDC-C62A-4F8C-A8EF-20868504A2BC@canonical.com>
References: <30ce4ee1eede47c09c3e7f277c26918a@realsil.com.cn>
To:     =?utf-8?B?6ZmG5pyx5Lyf?= <alex_lu@realsil.com.cn>
Precedence: bulk

Hi Alex,

> On Sep 25, 2020, at 16:23, 陆朱伟 <alex_lu@realsil.com.cn> wrote:
> 
> 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.

For my case, the firmware is retained after S3, despite of "reset_resume = 1":

[ 30.164036] ACPI: Waking up from system sleep state S3 
[ 30.167913] ACPI: EC: interrupt unblocked
[ 31.284138] ACPI: EC: event unblocked
...
[   31.467484] usb 1-14: reset full-speed USB device number 3 using xhci_hcd
...
[   32.732934] Bluetooth: hci0: RTL: examining hci_ver=08 hci_rev=826c lmp_ver=08 lmp_subver=a99e
[   32.732937] Bluetooth: hci0: RTL: unknown IC info, lmp subver a99e, hci rev 826c, hci ver 0008
[   32.732937] Bluetooth: hci0: RTL: assuming no firmware upload needed

Kai-Heng

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