[PATCH v2 0/5] LE scanning fixes & cleanups

[PATCH v2 0/5] LE scanning fixes & cleanups

[johan.hedberg]

Hi,

This v2 only modifies 3/5 by simplifying hci_pend_le_conn_add a bit further.

Johan

----------------------------------------------------------------
Johan Hedberg (5):
      Bluetooth: Fix missing update of pend_le_reports
      Bluetooth: Remove redundant IRK lookup
      Bluetooth: Use hci_conn_params in pend_le_conns
      Bluetooth: Remove unnecessary checks for auto-connected devices
      Bluetooth: Add identity address check in param lookup functions

 include/net/bluetooth/hci_core.h |  9 +++--
 net/bluetooth/hci_core.c         | 76 +++++++++++++++-----------------------
 net/bluetooth/hci_event.c        | 45 ++++++++++------------
 net/bluetooth/mgmt.c             |  7 +++-

[PATCH v2 1/5] Bluetooth: Fix missing update of pend_le_reports

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

When calling Remove Device for an entry using HCI_AUTO_CONN_REPORT we
need to decrement the pend_le_reports value correspondingly. This patch
fixes one such missing action in the Remove Device command handler.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c01cc5e37a6f..02a4d31fee30 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -5198,6 +5198,9 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
 			goto unlock;
 		}
 
+		if (params->auto_connect == HCI_AUTO_CONN_REPORT)
+			hdev->pend_le_reports--;
+
 		hci_pend_le_conn_del(hdev, &cp->addr.bdaddr, addr_type);
 		list_del(&params->list);
 		kfree(params);
-- 
1.9.3

[PATCH v2 2/5] Bluetooth: Remove redundant IRK lookup

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

When processing passive scanning results we need the resolved identity
address both in check_pending_le_conn() as well as later in
process_adv_report(). Since process_adv_report() calls
check_pending_le_conn() we can simply resolve the IRK earlier in the
function and thereby eliminate a second lookup.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_event.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index dd70e3a4fea5..643c5a8d4050 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4186,16 +4186,6 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
 				  u8 addr_type)
 {
 	struct hci_conn *conn;
-	struct smp_irk *irk;
-
-	/* If this is a resolvable address, we should resolve it and then
-	 * update address and address type variables.
-	 */
-	irk = hci_get_irk(hdev, addr, addr_type);
-	if (irk) {
-		addr = &irk->bdaddr;
-		addr_type = irk->addr_type;
-	}
 
 	if (!hci_pend_le_conn_lookup(hdev, addr, addr_type))
 		return;
@@ -4233,6 +4223,13 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 		struct hci_conn_params *param;
 		struct smp_irk *irk;
 
+		/* Check if we need to convert to identity address */
+		irk = hci_get_irk(hdev, bdaddr, bdaddr_type);
+		if (irk) {
+			bdaddr = &irk->bdaddr;
+			bdaddr_type = irk->addr_type;
+		}
+
 		if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
 			check_pending_le_conn(hdev, bdaddr, bdaddr_type);
 
@@ -4242,13 +4239,6 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 		if (type == LE_ADV_DIRECT_IND)
 			return;
 
-		/* Check if we need to convert to identity address */
-		irk = hci_get_irk(hdev, bdaddr, bdaddr_type);
-		if (irk) {
-			bdaddr = &irk->bdaddr;
-			bdaddr_type = irk->addr_type;
-		}
-
 		/* The conn params list only contains identity addresses */
 		if (!hci_is_identity_address(bdaddr, bdaddr_type))
 			return;
-- 
1.9.3

[PATCH v2 3/5] Bluetooth: Use hci_conn_params in pend_le_conns

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Since the connection parameters are always a basis for adding entries to
hdev->pend_le_conns (so far of type bdaddr_list) it's simpler and more
efficient to have the parameters themselves be the entries in the
pend_le_conns list. We do this by adding another list_head to the
hci_conn_params struct.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |  9 +++---
 net/bluetooth/hci_core.c         | 68 +++++++++++++---------------------------
 net/bluetooth/hci_event.c        |  7 +++--
 net/bluetooth/mgmt.c             |  4 +--
 4 files changed, 33 insertions(+), 55 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b5f4405b41c2..09f9fb85d8fd 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -439,6 +439,7 @@ struct hci_chan {
 
 struct hci_conn_params {
 	struct list_head list;
+	struct list_head pend_le_conn;
 
 	bdaddr_t addr;
 	u8 addr_type;
@@ -867,10 +868,10 @@ void hci_conn_params_clear_all(struct hci_dev *hdev);
 void hci_conn_params_clear_disabled(struct hci_dev *hdev);
 void hci_conn_params_clear_enabled(struct hci_dev *hdev);
 
-struct bdaddr_list *hci_pend_le_conn_lookup(struct hci_dev *hdev,
-					    bdaddr_t *addr, u8 addr_type);
-void hci_pend_le_conn_add(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
-void hci_pend_le_conn_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
+struct hci_conn_params *hci_pend_le_conn_lookup(struct hci_dev *hdev,
+						bdaddr_t *addr, u8 addr_type);
+void hci_pend_le_conn_add(struct hci_dev *hdev, struct hci_conn_params *params);
+void hci_pend_le_conn_del(struct hci_dev *hdev, struct hci_conn_params *params);
 void hci_pend_le_conns_clear(struct hci_dev *hdev);
 
 void hci_update_background_scan(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 41cc64429ea1..8882a6cd2876 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3427,73 +3427,46 @@ static bool is_connected(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
 }
 
 /* This function requires the caller holds hdev->lock */
-struct bdaddr_list *hci_pend_le_conn_lookup(struct hci_dev *hdev,
-					    bdaddr_t *addr, u8 addr_type)
+struct hci_conn_params *hci_pend_le_conn_lookup(struct hci_dev *hdev,
+						bdaddr_t *addr, u8 addr_type)
 {
-	struct bdaddr_list *entry;
+	struct hci_conn_params *param;
 
-	list_for_each_entry(entry, &hdev->pend_le_conns, list) {
-		if (bacmp(&entry->bdaddr, addr) == 0 &&
-		    entry->bdaddr_type == addr_type)
-			return entry;
+	list_for_each_entry(param, &hdev->pend_le_conns, pend_le_conn) {
+		if (bacmp(&param->addr, addr) == 0 &&
+		    param->addr_type == addr_type)
+			return param;
 	}
 
 	return NULL;
 }
 
 /* This function requires the caller holds hdev->lock */
-void hci_pend_le_conn_add(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
+void hci_pend_le_conn_add(struct hci_dev *hdev, struct hci_conn_params *params)
 {
-	struct bdaddr_list *entry;
-
-	entry = hci_pend_le_conn_lookup(hdev, addr, addr_type);
-	if (entry)
-		goto done;
-
-	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
-	if (!entry) {
-		BT_ERR("Out of memory");
-		return;
-	}
+	list_del_init(&params->pend_le_conn);
+	list_add(&params->pend_le_conn, &hdev->pend_le_conns);
 
-	bacpy(&entry->bdaddr, addr);
-	entry->bdaddr_type = addr_type;
-
-	list_add(&entry->list, &hdev->pend_le_conns);
-
-	BT_DBG("addr %pMR (type %u)", addr, addr_type);
+	BT_DBG("addr %pMR (type %u)", &params->addr, params->addr_type);
 
-done:
 	hci_update_background_scan(hdev);
 }
 
 /* This function requires the caller holds hdev->lock */
-void hci_pend_le_conn_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
+void hci_pend_le_conn_del(struct hci_dev *hdev, struct hci_conn_params *params)
 {
-	struct bdaddr_list *entry;
-
-	entry = hci_pend_le_conn_lookup(hdev, addr, addr_type);
-	if (!entry)
-		goto done;
-
-	list_del(&entry->list);
-	kfree(entry);
+	list_del_init(&params->pend_le_conn);
 
-	BT_DBG("addr %pMR (type %u)", addr, addr_type);
+	BT_DBG("addr %pMR (type %u)", &params->addr, params->addr_type);
 
-done:
 	hci_update_background_scan(hdev);
 }
 
 /* This function requires the caller holds hdev->lock */
 void hci_pend_le_conns_clear(struct hci_dev *hdev)
 {
-	struct bdaddr_list *entry, *tmp;
-
-	list_for_each_entry_safe(entry, tmp, &hdev->pend_le_conns, list) {
-		list_del(&entry->list);
-		kfree(entry);
-	}
+	while (!list_empty(&hdev->pend_le_conns))
+		list_del_init(hdev->pend_le_conns.next);
 
 	BT_DBG("All LE pending connections cleared");
 
@@ -3523,6 +3496,7 @@ struct hci_conn_params *hci_conn_params_add(struct hci_dev *hdev,
 	params->addr_type = addr_type;
 
 	list_add(&params->list, &hdev->le_conn_params);
+	INIT_LIST_HEAD(&params->pend_le_conn);
 
 	params->conn_min_interval = hdev->le_conn_min_interval;
 	params->conn_max_interval = hdev->le_conn_max_interval;
@@ -3552,16 +3526,16 @@ int hci_conn_params_set(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
 	switch (auto_connect) {
 	case HCI_AUTO_CONN_DISABLED:
 	case HCI_AUTO_CONN_LINK_LOSS:
-		hci_pend_le_conn_del(hdev, addr, addr_type);
+		hci_pend_le_conn_del(hdev, params);
 		break;
 	case HCI_AUTO_CONN_REPORT:
 		if (params->auto_connect != HCI_AUTO_CONN_REPORT)
 			hdev->pend_le_reports++;
-		hci_pend_le_conn_del(hdev, addr, addr_type);
+		hci_pend_le_conn_del(hdev, params);
 		break;
 	case HCI_AUTO_CONN_ALWAYS:
 		if (!is_connected(hdev, addr, addr_type))
-			hci_pend_le_conn_add(hdev, addr, addr_type);
+			hci_pend_le_conn_add(hdev, params);
 		break;
 	}
 
@@ -3585,7 +3559,7 @@ void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
 	if (params->auto_connect == HCI_AUTO_CONN_REPORT)
 		hdev->pend_le_reports--;
 
-	hci_pend_le_conn_del(hdev, addr, addr_type);
+	hci_pend_le_conn_del(hdev, params);
 
 	list_del(&params->list);
 	kfree(params);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 643c5a8d4050..4ebfcedd9ae7 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2209,7 +2209,7 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 			/* Fall through */
 
 		case HCI_AUTO_CONN_ALWAYS:
-			hci_pend_le_conn_add(hdev, &conn->dst, conn->dst_type);
+			hci_pend_le_conn_add(hdev, params);
 			break;
 
 		default:
@@ -4036,6 +4036,7 @@ static void hci_disconn_phylink_complete_evt(struct hci_dev *hdev,
 static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_le_conn_complete *ev = (void *) skb->data;
+	struct hci_conn_params *params;
 	struct hci_conn *conn;
 	struct smp_irk *irk;
 	u8 addr_type;
@@ -4152,7 +4153,9 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	hci_proto_connect_cfm(conn, ev->status);
 
-	hci_pend_le_conn_del(hdev, &conn->dst, conn->dst_type);
+	params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type);
+	if (params)
+		hci_pend_le_conn_del(hdev, params);
 
 unlock:
 	hci_dev_unlock(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 02a4d31fee30..59ca4057955c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -5201,7 +5201,7 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
 		if (params->auto_connect == HCI_AUTO_CONN_REPORT)
 			hdev->pend_le_reports--;
 
-		hci_pend_le_conn_del(hdev, &cp->addr.bdaddr, addr_type);
+		hci_pend_le_conn_del(hdev, params);
 		list_del(&params->list);
 		kfree(params);
 
@@ -5514,7 +5514,7 @@ static void restart_le_auto_conns(struct hci_dev *hdev)
 
 	list_for_each_entry(p, &hdev->le_conn_params, list) {
 		if (p->auto_connect == HCI_AUTO_CONN_ALWAYS) {
-			hci_pend_le_conn_add(hdev, &p->addr, p->addr_type);
+			hci_pend_le_conn_add(hdev, p);
 			added = true;
 		}
 	}
-- 
1.9.3

[PATCH v2 4/5] Bluetooth: Remove unnecessary checks for auto-connected devices

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

If a device is in the pend_le_conns list it cannot at the same time also
have the need to be notified through mgmt_device_found. By making
check_pending_le_conn return whether it found an entry or not we can
avoid unnecessary checks in process_adv_report().

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_event.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 4ebfcedd9ae7..e0b439d4f958 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4185,18 +4185,18 @@ static void hci_le_conn_update_complete_evt(struct hci_dev *hdev,
 }
 
 /* This function requires the caller holds hdev->lock */
-static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
+static bool check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
 				  u8 addr_type)
 {
 	struct hci_conn *conn;
 
 	if (!hci_pend_le_conn_lookup(hdev, addr, addr_type))
-		return;
+		return false;
 
 	conn = hci_connect_le(hdev, addr, addr_type, BT_SECURITY_LOW,
 			      HCI_AT_NO_BONDING);
 	if (!IS_ERR(conn))
-		return;
+		return true;
 
 	switch (PTR_ERR(conn)) {
 	case -EBUSY:
@@ -4209,6 +4209,8 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
 	default:
 		BT_DBG("Failed to connect: err %ld", PTR_ERR(conn));
 	}
+
+	return true;
 }
 
 static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
@@ -4233,8 +4235,10 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 			bdaddr_type = irk->addr_type;
 		}
 
-		if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
-			check_pending_le_conn(hdev, bdaddr, bdaddr_type);
+		if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND) {
+			if (check_pending_le_conn(hdev, bdaddr, bdaddr_type))
+				return;
+		}
 
 		if (!hdev->pend_le_reports)
 			return;
-- 
1.9.3

[PATCH v2 5/5] Bluetooth: Add identity address check in param lookup functions

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Since we only store entries with identity addresses in the
le_conn_params and pend_le_conns lists we can avoid unnecessary lookups
by checking for an identity address before diving into the lists
themselves.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c  | 8 ++++++++
 net/bluetooth/hci_event.c | 4 ----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 8882a6cd2876..750f969df8db 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3399,6 +3399,10 @@ struct hci_conn_params *hci_conn_params_lookup(struct hci_dev *hdev,
 {
 	struct hci_conn_params *params;
 
+	/* The conn params list only contains identity addresses */
+	if (!hci_is_identity_address(addr, addr_type))
+		return NULL;
+
 	list_for_each_entry(params, &hdev->le_conn_params, list) {
 		if (bacmp(&params->addr, addr) == 0 &&
 		    params->addr_type == addr_type) {
@@ -3432,6 +3436,10 @@ struct hci_conn_params *hci_pend_le_conn_lookup(struct hci_dev *hdev,
 {
 	struct hci_conn_params *param;
 
+	/* The list only contains identity addresses */
+	if (!hci_is_identity_address(addr, addr_type))
+		return NULL;
+
 	list_for_each_entry(param, &hdev->pend_le_conns, pend_le_conn) {
 		if (bacmp(&param->addr, addr) == 0 &&
 		    param->addr_type == addr_type)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e0b439d4f958..20317e516e74 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4246,10 +4246,6 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 		if (type == LE_ADV_DIRECT_IND)
 			return;
 
-		/* The conn params list only contains identity addresses */
-		if (!hci_is_identity_address(bdaddr, bdaddr_type))
-			return;
-
 		param = hci_conn_params_lookup(hdev, bdaddr, bdaddr_type);
 		if (!param || param->auto_connect != HCI_AUTO_CONN_REPORT)
 			return;
-- 
1.9.3

Re: [PATCH v2 0/5] LE scanning fixes & cleanups

[Marcel Holtmann]

Hi Johan,

> ----------------------------------------------------------------
> Johan Hedberg (5):
>      Bluetooth: Fix missing update of pend_le_reports
>      Bluetooth: Remove redundant IRK lookup
>      Bluetooth: Use hci_conn_params in pend_le_conns
>      Bluetooth: Remove unnecessary checks for auto-connected devices
>      Bluetooth: Add identity address check in param lookup functions
> 
> include/net/bluetooth/hci_core.h |  9 +++--
> net/bluetooth/hci_core.c         | 76 +++++++++++++++-----------------------
> net/bluetooth/hci_event.c        | 45 ++++++++++------------
> net/bluetooth/mgmt.c             |  7 +++-

all 5 patches have been applied to bluetooth-next tree.

Regards

Marcel