[BlueZ PATCH v2 1/3] monitor: add new Intel extended telemetry events

[BlueZ PATCH v2 1/3] monitor: add new Intel extended telemetry events

[Joseph Hwang]

This patch adds new Intel extended telemetry events for both ACL and
SCO/eSCO audio link quality reports.

For SCO/eSCO audio link quality report, it shows something like
> HCI Event: Vendor (0xff) plen 190  #120 [hci0] 2021-05-31 20:27:50.257
      Intel Extended Telemetry (0x87)
        Extended Telemetry (0x80): SubOpcode (0x03)
        Extended event type (0x01): Audio Link Quality Report Type(0x05)
        SCO/eSCO connection handle (0x6a): 0x0101
        Packets from host (0x6b): 399
        Tx packets (0x6c): 403
        Rx payload lost (0x6d): 3
        Tx payload lost (0x6e): 0
        Rx No SYNC errors (0x6f): 3 2 3 3 0
        Rx HEC errors (0x70): 0 0 0 0 0
        Rx CRC errors (0x71): 2 0 0 0 0
        Rx NAK errors (0x72): 6 0 0 0 0
        Failed Tx due to Wifi coex (0x73): 6 0 0 0 0
        Failed Rx due to Wifi coex (0x74): 0 0 0 0 0
        Late samples inserted based on CDC (0x75): 0
        Samples dropped (0x76): 0
        Mute samples sent at initial connection (0x77): 0
        PLC injection data (0x78): 0

For ACL audio link quality report, it shows something like
> HCI Event: Vendor (0xff) plen 142  #120 [hci0] 2021-05-31 20:27:50.261
      Intel Extended Telemetry (0x87)
        Extended Telemetry (0x80): SubOpcode (0x03)
        Extended event type (0x01): Audio Link Quality Report Type(0x05)
        ACL connection handle (0x4a): 0x0100
        Rx HEC errors (0x4b): 0
        Rx CRC errors (0x4c): 0
        Packets from host (0x4d): 100
        Tx packets (0x4e): 101
        Tx packets 0 retries (0x4f): 89
        Tx packets 1 retries (0x50): 11
        Tx packets 2 retries (0x51): 1
        Tx packets 3 retries (0x52): 0
        Tx packets 4 retries and more (0x53): 0
        Tx DH1 packets (0x54): 0
        Tx DH3 packets (0x55): 0
        Tx DH5 packets (0x56): 0
        Tx 2DH1 packets (0x57): 0
        Tx 2DH3 packets (0x58): 0
        Tx 2DH5 packets (0x59): 0
        Tx 3DH1 packets (0x5a): 6
        Tx 3DH3 packets (0x5b): 0
        Tx 3DH5 packets (0x5c): 94
        Rx packets (0x5d): 272
        ACL link throughput (KBps) (0x5e): 343815
        ACL max packet latency (ms) (0x5f): 20625
        ACL avg packet latency (ms) (0x60): 12

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

Changes in v2:
- Perform size checks for tlv subevents.
- Fix the Make errors about qualifiers.

 monitor/intel.c  | 472 +++++++++++++++++++++++++++++++++++++++++++++++
 monitor/vendor.h |   6 +
 2 files changed, 478 insertions(+)

diff --git a/monitor/intel.c b/monitor/intel.c
index d2aefa6a8..af7402db4 100644
--- a/monitor/intel.c
+++ b/monitor/intel.c
@@ -30,6 +30,7 @@
 
 #define COLOR_UNKNOWN_EVENT_MASK	COLOR_WHITE_BG
 #define COLOR_UNKNOWN_SCAN_STATUS	COLOR_WHITE_BG
+#define COLOR_UNKNOWN_EXT_EVENT		COLOR_WHITE_BG
 
 static void print_status(uint8_t status)
 {
@@ -958,6 +959,8 @@ static void system_exception_evt(const void *data, uint8_t size)
 	packet_hexdump(data + 1, size - 1);
 }
 
+static void intel_vendor_ext_evt(const void *data, uint8_t size);
+
 static const struct vendor_evt vendor_evt_table[] = {
 	{ 0x00, "Startup",
 			startup_evt, 0, true },
@@ -989,6 +992,7 @@ static const struct vendor_evt vendor_evt_table[] = {
 			system_exception_evt, 133, true },
 	{ 0x2c, "FW Trace String" },
 	{ 0x2e, "FW Trace Binary" },
+	{ 0x87, "Extended Telemetry", intel_vendor_ext_evt },
 	{ }
 };
 
@@ -1003,3 +1007,471 @@ const struct vendor_evt *intel_vendor_evt(uint8_t evt)
 
 	return NULL;
 }
+
+static void ext_evt_type(uint8_t subevent_id, const void *data, uint8_t size)
+{
+	uint8_t evt_type = get_u8(data);
+	const char *str;
+
+	switch (evt_type) {
+	case 0x00:
+		str = "System Exception";
+		break;
+	case 0x01:
+		str = "Fatal Exception";
+		break;
+	case 0x02:
+		str = "Debug Exception";
+		break;
+	case 0x03:
+		str = "Connection Event for BR/EDR Link Type";
+		break;
+	case 0x04:
+		str = "Disconnection Event";
+		break;
+	case 0x05:
+		str = "Audio Link Quality Report Type";
+		break;
+	case 0x06:
+		str = "Stats for BR/EDR Link Type";
+		break;
+	default:
+		print_text(COLOR_UNKNOWN_EXT_EVENT,
+			"Unknown extended telemetry event type (0x%2.2x)",
+			evt_type);
+		packet_hexdump(data, size);
+		return;
+	}
+
+	print_field("Extended event type (0x%2.2x): %s (0x%2.2x)",
+			subevent_id, str, evt_type);
+}
+
+static void ext_acl_evt_conn_handle(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	uint16_t conn_handle = get_le16(data);
+
+	print_field("ACL connection handle (0x%2.2x): 0x%4.4x",
+			subevent_id, conn_handle);
+}
+
+static void ext_acl_evt_hec_errors(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Rx HEC errors (0x%2.2x): %d", subevent_id, num);
+}
+
+static void ext_acl_evt_crc_errors(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Rx CRC errors (0x%2.2x): %d", subevent_id, num);
+}
+
+static void ext_acl_evt_num_pkt_from_host(uint8_t subevent_id, const void *data,
+						uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Packets from host (0x%2.2x): %d",
+			subevent_id, num);
+}
+
+static void ext_acl_evt_num_tx_pkt_to_air(uint8_t subevent_id, const void *data,
+						uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Tx packets (0x%2.2x): %d", subevent_id, num);
+}
+
+static void ext_acl_evt_num_tx_pkt_retry(uint8_t subevent_id, const void *data,
+						uint8_t size)
+{
+	char *subevent_str;
+	uint32_t num = get_le32(data);
+
+	switch (subevent_id) {
+	case 0x4f:
+		subevent_str = "Tx packets 0 retries";
+		break;
+	case 0x50:
+		subevent_str = "Tx packets 1 retries";
+		break;
+	case 0x51:
+		subevent_str = "Tx packets 2 retries";
+		break;
+	case 0x52:
+		subevent_str = "Tx packets 3 retries";
+		break;
+	case 0x53:
+		subevent_str = "Tx packets 4 retries and more";
+		break;
+	default:
+		subevent_str = "Unknown";
+		break;
+	}
+
+	print_field("%s (0x%2.2x): %d", subevent_str, subevent_id, num);
+}
+
+static void ext_acl_evt_num_tx_pkt_type(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	char *packet_type_str;
+	uint32_t num = get_le32(data);
+
+	switch (subevent_id) {
+	case 0x54:
+		packet_type_str = "DH1";
+		break;
+	case 0x55:
+		packet_type_str = "DH3";
+		break;
+	case 0x56:
+		packet_type_str = "DH5";
+		break;
+	case 0x57:
+		packet_type_str = "2DH1";
+		break;
+	case 0x58:
+		packet_type_str = "2DH3";
+		break;
+	case 0x59:
+		packet_type_str = "2DH5";
+		break;
+	case 0x5a:
+		packet_type_str = "3DH1";
+		break;
+	case 0x5b:
+		packet_type_str = "3DH3";
+		break;
+	case 0x5c:
+		packet_type_str = "3DH5";
+		break;
+	default:
+		packet_type_str = "Unknown";
+		break;
+	}
+
+	print_field("Tx %s packets (0x%2.2x): %d",
+			packet_type_str, subevent_id, num);
+}
+
+static void ext_acl_evt_num_rx_pkt_from_air(uint8_t subevent_id,
+						const void *data, uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Rx packets (0x%2.2x): %d",
+			subevent_id, num);
+}
+
+static void ext_acl_evt_link_throughput(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("ACL link throughput (KBps) (0x%2.2x): %d",
+			subevent_id, num);
+}
+
+static void ext_acl_evt_max_packet_latency(uint8_t subevent_id,
+						const void *data, uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("ACL max packet latency (ms) (0x%2.2x): %d",
+			subevent_id, num);
+}
+
+static void ext_acl_evt_avg_packet_latency(uint8_t subevent_id,
+						const void *data, uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("ACL avg packet latency (ms) (0x%2.2x): %d",
+			subevent_id, num);
+}
+
+static void ext_sco_evt_conn_handle(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	uint16_t conn_handle = get_le16(data);
+
+	print_field("SCO/eSCO connection handle (0x%2.2x): 0x%4.4x",
+			subevent_id, conn_handle);
+}
+
+static void ext_sco_evt_num_rx_pkt_from_air(uint8_t subevent_id,
+						const void *data, uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Packets from host (0x%2.2x): %d", subevent_id, num);
+}
+
+static void ext_sco_evt_num_tx_pkt_to_air(uint8_t subevent_id, const void *data,
+						uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Tx packets (0x%2.2x): %d", subevent_id, num);
+}
+
+static void ext_sco_evt_num_rx_payloads_lost(uint8_t subevent_id,
+						const void *data, uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Rx payload lost (0x%2.2x): %d", subevent_id, num);
+}
+
+static void ext_sco_evt_num_tx_payloads_lost(uint8_t subevent_id,
+						const void *data, uint8_t size)
+{
+
+	uint32_t num = get_le32(data);
+
+	print_field("Tx payload lost (0x%2.2x): %d", subevent_id, num);
+}
+
+static void slots_errors(const char *type_str, uint8_t subevent_id,
+				const void *data, uint8_t size)
+{
+	/* The subevent has 5 slots where each slot is of the uint32_t type. */
+	uint32_t num[5];
+	int i;
+
+	if (size != 5 * sizeof(uint32_t)) {
+		print_text(COLOR_UNKNOWN_EXT_EVENT,
+				"  Invalid subevent length (%d)", size);
+		return;
+	}
+
+	for (i = 0; i < 5; i++) {
+		num[i] = get_le32(data);
+		data = (const void *)((const uint8_t *) data +
+						sizeof(uint32_t));
+	}
+
+	print_field("%s (0x%2.2x): %d %d %d %d %d", type_str, subevent_id,
+			num[0], num[1], num[2], num[3], num[4]);
+}
+
+static void ext_sco_evt_num_no_sync_errors(uint8_t subevent_id,
+						const void *data, uint8_t size)
+{
+	slots_errors("Rx No SYNC errors", subevent_id, data, size);
+}
+
+static void ext_sco_evt_num_hec_errors(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	slots_errors("Rx HEC errors", subevent_id, data, size);
+}
+
+static void ext_sco_evt_num_crc_errors(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	slots_errors("Rx CRC errors", subevent_id, data, size);
+}
+
+static void ext_sco_evt_num_naks(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	slots_errors("Rx NAK errors", subevent_id, data, size);
+}
+
+static void ext_sco_evt_num_failed_tx_by_wifi(uint8_t subevent_id,
+						const void *data, uint8_t size)
+{
+	slots_errors("Failed Tx due to Wifi coex", subevent_id, data, size);
+}
+
+static void ext_sco_evt_num_failed_rx_by_wifi(uint8_t subevent_id,
+						const void *data, uint8_t size)
+{
+	slots_errors("Failed Rx due to Wifi coex", subevent_id, data, size);
+}
+
+static void ext_sco_evt_samples_inserted(uint8_t subevent_id, const void *data,
+						uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Late samples inserted based on CDC (0x%2.2x): %d",
+			subevent_id, num);
+}
+
+static void ext_sco_evt_samples_dropped(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Samples dropped (0x%2.2x): %d", subevent_id, num);
+}
+
+static void ext_sco_evt_mute_samples(uint8_t subevent_id, const void *data,
+					uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("Mute samples sent at initial connection (0x%2.2x): %d",
+			subevent_id, num);
+}
+
+static void ext_sco_evt_plc_injection_data(uint8_t subevent_id,
+						const void *data, uint8_t size)
+{
+	uint32_t num = get_le32(data);
+
+	print_field("PLC injection data (0x%2.2x): %d", subevent_id, num);
+}
+
+
+static const struct vendor_ext_subevent vendor_ext_subevent_table[] = {
+	{ 0x01, 1, ext_evt_type },
+
+	/* ACL audio link quality subevents */
+	{ 0x4a, 2, ext_acl_evt_conn_handle },
+	{ 0x4b, 4, ext_acl_evt_hec_errors },
+	{ 0x4c, 4, ext_acl_evt_crc_errors },
+	{ 0x4d, 4, ext_acl_evt_num_pkt_from_host },
+	{ 0x4e, 4, ext_acl_evt_num_tx_pkt_to_air },
+	{ 0x4f, 4, ext_acl_evt_num_tx_pkt_retry },
+	{ 0x50, 4, ext_acl_evt_num_tx_pkt_retry },
+	{ 0x51, 4, ext_acl_evt_num_tx_pkt_retry },
+	{ 0x52, 4, ext_acl_evt_num_tx_pkt_retry },
+	{ 0x53, 4, ext_acl_evt_num_tx_pkt_retry },
+	{ 0x54, 4, ext_acl_evt_num_tx_pkt_type },
+	{ 0x55, 4, ext_acl_evt_num_tx_pkt_type },
+	{ 0x56, 4, ext_acl_evt_num_tx_pkt_type },
+	{ 0x57, 4, ext_acl_evt_num_tx_pkt_type },
+	{ 0x58, 4, ext_acl_evt_num_tx_pkt_type },
+	{ 0x59, 4, ext_acl_evt_num_tx_pkt_type },
+	{ 0x5a, 4, ext_acl_evt_num_tx_pkt_type },
+	{ 0x5b, 4, ext_acl_evt_num_tx_pkt_type },
+	{ 0x5c, 4, ext_acl_evt_num_tx_pkt_type },
+	{ 0x5d, 4, ext_acl_evt_num_rx_pkt_from_air },
+	{ 0x5e, 4, ext_acl_evt_link_throughput },
+	{ 0x5f, 4, ext_acl_evt_max_packet_latency },
+	{ 0x60, 4, ext_acl_evt_avg_packet_latency },
+
+	/* SCO/eSCO audio link quality subevents */
+	{ 0x6a, 2, ext_sco_evt_conn_handle },
+	{ 0x6b, 4, ext_sco_evt_num_rx_pkt_from_air },
+	{ 0x6c, 4, ext_sco_evt_num_tx_pkt_to_air },
+	{ 0x6d, 4, ext_sco_evt_num_rx_payloads_lost },
+	{ 0x6e, 4, ext_sco_evt_num_tx_payloads_lost },
+	{ 0x6f, 20, ext_sco_evt_num_no_sync_errors },
+	{ 0x70, 20, ext_sco_evt_num_hec_errors },
+	{ 0x71, 20, ext_sco_evt_num_crc_errors },
+	{ 0x72, 20, ext_sco_evt_num_naks },
+	{ 0x73, 20, ext_sco_evt_num_failed_tx_by_wifi },
+	{ 0x74, 20, ext_sco_evt_num_failed_rx_by_wifi },
+	{ 0x75, 4, ext_sco_evt_samples_inserted },
+	{ 0x76, 4, ext_sco_evt_samples_dropped },
+	{ 0x77, 4, ext_sco_evt_mute_samples },
+	{ 0x78, 4, ext_sco_evt_plc_injection_data },
+
+	/* end */
+	{ 0x0, 0}
+};
+
+static const uint8_t *process_ext_subevent(const uint8_t *data,
+					const uint8_t *event_end)
+{
+	/*
+	 * A subevent is of the TLV format.
+	 * - Type: takes 1 byte. This is the subevent_id.
+	 * - Length: takes 1 byte.
+	 * - Value: takes |Length| bytes.
+	 */
+	uint8_t subevent_id = *data;
+	uint8_t length = *(data + sizeof(uint8_t));
+	const uint8_t *next_tlv = (const uint8_t *) data + 2 * sizeof(uint8_t) +
+					length;
+	const struct vendor_ext_subevent *subevent = NULL;
+	int i;
+
+	if (next_tlv > event_end) {
+		print_text(COLOR_ERROR, "Subevent exceeds the buffer size.");
+		return NULL;
+	}
+
+	for (i = 0; vendor_ext_subevent_table[i].length > 0; i++) {
+		if (vendor_ext_subevent_table[i].subevent_id == subevent_id) {
+			subevent = &vendor_ext_subevent_table[i];
+			break;
+		}
+	}
+
+	if (!subevent) {
+		print_text(COLOR_UNKNOWN_EXT_EVENT,
+				"Unknown extended event type id (0x%2.2x)",
+				subevent_id);
+		return NULL;
+	}
+
+	if (length != subevent->length) {
+		print_text(COLOR_ERROR, "Invalid subevent length");
+		return NULL;
+	}
+
+	subevent->func(subevent_id, data + 2 * sizeof(uint8_t), length);
+
+	return next_tlv;
+}
+
+static void intel_vendor_ext_evt(const void *data, uint8_t size)
+{
+	const uint8_t *tlv = (const uint8_t *) data;
+	uint8_t ext_telemetry_type = *tlv;
+	uint8_t sub_opcode = *(tlv + sizeof(uint8_t));
+	const uint8_t *event_end = (const uint8_t *) data + size;
+
+	/*
+	 * Intel extended telemetry event is of the format:
+	 *   Event Code: 0xff
+	 *   Length:     0xnn
+	 *   Event Type: 0x8780
+	 *   SubOpcode:  0x03
+	 *   A number of TLV data
+	 *
+	 *   Each TLV Data, i.e., a subevent, consists of
+	 *     Subevent Id
+	 *     Length
+	 *     Value
+	 *
+	 * Arrives here through the telemetry event (0x87) in vendor_evt_table.
+	 * It is required to further check the extended telemetry type (0x80)
+	 * and sub-opcode (0x03) in the tlv data.
+	 */
+	if (ext_telemetry_type == 0x80 && sub_opcode == 0x03) {
+		/* Consumed two octets, i.e., 0x80 and 0x03. */
+		print_field("Extended Telemetry (0x%2.2x): SubOpcode (0x%2.2x)",
+				ext_telemetry_type, sub_opcode);
+		tlv += 2 * sizeof(uint8_t);
+	} else {
+		print_text(COLOR_UNKNOWN_EXT_EVENT,
+			"  Unknown extended telemetry event (0x%2.2x 0x%2.2x)",
+			ext_telemetry_type, sub_opcode);
+		packet_hexdump(data, size);
+		return;
+	}
+
+	/* Process every TLV subevent data.
+	 * The decoding process normally terminates when tlv == event_end.
+	 */
+	while (tlv && tlv < event_end)
+		tlv = process_ext_subevent(tlv, event_end);
+
+	/* If an error occrus in decoding the subevents, hexdump the packet. */
+	if (!tlv)
+		packet_hexdump(data, size);
+}
diff --git a/monitor/vendor.h b/monitor/vendor.h
index c70552b66..347b4bfe9 100644
--- a/monitor/vendor.h
+++ b/monitor/vendor.h
@@ -30,4 +30,10 @@ struct vendor_evt {
 	bool evt_fixed;
 };
 
+struct vendor_ext_subevent {
+	uint8_t subevent_id;
+	uint8_t length;
+	void (*func)(uint8_t subevent_id, const void *data, uint8_t size);
+};
+
 void vendor_event(uint16_t manufacturer, const void *data, uint8_t size);
-- 
2.32.0.288.g62a8d224e6-goog

[BlueZ PATCH v2 2/3] adapter: read quality report feature

[Joseph Hwang]

This patch adds a new UUID for the quality report experimental
feature. When reading the experimental features, it checks if
the new feature is supported by the controller and stores the
value in the quality_report_supported flag of the adapter.

The quality_report_supported flag could be used by the bluetoothd
to determine if the quality report feature can be enabled.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

(no changes since v1)

 src/adapter.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index 98fc78f1e..e2873de46 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -284,6 +284,7 @@ struct btd_adapter {
 	bool is_default;		/* true if adapter is default one */
 
 	bool le_simult_roles_supported;
+	bool quality_report_supported;
 };
 
 typedef enum {
@@ -9234,6 +9235,12 @@ static const uint8_t le_simult_central_peripheral_uuid[16] = {
 	0x96, 0x46, 0xc0, 0x42, 0xb5, 0x10, 0x1b, 0x67,
 };
 
+/* 330859bc-7506-492d-9370-9a6f0614037f */
+static const uint8_t quality_report_uuid[16] = {
+	0x7f, 0x03, 0x14, 0x06, 0x6f, 0x9a, 0x70, 0x93,
+	0x2d, 0x49, 0x06, 0x75, 0xbc, 0x59, 0x08, 0x33,
+};
+
 /* 15c0a148-c273-11ea-b3de-0242ac130004 */
 static const uint8_t rpa_resolution_uuid[16] = {
 	0x04, 0x00, 0x13, 0xac, 0x42, 0x02, 0xde, 0xb3,
@@ -9276,6 +9283,14 @@ static void le_simult_central_peripheral_func(struct btd_adapter *adapter,
 	adapter->le_simult_roles_supported = flags & 0x01;
 }
 
+static void quality_report_func(struct btd_adapter *adapter, uint32_t flags)
+{
+	adapter->quality_report_supported = le32_to_cpu(flags) & 0x01;
+
+	btd_info(adapter->dev_id, "quality_report_supported %d",
+			adapter->quality_report_supported);
+}
+
 static void set_rpa_resolution_complete(uint8_t status, uint16_t len,
 					const void *param, void *user_data)
 {
@@ -9313,6 +9328,7 @@ static const struct exp_feat {
 	EXP_FEAT(debug_uuid, exp_debug_func),
 	EXP_FEAT(le_simult_central_peripheral_uuid,
 		 le_simult_central_peripheral_func),
+	EXP_FEAT(quality_report_uuid, quality_report_func),
 	EXP_FEAT(rpa_resolution_uuid, rpa_resolution_func),
 };
 
-- 
2.32.0.288.g62a8d224e6-goog

[BlueZ PATCH v2 3/3] adapter: set quality report feature

[Joseph Hwang]

This patch adds the function to enable/disable the quality report
experimental feature in the controller through MGMT_OP_SET_EXP_FEATURE.

A user space process can enable/disable the quality report feature
by sending a property changed signal to the bluetoothd. The bluetoothd
can set up the signal handlers to handle the signal in a file under
plugins/ to call this function.

Note that the bluetoothd calls the experimental feature only when
the quality_report_supported flag is true.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

(no changes since v1)

 src/adapter.c | 36 ++++++++++++++++++++++++++++++++++++
 src/adapter.h |  2 ++
 2 files changed, 38 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index e2873de46..829d9806b 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -9332,6 +9332,42 @@ static const struct exp_feat {
 	EXP_FEAT(rpa_resolution_uuid, rpa_resolution_func),
 };
 
+/* A user space process can enable/disable the quality report feature
+ * by sending a property changed signal to the bluetoothd. The bluetoothd
+ * can set up the signal handlers in a file under plugins/ to call
+ * this function.
+ */
+void btd_adapter_update_kernel_quality_report(uint8_t action)
+{
+	struct mgmt_cp_set_exp_feature cp;
+	struct btd_adapter *adapter;
+
+	adapter = btd_adapter_get_default();
+	if (!adapter) {
+		info("No default adapter. Skip enabling quality report.");
+		return;
+	}
+
+	if (!adapter->quality_report_supported) {
+		info("quality report feature not supported.");
+		return;
+	}
+
+	memset(&cp, 0, sizeof(cp));
+	memcpy(cp.uuid, quality_report_uuid, 16);
+
+	cp.action = action;
+	if (cp.action > 1) {
+		error("Unexpected quality report action %u", cp.action);
+		return;
+	}
+
+	mgmt_send(adapter->mgmt, MGMT_OP_SET_EXP_FEATURE, adapter->dev_id,
+			sizeof(cp), &cp, NULL, NULL, NULL);
+	info("update kernel quality report default adapter %d enable %d",
+		adapter->dev_id, cp.action);
+}
+
 static void read_exp_features_complete(uint8_t status, uint16_t length,
 					const void *param, void *user_data)
 {
diff --git a/src/adapter.h b/src/adapter.h
index 60b5e3bcc..001f784e4 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -240,3 +240,5 @@ enum kernel_features {
 };
 
 bool btd_has_kernel_features(uint32_t feature);
+
+void btd_adapter_update_kernel_quality_report(uint8_t action);
-- 
2.32.0.288.g62a8d224e6-goog

Re: [BlueZ PATCH v2 3/3] adapter: set quality report feature

[Luiz Augusto von Dentz]

Hi Joseph,

On Mon, Jun 21, 2021 at 3:23 AM Joseph Hwang <josephsih@chromium.org> wrote:
>
> This patch adds the function to enable/disable the quality report
> experimental feature in the controller through MGMT_OP_SET_EXP_FEATURE.
>
> A user space process can enable/disable the quality report feature
> by sending a property changed signal to the bluetoothd. The bluetoothd
> can set up the signal handlers to handle the signal in a file under
> plugins/ to call this function.
>
> Note that the bluetoothd calls the experimental feature only when
> the quality_report_supported flag is true.
>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
>
> (no changes since v1)
>
>  src/adapter.c | 36 ++++++++++++++++++++++++++++++++++++
>  src/adapter.h |  2 ++
>  2 files changed, 38 insertions(+)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index e2873de46..829d9806b 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -9332,6 +9332,42 @@ static const struct exp_feat {
>         EXP_FEAT(rpa_resolution_uuid, rpa_resolution_func),
>  };
>
> +/* A user space process can enable/disable the quality report feature
> + * by sending a property changed signal to the bluetoothd. The bluetoothd
> + * can set up the signal handlers in a file under plugins/ to call
> + * this function.
> + */
> +void btd_adapter_update_kernel_quality_report(uint8_t action)
> +{
> +       struct mgmt_cp_set_exp_feature cp;
> +       struct btd_adapter *adapter;
> +
> +       adapter = btd_adapter_get_default();
> +       if (!adapter) {
> +               info("No default adapter. Skip enabling quality report.");
> +               return;
> +       }
> +
> +       if (!adapter->quality_report_supported) {
> +               info("quality report feature not supported.");
> +               return;
> +       }
> +
> +       memset(&cp, 0, sizeof(cp));
> +       memcpy(cp.uuid, quality_report_uuid, 16);
> +
> +       cp.action = action;
> +       if (cp.action > 1) {
> +               error("Unexpected quality report action %u", cp.action);
> +               return;
> +       }
> +
> +       mgmt_send(adapter->mgmt, MGMT_OP_SET_EXP_FEATURE, adapter->dev_id,
> +                       sizeof(cp), &cp, NULL, NULL, NULL);
> +       info("update kernel quality report default adapter %d enable %d",
> +               adapter->dev_id, cp.action);
> +}
> +
>  static void read_exp_features_complete(uint8_t status, uint16_t length,
>                                         const void *param, void *user_data)
>  {
> diff --git a/src/adapter.h b/src/adapter.h
> index 60b5e3bcc..001f784e4 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -240,3 +240,5 @@ enum kernel_features {
>  };
>
>  bool btd_has_kernel_features(uint32_t feature);
> +
> +void btd_adapter_update_kernel_quality_report(uint8_t action);

It doesn't seem this is being used anywhere, in the patch description
it mentions a plugin is handling this via a signal but we could
actually make the core do that directly, in fact having a plugin
handling posix signals might be a bad idea. If this is something we
don't need to change at runtime I would expect it to be configurable
over main.conf, or are there legitimate reasons to not have the
controller generating these events all the time?

> --
> 2.32.0.288.g62a8d224e6-goog
>


-- 
Luiz Augusto von Dentz

Re: [BlueZ PATCH v2 3/3] adapter: set quality report feature

[Joseph Hwang]

Please read the replies in the lines below.

Thanks!

On Tue, Jun 22, 2021 at 2:25 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Joseph,
>
> On Mon, Jun 21, 2021 at 3:23 AM Joseph Hwang <josephsih@chromium.org> wrote:
> >
> > This patch adds the function to enable/disable the quality report
> > experimental feature in the controller through MGMT_OP_SET_EXP_FEATURE.
> >
> > A user space process can enable/disable the quality report feature
> > by sending a property changed signal to the bluetoothd. The bluetoothd
> > can set up the signal handlers to handle the signal in a file under
> > plugins/ to call this function.
> >
> > Note that the bluetoothd calls the experimental feature only when
> > the quality_report_supported flag is true.
> >
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  src/adapter.c | 36 ++++++++++++++++++++++++++++++++++++
> >  src/adapter.h |  2 ++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index e2873de46..829d9806b 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -9332,6 +9332,42 @@ static const struct exp_feat {
> >         EXP_FEAT(rpa_resolution_uuid, rpa_resolution_func),
> >  };
> >
> > +/* A user space process can enable/disable the quality report feature
> > + * by sending a property changed signal to the bluetoothd. The bluetoothd
> > + * can set up the signal handlers in a file under plugins/ to call
> > + * this function.
> > + */
> > +void btd_adapter_update_kernel_quality_report(uint8_t action)
> > +{
> > +       struct mgmt_cp_set_exp_feature cp;
> > +       struct btd_adapter *adapter;
> > +
> > +       adapter = btd_adapter_get_default();
> > +       if (!adapter) {
> > +               info("No default adapter. Skip enabling quality report.");
> > +               return;
> > +       }
> > +
> > +       if (!adapter->quality_report_supported) {
> > +               info("quality report feature not supported.");
> > +               return;
> > +       }
> > +
> > +       memset(&cp, 0, sizeof(cp));
> > +       memcpy(cp.uuid, quality_report_uuid, 16);
> > +
> > +       cp.action = action;
> > +       if (cp.action > 1) {
> > +               error("Unexpected quality report action %u", cp.action);
> > +               return;
> > +       }
> > +
> > +       mgmt_send(adapter->mgmt, MGMT_OP_SET_EXP_FEATURE, adapter->dev_id,
> > +                       sizeof(cp), &cp, NULL, NULL, NULL);
> > +       info("update kernel quality report default adapter %d enable %d",
> > +               adapter->dev_id, cp.action);
> > +}
> > +
> >  static void read_exp_features_complete(uint8_t status, uint16_t length,
> >                                         const void *param, void *user_data)
> >  {
> > diff --git a/src/adapter.h b/src/adapter.h
> > index 60b5e3bcc..001f784e4 100644
> > --- a/src/adapter.h
> > +++ b/src/adapter.h
> > @@ -240,3 +240,5 @@ enum kernel_features {
> >  };
> >
> >  bool btd_has_kernel_features(uint32_t feature);
> > +
> > +void btd_adapter_update_kernel_quality_report(uint8_t action);
>
> It doesn't seem this is being used anywhere, in the patch description

  It is not used elsewhere except in plugins/chromium.c. The function
is placed here so that
other user processes in the Linux community can use it very easily.
Please refer to how it is invoked
in chrome os

https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/bluez/current/plugins/chromium.c;l=748?q=btd_adapter_update_kernel_quality_report&ss=chromiumos

  This quality report experimental feature
(CONFIG_BT_FEATURE_QUALITY_REPORT) follows what was implemented for
the debug log experimental feature (CONFIG_BT_FEATURE_DEBUG).

> it mentions a plugin is handling this via a signal but we could
> actually make the core do that directly, in fact having a plugin
> handling posix signals might be a bad idea. If this is something we
> don't need to change at runtime I would expect it to be configurable

In chrome os, this quality report experimental feature, just like the
other debug log experimental feature, is required to change at run
time. Both experimental features are enabled/disabled at run time with
the same mechanism -- a user space process sends the dbus signals to
enable/disable the features.

> over main.conf, or are there legitimate reasons to not have the
> controller generating these events all the time?

Another reason not to generate the events all the time is to avoid the
overhead on the BT controller and the stack if the feature is not
required.

>
> > --
> > 2.32.0.288.g62a8d224e6-goog
> >
>
>
> --
> Luiz Augusto von Dentz



-- 

Joseph Shyh-In Hwang
Email: josephsih@google.com

RE: [BlueZ,v2,1/3] monitor: add new Intel extended telemetry events

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1953 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=504265

---Test result---

Test Summary:
CheckPatch                    PASS      1.48 seconds
GitLint                       PASS      0.38 seconds
Prep - Setup ELL              PASS      47.50 seconds
Build - Prep                  PASS      0.11 seconds
Build - Configure             PASS      8.27 seconds
Build - Make                  PASS      201.30 seconds
Make Check                    PASS      8.72 seconds
Make Distcheck                PASS      235.49 seconds
Build w/ext ELL - Configure   PASS      7.99 seconds
Build w/ext ELL - Make        PASS      187.68 seconds

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

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

##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL

##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build

##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree

##############################
Test: Build - Make - PASS
Desc: Build the BlueZ source tree

##############################
Test: Make Check - PASS
Desc: Run 'make check'

##############################
Test: Make Distcheck - PASS
Desc: Run distcheck to check the distribution

##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration

##############################
Test: Build w/ext ELL - Make - PASS
Desc: Build BlueZ source with '--enable-external-ell' configuration



---
Regards,
Linux Bluetooth

Re: [BlueZ PATCH v2 1/3] monitor: add new Intel extended telemetry events

[Luiz Augusto von Dentz]

Hi Joseph,

On Mon, Jun 21, 2021 at 3:23 AM Joseph Hwang <josephsih@chromium.org> wrote:
>
> This patch adds new Intel extended telemetry events for both ACL and
> SCO/eSCO audio link quality reports.
>
> For SCO/eSCO audio link quality report, it shows something like
> > HCI Event: Vendor (0xff) plen 190  #120 [hci0] 2021-05-31 20:27:50.257
>       Intel Extended Telemetry (0x87)
>         Extended Telemetry (0x80): SubOpcode (0x03)
>         Extended event type (0x01): Audio Link Quality Report Type(0x05)
>         SCO/eSCO connection handle (0x6a): 0x0101
>         Packets from host (0x6b): 399
>         Tx packets (0x6c): 403
>         Rx payload lost (0x6d): 3
>         Tx payload lost (0x6e): 0
>         Rx No SYNC errors (0x6f): 3 2 3 3 0
>         Rx HEC errors (0x70): 0 0 0 0 0
>         Rx CRC errors (0x71): 2 0 0 0 0
>         Rx NAK errors (0x72): 6 0 0 0 0
>         Failed Tx due to Wifi coex (0x73): 6 0 0 0 0
>         Failed Rx due to Wifi coex (0x74): 0 0 0 0 0
>         Late samples inserted based on CDC (0x75): 0
>         Samples dropped (0x76): 0
>         Mute samples sent at initial connection (0x77): 0
>         PLC injection data (0x78): 0
>
> For ACL audio link quality report, it shows something like
> > HCI Event: Vendor (0xff) plen 142  #120 [hci0] 2021-05-31 20:27:50.261
>       Intel Extended Telemetry (0x87)
>         Extended Telemetry (0x80): SubOpcode (0x03)
>         Extended event type (0x01): Audio Link Quality Report Type(0x05)
>         ACL connection handle (0x4a): 0x0100
>         Rx HEC errors (0x4b): 0
>         Rx CRC errors (0x4c): 0
>         Packets from host (0x4d): 100
>         Tx packets (0x4e): 101
>         Tx packets 0 retries (0x4f): 89
>         Tx packets 1 retries (0x50): 11
>         Tx packets 2 retries (0x51): 1
>         Tx packets 3 retries (0x52): 0
>         Tx packets 4 retries and more (0x53): 0
>         Tx DH1 packets (0x54): 0
>         Tx DH3 packets (0x55): 0
>         Tx DH5 packets (0x56): 0
>         Tx 2DH1 packets (0x57): 0
>         Tx 2DH3 packets (0x58): 0
>         Tx 2DH5 packets (0x59): 0
>         Tx 3DH1 packets (0x5a): 6
>         Tx 3DH3 packets (0x5b): 0
>         Tx 3DH5 packets (0x5c): 94
>         Rx packets (0x5d): 272
>         ACL link throughput (KBps) (0x5e): 343815
>         ACL max packet latency (ms) (0x5f): 20625
>         ACL avg packet latency (ms) (0x60): 12
>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
>
> Changes in v2:
> - Perform size checks for tlv subevents.
> - Fix the Make errors about qualifiers.
>
>  monitor/intel.c  | 472 +++++++++++++++++++++++++++++++++++++++++++++++
>  monitor/vendor.h |   6 +
>  2 files changed, 478 insertions(+)
>
> diff --git a/monitor/intel.c b/monitor/intel.c
> index d2aefa6a8..af7402db4 100644
> --- a/monitor/intel.c
> +++ b/monitor/intel.c
> @@ -30,6 +30,7 @@
>
>  #define COLOR_UNKNOWN_EVENT_MASK       COLOR_WHITE_BG
>  #define COLOR_UNKNOWN_SCAN_STATUS      COLOR_WHITE_BG
> +#define COLOR_UNKNOWN_EXT_EVENT                COLOR_WHITE_BG
>
>  static void print_status(uint8_t status)
>  {
> @@ -958,6 +959,8 @@ static void system_exception_evt(const void *data, uint8_t size)
>         packet_hexdump(data + 1, size - 1);
>  }
>
> +static void intel_vendor_ext_evt(const void *data, uint8_t size);
> +
>  static const struct vendor_evt vendor_evt_table[] = {
>         { 0x00, "Startup",
>                         startup_evt, 0, true },
> @@ -989,6 +992,7 @@ static const struct vendor_evt vendor_evt_table[] = {
>                         system_exception_evt, 133, true },
>         { 0x2c, "FW Trace String" },
>         { 0x2e, "FW Trace Binary" },
> +       { 0x87, "Extended Telemetry", intel_vendor_ext_evt },
>         { }
>  };
>
> @@ -1003,3 +1007,471 @@ const struct vendor_evt *intel_vendor_evt(uint8_t evt)
>
>         return NULL;
>  }
> +
> +static void ext_evt_type(uint8_t subevent_id, const void *data, uint8_t size)
> +{
> +       uint8_t evt_type = get_u8(data);
> +       const char *str;
> +
> +       switch (evt_type) {
> +       case 0x00:
> +               str = "System Exception";
> +               break;
> +       case 0x01:
> +               str = "Fatal Exception";
> +               break;
> +       case 0x02:
> +               str = "Debug Exception";
> +               break;
> +       case 0x03:
> +               str = "Connection Event for BR/EDR Link Type";
> +               break;
> +       case 0x04:
> +               str = "Disconnection Event";
> +               break;
> +       case 0x05:
> +               str = "Audio Link Quality Report Type";
> +               break;
> +       case 0x06:
> +               str = "Stats for BR/EDR Link Type";
> +               break;
> +       default:
> +               print_text(COLOR_UNKNOWN_EXT_EVENT,
> +                       "Unknown extended telemetry event type (0x%2.2x)",
> +                       evt_type);
> +               packet_hexdump(data, size);
> +               return;
> +       }
> +
> +       print_field("Extended event type (0x%2.2x): %s (0x%2.2x)",
> +                       subevent_id, str, evt_type);
> +}
> +
> +static void ext_acl_evt_conn_handle(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       uint16_t conn_handle = get_le16(data);
> +
> +       print_field("ACL connection handle (0x%2.2x): 0x%4.4x",
> +                       subevent_id, conn_handle);
> +}
> +
> +static void ext_acl_evt_hec_errors(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Rx HEC errors (0x%2.2x): %d", subevent_id, num);
> +}
> +
> +static void ext_acl_evt_crc_errors(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Rx CRC errors (0x%2.2x): %d", subevent_id, num);
> +}
> +
> +static void ext_acl_evt_num_pkt_from_host(uint8_t subevent_id, const void *data,
> +                                               uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Packets from host (0x%2.2x): %d",
> +                       subevent_id, num);
> +}
> +
> +static void ext_acl_evt_num_tx_pkt_to_air(uint8_t subevent_id, const void *data,
> +                                               uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Tx packets (0x%2.2x): %d", subevent_id, num);
> +}
> +
> +static void ext_acl_evt_num_tx_pkt_retry(uint8_t subevent_id, const void *data,
> +                                               uint8_t size)
> +{
> +       char *subevent_str;
> +       uint32_t num = get_le32(data);
> +
> +       switch (subevent_id) {
> +       case 0x4f:
> +               subevent_str = "Tx packets 0 retries";
> +               break;
> +       case 0x50:
> +               subevent_str = "Tx packets 1 retries";
> +               break;
> +       case 0x51:
> +               subevent_str = "Tx packets 2 retries";
> +               break;
> +       case 0x52:
> +               subevent_str = "Tx packets 3 retries";
> +               break;
> +       case 0x53:
> +               subevent_str = "Tx packets 4 retries and more";
> +               break;
> +       default:
> +               subevent_str = "Unknown";
> +               break;
> +       }
> +
> +       print_field("%s (0x%2.2x): %d", subevent_str, subevent_id, num);
> +}
> +
> +static void ext_acl_evt_num_tx_pkt_type(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       char *packet_type_str;
> +       uint32_t num = get_le32(data);
> +
> +       switch (subevent_id) {
> +       case 0x54:
> +               packet_type_str = "DH1";
> +               break;
> +       case 0x55:
> +               packet_type_str = "DH3";
> +               break;
> +       case 0x56:
> +               packet_type_str = "DH5";
> +               break;
> +       case 0x57:
> +               packet_type_str = "2DH1";
> +               break;
> +       case 0x58:
> +               packet_type_str = "2DH3";
> +               break;
> +       case 0x59:
> +               packet_type_str = "2DH5";
> +               break;
> +       case 0x5a:
> +               packet_type_str = "3DH1";
> +               break;
> +       case 0x5b:
> +               packet_type_str = "3DH3";
> +               break;
> +       case 0x5c:
> +               packet_type_str = "3DH5";
> +               break;
> +       default:
> +               packet_type_str = "Unknown";
> +               break;
> +       }
> +
> +       print_field("Tx %s packets (0x%2.2x): %d",
> +                       packet_type_str, subevent_id, num);
> +}
> +
> +static void ext_acl_evt_num_rx_pkt_from_air(uint8_t subevent_id,
> +                                               const void *data, uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Rx packets (0x%2.2x): %d",
> +                       subevent_id, num);
> +}
> +
> +static void ext_acl_evt_link_throughput(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("ACL link throughput (KBps) (0x%2.2x): %d",
> +                       subevent_id, num);
> +}
> +
> +static void ext_acl_evt_max_packet_latency(uint8_t subevent_id,
> +                                               const void *data, uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("ACL max packet latency (ms) (0x%2.2x): %d",
> +                       subevent_id, num);
> +}
> +
> +static void ext_acl_evt_avg_packet_latency(uint8_t subevent_id,
> +                                               const void *data, uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("ACL avg packet latency (ms) (0x%2.2x): %d",
> +                       subevent_id, num);
> +}
> +
> +static void ext_sco_evt_conn_handle(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       uint16_t conn_handle = get_le16(data);
> +
> +       print_field("SCO/eSCO connection handle (0x%2.2x): 0x%4.4x",
> +                       subevent_id, conn_handle);
> +}
> +
> +static void ext_sco_evt_num_rx_pkt_from_air(uint8_t subevent_id,
> +                                               const void *data, uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Packets from host (0x%2.2x): %d", subevent_id, num);
> +}
> +
> +static void ext_sco_evt_num_tx_pkt_to_air(uint8_t subevent_id, const void *data,
> +                                               uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Tx packets (0x%2.2x): %d", subevent_id, num);
> +}
> +
> +static void ext_sco_evt_num_rx_payloads_lost(uint8_t subevent_id,
> +                                               const void *data, uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Rx payload lost (0x%2.2x): %d", subevent_id, num);
> +}
> +
> +static void ext_sco_evt_num_tx_payloads_lost(uint8_t subevent_id,
> +                                               const void *data, uint8_t size)
> +{
> +
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Tx payload lost (0x%2.2x): %d", subevent_id, num);
> +}
> +
> +static void slots_errors(const char *type_str, uint8_t subevent_id,
> +                               const void *data, uint8_t size)
> +{
> +       /* The subevent has 5 slots where each slot is of the uint32_t type. */
> +       uint32_t num[5];
> +       int i;
> +
> +       if (size != 5 * sizeof(uint32_t)) {
> +               print_text(COLOR_UNKNOWN_EXT_EVENT,
> +                               "  Invalid subevent length (%d)", size);
> +               return;
> +       }
> +
> +       for (i = 0; i < 5; i++) {
> +               num[i] = get_le32(data);
> +               data = (const void *)((const uint8_t *) data +
> +                                               sizeof(uint32_t));
> +       }
> +
> +       print_field("%s (0x%2.2x): %d %d %d %d %d", type_str, subevent_id,
> +                       num[0], num[1], num[2], num[3], num[4]);
> +}
> +
> +static void ext_sco_evt_num_no_sync_errors(uint8_t subevent_id,
> +                                               const void *data, uint8_t size)
> +{
> +       slots_errors("Rx No SYNC errors", subevent_id, data, size);
> +}
> +
> +static void ext_sco_evt_num_hec_errors(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       slots_errors("Rx HEC errors", subevent_id, data, size);
> +}
> +
> +static void ext_sco_evt_num_crc_errors(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       slots_errors("Rx CRC errors", subevent_id, data, size);
> +}
> +
> +static void ext_sco_evt_num_naks(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       slots_errors("Rx NAK errors", subevent_id, data, size);
> +}
> +
> +static void ext_sco_evt_num_failed_tx_by_wifi(uint8_t subevent_id,
> +                                               const void *data, uint8_t size)
> +{
> +       slots_errors("Failed Tx due to Wifi coex", subevent_id, data, size);
> +}
> +
> +static void ext_sco_evt_num_failed_rx_by_wifi(uint8_t subevent_id,
> +                                               const void *data, uint8_t size)
> +{
> +       slots_errors("Failed Rx due to Wifi coex", subevent_id, data, size);
> +}
> +
> +static void ext_sco_evt_samples_inserted(uint8_t subevent_id, const void *data,
> +                                               uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Late samples inserted based on CDC (0x%2.2x): %d",
> +                       subevent_id, num);
> +}
> +
> +static void ext_sco_evt_samples_dropped(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Samples dropped (0x%2.2x): %d", subevent_id, num);
> +}
> +
> +static void ext_sco_evt_mute_samples(uint8_t subevent_id, const void *data,
> +                                       uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("Mute samples sent at initial connection (0x%2.2x): %d",
> +                       subevent_id, num);
> +}
> +
> +static void ext_sco_evt_plc_injection_data(uint8_t subevent_id,
> +                                               const void *data, uint8_t size)
> +{
> +       uint32_t num = get_le32(data);
> +
> +       print_field("PLC injection data (0x%2.2x): %d", subevent_id, num);
> +}
> +
> +
> +static const struct vendor_ext_subevent vendor_ext_subevent_table[] = {
> +       { 0x01, 1, ext_evt_type },
> +
> +       /* ACL audio link quality subevents */
> +       { 0x4a, 2, ext_acl_evt_conn_handle },
> +       { 0x4b, 4, ext_acl_evt_hec_errors },
> +       { 0x4c, 4, ext_acl_evt_crc_errors },
> +       { 0x4d, 4, ext_acl_evt_num_pkt_from_host },
> +       { 0x4e, 4, ext_acl_evt_num_tx_pkt_to_air },
> +       { 0x4f, 4, ext_acl_evt_num_tx_pkt_retry },
> +       { 0x50, 4, ext_acl_evt_num_tx_pkt_retry },
> +       { 0x51, 4, ext_acl_evt_num_tx_pkt_retry },
> +       { 0x52, 4, ext_acl_evt_num_tx_pkt_retry },
> +       { 0x53, 4, ext_acl_evt_num_tx_pkt_retry },
> +       { 0x54, 4, ext_acl_evt_num_tx_pkt_type },
> +       { 0x55, 4, ext_acl_evt_num_tx_pkt_type },
> +       { 0x56, 4, ext_acl_evt_num_tx_pkt_type },
> +       { 0x57, 4, ext_acl_evt_num_tx_pkt_type },
> +       { 0x58, 4, ext_acl_evt_num_tx_pkt_type },
> +       { 0x59, 4, ext_acl_evt_num_tx_pkt_type },
> +       { 0x5a, 4, ext_acl_evt_num_tx_pkt_type },
> +       { 0x5b, 4, ext_acl_evt_num_tx_pkt_type },
> +       { 0x5c, 4, ext_acl_evt_num_tx_pkt_type },
> +       { 0x5d, 4, ext_acl_evt_num_rx_pkt_from_air },
> +       { 0x5e, 4, ext_acl_evt_link_throughput },
> +       { 0x5f, 4, ext_acl_evt_max_packet_latency },
> +       { 0x60, 4, ext_acl_evt_avg_packet_latency },
> +
> +       /* SCO/eSCO audio link quality subevents */
> +       { 0x6a, 2, ext_sco_evt_conn_handle },
> +       { 0x6b, 4, ext_sco_evt_num_rx_pkt_from_air },
> +       { 0x6c, 4, ext_sco_evt_num_tx_pkt_to_air },
> +       { 0x6d, 4, ext_sco_evt_num_rx_payloads_lost },
> +       { 0x6e, 4, ext_sco_evt_num_tx_payloads_lost },
> +       { 0x6f, 20, ext_sco_evt_num_no_sync_errors },
> +       { 0x70, 20, ext_sco_evt_num_hec_errors },
> +       { 0x71, 20, ext_sco_evt_num_crc_errors },
> +       { 0x72, 20, ext_sco_evt_num_naks },
> +       { 0x73, 20, ext_sco_evt_num_failed_tx_by_wifi },
> +       { 0x74, 20, ext_sco_evt_num_failed_rx_by_wifi },
> +       { 0x75, 4, ext_sco_evt_samples_inserted },
> +       { 0x76, 4, ext_sco_evt_samples_dropped },
> +       { 0x77, 4, ext_sco_evt_mute_samples },
> +       { 0x78, 4, ext_sco_evt_plc_injection_data },
> +
> +       /* end */
> +       { 0x0, 0}
> +};
> +
> +static const uint8_t *process_ext_subevent(const uint8_t *data,
> +                                       const uint8_t *event_end)
> +{
> +       /*
> +        * A subevent is of the TLV format.
> +        * - Type: takes 1 byte. This is the subevent_id.
> +        * - Length: takes 1 byte.
> +        * - Value: takes |Length| bytes.
> +        */
> +       uint8_t subevent_id = *data;
> +       uint8_t length = *(data + sizeof(uint8_t));
> +       const uint8_t *next_tlv = (const uint8_t *) data + 2 * sizeof(uint8_t) +
> +                                       length;

It might be a good idea to have a packed struct that has these fields
instead of doing this pointer arithmetic manually.

> +       const struct vendor_ext_subevent *subevent = NULL;
> +       int i;
> +
> +       if (next_tlv > event_end) {
> +               print_text(COLOR_ERROR, "Subevent exceeds the buffer size.");
> +               return NULL;
> +       }

It might be better to move this check after processing the current tlv
entry, so in case there is an error while processing we know exactly
at which point it failed.

> +       for (i = 0; vendor_ext_subevent_table[i].length > 0; i++) {
> +               if (vendor_ext_subevent_table[i].subevent_id == subevent_id) {
> +                       subevent = &vendor_ext_subevent_table[i];
> +                       break;
> +               }
> +       }
> +
> +       if (!subevent) {
> +               print_text(COLOR_UNKNOWN_EXT_EVENT,
> +                               "Unknown extended event type id (0x%2.2x)",
> +                               subevent_id);
> +               return NULL;
> +       }
> +
> +       if (length != subevent->length) {
> +               print_text(COLOR_ERROR, "Invalid subevent length");
> +               return NULL;
> +       }
> +
> +       subevent->func(subevent_id, data + 2 * sizeof(uint8_t), length);
> +
> +       return next_tlv;
> +}
> +
> +static void intel_vendor_ext_evt(const void *data, uint8_t size)
> +{
> +       const uint8_t *tlv = (const uint8_t *) data;
> +       uint8_t ext_telemetry_type = *tlv;
> +       uint8_t sub_opcode = *(tlv + sizeof(uint8_t));
> +       const uint8_t *event_end = (const uint8_t *) data + size;

Same thing as above, use a packed struct to represent the tlv entries.

> +
> +       /*
> +        * Intel extended telemetry event is of the format:
> +        *   Event Code: 0xff
> +        *   Length:     0xnn
> +        *   Event Type: 0x8780
> +        *   SubOpcode:  0x03
> +        *   A number of TLV data
> +        *
> +        *   Each TLV Data, i.e., a subevent, consists of
> +        *     Subevent Id
> +        *     Length
> +        *     Value
> +        *
> +        * Arrives here through the telemetry event (0x87) in vendor_evt_table.
> +        * It is required to further check the extended telemetry type (0x80)
> +        * and sub-opcode (0x03) in the tlv data.
> +        */
> +       if (ext_telemetry_type == 0x80 && sub_opcode == 0x03) {
> +               /* Consumed two octets, i.e., 0x80 and 0x03. */

I'd probably add a table for these as well, so in the future if we
need to add handling of other subevents we just add another opcode and
callback to handle it.

> +               print_field("Extended Telemetry (0x%2.2x): SubOpcode (0x%2.2x)",
> +                               ext_telemetry_type, sub_opcode);
> +               tlv += 2 * sizeof(uint8_t);
> +       } else {
> +               print_text(COLOR_UNKNOWN_EXT_EVENT,
> +                       "  Unknown extended telemetry event (0x%2.2x 0x%2.2x)",
> +                       ext_telemetry_type, sub_opcode);
> +               packet_hexdump(data, size);
> +               return;
> +       }
> +
> +       /* Process every TLV subevent data.
> +        * The decoding process normally terminates when tlv == event_end.
> +        */
> +       while (tlv && tlv < event_end)
> +               tlv = process_ext_subevent(tlv, event_end);
> +
> +       /* If an error occrus in decoding the subevents, hexdump the packet. */

There is a typo above, it shall be: occurs.

> +       if (!tlv)
> +               packet_hexdump(data, size);
> +}
> diff --git a/monitor/vendor.h b/monitor/vendor.h
> index c70552b66..347b4bfe9 100644
> --- a/monitor/vendor.h
> +++ b/monitor/vendor.h
> @@ -30,4 +30,10 @@ struct vendor_evt {
>         bool evt_fixed;
>  };
>
> +struct vendor_ext_subevent {
> +       uint8_t subevent_id;
> +       uint8_t length;
> +       void (*func)(uint8_t subevent_id, const void *data, uint8_t size);
> +};
> +
>  void vendor_event(uint16_t manufacturer, const void *data, uint8_t size);
> --
> 2.32.0.288.g62a8d224e6-goog
>


-- 
Luiz Augusto von Dentz