2021-08-15 12:18:20

by Joseph Hwang

[permalink] [raw]
Subject: [PATCH v9 1/5] Bluetooth: btusb: disable Intel link statistics telemetry events

To avoid the overhead on both the controller and the host, the
Intel link statistics telemetry events are disabled by default.

Reviewed-by: Miao-chen Chou <[email protected]>
Signed-off-by: Chethan T N <[email protected]>
Signed-off-by: Kiran K <[email protected]>
Signed-off-by: Joseph Hwang <[email protected]>
---

Changes in v9:
- This version fixes the compile errors of patch 3/5.

Changes in v8:
- This version adds a new patch which refactors the set_exp_feature
function with a feature table.
- Swap the patches per the comments on v7.
- Remove the unsuitable debug messages.
- This patch is not changed in this version.

Changes in v7:
- Rebase on Tedd's patches that moved functionality from btusb to
btintel.

Changes in v6:
- Rebase on the latest commit.

Changes in v5:
- Rebase this patch 1/4 to resolve conflicts.
- There are changes in patches 3/4 and 4/4.

Changes in v4:
- The original 2 patches in Series-version 3 are split into
2 patches from each patch per reviewers' comments. There are
A total of 4 patches in this series now.
- The callback function is renamed from hdev->set_vs_dbg_evt to
hdev->set_quality_report. Note that there are two different
specifications which will be integrated soon and enabled/disabled
with the same callback. One is Android Bluetooth Quality Report
(BQR), and the other Intel link statistics telemetry events here.
While most Bluetooth controller vendors have supported or are
supporting the Android specification in their controllers, it looks
making sense to use set_quality_report as the callback name.
- Similarly, the config option BT_FEATURE_VS_DBG_EVT is renamed as
BT_FEATURE_QUALITY_REPORT which depends on BT now.
- The BQR is controller specific. There needs to be a valid hdev in the
first place. This is fixed in set_exp_feature().
- In set_exp_feature(), bluez will only set experimental feature to set
BQR when the feature is supported. Please refer to bluez CLs.
- Also refer to bluez patches for the decoding support of btmon.

Changes in v3:
- fix the long line in the commit message

Changes in v2:
- take care of intel_newgen as well as intel_new
- fix the long lines in mgmt.c

drivers/bluetooth/btintel.c | 20 --------------------
1 file changed, 20 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index f1705b46fc88..0fe093fa5158 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -1893,7 +1893,6 @@ static int btintel_bootloader_setup(struct hci_dev *hdev,
u32 boot_param;
char ddcname[64];
int err;
- struct intel_debug_features features;

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

@@ -1934,15 +1933,6 @@ static int btintel_bootloader_setup(struct hci_dev *hdev,
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.
- */
- err = btintel_read_debug_features(hdev, &features);
- if (!err) {
- /* 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, &new_ver);
if (err)
@@ -2089,7 +2079,6 @@ static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
u32 boot_param;
char ddcname[64];
int err;
- struct intel_debug_features features;
struct intel_version_tlv new_ver;

bt_dev_dbg(hdev, "");
@@ -2125,15 +2114,6 @@ static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
*/
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.
- */
- err = btintel_read_debug_features(hdev, &features);
- if (!err) {
- /* 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, &new_ver);
if (err)
--
2.33.0.rc1.237.g0d66db33f3-goog


2021-08-15 12:18:26

by Joseph Hwang

[permalink] [raw]
Subject: [PATCH v9 2/5] Bluetooth: btintel: support link statistics telemetry events

From: Chethan T N <[email protected]>

This patch supports the link statistics telemetry events for
intel controllers

Reviewed-by: Miao-chen Chou <[email protected]>
Signed-off-by: Chethan T N <[email protected]>
Signed-off-by: Kiran K <[email protected]>
Signed-off-by: Joseph Hwang <[email protected]>
---

(no changes since v7)

Changes in v7:
- Rebase on Tedd's patches that moved functionality from btusb to
btintel.

drivers/bluetooth/btintel.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 0fe093fa5158..643e2194ca01 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -1285,8 +1285,10 @@ static int btintel_read_debug_features(struct hci_dev *hdev,
static 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, 0x05, 0x00 };
+ u8 trace_enable = 0x02;
struct sk_buff *skb;

if (!features)
@@ -1303,8 +1305,24 @@ static int btintel_set_debug_features(struct hci_dev *hdev,
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)",
+ PTR_ERR(skb));
+ return PTR_ERR(skb);
+ }
+ kfree_skb(skb);

+ skb = __hci_cmd_sync(hdev, 0xfca1, 1, &trace_enable, HCI_INIT_TIMEOUT);
+ 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;
}

--
2.33.0.rc1.237.g0d66db33f3-goog

2021-08-15 12:18:31

by Joseph Hwang

[permalink] [raw]
Subject: [PATCH v9 3/5] Bluetooth: refactor set_exp_feature with a feature table

This patch refactors the set_exp_feature with a feature table
consisting of UUIDs and the corresponding callback functions.
In this way, a new experimental feature setting function can be
simply added with its UUID and callback function.

When looking at this patch, please use

git show --patience

which will display the diff much better than the default
diff-algorithm.

Signed-off-by: Joseph Hwang <[email protected]>
---

Changes in v9:
- This version fixes the compile errors of this patch.
The errors were caused by accidentally messing up my
development environment.

Changes in v8:
- Refactor the set_exp_feature function with a feature table.
- This is a new patch added in v8.

net/bluetooth/mgmt.c | 248 +++++++++++++++++++++++++------------------
1 file changed, 142 insertions(+), 106 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1e21e014efd2..42bd503da20d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3806,7 +3806,7 @@ static const u8 rpa_resolution_uuid[16] = {
static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
void *data, u16 data_len)
{
- char buf[62]; /* Enough space for 3 features */
+ char buf[62]; /* Enough space for 3 features */
struct mgmt_rp_read_exp_features_info *rp = (void *)buf;
u16 idx = 0;
u32 flags;
@@ -3892,150 +3892,186 @@ static int exp_debug_feature_changed(bool enabled, struct sock *skip)
}
#endif

-static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
- void *data, u16 data_len)
+#define EXP_FEAT(_uuid, _set_func) \
+{ \
+ .uuid = _uuid, \
+ .set_func = _set_func, \
+}
+
+/* The zero key uuid is special. Multiple exp features are set through it. */
+static int set_zero_key_func(struct sock *sk, struct hci_dev *hdev,
+ struct mgmt_cp_set_exp_feature *cp, u16 data_len)
{
- struct mgmt_cp_set_exp_feature *cp = data;
struct mgmt_rp_set_exp_feature rp;

- bt_dev_dbg(hdev, "sock %p", sk);
-
- if (!memcmp(cp->uuid, ZERO_KEY, 16)) {
- memset(rp.uuid, 0, 16);
- rp.flags = cpu_to_le32(0);
+ memset(rp.uuid, 0, 16);
+ rp.flags = cpu_to_le32(0);

#ifdef CONFIG_BT_FEATURE_DEBUG
- if (!hdev) {
- bool changed = bt_dbg_get();
+ if (!hdev) {
+ bool changed = bt_dbg_get();

- bt_dbg_set(false);
+ bt_dbg_set(false);

- if (changed)
- exp_debug_feature_changed(false, sk);
- }
+ if (changed)
+ exp_debug_feature_changed(false, sk);
+ }
#endif

- if (hdev && use_ll_privacy(hdev) && !hdev_is_powered(hdev)) {
- bool changed = hci_dev_test_flag(hdev,
- HCI_ENABLE_LL_PRIVACY);
+ if (hdev && use_ll_privacy(hdev) && !hdev_is_powered(hdev)) {
+ bool changed = hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);

- hci_dev_clear_flag(hdev, HCI_ENABLE_LL_PRIVACY);
+ hci_dev_clear_flag(hdev, HCI_ENABLE_LL_PRIVACY);

- if (changed)
- exp_ll_privacy_feature_changed(false, hdev, sk);
- }
+ if (changed)
+ exp_ll_privacy_feature_changed(false, hdev, sk);
+ }

- hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
+ hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);

- return mgmt_cmd_complete(sk, hdev ? hdev->id : MGMT_INDEX_NONE,
- MGMT_OP_SET_EXP_FEATURE, 0,
- &rp, sizeof(rp));
- }
+ return mgmt_cmd_complete(sk, hdev ? hdev->id : MGMT_INDEX_NONE,
+ MGMT_OP_SET_EXP_FEATURE, 0,
+ &rp, sizeof(rp));
+}

