[RFC PATCH v2 0/4] Bluetooth: Handle system suspend gracefully

[RFC PATCH v2 0/4] Bluetooth: Handle system suspend gracefully

[Abhishek Pandit-Subedi]

Hi linux-bluetooth,

This patch series prepares the Bluetooth controller for system suspend
by disconnecting all devices and preparing the event filter and LE
whitelist with devices that can wake the system from suspend.

The main motivation for doing this is so we can enable Bluetooth as
a wake up source during suspend without it being noisy. Bluetooth should
wake the system when a HID device receives user input but otherwise not
send any events to the host.

This patch series was tested on several Chromebooks with both btusb and
hci_serdev on kernel 4.19. The set of tests was basically the following:
* Reconnects after suspend succeed
* HID devices can wake the system from suspend (needs some related bluez
  changes to call the Set Wake Capable management command)
* System properly pauses and unpauses discovery + advertising around
  suspend
* System does not wake from any events from non wakeable devices

Series 2 has refactored the change into multiple smaller commits as
requested. I tried to simplify some of the whitelist filtering edge
cases but unfortunately it remains quite complex.

Please review and provide any feedback.

Thanks
Abhishek


Changes in v2:
* Moved pm notifier registration into its own patch and moved params out
  of separate suspend_state
* Refactored filters and whitelist settings to its own patch
* Refactored update_white_list to have clearer edge cases
* Add connected devices to whitelist (previously missing corner case)
* Refactored pause discovery + advertising into its own patch

Abhishek Pandit-Subedi (4):
  Bluetooth: Add mgmt op set_wake_capable
  Bluetooth: Handle PM_SUSPEND_PREPARE and PM_POST_SUSPEND
  Bluetooth: Update filters/whitelists for suspend
  Bluetooth: Pause discovery and advertising during suspend

 include/net/bluetooth/hci.h      |  17 +-
 include/net/bluetooth/hci_core.h |  38 ++++
 include/net/bluetooth/mgmt.h     |   7 +
 net/bluetooth/hci_core.c         |  71 ++++++
 net/bluetooth/hci_event.c        |  28 ++-
 net/bluetooth/hci_request.c      | 370 ++++++++++++++++++++++++++-----
 net/bluetooth/hci_request.h      |   2 +
 net/bluetooth/mgmt.c             |  89 ++++++++
 8 files changed, 554 insertions(+), 68 deletions(-)

-- 
2.25.0.341.g760bfbb309-goog

[RFC PATCH v2 1/4] Bluetooth: Add mgmt op set_wake_capable

[Abhishek Pandit-Subedi]

