[RFC 0/3] Bluetooth: Add a new mgmt_set_bredr command

[RFC 0/3] Bluetooth: Add a new mgmt_set_bredr command

[johan.hedberg]

Hi,

This patch set adds a new mgmt_set_bredr command that can be used to
make a dual mode controller behave like a single mode one. I'm sending
it as RFC since I haven't had the chance to test it properly yet.

Johan

----------------------------------------------------------------
Johan Hedberg (3):
      Bluetooth: Introduce a new HCI_BREDR_ENABLED flag
      Bluetooth: Move set_bredr_scan() to a more convenient location
      Bluetooth: Add a new mgmt_set_bredr command

 include/net/bluetooth/hci.h  |   1 +
 include/net/bluetooth/mgmt.h |   2 +
 net/bluetooth/hci_conn.c     |   3 +
 net/bluetooth/hci_core.c     |  21 ++++-
 net/bluetooth/mgmt.c         | 186 ++++++++++++++++++++++++++++++++++-------
 5 files changed, 180 insertions(+), 33 deletions(-)

[RFC 1/3] Bluetooth: Introduce a new HCI_BREDR_ENABLED flag

[johan.hedberg]

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

To allow treating dual-mode (BR/EDR/LE) controllers as single-mode ones
(LE-only) we want to introduce a new HCI_BREDR_ENABLED flag to track
whether BR/EDR is enabled or not (previously we simply looked at the
feature bit with lmp_bredr_enabled).

This patch add the new flag and updates the relevant places to test
against it instead of using lmp_bredr_enabled. The flag is by default
enabled when registering an adapter and only cleared if necessary once
the local features have been read during the HCI init procedure.

We cannot completely block BR/EDR usage in case user space uses raw HCI
sockets but the patch tries to block this in places where possible, such
as the various BR/EDR specific ioctls.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_conn.c    |  3 +++
 net/bluetooth/hci_core.c    | 21 +++++++++++++++++++--
 net/bluetooth/mgmt.c        | 24 +++++++++++++-----------
 4 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 7ede266..4fa08d7 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -122,6 +122,7 @@ enum {
 	HCI_LINK_SECURITY,
 	HCI_PERIODIC_INQ,
 	HCI_FAST_CONNECTABLE,
+	HCI_BREDR_ENABLED,
 };
 
 /* A mask for the flags that are supposed to remain when a reset happens
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index d2380e0..514148b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -581,6 +581,9 @@ static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
 {
 	struct hci_conn *acl;
 
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
+		return ERR_PTR(-ENOTSUPP);
+
 	acl = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
 	if (!acl) {
 		acl = hci_conn_add(hdev, ACL_LINK, dst);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 750c360..c16a49f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -519,6 +519,8 @@ static void hci_init2_req(struct hci_request *req, unsigned long opt)
 
 	if (lmp_bredr_capable(hdev))
 		bredr_setup(req);
+	else
+		clear_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
 
 	if (lmp_le_capable(hdev))
 		le_setup(req);
@@ -1034,6 +1036,11 @@ int hci_inquiry(void __user *arg)
 		goto done;
 	}
 
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
+		err = -ENOTSUPP;
+		goto done;
+	}
+
 	hci_dev_lock(hdev);
 	if (inquiry_cache_age(hdev) > INQUIRY_CACHE_AGE_MAX ||
 	    inquiry_cache_empty(hdev) || ir.flags & IREQ_CACHE_FLUSH) {
@@ -1101,7 +1108,7 @@ static u8 create_ad(struct hci_dev *hdev, u8 *ptr)
 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
 		flags |= LE_AD_GENERAL;
 
-	if (!lmp_bredr_capable(hdev))
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
 		flags |= LE_AD_NO_BREDR;
 
 	if (lmp_le_br_capable(hdev))
@@ -1473,6 +1480,11 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg)
 		goto done;
 	}
 
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
+		err = -ENOTSUPP;
+		goto done;
+	}
+
 	switch (cmd) {
 	case HCISETAUTH:
 		err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt,
@@ -2298,8 +2310,13 @@ int hci_register_dev(struct hci_dev *hdev)
 
 	set_bit(HCI_SETUP, &hdev->dev_flags);
 
-	if (hdev->dev_type != HCI_AMP)
+	if (hdev->dev_type != HCI_AMP) {
 		set_bit(HCI_AUTO_OFF, &hdev->dev_flags);
+		/* Assume BR/EDR support until proven otherwise (such as
+		 * through reading supported features during init.
+		 */
+		set_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+	}
 
 	write_lock(&hci_dev_list_lock);
 	list_add(&hdev->list, &hci_dev_list);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ad38629..e1c41b0 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -408,7 +408,7 @@ static u32 get_current_settings(struct hci_dev *hdev)
 	if (test_bit(HCI_PAIRABLE, &hdev->dev_flags))
 		settings |= MGMT_SETTING_PAIRABLE;
 
-	if (lmp_bredr_capable(hdev))
+	if (test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
 		settings |= MGMT_SETTING_BREDR;
 
 	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
@@ -929,7 +929,7 @@ 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))
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
 				 MGMT_STATUS_NOT_SUPPORTED);
 
@@ -1085,7 +1085,7 @@ 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))
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_CONNECTABLE,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
@@ -1208,7 +1208,7 @@ 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))
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
@@ -1342,7 +1342,7 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 
 	BT_DBG("request for %s", hdev->name);
 
-	if (!lmp_bredr_capable(hdev))
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
@@ -1409,7 +1409,7 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 				  MGMT_STATUS_INVALID_PARAMS);
 
 	/* LE-only devices do not allow toggling LE on/off */
-	if (!lmp_bredr_capable(hdev))
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
 				  MGMT_STATUS_REJECTED);
 
@@ -1720,7 +1720,7 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("request for %s", hdev->name);
 
-	if (!lmp_bredr_capable(hdev))
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
@@ -2803,7 +2803,7 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 
 	switch (hdev->discovery.type) {
 	case DISCOV_TYPE_BREDR:
-		if (!lmp_bredr_capable(hdev)) {
+		if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
 			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
 					 MGMT_STATUS_NOT_SUPPORTED);
 			mgmt_pending_remove(cmd);
@@ -2835,7 +2835,7 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 		}
 
 		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
-		    !lmp_bredr_capable(hdev)) {
+		    !test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
 			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
 					 MGMT_STATUS_NOT_SUPPORTED);
 			mgmt_pending_remove(cmd);
@@ -3282,7 +3282,8 @@ static int set_fast_connectable(struct sock *sk, struct hci_dev *hdev,
 
 	BT_DBG("%s", hdev->name);
 
-	if (!lmp_bredr_capable(hdev) || hdev->hci_ver < BLUETOOTH_VER_1_2)
+	if (!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags) ||
+	    hdev->hci_ver < BLUETOOTH_VER_1_2)
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
@@ -3646,7 +3647,8 @@ static int powered_update_hci(struct hci_dev *hdev)
 			    sizeof(link_sec), &link_sec);
 
 	if (lmp_bredr_capable(hdev)) {
-		set_bredr_scan(&req);
+		if (test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags))
+			set_bredr_scan(&req);
 		update_class(&req);
 		update_name(&req);
 		update_eir(&req);
-- 
1.8.3.1

Re: [RFC 1/3] Bluetooth: Introduce a new HCI_BREDR_ENABLED flag

[Marcel Holtmann]

Hi Johan,

> To allow treating dual-mode (BR/EDR/LE) controllers as single-mode ones
> (LE-only) we want to introduce a new HCI_BREDR_ENABLED flag to track
> whether BR/EDR is enabled or not (previously we simply looked at the
> feature bit with lmp_bredr_enabled).
> 
> This patch add the new flag and updates the relevant places to test
> against it instead of using lmp_bredr_enabled. The flag is by default
> enabled when registering an adapter and only cleared if necessary once
> the local features have been read during the HCI init procedure.
> 
> We cannot completely block BR/EDR usage in case user space uses raw HCI
> sockets but the patch tries to block this in places where possible, such
> as the various BR/EDR specific ioctls.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci.h |  1 +
> net/bluetooth/hci_conn.c    |  3 +++
> net/bluetooth/hci_core.c    | 21 +++++++++++++++++++--
> net/bluetooth/mgmt.c        | 24 +++++++++++++-----------
> 4 files changed, 36 insertions(+), 13 deletions(-)

unless you missed a spot, this patch looks pretty much correct.

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

Regards

Marcel

[RFC 2/3] Bluetooth: Move set_bredr_scan() to a more convenient location

[johan.hedberg]

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

The static set_bredr_scan function will soon be reused by a new
set_bredr method. This trivial patch moves the function to a new
location within the mgmt.c file where it can be reused without requiring
forward declarations.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e1c41b0..eea0b97 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3147,6 +3147,26 @@ static int set_device_id(struct sock *sk, struct hci_dev *hdev, void *data,
 	return err;
 }
 
+static void set_bredr_scan(struct hci_request *req)
+{
+	struct hci_dev *hdev = req->hdev;
+	u8 scan = 0;
+
+	/* Ensure that fast connectable is disabled. This function will
+	 * not do anything if the page scan parameters are already what
+	 * they should be.
+	 */
+	write_fast_connectable(req, false);
+
+	if (test_bit(HCI_CONNECTABLE, &hdev->dev_flags))
+		scan |= SCAN_PAGE;
+	if (test_bit(HCI_DISCOVERABLE, &hdev->dev_flags))
+		scan |= SCAN_INQUIRY;
+
+	if (scan)
+		hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
+}
+
 static void set_advertising_complete(struct hci_dev *hdev, u8 status)
 {
 	struct cmd_lookup match = { NULL, hdev };
@@ -3567,26 +3587,6 @@ int mgmt_index_removed(struct hci_dev *hdev)
 	return mgmt_event(MGMT_EV_INDEX_REMOVED, hdev, NULL, 0, NULL);
 }
 
-static void set_bredr_scan(struct hci_request *req)
-{
-	struct hci_dev *hdev = req->hdev;
-	u8 scan = 0;
-
-	/* Ensure that fast connectable is disabled. This function will
-	 * not do anything if the page scan parameters are already what
-	 * they should be.
-	 */
-	write_fast_connectable(req, false);
-
-	if (test_bit(HCI_CONNECTABLE, &hdev->dev_flags))
-		scan |= SCAN_PAGE;
-	if (test_bit(HCI_DISCOVERABLE, &hdev->dev_flags))
-		scan |= SCAN_INQUIRY;
-
-	if (scan)
-		hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
-}
-
 static void powered_complete(struct hci_dev *hdev, u8 status)
 {
 	struct cmd_lookup match = { NULL, hdev };
-- 
1.8.3.1

Re: [RFC 2/3] Bluetooth: Move set_bredr_scan() to a more convenient location

[Marcel Holtmann]

Hi Johan,

> The static set_bredr_scan function will soon be reused by a new
> set_bredr method. This trivial patch moves the function to a new
> location within the mgmt.c file where it can be reused without requiring
> forward declarations.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 40 ++++++++++++++++++++--------------------
> 1 file changed, 20 insertions(+), 20 deletions(-)

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

Regards

Marcel

[RFC 3/3] Bluetooth: Add a new mgmt_set_bredr command

[johan.hedberg]

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

This patch introduces a new mgmt command for enabling/disabling BR/EDR
functionality. This can be convenient when one wants to make a dual-mode
controller behave like a single-mode one. The command is only available
for dual-mode controllers and requires that LE is enabled before using
it.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/mgmt.h |   2 +
 net/bluetooth/mgmt.c         | 122 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)

diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 421d763..7347df8 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -354,6 +354,8 @@ struct mgmt_cp_set_device_id {
 
 #define MGMT_OP_SET_ADVERTISING		0x0029
 
+#define MGMT_OP_SET_BREDR		0x002A
+
 #define MGMT_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
 	__le16	opcode;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index eea0b97..6405406 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -75,6 +75,7 @@ static const u16 mgmt_commands[] = {
 	MGMT_OP_UNBLOCK_DEVICE,
 	MGMT_OP_SET_DEVICE_ID,
 	MGMT_OP_SET_ADVERTISING,
+	MGMT_OP_SET_BREDR,
 };
 
 static const u16 mgmt_events[] = {
@@ -3259,6 +3260,126 @@ unlock:
 	return err;
 }
 
+static void set_no_scan(struct hci_request *req)
+{
+	struct hci_dev *hdev = req->hdev;
+	u8 scan = 0x00;
+
+	if (!test_bit(HCI_ISCAN, &hdev->flags) &&
+	    !test_bit(HCI_PSCAN, &hdev->flags))
+		return;
+
+	hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, sizeof(scan), &scan);
+}
+
+static void set_bredr_complete(struct hci_dev *hdev, u8 status)
+{
+	struct pending_cmd *cmd;
+
+	BT_DBG("status 0x%02x", status);
+
+	hci_dev_lock(hdev);
+
+	cmd = mgmt_pending_find(MGMT_OP_SET_BREDR, hdev);
+	if (!cmd)
+		goto unlock;
+
+	if (status) {
+		u8 mgmt_err = mgmt_status(status);
+
+		/* We need to restore the flag if related HCI commands
+		 * failed.
+		 */
+		change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+
+		cmd_status(cmd->sk, cmd->index, cmd->opcode, mgmt_err);
+	} else {
+		send_settings_rsp(cmd->sk, MGMT_OP_SET_BREDR, hdev);
+	}
+
+	mgmt_pending_remove(cmd);
+
+unlock:
+	hci_dev_unlock(hdev);
+}
+
+static int set_bredr(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
+{
+	struct mgmt_mode *cp = data;
+	struct pending_cmd *cmd;
+	struct hci_request req;
+	u8 val, enabled;
+	int err;
+
+	BT_DBG("request for %s", hdev->name);
+
+	if (!lmp_bredr_capable(hdev) || !lmp_le_capable(hdev))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
+				  MGMT_STATUS_REJECTED);
+
+	if (!test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
+				  MGMT_STATUS_REJECTED);
+
+	if (cp->val != 0x00 && cp->val != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
+				  MGMT_STATUS_INVALID_PARAMS);
+
+	hci_dev_lock(hdev);
+
+	val = !!cp->val;
+	enabled = test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+
+	if (!hdev_is_powered(hdev) || val == enabled) {
+		bool changed = false;
+
+		if (val != test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
+			change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+			changed = true;
+		}
+
+		err = send_settings_rsp(sk, MGMT_OP_SET_BREDR, hdev);
+		if (err < 0)
+			goto unlock;
+
+		if (changed)
+			err = new_settings(hdev, sk);
+
+		goto unlock;
+	}
+
+	if (mgmt_pending_find(MGMT_OP_SET_BREDR, hdev)) {
+		err = cmd_status(sk, hdev->id, MGMT_OP_SET_BREDR,
+				 MGMT_STATUS_BUSY);
+		goto unlock;
+	}
+
+	cmd = mgmt_pending_add(sk, MGMT_OP_SET_BREDR, hdev, data, len);
+	if (!cmd) {
+		err = -ENOMEM;
+		goto unlock;
+	}
+
+	change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
+
+	hci_req_init(&req, hdev);
+
+	if (val)
+		set_bredr_scan(&req);
+	else
+		set_no_scan(&req);
+
+	hci_update_ad(&req);
+
+	err = hci_req_run(&req, set_bredr_complete);
+	if (err < 0)
+		mgmt_pending_remove(cmd);
+
+unlock:
+	hci_dev_unlock(hdev);
+	return err;
+}
+
 static void fast_connectable_complete(struct hci_dev *hdev, u8 status)
 {
 	struct pending_cmd *cmd;
@@ -3472,6 +3593,7 @@ static const struct mgmt_handler {
 	{ unblock_device,         false, MGMT_UNBLOCK_DEVICE_SIZE },
 	{ set_device_id,          false, MGMT_SET_DEVICE_ID_SIZE },
 	{ set_advertising,        false, MGMT_SETTING_SIZE },
+	{ set_bredr,              false, MGMT_SETTING_SIZE },
 };
 
 
-- 
1.8.3.1

Re: [RFC 3/3] Bluetooth: Add a new mgmt_set_bredr command

[Anderson Lizardo]

Hi Johan,

On Tue, Oct 1, 2013 at 11:22 AM,  <johan.hedberg@gmail.com> wrote:
> +       if (!lmp_bredr_capable(hdev) || !lmp_le_capable(hdev))
> +               return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +                                 MGMT_STATUS_REJECTED);
> +
> +       if (!test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
> +               return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +                                 MGMT_STATUS_REJECTED);
> +
> +       if (cp->val != 0x00 && cp->val != 0x01)
> +               return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +                                 MGMT_STATUS_INVALID_PARAMS);

Looks like the above 3 cmd_status() should use MGMT_OP_SET_BREDR.

> +
> +       hci_dev_lock(hdev);
> +
> +       val = !!cp->val;
> +       enabled = test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +
> +       if (!hdev_is_powered(hdev) || val == enabled) {
> +               bool changed = false;
> +
> +               if (val != test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
> +                       change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +                       changed = true;
> +               }

IMHO the trick used in another patch from Marcel (sent a while ago for
enabling/disabling HS) is easier to follow, something like (may
require adaptation if the logic is not the same than MGMT_OP_SET_HS):

if (cp->val)
    changed = !test_and_set_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
else
    changed = test_and_clear_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);


