Hi All,
While debugging an rtl8723bs bluetooth issue I noticed that last year
the bluetooth core has grown a new hci_suspend_notifier() mechanism and
I noticed a number of possible issues / improvements with that mechanism
which I noticed:
1. HCI_OP_WRITE_SCAN_ENABLE gets send to the HCI in some places without
a hci_dev_test_flag(hdev, HCI_BREDR_ENABLED) check
2. HCI_OP_SET_EVENT_FLT gets end to the HCI in some places without
a hci_dev_test_flag(hdev, HCI_BREDR_ENABLED) check
3. hci_req_set_event_filter sends the following requests:
1 HCI_OP_SET_EVENT_FLT
2 HCI_OP_WRITE_SCAN_ENABLE (if the scan parameters have changed only)
3 HCI_OP_SET_EVENT_FLT (if their are relevant whitelist entries_
4 HCI_OP_WRITE_SCAN_ENABLE unconditionally
It seems to me that 2. is unnecessary since it will immediately be
followed by 4. and 4. misses the check to see if the scan parameters
need updating which 2 does (this check is done in __hci_req_update_scan()
4. There is another unconditional, possibly unnecessary HCI_OP_WRITE_SCAN_ENABLE
in the if (next == BT_SUSPEND_DISCONNECT) {} block of hci_req_prepare_suspend()
Note if this is made conditional then the:
set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
should also be made conditional since then req might not contain any requests
in which case suspend_req_complete() will not run.
Regards,
Hans
Hi Hans,
Thanks for the pointers. I'll try to spin up a patch to fix these
issues (or ask for some help on my team).
Thanks,
Abhishek
On Thu, Jan 28, 2021 at 4:00 AM Hans de Goede <[email protected]> wrote:
>
> Hi All,
>
> While debugging an rtl8723bs bluetooth issue I noticed that last year
> the bluetooth core has grown a new hci_suspend_notifier() mechanism and
> I noticed a number of possible issues / improvements with that mechanism
> which I noticed:
>
> 1. HCI_OP_WRITE_SCAN_ENABLE gets send to the HCI in some places without
> a hci_dev_test_flag(hdev, HCI_BREDR_ENABLED) check
>
> 2. HCI_OP_SET_EVENT_FLT gets end to the HCI in some places without
> a hci_dev_test_flag(hdev, HCI_BREDR_ENABLED) check
>
> 3. hci_req_set_event_filter sends the following requests:
> 1 HCI_OP_SET_EVENT_FLT
> 2 HCI_OP_WRITE_SCAN_ENABLE (if the scan parameters have changed only)
> 3 HCI_OP_SET_EVENT_FLT (if their are relevant whitelist entries_
> 4 HCI_OP_WRITE_SCAN_ENABLE unconditionally
>
> It seems to me that 2. is unnecessary since it will immediately be
> followed by 4. and 4. misses the check to see if the scan parameters
> need updating which 2 does (this check is done in __hci_req_update_scan()
>
>
> 4. There is another unconditional, possibly unnecessary HCI_OP_WRITE_SCAN_ENABLE
> in the if (next == BT_SUSPEND_DISCONNECT) {} block of hci_req_prepare_suspend()
> Note if this is made conditional then the:
> set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
> should also be made conditional since then req might not contain any requests
> in which case suspend_req_complete() will not run.
>
> Regards,
>
> Hans
>