diff mbox series

[1/2] Bluetooth: hci_sync: Refactor add Adv Monitor

Message ID 20220524131406.1.If745ed1d05d98c002fc84ba60cef99eb786b7caa@changeid
State New
Headers show
Series [1/2] Bluetooth: hci_sync: Refactor add Adv Monitor | expand

Commit Message

Manish Mandlik May 24, 2022, 8:14 p.m. UTC
Make use of hci_cmd_sync_queue for adding an advertisement monitor.

Signed-off-by: Manish Mandlik <mmandlik@google.com>
Reviewed-by: Miao-chen Chou <mcchou@google.com>
---

 include/net/bluetooth/hci_core.h |   5 +-
 net/bluetooth/hci_core.c         |  47 ++++-----
 net/bluetooth/mgmt.c             | 121 +++++++----------------
 net/bluetooth/msft.c             | 161 ++++++++-----------------------
 4 files changed, 98 insertions(+), 236 deletions(-)

Comments

Luiz Augusto von Dentz May 26, 2022, 12:45 a.m. UTC | #1
Hi Manish,

On Tue, May 24, 2022 at 1:14 PM Manish Mandlik <mmandlik@google.com> wrote:
>
> Make use of hci_cmd_sync_queue for adding an advertisement monitor.

Im a little lost here, it seems you end up not really using
hci_cmd_sync_queue are you assuming these functions are already run
from a safe context?

