Re: [PATCH v2 2/2] Bluetooth: hci_h5: Disable the hci_suspend_notifier for btrtl devices

Hi,

On Fri, Jan 29, 2021 at 6:28 AM Hans de Goede [off-list ref] wrote:

Hi,

On 1/29/21 9:25 AM, Max Chou wrote:

quoted

Hi,

quoted

I'm not sure we should enable this for all RTL devices rather than based on the specific project. RTL8822C will also be using hci_h5 but intends to support wake on bt (meaning it shouldn't be losing firmware around suspend).

quoted

+ Max Chou: You are proposing a change to add project id to btrtl.
Should we use that instead to set this quirk for 8723 devices (and others which lose fw around suspend)?
(https://patchwork.kernel.org/project/bluetooth/patch/20210127030152.3940-1-max.chou@realtek.com/)

Agree. Recommend to use the same way as Abhishek mentions that limiting the quirk only for RTL8723B devices if this patch is necessary.
Therefore, some of the projects use RTL8822C devices with BT UART interface would apply BT wakes Host.
So far, I've not heard the issue as this topic.

So I just checked because I was not aware that the hci_h5 code was also being used for the RTL8822C.
I mainly focus on x86/ACPI use and there is no 8822 ACPI device-id in the h5_acpi_match table, but
there is indeed a match for this in the rtl_bluetooth_of_match table.

But ATM the code is treating the 8822 exactly the same as the 8723, including doing a device_reprobe
on resume. So to me it makes sense to set the HCI_UART_NO_SUSPEND_NOTIFIER unconditionally to, as
it is used because of the device_reprobe being done.

Now it might be a good idea to opt out of the device_reprobe for 8822 devices, and/or maybe even for
all devicetree enumerated cases (the device being completely shutoff is an ACPI thing, with dt/of we
should have more control).

To me it seems that since for now the device_reprobe() is unconditional that the matching setting
of the HCI_UART_NO_SUSPEND_NOTIFIER flag should be unconditional too; and then when the device_reprobe()
stuff is made unconditional, then we can make the setting of the flag unconditional using the same check.

I didn't realize this was currently unconditional in code. In that
case, I think it's fine for you to add the flag unconditionally in
hci_h5. When we add support for wake on bt to the h5 driver, we should
make this flag conditional based on whether the driver reprobes on
resume.

I will go back and +1 the original patch.

Thanks,
Abhishek

With that said if people really want it I'm happy to respin this to only apply to the 8723 case,
but that seems weird given that the device_reprobe ATM is being done on the 8822 too.

quoted

+Clair: Have you met this issue during suspend/resume when BT controller is RTL8822C with BT UART interface?
Please see the patchwork. https://patchwork.kernel.org/project/bluetooth/list/?series=423915

Regards,

Hans