All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7 1/2] Bluetooth: enumerate local supported codec and cache details
@ 2021-05-07 11:42 Kiran K
  2021-05-07 11:42 ` [PATCH v7 2/2] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Kiran K @ 2021-05-07 11:42 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Kiran K, Chethan T N, Srivatsa Ravishankar

Move reading of supported local codecs into a separate init function,
query codecs capabilities and cache the data

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Signed-off-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
---
* changes in v7:
  - keep codec enumeration call in hci_init instead of having a separate
    function
  - Remove unused bitmasks defined for LE transports

* changes  in v6:
  - fix compiler warning reported for ARCH=arc

* changes in v5:
  - fix review comments
  - move code used to read standard/vendor codecs caps into single function

* changes in v4:
  - convert  reading of codecs and codecs caps calls from async to sync

* changes in v3
  move codec enumeration into a new init function

* changes in v2
  add skb length check before accessing data

 include/net/bluetooth/hci.h      |  38 +++++++
 include/net/bluetooth/hci_core.h |  14 +++
 net/bluetooth/hci_core.c         | 163 ++++++++++++++++++++++++++++++-
 3 files changed, 211 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c4b0650fb9ae..901603e8b4ed 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1307,6 +1307,26 @@ struct hci_rp_read_data_block_size {
 } __packed;
 
 #define HCI_OP_READ_LOCAL_CODECS	0x100b
+struct hci_standard_codecs {
+	__u8	num;
+	__u8	codec[];
+} __packed;
+
+struct hci_vendor_codec {
+	__le16	company_id;
+	__le16	codec_id;
+} __packed;
+
+struct hci_vendor_codecs {
+	__u8	num;
+	struct hci_vendor_codec codec[];
+} __packed;
+
+struct hci_rp_read_local_supported_codecs {
+	__u8	status;
+	struct hci_standard_codecs std_codecs;
+	struct hci_vendor_codecs vendor_codecs;
+} __packed;
 
 #define HCI_OP_READ_LOCAL_PAIRING_OPTS	0x100c
 struct hci_rp_read_local_pairing_opts {
@@ -1315,6 +1335,24 @@ struct hci_rp_read_local_pairing_opts {
 	__u8     max_key_size;
 } __packed;
 
+#define HCI_OP_READ_LOCAL_CODEC_CAPS	0x100e
+struct hci_op_read_local_codec_caps {
+	__u8	codec_id[5];
+	__u8	transport;
+	__u8	direction;
+} __packed;
+
+struct hci_codec_caps {
+	__u8	len;
+	__u8	caps[];
+} __packed;
+
+struct hci_rp_read_local_codec_caps {
+	__u8	status;
+	__u8	num_caps;
+	struct hci_codec_caps caps[];
+} __packed;
+
 #define HCI_OP_READ_PAGE_SCAN_ACTIVITY	0x0c1b
 struct hci_rp_read_page_scan_activity {
 	__u8     status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 43b08bebae74..d6d0a535a82a 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -131,6 +131,14 @@ struct bdaddr_list {
 	u8 bdaddr_type;
 };
 
+struct codec_list {
+	struct list_head list;
+	u8	transport;
+	u8	codec_id[5];
+	u8	num_caps;
+	struct hci_codec_caps caps[];
+};
+
 struct bdaddr_list_with_irk {
 	struct list_head list;
 	bdaddr_t bdaddr;
@@ -535,6 +543,7 @@ struct hci_dev {
 	struct list_head	pend_le_conns;
 	struct list_head	pend_le_reports;
 	struct list_head	blocked_keys;
+	struct list_head	local_codecs;
 
 	struct hci_dev_stats	stat;
 
@@ -1849,4 +1858,9 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
 #define SCO_AIRMODE_CVSD       0x0000
 #define SCO_AIRMODE_TRANSP     0x0003
 
+#define LOCAL_CODEC_ACL_MASK	BIT(0)
+#define LOCAL_CODEC_SCO_MASK	BIT(1)
+
+#define TRANSPORT_TYPE_MAX	0x04
+
 #endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7baf93eda936..50947a1ed6a9 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -838,10 +838,6 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
 	if (hdev->commands[22] & 0x04)
 		hci_set_event_mask_page_2(req);
 
-	/* Read local codec list if the HCI command is supported */
-	if (hdev->commands[29] & 0x20)
-		hci_req_add(req, HCI_OP_READ_LOCAL_CODECS, 0, NULL);
-
 	/* Read local pairing options if the HCI command is supported */
 	if (hdev->commands[41] & 0x08)
 		hci_req_add(req, HCI_OP_READ_LOCAL_PAIRING_OPTS, 0, NULL);
@@ -907,6 +903,159 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
 	return 0;
 }
 
+static int hci_codec_list_add(struct list_head *list,
+			      struct hci_rp_read_local_codec_caps *rp,
+			      __u32 len,
+			      struct hci_op_read_local_codec_caps *sent)
+{
+	struct codec_list *entry;
+
+	entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	memcpy(entry->codec_id, sent->codec_id, 5);
+	entry->transport = sent->transport;
+	entry->num_caps = rp->num_caps;
+	if (rp->num_caps)
+		memcpy(entry->caps, rp->caps, len);
+	list_add(&entry->list, list);
+
+	return 0;
+}
+
+static void hci_codec_list_clear(struct list_head *codec_list)
+{
+	struct codec_list *c, *n;
+
+	list_for_each_entry_safe(c, n, codec_list, list) {
+		list_del(&c->list);
+		kfree(c);
+	}
+}
+
+static void hci_read_codec_capabilities(struct hci_dev *hdev, void *codec_id,
+					__u8 transport, bool is_vendor_codec)
+{
+	struct hci_op_read_local_codec_caps caps;
+	__u8 i;
+
+	memset(&caps, 0, sizeof(caps));
+
+	if (is_vendor_codec) {
+		caps.codec_id[0] = 0xFF;
+		memcpy(&caps.codec_id[1], codec_id, 4);
+	} else {
+		memcpy(caps.codec_id, codec_id, 1);
+	}
+
+	caps.direction = 0x00;
+
+	for (i = 0; i < TRANSPORT_TYPE_MAX; i++) {
+		if (transport & BIT(i)) {
+			struct hci_rp_read_local_codec_caps *rp;
+			struct sk_buff *skb;
+
+			caps.transport = i;
+			skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODEC_CAPS,
+					     sizeof(caps), &caps,
+					     HCI_CMD_TIMEOUT);
+			if (IS_ERR(skb)) {
+				bt_dev_err(hdev, "Failed to read codec capabilities (%ld)",
+					   PTR_ERR(skb));
+				continue;
+			}
+
+			if (skb->len < sizeof(*rp))
+				goto error;
+
+			rp = (void *)skb->data;
+
+			if (rp->status)
+				goto error;
+
+			hci_dev_lock(hdev);
+			hci_codec_list_add(&hdev->local_codecs, rp, skb->len - 2,
+					   &caps);
+			hci_dev_unlock(hdev);
+error:
+			kfree_skb(skb);
+		}
+	}
+}
+
+static void hci_codec_list_parse(struct hci_dev *hdev, __u8 num_codecs,
+				 void *codec_list, bool is_vendor_codec)
+{
+	__u8 i;
+
+	for (i = 0; i < num_codecs; i++) {
+		if (!is_vendor_codec) {
+			struct hci_standard_codecs *codecs = codec_list;
+
+			hci_read_codec_capabilities(hdev, &codecs->codec[i],
+						    LOCAL_CODEC_ACL_MASK,
+						    is_vendor_codec);
+		} else {
+			struct hci_vendor_codecs *codecs = codec_list;
+
+			hci_read_codec_capabilities(hdev, &codecs->codec[i],
+						    LOCAL_CODEC_ACL_MASK,
+						    is_vendor_codec);
+		}
+	}
+}
+
+static void hci_read_supported_codecs(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	struct hci_rp_read_local_supported_codecs *rp;
+	struct hci_standard_codecs *std_codecs;
+	struct hci_vendor_codecs *vendor_codecs;
+
+	skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODECS, 0, NULL,
+			     HCI_CMD_TIMEOUT);
+
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "Failed to read local supported codecs (%ld)",
+			   PTR_ERR(skb));
+		return;
+	}
+
+	if (skb->len < sizeof(*rp))
+		goto error;
+
+	rp = (void *)skb->data;
+
+	if (rp->status)
+		goto error;
+
+	skb_pull(skb, sizeof(rp->status));
+
+	std_codecs = (void *)skb->data;
+
+	if (skb->len < flex_array_size(std_codecs, codec, std_codecs->num)
+	    + sizeof(std_codecs->num))
+		goto error;
+
+	hci_codec_list_parse(hdev, std_codecs->num, std_codecs, false);
+
+	skb_pull(skb, flex_array_size(std_codecs, codec, std_codecs->num)
+		 + sizeof(std_codecs->num));
+
+	vendor_codecs = (void *)skb->data;
+
+	if (skb->len <
+	    flex_array_size(vendor_codecs, codec, vendor_codecs->num)
+	    + sizeof(vendor_codecs->num))
+		goto error;
+
+	hci_codec_list_parse(hdev, vendor_codecs->num, vendor_codecs, true);
+
+error:
+	kfree_skb(skb);
+}
+
 static int __hci_init(struct hci_dev *hdev)
 {
 	int err;
@@ -937,6 +1086,10 @@ static int __hci_init(struct hci_dev *hdev)
 	if (err < 0)
 		return err;
 
+	/* Read local codec list if the HCI command is supported */
+	if (hdev->commands[29] & 0x20)
+		hci_read_supported_codecs(hdev);
+
 	/* This function is only called when the controller is actually in
 	 * configured state. When the controller is marked as unconfigured,
 	 * this initialization procedure is not run.
@@ -1836,6 +1989,7 @@ int hci_dev_do_close(struct hci_dev *hdev)
 	memset(hdev->eir, 0, sizeof(hdev->eir));
 	memset(hdev->dev_class, 0, sizeof(hdev->dev_class));
 	bacpy(&hdev->random_addr, BDADDR_ANY);
+	hci_codec_list_clear(&hdev->local_codecs);
 
 	hci_req_sync_unlock(hdev);
 
@@ -3837,6 +3991,7 @@ struct hci_dev *hci_alloc_dev(void)
 	INIT_LIST_HEAD(&hdev->conn_hash.list);
 	INIT_LIST_HEAD(&hdev->adv_instances);
 	INIT_LIST_HEAD(&hdev->blocked_keys);
+	INIT_LIST_HEAD(&hdev->local_codecs);
 
 	INIT_WORK(&hdev->rx_work, hci_rx_work);
 	INIT_WORK(&hdev->cmd_work, hci_cmd_work);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v7 2/2] Bluetooth: Add support for Read Local Supported Codecs V2
  2021-05-07 11:42 [PATCH v7 1/2] Bluetooth: enumerate local supported codec and cache details Kiran K
@ 2021-05-07 11:42 ` Kiran K
  2021-05-07 12:08 ` [v7,1/2] Bluetooth: enumerate local supported codec and cache details bluez.test.bot
  2021-05-13 15:30 ` [PATCH v7 1/2] " Marcel Holtmann
  2 siblings, 0 replies; 5+ messages in thread
