All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details
@ 2021-06-08 12:24 Kiran K
  2021-06-08 12:24 ` [PATCH v9 02/10] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
                   ` (11 more replies)
  0 siblings, 12 replies; 31+ messages in thread
From: Kiran K @ 2021-06-08 12:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Kiran K, Chethan T N, Srivatsa Ravishankar

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

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Signed-off-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
---
* changes in 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/hci_core.c         | 199 ++++++++++++++++++++++++++++++-
 3 files changed, 253 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 2dc947341502..3eb723765669 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 212f46806ce7..3284044c3dd7 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;
+	__le16	cid;
+	__le16	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/hci_core.c b/net/bluetooth/hci_core.c
index 1eb7ffd0dd29..3f77ce1e9dd6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -838,10 +838,6 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
 	if (hdev->commands[22] & 0x04)
 		hci_set_event_mask_page_2(req);
 
-	/* Read local codec list if the HCI command is supported */
-	if (hdev->commands[29] & 0x20)
-		hci_req_add(req, HCI_OP_READ_LOCAL_CODECS, 0, NULL);
-
 	/* Read local pairing options if the HCI command is supported */
 	if (hdev->commands[41] & 0x08)
 		hci_req_add(req, HCI_OP_READ_LOCAL_PAIRING_OPTS, 0, NULL);
@@ -907,6 +903,195 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
 	return 0;
 }
 
+static int hci_codec_list_add(struct list_head *list,
+			      struct hci_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;
+}
+
+static void hci_codec_list_clear(struct list_head *codec_list)
+{
+	struct codec_list *c, *n;
+
+	list_for_each_entry_safe(c, n, codec_list, list) {
+		list_del(&c->list);
+		kfree(c);
+	}
+}
+
+static void hci_read_codec_capabilities(struct hci_dev *hdev, void *codec_id,
+					__u8 transport, bool is_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);
+		}
+	}
+}
+
+static 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);
+}
+
 static int __hci_init(struct hci_dev *hdev)
 {
 	int err;
@@ -937,6 +1122,10 @@ static int __hci_init(struct hci_dev *hdev)
 	if (err < 0)
 		return err;
 
+	/* Read local codec list if the HCI command is supported */
+	if (hdev->commands[29] & 0x20)
+		hci_read_supported_codecs(hdev);
+
 	/* This function is only called when the controller is actually in
 	 * configured state. When the controller is marked as unconfigured,
 	 * this initialization procedure is not run.
@@ -1836,6 +2025,7 @@ int hci_dev_do_close(struct hci_dev *hdev)
 	memset(hdev->eir, 0, sizeof(hdev->eir));
 	memset(hdev->dev_class, 0, sizeof(hdev->dev_class));
 	bacpy(&hdev->random_addr, BDADDR_ANY);
+	hci_codec_list_clear(&hdev->local_codecs);
 
 	hci_req_sync_unlock(hdev);
 
@@ -3837,6 +4027,7 @@ struct hci_dev *hci_alloc_dev(void)
 	INIT_LIST_HEAD(&hdev->conn_hash.list);
 	INIT_LIST_HEAD(&hdev->adv_instances);
 	INIT_LIST_HEAD(&hdev->blocked_keys);
+	INIT_LIST_HEAD(&hdev->local_codecs);
 
 	INIT_WORK(&hdev->rx_work, hci_rx_work);
 	INIT_WORK(&hdev->cmd_work, hci_cmd_work);
-- 
2.17.1


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

* [PATCH v9 02/10] Bluetooth: Add support for Read Local Supported Codecs V2
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
@ 2021-06-08 12:24 ` Kiran K
  2021-06-08 12:24 ` [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path Kiran K
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 31+ messages in thread
From: Kiran K @ 2021-06-08 12:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Kiran K, Chethan T N, Srivatsa Ravishankar

Use V2 version of read local supported command is controller
supports

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

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Signed-off-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
* changes in 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_core.c    | 78 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 106 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 3eb723765669..45bd9af4ce61 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_core.c b/net/bluetooth/hci_core.c
index 3f77ce1e9dd6..186b347ae9d3 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1038,6 +1038,28 @@ static void hci_codec_list_parse(struct hci_dev *hdev, __u8 num_codecs,
 	}
 }
 
+static void hci_codec_list_parse_v2(struct hci_dev *hdev, __u8 num_codecs,
+				    void *codec_list, bool is_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);
+		}
+	}
+}
+
 static void hci_read_supported_codecs(struct hci_dev *hdev)
 {
 	struct sk_buff *skb;
@@ -1092,6 +1114,58 @@ static void hci_read_supported_codecs(struct hci_dev *hdev)
 	kfree_skb(skb);
 }
 
+static void hci_read_supported_codecs_v2(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	struct hci_rp_read_local_supported_codecs_v2 *rp;
+	struct hci_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);
+}
+
 static int __hci_init(struct hci_dev *hdev)
 {
 	int err;
@@ -1123,7 +1197,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] 31+ messages in thread

* [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
  2021-06-08 12:24 ` [PATCH v9 02/10] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
@ 2021-06-08 12:24 ` Kiran K
  2021-06-15 19:26   ` Marcel Holtmann
  2021-06-08 12:24 ` [PATCH v9 04/10] Bluetooth: btintel: set get_data_path callback Kiran K
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 31+ messages in thread
From: Kiran K @ 2021-06-08 12:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Kiran K

There is no standard HCI command to retrieve data path for transport.
Add a new callback function to retrieve data path which is used
in offload usecase. This needs to be set at setup stage if controller
supports offload codecs

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 v9:
  - define a separate patch for core changes

 include/net/bluetooth/hci_core.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3284044c3dd7..641477396da3 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)(struct hci_dev *hdev);
 };
 
 #define HCI_PHY_HANDLE(handle)	(handle & 0xff)
-- 
2.17.1


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

* [PATCH v9 04/10] Bluetooth: btintel: set get_data_path callback
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
  2021-06-08 12:24 ` [PATCH v9 02/10] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
  2021-06-08 12:24 ` [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path Kiran K
@ 2021-06-08 12:24 ` Kiran K
  2021-06-15 19:30   ` Marcel Holtmann
  2021-06-08 12:24 ` [PATCH v9 05/10] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket Kiran K
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 31+ messages in thread
From: Kiran K @ 2021-06-08 12:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Kiran K

Read supported offload usecases and set get_data_path callback if
controller suppports offload codecs.

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>
---
 drivers/bluetooth/btintel.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btintel.h | 18 ++++++++++++++++++
 drivers/bluetooth/btusb.c   |  8 ++++++++
 3 files changed, 62 insertions(+)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index e44b6993cf91..95c6a1bef35e 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -1272,6 +1272,42 @@ int btintel_set_debug_features(struct hci_dev *hdev,
 }
 EXPORT_SYMBOL_GPL(btintel_set_debug_features);
 
+int btintel_get_data_path(struct hci_dev *hdev)
+{
+	return 1;
+}
+EXPORT_SYMBOL_GPL(btintel_get_data_path);
+
+int btintel_read_offload_usecases(struct hci_dev *hdev,
+				  struct intel_offload_usecases *usecases)
+{
+	struct sk_buff *skb;
+	int err = 0;
+
+	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;
+	}
+
+	if (skb->data[0]) {
+		err = -bt_to_errno(skb->data[0]);
+		goto error;
+	}
+
+	memcpy(usecases, skb->data, sizeof(*usecases));
+error:
+	kfree_skb(skb);
+	return err;
+}
+EXPORT_SYMBOL_GPL(btintel_read_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..9bcc489680db 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,9 @@ 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_read_offload_usecases(struct hci_dev *hdev,
+				  struct intel_offload_usecases *usecases);
+int btintel_get_data_path(struct hci_dev *hdev);
 #else
 
 static inline int btintel_check_bdaddr(struct hci_dev *hdev)
@@ -307,4 +315,14 @@ static inline int btintel_set_debug_features(struct hci_dev *hdev,
 	return -EOPNOTSUPP;
 }
 
+static int btintel_read_offload_usecases(struct hci_dev *hdev,
+					 struct intel_offload_usecases *usecases)
+{
+	return -EOPNOTSUPP;
+}
+
+static int btintel_get_data_path(struct hci_dev *hdev)
+{
+	return -EOPNOTSUPP;
+}
 #endif
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a9855a2dd561..1e51beec5776 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2952,6 +2952,7 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
 	int err;
 	struct intel_debug_features features;
 	struct intel_version_tlv version;
+	struct intel_offload_usecases usecases;
 
 	bt_dev_dbg(hdev, "");
 
@@ -3008,6 +3009,13 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
 	/* Set DDC mask for available debug features */
 	btintel_set_debug_features(hdev, &features);
 
+	err = btintel_read_offload_usecases(hdev, &usecases);
+	if (!err) {
+		/* set get_data_path callback if offload is supported */
+		if (usecases.preset[0] & 0x03)
+			hdev->get_data_path = btintel_get_data_path;
+	}
+
 	/* 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] 31+ messages in thread

* [PATCH v9 05/10] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
                   ` (2 preceding siblings ...)
  2021-06-08 12:24 ` [PATCH v9 04/10] Bluetooth: btintel: set get_data_path callback Kiran K
@ 2021-06-08 12:24 ` Kiran K
  2021-06-15 19:37   ` Marcel Holtmann
  2021-06-08 12:24 ` [PATCH v9 06/10] Bluetooth: Add a callback function to set data path Kiran K
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 31+ messages in thread
From: Kiran K @ 2021-06-08 12:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Kiran K

Add BT_CODEC option for getsockopt systemcall over SCO socket
to expose the codecs 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 on v9:
  - fix typos,review comments, remove quirk

 include/net/bluetooth/bluetooth.h |  20 ++++++
 include/net/bluetooth/hci.h       |   4 ++
 net/bluetooth/sco.c               | 111 +++++++++++++++++++++++++++++-
 3 files changed, 134 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 9125effbf448..1840756958ce 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;
+	__le16	cid;
+	__le16	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 45bd9af4ce61..31a5ac8918fc 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -2619,6 +2619,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/net/bluetooth/sco.c b/net/bluetooth/sco.c
index d9a4e88dacbb..98d5e24e5680 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -944,10 +944,15 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 			       char __user *optval, int __user *optlen)
 {
 	struct sock *sk = sock->sk;
-	int len, err = 0;
+	int len, err = 0, buf_len;
 	struct bt_voice voice;
 	u32 phys;
 	int pkt_status;
+	struct codec_list *c;
+	u8 num_codecs, i, __user *ptr;
+	struct hci_dev *hdev;
+	struct hci_codec_caps *caps;
+	__u8	data_path;
 
 	BT_DBG("sk %p", sk);
 
@@ -1012,6 +1017,110 @@ 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) {
+			err = -EOPNOTSUPP;
+			break;
+		}
+
+		hci_dev_lock(hdev);
+		/* find total buffer size  required to copy codec + caps */
+		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);
+
+		hci_dev_lock(hdev);
+		list_for_each_entry(c, &hdev->local_codecs, list) {
+			if (c->transport != TRANSPORT_SCO_ESCO)
+				continue;
+
+			if (put_user(c->id, ptr)) {
+				err = -EFAULT;
+				goto unlock;
+			}
+			ptr += sizeof(c->id);
+
+			if (put_user(c->cid, ptr)) {
+				err = -EFAULT;
+				goto unlock;
+			}
+			ptr += sizeof(c->cid);
+
+			if (put_user(c->vid, ptr)) {
+				err = -EFAULT;
+				goto unlock;
+			}
+			ptr += sizeof(c->vid);
+
+			err = hdev->get_data_path(hdev);
+			if (err < 0) {
+				err = -EFAULT;
+				goto unlock;
+			}
+
+			data_path = (__u8)err;
+			if (put_user(data_path, ptr)) {
+				err = -EFAULT;
+				goto unlock;
+			}
+			ptr += sizeof(data_path);
+
+			if (put_user(c->num_caps, ptr)) {
+				err = -EFAULT;
+				goto unlock;
+			}
+			ptr += sizeof(c->num_caps);
+
+			len = 0;
+			for (i = 0, caps = c->caps; i < c->num_caps; i++) {
+				len += 1 + caps->len;
+				caps = (void *)&caps->data[caps->len];
+			}
+
+			if (len && copy_to_user(ptr, c->caps, len)) {
+				err = -EFAULT;
+				goto unlock;
+			}
+			ptr += len;
+		}
+
+		if (put_user(buf_len, optlen))
+			err = -EFAULT;
+unlock:
+		hci_dev_unlock(hdev);
+
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
-- 
2.17.1


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

