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

Hi Marcel,

On Sat, Feb 16, 2013 at 8:32 PM, Marcel Holtmann [off-list ref] wrote:

Hi Andre,

quoted

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 <redacted>
---
 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.

quoted

 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.

quoted

      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.

quoted

 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