[PATCH v3 1/6] Bluetooth: btintel: Check firmware version before download

[PATCH v3 1/6] Bluetooth: btintel: Check firmware version before download

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This checks the firmware build number, week and year matches with
repective version loaded and then skip the download process.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
v2: Add patch that mover checks for operational mode after the version
checking.
v3: Fix not checking for operation mode before using btintel_read_boot_params
since some models depend on that to contruct the fw filename. Also attempt to
cleanup duplicated code.

 drivers/bluetooth/btintel.c   | 94 +++++++++++++++++++++++++++--------
 drivers/bluetooth/btintel.h   |  5 +-
 drivers/bluetooth/btusb.c     | 16 +++++-
 drivers/bluetooth/hci_intel.c |  7 ++-
 4 files changed, 96 insertions(+), 26 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 88ce5f0ffc4b..153989bd8d5f 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -24,6 +24,14 @@
 #define ECDSA_OFFSET		644
 #define ECDSA_HEADER_LEN	320
 
+#define CMD_WRITE_BOOT_PARAMS	0xfc0e
+struct cmd_write_boot_params {
+	u32 boot_addr;
+	u8  fw_build_num;
+	u8  fw_build_ww;
+	u8  fw_build_yy;
+} __packed;
+
 int btintel_check_bdaddr(struct hci_dev *hdev)
 {
 	struct hci_rp_read_bd_addr *bda;
@@ -841,7 +849,7 @@ static int btintel_sfi_ecdsa_header_secure_send(struct hci_dev *hdev,
 
 static int btintel_download_firmware_payload(struct hci_dev *hdev,
 					     const struct firmware *fw,
-					     u32 *boot_param, size_t offset)
+					     size_t offset)
 {
 	int err;
 	const u8 *fw_ptr;
@@ -854,20 +862,6 @@ static int btintel_download_firmware_payload(struct hci_dev *hdev,
 	while (fw_ptr - fw->data < fw->size) {
 		struct hci_command_hdr *cmd = (void *)(fw_ptr + frag_len);
 
-		/* Each SKU has a different reset parameter to use in the
-		 * HCI_Intel_Reset command and it is embedded in the firmware
-		 * data. So, instead of using static value per SKU, check
-		 * the firmware data and save it for later use.
-		 */
-		if (le16_to_cpu(cmd->opcode) == 0xfc0e) {
-			/* The boot parameter is the first 32-bit value
-			 * and rest of 3 octets are reserved.
-			 */
-			*boot_param = get_unaligned_le32(fw_ptr + sizeof(*cmd));
-
-			bt_dev_dbg(hdev, "boot_param=0x%x", *boot_param);
-		}
-
 		frag_len += sizeof(*cmd) + cmd->plen;
 
 		/* The parameter length of the secure send command requires
@@ -896,28 +890,90 @@ static int btintel_download_firmware_payload(struct hci_dev *hdev,
 	return err;
 }
 
+static bool btintel_firmware_version(struct hci_dev *hdev,
+				     u8 num, u8 ww, u8 yy,
+				     const struct firmware *fw,
+				     u32 *boot_addr)
+{
+	const u8 *fw_ptr;
+	u32 frag_len;
+
+	fw_ptr = fw->data;
+	frag_len = 0;
+
+	while (fw_ptr - fw->data < fw->size) {
+		struct hci_command_hdr *cmd = (void *)(fw_ptr + frag_len);
+
+		/* Each SKU has a different reset parameter to use in the
+		 * HCI_Intel_Reset command and it is embedded in the firmware
+		 * data. So, instead of using static value per SKU, check
+		 * the firmware data and save it for later use.
+		 */
+		if (le16_to_cpu(cmd->opcode) == CMD_WRITE_BOOT_PARAMS) {
+			struct cmd_write_boot_params *params;
+
+			params = (void *)(fw_ptr + sizeof(*cmd));
+
+			bt_dev_dbg(hdev, "Boot Address: 0x%x",
+				   le32_to_cpu(params->boot_addr));
+
+			bt_dev_dbg(hdev, "Firmware Version: %u-%u.%u",
+				   params->fw_build_num, params->fw_build_ww,
+				   params->fw_build_yy);
+
+			return (num == params->fw_build_num &&
+				ww == params->fw_build_ww &&
+				yy == params->fw_build_yy);
+		}
+
+		frag_len += sizeof(*cmd) + cmd->plen;
+	}
+
+	return false;
+}
+
 int btintel_download_firmware(struct hci_dev *hdev,
+			      struct intel_version *ver,
 			      const struct firmware *fw,
 			      u32 *boot_param)
 {
 	int err;
 
+	/* Skip download if firmware has the same version */
+	if (btintel_firmware_version(hdev, ver->fw_build_num, ver->fw_build_ww,
+				     ver->fw_build_yy, fw, boot_param)) {
+		/* Return -EALREADY to indicate that the firmware has already
+		 * been loaded.
+		 */
+		return -EALREADY;
+	}
+
 	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);
+	return btintel_download_firmware_payload(hdev, fw, RSA_HEADER_LEN);
 }
 EXPORT_SYMBOL_GPL(btintel_download_firmware);
 
 int btintel_download_firmware_newgen(struct hci_dev *hdev,
+				     struct intel_version_tlv *ver,
 				     const struct firmware *fw, u32 *boot_param,
 				     u8 hw_variant, u8 sbe_type)
 {
 	int err;
 	u32 css_header_ver;
 
+	/* Skip download if firmware has the same version */
+	if (btintel_firmware_version(hdev, ver->min_fw_build_nn,
+				     ver->min_fw_build_cw, ver->min_fw_build_yy,
+				     fw, boot_param)) {
+		/* Return -EALREADY to indicate that firmware has already been
+		 * loaded.
+		 */
+		return -EALREADY;
+	}
+
 	/* iBT hardware variants 0x0b, 0x0c, 0x11, 0x12, 0x13, 0x14 support
 	 * only RSA secure boot engine. Hence, the corresponding sfi file will
 	 * have RSA header of 644 bytes followed by Command Buffer.
@@ -947,7 +1003,7 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev,
 		if (err)
 			return err;
 
-		err = btintel_download_firmware_payload(hdev, fw, boot_param, RSA_HEADER_LEN);
+		err = btintel_download_firmware_payload(hdev, fw, RSA_HEADER_LEN);
 		if (err)
 			return err;
 	} else if (hw_variant >= 0x17) {
@@ -968,7 +1024,6 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev,
 				return err;
 
 			err = btintel_download_firmware_payload(hdev, fw,
-								boot_param,
 								RSA_HEADER_LEN + ECDSA_HEADER_LEN);
 			if (err)
 				return err;
@@ -978,7 +1033,6 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev,
 				return err;
 
 			err = btintel_download_firmware_payload(hdev, fw,
-								boot_param,
 								RSA_HEADER_LEN + ECDSA_HEADER_LEN);
 			if (err)
 				return err;
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index 6511b091caf5..51f1f2c883b4 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -163,9 +163,10 @@ struct regmap *btintel_regmap_init(struct hci_dev *hdev, u16 opcode_read,
 int btintel_send_intel_reset(struct hci_dev *hdev, u32 boot_param);
 int btintel_read_boot_params(struct hci_dev *hdev,
 			     struct intel_boot_params *params);
-int btintel_download_firmware(struct hci_dev *dev, const struct firmware *fw,
-			      u32 *boot_param);
+int btintel_download_firmware(struct hci_dev *dev, struct intel_version *ver,
+			      const struct firmware *fw, u32 *boot_param);
 int btintel_download_firmware_newgen(struct hci_dev *hdev,
+				     struct intel_version_tlv *ver,
 				     const struct firmware *fw,
 				     u32 *boot_param, u8 hw_variant,
 				     u8 sbe_type);
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 66ada8217797..e896c6702d60 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2623,10 +2623,16 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
 	set_bit(BTUSB_DOWNLOADING, &data->flags);
 
 	/* Start firmware downloading and get boot parameter */
-	err = btintel_download_firmware_newgen(hdev, fw, boot_param,
+	err = btintel_download_firmware_newgen(hdev, ver, fw, boot_param,
 					       INTEL_HW_VARIANT(ver->cnvi_bt),
 					       ver->sbe_type);
 	if (err < 0) {
+		if (err == -EALREADY) {
+			/* Firmware has already been loaded */
+			set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);
+			goto done;
+		}
+
 		/* When FW download fails, send Intel Reset to retry
 		 * FW download.
 		 */
@@ -2817,8 +2823,14 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 	set_bit(BTUSB_DOWNLOADING, &data->flags);
 
 	/* Start firmware downloading and get boot parameter */
-	err = btintel_download_firmware(hdev, fw, boot_param);
+	err = btintel_download_firmware(hdev, ver, fw, boot_param);
 	if (err < 0) {
+		if (err == -EALREADY) {
+			/* Firmware has already been loaded */
+			set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);
+			goto done;
+		}
+
 		/* When FW download fails, send Intel Reset to retry
 		 * FW download.
 		 */
diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index b20a40fab83e..7249b91d9b91 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -735,7 +735,7 @@ static int intel_setup(struct hci_uart *hu)
 	set_bit(STATE_DOWNLOADING, &intel->flags);
 
 	/* Start firmware downloading and get boot parameter */
-	err = btintel_download_firmware(hdev, fw, &boot_param);
+	err = btintel_download_firmware(hdev, &ver, fw, &boot_param);
 	if (err < 0)
 		goto done;
 
@@ -784,7 +784,10 @@ static int intel_setup(struct hci_uart *hu)
 done:
 	release_firmware(fw);
 
-	if (err < 0)
+	/* Check if there was an error and if is not -EALREADY which means the
+	 * firmware has already been loaded.
+	 */
+	if (err < 0 && err != -EALREADY)
 		return err;
 
 	/* We need to restore the default speed before Intel reset */
-- 
2.26.2

[PATCH v3 2/6] Bluetooth: btintel: Move operational checks after version check

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

In order to allow new firmware to be loaded it first needs to check if
the firmware version on file matches the one loaded if it doesn't then
it needs to revert to boorloader mode in order to load the new firmware.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 drivers/bluetooth/btintel.c | 22 +++++++++++
 drivers/bluetooth/btusb.c   | 74 +++++++++++++++----------------------
 2 files changed, 52 insertions(+), 44 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 153989bd8d5f..ccab05f67df9 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -948,6 +948,17 @@ int btintel_download_firmware(struct hci_dev *hdev,
 		return -EALREADY;
 	}
 
+	/* 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
+	 * firmware.
+	 *
+	 * If the firmware version has changed that means it needs to be reset
+	 * to bootloader when operational so the new firmware can be loaded.
+	 */
+	if (ver->fw_variant == 0x23)
+		return -EINVAL;
+
 	err = btintel_sfi_rsa_header_secure_send(hdev, fw);
 	if (err)
 		return err;
@@ -974,6 +985,17 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev,
 		return -EALREADY;
 	}
 
+	/* The firmware variant determines if the device is in bootloader
+	 * mode or is running operational firmware. The value 0x03 identifies
+	 * the bootloader and the value 0x23 identifies the operational
+	 * firmware.
+	 *
+	 * If the firmware version has changed that means it needs to be reset
+	 * to bootloader when operational so the new firmware can be loaded.
+	 */
+	if (ver->img_type == 0x03)
+		return -EINVAL;
+
 	/* iBT hardware variants 0x0b, 0x0c, 0x11, 0x12, 0x13, 0x14 support
 	 * only RSA secure boot engine. Hence, the corresponding sfi file will
 	 * have RSA header of 644 bytes followed by Command Buffer.
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index e896c6702d60..113ff780232f 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2469,14 +2469,30 @@ 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,
+static int 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)
 {
+	/* The hardware platform number has a fixed value of 0x37 and
+	 * for now only accept this single value.
+	 */
+	if (ver->hw_platform != 0x37)
+		return -EINVAL;
+
 	switch (ver->hw_variant) {
 	case 0x0b:	/* SfP */
 	case 0x0c:	/* WsP */
+		/* The firmware variant determines if the device is in
+		 * bootloader mode or is running operational firmware.
+		 *
+		 * Version checking cannot be performed in these models since
+		 * the firmware versioning depends on the firmware being in
+		 * bootloader mode.
+		 */
+		if (ver->fw_variant == 0x23)
+			return -EALREADY;
+
 		snprintf(fw_name, len, "intel/ibt-%u-%u.%s",
 			le16_to_cpu(ver->hw_variant),
 			le16_to_cpu(params->dev_revid),
@@ -2493,9 +2509,10 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
 			suffix);
 		break;
 	default:
-		return false;
+		return -EINVAL;
 	}
-	return true;
+
+	return 0;
 }
 
 static void btusb_setup_intel_newgen_get_fw_name(const struct intel_version_tlv *ver_tlv,
@@ -2550,7 +2567,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
 	if (ver->img_type == 0x03) {
 		clear_bit(BTUSB_BOOTLOADER, &data->flags);
 		btintel_check_bdaddr(hdev);
-		return 0;
 	}
 
 	/* Check for supported iBT hardware variants of this firmware
@@ -2693,35 +2709,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 	if (!ver || !params)
 		return -EINVAL;
 
-	/* The hardware platform number has a fixed value of 0x37 and
-	 * for now only accept this single value.
-	 */
-	if (ver->hw_platform != 0x37) {
-		bt_dev_err(hdev, "Unsupported Intel hardware platform (%u)",
-			   ver->hw_platform);
-		return -EINVAL;
-	}
-
-	/* Check for supported iBT hardware variants of this firmware
-	 * loading method.
-	 *
-	 * This check has been put in place to ensure correct forward
-	 * compatibility options when newer hardware variants come along.
-	 */
-	switch (ver->hw_variant) {
-	case 0x0b:	/* SfP */
-	case 0x0c:	/* WsP */
-	case 0x11:	/* JfP */
-	case 0x12:	/* ThP */
-	case 0x13:	/* HrP */
-	case 0x14:	/* CcP */
-		break;
-	default:
-		bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)",
-			   ver->hw_variant);
-		return -EINVAL;
-	}
-
 	btintel_version_info(hdev, ver);
 
 	/* The firmware variant determines if the device is in bootloader
@@ -2740,16 +2727,8 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 	if (ver->fw_variant == 0x23) {
 		clear_bit(BTUSB_BOOTLOADER, &data->flags);
 		btintel_check_bdaddr(hdev);
-		return 0;
-	}
-
-	/* If the device is not in bootloader mode, then the only possible
-	 * choice is to return an error and abort the device initialization.
-	 */
-	if (ver->fw_variant != 0x06) {
-		bt_dev_err(hdev, "Unsupported Intel firmware variant (%u)",
-			   ver->fw_variant);
-		return -ENODEV;
+		/* Proceed to download to check if the version matches */
+		goto download;
 	}
 
 	/* Read the secure boot parameters to identify the operating
@@ -2777,6 +2756,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 		set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
 	}
 
+download:
 	/* With this Intel bootloader only the hardware variant and device
 	 * revision information are used to select the right firmware for SfP
 	 * and WsP.
@@ -2800,7 +2780,13 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 	 */
 	err = btusb_setup_intel_new_get_fw_name(ver, params, fwname,
 						sizeof(fwname), "sfi");
