[PATCH 1/2] Bluetooth: msft: Clear tracked devices on resume

[PATCH 1/2] Bluetooth: msft: Clear tracked devices on resume

[Manish Mandlik]

Clear already tracked devices on system resume. Once the monitors are
reregistered after resume, matched devices in range will be found again.

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

 net/bluetooth/msft.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index 9a3d77d3ca86..f43994523b1f 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -330,12 +330,13 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
 		/* 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 (monitor && !msft->suspending) {
 			hci_free_adv_monitor(hdev, monitor);
 
-		/* Clear any monitored devices by this Adv Monitor */
-		msft_monitor_device_del(hdev, handle_data->mgmt_handle, NULL,
-					0, false);
+			/* Clear any monitored devices by this Adv Monitor */
+			msft_monitor_device_del(hdev, handle_data->mgmt_handle,
+						NULL, 0, false);
+		}
 
 		list_del(&handle_data->list);
 		kfree(handle_data);
@@ -522,6 +523,16 @@ int msft_resume_sync(struct hci_dev *hdev)
 	if (!msft || !msft_monitor_supported(hdev))
 		return 0;
 
+	hci_dev_lock(hdev);
+
+	/* Clear already tracked devices on resume. Once the monitors are
+	 * reregistered, devices in range will be found again after resume.
+	 */
+	hdev->advmon_pend_notify = false;
+	msft_monitor_device_del(hdev, 0, NULL, 0, true);
+
+	hci_dev_unlock(hdev);
+
 	msft->resuming = true;
 
 	while (1) {
-- 
2.35.1.723.g4982287a31-goog

[PATCH 2/2] Bluetooth: Send AdvMonitor Dev Found for all matched devices

[Manish Mandlik]

When an Advertisement Monitor is configured with SamplingPeriod 0xFF,
the controller reports only one adv report along with the MSFT Monitor
Device event.

When an advertiser matches multiple monitors, some controllers send one
adv report for each matched monitor; whereas, some controllers send just
one adv report for all matched monitors.

In such a case, report Adv Monitor Device Found event for each matched
monitor.

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

 net/bluetooth/mgmt.c | 70 +++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d59c70e9166f..e4da2318a2f6 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -9628,17 +9628,44 @@ void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
 		   NULL);
 }
 
+static void mgmt_send_adv_monitor_device_found(struct hci_dev *hdev,
+					       struct sk_buff *skb,
+					       struct sock *skip_sk,
+					       u16 handle)
+{
+	struct sk_buff *advmon_skb;
+	size_t advmon_skb_len;
+	__le16 *monitor_handle;
+
+	if (!skb)
+		return;
+
+	advmon_skb_len = (sizeof(struct mgmt_ev_adv_monitor_device_found) -
+			  sizeof(struct mgmt_ev_device_found)) + skb->len;
+	advmon_skb = mgmt_alloc_skb(hdev, MGMT_EV_ADV_MONITOR_DEVICE_FOUND,
+				    advmon_skb_len);
+	if (!advmon_skb)
+		return;
+
+	/* ADV_MONITOR_DEVICE_FOUND is similar to DEVICE_FOUND event except
+	 * that it also has 'monitor_handle'. Make a copy of DEVICE_FOUND and
+	 * store monitor_handle of the matched monitor.
+	 */
+	monitor_handle = skb_put(advmon_skb, sizeof(*monitor_handle));
+	*monitor_handle = cpu_to_le16(handle);
+	skb_put_data(advmon_skb, skb->data, skb->len);
+
+	mgmt_event_skb(advmon_skb, skip_sk);
+}
+
 static void mgmt_adv_monitor_device_found(struct hci_dev *hdev,
 					  bdaddr_t *bdaddr, bool report_device,
 					  struct sk_buff *skb,
 					  struct sock *skip_sk)
 {
-	struct sk_buff *advmon_skb;
-	size_t advmon_skb_len;
-	__le16 *monitor_handle;
 	struct monitored_device *dev, *tmp;
 	bool matched = false;
-	bool notify = false;
+	bool notified = false;
 
 	/* We have received the Advertisement Report because:
 	 * 1. the kernel has initiated active discovery
@@ -9660,25 +9687,6 @@ static void mgmt_adv_monitor_device_found(struct hci_dev *hdev,
 		return;
 	}
 
-	advmon_skb_len = (sizeof(struct mgmt_ev_adv_monitor_device_found) -
-			  sizeof(struct mgmt_ev_device_found)) + skb->len;
-	advmon_skb = mgmt_alloc_skb(hdev, MGMT_EV_ADV_MONITOR_DEVICE_FOUND,
-				    advmon_skb_len);
-	if (!advmon_skb) {
-		if (report_device)
-			mgmt_event_skb(skb, skip_sk);
-		else
-			kfree_skb(skb);
-		return;
-	}
-
-	/* ADV_MONITOR_DEVICE_FOUND is similar to DEVICE_FOUND event except
-	 * that it also has 'monitor_handle'. Make a copy of DEVICE_FOUND and
-	 * store monitor_handle of the matched monitor.
-	 */
-	monitor_handle = skb_put(advmon_skb, sizeof(*monitor_handle));
-	skb_put_data(advmon_skb, skb->data, skb->len);
-
 	hdev->advmon_pend_notify = false;
 
 	list_for_each_entry_safe(dev, tmp, &hdev->monitored_devices, list) {
@@ -9686,8 +9694,10 @@ static void mgmt_adv_monitor_device_found(struct hci_dev *hdev,
 			matched = true;
 
 			if (!dev->notified) {
-				*monitor_handle = cpu_to_le16(dev->handle);
-				notify = true;
+				mgmt_send_adv_monitor_device_found(hdev, skb,
+								   skip_sk,
+								   dev->handle);
+				notified = true;
 				dev->notified = true;
 			}
 		}
@@ -9697,25 +9707,19 @@ static void mgmt_adv_monitor_device_found(struct hci_dev *hdev,
 	}
 
 	if (!report_device &&
-	    ((matched && !notify) || !msft_monitor_supported(hdev))) {
+	    ((matched && !notified) || !msft_monitor_supported(hdev))) {
 		/* Handle 0 indicates that we are not active scanning and this
 		 * is a subsequent advertisement report for an already matched
 		 * Advertisement Monitor or the controller offloading support
 		 * is not available.
 		 */
-		*monitor_handle = 0;
-		notify = true;
+		mgmt_send_adv_monitor_device_found(hdev, skb, skip_sk, 0);
 	}
 
 	if (report_device)
 		mgmt_event_skb(skb, skip_sk);
 	else
 		kfree_skb(skb);
-
-	if (notify)
-		mgmt_event_skb(advmon_skb, skip_sk);
-	else
-		kfree_skb(advmon_skb);
 }
 
 void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
-- 
2.35.1.723.g4982287a31-goog

RE: [1/2] Bluetooth: msft: Clear tracked devices on resume

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=622845

---Test result---

Test Summary:
CheckPatch                    PASS      3.43 seconds
GitLint                       PASS      2.05 seconds
SubjectPrefix                 PASS      1.73 seconds
BuildKernel                   PASS      35.14 seconds
BuildKernel32                 PASS      31.60 seconds
Incremental Build with patchesPASS      52.52 seconds
TestRunner: Setup             PASS      568.49 seconds
TestRunner: l2cap-tester      PASS      17.60 seconds
TestRunner: bnep-tester       PASS      7.28 seconds
TestRunner: mgmt-tester       PASS      113.62 seconds
TestRunner: rfcomm-tester     PASS      9.33 seconds
TestRunner: sco-tester        PASS      9.00 seconds
TestRunner: smp-tester        PASS      8.92 seconds
TestRunner: userchan-tester   PASS      7.30 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH 1/2] Bluetooth: msft: Clear tracked devices on resume

[Marcel Holtmann]

Hi Manish,

> Clear already tracked devices on system resume. Once the monitors are
> reregistered after resume, matched devices in range will be found again.
> 
> Signed-off-by: Manish Mandlik <mmandlik@google.com>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
> 
> net/bluetooth/msft.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: msft: Clear tracked devices on resume

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Marcel Holtmann <marcel@holtmann.org>:

On Sat, 12 Mar 2022 02:08:58 -0800 you wrote:
> Clear already tracked devices on system resume. Once the monitors are
> reregistered after resume, matched devices in range will be found again.
> 
> Signed-off-by: Manish Mandlik <mmandlik@google.com>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
> 
> [...]

Here is the summary with links:
  - [1/2] Bluetooth: msft: Clear tracked devices on resume
    https://git.kernel.org/bluetooth/bluetooth-next/c/28c5124c1e07
  - [2/2] Bluetooth: Send AdvMonitor Dev Found for all matched devices
    https://git.kernel.org/bluetooth/bluetooth-next/c/1b144a7a0512

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 2/2] Bluetooth: Send AdvMonitor Dev Found for all matched devices

[Marcel Holtmann]

Hi Manish,

> When an Advertisement Monitor is configured with SamplingPeriod 0xFF,
> the controller reports only one adv report along with the MSFT Monitor
> Device event.
> 
> When an advertiser matches multiple monitors, some controllers send one
> adv report for each matched monitor; whereas, some controllers send just
> one adv report for all matched monitors.
> 
> In such a case, report Adv Monitor Device Found event for each matched
> monitor.
> 
> Signed-off-by: Manish Mandlik <mmandlik@google.com>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
> 
> net/bluetooth/mgmt.c | 70 +++++++++++++++++++++++---------------------
> 1 file changed, 37 insertions(+), 33 deletions(-)

patch has been applied to bluetooth-next tree.

> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index d59c70e9166f..e4da2318a2f6 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -9628,17 +9628,44 @@ void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> 		   NULL);
> }
> 
> +static void mgmt_send_adv_monitor_device_found(struct hci_dev *hdev,
> +					       struct sk_buff *skb,
> +					       struct sock *skip_sk,
> +					       u16 handle)
> +{
> +	struct sk_buff *advmon_skb;
> +	size_t advmon_skb_len;
> +	__le16 *monitor_handle;
> +
> +	if (!skb)
> +		return;
> +
> +	advmon_skb_len = (sizeof(struct mgmt_ev_adv_monitor_device_found) -
> +			  sizeof(struct mgmt_ev_device_found)) + skb->len;
> +	advmon_skb = mgmt_alloc_skb(hdev, MGMT_EV_ADV_MONITOR_DEVICE_FOUND,
> +				    advmon_skb_len);
> +	if (!advmon_skb)
> +		return;
> +
> +	/* ADV_MONITOR_DEVICE_FOUND is similar to DEVICE_FOUND event except
> +	 * that it also has 'monitor_handle'. Make a copy of DEVICE_FOUND and
> +	 * store monitor_handle of the matched monitor.
> +	 */
> +	monitor_handle = skb_put(advmon_skb, sizeof(*monitor_handle));
> +	*monitor_handle = cpu_to_le16(handle);
> +	skb_put_data(advmon_skb, skb->data, skb->len);
> +
> +	mgmt_event_skb(advmon_skb, skip_sk);
> +}
> +

However, this is rather hackish code. It will blow up in our faces at some point and we will spent weeks to find the bug.

I realized that you already got this pattern merged by Luiz and that is why I merged your patch. I would have not accepted this in the first place. Maybe you need to spent some development cycles and check how all DEVICE_FOUND events can be properly generalized.

Regards

Marcel