* [PATCH v9 06/10] Bluetooth: Add a callback function to set data path
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
                   ` (3 preceding siblings ...)
  2021-06-08 12:24 ` [PATCH v9 05/10] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket Kiran K
@ 2021-06-08 12:24 ` Kiran K
  2021-06-15 19:37   ` Marcel Holtmann
  2021-06-08 12:24 ` [PATCH v9 07/10] Bluetooth: btintel: define callback " Kiran K
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 31+ messages in thread
From: Kiran K @ 2021-06-08 12:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Kiran K

In HFP offload usecase, Intel controllers require offload use
case id (NBS or WBS) to be set before opening SCO connection. Define
a new callback which gets called on setsockopt SCO socket. User space
audio module is expected to set codec via setsockopt(sk, BT_CODEC, ....)
before opening SCO connection.

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>
---
 include/net/bluetooth/hci_core.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 641477396da3..ad0024891447 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -618,6 +618,8 @@ struct hci_dev {
 	void (*cmd_timeout)(struct hci_dev *hdev);
 	bool (*prevent_wake)(struct hci_dev *hdev);
 	int (*get_data_path)(struct hci_dev *hdev);
+	int (*set_data_path)(struct hci_dev *hdev, __u8 type,
+			     struct bt_codec *codec);
 };
 
 #define HCI_PHY_HANDLE(handle)	(handle & 0xff)
-- 
2.17.1


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

* [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
                   ` (4 preceding siblings ...)
  2021-06-08 12:24 ` [PATCH v9 06/10] Bluetooth: Add a callback function to set data path Kiran K
@ 2021-06-08 12:24 ` Kiran K
  2021-06-15 19:39   ` Marcel Holtmann
  2021-06-08 12:24 ` [PATCH v9 08/10] Bluetooth: Add BT_CODEC option for setsockopt over SCO Kiran K
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 31+ messages in thread
From: Kiran K @ 2021-06-08 12:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Kiran K

Adds callback function which is called to set the data path
for HFP offload case before opening SCO connection

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>
---
 drivers/bluetooth/btintel.c | 50 +++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btintel.h |  8 ++++++
 drivers/bluetooth/btusb.c   |  4 ++-
 include/net/bluetooth/hci.h |  8 ++++++
 4 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 95c6a1bef35e..3eba5c587ef6 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -1308,6 +1308,56 @@ int btintel_read_offload_usecases(struct hci_dev *hdev,
 }
 EXPORT_SYMBOL_GPL(btintel_read_offload_usecases);
 
+int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
+			  struct bt_codec *codec)
+{
+	__u8 preset;
+	struct hci_op_configure_data_path *cmd;
+	__u8 buffer[255];
+	struct sk_buff *skb;
+
+	if (type != SCO_LINK && type != ESCO_LINK)
+		return -EINVAL;
+
+	switch (codec->id) {
+	case 0x02:
+		preset = 0x00;
+	break;
+	case 0x05:
+		preset = 0x01;
+	break;
+	default:
+		return -EINVAL;
+	}
+
+	cmd = (void *)buffer;
+	cmd->data_path_id = 0x01;
+	cmd->len = 1;
+	cmd->data[0] = preset;
+
+	cmd->direction = 0x00;
+	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH, sizeof(*cmd) + 1,
+			     cmd, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "configure input data path failed (%ld)",
+			   PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+	kfree_skb(skb);
+
+	cmd->direction = 0x01;
+	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH, sizeof(*cmd) + 1,
+			     cmd, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "configure output data path failed (%ld)",
+			   PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+	kfree_skb(skb);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(btintel_set_data_path);
+
 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 9bcc489680db..9806970c9871 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -183,6 +183,8 @@ int btintel_set_debug_features(struct hci_dev *hdev,
 int btintel_read_offload_usecases(struct hci_dev *hdev,
 				  struct intel_offload_usecases *usecases);
 int btintel_get_data_path(struct hci_dev *hdev);
+int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
+			  struct bt_codec *codec);
 #else
 
 static inline int btintel_check_bdaddr(struct hci_dev *hdev)
@@ -325,4 +327,10 @@ static int btintel_get_data_path(struct hci_dev *hdev)
 {
 	return -EOPNOTSUPP;
 }
+
+static int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
+				 struct bt_codec *codec)
+{
+	return -EOPNOTSUPP;
+}
 #endif
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 1e51beec5776..afafa44752a1 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3012,8 +3012,10 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
 	err = btintel_read_offload_usecases(hdev, &usecases);
 	if (!err) {
 		/* set get_data_path callback if offload is supported */
-		if (usecases.preset[0] & 0x03)
+		if (usecases.preset[0] & 0x03) {
 			hdev->get_data_path = btintel_get_data_path;
+			hdev->set_data_path = btintel_set_data_path;
+		}
 	}
 
 	/* Read the Intel version information after loading the FW  */
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 31a5ac8918fc..42963188dcea 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	len;
+	__u8	data[];
+} __packed;
+
 #define HCI_OP_READ_LOCAL_VERSION	0x1001
 struct hci_rp_read_local_version {
 	__u8     status;
-- 
2.17.1


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

* [PATCH v9 08/10] Bluetooth: Add BT_CODEC option for setsockopt over SCO
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
                   ` (5 preceding siblings ...)
  2021-06-08 12:24 ` [PATCH v9 07/10] Bluetooth: btintel: define callback " Kiran K
@ 2021-06-08 12:24 ` Kiran K
  2021-06-08 12:24 ` [PATCH v9 09/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command Kiran K
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 31+ messages in thread
From: Kiran K @ 2021-06-08 12:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Kiran K

Add BT_CODEC option on setsockopt system call to allow user space
audio modules to set codec. Driver also sets data path if non-HCI is
selected.

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>
---
 include/net/bluetooth/bluetooth.h |  2 ++
 net/bluetooth/sco.c               | 59 +++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 1840756958ce..0e8802d09068 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/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 98d5e24e5680..5aa6808c1a22 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);
 
@@ -808,6 +814,9 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 	struct sock *sk = sock->sk;
 	int len, err = 0;
 	struct bt_voice voice;
+	struct bt_codecs *codecs;
+	struct hci_dev *hdev;
+	__u8 buffer[255];
 	u32 opt;
 
 	BT_DBG("sk %p", sk);
@@ -870,6 +879,56 @@ 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->set_data_path) {
+			err = -EOPNOTSUPP;
+			break;
+		}
+
+		if (optlen < sizeof(struct bt_codecs) || optlen > 255) {
+			err = -EINVAL;
+			break;
+		}
+
+		if (copy_from_sockptr(buffer, optval, optlen)) {
+			err = -EFAULT;
+			break;
+		}
+
+		codecs = (void *)buffer;
+
+		if (codecs->num_codecs > 1) {
+			err = -EINVAL;
+			break;
+		}
+
+		if (codecs->codecs[0].data_path) {
+			err = hdev->set_data_path(hdev, SCO_LINK,
+						  codecs->codecs);
+			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] 31+ messages in thread

* [PATCH v9 09/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
                   ` (6 preceding siblings ...)
  2021-06-08 12:24 ` [PATCH v9 08/10] Bluetooth: Add BT_CODEC option for setsockopt over SCO Kiran K
@ 2021-06-08 12:24 ` Kiran K
  2021-06-08 12:24 ` [PATCH v9 10/10] Bluetooth: Add support for msbc coding format Kiran K
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 31+ messages in thread
From: Kiran K @ 2021-06-08 12:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: 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 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  |   7 +-
 net/bluetooth/hci_conn.c          | 107 ++++++++++++++++++++++++++++--
 net/bluetooth/hci_event.c         |  48 +++++++++++++-
 net/bluetooth/sco.c               |  11 ++-
 6 files changed, 201 insertions(+), 9 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 0e8802d09068..af2809121571 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 42963188dcea..03241c15b5fb 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_trasnport_unit_size;
+	__u8	 out_trasnport_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 ad0024891447..4c2514135a5d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -712,6 +712,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);
@@ -1094,6 +1095,7 @@ static inline struct hci_conn *hci_lookup_le_connect(struct hci_dev *hdev)
 
 int hci_disconnect(struct hci_conn *conn, __u8 reason);
 bool hci_setup_sync(struct hci_conn *conn, __u16 handle);
+bool hci_enhanced_setup_sync(struct hci_conn *conn, __u16 handle);
 void hci_sco_setup(struct hci_conn *conn, __u8 status);
 
 struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
