| Message ID | 20201106092119.20486-1-sathish.narasimman@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] Bluetooth: btintel parse TLV structure | 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=378873 ---Test result--- ############################## Test: CheckPatch - PASS ############################## Test: CheckGitLint - PASS ############################## Test: CheckBuildK - PASS --- Regards, Linux Bluetooth
Hi > -----Original Message----- > From: Narasimman, Sathish <sathish.narasimman@intel.com> > Sent: Friday, November 6, 2020 2:51 PM > To: linux-bluetooth@vger.kernel.org > Cc: Tumkur Narayan, Chethan <chethan.tumkur.narayan@intel.com>; > Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; Narasimman, > Sathish <sathish.narasimman@intel.com> > Subject: [PATCH v3] 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 <sathish.narasimman@intel.com> > --- > 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, ¶ms, > &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, ¶ms, > + &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, ¶ms, > ddcname, > + err = btusb_setup_intel_new_get_fw_name(&bt_ver.ver, ¶ms, > 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
Hi > -----Original Message----- > From: Narasimman, Sathish > Sent: Tuesday, November 17, 2020 9:11 AM > To: linux-bluetooth@vger.kernel.org > Cc: Tumkur Narayan, Chethan <chethan.tumkur.narayan@intel.com>; > Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; K, Kiran > <kiran.k@intel.com> > Subject: RE: [PATCH v3] Bluetooth: btintel parse TLV structure > > Hi > > > -----Original Message----- > > From: Narasimman, Sathish <sathish.narasimman@intel.com> > > Sent: Friday, November 6, 2020 2:51 PM > > To: linux-bluetooth@vger.kernel.org > > Cc: Tumkur Narayan, Chethan <chethan.tumkur.narayan@intel.com>; > > Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; Narasimman, > > Sathish <sathish.narasimman@intel.com> > > Subject: [PATCH v3] 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 <sathish.narasimman@intel.com> > > --- > > 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, ¶ms, > > &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, ¶ms, > > + &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, ¶ms, > > ddcname, > > + err = btusb_setup_intel_new_get_fw_name(&bt_ver.ver, ¶ms, > > 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 Gentle reminder Regards Sathish N
Hi > -----Original Message----- > From: Narasimman, Sathish <sathish.narasimman@intel.com> > Sent: Monday, November 23, 2020 9:25 AM > To: 'linux-bluetooth@vger.kernel.org' <linux-bluetooth@vger.kernel.org> > Cc: Tumkur Narayan, Chethan <chethan.tumkur.narayan@intel.com>; > Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; K, Kiran > <kiran.k@intel.com>; Narasimman, Sathish > <sathish.narasimman@intel.com> > Subject: RE: [PATCH v3] Bluetooth: btintel parse TLV structure > > Hi > > > -----Original Message----- > > From: Narasimman, Sathish > > Sent: Tuesday, November 17, 2020 9:11 AM > > To: linux-bluetooth@vger.kernel.org > > Cc: Tumkur Narayan, Chethan <chethan.tumkur.narayan@intel.com>; > > Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; K, Kiran > > <kiran.k@intel.com> > > Subject: RE: [PATCH v3] Bluetooth: btintel parse TLV structure > > > > Hi > > > > > -----Original Message----- > > > From: Narasimman, Sathish <sathish.narasimman@intel.com> > > > Sent: Friday, November 6, 2020 2:51 PM > > > To: linux-bluetooth@vger.kernel.org > > > Cc: Tumkur Narayan, Chethan <chethan.tumkur.narayan@intel.com>; > > > Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; Narasimman, > > > Sathish <sathish.narasimman@intel.com> > > > Subject: [PATCH v3] 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 <sathish.narasimman@intel.com> > > > --- > > > 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, ¶ms, > > > &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, ¶ms, > > > + &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, ¶ms, > > > ddcname, > > > + err = btusb_setup_intel_new_get_fw_name(&bt_ver.ver, ¶ms, > > > 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 > > Gentle reminder > > Regards > Sathish N Please help to review the patches Regards Sathish N
Hi Sathish, > 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 <sathish.narasimman@intel.com> > --- > 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 isn’t it ThP here? > + * type cards. when the controller is in bootloader mode the controller Please state a sentence with uppercase. > + * 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 in doubt, please run a spell checker over patch. > + 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; > + So last time around I said, I don’t like it this way. Now I get the same thing again. Can we please stop trying to hack this into the driver like this. If we now actually start pushing these change into our firmware, then maybe it would be good to give Linux a heads up that this is changing. Intermixing these kind of firmware “modes” is something that should be designed properly. If not, this driver become unmaintainable and we start seeing bugs after bugs for corner cases that are not tested. The problem you have to address here is not the Intel Read Version command and trying to fit everything into some magic union struct and add if-clauses everywhere. The problem is that we have now 4 combinations of device initialization that we need to handle correctly (and I am including StP here as well). Let me try to summarize what we have. 1) Gen 1 ROM patching with maintainer mode and all the fun around it 2) Gen 2 RAM firmware loading and static structs bootloader and operational 3) Gen 2 RAM firmware loading with static struct for bootloader and TLV for operational 4) Gen 3 RAM firmware loading and TLV for bootloader and operational Is this correct? We have to realize that btintel_read_version function is no longer usable as it is. It can also not be extended properly. What we need is a generic btusb_setup_intel that executes the read version (without any helper), runs basic sanity checks and then segways into legacy, static or TLV versions of firmware loading and operational setup. Of course there are optimization in the logic for 1) and 4) since you know what the operational handling will be. Nevertheless this needs to be changed now before this grows out of control and we still have users that are testing it and reporting back problems. To achieve this, pick one hardware (say ThP since it is wildly available) and move it to a new btusb_setup_intel_generic() that does what I described above. Test this with 2) and 3) firmwares to ensure we can handle things correctly. Actually it might be worth while to build a btvirt that allows us to emulate all Intel controllers boot handling. So that we can have automated intel-tester tool to verify that we don’t break things. Regards Marcel
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, ¶ms, &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, ¶ms, + &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, ¶ms, ddcname, + err = btusb_setup_intel_new_get_fw_name(&bt_ver.ver, ¶ms, 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;
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 <sathish.narasimman@intel.com> --- drivers/bluetooth/btintel.c | 105 +++++++++++++++++++++++++++--------- drivers/bluetooth/btintel.h | 16 ++++++ drivers/bluetooth/btusb.c | 41 ++++++++++---- 3 files changed, 129 insertions(+), 33 deletions(-)