linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details
@ 2021-07-27  8:47 Kiran K
  2021-07-27  8:47 ` [PATCH v11 02/10] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
                   ` (10 more replies)
  0 siblings, 11 replies; 25+ messages in thread
From: Kiran K @ 2021-07-27  8:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: ravishankar.srivatsa, chethan.tumkur.narayan, Kiran K

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>
---
* changes in v11:
 - Remove Kconfig related changes
 - Address minor review comments
 - Move codec related functions to new file hci_codec.c

* changes in v10:
 - define Kconfig to control offload feature at build time
 - fix review comments

* changes in v9:
  - use shortname vnd instead of ven

* changes in v8:
  - add comments
  - split __u8 codec_id[5] into {__u8 id; __le16 cid, vid }
  - address review comment related codec caps structure

* 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      |  41 +++++++
 include/net/bluetooth/hci_core.h |  17 +++
 net/bluetooth/Makefile           |   2 +-
 net/bluetooth/hci_codec.c        | 194 +++++++++++++++++++++++++++++++
 net/bluetooth/hci_codec.h        |   6 +
 net/bluetooth/hci_core.c         |  12 +-
 6 files changed, 266 insertions(+), 6 deletions(-)
 create mode 100644 net/bluetooth/hci_codec.c
 create mode 100644 net/bluetooth/hci_codec.h

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index b80415011dcd..f76849c8eafd 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1307,6 +1307,28 @@ struct hci_rp_read_data_block_size {
 } __packed;
 
 #define HCI_OP_READ_LOCAL_CODECS	0x100b
+struct hci_std_codecs {
+	__u8	num;
+	__u8	codec[];
+} __packed;
+
+struct hci_vnd_codec {
+	/* company id */
+	__le16	cid;
+	/* vendor codec id */
+	__le16	vid;
+} __packed;
+
+struct hci_vnd_codecs {
+	__u8	num;
+	struct hci_vnd_codec codec[];
+} __packed;
+
+struct hci_rp_read_local_supported_codecs {
+	__u8	status;
+	struct hci_std_codecs std_codecs;
+	struct hci_vnd_codecs vnd_codecs;
+} __packed;
 
 #define HCI_OP_READ_LOCAL_PAIRING_OPTS	0x100c
 struct hci_rp_read_local_pairing_opts {
@@ -1315,6 +1337,25 @@ 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	id;
+	__le16	cid;
+	__le16	vid;
+	__u8	transport;
+	__u8	direction;
+} __packed;
+
+struct hci_codec_caps {
+	__u8	len;
+	__u8	data[];
+} __packed;
+
+struct hci_rp_read_local_codec_caps {
+	__u8	status;
+	__u8	num_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 a53e94459ecd..6742e6ad7b37 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -131,6 +131,17 @@ struct bdaddr_list {
 	u8 bdaddr_type;
 };
 
+struct codec_list {
+	struct list_head list;
+	u8	id;
+	__u16	cid;
+	__u16	vid;
+	u8	transport;
+	u8	num_caps;
+	u32	len;
+	struct hci_codec_caps caps[];
+};
+
 struct bdaddr_list_with_irk {
 	struct list_head list;
 	bdaddr_t bdaddr;
@@ -535,6 +546,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 +1861,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/Makefile b/net/bluetooth/Makefile
index cc0995301f93..f3e439d98b85 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -14,7 +14,7 @@ bluetooth_6lowpan-y := 6lowpan.o
 
 bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \
 	hci_sock.o hci_sysfs.o l2cap_core.o l2cap_sock.o smp.o lib.o \
-	ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o
+	ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o hci_codec.o
 
 bluetooth-$(CONFIG_BT_BREDR) += sco.o
 bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
diff --git a/net/bluetooth/hci_codec.c b/net/bluetooth/hci_codec.c
new file mode 100644
index 000000000000..205f3b04c172
--- /dev/null
+++ b/net/bluetooth/hci_codec.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021 Intel Corporation */
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+static int hci_codec_list_add(struct list_head *list,
+			      struct hci_op_read_local_codec_caps *sent,
+			      struct hci_rp_read_local_codec_caps *rp,
+			      void *caps,
+			      __u32 len)
+{
+	struct codec_list *entry;
+
+	entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	entry->id = sent->id;
+	if (sent->id == 0xFF) {
+		entry->cid = __le16_to_cpu(sent->cid);
+		entry->vid = __le16_to_cpu(sent->vid);
+	}
+	entry->transport = sent->transport;
+	entry->len = len;
+	entry->num_caps = rp->num_caps;
+	if (rp->num_caps)
+		memcpy(entry->caps, caps, len);
+	list_add(&entry->list, list);
+
+	return 0;
+}
+
+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_vnd_codec)
+{
+	struct hci_op_read_local_codec_caps cmd;
+	__u8 i;
+
+	memset(&cmd, 0, sizeof(cmd));
+
+	if (is_vnd_codec) {
+		struct hci_vnd_codec *vnd_codec;
+
+		vnd_codec = codec_id;
+		cmd.id = 0xFF;
+		cmd.cid = vnd_codec->cid;
+		cmd.vid = vnd_codec->vid;
+	} else {
+		cmd.id = *(__u8 *)codec_id;
+	}
+
+	cmd.direction = 0x00;
+
+	for (i = 0; i < TRANSPORT_TYPE_MAX; i++) {
+		if (transport & BIT(i)) {
+			struct hci_rp_read_local_codec_caps *rp;
+			struct hci_codec_caps *caps;
+			struct sk_buff *skb;
+			__u8 j;
+			__u32 len;
+
+			cmd.transport = i;
+			skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODEC_CAPS,
+					     sizeof(cmd), &cmd,
+					     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;
+
+			if (!rp->num_caps) {
+				len = 0;
+				/* this codec doesn't have capabilities */
+				goto skip_caps_parse;
+			}
+
+			skb_pull(skb, sizeof(*rp));
+
+			for (j = 0, len = 0; j < rp->num_caps; j++) {
+				caps = (void *)skb->data;
+				if (skb->len < sizeof(*caps))
+					goto error;
+				if (skb->len < caps->len)
+					goto error;
+				len += sizeof(caps->len) + caps->len;
+				skb_pull(skb,  sizeof(caps->len) + caps->len);
+			}
+
+skip_caps_parse:
+			hci_dev_lock(hdev);
+			hci_codec_list_add(&hdev->local_codecs, &cmd, rp,
+					   (__u8 *)rp + sizeof(*rp), len);
+			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_vnd_codec)
+{
+	__u8 i;
+
+	for (i = 0; i < num_codecs; i++) {
+		if (!is_vnd_codec) {
+			struct hci_std_codecs *codecs = codec_list;
+
+			hci_read_codec_capabilities(hdev, &codecs->codec[i],
+						    LOCAL_CODEC_ACL_MASK,
+						    is_vnd_codec);
+		} else {
+			struct hci_vnd_codecs *codecs = codec_list;
+
+			hci_read_codec_capabilities(hdev, &codecs->codec[i],
+						    LOCAL_CODEC_ACL_MASK,
+						    is_vnd_codec);
+		}
+	}
+}
+
+void hci_read_supported_codecs(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	struct hci_rp_read_local_supported_codecs *rp;
+	struct hci_std_codecs *std_codecs;
+	struct hci_vnd_codecs *vnd_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;
+
+	/* validate codecs length before accessing */
+	if (skb->len < flex_array_size(std_codecs, codec, std_codecs->num)
+	    + sizeof(std_codecs->num))
+		goto error;
+
+	/* enumerate codec capabilities of standard codecs */
+	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));
+
+	vnd_codecs = (void *)skb->data;
+
+	/* validate vendor codecs length before accessing */
+	if (skb->len <
+	    flex_array_size(vnd_codecs, codec, vnd_codecs->num)
+	    + sizeof(vnd_codecs->num))
+		goto error;
+
+	/* enumerate vendor codec capabilities */
+	hci_codec_list_parse(hdev, vnd_codecs->num, vnd_codecs, true);
+
+error:
+	kfree_skb(skb);
+}
diff --git a/net/bluetooth/hci_codec.h b/net/bluetooth/hci_codec.h
new file mode 100644
index 000000000000..e5e594933d07
--- /dev/null
+++ b/net/bluetooth/hci_codec.h
@@ -0,0 +1,6 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/* Copyright (C) 2021 Intel Corporation */
+
+void hci_read_supported_codecs(struct hci_dev *hdev);
+void hci_codec_list_clear(struct list_head *codec_list);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 2560ed2f144d..212d5066d413 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -45,6 +45,7 @@
 #include "leds.h"
 #include "msft.h"
 #include "aosp.h"
+#include "hci_codec.h"
 
 static void hci_rx_work(struct work_struct *work);
 static void hci_cmd_work(struct work_struct *work);
@@ -838,10 +839,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);
@@ -937,6 +934,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.
@@ -1841,6 +1842,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);
 
@@ -3842,7 +3844,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);
 	INIT_WORK(&hdev->tx_work, hci_tx_work);
-- 
2.17.1


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

* [PATCH v11 02/10] Bluetooth: Add support for Read Local Supported Codecs V2
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
@ 2021-07-27  8:47 ` Kiran K
  2021-07-30 14:07   ` Marcel Holtmann
  2021-07-27  8:47 ` [PATCH v11 03/10] Bluetooth: btintel: Read supported offload usecases Kiran K
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 25+ messages in thread
From: Kiran K @ 2021-07-27  8:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: ravishankar.srivatsa, chethan.tumkur.narayan, Kiran K

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 v11:
  - Remove Kconfig related changes
  - Move codec related functions to hci_codec.c

* changes in v10:
  no changes

* changes in v9:
  use vnd as shortcut name for vendor instead of ven

* changes in v8:
  no changes

* 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 | 29 +++++++++++++++
 net/bluetooth/hci_codec.c   | 74 +++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_codec.h   |  1 +
 net/bluetooth/hci_core.c    |  4 +-
 4 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index f76849c8eafd..b4e35cf5f4b1 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1337,6 +1337,35 @@ struct hci_rp_read_local_pairing_opts {
 	__u8     max_key_size;
 } __packed;
 
+#define HCI_OP_READ_LOCAL_CODECS_V2	0x100d
+struct hci_std_codec_v2 {
+	__u8	id;
+	__u8	transport;
+} __packed;
+
+struct hci_std_codecs_v2 {
+	__u8	num;
+	struct hci_std_codec_v2 codec[];
+} __packed;
+
+struct hci_vnd_codec_v2 {
+	__u8	id;
+	__le16	cid;
+	__le16	vid;
+	__u8	transport;
+} __packed;
+
+struct hci_vnd_codecs_v2 {
+	__u8	num;
+	struct hci_vnd_codec_v2 codec[];
+} __packed;
+
+struct hci_rp_read_local_supported_codecs_v2 {
+	__u8	status;
+	struct hci_std_codecs_v2 std_codecs;
+	struct hci_vnd_codecs_v2 vendor_codecs;
+} __packed;
+
 #define HCI_OP_READ_LOCAL_CODEC_CAPS	0x100e
 struct hci_op_read_local_codec_caps {
 	__u8	id;
diff --git a/net/bluetooth/hci_codec.c b/net/bluetooth/hci_codec.c
index 205f3b04c172..6f20a4b1fc9c 100644
--- a/net/bluetooth/hci_codec.c
+++ b/net/bluetooth/hci_codec.c
@@ -192,3 +192,77 @@ void hci_read_supported_codecs(struct hci_dev *hdev)
 error:
 	kfree_skb(skb);
 }
+
+static void hci_codec_list_parse_v2(struct hci_dev *hdev, __u8 num_codecs,
+				    void *codec_list, bool is_vnd_codec)
+{
+	__u8 i;
+
+	for (i = 0; i < num_codecs; i++) {
+		if (!is_vnd_codec) {
+			struct hci_std_codecs_v2 *codecs = codec_list;
+
+			hci_read_codec_capabilities(hdev, &codecs->codec[i],
+						    codecs->codec[i].transport,
+						    is_vnd_codec);
+		} else {
+			struct hci_vnd_codecs_v2 *codecs = codec_list;
+
+			hci_read_codec_capabilities(hdev, &codecs->codec[i],
+						    codecs->codec[i].transport,
+						    is_vnd_codec);
+		}
+	}
+}
+
+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_std_codecs_v2 *std_codecs;
+	struct hci_vnd_codecs_v2 *vnd_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;
+
+	/* check for payload data length before accessing */
+	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));
+
+	vnd_codecs = (void *)skb->data;
+
+	/* check for payload data length before accessing */
+	if (skb->len <
+	    flex_array_size(vnd_codecs, codec, vnd_codecs->num)
+	    + sizeof(vnd_codecs->num))
+		goto error;
+
+	hci_codec_list_parse_v2(hdev, vnd_codecs->num, vnd_codecs, true);
+
+error:
+	kfree_skb(skb);
+}
diff --git a/net/bluetooth/hci_codec.h b/net/bluetooth/hci_codec.h
index e5e594933d07..899e5dda801f 100644
--- a/net/bluetooth/hci_codec.h
+++ b/net/bluetooth/hci_codec.h
@@ -3,4 +3,5 @@
 /* Copyright (C) 2021 Intel Corporation */
 
 void hci_read_supported_codecs(struct hci_dev *hdev);
+void hci_read_supported_codecs_v2(struct hci_dev *hdev);
 void hci_codec_list_clear(struct list_head *codec_list);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 212d5066d413..513eae743d2f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -935,7 +935,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] 25+ messages in thread

* [PATCH v11 03/10] Bluetooth: btintel: Read supported offload usecases
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
  2021-07-27  8:47 ` [PATCH v11 02/10] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
@ 2021-07-27  8:47 ` Kiran K
  2021-07-30 14:10   ` Marcel Holtmann
  2021-07-27  8:47 ` [PATCH v11 04/10] Bluetooth: Allow querying of supported offload codecs over SCO socket Kiran K
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 25+ messages in thread
From: Kiran K @ 2021-07-27  8:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: ravishankar.srivatsa, chethan.tumkur.narayan, Kiran K

Read offload usecases supported by controller.

Signed-off-by: Kiran K <kiran.k@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
* changes in v11:
  - Remove Kconfig related changes
* changes in v10:
  - restructure patch to have  definition and call of callaback in the
    same patch
* changes in v9:
  - define a separate patch for core changes

 drivers/bluetooth/btintel.c | 30 ++++++++++++++++++++++++++++++
 drivers/bluetooth/btintel.h | 10 ++++++++++
 drivers/bluetooth/btusb.c   |  3 +++
 3 files changed, 43 insertions(+)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index e44b6993cf91..a6b81914766e 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -1272,6 +1272,36 @@ int btintel_set_debug_features(struct hci_dev *hdev,
 }
 EXPORT_SYMBOL_GPL(btintel_set_debug_features);
 
+int btintel_configure_offload_usecases(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	int err = 0;
+	struct intel_offload_usecases *usecases;
+
+	skb = __hci_cmd_sync(hdev, 0xfc86, 0, NULL, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "Reading offload usecases failed (%ld)",
+			   PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+
+	if (skb->len < sizeof(*usecases)) {
+		err = -EIO;
+		goto error;
+	}
+
+	usecases = (void *)skb->data;
+
+	if (usecases->status) {
+		err = -bt_to_errno(skb->data[0]);
+		goto error;
+	}
+error:
+	kfree_skb(skb);
+	return err;
+}
+EXPORT_SYMBOL_GPL(btintel_configure_offload_usecases);
+
 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 d184064a5e7c..45ca5c4ccf37 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -132,6 +132,11 @@ struct intel_debug_features {
 	__u8    page1[16];
 } __packed;
 
+struct intel_offload_usecases {
+	__u8	status;
+	__u8	preset[8];
+} __packed;
+
 #define INTEL_HW_PLATFORM(cnvx_bt)	((u8)(((cnvx_bt) & 0x0000ff00) >> 8))
 #define INTEL_HW_VARIANT(cnvx_bt)	((u8)(((cnvx_bt) & 0x003f0000) >> 16))
 #define INTEL_CNVX_TOP_TYPE(cnvx_top)	((cnvx_top) & 0x00000fff)
@@ -175,6 +180,7 @@ int btintel_read_debug_features(struct hci_dev *hdev,
 				struct intel_debug_features *features);
 int btintel_set_debug_features(struct hci_dev *hdev,
 			       const struct intel_debug_features *features);
+int btintel_configure_offload_usecases(struct hci_dev *hdev);
 #else
 
 static inline int btintel_check_bdaddr(struct hci_dev *hdev)
@@ -307,4 +313,8 @@ static inline int btintel_set_debug_features(struct hci_dev *hdev,
 	return -EOPNOTSUPP;
 }
 
+static int btintel_configure_offload_usecases(struct hci_dev *hdev)
+{
+	return -EOPNOTSUPP;
+}
 #endif
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 1876a960b3dc..0ff8d1288929 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3023,6 +3023,9 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
 		btintel_set_debug_features(hdev, &features);
 	}
 
+	/* Read supported usecases and set callbacks to fetch datapath id */
+	btintel_configure_offload_usecases(hdev);
+
 	/* Read the Intel version information after loading the FW  */
 	err = btintel_read_version_tlv(hdev, &version);
 	if (err)
-- 
2.17.1


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

* [PATCH v11 04/10] Bluetooth: Allow querying of supported offload codecs over SCO socket
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
  2021-07-27  8:47 ` [PATCH v11 02/10] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
  2021-07-27  8:47 ` [PATCH v11 03/10] Bluetooth: btintel: Read supported offload usecases Kiran K
@ 2021-07-27  8:47 ` Kiran K
  2021-07-30 14:14   ` Marcel Holtmann
  2021-07-27  8:47 ` [PATCH v11 05/10] Bluetooth: btintel: Define callback to fetch data_path_id Kiran K
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 25+ messages in thread
From: Kiran K @ 2021-07-27  8:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: ravishankar.srivatsa, chethan.tumkur.narayan, Kiran K

Add BT_CODEC option for getsockopt systemcall to get the details
of offload codecs supported over SCO socket

Signed-off-by: Kiran K <kiran.k@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
* changes in v11:
  - Remove Kconfig related changes
* changes in v10:
  - In getsockopt, prepare data in advance and call copy_to_user
    istead of calling put_user on each field of struct bt_codec

 include/net/bluetooth/bluetooth.h | 20 +++++++
 include/net/bluetooth/hci.h       |  4 ++
 include/net/bluetooth/hci_core.h  |  1 +
 net/bluetooth/sco.c               | 91 +++++++++++++++++++++++++++++++
 4 files changed, 116 insertions(+)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 9125effbf448..64cddff0c9c4 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -153,6 +153,26 @@ struct bt_voice {
 
 #define BT_SCM_PKT_STATUS	0x03
 
+#define BT_CODEC	19
+
+struct	bt_codec_caps {
+	__u8	len;
+	__u8	data[];
+} __packed;
+
+struct bt_codec {
+	__u8	id;
+	__u16	cid;
+	__u16	vid;
+	__u8	data_path;
+	__u8	num_caps;
+} __packed;
+
+struct bt_codecs {
+	__u8		num_codecs;
+	struct bt_codec	codecs[];
+} __packed;
+
 __printf(1, 2)
 void bt_info(const char *fmt, ...);
 __printf(1, 2)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index b4e35cf5f4b1..22e872e60ff8 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -2621,6 +2621,10 @@ static inline struct hci_sco_hdr *hci_sco_hdr(const struct sk_buff *skb)
 #define hci_iso_data_len(h)		((h) & 0x3fff)
 #define hci_iso_data_flags(h)		((h) >> 14)
 
+/* codec transport types */
+#define TRANSPORT_ACL		0x00
+#define TRANSPORT_SCO_ESCO	0x01
+
 /* le24 support */
 static inline void hci_cpu_to_le24(__u32 val, __u8 dst[3])
 {
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 6742e6ad7b37..71c409c8ab34 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -617,6 +617,7 @@ struct hci_dev {
 	int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
 	void (*cmd_timeout)(struct hci_dev *hdev);
 	bool (*prevent_wake)(struct hci_dev *hdev);
+	int (*get_data_path_id)(struct hci_dev *hdev, __u8 *data_path);
 };
 
 #define HCI_PHY_HANDLE(handle)	(handle & 0xff)
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index ffa2a77a3e4c..2f32693f09c1 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -953,6 +953,12 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 	struct bt_voice voice;
 	u32 phys;
 	int pkt_status;
+	int buf_len;
+	struct codec_list *c;
+	u8 num_codecs, i, __user *ptr;
+	struct hci_dev *hdev;
+	struct hci_codec_caps *caps;
+	struct bt_codec codec;
 
 	BT_DBG("sk %p", sk);
 
@@ -1017,6 +1023,91 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 			err = -EFAULT;
 		break;
 
+	case BT_CODEC:
+		num_codecs = 0;
+		buf_len = 0;
+
+		hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src, BDADDR_BREDR);
+		if (!hdev) {
+			err = -EBADFD;
+			break;
+		}
+
+		if (!hdev->get_data_path_id) {
+			err = -EOPNOTSUPP;
+			break;
+		}
+
+		/* find total buffer size required to copy codec + caps */
+		hci_dev_lock(hdev);
+		list_for_each_entry(c, &hdev->local_codecs, list) {
+			if (c->transport != TRANSPORT_SCO_ESCO)
+				continue;
+			num_codecs++;
+			for (i = 0, caps = c->caps; i < c->num_caps; i++) {
+				buf_len += 1 + caps->len;
+				caps = (void *)&caps->data[caps->len];
+			}
+			buf_len += sizeof(struct bt_codec);
+		}
+		hci_dev_unlock(hdev);
+
+		buf_len += sizeof(struct bt_codecs);
+		if (buf_len > len) {
+			err = -ENOBUFS;
+			break;
+		}
+		ptr = optval;
+
+		if (put_user(num_codecs, ptr)) {
+			err = -EFAULT;
+			break;
+		}
+		ptr += sizeof(num_codecs);
+
+		/* Iterate all the codecs supported over SCO and populate
+		 * codec data
+		 */
+		hci_dev_lock(hdev);
+		list_for_each_entry(c, &hdev->local_codecs, list) {
+			if (c->transport != TRANSPORT_SCO_ESCO)
+				continue;
+
+			codec.id = c->id;
+			codec.cid = c->cid;
+			codec.vid = c->vid;
+			err = hdev->get_data_path_id(hdev, &codec.data_path);
+			if (err < 0)
+				break;
+			codec.num_caps = c->num_caps;
+			if (copy_to_user(ptr, &codec, sizeof(codec))) {
+				err = -EFAULT;
+				break;
+			}
+			ptr += sizeof(codec);
+
+			/* find codec capabilities data length */
+			len = 0;
+			for (i = 0, caps = c->caps; i < c->num_caps; i++) {
+				len += 1 + caps->len;
+				caps = (void *)&caps->data[caps->len];
+			}
+
+			/* copy codec capabilities data */
+			if (len && copy_to_user(ptr, c->caps, len)) {
+				err = -EFAULT;
+				break;
+			}
+			ptr += len;
+		}
+
+		if (!err && put_user(buf_len, optlen))
+			err = -EFAULT;
+
+		hci_dev_unlock(hdev);
+
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
-- 
2.17.1


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

* [PATCH v11 05/10] Bluetooth: btintel: Define callback to fetch data_path_id
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
                   ` (2 preceding siblings ...)
  2021-07-27  8:47 ` [PATCH v11 04/10] Bluetooth: Allow querying of supported offload codecs over SCO socket Kiran K
@ 2021-07-27  8:47 ` Kiran K
  2021-07-27  8:47 ` [PATCH v11 06/10] Bluetooth: Allow setting of codec for HFP offload usecase Kiran K
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Kiran K @ 2021-07-27  8:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: ravishankar.srivatsa, chethan.tumkur.narayan, Kiran K

For Intel controllers supporting HFP offload usecase,
define a callback function to fetch data_path_id

Signed-off-by: Kiran K <kiran.k@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
* changes in v11:
  - no changes
* changes in v10:
  - new patch due to refactoring

 drivers/bluetooth/btintel.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index a6b81914766e..49813f2d2e27 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -1272,6 +1272,13 @@ int btintel_set_debug_features(struct hci_dev *hdev,
 }
 EXPORT_SYMBOL_GPL(btintel_set_debug_features);
 
+static int btintel_get_data_path_id(struct hci_dev *hdev, __u8 *data_path_id)
+{
+	/* Intel uses 1 as data path id for all the usecases */
+	*data_path_id = 1;
+	return 0;
+}
+
 int btintel_configure_offload_usecases(struct hci_dev *hdev)
 {
 	struct sk_buff *skb;
@@ -1296,6 +1303,9 @@ int btintel_configure_offload_usecases(struct hci_dev *hdev)
 		err = -bt_to_errno(skb->data[0]);
 		goto error;
 	}
+
+	if (usecases->preset[0] & 0x03)
+		hdev->get_data_path_id = btintel_get_data_path_id;
 error:
 	kfree_skb(skb);
 	return err;
-- 
2.17.1


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

* [PATCH v11 06/10] Bluetooth: Allow setting of codec for HFP offload usecase
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
                   ` (3 preceding siblings ...)
  2021-07-27  8:47 ` [PATCH v11 05/10] Bluetooth: btintel: Define callback to fetch data_path_id Kiran K
@ 2021-07-27  8:47 ` Kiran K
  2021-07-30 14:17   ` Marcel Holtmann
  2021-07-27  8:47 ` [PATCH v11 07/10] Bluetooth: btintel: Define a callback to fetch codec config data Kiran K
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 25+ messages in thread
From: Kiran K @ 2021-07-27  8:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: ravishankar.srivatsa, chethan.tumkur.narayan, Kiran K

For HFP offload usecase, controller needs to be configured
with codec data and capabilities. This patch uses Bluetooth
SIG defined command HCI_CONFIGURE_DATA_PATH to specify vendor
specific data and allows userspace modules to set the codec
via setsockopt systemcall.

Signed-off-by: Kiran K <kiran.k@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
* changes in v11:
  - Remove changes related to Kconfig
* changes in v10:
  - patch refactor - having callback definition and usage in the same patch

 include/net/bluetooth/bluetooth.h |   2 +
 include/net/bluetooth/hci.h       |   8 ++
 include/net/bluetooth/hci_core.h  |   3 +
 net/bluetooth/sco.c               | 118 ++++++++++++++++++++++++++++++
 4 files changed, 131 insertions(+)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 64cddff0c9c4..1a48b6732eef 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -173,6 +173,8 @@ struct bt_codecs {
 	struct bt_codec	codecs[];
 } __packed;
 
+#define CODING_FORMAT_CVSD	0x02
+
 __printf(1, 2)
 void bt_info(const char *fmt, ...);
 __printf(1, 2)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 22e872e60ff8..c998607fc517 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1250,6 +1250,14 @@ struct hci_rp_read_local_oob_ext_data {
 	__u8     rand256[16];
 } __packed;
 
+#define HCI_CONFIGURE_DATA_PATH	0x0c83
+struct hci_op_configure_data_path {
+	__u8	direction;
+	__u8	data_path_id;
+	__u8	vnd_len;
+	__u8	vnd_data[];
+} __packed;
+
 #define HCI_OP_READ_LOCAL_VERSION	0x1001
 struct hci_rp_read_local_version {
 	__u8     status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 71c409c8ab34..eafa6f8114cb 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -618,6 +618,9 @@ struct hci_dev {
 	void (*cmd_timeout)(struct hci_dev *hdev);
 	bool (*prevent_wake)(struct hci_dev *hdev);
 	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,
+				     __u8 **vnd_data);
 };
 
 #define HCI_PHY_HANDLE(handle)	(handle & 0xff)
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 2f32693f09c1..34541e971dc7 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -67,6 +67,7 @@ struct sco_pinfo {
 	__u32		flags;
 	__u16		setting;
 	__u8		cmsg_mask;
+	struct bt_codec codec;
 	struct sco_conn	*conn;
 };
 
@@ -438,6 +439,7 @@ static void __sco_sock_close(struct sock *sk)
 		sock_set_flag(sk, SOCK_ZAPPED);
 		break;
 	}
+
 }
 
 /* Must be called on unlocked socket. */
@@ -499,6 +501,10 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock,
 	sk->sk_state    = BT_OPEN;
 
 	sco_pi(sk)->setting = BT_VOICE_CVSD_16BIT;
+	sco_pi(sk)->codec.id = CODING_FORMAT_CVSD;
+	sco_pi(sk)->codec.cid = 0xffff;
+	sco_pi(sk)->codec.vid = 0xffff;
+	sco_pi(sk)->codec.data_path = 0x00;
 
 	timer_setup(&sk->sk_timer, sco_sock_timeout, 0);
 
@@ -807,6 +813,63 @@ static int sco_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 	return bt_sock_recvmsg(sock, msg, len, flags);
 }
 
+static int sco_configure_data_path(struct hci_dev *hdev, struct bt_codec *codec)
+{
+	__u8 vnd_len, *vnd_data = NULL;
+	struct hci_op_configure_data_path *cmd = NULL;
+	struct sk_buff *skb;
+	int err;
+
+	err = hdev->get_codec_config_data(hdev, SCO_LINK, codec, &vnd_len,
+					  &vnd_data);
+	if (err < 0)
+		goto error;
+
+	cmd = kzalloc(sizeof(*cmd) + vnd_len, GFP_KERNEL);
+	if (!cmd) {
+		err = -ENOMEM;
+		goto error;
+	}
+
+	err = hdev->get_data_path_id(hdev, &cmd->data_path_id);
+	if (err < 0)
+		goto error;
+
+	cmd->vnd_len = vnd_len;
+	memcpy(cmd->vnd_data, vnd_data, vnd_len);
+
+	cmd->direction = 0x00;
+	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
+			     sizeof(*cmd) + vnd_len,
+			     cmd, HCI_INIT_TIMEOUT);
+
+	if (IS_ERR(skb)) {
+		err = PTR_ERR(skb);
+		bt_dev_err(hdev, "configure input data path failed (%ld)",
+			   PTR_ERR(skb));
+		goto error;
+	}
+	kfree_skb(skb);
+
+	cmd->direction = 0x01;
+	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
+			     sizeof(*cmd) + vnd_len,
+			     cmd, HCI_INIT_TIMEOUT);
+
+	if (IS_ERR(skb)) {
+		err = PTR_ERR(skb);
+		bt_dev_err(hdev, "configure output data path failed (%ld)",
+			   PTR_ERR(skb));
+		goto error;
+	}
+	kfree_skb(skb);
+
+error:
+	kfree(vnd_data);
+	kfree(cmd);
+	return err;
+}
+
 static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 			       sockptr_t optval, unsigned int optlen)
 {
@@ -814,6 +877,9 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 	int len, err = 0;
 	struct bt_voice voice;
 	u32 opt;
+	struct bt_codecs *codecs;
+	struct hci_dev *hdev;
+	__u8 buffer[255];
 
 	BT_DBG("sk %p", sk);
 
@@ -875,6 +941,58 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 			sco_pi(sk)->cmsg_mask &= SCO_CMSG_PKT_STATUS;
 		break;
 
+	case BT_CODEC:
+		if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND &&
+		    sk->sk_state != BT_CONNECT2) {
+			err = -EINVAL;
+			break;
+		}
+
+		hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src,
+				     BDADDR_BREDR);
+		if (!hdev) {
+			err = -EBADFD;
+			break;
+		}
+
+		if (!hdev->get_codec_config_data) {
+			err = -EOPNOTSUPP;
+			break;
+		}
+
+		if (optlen < sizeof(struct bt_codecs) ||
+		    optlen > sizeof(buffer)) {
+			err = -EINVAL;
+			break;
+		}
+
+		if (copy_from_sockptr(buffer, optval, optlen)) {
+			err = -EFAULT;
+			break;
+		}
+
+		codecs = (void *)buffer;
+
+		if (codecs->num_codecs > 1) {
+			err = -EINVAL;
+			break;
+		}
+
+		/* configure data path only non-HCI data path is selected */
+		if (codecs->codecs[0].data_path) {
+			err = sco_configure_data_path(hdev, &codecs->codecs[0]);
+			if (err < 0)
+				break;
+
+			if (codecs->codecs[0].id == 0xff) {
+				sco_pi(sk)->codec.cid = codecs->codecs[0].cid;
+				sco_pi(sk)->codec.vid = codecs->codecs[0].vid;
+			}
+		}
+		sco_pi(sk)->codec.id = codecs->codecs[0].id;
+		sco_pi(sk)->codec.data_path = codecs->codecs[0].data_path;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
-- 
2.17.1


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

* [PATCH v11 07/10] Bluetooth: btintel: Define a callback to fetch codec config data
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
                   ` (4 preceding siblings ...)
  2021-07-27  8:47 ` [PATCH v11 06/10] Bluetooth: Allow setting of codec for HFP offload usecase Kiran K
@ 2021-07-27  8:47 ` Kiran K
  2021-07-27  8:47 ` [PATCH v11 08/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command Kiran K
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Kiran K @ 2021-07-27  8:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: ravishankar.srivatsa, chethan.tumkur.narayan, Kiran K

Define callback function to get codec config data. In HFP offload
usecase, controllers need to be set codec details before opening SCO.
This callback function is used to fetch vendor specific codec config
data.

Signed-off-by: Kiran K <kiran.k@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
* changes in v11:
 - Remove changes related to Kconfig

* changes in v10:
  - new patch due to refactoring

 drivers/bluetooth/btintel.c | 49 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 49813f2d2e27..11ac3bd6b5f6 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -1272,6 +1272,51 @@ int btintel_set_debug_features(struct hci_dev *hdev,
 }
 EXPORT_SYMBOL_GPL(btintel_set_debug_features);
 
+static int btintel_get_codec_config_data(struct hci_dev *hdev,
+					 __u8 link, struct bt_codec *codec,
+					 __u8 *ven_len, __u8 **ven_data)
+{
+	int err = 0;
+
+	if (!ven_data || !ven_len)
+		return -EINVAL;
+
+	*ven_len = 0;
+	*ven_data = NULL;
+
+	if (link != SCO_LINK && link != ESCO_LINK)
+		return -EINVAL;
+
+	*ven_data = kmalloc(sizeof(__u8), GFP_KERNEL);
+	if (!ven_data) {
+		err = -ENOMEM;
+		goto error;
+	}
+
+	/* supports only CVSD and mSBC offload codecs */
+	switch (codec->id) {
+	case 0x02:
+		**ven_data = 0x00;
+		break;
+	case 0x05:
+		**ven_data = 0x01;
+		break;
+	default:
+		err = -EINVAL;
+		goto error;
+	}
+	/* codec and its capabilities are pre-defined to ids
+	 * preset id = 0x00 represents CVSD codec with sampling rate 8K
+	 * preset id = 0x01 represents mSBC codec with sampling rate 16K
+	 */
+	*ven_len = sizeof(__u8);
+	return err;
+
+error:
+	kfree(*ven_data);
+	return err;
+}
+
 static int btintel_get_data_path_id(struct hci_dev *hdev, __u8 *data_path_id)
 {
 	/* Intel uses 1 as data path id for all the usecases */
@@ -1304,8 +1349,10 @@ int btintel_configure_offload_usecases(struct hci_dev *hdev)
 		goto error;
 	}
 
-	if (usecases->preset[0] & 0x03)
+	if (usecases->preset[0] & 0x03) {
 		hdev->get_data_path_id = btintel_get_data_path_id;
+		hdev->get_codec_config_data = btintel_get_codec_config_data;
+	}
 error:
 	kfree_skb(skb);
 	return err;
-- 
2.17.1


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

* [PATCH v11 08/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
                   ` (5 preceding siblings ...)
  2021-07-27  8:47 ` [PATCH v11 07/10] Bluetooth: btintel: Define a callback to fetch codec config data Kiran K
@ 2021-07-27  8:47 ` Kiran K
  2021-07-28  3:59   ` Luiz Augusto von Dentz
  2021-07-27  8:47 ` [PATCH v11 09/10] Bluetooth: Add support for msbc coding format Kiran K
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 25+ messages in thread
From: Kiran K @ 2021-07-27  8:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: ravishankar.srivatsa, chethan.tumkur.narayan, Kiran K

< HCI Command: Enhanced Setup Synchronous Connection (0x01|0x003d) plen 59
        Handle: 256
        Transmit bandwidth: 8000
        Receive bandwidth: 8000
        Max latency: 13
        Packet type: 0x0380
          3-EV3 may not be used
          2-EV5 may not be used
          3-EV5 may not be used
        Retransmission effort: Optimize for link quality (0x02)
> HCI Event: Command Status (0x0f) plen 4
      Enhanced Setup Synchronous Connection (0x01|0x003d) ncmd 1
        Status: Success (0x00)
> HCI Event: Synchronous Connect Complete (0x2c) plen 17
        Status: Success (0x00)
        Handle: 257
        Address: CC:98:8B:92:04:FD (SONY Visual Products Inc.)
        Link type: eSCO (0x02)
        Transmission interval: 0x0c
        Retransmission window: 0x06
        RX packet length: 60
        TX packet length: 60
        Air mode: Transparent (0x03)

Signed-off-by: Kiran K <kiran.k@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
* changes in v11:
  - Remove changes related to Kconfig
  - s/use_enhanced_sco_conn/enhanced_sco_capable/g
  - Minor review comments

* changes in v10:
  - Fix typos and remove unwanted chunks
* changes in v9:
  - Fix review comments, use bt_dev_dbg instead of BT_DBG

 include/net/bluetooth/bluetooth.h |   3 +-
 include/net/bluetooth/hci.h       |  34 ++++++++++
 include/net/bluetooth/hci_core.h  |   6 +-
 net/bluetooth/hci_conn.c          | 105 +++++++++++++++++++++++++++++-
 net/bluetooth/hci_event.c         |  39 +++++++++++
 net/bluetooth/sco.c               |  11 +++-
 6 files changed, 192 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 1a48b6732eef..f2df8bb108d9 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -173,7 +173,8 @@ struct bt_codecs {
 	struct bt_codec	codecs[];
 } __packed;
 
-#define CODING_FORMAT_CVSD	0x02
+#define CODING_FORMAT_CVSD		0x02
+#define CODING_FORMAT_TRANSPARENT	0x03
 
 __printf(1, 2)
 void bt_info(const char *fmt, ...);
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c998607fc517..8af8892e18bc 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -871,6 +871,40 @@ struct hci_cp_logical_link_cancel {
 	__u8     flow_spec_id;
 } __packed;
 
+#define HCI_OP_ENHANCED_SETUP_SYNC_CONN		0x043d
+struct hci_coding_format {
+	__u8	id;
+	__le16	cid;
+	__le16	vid;
+} __packed;
+
+struct hci_cp_enhanced_setup_sync_conn {
+	__le16   handle;
+	__le32   tx_bandwidth;
+	__le32   rx_bandwidth;
+	struct	 hci_coding_format tx_coding_format;
+	struct	 hci_coding_format rx_coding_format;
+	__le16	 tx_codec_frame_size;
+	__le16	 rx_codec_frame_size;
+	__le32	 in_bandwidth;
+	__le32	 out_bandwidth;
+	struct	 hci_coding_format in_coding_format;
+	struct	 hci_coding_format out_coding_format;
+	__le16   in_coded_data_size;
+	__le16	 out_coded_data_size;
+	__u8	 in_pcm_data_format;
+	__u8	 out_pcm_data_format;
+	__u8	 in_pcm_sample_payload_msb_pos;
+	__u8	 out_pcm_sample_payload_msb_pos;
+	__u8	 in_data_path;
+	__u8	 out_data_path;
+	__u8	 in_transport_unit_size;
+	__u8	 out_transport_unit_size;
+	__le16   max_latency;
+	__le16   pkt_type;
+	__u8     retrans_effort;
+} __packed;
+
 struct hci_rp_logical_link_cancel {
 	__u8     status;
 	__u8     phy_handle;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index eafa6f8114cb..4c310bd3e1c6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -713,6 +713,7 @@ struct hci_conn {
 	struct amp_mgr	*amp_mgr;
 
 	struct hci_conn	*link;
+	struct bt_codec codec;
 
 	void (*connect_cfm_cb)	(struct hci_conn *conn, u8 status);
 	void (*security_cfm_cb)	(struct hci_conn *conn, u8 status);
@@ -1119,7 +1120,7 @@ struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
 				 u8 sec_level, u8 auth_type,
 				 enum conn_reasons conn_reason);
 struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
-				 __u16 setting);
+				 __u16 setting, struct bt_codec *codec);
 int hci_conn_check_link_mode(struct hci_conn *conn);
 int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
 int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type,
@@ -1440,6 +1441,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
 /* Use LL Privacy based address resolution if supported */
 #define use_ll_privacy(dev) ((dev)->le_features[0] & HCI_LE_LL_PRIVACY)
 
+/* Use enhanced synchronous connection if command is supported */
+#define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
+
 /* Use ext scanning if set ext scan param and ext scan enable is supported */
 #define use_ext_scan(dev) (((dev)->commands[37] & 0x20) && \
 			   ((dev)->commands[37] & 0x40))
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 2b5059a56cda..48943b01a356 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -307,13 +307,103 @@ static bool find_next_esco_param(struct hci_conn *conn,
 	return conn->attempt <= size;
 }
 
-bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
+static bool hci_enhanced_setup_sync_conn(struct hci_conn *conn, __u16 handle)
+{
+	struct hci_dev *hdev = conn->hdev;
+	struct hci_cp_enhanced_setup_sync_conn cp;
+	const struct sco_param *param;
+
+	bt_dev_dbg(hdev, "hcon %p", conn);
+
+	conn->state = BT_CONNECT;
+	conn->out = true;
+
+	conn->attempt++;
+
+	memset(&cp, 0x00, sizeof(cp));
+
+	cp.handle   = cpu_to_le16(handle);
+
+	cp.tx_bandwidth   = cpu_to_le32(0x00001f40);
+	cp.rx_bandwidth   = cpu_to_le32(0x00001f40);
+
+	switch (conn->codec.id) {
+	case CODING_FORMAT_TRANSPARENT:
+		if (!find_next_esco_param(conn, esco_param_msbc,
+					  ARRAY_SIZE(esco_param_msbc)))
+			return false;
+		param = &esco_param_msbc[conn->attempt - 1];
+		cp.tx_coding_format.id = 0x03;
+		cp.rx_coding_format.id = 0x03;
+		cp.tx_codec_frame_size = __cpu_to_le16(60);
+		cp.rx_codec_frame_size = __cpu_to_le16(60);
+		cp.in_bandwidth = __cpu_to_le32(0x1f40);
+		cp.out_bandwidth = __cpu_to_le32(0x1f40);
+		cp.in_coding_format.id = 0x03;
+		cp.out_coding_format.id = 0x03;
+		cp.in_coded_data_size = __cpu_to_le16(16);
+		cp.out_coded_data_size = __cpu_to_le16(16);
+		cp.in_pcm_data_format = 2;
+		cp.out_pcm_data_format = 2;
+		cp.in_pcm_sample_payload_msb_pos = 0;
+		cp.out_pcm_sample_payload_msb_pos = 0;
+		cp.in_data_path = conn->codec.data_path;
+		cp.out_data_path = conn->codec.data_path;
+		cp.in_transport_unit_size = 1;
+		cp.out_transport_unit_size = 1;
+		break;
+
+	case CODING_FORMAT_CVSD:
+		if (lmp_esco_capable(conn->link)) {
+			if (!find_next_esco_param(conn, esco_param_cvsd,
+						  ARRAY_SIZE(esco_param_cvsd)))
+				return false;
+			param = &esco_param_cvsd[conn->attempt - 1];
+		} else {
+			if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
+				return false;
+			param = &sco_param_cvsd[conn->attempt - 1];
+		}
+		cp.tx_coding_format.id = 2;
+		cp.rx_coding_format.id = 2;
+		cp.tx_codec_frame_size = __cpu_to_le16(60);
+		cp.rx_codec_frame_size = __cpu_to_le16(60);
+		cp.in_bandwidth = __cpu_to_le32(16000);
+		cp.out_bandwidth = __cpu_to_le32(16000);
+		cp.in_coding_format.id = 4;
+		cp.out_coding_format.id = 4;
+		cp.in_coded_data_size = __cpu_to_le16(16);
+		cp.out_coded_data_size = __cpu_to_le16(16);
+		cp.in_pcm_data_format = 2;
+		cp.out_pcm_data_format = 2;
+		cp.in_pcm_sample_payload_msb_pos = 0;
+		cp.out_pcm_sample_payload_msb_pos = 0;
+		cp.in_data_path = conn->codec.data_path;
+		cp.out_data_path = conn->codec.data_path;
+		cp.in_transport_unit_size = 16;
+		cp.out_transport_unit_size = 16;
+		break;
+	default:
+		return false;
+	}
+
+	cp.retrans_effort = param->retrans_effort;
+	cp.pkt_type = __cpu_to_le16(param->pkt_type);
+	cp.max_latency = __cpu_to_le16(param->max_latency);
+
+	if (hci_send_cmd(hdev, HCI_OP_ENHANCED_SETUP_SYNC_CONN, sizeof(cp), &cp) < 0)
+		return false;
+
+	return true;
+}
+
+static bool hci_setup_sync_conn(struct hci_conn *conn, __u16 handle)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_setup_sync_conn cp;
 	const struct sco_param *param;
 
-	BT_DBG("hcon %p", conn);
+	bt_dev_dbg(hdev, "hcon %p", conn);
 
 	conn->state = BT_CONNECT;
 	conn->out = true;
@@ -359,6 +449,14 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
 	return true;
 }
 
+bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
+{
+	if (enhanced_sco_capable(conn->hdev))
+		return hci_enhanced_setup_sync_conn(conn, handle);
+
+	return hci_setup_sync_conn(conn, handle);
+}
+
 u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
 		      u16 to_multiplier)
 {
@@ -1319,7 +1417,7 @@ struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
 }
 
 struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
-				 __u16 setting)
+				 __u16 setting, struct bt_codec *codec)
 {
 	struct hci_conn *acl;
 	struct hci_conn *sco;
@@ -1344,6 +1442,7 @@ struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
 	hci_conn_hold(sco);
 
 	sco->setting = setting;
+	sco->codec = *codec;
 
 	if (acl->state == BT_CONNECTED &&
 	    (sco->state == BT_OPEN || sco->state == BT_CLOSED)) {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 1c3018202564..18985ac6a83f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2236,6 +2236,41 @@ static void hci_cs_setup_sync_conn(struct hci_dev *hdev, __u8 status)
 	hci_dev_unlock(hdev);
 }
 
+static void hci_cs_enhanced_setup_sync_conn(struct hci_dev *hdev, __u8 status)
+{
+	struct hci_cp_enhanced_setup_sync_conn *cp;
+	struct hci_conn *acl, *sco;
+	__u16 handle;
+
+	bt_dev_dbg(hdev, "status 0x%2.2x", status);
+
+	if (!status)
+		return;
+
+	cp = hci_sent_cmd_data(hdev, HCI_OP_ENHANCED_SETUP_SYNC_CONN);
+	if (!cp)
+		return;
+
+	handle = __le16_to_cpu(cp->handle);
+
+	bt_dev_dbg(hdev, "handle 0x%4.4x", handle);
+
+	hci_dev_lock(hdev);
+
+	acl = hci_conn_hash_lookup_handle(hdev, handle);
+	if (acl) {
+		sco = acl->link;
+		if (sco) {
+			sco->state = BT_CLOSED;
+
+			hci_connect_cfm(sco, status);
+			hci_conn_del(sco);
+		}
+	}
+
+	hci_dev_unlock(hdev);
+}
+
 static void hci_cs_sniff_mode(struct hci_dev *hdev, __u8 status)
 {
 	struct hci_cp_sniff_mode *cp;
@@ -3715,6 +3750,10 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb,
 		hci_cs_setup_sync_conn(hdev, ev->status);
 		break;
 
+	case HCI_OP_ENHANCED_SETUP_SYNC_CONN:
+		hci_cs_enhanced_setup_sync_conn(hdev, ev->status);
+		break;
+
 	case HCI_OP_SNIFF_MODE:
 		hci_cs_sniff_mode(hdev, ev->status);
 		break;
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 34541e971dc7..5ff06ac05fef 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -240,7 +240,7 @@ static int sco_connect(struct sock *sk)
 	}
 
 	hcon = hci_connect_sco(hdev, type, &sco_pi(sk)->dst,
-			       sco_pi(sk)->setting);
+			       sco_pi(sk)->setting, &sco_pi(sk)->codec);
 	if (IS_ERR(hcon)) {
 		err = PTR_ERR(hcon);
 		goto done;
@@ -927,6 +927,15 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 		}
 
 		sco_pi(sk)->setting = voice.setting;
+		hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src,
+				     BDADDR_BREDR);
+		if (!hdev) {
+			err = -EBADFD;
+			break;
+		}
+		if (enhanced_sco_capable(hdev) &&
+		    voice.setting == BT_VOICE_TRANSPARENT)
+			sco_pi(sk)->codec.id = CODING_FORMAT_TRANSPARENT;
 		break;
 
 	case BT_PKT_STATUS:
-- 
2.17.1


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

* [PATCH v11 09/10] Bluetooth: Add support for msbc coding format
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
                   ` (6 preceding siblings ...)
  2021-07-27  8:47 ` [PATCH v11 08/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command Kiran K
@ 2021-07-27  8:47 ` Kiran K
  2021-07-27  8:47 ` [PATCH v11 10/10] Bluetooth: Add offload feature under experimental flag Kiran K
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Kiran K @ 2021-07-27  8:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: ravishankar.srivatsa, chethan.tumkur.narayan, Kiran K

In Enhanced_Setup_Synchronous_Command, add support for msbc
coding format

Signed-off-by: Kiran K <kiran.k@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
* changes in v11:
  - Remove Kconfig

* changes on v10:
  - Fix typos and unwanted chunks

 include/net/bluetooth/bluetooth.h |  1 +
 net/bluetooth/hci_conn.c          | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index f2df8bb108d9..c1fa90fb7502 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -175,6 +175,7 @@ struct bt_codecs {
 
 #define CODING_FORMAT_CVSD		0x02
 #define CODING_FORMAT_TRANSPARENT	0x03
+#define CODING_FORMAT_MSBC		0x05
 
 __printf(1, 2)
 void bt_info(const char *fmt, ...);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 48943b01a356..95ffff4da885 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -328,6 +328,31 @@ static bool hci_enhanced_setup_sync_conn(struct hci_conn *conn, __u16 handle)
 	cp.rx_bandwidth   = cpu_to_le32(0x00001f40);
 
 	switch (conn->codec.id) {
+	case CODING_FORMAT_MSBC:
+		if (!find_next_esco_param(conn, esco_param_msbc,
+					  ARRAY_SIZE(esco_param_msbc)))
+			return false;
+
+		param = &esco_param_msbc[conn->attempt - 1];
+		cp.tx_coding_format.id = 0x05;
+		cp.rx_coding_format.id = 0x05;
+		cp.tx_codec_frame_size = __cpu_to_le16(60);
+		cp.rx_codec_frame_size = __cpu_to_le16(60);
+		cp.in_bandwidth = __cpu_to_le32(32000);
+		cp.out_bandwidth = __cpu_to_le32(32000);
+		cp.in_coding_format.id = 0x04;
+		cp.out_coding_format.id = 0x04;
+		cp.in_coded_data_size = __cpu_to_le16(16);
+		cp.out_coded_data_size = __cpu_to_le16(16);
+		cp.in_pcm_data_format = 2;
+		cp.out_pcm_data_format = 2;
+		cp.in_pcm_sample_payload_msb_pos = 0;
+		cp.out_pcm_sample_payload_msb_pos = 0;
+		cp.in_data_path = conn->codec.data_path;
+		cp.out_data_path = conn->codec.data_path;
+		cp.in_transport_unit_size = 1;
+		cp.out_transport_unit_size = 1;
+		break;
 	case CODING_FORMAT_TRANSPARENT:
 		if (!find_next_esco_param(conn, esco_param_msbc,
 					  ARRAY_SIZE(esco_param_msbc)))
-- 
2.17.1


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

* [PATCH v11 10/10] Bluetooth: Add offload feature under experimental flag
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
                   ` (7 preceding siblings ...)
  2021-07-27  8:47 ` [PATCH v11 09/10] Bluetooth: Add support for msbc coding format Kiran K
@ 2021-07-27  8:47 ` Kiran K
  2021-07-30 14:22   ` Marcel Holtmann
  2021-07-27  9:12 ` [v11,01/10] Bluetooth: Enumerate local supported codec and cache details bluez.test.bot
  2021-07-30 14:06 ` [PATCH v11 01/10] " Marcel Holtmann
  10 siblings, 1 reply; 25+ messages in thread
From: Kiran K @ 2021-07-27  8:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: ravishankar.srivatsa, chethan.tumkur.narayan, Kiran K

Allow user level process to enable / disable codec offload
feature through mgmt interface. By default offload codec feature
is disabled.

Signed-off-by: Kiran K <kiran.k@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
changes in v11:
- Remove Kconfig

 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/mgmt.c        | 98 ++++++++++++++++++++++++++++++++++++-
 net/bluetooth/sco.c         | 10 ++++
 3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 8af8892e18bc..dbbbb974ade6 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -330,6 +330,7 @@ enum {
 	HCI_ENABLE_LL_PRIVACY,
 	HCI_CMD_PENDING,
 	HCI_FORCE_NO_MITM,
+	HCI_OFFLOAD_CODECS_ENABLED,
 
 	__HCI_NUM_FLAGS,
 };
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3663f880df11..eb10f9597837 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3791,6 +3791,12 @@ static const u8 debug_uuid[16] = {
 };
 #endif
 
+/* a6695ace-ee7f-4fb9-881a-5fac66c629af */
+static const u8 offload_codecs_uuid[16] = {
+	0xaf, 0x29, 0xc6, 0x66, 0xac, 0x5f, 0x1a, 0x88,
+	0xb9, 0x4f, 0x7f, 0xee, 0xce, 0x5a, 0x69, 0xa6,
+};
+
 /* 671b10b5-42c0-4696-9227-eb28d1b049d6 */
 static const u8 simult_central_periph_uuid[16] = {
 	0xd6, 0x49, 0xb0, 0xd1, 0x28, 0xeb, 0x27, 0x92,
@@ -3806,7 +3812,7 @@ static const u8 rpa_resolution_uuid[16] = {
 static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
 				  void *data, u16 data_len)
 {
-	char buf[62];	/* Enough space for 3 features */
+	char buf[82];   /* Enough space for 4 features: 2 + 20 * 4 */
 	struct mgmt_rp_read_exp_features_info *rp = (void *)buf;
 	u16 idx = 0;
 	u32 flags;
@@ -3850,6 +3856,26 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
 		idx++;
 	}
 
+	if (hdev) {
+		if (hdev->get_data_path_id) {
+			/* BIT(0): indicating if offload codecs are
+			 * supported by controller.
+			 */
+			flags = BIT(0);
+
+			/* BIT(1): indicating if codec offload feature
+			 * is enabled.
+			 */
+			if (hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED))
+				flags |= BIT(1);
+		} else {
+			flags = 0;
+		}
+		memcpy(rp->features[idx].uuid, offload_codecs_uuid, 16);
+		rp->features[idx].flags = cpu_to_le32(flags);
+		idx++;
+	}
+
 	rp->feature_count = cpu_to_le16(idx);
 
 	/* After reading the experimental features information, enable
@@ -3892,6 +3918,21 @@ static int exp_debug_feature_changed(bool enabled, struct sock *skip)
 }
 #endif
 
+static int exp_offload_codec_feature_changed(bool enabled, struct sock *skip)
+{
+	struct mgmt_ev_exp_feature_changed ev;
+
+	BT_INFO("enabled %d", enabled);
+
+	memset(&ev, 0, sizeof(ev));
+	memcpy(ev.uuid, offload_codecs_uuid, 16);
+	ev.flags = cpu_to_le32(enabled ? BIT(0) : 0);
+
+	return mgmt_limited_event(MGMT_EV_EXP_FEATURE_CHANGED, NULL,
+				  &ev, sizeof(ev),
+				  HCI_MGMT_EXP_FEATURE_EVENTS, skip);
+}
+
 static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
 			   void *data, u16 data_len)
 {
@@ -4038,6 +4079,61 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
 		return err;
 	}
 
+	if (!memcmp(cp->uuid, offload_codecs_uuid, 16)) {
+		bool val, changed;
+		int err;
+
+		/* Command requires to use a valid controller index */
+		if (!hdev)
+			return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
+					       MGMT_OP_SET_EXP_FEATURE,
+					       MGMT_STATUS_INVALID_INDEX);
+
+		/* Parameters are limited to a single octet */
+		if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
+			return mgmt_cmd_status(sk, hdev->id,
+					       MGMT_OP_SET_EXP_FEATURE,
+					       MGMT_STATUS_INVALID_PARAMS);
+
+		/* Only boolean on/off is supported */
+		if (cp->param[0] != 0x00 && cp->param[0] != 0x01)
+			return mgmt_cmd_status(sk, hdev->id,
+					       MGMT_OP_SET_EXP_FEATURE,
+					       MGMT_STATUS_INVALID_PARAMS);
+
+		val = !!cp->param[0];
+		changed = (val != hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED));
+
+		if (!hdev->get_data_path_id) {
+			bt_dev_info(hdev, "offload codecs not supported");
+			return mgmt_cmd_status(sk, hdev->id,
+					       MGMT_OP_SET_EXP_FEATURE,
+					       MGMT_STATUS_NOT_SUPPORTED);
+		}
+
+		if (changed) {
+			if (val)
+				hci_dev_set_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED);
+			else
+				hci_dev_clear_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED);
+		}
+
+		bt_dev_info(hdev, "offload codecs enable %d changed %d",
+			    val, changed);
+
+		memcpy(rp.uuid, offload_codecs_uuid, 16);
+		rp.flags = cpu_to_le32(val ? BIT(0) : 0);
+		hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
+		err = mgmt_cmd_complete(sk, hdev->id,
+					MGMT_OP_SET_EXP_FEATURE, 0,
+					&rp, sizeof(rp));
+
+		if (changed)
+			exp_offload_codec_feature_changed(val, sk);
+
+		return err;
+	}
+
 	return mgmt_cmd_status(sk, hdev ? hdev->id : MGMT_INDEX_NONE,
 			       MGMT_OP_SET_EXP_FEATURE,
 			       MGMT_STATUS_NOT_SUPPORTED);
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 5ff06ac05fef..ba66e082ba9e 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -964,6 +964,11 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
+		if (!hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED)) {
+			err = -EOPNOTSUPP;
+			break;
+		}
+
 		if (!hdev->get_codec_config_data) {
 			err = -EOPNOTSUPP;
 			break;
@@ -1160,6 +1165,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
+		if (!hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED)) {
+			err = -EOPNOTSUPP;
+			break;
+		}
+
 		if (!hdev->get_data_path_id) {
 			err = -EOPNOTSUPP;
 			break;
-- 
2.17.1


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

* RE: [v11,01/10] Bluetooth: Enumerate local supported codec and cache details
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
                   ` (8 preceding siblings ...)
  2021-07-27  8:47 ` [PATCH v11 10/10] Bluetooth: Add offload feature under experimental flag Kiran K
@ 2021-07-27  9:12 ` bluez.test.bot
  2021-07-30 14:06 ` [PATCH v11 01/10] " Marcel Holtmann
  10 siblings, 0 replies; 25+ messages in thread
From: bluez.test.bot @ 2021-07-27  9:12 UTC (permalink / raw)
  To: linux-bluetooth, kiran.k

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

---Test result---

Test Summary:
CheckPatch                    FAIL      1.71 seconds
GitLint                       FAIL      1.01 seconds
BuildKernel                   PASS      651.06 seconds
TestRunner: Setup             PASS      418.48 seconds
TestRunner: l2cap-tester      PASS      3.03 seconds
TestRunner: bnep-tester       PASS      2.21 seconds
TestRunner: mgmt-tester       FAIL      32.55 seconds
TestRunner: rfcomm-tester     PASS      2.44 seconds
TestRunner: sco-tester        PASS      2.32 seconds
TestRunner: smp-tester        FAIL      2.39 seconds
TestRunner: userchan-tester   PASS      2.29 seconds

Details
##############################
Test: CheckPatch - FAIL - 1.71 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: Enumerate local supported codec and cache details
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#126: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 336 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: Enumerate local supported codec and cache details" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL - 1.01 seconds
Run gitlint with rule in .gitlint
Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command
1: T1 Title exceeds max length (76>72): "Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command"


##############################
Test: BuildKernel - PASS - 651.06 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 418.48 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 3.03 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 2.21 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - FAIL - 32.55 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 444 (99.1%), Failed: 1, Not Run: 3

Failed Test Cases
Read Exp Feature - Success                           Failed       0.012 seconds

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.44 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.32 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.39 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.025 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 2.29 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth


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

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

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

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

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

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

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

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

* Re: [PATCH v11 08/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command
  2021-07-27  8:47 ` [PATCH v11 08/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command Kiran K
@ 2021-07-28  3:59   ` Luiz Augusto von Dentz
  2021-08-14  6:41     ` K, Kiran
  0 siblings, 1 reply; 25+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-28  3:59 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth, Srivatsa, Ravishankar, Chethan T N

Hi Kiran,

On Tue, Jul 27, 2021 at 1:45 AM Kiran K <kiran.k@intel.com> wrote:
>
> < HCI Command: Enhanced Setup Synchronous Connection (0x01|0x003d) plen 59
>         Handle: 256
>         Transmit bandwidth: 8000
>         Receive bandwidth: 8000
>         Max latency: 13
>         Packet type: 0x0380
>           3-EV3 may not be used
>           2-EV5 may not be used
>           3-EV5 may not be used
>         Retransmission effort: Optimize for link quality (0x02)
> > HCI Event: Command Status (0x0f) plen 4
>       Enhanced Setup Synchronous Connection (0x01|0x003d) ncmd 1
>         Status: Success (0x00)
> > HCI Event: Synchronous Connect Complete (0x2c) plen 17
>         Status: Success (0x00)
>         Handle: 257
>         Address: CC:98:8B:92:04:FD (SONY Visual Products Inc.)
>         Link type: eSCO (0x02)
>         Transmission interval: 0x0c
>         Retransmission window: 0x06
>         RX packet length: 60
>         TX packet length: 60
>         Air mode: Transparent (0x03)
>
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> * changes in v11:
>   - Remove changes related to Kconfig
>   - s/use_enhanced_sco_conn/enhanced_sco_capable/g
>   - Minor review comments
>
> * changes in v10:
>   - Fix typos and remove unwanted chunks
> * changes in v9:
>   - Fix review comments, use bt_dev_dbg instead of BT_DBG
>
>  include/net/bluetooth/bluetooth.h |   3 +-
>  include/net/bluetooth/hci.h       |  34 ++++++++++
>  include/net/bluetooth/hci_core.h  |   6 +-
>  net/bluetooth/hci_conn.c          | 105 +++++++++++++++++++++++++++++-
>  net/bluetooth/hci_event.c         |  39 +++++++++++
>  net/bluetooth/sco.c               |  11 +++-
>  6 files changed, 192 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 1a48b6732eef..f2df8bb108d9 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -173,7 +173,8 @@ struct bt_codecs {
>         struct bt_codec codecs[];
>  } __packed;
>
> -#define CODING_FORMAT_CVSD     0x02
> +#define CODING_FORMAT_CVSD             0x02
> +#define CODING_FORMAT_TRANSPARENT      0x03
>
>  __printf(1, 2)
>  void bt_info(const char *fmt, ...);
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index c998607fc517..8af8892e18bc 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -871,6 +871,40 @@ struct hci_cp_logical_link_cancel {
>         __u8     flow_spec_id;
>  } __packed;
>
> +#define HCI_OP_ENHANCED_SETUP_SYNC_CONN                0x043d
> +struct hci_coding_format {
> +       __u8    id;
> +       __le16  cid;
> +       __le16  vid;
> +} __packed;
> +
> +struct hci_cp_enhanced_setup_sync_conn {
> +       __le16   handle;
> +       __le32   tx_bandwidth;
> +       __le32   rx_bandwidth;
> +       struct   hci_coding_format tx_coding_format;
> +       struct   hci_coding_format rx_coding_format;
> +       __le16   tx_codec_frame_size;
> +       __le16   rx_codec_frame_size;
> +       __le32   in_bandwidth;
> +       __le32   out_bandwidth;
> +       struct   hci_coding_format in_coding_format;
> +       struct   hci_coding_format out_coding_format;
> +       __le16   in_coded_data_size;
> +       __le16   out_coded_data_size;
> +       __u8     in_pcm_data_format;
> +       __u8     out_pcm_data_format;
> +       __u8     in_pcm_sample_payload_msb_pos;
> +       __u8     out_pcm_sample_payload_msb_pos;
> +       __u8     in_data_path;
> +       __u8     out_data_path;
> +       __u8     in_transport_unit_size;
> +       __u8     out_transport_unit_size;
> +       __le16   max_latency;
> +       __le16   pkt_type;
> +       __u8     retrans_effort;
> +} __packed;
> +
>  struct hci_rp_logical_link_cancel {
>         __u8     status;
>         __u8     phy_handle;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index eafa6f8114cb..4c310bd3e1c6 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -713,6 +713,7 @@ struct hci_conn {
>         struct amp_mgr  *amp_mgr;
>
>         struct hci_conn *link;
> +       struct bt_codec codec;
>
>         void (*connect_cfm_cb)  (struct hci_conn *conn, u8 status);
>         void (*security_cfm_cb) (struct hci_conn *conn, u8 status);
> @@ -1119,7 +1120,7 @@ struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
>                                  u8 sec_level, u8 auth_type,
>                                  enum conn_reasons conn_reason);
>  struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
> -                                __u16 setting);
> +                                __u16 setting, struct bt_codec *codec);
>  int hci_conn_check_link_mode(struct hci_conn *conn);
>  int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
>  int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type,
> @@ -1440,6 +1441,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>  /* Use LL Privacy based address resolution if supported */
>  #define use_ll_privacy(dev) ((dev)->le_features[0] & HCI_LE_LL_PRIVACY)
>
> +/* Use enhanced synchronous connection if command is supported */
> +#define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
> +
>  /* Use ext scanning if set ext scan param and ext scan enable is supported */
>  #define use_ext_scan(dev) (((dev)->commands[37] & 0x20) && \
>                            ((dev)->commands[37] & 0x40))
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 2b5059a56cda..48943b01a356 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -307,13 +307,103 @@ static bool find_next_esco_param(struct hci_conn *conn,
>         return conn->attempt <= size;
>  }
>
> -bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
> +static bool hci_enhanced_setup_sync_conn(struct hci_conn *conn, __u16 handle)
> +{
> +       struct hci_dev *hdev = conn->hdev;
> +       struct hci_cp_enhanced_setup_sync_conn cp;
> +       const struct sco_param *param;
> +
> +       bt_dev_dbg(hdev, "hcon %p", conn);
> +
> +       conn->state = BT_CONNECT;
> +       conn->out = true;
> +
> +       conn->attempt++;
> +
> +       memset(&cp, 0x00, sizeof(cp));
> +
> +       cp.handle   = cpu_to_le16(handle);
> +
> +       cp.tx_bandwidth   = cpu_to_le32(0x00001f40);
> +       cp.rx_bandwidth   = cpu_to_le32(0x00001f40);
> +
> +       switch (conn->codec.id) {
> +       case CODING_FORMAT_TRANSPARENT:
> +               if (!find_next_esco_param(conn, esco_param_msbc,
> +                                         ARRAY_SIZE(esco_param_msbc)))
> +                       return false;
> +               param = &esco_param_msbc[conn->attempt - 1];
> +               cp.tx_coding_format.id = 0x03;
> +               cp.rx_coding_format.id = 0x03;
> +               cp.tx_codec_frame_size = __cpu_to_le16(60);
> +               cp.rx_codec_frame_size = __cpu_to_le16(60);
> +               cp.in_bandwidth = __cpu_to_le32(0x1f40);
> +               cp.out_bandwidth = __cpu_to_le32(0x1f40);
> +               cp.in_coding_format.id = 0x03;
> +               cp.out_coding_format.id = 0x03;
> +               cp.in_coded_data_size = __cpu_to_le16(16);
> +               cp.out_coded_data_size = __cpu_to_le16(16);
> +               cp.in_pcm_data_format = 2;
> +               cp.out_pcm_data_format = 2;
> +               cp.in_pcm_sample_payload_msb_pos = 0;
> +               cp.out_pcm_sample_payload_msb_pos = 0;
> +               cp.in_data_path = conn->codec.data_path;
> +               cp.out_data_path = conn->codec.data_path;
> +               cp.in_transport_unit_size = 1;
> +               cp.out_transport_unit_size = 1;
> +               break;
> +
> +       case CODING_FORMAT_CVSD:
> +               if (lmp_esco_capable(conn->link)) {
> +                       if (!find_next_esco_param(conn, esco_param_cvsd,
> +                                                 ARRAY_SIZE(esco_param_cvsd)))
> +                               return false;
> +                       param = &esco_param_cvsd[conn->attempt - 1];
> +               } else {
> +                       if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
> +                               return false;
> +                       param = &sco_param_cvsd[conn->attempt - 1];
> +               }
> +               cp.tx_coding_format.id = 2;
> +               cp.rx_coding_format.id = 2;
> +               cp.tx_codec_frame_size = __cpu_to_le16(60);
> +               cp.rx_codec_frame_size = __cpu_to_le16(60);
> +               cp.in_bandwidth = __cpu_to_le32(16000);
> +               cp.out_bandwidth = __cpu_to_le32(16000);
> +               cp.in_coding_format.id = 4;
> +               cp.out_coding_format.id = 4;
> +               cp.in_coded_data_size = __cpu_to_le16(16);
> +               cp.out_coded_data_size = __cpu_to_le16(16);
> +               cp.in_pcm_data_format = 2;
> +               cp.out_pcm_data_format = 2;
> +               cp.in_pcm_sample_payload_msb_pos = 0;
> +               cp.out_pcm_sample_payload_msb_pos = 0;
> +               cp.in_data_path = conn->codec.data_path;
> +               cp.out_data_path = conn->codec.data_path;
> +               cp.in_transport_unit_size = 16;
> +               cp.out_transport_unit_size = 16;
> +               break;
> +       default:
> +               return false;
> +       }
> +
> +       cp.retrans_effort = param->retrans_effort;
> +       cp.pkt_type = __cpu_to_le16(param->pkt_type);
> +       cp.max_latency = __cpu_to_le16(param->max_latency);
> +
> +       if (hci_send_cmd(hdev, HCI_OP_ENHANCED_SETUP_SYNC_CONN, sizeof(cp), &cp) < 0)
> +               return false;
> +
> +       return true;
> +}
> +
> +static bool hci_setup_sync_conn(struct hci_conn *conn, __u16 handle)
>  {
>         struct hci_dev *hdev = conn->hdev;
>         struct hci_cp_setup_sync_conn cp;
>         const struct sco_param *param;
>
> -       BT_DBG("hcon %p", conn);
> +       bt_dev_dbg(hdev, "hcon %p", conn);
>
>         conn->state = BT_CONNECT;
>         conn->out = true;
> @@ -359,6 +449,14 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
>         return true;
>  }
>
> +bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
> +{
> +       if (enhanced_sco_capable(conn->hdev))
> +               return hci_enhanced_setup_sync_conn(conn, handle);
> +
> +       return hci_setup_sync_conn(conn, handle);
> +}
> +
>  u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
>                       u16 to_multiplier)
>  {
> @@ -1319,7 +1417,7 @@ struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
>  }
>
>  struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
> -                                __u16 setting)
> +                                __u16 setting, struct bt_codec *codec)
>  {
>         struct hci_conn *acl;
>         struct hci_conn *sco;
> @@ -1344,6 +1442,7 @@ struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
>         hci_conn_hold(sco);
>
>         sco->setting = setting;
> +       sco->codec = *codec;
>
>         if (acl->state == BT_CONNECTED &&
>             (sco->state == BT_OPEN || sco->state == BT_CLOSED)) {
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 1c3018202564..18985ac6a83f 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2236,6 +2236,41 @@ static void hci_cs_setup_sync_conn(struct hci_dev *hdev, __u8 status)
>         hci_dev_unlock(hdev);
>  }
>
> +static void hci_cs_enhanced_setup_sync_conn(struct hci_dev *hdev, __u8 status)
> +{
> +       struct hci_cp_enhanced_setup_sync_conn *cp;
> +       struct hci_conn *acl, *sco;
> +       __u16 handle;
> +
> +       bt_dev_dbg(hdev, "status 0x%2.2x", status);
> +
> +       if (!status)
> +               return;
> +
> +       cp = hci_sent_cmd_data(hdev, HCI_OP_ENHANCED_SETUP_SYNC_CONN);
> +       if (!cp)
> +               return;
> +
> +       handle = __le16_to_cpu(cp->handle);
> +
> +       bt_dev_dbg(hdev, "handle 0x%4.4x", handle);
> +
> +       hci_dev_lock(hdev);
> +
> +       acl = hci_conn_hash_lookup_handle(hdev, handle);
> +       if (acl) {
> +               sco = acl->link;
> +               if (sco) {
> +                       sco->state = BT_CLOSED;
> +
> +                       hci_connect_cfm(sco, status);
> +                       hci_conn_del(sco);
> +               }
> +       }
> +
> +       hci_dev_unlock(hdev);
> +}
> +
>  static void hci_cs_sniff_mode(struct hci_dev *hdev, __u8 status)
>  {
>         struct hci_cp_sniff_mode *cp;
> @@ -3715,6 +3750,10 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb,
>                 hci_cs_setup_sync_conn(hdev, ev->status);
>                 break;
>
> +       case HCI_OP_ENHANCED_SETUP_SYNC_CONN:
> +               hci_cs_enhanced_setup_sync_conn(hdev, ev->status);
> +               break;
> +
>         case HCI_OP_SNIFF_MODE:
>                 hci_cs_sniff_mode(hdev, ev->status);
>                 break;
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 34541e971dc7..5ff06ac05fef 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -240,7 +240,7 @@ static int sco_connect(struct sock *sk)
>         }
>
>         hcon = hci_connect_sco(hdev, type, &sco_pi(sk)->dst,
> -                              sco_pi(sk)->setting);
> +                              sco_pi(sk)->setting, &sco_pi(sk)->codec);
>         if (IS_ERR(hcon)) {
>                 err = PTR_ERR(hcon);
>                 goto done;
> @@ -927,6 +927,15 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
>                 }
>
>                 sco_pi(sk)->setting = voice.setting;
> +               hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src,
> +                                    BDADDR_BREDR);
> +               if (!hdev) {
> +                       err = -EBADFD;
> +                       break;
> +               }
> +               if (enhanced_sco_capable(hdev) &&
> +                   voice.setting == BT_VOICE_TRANSPARENT)
> +                       sco_pi(sk)->codec.id = CODING_FORMAT_TRANSPARENT;


hci_get_route does call hci_dev_hold so you need hci_dev_put once you
are done with it.

>                 break;
>
>         case BT_PKT_STATUS:
> --
> 2.17.1
>


-- 
Luiz Augusto von Dentz

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

* Re: [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details
  2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
                   ` (9 preceding siblings ...)
  2021-07-27  9:12 ` [v11,01/10] Bluetooth: Enumerate local supported codec and cache details bluez.test.bot
@ 2021-07-30 14:06 ` Marcel Holtmann
  2021-08-02  0:23   ` K, Kiran
  10 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2021-07-30 14:06 UTC (permalink / raw)
  To: Kiran K; +Cc: BlueZ, Srivatsa, Ravishankar, chethan.tumkur.narayan

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>
> ---
> * changes in v11:
> - Remove Kconfig related changes
> - Address minor review comments
> - Move codec related functions to new file hci_codec.c
> 
> * changes in v10:
> - define Kconfig to control offload feature at build time
> - fix review comments
> 
> * changes in v9:
>  - use shortname vnd instead of ven
> 
> * changes in v8:
>  - add comments
>  - split __u8 codec_id[5] into {__u8 id; __le16 cid, vid }
>  - address review comment related codec caps structure
> 
> * 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      |  41 +++++++
> include/net/bluetooth/hci_core.h |  17 +++
> net/bluetooth/Makefile           |   2 +-
> net/bluetooth/hci_codec.c        | 194 +++++++++++++++++++++++++++++++
> net/bluetooth/hci_codec.h        |   6 +
> net/bluetooth/hci_core.c         |  12 +-
> 6 files changed, 266 insertions(+), 6 deletions(-)
> create mode 100644 net/bluetooth/hci_codec.c
> create mode 100644 net/bluetooth/hci_codec.h
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index b80415011dcd..f76849c8eafd 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1307,6 +1307,28 @@ struct hci_rp_read_data_block_size {
> } __packed;
> 
> #define HCI_OP_READ_LOCAL_CODECS	0x100b
> +struct hci_std_codecs {
> +	__u8	num;
> +	__u8	codec[];
> +} __packed;
> +
> +struct hci_vnd_codec {
> +	/* company id */
> +	__le16	cid;
> +	/* vendor codec id */
> +	__le16	vid;
> +} __packed;
> +
> +struct hci_vnd_codecs {
> +	__u8	num;
> +	struct hci_vnd_codec codec[];
> +} __packed;
> +
> +struct hci_rp_read_local_supported_codecs {
> +	__u8	status;
> +	struct hci_std_codecs std_codecs;
> +	struct hci_vnd_codecs vnd_codecs;
> +} __packed;
> 
> #define HCI_OP_READ_LOCAL_PAIRING_OPTS	0x100c
> struct hci_rp_read_local_pairing_opts {
> @@ -1315,6 +1337,25 @@ 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	id;
> +	__le16	cid;
> +	__le16	vid;
> +	__u8	transport;
> +	__u8	direction;
> +} __packed;
> +
> +struct hci_codec_caps {
> +	__u8	len;
> +	__u8	data[];
> +} __packed;
> +
> +struct hci_rp_read_local_codec_caps {
> +	__u8	status;
> +	__u8	num_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 a53e94459ecd..6742e6ad7b37 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -131,6 +131,17 @@ struct bdaddr_list {
> 	u8 bdaddr_type;
> };
> 
> +struct codec_list {
> +	struct list_head list;
> +	u8	id;
> +	__u16	cid;
> +	__u16	vid;
> +	u8	transport;
> +	u8	num_caps;
> +	u32	len;
> +	struct hci_codec_caps caps[];
> +};
> +
> struct bdaddr_list_with_irk {
> 	struct list_head list;
> 	bdaddr_t bdaddr;
> @@ -535,6 +546,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 +1861,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/Makefile b/net/bluetooth/Makefile
> index cc0995301f93..f3e439d98b85 100644
> --- a/net/bluetooth/Makefile
> +++ b/net/bluetooth/Makefile
> @@ -14,7 +14,7 @@ bluetooth_6lowpan-y := 6lowpan.o
> 
> bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \
> 	hci_sock.o hci_sysfs.o l2cap_core.o l2cap_sock.o smp.o lib.o \
> -	ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o
> +	ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o hci_codec.o
> 
> bluetooth-$(CONFIG_BT_BREDR) += sco.o
> bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
> diff --git a/net/bluetooth/hci_codec.c b/net/bluetooth/hci_codec.c
> new file mode 100644
> index 000000000000..205f3b04c172
> --- /dev/null
> +++ b/net/bluetooth/hci_codec.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2021 Intel Corporation */
> +
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +static int hci_codec_list_add(struct list_head *list,
> +			      struct hci_op_read_local_codec_caps *sent,
> +			      struct hci_rp_read_local_codec_caps *rp,
> +			      void *caps,
> +			      __u32 len)
> +{
> +	struct codec_list *entry;
> +
> +	entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL);
> +	if (!entry)
> +		return -ENOMEM;
> +
> +	entry->id = sent->id;
> +	if (sent->id == 0xFF) {
> +		entry->cid = __le16_to_cpu(sent->cid);
> +		entry->vid = __le16_to_cpu(sent->vid);
> +	}
> +	entry->transport = sent->transport;
> +	entry->len = len;
> +	entry->num_caps = rp->num_caps;
> +	if (rp->num_caps)
> +		memcpy(entry->caps, caps, len);
> +	list_add(&entry->list, list);
> +
> +	return 0;
> +}
> +
> +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_vnd_codec)
> +{
> +	struct hci_op_read_local_codec_caps cmd;
> +	__u8 i;
> +
> +	memset(&cmd, 0, sizeof(cmd));
> +
> +	if (is_vnd_codec) {
> +		struct hci_vnd_codec *vnd_codec;
> +
> +		vnd_codec = codec_id;
> +		cmd.id = 0xFF;
> +		cmd.cid = vnd_codec->cid;
> +		cmd.vid = vnd_codec->vid;
> +	} else {
> +		cmd.id = *(__u8 *)codec_id;
> +	}
> +
> +	cmd.direction = 0x00;
> +
> +	for (i = 0; i < TRANSPORT_TYPE_MAX; i++) {
> +		if (transport & BIT(i)) {
> +			struct hci_rp_read_local_codec_caps *rp;
> +			struct hci_codec_caps *caps;
> +			struct sk_buff *skb;
> +			__u8 j;
> +			__u32 len;
> +
> +			cmd.transport = i;
> +			skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODEC_CAPS,
> +					     sizeof(cmd), &cmd,
> +					     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;
> +
> +			if (!rp->num_caps) {
> +				len = 0;
> +				/* this codec doesn't have capabilities */
> +				goto skip_caps_parse;
> +			}
> +
> +			skb_pull(skb, sizeof(*rp));
> +
> +			for (j = 0, len = 0; j < rp->num_caps; j++) {
> +				caps = (void *)skb->data;
> +				if (skb->len < sizeof(*caps))
> +					goto error;
> +				if (skb->len < caps->len)
> +					goto error;
> +				len += sizeof(caps->len) + caps->len;
> +				skb_pull(skb,  sizeof(caps->len) + caps->len);
> +			}
> +
> +skip_caps_parse:
> +			hci_dev_lock(hdev);
> +			hci_codec_list_add(&hdev->local_codecs, &cmd, rp,
> +					   (__u8 *)rp + sizeof(*rp), len);
> +			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_vnd_codec)
> +{
> +	__u8 i;
> +
> +	for (i = 0; i < num_codecs; i++) {
> +		if (!is_vnd_codec) {
> +			struct hci_std_codecs *codecs = codec_list;
> +
> +			hci_read_codec_capabilities(hdev, &codecs->codec[i],
> +						    LOCAL_CODEC_ACL_MASK,
> +						    is_vnd_codec);
> +		} else {
> +			struct hci_vnd_codecs *codecs = codec_list;
> +
> +			hci_read_codec_capabilities(hdev, &codecs->codec[i],
> +						    LOCAL_CODEC_ACL_MASK,
> +						    is_vnd_codec);
> +		}
> +	}
> +}

I said before that I don’t like it this way. The is_vnd_codec part is pointless and you are trickling this through. I really mean that I am not wanting to it this way upstream.

So first of all,

	if (is_vnd_codec) {
		int i;

		for (i = 0; i < num_codecs; i++) {
			struct hci_vnd_codecs *codecs = codec_list;
			hci_read_codec_capabilities(hdev, &codecs->codec[i],
						    LOCAL_CODEC_ACL_MASK, true);
		}
	} else {
		int I;

		for (i = 0; i < num_codecs; i++) {
			struct hci_std_codecs *codecs = codec_list;
			hci_read_codec_capabilities(hdev, &codecs->codec[i],
						    LOCAL_CODEC_ACL_MASK, false);
		}
	}

This is just by definition more efficient code. Now as caller you already know if you are a vendor codec list or not.

	static void hci_codec_list_parse_std(struct hci_dev *hdev, __u8 num_codecs,
					     struct hci_std_codecs *codecs)
	{
		int i;

		for (i = 0; i < num_codecs; i++)
			hci_read_codec_capabilities(hdev, &codecs->codec[i],
						    LOCAL_CODEC_ACL_MASK, false);
	}

	static void hci_codec_list_parse_vnd(struct hci_dev *hdev, __u8 num_codecs,
					     struct hci_vnd_codecs *codecs)
	{
		..
	}
	

This is already less code and easier to understand. Also it is type safer since the parameter is not a void pointer.

Now on to the LOCAL_CODEC_ACL_MASK transport setting and the hci_read_codec_capabilities. I have no idea why this is done this way. I can not follow this code since it is spaghetti left and right. This needs to be untangled.

I am not going to review hci_read_codec_capabilities right now, please look through it and come up with simpler solution.

> +
> +void hci_read_supported_codecs(struct hci_dev *hdev)
> +{
> +	struct sk_buff *skb;
> +	struct hci_rp_read_local_supported_codecs *rp;
> +	struct hci_std_codecs *std_codecs;
> +	struct hci_vnd_codecs *vnd_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;
> +
> +	/* validate codecs length before accessing */
> +	if (skb->len < flex_array_size(std_codecs, codec, std_codecs->num)
> +	    + sizeof(std_codecs->num))
> +		goto error;
> +
> +	/* enumerate codec capabilities of standard codecs */
> +	hci_codec_list_parse(hdev, std_codecs->num, std_codecs, false);


	for (i = 0; i < std_codecs->num; i++)
		hci_read_codec_capabilities(hdev, &std_codecs->codec[i],
					    LOCAL_CODEC_ACL_MASK, false);

Now I am saving the function above and only need 2 extra lines. This is more readable since it is easier to follow.

> +
> +	skb_pull(skb, flex_array_size(std_codecs, codec, std_codecs->num)
> +		 + sizeof(std_codecs->num));
> +
> +	vnd_codecs = (void *)skb->data;
> +
> +	/* validate vendor codecs length before accessing */
> +	if (skb->len <
> +	    flex_array_size(vnd_codecs, codec, vnd_codecs->num)
> +	    + sizeof(vnd_codecs->num))
> +		goto error;
> +
> +	/* enumerate vendor codec capabilities */
> +	hci_codec_list_parse(hdev, vnd_codecs->num, vnd_codecs, true);
> +
> +error:
> +	kfree_skb(skb);
> +}
> diff --git a/net/bluetooth/hci_codec.h b/net/bluetooth/hci_codec.h
> new file mode 100644
> index 000000000000..e5e594933d07
> --- /dev/null
> +++ b/net/bluetooth/hci_codec.h
> @@ -0,0 +1,6 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/* Copyright (C) 2021 Intel Corporation */
> +
> +void hci_read_supported_codecs(struct hci_dev *hdev);
> +void hci_codec_list_clear(struct list_head *codec_list);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 2560ed2f144d..212d5066d413 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -45,6 +45,7 @@
> #include "leds.h"
> #include "msft.h"
> #include "aosp.h"
> +#include "hci_codec.h"
> 
> static void hci_rx_work(struct work_struct *work);
> static void hci_cmd_work(struct work_struct *work);
> @@ -838,10 +839,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);
> @@ -937,6 +934,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.
> @@ -1841,6 +1842,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);
> 
> @@ -3842,7 +3844,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);
> 	INIT_WORK(&hdev->tx_work, hci_tx_work);

This kind of style changes by removing an on purpose place empty line as separator are not acceptable. Please avoid them.

Regards

Marcel


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

* Re: [PATCH v11 02/10] Bluetooth: Add support for Read Local Supported Codecs V2
  2021-07-27  8:47 ` [PATCH v11 02/10] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
@ 2021-07-30 14:07   ` Marcel Holtmann
  0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2021-07-30 14:07 UTC (permalink / raw)
  To: Kiran K; +Cc: BlueZ, Srivatsa, Ravishankar, chethan.tumkur.narayan

Hi Kiran,

> 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 v11:
>  - Remove Kconfig related changes
>  - Move codec related functions to hci_codec.c
> 
> * changes in v10:
>  no changes
> 
> * changes in v9:
>  use vnd as shortcut name for vendor instead of ven
> 
> * changes in v8:
>  no changes
> 
> * 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 | 29 +++++++++++++++
> net/bluetooth/hci_codec.c   | 74 +++++++++++++++++++++++++++++++++++++
> net/bluetooth/hci_codec.h   |  1 +
> net/bluetooth/hci_core.c    |  4 +-
> 4 files changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index f76849c8eafd..b4e35cf5f4b1 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1337,6 +1337,35 @@ struct hci_rp_read_local_pairing_opts {
> 	__u8     max_key_size;
> } __packed;
> 
> +#define HCI_OP_READ_LOCAL_CODECS_V2	0x100d
> +struct hci_std_codec_v2 {
> +	__u8	id;
> +	__u8	transport;
> +} __packed;
> +
> +struct hci_std_codecs_v2 {
> +	__u8	num;
> +	struct hci_std_codec_v2 codec[];
> +} __packed;
> +
> +struct hci_vnd_codec_v2 {
> +	__u8	id;
> +	__le16	cid;
> +	__le16	vid;
> +	__u8	transport;
> +} __packed;
> +
> +struct hci_vnd_codecs_v2 {
> +	__u8	num;
> +	struct hci_vnd_codec_v2 codec[];
> +} __packed;
> +
> +struct hci_rp_read_local_supported_codecs_v2 {
> +	__u8	status;
> +	struct hci_std_codecs_v2 std_codecs;
> +	struct hci_vnd_codecs_v2 vendor_codecs;
> +} __packed;
> +
> #define HCI_OP_READ_LOCAL_CODEC_CAPS	0x100e
> struct hci_op_read_local_codec_caps {
> 	__u8	id;
> diff --git a/net/bluetooth/hci_codec.c b/net/bluetooth/hci_codec.c
> index 205f3b04c172..6f20a4b1fc9c 100644
> --- a/net/bluetooth/hci_codec.c
> +++ b/net/bluetooth/hci_codec.c
> @@ -192,3 +192,77 @@ void hci_read_supported_codecs(struct hci_dev *hdev)
> error:
> 	kfree_skb(skb);
> }
> +
> +static void hci_codec_list_parse_v2(struct hci_dev *hdev, __u8 num_codecs,
> +				    void *codec_list, bool is_vnd_codec)
> +{
> +	__u8 i;
> +
> +	for (i = 0; i < num_codecs; i++) {
> +		if (!is_vnd_codec) {
> +			struct hci_std_codecs_v2 *codecs = codec_list;
> +
> +			hci_read_codec_capabilities(hdev, &codecs->codec[i],
> +						    codecs->codec[i].transport,
> +						    is_vnd_codec);
> +		} else {
> +			struct hci_vnd_codecs_v2 *codecs = codec_list;
> +
> +			hci_read_codec_capabilities(hdev, &codecs->codec[i],
> +						    codecs->codec[i].transport,
> +						    is_vnd_codec);
> +		}
> +	}
> +}
> +
> +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_std_codecs_v2 *std_codecs;
> +	struct hci_vnd_codecs_v2 *vnd_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;
> +
> +	/* check for payload data length before accessing */
> +	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));
> +
> +	vnd_codecs = (void *)skb->data;
> +
> +	/* check for payload data length before accessing */
> +	if (skb->len <
> +	    flex_array_size(vnd_codecs, codec, vnd_codecs->num)
> +	    + sizeof(vnd_codecs->num))
> +		goto error;
> +
> +	hci_codec_list_parse_v2(hdev, vnd_codecs->num, vnd_codecs, true);
> +
> +error:
> +	kfree_skb(skb);
> +}

once you apply my comments from the previous patch to this one as well, you will see that it looks a lot less complicated and entangled.

Regards

Marcel


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

* Re: [PATCH v11 03/10] Bluetooth: btintel: Read supported offload usecases
  2021-07-27  8:47 ` [PATCH v11 03/10] Bluetooth: btintel: Read supported offload usecases Kiran K
@ 2021-07-30 14:10   ` Marcel Holtmann
  2021-08-14  6:32     ` K, Kiran
  0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2021-07-30 14:10 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth, ravishankar.srivatsa, chethan.tumkur.narayan

Hi Kiran,

> Read offload usecases supported by controller.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> * changes in v11:
>  - Remove Kconfig related changes
> * changes in v10:
>  - restructure patch to have  definition and call of callaback in the
>    same patch
> * changes in v9:
>  - define a separate patch for core changes
> 
> drivers/bluetooth/btintel.c | 30 ++++++++++++++++++++++++++++++
> drivers/bluetooth/btintel.h | 10 ++++++++++
> drivers/bluetooth/btusb.c   |  3 +++
> 3 files changed, 43 insertions(+)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index e44b6993cf91..a6b81914766e 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -1272,6 +1272,36 @@ int btintel_set_debug_features(struct hci_dev *hdev,
> }
> EXPORT_SYMBOL_GPL(btintel_set_debug_features);
> 
> +int btintel_configure_offload_usecases(struct hci_dev *hdev)
> +{
> +	struct sk_buff *skb;
> +	int err = 0;
> +	struct intel_offload_usecases *usecases;
> +
> +	skb = __hci_cmd_sync(hdev, 0xfc86, 0, NULL, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		bt_dev_err(hdev, "Reading offload usecases failed (%ld)",
> +			   PTR_ERR(skb));
> +		return PTR_ERR(skb);
> +	}
> +
> +	if (skb->len < sizeof(*usecases)) {
> +		err = -EIO;
> +		goto error;
> +	}
> +
> +	usecases = (void *)skb->data;
> +
> +	if (usecases->status) {
> +		err = -bt_to_errno(skb->data[0]);
> +		goto error;
> +	}
> +error:
> +	kfree_skb(skb);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(btintel_configure_offload_usecases);
> +
> 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 d184064a5e7c..45ca5c4ccf37 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -132,6 +132,11 @@ struct intel_debug_features {
> 	__u8    page1[16];
> } __packed;
> 
> +struct intel_offload_usecases {
> +	__u8	status;
> +	__u8	preset[8];
> +} __packed;
> +
> #define INTEL_HW_PLATFORM(cnvx_bt)	((u8)(((cnvx_bt) & 0x0000ff00) >> 8))
> #define INTEL_HW_VARIANT(cnvx_bt)	((u8)(((cnvx_bt) & 0x003f0000) >> 16))
> #define INTEL_CNVX_TOP_TYPE(cnvx_top)	((cnvx_top) & 0x00000fff)
> @@ -175,6 +180,7 @@ int btintel_read_debug_features(struct hci_dev *hdev,
> 				struct intel_debug_features *features);
> int btintel_set_debug_features(struct hci_dev *hdev,
> 			       const struct intel_debug_features *features);
> +int btintel_configure_offload_usecases(struct hci_dev *hdev);

so usecases is not, it would be offload_use_cases. However btintel_configure_offload is just fine as a function name. With the work from Tedd, this would be removed anyway and all done internally via the hdev->setup().

Regards

Marcel


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

* Re: [PATCH v11 04/10] Bluetooth: Allow querying of supported offload codecs over SCO socket
  2021-07-27  8:47 ` [PATCH v11 04/10] Bluetooth: Allow querying of supported offload codecs over SCO socket Kiran K
@ 2021-07-30 14:14   ` Marcel Holtmann
  2021-08-14  6:33     ` K, Kiran
  0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2021-07-30 14:14 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth, ravishankar.srivatsa, chethan.tumkur.narayan

Hi Kiran,

> Add BT_CODEC option for getsockopt systemcall to get the details
> of offload codecs supported over SCO socket
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> * changes in v11:
>  - Remove Kconfig related changes
> * changes in v10:
>  - In getsockopt, prepare data in advance and call copy_to_user
>    istead of calling put_user on each field of struct bt_codec
> 
> include/net/bluetooth/bluetooth.h | 20 +++++++
> include/net/bluetooth/hci.h       |  4 ++
> include/net/bluetooth/hci_core.h  |  1 +
> net/bluetooth/sco.c               | 91 +++++++++++++++++++++++++++++++
> 4 files changed, 116 insertions(+)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 9125effbf448..64cddff0c9c4 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -153,6 +153,26 @@ struct bt_voice {
> 
> #define BT_SCM_PKT_STATUS	0x03
> 
> +#define BT_CODEC	19
> +
> +struct	bt_codec_caps {
> +	__u8	len;
> +	__u8	data[];
> +} __packed;
> +
> +struct bt_codec {
> +	__u8	id;
> +	__u16	cid;
> +	__u16	vid;
> +	__u8	data_path;
> +	__u8	num_caps;
> +} __packed;
> +
> +struct bt_codecs {
> +	__u8		num_codecs;
> +	struct bt_codec	codecs[];
> +} __packed;
> +
> __printf(1, 2)
> void bt_info(const char *fmt, ...);
> __printf(1, 2)
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index b4e35cf5f4b1..22e872e60ff8 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -2621,6 +2621,10 @@ static inline struct hci_sco_hdr *hci_sco_hdr(const struct sk_buff *skb)
> #define hci_iso_data_len(h)		((h) & 0x3fff)
> #define hci_iso_data_flags(h)		((h) >> 14)
> 
> +/* codec transport types */
> +#define TRANSPORT_ACL		0x00
> +#define TRANSPORT_SCO_ESCO	0x01
> +
> /* le24 support */
> static inline void hci_cpu_to_le24(__u32 val, __u8 dst[3])
> {
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 6742e6ad7b37..71c409c8ab34 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -617,6 +617,7 @@ struct hci_dev {
> 	int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> 	void (*cmd_timeout)(struct hci_dev *hdev);
> 	bool (*prevent_wake)(struct hci_dev *hdev);
> +	int (*get_data_path_id)(struct hci_dev *hdev, __u8 *data_path);
> };
> 
> #define HCI_PHY_HANDLE(handle)	(handle & 0xff)
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index ffa2a77a3e4c..2f32693f09c1 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -953,6 +953,12 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
> 	struct bt_voice voice;
> 	u32 phys;
> 	int pkt_status;
> +	int buf_len;
> +	struct codec_list *c;
> +	u8 num_codecs, i, __user *ptr;
> +	struct hci_dev *hdev;
> +	struct hci_codec_caps *caps;
> +	struct bt_codec codec;
> 
> 	BT_DBG("sk %p", sk);
> 
> @@ -1017,6 +1023,91 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
> 			err = -EFAULT;
> 		break;
> 
> +	case BT_CODEC:
> +		num_codecs = 0;
> +		buf_len = 0;
> +
> +		hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src, BDADDR_BREDR);
> +		if (!hdev) {
> +			err = -EBADFD;
> +			break;
> +		}
> +
> +		if (!hdev->get_data_path_id) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}

so I did say that this needs to be put behind an experimental feature setting. I am not committing to an userspace API until we have more feedback.

Regards

Marcel


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

* Re: [PATCH v11 06/10] Bluetooth: Allow setting of codec for HFP offload usecase
  2021-07-27  8:47 ` [PATCH v11 06/10] Bluetooth: Allow setting of codec for HFP offload usecase Kiran K
@ 2021-07-30 14:17   ` Marcel Holtmann
  2021-08-01 23:45     ` K, Kiran
  0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2021-07-30 14:17 UTC (permalink / raw)
  To: Kiran K; +Cc: BlueZ, ravishankar.srivatsa, chethan.tumkur.narayan

Hi Kiran,

> For HFP offload usecase, controller needs to be configured
> with codec data and capabilities. This patch uses Bluetooth
> SIG defined command HCI_CONFIGURE_DATA_PATH to specify vendor
> specific data and allows userspace modules to set the codec
> via setsockopt systemcall.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> * changes in v11:
>  - Remove changes related to Kconfig
> * changes in v10:
>  - patch refactor - having callback definition and usage in the same patch
> 
> include/net/bluetooth/bluetooth.h |   2 +
> include/net/bluetooth/hci.h       |   8 ++
> include/net/bluetooth/hci_core.h  |   3 +
> net/bluetooth/sco.c               | 118 ++++++++++++++++++++++++++++++
> 4 files changed, 131 insertions(+)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 64cddff0c9c4..1a48b6732eef 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -173,6 +173,8 @@ struct bt_codecs {
> 	struct bt_codec	codecs[];
> } __packed;
> 
> +#define CODING_FORMAT_CVSD	0x02
> +
> __printf(1, 2)
> void bt_info(const char *fmt, ...);
> __printf(1, 2)
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 22e872e60ff8..c998607fc517 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1250,6 +1250,14 @@ struct hci_rp_read_local_oob_ext_data {
> 	__u8     rand256[16];
> } __packed;
> 
> +#define HCI_CONFIGURE_DATA_PATH	0x0c83
> +struct hci_op_configure_data_path {
> +	__u8	direction;
> +	__u8	data_path_id;
> +	__u8	vnd_len;
> +	__u8	vnd_data[];
> +} __packed;
> +
> #define HCI_OP_READ_LOCAL_VERSION	0x1001
> struct hci_rp_read_local_version {
> 	__u8     status;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 71c409c8ab34..eafa6f8114cb 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -618,6 +618,9 @@ struct hci_dev {
> 	void (*cmd_timeout)(struct hci_dev *hdev);
> 	bool (*prevent_wake)(struct hci_dev *hdev);
> 	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,
> +				     __u8 **vnd_data);

why are these two independent callbacks? I seem to remember saying that it looks like we only need one.

Regards

Marcel


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

* Re: [PATCH v11 10/10] Bluetooth: Add offload feature under experimental flag
  2021-07-27  8:47 ` [PATCH v11 10/10] Bluetooth: Add offload feature under experimental flag Kiran K
@ 2021-07-30 14:22   ` Marcel Holtmann
  2021-08-14  6:42     ` K, Kiran
  0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2021-07-30 14:22 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth, ravishankar.srivatsa, chethan.tumkur.narayan

Hi Kiran,

> Allow user level process to enable / disable codec offload
> feature through mgmt interface. By default offload codec feature
> is disabled.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> changes in v11:
> - Remove Kconfig
> 
> include/net/bluetooth/hci.h |  1 +
> net/bluetooth/mgmt.c        | 98 ++++++++++++++++++++++++++++++++++++-
> net/bluetooth/sco.c         | 10 ++++
> 3 files changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 8af8892e18bc..dbbbb974ade6 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -330,6 +330,7 @@ enum {
> 	HCI_ENABLE_LL_PRIVACY,
> 	HCI_CMD_PENDING,
> 	HCI_FORCE_NO_MITM,
> +	HCI_OFFLOAD_CODECS_ENABLED,
> 
> 	__HCI_NUM_FLAGS,
> };
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3663f880df11..eb10f9597837 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3791,6 +3791,12 @@ static const u8 debug_uuid[16] = {
> };
> #endif
> 
> +/* a6695ace-ee7f-4fb9-881a-5fac66c629af */
> +static const u8 offload_codecs_uuid[16] = {
> +	0xaf, 0x29, 0xc6, 0x66, 0xac, 0x5f, 0x1a, 0x88,
> +	0xb9, 0x4f, 0x7f, 0xee, 0xce, 0x5a, 0x69, 0xa6,
> +};
> +
> /* 671b10b5-42c0-4696-9227-eb28d1b049d6 */
> static const u8 simult_central_periph_uuid[16] = {
> 	0xd6, 0x49, 0xb0, 0xd1, 0x28, 0xeb, 0x27, 0x92,
> @@ -3806,7 +3812,7 @@ static const u8 rpa_resolution_uuid[16] = {
> static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> 				  void *data, u16 data_len)
> {
> -	char buf[62];	/* Enough space for 3 features */
> +	char buf[82];   /* Enough space for 4 features: 2 + 20 * 4 */
> 	struct mgmt_rp_read_exp_features_info *rp = (void *)buf;
> 	u16 idx = 0;
> 	u32 flags;
> @@ -3850,6 +3856,26 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> 		idx++;
> 	}
> 
> +	if (hdev) {
> +		if (hdev->get_data_path_id) {
> +			/* BIT(0): indicating if offload codecs are
> +			 * supported by controller.
> +			 */
> +			flags = BIT(0);
> +
> +			/* BIT(1): indicating if codec offload feature
> +			 * is enabled.
> +			 */
> +			if (hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED))
> +				flags |= BIT(1);
> +		} else {
> +			flags = 0;
> +		}
> +		memcpy(rp->features[idx].uuid, offload_codecs_uuid, 16);
> +		rp->features[idx].flags = cpu_to_le32(flags);
> +		idx++;
> +	}
> +
> 	rp->feature_count = cpu_to_le16(idx);
> 
> 	/* After reading the experimental features information, enable
> @@ -3892,6 +3918,21 @@ static int exp_debug_feature_changed(bool enabled, struct sock *skip)
> }
> #endif
> 
> +static int exp_offload_codec_feature_changed(bool enabled, struct sock *skip)
> +{
> +	struct mgmt_ev_exp_feature_changed ev;
> +
> +	BT_INFO("enabled %d", enabled);
> +
> +	memset(&ev, 0, sizeof(ev));
> +	memcpy(ev.uuid, offload_codecs_uuid, 16);
> +	ev.flags = cpu_to_le32(enabled ? BIT(0) : 0);
> +
> +	return mgmt_limited_event(MGMT_EV_EXP_FEATURE_CHANGED, NULL,
> +				  &ev, sizeof(ev),
> +				  HCI_MGMT_EXP_FEATURE_EVENTS, skip);
> +}
> +
> static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> 			   void *data, u16 data_len)
> {
> @@ -4038,6 +4079,61 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> 		return err;
> 	}
> 
> +	if (!memcmp(cp->uuid, offload_codecs_uuid, 16)) {
> +		bool val, changed;
> +		int err;
> +
> +		/* Command requires to use a valid controller index */
> +		if (!hdev)
> +			return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
> +					       MGMT_OP_SET_EXP_FEATURE,
> +					       MGMT_STATUS_INVALID_INDEX);
> +
> +		/* Parameters are limited to a single octet */
> +		if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
> +			return mgmt_cmd_status(sk, hdev->id,
> +					       MGMT_OP_SET_EXP_FEATURE,
> +					       MGMT_STATUS_INVALID_PARAMS);
> +
> +		/* Only boolean on/off is supported */
> +		if (cp->param[0] != 0x00 && cp->param[0] != 0x01)
> +			return mgmt_cmd_status(sk, hdev->id,
> +					       MGMT_OP_SET_EXP_FEATURE,
> +					       MGMT_STATUS_INVALID_PARAMS);
> +
> +		val = !!cp->param[0];
> +		changed = (val != hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED));
> +
> +		if (!hdev->get_data_path_id) {
> +			bt_dev_info(hdev, "offload codecs not supported");
> +			return mgmt_cmd_status(sk, hdev->id,
> +					       MGMT_OP_SET_EXP_FEATURE,
> +					       MGMT_STATUS_NOT_SUPPORTED);
> +		}
> +
> +		if (changed) {
> +			if (val)
> +				hci_dev_set_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED);
> +			else
> +				hci_dev_clear_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED);
> +		}
> +
> +		bt_dev_info(hdev, "offload codecs enable %d changed %d",
> +			    val, changed);
> +
> +		memcpy(rp.uuid, offload_codecs_uuid, 16);
> +		rp.flags = cpu_to_le32(val ? BIT(0) : 0);
> +		hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
> +		err = mgmt_cmd_complete(sk, hdev->id,
> +					MGMT_OP_SET_EXP_FEATURE, 0,
> +					&rp, sizeof(rp));
> +
> +		if (changed)
> +			exp_offload_codec_feature_changed(val, sk);
> +
> +		return err;
> +	}
> +
> 	return mgmt_cmd_status(sk, hdev ? hdev->id : MGMT_INDEX_NONE,
> 			       MGMT_OP_SET_EXP_FEATURE,
> 			       MGMT_STATUS_NOT_SUPPORTED);
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 5ff06ac05fef..ba66e082ba9e 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -964,6 +964,11 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
> 			break;
> 		}
> 
> +		if (!hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED)) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}
> +
> 		if (!hdev->get_codec_config_data) {
> 			err = -EOPNOTSUPP;
> 			break;
> @@ -1160,6 +1165,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
> 			break;
> 		}
> 
> +		if (!hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED)) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}
> +
> 		if (!hdev->get_data_path_id) {
> 			err = -EOPNOTSUPP;
> 			break;

these two and the flag definition itself need to be a lot earlier in the series.

Regards

Marcel


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

* RE: [PATCH v11 06/10] Bluetooth: Allow setting of codec for HFP offload usecase
  2021-07-30 14:17   ` Marcel Holtmann
@ 2021-08-01 23:45     ` K, Kiran
  2021-08-14  6:39       ` K, Kiran
  0 siblings, 1 reply; 25+ messages in thread
From: K, Kiran @ 2021-08-01 23:45 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: BlueZ, Srivatsa, Ravishankar, Tumkur Narayan, Chethan

Hi Marcel,

> 
> Hi Kiran,
> 
> > For HFP offload usecase, controller needs to be configured with codec
> > data and capabilities. This patch uses Bluetooth SIG defined command
> > HCI_CONFIGURE_DATA_PATH to specify vendor specific data and allows
> > userspace modules to set the codec via setsockopt systemcall.
> >
> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > ---
> > * changes in v11:
> >  - Remove changes related to Kconfig
> > * changes in v10:
> >  - patch refactor - having callback definition and usage in the same
> > patch
> >
> > include/net/bluetooth/bluetooth.h |   2 +
> > include/net/bluetooth/hci.h       |   8 ++
> > include/net/bluetooth/hci_core.h  |   3 +
> > net/bluetooth/sco.c               | 118 ++++++++++++++++++++++++++++++
> > 4 files changed, 131 insertions(+)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h
> > b/include/net/bluetooth/bluetooth.h
> > index 64cddff0c9c4..1a48b6732eef 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -173,6 +173,8 @@ struct bt_codecs {
> > 	struct bt_codec	codecs[];
> > } __packed;
> >
> > +#define CODING_FORMAT_CVSD	0x02
> > +
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > __printf(1, 2)
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 22e872e60ff8..c998607fc517 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -1250,6 +1250,14 @@ struct hci_rp_read_local_oob_ext_data {
> > 	__u8     rand256[16];
> > } __packed;
> >
> > +#define HCI_CONFIGURE_DATA_PATH	0x0c83
> > +struct hci_op_configure_data_path {
> > +	__u8	direction;
> > +	__u8	data_path_id;
> > +	__u8	vnd_len;
> > +	__u8	vnd_data[];
> > +} __packed;
> > +
> > #define HCI_OP_READ_LOCAL_VERSION	0x1001
> > struct hci_rp_read_local_version {
> > 	__u8     status;
> > diff --git a/include/net/bluetooth/hci_core.h
> > b/include/net/bluetooth/hci_core.h
> > index 71c409c8ab34..eafa6f8114cb 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -618,6 +618,9 @@ struct hci_dev {
> > 	void (*cmd_timeout)(struct hci_dev *hdev);
> > 	bool (*prevent_wake)(struct hci_dev *hdev);
> > 	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,
> > +				     __u8 **vnd_data);
> 
> why are these two independent callbacks? I seem to remember saying that it
> looks like we only need one.

get_data_path_id,  gets called during getsockopt(BT_CODEC,...) when populating codec details to user space.

get_codec_config_data, gets called during setsockopt(BT_CODEC,...) 

> 
> Regards
> 
> Marcel

Regards,
Kiran



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

* RE: [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details
  2021-07-30 14:06 ` [PATCH v11 01/10] " Marcel Holtmann
@ 2021-08-02  0:23   ` K, Kiran
  0 siblings, 0 replies; 25+ messages in thread
From: K, Kiran @ 2021-08-02  0:23 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: BlueZ, Srivatsa, Ravishankar, Tumkur Narayan, Chethan

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Friday, July 30, 2021 7:36 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: BlueZ <linux-bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>
> Subject: Re: [PATCH v11 01/10] Bluetooth: Enumerate local supported codec
> and cache details
> 
> 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>
> > ---
> > * changes in v11:
> > - Remove Kconfig related changes
> > - Address minor review comments
> > - Move codec related functions to new file hci_codec.c
> >
> > * changes in v10:
> > - define Kconfig to control offload feature at build time
> > - fix review comments
> >
> > * changes in v9:
> >  - use shortname vnd instead of ven
> >
> > * changes in v8:
> >  - add comments
> >  - split __u8 codec_id[5] into {__u8 id; __le16 cid, vid }
> >  - address review comment related codec caps structure
> >
> > * 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      |  41 +++++++
> > include/net/bluetooth/hci_core.h |  17 +++
> > net/bluetooth/Makefile           |   2 +-
> > net/bluetooth/hci_codec.c        | 194 +++++++++++++++++++++++++++++++
> > net/bluetooth/hci_codec.h        |   6 +
> > net/bluetooth/hci_core.c         |  12 +-
> > 6 files changed, 266 insertions(+), 6 deletions(-) create mode 100644
> > net/bluetooth/hci_codec.c create mode 100644 net/bluetooth/hci_codec.h
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index b80415011dcd..f76849c8eafd 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -1307,6 +1307,28 @@ struct hci_rp_read_data_block_size { }
> > __packed;
> >
> > #define HCI_OP_READ_LOCAL_CODECS	0x100b
> > +struct hci_std_codecs {
> > +	__u8	num;
> > +	__u8	codec[];
> > +} __packed;
> > +
> > +struct hci_vnd_codec {
> > +	/* company id */
> > +	__le16	cid;
> > +	/* vendor codec id */
> > +	__le16	vid;
> > +} __packed;
> > +
> > +struct hci_vnd_codecs {
> > +	__u8	num;
> > +	struct hci_vnd_codec codec[];
> > +} __packed;
> > +
> > +struct hci_rp_read_local_supported_codecs {
> > +	__u8	status;
> > +	struct hci_std_codecs std_codecs;
> > +	struct hci_vnd_codecs vnd_codecs;
> > +} __packed;
> >
> > #define HCI_OP_READ_LOCAL_PAIRING_OPTS	0x100c
> > struct hci_rp_read_local_pairing_opts { @@ -1315,6 +1337,25 @@ 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	id;
> > +	__le16	cid;
> > +	__le16	vid;
> > +	__u8	transport;
> > +	__u8	direction;
> > +} __packed;
> > +
> > +struct hci_codec_caps {
> > +	__u8	len;
> > +	__u8	data[];
> > +} __packed;
> > +
> > +struct hci_rp_read_local_codec_caps {
> > +	__u8	status;
> > +	__u8	num_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 a53e94459ecd..6742e6ad7b37 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -131,6 +131,17 @@ struct bdaddr_list {
> > 	u8 bdaddr_type;
> > };
> >
> > +struct codec_list {
> > +	struct list_head list;
> > +	u8	id;
> > +	__u16	cid;
> > +	__u16	vid;
> > +	u8	transport;
> > +	u8	num_caps;
> > +	u32	len;
> > +	struct hci_codec_caps caps[];
> > +};
> > +
> > struct bdaddr_list_with_irk {
> > 	struct list_head list;
> > 	bdaddr_t bdaddr;
> > @@ -535,6 +546,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 +1861,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/Makefile b/net/bluetooth/Makefile index
> > cc0995301f93..f3e439d98b85 100644
> > --- a/net/bluetooth/Makefile
> > +++ b/net/bluetooth/Makefile
> > @@ -14,7 +14,7 @@ bluetooth_6lowpan-y := 6lowpan.o
> >
> > bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \
> > 	hci_sock.o hci_sysfs.o l2cap_core.o l2cap_sock.o smp.o lib.o \
> > -	ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o
> > +	ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o hci_codec.o
> >
> > bluetooth-$(CONFIG_BT_BREDR) += sco.o
> > bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o diff --git
> > a/net/bluetooth/hci_codec.c b/net/bluetooth/hci_codec.c new file mode
> > 100644 index 000000000000..205f3b04c172
> > --- /dev/null
> > +++ b/net/bluetooth/hci_codec.c
> > @@ -0,0 +1,194 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (C) 2021 Intel Corporation */
> > +
> > +#include <net/bluetooth/bluetooth.h>
> > +#include <net/bluetooth/hci_core.h>
> > +
> > +static int hci_codec_list_add(struct list_head *list,
> > +			      struct hci_op_read_local_codec_caps *sent,
> > +			      struct hci_rp_read_local_codec_caps *rp,
> > +			      void *caps,
> > +			      __u32 len)
> > +{
> > +	struct codec_list *entry;
> > +
> > +	entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL);
> > +	if (!entry)
> > +		return -ENOMEM;
> > +
> > +	entry->id = sent->id;
> > +	if (sent->id == 0xFF) {
> > +		entry->cid = __le16_to_cpu(sent->cid);
> > +		entry->vid = __le16_to_cpu(sent->vid);
> > +	}
> > +	entry->transport = sent->transport;
> > +	entry->len = len;
> > +	entry->num_caps = rp->num_caps;
> > +	if (rp->num_caps)
> > +		memcpy(entry->caps, caps, len);
> > +	list_add(&entry->list, list);
> > +
> > +	return 0;
> > +}
> > +
> > +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_vnd_codec) {
> > +	struct hci_op_read_local_codec_caps cmd;
> > +	__u8 i;
> > +
> > +	memset(&cmd, 0, sizeof(cmd));
> > +
> > +	if (is_vnd_codec) {
> > +		struct hci_vnd_codec *vnd_codec;
> > +
> > +		vnd_codec = codec_id;
> > +		cmd.id = 0xFF;
> > +		cmd.cid = vnd_codec->cid;
> > +		cmd.vid = vnd_codec->vid;
> > +	} else {
> > +		cmd.id = *(__u8 *)codec_id;
> > +	}
> > +
> > +	cmd.direction = 0x00;
> > +
> > +	for (i = 0; i < TRANSPORT_TYPE_MAX; i++) {
> > +		if (transport & BIT(i)) {
> > +			struct hci_rp_read_local_codec_caps *rp;
> > +			struct hci_codec_caps *caps;
> > +			struct sk_buff *skb;
> > +			__u8 j;
> > +			__u32 len;
> > +
> > +			cmd.transport = i;
> > +			skb = __hci_cmd_sync(hdev,
> HCI_OP_READ_LOCAL_CODEC_CAPS,
> > +					     sizeof(cmd), &cmd,
> > +					     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;
> > +
> > +			if (!rp->num_caps) {
> > +				len = 0;
> > +				/* this codec doesn't have capabilities */
> > +				goto skip_caps_parse;
> > +			}
> > +
> > +			skb_pull(skb, sizeof(*rp));
> > +
> > +			for (j = 0, len = 0; j < rp->num_caps; j++) {
> > +				caps = (void *)skb->data;
> > +				if (skb->len < sizeof(*caps))
> > +					goto error;
> > +				if (skb->len < caps->len)
> > +					goto error;
> > +				len += sizeof(caps->len) + caps->len;
> > +				skb_pull(skb,  sizeof(caps->len) + caps->len);
> > +			}
> > +
> > +skip_caps_parse:
> > +			hci_dev_lock(hdev);
> > +			hci_codec_list_add(&hdev->local_codecs, &cmd, rp,
> > +					   (__u8 *)rp + sizeof(*rp), len);
> > +			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_vnd_codec) {
> > +	__u8 i;
> > +
> > +	for (i = 0; i < num_codecs; i++) {
> > +		if (!is_vnd_codec) {
> > +			struct hci_std_codecs *codecs = codec_list;
> > +
> > +			hci_read_codec_capabilities(hdev, &codecs-
> >codec[i],
> > +						    LOCAL_CODEC_ACL_MASK,
> > +						    is_vnd_codec);
> > +		} else {
> > +			struct hci_vnd_codecs *codecs = codec_list;
> > +
> > +			hci_read_codec_capabilities(hdev, &codecs-
> >codec[i],
> > +						    LOCAL_CODEC_ACL_MASK,
> > +						    is_vnd_codec);
> > +		}
> > +	}
> > +}
> 
> I said before that I don’t like it this way. The is_vnd_codec part is pointless
> and you are trickling this through. I really mean that I am not wanting to it
> this way upstream.
> 
> So first of all,
> 
> 	if (is_vnd_codec) {
> 		int i;
> 
> 		for (i = 0; i < num_codecs; i++) {
> 			struct hci_vnd_codecs *codecs = codec_list;
> 			hci_read_codec_capabilities(hdev, &codecs-
> >codec[i],
> 						    LOCAL_CODEC_ACL_MASK,
> true);
> 		}
> 	} else {
> 		int I;
> 
> 		for (i = 0; i < num_codecs; i++) {
> 			struct hci_std_codecs *codecs = codec_list;
> 			hci_read_codec_capabilities(hdev, &codecs-
> >codec[i],
> 						    LOCAL_CODEC_ACL_MASK,
> false);
> 		}
> 	}
> 
> This is just by definition more efficient code. Now as caller you already know
> if you are a vendor codec list or not.
> 
> 	static void hci_codec_list_parse_std(struct hci_dev *hdev, __u8
> num_codecs,
> 					     struct hci_std_codecs *codecs)
> 	{
> 		int i;
> 
> 		for (i = 0; i < num_codecs; i++)
> 			hci_read_codec_capabilities(hdev, &codecs-
> >codec[i],
> 						    LOCAL_CODEC_ACL_MASK,
> false);
> 	}
> 
> 	static void hci_codec_list_parse_vnd(struct hci_dev *hdev, __u8
> num_codecs,
> 					     struct hci_vnd_codecs *codecs)
> 	{
> 		..
> 	}
> 
> 
> This is already less code and easier to understand. Also it is type safer since
> the parameter is not a void pointer.

Ack.

> 
> Now on to the LOCAL_CODEC_ACL_MASK transport setting and the
> hci_read_codec_capabilities. I have no idea why this is done this way. I can
> not follow this code since it is spaghetti left and right. This needs to be
> untangled.
> 

HCI_READ_LOCAL_SUPPORTED_CODEC_CAPABILITIES command requires transport type to be specified.  In HCI_READ_LOCAL_SUPPORTED_CODECS_V1, by default transport type is BREDR_ACL. Hence in this patch, caller is explicitly passing LOCAL_CODEC_ACL_MASK.

In patch 02/10, transport bit mask received in HCI_READ_LOCAL_SUPPORTED_CODECS_V2, is passed.

> I am not going to review hci_read_codec_capabilities right now, please look
> through it and come up with simpler solution.
> 
> > +
> > +void hci_read_supported_codecs(struct hci_dev *hdev) {
> > +	struct sk_buff *skb;
> > +	struct hci_rp_read_local_supported_codecs *rp;
> > +	struct hci_std_codecs *std_codecs;
> > +	struct hci_vnd_codecs *vnd_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;
> > +
> > +	/* validate codecs length before accessing */
> > +	if (skb->len < flex_array_size(std_codecs, codec, std_codecs->num)
> > +	    + sizeof(std_codecs->num))
> > +		goto error;
> > +
> > +	/* enumerate codec capabilities of standard codecs */
> > +	hci_codec_list_parse(hdev, std_codecs->num, std_codecs, false);
> 
> 
> 	for (i = 0; i < std_codecs->num; i++)
> 		hci_read_codec_capabilities(hdev, &std_codecs->codec[i],
> 					    LOCAL_CODEC_ACL_MASK, false);
> 
> Now I am saving the function above and only need 2 extra lines. This is more
> readable since it is easier to follow.
> 
> > +
> > +	skb_pull(skb, flex_array_size(std_codecs, codec, std_codecs->num)
> > +		 + sizeof(std_codecs->num));
> > +
> > +	vnd_codecs = (void *)skb->data;
> > +
> > +	/* validate vendor codecs length before accessing */
> > +	if (skb->len <
> > +	    flex_array_size(vnd_codecs, codec, vnd_codecs->num)
> > +	    + sizeof(vnd_codecs->num))
> > +		goto error;
> > +
> > +	/* enumerate vendor codec capabilities */
> > +	hci_codec_list_parse(hdev, vnd_codecs->num, vnd_codecs, true);
> > +
> > +error:
> > +	kfree_skb(skb);
> > +}
> > diff --git a/net/bluetooth/hci_codec.h b/net/bluetooth/hci_codec.h new
> > file mode 100644 index 000000000000..e5e594933d07
> > --- /dev/null
> > +++ b/net/bluetooth/hci_codec.h
> > @@ -0,0 +1,6 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/* Copyright (C) 2021 Intel Corporation */
> > +
> > +void hci_read_supported_codecs(struct hci_dev *hdev); void
> > +hci_codec_list_clear(struct list_head *codec_list);
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index
> > 2560ed2f144d..212d5066d413 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -45,6 +45,7 @@
> > #include "leds.h"
> > #include "msft.h"
> > #include "aosp.h"
> > +#include "hci_codec.h"
> >
> > static void hci_rx_work(struct work_struct *work); static void
> > hci_cmd_work(struct work_struct *work); @@ -838,10 +839,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); @@ -937,6
> > +934,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.
> > @@ -1841,6 +1842,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);
> >
> > @@ -3842,7 +3844,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);
> > 	INIT_WORK(&hdev->tx_work, hci_tx_work);
> 
> This kind of style changes by removing an on purpose place empty line as
> separator are not acceptable. Please avoid them.

Ack

> 
> Regards
> 
> Marcel

Regards,
Kiran


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

* RE: [PATCH v11 03/10] Bluetooth: btintel: Read supported offload usecases
  2021-07-30 14:10   ` Marcel Holtmann
@ 2021-08-14  6:32     ` K, Kiran
  0 siblings, 0 replies; 25+ messages in thread
From: K, Kiran @ 2021-08-14  6:32 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: linux-bluetooth, Srivatsa, Ravishankar, Tumkur Narayan, Chethan

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Friday, July 30, 2021 7:40 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>
> Subject: Re: [PATCH v11 03/10] Bluetooth: btintel: Read supported offload
> usecases
> 
> Hi Kiran,
> 
> > Read offload usecases supported by controller.
> >
> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > ---
> > * changes in v11:
> >  - Remove Kconfig related changes
> > * changes in v10:
> >  - restructure patch to have  definition and call of callaback in the
> >    same patch
> > * changes in v9:
> >  - define a separate patch for core changes
> >
> > drivers/bluetooth/btintel.c | 30 ++++++++++++++++++++++++++++++
> > drivers/bluetooth/btintel.h | 10 ++++++++++
> > drivers/bluetooth/btusb.c   |  3 +++
> > 3 files changed, 43 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index e44b6993cf91..a6b81914766e 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -1272,6 +1272,36 @@ int btintel_set_debug_features(struct hci_dev
> > *hdev, } EXPORT_SYMBOL_GPL(btintel_set_debug_features);
> >
> > +int btintel_configure_offload_usecases(struct hci_dev *hdev) {
> > +	struct sk_buff *skb;
> > +	int err = 0;
> > +	struct intel_offload_usecases *usecases;
> > +
> > +	skb = __hci_cmd_sync(hdev, 0xfc86, 0, NULL, HCI_INIT_TIMEOUT);
> > +	if (IS_ERR(skb)) {
> > +		bt_dev_err(hdev, "Reading offload usecases failed (%ld)",
> > +			   PTR_ERR(skb));
> > +		return PTR_ERR(skb);
> > +	}
> > +
> > +	if (skb->len < sizeof(*usecases)) {
> > +		err = -EIO;
> > +		goto error;
> > +	}
> > +
> > +	usecases = (void *)skb->data;
> > +
> > +	if (usecases->status) {
> > +		err = -bt_to_errno(skb->data[0]);
> > +		goto error;
> > +	}
> > +error:
> > +	kfree_skb(skb);
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(btintel_configure_offload_usecases);
> > +
> > 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
> > d184064a5e7c..45ca5c4ccf37 100644
> > --- a/drivers/bluetooth/btintel.h
> > +++ b/drivers/bluetooth/btintel.h
> > @@ -132,6 +132,11 @@ struct intel_debug_features {
> > 	__u8    page1[16];
> > } __packed;
> >
> > +struct intel_offload_usecases {
> > +	__u8	status;
> > +	__u8	preset[8];
> > +} __packed;
> > +
> > #define INTEL_HW_PLATFORM(cnvx_bt)	((u8)(((cnvx_bt) &
> 0x0000ff00) >> 8))
> > #define INTEL_HW_VARIANT(cnvx_bt)	((u8)(((cnvx_bt) &
> 0x003f0000) >> 16))
> > #define INTEL_CNVX_TOP_TYPE(cnvx_top)	((cnvx_top) & 0x00000fff)
> > @@ -175,6 +180,7 @@ int btintel_read_debug_features(struct hci_dev
> *hdev,
> > 				struct intel_debug_features *features); int
> > btintel_set_debug_features(struct hci_dev *hdev,
> > 			       const struct intel_debug_features *features);
> > +int btintel_configure_offload_usecases(struct hci_dev *hdev);
> 
> so usecases is not, it would be offload_use_cases. However
> btintel_configure_offload is just fine as a function name. With the work from
> Tedd, this would be removed anyway and all done internally via the hdev-
> >setup().

Ok. I will rebase my patches over Tedd's patches.
> 
> Regards
> 
> Marcel

Regards,
Kiran


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

* RE: [PATCH v11 04/10] Bluetooth: Allow querying of supported offload codecs over SCO socket
  2021-07-30 14:14   ` Marcel Holtmann
@ 2021-08-14  6:33     ` K, Kiran
  0 siblings, 0 replies; 25+ messages in thread
From: K, Kiran @ 2021-08-14  6:33 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: linux-bluetooth, Srivatsa, Ravishankar, Tumkur Narayan, Chethan

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Friday, July 30, 2021 7:44 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>
> Subject: Re: [PATCH v11 04/10] Bluetooth: Allow querying of supported
> offload codecs over SCO socket
> 
> Hi Kiran,
> 
> > Add BT_CODEC option for getsockopt systemcall to get the details of
> > offload codecs supported over SCO socket
> >
> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > ---
> > * changes in v11:
> >  - Remove Kconfig related changes
> > * changes in v10:
> >  - In getsockopt, prepare data in advance and call copy_to_user
> >    istead of calling put_user on each field of struct bt_codec
> >
> > include/net/bluetooth/bluetooth.h | 20 +++++++
> > include/net/bluetooth/hci.h       |  4 ++
> > include/net/bluetooth/hci_core.h  |  1 +
> > net/bluetooth/sco.c               | 91 +++++++++++++++++++++++++++++++
> > 4 files changed, 116 insertions(+)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h
> > b/include/net/bluetooth/bluetooth.h
> > index 9125effbf448..64cddff0c9c4 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -153,6 +153,26 @@ struct bt_voice {
> >
> > #define BT_SCM_PKT_STATUS	0x03
> >
> > +#define BT_CODEC	19
> > +
> > +struct	bt_codec_caps {
> > +	__u8	len;
> > +	__u8	data[];
> > +} __packed;
> > +
> > +struct bt_codec {
> > +	__u8	id;
> > +	__u16	cid;
> > +	__u16	vid;
> > +	__u8	data_path;
> > +	__u8	num_caps;
> > +} __packed;
> > +
> > +struct bt_codecs {
> > +	__u8		num_codecs;
> > +	struct bt_codec	codecs[];
> > +} __packed;
> > +
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > __printf(1, 2)
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index b4e35cf5f4b1..22e872e60ff8 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -2621,6 +2621,10 @@ static inline struct hci_sco_hdr
> *hci_sco_hdr(const struct sk_buff *skb)
> > #define hci_iso_data_len(h)		((h) & 0x3fff)
> > #define hci_iso_data_flags(h)		((h) >> 14)
> >
> > +/* codec transport types */
> > +#define TRANSPORT_ACL		0x00
> > +#define TRANSPORT_SCO_ESCO	0x01
> > +
> > /* le24 support */
> > static inline void hci_cpu_to_le24(__u32 val, __u8 dst[3]) { diff
> > --git a/include/net/bluetooth/hci_core.h
> > b/include/net/bluetooth/hci_core.h
> > index 6742e6ad7b37..71c409c8ab34 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -617,6 +617,7 @@ struct hci_dev {
> > 	int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> > 	void (*cmd_timeout)(struct hci_dev *hdev);
> > 	bool (*prevent_wake)(struct hci_dev *hdev);
> > +	int (*get_data_path_id)(struct hci_dev *hdev, __u8 *data_path);
> > };
> >
> > #define HCI_PHY_HANDLE(handle)	(handle & 0xff)
> > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index
> > ffa2a77a3e4c..2f32693f09c1 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -953,6 +953,12 @@ static int sco_sock_getsockopt(struct socket *sock,
> int level, int optname,
> > 	struct bt_voice voice;
> > 	u32 phys;
> > 	int pkt_status;
> > +	int buf_len;
> > +	struct codec_list *c;
> > +	u8 num_codecs, i, __user *ptr;
> > +	struct hci_dev *hdev;
> > +	struct hci_codec_caps *caps;
> > +	struct bt_codec codec;
> >
> > 	BT_DBG("sk %p", sk);
> >
> > @@ -1017,6 +1023,91 @@ static int sco_sock_getsockopt(struct socket
> *sock, int level, int optname,
> > 			err = -EFAULT;
> > 		break;
> >
> > +	case BT_CODEC:
> > +		num_codecs = 0;
> > +		buf_len = 0;
> > +
> > +		hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src,
> BDADDR_BREDR);
> > +		if (!hdev) {
> > +			err = -EBADFD;
> > +			break;
> > +		}
> > +
> > +		if (!hdev->get_data_path_id) {
> > +			err = -EOPNOTSUPP;
> > +			break;
> > +		}
> 
> so I did say that this needs to be put behind an experimental feature setting.
> I am not committing to an userspace API until we have more feedback.

Ack.
> 
> Regards
> 
> Marcel

Regards,
Kiran



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

* RE: [PATCH v11 06/10] Bluetooth: Allow setting of codec for HFP offload usecase
  2021-08-01 23:45     ` K, Kiran
@ 2021-08-14  6:39       ` K, Kiran
  0 siblings, 0 replies; 25+ messages in thread
From: K, Kiran @ 2021-08-14  6:39 UTC (permalink / raw)
  To: K, Kiran, Marcel Holtmann
  Cc: BlueZ, Srivatsa, Ravishankar, Tumkur Narayan, Chethan

Hi Marcel,

> -----Original Message-----
> From: K, Kiran <kiran.k@intel.com>
> Sent: Monday, August 2, 2021 5:16 AM
> To: Marcel Holtmann <marcel@holtmann.org>
> Cc: BlueZ <linux-bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>
> Subject: RE: [PATCH v11 06/10] Bluetooth: Allow setting of codec for HFP
> offload usecase
> 
> Hi Marcel,
> 
> >
> > Hi Kiran,
> >
> > > For HFP offload usecase, controller needs to be configured with
> > > codec data and capabilities. This patch uses Bluetooth SIG defined
> > > command HCI_CONFIGURE_DATA_PATH to specify vendor specific data
> and
> > > allows userspace modules to set the codec via setsockopt systemcall.
> > >
> > > Signed-off-by: Kiran K <kiran.k@intel.com>
> > > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > > ---
> > > * changes in v11:
> > >  - Remove changes related to Kconfig
> > > * changes in v10:
> > >  - patch refactor - having callback definition and usage in the same
> > > patch
> > >
> > > include/net/bluetooth/bluetooth.h |   2 +
> > > include/net/bluetooth/hci.h       |   8 ++
> > > include/net/bluetooth/hci_core.h  |   3 +
> > > net/bluetooth/sco.c               | 118 ++++++++++++++++++++++++++++++
> > > 4 files changed, 131 insertions(+)
> > >
> > > diff --git a/include/net/bluetooth/bluetooth.h
> > > b/include/net/bluetooth/bluetooth.h
> > > index 64cddff0c9c4..1a48b6732eef 100644
> > > --- a/include/net/bluetooth/bluetooth.h
> > > +++ b/include/net/bluetooth/bluetooth.h
> > > @@ -173,6 +173,8 @@ struct bt_codecs {
> > > 	struct bt_codec	codecs[];
> > > } __packed;
> > >
> > > +#define CODING_FORMAT_CVSD	0x02
> > > +
> > > __printf(1, 2)
> > > void bt_info(const char *fmt, ...);
> > > __printf(1, 2)
> > > diff --git a/include/net/bluetooth/hci.h
> > > b/include/net/bluetooth/hci.h index 22e872e60ff8..c998607fc517
> > > 100644
> > > --- a/include/net/bluetooth/hci.h
> > > +++ b/include/net/bluetooth/hci.h
> > > @@ -1250,6 +1250,14 @@ struct hci_rp_read_local_oob_ext_data {
> > > 	__u8     rand256[16];
> > > } __packed;
> > >
> > > +#define HCI_CONFIGURE_DATA_PATH	0x0c83
> > > +struct hci_op_configure_data_path {
> > > +	__u8	direction;
> > > +	__u8	data_path_id;
> > > +	__u8	vnd_len;
> > > +	__u8	vnd_data[];
> > > +} __packed;
> > > +
> > > #define HCI_OP_READ_LOCAL_VERSION	0x1001
> > > struct hci_rp_read_local_version {
> > > 	__u8     status;
> > > diff --git a/include/net/bluetooth/hci_core.h
> > > b/include/net/bluetooth/hci_core.h
> > > index 71c409c8ab34..eafa6f8114cb 100644
> > > --- a/include/net/bluetooth/hci_core.h
> > > +++ b/include/net/bluetooth/hci_core.h
> > > @@ -618,6 +618,9 @@ struct hci_dev {
> > > 	void (*cmd_timeout)(struct hci_dev *hdev);
> > > 	bool (*prevent_wake)(struct hci_dev *hdev);
> > > 	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,
> > > +				     __u8 **vnd_data);
> >
> > why are these two independent callbacks? I seem to remember saying
> > that it looks like we only need one.
> 
> get_data_path_id,  gets called during getsockopt(BT_CODEC,...) when
> populating codec details to user space.
> 
> get_codec_config_data, gets called during setsockopt(BT_CODEC,...)
> 
Not sure if I was clear here. The two callbacks serves different purpose. get_data_path_id gets called during getsockopt(BT_CODEC,...) and fetches data_path_path_id.
get_codec_config_data gets called during setsockopt(BT_CODEC,...) to fetch the vendor specific codec data. This is data is used to configure the codec before opening SCO connection. This data is passed on to controller via HCI_CONFIGURE_DATA_PATH command. 
> >
> > Regards
> >
> > Marcel
> 
> Regards,
> Kiran
> 

Thanks,
Kiran



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

* RE: [PATCH v11 08/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command
  2021-07-28  3:59   ` Luiz Augusto von Dentz
@ 2021-08-14  6:41     ` K, Kiran
  0 siblings, 0 replies; 25+ messages in thread
From: K, Kiran @ 2021-08-14  6:41 UTC (permalink / raw)
  To: Luiz Augusto von Dentz
  Cc: linux-bluetooth, Srivatsa, Ravishankar, Tumkur Narayan, Chethan

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Sent: Wednesday, July 28, 2021 9:30 AM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>
> Subject: Re: [PATCH v11 08/10] Bluetooth: Add support for
> HCI_Enhanced_Setup_Synchronous_Connection command
> 
> Hi Kiran,
> 
> On Tue, Jul 27, 2021 at 1:45 AM Kiran K <kiran.k@intel.com> wrote:
> >
> > < HCI Command: Enhanced Setup Synchronous Connection (0x01|0x003d)
> plen 59
> >         Handle: 256
> >         Transmit bandwidth: 8000
> >         Receive bandwidth: 8000
> >         Max latency: 13
> >         Packet type: 0x0380
> >           3-EV3 may not be used
> >           2-EV5 may not be used
> >           3-EV5 may not be used
> >         Retransmission effort: Optimize for link quality (0x02)
> > > HCI Event: Command Status (0x0f) plen 4
> >       Enhanced Setup Synchronous Connection (0x01|0x003d) ncmd 1
> >         Status: Success (0x00)
> > > HCI Event: Synchronous Connect Complete (0x2c) plen 17
> >         Status: Success (0x00)
> >         Handle: 257
> >         Address: CC:98:8B:92:04:FD (SONY Visual Products Inc.)
> >         Link type: eSCO (0x02)
> >         Transmission interval: 0x0c
> >         Retransmission window: 0x06
> >         RX packet length: 60
> >         TX packet length: 60
> >         Air mode: Transparent (0x03)
> >
> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > ---
> > * changes in v11:
> >   - Remove changes related to Kconfig
> >   - s/use_enhanced_sco_conn/enhanced_sco_capable/g
> >   - Minor review comments
> >
> > * changes in v10:
> >   - Fix typos and remove unwanted chunks
> > * changes in v9:
> >   - Fix review comments, use bt_dev_dbg instead of BT_DBG
> >
> >  include/net/bluetooth/bluetooth.h |   3 +-
> >  include/net/bluetooth/hci.h       |  34 ++++++++++
> >  include/net/bluetooth/hci_core.h  |   6 +-
> >  net/bluetooth/hci_conn.c          | 105 +++++++++++++++++++++++++++++-
> >  net/bluetooth/hci_event.c         |  39 +++++++++++
> >  net/bluetooth/sco.c               |  11 +++-
> >  6 files changed, 192 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h
> > b/include/net/bluetooth/bluetooth.h
> > index 1a48b6732eef..f2df8bb108d9 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -173,7 +173,8 @@ struct bt_codecs {
> >         struct bt_codec codecs[];
> >  } __packed;
> >
> > -#define CODING_FORMAT_CVSD     0x02
> > +#define CODING_FORMAT_CVSD             0x02
> > +#define CODING_FORMAT_TRANSPARENT      0x03
> >
> >  __printf(1, 2)
> >  void bt_info(const char *fmt, ...);
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index c998607fc517..8af8892e18bc 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -871,6 +871,40 @@ struct hci_cp_logical_link_cancel {
> >         __u8     flow_spec_id;
> >  } __packed;
> >
> > +#define HCI_OP_ENHANCED_SETUP_SYNC_CONN                0x043d
> > +struct hci_coding_format {
> > +       __u8    id;
> > +       __le16  cid;
> > +       __le16  vid;
> > +} __packed;
> > +
> > +struct hci_cp_enhanced_setup_sync_conn {
> > +       __le16   handle;
> > +       __le32   tx_bandwidth;
> > +       __le32   rx_bandwidth;
> > +       struct   hci_coding_format tx_coding_format;
> > +       struct   hci_coding_format rx_coding_format;
> > +       __le16   tx_codec_frame_size;
> > +       __le16   rx_codec_frame_size;
> > +       __le32   in_bandwidth;
> > +       __le32   out_bandwidth;
> > +       struct   hci_coding_format in_coding_format;
> > +       struct   hci_coding_format out_coding_format;
> > +       __le16   in_coded_data_size;
> > +       __le16   out_coded_data_size;
> > +       __u8     in_pcm_data_format;
> > +       __u8     out_pcm_data_format;
> > +       __u8     in_pcm_sample_payload_msb_pos;
> > +       __u8     out_pcm_sample_payload_msb_pos;
> > +       __u8     in_data_path;
> > +       __u8     out_data_path;
> > +       __u8     in_transport_unit_size;
> > +       __u8     out_transport_unit_size;
> > +       __le16   max_latency;
> > +       __le16   pkt_type;
> > +       __u8     retrans_effort;
> > +} __packed;
> > +
> >  struct hci_rp_logical_link_cancel {
> >         __u8     status;
> >         __u8     phy_handle;
> > diff --git a/include/net/bluetooth/hci_core.h
> > b/include/net/bluetooth/hci_core.h
> > index eafa6f8114cb..4c310bd3e1c6 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -713,6 +713,7 @@ struct hci_conn {
> >         struct amp_mgr  *amp_mgr;
> >
> >         struct hci_conn *link;
> > +       struct bt_codec codec;
> >
> >         void (*connect_cfm_cb)  (struct hci_conn *conn, u8 status);
> >         void (*security_cfm_cb) (struct hci_conn *conn, u8 status); @@
> > -1119,7 +1120,7 @@ struct hci_conn *hci_connect_acl(struct hci_dev
> *hdev, bdaddr_t *dst,
> >                                  u8 sec_level, u8 auth_type,
> >                                  enum conn_reasons conn_reason);
> > struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t
> *dst,
> > -                                __u16 setting);
> > +                                __u16 setting, struct bt_codec
> > + *codec);
> >  int hci_conn_check_link_mode(struct hci_conn *conn);  int
> > hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);  int
> > hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8
> > auth_type, @@ -1440,6 +1441,9 @@ void hci_conn_del_sysfs(struct
> > hci_conn *conn);
> >  /* Use LL Privacy based address resolution if supported */  #define
> > use_ll_privacy(dev) ((dev)->le_features[0] & HCI_LE_LL_PRIVACY)
> >
> > +/* Use enhanced synchronous connection if command is supported */
> > +#define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
> > +
> >  /* Use ext scanning if set ext scan param and ext scan enable is
> > supported */  #define use_ext_scan(dev) (((dev)->commands[37] & 0x20)
> && \
> >                            ((dev)->commands[37] & 0x40)) diff --git
> > a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index
> > 2b5059a56cda..48943b01a356 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -307,13 +307,103 @@ static bool find_next_esco_param(struct
> hci_conn *conn,
> >         return conn->attempt <= size;
> >  }
> >
> > -bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
> > +static bool hci_enhanced_setup_sync_conn(struct hci_conn *conn, __u16
> > +handle) {
> > +       struct hci_dev *hdev = conn->hdev;
> > +       struct hci_cp_enhanced_setup_sync_conn cp;
> > +       const struct sco_param *param;
> > +
> > +       bt_dev_dbg(hdev, "hcon %p", conn);
> > +
> > +       conn->state = BT_CONNECT;
> > +       conn->out = true;
> > +
> > +       conn->attempt++;
> > +
> > +       memset(&cp, 0x00, sizeof(cp));
> > +
> > +       cp.handle   = cpu_to_le16(handle);
> > +
> > +       cp.tx_bandwidth   = cpu_to_le32(0x00001f40);
> > +       cp.rx_bandwidth   = cpu_to_le32(0x00001f40);
> > +
> > +       switch (conn->codec.id) {
> > +       case CODING_FORMAT_TRANSPARENT:
> > +               if (!find_next_esco_param(conn, esco_param_msbc,
> > +                                         ARRAY_SIZE(esco_param_msbc)))
> > +                       return false;
> > +               param = &esco_param_msbc[conn->attempt - 1];
> > +               cp.tx_coding_format.id = 0x03;
> > +               cp.rx_coding_format.id = 0x03;
> > +               cp.tx_codec_frame_size = __cpu_to_le16(60);
> > +               cp.rx_codec_frame_size = __cpu_to_le16(60);
> > +               cp.in_bandwidth = __cpu_to_le32(0x1f40);
> > +               cp.out_bandwidth = __cpu_to_le32(0x1f40);
> > +               cp.in_coding_format.id = 0x03;
> > +               cp.out_coding_format.id = 0x03;
> > +               cp.in_coded_data_size = __cpu_to_le16(16);
> > +               cp.out_coded_data_size = __cpu_to_le16(16);
> > +               cp.in_pcm_data_format = 2;
> > +               cp.out_pcm_data_format = 2;
> > +               cp.in_pcm_sample_payload_msb_pos = 0;
> > +               cp.out_pcm_sample_payload_msb_pos = 0;
> > +               cp.in_data_path = conn->codec.data_path;
> > +               cp.out_data_path = conn->codec.data_path;
> > +               cp.in_transport_unit_size = 1;
> > +               cp.out_transport_unit_size = 1;
> > +               break;
> > +
> > +       case CODING_FORMAT_CVSD:
> > +               if (lmp_esco_capable(conn->link)) {
> > +                       if (!find_next_esco_param(conn, esco_param_cvsd,
> > +                                                 ARRAY_SIZE(esco_param_cvsd)))
> > +                               return false;
> > +                       param = &esco_param_cvsd[conn->attempt - 1];
> > +               } else {
> > +                       if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
> > +                               return false;
> > +                       param = &sco_param_cvsd[conn->attempt - 1];
> > +               }
> > +               cp.tx_coding_format.id = 2;
> > +               cp.rx_coding_format.id = 2;
> > +               cp.tx_codec_frame_size = __cpu_to_le16(60);
> > +               cp.rx_codec_frame_size = __cpu_to_le16(60);
> > +               cp.in_bandwidth = __cpu_to_le32(16000);
> > +               cp.out_bandwidth = __cpu_to_le32(16000);
> > +               cp.in_coding_format.id = 4;
> > +               cp.out_coding_format.id = 4;
> > +               cp.in_coded_data_size = __cpu_to_le16(16);
> > +               cp.out_coded_data_size = __cpu_to_le16(16);
> > +               cp.in_pcm_data_format = 2;
> > +               cp.out_pcm_data_format = 2;
> > +               cp.in_pcm_sample_payload_msb_pos = 0;
> > +               cp.out_pcm_sample_payload_msb_pos = 0;
> > +               cp.in_data_path = conn->codec.data_path;
> > +               cp.out_data_path = conn->codec.data_path;
> > +               cp.in_transport_unit_size = 16;
> > +               cp.out_transport_unit_size = 16;
> > +               break;
> > +       default:
> > +               return false;
> > +       }
> > +
> > +       cp.retrans_effort = param->retrans_effort;
> > +       cp.pkt_type = __cpu_to_le16(param->pkt_type);
> > +       cp.max_latency = __cpu_to_le16(param->max_latency);
> > +
> > +       if (hci_send_cmd(hdev, HCI_OP_ENHANCED_SETUP_SYNC_CONN,
> sizeof(cp), &cp) < 0)
> > +               return false;
> > +
> > +       return true;
> > +}
> > +
> > +static bool hci_setup_sync_conn(struct hci_conn *conn, __u16 handle)
> >  {
> >         struct hci_dev *hdev = conn->hdev;
> >         struct hci_cp_setup_sync_conn cp;
> >         const struct sco_param *param;
> >
> > -       BT_DBG("hcon %p", conn);
> > +       bt_dev_dbg(hdev, "hcon %p", conn);
> >
> >         conn->state = BT_CONNECT;
> >         conn->out = true;
> > @@ -359,6 +449,14 @@ bool hci_setup_sync(struct hci_conn *conn, __u16
> handle)
> >         return true;
> >  }
> >
> > +bool hci_setup_sync(struct hci_conn *conn, __u16 handle) {
> > +       if (enhanced_sco_capable(conn->hdev))
> > +               return hci_enhanced_setup_sync_conn(conn, handle);
> > +
> > +       return hci_setup_sync_conn(conn, handle); }
> > +
> >  u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16
> latency,
> >                       u16 to_multiplier)  { @@ -1319,7 +1417,7 @@
> > struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
> > }
> >
> >  struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t
> *dst,
> > -                                __u16 setting)
> > +                                __u16 setting, struct bt_codec
> > + *codec)
> >  {
> >         struct hci_conn *acl;
> >         struct hci_conn *sco;
> > @@ -1344,6 +1442,7 @@ struct hci_conn *hci_connect_sco(struct hci_dev
> *hdev, int type, bdaddr_t *dst,
> >         hci_conn_hold(sco);
> >
> >         sco->setting = setting;
> > +       sco->codec = *codec;
> >
> >         if (acl->state == BT_CONNECTED &&
> >             (sco->state == BT_OPEN || sco->state == BT_CLOSED)) { diff
> > --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index
> > 1c3018202564..18985ac6a83f 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -2236,6 +2236,41 @@ static void hci_cs_setup_sync_conn(struct
> hci_dev *hdev, __u8 status)
> >         hci_dev_unlock(hdev);
> >  }
> >
> > +static void hci_cs_enhanced_setup_sync_conn(struct hci_dev *hdev,
> > +__u8 status) {
> > +       struct hci_cp_enhanced_setup_sync_conn *cp;
> > +       struct hci_conn *acl, *sco;
> > +       __u16 handle;
> > +
> > +       bt_dev_dbg(hdev, "status 0x%2.2x", status);
> > +
> > +       if (!status)
> > +               return;
> > +
> > +       cp = hci_sent_cmd_data(hdev,
> HCI_OP_ENHANCED_SETUP_SYNC_CONN);
> > +       if (!cp)
> > +               return;
> > +
> > +       handle = __le16_to_cpu(cp->handle);
> > +
> > +       bt_dev_dbg(hdev, "handle 0x%4.4x", handle);
> > +
> > +       hci_dev_lock(hdev);
> > +
> > +       acl = hci_conn_hash_lookup_handle(hdev, handle);
> > +       if (acl) {
> > +               sco = acl->link;
> > +               if (sco) {
> > +                       sco->state = BT_CLOSED;
> > +
> > +                       hci_connect_cfm(sco, status);
> > +                       hci_conn_del(sco);
> > +               }
> > +       }
> > +
> > +       hci_dev_unlock(hdev);
> > +}
> > +
> >  static void hci_cs_sniff_mode(struct hci_dev *hdev, __u8 status)  {
> >         struct hci_cp_sniff_mode *cp;
> > @@ -3715,6 +3750,10 @@ static void hci_cmd_status_evt(struct hci_dev
> *hdev, struct sk_buff *skb,
> >                 hci_cs_setup_sync_conn(hdev, ev->status);
> >                 break;
> >
> > +       case HCI_OP_ENHANCED_SETUP_SYNC_CONN:
> > +               hci_cs_enhanced_setup_sync_conn(hdev, ev->status);
> > +               break;
> > +
> >         case HCI_OP_SNIFF_MODE:
> >                 hci_cs_sniff_mode(hdev, ev->status);
> >                 break;
> > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index
> > 34541e971dc7..5ff06ac05fef 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -240,7 +240,7 @@ static int sco_connect(struct sock *sk)
> >         }
> >
> >         hcon = hci_connect_sco(hdev, type, &sco_pi(sk)->dst,
> > -                              sco_pi(sk)->setting);
> > +                              sco_pi(sk)->setting,
> > + &sco_pi(sk)->codec);
> >         if (IS_ERR(hcon)) {
> >                 err = PTR_ERR(hcon);
> >                 goto done;
> > @@ -927,6 +927,15 @@ static int sco_sock_setsockopt(struct socket *sock,
> int level, int optname,
> >                 }
> >
> >                 sco_pi(sk)->setting = voice.setting;
> > +               hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src,
> > +                                    BDADDR_BREDR);
> > +               if (!hdev) {
> > +                       err = -EBADFD;
> > +                       break;
> > +               }
> > +               if (enhanced_sco_capable(hdev) &&
> > +                   voice.setting == BT_VOICE_TRANSPARENT)
> > +                       sco_pi(sk)->codec.id =
> > + CODING_FORMAT_TRANSPARENT;
> 
> 
> hci_get_route does call hci_dev_hold so you need hci_dev_put once you are
> done with it.

Ack. I think same comment is relevant in 04/10 and 06/10 patch. I will send an updated version.
> 
> >                 break;
> >
> >         case BT_PKT_STATUS:
> > --
> > 2.17.1
> >
> 
> 
> --
> Luiz Augusto von Dentz

Thanks,
Kiran


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

* RE: [PATCH v11 10/10] Bluetooth: Add offload feature under experimental flag
  2021-07-30 14:22   ` Marcel Holtmann
@ 2021-08-14  6:42     ` K, Kiran
  0 siblings, 0 replies; 25+ messages in thread
From: K, Kiran @ 2021-08-14  6:42 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: linux-bluetooth, Srivatsa, Ravishankar, Tumkur Narayan, Chethan

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Friday, July 30, 2021 7:52 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>
> Subject: Re: [PATCH v11 10/10] Bluetooth: Add offload feature under
> experimental flag
> 
> Hi Kiran,
> 
> > Allow user level process to enable / disable codec offload feature
> > through mgmt interface. By default offload codec feature is disabled.
> >
> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > ---
> > changes in v11:
> > - Remove Kconfig
> >
> > include/net/bluetooth/hci.h |  1 +
> > net/bluetooth/mgmt.c        | 98
> ++++++++++++++++++++++++++++++++++++-
> > net/bluetooth/sco.c         | 10 ++++
> > 3 files changed, 108 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 8af8892e18bc..dbbbb974ade6 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -330,6 +330,7 @@ enum {
> > 	HCI_ENABLE_LL_PRIVACY,
> > 	HCI_CMD_PENDING,
> > 	HCI_FORCE_NO_MITM,
> > +	HCI_OFFLOAD_CODECS_ENABLED,
> >
> > 	__HCI_NUM_FLAGS,
> > };
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index
> > 3663f880df11..eb10f9597837 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -3791,6 +3791,12 @@ static const u8 debug_uuid[16] = { }; #endif
> >
> > +/* a6695ace-ee7f-4fb9-881a-5fac66c629af */ static const u8
> > +offload_codecs_uuid[16] = {
> > +	0xaf, 0x29, 0xc6, 0x66, 0xac, 0x5f, 0x1a, 0x88,
> > +	0xb9, 0x4f, 0x7f, 0xee, 0xce, 0x5a, 0x69, 0xa6, };
> > +
> > /* 671b10b5-42c0-4696-9227-eb28d1b049d6 */ static const u8
> > simult_central_periph_uuid[16] = {
> > 	0xd6, 0x49, 0xb0, 0xd1, 0x28, 0xeb, 0x27, 0x92, @@ -3806,7 +3812,7
> @@
> > static const u8 rpa_resolution_uuid[16] = { static int
> > read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> > 				  void *data, u16 data_len)
> > {
> > -	char buf[62];	/* Enough space for 3 features */
> > +	char buf[82];   /* Enough space for 4 features: 2 + 20 * 4 */
> > 	struct mgmt_rp_read_exp_features_info *rp = (void *)buf;
> > 	u16 idx = 0;
> > 	u32 flags;
> > @@ -3850,6 +3856,26 @@ static int read_exp_features_info(struct sock
> *sk, struct hci_dev *hdev,
> > 		idx++;
> > 	}
> >
> > +	if (hdev) {
> > +		if (hdev->get_data_path_id) {
> > +			/* BIT(0): indicating if offload codecs are
> > +			 * supported by controller.
> > +			 */
> > +			flags = BIT(0);
> > +
> > +			/* BIT(1): indicating if codec offload feature
> > +			 * is enabled.
> > +			 */
> > +			if (hci_dev_test_flag(hdev,
> HCI_OFFLOAD_CODECS_ENABLED))
> > +				flags |= BIT(1);
> > +		} else {
> > +			flags = 0;
> > +		}
> > +		memcpy(rp->features[idx].uuid, offload_codecs_uuid, 16);
> > +		rp->features[idx].flags = cpu_to_le32(flags);
> > +		idx++;
> > +	}
> > +
> > 	rp->feature_count = cpu_to_le16(idx);
> >
> > 	/* After reading the experimental features information, enable @@
> > -3892,6 +3918,21 @@ static int exp_debug_feature_changed(bool enabled,
> > struct sock *skip) } #endif
> >
> > +static int exp_offload_codec_feature_changed(bool enabled, struct
> > +sock *skip) {
> > +	struct mgmt_ev_exp_feature_changed ev;
> > +
> > +	BT_INFO("enabled %d", enabled);
> > +
> > +	memset(&ev, 0, sizeof(ev));
> > +	memcpy(ev.uuid, offload_codecs_uuid, 16);
> > +	ev.flags = cpu_to_le32(enabled ? BIT(0) : 0);
> > +
> > +	return mgmt_limited_event(MGMT_EV_EXP_FEATURE_CHANGED,
> NULL,
> > +				  &ev, sizeof(ev),
> > +				  HCI_MGMT_EXP_FEATURE_EVENTS, skip); }
> > +
> > static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> > 			   void *data, u16 data_len)
> > {
> > @@ -4038,6 +4079,61 @@ static int set_exp_feature(struct sock *sk, struct
> hci_dev *hdev,
> > 		return err;
> > 	}
> >
> > +	if (!memcmp(cp->uuid, offload_codecs_uuid, 16)) {
> > +		bool val, changed;
> > +		int err;
> > +
> > +		/* Command requires to use a valid controller index */
> > +		if (!hdev)
> > +			return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
> > +					       MGMT_OP_SET_EXP_FEATURE,
> > +					       MGMT_STATUS_INVALID_INDEX);
> > +
> > +		/* Parameters are limited to a single octet */
> > +		if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
> > +			return mgmt_cmd_status(sk, hdev->id,
> > +					       MGMT_OP_SET_EXP_FEATURE,
> > +
> MGMT_STATUS_INVALID_PARAMS);
> > +
> > +		/* Only boolean on/off is supported */
> > +		if (cp->param[0] != 0x00 && cp->param[0] != 0x01)
> > +			return mgmt_cmd_status(sk, hdev->id,
> > +					       MGMT_OP_SET_EXP_FEATURE,
> > +
> MGMT_STATUS_INVALID_PARAMS);
> > +
> > +		val = !!cp->param[0];
> > +		changed = (val != hci_dev_test_flag(hdev,
> > +HCI_OFFLOAD_CODECS_ENABLED));
> > +
> > +		if (!hdev->get_data_path_id) {
> > +			bt_dev_info(hdev, "offload codecs not supported");
> > +			return mgmt_cmd_status(sk, hdev->id,
> > +					       MGMT_OP_SET_EXP_FEATURE,
> > +
> MGMT_STATUS_NOT_SUPPORTED);
> > +		}
> > +
> > +		if (changed) {
> > +			if (val)
> > +				hci_dev_set_flag(hdev,
> HCI_OFFLOAD_CODECS_ENABLED);
> > +			else
> > +				hci_dev_clear_flag(hdev,
> HCI_OFFLOAD_CODECS_ENABLED);
> > +		}
> > +
> > +		bt_dev_info(hdev, "offload codecs enable %d changed %d",
> > +			    val, changed);
> > +
> > +		memcpy(rp.uuid, offload_codecs_uuid, 16);
> > +		rp.flags = cpu_to_le32(val ? BIT(0) : 0);
> > +		hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
> > +		err = mgmt_cmd_complete(sk, hdev->id,
> > +					MGMT_OP_SET_EXP_FEATURE, 0,
> > +					&rp, sizeof(rp));
> > +
> > +		if (changed)
> > +			exp_offload_codec_feature_changed(val, sk);
> > +
> > +		return err;
> > +	}
> > +
> > 	return mgmt_cmd_status(sk, hdev ? hdev->id : MGMT_INDEX_NONE,
> > 			       MGMT_OP_SET_EXP_FEATURE,
> > 			       MGMT_STATUS_NOT_SUPPORTED);
> > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index
> > 5ff06ac05fef..ba66e082ba9e 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -964,6 +964,11 @@ static int sco_sock_setsockopt(struct socket *sock,
> int level, int optname,
> > 			break;
> > 		}
> >
> > +		if (!hci_dev_test_flag(hdev,
> HCI_OFFLOAD_CODECS_ENABLED)) {
> > +			err = -EOPNOTSUPP;
> > +			break;
> > +		}
> > +
> > 		if (!hdev->get_codec_config_data) {
> > 			err = -EOPNOTSUPP;
> > 			break;
> > @@ -1160,6 +1165,11 @@ static int sco_sock_getsockopt(struct socket
> *sock, int level, int optname,
> > 			break;
> > 		}
> >
> > +		if (!hci_dev_test_flag(hdev,
> HCI_OFFLOAD_CODECS_ENABLED)) {
> > +			err = -EOPNOTSUPP;
> > +			break;
> > +		}
> > +
> > 		if (!hdev->get_data_path_id) {
> > 			err = -EOPNOTSUPP;
> > 			break;
> 
> these two and the flag definition itself need to be a lot earlier in the series.
> 
Ack.

> Regards
> 
> Marcel

Thanks,
Kiran



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

end of thread, other threads:[~2021-08-14  6:42 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27  8:47 [PATCH v11 01/10] Bluetooth: Enumerate local supported codec and cache details Kiran K
2021-07-27  8:47 ` [PATCH v11 02/10] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
2021-07-30 14:07   ` Marcel Holtmann
2021-07-27  8:47 ` [PATCH v11 03/10] Bluetooth: btintel: Read supported offload usecases Kiran K
2021-07-30 14:10   ` Marcel Holtmann
2021-08-14  6:32     ` K, Kiran
2021-07-27  8:47 ` [PATCH v11 04/10] Bluetooth: Allow querying of supported offload codecs over SCO socket Kiran K
2021-07-30 14:14   ` Marcel Holtmann
2021-08-14  6:33     ` K, Kiran
2021-07-27  8:47 ` [PATCH v11 05/10] Bluetooth: btintel: Define callback to fetch data_path_id Kiran K
2021-07-27  8:47 ` [PATCH v11 06/10] Bluetooth: Allow setting of codec for HFP offload usecase Kiran K
2021-07-30 14:17   ` Marcel Holtmann
2021-08-01 23:45     ` K, Kiran
2021-08-14  6:39       ` K, Kiran
2021-07-27  8:47 ` [PATCH v11 07/10] Bluetooth: btintel: Define a callback to fetch codec config data Kiran K
2021-07-27  8:47 ` [PATCH v11 08/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command Kiran K
2021-07-28  3:59   ` Luiz Augusto von Dentz
2021-08-14  6:41     ` K, Kiran
2021-07-27  8:47 ` [PATCH v11 09/10] Bluetooth: Add support for msbc coding format Kiran K
2021-07-27  8:47 ` [PATCH v11 10/10] Bluetooth: Add offload feature under experimental flag Kiran K
2021-07-30 14:22   ` Marcel Holtmann
2021-08-14  6:42     ` K, Kiran
2021-07-27  9:12 ` [v11,01/10] Bluetooth: Enumerate local supported codec and cache details bluez.test.bot
2021-07-30 14:06 ` [PATCH v11 01/10] " Marcel Holtmann
2021-08-02  0:23   ` K, Kiran

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).