[PATCH 0/7] Bluetooth: Improved single-mode and LE peripheral support

[PATCH 0/7] Bluetooth: Improved single-mode and LE peripheral support

[Johan Hedberg]

Hi,

This set of patches builds on top of my previous one. It introduces a
few more fixes for single-mode controllers as well as adds a new feature
to the management interface to switch a controller to LE peripheral
mode.  User space already has code to support this and you can enable
the new mode using "tools/btmgmt le peripheral".

Johan

[PATCH 1/7] Bluetooth: mgmt: Add support for switching to LE peripheral mode

[Johan Hedberg]

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

This patch extends the mgmt_set_le command to allow for a new value
(0x02) to mean that LE should be enabled together with advertising
enabled. This paves the way for acting in a fully functional LE
peripheral role. The change to the mgmt_set_le command is fully
backwards compatible as the value 0x01 means LE central role (which was
the old behavior).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci.h      |    3 +
 include/net/bluetooth/hci_core.h |    1 +
 include/net/bluetooth/mgmt.h     |    6 ++
 net/bluetooth/hci_event.c        |   44 +++++++++++
 net/bluetooth/mgmt.c             |  159 +++++++++++++++++++++++++++++++-------
 5 files changed, 185 insertions(+), 28 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 348f4bf..a633da0 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -115,6 +115,7 @@ enum {
 	HCI_SSP_ENABLED,
 	HCI_HS_ENABLED,
 	HCI_LE_ENABLED,
+	HCI_LE_PERIPHERAL,
 	HCI_CONNECTABLE,
 	HCI_DISCOVERABLE,
 	HCI_LINK_SECURITY,
@@ -938,6 +939,8 @@ struct hci_rp_le_read_adv_tx_power {
 	__s8	tx_power;
 } __packed;
 
+#define HCI_OP_LE_SET_ADV_ENABLE	0x200a
+
 #define HCI_OP_LE_SET_SCAN_PARAM	0x200b
 struct hci_cp_le_set_scan_param {
 	__u8    type;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8260bf2..f288a8c 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1075,6 +1075,7 @@ int mgmt_set_local_name_complete(struct hci_dev *hdev, u8 *name, u8 status);
 int mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8 *hash,
 					    u8 *randomizer, u8 status);
 int mgmt_le_enable_complete(struct hci_dev *hdev, u8 enable, u8 status);
+int mgmt_le_adv_enable_complete(struct hci_dev *hdev, u8 enable, u8 status);
 int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 		      u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name,
 		      u8 ssp, u8 *eir, u16 eir_len);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 22980a7..26c9a4d 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -92,6 +92,7 @@ struct mgmt_rp_read_index_list {
 #define MGMT_SETTING_BREDR		0x00000080
 #define MGMT_SETTING_HS			0x00000100
 #define MGMT_SETTING_LE			0x00000200
+#define MGMT_SETTING_PERIPHERAL		0x00000400
 
 #define MGMT_OP_READ_INFO		0x0004
 #define MGMT_READ_INFO_SIZE		0
@@ -134,6 +135,11 @@ struct mgmt_cp_set_discoverable {
 #define MGMT_OP_SET_HS			0x000C
 
 #define MGMT_OP_SET_LE			0x000D
+
+#define MGMT_LE_OFF			0x00
+#define MGMT_LE_CENTRAL			0x01
+#define MGMT_LE_PERIPHERAL		0x02
+
 #define MGMT_OP_SET_DEV_CLASS		0x000E
 struct mgmt_cp_set_dev_class {
 	__u8	major;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index fd5a51c..0393b34 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -786,6 +786,9 @@ static void hci_set_le_support(struct hci_dev *hdev)
 	if (cp.le != !!(hdev->host_features[0] & LMP_HOST_LE))
 		hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
 			     &cp);
+	else if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
+		hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE, sizeof(cp.le),
+			     &cp.le);
 }
 
 static void hci_cc_read_local_ext_features(struct hci_dev *hdev,
@@ -1189,6 +1192,38 @@ static void hci_cc_le_set_scan_param(struct hci_dev *hdev, struct sk_buff *skb)
 	}
 }
 
+static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	__u8 *sent, status = *((__u8 *) skb->data);
+
+	BT_DBG("%s status 0x%2.2x", hdev->name, status);
+
+	sent = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_ADV_ENABLE);
+	if (!sent)
+		return;
+
+	hci_dev_lock(hdev);
+
+	if (!status) {
+		if (*sent)
+			set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
+		else
+			clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
+	} else {
+		if (*sent)
+			clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
+		else
+			set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
+	}
+
+	if (!test_bit(HCI_INIT, &hdev->flags))
+		mgmt_le_adv_enable_complete(hdev, *sent, status);
+
+	hci_dev_unlock(hdev);
+
+	hci_req_complete(hdev, HCI_OP_LE_SET_ADV_ENABLE, status);
+}
+
 static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 				      struct sk_buff *skb)
 {
@@ -1287,6 +1322,11 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
 			hdev->host_features[0] |= LMP_HOST_LE;
 		else
 			hdev->host_features[0] &= ~LMP_HOST_LE;
+
+		if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags) &&
+		    test_bit(HCI_INIT, &hdev->flags))
+			hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
+				     sizeof(sent->le), &sent->le);
 	}
 
 	if (test_bit(HCI_MGMT, &hdev->dev_flags) &&
@@ -2549,6 +2589,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cc_le_set_scan_param(hdev, skb);
 		break;
 
+	case HCI_OP_LE_SET_ADV_ENABLE:
+		hci_cc_le_set_adv_enable(hdev, skb);
+		break;
+
 	case HCI_OP_LE_SET_SCAN_ENABLE:
 		hci_cc_le_set_scan_enable(hdev, skb);
 		break;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fe74f4..1d79979 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -393,8 +393,10 @@ static u32 get_supported_settings(struct hci_dev *hdev)
 	if (enable_hs)
 		settings |= MGMT_SETTING_HS;
 
-	if (lmp_le_capable(hdev))
+	if (lmp_le_capable(hdev)) {
 		settings |= MGMT_SETTING_LE;
+		settings |= MGMT_SETTING_PERIPHERAL;
+	}
 
 	return settings;
 }
@@ -418,9 +420,13 @@ static u32 get_current_settings(struct hci_dev *hdev)
 	if (lmp_bredr_capable(hdev))
 		settings |= MGMT_SETTING_BREDR;
 
-	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
+	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
 		settings |= MGMT_SETTING_LE;
 
+		if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
+			settings |= MGMT_SETTING_PERIPHERAL;
+	}
+
 	if (test_bit(HCI_LINK_SECURITY, &hdev->dev_flags))
 		settings |= MGMT_SETTING_LINK_SECURITY;
 
@@ -1195,13 +1201,36 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 	return send_settings_rsp(sk, MGMT_OP_SET_HS, hdev);
 }
 
+static u8 get_le_mode(struct hci_dev *hdev)
+{
+	if (hdev->host_features[0] & LMP_HOST_LE) {
+		if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
+			return MGMT_LE_PERIPHERAL;
+		return MGMT_LE_CENTRAL;
+	}
+
+	return MGMT_LE_OFF;
+}
+
+static bool valid_le_mode(u8 mode)
+{
+	switch (mode) {
+	case MGMT_LE_OFF:
+	case MGMT_LE_CENTRAL:
+	case MGMT_LE_PERIPHERAL:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 {
-	struct mgmt_mode *cp = data;
 	struct hci_cp_write_le_host_supported hci_cp;
+	struct mgmt_mode *cp = data;
 	struct pending_cmd *cmd;
+	u8 old_mode, new_mode;
 	int err;
-	u8 val, enabled;
 
 	BT_DBG("request for %s", hdev->name);
 
@@ -1213,48 +1242,71 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 		goto unlock;
 	}
 
-	val = !!cp->val;
-	enabled = !!(hdev->host_features[0] & LMP_HOST_LE);
+	if (!valid_le_mode(cp->val)) {
+		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
+				 MGMT_STATUS_INVALID_PARAMS);
+		goto unlock;
+	}
 
-	if (!hdev_is_powered(hdev) || val == enabled) {
-		bool changed = false;
+	if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
+		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
+				 MGMT_STATUS_BUSY);
+		goto unlock;
+	}
+
+	new_mode = cp->val;
+	old_mode = get_le_mode(hdev);
+
+	BT_DBG("%s old_mode %u new_mode %u", hdev->name, old_mode, new_mode);
+
+	if (new_mode == old_mode) {
+		err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
+		goto unlock;
+	}
+
+	if (old_mode == MGMT_LE_PERIPHERAL || new_mode == MGMT_LE_PERIPHERAL)
+		change_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
 
-		if (val != test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
+	if (!hdev_is_powered(hdev)) {
+		if (old_mode == MGMT_LE_OFF || new_mode == MGMT_LE_OFF)
 			change_bit(HCI_LE_ENABLED, &hdev->dev_flags);
-			changed = true;
-		}
 
 		err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
 		if (err < 0)
 			goto unlock;
 
-		if (changed)
-			err = new_settings(hdev, sk);
+		err = new_settings(hdev, sk);
 
 		goto unlock;
 	}
 
-	if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
-		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
-				 MGMT_STATUS_BUSY);
-		goto unlock;
-	}
-
 	cmd = mgmt_pending_add(sk, MGMT_OP_SET_LE, hdev, data, len);
 	if (!cmd) {
 		err = -ENOMEM;
 		goto unlock;
 	}
 
+	if ((old_mode != MGMT_LE_OFF && new_mode == MGMT_LE_PERIPHERAL) ||
+	    old_mode == MGMT_LE_PERIPHERAL) {
+		u8 adv_enable = (new_mode == MGMT_LE_PERIPHERAL);
+
+		err = hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
+				   sizeof(adv_enable), &adv_enable);
+		if (err < 0)
+			mgmt_pending_remove(cmd);
+
+		goto unlock;
+	}
+
 	memset(&hci_cp, 0, sizeof(hci_cp));
 
