2021-03-15 17:50:44

by Luiz Augusto von Dentz

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

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

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 <[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.
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(-)

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 */
--
2.30.2


2021-03-15 17:50:44

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH v9 7/9] Bluetooth: btintel: Reorganized bootloader mode tlv checks in intel_version_tlv parsing

From: Lokendra Singh <[email protected]>

This moves limited_cce and sbe_type checks under bootloader during tlv parsing
as operational firmware don't have access to these values. Any attempt to read
such values in operational firmware will only fetch garbage data.

Signed-off-by: Lokendra Singh <[email protected]>
Signed-off-by: Kiran K <[email protected]>
---
drivers/bluetooth/btintel.c | 34 +++++++++++++++++-----------------
drivers/bluetooth/btintel.h | 8 ++++----
2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index bddaa4f32566..4ddbf895c382 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -434,26 +434,26 @@ int btintel_version_info_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver
return -EINVAL;
}

- /* It is required that every single firmware fragment is acknowledged
- * with a command complete event. If the boot parameters indicate
- * that this bootloader does not send them, then abort the setup.
- */
- if (version->limited_cce != 0x00) {
- bt_dev_err(hdev, "Unsupported Intel firmware loading method (0x%x)",
- version->limited_cce);
- return -EINVAL;
- }
-
- /* Secure boot engine type should be either 1 (ECDSA) or 0 (RSA) */
- if (version->sbe_type > 0x01) {
- bt_dev_err(hdev, "Unsupported Intel secure boot engine type (0x%x)",
- version->sbe_type);
- return -EINVAL;
- }
-
switch (version->img_type) {
case 0x01:
variant = "Bootloader";
+ /* It is required that every single firmware fragment is acknowledged
+ * with a command complete event. If the boot parameters indicate
+ * that this bootloader does not send them, then abort the setup.
+ */
+ if (version->limited_cce != 0x00) {
+ bt_dev_err(hdev, "Unsupported Intel firmware loading method (0x%x)",
+ version->limited_cce);
+ return -EINVAL;
+ }
+
+ /* Secure boot engine type should be either 1 (ECDSA) or 0 (RSA) */
+ if (version->sbe_type > 0x01) {
+ bt_dev_err(hdev, "Unsupported Intel secure boot engine type (0x%x)",
+ version->sbe_type);
+ return -EINVAL;
+ }
+
bt_dev_info(hdev, "Device revision is %u", version->dev_rev_id);
bt_dev_info(hdev, "Secure boot is %s",
version->secure_boot ? "enabled" : "disabled");
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index 7163170410a8..b34165a474c5 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -211,13 +211,13 @@ static inline void btintel_hw_error(struct hci_dev *hdev, u8 code)
{
}

-static inline void btintel_version_info(struct hci_dev *hdev,
- struct intel_version *ver)
+static inline int btintel_version_info(struct hci_dev *hdev,
+ struct intel_version *ver)
{
}

-static inline void btintel_version_info_tlv(struct hci_dev *hdev,
- struct intel_version_tlv *version)
+static inline int btintel_version_info_tlv(struct hci_dev *hdev,
+ struct intel_version_tlv *version)
{
}

--
2.30.2

2021-03-15 17:50:45

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH v9 8/9] Bluetooth: btintel: Collect tlv based active firmware build info in FW mode

From: Lokendra Singh <[email protected]>

In Operational firmware mode, 'Minimum FW version' TLV ID is not available.
So, we cannot fetch already running firmware info for comparison against
another build. However, It can be collected using a combination of other
TLV ID's information.

Signed-off-by: Lokendra Singh <[email protected]>
---
drivers/bluetooth/btintel.c | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 4ddbf895c382..6442acba76d1 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -537,12 +537,23 @@ int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver
version->img_type = tlv->val[0];
break;
case INTEL_TLV_TIME_STAMP:
+ /* If image type is Operational firmware (0x03), then
+ * running FW Calendar Week and Year information can
+ * be extracted from Timestamp information
+ */
+ version->min_fw_build_cw = tlv->val[0];
+ version->min_fw_build_yy = tlv->val[1];
version->timestamp = get_unaligned_le16(tlv->val);
break;
case INTEL_TLV_BUILD_TYPE:
version->build_type = tlv->val[0];
break;
case INTEL_TLV_BUILD_NUM:
+ /* If image type is Operational firmware (0x03), then
+ * running FW build number can be extracted from the
+ * Build information
+ */
+ version->min_fw_build_nn = tlv->val[0];
version->build_num = get_unaligned_le32(tlv->val);
break;
case INTEL_TLV_SECURE_BOOT:
--
2.30.2

2021-03-15 20:39:23

by bluez.test.bot

[permalink] [raw]
Subject: RE: [v9,1/9] 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=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


Attachments:
l2cap-tester.log (42.33 kB)
bnep-tester.log (3.45 kB)
mgmt-tester.log (533.86 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-03-15 23:06:48

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH v9 4/9] Bluetooth: btintel: Consolidate intel_version parsing

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

This moves version checks of intel_version() to btintel_version_info().

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
drivers/bluetooth/btintel.c | 36 ++++++++++++++++++++++++++++++++++--
drivers/bluetooth/btintel.h | 2 +-
drivers/bluetooth/btusb.c | 12 ++++--------
3 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index bb2546c4374a..bddaa4f32566 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -216,10 +216,39 @@ void btintel_hw_error(struct hci_dev *hdev, u8 code)
}
EXPORT_SYMBOL_GPL(btintel_hw_error);

-void btintel_version_info(struct hci_dev *hdev, struct intel_version *ver)
+int btintel_version_info(struct hci_dev *hdev, struct intel_version *ver)
{
const char *variant;

+ /* 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;
+ }
+
switch (ver->fw_variant) {
case 0x06:
variant = "Bootloader";
@@ -228,13 +257,16 @@ void btintel_version_info(struct hci_dev *hdev, struct intel_version *ver)
variant = "Firmware";
break;
default:
- return;
+ bt_dev_err(hdev, "Unsupported firmware variant(%02x)", ver->fw_variant);
+ return -EINVAL;
}

bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
variant, ver->fw_revision >> 4, ver->fw_revision & 0x0f,
ver->fw_build_num, ver->fw_build_ww,
2000 + ver->fw_build_yy);
+
+ return 0;
}
EXPORT_SYMBOL_GPL(btintel_version_info);

diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index 94a63e898826..7163170410a8 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -148,7 +148,7 @@ int btintel_set_diag(struct hci_dev *hdev, bool enable);
int btintel_set_diag_mfg(struct hci_dev *hdev, bool enable);
void btintel_hw_error(struct hci_dev *hdev, u8 code);

-void btintel_version_info(struct hci_dev *hdev, struct intel_version *ver);
+int btintel_version_info(struct hci_dev *hdev, struct intel_version *ver);
int btintel_version_info_tlv(struct hci_dev *hdev, struct intel_version_tlv *version);
int btintel_secure_send(struct hci_dev *hdev, u8 fragment_type, u32 plen,
const void *param);
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 2a1926552a50..dfc743e8fdf4 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2465,12 +2465,6 @@ static int btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
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 */
@@ -2640,8 +2634,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
if (!ver || !params)
return -EINVAL;

- btintel_version_info(hdev, ver);
-
/* 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
@@ -2834,6 +2826,10 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
return err;
}

+ err = btintel_version_info(hdev, &ver);
+ if (err)
+ return err;
+
err = btusb_intel_download_firmware(hdev, &ver, &params, &boot_param);
if (err)
return err;
--
2.30.2

2021-03-16 04:51:38

by Tedd Ho-Jeong An

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

Hi Luiz,

On Mon, 2021-03-15 at 10:39 -0700, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> 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 <[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.
> 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 <[email protected]>
Tested-by: Kiran K <[email protected]>

Regards,
Tedd





2021-03-16 20:31:19

by Marcel Holtmann

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

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 <[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.
>> 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 <[email protected]>
> Tested-by: Kiran K <[email protected]>

so I should go ahead and apply these patches?

Regards

Marcel

2021-03-17 10:13:26

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v9 7/9] Bluetooth: btintel: Reorganized bootloader mode tlv checks in intel_version_tlv parsing

Hi Luiz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on next-20210316]
[cannot apply to v5.12-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Luiz-Augusto-von-Dentz/Bluetooth-btintel-Check-firmware-version-before-download/20210316-014342
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: x86_64-randconfig-a011-20210317 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8ef111222a3dd12a9175f69c3bff598c46e8bdf7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/a6cfd08052b877b40b8fe958148f5d0da7e5cff7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Luiz-Augusto-von-Dentz/Bluetooth-btintel-Check-firmware-version-before-download/20210316-014342
git checkout a6cfd08052b877b40b8fe958148f5d0da7e5cff7
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

In file included from drivers/bluetooth/hci_ldisc.c:34:
>> drivers/bluetooth/btintel.h:217:1: error: non-void function does not return a value [-Werror,-Wreturn-type]
}
^
drivers/bluetooth/btintel.h:222:1: error: non-void function does not return a value [-Werror,-Wreturn-type]
}
^
2 errors generated.


vim +217 drivers/bluetooth/btintel.h

973bb97e5aee56 Marcel Holtmann 2015-07-05 213
a6cfd08052b877 Lokendra Singh 2021-03-15 214 static inline int btintel_version_info(struct hci_dev *hdev,
0eee53cdd98577 Vincent Stehl? 2015-09-03 215 struct intel_version *ver)
7feb99e1308204 Marcel Holtmann 2015-07-05 216 {
7feb99e1308204 Marcel Holtmann 2015-07-05 @217 }
7feb99e1308204 Marcel Holtmann 2015-07-05 218

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (2.57 kB)
.config.gz (34.78 kB)
Download all attachments

2021-03-23 18:55:38

by Luiz Augusto von Dentz

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

Hi Marcel,

On Tue, Mar 16, 2021 at 7:01 AM Marcel Holtmann <[email protected]> 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 <[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.
> >> 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 <[email protected]>
> > Tested-by: Kiran K <[email protected]>
>
> 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
>


--
Luiz Augusto von Dentz