2020-10-29 09:00:52

by Sathish Narasimman

[permalink] [raw]
Subject: [PATCH v2] Bluetooth: btintel parse TLV structure

Latest intel firmware supports TLV structure in operational mode for intel
read version. so made changes accordingly to support both bootloader
and operational mode . These changes are only to specific intel bluetooth
controller for example ThP, CcP.

Signed-off-by: Sathish Narasimman <[email protected]>
---
drivers/bluetooth/btintel.c | 105 +++++++++++++++++++++++++++---------
drivers/bluetooth/btintel.h | 16 ++++++
drivers/bluetooth/btusb.c | 41 ++++++++++----
3 files changed, 129 insertions(+), 33 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 88ce5f0ffc4b..67267afc83e1 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -401,31 +401,9 @@ void btintel_version_info_tlv(struct hci_dev *hdev, struct intel_version_tlv *ve
}
EXPORT_SYMBOL_GPL(btintel_version_info_tlv);

-int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *version)
+static void btintel_parse_tlv(struct sk_buff *skb,
+ struct intel_version_tlv *version)
{
- struct sk_buff *skb;
- const u8 param[1] = { 0xFF };
-
- if (!version)
- return -EINVAL;
-
- skb = __hci_cmd_sync(hdev, 0xfc05, 1, param, HCI_CMD_TIMEOUT);
- if (IS_ERR(skb)) {
- bt_dev_err(hdev, "Reading Intel version information failed (%ld)",
- PTR_ERR(skb));
- return PTR_ERR(skb);
- }
-
- if (skb->data[0]) {
- bt_dev_err(hdev, "Intel Read Version command failed (%02x)",
- skb->data[0]);
- kfree_skb(skb);
- return -EIO;
- }
-
- /* Consume Command Complete Status field */
- skb_pull(skb, 1);
-
/* Event parameters contatin multiple TLVs. Read each of them
* and only keep the required data. Also, it use existing legacy
* version field like hw_platform, hw_variant, and fw_variant
@@ -496,6 +474,85 @@ int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver
/* consume the current tlv and move to next*/
skb_pull(skb, tlv->len + sizeof(*tlv));
}
+}
+
+int btintel_read_version_new(struct hci_dev *hdev, struct btintel_version *ver)
+{
+ struct sk_buff *skb;
+ struct intel_version *version = &ver->ver;
+ const u8 param[1] = { 0xFF };
+
+ skb = __hci_cmd_sync(hdev, 0xfc05, 1, param, HCI_CMD_TIMEOUT);
+ if (IS_ERR(skb)) {
+ bt_dev_err(hdev, "Reading Intel version info failed (%ld)",
+ PTR_ERR(skb));
+ return PTR_ERR(skb);
+ }
+
+ if (skb->data[0]) {
+ bt_dev_err(hdev, "Intel Read Version command failed (%02x)",
+ skb->data[0]);
+ kfree_skb(skb);
+ return -EIO;
+ }
+
+ /* The new Intel read version is backward compatible for Thp and CcP
+ * type cards. when the controller is in bootloader mode the controller
+ * response remains same as old intel_read version. For ThP/CcP cards
+ * TLV structure supports only during the Operation Mode. The best way
+ * to differentiate the read_version response is to check the length
+ * parameter and the first byte of the payload, which is a fixed value.
+ * After the status parameter if the payload starts with 0x37(This is
+ * a fixed value) and length of the payload is 10 then it is identified
+ * as legacy struct intel_version. In the latest firmware the support
+ * of TLV structure is added during Operational Firmware.
+ */
+ if (skb->len == sizeof(*version) && skb->data[1] == 0x37) {
+ memcpy(version, skb->data, sizeof(*version));
+ ver->tlv_format = false;
+ goto finish;
+ }
+
+ /* Consume Command Complete Status field */
+ skb_pull(skb, 1);
+
+ ver->tlv_format = true;
+
+ bt_dev_info(hdev, "Parsing TLV Supported intel read version");
+ btintel_parse_tlv(skb, &ver->ver_tlv);
+
+finish:
+ kfree_skb(skb);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(btintel_read_version_new);
+
+int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *version)
+{
+ struct sk_buff *skb;
+ const u8 param[1] = { 0xFF };
+
+ if (!version)
+ return -EINVAL;
+
+ skb = __hci_cmd_sync(hdev, 0xfc05, 1, param, HCI_CMD_TIMEOUT);
+ if (IS_ERR(skb)) {
+ bt_dev_err(hdev, "Reading Intel version information failed (%ld)",
+ PTR_ERR(skb));
+ return PTR_ERR(skb);
+ }
+
+ if (skb->data[0]) {
+ bt_dev_err(hdev, "Intel Read Version command failed (%02x)",
+ skb->data[0]);
+ kfree_skb(skb);
+ return -EIO;
+ }
+
+ /* Consume Command Complete Status field */
+ skb_pull(skb, 1);
+
+ btintel_parse_tlv(skb, version);

kfree_skb(skb);
return 0;
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index 09346ae308eb..952da44b79de 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -132,6 +132,15 @@ struct intel_debug_features {
__u8 page1[16];
} __packed;

+struct btintel_version {
+ bool tlv_format;
+ union {
+ struct intel_version ver; /*Legacy Intel read version*/
+ struct intel_version_tlv ver_tlv;
+ };
+} __packed;
+
+#define INTEL_HW_VARIANT(cnvx_bt) ((u8)(((cnvx_bt) & 0x003f0000) >> 16))
#if IS_ENABLED(CONFIG_BT_INTEL)

int btintel_check_bdaddr(struct hci_dev *hdev);
@@ -151,6 +160,7 @@ int btintel_set_event_mask(struct hci_dev *hdev, bool debug);
int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug);
int btintel_read_version(struct hci_dev *hdev, struct intel_version *ver);
int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver);
+int btintel_read_version_new(struct hci_dev *hdev, struct btintel_version *ver);

struct regmap *btintel_regmap_init(struct hci_dev *hdev, u16 opcode_read,
u16 opcode_write);
@@ -248,6 +258,12 @@ static inline int btintel_read_version_tlv(struct hci_dev *hdev,
return -EOPNOTSUPP;
}

+static inline int btintel_read_version_new(struct hci_dev *hdev,
+ struct btintel_version *ver)
+{
+ return -EOPNOTSUPP;
+}
+
static inline struct regmap *btintel_regmap_init(struct hci_dev *hdev,
u16 opcode_read,
u16 opcode_write)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 1005b6e8ff74..c63bc8a0c84f 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2554,7 +2554,8 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
static int btusb_setup_intel_new(struct hci_dev *hdev)
{
struct btusb_data *data = hci_get_drvdata(hdev);
- struct intel_version ver;
+ struct btintel_version bt_ver;
+ u8 hw_variant;
struct intel_boot_params params;
u32 boot_param;
char ddcname[64];
@@ -2577,19 +2578,33 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
* is in bootloader mode or if it already has operational firmware
* loaded.
*/
- err = btintel_read_version(hdev, &ver);
+ err = btintel_read_version_new(hdev, &bt_ver);
if (err) {
bt_dev_err(hdev, "Intel Read version failed (%d)", err);
btintel_reset_to_bootloader(hdev);
return err;
}

- err = btusb_intel_download_firmware(hdev, &ver, &params, &boot_param);
+ /* If TLV format is supported then it is in Operational Firmware. TLV
+ * structure is supported only in operational mode in latest Firmware.
+ */
+ if (bt_ver.tlv_format && bt_ver.ver_tlv.img_type == 0x03) {
+ btintel_version_info_tlv(hdev, &bt_ver.ver_tlv);
+ clear_bit(BTUSB_BOOTLOADER, &data->flags);
+ goto finish;
+ }
+
+ err = btusb_intel_download_firmware(hdev, &bt_ver.ver, &params,
+ &boot_param);
if (err)
return err;

- /* controller is already having an operational firmware */
- if (ver.fw_variant == 0x23)
+ /* In case if old firmware is used, it should be backward compatible
+ * to check for operational firmware. only on latest firmware the
+ * support for TLV structure is added. so check for tlv is not
+ * required here.
+ */
+ if (bt_ver.ver.fw_variant == 0x23)
goto finish;

rettime = ktime_get();
@@ -2641,7 +2656,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)

