diff mbox series

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

Message ID 20220121192152.v1.1.I2015b42d2d0a502334c9c3a2983438b89716d4f0@changeid (mailing list archive)
State Superseded
Headers show
Series [v1,1/2] Bluetooth: aosp: surface AOSP quality report through mgmt | expand

Checks

Context Check Description
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/subjectprefix success PASS
tedd_an/buildkernel success Build Kernel PASS
tedd_an/buildkernel32 success Build Kernel32 PASS
tedd_an/incremental_build success Pass
tedd_an/testrunnersetup success Test Runner Setup PASS
tedd_an/testrunnerl2cap-tester success Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerbnep-tester success Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnermgmt-tester success Total: 493, Passed: 493 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerrfcomm-tester success Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersco-tester success Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersmp-tester success Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunneruserchan-tester success Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 0

Commit Message

Joseph Hwang Jan. 21, 2022, 11:22 a.m. UTC 
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(-)

Comments

bluez.test.bot@gmail.com Jan. 21, 2022, 11:57 a.m. UTC | #1 
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=607231

---Test result---

Test Summary:
CheckPatch                    PASS      4.77 seconds
GitLint                       PASS      1.99 seconds
SubjectPrefix                 PASS      1.69 seconds
BuildKernel                   PASS      36.40 seconds
BuildKernel32                 PASS      32.88 seconds
Incremental Build with patchesPASS      74.26 seconds
TestRunner: Setup             PASS      564.84 seconds
TestRunner: l2cap-tester      PASS      15.78 seconds
TestRunner: bnep-tester       PASS      7.40 seconds
TestRunner: mgmt-tester       PASS      126.11 seconds
TestRunner: rfcomm-tester     PASS      9.67 seconds
TestRunner: sco-tester        PASS      10.10 seconds
TestRunner: smp-tester        PASS      9.89 seconds
TestRunner: userchan-tester   PASS      8.40 seconds



---
Regards,
Linux Bluetooth
Marcel Holtmann Jan. 21, 2022, 8:16 p.m. UTC | #2 
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
diff mbox series

Patch

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)
 {