+ Alex Lu (who contributed the original change)
Hi Kai-Heng,
On Thu, Sep 24, 2020 at 12:10 AM Kai-Heng Feng
<[email protected]> wrote:
>
> [+Cc linux-usb]
>
> Hi Abhishek,
>
> > On Sep 24, 2020, at 04:41, Abhishek Pandit-Subedi <[email protected]> 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?
> >
> > 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
> > <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
> >>
>
Am Donnerstag, den 24.09.2020, 20:33 -0700 schrieb Abhishek Pandit-
Subedi:
> Runtime suspend always requires remote wakeup to be set
No, not entirely. Btusb requires remote wakeup between open()
and close(). On a closed device it is not set to save more power.
> and reset
> resume isn't used there.
reset_resume() is basically incompatible with remote wakeup.
Remote wakeup implies that you will ask the device to perform
some action after wakeup. A reset destroys that capability.
> During system suspend, when remote wakeup is not set, RTL8822CE loses
> the FW loaded by the driver and any state currently in the controller.
That is true after every suspend, whether runtime or system.
The device throws away its firmware when it does not need it.
You have control over this feature.
Hence the quirk enables remote
wakeup upon close().
That is the most important part of the original patch.
> 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.
The logic on whether reset_resume() should be used is imperfect.
> > 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.
Technically we tell the BIOS before suspend the system whether
a device should wake up the system during sleep and hope that
the BIOS is clever enough not to cut power to them.
Compliance is mixed.
> 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).
Well, no. Only between open() and close()
Please always test the case of Bluetooth not being used. I know
it is not sexy, but surprisingly common.
Regards
Oliver