Message ID | 20211215203919.v8.1.Ic0a40b84dee3825302890aaea690e73165c71820@changeid (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v8,1/3] bluetooth: msft: Handle MSFT Monitor Device Event | expand |
Hi Manish, Thank you for the patch! Yet something to improve: [auto build test ERROR on bluetooth-next/master] [also build test ERROR on next-20211215] [cannot apply to bluetooth/master v5.16-rc5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Manish-Mandlik/bluetooth-msft-Handle-MSFT-Monitor-Device-Event/20211216-124056 base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master config: arc-randconfig-r043-20211216 (https://download.01.org/0day-ci/archive/20211216/202112161736.WMgjxclO-lkp@intel.com/config) compiler: arc-elf-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/db0604d7b0d0308963bdc1465b486be11d5fdcb6 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Manish-Mandlik/bluetooth-msft-Handle-MSFT-Monitor-Device-Event/20211216-124056 git checkout db0604d7b0d0308963bdc1465b486be11d5fdcb6 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash net/bluetooth/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from include/linux/wait.h:7, from include/linux/poll.h:8, from include/net/bluetooth/bluetooth.h:28, from net/bluetooth/msft.c:6: net/bluetooth/msft.c: In function 'msft_le_cancel_monitor_advertisement_cb': >> net/bluetooth/msft.c:306:42: error: 'dev' undeclared (first use in this function); did you mean 'hdev'? 306 | list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, | ^~~ include/linux/list.h:717:14: note: in definition of macro 'list_for_each_entry_safe' 717 | for (pos = list_first_entry(head, typeof(*pos), member), \ | ^~~ net/bluetooth/msft.c:306:42: note: each undeclared identifier is reported only once for each function it appears in 306 | list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, | ^~~ include/linux/list.h:717:14: note: in definition of macro 'list_for_each_entry_safe' 717 | for (pos = list_first_entry(head, typeof(*pos), member), \ | ^~~ In file included from arch/arc/include/asm/cache.h:28, from include/linux/cache.h:6, from include/linux/time.h:5, from include/linux/ktime.h:24, from include/linux/poll.h:7, from include/net/bluetooth/bluetooth.h:28, from net/bluetooth/msft.c:6: include/linux/compiler_types.h:276:27: error: expression in static assertion is not an integer 276 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert' 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/container_of.h:19:9: note: in expansion of macro 'static_assert' 19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \ | ^~~~~~~~~~~~~ include/linux/container_of.h:19:23: note: in expansion of macro '__same_type' 19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \ | ^~~~~~~~~~~ include/linux/list.h:513:9: note: in expansion of macro 'container_of' 513 | container_of(ptr, type, member) | ^~~~~~~~~~~~ include/linux/list.h:524:9: note: in expansion of macro 'list_entry' 524 | list_entry((ptr)->next, type, member) | ^~~~~~~~~~ include/linux/list.h:717:20: note: in expansion of macro 'list_first_entry' 717 | for (pos = list_first_entry(head, typeof(*pos), member), \ | ^~~~~~~~~~~~~~~~ net/bluetooth/msft.c:306:17: note: in expansion of macro 'list_for_each_entry_safe' 306 | list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, | ^~~~~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/wait.h:7, from include/linux/poll.h:8, from include/net/bluetooth/bluetooth.h:28, from net/bluetooth/msft.c:6: >> net/bluetooth/msft.c:306:47: error: 'tmp' undeclared (first use in this function); did you mean 'tm'? 306 | list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, | ^~~ include/linux/list.h:718:17: note: in definition of macro 'list_for_each_entry_safe' 718 | n = list_next_entry(pos, member); \ | ^ In file included from arch/arc/include/asm/cache.h:28, from include/linux/cache.h:6, from include/linux/time.h:5, from include/linux/ktime.h:24, from include/linux/poll.h:7, from include/net/bluetooth/bluetooth.h:28, from net/bluetooth/msft.c:6: include/linux/compiler_types.h:276:27: error: expression in static assertion is not an integer 276 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert' 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/container_of.h:19:9: note: in expansion of macro 'static_assert' 19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \ | ^~~~~~~~~~~~~ include/linux/container_of.h:19:23: note: in expansion of macro '__same_type' 19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \ | ^~~~~~~~~~~ include/linux/list.h:513:9: note: in expansion of macro 'container_of' 513 | container_of(ptr, type, member) | ^~~~~~~~~~~~ include/linux/list.h:557:9: note: in expansion of macro 'list_entry' 557 | list_entry((pos)->member.next, typeof(*(pos)), member) | ^~~~~~~~~~ include/linux/list.h:718:21: note: in expansion of macro 'list_next_entry' 718 | n = list_next_entry(pos, member); \ | ^~~~~~~~~~~~~~~ net/bluetooth/msft.c:306:17: note: in expansion of macro 'list_for_each_entry_safe' 306 | list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, | ^~~~~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/wait.h:7, from include/linux/poll.h:8, from include/net/bluetooth/bluetooth.h:28, from net/bluetooth/msft.c:6: include/linux/list.h:717:64: warning: left-hand operand of comma expression has no effect [-Wunused-value] 717 | for (pos = list_first_entry(head, typeof(*pos), member), \ | ^ net/bluetooth/msft.c:306:17: note: in expansion of macro 'list_for_each_entry_safe' 306 | list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, | ^~~~~~~~~~~~~~~~~~~~~~~~ In file included from arch/arc/include/asm/cache.h:28, from include/linux/cache.h:6, from include/linux/time.h:5, from include/linux/ktime.h:24, from include/linux/poll.h:7, from include/net/bluetooth/bluetooth.h:28, from net/bluetooth/msft.c:6: include/linux/compiler_types.h:276:27: error: expression in static assertion is not an integer 276 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert' 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/container_of.h:19:9: note: in expansion of macro 'static_assert' 19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \ | ^~~~~~~~~~~~~ include/linux/container_of.h:19:23: note: in expansion of macro '__same_type' 19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \ | ^~~~~~~~~~~ include/linux/list.h:513:9: note: in expansion of macro 'container_of' 513 | container_of(ptr, type, member) | ^~~~~~~~~~~~ include/linux/list.h:557:9: note: in expansion of macro 'list_entry' 557 | list_entry((pos)->member.next, typeof(*(pos)), member) | ^~~~~~~~~~ include/linux/list.h:720:27: note: in expansion of macro 'list_next_entry' 720 | pos = n, n = list_next_entry(n, member)) | ^~~~~~~~~~~~~~~ net/bluetooth/msft.c:306:17: note: in expansion of macro 'list_for_each_entry_safe' 306 | list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, | ^~~~~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/wait.h:7, from include/linux/poll.h:8, from include/net/bluetooth/bluetooth.h:28, from net/bluetooth/msft.c:6: include/linux/list.h:720:21: warning: left-hand operand of comma expression has no effect [-Wunused-value] 720 | pos = n, n = list_next_entry(n, member)) | ^ net/bluetooth/msft.c:306:17: note: in expansion of macro 'list_for_each_entry_safe' 306 | list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, | ^~~~~~~~~~~~~~~~~~~~~~~~ vim +306 net/bluetooth/msft.c 265 266 static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev, 267 u8 status, u16 opcode, 268 struct sk_buff *skb) 269 { 270 struct msft_cp_le_cancel_monitor_advertisement *cp; 271 struct msft_rp_le_cancel_monitor_advertisement *rp; 272 struct adv_monitor *monitor; 273 struct msft_monitor_advertisement_handle_data *handle_data; 274 struct msft_data *msft = hdev->msft_data; 275 int err; 276 bool pending; 277 278 if (status) 279 goto done; 280 281 rp = (struct msft_rp_le_cancel_monitor_advertisement *)skb->data; 282 if (skb->len < sizeof(*rp)) { 283 status = HCI_ERROR_UNSPECIFIED; 284 goto done; 285 } 286 287 hci_dev_lock(hdev); 288 289 cp = hci_sent_cmd_data(hdev, hdev->msft_opcode); 290 handle_data = msft_find_handle_data(hdev, cp->handle, false); 291 292 if (handle_data) { 293 monitor = idr_find(&hdev->adv_monitors_idr, 294 handle_data->mgmt_handle); 295 296 if (monitor && monitor->state == ADV_MONITOR_STATE_OFFLOADED) 297 monitor->state = ADV_MONITOR_STATE_REGISTERED; 298 299 /* Do not free the monitor if it is being removed due to 300 * suspend. It will be re-monitored on resume. 301 */ 302 if (monitor && !msft->suspending) 303 hci_free_adv_monitor(hdev, monitor); 304 305 /* Clear any monitored devices by this Adv Monitor */ > 306 list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, 307 list) { 308 if (dev->handle == handle_data->mgmt_handle) { 309 list_del(&dev->list); 310 kfree(dev); 311 } 312 } 313 314 list_del(&handle_data->list); 315 kfree(handle_data); 316 } 317 318 /* If remove all monitors is required, we need to continue the process 319 * here because the earlier it was paused when waiting for the 320 * response from controller. 321 */ 322 if (msft->pending_remove_handle == 0) { 323 pending = hci_remove_all_adv_monitor(hdev, &err); 324 if (pending) { 325 hci_dev_unlock(hdev); 326 return; 327 } 328 329 if (err) 330 status = HCI_ERROR_UNSPECIFIED; 331 } 332 333 hci_dev_unlock(hdev); 334 335 done: 336 if (!msft->suspending) 337 hci_remove_adv_monitor_complete(hdev, status); 338 } 339 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 4d69dcfebd63..c2a8b1163c30 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; @@ -590,6 +599,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 38063bf1fdc5..c67e6d40c97d 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..b4544b1acf8f 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; @@ -294,6 +302,15 @@ 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 */ + list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, + list) { + if (dev->handle == handle_data->mgmt_handle) { + list_del(&dev->list); + kfree(dev); + } + } + list_del(&handle_data->list); kfree(handle_data); } @@ -538,6 +555,7 @@ void msft_do_close(struct hci_dev *hdev) struct msft_data *msft = hdev->msft_data; struct msft_monitor_advertisement_handle_data *handle_data, *tmp; struct adv_monitor *monitor; + struct monitored_device *dev, *tmp_dev; if (!msft) return; @@ -557,6 +575,16 @@ 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 */ + list_for_each_entry_safe(dev, tmp_dev, &hdev->monitored_devices, list) { + list_del(&dev->list); + kfree(dev); + } + + hci_dev_unlock(hdev); } void msft_register(struct hci_dev *hdev) @@ -590,10 +618,103 @@ 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) +{ + struct monitored_device *dev, *tmp; + + list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, list) { + if (dev->handle == mgmt_handle) { + list_del(&dev->list); + kfree(dev); + + break; + } + } +} + +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 +723,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 +737,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); + hci_dev_lock(hdev); + + evt = msft_skb_pull(hdev, skb, 0, sizeof(*evt)); + if (!evt) + return; + + 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)