[PATCH 0/8] Get Connection Information

[PATCH 0/8] Get Connection Information

[Andrzej Kaczmarek]

Hi,

Here are patches which implement Get Connection Information mgmt command.
Patches 1-7 provide implementation based on API v2 and I added separate
patch (8) which updates to API v3 to make it easier to spot differences
between v2 and v3 API.

Addition of max TX power level is due to proposed device property with
this value exposed and it seems convenient to retrieve this value using
new Connection Information API.

As for now both RSSI and TX power level are read on request - it's much
simpler this way and later it can be modified to refresh only what is
required for specific request, i.e. refresh only RSSI if TX power level
was not requested.


Andrzej Kaczmarek (8):
  Bluetooth: Store RSSI for connection
  Bluetooth: Store TX power level for connection
  Bluetooth: Move eir_append_data up
  Bluetooth: Add support for user data in hci_request
  Bluetooth: Add support to get connection information
  Bluetooth: Make min interval for connection information configurable
  Bluetooth: Avoid pooling TX power for LE links
  Bluetooth: Add support for max_tx_power in Get Conn Info

 include/net/bluetooth/bluetooth.h |   3 +-
 include/net/bluetooth/hci.h       |  23 +++
 include/net/bluetooth/hci_core.h  |  11 +-
 include/net/bluetooth/mgmt.h      |  17 +++
 net/bluetooth/hci_conn.c          |   6 +-
 net/bluetooth/hci_core.c          |  33 +++--
 net/bluetooth/hci_event.c         |  68 +++++++++
 net/bluetooth/mgmt.c              | 295 +++++++++++++++++++++++++++++++-------
 8 files changed, 391 insertions(+), 65 deletions(-)

-- 
1.9.2

[PATCH 1/8] Bluetooth: Store RSSI for connection

[Andrzej Kaczmarek]

This patch adds support to store RSSI for connection when reply for
HCI_Read_RSSI is received.

Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
---
 include/net/bluetooth/hci.h      | 10 ++++++++++
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_event.c        | 23 +++++++++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 4261a67..ad2ecc9 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1064,6 +1064,16 @@ struct hci_rp_read_page_scan_type {
 	#define PAGE_SCAN_TYPE_STANDARD		0x00
 	#define PAGE_SCAN_TYPE_INTERLACED	0x01
 
+#define HCI_OP_READ_RSSI		0x1405
+struct hci_cp_read_rssi {
+	__le16   handle;
+} __packed;
+struct hci_rp_read_rssi {
+	__u8     status;
+	__le16   handle;
+	__s8     rssi;
+} __packed;
+
 #define HCI_OP_READ_LOCAL_AMP_INFO	0x1409
 struct hci_rp_read_local_amp_info {
 	__u8     status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index d73f418..0318d52 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -374,6 +374,7 @@ struct hci_conn {
 	__u16		setting;
 	__u16		le_conn_min_interval;
 	__u16		le_conn_max_interval;
+	__s8		rssi;
 	unsigned long	flags;
 
 	__u8		remote_cap;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 07c37d0..2bb0053 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1245,6 +1245,25 @@ static void hci_cc_write_remote_amp_assoc(struct hci_dev *hdev,
 	amp_write_rem_assoc_continue(hdev, rp->phy_handle);
 }
 
+static void hci_cc_read_rssi(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct hci_rp_read_rssi *rp = (void *) skb->data;
+	struct hci_conn *conn;
+
+	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
+
+	if (rp->status)
+		return;
+
+	hci_dev_lock(hdev);
+
+	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(rp->handle));
+	if (conn)
+		conn->rssi = rp->rssi;
+
+	hci_dev_unlock(hdev);
+}
+
 static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 {
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
@@ -2637,6 +2656,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cc_write_remote_amp_assoc(hdev, skb);
 		break;
 
+	case HCI_OP_READ_RSSI:
+		hci_cc_read_rssi(hdev, skb);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%4.4x", hdev->name, opcode);
 		break;
-- 
1.9.2

Re: [PATCH 1/8] Bluetooth: Store RSSI for connection

[Marcel Holtmann]

Hi Andrzej,

> This patch adds support to store RSSI for connection when reply for
> HCI_Read_RSSI is received.
> 
> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
> ---
> include/net/bluetooth/hci.h      | 10 ++++++++++
> include/net/bluetooth/hci_core.h |  1 +
> net/bluetooth/hci_event.c        | 23 +++++++++++++++++++++++
> 3 files changed, 34 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[PATCH 2/8] Bluetooth: Store TX power level for connection

[Andrzej Kaczmarek]

This patch adds support to store local TX power level for connection
when reply for HCI_Read_Transmit_Power_Level is received.

Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
---
 include/net/bluetooth/hci.h      | 13 +++++++++++++
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_conn.c         |  1 +
 net/bluetooth/hci_event.c        | 29 +++++++++++++++++++++++++++++
 4 files changed, 44 insertions(+)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index ad2ecc9..0056835 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1054,6 +1054,19 @@ struct hci_cp_write_page_scan_activity {
 	__le16   window;
 } __packed;
 
+#define HCI_OP_READ_TX_POWER_LEVEL	0x0c2d
+	#define TX_POWER_TYPE_CURRENT	0x00
+	#define TX_POWER_TYPE_MAXIMUM	0x01
+struct hci_cp_read_tx_power_level {
+	__le16   handle;
+	__u8     type;
+} __packed;
+struct hci_rp_read_tx_power_level {
+	__u8     status;
+	__le16   handle;
+	__s8     tx_power_level;
+} __packed;
+
 #define HCI_OP_READ_PAGE_SCAN_TYPE	0x0c46
 struct hci_rp_read_page_scan_type {
 	__u8     status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 0318d52..211bad6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -375,6 +375,7 @@ struct hci_conn {
 	__u16		le_conn_min_interval;
 	__u16		le_conn_max_interval;
 	__s8		rssi;
+	__s8		tx_power;
 	unsigned long	flags;
 
 	__u8		remote_cap;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 55a1743..74b368b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -407,6 +407,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
 	conn->io_capability = hdev->io_capability;
 	conn->remote_auth = 0xff;
 	conn->key_type = 0xff;
+	conn->tx_power = HCI_TX_POWER_INVALID;
 
 	set_bit(HCI_CONN_POWER_SAVE, &conn->flags);
 	conn->disc_timeout = HCI_DISCONN_TIMEOUT;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 2bb0053..0e67c8e 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1264,6 +1264,31 @@ static void hci_cc_read_rssi(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_dev_unlock(hdev);
 }
 
+static void hci_cc_read_tx_power_level(struct hci_dev *hdev,
+				       struct sk_buff *skb)
+{
+	struct hci_cp_read_tx_power_level *sent;
+	struct hci_rp_read_tx_power_level *rp = (void *) skb->data;
+	struct hci_conn *conn;
+
+	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
+
+	if (rp->status)
+		return;
+
+	sent = hci_sent_cmd_data(hdev, HCI_OP_READ_TX_POWER_LEVEL);
+	if (!sent)
+		return;
+
+	hci_dev_lock(hdev);
+
+	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(rp->handle));
+	if (conn && sent->type == TX_POWER_TYPE_CURRENT)
+		conn->tx_power = rp->tx_power_level;
+
+	hci_dev_unlock(hdev);
+}
+
 static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 {
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
@@ -2660,6 +2685,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cc_read_rssi(hdev, skb);
 		break;
 
+	case HCI_OP_READ_TX_POWER_LEVEL:
+		hci_cc_read_tx_power_level(hdev, skb);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%4.4x", hdev->name, opcode);
 		break;
-- 
1.9.2

Re: [PATCH 2/8] Bluetooth: Store TX power level for connection

[Marcel Holtmann]

Hi Andrzej,

> This patch adds support to store local TX power level for connection
> when reply for HCI_Read_Transmit_Power_Level is received.
> 
> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
> ---
> include/net/bluetooth/hci.h      | 13 +++++++++++++
> include/net/bluetooth/hci_core.h |  1 +
> net/bluetooth/hci_conn.c         |  1 +
> net/bluetooth/hci_event.c        | 29 +++++++++++++++++++++++++++++
> 4 files changed, 44 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index ad2ecc9..0056835 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1054,6 +1054,19 @@ struct hci_cp_write_page_scan_activity {
> 	__le16   window;
> } __packed;
> 
> +#define HCI_OP_READ_TX_POWER_LEVEL	0x0c2d
> +	#define TX_POWER_TYPE_CURRENT	0x00
> +	#define TX_POWER_TYPE_MAXIMUM	0x01

I am not really liking this part that much at the moment.

> +struct hci_cp_read_tx_power_level {
> +	__le16   handle;
> +	__u8     type;
> +} __packed;
> +struct hci_rp_read_tx_power_level {
> +	__u8     status;
> +	__le16   handle;
> +	__s8     tx_power_level;
> +} __packed;
> +
> #define HCI_OP_READ_PAGE_SCAN_TYPE	0x0c46
> struct hci_rp_read_page_scan_type {
> 	__u8     status;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 0318d52..211bad6 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -375,6 +375,7 @@ struct hci_conn {
> 	__u16		le_conn_min_interval;
> 	__u16		le_conn_max_interval;
> 	__s8		rssi;
> +	__s8		tx_power;
> 	unsigned long	flags;
> 
> 	__u8		remote_cap;
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 55a1743..74b368b 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -407,6 +407,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
> 	conn->io_capability = hdev->io_capability;
> 	conn->remote_auth = 0xff;
> 	conn->key_type = 0xff;
> +	conn->tx_power = HCI_TX_POWER_INVALID;
> 
> 	set_bit(HCI_CONN_POWER_SAVE, &conn->flags);
> 	conn->disc_timeout = HCI_DISCONN_TIMEOUT;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 2bb0053..0e67c8e 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1264,6 +1264,31 @@ static void hci_cc_read_rssi(struct hci_dev *hdev, struct sk_buff *skb)
> 	hci_dev_unlock(hdev);
> }
> 
> +static void hci_cc_read_tx_power_level(struct hci_dev *hdev,
> +				       struct sk_buff *skb)
> +{
> +	struct hci_cp_read_tx_power_level *sent;
> +	struct hci_rp_read_tx_power_level *rp = (void *) skb->data;
> +	struct hci_conn *conn;
> +
> +	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
> +
> +	if (rp->status)
> +		return;
> +
> +	sent = hci_sent_cmd_data(hdev, HCI_OP_READ_TX_POWER_LEVEL);
> +	if (!sent)
> +		return;
> +
> +	hci_dev_lock(hdev);
> +
> +	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(rp->handle));
> +	if (conn && sent->type == TX_POWER_TYPE_CURRENT)

Use 0x00 here instead of TX_POWER_TYPE_CURRENT. We need to fix this later on. We need to care a little bit of namespacing our constants. So we can just do that later.

> +		conn->tx_power = rp->tx_power_level;
> +
> +	hci_dev_unlock(hdev);
> +}
> +
> static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
> {
> 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
> @@ -2660,6 +2685,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 		hci_cc_read_rssi(hdev, skb);
> 		break;
> 
> +	case HCI_OP_READ_TX_POWER_LEVEL:
> +		hci_cc_read_tx_power_level(hdev, skb);
> +		break;
> +
> 	default:
> 		BT_DBG("%s opcode 0x%4.4x", hdev->name, opcode);
> 		break;

Regards

Marcel

[PATCH 3/8] Bluetooth: Move eir_append_data up

[Andrzej Kaczmarek]

This patch moves eir_append_data up in file, close to other eir-related
functions, so we can use it later in mgmt handlers.

Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
---
 net/bluetooth/mgmt.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 54abbce..840b273 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -701,6 +701,17 @@ static void update_adv_data(struct hci_request *req)
 	hci_req_add(req, HCI_OP_LE_SET_ADV_DATA, sizeof(cp), &cp);
 }
 
+static inline u16 eir_append_data(u8 *eir, u16 eir_len, u8 type, u8 *data,
+				  u8 data_len)
+{
+	eir[eir_len++] = sizeof(type) + data_len;
+	eir[eir_len++] = type;
+	memcpy(&eir[eir_len], data, data_len);
+	eir_len += data_len;
+
+	return eir_len;
+}
+
 static void create_eir(struct hci_dev *hdev, u8 *data)
 {
 	u8 *ptr = data;
@@ -5105,17 +5116,6 @@ void mgmt_new_csrk(struct hci_dev *hdev, struct smp_csrk *csrk,
 	mgmt_event(MGMT_EV_NEW_CSRK, hdev, &ev, sizeof(ev), NULL);
 }
 
-static inline u16 eir_append_data(u8 *eir, u16 eir_len, u8 type, u8 *data,
-				  u8 data_len)
-{
-	eir[eir_len++] = sizeof(type) + data_len;
-	eir[eir_len++] = type;
-	memcpy(&eir[eir_len], data, data_len);
-	eir_len += data_len;
-
-	return eir_len;
-}
-
 void mgmt_device_connected(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 			   u8 addr_type, u32 flags, u8 *name, u8 name_len,
 			   u8 *dev_class)
-- 
1.9.2

[PATCH 4/8] Bluetooth: Add support for user data in hci_request

[Andrzej Kaczmarek]

This patch makes possible to store some user data in hci_request struct
which will be available in completion callback. Data can be added when
initializing new request using hci_req_init(). With this it's now easy
to run request which can be associated with e.g. specific hci_conn.

Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
---
 include/net/bluetooth/bluetooth.h |  3 +-
 include/net/bluetooth/hci_core.h  |  3 +-
 net/bluetooth/hci_conn.c          |  4 +-
 net/bluetooth/hci_core.c          | 29 +++++++++-----
 net/bluetooth/mgmt.c              | 84 +++++++++++++++++++++------------------
 5 files changed, 69 insertions(+), 54 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 904777c1..28378ad 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -273,12 +273,13 @@ struct l2cap_ctrl {
 
 struct hci_dev;
 
-typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status);
+typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status, void *data);
 
 struct hci_req_ctrl {
 	bool			start;
 	u8			event;
 	hci_req_complete_t	complete;
+	void			*data;
 };
 
 struct bt_skb_cb {
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 211bad6..30d245f 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1162,6 +1162,7 @@ int hci_unregister_cb(struct hci_cb *hcb);
 struct hci_request {
 	struct hci_dev		*hdev;
 	struct sk_buff_head	cmd_q;
+	void			*data;
 
 	/* If something goes wrong when building the HCI request, the error
 	 * value is stored in this field.
@@ -1169,7 +1170,7 @@ struct hci_request {
 	int			err;
 };
 
-void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
+void hci_req_init(struct hci_request *req, struct hci_dev *hdev, void *data);
 int hci_req_run(struct hci_request *req, hci_req_complete_t complete);
 void hci_req_add(struct hci_request *req, u16 opcode, u32 plen,
 		 const void *param);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 74b368b..78cfe3b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -571,7 +571,7 @@ void hci_le_conn_failed(struct hci_conn *conn, u8 status)
 	mgmt_reenable_advertising(hdev);
 }
 
-static void create_le_conn_complete(struct hci_dev *hdev, u8 status)
+static void create_le_conn_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	struct hci_conn *conn;
 
@@ -728,7 +728,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 	conn->pending_sec_level = sec_level;
 	conn->auth_type = auth_type;
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	if (test_bit(HCI_ADVERTISING, &hdev->dev_flags)) {
 		hci_req_directed_advertising(&req, conn);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d31f144..f5dcbb1 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1020,7 +1020,7 @@ static const struct file_operations le_auto_conn_fops = {
 
 /* ---- HCI requests ---- */
 
-static void hci_req_sync_complete(struct hci_dev *hdev, u8 result)
+static void hci_req_sync_complete(struct hci_dev *hdev, u8 result, void *data)
 {
 	BT_DBG("%s result 0x%2.2x", hdev->name, result);
 
@@ -1106,7 +1106,7 @@ struct sk_buff *__hci_cmd_sync_ev(struct hci_dev *hdev, u16 opcode, u32 plen,
 
 	BT_DBG("%s", hdev->name);
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	hci_req_add_ev(&req, opcode, plen, param, event);
 
@@ -1170,7 +1170,7 @@ static int __hci_req_sync(struct hci_dev *hdev,
 
 	BT_DBG("%s start", hdev->name);
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	hdev->req_status = HCI_REQ_PEND;
 
@@ -3573,7 +3573,7 @@ void hci_pend_le_conns_clear(struct hci_dev *hdev)
 	BT_DBG("All LE pending connections cleared");
 }
 
-static void inquiry_complete(struct hci_dev *hdev, u8 status)
+static void inquiry_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	if (status) {
 		BT_ERR("Failed to start inquiry: status %d", status);
@@ -3585,7 +3585,8 @@ static void inquiry_complete(struct hci_dev *hdev, u8 status)
 	}
 }
 
-static void le_scan_disable_work_complete(struct hci_dev *hdev, u8 status)
+static void le_scan_disable_work_complete(struct hci_dev *hdev, u8 status,
+					  void *data)
 {
 	/* General inquiry access code (GIAC) */
 	u8 lap[3] = { 0x33, 0x8b, 0x9e };
@@ -3606,7 +3607,7 @@ static void le_scan_disable_work_complete(struct hci_dev *hdev, u8 status)
 		break;
 
 	case DISCOV_TYPE_INTERLEAVED:
-		hci_req_init(&req, hdev);
+		hci_req_init(&req, hdev, NULL);
 
 		memset(&cp, 0, sizeof(cp));
 		memcpy(&cp.lap, lap, sizeof(cp.lap));
@@ -3637,7 +3638,7 @@ static void le_scan_disable_work(struct work_struct *work)
 
 	BT_DBG("%s", hdev->name);
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	hci_req_add_le_scan_disable(&req);
 
@@ -4263,10 +4264,11 @@ static void hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 		BT_ERR("%s sending frame failed", hdev->name);
 }
 
-void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
+void hci_req_init(struct hci_request *req, struct hci_dev *hdev, void *data)
 {
 	skb_queue_head_init(&req->cmd_q);
 	req->hdev = hdev;
+	req->data = data;
 	req->err = 0;
 }
 
@@ -4292,6 +4294,7 @@ int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
 
 	skb = skb_peek_tail(&req->cmd_q);
 	bt_cb(skb)->req.complete = complete;
+	bt_cb(skb)->req.data = req->data;
 
 	spin_lock_irqsave(&hdev->cmd_q.lock, flags);
 	skb_queue_splice_tail(&req->cmd_q, &hdev->cmd_q);
@@ -5083,6 +5086,7 @@ static void hci_resend_last(struct hci_dev *hdev)
 void hci_req_cmd_complete(struct hci_dev *hdev, u16 opcode, u8 status)
 {
 	hci_req_complete_t req_complete = NULL;
+	void *req_data = NULL;
 	struct sk_buff *skb;
 	unsigned long flags;
 
@@ -5116,6 +5120,7 @@ void hci_req_cmd_complete(struct hci_dev *hdev, u16 opcode, u8 status)
 	 */
 	if (hdev->sent_cmd) {
 		req_complete = bt_cb(hdev->sent_cmd)->req.complete;
+		req_data = bt_cb(hdev->sent_cmd)->req.data;
 
 		if (req_complete) {
 			/* We must set the complete callback to NULL to
@@ -5137,13 +5142,14 @@ void hci_req_cmd_complete(struct hci_dev *hdev, u16 opcode, u8 status)
 		}
 
 		req_complete = bt_cb(skb)->req.complete;
+		req_data = bt_cb(skb)->req.data;
 		kfree_skb(skb);
 	}
 	spin_unlock_irqrestore(&hdev->cmd_q.lock, flags);
 
 call_complete:
 	if (req_complete)
-		req_complete(hdev, status);
+		req_complete(hdev, status, req_data);
 }
 
 static void hci_rx_work(struct work_struct *work)
@@ -5273,7 +5279,8 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
 		    &enable_cp);
 }
 
-static void update_background_scan_complete(struct hci_dev *hdev, u8 status)
+static void update_background_scan_complete(struct hci_dev *hdev, u8 status,
+					    void *data)
 {
 	if (status)
 		BT_DBG("HCI request failed to update background scanning: "
@@ -5292,7 +5299,7 @@ void hci_update_background_scan(struct hci_dev *hdev)
 	struct hci_conn *conn;
 	int err;
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	if (list_empty(&hdev->pend_le_conns)) {
 		/* If there is no pending LE connections, we should stop
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 840b273..bb3188f 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -896,7 +896,7 @@ static void service_cache_off(struct work_struct *work)
 	if (!test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->dev_flags))
 		return;
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	hci_dev_lock(hdev);
 
@@ -926,7 +926,7 @@ static void rpa_expired(struct work_struct *work)
 	 * controller happens in the enable_advertising() function.
 	 */
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	disable_advertising(&req);
 	enable_advertising(&req);
@@ -1046,7 +1046,7 @@ static int send_settings_rsp(struct sock *sk, u16 opcode, struct hci_dev *hdev)
 			    sizeof(settings));
 }
 
-static void clean_up_hci_complete(struct hci_dev *hdev, u8 status)
+static void clean_up_hci_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	BT_DBG("%s status 0x%02x", hdev->name, status);
 
@@ -1061,7 +1061,7 @@ static int clean_up_hci_state(struct hci_dev *hdev)
 	struct hci_request req;
 	struct hci_conn *conn;
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	if (test_bit(HCI_ISCAN, &hdev->flags) ||
 	    test_bit(HCI_PSCAN, &hdev->flags)) {
@@ -1266,7 +1266,8 @@ static u8 mgmt_le_support(struct hci_dev *hdev)
 		return MGMT_STATUS_SUCCESS;
 }
 
-static void set_discoverable_complete(struct hci_dev *hdev, u8 status)
+static void set_discoverable_complete(struct hci_dev *hdev, u8 status,
+				      void *data)
 {
 	struct pending_cmd *cmd;
 	struct mgmt_mode *cp;
@@ -1312,7 +1313,7 @@ static void set_discoverable_complete(struct hci_dev *hdev, u8 status)
 	 * that class of device has the limited discoverable
 	 * bit correctly set.
 	 */
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 	update_class(&req);
 	hci_req_run(&req, NULL);
 
@@ -1436,7 +1437,7 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
 	else
 		clear_bit(HCI_LIMITED_DISCOVERABLE, &hdev->dev_flags);
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	/* The procedure for LE-only controllers is much simpler - just
 	 * update the advertising data.
@@ -1523,7 +1524,8 @@ static void write_fast_connectable(struct hci_request *req, bool enable)
 		hci_req_add(req, HCI_OP_WRITE_PAGE_SCAN_TYPE, 1, &type);
 }
 
-static void set_connectable_complete(struct hci_dev *hdev, u8 status)
+static void set_connectable_complete(struct hci_dev *hdev, u8 status,
+				     void *data)
 {
 	struct pending_cmd *cmd;
 	struct mgmt_mode *cp;
@@ -1627,7 +1629,7 @@ static int set_connectable(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto failed;
 	}
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	/* If BR/EDR is not enabled and we disable advertising as a
 	 * by-product of disabling connectable, we need to update the
@@ -1913,7 +1915,7 @@ unlock:
 	return err;
 }
 
-static void le_enable_complete(struct hci_dev *hdev, u8 status)
+static void le_enable_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	struct cmd_lookup match = { NULL, hdev };
 
@@ -1942,7 +1944,7 @@ static void le_enable_complete(struct hci_dev *hdev, u8 status)
 
 		hci_dev_lock(hdev);
 
-		hci_req_init(&req, hdev);
+		hci_req_init(&req, hdev, NULL);
 		update_adv_data(&req);
 		update_scan_rsp_data(&req);
 		hci_req_run(&req, NULL);
@@ -2016,7 +2018,7 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 		goto unlock;
 	}
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	memset(&hci_cp, 0, sizeof(hci_cp));
 
@@ -2101,7 +2103,7 @@ unlock:
 	hci_dev_unlock(hdev);
 }
 
-static void add_uuid_complete(struct hci_dev *hdev, u8 status)
+static void add_uuid_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	BT_DBG("status 0x%02x", status);
 
@@ -2138,7 +2140,7 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 
 	list_add_tail(&uuid->list, &hdev->uuids);
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	update_class(&req);
 	update_eir(&req);
@@ -2180,7 +2182,7 @@ static bool enable_service_cache(struct hci_dev *hdev)
 	return false;
 }
 
-static void remove_uuid_complete(struct hci_dev *hdev, u8 status)
+static void remove_uuid_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	BT_DBG("status 0x%02x", status);
 
@@ -2237,7 +2239,7 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data,
 	}
 
 update_class:
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	update_class(&req);
 	update_eir(&req);
@@ -2265,7 +2267,7 @@ unlock:
 	return err;
 }
 
-static void set_class_complete(struct hci_dev *hdev, u8 status)
+static void set_class_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	BT_DBG("status 0x%02x", status);
 
@@ -2309,7 +2311,7 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->dev_flags)) {
 		hci_dev_unlock(hdev);
@@ -3119,7 +3121,7 @@ static void update_name(struct hci_request *req)
 	hci_req_add(req, HCI_OP_WRITE_LOCAL_NAME, sizeof(cp), &cp);
 }
 
-static void set_name_complete(struct hci_dev *hdev, u8 status)
+static void set_name_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	struct mgmt_cp_set_local_name *cp;
 	struct pending_cmd *cmd;
@@ -3194,7 +3196,7 @@ static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	memcpy(hdev->dev_name, cp->name, sizeof(hdev->dev_name));
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	if (lmp_bredr_capable(hdev)) {
 		update_name(&req);
@@ -3357,7 +3359,8 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
 	return err;
 }
 
-static void start_discovery_complete(struct hci_dev *hdev, u8 status)
+static void start_discovery_complete(struct hci_dev *hdev, u8 status,
+				     void *data)
 {
 	unsigned long timeout = 0;
 
@@ -3440,7 +3443,7 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 
 	hdev->discovery.type = cp->type;
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	switch (hdev->discovery.type) {
 	case DISCOV_TYPE_BREDR:
@@ -3561,7 +3564,8 @@ static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
 	return err;
 }
 
-static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
+static void stop_discovery_complete(struct hci_dev *hdev, u8 status,
+				    void *data)
 {
 	BT_DBG("status %d", status);
 
@@ -3612,7 +3616,7 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	switch (hdev->discovery.state) {
 	case DISCOVERY_FINDING:
@@ -3793,7 +3797,7 @@ static int set_device_id(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	err = cmd_complete(sk, hdev->id, MGMT_OP_SET_DEVICE_ID, 0, NULL, 0);
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 	update_eir(&req);
 	hci_req_run(&req, NULL);
 
@@ -3802,7 +3806,8 @@ static int set_device_id(struct sock *sk, struct hci_dev *hdev, void *data,
 	return err;
 }
 
-static void set_advertising_complete(struct hci_dev *hdev, u8 status)
+static void set_advertising_complete(struct hci_dev *hdev, u8 status,
+				     void *data)
 {
 	struct cmd_lookup match = { NULL, hdev };
 
@@ -3885,7 +3890,7 @@ static int set_advertising(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	if (val)
 		enable_advertising(&req);
@@ -3984,7 +3989,7 @@ static int set_scan_params(struct sock *sk, struct hci_dev *hdev,
 	    hdev->discovery.state == DISCOVERY_STOPPED) {
 		struct hci_request req;
 
-		hci_req_init(&req, hdev);
+		hci_req_init(&req, hdev, NULL);
 
 		hci_req_add_le_scan_disable(&req);
 		hci_req_add_le_passive_scan(&req);
@@ -3997,7 +4002,8 @@ static int set_scan_params(struct sock *sk, struct hci_dev *hdev,
 	return err;
 }
 
-static void fast_connectable_complete(struct hci_dev *hdev, u8 status)
+static void fast_connectable_complete(struct hci_dev *hdev, u8 status,
+				      void *data)
 {
 	struct pending_cmd *cmd;
 
@@ -4078,7 +4084,7 @@ static int set_fast_connectable(struct sock *sk, struct hci_dev *hdev,
 		goto unlock;
 	}
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	write_fast_connectable(&req, cp->val);
 
@@ -4115,7 +4121,7 @@ static void set_bredr_scan(struct hci_request *req)
 		hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
 }
 
-static void set_bredr_complete(struct hci_dev *hdev, u8 status)
+static void set_bredr_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	struct pending_cmd *cmd;
 
@@ -4218,7 +4224,7 @@ static int set_bredr(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 	 */
 	set_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	if (test_bit(HCI_CONNECTABLE, &hdev->dev_flags))
 		set_bredr_scan(&req);
@@ -4750,7 +4756,7 @@ static void restart_le_auto_conns(struct hci_dev *hdev)
 	}
 }
 
-static void powered_complete(struct hci_dev *hdev, u8 status)
+static void powered_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	struct cmd_lookup match = { NULL, hdev };
 
@@ -4775,7 +4781,7 @@ static int powered_update_hci(struct hci_dev *hdev)
 	struct hci_request req;
 	u8 link_sec;
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags) &&
 	    !lmp_host_ssp_capable(hdev)) {
@@ -4898,7 +4904,7 @@ void mgmt_discoverable_timeout(struct hci_dev *hdev)
 	clear_bit(HCI_LIMITED_DISCOVERABLE, &hdev->dev_flags);
 	clear_bit(HCI_DISCOVERABLE, &hdev->dev_flags);
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 	if (test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
 		u8 scan = SCAN_PAGE;
 		hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE,
@@ -4944,7 +4950,7 @@ void mgmt_discoverable(struct hci_dev *hdev, u8 discoverable)
 		 * a disabling of connectable there could be a need to
 		 * update the advertising flags.
 		 */
-		hci_req_init(&req, hdev);
+		hci_req_init(&req, hdev, NULL);
 		update_adv_data(&req);
 		hci_req_run(&req, NULL);
 
@@ -5521,7 +5527,7 @@ void mgmt_ssp_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
 	if (match.sk)
 		sock_put(match.sk);
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 
 	if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags))
 		update_eir(&req);
@@ -5799,7 +5805,7 @@ int mgmt_device_unblocked(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type)
 			  cmd ? cmd->sk : NULL);
 }
 
-static void adv_enable_complete(struct hci_dev *hdev, u8 status)
+static void adv_enable_complete(struct hci_dev *hdev, u8 status, void *data)
 {
 	BT_DBG("%s status %u", hdev->name, status);
 
@@ -5820,7 +5826,7 @@ void mgmt_reenable_advertising(struct hci_dev *hdev)
 	if (!test_bit(HCI_ADVERTISING, &hdev->dev_flags))
 		return;
 
-	hci_req_init(&req, hdev);
+	hci_req_init(&req, hdev, NULL);
 	enable_advertising(&req);
 
 	/* If this fails we have no option but to let user space know
-- 
1.9.2

Re: [PATCH 4/8] Bluetooth: Add support for user data in hci_request

[Marcel Holtmann]

Hi Andrzej,

> This patch makes possible to store some user data in hci_request struct
> which will be available in completion callback. Data can be added when
> initializing new request using hci_req_init(). With this it's now easy
> to run request which can be associated with e.g. specific hci_conn.
> 
> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
> ---
> include/net/bluetooth/bluetooth.h |  3 +-
> include/net/bluetooth/hci_core.h  |  3 +-
> net/bluetooth/hci_conn.c          |  4 +-
> net/bluetooth/hci_core.c          | 29 +++++++++-----
> net/bluetooth/mgmt.c              | 84 +++++++++++++++++++++------------------
> 5 files changed, 69 insertions(+), 54 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 904777c1..28378ad 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -273,12 +273,13 @@ struct l2cap_ctrl {
> 
> struct hci_dev;
> 
> -typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status);
> +typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status, void *data);
> 
> struct hci_req_ctrl {
> 	bool			start;
> 	u8			event;
> 	hci_req_complete_t	complete;
> +	void			*data;
> };
> 
> struct bt_skb_cb {
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 211bad6..30d245f 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1162,6 +1162,7 @@ int hci_unregister_cb(struct hci_cb *hcb);
> struct hci_request {
> 	struct hci_dev		*hdev;
> 	struct sk_buff_head	cmd_q;
> +	void			*data;
> 
> 	/* If something goes wrong when building the HCI request, the error
> 	 * value is stored in this field.
> @@ -1169,7 +1170,7 @@ struct hci_request {
> 	int			err;
> };
> 
> -void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
> +void hci_req_init(struct hci_request *req, struct hci_dev *hdev, void *data);

do we really want to have everybody add extra data in here. We are now touching every possible call that we have so far. I do not really like this at the moment.

Two questions. a) can we do this without this change and b) if we do it can someone benefit from it.

Regards

Marcel

Re: [PATCH 4/8] Bluetooth: Add support for user data in hci_request

[Andrzej Kaczmarek]

Hi Marcel,

On 8 May 2014 17:32, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andrzej,
>
>> This patch makes possible to store some user data in hci_request struct
>> which will be available in completion callback. Data can be added when
>> initializing new request using hci_req_init(). With this it's now easy
>> to run request which can be associated with e.g. specific hci_conn.
>>
>> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
>> ---
>> include/net/bluetooth/bluetooth.h |  3 +-
>> include/net/bluetooth/hci_core.h  |  3 +-
>> net/bluetooth/hci_conn.c          |  4 +-
>> net/bluetooth/hci_core.c          | 29 +++++++++-----
>> net/bluetooth/mgmt.c              | 84 +++++++++++++++++++++------------------
>> 5 files changed, 69 insertions(+), 54 deletions(-)
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index 904777c1..28378ad 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -273,12 +273,13 @@ struct l2cap_ctrl {
>>
>> struct hci_dev;
>>
>> -typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status);
>> +typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status, void *data);
>>
>> struct hci_req_ctrl {
>>       bool                    start;
>>       u8                      event;
>>       hci_req_complete_t      complete;
>> +     void                    *data;
>> };
>>
>> struct bt_skb_cb {
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 211bad6..30d245f 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -1162,6 +1162,7 @@ int hci_unregister_cb(struct hci_cb *hcb);
>> struct hci_request {
>>       struct hci_dev          *hdev;
>>       struct sk_buff_head     cmd_q;
>> +     void                    *data;
>>
>>       /* If something goes wrong when building the HCI request, the error
>>        * value is stored in this field.
>> @@ -1169,7 +1170,7 @@ struct hci_request {
>>       int                     err;
>> };
>>
>> -void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
>> +void hci_req_init(struct hci_request *req, struct hci_dev *hdev, void *data);
>
> do we really want to have everybody add extra data in here. We are now touching every possible call that we have so far. I do not really like this at the moment.

We can skip it here and either introduce helper to set user data for
request or set it directly in structure, but anyway we'll still need
to modify callbacks.

> Two questions. a) can we do this without this change and b) if we do it can someone benefit from it.

In our case, perhaps we can create queue (in hdev?) where we'll put
hci_conn for each conn info request run so we can pop it from queue in
callback. Should be enough.
But user data could be reused in case other request needs more context
than just hdev, e.g. hci_connect_le has request which also needs
hci_conn in callback but right now it deals with it by looking for
single connection in BT_CONNECT state. We obviously can't do this
since we'll have multiple connections in BT_CONNECTED state.

BR,
Andrzej

Re: [PATCH 4/8] Bluetooth: Add support for user data in hci_request

[Johan Hedberg]

Hi Marcel,

On Thu, May 08, 2014, Marcel Holtmann wrote:
> > This patch makes possible to store some user data in hci_request struct
> > which will be available in completion callback. Data can be added when
> > initializing new request using hci_req_init(). With this it's now easy
> > to run request which can be associated with e.g. specific hci_conn.
> > 
> > Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
> > ---
> > include/net/bluetooth/bluetooth.h |  3 +-
> > include/net/bluetooth/hci_core.h  |  3 +-
> > net/bluetooth/hci_conn.c          |  4 +-
> > net/bluetooth/hci_core.c          | 29 +++++++++-----
> > net/bluetooth/mgmt.c              | 84 +++++++++++++++++++++------------------
> > 5 files changed, 69 insertions(+), 54 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 904777c1..28378ad 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -273,12 +273,13 @@ struct l2cap_ctrl {
> > 
> > struct hci_dev;
> > 
> > -typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status);
> > +typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status, void *data);
> > 
> > struct hci_req_ctrl {
> > 	bool			start;
> > 	u8			event;
> > 	hci_req_complete_t	complete;
> > +	void			*data;
> > };
> > 
> > struct bt_skb_cb {
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 211bad6..30d245f 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -1162,6 +1162,7 @@ int hci_unregister_cb(struct hci_cb *hcb);
> > struct hci_request {
> > 	struct hci_dev		*hdev;
> > 	struct sk_buff_head	cmd_q;
> > +	void			*data;
> > 
> > 	/* If something goes wrong when building the HCI request, the error
> > 	 * value is stored in this field.
> > @@ -1169,7 +1170,7 @@ struct hci_request {
> > 	int			err;
> > };
> > 
> > -void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
> > +void hci_req_init(struct hci_request *req, struct hci_dev *hdev, void *data);
> 
> do we really want to have everybody add extra data in here. We are now
> touching every possible call that we have so far. I do not really like
> this at the moment.

Maybe hci_sent_cmd_data() could be used instead to help look up the
connection that the completed command was targeting?

Johan

Re: [PATCH 4/8] Bluetooth: Add support for user data in hci_request

[Andrzej Kaczmarek]

Hi Johan,

On 9 May 2014 12:27, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Marcel,
>
> On Thu, May 08, 2014, Marcel Holtmann wrote:
>> > This patch makes possible to store some user data in hci_request struct
>> > which will be available in completion callback. Data can be added when
>> > initializing new request using hci_req_init(). With this it's now easy
>> > to run request which can be associated with e.g. specific hci_conn.
>> >
>> > Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
>> > ---
>> > include/net/bluetooth/bluetooth.h |  3 +-
>> > include/net/bluetooth/hci_core.h  |  3 +-
>> > net/bluetooth/hci_conn.c          |  4 +-
>> > net/bluetooth/hci_core.c          | 29 +++++++++-----
>> > net/bluetooth/mgmt.c              | 84 +++++++++++++++++++++------------------
>> > 5 files changed, 69 insertions(+), 54 deletions(-)
>> >
>> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> > index 904777c1..28378ad 100644
>> > --- a/include/net/bluetooth/bluetooth.h
>> > +++ b/include/net/bluetooth/bluetooth.h
>> > @@ -273,12 +273,13 @@ struct l2cap_ctrl {
>> >
>> > struct hci_dev;
>> >
>> > -typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status);
>> > +typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status, void *data);
>> >
>> > struct hci_req_ctrl {
>> >     bool                    start;
>> >     u8                      event;
>> >     hci_req_complete_t      complete;
>> > +   void                    *data;
>> > };
>> >
>> > struct bt_skb_cb {
>> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> > index 211bad6..30d245f 100644
>> > --- a/include/net/bluetooth/hci_core.h
>> > +++ b/include/net/bluetooth/hci_core.h
>> > @@ -1162,6 +1162,7 @@ int hci_unregister_cb(struct hci_cb *hcb);
>> > struct hci_request {
>> >     struct hci_dev          *hdev;
>> >     struct sk_buff_head     cmd_q;
>> > +   void                    *data;
>> >
>> >     /* If something goes wrong when building the HCI request, the error
>> >      * value is stored in this field.
>> > @@ -1169,7 +1170,7 @@ struct hci_request {
>> >     int                     err;
>> > };
>> >
>> > -void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
>> > +void hci_req_init(struct hci_request *req, struct hci_dev *hdev, void *data);
>>
>> do we really want to have everybody add extra data in here. We are now
>> touching every possible call that we have so far. I do not really like
>> this at the moment.
>
> Maybe hci_sent_cmd_data() could be used instead to help look up the
> connection that the completed command was targeting?

For purpose of this set of patches it should be fine, both Read RSSI
and Read Transmit Power Level have handle as first parameter so we
don't even need to bother with checking which command was sent last in
case of error. So we can use this now and just get back to user_data
in case something pops in future which cannot be resolved using
available structures.

BR,
Andrzej

[PATCH 5/8] Bluetooth: Add support to get connection information

[Andrzej Kaczmarek]

This patch adds support for Get Connection Information mgmt command
which can be used to query for information about connection, i.e. RSSI
or local TX power level.

RSSI is returned as response parameter while any other information is
returned in standard EIR format.

Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
---
 include/net/bluetooth/hci_core.h |   2 +
 include/net/bluetooth/mgmt.h     |  16 ++++
 net/bluetooth/mgmt.c             | 176 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 30d245f..8d7d48b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -378,6 +378,8 @@ struct hci_conn {
 	__s8		tx_power;
 	unsigned long	flags;
 
+	unsigned long	last_info_read;
+
 	__u8		remote_cap;
 	__u8		remote_auth;
 	__u8		remote_id;
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index d4b571c..c26b7e2 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -409,6 +409,22 @@ struct mgmt_cp_load_irks {
 } __packed;
 #define MGMT_LOAD_IRKS_SIZE		2
 
+#define MGMT_CONN_INFO_DATA_TX_POWER	0x00000001
+
+#define MGMT_OP_GET_CONN_INFORMATION	0x0031
+struct mgmt_cp_get_conn_information {
+	struct mgmt_addr_info addr;
+	__u32	data_type;
+} __packed;
+#define MGMT_GET_CONN_INFORMATION_SIZE	(MGMT_ADDR_INFO_SIZE + 4)
+struct mgmt_rp_get_conn_information {
+	struct mgmt_addr_info addr;
+	__s8	rssi;
+	__u32	flags;
+	__le16	eir_len;
+	__u8	eir[0];
+} __packed;
+
 #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 bb3188f..58074c8 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -83,6 +83,7 @@ static const u16 mgmt_commands[] = {
 	MGMT_OP_SET_DEBUG_KEYS,
 	MGMT_OP_SET_PRIVACY,
 	MGMT_OP_LOAD_IRKS,
+	MGMT_OP_GET_CONN_INFORMATION,
 };
 
 static const u16 mgmt_events[] = {
@@ -113,6 +114,8 @@ static const u16 mgmt_events[] = {
 
 #define CACHE_TIMEOUT	msecs_to_jiffies(2 * 1000)
 
+#define CONN_INFO_INTERVAL	msecs_to_jiffies(2 * 1000)
+
 #define hdev_is_powered(hdev) (test_bit(HCI_UP, &hdev->flags) && \
 				!test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
 
@@ -4574,6 +4577,178 @@ static int load_long_term_keys(struct sock *sk, struct hci_dev *hdev,
 	return err;
 }
 
+struct cmd_conn_lookup {
+	struct hci_conn *conn;
+	u8 mgmt_status;
+};
+
+static void conn_info_complete(struct pending_cmd *cmd, void *data)
+{
+	struct cmd_conn_lookup *match = data;
+	struct mgmt_cp_get_conn_information *cp;
+	struct mgmt_rp_get_conn_information *rp;
+	char buf[sizeof(*rp) + 3];
+	struct hci_conn *conn = cmd->user_data;
+	__u16 eir_len = 0;
+
+	if (conn != match->conn)
+		return;
+
+	cp = (struct mgmt_cp_get_conn_information *) cmd->param;
+	rp = (struct mgmt_rp_get_conn_information *) buf;
+
+	memset(buf, 0, sizeof(buf));
+	memcpy(&rp->addr, &cp->addr, sizeof(rp->addr));
+
+	if (match->mgmt_status) {
+		cmd_complete(cmd->sk, cmd->index, MGMT_OP_GET_CONN_INFORMATION,
+			     match->mgmt_status, rp, sizeof(*rp));
+		goto done;
+	}
+
+	rp->rssi = conn->rssi;
+
+	if (cp->data_type & MGMT_CONN_INFO_DATA_TX_POWER) {
+		eir_len = eir_append_data(rp->eir, 0, EIR_TX_POWER,
+					  &conn->tx_power,
+					  sizeof(conn->tx_power));
+
+		rp->eir_len = cpu_to_le16(eir_len);
+	}
+
+	cmd_complete(cmd->sk, cmd->index, MGMT_OP_GET_CONN_INFORMATION,
+			MGMT_STATUS_SUCCESS, rp, sizeof(*rp) + eir_len);
+
+done:
+	hci_conn_drop(conn);
+
+	mgmt_pending_remove(cmd);
+}
+
+static void conn_info_query_complete(struct hci_dev *hdev, u8 status,
+				     void *data)
+{
+	struct hci_conn *conn = data;
+	struct cmd_conn_lookup match = { conn, mgmt_status(status) };
+
+	BT_DBG("status 0x%02x", status);
+
+	if (!status)
+		conn->last_info_read = jiffies;
+
+	hci_dev_lock(hdev);
+
+	mgmt_pending_foreach(MGMT_OP_GET_CONN_INFORMATION, hdev,
+			     conn_info_complete, &match);
+
+	hci_dev_unlock(hdev);
+}
+
+static bool check_pending_conn_info(struct hci_dev *hdev, struct hci_conn *conn)
+{
+	struct pending_cmd *cmd;
+
+	list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
+		if (cmd->opcode == MGMT_OP_GET_CONN_INFORMATION &&
+		    cmd->user_data == conn)
+			return true;
+	}
+
+	return false;
+}
+
+static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *data,
+			 u16 len)
+{
+	struct mgmt_cp_get_conn_information *cp = data;
+	struct mgmt_rp_get_conn_information rp;
+	struct hci_conn *conn;
+	struct pending_cmd *cmd;
+	struct hci_request req;
+	bool is_pending;
+	int err = 0;
+
+	BT_DBG("%s", hdev->name);
+
+	memset(&rp, 0, sizeof(rp));
+	memcpy(&rp.addr, &cp->addr, sizeof(rp.addr));
+
+	if (!bdaddr_type_is_valid(cp->addr.type))
+		return cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
+				    MGMT_STATUS_INVALID_PARAMS,
+				    &rp, sizeof(rp));
+
+	hci_dev_lock(hdev);
+
+	if (!hdev_is_powered(hdev)) {
+		err = cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
+				   MGMT_STATUS_NOT_POWERED, &rp, sizeof(rp));
+		goto unlock;
+	}
+
+	if (cp->addr.type == BDADDR_BREDR)
+		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
+					       &cp->addr.bdaddr);
+	else
+		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->addr.bdaddr);
+
+	if (!conn || conn->state == BT_OPEN || conn->state == BT_CLOSED) {
+		err = cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
+				   MGMT_STATUS_NOT_CONNECTED, &rp, sizeof(rp));
+		goto unlock;
+	}
+
+	is_pending = check_pending_conn_info(hdev, conn);
+
+	cmd = mgmt_pending_add(sk, MGMT_OP_GET_CONN_INFORMATION, hdev,
+			       data, len);
+	if (!cmd) {
+		err = -ENOMEM;
+		goto unlock;
+	}
+
+	hci_conn_hold(conn);
+	cmd->user_data = conn;
+
+	/* There was already pending request for this connection, no need to
+	 * start another request to controller.
+	 */
+	if (is_pending)
+		goto unlock;
+
+	hci_req_init(&req, hdev, conn);
+
+	if (time_after(jiffies, conn->last_info_read + CONN_INFO_INTERVAL) ||
+	    conn->tx_power == HCI_TX_POWER_INVALID) {
+		struct hci_cp_read_rssi req_rssi_cp;
+		struct hci_cp_read_tx_power_level req_txp_cp;
+
+		req_rssi_cp.handle = cpu_to_le16(conn->handle);
+		hci_req_add(&req, HCI_OP_READ_RSSI, sizeof(req_rssi_cp),
+			    &req_rssi_cp);
+
+		req_txp_cp.handle = cpu_to_le16(conn->handle);
+		req_txp_cp.type = TX_POWER_TYPE_CURRENT;
+		hci_req_add(&req, HCI_OP_READ_TX_POWER_LEVEL,
+			    sizeof(req_txp_cp), &req_txp_cp);
+
+		err = hci_req_run(&req, conn_info_query_complete);
+		if (err < 0) {
+			hci_conn_drop(conn);
+			mgmt_pending_remove(cmd);
+		}
+	} else {
+		struct cmd_conn_lookup match = { conn, MGMT_STATUS_SUCCESS };
+
+		/* We know all values and can reply immediately */
+		conn_info_complete(cmd, &match);
+	}
+
+unlock:
+	hci_dev_unlock(hdev);
+	return err;
+}
+
 static const struct mgmt_handler {
 	int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
 		     u16 data_len);
@@ -4629,6 +4804,7 @@ static const struct mgmt_handler {
 	{ set_debug_keys,         false, MGMT_SETTING_SIZE },
 	{ set_privacy,            false, MGMT_SET_PRIVACY_SIZE },
 	{ load_irks,              true,  MGMT_LOAD_IRKS_SIZE },
+	{ get_conn_info,          false, MGMT_GET_CONN_INFORMATION_SIZE },
 };
 
 
-- 
1.9.2

Re: [PATCH 5/8] Bluetooth: Add support to get connection information

[Szymon Janc]

Hi Andrzej,

On Thursday 08 of May 2014 15:32:12 Andrzej Kaczmarek wrote:
> This patch adds support for Get Connection Information mgmt command
> which can be used to query for information about connection, i.e. RSSI
> or local TX power level.
> 
> RSSI is returned as response parameter while any other information is
> returned in standard EIR format.
> 
> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
> ---
>  include/net/bluetooth/hci_core.h |   2 +
>  include/net/bluetooth/mgmt.h     |  16 ++++
>  net/bluetooth/mgmt.c             | 176 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 194 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 30d245f..8d7d48b 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -378,6 +378,8 @@ struct hci_conn {
>  	__s8		tx_power;
>  	unsigned long	flags;
>  
> +	unsigned long	last_info_read;
> +
>  	__u8		remote_cap;
>  	__u8		remote_auth;
>  	__u8		remote_id;
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index d4b571c..c26b7e2 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -409,6 +409,22 @@ struct mgmt_cp_load_irks {
>  } __packed;
>  #define MGMT_LOAD_IRKS_SIZE		2
>  
> +#define MGMT_CONN_INFO_DATA_TX_POWER	0x00000001
> +
> +#define MGMT_OP_GET_CONN_INFORMATION	0x0031
> +struct mgmt_cp_get_conn_information {
> +	struct mgmt_addr_info addr;
> +	__u32	data_type;
> +} __packed;
> +#define MGMT_GET_CONN_INFORMATION_SIZE	(MGMT_ADDR_INFO_SIZE + 4)
> +struct mgmt_rp_get_conn_information {
> +	struct mgmt_addr_info addr;
> +	__s8	rssi;
> +	__u32	flags;
> +	__le16	eir_len;
> +	__u8	eir[0];
> +} __packed;
> +
>  #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 bb3188f..58074c8 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -83,6 +83,7 @@ static const u16 mgmt_commands[] = {
>  	MGMT_OP_SET_DEBUG_KEYS,
>  	MGMT_OP_SET_PRIVACY,
>  	MGMT_OP_LOAD_IRKS,
> +	MGMT_OP_GET_CONN_INFORMATION,
>  };
>  
>  static const u16 mgmt_events[] = {
> @@ -113,6 +114,8 @@ static const u16 mgmt_events[] = {
>  
>  #define CACHE_TIMEOUT	msecs_to_jiffies(2 * 1000)
>  
> +#define CONN_INFO_INTERVAL	msecs_to_jiffies(2 * 1000)
> +
>  #define hdev_is_powered(hdev) (test_bit(HCI_UP, &hdev->flags) && \
>  				!test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
>  
> @@ -4574,6 +4577,178 @@ static int load_long_term_keys(struct sock *sk, struct hci_dev *hdev,
>  	return err;
>  }
>  
> +struct cmd_conn_lookup {
> +	struct hci_conn *conn;
> +	u8 mgmt_status;
> +};
> +
> +static void conn_info_complete(struct pending_cmd *cmd, void *data)
> +{
> +	struct cmd_conn_lookup *match = data;
> +	struct mgmt_cp_get_conn_information *cp;
> +	struct mgmt_rp_get_conn_information *rp;
> +	char buf[sizeof(*rp) + 3];
> +	struct hci_conn *conn = cmd->user_data;
> +	__u16 eir_len = 0;
> +
> +	if (conn != match->conn)
> +		return;
> +
> +	cp = (struct mgmt_cp_get_conn_information *) cmd->param;
> +	rp = (struct mgmt_rp_get_conn_information *) buf;
> +
> +	memset(buf, 0, sizeof(buf));
> +	memcpy(&rp->addr, &cp->addr, sizeof(rp->addr));
> +
> +	if (match->mgmt_status) {
> +		cmd_complete(cmd->sk, cmd->index, MGMT_OP_GET_CONN_INFORMATION,
> +			     match->mgmt_status, rp, sizeof(*rp));
> +		goto done;
> +	}
> +
> +	rp->rssi = conn->rssi;
> +
> +	if (cp->data_type & MGMT_CONN_INFO_DATA_TX_POWER) {
> +		eir_len = eir_append_data(rp->eir, 0, EIR_TX_POWER,
> +					  &conn->tx_power,
> +					  sizeof(conn->tx_power));
> +
> +		rp->eir_len = cpu_to_le16(eir_len);
> +	}
> +
> +	cmd_complete(cmd->sk, cmd->index, MGMT_OP_GET_CONN_INFORMATION,
> +			MGMT_STATUS_SUCCESS, rp, sizeof(*rp) + eir_len);
> +
> +done:
> +	hci_conn_drop(conn);
> +
> +	mgmt_pending_remove(cmd);
> +}
> +
> +static void conn_info_query_complete(struct hci_dev *hdev, u8 status,
> +				     void *data)
> +{
> +	struct hci_conn *conn = data;
> +	struct cmd_conn_lookup match = { conn, mgmt_status(status) };
> +
> +	BT_DBG("status 0x%02x", status);
> +
> +	if (!status)
> +		conn->last_info_read = jiffies;
> +
> +	hci_dev_lock(hdev);
> +
> +	mgmt_pending_foreach(MGMT_OP_GET_CONN_INFORMATION, hdev,
> +			     conn_info_complete, &match);
> +
> +	hci_dev_unlock(hdev);
> +}
> +
> +static bool check_pending_conn_info(struct hci_dev *hdev, struct hci_conn *conn)
> +{
> +	struct pending_cmd *cmd;
> +
> +	list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
> +		if (cmd->opcode == MGMT_OP_GET_CONN_INFORMATION &&
> +		    cmd->user_data == conn)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *data,
> +			 u16 len)
> +{
> +	struct mgmt_cp_get_conn_information *cp = data;
> +	struct mgmt_rp_get_conn_information rp;
> +	struct hci_conn *conn;
> +	struct pending_cmd *cmd;
> +	struct hci_request req;
> +	bool is_pending;
> +	int err = 0;
> +
> +	BT_DBG("%s", hdev->name);
> +
> +	memset(&rp, 0, sizeof(rp));
> +	memcpy(&rp.addr, &cp->addr, sizeof(rp.addr));
> +
> +	if (!bdaddr_type_is_valid(cp->addr.type))
> +		return cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
> +				    MGMT_STATUS_INVALID_PARAMS,
> +				    &rp, sizeof(rp));
> +
> +	hci_dev_lock(hdev);
> +
> +	if (!hdev_is_powered(hdev)) {
> +		err = cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
> +				   MGMT_STATUS_NOT_POWERED, &rp, sizeof(rp));
> +		goto unlock;
> +	}
> +
> +	if (cp->addr.type == BDADDR_BREDR)
> +		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> +					       &cp->addr.bdaddr);
> +	else
> +		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->addr.bdaddr);
> +
> +	if (!conn || conn->state == BT_OPEN || conn->state == BT_CLOSED) {

I think this should be:
if (!conn || conn->state != BT_CONNECTED) {

> +		err = cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
> +				   MGMT_STATUS_NOT_CONNECTED, &rp, sizeof(rp));
> +		goto unlock;
> +	}
> +
> +	is_pending = check_pending_conn_info(hdev, conn);
> +
> +	cmd = mgmt_pending_add(sk, MGMT_OP_GET_CONN_INFORMATION, hdev,
> +			       data, len);
> +	if (!cmd) {
> +		err = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	hci_conn_hold(conn);
> +	cmd->user_data = conn;
> +
> +	/* There was already pending request for this connection, no need to
> +	 * start another request to controller.
> +	 */
> +	if (is_pending)
> +		goto unlock;
> +
> +	hci_req_init(&req, hdev, conn);
> +
> +	if (time_after(jiffies, conn->last_info_read + CONN_INFO_INTERVAL) ||
> +	    conn->tx_power == HCI_TX_POWER_INVALID) {
> +		struct hci_cp_read_rssi req_rssi_cp;
> +		struct hci_cp_read_tx_power_level req_txp_cp;
> +
> +		req_rssi_cp.handle = cpu_to_le16(conn->handle);
> +		hci_req_add(&req, HCI_OP_READ_RSSI, sizeof(req_rssi_cp),
> +			    &req_rssi_cp);
> +
> +		req_txp_cp.handle = cpu_to_le16(conn->handle);
> +		req_txp_cp.type = TX_POWER_TYPE_CURRENT;
> +		hci_req_add(&req, HCI_OP_READ_TX_POWER_LEVEL,
> +			    sizeof(req_txp_cp), &req_txp_cp);
> +
> +		err = hci_req_run(&req, conn_info_query_complete);
> +		if (err < 0) {
> +			hci_conn_drop(conn);
> +			mgmt_pending_remove(cmd);
> +		}
> +	} else {
> +		struct cmd_conn_lookup match = { conn, MGMT_STATUS_SUCCESS };
> +
> +		/* We know all values and can reply immediately */
> +		conn_info_complete(cmd, &match);
> +	}
> +
> +unlock:
> +	hci_dev_unlock(hdev);
> +	return err;
> +}
> +
>  static const struct mgmt_handler {
>  	int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
>  		     u16 data_len);
> @@ -4629,6 +4804,7 @@ static const struct mgmt_handler {
>  	{ set_debug_keys,         false, MGMT_SETTING_SIZE },
>  	{ set_privacy,            false, MGMT_SET_PRIVACY_SIZE },
>  	{ load_irks,              true,  MGMT_LOAD_IRKS_SIZE },
> +	{ get_conn_info,          false, MGMT_GET_CONN_INFORMATION_SIZE },
>  };
>  
>  
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH 5/8] Bluetooth: Add support to get connection information

[Marcel Holtmann]

Hi Andrzej,

> This patch adds support for Get Connection Information mgmt command
> which can be used to query for information about connection, i.e. RSSI
> or local TX power level.
> 
> RSSI is returned as response parameter while any other information is
> returned in standard EIR format.
> 
> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
> ---
> include/net/bluetooth/hci_core.h |   2 +
> include/net/bluetooth/mgmt.h     |  16 ++++
> net/bluetooth/mgmt.c             | 176 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 194 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 30d245f..8d7d48b 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -378,6 +378,8 @@ struct hci_conn {
> 	__s8		tx_power;
> 	unsigned long	flags;
> 
> +	unsigned long	last_info_read;
> +

I will clearly mark this as timestamp. Similar to what we do with the inquiry cache.

> 	__u8		remote_cap;
> 	__u8		remote_auth;
> 	__u8		remote_id;
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index d4b571c..c26b7e2 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -409,6 +409,22 @@ struct mgmt_cp_load_irks {
> } __packed;
> #define MGMT_LOAD_IRKS_SIZE		2
> 
> +#define MGMT_CONN_INFO_DATA_TX_POWER	0x00000001
> +
> +#define MGMT_OP_GET_CONN_INFORMATION	0x0031
> +struct mgmt_cp_get_conn_information {
> +	struct mgmt_addr_info addr;
> +	__u32	data_type;
> +} __packed;
> +#define MGMT_GET_CONN_INFORMATION_SIZE	(MGMT_ADDR_INFO_SIZE + 4)
> +struct mgmt_rp_get_conn_information {
> +	struct mgmt_addr_info addr;
> +	__s8	rssi;
> +	__u32	flags;
> +	__le16	eir_len;
> +	__u8	eir[0];
> +} __packed;

Short this to GET_CONN_INFO and get_conn_info please. We never use the full “information” name.

> +
> #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 bb3188f..58074c8 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -83,6 +83,7 @@ static const u16 mgmt_commands[] = {
> 	MGMT_OP_SET_DEBUG_KEYS,
> 	MGMT_OP_SET_PRIVACY,
> 	MGMT_OP_LOAD_IRKS,
> +	MGMT_OP_GET_CONN_INFORMATION,

Short this to GET_CONN_INFO.

> };
> 
> static const u16 mgmt_events[] = {
> @@ -113,6 +114,8 @@ static const u16 mgmt_events[] = {
> 
> #define CACHE_TIMEOUT	msecs_to_jiffies(2 * 1000)
> 
> +#define CONN_INFO_INTERVAL	msecs_to_jiffies(2 * 1000)
> +

With the inquiry cache we are calling this MAX_AGE. So we should follow this here as well. INTERVAL is the wrong name here.

> #define hdev_is_powered(hdev) (test_bit(HCI_UP, &hdev->flags) && \
> 				!test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
> 
> @@ -4574,6 +4577,178 @@ static int load_long_term_keys(struct sock *sk, struct hci_dev *hdev,
> 	return err;
> }
> 
> +struct cmd_conn_lookup {
> +	struct hci_conn *conn;
> +	u8 mgmt_status;
> +};
> +
> +static void conn_info_complete(struct pending_cmd *cmd, void *data)
> +{
> +	struct cmd_conn_lookup *match = data;
> +	struct mgmt_cp_get_conn_information *cp;
> +	struct mgmt_rp_get_conn_information *rp;
> +	char buf[sizeof(*rp) + 3];
> +	struct hci_conn *conn = cmd->user_data;
> +	__u16 eir_len = 0;
> +
> +	if (conn != match->conn)
> +		return;
> +
> +	cp = (struct mgmt_cp_get_conn_information *) cmd->param;
> +	rp = (struct mgmt_rp_get_conn_information *) buf;
> +
> +	memset(buf, 0, sizeof(buf));
> +	memcpy(&rp->addr, &cp->addr, sizeof(rp->addr));
> +
> +	if (match->mgmt_status) {
> +		cmd_complete(cmd->sk, cmd->index, MGMT_OP_GET_CONN_INFORMATION,
> +			     match->mgmt_status, rp, sizeof(*rp));
> +		goto done;
> +	}
> +
> +	rp->rssi = conn->rssi;
> +
> +	if (cp->data_type & MGMT_CONN_INFO_DATA_TX_POWER) {
> +		eir_len = eir_append_data(rp->eir, 0, EIR_TX_POWER,
> +					  &conn->tx_power,
> +					  sizeof(conn->tx_power));
> +
> +		rp->eir_len = cpu_to_le16(eir_len);
> +	}
> +
> +	cmd_complete(cmd->sk, cmd->index, MGMT_OP_GET_CONN_INFORMATION,
> +			MGMT_STATUS_SUCCESS, rp, sizeof(*rp) + eir_len);
> +
> +done:
> +	hci_conn_drop(conn);
> +
> +	mgmt_pending_remove(cmd);
> +}
> +
> +static void conn_info_query_complete(struct hci_dev *hdev, u8 status,
> +				     void *data)
> +{
> +	struct hci_conn *conn = data;
> +	struct cmd_conn_lookup match = { conn, mgmt_status(status) };
> +
> +	BT_DBG("status 0x%02x", status);
> +
> +	if (!status)
> +		conn->last_info_read = jiffies;
> +
> +	hci_dev_lock(hdev);
> +
> +	mgmt_pending_foreach(MGMT_OP_GET_CONN_INFORMATION, hdev,
> +			     conn_info_complete, &match);
> +
> +	hci_dev_unlock(hdev);
> +}

This whole thing is a bit convoluted. Can we get this done simpler or add some extra comments for people follow through on how this works.

We do need a mgmt-tester test case for this of course.

> +
> +static bool check_pending_conn_info(struct hci_dev *hdev, struct hci_conn *conn)
> +{
> +	struct pending_cmd *cmd;
> +
> +	list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
> +		if (cmd->opcode == MGMT_OP_GET_CONN_INFORMATION &&
> +		    cmd->user_data == conn)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *data,
> +			 u16 len)
> +{
> +	struct mgmt_cp_get_conn_information *cp = data;
> +	struct mgmt_rp_get_conn_information rp;
> +	struct hci_conn *conn;
> +	struct pending_cmd *cmd;
> +	struct hci_request req;
> +	bool is_pending;
> +	int err = 0;
> +
> +	BT_DBG("%s", hdev->name);
> +
> +	memset(&rp, 0, sizeof(rp));
> +	memcpy(&rp.addr, &cp->addr, sizeof(rp.addr));

Other locations in the code are doing this:

        memset(&rp, 0, sizeof(rp));   
        bacpy(&rp.addr.bdaddr, &cp->addr.bdaddr);
        rp.addr.type = cp->addr.type;

> +
> +	if (!bdaddr_type_is_valid(cp->addr.type))
> +		return cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
> +				    MGMT_STATUS_INVALID_PARAMS,
> +				    &rp, sizeof(rp));

> +
> +	hci_dev_lock(hdev);
> +
> +	if (!hdev_is_powered(hdev)) {
> +		err = cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
> +				   MGMT_STATUS_NOT_POWERED, &rp, sizeof(rp));
> +		goto unlock;
> +	}
> +
> +	if (cp->addr.type == BDADDR_BREDR)
> +		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> +					       &cp->addr.bdaddr);
> +	else
> +		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->addr.bdaddr);
> +
> +	if (!conn || conn->state == BT_OPEN || conn->state == BT_CLOSED) {

Lets check for != BT_CONNECTED here.

> +		err = cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
> +				   MGMT_STATUS_NOT_CONNECTED, &rp, sizeof(rp));
> +		goto unlock;
> +	}
> +
> +	is_pending = check_pending_conn_info(hdev, conn);

Don’t we have a mgmt_pending_find for this.

> +
> +	cmd = mgmt_pending_add(sk, MGMT_OP_GET_CONN_INFORMATION, hdev,
> +			       data, len);
> +	if (!cmd) {
> +		err = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	hci_conn_hold(conn);
> +	cmd->user_data = conn;
> +
> +	/* There was already pending request for this connection, no need to
> +	 * start another request to controller.
> +	 */
> +	if (is_pending)
> +		goto unlock;
> +
> +	hci_req_init(&req, hdev, conn);

Move this into the if context.

> +
> +	if (time_after(jiffies, conn->last_info_read + CONN_INFO_INTERVAL) ||
> +	    conn->tx_power == HCI_TX_POWER_INVALID) {
> +		struct hci_cp_read_rssi req_rssi_cp;
> +		struct hci_cp_read_tx_power_level req_txp_cp;
> +
> +		req_rssi_cp.handle = cpu_to_le16(conn->handle);
> +		hci_req_add(&req, HCI_OP_READ_RSSI, sizeof(req_rssi_cp),
> +			    &req_rssi_cp);
> +
> +		req_txp_cp.handle = cpu_to_le16(conn->handle);
> +		req_txp_cp.type = TX_POWER_TYPE_CURRENT;
> +		hci_req_add(&req, HCI_OP_READ_TX_POWER_LEVEL,
> +			    sizeof(req_txp_cp), &req_txp_cp);
> +
> +		err = hci_req_run(&req, conn_info_query_complete);
> +		if (err < 0) {
> +			hci_conn_drop(conn);
> +			mgmt_pending_remove(cmd);
> +		}
> +	} else {
> +		struct cmd_conn_lookup match = { conn, MGMT_STATUS_SUCCESS };
> +
> +		/* We know all values and can reply immediately */
> +		conn_info_complete(cmd, &match);
> +	}
> +
> +unlock:
> +	hci_dev_unlock(hdev);
> +	return err;
> +}
> +
> static const struct mgmt_handler {
> 	int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
> 		     u16 data_len);
> @@ -4629,6 +4804,7 @@ static const struct mgmt_handler {
> 	{ set_debug_keys,         false, MGMT_SETTING_SIZE },
> 	{ set_privacy,            false, MGMT_SET_PRIVACY_SIZE },
> 	{ load_irks,              true,  MGMT_LOAD_IRKS_SIZE },
> +	{ get_conn_info,          false, MGMT_GET_CONN_INFORMATION_SIZE },
> };

Regards

Marcel

Re: [PATCH 5/8] Bluetooth: Add support to get connection information

[Andrzej Kaczmarek]

Hi Marcel,

On 8 May 2014 17:26, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andrzej,
>
>> This patch adds support for Get Connection Information mgmt command
>> which can be used to query for information about connection, i.e. RSSI
>> or local TX power level.
>>
>> RSSI is returned as response parameter while any other information is
>> returned in standard EIR format.
>>
>> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
>> ---
>> include/net/bluetooth/hci_core.h |   2 +
>> include/net/bluetooth/mgmt.h     |  16 ++++
>> net/bluetooth/mgmt.c             | 176 +++++++++++++++++++++++++++++++++=
++++++
>> 3 files changed, 194 insertions(+)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hc=
i_core.h
>> index 30d245f..8d7d48b 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -378,6 +378,8 @@ struct hci_conn {
>>       __s8            tx_power;
>>       unsigned long   flags;
>>
>> +     unsigned long   last_info_read;
>> +
>
> I will clearly mark this as timestamp. Similar to what we do with the inq=
uiry cache.
>
>>       __u8            remote_cap;
>>       __u8            remote_auth;
>>       __u8            remote_id;
>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
>> index d4b571c..c26b7e2 100644
>> --- a/include/net/bluetooth/mgmt.h
>> +++ b/include/net/bluetooth/mgmt.h
>> @@ -409,6 +409,22 @@ struct mgmt_cp_load_irks {
>> } __packed;
>> #define MGMT_LOAD_IRKS_SIZE           2
>>
>> +#define MGMT_CONN_INFO_DATA_TX_POWER 0x00000001
>> +
>> +#define MGMT_OP_GET_CONN_INFORMATION 0x0031
>> +struct mgmt_cp_get_conn_information {
>> +     struct mgmt_addr_info addr;
>> +     __u32   data_type;
>> +} __packed;
>> +#define MGMT_GET_CONN_INFORMATION_SIZE       (MGMT_ADDR_INFO_SIZE + 4)
>> +struct mgmt_rp_get_conn_information {
>> +     struct mgmt_addr_info addr;
>> +     __s8    rssi;
>> +     __u32   flags;
>> +     __le16  eir_len;
>> +     __u8    eir[0];
>> +} __packed;
>
> Short this to GET_CONN_INFO and get_conn_info please. We never use the fu=
ll =E2=80=9Cinformation=E2=80=9D name.
>
>> +
>> #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 bb3188f..58074c8 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -83,6 +83,7 @@ static const u16 mgmt_commands[] =3D {
>>       MGMT_OP_SET_DEBUG_KEYS,
>>       MGMT_OP_SET_PRIVACY,
>>       MGMT_OP_LOAD_IRKS,
>> +     MGMT_OP_GET_CONN_INFORMATION,
>
> Short this to GET_CONN_INFO.
>
>> };
>>
>> static const u16 mgmt_events[] =3D {
>> @@ -113,6 +114,8 @@ static const u16 mgmt_events[] =3D {
>>
>> #define CACHE_TIMEOUT msecs_to_jiffies(2 * 1000)
>>
>> +#define CONN_INFO_INTERVAL   msecs_to_jiffies(2 * 1000)
>> +
>
> With the inquiry cache we are calling this MAX_AGE. So we should follow t=
his here as well. INTERVAL is the wrong name here.
>
>> #define hdev_is_powered(hdev) (test_bit(HCI_UP, &hdev->flags) && \
>>                               !test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
>>
>> @@ -4574,6 +4577,178 @@ static int load_long_term_keys(struct sock *sk, =
struct hci_dev *hdev,
>>       return err;
>> }
>>
>> +struct cmd_conn_lookup {
>> +     struct hci_conn *conn;
>> +     u8 mgmt_status;
>> +};
>> +
>> +static void conn_info_complete(struct pending_cmd *cmd, void *data)
>> +{
>> +     struct cmd_conn_lookup *match =3D data;
>> +     struct mgmt_cp_get_conn_information *cp;
>> +     struct mgmt_rp_get_conn_information *rp;
>> +     char buf[sizeof(*rp) + 3];
>> +     struct hci_conn *conn =3D cmd->user_data;
>> +     __u16 eir_len =3D 0;
>> +
>> +     if (conn !=3D match->conn)
>> +             return;
>> +
>> +     cp =3D (struct mgmt_cp_get_conn_information *) cmd->param;
>> +     rp =3D (struct mgmt_rp_get_conn_information *) buf;
>> +
>> +     memset(buf, 0, sizeof(buf));
>> +     memcpy(&rp->addr, &cp->addr, sizeof(rp->addr));
>> +
>> +     if (match->mgmt_status) {
>> +             cmd_complete(cmd->sk, cmd->index, MGMT_OP_GET_CONN_INFORMA=
TION,
>> +                          match->mgmt_status, rp, sizeof(*rp));
>> +             goto done;
>> +     }
>> +
>> +     rp->rssi =3D conn->rssi;
>> +
>> +     if (cp->data_type & MGMT_CONN_INFO_DATA_TX_POWER) {
>> +             eir_len =3D eir_append_data(rp->eir, 0, EIR_TX_POWER,
>> +                                       &conn->tx_power,
>> +                                       sizeof(conn->tx_power));
>> +
>> +             rp->eir_len =3D cpu_to_le16(eir_len);
>> +     }
>> +
>> +     cmd_complete(cmd->sk, cmd->index, MGMT_OP_GET_CONN_INFORMATION,
>> +                     MGMT_STATUS_SUCCESS, rp, sizeof(*rp) + eir_len);
>> +
>> +done:
>> +     hci_conn_drop(conn);
>> +
>> +     mgmt_pending_remove(cmd);
>> +}
>> +
>> +static void conn_info_query_complete(struct hci_dev *hdev, u8 status,
>> +                                  void *data)
>> +{
>> +     struct hci_conn *conn =3D data;
>> +     struct cmd_conn_lookup match =3D { conn, mgmt_status(status) };
>> +
>> +     BT_DBG("status 0x%02x", status);
>> +
>> +     if (!status)
>> +             conn->last_info_read =3D jiffies;
>> +
>> +     hci_dev_lock(hdev);
>> +
>> +     mgmt_pending_foreach(MGMT_OP_GET_CONN_INFORMATION, hdev,
>> +                          conn_info_complete, &match);
>> +
>> +     hci_dev_unlock(hdev);
>> +}
>
> This whole thing is a bit convoluted. Can we get this done simpler or add=
 some extra comments for people follow through on how this works.

I'll see if this can be made any simpler and add some extra comments at lea=
st.

> We do need a mgmt-tester test case for this of course.

Sure, I'll create one once we have final specification for this command.

>> +
>> +static bool check_pending_conn_info(struct hci_dev *hdev, struct hci_co=
nn *conn)
>> +{
>> +     struct pending_cmd *cmd;
>> +
>> +     list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
>> +             if (cmd->opcode =3D=3D MGMT_OP_GET_CONN_INFORMATION &&
>> +                 cmd->user_data =3D=3D conn)
>> +                     return true;
>> +     }
>> +
>> +     return false;
>> +}
>> +
>> +static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *d=
ata,
>> +                      u16 len)
>> +{
>> +     struct mgmt_cp_get_conn_information *cp =3D data;
>> +     struct mgmt_rp_get_conn_information rp;
>> +     struct hci_conn *conn;
>> +     struct pending_cmd *cmd;
>> +     struct hci_request req;
>> +     bool is_pending;
>> +     int err =3D 0;
>> +
>> +     BT_DBG("%s", hdev->name);
>> +
>> +     memset(&rp, 0, sizeof(rp));
>> +     memcpy(&rp.addr, &cp->addr, sizeof(rp.addr));
>
> Other locations in the code are doing this:
>
>         memset(&rp, 0, sizeof(rp));
>         bacpy(&rp.addr.bdaddr, &cp->addr.bdaddr);
>         rp.addr.type =3D cp->addr.type;
>
>> +
>> +     if (!bdaddr_type_is_valid(cp->addr.type))
>> +             return cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMA=
TION,
>> +                                 MGMT_STATUS_INVALID_PARAMS,
>> +                                 &rp, sizeof(rp));
>
>> +
>> +     hci_dev_lock(hdev);
>> +
>> +     if (!hdev_is_powered(hdev)) {
>> +             err =3D cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORM=
ATION,
>> +                                MGMT_STATUS_NOT_POWERED, &rp, sizeof(rp=
));
>> +             goto unlock;
>> +     }
>> +
>> +     if (cp->addr.type =3D=3D BDADDR_BREDR)
>> +             conn =3D hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>> +                                            &cp->addr.bdaddr);
>> +     else
>> +             conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->addr.=
bdaddr);
>> +
>> +     if (!conn || conn->state =3D=3D BT_OPEN || conn->state =3D=3D BT_C=
LOSED) {
>
> Lets check for !=3D BT_CONNECTED here.
>
>> +             err =3D cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORM=
ATION,
>> +                                MGMT_STATUS_NOT_CONNECTED, &rp, sizeof(=
rp));
>> +             goto unlock;
>> +     }
>> +
>> +     is_pending =3D check_pending_conn_info(hdev, conn);
>
> Don=E2=80=99t we have a mgmt_pending_find for this.

We have but there's similar problem as with hci_request in other patch
- we want pending command for given hci_conn. So this actually does
something like mgmt_pending_find_if (with cmp callback). Do you want
to have such generic function instead of dedicated helper?

BR,
Andrzej

[PATCH 6/8] Bluetooth: Make min interval for connection information configurable

[Andrzej Kaczmarek]

This patch adds debugfs entry to configure minimum interval between
refreshing data using Get Connection Information request. Default value
is 2000ms.

Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
---
 include/net/bluetooth/hci_core.h | 3 +++
 net/bluetooth/hci_core.c         | 4 ++++
 net/bluetooth/mgmt.c             | 5 ++---
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8d7d48b..a1eb022 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -145,6 +145,8 @@ struct oob_data {
 /* Default LE RPA expiry time, 15 minutes */
 #define HCI_DEFAULT_RPA_TIMEOUT		(15 * 60)
 
+#define DEFAULT_CONN_INFO_INTERVAL	2000
+
 struct amp_assoc {
 	__u16	len;
 	__u16	offset;
@@ -200,6 +202,7 @@ struct hci_dev {
 	__u16		le_conn_min_interval;
 	__u16		le_conn_max_interval;
 	__u16		discov_interleaved_timeout;
+	__u16		conn_info_interval;
 	__u8		ssp_debug_mode;
 
 	__u16		devid_source;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f5dcbb1..611f707 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1754,6 +1754,9 @@ static int __hci_init(struct hci_dev *hdev)
 			    &blacklist_fops);
 	debugfs_create_file("uuids", 0444, hdev->debugfs, hdev, &uuids_fops);
 
+	debugfs_create_u16("conn_info_interval", 0644, hdev->debugfs,
+			   &hdev->conn_info_interval);
+
 	if (lmp_bredr_capable(hdev)) {
 		debugfs_create_file("inquiry_cache", 0444, hdev->debugfs,
 				    hdev, &inquiry_cache_fops);
@@ -3790,6 +3793,7 @@ struct hci_dev *hci_alloc_dev(void)
 
 	hdev->rpa_timeout = HCI_DEFAULT_RPA_TIMEOUT;
 	hdev->discov_interleaved_timeout = DISCOV_INTERLEAVED_TIMEOUT;
+	hdev->conn_info_interval = DEFAULT_CONN_INFO_INTERVAL;
 
 	mutex_init(&hdev->lock);
 	mutex_init(&hdev->req_lock);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 58074c8..e84adf9 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -114,8 +114,6 @@ static const u16 mgmt_events[] = {
 
 #define CACHE_TIMEOUT	msecs_to_jiffies(2 * 1000)
 
-#define CONN_INFO_INTERVAL	msecs_to_jiffies(2 * 1000)
-
 #define hdev_is_powered(hdev) (test_bit(HCI_UP, &hdev->flags) && \
 				!test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
 
@@ -4718,7 +4716,8 @@ static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	hci_req_init(&req, hdev, conn);
 
-	if (time_after(jiffies, conn->last_info_read + CONN_INFO_INTERVAL) ||
+	if (time_after(jiffies, conn->last_info_read +
+		       msecs_to_jiffies(hdev->conn_info_interval)) ||
 	    conn->tx_power == HCI_TX_POWER_INVALID) {
 		struct hci_cp_read_rssi req_rssi_cp;
 		struct hci_cp_read_tx_power_level req_txp_cp;
-- 
1.9.2

Re: [PATCH 6/8] Bluetooth: Make min interval for connection information configurable

[Marcel Holtmann]

Hi Andrzej,

> This patch adds debugfs entry to configure minimum interval between
> refreshing data using Get Connection Information request. Default value
> is 2000ms.
> 
> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
> ---
> include/net/bluetooth/hci_core.h | 3 +++
> net/bluetooth/hci_core.c         | 4 ++++
> net/bluetooth/mgmt.c             | 5 ++---
> 3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 8d7d48b..a1eb022 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -145,6 +145,8 @@ struct oob_data {
> /* Default LE RPA expiry time, 15 minutes */
> #define HCI_DEFAULT_RPA_TIMEOUT		(15 * 60)
> 
> +#define DEFAULT_CONN_INFO_INTERVAL	2000
> +
> struct amp_assoc {
> 	__u16	len;
> 	__u16	offset;
> @@ -200,6 +202,7 @@ struct hci_dev {
> 	__u16		le_conn_min_interval;
> 	__u16		le_conn_max_interval;
> 	__u16		discov_interleaved_timeout;
> +	__u16		conn_info_interval;
> 	__u8		ssp_debug_mode;

the name should be either conn_info_max_age or conn_info_lifetime. It is not an interval since we are not the ones doing the polling.

Regards

Marcel

[PATCH 7/8] Bluetooth: Avoid pooling TX power for LE links

[Andrzej Kaczmarek]

TX power for LE links is immutable thus we do not need to query for it
if already have value.

Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
---
 net/bluetooth/mgmt.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e84adf9..3f6c81a 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4726,10 +4726,16 @@ static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *data,
 		hci_req_add(&req, HCI_OP_READ_RSSI, sizeof(req_rssi_cp),
 			    &req_rssi_cp);
 
-		req_txp_cp.handle = cpu_to_le16(conn->handle);
-		req_txp_cp.type = TX_POWER_TYPE_CURRENT;
-		hci_req_add(&req, HCI_OP_READ_TX_POWER_LEVEL,
-			    sizeof(req_txp_cp), &req_txp_cp);
+		/* For LE links TX power is immutable so we don't need to query
+		 * controller again once value is known.
+		 */
+		if (!bdaddr_type_is_le(cp->addr.type) ||
+		    conn->tx_power == HCI_TX_POWER_INVALID) {
+			req_txp_cp.handle = cpu_to_le16(conn->handle);
+			req_txp_cp.type = TX_POWER_TYPE_CURRENT;
+			hci_req_add(&req, HCI_OP_READ_TX_POWER_LEVEL,
+				    sizeof(req_txp_cp), &req_txp_cp);
+		}
 
 		err = hci_req_run(&req, conn_info_query_complete);
 		if (err < 0) {
-- 
1.9.2

Re: [PATCH 7/8] Bluetooth: Avoid pooling TX power for LE links

[Marcel Holtmann]

Hi Andrzej,

> TX power for LE links is immutable thus we do not need to query for it
> if already have value.

can you include the part from the specification that clarifies this.

> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
> ---
> net/bluetooth/mgmt.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index e84adf9..3f6c81a 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4726,10 +4726,16 @@ static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *data,
> 		hci_req_add(&req, HCI_OP_READ_RSSI, sizeof(req_rssi_cp),
> 			    &req_rssi_cp);
> 
> -		req_txp_cp.handle = cpu_to_le16(conn->handle);
> -		req_txp_cp.type = TX_POWER_TYPE_CURRENT;
> -		hci_req_add(&req, HCI_OP_READ_TX_POWER_LEVEL,
> -			    sizeof(req_txp_cp), &req_txp_cp);
> +		/* For LE links TX power is immutable so we don't need to query
> +		 * controller again once value is known.
> +		 */
> +		if (!bdaddr_type_is_le(cp->addr.type) ||
> +		    conn->tx_power == HCI_TX_POWER_INVALID) {
> +			req_txp_cp.handle = cpu_to_le16(conn->handle);
> +			req_txp_cp.type = TX_POWER_TYPE_CURRENT;
> +			hci_req_add(&req, HCI_OP_READ_TX_POWER_LEVEL,
> +				    sizeof(req_txp_cp), &req_txp_cp);
> +		}

Regards

Marcel

Re: [PATCH 7/8] Bluetooth: Avoid pooling TX power for LE links

[Andrzej Kaczmarek]

Hi Marcel,

On 8 May 2014 17:29, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andrzej,
>
>> TX power for LE links is immutable thus we do not need to query for it
>> if already have value.
>
> can you include the part from the specification that clarifies this.

I'm not sure there's quote in spec that says this explicitly... iirc
there's explicit statement that it can be controlled on physical link
by LM for BR/EDR, but for LE I could not find such information. I
assume it does not change as otherwise TPS spec does not make sense.
But does my reasoning make sense? :-)

BR,
Andrzej

[PATCH 8/8] Bluetooth: Add support for max_tx_power in Get Conn Info

[Andrzej Kaczmarek]

This patch adds support for max_tx_power in Get Connection Information
request. Value is read only once for given connection and then always
returned in response as parameter.

Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
---
 include/net/bluetooth/hci_core.h |  1 +
 include/net/bluetooth/mgmt.h     |  1 +
 net/bluetooth/hci_conn.c         |  1 +
 net/bluetooth/hci_event.c        | 18 +++++++++++++++++-
 net/bluetooth/mgmt.c             |  8 ++++++++
 5 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a1eb022..b0f958a 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -379,6 +379,7 @@ struct hci_conn {
 	__u16		le_conn_max_interval;
 	__s8		rssi;
 	__s8		tx_power;
+	__s8		max_tx_power;
 	unsigned long	flags;
 
 	unsigned long	last_info_read;
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index c26b7e2..ce7315d 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -420,6 +420,7 @@ struct mgmt_cp_get_conn_information {
 struct mgmt_rp_get_conn_information {
 	struct mgmt_addr_info addr;
 	__s8	rssi;
+	__s8	max_tx_power;
 	__u32	flags;
 	__le16	eir_len;
 	__u8	eir[0];
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 78cfe3b..05125b9 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -408,6 +408,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
 	conn->remote_auth = 0xff;
 	conn->key_type = 0xff;
 	conn->tx_power = HCI_TX_POWER_INVALID;
+	conn->max_tx_power = HCI_TX_POWER_INVALID;
 
 	set_bit(HCI_CONN_POWER_SAVE, &conn->flags);
 	conn->disc_timeout = HCI_DISCONN_TIMEOUT;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0e67c8e..a80b51b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1283,9 +1283,25 @@ static void hci_cc_read_tx_power_level(struct hci_dev *hdev,
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(rp->handle));
-	if (conn && sent->type == TX_POWER_TYPE_CURRENT)
+	if (!conn)
+		goto unlock;
+
+	switch (sent->type) {
+	case TX_POWER_TYPE_CURRENT:
 		conn->tx_power = rp->tx_power_level;
+		break;
+
+	case TX_POWER_TYPE_MAXIMUM:
+		conn->max_tx_power = rp->tx_power_level;
+		break;
 
+	default:
+		BT_ERR("Used reserved Read_Transmit_Power_Level param %d",
+		       sent->type);
+		break;
+	}
+
+unlock:
 	hci_dev_unlock(hdev);
 }
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3f6c81a..81e412f 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4605,6 +4605,7 @@ static void conn_info_complete(struct pending_cmd *cmd, void *data)
 	}
 
 	rp->rssi = conn->rssi;
+	rp->max_tx_power = conn->max_tx_power;
 
 	if (cp->data_type & MGMT_CONN_INFO_DATA_TX_POWER) {
 		eir_len = eir_append_data(rp->eir, 0, EIR_TX_POWER,
@@ -4737,6 +4738,13 @@ static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *data,
 				    sizeof(req_txp_cp), &req_txp_cp);
 		}
 
+		if (conn->max_tx_power == HCI_TX_POWER_INVALID) {
+			req_txp_cp.handle = cpu_to_le16(conn->handle);
+			req_txp_cp.type = TX_POWER_TYPE_MAXIMUM;
+			hci_req_add(&req, HCI_OP_READ_TX_POWER_LEVEL,
+				    sizeof(req_txp_cp), &req_txp_cp);
+		}
+
 		err = hci_req_run(&req, conn_info_query_complete);
 		if (err < 0) {
 			hci_conn_drop(conn);
-- 
1.9.2