From: Kiran K @ 2021-05-07 11:42 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Kiran K, Chethan T N, Srivatsa Ravishankar

Use V2 version of read local supported command is controller
supports

snoop:
> HCI Event: Command Complete (0x0e) plen 20
      Read Local Supported Codecs V2 (0x04|0x000d) ncmd 1
        Status: Success (0x00)
        Number of supported codecs: 7
          Codec: u-law log (0x00)
          Logical Transport Type: 0x02
            Codec supported over BR/EDR SCO and eSCO
          Codec: A-law log (0x01)
          Logical Transport Type: 0x02
            Codec supported over BR/EDR SCO and eSCO
          Codec: CVSD (0x02)
          Logical Transport Type: 0x02
            Codec supported over BR/EDR SCO and eSCO
          Codec: Transparent (0x03)
          Logical Transport Type: 0x02
            Codec supported over BR/EDR SCO and eSCO
          Codec: Linear PCM (0x04)
          Logical Transport Type: 0x02
            Codec supported over BR/EDR SCO and eSCO
          Codec: Reserved (0x08)
          Logical Transport Type: 0x03
            Codec supported over BR/EDR ACL
            Codec supported over BR/EDR SCO and eSCO
          Codec: mSBC (0x05)
          Logical Transport Type: 0x03
            Codec supported over BR/EDR ACL
            Codec supported over BR/EDR SCO and eSCO
        Number of vendor codecs: 0
