| Message ID | 20220111081048.v10.1.Ic0a40b84dee3825302890aaea690e73165c71820@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0e81371960da1de899320f8fb13e1988713270e4 |
| Headers | show |
| Series | [v10,1/2] bluetooth: msft: Handle MSFT Monitor Device Event | expand |
Hi Manish, On Tue, Jan 18, 2022 at 10:32 AM Manish Mandlik <mmandlik@google.com> wrote: > > Friendly ping to review this patch series. :) > > On Tue, Jan 11, 2022 at 11:14 AM Manish Mandlik <mmandlik@google.com> wrote: >> >> Whenever the controller starts/stops monitoring a bt device, it sends >> MSFT Monitor Device event. Add handler to read this vendor event. >> >> Test performed: >> - Verified by logs that the MSFT Monitor Device event is received from >> the controller whenever it starts/stops monitoring a device. >> >> Signed-off-by: Manish Mandlik <mmandlik@google.com> >> Reviewed-by: Miao-chen Chou <mcchou@google.com> >> --- >> Hello Bt-Maintainers, >> >> As mentioned in the bluez patch series [1], we need to capture the 'MSFT >> Monitor Device' event from the controller and pass on the necessary >> information to the bluetoothd. >> >> This is required to further optimize the power consumption by avoiding >> handling of RSSI thresholds and timeouts in the user space and let the >> controller do the RSSI tracking. >> >> This patch series adds support to read the MSFT vendor event >> HCI_VS_MSFT_LE_Monitor_Device_Event and introduces new MGMT events >> MGMT_EV_ADV_MONITOR_DEVICE_FOUND and MGMT_EV_ADV_MONITOR_DEVICE_LOST to >> indicate that the controller has started/stopped tracking a particular >> device. >> >> Please let me know what you think about this or if you have any further >> questions. >> >> [1] https://patchwork.kernel.org/project/bluetooth/list/?series=583423 >> >> Thanks, >> Manish. >> >> Changes in v10: >> - Create a helper function to delete monitor device. >> - Fix inconsistent returns '&hdev->lock'. >> >> Changes in v9: >> - Fix compiler error. >> >> Changes in v8: >> - Fix use-after-free in msft_le_cancel_monitor_advertisement_cb(). >> - Use skb_pull_data() instead of skb_pull(). >> >> Changes in v6: >> - Fix compiler warning bt_dev_err() missing argument. >> >> Changes in v5: >> - Split v4 into two patches. >> - Buffer controller Device Found event and maintain the device tracking >> state in the kernel. >> >> Changes in v4: >> - Add Advertisement Monitor Device Found event and update addr type. >> >> Changes in v3: >> - Discard changes to the Device Found event and notify bluetoothd only >> when the controller stops monitoring the device via new Device Lost >> event. >> >> Changes in v2: >> - Instead of creating a new 'Device Tracking' event, add a flag 'Device >> Tracked' in the existing 'Device Found' event and add a new 'Device >> Lost' event to indicate that the controller has stopped tracking that >> device. >> >> include/net/bluetooth/hci_core.h | 11 +++ >> net/bluetooth/hci_core.c | 1 + >> net/bluetooth/msft.c | 158 +++++++++++++++++++++++++++++-- >> 3 files changed, 162 insertions(+), 8 deletions(-) >> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h >> index 586f69d084a2..639fb9f57ae7 100644 >> --- a/include/net/bluetooth/hci_core.h >> +++ b/include/net/bluetooth/hci_core.h >> @@ -258,6 +258,15 @@ struct adv_info { >> >> #define HCI_ADV_TX_POWER_NO_PREFERENCE 0x7F >> >> +struct monitored_device { >> + struct list_head list; >> + >> + bdaddr_t bdaddr; >> + __u8 addr_type; >> + __u16 handle; >> + bool notified; >> +}; >> + >> struct adv_pattern { >> struct list_head list; >> __u8 ad_type; >> @@ -591,6 +600,8 @@ struct hci_dev { >> >> struct delayed_work interleave_scan; >> >> + struct list_head monitored_devices; >> + >> #if IS_ENABLED(CONFIG_BT_LEDS) >> struct led_trigger *power_led; >> #endif >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c >> index 2b7bd3655b07..14c2da9d33ff 100644 >> --- a/net/bluetooth/hci_core.c >> +++ b/net/bluetooth/hci_core.c >> @@ -2503,6 +2503,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) >> INIT_LIST_HEAD(&hdev->conn_hash.list); >> INIT_LIST_HEAD(&hdev->adv_instances); >> INIT_LIST_HEAD(&hdev->blocked_keys); >> + INIT_LIST_HEAD(&hdev->monitored_devices); >> >> INIT_LIST_HEAD(&hdev->local_codecs); >> INIT_WORK(&hdev->rx_work, hci_rx_work); >> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c >> index 6a943634b31a..213eab2f085a 100644 >> --- a/net/bluetooth/msft.c >> +++ b/net/bluetooth/msft.c >> @@ -80,6 +80,14 @@ struct msft_rp_le_set_advertisement_filter_enable { >> __u8 sub_opcode; >> } __packed; >> >> +#define MSFT_EV_LE_MONITOR_DEVICE 0x02 >> +struct msft_ev_le_monitor_device { >> + __u8 addr_type; >> + bdaddr_t bdaddr; >> + __u8 monitor_handle; >> + __u8 monitor_state; >> +} __packed; >> + >> struct msft_monitor_advertisement_handle_data { >> __u8 msft_handle; >> __u16 mgmt_handle; >> @@ -204,6 +212,30 @@ static struct msft_monitor_advertisement_handle_data *msft_find_handle_data >> return NULL; >> } >> >> +/* This function requires the caller holds hdev->lock */ >> +static int msft_monitor_device_del(struct hci_dev *hdev, __u16 mgmt_handle, >> + bdaddr_t *bdaddr, __u8 addr_type) >> +{ >> + struct monitored_device *dev, *tmp; >> + int count = 0; >> + >> + list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, list) { >> + /* mgmt_handle == 0 indicates remove all devices, whereas, >> + * bdaddr == NULL indicates remove all devices matching the >> + * mgmt_handle. >> + */ >> + if ((!mgmt_handle || dev->handle == mgmt_handle) && >> + (!bdaddr || (!bacmp(bdaddr, &dev->bdaddr) && >> + addr_type == dev->addr_type))) { >> + list_del(&dev->list); >> + kfree(dev); >> + count++; >> + } >> + } >> + >> + return count; >> +} >> + >> static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev, >> u8 status, u16 opcode, >> struct sk_buff *skb) >> @@ -294,6 +326,10 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev, >> if (monitor && !msft->suspending) >> hci_free_adv_monitor(hdev, monitor); >> >> + /* Clear any monitored devices by this Adv Monitor */ >> + msft_monitor_device_del(hdev, handle_data->mgmt_handle, NULL, >> + 0); >> + >> list_del(&handle_data->list); >> kfree(handle_data); >> } >> @@ -557,6 +593,13 @@ void msft_do_close(struct hci_dev *hdev) >> list_del(&handle_data->list); >> kfree(handle_data); >> } >> + >> + hci_dev_lock(hdev); >> + >> + /* Clear any devices that are being monitored */ >> + msft_monitor_device_del(hdev, 0, NULL, 0); >> + >> + hci_dev_unlock(hdev); >> } >> >> void msft_register(struct hci_dev *hdev) >> @@ -590,10 +633,97 @@ void msft_unregister(struct hci_dev *hdev) >> kfree(msft); >> } >> >> +/* This function requires the caller holds hdev->lock */ >> +static void msft_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, >> + __u8 addr_type, __u16 mgmt_handle) >> +{ >> + struct monitored_device *dev; >> + >> + dev = kmalloc(sizeof(*dev), GFP_KERNEL); >> + if (!dev) { >> + bt_dev_err(hdev, "MSFT vendor event %u: no memory", >> + MSFT_EV_LE_MONITOR_DEVICE); >> + return; >> + } >> + >> + bacpy(&dev->bdaddr, bdaddr); >> + dev->addr_type = addr_type; >> + dev->handle = mgmt_handle; >> + dev->notified = false; >> + >> + INIT_LIST_HEAD(&dev->list); >> + list_add(&dev->list, &hdev->monitored_devices); >> +} >> + >> +/* This function requires the caller holds hdev->lock */ >> +static void msft_device_lost(struct hci_dev *hdev, bdaddr_t *bdaddr, >> + __u8 addr_type, __u16 mgmt_handle) >> +{ >> + if (!msft_monitor_device_del(hdev, mgmt_handle, bdaddr, addr_type)) { >> + bt_dev_err(hdev, "MSFT vendor event %u: dev %pMR not in list", >> + MSFT_EV_LE_MONITOR_DEVICE, bdaddr); >> + } >> +} >> + >> +static void *msft_skb_pull(struct hci_dev *hdev, struct sk_buff *skb, >> + u8 ev, size_t len) >> +{ >> + void *data; >> + >> + data = skb_pull_data(skb, len); >> + if (!data) >> + bt_dev_err(hdev, "Malformed MSFT vendor event: 0x%02x", ev); >> + >> + return data; >> +} >> + >> +/* This function requires the caller holds hdev->lock */ >> +static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb) >> +{ >> + struct msft_ev_le_monitor_device *ev; >> + struct msft_monitor_advertisement_handle_data *handle_data; >> + u8 addr_type; >> + >> + ev = msft_skb_pull(hdev, skb, MSFT_EV_LE_MONITOR_DEVICE, sizeof(*ev)); >> + if (!ev) >> + return; >> + >> + bt_dev_dbg(hdev, >> + "MSFT vendor event 0x%02x: handle 0x%04x state %d addr %pMR", >> + MSFT_EV_LE_MONITOR_DEVICE, ev->monitor_handle, >> + ev->monitor_state, &ev->bdaddr); >> + >> + handle_data = msft_find_handle_data(hdev, ev->monitor_handle, false); >> + >> + switch (ev->addr_type) { >> + case ADDR_LE_DEV_PUBLIC: >> + addr_type = BDADDR_LE_PUBLIC; >> + break; >> + >> + case ADDR_LE_DEV_RANDOM: >> + addr_type = BDADDR_LE_RANDOM; >> + break; >> + >> + default: >> + bt_dev_err(hdev, >> + "MSFT vendor event 0x%02x: unknown addr type 0x%02x", >> + MSFT_EV_LE_MONITOR_DEVICE, ev->addr_type); >> + return; >> + } >> + >> + if (ev->monitor_state) >> + msft_device_found(hdev, &ev->bdaddr, addr_type, >> + handle_data->mgmt_handle); >> + else >> + msft_device_lost(hdev, &ev->bdaddr, addr_type, >> + handle_data->mgmt_handle); >> +} >> + >> void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb) >> { >> struct msft_data *msft = hdev->msft_data; >> - u8 event; >> + u8 *evt_prefix; >> + u8 *evt; >> >> if (!msft) >> return; >> @@ -602,13 +732,12 @@ void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb) >> * matches, and otherwise just return. >> */ >> if (msft->evt_prefix_len > 0) { >> - if (skb->len < msft->evt_prefix_len) >> + evt_prefix = msft_skb_pull(hdev, skb, 0, msft->evt_prefix_len); >> + if (!evt_prefix) >> return; >> >> - if (memcmp(skb->data, msft->evt_prefix, msft->evt_prefix_len)) >> + if (memcmp(evt_prefix, msft->evt_prefix, msft->evt_prefix_len)) >> return; >> - >> - skb_pull(skb, msft->evt_prefix_len); >> } >> >> /* Every event starts at least with an event code and the rest of >> @@ -617,10 +746,23 @@ void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb) >> if (skb->len < 1) >> return; >> >> - event = *skb->data; >> - skb_pull(skb, 1); >> + evt = msft_skb_pull(hdev, skb, 0, sizeof(*evt)); >> + if (!evt) >> + return; >> + >> + hci_dev_lock(hdev); >> + >> + switch (*evt) { >> + case MSFT_EV_LE_MONITOR_DEVICE: >> + msft_monitor_device_evt(hdev, skb); >> + break; >> >> - bt_dev_dbg(hdev, "MSFT vendor event %u", event); >> + default: >> + bt_dev_dbg(hdev, "MSFT vendor event 0x%02x", *evt); >> + break; >> + } >> + >> + hci_dev_unlock(hdev); >> } >> >> __u64 msft_get_features(struct hci_dev *hdev) >> -- >> 2.34.1.575.g55b058a8bb-goog >> Applied, thanks.
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 586f69d084a2..639fb9f57ae7 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -258,6 +258,15 @@ struct adv_info { #define HCI_ADV_TX_POWER_NO_PREFERENCE 0x7F +struct monitored_device { + struct list_head list; + + bdaddr_t bdaddr; + __u8 addr_type; + __u16 handle; + bool notified; +}; + struct adv_pattern { struct list_head list; __u8 ad_type; @@ -591,6 +600,8 @@ struct hci_dev { struct delayed_work interleave_scan; + struct list_head monitored_devices; + #if IS_ENABLED(CONFIG_BT_LEDS) struct led_trigger *power_led; #endif diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 2b7bd3655b07..14c2da9d33ff 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -2503,6 +2503,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) INIT_LIST_HEAD(&hdev->conn_hash.list); INIT_LIST_HEAD(&hdev->adv_instances); INIT_LIST_HEAD(&hdev->blocked_keys); + INIT_LIST_HEAD(&hdev->monitored_devices); INIT_LIST_HEAD(&hdev->local_codecs); INIT_WORK(&hdev->rx_work, hci_rx_work); diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c index 6a943634b31a..213eab2f085a 100644 --- a/net/bluetooth/msft.c +++ b/net/bluetooth/msft.c @@ -80,6 +80,14 @@ struct msft_rp_le_set_advertisement_filter_enable { __u8 sub_opcode; } __packed; +#define MSFT_EV_LE_MONITOR_DEVICE 0x02 +struct msft_ev_le_monitor_device { + __u8 addr_type; + bdaddr_t bdaddr; + __u8 monitor_handle; + __u8 monitor_state; +} __packed; + struct msft_monitor_advertisement_handle_data { __u8 msft_handle; __u16 mgmt_handle; @@ -204,6 +212,30 @@ static struct msft_monitor_advertisement_handle_data *msft_find_handle_data return NULL; } +/* This function requires the caller holds hdev->lock */ +static int msft_monitor_device_del(struct hci_dev *hdev, __u16 mgmt_handle, + bdaddr_t *bdaddr, __u8 addr_type) +{ + struct monitored_device *dev, *tmp; + int count = 0; + + list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, list) { + /* mgmt_handle == 0 indicates remove all devices, whereas, + * bdaddr == NULL indicates remove all devices matching the + * mgmt_handle. + */ + if ((!mgmt_handle || dev->handle == mgmt_handle) && + (!bdaddr || (!bacmp(bdaddr, &dev->bdaddr) && + addr_type == dev->addr_type))) { + list_del(&dev->list); + kfree(dev); + count++; + } + } + + return count; +} + static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev, u8 status, u16 opcode, struct sk_buff *skb) @@ -294,6 +326,10 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev, if (monitor && !msft->suspending) hci_free_adv_monitor(hdev, monitor); + /* Clear any monitored devices by this Adv Monitor */ + msft_monitor_device_del(hdev, handle_data->mgmt_handle, NULL, + 0); + list_del(&handle_data->list); kfree(handle_data); } @@ -557,6 +593,13 @@ void msft_do_close(struct hci_dev *hdev) list_del(&handle_data->list); kfree(handle_data); } + + hci_dev_lock(hdev); + + /* Clear any devices that are being monitored */ + msft_monitor_device_del(hdev, 0, NULL, 0); + + hci_dev_unlock(hdev); } void msft_register(struct hci_dev *hdev) @@ -590,10 +633,97 @@ void msft_unregister(struct hci_dev *hdev) kfree(msft); } +/* This function requires the caller holds hdev->lock */ +static void msft_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, + __u8 addr_type, __u16 mgmt_handle) +{ + struct monitored_device *dev; + + dev = kmalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) { + bt_dev_err(hdev, "MSFT vendor event %u: no memory", + MSFT_EV_LE_MONITOR_DEVICE); + return; + } + + bacpy(&dev->bdaddr, bdaddr); + dev->addr_type = addr_type; + dev->handle = mgmt_handle; + dev->notified = false; + + INIT_LIST_HEAD(&dev->list); + list_add(&dev->list, &hdev->monitored_devices); +} + +/* This function requires the caller holds hdev->lock */ +static void msft_device_lost(struct hci_dev *hdev, bdaddr_t *bdaddr, + __u8 addr_type, __u16 mgmt_handle) +{ + if (!msft_monitor_device_del(hdev, mgmt_handle, bdaddr, addr_type)) { + bt_dev_err(hdev, "MSFT vendor event %u: dev %pMR not in list", + MSFT_EV_LE_MONITOR_DEVICE, bdaddr); + } +} + +static void *msft_skb_pull(struct hci_dev *hdev, struct sk_buff *skb, + u8 ev, size_t len) +{ + void *data; + + data = skb_pull_data(skb, len); + if (!data) + bt_dev_err(hdev, "Malformed MSFT vendor event: 0x%02x", ev); + + return data; +} + +/* This function requires the caller holds hdev->lock */ +static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb) +{ + struct msft_ev_le_monitor_device *ev; + struct msft_monitor_advertisement_handle_data *handle_data; + u8 addr_type; + + ev = msft_skb_pull(hdev, skb, MSFT_EV_LE_MONITOR_DEVICE, sizeof(*ev)); + if (!ev) + return; + + bt_dev_dbg(hdev, + "MSFT vendor event 0x%02x: handle 0x%04x state %d addr %pMR", + MSFT_EV_LE_MONITOR_DEVICE, ev->monitor_handle, + ev->monitor_state, &ev->bdaddr); + + handle_data = msft_find_handle_data(hdev, ev->monitor_handle, false); + + switch (ev->addr_type) { + case ADDR_LE_DEV_PUBLIC: + addr_type = BDADDR_LE_PUBLIC; + break; + + case ADDR_LE_DEV_RANDOM: + addr_type = BDADDR_LE_RANDOM; + break; + + default: + bt_dev_err(hdev, + "MSFT vendor event 0x%02x: unknown addr type 0x%02x", + MSFT_EV_LE_MONITOR_DEVICE, ev->addr_type); + return; + } + + if (ev->monitor_state) + msft_device_found(hdev, &ev->bdaddr, addr_type, + handle_data->mgmt_handle); + else + msft_device_lost(hdev, &ev->bdaddr, addr_type, + handle_data->mgmt_handle); +} + void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb) { struct msft_data *msft = hdev->msft_data; - u8 event; + u8 *evt_prefix; + u8 *evt; if (!msft) return; @@ -602,13 +732,12 @@ void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb) * matches, and otherwise just return. */ if (msft->evt_prefix_len > 0) { - if (skb->len < msft->evt_prefix_len) + evt_prefix = msft_skb_pull(hdev, skb, 0, msft->evt_prefix_len); + if (!evt_prefix) return; - if (memcmp(skb->data, msft->evt_prefix, msft->evt_prefix_len)) + if (memcmp(evt_prefix, msft->evt_prefix, msft->evt_prefix_len)) return; - - skb_pull(skb, msft->evt_prefix_len); } /* Every event starts at least with an event code and the rest of @@ -617,10 +746,23 @@ void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb) if (skb->len < 1) return; - event = *skb->data; - skb_pull(skb, 1); + evt = msft_skb_pull(hdev, skb, 0, sizeof(*evt)); + if (!evt) + return; + + hci_dev_lock(hdev); + + switch (*evt) { + case MSFT_EV_LE_MONITOR_DEVICE: + msft_monitor_device_evt(hdev, skb); + break; - bt_dev_dbg(hdev, "MSFT vendor event %u", event); + default: + bt_dev_dbg(hdev, "MSFT vendor event 0x%02x", *evt); + break; + } + + hci_dev_unlock(hdev); } __u64 msft_get_features(struct hci_dev *hdev)