Hi Luiz,
> This checking the firmware build number, week and years matches and then
> skip the download process.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> ---
> 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.
> 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.
> /* 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.
> }
> 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
