[PATCH 0/5] Bluetooth: Clean up Set Connectable related code

[PATCH 0/5] Bluetooth: Clean up Set Connectable related code

[johan.hedberg]

Hi,

This is a set of cleanup patches (and one fix) split out from my earlier
set. I'm hoping to get these in first to make it easier to understand
how the code works and what the possible ways of adding LE Advertising
hooks are.

The main thing with these patches is that we make a clear separation of
what happens if someone uses a raw HCI socket compared to sending a Set
Connectable command using the management interface. Now both cases have
their clear functions for handling the HCI_CONNECTABLE flag and the
necessary mgmt events.

----------------------------------------------------------------
Johan Hedberg (5):
      Bluetooth: Reorganize set_connectable HCI command sending
      Bluetooth: Move move logic into set_connectable complete callback
      Bluetooth: Add missing error handling for Set Connectable
      Bluetooth: Move static advertising functions to avoid forward declarations
      Bluetooth: Fix updating advertising data needlessly

 net/bluetooth/hci_event.c |   2 +-
 net/bluetooth/mgmt.c      | 122 ++++++++++++++++++++++++++++------------------
 2 files changed, 75 insertions(+), 49 deletions(-)

[PATCH 1/5] Bluetooth: Reorganize set_connectable HCI command sending

[johan.hedberg]

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

This patch moves all the decisions of which HCI commands to send (or not
to send) to the code between hci_req_init() and hci_req_run() this
allows us to further extend the request with further commands but still
keep the same logic of handling whether to return a direct mgmt response
in the case that no HCI commands were sent.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 020f95b..cfd8d44 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1144,30 +1144,29 @@ static int set_connectable(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto failed;
 	}
 
-	if (!!cp->val == test_bit(HCI_PSCAN, &hdev->flags)) {
-		err = send_settings_rsp(sk, MGMT_OP_SET_CONNECTABLE, hdev);
-		goto failed;
-	}
-
 	cmd = mgmt_pending_add(sk, MGMT_OP_SET_CONNECTABLE, hdev, data, len);
 	if (!cmd) {
 		err = -ENOMEM;
 		goto failed;
 	}
 
-	if (cp->val) {
-		scan = SCAN_PAGE;
-	} else {
-		scan = 0;
+	hci_req_init(&req, hdev);
 
-		if (test_bit(HCI_ISCAN, &hdev->flags) &&
-		    hdev->discov_timeout > 0)
-			cancel_delayed_work(&hdev->discov_off);
-	}
+	if (test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags) &&
+	    cp->val != test_bit(HCI_PSCAN, &hdev->flags)) {
 
-	hci_req_init(&req, hdev);
+		if (cp->val) {
+			scan = SCAN_PAGE;
+		} else {
+			scan = 0;
 
-	hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
+			if (test_bit(HCI_ISCAN, &hdev->flags) &&
+					hdev->discov_timeout > 0)
+				cancel_delayed_work(&hdev->discov_off);
+		}
+
+		hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
+	}
 
 	/* If we're going from non-connectable to connectable or
 	 * vice-versa when fast connectable is enabled ensure that fast
@@ -1179,8 +1178,13 @@ static int set_connectable(struct sock *sk, struct hci_dev *hdev, void *data,
 		write_fast_connectable(&req, false);
 
 	err = hci_req_run(&req, set_connectable_complete);
-	if (err < 0)
+	if (err < 0) {
 		mgmt_pending_remove(cmd);
+		if (err == -ENODATA)
+			err = send_settings_rsp(sk, MGMT_OP_SET_CONNECTABLE,
+						hdev);
+		goto failed;
+	}
 
 failed:
 	hci_dev_unlock(hdev);
-- 
1.8.3.1

[PATCH 2/5] Bluetooth: Move move logic into set_connectable complete callback

[johan.hedberg]

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

This patch moves the responsibility of setting/clearing the
HCI_CONNECTABLE flag to the request completion callback of the Set
Connectable command. This will allow us to cleanly add support for LE
Advertising hooks in later patches.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index cfd8d44..8dcea77 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1075,6 +1075,8 @@ static void write_fast_connectable(struct hci_request *req, bool enable)
 static void set_connectable_complete(struct hci_dev *hdev, u8 status)
 {
 	struct pending_cmd *cmd;
+	struct mgmt_mode *cp;
+	bool changed;
 
 	BT_DBG("status 0x%02x", status);
 
@@ -1084,8 +1086,17 @@ static void set_connectable_complete(struct hci_dev *hdev, u8 status)
 	if (!cmd)
 		goto unlock;
 
+	cp = cmd->param;
+	if (cp->val)
+		changed = !test_and_set_bit(HCI_CONNECTABLE, &hdev->dev_flags);
+	else
+		changed = test_and_clear_bit(HCI_CONNECTABLE, &hdev->dev_flags);
+
 	send_settings_rsp(cmd->sk, MGMT_OP_SET_CONNECTABLE, hdev);
 
+	if (changed)
+		new_settings(hdev, cmd->sk);
+
 	mgmt_pending_remove(cmd);
 
 unlock:
@@ -4053,10 +4064,16 @@ int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable)
 
 int mgmt_connectable(struct hci_dev *hdev, u8 connectable)
 {
-	struct pending_cmd *cmd;
 	bool changed = false;
 	int err = 0;
 
+	/* Nothing needed here if there's a pending command since that
+	 * commands request completion callback takes care of everything
+	 * necessary.
+	 */
+	if (mgmt_pending_find(MGMT_OP_SET_CONNECTABLE, hdev))
+		return 0;
+
 	if (connectable) {
 		if (!test_and_set_bit(HCI_CONNECTABLE, &hdev->dev_flags))
 			changed = true;
@@ -4065,10 +4082,8 @@ int mgmt_connectable(struct hci_dev *hdev, u8 connectable)
 			changed = true;
 	}
 