> Signed-off-by: Manish Mandlik <mmandlik@google.com>
> Reviewed-by: Miao-chen Chou <mcchou@google.com>
> ---
>
>  include/net/bluetooth/hci_core.h |   5 +-
>  net/bluetooth/hci_core.c         |  47 ++++-----
>  net/bluetooth/mgmt.c             | 121 +++++++----------------
>  net/bluetooth/msft.c             | 161 ++++++++-----------------------
>  4 files changed, 98 insertions(+), 236 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 5a52a2018b56..59953a7a6328 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1410,10 +1410,8 @@ bool hci_adv_instance_is_scannable(struct hci_dev *hdev, u8 instance);
>
>  void hci_adv_monitors_clear(struct hci_dev *hdev);
>  void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
> -int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
>  int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> -bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
> -                       int *err);
> +int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
>  bool hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle, int *err);
>  bool hci_remove_all_adv_monitor(struct hci_dev *hdev, int *err);
>  bool hci_is_adv_monitoring(struct hci_dev *hdev);
> @@ -1875,7 +1873,6 @@ void mgmt_advertising_removed(struct sock *sk, struct hci_dev *hdev,
>                               u8 instance);
>  void mgmt_adv_monitor_removed(struct hci_dev *hdev, u16 handle);
>  int mgmt_phy_configuration_changed(struct hci_dev *hdev, struct sock *skip);
> -int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
>  int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
>  void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
>                                   bdaddr_t *bdaddr, u8 addr_type);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5abb2ca5b129..bbbbe3203130 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1873,11 +1873,6 @@ void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
>         kfree(monitor);
>  }
>
> -int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
> -{
> -       return mgmt_add_adv_patterns_monitor_complete(hdev, status);
> -}
> -
>  int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
>  {
>         return mgmt_remove_adv_monitor_complete(hdev, status);
> @@ -1885,49 +1880,49 @@ int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
>
>  /* Assigns handle to a monitor, and if offloading is supported and power is on,
>   * also attempts to forward the request to the controller.
> - * Returns true if request is forwarded (result is pending), false otherwise.
> - * This function requires the caller holds hdev->lock.
>   */
> -bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
> -                        int *err)
> +int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
>  {
>         int min, max, handle;
> +       int status = 0;
>
> -       *err = 0;
> +       if (!monitor)
> +               return -EINVAL;
>
> -       if (!monitor) {
> -               *err = -EINVAL;
> -               return false;
> -       }
> +       hci_dev_lock(hdev);
>
>         min = HCI_MIN_ADV_MONITOR_HANDLE;
>         max = HCI_MIN_ADV_MONITOR_HANDLE + HCI_MAX_ADV_MONITOR_NUM_HANDLES;
>         handle = idr_alloc(&hdev->adv_monitors_idr, monitor, min, max,
>                            GFP_KERNEL);
> -       if (handle < 0) {
> -               *err = handle;
> -               return false;
> -       }
> +
> +       hci_dev_unlock(hdev);
> +
> +       if (handle < 0)
> +               return handle;
>
>         monitor->handle = handle;
>
>         if (!hdev_is_powered(hdev))
> -               return false;
> +               return status;
>
>         switch (hci_get_adv_monitor_offload_ext(hdev)) {
>         case HCI_ADV_MONITOR_EXT_NONE:
> -               hci_update_passive_scan(hdev);
> -               bt_dev_dbg(hdev, "%s add monitor status %d", hdev->name, *err);
> +               bt_dev_dbg(hdev, "%s add monitor %d status %d", hdev->name,
> +                          monitor->handle, status);
>                 /* Message was not forwarded to controller - not an error */
> -               return false;
> +               break;
> +
>         case HCI_ADV_MONITOR_EXT_MSFT:
> -               *err = msft_add_monitor_pattern(hdev, monitor);
> -               bt_dev_dbg(hdev, "%s add monitor msft status %d", hdev->name,
> -                          *err);
> +               hci_req_sync_lock(hdev);
> +               status = msft_add_monitor_pattern(hdev, monitor);
> +               hci_req_sync_unlock(hdev);
> +               bt_dev_dbg(hdev, "%s add monitor %d msft status %d", hdev->name,
> +                          monitor->handle, status);
>                 break;
>         }
>
> -       return (*err == 0);
> +       return status;
>  }
>
>  /* Attempts to tell the controller and free the monitor. If somehow the
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 74937a834648..d04f90698a87 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4653,75 +4653,21 @@ static int read_adv_mon_features(struct sock *sk, struct hci_dev *hdev,
>         return err;
>  }
>
> -int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
> -{
> -       struct mgmt_rp_add_adv_patterns_monitor rp;
> -       struct mgmt_pending_cmd *cmd;
> -       struct adv_monitor *monitor;
> -       int err = 0;
> -
> -       hci_dev_lock(hdev);
> -
> -       cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev);
> -       if (!cmd) {
> -               cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev);
> -               if (!cmd)
> -                       goto done;
> -       }
> -
> -       monitor = cmd->user_data;
> -       rp.monitor_handle = cpu_to_le16(monitor->handle);
> -
> -       if (!status) {
> -               mgmt_adv_monitor_added(cmd->sk, hdev, monitor->handle);
> -               hdev->adv_monitors_cnt++;
> -               if (monitor->state == ADV_MONITOR_STATE_NOT_REGISTERED)
> -                       monitor->state = ADV_MONITOR_STATE_REGISTERED;
> -               hci_update_passive_scan(hdev);
> -       }
> -
> -       err = mgmt_cmd_complete(cmd->sk, cmd->index, cmd->opcode,
> -                               mgmt_status(status), &rp, sizeof(rp));
> -       mgmt_pending_remove(cmd);
> -
> -done:
> -       hci_dev_unlock(hdev);
> -       bt_dev_dbg(hdev, "add monitor %d complete, status %u",
> -                  rp.monitor_handle, status);
> -
> -       return err;
> -}
> -
>  static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
> -                                     struct adv_monitor *m, u8 status,
> -                                     void *data, u16 len, u16 op)
> +                                     struct adv_monitor *m, void *data,
> +                                     u16 len, u16 op)
>  {
>         struct mgmt_rp_add_adv_patterns_monitor rp;
> -       struct mgmt_pending_cmd *cmd;
> +       u8 status = MGMT_STATUS_SUCCESS;
>         int err;
> -       bool pending;
> -
> -       hci_dev_lock(hdev);
> -
> -       if (status)
> -               goto unlock;
>
>         if (pending_find(MGMT_OP_SET_LE, hdev) ||
> -           pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
> -           pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev) ||
>             pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
>                 status = MGMT_STATUS_BUSY;
> -               goto unlock;
> -       }
> -
> -       cmd = mgmt_pending_add(sk, op, hdev, data, len);
> -       if (!cmd) {
> -               status = MGMT_STATUS_NO_RESOURCES;
> -               goto unlock;
> +               goto done;
>         }
>
> -       cmd->user_data = m;
> -       pending = hci_add_adv_monitor(hdev, m, &err);
> +       err = hci_add_adv_monitor(hdev, m);
>         if (err) {
>                 if (err == -ENOSPC || err == -ENOMEM)
>                         status = MGMT_STATUS_NO_RESOURCES;
> @@ -4730,30 +4676,29 @@ static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>                 else
>                         status = MGMT_STATUS_FAILED;
>
> -               mgmt_pending_remove(cmd);
> -               goto unlock;
> +               goto done;
>         }
>
> -       if (!pending) {
> -               mgmt_pending_remove(cmd);
> -               rp.monitor_handle = cpu_to_le16(m->handle);
> -               mgmt_adv_monitor_added(sk, hdev, m->handle);
> -               m->state = ADV_MONITOR_STATE_REGISTERED;
> -               hdev->adv_monitors_cnt++;
> +       hci_dev_lock(hdev);
>
> -               hci_dev_unlock(hdev);
> -               return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS,
> -                                        &rp, sizeof(rp));
> -       }
> +       rp.monitor_handle = cpu_to_le16(m->handle);
> +       mgmt_adv_monitor_added(sk, hdev, m->handle);
> +       if (m->state == ADV_MONITOR_STATE_NOT_REGISTERED)
> +               m->state = ADV_MONITOR_STATE_REGISTERED;
> +       hdev->adv_monitors_cnt++;
> +       hci_update_passive_scan(hdev);
>
>         hci_dev_unlock(hdev);
>
> -       return 0;
> +done:
> +       bt_dev_dbg(hdev, "add monitor %d complete, status %u", m->handle,
> +                  status);
>
> -unlock:
> -       hci_free_adv_monitor(hdev, m);
> -       hci_dev_unlock(hdev);
> -       return mgmt_cmd_status(sk, hdev->id, op, status);
> +       if (status)
> +               return mgmt_cmd_status(sk, hdev->id, op, status);
> +
> +       return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS, &rp,
> +                                sizeof(rp));
>  }
>
>  static void parse_adv_monitor_rssi(struct adv_monitor *m,
> @@ -4817,7 +4762,7 @@ static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>  {
>         struct mgmt_cp_add_adv_patterns_monitor *cp = data;
>         struct adv_monitor *m = NULL;
> -       u8 status = MGMT_STATUS_SUCCESS;
> +       int status = MGMT_STATUS_SUCCESS;
>         size_t expected_size = sizeof(*cp);
>
>         BT_DBG("request for %s", hdev->name);
> @@ -4843,10 +4788,14 @@ static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>
>         parse_adv_monitor_rssi(m, NULL);
>         status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
> +       if (status)
> +               goto done;
> +
> +       status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
> +                                           MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
>
>  done:
> -       return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
> -                                         MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
> +       return status;
>  }
>
>  static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
> @@ -4854,7 +4803,7 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
>  {
>         struct mgmt_cp_add_adv_patterns_monitor_rssi *cp = data;
>         struct adv_monitor *m = NULL;
> -       u8 status = MGMT_STATUS_SUCCESS;
> +       int status = MGMT_STATUS_SUCCESS;
>         size_t expected_size = sizeof(*cp);
>
>         BT_DBG("request for %s", hdev->name);
> @@ -4880,10 +4829,14 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
>
>         parse_adv_monitor_rssi(m, &cp->rssi);
>         status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
> +       if (status)
> +               goto done;
>
> -done:
> -       return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
> +       status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
>                                          MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI);
> +
> +done:
> +       return status;
>  }
>
>  int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
> @@ -4933,9 +4886,7 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
>         hci_dev_lock(hdev);
>
>         if (pending_find(MGMT_OP_SET_LE, hdev) ||
> -           pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev) ||
> -           pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
> -           pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev)) {
> +           pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
>                 status = MGMT_STATUS_BUSY;
>                 goto unlock;
>         }
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> index f43994523b1f..9abea16c4305 100644
> --- a/net/bluetooth/msft.c
> +++ b/net/bluetooth/msft.c
> @@ -106,8 +106,6 @@ struct msft_data {
>         __u8 filter_enabled;
>  };
>
> -static int __msft_add_monitor_pattern(struct hci_dev *hdev,
> -                                     struct adv_monitor *monitor);
>  static int __msft_remove_monitor(struct hci_dev *hdev,
>                                  struct adv_monitor *monitor, u16 handle);
>
> @@ -164,34 +162,6 @@ static bool read_supported_features(struct hci_dev *hdev,
>         return false;
>  }
>
> -static void reregister_monitor(struct hci_dev *hdev, int handle)
> -{
> -       struct adv_monitor *monitor;
> -       struct msft_data *msft = hdev->msft_data;
> -       int err;
> -
> -       while (1) {
> -               monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
> -               if (!monitor) {
> -                       /* All monitors have been resumed */
> -                       msft->resuming = false;
> -                       hci_update_passive_scan(hdev);
> -                       return;
> -               }
> -
> -               msft->pending_add_handle = (u16)handle;
> -               err = __msft_add_monitor_pattern(hdev, monitor);
> -
> -               /* If success, we return and wait for monitor added callback */
> -               if (!err)
> -                       return;
> -
> -               /* Otherwise remove the monitor and keep registering */
> -               hci_free_adv_monitor(hdev, monitor);
> -               handle++;
> -       }
> -}
> -
>  /* is_mgmt = true matches the handle exposed to userspace via mgmt.
>   * is_mgmt = false matches the handle used by the msft controller.
>   * This function requires the caller holds hdev->lock
> @@ -243,14 +213,14 @@ static int msft_monitor_device_del(struct hci_dev *hdev, __u16 mgmt_handle,
>         return count;
>  }
>
> -static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
> -                                            u8 status, u16 opcode,
> -                                            struct sk_buff *skb)
> +static int msft_le_monitor_advertisement_cb(struct hci_dev *hdev, u16 opcode,
> +                                           struct sk_buff *skb)
>  {
>         struct msft_rp_le_monitor_advertisement *rp;
>         struct adv_monitor *monitor;
>         struct msft_monitor_advertisement_handle_data *handle_data;
>         struct msft_data *msft = hdev->msft_data;
> +       int status = 0;
>
>         hci_dev_lock(hdev);
>
> @@ -262,15 +232,16 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
>                 goto unlock;
>         }
>
> -       if (status)
> -               goto unlock;
> -
>         rp = (struct msft_rp_le_monitor_advertisement *)skb->data;
>         if (skb->len < sizeof(*rp)) {
>                 status = HCI_ERROR_UNSPECIFIED;
>                 goto unlock;
>         }
>
> +       status = rp->status;
> +       if (status)
> +               goto unlock;
> +
>         handle_data = kmalloc(sizeof(*handle_data), GFP_KERNEL);
>         if (!handle_data) {
>                 status = HCI_ERROR_UNSPECIFIED;
> @@ -290,8 +261,7 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
>
>         hci_dev_unlock(hdev);
>
> -       if (!msft->resuming)
> -               hci_add_adv_patterns_monitor_complete(hdev, status);
> +       return status;
>  }
>
>  static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
> @@ -463,7 +433,7 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
>         ptrdiff_t offset = 0;
>         u8 pattern_count = 0;
>         struct sk_buff *skb;
> -       u8 status;
> +       struct msft_data *msft = hdev->msft_data;
>
>         if (!msft_monitor_pattern_valid(monitor))
>                 return -EINVAL;
> @@ -505,20 +475,40 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
>         if (IS_ERR(skb))
>                 return PTR_ERR(skb);
>
> -       status = skb->data[0];
> -       skb_pull(skb, 1);
> +       msft->pending_add_handle = monitor->handle;
>
> -       msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb);
> +       return msft_le_monitor_advertisement_cb(hdev, hdev->msft_opcode, skb);
> +}
>
> -       return status;
> +static void reregister_monitor(struct hci_dev *hdev)
> +{
> +       struct adv_monitor *monitor;
> +       struct msft_data *msft = hdev->msft_data;
> +       int handle = 0;
> +
> +       if (!msft)
> +               return;
> +
> +       msft->resuming = true;
> +
> +       while (1) {
> +               monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
> +               if (!monitor)
> +                       break;
> +
> +               msft_add_monitor_sync(hdev, monitor);
> +
> +               handle++;
> +       }
> +
> +       /* All monitors have been reregistered */
> +       msft->resuming = false;
>  }
>
>  /* This function requires the caller holds hci_req_sync_lock */
>  int msft_resume_sync(struct hci_dev *hdev)
>  {
>         struct msft_data *msft = hdev->msft_data;
> -       struct adv_monitor *monitor;
> -       int handle = 0;
>
>         if (!msft || !msft_monitor_supported(hdev))
>                 return 0;
> @@ -533,24 +523,12 @@ int msft_resume_sync(struct hci_dev *hdev)
>
>         hci_dev_unlock(hdev);
>
> -       msft->resuming = true;
> -
> -       while (1) {
> -               monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
> -               if (!monitor)
> -                       break;
> -
> -               msft_add_monitor_sync(hdev, monitor);
> -
> -               handle++;
> -       }
> -
> -       /* All monitors have been resumed */
> -       msft->resuming = false;
> +       reregister_monitor(hdev);
>
>         return 0;
>  }
>
> +/* This function requires the caller holds hci_req_sync_lock */
>  void msft_do_open(struct hci_dev *hdev)
>  {
>         struct msft_data *msft = hdev->msft_data;
> @@ -583,7 +561,7 @@ void msft_do_open(struct hci_dev *hdev)
>                 /* Monitors get removed on power off, so we need to explicitly
>                  * tell the controller to re-monitor.
>                  */
> -               reregister_monitor(hdev, 0);
> +               reregister_monitor(hdev);
>         }
>  }
>
> @@ -829,66 +807,7 @@ static void msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
>         hci_dev_unlock(hdev);
>  }
>
> -/* This function requires the caller holds hdev->lock */
> -static int __msft_add_monitor_pattern(struct hci_dev *hdev,
> -                                     struct adv_monitor *monitor)
> -{
> -       struct msft_cp_le_monitor_advertisement *cp;
> -       struct msft_le_monitor_advertisement_pattern_data *pattern_data;
> -       struct msft_le_monitor_advertisement_pattern *pattern;
> -       struct adv_pattern *entry;
> -       struct hci_request req;
> -       struct msft_data *msft = hdev->msft_data;
> -       size_t total_size = sizeof(*cp) + sizeof(*pattern_data);
> -       ptrdiff_t offset = 0;
> -       u8 pattern_count = 0;
> -       int err = 0;
> -
> -       if (!msft_monitor_pattern_valid(monitor))
> -               return -EINVAL;
> -
> -       list_for_each_entry(entry, &monitor->patterns, list) {
> -               pattern_count++;
> -               total_size += sizeof(*pattern) + entry->length;
> -       }
> -
> -       cp = kmalloc(total_size, GFP_KERNEL);
> -       if (!cp)
> -               return -ENOMEM;
> -
> -       cp->sub_opcode = MSFT_OP_LE_MONITOR_ADVERTISEMENT;
> -       cp->rssi_high = monitor->rssi.high_threshold;
> -       cp->rssi_low = monitor->rssi.low_threshold;
> -       cp->rssi_low_interval = (u8)monitor->rssi.low_threshold_timeout;
> -       cp->rssi_sampling_period = monitor->rssi.sampling_period;
> -
> -       cp->cond_type = MSFT_MONITOR_ADVERTISEMENT_TYPE_PATTERN;
> -
> -       pattern_data = (void *)cp->data;
> -       pattern_data->count = pattern_count;
> -
> -       list_for_each_entry(entry, &monitor->patterns, list) {
> -               pattern = (void *)(pattern_data->data + offset);
> -               /* the length also includes data_type and offset */
> -               pattern->length = entry->length + 2;
> -               pattern->data_type = entry->ad_type;
> -               pattern->start_byte = entry->offset;
> -               memcpy(pattern->pattern, entry->value, entry->length);
> -               offset += sizeof(*pattern) + entry->length;
> -       }
> -
> -       hci_req_init(&req, hdev);
> -       hci_req_add(&req, hdev->msft_opcode, total_size, cp);
> -       err = hci_req_run_skb(&req, msft_le_monitor_advertisement_cb);
> -       kfree(cp);
> -
> -       if (!err)
> -               msft->pending_add_handle = monitor->handle;
> -
> -       return err;
> -}
> -
> -/* This function requires the caller holds hdev->lock */
> +/* This function requires the caller holds hci_req_sync_lock */
>  int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
>  {
>         struct msft_data *msft = hdev->msft_data;
> @@ -899,7 +818,7 @@ int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
>         if (msft->resuming || msft->suspending)
>                 return -EBUSY;
>
> -       return __msft_add_monitor_pattern(hdev, monitor);
> +       return msft_add_monitor_sync(hdev, monitor);
>  }
>
>  /* This function requires the caller holds hdev->lock */
> --
> 2.36.1.124.g0e6072fb45-goog
>
Luiz Augusto von Dentz June 7, 2022, 9:11 p.m. UTC | #2
Hi Manish,

On Tue, Jun 7, 2022 at 9:57 AM Manish Mandlik <mmandlik@google.com> wrote:
>
> Hi Luiz,
>
> On Thu, May 26, 2022 at 7:01 PM Manish Mandlik <mmandlik@google.com> wrote:
>>
>> Hi Luiz,
>>
>>
>> On Wed, May 25, 2022 at 5:46 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>>
>>> Hi Manish,
>>>
>>> On Tue, May 24, 2022 at 1:14 PM Manish Mandlik <mmandlik@google.com> wrote:
>>> >
>>> > Make use of hci_cmd_sync_queue for adding an advertisement monitor.
>>>
>>> Im a little lost here, it seems you end up not really using
>>> hci_cmd_sync_queue are you assuming these functions are already run
>>> from a safe context?
>>
>> Not sure if I understand the question. But I am using msft_add_monitor_sync() to send a monitor to the controller which uses hci_cmd_sync_queue. It requires the caller to hold hci_req_sync_lock, which I have used at the appropriate places. Let me know if that looks correct.

It sounds like you are doing the hci_req_sync_lock from any thread
instead of using hci_cmd_sync_queue and then from its callback call
msft_add_monitor_sync, that way we guarantee only one task is
scheduling HCI traffic.

>>
>>>
>>> > Signed-off-by: Manish Mandlik <mmandlik@google.com>
>>> > Reviewed-by: Miao-chen Chou <mcchou@google.com>
>>> > ---
>>> >
>>> >  include/net/bluetooth/hci_core.h |   5 +-
>>> >  net/bluetooth/hci_core.c         |  47 ++++-----
>>> >  net/bluetooth/mgmt.c             | 121 +++++++----------------
>>> >  net/bluetooth/msft.c             | 161 ++++++++-----------------------
>>> >  4 files changed, 98 insertions(+), 236 deletions(-)
>>> >
>>> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> > index 5a52a2018b56..59953a7a6328 100644
>>> > --- a/include/net/bluetooth/hci_core.h
>>> > +++ b/include/net/bluetooth/hci_core.h
>>> > @@ -1410,10 +1410,8 @@ bool hci_adv_instance_is_scannable(struct hci_dev *hdev, u8 instance);
>>> >
>>> >  void hci_adv_monitors_clear(struct hci_dev *hdev);
>>> >  void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
>>> > -int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
>>> >  int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
>>> > -bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
>>> > -                       int *err);
>>> > +int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
>>> >  bool hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle, int *err);
>>> >  bool hci_remove_all_adv_monitor(struct hci_dev *hdev, int *err);
>>> >  bool hci_is_adv_monitoring(struct hci_dev *hdev);
>>> > @@ -1875,7 +1873,6 @@ void mgmt_advertising_removed(struct sock *sk, struct hci_dev *hdev,
>>> >                               u8 instance);
>>> >  void mgmt_adv_monitor_removed(struct hci_dev *hdev, u16 handle);
>>> >  int mgmt_phy_configuration_changed(struct hci_dev *hdev, struct sock *skip);
>>> > -int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
>>> >  int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
>>> >  void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
>>> >                                   bdaddr_t *bdaddr, u8 addr_type);
>>> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>> > index 5abb2ca5b129..bbbbe3203130 100644
>>> > --- a/net/bluetooth/hci_core.c
>>> > +++ b/net/bluetooth/hci_core.c
>>> > @@ -1873,11 +1873,6 @@ void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
>>> >         kfree(monitor);
>>> >  }
>>> >
>>> > -int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
>>> > -{
>>> > -       return mgmt_add_adv_patterns_monitor_complete(hdev, status);
>>> > -}
>>> > -
>>> >  int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
>>> >  {
>>> >         return mgmt_remove_adv_monitor_complete(hdev, status);
>>> > @@ -1885,49 +1880,49 @@ int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
>>> >
>>> >  /* Assigns handle to a monitor, and if offloading is supported and power is on,
>>> >   * also attempts to forward the request to the controller.
>>> > - * Returns true if request is forwarded (result is pending), false otherwise.
>>> > - * This function requires the caller holds hdev->lock.
>>> >   */
>>> > -bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
>>> > -                        int *err)
>>> > +int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
>>> >  {
>>> >         int min, max, handle;
>>> > +       int status = 0;
>>> >
>>> > -       *err = 0;
>>> > +       if (!monitor)
>>> > +               return -EINVAL;
>>> >
>>> > -       if (!monitor) {
>>> > -               *err = -EINVAL;
>>> > -               return false;
>>> > -       }
>>> > +       hci_dev_lock(hdev);
>>> >
>>> >         min = HCI_MIN_ADV_MONITOR_HANDLE;
>>> >         max = HCI_MIN_ADV_MONITOR_HANDLE + HCI_MAX_ADV_MONITOR_NUM_HANDLES;
>>> >         handle = idr_alloc(&hdev->adv_monitors_idr, monitor, min, max,
>>> >                            GFP_KERNEL);
>>> > -       if (handle < 0) {
>>> > -               *err = handle;
>>> > -               return false;
>>> > -       }
>>> > +
>>> > +       hci_dev_unlock(hdev);
>>> > +
>>> > +       if (handle < 0)
>>> > +               return handle;
>>> >
>>> >         monitor->handle = handle;
>>> >
>>> >         if (!hdev_is_powered(hdev))
>>> > -               return false;
>>> > +               return status;
>>> >
>>> >         switch (hci_get_adv_monitor_offload_ext(hdev)) {
>>> >         case HCI_ADV_MONITOR_EXT_NONE:
>>> > -               hci_update_passive_scan(hdev);
>>> > -               bt_dev_dbg(hdev, "%s add monitor status %d", hdev->name, *err);
>>> > +               bt_dev_dbg(hdev, "%s add monitor %d status %d", hdev->name,
>>> > +                          monitor->handle, status);
>>> >                 /* Message was not forwarded to controller - not an error */
>>> > -               return false;
>>> > +               break;
>>> > +
>>> >         case HCI_ADV_MONITOR_EXT_MSFT:
>>> > -               *err = msft_add_monitor_pattern(hdev, monitor);
>>> > -               bt_dev_dbg(hdev, "%s add monitor msft status %d", hdev->name,
>>> > -                          *err);
>>> > +               hci_req_sync_lock(hdev);
>>> > +               status = msft_add_monitor_pattern(hdev, monitor);
>>> > +               hci_req_sync_unlock(hdev);
>>> > +               bt_dev_dbg(hdev, "%s add monitor %d msft status %d", hdev->name,
>>> > +                          monitor->handle, status);
>>> >                 break;
>>> >         }
>>> >
>>> > -       return (*err == 0);
>>> > +       return status;
>>> >  }
>>> >
>>> >  /* Attempts to tell the controller and free the monitor. If somehow the
>>> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>>> > index 74937a834648..d04f90698a87 100644
>>> > --- a/net/bluetooth/mgmt.c
>>> > +++ b/net/bluetooth/mgmt.c
>>> > @@ -4653,75 +4653,21 @@ static int read_adv_mon_features(struct sock *sk, struct hci_dev *hdev,
>>> >         return err;
>>> >  }
>>> >
>>> > -int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
>>> > -{
>>> > -       struct mgmt_rp_add_adv_patterns_monitor rp;
>>> > -       struct mgmt_pending_cmd *cmd;
>>> > -       struct adv_monitor *monitor;
>>> > -       int err = 0;
>>> > -
>>> > -       hci_dev_lock(hdev);
>>> > -
>>> > -       cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev);
>>> > -       if (!cmd) {
>>> > -               cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev);
>>> > -               if (!cmd)
>>> > -                       goto done;
>>> > -       }
>>> > -
>>> > -       monitor = cmd->user_data;
>>> > -       rp.monitor_handle = cpu_to_le16(monitor->handle);
>>> > -
>>> > -       if (!status) {
>>> > -               mgmt_adv_monitor_added(cmd->sk, hdev, monitor->handle);
>>> > -               hdev->adv_monitors_cnt++;
>>> > -               if (monitor->state == ADV_MONITOR_STATE_NOT_REGISTERED)
>>> > -                       monitor->state = ADV_MONITOR_STATE_REGISTERED;
>>> > -               hci_update_passive_scan(hdev);
>>> > -       }
>>> > -
>>> > -       err = mgmt_cmd_complete(cmd->sk, cmd->index, cmd->opcode,
>>> > -                               mgmt_status(status), &rp, sizeof(rp));
>>> > -       mgmt_pending_remove(cmd);
>>> > -
>>> > -done:
>>> > -       hci_dev_unlock(hdev);
>>> > -       bt_dev_dbg(hdev, "add monitor %d complete, status %u",
>>> > -                  rp.monitor_handle, status);
>>> > -
>>> > -       return err;
>>> > -}
>>> > -
>>> >  static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>>> > -                                     struct adv_monitor *m, u8 status,
>>> > -                                     void *data, u16 len, u16 op)
>>> > +                                     struct adv_monitor *m, void *data,
>>> > +                                     u16 len, u16 op)
>>> >  {
>>> >         struct mgmt_rp_add_adv_patterns_monitor rp;
>>> > -       struct mgmt_pending_cmd *cmd;
>>> > +       u8 status = MGMT_STATUS_SUCCESS;
>>> >         int err;
>>> > -       bool pending;
>>> > -
>>> > -       hci_dev_lock(hdev);
>>> > -
>>> > -       if (status)
>>> > -               goto unlock;
>>> >
>>> >         if (pending_find(MGMT_OP_SET_LE, hdev) ||
>>> > -           pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
>>> > -           pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev) ||
>>> >             pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
>>> >                 status = MGMT_STATUS_BUSY;
>>> > -               goto unlock;
>>> > -       }
>>> > -
>>> > -       cmd = mgmt_pending_add(sk, op, hdev, data, len);
>>> > -       if (!cmd) {
>>> > -               status = MGMT_STATUS_NO_RESOURCES;
>>> > -               goto unlock;
>>> > +               goto done;
>>> >         }
>>> >
>>> > -       cmd->user_data = m;
>>> > -       pending = hci_add_adv_monitor(hdev, m, &err);
>>> > +       err = hci_add_adv_monitor(hdev, m);
>>> >         if (err) {
>>> >                 if (err == -ENOSPC || err == -ENOMEM)
>>> >                         status = MGMT_STATUS_NO_RESOURCES;
>>> > @@ -4730,30 +4676,29 @@ static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>>> >                 else
>>> >                         status = MGMT_STATUS_FAILED;
>>> >
>>> > -               mgmt_pending_remove(cmd);
>>> > -               goto unlock;
>>> > +               goto done;
>>> >         }
>>> >
>>> > -       if (!pending) {
>>> > -               mgmt_pending_remove(cmd);
>>> > -               rp.monitor_handle = cpu_to_le16(m->handle);
>>> > -               mgmt_adv_monitor_added(sk, hdev, m->handle);
>>> > -               m->state = ADV_MONITOR_STATE_REGISTERED;
>>> > -               hdev->adv_monitors_cnt++;
>>> > +       hci_dev_lock(hdev);
>>> >
>>> > -               hci_dev_unlock(hdev);
>>> > -               return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS,
>>> > -                                        &rp, sizeof(rp));
>>> > -       }
>>> > +       rp.monitor_handle = cpu_to_le16(m->handle);
>>> > +       mgmt_adv_monitor_added(sk, hdev, m->handle);
>>> > +       if (m->state == ADV_MONITOR_STATE_NOT_REGISTERED)
>>> > +               m->state = ADV_MONITOR_STATE_REGISTERED;
>>> > +       hdev->adv_monitors_cnt++;
>>> > +       hci_update_passive_scan(hdev);
>>> >
>>> >         hci_dev_unlock(hdev);
>>> >
>>> > -       return 0;
>>> > +done:
>>> > +       bt_dev_dbg(hdev, "add monitor %d complete, status %u", m->handle,
>>> > +                  status);
>>> >
>>> > -unlock:
>>> > -       hci_free_adv_monitor(hdev, m);
>>> > -       hci_dev_unlock(hdev);
>>> > -       return mgmt_cmd_status(sk, hdev->id, op, status);
>>> > +       if (status)
>>> > +               return mgmt_cmd_status(sk, hdev->id, op, status);
>>> > +
>>> > +       return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS, &rp,
>>> > +                                sizeof(rp));
>>> >  }
>>> >
>>> >  static void parse_adv_monitor_rssi(struct adv_monitor *m,
>>> > @@ -4817,7 +4762,7 @@ static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>>> >  {
>>> >         struct mgmt_cp_add_adv_patterns_monitor *cp = data;
>>> >         struct adv_monitor *m = NULL;
>>> > -       u8 status = MGMT_STATUS_SUCCESS;
>>> > +       int status = MGMT_STATUS_SUCCESS;
>>> >         size_t expected_size = sizeof(*cp);
>>> >
>>> >         BT_DBG("request for %s", hdev->name);
>>> > @@ -4843,10 +4788,14 @@ static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>>> >
>>> >         parse_adv_monitor_rssi(m, NULL);
>>> >         status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
>>> > +       if (status)
>>> > +               goto done;
>>> > +
>>> > +       status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
>>> > +                                           MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
>>> >
>>> >  done:
>>> > -       return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
>>> > -                                         MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
>>> > +       return status;
>>> >  }
>>> >
>>> >  static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
>>> > @@ -4854,7 +4803,7 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
>>> >  {
>>> >         struct mgmt_cp_add_adv_patterns_monitor_rssi *cp = data;
>>> >         struct adv_monitor *m = NULL;
>>> > -       u8 status = MGMT_STATUS_SUCCESS;
>>> > +       int status = MGMT_STATUS_SUCCESS;
>>> >         size_t expected_size = sizeof(*cp);
>>> >
>>> >         BT_DBG("request for %s", hdev->name);
>>> > @@ -4880,10 +4829,14 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
>>> >
>>> >         parse_adv_monitor_rssi(m, &cp->rssi);
>>> >         status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
>>> > +       if (status)
>>> > +               goto done;
>>> >
>>> > -done:
>>> > -       return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
>>> > +       status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
>>> >                                          MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI);
>>> > +
>>> > +done:
>>> > +       return status;
>>> >  }
>>> >
>>> >  int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
>>> > @@ -4933,9 +4886,7 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
>>> >         hci_dev_lock(hdev);
>>> >
>>> >         if (pending_find(MGMT_OP_SET_LE, hdev) ||
>>> > -           pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev) ||
>>> > -           pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
>>> > -           pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev)) {
>>> > +           pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
>>> >                 status = MGMT_STATUS_BUSY;
>>> >                 goto unlock;
>>> >         }
>>> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
>>> > index f43994523b1f..9abea16c4305 100644
>>> > --- a/net/bluetooth/msft.c
>>> > +++ b/net/bluetooth/msft.c
>>> > @@ -106,8 +106,6 @@ struct msft_data {
>>> >         __u8 filter_enabled;
>>> >  };
>>> >
>>> > -static int __msft_add_monitor_pattern(struct hci_dev *hdev,
>>> > -                                     struct adv_monitor *monitor);
>>> >  static int __msft_remove_monitor(struct hci_dev *hdev,
>>> >                                  struct adv_monitor *monitor, u16 handle);
>>> >
>>> > @@ -164,34 +162,6 @@ static bool read_supported_features(struct hci_dev *hdev,
>>> >         return false;
>>> >  }
>>> >
>>> > -static void reregister_monitor(struct hci_dev *hdev, int handle)
>>> > -{
>>> > -       struct adv_monitor *monitor;
>>> > -       struct msft_data *msft = hdev->msft_data;
>>> > -       int err;
>>> > -
>>> > -       while (1) {
>>> > -               monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
>>> > -               if (!monitor) {
>>> > -                       /* All monitors have been resumed */
>>> > -                       msft->resuming = false;
>>> > -                       hci_update_passive_scan(hdev);
>>> > -                       return;
>>> > -               }
>>> > -
>>> > -               msft->pending_add_handle = (u16)handle;
>>> > -               err = __msft_add_monitor_pattern(hdev, monitor);
>>> > -
>>> > -               /* If success, we return and wait for monitor added callback */
>>> > -               if (!err)
>>> > -                       return;
>>> > -
>>> > -               /* Otherwise remove the monitor and keep registering */
>>> > -               hci_free_adv_monitor(hdev, monitor);
>>> > -               handle++;
>>> > -       }
>>> > -}
>>> > -
>>> >  /* is_mgmt = true matches the handle exposed to userspace via mgmt.
>>> >   * is_mgmt = false matches the handle used by the msft controller.
>>> >   * This function requires the caller holds hdev->lock
>>> > @@ -243,14 +213,14 @@ static int msft_monitor_device_del(struct hci_dev *hdev, __u16 mgmt_handle,
>>> >         return count;
>>> >  }
>>> >
>>> > -static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
>>> > -                                            u8 status, u16 opcode,
>>> > -                                            struct sk_buff *skb)
>>> > +static int msft_le_monitor_advertisement_cb(struct hci_dev *hdev, u16 opcode,
>>> > +                                           struct sk_buff *skb)
>>> >  {
>>> >         struct msft_rp_le_monitor_advertisement *rp;
>>> >         struct adv_monitor *monitor;
>>> >         struct msft_monitor_advertisement_handle_data *handle_data;
>>> >         struct msft_data *msft = hdev->msft_data;
>>> > +       int status = 0;
>>> >
>>> >         hci_dev_lock(hdev);
>>> >
>>> > @@ -262,15 +232,16 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
>>> >                 goto unlock;
>>> >         }
>>> >
>>> > -       if (status)
>>> > -               goto unlock;
>>> > -
>>> >         rp = (struct msft_rp_le_monitor_advertisement *)skb->data;
>>> >         if (skb->len < sizeof(*rp)) {
>>> >                 status = HCI_ERROR_UNSPECIFIED;
>>> >                 goto unlock;
>>> >         }
>>> >
>>> > +       status = rp->status;
>>> > +       if (status)
>>> > +               goto unlock;
>>> > +
>>> >         handle_data = kmalloc(sizeof(*handle_data), GFP_KERNEL);
>>> >         if (!handle_data) {
>>> >                 status = HCI_ERROR_UNSPECIFIED;
>>> > @@ -290,8 +261,7 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
>>> >
>>> >         hci_dev_unlock(hdev);
>>> >
>>> > -       if (!msft->resuming)
>>> > -               hci_add_adv_patterns_monitor_complete(hdev, status);
>>> > +       return status;
>>> >  }
>>> >
>>> >  static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
>>> > @@ -463,7 +433,7 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
>>> >         ptrdiff_t offset = 0;
>>> >         u8 pattern_count = 0;
>>> >         struct sk_buff *skb;
>>> > -       u8 status;
>>> > +       struct msft_data *msft = hdev->msft_data;
>>> >
>>> >         if (!msft_monitor_pattern_valid(monitor))
>>> >                 return -EINVAL;
>>> > @@ -505,20 +475,40 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
>>> >         if (IS_ERR(skb))
>>> >                 return PTR_ERR(skb);
>>> >
>>> > -       status = skb->data[0];
>>> > -       skb_pull(skb, 1);
>>> > +       msft->pending_add_handle = monitor->handle;
>>> >
>>> > -       msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb);
>>> > +       return msft_le_monitor_advertisement_cb(hdev, hdev->msft_opcode, skb);
>>> > +}
>>> >
>>> > -       return status;
>>> > +static void reregister_monitor(struct hci_dev *hdev)
>>> > +{
>>> > +       struct adv_monitor *monitor;
>>> > +       struct msft_data *msft = hdev->msft_data;
>>> > +       int handle = 0;
>>> > +
>>> > +       if (!msft)
>>> > +               return;
>>> > +
>>> > +       msft->resuming = true;
>>> > +
>>> > +       while (1) {
>>> > +               monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
>>> > +               if (!monitor)
>>> > +                       break;
>>> > +
>>> > +               msft_add_monitor_sync(hdev, monitor);
>>> > +
>>> > +               handle++;
>>> > +       }
>>> > +
>>> > +       /* All monitors have been reregistered */
>>> > +       msft->resuming = false;
>>> >  }
>>> >
>>> >  /* This function requires the caller holds hci_req_sync_lock */
>>> >  int msft_resume_sync(struct hci_dev *hdev)
>>> >  {
>>> >         struct msft_data *msft = hdev->msft_data;
>>> > -       struct adv_monitor *monitor;
>>> > -       int handle = 0;
>>> >
>>> >         if (!msft || !msft_monitor_supported(hdev))
>>> >                 return 0;
>>> > @@ -533,24 +523,12 @@ int msft_resume_sync(struct hci_dev *hdev)
>>> >
>>> >         hci_dev_unlock(hdev);
>>> >
>>> > -       msft->resuming = true;
>>> > -
>>> > -       while (1) {
>>> > -               monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
>>> > -               if (!monitor)
>>> > -                       break;
>>> > -
>>> > -               msft_add_monitor_sync(hdev, monitor);
>>> > -
>>> > -               handle++;
>>> > -       }
>>> > -
>>> > -       /* All monitors have been resumed */
>>> > -       msft->resuming = false;
>>> > +       reregister_monitor(hdev);
>>> >
>>> >         return 0;
>>> >  }
>>> >
>>> > +/* This function requires the caller holds hci_req_sync_lock */
>>> >  void msft_do_open(struct hci_dev *hdev)
>>> >  {
>>> >         struct msft_data *msft = hdev->msft_data;
>>> > @@ -583,7 +561,7 @@ void msft_do_open(struct hci_dev *hdev)
>>> >                 /* Monitors get removed on power off, so we need to explicitly
>>> >                  * tell the controller to re-monitor.
>>> >                  */
>>> > -               reregister_monitor(hdev, 0);
>>> > +               reregister_monitor(hdev);
>>> >         }
>>> >  }
>>> >
>>> > @@ -829,66 +807,7 @@ static void msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
>>> >         hci_dev_unlock(hdev);
>>> >  }
>>> >
>>> > -/* This function requires the caller holds hdev->lock */
>>> > -static int __msft_add_monitor_pattern(struct hci_dev *hdev,
>>> > -                                     struct adv_monitor *monitor)
>>> > -{
>>> > -       struct msft_cp_le_monitor_advertisement *cp;
>>> > -       struct msft_le_monitor_advertisement_pattern_data *pattern_data;
>>> > -       struct msft_le_monitor_advertisement_pattern *pattern;
>>> > -       struct adv_pattern *entry;
>>> > -       struct hci_request req;
>>> > -       struct msft_data *msft = hdev->msft_data;
>>> > -       size_t total_size = sizeof(*cp) + sizeof(*pattern_data);
>>> > -       ptrdiff_t offset = 0;
>>> > -       u8 pattern_count = 0;
>>> > -       int err = 0;
>>> > -
>>> > -       if (!msft_monitor_pattern_valid(monitor))
>>> > -               return -EINVAL;
>>> > -
>>> > -       list_for_each_entry(entry, &monitor->patterns, list) {
>>> > -               pattern_count++;
>>> > -               total_size += sizeof(*pattern) + entry->length;
>>> > -       }
>>> > -
>>> > -       cp = kmalloc(total_size, GFP_KERNEL);
>>> > -       if (!cp)
>>> > -               return -ENOMEM;
>>> > -
>>> > -       cp->sub_opcode = MSFT_OP_LE_MONITOR_ADVERTISEMENT;
>>> > -       cp->rssi_high = monitor->rssi.high_threshold;
>>> > -       cp->rssi_low = monitor->rssi.low_threshold;
>>> > -       cp->rssi_low_interval = (u8)monitor->rssi.low_threshold_timeout;
>>> > -       cp->rssi_sampling_period = monitor->rssi.sampling_period;
>>> > -
>>> > -       cp->cond_type = MSFT_MONITOR_ADVERTISEMENT_TYPE_PATTERN;
>>> > -
>>> > -       pattern_data = (void *)cp->data;
>>> > -       pattern_data->count = pattern_count;
>>> > -
>>> > -       list_for_each_entry(entry, &monitor->patterns, list) {
>>> > -               pattern = (void *)(pattern_data->data + offset);
>>> > -               /* the length also includes data_type and offset */
>>> > -               pattern->length = entry->length + 2;
>>> > -               pattern->data_type = entry->ad_type;
>>> > -               pattern->start_byte = entry->offset;
>>> > -               memcpy(pattern->pattern, entry->value, entry->length);
>>> > -               offset += sizeof(*pattern) + entry->length;
>>> > -       }
>>> > -
>>> > -       hci_req_init(&req, hdev);
>>> > -       hci_req_add(&req, hdev->msft_opcode, total_size, cp);
>>> > -       err = hci_req_run_skb(&req, msft_le_monitor_advertisement_cb);
>>> > -       kfree(cp);
>>> > -
>>> > -       if (!err)
>>> > -               msft->pending_add_handle = monitor->handle;
>>> > -
>>> > -       return err;
>>> > -}
>>> > -
>>> > -/* This function requires the caller holds hdev->lock */
>>> > +/* This function requires the caller holds hci_req_sync_lock */
>>> >  int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
>>> >  {
>>> >         struct msft_data *msft = hdev->msft_data;
>>> > @@ -899,7 +818,7 @@ int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
>>> >         if (msft->resuming || msft->suspending)
>>> >                 return -EBUSY;
>>> >
>>> > -       return __msft_add_monitor_pattern(hdev, monitor);
>>> > +       return msft_add_monitor_sync(hdev, monitor);
>>> >  }
>>> >
>>> >  /* This function requires the caller holds hdev->lock */
>>> > --
>>> > 2.36.1.124.g0e6072fb45-goog
>>> >
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>>
>> Regards,
>> Manish.
>
>
> Friendly ping to review this..
>
> Regards,
> Manish.
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5a52a2018b56..59953a7a6328 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1410,10 +1410,8 @@  bool hci_adv_instance_is_scannable(struct hci_dev *hdev, u8 instance);
 
 void hci_adv_monitors_clear(struct hci_dev *hdev);
 void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