-	if (!err) {
+	if (err < 0) {
+		if (err == -EALREADY) {
+			/* Firmware has already been loaded */
+			set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);
+			goto done;
+		}
+
 		bt_dev_err(hdev, "Unsupported Intel firmware naming");
 		return -EINVAL;
 	}
-- 
2.26.2

[PATCH v3 3/6] Bluetooth: btintel: Consolidate intel_version_tlv parsing

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This moves checks version checks of intel_version_tlv to
btintel_version_info_tlv.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 drivers/bluetooth/btintel.c | 50 ++++++++++++++++++++++++++++++---
 drivers/bluetooth/btintel.h |  2 +-
 drivers/bluetooth/btusb.c   | 56 ++-----------------------------------
 3 files changed, 50 insertions(+), 58 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index ccab05f67df9..ce4b9ef2015f 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -372,10 +372,53 @@ int btintel_read_version(struct hci_dev *hdev, struct intel_version *ver)
 }
 EXPORT_SYMBOL_GPL(btintel_read_version);
 
-void btintel_version_info_tlv(struct hci_dev *hdev, struct intel_version_tlv *version)
+int btintel_version_info_tlv(struct hci_dev *hdev, struct intel_version_tlv *version)
 {
 	const char *variant;
 
+	/* The hardware platform number has a fixed value of 0x37 and
+	 * for now only accept this single value.
+	 */
+	if (INTEL_HW_PLATFORM(version->cnvi_bt) != 0x37) {
+		bt_dev_err(hdev, "Unsupported Intel hardware platform (0x%2x)",
+			   INTEL_HW_PLATFORM(version->cnvi_bt));
+		return -EINVAL;
+	}
+
+	/* Check for supported iBT hardware variants of this firmware
+	 * loading method.
+	 *
+	 * This check has been put in place to ensure correct forward
+	 * compatibility options when newer hardware variants come along.
+	 */
+	switch (INTEL_HW_VARIANT(version->cnvi_bt)) {
+	case 0x17:	/* TyP */
+	case 0x18:	/* Slr */
+	case 0x19:	/* Slr-F */
+		break;
+	default:
+		bt_dev_err(hdev, "Unsupported Intel hardware variant (0x%x)",
+			   INTEL_HW_VARIANT(version->cnvi_bt));
+		return -EINVAL;
+	}
+
+	/* It is required that every single firmware fragment is acknowledged
+	 * with a command complete event. If the boot parameters indicate
+	 * that this bootloader does not send them, then abort the setup.
+	 */
+	if (version->limited_cce != 0x00) {
+		bt_dev_err(hdev, "Unsupported Intel firmware loading method (0x%x)",
+			   version->limited_cce);
+		return -EINVAL;
+	}
+
+	/* Secure boot engine type should be either 1 (ECDSA) or 0 (RSA) */
+	if (version->sbe_type > 0x01) {
+		bt_dev_err(hdev, "Unsupported Intel secure boot engine type (0x%x)",
+			   version->sbe_type);
+		return -EINVAL;
+	}
+
 	switch (version->img_type) {
 	case 0x01:
 		variant = "Bootloader";
@@ -397,15 +440,14 @@ void btintel_version_info_tlv(struct hci_dev *hdev, struct intel_version_tlv *ve
 		break;
 	default:
 		bt_dev_err(hdev, "Unsupported image type(%02x)", version->img_type);
-		goto done;
+		return -EINVAL;
 	}
 
 	bt_dev_info(hdev, "%s timestamp %u.%u buildtype %u build %u", variant,
 		    2000 + (version->timestamp >> 8), version->timestamp & 0xff,
 		    version->build_type, version->build_num);
 
-done:
-	return;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(btintel_version_info_tlv);
 
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index 51f1f2c883b4..94a63e898826 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -149,7 +149,7 @@ 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_tlv(struct hci_dev *hdev, struct intel_version_tlv *version);
+int btintel_version_info_tlv(struct hci_dev *hdev, struct intel_version_tlv *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);
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 113ff780232f..895950a79400 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2542,15 +2542,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
 	if (!ver || !boot_param)
 		return -EINVAL;
 
-	/* The hardware platform number has a fixed value of 0x37 and
-	 * for now only accept this single value.
-	 */
-	if (INTEL_HW_PLATFORM(ver->cnvi_bt) != 0x37) {
-		bt_dev_err(hdev, "Unsupported Intel hardware platform (0x%2x)",
-			   INTEL_HW_PLATFORM(ver->cnvi_bt));
-		return -EINVAL;
-	}
-
 	/* The firmware variant determines if the device is in bootloader
 	 * mode or is running operational firmware. The value 0x03 identifies
 	 * the bootloader and the value 0x23 identifies the operational
@@ -2569,49 +2560,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
 		btintel_check_bdaddr(hdev);
 	}
 
-	/* Check for supported iBT hardware variants of this firmware
-	 * loading method.
-	 *
-	 * This check has been put in place to ensure correct forward
-	 * compatibility options when newer hardware variants come along.
-	 */
-	switch (INTEL_HW_VARIANT(ver->cnvi_bt)) {
-	case 0x17:	/* TyP */
-	case 0x18:	/* Slr */
-	case 0x19:	/* Slr-F */
-		break;
-	default:
-		bt_dev_err(hdev, "Unsupported Intel hardware variant (0x%x)",
-			   INTEL_HW_VARIANT(ver->cnvi_bt));
-		return -EINVAL;
-	}
-
-	/* If the device is not in bootloader mode, then the only possible
-	 * choice is to return an error and abort the device initialization.
-	 */
-	if (ver->img_type != 0x01) {
-		bt_dev_err(hdev, "Unsupported Intel firmware variant (0x%x)",
-			   ver->img_type);
-		return -ENODEV;
-	}
-
-	/* It is required that every single firmware fragment is acknowledged
-	 * with a command complete event. If the boot parameters indicate
-	 * that this bootloader does not send them, then abort the setup.
-	 */
-	if (ver->limited_cce != 0x00) {
-		bt_dev_err(hdev, "Unsupported Intel firmware loading method (0x%x)",
-			   ver->limited_cce);
-		return -EINVAL;
-	}
-
-	/* Secure boot engine type should be either 1 (ECDSA) or 0 (RSA) */
-	if (ver->sbe_type > 0x01) {
-		bt_dev_err(hdev, "Unsupported Intel secure boot engine type (0x%x)",
-			   ver->sbe_type);
-		return -EINVAL;
-	}
-
 	/* If the OTP has no valid Bluetooth device address, then there will
 	 * also be no valid address for the operational firmware.
 	 */
@@ -3041,7 +2989,9 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
 		return err;
 	}
 
-	btintel_version_info_tlv(hdev, &version);
+	err = btintel_version_info_tlv(hdev, &version);
+	if (err)
+		return err;
 
 	err = btusb_intel_download_firmware_newgen(hdev, &version, &boot_param);
 	if (err)
-- 
2.26.2

[PATCH v3 4/6] Bluetooth: btintel: Consolidate intel_version parsing

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This moves checks version checks of intel_version to
btintel_version_info.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 drivers/bluetooth/btintel.c | 36 ++++++++++++++++++++++++++++++++++--
 drivers/bluetooth/btintel.h |  2 +-
 drivers/bluetooth/btusb.c   | 12 ++++--------
 3 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index ce4b9ef2015f..17f5e3d0dad9 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -216,10 +216,39 @@ 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)