......
< HCI Command: Read Local Suppor.. (0x04|0x000e) plen 7
        Codec: mSBC (0x05)
        Logical Transport Type: 0x00
        Direction: Input (Host to Controller) (0x00)
> HCI Event: Command Complete (0x0e) plen 12
      Read Local Supported Codec Capabilities (0x04|0x000e) ncmd 1
        Status: Success (0x00)
        Number of codec capabilities: 1
         Capabilities #0:
        00 00 11 15 02 33

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Signed-off-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
* changes in v7:
  call codec enumeration code in hci_init instead of having it in a separate
  function

* changes in v6
  no changes

* changes in v5:
  fix review comments

* changes in v4:
  converts codec read capabilities calls from async to sync

* changes in v3:
  No changes

* changes in v2:
  add length check for event data before accessing

 include/net/bluetooth/hci.h | 28 ++++++++++++++
 net/bluetooth/hci_core.c    | 76 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 901603e8b4ed..b36ac8512a11 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1335,6 +1335,34 @@ struct hci_rp_read_local_pairing_opts {
 	__u8     max_key_size;
 } __packed;
 
+#define HCI_OP_READ_LOCAL_CODECS_V2	0x100d
+struct hci_standard_codec_v2 {
+	__u8	codec_id;
+	__u8	transport;
+} __packed;
+
+struct hci_standard_codecs_v2 {
+	__u8	num;
+	struct hci_standard_codec_v2 codec[];
+} __packed;
+
+struct hci_vendor_codec_v2 {
+	__le16	company_id;
+	__le16	vendor_id;
+	__u8	transport;
+} __packed;
+
+struct hci_vendor_codecs_v2 {
+	__u8	num;
+	struct hci_vendor_codec_v2 codec[];
+} __packed;
+
+struct hci_rp_read_local_supported_codecs_v2 {
+	__u8	status;
+	struct hci_standard_codecs_v2 std_codecs;
+	struct hci_vendor_codecs_v2 vendor_codecs;
+} __packed;
+
 #define HCI_OP_READ_LOCAL_CODEC_CAPS	0x100e
 struct hci_op_read_local_codec_caps {
 	__u8	codec_id[5];
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 50947a1ed6a9..debb9f871479 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1006,6 +1006,28 @@ static void hci_codec_list_parse(struct hci_dev *hdev, __u8 num_codecs,
 	}
 }
 
+static void hci_codec_list_parse_v2(struct hci_dev *hdev, __u8 num_codecs,
+				    void *codec_list, bool is_vendor_codec)
+{
+	__u8 i;
+
+	for (i = 0; i < num_codecs; i++) {
+		if (!is_vendor_codec) {
+			struct hci_standard_codecs_v2 *codecs = codec_list;
+
+			hci_read_codec_capabilities(hdev, &codecs->codec[i],
+						    codecs->codec[i].transport,
+						    is_vendor_codec);
+		} else {
+			struct hci_vendor_codecs_v2 *codecs = codec_list;
+
+			hci_read_codec_capabilities(hdev, &codecs->codec[i],
+						    codecs->codec[i].transport,
+						    is_vendor_codec);
+		}
+	}
+}
+
 static void hci_read_supported_codecs(struct hci_dev *hdev)
 {
 	struct sk_buff *skb;
@@ -1056,6 +1078,56 @@ static void hci_read_supported_codecs(struct hci_dev *hdev)
 	kfree_skb(skb);
 }
 
+static void hci_read_supported_codecs_v2(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	struct hci_rp_read_local_supported_codecs_v2 *rp;
+	struct hci_standard_codecs_v2 *std_codecs;
+	struct hci_vendor_codecs_v2 *vendor_codecs;
+
+	skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODECS_V2, 0, NULL,
+			     HCI_CMD_TIMEOUT);
+
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "Failed to read local supported codecs (%ld)",
+			   PTR_ERR(skb));
+		return;
+	}
+
+	if (skb->len < sizeof(*rp))
+		goto error;
+
+	rp = (void *)skb->data;
+
+	if (rp->status)
+		goto error;
+
+	skb_pull(skb, sizeof(rp->status));
+
+	std_codecs = (void *)skb->data;
+
+	if (skb->len < flex_array_size(std_codecs, codec, std_codecs->num)
+	    + sizeof(std_codecs->num))
+		goto error;
+
+	hci_codec_list_parse_v2(hdev, std_codecs->num, std_codecs, false);
+
+	skb_pull(skb, flex_array_size(std_codecs, codec, std_codecs->num)
+		 + sizeof(std_codecs->num));
+
+	vendor_codecs = (void *)skb->data;
+
+	if (skb->len <
+	    flex_array_size(vendor_codecs, codec, vendor_codecs->num)
+	    + sizeof(vendor_codecs->num))
+		goto error;
+
+	hci_codec_list_parse_v2(hdev, vendor_codecs->num, vendor_codecs, true);
+
+error:
+	kfree_skb(skb);
+}
+
 static int __hci_init(struct hci_dev *hdev)
 {
 	int err;
@@ -1087,7 +1159,9 @@ static int __hci_init(struct hci_dev *hdev)
 		return err;
 
 	/* Read local codec list if the HCI command is supported */
-	if (hdev->commands[29] & 0x20)
+	if (hdev->commands[45] & 0x04)
+		hci_read_supported_codecs_v2(hdev);
+	else if (hdev->commands[29] & 0x20)
 		hci_read_supported_codecs(hdev);
 
 	/* This function is only called when the controller is actually in
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* RE: [v7,1/2] Bluetooth: enumerate local supported codec and cache details
  2021-05-07 11:42 [PATCH v7 1/2] Bluetooth: enumerate local supported codec and cache details Kiran K
  2021-05-07 11:42 ` [PATCH v7 2/2] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
@ 2021-05-07 12:08 ` bluez.test.bot
  2021-05-13 15:30 ` [PATCH v7 1/2] " Marcel Holtmann
  2 siblings, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2021-05-07 12:08 UTC (permalink / raw)
  To: linux-bluetooth, kiran.k

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

---Test result---

##############################
Test: CheckPatch - PASS


##############################
Test: CheckGitLint - PASS


##############################
Test: CheckBuildK - PASS


##############################
Test: CheckTestRunner: Setup - PASS


##############################
Test: CheckTestRunner: l2cap-tester - PASS
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: CheckTestRunner: bnep-tester - PASS
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: CheckTestRunner: mgmt-tester - PASS
Total: 416, Passed: 403 (96.9%), Failed: 0, Not Run: 13

##############################
Test: CheckTestRunner: rfcomm-tester - PASS
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: CheckTestRunner: sco-tester - PASS
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: CheckTestRunner: smp-tester - PASS
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: CheckTestRunner: userchan-tester - PASS
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 51410 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3897 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 551028 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 14754 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9911 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11822 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 6478 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v7 1/2] Bluetooth: enumerate local supported codec and cache details
  2021-05-07 11:42 [PATCH v7 1/2] Bluetooth: enumerate local supported codec and cache details Kiran K
  2021-05-07 11:42 ` [PATCH v7 2/2] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
  2021-05-07 12:08 ` [v7,1/2] Bluetooth: enumerate local supported codec and cache details bluez.test.bot
@ 2021-05-13 15:30 ` Marcel Holtmann
  2021-05-17 12:57   ` K, Kiran
  2 siblings, 1 reply; 5+ messages in thread
From: Marcel Holtmann @ 2021-05-13 15:30 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth, Chethan T N, Srivatsa Ravishankar

Hi Kiran,

> Move reading of supported local codecs into a separate init function,
> query codecs capabilities and cache the data
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Signed-off-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
> * changes in v7:
>  - keep codec enumeration call in hci_init instead of having a separate
>    function
>  - Remove unused bitmasks defined for LE transports
> 
> * changes  in v6:
>  - fix compiler warning reported for ARCH=arc
> 
> * changes in v5:
>  - fix review comments
>  - move code used to read standard/vendor codecs caps into single function
> 
> * changes in v4:
>  - convert  reading of codecs and codecs caps calls from async to sync
> 
> * changes in v3
>  move codec enumeration into a new init function
> 
> * changes in v2
>  add skb length check before accessing data
> 
> include/net/bluetooth/hci.h      |  38 +++++++
> include/net/bluetooth/hci_core.h |  14 +++
> net/bluetooth/hci_core.c         | 163 ++++++++++++++++++++++++++++++-
> 3 files changed, 211 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index c4b0650fb9ae..901603e8b4ed 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1307,6 +1307,26 @@ struct hci_rp_read_data_block_size {
> } __packed;
> 
> #define HCI_OP_READ_LOCAL_CODECS	0x100b
> +struct hci_standard_codecs {
> +	__u8	num;
> +	__u8	codec[];
> +} __packed;
> +
> +struct hci_vendor_codec {
> +	__le16	company_id;
> +	__le16	codec_id;
> +} __packed;
> +
> +struct hci_vendor_codecs {
> +	__u8	num;
> +	struct hci_vendor_codec codec[];
> +} __packed;
> +
> +struct hci_rp_read_local_supported_codecs {
> +	__u8	status;
> +	struct hci_standard_codecs std_codecs;
> +	struct hci_vendor_codecs vendor_codecs;
> +} __packed;

I am really trying to understand this business, but my brain keeps failing me. I really don’t know how the compiler should work this out. So have we tested this with btvirt returning some random vendor codecs anyway.

And on a side note, I rather have std_codec and vnd_codec here as naming to avoid long names like standard and vendor that are used to often that abbreviations are fine.

> 
> #define HCI_OP_READ_LOCAL_PAIRING_OPTS	0x100c
> struct hci_rp_read_local_pairing_opts {
> @@ -1315,6 +1335,24 @@ struct hci_rp_read_local_pairing_opts {
> 	__u8     max_key_size;
> } __packed;
> 
> +#define HCI_OP_READ_LOCAL_CODEC_CAPS	0x100e
> +struct hci_op_read_local_codec_caps {
> +	__u8	codec_id[5];
> +	__u8	transport;
> +	__u8	direction;
> +} __packed;
> +
> +struct hci_codec_caps {
> +	__u8	len;
> +	__u8	caps[];
> +} __packed;
> +
> +struct hci_rp_read_local_codec_caps {
> +	__u8	status;
> +	__u8	num_caps;
> +	struct hci_codec_caps caps[];
> +} __packed;
> +

This is another one that I do not get. Frankly the structure can just end at num_caps and we just parse the rest one by one.

> #define HCI_OP_READ_PAGE_SCAN_ACTIVITY	0x0c1b
> struct hci_rp_read_page_scan_activity {
> 	__u8     status;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 43b08bebae74..d6d0a535a82a 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -131,6 +131,14 @@ struct bdaddr_list {
> 	u8 bdaddr_type;
> };
> 
> +struct codec_list {
> +	struct list_head list;
> +	u8	transport;
> +	u8	codec_id[5];
> +	u8	num_caps;
> +	struct hci_codec_caps caps[];
> +};
> +
> struct bdaddr_list_with_irk {
> 	struct list_head list;
> 	bdaddr_t bdaddr;
> @@ -535,6 +543,7 @@ struct hci_dev {
> 	struct list_head	pend_le_conns;
> 	struct list_head	pend_le_reports;
> 	struct list_head	blocked_keys;
> +	struct list_head	local_codecs;
> 
> 	struct hci_dev_stats	stat;
> 
> @@ -1849,4 +1858,9 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
> #define SCO_AIRMODE_CVSD       0x0000
> #define SCO_AIRMODE_TRANSP     0x0003
> 
> +#define LOCAL_CODEC_ACL_MASK	BIT(0)
> +#define LOCAL_CODEC_SCO_MASK	BIT(1)
> +
> +#define TRANSPORT_TYPE_MAX	0x04
> +
> #endif /* __HCI_CORE_H */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 7baf93eda936..50947a1ed6a9 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -838,10 +838,6 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
> 	if (hdev->commands[22] & 0x04)
> 		hci_set_event_mask_page_2(req);
> 
> -	/* Read local codec list if the HCI command is supported */
> -	if (hdev->commands[29] & 0x20)
> -		hci_req_add(req, HCI_OP_READ_LOCAL_CODECS, 0, NULL);
> -
> 	/* Read local pairing options if the HCI command is supported */
> 	if (hdev->commands[41] & 0x08)
> 		hci_req_add(req, HCI_OP_READ_LOCAL_PAIRING_OPTS, 0, NULL);
> @@ -907,6 +903,159 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
> 	return 0;
> }
> 
> +static int hci_codec_list_add(struct list_head *list,
> +			      struct hci_rp_read_local_codec_caps *rp,
> +			      __u32 len,
> +			      struct hci_op_read_local_codec_caps *sent)
> +{
> +	struct codec_list *entry;
> +
> +	entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL);
> +	if (!entry)
> +		return -ENOMEM;
> +
> +	memcpy(entry->codec_id, sent->codec_id, 5);
> +	entry->transport = sent->transport;
> +	entry->num_caps = rp->num_caps;
> +	if (rp->num_caps)
> +		memcpy(entry->caps, rp->caps, len);
> +	list_add(&entry->list, list);
> +
> +	return 0;
> +}
> +
> +static void hci_codec_list_clear(struct list_head *codec_list)
> +{
> +	struct codec_list *c, *n;
> +
> +	list_for_each_entry_safe(c, n, codec_list, list) {
> +		list_del(&c->list);
> +		kfree(c);
> +	}
> +}
> +
> +static void hci_read_codec_capabilities(struct hci_dev *hdev, void *codec_id,
> +					__u8 transport, bool is_vendor_codec)
> +{
> +	struct hci_op_read_local_codec_caps caps;
> +	__u8 i;
> +
> +	memset(&caps, 0, sizeof(caps));
> +
> +	if (is_vendor_codec) {
> +		caps.codec_id[0] = 0xFF;
> +		memcpy(&caps.codec_id[1], codec_id, 4);
> +	} else {
> +		memcpy(caps.codec_id, codec_id, 1);
> +	}

This business might work, but I am not a big fan. There is most likely luck here that what you receive from the wire gets send back and thus with luck this will also work on big endian, but it is rather unclean.

When reading this patch, the business of is_vendor_codec seems to introduce a complexity without any benefit. Does it really make the code smaller or are we just trying to hard to unify standard codecs with vendor codecs.

> +
> +	caps.direction = 0x00;
> +
> +	for (i = 0; i < TRANSPORT_TYPE_MAX; i++) {
> +		if (transport & BIT(i)) {
> +			struct hci_rp_read_local_codec_caps *rp;
> +			struct sk_buff *skb;
> +
> +			caps.transport = i;
> +			skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODEC_CAPS,
> +					     sizeof(caps), &caps,
> +					     HCI_CMD_TIMEOUT);
> +			if (IS_ERR(skb)) {
> +				bt_dev_err(hdev, "Failed to read codec capabilities (%ld)",
> +					   PTR_ERR(skb));
> +				continue;
> +			}
> +
> +			if (skb->len < sizeof(*rp))
> +				goto error;
> +
> +			rp = (void *)skb->data;
> +
> +			if (rp->status)
> +				goto error;
> +
> +			hci_dev_lock(hdev);
> +			hci_codec_list_add(&hdev->local_codecs, rp, skb->len - 2,
> +					   &caps);
> +			hci_dev_unlock(hdev);
> +error:
> +			kfree_skb(skb);
> +		}
> +	}
> +}
> +
> +static void hci_codec_list_parse(struct hci_dev *hdev, __u8 num_codecs,
> +				 void *codec_list, bool is_vendor_codec)
> +{
> +	__u8 i;
> +
> +	for (i = 0; i < num_codecs; i++) {
> +		if (!is_vendor_codec) {
> +			struct hci_standard_codecs *codecs = codec_list;
> +
> +			hci_read_codec_capabilities(hdev, &codecs->codec[i],
> +						    LOCAL_CODEC_ACL_MASK,
> +						    is_vendor_codec);
> +		} else {
> +			struct hci_vendor_codecs *codecs = codec_list;
> +
> +			hci_read_codec_capabilities(hdev, &codecs->codec[i],
> +						    LOCAL_CODEC_ACL_MASK,
> +						    is_vendor_codec);
> +		}
> +	}
> +}
> +
> +static void hci_read_supported_codecs(struct hci_dev *hdev)
> +{
> +	struct sk_buff *skb;
> +	struct hci_rp_read_local_supported_codecs *rp;
> +	struct hci_standard_codecs *std_codecs;
> +	struct hci_vendor_codecs *vendor_codecs;
> +
> +	skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODECS, 0, NULL,
> +			     HCI_CMD_TIMEOUT);
> +
> +	if (IS_ERR(skb)) {
> +		bt_dev_err(hdev, "Failed to read local supported codecs (%ld)",
> +			   PTR_ERR(skb));
> +		return;
> +	}
> +
> +	if (skb->len < sizeof(*rp))
> +		goto error;
> +
> +	rp = (void *)skb->data;
> +
> +	if (rp->status)
> +		goto error;
> +
> +	skb_pull(skb, sizeof(rp->status));
> +
> +	std_codecs = (void *)skb->data;
> +
> +	if (skb->len < flex_array_size(std_codecs, codec, std_codecs->num)
> +	    + sizeof(std_codecs->num))
> +		goto error;
> +
> +	hci_codec_list_parse(hdev, std_codecs->num, std_codecs, false);
> +
> +	skb_pull(skb, flex_array_size(std_codecs, codec, std_codecs->num)
> +		 + sizeof(std_codecs->num));
> +
> +	vendor_codecs = (void *)skb->data;
> +
> +	if (skb->len <
> +	    flex_array_size(vendor_codecs, codec, vendor_codecs->num)
> +	    + sizeof(vendor_codecs->num))
> +		goto error;