-	if (val) {
-		hci_cp.le = val;
+	if (old_mode == MGMT_LE_OFF) {
+		hci_cp.le = 1;
 		hci_cp.simul = !!(hdev->features[6] & LMP_SIMUL_LE_BR);
 	}
 
-	err = hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(hci_cp),
-			   &hci_cp);
+	err = hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED,
+			   sizeof(hci_cp), &hci_cp);
 	if (err < 0)
 		mgmt_pending_remove(cmd);
 
@@ -3525,7 +3577,8 @@ int mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8 *hash,
 
 int mgmt_le_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
 {
-	struct cmd_lookup match = { NULL, hdev };
+	struct pending_cmd *cmd;
+	struct mgmt_mode *cp;
 	bool changed = false;
 	int err = 0;
 
@@ -3550,17 +3603,67 @@ int mgmt_le_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
 			changed = true;
 	}
 
-	mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, settings_rsp, &match);
+	cmd = mgmt_pending_find(MGMT_OP_SET_LE, hdev);
+	if (!cmd)
+		goto done;
+
+	cp = cmd->param;
+	if (enable && cp->val == MGMT_LE_PERIPHERAL)
+		return hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
+				    sizeof(enable), &enable);
 
+	send_settings_rsp(cmd->sk, cmd->opcode, hdev);
+	list_del(&cmd->list);
+
+done:
 	if (changed)
-		err = new_settings(hdev, match.sk);
+		err = new_settings(hdev, cmd ? cmd->sk : NULL);
 
-	if (match.sk)
-		sock_put(match.sk);
+	if (cmd)
+		mgmt_pending_free(cmd);
 
 	return err;
 }
 
+int mgmt_le_adv_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
+{
+	struct pending_cmd *cmd;
+	struct mgmt_mode *cp;
+
+	if (status) {
+		u8 mgmt_err = mgmt_status(status);
+
+		mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, cmd_status_rsp,
+				     &mgmt_err);
+
+		return 0;
+	}
+
+	cmd = mgmt_pending_find(MGMT_OP_SET_LE, hdev);
+	if (!cmd) {
+		new_settings(hdev, NULL);
+		return 0;
+	}
+
+	cp = cmd->param;
+	if (!enable && cp->val == MGMT_LE_OFF) {
+		struct hci_cp_write_le_host_supported hci_cp;
+
+		memset(&hci_cp, 0, sizeof(hci_cp));
+
+		return hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED,
+				    sizeof(hci_cp), &hci_cp);
+	}
+
+	new_settings(hdev, cmd->sk);
+	send_settings_rsp(cmd->sk, cmd->opcode, hdev);
+
+	list_del(&cmd->list);
+	mgmt_pending_free(cmd);
+
+	return 0;
+}
+
 int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 		      u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name, u8
 		      ssp, u8 *eir, u16 eir_len)
-- 
1.7.10.4

[PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters

[Johan Hedberg]

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

This patch makes sure that settings which are specific for BR/EDR
capable adapters are not allowed for non-BR/EDR (e.g. LE-only) adapters.
Instead, a "not supported" error is returned of such a setting is
attempted to be set for a non-BR/EDR adapter.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1d79979..eaa6fdc 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -377,15 +377,15 @@ static u32 get_supported_settings(struct hci_dev *hdev)
 	u32 settings = 0;
 
 	settings |= MGMT_SETTING_POWERED;
-	settings |= MGMT_SETTING_CONNECTABLE;
-	settings |= MGMT_SETTING_FAST_CONNECTABLE;
-	settings |= MGMT_SETTING_DISCOVERABLE;
 	settings |= MGMT_SETTING_PAIRABLE;
 
 	if (lmp_ssp_capable(hdev))
 		settings |= MGMT_SETTING_SSP;
 
 	if (lmp_bredr_capable(hdev)) {
+		settings |= MGMT_SETTING_CONNECTABLE;
+		settings |= MGMT_SETTING_FAST_CONNECTABLE;
+		settings |= MGMT_SETTING_DISCOVERABLE;
 		settings |= MGMT_SETTING_BREDR;
 		settings |= MGMT_SETTING_LINK_SECURITY;
 	}
@@ -874,6 +874,10 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("request for %s", hdev->name);
 
+	if (!lmp_bredr_capable(hdev))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
+				 MGMT_STATUS_NOT_SUPPORTED);
+
 	timeout = __le16_to_cpu(cp->timeout);
 	if (!cp->val && timeout > 0)
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
@@ -969,6 +973,10 @@ static int set_connectable(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("request for %s", hdev->name);
 
+	if (!lmp_bredr_capable(hdev))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_CONNECTABLE,
+				  MGMT_STATUS_NOT_SUPPORTED);
+
 	hci_dev_lock(hdev);
 
 	if (!hdev_is_powered(hdev)) {
@@ -1067,6 +1075,10 @@ static int set_link_security(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("request for %s", hdev->name);
 
+	if (!lmp_bredr_capable(hdev))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
+				  MGMT_STATUS_NOT_SUPPORTED);
+
 	hci_dev_lock(hdev);
 
 	if (!hdev_is_powered(hdev)) {
@@ -2647,6 +2659,10 @@ static int set_fast_connectable(struct sock *sk, struct hci_dev *hdev,
 
 	BT_DBG("%s", hdev->name);
 
+	if (!lmp_bredr_capable(hdev))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
+				  MGMT_STATUS_NOT_SUPPORTED);
+
 	if (!hdev_is_powered(hdev))
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
 				  MGMT_STATUS_NOT_POWERED);
-- 
1.7.10.4

[PATCH 3/7] Bluetooth: Disallow LE scanning and connecting in peripheral mode

[Johan Hedberg]

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

When an adapter is in the LE peripheral mode scanning for other devices
or initiating connections to them is not allowed. This patch makes sure
that such attempts will result in appropriate error returns.

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

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index fe64621..a75b30e 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -502,6 +502,9 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 {
 	struct hci_conn *le;
 
+	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
+		return ERR_PTR(-ENOTSUPP);
+
 	le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
 	if (!le) {
 		le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 487308b..b2adef4 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1575,6 +1575,9 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
 
 	BT_DBG("%s", hdev->name);
 
+	if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
+		return -ENOTSUPP;
+
 	if (work_busy(&hdev->le_scan))
 		return -EINPROGRESS;
 
-- 
1.7.10.4

[PATCH 4/7] Bluetooth: Add support for setting LE advertising data

[Johan Hedberg]

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

This patch adds support for setting basing LE advertising data. The
three elements supported for now are the advertising flags, the TX power
and the friendly name.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci.h      |   15 ++++++
 include/net/bluetooth/hci_core.h |    4 ++
 net/bluetooth/hci_event.c        |    9 ++++
 net/bluetooth/mgmt.c             |   97 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index a633da0..f850e94 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -335,6 +335,13 @@ enum {
 #define EIR_SSP_RAND_R		0x0F /* Simple Pairing Randomizer R */
 #define EIR_DEVICE_ID		0x10 /* device ID */
 
+/* Low Energy Advertising Flags */
+#define LE_AD_LIMITED		0x01 /* Limited Discoverable */
+#define LE_AD_GENERAL		0x02 /* General Discoverable */
+#define LE_AD_NO_BREDR		0x04 /* BR/EDR not supported */
+#define LE_AD_SIM_LE_BREDR_CTRL	0x08 /* Simultaneous LE & BR/EDR Controller */
+#define LE_AD_SIM_LE_BREDR_HOST	0x10 /* Simultaneous LE & BR/EDR Host */
+
 /* -----  HCI Commands ---- */
 #define HCI_OP_NOP			0x0000
 
@@ -939,6 +946,14 @@ struct hci_rp_le_read_adv_tx_power {
 	__s8	tx_power;
 } __packed;
 
+#define HCI_MAX_AD_LENGTH		31
+
+#define HCI_OP_LE_SET_ADV_DATA		0x2008
+struct hci_cp_le_set_adv_data {
+	__u8		length;
+	__u8		data[HCI_MAX_AD_LENGTH];
+} __packed;
+
 #define HCI_OP_LE_SET_ADV_ENABLE	0x200a
 
 #define HCI_OP_LE_SET_SCAN_PARAM	0x200b
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index f288a8c..3ad3281 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -279,6 +279,8 @@ struct hci_dev {
 	struct le_scan_params	le_scan_params;
 
 	__s8			adv_tx_power;
+	__u8			adv_data[HCI_MAX_AD_LENGTH];
+	__u8			adv_data_len;
 
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
@@ -758,9 +760,11 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
 #define lmp_no_flush_capable(dev)  ((dev)->features[6] & LMP_NO_FLUSH)
 #define lmp_le_capable(dev)        ((dev)->features[4] & LMP_LE)
 #define lmp_bredr_capable(dev)     (!((dev)->features[4] & LMP_NO_BREDR))
+#define lmp_le_br_capable(dev)     ((dev)->features[6] & LMP_SIMUL_LE_BR)
 
 /* ----- Extended LMP capabilities ----- */
 #define lmp_host_le_capable(dev)   ((dev)->host_features[0] & LMP_HOST_LE)
+#define lmp_host_le_br_capable(dev) ((dev)->host_features[0] & LMP_HOST_LE_BREDR)
 
 /* ----- HCI protocols ----- */
 static inline int hci_proto_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr,
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0393b34..abad39c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1089,6 +1089,15 @@ static void hci_cc_le_read_adv_tx_power(struct hci_dev *hdev,
 	if (!rp->status)
 		hdev->adv_tx_power = rp->tx_power;
 
+	if (hdev->adv_data_len > 0) {
+		struct hci_cp_le_set_adv_data cp;
+
+		memcpy(cp.data, hdev->adv_data, sizeof(cp.data));
+		cp.length = cpu_to_le16(hdev->adv_data_len);
+
+		hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_DATA, sizeof(cp), &cp);
+	}
+
 	hci_req_complete(hdev, HCI_OP_LE_READ_ADV_TX_POWER, rp->status);
 }
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index eaa6fdc..fe77f2c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -593,6 +593,93 @@ static int update_eir(struct hci_dev *hdev)
 	return hci_send_cmd(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp);
 }
 
+static u16 create_ad(struct hci_dev *hdev, u8 *data)
+{
+	u8 *ptr = data;
+	u16 ad_len = 0;
+	size_t name_len;
+	u8 flags = 0;
+
+	if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
+		flags |= LE_AD_GENERAL;
+
+	if (!lmp_bredr_capable(hdev))
+		flags |= LE_AD_NO_BREDR;
+
+	if (lmp_le_br_capable(hdev))
+		flags |= LE_AD_SIM_LE_BREDR_CTRL;
+
+	if (lmp_host_le_br_capable(hdev))
+		flags |= LE_AD_SIM_LE_BREDR_HOST;
+
+	if (flags) {
+		BT_DBG("adv flags 0x%02x", flags);
+
+		ptr[0] = 2;
+		ptr[1] = EIR_FLAGS;
+		ptr[2] = flags;
+
+		ad_len += 3;
+		ptr += 3;
+	}
+
+	if (hdev->adv_tx_power) {
+		ptr[0] = 2;
+		ptr[1] = EIR_TX_POWER;
+		ptr[2] = (u8) hdev->adv_tx_power;
+
+		ad_len += 3;
+		ptr += 3;
+	}
+
+	name_len = strlen(hdev->dev_name);
+	if (name_len > 0) {
+		size_t max_len = HCI_MAX_AD_LENGTH - ad_len - 2;
+
+		if (name_len > max_len) {
+			name_len = max_len;
+			ptr[1] = EIR_NAME_SHORT;
+		} else
+			ptr[1] = EIR_NAME_COMPLETE;
+
+		ptr[0] = name_len + 1;
+
+		memcpy(ptr + 2, hdev->dev_name, name_len);
+
+		ad_len += (name_len + 2);
+		ptr += (name_len + 2);
+	}
+
+	return ad_len;
+}
+
+static int update_ad(struct hci_dev *hdev)
+{
+	struct hci_cp_le_set_adv_data cp;
+	u16 len;
+
+	if (!hdev_is_powered(hdev))
+		return 0;
+
+	if (!lmp_le_capable(hdev))
+		return 0;
+
+	memset(&cp, 0, sizeof(cp));
+
+	len = create_ad(hdev, cp.data);
+
+	if (hdev->adv_data_len == len &&
+	    memcmp(cp.data, hdev->adv_data, len) == 0)
+		return 0;
+
+	memcpy(hdev->adv_data, cp.data, sizeof(cp.data));
+	hdev->adv_data_len = len;
+
+	cp.length = cpu_to_le16(len);
+
+	return hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_DATA, sizeof(cp), &cp);
+}
+
 static u8 get_service_classes(struct hci_dev *hdev)
 {
 	struct bt_uuid *uuid;
@@ -1276,8 +1363,10 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 		goto unlock;
 	}
 
-	if (old_mode == MGMT_LE_PERIPHERAL || new_mode == MGMT_LE_PERIPHERAL)
+	if (old_mode == MGMT_LE_PERIPHERAL || new_mode == MGMT_LE_PERIPHERAL) {
 		change_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
+		update_ad(hdev);
+	}
 
 	if (!hdev_is_powered(hdev)) {
 		if (old_mode == MGMT_LE_OFF || new_mode == MGMT_LE_OFF)
@@ -2972,6 +3061,7 @@ int mgmt_powered(struct hci_dev *hdev, u8 powered)
 			update_name(hdev, hdev->dev_name);
 			update_eir(hdev);
 		}
+		update_ad(hdev);
 	} else {
 		u8 status = MGMT_STATUS_NOT_POWERED;
 		mgmt_pending_foreach(0, hdev, cmd_status_rsp, &status);
@@ -3553,6 +3643,7 @@ send_event:
 				 sizeof(ev), cmd ? cmd->sk : NULL);
 
 	update_eir(hdev);
+	update_ad(hdev);
 
 failed:
 	if (cmd)
@@ -3627,6 +3718,8 @@ int mgmt_le_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
 	if (enable && cp->val == MGMT_LE_PERIPHERAL)
 		return hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
 				    sizeof(enable), &enable);
