* [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
@ 2021-06-09 11:40 Kiran K
2021-06-09 12:07 ` [v1] " bluez.test.bot
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Kiran K @ 2021-06-09 11:40 UTC (permalink / raw)
To: linux-bluetooth
Cc: tedd.an, luiz.von.dentz, chethan.tumkur.narayan,
ravishankar.srivatsa, Kiran K
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;
+ 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
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
2021-06-09 11:40 [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination Kiran K
@ 2021-06-09 12:07 ` bluez.test.bot
2021-06-09 19:12 ` [PATCH v1] " Marcel Holtmann
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: bluez.test.bot @ 2021-06-09 12:07 UTC (permalink / raw)
To: linux-bluetooth, kiran.k
[-- Attachment #1: Type: text/plain, Size: 2630 bytes --]
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
[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44348 bytes --]
[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3555 bytes --]
[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 608523 bytes --]
[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11676 bytes --]
[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9911 bytes --]
[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11821 bytes --]
[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5452 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
2021-06-09 11:40 [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination Kiran K
2021-06-09 12:07 ` [v1] " bluez.test.bot
@ 2021-06-09 19:12 ` Marcel Holtmann
2021-06-09 22:32 ` Luiz Augusto von Dentz
2021-06-16 14:40 ` kernel test robot
2021-06-16 16:08 ` kernel test robot
3 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2021-06-09 19:12 UTC (permalink / raw)
To: Kiran K
Cc: Bluez mailing list, Tedd Ho-Jeong An, Luiz Augusto von Dentz,
Tumkur Narayan, Chethan, ravishankar.srivatsa
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
2021-06-09 19:12 ` [PATCH v1] " Marcel Holtmann
@ 2021-06-09 22:32 ` Luiz Augusto von Dentz
2021-06-10 7:39 ` Marcel Holtmann
2021-07-13 3:16 ` K, Kiran
0 siblings, 2 replies; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2021-06-09 22:32 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Kiran K, Bluez mailing list, Tedd Ho-Jeong An,
Luiz Augusto von Dentz, Tumkur Narayan, Chethan, Srivatsa,
Ravishankar
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
2021-06-09 22:32 ` Luiz Augusto von Dentz
@ 2021-06-10 7:39 ` Marcel Holtmann
2021-07-13 3:16 ` K, Kiran
1 sibling, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2021-06-10 7:39 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Kiran K, Bluez mailing list, Tedd Ho-Jeong An,
Luiz Augusto von Dentz, Tumkur Narayan, Chethan, Srivatsa,
Ravishankar
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
2021-06-09 11:40 [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination Kiran K
@ 2021-06-16 14:40 ` kernel test robot
2021-06-09 19:12 ` [PATCH v1] " Marcel Holtmann
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-06-16 14:40 UTC (permalink / raw)
To: Kiran K, linux-bluetooth
Cc: kbuild-all, tedd.an, luiz.von.dentz, chethan.tumkur.narayan,
ravishankar.srivatsa, Kiran K
[-- Attachment #1: Type: text/plain, Size: 4715 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67916 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
@ 2021-06-16 14:40 ` kernel test robot
0 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-06-16 14:40 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4837 bytes --]
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(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 67916 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
2021-06-09 11:40 [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination Kiran K
@ 2021-06-16 16:08 ` kernel test robot
2021-06-09 19:12 ` [PATCH v1] " Marcel Holtmann
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-06-16 16:08 UTC (permalink / raw)
To: Kiran K, linux-bluetooth
Cc: kbuild-all, clang-built-linux, tedd.an, luiz.von.dentz,
chethan.tumkur.narayan, ravishankar.srivatsa, Kiran K
[-- Attachment #1: Type: text/plain, Size: 5141 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40505 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
@ 2021-06-16 16:08 ` kernel test robot
0 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-06-16 16:08 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5270 bytes --]
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(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 40505 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
2021-06-09 22:32 ` Luiz Augusto von Dentz
2021-06-10 7:39 ` Marcel Holtmann
@ 2021-07-13 3:16 ` K, Kiran
1 sibling, 0 replies; 10+ messages in thread
From: K, Kiran @ 2021-07-13 3:16 UTC (permalink / raw)
To: Luiz Augusto von Dentz, Marcel Holtmann
Cc: Bluez mailing list, An, Tedd, Von Dentz, Luiz, Tumkur Narayan,
Chethan, Srivatsa, Ravishankar
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-07-13 3:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-09 11:40 [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination Kiran K
2021-06-09 12:07 ` [v1] " bluez.test.bot
2021-06-09 19:12 ` [PATCH v1] " Marcel Holtmann
2021-06-09 22:32 ` Luiz Augusto von Dentz
2021-06-10 7:39 ` Marcel Holtmann
2021-07-13 3:16 ` K, Kiran
2021-06-16 14:40 ` kernel test robot
2021-06-16 14:40 ` kernel test robot
2021-06-16 16:08 ` kernel test robot
2021-06-16 16:08 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.