@@ -1118,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,
@@ -1439,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 use_enhanced_sco_conn(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..9569b21adb88 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -307,6 +307,97 @@ static bool find_next_esco_param(struct hci_conn *conn,
 	return conn->attempt <= size;
 }
 
+bool hci_enhanced_setup_sync(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_trasnport_unit_size = 1;
+		cp.out_trasnport_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_trasnport_unit_size = 16;
+		cp.out_trasnport_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;
+}
+
 bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
 {
 	struct hci_dev *hdev = conn->hdev;
@@ -424,10 +515,14 @@ void hci_sco_setup(struct hci_conn *conn, __u8 status)
 	BT_DBG("hcon %p", conn);
 
 	if (!status) {
-		if (lmp_esco_capable(conn->hdev))
-			hci_setup_sync(sco, conn->handle);
-		else
+		if (lmp_esco_capable(conn->hdev)) {
+			if (use_enhanced_sco_conn(conn->hdev))
+				hci_enhanced_setup_sync(sco, conn->handle);
+			else
+				hci_setup_sync(sco, conn->handle);
+		} else {
 			hci_add_sco(sco, conn->handle);
+		}
 	} else {
 		hci_connect_cfm(sco, status);
 		hci_conn_del(sco);
@@ -1319,7 +1414,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 +1439,10 @@ struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
 	hci_conn_hold(sco);
 
 	sco->setting = setting;
+	sco->codec.id = codec->id;
+	sco->codec.cid = codec->cid;
+	sco->codec.vid = codec->vid;
+	sco->codec.data_path = codec->data_path;
 
 	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 98ec486743ba..29a769a1a5e7 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;
@@ -4401,8 +4440,13 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
 		if (conn->out) {
 			conn->pkt_type = (hdev->esco_type & SCO_ESCO_MASK) |
 					(hdev->esco_type & EDR_ESCO_MASK);
-			if (hci_setup_sync(conn, conn->link->handle))
-				goto unlock;
+			if (use_enhanced_sco_conn(hdev)) {
+				if (hci_enhanced_setup_sync(conn, conn->link->handle))
+					goto unlock;
+			} else {
+				if (hci_setup_sync(conn, conn->link->handle))
+					goto unlock;
+			}
 		}
 		fallthrough;
 
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 5aa6808c1a22..f4eea0ae3af2 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;
@@ -865,6 +865,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 (use_enhanced_sco_conn(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] 31+ messages in thread

* [PATCH v9 10/10] Bluetooth: Add support for msbc coding format
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
                   ` (7 preceding siblings ...)
  2021-06-08 12:24 ` [PATCH v9 09/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command Kiran K
@ 2021-06-08 12:24 ` Kiran K
  2021-06-15 19:43   ` Marcel Holtmann
  2021-06-08 13:39 ` [v9,01/10] Bluetooth: enumerate local supported codec and cache details bluez.test.bot
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 31+ messages in thread
From: Kiran K @ 2021-06-08 12:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: 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>
---
 include/net/bluetooth/bluetooth.h |  1 +
 net/bluetooth/hci_conn.c          | 27 ++++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index af2809121571..056699224da7 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 9569b21adb88..73c134459361 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -328,6 +328,32 @@ bool hci_enhanced_setup_sync(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_trasnport_unit_size = 1;
+		cp.out_trasnport_unit_size = 1;
+		break;
+
 	case CODING_FORMAT_TRANSPARENT:
 		if (!find_next_esco_param(conn, esco_param_msbc,
 					  ARRAY_SIZE(esco_param_msbc)))
@@ -383,7 +409,6 @@ bool hci_enhanced_setup_sync(struct hci_conn *conn, __u16 handle)
 		cp.in_trasnport_unit_size = 16;
 		cp.out_trasnport_unit_size = 16;
 		break;
-
 	default:
 		return false;
 	}
-- 
2.17.1


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

* RE: [v9,01/10] Bluetooth: enumerate local supported codec and cache details
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
                   ` (8 preceding siblings ...)
  2021-06-08 12:24 ` [PATCH v9 10/10] Bluetooth: Add support for msbc coding format Kiran K
@ 2021-06-08 13:39 ` bluez.test.bot
  2021-06-08 18:49   ` kernel test robot
  2021-06-15 19:25 ` Marcel Holtmann
  11 siblings, 0 replies; 31+ messages in thread
From: bluez.test.bot @ 2021-06-08 13:39 UTC (permalink / raw)
  To: linux-bluetooth, kiran.k

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

---Test result---

Test Summary:
CheckPatch                    PASS      8.22 seconds
GitLint                       FAIL      1.15 seconds
BuildKernel                   PASS      689.43 seconds
TestRunner: Setup             PASS      444.12 seconds
TestRunner: l2cap-tester      PASS      3.26 seconds
TestRunner: bnep-tester       PASS      2.18 seconds
TestRunner: mgmt-tester       PASS      33.21 seconds
TestRunner: rfcomm-tester     PASS      2.59 seconds
TestRunner: sco-tester        PASS      2.41 seconds
TestRunner: smp-tester        PASS      2.57 seconds
TestRunner: userchan-tester   PASS      2.26 seconds

Details
##############################
Test: CheckPatch - PASS - 8.22 seconds
Run checkpatch.pl script with rule in .checkpatch.conf


##############################
Test: GitLint - FAIL - 1.15 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 - 689.43 seconds
Build Kernel with minimal configuration supports Bluetooth


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


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

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

##############################
Test: TestRunner: mgmt-tester - PASS - 33.21 seconds
Run test-runner with mgmt-tester
Total: 446, Passed: 433 (97.1%), Failed: 0, Not Run: 13

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

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

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

##############################
Test: TestRunner: userchan-tester - PASS - 2.26 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: 51530 bytes --]

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

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

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

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

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

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

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

* Re: [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
@ 2021-06-08 18:49   ` kernel test robot
  2021-06-08 12:24 ` [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path Kiran K
                     ` (10 subsequent siblings)
  11 siblings, 0 replies; 31+ messages in thread
From: kernel test robot @ 2021-06-08 18:49 UTC (permalink / raw)
  To: Kiran K, linux-bluetooth
  Cc: kbuild-all, Kiran K, Chethan T N, Srivatsa Ravishankar

[-- Attachment #1: Type: text/plain, Size: 4052 bytes --]

Hi Kiran,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on bluetooth/master v5.13-rc5 next-20210608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kiran-K/Bluetooth-enumerate-local-supported-codec-and-cache-details/20210608-202239
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: mips-randconfig-s032-20210608 (attached as .config)
compiler: mipsel-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/38ec55e0fc2fabf67d0b5f151700afae12db44f7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kiran-K/Bluetooth-enumerate-local-supported-codec-and-cache-details/20210608-202239
        git checkout 38ec55e0fc2fabf67d0b5f151700afae12db44f7
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
   command-line: note: in included file:
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQUIRE redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_SEQ_CST redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQ_REL redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_RELEASE redefined
   builtin:0:0: sparse: this was the original definition
   net/bluetooth/hci_core.c: note: in included file:
   include/net/bluetooth/hci_core.h:142:35: sparse: sparse: array of flexible structures
>> net/bluetooth/hci_core.c:920:28: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le16 [usertype] cid @@     got unsigned short [usertype] @@
   net/bluetooth/hci_core.c:920:28: sparse:     expected restricted __le16 [usertype] cid
   net/bluetooth/hci_core.c:920:28: sparse:     got unsigned short [usertype]
>> net/bluetooth/hci_core.c:921:28: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le16 [usertype] vid @@     got unsigned short [usertype] @@
   net/bluetooth/hci_core.c:921:28: sparse:     expected restricted __le16 [usertype] vid
   net/bluetooth/hci_core.c:921:28: sparse:     got unsigned short [usertype]

vim +920 net/bluetooth/hci_core.c

   905	
   906	static int hci_codec_list_add(struct list_head *list,
   907				      struct hci_op_read_local_codec_caps *sent,
   908				      struct hci_rp_read_local_codec_caps *rp,
   909				      void *caps,
   910				      __u32 len)
   911	{
   912		struct codec_list *entry;
   913	
   914		entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL);
   915		if (!entry)
   916			return -ENOMEM;
   917	
   918		entry->id = sent->id;
   919		if (sent->id == 0xFF) {
 > 920			entry->cid = __le16_to_cpu(sent->cid);
 > 921			entry->vid = __le16_to_cpu(sent->vid);
   922		}
   923		entry->transport = sent->transport;
   924		entry->len = len;
   925		entry->num_caps = rp->num_caps;
   926		if (rp->num_caps)
   927			memcpy(entry->caps, caps, len);
   928		list_add(&entry->list, list);
   929	
   930		return 0;
   931	}
   932	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24016 bytes --]

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

* Re: [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details
@ 2021-06-08 18:49   ` kernel test robot
  0 siblings, 0 replies; 31+ messages in thread
From: kernel test robot @ 2021-06-08 18:49 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 4137 bytes --]

Hi Kiran,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on bluetooth/master v5.13-rc5 next-20210608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kiran-K/Bluetooth-enumerate-local-supported-codec-and-cache-details/20210608-202239
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: mips-randconfig-s032-20210608 (attached as .config)
compiler: mipsel-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/38ec55e0fc2fabf67d0b5f151700afae12db44f7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kiran-K/Bluetooth-enumerate-local-supported-codec-and-cache-details/20210608-202239
        git checkout 38ec55e0fc2fabf67d0b5f151700afae12db44f7
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
   command-line: note: in included file:
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQUIRE redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_SEQ_CST redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQ_REL redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_RELEASE redefined
   builtin:0:0: sparse: this was the original definition
   net/bluetooth/hci_core.c: note: in included file:
   include/net/bluetooth/hci_core.h:142:35: sparse: sparse: array of flexible structures
>> net/bluetooth/hci_core.c:920:28: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le16 [usertype] cid @@     got unsigned short [usertype] @@
   net/bluetooth/hci_core.c:920:28: sparse:     expected restricted __le16 [usertype] cid
   net/bluetooth/hci_core.c:920:28: sparse:     got unsigned short [usertype]
>> net/bluetooth/hci_core.c:921:28: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le16 [usertype] vid @@     got unsigned short [usertype] @@
   net/bluetooth/hci_core.c:921:28: sparse:     expected restricted __le16 [usertype] vid
   net/bluetooth/hci_core.c:921:28: sparse:     got unsigned short [usertype]

vim +920 net/bluetooth/hci_core.c

   905	
   906	static int hci_codec_list_add(struct list_head *list,
   907				      struct hci_op_read_local_codec_caps *sent,
   908				      struct hci_rp_read_local_codec_caps *rp,
   909				      void *caps,
   910				      __u32 len)
   911	{
   912		struct codec_list *entry;
   913	
   914		entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL);
   915		if (!entry)
   916			return -ENOMEM;
   917	
   918		entry->id = sent->id;
   919		if (sent->id == 0xFF) {
 > 920			entry->cid = __le16_to_cpu(sent->cid);
 > 921			entry->vid = __le16_to_cpu(sent->vid);
   922		}
   923		entry->transport = sent->transport;
   924		entry->len = len;
   925		entry->num_caps = rp->num_caps;
   926		if (rp->num_caps)
   927			memcpy(entry->caps, caps, len);
   928		list_add(&entry->list, list);
   929	
   930		return 0;
   931	}
   932	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 24016 bytes --]

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

* Re: [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details
  2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
                   ` (10 preceding siblings ...)
  2021-06-08 18:49   ` kernel test robot
@ 2021-06-15 19:25 ` Marcel Holtmann
  2021-06-16  2:51   ` K, Kiran
  11 siblings, 1 reply; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-15 19:25 UTC (permalink / raw)
  To: Kiran K; +Cc: Bluez mailing list, Chethan T N, Srivatsa Ravishankar

Hi Kiran,

> Move reading of supported local codecs into a separate init function,
> query codecs capabilities and cache the data
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Signed-off-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> Reported-by: kernel test robot <lkp@intel.com>

what is Reported-by? This makes no sense since this is original code.

Regards

Marcel


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

* Re: [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path
  2021-06-08 12:24 ` [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path Kiran K
@ 2021-06-15 19:26   ` Marcel Holtmann
  2021-06-16  2:56     ` K, Kiran
  0 siblings, 1 reply; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-15 19:26 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth

Hi Kiran,

> There is no standard HCI command to retrieve data path for transport.
> Add a new callback function to retrieve data path which is used
> in offload usecase. This needs to be set at setup stage if controller
> supports offload codecs
> 
> 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 v9:
>  - define a separate patch for core changes
> 
> include/net/bluetooth/hci_core.h | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 3284044c3dd7..641477396da3 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)(struct hci_dev *hdev);
> };

and where is the code using hdev->get_data_path. That code needs to be in this patch.

Regards

Marcel


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

* Re: [PATCH v9 04/10] Bluetooth: btintel: set get_data_path callback
  2021-06-08 12:24 ` [PATCH v9 04/10] Bluetooth: btintel: set get_data_path callback Kiran K
@ 2021-06-15 19:30   ` Marcel Holtmann
  2021-06-16  3:02     ` K, Kiran
  0 siblings, 1 reply; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-15 19:30 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth

Hi Kiran,

> Read supported offload usecases and set get_data_path callback if
> controller suppports offload codecs.
> 
> 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>
> ---
> drivers/bluetooth/btintel.c | 36 ++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btintel.h | 18 ++++++++++++++++++
> drivers/bluetooth/btusb.c   |  8 ++++++++
> 3 files changed, 62 insertions(+)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index e44b6993cf91..95c6a1bef35e 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -1272,6 +1272,42 @@ int btintel_set_debug_features(struct hci_dev *hdev,
> }
> EXPORT_SYMBOL_GPL(btintel_set_debug_features);
> 
> +int btintel_get_data_path(struct hci_dev *hdev)
> +{
> +	return 1;
> +}
> +EXPORT_SYMBOL_GPL(btintel_get_data_path);
> +
> +int btintel_read_offload_usecases(struct hci_dev *hdev,
> +				  struct intel_offload_usecases *usecases)
> +{
> +	struct sk_buff *skb;
> +	int err = 0;
> +
> +	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;
> +	}
> +
> +	if (skb->data[0]) {
> +		err = -bt_to_errno(skb->data[0]);
> +		goto error;
> +	}
> +
> +	memcpy(usecases, skb->data, sizeof(*usecases));
> +error:
> +	kfree_skb(skb);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(btintel_read_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..9bcc489680db 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,9 @@ 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_read_offload_usecases(struct hci_dev *hdev,
> +				  struct intel_offload_usecases *usecases);
> +int btintel_get_data_path(struct hci_dev *hdev);
> #else
> 
> static inline int btintel_check_bdaddr(struct hci_dev *hdev)
> @@ -307,4 +315,14 @@ static inline int btintel_set_debug_features(struct hci_dev *hdev,
> 	return -EOPNOTSUPP;
> }
> 
> +static int btintel_read_offload_usecases(struct hci_dev *hdev,
> +					 struct intel_offload_usecases *usecases)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int btintel_get_data_path(struct hci_dev *hdev)
> +{
> +	return -EOPNOTSUPP;
> +}
> #endif
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index a9855a2dd561..1e51beec5776 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2952,6 +2952,7 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
> 	int err;
> 	struct intel_debug_features features;
> 	struct intel_version_tlv version;
> +	struct intel_offload_usecases usecases;
> 
> 	bt_dev_dbg(hdev, "");
> 
> @@ -3008,6 +3009,13 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
> 	/* Set DDC mask for available debug features */
> 	btintel_set_debug_features(hdev, &features);
> 
> +	err = btintel_read_offload_usecases(hdev, &usecases);
> +	if (!err) {
> +		/* set get_data_path callback if offload is supported */
> +		if (usecases.preset[0] & 0x03)
> +			hdev->get_data_path = btintel_get_data_path;
> +	}
> +

I really wonder if this exporting everything single detail to be just used by btusb is really a good idea. Would it be just enough to have a btintel_configure_offload(hdev); helper that does everything and be done with it. There is already too much Intel specifics bleeding into btusb.c and I think we need to correct that going forward.

Regards

Marcel


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

* Re: [PATCH v9 05/10] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket
  2021-06-08 12:24 ` [PATCH v9 05/10] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket Kiran K
@ 2021-06-15 19:37   ` Marcel Holtmann
  0 siblings, 0 replies; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-15 19:37 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth

Hi Kiran,

> Add BT_CODEC option for getsockopt systemcall over SCO socket
> to expose the codecs 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 on v9:
>  - fix typos,review comments, remove quirk
> 
> include/net/bluetooth/bluetooth.h |  20 ++++++
> include/net/bluetooth/hci.h       |   4 ++
> net/bluetooth/sco.c               | 111 +++++++++++++++++++++++++++++-
> 3 files changed, 134 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 9125effbf448..1840756958ce 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;
> +	__le16	cid;
> +	__le16	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 45bd9af4ce61..31a5ac8918fc 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -2619,6 +2619,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/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index d9a4e88dacbb..98d5e24e5680 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -944,10 +944,15 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
> 			       char __user *optval, int __user *optlen)
> {
> 	struct sock *sk = sock->sk;
> -	int len, err = 0;
> +	int len, err = 0, buf_len;
> 	struct bt_voice voice;
> 	u32 phys;
> 	int pkt_status;
> +	struct codec_list *c;
> +	u8 num_codecs, i, __user *ptr;
> +	struct hci_dev *hdev;
> +	struct hci_codec_caps *caps;
> +	__u8	data_path;
> 
> 	BT_DBG("sk %p", sk);
> 
> @@ -1012,6 +1017,110 @@ 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) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}
> +
> +		hci_dev_lock(hdev);
> +		/* find total buffer size  required to copy codec + caps */

please check for simple style mistakes like double spaces. Also I would put the comment above the hci_dev_lock().

> +		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);
> +

And this is missing comment as well.

> +		hci_dev_lock(hdev);
> +		list_for_each_entry(c, &hdev->local_codecs, list) {
> +			if (c->transport != TRANSPORT_SCO_ESCO)
> +				continue;
> +
> +			if (put_user(c->id, ptr)) {
> +				err = -EFAULT;
> +				goto unlock;
> +			}
> +			ptr += sizeof(c->id);
> +
> +			if (put_user(c->cid, ptr)) {
> +				err = -EFAULT;
> +				goto unlock;
> +			}
> +			ptr += sizeof(c->cid);
> +
> +			if (put_user(c->vid, ptr)) {
> +				err = -EFAULT;
> +				goto unlock;
> +			}
> +			ptr += sizeof(c->vid);
> +
> +			err = hdev->get_data_path(hdev);
> +			if (err < 0) {
> +				err = -EFAULT;
> +				goto unlock;
> +			}

Using the variable name err is really bad here. It is also not an EFAULT type of error. They are really specific. I really don’t get why not prepare the data in advance and have a single put_user call.

> +
> +			data_path = (__u8)err;
> +			if (put_user(data_path, ptr)) {
> +				err = -EFAULT;
> +				goto unlock;
> +			}
> +			ptr += sizeof(data_path);
> +
> +			if (put_user(c->num_caps, ptr)) {
> +				err = -EFAULT;
> +				goto unlock;
> +			}
> +			ptr += sizeof(c->num_caps);
> +
> +			len = 0;
> +			for (i = 0, caps = c->caps; i < c->num_caps; i++) {
> +				len += 1 + caps->len;
> +				caps = (void *)&caps->data[caps->len];
> +			}
> +
> +			if (len && copy_to_user(ptr, c->caps, len)) {
> +				err = -EFAULT;
> +				goto unlock;
> +			}
> +			ptr += len;
> +		}
> +
> +		if (put_user(buf_len, optlen))
> +			err = -EFAULT;
> +unlock:
> +		hci_dev_unlock(hdev);

Jumping from within a for-loop is nothing something that I actually like. It is better you break out of via break.

Regards

Marcel


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

* Re: [PATCH v9 06/10] Bluetooth: Add a callback function to set data path
  2021-06-08 12:24 ` [PATCH v9 06/10] Bluetooth: Add a callback function to set data path Kiran K
@ 2021-06-15 19:37   ` Marcel Holtmann
  0 siblings, 0 replies; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-15 19:37 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth

Hi Kiran,

> In HFP offload usecase, Intel controllers require offload use
> case id (NBS or WBS) to be set before opening SCO connection. Define
> a new callback which gets called on setsockopt SCO socket. User space
> audio module is expected to set codec via setsockopt(sk, BT_CODEC, ....)
> before opening SCO connection.
> 
> 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>
> ---
> include/net/bluetooth/hci_core.h | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 641477396da3..ad0024891447 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -618,6 +618,8 @@ struct hci_dev {
> 	void (*cmd_timeout)(struct hci_dev *hdev);
> 	bool (*prevent_wake)(struct hci_dev *hdev);
> 	int (*get_data_path)(struct hci_dev *hdev);
> +	int (*set_data_path)(struct hci_dev *hdev, __u8 type,
> +			     struct bt_codec *codec);
> };
> 

same as the other one, this needs to also provide the user of hdev->set_data_path.

Regards

Marcel


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

* Re: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path
  2021-06-08 12:24 ` [PATCH v9 07/10] Bluetooth: btintel: define callback " Kiran K
@ 2021-06-15 19:39   ` Marcel Holtmann
  2021-06-16  3:10     ` K, Kiran
  0 siblings, 1 reply; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-15 19:39 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth

Hi Kiran,

> Adds callback function which is called to set the data path
> for HFP offload case before opening SCO connection
> 
> 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>
> ---
> drivers/bluetooth/btintel.c | 50 +++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btintel.h |  8 ++++++
> drivers/bluetooth/btusb.c   |  4 ++-
> include/net/bluetooth/hci.h |  8 ++++++
> 4 files changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 95c6a1bef35e..3eba5c587ef6 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -1308,6 +1308,56 @@ int btintel_read_offload_usecases(struct hci_dev *hdev,
> }
> EXPORT_SYMBOL_GPL(btintel_read_offload_usecases);
> 
> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> +			  struct bt_codec *codec)
> +{
> +	__u8 preset;
> +	struct hci_op_configure_data_path *cmd;
> +	__u8 buffer[255];
> +	struct sk_buff *skb;
> +
> +	if (type != SCO_LINK && type != ESCO_LINK)
> +		return -EINVAL;
> +
> +	switch (codec->id) {
> +	case 0x02:
> +		preset = 0x00;
> +	break;
> +	case 0x05:
> +		preset = 0x01;
> +	break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	cmd = (void *)buffer;
> +	cmd->data_path_id = 0x01;
> +	cmd->len = 1;
> +	cmd->data[0] = preset;
> +
> +	cmd->direction = 0x00;
> +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH, sizeof(*cmd) + 1,
> +			     cmd, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		bt_dev_err(hdev, "configure input data path failed (%ld)",
> +			   PTR_ERR(skb));
> +		return PTR_ERR(skb);
> +	}
> +	kfree_skb(skb);
> +
> +	cmd->direction = 0x01;
> +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH, sizeof(*cmd) + 1,
> +			     cmd, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		bt_dev_err(hdev, "configure output data path failed (%ld)",
> +			   PTR_ERR(skb));
> +		return PTR_ERR(skb);
> +	}
> +	kfree_skb(skb);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(btintel_set_data_path);
> +
> 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 9bcc489680db..9806970c9871 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -183,6 +183,8 @@ int btintel_set_debug_features(struct hci_dev *hdev,
> int btintel_read_offload_usecases(struct hci_dev *hdev,
> 				  struct intel_offload_usecases *usecases);
> int btintel_get_data_path(struct hci_dev *hdev);
> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> +			  struct bt_codec *codec);
> #else
> 
> static inline int btintel_check_bdaddr(struct hci_dev *hdev)
> @@ -325,4 +327,10 @@ static int btintel_get_data_path(struct hci_dev *hdev)
> {
> 	return -EOPNOTSUPP;
> }
> +
> +static int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> +				 struct bt_codec *codec)
> +{
> +	return -EOPNOTSUPP;
> +}
> #endif
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 1e51beec5776..afafa44752a1 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3012,8 +3012,10 @@ static int btusb_setup_intel_newgen(struct hci_dev *hdev)
> 	err = btintel_read_offload_usecases(hdev, &usecases);
> 	if (!err) {
> 		/* set get_data_path callback if offload is supported */
> -		if (usecases.preset[0] & 0x03)
> +		if (usecases.preset[0] & 0x03) {
> 			hdev->get_data_path = btintel_get_data_path;
> +			hdev->set_data_path = btintel_set_data_path;
> +		}
> 	}

> 	/* Read the Intel version information after loading the FW  */
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 31a5ac8918fc..42963188dcea 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	len;
> +	__u8	data[];
> +} __packed;
> +

if this is a standard HCI command, why is this done as hdev->set_data_path? This makes no sense too me.

Regards

Marcel


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

* Re: [PATCH v9 10/10] Bluetooth: Add support for msbc coding format
  2021-06-08 12:24 ` [PATCH v9 10/10] Bluetooth: Add support for msbc coding format Kiran K
@ 2021-06-15 19:43   ` Marcel Holtmann
  2021-06-23  3:24     ` K, Kiran
  0 siblings, 1 reply; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-15 19:43 UTC (permalink / raw)
  To: Kiran K; +Cc: linux-bluetooth

Hi Kiran,

> 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>
> ---
> include/net/bluetooth/bluetooth.h |  1 +
> net/bluetooth/hci_conn.c          | 27 ++++++++++++++++++++++++++-
> 2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index af2809121571..056699224da7 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 9569b21adb88..73c134459361 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -328,6 +328,32 @@ bool hci_enhanced_setup_sync(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_trasnport_unit_size = 1;
> +		cp.out_trasnport_unit_size = 1;

so s/trasnport/transport/

Please spellcheck your structs.

> +		break;
> +
> 	case CODING_FORMAT_TRANSPARENT:
> 		if (!find_next_esco_param(conn, esco_param_msbc,
> 					  ARRAY_SIZE(esco_param_msbc)))
> @@ -383,7 +409,6 @@ bool hci_enhanced_setup_sync(struct hci_conn *conn, __u16 handle)
> 		cp.in_trasnport_unit_size = 16;
> 		cp.out_trasnport_unit_size = 16;
> 		break;
> -

We can not have these random hunks in patches. You need to review your final set before sending it out.

Regards

Marcel


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

* RE: [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details
  2021-06-15 19:25 ` Marcel Holtmann
@ 2021-06-16  2:51   ` K, Kiran
  2021-06-16  5:15     ` Marcel Holtmann
  0 siblings, 1 reply; 31+ messages in thread
From: K, Kiran @ 2021-06-16  2:51 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Bluez mailing list, Tumkur Narayan, Chethan, Srivatsa, Ravishankar

Hi Marcel,

> 
> Hi Kiran,
> 
> > Move reading of supported local codecs into a separate init function,
> > query codecs capabilities and cache the data
> >
> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Signed-off-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> what is Reported-by? This makes no sense since this is original code.

Got a compiler warning in one of the patchset. Hence added "Reported-by". Ok to remove this in the next patchset.
> 
> Regards
> 
> Marcel

Regards,
Kiran



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

* RE: [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path
  2021-06-15 19:26   ` Marcel Holtmann
@ 2021-06-16  2:56     ` K, Kiran
  2021-06-16  5:17       ` Marcel Holtmann
  0 siblings, 1 reply; 31+ messages in thread
From: K, Kiran @ 2021-06-16  2:56 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

> Hi Kiran,
> 
> > There is no standard HCI command to retrieve data path for transport.
> > Add a new callback function to retrieve data path which is used in
> > offload usecase. This needs to be set at setup stage if controller
> > supports offload codecs
> >
> > 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 v9:
> >  - define a separate patch for core changes
> >
> > include/net/bluetooth/hci_core.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/include/net/bluetooth/hci_core.h
> > b/include/net/bluetooth/hci_core.h
> > index 3284044c3dd7..641477396da3 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)(struct hci_dev *hdev);
> > };
> 
> and where is the code using hdev->get_data_path. That code needs to be in
> this patch.

In the previous patchset, there was a comment to separate out driver and core changes. Let me know if I am missing something here.
https://patchwork.kernel.org/project/bluetooth/patch/20210518104232.5431-3-kiran.k@intel.com/

> 
> Regards
> 
> Marcel

Regards,
Kiran



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

* RE: [PATCH v9 04/10] Bluetooth: btintel: set get_data_path callback
  2021-06-15 19:30   ` Marcel Holtmann
@ 2021-06-16  3:02     ` K, Kiran
  0 siblings, 0 replies; 31+ messages in thread
From: K, Kiran @ 2021-06-16  3:02 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

> Hi Kiran,
> 
> > Read supported offload usecases and set get_data_path callback if
> > controller suppports offload codecs.
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index a9855a2dd561..1e51beec5776 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2952,6 +2952,7 @@ static int btusb_setup_intel_newgen(struct
> hci_dev *hdev)
> > 	int err;
> > 	struct intel_debug_features features;
> > 	struct intel_version_tlv version;
> > +	struct intel_offload_usecases usecases;
> >
> > 	bt_dev_dbg(hdev, "");
> >
> > @@ -3008,6 +3009,13 @@ static int btusb_setup_intel_newgen(struct
> hci_dev *hdev)
> > 	/* Set DDC mask for available debug features */
> > 	btintel_set_debug_features(hdev, &features);
> >
> > +	err = btintel_read_offload_usecases(hdev, &usecases);
> > +	if (!err) {
> > +		/* set get_data_path callback if offload is supported */
> > +		if (usecases.preset[0] & 0x03)
> > +			hdev->get_data_path = btintel_get_data_path;
> > +	}
> > +
> 
> I really wonder if this exporting everything single detail to be just used by
> btusb is really a good idea. Would it be just enough to have a
> btintel_configure_offload(hdev); helper that does everything and be done
> with it. There is already too much Intel specifics bleeding into btusb.c and I
> think we need to correct that going forward.
> 

Ok. I will refactor this to move Intel specific code to btintel*
> Regards
> 
> Marcel

Thanks,
Kiran



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

* RE: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path
  2021-06-15 19:39   ` Marcel Holtmann
@ 2021-06-16  3:10     ` K, Kiran
  2021-06-16  5:18       ` Marcel Holtmann
  0 siblings, 1 reply; 31+ messages in thread
From: K, Kiran @ 2021-06-16  3:10 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

> 
> Hi Kiran,
> 
> > Adds callback function which is called to set the data path for HFP
> > offload case before opening SCO connection
> >
> > 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>
> > ---
> > drivers/bluetooth/btintel.c | 50
> +++++++++++++++++++++++++++++++++++++
> > drivers/bluetooth/btintel.h |  8 ++++++
> > drivers/bluetooth/btusb.c   |  4 ++-
> > include/net/bluetooth/hci.h |  8 ++++++
> > 4 files changed, 69 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 95c6a1bef35e..3eba5c587ef6 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -1308,6 +1308,56 @@ int btintel_read_offload_usecases(struct
> > hci_dev *hdev, } EXPORT_SYMBOL_GPL(btintel_read_offload_usecases);
> >
> > +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> > +			  struct bt_codec *codec)
> > +{
> > +	__u8 preset;
> > +	struct hci_op_configure_data_path *cmd;
> > +	__u8 buffer[255];
> > +	struct sk_buff *skb;
> > +
> > +	if (type != SCO_LINK && type != ESCO_LINK)
> > +		return -EINVAL;
> > +
> > +	switch (codec->id) {
> > +	case 0x02:
> > +		preset = 0x00;
> > +	break;
> > +	case 0x05:
> > +		preset = 0x01;
> > +	break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	cmd = (void *)buffer;
> > +	cmd->data_path_id = 0x01;
> > +	cmd->len = 1;
> > +	cmd->data[0] = preset;
> > +
> > +	cmd->direction = 0x00;
> > +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
> sizeof(*cmd) + 1,
> > +			     cmd, HCI_INIT_TIMEOUT);
> > +	if (IS_ERR(skb)) {
> > +		bt_dev_err(hdev, "configure input data path failed (%ld)",
> > +			   PTR_ERR(skb));
> > +		return PTR_ERR(skb);
> > +	}
> > +	kfree_skb(skb);
> > +
> > +	cmd->direction = 0x01;
> > +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
> sizeof(*cmd) + 1,
> > +			     cmd, HCI_INIT_TIMEOUT);
> > +	if (IS_ERR(skb)) {
> > +		bt_dev_err(hdev, "configure output data path failed (%ld)",
> > +			   PTR_ERR(skb));
> > +		return PTR_ERR(skb);
> > +	}
> > +	kfree_skb(skb);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(btintel_set_data_path);
> > +
> > 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
> > 9bcc489680db..9806970c9871 100644
> > --- a/drivers/bluetooth/btintel.h
> > +++ b/drivers/bluetooth/btintel.h
> > @@ -183,6 +183,8 @@ int btintel_set_debug_features(struct hci_dev
> > *hdev, int btintel_read_offload_usecases(struct hci_dev *hdev,
> > 				  struct intel_offload_usecases *usecases); int
> > btintel_get_data_path(struct hci_dev *hdev);
> > +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> > +			  struct bt_codec *codec);
> > #else
> >
> > static inline int btintel_check_bdaddr(struct hci_dev *hdev) @@ -325,4
> > +327,10 @@ static int btintel_get_data_path(struct hci_dev *hdev) {
> > 	return -EOPNOTSUPP;
> > }
> > +
> > +static int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> > +				 struct bt_codec *codec)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > #endif
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 1e51beec5776..afafa44752a1 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -3012,8 +3012,10 @@ static int btusb_setup_intel_newgen(struct
> hci_dev *hdev)
> > 	err = btintel_read_offload_usecases(hdev, &usecases);
> > 	if (!err) {
> > 		/* set get_data_path callback if offload is supported */
> > -		if (usecases.preset[0] & 0x03)
> > +		if (usecases.preset[0] & 0x03) {
> > 			hdev->get_data_path = btintel_get_data_path;
> > +			hdev->set_data_path = btintel_set_data_path;
> > +		}
> > 	}
> 
> > 	/* Read the Intel version information after loading the FW  */ diff
> > --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 31a5ac8918fc..42963188dcea 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	len;
> > +	__u8	data[];
> > +} __packed;
> > +
> 
> if this is a standard HCI command, why is this done as hdev->set_data_path?
> This makes no sense too me.
Intel uses  HCI_CONFIGURE_DATA_PATH to command to set the preset ID (NBS, WBS, ...). Here len and data[] are vendor specific. I should have prefixed these fields with vnd_. I will address this in next patchset.
> 
> Regards
> 
> Marcel

Thanks,
Kiran


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

* Re: [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details
  2021-06-16  2:51   ` K, Kiran
@ 2021-06-16  5:15     ` Marcel Holtmann
  0 siblings, 0 replies; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-16  5:15 UTC (permalink / raw)
  To: K, Kiran
  Cc: Bluez mailing list, Tumkur Narayan, Chethan, Srivatsa, Ravishankar

Hi Kiran,

>>> Move reading of supported local codecs into a separate init function,
>>> query codecs capabilities and cache the data
>>> 
>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>> Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
>>> Signed-off-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
>>> Reported-by: kernel test robot <lkp@intel.com>
>> 
>> what is Reported-by? This makes no sense since this is original code.
> 
> Got a compiler warning in one of the patchset. Hence added "Reported-by". Ok to remove this in the next patchset.

yes since they have not been yet upstream.

Regards

Marcel


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

* Re: [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path
  2021-06-16  2:56     ` K, Kiran
@ 2021-06-16  5:17       ` Marcel Holtmann
  0 siblings, 0 replies; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-16  5:17 UTC (permalink / raw)
  To: K, Kiran; +Cc: linux-bluetooth

Hi Kiran,

>>> There is no standard HCI command to retrieve data path for transport.
>>> Add a new callback function to retrieve data path which is used in
>>> offload usecase. This needs to be set at setup stage if controller
>>> supports offload codecs
>>> 
>>> 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 v9:
>>> - define a separate patch for core changes
>>> 
>>> include/net/bluetooth/hci_core.h | 1 +
>>> 1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/include/net/bluetooth/hci_core.h
>>> b/include/net/bluetooth/hci_core.h
>>> index 3284044c3dd7..641477396da3 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)(struct hci_dev *hdev);
>>> };
>> 
>> and where is the code using hdev->get_data_path. That code needs to be in
>> this patch.
> 
> In the previous patchset, there was a comment to separate out driver and core changes. Let me know if I am missing something here.
> https://patchwork.kernel.org/project/bluetooth/patch/20210518104232.5431-3-kiran.k@intel.com/
> 

I know that and this is not contradictory. Introducing such a callback must come with the usage of said callback. Usage means the core side and not the driver side of it.

Regards

Marcel


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

* Re: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path
  2021-06-16  3:10     ` K, Kiran
@ 2021-06-16  5:18       ` Marcel Holtmann
  2021-06-17  7:53         ` K, Kiran
  0 siblings, 1 reply; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-16  5:18 UTC (permalink / raw)
  To: K, Kiran; +Cc: linux-bluetooth

Hi Kiran,

>>> Adds callback function which is called to set the data path for HFP
>>> offload case before opening SCO connection
>>> 
>>> 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>
>>> ---
>>> drivers/bluetooth/btintel.c | 50
>> +++++++++++++++++++++++++++++++++++++
>>> drivers/bluetooth/btintel.h |  8 ++++++
>>> drivers/bluetooth/btusb.c   |  4 ++-
>>> include/net/bluetooth/hci.h |  8 ++++++
>>> 4 files changed, 69 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
>>> index 95c6a1bef35e..3eba5c587ef6 100644
>>> --- a/drivers/bluetooth/btintel.c
>>> +++ b/drivers/bluetooth/btintel.c
>>> @@ -1308,6 +1308,56 @@ int btintel_read_offload_usecases(struct
>>> hci_dev *hdev, } EXPORT_SYMBOL_GPL(btintel_read_offload_usecases);
>>> 
>>> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
>>> +			  struct bt_codec *codec)
>>> +{
>>> +	__u8 preset;
>>> +	struct hci_op_configure_data_path *cmd;
>>> +	__u8 buffer[255];
>>> +	struct sk_buff *skb;
>>> +
>>> +	if (type != SCO_LINK && type != ESCO_LINK)
>>> +		return -EINVAL;
>>> +
>>> +	switch (codec->id) {
>>> +	case 0x02:
>>> +		preset = 0x00;
>>> +	break;
>>> +	case 0x05:
>>> +		preset = 0x01;
>>> +	break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	cmd = (void *)buffer;
>>> +	cmd->data_path_id = 0x01;
>>> +	cmd->len = 1;
>>> +	cmd->data[0] = preset;
>>> +
>>> +	cmd->direction = 0x00;
>>> +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
>> sizeof(*cmd) + 1,
>>> +			     cmd, HCI_INIT_TIMEOUT);
>>> +	if (IS_ERR(skb)) {
>>> +		bt_dev_err(hdev, "configure input data path failed (%ld)",
>>> +			   PTR_ERR(skb));
>>> +		return PTR_ERR(skb);
>>> +	}
>>> +	kfree_skb(skb);
>>> +
>>> +	cmd->direction = 0x01;
>>> +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
>> sizeof(*cmd) + 1,
>>> +			     cmd, HCI_INIT_TIMEOUT);
>>> +	if (IS_ERR(skb)) {
>>> +		bt_dev_err(hdev, "configure output data path failed (%ld)",
>>> +			   PTR_ERR(skb));
>>> +		return PTR_ERR(skb);
>>> +	}
>>> +	kfree_skb(skb);
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(btintel_set_data_path);
>>> +
>>> 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
>>> 9bcc489680db..9806970c9871 100644
>>> --- a/drivers/bluetooth/btintel.h
>>> +++ b/drivers/bluetooth/btintel.h
>>> @@ -183,6 +183,8 @@ int btintel_set_debug_features(struct hci_dev
>>> *hdev, int btintel_read_offload_usecases(struct hci_dev *hdev,
>>> 				  struct intel_offload_usecases *usecases); int
>>> btintel_get_data_path(struct hci_dev *hdev);
>>> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
>>> +			  struct bt_codec *codec);
>>> #else
>>> 
>>> static inline int btintel_check_bdaddr(struct hci_dev *hdev) @@ -325,4
>>> +327,10 @@ static int btintel_get_data_path(struct hci_dev *hdev) {
>>> 	return -EOPNOTSUPP;
>>> }
>>> +
>>> +static int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
>>> +				 struct bt_codec *codec)
>>> +{
>>> +	return -EOPNOTSUPP;
>>> +}
>>> #endif
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 1e51beec5776..afafa44752a1 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -3012,8 +3012,10 @@ static int btusb_setup_intel_newgen(struct
>> hci_dev *hdev)
>>> 	err = btintel_read_offload_usecases(hdev, &usecases);
>>> 	if (!err) {
>>> 		/* set get_data_path callback if offload is supported */
>>> -		if (usecases.preset[0] & 0x03)
>>> +		if (usecases.preset[0] & 0x03) {
>>> 			hdev->get_data_path = btintel_get_data_path;
>>> +			hdev->set_data_path = btintel_set_data_path;
>>> +		}
>>> 	}
>> 
>>> 	/* Read the Intel version information after loading the FW  */ diff
>>> --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>> index 31a5ac8918fc..42963188dcea 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	len;
>>> +	__u8	data[];
>>> +} __packed;
>>> +
>> 
>> if this is a standard HCI command, why is this done as hdev->set_data_path?
>> This makes no sense too me.
> Intel uses  HCI_CONFIGURE_DATA_PATH to command to set the preset ID (NBS, WBS, ...). Here len and data[] are vendor specific. I should have prefixed these fields with vnd_. I will address this in next patchset.

