diff mbox series

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

Message ID 20210609114029.1656-1-kiran.k@intel.com (mailing list archive)
State New, archived
Headers show
Series [v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination | expand

Commit Message

K, Kiran June 9, 2021, 11:40 a.m. UTC
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
if new generation controller is found and attempts *setup* of
BTUSB_INTEL_NEWGEN.

Signed-off-by: Kiran K <kiran.k@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
 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(-)

Comments

bluez.test.bot@gmail.com June 9, 2021, 12:07 p.m. UTC | #1
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=497137

---Test result---

Test Summary:
CheckPatch                    PASS      1.91 seconds
GitLint                       PASS      0.19 seconds
BuildKernel                   PASS      668.34 seconds
TestRunner: Setup             PASS      433.53 seconds
TestRunner: l2cap-tester      PASS      3.23 seconds
TestRunner: bnep-tester       PASS      2.16 seconds
TestRunner: mgmt-tester       PASS      31.77 seconds
TestRunner: rfcomm-tester     PASS      2.40 seconds
TestRunner: sco-tester        PASS      2.37 seconds
TestRunner: smp-tester        PASS      2.36 seconds
TestRunner: userchan-tester   PASS      2.13 seconds

Details
##############################
Test: CheckPatch - PASS - 1.91 seconds
Run checkpatch.pl script with rule in .checkpatch.conf


##############################
Test: GitLint - PASS - 0.19 seconds
Run gitlint with rule in .gitlint


##############################
Test: BuildKernel - PASS - 668.34 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 433.53 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 3.23 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 2.16 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 31.77 seconds
Run test-runner with mgmt-tester
Total: 446, Passed: 433 (97.1%), Failed: 0, Not Run: 13

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.40 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.37 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - PASS - 2.36 seconds
Run test-runner with smp-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: userchan-tester - PASS - 2.13 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth
Marcel Holtmann June 9, 2021, 7:12 p.m. UTC | #2
Hi Kiran,

> 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
> if new generation controller is found and attempts *setup* of
> BTUSB_INTEL_NEWGEN.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> 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.

> +		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)",
> +			   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.

