[PATCH v2 1/5] Bluetooth: btintel: Make controller version read generic

[PATCH v2 1/5] Bluetooth: btintel: Make controller version read generic

[Kiran K]

Make controller read vesion function more generic to support different
type of controllers.

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---

Changes in v2: None
Changes in v1:
- Make controller read version function generic

 drivers/bluetooth/btintel.c   | 36 ++++++++++++++----
 drivers/bluetooth/btintel.h   | 15 ++++++--
 drivers/bluetooth/btusb.c     | 71 +++++++++++++++++++++++------------
 drivers/bluetooth/hci_ag6xx.c | 12 +++++-
 drivers/bluetooth/hci_intel.c | 12 +++++-
 5 files changed, 106 insertions(+), 40 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 5fa5be3c5598..dea96c585ecb 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -204,9 +204,15 @@ void btintel_hw_error(struct hci_dev *hdev, u8 code)
 }
 EXPORT_SYMBOL_GPL(btintel_hw_error);
 
-void btintel_version_info(struct hci_dev *hdev, struct intel_version *ver)
+void btintel_version_info(struct hci_dev *hdev, const struct btintel_version *version)
 {
 	const char *variant;
+	const struct intel_version *ver;
+
+	if (version->is_tlv_supported)
+		return;
+
+	ver = &version->intel_version;
 
 	switch (ver->fw_variant) {
 	case 0x06:
@@ -335,27 +341,41 @@ int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug)
 }
 EXPORT_SYMBOL_GPL(btintel_set_event_mask_mfg);
 
-int btintel_read_version(struct hci_dev *hdev, struct intel_version *ver)
+int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
 {
 	struct sk_buff *skb;
+	u8 *data, param, status, check_tlv;
+
+	if (!version)
+		return -EINVAL;
 
-	skb = __hci_cmd_sync(hdev, 0xfc05, 0, NULL, HCI_CMD_TIMEOUT);
+	param = 0xFF;
+
+	skb = __hci_cmd_sync(hdev, 0xfc05, 1, &param, HCI_CMD_TIMEOUT);
 	if (IS_ERR(skb)) {
 		bt_dev_err(hdev, "Reading Intel version information failed (%ld)",
 			   PTR_ERR(skb));
 		return PTR_ERR(skb);
 	}
 
-	if (skb->len != sizeof(*ver)) {
-		bt_dev_err(hdev, "Intel version event size mismatch");
+	data = skb->data;
+	status = *data;
+	if (status) {
+		bt_dev_err(hdev, "Intel Read Version command failed (%02x)",
+			   status);
 		kfree_skb(skb);
-		return -EILSEQ;
+		return -bt_to_errno(status);
 	}
 
-	memcpy(ver, skb->data, sizeof(*ver));
+	check_tlv = *(data + 1);
 
+	if (skb->len == sizeof(version->intel_version) && check_tlv == 0x37) {
+		memcpy(&version->intel_version, skb->data, sizeof(version->intel_version));
+		version->is_tlv_supported = false;
+	} else {
+		version->is_tlv_supported = true;
+	}
 	kfree_skb(skb);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(btintel_read_version);
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index 08e20606fb58..0865d2d4aca7 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -66,6 +66,13 @@ struct intel_debug_features {
 	__u8    page1[16];
 } __packed;
 
+struct btintel_version {
+	bool is_tlv_supported;
+	union {
+		struct intel_version intel_version; /* legacy version */
+	};
+} __packed;
+
 #if IS_ENABLED(CONFIG_BT_INTEL)
 
 int btintel_check_bdaddr(struct hci_dev *hdev);
@@ -76,13 +83,13 @@ int btintel_set_diag(struct hci_dev *hdev, bool enable);
 int btintel_set_diag_mfg(struct hci_dev *hdev, bool enable);
 void btintel_hw_error(struct hci_dev *hdev, u8 code);
 
-void btintel_version_info(struct hci_dev *hdev, struct intel_version *ver);
+void btintel_version_info(struct hci_dev *hdev, const struct btintel_version *version);
 int btintel_secure_send(struct hci_dev *hdev, u8 fragment_type, u32 plen,
 			const void *param);
 int btintel_load_ddc_config(struct hci_dev *hdev, const char *ddc_name);
 int btintel_set_event_mask(struct hci_dev *hdev, bool debug);
 int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug);
-int btintel_read_version(struct hci_dev *hdev, struct intel_version *ver);
+int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version);
 
 struct regmap *btintel_regmap_init(struct hci_dev *hdev, u16 opcode_read,
 				   u16 opcode_write);
@@ -133,7 +140,7 @@ static inline void btintel_hw_error(struct hci_dev *hdev, u8 code)
 }
 
 static inline void btintel_version_info(struct hci_dev *hdev,
-					struct intel_version *ver)
+					struct btintel_version *version)
 {
 }
 
@@ -160,7 +167,7 @@ static inline int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug)
 }
 
 static inline int btintel_read_version(struct hci_dev *hdev,
-				       struct intel_version *ver)
+				       struct btintel_version *version)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index faa863dd5d0a..d06c946f7810 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1938,6 +1938,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
 	const u8 *fw_ptr;
 	int disable_patch, err;
 	struct intel_version ver;
+	struct btintel_version version;
 
 	BT_DBG("%s", hdev->name);
 
@@ -1963,10 +1964,16 @@ static int btusb_setup_intel(struct hci_dev *hdev)
 	 * The returned information are hardware variant and revision plus
 	 * firmware variant, revision and build number.
 	 */
-	err = btintel_read_version(hdev, &ver);
+	err = btintel_read_version(hdev, &version);
 	if (err)
 		return err;
 
+	if (version.is_tlv_supported) {
+		bt_dev_err(hdev, "FW download in tlv format not supported");
+		return -EOPNOTSUPP;
+	}
+	ver = version.intel_version;
+
 	bt_dev_info(hdev, "read Intel version: %02x%02x%02x%02x%02x%02x%02x%02x%02x",
 		    ver.hw_platform, ver.hw_variant, ver.hw_revision,
 		    ver.fw_variant,  ver.fw_revision, ver.fw_build_num,
@@ -2049,11 +2056,11 @@ static int btusb_setup_intel(struct hci_dev *hdev)
 	/* Need build number for downloaded fw patches in
 	 * every power-on boot
 	 */
-       err = btintel_read_version(hdev, &ver);
-       if (err)
-               return err;
-       bt_dev_info(hdev, "Intel BT fw patch 0x%02x completed & activated",
-		   ver.fw_patch_num);
+	err = btintel_read_version(hdev, &version);
+	if (err)
+		return err;
+	bt_dev_info(hdev, "Intel BT fw patch 0x%02x completed & activated",
+		    version.intel_version.fw_patch_num);
 
 	goto complete;
 
@@ -2251,11 +2258,18 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
 	return -EILSEQ;
 }
 