> +
> +               err = send_settings_rsp(sk, MGMT_OP_SET_BREDR, hdev);
> +               if (err < 0)
> +                       goto unlock;
> +
> +               if (changed)
> +                       err = new_settings(hdev, sk);
> +
> +               goto unlock;
> +       }

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

Re: [RFC 3/3] Bluetooth: Add a new mgmt_set_bredr command

[Marcel Holtmann]

Hi Johan,

> This patch introduces a new mgmt command for enabling/disabling BR/EDR
> functionality. This can be convenient when one wants to make a dual-mode
> controller behave like a single-mode one. The command is only available
> for dual-mode controllers and requires that LE is enabled before using
> it.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/mgmt.h |   2 +
> net/bluetooth/mgmt.c         | 122 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 124 insertions(+)
> 
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 421d763..7347df8 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -354,6 +354,8 @@ struct mgmt_cp_set_device_id {
> 
> #define MGMT_OP_SET_ADVERTISING		0x0029
> 
> +#define MGMT_OP_SET_BREDR		0x002A
> +
> #define MGMT_EV_CMD_COMPLETE		0x0001
> struct mgmt_ev_cmd_complete {
> 	__le16	opcode;
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index eea0b97..6405406 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -75,6 +75,7 @@ static const u16 mgmt_commands[] = {
> 	MGMT_OP_UNBLOCK_DEVICE,
> 	MGMT_OP_SET_DEVICE_ID,
> 	MGMT_OP_SET_ADVERTISING,
> +	MGMT_OP_SET_BREDR,
> };
> 
> static const u16 mgmt_events[] = {
> @@ -3259,6 +3260,126 @@ unlock:
> 	return err;
> }
> 
> +static void set_no_scan(struct hci_request *req)
> +{
> +	struct hci_dev *hdev = req->hdev;
> +	u8 scan = 0x00;
> +
> +	if (!test_bit(HCI_ISCAN, &hdev->flags) &&
> +	    !test_bit(HCI_PSCAN, &hdev->flags))
> +		return;

this one needs a comment on why we do it this way. I had to read it twice to get it.

> +
> +	hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, sizeof(scan), &scan);
> +}
> +
> +static void set_bredr_complete(struct hci_dev *hdev, u8 status)
> +{
> +	struct pending_cmd *cmd;
> +
> +	BT_DBG("status 0x%02x", status);
> +
> +	hci_dev_lock(hdev);
> +
> +	cmd = mgmt_pending_find(MGMT_OP_SET_BREDR, hdev);
> +	if (!cmd)
> +		goto unlock;
> +
> +	if (status) {
> +		u8 mgmt_err = mgmt_status(status);
> +
> +		/* We need to restore the flag if related HCI commands
> +		 * failed.
> +		 */
> +		change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +
> +		cmd_status(cmd->sk, cmd->index, cmd->opcode, mgmt_err);
> +	} else {
> +		send_settings_rsp(cmd->sk, MGMT_OP_SET_BREDR, hdev);
> +	}

Who is sending the new settings event when this succeeds?

> +
> +	mgmt_pending_remove(cmd);
> +
> +unlock:
> +	hci_dev_unlock(hdev);
> +}
> +
> +static int set_bredr(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> +{
> +	struct mgmt_mode *cp = data;
> +	struct pending_cmd *cmd;
> +	struct hci_request req;
> +	u8 val, enabled;
> +	int err;
> +
> +	BT_DBG("request for %s", hdev->name);
> +
> +	if (!lmp_bredr_capable(hdev) || !lmp_le_capable(hdev))
> +		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +				  MGMT_STATUS_REJECTED);
> +
> +	if (!test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
> +		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +				  MGMT_STATUS_REJECTED);
> +
> +	if (cp->val != 0x00 && cp->val != 0x01)
> +		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +				  MGMT_STATUS_INVALID_PARAMS);

As already pointed out, copy & paste mistake here.

In addition we might either want to reject this command when HCI_HS_ENABLED or maybe even better also disable HS at the same time. I prefer just disabling HS since we do a similar thing with discoverable when turning off connectable.

The same applies to connectable and advertising and fast connectable. I think these settings need to be cleared as well. Essentially all settings that require BR/EDR need to be cleared.

> +
> +	hci_dev_lock(hdev);
> +
> +	val = !!cp->val;
> +	enabled = test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +
> +	if (!hdev_is_powered(hdev) || val == enabled) {
> +		bool changed = false;
> +
> +		if (val != test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
> +			change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +			changed = true;
> +		}
> +
> +		err = send_settings_rsp(sk, MGMT_OP_SET_BREDR, hdev);
> +		if (err < 0)
> +			goto unlock;
> +
> +		if (changed)
> +			err = new_settings(hdev, sk);
> +
> +		goto unlock;
> +	}

I keep seeing this snippet over and over again. And actually the check if changed is done twice here. First we check if val == enabled and then we check it again and set changed = true. This looks a bit redundant.

What we should do is run the check for if the value actually changed and if not, just send the response and exit. And only then do the check for the case when the controller is not powered.

> +
> +	if (mgmt_pending_find(MGMT_OP_SET_BREDR, hdev)) {
> +		err = cmd_status(sk, hdev->id, MGMT_OP_SET_BREDR,
> +				 MGMT_STATUS_BUSY);
> +		goto unlock;
> +	}
> +
> +	cmd = mgmt_pending_add(sk, MGMT_OP_SET_BREDR, hdev, data, len);
> +	if (!cmd) {
> +		err = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +
> +	hci_req_init(&req, hdev);
> +
> +	if (val)
> +		set_bredr_scan(&req);
> +	else
> +		set_no_scan(&req);
> +
> +	hci_update_ad(&req);
> +
> +	err = hci_req_run(&req, set_bredr_complete);
> +	if (err < 0)
> +		mgmt_pending_remove(cmd);
> +
> +unlock:
> +	hci_dev_unlock(hdev);
> +	return err;
> +}
> +
> static void fast_connectable_complete(struct hci_dev *hdev, u8 status)
> {
> 	struct pending_cmd *cmd;

On a total unrelated note, why is the fast connectable code at the bottom. I think that should be rearranged to be in the same order as the opcodes.

> @@ -3472,6 +3593,7 @@ static const struct mgmt_handler {
> 	{ unblock_device,         false, MGMT_UNBLOCK_DEVICE_SIZE },
> 	{ set_device_id,          false, MGMT_SET_DEVICE_ID_SIZE },
> 	{ set_advertising,        false, MGMT_SETTING_SIZE },
> +	{ set_bredr,              false, MGMT_SETTING_SIZE },
> };

Regards

Marcel

Re: [RFC 3/3] Bluetooth: Add a new mgmt_set_bredr command

[Andrei Emeltchenko]

Hi Johan,

On Tue, Oct 01, 2013 at 06:22:02PM +0300, johan.hedberg@gmail.com wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> This patch introduces a new mgmt command for enabling/disabling BR/EDR
> functionality. This can be convenient when one wants to make a dual-mode
> controller behave like a single-mode one. The command is only available
> for dual-mode controllers and requires that LE is enabled before using
> it.

...

> +
> +	if (cp->val != 0x00 && cp->val != 0x01)
> +		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +				  MGMT_STATUS_INVALID_PARAMS);
> +
> +	hci_dev_lock(hdev);
> +
> +	val = !!cp->val;

You have already checked that cp->val is 0x0 or 0x1, right?

Best regards 
Andrei Emeltchenko 

Re: [RFC 3/3] Bluetooth: Add a new mgmt_set_bredr command

[Johan Hedberg]

Hi Andrei,

On Wed, Oct 02, 2013, Andrei Emeltchenko wrote:
> On Tue, Oct 01, 2013 at 06:22:02PM +0300, johan.hedberg@gmail.com wrote:
> > From: Johan Hedberg <johan.hedberg@intel.com>
> > 
> > This patch introduces a new mgmt command for enabling/disabling BR/EDR
> > functionality. This can be convenient when one wants to make a dual-mode
> > controller behave like a single-mode one. The command is only available
> > for dual-mode controllers and requires that LE is enabled before using
> > it.
> 
> ...
> 
> > +
> > +	if (cp->val != 0x00 && cp->val != 0x01)
> > +		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> > +				  MGMT_STATUS_INVALID_PARAMS);
> > +
> > +	hci_dev_lock(hdev);
> > +
> > +	val = !!cp->val;
> 
> You have already checked that cp->val is 0x0 or 0x1, right?

Good point (this was inherited from code that I copied it from). I've
removed both val and enabled in my latest set.

Johan