if the command is defined by the Bluetooth SIG, it is handle in the core. However if it needs vendor specific input that we need a callback for just that data.

Regards

Marcel


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

* RE: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path
  2021-06-16  5:18       ` Marcel Holtmann
@ 2021-06-17  7:53         ` K, Kiran
  2021-06-17 10:19           ` Marcel Holtmann
  0 siblings, 1 reply; 31+ messages in thread
From: K, Kiran @ 2021-06-17  7:53 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Wednesday, June 16, 2021 10:48 AM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data
> path
> 
> Hi Kiran,
> 
> >>> Adds callback function which is called to set the data path for HFP
> >>> offload case before opening SCO connection
> >>>
> >>> 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>
> >>> ---
> >>> drivers/bluetooth/btintel.c | 50
> >> +++++++++++++++++++++++++++++++++++++
> >>> drivers/bluetooth/btintel.h |  8 ++++++
> >>> drivers/bluetooth/btusb.c   |  4 ++-
> >>> include/net/bluetooth/hci.h |  8 ++++++
> >>> 4 files changed, 69 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/bluetooth/btintel.c
> >>> b/drivers/bluetooth/btintel.c index 95c6a1bef35e..3eba5c587ef6
> >>> 100644
> >>> --- a/drivers/bluetooth/btintel.c
> >>> +++ b/drivers/bluetooth/btintel.c
> >>> @@ -1308,6 +1308,56 @@ int btintel_read_offload_usecases(struct
> >>> hci_dev *hdev, } EXPORT_SYMBOL_GPL(btintel_read_offload_usecases);
> >>>
> >>> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> >>> +			  struct bt_codec *codec)
> >>> +{
> >>> +	__u8 preset;
> >>> +	struct hci_op_configure_data_path *cmd;
> >>> +	__u8 buffer[255];
> >>> +	struct sk_buff *skb;
> >>> +
> >>> +	if (type != SCO_LINK && type != ESCO_LINK)
> >>> +		return -EINVAL;
> >>> +
> >>> +	switch (codec->id) {
> >>> +	case 0x02:
> >>> +		preset = 0x00;
> >>> +	break;
> >>> +	case 0x05:
> >>> +		preset = 0x01;
> >>> +	break;
> >>> +	default:
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	cmd = (void *)buffer;
> >>> +	cmd->data_path_id = 0x01;
> >>> +	cmd->len = 1;
> >>> +	cmd->data[0] = preset;
> >>> +
> >>> +	cmd->direction = 0x00;
> >>> +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
> >> sizeof(*cmd) + 1,
> >>> +			     cmd, HCI_INIT_TIMEOUT);
> >>> +	if (IS_ERR(skb)) {
> >>> +		bt_dev_err(hdev, "configure input data path failed (%ld)",
> >>> +			   PTR_ERR(skb));
> >>> +		return PTR_ERR(skb);
> >>> +	}
> >>> +	kfree_skb(skb);
> >>> +
> >>> +	cmd->direction = 0x01;
> >>> +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
> >> sizeof(*cmd) + 1,
> >>> +			     cmd, HCI_INIT_TIMEOUT);
> >>> +	if (IS_ERR(skb)) {
> >>> +		bt_dev_err(hdev, "configure output data path failed (%ld)",
> >>> +			   PTR_ERR(skb));
> >>> +		return PTR_ERR(skb);
> >>> +	}
> >>> +	kfree_skb(skb);
> >>> +	return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(btintel_set_data_path);
> >>> +
> >>> 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
> >>> 9bcc489680db..9806970c9871 100644
> >>> --- a/drivers/bluetooth/btintel.h
> >>> +++ b/drivers/bluetooth/btintel.h
> >>> @@ -183,6 +183,8 @@ int btintel_set_debug_features(struct hci_dev
> >>> *hdev, int btintel_read_offload_usecases(struct hci_dev *hdev,
> >>> 				  struct intel_offload_usecases *usecases); int
> >>> btintel_get_data_path(struct hci_dev *hdev);
> >>> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> >>> +			  struct bt_codec *codec);
> >>> #else
> >>>
> >>> static inline int btintel_check_bdaddr(struct hci_dev *hdev) @@
> >>> -325,4
> >>> +327,10 @@ static int btintel_get_data_path(struct hci_dev *hdev) {
> >>> 	return -EOPNOTSUPP;
> >>> }
> >>> +
> >>> +static int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> >>> +				 struct bt_codec *codec)
> >>> +{
> >>> +	return -EOPNOTSUPP;
> >>> +}
> >>> #endif
> >>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> >>> index 1e51beec5776..afafa44752a1 100644
> >>> --- a/drivers/bluetooth/btusb.c
> >>> +++ b/drivers/bluetooth/btusb.c
> >>> @@ -3012,8 +3012,10 @@ static int btusb_setup_intel_newgen(struct
> >> hci_dev *hdev)
> >>> 	err = btintel_read_offload_usecases(hdev, &usecases);
> >>> 	if (!err) {
> >>> 		/* set get_data_path callback if offload is supported */
> >>> -		if (usecases.preset[0] & 0x03)
> >>> +		if (usecases.preset[0] & 0x03) {
> >>> 			hdev->get_data_path = btintel_get_data_path;
> >>> +			hdev->set_data_path = btintel_set_data_path;
> >>> +		}
> >>> 	}
> >>
> >>> 	/* Read the Intel version information after loading the FW  */ diff
> >>> --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> >>> index 31a5ac8918fc..42963188dcea 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	len;
> >>> +	__u8	data[];
> >>> +} __packed;
> >>> +
> >>
> >> if this is a standard HCI command, why is this done as hdev-
> >set_data_path?
> >> This makes no sense too me.
> > Intel uses  HCI_CONFIGURE_DATA_PATH to command to set the preset ID
> (NBS, WBS, ...). Here len and data[] are vendor specific. I should have
> prefixed these fields with vnd_. I will address this in next patchset.
> 
> if the command is defined by the Bluetooth SIG, it is handle in the core.
> However if it needs vendor specific input that we need a callback for just that
> data.

The current design uses HCI_CONFIGURE_DATA_PATH inside  set_data_path callback and its not used at core.  I have leveraged SIG command here  to minimize defining  of new vendor command as vnd_data[] gives flexibility to pass in non-standard values. Other vendors may not have same command/flow to configure data path. 

If we are not supposed to use Bluetooth SIG command at driver level, then I need to come up with a new vendor specific command.  Please help with your input.
> 
> Regards
> 
> Marcel
Thanks,
Kiran


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

* Re: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path
  2021-06-17  7:53         ` K, Kiran
