Bluetooth: Fix Advertisement Monitor Suspend/Resume

During system suspend, advertisement monitoring is disabled by setting
the HCI_VS_MSFT_LE_Set_Advertisement_Filter_Enable to False. This
disables the monitoring during suspend, however, if the controller is
monitoring a device, it sends HCI_VS_MSFT_LE_Monitor_Device_Event to
indicate that the monitoring has been stopped for that particular
device. This event may occur after suspend depending on the
low_threshold_timeout and peer device advertisement frequency, which
causes early wake up.

Right way to disable the monitoring for suspend is by removing all the
monitors before suspend and re-monitor after resume to ensure no events
are received during suspend. This patch fixes this suspend/resume issue.

Following tests are performed:
- Add monitors before suspend and make sure DeviceFound gets triggered
- Suspend the system and verify that all monitors are removed by kernel
  but not Released by bluetoothd
- Wake up and verify that all monitors are added again and DeviceFound
  gets triggered

Signed-off-by: Manish Mandlik <mmandlik@google.com>
Reviewed-by: Archie Pusaka <apusaka@google.com>
Reviewed-by: Miao-chen Chou <mcchou@google.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 55885c4651abc942cbe819232dc61497bbb4f87a..92611bfc0b9e1655696e8f8163b2a687d8e65d4f 100644 (file)
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1105,21 +1105,24 @@ static void suspend_req_complete(struct hci_dev *hdev, u8 status, u16 opcode)
        }
 }
 
-static void hci_req_add_set_adv_filter_enable(struct hci_request *req,
-                                             bool enable)
+static void hci_req_prepare_adv_monitor_suspend(struct hci_request *req,
+                                               bool suspending)
 {
        struct hci_dev *hdev = req->hdev;
 
        switch (hci_get_adv_monitor_offload_ext(hdev)) {
        case HCI_ADV_MONITOR_EXT_MSFT:
-               msft_req_add_set_filter_enable(req, enable);
+               if (suspending)
+                       msft_suspend(hdev);
+               else
+                       msft_resume(hdev);
                break;
        default:
                return;
        }
 
        /* No need to block when enabling since it's on resume path */
-       if (hdev->suspended && !enable)
+       if (hdev->suspended && suspending)
                set_bit(SUSPEND_SET_ADV_FILTER, hdev->suspend_tasks);
 }
 
@@ -1186,7 +1189,7 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
                }
 
                /* Disable advertisement filters */
-               hci_req_add_set_adv_filter_enable(&req, false);
+               hci_req_prepare_adv_monitor_suspend(&req, true);
 
                /* Prevent disconnects from causing scanning to be re-enabled */
                hdev->scanning_paused = true;
@@ -1228,7 +1231,7 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
                /* Reset passive/background scanning to normal */
                __hci_update_background_scan(&req);
                /* Enable all of the advertisement filters */
-               hci_req_add_set_adv_filter_enable(&req, true);
+               hci_req_prepare_adv_monitor_suspend(&req, false);
 
                /* Unpause directed advertising */
                hdev->advertising_paused = false;

diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index 21b1787e78933b5e824e42a9a8058c844bfd64b2..255cffa554ee5bcc487b92a010a39cb614d08bdb 100644 (file)
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -94,11 +94,14 @@ struct msft_data {
        __u16 pending_add_handle;
        __u16 pending_remove_handle;
        __u8 reregistering;
+       __u8 suspending;
        __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);
 
 bool msft_monitor_supported(struct hci_dev *hdev)
 {
@@ -154,7 +157,7 @@ failed:
 }
 
 /* This function requires the caller holds hdev->lock */
-static void reregister_monitor_on_restart(struct hci_dev *hdev, int handle)
+static void reregister_monitor(struct hci_dev *hdev, int handle)
 {
        struct adv_monitor *monitor;
        struct msft_data *msft = hdev->msft_data;
@@ -182,6 +185,69 @@ static void reregister_monitor_on_restart(struct hci_dev *hdev, int handle)
        }
 }
 
