[PATCH v1 1/2] Bluetooth: aosp: surface AOSP quality report through mgmt

[PATCH v1 1/2] Bluetooth: aosp: surface AOSP quality report through mgmt

[Joseph Hwang]

When receiving a HCI vendor event, the kernel checks if it is an
AOSP bluetooth quality report. If yes, the event is sent to bluez
user space through the mgmt socket.

Signed-off-by: Joseph Hwang <josephsih@chromium.org>
Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---

 include/net/bluetooth/hci_core.h |  2 ++
 include/net/bluetooth/mgmt.h     |  7 ++++
 net/bluetooth/aosp.c             | 61 ++++++++++++++++++++++++++++++++
 net/bluetooth/aosp.h             | 12 +++++++
 net/bluetooth/hci_event.c        | 33 ++++++++++++++++-
 net/bluetooth/mgmt.c             | 22 ++++++++++++
 6 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 21eadb113a31..727cb9c056b2 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1861,6 +1861,8 @@ int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
 int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
 void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
 				  bdaddr_t *bdaddr, u8 addr_type);
+int mgmt_quality_report(struct hci_dev *hdev, struct sk_buff *skb,
+			u8 quality_spec);
 
 u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
 		      u16 to_multiplier);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 99266f7aebdc..6a0fcb3aef8a 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -1120,3 +1120,10 @@ struct mgmt_ev_adv_monitor_device_lost {
 	__le16 monitor_handle;
 	struct mgmt_addr_info addr;
 } __packed;
+
+#define MGMT_EV_QUALITY_REPORT			0x0031
+struct mgmt_ev_quality_report {
+	__u8 quality_spec;
+	__u8 data_len;
+	__u8 data[0];
+} __packed;
diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
index 432ae3aac9e3..9e3551627ad5 100644
--- a/net/bluetooth/aosp.c
+++ b/net/bluetooth/aosp.c
@@ -199,3 +199,64 @@ int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
 	else
 		return disable_quality_report(hdev);
 }
+
+#define BLUETOOTH_QUALITY_REPORT_EV		0x58
+struct bqr_data {
+	__u8 quality_report_id;
+	__u8 packet_type;
+	__le16 conn_handle;
+	__u8 conn_role;
+	__s8 tx_power_level;
+	__s8 rssi;
+	__u8 snr;
+	__u8 unused_afh_channel_count;
+	__u8 afh_select_unideal_channel_count;
+	__le16 lsto;
+	__le32 conn_piconet_clock;
+	__le32 retransmission_count;
+	__le32 no_rx_count;
+	__le32 nak_count;
+	__le32 last_tx_ack_timestamp;
+	__le32 flow_off_count;
+	__le32 last_flow_on_timestamp;
+	__le32 buffer_overflow_bytes;
+	__le32 buffer_underflow_bytes;
+
+	/* Vendor Specific Parameter */
+	__u8 vsp[0];
+} __packed;
+
+struct aosp_hci_vs_data {
+	__u8 code;
+	__u8 data[0];
+} __packed;
+
+bool aosp_is_quality_report_evt(struct sk_buff *skb)
+{
+	struct aosp_hci_vs_data *ev;
+
+	if (skb->len < sizeof(struct aosp_hci_vs_data))
+		return false;
+
+	ev = (struct aosp_hci_vs_data *)skb->data;
+
+	return ev->code == BLUETOOTH_QUALITY_REPORT_EV;
+}
+
+bool aosp_pull_quality_report_data(struct sk_buff *skb)
+{
+	size_t bqr_data_len = sizeof(struct bqr_data);
+
+	skb_pull(skb, sizeof(struct aosp_hci_vs_data));
+
+	/* skb->len is allowed to be larger than bqr_data_len to have
+	 * the Vendor Specific Parameter (vsp) field.
+	 */
+	if (skb->len < bqr_data_len) {
+		BT_ERR("AOSP evt data len %d too short (%u expected)",
+		       skb->len, bqr_data_len);
+		return false;
+	}
+
+	return true;
+}
diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
index 2fd8886d51b2..49894a995647 100644
--- a/net/bluetooth/aosp.h
+++ b/net/bluetooth/aosp.h
@@ -10,6 +10,8 @@ void aosp_do_close(struct hci_dev *hdev);
 
 bool aosp_has_quality_report(struct hci_dev *hdev);
 int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
+bool aosp_is_quality_report_evt(struct sk_buff *skb);
+bool aosp_pull_quality_report_data(struct sk_buff *skb);
 
 #else
 
@@ -26,4 +28,14 @@ static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
 	return -EOPNOTSUPP;
 }
 
+static inline bool aosp_is_quality_report_evt(struct sk_buff *skb)
+{
+	return false;
+}
+
+static inline bool aosp_pull_quality_report_data(struct sk_buff *skb)
+{
+	return false;
+}
+
 #endif
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 681c623aa380..bccb659a9454 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -37,6 +37,7 @@
 #include "smp.h"
 #include "msft.h"
 #include "eir.h"
+#include "aosp.h"
 
 #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
 		 "\x00\x00\x00\x00\x00\x00\x00\x00"
@@ -4225,6 +4226,36 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
 	queue_work(hdev->workqueue, &hdev->tx_work);
 }
 
+#define QUALITY_SPEC_NA			0x0
+#define QUALITY_SPEC_INTEL_TELEMETRY	0x1
+#define QUALITY_SPEC_AOSP_BQR		0x2
+
+static bool quality_report_evt(struct hci_dev *hdev,  void *data,
+			       struct sk_buff *skb)
+{
+	if (aosp_is_quality_report_evt(skb)) {
+		if (aosp_has_quality_report(hdev) &&
+		    aosp_pull_quality_report_data(skb))
+			mgmt_quality_report(hdev, skb, QUALITY_SPEC_AOSP_BQR);
+
+		return true;
+	}
+
+	return false;
+}
+
+static void hci_vendor_evt(struct hci_dev *hdev, void *data,
+			   struct sk_buff *skb)
+{
+	/* Every distinct vendor specification must have a well-defined
+	 * condition to determine if an event meets the specification.
+	 * The skb is consumed by a specification only if the event meets
+	 * the specification.
+	 */
+	if (!quality_report_evt(hdev, data, skb))
+		msft_vendor_evt(hdev, data, skb);
+}
+
 static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
 				struct sk_buff *skb)
 {
@@ -6811,7 +6842,7 @@ static const struct hci_ev {
 	HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
 	       sizeof(struct hci_ev_num_comp_blocks)),
 	/* [0xff = HCI_EV_VENDOR] */
-	HCI_EV(HCI_EV_VENDOR, msft_vendor_evt, 0),
+	HCI_EV(HCI_EV_VENDOR, hci_vendor_evt, 0),
 };
 
 static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 08d6494f1b34..78687ae885be 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4389,6 +4389,28 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
 			       MGMT_STATUS_NOT_SUPPORTED);
 }
 
+int mgmt_quality_report(struct hci_dev *hdev, struct sk_buff *skb,
+			u8 quality_spec)
+{
+	struct mgmt_ev_quality_report *ev;
+	size_t ev_len;
+	int err;
+
+	/* The ev comes with a variable-length data field. */
+	ev_len = sizeof(*ev) + skb->len;
+	ev = kmalloc(ev_len, GFP_KERNEL);
+	if (!ev)
+		return -ENOMEM;
+
+	ev->quality_spec = quality_spec;
+	ev->data_len = skb->len;
+	memcpy(ev->data, skb->data, skb->len);
+	err = mgmt_event(MGMT_EV_QUALITY_REPORT, hdev, ev, ev_len, NULL);
+	kfree(ev);
+
+	return err;
+}
+
 static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 			    u16 data_len)
 {
-- 
2.35.0.rc0.227.g00780c9af4-goog

[PATCH v1 2/2] Bluetooth: btintel: surface Intel telemetry events through mgmt

[Joseph Hwang]

When receiving a HCI vendor event, the kernel checks if it is an
Intel telemetry event. If yes, the event is sent to bluez user
space through the mgmt socket.

Signed-off-by: Joseph Hwang <josephsih@chromium.org>
Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---

 drivers/bluetooth/btintel.c      | 43 +++++++++++++++++++++++++++++++-
 drivers/bluetooth/btintel.h      | 12 +++++++++
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_event.c        | 12 ++++++---
 4 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 1a4f8b227eac..d3b7a796cb91 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -2401,8 +2401,10 @@ static int btintel_setup_combined(struct hci_dev *hdev)
 	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 	set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
 
-	/* Set up the quality report callback for Intel devices */
+	/* Set up the quality report callbacks for Intel devices */
 	hdev->set_quality_report = btintel_set_quality_report;
+	hdev->is_quality_report_evt = btintel_is_quality_report_evt;
+	hdev->pull_quality_report_data = btintel_pull_quality_report_data;
 
 	/* For Legacy device, check the HW platform value and size */
 	if (skb->len == sizeof(ver) && skb->data[1] == 0x37) {
@@ -2645,6 +2647,45 @@ void btintel_secure_send_result(struct hci_dev *hdev,
 }
 EXPORT_SYMBOL_GPL(btintel_secure_send_result);
 
+#define INTEL_PREFIX		0x8087
+#define TELEMETRY_CODE		0x03
+
+struct intel_prefix_evt_data {
+	__le16 vendor_prefix;
+	__u8 code;
+	__u8 data[0];   /* a number of struct intel_tlv subevents */
+} __packed;
+
+bool btintel_is_quality_report_evt(struct sk_buff *skb)
+{
+	struct intel_prefix_evt_data *ev;
+	u16 vendor_prefix;
+
+	if (skb->len < sizeof(struct intel_prefix_evt_data))
+		return false;
+
+	ev = (struct intel_prefix_evt_data *)skb->data;
+	vendor_prefix = __le16_to_cpu(ev->vendor_prefix);
+
+	return vendor_prefix == INTEL_PREFIX && ev->code == TELEMETRY_CODE;
+}
+EXPORT_SYMBOL_GPL(btintel_is_quality_report_evt);
+
+bool btintel_pull_quality_report_data(struct sk_buff *skb)
+{
+	skb_pull(skb, sizeof(struct intel_prefix_evt_data));
+
+	/* A telemetry event contains at least one intel_tlv subevent. */
+	if (skb->len < sizeof(struct intel_tlv)) {
+		BT_ERR("Telemetry event length %d too short (at least %u)",
+		       skb->len, sizeof(struct intel_tlv));
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(btintel_pull_quality_report_data);
+
 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 c9b24e9299e2..841aef3dbd4c 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -210,6 +210,8 @@ void btintel_bootup(struct hci_dev *hdev, const void *ptr, unsigned int len);
 void btintel_secure_send_result(struct hci_dev *hdev,
 				const void *ptr, unsigned int len);
 int btintel_set_quality_report(struct hci_dev *hdev, bool enable);
+bool btintel_is_quality_report_evt(struct sk_buff *skb);
+bool btintel_pull_quality_report_data(struct sk_buff *skb);
 #else
 
 static inline int btintel_check_bdaddr(struct hci_dev *hdev)
@@ -305,4 +307,14 @@ static inline int btintel_set_quality_report(struct hci_dev *hdev, bool enable)
 {
 	return -ENODEV;
 }
+
+static inline bool btintel_is_quality_report_evt(struct sk_buff *skb)
+{
+	return false;
+}
+
+static inline bool btintel_pull_quality_report_data(struct sk_buff *skb);
+{
+	return false;
+}
 #endif
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 727cb9c056b2..b74ba1585df9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -632,6 +632,8 @@ struct hci_dev {
 	void (*cmd_timeout)(struct hci_dev *hdev);
 	bool (*wakeup)(struct hci_dev *hdev);
 	int (*set_quality_report)(struct hci_dev *hdev, bool enable);
+	bool (*is_quality_report_evt)(struct sk_buff *skb);
+	bool (*pull_quality_report_data)(struct sk_buff *skb);
 	int (*get_data_path_id)(struct hci_dev *hdev, __u8 *data_path);
 	int (*get_codec_config_data)(struct hci_dev *hdev, __u8 type,
 				     struct bt_codec *codec, __u8 *vnd_len,
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index bccb659a9454..5f9cc7b942a1 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4237,11 +4237,17 @@ static bool quality_report_evt(struct hci_dev *hdev,  void *data,
 		if (aosp_has_quality_report(hdev) &&
 		    aosp_pull_quality_report_data(skb))
 			mgmt_quality_report(hdev, skb, QUALITY_SPEC_AOSP_BQR);
-
-		return true;
+	} else if (hdev->is_quality_report_evt &&
+		   hdev->is_quality_report_evt(skb)) {
+		if (hdev->set_quality_report &&
+		    hdev->pull_quality_report_data(skb))
+			mgmt_quality_report(hdev, skb,
+					    QUALITY_SPEC_INTEL_TELEMETRY);
+	} else {
+		return false;
 	}
 
-	return false;
+	return true;
 }
 
 static void hci_vendor_evt(struct hci_dev *hdev, void *data,
-- 
2.35.0.rc0.227.g00780c9af4-goog

Re: [PATCH v1 1/2] Bluetooth: aosp: surface AOSP quality report through mgmt

[Marcel Holtmann]

Hi Joseph,

> When receiving a HCI vendor event, the kernel checks if it is an
> AOSP bluetooth quality report. If yes, the event is sent to bluez
> user space through the mgmt socket.
> 
> Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
> 
> include/net/bluetooth/hci_core.h |  2 ++
> include/net/bluetooth/mgmt.h     |  7 ++++
> net/bluetooth/aosp.c             | 61 ++++++++++++++++++++++++++++++++
> net/bluetooth/aosp.h             | 12 +++++++
> net/bluetooth/hci_event.c        | 33 ++++++++++++++++-
> net/bluetooth/mgmt.c             | 22 ++++++++++++
> 6 files changed, 136 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 21eadb113a31..727cb9c056b2 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1861,6 +1861,8 @@ int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
> int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> 				  bdaddr_t *bdaddr, u8 addr_type);
> +int mgmt_quality_report(struct hci_dev *hdev, struct sk_buff *skb,
> +			u8 quality_spec);
> 
> u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> 		      u16 to_multiplier);
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 99266f7aebdc..6a0fcb3aef8a 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -1120,3 +1120,10 @@ struct mgmt_ev_adv_monitor_device_lost {
> 	__le16 monitor_handle;
> 	struct mgmt_addr_info addr;
> } __packed;
> +
> +#define MGMT_EV_QUALITY_REPORT			0x0031
> +struct mgmt_ev_quality_report {
> +	__u8 quality_spec;
> +	__u8 data_len;
> +	__u8 data[0];
> +} __packed;
> diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> index 432ae3aac9e3..9e3551627ad5 100644
> --- a/net/bluetooth/aosp.c
> +++ b/net/bluetooth/aosp.c
> @@ -199,3 +199,64 @@ int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> 	else
> 		return disable_quality_report(hdev);
> }
> +
> +#define BLUETOOTH_QUALITY_REPORT_EV		0x58
> +struct bqr_data {
> +	__u8 quality_report_id;
> +	__u8 packet_type;
> +	__le16 conn_handle;
> +	__u8 conn_role;
> +	__s8 tx_power_level;
> +	__s8 rssi;
> +	__u8 snr;
> +	__u8 unused_afh_channel_count;
> +	__u8 afh_select_unideal_channel_count;
> +	__le16 lsto;
> +	__le32 conn_piconet_clock;
> +	__le32 retransmission_count;
> +	__le32 no_rx_count;
> +	__le32 nak_count;
> +	__le32 last_tx_ack_timestamp;
> +	__le32 flow_off_count;
> +	__le32 last_flow_on_timestamp;
> +	__le32 buffer_overflow_bytes;
> +	__le32 buffer_underflow_bytes;
> +
> +	/* Vendor Specific Parameter */
> +	__u8 vsp[0];
> +} __packed;
> +
> +struct aosp_hci_vs_data {
> +	__u8 code;
> +	__u8 data[0];
> +} __packed;

unless you need these two for something, scrap them. You can define constants for the size check.

> +
> +bool aosp_is_quality_report_evt(struct sk_buff *skb)
> +{
> +	struct aosp_hci_vs_data *ev;
> +
> +	if (skb->len < sizeof(struct aosp_hci_vs_data))
> +		return false;
> +
> +	ev = (struct aosp_hci_vs_data *)skb->data;
> +
> +	return ev->code == BLUETOOTH_QUALITY_REPORT_EV;
> +}
> +
> +bool aosp_pull_quality_report_data(struct sk_buff *skb)
> +{
> +	size_t bqr_data_len = sizeof(struct bqr_data);
> +
> +	skb_pull(skb, sizeof(struct aosp_hci_vs_data));
> +
> +	/* skb->len is allowed to be larger than bqr_data_len to have
> +	 * the Vendor Specific Parameter (vsp) field.
> +	 */
> +	if (skb->len < bqr_data_len) {
> +		BT_ERR("AOSP evt data len %d too short (%u expected)",
> +		       skb->len, bqr_data_len);
> +		return false;
> +	}
> +
> +	return true;
> +}

This part I find a bit convoluted, just do a basic length check and then move on. The kernel has no interest in this data.

> diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> index 2fd8886d51b2..49894a995647 100644
> --- a/net/bluetooth/aosp.h
> +++ b/net/bluetooth/aosp.h
> @@ -10,6 +10,8 @@ void aosp_do_close(struct hci_dev *hdev);
> 
> bool aosp_has_quality_report(struct hci_dev *hdev);
> int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> +bool aosp_is_quality_report_evt(struct sk_buff *skb);
> +bool aosp_pull_quality_report_data(struct sk_buff *skb);
> 
> #else
> 
> @@ -26,4 +28,14 @@ static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> 	return -EOPNOTSUPP;
> }
> 
> +static inline bool aosp_is_quality_report_evt(struct sk_buff *skb)
> +{
> +	return false;
> +}
> +
> +static inline bool aosp_pull_quality_report_data(struct sk_buff *skb)
> +{
> +	return false;
> +}
> +
> #endif
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 681c623aa380..bccb659a9454 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -37,6 +37,7 @@
> #include "smp.h"
> #include "msft.h"
> #include "eir.h"
> +#include "aosp.h"
> 
> #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> 		 "\x00\x00\x00\x00\x00\x00\x00\x00"
> @@ -4225,6 +4226,36 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
> 	queue_work(hdev->workqueue, &hdev->tx_work);
> }
> 
> +#define QUALITY_SPEC_NA			0x0
> +#define QUALITY_SPEC_INTEL_TELEMETRY	0x1
> +#define QUALITY_SPEC_AOSP_BQR		0x2
> +
> +static bool quality_report_evt(struct hci_dev *hdev,  void *data,
> +			       struct sk_buff *skb)
> +{
> +	if (aosp_is_quality_report_evt(skb)) {
> +		if (aosp_has_quality_report(hdev) &&
> +		    aosp_pull_quality_report_data(skb))
> +			mgmt_quality_report(hdev, skb, QUALITY_SPEC_AOSP_BQR);
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void hci_vendor_evt(struct hci_dev *hdev, void *data,
> +			   struct sk_buff *skb)
> +{
> +	/* Every distinct vendor specification must have a well-defined
> +	 * condition to determine if an event meets the specification.
> +	 * The skb is consumed by a specification only if the event meets
> +	 * the specification.
> +	 */
> +	if (!quality_report_evt(hdev, data, skb))
> +		msft_vendor_evt(hdev, data, skb);
> +}

No, not like this. This gets messy really quickly.

We should allow for defining vendor event prefixes here. That AOSP decided to convolute the space 0x54 and above in unfortunate, but that is what we have to deal with.

> +
> static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
> 				struct sk_buff *skb)
> {
> @@ -6811,7 +6842,7 @@ static const struct hci_ev {
> 	HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
> 	       sizeof(struct hci_ev_num_comp_blocks)),
> 	/* [0xff = HCI_EV_VENDOR] */
> -	HCI_EV(HCI_EV_VENDOR, msft_vendor_evt, 0),
> +	HCI_EV(HCI_EV_VENDOR, hci_vendor_evt, 0),
> };
> 
> static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 08d6494f1b34..78687ae885be 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4389,6 +4389,28 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> 			       MGMT_STATUS_NOT_SUPPORTED);
> }
> 
> +int mgmt_quality_report(struct hci_dev *hdev, struct sk_buff *skb,
> +			u8 quality_spec)
> +{
> +	struct mgmt_ev_quality_report *ev;
> +	size_t ev_len;
> +	int err;
> +
> +	/* The ev comes with a variable-length data field. */
> +	ev_len = sizeof(*ev) + skb->len;
> +	ev = kmalloc(ev_len, GFP_KERNEL);
> +	if (!ev)
> +		return -ENOMEM;
> +
> +	ev->quality_spec = quality_spec;
> +	ev->data_len = skb->len;
> +	memcpy(ev->data, skb->data, skb->len);
> +	err = mgmt_event(MGMT_EV_QUALITY_REPORT, hdev, ev, ev_len, NULL);
> +	kfree(ev);
> +
> +	return err;
> +}
> +

Don’t we have mgmt helper functions that allow us to add headers to a mgmt skb. I think there is really no point in allocating memory via kmalloc.

Regards

Marcel

Re: [PATCH v1 2/2] Bluetooth: btintel: surface Intel telemetry events through mgmt

[Marcel Holtmann]

Hi Jospeh,

> When receiving a HCI vendor event, the kernel checks if it is an
> Intel telemetry event. If yes, the event is sent to bluez user
> space through the mgmt socket.
> 
> Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
> 
> drivers/bluetooth/btintel.c      | 43 +++++++++++++++++++++++++++++++-
> drivers/bluetooth/btintel.h      | 12 +++++++++
> include/net/bluetooth/hci_core.h |  2 ++
> net/bluetooth/hci_event.c        | 12 ++++++---
> 4 files changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 1a4f8b227eac..d3b7a796cb91 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -2401,8 +2401,10 @@ static int btintel_setup_combined(struct hci_dev *hdev)
> 	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> 	set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> 
> -	/* Set up the quality report callback for Intel devices */
> +	/* Set up the quality report callbacks for Intel devices */
> 	hdev->set_quality_report = btintel_set_quality_report;
> +	hdev->is_quality_report_evt = btintel_is_quality_report_evt;
> +	hdev->pull_quality_report_data = btintel_pull_quality_report_data;

we are not doing this. This is all internal handling. Don’t bother the core hci_dev with it.

> 
> 	/* For Legacy device, check the HW platform value and size */
> 	if (skb->len == sizeof(ver) && skb->data[1] == 0x37) {
> @@ -2645,6 +2647,45 @@ void btintel_secure_send_result(struct hci_dev *hdev,
> }
> EXPORT_SYMBOL_GPL(btintel_secure_send_result);
> 
> +#define INTEL_PREFIX		0x8087
> +#define TELEMETRY_CODE		0x03
> +
> +struct intel_prefix_evt_data {
> +	__le16 vendor_prefix;
> +	__u8 code;
> +	__u8 data[0];   /* a number of struct intel_tlv subevents */
> +} __packed;
> +
> +bool btintel_is_quality_report_evt(struct sk_buff *skb)
> +{
> +	struct intel_prefix_evt_data *ev;
> +	u16 vendor_prefix;
> +
> +	if (skb->len < sizeof(struct intel_prefix_evt_data))
> +		return false;
> +
> +	ev = (struct intel_prefix_evt_data *)skb->data;
> +	vendor_prefix = __le16_to_cpu(ev->vendor_prefix);
> +
> +	return vendor_prefix == INTEL_PREFIX && ev->code == TELEMETRY_CODE;
> +}
> +EXPORT_SYMBOL_GPL(btintel_is_quality_report_evt);
> +
> +bool btintel_pull_quality_report_data(struct sk_buff *skb)
> +{
> +	skb_pull(skb, sizeof(struct intel_prefix_evt_data));
> +
> +	/* A telemetry event contains at least one intel_tlv subevent. */
> +	if (skb->len < sizeof(struct intel_tlv)) {
> +		BT_ERR("Telemetry event length %d too short (at least %u)",
> +		       skb->len, sizeof(struct intel_tlv));
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(btintel_pull_quality_report_data);
> +
> 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 c9b24e9299e2..841aef3dbd4c 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -210,6 +210,8 @@ void btintel_bootup(struct hci_dev *hdev, const void *ptr, unsigned int len);
> void btintel_secure_send_result(struct hci_dev *hdev,
> 				const void *ptr, unsigned int len);
> int btintel_set_quality_report(struct hci_dev *hdev, bool enable);
> +bool btintel_is_quality_report_evt(struct sk_buff *skb);
> +bool btintel_pull_quality_report_data(struct sk_buff *skb);
> #else
> 
> static inline int btintel_check_bdaddr(struct hci_dev *hdev)
> @@ -305,4 +307,14 @@ static inline int btintel_set_quality_report(struct hci_dev *hdev, bool enable)
> {
> 	return -ENODEV;
> }
> +
> +static inline bool btintel_is_quality_report_evt(struct sk_buff *skb)
> +{
> +	return false;
> +}
> +
> +static inline bool btintel_pull_quality_report_data(struct sk_buff *skb);
> +{
> +	return false;
> +}
> #endif
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 727cb9c056b2..b74ba1585df9 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -632,6 +632,8 @@ struct hci_dev {
> 	void (*cmd_timeout)(struct hci_dev *hdev);
> 	bool (*wakeup)(struct hci_dev *hdev);
> 	int (*set_quality_report)(struct hci_dev *hdev, bool enable);
> +	bool (*is_quality_report_evt)(struct sk_buff *skb);
> +	bool (*pull_quality_report_data)(struct sk_buff *skb);
> 	int (*get_data_path_id)(struct hci_dev *hdev, __u8 *data_path);
> 	int (*get_codec_config_data)(struct hci_dev *hdev, __u8 type,
> 				     struct bt_codec *codec, __u8 *vnd_len,
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index bccb659a9454..5f9cc7b942a1 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4237,11 +4237,17 @@ static bool quality_report_evt(struct hci_dev *hdev,  void *data,
> 		if (aosp_has_quality_report(hdev) &&
> 		    aosp_pull_quality_report_data(skb))
> 			mgmt_quality_report(hdev, skb, QUALITY_SPEC_AOSP_BQR);
> -
> -		return true;
> +	} else if (hdev->is_quality_report_evt &&
> +		   hdev->is_quality_report_evt(skb)) {
> +		if (hdev->set_quality_report &&
> +		    hdev->pull_quality_report_data(skb))
> +			mgmt_quality_report(hdev, skb,
> +					    QUALITY_SPEC_INTEL_TELEMETRY);
> +	} else {
> +		return false;
> 	}

No. You now have Intel internal details bleeding into the core.

Regards

Marcel