[PATCH 1/7] Bluetooth: Add macros for advertising instance flags

[PATCH 1/7] Bluetooth: Add macros for advertising instance flags

[Arman Uguray]

This patch adds macro definitions for possible advertising instance
flags that can be passed to the "Add Advertising" command.

Signed-off-by: Arman Uguray <armansito@chromium.org>
---
 include/net/bluetooth/mgmt.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 68abd4b..fc50cee 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -554,6 +554,14 @@ struct mgmt_rp_add_advertising {
 	__u8	instance;
 } __packed;
 
+#define MGMT_ADV_FLAG_CONNECTABLE	(1 << 0)
+#define MGMT_ADV_FLAG_DISCOV		(1 << 1)
+#define MGMT_ADV_FLAG_LIMITED_DISCOV	(1 << 2)
+#define MGMT_ADV_FLAG_MANAGED_FLAGS	(1 << 3)
+#define MGMT_ADV_FLAG_TX_POWER		(1 << 4)
+#define MGMT_ADV_FLAG_APPEARANCE	(1 << 5)
+#define MGMT_ADV_FLAG_LOCAL_NAME	(1 << 6)
+
 #define MGMT_OP_REMOVE_ADVERTISING	0x003F
 struct mgmt_cp_remove_advertising {
 	__u8	instance;
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 2/7] Bluetooth: Support the "connectable mode" adv flag

[Arman Uguray]

This patch adds support for the "connectable mode" flag of the
Add Advertising command.

Signed-off-by: Arman Uguray <armansito@chromium.org>
---
 net/bluetooth/mgmt.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 38b03bd..a5995a2 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1013,11 +1013,8 @@ static void update_adv_data_for_instance(struct hci_request *req, u8 instance)
 	hci_req_add(req, HCI_OP_LE_SET_ADV_DATA, sizeof(cp), &cp);
 }
 
-static void update_adv_data(struct hci_request *req)
+static u8 get_current_adv_instance(struct hci_dev *hdev)
 {
-	struct hci_dev *hdev = req->hdev;
-	u8 instance;
-
 	/* The "Set Advertising" setting supersedes the "Add Advertising"
 	 * setting. Here we set the advertising data based on which
 	 * setting was set. When neither apply, default to the global settings,
@@ -1025,9 +1022,15 @@ static void update_adv_data(struct hci_request *req)
 	 */
 	if (hci_dev_test_flag(hdev, HCI_ADVERTISING_INSTANCE) &&
 	    !hci_dev_test_flag(hdev, HCI_ADVERTISING))
-		instance = 0x01;
-	else
-		instance = 0x00;
+		return 0x01;
+
+	return 0x00;
+}
+
+static void update_adv_data(struct hci_request *req)
+{
+	struct hci_dev *hdev = req->hdev;
+	u8 instance = get_current_adv_instance(hdev);
 
 	update_adv_data_for_instance(req, instance);
 }
@@ -1188,6 +1191,7 @@ static void enable_advertising(struct hci_request *req)
 	struct hci_cp_le_set_adv_param cp;
 	u8 own_addr_type, enable = 0x01;
 	bool connectable;
+	u8 instance;
 
 	if (hci_conn_num(hdev, LE_LINK) > 0)
 		return;
@@ -1202,7 +1206,13 @@ static void enable_advertising(struct hci_request *req)
 	 */
 	hci_dev_clear_flag(hdev, HCI_LE_ADV);
 
-	if (hci_dev_test_flag(hdev, HCI_ADVERTISING_CONNECTABLE))
+	instance = get_current_adv_instance(hdev);
+
+	if (instance == 0x01 &&
+	    hdev->adv_instance.flags & MGMT_ADV_FLAG_CONNECTABLE)
+		connectable = true;
+	else if (instance == 0x00 &&
+		 hci_dev_test_flag(hdev, HCI_ADVERTISING_CONNECTABLE))
 		connectable = true;
 	else
 		connectable = get_connectable(hdev);
@@ -6623,10 +6633,8 @@ static int add_advertising(struct sock *sk, struct hci_dev *hdev,
 	flags = __le32_to_cpu(cp->flags);
 	timeout = __le16_to_cpu(cp->timeout);
 
-	/* The current implementation only supports adding one instance and
-	 * doesn't support flags.
-	 */
-	if (cp->instance != 0x01 || flags)
+	/* The current implementation only supports adding one instance */
+	if (cp->instance != 0x01)
 		return mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_ADVERTISING,
 				       MGMT_STATUS_INVALID_PARAMS);
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 3/7] Bluetooth: Support the "discoverable" adv flag

[Arman Uguray]

This patch adds support for the "discoverable" flag of the
Add Advertising command.

Signed-off-by: Arman Uguray <armansito@chromium.org>
---
 net/bluetooth/mgmt.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a5995a2..b197dc1 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -975,13 +975,28 @@ static u8 create_default_adv_data(struct hci_dev *hdev, u8 *ptr)
 
 static u8 create_instance_adv_data(struct hci_dev *hdev, u8 *ptr)
 {
-	/* TODO: Set the appropriate entries based on advertising instance flags
-	 * here once flags other than 0 are supported.
-	 */
+	u8 ad_len = 0, flags = 0;
+
+	if (hdev->adv_instance.flags & MGMT_ADV_FLAG_DISCOV)
+		flags |= LE_AD_GENERAL;
+
+	if (flags) {
+		if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED))
+			flags |= LE_AD_NO_BREDR;
+
+		ptr[0] = 0x02;
+		ptr[1] = EIR_FLAGS;
+		ptr[2] = flags;
+
+		ad_len += 3;
+		ptr += 3;
+	}
+
 	memcpy(ptr, hdev->adv_instance.adv_data,
 	       hdev->adv_instance.adv_data_len);
+	ad_len += hdev->adv_instance.adv_data_len;
 
-	return hdev->adv_instance.adv_data_len;
+	return ad_len;
 }
 
 static void update_adv_data_for_instance(struct hci_request *req, u8 instance)
@@ -6539,12 +6554,16 @@ static int read_adv_features(struct sock *sk, struct hci_dev *hdev,
 }
 
 static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
-			      u8 len)
+			      u8 len, bool is_adv_data)
 {
 	u8 max_len = HCI_MAX_AD_LENGTH;
 	int i, cur_len;
+	bool flags_managed = false;
 
-	/* TODO: Correctly reduce len based on adv_flags. */
+	if (is_adv_data && (adv_flags & MGMT_ADV_FLAG_DISCOV)) {
+		flags_managed = true;
+		max_len -= 3;
+	}
 
 	if (len > max_len)
 		return false;
@@ -6553,6 +6572,9 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
 	for (i = 0, cur_len = 0; i < len; i += (cur_len + 1)) {
 		cur_len = data[i];
 
+		if (flags_managed && data[i + 1] == EIR_FLAGS)
+			return false;
+
 		/* If the current field length would exceed the total data
 		 * length, then it's invalid.
 		 */
@@ -6654,9 +6676,9 @@ static int add_advertising(struct sock *sk, struct hci_dev *hdev,
 		goto unlock;
 	}
 
-	if (!tlv_data_is_valid(hdev, flags, cp->data, cp->adv_data_len) ||
+	if (!tlv_data_is_valid(hdev, flags, cp->data, cp->adv_data_len, true) ||
 	    !tlv_data_is_valid(hdev, flags, cp->data + cp->adv_data_len,
-			       cp->scan_rsp_len)) {
+			       cp->scan_rsp_len, false)) {
 		err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_ADVERTISING,
 				      MGMT_STATUS_INVALID_PARAMS);
 		goto unlock;
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 4/7] Bluetooth: Support the "limited-discoverable" adv flag

[Arman Uguray]

This patch adds support for the "limited-discoverable" flag of the
Add Advertising command.

Signed-off-by: Arman Uguray <armansito@chromium.org>
---
 net/bluetooth/mgmt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index b197dc1..846546d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -977,9 +977,15 @@ static u8 create_instance_adv_data(struct hci_dev *hdev, u8 *ptr)
 {
 	u8 ad_len = 0, flags = 0;
 
+	/* The Add Advertising command allows userspace to set both the general
+	 * and limited discoverable flags.
+	 */
 	if (hdev->adv_instance.flags & MGMT_ADV_FLAG_DISCOV)
 		flags |= LE_AD_GENERAL;
 
+	if (hdev->adv_instance.flags & MGMT_ADV_FLAG_LIMITED_DISCOV)
+		flags |= LE_AD_LIMITED;
+
 	if (flags) {
 		if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED))
 			flags |= LE_AD_NO_BREDR;
@@ -6559,8 +6565,9 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
 	u8 max_len = HCI_MAX_AD_LENGTH;
 	int i, cur_len;
 	bool flags_managed = false;
+	u32 flags_params = MGMT_ADV_FLAG_DISCOV | MGMT_ADV_FLAG_LIMITED_DISCOV;
 
-	if (is_adv_data && (adv_flags & MGMT_ADV_FLAG_DISCOV)) {
+	if (is_adv_data && (adv_flags & flags_params)) {
 		flags_managed = true;
 		max_len -= 3;
 	}
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 5/7] Bluetooth: Support the "managed-flags" adv flag

[Arman Uguray]

This patch adds support for the "managed-flags" flag of the Add
Advertising command.

Signed-off-by: Arman Uguray <armansito@chromium.org>
---
 net/bluetooth/mgmt.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 846546d..9905614b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -986,7 +986,13 @@ static u8 create_instance_adv_data(struct hci_dev *hdev, u8 *ptr)
 	if (hdev->adv_instance.flags & MGMT_ADV_FLAG_LIMITED_DISCOV)
 		flags |= LE_AD_LIMITED;
 
-	if (flags) {
+	if (flags || (hdev->adv_instance.flags & MGMT_ADV_FLAG_MANAGED_FLAGS)) {
+		/* If a discovery flag wasn't provided, simply use the global
+		 * settings.
+		 */
+		if (!flags)
+			flags |= get_adv_discov_flags(hdev);
+
 		if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED))
 			flags |= LE_AD_NO_BREDR;
 
@@ -6565,7 +6571,8 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
 	u8 max_len = HCI_MAX_AD_LENGTH;
 	int i, cur_len;
 	bool flags_managed = false;
-	u32 flags_params = MGMT_ADV_FLAG_DISCOV | MGMT_ADV_FLAG_LIMITED_DISCOV;
+	u32 flags_params = MGMT_ADV_FLAG_DISCOV | MGMT_ADV_FLAG_LIMITED_DISCOV |
+			   MGMT_ADV_FLAG_MANAGED_FLAGS;
 
 	if (is_adv_data && (adv_flags & flags_params)) {
 		flags_managed = true;
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 6/7] Bluetooth: Support the "tx-power" adv flag

[Arman Uguray]

This patch adds support for the "tx-power" flag of the Add
Advertising command.

Signed-off-by: Arman Uguray <armansito@chromium.org>
---
 net/bluetooth/mgmt.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 9905614b..8795037 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1004,6 +1004,16 @@ static u8 create_instance_adv_data(struct hci_dev *hdev, u8 *ptr)
 		ptr += 3;
 	}
 
+	if (hdev->adv_tx_power != HCI_TX_POWER_INVALID &&
+	    (hdev->adv_instance.flags & MGMT_ADV_FLAG_TX_POWER)) {
+		ptr[0] = 0x02;
+		ptr[1] = EIR_TX_POWER;
+		ptr[2] = (u8)hdev->adv_tx_power;
+
+		ad_len += 3;
+		ptr += 3;
+	}
+
 	memcpy(ptr, hdev->adv_instance.adv_data,
 	       hdev->adv_instance.adv_data_len);
 	ad_len += hdev->adv_instance.adv_data_len;
@@ -6571,6 +6581,7 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
 	u8 max_len = HCI_MAX_AD_LENGTH;
 	int i, cur_len;
 	bool flags_managed = false;
+	bool tx_power_managed = false;
 	u32 flags_params = MGMT_ADV_FLAG_DISCOV | MGMT_ADV_FLAG_LIMITED_DISCOV |
 			   MGMT_ADV_FLAG_MANAGED_FLAGS;
 
@@ -6579,6 +6590,11 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
 		max_len -= 3;
 	}
 
+	if (is_adv_data && (adv_flags & MGMT_ADV_FLAG_TX_POWER)) {
+		tx_power_managed = true;
+		max_len -= 3;
+	}
+
 	if (len > max_len)
 		return false;
 
@@ -6589,6 +6605,9 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
 		if (flags_managed && data[i + 1] == EIR_FLAGS)
 			return false;
 
+		if (tx_power_managed && data[i + 1] == EIR_TX_POWER)
+			return false;
+
 		/* If the current field length would exceed the total data
 		 * length, then it's invalid.
 		 */
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 7/7] Bluetooth: Update supported_flags for AD features

[Arman Uguray]

This patch updates the "supported_flags" parameter returned from the
"Read Advertising Features" command.

Signed-off-by: Arman Uguray <armansito@chromium.org>
---
 net/bluetooth/mgmt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 8795037..a4a0d6b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -6530,6 +6530,7 @@ static int read_adv_features(struct sock *sk, struct hci_dev *hdev,
 	size_t rp_len;
 	int err;
 	bool instance;
+	u32 supported_flags;
 
 	BT_DBG("%s", hdev->name);
 
@@ -6550,7 +6551,13 @@ static int read_adv_features(struct sock *sk, struct hci_dev *hdev,
 		return -ENOMEM;
 	}
 
-	rp->supported_flags = cpu_to_le32(0);
+	supported_flags = MGMT_ADV_FLAG_CONNECTABLE;
+	supported_flags |= MGMT_ADV_FLAG_DISCOV;
+	supported_flags |= MGMT_ADV_FLAG_LIMITED_DISCOV;
+	supported_flags |= MGMT_ADV_FLAG_MANAGED_FLAGS;
+	supported_flags |= MGMT_ADV_FLAG_TX_POWER;
+
+	rp->supported_flags = cpu_to_le32(supported_flags);
 	rp->max_adv_data_len = HCI_MAX_AD_LENGTH;
 	rp->max_scan_rsp_len = HCI_MAX_AD_LENGTH;
 	rp->max_instances = 1;
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 7/7] Bluetooth: Update supported_flags for AD features

[Marcel Holtmann]

Hi Arman,

> This patch updates the "supported_flags" parameter returned from the
> "Read Advertising Features" command.
> 
> Signed-off-by: Arman Uguray <armansito@chromium.org>
> ---
> net/bluetooth/mgmt.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 8795037..a4a0d6b 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -6530,6 +6530,7 @@ static int read_adv_features(struct sock *sk, struct hci_dev *hdev,
> 	size_t rp_len;
> 	int err;
> 	bool instance;
> +	u32 supported_flags;
> 
> 	BT_DBG("%s", hdev->name);
> 
> @@ -6550,7 +6551,13 @@ static int read_adv_features(struct sock *sk, struct hci_dev *hdev,
> 		return -ENOMEM;
> 	}
> 
> -	rp->supported_flags = cpu_to_le32(0);
> +	supported_flags = MGMT_ADV_FLAG_CONNECTABLE;
> +	supported_flags |= MGMT_ADV_FLAG_DISCOV;
> +	supported_flags |= MGMT_ADV_FLAG_LIMITED_DISCOV;
> +	supported_flags |= MGMT_ADV_FLAG_MANAGED_FLAGS;
> +	supported_flags |= MGMT_ADV_FLAG_TX_POWER;
> +

	supported_flags = MGMT_foo |
			  MGMT_bar;

I would prefer we do it this way.

> +	rp->supported_flags = cpu_to_le32(supported_flags);
> 	rp->max_adv_data_len = HCI_MAX_AD_LENGTH;
> 	rp->max_scan_rsp_len = HCI_MAX_AD_LENGTH;
> 	rp->max_instances = 1;

Regards

Marcel

Re: [PATCH 2/7] Bluetooth: Support the "connectable mode" adv flag

[Marcel Holtmann]

Hi Arman,

> This patch adds support for the "connectable mode" flag of the
> Add Advertising command.
> 
> Signed-off-by: Arman Uguray <armansito@chromium.org>
> ---
> net/bluetooth/mgmt.c | 32 ++++++++++++++++++++------------
> 1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 38b03bd..a5995a2 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1013,11 +1013,8 @@ static void update_adv_data_for_instance(struct hci_request *req, u8 instance)
> 	hci_req_add(req, HCI_OP_LE_SET_ADV_DATA, sizeof(cp), &cp);
> }
> 
> -static void update_adv_data(struct hci_request *req)
> +static u8 get_current_adv_instance(struct hci_dev *hdev)
> {
> -	struct hci_dev *hdev = req->hdev;
> -	u8 instance;
> -
> 	/* The "Set Advertising" setting supersedes the "Add Advertising"
> 	 * setting. Here we set the advertising data based on which
> 	 * setting was set. When neither apply, default to the global settings,
> @@ -1025,9 +1022,15 @@ static void update_adv_data(struct hci_request *req)
> 	 */
> 	if (hci_dev_test_flag(hdev, HCI_ADVERTISING_INSTANCE) &&
> 	    !hci_dev_test_flag(hdev, HCI_ADVERTISING))
> -		instance = 0x01;
> -	else
> -		instance = 0x00;
> +		return 0x01;
> +
> +	return 0x00;
> +}
> +
> +static void update_adv_data(struct hci_request *req)
> +{
> +	struct hci_dev *hdev = req->hdev;
> +	u8 instance = get_current_adv_instance(hdev);
> 
> 	update_adv_data_for_instance(req, instance);
> }
> @@ -1188,6 +1191,7 @@ static void enable_advertising(struct hci_request *req)
> 	struct hci_cp_le_set_adv_param cp;
> 	u8 own_addr_type, enable = 0x01;
> 	bool connectable;
> +	u8 instance;
> 
> 	if (hci_conn_num(hdev, LE_LINK) > 0)
> 		return;
> @@ -1202,7 +1206,13 @@ static void enable_advertising(struct hci_request *req)
> 	 */
> 	hci_dev_clear_flag(hdev, HCI_LE_ADV);
> 
> -	if (hci_dev_test_flag(hdev, HCI_ADVERTISING_CONNECTABLE))
> +	instance = get_current_adv_instance(hdev);
> +
> +	if (instance == 0x01 &&
> +	    hdev->adv_instance.flags & MGMT_ADV_FLAG_CONNECTABLE)
> +		connectable = true;
> +	else if (instance == 0x00 &&
> +		 hci_dev_test_flag(hdev, HCI_ADVERTISING_CONNECTABLE))
> 		connectable = true;
> 	else
> 		connectable = get_connectable(hdev);

this code seems testing the same thing over and over again. I have the feeling this needs a bit smart way of figuring out the flags and instance ids.

> @@ -6623,10 +6633,8 @@ static int add_advertising(struct sock *sk, struct hci_dev *hdev,
> 	flags = __le32_to_cpu(cp->flags);
> 	timeout = __le16_to_cpu(cp->timeout);
> 
> -	/* The current implementation only supports adding one instance and
> -	 * doesn't support flags.
> -	 */
> -	if (cp->instance != 0x01 || flags)
> +	/* The current implementation only supports adding one instance */
> +	if (cp->instance != 0x01)
> 		return mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_ADVERTISING,
> 				       MGMT_STATUS_INVALID_PARAMS);

I would prefer that one we take this check out, we add a new one that check that only supported flags are given. It is also fine to do this in a second patch or the one that changes the supported flags.

Regards

Marcel

Re: [PATCH 1/7] Bluetooth: Add macros for advertising instance flags

[Marcel Holtmann]

Hi Arman,

> This patch adds macro definitions for possible advertising instance
> flags that can be passed to the "Add Advertising" command.
> 
> Signed-off-by: Arman Uguray <armansito@chromium.org>
> ---
> include/net/bluetooth/mgmt.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 68abd4b..fc50cee 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -554,6 +554,14 @@ struct mgmt_rp_add_advertising {
> 	__u8	instance;
> } __packed;
> 
> +#define MGMT_ADV_FLAG_CONNECTABLE	(1 << 0)
> +#define MGMT_ADV_FLAG_DISCOV		(1 << 1)
> +#define MGMT_ADV_FLAG_LIMITED_DISCOV	(1 << 2)
> +#define MGMT_ADV_FLAG_MANAGED_FLAGS	(1 << 3)
> +#define MGMT_ADV_FLAG_TX_POWER		(1 << 4)
> +#define MGMT_ADV_FLAG_APPEARANCE	(1 << 5)
> +#define MGMT_ADV_FLAG_LOCAL_NAME	(1 << 6)
> +

any reason to not use BIT(0) etc. here.

Regards

Marcel

Re: [PATCH 1/7] Bluetooth: Add macros for advertising instance flags

[Arman Uguray]

Hi Marcel,

> On Wed, Mar 25, 2015 at 9:21 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Arman,
>
>> This patch adds macro definitions for possible advertising instance
>> flags that can be passed to the "Add Advertising" command.
>>
>> Signed-off-by: Arman Uguray <armansito@chromium.org>
>> ---
>> include/net/bluetooth/mgmt.h | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
>> index 68abd4b..fc50cee 100644
>> --- a/include/net/bluetooth/mgmt.h
>> +++ b/include/net/bluetooth/mgmt.h
>> @@ -554,6 +554,14 @@ struct mgmt_rp_add_advertising {
>>       __u8    instance;
>> } __packed;
>>
>> +#define MGMT_ADV_FLAG_CONNECTABLE    (1 << 0)
>> +#define MGMT_ADV_FLAG_DISCOV         (1 << 1)
>> +#define MGMT_ADV_FLAG_LIMITED_DISCOV (1 << 2)
>> +#define MGMT_ADV_FLAG_MANAGED_FLAGS  (1 << 3)
>> +#define MGMT_ADV_FLAG_TX_POWER               (1 << 4)
>> +#define MGMT_ADV_FLAG_APPEARANCE     (1 << 5)
>> +#define MGMT_ADV_FLAG_LOCAL_NAME     (1 << 6)
>> +
>
> any reason to not use BIT(0) etc. here.
>

No reason at all, fixed for next patch set.

> Regards
>
> Marcel
>

Arman

Re: [PATCH 2/7] Bluetooth: Support the "connectable mode" adv flag

[Arman Uguray]

Hi Marcel,

> On Wed, Mar 25, 2015 at 9:21 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Arman,
>
>> This patch adds support for the "connectable mode" flag of the
>> Add Advertising command.
>>
>> Signed-off-by: Arman Uguray <armansito@chromium.org>
>> ---
>> net/bluetooth/mgmt.c | 32 ++++++++++++++++++++------------
>> 1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 38b03bd..a5995a2 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -1013,11 +1013,8 @@ static void update_adv_data_for_instance(struct hci_request *req, u8 instance)
>>       hci_req_add(req, HCI_OP_LE_SET_ADV_DATA, sizeof(cp), &cp);
>> }
>>
>> -static void update_adv_data(struct hci_request *req)
>> +static u8 get_current_adv_instance(struct hci_dev *hdev)
>> {
>> -     struct hci_dev *hdev = req->hdev;
>> -     u8 instance;
>> -
>>       /* The "Set Advertising" setting supersedes the "Add Advertising"
>>        * setting. Here we set the advertising data based on which
>>        * setting was set. When neither apply, default to the global settings,
>> @@ -1025,9 +1022,15 @@ static void update_adv_data(struct hci_request *req)
>>        */
>>       if (hci_dev_test_flag(hdev, HCI_ADVERTISING_INSTANCE) &&
>>           !hci_dev_test_flag(hdev, HCI_ADVERTISING))
>> -             instance = 0x01;
>> -     else
>> -             instance = 0x00;
>> +             return 0x01;
>> +
>> +     return 0x00;
>> +}
>> +
>> +static void update_adv_data(struct hci_request *req)
>> +{
>> +     struct hci_dev *hdev = req->hdev;
>> +     u8 instance = get_current_adv_instance(hdev);
>>
>>       update_adv_data_for_instance(req, instance);
>> }
>> @@ -1188,6 +1191,7 @@ static void enable_advertising(struct hci_request *req)
>>       struct hci_cp_le_set_adv_param cp;
>>       u8 own_addr_type, enable = 0x01;
>>       bool connectable;
>> +     u8 instance;
>>
>>       if (hci_conn_num(hdev, LE_LINK) > 0)
>>               return;
>> @@ -1202,7 +1206,13 @@ static void enable_advertising(struct hci_request *req)
>>        */
>>       hci_dev_clear_flag(hdev, HCI_LE_ADV);
>>
>> -     if (hci_dev_test_flag(hdev, HCI_ADVERTISING_CONNECTABLE))
>> +     instance = get_current_adv_instance(hdev);
>> +
>> +     if (instance == 0x01 &&
>> +         hdev->adv_instance.flags & MGMT_ADV_FLAG_CONNECTABLE)
>> +             connectable = true;
>> +     else if (instance == 0x00 &&
>> +              hci_dev_test_flag(hdev, HCI_ADVERTISING_CONNECTABLE))
>>               connectable = true;
>>       else
>>               connectable = get_connectable(hdev);
>
> this code seems testing the same thing over and over again. I have the feeling this needs a bit smart way of figuring out the flags and instance ids.
>

I refactored the code a little bit in this patch. What I'll do is to
add another patch to the end of the set that unifies the default vs
instance AD flow, that should make things a little nicer.

>> @@ -6623,10 +6633,8 @@ static int add_advertising(struct sock *sk, struct hci_dev *hdev,
>>       flags = __le32_to_cpu(cp->flags);
>>       timeout = __le16_to_cpu(cp->timeout);
>>
>> -     /* The current implementation only supports adding one instance and
>> -      * doesn't support flags.
>> -      */
>> -     if (cp->instance != 0x01 || flags)
>> +     /* The current implementation only supports adding one instance */
>> +     if (cp->instance != 0x01)
>>               return mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_ADVERTISING,
>>                                      MGMT_STATUS_INVALID_PARAMS);
>
> I would prefer that one we take this check out, we add a new one that check that only supported flags are given. It is also fine to do this in a second patch or the one that changes the supported flags.
>

I decided to put this into the supported flags patch that I already have.

> Regards
>
> Marcel
>

Thanks,
Arman