When the system is suspended, only some connected Bluetooth devices
cause user input that should wake the system (mostly HID devices). Add
a list to keep track of devices that can wake the system and add
a management API to let userspace tell the kernel whether a device is
wake capable or not.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2: None

 include/net/bluetooth/hci_core.h |  1 +
 include/net/bluetooth/mgmt.h     |  7 ++++++
 net/bluetooth/hci_core.c         |  1 +
 net/bluetooth/mgmt.c             | 40 ++++++++++++++++++++++++++++++++
 4 files changed, 49 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 89ecf0a80aa1..ce4bebcb0265 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -394,6 +394,7 @@ struct hci_dev {
 	struct list_head	mgmt_pending;
 	struct list_head	blacklist;
 	struct list_head	whitelist;
+	struct list_head	wakeable;
 	struct list_head	uuids;
 	struct list_head	link_keys;
 	struct list_head	long_term_keys;
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index a90666af05bd..283ba5320bdb 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -671,6 +671,13 @@ struct mgmt_cp_set_blocked_keys {
 } __packed;
 #define MGMT_OP_SET_BLOCKED_KEYS_SIZE 2
 
+#define MGMT_OP_SET_WAKE_CAPABLE	0x0047
+#define MGMT_SET_WAKE_CAPABLE_SIZE	8
+struct mgmt_cp_set_wake_capable {
+	struct mgmt_addr_info addr;
+	u8 wake_capable;
+} __packed;
+
 #define MGMT_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
 	__le16	opcode;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index cbbc34a006d1..2fceaf76644a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3299,6 +3299,7 @@ struct hci_dev *hci_alloc_dev(void)
 	INIT_LIST_HEAD(&hdev->mgmt_pending);
 	INIT_LIST_HEAD(&hdev->blacklist);
 	INIT_LIST_HEAD(&hdev->whitelist);
+	INIT_LIST_HEAD(&hdev->wakeable);
 	INIT_LIST_HEAD(&hdev->uuids);
 	INIT_LIST_HEAD(&hdev->link_keys);
 	INIT_LIST_HEAD(&hdev->long_term_keys);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3074363c68df..58468dfa112f 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -107,6 +107,7 @@ static const u16 mgmt_commands[] = {
 	MGMT_OP_READ_EXT_INFO,
 	MGMT_OP_SET_APPEARANCE,
 	MGMT_OP_SET_BLOCKED_KEYS,
+	MGMT_OP_SET_WAKE_CAPABLE,
 };
 
 static const u16 mgmt_events[] = {
@@ -4663,6 +4664,37 @@ static int set_fast_connectable(struct sock *sk, struct hci_dev *hdev,
 	return err;
 }
 
+static int set_wake_capable(struct sock *sk, struct hci_dev *hdev, void *data,
+			    u16 len)
+{
+	int err;
+	u8 status;
+	struct mgmt_cp_set_wake_capable *cp = data;
+	u8 addr_type = cp->addr.type == BDADDR_BREDR ?
+			       cp->addr.type :
+			       le_addr_type(cp->addr.type);
+
+	BT_DBG("Set wake capable %pMR (type 0x%x) = 0x%x\n", &cp->addr.bdaddr,
+	       addr_type, cp->wake_capable);
+
+	if (cp->wake_capable)
+		err = hci_bdaddr_list_add(&hdev->wakeable, &cp->addr.bdaddr,
+					  addr_type);
+	else
+		err = hci_bdaddr_list_del(&hdev->wakeable, &cp->addr.bdaddr,
+					  addr_type);
+
+	if (!err || err == -EEXIST || err == -ENOENT)
+		status = MGMT_STATUS_SUCCESS;
+	else
+		status = MGMT_STATUS_FAILED;
+
+	err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_WAKE_CAPABLE, status,
+				cp, sizeof(*cp));
+
+	return err;
+}
+
 static void set_bredr_complete(struct hci_dev *hdev, u8 status, u16 opcode)
 {
 	struct mgmt_pending_cmd *cmd;
@@ -5791,6 +5823,13 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
 			err = hci_bdaddr_list_del(&hdev->whitelist,
 						  &cp->addr.bdaddr,
 						  cp->addr.type);
+
+			/* Don't check result since it either succeeds or device
+			 * wasn't there (not wakeable or invalid params as
+			 * covered by deleting from whitelist).
+			 */
+			hci_bdaddr_list_del(&hdev->wakeable, &cp->addr.bdaddr,
+					    cp->addr.type);
 			if (err) {
 				err = mgmt_cmd_complete(sk, hdev->id,
 							MGMT_OP_REMOVE_DEVICE,
@@ -6990,6 +7029,7 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
 	{ set_phy_configuration,   MGMT_SET_PHY_CONFIGURATION_SIZE },
 	{ set_blocked_keys,	   MGMT_OP_SET_BLOCKED_KEYS_SIZE,
 						HCI_MGMT_VAR_LEN },
+	{ set_wake_capable,	   MGMT_SET_WAKE_CAPABLE_SIZE },
 };
 
 void mgmt_index_added(struct hci_dev *hdev)
-- 
2.25.0.341.g760bfbb309-goog

Re: [RFC PATCH v2 1/4] Bluetooth: Add mgmt op set_wake_capable

[Marcel Holtmann]

Hi Abhishek.

> When the system is suspended, only some connected Bluetooth devices
> cause user input that should wake the system (mostly HID devices). Add
> a list to keep track of devices that can wake the system and add
> a management API to let userspace tell the kernel whether a device is
> wake capable or not.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v2: None
> 
> include/net/bluetooth/hci_core.h |  1 +
> include/net/bluetooth/mgmt.h     |  7 ++++++
> net/bluetooth/hci_core.c         |  1 +
> net/bluetooth/mgmt.c             | 40 ++++++++++++++++++++++++++++++++
> 4 files changed, 49 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 89ecf0a80aa1..ce4bebcb0265 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -394,6 +394,7 @@ struct hci_dev {
> 	struct list_head	mgmt_pending;
> 	struct list_head	blacklist;
> 	struct list_head	whitelist;
> +	struct list_head	wakeable;

I have the feeling that using a separate list is making this more complicated for us than it needs to be. I think in parts this comes through the fact that BR/EDR devices are handled via the whitelist list and for LE devices we are using the conn params “framework”.

So maybe it is actually better to store in wakeable list just the BR/EDR devices. And ensure that wakeable is a subset of the whitelist list. And for LE we store it in the conn params lists.

At some point, I think we need to merge whitelist and wakeable list for BR/EDR into a proper structure that can hold this information and maybe also revamp the inquiry cache we are using.

I will comment on the rest when I go through the LE whitelist update patch (and yes, I realize or variable whitelist for BR/EDR is confusing).

Regards

Marcel

[RFC PATCH v2 2/4] Bluetooth: Handle PM_SUSPEND_PREPARE and PM_POST_SUSPEND

[Abhishek Pandit-Subedi]

Register for PM_SUSPEND_PREPARE and PM_POST_SUSPEND to make sure the
Bluetooth controller is prepared correctly for suspend/resume. Implement
the registration, scheduling and task handling portions only in this
patch.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2:
* Moved pm notifier registration into its own patch and moved params out
  of separate suspend_state

 include/net/bluetooth/hci_core.h | 21 ++++++++++
 net/bluetooth/hci_core.c         | 70 ++++++++++++++++++++++++++++++++
 net/bluetooth/hci_request.c      | 19 +++++++++
 net/bluetooth/hci_request.h      |  2 +
 4 files changed, 112 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index ce4bebcb0265..74d703e46fb4 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -88,6 +88,18 @@ struct discovery_state {
 	unsigned long		scan_duration;
 };
 
+#define SUSPEND_NOTIFIER_TIMEOUT	msecs_to_jiffies(2000) /* 2 seconds */
+
+enum suspend_tasks {
+	SUSPEND_PREPARE_NOTIFIER,
+	__SUSPEND_NUM_TASKS
+};
+
+enum suspended_state {
+	BT_RUNNING = 0,
+	BT_SUSPENDED,
+};
+
 struct hci_conn_hash {
 	struct list_head list;
 	unsigned int     acl_num;
@@ -389,6 +401,15 @@ struct hci_dev {
 	void			*smp_bredr_data;
 
 	struct discovery_state	discovery;
+
+	struct notifier_block	suspend_notifier;
+	struct work_struct	suspend_prepare;
+	enum suspended_state	suspend_state_next;
+	enum suspended_state	suspend_state;
+
+	wait_queue_head_t	suspend_wait_q;
+	DECLARE_BITMAP(suspend_tasks, __SUSPEND_NUM_TASKS);
+
 	struct hci_conn_hash	conn_hash;
 
 	struct list_head	mgmt_pending;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 2fceaf76644a..6ca0aa8d30dd 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -31,6 +31,8 @@
 #include <linux/debugfs.h>
 #include <linux/crypto.h>
 #include <linux/property.h>
+#include <linux/suspend.h>
+#include <linux/wait.h>
 #include <asm/unaligned.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -3241,6 +3243,65 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
 	}
 }
 
+static int hci_suspend_wait_event(struct hci_dev *hdev)
+{
+#define WAKE_COND                                                              \
+	(find_first_bit(hdev->suspend_tasks, __SUSPEND_NUM_TASKS) ==           \
+	 __SUSPEND_NUM_TASKS)
+
+	int i;
+	int ret = wait_event_timeout(hdev->suspend_wait_q,
+				     WAKE_COND, SUSPEND_NOTIFIER_TIMEOUT);
+
+	if (ret == 0) {
+		BT_DBG("Timed out waiting for suspend");
+		for (i = 0; i < __SUSPEND_NUM_TASKS; ++i) {
+			if (test_bit(i, hdev->suspend_tasks))
+				BT_DBG("Bit %d is set", i);
+			clear_bit(i, hdev->suspend_tasks);
+		}
+
+		ret = -ETIMEDOUT;
+	} else {
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static void hci_prepare_suspend(struct work_struct *work)
+{
+	struct hci_dev *hdev =
+		container_of(work, struct hci_dev, suspend_prepare);
+
+	hci_dev_lock(hdev);
+	hci_req_prepare_suspend(hdev, hdev->suspend_state_next);
+	hci_dev_unlock(hdev);
+}
+
+static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
+				void *data)
+{
+	struct hci_dev *hdev =
+		container_of(nb, struct hci_dev, suspend_notifier);
+	int ret = 0;
+
+	if (action == PM_SUSPEND_PREPARE) {
+		hdev->suspend_state_next = BT_SUSPENDED;
+		set_bit(SUSPEND_PREPARE_NOTIFIER, hdev->suspend_tasks);
+		queue_work(hdev->req_workqueue, &hdev->suspend_prepare);
+
+		ret = hci_suspend_wait_event(hdev);
+	} else if (action == PM_POST_SUSPEND) {
+		hdev->suspend_state_next = BT_RUNNING;
+		set_bit(SUSPEND_PREPARE_NOTIFIER, hdev->suspend_tasks);
+		queue_work(hdev->req_workqueue, &hdev->suspend_prepare);
+
+		ret = hci_suspend_wait_event(hdev);
+	}
+
+	return ret ? notifier_from_errno(-EBUSY) : NOTIFY_STOP;
+}
 /* Alloc HCI device */
 struct hci_dev *hci_alloc_dev(void)
 {
@@ -3319,6 +3380,7 @@ struct hci_dev *hci_alloc_dev(void)
 	INIT_WORK(&hdev->tx_work, hci_tx_work);
 	INIT_WORK(&hdev->power_on, hci_power_on);
 	INIT_WORK(&hdev->error_reset, hci_error_reset);
+	INIT_WORK(&hdev->suspend_prepare, hci_prepare_suspend);
 
 	INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
 
@@ -3327,6 +3389,7 @@ struct hci_dev *hci_alloc_dev(void)
 	skb_queue_head_init(&hdev->raw_q);
 
 	init_waitqueue_head(&hdev->req_wait_q);
+	init_waitqueue_head(&hdev->suspend_wait_q);
 
 	INIT_DELAYED_WORK(&hdev->cmd_timer, hci_cmd_timeout);
 
@@ -3438,6 +3501,11 @@ int hci_register_dev(struct hci_dev *hdev)
 	hci_sock_dev_event(hdev, HCI_DEV_REG);
 	hci_dev_hold(hdev);
 
+	hdev->suspend_notifier.notifier_call = hci_suspend_notifier;
+	error = register_pm_notifier(&hdev->suspend_notifier);
+	if (error)
+		goto err_wqueue;
+
 	queue_work(hdev->req_workqueue, &hdev->power_on);
 
 	return id;
@@ -3471,6 +3539,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
 
 	hci_dev_do_close(hdev);
 
+	unregister_pm_notifier(&hdev->suspend_notifier);
+
 	if (!test_bit(HCI_INIT, &hdev->flags) &&
 	    !hci_dev_test_flag(hdev, HCI_SETUP) &&
 	    !hci_dev_test_flag(hdev, HCI_CONFIG)) {
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 2a1b64dbf76e..08908469c043 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -918,6 +918,25 @@ static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
 	return adv_instance->scan_rsp_len;
 }
 
+/* Call with hci_dev_lock */
+void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
+{
+	int old_state;
+	struct hci_conn *conn;
+	struct hci_request req;
+
+	if (next == hdev->suspend_state) {
+		BT_DBG("Same state before and after: %d", next);
+		goto done;
+	}
+
+	hdev->suspend_state = next;
+
+done:
+	clear_bit(SUSPEND_PREPARE_NOTIFIER, hdev->suspend_tasks);
+	wake_up(&hdev->suspend_wait_q);
+}
+
 static u8 get_cur_adv_instance_scan_rsp_len(struct hci_dev *hdev)
 {
 	u8 instance = hdev->cur_adv_instance;
diff --git a/net/bluetooth/hci_request.h b/net/bluetooth/hci_request.h
index a7019fbeadd3..0e81614d235e 100644
--- a/net/bluetooth/hci_request.h
+++ b/net/bluetooth/hci_request.h
@@ -68,6 +68,8 @@ void __hci_req_update_eir(struct hci_request *req);
 void hci_req_add_le_scan_disable(struct hci_request *req);
 void hci_req_add_le_passive_scan(struct hci_request *req);
 
+void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next);
+
 void hci_req_reenable_advertising(struct hci_dev *hdev);
 void __hci_req_enable_advertising(struct hci_request *req);
 void __hci_req_disable_advertising(struct hci_request *req);
-- 
2.25.0.341.g760bfbb309-goog

[RFC PATCH v2 3/4] Bluetooth: Update filters/whitelists for suspend

[Abhishek Pandit-Subedi]

When suspending, update the event filter for BR/EDR devices and the
whitelist for LE devices. BR/EDR devices are added to the event filter
and will auto-connect if found during suspend. For LE, we update the
filter to remove everything that is not wakeable during suspend.
Finally, we disconnect all connected devices and wait for that to
complete before returning in the suspend notifier.

An example suspend flow with 1 BR/EDR HID device, 1 BR/EDR audio device,
1 LE HID device, 1 LE non-HID (where HIDs are wakeable):

PM_PREPARE_SUSPEND:
  - Set event filter for BR/EDR HID device
  - Clear anything from LE whitelist that is not wakeable
  - Add (if not there) the LE HID device
  - Disconnect all devices

PM_POST_SUSPEND:
  - Clear event filter
  - Restore LE whitelist (deleting anything not in le_pend_conn or
    le_pend_report and then adding from those lists)

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2:
* Refactored filters and whitelist settings to its own patch
* Refactored update_white_list to have clearer edge cases
* Add connected devices to whitelist (previously missing corner case)

 include/net/bluetooth/hci.h      |  17 +-
 include/net/bluetooth/hci_core.h |   5 +
 net/bluetooth/hci_event.c        |  28 ++-
 net/bluetooth/hci_request.c      | 308 +++++++++++++++++++++++++------
 net/bluetooth/mgmt.c             |   8 +
 5 files changed, 298 insertions(+), 68 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 6293bdd7d862..720d8e633f7e 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -932,10 +932,14 @@ struct hci_cp_sniff_subrate {
 #define HCI_OP_RESET			0x0c03
 
 #define HCI_OP_SET_EVENT_FLT		0x0c05
-struct hci_cp_set_event_flt {
-	__u8     flt_type;
-	__u8     cond_type;
-	__u8     condition[0];
+#define HCI_SET_EVENT_FLT_SIZE		9
+struct hci_cp_set_event_filter {
+	__u8		flt_type;
+	__u8		cond_type;
+	struct {
+		bdaddr_t bdaddr;
+		__u8 auto_accept;
+	} __packed	addr_conn_flt;
 } __packed;
 
 /* Filter types */
@@ -949,8 +953,9 @@ struct hci_cp_set_event_flt {
 #define HCI_CONN_SETUP_ALLOW_BDADDR	0x02
 
 /* CONN_SETUP Conditions */
-#define HCI_CONN_SETUP_AUTO_OFF	0x01
-#define HCI_CONN_SETUP_AUTO_ON	0x02
+#define HCI_CONN_SETUP_AUTO_OFF		0x01
+#define HCI_CONN_SETUP_AUTO_ON		0x02
+#define HCI_CONN_SETUP_AUTO_ON_WITH_RS	0x03
 
 #define HCI_OP_READ_STORED_LINK_KEY	0x0c0d
 struct hci_cp_read_stored_link_key {
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 74d703e46fb4..49eae4a802ac 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -91,6 +91,9 @@ struct discovery_state {
 #define SUSPEND_NOTIFIER_TIMEOUT	msecs_to_jiffies(2000) /* 2 seconds */
 
 enum suspend_tasks {
+	SUSPEND_LE_SET_SCAN_ENABLE,
+	SUSPEND_DISCONNECTING,
+
 	SUSPEND_PREPARE_NOTIFIER,
 	__SUSPEND_NUM_TASKS
 };
@@ -406,6 +409,8 @@ struct hci_dev {
 	struct work_struct	suspend_prepare;
 	enum suspended_state	suspend_state_next;
 	enum suspended_state	suspend_state;
+	int			disconnect_counter;
+	bool			freeze_filters;
 
 	wait_queue_head_t	suspend_wait_q;
 	DECLARE_BITMAP(suspend_tasks, __SUSPEND_NUM_TASKS);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6ddc4a74a5e4..76d25b3f4c73 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2474,6 +2474,7 @@ static void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
 static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_conn_complete *ev = (void *) skb->data;
+	struct inquiry_entry *ie;
 	struct hci_conn *conn;
 
 	BT_DBG("%s", hdev->name);
@@ -2482,14 +2483,29 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
 	if (!conn) {
-		if (ev->link_type != SCO_LINK)
-			goto unlock;
+		/* Connection may not exist if auto-connected. Check the inquiry
+		 * cache to see if we've already discovered this bdaddr before.
+		 * Create a new connection if it was previously discovered.
+		 */
+		ie = hci_inquiry_cache_lookup(hdev, &ev->bdaddr);
+		if (ie) {
+			conn = hci_conn_add(hdev, ev->link_type, &ev->bdaddr,
+					    HCI_ROLE_SLAVE);
+			if (!conn) {
+				bt_dev_err(hdev, "no memory for new conn");
+				goto unlock;
+			}
+		} else {
+			if (ev->link_type != SCO_LINK)
+				goto unlock;
 
-		conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK, &ev->bdaddr);
-		if (!conn)
-			goto unlock;
+			conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK,
+						       &ev->bdaddr);
+			if (!conn)
+				goto unlock;
 
-		conn->type = SCO_LINK;
+			conn->type = SCO_LINK;
+		}
 	}
 
 	if (!ev->status) {
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 08908469c043..c930b9ff1cfd 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -34,6 +34,12 @@
 #define HCI_REQ_PEND	  1
 #define HCI_REQ_CANCELED  2
 
+#define LE_SCAN_FLAG_SUSPEND	0x1
+#define LE_SCAN_FLAG_ALLOW_RPA	0x2
+
+#define LE_SUSPEND_SCAN_WINDOW		0x0012
+#define LE_SUSPEND_SCAN_INTERVAL	0x0060
+
 void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
 {
 	skb_queue_head_init(&req->cmd_q);
@@ -654,6 +660,12 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
 {
 	struct hci_dev *hdev = req->hdev;
 
+	/* Early exit if we've frozen filters for suspend*/
+	if (hdev->freeze_filters) {
+		BT_DBG("Filters are frozen for suspend");
+		return;
+	}
+
 	if (use_ext_scan(hdev)) {
 		struct hci_cp_le_set_ext_scan_enable cp;
 
@@ -670,23 +682,67 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
 	}
 }
 
-static void add_to_white_list(struct hci_request *req,
-			      struct hci_conn_params *params)
+static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr,
+				u8 bdaddr_type)
+{
+	struct hci_cp_le_del_from_white_list cp;
+
+	cp.bdaddr_type = bdaddr_type;
+	bacpy(&cp.bdaddr, bdaddr);
+
+	BT_DBG("Remove %pMR (0x%x) from whitelist", &cp.bdaddr, cp.bdaddr_type);
+	hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp);
+}
+
+/* Adds connection to white list if needed. On error, returns -1 */
+static int add_to_white_list(struct hci_request *req,
+			     struct hci_conn_params *params, u8 *num_entries,
+			     u8 flags)
 {
 	struct hci_cp_le_add_to_white_list cp;
+	struct hci_dev *hdev = req->hdev;
+	bool allow_rpa = !!(flags & LE_SCAN_FLAG_ALLOW_RPA);
+	bool suspend = !!(flags & LE_SCAN_FLAG_SUSPEND);
 
+	/* Already in white list */
+	if (hci_bdaddr_list_lookup(&hdev->le_white_list, &params->addr,
+				   params->addr_type))
+		return 0;
+
+	/* Select filter policy to accept all advertising */
+	if (*num_entries >= hdev->le_white_list_size)
+		return -1;
+
+	/* White list can not be used with RPAs */
+	if (!allow_rpa &&
+	    hci_find_irk_by_addr(hdev, &params->addr, params->addr_type)) {
+		return -1;
+	}
+
+	/* During suspend, only wakeable devices can be in whitelist */
+	if (suspend && !hci_bdaddr_list_lookup(&hdev->wakeable, &params->addr,
+					       params->addr_type))
+		return 0;
+
+	*num_entries += 1;
 	cp.bdaddr_type = params->addr_type;
 	bacpy(&cp.bdaddr, &params->addr);
 
+	BT_DBG("Add %pMR (0x%x) to whitelist", &cp.bdaddr, cp.bdaddr_type);
 	hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp);
+
+	return 0;
 }
 
-static u8 update_white_list(struct hci_request *req)
+static u8 update_white_list(struct hci_request *req, u8 flags)
 {
 	struct hci_dev *hdev = req->hdev;
 	struct hci_conn_params *params;
 	struct bdaddr_list *b;
-	uint8_t white_list_entries = 0;
+	u8 white_list_entries = 0;
+	bool allow_rpa = !!(flags & LE_SCAN_FLAG_ALLOW_RPA);
+	bool suspend = !!(flags & LE_SCAN_FLAG_SUSPEND);
+	bool wakeable, pend_conn, pend_report;
 
 	/* Go through the current white list programmed into the
 	 * controller one by one and check if that address is still
@@ -695,26 +751,42 @@ static u8 update_white_list(struct hci_request *req)
 	 * command to remove it from the controller.
 	 */
 	list_for_each_entry(b, &hdev->le_white_list, list) {
-		/* If the device is neither in pend_le_conns nor
-		 * pend_le_reports then remove it from the whitelist.
+		wakeable = !!hci_bdaddr_list_lookup(&hdev->wakeable, &b->bdaddr,
+						    b->bdaddr_type);
+		pend_conn = hci_pend_le_action_lookup(&hdev->pend_le_conns,
+						      &b->bdaddr,
+						      b->bdaddr_type);
+		pend_report = hci_pend_le_action_lookup(&hdev->pend_le_reports,
+							&b->bdaddr,
+							b->bdaddr_type);
+
+		/* During suspend, we remove all non-wakeable devices
+		 * and leave all others alone. Connected devices will be
+		 * disconnected during suspend but may not be in the pending
+		 * list yet.
 		 */
-		if (!hci_pend_le_action_lookup(&hdev->pend_le_conns,
-					       &b->bdaddr, b->bdaddr_type) &&
-		    !hci_pend_le_action_lookup(&hdev->pend_le_reports,
-					       &b->bdaddr, b->bdaddr_type)) {
-			struct hci_cp_le_del_from_white_list cp;
-
-			cp.bdaddr_type = b->bdaddr_type;
-			bacpy(&cp.bdaddr, &b->bdaddr);
-
-			hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST,
-				    sizeof(cp), &cp);
-			continue;
-		}
+		if (suspend) {
+			if (!wakeable) {
+				del_from_white_list(req, &b->bdaddr,
+						    b->bdaddr_type);
+				continue;
+			}
+		} else {
+			/* If the device is not likely to connect or report,
+			 * remove it from the whitelist.
+			 */
+			if (!pend_conn && !pend_report) {
+				del_from_white_list(req, &b->bdaddr,
+						    b->bdaddr_type);
+				continue;
+			}
 
-		if (hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) {
 			/* White list can not be used with RPAs */
-			return 0x00;
+			if (!allow_rpa &&
+			    hci_find_irk_by_addr(hdev, &b->bdaddr,
+						 b->bdaddr_type)) {
+				return 0x00;
+			}
 		}
 
 		white_list_entries++;
@@ -731,47 +803,30 @@ static u8 update_white_list(struct hci_request *req)
 	 * white list.
 	 */
 	list_for_each_entry(params, &hdev->pend_le_conns, action) {
-		if (hci_bdaddr_list_lookup(&hdev->le_white_list,
-					   &params->addr, params->addr_type))
-			continue;
-
-		if (white_list_entries >= hdev->le_white_list_size) {
-			/* Select filter policy to accept all advertising */
+		if (add_to_white_list(req, params, &white_list_entries, flags))
 			return 0x00;
-		}
-
-		if (hci_find_irk_by_addr(hdev, &params->addr,
-					 params->addr_type)) {
-			/* White list can not be used with RPAs */
-			return 0x00;
-		}
-
-		white_list_entries++;
-		add_to_white_list(req, params);
 	}
 
 	/* After adding all new pending connections, walk through
 	 * the list of pending reports and also add these to the
-	 * white list if there is still space.
+	 * white list if there is still space. Abort if space runs out.
 	 */
 	list_for_each_entry(params, &hdev->pend_le_reports, action) {
-		if (hci_bdaddr_list_lookup(&hdev->le_white_list,
-					   &params->addr, params->addr_type))
-			continue;
-
-		if (white_list_entries >= hdev->le_white_list_size) {
-			/* Select filter policy to accept all advertising */
+		if (add_to_white_list(req, params, &white_list_entries, flags))
 			return 0x00;
-		}
+	}
 
-		if (hci_find_irk_by_addr(hdev, &params->addr,
-					 params->addr_type)) {
-			/* White list can not be used with RPAs */
-			return 0x00;
+	/* Currently connected devices will be missing from the white list and
+	 * we need to insert them into the whitelist if they are wakeable. We
+	 * can't insert later because we will have already returned from the
+	 * suspend notifier and would cause a spurious wakeup.
+	 */
+	if (suspend) {
+		list_for_each_entry(params, &hdev->le_conn_params, list) {
+			if (add_to_white_list(req, params, &white_list_entries,
+					      flags))
+				return 0x00;
 		}
-
-		white_list_entries++;
-		add_to_white_list(req, params);
 	}
 
 	/* Select filter policy to use white list */
@@ -861,11 +916,26 @@ static void hci_req_start_scan(struct hci_request *req, u8 type, u16 interval,
 	}
 }
 
-void hci_req_add_le_passive_scan(struct hci_request *req)
+void __hci_req_add_le_passive_scan(struct hci_request *req, u8 flags)
 {
 	struct hci_dev *hdev = req->hdev;
 	u8 own_addr_type;
 	u8 filter_policy;
+	u8 window, interval;
+
+	/* We allow whitelisting even with RPAs in suspend. In the worst case,
+	 * we won't be able to wake from devices that use the privacy1.2
+	 * features. Additionally, once we support privacy1.2 and IRK
+	 * offloading, we can update this to also check for those conditions.
+	 */
+	if (flags & LE_SCAN_FLAG_SUSPEND)
+		flags |= LE_SCAN_FLAG_ALLOW_RPA;
+
+	/* Early exit if we've frozen filters for suspend */
+	if (hdev->freeze_filters) {
+		BT_DBG("Filters are frozen for suspend");
+		return;
+	}
 
 	/* Set require_privacy to false since no SCAN_REQ are send
 	 * during passive scanning. Not using an non-resolvable address
@@ -881,7 +951,8 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
 	 * happen before enabling scanning. The controller does
 	 * not allow white list modification while scanning.
 	 */
-	filter_policy = update_white_list(req);
+	BT_DBG("Updating white list with flags = %d", flags);
+	filter_policy = update_white_list(req, flags);
 
 	/* When the controller is using random resolvable addresses and
 	 * with that having LE privacy enabled, then controllers with
@@ -896,8 +967,22 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
 	    (hdev->le_features[0] & HCI_LE_EXT_SCAN_POLICY))
 		filter_policy |= 0x02;
 
-	hci_req_start_scan(req, LE_SCAN_PASSIVE, hdev->le_scan_interval,
-			   hdev->le_scan_window, own_addr_type, filter_policy);
+	if (flags & LE_SCAN_FLAG_SUSPEND) {
+		window = LE_SUSPEND_SCAN_WINDOW;
+		interval = LE_SUSPEND_SCAN_INTERVAL;
+	} else {
+		window = hdev->le_scan_window;
+		interval = hdev->le_scan_interval;
+	}
+
+	BT_DBG("LE passive scan with whitelist = %d", filter_policy);
+	hci_req_start_scan(req, LE_SCAN_PASSIVE, interval, window,
+			   own_addr_type, filter_policy);
+}
+
+void hci_req_add_le_passive_scan(struct hci_request *req)
+{
+	__hci_req_add_le_passive_scan(req, 0);
 }
 
 static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
@@ -918,6 +1003,76 @@ static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
 	return adv_instance->scan_rsp_len;
 }
 
+static void hci_req_clear_event_filter(struct hci_request *req)
+{
+	struct hci_cp_set_event_filter f;
+
+	memset(&f, 0, sizeof(f));
+	f.flt_type = HCI_FLT_CLEAR_ALL;
+	hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &f);
+
+	/* Update page scan state (since we may have modified it when setting
+	 * the event filter).
+	 */
+	__hci_req_update_scan(req);
+}
+
+static void hci_req_set_event_filter(struct hci_request *req)
+{
+	struct bdaddr_list *b;
+	struct hci_cp_set_event_filter f;
+	struct hci_dev *hdev = req->hdev;
+	int filters_updated = 0;
+	u8 scan;
+
+	/* Always clear event filter when starting */
+	hci_req_clear_event_filter(req);
+
+	list_for_each_entry(b, &hdev->wakeable, list) {
+		if (b->bdaddr_type != BDADDR_BREDR)
+			continue;
+
+		memset(&f, 0, sizeof(f));
+		bacpy(&f.addr_conn_flt.bdaddr, &b->bdaddr);
+		f.flt_type = HCI_FLT_CONN_SETUP;
+		f.cond_type = HCI_CONN_SETUP_ALLOW_BDADDR;
+		f.addr_conn_flt.auto_accept = HCI_CONN_SETUP_AUTO_ON;
+
+		BT_DBG("Adding event filters for %pMR", &b->bdaddr);
+		hci_req_add(req, HCI_OP_SET_EVENT_FLT, sizeof(f), &f);
+
+		filters_updated++;
+	}
+
+	scan = filters_updated ? SCAN_PAGE : SCAN_DISABLED;
+	hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
+}
+
+static void hci_req_enable_le_suspend_scan(struct hci_request *req,
+					   u8 flags)
+{
+	/* Can't change params without disabling first */
+	hci_req_add_le_scan_disable(req);
+
+	/* Configure params and enable scanning */
+	__hci_req_add_le_passive_scan(req, flags);
+
+	/* Block suspend notifier on response */
+	set_bit(SUSPEND_LE_SET_SCAN_ENABLE, req->hdev->suspend_tasks);
+}
+
+static void le_suspend_req_complete(struct hci_dev *hdev, u8 status, u16 opcode)
+{
+	BT_DBG("Request complete opcode=0x%x, status=0x%x", opcode, status);
+
+	/* Expecting LE Set scan to return */
+	if (opcode == HCI_OP_LE_SET_SCAN_ENABLE &&
+	    test_and_clear_bit(SUSPEND_LE_SET_SCAN_ENABLE,
+			       hdev->suspend_tasks)) {
+		wake_up(&hdev->suspend_wait_q);
+	}
+}
+
 /* Call with hci_dev_lock */
 void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
 {
@@ -932,6 +1087,44 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
 
 	hdev->suspend_state = next;
 
+	hci_req_init(&req, hdev);
+	if (next == BT_SUSPENDED) {
+		/* Enable event filter for existing devices */
+		hci_req_set_event_filter(&req);
+
+		/* Enable passive scan at lower duty cycle */
+		hci_req_enable_le_suspend_scan(&req, LE_SCAN_FLAG_SUSPEND);
+
+		hdev->freeze_filters = true;
+
+		/* Run commands before disconnecting */
+		hci_req_run(&req, le_suspend_req_complete);
+
+		hdev->disconnect_counter = 0;
+		/* Soft disconnect everything (power off)*/
+		list_for_each_entry(conn, &hdev->conn_hash.list, list) {
+			hci_disconnect(conn, HCI_ERROR_REMOTE_POWER_OFF);
+			hdev->disconnect_counter++;
+		}
+
+		if (hdev->disconnect_counter > 0) {
+			BT_DBG("Had %d disconnects. Will wait on them",
+			       hdev->disconnect_counter);
+			set_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks);
+		}
+	} else {
+		hdev->freeze_filters = false;
+
+		hci_req_clear_event_filter(&req);
+
+		/* Reset passive/background scanning to normal */
+		hci_req_enable_le_suspend_scan(&req, 0);
+
+		hci_req_run(&req, le_suspend_req_complete);
+	}
+
+	hdev->suspend_state = next;
+
 done:
 	clear_bit(SUSPEND_PREPARE_NOTIFIER, hdev->suspend_tasks);
 	wake_up(&hdev->suspend_wait_q);
@@ -2034,6 +2227,9 @@ void __hci_req_update_scan(struct hci_request *req)
 	if (mgmt_powering_down(hdev))
 		return;
 
+	if (hdev->freeze_filters)
+		return;
+
 	if (hci_dev_test_flag(hdev, HCI_CONNECTABLE) ||
 	    disconnected_whitelist_entries(hdev))
 		scan = SCAN_PAGE;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 58468dfa112f..269ce70e501c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -7451,6 +7451,14 @@ void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
 
 	mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, &ev, sizeof(ev), sk);
 
+	if (hdev->disconnect_counter > 0) {
+		hdev->disconnect_counter--;
+		if (hdev->disconnect_counter <= 0) {
+			clear_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks);
+			wake_up(&hdev->suspend_wait_q);
+		}
+	}
+
 	if (sk)
 		sock_put(sk);
 
-- 
2.25.0.341.g760bfbb309-goog

Re: [RFC PATCH v2 3/4] Bluetooth: Update filters/whitelists for suspend

[Marcel Holtmann]

Hi Abhishek,

> When suspending, update the event filter for BR/EDR devices and the
> whitelist for LE devices. BR/EDR devices are added to the event filter
> and will auto-connect if found during suspend. For LE, we update the
> filter to remove everything that is not wakeable during suspend.
> Finally, we disconnect all connected devices and wait for that to
> complete before returning in the suspend notifier.
> 
> An example suspend flow with 1 BR/EDR HID device, 1 BR/EDR audio device,
> 1 LE HID device, 1 LE non-HID (where HIDs are wakeable):
> 
> PM_PREPARE_SUSPEND:
>  - Set event filter for BR/EDR HID device
>  - Clear anything from LE whitelist that is not wakeable
>  - Add (if not there) the LE HID device
>  - Disconnect all devices

is this really the right order? Why not disconnect all devices first. We are suspending after all, so lets disconnect first and then create the appropriate accept filters.

I have the feeling it would also make the overall logic a lot simpler since it fits more into the current model that is used internally for a lot of decision points (especially with LE).

> PM_POST_SUSPEND:
>  - Clear event filter
>  - Restore LE whitelist (deleting anything not in le_pend_conn or
>    le_pend_report and then adding from those lists)
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v2:
> * Refactored filters and whitelist settings to its own patch
> * Refactored update_white_list to have clearer edge cases
> * Add connected devices to whitelist (previously missing corner case)
> 
> include/net/bluetooth/hci.h      |  17 +-
> include/net/bluetooth/hci_core.h |   5 +
> net/bluetooth/hci_event.c        |  28 ++-
> net/bluetooth/hci_request.c      | 308 +++++++++++++++++++++++++------
> net/bluetooth/mgmt.c             |   8 +
> 5 files changed, 298 insertions(+), 68 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 6293bdd7d862..720d8e633f7e 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -932,10 +932,14 @@ struct hci_cp_sniff_subrate {
> #define HCI_OP_RESET			0x0c03
> 
> #define HCI_OP_SET_EVENT_FLT		0x0c05
> -struct hci_cp_set_event_flt {
> -	__u8     flt_type;
> -	__u8     cond_type;
> -	__u8     condition[0];
> +#define HCI_SET_EVENT_FLT_SIZE		9
> +struct hci_cp_set_event_filter {
> +	__u8		flt_type;
> +	__u8		cond_type;
> +	struct {
> +		bdaddr_t bdaddr;
> +		__u8 auto_accept;
> +	} __packed	addr_conn_flt;
> } __packed;
> 
> /* Filter types */
> @@ -949,8 +953,9 @@ struct hci_cp_set_event_flt {
> #define HCI_CONN_SETUP_ALLOW_BDADDR	0x02
> 
> /* CONN_SETUP Conditions */
> -#define HCI_CONN_SETUP_AUTO_OFF	0x01
> -#define HCI_CONN_SETUP_AUTO_ON	0x02
> +#define HCI_CONN_SETUP_AUTO_OFF		0x01
> +#define HCI_CONN_SETUP_AUTO_ON		0x02
> +#define HCI_CONN_SETUP_AUTO_ON_WITH_RS	0x03
> 
> #define HCI_OP_READ_STORED_LINK_KEY	0x0c0d
> struct hci_cp_read_stored_link_key {
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 74d703e46fb4..49eae4a802ac 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -91,6 +91,9 @@ struct discovery_state {
> #define SUSPEND_NOTIFIER_TIMEOUT	msecs_to_jiffies(2000) /* 2 seconds */
> 
> enum suspend_tasks {
> +	SUSPEND_LE_SET_SCAN_ENABLE,
> +	SUSPEND_DISCONNECTING,
> +
> 	SUSPEND_PREPARE_NOTIFIER,
> 	__SUSPEND_NUM_TASKS
> };
> @@ -406,6 +409,8 @@ struct hci_dev {
> 	struct work_struct	suspend_prepare;
> 	enum suspended_state	suspend_state_next;
> 	enum suspended_state	suspend_state;
> +	int			disconnect_counter;
> +	bool			freeze_filters;
> 
> 	wait_queue_head_t	suspend_wait_q;
> 	DECLARE_BITMAP(suspend_tasks, __SUSPEND_NUM_TASKS);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 6ddc4a74a5e4..76d25b3f4c73 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2474,6 +2474,7 @@ static void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
> static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> 	struct hci_ev_conn_complete *ev = (void *) skb->data;
> +	struct inquiry_entry *ie;
> 	struct hci_conn *conn;
> 
> 	BT_DBG("%s", hdev->name);
> @@ -2482,14 +2483,29 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 
> 	conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
> 	if (!conn) {
> -		if (ev->link_type != SCO_LINK)
> -			goto unlock;
> +		/* Connection may not exist if auto-connected. Check the inquiry
> +		 * cache to see if we've already discovered this bdaddr before.
> +		 * Create a new connection if it was previously discovered.
> +		 */
> +		ie = hci_inquiry_cache_lookup(hdev, &ev->bdaddr);
> +		if (ie) {
> +			conn = hci_conn_add(hdev, ev->link_type, &ev->bdaddr,
> +					    HCI_ROLE_SLAVE);
> +			if (!conn) {
> +				bt_dev_err(hdev, "no memory for new conn");
> +				goto unlock;
> +			}
> +		} else {
> +			if (ev->link_type != SCO_LINK)
> +				goto unlock;

We are not going to get SCO_LINK auto-connected and thus we should really have it done just for ACL links. I don’t have good proposal to structure this handling at the moment, but we need to clean this up a bit.

> -		conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK, &ev->bdaddr);
> -		if (!conn)
> -			goto unlock;
> +			conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK,
> +						       &ev->bdaddr);
> +			if (!conn)
> +				goto unlock;
> 
> -		conn->type = SCO_LINK;
> +			conn->type = SCO_LINK;
> +		}
> 	}
> 
> 	if (!ev->status) {
> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> index 08908469c043..c930b9ff1cfd 100644
> --- a/net/bluetooth/hci_request.c
> +++ b/net/bluetooth/hci_request.c
> @@ -34,6 +34,12 @@
> #define HCI_REQ_PEND	  1
> #define HCI_REQ_CANCELED  2
> 
> +#define LE_SCAN_FLAG_SUSPEND	0x1
> +#define LE_SCAN_FLAG_ALLOW_RPA	0x2
> +
> +#define LE_SUSPEND_SCAN_WINDOW		0x0012
> +#define LE_SUSPEND_SCAN_INTERVAL	0x0060
> +
> void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
> {
> 	skb_queue_head_init(&req->cmd_q);
> @@ -654,6 +660,12 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
> {
> 	struct hci_dev *hdev = req->hdev;
> 
> +	/* Early exit if we've frozen filters for suspend*/

Please fix the coding style for comments. Extra space required between suspend and */

I also have the feeling that we should split BR/EDR from LE support. I think we can merge the BR/EDR support quicker since it is a lot simpler. Especially when it stands on its own.

> +	if (hdev->freeze_filters) {
> +		BT_DBG("Filters are frozen for suspend");
> +		return;
> +	}
> +
> 	if (use_ext_scan(hdev)) {
> 		struct hci_cp_le_set_ext_scan_enable cp;
> 
> @@ -670,23 +682,67 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
> 	}
> }
> 
> -static void add_to_white_list(struct hci_request *req,
> -			      struct hci_conn_params *params)
> +static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr,
> +				u8 bdaddr_type)
> +{
> +	struct hci_cp_le_del_from_white_list cp;
> +
> +	cp.bdaddr_type = bdaddr_type;
> +	bacpy(&cp.bdaddr, bdaddr);
> +
> +	BT_DBG("Remove %pMR (0x%x) from whitelist", &cp.bdaddr, cp.bdaddr_type);
> +	hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp);
> +}
> +
> +/* Adds connection to white list if needed. On error, returns -1 */
> +static int add_to_white_list(struct hci_request *req,
> +			     struct hci_conn_params *params, u8 *num_entries,
> +			     u8 flags)
> {
> 	struct hci_cp_le_add_to_white_list cp;
> +	struct hci_dev *hdev = req->hdev;
> +	bool allow_rpa = !!(flags & LE_SCAN_FLAG_ALLOW_RPA);
> +	bool suspend = !!(flags & LE_SCAN_FLAG_SUSPEND);
> 
> +	/* Already in white list */
> +	if (hci_bdaddr_list_lookup(&hdev->le_white_list, &params->addr,
> +				   params->addr_type))
> +		return 0;
> +
> +	/* Select filter policy to accept all advertising */
> +	if (*num_entries >= hdev->le_white_list_size)
> +		return -1;
> +
> +	/* White list can not be used with RPAs */
> +	if (!allow_rpa &&
> +	    hci_find_irk_by_addr(hdev, &params->addr, params->addr_type)) {
> +		return -1;
> +	}
> +
> +	/* During suspend, only wakeable devices can be in whitelist */
> +	if (suspend && !hci_bdaddr_list_lookup(&hdev->wakeable, &params->addr,
> +					       params->addr_type))
> +		return 0;
> +
> +	*num_entries += 1;
> 	cp.bdaddr_type = params->addr_type;
> 	bacpy(&cp.bdaddr, &params->addr);
> 
> +	BT_DBG("Add %pMR (0x%x) to whitelist", &cp.bdaddr, cp.bdaddr_type);
> 	hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp);
> +
> +	return 0;
> }
> 
> -static u8 update_white_list(struct hci_request *req)
> +static u8 update_white_list(struct hci_request *req, u8 flags)
> {
> 	struct hci_dev *hdev = req->hdev;
> 	struct hci_conn_params *params;
> 	struct bdaddr_list *b;
> -	uint8_t white_list_entries = 0;
> +	u8 white_list_entries = 0;
> +	bool allow_rpa = !!(flags & LE_SCAN_FLAG_ALLOW_RPA);
> +	bool suspend = !!(flags & LE_SCAN_FLAG_SUSPEND);
> +	bool wakeable, pend_conn, pend_report;
> 
> 	/* Go through the current white list programmed into the
> 	 * controller one by one and check if that address is still
> @@ -695,26 +751,42 @@ static u8 update_white_list(struct hci_request *req)
> 	 * command to remove it from the controller.
> 	 */
> 	list_for_each_entry(b, &hdev->le_white_list, list) {
> -		/* If the device is neither in pend_le_conns nor
> -		 * pend_le_reports then remove it from the whitelist.
> +		wakeable = !!hci_bdaddr_list_lookup(&hdev->wakeable, &b->bdaddr,
> +						    b->bdaddr_type);
> +		pend_conn = hci_pend_le_action_lookup(&hdev->pend_le_conns,
> +						      &b->bdaddr,
> +						      b->bdaddr_type);
> +		pend_report = hci_pend_le_action_lookup(&hdev->pend_le_reports,
> +							&b->bdaddr,
> +							b->bdaddr_type);
> +
> +		/* During suspend, we remove all non-wakeable devices
> +		 * and leave all others alone. Connected devices will be
> +		 * disconnected during suspend but may not be in the pending
> +		 * list yet.
> 		 */
> -		if (!hci_pend_le_action_lookup(&hdev->pend_le_conns,
> -					       &b->bdaddr, b->bdaddr_type) &&
> -		    !hci_pend_le_action_lookup(&hdev->pend_le_reports,
> -					       &b->bdaddr, b->bdaddr_type)) {
> -			struct hci_cp_le_del_from_white_list cp;
> -
> -			cp.bdaddr_type = b->bdaddr_type;
> -			bacpy(&cp.bdaddr, &b->bdaddr);
> -
> -			hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST,
> -				    sizeof(cp), &cp);
> -			continue;
> -		}
> +		if (suspend) {
> +			if (!wakeable) {
> +				del_from_white_list(req, &b->bdaddr,
> +						    b->bdaddr_type);
> +				continue;
> +			}
> +		} else {
> +			/* If the device is not likely to connect or report,
> +			 * remove it from the whitelist.
> +			 */
> +			if (!pend_conn && !pend_report) {
> +				del_from_white_list(req, &b->bdaddr,
> +						    b->bdaddr_type);
> +				continue;
> +			}
> 
> -		if (hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) {
> 			/* White list can not be used with RPAs */
> -			return 0x00;
> +			if (!allow_rpa &&
> +			    hci_find_irk_by_addr(hdev, &b->bdaddr,
> +						 b->bdaddr_type)) {
> +				return 0x00;
> +			}
> 		}
> 
> 		white_list_entries++;
> @@ -731,47 +803,30 @@ static u8 update_white_list(struct hci_request *req)
> 	 * white list.
> 	 */
> 	list_for_each_entry(params, &hdev->pend_le_conns, action) {
> -		if (hci_bdaddr_list_lookup(&hdev->le_white_list,
> -					   &params->addr, params->addr_type))
> -			continue;
> -
> -		if (white_list_entries >= hdev->le_white_list_size) {
> -			/* Select filter policy to accept all advertising */
> +		if (add_to_white_list(req, params, &white_list_entries, flags))
> 			return 0x00;
> -		}
> -
> -		if (hci_find_irk_by_addr(hdev, &params->addr,
> -					 params->addr_type)) {
> -			/* White list can not be used with RPAs */
> -			return 0x00;
> -		}
> -
> -		white_list_entries++;
> -		add_to_white_list(req, params);
> 	}
> 
> 	/* After adding all new pending connections, walk through
> 	 * the list of pending reports and also add these to the
> -	 * white list if there is still space.
> +	 * white list if there is still space. Abort if space runs out.
> 	 */
> 	list_for_each_entry(params, &hdev->pend_le_reports, action) {
> -		if (hci_bdaddr_list_lookup(&hdev->le_white_list,
> -					   &params->addr, params->addr_type))
> -			continue;
> -
> -		if (white_list_entries >= hdev->le_white_list_size) {
> -			/* Select filter policy to accept all advertising */
> +		if (add_to_white_list(req, params, &white_list_entries, flags))
> 			return 0x00;
> -		}
> +	}
> 
> -		if (hci_find_irk_by_addr(hdev, &params->addr,
> -					 params->addr_type)) {
> -			/* White list can not be used with RPAs */
> -			return 0x00;
> +	/* Currently connected devices will be missing from the white list and
> +	 * we need to insert them into the whitelist if they are wakeable. We
> +	 * can't insert later because we will have already returned from the
> +	 * suspend notifier and would cause a spurious wakeup.
> +	 */
> +	if (suspend) {
> +		list_for_each_entry(params, &hdev->le_conn_params, list) {
> +			if (add_to_white_list(req, params, &white_list_entries,
> +					      flags))
> +				return 0x00;
> 		}
> -
> -		white_list_entries++;
> -		add_to_white_list(req, params);
> 	}

This goes to my point above. If we just disconnect everything, then the white list programming should be easily. Everything that is pending and have been elected for wakeup + incoming connection will be added. And then we are done. I don’t think much has to change except one extra check if wakeup is allowed or not.

> 
> 	/* Select filter policy to use white list */
> @@ -861,11 +916,26 @@ static void hci_req_start_scan(struct hci_request *req, u8 type, u16 interval,
> 	}
> }
> 
> -void hci_req_add_le_passive_scan(struct hci_request *req)
> +void __hci_req_add_le_passive_scan(struct hci_request *req, u8 flags)
> {
> 	struct hci_dev *hdev = req->hdev;
> 	u8 own_addr_type;
> 	u8 filter_policy;
> +	u8 window, interval;
> +
> +	/* We allow whitelisting even with RPAs in suspend. In the worst case,
> +	 * we won't be able to wake from devices that use the privacy1.2
> +	 * features. Additionally, once we support privacy1.2 and IRK
> +	 * offloading, we can update this to also check for those conditions.
> +	 */
> +	if (flags & LE_SCAN_FLAG_SUSPEND)
> +		flags |= LE_SCAN_FLAG_ALLOW_RPA;
> +
> +	/* Early exit if we've frozen filters for suspend */
> +	if (hdev->freeze_filters) {
> +		BT_DBG("Filters are frozen for suspend");
> +		return;
> +	}
> 
> 	/* Set require_privacy to false since no SCAN_REQ are send
> 	 * during passive scanning. Not using an non-resolvable address
> @@ -881,7 +951,8 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
> 	 * happen before enabling scanning. The controller does
> 	 * not allow white list modification while scanning.
> 	 */
> -	filter_policy = update_white_list(req);
> +	BT_DBG("Updating white list with flags = %d", flags);
> +	filter_policy = update_white_list(req, flags);
> 
> 	/* When the controller is using random resolvable addresses and
> 	 * with that having LE privacy enabled, then controllers with
> @@ -896,8 +967,22 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
> 	    (hdev->le_features[0] & HCI_LE_EXT_SCAN_POLICY))
> 		filter_policy |= 0x02;
> 
> -	hci_req_start_scan(req, LE_SCAN_PASSIVE, hdev->le_scan_interval,
> -			   hdev->le_scan_window, own_addr_type, filter_policy);
> +	if (flags & LE_SCAN_FLAG_SUSPEND) {
> +		window = LE_SUSPEND_SCAN_WINDOW;
> +		interval = LE_SUSPEND_SCAN_INTERVAL;

I think we just want this done via some hdev->suspended variable or similar.

> +	} else {
> +		window = hdev->le_scan_window;
> +		interval = hdev->le_scan_interval;
> +	}
> +
> +	BT_DBG("LE passive scan with whitelist = %d", filter_policy);
> +	hci_req_start_scan(req, LE_SCAN_PASSIVE, interval, window,
> +			   own_addr_type, filter_policy);
> +}
> +
> +void hci_req_add_le_passive_scan(struct hci_request *req)
> +{
> +	__hci_req_add_le_passive_scan(req, 0);
> }
> 
> static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
> @@ -918,6 +1003,76 @@ static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
> 	return adv_instance->scan_rsp_len;
> }
> 
> +static void hci_req_clear_event_filter(struct hci_request *req)
> +{
> +	struct hci_cp_set_event_filter f;
> +
> +	memset(&f, 0, sizeof(f));
> +	f.flt_type = HCI_FLT_CLEAR_ALL;
> +	hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &f);
> +
> +	/* Update page scan state (since we may have modified it when setting
> +	 * the event filter).
> +	 */
> +	__hci_req_update_scan(req);
> +}
> +
> +static void hci_req_set_event_filter(struct hci_request *req)
> +{
> +	struct bdaddr_list *b;
> +	struct hci_cp_set_event_filter f;
> +	struct hci_dev *hdev = req->hdev;
> +	int filters_updated = 0;
> +	u8 scan;
> +
> +	/* Always clear event filter when starting */
> +	hci_req_clear_event_filter(req);
> +
> +	list_for_each_entry(b, &hdev->wakeable, list) {
> +		if (b->bdaddr_type != BDADDR_BREDR)
> +			continue;
> +
> +		memset(&f, 0, sizeof(f));
> +		bacpy(&f.addr_conn_flt.bdaddr, &b->bdaddr);
> +		f.flt_type = HCI_FLT_CONN_SETUP;
> +		f.cond_type = HCI_CONN_SETUP_ALLOW_BDADDR;
> +		f.addr_conn_flt.auto_accept = HCI_CONN_SETUP_AUTO_ON;
> +
> +		BT_DBG("Adding event filters for %pMR", &b->bdaddr);
> +		hci_req_add(req, HCI_OP_SET_EVENT_FLT, sizeof(f), &f);
> +
> +		filters_updated++;
> +	}

If we would use the wakeable list just for BR/EDR then the filters_updated++ would become redundant and we could just check if the list is empty.

> +
> +	scan = filters_updated ? SCAN_PAGE : SCAN_DISABLED;
> +	hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
> +}
> +
> +static void hci_req_enable_le_suspend_scan(struct hci_request *req,
> +					   u8 flags)
> +{
> +	/* Can't change params without disabling first */
> +	hci_req_add_le_scan_disable(req);
> +
> +	/* Configure params and enable scanning */
> +	__hci_req_add_le_passive_scan(req, flags);
> +
> +	/* Block suspend notifier on response */
> +	set_bit(SUSPEND_LE_SET_SCAN_ENABLE, req->hdev->suspend_tasks);
> +}
> +
> +static void le_suspend_req_complete(struct hci_dev *hdev, u8 status, u16 opcode)
> +{
> +	BT_DBG("Request complete opcode=0x%x, status=0x%x", opcode, status);
> +
> +	/* Expecting LE Set scan to return */
> +	if (opcode == HCI_OP_LE_SET_SCAN_ENABLE &&
> +	    test_and_clear_bit(SUSPEND_LE_SET_SCAN_ENABLE,
> +			       hdev->suspend_tasks)) {
> +		wake_up(&hdev->suspend_wait_q);
> +	}
> +}
> +
> /* Call with hci_dev_lock */
> void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> {
> @@ -932,6 +1087,44 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> 
> 	hdev->suspend_state = next;
> 
> +	hci_req_init(&req, hdev);
> +	if (next == BT_SUSPENDED) {
> +		/* Enable event filter for existing devices */
> +		hci_req_set_event_filter(&req);
> +
> +		/* Enable passive scan at lower duty cycle */
> +		hci_req_enable_le_suspend_scan(&req, LE_SCAN_FLAG_SUSPEND);
> +
> +		hdev->freeze_filters = true;
> +
> +		/* Run commands before disconnecting */
> +		hci_req_run(&req, le_suspend_req_complete);
> +
> +		hdev->disconnect_counter = 0;
> +		/* Soft disconnect everything (power off)*/
> +		list_for_each_entry(conn, &hdev->conn_hash.list, list) {
> +			hci_disconnect(conn, HCI_ERROR_REMOTE_POWER_OFF);
> +			hdev->disconnect_counter++;
> +		}
> +
> +		if (hdev->disconnect_counter > 0) {
> +			BT_DBG("Had %d disconnects. Will wait on them",
> +			       hdev->disconnect_counter);
> +			set_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks);
> +		}
> +	} else {
> +		hdev->freeze_filters = false;
> +
> +		hci_req_clear_event_filter(&req);
> +
> +		/* Reset passive/background scanning to normal */
> +		hci_req_enable_le_suspend_scan(&req, 0);
> +
> +		hci_req_run(&req, le_suspend_req_complete);
> +	}
> +
> +	hdev->suspend_state = next;
> +
> done:
> 	clear_bit(SUSPEND_PREPARE_NOTIFIER, hdev->suspend_tasks);
> 	wake_up(&hdev->suspend_wait_q);
> @@ -2034,6 +2227,9 @@ void __hci_req_update_scan(struct hci_request *req)
> 	if (mgmt_powering_down(hdev))
> 		return;
> 
> +	if (hdev->freeze_filters)
> +		return;
> +
> 	if (hci_dev_test_flag(hdev, HCI_CONNECTABLE) ||
> 	    disconnected_whitelist_entries(hdev))
> 		scan = SCAN_PAGE;
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 58468dfa112f..269ce70e501c 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -7451,6 +7451,14 @@ void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
> 
> 	mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, &ev, sizeof(ev), sk);
> 
> +	if (hdev->disconnect_counter > 0) {
> +		hdev->disconnect_counter--;
> +		if (hdev->disconnect_counter <= 0) {
> +			clear_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks);
> +			wake_up(&hdev->suspend_wait_q);
> +		}
> +	}
> +

I think this disconnect_counter is a bit racy. I would rather check that our connection hash is empty.

In addition, I have the feeling we better disable LE scanning BR/EDR page scan first. So that we clearly do not have any artifacts of accidental incoming connections.

The first step when going to suspend should clean the house

	1) Set a flag that all BR/EDR and LE connection attempts are rejected
	2) Disable BR/EDR page scan
	3) Disable LE advertising
	2) Disable LE scanning / BR/EDR inquiry
	4) Disconnect all

Now we have a clean house and we now prepare for wakeup triggers

	1) Clear flag and allow incoming connection
	2) Set BR/EDR event filter and enable page scan if needed
	3) Set LE whitelist and enable scanning if needed

When we wake up, then we also just need to reset the filters

	1) Clear BR/EDR event filter and enable page scan if needed
	2) Update LE white and enable scanning if needed
	3) Restore LE scanning / BR/EDR inquiry

Regards

Marcel

Re: [RFC PATCH v2 3/4] Bluetooth: Update filters/whitelists for suspend

[Abhishek Pandit-Subedi]

Hi Marcel,

On Wed, Jan 29, 2020 at 12:40 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > When suspending, update the event filter for BR/EDR devices and the
> > whitelist for LE devices. BR/EDR devices are added to the event filter
> > and will auto-connect if found during suspend. For LE, we update the
> > filter to remove everything that is not wakeable during suspend.
> > Finally, we disconnect all connected devices and wait for that to
> > complete before returning in the suspend notifier.
> >
> > An example suspend flow with 1 BR/EDR HID device, 1 BR/EDR audio device,
> > 1 LE HID device, 1 LE non-HID (where HIDs are wakeable):
> >
> > PM_PREPARE_SUSPEND:
> >  - Set event filter for BR/EDR HID device
> >  - Clear anything from LE whitelist that is not wakeable
> >  - Add (if not there) the LE HID device
> >  - Disconnect all devices
>
> is this really the right order? Why not disconnect all devices first. We are suspending after all, so lets disconnect first and then create the appropriate accept filters.
>
> I have the feeling it would also make the overall logic a lot simpler since it fits more into the current model that is used internally for a lot of decision points (especially with LE).
>
> > PM_POST_SUSPEND:
> >  - Clear event filter
> >  - Restore LE whitelist (deleting anything not in le_pend_conn or
> >    le_pend_report and then adding from those lists)
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v2:
> > * Refactored filters and whitelist settings to its own patch
> > * Refactored update_white_list to have clearer edge cases
> > * Add connected devices to whitelist (previously missing corner case)
> >
> > include/net/bluetooth/hci.h      |  17 +-
> > include/net/bluetooth/hci_core.h |   5 +
> > net/bluetooth/hci_event.c        |  28 ++-
> > net/bluetooth/hci_request.c      | 308 +++++++++++++++++++++++++------
> > net/bluetooth/mgmt.c             |   8 +
> > 5 files changed, 298 insertions(+), 68 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 6293bdd7d862..720d8e633f7e 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -932,10 +932,14 @@ struct hci_cp_sniff_subrate {
> > #define HCI_OP_RESET                  0x0c03
> >
> > #define HCI_OP_SET_EVENT_FLT          0x0c05
> > -struct hci_cp_set_event_flt {
> > -     __u8     flt_type;
> > -     __u8     cond_type;
> > -     __u8     condition[0];
> > +#define HCI_SET_EVENT_FLT_SIZE               9
> > +struct hci_cp_set_event_filter {
> > +     __u8            flt_type;
> > +     __u8            cond_type;
> > +     struct {
> > +             bdaddr_t bdaddr;
> > +             __u8 auto_accept;
> > +     } __packed      addr_conn_flt;
> > } __packed;
> >
> > /* Filter types */
> > @@ -949,8 +953,9 @@ struct hci_cp_set_event_flt {
> > #define HCI_CONN_SETUP_ALLOW_BDADDR   0x02
> >
> > /* CONN_SETUP Conditions */
> > -#define HCI_CONN_SETUP_AUTO_OFF      0x01
> > -#define HCI_CONN_SETUP_AUTO_ON       0x02
> > +#define HCI_CONN_SETUP_AUTO_OFF              0x01
> > +#define HCI_CONN_SETUP_AUTO_ON               0x02
> > +#define HCI_CONN_SETUP_AUTO_ON_WITH_RS       0x03
> >
> > #define HCI_OP_READ_STORED_LINK_KEY   0x0c0d
> > struct hci_cp_read_stored_link_key {
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 74d703e46fb4..49eae4a802ac 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -91,6 +91,9 @@ struct discovery_state {
> > #define SUSPEND_NOTIFIER_TIMEOUT      msecs_to_jiffies(2000) /* 2 seconds */
> >
> > enum suspend_tasks {
> > +     SUSPEND_LE_SET_SCAN_ENABLE,
> > +     SUSPEND_DISCONNECTING,
> > +
> >       SUSPEND_PREPARE_NOTIFIER,
> >       __SUSPEND_NUM_TASKS
> > };
> > @@ -406,6 +409,8 @@ struct hci_dev {
> >       struct work_struct      suspend_prepare;
> >       enum suspended_state    suspend_state_next;
> >       enum suspended_state    suspend_state;
> > +     int                     disconnect_counter;
> > +     bool                    freeze_filters;
> >
> >       wait_queue_head_t       suspend_wait_q;
> >       DECLARE_BITMAP(suspend_tasks, __SUSPEND_NUM_TASKS);
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 6ddc4a74a5e4..76d25b3f4c73 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -2474,6 +2474,7 @@ static void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > {
> >       struct hci_ev_conn_complete *ev = (void *) skb->data;
> > +     struct inquiry_entry *ie;
> >       struct hci_conn *conn;
> >
> >       BT_DBG("%s", hdev->name);
> > @@ -2482,14 +2483,29 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >
> >       conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
> >       if (!conn) {
> > -             if (ev->link_type != SCO_LINK)
> > -                     goto unlock;
> > +             /* Connection may not exist if auto-connected. Check the inquiry
> > +              * cache to see if we've already discovered this bdaddr before.
> > +              * Create a new connection if it was previously discovered.
> > +              */
> > +             ie = hci_inquiry_cache_lookup(hdev, &ev->bdaddr);
> > +             if (ie) {
> > +                     conn = hci_conn_add(hdev, ev->link_type, &ev->bdaddr,
> > +                                         HCI_ROLE_SLAVE);
> > +                     if (!conn) {
> > +                             bt_dev_err(hdev, "no memory for new conn");
> > +                             goto unlock;
> > +                     }
> > +             } else {
> > +                     if (ev->link_type != SCO_LINK)
> > +                             goto unlock;
>
> We are not going to get SCO_LINK auto-connected and thus we should really have it done just for ACL links. I don’t have good proposal to structure this handling at the moment, but we need to clean this up a bit.

Ack, will look deeper into this.

>
> > -             conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK, &ev->bdaddr);
> > -             if (!conn)
> > -                     goto unlock;
> > +                     conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK,
> > +                                                    &ev->bdaddr);
> > +                     if (!conn)
> > +                             goto unlock;
> >
> > -             conn->type = SCO_LINK;
> > +                     conn->type = SCO_LINK;
> > +             }
> >       }
> >
> >       if (!ev->status) {
> > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> > index 08908469c043..c930b9ff1cfd 100644
> > --- a/net/bluetooth/hci_request.c
> > +++ b/net/bluetooth/hci_request.c
> > @@ -34,6 +34,12 @@
> > #define HCI_REQ_PEND    1
> > #define HCI_REQ_CANCELED  2
> >
> > +#define LE_SCAN_FLAG_SUSPEND 0x1
> > +#define LE_SCAN_FLAG_ALLOW_RPA       0x2
> > +
> > +#define LE_SUSPEND_SCAN_WINDOW               0x0012
> > +#define LE_SUSPEND_SCAN_INTERVAL     0x0060
> > +
> > void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
> > {
> >       skb_queue_head_init(&req->cmd_q);
> > @@ -654,6 +660,12 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
> > {
> >       struct hci_dev *hdev = req->hdev;
> >
> > +     /* Early exit if we've frozen filters for suspend*/
>
> Please fix the coding style for comments. Extra space required between suspend and */
>
> I also have the feeling that we should split BR/EDR from LE support. I think we can merge the BR/EDR support quicker since it is a lot simpler. Especially when it stands on its own.

Ack. I kept them merged because I didn't want to change the request
completion function but I can separate them.

>
> > +     if (hdev->freeze_filters) {
> > +             BT_DBG("Filters are frozen for suspend");
> > +             return;
> > +     }
> > +
> >       if (use_ext_scan(hdev)) {
> >               struct hci_cp_le_set_ext_scan_enable cp;
> >
> > @@ -670,23 +682,67 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
> >       }
> > }
> >
> > -static void add_to_white_list(struct hci_request *req,
> > -                           struct hci_conn_params *params)
> > +static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr,
> > +                             u8 bdaddr_type)
> > +{
> > +     struct hci_cp_le_del_from_white_list cp;
> > +
> > +     cp.bdaddr_type = bdaddr_type;
> > +     bacpy(&cp.bdaddr, bdaddr);
> > +
> > +     BT_DBG("Remove %pMR (0x%x) from whitelist", &cp.bdaddr, cp.bdaddr_type);
> > +     hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp);
> > +}
> > +
> > +/* Adds connection to white list if needed. On error, returns -1 */
> > +static int add_to_white_list(struct hci_request *req,
> > +                          struct hci_conn_params *params, u8 *num_entries,
> > +                          u8 flags)
> > {
> >       struct hci_cp_le_add_to_white_list cp;
> > +     struct hci_dev *hdev = req->hdev;
> > +     bool allow_rpa = !!(flags & LE_SCAN_FLAG_ALLOW_RPA);
> > +     bool suspend = !!(flags & LE_SCAN_FLAG_SUSPEND);
> >
> > +     /* Already in white list */
> > +     if (hci_bdaddr_list_lookup(&hdev->le_white_list, &params->addr,
> > +                                params->addr_type))
> > +             return 0;
> > +
> > +     /* Select filter policy to accept all advertising */
> > +     if (*num_entries >= hdev->le_white_list_size)
> > +             return -1;
> > +
> > +     /* White list can not be used with RPAs */
> > +     if (!allow_rpa &&
> > +         hci_find_irk_by_addr(hdev, &params->addr, params->addr_type)) {
> > +             return -1;
> > +     }
> > +
> > +     /* During suspend, only wakeable devices can be in whitelist */
> > +     if (suspend && !hci_bdaddr_list_lookup(&hdev->wakeable, &params->addr,
> > +                                            params->addr_type))
> > +             return 0;
> > +
> > +     *num_entries += 1;
> >       cp.bdaddr_type = params->addr_type;
> >       bacpy(&cp.bdaddr, &params->addr);
> >
> > +     BT_DBG("Add %pMR (0x%x) to whitelist", &cp.bdaddr, cp.bdaddr_type);
> >       hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp);
> > +
> > +     return 0;
> > }
> >
> > -static u8 update_white_list(struct hci_request *req)
> > +static u8 update_white_list(struct hci_request *req, u8 flags)
> > {
> >       struct hci_dev *hdev = req->hdev;
> >       struct hci_conn_params *params;
> >       struct bdaddr_list *b;
> > -     uint8_t white_list_entries = 0;
> > +     u8 white_list_entries = 0;
> > +     bool allow_rpa = !!(flags & LE_SCAN_FLAG_ALLOW_RPA);
> > +     bool suspend = !!(flags & LE_SCAN_FLAG_SUSPEND);
> > +     bool wakeable, pend_conn, pend_report;
> >
> >       /* Go through the current white list programmed into the
> >        * controller one by one and check if that address is still
> > @@ -695,26 +751,42 @@ static u8 update_white_list(struct hci_request *req)
> >        * command to remove it from the controller.
> >        */
> >       list_for_each_entry(b, &hdev->le_white_list, list) {
> > -             /* If the device is neither in pend_le_conns nor
> > -              * pend_le_reports then remove it from the whitelist.
> > +             wakeable = !!hci_bdaddr_list_lookup(&hdev->wakeable, &b->bdaddr,
> > +                                                 b->bdaddr_type);
> > +             pend_conn = hci_pend_le_action_lookup(&hdev->pend_le_conns,
> > +                                                   &b->bdaddr,
> > +                                                   b->bdaddr_type);
> > +             pend_report = hci_pend_le_action_lookup(&hdev->pend_le_reports,
> > +                                                     &b->bdaddr,
> > +                                                     b->bdaddr_type);
> > +
> > +             /* During suspend, we remove all non-wakeable devices
> > +              * and leave all others alone. Connected devices will be
> > +              * disconnected during suspend but may not be in the pending
> > +              * list yet.
> >                */
> > -             if (!hci_pend_le_action_lookup(&hdev->pend_le_conns,
> > -                                            &b->bdaddr, b->bdaddr_type) &&
> > -                 !hci_pend_le_action_lookup(&hdev->pend_le_reports,
> > -                                            &b->bdaddr, b->bdaddr_type)) {
> > -                     struct hci_cp_le_del_from_white_list cp;
> > -
> > -                     cp.bdaddr_type = b->bdaddr_type;
> > -                     bacpy(&cp.bdaddr, &b->bdaddr);
> > -
> > -                     hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST,
> > -                                 sizeof(cp), &cp);
> > -                     continue;
> > -             }
> > +             if (suspend) {
> > +                     if (!wakeable) {
> > +                             del_from_white_list(req, &b->bdaddr,
> > +                                                 b->bdaddr_type);
> > +                             continue;
> > +                     }
> > +             } else {
> > +                     /* If the device is not likely to connect or report,
> > +                      * remove it from the whitelist.
> > +                      */
> > +                     if (!pend_conn && !pend_report) {
> > +                             del_from_white_list(req, &b->bdaddr,
> > +                                                 b->bdaddr_type);
> > +                             continue;
> > +                     }
> >
> > -             if (hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) {
> >                       /* White list can not be used with RPAs */
> > -                     return 0x00;
> > +                     if (!allow_rpa &&
> > +                         hci_find_irk_by_addr(hdev, &b->bdaddr,
> > +                                              b->bdaddr_type)) {
> > +                             return 0x00;
> > +                     }
> >               }
> >
> >               white_list_entries++;
> > @@ -731,47 +803,30 @@ static u8 update_white_list(struct hci_request *req)
> >        * white list.
> >        */
> >       list_for_each_entry(params, &hdev->pend_le_conns, action) {
> > -             if (hci_bdaddr_list_lookup(&hdev->le_white_list,
> > -                                        &params->addr, params->addr_type))
> > -                     continue;
> > -
> > -             if (white_list_entries >= hdev->le_white_list_size) {
> > -                     /* Select filter policy to accept all advertising */
> > +             if (add_to_white_list(req, params, &white_list_entries, flags))
> >                       return 0x00;
> > -             }
> > -
> > -             if (hci_find_irk_by_addr(hdev, &params->addr,
> > -                                      params->addr_type)) {
> > -                     /* White list can not be used with RPAs */
> > -                     return 0x00;
> > -             }
> > -
> > -             white_list_entries++;
> > -             add_to_white_list(req, params);
> >       }
> >
> >       /* After adding all new pending connections, walk through
> >        * the list of pending reports and also add these to the
> > -      * white list if there is still space.
> > +      * white list if there is still space. Abort if space runs out.
> >        */
> >       list_for_each_entry(params, &hdev->pend_le_reports, action) {
> > -             if (hci_bdaddr_list_lookup(&hdev->le_white_list,
> > -                                        &params->addr, params->addr_type))
> > -                     continue;
> > -
> > -             if (white_list_entries >= hdev->le_white_list_size) {
> > -                     /* Select filter policy to accept all advertising */
> > +             if (add_to_white_list(req, params, &white_list_entries, flags))
> >                       return 0x00;
> > -             }
> > +     }
> >
> > -             if (hci_find_irk_by_addr(hdev, &params->addr,
> > -                                      params->addr_type)) {
> > -                     /* White list can not be used with RPAs */
> > -                     return 0x00;
> > +     /* Currently connected devices will be missing from the white list and
> > +      * we need to insert them into the whitelist if they are wakeable. We
> > +      * can't insert later because we will have already returned from the
> > +      * suspend notifier and would cause a spurious wakeup.
> > +      */
> > +     if (suspend) {
> > +             list_for_each_entry(params, &hdev->le_conn_params, list) {
> > +                     if (add_to_white_list(req, params, &white_list_entries,
> > +                                           flags))
> > +                             return 0x00;
> >               }
> > -
> > -             white_list_entries++;
> > -             add_to_white_list(req, params);
> >       }
>
> This goes to my point above. If we just disconnect everything, then the white list programming should be easily. Everything that is pending and have been elected for wakeup + incoming connection will be added. And then we are done. I don’t think much has to change except one extra check if wakeup is allowed or not.
>
> >
> >       /* Select filter policy to use white list */
> > @@ -861,11 +916,26 @@ static void hci_req_start_scan(struct hci_request *req, u8 type, u16 interval,
> >       }
> > }
> >
> > -void hci_req_add_le_passive_scan(struct hci_request *req)
> > +void __hci_req_add_le_passive_scan(struct hci_request *req, u8 flags)
> > {
> >       struct hci_dev *hdev = req->hdev;
> >       u8 own_addr_type;
> >       u8 filter_policy;
> > +     u8 window, interval;
> > +
> > +     /* We allow whitelisting even with RPAs in suspend. In the worst case,
> > +      * we won't be able to wake from devices that use the privacy1.2
> > +      * features. Additionally, once we support privacy1.2 and IRK
> > +      * offloading, we can update this to also check for those conditions.
> > +      */
> > +     if (flags & LE_SCAN_FLAG_SUSPEND)
> > +             flags |= LE_SCAN_FLAG_ALLOW_RPA;
> > +
> > +     /* Early exit if we've frozen filters for suspend */
> > +     if (hdev->freeze_filters) {
> > +             BT_DBG("Filters are frozen for suspend");
> > +             return;
> > +     }
> >
> >       /* Set require_privacy to false since no SCAN_REQ are send
> >        * during passive scanning. Not using an non-resolvable address
> > @@ -881,7 +951,8 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
> >        * happen before enabling scanning. The controller does
> >        * not allow white list modification while scanning.
> >        */
> > -     filter_policy = update_white_list(req);
> > +     BT_DBG("Updating white list with flags = %d", flags);
> > +     filter_policy = update_white_list(req, flags);
> >
> >       /* When the controller is using random resolvable addresses and
> >        * with that having LE privacy enabled, then controllers with
> > @@ -896,8 +967,22 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
> >           (hdev->le_features[0] & HCI_LE_EXT_SCAN_POLICY))
> >               filter_policy |= 0x02;
> >
> > -     hci_req_start_scan(req, LE_SCAN_PASSIVE, hdev->le_scan_interval,
> > -                        hdev->le_scan_window, own_addr_type, filter_policy);
> > +     if (flags & LE_SCAN_FLAG_SUSPEND) {
> > +             window = LE_SUSPEND_SCAN_WINDOW;
> > +             interval = LE_SUSPEND_SCAN_INTERVAL;
>
> I think we just want this done via some hdev->suspended variable or similar.

In support of setting a separate flag, it is easier to grep for when a
specific flag is being used and passed to a function than it is to
check when a variable is being set in a global context variable.

>
> > +     } else {
> > +             window = hdev->le_scan_window;
> > +             interval = hdev->le_scan_interval;
> > +     }
> > +
> > +     BT_DBG("LE passive scan with whitelist = %d", filter_policy);
> > +     hci_req_start_scan(req, LE_SCAN_PASSIVE, interval, window,
> > +                        own_addr_type, filter_policy);
> > +}
> > +
> > +void hci_req_add_le_passive_scan(struct hci_request *req)
> > +{
> > +     __hci_req_add_le_passive_scan(req, 0);
> > }
> >
> > static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
> > @@ -918,6 +1003,76 @@ static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
> >       return adv_instance->scan_rsp_len;
> > }
> >
> > +static void hci_req_clear_event_filter(struct hci_request *req)
> > +{
> > +     struct hci_cp_set_event_filter f;
> > +
> > +     memset(&f, 0, sizeof(f));
> > +     f.flt_type = HCI_FLT_CLEAR_ALL;
> > +     hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &f);
> > +
> > +     /* Update page scan state (since we may have modified it when setting
> > +      * the event filter).
> > +      */
> > +     __hci_req_update_scan(req);
> > +}
> > +
> > +static void hci_req_set_event_filter(struct hci_request *req)
> > +{
> > +     struct bdaddr_list *b;
> > +     struct hci_cp_set_event_filter f;
> > +     struct hci_dev *hdev = req->hdev;
> > +     int filters_updated = 0;
> > +     u8 scan;
> > +
> > +     /* Always clear event filter when starting */
> > +     hci_req_clear_event_filter(req);
> > +
> > +     list_for_each_entry(b, &hdev->wakeable, list) {
> > +             if (b->bdaddr_type != BDADDR_BREDR)
> > +                     continue;
> > +
> > +             memset(&f, 0, sizeof(f));
> > +             bacpy(&f.addr_conn_flt.bdaddr, &b->bdaddr);
> > +             f.flt_type = HCI_FLT_CONN_SETUP;
> > +             f.cond_type = HCI_CONN_SETUP_ALLOW_BDADDR;
> > +             f.addr_conn_flt.auto_accept = HCI_CONN_SETUP_AUTO_ON;
> > +
> > +             BT_DBG("Adding event filters for %pMR", &b->bdaddr);
> > +             hci_req_add(req, HCI_OP_SET_EVENT_FLT, sizeof(f), &f);
> > +
> > +             filters_updated++;
> > +     }
>
> If we would use the wakeable list just for BR/EDR then the filters_updated++ would become redundant and we could just check if the list is empty.
>
> > +
> > +     scan = filters_updated ? SCAN_PAGE : SCAN_DISABLED;
> > +     hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
> > +}
> > +
> > +static void hci_req_enable_le_suspend_scan(struct hci_request *req,
> > +                                        u8 flags)
> > +{
> > +     /* Can't change params without disabling first */
> > +     hci_req_add_le_scan_disable(req);
> > +
> > +     /* Configure params and enable scanning */
> > +     __hci_req_add_le_passive_scan(req, flags);
> > +
> > +     /* Block suspend notifier on response */
> > +     set_bit(SUSPEND_LE_SET_SCAN_ENABLE, req->hdev->suspend_tasks);
> > +}
> > +
> > +static void le_suspend_req_complete(struct hci_dev *hdev, u8 status, u16 opcode)
> > +{
> > +     BT_DBG("Request complete opcode=0x%x, status=0x%x", opcode, status);
> > +
> > +     /* Expecting LE Set scan to return */
> > +     if (opcode == HCI_OP_LE_SET_SCAN_ENABLE &&
> > +         test_and_clear_bit(SUSPEND_LE_SET_SCAN_ENABLE,
> > +                            hdev->suspend_tasks)) {
> > +             wake_up(&hdev->suspend_wait_q);
> > +     }
> > +}
> > +
> > /* Call with hci_dev_lock */
> > void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> > {
> > @@ -932,6 +1087,44 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> >
> >       hdev->suspend_state = next;
> >
> > +     hci_req_init(&req, hdev);
> > +     if (next == BT_SUSPENDED) {
> > +             /* Enable event filter for existing devices */
> > +             hci_req_set_event_filter(&req);
> > +
> > +             /* Enable passive scan at lower duty cycle */
> > +             hci_req_enable_le_suspend_scan(&req, LE_SCAN_FLAG_SUSPEND);
> > +
> > +             hdev->freeze_filters = true;
> > +
> > +             /* Run commands before disconnecting */
> > +             hci_req_run(&req, le_suspend_req_complete);
> > +
> > +             hdev->disconnect_counter = 0;
> > +             /* Soft disconnect everything (power off)*/
> > +             list_for_each_entry(conn, &hdev->conn_hash.list, list) {
> > +                     hci_disconnect(conn, HCI_ERROR_REMOTE_POWER_OFF);
> > +                     hdev->disconnect_counter++;
> > +             }
> > +
> > +             if (hdev->disconnect_counter > 0) {
> > +                     BT_DBG("Had %d disconnects. Will wait on them",
> > +                            hdev->disconnect_counter);
> > +                     set_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks);
> > +             }
> > +     } else {
> > +             hdev->freeze_filters = false;
> > +
> > +             hci_req_clear_event_filter(&req);
> > +
> > +             /* Reset passive/background scanning to normal */
> > +             hci_req_enable_le_suspend_scan(&req, 0);
> > +
> > +             hci_req_run(&req, le_suspend_req_complete);
> > +     }
> > +
> > +     hdev->suspend_state = next;
> > +
> > done:
> >       clear_bit(SUSPEND_PREPARE_NOTIFIER, hdev->suspend_tasks);
> >       wake_up(&hdev->suspend_wait_q);
> > @@ -2034,6 +2227,9 @@ void __hci_req_update_scan(struct hci_request *req)
> >       if (mgmt_powering_down(hdev))
> >               return;
> >
> > +     if (hdev->freeze_filters)
> > +             return;
> > +
> >       if (hci_dev_test_flag(hdev, HCI_CONNECTABLE) ||
> >           disconnected_whitelist_entries(hdev))
> >               scan = SCAN_PAGE;
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 58468dfa112f..269ce70e501c 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -7451,6 +7451,14 @@ void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
> >
> >       mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, &ev, sizeof(ev), sk);
> >
> > +     if (hdev->disconnect_counter > 0) {
> > +             hdev->disconnect_counter--;
> > +             if (hdev->disconnect_counter <= 0) {
> > +                     clear_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks);
> > +                     wake_up(&hdev->suspend_wait_q);
> > +             }
> > +     }
> > +
>
> I think this disconnect_counter is a bit racy. I would rather check that our connection hash is empty.
>
> In addition, I have the feeling we better disable LE scanning BR/EDR page scan first. So that we clearly do not have any artifacts of accidental incoming connections.
>
> The first step when going to suspend should clean the house
>
>         1) Set a flag that all BR/EDR and LE connection attempts are rejected
>         2) Disable BR/EDR page scan
>         3) Disable LE advertising
>         2) Disable LE scanning / BR/EDR inquiry
>         4) Disconnect all
>
> Now we have a clean house and we now prepare for wakeup triggers
>
>         1) Clear flag and allow incoming connection
>         2) Set BR/EDR event filter and enable page scan if needed
>         3) Set LE whitelist and enable scanning if needed
>
> When we wake up, then we also just need to reset the filters
>
>         1) Clear BR/EDR event filter and enable page scan if needed
>         2) Update LE white and enable scanning if needed
>         3) Restore LE scanning / BR/EDR inquiry
>

