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=us-ascii
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: <CANFp7mV7fC9_EZHd7B0Cu-owgCVdA6CNd2bb7XwFf5+6b7FVpg@mail.gmail.com>
Date:   Thu, 24 Sep 2020 15:10:31 +0800
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Alex Lu <alex_lu@realsil.com.cn>,
        "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: <A6F728DB-A07B-4323-83A5-3DABA8FDC156@canonical.com>
References: <20200923175602.9523-1-kai.heng.feng@canonical.com>
 <CANFp7mV7fC9_EZHd7B0Cu-owgCVdA6CNd2bb7XwFf5+6b7FVpg@mail.gmail.com>
To:     Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Precedence: bulk

[+Cc linux-usb]

Hi Abhishek,

> On Sep 24, 2020, at 04:41, Abhishek Pandit-Subedi <abhishekpandit@chromium.org> wrote:
> 
> Hi Kai-Heng,
> 
> Which Realtek controller is this on?'

The issue happens on 8821CE.

> 
> Specifically for RTL8822CE, we tested without reset_resume being set
> and that was causing the controller being reset without bluez ever
> learning about it (resulting in devices being unusable without
> toggling the BT power).

The reset is done by the kernel, so how does that affect bluez?

From what you described, it sounds more like runtime resume since bluez is already running.
If we need reset resume for runtime resume, maybe it's another bug which needs to be addressed?

> If the firmware doesn't cut off power during suspend, maybe you
> shouldn't set the BTUSB_WAKEUP_DISABLE flag for that controller.

We don't know beforehand if the platform firmware (BIOS for my case) will cut power off or not.

In general, laptops will cut off the USB power during S3.
When AC is plugged, some laptops cuts USB power off and some don't. This also applies to many desktops. Not to mention there can be BIOS options to control USB power under S3/S4/S5...

So we don't know beforehand.

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