RE: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination

Hi Luiz, Marcel,

Thanks for the comments. I will make a new patch and suggested and send it for review.

Regards,
Kiran

-----Original Message-----
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Sent: Thursday, June 10, 2021 4:02 AM
To: Marcel Holtmann <marcel@holtmann.org>
Cc: K, Kiran <redacted>; Bluez mailing list <linux-
bluetooth@vger.kernel.org>; An, Tedd [off-list ref]; Von Dentz,
Luiz [off-list ref]; Tumkur Narayan, Chethan
[off-list ref]; Srivatsa, Ravishankar
[off-list ref]
Subject: Re: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1)
combination

Hi Marcel, Kiran,

On Wed, Jun 9, 2021 at 12:15 PM Marcel Holtmann [off-list ref]
wrote:

quoted

Hi Kiran,

quoted

New generation Intel controllers(N) need to support RF from (N-1)
generation. Since PID comes from OTP present in RF module,
*setup* function gets mapped to BTUSB_INTEL_NEW instead of
BTUSB_INTEL_NEWGEN. This patch checks generation of CNVi in
*setup* of BTUSB_INTEL_NEW and maps callbacks to

BTUSB_INTEL_NEWGEN

quoted

quoted

if new generation controller is found and attempts *setup* of
BTUSB_INTEL_NEWGEN.

Signed-off-by: Kiran K <redacted>
Reviewed-by: Chethan T N <redacted>
Reviewed-by: Srivatsa Ravishankar <redacted>
---
drivers/bluetooth/btintel.c | 119
++++++++++++++++++++++++++++++++++++
drivers/bluetooth/btintel.h |  10 +++
drivers/bluetooth/btusb.c   |  45 +++++++++++++-
net/bluetooth/hci_core.c    |   5 +-
4 files changed, 177 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btintel.c

b/drivers/bluetooth/btintel.c index e44b6993cf91..1d9ecc481f14
100644

--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c

@@ -483,6 +483,85 @@ int btintel_version_info_tlv(struct hci_dev

*hdev, struct intel_version_tlv *ver }
EXPORT_SYMBOL_GPL(btintel_version_info_tlv);

+void btintel_parse_version_tlv(struct hci_dev *hdev, struct sk_buff *skb,
+                            struct intel_version_tlv *version) {
+     /* Consume Command Complete Status field */
+     skb_pull(skb, sizeof(__u8));
+
+     /* 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
+      * to keep the existing setup flow
+      */
+     while (skb->len) {
+             struct intel_tlv *tlv;
+
+             tlv = (struct intel_tlv *)skb->data;
+             switch (tlv->type) {
+             case INTEL_TLV_CNVI_TOP:
+                     version->cnvi_top = get_unaligned_le32(tlv->val);
+                     break;

I think we already had this issue that you need to check that enough data is

actually in the SKB.

quoted

quoted

+             case INTEL_TLV_CNVR_TOP:
+                     version->cnvr_top = get_unaligned_le32(tlv->val);
+                     break;
+             case INTEL_TLV_CNVI_BT:
+                     version->cnvi_bt = get_unaligned_le32(tlv->val);
+                     break;
+             case INTEL_TLV_CNVR_BT:
+                     version->cnvr_bt = get_unaligned_le32(tlv->val);
+                     break;
+             case INTEL_TLV_DEV_REV_ID:
+                     version->dev_rev_id = get_unaligned_le16(tlv->val);
+                     break;
+             case INTEL_TLV_IMAGE_TYPE:
+                     version->img_type = tlv->val[0];
+                     break;
+             case INTEL_TLV_TIME_STAMP:
+                     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:
+                     version->build_num = get_unaligned_le32(tlv->val);
+                     break;
+             case INTEL_TLV_SECURE_BOOT:
+                     version->secure_boot = tlv->val[0];
+                     break;
+             case INTEL_TLV_OTP_LOCK:
+                     version->otp_lock = tlv->val[0];
+                     break;
+             case INTEL_TLV_API_LOCK:
+                     version->api_lock = tlv->val[0];
+                     break;
+             case INTEL_TLV_DEBUG_LOCK:
+                     version->debug_lock = tlv->val[0];
+                     break;
+             case INTEL_TLV_MIN_FW:
+                     version->min_fw_build_nn = tlv->val[0];
+                     version->min_fw_build_cw = tlv->val[1];
+                     version->min_fw_build_yy = tlv->val[2];
+                     break;
+             case INTEL_TLV_LIMITED_CCE:
+                     version->limited_cce = tlv->val[0];
+                     break;
+             case INTEL_TLV_SBE_TYPE:
+                     version->sbe_type = tlv->val[0];
+                     break;
+             case INTEL_TLV_OTP_BDADDR:
+                     memcpy(&version->otp_bd_addr, tlv->val, tlv->len);
+                     break;
+             default:
+                     /* Ignore rest of information */
+                     break;
+             }
+             /* consume the current tlv and move to next*/
+             skb_pull(skb, tlv->len + sizeof(*tlv));
+     }
+}
+EXPORT_SYMBOL_GPL(btintel_parse_version_tlv);
+
int btintel_read_version_tlv(struct hci_dev *hdev, struct
intel_version_tlv *version) {
      struct sk_buff *skb;

@@ -595,6 +674,46 @@ int btintel_read_version_tlv(struct hci_dev

*hdev, struct intel_version_tlv *ver }
EXPORT_SYMBOL_GPL(btintel_read_version_tlv);

+int btintel_generic_read_version(struct hci_dev *hdev,
+                              struct intel_version_tlv *ver_tlv,
+                              struct intel_version *ver, bool
+*is_tlv) {
+     struct sk_buff *skb;
+     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 information failed

(%ld)",

quoted

quoted

+                        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;
+     }
+
+     if (skb->len < sizeof(struct intel_version))
+             return -EILSEQ;
+
+     if (skb->len == sizeof(struct intel_version) &&
+         skb->data[1] == 0x37)
+             *is_tlv = false;
+     else
+             *is_tlv = true;
+
+     if (*is_tlv)
+             btintel_parse_version_tlv(hdev, skb, ver_tlv);
+     else
+             memcpy(ver, skb->data, sizeof(*ver));
+
+     kfree_skb(skb);
+     return 0;
+}
+EXPORT_SYMBOL_GPL(btintel_generic_read_version);
+

I have the feeling that we are falling back to a patch that I already rejected.

quoted

/* ------- REGMAP IBT SUPPORT ------- */

#define IBT_REG_MODE_8BIT  0x00

diff --git a/drivers/bluetooth/btintel.h

b/drivers/bluetooth/btintel.h index d184064a5e7c..366cb746f9c4
100644

--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h

@@ -175,6 +175,10 @@ int btintel_read_debug_features(struct hci_dev

*hdev,

quoted

quoted

                              struct intel_debug_features
*features); int btintel_set_debug_features(struct hci_dev *hdev,
                             const struct intel_debug_features
*features);
+int btintel_generic_read_version(struct hci_dev *hdev,
+                              struct intel_version_tlv *ver_tlv,
+                              struct intel_version *ver,
+                              bool *is_tlv);
#else

static inline int btintel_check_bdaddr(struct hci_dev *hdev) @@
-307,4 +311,10 @@ static inline int btintel_set_debug_features(struct

hci_dev *hdev,

quoted

quoted

      return -EOPNOTSUPP;
}

+static int btintel_generic_read_version(struct hci_dev *hdev,
+                                     struct intel_version_tlv *ver_tlv,
+                                     struct intel_version *ver,
+bool *is_tlv) {
+     return -EOPNOTSUPP;
+}
#endif

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a9855a2dd561..15d91aae52cc 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c

@@ -583,6 +583,9 @@ struct btusb_data {
      unsigned cmd_timeout_cnt;

};

+static int btusb_setup_intel_newgen(struct hci_dev *hdev); static
+int btusb_shutdown_intel_new(struct hci_dev *hdev);
+
static void btusb_intel_cmd_timeout(struct hci_dev *hdev) {
      struct btusb_data *data = hci_get_drvdata(hdev); @@ -2842,6
+2845,18 @@ static int btusb_intel_boot(struct hci_dev *hdev, u32

boot_addr)

quoted

quoted

      return err;
}

+static bool btintel_is_newgen_controller(struct hci_dev *hdev, u32
+cnvi) {
+     switch (cnvi & 0xFFF) {
+     case 0x400: /* Slr */
+     case 0x401: /* Slr-F */
+     case 0x410: /* TyP */
+     case 0x810: /* Mgr */
+             return true;
+     }
+     return false;
+}
+
static int btusb_setup_intel_new(struct hci_dev *hdev) {
      struct btusb_data *data = hci_get_drvdata(hdev); @@ -2851,6
+2866,8 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
      char ddcname[64];
      int err;
      struct intel_debug_features features;
+     struct intel_version_tlv ver_tlv;
+     bool is_tlv;

      BT_DBG("%s", hdev->name);

@@ -2864,12 +2881,38 @@ static int btusb_setup_intel_new(struct

hci_dev *hdev)

quoted

quoted

       * is in bootloader mode or if it already has operational firmware
       * loaded.
       */
-     err = btintel_read_version(hdev, &ver);
+     err = btintel_generic_read_version(hdev, &ver_tlv, &ver,
+ &is_tlv);
      if (err) {
              bt_dev_err(hdev, "Intel Read version failed (%d)", err);
              btintel_reset_to_bootloader(hdev);
              return err;
      }
+     if (is_tlv) {
+             /* We got TLV data. Check for new generation CNVi. If present,
+              * then map the callbacks to BTUSB_INTEL_NEWGEN and attempt
+              * setup function again
+              */
+             if (btintel_is_newgen_controller(hdev, ver_tlv.cnvi_top)) {
+                     hdev->send = btusb_send_frame_intel;
+                     hdev->setup = btusb_setup_intel_newgen;

So this is a clear no, your are not changing hdev->setup within hdev->setup.

quoted

+                     hdev->shutdown = btusb_shutdown_intel_new;
+                     hdev->hw_error = btintel_hw_error;
+                     hdev->set_diag = btintel_set_diag;
+                     hdev->set_bdaddr = btintel_set_bdaddr;
+                     hdev->cmd_timeout = btusb_intel_cmd_timeout;
+                     return -EAGAIN;
+             }
+
+             /* we need to read legacy version here to minimize the changes
+              * and keep the esixiting flow
+              */
+             err = btintel_read_version(hdev, &ver);
+             if (err) {
+                     bt_dev_err(hdev, "Intel Read version failed (%d)", err);
+                     btintel_reset_to_bootloader(hdev);
+                     return err;
+             }
+     }

      err = btintel_version_info(hdev, &ver);
      if (err)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 1eb7ffd0dd29..8e407bad0e31 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c

@@ -1496,8 +1496,11 @@ static int hci_dev_do_open(struct hci_dev

*hdev)

              hci_sock_dev_event(hdev, HCI_DEV_SETUP);

-             if (hdev->setup)
+             if (hdev->setup) {
                      ret = hdev->setup(hdev);
+                     if (ret && ret == -EAGAIN)
+                             ret = hdev->setup(hdev);
+             }

NO. Please stop hacking here. I think you need to take a whiteboard and

draw how our controllers are initialized.

+1

It is already strange that we have mixed generation, besides we never really
did a good job tracking the generation properly, but with this it seems we are
attempting to mix generations so we no longer can detect them based on
USB PID/VID but need to dig into the version information at runtime, so imo
we should either detected based on USB PID/VID or if that is not possible
then switch it to be runtime only and not try to do both as it should be clear
by now that will just make the code really hard to follow.

--
Luiz Augusto von Dentz