-	cmd = mgmt_pending_find(MGMT_OP_SET_CONNECTABLE, hdev);
-
 	if (changed)
-		err = new_settings(hdev, cmd ? cmd->sk : NULL);
+		err = new_settings(hdev, NULL);
 
 	return err;
 }
-- 
1.8.3.1

[PATCH 3/5] Bluetooth: Add missing error handling for Set Connectable

[johan.hedberg]

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

If the HCI commands related to the Set Connectable command fail we will
get a non-zero status in the request completion callback. In such a case
we must respond with the appropriate command status message to user space.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 8dcea77..a5c015c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1086,6 +1086,12 @@ static void set_connectable_complete(struct hci_dev *hdev, u8 status)
 	if (!cmd)
 		goto unlock;
 
+	if (status) {
+		u8 mgmt_err = mgmt_status(status);
+		cmd_status(cmd->sk, cmd->index, cmd->opcode, mgmt_err);
+		goto remove_cmd;
+	}
+
 	cp = cmd->param;
 	if (cp->val)
 		changed = !test_and_set_bit(HCI_CONNECTABLE, &hdev->dev_flags);
@@ -1097,6 +1103,7 @@ static void set_connectable_complete(struct hci_dev *hdev, u8 status)
 	if (changed)
 		new_settings(hdev, cmd->sk);
 
+remove_cmd:
 	mgmt_pending_remove(cmd);
 
 unlock:
-- 
1.8.3.1

[PATCH 4/5] Bluetooth: Move static advertising functions to avoid forward declarations

[johan.hedberg]

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

These functions will soon be used by set_connectable() so move them to a
location in mgmt.c that doesn't require forward declarations.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a5c015c..caa552c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1072,6 +1072,34 @@ static void write_fast_connectable(struct hci_request *req, bool enable)
 		hci_req_add(req, HCI_OP_WRITE_PAGE_SCAN_TYPE, 1, &type);
 }
 
+static void enable_advertising(struct hci_request *req)
+{
+	struct hci_dev *hdev = req->hdev;
+	struct hci_cp_le_set_adv_param cp;
+	u8 enable = 0x01;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.min_interval = __constant_cpu_to_le16(0x0800);
+	cp.max_interval = __constant_cpu_to_le16(0x0800);
+	cp.type = LE_ADV_IND;
+	if (bacmp(&hdev->bdaddr, BDADDR_ANY))
+		cp.own_address_type = ADDR_LE_DEV_PUBLIC;
+	else
+		cp.own_address_type = ADDR_LE_DEV_RANDOM;
+	cp.channel_map = 0x07;
+
+	hci_req_add(req, HCI_OP_LE_SET_ADV_PARAM, sizeof(cp), &cp);
+
+	hci_req_add(req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(enable), &enable);
+}
+
+static void disable_advertising(struct hci_request *req)
+{
+	u8 enable = 0x00;
+
+	hci_req_add(req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(enable), &enable);
+}
+
 static void set_connectable_complete(struct hci_dev *hdev, u8 status)
 {
 	struct pending_cmd *cmd;
@@ -1440,34 +1468,6 @@ unlock:
 	return err;
 }
 
-static void enable_advertising(struct hci_request *req)
-{
-	struct hci_dev *hdev = req->hdev;
-	struct hci_cp_le_set_adv_param cp;
-	u8 enable = 0x01;
-
-	memset(&cp, 0, sizeof(cp));
-	cp.min_interval = __constant_cpu_to_le16(0x0800);
-	cp.max_interval = __constant_cpu_to_le16(0x0800);
-	cp.type = LE_ADV_IND;
-	if (bacmp(&hdev->bdaddr, BDADDR_ANY))
-		cp.own_address_type = ADDR_LE_DEV_PUBLIC;
-	else
-		cp.own_address_type = ADDR_LE_DEV_RANDOM;
-	cp.channel_map = 0x07;
-
-	hci_req_add(req, HCI_OP_LE_SET_ADV_PARAM, sizeof(cp), &cp);
-
-	hci_req_add(req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(enable), &enable);
-}
-
-static void disable_advertising(struct hci_request *req)
-{
-	u8 enable = 0x00;
-
-	hci_req_add(req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(enable), &enable);
-}
-
 static void le_enable_complete(struct hci_dev *hdev, u8 status)
 {
 	struct cmd_lookup match = { NULL, hdev };
-- 
1.8.3.1

[PATCH 5/5] Bluetooth: Fix updating advertising data needlessly

[johan.hedberg]

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

We need to ensure that the advertising data is up-to-date whenever
advertising is enabled, but when disabling advertising we do not need to
worry about it (since it will eventually get fixed as soon as
advertising is enabled again). This patch fixes this in the command
complete callback for set_adv_enable.

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

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index aa6fed3..da2bc3d 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -924,7 +924,7 @@ static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
 			clear_bit(HCI_ADVERTISING, &hdev->dev_flags);
 	}
 
-	if (!test_bit(HCI_INIT, &hdev->flags)) {
+	if (*sent && !test_bit(HCI_INIT, &hdev->flags)) {
 		struct hci_request req;
 
 		hci_req_init(&req, hdev);
-- 
1.8.3.1

Re: [PATCH 0/5] Bluetooth: Clean up Set Connectable related code

[Marcel Holtmann]

Hi Johan,

> This is a set of cleanup patches (and one fix) split out from my earlier
> set. I'm hoping to get these in first to make it easier to understand
> how the code works and what the possible ways of adding LE Advertising
> hooks are.
> 
> The main thing with these patches is that we make a clear separation of
> what happens if someone uses a raw HCI socket compared to sending a Set
> Connectable command using the management interface. Now both cases have
> their clear functions for handling the HCI_CONNECTABLE flag and the
> necessary mgmt events.
> 
> ----------------------------------------------------------------
> Johan Hedberg (5):
>      Bluetooth: Reorganize set_connectable HCI command sending
>      Bluetooth: Move move logic into set_connectable complete callback
>      Bluetooth: Add missing error handling for Set Connectable
>      Bluetooth: Move static advertising functions to avoid forward declarations
>      Bluetooth: Fix updating advertising data needlessly
> 
> net/bluetooth/hci_event.c |   2 +-
> net/bluetooth/mgmt.c      | 122 ++++++++++++++++++++++++++++------------------
> 2 files changed, 75 insertions(+), 49 deletions(-)

all 5 patches have been applied to bluetooth-next tree.

Regards

Marcel