Date:   Fri, 25 Sep 2020 06:40:20 +0000
From:   陆朱伟 <alex_lu@...lsil.com.cn>
To:     Abhishek Pandit-Subedi <abhishekpandit@...omium.org>,
        Kai-Heng Feng <kai.heng.feng@...onical.com>
CC:     Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        "open list:BLUETOOTH DRIVERS" <linux-bluetooth@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:USB XHCI DRIVER" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH] Bluetooth: btusb: Avoid unnecessary reset upon system resume

Hi Abhishek,

> On September 25, 2020 at 11:34, Abhishek Pandit-Subedi wrote:
> 
> + Alex Lu (who contributed the original change)
> 
> Hi Kai-Heng,
> 
> 
> On Thu, Sep 24, 2020 at 12:10 AM Kai-Heng Feng
> <kai.heng.feng@...onical.com> wrote:
> >
> > [+Cc linux-usb]
> >
> > Hi Abhishek,
> >
> > > On Sep 24, 2020, at 04:41, Abhishek Pandit-Subedi
> <abhishekpandit@...omium.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?
> 
> From btusb.c:
> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-
> next.git/tree/drivers/bluetooth/btusb.c#n4189
> /* Realtek devices lose their updated firmware over global
> * suspend that means host doesn't send SET_FEATURE
> * (DEVICE_REMOTE_WAKEUP)
> */
> 
> Runtime suspend always requires remote wakeup to be set and reset
> resume isn't used there.
> 
> During system suspend, when remote wakeup is not set, RTL8822CE loses
> the FW loaded by the driver and any state currently in the controller.
> This causes the kernel and the controller state to go out of sync.
> One of the issues we observed on the Realtek controller without the
> reset resume quirk was that paired or connected devices would just
> stop working after resume.
> 
> >
> > > 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 think the confusion here stems from what is actually being turned
> off between our two boards and what we're referring to as firmware :)
> 
> In your case, the Realtek controller retains firmware unless the
> platform cuts of power to USB (which it does during S3).
> In my case, the Realtek controller loses firmware when Remote Wakeup
> isn't set, even if the platform doesn't cut power to USB.
> 
> In your case, since you don't need to enforce the 'Remote Wakeup' bit,
> if you unset the BTUSB_WAKEUP_DISABLE for that VID:PID, you should get
> the desirable behavior (which is actually the default behavior; remote
> wake will always be asserted instead of only during Runtime Suspend).
> 
> @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?
> 

According to the feedback from our firmware engineers, all Realtek controllers should have the same behavior on suspend and resume.
@ Kai-Heng, " Bluetooth: hci0: command 0x1001 tx timeout " is irrelevant to firmware loss or not. The rom code in controller supports this command.

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

Powered by blists - more mailing lists