| From 3148af2f656325278e265c21e86d08979a64a382 Mon Sep 17 00:00:00 2001 |
| From: Daniel Winkler <danielwinkler@google.com> |
| Date: Wed, 10 Feb 2021 13:52:05 -0800 |
| Subject: [PATCH] CHROMIUM: Track adv state correctly when extended adv |
| available |
| |
| We have discovered that in some use cases, the HCI_LE_ADV will not |
| correctly reflect the advertising state when extended advertising is |
| available. Due to the logic from pre-extended implementation, when an |
| "enable advertising" command is received with enable=1, HCI_LE_ADV is |
| set to True, and if enable=0, HCI_LE_ADV is set to false. However, as |
| ext_adv allows multiple concurrent advertisements, this logic is now |
| incorrect. Consider the following sequence of bluez events: |
| |
| 1. RegisterAdvertisement(adv1) |
| 2. RegisterAdvertisement(adv2) |
| 3. UnregisterAdvertisement(adv2) |
| |
| In step 3, HCI_LE_ADV is set to false even though adv1 is still active. |
| This patch does the following to improve correctness of the HCI_LE_ADV |
| flag: |
| |
| * Since directed advertisements are not tracked via the |
| hdev->adv_instances list, add a new hdev->ext_directed_advertising |
| bool to track when directed advertising is occurring. |
| * Since extended advertising now has a new hci_le_ext_adv_term_evt event |
| that fires when the controller decides to cancel an advertisement due |
| to a timeout, a connect event to that adv's address, etc., allow |
| HCI_LE_ADV to be updated here as well. |
| * Since hci_le_ext_adv_term_evt cancels an adv, we now remove this adv |
| from hdev->adv_instances. |
| |
| Note that in the third point here, we remove the cancelled adv from |
| hdev->adv_instances, but this patch does not handle the propagation to |
| userspace (and client). This patch simply syncs the kernel to the |
| controller's adv state, which is necessary to have an accurate count of |
| active advertisements. |
| |
| The patch is tested with our QuickHealth and AdvHealth automated test |
| suites. Additionally, it is tested by performing the above steps and |
| then validating that the HCI_LE_ADV flag correctly represents the |
| controller state. |
| |
| Signed-off-by: Daniel Winkler <danielwinkler@google.com> |
| |
| BUG=b:181049961 |
| TEST=AdvHealth.adv_nearby_test on Ezkinil |
| |
| Change-Id: I3a0f06b0eaafff9bece6e073326b8986a3098532 |
| Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2733175 |
| Reviewed-by: Alain Michaud <alainm@chromium.org> |
| Commit-Queue: Daniel Winkler <danielwinkler@google.com> |
| Tested-by: Daniel Winkler <danielwinkler@google.com> |
| --- |
| include/net/bluetooth/hci.h | 1 + |
| include/net/bluetooth/hci_core.h | 2 ++ |
| net/bluetooth/hci_core.c | 1 + |
| net/bluetooth/hci_event.c | 24 ++++++++++++++++++++++-- |
| 4 files changed, 26 insertions(+), 2 deletions(-) |
| |
| diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h |
| --- a/include/net/bluetooth/hci.h |
| +++ b/include/net/bluetooth/hci.h |
| @@ -564,6 +564,7 @@ enum { |
| #define HCI_ERROR_INVALID_LL_PARAMS 0x1e |
| #define HCI_ERROR_UNSPECIFIED 0x1f |
| #define HCI_ERROR_ADVERTISING_TIMEOUT 0x3c |
| +#define HCI_ERROR_CANCELLED_BY_HOST 0x44 |
| |
| /* Flow control modes */ |
| #define HCI_FLOW_CTL_MODE_PACKET_BASED 0x00 |
| diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h |
| --- a/include/net/bluetooth/hci_core.h |
| +++ b/include/net/bluetooth/hci_core.h |
| @@ -564,6 +564,8 @@ struct hci_dev { |
| __u8 cur_adv_instance; |
| __u16 adv_instance_timeout; |
| struct delayed_work adv_instance_expire; |
| + /* We can have a single directed advertisement using adv handle 0 */ |
| + bool ext_directed_advertising; |
| |
| struct idr adv_monitors_idr; |
| unsigned int adv_monitors_cnt; |
| diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c |
| --- a/net/bluetooth/hci_core.c |
| +++ b/net/bluetooth/hci_core.c |
| @@ -3785,6 +3785,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) |
| hdev->adv_instance_cnt = 0; |
| hdev->cur_adv_instance = 0x00; |
| hdev->adv_instance_timeout = 0; |
| + hdev->ext_directed_advertising = false; |
| hdev->eir_max_name_len = 48; |
| |
| hdev->advmon_allowlist_duration = 300; |
| diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c |
| --- a/net/bluetooth/hci_event.c |
| +++ b/net/bluetooth/hci_event.c |
| @@ -5441,16 +5441,36 @@ static void hci_le_ext_adv_term_evt(struct hci_dev *hdev, struct sk_buff *skb) |
| |
| if (hdev->adv_addr_type != ADDR_LE_DEV_RANDOM || |
| bacmp(&conn->resp_addr, BDADDR_ANY)) |
| - return; |
| + goto cleanup_instance; |
| |
| if (!ev->handle) { |
| bacpy(&conn->resp_addr, &hdev->random_addr); |
| - return; |
| + goto cleanup_instance; |
| } |
| |
| if (adv) |
| bacpy(&conn->resp_addr, &adv->random_addr); |
| } |
| + |
| +cleanup_instance: |
| + /* Since the controller tells us this instance is no longer active, we |
| + * remove it. |
| + * |
| + * One caveat is that if the status is HCI_ERROR_CANCELLED_BY_HOST, this |
| + * event is being fired as a result of a hci_cp_le_set_ext_adv_enable |
| + * disable request, which will have its own callback and cleanup via |
| + * the hci_cc_le_set_ext_adv_enable path. If this is the case, we will |
| + * not remove the instance, as it may only be a temporary disable. |
| + */ |
| + if (ev->status != HCI_ERROR_CANCELLED_BY_HOST) { |
| + hci_remove_adv_instance(hdev, ev->handle); |
| + |
| + /* If we are no longer advertising, clear HCI_LE_ADV */ |
| + if (list_empty(&hdev->adv_instances) && |
| + !hdev->ext_directed_advertising) { |
| + hci_dev_clear_flag(hdev, HCI_LE_ADV); |
| + } |
| + } |
| } |
| |
| static void hci_le_conn_update_complete_evt(struct hci_dev *hdev, |
| -- |
| 2.33.0.464.g1972c5931b-goog |
| |