Hi Manish,
On Tue, Sep 21, 2021 at 8:57 AM Manish Mandlik <mmandlik@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> Reviewed-by: Archie Pusaka <apusaka@xxxxxxxxxx>
> Reviewed-by: Miao-chen Chou <mcchou@xxxxxxxxxx>
> ---
>
> Changes in v2:
> - Updated the Reviewd-by names
>
> net/bluetooth/hci_request.c | 15 +++--
> net/bluetooth/msft.c | 117 +++++++++++++++++++++++++++++++-----
> net/bluetooth/msft.h | 5 ++
> 3 files changed, 116 insertions(+), 21 deletions(-)
>
> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> index 47fb665277d4..c018a172ced3 100644
> --- a/net/bluetooth/hci_request.c
> +++ b/net/bluetooth/hci_request.c
> @@ -1281,21 +1281,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_remove_all_monitors_on_suspend(hdev);
> + else
> + msft_reregister_monitors_on_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);
> }
>
> @@ -1362,7 +1365,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;
> @@ -1404,7 +1407,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 21b1787e7893..328d5e341f9a 100644
> --- 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 @@ static bool read_supported_features(struct hci_dev *hdev,
> }
>
> /* 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;
Not sure I follow this one, why you are not continuing removing the
other ones? Also doesn't adv monitor have a way to clear all monitors
at once?
> +
> + /* Otherwise free the monitor and keep removing */
> + hci_free_adv_monitor(hdev, monitor);
> + handle++;
> + }
> +}
> +
> +/* This function requires the caller holds hdev->lock */
> +void msft_remove_all_monitors_on_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.
> + */
The above comment suggests it would remove all monitors, not just one.
> + remove_monitor_on_suspend(hdev, 0);
> + }
> +}
> +
> +/* This function requires the caller holds hdev->lock */
> +void msft_reregister_monitors_on_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 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
>
> /* 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 8018948c5975..6ec843b94d16 100644
> --- 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_remove_all_monitors_on_suspend(struct hci_dev *hdev);
> +void msft_reregister_monitors_on_resume(struct hci_dev *hdev);
I'd go with msft_suspend and msft_resume, that should be enough to
indicate what their intent are.
> 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;
> }
>
> +void msft_remove_all_monitors_on_suspend(struct hci_dev *hdev) {}
> +void msft_reregister_monitors_on_resume(struct hci_dev *hdev) {}
> +
> static inline bool msft_curve_validity(struct hci_dev *hdev)
> {
> return false;
> --
> 2.33.0.464.g1972c5931b-goog
>
--
Luiz Augusto von Dentz