-int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
 int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
-bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
-			int *err);
+int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
 bool hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle, int *err);
 bool hci_remove_all_adv_monitor(struct hci_dev *hdev, int *err);
 bool hci_is_adv_monitoring(struct hci_dev *hdev);
@@ -1875,7 +1873,6 @@  void mgmt_advertising_removed(struct sock *sk, struct hci_dev *hdev,
 			      u8 instance);
 void mgmt_adv_monitor_removed(struct hci_dev *hdev, u16 handle);
 int mgmt_phy_configuration_changed(struct hci_dev *hdev, struct sock *skip);
-int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
 int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
 void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
 				  bdaddr_t *bdaddr, u8 addr_type);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5abb2ca5b129..bbbbe3203130 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1873,11 +1873,6 @@  void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
 	kfree(monitor);
 }
 
-int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
-{
-	return mgmt_add_adv_patterns_monitor_complete(hdev, status);
-}
-
 int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
 {
 	return mgmt_remove_adv_monitor_complete(hdev, status);
@@ -1885,49 +1880,49 @@  int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
 
 /* Assigns handle to a monitor, and if offloading is supported and power is on,
  * also attempts to forward the request to the controller.
- * Returns true if request is forwarded (result is pending), false otherwise.
- * This function requires the caller holds hdev->lock.
  */
-bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
-			 int *err)
+int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
 {
 	int min, max, handle;
+	int status = 0;
 
-	*err = 0;
+	if (!monitor)
+		return -EINVAL;
 
-	if (!monitor) {
-		*err = -EINVAL;
-		return false;
-	}
+	hci_dev_lock(hdev);
 
 	min = HCI_MIN_ADV_MONITOR_HANDLE;
 	max = HCI_MIN_ADV_MONITOR_HANDLE + HCI_MAX_ADV_MONITOR_NUM_HANDLES;
 	handle = idr_alloc(&hdev->adv_monitors_idr, monitor, min, max,
 			   GFP_KERNEL);
-	if (handle < 0) {
-		*err = handle;
-		return false;
-	}
+
+	hci_dev_unlock(hdev);
+
+	if (handle < 0)
+		return handle;
 
 	monitor->handle = handle;
 
 	if (!hdev_is_powered(hdev))
-		return false;
+		return status;
 
 	switch (hci_get_adv_monitor_offload_ext(hdev)) {
 	case HCI_ADV_MONITOR_EXT_NONE:
-		hci_update_passive_scan(hdev);
-		bt_dev_dbg(hdev, "%s add monitor status %d", hdev->name, *err);
+		bt_dev_dbg(hdev, "%s add monitor %d status %d", hdev->name,
+			   monitor->handle, status);
 		/* Message was not forwarded to controller - not an error */
-		return false;
+		break;
+
 	case HCI_ADV_MONITOR_EXT_MSFT:
-		*err = msft_add_monitor_pattern(hdev, monitor);
-		bt_dev_dbg(hdev, "%s add monitor msft status %d", hdev->name,
-			   *err);
+		hci_req_sync_lock(hdev);
+		status = msft_add_monitor_pattern(hdev, monitor);
+		hci_req_sync_unlock(hdev);
+		bt_dev_dbg(hdev, "%s add monitor %d msft status %d", hdev->name,
+			   monitor->handle, status);
 		break;
 	}
 
-	return (*err == 0);
+	return status;
 }
 
 /* Attempts to tell the controller and free the monitor. If somehow the
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 74937a834648..d04f90698a87 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4653,75 +4653,21 @@  static int read_adv_mon_features(struct sock *sk, struct hci_dev *hdev,
 	return err;
 }
 
-int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
-{
-	struct mgmt_rp_add_adv_patterns_monitor rp;
-	struct mgmt_pending_cmd *cmd;
-	struct adv_monitor *monitor;
-	int err = 0;
-
-	hci_dev_lock(hdev);
-
-	cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev);
-	if (!cmd) {
-		cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev);
-		if (!cmd)
-			goto done;
-	}
-
-	monitor = cmd->user_data;
-	rp.monitor_handle = cpu_to_le16(monitor->handle);
-
-	if (!status) {
-		mgmt_adv_monitor_added(cmd->sk, hdev, monitor->handle);
-		hdev->adv_monitors_cnt++;
-		if (monitor->state == ADV_MONITOR_STATE_NOT_REGISTERED)
-			monitor->state = ADV_MONITOR_STATE_REGISTERED;
-		hci_update_passive_scan(hdev);
-	}
-
-	err = mgmt_cmd_complete(cmd->sk, cmd->index, cmd->opcode,
-				mgmt_status(status), &rp, sizeof(rp));
-	mgmt_pending_remove(cmd);
-
-done:
-	hci_dev_unlock(hdev);
-	bt_dev_dbg(hdev, "add monitor %d complete, status %u",
-		   rp.monitor_handle, status);
-
-	return err;
-}
-
 static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
-				      struct adv_monitor *m, u8 status,
-				      void *data, u16 len, u16 op)
+				      struct adv_monitor *m, void *data,
+				      u16 len, u16 op)
 {
 	struct mgmt_rp_add_adv_patterns_monitor rp;
-	struct mgmt_pending_cmd *cmd;
+	u8 status = MGMT_STATUS_SUCCESS;
 	int err;
-	bool pending;
-
-	hci_dev_lock(hdev);
-
-	if (status)
-		goto unlock;
 
 	if (pending_find(MGMT_OP_SET_LE, hdev) ||
-	    pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
-	    pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev) ||
 	    pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
 		status = MGMT_STATUS_BUSY;
-		goto unlock;
-	}
-
-	cmd = mgmt_pending_add(sk, op, hdev, data, len);
-	if (!cmd) {
-		status = MGMT_STATUS_NO_RESOURCES;
-		goto unlock;
+		goto done;
 	}
 
-	cmd->user_data = m;
-	pending = hci_add_adv_monitor(hdev, m, &err);
+	err = hci_add_adv_monitor(hdev, m);
 	if (err) {
 		if (err == -ENOSPC || err == -ENOMEM)
 			status = MGMT_STATUS_NO_RESOURCES;
@@ -4730,30 +4676,29 @@  static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
 		else
 			status = MGMT_STATUS_FAILED;
 
-		mgmt_pending_remove(cmd);
-		goto unlock;
+		goto done;
 	}
 
-	if (!pending) {
-		mgmt_pending_remove(cmd);
-		rp.monitor_handle = cpu_to_le16(m->handle);
-		mgmt_adv_monitor_added(sk, hdev, m->handle);
-		m->state = ADV_MONITOR_STATE_REGISTERED;
-		hdev->adv_monitors_cnt++;
+	hci_dev_lock(hdev);
 
-		hci_dev_unlock(hdev);
-		return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS,
-					 &rp, sizeof(rp));
-	}
+	rp.monitor_handle = cpu_to_le16(m->handle);
+	mgmt_adv_monitor_added(sk, hdev, m->handle);
+	if (m->state == ADV_MONITOR_STATE_NOT_REGISTERED)
+		m->state = ADV_MONITOR_STATE_REGISTERED;
+	hdev->adv_monitors_cnt++;
+	hci_update_passive_scan(hdev);
 
 	hci_dev_unlock(hdev);
 
-	return 0;
+done:
+	bt_dev_dbg(hdev, "add monitor %d complete, status %u", m->handle,
+		   status);
 
-unlock:
-	hci_free_adv_monitor(hdev, m);
-	hci_dev_unlock(hdev);
-	return mgmt_cmd_status(sk, hdev->id, op, status);
+	if (status)
+		return mgmt_cmd_status(sk, hdev->id, op, status);
+
+	return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS, &rp,
+				 sizeof(rp));
 }
 
 static void parse_adv_monitor_rssi(struct adv_monitor *m,
@@ -4817,7 +4762,7 @@  static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
 {
 	struct mgmt_cp_add_adv_patterns_monitor *cp = data;
 	struct adv_monitor *m = NULL;
-	u8 status = MGMT_STATUS_SUCCESS;
+	int status = MGMT_STATUS_SUCCESS;
 	size_t expected_size = sizeof(*cp);
 
 	BT_DBG("request for %s", hdev->name);
@@ -4843,10 +4788,14 @@  static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
 
 	parse_adv_monitor_rssi(m, NULL);
 	status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
+	if (status)
+		goto done;
+
+	status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
+					    MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
 
 done:
-	return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
-					  MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
+	return status;
 }
 
 static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
@@ -4854,7 +4803,7 @@  static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
 {
 	struct mgmt_cp_add_adv_patterns_monitor_rssi *cp = data;
 	struct adv_monitor *m = NULL;
-	u8 status = MGMT_STATUS_SUCCESS;
+	int status = MGMT_STATUS_SUCCESS;
 	size_t expected_size = sizeof(*cp);
 
 	BT_DBG("request for %s", hdev->name);
@@ -4880,10 +4829,14 @@  static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
 
 	parse_adv_monitor_rssi(m, &cp->rssi);
 	status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
+	if (status)
+		goto done;
 
-done:
-	return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
+	status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
 					 MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI);
+
+done:
+	return status;
 }
 
 int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
@@ -4933,9 +4886,7 @@  static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
 	hci_dev_lock(hdev);
 
 	if (pending_find(MGMT_OP_SET_LE, hdev) ||
-	    pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev) ||
-	    pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
-	    pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev)) {
+	    pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
 		status = MGMT_STATUS_BUSY;
 		goto unlock;
 	}
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index f43994523b1f..9abea16c4305 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -106,8 +106,6 @@  struct msft_data {
 	__u8 filter_enabled;
 };
 
-static int __msft_add_monitor_pattern(struct hci_dev *hdev,
-				      struct adv_monitor *monitor);
 static int __msft_remove_monitor(struct hci_dev *hdev,
 				 struct adv_monitor *monitor, u16 handle);
 
@@ -164,34 +162,6 @@  static bool read_supported_features(struct hci_dev *hdev,
 	return false;
 }
 
-static void reregister_monitor(struct hci_dev *hdev, int handle)
-{
-	struct adv_monitor *monitor;
-	struct msft_data *msft = hdev->msft_data;
-	int err;
-
-	while (1) {
-		monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
-		if (!monitor) {
-			/* All monitors have been resumed */
-			msft->resuming = false;
-			hci_update_passive_scan(hdev);
-			return;
-		}
-
-		msft->pending_add_handle = (u16)handle;
-		err = __msft_add_monitor_pattern(hdev, monitor);
-
-		/* If success, we return and wait for monitor added callback */
-		if (!err)
-			return;
-
-		/* Otherwise remove the monitor and keep registering */
-		hci_free_adv_monitor(hdev, monitor);
-		handle++;
-	}
-}
-
 /* is_mgmt = true matches the handle exposed to userspace via mgmt.
  * is_mgmt = false matches the handle used by the msft controller.
  * This function requires the caller holds hdev->lock
@@ -243,14 +213,14 @@  static int msft_monitor_device_del(struct hci_dev *hdev, __u16 mgmt_handle,
 	return count;
 }
 
-static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
-					     u8 status, u16 opcode,
-					     struct sk_buff *skb)
+static int msft_le_monitor_advertisement_cb(struct hci_dev *hdev, u16 opcode,
+					    struct sk_buff *skb)
 {
 	struct msft_rp_le_monitor_advertisement *rp;
 	struct adv_monitor *monitor;
 	struct msft_monitor_advertisement_handle_data *handle_data;
 	struct msft_data *msft = hdev->msft_data;
+	int status = 0;
 
 	hci_dev_lock(hdev);
 
@@ -262,15 +232,16 @@  static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
 		goto unlock;
 	}
 
-	if (status)
-		goto unlock;
-
 	rp = (struct msft_rp_le_monitor_advertisement *)skb->data;
 	if (skb->len < sizeof(*rp)) {
 		status = HCI_ERROR_UNSPECIFIED;
 		goto unlock;
 	}
 
+	status = rp->status;
+	if (status)
+		goto unlock;
+
 	handle_data = kmalloc(sizeof(*handle_data), GFP_KERNEL);
 	if (!handle_data) {
 		status = HCI_ERROR_UNSPECIFIED;
@@ -290,8 +261,7 @@  static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
 
 	hci_dev_unlock(hdev);
 
-	if (!msft->resuming)
-		hci_add_adv_patterns_monitor_complete(hdev, status);
+	return status;
 }
 
 static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
@@ -463,7 +433,7 @@  static int msft_add_monitor_sync(struct hci_dev *hdev,
 	ptrdiff_t offset = 0;
 	u8 pattern_count = 0;
 	struct sk_buff *skb;
-	u8 status;
+	struct msft_data *msft = hdev->msft_data;
 
 	if (!msft_monitor_pattern_valid(monitor))
 		return -EINVAL;
@@ -505,20 +475,40 @@  static int msft_add_monitor_sync(struct hci_dev *hdev,
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
 
-	status = skb->data[0];
-	skb_pull(skb, 1);
+	msft->pending_add_handle = monitor->handle;
 
-	msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb);
+	return msft_le_monitor_advertisement_cb(hdev, hdev->msft_opcode, skb);
+}
 
-	return status;
+static void reregister_monitor(struct hci_dev *hdev)
+{
+	struct adv_monitor *monitor;
+	struct msft_data *msft = hdev->msft_data;
+	int handle = 0;
+
+	if (!msft)
+		return;
+
+	msft->resuming = true;
+
+	while (1) {
+		monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
+		if (!monitor)
+			break;
+
+		msft_add_monitor_sync(hdev, monitor);
+
+		handle++;
+	}
+
+	/* All monitors have been reregistered */
+	msft->resuming = false;
 }
 
 /* This function requires the caller holds hci_req_sync_lock */
 int msft_resume_sync(struct hci_dev *hdev)
 {
 	struct msft_data *msft = hdev->msft_data;
-	struct adv_monitor *monitor;
-	int handle = 0;
 
 	if (!msft || !msft_monitor_supported(hdev))
 		return 0;
@@ -533,24 +523,12 @@  int msft_resume_sync(struct hci_dev *hdev)
 
 	hci_dev_unlock(hdev);
 
-	msft->resuming = true;
-
-	while (1) {
-		monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
-		if (!monitor)
-			break;
-
-		msft_add_monitor_sync(hdev, monitor);
-
-		handle++;
-	}
-
-	/* All monitors have been resumed */
-	msft->resuming = false;
+	reregister_monitor(hdev);
 
 	return 0;
 }
 