I think this is reasonable so I will try to implement it. Might take
me a couple of days (traveling out of country) so expect the next
patch series early next week.

> Regards
>
> Marcel
>

Cheers
Abhishek

Re: [RFC PATCH v2 3/4] Bluetooth: Update filters/whitelists for suspend

[Marcel Holtmann]

Hi Abhishek,

>>> When suspending, update the event filter for BR/EDR devices and the
>>> whitelist for LE devices. BR/EDR devices are added to the event filter
>>> and will auto-connect if found during suspend. For LE, we update the
>>> filter to remove everything that is not wakeable during suspend.
>>> Finally, we disconnect all connected devices and wait for that to
>>> complete before returning in the suspend notifier.
>>> 
>>> An example suspend flow with 1 BR/EDR HID device, 1 BR/EDR audio device,
>>> 1 LE HID device, 1 LE non-HID (where HIDs are wakeable):
>>> 
>>> PM_PREPARE_SUSPEND:
>>> - Set event filter for BR/EDR HID device
>>> - Clear anything from LE whitelist that is not wakeable
>>> - Add (if not there) the LE HID device
>>> - Disconnect all devices
>> 
>> is this really the right order? Why not disconnect all devices first. We are suspending after all, so lets disconnect first and then create the appropriate accept filters.
>> 
>> I have the feeling it would also make the overall logic a lot simpler since it fits more into the current model that is used internally for a lot of decision points (especially with LE).
>> 
>>> PM_POST_SUSPEND:
>>> - Clear event filter
>>> - Restore LE whitelist (deleting anything not in le_pend_conn or
>>>   le_pend_report and then adding from those lists)
>>> 
>>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>>> ---
>>> 
>>> Changes in v2:
>>> * Refactored filters and whitelist settings to its own patch
>>> * Refactored update_white_list to have clearer edge cases
>>> * Add connected devices to whitelist (previously missing corner case)
>>> 
>>> include/net/bluetooth/hci.h      |  17 +-
>>> include/net/bluetooth/hci_core.h |   5 +
>>> net/bluetooth/hci_event.c        |  28 ++-
>>> net/bluetooth/hci_request.c      | 308 +++++++++++++++++++++++++------
>>> net/bluetooth/mgmt.c             |   8 +
>>> 5 files changed, 298 insertions(+), 68 deletions(-)
>>> 
>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>> index 6293bdd7d862..720d8e633f7e 100644
>>> --- a/include/net/bluetooth/hci.h
>>> +++ b/include/net/bluetooth/hci.h
>>> @@ -932,10 +932,14 @@ struct hci_cp_sniff_subrate {
>>> #define HCI_OP_RESET                  0x0c03
>>> 
>>> #define HCI_OP_SET_EVENT_FLT          0x0c05
>>> -struct hci_cp_set_event_flt {
>>> -     __u8     flt_type;
>>> -     __u8     cond_type;
>>> -     __u8     condition[0];
>>> +#define HCI_SET_EVENT_FLT_SIZE               9
>>> +struct hci_cp_set_event_filter {
>>> +     __u8            flt_type;
>>> +     __u8            cond_type;
>>> +     struct {
>>> +             bdaddr_t bdaddr;
>>> +             __u8 auto_accept;
>>> +     } __packed      addr_conn_flt;
>>> } __packed;
>>> 
>>> /* Filter types */
>>> @@ -949,8 +953,9 @@ struct hci_cp_set_event_flt {
>>> #define HCI_CONN_SETUP_ALLOW_BDADDR   0x02
>>> 
>>> /* CONN_SETUP Conditions */
>>> -#define HCI_CONN_SETUP_AUTO_OFF      0x01
>>> -#define HCI_CONN_SETUP_AUTO_ON       0x02
>>> +#define HCI_CONN_SETUP_AUTO_OFF              0x01
>>> +#define HCI_CONN_SETUP_AUTO_ON               0x02
>>> +#define HCI_CONN_SETUP_AUTO_ON_WITH_RS       0x03
>>> 
>>> #define HCI_OP_READ_STORED_LINK_KEY   0x0c0d
>>> struct hci_cp_read_stored_link_key {
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index 74d703e46fb4..49eae4a802ac 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -91,6 +91,9 @@ struct discovery_state {
>>> #define SUSPEND_NOTIFIER_TIMEOUT      msecs_to_jiffies(2000) /* 2 seconds */
>>> 
>>> enum suspend_tasks {
>>> +     SUSPEND_LE_SET_SCAN_ENABLE,
>>> +     SUSPEND_DISCONNECTING,
>>> +
>>>      SUSPEND_PREPARE_NOTIFIER,
>>>      __SUSPEND_NUM_TASKS
>>> };
>>> @@ -406,6 +409,8 @@ struct hci_dev {
>>>      struct work_struct      suspend_prepare;
>>>      enum suspended_state    suspend_state_next;
>>>      enum suspended_state    suspend_state;
>>> +     int                     disconnect_counter;
>>> +     bool                    freeze_filters;
>>> 
>>>      wait_queue_head_t       suspend_wait_q;
>>>      DECLARE_BITMAP(suspend_tasks, __SUSPEND_NUM_TASKS);
>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>> index 6ddc4a74a5e4..76d25b3f4c73 100644
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -2474,6 +2474,7 @@ static void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
>>> static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>>> {
>>>      struct hci_ev_conn_complete *ev = (void *) skb->data;
>>> +     struct inquiry_entry *ie;
>>>      struct hci_conn *conn;
>>> 
>>>      BT_DBG("%s", hdev->name);
>>> @@ -2482,14 +2483,29 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>>> 
>>>      conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
>>>      if (!conn) {
>>> -             if (ev->link_type != SCO_LINK)
>>> -                     goto unlock;
>>> +             /* Connection may not exist if auto-connected. Check the inquiry
>>> +              * cache to see if we've already discovered this bdaddr before.
>>> +              * Create a new connection if it was previously discovered.
>>> +              */
>>> +             ie = hci_inquiry_cache_lookup(hdev, &ev->bdaddr);
>>> +             if (ie) {
>>> +                     conn = hci_conn_add(hdev, ev->link_type, &ev->bdaddr,
>>> +                                         HCI_ROLE_SLAVE);
>>> +                     if (!conn) {
>>> +                             bt_dev_err(hdev, "no memory for new conn");
>>> +                             goto unlock;
>>> +                     }
>>> +             } else {
>>> +                     if (ev->link_type != SCO_LINK)
>>> +                             goto unlock;
>> 
>> We are not going to get SCO_LINK auto-connected and thus we should really have it done just for ACL links. I don’t have good proposal to structure this handling at the moment, but we need to clean this up a bit.
> 
> Ack, will look deeper into this.
> 
>> 
>>> -             conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK, &ev->bdaddr);
>>> -             if (!conn)
>>> -                     goto unlock;
>>> +                     conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK,
>>> +                                                    &ev->bdaddr);
>>> +                     if (!conn)
>>> +                             goto unlock;
>>> 
>>> -             conn->type = SCO_LINK;
>>> +                     conn->type = SCO_LINK;
>>> +             }
>>>      }
>>> 
>>>      if (!ev->status) {
>>> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
>>> index 08908469c043..c930b9ff1cfd 100644
>>> --- a/net/bluetooth/hci_request.c
>>> +++ b/net/bluetooth/hci_request.c
>>> @@ -34,6 +34,12 @@
>>> #define HCI_REQ_PEND    1
>>> #define HCI_REQ_CANCELED  2
>>> 
>>> +#define LE_SCAN_FLAG_SUSPEND 0x1
>>> +#define LE_SCAN_FLAG_ALLOW_RPA       0x2
>>> +
>>> +#define LE_SUSPEND_SCAN_WINDOW               0x0012
>>> +#define LE_SUSPEND_SCAN_INTERVAL     0x0060
>>> +
>>> void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
>>> {
>>>      skb_queue_head_init(&req->cmd_q);
>>> @@ -654,6 +660,12 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
>>> {
>>>      struct hci_dev *hdev = req->hdev;
>>> 
>>> +     /* Early exit if we've frozen filters for suspend*/
>> 
>> Please fix the coding style for comments. Extra space required between suspend and */
>> 
>> I also have the feeling that we should split BR/EDR from LE support. I think we can merge the BR/EDR support quicker since it is a lot simpler. Especially when it stands on its own.
> 
> Ack. I kept them merged because I didn't want to change the request
> completion function but I can separate them.
> 
>> 
>>> +     if (hdev->freeze_filters) {
>>> +             BT_DBG("Filters are frozen for suspend");
>>> +             return;
>>> +     }
>>> +
>>>      if (use_ext_scan(hdev)) {
>>>              struct hci_cp_le_set_ext_scan_enable cp;
>>> 
>>> @@ -670,23 +682,67 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
>>>      }
>>> }
>>> 
>>> -static void add_to_white_list(struct hci_request *req,
>>> -                           struct hci_conn_params *params)
>>> +static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr,
>>> +                             u8 bdaddr_type)
>>> +{
>>> +     struct hci_cp_le_del_from_white_list cp;
>>> +
>>> +     cp.bdaddr_type = bdaddr_type;
>>> +     bacpy(&cp.bdaddr, bdaddr);
>>> +
>>> +     BT_DBG("Remove %pMR (0x%x) from whitelist", &cp.bdaddr, cp.bdaddr_type);
>>> +     hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp);
>>> +}
>>> +
>>> +/* Adds connection to white list if needed. On error, returns -1 */
>>> +static int add_to_white_list(struct hci_request *req,
>>> +                          struct hci_conn_params *params, u8 *num_entries,
>>> +                          u8 flags)
>>> {
>>>      struct hci_cp_le_add_to_white_list cp;
>>> +     struct hci_dev *hdev = req->hdev;
>>> +     bool allow_rpa = !!(flags & LE_SCAN_FLAG_ALLOW_RPA);
>>> +     bool suspend = !!(flags & LE_SCAN_FLAG_SUSPEND);
>>> 
>>> +     /* Already in white list */
>>> +     if (hci_bdaddr_list_lookup(&hdev->le_white_list, &params->addr,
>>> +                                params->addr_type))
>>> +             return 0;
>>> +
>>> +     /* Select filter policy to accept all advertising */
>>> +     if (*num_entries >= hdev->le_white_list_size)
>>> +             return -1;
>>> +
>>> +     /* White list can not be used with RPAs */
>>> +     if (!allow_rpa &&
>>> +         hci_find_irk_by_addr(hdev, &params->addr, params->addr_type)) {
>>> +             return -1;
>>> +     }
>>> +
>>> +     /* During suspend, only wakeable devices can be in whitelist */
>>> +     if (suspend && !hci_bdaddr_list_lookup(&hdev->wakeable, &params->addr,
>>> +                                            params->addr_type))
>>> +             return 0;
>>> +
>>> +     *num_entries += 1;
>>>      cp.bdaddr_type = params->addr_type;
>>>      bacpy(&cp.bdaddr, &params->addr);
>>> 
>>> +     BT_DBG("Add %pMR (0x%x) to whitelist", &cp.bdaddr, cp.bdaddr_type);
>>>      hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp);
>>> +
>>> +     return 0;
>>> }
>>> 
>>> -static u8 update_white_list(struct hci_request *req)
>>> +static u8 update_white_list(struct hci_request *req, u8 flags)
>>> {
>>>      struct hci_dev *hdev = req->hdev;
>>>      struct hci_conn_params *params;
>>>      struct bdaddr_list *b;
>>> -     uint8_t white_list_entries = 0;
>>> +     u8 white_list_entries = 0;
>>> +     bool allow_rpa = !!(flags & LE_SCAN_FLAG_ALLOW_RPA);
>>> +     bool suspend = !!(flags & LE_SCAN_FLAG_SUSPEND);
>>> +     bool wakeable, pend_conn, pend_report;
>>> 
>>>      /* Go through the current white list programmed into the
>>>       * controller one by one and check if that address is still
>>> @@ -695,26 +751,42 @@ static u8 update_white_list(struct hci_request *req)
>>>       * command to remove it from the controller.
>>>       */
>>>      list_for_each_entry(b, &hdev->le_white_list, list) {
>>> -             /* If the device is neither in pend_le_conns nor
>>> -              * pend_le_reports then remove it from the whitelist.
>>> +             wakeable = !!hci_bdaddr_list_lookup(&hdev->wakeable, &b->bdaddr,
>>> +                                                 b->bdaddr_type);
>>> +             pend_conn = hci_pend_le_action_lookup(&hdev->pend_le_conns,
>>> +                                                   &b->bdaddr,
>>> +                                                   b->bdaddr_type);
>>> +             pend_report = hci_pend_le_action_lookup(&hdev->pend_le_reports,
>>> +                                                     &b->bdaddr,
>>> +                                                     b->bdaddr_type);
>>> +
>>> +             /* During suspend, we remove all non-wakeable devices
>>> +              * and leave all others alone. Connected devices will be
>>> +              * disconnected during suspend but may not be in the pending
>>> +              * list yet.
>>>               */
>>> -             if (!hci_pend_le_action_lookup(&hdev->pend_le_conns,
>>> -                                            &b->bdaddr, b->bdaddr_type) &&
>>> -                 !hci_pend_le_action_lookup(&hdev->pend_le_reports,
>>> -                                            &b->bdaddr, b->bdaddr_type)) {
>>> -                     struct hci_cp_le_del_from_white_list cp;
>>> -
>>> -                     cp.bdaddr_type = b->bdaddr_type;
>>> -                     bacpy(&cp.bdaddr, &b->bdaddr);
>>> -
>>> -                     hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST,
>>> -                                 sizeof(cp), &cp);
>>> -                     continue;
>>> -             }
>>> +             if (suspend) {
>>> +                     if (!wakeable) {
>>> +                             del_from_white_list(req, &b->bdaddr,
>>> +                                                 b->bdaddr_type);
>>> +                             continue;
>>> +                     }
>>> +             } else {
>>> +                     /* If the device is not likely to connect or report,
>>> +                      * remove it from the whitelist.
>>> +                      */
>>> +                     if (!pend_conn && !pend_report) {
>>> +                             del_from_white_list(req, &b->bdaddr,
>>> +                                                 b->bdaddr_type);
>>> +                             continue;
>>> +                     }
>>> 
>>> -             if (hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) {
>>>                      /* White list can not be used with RPAs */
>>> -                     return 0x00;
>>> +                     if (!allow_rpa &&
>>> +                         hci_find_irk_by_addr(hdev, &b->bdaddr,
>>> +                                              b->bdaddr_type)) {
>>> +                             return 0x00;
>>> +                     }
>>>              }
>>> 
>>>              white_list_entries++;
>>> @@ -731,47 +803,30 @@ static u8 update_white_list(struct hci_request *req)
>>>       * white list.
>>>       */
>>>      list_for_each_entry(params, &hdev->pend_le_conns, action) {
>>> -             if (hci_bdaddr_list_lookup(&hdev->le_white_list,
>>> -                                        &params->addr, params->addr_type))
>>> -                     continue;
>>> -
>>> -             if (white_list_entries >= hdev->le_white_list_size) {
>>> -                     /* Select filter policy to accept all advertising */
>>> +             if (add_to_white_list(req, params, &white_list_entries, flags))
>>>                      return 0x00;
>>> -             }
>>> -
>>> -             if (hci_find_irk_by_addr(hdev, &params->addr,
>>> -                                      params->addr_type)) {
>>> -                     /* White list can not be used with RPAs */
>>> -                     return 0x00;
>>> -             }
>>> -
>>> -             white_list_entries++;
>>> -             add_to_white_list(req, params);
>>>      }
>>> 
>>>      /* After adding all new pending connections, walk through
>>>       * the list of pending reports and also add these to the
>>> -      * white list if there is still space.
>>> +      * white list if there is still space. Abort if space runs out.
>>>       */
>>>      list_for_each_entry(params, &hdev->pend_le_reports, action) {
>>> -             if (hci_bdaddr_list_lookup(&hdev->le_white_list,
>>> -                                        &params->addr, params->addr_type))
>>> -                     continue;
>>> -
>>> -             if (white_list_entries >= hdev->le_white_list_size) {
>>> -                     /* Select filter policy to accept all advertising */
>>> +             if (add_to_white_list(req, params, &white_list_entries, flags))
>>>                      return 0x00;
>>> -             }
>>> +     }
>>> 
>>> -             if (hci_find_irk_by_addr(hdev, &params->addr,
>>> -                                      params->addr_type)) {
>>> -                     /* White list can not be used with RPAs */
>>> -                     return 0x00;
>>> +     /* Currently connected devices will be missing from the white list and
>>> +      * we need to insert them into the whitelist if they are wakeable. We
>>> +      * can't insert later because we will have already returned from the
>>> +      * suspend notifier and would cause a spurious wakeup.
>>> +      */
>>> +     if (suspend) {
>>> +             list_for_each_entry(params, &hdev->le_conn_params, list) {
>>> +                     if (add_to_white_list(req, params, &white_list_entries,
>>> +                                           flags))
>>> +                             return 0x00;
>>>              }
>>> -
>>> -             white_list_entries++;
>>> -             add_to_white_list(req, params);
>>>      }
>> 
>> This goes to my point above. If we just disconnect everything, then the white list programming should be easily. Everything that is pending and have been elected for wakeup + incoming connection will be added. And then we are done. I don’t think much has to change except one extra check if wakeup is allowed or not.
>> 
>>> 
>>>      /* Select filter policy to use white list */
>>> @@ -861,11 +916,26 @@ static void hci_req_start_scan(struct hci_request *req, u8 type, u16 interval,
>>>      }
>>> }
>>> 
>>> -void hci_req_add_le_passive_scan(struct hci_request *req)
>>> +void __hci_req_add_le_passive_scan(struct hci_request *req, u8 flags)
>>> {
>>>      struct hci_dev *hdev = req->hdev;
>>>      u8 own_addr_type;
>>>      u8 filter_policy;
>>> +     u8 window, interval;
>>> +
>>> +     /* We allow whitelisting even with RPAs in suspend. In the worst case,
>>> +      * we won't be able to wake from devices that use the privacy1.2
>>> +      * features. Additionally, once we support privacy1.2 and IRK
>>> +      * offloading, we can update this to also check for those conditions.
>>> +      */
>>> +     if (flags & LE_SCAN_FLAG_SUSPEND)
>>> +             flags |= LE_SCAN_FLAG_ALLOW_RPA;
>>> +
>>> +     /* Early exit if we've frozen filters for suspend */
>>> +     if (hdev->freeze_filters) {
>>> +             BT_DBG("Filters are frozen for suspend");
>>> +             return;
>>> +     }
>>> 
>>>      /* Set require_privacy to false since no SCAN_REQ are send
>>>       * during passive scanning. Not using an non-resolvable address
>>> @@ -881,7 +951,8 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
>>>       * happen before enabling scanning. The controller does
>>>       * not allow white list modification while scanning.
>>>       */
>>> -     filter_policy = update_white_list(req);
>>> +     BT_DBG("Updating white list with flags = %d", flags);
>>> +     filter_policy = update_white_list(req, flags);
>>> 
>>>      /* When the controller is using random resolvable addresses and
>>>       * with that having LE privacy enabled, then controllers with
>>> @@ -896,8 +967,22 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
>>>          (hdev->le_features[0] & HCI_LE_EXT_SCAN_POLICY))
>>>              filter_policy |= 0x02;
>>> 
>>> -     hci_req_start_scan(req, LE_SCAN_PASSIVE, hdev->le_scan_interval,
>>> -                        hdev->le_scan_window, own_addr_type, filter_policy);
>>> +     if (flags & LE_SCAN_FLAG_SUSPEND) {
>>> +             window = LE_SUSPEND_SCAN_WINDOW;
>>> +             interval = LE_SUSPEND_SCAN_INTERVAL;
>> 
>> I think we just want this done via some hdev->suspended variable or similar.
> 
> In support of setting a separate flag, it is easier to grep for when a
> specific flag is being used and passed to a function than it is to
> check when a variable is being set in a global context variable.
> 
>> 
>>> +     } else {
>>> +             window = hdev->le_scan_window;
>>> +             interval = hdev->le_scan_interval;
>>> +     }
>>> +
>>> +     BT_DBG("LE passive scan with whitelist = %d", filter_policy);
>>> +     hci_req_start_scan(req, LE_SCAN_PASSIVE, interval, window,
>>> +                        own_addr_type, filter_policy);
>>> +}
>>> +
>>> +void hci_req_add_le_passive_scan(struct hci_request *req)
>>> +{
>>> +     __hci_req_add_le_passive_scan(req, 0);
>>> }
>>> 
>>> static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
>>> @@ -918,6 +1003,76 @@ static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
>>>      return adv_instance->scan_rsp_len;
>>> }
>>> 
>>> +static void hci_req_clear_event_filter(struct hci_request *req)
>>> +{
>>> +     struct hci_cp_set_event_filter f;
>>> +
>>> +     memset(&f, 0, sizeof(f));
>>> +     f.flt_type = HCI_FLT_CLEAR_ALL;
>>> +     hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &f);
>>> +
>>> +     /* Update page scan state (since we may have modified it when setting
>>> +      * the event filter).
>>> +      */
>>> +     __hci_req_update_scan(req);
>>> +}
>>> +
>>> +static void hci_req_set_event_filter(struct hci_request *req)
>>> +{
>>> +     struct bdaddr_list *b;
>>> +     struct hci_cp_set_event_filter f;
>>> +     struct hci_dev *hdev = req->hdev;
>>> +     int filters_updated = 0;
>>> +     u8 scan;
>>> +
>>> +     /* Always clear event filter when starting */
>>> +     hci_req_clear_event_filter(req);
>>> +
>>> +     list_for_each_entry(b, &hdev->wakeable, list) {
>>> +             if (b->bdaddr_type != BDADDR_BREDR)
>>> +                     continue;
>>> +
>>> +             memset(&f, 0, sizeof(f));
>>> +             bacpy(&f.addr_conn_flt.bdaddr, &b->bdaddr);
>>> +             f.flt_type = HCI_FLT_CONN_SETUP;
>>> +             f.cond_type = HCI_CONN_SETUP_ALLOW_BDADDR;
>>> +             f.addr_conn_flt.auto_accept = HCI_CONN_SETUP_AUTO_ON;
>>> +
>>> +             BT_DBG("Adding event filters for %pMR", &b->bdaddr);
>>> +             hci_req_add(req, HCI_OP_SET_EVENT_FLT, sizeof(f), &f);
>>> +
>>> +             filters_updated++;
>>> +     }
>> 
>> If we would use the wakeable list just for BR/EDR then the filters_updated++ would become redundant and we could just check if the list is empty.
>> 
>>> +
>>> +     scan = filters_updated ? SCAN_PAGE : SCAN_DISABLED;
>>> +     hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
>>> +}
>>> +
>>> +static void hci_req_enable_le_suspend_scan(struct hci_request *req,
>>> +                                        u8 flags)
>>> +{
>>> +     /* Can't change params without disabling first */
>>> +     hci_req_add_le_scan_disable(req);
>>> +
>>> +     /* Configure params and enable scanning */
>>> +     __hci_req_add_le_passive_scan(req, flags);
>>> +
>>> +     /* Block suspend notifier on response */
>>> +     set_bit(SUSPEND_LE_SET_SCAN_ENABLE, req->hdev->suspend_tasks);
>>> +}
>>> +
>>> +static void le_suspend_req_complete(struct hci_dev *hdev, u8 status, u16 opcode)
>>> +{
>>> +     BT_DBG("Request complete opcode=0x%x, status=0x%x", opcode, status);
>>> +
>>> +     /* Expecting LE Set scan to return */
>>> +     if (opcode == HCI_OP_LE_SET_SCAN_ENABLE &&
>>> +         test_and_clear_bit(SUSPEND_LE_SET_SCAN_ENABLE,
>>> +                            hdev->suspend_tasks)) {
>>> +             wake_up(&hdev->suspend_wait_q);
>>> +     }
>>> +}
>>> +
>>> /* Call with hci_dev_lock */
>>> void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
>>> {
>>> @@ -932,6 +1087,44 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
>>> 
>>>      hdev->suspend_state = next;
>>> 
>>> +     hci_req_init(&req, hdev);
>>> +     if (next == BT_SUSPENDED) {
>>> +             /* Enable event filter for existing devices */
>>> +             hci_req_set_event_filter(&req);
>>> +
>>> +             /* Enable passive scan at lower duty cycle */
>>> +             hci_req_enable_le_suspend_scan(&req, LE_SCAN_FLAG_SUSPEND);
>>> +
>>> +             hdev->freeze_filters = true;
>>> +
>>> +             /* Run commands before disconnecting */
>>> +             hci_req_run(&req, le_suspend_req_complete);
>>> +
>>> +             hdev->disconnect_counter = 0;
>>> +             /* Soft disconnect everything (power off)*/
>>> +             list_for_each_entry(conn, &hdev->conn_hash.list, list) {
>>> +                     hci_disconnect(conn, HCI_ERROR_REMOTE_POWER_OFF);
>>> +                     hdev->disconnect_counter++;
>>> +             }
>>> +
>>> +             if (hdev->disconnect_counter > 0) {
>>> +                     BT_DBG("Had %d disconnects. Will wait on them",
>>> +                            hdev->disconnect_counter);
>>> +                     set_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks);
>>> +             }
>>> +     } else {
>>> +             hdev->freeze_filters = false;
>>> +
>>> +             hci_req_clear_event_filter(&req);
>>> +
>>> +             /* Reset passive/background scanning to normal */
>>> +             hci_req_enable_le_suspend_scan(&req, 0);
>>> +
>>> +             hci_req_run(&req, le_suspend_req_complete);
>>> +     }
>>> +
>>> +     hdev->suspend_state = next;
>>> +
>>> done:
>>>      clear_bit(SUSPEND_PREPARE_NOTIFIER, hdev->suspend_tasks);
>>>      wake_up(&hdev->suspend_wait_q);
>>> @@ -2034,6 +2227,9 @@ void __hci_req_update_scan(struct hci_request *req)
>>>      if (mgmt_powering_down(hdev))
>>>              return;
>>> 
>>> +     if (hdev->freeze_filters)
>>> +             return;
>>> +
>>>      if (hci_dev_test_flag(hdev, HCI_CONNECTABLE) ||
>>>          disconnected_whitelist_entries(hdev))
>>>              scan = SCAN_PAGE;
>>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>>> index 58468dfa112f..269ce70e501c 100644
>>> --- a/net/bluetooth/mgmt.c
>>> +++ b/net/bluetooth/mgmt.c
>>> @@ -7451,6 +7451,14 @@ void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
>>> 
>>>      mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, &ev, sizeof(ev), sk);
>>> 
>>> +     if (hdev->disconnect_counter > 0) {
>>> +             hdev->disconnect_counter--;
>>> +             if (hdev->disconnect_counter <= 0) {
>>> +                     clear_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks);
>>> +                     wake_up(&hdev->suspend_wait_q);
>>> +             }
>>> +     }
>>> +
>> 
>> I think this disconnect_counter is a bit racy. I would rather check that our connection hash is empty.
>> 
>> In addition, I have the feeling we better disable LE scanning BR/EDR page scan first. So that we clearly do not have any artifacts of accidental incoming connections.
>> 
>> The first step when going to suspend should clean the house
>> 
>>        1) Set a flag that all BR/EDR and LE connection attempts are rejected
>>        2) Disable BR/EDR page scan
>>        3) Disable LE advertising
>>        2) Disable LE scanning / BR/EDR inquiry
>>        4) Disconnect all
>> 
>> Now we have a clean house and we now prepare for wakeup triggers
>> 
>>        1) Clear flag and allow incoming connection
>>        2) Set BR/EDR event filter and enable page scan if needed
>>        3) Set LE whitelist and enable scanning if needed
>> 
>> When we wake up, then we also just need to reset the filters
>> 
>>        1) Clear BR/EDR event filter and enable page scan if needed
>>        2) Update LE white and enable scanning if needed
>>        3) Restore LE scanning / BR/EDR inquiry
>> 
> 
> I think this is reasonable so I will try to implement it. Might take
> me a couple of days (traveling out of country) so expect the next
> patch series early next week.

