Recent(ish) bluetooth hci_suspend_notifier() changes

Recent(ish) bluetooth hci_suspend_notifier() changes

[Hans de Goede]

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

Re: Recent(ish) bluetooth hci_suspend_notifier() changes

[Abhishek Pandit-Subedi]

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 <hdegoede@redhat.com> 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
>