This part would benefit from some simple comments on what is done.

> +
> +	hci_codec_list_parse(hdev, vendor_codecs->num, vendor_codecs, true);
> +
> +error:
> +	kfree_skb(skb);
> +}
> +
> static int __hci_init(struct hci_dev *hdev)
> {
> 	int err;
> @@ -937,6 +1086,10 @@ static int __hci_init(struct hci_dev *hdev)
> 	if (err < 0)
> 		return err;
> 
> +	/* Read local codec list if the HCI command is supported */
> +	if (hdev->commands[29] & 0x20)
> +		hci_read_supported_codecs(hdev);
> +
> 	/* This function is only called when the controller is actually in
> 	 * configured state. When the controller is marked as unconfigured,
> 	 * this initialization procedure is not run.
> @@ -1836,6 +1989,7 @@ int hci_dev_do_close(struct hci_dev *hdev)
> 	memset(hdev->eir, 0, sizeof(hdev->eir));
> 	memset(hdev->dev_class, 0, sizeof(hdev->dev_class));
> 	bacpy(&hdev->random_addr, BDADDR_ANY);
> +	hci_codec_list_clear(&hdev->local_codecs);
> 
> 	hci_req_sync_unlock(hdev);
> 
> @@ -3837,6 +3991,7 @@ struct hci_dev *hci_alloc_dev(void)
> 	INIT_LIST_HEAD(&hdev->conn_hash.list);
> 	INIT_LIST_HEAD(&hdev->adv_instances);
> 	INIT_LIST_HEAD(&hdev->blocked_keys);
> +	INIT_LIST_HEAD(&hdev->local_codecs);
> 
> 	INIT_WORK(&hdev->rx_work, hci_rx_work);
> 	INIT_WORK(&hdev->cmd_work, hci_cmd_work);

Regards

Marcel


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH v7 1/2] Bluetooth: enumerate local supported codec and cache details
  2021-05-13 15:30 ` [PATCH v7 1/2] " Marcel Holtmann
@ 2021-05-17 12:57   ` K, Kiran
  0 siblings, 0 replies; 5+ messages in thread
From: K, Kiran @ 2021-05-17 12:57 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: linux-bluetooth, Tumkur Narayan, Chethan, Srivatsa, Ravishankar

Hi Marcel,

> > #define HCI_OP_READ_LOCAL_CODECS	0x100b
> > +struct hci_standard_codecs {
> > +	__u8	num;
> > +	__u8	codec[];
> > +} __packed;
> > +
> > +struct hci_vendor_codec {
> > +	__le16	company_id;
> > +	__le16	codec_id;
> > +} __packed;
> > +
> > +struct hci_vendor_codecs {
> > +	__u8	num;
> > +	struct hci_vendor_codec codec[];
> > +} __packed;
> > +
> > +struct hci_rp_read_local_supported_codecs {
> > +	__u8	status;
> > +	struct hci_standard_codecs std_codecs;
> > +	struct hci_vendor_codecs vendor_codecs; } __packed;
> 
> I am really trying to understand this business, but my brain keeps failing me.
> I really don’t know how the compiler should work this out. So have we tested
> this with btvirt returning some random vendor codecs anyway.
> 
Yes. I made changes in btvirt to send random vendor codec and it seems to be working as expected.  sizeof(struct hci_rp_read_local_supported_codecs) is giving 3 bytes where in status, num_std_codecs, num_ven_codecs fields are fixed  and it looks compiler is not accounting the size of any array member with empty / unspecified size.

> And on a side note, I rather have std_codec and vnd_codec here as naming
> to avoid long names like standard and vendor that are used to often that
> abbreviations are fine.

Ack

> 
> >
> > #define HCI_OP_READ_LOCAL_PAIRING_OPTS	0x100c
> > struct hci_rp_read_local_pairing_opts { @@ -1315,6 +1335,24 @@ struct
> > hci_rp_read_local_pairing_opts {
> > 	__u8     max_key_size;
> > } __packed;
> >
> > +#define HCI_OP_READ_LOCAL_CODEC_CAPS	0x100e
> > +struct hci_op_read_local_codec_caps {
> > +	__u8	codec_id[5];
> > +	__u8	transport;
> > +	__u8	direction;
> > +} __packed;
> > +
> > +struct hci_codec_caps {
> > +	__u8	len;
> > +	__u8	caps[];
> > +} __packed;
> > +
> > +struct hci_rp_read_local_codec_caps {
> > +	__u8	status;
> > +	__u8	num_caps;
> > +	struct hci_codec_caps caps[];
> > +} __packed;
> > +
> 
> This is another one that I do not get. Frankly the structure can just end at
> num_caps and we just parse the rest one by one.

Ack

> 
> > #define HCI_OP_READ_PAGE_SCAN_ACTIVITY	0x0c1b
> > struct hci_rp_read_page_scan_activity {
> > 	__u8     status;
> > diff --git a/include/net/bluetooth/hci_core.h
> > b/include/net/bluetooth/hci_core.h
> > index 43b08bebae74..d6d0a535a82a 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -131,6 +131,14 @@ struct bdaddr_list {
> > 	u8 bdaddr_type;
> > };
> >
> > +struct codec_list {
> > +	struct list_head list;
> > +	u8	transport;
> > +	u8	codec_id[5];
> > +	u8	num_caps;
> > +	struct hci_codec_caps caps[];
> > +};
> > +
> > struct bdaddr_list_with_irk {
> > 	struct list_head list;
> > 	bdaddr_t bdaddr;
> > @@ -535,6 +543,7 @@ struct hci_dev {
> > 	struct list_head	pend_le_conns;
> > 	struct list_head	pend_le_reports;
> > 	struct list_head	blocked_keys;
> > +	struct list_head	local_codecs;
> >
> > 	struct hci_dev_stats	stat;
> >
> > @@ -1849,4 +1858,9 @@ void hci_copy_identity_address(struct hci_dev
> *hdev, bdaddr_t *bdaddr,
> > #define SCO_AIRMODE_CVSD       0x0000
> > #define SCO_AIRMODE_TRANSP     0x0003
> >
> > +#define LOCAL_CODEC_ACL_MASK	BIT(0)
> > +#define LOCAL_CODEC_SCO_MASK	BIT(1)
> > +
> > +#define TRANSPORT_TYPE_MAX	0x04
> > +
> > #endif /* __HCI_CORE_H */
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index
> > 7baf93eda936..50947a1ed6a9 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -838,10 +838,6 @@ static int hci_init4_req(struct hci_request *req,
> unsigned long opt)
> > 	if (hdev->commands[22] & 0x04)
> > 		hci_set_event_mask_page_2(req);
> >
> > -	/* Read local codec list if the HCI command is supported */
> > -	if (hdev->commands[29] & 0x20)
> > -		hci_req_add(req, HCI_OP_READ_LOCAL_CODECS, 0, NULL);
> > -
> > 	/* Read local pairing options if the HCI command is supported */
> > 	if (hdev->commands[41] & 0x08)
> > 		hci_req_add(req, HCI_OP_READ_LOCAL_PAIRING_OPTS, 0,
> NULL); @@ -907,6
> > +903,159 @@ static int hci_init4_req(struct hci_request *req, unsigned
> long opt)
> > 	return 0;
> > }
> >
> > +static int hci_codec_list_add(struct list_head *list,
> > +			      struct hci_rp_read_local_codec_caps *rp,
> > +			      __u32 len,
> > +			      struct hci_op_read_local_codec_caps *sent) {
> > +	struct codec_list *entry;
> > +
> > +	entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL);
> > +	if (!entry)
> > +		return -ENOMEM;
> > +
> > +	memcpy(entry->codec_id, sent->codec_id, 5);
> > +	entry->transport = sent->transport;
> > +	entry->num_caps = rp->num_caps;
> > +	if (rp->num_caps)
> > +		memcpy(entry->caps, rp->caps, len);
> > +	list_add(&entry->list, list);
> > +
> > +	return 0;
> > +}
> > +
> > +static void hci_codec_list_clear(struct list_head *codec_list) {
> > +	struct codec_list *c, *n;
> > +
> > +	list_for_each_entry_safe(c, n, codec_list, list) {
> > +		list_del(&c->list);
> > +		kfree(c);
> > +	}
> > +}
> > +
> > +static void hci_read_codec_capabilities(struct hci_dev *hdev, void
> *codec_id,
> > +					__u8 transport, bool
> is_vendor_codec) {
> > +	struct hci_op_read_local_codec_caps caps;
> > +	__u8 i;
> > +
> > +	memset(&caps, 0, sizeof(caps));
> > +
> > +	if (is_vendor_codec) {
> > +		caps.codec_id[0] = 0xFF;
> > +		memcpy(&caps.codec_id[1], codec_id, 4);
> > +	} else {
> > +		memcpy(caps.codec_id, codec_id, 1);
> > +	}
> 
> This business might work, but I am not a big fan. There is most likely luck
> here that what you receive from the wire gets send back and thus with luck
> this will also work on big endian, but it is rather unclean.
> 
> When reading this patch, the business of is_vendor_codec seems to
> introduce a complexity without any benefit. Does it really make the code
> smaller or are we just trying to hard to unify standard codecs with vendor
> codecs.

Except for copying codec id, rest of the code in this function remains same for std_codec and ven_codec. Thus function is also reused in v7 2/2. I am OK to change if you think this can be done in a much cleaner way. Please suggest. 
> 
> > +
> > +	caps.direction = 0x00;
> > +
> > +	for (i = 0; i < TRANSPORT_TYPE_MAX; i++) {
> > +		if (transport & BIT(i)) {
> > +			struct hci_rp_read_local_codec_caps *rp;
> > +			struct sk_buff *skb;
> > +
> > +			caps.transport = i;
> > +			skb = __hci_cmd_sync(hdev,
> HCI_OP_READ_LOCAL_CODEC_CAPS,
> > +					     sizeof(caps), &caps,
> > +					     HCI_CMD_TIMEOUT);
> > +			if (IS_ERR(skb)) {
> > +				bt_dev_err(hdev, "Failed to read codec
> capabilities (%ld)",
> > +					   PTR_ERR(skb));
> > +				continue;
> > +			}
> > +
> > +			if (skb->len < sizeof(*rp))
> > +				goto error;
> > +
> > +			rp = (void *)skb->data;
> > +
> > +			if (rp->status)
> > +				goto error;
> > +
> > +			hci_dev_lock(hdev);
> > +			hci_codec_list_add(&hdev->local_codecs, rp, skb-
> >len - 2,
> > +					   &caps);
> > +			hci_dev_unlock(hdev);
> > +error:
> > +			kfree_skb(skb);
> > +		}
> > +	}
> > +}
> > +
> > +static void hci_codec_list_parse(struct hci_dev *hdev, __u8 num_codecs,
> > +				 void *codec_list, bool is_vendor_codec) {
> > +	__u8 i;
> > +
> > +	for (i = 0; i < num_codecs; i++) {
> > +		if (!is_vendor_codec) {
> > +			struct hci_standard_codecs *codecs = codec_list;
> > +
> > +			hci_read_codec_capabilities(hdev, &codecs-
> >codec[i],
> > +						    LOCAL_CODEC_ACL_MASK,
> > +						    is_vendor_codec);
> > +		} else {
> > +			struct hci_vendor_codecs *codecs = codec_list;
> > +
> > +			hci_read_codec_capabilities(hdev, &codecs-
> >codec[i],
> > +						    LOCAL_CODEC_ACL_MASK,
> > +						    is_vendor_codec);
> > +		}
> > +	}
> > +}
> > +
> > +static void hci_read_supported_codecs(struct hci_dev *hdev) {
> > +	struct sk_buff *skb;
> > +	struct hci_rp_read_local_supported_codecs *rp;
> > +	struct hci_standard_codecs *std_codecs;
> > +	struct hci_vendor_codecs *vendor_codecs;
> > +
> > +	skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODECS, 0, NULL,
> > +			     HCI_CMD_TIMEOUT);
> > +
> > +	if (IS_ERR(skb)) {
> > +		bt_dev_err(hdev, "Failed to read local supported codecs
> (%ld)",
> > +			   PTR_ERR(skb));
> > +		return;
> > +	}
> > +
> > +	if (skb->len < sizeof(*rp))
> > +		goto error;
> > +
> > +	rp = (void *)skb->data;
> > +
> > +	if (rp->status)
> > +		goto error;
> > +
> > +	skb_pull(skb, sizeof(rp->status));
> > +
> > +	std_codecs = (void *)skb->data;
> > +
> > +	if (skb->len < flex_array_size(std_codecs, codec, std_codecs->num)
> > +	    + sizeof(std_codecs->num))
> > +		goto error;
> > +
> > +	hci_codec_list_parse(hdev, std_codecs->num, std_codecs, false);
> > +
> > +	skb_pull(skb, flex_array_size(std_codecs, codec, std_codecs->num)
> > +		 + sizeof(std_codecs->num));
> > +
> > +	vendor_codecs = (void *)skb->data;
> > +
> > +	if (skb->len <
> > +	    flex_array_size(vendor_codecs, codec, vendor_codecs->num)
> > +	    + sizeof(vendor_codecs->num))
> > +		goto error;
> 
> This part would benefit from some simple comments on what is done.

Ack
> 
> > +
> > +	hci_codec_list_parse(hdev, vendor_codecs->num, vendor_codecs,
> true);
> > +
> > +error:
> > +	kfree_skb(skb);
> > +}
> > +
> > static int __hci_init(struct hci_dev *hdev) {
> > 	int err;
> > @@ -937,6 +1086,10 @@ static int __hci_init(struct hci_dev *hdev)
> > 	if (err < 0)
> > 		return err;
> >
> > +	/* Read local codec list if the HCI command is supported */
> > +	if (hdev->commands[29] & 0x20)
> > +		hci_read_supported_codecs(hdev);
> > +
> > 	/* This function is only called when the controller is actually in
> > 	 * configured state. When the controller is marked as unconfigured,
> > 	 * this initialization procedure is not run.
> > @@ -1836,6 +1989,7 @@ int hci_dev_do_close(struct hci_dev *hdev)
> > 	memset(hdev->eir, 0, sizeof(hdev->eir));
> > 	memset(hdev->dev_class, 0, sizeof(hdev->dev_class));
> > 	bacpy(&hdev->random_addr, BDADDR_ANY);
> > +	hci_codec_list_clear(&hdev->local_codecs);
> >
> > 	hci_req_sync_unlock(hdev);
> >
> > @@ -3837,6 +3991,7 @@ struct hci_dev *hci_alloc_dev(void)
> > 	INIT_LIST_HEAD(&hdev->conn_hash.list);
> > 	INIT_LIST_HEAD(&hdev->adv_instances);
> > 	INIT_LIST_HEAD(&hdev->blocked_keys);
> > +	INIT_LIST_HEAD(&hdev->local_codecs);
> >
> > 	INIT_WORK(&hdev->rx_work, hci_rx_work);
> > 	INIT_WORK(&hdev->cmd_work, hci_cmd_work);
> 
> Regards
> 
> Marcel

I will address the above comments and also publish few more patches related to HFP offload in the next patchset.

Thanks,
Kiran
 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-05-17 12:58 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07 11:42 [PATCH v7 1/2] Bluetooth: enumerate local supported codec and cache details Kiran K
2021-05-07 11:42 ` [PATCH v7 2/2] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
2021-05-07 12:08 ` [v7,1/2] Bluetooth: enumerate local supported codec and cache details bluez.test.bot
2021-05-13 15:30 ` [PATCH v7 1/2] " Marcel Holtmann
2021-05-17 12:57   ` K, Kiran

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.