+int btintel_version_info(struct hci_dev *hdev, struct intel_version *ver)
 {
 	const char *variant;
 
+	/* The hardware platform number has a fixed value of 0x37 and
+	 * for now only accept this single value.
+	 */
+	if (ver->hw_platform != 0x37) {
+		bt_dev_err(hdev, "Unsupported Intel hardware platform (%u)",
+			   ver->hw_platform);
+		return -EINVAL;
+	}
+
+	/* Check for supported iBT hardware variants of this firmware
+	 * loading method.
+	 *
+	 * This check has been put in place to ensure correct forward
+	 * compatibility options when newer hardware variants come along.
+	 */
+	switch (ver->hw_variant) {
+	case 0x0b:      /* SfP */
+	case 0x0c:      /* WsP */
+	case 0x11:      /* JfP */
+	case 0x12:      /* ThP */
+	case 0x13:      /* HrP */
+	case 0x14:      /* CcP */
+		break;
+	default:
+		bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)",
+			   ver->hw_variant);
+		return -EINVAL;
+	}
+
 	switch (ver->fw_variant) {
 	case 0x06:
 		variant = "Bootloader";
@@ -228,13 +257,16 @@ void btintel_version_info(struct hci_dev *hdev, struct intel_version *ver)
 		variant = "Firmware";
 		break;
 	default:
-		return;
+		bt_dev_err(hdev, "Unsupported firmware variant(%02x)", ver->fw_variant);
+		return -EINVAL;
 	}
 
 	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);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(btintel_version_info);
 
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index 94a63e898826..7163170410a8 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -148,7 +148,7 @@ 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);
+int btintel_version_info(struct hci_dev *hdev, struct intel_version *ver);
 int btintel_version_info_tlv(struct hci_dev *hdev, struct intel_version_tlv *version);
 int btintel_secure_send(struct hci_dev *hdev, u8 fragment_type, u32 plen,
 			const void *param);
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 895950a79400..c1f8d61ced65 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2474,12 +2474,6 @@ static int btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
 					     char *fw_name, size_t len,
 					     const char *suffix)
 {
-	/* The hardware platform number has a fixed value of 0x37 and
-	 * for now only accept this single value.
-	 */
-	if (ver->hw_platform != 0x37)
-		return -EINVAL;
-
 	switch (ver->hw_variant) {
 	case 0x0b:	/* SfP */
 	case 0x0c:	/* WsP */
@@ -2657,8 +2651,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 	if (!ver || !params)
 		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
@@ -2845,6 +2837,10 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 		return err;
 	}
 
+	err = btintel_version_info(hdev, &ver);
+	if (err)
+		return err;
+
 	err = btusb_intel_download_firmware(hdev, &ver, &params, &boot_param);
 	if (err)
 		return err;
-- 
2.26.2

[PATCH v3 5/6] Bluetooth: btusb: Consolidate code for waiting firmware download

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This moves duplicated code for waiting firmware download to complete to
a function that can then be reused.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 drivers/bluetooth/btusb.c | 108 +++++++++++++++++---------------------
 1 file changed, 48 insertions(+), 60 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index c1f8d61ced65..0462961f4690 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2524,6 +2524,44 @@ static void btusb_setup_intel_newgen_get_fw_name(const struct intel_version_tlv
 		 suffix);
 }
 
+static int btusb_download_wait(struct hci_dev *hdev, ktime_t calltime, int msec)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	ktime_t delta, rettime;
+	unsigned long long duration;
+	int err;
+
+	set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);
+
+	bt_dev_info(hdev, "Waiting for firmware download to complete");
+
+	err = wait_on_bit_timeout(&data->flags, BTUSB_DOWNLOADING,
+				  TASK_INTERRUPTIBLE,
+				  msecs_to_jiffies(msec));
+	if (err == -EINTR) {
+		bt_dev_err(hdev, "Firmware loading interrupted");
+		return err;
+	}
+
+	if (err) {
+		bt_dev_err(hdev, "Firmware loading timeout");
+		return -ETIMEDOUT;
+	}
+
+	if (test_bit(BTUSB_FIRMWARE_FAILED, &data->flags)) {
+		bt_dev_err(hdev, "Firmware loading failed");
+		return -ENOEXEC;
+	}
+
+	rettime = ktime_get();
+	delta = ktime_sub(rettime, calltime);
+	duration = (unsigned long long)ktime_to_ns(delta) >> 10;
+
+	bt_dev_info(hdev, "Firmware loaded in %llu usecs", duration);
+
+	return 0;
+}
+
 static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
 						struct intel_version_tlv *ver,
 						u32 *boot_param)
@@ -2532,6 +2570,7 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
 	char fwname[64];
 	int err;
 	struct btusb_data *data = hci_get_drvdata(hdev);
+	ktime_t calltime;
 
 	if (!ver || !boot_param)
 		return -EINVAL;
@@ -2578,6 +2617,8 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
 		goto done;
 	}
 
+	calltime = ktime_get();
+
 	set_bit(BTUSB_DOWNLOADING, &data->flags);
 
 	/* Start firmware downloading and get boot parameter */
@@ -2597,9 +2638,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
 		btintel_reset_to_bootloader(hdev);
 		goto done;
 	}
-	set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);
-
-	bt_dev_info(hdev, "Waiting for firmware download to complete");
 
 	/* Before switching the device into operational mode and with that
 	 * booting the loaded firmware, wait for the bootloader notification
@@ -2612,26 +2650,9 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
 	 * and thus just timeout if that happens and fail the setup
 	 * of this device.
 	 */
-	err = wait_on_bit_timeout(&data->flags, BTUSB_DOWNLOADING,
-				  TASK_INTERRUPTIBLE,
-				  msecs_to_jiffies(5000));
-	if (err == -EINTR) {
-		bt_dev_err(hdev, "Firmware loading interrupted");
-		goto done;
-	}
-
-	if (err) {
-		bt_dev_err(hdev, "Firmware loading timeout");
-		err = -ETIMEDOUT;
+	err = btusb_download_wait(hdev, calltime, 5000);
+	if (err == -ETIMEDOUT)
 		btintel_reset_to_bootloader(hdev);
-		goto done;
-	}
-
-	if (test_bit(BTUSB_FIRMWARE_FAILED, &data->flags)) {
-		bt_dev_err(hdev, "Firmware loading failed");
-		err = -ENOEXEC;
-		goto done;
-	}
 
 done:
 	release_firmware(fw);
@@ -2647,6 +2668,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 	char fwname[64];
 	int err;
 	struct btusb_data *data = hci_get_drvdata(hdev);
+	ktime_t calltime;
 
 	if (!ver || !params)
 		return -EINVAL;
@@ -2746,6 +2768,8 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 		goto done;
 	}
 
+	calltime = ktime_get();
+
 	set_bit(BTUSB_DOWNLOADING, &data->flags);
 
 	/* Start firmware downloading and get boot parameter */
@@ -2763,9 +2787,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 		btintel_reset_to_bootloader(hdev);
 		goto done;
 	}
-	set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);
-
-	bt_dev_info(hdev, "Waiting for firmware download to complete");
 
 	/* Before switching the device into operational mode and with that
 	 * booting the loaded firmware, wait for the bootloader notification
@@ -2778,26 +2799,9 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 	 * and thus just timeout if that happens and fail the setup
 	 * of this device.
 	 */
