Bluetooth: hci_sync: Refactor remove Adv Monitor
authorManish Mandlik <mmandlik@google.com>
Wed, 20 Jul 2022 23:21:14 +0000 (16:21 -0700)
committerLuiz Augusto von Dentz <luiz.von.dentz@intel.com>
Fri, 22 Jul 2022 00:14:55 +0000 (17:14 -0700)
Make use of hci_cmd_sync_queue for removing an advertisement monitor.

Signed-off-by: Manish Mandlik <mmandlik@google.com>
Reviewed-by: Miao-chen Chou <mcchou@google.com>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
include/net/bluetooth/hci_core.h
net/bluetooth/hci_core.c
net/bluetooth/mgmt.c
net/bluetooth/msft.c
net/bluetooth/msft.h

index b45d312..df7dac4 100644 (file)
@@ -1420,10 +1420,9 @@ 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_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
 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);
+int hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle);
+int hci_remove_all_adv_monitor(struct hci_dev *hdev);
 bool hci_is_adv_monitoring(struct hci_dev *hdev);
 int hci_get_adv_monitor_offload_ext(struct hci_dev *hdev);
 
@@ -1883,7 +1882,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_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);
 
index 6f9ce2f..e42824e 100644 (file)
@@ -1880,11 +1880,6 @@ void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
        kfree(monitor);
 }
 
-int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
-{
-       return mgmt_remove_adv_monitor_complete(hdev, status);
-}
-
 /* Assigns handle to a monitor, and if offloading is supported and power is on,
  * also attempts to forward the request to the controller.
  * This function requires the caller holds hci_req_sync_lock.
@@ -1933,92 +1928,72 @@ int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
 
 /* Attempts to tell the controller and free the monitor. If somehow the
  * controller doesn't have a corresponding handle, remove anyway.
- * Returns true if request is forwarded (result is pending), false otherwise.
- * This function requires the caller holds hdev->lock.
+ * This function requires the caller holds hci_req_sync_lock.
  */
-static bool hci_remove_adv_monitor(struct hci_dev *hdev,
-                                  struct adv_monitor *monitor,
-                                  u16 handle, int *err)
+static int hci_remove_adv_monitor(struct hci_dev *hdev,
+                                 struct adv_monitor *monitor)
 {
-       *err = 0;
+       int status = 0;
 
        switch (hci_get_adv_monitor_offload_ext(hdev)) {
        case HCI_ADV_MONITOR_EXT_NONE: /* also goes here when powered off */
+               bt_dev_dbg(hdev, "%s remove monitor %d status %d", hdev->name,
+                          monitor->handle, status);
                goto free_monitor;
+
        case HCI_ADV_MONITOR_EXT_MSFT:
-               *err = msft_remove_monitor(hdev, monitor, handle);
+               status = msft_remove_monitor(hdev, monitor);
+               bt_dev_dbg(hdev, "%s remove monitor %d msft status %d",
+                          hdev->name, monitor->handle, status);
                break;
        }
 
        /* In case no matching handle registered, just free the monitor */
-       if (*err == -ENOENT)
+       if (status == -ENOENT)
                goto free_monitor;
 
-       return (*err == 0);
+       return status;
 
 free_monitor:
-       if (*err == -ENOENT)
+       if (status == -ENOENT)
                bt_dev_warn(hdev, "Removing monitor with no matching handle %d",
                            monitor->handle);
        hci_free_adv_monitor(hdev, monitor);
 
-       *err = 0;
-       return false;
+       return status;
 }
 
-/* Returns true if request is forwarded (result is pending), false otherwise.
- * This function requires the caller holds hdev->lock.
- */
-bool hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle, int *err)
+/* This function requires the caller holds hci_req_sync_lock */
+int hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle)
 {
        struct adv_monitor *monitor = idr_find(&hdev->adv_monitors_idr, handle);
-       bool pending;
-
-       if (!monitor) {
-               *err = -EINVAL;
-               return false;
-       }
-
-       pending = hci_remove_adv_monitor(hdev, monitor, handle, err);
-       if (!*err && !pending)
-               hci_update_passive_scan(hdev);
 
-       bt_dev_dbg(hdev, "%s remove monitor handle %d, status %d, %spending",
-                  hdev->name, handle, *err, pending ? "" : "not ");
+       if (!monitor)
+               return -EINVAL;
 
-       return pending;
+       return hci_remove_adv_monitor(hdev, monitor);
 }
 
-/* Returns true if request is forwarded (result is pending), false otherwise.
- * This function requires the caller holds hdev->lock.
- */
-bool hci_remove_all_adv_monitor(struct hci_dev *hdev, int *err)
+/* This function requires the caller holds hci_req_sync_lock */
+int hci_remove_all_adv_monitor(struct hci_dev *hdev)
 {
        struct adv_monitor *monitor;
        int idr_next_id = 0;
-       bool pending = false;
-       bool update = false;
-
-       *err = 0;
+       int status = 0;
 
-       while (!*err && !pending) {
+       while (1) {
                monitor = idr_get_next(&hdev->adv_monitors_idr, &idr_next_id);
                if (!monitor)
                        break;
 
-               pending = hci_remove_adv_monitor(hdev, monitor, 0, err);
+               status = hci_remove_adv_monitor(hdev, monitor);
+               if (status)
+                       return status;
 
-               if (!*err && !pending)
-                       update = true;
+               idr_next_id++;
        }
 
-       if (update)
-               hci_update_passive_scan(hdev);
-
-       bt_dev_dbg(hdev, "%s remove all monitors status %d, %spending",
-                  hdev->name, *err, pending ? "" : "not ");
-
-       return pending;
+       return status;
 }
 
 /* This function requires the caller holds hdev->lock */
index e2d41dc..05a680a 100644 (file)
@@ -4861,49 +4861,46 @@ done:
                                         MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI);
 }
 
-int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
+static void mgmt_remove_adv_monitor_complete(struct hci_dev *hdev,
+                                            void *data, int status)
 {
        struct mgmt_rp_remove_adv_monitor rp;
-       struct mgmt_cp_remove_adv_monitor *cp;
-       struct mgmt_pending_cmd *cmd;
-       int err = 0;
+       struct mgmt_pending_cmd *cmd = data;
+       struct mgmt_cp_remove_adv_monitor *cp = cmd->param;
 
        hci_dev_lock(hdev);
 
-       cmd = pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev);
-       if (!cmd)
-               goto done;
-
-       cp = cmd->param;
        rp.monitor_handle = cp->monitor_handle;
 
        if (!status)
                hci_update_passive_scan(hdev);
 
-       err = mgmt_cmd_complete(cmd->sk, cmd->index, cmd->opcode,
-                               mgmt_status(status), &rp, sizeof(rp));
+       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, "remove monitor %d complete, status %u",
+       bt_dev_dbg(hdev, "remove monitor %d complete, status %d",
                   rp.monitor_handle, status);
+}
 
-       return err;
+static int mgmt_remove_adv_monitor_sync(struct hci_dev *hdev, void *data)
+{
+       struct mgmt_pending_cmd *cmd = data;
+       struct mgmt_cp_remove_adv_monitor *cp = cmd->param;
+       u16 handle = __le16_to_cpu(cp->monitor_handle);
+
+       if (!handle)
+               return hci_remove_all_adv_monitor(hdev);
+
+       return hci_remove_single_adv_monitor(hdev, handle);
 }
 
 static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
                              void *data, u16 len)
 {
-       struct mgmt_cp_remove_adv_monitor *cp = data;
-       struct mgmt_rp_remove_adv_monitor rp;
        struct mgmt_pending_cmd *cmd;
-       u16 handle = __le16_to_cpu(cp->monitor_handle);
        int err, status;
-       bool pending;
-
-       BT_DBG("request for %s", hdev->name);
-       rp.monitor_handle = cp->monitor_handle;
 
        hci_dev_lock(hdev);
 
@@ -4921,34 +4918,23 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
                goto unlock;
        }
 
-       if (handle)
-               pending = hci_remove_single_adv_monitor(hdev, handle, &err);
-       else
-               pending = hci_remove_all_adv_monitor(hdev, &err);
+       err = hci_cmd_sync_queue(hdev, mgmt_remove_adv_monitor_sync, cmd,
+                                mgmt_remove_adv_monitor_complete);
 
        if (err) {
                mgmt_pending_remove(cmd);
 
-               if (err == -ENOENT)
-                       status = MGMT_STATUS_INVALID_INDEX;
+               if (err == -ENOMEM)
+                       status = MGMT_STATUS_NO_RESOURCES;
                else
                        status = MGMT_STATUS_FAILED;
 
-               goto unlock;
-       }
-
-       /* monitor can be removed without forwarding request to controller */
-       if (!pending) {
                mgmt_pending_remove(cmd);
-               hci_dev_unlock(hdev);
-
-               return mgmt_cmd_complete(sk, hdev->id,
-                                        MGMT_OP_REMOVE_ADV_MONITOR,
-                                        MGMT_STATUS_SUCCESS,
-                                        &rp, sizeof(rp));
+               goto unlock;
        }
 
        hci_dev_unlock(hdev);
+
        return 0;
 
 unlock:
index 54fbc71..1497576 100644 (file)
@@ -99,15 +99,11 @@ struct msft_data {
        __u8  evt_prefix_len;
        __u8  *evt_prefix;
        struct list_head handle_map;
-       __u16 pending_remove_handle;
        __u8 resuming;
        __u8 suspending;
        __u8 filter_enabled;
 };
 
-static int __msft_remove_monitor(struct hci_dev *hdev,
-                                struct adv_monitor *monitor, u16 handle);
-
 bool msft_monitor_supported(struct hci_dev *hdev)
 {
        return !!(msft_get_features(hdev) & MSFT_FEATURE_MASK_LE_ADV_MONITOR);
@@ -255,20 +251,15 @@ unlock:
        return status;
 }
 
-static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
-                                                   u8 status, u16 opcode,
-                                                   struct sk_buff *skb)
+static int msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
+                                                  u16 opcode,
+                                                  struct adv_monitor *monitor,
+                                                  struct sk_buff *skb)
 {
-       struct msft_cp_le_cancel_monitor_advertisement *cp;
        struct msft_rp_le_cancel_monitor_advertisement *rp;
-       struct adv_monitor *monitor;
        struct msft_monitor_advertisement_handle_data *handle_data;
        struct msft_data *msft = hdev->msft_data;
-       int err;
-       bool pending;
-
-       if (status)
-               goto done;
+       int status = 0;
 
        rp = (struct msft_rp_le_cancel_monitor_advertisement *)skb->data;
        if (skb->len < sizeof(*rp)) {
@@ -276,22 +267,22 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
                goto done;
        }
 
+       status = rp->status;
+       if (status)
+               goto done;
+
        hci_dev_lock(hdev);
 
-       cp = hci_sent_cmd_data(hdev, hdev->msft_opcode);
-       handle_data = msft_find_handle_data(hdev, cp->handle, false);
+       handle_data = msft_find_handle_data(hdev, monitor->handle, true);
 
        if (handle_data) {
-               monitor = idr_find(&hdev->adv_monitors_idr,
-                                  handle_data->mgmt_handle);
-
-               if (monitor && monitor->state == ADV_MONITOR_STATE_OFFLOADED)
+               if (monitor->state == ADV_MONITOR_STATE_OFFLOADED)
                        monitor->state = ADV_MONITOR_STATE_REGISTERED;
 
                /* Do not free the monitor if it is being removed due to
                 * suspend. It will be re-monitored on resume.
                 */
-               if (monitor && !msft->suspending) {
+               if (!msft->suspending) {
                        hci_free_adv_monitor(hdev, monitor);
 
                        /* Clear any monitored devices by this Adv Monitor */
@@ -303,35 +294,19 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
                kfree(handle_data);
        }
 
-       /* If remove all monitors is required, we need to continue the process
-        * here because the earlier it was paused when waiting for the
-        * response from controller.
-        */
-       if (msft->pending_remove_handle == 0) {
-               pending = hci_remove_all_adv_monitor(hdev, &err);
-               if (pending) {
-                       hci_dev_unlock(hdev);
-                       return;
-               }
-
-               if (err)
-                       status = HCI_ERROR_UNSPECIFIED;
-       }
-
        hci_dev_unlock(hdev);
 
 done:
-       if (!msft->suspending)
-               hci_remove_adv_monitor_complete(hdev, status);
+       return status;
 }
 
+/* This function requires the caller holds hci_req_sync_lock */
 static int msft_remove_monitor_sync(struct hci_dev *hdev,
                                    struct adv_monitor *monitor)
 {
        struct msft_cp_le_cancel_monitor_advertisement cp;
        struct msft_monitor_advertisement_handle_data *handle_data;
        struct sk_buff *skb;
-       u8 status;
 
        handle_data = msft_find_handle_data(hdev, monitor->handle, true);
 
@@ -347,13 +322,8 @@ static int msft_remove_monitor_sync(struct hci_dev *hdev,
        if (IS_ERR(skb))
                return PTR_ERR(skb);
 
-       status = skb->data[0];
-       skb_pull(skb, 1);
-
-       msft_le_cancel_monitor_advertisement_cb(hdev, status, hdev->msft_opcode,
-                                               skb);
-
-       return status;
+       return msft_le_cancel_monitor_advertisement_cb(hdev, hdev->msft_opcode,
+                                                      monitor, skb);
 }
 
 /* This function requires the caller holds hci_req_sync_lock */
@@ -811,38 +781,8 @@ int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
        return msft_add_monitor_sync(hdev, monitor);
 }
 
-/* This function requires the caller holds hdev->lock */
-static int __msft_remove_monitor(struct hci_dev *hdev,
-                                struct adv_monitor *monitor, u16 handle)
-{
-       struct msft_cp_le_cancel_monitor_advertisement cp;
-       struct msft_monitor_advertisement_handle_data *handle_data;
-       struct hci_request req;
-       struct msft_data *msft = hdev->msft_data;
-       int err = 0;
-
-       handle_data = msft_find_handle_data(hdev, monitor->handle, true);
-
-       /* If no matched handle, just remove without telling controller */
-       if (!handle_data)
-               return -ENOENT;
-
-       cp.sub_opcode = MSFT_OP_LE_CANCEL_MONITOR_ADVERTISEMENT;
-       cp.handle = handle_data->msft_handle;
-
-       hci_req_init(&req, hdev);
-       hci_req_add(&req, hdev->msft_opcode, sizeof(cp), &cp);
-       err = hci_req_run_skb(&req, msft_le_cancel_monitor_advertisement_cb);
-
-       if (!err)
-               msft->pending_remove_handle = handle;
-
-       return err;
-}
-
-/* This function requires the caller holds hdev->lock */
-int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
-                       u16 handle)
+/* This function requires the caller holds hci_req_sync_lock */
+int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
 {
        struct msft_data *msft = hdev->msft_data;
 
@@ -852,7 +792,7 @@ int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
        if (msft->resuming || msft->suspending)
                return -EBUSY;
 
-       return __msft_remove_monitor(hdev, monitor, handle);
+       return msft_remove_monitor_sync(hdev, monitor);
 }
 
 void msft_req_add_set_filter_enable(struct hci_request *req, bool enable)
index afcaf7d..2a63205 100644 (file)
@@ -20,8 +20,7 @@ void msft_do_close(struct hci_dev *hdev);
 void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb);
 __u64 msft_get_features(struct hci_dev *hdev);
 int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor);
-int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
-                       u16 handle);
+int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
 void msft_req_add_set_filter_enable(struct hci_request *req, bool enable);
 int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
 int msft_suspend_sync(struct hci_dev *hdev);
@@ -49,8 +48,7 @@ static inline int msft_add_monitor_pattern(struct hci_dev *hdev,
 }
 
 static inline int msft_remove_monitor(struct hci_dev *hdev,
-                                     struct adv_monitor *monitor,
-                                     u16 handle)
+                                     struct adv_monitor *monitor)
 {
        return -EOPNOTSUPP;
 }