-static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
-					     struct intel_boot_params *params,
-					     char *fw_name, size_t len,
-					     const char *suffix)
+static bool btusb_setup_intel_new_get_fw_name(const struct btintel_version *version,
+					      struct intel_boot_params *params,
+					      char *fw_name, size_t len,
+					      const char *suffix)
 {
+	const struct intel_version *ver;
+
+	if (version->is_tlv_supported)
+		return false;
+
+	ver = &version->intel_version;
+
 	switch (ver->hw_variant) {
 	case 0x0b:	/* SfP */
 	case 0x0c:	/* WsP */
@@ -2281,18 +2295,21 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
 }
 
 static int btusb_intel_download_firmware(struct hci_dev *hdev,
-					 struct intel_version *ver,
+					 struct btintel_version *version,
 					 struct intel_boot_params *params)
 {
 	const struct firmware *fw;
 	u32 boot_param;
 	char fwname[64];
 	int err;
+	const struct intel_version *ver;
 	struct btusb_data *data = hci_get_drvdata(hdev);
 
-	if (!ver || !params)
+	if (!version || !params)
 		return -EINVAL;
 
+	ver = &version->intel_version;
+
 	/* The hardware platform number has a fixed value of 0x37 and
 	 * for now only accept this single value.
 	 */
@@ -2322,8 +2339,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 		return -EINVAL;
 	}
 
-	btintel_version_info(hdev, ver);
-
 	/* The firmware variant determines if the device is in bootloader
 	 * mode or is running operational firmware. The value 0x06 identifies
 	 * the bootloader and the value 0x23 identifies the operational
@@ -2398,7 +2413,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 	 * ibt-<hw_variant>-<hw_revision>-<fw_revision>.sfi.
 	 *
 	 */
-	err = btusb_setup_intel_new_get_fw_name(ver, params, fwname,
+	err = btusb_setup_intel_new_get_fw_name(version, params, fwname,
 						sizeof(fwname), "sfi");
 	if (!err) {
 		bt_dev_err(hdev, "Unsupported Intel firmware naming");
@@ -2483,6 +2498,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 	unsigned long long duration;
 	int err;
 	struct intel_debug_features features;
+	struct btintel_version version;
 
 	BT_DBG("%s", hdev->name);
 
@@ -2494,21 +2510,28 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 
 	calltime = ktime_get();
 
-	/* Read the Intel version information to determine if the device
-	 * is in bootloader mode or if it already has operational firmware
-	 * loaded.
+	/* Read controller version information and support of tlv format
 	 */
-	err = btintel_read_version(hdev, &ver);
+	err = btintel_read_version(hdev, &version);
 	if (err) {
-		bt_dev_err(hdev, "Intel Read version failed (%d)", err);
+		bt_dev_err(hdev, "Intel Read version new failed (%d)", err);
 		btintel_reset_to_bootloader(hdev);
 		return err;
 	}
 
-	err = btusb_intel_download_firmware(hdev, &ver, &params);
+	if (version.is_tlv_supported) {
+		bt_dev_err(hdev, "Firmware download in tlv format is not supported");
+		return -EOPNOTSUPP;
+	}
+
+	btintel_version_info(hdev, &version);
+
+	err = btusb_intel_download_firmware(hdev, &version, &params);
 	if (err)
 		return err;
 
+	ver = version.intel_version;
+
 	/* controller is already having an operational firmware */
 	if (ver.fw_variant == 0x23)
 		goto finish;
@@ -2562,7 +2585,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 
 	clear_bit(BTUSB_BOOTLOADER, &data->flags);
 
-	err = btusb_setup_intel_new_get_fw_name(&ver, &params, ddcname,
+	err = btusb_setup_intel_new_get_fw_name(&version, &params, ddcname,
 						sizeof(ddcname), "ddc");
 
 	if (!err) {
@@ -2586,11 +2609,11 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 	btintel_set_debug_features(hdev, &features);
 
 	/* Read the Intel version information after loading the FW  */
-	err = btintel_read_version(hdev, &ver);
+	err = btintel_read_version(hdev, &version);
 	if (err)
 		return err;
 
-	btintel_version_info(hdev, &ver);
+	btintel_version_info(hdev, &version);
 
 finish:
 	/* All Intel controllers that support the Microsoft vendor
diff --git a/drivers/bluetooth/hci_ag6xx.c b/drivers/bluetooth/hci_ag6xx.c
index 1f55df93e4ce..6f6a1e061972 100644
--- a/drivers/bluetooth/hci_ag6xx.c
+++ b/drivers/bluetooth/hci_ag6xx.c
@@ -153,6 +153,7 @@ static int ag6xx_setup(struct hci_uart *hu)
 	struct hci_dev *hdev = hu->hdev;
 	struct sk_buff *skb;
 	struct intel_version ver;
+	struct btintel_version version;
 	const struct firmware *fw;
 	const u8 *fw_ptr;
 	char fwname[64];
@@ -166,11 +167,18 @@ static int ag6xx_setup(struct hci_uart *hu)
 	if (err)
 		return err;
 
-	err = btintel_read_version(hdev, &ver);
+	err = btintel_read_version(hdev, &version);
 	if (err)
 		return err;
 
-	btintel_version_info(hdev, &ver);
+	if (version.is_tlv_supported) {
+		bt_dev_err(hdev, "Firmware download in tlv format over ag6xx is not supported");
+		return -EOPNOTSUPP;
+	}
+
+	btintel_version_info(hdev, &version);
+
+	ver = version.intel_version;
 
 	/* The hardware platform number has a fixed value of 0x37 and
 	 * for now only accept this single value.
diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index f1299da6eed8..f30cbc66d48f 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -532,6 +532,7 @@ static int intel_setup(struct hci_uart *hu)
 	struct hci_dev *hdev = hu->hdev;
 	struct sk_buff *skb;
 	struct intel_version ver;
+	struct btintel_version version;
 	struct intel_boot_params params;
 	struct list_head *p;
 	const struct firmware *fw;
@@ -584,10 +585,17 @@ static int intel_setup(struct hci_uart *hu)
 	 * is in bootloader mode or if it already has operational firmware
 	 * loaded.
 	 */
-	err = btintel_read_version(hdev, &ver);
+	err = btintel_read_version(hdev, &version);
 	if (err)
 		return err;
 
+	if (version.is_tlv_supported) {
+		/* firmware download in tlv format is not supported on UART transport */
+		bt_dev_err(hdev, "Firmware download in tlv format is not supported");
+		return -EOPNOTSUPP;
+	}
+	ver = version.intel_version;
+
 	/* The hardware platform number has a fixed value of 0x37 and
 	 * for now only accept this single value.
 	 */
@@ -614,7 +622,7 @@ static int intel_setup(struct hci_uart *hu)
 		return -EINVAL;
 	}
 
-	btintel_version_info(hdev, &ver);
+	btintel_version_info(hdev, &version);
 
 	/* The firmware variant determines if the device is in bootloader
 	 * mode or is running operational firmware. The value 0x06 identifies
-- 
2.17.1

[PATCH v2 2/5] Bluetooth: btintel: Refactor firmware header download sequence

[Kiran K]

Move firmware header download code to a separate function to
enhance readability and reusability

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---

Changes in v2:
  - Remove unwanted blank line, add empty line before goto label
Changes in v1:
  - Move firmware header download code to new function


 drivers/bluetooth/btintel.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index dea96c585ecb..ffc888c4e9e3 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -646,12 +646,10 @@ int btintel_read_boot_params(struct hci_dev *hdev,
 }
 EXPORT_SYMBOL_GPL(btintel_read_boot_params);
 
-int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
-			      u32 *boot_param)
+static int btintel_sfi_rsa_header_secure_send(struct hci_dev *hdev,
+					      const struct firmware *fw)
 {
 	int err;
-	const u8 *fw_ptr;
-	u32 frag_len;
 
 	/* Start the firmware download transaction with the Init fragment
 	 * represented by the 128 bytes of CSS header.
@@ -680,6 +678,21 @@ int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
 		goto done;
 	}
 
+done:
+	return err;
+}
+
+int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
+			      u32 *boot_param)
+{
+	int err;
+	const u8 *fw_ptr;
+	u32 frag_len;
+
+	err = btintel_sfi_rsa_header_secure_send(hdev, fw);
+	if (err)
+		goto done;
+
 	fw_ptr = fw->data + 644;
 	frag_len = 0;
 
-- 
2.17.1

[PATCH v2 3/5] Bluetooth: btintel: Refactor firmware payload download code

[Kiran K]

Move firmware payload download code to a separate function to
enhance readability and reusability

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---

Changes in v2:
- Avoid exporting new public function for firmware download
- Move firmware payload download code to a new function
Changes in v1:
- Move firmware payload download code to a new function


 drivers/bluetooth/btintel.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index ffc888c4e9e3..2cb55a97598c 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -19,6 +19,7 @@
 #define VERSION "0.1"
 
 #define BDADDR_INTEL (&(bdaddr_t) {{0x00, 0x8b, 0x9e, 0x19, 0x03, 0x00}})
+#define RSA_HEADER_LEN	644
 
 int btintel_check_bdaddr(struct hci_dev *hdev)
 {
@@ -682,8 +683,9 @@ static int btintel_sfi_rsa_header_secure_send(struct hci_dev *hdev,
 	return err;
 }
 
-int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
-			      u32 *boot_param)
+static int btintel_download_firmware_payload(struct hci_dev *hdev,
+					     const struct firmware *fw,
+					     u32 *boot_param, size_t offset)
 {
 	int err;
 	const u8 *fw_ptr;
@@ -693,8 +695,9 @@ int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
 	if (err)
 		goto done;
 
-	fw_ptr = fw->data + 644;
+	fw_ptr = fw->data + offset;
 	frag_len = 0;
+	err = -EINVAL;
 
 	while (fw_ptr - fw->data < fw->size) {
 		struct hci_command_hdr *cmd = (void *)(fw_ptr + frag_len);
@@ -740,6 +743,19 @@ int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
 done:
 	return err;
 }
+
+int btintel_download_firmware(struct hci_dev *hdev,
+			      const struct firmware *fw,
+			      u32 *boot_param)
+{
+	int err;
+
+	err = btintel_sfi_rsa_header_secure_send(hdev, fw);
+	if (err)
+		return err;
+
+	return btintel_download_firmware_payload(hdev, fw, boot_param, RSA_HEADER_LEN);
+}
 EXPORT_SYMBOL_GPL(btintel_download_firmware);
 
 void btintel_reset_to_bootloader(struct hci_dev *hdev)
-- 
2.17.1

Re: [PATCH v2 3/5] Bluetooth: btintel: Refactor firmware payload download code

[Marcel Holtmann]

Hi Kiran,

> Move firmware payload download code to a separate function to
> enhance readability and reusability
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> 
> Changes in v2:
> - Avoid exporting new public function for firmware download
> - Move firmware payload download code to a new function
> Changes in v1:
> - Move firmware payload download code to a new function
> 
> 
> drivers/bluetooth/btintel.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index ffc888c4e9e3..2cb55a97598c 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -19,6 +19,7 @@
> #define VERSION "0.1"
> 
> #define BDADDR_INTEL (&(bdaddr_t) {{0x00, 0x8b, 0x9e, 0x19, 0x03, 0x00}})
> +#define RSA_HEADER_LEN	644
> 
> int btintel_check_bdaddr(struct hci_dev *hdev)
> {
> @@ -682,8 +683,9 @@ static int btintel_sfi_rsa_header_secure_send(struct hci_dev *hdev,
> 	return err;
> }
> 
> -int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
> -			      u32 *boot_param)
> +static int btintel_download_firmware_payload(struct hci_dev *hdev,
> +					     const struct firmware *fw,
> +					     u32 *boot_param, size_t offset)
> {
> 	int err;
> 	const u8 *fw_ptr;
> @@ -693,8 +695,9 @@ int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
> 	if (err)
> 		goto done;
> 
> -	fw_ptr = fw->data + 644;
> +	fw_ptr = fw->data + offset;
> 	frag_len = 0;
> +	err = -EINVAL;
> 
> 	while (fw_ptr - fw->data < fw->size) {
> 		struct hci_command_hdr *cmd = (void *)(fw_ptr + frag_len);
> @@ -740,6 +743,19 @@ int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
> done:
> 	return err;
> }
> +
> +int btintel_download_firmware(struct hci_dev *hdev,
> +			      const struct firmware *fw,
> +			      u32 *boot_param)
> +{
> +	int err;
> +
> +	err = btintel_sfi_rsa_header_secure_send(hdev, fw);
> +	if (err)
> +		return err;
> +
> +	return btintel_download_firmware_payload(hdev, fw, boot_param, RSA_HEADER_LEN);
> +}
> EXPORT_SYMBOL_GPL(btintel_download_firmware);
> 
> void btintel_reset_to_bootloader(struct hci_dev *hdev)

can we apply patch 1 and 2 independently from general TLV support? If so, then I would just try to get these two applied right away.

Regards

Marcel

[PATCH v2 4/5] Bluetooth: btintel: Define tlv structure for new generation Controllers

[Kiran K]

Define structure used for reading controller information and
to downloading firmware in tlv format used for new generation
Intel controllers

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---

Changes in v2: None
Changes in v1:
- Add tlv structure definition


 drivers/bluetooth/btintel.h | 85 +++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index 0865d2d4aca7..20007da6b9bd 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -6,6 +6,90 @@
  *  Copyright (C) 2015  Intel Corporation
  */
 
+/* List of tlv type */
+enum {
+	INTEL_TLV_CNVI_TOP = 0x10,
+	INTEL_TLV_CNVR_TOP,
+	INTEL_TLV_CNVI_BT,
+	INTEL_TLV_CNVR_BT,
+	INTEL_TLV_CNVI_OTP,
+	INTEL_TLV_CNVR_OTP,
+	INTEL_TLV_DEV_REV_ID,
+	INTEL_TLV_USB_VENDOR_ID,
+	INTEL_TLV_USB_PRODUCT_ID,
+	INTEL_TLV_PCIE_VENDOR_ID,
+	INTEL_TLV_PCIE_DEVICE_ID,
+	INTEL_TLV_PCIE_SUBSYSTEM_ID,
+	INTEL_TLV_IMAGE_TYPE,
+	INTEL_TLV_TIME_STAMP,
+	INTEL_TLV_BUILD_TYPE,
+	INTEL_TLV_BUILD_NUM,
+	INTEL_TLV_FW_BUILD_PRODUCT,
+	INTEL_TLV_FW_BUILD_HW,
+	INTEL_TLV_FW_STEP,
+	INTEL_TLV_BT_SPEC,
+	INTEL_TLV_MFG_NAME,
+	INTEL_TLV_HCI_REV,
+	INTEL_TLV_LMP_SUBVER,
+	INTEL_TLV_OTP_PATCH_VER,
+	INTEL_TLV_SECURE_BOOT,
+	INTEL_TLV_KEY_FROM_HDR,
+	INTEL_TLV_OTP_LOCK,
+	INTEL_TLV_API_LOCK,
+	INTEL_TLV_DEBUG_LOCK,
+	INTEL_TLV_MIN_FW,
+	INTEL_TLV_LIMITED_CCE,
+	INTEL_TLV_SBE_TYPE,
+	INTEL_TLV_OTP_BDADDR,
+	INTEL_TLV_UNLOCKED_STATE
+};
+
+struct intel_tlv {
+	u8 type;
+	u8 len;
+	u8 val[0];
+} __packed;
+
+struct intel_version_tlv {
+	u8	 status;
+	u32	 cnvi_top;
+	u32	 cnvr_top;
+	u32	 cnvi_bt;
+	u32	 cnvr_bt;
+	u16	 cnvi_otp;
+	u16	 cnvr_otp;
+	u16	 dev_rev_id;
+	u16	 usb_vid;
+	u16	 usb_pid;
+	u16	 pcie_vendor_id;
+	u16	 pcie_dev_id;
+	u16	 pcie_subsys_id;
+	u8	 img_type;
+	u16	 timestamp;
+	u8	 build_type;
+	u32	 build_num;
+	u8	 fw_build_prod;
+	u8	 fw_build_hw;
+	u8	 fw_build_step;
+	u8	 bt_spec_ver;
+	u16	 mfg_name;
+	u16	 hci_rev;
+	u16	 lmp_sub_ver;
+	u8	 otp_patch_ver;
+	u8	 secure_boot;
+	u8	 key_from_hdr;
+	u8	 otp_lock;
+	u8	 api_lock;
+	u8	 debug_lock;
+	u8	 min_fw_build_nn;
+	u8	 min_fw_build_cw;
+	u8	 min_fw_build_yy;
+	u8	 limited_cce;
+	u8	 sbe_type;
+	bdaddr_t otp_bd_addr;
+	u8	 unlocked_state;
+} __packed;
+
 struct intel_version {
 	u8 status;
 	u8 hw_platform;
@@ -70,6 +154,7 @@ struct btintel_version {
 	bool is_tlv_supported;
 	union {
 		struct intel_version intel_version; /* legacy version */
+		struct intel_version_tlv intel_version_tlv;
 	};
 } __packed;
 
-- 
2.17.1

Re: [PATCH v2 4/5] Bluetooth: btintel: Define tlv structure for new generation Controllers

[Marcel Holtmann]

Hi Kiran,

> Define structure used for reading controller information and
> to downloading firmware in tlv format used for new generation
> Intel controllers
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> 
> Changes in v2: None
> Changes in v1:
> - Add tlv structure definition
> 
> 
> drivers/bluetooth/btintel.h | 85 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 85 insertions(+)
> 
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index 0865d2d4aca7..20007da6b9bd 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -6,6 +6,90 @@
>  *  Copyright (C) 2015  Intel Corporation
>  */
> 
> +/* List of tlv type */
> +enum {
> +	INTEL_TLV_CNVI_TOP = 0x10,
> +	INTEL_TLV_CNVR_TOP,
> +	INTEL_TLV_CNVI_BT,
> +	INTEL_TLV_CNVR_BT,
> +	INTEL_TLV_CNVI_OTP,
> +	INTEL_TLV_CNVR_OTP,
> +	INTEL_TLV_DEV_REV_ID,
> +	INTEL_TLV_USB_VENDOR_ID,
> +	INTEL_TLV_USB_PRODUCT_ID,
> +	INTEL_TLV_PCIE_VENDOR_ID,
> +	INTEL_TLV_PCIE_DEVICE_ID,
> +	INTEL_TLV_PCIE_SUBSYSTEM_ID,
> +	INTEL_TLV_IMAGE_TYPE,
> +	INTEL_TLV_TIME_STAMP,
> +	INTEL_TLV_BUILD_TYPE,
> +	INTEL_TLV_BUILD_NUM,
> +	INTEL_TLV_FW_BUILD_PRODUCT,
> +	INTEL_TLV_FW_BUILD_HW,
> +	INTEL_TLV_FW_STEP,
> +	INTEL_TLV_BT_SPEC,
> +	INTEL_TLV_MFG_NAME,
> +	INTEL_TLV_HCI_REV,
> +	INTEL_TLV_LMP_SUBVER,
> +	INTEL_TLV_OTP_PATCH_VER,
> +	INTEL_TLV_SECURE_BOOT,
> +	INTEL_TLV_KEY_FROM_HDR,
> +	INTEL_TLV_OTP_LOCK,
> +	INTEL_TLV_API_LOCK,
> +	INTEL_TLV_DEBUG_LOCK,
> +	INTEL_TLV_MIN_FW,
> +	INTEL_TLV_LIMITED_CCE,
> +	INTEL_TLV_SBE_TYPE,
> +	INTEL_TLV_OTP_BDADDR,
> +	INTEL_TLV_UNLOCKED_STATE
> +};
> +
> +struct intel_tlv {
> +	u8 type;
> +	u8 len;
> +	u8 val[0];
> +} __packed;
> +
> +struct intel_version_tlv {
> +	u8	 status;
> +	u32	 cnvi_top;
> +	u32	 cnvr_top;
> +	u32	 cnvi_bt;
> +	u32	 cnvr_bt;
> +	u16	 cnvi_otp;
> +	u16	 cnvr_otp;
> +	u16	 dev_rev_id;
> +	u16	 usb_vid;
> +	u16	 usb_pid;
> +	u16	 pcie_vendor_id;
> +	u16	 pcie_dev_id;
> +	u16	 pcie_subsys_id;
> +	u8	 img_type;
> +	u16	 timestamp;
> +	u8	 build_type;
> +	u32	 build_num;
> +	u8	 fw_build_prod;
> +	u8	 fw_build_hw;
> +	u8	 fw_build_step;
> +	u8	 bt_spec_ver;
> +	u16	 mfg_name;
> +	u16	 hci_rev;
> +	u16	 lmp_sub_ver;
> +	u8	 otp_patch_ver;
> +	u8	 secure_boot;
> +	u8	 key_from_hdr;
> +	u8	 otp_lock;
> +	u8	 api_lock;
> +	u8	 debug_lock;
> +	u8	 min_fw_build_nn;
> +	u8	 min_fw_build_cw;
> +	u8	 min_fw_build_yy;
> +	u8	 limited_cce;
> +	u8	 sbe_type;
> +	bdaddr_t otp_bd_addr;
> +	u8	 unlocked_state;
> +} __packed;
> +

This is not required to be __packed. It is not a on-wire structure. In addition, I would just only include the data fields we currently require or want to show in dmesg.

Regards

Marcel

[PATCH v2 5/5] Bluetooth: btintel: Parse controller information present in TLV format

[Kiran K]

New generation Intel controllers returns controller information
in TLV format. Adding capability to parse and log it for debug purpose

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---

Changes in v2:
- Fix alignment for break statement
- Use get_unaligned_*
- Add empty line before goto label

 drivers/bluetooth/btintel.c | 144 ++++++++++++++++++++++++++++++++----
 drivers/bluetooth/btusb.c   |   4 +-
 2 files changed, 133 insertions(+), 15 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 2cb55a97598c..d71dcef58a89 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -209,27 +209,60 @@ void btintel_version_info(struct hci_dev *hdev, const struct btintel_version *ve
 {
 	const char *variant;
 	const struct intel_version *ver;
+	const struct intel_version_tlv *ver_tlv;
+
+	if (!version->is_tlv_supported) {
+		ver = &version->intel_version;
+
+		switch (ver->fw_variant) {
+		case 0x06:
+			variant = "Bootloader";
+			break;
+		case 0x23:
+			variant = "Firmware";
+			break;
+		default:
+			goto done;
+		}
 
-	if (version->is_tlv_supported)
-		return;
+		bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
+			    variant, ver->fw_revision >> 4,
+			    ver->fw_revision & 0x0f, ver->fw_build_num,
+			    ver->fw_build_ww, 2000 + ver->fw_build_yy);
+		goto done;
+	}
 
-	ver = &version->intel_version;
+	ver_tlv = &version->intel_version_tlv;
 
-	switch (ver->fw_variant) {
-	case 0x06:
+	switch (ver_tlv->img_type) {
+	case 0x01:
 		variant = "Bootloader";
+		bt_dev_info(hdev, "Device revision is %u", ver_tlv->dev_rev_id);
+		bt_dev_info(hdev, "Secure boot is %s",
+			    ver_tlv->secure_boot ? "enabled" : "disabled");
+		bt_dev_info(hdev, "OTP lock is %s",
+			    ver_tlv->otp_lock ? "enabled" : "disabled");
+		bt_dev_info(hdev, "API lock is %s",
+			    ver_tlv->api_lock ? "enabled" : "disabled");
+		bt_dev_info(hdev, "Debug lock is %s",
+			    ver_tlv->debug_lock ? "enabled" : "disabled");
+		bt_dev_info(hdev, "Minimum firmware build %u week %u %u",
+			    ver_tlv->min_fw_build_nn, ver_tlv->min_fw_build_cw,
+			    2000 + ver_tlv->min_fw_build_yy);
 		break;
-	case 0x23:
+	case 0x03:
 		variant = "Firmware";
 		break;
 	default:
-		return;
+		goto done;
 	}
 
-	bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
-		    variant, ver->fw_revision >> 4, ver->fw_revision & 0x0f,
-		    ver->fw_build_num, ver->fw_build_ww,
-		    2000 + ver->fw_build_yy);
+	bt_dev_info(hdev, "%s timestamp %u.%u buildtype %u build %u", variant,
+		    2000 + (ver_tlv->timestamp >> 8), ver_tlv->timestamp & 0xff,
+		    ver_tlv->build_type, ver_tlv->build_num);
+
+done:
+	return;
 }
 EXPORT_SYMBOL_GPL(btintel_version_info);
 
@@ -346,6 +379,8 @@ int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
 {
 	struct sk_buff *skb;
 	u8 *data, param, status, check_tlv;
+	struct intel_version_tlv *ver_tlv;
+	struct intel_tlv *tlv;
 
 	if (!version)
 		return -EINVAL;
@@ -373,9 +408,92 @@ int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
 	if (skb->len == sizeof(version->intel_version) && check_tlv == 0x37) {
 		memcpy(&version->intel_version, skb->data, sizeof(version->intel_version));
 		version->is_tlv_supported = false;
-	} else {
-		version->is_tlv_supported = true;
+		goto done;
 	}
+
+	bt_dev_info(hdev, "Supports tlv firmware download sequence");
+	version->is_tlv_supported = true;
+	ver_tlv = &version->intel_version_tlv;
+
+	/* Consume Command Complete Status field */
+	skb_pull(skb, 1);
+
+	/* Event parameters contatin multiple TLVs. Read each of them
+	 * and only keep the required data. Also, it use existing legacy
+	 * version field like hw_platform, hw_variant, and fw_variant
+	 * to keep the existing setup flow
+	 */
+	while (skb->len) {
+		tlv = (struct intel_tlv *)skb->data;
+		switch (tlv->type) {
+		case INTEL_TLV_CNVI_TOP:
+			ver_tlv->cnvi_top = get_unaligned_le32(tlv->val);
+			break;
+		case INTEL_TLV_CNVR_TOP:
+			ver_tlv->cnvr_top = get_unaligned_le32(tlv->val);
+			break;
+		case INTEL_TLV_CNVI_BT:
+			ver_tlv->cnvi_bt = get_unaligned_le32(tlv->val);
+			break;
+		case INTEL_TLV_CNVR_BT:
+			ver_tlv->cnvr_bt = get_unaligned_le32(tlv->val);
+			break;
+		case INTEL_TLV_USB_VENDOR_ID:
+			ver_tlv->usb_vid = get_unaligned_le16(tlv->val);
+			break;
+		case INTEL_TLV_USB_PRODUCT_ID:
+			ver_tlv->usb_pid = get_unaligned_le16(tlv->val);
+			break;
+		case INTEL_TLV_IMAGE_TYPE:
+			ver_tlv->img_type = tlv->val[0];
+			break;
+		case INTEL_TLV_TIME_STAMP:
+			ver_tlv->timestamp = get_unaligned_le16(tlv->val);
+			break;
+		case INTEL_TLV_BUILD_TYPE:
+			ver_tlv->build_type = tlv->val[0];
+			break;
+		case INTEL_TLV_BUILD_NUM:
+			ver_tlv->build_num = get_unaligned_le32(tlv->val);
+			break;
+		case INTEL_TLV_SECURE_BOOT:
+			ver_tlv->secure_boot = tlv->val[0];
+			break;
+		case INTEL_TLV_KEY_FROM_HDR:
+			ver_tlv->key_from_hdr = tlv->val[0];
+			break;
+		case INTEL_TLV_OTP_LOCK:
+			ver_tlv->otp_lock = tlv->val[0];
+			break;
+		case INTEL_TLV_API_LOCK:
+			ver_tlv->api_lock = tlv->val[0];
+			break;
+		case INTEL_TLV_DEBUG_LOCK:
+			ver_tlv->debug_lock = tlv->val[0];
+			break;
+		case INTEL_TLV_MIN_FW:
+			ver_tlv->min_fw_build_nn = tlv->val[0];
+			ver_tlv->min_fw_build_cw = tlv->val[1];
+			ver_tlv->min_fw_build_yy = tlv->val[2];
+			break;
+		case INTEL_TLV_LIMITED_CCE:
+			ver_tlv->limited_cce = tlv->val[0];
+			break;
+		case INTEL_TLV_SBE_TYPE:
+			ver_tlv->sbe_type = tlv->val[0];
+			break;
+		case INTEL_TLV_OTP_BDADDR:
+			memcpy(&ver_tlv->otp_bd_addr, tlv->val, tlv->len);
+			break;
+		default:
+			/* Ignore rest of information */
+			break;
+		}
+		/* consume the current tlv and move to next*/
+		skb_pull(skb, tlv->len + sizeof(*tlv));
+	}
+
+done:
 	kfree_skb(skb);
 	return 0;
 }
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index d06c946f7810..39f0e4522b06 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2519,13 +2519,13 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 		return err;
 	}
 
+	btintel_version_info(hdev, &version);
+
 	if (version.is_tlv_supported) {
 		bt_dev_err(hdev, "Firmware download in tlv format is not supported");
 		return -EOPNOTSUPP;
 	}
 
-	btintel_version_info(hdev, &version);
-
 	err = btusb_intel_download_firmware(hdev, &version, &params);
 	if (err)
 		return err;
-- 
2.17.1

Re: [PATCH v2 5/5] Bluetooth: btintel: Parse controller information present in TLV format

[Marcel Holtmann]

Hi Kiran,

> New generation Intel controllers returns controller information
> in TLV format. Adding capability to parse and log it for debug purpose
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> 
> Changes in v2:
> - Fix alignment for break statement
> - Use get_unaligned_*
> - Add empty line before goto label
> 
> drivers/bluetooth/btintel.c | 144 ++++++++++++++++++++++++++++++++----
> drivers/bluetooth/btusb.c   |   4 +-
> 2 files changed, 133 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 2cb55a97598c..d71dcef58a89 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -209,27 +209,60 @@ void btintel_version_info(struct hci_dev *hdev, const struct btintel_version *ve
> {
> 	const char *variant;
> 	const struct intel_version *ver;
> +	const struct intel_version_tlv *ver_tlv;
> +
> +	if (!version->is_tlv_supported) {
> +		ver = &version->intel_version;
> +
> +		switch (ver->fw_variant) {
> +		case 0x06:
> +			variant = "Bootloader";
> +			break;
> +		case 0x23:
> +			variant = "Firmware";
> +			break;
> +		default:
> +			goto done;
> +		}
> 
> -	if (version->is_tlv_supported)
> -		return;
> +		bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
> +			    variant, ver->fw_revision >> 4,
> +			    ver->fw_revision & 0x0f, ver->fw_build_num,
> +			    ver->fw_build_ww, 2000 + ver->fw_build_yy);
> +		goto done;
> +	}
> 
> -	ver = &version->intel_version;
> +	ver_tlv = &version->intel_version_tlv;
> 
> -	switch (ver->fw_variant) {
> -	case 0x06:
> +	switch (ver_tlv->img_type) {
> +	case 0x01:
> 		variant = "Bootloader";
> +		bt_dev_info(hdev, "Device revision is %u", ver_tlv->dev_rev_id);
> +		bt_dev_info(hdev, "Secure boot is %s",
> +			    ver_tlv->secure_boot ? "enabled" : "disabled");
> +		bt_dev_info(hdev, "OTP lock is %s",
> +			    ver_tlv->otp_lock ? "enabled" : "disabled");
> +		bt_dev_info(hdev, "API lock is %s",
> +			    ver_tlv->api_lock ? "enabled" : "disabled");
> +		bt_dev_info(hdev, "Debug lock is %s",
> +			    ver_tlv->debug_lock ? "enabled" : "disabled");
> +		bt_dev_info(hdev, "Minimum firmware build %u week %u %u",
> +			    ver_tlv->min_fw_build_nn, ver_tlv->min_fw_build_cw,
> +			    2000 + ver_tlv->min_fw_build_yy);
> 		break;

since we have everything stored in two independent data structures now, I have the feeling it is best to actually re-arrange the code around so that we gather all information we need. And once we are done, we handle the required print outs and logic in a common place. Duplicating these print outs now is not going to help since it creates hard to read code and if we have bugs we have to fix them in two places now.

> -	case 0x23:
> +	case 0x03:
> 		variant = "Firmware";
> 		break;
> 	default:
> -		return;
> +		goto done;
> 	}
> 
> -	bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
> -		    variant, ver->fw_revision >> 4, ver->fw_revision & 0x0f,
> -		    ver->fw_build_num, ver->fw_build_ww,
> -		    2000 + ver->fw_build_yy);
> +	bt_dev_info(hdev, "%s timestamp %u.%u buildtype %u build %u", variant,
> +		    2000 + (ver_tlv->timestamp >> 8), ver_tlv->timestamp & 0xff,
> +		    ver_tlv->build_type, ver_tlv->build_num);
> +
> +done:
> +	return;
> }
> EXPORT_SYMBOL_GPL(btintel_version_info);
> 
> @@ -346,6 +379,8 @@ int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
> {
> 	struct sk_buff *skb;
> 	u8 *data, param, status, check_tlv;
> +	struct intel_version_tlv *ver_tlv;
> +	struct intel_tlv *tlv;
> 
> 	if (!version)
> 		return -EINVAL;
> @@ -373,9 +408,92 @@ int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
> 	if (skb->len == sizeof(version->intel_version) && check_tlv == 0x37) {
> 		memcpy(&version->intel_version, skb->data, sizeof(version->intel_version));
> 		version->is_tlv_supported = false;
> -	} else {
> -		version->is_tlv_supported = true;
> +		goto done;
> 	}
> +
> +	bt_dev_info(hdev, "Supports tlv firmware download sequence");
> +	version->is_tlv_supported = true;
> +	ver_tlv = &version->intel_version_tlv;
> +
> +	/* Consume Command Complete Status field */
> +	skb_pull(skb, 1);
> +
> +	/* Event parameters contatin multiple TLVs. Read each of them
> +	 * and only keep the required data. Also, it use existing legacy
> +	 * version field like hw_platform, hw_variant, and fw_variant
> +	 * to keep the existing setup flow
> +	 */
> +	while (skb->len) {

Keep the scope of variables as local as possible.

		struct intel_tlv *tlv

> +		tlv = (struct intel_tlv *)skb->data;
> +		switch (tlv->type) {
> +		case INTEL_TLV_CNVI_TOP:
> +			ver_tlv->cnvi_top = get_unaligned_le32(tlv->val);
> +			break;
> +		case INTEL_TLV_CNVR_TOP:
> +			ver_tlv->cnvr_top = get_unaligned_le32(tlv->val);
> +			break;
> +		case INTEL_TLV_CNVI_BT:
> +			ver_tlv->cnvi_bt = get_unaligned_le32(tlv->val);
> +			break;
> +		case INTEL_TLV_CNVR_BT:
> +			ver_tlv->cnvr_bt = get_unaligned_le32(tlv->val);
> +			break;
> +		case INTEL_TLV_USB_VENDOR_ID:
> +			ver_tlv->usb_vid = get_unaligned_le16(tlv->val);
> +			break;
> +		case INTEL_TLV_USB_PRODUCT_ID:
> +			ver_tlv->usb_pid = get_unaligned_le16(tlv->val);
> +			break;
> +		case INTEL_TLV_IMAGE_TYPE:
> +			ver_tlv->img_type = tlv->val[0];
> +			break;
> +		case INTEL_TLV_TIME_STAMP:
> +			ver_tlv->timestamp = get_unaligned_le16(tlv->val);
> +			break;
> +		case INTEL_TLV_BUILD_TYPE:
> +			ver_tlv->build_type = tlv->val[0];
> +			break;
> +		case INTEL_TLV_BUILD_NUM:
> +			ver_tlv->build_num = get_unaligned_le32(tlv->val);
> +			break;
> +		case INTEL_TLV_SECURE_BOOT:
> +			ver_tlv->secure_boot = tlv->val[0];
> +			break;
> +		case INTEL_TLV_KEY_FROM_HDR:
> +			ver_tlv->key_from_hdr = tlv->val[0];
> +			break;
> +		case INTEL_TLV_OTP_LOCK:
> +			ver_tlv->otp_lock = tlv->val[0];
> +			break;
> +		case INTEL_TLV_API_LOCK:
> +			ver_tlv->api_lock = tlv->val[0];
> +			break;
> +		case INTEL_TLV_DEBUG_LOCK:
> +			ver_tlv->debug_lock = tlv->val[0];
> +			break;
> +		case INTEL_TLV_MIN_FW:
> +			ver_tlv->min_fw_build_nn = tlv->val[0];
> +			ver_tlv->min_fw_build_cw = tlv->val[1];
> +			ver_tlv->min_fw_build_yy = tlv->val[2];
> +			break;
> +		case INTEL_TLV_LIMITED_CCE:
> +			ver_tlv->limited_cce = tlv->val[0];
> +			break;
> +		case INTEL_TLV_SBE_TYPE:
> +			ver_tlv->sbe_type = tlv->val[0];
> +			break;
> +		case INTEL_TLV_OTP_BDADDR:
> +			memcpy(&ver_tlv->otp_bd_addr, tlv->val, tlv->len);
> +			break;
> +		default:
> +			/* Ignore rest of information */
> +			break;
> +		}
> +		/* consume the current tlv and move to next*/
> +		skb_pull(skb, tlv->len + sizeof(*tlv));
> +	}
> +
> +done:
> 	kfree_skb(skb);
> 	return 0;
> }
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index d06c946f7810..39f0e4522b06 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2519,13 +2519,13 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> 		return err;
> 	}
> 
> +	btintel_version_info(hdev, &version);
> +
> 	if (version.is_tlv_supported) {
> 		bt_dev_err(hdev, "Firmware download in tlv format is not supported");
> 		return -EOPNOTSUPP;
> 	}
> 
> -	btintel_version_info(hdev, &version);
> -
> 	err = btusb_intel_download_firmware(hdev, &version, &params);
> 	if (err)
> 		return err;

Regards

Marcel

Re: [PATCH v2 1/5] Bluetooth: btintel: Make controller version read generic

[Marcel Holtmann]

Hi Kiran,

> Make controller read vesion function more generic to support different
> type of controllers.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> 
> Changes in v2: None
> Changes in v1:
> - Make controller read version function generic
> 
> drivers/bluetooth/btintel.c   | 36 ++++++++++++++----
> drivers/bluetooth/btintel.h   | 15 ++++++--
> drivers/bluetooth/btusb.c     | 71 +++++++++++++++++++++++------------
> drivers/bluetooth/hci_ag6xx.c | 12 +++++-
> drivers/bluetooth/hci_intel.c | 12 +++++-
> 5 files changed, 106 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 5fa5be3c5598..dea96c585ecb 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -204,9 +204,15 @@ void btintel_hw_error(struct hci_dev *hdev, u8 code)
> }
> EXPORT_SYMBOL_GPL(btintel_hw_error);
> 
> -void btintel_version_info(struct hci_dev *hdev, struct intel_version *ver)
> +void btintel_version_info(struct hci_dev *hdev, const struct btintel_version *version)
> {

so I am not fully on board with the new btintel_version struct and using it as union. Maybe it is better the function just returns allocated memory and a type indicator. I need to let this stew a little bit.

> 	const char *variant;
> +	const struct intel_version *ver;
> +
> +	if (version->is_tlv_supported)
> +		return;
> +
> +	ver = &version->intel_version;
> 
> 	switch (ver->fw_variant) {
> 	case 0x06:
> @@ -335,27 +341,41 @@ int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug)
> }
> EXPORT_SYMBOL_GPL(btintel_set_event_mask_mfg);
> 
> -int btintel_read_version(struct hci_dev *hdev, struct intel_version *ver)
> +int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
> {
> 	struct sk_buff *skb;

	const u8 param[1] = 0xff;

> +	u8 *data, param, status, check_tlv;
> +
> +	if (!version)
> +		return -EINVAL;
> 
> -	skb = __hci_cmd_sync(hdev, 0xfc05, 0, NULL, HCI_CMD_TIMEOUT);
> +	param = 0xFF;
> +
> +	skb = __hci_cmd_sync(hdev, 0xfc05, 1, &param, HCI_CMD_TIMEOUT);
> 	if (IS_ERR(skb)) {
> 		bt_dev_err(hdev, "Reading Intel version information failed (%ld)",
> 			   PTR_ERR(skb));
> 		return PTR_ERR(skb);
> 	}
> 
> -	if (skb->len != sizeof(*ver)) {
> -		bt_dev_err(hdev, "Intel version event size mismatch");
> +	data = skb->data;
> +	status = *data;
> +	if (status) {
> +		bt_dev_err(hdev, "Intel Read Version command failed (%02x)",
> +			   status);
> 		kfree_skb(skb);
> -		return -EILSEQ;
> +		return -bt_to_errno(status);
> 	}
> 
> -	memcpy(ver, skb->data, sizeof(*ver));
> +	check_tlv = *(data + 1);

Lets not introduce the data variable since it is pointless. And even check_tlv variable seems not really needed.

> 
> +	if (skb->len == sizeof(version->intel_version) && check_tlv == 0x37) {
> +		memcpy(&version->intel_version, skb->data, sizeof(version->intel_version));
> +		version->is_tlv_supported = false;
> +	} else {
> +		version->is_tlv_supported = true;
> +	}
> 	kfree_skb(skb);
> -
> 	return 0;
> }
> EXPORT_SYMBOL_GPL(btintel_read_version);
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index 08e20606fb58..0865d2d4aca7 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -66,6 +66,13 @@ struct intel_debug_features {
> 	__u8    page1[16];
> } __packed;
> 
> +struct btintel_version {
> +	bool is_tlv_supported;
> +	union {
> +		struct intel_version intel_version; /* legacy version */
> +	};
> +} __packed;
> +
> #if IS_ENABLED(CONFIG_BT_INTEL)
> 
> int btintel_check_bdaddr(struct hci_dev *hdev);
> @@ -76,13 +83,13 @@ int btintel_set_diag(struct hci_dev *hdev, bool enable);
> int btintel_set_diag_mfg(struct hci_dev *hdev, bool enable);
> void btintel_hw_error(struct hci_dev *hdev, u8 code);
> 
> -void btintel_version_info(struct hci_dev *hdev, struct intel_version *ver);
> +void btintel_version_info(struct hci_dev *hdev, const struct btintel_version *version);

This might be better done like this:

void btintel_version_info(struct hci_dev *hdev, bool tlv_format, void *buf, u8 len);

Depending on from where this is called and if we have existing knowledge of the format, we can keep the current one and just add btintel_version_tlv(..

I do like the latter better, but I would have to look at the whole call-chain first.

> int btintel_secure_send(struct hci_dev *hdev, u8 fragment_type, u32 plen,
> 			const void *param);
> int btintel_load_ddc_config(struct hci_dev *hdev, const char *ddc_name);
> int btintel_set_event_mask(struct hci_dev *hdev, bool debug);
> int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug);
> -int btintel_read_version(struct hci_dev *hdev, struct intel_version *ver);
> +int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version);

int btintel_read_version(struct hci_dev *hdev, u8 *buf, u8 *len, bool *tlv_format);

> struct regmap *btintel_regmap_init(struct hci_dev *hdev, u16 opcode_read,
> 				   u16 opcode_write);
> @@ -133,7 +140,7 @@ static inline void btintel_hw_error(struct hci_dev *hdev, u8 code)
> }
> 
> static inline void btintel_version_info(struct hci_dev *hdev,
> -					struct intel_version *ver)
> +					struct btintel_version *version)
> {
> }
> 
> @@ -160,7 +167,7 @@ static inline int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug)
> }
> 
> static inline int btintel_read_version(struct hci_dev *hdev,
> -				       struct intel_version *ver)
> +				       struct btintel_version *version)
> {
> 	return -EOPNOTSUPP;
> }
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index faa863dd5d0a..d06c946f7810 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1938,6 +1938,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
> 	const u8 *fw_ptr;
> 	int disable_patch, err;
> 	struct intel_version ver;
> +	struct btintel_version version;
> 
> 	BT_DBG("%s", hdev->name);
> 
> @@ -1963,10 +1964,16 @@ static int btusb_setup_intel(struct hci_dev *hdev)
> 	 * The returned information are hardware variant and revision plus
> 	 * firmware variant, revision and build number.
> 	 */
> -	err = btintel_read_version(hdev, &ver);
> +	err = btintel_read_version(hdev, &version);
> 	if (err)
> 		return err;
> 
> +	if (version.is_tlv_supported) {
> +		bt_dev_err(hdev, "FW download in tlv format not supported");
> +		return -EOPNOTSUPP;
> +	}
> +	ver = version.intel_version;
> +
> 	bt_dev_info(hdev, "read Intel version: %02x%02x%02x%02x%02x%02x%02x%02x%02x",
> 		    ver.hw_platform, ver.hw_variant, ver.hw_revision,
> 		    ver.fw_variant,  ver.fw_revision, ver.fw_build_num,
> @@ -2049,11 +2056,11 @@ static int btusb_setup_intel(struct hci_dev *hdev)
> 	/* Need build number for downloaded fw patches in
> 	 * every power-on boot
> 	 */
> -       err = btintel_read_version(hdev, &ver);
> -       if (err)
> -               return err;
> -       bt_dev_info(hdev, "Intel BT fw patch 0x%02x completed & activated",
> -		   ver.fw_patch_num);
> +	err = btintel_read_version(hdev, &version);
> +	if (err)
> +		return err;
> +	bt_dev_info(hdev, "Intel BT fw patch 0x%02x completed & activated",
> +		    version.intel_version.fw_patch_num);
> 
> 	goto complete;
> 
> @@ -2251,11 +2258,18 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> 	return -EILSEQ;
> }
> 
> -static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
> -					     struct intel_boot_params *params,
> -					     char *fw_name, size_t len,
> -					     const char *suffix)
> +static bool btusb_setup_intel_new_get_fw_name(const struct btintel_version *version,
> +					      struct intel_boot_params *params,
> +					      char *fw_name, size_t len,
> +					      const char *suffix)