@ 2021-06-17 10:19           ` Marcel Holtmann
  2021-06-23  3:21             ` K, Kiran
  0 siblings, 1 reply; 31+ messages in thread
From: Marcel Holtmann @ 2021-06-17 10:19 UTC (permalink / raw)
  To: K, Kiran; +Cc: linux-bluetooth

Hi Kiran,

>>>>> Adds callback function which is called to set the data path for HFP
>>>>> offload case before opening SCO connection
>>>>> 
>>>>> 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>
>>>>> ---
>>>>> drivers/bluetooth/btintel.c | 50
>>>> +++++++++++++++++++++++++++++++++++++
>>>>> drivers/bluetooth/btintel.h |  8 ++++++
>>>>> drivers/bluetooth/btusb.c   |  4 ++-
>>>>> include/net/bluetooth/hci.h |  8 ++++++
>>>>> 4 files changed, 69 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/drivers/bluetooth/btintel.c
>>>>> b/drivers/bluetooth/btintel.c index 95c6a1bef35e..3eba5c587ef6
>>>>> 100644
>>>>> --- a/drivers/bluetooth/btintel.c
>>>>> +++ b/drivers/bluetooth/btintel.c
>>>>> @@ -1308,6 +1308,56 @@ int btintel_read_offload_usecases(struct
>>>>> hci_dev *hdev, } EXPORT_SYMBOL_GPL(btintel_read_offload_usecases);
>>>>> 
>>>>> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
>>>>> +			  struct bt_codec *codec)
>>>>> +{
>>>>> +	__u8 preset;
>>>>> +	struct hci_op_configure_data_path *cmd;
>>>>> +	__u8 buffer[255];
>>>>> +	struct sk_buff *skb;
>>>>> +
>>>>> +	if (type != SCO_LINK && type != ESCO_LINK)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	switch (codec->id) {
>>>>> +	case 0x02:
>>>>> +		preset = 0x00;
>>>>> +	break;
>>>>> +	case 0x05:
>>>>> +		preset = 0x01;
>>>>> +	break;
>>>>> +	default:
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	cmd = (void *)buffer;
>>>>> +	cmd->data_path_id = 0x01;
>>>>> +	cmd->len = 1;
>>>>> +	cmd->data[0] = preset;
>>>>> +
>>>>> +	cmd->direction = 0x00;
>>>>> +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
>>>> sizeof(*cmd) + 1,
>>>>> +			     cmd, HCI_INIT_TIMEOUT);
>>>>> +	if (IS_ERR(skb)) {
>>>>> +		bt_dev_err(hdev, "configure input data path failed (%ld)",
>>>>> +			   PTR_ERR(skb));
>>>>> +		return PTR_ERR(skb);
>>>>> +	}
>>>>> +	kfree_skb(skb);
>>>>> +
>>>>> +	cmd->direction = 0x01;
>>>>> +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
>>>> sizeof(*cmd) + 1,
>>>>> +			     cmd, HCI_INIT_TIMEOUT);
>>>>> +	if (IS_ERR(skb)) {
>>>>> +		bt_dev_err(hdev, "configure output data path failed (%ld)",
>>>>> +			   PTR_ERR(skb));
>>>>> +		return PTR_ERR(skb);
>>>>> +	}
>>>>> +	kfree_skb(skb);
>>>>> +	return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(btintel_set_data_path);
>>>>> +
>>>>> 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
>>>>> 9bcc489680db..9806970c9871 100644
>>>>> --- a/drivers/bluetooth/btintel.h
>>>>> +++ b/drivers/bluetooth/btintel.h
>>>>> @@ -183,6 +183,8 @@ int btintel_set_debug_features(struct hci_dev
>>>>> *hdev, int btintel_read_offload_usecases(struct hci_dev *hdev,
>>>>> 				  struct intel_offload_usecases *usecases); int
>>>>> btintel_get_data_path(struct hci_dev *hdev);
>>>>> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
>>>>> +			  struct bt_codec *codec);
>>>>> #else
>>>>> 
>>>>> static inline int btintel_check_bdaddr(struct hci_dev *hdev) @@
>>>>> -325,4
>>>>> +327,10 @@ static int btintel_get_data_path(struct hci_dev *hdev) {
>>>>> 	return -EOPNOTSUPP;
>>>>> }
>>>>> +
>>>>> +static int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
>>>>> +				 struct bt_codec *codec)
>>>>> +{
>>>>> +	return -EOPNOTSUPP;
>>>>> +}
>>>>> #endif
>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>> index 1e51beec5776..afafa44752a1 100644
>>>>> --- a/drivers/bluetooth/btusb.c
>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>> @@ -3012,8 +3012,10 @@ static int btusb_setup_intel_newgen(struct
>>>> hci_dev *hdev)
>>>>> 	err = btintel_read_offload_usecases(hdev, &usecases);
>>>>> 	if (!err) {
>>>>> 		/* set get_data_path callback if offload is supported */
>>>>> -		if (usecases.preset[0] & 0x03)
>>>>> +		if (usecases.preset[0] & 0x03) {
>>>>> 			hdev->get_data_path = btintel_get_data_path;
>>>>> +			hdev->set_data_path = btintel_set_data_path;
>>>>> +		}
>>>>> 	}
>>>> 
>>>>> 	/* Read the Intel version information after loading the FW  */ diff
>>>>> --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>>>> index 31a5ac8918fc..42963188dcea 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	len;
>>>>> +	__u8	data[];
>>>>> +} __packed;
>>>>> +
>>>> 
>>>> if this is a standard HCI command, why is this done as hdev-
>>> set_data_path?
>>>> This makes no sense too me.
>>> Intel uses  HCI_CONFIGURE_DATA_PATH to command to set the preset ID
>> (NBS, WBS, ...). Here len and data[] are vendor specific. I should have
>> prefixed these fields with vnd_. I will address this in next patchset.
>> 
>> if the command is defined by the Bluetooth SIG, it is handle in the core.
>> However if it needs vendor specific input that we need a callback for just that
>> data.
> 
> The current design uses HCI_CONFIGURE_DATA_PATH inside  set_data_path callback and its not used at core.  I have leveraged SIG command here  to minimize defining  of new vendor command as vnd_data[] gives flexibility to pass in non-standard values. Other vendors may not have same command/flow to configure data path. 
> 
> If we are not supposed to use Bluetooth SIG command at driver level, then I need to come up with a new vendor specific command.  Please help with your input.

