diff mbox series

[v9,1/9] Bluetooth: btintel: Check firmware version before download

Message ID 20210315174002.1778447-1-luiz.dentz@gmail.com (mailing list archive)
State Accepted
Delegated to: Marcel Holtmann
Headers show
Series [v9,1/9] Bluetooth: btintel: Check firmware version before download | expand

Commit Message

Luiz Augusto von Dentz March 15, 2021, 5:39 p.m. UTC 
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This checks the firmware build number, week and year against the
repective loaded version. If details are a match, 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.
v4: Fix forwarding -EALREADY when firmware has already been loaded.
v5: Fix not advancing fw_ptr.
v6: Fix btusb_setup_intel_new_get_fw_name error checking for ddc.
v7: Disable version checking for WsP/SfP.
v8: Really disables version checking for WsP/SfP.
v9: Reintroduce bootloader checks and add workaround for version checking when
operation and version cannot be read.

 drivers/bluetooth/btintel.c   | 106 +++++++++++++++++++++++++++-------
 drivers/bluetooth/btintel.h   |   5 +-
 drivers/bluetooth/btusb.c     |  18 +++++-
 drivers/bluetooth/hci_intel.c |   7 ++-
 4 files changed, 109 insertions(+), 27 deletions(-)

Comments

bluez.test.bot@gmail.com March 15, 2021, 6:25 p.m. UTC | #1 
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=448497

---Test result---

##############################
    Test: CheckPatch - FAIL
    Bluetooth: btintel: Consolidate intel_version_tlv parsing
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#6: 
This moves version checks of intel_version_tlv() to btintel_version_info_tlv().

total: 0 errors, 1 warnings, 153 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: btintel: Consolidate intel_version_tlv parsing" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Bluetooth: btintel: Reorganized bootloader mode tlv checks in intel_version_tlv parsing
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7: 
This moves limited_cce and sbe_type checks under bootloader during tlv parsing

total: 0 errors, 1 warnings, 60 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: btintel: Reorganized bootloader mode tlv checks in" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


    ##############################
    Test: CheckGitLint - FAIL
    Bluetooth: btintel: Reorganized bootloader mode tlv checks in intel_version_tlv parsing
1: T1 Title exceeds max length (87>72): "Bluetooth: btintel: Reorganized bootloader mode tlv checks in intel_version_tlv parsing"

Bluetooth: btintel: Collect tlv based active firmware build info in FW mode
1: T1 Title exceeds max length (75>72): "Bluetooth: btintel: Collect tlv based active firmware build info in FW mode"

Bluetooth: btintel: Skip reading firmware file version while in bootloader mode
1: T1 Title exceeds max length (79>72): "Bluetooth: btintel: Skip reading firmware file version while in bootloader mode"


    ##############################
    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 - FAIL
    Total: 416, Passed: 397 (95.4%), Failed: 3, Not Run: 16

    ##############################
    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
Tedd Ho-Jeong An March 15, 2021, 8:52 p.m. UTC | #2 
Hi Luiz,

On Mon, 2021-03-15 at 10:39 -0700, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This checks the firmware build number, week and year against the
> repective loaded version. If details are a match, 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.
> v4: Fix forwarding -EALREADY when firmware has already been loaded.
> v5: Fix not advancing fw_ptr.
> v6: Fix btusb_setup_intel_new_get_fw_name error checking for ddc.
> v7: Disable version checking for WsP/SfP.
> v8: Really disables version checking for WsP/SfP.
> v9: Reintroduce bootloader checks and add workaround for version checking when
> operation and version cannot be read.
> 
>  drivers/bluetooth/btintel.c   | 106 +++++++++++++++++++++++++++-------
>  drivers/bluetooth/btintel.h   |   5 +-
>  drivers/bluetooth/btusb.c     |  18 +++++-
>  drivers/bluetooth/hci_intel.c |   7 ++-
>  4 files changed, 109 insertions(+), 27 deletions(-)

I ran a quick test the v9 with the devices what I have.

Test cases are:
- cold boot - expect to downloading the firmware
- reboot - expect to no firmware downloading
- fw upgrade - expect to device reset and download new firmware

Devices tests:
SfP, WsP, ThP, TyP

