* [PATCH v3 1/2] Bluetooth: btusb: support link statistics telemetry events
@ 2021-04-13 10:18 Joseph Hwang
2021-04-13 10:18 ` [PATCH v3 2/2] Bluetooth: Support the vendor specific debug events Joseph Hwang
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Joseph Hwang @ 2021-04-13 10:18 UTC (permalink / raw)
To: linux-bluetooth, marcel, luiz.dentz, pali
Cc: josephsih, chromeos-bluetooth-upstreaming, Chethan T N,
Miao-chen Chou, Kiran K, Joseph Hwang, Johan Hedberg,
linux-kernel
From: Chethan T N <chethan.tumkur.narayan@intel.com>
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 <mcchou@chromium.org>
Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Joseph Hwang <josephsih@chromium.org>
---
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 +++++++++++++++++++-
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);
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;
if (!features)
@@ -1266,8 +1268,24 @@ 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;
}
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)
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)
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)
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)
*/
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 2/2] Bluetooth: Support the vendor specific debug events 2021-04-13 10:18 [PATCH v3 1/2] Bluetooth: btusb: support link statistics telemetry events Joseph Hwang @ 2021-04-13 10:18 ` Joseph Hwang 2021-04-13 19:03 ` Marcel Holtmann 2021-04-13 11:13 ` [v3,1/2] Bluetooth: btusb: support link statistics telemetry events bluez.test.bot ` (2 subsequent siblings) 3 siblings, 1 reply; 6+ messages in thread From: Joseph Hwang @ 2021-04-13 10:18 UTC (permalink / raw) To: linux-bluetooth, marcel, luiz.dentz, pali Cc: josephsih, chromeos-bluetooth-upstreaming, Joseph Hwang, Chethan T N, Kiran Krishnappa, Miao-chen Chou, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev This patch allows a user space process to enable/disable the vendor specific (vs) debug events dynamically through the set experimental feature mgmt interface if CONFIG_BT_FEATURE_VS_DBG_EVT is enabled. Since the debug event feature needs to invoke the callback function provided by the driver, i.e., hdev->set_vs_dbg_evt, a valid controller index is required. For generic Linux machines, the vendor specific debug events are disabled by default. Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com> Reviewed-by: Kiran Krishnappa <kiran.k@intel.com> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> Signed-off-by: Joseph Hwang <josephsih@chromium.org> --- (no changes since v1) drivers/bluetooth/btintel.c | 73 ++++++++++++++++++++- drivers/bluetooth/btintel.h | 13 ++++ drivers/bluetooth/btusb.c | 16 +++++ include/net/bluetooth/hci.h | 4 ++ include/net/bluetooth/hci_core.h | 10 +++ net/bluetooth/Kconfig | 10 +++ net/bluetooth/mgmt.c | 108 ++++++++++++++++++++++++++++++- 7 files changed, 232 insertions(+), 2 deletions(-) diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c index de1dbdc01e5a..c0f81d29aa5f 100644 --- a/drivers/bluetooth/btintel.c +++ b/drivers/bluetooth/btintel.c @@ -1213,6 +1213,7 @@ void btintel_reset_to_bootloader(struct hci_dev *hdev) } EXPORT_SYMBOL_GPL(btintel_reset_to_bootloader); +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT int btintel_read_debug_features(struct hci_dev *hdev, struct intel_debug_features *features) { @@ -1254,14 +1255,18 @@ int btintel_set_debug_features(struct hci_dev *hdev, u8 trace_enable = 0x02; struct sk_buff *skb; - if (!features) + if (!features) { + bt_dev_warn(hdev, "Debug features not read"); return -EINVAL; + } if (!(features->page1[0] & 0x3f)) { bt_dev_info(hdev, "Telemetry exception format not supported"); return 0; } + bt_dev_info(hdev, "trace_enable %d mask %d", trace_enable, mask[3]); + skb = __hci_cmd_sync(hdev, 0xfc8b, 11, mask, HCI_INIT_TIMEOUT); if (IS_ERR(skb)) { bt_dev_err(hdev, "Setting Intel telemetry ddc write event mask failed (%ld)", @@ -1290,6 +1295,72 @@ int btintel_set_debug_features(struct hci_dev *hdev, } EXPORT_SYMBOL_GPL(btintel_set_debug_features); +int btintel_reset_debug_features(struct hci_dev *hdev, + const struct intel_debug_features *features) +{ + u8 mask[11] = { 0x0a, 0x92, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00 }; + u8 trace_enable = 0x00; + struct sk_buff *skb; + + if (!features) { + bt_dev_warn(hdev, "Debug features not read"); + return -EINVAL; + } + + if (!(features->page1[0] & 0x3f)) { + bt_dev_info(hdev, "Telemetry exception format not supported"); + return 0; + } + + bt_dev_info(hdev, "trace_enable %d mask %d", trace_enable, mask[3]); + + /* Should stop the trace before writing ddc event mask. */ + skb = __hci_cmd_sync(hdev, 0xfca1, 1, &trace_enable, HCI_INIT_TIMEOUT); + if (IS_ERR(skb)) { + bt_dev_err(hdev, "Stop tracing of link statistics events failed (%ld)", + PTR_ERR(skb)); + return PTR_ERR(skb); + } + kfree_skb(skb); + + skb = __hci_cmd_sync(hdev, 0xfc8b, 11, mask, HCI_INIT_TIMEOUT); + if (IS_ERR(skb)) { + bt_dev_err(hdev, "Setting Intel telemetry ddc write event mask failed (%ld)", + PTR_ERR(skb)); + return PTR_ERR(skb); + } + kfree_skb(skb); + + return 0; +} +EXPORT_SYMBOL_GPL(btintel_reset_debug_features); + +int btintel_set_vs_dbg_evt(struct hci_dev *hdev, bool enable) +{ + struct intel_debug_features features; + int err; + + bt_dev_dbg(hdev, "enable %d", enable); + + /* 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) + return err; + + /* Set or reset the debug features. */ + if (enable) + err = btintel_set_debug_features(hdev, &features); + else + err = btintel_reset_debug_features(hdev, &features); + + return err; +} +EXPORT_SYMBOL_GPL(btintel_set_vs_dbg_evt); +#endif + MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>"); MODULE_DESCRIPTION("Bluetooth support for Intel devices ver " VERSION); MODULE_VERSION(VERSION); diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h index d184064a5e7c..0b35b0248b91 100644 --- a/drivers/bluetooth/btintel.h +++ b/drivers/bluetooth/btintel.h @@ -171,10 +171,15 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev, u32 *boot_param, u8 hw_variant, u8 sbe_type); void btintel_reset_to_bootloader(struct hci_dev *hdev); +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT 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_reset_debug_features(struct hci_dev *hdev, + const struct intel_debug_features *features); +int btintel_set_vs_dbg_evt(struct hci_dev *hdev, bool enable); +#endif #else static inline int btintel_check_bdaddr(struct hci_dev *hdev) @@ -301,10 +306,18 @@ static inline int btintel_read_debug_features(struct hci_dev *hdev, return -EOPNOTSUPP; } +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT static inline int btintel_set_debug_features(struct hci_dev *hdev, const struct intel_debug_features *features) { return -EOPNOTSUPP; } +static inline int btintel_reset_debug_features(struct hci_dev *hdev, + const struct intel_debug_features *features) +{ + return -EOPNOTSUPP; +} +#endif + #endif diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index f29946f15f59..55e73d4f419f 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -2864,6 +2864,11 @@ static int btusb_setup_intel_new(struct hci_dev *hdev) btintel_load_ddc_config(hdev, ddcname); } +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT + hci_dev_clear_flag(hdev, HCI_VS_DBG_EVT); + bt_dev_dbg(hdev, "HCI_VS_DBG_EVT cleared"); +#endif + /* Read the Intel version information after loading the FW */ err = btintel_read_version(hdev, &ver); if (err) @@ -2951,6 +2956,11 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev) */ btintel_load_ddc_config(hdev, ddcname); +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT + hci_dev_clear_flag(hdev, HCI_VS_DBG_EVT); + bt_dev_dbg(hdev, "HCI_VS_DBG_EVT cleared"); +#endif + /* Read the Intel version information after loading the FW */ err = btintel_read_version_tlv(hdev, &version); if (err) @@ -4587,6 +4597,9 @@ static int btusb_probe(struct usb_interface *intf, hdev->set_diag = btintel_set_diag; hdev->set_bdaddr = btintel_set_bdaddr; hdev->cmd_timeout = btusb_intel_cmd_timeout; +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT + hdev->set_vs_dbg_evt = btintel_set_vs_dbg_evt; +#endif set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks); set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks); set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks); @@ -4601,6 +4614,9 @@ static int btusb_probe(struct usb_interface *intf, hdev->set_diag = btintel_set_diag; hdev->set_bdaddr = btintel_set_bdaddr; hdev->cmd_timeout = btusb_intel_cmd_timeout; +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT + hdev->set_vs_dbg_evt = btintel_set_vs_dbg_evt; +#endif set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks); set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks); set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks); diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index ea4ae551c426..0c1eba73844a 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -331,6 +331,10 @@ enum { HCI_CMD_PENDING, HCI_FORCE_NO_MITM, +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT + HCI_VS_DBG_EVT, +#endif + __HCI_NUM_FLAGS, }; diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index c73ac52af186..d68e06be51c7 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -604,6 +604,9 @@ struct hci_dev { int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr); void (*cmd_timeout)(struct hci_dev *hdev); bool (*prevent_wake)(struct hci_dev *hdev); +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT + int (*set_vs_dbg_evt)(struct hci_dev *hdev, bool enable); +#endif }; #define HCI_PHY_HANDLE(handle) (handle & 0xff) @@ -751,12 +754,19 @@ extern struct mutex hci_cb_list_lock; #define hci_dev_test_and_clear_flag(hdev, nr) test_and_clear_bit((nr), (hdev)->dev_flags) #define hci_dev_test_and_change_flag(hdev, nr) test_and_change_bit((nr), (hdev)->dev_flags) +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT +#define hci_dev_clear_flag_vs_dbg_evt(x) { hci_dev_clear_flag(hdev, x); } +#else +#define hci_dev_clear_flag_vs_dbg_evt(x) {} +#endif + #define hci_dev_clear_volatile_flags(hdev) \ do { \ hci_dev_clear_flag(hdev, HCI_LE_SCAN); \ hci_dev_clear_flag(hdev, HCI_LE_ADV); \ hci_dev_clear_flag(hdev, HCI_LL_RPA_RESOLUTION);\ hci_dev_clear_flag(hdev, HCI_PERIODIC_INQ); \ + hci_dev_clear_flag_vs_dbg_evt(HCI_VS_DBG_EVT) \ } while (0) /* ----- HCI interface to upper protocols ----- */ diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig index e0ab4cd7afc3..4930ec495f60 100644 --- a/net/bluetooth/Kconfig +++ b/net/bluetooth/Kconfig @@ -148,4 +148,14 @@ config BT_FEATURE_DEBUG This provides an option to enable/disable debugging statements at runtime via the experimental features interface. +config BT_FEATURE_VS_DBG_EVT + bool "Enable runtime option for logging vendor specific debug events" + depends on BT && !DYNAMIC_DEBUG + default n + help + This provides an option to enable/disable vendor specific debug + events logging at runtime via the experimental features interface. + The debug events may include the categories of system exceptions, + connections/disconnection, the link quality statistics, etc. + source "drivers/bluetooth/Kconfig" diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index f9be7f9084d6..a4344b78a6a0 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -3788,6 +3788,14 @@ static const u8 debug_uuid[16] = { }; #endif +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT +/* 330859bc-7506-492d-9370-9a6f0614037f */ +static const u8 vs_dbg_evt_uuid[16] = { + 0x7f, 0x03, 0x14, 0x06, 0x6f, 0x9a, 0x70, 0x93, + 0x2d, 0x49, 0x06, 0x75, 0xbc, 0x59, 0x08, 0x33, +}; +#endif + /* 671b10b5-42c0-4696-9227-eb28d1b049d6 */ static const u8 simult_central_periph_uuid[16] = { 0xd6, 0x49, 0xb0, 0xd1, 0x28, 0xeb, 0x27, 0x92, @@ -3803,7 +3811,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[82]; /* Enough space for 4 features: 2 + 20 * 4 */ struct mgmt_rp_read_exp_features_info *rp = (void *)buf; u16 idx = 0; u32 flags; @@ -3847,6 +3855,15 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev, idx++; } +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT + if (hdev) { + flags = hci_dev_test_flag(hdev, HCI_VS_DBG_EVT) ? BIT(0) : 0; + memcpy(rp->features[idx].uuid, vs_dbg_evt_uuid, 16); + rp->features[idx].flags = cpu_to_le32(flags); + idx++; + } +#endif + rp->feature_count = cpu_to_le16(idx); /* After reading the experimental features information, enable @@ -3889,6 +3906,23 @@ static int exp_debug_feature_changed(bool enabled, struct sock *skip) } #endif +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT +static int exp_vs_dbg_evt_feature_changed(bool enabled, struct sock *skip) +{ + struct mgmt_ev_exp_feature_changed ev; + + BT_INFO("enabled %d", enabled); + + memset(&ev, 0, sizeof(ev)); + memcpy(ev.uuid, vs_dbg_evt_uuid, 16); + ev.flags = cpu_to_le32(enabled ? BIT(0) : 0); + + return mgmt_limited_event(MGMT_EV_EXP_FEATURE_CHANGED, NULL, + &ev, sizeof(ev), + HCI_MGMT_EXP_FEATURE_EVENTS, skip); +} +#endif + static int set_exp_feature(struct sock *sk, struct hci_dev *hdev, void *data, u16 data_len) { @@ -4035,6 +4069,78 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev, return err; } +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT + if (!memcmp(cp->uuid, vs_dbg_evt_uuid, 16)) { + bool val, changed; + int err; + u16 mgmt_index = hdev ? hdev->id : MGMT_INDEX_NONE; + + /* Command requires to use a valid controller index */ + if (!hdev) + return mgmt_cmd_status(sk, hdev->id, + MGMT_OP_SET_EXP_FEATURE, + MGMT_STATUS_INVALID_INDEX); + + /* Parameters are limited to a single octet */ + if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1) + return mgmt_cmd_status(sk, mgmt_index, + 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, mgmt_index, + MGMT_OP_SET_EXP_FEATURE, + MGMT_STATUS_INVALID_PARAMS); + + hci_req_sync_lock(hdev); + + val = !!cp->param[0]; + changed = (val != hci_dev_test_flag(hdev, HCI_VS_DBG_EVT)); + + if (!hdev->set_vs_dbg_evt) { + BT_INFO("vendor specific debug event not supported"); + err = mgmt_cmd_status(sk, mgmt_index, + MGMT_OP_SET_EXP_FEATURE, + MGMT_STATUS_NOT_SUPPORTED); + goto unlock_vs_dbg_evt; + } + + if (changed) { + err = hdev->set_vs_dbg_evt(hdev, val); + if (err) { + BT_ERR("set_vs_dbg_evt value %d err %d", + val, err); + err = mgmt_cmd_status(sk, mgmt_index, + MGMT_OP_SET_EXP_FEATURE, + MGMT_STATUS_FAILED); + goto unlock_vs_dbg_evt; + } + if (val) + hci_dev_set_flag(hdev, HCI_VS_DBG_EVT); + else + hci_dev_clear_flag(hdev, HCI_VS_DBG_EVT); + } + + BT_INFO("vendor specific debug event %d changed %d", + val, changed); + + memcpy(rp.uuid, vs_dbg_evt_uuid, 16); + rp.flags = cpu_to_le32(val ? BIT(0) : 0); + hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS); + err = mgmt_cmd_complete(sk, mgmt_index, + MGMT_OP_SET_EXP_FEATURE, 0, + &rp, sizeof(rp)); + + if (changed) + exp_vs_dbg_evt_feature_changed(val, sk); + +unlock_vs_dbg_evt: + hci_req_sync_unlock(hdev); + return err; + } +#endif + return mgmt_cmd_status(sk, hdev ? hdev->id : MGMT_INDEX_NONE, MGMT_OP_SET_EXP_FEATURE, MGMT_STATUS_NOT_SUPPORTED); -- 2.31.1.295.g9ea45b61b8-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] Bluetooth: Support the vendor specific debug events 2021-04-13 10:18 ` [PATCH v3 2/2] Bluetooth: Support the vendor specific debug events Joseph Hwang @ 2021-04-13 19:03 ` Marcel Holtmann 0 siblings, 0 replies; 6+ messages in thread From: Marcel Holtmann @ 2021-04-13 19:03 UTC (permalink / raw) To: Joseph Hwang Cc: open list:BLUETOOTH DRIVERS, Luiz Augusto von Dentz, Pali Rohár, josephsih, CrosBT Upstreaming, Chethan T N, Kiran Krishnappa, Miao-chen Chou, David S. Miller, Jakub Kicinski, Johan Hedberg, open list, netdev Hi Joseph, > This patch allows a user space process to enable/disable the vendor > specific (vs) debug events dynamically through the set experimental > feature mgmt interface if CONFIG_BT_FEATURE_VS_DBG_EVT is enabled. > > Since the debug event feature needs to invoke the callback function > provided by the driver, i.e., hdev->set_vs_dbg_evt, a valid controller > index is required. > > For generic Linux machines, the vendor specific debug events are > disabled by default. > > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com> > Reviewed-by: Kiran Krishnappa <kiran.k@intel.com> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org> > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > --- > > (no changes since v1) > > drivers/bluetooth/btintel.c | 73 ++++++++++++++++++++- > drivers/bluetooth/btintel.h | 13 ++++ > drivers/bluetooth/btusb.c | 16 +++++ > include/net/bluetooth/hci.h | 4 ++ > include/net/bluetooth/hci_core.h | 10 +++ > net/bluetooth/Kconfig | 10 +++ > net/bluetooth/mgmt.c | 108 ++++++++++++++++++++++++++++++- > 7 files changed, 232 insertions(+), 2 deletions(-) > > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c > index de1dbdc01e5a..c0f81d29aa5f 100644 > --- a/drivers/bluetooth/btintel.c > +++ b/drivers/bluetooth/btintel.c > @@ -1213,6 +1213,7 @@ void btintel_reset_to_bootloader(struct hci_dev *hdev) > } > EXPORT_SYMBOL_GPL(btintel_reset_to_bootloader); > > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > int btintel_read_debug_features(struct hci_dev *hdev, > struct intel_debug_features *features) > { > @@ -1254,14 +1255,18 @@ int btintel_set_debug_features(struct hci_dev *hdev, > u8 trace_enable = 0x02; > struct sk_buff *skb; > > - if (!features) > + if (!features) { > + bt_dev_warn(hdev, "Debug features not read"); > return -EINVAL; > + } > > if (!(features->page1[0] & 0x3f)) { > bt_dev_info(hdev, "Telemetry exception format not supported"); > return 0; > } > > + bt_dev_info(hdev, "trace_enable %d mask %d", trace_enable, mask[3]); > + > skb = __hci_cmd_sync(hdev, 0xfc8b, 11, mask, HCI_INIT_TIMEOUT); > if (IS_ERR(skb)) { > bt_dev_err(hdev, "Setting Intel telemetry ddc write event mask failed (%ld)", > @@ -1290,6 +1295,72 @@ int btintel_set_debug_features(struct hci_dev *hdev, > } > EXPORT_SYMBOL_GPL(btintel_set_debug_features); > > +int btintel_reset_debug_features(struct hci_dev *hdev, > + const struct intel_debug_features *features) > +{ > + u8 mask[11] = { 0x0a, 0x92, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00 }; > + u8 trace_enable = 0x00; > + struct sk_buff *skb; > + > + if (!features) { > + bt_dev_warn(hdev, "Debug features not read"); > + return -EINVAL; > + } > + > + if (!(features->page1[0] & 0x3f)) { > + bt_dev_info(hdev, "Telemetry exception format not supported"); > + return 0; > + } > + > + bt_dev_info(hdev, "trace_enable %d mask %d", trace_enable, mask[3]); > + > + /* Should stop the trace before writing ddc event mask. */ > + skb = __hci_cmd_sync(hdev, 0xfca1, 1, &trace_enable, HCI_INIT_TIMEOUT); > + if (IS_ERR(skb)) { > + bt_dev_err(hdev, "Stop tracing of link statistics events failed (%ld)", > + PTR_ERR(skb)); > + return PTR_ERR(skb); > + } > + kfree_skb(skb); > + > + skb = __hci_cmd_sync(hdev, 0xfc8b, 11, mask, HCI_INIT_TIMEOUT); > + if (IS_ERR(skb)) { > + bt_dev_err(hdev, "Setting Intel telemetry ddc write event mask failed (%ld)", > + PTR_ERR(skb)); > + return PTR_ERR(skb); > + } > + kfree_skb(skb); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(btintel_reset_debug_features); > + > +int btintel_set_vs_dbg_evt(struct hci_dev *hdev, bool enable) > +{ > + struct intel_debug_features features; > + int err; > + > + bt_dev_dbg(hdev, "enable %d", enable); > + > + /* 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) > + return err; > + > + /* Set or reset the debug features. */ > + if (enable) > + err = btintel_set_debug_features(hdev, &features); > + else > + err = btintel_reset_debug_features(hdev, &features); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(btintel_set_vs_dbg_evt); > +#endif > + > MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>"); > MODULE_DESCRIPTION("Bluetooth support for Intel devices ver " VERSION); > MODULE_VERSION(VERSION); > diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h > index d184064a5e7c..0b35b0248b91 100644 > --- a/drivers/bluetooth/btintel.h > +++ b/drivers/bluetooth/btintel.h > @@ -171,10 +171,15 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev, > u32 *boot_param, u8 hw_variant, > u8 sbe_type); > void btintel_reset_to_bootloader(struct hci_dev *hdev); > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > 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_reset_debug_features(struct hci_dev *hdev, > + const struct intel_debug_features *features); > +int btintel_set_vs_dbg_evt(struct hci_dev *hdev, bool enable); > +#endif > #else > > static inline int btintel_check_bdaddr(struct hci_dev *hdev) > @@ -301,10 +306,18 @@ static inline int btintel_read_debug_features(struct hci_dev *hdev, > return -EOPNOTSUPP; > } > > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > static inline int btintel_set_debug_features(struct hci_dev *hdev, > const struct intel_debug_features *features) > { > return -EOPNOTSUPP; > } > > +static inline int btintel_reset_debug_features(struct hci_dev *hdev, > + const struct intel_debug_features *features) > +{ > + return -EOPNOTSUPP; > +} > +#endif > + > #endif > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index f29946f15f59..55e73d4f419f 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -2864,6 +2864,11 @@ static int btusb_setup_intel_new(struct hci_dev *hdev) > btintel_load_ddc_config(hdev, ddcname); > } > > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > + hci_dev_clear_flag(hdev, HCI_VS_DBG_EVT); > + bt_dev_dbg(hdev, "HCI_VS_DBG_EVT cleared"); > +#endif > + > /* Read the Intel version information after loading the FW */ > err = btintel_read_version(hdev, &ver); > if (err) > @@ -2951,6 +2956,11 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev) > */ > btintel_load_ddc_config(hdev, ddcname); > > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > + hci_dev_clear_flag(hdev, HCI_VS_DBG_EVT); > + bt_dev_dbg(hdev, "HCI_VS_DBG_EVT cleared"); > +#endif > + > /* Read the Intel version information after loading the FW */ > err = btintel_read_version_tlv(hdev, &version); > if (err) > @@ -4587,6 +4597,9 @@ static int btusb_probe(struct usb_interface *intf, > hdev->set_diag = btintel_set_diag; > hdev->set_bdaddr = btintel_set_bdaddr; > hdev->cmd_timeout = btusb_intel_cmd_timeout; > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > + hdev->set_vs_dbg_evt = btintel_set_vs_dbg_evt; > +#endif > set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks); > set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks); > set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks); > @@ -4601,6 +4614,9 @@ static int btusb_probe(struct usb_interface *intf, > hdev->set_diag = btintel_set_diag; > hdev->set_bdaddr = btintel_set_bdaddr; > hdev->cmd_timeout = btusb_intel_cmd_timeout; > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > + hdev->set_vs_dbg_evt = btintel_set_vs_dbg_evt; > +#endif > set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks); > set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks); > set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks); > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index ea4ae551c426..0c1eba73844a 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -331,6 +331,10 @@ enum { > HCI_CMD_PENDING, > HCI_FORCE_NO_MITM, > > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > + HCI_VS_DBG_EVT, > +#endif > + > __HCI_NUM_FLAGS, > }; > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index c73ac52af186..d68e06be51c7 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -604,6 +604,9 @@ struct hci_dev { > int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr); > void (*cmd_timeout)(struct hci_dev *hdev); > bool (*prevent_wake)(struct hci_dev *hdev); > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > + int (*set_vs_dbg_evt)(struct hci_dev *hdev, bool enable); > +#endif lets find a better name for this. > }; > > #define HCI_PHY_HANDLE(handle) (handle & 0xff) > @@ -751,12 +754,19 @@ extern struct mutex hci_cb_list_lock; > #define hci_dev_test_and_clear_flag(hdev, nr) test_and_clear_bit((nr), (hdev)->dev_flags) > #define hci_dev_test_and_change_flag(hdev, nr) test_and_change_bit((nr), (hdev)->dev_flags) > > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > +#define hci_dev_clear_flag_vs_dbg_evt(x) { hci_dev_clear_flag(hdev, x); } > +#else > +#define hci_dev_clear_flag_vs_dbg_evt(x) {} > +#endif > + > #define hci_dev_clear_volatile_flags(hdev) \ > do { \ > hci_dev_clear_flag(hdev, HCI_LE_SCAN); \ > hci_dev_clear_flag(hdev, HCI_LE_ADV); \ > hci_dev_clear_flag(hdev, HCI_LL_RPA_RESOLUTION);\ > hci_dev_clear_flag(hdev, HCI_PERIODIC_INQ); \ > + hci_dev_clear_flag_vs_dbg_evt(HCI_VS_DBG_EVT) \ > } while (0) > > /* ----- HCI interface to upper protocols ----- */ > diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig > index e0ab4cd7afc3..4930ec495f60 100644 > --- a/net/bluetooth/Kconfig > +++ b/net/bluetooth/Kconfig > @@ -148,4 +148,14 @@ config BT_FEATURE_DEBUG > This provides an option to enable/disable debugging statements > at runtime via the experimental features interface. > > +config BT_FEATURE_VS_DBG_EVT > + bool "Enable runtime option for logging vendor specific debug events" > + depends on BT && !DYNAMIC_DEBUG > + default n > + help > + This provides an option to enable/disable vendor specific debug > + events logging at runtime via the experimental features interface. > + The debug events may include the categories of system exceptions, > + connections/disconnection, the link quality statistics, etc. > + So I am not convinced this needs to be compile time option. I also don’t understand the !DYNAMIC_DEBUG portion. > source "drivers/bluetooth/Kconfig" > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c > index f9be7f9084d6..a4344b78a6a0 100644 > --- a/net/bluetooth/mgmt.c > +++ b/net/bluetooth/mgmt.c > @@ -3788,6 +3788,14 @@ static const u8 debug_uuid[16] = { > }; > #endif > > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > +/* 330859bc-7506-492d-9370-9a6f0614037f */ > +static const u8 vs_dbg_evt_uuid[16] = { > + 0x7f, 0x03, 0x14, 0x06, 0x6f, 0x9a, 0x70, 0x93, > + 0x2d, 0x49, 0x06, 0x75, 0xbc, 0x59, 0x08, 0x33, > +}; > +#endif > + > /* 671b10b5-42c0-4696-9227-eb28d1b049d6 */ > static const u8 simult_central_periph_uuid[16] = { > 0xd6, 0x49, 0xb0, 0xd1, 0x28, 0xeb, 0x27, 0x92, > @@ -3803,7 +3811,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[82]; /* Enough space for 4 features: 2 + 20 * 4 */ > struct mgmt_rp_read_exp_features_info *rp = (void *)buf; > u16 idx = 0; > u32 flags; > @@ -3847,6 +3855,15 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev, > idx++; > } > > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > + if (hdev) { > + flags = hci_dev_test_flag(hdev, HCI_VS_DBG_EVT) ? BIT(0) : 0; > + memcpy(rp->features[idx].uuid, vs_dbg_evt_uuid, 16); > + rp->features[idx].flags = cpu_to_le32(flags); > + idx++; > + } > +#endif > + > rp->feature_count = cpu_to_le16(idx); > > /* After reading the experimental features information, enable > @@ -3889,6 +3906,23 @@ static int exp_debug_feature_changed(bool enabled, struct sock *skip) > } > #endif > > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > +static int exp_vs_dbg_evt_feature_changed(bool enabled, struct sock *skip) > +{ > + struct mgmt_ev_exp_feature_changed ev; > + > + BT_INFO("enabled %d", enabled); > + > + memset(&ev, 0, sizeof(ev)); > + memcpy(ev.uuid, vs_dbg_evt_uuid, 16); > + ev.flags = cpu_to_le32(enabled ? BIT(0) : 0); > + > + return mgmt_limited_event(MGMT_EV_EXP_FEATURE_CHANGED, NULL, > + &ev, sizeof(ev), > + HCI_MGMT_EXP_FEATURE_EVENTS, skip); > +} > +#endif > + > static int set_exp_feature(struct sock *sk, struct hci_dev *hdev, > void *data, u16 data_len) > { > @@ -4035,6 +4069,78 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev, > return err; > } > > +#ifdef CONFIG_BT_FEATURE_VS_DBG_EVT > + if (!memcmp(cp->uuid, vs_dbg_evt_uuid, 16)) { > + bool val, changed; > + int err; > + u16 mgmt_index = hdev ? hdev->id : MGMT_INDEX_NONE; Since this is controller specific, this needs to be a valid hdev in the first place. > + > + /* Command requires to use a valid controller index */ > + if (!hdev) > + return mgmt_cmd_status(sk, hdev->id, > + MGMT_OP_SET_EXP_FEATURE, > + MGMT_STATUS_INVALID_INDEX); > + > + /* Parameters are limited to a single octet */ > + if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1) > + return mgmt_cmd_status(sk, mgmt_index, > + 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, mgmt_index, > + MGMT_OP_SET_EXP_FEATURE, > + MGMT_STATUS_INVALID_PARAMS); > + > + hci_req_sync_lock(hdev); > + > + val = !!cp->param[0]; > + changed = (val != hci_dev_test_flag(hdev, HCI_VS_DBG_EVT)); > + > + if (!hdev->set_vs_dbg_evt) { > + BT_INFO("vendor specific debug event not supported"); No, this is not how we do things. If there is no support for debug events, then we don’t advertise this experimental feature. > + err = mgmt_cmd_status(sk, mgmt_index, > + MGMT_OP_SET_EXP_FEATURE, > + MGMT_STATUS_NOT_SUPPORTED); > + goto unlock_vs_dbg_evt; > + } > + > + if (changed) { > + err = hdev->set_vs_dbg_evt(hdev, val); So is this something that requires a powered controller or is it fine to just do it on a powered down controller? > + if (err) { > + BT_ERR("set_vs_dbg_evt value %d err %d", > + val, err); > + err = mgmt_cmd_status(sk, mgmt_index, > + MGMT_OP_SET_EXP_FEATURE, > + MGMT_STATUS_FAILED); > + goto unlock_vs_dbg_evt; > + } > + if (val) > + hci_dev_set_flag(hdev, HCI_VS_DBG_EVT); > + else > + hci_dev_clear_flag(hdev, HCI_VS_DBG_EVT); > + } > + > + BT_INFO("vendor specific debug event %d changed %d", > + val, changed); > + > + memcpy(rp.uuid, vs_dbg_evt_uuid, 16); > + rp.flags = cpu_to_le32(val ? BIT(0) : 0); > + hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS); > + err = mgmt_cmd_complete(sk, mgmt_index, > + MGMT_OP_SET_EXP_FEATURE, 0, > + &rp, sizeof(rp)); > + > + if (changed) > + exp_vs_dbg_evt_feature_changed(val, sk); > + > +unlock_vs_dbg_evt: > + hci_req_sync_unlock(hdev); > + return err; > + } > +#endif > + > return mgmt_cmd_status(sk, hdev ? hdev->id : MGMT_INDEX_NONE, > MGMT_OP_SET_EXP_FEATURE, > MGMT_STATUS_NOT_SUPPORTED); Regards Marcel ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [v3,1/2] Bluetooth: btusb: support link statistics telemetry events 2021-04-13 10:18 [PATCH v3 1/2] Bluetooth: btusb: support link statistics telemetry events Joseph Hwang 2021-04-13 10:18 ` [PATCH v3 2/2] Bluetooth: Support the vendor specific debug events Joseph Hwang @ 2021-04-13 11:13 ` bluez.test.bot 2021-04-13 18:48 ` [PATCH v3 1/2] " Marcel Holtmann 2021-04-13 22:04 ` Luiz Augusto von Dentz 3 siblings, 0 replies; 6+ messages in thread From: bluez.test.bot @ 2021-04-13 11:13 UTC (permalink / raw) To: linux-bluetooth, josephsih [-- Attachment #1: Type: text/plain, Size: 1467 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=466255 ---Test result--- ############################## Test: CheckPatch - PASS ############################## Test: CheckGitLint - PASS ############################## Test: CheckBuildK - PASS ############################## Test: CheckTestRunner: Setup - PASS ############################## Test: CheckTestRunner: l2cap-tester - PASS Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6 ############################## Test: CheckTestRunner: bnep-tester - PASS Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 ############################## Test: CheckTestRunner: mgmt-tester - PASS Total: 416, Passed: 402 (96.6%), Failed: 0, Not Run: 14 ############################## Test: CheckTestRunner: rfcomm-tester - PASS Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0 ############################## Test: CheckTestRunner: sco-tester - PASS Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: CheckTestRunner: smp-tester - PASS Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: CheckTestRunner: userchan-tester - PASS Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0 --- Regards, Linux Bluetooth [-- Attachment #2: l2cap-tester.log --] [-- Type: application/octet-stream, Size: 43346 bytes --] [-- Attachment #3: bnep-tester.log --] [-- Type: application/octet-stream, Size: 3537 bytes --] [-- Attachment #4: mgmt-tester.log --] [-- Type: application/octet-stream, Size: 546684 bytes --] [-- Attachment #5: rfcomm-tester.log --] [-- Type: application/octet-stream, Size: 11657 bytes --] [-- Attachment #6: sco-tester.log --] [-- Type: application/octet-stream, Size: 9892 bytes --] [-- Attachment #7: smp-tester.log --] [-- Type: application/octet-stream, Size: 11803 bytes --] [-- Attachment #8: userchan-tester.log --] [-- Type: application/octet-stream, Size: 5433 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] Bluetooth: btusb: support link statistics telemetry events 2021-04-13 10:18 [PATCH v3 1/2] Bluetooth: btusb: support link statistics telemetry events Joseph Hwang 2021-04-13 10:18 ` [PATCH v3 2/2] Bluetooth: Support the vendor specific debug events Joseph Hwang 2021-04-13 11:13 ` [v3,1/2] Bluetooth: btusb: support link statistics telemetry events bluez.test.bot @ 2021-04-13 18:48 ` Marcel Holtmann 2021-04-13 22:04 ` Luiz Augusto von Dentz 3 siblings, 0 replies; 6+ messages in thread From: Marcel Holtmann @ 2021-04-13 18:48 UTC (permalink / raw) To: Joseph Hwang Cc: open list:BLUETOOTH DRIVERS, Luiz Augusto von Dentz, Pali Rohár, josephsih, CrosBT Upstreaming, Chethan T N, Miao-chen Chou, Kiran K, Johan Hedberg, linux-kernel Hi Joseph, > 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 <mcchou@chromium.org> > Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com> > Signed-off-by: Kiran K <kiran.k@intel.com> > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > --- > > 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 +++++++++++++++++++- > 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); > 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; > > if (!features) > @@ -1266,8 +1268,24 @@ 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; > } > 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) > 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) > 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) > 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) > */ > 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) so I don’t like this kind of removing and adding things at the same time. Please separate these patches properly. Regards Marcel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] Bluetooth: btusb: support link statistics telemetry events 2021-04-13 10:18 [PATCH v3 1/2] Bluetooth: btusb: support link statistics telemetry events Joseph Hwang ` (2 preceding siblings ...) 2021-04-13 18:48 ` [PATCH v3 1/2] " Marcel Holtmann @ 2021-04-13 22:04 ` Luiz Augusto von Dentz 3 siblings, 0 replies; 6+ messages in thread From: Luiz Augusto von Dentz @ 2021-04-13 22:04 UTC (permalink / raw) To: Joseph Hwang Cc: linux-bluetooth, Marcel Holtmann, Pali Rohár, Joseph Hwang, ChromeOS Bluetooth Upstreaming, Chethan T N, Miao-chen Chou, Kiran K, Johan Hedberg, Linux Kernel Mailing List Hi Joseph, On Tue, Apr 13, 2021 at 3:18 AM Joseph Hwang <josephsih@chromium.org> wrote: > > From: Chethan T N <chethan.tumkur.narayan@intel.com> > > 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 <mcchou@chromium.org> > Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com> > Signed-off-by: Kiran K <kiran.k@intel.com> > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > --- > > 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 +++++++++++++++++++- > 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); > 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; Looks like I commented on the wrong version, anyway the comments I made on v2 also apply here. > if (!features) > @@ -1266,8 +1268,24 @@ 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; > } > 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) > 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) > 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) > 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) > */ > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-13 22:04 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-13 10:18 [PATCH v3 1/2] Bluetooth: btusb: support link statistics telemetry events Joseph Hwang 2021-04-13 10:18 ` [PATCH v3 2/2] Bluetooth: Support the vendor specific debug events Joseph Hwang 2021-04-13 19:03 ` Marcel Holtmann 2021-04-13 11:13 ` [v3,1/2] Bluetooth: btusb: support link statistics telemetry events bluez.test.bot 2021-04-13 18:48 ` [PATCH v3 1/2] " Marcel Holtmann 2021-04-13 22:04 ` Luiz Augusto von Dentz
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.