> /* ------- 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,
> 				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,
> 	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)
> 	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)
> 	 * 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.

> +			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.

Regards

Marcel
Luiz Augusto von Dentz June 9, 2021, 10:32 p.m. UTC | #3
Hi Marcel, Kiran,

On Wed, Jun 9, 2021 at 12:15 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Kiran,
>
> > 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
> > if new generation controller is found and attempts *setup* of
> > BTUSB_INTEL_NEWGEN.
> >
> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > ---
> > 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.
>
> > +             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)",
> > +                        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.
>
> > /* ------- 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,
> >                               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,
> >       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)
> >       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)
> >        * 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.
>
> > +                     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.
Marcel Holtmann June 10, 2021, 7:39 a.m. UTC | #4
Hi Luiz,

>>> 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
>>> if new generation controller is found and attempts *setup* of
>>> BTUSB_INTEL_NEWGEN.
>>> 
>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
>>> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
>>> ---
>>> 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.
>> 
>>> +             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)",
>>> +                        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.
>> 
>>> /* ------- 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,
>>>                              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,
>>>      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)
>>>      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)
>>>       * 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.
>> 
>>> +                     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.

we need to do full runtime detection. Meaning that essentially we just set BTUSB_CSR or BTUSB_INTEL based on the VID/PID and that is it. This needs to include also the early generation WP2 etc. of course.

Regards

Marcel
kernel test robot June 16, 2021, 2:40 p.m. UTC | #5
Hi Kiran,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on bluetooth/master v5.13-rc6 next-20210616]
[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/Kiran-K/Bluetooth-btintel-Support-Digital-N-RF-N-1-combination/20210616-183512
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
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
        # https://github.com/0day-ci/linux/commit/6e8c708932770f46284508ca6f027fa39393389e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kiran-K/Bluetooth-btintel-Support-Digital-N-RF-N-1-combination/20210616-183512
        git checkout 6e8c708932770f46284508ca6f027fa39393389e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/bluetooth/btintel.c:486:6: warning: no previous prototype for 'btintel_parse_version_tlv' [-Wmissing-prototypes]
     486 | void btintel_parse_version_tlv(struct hci_dev *hdev, struct sk_buff *skb,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/btintel_parse_version_tlv +486 drivers/bluetooth/btintel.c

   485	
 > 486	void btintel_parse_version_tlv(struct hci_dev *hdev, struct sk_buff *skb,
   487				       struct intel_version_tlv *version)
   488	{
   489		/* Consume Command Complete Status field */
   490		skb_pull(skb, sizeof(__u8));
   491	
   492		/* Event parameters contatin multiple TLVs. Read each of them
   493		 * and only keep the required data. Also, it use existing legacy
   494		 * version field like hw_platform, hw_variant, and fw_variant
   495		 * to keep the existing setup flow
   496		 */
   497		while (skb->len) {
   498			struct intel_tlv *tlv;
   499	
   500			tlv = (struct intel_tlv *)skb->data;
   501			switch (tlv->type) {
   502			case INTEL_TLV_CNVI_TOP:
   503				version->cnvi_top = get_unaligned_le32(tlv->val);
   504				break;
   505			case INTEL_TLV_CNVR_TOP:
   506				version->cnvr_top = get_unaligned_le32(tlv->val);
   507				break;
   508			case INTEL_TLV_CNVI_BT:
   509				version->cnvi_bt = get_unaligned_le32(tlv->val);
   510				break;
   511			case INTEL_TLV_CNVR_BT:
   512				version->cnvr_bt = get_unaligned_le32(tlv->val);
   513				break;
   514			case INTEL_TLV_DEV_REV_ID:
   515				version->dev_rev_id = get_unaligned_le16(tlv->val);
   516				break;
   517			case INTEL_TLV_IMAGE_TYPE:
   518				version->img_type = tlv->val[0];
   519				break;
   520			case INTEL_TLV_TIME_STAMP:
   521				version->timestamp = get_unaligned_le16(tlv->val);
   522				break;
   523			case INTEL_TLV_BUILD_TYPE:
   524				version->build_type = tlv->val[0];
   525				break;
   526			case INTEL_TLV_BUILD_NUM:
   527				version->build_num = get_unaligned_le32(tlv->val);
   528				break;
   529			case INTEL_TLV_SECURE_BOOT:
   530				version->secure_boot = tlv->val[0];
   531				break;
   532			case INTEL_TLV_OTP_LOCK:
   533				version->otp_lock = tlv->val[0];
   534				break;
   535			case INTEL_TLV_API_LOCK:
   536				version->api_lock = tlv->val[0];
   537				break;
   538			case INTEL_TLV_DEBUG_LOCK:
   539				version->debug_lock = tlv->val[0];
   540				break;
   541			case INTEL_TLV_MIN_FW:
   542				version->min_fw_build_nn = tlv->val[0];
   543				version->min_fw_build_cw = tlv->val[1];
   544				version->min_fw_build_yy = tlv->val[2];
   545				break;
   546			case INTEL_TLV_LIMITED_CCE:
   547				version->limited_cce = tlv->val[0];
   548				break;
   549			case INTEL_TLV_SBE_TYPE:
   550				version->sbe_type = tlv->val[0];
   551				break;
   552			case INTEL_TLV_OTP_BDADDR:
   553				memcpy(&version->otp_bd_addr, tlv->val, tlv->len);
   554				break;
   555			default:
   556				/* Ignore rest of information */
   557				break;
   558			}
   559			/* consume the current tlv and move to next*/
   560			skb_pull(skb, tlv->len + sizeof(*tlv));
   561		}
   562	}
   563	EXPORT_SYMBOL_GPL(btintel_parse_version_tlv);
   564	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot June 16, 2021, 4:08 p.m. UTC | #6
Hi Kiran,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on bluetooth/master v5.13-rc6 next-20210616]
[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/Kiran-K/Bluetooth-btintel-Support-Digital-N-RF-N-1-combination/20210616-183512
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: x86_64-randconfig-a015-20210615 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
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/6e8c708932770f46284508ca6f027fa39393389e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kiran-K/Bluetooth-btintel-Support-Digital-N-RF-N-1-combination/20210616-183512
        git checkout 6e8c708932770f46284508ca6f027fa39393389e
        # 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 <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/bluetooth/btintel.c:486:6: warning: no previous prototype for function 'btintel_parse_version_tlv' [-Wmissing-prototypes]
   void btintel_parse_version_tlv(struct hci_dev *hdev, struct sk_buff *skb,
        ^
   drivers/bluetooth/btintel.c:486:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void btintel_parse_version_tlv(struct hci_dev *hdev, struct sk_buff *skb,
   ^
   static 
   1 warning generated.


vim +/btintel_parse_version_tlv +486 drivers/bluetooth/btintel.c

   485	
 > 486	void btintel_parse_version_tlv(struct hci_dev *hdev, struct sk_buff *skb,
   487				       struct intel_version_tlv *version)
   488	{
   489		/* Consume Command Complete Status field */
   490		skb_pull(skb, sizeof(__u8));
   491	
   492		/* Event parameters contatin multiple TLVs. Read each of them
   493		 * and only keep the required data. Also, it use existing legacy
   494		 * version field like hw_platform, hw_variant, and fw_variant
   495		 * to keep the existing setup flow
   496		 */
   497		while (skb->len) {
   498			struct intel_tlv *tlv;
   499	
   500			tlv = (struct intel_tlv *)skb->data;
   501			switch (tlv->type) {
   502			case INTEL_TLV_CNVI_TOP:
   503				version->cnvi_top = get_unaligned_le32(tlv->val);
   504				break;
   505			case INTEL_TLV_CNVR_TOP:
   506				version->cnvr_top = get_unaligned_le32(tlv->val);
   507				break;
   508			case INTEL_TLV_CNVI_BT:
   509				version->cnvi_bt = get_unaligned_le32(tlv->val);
   510				break;
   511			case INTEL_TLV_CNVR_BT:
   512				version->cnvr_bt = get_unaligned_le32(tlv->val);
   513				break;
   514			case INTEL_TLV_DEV_REV_ID:
   515				version->dev_rev_id = get_unaligned_le16(tlv->val);
   516				break;
   517			case INTEL_TLV_IMAGE_TYPE:
   518				version->img_type = tlv->val[0];
   519				break;
   520			case INTEL_TLV_TIME_STAMP:
   521				version->timestamp = get_unaligned_le16(tlv->val);
   522				break;
   523			case INTEL_TLV_BUILD_TYPE:
   524				version->build_type = tlv->val[0];
   525				break;
   526			case INTEL_TLV_BUILD_NUM:
   527				version->build_num = get_unaligned_le32(tlv->val);
   528				break;
   529			case INTEL_TLV_SECURE_BOOT:
   530				version->secure_boot = tlv->val[0];
   531				break;
   532			case INTEL_TLV_OTP_LOCK:
   533				version->otp_lock = tlv->val[0];
   534				break;
   535			case INTEL_TLV_API_LOCK:
   536				version->api_lock = tlv->val[0];
   537				break;
   538			case INTEL_TLV_DEBUG_LOCK:
   539				version->debug_lock = tlv->val[0];
   540				break;
   541			case INTEL_TLV_MIN_FW:
   542				version->min_fw_build_nn = tlv->val[0];
   543				version->min_fw_build_cw = tlv->val[1];
   544				version->min_fw_build_yy = tlv->val[2];
   545				break;
   546			case INTEL_TLV_LIMITED_CCE:
   547				version->limited_cce = tlv->val[0];
   548				break;
   549			case INTEL_TLV_SBE_TYPE:
   550				version->sbe_type = tlv->val[0];
   551				break;
   552			case INTEL_TLV_OTP_BDADDR:
   553				memcpy(&version->otp_bd_addr, tlv->val, tlv->len);
   554				break;
   555			default:
   556				/* Ignore rest of information */
   557				break;
   558			}
   559			/* consume the current tlv and move to next*/
   560			skb_pull(skb, tlv->len + sizeof(*tlv));
   561		}
   562	}
   563	EXPORT_SYMBOL_GPL(btintel_parse_version_tlv);
   564	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
K, Kiran July 13, 2021, 3:16 a.m. UTC | #7
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 <kiran.k@intel.com>; Bluez mailing list <linux-
> bluetooth@vger.kernel.org>; An, Tedd <tedd.an@intel.com>; Von Dentz,
> Luiz <luiz.von.dentz@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>
> 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 <marcel@holtmann.org>
> wrote:
> >
> > Hi Kiran,
> >
> > > 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
> > > if new generation controller is found and attempts *setup* of
> > > BTUSB_INTEL_NEWGEN.
> > >
> > > Signed-off-by: Kiran K <kiran.k@intel.com>
> > > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > > ---
> > > 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.
> >
> > > +             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)",
> > > +                        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.
> >
> > > /* ------- 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,
> > >                               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,
> > >       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)
> > >       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)
> > >        * 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.
> >
> > > +                     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
diff mbox series

Patch

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;
+		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)",
+			   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);
+
 /* ------- 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,
 				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,
 	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)
 	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)
 	 * 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;
+			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);
+		}
 
 		/* The transport driver can set the quirk to mark the
 		 * BD_ADDR invalid before creating the HCI device or in