+/* This function requires the caller holds hci_req_sync_lock */
 void msft_do_open(struct hci_dev *hdev)
 {
 	struct msft_data *msft = hdev->msft_data;
@@ -583,7 +561,7 @@  void msft_do_open(struct hci_dev *hdev)
 		/* Monitors get removed on power off, so we need to explicitly
 		 * tell the controller to re-monitor.
 		 */
-		reregister_monitor(hdev, 0);
+		reregister_monitor(hdev);
 	}
 }
 
@@ -829,66 +807,7 @@  static void msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
 	hci_dev_unlock(hdev);
 }
 
-/* This function requires the caller holds hdev->lock */
-static int __msft_add_monitor_pattern(struct hci_dev *hdev,
-				      struct adv_monitor *monitor)
-{
-	struct msft_cp_le_monitor_advertisement *cp;
-	struct msft_le_monitor_advertisement_pattern_data *pattern_data;
-	struct msft_le_monitor_advertisement_pattern *pattern;
-	struct adv_pattern *entry;
-	struct hci_request req;
-	struct msft_data *msft = hdev->msft_data;
-	size_t total_size = sizeof(*cp) + sizeof(*pattern_data);
-	ptrdiff_t offset = 0;
-	u8 pattern_count = 0;
-	int err = 0;
-
-	if (!msft_monitor_pattern_valid(monitor))
-		return -EINVAL;
-
-	list_for_each_entry(entry, &monitor->patterns, list) {
-		pattern_count++;
-		total_size += sizeof(*pattern) + entry->length;
-	}
-
-	cp = kmalloc(total_size, GFP_KERNEL);
-	if (!cp)
-		return -ENOMEM;
-
-	cp->sub_opcode = MSFT_OP_LE_MONITOR_ADVERTISEMENT;
-	cp->rssi_high = monitor->rssi.high_threshold;
-	cp->rssi_low = monitor->rssi.low_threshold;
-	cp->rssi_low_interval = (u8)monitor->rssi.low_threshold_timeout;
-	cp->rssi_sampling_period = monitor->rssi.sampling_period;
-
-	cp->cond_type = MSFT_MONITOR_ADVERTISEMENT_TYPE_PATTERN;
-
-	pattern_data = (void *)cp->data;
-	pattern_data->count = pattern_count;
-
-	list_for_each_entry(entry, &monitor->patterns, list) {
-		pattern = (void *)(pattern_data->data + offset);
-		/* the length also includes data_type and offset */
-		pattern->length = entry->length + 2;
-		pattern->data_type = entry->ad_type;
-		pattern->start_byte = entry->offset;
-		memcpy(pattern->pattern, entry->value, entry->length);
-		offset += sizeof(*pattern) + entry->length;
-	}
-
-	hci_req_init(&req, hdev);
-	hci_req_add(&req, hdev->msft_opcode, total_size, cp);
-	err = hci_req_run_skb(&req, msft_le_monitor_advertisement_cb);
-	kfree(cp);
-
-	if (!err)
-		msft->pending_add_handle = monitor->handle;
-
-	return err;
-}
-
-/* This function requires the caller holds hdev->lock */
+/* This function requires the caller holds hci_req_sync_lock */
 int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
 {
 	struct msft_data *msft = hdev->msft_data;
@@ -899,7 +818,7 @@  int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
 	if (msft->resuming || msft->suspending)
 		return -EBUSY;
 
-	return __msft_add_monitor_pattern(hdev, monitor);
+	return msft_add_monitor_sync(hdev, monitor);
 }
 
 /* This function requires the caller holds hdev->lock */