I just wanted to quickly check if you run into any troubles with the changes?

Regards

Marcel

Re: [RFC PATCH v2 3/4] Bluetooth: Update filters/whitelists for suspend

[Abhishek Pandit-Subedi]

Hey Marcel,

I was working on other changes and didn't get a chance to come back to
this yet. I will be working on this refactoring this week so please
expect another patch series by the end of the week.

Thanks
Abhishek

On Thu, Feb 13, 2020 at 12:29 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> >>> When suspending, update the event filter for BR/EDR devices and the
> >>> whitelist for LE devices. BR/EDR devices are added to the event filter
> >>> and will auto-connect if found during suspend. For LE, we update the
> >>> filter to remove everything that is not wakeable during suspend.
> >>> Finally, we disconnect all connected devices and wait for that to
> >>> complete before returning in the suspend notifier.
> >>>
> >>> An example suspend flow with 1 BR/EDR HID device, 1 BR/EDR audio device,
> >>> 1 LE HID device, 1 LE non-HID (where HIDs are wakeable):
> >>>
> >>> PM_PREPARE_SUSPEND:
> >>> - Set event filter for BR/EDR HID device
> >>> - Clear anything from LE whitelist that is not wakeable
> >>> - Add (if not there) the LE HID device
> >>> - Disconnect all devices
> >>
> >> is this really the right order? Why not disconnect all devices first. We are suspending after all, so lets disconnect first and then create the appropriate accept filters.
> >>
> >> I have the feeling it would also make the overall logic a lot simpler since it fits more into the current model that is used internally for a lot of decision points (especially with LE).
> >>
> >>> PM_POST_SUSPEND:
> >>> - Clear event filter
> >>> - Restore LE whitelist (deleting anything not in le_pend_conn or
> >>>   le_pend_report and then adding from those lists)
> >>>
> >>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> * Refactored filters and whitelist settings to its own patch
> >>> * Refactored update_white_list to have clearer edge cases
> >>> * Add connected devices to whitelist (previously missing corner case)
> >>>
> >>> include/net/bluetooth/hci.h      |  17 +-
> >>> include/net/bluetooth/hci_core.h |   5 +
> >>> net/bluetooth/hci_event.c        |  28 ++-
> >>> net/bluetooth/hci_request.c      | 308 +++++++++++++++++++++++++------
> >>> net/bluetooth/mgmt.c             |   8 +
> >>> 5 files changed, 298 insertions(+), 68 deletions(-)
> >>>
> >>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> >>> index 6293bdd7d862..720d8e633f7e 100644
> >>> --- a/include/net/bluetooth/hci.h
> >>> +++ b/include/net/bluetooth/hci.h
> >>> @@ -932,10 +932,14 @@ struct hci_cp_sniff_subrate {
> >>> #define HCI_OP_RESET                  0x0c03
> >>>
> >>> #define HCI_OP_SET_EVENT_FLT          0x0c05
> >>> -struct hci_cp_set_event_flt {
> >>> -     __u8     flt_type;
> >>> -     __u8     cond_type;
> >>> -     __u8     condition[0];
> >>> +#define HCI_SET_EVENT_FLT_SIZE               9
> >>> +struct hci_cp_set_event_filter {
> >>> +     __u8            flt_type;
> >>> +     __u8            cond_type;
> >>> +     struct {
> >>> +             bdaddr_t bdaddr;
> >>> +             __u8 auto_accept;
> >>> +     } __packed      addr_conn_flt;
> >>> } __packed;
> >>>
> >>> /* Filter types */
> >>> @@ -949,8 +953,9 @@ struct hci_cp_set_event_flt {
> >>> #define HCI_CONN_SETUP_ALLOW_BDADDR   0x02
> >>>
> >>> /* CONN_SETUP Conditions */
> >>> -#define HCI_CONN_SETUP_AUTO_OFF      0x01
> >>> -#define HCI_CONN_SETUP_AUTO_ON       0x02
> >>> +#define HCI_CONN_SETUP_AUTO_OFF              0x01
> >>> +#define HCI_CONN_SETUP_AUTO_ON               0x02
> >>> +#define HCI_CONN_SETUP_AUTO_ON_WITH_RS       0x03
> >>>
> >>> #define HCI_OP_READ_STORED_LINK_KEY   0x0c0d
> >>> struct hci_cp_read_stored_link_key {
> >>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >>> index 74d703e46fb4..49eae4a802ac 100644
> >>> --- a/include/net/bluetooth/hci_core.h
> >>> +++ b/include/net/bluetooth/hci_core.h
> >>> @@ -91,6 +91,9 @@ struct discovery_state {
> >>> #define SUSPEND_NOTIFIER_TIMEOUT      msecs_to_jiffies(2000) /* 2 seconds */
> >>>
> >>> enum suspend_tasks {
> >>> +     SUSPEND_LE_SET_SCAN_ENABLE,
> >>> +     SUSPEND_DISCONNECTING,
> >>> +
> >>>      SUSPEND_PREPARE_NOTIFIER,
> >>>      __SUSPEND_NUM_TASKS
> >>> };
> >>> @@ -406,6 +409,8 @@ struct hci_dev {
> >>>      struct work_struct      suspend_prepare;
> >>>      enum suspended_state    suspend_state_next;
> >>>      enum suspended_state    suspend_state;
> >>> +     int                     disconnect_counter;
> >>> +     bool                    freeze_filters;
> >>>
> >>>      wait_queue_head_t       suspend_wait_q;
> >>>      DECLARE_BITMAP(suspend_tasks, __SUSPEND_NUM_TASKS);
> >>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >>> index 6ddc4a74a5e4..76d25b3f4c73 100644
> >>> --- a/net/bluetooth/hci_event.c
> >>> +++ b/net/bluetooth/hci_event.c
> >>> @@ -2474,6 +2474,7 @@ static void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >>> static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >>> {
> >>>      struct hci_ev_conn_complete *ev = (void *) skb->data;
> >>> +     struct inquiry_entry *ie;
> >>>      struct hci_conn *conn;
> >>>
> >>>      BT_DBG("%s", hdev->name);
> >>> @@ -2482,14 +2483,29 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >>>
> >>>      conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
> >>>      if (!conn) {
> >>> -             if (ev->link_type != SCO_LINK)
> >>> -                     goto unlock;
> >>> +             /* Connection may not exist if auto-connected. Check the inquiry
> >>> +              * cache to see if we've already discovered this bdaddr before.
> >>> +              * Create a new connection if it was previously discovered.
> >>> +              */
> >>> +             ie = hci_inquiry_cache_lookup(hdev, &ev->bdaddr);
> >>> +             if (ie) {
> >>> +                     conn = hci_conn_add(hdev, ev->link_type, &ev->bdaddr,
> >>> +                                         HCI_ROLE_SLAVE);
> >>> +                     if (!conn) {
> >>> +                             bt_dev_err(hdev, "no memory for new conn");
> >>> +                             goto unlock;
> >>> +                     }
> >>> +             } else {
> >>> +                     if (ev->link_type != SCO_LINK)
> >>> +                             goto unlock;
> >>
> >> We are not going to get SCO_LINK auto-connected and thus we should really have it done just for ACL links. I don’t have good proposal to structure this handling at the moment, but we need to clean this up a bit.
> >
> > Ack, will look deeper into this.
> >
> >>
> >>> -             conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK, &ev->bdaddr);
> >>> -             if (!conn)
> >>> -                     goto unlock;
> >>> +                     conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK,
> >>> +                                                    &ev->bdaddr);
> >>> +                     if (!conn)
> >>> +                             goto unlock;
> >>>
> >>> -             conn->type = SCO_LINK;
> >>> +                     conn->type = SCO_LINK;
> >>> +             }
> >>>      }
> >>>
> >>>      if (!ev->status) {
> >>> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> >>> index 08908469c043..c930b9ff1cfd 100644
> >>> --- a/net/bluetooth/hci_request.c
> >>> +++ b/net/bluetooth/hci_request.c
> >>> @@ -34,6 +34,12 @@
> >>> #define HCI_REQ_PEND    1
> >>> #define HCI_REQ_CANCELED  2
> >>>
> >>> +#define LE_SCAN_FLAG_SUSPEND 0x1
> >>> +#define LE_SCAN_FLAG_ALLOW_RPA       0x2
> >>> +
> >>> +#define LE_SUSPEND_SCAN_WINDOW               0x0012
> >>> +#define LE_SUSPEND_SCAN_INTERVAL     0x0060
> >>> +
> >>> void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
> >>> {
> >>>      skb_queue_head_init(&req->cmd_q);
> >>> @@ -654,6 +660,12 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
> >>> {
> >>>      struct hci_dev *hdev = req->hdev;
> >>>
> >>> +     /* Early exit if we've frozen filters for suspend*/
> >>
> >> Please fix the coding style for comments. Extra space required between suspend and */
> >>
> >> I also have the feeling that we should split BR/EDR from LE support. I think we can merge the BR/EDR support quicker since it is a lot simpler. Especially when it stands on its own.
> >
> > Ack. I kept them merged because I didn't want to change the request
> > completion function but I can separate them.
> >
> >>
> >>> +     if (hdev->freeze_filters) {
> >>> +             BT_DBG("Filters are frozen for suspend");
> >>> +             return;
> >>> +     }
> >>> +
> >>>      if (use_ext_scan(hdev)) {
> >>>              struct hci_cp_le_set_ext_scan_enable cp;
> >>>
> >>> @@ -670,23 +682,67 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
> >>>      }
> >>> }
> >>>
> >>> -static void add_to_white_list(struct hci_request *req,
> >>> -                           struct hci_conn_params *params)
> >>> +static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr,
> >>> +                             u8 bdaddr_type)
> >>> +{
> >>> +     struct hci_cp_le_del_from_white_list cp;
> >>> +
> >>> +     cp.bdaddr_type = bdaddr_type;
> >>> +     bacpy(&cp.bdaddr, bdaddr);
> >>> +
> >>> +     BT_DBG("Remove %pMR (0x%x) from whitelist", &cp.bdaddr, cp.bdaddr_type);
> >>> +     hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp);
> >>> +}
> >>> +
> >>> +/* Adds connection to white list if needed. On error, returns -1 */
> >>> +static int add_to_white_list(struct hci_request *req,
> >>> +                          struct hci_conn_params *params, u8 *num_entries,
> >>> +                          u8 flags)
> >>> {
> >>>      struct hci_cp_le_add_to_white_list cp;
> >>> +     struct hci_dev *hdev = req->hdev;
> >>> +     bool allow_rpa = !!(flags & LE_SCAN_FLAG_ALLOW_RPA);
> >>> +     bool suspend = !!(flags & LE_SCAN_FLAG_SUSPEND);
> >>>
> >>> +     /* Already in white list */
> >>> +     if (hci_bdaddr_list_lookup(&hdev->le_white_list, &params->addr,
> >>> +                                params->addr_type))
> >>> +             return 0;
> >>> +
> >>> +     /* Select filter policy to accept all advertising */
> >>> +     if (*num_entries >= hdev->le_white_list_size)
> >>> +             return -1;
> >>> +
> >>> +     /* White list can not be used with RPAs */
> >>> +     if (!allow_rpa &&
> >>> +         hci_find_irk_by_addr(hdev, &params->addr, params->addr_type)) {
> >>> +             return -1;
> >>> +     }
> >>> +
> >>> +     /* During suspend, only wakeable devices can be in whitelist */
> >>> +     if (suspend && !hci_bdaddr_list_lookup(&hdev->wakeable, &params->addr,
> >>> +                                            params->addr_type))
> >>> +             return 0;
> >>> +
> >>> +     *num_entries += 1;
> >>>      cp.bdaddr_type = params->addr_type;
> >>>      bacpy(&cp.bdaddr, &params->addr);
> >>>
> >>> +     BT_DBG("Add %pMR (0x%x) to whitelist", &cp.bdaddr, cp.bdaddr_type);
> >>>      hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp);
> >>> +
> >>> +     return 0;
> >>> }
> >>>
> >>> -static u8 update_white_list(struct hci_request *req)
> >>> +static u8 update_white_list(struct hci_request *req, u8 flags)
> >>> {
> >>>      struct hci_dev *hdev = req->hdev;
> >>>      struct hci_conn_params *params;
> >>>      struct bdaddr_list *b;
> >>> -     uint8_t white_list_entries = 0;
> >>> +     u8 white_list_entries = 0;
> >>> +     bool allow_rpa = !!(flags & LE_SCAN_FLAG_ALLOW_RPA);
> >>> +     bool suspend = !!(flags & LE_SCAN_FLAG_SUSPEND);
> >>> +     bool wakeable, pend_conn, pend_report;
> >>>
> >>>      /* Go through the current white list programmed into the
> >>>       * controller one by one and check if that address is still
> >>> @@ -695,26 +751,42 @@ static u8 update_white_list(struct hci_request *req)
> >>>       * command to remove it from the controller.
> >>>       */
> >>>      list_for_each_entry(b, &hdev->le_white_list, list) {
> >>> -             /* If the device is neither in pend_le_conns nor
> >>> -              * pend_le_reports then remove it from the whitelist.
> >>> +             wakeable = !!hci_bdaddr_list_lookup(&hdev->wakeable, &b->bdaddr,
> >>> +                                                 b->bdaddr_type);
> >>> +             pend_conn = hci_pend_le_action_lookup(&hdev->pend_le_conns,
> >>> +                                                   &b->bdaddr,
> >>> +                                                   b->bdaddr_type);
> >>> +             pend_report = hci_pend_le_action_lookup(&hdev->pend_le_reports,
> >>> +                                                     &b->bdaddr,
> >>> +                                                     b->bdaddr_type);
> >>> +
> >>> +             /* During suspend, we remove all non-wakeable devices
> >>> +              * and leave all others alone. Connected devices will be
> >>> +              * disconnected during suspend but may not be in the pending
> >>> +              * list yet.
> >>>               */
> >>> -             if (!hci_pend_le_action_lookup(&hdev->pend_le_conns,
> >>> -                                            &b->bdaddr, b->bdaddr_type) &&
> >>> -                 !hci_pend_le_action_lookup(&hdev->pend_le_reports,
> >>> -                                            &b->bdaddr, b->bdaddr_type)) {
> >>> -                     struct hci_cp_le_del_from_white_list cp;
> >>> -
> >>> -                     cp.bdaddr_type = b->bdaddr_type;
> >>> -                     bacpy(&cp.bdaddr, &b->bdaddr);
> >>> -
> >>> -                     hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST,
> >>> -                                 sizeof(cp), &cp);
> >>> -                     continue;
> >>> -             }
> >>> +             if (suspend) {
> >>> +                     if (!wakeable) {
> >>> +                             del_from_white_list(req, &b->bdaddr,
> >>> +                                                 b->bdaddr_type);
> >>> +                             continue;
> >>> +                     }
> >>> +             } else {
> >>> +                     /* If the device is not likely to connect or report,
> >>> +                      * remove it from the whitelist.
> >>> +                      */
> >>> +                     if (!pend_conn && !pend_report) {
> >>> +                             del_from_white_list(req, &b->bdaddr,
> >>> +                                                 b->bdaddr_type);
> >>> +                             continue;
> >>> +                     }
> >>>
> >>> -             if (hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) {
> >>>                      /* White list can not be used with RPAs */
> >>> -                     return 0x00;
> >>> +                     if (!allow_rpa &&
> >>> +                         hci_find_irk_by_addr(hdev, &b->bdaddr,
> >>> +                                              b->bdaddr_type)) {
> >>> +                             return 0x00;
> >>> +                     }
> >>>              }
> >>>
> >>>              white_list_entries++;
> >>> @@ -731,47 +803,30 @@ static u8 update_white_list(struct hci_request *req)
> >>>       * white list.
> >>>       */
> >>>      list_for_each_entry(params, &hdev->pend_le_conns, action) {
> >>> -             if (hci_bdaddr_list_lookup(&hdev->le_white_list,
> >>> -                                        &params->addr, params->addr_type))
> >>> -                     continue;
> >>> -
> >>> -             if (white_list_entries >= hdev->le_white_list_size) {
> >>> -                     /* Select filter policy to accept all advertising */
> >>> +             if (add_to_white_list(req, params, &white_list_entries, flags))
> >>>                      return 0x00;
> >>> -             }
> >>> -
> >>> -             if (hci_find_irk_by_addr(hdev, &params->addr,
> >>> -                                      params->addr_type)) {
> >>> -                     /* White list can not be used with RPAs */
> >>> -                     return 0x00;
> >>> -             }
> >>> -
> >>> -             white_list_entries++;
> >>> -             add_to_white_list(req, params);
> >>>      }
> >>>
> >>>      /* After adding all new pending connections, walk through
> >>>       * the list of pending reports and also add these to the
> >>> -      * white list if there is still space.
> >>> +      * white list if there is still space. Abort if space runs out.
> >>>       */
> >>>      list_for_each_entry(params, &hdev->pend_le_reports, action) {
> >>> -             if (hci_bdaddr_list_lookup(&hdev->le_white_list,
> >>> -                                        &params->addr, params->addr_type))
> >>> -                     continue;
> >>> -
> >>> -             if (white_list_entries >= hdev->le_white_list_size) {
> >>> -                     /* Select filter policy to accept all advertising */
> >>> +             if (add_to_white_list(req, params, &white_list_entries, flags))
> >>>                      return 0x00;
> >>> -             }
> >>> +     }
> >>>
> >>> -             if (hci_find_irk_by_addr(hdev, &params->addr,
> >>> -                                      params->addr_type)) {
> >>> -                     /* White list can not be used with RPAs */
> >>> -                     return 0x00;
> >>> +     /* Currently connected devices will be missing from the white list and
> >>> +      * we need to insert them into the whitelist if they are wakeable. We
> >>> +      * can't insert later because we will have already returned from the
> >>> +      * suspend notifier and would cause a spurious wakeup.
> >>> +      */
> >>> +     if (suspend) {
> >>> +             list_for_each_entry(params, &hdev->le_conn_params, list) {
> >>> +                     if (add_to_white_list(req, params, &white_list_entries,
> >>> +                                           flags))
> >>> +                             return 0x00;
> >>>              }
> >>> -
> >>> -             white_list_entries++;
> >>> -             add_to_white_list(req, params);
> >>>      }
> >>
> >> This goes to my point above. If we just disconnect everything, then the white list programming should be easily. Everything that is pending and have been elected for wakeup + incoming connection will be added. And then we are done. I don’t think much has to change except one extra check if wakeup is allowed or not.
> >>
> >>>
> >>>      /* Select filter policy to use white list */
> >>> @@ -861,11 +916,26 @@ static void hci_req_start_scan(struct hci_request *req, u8 type, u16 interval,
> >>>      }
> >>> }
> >>>
> >>> -void hci_req_add_le_passive_scan(struct hci_request *req)
> >>> +void __hci_req_add_le_passive_scan(struct hci_request *req, u8 flags)
> >>> {
> >>>      struct hci_dev *hdev = req->hdev;
> >>>      u8 own_addr_type;
> >>>      u8 filter_policy;
> >>> +     u8 window, interval;
> >>> +
> >>> +     /* We allow whitelisting even with RPAs in suspend. In the worst case,
> >>> +      * we won't be able to wake from devices that use the privacy1.2
> >>> +      * features. Additionally, once we support privacy1.2 and IRK
> >>> +      * offloading, we can update this to also check for those conditions.
> >>> +      */
> >>> +     if (flags & LE_SCAN_FLAG_SUSPEND)
> >>> +             flags |= LE_SCAN_FLAG_ALLOW_RPA;
> >>> +
> >>> +     /* Early exit if we've frozen filters for suspend */
> >>> +     if (hdev->freeze_filters) {
> >>> +             BT_DBG("Filters are frozen for suspend");
> >>> +             return;
> >>> +     }
> >>>
> >>>      /* Set require_privacy to false since no SCAN_REQ are send
> >>>       * during passive scanning. Not using an non-resolvable address
> >>> @@ -881,7 +951,8 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
> >>>       * happen before enabling scanning. The controller does
> >>>       * not allow white list modification while scanning.
> >>>       */
> >>> -     filter_policy = update_white_list(req);
> >>> +     BT_DBG("Updating white list with flags = %d", flags);
> >>> +     filter_policy = update_white_list(req, flags);
> >>>
> >>>      /* When the controller is using random resolvable addresses and
> >>>       * with that having LE privacy enabled, then controllers with
> >>> @@ -896,8 +967,22 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
> >>>          (hdev->le_features[0] & HCI_LE_EXT_SCAN_POLICY))
> >>>              filter_policy |= 0x02;
> >>>
> >>> -     hci_req_start_scan(req, LE_SCAN_PASSIVE, hdev->le_scan_interval,
> >>> -                        hdev->le_scan_window, own_addr_type, filter_policy);
> >>> +     if (flags & LE_SCAN_FLAG_SUSPEND) {
> >>> +             window = LE_SUSPEND_SCAN_WINDOW;
> >>> +             interval = LE_SUSPEND_SCAN_INTERVAL;
> >>
> >> I think we just want this done via some hdev->suspended variable or similar.
> >
> > In support of setting a separate flag, it is easier to grep for when a
> > specific flag is being used and passed to a function than it is to
> > check when a variable is being set in a global context variable.
> >
> >>
> >>> +     } else {
> >>> +             window = hdev->le_scan_window;
> >>> +             interval = hdev->le_scan_interval;
> >>> +     }
> >>> +
> >>> +     BT_DBG("LE passive scan with whitelist = %d", filter_policy);
> >>> +     hci_req_start_scan(req, LE_SCAN_PASSIVE, interval, window,
> >>> +                        own_addr_type, filter_policy);
> >>> +}
> >>> +
> >>> +void hci_req_add_le_passive_scan(struct hci_request *req)
> >>> +{
> >>> +     __hci_req_add_le_passive_scan(req, 0);
> >>> }
> >>>
> >>> static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
> >>> @@ -918,6 +1003,76 @@ static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
> >>>      return adv_instance->scan_rsp_len;
> >>> }
> >>>
> >>> +static void hci_req_clear_event_filter(struct hci_request *req)
> >>> +{
> >>> +     struct hci_cp_set_event_filter f;
> >>> +
> >>> +     memset(&f, 0, sizeof(f));
> >>> +     f.flt_type = HCI_FLT_CLEAR_ALL;
> >>> +     hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &f);
> >>> +
> >>> +     /* Update page scan state (since we may have modified it when setting
> >>> +      * the event filter).
> >>> +      */
> >>> +     __hci_req_update_scan(req);
> >>> +}
> >>> +
> >>> +static void hci_req_set_event_filter(struct hci_request *req)
> >>> +{
> >>> +     struct bdaddr_list *b;
> >>> +     struct hci_cp_set_event_filter f;
> >>> +     struct hci_dev *hdev = req->hdev;
> >>> +     int filters_updated = 0;
> >>> +     u8 scan;
> >>> +
> >>> +     /* Always clear event filter when starting */
> >>> +     hci_req_clear_event_filter(req);
> >>> +
> >>> +     list_for_each_entry(b, &hdev->wakeable, list) {
> >>> +             if (b->bdaddr_type != BDADDR_BREDR)
> >>> +                     continue;
> >>> +
> >>> +             memset(&f, 0, sizeof(f));
> >>> +             bacpy(&f.addr_conn_flt.bdaddr, &b->bdaddr);
> >>> +             f.flt_type = HCI_FLT_CONN_SETUP;
> >>> +             f.cond_type = HCI_CONN_SETUP_ALLOW_BDADDR;
> >>> +             f.addr_conn_flt.auto_accept = HCI_CONN_SETUP_AUTO_ON;
> >>> +
> >>> +             BT_DBG("Adding event filters for %pMR", &b->bdaddr);
> >>> +             hci_req_add(req, HCI_OP_SET_EVENT_FLT, sizeof(f), &f);
> >>> +
> >>> +             filters_updated++;
> >>> +     }
> >>
> >> If we would use the wakeable list just for BR/EDR then the filters_updated++ would become redundant and we could just check if the list is empty.
> >>
> >>> +
> >>> +     scan = filters_updated ? SCAN_PAGE : SCAN_DISABLED;
> >>> +     hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
> >>> +}
> >>> +
> >>> +static void hci_req_enable_le_suspend_scan(struct hci_request *req,
> >>> +                                        u8 flags)
> >>> +{
> >>> +     /* Can't change params without disabling first */
> >>> +     hci_req_add_le_scan_disable(req);
> >>> +
> >>> +     /* Configure params and enable scanning */
> >>> +     __hci_req_add_le_passive_scan(req, flags);
> >>> +
> >>> +     /* Block suspend notifier on response */
> >>> +     set_bit(SUSPEND_LE_SET_SCAN_ENABLE, req->hdev->suspend_tasks);
> >>> +}
> >>> +
> >>> +static void le_suspend_req_complete(struct hci_dev *hdev, u8 status, u16 opcode)
> >>> +{
> >>> +     BT_DBG("Request complete opcode=0x%x, status=0x%x", opcode, status);
> >>> +
> >>> +     /* Expecting LE Set scan to return */
> >>> +     if (opcode == HCI_OP_LE_SET_SCAN_ENABLE &&
> >>> +         test_and_clear_bit(SUSPEND_LE_SET_SCAN_ENABLE,
> >>> +                            hdev->suspend_tasks)) {
> >>> +             wake_up(&hdev->suspend_wait_q);
> >>> +     }
> >>> +}
> >>> +
> >>> /* Call with hci_dev_lock */
> >>> void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> >>> {
> >>> @@ -932,6 +1087,44 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> >>>
> >>>      hdev->suspend_state = next;
> >>>
> >>> +     hci_req_init(&req, hdev);
> >>> +     if (next == BT_SUSPENDED) {
> >>> +             /* Enable event filter for existing devices */
> >>> +             hci_req_set_event_filter(&req);
> >>> +
> >>> +             /* Enable passive scan at lower duty cycle */
> >>> +             hci_req_enable_le_suspend_scan(&req, LE_SCAN_FLAG_SUSPEND);
> >>> +
> >>> +             hdev->freeze_filters = true;
> >>> +
> >>> +             /* Run commands before disconnecting */
> >>> +             hci_req_run(&req, le_suspend_req_complete);
> >>> +
> >>> +             hdev->disconnect_counter = 0;
> >>> +             /* Soft disconnect everything (power off)*/
> >>> +             list_for_each_entry(conn, &hdev->conn_hash.list, list) {
> >>> +                     hci_disconnect(conn, HCI_ERROR_REMOTE_POWER_OFF);
> >>> +                     hdev->disconnect_counter++;
> >>> +             }
> >>> +
> >>> +             if (hdev->disconnect_counter > 0) {
> >>> +                     BT_DBG("Had %d disconnects. Will wait on them",
> >>> +                            hdev->disconnect_counter);
> >>> +                     set_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks);
> >>> +             }
> >>> +     } else {
> >>> +             hdev->freeze_filters = false;
> >>> +
> >>> +             hci_req_clear_event_filter(&req);
> >>> +
> >>> +             /* Reset passive/background scanning to normal */
> >>> +             hci_req_enable_le_suspend_scan(&req, 0);
> >>> +
> >>> +             hci_req_run(&req, le_suspend_req_complete);
> >>> +     }
> >>> +
> >>> +     hdev->suspend_state = next;
> >>> +
> >>> done:
> >>>      clear_bit(SUSPEND_PREPARE_NOTIFIER, hdev->suspend_tasks);
> >>>      wake_up(&hdev->suspend_wait_q);
> >>> @@ -2034,6 +2227,9 @@ void __hci_req_update_scan(struct hci_request *req)
> >>>      if (mgmt_powering_down(hdev))
> >>>              return;
> >>>
> >>> +     if (hdev->freeze_filters)
> >>> +             return;
> >>> +
> >>>      if (hci_dev_test_flag(hdev, HCI_CONNECTABLE) ||
> >>>          disconnected_whitelist_entries(hdev))
> >>>              scan = SCAN_PAGE;
> >>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> >>> index 58468dfa112f..269ce70e501c 100644
> >>> --- a/net/bluetooth/mgmt.c
> >>> +++ b/net/bluetooth/mgmt.c
> >>> @@ -7451,6 +7451,14 @@ void mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
> >>>
> >>>      mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, &ev, sizeof(ev), sk);
> >>>
> >>> +     if (hdev->disconnect_counter > 0) {
> >>> +             hdev->disconnect_counter--;
> >>> +             if (hdev->disconnect_counter <= 0) {
> >>> +                     clear_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks);
> >>> +                     wake_up(&hdev->suspend_wait_q);
> >>> +             }
> >>> +     }
> >>> +
> >>
> >> I think this disconnect_counter is a bit racy. I would rather check that our connection hash is empty.
> >>
> >> In addition, I have the feeling we better disable LE scanning BR/EDR page scan first. So that we clearly do not have any artifacts of accidental incoming connections.
> >>
> >> The first step when going to suspend should clean the house
> >>
> >>        1) Set a flag that all BR/EDR and LE connection attempts are rejected
> >>        2) Disable BR/EDR page scan
> >>        3) Disable LE advertising
> >>        2) Disable LE scanning / BR/EDR inquiry
> >>        4) Disconnect all
> >>
> >> Now we have a clean house and we now prepare for wakeup triggers
> >>
> >>        1) Clear flag and allow incoming connection
> >>        2) Set BR/EDR event filter and enable page scan if needed
> >>        3) Set LE whitelist and enable scanning if needed
> >>
> >> When we wake up, then we also just need to reset the filters
> >>
> >>        1) Clear BR/EDR event filter and enable page scan if needed
> >>        2) Update LE white and enable scanning if needed
> >>        3) Restore LE scanning / BR/EDR inquiry
> >>
> >
> > I think this is reasonable so I will try to implement it. Might take
> > me a couple of days (traveling out of country) so expect the next
> > patch series early next week.
>
> I just wanted to quickly check if you run into any troubles with the changes?
>
> Regards
>
> Marcel
>