I don’t understand this argumentation. The Bluetooth standard defined HCI_Configure_Data_Path with Vendor_Specific_Config for exactly this reason. So just use it especially if our controllers already support it.

Now I am starting to wonder if this design is making things complicated for no reason. Isn’t it enough to have a hdev->get_data_path_config callback that allows to retrieve such data from the driver.

Frankly, the only thing you need from a driver is that it tells you the values of data_path_id and the vendor_config so that you can feed it back into the controller. Or am I missing anything here?

Let me be clear, if there is a SIG defined command, we implement support for in the core and not the driver. I do not want that other vendors have to define everything over and over again. That is what a standard is for. Only the vendor specific portions are handed off to the driver or userspace to provide.

Regards

Marcel


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

* RE: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path
  2021-06-17 10:19           ` Marcel Holtmann
@ 2021-06-23  3:21             ` K, Kiran
  0 siblings, 0 replies; 31+ messages in thread
From: K, Kiran @ 2021-06-23  3:21 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Thursday, June 17, 2021 3:50 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data
> path
> 
> Hi Kiran,
> 
> >>>>> Adds callback function which is called to set the data path for
> >>>>> HFP offload case before opening SCO connection
> >>>>>
> >>>>> 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>
> >>>>> ---
> >>>>> drivers/bluetooth/btintel.c | 50
> >>>> +++++++++++++++++++++++++++++++++++++
> >>>>> drivers/bluetooth/btintel.h |  8 ++++++
> >>>>> drivers/bluetooth/btusb.c   |  4 ++-
> >>>>> include/net/bluetooth/hci.h |  8 ++++++
> >>>>> 4 files changed, 69 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/bluetooth/btintel.c
> >>>>> b/drivers/bluetooth/btintel.c index 95c6a1bef35e..3eba5c587ef6
> >>>>> 100644
> >>>>> --- a/drivers/bluetooth/btintel.c
> >>>>> +++ b/drivers/bluetooth/btintel.c
> >>>>> @@ -1308,6 +1308,56 @@ int btintel_read_offload_usecases(struct
> >>>>> hci_dev *hdev, }
> EXPORT_SYMBOL_GPL(btintel_read_offload_usecases);
> >>>>>
> >>>>> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> >>>>> +			  struct bt_codec *codec)
> >>>>> +{
> >>>>> +	__u8 preset;
> >>>>> +	struct hci_op_configure_data_path *cmd;
> >>>>> +	__u8 buffer[255];
> >>>>> +	struct sk_buff *skb;
> >>>>> +
> >>>>> +	if (type != SCO_LINK && type != ESCO_LINK)
> >>>>> +		return -EINVAL;
> >>>>> +
> >>>>> +	switch (codec->id) {
> >>>>> +	case 0x02:
> >>>>> +		preset = 0x00;
> >>>>> +	break;
> >>>>> +	case 0x05:
> >>>>> +		preset = 0x01;
> >>>>> +	break;
> >>>>> +	default:
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>> +
> >>>>> +	cmd = (void *)buffer;
> >>>>> +	cmd->data_path_id = 0x01;
> >>>>> +	cmd->len = 1;
> >>>>> +	cmd->data[0] = preset;
> >>>>> +
> >>>>> +	cmd->direction = 0x00;
> >>>>> +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
> >>>> sizeof(*cmd) + 1,
> >>>>> +			     cmd, HCI_INIT_TIMEOUT);
> >>>>> +	if (IS_ERR(skb)) {
> >>>>> +		bt_dev_err(hdev, "configure input data path failed
> (%ld)",
> >>>>> +			   PTR_ERR(skb));
> >>>>> +		return PTR_ERR(skb);
> >>>>> +	}
> >>>>> +	kfree_skb(skb);
> >>>>> +
> >>>>> +	cmd->direction = 0x01;
> >>>>> +	skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
> >>>> sizeof(*cmd) + 1,
> >>>>> +			     cmd, HCI_INIT_TIMEOUT);
> >>>>> +	if (IS_ERR(skb)) {
> >>>>> +		bt_dev_err(hdev, "configure output data path failed
> (%ld)",
> >>>>> +			   PTR_ERR(skb));
> >>>>> +		return PTR_ERR(skb);
> >>>>> +	}
> >>>>> +	kfree_skb(skb);
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(btintel_set_data_path);
> >>>>> +
> >>>>> 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
> >>>>> 9bcc489680db..9806970c9871 100644
> >>>>> --- a/drivers/bluetooth/btintel.h
> >>>>> +++ b/drivers/bluetooth/btintel.h
> >>>>> @@ -183,6 +183,8 @@ int btintel_set_debug_features(struct hci_dev
> >>>>> *hdev, int btintel_read_offload_usecases(struct hci_dev *hdev,
> >>>>> 				  struct intel_offload_usecases *usecases); int
> >>>>> btintel_get_data_path(struct hci_dev *hdev);
> >>>>> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> >>>>> +			  struct bt_codec *codec);
> >>>>> #else
> >>>>>
> >>>>> static inline int btintel_check_bdaddr(struct hci_dev *hdev) @@
> >>>>> -325,4
> >>>>> +327,10 @@ static int btintel_get_data_path(struct hci_dev *hdev)
> >>>>> +{
> >>>>> 	return -EOPNOTSUPP;
> >>>>> }
> >>>>> +
> >>>>> +static int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
> >>>>> +				 struct bt_codec *codec)
> >>>>> +{
> >>>>> +	return -EOPNOTSUPP;
> >>>>> +}
> >>>>> #endif
> >>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> >>>>> index 1e51beec5776..afafa44752a1 100644
> >>>>> --- a/drivers/bluetooth/btusb.c
> >>>>> +++ b/drivers/bluetooth/btusb.c
> >>>>> @@ -3012,8 +3012,10 @@ static int btusb_setup_intel_newgen(struct
> >>>> hci_dev *hdev)
> >>>>> 	err = btintel_read_offload_usecases(hdev, &usecases);
> >>>>> 	if (!err) {
> >>>>> 		/* set get_data_path callback if offload is supported */
> >>>>> -		if (usecases.preset[0] & 0x03)
> >>>>> +		if (usecases.preset[0] & 0x03) {
> >>>>> 			hdev->get_data_path = btintel_get_data_path;
> >>>>> +			hdev->set_data_path =
> btintel_set_data_path;
> >>>>> +		}
> >>>>> 	}
> >>>>
> >>>>> 	/* Read the Intel version information after loading the FW  */
> >>>>> diff --git a/include/net/bluetooth/hci.h
> >>>>> b/include/net/bluetooth/hci.h index 31a5ac8918fc..42963188dcea
> >>>>> 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	len;
> >>>>> +	__u8	data[];
> >>>>> +} __packed;
> >>>>> +
> >>>>
> >>>> if this is a standard HCI command, why is this done as hdev-
> >>> set_data_path?
> >>>> This makes no sense too me.
> >>> Intel uses  HCI_CONFIGURE_DATA_PATH to command to set the preset
> ID
> >> (NBS, WBS, ...). Here len and data[] are vendor specific. I should
> >> have prefixed these fields with vnd_. I will address this in next patchset.
> >>
> >> if the command is defined by the Bluetooth SIG, it is handle in the core.
> >> However if it needs vendor specific input that we need a callback for
> >> just that data.
> >
> > The current design uses HCI_CONFIGURE_DATA_PATH inside
> set_data_path callback and its not used at core.  I have leveraged SIG
> command here  to minimize defining  of new vendor command as vnd_data[]
> gives flexibility to pass in non-standard values. Other vendors may not have
> same command/flow to configure data path.
> >
> > If we are not supposed to use Bluetooth SIG command at driver level, then
> I need to come up with a new vendor specific command.  Please help with
> your input.
> 
> I don’t understand this argumentation. The Bluetooth standard defined
> HCI_Configure_Data_Path with Vendor_Specific_Config for exactly this
> reason. So just use it especially if our controllers already support it.
> 
> Now I am starting to wonder if this design is making things complicated for
> no reason. Isn’t it enough to have a hdev->get_data_path_config callback
> that allows to retrieve such data from the driver.
> 
> Frankly, the only thing you need from a driver is that it tells you the values of
> data_path_id and the vendor_config so that you can feed it back into the
> controller. Or am I missing anything here?
> 
> Let me be clear, if there is a SIG defined command, we implement support
> for in the core and not the driver. I do not want that other vendors have to
> define everything over and over again. That is what a standard is for. Only
> the vendor specific portions are handed off to the driver or userspace to
> provide.