clear_bit(BTUSB_BOOTLOADER, &data->flags);

- err = btusb_setup_intel_new_get_fw_name(&ver, &params, ddcname,
+ err = btusb_setup_intel_new_get_fw_name(&bt_ver.ver, &params, ddcname,
sizeof(ddcname), "ddc");

if (!err) {
@@ -2665,17 +2680,25 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
btintel_set_debug_features(hdev, &features);

/* Read the Intel version information after loading the FW */
- err = btintel_read_version(hdev, &ver);
+ err = btintel_read_version_new(hdev, &bt_ver);
if (err)
return err;

- btintel_version_info(hdev, &ver);
+ if (bt_ver.tlv_format)
+ btintel_version_info_tlv(hdev, &bt_ver.ver_tlv);
+ else
+ btintel_version_info(hdev, &bt_ver.ver);

finish:
/* All Intel controllers that support the Microsoft vendor
* extension are using 0xFC1E for VsMsftOpCode.
*/
- switch (ver.hw_variant) {
+ if (!bt_ver.tlv_format)
+ hw_variant = bt_ver.ver.hw_variant;
+ else
+ hw_variant = INTEL_HW_VARIANT(bt_ver.ver_tlv.cnvi_bt);
+
+ switch (hw_variant) {
case 0x12: /* ThP */
hci_set_msft_opcode(hdev, 0xFC1E);
break;
--
2.17.1


2020-11-03 02:04:48

by Sathish Narasimman

[permalink] [raw]
Subject: Re: [PATCH v2] Bluetooth: btintel parse TLV structure

Hi


On Thu, Oct 29, 2020 at 11:47 AM Sathish Narasimman
<[email protected]> wrote:
>
> Latest intel firmware supports TLV structure in operational mode for intel
> read version. so made changes accordingly to support both bootloader
> and operational mode . These changes are only to specific intel bluetooth
> controller for example ThP, CcP.
>
> Signed-off-by: Sathish Narasimman <[email protected]>
> ---
> drivers/bluetooth/btintel.c | 105 +++++++++++++++++++++++++++---------
> drivers/bluetooth/btintel.h | 16 ++++++
> drivers/bluetooth/btusb.c | 41 ++++++++++----
> 3 files changed, 129 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 88ce5f0ffc4b..67267afc83e1 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -401,31 +401,9 @@ void btintel_version_info_tlv(struct hci_dev *hdev, struct intel_version_tlv *ve
> }
> EXPORT_SYMBOL_GPL(btintel_version_info_tlv);
>
> -int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *version)
> +static void btintel_parse_tlv(struct sk_buff *skb,
> + struct intel_version_tlv *version)
> {
> - struct sk_buff *skb;
> - const u8 param[1] = { 0xFF };
> -
> - if (!version)
> - return -EINVAL;
> -
> - skb = __hci_cmd_sync(hdev, 0xfc05, 1, param, HCI_CMD_TIMEOUT);
> - if (IS_ERR(skb)) {
> - bt_dev_err(hdev, "Reading Intel version information failed (%ld)",
> - PTR_ERR(skb));
> - return PTR_ERR(skb);
> - }
> -
> - if (skb->data[0]) {
> - bt_dev_err(hdev, "Intel Read Version command failed (%02x)",
> - skb->data[0]);
> - kfree_skb(skb);
> - return -EIO;
> - }
> -
> - /* Consume Command Complete Status field */
> - skb_pull(skb, 1);
> -
> /* Event parameters contatin multiple TLVs. Read each of them
> * and only keep the required data. Also, it use existing legacy
> * version field like hw_platform, hw_variant, and fw_variant
> @@ -496,6 +474,85 @@ int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver
> /* consume the current tlv and move to next*/
> skb_pull(skb, tlv->len + sizeof(*tlv));
> }
> +}
> +
> +int btintel_read_version_new(struct hci_dev *hdev, struct btintel_version *ver)
> +{
> + struct sk_buff *skb;
> + struct intel_version *version = &ver->ver;
> + const u8 param[1] = { 0xFF };
> +
> + skb = __hci_cmd_sync(hdev, 0xfc05, 1, param, HCI_CMD_TIMEOUT);
> + if (IS_ERR(skb)) {
> + bt_dev_err(hdev, "Reading Intel version info failed (%ld)",
> + PTR_ERR(skb));
> + return PTR_ERR(skb);
> + }
> +
> + if (skb->data[0]) {
> + bt_dev_err(hdev, "Intel Read Version command failed (%02x)",
> + skb->data[0]);
> + kfree_skb(skb);
> + return -EIO;
> + }
> +
> + /* The new Intel read version is backward compatible for Thp and CcP
> + * type cards. when the controller is in bootloader mode the controller
> + * response remains same as old intel_read version. For ThP/CcP cards
> + * TLV structure supports only during the Operation Mode. The best way
> + * to differentiate the read_version response is to check the length
> + * parameter and the first byte of the payload, which is a fixed value.
> + * After the status parameter if the payload starts with 0x37(This is
> + * a fixed value) and length of the payload is 10 then it is identified
> + * as legacy struct intel_version. In the latest firmware the support
> + * of TLV structure is added during Operational Firmware.
> + */
> + if (skb->len == sizeof(*version) && skb->data[1] == 0x37) {
> + memcpy(version, skb->data, sizeof(*version));
> + ver->tlv_format = false;
> + goto finish;
> + }
> +
> + /* Consume Command Complete Status field */
> + skb_pull(skb, 1);
> +
> + ver->tlv_format = true;
> +
> + bt_dev_info(hdev, "Parsing TLV Supported intel read version");
> + btintel_parse_tlv(skb, &ver->ver_tlv);
> +
> +finish:
> + kfree_skb(skb);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(btintel_read_version_new);
> +
> +int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *version)
> +{
> + struct sk_buff *skb;
> + const u8 param[1] = { 0xFF };
> +
> + if (!version)
> + return -EINVAL;
> +
> + skb = __hci_cmd_sync(hdev, 0xfc05, 1, param, HCI_CMD_TIMEOUT);
> + if (IS_ERR(skb)) {
> + bt_dev_err(hdev, "Reading Intel version information failed (%ld)",
> + PTR_ERR(skb));
> + return PTR_ERR(skb);
> + }
> +
> + if (skb->data[0]) {
> + bt_dev_err(hdev, "Intel Read Version command failed (%02x)",
> + skb->data[0]);
> + kfree_skb(skb);
> + return -EIO;
> + }
> +
> + /* Consume Command Complete Status field */
> + skb_pull(skb, 1);
> +
> + btintel_parse_tlv(skb, version);
>
> kfree_skb(skb);
> return 0;
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index 09346ae308eb..952da44b79de 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -132,6 +132,15 @@ struct intel_debug_features {
> __u8 page1[16];
> } __packed;
>
> +struct btintel_version {
> + bool tlv_format;
> + union {
> + struct intel_version ver; /*Legacy Intel read version*/
> + struct intel_version_tlv ver_tlv;
> + };
> +} __packed;
> +
> +#define INTEL_HW_VARIANT(cnvx_bt) ((u8)(((cnvx_bt) & 0x003f0000) >> 16))
> #if IS_ENABLED(CONFIG_BT_INTEL)
>
> int btintel_check_bdaddr(struct hci_dev *hdev);
> @@ -151,6 +160,7 @@ int btintel_set_event_mask(struct hci_dev *hdev, bool debug);
> int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug);
> int btintel_read_version(struct hci_dev *hdev, struct intel_version *ver);
> int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver);
> +int btintel_read_version_new(struct hci_dev *hdev, struct btintel_version *ver);
>
> struct regmap *btintel_regmap_init(struct hci_dev *hdev, u16 opcode_read,
> u16 opcode_write);
> @@ -248,6 +258,12 @@ static inline int btintel_read_version_tlv(struct hci_dev *hdev,
> return -EOPNOTSUPP;
> }
>
> +static inline int btintel_read_version_new(struct hci_dev *hdev,
> + struct btintel_version *ver)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> static inline struct regmap *btintel_regmap_init(struct hci_dev *hdev,
> u16 opcode_read,
> u16 opcode_write)
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 1005b6e8ff74..c63bc8a0c84f 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2554,7 +2554,8 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
> static int btusb_setup_intel_new(struct hci_dev *hdev)
> {
> struct btusb_data *data = hci_get_drvdata(hdev);
> - struct intel_version ver;
> + struct btintel_version bt_ver;
> + u8 hw_variant;
> struct intel_boot_params params;
> u32 boot_param;
> char ddcname[64];
> @@ -2577,19 +2578,33 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> * is in bootloader mode or if it already has operational firmware
> * loaded.
> */
> - err = btintel_read_version(hdev, &ver);
> + err = btintel_read_version_new(hdev, &bt_ver);
> if (err) {
> bt_dev_err(hdev, "Intel Read version failed (%d)", err);
> btintel_reset_to_bootloader(hdev);
> return err;
> }
>
> - err = btusb_intel_download_firmware(hdev, &ver, &params, &boot_param);
> + /* If TLV format is supported then it is in Operational Firmware. TLV
> + * structure is supported only in operational mode in latest Firmware.
> + */
> + if (bt_ver.tlv_format && bt_ver.ver_tlv.img_type == 0x03) {
> + btintel_version_info_tlv(hdev, &bt_ver.ver_tlv);
> + clear_bit(BTUSB_BOOTLOADER, &data->flags);
> + goto finish;
> + }
> +
> + err = btusb_intel_download_firmware(hdev, &bt_ver.ver, &params,
> + &boot_param);
> if (err)
> return err;
>
> - /* controller is already having an operational firmware */
> - if (ver.fw_variant == 0x23)
> + /* In case if old firmware is used, it should be backward compatible
> + * to check for operational firmware. only on latest firmware the
> + * support for TLV structure is added. so check for tlv is not
> + * required here.
> + */
> + if (bt_ver.ver.fw_variant == 0x23)
> goto finish;
>
> rettime = ktime_get();
> @@ -2641,7 +2656,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
>
> clear_bit(BTUSB_BOOTLOADER, &data->flags);
>
> - err = btusb_setup_intel_new_get_fw_name(&ver, &params, ddcname,
> + err = btusb_setup_intel_new_get_fw_name(&bt_ver.ver, &params, ddcname,
> sizeof(ddcname), "ddc");
>
> if (!err) {
> @@ -2665,17 +2680,25 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> btintel_set_debug_features(hdev, &features);
>
> /* Read the Intel version information after loading the FW */
> - err = btintel_read_version(hdev, &ver);
> + err = btintel_read_version_new(hdev, &bt_ver);
> if (err)
> return err;
>
> - btintel_version_info(hdev, &ver);
> + if (bt_ver.tlv_format)
> + btintel_version_info_tlv(hdev, &bt_ver.ver_tlv);
> + else
> + btintel_version_info(hdev, &bt_ver.ver);
>
> finish:
> /* All Intel controllers that support the Microsoft vendor
> * extension are using 0xFC1E for VsMsftOpCode.
> */
> - switch (ver.hw_variant) {
> + if (!bt_ver.tlv_format)
> + hw_variant = bt_ver.ver.hw_variant;
> + else
> + hw_variant = INTEL_HW_VARIANT(bt_ver.ver_tlv.cnvi_bt);
> +
> + switch (hw_variant) {
> case 0x12: /* ThP */
> hci_set_msft_opcode(hdev, 0xFC1E);
> break;
> --
> 2.17.1
>

Gentle Reminder

Regards
Sathish N

2020-11-05 07:46:28

by bluez.test.bot

[permalink] [raw]
Subject: RE: [v2] Bluetooth: btintel parse TLV structure

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

---Test result---

##############################
Test: CheckPatch - FAIL
Output:
Bluetooth: btintel parse TLV structure
WARNING: From:/Signed-off-by: email address mismatch: 'From: Sathish Narasimman <[email protected]>' != 'Signed-off-by: Sathish Narasimman <[email protected]>'

total: 0 errors, 1 warnings, 234 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 parse TLV structure" 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 - PASS

##############################
Test: CheckBuildK - PASS



---
Regards,
Linux Bluetooth