[PATCH v3 00/16] Bluetooth: Add HCI transaction framework

[PATCH v3 00/16] Bluetooth: Add HCI transaction framework

[Johan Hedberg]

Hi,

Here's a revised set with the left-over init_last_cmd patch inserted
where it belongs in the set.

The other changes are based on an (off-list) review from Gustavo
Padovan, resulting in removing unnecessary _irqsave versions of spinlock
functions (for the skb queue), adding a hci_ prefix to some static
hci_core.c functions as well as really removing the HCI_PENDING_CLASS
flag (and not just its users).

Johan

----------------------------------------------------------------
Johan Hedberg (16):
      Bluetooth: Fix __hci_request() handling of empty requests
      Bluetooth: Split HCI init sequence into three stages
      Bluetooth: Add initial skeleton for HCI transaction framework
      Bluetooth: Refactor HCI command skb creation
      Bluetooth: Introduce new hci_transaction_cmd function
      Bluetooth: Introduce a hci_transaction_from_skb function
      Bluetooth: Add transaction cmd_complete and cmd_status functions
      Bluetooth: Convert hci_request to use HCI transaction framework
      Bluetooth: Remove unused hdev->init_last_cmd
      Bluetooth: Move power on HCI command updates to their own function
      Bluetooth: Update mgmt powered HCI commands to use transactions
      Bluetooth: Wait for HCI command completion with mgmt_set_powered
      Bluetooth: Fix busy condition testing for EIR and class updates
      Bluetooth: Fix UUID/class mgmt command response synchronization
      Bluetooth: Remove useless HCI_PENDING_CLASS flag
      Bluetooth: Remove empty HCI event handlers

 include/net/bluetooth/bluetooth.h |   11 +
 include/net/bluetooth/hci.h       |    1 -
 include/net/bluetooth/hci_core.h  |   20 +-
 net/bluetooth/hci_core.c          |  649 ++++++++++++++++++++++++++++++++-----
 net/bluetooth/hci_event.c         |  507 +----------------------------
 net/bluetooth/hci_sock.c          |    3 +-
 net/bluetooth/mgmt.c              |  326 +++++++++++++------
 7 files changed, 826 insertions(+), 691 deletions(-)

[PATCH v3 01/16] Bluetooth: Fix __hci_request() handling of empty requests

[Johan Hedberg]

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

If a request callback doesn't send any commands __hci_request() should
fail imediately instead of waiting for the inevitable timeout to occur.
This is particularly important once we start creating requests with
conditional command sending which can potentially result in no commands
being sent at all.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index aeb2b26..fd6921f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -122,6 +122,14 @@ static int __hci_request(struct hci_dev *hdev,
 	set_current_state(TASK_INTERRUPTIBLE);
 
 	req(hdev, opt);
+
+	/* If the request didn't send any commands return immediately */
+	if (skb_queue_empty(&hdev->cmd_q) && atomic_read(&hdev->cmd_cnt)) {
+		hdev->req_status = 0;
+		remove_wait_queue(&hdev->req_wait_q, &wait);
+		return err;
+	}
+
 	schedule_timeout(timeout);
 
 	remove_wait_queue(&hdev->req_wait_q, &wait);
-- 
1.7.10.4

[PATCH v3 02/16] Bluetooth: Split HCI init sequence into three stages

[Johan Hedberg]

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

Having contitional command sending during a request has always been
problematic and caused hacks like the hdev->init_last_cmd variable. This
patch removes these conditionals and instead splits the init sequence
into three stages, each with its own __hci_request() call.

This also paves the way to the upcoming transaction framework which will
also benefit by having a simpler implementation if it doesn't need to
cater for transactions that change on the fly.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fd6921f..f71ad76 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -193,6 +193,9 @@ static void bredr_init(struct hci_dev *hdev)
 
 	/* Read Local Version */
 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
+
+	/* Read BD Address */
+	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
 }
 
 static void amp_init(struct hci_dev *hdev)
@@ -209,7 +212,7 @@ static void amp_init(struct hci_dev *hdev)
 	hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
 }
 
-static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
+static void hci_init1_req(struct hci_dev *hdev, unsigned long opt)
 {
 	struct sk_buff *skb;
 
@@ -246,6 +249,270 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
 	}
 }
 
+static void bredr_setup(struct hci_dev *hdev)
+{
+	struct hci_cp_delete_stored_link_key cp;
+	__le16 param;
+	__u8 flt_type;
+
+	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
+	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
+
+	/* Read Class of Device */
+	hci_send_cmd(hdev, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);
+
+	/* Read Local Name */
+	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL);
+
+	/* Read Voice Setting */
+	hci_send_cmd(hdev, HCI_OP_READ_VOICE_SETTING, 0, NULL);
+
+	/* Clear Event Filters */
+	flt_type = HCI_FLT_CLEAR_ALL;
+	hci_send_cmd(hdev, HCI_OP_SET_EVENT_FLT, 1, &flt_type);
+
+	/* Connection accept timeout ~20 secs */
+	param = __constant_cpu_to_le16(0x7d00);
+	hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
+
+	bacpy(&cp.bdaddr, BDADDR_ANY);
+	cp.delete_all = 1;
+	hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
+}
+
+static void le_setup(struct hci_dev *hdev)
+{
+	/* Read LE Buffer Size */
+	hci_send_cmd(hdev, HCI_OP_LE_READ_BUFFER_SIZE, 0, NULL);
+
+	/* Read LE Local Supported Features */
+	hci_send_cmd(hdev, HCI_OP_LE_READ_LOCAL_FEATURES, 0, NULL);
+
+	/* Read LE Advertising Channel TX Power */
+	hci_send_cmd(hdev, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL);
+
+	/* Read LE White List Size */
+	hci_send_cmd(hdev, HCI_OP_LE_READ_WHITE_LIST_SIZE, 0, NULL);
+
+	/* Read LE Supported States */
+	hci_send_cmd(hdev, HCI_OP_LE_READ_SUPPORTED_STATES, 0, NULL);
+}
+
+static u8 hci_get_inquiry_mode(struct hci_dev *hdev)
+{
+	if (lmp_ext_inq_capable(hdev))
+		return 2;
+
+	if (lmp_inq_rssi_capable(hdev))
+		return 1;
+
+	if (hdev->manufacturer == 11 && hdev->hci_rev == 0x00 &&
+	    hdev->lmp_subver == 0x0757)
+		return 1;
+
+	if (hdev->manufacturer == 15) {
+		if (hdev->hci_rev == 0x03 && hdev->lmp_subver == 0x6963)
+			return 1;
+		if (hdev->hci_rev == 0x09 && hdev->lmp_subver == 0x6963)
+			return 1;
+		if (hdev->hci_rev == 0x00 && hdev->lmp_subver == 0x6965)
+			return 1;
+	}
+
+	if (hdev->manufacturer == 31 && hdev->hci_rev == 0x2005 &&
+	    hdev->lmp_subver == 0x1805)
+		return 1;
+
+	return 0;
+}
+
+static void hci_setup_inquiry_mode(struct hci_dev *hdev)
+{
+	u8 mode;
+
+	mode = hci_get_inquiry_mode(hdev);
+
+	hci_send_cmd(hdev, HCI_OP_WRITE_INQUIRY_MODE, 1, &mode);
+}
+
+static void hci_setup_event_mask(struct hci_dev *hdev)
+{
+	/* The second byte is 0xff instead of 0x9f (two reserved bits
+	 * disabled) since a Broadcom 1.2 dongle doesn't respond to the
+	 * command otherwise.
+	 */
+	u8 events[8] = { 0xff, 0xff, 0xfb, 0xff, 0x00, 0x00, 0x00, 0x00 };
+
+	/* CSR 1.1 dongles does not accept any bitfield so don't try to set
+	 * any event mask for pre 1.2 devices.
+	 */
+	if (hdev->hci_ver < BLUETOOTH_VER_1_2)
+		return;
+
+	if (lmp_bredr_capable(hdev)) {
+		events[4] |= 0x01; /* Flow Specification Complete */
+		events[4] |= 0x02; /* Inquiry Result with RSSI */
+		events[4] |= 0x04; /* Read Remote Extended Features Complete */
+		events[5] |= 0x08; /* Synchronous Connection Complete */
+		events[5] |= 0x10; /* Synchronous Connection Changed */
+	}
+
+	if (lmp_inq_rssi_capable(hdev))
+		events[4] |= 0x02; /* Inquiry Result with RSSI */
+
+	if (lmp_sniffsubr_capable(hdev))
+		events[5] |= 0x20; /* Sniff Subrating */
+
+	if (lmp_pause_enc_capable(hdev))
+		events[5] |= 0x80; /* Encryption Key Refresh Complete */
+
+	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 (lmp_lsto_capable(hdev))
+		events[6] |= 0x80; /* Link Supervision Timeout Changed */
+
+	if (lmp_ssp_capable(hdev)) {
+		events[6] |= 0x01;	/* IO Capability Request */
+		events[6] |= 0x02;	/* IO Capability Response */
+		events[6] |= 0x04;	/* User Confirmation Request */
+		events[6] |= 0x08;	/* User Passkey Request */
+		events[6] |= 0x10;	/* Remote OOB Data Request */
+		events[6] |= 0x20;	/* Simple Pairing Complete */
+		events[7] |= 0x04;	/* User Passkey Notification */
+		events[7] |= 0x08;	/* Keypress Notification */
+		events[7] |= 0x10;	/* Remote Host Supported
+					 * Features Notification
+					 */
+	}
+
+	if (lmp_le_capable(hdev))
+		events[7] |= 0x20;	/* LE Meta-Event */
+
+	hci_send_cmd(hdev, HCI_OP_SET_EVENT_MASK, sizeof(events), events);
+
+	if (lmp_le_capable(hdev)) {
+		memset(events, 0, sizeof(events));
+		events[0] = 0x1f;
+		hci_send_cmd(hdev, HCI_OP_LE_SET_EVENT_MASK,
+			     sizeof(events), events);
+	}
+}
+
+static void hci_init2_req(struct hci_dev *hdev, unsigned long opt)
+{
+	if (lmp_bredr_capable(hdev))
+		bredr_setup(hdev);
+
+	if (lmp_le_capable(hdev))
+		le_setup(hdev);
+
+	hci_setup_event_mask(hdev);
+
+	if (hdev->hci_ver > BLUETOOTH_VER_1_1)
+		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_COMMANDS, 0, NULL);
+
+	if (lmp_ssp_capable(hdev)) {
+		if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags)) {
+			u8 mode = 0x01;
+			hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE,
+				     sizeof(mode), &mode);
+		} else {
+			struct hci_cp_write_eir cp;
+
+			memset(hdev->eir, 0, sizeof(hdev->eir));
+			memset(&cp, 0, sizeof(cp));
+
+			hci_send_cmd(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp);
+		}
+	}
+
+	if (lmp_inq_rssi_capable(hdev))
+		hci_setup_inquiry_mode(hdev);
+
+	if (lmp_inq_tx_pwr_capable(hdev))
+		hci_send_cmd(hdev, HCI_OP_READ_INQ_RSP_TX_POWER, 0, NULL);
+
+	if (lmp_ext_feat_capable(hdev)) {
+		struct hci_cp_read_local_ext_features cp;
+
+		cp.page = 0x01;
+		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, sizeof(cp),
+			     &cp);
+	}
+
+	if (test_bit(HCI_LINK_SECURITY, &hdev->dev_flags)) {
+		u8 enable = 1;
+		hci_send_cmd(hdev, HCI_OP_WRITE_AUTH_ENABLE, sizeof(enable),
+			     &enable);
+	}
+}
+
+static void hci_setup_link_policy(struct hci_dev *hdev)
+{
+	struct hci_cp_write_def_link_policy cp;
+	u16 link_policy = 0;
+
+	if (lmp_rswitch_capable(hdev))
+		link_policy |= HCI_LP_RSWITCH;
+	if (lmp_hold_capable(hdev))
+		link_policy |= HCI_LP_HOLD;
+	if (lmp_sniff_capable(hdev))
+		link_policy |= HCI_LP_SNIFF;
+	if (lmp_park_capable(hdev))
+		link_policy |= HCI_LP_PARK;
+
+	cp.policy = cpu_to_le16(link_policy);
+	hci_send_cmd(hdev, HCI_OP_WRITE_DEF_LINK_POLICY, sizeof(cp), &cp);
+}
+
+static void hci_set_le_support(struct hci_dev *hdev)
+{
+	struct hci_cp_write_le_host_supported cp;
+
+	memset(&cp, 0, sizeof(cp));
+
+	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
+		cp.le = 1;
+		cp.simul = lmp_le_br_capable(hdev);
+	}
+
+	if (cp.le != lmp_host_le_capable(hdev))
+		hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
+			     &cp);
+}
+
+static void hci_init3_req(struct hci_dev *hdev, unsigned long opt)
+{
+	if (hdev->commands[5] & 0x10)
+		hci_setup_link_policy(hdev);
+
+	if (lmp_le_capable(hdev))
+		hci_set_le_support(hdev);
+}
+
+static int __hci_init(struct hci_dev *hdev)
+{
+	int err;
+
+	err = __hci_request(hdev, hci_init1_req, 0, HCI_INIT_TIMEOUT);
+	if (err < 0)
+		return err;
+
+	/* AMP controllers do not need stage 2/3 init */
+	if (hdev->dev_type != HCI_BREDR)
+		return 0;
+
+	err = __hci_request(hdev, hci_init2_req, 0, HCI_INIT_TIMEOUT);
+	if (err < 0)
+		return err;
+
+	return __hci_request(hdev, hci_init3_req, 0, HCI_INIT_TIMEOUT);
+}
+
 static void hci_scan_req(struct hci_dev *hdev, unsigned long opt)
 {
 	__u8 scan = opt;
@@ -745,7 +1012,7 @@ int hci_dev_open(__u16 dev)
 		set_bit(HCI_INIT, &hdev->flags);
 		hdev->init_last_cmd = 0;
 
-		ret = __hci_request(hdev, hci_init_req, 0, HCI_INIT_TIMEOUT);
+		ret = __hci_init(hdev);
 
 		clear_bit(HCI_INIT, &hdev->flags);
 	}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 5892e54..14e872a 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -472,211 +472,6 @@ 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 (lmp_ext_inq_capable(hdev))
-		return 2;
-
-	if (lmp_inq_rssi_capable(hdev))
-		return 1;
-
-	if (hdev->manufacturer == 11 && hdev->hci_rev == 0x00 &&
-	    hdev->lmp_subver == 0x0757)
-		return 1;
-
-	if (hdev->manufacturer == 15) {
-		if (hdev->hci_rev == 0x03 && hdev->lmp_subver == 0x6963)
-			return 1;
-		if (hdev->hci_rev == 0x09 && hdev->lmp_subver == 0x6963)
-			return 1;
-		if (hdev->hci_rev == 0x00 && hdev->lmp_subver == 0x6965)
-			return 1;
-	}
-
-	if (hdev->manufacturer == 31 && hdev->hci_rev == 0x2005 &&
-	    hdev->lmp_subver == 0x1805)
-		return 1;
-
-	return 0;
-}
-
-static void hci_setup_inquiry_mode(struct hci_dev *hdev)
-{
-	u8 mode;
-
-	mode = hci_get_inquiry_mode(hdev);
-
-	hci_send_cmd(hdev, HCI_OP_WRITE_INQUIRY_MODE, 1, &mode);
-}
-
-static void hci_setup_event_mask(struct hci_dev *hdev)
-{
-	/* The second byte is 0xff instead of 0x9f (two reserved bits
-	 * disabled) since a Broadcom 1.2 dongle doesn't respond to the
-	 * command otherwise */
-	u8 events[8] = { 0xff, 0xff, 0xfb, 0xff, 0x00, 0x00, 0x00, 0x00 };
-
-	/* CSR 1.1 dongles does not accept any bitfield so don't try to set
-	 * any event mask for pre 1.2 devices */
-	if (hdev->hci_ver < BLUETOOTH_VER_1_2)
-		return;
-
-	if (lmp_bredr_capable(hdev)) {
-		events[4] |= 0x01; /* Flow Specification Complete */
-		events[4] |= 0x02; /* Inquiry Result with RSSI */
-		events[4] |= 0x04; /* Read Remote Extended Features Complete */
-		events[5] |= 0x08; /* Synchronous Connection Complete */
-		events[5] |= 0x10; /* Synchronous Connection Changed */
-	}
-
-	if (lmp_inq_rssi_capable(hdev))
-		events[4] |= 0x02; /* Inquiry Result with RSSI */
-
-	if (lmp_sniffsubr_capable(hdev))
-		events[5] |= 0x20; /* Sniff Subrating */
-
-	if (lmp_pause_enc_capable(hdev))
-		events[5] |= 0x80; /* Encryption Key Refresh Complete */
-
-	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 (lmp_lsto_capable(hdev))
-		events[6] |= 0x80; /* Link Supervision Timeout Changed */
-
-	if (lmp_ssp_capable(hdev)) {
-		events[6] |= 0x01;	/* IO Capability Request */
-		events[6] |= 0x02;	/* IO Capability Response */
-		events[6] |= 0x04;	/* User Confirmation Request */
-		events[6] |= 0x08;	/* User Passkey Request */
-		events[6] |= 0x10;	/* Remote OOB Data Request */
-		events[6] |= 0x20;	/* Simple Pairing Complete */
-		events[7] |= 0x04;	/* User Passkey Notification */
-		events[7] |= 0x08;	/* Keypress Notification */
-		events[7] |= 0x10;	/* Remote Host Supported
-					 * Features Notification */
-	}
-
-	if (lmp_le_capable(hdev))
-		events[7] |= 0x20;	/* LE Meta-Event */
-
-	hci_send_cmd(hdev, HCI_OP_SET_EVENT_MASK, sizeof(events), events);
-
-	if (lmp_le_capable(hdev)) {
-		memset(events, 0, sizeof(events));
-		events[0] = 0x1f;
-		hci_send_cmd(hdev, HCI_OP_LE_SET_EVENT_MASK,
-			     sizeof(events), events);
-	}
-}
-
-static void bredr_setup(struct hci_dev *hdev)
-{
-	struct hci_cp_delete_stored_link_key cp;
-	__le16 param;
-	__u8 flt_type;
-
-	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
-	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
-
-	/* Read Class of Device */
-	hci_send_cmd(hdev, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);
-
-	/* Read Local Name */
-	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL);
-
-	/* Read Voice Setting */
-	hci_send_cmd(hdev, HCI_OP_READ_VOICE_SETTING, 0, NULL);
-
-	/* Clear Event Filters */
-	flt_type = HCI_FLT_CLEAR_ALL;
-	hci_send_cmd(hdev, HCI_OP_SET_EVENT_FLT, 1, &flt_type);
-
-	/* Connection accept timeout ~20 secs */
-	param = __constant_cpu_to_le16(0x7d00);
-	hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
-
-	bacpy(&cp.bdaddr, BDADDR_ANY);
-	cp.delete_all = 1;
-	hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
-}
-
-static void le_setup(struct hci_dev *hdev)
-{
-	/* Read LE Buffer Size */
-	hci_send_cmd(hdev, HCI_OP_LE_READ_BUFFER_SIZE, 0, NULL);
-
-	/* Read LE Local Supported Features */
-	hci_send_cmd(hdev, HCI_OP_LE_READ_LOCAL_FEATURES, 0, NULL);
-
-	/* Read LE Advertising Channel TX Power */
-	hci_send_cmd(hdev, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL);
-
-	/* Read LE White List Size */
-	hci_send_cmd(hdev, HCI_OP_LE_READ_WHITE_LIST_SIZE, 0, NULL);
-
-	/* Read LE Supported States */
-	hci_send_cmd(hdev, HCI_OP_LE_READ_SUPPORTED_STATES, 0, NULL);
-}
-
-static void hci_setup(struct hci_dev *hdev)
-{
-	if (hdev->dev_type != HCI_BREDR)
-		return;
-
-	/* Read BD Address */
-	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
-
-	if (lmp_bredr_capable(hdev))
-		bredr_setup(hdev);
-
-	if (lmp_le_capable(hdev))
-		le_setup(hdev);
-
-	hci_setup_event_mask(hdev);
-
-	if (hdev->hci_ver > BLUETOOTH_VER_1_1)
-		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_COMMANDS, 0, NULL);
-
-	if (lmp_ssp_capable(hdev)) {
-		if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags)) {
-			u8 mode = 0x01;
-			hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE,
-				     sizeof(mode), &mode);
-		} else {
-			struct hci_cp_write_eir cp;
-
-			memset(hdev->eir, 0, sizeof(hdev->eir));
-			memset(&cp, 0, sizeof(cp));
-
-			hci_send_cmd(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp);
-		}
-	}
-
-	if (lmp_inq_rssi_capable(hdev))
-		hci_setup_inquiry_mode(hdev);
-
-	if (lmp_inq_tx_pwr_capable(hdev))
-		hci_send_cmd(hdev, HCI_OP_READ_INQ_RSP_TX_POWER, 0, NULL);
-
-	if (lmp_ext_feat_capable(hdev)) {
-		struct hci_cp_read_local_ext_features cp;
-
-		cp.page = 0x01;
-		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, sizeof(cp),
-			     &cp);
-	}
-
-	if (test_bit(HCI_LINK_SECURITY, &hdev->dev_flags)) {
-		u8 enable = 1;
-		hci_send_cmd(hdev, HCI_OP_WRITE_AUTH_ENABLE, sizeof(enable),
-			     &enable);
-	}
-}
-
 static void hci_cc_read_local_version(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_rp_read_local_version *rp = (void *) skb->data;
@@ -695,31 +490,10 @@ static void hci_cc_read_local_version(struct hci_dev *hdev, struct sk_buff *skb)
 	BT_DBG("%s manufacturer 0x%4.4x hci ver %d:%d", hdev->name,
 	       hdev->manufacturer, hdev->hci_ver, hdev->hci_rev);
 
-	if (test_bit(HCI_INIT, &hdev->flags))
-		hci_setup(hdev);
-
 done:
 	hci_req_complete(hdev, HCI_OP_READ_LOCAL_VERSION, rp->status);
 }
 