-	err = wait_on_bit_timeout(&data->flags, BTUSB_DOWNLOADING,
-				  TASK_INTERRUPTIBLE,
-				  msecs_to_jiffies(5000));
-	if (err == -EINTR) {
-		bt_dev_err(hdev, "Firmware loading interrupted");
-		goto done;
-	}
-
-	if (err) {
-		bt_dev_err(hdev, "Firmware loading timeout");
-		err = -ETIMEDOUT;
+	err = btusb_download_wait(hdev, calltime, 5000);
+	if (err == -ETIMEDOUT)
 		btintel_reset_to_bootloader(hdev);
-		goto done;
-	}
-
-	if (test_bit(BTUSB_FIRMWARE_FAILED, &data->flags)) {
-		bt_dev_err(hdev, "Firmware loading failed");
-		err = -ENOEXEC;
-		goto done;
-	}
 
 done:
 	release_firmware(fw);
@@ -2824,8 +2828,6 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 	 */
 	boot_param = 0x00000000;
 
-	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.
@@ -2849,12 +2851,6 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 	if (ver.fw_variant == 0x23)
 		goto finish;
 
-	rettime = ktime_get();
-	delta = ktime_sub(rettime, calltime);
-	duration = (unsigned long long) ktime_to_ns(delta) >> 10;
-
-	bt_dev_info(hdev, "Firmware loaded in %llu usecs", duration);
-
 	calltime = ktime_get();
 
 	set_bit(BTUSB_BOOTING, &data->flags);
@@ -2972,8 +2968,6 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
 	 */
 	boot_param = 0x00000000;
 
-	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.
@@ -2997,12 +2991,6 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
 	if (version.img_type == 0x03)
 		goto finish;
 
-	rettime = ktime_get();
-	delta = ktime_sub(rettime, calltime);
-	duration = (unsigned long long)ktime_to_ns(delta) >> 10;
-
-	bt_dev_info(hdev, "Firmware loaded in %llu usecs", duration);
-
 	calltime = ktime_get();
 
 	set_bit(BTUSB_BOOTING, &data->flags);
-- 
2.26.2

[PATCH v3 6/6] Bluetooth: btusb: Consolidate code for waiting firmware to boot

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This moves duplicated code for waiting firmware download to complete to
a function that can then be reused.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 drivers/bluetooth/btusb.c | 148 +++++++++++++++++---------------------
 1 file changed, 66 insertions(+), 82 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 0462961f4690..870da9eecfee 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2808,6 +2808,68 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
 	return err;
 }
 
+static int btusb_boot_wait(struct hci_dev *hdev, ktime_t calltime, int msec)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	ktime_t delta, rettime;
+	unsigned long long duration;
+	int err;
+
+	bt_dev_info(hdev, "Waiting for device to boot");
+
+	err = wait_on_bit_timeout(&data->flags, BTUSB_BOOTING,
+				  TASK_INTERRUPTIBLE,
+				  msecs_to_jiffies(msec));
+	if (err == -EINTR) {
+		bt_dev_err(hdev, "Device boot interrupted");
+		return -EINTR;
+	}
+
+	if (err) {
+		bt_dev_err(hdev, "Device boot timeout");
+		return -ETIMEDOUT;
+	}
+
+	rettime = ktime_get();
+	delta = ktime_sub(rettime, calltime);
+	duration = (unsigned long long) ktime_to_ns(delta) >> 10;
+
+	bt_dev_info(hdev, "Device booted in %llu usecs", duration);
+
+	return 0;
+}
+
+static int btusb_intel_boot(struct hci_dev *hdev, u32 boot_addr)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	ktime_t calltime;
+	int err;
+
+	calltime = ktime_get();
+
+	set_bit(BTUSB_BOOTING, &data->flags);
+
+	err = btintel_send_intel_reset(hdev, boot_addr);
+	if (err) {
+		bt_dev_err(hdev, "Intel Soft Reset failed (%d)", err);
+		btintel_reset_to_bootloader(hdev);
+		return err;
+	}
+
+	/* The bootloader will not indicate when the device is ready. This
+	 * is done by the operational firmware sending bootup notification.
+	 *
+	 * Booting into operational firmware should not take longer than
+	 * 1 second. However if that happens, then just fail the setup
+	 * since something went wrong.
+	 */
+	err = btusb_boot_wait(hdev, calltime, 1000);
+	if (err == -ETIMEDOUT)
+		btintel_reset_to_bootloader(hdev);
+
+	return err;
+}
+
 static int btusb_setup_intel_new(struct hci_dev *hdev)
 {
 	struct btusb_data *data = hci_get_drvdata(hdev);
@@ -2815,8 +2877,6 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 	struct intel_boot_params params;
 	u32 boot_param;
 	char ddcname[64];
-	ktime_t calltime, delta, rettime;
-	unsigned long long duration;
 	int err;
 	struct intel_debug_features features;
 
@@ -2851,46 +2911,9 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 	if (ver.fw_variant == 0x23)
 		goto finish;
 
-	calltime = ktime_get();
-
-	set_bit(BTUSB_BOOTING, &data->flags);
-
-	err = btintel_send_intel_reset(hdev, boot_param);
-	if (err) {
-		bt_dev_err(hdev, "Intel Soft Reset failed (%d)", err);
-		btintel_reset_to_bootloader(hdev);
+	err = btusb_intel_boot(hdev, boot_param);
+	if (err)
 		return err;
-	}
-
-	/* The bootloader will not indicate when the device is ready. This
-	 * is done by the operational firmware sending bootup notification.
-	 *
-	 * Booting into operational firmware should not take longer than
-	 * 1 second. However if that happens, then just fail the setup
-	 * since something went wrong.
-	 */
-	bt_dev_info(hdev, "Waiting for device to boot");
-
-	err = wait_on_bit_timeout(&data->flags, BTUSB_BOOTING,
-				  TASK_INTERRUPTIBLE,
-				  msecs_to_jiffies(1000));
-
-	if (err == -EINTR) {
-		bt_dev_err(hdev, "Device boot interrupted");
-		return -EINTR;
-	}
-
-	if (err) {
-		bt_dev_err(hdev, "Device boot timeout");
-		btintel_reset_to_bootloader(hdev);
-		return -ETIMEDOUT;
-	}
-
-	rettime = ktime_get();
-	delta = ktime_sub(rettime, calltime);
-	duration = (unsigned long long) ktime_to_ns(delta) >> 10;
-
-	bt_dev_info(hdev, "Device booted in %llu usecs", duration);
 
 	clear_bit(BTUSB_BOOTLOADER, &data->flags);
 
@@ -2954,8 +2977,6 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
 	struct btusb_data *data = hci_get_drvdata(hdev);
 	u32 boot_param;
 	char ddcname[64];
-	ktime_t calltime, delta, rettime;
-	unsigned long long duration;
 	int err;
 	struct intel_debug_features features;
 	struct intel_version_tlv version;
@@ -2991,46 +3012,9 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
 	if (version.img_type == 0x03)
 		goto finish;
 
-	calltime = ktime_get();
-
-	set_bit(BTUSB_BOOTING, &data->flags);
-
-	err = btintel_send_intel_reset(hdev, boot_param);
-	if (err) {
-		bt_dev_err(hdev, "Intel Soft Reset failed (%d)", err);
-		btintel_reset_to_bootloader(hdev);
+	err = btusb_intel_boot(hdev, boot_param);
+	if (err)
 		return err;
-	}
-
-	/* The bootloader will not indicate when the device is ready. This
-	 * is done by the operational firmware sending bootup notification.
-	 *
-	 * Booting into operational firmware should not take longer than
-	 * 1 second. However if that happens, then just fail the setup
-	 * since something went wrong.
-	 */
-	bt_dev_info(hdev, "Waiting for device to boot");
-
-	err = wait_on_bit_timeout(&data->flags, BTUSB_BOOTING,
-				  TASK_INTERRUPTIBLE,
-				  msecs_to_jiffies(1000));
-
-	if (err == -EINTR) {
-		bt_dev_err(hdev, "Device boot interrupted");
-		return -EINTR;
-	}
-
-	if (err) {
-		bt_dev_err(hdev, "Device boot timeout");
-		btintel_reset_to_bootloader(hdev);
-		return -ETIMEDOUT;
-	}
-
-	rettime = ktime_get();
-	delta = ktime_sub(rettime, calltime);
-	duration = (unsigned long long)ktime_to_ns(delta) >> 10;
-
-	bt_dev_info(hdev, "Device booted in %llu usecs", duration);
 
 	clear_bit(BTUSB_BOOTLOADER, &data->flags);
 
-- 
2.26.2

Re: [PATCH v3 1/6] Bluetooth: btintel: Check firmware version before download

[Tedd Ho-Jeong An]

Hi Luiz,

On Tue, 2021-02-09 at 15:33 -0800, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This checks the firmware build number, week and year matches with
> repective version loaded and then skip the download process.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> v2: Add patch that mover checks for operational mode after the version
> checking.
> v3: Fix not checking for operation mode before using btintel_read_boot_params
> since some models depend on that to contruct the fw filename. Also attempt to
> cleanup duplicated code.
> 
>  drivers/bluetooth/btintel.c   | 94 +++++++++++++++++++++++++++--------
>  drivers/bluetooth/btintel.h   |  5 +-
>  drivers/bluetooth/btusb.c     | 16 +++++-
>  drivers/bluetooth/hci_intel.c |  7 ++-
>  4 files changed, 96 insertions(+), 26 deletions(-)

Here is a quick test result after plug in the card to the USB, which is similar to "reboot" case:

dmesg:
[  380.993756] usb 1-2: new full-speed USB device number 6 using xhci_hcd
[  381.441601] usb 1-2: New USB device found, idVendor=8087, idProduct=0025, bcdDevice= 0.02
[  381.441614] usb 1-2: New USB device strings: Mfr=0, Product=0, SerialNumber=0
[  381.456056] btusb:btusb_probe:4418: intf 00000000daf5923a id 0000000054fbca43
[  381.458813] btusb:btusb_open:1314: hci0
[  381.458824] btusb:btusb_submit_intr_urb:886: hci0
[  381.458902] btusb:btusb_submit_bulk_urb:976: hci0
[  381.458935] btusb:btusb_submit_bulk_urb:976: hci0
[  381.459006] btusb:btusb_setup_intel_new:2814: hci0
[  381.459151] btusb:btusb_send_frame_intel:2347: hci0
[  381.460077] btusb:btusb_intr_complete:841: hci0 urb 00000000fba620cd status 0 count 15
[  381.460167] btusb:btusb_tx_complete:1266: hci0 urb 00000000d075e504 status 0 count 3
[  381.460579] Bluetooth: hci0: Firmware revision 0.1 build 26 week 11 2020
[  381.460685] btusb:btusb_send_frame_intel:2347: hci0
[  381.461515] btusb:btusb_tx_complete:1266: hci0 urb 00000000eac374bf status 0 count 3
[  381.461986] btusb:btusb_intr_complete:841: hci0 urb 00000000fba620cd status 0 count 12
[  381.463058] Bluetooth: hci0: Found device firmware: intel/ibt-18-16-1.sfi
[  381.463283] btintel:btintel_firmware_version:991: hci0: Boot Address: 0xa100
[  381.463291] btintel:btintel_firmware_version:994: hci0: Firmware Version: 0-0.0
[  381.463401] btusb:btusb_send_frame_intel:2347: hci0
[  381.464156] btusb:btusb_tx_complete:1266: hci0 urb 0000000005038ab1 status 0 count 11
[  381.466847] btusb:btusb_intr_complete:841: hci0 urb 00000000fba620cd status -32 count 64
[  381.494960] usb 1-2: USB disconnect, device number 6
[  381.495216] btusb:btusb_intr_complete:841: hci0 urb 00000000fba620cd status -108 count 0
[  381.495324] btusb:btusb_bulk_complete:931: hci0 urb 00000000f4141fb0 status -108 count 0
[  381.495329] btusb:btusb_bulk_complete:931: hci0 urb 00000000a595df06 status -108 count 0
[  381.495342] btusb:btusb_disconnect:4791: intf 00000000daf5923a

[  383.519564] Bluetooth: hci0: command 0xfc01 tx timeout
[  391.555247] Bluetooth: hci0: FW download error recovery failed (-110)

[  391.555395] btusb:btusb_flush:1418: hci0
[  391.555479] btusb:btusb_close:1384: hci0
[  391.556244] btusb:btusb_disconnect:4791: intf 00000000ce1879b8


Timeout from reset command. For hard reset, I don't think it sends the CommandComplete/CommandStatus
events.


< HCI Command: Intel Read Version (0x3f|0x0005) plen
0                                                           #1 [hci0] 9.028782
> HCI Event: Command Complete (0x0e) plen
13                                                                     #2 [hci0] 9.029760
      Intel Read Version (0x3f|0x0005) ncmd 1
        Status: Success (0x00)
        Hardware platform: 0x37
        Hardware variant: 0x12
        Hardware revision: 1.0
        Firmware variant: 0x23
        Firmware revision: 0.1
        Firmware build: 26-11.2020
        Firmware patch: 0
< HCI Command: Read BD ADDR (0x04|0x0009) plen
0                                                                 #3 [hci0] 9.030334
> HCI Event: Command Complete (0x0e) plen
10                                                                     #4 [hci0] 9.031668
      Read BD ADDR (0x04|0x0009) ncmd 1
        Status: Success (0x00)
        Address: 9C:DA:3E:F2:8F:A4 (Intel Corporate)
< HCI Command: Intel Reset (0x3f|0x0001) plen
8                                                                  #5 [hci0] 9.033055
        Reset type: Hard software reset (0x01)
        Patch vectors: Enable (0x01)
        DDC parameters: Reload from OTP (0x01)
        Boot option: Current image (0x00)
        Boot address: 0x00000000
= Close Index:
9C:DA:3E:F2:8F:A4                                                                                   
[hci0] 19.130135


Regards,
Tedd

Re: [PATCH v3 1/6] Bluetooth: btintel: Check firmware version before download

[Tedd Ho-Jeong An]

Hi, Luiz,

On Tue, 2021-02-09 at 17:04 -0800, Tedd Ho-Jeong An wrote:
> Hi Luiz,
> 
> On Tue, 2021-02-09 at 15:33 -0800, Luiz Augusto von Dentz wrote:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > 
> > This checks the firmware build number, week and year matches with
> > repective version loaded and then skip the download process.
> > 
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > v2: Add patch that mover checks for operational mode after the version
> > checking.
> > v3: Fix not checking for operation mode before using btintel_read_boot_params
> > since some models depend on that to contruct the fw filename. Also attempt to
> > cleanup duplicated code.
> > 
> >  drivers/bluetooth/btintel.c   | 94 +++++++++++++++++++++++++++--------
> >  drivers/bluetooth/btintel.h   |  5 +-
> >  drivers/bluetooth/btusb.c     | 16 +++++-
> >  drivers/bluetooth/hci_intel.c |  7 ++-
> >  4 files changed, 96 insertions(+), 26 deletions(-)
> 
> Here is a quick test result after plug in the card to the USB, which is similar to "reboot" case:

This is not actually correct. I was using VM to test so the device was alrady running in
operational firmware.
Let me run on real hw and send the new result.

> 
> dmesg:
> [  380.993756] usb 1-2: new full-speed USB device number 6 using xhci_hcd
> [  381.441601] usb 1-2: New USB device found, idVendor=8087, idProduct=0025, bcdDevice= 0.02
> [  381.441614] usb 1-2: New USB device strings: Mfr=0, Product=0, SerialNumber=0
> [  381.456056] btusb:btusb_probe:4418: intf 00000000daf5923a id 0000000054fbca43
> [  381.458813] btusb:btusb_open:1314: hci0
> [  381.458824] btusb:btusb_submit_intr_urb:886: hci0
> [  381.458902] btusb:btusb_submit_bulk_urb:976: hci0
> [  381.458935] btusb:btusb_submit_bulk_urb:976: hci0
> [  381.459006] btusb:btusb_setup_intel_new:2814: hci0
> [  381.459151] btusb:btusb_send_frame_intel:2347: hci0
> [  381.460077] btusb:btusb_intr_complete:841: hci0 urb 00000000fba620cd status 0 count 15
> [  381.460167] btusb:btusb_tx_complete:1266: hci0 urb 00000000d075e504 status 0 count 3
> [  381.460579] Bluetooth: hci0: Firmware revision 0.1 build 26 week 11 2020
> [  381.460685] btusb:btusb_send_frame_intel:2347: hci0
> [  381.461515] btusb:btusb_tx_complete:1266: hci0 urb 00000000eac374bf status 0 count 3
> [  381.461986] btusb:btusb_intr_complete:841: hci0 urb 00000000fba620cd status 0 count 12
> [  381.463058] Bluetooth: hci0: Found device firmware: intel/ibt-18-16-1.sfi
> [  381.463283] btintel:btintel_firmware_version:991: hci0: Boot Address: 0xa100
> [  381.463291] btintel:btintel_firmware_version:994: hci0: Firmware Version: 0-0.0

I have noticed this line, which isn't correct. it is actually
Boot Parameters
Boot Address:	0x00040800
Firmware build:	26-11.2020

[  381.463401] btusb:btusb_send_frame_intel:2347: hci0
> [  381.464156] btusb:btusb_tx_complete:1266: hci0 urb 0000000005038ab1 status 0 count 11
> [  381.466847] btusb:btusb_intr_complete:841: hci0 urb 00000000fba620cd status -32 count 64
> [  381.494960] usb 1-2: USB disconnect, device number 6
> [  381.495216] btusb:btusb_intr_complete:841: hci0 urb 00000000fba620cd status -108 count 0
> [  381.495324] btusb:btusb_bulk_complete:931: hci0 urb 00000000f4141fb0 status -108 count 0
> [  381.495329] btusb:btusb_bulk_complete:931: hci0 urb 00000000a595df06 status -108 count 0
> [  381.495342] btusb:btusb_disconnect:4791: intf 00000000daf5923a
> 
> [  383.519564] Bluetooth: hci0: command 0xfc01 tx timeout
> [  391.555247] Bluetooth: hci0: FW download error recovery failed (-110)
> 
> [  391.555395] btusb:btusb_flush:1418: hci0
> [  391.555479] btusb:btusb_close:1384: hci0
> [  391.556244] btusb:btusb_disconnect:4791: intf 00000000ce1879b8
> 
> 
> Timeout from reset command. For hard reset, I don't think it sends the
> CommandComplete/CommandStatus
> events.
> 
> 
> < HCI Command: Intel Read Version (0x3f|0x0005) plen
> 0                                                           #1 [hci0] 9.028782
> > HCI Event: Command Complete (0x0e) plen
> 13                                                                     #2 [hci0] 9.029760
>       Intel Read Version (0x3f|0x0005) ncmd 1
>         Status: Success (0x00)
>         Hardware platform: 0x37
>         Hardware variant: 0x12
>         Hardware revision: 1.0
>         Firmware variant: 0x23
>         Firmware revision: 0.1
>         Firmware build: 26-11.2020
>         Firmware patch: 0
> < HCI Command: Read BD ADDR (0x04|0x0009) plen
> 0                                                                 #3 [hci0] 9.030334
> > HCI Event: Command Complete (0x0e) plen
> 10                                                                     #4 [hci0] 9.031668
>       Read BD ADDR (0x04|0x0009) ncmd 1
>         Status: Success (0x00)
>         Address: 9C:DA:3E:F2:8F:A4 (Intel Corporate)
> < HCI Command: Intel Reset (0x3f|0x0001) plen
> 8                                                                  #5 [hci0] 9.033055
>         Reset type: Hard software reset (0x01)
>         Patch vectors: Enable (0x01)
>         DDC parameters: Reload from OTP (0x01)
>         Boot option: Current image (0x00)
>         Boot address: 0x00000000
> = Close Index:
> 9C:DA:3E:F2:8F:A4                                                                                 
>   
> [hci0] 19.130135
> 
> 
> Regards,
> Tedd
> 

RE: [v3,1/6] Bluetooth: btintel: Check firmware version before download

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=431087

---Test result---

##############################
    Test: CheckPatch - PASS
    

    ##############################
    Test: CheckGitLint - PASS
    

    ##############################
    Test: CheckBuildK - PASS
    

    ##############################
    Test: CheckTestRunner: Setup - PASS
    

    ##############################
    Test: CheckTestRunner: l2cap-tester - PASS
    Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6

    ##############################
    Test: CheckTestRunner: bnep-tester - PASS
    Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: mgmt-tester - PASS
    Total: 416, Passed: 402 (96.6%), Failed: 0, Not Run: 14

    ##############################
    Test: CheckTestRunner: rfcomm-tester - PASS
    Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: sco-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: smp-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: userchan-tester - PASS
    Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0

    

---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 43342 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3533 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 546680 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11653 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9888 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11799 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5430 bytes --]

Re: [PATCH v3 1/6] Bluetooth: btintel: Check firmware version before download

[Tedd Ho-Jeong An]

[-- Attachment #1: Type: text/plain, Size: 3779 bytes --]

Hi Luiz,

On Tue, 2021-02-09 at 17:26 -0800, Tedd Ho-Jeong An wrote:
> Hi, Luiz,
> 
> On Tue, 2021-02-09 at 17:04 -0800, Tedd Ho-Jeong An wrote:
> > Hi Luiz,
> > 
> > On Tue, 2021-02-09 at 15:33 -0800, Luiz Augusto von Dentz wrote:
> > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > 
> > > This checks the firmware build number, week and year matches with
> > > repective version loaded and then skip the download process.
> > > 
> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > ---
> > > v2: Add patch that mover checks for operational mode after the version
> > > checking.
> > > v3: Fix not checking for operation mode before using btintel_read_boot_params
> > > since some models depend on that to contruct the fw filename. Also attempt to
> > > cleanup duplicated code.
> > > 
> > >  drivers/bluetooth/btintel.c   | 94 +++++++++++++++++++++++++++--------
> > >  drivers/bluetooth/btintel.h   |  5 +-
> > >  drivers/bluetooth/btusb.c     | 16 +++++-
> > >  drivers/bluetooth/hci_intel.c |  7 ++-
> > >  4 files changed, 96 insertions(+), 26 deletions(-)

I just completed the following test with this patches and here are the results.
Dmesg outputs are attached.

1. Cold boot
	FW loaded as expected.
2. New FW + cold boot
	New FW loaded as expected.
3. Reboot
	Same FW loaded again after restting the device to bootloader.
4. New FW + Reboot
	New FW loaded after resetting the device to bootloader.
5. Suspend
	No BT driver activity since no enumeration

Please check #3 case which loading the firmware again even if the same
version of firmeare is already running.

This is the dmesg output.
[    4.021072] Bluetooth: Core ver 2.22
[    4.021089] Bluetooth: HCI device and connection manager initialized
[    4.021092] Bluetooth: HCI socket layer initialized
[    4.021093] Bluetooth: L2CAP socket layer initialized
[    4.021097] Bluetooth: SCO socket layer initialized
[    4.625690] Bluetooth: hci0: Firmware revision 0.1 build 26 week 11 2020
[    4.631875] Bluetooth: hci0: Found device firmware: intel/ibt-17-16-1.sfi
[    5.955215] Bluetooth: BNEP (Ethernet Emulation) ver 1.3
[    5.955217] Bluetooth: BNEP filters: protocol multicast
[    5.955220] Bluetooth: BNEP socket layer initialized
[    6.660529] Bluetooth: hci0: command 0xfc01 tx timeout
[   14.654714] Bluetooth: hci0: FW download error recovery failed (-110)
[   15.079684] Bluetooth: hci0: Bootloader revision 0.1 build 42 week 52 2015
[   15.080652] Bluetooth: hci0: Device revision is 2
[   15.080662] Bluetooth: hci0: Secure boot is enabled
[   15.080667] Bluetooth: hci0: OTP lock is enabled
[   15.080673] Bluetooth: hci0: API lock is enabled
[   15.080678] Bluetooth: hci0: Debug lock is disabled
[   15.080687] Bluetooth: hci0: Minimum firmware build 1 week 10 2014
[   15.081832] Bluetooth: hci0: Found device firmware: intel/ibt-17-16-1.sfi
[   16.965194] Bluetooth: hci0: Waiting for firmware download to complete
[   16.965560] Bluetooth: hci0: Firmware loaded in 1839570 usecs
[   16.965637] Bluetooth: hci0: Waiting for device to boot
[   16.978543] Bluetooth: hci0: Device booted in 12630 usecs
[   16.978895] Bluetooth: hci0: Found Intel DDC parameters: intel/ibt-17-16-1.ddc
[   16.981587] Bluetooth: hci0: Applying Intel DDC parameters completed
[   16.984616] Bluetooth: hci0: Firmware revision 0.1 build 26 week 11 2020
[   17.045576] Bluetooth: hci0: MSFT filter_enable is already on
[   70.621004] Bluetooth: RFCOMM TTY layer initialized
[   70.621010] Bluetooth: RFCOMM socket layer initialized
[   70.621014] Bluetooth: RFCOMM ver 1.11

While testing the scenarios, I found a bug (not from your patch series)
which prevents the driver from loading DDC. I will submit patch separately.

> > 
> > Regards,
> > Tedd
> > 

Regards,
Tedd

[-- Attachment #2: fw_check_test.txt --]
[-- Type: text/plain, Size: 6282 bytes --]

cold reboot:
[    4.203608] Bluetooth: hci0: Bootloader revision 0.1 build 42 week 52 2015
[    4.235127] Bluetooth: hci0: Found device firmware: intel/ibt-17-16-1.sfi
[    5.909625] Bluetooth: hci0: Waiting for firmware download to complete
[    5.910518] Bluetooth: hci0: Firmware loaded in 1636120 usecs
[    5.910547] Bluetooth: hci0: Waiting for device to boot
[    5.924519] Bluetooth: hci0: Device booted in 13651 usecs
[    5.926792] Bluetooth: hci0: Found Intel DDC parameters: intel/ibt-17-16-1.ddc
[    5.928523] Bluetooth: hci0: Applying Intel DDC parameters completed
[    5.931523] Bluetooth: hci0: Firmware revision 0.1 build 168 week 48 2020

New FW + cold reboot:
[    4.003687] Bluetooth: Core ver 2.22
[    4.003710] Bluetooth: HCI device and connection manager initialized
[    4.003713] Bluetooth: HCI socket layer initialized
[    4.003715] Bluetooth: L2CAP socket layer initialized
[    4.003718] Bluetooth: SCO socket layer initialized
[    4.238399] Bluetooth: hci0: Bootloader revision 0.1 build 42 week 52 2015
[    4.239395] Bluetooth: hci0: Device revision is 2
[    4.239396] Bluetooth: hci0: Secure boot is enabled
[    4.239396] Bluetooth: hci0: OTP lock is enabled
[    4.239397] Bluetooth: hci0: API lock is enabled
[    4.239398] Bluetooth: hci0: Debug lock is disabled
[    4.239399] Bluetooth: hci0: Minimum firmware build 1 week 10 2014
[    4.311738] Bluetooth: hci0: Found device firmware: intel/ibt-17-16-1.sfi
[    5.629135] Bluetooth: BNEP (Ethernet Emulation) ver 1.3
[    5.629137] Bluetooth: BNEP filters: protocol multicast
[    5.629139] Bluetooth: BNEP socket layer initialized
[    5.703020] Bluetooth: hci0: Waiting for firmware download to complete
[    5.703384] Bluetooth: hci0: Firmware loaded in 1359026 usecs
[    5.703411] Bluetooth: hci0: Waiting for device to boot
[    5.716412] Bluetooth: hci0: Device booted in 12707 usecs
[    5.716714] Bluetooth: hci0: Found Intel DDC parameters: intel/ibt-17-16-1.ddc
[    5.719383] Bluetooth: hci0: Applying Intel DDC parameters completed
[    5.722389] Bluetooth: hci0: Firmware revision 0.1 build 26 week 11 2020
[    5.780406] Bluetooth: hci0: MSFT filter_enable is already on
[   14.822336] Bluetooth: RFCOMM TTY layer initialized
[   14.822342] Bluetooth: RFCOMM socket layer initialized
[   14.822345] Bluetooth: RFCOMM ver 1.11


Reboot
[    4.021072] Bluetooth: Core ver 2.22
[    4.021089] Bluetooth: HCI device and connection manager initialized
[    4.021092] Bluetooth: HCI socket layer initialized
[    4.021093] Bluetooth: L2CAP socket layer initialized
[    4.021097] Bluetooth: SCO socket layer initialized
[    4.625690] Bluetooth: hci0: Firmware revision 0.1 build 26 week 11 2020
[    4.631875] Bluetooth: hci0: Found device firmware: intel/ibt-17-16-1.sfi
[    5.955215] Bluetooth: BNEP (Ethernet Emulation) ver 1.3
[    5.955217] Bluetooth: BNEP filters: protocol multicast
[    5.955220] Bluetooth: BNEP socket layer initialized
[    6.660529] Bluetooth: hci0: command 0xfc01 tx timeout
[   14.654714] Bluetooth: hci0: FW download error recovery failed (-110)
[   15.079684] Bluetooth: hci0: Bootloader revision 0.1 build 42 week 52 2015
[   15.080652] Bluetooth: hci0: Device revision is 2
[   15.080662] Bluetooth: hci0: Secure boot is enabled
[   15.080667] Bluetooth: hci0: OTP lock is enabled
[   15.080673] Bluetooth: hci0: API lock is enabled
[   15.080678] Bluetooth: hci0: Debug lock is disabled
[   15.080687] Bluetooth: hci0: Minimum firmware build 1 week 10 2014
[   15.081832] Bluetooth: hci0: Found device firmware: intel/ibt-17-16-1.sfi
[   16.965194] Bluetooth: hci0: Waiting for firmware download to complete
[   16.965560] Bluetooth: hci0: Firmware loaded in 1839570 usecs
[   16.965637] Bluetooth: hci0: Waiting for device to boot
[   16.978543] Bluetooth: hci0: Device booted in 12630 usecs
[   16.978895] Bluetooth: hci0: Found Intel DDC parameters: intel/ibt-17-16-1.ddc
[   16.981587] Bluetooth: hci0: Applying Intel DDC parameters completed
[   16.984616] Bluetooth: hci0: Firmware revision 0.1 build 26 week 11 2020
[   17.045576] Bluetooth: hci0: MSFT filter_enable is already on
[   70.621004] Bluetooth: RFCOMM TTY layer initialized
[   70.621010] Bluetooth: RFCOMM socket layer initialized
[   70.621014] Bluetooth: RFCOMM ver 1.11

New FW + Reboot
[    3.814893] Bluetooth: Core ver 2.22
[    3.814906] Bluetooth: HCI device and connection manager initialized
[    3.814908] Bluetooth: HCI socket layer initialized
[    3.814910] Bluetooth: L2CAP socket layer initialized
[    3.814911] Bluetooth: SCO socket layer initialized
[    3.999600] Bluetooth: hci0: Firmware revision 0.1 build 26 week 11 2020
[    4.003750] Bluetooth: hci0: Found device firmware: intel/ibt-17-16-1.sfi
[    6.014371] Bluetooth: hci0: command 0xfc01 tx timeout
[    7.268662] Bluetooth: BNEP (Ethernet Emulation) ver 1.3
[    7.268664] Bluetooth: BNEP filters: protocol multicast
[    7.268667] Bluetooth: BNEP socket layer initialized
[   14.142644] Bluetooth: hci0: FW download error recovery failed (-110)
[   14.569698] Bluetooth: hci0: Bootloader revision 0.1 build 42 week 52 2015
[   14.570636] Bluetooth: hci0: Device revision is 2
[   14.570645] Bluetooth: hci0: Secure boot is enabled
[   14.570650] Bluetooth: hci0: OTP lock is enabled
[   14.570656] Bluetooth: hci0: API lock is enabled
[   14.570661] Bluetooth: hci0: Debug lock is disabled
[   14.570669] Bluetooth: hci0: Minimum firmware build 1 week 10 2014
[   14.571733] Bluetooth: hci0: Found device firmware: intel/ibt-17-16-1.sfi
[   16.017567] Bluetooth: hci0: Waiting for firmware download to complete
[   16.018529] Bluetooth: hci0: Firmware loaded in 1412881 usecs
[   16.018666] Bluetooth: hci0: Waiting for device to boot
[   16.032624] Bluetooth: hci0: Device booted in 13702 usecs
[   16.035203] Bluetooth: hci0: Found Intel DDC parameters: intel/ibt-17-16-1.ddc
[   16.036755] Bluetooth: hci0: Applying Intel DDC parameters completed
[   16.039822] Bluetooth: hci0: Firmware revision 0.1 build 168 week 48 2020
[   16.099690] Bluetooth: hci0: MSFT filter_enable is already on
[   18.660811] Bluetooth: RFCOMM TTY layer initialized
[   18.660815] Bluetooth: RFCOMM socket layer initialized
[   18.660819] Bluetooth: RFCOMM ver 1.11

Suspend
No Change since no enumeration

Re: [PATCH v3 2/6] Bluetooth: btintel: Move operational checks after version check

[Tedd Ho-Jeong An]

Hi Luiz,

On Tue, 2021-02-09 at 15:34 -0800, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> In order to allow new firmware to be loaded it first needs to check if
> the firmware version on file matches the one loaded if it doesn't then
> it needs to revert to boorloader mode in order to load the new firmware.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  drivers/bluetooth/btintel.c | 22 +++++++++++
>  drivers/bluetooth/btusb.c   | 74 +++++++++++++++----------------------
>  2 files changed, 52 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 153989bd8d5f..ccab05f67df9 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -948,6 +948,17 @@ int btintel_download_firmware(struct hci_dev *hdev,
>  		return -EALREADY;
>  	}
>  
> +	/* 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
> +	 * firmware.
> +	 *
> +	 * If the firmware version has changed that means it needs to be reset
> +	 * to bootloader when operational so the new firmware can be loaded.
> +	 */
> +	if (ver->fw_variant == 0x23)
> +		return -EINVAL;
> +
>  	err = btintel_sfi_rsa_header_secure_send(hdev, fw);
>  	if (err)
>  		return err;
> @@ -974,6 +985,17 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev,
>  		return -EALREADY;
>  	}
>  
> +	/* The firmware variant determines if the device is in bootloader
> +	 * mode or is running operational firmware. The value 0x03 identifies
> +	 * the bootloader and the value 0x23 identifies the operational
> +	 * firmware.
> +	 *
> +	 * If the firmware version has changed that means it needs to be reset
> +	 * to bootloader when operational so the new firmware can be loaded.
> +	 */
> +	if (ver->img_type == 0x03)
> +		return -EINVAL;
> +
>  	/* iBT hardware variants 0x0b, 0x0c, 0x11, 0x12, 0x13, 0x14 support
>  	 * only RSA secure boot engine. Hence, the corresponding sfi file will
>  	 * have RSA header of 644 bytes followed by Command Buffer.
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index e896c6702d60..113ff780232f 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2469,14 +2469,30 @@ 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,
> +static int 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)
>  {
> +	/* The hardware platform number has a fixed value of 0x37 and
> +	 * for now only accept this single value.
> +	 */
> +	if (ver->hw_platform != 0x37)
> +		return -EINVAL;
> +
>  	switch (ver->hw_variant) {
>  	case 0x0b:	/* SfP */
>  	case 0x0c:	/* WsP */
> +		/* The firmware variant determines if the device is in
> +		 * bootloader mode or is running operational firmware.
> +		 *
> +		 * Version checking cannot be performed in these models since
> +		 * the firmware versioning depends on the firmware being in
> +		 * bootloader mode.
> +		 */
> +		if (ver->fw_variant == 0x23)
> +			return -EALREADY;
> +
>  		snprintf(fw_name, len, "intel/ibt-%u-%u.%s",
>  			le16_to_cpu(ver->hw_variant),
>  			le16_to_cpu(params->dev_revid),
> @@ -2493,9 +2509,10 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
>  			suffix);
>  		break;
>  	default:
> -		return false;
> +		return -EINVAL;
>  	}
> -	return true;
> +
> +	return 0;
>  }
>  
>  static void btusb_setup_intel_newgen_get_fw_name(const struct intel_version_tlv *ver_tlv,
> @@ -2550,7 +2567,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
>  	if (ver->img_type == 0x03) {
>  		clear_bit(BTUSB_BOOTLOADER, &data->flags);
>  		btintel_check_bdaddr(hdev);
> -		return 0;
>  	}
>  
>  	/* Check for supported iBT hardware variants of this firmware
> @@ -2693,35 +2709,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
>  	if (!ver || !params)
>  		return -EINVAL;
>  
> -	/* The hardware platform number has a fixed value of 0x37 and
> -	 * for now only accept this single value.
> -	 */
> -	if (ver->hw_platform != 0x37) {
> -		bt_dev_err(hdev, "Unsupported Intel hardware platform (%u)",
> -			   ver->hw_platform);
> -		return -EINVAL;
> -	}
> -
> -	/* Check for supported iBT hardware variants of this firmware
> -	 * loading method.
> -	 *
> -	 * This check has been put in place to ensure correct forward
> -	 * compatibility options when newer hardware variants come along.
> -	 */
> -	switch (ver->hw_variant) {
> -	case 0x0b:	/* SfP */
> -	case 0x0c:	/* WsP */
> -	case 0x11:	/* JfP */
> -	case 0x12:	/* ThP */
> -	case 0x13:	/* HrP */
> -	case 0x14:	/* CcP */
> -		break;
> -	default:
> -		bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)",
> -			   ver->hw_variant);
> -		return -EINVAL;
> -	}
> -
>  	btintel_version_info(hdev, ver);
>  
>  	/* The firmware variant determines if the device is in bootloader
> @@ -2740,16 +2727,8 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
>  	if (ver->fw_variant == 0x23) {
>  		clear_bit(BTUSB_BOOTLOADER, &data->flags);
>  		btintel_check_bdaddr(hdev);
> -		return 0;
> -	}
> -
> -	/* If the device is not in bootloader mode, then the only possible
> -	 * choice is to return an error and abort the device initialization.
> -	 */
> -	if (ver->fw_variant != 0x06) {
> -		bt_dev_err(hdev, "Unsupported Intel firmware variant (%u)",
> -			   ver->fw_variant);
> -		return -ENODEV;
> +		/* Proceed to download to check if the version matches */
> +		goto download;
>  	}
>  
>  	/* Read the secure boot parameters to identify the operating
> @@ -2777,6 +2756,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
>  		set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
>  	}
>  
> +download:
>  	/* With this Intel bootloader only the hardware variant and device
>  	 * revision information are used to select the right firmware for SfP
>  	 * and WsP.
> @@ -2800,7 +2780,13 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
>  	 */
>  	err = btusb_setup_intel_new_get_fw_name(ver, params, fwname,
>  						sizeof(fwname), "sfi");
> -	if (!err) {

There is one more place to change the handling of return code for this function.

err = btusb_setup_intel_new_get_fw_name(&ver, &params, ddcname,
					sizeof(ddcname), "ddc");
if (!err) {
	bt_dev_err(hdev, "Unsupported Intel firmware naming");
} else {



> +	if (err < 0) {
> +		if (err == -EALREADY) {
> +			/* Firmware has already been loaded */
> +			set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);
> +			goto done;
> +		}
> +
>  		bt_dev_err(hdev, "Unsupported Intel firmware naming");
>  		return -EINVAL;
>  	}

Re: [PATCH v3 2/6] Bluetooth: btintel: Move operational checks after version check

[Marcel Holtmann]

Hi Luiz,

> In order to allow new firmware to be loaded it first needs to check if
> the firmware version on file matches the one loaded if it doesn't then
> it needs to revert to boorloader mode in order to load the new firmware.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> drivers/bluetooth/btintel.c | 22 +++++++++++
> drivers/bluetooth/btusb.c   | 74 +++++++++++++++----------------------
> 2 files changed, 52 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 153989bd8d5f..ccab05f67df9 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -948,6 +948,17 @@ int btintel_download_firmware(struct hci_dev *hdev,
> 		return -EALREADY;
> 	}
> 
> +	/* 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
> +	 * firmware.
> +	 *
> +	 * If the firmware version has changed that means it needs to be reset
> +	 * to bootloader when operational so the new firmware can be loaded.
> +	 */
> +	if (ver->fw_variant == 0x23)
> +		return -EINVAL;
> +
> 	err = btintel_sfi_rsa_header_secure_send(hdev, fw);
> 	if (err)
> 		return err;
> @@ -974,6 +985,17 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev,
> 		return -EALREADY;
> 	}
> 
> +	/* The firmware variant determines if the device is in bootloader
> +	 * mode or is running operational firmware. The value 0x03 identifies
> +	 * the bootloader and the value 0x23 identifies the operational
> +	 * firmware.
> +	 *
> +	 * If the firmware version has changed that means it needs to be reset
> +	 * to bootloader when operational so the new firmware can be loaded.
> +	 */
> +	if (ver->img_type == 0x03)
> +		return -EINVAL;
> +
> 	/* iBT hardware variants 0x0b, 0x0c, 0x11, 0x12, 0x13, 0x14 support
> 	 * only RSA secure boot engine. Hence, the corresponding sfi file will
> 	 * have RSA header of 644 bytes followed by Command Buffer.
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index e896c6702d60..113ff780232f 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2469,14 +2469,30 @@ 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,
> +static int 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)
> {
> +	/* The hardware platform number has a fixed value of 0x37 and
> +	 * for now only accept this single value.
> +	 */
> +	if (ver->hw_platform != 0x37)
> +		return -EINVAL;
> +
> 	switch (ver->hw_variant) {
> 	case 0x0b:	/* SfP */
> 	case 0x0c:	/* WsP */
> +		/* The firmware variant determines if the device is in
> +		 * bootloader mode or is running operational firmware.
> +		 *
> +		 * Version checking cannot be performed in these models since
> +		 * the firmware versioning depends on the firmware being in
> +		 * bootloader mode.
> +		 */
> +		if (ver->fw_variant == 0x23)
> +			return -EALREADY;
> +
> 		snprintf(fw_name, len, "intel/ibt-%u-%u.%s",
> 			le16_to_cpu(ver->hw_variant),
> 			le16_to_cpu(params->dev_revid),
> @@ -2493,9 +2509,10 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
> 			suffix);
> 		break;
> 	default:
> -		return false;
> +		return -EINVAL;
> 	}
> -	return true;
> +
> +	return 0;
> }
> 
> static void btusb_setup_intel_newgen_get_fw_name(const struct intel_version_tlv *ver_tlv,
> @@ -2550,7 +2567,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
> 	if (ver->img_type == 0x03) {
> 		clear_bit(BTUSB_BOOTLOADER, &data->flags);
> 		btintel_check_bdaddr(hdev);
> -		return 0;
> 	}
> 
> 	/* Check for supported iBT hardware variants of this firmware
> @@ -2693,35 +2709,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
> 	if (!ver || !params)
> 		return -EINVAL;
> 
> -	/* The hardware platform number has a fixed value of 0x37 and
> -	 * for now only accept this single value.
> -	 */
> -	if (ver->hw_platform != 0x37) {
> -		bt_dev_err(hdev, "Unsupported Intel hardware platform (%u)",
> -			   ver->hw_platform);
> -		return -EINVAL;
> -	}
> -
> -	/* Check for supported iBT hardware variants of this firmware
> -	 * loading method.
> -	 *
> -	 * This check has been put in place to ensure correct forward
> -	 * compatibility options when newer hardware variants come along.
> -	 */
> -	switch (ver->hw_variant) {
> -	case 0x0b:	/* SfP */
> -	case 0x0c:	/* WsP */
> -	case 0x11:	/* JfP */
> -	case 0x12:	/* ThP */
> -	case 0x13:	/* HrP */
> -	case 0x14:	/* CcP */
> -		break;
> -	default:
> -		bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)",
> -			   ver->hw_variant);
> -		return -EINVAL;
> -	}
> -

why are you removing this? I put this in so that really only supported platforms are tried. We want to fail if you run on hardware that isn’t officially tested with this driver.

Regards

Marcel

Re: [PATCH v3 2/6] Bluetooth: btintel: Move operational checks after version check

[Luiz Augusto von Dentz]

Hi Marcel,

On Wed, Feb 10, 2021 at 7:20 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > In order to allow new firmware to be loaded it first needs to check if
> > the firmware version on file matches the one loaded if it doesn't then
> > it needs to revert to boorloader mode in order to load the new firmware.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > drivers/bluetooth/btintel.c | 22 +++++++++++
> > drivers/bluetooth/btusb.c   | 74 +++++++++++++++----------------------
> > 2 files changed, 52 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 153989bd8d5f..ccab05f67df9 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -948,6 +948,17 @@ int btintel_download_firmware(struct hci_dev *hdev,
> >               return -EALREADY;
> >       }
> >
> > +     /* 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
> > +      * firmware.
> > +      *
> > +      * If the firmware version has changed that means it needs to be reset
> > +      * to bootloader when operational so the new firmware can be loaded.
> > +      */
> > +     if (ver->fw_variant == 0x23)
> > +             return -EINVAL;
> > +
> >       err = btintel_sfi_rsa_header_secure_send(hdev, fw);
> >       if (err)
> >               return err;
> > @@ -974,6 +985,17 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev,
> >               return -EALREADY;
> >       }
> >
> > +     /* The firmware variant determines if the device is in bootloader
> > +      * mode or is running operational firmware. The value 0x03 identifies
> > +      * the bootloader and the value 0x23 identifies the operational
> > +      * firmware.
> > +      *
> > +      * If the firmware version has changed that means it needs to be reset
> > +      * to bootloader when operational so the new firmware can be loaded.
> > +      */
> > +     if (ver->img_type == 0x03)
> > +             return -EINVAL;
> > +
> >       /* iBT hardware variants 0x0b, 0x0c, 0x11, 0x12, 0x13, 0x14 support
> >        * only RSA secure boot engine. Hence, the corresponding sfi file will
> >        * have RSA header of 644 bytes followed by Command Buffer.
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index e896c6702d60..113ff780232f 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2469,14 +2469,30 @@ 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,
> > +static int 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)
> > {
> > +     /* The hardware platform number has a fixed value of 0x37 and
> > +      * for now only accept this single value.
> > +      */
> > +     if (ver->hw_platform != 0x37)
> > +             return -EINVAL;
> > +
> >       switch (ver->hw_variant) {
> >       case 0x0b:      /* SfP */
> >       case 0x0c:      /* WsP */
> > +             /* The firmware variant determines if the device is in
> > +              * bootloader mode or is running operational firmware.
> > +              *
> > +              * Version checking cannot be performed in these models since
> > +              * the firmware versioning depends on the firmware being in
> > +              * bootloader mode.
> > +              */
> > +             if (ver->fw_variant == 0x23)
> > +                     return -EALREADY;
> > +
> >               snprintf(fw_name, len, "intel/ibt-%u-%u.%s",
> >                       le16_to_cpu(ver->hw_variant),
> >                       le16_to_cpu(params->dev_revid),
> > @@ -2493,9 +2509,10 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
> >                       suffix);
> >               break;
> >       default:
> > -             return false;
> > +             return -EINVAL;
> >       }
> > -     return true;
> > +
> > +     return 0;
> > }
> >
> > static void btusb_setup_intel_newgen_get_fw_name(const struct intel_version_tlv *ver_tlv,
> > @@ -2550,7 +2567,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
> >       if (ver->img_type == 0x03) {
> >               clear_bit(BTUSB_BOOTLOADER, &data->flags);
> >               btintel_check_bdaddr(hdev);
> > -             return 0;
> >       }
> >
> >       /* Check for supported iBT hardware variants of this firmware
> > @@ -2693,35 +2709,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
> >       if (!ver || !params)
> >               return -EINVAL;
> >
> > -     /* The hardware platform number has a fixed value of 0x37 and
> > -      * for now only accept this single value.
> > -      */
> > -     if (ver->hw_platform != 0x37) {
> > -             bt_dev_err(hdev, "Unsupported Intel hardware platform (%u)",
> > -                        ver->hw_platform);
> > -             return -EINVAL;
> > -     }
> > -
> > -     /* Check for supported iBT hardware variants of this firmware
> > -      * loading method.
> > -      *
> > -      * This check has been put in place to ensure correct forward
> > -      * compatibility options when newer hardware variants come along.
> > -      */
> > -     switch (ver->hw_variant) {
> > -     case 0x0b:      /* SfP */
> > -     case 0x0c:      /* WsP */
> > -     case 0x11:      /* JfP */
> > -     case 0x12:      /* ThP */
> > -     case 0x13:      /* HrP */
> > -     case 0x14:      /* CcP */
> > -             break;
> > -     default:
> > -             bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)",
> > -                        ver->hw_variant);
> > -             return -EINVAL;
> > -     }
> > -
>
> why are you removing this? I put this in so that really only supported platforms are tried. We want to fail if you run on hardware that isn’t officially tested with this driver.

It is not really removed it actually checked on btintel_version_info
or btintel_version_info_tlv so we keep the checks in one place.

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz