Re: [PATCH v6 1/3] Bluetooth: Add struct of reading AOSP vendor capabilities
From: Marcel Holtmann <marcel@holtmann.org>
Date: 2021-10-25 13:18:16
Also in:
linux-bluetooth, lkml
Hi Jospeh,
quoted hunk ↗ jump to hunk
This patch adds the struct of reading AOSP vendor capabilities. New capabilities are added incrementally. Note that the version_supported octets will be used to determine whether a capability has been defined for the version. Signed-off-by: Joseph Hwang <redacted> --- Changes in v6: - Add historical versions of struct aosp_rp_le_get_vendor_capabilities. - Perform the basic check about the struct length. - Through the version, bluetooth_quality_report_support can be checked. Changes in v5: - This is a new patch. - Add struct aosp_rp_le_get_vendor_capabilities so that next patch can determine whether a particular capability is supported or not. include/net/bluetooth/hci_core.h | 1 + net/bluetooth/aosp.c | 116 ++++++++++++++++++++++++++++++- 2 files changed, 116 insertions(+), 1 deletion(-)diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index dd8840e70e25..32b3774227f2 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h@@ -603,6 +603,7 @@ struct hci_dev {#if IS_ENABLED(CONFIG_BT_AOSPEXT) bool aosp_capable; + bool aosp_quality_report; #endif int (*open)(struct hci_dev *hdev);diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c index a1b7762335a5..64684b2bf79b 100644 --- a/net/bluetooth/aosp.c +++ b/net/bluetooth/aosp.c@@ -8,9 +8,53 @@#include "aosp.h" +/* Command complete parameters of LE_Get_Vendor_Capabilities_Command + * The parameters grow over time. The first version that declares the + * version_supported field is v0.95. Refer to + * https://cs.android.com/android/platform/superproject/+/master:system/ + * bt/gd/hci/controller.cc;l=452?q=le_get_vendor_capabilities_handler + */ + +/* the base capabilities struct with the version_supported field */ +struct aosp_rp_le_get_vendor_capa_v95 { + __u8 status; + __u8 max_advt_instances; + __u8 offloaded_resolution_of_private_address; + __u16 total_scan_results_storage; + __u8 max_irk_list_sz; + __u8 filtering_support; + __u8 max_filter; + __u8 activity_energy_info_support; + __u16 version_supported; + __u16 total_num_of_advt_tracked; + __u8 extended_scan_support; + __u8 debug_logging_supported; +} __packed; + +struct aosp_rp_le_get_vendor_capa_v96 { + struct aosp_rp_le_get_vendor_capa_v95 v95; + /* v96 */ + __u8 le_address_generation_offloading_support; +} __packed; + +struct aosp_rp_le_get_vendor_capa_v98 { + struct aosp_rp_le_get_vendor_capa_v96 v96; + /* v98 */ + __u32 a2dp_source_offload_capability_mask; + __u8 bluetooth_quality_report_support; +} __packed; + +struct aosp_rp_le_get_vendor_capa_v100 { + struct aosp_rp_le_get_vendor_capa_v98 v98; + /* v100 */ + __u32 dynamic_audio_buffer_support; +} __packed; +
while this is an interesting idea to document the different versions, I don’t thing you need to overboard like this. Just use the full struct and make sure you check the version_supported to ensure you are not reading beyond the bounds of your result. Meaning you can have a struct pointer point to smaller memory, just make sure you never access it if it is not there. So I would just add defines for the different versions and their length of the struct. Oh, I might have forgotten that, you need to use __le16 and __le32 to indicate the endianness of the data on the wire.
quoted hunk ↗ jump to hunk
void aosp_do_open(struct hci_dev *hdev) { struct sk_buff *skb; + struct aosp_rp_le_get_vendor_capa_v95 *base_rp; + u16 version_supported; if (!hdev->aosp_capable) return;@@ -20,9 +64,79 @@ void aosp_do_open(struct hci_dev *hdev)/* LE Get Vendor Capabilities Command */ skb = __hci_cmd_sync(hdev, hci_opcode_pack(0x3f, 0x153), 0, NULL, HCI_CMD_TIMEOUT); - if (IS_ERR(skb)) + if (IS_ERR(skb)) { + bt_dev_warn(hdev, "AOSP get vendor capabilities (%ld)", + PTR_ERR(skb));
This is actually an error. If the driver indicates support for it, this better succeed. If not complain loudly.
return; + } + + bt_dev_dbg(hdev, "aosp le vendor capabilities length %d", skb->len);
Skip this one. Add a basic length check that you know you can get to rp->version_supported field.
+
+ base_rp = (struct aosp_rp_le_get_vendor_capa_v95 *)skb->data;
+
+ if (base_rp->status) {
+ bt_dev_err(hdev, "AOSP LE Get Vendor Capabilities status %d",
+ base_rp->status);
+ goto done;
+ }Actually the status is already evaluated via the __hci_cmd_sync command. No need to repeat it here.
+
+ version_supported = le16_to_cpu(base_rp->version_supported);
+ bt_dev_info(hdev, "AOSP version %u", version_supported);
+
+ /* Do not support very old versions. */
+ if (version_supported < 95) {
+ bt_dev_err(hdev, "capabilities version %u too old",
+ version_supported);
+ goto done;
+ }This is not an error. Just print a warning here. And “AOSP capabilities ..” please.
+
+ if (version_supported >= 95) {
+ struct aosp_rp_le_get_vendor_capa_v95 *rp;
+
+ rp = (struct aosp_rp_le_get_vendor_capa_v95 *)skb->data;
+ if (skb->len < sizeof(*rp))
+ goto length_error;
+ }
+
+ if (version_supported >= 96) {
+ struct aosp_rp_le_get_vendor_capa_v96 *rp;
+
+ rp = (struct aosp_rp_le_get_vendor_capa_v96 *)skb->data;
+ if (skb->len < sizeof(*rp))
+ goto length_error;
+ }Since we don’t use any data out of these two above, skip it. If the version is less than < 0.98 we are just ignoring it. You can print a warning that "AOSP quality report is not supported”.
+
+ if (version_supported >= 98) {
+ struct aosp_rp_le_get_vendor_capa_v98 *rp;
+
+ rp = (struct aosp_rp_le_get_vendor_capa_v98 *)skb->data;
+ if (skb->len < sizeof(*rp))
+ goto length_error;
+
+ /* The bluetooth_quality_report_support is defined at version v0.98.
+ * Refer to https://cs.android.com/android/platform/superproject/+/
+ * master:system/bt/gd/hci/controller.cc;l=477
+ */
+ if (rp->bluetooth_quality_report_support) {
+ hdev->aosp_quality_report = true;
+ bt_dev_info(hdev, "bluetooth quality report is supported");
+ }
+ }
+
+ if (version_supported >= 100) {
+ struct aosp_rp_le_get_vendor_capa_v100 *rp;
+
+ rp = (struct aosp_rp_le_get_vendor_capa_v100 *)skb->data;
+ if (skb->len < sizeof(*rp))
+ goto length_error;
+ }Skip that one as well. We only care about quality report support.
+ + goto done; + +length_error: + bt_dev_err(hdev, "AOSP capabilities length %d too short", skb->len); +done: kfree_skb(skb); }
Regards Marcel