[RFC PATCH v2 4/4] Bluetooth: Pause discovery and advertising during suspend

[Abhishek Pandit-Subedi]

To prevent spurious wake ups, we disable any discovery or advertising
when we enter suspend and restore it when we exit suspend. While paused,
we disable any management requests to modify discovery or advertising.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2:
* Refactored pause discovery + advertising into its own patch

 include/net/bluetooth/hci_core.h | 11 ++++++++
 net/bluetooth/hci_request.c      | 43 ++++++++++++++++++++++++++++++++
 net/bluetooth/mgmt.c             | 41 ++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 49eae4a802ac..cbfaa80067d6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -91,6 +91,12 @@ struct discovery_state {
 #define SUSPEND_NOTIFIER_TIMEOUT	msecs_to_jiffies(2000) /* 2 seconds */
 
 enum suspend_tasks {
+	SUSPEND_PAUSE_DISCOVERY,
+	SUSPEND_UNPAUSE_DISCOVERY,
+
+	SUSPEND_PAUSE_ADVERTISING,
+	SUSPEND_UNPAUSE_ADVERTISING,
+
 	SUSPEND_LE_SET_SCAN_ENABLE,
 	SUSPEND_DISCONNECTING,
 
@@ -405,6 +411,11 @@ struct hci_dev {
 
 	struct discovery_state	discovery;
 
+	int			discovery_old_state;
+	bool			discovery_paused;
+	int			advertising_old_state;
+	bool			advertising_paused;
+
 	struct notifier_block	suspend_notifier;
 	struct work_struct	suspend_prepare;
 	enum suspended_state	suspend_state_next;
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index c930b9ff1cfd..487c84841351 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1089,6 +1089,29 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
 
 	hci_req_init(&req, hdev);
 	if (next == BT_SUSPENDED) {
+		/* Pause discovery if not already stopped */
+		old_state = hdev->discovery.state;
+		if (old_state != DISCOVERY_STOPPED) {
+			set_bit(SUSPEND_PAUSE_DISCOVERY, hdev->suspend_tasks);
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
+			queue_work(hdev->req_workqueue, &hdev->discov_update);
+		}
+
+		hdev->discovery_paused = true;
+		hdev->discovery_old_state = old_state;
+
+		/* Stop advertising */
+		old_state = hci_dev_test_flag(hdev, HCI_ADVERTISING);
+		if (old_state) {
+			set_bit(SUSPEND_PAUSE_ADVERTISING, hdev->suspend_tasks);
+			cancel_delayed_work(&hdev->discov_off);
+			queue_delayed_work(hdev->req_workqueue,
+					   &hdev->discov_off, 0);
+		}
+
+		hdev->advertising_paused = true;
+		hdev->advertising_old_state = old_state;
+
 		/* Enable event filter for existing devices */
 		hci_req_set_event_filter(&req);
 
@@ -1120,6 +1143,26 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
 		/* Reset passive/background scanning to normal */
 		hci_req_enable_le_suspend_scan(&req, 0);
 
+		/* Unpause advertising */
+		hdev->advertising_paused = false;
+		if (hdev->advertising_old_state) {
+			set_bit(SUSPEND_UNPAUSE_ADVERTISING,
+				hdev->suspend_tasks);
+			hci_dev_set_flag(hdev, HCI_ADVERTISING);
+			queue_work(hdev->req_workqueue,
+				   &hdev->discoverable_update);
+			hdev->advertising_old_state = 0;
+		}
+
+		/* Unpause discovery */
+		hdev->discovery_paused = false;
+		if (hdev->discovery_old_state != DISCOVERY_STOPPED &&
+		    hdev->discovery_old_state != DISCOVERY_STOPPING) {
+			set_bit(SUSPEND_UNPAUSE_DISCOVERY, hdev->suspend_tasks);
+			hci_discovery_set_state(hdev, DISCOVERY_STARTING);
+			queue_work(hdev->req_workqueue, &hdev->discov_update);
+		}
+
 		hci_req_run(&req, le_suspend_req_complete);
 	}
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 269ce70e501c..b3bd7a5ae718 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1383,6 +1383,12 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto failed;
 	}
 
+	if (hdev->advertising_paused) {
+		err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
+				      MGMT_STATUS_BUSY);
+		goto failed;
+	}
+
 	if (!hdev_is_powered(hdev)) {
 		bool changed = false;
 
@@ -3866,6 +3872,13 @@ void mgmt_start_discovery_complete(struct hci_dev *hdev, u8 status)
 	}
 
 	hci_dev_unlock(hdev);
+
+	/* Handle suspend notifier */
+	if (test_and_clear_bit(SUSPEND_UNPAUSE_DISCOVERY,
+			       hdev->suspend_tasks)) {
+		BT_DBG("Unpaused discovery");
+		wake_up(&hdev->suspend_wait_q);
+	}
 }
 
 static bool discovery_type_is_valid(struct hci_dev *hdev, uint8_t type,
@@ -3927,6 +3940,13 @@ static int start_discovery_internal(struct sock *sk, struct hci_dev *hdev,
 		goto failed;
 	}
 
+	/* Can't start discovery when it is paused */
+	if (hdev->discovery_paused) {
+		err = mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_BUSY,
+					&cp->type, sizeof(cp->type));
+		goto failed;
+	}
+
 	/* Clear the discovery filter first to free any previously
 	 * allocated memory for the UUID list.
 	 */
