| Message ID | 20210323185904.3372987-1-luiz.dentz@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v10,1/9] Bluetooth: btintel: Check firmware version before download | expand |
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=454135
---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, 165 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, 43 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 - 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
Hi Tedd, > On 3/23/21, 12:00 PM, "Luiz Augusto von Dentz" <luiz.dentz@gmail.com> 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. > v10: Fix build error when BT_CONFIG_INTEL is not set. > > 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 did a quick check with v10 and all tests passed/expected. > > Test Scenarios: > Cold boot: Expect to download firmware > Reboot: Expect no firmware downloading > FW upgrade: Expect to detect firmware change and download new firmware. > > ThP, TyP: All 3 tests passed > WsP, SfP: Cold boot, Reboot passed. FW upgrade didn't work due to known issue. > > Tested-by: Tedd Ho-Jeong An <tedd.an@intel.com> so the verdict is that we should go ahead and apply this set? Regards Marcel
Hi Marcel,
On 3/24/21, 12:05 AM, "Marcel Holtmann" <marcel@holtmann.org> wrote:
Hi Tedd,
> On 3/23/21, 12:00 PM, "Luiz Augusto von Dentz" <luiz.dentz@gmail.com> 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.
> v10: Fix build error when BT_CONFIG_INTEL is not set.
>
> 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 did a quick check with v10 and all tests passed/expected.
>
> Test Scenarios:
> Cold boot: Expect to download firmware
> Reboot: Expect no firmware downloading
> FW upgrade: Expect to detect firmware change and download new firmware.
>
> ThP, TyP: All 3 tests passed
> WsP, SfP: Cold boot, Reboot passed. FW upgrade didn't work due to known issue.
>
> Tested-by: Tedd Ho-Jeong An <tedd.an@intel.com>
so the verdict is that we should go ahead and apply this set?
Yes, it is good to go.
I am sorry that I should be more clear in my last report. The behavior of WsP and SfP
remains the same with or without this set. WsP and SfP will be updated to new
firmware only after the device reset to the bootloader like a cold boot.
For the newer devices, this set will download the new firmware even with a normal reboot.
In case of WsP and SfP, the current firmware files for those two devices don't have
the correct firmware file version and there is no way to tell the difference between
the firmware loaded in the device and in the file. So, this set explicitly excludes those
two devices from the firmware upgrade.
Regards
Marcel
Hi Luiz, > 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. > v10: Fix build error when BT_CONFIG_INTEL is not set. > > 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(-) all 9 patches have been applied to bluetooth-next tree. Regards Marcel
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 0d355bb45e08..de220ab7f231 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -2557,10 +2557,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. */ @@ -2752,8 +2759,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 */