#ifdef CONFIG_BT_FEATURE_DEBUG
- if (!memcmp(cp->uuid, debug_uuid, 16)) {
- bool val, changed;
- int err;
+static int set_debug_func(struct sock *sk, struct hci_dev *hdev,
+ struct mgmt_cp_set_exp_feature *cp, u16 data_len)
+{
+ struct mgmt_rp_set_exp_feature rp;

- /* Command requires to use the non-controller index */
- if (hdev)
- return mgmt_cmd_status(sk, hdev->id,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_INVALID_INDEX);
+ bool val, changed;
+ int err;

- /* Parameters are limited to a single octet */
- if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
- return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_INVALID_PARAMS);
+ /* Command requires to use the non-controller index */
+ if (hdev)
+ return mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_EXP_FEATURE,
+ MGMT_STATUS_INVALID_INDEX);

- /* Only boolean on/off is supported */
- if (cp->param[0] != 0x00 && cp->param[0] != 0x01)
- return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_INVALID_PARAMS);
+ /* Parameters are limited to a single octet */
+ if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
+ return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
+ MGMT_OP_SET_EXP_FEATURE,
+ MGMT_STATUS_INVALID_PARAMS);

- val = !!cp->param[0];
- changed = val ? !bt_dbg_get() : bt_dbg_get();
- bt_dbg_set(val);
+ /* Only boolean on/off is supported */
+ if (cp->param[0] != 0x00 && cp->param[0] != 0x01)
+ return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
+ MGMT_OP_SET_EXP_FEATURE,
+ MGMT_STATUS_INVALID_PARAMS);

- memcpy(rp.uuid, debug_uuid, 16);
- rp.flags = cpu_to_le32(val ? BIT(0) : 0);
+ val = !!cp->param[0];
+ changed = val ? !bt_dbg_get() : bt_dbg_get();
+ bt_dbg_set(val);

- hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
+ memcpy(rp.uuid, debug_uuid, 16);
+ rp.flags = cpu_to_le32(val ? BIT(0) : 0);

- err = mgmt_cmd_complete(sk, MGMT_INDEX_NONE,
- MGMT_OP_SET_EXP_FEATURE, 0,
- &rp, sizeof(rp));
+ hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);

- if (changed)
- exp_debug_feature_changed(val, sk);
+ err = mgmt_cmd_complete(sk, MGMT_INDEX_NONE,
+ MGMT_OP_SET_EXP_FEATURE, 0,
+ &rp, sizeof(rp));

- return err;
- }
+ if (changed)
+ exp_debug_feature_changed(val, sk);
+
+ return err;
+}
#endif

- if (!memcmp(cp->uuid, rpa_resolution_uuid, 16)) {
- bool val, changed;
- int err;
- u32 flags;
+static int set_rpa_resolution_func(struct sock *sk, struct hci_dev *hdev,
+ struct mgmt_cp_set_exp_feature *cp,
+ u16 data_len)
+{
+ struct mgmt_rp_set_exp_feature rp;
+ bool val, changed;
+ int err;
+ u32 flags;
+
+ /* Command requires to use the controller index */
+ if (!hdev)
+ return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
+ MGMT_OP_SET_EXP_FEATURE,
+ MGMT_STATUS_INVALID_INDEX);

- /* Command requires to use the controller index */
- if (!hdev)
- return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_INVALID_INDEX);
+ /* Changes can only be made when controller is powered down */
+ if (hdev_is_powered(hdev))
+ return mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_EXP_FEATURE,
+ MGMT_STATUS_REJECTED);

- /* Changes can only be made when controller is powered down */
- if (hdev_is_powered(hdev))
- return mgmt_cmd_status(sk, hdev->id,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_REJECTED);
+ /* Parameters are limited to a single octet */
+ if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
+ return mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_EXP_FEATURE,
+ MGMT_STATUS_INVALID_PARAMS);

- /* Parameters are limited to a single octet */
- if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
- return mgmt_cmd_status(sk, hdev->id,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_INVALID_PARAMS);
+ /* Only boolean on/off is supported */
+ if (cp->param[0] != 0x00 && cp->param[0] != 0x01)
+ return mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_EXP_FEATURE,
+ MGMT_STATUS_INVALID_PARAMS);

- /* Only boolean on/off is supported */
- if (cp->param[0] != 0x00 && cp->param[0] != 0x01)
- return mgmt_cmd_status(sk, hdev->id,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_INVALID_PARAMS);
+ val = !!cp->param[0];

- val = !!cp->param[0];
+ if (val) {
+ changed = !hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);
+ hci_dev_set_flag(hdev, HCI_ENABLE_LL_PRIVACY);
+ hci_dev_clear_flag(hdev, HCI_ADVERTISING);

- if (val) {
- changed = !hci_dev_test_flag(hdev,
- HCI_ENABLE_LL_PRIVACY);
- hci_dev_set_flag(hdev, HCI_ENABLE_LL_PRIVACY);
- hci_dev_clear_flag(hdev, HCI_ADVERTISING);
+ /* Enable LL privacy + supported settings changed */
+ flags = BIT(0) | BIT(1);
+ } else {
+ changed = hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);
+ hci_dev_clear_flag(hdev, HCI_ENABLE_LL_PRIVACY);

- /* Enable LL privacy + supported settings changed */
- flags = BIT(0) | BIT(1);
- } else {
- changed = hci_dev_test_flag(hdev,
- HCI_ENABLE_LL_PRIVACY);
- hci_dev_clear_flag(hdev, HCI_ENABLE_LL_PRIVACY);
+ /* Disable LL privacy + supported settings changed */
+ flags = BIT(1);
+ }

- /* Disable LL privacy + supported settings changed */
- flags = BIT(1);
- }
+ memcpy(rp.uuid, rpa_resolution_uuid, 16);
+ rp.flags = cpu_to_le32(flags);

- memcpy(rp.uuid, rpa_resolution_uuid, 16);
- rp.flags = cpu_to_le32(flags);
+ hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);

- hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
+ err = mgmt_cmd_complete(sk, hdev->id,
+ MGMT_OP_SET_EXP_FEATURE, 0,
+ &rp, sizeof(rp));

- err = mgmt_cmd_complete(sk, hdev->id,
- MGMT_OP_SET_EXP_FEATURE, 0,
- &rp, sizeof(rp));
+ if (changed)
+ exp_ll_privacy_feature_changed(val, hdev, sk);

- if (changed)
- exp_ll_privacy_feature_changed(val, hdev, sk);
+ return err;
+}

- return err;
+static const struct mgmt_exp_feature {
+ const u8 *uuid;
+ int (*set_func)(struct sock *sk, struct hci_dev *hdev,
+ struct mgmt_cp_set_exp_feature *cp, u16 data_len);
+} exp_features[] = {
+ EXP_FEAT(ZERO_KEY, set_zero_key_func),
+#ifdef CONFIG_BT_FEATURE_DEBUG
+ EXP_FEAT(debug_uuid, set_debug_func),
+#endif
+ EXP_FEAT(rpa_resolution_uuid, set_rpa_resolution_func),
+
+ /* end with a null feature */
+ EXP_FEAT(NULL, NULL)
+};
+
+static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 data_len)
+{
+ struct mgmt_cp_set_exp_feature *cp = data;
+ size_t i = 0;
+
+ bt_dev_dbg(hdev, "sock %p", sk);
+
+ for (i = 0; exp_features[i].uuid; i++) {
+ if (!memcmp(cp->uuid, exp_features[i].uuid, 16))
+ return exp_features[i].set_func(sk, hdev, cp, data_len);
}

return mgmt_cmd_status(sk, hdev ? hdev->id : MGMT_INDEX_NONE,
--
2.33.0.rc1.237.g0d66db33f3-goog