[RFC 00/15] LE auto connection and connection parameters

[RFC 00/15] LE auto connection and connection parameters

[Andre Guedes]

Hi all,

This patchset adds support for the LE auto connection mechanism discussed
during the Wireless Summit in New Orleans.

The user configures the connection parameters and the auto connection
policy for each device according to the LE profiles that device supports.
The auto connection policies are:
* DISABLED:  kernel never establishes connections autonomously.
* LINK_LOSS: kernel automatically reconnects if connection terminates due to
             a link loss.
* ALWAYS:    kernel always autonomously connects to device

In order to implement the auto connection mechanism, it was introduced the
background scanning infrastructure which keeps the controller (passively)
scanning as long as there is auto connection pending. Also, two new Mgmt
commands are introduced to add and remove connection parameters for a certain
device.

This patchset is organized as follows:
* Patch 1 - 3:   Adds the new Mgmt commands.
* Patch 4 - 5:   Use connection parameters specified by user.
* Patch 6:       Introduce the background scan infrastructure.
* Patch 7 - 9:   Implement the auto connection policies. 
* Patch 10 - 11: Add special handling for devices that don't support scanning
                 and initiating connections at the same time.
* Patch 12 - 14: Add proper handling for auto connection and discovery running
                 at the same time.
* Patch 15:      Add special handling for power off/on.

Regards,

Andre


Andre Guedes (15):
  Bluetooth: Introduce connection parameters list
  Bluetooth: Mgmt command for adding connection parameters
  Bluetooth: Mgmt command for removing connection parameters
  Bluetooth: Make find_conn_param() helper non-local
  Bluetooth: Use connection parameters if any
  Bluetooth: Background scanning
  Bluetooth: Refactor hci_disconn_complete_evt
  Bluetooth: Add support for BT_AUTO_CONN_ALWAYS
  Bluetooth: Add support for BT_AUTO_CONN_LINK_LOSS option
  Bluetooth: Create start_background_scan helper
  Bluetooth: Temporarily stop background scanning on connection
  Bluetooth: Temporarily stop background scanning on discovery
  Bluetooth: Fix background trigger/untrigger functions
  Bluetooth: Fix hci_create_le_conn()
  Bluetooth: Auto connection and power off/on

 include/net/bluetooth/bluetooth.h |   6 +
 include/net/bluetooth/hci_core.h  |  36 ++++
 include/net/bluetooth/mgmt.h      |  15 ++
 net/bluetooth/hci_conn.c          |  77 ++++++++-
 net/bluetooth/hci_core.c          | 338 ++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c         | 112 +++++++++----
 net/bluetooth/mgmt.c              |  76 ++++++++-
 7 files changed, 626 insertions(+), 34 deletions(-)

-- 
1.8.4

[RFC 01/15] Bluetooth: Introduce connection parameters list

[Andre Guedes]

This patch adds to hdev the connection parameters list (hdev->
conn_param). The elements from this list (struct hci_conn_param)
contains the connection parameters (for now, minimum and maximum
connection interval) that should be used for establishing
connection.

The struct hci_conn_param also defines the 'auto_connect' field
which will be used to implement the auto connection mechanism.

Moreover, this patch adds helper functions to manipulate hdev->conn_
param list. Some of these functions are also declared in hci_core.h
since they will be used outside hci_core.c in upcoming patches.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07c2da4..5052bf0 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -266,6 +266,8 @@ struct hci_dev {
 
 	struct list_head	remote_oob_data;
 
+	struct list_head	conn_param;
+
 	struct hci_dev_stats	stat;
 
 	atomic_t		promisc;
@@ -368,6 +370,22 @@ struct hci_chan {
 	__u8		state;
 };
 
+struct hci_conn_param {
+	struct list_head list;
+
+	bdaddr_t addr;
+	u8 addr_type;
+
+	enum {
+		HCI_AUTO_CONN_DISABLED,
+		HCI_AUTO_CONN_ALWAYS,
+		HCI_AUTO_CONN_LINK_LOSS,
+	} auto_connect;
+
+	u16 min_conn_interval;
+	u16 max_conn_interval;
+};
+
 extern struct list_head hci_dev_list;
 extern struct list_head hci_cb_list;
 extern rwlock_t hci_dev_list_lock;
@@ -737,6 +755,11 @@ int hci_blacklist_clear(struct hci_dev *hdev);
 int hci_blacklist_add(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type);
 int hci_blacklist_del(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type);
 
+int hci_add_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
+		       u8 auto_connect, u16 min_conn_interval,
+		       u16 max_conn_interval);
+void hci_remove_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
+
 int hci_uuids_clear(struct hci_dev *hdev);
 
 int hci_link_keys_clear(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 73c8def..a4242ac 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2151,6 +2151,92 @@ int hci_blacklist_del(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type)
 	return mgmt_device_unblocked(hdev, bdaddr, type);
 }
 
+struct hci_conn_param *find_conn_param(struct hci_dev *hdev,
+				       bdaddr_t *addr, u8 addr_type)
+{
+	struct hci_conn_param *param;
+
+	rcu_read_lock();
+
+	list_for_each_entry(param, &hdev->conn_param, list) {
+		if (bacmp(&param->addr, addr))
+			continue;
+		if (param->addr_type != addr_type)
+			continue;
+
+		rcu_read_unlock();
+		return param;
+	}
+
+	rcu_read_unlock();
+	return NULL;
+}
+
+int hci_add_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
+		       u8 auto_connect, u16 min_conn_interval,
+		       u16 max_conn_interval)
+{
+	struct hci_conn_param *param;
+
+	param = find_conn_param(hdev, addr, addr_type);
+	if (param)
+		return -EEXIST;
+
+	param = kmalloc(sizeof(*param), GFP_KERNEL);
+	if (!param)
+		return -ENOMEM;
+
+	bacpy(&param->addr, addr);
+	param->addr_type = addr_type;
+	param->auto_connect = auto_connect;
+	param->min_conn_interval = min_conn_interval;
+	param->max_conn_interval = max_conn_interval;
+
+	hci_dev_lock(hdev);
+	list_add_rcu(&param->list, &hdev->conn_param);
+	hci_dev_unlock(hdev);
+	return 0;
+}
+
+/*
+ * Remove from hdev->conn_param and free hci_conn_param.
+ *
+ * This function requires the caller holds hdev->lock.
+ */
+static void __remove_conn_param(struct hci_conn_param *param)
+{
+	list_del_rcu(&param->list);
+	synchronize_rcu();
+
+	kfree(param);
+}
+
+void hci_remove_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
+{
+	struct hci_conn_param *param;
+
+	param = find_conn_param(hdev, addr, addr_type);
+	if (!param)
+		return;
+
+	hci_dev_lock(hdev);
+	__remove_conn_param(param);
+	hci_dev_unlock(hdev);
+}
+
+/*
+ * Remove all elements from hdev->conn_param list.
+ *
+ * This function requires the caller holds hdev->lock.
+ */
+static void __clear_conn_param(struct hci_dev *hdev)
+{
+	struct hci_conn_param *param, *tmp;
+
+	list_for_each_entry_safe(param, tmp, &hdev->conn_param, list)
+		__remove_conn_param(param);
+}
+
 static void inquiry_complete(struct hci_dev *hdev, u8 status)
 {
 	if (status) {
@@ -2259,6 +2345,7 @@ struct hci_dev *hci_alloc_dev(void)
 	INIT_LIST_HEAD(&hdev->link_keys);
 	INIT_LIST_HEAD(&hdev->long_term_keys);
 	INIT_LIST_HEAD(&hdev->remote_oob_data);
+	INIT_LIST_HEAD(&hdev->conn_param);
 	INIT_LIST_HEAD(&hdev->conn_hash.list);
 
 	INIT_WORK(&hdev->rx_work, hci_rx_work);
@@ -2437,6 +2524,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	hci_link_keys_clear(hdev);
 	hci_smp_ltks_clear(hdev);
 	hci_remote_oob_data_clear(hdev);
+	__clear_conn_param(hdev);
 	hci_dev_unlock(hdev);
 
 	hci_dev_put(hdev);
-- 
1.8.4

[RFC 02/15] Bluetooth: Mgmt command for adding connection parameters

[Andre Guedes]

This patch introduces a new Mgmt command (MGMT_OP_ADD_CONN_PARAM) which
adds the connection parameters to controller's connection parameters
list. These parameters will be used by the kernel when it establishes a
connection with the given device.

This patch also moves the 'auto_connect' enum from hci_core.h to
bluetooth.h since the will accessed used by userspace in order to
send the MGMT_OP_ADD_CONN_PARAM command.

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

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index bf2ddff..8509520 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -354,4 +354,10 @@ void sco_exit(void);
 
 void bt_sock_reclassify_lock(struct sock *sk, int proto);
 
+enum {
+	BT_AUTO_CONN_DISABLED,
+	BT_AUTO_CONN_ALWAYS,
+	BT_AUTO_CONN_LINK_LOSS,
+};
+
 #endif /* __BLUETOOTH_H */
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5052bf0..98be273 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -376,11 +376,7 @@ struct hci_conn_param {
 	bdaddr_t addr;
 	u8 addr_type;
 
-	enum {
-		HCI_AUTO_CONN_DISABLED,
-		HCI_AUTO_CONN_ALWAYS,
-		HCI_AUTO_CONN_LINK_LOSS,
-	} auto_connect;
+	u8 auto_connect;
 
 	u16 min_conn_interval;
 	u16 max_conn_interval;
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 518c5c8..ed689b5 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -369,6 +369,15 @@ struct mgmt_cp_set_scan_params {
 } __packed;
 #define MGMT_SET_SCAN_PARAMS_SIZE	4
 
+#define MGMT_OP_ADD_CONN_PARAM		0x002D
+struct mgmt_cp_add_conn_param {
+	struct mgmt_addr_info addr;
+	__u8 auto_connect;
+	__le16 min_conn_interval;
+	__le16 max_conn_interval;
+} __packed;
+#define MGMT_ADD_CONN_PARAM_SIZE	(MGMT_ADDR_INFO_SIZE + 5)
+
 #define MGMT_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
 	__le16	opcode;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a727b47..5d1a2e8 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -79,6 +79,7 @@ static const u16 mgmt_commands[] = {
 	MGMT_OP_SET_BREDR,
 	MGMT_OP_SET_STATIC_ADDRESS,
 	MGMT_OP_SET_SCAN_PARAMS,
+	MGMT_OP_ADD_CONN_PARAM,
 };
 
 static const u16 mgmt_events[] = {
@@ -4025,6 +4026,39 @@ static int load_long_term_keys(struct sock *sk, struct hci_dev *hdev,
 	return err;
 }
 
+static int add_conn_param(struct sock *sk, struct hci_dev *hdev, void *cp_data,
+			  u16 len)
+{
+	struct mgmt_cp_add_conn_param *cp = cp_data;
+	u8 status;
+	u8 addr_type;
+
+	if (cp->addr.type == BDADDR_BREDR)
+		return cmd_complete(sk, hdev->id, MGMT_OP_ADD_CONN_PARAM,
+				    MGMT_STATUS_NOT_SUPPORTED, NULL, 0);
+
+	status = mgmt_le_support(hdev);
+	if (status)
+		return cmd_complete(sk, hdev->id, MGMT_OP_ADD_CONN_PARAM,
+				    status, NULL, 0);
+
+	if (cp->addr.type == BDADDR_LE_PUBLIC)
+		addr_type = ADDR_LE_DEV_PUBLIC;
+	else
+		addr_type = ADDR_LE_DEV_RANDOM;
+
+	if (hci_add_conn_param(hdev, &cp->addr.bdaddr, addr_type,
+			       cp->auto_connect,
+			       __le16_to_cpu(cp->min_conn_interval),
+			       __le16_to_cpu(cp->max_conn_interval)))
+		status = MGMT_STATUS_FAILED;
+	else
+		status = MGMT_STATUS_SUCCESS;
+
+	return cmd_complete(sk, hdev->id, MGMT_OP_ADD_CONN_PARAM, status,
+			    NULL, 0);
+}
+
 static const struct mgmt_handler {
 	int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
 		     u16 data_len);
@@ -4076,6 +4110,7 @@ static const struct mgmt_handler {
 	{ set_bredr,              false, MGMT_SETTING_SIZE },
 	{ set_static_address,     false, MGMT_SET_STATIC_ADDRESS_SIZE },
 	{ set_scan_params,        false, MGMT_SET_SCAN_PARAMS_SIZE },
+	{ add_conn_param,         false, MGMT_ADD_CONN_PARAM_SIZE },
 };
 
 
-- 
1.8.4

Re: [RFC 02/15] Bluetooth: Mgmt command for adding connection parameters

[Marcel Holtmann]

Hi Andre,

> This patch introduces a new Mgmt command (MGMT_OP_ADD_CONN_PARAM) which
> adds the connection parameters to controller's connection parameters
> list. These parameters will be used by the kernel when it establishes a
> connection with the given device.
> 
> This patch also moves the 'auto_connect' enum from hci_core.h to
> bluetooth.h since the will accessed used by userspace in order to
> send the MGMT_OP_ADD_CONN_PARAM command.

the comment about userspace makes no sense. And is not an argument to move the enum anywhere. Also if these are mgmt "modes" or "types" then they should be somewhere else and not in bluetooth.h.

> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/bluetooth.h |  6 ++++++
> include/net/bluetooth/hci_core.h  |  6 +-----
> include/net/bluetooth/mgmt.h      |  9 +++++++++
> net/bluetooth/mgmt.c              | 35 +++++++++++++++++++++++++++++++++++
> 4 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index bf2ddff..8509520 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -354,4 +354,10 @@ void sco_exit(void);
> 
> void bt_sock_reclassify_lock(struct sock *sk, int proto);
> 
> +enum {
> +	BT_AUTO_CONN_DISABLED,
> +	BT_AUTO_CONN_ALWAYS,
> +	BT_AUTO_CONN_LINK_LOSS,
> +};
> +
> #endif /* __BLUETOOTH_H */
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 5052bf0..98be273 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -376,11 +376,7 @@ struct hci_conn_param {
> 	bdaddr_t addr;
> 	u8 addr_type;
> 
> -	enum {
> -		HCI_AUTO_CONN_DISABLED,
> -		HCI_AUTO_CONN_ALWAYS,
> -		HCI_AUTO_CONN_LINK_LOSS,
> -	} auto_connect;
> +	u8 auto_connect;
> 
> 	u16 min_conn_interval;
> 	u16 max_conn_interval;
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 518c5c8..ed689b5 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -369,6 +369,15 @@ struct mgmt_cp_set_scan_params {
> } __packed;
> #define MGMT_SET_SCAN_PARAMS_SIZE	4
> 
> +#define MGMT_OP_ADD_CONN_PARAM		0x002D
> +struct mgmt_cp_add_conn_param {
> +	struct mgmt_addr_info addr;
> +	__u8 auto_connect;
> +	__le16 min_conn_interval;
> +	__le16 max_conn_interval;
> +} __packed;
> +#define MGMT_ADD_CONN_PARAM_SIZE	(MGMT_ADDR_INFO_SIZE + 5)
> +
> #define MGMT_EV_CMD_COMPLETE		0x0001
> struct mgmt_ev_cmd_complete {
> 	__le16	opcode;
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index a727b47..5d1a2e8 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -79,6 +79,7 @@ static const u16 mgmt_commands[] = {
> 	MGMT_OP_SET_BREDR,
> 	MGMT_OP_SET_STATIC_ADDRESS,
> 	MGMT_OP_SET_SCAN_PARAMS,
> +	MGMT_OP_ADD_CONN_PARAM,
> };

So my 2nd patch wouldn't have been the mgmt command. I would have put all the cleanup and infrastructure changes in first. And I would have turned the connect() into the first user with a mode of "connect once" and default connection parameters.

This clearly shows later on where all this magic handling comes into place. But in the end, the connect() call is just a connect once type of passive scanning. And maybe with learned connection slave interval values from advertising data during the scanning.

The other problem that I have is that we can never update the parameters for a device. We need to remove them and re-add them. That is just not a good idea since we will almost certainly learn about updated values here. And maybe even change the mode of a device.

In general just having one Update Connection Parameters call might be better. The kernel can happily just expire values from connection once or disabled by itself. So this needs a bit more discussion. Doing the mgmt interface last would allow to get all the other patches merged. Otherwise you are stuck on the mgmt part until we figure that out.

> static const u16 mgmt_events[] = {
> @@ -4025,6 +4026,39 @@ static int load_long_term_keys(struct sock *sk, struct hci_dev *hdev,
> 	return err;
> }
> 
> +static int add_conn_param(struct sock *sk, struct hci_dev *hdev, void *cp_data,
> +			  u16 len)
> +{
> +	struct mgmt_cp_add_conn_param *cp = cp_data;
> +	u8 status;
> +	u8 addr_type;
> +
> +	if (cp->addr.type == BDADDR_BREDR)
> +		return cmd_complete(sk, hdev->id, MGMT_OP_ADD_CONN_PARAM,
> +				    MGMT_STATUS_NOT_SUPPORTED, NULL, 0);

Instead of checking for BDADDR_BREDR it would be better to check for !bdaddr_type_is_le().

I really want everybody to get into the habit to do proper input validation. Not the half backed thing. That is why mgmt-tester can catch these kind of things easily and repeatedly.

> +
> +	status = mgmt_le_support(hdev);
> +	if (status)
> +		return cmd_complete(sk, hdev->id, MGMT_OP_ADD_CONN_PARAM,
> +				    status, NULL, 0);
> +
> +	if (cp->addr.type == BDADDR_LE_PUBLIC)
> +		addr_type = ADDR_LE_DEV_PUBLIC;
> +	else
> +		addr_type = ADDR_LE_DEV_RANDOM;
> +
> +	if (hci_add_conn_param(hdev, &cp->addr.bdaddr, addr_type,
> +			       cp->auto_connect,
> +			       __le16_to_cpu(cp->min_conn_interval),
> +			       __le16_to_cpu(cp->max_conn_interval)))
> +		status = MGMT_STATUS_FAILED;
> +	else
> +		status = MGMT_STATUS_SUCCESS;
> +
> +	return cmd_complete(sk, hdev->id, MGMT_OP_ADD_CONN_PARAM, status,
> +			    NULL, 0);
> +}
> +
> static const struct mgmt_handler {
> 	int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
> 		     u16 data_len);
> @@ -4076,6 +4110,7 @@ static const struct mgmt_handler {
> 	{ set_bredr,              false, MGMT_SETTING_SIZE },
> 	{ set_static_address,     false, MGMT_SET_STATIC_ADDRESS_SIZE },
> 	{ set_scan_params,        false, MGMT_SET_SCAN_PARAMS_SIZE },
> +	{ add_conn_param,         false, MGMT_ADD_CONN_PARAM_SIZE },
> };
> 

And btw. it is always plural "params". It is not single parameter.

Regards

Marcel

Re: [RFC 02/15] Bluetooth: Mgmt command for adding connection parameters

[Andre Guedes]

Hi Marcel,

On Oct 17, 2013, at 6:22 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch introduces a new Mgmt command (MGMT_OP_ADD_CONN_PARAM) =
which
>> adds the connection parameters to controller's connection parameters
>> list. These parameters will be used by the kernel when it establishes =
a
>> connection with the given device.
>>=20
>> This patch also moves the 'auto_connect' enum from hci_core.h to
>> bluetooth.h since the will accessed used by userspace in order to
>> send the MGMT_OP_ADD_CONN_PARAM command.
>=20
> the comment about userspace makes no sense. And is not an argument to =
move the enum anywhere. Also if these are mgmt "modes" or "types" then =
they should be somewhere else and not in bluetooth.h.

Ok, so for now I'll leave these macros in hci_core.h.

>=20
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/bluetooth.h |  6 ++++++
>> include/net/bluetooth/hci_core.h  |  6 +-----
>> include/net/bluetooth/mgmt.h      |  9 +++++++++
>> net/bluetooth/mgmt.c              | 35 =
+++++++++++++++++++++++++++++++++++
>> 4 files changed, 51 insertions(+), 5 deletions(-)
>>=20
>> diff --git a/include/net/bluetooth/bluetooth.h =
b/include/net/bluetooth/bluetooth.h
>> index bf2ddff..8509520 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -354,4 +354,10 @@ void sco_exit(void);
>>=20
>> void bt_sock_reclassify_lock(struct sock *sk, int proto);
>>=20
>> +enum {
>> +	BT_AUTO_CONN_DISABLED,
>> +	BT_AUTO_CONN_ALWAYS,
>> +	BT_AUTO_CONN_LINK_LOSS,
>> +};
>> +
>> #endif /* __BLUETOOTH_H */
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index 5052bf0..98be273 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -376,11 +376,7 @@ struct hci_conn_param {
>> 	bdaddr_t addr;
>> 	u8 addr_type;
>>=20
>> -	enum {
>> -		HCI_AUTO_CONN_DISABLED,
>> -		HCI_AUTO_CONN_ALWAYS,
>> -		HCI_AUTO_CONN_LINK_LOSS,
>> -	} auto_connect;
>> +	u8 auto_connect;
>>=20
>> 	u16 min_conn_interval;
>> 	u16 max_conn_interval;
>> diff --git a/include/net/bluetooth/mgmt.h =
b/include/net/bluetooth/mgmt.h
>> index 518c5c8..ed689b5 100644
>> --- a/include/net/bluetooth/mgmt.h
>> +++ b/include/net/bluetooth/mgmt.h
>> @@ -369,6 +369,15 @@ struct mgmt_cp_set_scan_params {
>> } __packed;
>> #define MGMT_SET_SCAN_PARAMS_SIZE	4
>>=20
>> +#define MGMT_OP_ADD_CONN_PARAM		0x002D
>> +struct mgmt_cp_add_conn_param {
>> +	struct mgmt_addr_info addr;
>> +	__u8 auto_connect;
>> +	__le16 min_conn_interval;
>> +	__le16 max_conn_interval;
>> +} __packed;
>> +#define MGMT_ADD_CONN_PARAM_SIZE	(MGMT_ADDR_INFO_SIZE + 5)
>> +
>> #define MGMT_EV_CMD_COMPLETE		0x0001
>> struct mgmt_ev_cmd_complete {
>> 	__le16	opcode;
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index a727b47..5d1a2e8 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -79,6 +79,7 @@ static const u16 mgmt_commands[] =3D {
>> 	MGMT_OP_SET_BREDR,
>> 	MGMT_OP_SET_STATIC_ADDRESS,
>> 	MGMT_OP_SET_SCAN_PARAMS,
>> +	MGMT_OP_ADD_CONN_PARAM,
>> };
>=20
> So my 2nd patch wouldn't have been the mgmt command. I would have put =
all the cleanup and infrastructure changes in first. And I would have =
turned the connect() into the first user with a mode of "connect once" =
and default connection parameters.
>=20
> This clearly shows later on where all this magic handling comes into =
place. But in the end, the connect() call is just a connect once type of =
passive scanning. And maybe with learned connection slave interval =
values from advertising data during the scanning.

In v2, I'll put this patch after the infrastructure changes.

I didn't know we want to change connect() behavior. What is the issue we =
are trying address with this changing?

>=20
> The other problem that I have is that we can never update the =
parameters for a device. We need to remove them and re-add them. That is =
just not a good idea since we will almost certainly learn about updated =
values here. And maybe even change the mode of a device.

These connection parameters and auto connection policy is pretty much =
static values. We know ahead configuring the kernel what profiles the =
device supports. And the profiles the device supports don't dynamically =
change.

>=20
> In general just having one Update Connection Parameters call might be =
better. The kernel can happily just expire values from connection once =
or disabled by itself. So this needs a bit more discussion. Doing the =
mgmt interface last would allow to get all the other patches merged. =
Otherwise you are stuck on the mgmt part until we figure that out.

Sure we need more discussion on this. As I said, I'll move this mgmt =
command patches to the end of this patch set.

>=20
>> static const u16 mgmt_events[] =3D {
>> @@ -4025,6 +4026,39 @@ static int load_long_term_keys(struct sock =
*sk, struct hci_dev *hdev,
>> 	return err;
>> }
>>=20
>> +static int add_conn_param(struct sock *sk, struct hci_dev *hdev, =
void *cp_data,
>> +			  u16 len)
>> +{
>> +	struct mgmt_cp_add_conn_param *cp =3D cp_data;
>> +	u8 status;
>> +	u8 addr_type;
>> +
>> +	if (cp->addr.type =3D=3D BDADDR_BREDR)
>> +		return cmd_complete(sk, hdev->id, =
MGMT_OP_ADD_CONN_PARAM,
>> +				    MGMT_STATUS_NOT_SUPPORTED, NULL, 0);
>=20
> Instead of checking for BDADDR_BREDR it would be better to check for =
!bdaddr_type_is_le().
>=20
> I really want everybody to get into the habit to do proper input =
validation. Not the half backed thing. That is why mgmt-tester can catch =
these kind of things easily and repeatedly.

I'll replace this with !bdaddr_type_is_le().

>=20
>> +
>> +	status =3D mgmt_le_support(hdev);
>> +	if (status)
>> +		return cmd_complete(sk, hdev->id, =
MGMT_OP_ADD_CONN_PARAM,
>> +				    status, NULL, 0);
>> +
>> +	if (cp->addr.type =3D=3D BDADDR_LE_PUBLIC)
>> +		addr_type =3D ADDR_LE_DEV_PUBLIC;
>> +	else
>> +		addr_type =3D ADDR_LE_DEV_RANDOM;
>> +
>> +	if (hci_add_conn_param(hdev, &cp->addr.bdaddr, addr_type,
>> +			       cp->auto_connect,
>> +			       __le16_to_cpu(cp->min_conn_interval),
>> +			       __le16_to_cpu(cp->max_conn_interval)))
>> +		status =3D MGMT_STATUS_FAILED;
>> +	else
>> +		status =3D MGMT_STATUS_SUCCESS;
>> +
>> +	return cmd_complete(sk, hdev->id, MGMT_OP_ADD_CONN_PARAM, =
status,
>> +			    NULL, 0);
>> +}
>> +
>> static const struct mgmt_handler {
>> 	int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
>> 		     u16 data_len);
>> @@ -4076,6 +4110,7 @@ static const struct mgmt_handler {
>> 	{ set_bredr,              false, MGMT_SETTING_SIZE },
>> 	{ set_static_address,     false, MGMT_SET_STATIC_ADDRESS_SIZE },
>> 	{ set_scan_params,        false, MGMT_SET_SCAN_PARAMS_SIZE },
>> +	{ add_conn_param,         false, MGMT_ADD_CONN_PARAM_SIZE },
>> };
>>=20
>=20
> And btw. it is always plural "params". It is not single parameter.

I'll fix it.

Regards,

Andre

[RFC 03/15] Bluetooth: Mgmt command for removing connection parameters

[Andre Guedes]

This patch introduces the MGMT_OP_REMOVE_CONN_PARAM which removes from
the controller's list the connection parameters from a given device.

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

diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index ed689b5..a79100a 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -378,6 +378,12 @@ struct mgmt_cp_add_conn_param {
 } __packed;
 #define MGMT_ADD_CONN_PARAM_SIZE	(MGMT_ADDR_INFO_SIZE + 5)
 
+#define MGMT_OP_REMOVE_CONN_PARAM	0x002E
+struct mgmt_cp_remove_conn_param {
+	struct mgmt_addr_info addr;
+} __packed;
+#define MGMT_REMOVE_CONN_PARAM_SIZE	MGMT_ADDR_INFO_SIZE
+
 #define MGMT_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
 	__le16	opcode;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5d1a2e8..c68aea5 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -80,6 +80,7 @@ static const u16 mgmt_commands[] = {
 	MGMT_OP_SET_STATIC_ADDRESS,
 	MGMT_OP_SET_SCAN_PARAMS,
 	MGMT_OP_ADD_CONN_PARAM,
+	MGMT_OP_REMOVE_CONN_PARAM,
 };
 
 static const u16 mgmt_events[] = {
@@ -4059,6 +4060,33 @@ static int add_conn_param(struct sock *sk, struct hci_dev *hdev, void *cp_data,
 			    NULL, 0);
 }
 
+static int remove_conn_param(struct sock *sk, struct hci_dev *hdev,
+			     void *cp_data, u16 len)
+{
+	struct mgmt_cp_remove_conn_param *cp = cp_data;
+	u8 status;
+	u8 addr_type;
+
+	if (cp->addr.type == BDADDR_BREDR)
+		return cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_CONN_PARAM,
+				    MGMT_STATUS_NOT_SUPPORTED, NULL, 0);
+
+	status = mgmt_le_support(hdev);
+	if (status)
+		return cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_CONN_PARAM,
+				    status, NULL, 0);
+
+	if (cp->addr.type == BDADDR_LE_PUBLIC)
+		addr_type = ADDR_LE_DEV_PUBLIC;
+	else
+		addr_type = ADDR_LE_DEV_RANDOM;
+
+	hci_remove_conn_param(hdev, &cp->addr.bdaddr, addr_type);
+
+	return cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_CONN_PARAM,
+			    MGMT_STATUS_SUCCESS, NULL, 0);
+}
+
 static const struct mgmt_handler {
 	int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
 		     u16 data_len);
@@ -4111,6 +4139,7 @@ static const struct mgmt_handler {
 	{ set_static_address,     false, MGMT_SET_STATIC_ADDRESS_SIZE },
 	{ set_scan_params,        false, MGMT_SET_SCAN_PARAMS_SIZE },
 	{ add_conn_param,         false, MGMT_ADD_CONN_PARAM_SIZE },
+	{ remove_conn_param,      false, MGMT_REMOVE_CONN_PARAM_SIZE },
 };
 
 
-- 
1.8.4

[RFC 04/15] Bluetooth: Make find_conn_param() helper non-local

[Andre Guedes]

This patch makes the find_conn_param() helper non-local by adding the
hci_ prefix and declaring it in hci_core.h. This helper will be used
in hci_conn.c to get the connection parameters when establishing
connections.

Since hci_find_conn_param() returns a reference to the hci_conn_param
object, it was added a refcount to hci_conn_param to control its
lifetime. This way, we avoid bugs such as one thread deletes hci_conn_
param (e.g. thread running MGMT_OP_REMOVE_CONN_PARAM command) while
another thread holds a reference to that object (e.g thread carrying
out the connection establishment).	.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 98be273..1e67da5 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -371,6 +371,8 @@ struct hci_chan {
 };
 
 struct hci_conn_param {
+	struct kref refcount;
+
 	struct list_head list;
 
 	bdaddr_t addr;
@@ -751,6 +753,9 @@ int hci_blacklist_clear(struct hci_dev *hdev);
 int hci_blacklist_add(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type);
 int hci_blacklist_del(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type);
 
+struct hci_conn_param *hci_find_conn_param(struct hci_dev *hdev,
+					   bdaddr_t *addr, u8 addr_type);
+void hci_conn_param_put(struct hci_conn_param *param);
 int hci_add_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
 		       u8 auto_connect, u16 min_conn_interval,
 		       u16 max_conn_interval);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index a4242ac..c9c3390 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2151,8 +2151,34 @@ int hci_blacklist_del(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type)
 	return mgmt_device_unblocked(hdev, bdaddr, type);
 }
 
-struct hci_conn_param *find_conn_param(struct hci_dev *hdev,
-				       bdaddr_t *addr, u8 addr_type)
+static void hci_conn_param_get(struct hci_conn_param *param)
+{
+	kref_get(&param->refcount);
+}
+
+static void release_hci_conn_param(struct kref *kref)
+{
+	struct hci_conn_param *param = container_of(kref,
+						    struct hci_conn_param,
+						    refcount);
+
+	kfree(param);
+}
+
+void hci_conn_param_put(struct hci_conn_param *param)
+{
+	kref_put(&param->refcount, release_hci_conn_param);
+}
+
+/*
+ * Lookup hci_conn_param in hdev->conn_param list.
+ *
+ * Return a reference to hci_conn_param object with refcount incremented.
+ * The caller should drop its reference by using hci_conn_param_put(). If
+ * hci_conn_param is not found, NULL is returned.
+ */
+struct hci_conn_param *hci_find_conn_param(struct hci_dev *hdev,
+					   bdaddr_t *addr, u8 addr_type)
 {
 	struct hci_conn_param *param;
 
@@ -2164,6 +2190,8 @@ struct hci_conn_param *find_conn_param(struct hci_dev *hdev,
 		if (param->addr_type != addr_type)
 			continue;
 
+		hci_conn_param_get(param);
+
 		rcu_read_unlock();
 		return param;
 	}
@@ -2178,14 +2206,18 @@ int hci_add_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
 {
 	struct hci_conn_param *param;
 
-	param = find_conn_param(hdev, addr, addr_type);
-	if (param)
+	param = hci_find_conn_param(hdev, addr, addr_type);
+	if (param) {
+		hci_conn_param_put(param);
 		return -EEXIST;
+	}
 
 	param = kmalloc(sizeof(*param), GFP_KERNEL);
 	if (!param)
 		return -ENOMEM;
 
+	kref_init(&param->refcount);
+
 	bacpy(&param->addr, addr);
 	param->addr_type = addr_type;
 	param->auto_connect = auto_connect;
@@ -2208,20 +2240,22 @@ static void __remove_conn_param(struct hci_conn_param *param)
 	list_del_rcu(&param->list);
 	synchronize_rcu();
 
-	kfree(param);
+	hci_conn_param_put(param);
 }
 
 void hci_remove_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
 {
 	struct hci_conn_param *param;
 
-	param = find_conn_param(hdev, addr, addr_type);
+	param = hci_find_conn_param(hdev, addr, addr_type);
 	if (!param)
 		return;
 
 	hci_dev_lock(hdev);
 	__remove_conn_param(param);
 	hci_dev_unlock(hdev);
+
+	hci_conn_param_put(param);
 }
 
 /*
-- 
1.8.4

[RFC 05/15] Bluetooth: Use connection parameters if any

[Andre Guedes]

This patch changes hci_create_le_conn() so it uses the connection
parameters specified by the user. If no parameters were configured,
we use the default values.

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

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 4e72650..d64000e 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -548,6 +548,7 @@ static int hci_create_le_conn(struct hci_conn *conn)
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_le_create_conn cp;
 	struct hci_request req;
+	struct hci_conn_param *param;
 	int err;
 
 	hci_req_init(&req, hdev);
@@ -558,11 +559,18 @@ static int hci_create_le_conn(struct hci_conn *conn)
 	bacpy(&cp.peer_addr, &conn->dst);
 	cp.peer_addr_type = conn->dst_type;
 	cp.own_address_type = conn->src_type;
-	cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
-	cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
 	cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
 	cp.min_ce_len = __constant_cpu_to_le16(0x0000);
 	cp.max_ce_len = __constant_cpu_to_le16(0x0000);
+	param = hci_find_conn_param(hdev, &conn->dst, conn->dst_type);
+	if (param) {
+		cp.conn_interval_min = cpu_to_le16(param->min_conn_interval);
+		cp.conn_interval_max = cpu_to_le16(param->max_conn_interval);
+		hci_conn_param_put(param);
+	} else {
+		cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
+		cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
+	}
 	hci_req_add(&req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
 
 	err = hci_req_run(&req, create_le_conn_complete);
-- 
1.8.4

Re: [RFC 05/15] Bluetooth: Use connection parameters if any

[Marcel Holtmann]

Hi Andre,

> This patch changes hci_create_le_conn() so it uses the connection
> parameters specified by the user. If no parameters were configured,
> we use the default values.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 4e72650..d64000e 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -548,6 +548,7 @@ static int hci_create_le_conn(struct hci_conn *conn)
> 	struct hci_dev *hdev = conn->hdev;
> 	struct hci_cp_le_create_conn cp;
> 	struct hci_request req;
> +	struct hci_conn_param *param;
> 	int err;
> 
> 	hci_req_init(&req, hdev);
> @@ -558,11 +559,18 @@ static int hci_create_le_conn(struct hci_conn *conn)
> 	bacpy(&cp.peer_addr, &conn->dst);
> 	cp.peer_addr_type = conn->dst_type;
> 	cp.own_address_type = conn->src_type;
> -	cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
> -	cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
> 	cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
> 	cp.min_ce_len = __constant_cpu_to_le16(0x0000);
> 	cp.max_ce_len = __constant_cpu_to_le16(0x0000);
> +	param = hci_find_conn_param(hdev, &conn->dst, conn->dst_type);
> +	if (param) {
> +		cp.conn_interval_min = cpu_to_le16(param->min_conn_interval);
> +		cp.conn_interval_max = cpu_to_le16(param->max_conn_interval);
> +		hci_conn_param_put(param);
> +	} else {
> +		cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
> +		cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
> +	}
> 	hci_req_add(&req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);

so this is part that I do not like at all. We already have the hci_conn connection object at this point. So why are these values not stored in there. In the end we are paying the price for code like this where we have to check if parameters exists, if they do apply them, if not use the defaults.

I did change the code back from the check for public address and what own address type to use. Since it turned out that later on you actually need to what you where doing.

And this is the same thing in the future. We actually want to know what connection parameters we current used. In case we have to update them or they change while the connection is on-going.

Regards

Marcel

Re: [RFC 05/15] Bluetooth: Use connection parameters if any

[Andre Guedes]

Hi Marcel,

On Oct 17, 2013, at 6:27 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch changes hci_create_le_conn() so it uses the connection
>> parameters specified by the user. If no parameters were configured,
>> we use the default values.
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_conn.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>=20
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index 4e72650..d64000e 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -548,6 +548,7 @@ static int hci_create_le_conn(struct hci_conn =
*conn)
>> 	struct hci_dev *hdev =3D conn->hdev;
>> 	struct hci_cp_le_create_conn cp;
>> 	struct hci_request req;
>> +	struct hci_conn_param *param;
>> 	int err;
>>=20
>> 	hci_req_init(&req, hdev);
>> @@ -558,11 +559,18 @@ static int hci_create_le_conn(struct hci_conn =
*conn)
>> 	bacpy(&cp.peer_addr, &conn->dst);
>> 	cp.peer_addr_type =3D conn->dst_type;
>> 	cp.own_address_type =3D conn->src_type;
>> -	cp.conn_interval_min =3D __constant_cpu_to_le16(0x0028);
>> -	cp.conn_interval_max =3D __constant_cpu_to_le16(0x0038);
>> 	cp.supervision_timeout =3D __constant_cpu_to_le16(0x002a);
>> 	cp.min_ce_len =3D __constant_cpu_to_le16(0x0000);
>> 	cp.max_ce_len =3D __constant_cpu_to_le16(0x0000);
>> +	param =3D hci_find_conn_param(hdev, &conn->dst, conn->dst_type);
>> +	if (param) {
>> +		cp.conn_interval_min =3D =
cpu_to_le16(param->min_conn_interval);
>> +		cp.conn_interval_max =3D =
cpu_to_le16(param->max_conn_interval);
>> +		hci_conn_param_put(param);
>> +	} else {
>> +		cp.conn_interval_min =3D __constant_cpu_to_le16(0x0028);
>> +		cp.conn_interval_max =3D __constant_cpu_to_le16(0x0038);
>> +	}
>> 	hci_req_add(&req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
>=20
> so this is part that I do not like at all. We already have the =
hci_conn connection object at this point. So why are these values not =
stored in there. In the end we are paying the price for code like this =
where we have to check if parameters exists, if they do apply them, if =
not use the defaults.

>=20
> I did change the code back from the check for public address and what =
own address type to use. Since it turned out that later on you actually =
need to what you where doing.
>=20
> And this is the same thing in the future. We actually want to know =
what connection parameters we current used. In case we have to update =
them or they change while the connection is on-going.

Sure, I'll save these parameters in hci_conn object.

Regards,

Andre=

[RFC 06/15] Bluetooth: Background scanning

[Andre Guedes]

This patch adds helpers to trigger and untrigger the background
scanning. As long as the number of triggers are greater than zero,
we keep the background scanning running. Once the number of triggers
reaches zero, it is stopped.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 1e67da5..cb6458a 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -288,6 +288,10 @@ struct hci_dev {
 	__u8			scan_rsp_data[HCI_MAX_AD_LENGTH];
 	__u8			scan_rsp_data_len;
 
+	/* This counter tracks the number of background scanning triggers
+	 */
+	atomic_t		background_scan_cnt;
+
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
 	int (*flush)(struct hci_dev *hdev);
@@ -1191,4 +1195,7 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
 #define SCO_AIRMODE_CVSD       0x0000
 #define SCO_AIRMODE_TRANSP     0x0003
 
+int hci_trigger_background_scan(struct hci_dev *hdev);
+int hci_untrigger_background_scan(struct hci_dev *hdev);
+
 #endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index c9c3390..c3e47e9 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2402,6 +2402,8 @@ struct hci_dev *hci_alloc_dev(void)
 	hci_init_sysfs(hdev);
 	discovery_init(hdev);
 
+	atomic_set(&hdev->background_scan_cnt, 0);
+
 	return hdev;
 }
 EXPORT_SYMBOL(hci_alloc_dev);
@@ -3786,3 +3788,84 @@ static void hci_cmd_work(struct work_struct *work)
 		}
 	}
 }
+
+static void start_background_scan_complete(struct hci_dev *hdev, u8 status)
+{
+	if (status)
+		BT_DBG("HCI request failed to start background scanning: "
+		       "status 0x%2.2x", status);
+}
+
+int hci_trigger_background_scan(struct hci_dev *hdev)
+{
+	struct hci_cp_le_set_scan_param param_cp;
+	struct hci_cp_le_set_scan_enable enable_cp;
+	struct hci_request req;
+	int err;
+
+	BT_DBG("%s", hdev->name);
+
+	/* If we already have triggers, there is no need to send HCI command
+	 * to start the background scanning.
+	 */
+	if (atomic_read(&hdev->background_scan_cnt) > 0)
+		goto done;
+
+	hci_req_init(&req, hdev);
+
+	memset(&param_cp, 0, sizeof(param_cp));
+	param_cp.type = LE_SCAN_PASSIVE;
+	param_cp.interval = cpu_to_le16(hdev->le_scan_interval);
+	param_cp.window = cpu_to_le16(hdev->le_scan_window);
+	hci_req_add(&req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp),
+		    &param_cp);
+
+	memset(&enable_cp, 0, sizeof(enable_cp));
+	enable_cp.enable = LE_SCAN_ENABLE;
+	enable_cp.filter_dup = LE_SCAN_FILTER_DUP_DISABLE;
+	hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
+		    &enable_cp);
+
+	err = hci_req_run(&req, start_background_scan_complete);
+	if (err)
+		return err;
+
+done:
+	atomic_inc(&hdev->background_scan_cnt);
+	return 0;
+}
+
+static void stop_background_scan_complete(struct hci_dev *hdev, u8 status)
+{
+	if (status)
+		BT_DBG("HCI request failed to stop background scanning: "
+		       "status 0x%2.2x", status);
+}
+
+int hci_untrigger_background_scan(struct hci_dev *hdev)
+{
+	struct hci_cp_le_set_scan_enable cp;
+	struct hci_request req;
+	int err;
+
+	/* If we have more triggers, we should keep scanning. */
+	if (atomic_read(&hdev->background_scan_cnt) > 1)
+		goto done;
+
+	if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+		goto done;
+
+	hci_req_init(&req, hdev);
+
+	memset(&cp, 0, sizeof(cp));
+	cp.enable = LE_SCAN_DISABLE;
+	hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+
+	err = hci_req_run(&req, stop_background_scan_complete);
+	if (err)
+		return err;
+
+done:
+	atomic_dec(&hdev->background_scan_cnt);
+	return 0;
+}
-- 
1.8.4

Re: [RFC 06/15] Bluetooth: Background scanning

[Marcel Holtmann]

Hi Andre,

> This patch adds helpers to trigger and untrigger the background
> scanning. As long as the number of triggers are greater than zero,
> we keep the background scanning running. Once the number of triggers
> reaches zero, it is stopped.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h |  7 ++++
> net/bluetooth/hci_core.c         | 83 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 90 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 1e67da5..cb6458a 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -288,6 +288,10 @@ struct hci_dev {
> 	__u8			scan_rsp_data[HCI_MAX_AD_LENGTH];
> 	__u8			scan_rsp_data_len;
> 
> +	/* This counter tracks the number of background scanning triggers
> +	 */
> +	atomic_t		background_scan_cnt;
> +
> 	int (*open)(struct hci_dev *hdev);
> 	int (*close)(struct hci_dev *hdev);
> 	int (*flush)(struct hci_dev *hdev);
> @@ -1191,4 +1195,7 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
> #define SCO_AIRMODE_CVSD       0x0000
> #define SCO_AIRMODE_TRANSP     0x0003
> 
> +int hci_trigger_background_scan(struct hci_dev *hdev);
> +int hci_untrigger_background_scan(struct hci_dev *hdev);
> +

instead of plastering the code with all these places it might be better to create a connection parameter hash that actually knows if it has any connection parameters stored that need require a passive scan now.

So instead of trigger and intriguer, the code that adds or removes the actual data from our hash can be the trigger itself. Since it knows better. And checking after disconnection or other cases becomes a lot simpler since you just check the hash if you should be connecting.

Regards

Marcel

Re: [RFC 06/15] Bluetooth: Background scanning

[Andre Guedes]

Hi Marcel,

On Oct 17, 2013, at 6:31 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch adds helpers to trigger and untrigger the background
>> scanning. As long as the number of triggers are greater than zero,
>> we keep the background scanning running. Once the number of triggers
>> reaches zero, it is stopped.
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h |  7 ++++
>> net/bluetooth/hci_core.c         | 83 =
++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 90 insertions(+)
>>=20
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index 1e67da5..cb6458a 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -288,6 +288,10 @@ struct hci_dev {
>> 	__u8			scan_rsp_data[HCI_MAX_AD_LENGTH];
>> 	__u8			scan_rsp_data_len;
>>=20
>> +	/* This counter tracks the number of background scanning =
triggers
>> +	 */
>> +	atomic_t		background_scan_cnt;
>> +
>> 	int (*open)(struct hci_dev *hdev);
>> 	int (*close)(struct hci_dev *hdev);
>> 	int (*flush)(struct hci_dev *hdev);
>> @@ -1191,4 +1195,7 @@ void hci_le_start_enc(struct hci_conn *conn, =
__le16 ediv, __u8 rand[8],
>> #define SCO_AIRMODE_CVSD       0x0000
>> #define SCO_AIRMODE_TRANSP     0x0003
>>=20
>> +int hci_trigger_background_scan(struct hci_dev *hdev);
>> +int hci_untrigger_background_scan(struct hci_dev *hdev);
>> +
>=20
> instead of plastering the code with all these places it might be =
better to create a connection parameter hash that actually knows if it =
has any connection parameters stored that need require a passive scan =
now.
>=20
> So instead of trigger and intriguer, the code that adds or removes the =
actual data from our hash can be the trigger itself. Since it knows =
better. And checking after disconnection or other cases becomes a lot =
simpler since you just check the hash if you should be connecting.

I'll follow this approach in v2.

Regards,

Andre=

[RFC 07/15] Bluetooth: Refactor hci_disconn_complete_evt

[Andre Guedes]

hci_disconn_complete_evt() logic is more complicated than what it
should be, making it hard to follow and add new features. This patch
does some code refactoring by letting the main flow of the function
in first level of function scope.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c | 62 +++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6c3b193..edb2342 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1781,6 +1781,8 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_disconn_complete *ev = (void *) skb->data;
 	struct hci_conn *conn;
+	u8 type;
+	bool send_mgmt_event = false;
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
 
@@ -1790,44 +1792,46 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	if (!conn)
 		goto unlock;
 
-	if (ev->status == 0)
-		conn->state = BT_CLOSED;
-
 	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags) &&
-	    (conn->type == ACL_LINK || conn->type == LE_LINK)) {
-		if (ev->status) {
+	    (conn->type == ACL_LINK || conn->type == LE_LINK))
+		send_mgmt_event = true;
+
+	if (ev->status) {
+		if (send_mgmt_event)
 			mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
 					       conn->dst_type, ev->status);
-		} else {
-			u8 reason = hci_to_mgmt_reason(ev->reason);
-
-			mgmt_device_disconnected(hdev, &conn->dst, conn->type,
-						 conn->dst_type, reason);
-		}
+		return;
 	}
 
-	if (ev->status == 0) {
-		u8 type = conn->type;
+	conn->state = BT_CLOSED;
 
-		if (type == ACL_LINK && conn->flush_key)
-			hci_remove_link_key(hdev, &conn->dst);
-		hci_proto_disconn_cfm(conn, ev->reason);
-		hci_conn_del(conn);
+	if (send_mgmt_event) {
+		u8 reason = hci_to_mgmt_reason(ev->reason);
 
-		/* Re-enable advertising if necessary, since it might
-		 * have been disabled by the connection. From the
-		 * HCI_LE_Set_Advertise_Enable command description in
-		 * the core specification (v4.0):
-		 * "The Controller shall continue advertising until the Host
-		 * issues an LE_Set_Advertise_Enable command with
-		 * Advertising_Enable set to 0x00 (Advertising is disabled)
-		 * or until a connection is created or until the Advertising
-		 * is timed out due to Directed Advertising."
-		 */
-		if (type == LE_LINK)
-			mgmt_reenable_advertising(hdev);
+		mgmt_device_disconnected(hdev, &conn->dst, conn->type,
+					 conn->dst_type, reason);
 	}
 
+	if (conn->type == ACL_LINK && conn->flush_key)
+		hci_remove_link_key(hdev, &conn->dst);
+
+	type = conn->type;
+	hci_proto_disconn_cfm(conn, ev->reason);
+	hci_conn_del(conn);
+
+	/* Re-enable advertising if necessary, since it might
+	 * have been disabled by the connection. From the
+	 * HCI_LE_Set_Advertise_Enable command description in
+	 * the core specification (v4.0):
+	 * "The Controller shall continue advertising until the Host
+	 * issues an LE_Set_Advertise_Enable command with
+	 * Advertising_Enable set to 0x00 (Advertising is disabled)
+	 * or until a connection is created or until the Advertising
+	 * is timed out due to Directed Advertising."
+	 */
+	if (type == LE_LINK)
+		mgmt_reenable_advertising(hdev);
+
 unlock:
 	hci_dev_unlock(hdev);
 }
-- 
1.8.4

Re: [RFC 07/15] Bluetooth: Refactor hci_disconn_complete_evt

[Marcel Holtmann]

Hi Andrei,

> hci_disconn_complete_evt() logic is more complicated than what it
> should be, making it hard to follow and add new features. This patch
> does some code refactoring by letting the main flow of the function
> in first level of function scope.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_event.c | 62 +++++++++++++++++++++++++----------------------
> 1 file changed, 33 insertions(+), 29 deletions(-)

why this is patch 7/15 is beyond me. These things should go first and with a clear commit message why they are are needed. Since if that is clear and easily to review, then you do not have to re-base them all the time.

> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 6c3b193..edb2342 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1781,6 +1781,8 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> 	struct hci_ev_disconn_complete *ev = (void *) skb->data;
> 	struct hci_conn *conn;
> +	u8 type;
> +	bool send_mgmt_event = false;
> 
> 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
> 
> @@ -1790,44 +1792,46 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 	if (!conn)
> 		goto unlock;
> 
> -	if (ev->status == 0)
> -		conn->state = BT_CLOSED;
> -
> 	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags) &&
> -	    (conn->type == ACL_LINK || conn->type == LE_LINK)) {
> -		if (ev->status) {
> +	    (conn->type == ACL_LINK || conn->type == LE_LINK))
> +		send_mgmt_event = true;
> +
> +	if (ev->status) {
> +		if (send_mgmt_event)
> 			mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
> 					       conn->dst_type, ev->status);
> -		} else {
> -			u8 reason = hci_to_mgmt_reason(ev->reason);
> -
> -			mgmt_device_disconnected(hdev, &conn->dst, conn->type,
> -						 conn->dst_type, reason);
> -		}
> +		return;
> 	}

Unfortunately this diff is hard to read. So you need to explain to me in plain English why we are doing this and how it is the same.

Regards

Marcel

Re: [RFC 07/15] Bluetooth: Refactor hci_disconn_complete_evt

[Andre Guedes]

Hi Marcel,

On Oct 17, 2013, at 6:33 AM, Marcel Holtmann wrote:

> Hi Andrei,
> 
>> hci_disconn_complete_evt() logic is more complicated than what it
>> should be, making it hard to follow and add new features. This patch
>> does some code refactoring by letting the main flow of the function
>> in first level of function scope.
>> 
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_event.c | 62 +++++++++++++++++++++++++----------------------
>> 1 file changed, 33 insertions(+), 29 deletions(-)
> 
> why this is patch 7/15 is beyond me. These things should go first and with a clear commit message why they are are needed. Since if that is clear and easily to review, then you do not have to re-base them all the time.

I'll move this patch to the beginning of the patch set.

> 
>> 
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 6c3b193..edb2342 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -1781,6 +1781,8 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>> {
>> 	struct hci_ev_disconn_complete *ev = (void *) skb->data;
>> 	struct hci_conn *conn;
>> +	u8 type;
>> +	bool send_mgmt_event = false;
>> 
>> 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
>> 
>> @@ -1790,44 +1792,46 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>> 	if (!conn)
>> 		goto unlock;
>> 
>> -	if (ev->status == 0)
>> -		conn->state = BT_CLOSED;
>> -
>> 	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags) &&
>> -	    (conn->type == ACL_LINK || conn->type == LE_LINK)) {
>> -		if (ev->status) {
>> +	    (conn->type == ACL_LINK || conn->type == LE_LINK))
>> +		send_mgmt_event = true;
>> +
>> +	if (ev->status) {
>> +		if (send_mgmt_event)
>> 			mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
>> 					       conn->dst_type, ev->status);
>> -		} else {
>> -			u8 reason = hci_to_mgmt_reason(ev->reason);
>> -
>> -			mgmt_device_disconnected(hdev, &conn->dst, conn->type,
>> -						 conn->dst_type, reason);
>> -		}
>> +		return;
>> 	}
> 
> Unfortunately this diff is hard to read. So you need to explain to me in plain English why we are doing this and how it is the same.

Yes, this diff is not good. I'll be more verbose in the commit message.

Regards,

Andre

Re: [RFC 07/15] Bluetooth: Refactor hci_disconn_complete_evt

[Andre Guedes]

Hi Marcel,

On Oct 17, 2013, at 6:33 AM, Marcel Holtmann wrote:

> Hi Andrei,
>=20
>> hci_disconn_complete_evt() logic is more complicated than what it
>> should be, making it hard to follow and add new features. This patch
>> does some code refactoring by letting the main flow of the function
>> in first level of function scope.
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_event.c | 62 =
+++++++++++++++++++++++++----------------------
>> 1 file changed, 33 insertions(+), 29 deletions(-)
>=20
> why this is patch 7/15 is beyond me. These things should go first and =
with a clear commit message why they are are needed. Since if that is =
clear and easily to review, then you do not have to re-base them all the =
time.

I'll move this patch to the beginning of v2 patch set.

>=20
>>=20
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 6c3b193..edb2342 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -1781,6 +1781,8 @@ static void hci_disconn_complete_evt(struct =
hci_dev *hdev, struct sk_buff *skb)
>> {
>> 	struct hci_ev_disconn_complete *ev =3D (void *) skb->data;
>> 	struct hci_conn *conn;
>> +	u8 type;
>> +	bool send_mgmt_event =3D false;
>>=20
>> 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
>>=20
>> @@ -1790,44 +1792,46 @@ static void hci_disconn_complete_evt(struct =
hci_dev *hdev, struct sk_buff *skb)
>> 	if (!conn)
>> 		goto unlock;
>>=20
>> -	if (ev->status =3D=3D 0)
>> -		conn->state =3D BT_CLOSED;
>> -
>> 	if (test_and_clear_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags) &&
>> -	    (conn->type =3D=3D ACL_LINK || conn->type =3D=3D LE_LINK)) {
>> -		if (ev->status) {
>> +	    (conn->type =3D=3D ACL_LINK || conn->type =3D=3D LE_LINK))
>> +		send_mgmt_event =3D true;
>> +
>> +	if (ev->status) {
>> +		if (send_mgmt_event)
>> 			mgmt_disconnect_failed(hdev, &conn->dst, =
conn->type,
>> 					       conn->dst_type, =
ev->status);
>> -		} else {
>> -			u8 reason =3D hci_to_mgmt_reason(ev->reason);
>> -
>> -			mgmt_device_disconnected(hdev, &conn->dst, =
conn->type,
>> -						 conn->dst_type, =
reason);
>> -		}
>> +		return;
>> 	}
>=20
> Unfortunately this diff is hard to read. So you need to explain to me =
in plain English why we are doing this and how it is the same.

Yeah, diff is not good. I'll be more verbose on next version.

Regards,

Andre=

[RFC 08/15] Bluetooth: Add support for BT_AUTO_CONN_ALWAYS

[Andre Guedes]

This patch adds support for the BT_AUTO_CONN_ALWAYS option which
configures the kernel to autonomously establish a connection to
certain device. This option configures the kernel to always
establish the connection, no matter the reason the connection was
terminated.

This feature is required by some LE profiles such as HID over GATT,
Health Termomether and Blood Pressure. These profiles require the
host autonomously connect to the device as soon as it enters in
connectable mode (start advertising) so the device is able to
delivery a notification or indication.

The auto connection procedure the BT_AUTO_CONN_ALWAYS option implements
is summarized as follows: when a new device configured with BT_AUTO_
CONN_ALWAYS is added to the connection parameter list, the background
scan is triggered. Once the target device is found in range, the kernel
creates a connection to the device. If the connection is established
successfully, the background scan is untriggered. If the target device
disconnects, the background scan is triggered again and the whole auto
connect procedure starts again.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |  4 ++
 net/bluetooth/hci_core.c         | 92 ++++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        | 36 ++++++++++++++++
 3 files changed, 132 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index cb6458a..db39eca 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -383,6 +383,7 @@ struct hci_conn_param {
 	u8 addr_type;
 
 	u8 auto_connect;
+	bool bg_scan_triggered;
 
 	u16 min_conn_interval;
 	u16 max_conn_interval;
@@ -765,6 +766,9 @@ int hci_add_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
 		       u16 max_conn_interval);
 void hci_remove_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
 
+void hci_auto_connect_check(struct hci_dev *hdev, bdaddr_t *addr,
+			    u8 addr_type);
+
 int hci_uuids_clear(struct hci_dev *hdev);
 
 int hci_link_keys_clear(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index c3e47e9..f232965 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2200,6 +2200,24 @@ struct hci_conn_param *hci_find_conn_param(struct hci_dev *hdev,
 	return NULL;
 }
 
+static bool is_connected(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 false;
+
+	if (conn->dst_type != type)
+		return false;
+
+	if (conn->state != BT_CONNECTED)
+		return false;
+
+	return true;
+}
+
 int hci_add_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
 		       u8 auto_connect, u16 min_conn_interval,
 		       u16 max_conn_interval)
@@ -2221,12 +2239,29 @@ int hci_add_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
 	bacpy(&param->addr, addr);
 	param->addr_type = addr_type;
 	param->auto_connect = auto_connect;
+	param->bg_scan_triggered = false;
 	param->min_conn_interval = min_conn_interval;
 	param->max_conn_interval = max_conn_interval;
 
 	hci_dev_lock(hdev);
 	list_add_rcu(&param->list, &hdev->conn_param);
 	hci_dev_unlock(hdev);
+
+	/* If it is configured to always automatically connect and we are not
+	 * connected to the given device, we trigger the background scan so
+	 * the connection is established as soon as the device gets in range.
+	 */
+	if (auto_connect == BT_AUTO_CONN_ALWAYS &&
+	    !is_connected(hdev, addr, addr_type)) {
+		int err;
+
+		err = hci_trigger_background_scan(hdev);
+		if (err)
+			return err;
+
+		param->bg_scan_triggered = true;
+	}
+
 	return 0;
 }
 
@@ -2251,6 +2286,17 @@ void hci_remove_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
 	if (!param)
 		return;
 
+	if (param->bg_scan_triggered) {
+		int err;
+
+		err = hci_untrigger_background_scan(hdev);
+		if (err)
+			BT_ERR("Failed to untrigger background scanning: %d",
+			       err);
+
+		param->bg_scan_triggered = false;
+	}
+
 	hci_dev_lock(hdev);
 	__remove_conn_param(param);
 	hci_dev_unlock(hdev);
@@ -2271,6 +2317,52 @@ static void __clear_conn_param(struct hci_dev *hdev)
 		__remove_conn_param(param);
 }
 
+void hci_auto_connect_check(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
+{
+	struct hci_conn_param *param;
+	struct hci_conn *conn;
+	u8 bdaddr_type;
+
+	/* If there is no background scan trigger, it means the auto connection
+	 * procedure is not runninng.
+	 */
+	if (atomic_read(&hdev->background_scan_cnt) == 0)
+		return;
+
+	param = hci_find_conn_param(hdev, addr, addr_type);
+	if (!param)
+		return;
+
+	if (!param->bg_scan_triggered)
+		goto done;
+
+	if (addr_type == ADDR_LE_DEV_PUBLIC)
+		bdaddr_type = BDADDR_LE_PUBLIC;
+	else
+		bdaddr_type = BDADDR_LE_RANDOM;
+
+	conn = hci_connect(hdev, LE_LINK, addr, bdaddr_type, BT_SECURITY_LOW,
+			   HCI_AT_NO_BONDING);
+	if (IS_ERR(conn)) {
+		switch(PTR_ERR(conn)) {
+		case -EBUSY:
+			/* When hci_connect() returns EBUSY it means there is
+			 * already an LE connection attempt going on. Since the
+			 * controller supports only one connection attempt at
+			 * the time, we simply return.
+			 */
+			goto done;
+		default:
+			BT_ERR("Failed to auto connect: err %ld",
+			       PTR_ERR(conn));
+			goto done;
+		}
+	}
+
+done:
+	hci_conn_param_put(param);
+}
+
 static void inquiry_complete(struct hci_dev *hdev, u8 status)
 {
 	if (status) {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index edb2342..b34ff1d 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1782,6 +1782,7 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	struct hci_ev_disconn_complete *ev = (void *) skb->data;
 	struct hci_conn *conn;
 	u8 type;
+	struct hci_conn_param *param;
 	bool send_mgmt_event = false;
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
@@ -1815,6 +1816,22 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	if (conn->type == ACL_LINK && conn->flush_key)
 		hci_remove_link_key(hdev, &conn->dst);
 
+	/* Trigger the background scan if auto connection is required for this
+	 * device.
+	 */
+	param = hci_find_conn_param(hdev, &conn->dst, conn->dst_type);
+	if (param && param->auto_connect == BT_AUTO_CONN_ALWAYS) {
+		int err;
+
+		err = hci_trigger_background_scan(hdev);
+		if (err)
+			BT_ERR("Failed to trigger background "
+					"scanning: %d", err);
+
+		param->bg_scan_triggered = true;
+		hci_conn_param_put(param);
+	}
+
 	type = conn->type;
 	hci_proto_disconn_cfm(conn, ev->reason);
 	hci_conn_del(conn);
@@ -3493,6 +3510,7 @@ 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 *conn;
+	struct hci_conn_param *param;
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
 
@@ -3546,6 +3564,22 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	hci_proto_connect_cfm(conn, ev->status);
 
+	/* Since the connection has been established successfully, we should
+	 * untrigger the background scan if this is an auto connect device.
+	 */
+	param = hci_find_conn_param(hdev, &ev->bdaddr, ev->bdaddr_type);
+	if (param && param->bg_scan_triggered) {
+		int err;
+
+		err = hci_untrigger_background_scan(hdev);
+		if (err)
+			BT_ERR("Failed to untrigger background "
+					"scanning: %d", err);
+
+		param->bg_scan_triggered = false;
+		hci_conn_param_put(param);
+	}
+
 unlock:
 	hci_dev_unlock(hdev);
 }
@@ -3559,6 +3593,8 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	while (num_reports--) {
 		struct hci_ev_le_advertising_info *ev = ptr;
 
+		hci_auto_connect_check(hdev, &ev->bdaddr, ev->bdaddr_type);
+
 		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.4

Re: [RFC 08/15] Bluetooth: Add support for BT_AUTO_CONN_ALWAYS

[Marcel Holtmann]

Hi Andre,

> This patch adds support for the BT_AUTO_CONN_ALWAYS option which
> configures the kernel to autonomously establish a connection to
> certain device. This option configures the kernel to always
> establish the connection, no matter the reason the connection was
> terminated.
> 
> This feature is required by some LE profiles such as HID over GATT,
> Health Termomether and Blood Pressure. These profiles require the
> host autonomously connect to the device as soon as it enters in
> connectable mode (start advertising) so the device is able to
> delivery a notification or indication.
> 
> The auto connection procedure the BT_AUTO_CONN_ALWAYS option implements
> is summarized as follows: when a new device configured with BT_AUTO_
> CONN_ALWAYS is added to the connection parameter list, the background
> scan is triggered. Once the target device is found in range, the kernel
> creates a connection to the device. If the connection is established
> successfully, the background scan is untriggered. If the target device
> disconnects, the background scan is triggered again and the whole auto
> connect procedure starts again.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h |  4 ++
> net/bluetooth/hci_core.c         | 92 ++++++++++++++++++++++++++++++++++++++++
> net/bluetooth/hci_event.c        | 36 ++++++++++++++++
> 3 files changed, 132 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index cb6458a..db39eca 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -383,6 +383,7 @@ struct hci_conn_param {
> 	u8 addr_type;
> 
> 	u8 auto_connect;
> +	bool bg_scan_triggered;
> 
> 	u16 min_conn_interval;
> 	u16 max_conn_interval;
> @@ -765,6 +766,9 @@ int hci_add_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
> 		       u16 max_conn_interval);
> void hci_remove_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
> 
> +void hci_auto_connect_check(struct hci_dev *hdev, bdaddr_t *addr,
> +			    u8 addr_type);
> +
> int hci_uuids_clear(struct hci_dev *hdev);
> 
> int hci_link_keys_clear(struct hci_dev *hdev);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index c3e47e9..f232965 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2200,6 +2200,24 @@ struct hci_conn_param *hci_find_conn_param(struct hci_dev *hdev,
> 	return NULL;
> }
> 
> +static bool is_connected(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 false;
> +
> +	if (conn->dst_type != type)
> +		return false;
> +
> +	if (conn->state != BT_CONNECTED)
> +		return false;
> +
> +	return true;
> +}
> +
> int hci_add_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
> 		       u8 auto_connect, u16 min_conn_interval,
> 		       u16 max_conn_interval)
> @@ -2221,12 +2239,29 @@ int hci_add_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
> 	bacpy(&param->addr, addr);
> 	param->addr_type = addr_type;
> 	param->auto_connect = auto_connect;
> +	param->bg_scan_triggered = false;
> 	param->min_conn_interval = min_conn_interval;
> 	param->max_conn_interval = max_conn_interval;
> 
> 	hci_dev_lock(hdev);
> 	list_add_rcu(&param->list, &hdev->conn_param);
> 	hci_dev_unlock(hdev);
> +
> +	/* If it is configured to always automatically connect and we are not
> +	 * connected to the given device, we trigger the background scan so
> +	 * the connection is established as soon as the device gets in range.
> +	 */
> +	if (auto_connect == BT_AUTO_CONN_ALWAYS &&
> +	    !is_connected(hdev, addr, addr_type)) {
> +		int err;
> +
> +		err = hci_trigger_background_scan(hdev);
> +		if (err)
> +			return err;
> +
> +		param->bg_scan_triggered = true;
> +	}

and this will get complicated very quickly. You have the counter for number of triggers and now you also store locally if someone triggered.

> +
> 	return 0;
> }
> 
> @@ -2251,6 +2286,17 @@ void hci_remove_conn_param(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
> 	if (!param)
> 		return;
> 
> +	if (param->bg_scan_triggered) {
> +		int err;
> +
> +		err = hci_untrigger_background_scan(hdev);
> +		if (err)
> +			BT_ERR("Failed to untrigger background scanning: %d",
> +			       err);
> +
> +		param->bg_scan_triggered = false;
> +	}
> +
> 	hci_dev_lock(hdev);
> 	__remove_conn_param(param);
> 	hci_dev_unlock(hdev);
> @@ -2271,6 +2317,52 @@ static void __clear_conn_param(struct hci_dev *hdev)
> 		__remove_conn_param(param);
> }
> 
> +void hci_auto_connect_check(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
> +{
> +	struct hci_conn_param *param;
> +	struct hci_conn *conn;
> +	u8 bdaddr_type;
> +
> +	/* If there is no background scan trigger, it means the auto connection
> +	 * procedure is not runninng.
> +	 */
> +	if (atomic_read(&hdev->background_scan_cnt) == 0)
> +		return;
> +
> +	param = hci_find_conn_param(hdev, addr, addr_type);
> +	if (!param)
> +		return;
> +
> +	if (!param->bg_scan_triggered)
> +		goto done;
> +
> +	if (addr_type == ADDR_LE_DEV_PUBLIC)
> +		bdaddr_type = BDADDR_LE_PUBLIC;
> +	else
> +		bdaddr_type = BDADDR_LE_RANDOM;
> +
> +	conn = hci_connect(hdev, LE_LINK, addr, bdaddr_type, BT_SECURITY_LOW,
> +			   HCI_AT_NO_BONDING);
> +	if (IS_ERR(conn)) {
> +		switch(PTR_ERR(conn)) {
> +		case -EBUSY:
> +			/* When hci_connect() returns EBUSY it means there is
> +			 * already an LE connection attempt going on. Since the
> +			 * controller supports only one connection attempt at
> +			 * the time, we simply return.
> +			 */
> +			goto done;
> +		default:
> +			BT_ERR("Failed to auto connect: err %ld",
> +			       PTR_ERR(conn));
> +			goto done;
> +		}
> +	}
> +
> +done:
> +	hci_conn_param_put(param);
> +}
> +
> static void inquiry_complete(struct hci_dev *hdev, u8 status)
> {
> 	if (status) {
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index edb2342..b34ff1d 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1782,6 +1782,7 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 	struct hci_ev_disconn_complete *ev = (void *) skb->data;
> 	struct hci_conn *conn;
> 	u8 type;
> +	struct hci_conn_param *param;
> 	bool send_mgmt_event = false;
> 
> 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
> @@ -1815,6 +1816,22 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 	if (conn->type == ACL_LINK && conn->flush_key)
> 		hci_remove_link_key(hdev, &conn->dst);
> 
> +	/* Trigger the background scan if auto connection is required for this
> +	 * device.
> +	 */
> +	param = hci_find_conn_param(hdev, &conn->dst, conn->dst_type);
> +	if (param && param->auto_connect == BT_AUTO_CONN_ALWAYS) {
> +		int err;
> +
> +		err = hci_trigger_background_scan(hdev);
> +		if (err)
> +			BT_ERR("Failed to trigger background "
> +					"scanning: %d", err);
> +
> +		param->bg_scan_triggered = true;
> +		hci_conn_param_put(param);
> +	}
> +
> 	type = conn->type;
> 	hci_proto_disconn_cfm(conn, ev->reason);
> 	hci_conn_del(conn);
> @@ -3493,6 +3510,7 @@ 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 *conn;
> +	struct hci_conn_param *param;
> 
> 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
> 
> @@ -3546,6 +3564,22 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 
> 	hci_proto_connect_cfm(conn, ev->status);
> 
> +	/* Since the connection has been established successfully, we should
> +	 * untrigger the background scan if this is an auto connect device.
> +	 */
> +	param = hci_find_conn_param(hdev, &ev->bdaddr, ev->bdaddr_type);
> +	if (param && param->bg_scan_triggered) {
> +		int err;
> +
> +		err = hci_untrigger_background_scan(hdev);
> +		if (err)
> +			BT_ERR("Failed to untrigger background "
> +					"scanning: %d", err);
> +
> +		param->bg_scan_triggered = false;
> +		hci_conn_param_put(param);
> +	}
> +

The worst part here is actually that you make this part of the connection parameters, but the value is really only needed during the lifetime of a hci_conn. It is mainly, do we need to keep scanning after this connection has been established aka do we have more devices in the list or not.

If all connections including connnect() would go through the background scan handling then this would not even be needed since everything would be in a central place.

That is why I am saying, first convert connect() into a connect once passive scanning consumer that uses the auto-connection handling.

Regards

Marcel

[RFC 09/15] Bluetooth: Add support for BT_AUTO_CONN_LINK_LOSS option

[Andre Guedes]

This patch adds support for the BT_AUTO_CONN_LINK_LOSS option which
configures the kernel to autonomously reconnect to a certainn device
in case the connection is terminated by link loss (which maps to HCI
Connection Timeout error.

This feature is required by the majority of LE profiles such as
Proximity, Find Me, Cycling Speed and Cadence and Time.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index b34ff1d..570f27d 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1820,15 +1820,25 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	 * device.
 	 */
 	param = hci_find_conn_param(hdev, &conn->dst, conn->dst_type);
-	if (param && param->auto_connect == BT_AUTO_CONN_ALWAYS) {
+	if (param) {
 		int err;
 
-		err = hci_trigger_background_scan(hdev);
-		if (err)
-			BT_ERR("Failed to trigger background "
-					"scanning: %d", err);
+		switch (param->auto_connect){
+		case BT_AUTO_CONN_LINK_LOSS:
+			if (ev->reason != HCI_ERROR_CONNECTION_TIMEOUT)
+				break;
+			/* Fall through */
+
+		case BT_AUTO_CONN_ALWAYS:
+			err = hci_trigger_background_scan(hdev);
+			if (err)
+				BT_ERR("Failed to trigger background "
+						"scanning: %d", err);
+
+			param->bg_scan_triggered = true;
+			break;
+		}
 
-		param->bg_scan_triggered = true;
 		hci_conn_param_put(param);
 	}
 
-- 
1.8.4

[RFC 10/15] Bluetooth: Create start_background_scan helper

[Andre Guedes]

This patch creates the start_background_scan() function so it can
be reused in the next patch.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f232965..68f3c0a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3888,20 +3888,11 @@ static void start_background_scan_complete(struct hci_dev *hdev, u8 status)
 		       "status 0x%2.2x", status);
 }
 
-int hci_trigger_background_scan(struct hci_dev *hdev)
+static int start_background_scan(struct hci_dev *hdev)
 {
 	struct hci_cp_le_set_scan_param param_cp;
 	struct hci_cp_le_set_scan_enable enable_cp;
 	struct hci_request req;
-	int err;
-
-	BT_DBG("%s", hdev->name);
-
-	/* If we already have triggers, there is no need to send HCI command
-	 * to start the background scanning.
-	 */
-	if (atomic_read(&hdev->background_scan_cnt) > 0)
-		goto done;
 
 	hci_req_init(&req, hdev);
 
@@ -3918,7 +3909,22 @@ int hci_trigger_background_scan(struct hci_dev *hdev)
 	hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
 		    &enable_cp);
 
-	err = hci_req_run(&req, start_background_scan_complete);
+	return hci_req_run(&req, start_background_scan_complete);
+}
+
+int hci_trigger_background_scan(struct hci_dev *hdev)
+{
+	int err;
+
+	BT_DBG("%s", hdev->name);
+
+	/* If we already have triggers, there is no need to send HCI command
+	 * to start the background scanning.
+	 */
+	if (atomic_read(&hdev->background_scan_cnt) > 0)
+		goto done;
+
+	err = start_background_scan(hdev);
 	if (err)
 		return err;
 
-- 
1.8.4

[RFC 11/15] Bluetooth: Temporarily stop background scanning on connection

[Andre Guedes]

Some LE controllers don't support scanning and initiating a connection
at the same time. So, for those controllers, we should temporarily
stop the background scanning and start it again once the connection
attempt is finished (successfully or not).

So this patch introduces the hci_check_background_scan() which checks
if the background scanning should be started.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_conn.c         | 25 +++++++++++++++++++++++++
 net/bluetooth/hci_core.c         | 18 ++++++++++++++++++
 net/bluetooth/hci_event.c        |  6 ++++++
 4 files changed, 50 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index db39eca..017decc 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1201,5 +1201,6 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
 
 int hci_trigger_background_scan(struct hci_dev *hdev);
 int hci_untrigger_background_scan(struct hci_dev *hdev);
+void hci_check_background_scan(struct hci_dev *hdev);
 
 #endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index d64000e..6ae42c2 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -541,6 +541,18 @@ static void create_le_conn_complete(struct hci_dev *hdev, u8 status)
 
 done:
 	hci_dev_unlock(hdev);
+
+	/* Check the background scanning since it may have been temporarily
+	 * stopped if the controller doesn't support scanning and initiate
+	 * state combination.
+	 */
+	hci_check_background_scan(hdev);
+}
+
+/* Check if controller supports scanning and initiating states combination */
+static bool is_state_combination_supported(struct hci_dev *hdev)
+{
+        return (hdev->le_states[2] & BIT(6)) ? true : false;
 }
 
 static int hci_create_le_conn(struct hci_conn *conn)
@@ -553,6 +565,19 @@ static int hci_create_le_conn(struct hci_conn *conn)
 
 	hci_req_init(&req, hdev);
 
+	/* If controller is scanning but it doesn't support initiating and
+	 * scanning states combination, we stop scanning.
+	 */
+	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags) &&
+	    !is_state_combination_supported(hdev)) {
+		struct hci_cp_le_set_scan_enable enable_cp;
+
+		memset(&enable_cp, 0, sizeof(enable_cp));
+		enable_cp.enable = LE_SCAN_DISABLE;
+		hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
+			    &enable_cp);
+	}
+
 	memset(&cp, 0, sizeof(cp));
 	cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
 	cp.scan_window = cpu_to_le16(hdev->le_scan_window);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 68f3c0a..e1e8f8a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3967,3 +3967,21 @@ done:
 	atomic_dec(&hdev->background_scan_cnt);
 	return 0;
 }
+
+/* This function checks if there is background scan triggers and starts
+ * scanning.
+ */
+void hci_check_background_scan(struct hci_dev *hdev)
+{
+	int err;
+
+	if (atomic_read(&hdev->background_scan_cnt) == 0)
+		return;
+
+	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+		return;
+
+	err = start_background_scan(hdev);
+	if (err)
+		BT_ERR("Failed to start background scanning: err %d", err);
+}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 570f27d..a8c7b47 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3592,6 +3592,12 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 unlock:
 	hci_dev_unlock(hdev);
+
+	/* Check the background scanning since it may have been temporarily
+	 * stopped if the controller doesn't support scanning and initiate
+	 * state combination.
+	 */
+	hci_check_background_scan(hdev);
 }
 
 static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
-- 
1.8.4

Re: [RFC 11/15] Bluetooth: Temporarily stop background scanning on connection

[Marcel Holtmann]

Hi Andre,

> Some LE controllers don't support scanning and initiating a connection
> at the same time. So, for those controllers, we should temporarily
> stop the background scanning and start it again once the connection
> attempt is finished (successfully or not).
> 
> So this patch introduces the hci_check_background_scan() which checks
> if the background scanning should be started.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h |  1 +
> net/bluetooth/hci_conn.c         | 25 +++++++++++++++++++++++++
> net/bluetooth/hci_core.c         | 18 ++++++++++++++++++
> net/bluetooth/hci_event.c        |  6 ++++++
> 4 files changed, 50 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index db39eca..017decc 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1201,5 +1201,6 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
> 
> int hci_trigger_background_scan(struct hci_dev *hdev);
> int hci_untrigger_background_scan(struct hci_dev *hdev);
> +void hci_check_background_scan(struct hci_dev *hdev);
> 
> #endif /* __HCI_CORE_H */
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index d64000e..6ae42c2 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -541,6 +541,18 @@ static void create_le_conn_complete(struct hci_dev *hdev, u8 status)
> 
> done:
> 	hci_dev_unlock(hdev);
> +
> +	/* Check the background scanning since it may have been temporarily
> +	 * stopped if the controller doesn't support scanning and initiate
> +	 * state combination.
> +	 */
> +	hci_check_background_scan(hdev);
> +}

so what I would do first is assume the controller can not scan while being in initiating state or being connected. Make the dead simple only a single connection at a time work perfectly. Get the basic infrastructure in place.

> +
> +/* Check if controller supports scanning and initiating states combination */
> +static bool is_state_combination_supported(struct hci_dev *hdev)
> +{
> +        return (hdev->le_states[2] & BIT(6)) ? true : false;
> }
> 

Worst function name ever. Also this should use !!(hdev->le_states & x) style.

Also we need this more generic anyway. So get the simple part working for dumb controllers and we deal with the rest when that part works.

Regards

Marcel

Re: [RFC 11/15] Bluetooth: Temporarily stop background scanning on connection

[Andre Guedes]

Hi Marcel,

On Oct 17, 2013, at 6:43 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> Some LE controllers don't support scanning and initiating a =
connection
>> at the same time. So, for those controllers, we should temporarily
>> stop the background scanning and start it again once the connection
>> attempt is finished (successfully or not).
>>=20
>> So this patch introduces the hci_check_background_scan() which checks
>> if the background scanning should be started.
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h |  1 +
>> net/bluetooth/hci_conn.c         | 25 +++++++++++++++++++++++++
>> net/bluetooth/hci_core.c         | 18 ++++++++++++++++++
>> net/bluetooth/hci_event.c        |  6 ++++++
>> 4 files changed, 50 insertions(+)
>>=20
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index db39eca..017decc 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -1201,5 +1201,6 @@ void hci_le_start_enc(struct hci_conn *conn, =
__le16 ediv, __u8 rand[8],
>>=20
>> int hci_trigger_background_scan(struct hci_dev *hdev);
>> int hci_untrigger_background_scan(struct hci_dev *hdev);
>> +void hci_check_background_scan(struct hci_dev *hdev);
>>=20
>> #endif /* __HCI_CORE_H */
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index d64000e..6ae42c2 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -541,6 +541,18 @@ static void create_le_conn_complete(struct =
hci_dev *hdev, u8 status)
>>=20
>> done:
>> 	hci_dev_unlock(hdev);
>> +
>> +	/* Check the background scanning since it may have been =
temporarily
>> +	 * stopped if the controller doesn't support scanning and =
initiate
>> +	 * state combination.
>> +	 */
>> +	hci_check_background_scan(hdev);
>> +}
>=20
> so what I would do first is assume the controller can not scan while =
being in initiating state or being connected. Make the dead simple only =
a single connection at a time work perfectly. Get the basic =
infrastructure in place.

Ok, I'll do like that.

>=20
>> +
>> +/* Check if controller supports scanning and initiating states =
combination */
>> +static bool is_state_combination_supported(struct hci_dev *hdev)
>> +{
>> +        return (hdev->le_states[2] & BIT(6)) ? true : false;
>> }
>>=20
>=20
> Worst function name ever. Also this should use !!(hdev->le_states & x) =
style.

I'll come up with a better name for this helper and use the !! approach.

>=20
> Also we need this more generic anyway. So get the simple part working =
for dumb controllers and we deal with the rest when that part works.

Ok.

Regards,

Andre=

[RFC 12/15] Bluetooth: Temporarily stop background scanning on discovery

[Andre Guedes]

If the user send a mgmt start discovery command while the background
scanning is running, we should temporarily stop it. Once the discovery
finishes, we start the background scanning again.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index e1e8f8a..ed5ce58 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -877,6 +877,11 @@ void hci_discovery_set_state(struct hci_dev *hdev, int state)
 
 	switch (state) {
 	case DISCOVERY_STOPPED:
+		/* Check the background scanning since it may have been
+		 * temporarily stopped by the start discovery command.
+		 */
+		hci_check_background_scan(hdev);
+
 		if (hdev->discovery.state != DISCOVERY_STARTING)
 			mgmt_discovering(hdev, 0);
 		break;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c68aea5..f6f1524 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3228,11 +3228,15 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 			goto failed;
 		}
 
+		/* If controller is scanning, it means the background scanning
+		 * is running. Thus, we should temporarily stop it in order to
+		 * set the discovery scanning parameters.
+		 */
 		if (test_bit(HCI_LE_SCAN, &hdev->dev_flags)) {
-			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
-					 MGMT_STATUS_BUSY);
-			mgmt_pending_remove(cmd);
-			goto failed;
+			memset(&enable_cp, 0, sizeof(enable_cp));
+			enable_cp.enable = LE_SCAN_DISABLE;
+			hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE,
+				    sizeof(enable_cp), &enable_cp);
 		}
 
 		memset(&param_cp, 0, sizeof(param_cp));
-- 
1.8.4

[RFC 13/15] Bluetooth: Fix background trigger/untrigger functions

[Andre Guedes]

This patch fixes to hci_trigger_background_scan() and hci_untrigger_
background_scan() so they can be called concurrently with the device
discovery procedure.

In hci_trigger_background_scan(), if it is the first trigger, we are
supposed to start the background scanning. However, if device discovery
is running, we should not send any HCI command. The background scanning
will be started once the discovery procedure is finished.

In hci_untrigger_background_scan(), if it is the last trigger, we are
supposed to stop the background scanning. However, if discovery is
running, it means the background is stopped so we don't need to send
any HCI command.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ed5ce58..ace77e3 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3929,6 +3929,12 @@ int hci_trigger_background_scan(struct hci_dev *hdev)
 	if (atomic_read(&hdev->background_scan_cnt) > 0)
 		goto done;
 
+	/* If discovery is running, we should not start the background
+	 * scanning. It will be started once the discovery procedure finishes.
+	 */
+	if (hdev->discovery.state == DISCOVERY_FINDING)
+		goto done;
+
 	err = start_background_scan(hdev);
 	if (err)
 		return err;
@@ -3958,6 +3964,10 @@ int hci_untrigger_background_scan(struct hci_dev *hdev)
 	if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
 		goto done;
 
+	/* If device discovery is running, don't stop scanning. */
+	if (hdev->discovery.state == DISCOVERY_FINDING)
+		goto done;
+
 	hci_req_init(&req, hdev);
 
 	memset(&cp, 0, sizeof(cp));
-- 
1.8.4

[RFC 14/15] Bluetooth: Fix hci_create_le_conn()

[Andre Guedes]

When a LE device we want automatically connect to is found during the
device discovery procedure, we create the connection. Since some LE
controllers don't support scanning and initiating state combination,
the scanning will be disabled therefore we should set the discovery
state to DISCOVERY_STOPPED

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

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 6ae42c2..5caf13b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -576,6 +576,11 @@ static int hci_create_le_conn(struct hci_conn *conn)
 		enable_cp.enable = LE_SCAN_DISABLE;
 		hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
 			    &enable_cp);
+
+		if (hdev->discovery.state == DISCOVERY_FINDING) {
+			cancel_delayed_work(&hdev->le_scan_disable);
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+		}
 	}
 
 	memset(&cp, 0, sizeof(cp));
-- 
1.8.4

[RFC 15/15] Bluetooth: Auto connection and power off/on

[Andre Guedes]

If hdev is closed (e.g. Mgmt power off command, RFKILL or controller is
reset), the established connections are dropped and no Disconnection
Complete Event is sent to host. This way, the background scan is not
triggered when devices configured with BT_AUTO_CONN_ALWAYS option
disconnect. To fix this issue, before dropping the LE connections, we
trigger the background scan for each connected device that requires
BT_AUTO_CONN_ALWAYS auto connection.

Moreover, once the adapter is powered on, we should start the background
scan if we have triggers registered. This way, we keep the background
scan running after a power off and power on sequence.

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

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 5caf13b..66823eb 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -954,6 +954,31 @@ timer:
 				   msecs_to_jiffies(hdev->idle_timeout));
 }
 
+static void le_conn_drop_fixup(struct hci_conn *conn)
+{
+	struct hci_dev *hdev = conn->hdev;
+	struct hci_conn_param *param;
+	int err;
+
+	param = hci_find_conn_param(hdev, &conn->dst, conn->dst_type);
+	if (!param)
+		return;
+
+	if (param->auto_connect != BT_AUTO_CONN_ALWAYS)
+		goto done;
+
+	err = hci_trigger_background_scan(hdev);
+	if (err) {
+		BT_ERR("Failed to trigger background scanning: %d", err);
+		goto done;
+	}
+
+	param->bg_scan_triggered = true;
+
+done:
+	hci_conn_param_put(param);
+}
+
 /* Drop all connection on the device */
 void hci_conn_hash_flush(struct hci_dev *hdev)
 {
@@ -963,6 +988,16 @@ void hci_conn_hash_flush(struct hci_dev *hdev)
 	BT_DBG("hdev %s", hdev->name);
 
 	list_for_each_entry_safe(c, n, &h->list, list) {
+		/* If this is a LE connection in connected state we should do
+		 * some fixup before dropping this connection. Since no
+		 * Disconnection Complete Event will be sent to the host, we
+		 * have to trigger the background scan in case this is a
+		 * BT_AUTO_CONN_ALWAYS device. This is handled by the le_conn_
+		 * drop_fixup() helper.
+		 */
+		if (c->type == LE_LINK && c->state == BT_CONNECTED)
+			le_conn_drop_fixup(c);
+
 		c->state = BT_CLOSED;
 
 		hci_proto_disconn_cfm(c, HCI_ERROR_LOCAL_HOST_TERM);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ace77e3..fe18801 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1748,6 +1748,8 @@ static void hci_power_on(struct work_struct *work)
 
 	if (test_and_clear_bit(HCI_SETUP, &hdev->dev_flags))
 		mgmt_index_added(hdev);
+
+	hci_check_background_scan(hdev);
 }
 
 static void hci_power_off(struct work_struct *work)
-- 
1.8.4

Re: [RFC 15/15] Bluetooth: Auto connection and power off/on

[Marcel Holtmann]

Hi Andre,

> If hdev is closed (e.g. Mgmt power off command, RFKILL or controller is
> reset), the established connections are dropped and no Disconnection
> Complete Event is sent to host. This way, the background scan is not
> triggered when devices configured with BT_AUTO_CONN_ALWAYS option
> disconnect. To fix this issue, before dropping the LE connections, we
> trigger the background scan for each connected device that requires
> BT_AUTO_CONN_ALWAYS auto connection.
> 
> Moreover, once the adapter is powered on, we should start the background
> scan if we have triggers registered. This way, we keep the background
> scan running after a power off and power on sequence.

these are actually two independent patches.

> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 35 +++++++++++++++++++++++++++++++++++
> net/bluetooth/hci_core.c |  2 ++
> 2 files changed, 37 insertions(+)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 5caf13b..66823eb 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -954,6 +954,31 @@ timer:
> 				   msecs_to_jiffies(hdev->idle_timeout));
> }
> 
> +static void le_conn_drop_fixup(struct hci_conn *conn)
> +{
> +	struct hci_dev *hdev = conn->hdev;
> +	struct hci_conn_param *param;
> +	int err;
> +
> +	param = hci_find_conn_param(hdev, &conn->dst, conn->dst_type);
> +	if (!param)
> +		return;
> +
> +	if (param->auto_connect != BT_AUTO_CONN_ALWAYS)
> +		goto done;
> +
> +	err = hci_trigger_background_scan(hdev);
> +	if (err) {
> +		BT_ERR("Failed to trigger background scanning: %d", err);
> +		goto done;
> +	}
> +
> +	param->bg_scan_triggered = true;
> +
> +done:
> +	hci_conn_param_put(param);
> +}
> +
> /* Drop all connection on the device */
> void hci_conn_hash_flush(struct hci_dev *hdev)
> {
> @@ -963,6 +988,16 @@ void hci_conn_hash_flush(struct hci_dev *hdev)
> 	BT_DBG("hdev %s", hdev->name);
> 
> 	list_for_each_entry_safe(c, n, &h->list, list) {
> +		/* If this is a LE connection in connected state we should do
> +		 * some fixup before dropping this connection. Since no
> +		 * Disconnection Complete Event will be sent to the host, we
> +		 * have to trigger the background scan in case this is a
> +		 * BT_AUTO_CONN_ALWAYS device. This is handled by the le_conn_
> +		 * drop_fixup() helper.
> +		 */
> +		if (c->type == LE_LINK && c->state == BT_CONNECTED)
> +			le_conn_drop_fixup(c);
> +
> 		c->state = BT_CLOSED;

I do not like this part. We already have the case where we need to re-enable advertising after a connection drops. So this should be in a common place. I do not want to hack this in into all kinds of places.

Regards

Marcel

Re: [RFC 15/15] Bluetooth: Auto connection and power off/on

[Andre Guedes]

Hi Marcel,

On Oct 17, 2013, at 7:55 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> If hdev is closed (e.g. Mgmt power off command, RFKILL or controller =
is
>> reset), the established connections are dropped and no Disconnection
>> Complete Event is sent to host. This way, the background scan is not
>> triggered when devices configured with BT_AUTO_CONN_ALWAYS option
>> disconnect. To fix this issue, before dropping the LE connections, we
>> trigger the background scan for each connected device that requires
>> BT_AUTO_CONN_ALWAYS auto connection.
>>=20
>> Moreover, once the adapter is powered on, we should start the =
background
>> scan if we have triggers registered. This way, we keep the background
>> scan running after a power off and power on sequence.
>=20
> these are actually two independent patches.

I'll split this in two patches.

>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_conn.c | 35 +++++++++++++++++++++++++++++++++++
>> net/bluetooth/hci_core.c |  2 ++
>> 2 files changed, 37 insertions(+)
>>=20
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index 5caf13b..66823eb 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -954,6 +954,31 @@ timer:
>> 				   =
msecs_to_jiffies(hdev->idle_timeout));
>> }
>>=20
>> +static void le_conn_drop_fixup(struct hci_conn *conn)
>> +{
>> +	struct hci_dev *hdev =3D conn->hdev;
>> +	struct hci_conn_param *param;
>> +	int err;
>> +
>> +	param =3D hci_find_conn_param(hdev, &conn->dst, conn->dst_type);
>> +	if (!param)
>> +		return;
>> +
>> +	if (param->auto_connect !=3D BT_AUTO_CONN_ALWAYS)
>> +		goto done;
>> +
>> +	err =3D hci_trigger_background_scan(hdev);
>> +	if (err) {
>> +		BT_ERR("Failed to trigger background scanning: %d", =
err);
>> +		goto done;
>> +	}
>> +
>> +	param->bg_scan_triggered =3D true;
>> +
>> +done:
>> +	hci_conn_param_put(param);
>> +}
>> +
>> /* Drop all connection on the device */
>> void hci_conn_hash_flush(struct hci_dev *hdev)
>> {
>> @@ -963,6 +988,16 @@ void hci_conn_hash_flush(struct hci_dev *hdev)
>> 	BT_DBG("hdev %s", hdev->name);
>>=20
>> 	list_for_each_entry_safe(c, n, &h->list, list) {
>> +		/* If this is a LE connection in connected state we =
should do
>> +		 * some fixup before dropping this connection. Since no
>> +		 * Disconnection Complete Event will be sent to the =
host, we
>> +		 * have to trigger the background scan in case this is a
>> +		 * BT_AUTO_CONN_ALWAYS device. This is handled by the =
le_conn_
>> +		 * drop_fixup() helper.
>> +		 */
>> +		if (c->type =3D=3D LE_LINK && c->state =3D=3D =
BT_CONNECTED)
>> +			le_conn_drop_fixup(c);
>> +
>> 		c->state =3D BT_CLOSED;
>=20
> I do not like this part. We already have the case where we need to =
re-enable advertising after a connection drops. So this should be in a =
common place. I do not want to hack this in into all kinds of places.

Ok, I'll follow the same approach we have for re-enable advertising.

Regards,

Andre=