So either we convert the required fields out of the TLV into intel_version or we might have provide them as parameters or an actual separate struct.

> {
> +	const struct intel_version *ver;
> +
> +	if (version->is_tlv_supported)
> +		return false;
> +
> +	ver = &version->intel_version;
> +
> 	switch (ver->hw_variant) {
> 	case 0x0b:	/* SfP */
> 	case 0x0c:	/* WsP */
> @@ -2281,18 +2295,21 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
> }
> 
> static int btusb_intel_download_firmware(struct hci_dev *hdev,
> -					 struct intel_version *ver,
> +					 struct btintel_version *version,
> 					 struct intel_boot_params *params)
> {
> 	const struct firmware *fw;
> 	u32 boot_param;
> 	char fwname[64];
> 	int err;
> +	const struct intel_version *ver;
> 	struct btusb_data *data = hci_get_drvdata(hdev);
> 
> -	if (!ver || !params)
> +	if (!version || !params)
> 		return -EINVAL;
> 
> +	ver = &version->intel_version;
> +
> 	/* The hardware platform number has a fixed value of 0x37 and
> 	 * for now only accept this single value.
> 	 */
> @@ -2322,8 +2339,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
> 		return -EINVAL;
> 	}
> 
> -	btintel_version_info(hdev, ver);
> -

I don’t like just removing this here. We add the print outs for a reason.

> 	/* The firmware variant determines if the device is in bootloader
> 	 * mode or is running operational firmware. The value 0x06 identifies
> 	 * the bootloader and the value 0x23 identifies the operational
> @@ -2398,7 +2413,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
> 	 * ibt-<hw_variant>-<hw_revision>-<fw_revision>.sfi.
> 	 *
> 	 */
> -	err = btusb_setup_intel_new_get_fw_name(ver, params, fwname,
> +	err = btusb_setup_intel_new_get_fw_name(version, params, fwname,
> 						sizeof(fwname), "sfi");
> 	if (!err) {
> 		bt_dev_err(hdev, "Unsupported Intel firmware naming");
> @@ -2483,6 +2498,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> 	unsigned long long duration;
> 	int err;
> 	struct intel_debug_features features;
> +	struct btintel_version version;
> 
> 	BT_DBG("%s", hdev->name);
> 
> @@ -2494,21 +2510,28 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> 
> 	calltime = ktime_get();
> 
> -	/* Read the Intel version information to determine if the device
> -	 * is in bootloader mode or if it already has operational firmware
> -	 * loaded.
> +	/* Read controller version information and support of tlv format
> 	 */

I would leave the comment as is.

> -	err = btintel_read_version(hdev, &ver);
> +	err = btintel_read_version(hdev, &version);
> 	if (err) {
> -		bt_dev_err(hdev, "Intel Read version failed (%d)", err);
> +		bt_dev_err(hdev, "Intel Read version new failed (%d)", err);

Don’t add random “new” parts in the existing messages.

> 		btintel_reset_to_bootloader(hdev);
> 		return err;
> 	}
> 
> -	err = btusb_intel_download_firmware(hdev, &ver, &params);
> +	if (version.is_tlv_supported) {
> +		bt_dev_err(hdev, "Firmware download in tlv format is not supported");
> +		return -EOPNOTSUPP;
> +	}

Do we need this twice?

> +
> +	btintel_version_info(hdev, &version);
> +
> +	err = btusb_intel_download_firmware(hdev, &version, &params);
> 	if (err)
> 		return err;
> 
> +	ver = version.intel_version;
> +
> 	/* controller is already having an operational firmware */
> 	if (ver.fw_variant == 0x23)
> 		goto finish;
> @@ -2562,7 +2585,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> 
> 	clear_bit(BTUSB_BOOTLOADER, &data->flags);
> 
> -	err = btusb_setup_intel_new_get_fw_name(&ver, &params, ddcname,
> +	err = btusb_setup_intel_new_get_fw_name(&version, &params, ddcname,
> 						sizeof(ddcname), "ddc");
> 
> 	if (!err) {
> @@ -2586,11 +2609,11 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> 	btintel_set_debug_features(hdev, &features);
> 
> 	/* Read the Intel version information after loading the FW  */
> -	err = btintel_read_version(hdev, &ver);
> +	err = btintel_read_version(hdev, &version);
> 	if (err)
> 		return err;
> 
> -	btintel_version_info(hdev, &ver);
> +	btintel_version_info(hdev, &version);
> 
> finish:
> 	/* All Intel controllers that support the Microsoft vendor
> diff --git a/drivers/bluetooth/hci_ag6xx.c b/drivers/bluetooth/hci_ag6xx.c
> index 1f55df93e4ce..6f6a1e061972 100644
> --- a/drivers/bluetooth/hci_ag6xx.c
> +++ b/drivers/bluetooth/hci_ag6xx.c
> @@ -153,6 +153,7 @@ static int ag6xx_setup(struct hci_uart *hu)
> 	struct hci_dev *hdev = hu->hdev;
> 	struct sk_buff *skb;
> 	struct intel_version ver;
> +	struct btintel_version version;
> 	const struct firmware *fw;
> 	const u8 *fw_ptr;
> 	char fwname[64];
> @@ -166,11 +167,18 @@ static int ag6xx_setup(struct hci_uart *hu)
> 	if (err)
> 		return err;
> 
> -	err = btintel_read_version(hdev, &ver);
> +	err = btintel_read_version(hdev, &version);
> 	if (err)
> 		return err;
> 
> -	btintel_version_info(hdev, &ver);
> +	if (version.is_tlv_supported) {
> +		bt_dev_err(hdev, "Firmware download in tlv format over ag6xx is not supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	btintel_version_info(hdev, &version);
> +
> +	ver = version.intel_version;
> 
> 	/* The hardware platform number has a fixed value of 0x37 and
> 	 * for now only accept this single value.
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index f1299da6eed8..f30cbc66d48f 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -532,6 +532,7 @@ static int intel_setup(struct hci_uart *hu)
> 	struct hci_dev *hdev = hu->hdev;
> 	struct sk_buff *skb;
> 	struct intel_version ver;
> +	struct btintel_version version;
> 	struct intel_boot_params params;
> 	struct list_head *p;
> 	const struct firmware *fw;
> @@ -584,10 +585,17 @@ static int intel_setup(struct hci_uart *hu)
> 	 * is in bootloader mode or if it already has operational firmware
> 	 * loaded.
> 	 */
> -	err = btintel_read_version(hdev, &ver);
> +	err = btintel_read_version(hdev, &version);
> 	if (err)
> 		return err;
> 
> +	if (version.is_tlv_supported) {
> +		/* firmware download in tlv format is not supported on UART transport */
> +		bt_dev_err(hdev, "Firmware download in tlv format is not supported");
> +		return -EOPNOTSUPP;
> +	}

So now I wonder if we really want to send that 0xff even on UART. So maybe btintel_read_version needs to get an extra bool parameter for requesting TLV or not.

> +	ver = version.intel_version;
> +
> 	/* The hardware platform number has a fixed value of 0x37 and
> 	 * for now only accept this single value.
> 	 */
> @@ -614,7 +622,7 @@ static int intel_setup(struct hci_uart *hu)
> 		return -EINVAL;
> 	}
> 
> -	btintel_version_info(hdev, &ver);
> +	btintel_version_info(hdev, &version);

I really think now that btintel_version_info and btintel_version_info_tlv are needed. If we don’t have this on UART, then we should try to change the UART code as less as possible.

Or we first need to unify the UART and USB code a lot more into btintel.c so that it is all handled there.

> 
> 	/* The firmware variant determines if the device is in bootloader
> 	 * mode or is running operational firmware. The value 0x06 identifies

Regards

Marcel