Re: [PATCH] Bluetooth: btintel: Check firmware version before download
From: Marcel Holtmann <marcel@holtmann.org>
Date: 2021-02-04 12:56:54
Hi Luiz,
quoted hunk ↗ jump to hunk
This checking the firmware build number, week and years matches and then skip the download process. Signed-off-by: Luiz Augusto von Dentz <redacted> --- drivers/bluetooth/btintel.c | 92 +++++++++++++++++++++++++++-------- drivers/bluetooth/btintel.h | 5 +- drivers/bluetooth/btusb.c | 28 +++++++---- drivers/bluetooth/hci_intel.c | 7 ++- 4 files changed, 98 insertions(+), 34 deletions(-)diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c index 88ce5f0ffc4b..7853ff4df237 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; +
it should be __packed here. Didn’t know that _packed exists.
quoted hunk ↗ jump to hunk
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,88 @@ 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; +
In these cases we put the { } around it to make it visually more clear. And I would actually shorten the comment to “Return here to indicate that the firmware has already been loaded */. The EALREADY is self-explanatory.
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;
+Same as above.
quoted hunk ↗ jump to hunk
/* 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 +1001,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 +1022,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 +1031,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 4266c057746e..46a6795198da 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c@@ -2612,14 +2612,18 @@ 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) { - /* When FW download fails, send Intel Reset to retry - * FW download. - */ - btintel_reset_to_bootloader(hdev); + /* -EALREADY indicates the firmware has already been loaded */ + if (err == -EALREADY) + set_bit(BTUSB_FIRMWARE_LOADED, &data->flags); + else + /* When FW download fails, send Intel Reset to retry + * FW download. + */ + btintel_reset_to_bootloader(hdev); goto done; } set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);@@ -2806,12 +2810,16 @@ 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) { - /* When FW download fails, send Intel Reset to retry - * FW download. - */ - btintel_reset_to_bootloader(hdev); + /* -EALREADY indicates the firmware has already been loaded */
+ if (err == -EALREADY) + set_bit(BTUSB_FIRMWARE_LOADED, &data->flags); + else + /* When FW download fails, send Intel Reset to retry + * FW download. + */ + btintel_reset_to_bootloader(hdev); goto done;
if (err == -EALREADY) {
/* Firmware has already been loaded */
set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);
goto done;
}
/* If the firmware download fails, send Intel Reset to retry
* the download. */
btintel_reset_to_bootloader(hdev);
goto done;
This way it is just a lot easy to read in case we ever need to look at this part of the code again.
quoted hunk ↗ jump to hunk
} set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);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 */
Regards Marcel