RE: [PATCH v2 1/2] Bluetooth: btusb: support link statistics telemetry events

Hi Luiz,

Thanks for your feedback and sorry for the delay in response.

Hi Joseph,

On Tue, Apr 13, 2021 at 12:45 AM Joseph Hwang [off-list ref]
wrote:

quoted

From: Chethan T N <redacted>

This patch supports the link statistics telemetry events for Intel
controllers

To avoid the overhead, this debug feature is disabled by default.

Reviewed-by: Miao-chen Chou <redacted>
Signed-off-by: Chethan T N <redacted>
Signed-off-by: Kiran K <redacted>
Signed-off-by: Joseph Hwang <redacted>
---

Changes in v2:
- take care of intel_newgen as well as intel_new
- fix the long-line issue

 drivers/bluetooth/btintel.c | 20 +++++++++++++++++++-
 drivers/bluetooth/btusb.c   | 18 ------------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index e44b6993cf91..de1dbdc01e5a 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c

@@ -1248,8 +1248,10 @@

EXPORT_SYMBOL_GPL(btintel_read_debug_features);

quoted

 int btintel_set_debug_features(struct hci_dev *hdev,
                               const struct intel_debug_features
*features)  {
-       u8 mask[11] = { 0x0a, 0x92, 0x02, 0x07, 0x00, 0x00, 0x00, 0x00,
+       u8 mask[11] = { 0x0a, 0x92, 0x02, 0x7f, 0x00, 0x00, 0x00,
+ 0x00,
                        0x00, 0x00, 0x00 };
+       u8 period[5] = { 0x04, 0x91, 0x02, 0x01, 0x00 };
+       u8 trace_enable = 0x02;
        struct sk_buff *skb;

Instead of using a byte array and custo opcode I would recommend we start
adding proper defines for Intel vendor opcodes with structs for them, similar
opcodes and structs are also very useful for adding support for btmon, in fact I
would start with btmon first and only when we have proper decoding support
start changing the kernel parts so you can actually add btmon output to the
patch description here.

As of now I don't see any of the VS commands have been defined as per your suggestion. However upcoming patches of btmon tool shall have the decoding of the commands and events for debug purpose as per your recommendation. 
And completely abide to the GPL licensing terms and open for anyone to modify these patches later. I shall rebased these patches; re-send them again.

quoted

        if (!features)

@@ -1266,8 +1268,24 @@ int btintel_set_debug_features(struct hci_dev

*hdev,

quoted

                           PTR_ERR(skb));
                return PTR_ERR(skb);
        }
+       kfree_skb(skb);
+
+       skb = __hci_cmd_sync(hdev, 0xfc8b, 5, period, HCI_INIT_TIMEOUT);
+       if (IS_ERR(skb)) {
+               bt_dev_err(hdev, "Setting periodicity for link statistics traces failed

(%ld)",

quoted

+                          PTR_ERR(skb));
+               return PTR_ERR(skb);
+       }
+       kfree_skb(skb);

+       skb = __hci_cmd_sync(hdev, 0xfca1, 1, &trace_enable,

HCI_INIT_TIMEOUT);

quoted

+       if (IS_ERR(skb)) {
+               bt_dev_err(hdev, "Enable tracing of link statistics events failed (%ld)",
+                          PTR_ERR(skb));
+               return PTR_ERR(skb);
+       }
        kfree_skb(skb);
+
        return 0;
 }
 EXPORT_SYMBOL_GPL(btintel_set_debug_features);

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 192cb8c191bc..f29946f15f59 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c

@@ -2811,7 +2811,6 @@ static int btusb_setup_intel_new(struct hci_dev

*hdev)

quoted

        u32 boot_param;
        char ddcname[64];
        int err;
-       struct intel_debug_features features;

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

@@ -2865,14 +2864,6 @@ static int btusb_setup_intel_new(struct hci_dev

*hdev)

quoted

                btintel_load_ddc_config(hdev, ddcname);
        }

-       /* Read the Intel supported features and if new exception formats
-        * supported, need to load the additional DDC config to enable.
-        */
-       btintel_read_debug_features(hdev, &features);
-
-       /* Set DDC mask for available debug features */
-       btintel_set_debug_features(hdev, &features);
-
        /* Read the Intel version information after loading the FW  */
        err = btintel_read_version(hdev, &ver);
        if (err)

@@ -2911,7 +2902,6 @@ static int btusb_setup_intel_newgen(struct hci_dev

*hdev)

quoted

        u32 boot_param;
        char ddcname[64];
        int err;
-       struct intel_debug_features features;
        struct intel_version_tlv version;

        bt_dev_dbg(hdev, "");

@@ -2961,14 +2951,6 @@ static int btusb_setup_intel_newgen(struct hci_dev

*hdev)

quoted

         */
        btintel_load_ddc_config(hdev, ddcname);

-       /* Read the Intel supported features and if new exception formats
-        * supported, need to load the additional DDC config to enable.
-        */
-       btintel_read_debug_features(hdev, &features);
-
-       /* Set DDC mask for available debug features */
-       btintel_set_debug_features(hdev, &features);
-
        /* Read the Intel version information after loading the FW  */
        err = btintel_read_version_tlv(hdev, &version);
        if (err)
--
2.31.1.295.g9ea45b61b8-goog

--
Luiz Augusto von Dentz