Results:
ThP, TyP: All 3 test cases were passed.
SfP, WsP: fw upgrade case(#3) didn't work but it was known issue
		- insufficient fw version information in the firmware file

Tested-by: Tedd Ho-Jeong An <tedd.an@intel.com>
Tested-by: Kiran K <kiran.k@intel.com>

Regards,
Tedd
Marcel Holtmann March 16, 2021, 2:01 p.m. UTC | #3 
Hi Tedd,

>> This checks the firmware build number, week and year against the
>> repective loaded version. If details are a match, 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.
>> v4: Fix forwarding -EALREADY when firmware has already been loaded.
>> v5: Fix not advancing fw_ptr.
>> v6: Fix btusb_setup_intel_new_get_fw_name error checking for ddc.
>> v7: Disable version checking for WsP/SfP.
>> v8: Really disables version checking for WsP/SfP.
>> v9: Reintroduce bootloader checks and add workaround for version checking when
>> operation and version cannot be read.
>> 
>> drivers/bluetooth/btintel.c   | 106 +++++++++++++++++++++++++++-------
>> drivers/bluetooth/btintel.h   |   5 +-
>> drivers/bluetooth/btusb.c     |  18 +++++-
>> drivers/bluetooth/hci_intel.c |   7 ++-
>> 4 files changed, 109 insertions(+), 27 deletions(-)
> 
> I ran a quick test the v9 with the devices what I have.
> 
> Test cases are:
> - cold boot - expect to downloading the firmware
> - reboot - expect to no firmware downloading
> - fw upgrade - expect to device reset and download new firmware
> 
> Devices tests:
> SfP, WsP, ThP, TyP
> 
> Results:
> ThP, TyP: All 3 test cases were passed.
> SfP, WsP: fw upgrade case(#3) didn't work but it was known issue
> 		- insufficient fw version information in the firmware file
> 
> Tested-by: Tedd Ho-Jeong An <tedd.an@intel.com>
> Tested-by: Kiran K <kiran.k@intel.com>

so I should go ahead and apply these patches?

Regards

Marcel
Luiz Augusto von Dentz March 23, 2021, 6:49 p.m. UTC | #4 
Hi Marcel,

On Tue, Mar 16, 2021 at 7:01 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Tedd,
>
> >> This checks the firmware build number, week and year against the
> >> repective loaded version. If details are a match, 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.
> >> v4: Fix forwarding -EALREADY when firmware has already been loaded.
> >> v5: Fix not advancing fw_ptr.
> >> v6: Fix btusb_setup_intel_new_get_fw_name error checking for ddc.
> >> v7: Disable version checking for WsP/SfP.
> >> v8: Really disables version checking for WsP/SfP.
> >> v9: Reintroduce bootloader checks and add workaround for version checking when
> >> operation and version cannot be read.
> >>
> >> drivers/bluetooth/btintel.c   | 106 +++++++++++++++++++++++++++-------
> >> drivers/bluetooth/btintel.h   |   5 +-
> >> drivers/bluetooth/btusb.c     |  18 +++++-
> >> drivers/bluetooth/hci_intel.c |   7 ++-
> >> 4 files changed, 109 insertions(+), 27 deletions(-)
> >
> > I ran a quick test the v9 with the devices what I have.
> >
> > Test cases are:
> > - cold boot - expect to downloading the firmware
> > - reboot - expect to no firmware downloading
> > - fw upgrade - expect to device reset and download new firmware
> >
> > Devices tests:
> > SfP, WsP, ThP, TyP
> >
> > Results:
> > ThP, TyP: All 3 test cases were passed.
> > SfP, WsP: fw upgrade case(#3) didn't work but it was known issue
> >               - insufficient fw version information in the firmware file
> >
> > Tested-by: Tedd Ho-Jeong An <tedd.an@intel.com>
> > Tested-by: Kiran K <kiran.k@intel.com>
>
> so I should go ahead and apply these patches?

I will send a v10 shortly, there was a build problem when
CONFIG_BT_INTEL is not set.

> Regards
>
> Marcel
>
diff mbox series

Patch

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index fa97324454ea..3ff698a0bd25 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,21 +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 + frag_len +
-							 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
@@ -897,28 +890,101 @@  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;
+
+	fw_ptr = fw->data;
+
+	while (fw_ptr - fw->data < fw->size) {
+		struct hci_command_hdr *cmd = (void *)(fw_ptr);
+
+		/* 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_info(hdev, "Boot Address: 0x%x",
+				    le32_to_cpu(params->boot_addr));
+
+			bt_dev_info(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);
+		}
+
+		fw_ptr += 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;
 
+	/* SfP and WsP don't seem to update the firmware version on file
+	 * so version checking is currently not possible.
+	 */
+	switch (ver->hw_variant) {
+	case 0x0b:	/* SfP */
+	case 0x0c:	/* WsP */
+		/* Skip version checking */
+		break;
+	default:
+		/* 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)) {
+			bt_dev_info(hdev, "Firmware already loaded");
+			/* 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)) {
+		bt_dev_info(hdev, "Firmware already loaded");
+		/* 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.
@@ -948,7 +1014,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) {
@@ -969,7 +1035,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;
@@ -979,7 +1044,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 d2ef06141774..e004bfdc2ce2 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2615,10 +2615,17 @@  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);
+			err = 0;
+			goto done;
+		}
+
 		/* When FW download fails, send Intel Reset to retry
 		 * FW download.
 		 */
@@ -2810,8 +2817,15 @@  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);
+			err = 0;
+			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 */