Agree. I will make the suggested changes in the next patchset.
> 
> Regards
> 
> Marcel

Thanks,
Kiran



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

* RE: [PATCH v9 10/10] Bluetooth: Add support for msbc coding format
  2021-06-15 19:43   ` Marcel Holtmann
@ 2021-06-23  3:24     ` K, Kiran
  0 siblings, 0 replies; 31+ messages in thread
From: K, Kiran @ 2021-06-23  3:24 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Wednesday, June 16, 2021 1:14 AM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH v9 10/10] Bluetooth: Add support for msbc coding
> format
> 
> Hi Kiran,
> 
> > 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>
> > ---
> > include/net/bluetooth/bluetooth.h |  1 +
> > net/bluetooth/hci_conn.c          | 27 ++++++++++++++++++++++++++-
> > 2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h
> > b/include/net/bluetooth/bluetooth.h
> > index af2809121571..056699224da7 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
> > 9569b21adb88..73c134459361 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -328,6 +328,32 @@ bool hci_enhanced_setup_sync(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_trasnport_unit_size = 1;
> > +		cp.out_trasnport_unit_size = 1;
> 
> so s/trasnport/transport/
> 
> Please spellcheck your structs.

Ack.

> 
> > +		break;
> > +
> > 	case CODING_FORMAT_TRANSPARENT:
> > 		if (!find_next_esco_param(conn, esco_param_msbc,
> > 					  ARRAY_SIZE(esco_param_msbc)))
> > @@ -383,7 +409,6 @@ bool hci_enhanced_setup_sync(struct hci_conn
> *conn, __u16 handle)
> > 		cp.in_trasnport_unit_size = 16;
> > 		cp.out_trasnport_unit_size = 16;
> > 		break;
> > -
> 
> We can not have these random hunks in patches. You need to review your
> final set before sending it out.

Ack.

> 
> Regards
> 
> Marcel

Thanks,
Kiran



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

end of thread, other threads:[~2021-06-23  3:24 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
2021-06-08 12:24 ` [PATCH v9 02/10] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
2021-06-08 12:24 ` [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path Kiran K
2021-06-15 19:26   ` Marcel Holtmann
2021-06-16  2:56     ` K, Kiran
2021-06-16  5:17       ` Marcel Holtmann
2021-06-08 12:24 ` [PATCH v9 04/10] Bluetooth: btintel: set get_data_path callback Kiran K
2021-06-15 19:30   ` Marcel Holtmann
2021-06-16  3:02     ` K, Kiran
2021-06-08 12:24 ` [PATCH v9 05/10] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket Kiran K
2021-06-15 19:37   ` Marcel Holtmann
2021-06-08 12:24 ` [PATCH v9 06/10] Bluetooth: Add a callback function to set data path Kiran K
2021-06-15 19:37   ` Marcel Holtmann
2021-06-08 12:24 ` [PATCH v9 07/10] Bluetooth: btintel: define callback " Kiran K
2021-06-15 19:39   ` Marcel Holtmann
2021-06-16  3:10     ` K, Kiran
2021-06-16  5:18       ` Marcel Holtmann
2021-06-17  7:53         ` K, Kiran
2021-06-17 10:19           ` Marcel Holtmann
2021-06-23  3:21             ` K, Kiran
2021-06-08 12:24 ` [PATCH v9 08/10] Bluetooth: Add BT_CODEC option for setsockopt over SCO Kiran K
2021-06-08 12:24 ` [PATCH v9 09/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command Kiran K
2021-06-08 12:24 ` [PATCH v9 10/10] Bluetooth: Add support for msbc coding format Kiran K
2021-06-15 19:43   ` Marcel Holtmann
2021-06-23  3:24     ` K, Kiran
2021-06-08 13:39 ` [v9,01/10] Bluetooth: enumerate local supported codec and cache details bluez.test.bot
2021-06-08 18:49 ` [PATCH v9 01/10] " kernel test robot
2021-06-08 18:49   ` kernel test robot
2021-06-15 19:25 ` Marcel Holtmann
2021-06-16  2:51   ` K, Kiran
2021-06-16  5:15     ` Marcel Holtmann

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