-static void hci_setup_link_policy(struct hci_dev *hdev)
-{
-	struct hci_cp_write_def_link_policy cp;
-	u16 link_policy = 0;
-
-	if (lmp_rswitch_capable(hdev))
-		link_policy |= HCI_LP_RSWITCH;
-	if (lmp_hold_capable(hdev))
-		link_policy |= HCI_LP_HOLD;
-	if (lmp_sniff_capable(hdev))
-		link_policy |= HCI_LP_SNIFF;
-	if (lmp_park_capable(hdev))
-		link_policy |= HCI_LP_PARK;
-
-	cp.policy = cpu_to_le16(link_policy);
-	hci_send_cmd(hdev, HCI_OP_WRITE_DEF_LINK_POLICY, sizeof(cp), &cp);
-}
-
 static void hci_cc_read_local_commands(struct hci_dev *hdev,
 				       struct sk_buff *skb)
 {
@@ -727,15 +501,9 @@ static void hci_cc_read_local_commands(struct hci_dev *hdev,
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
 
-	if (rp->status)
-		goto done;
-
-	memcpy(hdev->commands, rp->commands, sizeof(hdev->commands));
-
-	if (test_bit(HCI_INIT, &hdev->flags) && (hdev->commands[5] & 0x10))
-		hci_setup_link_policy(hdev);
+	if (!rp->status)
+		memcpy(hdev->commands, rp->commands, sizeof(hdev->commands));
 
-done:
 	hci_req_complete(hdev, HCI_OP_READ_LOCAL_COMMANDS, rp->status);
 }
 
@@ -795,22 +563,6 @@ static void hci_cc_read_local_features(struct hci_dev *hdev,
 	       hdev->features[6], hdev->features[7]);
 }
 
-static void hci_set_le_support(struct hci_dev *hdev)
-{
-	struct hci_cp_write_le_host_supported cp;
-
-	memset(&cp, 0, sizeof(cp));
-
-	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
-		cp.le = 1;
-		cp.simul = lmp_le_br_capable(hdev);
-	}
-
-	if (cp.le != lmp_host_le_capable(hdev))
-		hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
-			     &cp);
-}
-
 static void hci_cc_read_local_ext_features(struct hci_dev *hdev,
 					   struct sk_buff *skb)
 {
@@ -830,9 +582,6 @@ static void hci_cc_read_local_ext_features(struct hci_dev *hdev,
 		break;
 	}
 
-	if (test_bit(HCI_INIT, &hdev->flags) && lmp_le_capable(hdev))
-		hci_set_le_support(hdev);
-
 done:
 	hci_req_complete(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, rp->status);
 }
-- 
1.7.10.4

Re: [PATCH v3 02/16] Bluetooth: Split HCI init sequence into three stages

[Marcel Holtmann]

Hi Johan,


> Having contitional command sending during a request has always been
> problematic and caused hacks like the hdev->init_last_cmd variable. This
> patch removes these conditionals and instead splits the init sequence
> into three stages, each with its own __hci_request() call.
> 
> This also paves the way to the upcoming transaction framework which will
> also benefit by having a simpler implementation if it doesn't need to
> cater for transactions that change on the fly.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_core.c  |  271 ++++++++++++++++++++++++++++++++++++++++++++-
> net/bluetooth/hci_event.c |  255 +-----------------------------------------
> 2 files changed, 271 insertions(+), 255 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index fd6921f..f71ad76 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -193,6 +193,9 @@ static void bredr_init(struct hci_dev *hdev)
> 
> 	/* Read Local Version */
> 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> +
> +	/* Read BD Address */
> +	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
> }
> 
> static void amp_init(struct hci_dev *hdev)
> @@ -209,7 +212,7 @@ static void amp_init(struct hci_dev *hdev)
> 	hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
> }
> 
> -static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
> +static void hci_init1_req(struct hci_dev *hdev, unsigned long opt)
> {
> 	struct sk_buff *skb;
> 
> @@ -246,6 +249,270 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
> 	}
> }
> 
> +static void bredr_setup(struct hci_dev *hdev)
> +{
> +	struct hci_cp_delete_stored_link_key cp;
> +	__le16 param;
> +	__u8 flt_type;
> +
> +	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
> +	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> +
> +	/* Read Class of Device */
> +	hci_send_cmd(hdev, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);
> +
> +	/* Read Local Name */
> +	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL);
> +
> +	/* Read Voice Setting */
> +	hci_send_cmd(hdev, HCI_OP_READ_VOICE_SETTING, 0, NULL);
> +
> +	/* Clear Event Filters */
> +	flt_type = HCI_FLT_CLEAR_ALL;
> +	hci_send_cmd(hdev, HCI_OP_SET_EVENT_FLT, 1, &flt_type);
> +
> +	/* Connection accept timeout ~20 secs */
> +	param = __constant_cpu_to_le16(0x7d00);
> +	hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
> +
> +	bacpy(&cp.bdaddr, BDADDR_ANY);
> +	cp.delete_all = 1;
> +	hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
> +}
> +
> +static void le_setup(struct hci_dev *hdev)
> +{
> +	/* Read LE Buffer Size */
> +	hci_send_cmd(hdev, HCI_OP_LE_READ_BUFFER_SIZE, 0, NULL);
> +
> +	/* Read LE Local Supported Features */
> +	hci_send_cmd(hdev, HCI_OP_LE_READ_LOCAL_FEATURES, 0, NULL);
> +
> +	/* Read LE Advertising Channel TX Power */
> +	hci_send_cmd(hdev, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL);
> +
> +	/* Read LE White List Size */
> +	hci_send_cmd(hdev, HCI_OP_LE_READ_WHITE_LIST_SIZE, 0, NULL);
> +
> +	/* Read LE Supported States */
> +	hci_send_cmd(hdev, HCI_OP_LE_READ_SUPPORTED_STATES, 0, NULL);
> +}
> +
> +static u8 hci_get_inquiry_mode(struct hci_dev *hdev)
> +{
> +	if (lmp_ext_inq_capable(hdev))
> +		return 2;
> +
> +	if (lmp_inq_rssi_capable(hdev))
> +		return 1;
> +
> +	if (hdev->manufacturer == 11 && hdev->hci_rev == 0x00 &&
> +	    hdev->lmp_subver == 0x0757)
> +		return 1;
> +
> +	if (hdev->manufacturer == 15) {
> +		if (hdev->hci_rev == 0x03 && hdev->lmp_subver == 0x6963)
> +			return 1;
> +		if (hdev->hci_rev == 0x09 && hdev->lmp_subver == 0x6963)
> +			return 1;
> +		if (hdev->hci_rev == 0x00 && hdev->lmp_subver == 0x6965)
> +			return 1;
> +	}
> +
> +	if (hdev->manufacturer == 31 && hdev->hci_rev == 0x2005 &&
> +	    hdev->lmp_subver == 0x1805)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static void hci_setup_inquiry_mode(struct hci_dev *hdev)
> +{
> +	u8 mode;
> +
> +	mode = hci_get_inquiry_mode(hdev);
> +
> +	hci_send_cmd(hdev, HCI_OP_WRITE_INQUIRY_MODE, 1, &mode);
> +}
> +
> +static void hci_setup_event_mask(struct hci_dev *hdev)
> +{
> +	/* The second byte is 0xff instead of 0x9f (two reserved bits
> +	 * disabled) since a Broadcom 1.2 dongle doesn't respond to the
> +	 * command otherwise.
> +	 */
> +	u8 events[8] = { 0xff, 0xff, 0xfb, 0xff, 0x00, 0x00, 0x00, 0x00 };
> +
> +	/* CSR 1.1 dongles does not accept any bitfield so don't try to set
> +	 * any event mask for pre 1.2 devices.
> +	 */
> +	if (hdev->hci_ver < BLUETOOTH_VER_1_2)
> +		return;
> +
> +	if (lmp_bredr_capable(hdev)) {
> +		events[4] |= 0x01; /* Flow Specification Complete */
> +		events[4] |= 0x02; /* Inquiry Result with RSSI */
> +		events[4] |= 0x04; /* Read Remote Extended Features Complete */
> +		events[5] |= 0x08; /* Synchronous Connection Complete */
> +		events[5] |= 0x10; /* Synchronous Connection Changed */
> +	}
> +
> +	if (lmp_inq_rssi_capable(hdev))
> +		events[4] |= 0x02; /* Inquiry Result with RSSI */
> +
> +	if (lmp_sniffsubr_capable(hdev))
> +		events[5] |= 0x20; /* Sniff Subrating */
> +
> +	if (lmp_pause_enc_capable(hdev))
> +		events[5] |= 0x80; /* Encryption Key Refresh Complete */
> +
> +	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 (lmp_lsto_capable(hdev))
> +		events[6] |= 0x80; /* Link Supervision Timeout Changed */
> +
> +	if (lmp_ssp_capable(hdev)) {
> +		events[6] |= 0x01;	/* IO Capability Request */
> +		events[6] |= 0x02;	/* IO Capability Response */
> +		events[6] |= 0x04;	/* User Confirmation Request */
> +		events[6] |= 0x08;	/* User Passkey Request */
> +		events[6] |= 0x10;	/* Remote OOB Data Request */
> +		events[6] |= 0x20;	/* Simple Pairing Complete */
> +		events[7] |= 0x04;	/* User Passkey Notification */
> +		events[7] |= 0x08;	/* Keypress Notification */
> +		events[7] |= 0x10;	/* Remote Host Supported
> +					 * Features Notification
> +					 */
> +	}
> +
> +	if (lmp_le_capable(hdev))
> +		events[7] |= 0x20;	/* LE Meta-Event */
> +
> +	hci_send_cmd(hdev, HCI_OP_SET_EVENT_MASK, sizeof(events), events);
> +
> +	if (lmp_le_capable(hdev)) {
> +		memset(events, 0, sizeof(events));
> +		events[0] = 0x1f;
> +		hci_send_cmd(hdev, HCI_OP_LE_SET_EVENT_MASK,
> +			     sizeof(events), events);
> +	}
> +}
> +
> +static void hci_init2_req(struct hci_dev *hdev, unsigned long opt)
> +{
> +	if (lmp_bredr_capable(hdev))
> +		bredr_setup(hdev);
> +
> +	if (lmp_le_capable(hdev))
> +		le_setup(hdev);
> +
> +	hci_setup_event_mask(hdev);
> +
> +	if (hdev->hci_ver > BLUETOOTH_VER_1_1)
> +		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_COMMANDS, 0, NULL);
> +
> +	if (lmp_ssp_capable(hdev)) {
> +		if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags)) {
> +			u8 mode = 0x01;
> +			hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE,
> +				     sizeof(mode), &mode);
> +		} else {
> +			struct hci_cp_write_eir cp;
> +
> +			memset(hdev->eir, 0, sizeof(hdev->eir));
> +			memset(&cp, 0, sizeof(cp));
> +
> +			hci_send_cmd(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp);
> +		}
> +	}
> +
> +	if (lmp_inq_rssi_capable(hdev))
> +		hci_setup_inquiry_mode(hdev);
> +
> +	if (lmp_inq_tx_pwr_capable(hdev))
> +		hci_send_cmd(hdev, HCI_OP_READ_INQ_RSP_TX_POWER, 0, NULL);
> +
> +	if (lmp_ext_feat_capable(hdev)) {
> +		struct hci_cp_read_local_ext_features cp;
> +
> +		cp.page = 0x01;
> +		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, sizeof(cp),
> +			     &cp);
> +	}
> +
> +	if (test_bit(HCI_LINK_SECURITY, &hdev->dev_flags)) {
> +		u8 enable = 1;
> +		hci_send_cmd(hdev, HCI_OP_WRITE_AUTH_ENABLE, sizeof(enable),
> +			     &enable);
> +	}
> +}
> +
> +static void hci_setup_link_policy(struct hci_dev *hdev)
> +{
> +	struct hci_cp_write_def_link_policy cp;
> +	u16 link_policy = 0;
> +
> +	if (lmp_rswitch_capable(hdev))
> +		link_policy |= HCI_LP_RSWITCH;
> +	if (lmp_hold_capable(hdev))
> +		link_policy |= HCI_LP_HOLD;
> +	if (lmp_sniff_capable(hdev))
> +		link_policy |= HCI_LP_SNIFF;
> +	if (lmp_park_capable(hdev))
> +		link_policy |= HCI_LP_PARK;
> +
> +	cp.policy = cpu_to_le16(link_policy);
> +	hci_send_cmd(hdev, HCI_OP_WRITE_DEF_LINK_POLICY, sizeof(cp), &cp);
> +}
> +
> +static void hci_set_le_support(struct hci_dev *hdev)
> +{
> +	struct hci_cp_write_le_host_supported cp;
> +
> +	memset(&cp, 0, sizeof(cp));
> +
> +	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
> +		cp.le = 1;
> +		cp.simul = lmp_le_br_capable(hdev);
> +	}

I fully realise that you just copied this code, but use 0x01 here instead of just 1 for variables inside HCI command structs.

Regards

Marcel

Re: [PATCH v3 02/16] Bluetooth: Split HCI init sequence into three stages

[Johan Hedberg]

Hi Marcel,

On Thu, Feb 28, 2013, Marcel Holtmann wrote:
> > +static void hci_set_le_support(struct hci_dev *hdev)
> > +{
> > +	struct hci_cp_write_le_host_supported cp;
> > +
> > +	memset(&cp, 0, sizeof(cp));
> > +
> > +	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
> > +		cp.le = 1;
> > +		cp.simul = lmp_le_br_capable(hdev);
> > +	}
> 
> I fully realise that you just copied this code, but use 0x01 here
> instead of just 1 for variables inside HCI command structs.

Sure, I'll fix it for the next iteration of this set.

Johan

[PATCH v3 03/16] Bluetooth: Add initial skeleton for HCI transaction framework

[Johan Hedberg]

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

This patch adds the initial definitions and functions for HCI
transactions. HCI transactions are essentially a group of HCI commands
together with an optional completion callback. The transaction is
tracked through the already existing command queue by having the
necessary context information as part of the control buffer of each skb.

The only information needed in the skb control buffer is a flag for
indicating that the skb is the start of a transaction as well as the
optional complete callback that should be used for the transaction (this
will only be set for skbs that also have the start flag set).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/bluetooth.h |   11 +++++++++++
 include/net/bluetooth/hci_core.h  |   13 +++++++++++++
 net/bluetooth/hci_core.c          |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 5f51bef..bfcdb56 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -260,12 +260,23 @@ struct l2cap_ctrl {
 	__u8		retries;
 };
 
+struct hci_dev;
+
+typedef void (*transaction_complete_t)(struct hci_dev *hdev, u16 last_cmd,
+				       int status);
+
+struct transaction_ctrl {
+	__u8			start;
+	transaction_complete_t	complete;
+};
+
 struct bt_skb_cb {
 	__u8 pkt_type;
 	__u8 incoming;
 	__u16 expect;
 	__u8 force_active;
 	struct l2cap_ctrl control;
+	struct transaction_ctrl transaction;
 };
 #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
 
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 787d3b9..e97d8e5 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -248,6 +248,8 @@ struct hci_dev {
 	__u32			req_status;
 	__u32			req_result;
 
+	transaction_complete_t	transaction_complete;
+
 	__u16			init_last_cmd;
 
 	struct list_head	mgmt_pending;
@@ -1041,6 +1043,17 @@ static inline u16 eir_append_data(u8 *eir, u16 eir_len, u8 type, u8 *data,
 int hci_register_cb(struct hci_cb *hcb);
 int hci_unregister_cb(struct hci_cb *hcb);
 
+struct hci_transaction {
+	struct hci_dev		*hdev;
+	struct sk_buff_head	cmd_q;
+	transaction_complete_t	complete;
+};
+
+void hci_transaction_init(struct hci_transaction *transaction,
+			  struct hci_dev *hdev,
+			  transaction_complete_t complete);
+int hci_transaction_run(struct hci_transaction *transaction);
+
 int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param);
 void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
 void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f71ad76..ef051b7 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2435,6 +2435,35 @@ static int hci_send_frame(struct sk_buff *skb)
 	return hdev->send(skb);
 }
 