+	else
+		update_ad(hdev);
 
 	send_settings_rsp(cmd->sk, cmd->opcode, hdev);
 	list_del(&cmd->list);
@@ -3652,6 +3745,8 @@ int mgmt_le_adv_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
 		mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, cmd_status_rsp,
 				     &mgmt_err);
 
+		update_ad(hdev);
+
 		return 0;
 	}
 
-- 
1.7.10.4

[PATCH 5/7] Bluetooth: Fix updating host feature bits for LE

[Johan Hedberg]

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

When LE has been enabled with the simultaneous BR/EDR & LE parameter set
to true we should also update the host features stored in struct hci_dev
accordingly.

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

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index abad39c..b63a5f3 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1332,6 +1332,11 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
 		else
 			hdev->host_features[0] &= ~LMP_HOST_LE;
 
+		if (sent->simul)
+			hdev->host_features[0] |= LMP_HOST_LE_BREDR;
+		else
+			hdev->host_features[0] &= ~LMP_HOST_LE_BREDR;
+
 		if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags) &&
 		    test_bit(HCI_INIT, &hdev->flags))
 			hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
-- 
1.7.10.4

[PATCH 6/7] Bluetooth: Sort feature test macros by bitmask location

[Johan Hedberg]

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

This patch sorts the feature test marcos according to the location of
each feature bit in the features bit mask. This helps easy lookup when
adding new marcos and wanting to check if there already is a macro

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3ad3281..006aca7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -751,16 +751,16 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
 #define SET_HCIDEV_DEV(hdev, pdev) ((hdev)->dev.parent = (pdev))
 
 /* ----- LMP capabilities ----- */
-#define lmp_rswitch_capable(dev)   ((dev)->features[0] & LMP_RSWITCH)
 #define lmp_encrypt_capable(dev)   ((dev)->features[0] & LMP_ENCRYPT)
+#define lmp_rswitch_capable(dev)   ((dev)->features[0] & LMP_RSWITCH)
 #define lmp_sniff_capable(dev)     ((dev)->features[0] & LMP_SNIFF)
-#define lmp_sniffsubr_capable(dev) ((dev)->features[5] & LMP_SNIFF_SUBR)
 #define lmp_esco_capable(dev)      ((dev)->features[3] & LMP_ESCO)
-#define lmp_ssp_capable(dev)       ((dev)->features[6] & LMP_SIMPLE_PAIR)
-#define lmp_no_flush_capable(dev)  ((dev)->features[6] & LMP_NO_FLUSH)
-#define lmp_le_capable(dev)        ((dev)->features[4] & LMP_LE)
 #define lmp_bredr_capable(dev)     (!((dev)->features[4] & LMP_NO_BREDR))
+#define lmp_le_capable(dev)        ((dev)->features[4] & LMP_LE)
+#define lmp_sniffsubr_capable(dev) ((dev)->features[5] & LMP_SNIFF_SUBR)
 #define lmp_le_br_capable(dev)     ((dev)->features[6] & LMP_SIMUL_LE_BR)
+#define lmp_ssp_capable(dev)       ((dev)->features[6] & LMP_SIMPLE_PAIR)
+#define lmp_no_flush_capable(dev)  ((dev)->features[6] & LMP_NO_FLUSH)
 
 /* ----- Extended LMP capabilities ----- */
 #define lmp_host_le_capable(dev)   ((dev)->host_features[0] & LMP_HOST_LE)
-- 
1.7.10.4

[PATCH 7/7] Bluetooth: Make use feature test macros

[Johan Hedberg]

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

For better code readability and avoiding simple bugs of checking the
wrong byte of the features make use of feature test macros whenever
possible.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |    8 ++++++++
 net/bluetooth/hci_event.c        |   26 +++++++++++++-------------
 net/bluetooth/mgmt.c             |    8 ++++----
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 006aca7..d447f45fa 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -753,14 +753,22 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
 /* ----- LMP capabilities ----- */
 #define lmp_encrypt_capable(dev)   ((dev)->features[0] & LMP_ENCRYPT)
 #define lmp_rswitch_capable(dev)   ((dev)->features[0] & LMP_RSWITCH)
+#define lmp_hold_capable(dev)      ((dev)->features[0] & LMP_HOLD)
 #define lmp_sniff_capable(dev)     ((dev)->features[0] & LMP_SNIFF)
+#define lmp_park_capable(dev)      ((dev)->features[1] & LMP_PARK)
+#define lmp_inq_rssi_capable(dev)  ((dev)->features[3] & LMP_RSSI_INQ)
 #define lmp_esco_capable(dev)      ((dev)->features[3] & LMP_ESCO)
 #define lmp_bredr_capable(dev)     (!((dev)->features[4] & LMP_NO_BREDR))
 #define lmp_le_capable(dev)        ((dev)->features[4] & LMP_LE)
 #define lmp_sniffsubr_capable(dev) ((dev)->features[5] & LMP_SNIFF_SUBR)
+#define lmp_pause_enc_capable(dev) ((dev)->features[5] & LMP_PAUSE_ENC)
+#define lmp_ext_inq_capable(dev)   ((dev)->features[6] & LMP_EXT_INQ)
 #define lmp_le_br_capable(dev)     ((dev)->features[6] & LMP_SIMUL_LE_BR)
 #define lmp_ssp_capable(dev)       ((dev)->features[6] & LMP_SIMPLE_PAIR)
 #define lmp_no_flush_capable(dev)  ((dev)->features[6] & LMP_NO_FLUSH)
+#define lmp_lsto_capable(dev)      ((dev)->features[7] & LMP_LSTO)
+#define lmp_inq_tx_pwr_capable(dev) ((dev)->features[7] & LMP_INQ_TX_PWR)
+#define lmp_ext_feat_capable(dev)  ((dev)->features[7] & LMP_EXTFEATURES)
 
 /* ----- Extended LMP capabilities ----- */
 #define lmp_host_le_capable(dev)   ((dev)->host_features[0] & LMP_HOST_LE)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index b63a5f3..0dbdf9b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -460,10 +460,10 @@ static void hci_cc_write_ssp_mode(struct hci_dev *hdev, struct sk_buff *skb)
 
 static u8 hci_get_inquiry_mode(struct hci_dev *hdev)
 {
-	if (hdev->features[6] & LMP_EXT_INQ)
+	if (lmp_ext_inq_capable(hdev))
 		return 2;
 
-	if (hdev->features[3] & LMP_RSSI_INQ)
+	if (lmp_inq_rssi_capable(hdev))
 		return 1;
 
 	if (hdev->manufacturer == 11 && hdev->hci_rev == 0x00 &&
@@ -515,22 +515,22 @@ static void hci_setup_event_mask(struct hci_dev *hdev)
 		events[5] |= 0x10; /* Synchronous Connection Changed */
 	}
 
-	if (hdev->features[3] & LMP_RSSI_INQ)
+	if (lmp_inq_rssi_capable(hdev))
 		events[4] |= 0x02; /* Inquiry Result with RSSI */
 
 	if (lmp_sniffsubr_capable(hdev))
 		events[5] |= 0x20; /* Sniff Subrating */
 
-	if (hdev->features[5] & LMP_PAUSE_ENC)
+	if (lmp_pause_enc_capable(hdev))
 		events[5] |= 0x80; /* Encryption Key Refresh Complete */
 
-	if (hdev->features[6] & LMP_EXT_INQ)
+	if (lmp_ext_inq_capable(hdev))
 		events[5] |= 0x40; /* Extended Inquiry Result */
 
 	if (lmp_no_flush_capable(hdev))
 		events[7] |= 0x01; /* Enhanced Flush Complete */
 
-	if (hdev->features[7] & LMP_LSTO)
+	if (lmp_lsto_capable(hdev))
 		events[6] |= 0x80; /* Link Supervision Timeout Changed */
 
 	if (lmp_ssp_capable(hdev)) {
@@ -633,13 +633,13 @@ static void hci_setup(struct hci_dev *hdev)
 		}
 	}
 
-	if (hdev->features[3] & LMP_RSSI_INQ)
+	if (lmp_inq_rssi_capable(hdev))
 		hci_setup_inquiry_mode(hdev);
 
-	if (hdev->features[7] & LMP_INQ_TX_PWR)
+	if (lmp_inq_tx_pwr_capable(hdev))
 		hci_send_cmd(hdev, HCI_OP_READ_INQ_RSP_TX_POWER, 0, NULL);
 
-	if (hdev->features[7] & LMP_EXTFEATURES) {
+	if (lmp_ext_feat_capable(hdev)) {
 		struct hci_cp_read_local_ext_features cp;
 
 		cp.page = 0x01;
@@ -686,11 +686,11 @@ static void hci_setup_link_policy(struct hci_dev *hdev)
 
 	if (lmp_rswitch_capable(hdev))
 		link_policy |= HCI_LP_RSWITCH;
-	if (hdev->features[0] & LMP_HOLD)
+	if (lmp_hold_capable(hdev))
 		link_policy |= HCI_LP_HOLD;
 	if (lmp_sniff_capable(hdev))
 		link_policy |= HCI_LP_SNIFF;
-	if (hdev->features[1] & LMP_PARK)
+	if (lmp_park_capable(hdev))
 		link_policy |= HCI_LP_PARK;
 
 	cp.policy = cpu_to_le16(link_policy);
@@ -780,10 +780,10 @@ static void hci_set_le_support(struct hci_dev *hdev)
 
 	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
 		cp.le = 1;
-		cp.simul = !!(hdev->features[6] & LMP_SIMUL_LE_BR);
+		cp.simul = !!lmp_le_br_capable(hdev);
 	}
 
-	if (cp.le != !!(hdev->host_features[0] & LMP_HOST_LE))
+	if (cp.le != !!lmp_host_le_capable(hdev))
 		hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
 			     &cp);
 	else if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index fe77f2c..e024f91 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -572,7 +572,7 @@ static int update_eir(struct hci_dev *hdev)
 	if (!hdev_is_powered(hdev))
 		return 0;
 
-	if (!(hdev->features[6] & LMP_EXT_INQ))
+	if (!lmp_ext_inq_capable(hdev))
 		return 0;
 
 	if (!test_bit(HCI_SSP_ENABLED, &hdev->dev_flags))
@@ -1302,7 +1302,7 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 
 static u8 get_le_mode(struct hci_dev *hdev)
 {
-	if (hdev->host_features[0] & LMP_HOST_LE) {
+	if (lmp_host_le_capable(hdev)) {
 		if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
 			return MGMT_LE_PERIPHERAL;
 		return MGMT_LE_CENTRAL;
@@ -1403,7 +1403,7 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 
 	if (old_mode == MGMT_LE_OFF) {
 		hci_cp.le = 1;
-		hci_cp.simul = !!(hdev->features[6] & LMP_SIMUL_LE_BR);
+		hci_cp.simul = !!lmp_le_br_capable(hdev);
 	}
 
 	err = hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED,
@@ -3510,7 +3510,7 @@ static int clear_eir(struct hci_dev *hdev)
 {
 	struct hci_cp_write_eir cp;
 
-	if (!(hdev->features[6] & LMP_EXT_INQ))
+	if (!lmp_ext_inq_capable(hdev))
 		return 0;
 
 	memset(hdev->eir, 0, sizeof(hdev->eir));
-- 
1.7.10.4

Re: [PATCH 4/7] Bluetooth: Add support for setting LE advertising data

[Anderson Lizardo]

Hi Johan,

On Tue, Oct 23, 2012 at 12:53 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> +       if (hdev->adv_tx_power) {
> +               ptr[0] = 2;
> +               ptr[1] = EIR_TX_POWER;
> +               ptr[2] = (u8) hdev->adv_tx_power;
> +
> +               ad_len += 3;
> +               ptr += 3;
> +       }

0dBm is a valid TX power. Not sure the if() clause is valid here.

Also, I'm worried how we are going to put other advertising data here,
i.e. Manufacturer Specific data or Service Data. On last BlueZ meeting
we proposed (and have been implementing) the Set Controller Data mgmt
command to set them. Is this still an acceptable approach?

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 3/7] Bluetooth: Disallow LE scanning and connecting in peripheral mode

[Anderson Lizardo]

Hi Johan,

On Tue, Oct 23, 2012 at 12:53 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
>
> When an adapter is in the LE peripheral mode scanning for other devices
> or initiating connections to them is not allowed. This patch makes sure
> that such attempts will result in appropriate error returns.

This will conflict with an Observer role running on the same device.
Multiple roles are allowed to run concurrently if supported by the
controller. From Core spec page 1639:

A device may operate in multiple GAP roles concurrently if supported by the
Controller. The Host should read the supported Link Layer States and State
combinations from the Controller before any procedures or modes are used.

Do you propose SET_LE to become a bitfield to support concurrent
roles?  I.e. Broadcaster/Observer.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters

[Anderson Lizardo]

Hi Johan,

On Tue, Oct 23, 2012 at 12:53 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> @@ -377,15 +377,15 @@ static u32 get_supported_settings(struct hci_dev *hdev)
>         u32 settings = 0;
>
>         settings |= MGMT_SETTING_POWERED;
> -       settings |= MGMT_SETTING_CONNECTABLE;
> -       settings |= MGMT_SETTING_FAST_CONNECTABLE;
> -       settings |= MGMT_SETTING_DISCOVERABLE;
>         settings |= MGMT_SETTING_PAIRABLE;
>
>         if (lmp_ssp_capable(hdev))
>                 settings |= MGMT_SETTING_SSP;
>
>         if (lmp_bredr_capable(hdev)) {
> +               settings |= MGMT_SETTING_CONNECTABLE;
> +               settings |= MGMT_SETTING_FAST_CONNECTABLE;
> +               settings |= MGMT_SETTING_DISCOVERABLE;
>                 settings |= MGMT_SETTING_BREDR;
>                 settings |= MGMT_SETTING_LINK_SECURITY;

"Discoverable" (at least as a Discovery mode from GAP) is mandatory
for LE Peripheral role (see page 1698). Same for "Connectable" (see
page 1706).

Unless you have other plans to implement this for single mode LE
peripherals running bluez?

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 1/7] Bluetooth: mgmt: Add support for switching to LE peripheral mode

[Marcel Holtmann]

Hi Johan,

> This patch extends the mgmt_set_le command to allow for a new value
> (0x02) to mean that LE should be enabled together with advertising
> enabled. This paves the way for acting in a fully functional LE
> peripheral role. The change to the mgmt_set_le command is fully
> backwards compatible as the value 0x01 means LE central role (which was
> the old behavior).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/hci.h      |    3 +
>  include/net/bluetooth/hci_core.h |    1 +
>  include/net/bluetooth/mgmt.h     |    6 ++
>  net/bluetooth/hci_event.c        |   44 +++++++++++
>  net/bluetooth/mgmt.c             |  159 +++++++++++++++++++++++++++++++-------
>  5 files changed, 185 insertions(+), 28 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 348f4bf..a633da0 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -115,6 +115,7 @@ enum {
>  	HCI_SSP_ENABLED,
>  	HCI_HS_ENABLED,
>  	HCI_LE_ENABLED,
> +	HCI_LE_PERIPHERAL,
>  	HCI_CONNECTABLE,
>  	HCI_DISCOVERABLE,
>  	HCI_LINK_SECURITY,
> @@ -938,6 +939,8 @@ struct hci_rp_le_read_adv_tx_power {
>  	__s8	tx_power;
>  } __packed;
>  
> +#define HCI_OP_LE_SET_ADV_ENABLE	0x200a
> +
>  #define HCI_OP_LE_SET_SCAN_PARAM	0x200b
>  struct hci_cp_le_set_scan_param {
>  	__u8    type;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 8260bf2..f288a8c 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1075,6 +1075,7 @@ int mgmt_set_local_name_complete(struct hci_dev *hdev, u8 *name, u8 status);
>  int mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8 *hash,
>  					    u8 *randomizer, u8 status);
>  int mgmt_le_enable_complete(struct hci_dev *hdev, u8 enable, u8 status);
> +int mgmt_le_adv_enable_complete(struct hci_dev *hdev, u8 enable, u8 status);
>  int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>  		      u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name,
>  		      u8 ssp, u8 *eir, u16 eir_len);
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 22980a7..26c9a4d 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -92,6 +92,7 @@ struct mgmt_rp_read_index_list {
>  #define MGMT_SETTING_BREDR		0x00000080
>  #define MGMT_SETTING_HS			0x00000100
>  #define MGMT_SETTING_LE			0x00000200
> +#define MGMT_SETTING_PERIPHERAL		0x00000400
>  
>  #define MGMT_OP_READ_INFO		0x0004
>  #define MGMT_READ_INFO_SIZE		0
> @@ -134,6 +135,11 @@ struct mgmt_cp_set_discoverable {
>  #define MGMT_OP_SET_HS			0x000C
>  
>  #define MGMT_OP_SET_LE			0x000D
> +
> +#define MGMT_LE_OFF			0x00
> +#define MGMT_LE_CENTRAL			0x01
> +#define MGMT_LE_PERIPHERAL		0x02
> +
>  #define MGMT_OP_SET_DEV_CLASS		0x000E
>  struct mgmt_cp_set_dev_class {
>  	__u8	major;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index fd5a51c..0393b34 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -786,6 +786,9 @@ static void hci_set_le_support(struct hci_dev *hdev)
>  	if (cp.le != !!(hdev->host_features[0] & LMP_HOST_LE))
>  		hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
>  			     &cp);
> +	else if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> +		hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE, sizeof(cp.le),
> +			     &cp.le);

What is this doing and why it is correct? I am failing to see this. We
need to enable LE host supported first anyway.

>  }
>  
>  static void hci_cc_read_local_ext_features(struct hci_dev *hdev,
> @@ -1189,6 +1192,38 @@ static void hci_cc_le_set_scan_param(struct hci_dev *hdev, struct sk_buff *skb)
>  	}
>  }
>  
> +static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	__u8 *sent, status = *((__u8 *) skb->data);
> +
> +	BT_DBG("%s status 0x%2.2x", hdev->name, status);
> +
> +	sent = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_ADV_ENABLE);
> +	if (!sent)
> +		return;
> +
> +	hci_dev_lock(hdev);
> +
> +	if (!status) {
> +		if (*sent)
> +			set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> +		else
> +			clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> +	} else {
> +		if (*sent)
> +			clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> +		else
> +			set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> +	}

Are you sure we want to based LE peripheral enabling based on if we
advertise or not. I can see cases where we are a peripheral, but might
want to not always advertise.

> +
> +	if (!test_bit(HCI_INIT, &hdev->flags))
> +		mgmt_le_adv_enable_complete(hdev, *sent, status);
> +
> +	hci_dev_unlock(hdev);
> +
> +	hci_req_complete(hdev, HCI_OP_LE_SET_ADV_ENABLE, status);
> +}
> +
>  static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>  				      struct sk_buff *skb)
>  {
> @@ -1287,6 +1322,11 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
>  			hdev->host_features[0] |= LMP_HOST_LE;
>  		else
>  			hdev->host_features[0] &= ~LMP_HOST_LE;
> +
> +		if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags) &&
> +		    test_bit(HCI_INIT, &hdev->flags))
> +			hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
> +				     sizeof(sent->le), &sent->le);
>  	}
>  
>  	if (test_bit(HCI_MGMT, &hdev->dev_flags) &&
> @@ -2549,6 +2589,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>  		hci_cc_le_set_scan_param(hdev, skb);
>  		break;
>  
> +	case HCI_OP_LE_SET_ADV_ENABLE:
> +		hci_cc_le_set_adv_enable(hdev, skb);
> +		break;
> +
>  	case HCI_OP_LE_SET_SCAN_ENABLE:
>  		hci_cc_le_set_scan_enable(hdev, skb);
>  		break;
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fe74f4..1d79979 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -393,8 +393,10 @@ static u32 get_supported_settings(struct hci_dev *hdev)
>  	if (enable_hs)
>  		settings |= MGMT_SETTING_HS;
>  
> -	if (lmp_le_capable(hdev))
> +	if (lmp_le_capable(hdev)) {
>  		settings |= MGMT_SETTING_LE;
> +		settings |= MGMT_SETTING_PERIPHERAL;
> +	}
>  
>  	return settings;
>  }
> @@ -418,9 +420,13 @@ static u32 get_current_settings(struct hci_dev *hdev)
>  	if (lmp_bredr_capable(hdev))
>  		settings |= MGMT_SETTING_BREDR;
>  
> -	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
> +	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
>  		settings |= MGMT_SETTING_LE;
>  
> +		if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> +			settings |= MGMT_SETTING_PERIPHERAL;
> +	}
> +
>  	if (test_bit(HCI_LINK_SECURITY, &hdev->dev_flags))
>  		settings |= MGMT_SETTING_LINK_SECURITY;
>  
> @@ -1195,13 +1201,36 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
>  	return send_settings_rsp(sk, MGMT_OP_SET_HS, hdev);
>  }
>  
> +static u8 get_le_mode(struct hci_dev *hdev)
> +{
> +	if (hdev->host_features[0] & LMP_HOST_LE) {
> +		if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> +			return MGMT_LE_PERIPHERAL;
> +		return MGMT_LE_CENTRAL;
> +	}
> +
> +	return MGMT_LE_OFF;
> +}
> +
> +static bool valid_le_mode(u8 mode)
> +{
> +	switch (mode) {
> +	case MGMT_LE_OFF:
> +	case MGMT_LE_CENTRAL:
> +	case MGMT_LE_PERIPHERAL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +

I prefer to not use the default and just return false at the end of the
function.

>  static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
>  {
> -	struct mgmt_mode *cp = data;
>  	struct hci_cp_write_le_host_supported hci_cp;
> +	struct mgmt_mode *cp = data;
>  	struct pending_cmd *cmd;
> +	u8 old_mode, new_mode;
>  	int err;
> -	u8 val, enabled;
>  
>  	BT_DBG("request for %s", hdev->name);
>  
> @@ -1213,48 +1242,71 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
>  		goto unlock;
>  	}
>  
> -	val = !!cp->val;
> -	enabled = !!(hdev->host_features[0] & LMP_HOST_LE);
> +	if (!valid_le_mode(cp->val)) {
> +		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> +				 MGMT_STATUS_INVALID_PARAMS);
> +		goto unlock;
> +	}
>  
> -	if (!hdev_is_powered(hdev) || val == enabled) {
> -		bool changed = false;
> +	if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> +		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> +				 MGMT_STATUS_BUSY);
> +		goto unlock;
> +	}
> +
> +	new_mode = cp->val;
> +	old_mode = get_le_mode(hdev);
> +
> +	BT_DBG("%s old_mode %u new_mode %u", hdev->name, old_mode, new_mode);
> +
> +	if (new_mode == old_mode) {
> +		err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> +		goto unlock;
> +	}
> +
> +	if (old_mode == MGMT_LE_PERIPHERAL || new_mode == MGMT_LE_PERIPHERAL)
> +		change_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
>  
> -		if (val != test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
> +	if (!hdev_is_powered(hdev)) {
> +		if (old_mode == MGMT_LE_OFF || new_mode == MGMT_LE_OFF)
>  			change_bit(HCI_LE_ENABLED, &hdev->dev_flags);
> -			changed = true;
> -		}
>  
>  		err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
>  		if (err < 0)
>  			goto unlock;
>  
> -		if (changed)
> -			err = new_settings(hdev, sk);
> +		err = new_settings(hdev, sk);
>  
>  		goto unlock;
>  	}
>  
> -	if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> -		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> -				 MGMT_STATUS_BUSY);
> -		goto unlock;
> -	}
> -
>  	cmd = mgmt_pending_add(sk, MGMT_OP_SET_LE, hdev, data, len);
>  	if (!cmd) {
>  		err = -ENOMEM;
>  		goto unlock;
>  	}
>  
> +	if ((old_mode != MGMT_LE_OFF && new_mode == MGMT_LE_PERIPHERAL) ||
> +	    old_mode == MGMT_LE_PERIPHERAL) {
> +		u8 adv_enable = (new_mode == MGMT_LE_PERIPHERAL);
> +
> +		err = hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
> +				   sizeof(adv_enable), &adv_enable);
> +		if (err < 0)
> +			mgmt_pending_remove(cmd);
> +
> +		goto unlock;
> +	}
> +

this gets a bit complicated. We either need more comments or split this
out in separate helper functions.

>  	memset(&hci_cp, 0, sizeof(hci_cp));
>  
> -	if (val) {
> -		hci_cp.le = val;
> +	if (old_mode == MGMT_LE_OFF) {
> +		hci_cp.le = 1;
>  		hci_cp.simul = !!(hdev->features[6] & LMP_SIMUL_LE_BR);
>  	}
>  
> -	err = hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(hci_cp),
> -			   &hci_cp);
> +	err = hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED,
> +			   sizeof(hci_cp), &hci_cp);
>  	if (err < 0)
>  		mgmt_pending_remove(cmd);
>  
> @@ -3525,7 +3577,8 @@ int mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8 *hash,
>  
>  int mgmt_le_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
>  {
> -	struct cmd_lookup match = { NULL, hdev };
> +	struct pending_cmd *cmd;
> +	struct mgmt_mode *cp;
>  	bool changed = false;
>  	int err = 0;
>  
> @@ -3550,17 +3603,67 @@ int mgmt_le_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
>  			changed = true;
>  	}
>  
> -	mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, settings_rsp, &match);
> +	cmd = mgmt_pending_find(MGMT_OP_SET_LE, hdev);
> +	if (!cmd)
> +		goto done;
> +
> +	cp = cmd->param;
> +	if (enable && cp->val == MGMT_LE_PERIPHERAL)
> +		return hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
> +				    sizeof(enable), &enable);
>  
> +	send_settings_rsp(cmd->sk, cmd->opcode, hdev);
> +	list_del(&cmd->list);
> +
> +done:
>  	if (changed)
> -		err = new_settings(hdev, match.sk);
> +		err = new_settings(hdev, cmd ? cmd->sk : NULL);
>  
> -	if (match.sk)
> -		sock_put(match.sk);
> +	if (cmd)
> +		mgmt_pending_free(cmd);
>  
>  	return err;
>  }
>  
> +int mgmt_le_adv_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
> +{
> +	struct pending_cmd *cmd;
> +	struct mgmt_mode *cp;
> +
> +	if (status) {
> +		u8 mgmt_err = mgmt_status(status);
> +
> +		mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, cmd_status_rsp,
> +				     &mgmt_err);
> +
> +		return 0;
> +	}
> +
> +	cmd = mgmt_pending_find(MGMT_OP_SET_LE, hdev);
> +	if (!cmd) {
> +		new_settings(hdev, NULL);
> +		return 0;
> +	}
> +
> +	cp = cmd->param;
> +	if (!enable && cp->val == MGMT_LE_OFF) {
> +		struct hci_cp_write_le_host_supported hci_cp;
> +
> +		memset(&hci_cp, 0, sizeof(hci_cp));
> +
> +		return hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED,
> +				    sizeof(hci_cp), &hci_cp);
> +	}
> +
> +	new_settings(hdev, cmd->sk);
> +	send_settings_rsp(cmd->sk, cmd->opcode, hdev);
> +
> +	list_del(&cmd->list);
> +	mgmt_pending_free(cmd);
> +
> +	return 0;
> +}
> +
>  int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>  		      u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name, u8
>  		      ssp, u8 *eir, u16 eir_len)

Regards

Marcel

Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters

[Marcel Holtmann]

Hi Johan,

> This patch makes sure that settings which are specific for BR/EDR
> capable adapters are not allowed for non-BR/EDR (e.g. LE-only) adapters.
> Instead, a "not supported" error is returned of such a setting is
> attempted to be set for a non-BR/EDR adapter.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |   22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)

can we lead with this patch. It seems to be useful no matter what we do
for LE peripheral or single-mode controllers.

Regards

Marcel

Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters

[Marcel Holtmann]

Hi Anderson,

> > @@ -377,15 +377,15 @@ static u32 get_supported_settings(struct hci_dev *hdev)
> >         u32 settings = 0;
> >
> >         settings |= MGMT_SETTING_POWERED;
> > -       settings |= MGMT_SETTING_CONNECTABLE;
> > -       settings |= MGMT_SETTING_FAST_CONNECTABLE;
> > -       settings |= MGMT_SETTING_DISCOVERABLE;
> >         settings |= MGMT_SETTING_PAIRABLE;
> >
> >         if (lmp_ssp_capable(hdev))
> >                 settings |= MGMT_SETTING_SSP;
> >
> >         if (lmp_bredr_capable(hdev)) {
> > +               settings |= MGMT_SETTING_CONNECTABLE;
> > +               settings |= MGMT_SETTING_FAST_CONNECTABLE;
> > +               settings |= MGMT_SETTING_DISCOVERABLE;
> >                 settings |= MGMT_SETTING_BREDR;
> >                 settings |= MGMT_SETTING_LINK_SECURITY;
> 
> "Discoverable" (at least as a Discovery mode from GAP) is mandatory
> for LE Peripheral role (see page 1698). Same for "Connectable" (see
> page 1706).
> 
> Unless you have other plans to implement this for single mode LE
> peripherals running bluez?

at this moment, we plan to keep LE a bit independent from BR/EDR.

Regards

Marcel

Re: [PATCH 5/7] Bluetooth: Fix updating host feature bits for LE

[Marcel Holtmann]

Hi Johan,

> When LE has been enabled with the simultaneous BR/EDR & LE parameter set
> to true we should also update the host features stored in struct hci_dev
> accordingly.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_event.c |    5 +++++
>  1 file changed, 5 insertions(+)

should go first as well.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 6/7] Bluetooth: Sort feature test macros by bitmask location

[Marcel Holtmann]

Hi Johan,

> This patch sorts the feature test marcos according to the location of
> each feature bit in the features bit mask. This helps easy lookup when
> adding new marcos and wanting to check if there already is a macro
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

this could go first as well.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 7/7] Bluetooth: Make use feature test macros

[Marcel Holtmann]

Hi Johan,

> For better code readability and avoiding simple bugs of checking the
> wrong byte of the features make use of feature test macros whenever
> possible.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |    8 ++++++++
>  net/bluetooth/hci_event.c        |   26 +++++++++++++-------------
>  net/bluetooth/mgmt.c             |    8 ++++----
>  3 files changed, 25 insertions(+), 17 deletions(-)

nice cleanup we should do first as well.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters

[Anderson Lizardo]

Hi Marcel,

On Tue, Oct 23, 2012 at 3:02 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Johan,
>
>> This patch makes sure that settings which are specific for BR/EDR
>> capable adapters are not allowed for non-BR/EDR (e.g. LE-only) adapters.
>> Instead, a "not supported" error is returned of such a setting is
>> attempted to be set for a non-BR/EDR adapter.
>>
>> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>> ---
>>  net/bluetooth/mgmt.c |   22 +++++++++++++++++++---
>>  1 file changed, 19 insertions(+), 3 deletions(-)
>
> can we lead with this patch. It seems to be useful no matter what we do
> for LE peripheral or single-mode controllers.

I agree that current upstream will be more consistent if these
settings are made "not supported" for LE (as they indeed are not
implemented yet). I was just confused that the same patch series that
improve LE peripheral support does not include properly support for
Connectable/Discoverable modes on this role (which may restrict the
usefulness?).

Johan: do you have further plans on improving LE peripheral support
after these patches? If yes, please keep in mind that we still have
ongoing plans to push broadcaster/observer roles upstream, which also
requires hability to enable/disable advertising and set advertising
data/parameters.

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 1/7] Bluetooth: mgmt: Add support for switching to LE peripheral mode

[Johan Hedberg]

Hi Marcel,

On Tue, Oct 23, 2012, Marcel Holtmann wrote:
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -786,6 +786,9 @@ static void hci_set_le_support(struct hci_dev *hdev)
> >  	if (cp.le != !!(hdev->host_features[0] & LMP_HOST_LE))
> >  		hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
> >  			     &cp);
> > +	else if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> > +		hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE, sizeof(cp.le),
> > +			     &cp.le);
> 
> What is this doing and why it is correct? I am failing to see this. We
> need to enable LE host supported first anyway.

The first if-statement checks if the host feature bits are what we want
them to be, and if not send the WRITE_LE_HOST_SUPPORTED command. The
else if part (if the host features are already correct and don't need
updating) enables advertising if necessary. There's a similar check for
HCI_LE_PERIPHERAL in the command complete handler for
HCI_OP_WRITE_LE_HOST_SUPPORTED in case the first if statement was
triggered. FWIW, I have actually tested this both with hciconfig and
btmgmt and it works fine.

> > +static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	__u8 *sent, status = *((__u8 *) skb->data);
> > +
> > +	BT_DBG("%s status 0x%2.2x", hdev->name, status);
> > +
> > +	sent = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_ADV_ENABLE);
> > +	if (!sent)
> > +		return;
> > +
> > +	hci_dev_lock(hdev);
> > +
> > +	if (!status) {
> > +		if (*sent)
> > +			set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > +		else
> > +			clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > +	} else {
> > +		if (*sent)
> > +			clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > +		else
> > +			set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > +	}
> 
> Are you sure we want to based LE peripheral enabling based on if we
> advertise or not. I can see cases where we are a peripheral, but might
> want to not always advertise.

These set_bit/clear_bit calls are actually all no-op in the case that
the mgmt interface is used (since the LE_PERIPHERAL flag is already
modified when receiving the mgmt command). The only reason I put them
here is so that the reported mgmt settings remain correct and we reject
connection initiation and scanning if "hciconfig hci0 leadv" or similar
is used.

The simplistic way I thought we'd initially keep peripheral mode is
that it always implies advertising (unless a connection exists).
Otherwise the user would need to switch to LE-off or Central mode.

> > @@ -1213,48 +1242,71 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> >  		goto unlock;
> >  	}
> >  
> > -	val = !!cp->val;
> > -	enabled = !!(hdev->host_features[0] & LMP_HOST_LE);
> > +	if (!valid_le_mode(cp->val)) {
> > +		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > +				 MGMT_STATUS_INVALID_PARAMS);
> > +		goto unlock;
> > +	}
> >  
> > -	if (!hdev_is_powered(hdev) || val == enabled) {
> > -		bool changed = false;
> > +	if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> > +		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > +				 MGMT_STATUS_BUSY);
> > +		goto unlock;
> > +	}
> > +
> > +	new_mode = cp->val;
> > +	old_mode = get_le_mode(hdev);
> > +
> > +	BT_DBG("%s old_mode %u new_mode %u", hdev->name, old_mode, new_mode);
> > +
> > +	if (new_mode == old_mode) {
> > +		err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> > +		goto unlock;
> > +	}
> > +
> > +	if (old_mode == MGMT_LE_PERIPHERAL || new_mode == MGMT_LE_PERIPHERAL)
> > +		change_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> >  
> > -		if (val != test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
> > +	if (!hdev_is_powered(hdev)) {
> > +		if (old_mode == MGMT_LE_OFF || new_mode == MGMT_LE_OFF)
> >  			change_bit(HCI_LE_ENABLED, &hdev->dev_flags);
> > -			changed = true;
> > -		}
> >  
> >  		err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> >  		if (err < 0)
> >  			goto unlock;
> >  
> > -		if (changed)
> > -			err = new_settings(hdev, sk);
> > +		err = new_settings(hdev, sk);
> >  
> >  		goto unlock;
> >  	}
> >  
> > -	if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> > -		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > -				 MGMT_STATUS_BUSY);
> > -		goto unlock;
> > -	}
> > -
> >  	cmd = mgmt_pending_add(sk, MGMT_OP_SET_LE, hdev, data, len);
> >  	if (!cmd) {
> >  		err = -ENOMEM;
> >  		goto unlock;
> >  	}
> >  
> > +	if ((old_mode != MGMT_LE_OFF && new_mode == MGMT_LE_PERIPHERAL) ||
> > +	    old_mode == MGMT_LE_PERIPHERAL) {
> > +		u8 adv_enable = (new_mode == MGMT_LE_PERIPHERAL);
> > +
> > +		err = hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
> > +				   sizeof(adv_enable), &adv_enable);
> > +		if (err < 0)
> > +			mgmt_pending_remove(cmd);
> > +
> > +		goto unlock;
> > +	}
> > +
> 
> this gets a bit complicated. We either need more comments or split this
> out in separate helper functions.

I actually spent over a half a day trying to figure out an easy way to
simplify this, but the best I got was those two simple helper functions.
So I think I'll opt for just adding good code comments.

Johan

Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters

[Johan Hedberg]

Hi Lizardo,

On Tue, Oct 23, 2012, Anderson Lizardo wrote:
> Johan: do you have further plans on improving LE peripheral support
> after these patches? If yes, please keep in mind that we still have
> ongoing plans to push broadcaster/observer roles upstream, which also
> requires hability to enable/disable advertising and set advertising
> data/parameters.

Looking through the various "shall" and "shall not" clauses in the core
spec it seems to me that only Central, Observer and Broadcaster roles
are compatible with each other, and Peripheral is mutually exclusive
with anything else. E.g. section 9.2.4.2 (page 1700):

"While a device is in the Broadcaster, Observer or Central role the
device shall not support the general discoverable mode."

and section 9.3.4.2 (page 1709):

"While a device is in the Broadcaster, Observer, or the Central role
the device shall not support the undirected connectable mode."

Both general discoverable and undirected connectable are needed by
Peripheral role, i.e. essentially excluding the other roles.

Extending the other roles could indeed be a matter of interpreting the
mgmt_set_le parameter as a bit mask and then returning an error if an
incompatible combination of roles is attempted. Also, once support for
new roles is added it should be easy to extend the current state checks
to make a distinction on exactly what kind of scanning/advertising is
done instead of just looking at "advertising or not" on a general level.

Johan

Re: [PATCH 3/7] Bluetooth: Disallow LE scanning and connecting in peripheral mode

[Johan Hedberg]

Hi Lizardo,

On Tue, Oct 23, 2012, Anderson Lizardo wrote:
> This will conflict with an Observer role running on the same device.
> Multiple roles are allowed to run concurrently if supported by the
> controller. From Core spec page 1639:
> 
> A device may operate in multiple GAP roles concurrently if supported
> by the Controller. The Host should read the supported Link Layer
> States and State combinations from the Controller before any
> procedures or modes are used.
> 
> Do you propose SET_LE to become a bitfield to support concurrent
> roles?  I.e. Broadcaster/Observer.

See the other email I sent about this. If peripheral mode is enabled I
don't see how we could simultaneously allow another LE GAP mode based on
the restrictions of the core spec. And yes, we could make set_le a bit
field, but peripheral will probably still remain mutually exclusive with
any other LE GAP mode.

Johan

Re: [PATCH 4/7] Bluetooth: Add support for setting LE advertising data

[Johan Hedberg]

Hi Lizardo,

On Tue, Oct 23, 2012, Anderson Lizardo wrote:
> On Tue, Oct 23, 2012 at 12:53 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > +       if (hdev->adv_tx_power) {
> > +               ptr[0] = 2;
> > +               ptr[1] = EIR_TX_POWER;
> > +               ptr[2] = (u8) hdev->adv_tx_power;
> > +
> > +               ad_len += 3;
> > +               ptr += 3;
> > +       }
> 
> 0dBm is a valid TX power. Not sure the if() clause is valid here.

If it's not, we'd need to extend the storage size of hdev->adv_tx_power
(to have some invalid value) or add another variable to hdev to indicate
that we know the tx_power. FWIW, create_eir also uses the same test for
including the inq_tx_power, so either both or neither should be fixed.
For simplicity I'd just keep the 0-test unless 0 is a really common
value.

> Also, I'm worried how we are going to put other advertising data here,
> i.e. Manufacturer Specific data or Service Data. On last BlueZ meeting
> we proposed (and have been implementing) the Set Controller Data mgmt
> command to set them. Is this still an acceptable approach?

Yes, though we only have 31 bytes to play with here which is quite
little. Also, we might want to make this dependent on the LE GAP role.
E.g. we might be more interested in manufacturer specific data for
broadcaster role than for peripheral role (maybe a user space
application *only* wants its data for broadcaster role).

Johan

Re: [PATCH 3/7] Bluetooth: Disallow LE scanning and connecting in peripheral mode

[Marcel Holtmann]

Hi Johan,

> When an adapter is in the LE peripheral mode scanning for other devices
> or initiating connections to them is not allowed. This patch makes sure
> that such attempts will result in appropriate error returns.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_conn.c |    3 +++
>  net/bluetooth/hci_core.c |    3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index fe64621..a75b30e 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -502,6 +502,9 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
>  {
>  	struct hci_conn *le;
>  
> +	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
> +		return ERR_PTR(-ENOTSUPP);
> +
>  	le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
>  	if (!le) {
>  		le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 487308b..b2adef4 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1575,6 +1575,9 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>  
>  	BT_DBG("%s", hdev->name);
>  
> +	if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> +		return -ENOTSUPP;
> +
>  	if (work_busy(&hdev->le_scan))
>  		return -EINPROGRESS;
>  

I am fine with this patch, but maybe it would be better to introduce
this flag first. Specific the semantics in the commit message and the
mgmt documentation, then add this patch and only after that actually
start implementing support for it.

Regards

Marcel

Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters

[Marcel Holtmann]

Hi Johan,

> > Johan: do you have further plans on improving LE peripheral support
> > after these patches? If yes, please keep in mind that we still have
> > ongoing plans to push broadcaster/observer roles upstream, which also
> > requires hability to enable/disable advertising and set advertising
> > data/parameters.
> 
> Looking through the various "shall" and "shall not" clauses in the core
> spec it seems to me that only Central, Observer and Broadcaster roles
> are compatible with each other, and Peripheral is mutually exclusive
> with anything else. E.g. section 9.2.4.2 (page 1700):
> 
> "While a device is in the Broadcaster, Observer or Central role the
> device shall not support the general discoverable mode."
> 
> and section 9.3.4.2 (page 1709):
> 
> "While a device is in the Broadcaster, Observer, or the Central role
> the device shall not support the undirected connectable mode."
> 
> Both general discoverable and undirected connectable are needed by
> Peripheral role, i.e. essentially excluding the other roles.
> 
> Extending the other roles could indeed be a matter of interpreting the
> mgmt_set_le parameter as a bit mask and then returning an error if an
> incompatible combination of roles is attempted. Also, once support for
> new roles is added it should be easy to extend the current state checks
> to make a distinction on exactly what kind of scanning/advertising is
> done instead of just looking at "advertising or not" on a general level.

the way I see this now (which might change), we are doing LE on/off. And
by default this means actually central. For us it is a good assumption
that we are central if LE is enabled. And this means if the LE flag is
set, then we are a central. As it has been so far. In case we want to
switch this to peripheral, then we have to set PERIPHERAL flag. The LE
flag stays on to indicate LE is enabled. !PERIPHERAL flag here means we
are central.

That is why I like the explanation above, if we set ourselves as
peripheral, then we are peripheral and everything else is out of the
question. So either LE is enabled and we act as central or we are acting
as peripheral.

So broadcaster and observer are extra features on top of central. For me
that means we have treat them independent. How much you can be
broadcaster and observer and central at the same time is then again a
different question that we need to answer.

Regards

Marcel

Re: [PATCH 1/7] Bluetooth: mgmt: Add support for switching to LE peripheral mode

[Marcel Holtmann]

Hi Johan,

> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -786,6 +786,9 @@ static void hci_set_le_support(struct hci_dev *hdev)
> > >  	if (cp.le != !!(hdev->host_features[0] & LMP_HOST_LE))
> > >  		hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
> > >  			     &cp);
> > > +	else if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> > > +		hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE, sizeof(cp.le),
> > > +			     &cp.le);
> > 
> > What is this doing and why it is correct? I am failing to see this. We
> > need to enable LE host supported first anyway.
> 
> The first if-statement checks if the host feature bits are what we want
> them to be, and if not send the WRITE_LE_HOST_SUPPORTED command. The
> else if part (if the host features are already correct and don't need
> updating) enables advertising if necessary. There's a similar check for
> HCI_LE_PERIPHERAL in the command complete handler for
> HCI_OP_WRITE_LE_HOST_SUPPORTED in case the first if statement was
> triggered. FWIW, I have actually tested this both with hciconfig and
> btmgmt and it works fine.
> 
> > > +static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> > > +{
> > > +	__u8 *sent, status = *((__u8 *) skb->data);
> > > +
> > > +	BT_DBG("%s status 0x%2.2x", hdev->name, status);
> > > +
> > > +	sent = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_ADV_ENABLE);
> > > +	if (!sent)
> > > +		return;
> > > +
> > > +	hci_dev_lock(hdev);
> > > +
> > > +	if (!status) {
> > > +		if (*sent)
> > > +			set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > > +		else
> > > +			clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > > +	} else {
> > > +		if (*sent)
> > > +			clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > > +		else
> > > +			set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > > +	}
> > 
> > Are you sure we want to based LE peripheral enabling based on if we
> > advertise or not. I can see cases where we are a peripheral, but might
> > want to not always advertise.
> 
> These set_bit/clear_bit calls are actually all no-op in the case that
> the mgmt interface is used (since the LE_PERIPHERAL flag is already
> modified when receiving the mgmt command). The only reason I put them
> here is so that the reported mgmt settings remain correct and we reject
> connection initiation and scanning if "hciconfig hci0 leadv" or similar
> is used.
> 
> The simplistic way I thought we'd initially keep peripheral mode is
> that it always implies advertising (unless a connection exists).
> Otherwise the user would need to switch to LE-off or Central mode.

I am fine with this. However we can not merge this upstream until we
have this figured out and have a good grasp on this. So it would be nice
to have this patch last. Since some of the other patches could be
actually merged right now. They are useful fixes no matter what.

We might want to introduce a enable_peripheral module parameter as safe
guard for the time being. So that the features bit PERIPHERAL is not set
if this is disabled. Just an idea.

> 
> > > @@ -1213,48 +1242,71 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> > >  		goto unlock;
> > >  	}
> > >  
> > > -	val = !!cp->val;
> > > -	enabled = !!(hdev->host_features[0] & LMP_HOST_LE);
> > > +	if (!valid_le_mode(cp->val)) {
> > > +		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > > +				 MGMT_STATUS_INVALID_PARAMS);
> > > +		goto unlock;
> > > +	}
> > >  
> > > -	if (!hdev_is_powered(hdev) || val == enabled) {
> > > -		bool changed = false;
> > > +	if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> > > +		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > > +				 MGMT_STATUS_BUSY);
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	new_mode = cp->val;
> > > +	old_mode = get_le_mode(hdev);
> > > +
> > > +	BT_DBG("%s old_mode %u new_mode %u", hdev->name, old_mode, new_mode);
> > > +
> > > +	if (new_mode == old_mode) {
> > > +		err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	if (old_mode == MGMT_LE_PERIPHERAL || new_mode == MGMT_LE_PERIPHERAL)
> > > +		change_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > >  
> > > -		if (val != test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
> > > +	if (!hdev_is_powered(hdev)) {
> > > +		if (old_mode == MGMT_LE_OFF || new_mode == MGMT_LE_OFF)
> > >  			change_bit(HCI_LE_ENABLED, &hdev->dev_flags);
> > > -			changed = true;
> > > -		}
> > >  
> > >  		err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> > >  		if (err < 0)
> > >  			goto unlock;
> > >  
> > > -		if (changed)
> > > -			err = new_settings(hdev, sk);
> > > +		err = new_settings(hdev, sk);
> > >  
> > >  		goto unlock;
> > >  	}
> > >  
> > > -	if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> > > -		err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > > -				 MGMT_STATUS_BUSY);
> > > -		goto unlock;
> > > -	}
> > > -
> > >  	cmd = mgmt_pending_add(sk, MGMT_OP_SET_LE, hdev, data, len);
> > >  	if (!cmd) {
> > >  		err = -ENOMEM;
> > >  		goto unlock;
> > >  	}
> > >  
> > > +	if ((old_mode != MGMT_LE_OFF && new_mode == MGMT_LE_PERIPHERAL) ||
> > > +	    old_mode == MGMT_LE_PERIPHERAL) {
> > > +		u8 adv_enable = (new_mode == MGMT_LE_PERIPHERAL);
> > > +
> > > +		err = hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
> > > +				   sizeof(adv_enable), &adv_enable);
> > > +		if (err < 0)
> > > +			mgmt_pending_remove(cmd);
> > > +
> > > +		goto unlock;
> > > +	}
> > > +
> > 
> > this gets a bit complicated. We either need more comments or split this
> > out in separate helper functions.
> 
> I actually spent over a half a day trying to figure out an easy way to
> simplify this, but the best I got was those two simple helper functions.
> So I think I'll opt for just adding good code comments.

Sounds good to me.

Regards

Marcel

Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters

[Anderson Lizardo]

Hi Marcel,

On Tue, Oct 23, 2012 at 5:46 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> That is why I like the explanation above, if we set ourselves as
> peripheral, then we are peripheral and everything else is out of the
> question. So either LE is enabled and we act as central or we are acting
> as peripheral.
>
> So broadcaster and observer are extra features on top of central. For me
> that means we have treat them independent. How much you can be
> broadcaster and observer and central at the same time is then again a
> different question that we need to answer.

So do you suggest we keep Broadcaster/Observer as separate settings
(which actually we prefer, given that we have been working on
supporting those modes for BR/EDR only adapters, by using EIR for
broadcasting data allowed on EIR, such as Manufacturer Specific Data,
and parsing them from Device Found mgmt event when on Observer mode),
and keep "Set Low Energy" setting just for Central/Peripheral modes?

Regards
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters

[Marcel Holtmann]

Hi Anderson,

> > That is why I like the explanation above, if we set ourselves as
> > peripheral, then we are peripheral and everything else is out of the
> > question. So either LE is enabled and we act as central or we are acting
> > as peripheral.
> >
> > So broadcaster and observer are extra features on top of central. For me
> > that means we have treat them independent. How much you can be
> > broadcaster and observer and central at the same time is then again a
> > different question that we need to answer.
> 
> So do you suggest we keep Broadcaster/Observer as separate settings
> (which actually we prefer, given that we have been working on
> supporting those modes for BR/EDR only adapters, by using EIR for
> broadcasting data allowed on EIR, such as Manufacturer Specific Data,
> and parsing them from Device Found mgmt event when on Observer mode),
> and keep "Set Low Energy" setting just for Central/Peripheral modes?

if we can stay in central role and do broadcaster/observer as an
additional feature, then yes. If they are mutually exclusive, we have to
find something else.

But with all of this, I do not have the final answer until we tried it
and had a couple of review rounds.

Regards

Marcel

Re: [PATCH 4/7] Bluetooth: Add support for setting LE advertising data

[Anderson Lizardo]

Hi Johan,

On Tue, Oct 23, 2012 at 5:26 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Lizardo,
>
> On Tue, Oct 23, 2012, Anderson Lizardo wrote:
>> On Tue, Oct 23, 2012 at 12:53 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> > +       if (hdev->adv_tx_power) {
>> > +               ptr[0] = 2;
>> > +               ptr[1] = EIR_TX_POWER;
>> > +               ptr[2] = (u8) hdev->adv_tx_power;
>> > +
>> > +               ad_len += 3;
>> > +               ptr += 3;
>> > +       }
>>
>> 0dBm is a valid TX power. Not sure the if() clause is valid here.
>
> If it's not, we'd need to extend the storage size of hdev->adv_tx_power
> (to have some invalid value) or add another variable to hdev to indicate
> that we know the tx_power. FWIW, create_eir also uses the same test for
> including the inq_tx_power, so either both or neither should be fixed.
> For simplicity I'd just keep the 0-test unless 0 is a really common
> value.

0dBm is right in the middle of the valid range for BR/EDR. From "7.5.4
Read RSSI Command" (page 934):

BR/EDR
Range: -128 <= N <= 127 (signed integer)
Units: dBm
...
LE:
Range: -127 to 20, 127 (signed integer)
Units: dBm

>From "7.8.6 LE Read Advertising Channel Tx Power Command" (page 1061):

Range: -20 <= N <= 10

and from "7.3.35 Read Transmit Power Level Command" (page 857):

Range: -30 <= N <= 20
Units: dBm

IIRC, TI based LE controllers I tested for Proximity used to have 0dBm
TX power for active connections (as measured using Read Transmit Power
Level Command or TX Power GATT characteristic). It's not uncommon,
IMHO.

My suggestion would be to have a separate boolean for checking if
Advertising/Inquiry TX Power was read. I suppose this is not critical
as this information is not mandatory on Advertising Data anyway (at
least LE Proximity profile uses the connection specific TX power, not
advertising TX Power).

>> Also, I'm worried how we are going to put other advertising data here,
>> i.e. Manufacturer Specific data or Service Data. On last BlueZ meeting
>> we proposed (and have been implementing) the Set Controller Data mgmt
>> command to set them. Is this still an acceptable approach?
>
> Yes, though we only have 31 bytes to play with here which is quite
> little. Also, we might want to make this dependent on the LE GAP role.
> E.g. we might be more interested in manufacturer specific data for
> broadcaster role than for peripheral role (maybe a user space
> application *only* wants its data for broadcaster role).

I agree, but If I understand correctly, it will be BlueZ's
responsibility to enable/disable Broadcaster/Observer modes depending
on whether there is any active application registered over D-Bus.

>
> Johan

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil