2021-02-11 01:17:06

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH v8 1/6] Bluetooth: btintel: Check firmware version before download

From: Luiz Augusto von Dentz <[email protected]>

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 <[email protected]>
---
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.

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

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 88ce5f0ffc4b..4dba31a33ce4 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,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.
@@ -947,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) {
@@ -968,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;
@@ -978,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 66ada8217797..c92060e7472c 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2623,10 +2623,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.
*/
@@ -2817,8 +2824,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 */
--
2.26.2


2021-02-11 01:17:06

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH v8 2/6] Bluetooth: btintel: Move operational checks after version check

From: Luiz Augusto von Dentz <[email protected]>

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 <[email protected]>
---
drivers/bluetooth/btintel.c | 22 ++++++++++++
drivers/bluetooth/btusb.c | 69 +++++++++++++------------------------
2 files changed, 46 insertions(+), 45 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 4dba31a33ce4..8a9a2368453b 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -958,6 +958,17 @@ int btintel_download_firmware(struct hci_dev *hdev,
}
}

+ /* 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;
@@ -985,6 +996,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 c92060e7472c..115a8ccb1468 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2469,11 +2469,17 @@ 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 */
@@ -2493,9 +2499,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 +2557,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
@@ -2694,35 +2700,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
@@ -2741,16 +2718,18 @@ 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;
+ /* SfP and WsP don't seem to update the firmware version on file
+ * so version checking is currently possible.
+ */
+ switch (ver->hw_variant) {
+ case 0x0b: /* SfP */
+ case 0x0c: /* WsP */
+ return 0;
+ }
+
+ /* Proceed to download to check if the version matches */
+ goto download;
}

/* Read the secure boot parameters to identify the operating
@@ -2778,6 +2757,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.
@@ -2801,7 +2781,7 @@ 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) {
bt_dev_err(hdev, "Unsupported Intel firmware naming");
return -EINVAL;
}
@@ -2972,8 +2952,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)

err = btusb_setup_intel_new_get_fw_name(&ver, &params, ddcname,
sizeof(ddcname), "ddc");
-
- if (!err) {
+ if (err < 0) {
bt_dev_err(hdev, "Unsupported Intel firmware naming");
} else {
/* Once the device is running in operational mode, it needs to
--
2.26.2

2021-02-11 02:00:44

by bluez.test.bot

[permalink] [raw]
Subject: RE: [v8,1/6] Bluetooth: btintel: Check firmware version before download

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=431903

---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


Attachments:
l2cap-tester.log (42.33 kB)
bnep-tester.log (3.45 kB)
mgmt-tester.log (533.87 kB)
rfcomm-tester.log (11.38 kB)
sco-tester.log (9.66 kB)
smp-tester.log (11.52 kB)
userchan-tester.log (5.30 kB)
Download all attachments

2021-02-11 07:37:51

by Tedd Ho-Jeong An

[permalink] [raw]
Subject: Re: [PATCH v8 1/6] Bluetooth: btintel: Check firmware version before download

Hi Luiz,

On Wed, 2021-02-10 at 17:08 -0800, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
>
> drivers/bluetooth/btintel.c | 105 +++++++++++++++++++++++++++-------
> drivers/bluetooth/btintel.h | 5 +-
> drivers/bluetooth/btusb.c | 18 +++++-
> drivers/bluetooth/hci_intel.c | 7 ++-
> 4 files changed, 109 insertions(+), 26 deletions(-)
>

I tested with ThP and no issue found.
I also tested with WsP and fw loading fine and no upgrading as expected.

More devices will be tested. Stay tune.

Regards,
Tedd