+void hci_transaction_init(struct hci_transaction *transaction,
+			  struct hci_dev *hdev,
+			  transaction_complete_t complete)
+{
+	memset(transaction, 0, sizeof(*transaction));
+	skb_queue_head_init(&transaction->cmd_q);
+	transaction->hdev = hdev;
+	transaction->complete = complete;
+}
+
+int hci_transaction_run(struct hci_transaction *transaction)
+{
+	struct hci_dev *hdev = transaction->hdev;
+
+	BT_DBG("length %u", skb_queue_len(&transaction->cmd_q));
+
+	/* Do not allow empty transactions */
+	if (skb_queue_empty(&transaction->cmd_q))
+		return -EINVAL;
+
+	spin_lock(&hdev->cmd_q.lock);
+	skb_queue_splice_tail(&transaction->cmd_q, &hdev->cmd_q);
+	spin_unlock(&hdev->cmd_q.lock);
+
+	queue_work(hdev->workqueue, &hdev->cmd_work);
+
+	return 0;
+}
+
 /* Send HCI command */
 int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
 {
@@ -3209,6 +3238,11 @@ static void hci_cmd_work(struct work_struct *work)
 		hdev->sent_cmd = skb_clone(skb, GFP_ATOMIC);
 		if (hdev->sent_cmd) {
 			atomic_dec(&hdev->cmd_cnt);
+
+			if (bt_cb(skb)->transaction.start)
+				hdev->transaction_complete =
+					bt_cb(skb)->transaction.complete;
+
 			hci_send_frame(skb);
 			if (test_bit(HCI_RESET, &hdev->flags))
 				del_timer(&hdev->cmd_timer);
-- 
1.7.10.4

Re: [PATCH v3 03/16] Bluetooth: Add initial skeleton for HCI transaction framework

[Marcel Holtmann]

Hi Johan,


> This patch adds the initial definitions and functions for HCI
> transactions. HCI transactions are essentially a group of HCI commands
> together with an optional completion callback. The transaction is
> tracked through the already existing command queue by having the
> necessary context information as part of the control buffer of each skb.
> 
> The only information needed in the skb control buffer is a flag for
> indicating that the skb is the start of a transaction as well as the
> optional complete callback that should be used for the transaction (this
> will only be set for skbs that also have the start flag set).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/bluetooth.h |   11 +++++++++++
> include/net/bluetooth/hci_core.h  |   13 +++++++++++++
> net/bluetooth/hci_core.c          |   34 ++++++++++++++++++++++++++++++++++
> 3 files changed, 58 insertions(+)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 5f51bef..bfcdb56 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -260,12 +260,23 @@ struct l2cap_ctrl {
> 	__u8		retries;
> };
> 
> +struct hci_dev;
> +
> +typedef void (*transaction_complete_t)(struct hci_dev *hdev, u16 last_cmd,
> +				       int status);

why is status an int and not __u8?

We might want to prefix this with hci_ here. Just in case someone includes this header.

> +
> +struct transaction_ctrl {

Same here. hci_ prefix seems like a good idea.

> +	__u8			start;

Why is not a bool and we are using true/false. Magic numbers like 1 in the code seem like a bad idea.

> +	transaction_complete_t	complete;
> +};
> +
> struct bt_skb_cb {
> 	__u8 pkt_type;
> 	__u8 incoming;
> 	__u16 expect;
> 	__u8 force_active;
> 	struct l2cap_ctrl control;
> +	struct transaction_ctrl transaction;
> };
> #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 787d3b9..e97d8e5 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -248,6 +248,8 @@ struct hci_dev {
> 	__u32			req_status;
> 	__u32			req_result;
> 
> +	transaction_complete_t	transaction_complete;
> +
> 	__u16			init_last_cmd;
> 
> 	struct list_head	mgmt_pending;
> @@ -1041,6 +1043,17 @@ static inline u16 eir_append_data(u8 *eir, u16 eir_len, u8 type, u8 *data,
> int hci_register_cb(struct hci_cb *hcb);
> int hci_unregister_cb(struct hci_cb *hcb);
> 
> +struct hci_transaction {
> +	struct hci_dev		*hdev;
> +	struct sk_buff_head	cmd_q;
> +	transaction_complete_t	complete;
> +};
> +
> +void hci_transaction_init(struct hci_transaction *transaction,
> +			  struct hci_dev *hdev,
> +			  transaction_complete_t complete);
> +int hci_transaction_run(struct hci_transaction *transaction);
> +
> int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param);
> void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
> void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index f71ad76..ef051b7 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2435,6 +2435,35 @@ static int hci_send_frame(struct sk_buff *skb)
> 	return hdev->send(skb);
> }
> 
> +void hci_transaction_init(struct hci_transaction *transaction,
> +			  struct hci_dev *hdev,
> +			  transaction_complete_t complete)
> +{
> +	memset(transaction, 0, sizeof(*transaction));
> +	skb_queue_head_init(&transaction->cmd_q);
> +	transaction->hdev = hdev;
> +	transaction->complete = complete;
> +}
> +
> +int hci_transaction_run(struct hci_transaction *transaction)
> +{
> +	struct hci_dev *hdev = transaction->hdev;
> +
> +	BT_DBG("length %u", skb_queue_len(&transaction->cmd_q));
> +
> +	/* Do not allow empty transactions */
> +	if (skb_queue_empty(&transaction->cmd_q))
> +		return -EINVAL;
> +
> +	spin_lock(&hdev->cmd_q.lock);
> +	skb_queue_splice_tail(&transaction->cmd_q, &hdev->cmd_q);
> +	spin_unlock(&hdev->cmd_q.lock);
> +
> +	queue_work(hdev->workqueue, &hdev->cmd_work);
> +
> +	return 0;
> +}

I am wondering why we are not giving the complete handler when finishing the transaction. Having a copy of the handler in hci_dev structure seems a bit pointless. What is the reason here?

> +
> /* Send HCI command */
> int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
> {
> @@ -3209,6 +3238,11 @@ static void hci_cmd_work(struct work_struct *work)
> 		hdev->sent_cmd = skb_clone(skb, GFP_ATOMIC);
> 		if (hdev->sent_cmd) {
> 			atomic_dec(&hdev->cmd_cnt);
> +
> +			if (bt_cb(skb)->transaction.start)
> +				hdev->transaction_complete =
> +					bt_cb(skb)->transaction.complete;
> +
> 			hci_send_frame(skb);
> 			if (test_bit(HCI_RESET, &hdev->flags))
> 				del_timer(&hdev->cmd_timer);

Regards

Marcel

Re: [PATCH v3 03/16] Bluetooth: Add initial skeleton for HCI transaction framework

[Johan Hedberg]

Hi Marcel,

On Thu, Feb 28, 2013, Marcel Holtmann wrote:
> > +typedef void (*transaction_complete_t)(struct hci_dev *hdev, u16 last_cmd,
> > +				       int status);
> 
> why is status an int and not __u8?
> 
> We might want to prefix this with hci_ here. Just in case someone
> includes this header.

The int came from the old hci_req_complete() API, but you're right it
doesn't really make sense to have anything else than u8. I'll fix both
issues for the next iteration of the set.

> > +struct transaction_ctrl {
> 
> Same here. hci_ prefix seems like a good idea.

Sure.

> > +	__u8			start;
> 
> Why is not a bool and we are using true/false. Magic numbers like 1 in
> the code seem like a bad idea.

The main reason is that I wanted to control the amount of data we
consume from the control buffer (which is limited to 40 bytes) and the
size of bool is architecture dependent (could be 1 byte or 4 or
something else). E.g. all other existing variables part of the buffer
are either u8 or u16. We still have plenty of space left though and the
complete callback pointer is anyway not a fixed size, so if you're not
too worried that we'll continue extending the buffer in the future I'll
just change this to bool.

> > +int hci_transaction_run(struct hci_transaction *transaction)
> > +{
> > +	struct hci_dev *hdev = transaction->hdev;
> > +
> > +	BT_DBG("length %u", skb_queue_len(&transaction->cmd_q));
> > +
> > +	/* Do not allow empty transactions */
> > +	if (skb_queue_empty(&transaction->cmd_q))
> > +		return -EINVAL;
> > +
> > +	spin_lock(&hdev->cmd_q.lock);
> > +	skb_queue_splice_tail(&transaction->cmd_q, &hdev->cmd_q);
> > +	spin_unlock(&hdev->cmd_q.lock);
> > +
> > +	queue_work(hdev->workqueue, &hdev->cmd_work);
> > +
> > +	return 0;
> > +}
> 
> I am wondering why we are not giving the complete handler when
> finishing the transaction. Having a copy of the handler in hci_dev
> structure seems a bit pointless. What is the reason here?

The hdev->transaction_complete is not related to building transactions
but for running them. I can move giving the callback to the
transaction_run function and thereby remove the need to store it in
struct hci_transaction, but the fact remains that it still needs to be
copied to the first skb of the transaction (since the moment we start
processing the transaction the callback needs to be copied to
hdev->transaction_complete). Storing the callback in struct
hci_transaction made it easy to copy it to the first skb when
hci_transaction_cmd() gets called for the first time on the transaction.

So let me repeat, we need the hdev->complete_transaction since the
callback could be needed for any individual HCI command that's part of
the transaction in case that command fails (since then we stop
processing the transaction and call the callback). Because of this the
callback needs to be part of the first skb of the transaction.

Johan

Re: [PATCH v3 03/16] Bluetooth: Add initial skeleton for HCI transaction framework

[Marcel Holtmann]

Hi Johan,


>>> +typedef void (*transaction_complete_t)(struct hci_dev *hdev, u16 last_cmd,
>>> +				       int status);
>> 
>> why is status an int and not __u8?
>> 
>> We might want to prefix this with hci_ here. Just in case someone
>> includes this header.
> 
> The int came from the old hci_req_complete() API, but you're right it
> doesn't really make sense to have anything else than u8. I'll fix both
> issues for the next iteration of the set.
> 
>>> +struct transaction_ctrl {
>> 
>> Same here. hci_ prefix seems like a good idea.
> 
> Sure.
> 
>>> +	__u8			start;
>> 
>> Why is not a bool and we are using true/false. Magic numbers like 1 in
>> the code seem like a bad idea.
> 
> The main reason is that I wanted to control the amount of data we
> consume from the control buffer (which is limited to 40 bytes) and the
> size of bool is architecture dependent (could be 1 byte or 4 or
> something else). E.g. all other existing variables part of the buffer
> are either u8 or u16. We still have plenty of space left though and the
> complete callback pointer is anyway not a fixed size, so if you're not
> too worried that we'll continue extending the buffer in the future I'll
> just change this to bool.

since we are not short on space right now, lets use bool to clearly indicate the range we are expecting.

At some point, we might consider having separate control buffer between L2CAP and HCI, but lets not worry too much about that now.

> 
>>> +int hci_transaction_run(struct hci_transaction *transaction)
>>> +{
>>> +	struct hci_dev *hdev = transaction->hdev;
>>> +
>>> +	BT_DBG("length %u", skb_queue_len(&transaction->cmd_q));
>>> +
>>> +	/* Do not allow empty transactions */
>>> +	if (skb_queue_empty(&transaction->cmd_q))
>>> +		return -EINVAL;
>>> +
>>> +	spin_lock(&hdev->cmd_q.lock);
>>> +	skb_queue_splice_tail(&transaction->cmd_q, &hdev->cmd_q);
>>> +	spin_unlock(&hdev->cmd_q.lock);
>>> +
>>> +	queue_work(hdev->workqueue, &hdev->cmd_work);
>>> +
>>> +	return 0;
>>> +}
>> 
>> I am wondering why we are not giving the complete handler when
>> finishing the transaction. Having a copy of the handler in hci_dev
>> structure seems a bit pointless. What is the reason here?
> 
> The hdev->transaction_complete is not related to building transactions
> but for running them. I can move giving the callback to the
> transaction_run function and thereby remove the need to store it in
> struct hci_transaction, but the fact remains that it still needs to be
> copied to the first skb of the transaction (since the moment we start
> processing the transaction the callback needs to be copied to
> hdev->transaction_complete). Storing the callback in struct
> hci_transaction made it easy to copy it to the first skb when
> hci_transaction_cmd() gets called for the first time on the transaction.
> 
> So let me repeat, we need the hdev->complete_transaction since the
> callback could be needed for any individual HCI command that's part of
> the transaction in case that command fails (since then we stop
> processing the transaction and call the callback). Because of this the
> callback needs to be part of the first skb of the transaction.

The way I see this is that if one command of the transaction fails, we need to not continue and just discard the rest of the transaction. Why not just go through the queue and find the complete callback attached with the last skb of transaction. Either that is the last successful command, or we had to go and remove the rest of the queue.

This reminds me. Have you tested this with random commands in the transaction failing?

Regards

Marcel

Re: [PATCH v3 03/16] Bluetooth: Add initial skeleton for HCI transaction framework

[Johan Hedberg]

Hi Marcel,

On Thu, Feb 28, 2013, Marcel Holtmann wrote:
> >>> +int hci_transaction_run(struct hci_transaction *transaction)
> >>> +{
> >>> +	struct hci_dev *hdev = transaction->hdev;
> >>> +
> >>> +	BT_DBG("length %u", skb_queue_len(&transaction->cmd_q));
> >>> +
> >>> +	/* Do not allow empty transactions */
> >>> +	if (skb_queue_empty(&transaction->cmd_q))
> >>> +		return -EINVAL;
> >>> +
> >>> +	spin_lock(&hdev->cmd_q.lock);
> >>> +	skb_queue_splice_tail(&transaction->cmd_q, &hdev->cmd_q);
> >>> +	spin_unlock(&hdev->cmd_q.lock);
> >>> +
> >>> +	queue_work(hdev->workqueue, &hdev->cmd_work);
> >>> +
> >>> +	return 0;
> >>> +}
> >> 
> >> I am wondering why we are not giving the complete handler when
> >> finishing the transaction. Having a copy of the handler in hci_dev
> >> structure seems a bit pointless. What is the reason here?
> > 
> > The hdev->transaction_complete is not related to building transactions
> > but for running them. I can move giving the callback to the
> > transaction_run function and thereby remove the need to store it in
> > struct hci_transaction, but the fact remains that it still needs to be
> > copied to the first skb of the transaction (since the moment we start
> > processing the transaction the callback needs to be copied to
> > hdev->transaction_complete). Storing the callback in struct
> > hci_transaction made it easy to copy it to the first skb when
> > hci_transaction_cmd() gets called for the first time on the transaction.
> > 
> > So let me repeat, we need the hdev->complete_transaction since the
> > callback could be needed for any individual HCI command that's part of
> > the transaction in case that command fails (since then we stop
> > processing the transaction and call the callback). Because of this the
> > callback needs to be part of the first skb of the transaction.
> 
> The way I see this is that if one command of the transaction fails, we
> need to not continue and just discard the rest of the transaction. Why
> not just go through the queue and find the complete callback attached
> with the last skb of transaction. Either that is the last successful
> command, or we had to go and remove the rest of the queue.

A command that completes it not part of hdev->cmd_q anymore but can be
found in hdev->sent_cmd instead. This means that to find the complete
callback we first need to check for
bt_cb(hdev->sent_cmd)->transaction.complete and if it's NULL start going
through hdev->cmd_q. This is more complicated than just checking for
hdev->transaction_complete, but if you think it's worth not having to
add another hdev member then I'll go with it.

Another problem is that the control buffer gets lost when doing
skb_clone. This means we have to add a memcpy of the skb->cb after doing
the hdev->sent_cmd = skb_clone(skb) in hci_cmd_work.

> This reminds me. Have you tested this with random commands in the
> transaction failing?

I *think* I tested it, but I'll make sure to do it at least before
sending the next patch set iteration.

Johan

Re: [PATCH v3 03/16] Bluetooth: Add initial skeleton for HCI transaction framework

[Johan Hedberg]

Hi Marcel,

On Fri, Mar 01, 2013, Johan Hedberg wrote:
> > > The hdev->transaction_complete is not related to building transactions
> > > but for running them. I can move giving the callback to the
> > > transaction_run function and thereby remove the need to store it in
> > > struct hci_transaction, but the fact remains that it still needs to be
> > > copied to the first skb of the transaction (since the moment we start
> > > processing the transaction the callback needs to be copied to
> > > hdev->transaction_complete). Storing the callback in struct
> > > hci_transaction made it easy to copy it to the first skb when
> > > hci_transaction_cmd() gets called for the first time on the transaction.
> > > 
> > > So let me repeat, we need the hdev->complete_transaction since the
> > > callback could be needed for any individual HCI command that's part of
> > > the transaction in case that command fails (since then we stop
> > > processing the transaction and call the callback). Because of this the
> > > callback needs to be part of the first skb of the transaction.
> > 
> > The way I see this is that if one command of the transaction fails, we
> > need to not continue and just discard the rest of the transaction. Why
> > not just go through the queue and find the complete callback attached
> > with the last skb of transaction. Either that is the last successful
> > command, or we had to go and remove the rest of the queue.
> 
> A command that completes it not part of hdev->cmd_q anymore but can be
> found in hdev->sent_cmd instead. This means that to find the complete
> callback we first need to check for
> bt_cb(hdev->sent_cmd)->transaction.complete and if it's NULL start going
> through hdev->cmd_q. This is more complicated than just checking for
> hdev->transaction_complete, but if you think it's worth not having to
> add another hdev member then I'll go with it.
> 
> Another problem is that the control buffer gets lost when doing
> skb_clone. This means we have to add a memcpy of the skb->cb after doing
> the hdev->sent_cmd = skb_clone(skb) in hci_cmd_work.

Thinking a bit more about this we should probably be passing the clone
to the HCI driver and not the original skb. The skb API documentation
talks about doing skb_clone() to retain ownership of the control buffer
between different layers and it's quite natural to consider the HCI
driver one layer and the HCI core another. I.e. the original skb
"belongs" to the HCI core and only the clone should be passed to the HCI
driver. This however means that we'll need to pass at least the pkt_type
as a separate parameter to hci_send_frame().

Johan

Re: [PATCH v3 03/16] Bluetooth: Add initial skeleton for HCI transaction framework

[Marcel Holtmann]

Hi Johan,


>>>> The hdev->transaction_complete is not related to building transactions
>>>> but for running them. I can move giving the callback to the
>>>> transaction_run function and thereby remove the need to store it in
>>>> struct hci_transaction, but the fact remains that it still needs to be
>>>> copied to the first skb of the transaction (since the moment we start
>>>> processing the transaction the callback needs to be copied to
>>>> hdev->transaction_complete). Storing the callback in struct
>>>> hci_transaction made it easy to copy it to the first skb when
>>>> hci_transaction_cmd() gets called for the first time on the transaction.
>>>> 
>>>> So let me repeat, we need the hdev->complete_transaction since the
>>>> callback could be needed for any individual HCI command that's part of
>>>> the transaction in case that command fails (since then we stop
>>>> processing the transaction and call the callback). Because of this the
>>>> callback needs to be part of the first skb of the transaction.
>>> 
>>> The way I see this is that if one command of the transaction fails, we
>>> need to not continue and just discard the rest of the transaction. Why
>>> not just go through the queue and find the complete callback attached
>>> with the last skb of transaction. Either that is the last successful
>>> command, or we had to go and remove the rest of the queue.
>> 
>> A command that completes it not part of hdev->cmd_q anymore but can be
>> found in hdev->sent_cmd instead. This means that to find the complete
>> callback we first need to check for
>> bt_cb(hdev->sent_cmd)->transaction.complete and if it's NULL start going
>> through hdev->cmd_q. This is more complicated than just checking for
>> hdev->transaction_complete, but if you think it's worth not having to
>> add another hdev member then I'll go with it.
>> 
>> Another problem is that the control buffer gets lost when doing
>> skb_clone. This means we have to add a memcpy of the skb->cb after doing
>> the hdev->sent_cmd = skb_clone(skb) in hci_cmd_work.
> 
> Thinking a bit more about this we should probably be passing the clone
> to the HCI driver and not the original skb. The skb API documentation
> talks about doing skb_clone() to retain ownership of the control buffer
> between different layers and it's quite natural to consider the HCI
> driver one layer and the HCI core another. I.e. the original skb
> "belongs" to the HCI core and only the clone should be passed to the HCI
> driver. This however means that we'll need to pass at least the pkt_type
> as a separate parameter to hci_send_frame().


or we just set the pkt_type in the cb of the cloned skb. Do we also have to set the dev value for hci_dev with it?

Sounds like a good idea, but I think the reason why we were never doing it is that it has an impact on ACL and SCO packets where we would be doing a clone for no reason.

Regards

Marcel

Re: [PATCH v3 03/16] Bluetooth: Add initial skeleton for HCI transaction framework

[Marcel Holtmann]

Hi Johan,


>>>>> +int hci_transaction_run(struct hci_transaction *transaction)
>>>>> +{
>>>>> +	struct hci_dev *hdev = transaction->hdev;
>>>>> +
>>>>> +	BT_DBG("length %u", skb_queue_len(&transaction->cmd_q));
>>>>> +
>>>>> +	/* Do not allow empty transactions */
>>>>> +	if (skb_queue_empty(&transaction->cmd_q))
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	spin_lock(&hdev->cmd_q.lock);
>>>>> +	skb_queue_splice_tail(&transaction->cmd_q, &hdev->cmd_q);
>>>>> +	spin_unlock(&hdev->cmd_q.lock);
>>>>> +
>>>>> +	queue_work(hdev->workqueue, &hdev->cmd_work);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>> 
>>>> I am wondering why we are not giving the complete handler when
>>>> finishing the transaction. Having a copy of the handler in hci_dev
>>>> structure seems a bit pointless. What is the reason here?
>>> 
>>> The hdev->transaction_complete is not related to building transactions
>>> but for running them. I can move giving the callback to the
>>> transaction_run function and thereby remove the need to store it in
>>> struct hci_transaction, but the fact remains that it still needs to be
>>> copied to the first skb of the transaction (since the moment we start
>>> processing the transaction the callback needs to be copied to
>>> hdev->transaction_complete). Storing the callback in struct
>>> hci_transaction made it easy to copy it to the first skb when
>>> hci_transaction_cmd() gets called for the first time on the transaction.
>>> 
>>> So let me repeat, we need the hdev->complete_transaction since the
>>> callback could be needed for any individual HCI command that's part of
>>> the transaction in case that command fails (since then we stop
>>> processing the transaction and call the callback). Because of this the
>>> callback needs to be part of the first skb of the transaction.
>> 
>> The way I see this is that if one command of the transaction fails, we
>> need to not continue and just discard the rest of the transaction. Why
>> not just go through the queue and find the complete callback attached
>> with the last skb of transaction. Either that is the last successful
>> command, or we had to go and remove the rest of the queue.
> 
> A command that completes it not part of hdev->cmd_q anymore but can be
> found in hdev->sent_cmd instead. This means that to find the complete
> callback we first need to check for
> bt_cb(hdev->sent_cmd)->transaction.complete and if it's NULL start going
> through hdev->cmd_q. This is more complicated than just checking for
> hdev->transaction_complete, but if you think it's worth not having to
> add another hdev member then I'll go with it.
> 
> Another problem is that the control buffer gets lost when doing
> skb_clone. This means we have to add a memcpy of the skb->cb after doing
> the hdev->sent_cmd = skb_clone(skb) in hci_cmd_work.

maybe we are trying to hard to make this fit with our existing model and we just need to redo it. The feeling that I am getting is that a complete callback function "temporary" storage inside hci_dev is a pretty bad idea. It should be part of the context of transaction itself and this mean the skb.

Not that we have been really great with this in the past, we clearly were not. But since we are touching tis code now, I might make sense to have a look on how HCI command handling can be done better.

Same goes for the sent_cmd data. As I said, in userspace we either keep the current packet at the head of the queue until it gets a response or we create a separate pending queue for the packets in fly. Since we only allow one HCI command at a time anyway, maybe just hanging on to cmd skb would be a good idea and in conjunction with your other comment, give a clone to the driver.

>> This reminds me. Have you tested this with random commands in the
>> transaction failing?
> 
> I *think* I tested it, but I'll make sure to do it at least before
> sending the next patch set iteration.

Please test this. Since if any comment of the transaction fails, we need to remove all pending commands of that said transaction.

Regards

Marcel

[PATCH v3 04/16] Bluetooth: Refactor HCI command skb creation

[Johan Hedberg]

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

This patch moves out the skb creation from hci_send_cmd() into its own
prepare_cmd() function. This is essential so the same prepare_cmd()
function can be easily reused for skb creation for HCI transactions.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ef051b7..8def4e6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2464,20 +2464,16 @@ int hci_transaction_run(struct hci_transaction *transaction)
 	return 0;
 }
 
-/* Send HCI command */
-int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
+static struct sk_buff *hci_prepare_cmd(struct hci_dev *hdev, u16 opcode,
+				       u32 plen, void *param)
 {
 	int len = HCI_COMMAND_HDR_SIZE + plen;
 	struct hci_command_hdr *hdr;
 	struct sk_buff *skb;
 
-	BT_DBG("%s opcode 0x%4.4x plen %d", hdev->name, opcode, plen);
-
 	skb = bt_skb_alloc(len, GFP_ATOMIC);
-	if (!skb) {
-		BT_ERR("%s no memory for command", hdev->name);
-		return -ENOMEM;
-	}
+	if (!skb)
+		return NULL;
 
 	hdr = (struct hci_command_hdr *) skb_put(skb, HCI_COMMAND_HDR_SIZE);
 	hdr->opcode = cpu_to_le16(opcode);
@@ -2491,6 +2487,22 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
 	bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
 	skb->dev = (void *) hdev;
 
+	return skb;
+}
+
+/* Send HCI command */
+int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
+{
+	struct sk_buff *skb;
+
+	BT_DBG("%s opcode 0x%4.4x plen %d", hdev->name, opcode, plen);
+
+	skb = hci_prepare_cmd(hdev, opcode, plen, param);
+	if (!skb) {
+		BT_ERR("%s no memory for command", hdev->name);
+		return -ENOMEM;
+	}
+
 	if (test_bit(HCI_INIT, &hdev->flags))
 		hdev->init_last_cmd = opcode;
 
-- 
1.7.10.4

[PATCH v3 05/16] Bluetooth: Introduce new hci_transaction_cmd function

[Johan Hedberg]

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

This function is analogous to hci_send_cmd() but instead of directly
queuing the command to hdev->cmd_q it adds it to the local queue of the
transaction being build (in struct hci_transaction).

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e97d8e5..777005c 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1053,6 +1053,8 @@ void hci_transaction_init(struct hci_transaction *transaction,
 			  struct hci_dev *hdev,
 			  transaction_complete_t complete);
 int hci_transaction_run(struct hci_transaction *transaction);
+int hci_transaction_cmd(struct hci_transaction *transaction, u16 opcode,
+			u32 plen, void *param);
 
 int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param);
 void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 8def4e6..5e54743 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -175,6 +175,16 @@ static int hci_request(struct hci_dev *hdev,
 	return ret;
 }
 
+/* Mark an skb as the starting point for a transaction */
+static void hci_transaction_start(struct sk_buff *skb,
+				  transaction_complete_t complete)
+{
+	struct transaction_ctrl *cb = &bt_cb(skb)->transaction;
+
+	cb->start = 1;
+	cb->complete = complete;
+}
+
 static void hci_reset_req(struct hci_dev *hdev, unsigned long opt)
 {
 	BT_DBG("%s %ld", hdev->name, opt);
@@ -2512,6 +2522,29 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
 	return 0;
 }
 
+/* Queue a command to a HCI transaction */
+int hci_transaction_cmd(struct hci_transaction *transaction, u16 opcode,
+			u32 plen, void *param)
+{
+	struct hci_dev *hdev = transaction->hdev;
+	struct sk_buff *skb;
+
+	BT_DBG("%s opcode 0x%4.4x plen %d", hdev->name, opcode, plen);
+
+	skb = hci_prepare_cmd(hdev, opcode, plen, param);
+	if (!skb) {
+		BT_ERR("%s no memory for command", hdev->name);
+		return -ENOMEM;
+	}
+
+	if (skb_queue_empty(&transaction->cmd_q))
+		hci_transaction_start(skb, transaction->complete);
+
+	skb_queue_tail(&transaction->cmd_q, skb);
+
+	return 0;
+}
+
 /* Get data from the previously sent command */
 void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
 {
-- 
1.7.10.4

[PATCH v3 06/16] Bluetooth: Introduce a hci_transaction_from_skb function

[Johan Hedberg]

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

To have a consistent content for hdev->cmd_q all entries need to follow
the semantics of HCI transactions. This means that even single commands
need to be dressed as transactions. This patch introduces a new function
to create a single-command transaction and converts the two places
needing this (hci_send_cmd and hci_sock_sendmsg) to use it.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_core.c         |   11 +++++++++--
 net/bluetooth/hci_sock.c         |    3 +--
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 777005c..8c2553f 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1055,6 +1055,7 @@ void hci_transaction_init(struct hci_transaction *transaction,
 int hci_transaction_run(struct hci_transaction *transaction);
 int hci_transaction_cmd(struct hci_transaction *transaction, u16 opcode,
 			u32 plen, void *param);
+void hci_transaction_from_skb(struct hci_dev *hdev, struct sk_buff *skb);
 
 int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param);
 void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5e54743..315728c 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2474,6 +2474,14 @@ int hci_transaction_run(struct hci_transaction *transaction)
 	return 0;
 }
 
+void hci_transaction_from_skb(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	hci_transaction_start(skb, NULL);
+
+	skb_queue_tail(&hdev->cmd_q, skb);
+	queue_work(hdev->workqueue, &hdev->cmd_work);
+}
+
 static struct sk_buff *hci_prepare_cmd(struct hci_dev *hdev, u16 opcode,
 				       u32 plen, void *param)
 {
@@ -2516,8 +2524,7 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
 	if (test_bit(HCI_INIT, &hdev->flags))
 		hdev->init_last_cmd = opcode;
 
-	skb_queue_tail(&hdev->cmd_q, skb);
-	queue_work(hdev->workqueue, &hdev->cmd_work);
+	hci_transaction_from_skb(hdev, skb);
 
 	return 0;
 }
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 20c92de..d6f1cbe 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -859,8 +859,7 @@ static int hci_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 			skb_queue_tail(&hdev->raw_q, skb);
 			queue_work(hdev->workqueue, &hdev->tx_work);
 		} else {
-			skb_queue_tail(&hdev->cmd_q, skb);
-			queue_work(hdev->workqueue, &hdev->cmd_work);
+			hci_transaction_from_skb(hdev, skb);
 		}
 	} else {
 		if (!capable(CAP_NET_RAW)) {
-- 
1.7.10.4

[PATCH v3 07/16] Bluetooth: Add transaction cmd_complete and cmd_status functions

[Johan Hedberg]

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

This patch introduces functions to process the transaction state when
receiving HCI Command Status or Command Complete events. Some HCI
commands, like Inquiry do not result in a Command complete event so
special handling is needed for them. Inquiry is a particularly important
one since it is the only forseeable "non-cmd_complete" command that will
make good use of the transaction framework, and its completion is either
indicated by an Inquiry Complete event of a successful Command Complete
for HCI_Inquiry_Cancel.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |    2 ++
 net/bluetooth/hci_core.c         |   68 ++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |    7 ++++
 3 files changed, 77 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8c2553f..038f8c7a 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1056,6 +1056,8 @@ int hci_transaction_run(struct hci_transaction *transaction);
 int hci_transaction_cmd(struct hci_transaction *transaction, u16 opcode,
 			u32 plen, void *param);
 void hci_transaction_from_skb(struct hci_dev *hdev, struct sk_buff *skb);
+void hci_transaction_cmd_complete(struct hci_dev *hdev, u16 opcode, u8 status);
+void hci_transaction_cmd_status(struct hci_dev *hdev, u16 opcode, u8 status);
 
 int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param);
 void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 315728c..15ec531 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3216,6 +3216,74 @@ static void hci_scodata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 	kfree_skb(skb);
 }
 
+static bool hci_transaction_is_complete(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+
+	skb = skb_peek(&hdev->cmd_q);
+	if (!skb)
+		return true;
+
+	return bt_cb(skb)->transaction.start;
+}
+
+void hci_transaction_cmd_complete(struct hci_dev *hdev, u16 opcode, u8 status)
+{
+	struct sk_buff *skb;
+
+	BT_DBG("opcode 0x%04x status 0x%02x", opcode, status);
+
+	/* Check that the completed command really matches the last one
+	 * that was sent.
+	 */
+	if (!hci_sent_cmd_data(hdev, opcode))
+		return;
+
+	/* If the command succeeded and there's still more commands in
+	 * this transaction the transaction is not yet complete.
+	 */
+	if (!status && !hci_transaction_is_complete(hdev))
+		return;
+
+	/* Remove all pending commands belonging to this transaction */
+	spin_lock(&hdev->cmd_q.lock);
+	while ((skb = __skb_dequeue(&hdev->cmd_q))) {
+		if (bt_cb(skb)->transaction.start) {
+			__skb_queue_head(&hdev->cmd_q, skb);
+			break;
+		}
+
+		kfree_skb(skb);
+	}
+	spin_unlock(&hdev->cmd_q.lock);
+
+	if (hdev->transaction_complete) {
+		hdev->transaction_complete(hdev, opcode, status);
+		hdev->transaction_complete = NULL;
+	}
+}
+
+void hci_transaction_cmd_status(struct hci_dev *hdev, u16 opcode, u8 status)
+{
+	BT_DBG("opcode 0x%04x status 0x%02x", opcode, status);
+
+	if (status) {
+		hci_transaction_cmd_complete(hdev, opcode, status);
+		return;
+	}
+
+	/* No need to handle success status if there are more commands */
+	if (!hci_transaction_is_complete(hdev))
+		return;
+
+	/* If the transaction doesn't have a complete callback or there
+	 * are other commands/transactions in the hdev queue we consider
+	 * this transaction as completed.
+	 */
+	if (!hdev->transaction_complete || !skb_queue_empty(&hdev->cmd_q))
+		hci_transaction_cmd_complete(hdev, opcode, status);
+}
+
 static void hci_rx_work(struct work_struct *work)
 {
 	struct hci_dev *hdev = container_of(work, struct hci_dev, rx_work);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 14e872a..6dd5fd4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -53,6 +53,7 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 	hci_dev_unlock(hdev);
 
+	hci_transaction_cmd_complete(hdev, HCI_OP_INQUIRY, status);
 	hci_req_complete(hdev, HCI_OP_INQUIRY_CANCEL, status);
 
 	hci_conn_check_pending(hdev);
@@ -1692,6 +1693,7 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
 
+	hci_transaction_cmd_complete(hdev, HCI_OP_INQUIRY, status);
 	hci_req_complete(hdev, HCI_OP_INQUIRY, status);
 
 	hci_conn_check_pending(hdev);
@@ -2254,6 +2256,7 @@ static void hci_qos_setup_complete_evt(struct hci_dev *hdev,
 static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_cmd_complete *ev = (void *) skb->data;
+	u8 status = skb->data[sizeof(*ev)];
 	__u16 opcode;
 
 	skb_pull(skb, sizeof(*ev));
@@ -2497,6 +2500,8 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	if (ev->opcode != HCI_OP_NOP)
 		del_timer(&hdev->cmd_timer);
 
+	hci_transaction_cmd_complete(hdev, ev->opcode, status);
+
 	if (ev->ncmd && !test_bit(HCI_RESET, &hdev->flags)) {
 		atomic_set(&hdev->cmd_cnt, 1);
 		if (!skb_queue_empty(&hdev->cmd_q))
@@ -2590,6 +2595,8 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	if (ev->opcode != HCI_OP_NOP)
 		del_timer(&hdev->cmd_timer);
 
+	hci_transaction_cmd_status(hdev, ev->opcode, ev->status);
+
 	if (ev->ncmd && !test_bit(HCI_RESET, &hdev->flags)) {
 		atomic_set(&hdev->cmd_cnt, 1);
 		if (!skb_queue_empty(&hdev->cmd_q))
-- 
1.7.10.4

[PATCH v3 08/16] Bluetooth: Convert hci_request to use HCI transaction framework

[Johan Hedberg]

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

This patch converts the hci_request() procedure to use the HCI
transaction framework. The consequence is that hci_req_complete()
becomes private to hci_core.c (the extra reset command handling needs to
be moved into hci_transaction_cmd_complete) and the HCI driver init
commands are run in their own transaction prior to the HCI init
requests.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |    2 -
 net/bluetooth/hci_core.c         |  296 +++++++++++++++++++++++---------------
 net/bluetooth/hci_event.c        |   78 +---------
 3 files changed, 181 insertions(+), 195 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 038f8c7a..805551b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1171,8 +1171,6 @@ struct hci_sec_filter {
 #define hci_req_lock(d)		mutex_lock(&d->req_lock)
 #define hci_req_unlock(d)	mutex_unlock(&d->req_lock)
 
-void hci_req_complete(struct hci_dev *hdev, __u16 cmd, int result);
-
 void hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max,
 					u16 latency, u16 to_multiplier);
 void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 15ec531..2657a9e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -57,37 +57,10 @@ static void hci_notify(struct hci_dev *hdev, int event)
 
 /* ---- HCI requests ---- */
 
-void hci_req_complete(struct hci_dev *hdev, __u16 cmd, int result)
+static void hci_req_complete(struct hci_dev *hdev, u16 cmd, int result)
 {
 	BT_DBG("%s command 0x%4.4x result 0x%2.2x", hdev->name, cmd, result);
 
-	/* If this is the init phase check if the completed command matches
-	 * the last init command, and if not just return.
-	 */
-	if (test_bit(HCI_INIT, &hdev->flags) && hdev->init_last_cmd != cmd) {
-		struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
-		u16 opcode = __le16_to_cpu(sent->opcode);
-		struct sk_buff *skb;
-
-		/* Some CSR based controllers generate a spontaneous
-		 * reset complete event during init and any pending
-		 * command will never be completed. In such a case we
-		 * need to resend whatever was the last sent
-		 * command.
-		 */
-
-		if (cmd != HCI_OP_RESET || opcode == HCI_OP_RESET)
-			return;
-
-		skb = skb_clone(hdev->sent_cmd, GFP_ATOMIC);
-		if (skb) {
-			skb_queue_head(&hdev->cmd_q, skb);
-			queue_work(hdev->workqueue, &hdev->cmd_work);
-		}
-
-		return;
-	}
-
 	if (hdev->req_status == HCI_REQ_PEND) {
 		hdev->req_result = result;
 		hdev->req_status = HCI_REQ_DONE;
@@ -108,26 +81,36 @@ static void hci_req_cancel(struct hci_dev *hdev, int err)
 
 /* Execute request and wait for completion. */
 static int __hci_request(struct hci_dev *hdev,
-			 void (*req)(struct hci_dev *hdev, unsigned long opt),
+			 void (*req)(struct hci_transaction *transaction,
+				     unsigned long opt),
 			 unsigned long opt, __u32 timeout)
 {
+	struct hci_transaction transaction;
 	DECLARE_WAITQUEUE(wait, current);
 	int err = 0;
 
 	BT_DBG("%s start", hdev->name);
 
+	hci_transaction_init(&transaction, hdev, hci_req_complete);
+
 	hdev->req_status = HCI_REQ_PEND;
 
 	add_wait_queue(&hdev->req_wait_q, &wait);
 	set_current_state(TASK_INTERRUPTIBLE);
 
-	req(hdev, opt);
+	req(&transaction, opt);
 
-	/* If the request didn't send any commands return immediately */
-	if (skb_queue_empty(&hdev->cmd_q) && atomic_read(&hdev->cmd_cnt)) {
+	err = hci_transaction_run(&transaction);
+	if (err < 0) {
 		hdev->req_status = 0;
 		remove_wait_queue(&hdev->req_wait_q, &wait);
-		return err;
+		/* transaction_run will fail if the request did not add
+		 * any commands to the queue, something that can happen
+		 * when a request with conditionals doesn't trigger any
+		 * commands to be sent. This is normal behavior and
+		 * should not trigger an error return.
+		 */
+		return 0;
 	}
 
 	schedule_timeout(timeout);
@@ -159,7 +142,8 @@ static int __hci_request(struct hci_dev *hdev,
 }
 
 static int hci_request(struct hci_dev *hdev,
-		       void (*req)(struct hci_dev *hdev, unsigned long opt),
+		       void (*req)(struct hci_transaction *transaction,
+				   unsigned long opt),
 		       unsigned long opt, __u32 timeout)
 {
 	int ret;
@@ -185,72 +169,82 @@ static void hci_transaction_start(struct sk_buff *skb,
 	cb->complete = complete;
 }
 
-static void hci_reset_req(struct hci_dev *hdev, unsigned long opt)
+static void hci_reset_req(struct hci_transaction *transaction,
+			  unsigned long opt)
 {
-	BT_DBG("%s %ld", hdev->name, opt);
+	BT_DBG("%s %ld", transaction->hdev->name, opt);
 
 	/* Reset device */
-	set_bit(HCI_RESET, &hdev->flags);
-	hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
+	set_bit(HCI_RESET, &transaction->hdev->flags);
+	hci_transaction_cmd(transaction, HCI_OP_RESET, 0, NULL);
 }
 
-static void bredr_init(struct hci_dev *hdev)
+static void bredr_init(struct hci_transaction *transaction)
 {
-	hdev->flow_ctl_mode = HCI_FLOW_CTL_MODE_PACKET_BASED;
+	transaction->hdev->flow_ctl_mode = HCI_FLOW_CTL_MODE_PACKET_BASED;
 
 	/* Read Local Supported Features */
-	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
 
 	/* Read Local Version */
-	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
 
 	/* Read BD Address */
-	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_READ_BD_ADDR, 0, NULL);
 }
 
-static void amp_init(struct hci_dev *hdev)
+static void amp_init(struct hci_transaction *transaction)
 {
-	hdev->flow_ctl_mode = HCI_FLOW_CTL_MODE_BLOCK_BASED;
+	transaction->hdev->flow_ctl_mode = HCI_FLOW_CTL_MODE_BLOCK_BASED;
 
 	/* Read Local Version */
-	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
 
 	/* Read Local AMP Info */
-	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
 
 	/* Read Data Blk size */
-	hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
 }
 
-static void hci_init1_req(struct hci_dev *hdev, unsigned long opt)
+static void hci_init1_req(struct hci_transaction *transaction,
+			  unsigned long opt)
 {
+	struct hci_dev *hdev = transaction->hdev;
+	struct hci_transaction init_xact;
 	struct sk_buff *skb;
 
 	BT_DBG("%s %ld", hdev->name, opt);
 
 	/* Driver initialization */
 
+	hci_transaction_init(&init_xact, hdev, NULL);
+
 	/* Special commands */
 	while ((skb = skb_dequeue(&hdev->driver_init))) {
 		bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
 		skb->dev = (void *) hdev;
 
-		skb_queue_tail(&hdev->cmd_q, skb);
-		queue_work(hdev->workqueue, &hdev->cmd_work);
+		if (skb_queue_empty(&init_xact.cmd_q))
+			hci_transaction_start(skb, NULL);
+
+		skb_queue_tail(&init_xact.cmd_q, skb);
 	}
 	skb_queue_purge(&hdev->driver_init);
 
+	hci_transaction_run(&init_xact);
+
 	/* Reset */
 	if (!test_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks))
-		hci_reset_req(hdev, 0);
+		hci_reset_req(transaction, 0);
 
 	switch (hdev->dev_type) {
 	case HCI_BREDR:
-		bredr_init(hdev);
+		bredr_init(transaction);
 		break;
 
 	case HCI_AMP:
-		amp_init(hdev);
+		amp_init(transaction);
 		break;
 
 	default:
@@ -259,53 +253,57 @@ static void hci_init1_req(struct hci_dev *hdev, unsigned long opt)
 	}
 }
 
-static void bredr_setup(struct hci_dev *hdev)
+static void bredr_setup(struct hci_transaction *transaction)
 {
 	struct hci_cp_delete_stored_link_key cp;
 	__le16 param;
 	__u8 flt_type;
 
 	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
-	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
 
 	/* Read Class of Device */
-	hci_send_cmd(hdev, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);
 
 	/* Read Local Name */
-	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_READ_LOCAL_NAME, 0, NULL);
 
 	/* Read Voice Setting */
-	hci_send_cmd(hdev, HCI_OP_READ_VOICE_SETTING, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_READ_VOICE_SETTING, 0, NULL);
 
 	/* Clear Event Filters */
 	flt_type = HCI_FLT_CLEAR_ALL;
-	hci_send_cmd(hdev, HCI_OP_SET_EVENT_FLT, 1, &flt_type);
+	hci_transaction_cmd(transaction, HCI_OP_SET_EVENT_FLT, 1, &flt_type);
 
 	/* Connection accept timeout ~20 secs */
 	param = __constant_cpu_to_le16(0x7d00);
-	hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
+	hci_transaction_cmd(transaction, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
 
 	bacpy(&cp.bdaddr, BDADDR_ANY);
 	cp.delete_all = 1;
-	hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
+	hci_transaction_cmd(transaction, HCI_OP_DELETE_STORED_LINK_KEY,
+			    sizeof(cp), &cp);
 }
 
-static void le_setup(struct hci_dev *hdev)
+static void le_setup(struct hci_transaction *transaction)
 {
 	/* Read LE Buffer Size */
-	hci_send_cmd(hdev, HCI_OP_LE_READ_BUFFER_SIZE, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_LE_READ_BUFFER_SIZE, 0, NULL);
 
 	/* Read LE Local Supported Features */
-	hci_send_cmd(hdev, HCI_OP_LE_READ_LOCAL_FEATURES, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_LE_READ_LOCAL_FEATURES,
+			    0, NULL);
 
 	/* Read LE Advertising Channel TX Power */
-	hci_send_cmd(hdev, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL);
 
 	/* Read LE White List Size */
-	hci_send_cmd(hdev, HCI_OP_LE_READ_WHITE_LIST_SIZE, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_LE_READ_WHITE_LIST_SIZE,
+			    0, NULL);
 
 	/* Read LE Supported States */
-	hci_send_cmd(hdev, HCI_OP_LE_READ_SUPPORTED_STATES, 0, NULL);
+	hci_transaction_cmd(transaction, HCI_OP_LE_READ_SUPPORTED_STATES,
+			    0, NULL);
 }
 
 static u8 hci_get_inquiry_mode(struct hci_dev *hdev)
@@ -336,17 +334,19 @@ static u8 hci_get_inquiry_mode(struct hci_dev *hdev)
 	return 0;
 }
 
-static void hci_setup_inquiry_mode(struct hci_dev *hdev)
+static void hci_setup_inquiry_mode(struct hci_transaction *transaction)
 {
 	u8 mode;
 
-	mode = hci_get_inquiry_mode(hdev);
+	mode = hci_get_inquiry_mode(transaction->hdev);
 
-	hci_send_cmd(hdev, HCI_OP_WRITE_INQUIRY_MODE, 1, &mode);
+	hci_transaction_cmd(transaction, HCI_OP_WRITE_INQUIRY_MODE, 1, &mode);
 }
 
-static void hci_setup_event_mask(struct hci_dev *hdev)
+static void hci_setup_event_mask(struct hci_transaction *transaction)
 {
+	struct hci_dev *hdev = transaction->hdev;
+
 	/* The second byte is 0xff instead of 0x9f (two reserved bits
 	 * disabled) since a Broadcom 1.2 dongle doesn't respond to the
 	 * command otherwise.
@@ -402,67 +402,75 @@ static void hci_setup_event_mask(struct hci_dev *hdev)
 	if (lmp_le_capable(hdev))
 		events[7] |= 0x20;	/* LE Meta-Event */
 
-	hci_send_cmd(hdev, HCI_OP_SET_EVENT_MASK, sizeof(events), events);
+	hci_transaction_cmd(transaction, HCI_OP_SET_EVENT_MASK,
+			    sizeof(events), events);
 
 	if (lmp_le_capable(hdev)) {
 		memset(events, 0, sizeof(events));
 		events[0] = 0x1f;
-		hci_send_cmd(hdev, HCI_OP_LE_SET_EVENT_MASK,
-			     sizeof(events), events);
+		hci_transaction_cmd(transaction, HCI_OP_LE_SET_EVENT_MASK,
+				    sizeof(events), events);
 	}
 }
 
-static void hci_init2_req(struct hci_dev *hdev, unsigned long opt)
+static void hci_init2_req(struct hci_transaction *transaction,
+			  unsigned long opt)
 {
+	struct hci_dev *hdev = transaction->hdev;
+
 	if (lmp_bredr_capable(hdev))
-		bredr_setup(hdev);
+		bredr_setup(transaction);
 
 	if (lmp_le_capable(hdev))
-		le_setup(hdev);
+		le_setup(transaction);
 
-	hci_setup_event_mask(hdev);
+	hci_setup_event_mask(transaction);
 
 	if (hdev->hci_ver > BLUETOOTH_VER_1_1)
-		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_COMMANDS, 0, NULL);
+		hci_transaction_cmd(transaction, HCI_OP_READ_LOCAL_COMMANDS,
+				    0, NULL);
 
 	if (lmp_ssp_capable(hdev)) {
 		if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags)) {
 			u8 mode = 0x01;
-			hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE,
-				     sizeof(mode), &mode);
+			hci_transaction_cmd(transaction, HCI_OP_WRITE_SSP_MODE,
+					    sizeof(mode), &mode);
 		} else {
 			struct hci_cp_write_eir cp;
 
 			memset(hdev->eir, 0, sizeof(hdev->eir));
 			memset(&cp, 0, sizeof(cp));
 
-			hci_send_cmd(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp);
+			hci_transaction_cmd(transaction, HCI_OP_WRITE_EIR,
+					    sizeof(cp), &cp);
 		}
 	}
 
 	if (lmp_inq_rssi_capable(hdev))
-		hci_setup_inquiry_mode(hdev);
+		hci_setup_inquiry_mode(transaction);
 
 	if (lmp_inq_tx_pwr_capable(hdev))
-		hci_send_cmd(hdev, HCI_OP_READ_INQ_RSP_TX_POWER, 0, NULL);
+		hci_transaction_cmd(transaction, HCI_OP_READ_INQ_RSP_TX_POWER,
+				    0, NULL);
 
 	if (lmp_ext_feat_capable(hdev)) {
 		struct hci_cp_read_local_ext_features cp;
 
 		cp.page = 0x01;
-		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, sizeof(cp),
-			     &cp);
+		hci_transaction_cmd(transaction, HCI_OP_READ_LOCAL_EXT_FEATURES,
+				    sizeof(cp), &cp);
 	}
 
 	if (test_bit(HCI_LINK_SECURITY, &hdev->dev_flags)) {
 		u8 enable = 1;
-		hci_send_cmd(hdev, HCI_OP_WRITE_AUTH_ENABLE, sizeof(enable),
-			     &enable);
+		hci_transaction_cmd(transaction, HCI_OP_WRITE_AUTH_ENABLE,
+				    sizeof(enable), &enable);
 	}
 }
 
-static void hci_setup_link_policy(struct hci_dev *hdev)
+static void hci_setup_link_policy(struct hci_transaction *transaction)
 {
+	struct hci_dev *hdev = transaction->hdev;
 	struct hci_cp_write_def_link_policy cp;
 	u16 link_policy = 0;
 
@@ -476,11 +484,13 @@ static void hci_setup_link_policy(struct hci_dev *hdev)
 		link_policy |= HCI_LP_PARK;
 
 	cp.policy = cpu_to_le16(link_policy);
-	hci_send_cmd(hdev, HCI_OP_WRITE_DEF_LINK_POLICY, sizeof(cp), &cp);
+	hci_transaction_cmd(transaction, HCI_OP_WRITE_DEF_LINK_POLICY,
+			    sizeof(cp), &cp);
 }
 
-static void hci_set_le_support(struct hci_dev *hdev)
+static void hci_set_le_support(struct hci_transaction *transaction)
 {
+	struct hci_dev *hdev = transaction->hdev;
 	struct hci_cp_write_le_host_supported cp;
 
 	memset(&cp, 0, sizeof(cp));
@@ -490,18 +500,23 @@ static void hci_set_le_support(struct hci_dev *hdev)
 		cp.simul = lmp_le_br_capable(hdev);
 	}
 
-	if (cp.le != lmp_host_le_capable(hdev))
-		hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
-			     &cp);
+	if (cp.le == lmp_host_le_capable(hdev))
+		return;
+
+	hci_transaction_cmd(transaction, HCI_OP_WRITE_LE_HOST_SUPPORTED,
+			    sizeof(cp), &cp);
 }
 
-static void hci_init3_req(struct hci_dev *hdev, unsigned long opt)
+static void hci_init3_req(struct hci_transaction *transaction,
+			  unsigned long opt)
 {
+	struct hci_dev *hdev = transaction->hdev;
+
 	if (hdev->commands[5] & 0x10)
-		hci_setup_link_policy(hdev);
+		hci_setup_link_policy(transaction);
 
 	if (lmp_le_capable(hdev))
-		hci_set_le_support(hdev);
+		hci_set_le_support(transaction);
 }
 
 static int __hci_init(struct hci_dev *hdev)
@@ -523,44 +538,50 @@ static int __hci_init(struct hci_dev *hdev)
 	return __hci_request(hdev, hci_init3_req, 0, HCI_INIT_TIMEOUT);
 }
 
-static void hci_scan_req(struct hci_dev *hdev, unsigned long opt)
+static void hci_scan_req(struct hci_transaction *transaction,
+			 unsigned long opt)
 {
 	__u8 scan = opt;
 
-	BT_DBG("%s %x", hdev->name, scan);
+	BT_DBG("%s %x", transaction->hdev->name, scan);
 
 	/* Inquiry and Page scans */
-	hci_send_cmd(hdev, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
+	hci_transaction_cmd(transaction, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
 }
 
-static void hci_auth_req(struct hci_dev *hdev, unsigned long opt)
+static void hci_auth_req(struct hci_transaction *transaction,
+			 unsigned long opt)
 {
 	__u8 auth = opt;
 
-	BT_DBG("%s %x", hdev->name, auth);
+	BT_DBG("%s %x", transaction->hdev->name, auth);
 
 	/* Authentication */
-	hci_send_cmd(hdev, HCI_OP_WRITE_AUTH_ENABLE, 1, &auth);
+	hci_transaction_cmd(transaction, HCI_OP_WRITE_AUTH_ENABLE, 1, &auth);
 }
 
-static void hci_encrypt_req(struct hci_dev *hdev, unsigned long opt)
+static void hci_encrypt_req(struct hci_transaction *transaction,
+			    unsigned long opt)
 {
 	__u8 encrypt = opt;
 
-	BT_DBG("%s %x", hdev->name, encrypt);
+	BT_DBG("%s %x", transaction->hdev->name, encrypt);
 
 	/* Encryption */
-	hci_send_cmd(hdev, HCI_OP_WRITE_ENCRYPT_MODE, 1, &encrypt);
+	hci_transaction_cmd(transaction, HCI_OP_WRITE_ENCRYPT_MODE, 1,
+			    &encrypt);
 }
 
-static void hci_linkpol_req(struct hci_dev *hdev, unsigned long opt)
+static void hci_linkpol_req(struct hci_transaction *transaction,
+			    unsigned long opt)
 {
 	__le16 policy = cpu_to_le16(opt);
 
-	BT_DBG("%s %x", hdev->name, policy);
+	BT_DBG("%s %x", transaction->hdev->name, policy);
 
 	/* Default link policy */
-	hci_send_cmd(hdev, HCI_OP_WRITE_DEF_LINK_POLICY, 2, &policy);
+	hci_transaction_cmd(transaction, HCI_OP_WRITE_DEF_LINK_POLICY,
+			    2, &policy);
 }
 
 /* Get HCI device by index.
@@ -797,9 +818,10 @@ static int inquiry_cache_dump(struct hci_dev *hdev, int num, __u8 *buf)
 	return copied;
 }
 
-static void hci_inq_req(struct hci_dev *hdev, unsigned long opt)
+static void hci_inq_req(struct hci_transaction *transaction, unsigned long opt)
 {
 	struct hci_inquiry_req *ir = (struct hci_inquiry_req *) opt;
+	struct hci_dev *hdev = transaction->hdev;
 	struct hci_cp_inquiry cp;
 
 	BT_DBG("%s", hdev->name);
@@ -811,7 +833,7 @@ static void hci_inq_req(struct hci_dev *hdev, unsigned long opt)
 	memcpy(&cp.lap, &ir->lap, 3);
 	cp.length  = ir->length;
 	cp.num_rsp = ir->num_rsp;
-	hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
+	hci_transaction_cmd(transaction, HCI_OP_INQUIRY, sizeof(cp), &cp);
 }
 
 int hci_inquiry(void __user *arg)
@@ -1851,7 +1873,8 @@ int hci_blacklist_del(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type)
 	return mgmt_device_unblocked(hdev, bdaddr, type);
 }
 
-static void le_scan_param_req(struct hci_dev *hdev, unsigned long opt)
+static void le_scan_param_req(struct hci_transaction *transaction,
+			      unsigned long opt)
 {
 	struct le_scan_params *param =  (struct le_scan_params *) opt;
 	struct hci_cp_le_set_scan_param cp;
@@ -1861,10 +1884,12 @@ static void le_scan_param_req(struct hci_dev *hdev, unsigned long opt)
 	cp.interval = cpu_to_le16(param->interval);
 	cp.window = cpu_to_le16(param->window);
 
-	hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
+	hci_transaction_cmd(transaction, HCI_OP_LE_SET_SCAN_PARAM,
+			    sizeof(cp), &cp);
 }
 
-static void le_scan_enable_req(struct hci_dev *hdev, unsigned long opt)
+static void le_scan_enable_req(struct hci_transaction *transaction,
+			       unsigned long opt)
 {
 	struct hci_cp_le_set_scan_enable cp;
 
@@ -1872,7 +1897,8 @@ static void le_scan_enable_req(struct hci_dev *hdev, unsigned long opt)
 	cp.enable = 1;
 	cp.filter_dup = 1;
 
-	hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+	hci_transaction_cmd(transaction, HCI_OP_LE_SET_SCAN_ENABLE,
+			    sizeof(cp), &cp);
 }
 
 static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
@@ -3227,17 +3253,49 @@ static bool hci_transaction_is_complete(struct hci_dev *hdev)
 	return bt_cb(skb)->transaction.start;
 }
 
+static void hci_resend_last(struct hci_dev *hdev)
+{
+	struct hci_command_hdr *sent;
+	struct sk_buff *skb;
+	u16 opcode;
+
+	if (!hdev->sent_cmd)
+		return;
+
+	sent = (void *) hdev->sent_cmd->data;
+	opcode = __le16_to_cpu(sent->opcode);
+	if (opcode == HCI_OP_RESET)
+		return;
+
+	skb = skb_clone(hdev->sent_cmd, GFP_KERNEL);
+	if (!skb)
+		return;
+
+	skb_queue_head(&hdev->cmd_q, skb);
+	queue_work(hdev->workqueue, &hdev->cmd_work);
+}
+
 void hci_transaction_cmd_complete(struct hci_dev *hdev, u16 opcode, u8 status)
 {
 	struct sk_buff *skb;
 
 	BT_DBG("opcode 0x%04x status 0x%02x", opcode, status);
 
-	/* Check that the completed command really matches the last one
-	 * that was sent.
+	/* If the completed command doesn't match the last one that was
+	 * sent we need to do special handling of it.
 	 */
-	if (!hci_sent_cmd_data(hdev, opcode))
+	if (!hci_sent_cmd_data(hdev, opcode)) {
+		/* Some CSR based controllers generate a spontaneous
+		 * reset complete event during init and any pending
+		 * command will never be completed. In such a case we
+		 * need to resend whatever was the last sent
+		 * command.
+		 */
+		if (test_bit(HCI_INIT, &hdev->flags) && opcode == HCI_OP_RESET)
+			hci_resend_last(hdev);
+
 		return;
+	}
 
 	/* If the command succeeded and there's still more commands in
 	 * this transaction the transaction is not yet complete.
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6dd5fd4..d63efc8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -54,7 +54,6 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_dev_unlock(hdev);
 
 	hci_transaction_cmd_complete(hdev, HCI_OP_INQUIRY, status);
-	hci_req_complete(hdev, HCI_OP_INQUIRY_CANCEL, status);
 
 	hci_conn_check_pending(hdev);
 }
@@ -184,8 +183,6 @@ static void hci_cc_write_def_link_policy(struct hci_dev *hdev,
 
 	if (!status)
 		hdev->link_policy = get_unaligned_le16(sent);
-
-	hci_req_complete(hdev, HCI_OP_WRITE_DEF_LINK_POLICY, status);
 }
 
 static void hci_cc_reset(struct hci_dev *hdev, struct sk_buff *skb)
@@ -196,8 +193,6 @@ static void hci_cc_reset(struct hci_dev *hdev, struct sk_buff *skb)
 
 	clear_bit(HCI_RESET, &hdev->flags);
 
-	hci_req_complete(hdev, HCI_OP_RESET, status);
-
 	/* Reset all non-persistent flags */
 	hdev->dev_flags &= ~(BIT(HCI_LE_SCAN) | BIT(HCI_PENDING_CLASS) |
 			     BIT(HCI_PERIODIC_INQ));
@@ -232,8 +227,6 @@ static void hci_cc_write_local_name(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (!status && !test_bit(HCI_INIT, &hdev->flags))
 		hci_update_ad(hdev);
-
-	hci_req_complete(hdev, HCI_OP_WRITE_LOCAL_NAME, status);
 }
 
 static void hci_cc_read_local_name(struct hci_dev *hdev, struct sk_buff *skb)
@@ -271,8 +264,6 @@ static void hci_cc_write_auth_enable(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (test_bit(HCI_MGMT, &hdev->dev_flags))
 		mgmt_auth_enable_complete(hdev, status);
-
-	hci_req_complete(hdev, HCI_OP_WRITE_AUTH_ENABLE, status);
 }
 
 static void hci_cc_write_encrypt_mode(struct hci_dev *hdev, struct sk_buff *skb)
@@ -294,8 +285,6 @@ static void hci_cc_write_encrypt_mode(struct hci_dev *hdev, struct sk_buff *skb)
 		else
 			clear_bit(HCI_ENCRYPT, &hdev->flags);
 	}
-
-	hci_req_complete(hdev, HCI_OP_WRITE_ENCRYPT_MODE, status);
 }
 
 static void hci_cc_write_scan_enable(struct hci_dev *hdev, struct sk_buff *skb)
@@ -344,7 +333,6 @@ static void hci_cc_write_scan_enable(struct hci_dev *hdev, struct sk_buff *skb)
 
 done:
 	hci_dev_unlock(hdev);
-	hci_req_complete(hdev, HCI_OP_WRITE_SCAN_ENABLE, status);
 }
 
 static void hci_cc_read_class_of_dev(struct hci_dev *hdev, struct sk_buff *skb)
@@ -441,8 +429,6 @@ static void hci_cc_host_buffer_size(struct hci_dev *hdev, struct sk_buff *skb)
 	__u8 status = *((__u8 *) skb->data);
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
-	hci_req_complete(hdev, HCI_OP_HOST_BUFFER_SIZE, status);
 }
 
 static void hci_cc_write_ssp_mode(struct hci_dev *hdev, struct sk_buff *skb)
@@ -480,7 +466,7 @@ static void hci_cc_read_local_version(struct hci_dev *hdev, struct sk_buff *skb)
 	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
 
 	if (rp->status)
-		goto done;
+		return;
 
 	hdev->hci_ver = rp->hci_ver;
 	hdev->hci_rev = __le16_to_cpu(rp->hci_rev);
@@ -490,9 +476,6 @@ static void hci_cc_read_local_version(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s manufacturer 0x%4.4x hci ver %d:%d", hdev->name,
 	       hdev->manufacturer, hdev->hci_ver, hdev->hci_rev);
-
-done:
-	hci_req_complete(hdev, HCI_OP_READ_LOCAL_VERSION, rp->status);
 }
 
 static void hci_cc_read_local_commands(struct hci_dev *hdev,
@@ -504,8 +487,6 @@ static void hci_cc_read_local_commands(struct hci_dev *hdev,
 
 	if (!rp->status)
 		memcpy(hdev->commands, rp->commands, sizeof(hdev->commands));
-
-	hci_req_complete(hdev, HCI_OP_READ_LOCAL_COMMANDS, rp->status);
 }
 
 static void hci_cc_read_local_features(struct hci_dev *hdev,
@@ -572,7 +553,7 @@ static void hci_cc_read_local_ext_features(struct hci_dev *hdev,
 	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
 
 	if (rp->status)
-		goto done;
+		return;
 
 	switch (rp->page) {
 	case 0:
@@ -582,9 +563,6 @@ static void hci_cc_read_local_ext_features(struct hci_dev *hdev,
 		memcpy(hdev->host_features, rp->features, 8);
 		break;
 	}
-
-done:
-	hci_req_complete(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, rp->status);
 }
 
 static void hci_cc_read_flow_control_mode(struct hci_dev *hdev,
@@ -594,12 +572,8 @@ static void hci_cc_read_flow_control_mode(struct hci_dev *hdev,
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
 
-	if (rp->status)
-		return;
-
-	hdev->flow_ctl_mode = rp->mode;
-
-	hci_req_complete(hdev, HCI_OP_READ_FLOW_CONTROL_MODE, rp->status);
+	if (!rp->status)
+		hdev->flow_ctl_mode = rp->mode;
 }
 
 static void hci_cc_read_buffer_size(struct hci_dev *hdev, struct sk_buff *skb)
@@ -636,8 +610,6 @@ static void hci_cc_read_bd_addr(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (!rp->status)
 		bacpy(&hdev->bdaddr, &rp->bdaddr);
-
-	hci_req_complete(hdev, HCI_OP_READ_BD_ADDR, rp->status);
 }
 
 static void hci_cc_read_data_block_size(struct hci_dev *hdev,
@@ -658,8 +630,6 @@ static void hci_cc_read_data_block_size(struct hci_dev *hdev,
 
 	BT_DBG("%s blk mtu %d cnt %d len %d", hdev->name, hdev->block_mtu,
 	       hdev->block_cnt, hdev->block_len);
-
-	hci_req_complete(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, rp->status);
 }
 
 static void hci_cc_write_ca_timeout(struct hci_dev *hdev, struct sk_buff *skb)
@@ -667,8 +637,6 @@ static void hci_cc_write_ca_timeout(struct hci_dev *hdev, struct sk_buff *skb)
 	__u8 status = *((__u8 *) skb->data);
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
-	hci_req_complete(hdev, HCI_OP_WRITE_CA_TIMEOUT, status);
 }
 
 static void hci_cc_read_local_amp_info(struct hci_dev *hdev,
@@ -692,8 +660,6 @@ static void hci_cc_read_local_amp_info(struct hci_dev *hdev,
 	hdev->amp_be_flush_to = __le32_to_cpu(rp->be_flush_to);
 	hdev->amp_max_flush_to = __le32_to_cpu(rp->max_flush_to);
 
-	hci_req_complete(hdev, HCI_OP_READ_LOCAL_AMP_INFO, rp->status);
-
 a2mp_rsp:
 	a2mp_send_getinfo_rsp(hdev);
 }
@@ -741,8 +707,6 @@ static void hci_cc_delete_stored_link_key(struct hci_dev *hdev,
 	__u8 status = *((__u8 *) skb->data);
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
-	hci_req_complete(hdev, HCI_OP_DELETE_STORED_LINK_KEY, status);
 }
 
 static void hci_cc_set_event_mask(struct hci_dev *hdev, struct sk_buff *skb)
@@ -750,8 +714,6 @@ static void hci_cc_set_event_mask(struct hci_dev *hdev, struct sk_buff *skb)
 	__u8 status = *((__u8 *) skb->data);
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
-	hci_req_complete(hdev, HCI_OP_SET_EVENT_MASK, status);
 }
 
 static void hci_cc_write_inquiry_mode(struct hci_dev *hdev,
@@ -760,8 +722,6 @@ static void hci_cc_write_inquiry_mode(struct hci_dev *hdev,
 	__u8 status = *((__u8 *) skb->data);
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
-	hci_req_complete(hdev, HCI_OP_WRITE_INQUIRY_MODE, status);
 }
 
 static void hci_cc_read_inq_rsp_tx_power(struct hci_dev *hdev,
@@ -773,8 +733,6 @@ static void hci_cc_read_inq_rsp_tx_power(struct hci_dev *hdev,
 
 	if (!rp->status)
 		hdev->inq_tx_power = rp->tx_power;
-
-	hci_req_complete(hdev, HCI_OP_READ_INQ_RSP_TX_POWER, rp->status);
 }
 
 static void hci_cc_set_event_flt(struct hci_dev *hdev, struct sk_buff *skb)
@@ -782,8 +740,6 @@ static void hci_cc_set_event_flt(struct hci_dev *hdev, struct sk_buff *skb)
 	__u8 status = *((__u8 *) skb->data);
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
-	hci_req_complete(hdev, HCI_OP_SET_EVENT_FLT, status);
 }
 
 static void hci_cc_pin_code_reply(struct hci_dev *hdev, struct sk_buff *skb)
@@ -845,8 +801,6 @@ static void hci_cc_le_read_buffer_size(struct hci_dev *hdev,
 	hdev->le_cnt = hdev->le_pkts;
 
 	BT_DBG("%s le mtu %d:%d", hdev->name, hdev->le_mtu, hdev->le_pkts);
-
-	hci_req_complete(hdev, HCI_OP_LE_READ_BUFFER_SIZE, rp->status);
 }
 
 static void hci_cc_le_read_local_features(struct hci_dev *hdev,
@@ -858,8 +812,6 @@ static void hci_cc_le_read_local_features(struct hci_dev *hdev,
 
 	if (!rp->status)
 		memcpy(hdev->le_features, rp->features, 8);
-
-	hci_req_complete(hdev, HCI_OP_LE_READ_LOCAL_FEATURES, rp->status);
 }
 
 static void hci_cc_le_read_adv_tx_power(struct hci_dev *hdev,
@@ -874,8 +826,6 @@ static void hci_cc_le_read_adv_tx_power(struct hci_dev *hdev,
 		if (!test_bit(HCI_INIT, &hdev->flags))
 			hci_update_ad(hdev);
 	}
-
-	hci_req_complete(hdev, HCI_OP_LE_READ_ADV_TX_POWER, rp->status);
 }
 
 static void hci_cc_le_set_event_mask(struct hci_dev *hdev, struct sk_buff *skb)
@@ -883,8 +833,6 @@ static void hci_cc_le_set_event_mask(struct hci_dev *hdev, struct sk_buff *skb)
 	__u8 status = *((__u8 *) skb->data);
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
-	hci_req_complete(hdev, HCI_OP_LE_SET_EVENT_MASK, status);
 }
 
 static void hci_cc_user_confirm_reply(struct hci_dev *hdev, struct sk_buff *skb)
@@ -985,8 +933,6 @@ static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (!test_bit(HCI_INIT, &hdev->flags))
 		hci_update_ad(hdev);
-
-	hci_req_complete(hdev, HCI_OP_LE_SET_ADV_ENABLE, status);
 }
 
 static void hci_cc_le_set_scan_param(struct hci_dev *hdev, struct sk_buff *skb)
@@ -995,8 +941,6 @@ static void hci_cc_le_set_scan_param(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
 
-	hci_req_complete(hdev, HCI_OP_LE_SET_SCAN_PARAM, status);
-
 	if (status) {
 		hci_dev_lock(hdev);
 		mgmt_start_discovery_failed(hdev, status);
@@ -1019,8 +963,6 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 
 	switch (cp->enable) {
 	case LE_SCANNING_ENABLED:
-		hci_req_complete(hdev, HCI_OP_LE_SET_SCAN_ENABLE, status);
-
 		if (status) {
 			hci_dev_lock(hdev);
 			mgmt_start_discovery_failed(hdev, status);
@@ -1071,8 +1013,6 @@ static void hci_cc_le_read_white_list_size(struct hci_dev *hdev,
 
 	if (!rp->status)
 		hdev->le_white_list_size = rp->size;
-
-	hci_req_complete(hdev, HCI_OP_LE_READ_WHITE_LIST_SIZE, rp->status);
 }
 
 static void hci_cc_le_ltk_reply(struct hci_dev *hdev, struct sk_buff *skb)
@@ -1083,8 +1023,6 @@ static void hci_cc_le_ltk_reply(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (rp->status)
 		return;
-
-	hci_req_complete(hdev, HCI_OP_LE_LTK_REPLY, rp->status);
 }
 
 static void hci_cc_le_ltk_neg_reply(struct hci_dev *hdev, struct sk_buff *skb)
@@ -1095,8 +1033,6 @@ static void hci_cc_le_ltk_neg_reply(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (rp->status)
 		return;
-
-	hci_req_complete(hdev, HCI_OP_LE_LTK_NEG_REPLY, rp->status);
 }
 
 static void hci_cc_le_read_supported_states(struct hci_dev *hdev,
@@ -1108,8 +1044,6 @@ static void hci_cc_le_read_supported_states(struct hci_dev *hdev,
 
 	if (!rp->status)
 		memcpy(hdev->le_states, rp->le_states, 8);
-
-	hci_req_complete(hdev, HCI_OP_LE_READ_SUPPORTED_STATES, rp->status);
 }
 
 static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
@@ -1139,8 +1073,6 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
 	if (test_bit(HCI_MGMT, &hdev->dev_flags) &&
 	    !test_bit(HCI_INIT, &hdev->flags))
 		mgmt_le_enable_complete(hdev, sent->le, status);
-
-	hci_req_complete(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, status);
 }
 
 static void hci_cc_write_remote_amp_assoc(struct hci_dev *hdev,
@@ -1162,7 +1094,6 @@ static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
 
 	if (status) {
-		hci_req_complete(hdev, HCI_OP_INQUIRY, status);
 		hci_conn_check_pending(hdev);
 		hci_dev_lock(hdev);
 		if (test_bit(HCI_MGMT, &hdev->dev_flags))
@@ -1694,7 +1625,6 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	BT_DBG("%s status 0x%2.2x", hdev->name, status);
 
 	hci_transaction_cmd_complete(hdev, HCI_OP_INQUIRY, status);
-	hci_req_complete(hdev, HCI_OP_INQUIRY, status);
 
 	hci_conn_check_pending(hdev);
 
-- 
1.7.10.4

[PATCH v3 09/16] Bluetooth: Remove unused hdev->init_last_cmd

[Johan Hedberg]

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

This variable is no longer needed (due to the transaction framweork and
the conversion of hci_request to use it), so it can be safely removed.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |    2 --
 net/bluetooth/hci_core.c         |    6 ------
 2 files changed, 8 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 805551b..c9827f9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -250,8 +250,6 @@ struct hci_dev {
 
 	transaction_complete_t	transaction_complete;
 
-	__u16			init_last_cmd;
-
 	struct list_head	mgmt_pending;
 
 	struct discovery_state	discovery;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 2657a9e..92b6444 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1042,10 +1042,7 @@ int hci_dev_open(__u16 dev)
 	if (!test_bit(HCI_RAW, &hdev->flags)) {
 		atomic_set(&hdev->cmd_cnt, 1);
 		set_bit(HCI_INIT, &hdev->flags);
-		hdev->init_last_cmd = 0;
-
 		ret = __hci_init(hdev);
-
 		clear_bit(HCI_INIT, &hdev->flags);
 	}
 
@@ -2547,9 +2544,6 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
 		return -ENOMEM;
 	}
 
-	if (test_bit(HCI_INIT, &hdev->flags))
-		hdev->init_last_cmd = opcode;
-
 	hci_transaction_from_skb(hdev, skb);
 
 	return 0;
-- 
1.7.10.4

[PATCH v3 10/16] Bluetooth: Move power on HCI command updates to their own function

[Johan Hedberg]

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

These commands will in a subsequent patch be performed in their own
transaction, so it's more readable (not just from a resulting code
perspective but also the way the patches look like) to have them
performed in their own function.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 39395c7..025f6f0 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3058,53 +3058,57 @@ static int set_bredr_scan(struct hci_dev *hdev)
 	return hci_send_cmd(hdev, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
 }
 
-int mgmt_powered(struct hci_dev *hdev, u8 powered)
+static int powered_update_hci(struct hci_dev *hdev)
 {
-	struct cmd_lookup match = { NULL, hdev };
-	int err;
+	u8 link_sec;
 
-	if (!test_bit(HCI_MGMT, &hdev->dev_flags))
-		return 0;
+	if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags) &&
+	    !lmp_host_ssp_capable(hdev)) {
+		u8 ssp = 1;
 
-	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
+		hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE, 1, &ssp);
+	}
 
-	if (powered) {
-		u8 link_sec;
+	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
+		struct hci_cp_write_le_host_supported cp;
 
-		if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags) &&
-		    !lmp_host_ssp_capable(hdev)) {
-			u8 ssp = 1;
+		cp.le = 1;
+		cp.simul = lmp_le_br_capable(hdev);
 
-			hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE, 1, &ssp);
-		}
+		/* Check first if we already have the right
+		 * host state (host features set)
+		 */
+		if (cp.le != lmp_host_le_capable(hdev) ||
+		    cp.simul != lmp_host_le_br_capable(hdev))
+			hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED,
+				     sizeof(cp), &cp);
+	}
 
-		if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
-			struct hci_cp_write_le_host_supported cp;
+	link_sec = test_bit(HCI_LINK_SECURITY, &hdev->dev_flags);
+	if (link_sec != test_bit(HCI_AUTH, &hdev->flags))
+		hci_send_cmd(hdev, HCI_OP_WRITE_AUTH_ENABLE,
+			     sizeof(link_sec), &link_sec);
 
-			cp.le = 1;
-			cp.simul = lmp_le_br_capable(hdev);
+	if (lmp_bredr_capable(hdev)) {
+		set_bredr_scan(hdev);
+		update_class(hdev);
+		update_name(hdev, hdev->dev_name);
+		update_eir(hdev);
+	}
+}
 
-			/* Check first if we already have the right
-			 * host state (host features set)
-			 */
-			if (cp.le != lmp_host_le_capable(hdev) ||
-			    cp.simul != lmp_host_le_br_capable(hdev))
-				hci_send_cmd(hdev,
-					     HCI_OP_WRITE_LE_HOST_SUPPORTED,
-					     sizeof(cp), &cp);
-		}
+int mgmt_powered(struct hci_dev *hdev, u8 powered)
+{
+	struct cmd_lookup match = { NULL, hdev };
+	int err;
 
-		link_sec = test_bit(HCI_LINK_SECURITY, &hdev->dev_flags);
-		if (link_sec != test_bit(HCI_AUTH, &hdev->flags))
-			hci_send_cmd(hdev, HCI_OP_WRITE_AUTH_ENABLE,
-				     sizeof(link_sec), &link_sec);
+	if (!test_bit(HCI_MGMT, &hdev->dev_flags))
+		return 0;
 
-		if (lmp_bredr_capable(hdev)) {
-			set_bredr_scan(hdev);
-			update_class(hdev);
-			update_name(hdev, hdev->dev_name);
-			update_eir(hdev);
-		}
+	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
+
+	if (powered) {
+		powered_update_hci(hdev);
 	} else {
 		u8 status = MGMT_STATUS_NOT_POWERED;
 		u8 zero_cod[] = { 0, 0, 0 };
-- 
1.7.10.4

[PATCH v3 11/16] Bluetooth: Update mgmt powered HCI commands to use transactions

[Johan Hedberg]

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

This patch updates sending of HCI commands related to mgmt_set_powered
(e.g. class, name and EIR data) to be sent using transactions. This is
necessary since it's the only (well, at least the cleanest) way to keep
the power on procedure synchronized and let user space know it has
completed only when all HCI commands are completed (this actual fix is
coming in a subsequent patch).

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 025f6f0..e76581d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -591,8 +591,9 @@ static void create_eir(struct hci_dev *hdev, u8 *data)
 	ptr = create_uuid128_list(hdev, ptr, HCI_MAX_EIR_LENGTH - (ptr - data));
 }
 
-static int update_eir(struct hci_dev *hdev)
+static int update_eir(struct hci_transaction *transaction)
 {
+	struct hci_dev *hdev = transaction->hdev;
 	struct hci_cp_write_eir cp;
 
 	if (!hdev_is_powered(hdev))
@@ -616,7 +617,8 @@ static int update_eir(struct hci_dev *hdev)
 
 	memcpy(hdev->eir, cp.data, sizeof(cp.data));
 
-	return hci_send_cmd(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp);
+	return hci_transaction_cmd(transaction, HCI_OP_WRITE_EIR,
+				   sizeof(cp), &cp);
 }
 
 static u8 get_service_classes(struct hci_dev *hdev)
@@ -630,8 +632,9 @@ static u8 get_service_classes(struct hci_dev *hdev)
 	return val;
 }
 
-static int update_class(struct hci_dev *hdev)
+static int update_class(struct hci_transaction *transaction)
 {
+	struct hci_dev *hdev = transaction->hdev;
 	u8 cod[3];
 	int err;
 
@@ -650,7 +653,8 @@ static int update_class(struct hci_dev *hdev)
 	if (memcmp(cod, hdev->dev_class, 3) == 0)
 		return 0;
 
-	err = hci_send_cmd(hdev, HCI_OP_WRITE_CLASS_OF_DEV, sizeof(cod), cod);
+	err = hci_transaction_cmd(transaction, HCI_OP_WRITE_CLASS_OF_DEV,
+				  sizeof(cod), cod);
 	if (err == 0)
 		set_bit(HCI_PENDING_CLASS, &hdev->dev_flags);
 
@@ -661,16 +665,21 @@ static void service_cache_off(struct work_struct *work)
 {
 	struct hci_dev *hdev = container_of(work, struct hci_dev,
 					    service_cache.work);
+	struct hci_transaction transaction;
 
 	if (!test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->dev_flags))
 		return;
 
+	hci_transaction_init(&transaction, hdev, NULL);
+
 	hci_dev_lock(hdev);
 
-	update_eir(hdev);
-	update_class(hdev);
+	update_eir(&transaction);
+	update_class(&transaction);
 
 	hci_dev_unlock(hdev);
+
+	hci_transaction_run(&transaction);
 }
 
 static void mgmt_init_hdev(struct sock *sk, struct hci_dev *hdev)
@@ -1354,6 +1363,7 @@ static u8 get_uuid_size(const u8 *uuid)
 static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 {
 	struct mgmt_cp_add_uuid *cp = data;
+	struct hci_transaction transaction;
 	struct pending_cmd *cmd;
 	struct bt_uuid *uuid;
 	int err;
@@ -1380,13 +1390,12 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 
 	list_add_tail(&uuid->list, &hdev->uuids);
 
-	err = update_class(hdev);
-	if (err < 0)
-		goto failed;
+	hci_transaction_init(&transaction, hdev, NULL);
 
-	err = update_eir(hdev);
-	if (err < 0)
-		goto failed;
+	update_class(&transaction);
+	update_eir(&transaction);
+
+	hci_transaction_run(&transaction);
 
 	if (!test_bit(HCI_PENDING_CLASS, &hdev->dev_flags)) {
 		err = cmd_complete(sk, hdev->id, MGMT_OP_ADD_UUID, 0,
@@ -1395,8 +1404,12 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 	}
 
 	cmd = mgmt_pending_add(sk, MGMT_OP_ADD_UUID, hdev, data, len);
-	if (!cmd)
+	if (!cmd) {
 		err = -ENOMEM;
+		goto failed;
+	}
+
+	err = 0;
 
 failed:
 	hci_dev_unlock(hdev);
@@ -1421,6 +1434,7 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data,
 		       u16 len)
 {
 	struct mgmt_cp_remove_uuid *cp = data;
+	struct hci_transaction transaction;
 	struct pending_cmd *cmd;
 	struct bt_uuid *match, *tmp;
 	u8 bt_uuid_any[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
@@ -1466,13 +1480,12 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data,
 	}
 
 update_class:
-	err = update_class(hdev);
-	if (err < 0)
-		goto unlock;
+	hci_transaction_init(&transaction, hdev, NULL);
 
-	err = update_eir(hdev);
-	if (err < 0)
-		goto unlock;
+	update_class(&transaction);
+	update_eir(&transaction);
+
+	hci_transaction_run(&transaction);
 
 	if (!test_bit(HCI_PENDING_CLASS, &hdev->dev_flags)) {
 		err = cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_UUID, 0,
@@ -1481,8 +1494,12 @@ update_class:
 	}
 
 	cmd = mgmt_pending_add(sk, MGMT_OP_REMOVE_UUID, hdev, data, len);
-	if (!cmd)
+	if (!cmd) {
 		err = -ENOMEM;
+		goto unlock;
+	}
+
+	err = 0;
 
 unlock:
 	hci_dev_unlock(hdev);
@@ -1493,6 +1510,7 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 			 u16 len)
 {
 	struct mgmt_cp_set_dev_class *cp = data;
+	struct hci_transaction transaction;
 	struct pending_cmd *cmd;
 	int err;
 
@@ -1521,16 +1539,18 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
+	hci_transaction_init(&transaction, hdev, NULL);
+
 	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->dev_flags)) {
 		hci_dev_unlock(hdev);
 		cancel_delayed_work_sync(&hdev->service_cache);
 		hci_dev_lock(hdev);
-		update_eir(hdev);
+		update_eir(&transaction);
 	}
 
-	err = update_class(hdev);
-	if (err < 0)
-		goto unlock;
+	update_class(&transaction);
+
+	hci_transaction_run(&transaction);
 
 	if (!test_bit(HCI_PENDING_CLASS, &hdev->dev_flags)) {
 		err = cmd_complete(sk, hdev->id, MGMT_OP_SET_DEV_CLASS, 0,
@@ -1539,8 +1559,12 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 	}
 
 	cmd = mgmt_pending_add(sk, MGMT_OP_SET_DEV_CLASS, hdev, data, len);
-	if (!cmd)
+	if (!cmd) {
 		err = -ENOMEM;
+		goto unlock;
+	}
+
+	err = 0;
 
 unlock:
 	hci_dev_unlock(hdev);
@@ -2268,19 +2292,21 @@ static int user_passkey_neg_reply(struct sock *sk, struct hci_dev *hdev,
 				 HCI_OP_USER_PASSKEY_NEG_REPLY, 0);
 }
 
-static int update_name(struct hci_dev *hdev, const char *name)
+static void update_name(struct hci_transaction *transaction, const char *name)
 {
 	struct hci_cp_write_local_name cp;
 
 	memcpy(cp.name, name, sizeof(cp.name));
 
-	return hci_send_cmd(hdev, HCI_OP_WRITE_LOCAL_NAME, sizeof(cp), &cp);
+	hci_transaction_cmd(transaction, HCI_OP_WRITE_LOCAL_NAME,
+			    sizeof(cp), &cp);
 }
 
 static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
 			  u16 len)
 {
 	struct mgmt_cp_set_local_name *cp = data;
+	struct hci_transaction transaction;
 	struct pending_cmd *cmd;
 	int err;
 
@@ -2310,7 +2336,9 @@ static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto failed;
 	}
 
-	err = update_name(hdev, cp->name);
+	hci_transaction_init(&transaction, hdev, NULL);
+	update_name(&transaction, cp->name);
+	err = hci_transaction_run(&transaction);
 	if (err < 0)
 		mgmt_pending_remove(cmd);
 
@@ -2698,6 +2726,7 @@ static int set_device_id(struct sock *sk, struct hci_dev *hdev, void *data,
 			 u16 len)
 {
 	struct mgmt_cp_set_device_id *cp = data;
+	struct hci_transaction transaction;
 	int err;
 	__u16 source;
 
@@ -2718,7 +2747,9 @@ static int set_device_id(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	err = cmd_complete(sk, hdev->id, MGMT_OP_SET_DEVICE_ID, 0, NULL, 0);
 
-	update_eir(hdev);
+	hci_transaction_init(&transaction, hdev, NULL);
+	update_eir(&transaction);
+	hci_transaction_run(&transaction);
 
 	hci_dev_unlock(hdev);
 
@@ -3043,8 +3074,9 @@ static void settings_rsp(struct pending_cmd *cmd, void *data)
 	mgmt_pending_free(cmd);
 }
 
-static int set_bredr_scan(struct hci_dev *hdev)
+static int set_bredr_scan(struct hci_transaction *transaction)
 {
+	struct hci_dev *hdev = transaction->hdev;
 	u8 scan = 0;
 
 	if (test_bit(HCI_CONNECTABLE, &hdev->dev_flags))
@@ -3055,18 +3087,23 @@ static int set_bredr_scan(struct hci_dev *hdev)
 	if (!scan)
 		return 0;
 
-	return hci_send_cmd(hdev, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
+	return hci_transaction_cmd(transaction, HCI_OP_WRITE_SCAN_ENABLE,
+				   1, &scan);
 }
 
 static int powered_update_hci(struct hci_dev *hdev)
 {
+	struct hci_transaction transaction;
 	u8 link_sec;
 
+	hci_transaction_init(&transaction, hdev, NULL);
+
 	if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags) &&
-	    !lmp_host_ssp_capable(hdev)) {
+			!lmp_host_ssp_capable(hdev)) {
 		u8 ssp = 1;
 
-		hci_send_cmd(hdev, HCI_OP_WRITE_SSP_MODE, 1, &ssp);
+		hci_transaction_cmd(&transaction, HCI_OP_WRITE_SSP_MODE,
+				    1, &ssp);
 	}
 
 	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
@@ -3079,22 +3116,25 @@ static int powered_update_hci(struct hci_dev *hdev)
 		 * host state (host features set)
 		 */
 		if (cp.le != lmp_host_le_capable(hdev) ||
-		    cp.simul != lmp_host_le_br_capable(hdev))
-			hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED,
-				     sizeof(cp), &cp);
+				cp.simul != lmp_host_le_br_capable(hdev))
+			hci_transaction_cmd(&transaction,
+					    HCI_OP_WRITE_LE_HOST_SUPPORTED,
+					    sizeof(cp), &cp);
 	}
 
 	link_sec = test_bit(HCI_LINK_SECURITY, &hdev->dev_flags);
 	if (link_sec != test_bit(HCI_AUTH, &hdev->flags))
-		hci_send_cmd(hdev, HCI_OP_WRITE_AUTH_ENABLE,
-			     sizeof(link_sec), &link_sec);
+		hci_transaction_cmd(&transaction, HCI_OP_WRITE_AUTH_ENABLE,
+				    sizeof(link_sec), &link_sec);
 
 	if (lmp_bredr_capable(hdev)) {
-		set_bredr_scan(hdev);
-		update_class(hdev);
-		update_name(hdev, hdev->dev_name);
-		update_eir(hdev);
+		set_bredr_scan(&transaction);
+		update_class(&transaction);
+		update_name(&transaction, hdev->dev_name);
+		update_eir(&transaction);
 	}
+
+	return hci_transaction_run(&transaction);
 }
 
 int mgmt_powered(struct hci_dev *hdev, u8 powered)
@@ -3559,8 +3599,9 @@ int mgmt_auth_enable_complete(struct hci_dev *hdev, u8 status)
 	return err;
 }
 
-static int clear_eir(struct hci_dev *hdev)
+static int clear_eir(struct hci_transaction *transaction)
 {
+	struct hci_dev *hdev = transaction->hdev;
 	struct hci_cp_write_eir cp;
 
 	if (!lmp_ext_inq_capable(hdev))
@@ -3570,12 +3611,14 @@ static int clear_eir(struct hci_dev *hdev)
 
 	memset(&cp, 0, sizeof(cp));
 
-	return hci_send_cmd(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp);
+	return hci_transaction_cmd(transaction, HCI_OP_WRITE_EIR,
+				   sizeof(cp), &cp);
 }
 
 int mgmt_ssp_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
 {
 	struct cmd_lookup match = { NULL, hdev };
+	struct hci_transaction transaction;
 	bool changed = false;
 	int err = 0;
 
@@ -3608,10 +3651,14 @@ int mgmt_ssp_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
 	if (match.sk)
 		sock_put(match.sk);
 
+	hci_transaction_init(&transaction, hdev, NULL);
+
 	if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags))
-		update_eir(hdev);
+		update_eir(&transaction);
 	else
-		clear_eir(hdev);
+		clear_eir(&transaction);
+
+	hci_transaction_run(&transaction);
 
 	return err;
 }
@@ -3699,8 +3746,12 @@ send_event:
 	 * adapter so only update them here if this is a name change
 	 * unrelated to power on.
 	 */
-	if (!test_bit(HCI_INIT, &hdev->flags))
-		update_eir(hdev);
+	if (!test_bit(HCI_INIT, &hdev->flags)) {
+		struct hci_transaction transaction;
+		hci_transaction_init(&transaction, hdev, NULL);
+		update_eir(&transaction);
+		hci_transaction_run(&transaction);
+	}
 
 failed:
 	if (cmd)
-- 
1.7.10.4

[PATCH v3 12/16] Bluetooth: Wait for HCI command completion with mgmt_set_powered

[Johan Hedberg]

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

We should only notify user space that the adapter has been powered on
after all HCI commands related to the action have completed. This patch
fixes the issue by instating a transaction complete callback for these
HCI commands and only notifies user space in the callback.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e76581d..b2a9697 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3091,12 +3091,26 @@ static int set_bredr_scan(struct hci_transaction *transaction)
 				   1, &scan);
 }
 
+static void powered_complete(struct hci_dev *hdev, u16 opcode, int status)
+{
+	struct cmd_lookup match = { NULL, hdev };
+
+	BT_DBG("opcode 0x%04x status %d", opcode, status);
+
+	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
+
+	new_settings(hdev, match.sk);
+
+	if (match.sk)
+		sock_put(match.sk);
+}
+
 static int powered_update_hci(struct hci_dev *hdev)
 {
 	struct hci_transaction transaction;
 	u8 link_sec;
 
-	hci_transaction_init(&transaction, hdev, NULL);
+	hci_transaction_init(&transaction, hdev, powered_complete);
 
 	if (test_bit(HCI_SSP_ENABLED, &hdev->dev_flags) &&
 			!lmp_host_ssp_capable(hdev)) {
@@ -3140,26 +3154,30 @@ static int powered_update_hci(struct hci_dev *hdev)
 int mgmt_powered(struct hci_dev *hdev, u8 powered)
 {
 	struct cmd_lookup match = { NULL, hdev };
+	u8 status_not_powered = MGMT_STATUS_NOT_POWERED;
+	u8 zero_cod[] = { 0, 0, 0 };
 	int err;
 
 	if (!test_bit(HCI_MGMT, &hdev->dev_flags))
 		return 0;
 
-	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
-
 	if (powered) {
-		powered_update_hci(hdev);
-	} else {
-		u8 status = MGMT_STATUS_NOT_POWERED;
-		u8 zero_cod[] = { 0, 0, 0 };
+		if (powered_update_hci(hdev) == 0)
+			return 0;
 
-		mgmt_pending_foreach(0, hdev, cmd_status_rsp, &status);
-
-		if (memcmp(hdev->dev_class, zero_cod, sizeof(zero_cod)) != 0)
-			mgmt_event(MGMT_EV_CLASS_OF_DEV_CHANGED, hdev,
-				   zero_cod, sizeof(zero_cod), NULL);
+		mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp,
+				     &match);
+		goto new_settings;
 	}
 
+	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
+	mgmt_pending_foreach(0, hdev, cmd_status_rsp, &status_not_powered);
+
+	if (memcmp(hdev->dev_class, zero_cod, sizeof(zero_cod)) != 0)
+		mgmt_event(MGMT_EV_CLASS_OF_DEV_CHANGED, hdev,
+			   zero_cod, sizeof(zero_cod), NULL);
+
+new_settings:
 	err = new_settings(hdev, match.sk);
 
 	if (match.sk)
-- 
1.7.10.4

[PATCH v3 13/16] Bluetooth: Fix busy condition testing for EIR and class updates

[Johan Hedberg]

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

The add/remove_uuid and set_dev_class mgmt commands can trigger both EIR
and class HCI commands, so testing just for a pending class command is
enough. The simplest way to monitor conflicts that should trigger "busy"
error returns is to check for any pending mgmt command that can trigger
these HCI commands. This patch adds a helper function for this
(pending_eir_or_class) and uses it instead of the old HCI_PENDING_CLASS
flag to test for busy conditions.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index b2a9697..41a3265 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1341,6 +1341,23 @@ unlock:
 	return err;
 }
 
+static bool pending_eir_or_class(struct hci_dev *hdev)
+{
+	struct pending_cmd *cmd;
+
+	list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
+		switch (cmd->opcode) {
+		case MGMT_OP_ADD_UUID:
+		case MGMT_OP_REMOVE_UUID:
+		case MGMT_OP_SET_DEV_CLASS:
+		case MGMT_OP_SET_POWERED:
+			return true;
+		}
+	}
+
+	return false;
+}
+
 static const u8 bluetooth_base_uuid[] = {
 			0xfb, 0x34, 0x9b, 0x5f, 0x80, 0x00, 0x00, 0x80,
 			0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -1372,7 +1389,7 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 
 	hci_dev_lock(hdev);
 
-	if (test_bit(HCI_PENDING_CLASS, &hdev->dev_flags)) {
+	if (pending_eir_or_class(hdev)) {
 		err = cmd_status(sk, hdev->id, MGMT_OP_ADD_UUID,
 				 MGMT_STATUS_BUSY);
 		goto failed;
@@ -1444,7 +1461,7 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	hci_dev_lock(hdev);
 
-	if (test_bit(HCI_PENDING_CLASS, &hdev->dev_flags)) {
+	if (pending_eir_or_class(hdev)) {
 		err = cmd_status(sk, hdev->id, MGMT_OP_REMOVE_UUID,
 				 MGMT_STATUS_BUSY);
 		goto unlock;
@@ -1520,15 +1537,19 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 		return cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
 				  MGMT_STATUS_NOT_SUPPORTED);
 
-	if (test_bit(HCI_PENDING_CLASS, &hdev->dev_flags))
-		return cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
-				  MGMT_STATUS_BUSY);
+	hci_dev_lock(hdev);
 
-	if ((cp->minor & 0x03) != 0 || (cp->major & 0xe0) != 0)
-		return cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
-				  MGMT_STATUS_INVALID_PARAMS);
+	if (pending_eir_or_class(hdev)) {
+		err = cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
+				 MGMT_STATUS_BUSY);
+		goto unlock;
+	}
 
-	hci_dev_lock(hdev);
+	if ((cp->minor & 0x03) != 0 || (cp->major & 0xe0) != 0) {
+		err = cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
+				 MGMT_STATUS_INVALID_PARAMS);
+		goto unlock;
+	}
 
 	hdev->major_class = cp->major;
 	hdev->minor_class = cp->minor;
-- 
1.7.10.4

Re: [PATCH v3 13/16] Bluetooth: Fix busy condition testing for EIR and class updates

[Marcel Holtmann]

Hi Johan,


> The add/remove_uuid and set_dev_class mgmt commands can trigger both EIR
> and class HCI commands, so testing just for a pending class command is
> enough. The simplest way to monitor conflicts that should trigger "busy"
> error returns is to check for any pending mgmt command that can trigger
> these HCI commands. This patch adds a helper function for this
> (pending_eir_or_class) and uses it instead of the old HCI_PENDING_CLASS
> flag to test for busy conditions.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c |   39 ++++++++++++++++++++++++++++++---------
> 1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index b2a9697..41a3265 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1341,6 +1341,23 @@ unlock:
> 	return err;
> }
> 
> +static bool pending_eir_or_class(struct hci_dev *hdev)
> +{
> +	struct pending_cmd *cmd;
> +
> +	list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
> +		switch (cmd->opcode) {
> +		case MGMT_OP_ADD_UUID:
> +		case MGMT_OP_REMOVE_UUID:
> +		case MGMT_OP_SET_DEV_CLASS:
> +		case MGMT_OP_SET_POWERED:
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +

I do not like this at all. Why would we do it like this?

The transaction framework should allow us to just process these one at a time. So even if userspace send 20 add_uuid commands at the same time, we would deal with them one after the other.

What I am trying to understand what is the benefit of returning busy here.

Regards

Marcel

Re: [PATCH v3 13/16] Bluetooth: Fix busy condition testing for EIR and class updates

[Johan Hedberg]

Hi Marcel,

On Thu, Feb 28, 2013, Marcel Holtmann wrote:
> > +static bool pending_eir_or_class(struct hci_dev *hdev)
> > +{
> > +	struct pending_cmd *cmd;
> > +
> > +	list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
> > +		switch (cmd->opcode) {
> > +		case MGMT_OP_ADD_UUID:
> > +		case MGMT_OP_REMOVE_UUID:
> > +		case MGMT_OP_SET_DEV_CLASS:
> > +		case MGMT_OP_SET_POWERED:
> > +			return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> 
> I do not like this at all. Why would we do it like this?
> 
> The transaction framework should allow us to just process these one at
> a time. So even if userspace send 20 add_uuid commands at the same
> time, we would deal with them one after the other.
> 
> What I am trying to understand what is the benefit of returning busy
> here.

If we have multiple commands/transactions in the queue wanting to send
the same HCI commands the problem is that the content of each HCI
command (the CoD or EIR data) should depend on the content of the
preceding HCI commands. The current EIR data or CoD (as stored in hdev
variables) depends on the last completed write HCI command for the data.
When we decide whether another command needs to be sent we compare what
we want to send based on what the current value is and not what it will
be (based on what's already in the queue).

Another problem is the handling of HCI command complete events for the
same HCI commands. We don't know which pending mgmt command should be
indicated as completed since there's no tight coupling of HCI commands
and the mgmt commands that triggered them.

Since we strictly speaking don't need to send all those write_eir or
write_cod commands in the queue but just the very last one (since that's
what ultimately takes effect once everything is complete) we could in
principle go digging into the queue and remove any earlier duplicates of
the same HCI command and reply mgmt_cmd_complete for all pending
commands when this one single HCI command completes. This is a bit messy
for several reasons. One is that we still need to ensure that the
complete callbacks for the transactions/commands we removed still get
called. Another is that this will result in some quite interesting
behavior:

	1. Call add_uuid
	2. Call remove_uuid with the same uuid as in 1.
	3. The HCI command from 2. completes (the one from 1. was
	replaced).
	4. Success is returned for both mgmt commands when in fact the
	UUID added in 1 never got added in reality.

Considering the fact that we queue up all commands anyway I don't really
have such a strong opinion on whether we allow potentially messed up
behavior when such queuing doesn't happen, but the above is at least an
attempt at explaining why I added this busy check.

One possible solution I can think of is that we update the CoD or EIR
value in hdev as soon as we create a HCI command to update the value, as
opposed to updating the hdev value when the HCI command completes. This
would allow us to have sensible checks on whether new HCI commands need
to be queued or not. We'd then have to have some flag to indicate that
there are pending EIR or CoD commands so that any of these mgmt commands
don't return a direct cmd_complete just because it looks like everything
is fine based on the CoD/EIR values in hdev.

Johan

Re: [PATCH v3 13/16] Bluetooth: Fix busy condition testing for EIR and class updates

[Marcel Holtmann]

Hi Johan,


>>> +static bool pending_eir_or_class(struct hci_dev *hdev)
>>> +{
>>> +	struct pending_cmd *cmd;
>>> +
>>> +	list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
>>> +		switch (cmd->opcode) {
>>> +		case MGMT_OP_ADD_UUID:
>>> +		case MGMT_OP_REMOVE_UUID:
>>> +		case MGMT_OP_SET_DEV_CLASS:
>>> +		case MGMT_OP_SET_POWERED:
>>> +			return true;
>>> +		}
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>> 
>> I do not like this at all. Why would we do it like this?
>> 
>> The transaction framework should allow us to just process these one at
>> a time. So even if userspace send 20 add_uuid commands at the same
>> time, we would deal with them one after the other.
>> 
>> What I am trying to understand what is the benefit of returning busy
>> here.
> 
> If we have multiple commands/transactions in the queue wanting to send
> the same HCI commands the problem is that the content of each HCI
> command (the CoD or EIR data) should depend on the content of the
> preceding HCI commands. The current EIR data or CoD (as stored in hdev
> variables) depends on the last completed write HCI command for the data.
> When we decide whether another command needs to be sent we compare what
> we want to send based on what the current value is and not what it will
> be (based on what's already in the queue).
> 
> Another problem is the handling of HCI command complete events for the
> same HCI commands. We don't know which pending mgmt command should be
> indicated as completed since there's no tight coupling of HCI commands
> and the mgmt commands that triggered them.
> 
> Since we strictly speaking don't need to send all those write_eir or
> write_cod commands in the queue but just the very last one (since that's
> what ultimately takes effect once everything is complete) we could in
> principle go digging into the queue and remove any earlier duplicates of
> the same HCI command and reply mgmt_cmd_complete for all pending
> commands when this one single HCI command completes. This is a bit messy
> for several reasons. One is that we still need to ensure that the
> complete callbacks for the transactions/commands we removed still get
> called. Another is that this will result in some quite interesting
> behavior:
> 
> 	1. Call add_uuid
> 	2. Call remove_uuid with the same uuid as in 1.
> 	3. The HCI command from 2. completes (the one from 1. was
> 	replaced).
> 	4. Success is returned for both mgmt commands when in fact the
> 	UUID added in 1 never got added in reality.
> 
> Considering the fact that we queue up all commands anyway I don't really
> have such a strong opinion on whether we allow potentially messed up
> behavior when such queuing doesn't happen, but the above is at least an
> attempt at explaining why I added this busy check.
> 
> One possible solution I can think of is that we update the CoD or EIR
> value in hdev as soon as we create a HCI command to update the value, as
> opposed to updating the hdev value when the HCI command completes. This
> would allow us to have sensible checks on whether new HCI commands need
> to be queued or not. We'd then have to have some flag to indicate that
> there are pending EIR or CoD commands so that any of these mgmt commands
> don't return a direct cmd_complete just because it looks like everything
> is fine based on the CoD/EIR values in hdev.

I see the dilemma now. Still not liking it that much. I wonder if we should just force one command of each opcode present at all time. No special handling for any of them. If you send the same opcode twice before it returned, you get busy error.

Regards

Marcel

Re: [PATCH v3 13/16] Bluetooth: Fix busy condition testing for EIR and class updates

[Johan Hedberg]

Hi Marcel,

On Fri, Mar 01, 2013, Marcel Holtmann wrote:
> > One possible solution I can think of is that we update the CoD or EIR
> > value in hdev as soon as we create a HCI command to update the value, as
> > opposed to updating the hdev value when the HCI command completes. This
> > would allow us to have sensible checks on whether new HCI commands need
> > to be queued or not. We'd then have to have some flag to indicate that
> > there are pending EIR or CoD commands so that any of these mgmt commands
> > don't return a direct cmd_complete just because it looks like everything
> > is fine based on the CoD/EIR values in hdev.
> 
> I see the dilemma now. Still not liking it that much. I wonder if we
> should just force one command of each opcode present at all time. No
> special handling for any of them. If you send the same opcode twice
> before it returned, you get busy error.

That still doesn't completely solve the issue since what we have is not
just the same command conflicting against another pending instance of
itself but several different commands conflicting against each other
(since they all can cause changes to the CoD or EIR).

Johan

Re: [PATCH v3 13/16] Bluetooth: Fix busy condition testing for EIR and class updates

[Marcel Holtmann]

Hi Johan,


>>> One possible solution I can think of is that we update the CoD or EIR
>>> value in hdev as soon as we create a HCI command to update the value, as
>>> opposed to updating the hdev value when the HCI command completes. This
>>> would allow us to have sensible checks on whether new HCI commands need
>>> to be queued or not. We'd then have to have some flag to indicate that
>>> there are pending EIR or CoD commands so that any of these mgmt commands
>>> don't return a direct cmd_complete just because it looks like everything
>>> is fine based on the CoD/EIR values in hdev.
>> 
>> I see the dilemma now. Still not liking it that much. I wonder if we
>> should just force one command of each opcode present at all time. No
>> special handling for any of them. If you send the same opcode twice
>> before it returned, you get busy error.
> 
> That still doesn't completely solve the issue since what we have is not
> just the same command conflicting against another pending instance of
> itself but several different commands conflicting against each other
> (since they all can cause changes to the CoD or EIR).

that is indeed a problem. As much as I do not like it, maybe doing it like you did is the only way to handle this. Can you add a comment on why we are doing this and send an updated patch series.

Regards

Marcel

[PATCH v3 14/16] Bluetooth: Fix UUID/class mgmt command response synchronization

[Johan Hedberg]

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

We should only return a mgmt command complete once all HCI commands to a
mgmt_set_dev_class or mgmt_add/remove_uuid command have completed. This
patch fixes the issue by having a proper transaction complete callback
for these actions and responding to user space in the callback.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 41a3265..d035bee 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1377,6 +1377,28 @@ static u8 get_uuid_size(const u8 *uuid)
 	return 16;
 }
 
+static void mgmt_class_complete(struct hci_dev *hdev, u16 mgmt_op, int status)
+{
+	struct pending_cmd *cmd;
+
+	cmd = mgmt_pending_find(mgmt_op, hdev);
+	if (!cmd)
+		return;
+
+	cmd_complete(cmd->sk, cmd->index, cmd->opcode, mgmt_status(status),
+		     hdev->dev_class, 3);
+
+	list_del(&cmd->list);
+	mgmt_pending_free(cmd);
+}
+
+static void add_uuid_complete(struct hci_dev *hdev, u16 opcode, int status)
+{
+	BT_DBG("opcode 0x%02x status %d", opcode, status);
+
+	mgmt_class_complete(hdev, MGMT_OP_ADD_UUID, status);
+}
+
 static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 {
 	struct mgmt_cp_add_uuid *cp = data;
@@ -1407,14 +1429,12 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 
 	list_add_tail(&uuid->list, &hdev->uuids);
 
-	hci_transaction_init(&transaction, hdev, NULL);
+	hci_transaction_init(&transaction, hdev, add_uuid_complete);
 
 	update_class(&transaction);
 	update_eir(&transaction);
 
-	hci_transaction_run(&transaction);
-
-	if (!test_bit(HCI_PENDING_CLASS, &hdev->dev_flags)) {
+	if (hci_transaction_run(&transaction) < 0) {
 		err = cmd_complete(sk, hdev->id, MGMT_OP_ADD_UUID, 0,
 				   hdev->dev_class, 3);
 		goto failed;
@@ -1447,6 +1467,13 @@ static bool enable_service_cache(struct hci_dev *hdev)
 	return false;
 }
 
+static void remove_uuid_complete(struct hci_dev *hdev, u16 opcode, int status)
+{
+	BT_DBG("opcode 0x%02x status %d", opcode, status);
+
+	mgmt_class_complete(hdev, MGMT_OP_REMOVE_UUID, status);
+}
+
 static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data,
 		       u16 len)
 {
@@ -1497,14 +1524,12 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data,
 	}
 
 update_class:
-	hci_transaction_init(&transaction, hdev, NULL);
+	hci_transaction_init(&transaction, hdev, remove_uuid_complete);
 
 	update_class(&transaction);
 	update_eir(&transaction);
 
-	hci_transaction_run(&transaction);
-
-	if (!test_bit(HCI_PENDING_CLASS, &hdev->dev_flags)) {
+	if (hci_transaction_run(&transaction) < 0) {
 		err = cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_UUID, 0,
 				   hdev->dev_class, 3);
 		goto unlock;
@@ -1523,6 +1548,13 @@ unlock:
 	return err;
 }
 
+static void set_class_complete(struct hci_dev *hdev, u16 opcode, int status)
+{
+	BT_DBG("opcode 0x%02x status %d", opcode, status);
+
+	mgmt_class_complete(hdev, MGMT_OP_SET_DEV_CLASS, status);
+}
+
 static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 			 u16 len)
 {
@@ -1560,7 +1592,7 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
-	hci_transaction_init(&transaction, hdev, NULL);
+	hci_transaction_init(&transaction, hdev, set_class_complete);
 
 	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->dev_flags)) {
 		hci_dev_unlock(hdev);
@@ -1571,9 +1603,7 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	update_class(&transaction);
 
-	hci_transaction_run(&transaction);
-
-	if (!test_bit(HCI_PENDING_CLASS, &hdev->dev_flags)) {
+	if (hci_transaction_run(&transaction) < 0) {
 		err = cmd_complete(sk, hdev->id, MGMT_OP_SET_DEV_CLASS, 0,
 				   hdev->dev_class, 3);
 		goto unlock;
@@ -3702,21 +3732,14 @@ int mgmt_ssp_enable_complete(struct hci_dev *hdev, u8 enable, u8 status)
 	return err;
 }
 
-static void class_rsp(struct pending_cmd *cmd, void *data)
+static void sk_lookup(struct pending_cmd *cmd, void *data)
 {
 	struct cmd_lookup *match = data;
 
-	cmd_complete(cmd->sk, cmd->index, cmd->opcode, match->mgmt_status,
-		     match->hdev->dev_class, 3);
-
-	list_del(&cmd->list);
-
 	if (match->sk == NULL) {
 		match->sk = cmd->sk;
 		sock_hold(match->sk);
 	}
-
-	mgmt_pending_free(cmd);
 }
 
 int mgmt_set_class_of_dev_complete(struct hci_dev *hdev, u8 *dev_class,
@@ -3727,9 +3750,9 @@ int mgmt_set_class_of_dev_complete(struct hci_dev *hdev, u8 *dev_class,
 
 	clear_bit(HCI_PENDING_CLASS, &hdev->dev_flags);
 
-	mgmt_pending_foreach(MGMT_OP_SET_DEV_CLASS, hdev, class_rsp, &match);
-	mgmt_pending_foreach(MGMT_OP_ADD_UUID, hdev, class_rsp, &match);
-	mgmt_pending_foreach(MGMT_OP_REMOVE_UUID, hdev, class_rsp, &match);
+	mgmt_pending_foreach(MGMT_OP_SET_DEV_CLASS, hdev, sk_lookup, &match);
+	mgmt_pending_foreach(MGMT_OP_ADD_UUID, hdev, sk_lookup, &match);
+	mgmt_pending_foreach(MGMT_OP_REMOVE_UUID, hdev, sk_lookup, &match);
 
 	if (!status)
 		err = mgmt_event(MGMT_EV_CLASS_OF_DEV_CHANGED, hdev, dev_class,
-- 
1.7.10.4

[PATCH v3 15/16] Bluetooth: Remove useless HCI_PENDING_CLASS flag

[Johan Hedberg]

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

Now that class related operations are tracked through HCI transactions
this flag is no longer needed.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci.h |    1 -
 net/bluetooth/hci_event.c   |    3 +--
 net/bluetooth/mgmt.c        |   11 ++---------
 3 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 7f12c25f..1e723c7 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -119,7 +119,6 @@ enum {
 	HCI_CONNECTABLE,
 	HCI_DISCOVERABLE,
 	HCI_LINK_SECURITY,
-	HCI_PENDING_CLASS,
 	HCI_PERIODIC_INQ,
 };
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d63efc8..8ab12fb 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -194,8 +194,7 @@ static void hci_cc_reset(struct hci_dev *hdev, struct sk_buff *skb)
 	clear_bit(HCI_RESET, &hdev->flags);
 
 	/* Reset all non-persistent flags */
-	hdev->dev_flags &= ~(BIT(HCI_LE_SCAN) | BIT(HCI_PENDING_CLASS) |
-			     BIT(HCI_PERIODIC_INQ));
+	hdev->dev_flags &= ~(BIT(HCI_LE_SCAN) | BIT(HCI_PERIODIC_INQ));
 
 	hdev->discovery.state = DISCOVERY_STOPPED;
 	hdev->inq_tx_power = HCI_TX_POWER_INVALID;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d035bee..f73c7da 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -636,7 +636,6 @@ static int update_class(struct hci_transaction *transaction)
 {
 	struct hci_dev *hdev = transaction->hdev;
 	u8 cod[3];
-	int err;
 
 	BT_DBG("%s", hdev->name);
 
@@ -653,12 +652,8 @@ static int update_class(struct hci_transaction *transaction)
 	if (memcmp(cod, hdev->dev_class, 3) == 0)
 		return 0;
 
-	err = hci_transaction_cmd(transaction, HCI_OP_WRITE_CLASS_OF_DEV,
-				  sizeof(cod), cod);
-	if (err == 0)
-		set_bit(HCI_PENDING_CLASS, &hdev->dev_flags);
-
-	return err;
+	return hci_transaction_cmd(transaction, HCI_OP_WRITE_CLASS_OF_DEV,
+				   sizeof(cod), cod);
 }
 
 static void service_cache_off(struct work_struct *work)
@@ -3748,8 +3743,6 @@ int mgmt_set_class_of_dev_complete(struct hci_dev *hdev, u8 *dev_class,
 	struct cmd_lookup match = { NULL, hdev, mgmt_status(status) };
 	int err = 0;
 
-	clear_bit(HCI_PENDING_CLASS, &hdev->dev_flags);
-
 	mgmt_pending_foreach(MGMT_OP_SET_DEV_CLASS, hdev, sk_lookup, &match);
 	mgmt_pending_foreach(MGMT_OP_ADD_UUID, hdev, sk_lookup, &match);
 	mgmt_pending_foreach(MGMT_OP_REMOVE_UUID, hdev, sk_lookup, &match);
-- 
1.7.10.4

[PATCH v3 16/16] Bluetooth: Remove empty HCI event handlers

[Johan Hedberg]

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

With the removal of hci_req_complete() several HCI event handles have
essentially become empty and can be removed. The only potential benefit
of these could have been logging, but the hci_event, hci_cmd_complete
and hci_cmd_status already provide a log for events which they do not
have an explicit handler for.

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

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 8ab12fb..d766cf0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -423,13 +423,6 @@ static void hci_cc_write_voice_setting(struct hci_dev *hdev,
 		hdev->notify(hdev, HCI_NOTIFY_VOICE_SETTING);
 }
 
-static void hci_cc_host_buffer_size(struct hci_dev *hdev, struct sk_buff *skb)
-{
-	__u8 status = *((__u8 *) skb->data);
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-}
-
 static void hci_cc_write_ssp_mode(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	__u8 status = *((__u8 *) skb->data);
@@ -631,13 +624,6 @@ static void hci_cc_read_data_block_size(struct hci_dev *hdev,
 	       hdev->block_cnt, hdev->block_len);
 }
 
-static void hci_cc_write_ca_timeout(struct hci_dev *hdev, struct sk_buff *skb)
-{
-	__u8 status = *((__u8 *) skb->data);
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-}
-
 static void hci_cc_read_local_amp_info(struct hci_dev *hdev,
 				       struct sk_buff *skb)
 {
@@ -700,29 +686,6 @@ a2mp_rsp:
 	a2mp_send_create_phy_link_req(hdev, rp->status);
 }
 
-static void hci_cc_delete_stored_link_key(struct hci_dev *hdev,
-					  struct sk_buff *skb)
-{
-	__u8 status = *((__u8 *) skb->data);
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-}
-
-static void hci_cc_set_event_mask(struct hci_dev *hdev, struct sk_buff *skb)
-{
-	__u8 status = *((__u8 *) skb->data);
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-}
-
-static void hci_cc_write_inquiry_mode(struct hci_dev *hdev,
-				      struct sk_buff *skb)
-{
-	__u8 status = *((__u8 *) skb->data);
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-}
-
 static void hci_cc_read_inq_rsp_tx_power(struct hci_dev *hdev,
 					 struct sk_buff *skb)
 {
@@ -734,13 +697,6 @@ static void hci_cc_read_inq_rsp_tx_power(struct hci_dev *hdev,
 		hdev->inq_tx_power = rp->tx_power;
 }
 
-static void hci_cc_set_event_flt(struct hci_dev *hdev, struct sk_buff *skb)
-{
-	__u8 status = *((__u8 *) skb->data);
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-}
-
 static void hci_cc_pin_code_reply(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_rp_pin_code_reply *rp = (void *) skb->data;
@@ -827,13 +783,6 @@ static void hci_cc_le_read_adv_tx_power(struct hci_dev *hdev,
 	}
 }
 
-static void hci_cc_le_set_event_mask(struct hci_dev *hdev, struct sk_buff *skb)
-{
-	__u8 status = *((__u8 *) skb->data);
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-}
-
 static void hci_cc_user_confirm_reply(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_rp_user_confirm_reply *rp = (void *) skb->data;
@@ -1014,26 +963,6 @@ static void hci_cc_le_read_white_list_size(struct hci_dev *hdev,
 		hdev->le_white_list_size = rp->size;
 }
 
-static void hci_cc_le_ltk_reply(struct hci_dev *hdev, struct sk_buff *skb)
-{
-	struct hci_rp_le_ltk_reply *rp = (void *) skb->data;
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
-
-	if (rp->status)
-		return;
-}
-
-static void hci_cc_le_ltk_neg_reply(struct hci_dev *hdev, struct sk_buff *skb)
-{
-	struct hci_rp_le_ltk_neg_reply *rp = (void *) skb->data;
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
-
-	if (rp->status)
-		return;
-}
-
 static void hci_cc_le_read_supported_states(struct hci_dev *hdev,
 					    struct sk_buff *skb)
 {
@@ -1564,11 +1493,6 @@ static void hci_cs_le_create_conn(struct hci_dev *hdev, __u8 status)
 	}
 }
 
-static void hci_cs_le_start_enc(struct hci_dev *hdev, u8 status)
-{
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-}
-
 static void hci_cs_create_phylink(struct hci_dev *hdev, u8 status)
 {
 	struct hci_cp_create_phy_link *cp;
@@ -1610,11 +1534,6 @@ static void hci_cs_accept_phylink(struct hci_dev *hdev, u8 status)
 	amp_write_remote_assoc(hdev, cp->phy_handle);
 }
 
-static void hci_cs_create_logical_link(struct hci_dev *hdev, u8 status)
-{
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-}
-
 static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	__u8 status = *((__u8 *) skb->data);
@@ -2171,17 +2090,6 @@ unlock:
 	hci_dev_unlock(hdev);
 }
 
-static void hci_remote_version_evt(struct hci_dev *hdev, struct sk_buff *skb)
-{
-	BT_DBG("%s", hdev->name);
-}
-
-static void hci_qos_setup_complete_evt(struct hci_dev *hdev,
-				       struct sk_buff *skb)
-{
-	BT_DBG("%s", hdev->name);
-}
-
 static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_cmd_complete *ev = (void *) skb->data;
@@ -2269,10 +2177,6 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cc_write_voice_setting(hdev, skb);
 		break;
 
-	case HCI_OP_HOST_BUFFER_SIZE:
-		hci_cc_host_buffer_size(hdev, skb);
-		break;
-
 	case HCI_OP_WRITE_SSP_MODE:
 		hci_cc_write_ssp_mode(hdev, skb);
 		break;
@@ -2305,10 +2209,6 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cc_read_data_block_size(hdev, skb);
 		break;
 
-	case HCI_OP_WRITE_CA_TIMEOUT:
-		hci_cc_write_ca_timeout(hdev, skb);
-		break;
-
 	case HCI_OP_READ_FLOW_CONTROL_MODE:
 		hci_cc_read_flow_control_mode(hdev, skb);
 		break;
@@ -2321,26 +2221,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cc_read_local_amp_assoc(hdev, skb);
 		break;
 
-	case HCI_OP_DELETE_STORED_LINK_KEY:
-		hci_cc_delete_stored_link_key(hdev, skb);
-		break;
-
-	case HCI_OP_SET_EVENT_MASK:
-		hci_cc_set_event_mask(hdev, skb);
-		break;
-
-	case HCI_OP_WRITE_INQUIRY_MODE:
-		hci_cc_write_inquiry_mode(hdev, skb);
-		break;
-
 	case HCI_OP_READ_INQ_RSP_TX_POWER:
 		hci_cc_read_inq_rsp_tx_power(hdev, skb);
 		break;
 
-	case HCI_OP_SET_EVENT_FLT:
-		hci_cc_set_event_flt(hdev, skb);
-		break;
-
 	case HCI_OP_PIN_CODE_REPLY:
 		hci_cc_pin_code_reply(hdev, skb);
 		break;
@@ -2365,10 +2249,6 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cc_le_read_adv_tx_power(hdev, skb);
 		break;
 
-	case HCI_OP_LE_SET_EVENT_MASK:
-		hci_cc_le_set_event_mask(hdev, skb);
-		break;
-
 	case HCI_OP_USER_CONFIRM_REPLY:
 		hci_cc_user_confirm_reply(hdev, skb);
 		break;
@@ -2401,14 +2281,6 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cc_le_read_white_list_size(hdev, skb);
 		break;
 
-	case HCI_OP_LE_LTK_REPLY:
-		hci_cc_le_ltk_reply(hdev, skb);
-		break;
-
-	case HCI_OP_LE_LTK_NEG_REPLY:
-		hci_cc_le_ltk_neg_reply(hdev, skb);
-		break;
-
 	case HCI_OP_LE_READ_SUPPORTED_STATES:
 		hci_cc_le_read_supported_states(hdev, skb);
 		break;
@@ -2500,10 +2372,6 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cs_le_create_conn(hdev, ev->status);
 		break;
 
-	case HCI_OP_LE_START_ENC:
-		hci_cs_le_start_enc(hdev, ev->status);
-		break;
-
 	case HCI_OP_CREATE_PHY_LINK:
 		hci_cs_create_phylink(hdev, ev->status);
 		break;
@@ -2512,10 +2380,6 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cs_accept_phylink(hdev, ev->status);
 		break;
 
-	case HCI_OP_CREATE_LOGICAL_LINK:
-		hci_cs_create_logical_link(hdev, ev->status);
-		break;
-
 	default:
 		BT_DBG("%s opcode 0x%4.4x", hdev->name, opcode);
 		break;
@@ -3076,18 +2940,6 @@ unlock:
 	hci_dev_unlock(hdev);
 }
 
-static void hci_sync_conn_changed_evt(struct hci_dev *hdev, struct sk_buff *skb)
-{
-	BT_DBG("%s", hdev->name);
-}
-
-static void hci_sniff_subrate_evt(struct hci_dev *hdev, struct sk_buff *skb)
-{
-	struct hci_ev_sniff_subrate *ev = (void *) skb->data;
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
-}
-
 static void hci_extended_inquiry_result_evt(struct hci_dev *hdev,
 					    struct sk_buff *skb)
 {
@@ -3815,14 +3667,6 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_remote_features_evt(hdev, skb);
 		break;
 
-	case HCI_EV_REMOTE_VERSION:
-		hci_remote_version_evt(hdev, skb);
-		break;
-
-	case HCI_EV_QOS_SETUP_COMPLETE:
-		hci_qos_setup_complete_evt(hdev, skb);
-		break;
-
 	case HCI_EV_CMD_COMPLETE:
 		hci_cmd_complete_evt(hdev, skb);
 		break;
@@ -3879,14 +3723,6 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_sync_conn_complete_evt(hdev, skb);
 		break;
 
-	case HCI_EV_SYNC_CONN_CHANGED:
-		hci_sync_conn_changed_evt(hdev, skb);
-		break;
-
-	case HCI_EV_SNIFF_SUBRATE:
-		hci_sniff_subrate_evt(hdev, skb);
-		break;
-
 	case HCI_EV_EXTENDED_INQUIRY_RESULT:
 		hci_extended_inquiry_result_evt(hdev, skb);
 		break;
-- 
1.7.10.4

Re: [PATCH v3 00/16] Bluetooth: Add HCI transaction framework

[Vinicius Costa Gomes]

Hi Johan,

On 09:57 Wed 27 Feb, Johan Hedberg wrote:
> Hi,
> 
> Here's a revised set with the left-over init_last_cmd patch inserted
> where it belongs in the set.
> 
> The other changes are based on an (off-list) review from Gustavo
> Padovan, resulting in removing unnecessary _irqsave versions of spinlock
> functions (for the skb queue), adding a hci_ prefix to some static
> hci_core.c functions as well as really removing the HCI_PENDING_CLASS
> flag (and not just its users).

Just got this while pluging a controller while bluetoothd was already running
(don't know if this is relevant):

[  294.725077] usb 1-2: new full-speed USB device number 2 using uhci_hcd
[  294.880486] usb 1-2: New USB device found, idVendor=0409, idProduct=55aa
[  294.881671] usb 1-2: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[  294.882815] usb 1-2: Product: QEMU USB Hub
[  294.883476] usb 1-2: Manufacturer: QEMU
[  294.884092] usb 1-2: SerialNumber: 314159-0000:00:01.2-2
[  294.887922] hub 1-2:1.0: USB hub found
[  294.889696] hub 1-2:1.0: 8 ports detected
[  295.333565] usb 1-2.1: new full-speed USB device number 3 using uhci_hcd
[  295.590206] usb 1-2.1: New USB device found, idVendor=0a5c, idProduct=21e8
[  295.590928] usb 1-2.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[  295.591799] usb 1-2.1: Product: BCM20702A0
[  295.592315] usb 1-2.1: Manufacturer: Broadcom Corp
[  295.592810] usb 1-2.1: SerialNumber: 000272D69969
[  295.627414] usbcore: registered new interface driver btusb
[  295.629706] 
[  295.630090] =================================
[  295.630668] [ INFO: inconsistent lock state ]
[  295.630668] 3.8.0-rc1-13045-g6303877 #1 Tainted: G        W   
[  295.630668] ---------------------------------
[  295.630668] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
[  295.630668] swapper/1/0 [HC1[1]:SC0[0]:HE0:SE1] takes:
[  295.630668]  (&(&list->lock)->rlock#5){?.+...}, at: [<ffffffff813c5a41>] skb_queue_tail+0x1c/0x46
[  295.630668] {HARDIRQ-ON-W} state was registered at:
[  295.630668]   [<ffffffff8106256e>] __lock_acquire+0x323/0xe85
[  295.630668]   [<ffffffff8106353f>] lock_acquire+0x93/0xb1
[  295.630668]   [<ffffffff814fd240>] _raw_spin_lock+0x40/0x74
[  295.630668]   [<ffffffffa0004345>] hci_transaction_run+0x4d/0xb3 [bluetooth]
[  295.630668]   [<ffffffffa000449a>] __hci_request+0xef/0x1c7 [bluetooth]
[  295.630668]   [<ffffffffa000565e>] hci_dev_open+0x15d/0x29a [bluetooth]
[  295.630668]   [<ffffffffa00057d6>] hci_power_on+0x3b/0x8a [bluetooth]
[  295.630668]   [<ffffffff8103eb46>] process_one_work+0x1c2/0x31b
[  295.630668]   [<ffffffff8103ef7e>] worker_thread+0x12e/0x1cc
[  295.630668]   [<ffffffff8104393d>] kthread+0x9d/0xa5
[  295.630668]   [<ffffffff814fe7bc>] ret_from_fork+0x7c/0xb0
[  295.630668] irq event stamp: 289936
[  295.630668] hardirqs last  enabled at (289933): [<ffffffff81008a7c>] default_idle+0x25/0x4a
[  295.630668] hardirqs last disabled at (289934): [<ffffffff814fdf2d>] common_interrupt+0x6d/0x72
[  295.630668] softirqs last  enabled at (289936): [<ffffffff810309f9>] _local_bh_enable+0xe/0x10
[  295.630668] softirqs last disabled at (289935): [<ffffffff81030fb5>] irq_enter+0x3f/0x71
[  295.630668] 
[  295.630668] other info that might help us debug this:
[  295.630668]  Possible unsafe locking scenario:
[  295.630668] 
[  295.630668]        CPU0
[  295.630668]        ----
[  295.630668]   lock(&(&list->lock)->rlock#5);
[  295.630668]   <Interrupt>
[  295.630668]     lock(&(&list->lock)->rlock#5);
[  295.630668] 
[  295.630668]  *** DEADLOCK ***
[  295.630668] 
[  295.630668] no locks held by swapper/1/0.
[  295.630668] 
[  295.630668] stack backtrace:
[  295.630668] Pid: 0, comm: swapper/1 Tainted: G        W    3.8.0-rc1-13045-g6303877 #1
[  295.630668] Call Trace:
[  295.630668]  <IRQ>  [<ffffffff814f6c7c>] print_usage_bug+0x1f7/0x208
[  295.630668]  [<ffffffff8100c3d6>] ? save_stack_trace+0x27/0x44
[  295.630668]  [<ffffffff81060548>] ? check_usage_backwards+0x96/0x96
[  295.630668]  [<ffffffff81060d45>] mark_lock+0x11b/0x247
[  295.630668]  [<ffffffff810624ff>] __lock_acquire+0x2b4/0xe85
[  295.630668]  [<ffffffff81060c57>] ? mark_lock+0x2d/0x247
[  295.630668]  [<ffffffff8106353f>] lock_acquire+0x93/0xb1
[  295.630668]  [<ffffffff813c5a41>] ? skb_queue_tail+0x1c/0x46
[  295.630668]  [<ffffffff810c2992>] ? __kmalloc_track_caller+0xf2/0x10a
[  295.630668]  [<ffffffff814fdccb>] _raw_spin_lock_irqsave+0x55/0x8f
[  295.630668]  [<ffffffff813c5a41>] ? skb_queue_tail+0x1c/0x46
[  295.630668]  [<ffffffff810583aa>] ? timekeeping_get_ns.constprop.10+0x12/0x38
[  295.630668]  [<ffffffff813c5a41>] skb_queue_tail+0x1c/0x46
[  295.630668]  [<ffffffffa000132f>] hci_recv_frame+0x57/0x71 [bluetooth]
[  295.630668]  [<ffffffffa0002422>] hci_reassembly+0x15d/0x196 [bluetooth]
[  295.630668]  [<ffffffffa00024f8>] hci_recv_fragment+0x41/0x66 [bluetooth]
[  295.630668]  [<ffffffffa006d31d>] btusb_intr_complete+0x8f/0x123 [btusb]
[  295.630668]  [<ffffffff81355f46>] usb_hcd_giveback_urb+0x76/0xbe
[  295.630668]  [<ffffffff813746b1>] uhci_giveback_urb+0x107/0x20c
[  295.630668]  [<ffffffff81060c57>] ? mark_lock+0x2d/0x247
[  295.630668]  [<ffffffff81374cfa>] uhci_scan_schedule+0x544/0x7b2
[  295.630668]  [<ffffffff81061c4b>] ? lock_acquired+0x1b5/0x1cf
[  295.630668]  [<ffffffff81376034>] uhci_irq+0xf4/0x10a
[  295.630668]  [<ffffffff813553bb>] usb_hcd_irq+0x41/0x75
[  295.630668]  [<ffffffff8107afaa>] handle_irq_event_percpu+0x2a/0x135
[  295.630668]  [<ffffffff8107b0f1>] handle_irq_event+0x3c/0x5f
[  295.630668]  [<ffffffff814fd26c>] ? _raw_spin_lock+0x6c/0x74
[  295.630668]  [<ffffffff8107d778>] ? handle_fasteoi_irq+0x19/0xb0
[  295.630668]  [<ffffffff8107d7d9>] handle_fasteoi_irq+0x7a/0xb0
[  295.630668]  [<ffffffff81003593>] handle_irq+0x1a/0x24
[  295.630668]  [<ffffffff810032aa>] do_IRQ+0x48/0xa0
[  295.630668]  [<ffffffff814fdf32>] common_interrupt+0x72/0x72
[  295.630668]  <EOI>  [<ffffffff8101eca3>] ? native_safe_halt+0x6/0x8
[  295.630668]  [<ffffffff810610c4>] ? trace_hardirqs_on+0xd/0xf
[  295.630668]  [<ffffffff81008a81>] default_idle+0x2a/0x4a
[  295.630668]  [<ffffffff8100920d>] cpu_idle+0x6a/0xb8
[  295.630668]  [<ffffffff814f0814>] start_secondary+0x21e/0x220


Cheers,
-- 
Vinicius

Re: [PATCH v3 00/16] Bluetooth: Add HCI transaction framework

[Johan Hedberg]

Hi Vinicius,

On Thu, Feb 28, 2013, Vinicius Costa Gomes wrote:
> [  295.630668]  (&(&list->lock)->rlock#5){?.+...}, at: [<ffffffff813c5a41>] skb_queue_tail+0x1c/0x46
> [  295.630668] {HARDIRQ-ON-W} state was registered at:
> [  295.630668]   [<ffffffff8106256e>] __lock_acquire+0x323/0xe85
> [  295.630668]   [<ffffffff8106353f>] lock_acquire+0x93/0xb1
> [  295.630668]   [<ffffffff814fd240>] _raw_spin_lock+0x40/0x74
> [  295.630668]   [<ffffffffa0004345>] hci_transaction_run+0x4d/0xb3 [bluetooth]
> [  295.630668]   [<ffffffffa000449a>] __hci_request+0xef/0x1c7 [bluetooth]
> [  295.630668]   [<ffffffffa000565e>] hci_dev_open+0x15d/0x29a [bluetooth]
> [  295.630668]   [<ffffffffa00057d6>] hci_power_on+0x3b/0x8a [bluetooth]
> [  295.630668]   [<ffffffff8103eb46>] process_one_work+0x1c2/0x31b
> [  295.630668]   [<ffffffff8103ef7e>] worker_thread+0x12e/0x1cc
> [  295.630668]   [<ffffffff8104393d>] kthread+0x9d/0xa5
> [  295.630668]   [<ffffffff814fe7bc>] ret_from_fork+0x7c/0xb0

Ok, so if I interpret that right it says that some lock in
hci_transaction_run (the only one it could be is the hdev->cmd_q one) is
deadlocking against a lock taken at:
[<ffffffff813c5a41>] skb_queue_tail+0x1c/0x46

Then, looking at the rest of the trace:

> [  295.630668]  [<ffffffff8106353f>] lock_acquire+0x93/0xb1
> [  295.630668]  [<ffffffff813c5a41>] ? skb_queue_tail+0x1c/0x46
> [  295.630668]  [<ffffffff810c2992>] ? __kmalloc_track_caller+0xf2/0x10a
> [  295.630668]  [<ffffffff814fdccb>] _raw_spin_lock_irqsave+0x55/0x8f
> [  295.630668]  [<ffffffff813c5a41>] ? skb_queue_tail+0x1c/0x46
> [  295.630668]  [<ffffffff810583aa>] ? timekeeping_get_ns.constprop.10+0x12/0x38
> [  295.630668]  [<ffffffff813c5a41>] skb_queue_tail+0x1c/0x46
> [  295.630668]  [<ffffffffa000132f>] hci_recv_frame+0x57/0x71 [bluetooth]
> [  295.630668]  [<ffffffffa0002422>] hci_reassembly+0x15d/0x196 [bluetooth]
> [  295.630668]  [<ffffffffa00024f8>] hci_recv_fragment+0x41/0x66 [bluetooth]
> [  295.630668]  [<ffffffffa006d31d>] btusb_intr_complete+0x8f/0x123 [btusb]

There we have the "[<ffffffff813c5a41>] ? skb_queue_tail+0x1c/0x46" call
which looks like the lock for hdev->rx_q that hci_recv_frame operates on
(hci_recv_frame is typically run in interrupt context).

This doesn't make any sense to me though. How could the lock for
hdev->rx_q deadlock with the lock for hdev->cmd_q? Those are two
different spinlocks. Or am I misinterpreting the backtrace?

Could this be a false-positive report from the kernel, i.e. did
everything work fine even though you got the report? What I could do is
bring back the _irqsave versions of the locking functions (something I
removed since Gustavo asked me to) in case that's what's causing this
deadlock report/warning to be generated.

Johan