+/* This function requires the caller holds hdev->lock */
+static void remove_monitor_on_suspend(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 removed */
+                       msft->suspending = false;
+                       hci_update_background_scan(hdev);
+                       return;
+               }
+
+               msft->pending_remove_handle = (u16)handle;
+               err = __msft_remove_monitor(hdev, monitor, handle);
+
+               /* If success, return and wait for monitor removed callback */
+               if (!err)
+                       return;
+
+               /* Otherwise free the monitor and keep removing */
+               hci_free_adv_monitor(hdev, monitor);
+               handle++;
+       }
+}
+
+/* This function requires the caller holds hdev->lock */
+void msft_suspend(struct hci_dev *hdev)
+{
+       struct msft_data *msft = hdev->msft_data;
+
+       if (!msft)
+               return;
+
+       if (msft_monitor_supported(hdev)) {
+               msft->suspending = true;
+               /* Quitely remove all monitors on suspend to avoid waking up
+                * the system.
+                */
+               remove_monitor_on_suspend(hdev, 0);
+       }
+}
+
+/* This function requires the caller holds hdev->lock */
+void msft_resume(struct hci_dev *hdev)
+{
+       struct msft_data *msft = hdev->msft_data;
+
+       if (!msft)
+               return;
+
+       if (msft_monitor_supported(hdev)) {
+               msft->reregistering = true;
+               /* Monitors are removed on suspend, so we need to add all
+                * monitors on resume.
+                */
+               reregister_monitor(hdev, 0);
+       }
+}
+
 void msft_do_open(struct hci_dev *hdev)
 {
        struct msft_data *msft = hdev->msft_data;
@@ -214,7 +280,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_on_restart(hdev, 0);
+               reregister_monitor(hdev, 0);
        }
 }
 
@@ -382,8 +448,7 @@ unlock:
 
        /* If in restart/reregister sequence, keep registering. */
        if (msft->reregistering)
-               reregister_monitor_on_restart(hdev,
-                                             msft->pending_add_handle + 1);
+               reregister_monitor(hdev, msft->pending_add_handle + 1);
 
        hci_dev_unlock(hdev);
 
@@ -420,13 +485,25 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
        if (handle_data) {
                monitor = idr_find(&hdev->adv_monitors_idr,
                                   handle_data->mgmt_handle);
-               if (monitor)
+
+               if (monitor && 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)
                        hci_free_adv_monitor(hdev, monitor);
 
                list_del(&handle_data->list);
                kfree(handle_data);
        }
 
+       /* If in suspend/remove sequence, keep removing. */
+       if (msft->suspending)
+               remove_monitor_on_suspend(hdev,
+                                         msft->pending_remove_handle + 1);
+
        /* 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.
@@ -445,7 +522,8 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
        hci_dev_unlock(hdev);
 
 done:
-       hci_remove_adv_monitor_complete(hdev, status);
+       if (!msft->suspending)
+               hci_remove_adv_monitor_complete(hdev, status);
 }
 
 static void msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
@@ -578,15 +656,15 @@ int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
        if (!msft)
                return -EOPNOTSUPP;
 
-       if (msft->reregistering)
+       if (msft->reregistering || msft->suspending)
                return -EBUSY;
 
        return __msft_add_monitor_pattern(hdev, monitor);
 }
 
 /* This function requires the caller holds hdev->lock */
-int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
-                       u16 handle)
+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;
@@ -594,12 +672,6 @@ int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
        struct msft_data *msft = hdev->msft_data;
        int err = 0;
 
-       if (!msft)
-               return -EOPNOTSUPP;
-
-       if (msft->reregistering)
-               return -EBUSY;
-
        handle_data = msft_find_handle_data(hdev, monitor->handle, true);
 
        /* If no matched handle, just remove without telling controller */
@@ -619,6 +691,21 @@ int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
        return err;
 }
 
+/* This function requires the caller holds hdev->lock */
+int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
+                       u16 handle)
+{
+       struct msft_data *msft = hdev->msft_data;
+
+       if (!msft)
+               return -EOPNOTSUPP;
+
+       if (msft->reregistering || msft->suspending)
+               return -EBUSY;
+
+       return __msft_remove_monitor(hdev, monitor, handle);
+}
+
 void msft_req_add_set_filter_enable(struct hci_request *req, bool enable)
 {
        struct hci_dev *hdev = req->hdev;

diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
index 8018948c59754ec64552479b5f11df8ee6a7eafe..59c6e081c789b68799378e393804717a10c16f10 100644 (file)
--- a/net/bluetooth/msft.h
+++ b/net/bluetooth/msft.h
@@ -24,6 +24,8 @@ int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
                        u16 handle);
 void msft_req_add_set_filter_enable(struct hci_request *req, bool enable);
 int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
+void msft_suspend(struct hci_dev *hdev);
+void msft_resume(struct hci_dev *hdev);
 bool msft_curve_validity(struct hci_dev *hdev);
 
 #else
@@ -59,6 +61,9 @@ static inline int msft_set_filter_enable(struct hci_dev *hdev, bool enable)
        return -EOPNOTSUPP;
 }
 
+static inline void msft_suspend(struct hci_dev *hdev) {}
+static inline void msft_resume(struct hci_dev *hdev) {}
+
 static inline bool msft_curve_validity(struct hci_dev *hdev)
 {
        return false;