@@ -4094,6 +4114,12 @@ void mgmt_stop_discovery_complete(struct hci_dev *hdev, u8 status)
 	}
 
 	hci_dev_unlock(hdev);
+
+	/* Handle suspend notifier */
+	if (test_and_clear_bit(SUSPEND_PAUSE_DISCOVERY, hdev->suspend_tasks)) {
+		BT_DBG("Paused discovery");
+		wake_up(&hdev->suspend_wait_q);
+	}
 }
 
 static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
@@ -4325,6 +4351,17 @@ static void set_advertising_complete(struct hci_dev *hdev, u8 status,
 	if (match.sk)
 		sock_put(match.sk);
 
+	/* Handle suspend notifier */
+	if (test_and_clear_bit(SUSPEND_PAUSE_ADVERTISING,
+			       hdev->suspend_tasks)) {
+		BT_DBG("Paused advertising");
+		wake_up(&hdev->suspend_wait_q);
+	} else if (test_and_clear_bit(SUSPEND_UNPAUSE_ADVERTISING,
+				      hdev->suspend_tasks)) {
+		BT_DBG("Unpaused advertising");
+		wake_up(&hdev->suspend_wait_q);
+	}
+
 	/* If "Set Advertising" was just disabled and instance advertising was
 	 * set up earlier, then re-enable multi-instance advertising.
 	 */
@@ -4376,6 +4413,10 @@ static int set_advertising(struct sock *sk, struct hci_dev *hdev, void *data,
 		return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
 				       MGMT_STATUS_INVALID_PARAMS);
 
+	if (hdev->advertising_paused)
+		return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
+				       MGMT_STATUS_BUSY);
+
 	hci_dev_lock(hdev);
 
 	val = !!cp->val;
-- 
2.25.0.341.g760bfbb309-goog