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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Manish,

On Tue, Jun 7, 2022 at 9:57 AM Manish Mandlik <mmandlik@xxxxxxxxxx> wrote:
>
> Hi Luiz,
>
> On Thu, May 26, 2022 at 7:01 PM Manish Mandlik <mmandlik@xxxxxxxxxx> wrote:
>>
>> Hi Luiz,
>>
>>
>> On Wed, May 25, 2022 at 5:46 PM Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote:
>>>
>>> Hi Manish,
>>>
>>> On Tue, May 24, 2022 at 1:14 PM Manish Mandlik <mmandlik@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> > Reviewed-by: Miao-chen Chou <mcchou@xxxxxxxxxx>
>>> > ---
>>> >
>>> >  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.



-- 
Luiz Augusto von Dentz




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux