[RFC v2 0/8] LE Connection Routine

[RFC v2 0/8] LE Connection Routine

[Andre Guedes]

Hi all,

This v2 RFC series address Marcel's comment regarding a separated state
machine for handling LE connection attempts. So in this series it was
introduced a new state machine for carrying out the general connection
establishment procedure.

>From the previous cover letter:

In order to better support LE connection requirements, such as multiple
connections and re-connections, we should support the general connection
establishment procedure in kernel side. Today, we support only the direct
connection establishment procedure which has some limitations and, therefore,
requires extra connection management at user-space.

According to the spec (Vol 3, Part C, section 9.3.6), the general procedure is
described as follows: The host starts scanning for LE devices, once the device
we want to connect to is in-range, the host stops scanning and initiates a
connection. The procedure is terminated when the connection is established or
when the host terminates the procedure.

This RFC series implements the basic support for general connection procedure.
The first patches do simple changes required to implement this new LE
connection routine. It has not been well tested, but the basic LE connection
and disconnection cases have been covered.

This is an initial work, so it doesn't support multiple LE connection attempts
at the same time (current kernel doesn't support too) and doesn't handle
concurrent device discovery properly.

The next steps are the following:
1. Cover some corner cases in this series.
2. Add support for multiple LE connection attempts.
3. Handle concurrent LE connections and device discovery
4. Add better support for controller which a capable of scanning and initiating
LE connection at the same time.
5. Add API so userspace is able to configure connection parameters.
6. Remove LE connection management code from bluetoothd.

Feedback is welcome.

Regards,

Andre


Andre Guedes (8):
  Bluetooth: Setup LE scan with no timeout
  Bluetooth: Add LE scan type macros
  Bluetooth: LE connection state machine
  Bluetooth: Add hci_conn helpers
  Bluetooth: Change LE connection routine
  Bluetooth: Handle hci_conn timeout in BT_CONNECT
  Bluetooth: Count pending LE connection attempts
  Bluetooth: Initialize some hci_conn fields earlier

 include/net/bluetooth/hci_core.h |  25 +++++++++
 net/bluetooth/hci_conn.c         | 111 ++++++++++++++++++++++++++++++++++++---
 net/bluetooth/hci_core.c         |  19 ++++---
 net/bluetooth/hci_event.c        |  30 ++++++++++-
 net/bluetooth/mgmt.c             |   7 ++-
 5 files changed, 171 insertions(+), 21 deletions(-)

-- 
1.8.1.2

[RFC v2 1/8] Bluetooth: Setup LE scan with no timeout

[Andre Guedes]

This patch modifies hci_do_le_scan and hci_cancel_le_scan helpers so
we are able to start and stop LE scan with no timeout. This feature
will be used by the LE connection routine.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_core.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 22e77a7..3aa0345 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1618,26 +1618,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
 	if (err < 0)
 		return err;
 
-	queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
-			   msecs_to_jiffies(timeout));
+	if (timeout > 0)
+		queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
+				   msecs_to_jiffies(timeout));
 
 	return 0;
 }
 
 int hci_cancel_le_scan(struct hci_dev *hdev)
 {
+	struct hci_cp_le_set_scan_enable cp;
+
 	BT_DBG("%s", hdev->name);
 
 	if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
 		return -EALREADY;
 
-	if (cancel_delayed_work(&hdev->le_scan_disable)) {
-		struct hci_cp_le_set_scan_enable cp;
+	cancel_delayed_work(&hdev->le_scan_disable);
 
-		/* Send HCI command to disable LE Scan */
-		memset(&cp, 0, sizeof(cp));
-		hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
-	}
+	/* Send HCI command to disable LE Scan */
+	memset(&cp, 0, sizeof(cp));
+	hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
 
 	return 0;
 }
-- 
1.8.1.2

[RFC v2 2/8] Bluetooth: Add LE scan type macros

[Andre Guedes]

This patch adds macros for active and passive LE scan type values. It
also removes the LE_SCAN_TYPE macro since it is not used anymore.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h | 3 +++
 net/bluetooth/mgmt.c             | 7 +++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 90cf75a..48c28ca 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -117,6 +117,9 @@ struct oob_data {
 	u8 randomizer[16];
 };
 
+#define LE_SCAN_PASSIVE		0x00
+#define LE_SCAN_ACTIVE		0x01
+
 struct le_scan_params {
 	u8 type;
 	u16 interval;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 39395c7..1e5bd74 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -106,7 +106,6 @@ static const u16 mgmt_events[] = {
  * These LE scan and inquiry parameters were chosen according to LE General
  * Discovery Procedure specification.
  */
-#define LE_SCAN_TYPE			0x01
 #define LE_SCAN_WIN			0x12
 #define LE_SCAN_INT			0x12
 #define LE_SCAN_TIMEOUT_LE_ONLY		10240	/* TGAP(gen_disc_scan_min) */
@@ -2485,7 +2484,7 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 			goto failed;
 		}
 
-		err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
+		err = hci_le_scan(hdev, LE_SCAN_ACTIVE, LE_SCAN_INT,
 				  LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
 		break;
 
@@ -2497,8 +2496,8 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 			goto failed;
 		}
 
-		err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT, LE_SCAN_WIN,
-				  LE_SCAN_TIMEOUT_BREDR_LE);
+		err = hci_le_scan(hdev, LE_SCAN_ACTIVE, LE_SCAN_INT,
+				  LE_SCAN_WIN, LE_SCAN_TIMEOUT_BREDR_LE);
 		break;
 
 	default:
-- 
1.8.1.2

[RFC v2 3/8] Bluetooth: LE connection state machine

[Andre Guedes]

This patch implements a state machine for carrying out the general
connection establishment procedure described in Core spec.

The state machine should be used as follows: when the kernel
receives a new LE connection attempt, it should go to HCI_CONN_LE_
SCAN state, starting the passive LE scanning. Once the target remote
device is in-range, it should go to HCI_CONN_LE_FOUND, stopping the
ongoing LE scanning. After the LE scanning is disabled, it should go
to HCI_CONN_LE_INITIATE state where the LE connection is created.

This state machine will be used by the LE connection routine in
order to establish LE connections.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h | 13 +++++++++++++
 net/bluetooth/hci_conn.c         | 26 ++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 48c28ca..c704737 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -300,6 +300,16 @@ struct hci_dev {
 
 #define HCI_PHY_HANDLE(handle)	(handle & 0xff)
 
+/*
+ * States from LE connection establishment state machine.
+ * State 0 is reserved and indicates the state machine is not running.
+ */
+enum {
+	HCI_CONN_LE_SCAN = 1,
+	HCI_CONN_LE_FOUND,
+	HCI_CONN_LE_INITIATE,
+};
+
 struct hci_conn {
 	struct list_head list;
 
@@ -356,6 +366,8 @@ struct hci_conn {
 
 	struct hci_conn	*link;
 
+	atomic_t	le_state;
+
 	void (*connect_cfm_cb)	(struct hci_conn *conn, u8 status);
 	void (*security_cfm_cb)	(struct hci_conn *conn, u8 status);
 	void (*disconn_cfm_cb)	(struct hci_conn *conn, u8 reason);
@@ -599,6 +611,7 @@ int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
 int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
 int hci_conn_change_link_key(struct hci_conn *conn);
 int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
+void hci_conn_set_le_state(struct hci_conn *conn, int state);
 
 void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
 
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 25bfce0..d54c2a0 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -391,6 +391,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
 		    (unsigned long) conn);
 
 	atomic_set(&conn->refcnt, 0);
+	atomic_set(&conn->le_state, 0);
 
 	hci_dev_hold(hdev);
 
@@ -1027,3 +1028,28 @@ struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle)
 
 	return hchan;
 }
+
+void hci_conn_set_le_state(struct hci_conn *conn, int state)
+{
+	struct hci_dev *hdev = conn->hdev;
+
+	BT_DBG("conn %p state %d -> %d", conn, atomic_read(&conn->le_state),
+	       state);
+
+	if (state == atomic_read(&conn->le_state))
+		return;
+
+	switch (state) {
+	case HCI_CONN_LE_SCAN:
+		hci_le_scan(hdev, LE_SCAN_PASSIVE, 0x60, 0x30, 0);
+		break;
+	case HCI_CONN_LE_FOUND:
+		hci_cancel_le_scan(hdev);
+		break;
+	case HCI_CONN_LE_INITIATE:
+		hci_le_create_connection(conn);
+		break;
+	}
+
+	atomic_set(&conn->le_state, state);
+}
-- 
1.8.1.2

[RFC v2 4/8] Bluetooth: Add hci_conn helpers

[Andre Guedes]

This patch adds two helper functions for looking up hci_conn objects
according to its LE state. hci_conn_has_le_pending helper returns
a pending connection (hci_conn in HCI_CONN_LE_SCAN state) if any.
hci_conn_hash_lookup_le_state helper performs a lookup for hci_conn
objects in a given LE state.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |  3 +++
 net/bluetooth/hci_conn.c         | 47 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c704737..c797717 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -612,6 +612,9 @@ int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
 int hci_conn_change_link_key(struct hci_conn *conn);
 int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
 void hci_conn_set_le_state(struct hci_conn *conn, int state);
+struct hci_conn *hci_conn_has_le_pending(struct hci_dev *hdev, bdaddr_t *addr,
+					 __u8 type);
+struct hci_conn *hci_conn_hash_lookup_le_state(struct hci_dev *hdev, int state);
 
 void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
 
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index d54c2a0..b1a162f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1053,3 +1053,50 @@ void hci_conn_set_le_state(struct hci_conn *conn, int state)
 
 	atomic_set(&conn->le_state, state);
 }
+
+struct hci_conn *hci_conn_has_le_pending(struct hci_dev *hdev, bdaddr_t *addr,
+					 __u8 type)
+{
+	struct hci_conn *conn;
+
+	conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, addr);
+	if (!conn)
+		return NULL;
+
+	if (conn->state != BT_CONNECT)
+		return NULL;
+
+	if (conn->dst_type != type)
+		return NULL;
+
+	if (atomic_read(&conn->le_state) != HCI_CONN_LE_SCAN)
+		return NULL;
+
+	return conn;
+}
+
+struct hci_conn *hci_conn_hash_lookup_le_state(struct hci_dev *hdev, int state)
+{
+	struct hci_conn_hash *conn_hash = &hdev->conn_hash;
+	struct hci_conn *conn, *ret = NULL;
+
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(conn, &conn_hash->list, list) {
+		if (conn->type != LE_LINK)
+			continue;
+
+		if (conn->state != BT_CONNECT)
+			continue;
+
+		if (atomic_read(&conn->le_state) != state)
+			continue;
+
+		ret = conn;
+		break;
+	}
+
+	rcu_read_unlock();
+
+	return ret;
+}
-- 
1.8.1.2

[RFC v2 5/8] Bluetooth: Change LE connection routine

[Andre Guedes]

In order to better support LE connection requirements, such as multiple
connections and re-connections, we should use the general connection
establishment procedure described in Core spec. Today, we use the direct
connection establishment procedure which has some limitations and,
therefore, requires extra connection management at user-space in order
to support LE connection requirements.

According to the spec, the general procedure is described as follows:
The host starts scanning for LE devices, once the device we want to
connect to is in-range, the host stops scanning and initiates a
connection. The procedure is terminated when the connection is
established or when the host terminates the procedure.

This patch changes the LE connection routine so we perform the general
procedure instead of the direct procedure.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_conn.c  |  4 +++-
 net/bluetooth/hci_event.c | 19 +++++++++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b1a162f..308c87a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -517,7 +517,9 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 			return ERR_PTR(-ENOMEM);
 
 		le->dst_type = bdaddr_to_le(dst_type);
-		hci_le_create_connection(le);
+		le->state = BT_CONNECT;
+
+		hci_conn_set_le_state(le, HCI_CONN_LE_SCAN);
 	}
 
 	le->pending_sec_level = sec_level;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 477726a..84648c8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1260,6 +1260,7 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 {
 	struct hci_cp_le_set_scan_enable *cp;
 	__u8 status = *((__u8 *) skb->data);
+	struct hci_conn *hcon;
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
 
@@ -1295,8 +1296,15 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 
 		clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
 
-		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
-		    hdev->discovery.state == DISCOVERY_FINDING) {
+		hcon = hci_conn_hash_lookup_le_state(hdev, HCI_CONN_LE_FOUND);
+		if (hcon) {
+			hci_dev_lock(hdev);
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+			hci_dev_unlock(hdev);
+
+			hci_conn_set_le_state(hcon, HCI_CONN_LE_INITIATE);
+		} else if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
+			   hdev->discovery.state == DISCOVERY_FINDING) {
 			mgmt_interleaved_discovery(hdev);
 		} else {
 			hci_dev_lock(hdev);
@@ -3971,6 +3979,7 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	conn->sec_level = BT_SECURITY_LOW;
 	conn->handle = __le16_to_cpu(ev->handle);
+	hci_conn_set_le_state(conn, 0);
 	conn->state = BT_CONNECTED;
 
 	hci_conn_hold_device(conn);
@@ -3987,10 +3996,16 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	u8 num_reports = skb->data[0];
 	void *ptr = &skb->data[1];
 	s8 rssi;
+	struct hci_conn *hcon;
 
 	while (num_reports--) {
 		struct hci_ev_le_advertising_info *ev = ptr;
 
+		hcon = hci_conn_has_le_pending(hdev, &ev->bdaddr,
+					       ev->bdaddr_type);
+		if (hcon)
+			hci_conn_set_le_state(hcon, HCI_CONN_LE_FOUND);
+
 		rssi = ev->data[ev->length];
 		mgmt_device_found(hdev, &ev->bdaddr, LE_LINK, ev->bdaddr_type,
 				  NULL, rssi, 0, 1, ev->data, ev->length);
-- 
1.8.1.2

[RFC v2 6/8] Bluetooth: Handle hci_conn timeout in BT_CONNECT

[Andre Guedes]

In order to properly cancel an ongoing LE connection attempt (hci_conn
in BT_CONNECT state), we need to check the current le_state.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_conn.c  | 20 +++++++++++++++++++-
 net/bluetooth/hci_event.c | 11 +++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 308c87a..e259745 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -258,6 +258,24 @@ static void hci_conn_disconnect(struct hci_conn *conn)
 	}
 }
 
+static void cancel_le_connection(struct hci_conn *conn)
+{
+	struct hci_dev *hdev = conn->hdev;
+
+	BT_DBG("conn %p le state %d", conn, atomic_read(&conn->le_state));
+
+	switch (atomic_read(&conn->le_state)) {
+	case HCI_CONN_LE_SCAN:
+		hci_cancel_le_scan(hdev);
+		break;
+	case HCI_CONN_LE_INITIATE:
+		hci_le_create_connection_cancel(conn);
+		break;
+	}
+
+	hci_conn_set_le_state(conn, 0);
+}
+
 static void hci_conn_timeout(struct work_struct *work)
 {
 	struct hci_conn *conn = container_of(work, struct hci_conn,
@@ -275,7 +293,7 @@ static void hci_conn_timeout(struct work_struct *work)
 			if (conn->type == ACL_LINK)
 				hci_acl_create_connection_cancel(conn);
 			else if (conn->type == LE_LINK)
-				hci_le_create_connection_cancel(conn);
+				cancel_le_connection(conn);
 		}
 		break;
 	case BT_CONFIG:
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 84648c8..625eada 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1296,6 +1296,17 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 
 		clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
 
+		hcon = hci_conn_hash_lookup_le_state(hdev, 0);
+		if (hcon) {
+			mgmt_connect_failed(hdev, &hcon->dst, hcon->type,
+					    hcon->dst_type,
+					    HCI_ERROR_LOCAL_HOST_TERM);
+
+			hcon->state = BT_CLOSED;
+			hci_proto_connect_cfm(hcon, HCI_ERROR_LOCAL_HOST_TERM);
+			hci_conn_del(hcon);
+		}
+
 		hcon = hci_conn_hash_lookup_le_state(hdev, HCI_CONN_LE_FOUND);
 		if (hcon) {
 			hci_dev_lock(hdev);
-- 
1.8.1.2

[RFC v2 7/8] Bluetooth: Count pending LE connection attempts

[Andre Guedes]

To avoid traversing the hci_conn list every time we get an advertising
report, we keep a counter of pending LE connection attempts (connections
in HCI_CONN_LE_SCAN state). This way, we only traverse the list if the
counter is greater from zero.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h | 6 ++++++
 net/bluetooth/hci_conn.c         | 6 ++++++
 net/bluetooth/hci_core.c         | 2 ++
 3 files changed, 14 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c797717..5f2a991 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -290,6 +290,12 @@ struct hci_dev {
 	__u8			adv_data[HCI_MAX_AD_LENGTH];
 	__u8			adv_data_len;
 
+	/*
+	 * This counter tracks the number of pending LE connections
+	 * (connections in HCI_CONN_LE_SCAN state).
+	 */
+	atomic_t		le_conn_pend;
+
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
 	int (*flush)(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index e259745..29192e0 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -266,6 +266,7 @@ static void cancel_le_connection(struct hci_conn *conn)
 
 	switch (atomic_read(&conn->le_state)) {
 	case HCI_CONN_LE_SCAN:
+		atomic_dec(&hdev->le_conn_pend);
 		hci_cancel_le_scan(hdev);
 		break;
 	case HCI_CONN_LE_INITIATE:
@@ -1061,9 +1062,11 @@ void hci_conn_set_le_state(struct hci_conn *conn, int state)
 
 	switch (state) {
 	case HCI_CONN_LE_SCAN:
+		atomic_inc(&hdev->le_conn_pend);
 		hci_le_scan(hdev, LE_SCAN_PASSIVE, 0x60, 0x30, 0);
 		break;
 	case HCI_CONN_LE_FOUND:
+		atomic_dec(&hdev->le_conn_pend);
 		hci_cancel_le_scan(hdev);
 		break;
 	case HCI_CONN_LE_INITIATE:
@@ -1079,6 +1082,9 @@ struct hci_conn *hci_conn_has_le_pending(struct hci_dev *hdev, bdaddr_t *addr,
 {
 	struct hci_conn *conn;
 
+	if (atomic_read(&hdev->le_conn_pend) == 0)
+		return NULL;
+
 	conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, addr);
 	if (!conn)
 		return NULL;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3aa0345..acdfe15 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1742,6 +1742,8 @@ struct hci_dev *hci_alloc_dev(void)
 	hci_init_sysfs(hdev);
 	discovery_init(hdev);
 
+	atomic_set(&hdev->le_conn_pend, 0);
+
 	return hdev;
 }
 EXPORT_SYMBOL(hci_alloc_dev);
-- 
1.8.1.2

[RFC v2 8/8] Bluetooth: Initialize some hci_conn fields earlier

[Andre Guedes]

This patch moves some hci_conn fields initialization to hci_connect_le.
There is no reason we initialize them later at hci_le_create_connection.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_conn.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 29192e0..93b594b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -36,11 +36,6 @@ static void hci_le_create_connection(struct hci_conn *conn)
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_le_create_conn cp;
 
-	conn->state = BT_CONNECT;
-	conn->out = true;
-	conn->link_mode |= HCI_LM_MASTER;
-	conn->sec_level = BT_SECURITY_LOW;
-
 	memset(&cp, 0, sizeof(cp));
 	cp.scan_interval = __constant_cpu_to_le16(0x0060);
 	cp.scan_window = __constant_cpu_to_le16(0x0030);
@@ -537,6 +532,9 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 
 		le->dst_type = bdaddr_to_le(dst_type);
 		le->state = BT_CONNECT;
+		le->out = true;
+		le->link_mode |= HCI_LM_MASTER;
+		le->sec_level = BT_SECURITY_LOW;
 
 		hci_conn_set_le_state(le, HCI_CONN_LE_SCAN);
 	}
-- 
1.8.1.2

Re: [RFC v2 1/8] Bluetooth: Setup LE scan with no timeout

[Marcel Holtmann]

Hi Andre,

> This patch modifies hci_do_le_scan and hci_cancel_le_scan helpers so
> we are able to start and stop LE scan with no timeout. This feature
> will be used by the LE connection routine.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  net/bluetooth/hci_core.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 22e77a7..3aa0345 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1618,26 +1618,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
>  	if (err < 0)
>  		return err;
>  
> -	queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
> -			   msecs_to_jiffies(timeout));
> +	if (timeout > 0)
> +		queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
> +				   msecs_to_jiffies(timeout));
>  
>  	return 0;
>  }

I really do not like this magic handling of scan disable. Maybe you
better remove the timeout handling completely and put it into the
discovery functionality.

Doing it like this seems pretty much hacked together. It no longer looks
like the right place to do handle it.

Regards

Marcel

Re: [RFC v2 3/8] Bluetooth: LE connection state machine

[Marcel Holtmann]

Hi Andre,

> This patch implements a state machine for carrying out the general
> connection establishment procedure described in Core spec.
> 
> The state machine should be used as follows: when the kernel
> receives a new LE connection attempt, it should go to HCI_CONN_LE_
> SCAN state, starting the passive LE scanning. Once the target remote
> device is in-range, it should go to HCI_CONN_LE_FOUND, stopping the
> ongoing LE scanning. After the LE scanning is disabled, it should go
> to HCI_CONN_LE_INITIATE state where the LE connection is created.
> 
> This state machine will be used by the LE connection routine in
> order to establish LE connections.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h | 13 +++++++++++++
>  net/bluetooth/hci_conn.c         | 26 ++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 48c28ca..c704737 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -300,6 +300,16 @@ struct hci_dev {
>  
>  #define HCI_PHY_HANDLE(handle)	(handle & 0xff)
>  
> +/*
> + * States from LE connection establishment state machine.
> + * State 0 is reserved and indicates the state machine is not running.
> + */
> +enum {
> +	HCI_CONN_LE_SCAN = 1,
> +	HCI_CONN_LE_FOUND,
> +	HCI_CONN_LE_INITIATE,
> +};
> +

and why don't we have HCI_CONN_LE_IDLE = 0 then. I do not get this magic
number handling when you introduce an enum anyway. You spent more time
explaining it with a comment instead of just using an extra enum entry.

>  struct hci_conn {
>  	struct list_head list;
>  
> @@ -356,6 +366,8 @@ struct hci_conn {
>  
>  	struct hci_conn	*link;
>  
> +	atomic_t	le_state;
> +

Why is this atomic_t. I am lost on what you are trying to solve here.
This requires a detailed explanation or you just get rid of it and use
the enum.

>  	void (*connect_cfm_cb)	(struct hci_conn *conn, u8 status);
>  	void (*security_cfm_cb)	(struct hci_conn *conn, u8 status);
>  	void (*disconn_cfm_cb)	(struct hci_conn *conn, u8 reason);
> @@ -599,6 +611,7 @@ int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
>  int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
>  int hci_conn_change_link_key(struct hci_conn *conn);
>  int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
> +void hci_conn_set_le_state(struct hci_conn *conn, int state);

External state setting. That does not look like a good idea in the first
place. Why do you want to do it like this. Shouldn't be this self
contained.

>  
>  void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
>  
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 25bfce0..d54c2a0 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -391,6 +391,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
>  		    (unsigned long) conn);
>  
>  	atomic_set(&conn->refcnt, 0);
> +	atomic_set(&conn->le_state, 0);
>  
>  	hci_dev_hold(hdev);
>  
> @@ -1027,3 +1028,28 @@ struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle)
>  
>  	return hchan;
>  }
> +
> +void hci_conn_set_le_state(struct hci_conn *conn, int state)
> +{
> +	struct hci_dev *hdev = conn->hdev;
> +
> +	BT_DBG("conn %p state %d -> %d", conn, atomic_read(&conn->le_state),
> +	       state);
> +
> +	if (state == atomic_read(&conn->le_state))
> +		return;
> +
> +	switch (state) {
> +	case HCI_CONN_LE_SCAN:
> +		hci_le_scan(hdev, LE_SCAN_PASSIVE, 0x60, 0x30, 0);
> +		break;
> +	case HCI_CONN_LE_FOUND:
> +		hci_cancel_le_scan(hdev);
> +		break;
> +	case HCI_CONN_LE_INITIATE:
> +		hci_le_create_connection(conn);
> +		break;
> +	}
> +
> +	atomic_set(&conn->le_state, state);
> +}

The more I read this, the more I think this is the wrong approach to
this. The kernel should have a list of device it wants to connect to. If
that list is non-empty, start scanning, if one device is found, try to
connect it, once done, keep scanning if other devices are not connected.

You need to build a foundation for LE devices that the kernel wants to
connect to. And not hack in some state machinery.

And of course, can we please do proper error handling here. This whole
thing is broken. If any of the HCI commands fail, your state machine is
screwed up.

Regards

Marcel

Re: [RFC v2 1/8] Bluetooth: Setup LE scan with no timeout

[Andre Guedes]

Hi Marcel,

On Sat, Feb 16, 2013 at 8:26 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> This patch modifies hci_do_le_scan and hci_cancel_le_scan helpers so
>> we are able to start and stop LE scan with no timeout. This feature
>> will be used by the LE connection routine.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>>  net/bluetooth/hci_core.c | 17 +++++++++--------
>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 22e77a7..3aa0345 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1618,26 +1618,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
>>       if (err < 0)
>>               return err;
>>
>> -     queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
>> -                        msecs_to_jiffies(timeout));
>> +     if (timeout > 0)
>> +             queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
>> +                                msecs_to_jiffies(timeout));
>>
>>       return 0;
>>  }
>
> I really do not like this magic handling of scan disable. Maybe you
> better remove the timeout handling completely and put it into the
> discovery functionality.
>
> Doing it like this seems pretty much hacked together. It no longer looks
> like the right place to do handle it.

The le_scan_disable work should be scheduled only if LE scanning was
started successfully.

So hci_do_le_scan seems to be a better place than discovery
functionality for the timeout handling. Besides, these LE scan helpers
were originally designed to provide a self-contained framework to
start and stop LE scanning.

The framework lacks support for starting LE scanning with no timeout.
This patch aims to address this issue.

Anyway, if you still think it is a good idea to remove this timeout
handling, I'll remove it in the next series.

Regards,

Andre

Re: [RFC v2 3/8] Bluetooth: LE connection state machine

[Andre Guedes]

Hi Marcel,

On Sat, Feb 16, 2013 at 8:32 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> This patch implements a state machine for carrying out the general
>> connection establishment procedure described in Core spec.
>>
>> The state machine should be used as follows: when the kernel
>> receives a new LE connection attempt, it should go to HCI_CONN_LE_
>> SCAN state, starting the passive LE scanning. Once the target remote
>> device is in-range, it should go to HCI_CONN_LE_FOUND, stopping the
>> ongoing LE scanning. After the LE scanning is disabled, it should go
>> to HCI_CONN_LE_INITIATE state where the LE connection is created.
>>
>> This state machine will be used by the LE connection routine in
>> order to establish LE connections.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>>  include/net/bluetooth/hci_core.h | 13 +++++++++++++
>>  net/bluetooth/hci_conn.c         | 26 ++++++++++++++++++++++++++
>>  2 files changed, 39 insertions(+)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 48c28ca..c704737 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -300,6 +300,16 @@ struct hci_dev {
>>
>>  #define HCI_PHY_HANDLE(handle)       (handle & 0xff)
>>
>> +/*
>> + * States from LE connection establishment state machine.
>> + * State 0 is reserved and indicates the state machine is not running.
>> + */
>> +enum {
>> +     HCI_CONN_LE_SCAN = 1,
>> +     HCI_CONN_LE_FOUND,
>> +     HCI_CONN_LE_INITIATE,
>> +};
>> +
>
> and why don't we have HCI_CONN_LE_IDLE = 0 then. I do not get this magic
> number handling when you introduce an enum anyway. You spent more time
> explaining it with a comment instead of just using an extra enum entry.

Ok, I'll introduce the HCI_CONN_LE_IDLE state.

>>  struct hci_conn {
>>       struct list_head list;
>>
>> @@ -356,6 +366,8 @@ struct hci_conn {
>>
>>       struct hci_conn *link;
>>
>> +     atomic_t        le_state;
>> +
>
> Why is this atomic_t. I am lost on what you are trying to solve here.
> This requires a detailed explanation or you just get rid of it and use
> the enum.

le_state was defined as atomic_t so read and write are atomic
operations, avoiding to acquire hdev->lock every time we want to read
or write this variable.

>>       void (*connect_cfm_cb)  (struct hci_conn *conn, u8 status);
>>       void (*security_cfm_cb) (struct hci_conn *conn, u8 status);
>>       void (*disconn_cfm_cb)  (struct hci_conn *conn, u8 reason);
>> @@ -599,6 +611,7 @@ int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
>>  int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
>>  int hci_conn_change_link_key(struct hci_conn *conn);
>>  int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
>> +void hci_conn_set_le_state(struct hci_conn *conn, int state);
>
> External state setting. That does not look like a good idea in the first
> place. Why do you want to do it like this. Shouldn't be this self
> contained.

We need to set le_state variable in hci_event.c (see patch 5/8). That
is why this function was added to hci_core.h.

>>  void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
>>
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index 25bfce0..d54c2a0 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -391,6 +391,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
>>                   (unsigned long) conn);
>>
>>       atomic_set(&conn->refcnt, 0);
>> +     atomic_set(&conn->le_state, 0);
>>
>>       hci_dev_hold(hdev);
>>
>> @@ -1027,3 +1028,28 @@ struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle)
>>
>>       return hchan;
>>  }
>> +
>> +void hci_conn_set_le_state(struct hci_conn *conn, int state)
>> +{
>> +     struct hci_dev *hdev = conn->hdev;
>> +
>> +     BT_DBG("conn %p state %d -> %d", conn, atomic_read(&conn->le_state),
>> +            state);
>> +
>> +     if (state == atomic_read(&conn->le_state))
>> +             return;
>> +
>> +     switch (state) {
>> +     case HCI_CONN_LE_SCAN:
>> +             hci_le_scan(hdev, LE_SCAN_PASSIVE, 0x60, 0x30, 0);
>> +             break;
>> +     case HCI_CONN_LE_FOUND:
>> +             hci_cancel_le_scan(hdev);
>> +             break;
>> +     case HCI_CONN_LE_INITIATE:
>> +             hci_le_create_connection(conn);
>> +             break;
>> +     }
>> +
>> +     atomic_set(&conn->le_state, state);
>> +}
>
> The more I read this, the more I think this is the wrong approach to
> this. The kernel should have a list of device it wants to connect to. If
> that list is non-empty, start scanning, if one device is found, try to
> connect it, once done, keep scanning if other devices are not connected.
>
> You need to build a foundation for LE devices that the kernel wants to
> connect to. And not hack in some state machinery.

As commented in the cover letter, this RFC series is an initial work,
it supports only one LE connection attempt at a time. It doesn't
support multiple LE connection attempts and doesn't handle concurrent
device discovery properly yet.

Let me try to give you the big picture:
LE connection attempts come from user-space through the connect()
call. Each connect() call adds a hci_conn object into a connection
list (hdev->conn_hash) and start the LE passive scan if not already
running. So the kernel has a list of devices it wants to connect to
(hci_conn objects in HCI_CONN_LE_SCAN le_state). When the target
device is found, we stop the scanning and initiate the connection.
Once the connection is done (successfully or not), we start passive
scanning again if we still have devices we want to connect to.

I think this is pretty much the approach you described. Are we on the
same page about this approach?

> And of course, can we please do proper error handling here. This whole
> thing is broken. If any of the HCI commands fail, your state machine is
> screwed up.

Sure, as I said in the cover letter, the next step is to handle corner
cases like that.

In next RFC series I'll cover all those corner cases and I'll also add
support for multiple LE connections attempts. I believe the next RFC
series will clarify things a little bit.

Regards,

Andre

Re: [RFC v2 1/8] Bluetooth: Setup LE scan with no timeout

[Marcel Holtmann]

Hi Andre,

> >> This patch modifies hci_do_le_scan and hci_cancel_le_scan helpers so
> >> we are able to start and stop LE scan with no timeout. This feature
> >> will be used by the LE connection routine.
> >>
> >> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> >> ---
> >>  net/bluetooth/hci_core.c | 17 +++++++++--------
> >>  1 file changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >> index 22e77a7..3aa0345 100644
> >> --- a/net/bluetooth/hci_core.c
> >> +++ b/net/bluetooth/hci_core.c
> >> @@ -1618,26 +1618,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
> >>       if (err < 0)
> >>               return err;
> >>
> >> -     queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
> >> -                        msecs_to_jiffies(timeout));
> >> +     if (timeout > 0)
> >> +             queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
> >> +                                msecs_to_jiffies(timeout));
> >>
> >>       return 0;
> >>  }
> >
> > I really do not like this magic handling of scan disable. Maybe you
> > better remove the timeout handling completely and put it into the
> > discovery functionality.
> >
> > Doing it like this seems pretty much hacked together. It no longer looks
> > like the right place to do handle it.
> 
> The le_scan_disable work should be scheduled only if LE scanning was
> started successfully.
> 
> So hci_do_le_scan seems to be a better place than discovery
> functionality for the timeout handling. Besides, these LE scan helpers
> were originally designed to provide a self-contained framework to
> start and stop LE scanning.
> 
> The framework lacks support for starting LE scanning with no timeout.
> This patch aims to address this issue.
> 
> Anyway, if you still think it is a good idea to remove this timeout
> handling, I'll remove it in the next series.

look into what Johan is working on with the transaction support. I am
getting the feeling that moving everything over to proper transaction
and not just trying it hack stuff in seems to be the right approach.

For example the actual discovery handling needs to be able to report
errors anyway. So that can be handled there.

Regards

Marcel

Re: [RFC v2 3/8] Bluetooth: LE connection state machine

[Marcel Holtmann]

Hi Andre,

> >> This patch implements a state machine for carrying out the general
> >> connection establishment procedure described in Core spec.
> >>
> >> The state machine should be used as follows: when the kernel
> >> receives a new LE connection attempt, it should go to HCI_CONN_LE_
> >> SCAN state, starting the passive LE scanning. Once the target remote
> >> device is in-range, it should go to HCI_CONN_LE_FOUND, stopping the
> >> ongoing LE scanning. After the LE scanning is disabled, it should go
> >> to HCI_CONN_LE_INITIATE state where the LE connection is created.
> >>
> >> This state machine will be used by the LE connection routine in
> >> order to establish LE connections.
> >>
> >> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> >> ---
> >>  include/net/bluetooth/hci_core.h | 13 +++++++++++++
> >>  net/bluetooth/hci_conn.c         | 26 ++++++++++++++++++++++++++
> >>  2 files changed, 39 insertions(+)
> >>
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index 48c28ca..c704737 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -300,6 +300,16 @@ struct hci_dev {
> >>
> >>  #define HCI_PHY_HANDLE(handle)       (handle & 0xff)
> >>
> >> +/*
> >> + * States from LE connection establishment state machine.
> >> + * State 0 is reserved and indicates the state machine is not running.
> >> + */
> >> +enum {
> >> +     HCI_CONN_LE_SCAN = 1,
> >> +     HCI_CONN_LE_FOUND,
> >> +     HCI_CONN_LE_INITIATE,
> >> +};
> >> +
> >
> > and why don't we have HCI_CONN_LE_IDLE = 0 then. I do not get this magic
> > number handling when you introduce an enum anyway. You spent more time
> > explaining it with a comment instead of just using an extra enum entry.
> 
> Ok, I'll introduce the HCI_CONN_LE_IDLE state.
> 
> >>  struct hci_conn {
> >>       struct list_head list;
> >>
> >> @@ -356,6 +366,8 @@ struct hci_conn {
> >>
> >>       struct hci_conn *link;
> >>
> >> +     atomic_t        le_state;
> >> +
> >
> > Why is this atomic_t. I am lost on what you are trying to solve here.
> > This requires a detailed explanation or you just get rid of it and use
> > the enum.
> 
> le_state was defined as atomic_t so read and write are atomic
> operations, avoiding to acquire hdev->lock every time we want to read
> or write this variable.

that seems pretty much short sighted. Use a proper look and try not to
mix an atomic in here.

> >>       void (*connect_cfm_cb)  (struct hci_conn *conn, u8 status);
> >>       void (*security_cfm_cb) (struct hci_conn *conn, u8 status);
> >>       void (*disconn_cfm_cb)  (struct hci_conn *conn, u8 reason);
> >> @@ -599,6 +611,7 @@ int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
> >>  int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
> >>  int hci_conn_change_link_key(struct hci_conn *conn);
> >>  int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
> >> +void hci_conn_set_le_state(struct hci_conn *conn, int state);
> >
> > External state setting. That does not look like a good idea in the first
> > place. Why do you want to do it like this. Shouldn't be this self
> > contained.
> 
> We need to set le_state variable in hci_event.c (see patch 5/8). That
> is why this function was added to hci_core.h.

That is not a good enough explanation. Doing it like this opens a can of
worms. I do not not get why we can not just trigger the next operation
from within the event callbacks.

You need to explain the point of all these indirection. Who is going to
read or even understand that in 12 month. I have a hard time to follow
by just reviewing these patches. That does not sound like a good
approach for re-designing the LE connection handling.

> >>  void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
> >>
> >> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> >> index 25bfce0..d54c2a0 100644
> >> --- a/net/bluetooth/hci_conn.c
> >> +++ b/net/bluetooth/hci_conn.c
> >> @@ -391,6 +391,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
> >>                   (unsigned long) conn);
> >>
> >>       atomic_set(&conn->refcnt, 0);
> >> +     atomic_set(&conn->le_state, 0);
> >>
> >>       hci_dev_hold(hdev);
> >>
> >> @@ -1027,3 +1028,28 @@ struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle)
> >>
> >>       return hchan;
> >>  }
> >> +
> >> +void hci_conn_set_le_state(struct hci_conn *conn, int state)
> >> +{
> >> +     struct hci_dev *hdev = conn->hdev;
> >> +
> >> +     BT_DBG("conn %p state %d -> %d", conn, atomic_read(&conn->le_state),
> >> +            state);
> >> +
> >> +     if (state == atomic_read(&conn->le_state))
> >> +             return;
> >> +
> >> +     switch (state) {
> >> +     case HCI_CONN_LE_SCAN:
> >> +             hci_le_scan(hdev, LE_SCAN_PASSIVE, 0x60, 0x30, 0);
> >> +             break;
> >> +     case HCI_CONN_LE_FOUND:
> >> +             hci_cancel_le_scan(hdev);
> >> +             break;
> >> +     case HCI_CONN_LE_INITIATE:
> >> +             hci_le_create_connection(conn);
> >> +             break;
> >> +     }
> >> +
> >> +     atomic_set(&conn->le_state, state);
> >> +}
> >
> > The more I read this, the more I think this is the wrong approach to
> > this. The kernel should have a list of device it wants to connect to. If
> > that list is non-empty, start scanning, if one device is found, try to
> > connect it, once done, keep scanning if other devices are not connected.
> >
> > You need to build a foundation for LE devices that the kernel wants to
> > connect to. And not hack in some state machinery.
> 
> As commented in the cover letter, this RFC series is an initial work,
> it supports only one LE connection attempt at a time. It doesn't
> support multiple LE connection attempts and doesn't handle concurrent
> device discovery properly yet.
> 
> Let me try to give you the big picture:
> LE connection attempts come from user-space through the connect()
> call. Each connect() call adds a hci_conn object into a connection
> list (hdev->conn_hash) and start the LE passive scan if not already
> running. So the kernel has a list of devices it wants to connect to
> (hci_conn objects in HCI_CONN_LE_SCAN le_state). When the target
> device is found, we stop the scanning and initiate the connection.
> Once the connection is done (successfully or not), we start passive
> scanning again if we still have devices we want to connect to.
> 
> I think this is pretty much the approach you described. Are we on the
> same page about this approach?

I got what you are trying to achieve, but that is just for connection
handling of connect().

What I really care is about a bigger picture for handling auto-connect
and background scanning.

Same as I care about being able to use the white list as much as
possible. As long as we do not require IRK and the white list is large
enough, lets use it. If it is possible to use a white list, the power
impact will be large. If we can not use the white list, we have to
consider down periods for the passive scanning to allow the host to
sleep for certain amount of time.

> > And of course, can we please do proper error handling here. This whole
> > thing is broken. If any of the HCI commands fail, your state machine is
> > screwed up.
> 
> Sure, as I said in the cover letter, the next step is to handle corner
> cases like that.

Error cases are not corner cases. Can we please stop such a thinking. At
random times the controller will have resource limits and thus HCI
commands will fail. Either you handle that right away or this is a
pretty much useless attempt.

The reason I pointed you to Johan's work for transaction is actually the
case of being able to handle error conditions. With LE is has become
obvious that in a lot of cases you have more than just one HCI command
to trigger some actions. The handling gets a lot more complicated. We
will have many small state machines to keep track. This needs clean code
that can be understood by more than just 2 people.

Regards

Marcel

Re: [RFC v2 1/8] Bluetooth: Setup LE scan with no timeout

[Andre Guedes]

Hi Marcel,

On Mon, Feb 18, 2013 at 7:41 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> >> This patch modifies hci_do_le_scan and hci_cancel_le_scan helpers so
>> >> we are able to start and stop LE scan with no timeout. This feature
>> >> will be used by the LE connection routine.
>> >>
>> >> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> >> ---
>> >>  net/bluetooth/hci_core.c | 17 +++++++++--------
>> >>  1 file changed, 9 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> >> index 22e77a7..3aa0345 100644
>> >> --- a/net/bluetooth/hci_core.c
>> >> +++ b/net/bluetooth/hci_core.c
>> >> @@ -1618,26 +1618,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
>> >>       if (err < 0)
>> >>               return err;
>> >>
>> >> -     queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
>> >> -                        msecs_to_jiffies(timeout));
>> >> +     if (timeout > 0)
>> >> +             queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
>> >> +                                msecs_to_jiffies(timeout));
>> >>
>> >>       return 0;
>> >>  }
>> >
>> > I really do not like this magic handling of scan disable. Maybe you
>> > better remove the timeout handling completely and put it into the
>> > discovery functionality.
>> >
>> > Doing it like this seems pretty much hacked together. It no longer looks
>> > like the right place to do handle it.
>>
>> The le_scan_disable work should be scheduled only if LE scanning was
>> started successfully.
>>
>> So hci_do_le_scan seems to be a better place than discovery
>> functionality for the timeout handling. Besides, these LE scan helpers
>> were originally designed to provide a self-contained framework to
>> start and stop LE scanning.
>>
>> The framework lacks support for starting LE scanning with no timeout.
>> This patch aims to address this issue.
>>
>> Anyway, if you still think it is a good idea to remove this timeout
>> handling, I'll remove it in the next series.
>
> look into what Johan is working on with the transaction support. I am
> getting the feeling that moving everything over to proper transaction
> and not just trying it hack stuff in seems to be the right approach.
>
> For example the actual discovery handling needs to be able to report
> errors anyway. So that can be handled there.

Good, so I'll rebase on top of the latest hci transaction support and use it.

Regards,

Andre

Re: [RFC v2 3/8] Bluetooth: LE connection state machine

[Andre Guedes]

Hi Marcel,

On Mon, Feb 18, 2013 at 7:50 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> >> This patch implements a state machine for carrying out the general
>> >> connection establishment procedure described in Core spec.
>> >>
>> >> The state machine should be used as follows: when the kernel
>> >> receives a new LE connection attempt, it should go to HCI_CONN_LE_
>> >> SCAN state, starting the passive LE scanning. Once the target remote
>> >> device is in-range, it should go to HCI_CONN_LE_FOUND, stopping the
>> >> ongoing LE scanning. After the LE scanning is disabled, it should go
>> >> to HCI_CONN_LE_INITIATE state where the LE connection is created.
>> >>
>> >> This state machine will be used by the LE connection routine in
>> >> order to establish LE connections.
>> >>
>> >> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> >> ---
>> >>  include/net/bluetooth/hci_core.h | 13 +++++++++++++
>> >>  net/bluetooth/hci_conn.c         | 26 ++++++++++++++++++++++++++
>> >>  2 files changed, 39 insertions(+)
>> >>
>> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> >> index 48c28ca..c704737 100644
>> >> --- a/include/net/bluetooth/hci_core.h
>> >> +++ b/include/net/bluetooth/hci_core.h
>> >> @@ -300,6 +300,16 @@ struct hci_dev {
>> >>
>> >>  #define HCI_PHY_HANDLE(handle)       (handle & 0xff)
>> >>
>> >> +/*
>> >> + * States from LE connection establishment state machine.
>> >> + * State 0 is reserved and indicates the state machine is not running.
>> >> + */
>> >> +enum {
>> >> +     HCI_CONN_LE_SCAN = 1,
>> >> +     HCI_CONN_LE_FOUND,
>> >> +     HCI_CONN_LE_INITIATE,
>> >> +};
>> >> +
>> >
>> > and why don't we have HCI_CONN_LE_IDLE = 0 then. I do not get this magic
>> > number handling when you introduce an enum anyway. You spent more time
>> > explaining it with a comment instead of just using an extra enum entry.
>>
>> Ok, I'll introduce the HCI_CONN_LE_IDLE state.
>>
>> >>  struct hci_conn {
>> >>       struct list_head list;
>> >>
>> >> @@ -356,6 +366,8 @@ struct hci_conn {
>> >>
>> >>       struct hci_conn *link;
>> >>
>> >> +     atomic_t        le_state;
>> >> +
>> >
>> > Why is this atomic_t. I am lost on what you are trying to solve here.
>> > This requires a detailed explanation or you just get rid of it and use
>> > the enum.
>>
>> le_state was defined as atomic_t so read and write are atomic
>> operations, avoiding to acquire hdev->lock every time we want to read
>> or write this variable.
>
> that seems pretty much short sighted. Use a proper look and try not to
> mix an atomic in here.

Ok, I'll use lock instead.

>> >>       void (*connect_cfm_cb)  (struct hci_conn *conn, u8 status);
>> >>       void (*security_cfm_cb) (struct hci_conn *conn, u8 status);
>> >>       void (*disconn_cfm_cb)  (struct hci_conn *conn, u8 reason);
>> >> @@ -599,6 +611,7 @@ int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
>> >>  int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
>> >>  int hci_conn_change_link_key(struct hci_conn *conn);
>> >>  int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
>> >> +void hci_conn_set_le_state(struct hci_conn *conn, int state);
>> >
>> > External state setting. That does not look like a good idea in the first
>> > place. Why do you want to do it like this. Shouldn't be this self
>> > contained.
>>
>> We need to set le_state variable in hci_event.c (see patch 5/8). That
>> is why this function was added to hci_core.h.
>
> That is not a good enough explanation. Doing it like this opens a can of
> worms. I do not not get why we can not just trigger the next operation
> from within the event callbacks.
>
> You need to explain the point of all these indirection. Who is going to
> read or even understand that in 12 month. I have a hard time to follow
> by just reviewing these patches. That does not sound like a good
> approach for re-designing the LE connection handling.

Fair enough, I'll change this and use callbacks to trigger the next state.

>> >>  void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
>> >>
>> >> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> >> index 25bfce0..d54c2a0 100644
>> >> --- a/net/bluetooth/hci_conn.c
>> >> +++ b/net/bluetooth/hci_conn.c
>> >> @@ -391,6 +391,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
>> >>                   (unsigned long) conn);
>> >>
>> >>       atomic_set(&conn->refcnt, 0);
>> >> +     atomic_set(&conn->le_state, 0);
>> >>
>> >>       hci_dev_hold(hdev);
>> >>
>> >> @@ -1027,3 +1028,28 @@ struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle)
>> >>
>> >>       return hchan;
>> >>  }
>> >> +
>> >> +void hci_conn_set_le_state(struct hci_conn *conn, int state)
>> >> +{
>> >> +     struct hci_dev *hdev = conn->hdev;
>> >> +
>> >> +     BT_DBG("conn %p state %d -> %d", conn, atomic_read(&conn->le_state),
>> >> +            state);
>> >> +
>> >> +     if (state == atomic_read(&conn->le_state))
>> >> +             return;
>> >> +
>> >> +     switch (state) {
>> >> +     case HCI_CONN_LE_SCAN:
>> >> +             hci_le_scan(hdev, LE_SCAN_PASSIVE, 0x60, 0x30, 0);
>> >> +             break;
>> >> +     case HCI_CONN_LE_FOUND:
>> >> +             hci_cancel_le_scan(hdev);
>> >> +             break;
>> >> +     case HCI_CONN_LE_INITIATE:
>> >> +             hci_le_create_connection(conn);
>> >> +             break;
>> >> +     }
>> >> +
>> >> +     atomic_set(&conn->le_state, state);
>> >> +}
>> >
>> > The more I read this, the more I think this is the wrong approach to
>> > this. The kernel should have a list of device it wants to connect to. If
>> > that list is non-empty, start scanning, if one device is found, try to
>> > connect it, once done, keep scanning if other devices are not connected.
>> >
>> > You need to build a foundation for LE devices that the kernel wants to
>> > connect to. And not hack in some state machinery.
>>
>> As commented in the cover letter, this RFC series is an initial work,
>> it supports only one LE connection attempt at a time. It doesn't
>> support multiple LE connection attempts and doesn't handle concurrent
>> device discovery properly yet.
>>
>> Let me try to give you the big picture:
>> LE connection attempts come from user-space through the connect()
>> call. Each connect() call adds a hci_conn object into a connection
>> list (hdev->conn_hash) and start the LE passive scan if not already
>> running. So the kernel has a list of devices it wants to connect to
>> (hci_conn objects in HCI_CONN_LE_SCAN le_state). When the target
>> device is found, we stop the scanning and initiate the connection.
>> Once the connection is done (successfully or not), we start passive
>> scanning again if we still have devices we want to connect to.
>>
>> I think this is pretty much the approach you described. Are we on the
>> same page about this approach?
>
> I got what you are trying to achieve, but that is just for connection
> handling of connect().
>
> What I really care is about a bigger picture for handling auto-connect
> and background scanning.

Ok, let me try an even bigger picture about the whole LE connection:
At kernel-space, as already said, the connect() call implements the
passive scanning + the connection attempt.
At user-space, we will simply call connect() to establish LE
connections. The whole code to add devices to the connect_list and
trigger the fake passive scanning can be removed (since this is now
handled by the kernel). If auto-connect is enabled and a disconnection
occurs, all we have to do is calling connect() in
attrib_disconnect_cb.

> Same as I care about being able to use the white list as much as
> possible. As long as we do not require IRK and the white list is large
> enough, lets use it. If it is possible to use a white list, the power
> impact will be large. If we can not use the white list, we have to
> consider down periods for the passive scanning to allow the host to
> sleep for certain amount of time.

Using white list requires some changes in hci_connect_le so it adds
the address into the white list instead of starting the passive
scanning and removes it if the connection is established successfully.
As soon as we have LE connection stable enough, we can work on the
white list support too.

We may achieve these down periods you mentioned by choosing
power-saving scanning parameters values (window and interval).
However, they will impact directly on the connection latency.

>> > And of course, can we please do proper error handling here. This whole
>> > thing is broken. If any of the HCI commands fail, your state machine is
>> > screwed up.
>>
>> Sure, as I said in the cover letter, the next step is to handle corner
>> cases like that.
>
> Error cases are not corner cases. Can we please stop such a thinking. At
> random times the controller will have resource limits and thus HCI
> commands will fail. Either you handle that right away or this is a
> pretty much useless attempt.
>
> The reason I pointed you to Johan's work for transaction is actually the
> case of being able to handle error conditions. With LE is has become
> obvious that in a lot of cases you have more than just one HCI command
> to trigger some actions. The handling gets a lot more complicated. We
> will have many small state machines to keep track. This needs clean code
> that can be understood by more than just 2 people.

Sure. I'll rebase on top of transactions patches and all error cases
will be handled in the next series.

Regards,

Andre