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;
MIME-Version: 1.0
References: <20200923175602.9523-1-kai.heng.feng@canonical.com>
In-Reply-To: <20200923175602.9523-1-kai.heng.feng@canonical.com>
From:   Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Date:   Wed, 23 Sep 2020 13:41:11 -0700
Message-ID: <CANFp7mV7fC9_EZHd7B0Cu-owgCVdA6CNd2bb7XwFf5+6b7FVpg@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: btusb: Avoid unnecessary reset upon system resume
To:     Kai-Heng Feng <kai.heng.feng@canonical.com>
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Kai-Heng,

Which Realtek controller is this on?

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).
If the firmware doesn't cut off power during suspend, maybe you
shouldn't set the BTUSB_WAKEUP_DISABLE flag for that controller.

I would prefer this doesn't get accepted in its current state.

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
>