2021-06-08 12:23:00

by K, Kiran

[permalink] [raw]
Subject: [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details

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

Signed-off-by: Kiran K <[email protected]>
Signed-off-by: Chethan T N <[email protected]>
Signed-off-by: Srivatsa Ravishankar <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
* 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


2021-06-08 12:23:00

by K, Kiran

[permalink] [raw]
Subject: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path

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

Signed-off-by: Kiran K <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
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 <[email protected]>");
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

2021-06-08 12:23:13

by K, Kiran

[permalink] [raw]
Subject: [PATCH v9 08/10] Bluetooth: Add BT_CODEC option for setsockopt over SCO

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 <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
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

2021-06-08 12:24:51

by K, Kiran

[permalink] [raw]
Subject: [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path

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 <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
* 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

2021-06-08 12:25:01

by K, Kiran

[permalink] [raw]
Subject: [PATCH v9 06/10] Bluetooth: Add a callback function to set data path

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 <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
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

2021-06-08 12:25:01

by K, Kiran

[permalink] [raw]
Subject: [PATCH v9 05/10] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket

Add BT_CODEC option for getsockopt systemcall over SCO socket
to expose the codecs supported by controller

Signed-off-by: Kiran K <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
* 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

2021-06-08 12:25:01

by K, Kiran

[permalink] [raw]
Subject: [PATCH v9 09/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command

< 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 <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
* 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

2021-06-08 13:45:42

by bluez.test.bot

[permalink] [raw]
Subject: RE: [v9,01/10] Bluetooth: enumerate local supported codec and cache details

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


Attachments:
l2cap-tester.log (50.32 kB)
bnep-tester.log (3.81 kB)
mgmt-tester.log (598.61 kB)
rfcomm-tester.log (14.41 kB)
sco-tester.log (9.68 kB)
smp-tester.log (11.54 kB)
userchan-tester.log (6.33 kB)
Download all attachments

2021-06-09 00:28:07

by K, Kiran

[permalink] [raw]
Subject: [PATCH v9 10/10] Bluetooth: Add support for msbc coding format

In Enhanced_Setup_Synchronous_Command, add support for msbc
coding format

Signed-off-by: Kiran K <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
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

2021-06-09 00:28:07

by K, Kiran

[permalink] [raw]
Subject: [PATCH v9 02/10] Bluetooth: Add support for Read Local Supported Codecs V2

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 <[email protected]>
Signed-off-by: Chethan T N <[email protected]>
Signed-off-by: Srivatsa Ravishankar <[email protected]>
---
* 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

2021-06-09 07:48:05

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details

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 <[email protected]>


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/[email protected]


Attachments:
(No filename) (4.04 kB)
.config.gz (23.45 kB)
Download all attachments

2021-06-15 19:25:56

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details

Hi Kiran,

> Move reading of supported local codecs into a separate init function,
> query codecs capabilities and cache the data
>
> Signed-off-by: Kiran K <[email protected]>
> Signed-off-by: Chethan T N <[email protected]>
> Signed-off-by: Srivatsa Ravishankar <[email protected]>
> Reported-by: kernel test robot <[email protected]>

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

Regards

Marcel

2021-06-15 19:27:29

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path

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 <[email protected]>
> Reviewed-by: Chethan T N <[email protected]>
> Reviewed-by: Srivatsa Ravishankar <[email protected]>
> ---
> * 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

2021-06-15 19:39:23

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v9 06/10] Bluetooth: Add a callback function to set data path

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 <[email protected]>
> Reviewed-by: Chethan T N <chethan.tumk[email protected]>
> Reviewed-by: Srivatsa Ravishankar <[email protected]>
> ---
> 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

2021-06-15 19:40:21

by Marcel Holtmann

[permalink] [raw]
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 <[email protected]>
> Reviewed-by: Chethan T N <[email protected]>
> Reviewed-by: Srivatsa Ravishankar <[email protected]>
> ---
> 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 <[email protected]>");
> 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

2021-06-15 19:41:05

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v9 05/10] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket

Hi Kiran,

> Add BT_CODEC option for getsockopt systemcall over SCO socket
> to expose the codecs supported by controller
>
> Signed-off-by: Kiran K <[email protected]>
> Reviewed-by: Chethan T N <[email protected]>
> Reviewed-by: Srivatsa Ravishankar <[email protected]>
> ---
> * 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

2021-06-15 19:44:33

by Marcel Holtmann

[permalink] [raw]
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 <[email protected]>
> Reviewed-by: Chethan T N <[email protected]>
> Reviewed-by: Srivatsa Ravishankar <[email protected]>
> ---
> 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

2021-06-16 02:54:33

by K, Kiran

[permalink] [raw]
Subject: RE: [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details

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 <[email protected]>
> > Signed-off-by: Chethan T N <[email protected]>
> > Signed-off-by: Srivatsa Ravishankar <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
>
> 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


2021-06-16 02:57:42

by K, Kiran

[permalink] [raw]
Subject: RE: [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path

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 <[email protected]>
> > Reviewed-by: Chethan T N <[email protected]>
> > Reviewed-by: Srivatsa Ravishankar <[email protected]>
> > ---
> > * 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/[email protected]/

>
> Regards
>
> Marcel

Regards,
Kiran


2021-06-16 03:12:27

by K, Kiran

[permalink] [raw]
Subject: RE: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path

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 <[email protected]>
> > Reviewed-by: Chethan T N <[email protected]>
> > Reviewed-by: Srivatsa Ravishankar <[email protected]>
> > ---
> > 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 <[email protected]>");
> > 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

2021-06-16 05:19:25

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details

Hi Kiran,

>>> Move reading of supported local codecs into a separate init function,
>>> query codecs capabilities and cache the data
>>>
>>> Signed-off-by: Kiran K <[email protected]>
>>> Signed-off-by: Chethan T N <[email protected]>
>>> Signed-off-by: Srivatsa Ravishankar <[email protected]>
>>> Reported-by: kernel test robot <[email protected]>
>>
>> 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

2021-06-16 05:19:25

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path

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 <[email protected]>
>>> Reviewed-by: Chethan T N <[email protected]>
>>> Reviewed-by: Srivatsa Ravishankar <[email protected]>
>>> ---
>>> * 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/[email protected]/
>

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

2021-06-16 05:19:54

by Marcel Holtmann

[permalink] [raw]
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 <[email protected]>
>>> Reviewed-by: Chethan T N <[email protected]>
>>> Reviewed-by: Srivatsa Ravishankar <[email protected]>
>>> ---
>>> 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 <[email protected]>");
>>> 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

2021-06-17 07:55:38

by K, Kiran

[permalink] [raw]
Subject: RE: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <[email protected]>
> Sent: Wednesday, June 16, 2021 10:48 AM
> To: K, Kiran <[email protected]>
> Cc: [email protected]
> 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 <[email protected]>
> >>> Reviewed-by: Chethan T N <[email protected]>
> >>> Reviewed-by: Srivatsa Ravishankar <[email protected]>
> >>> ---
> >>> 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 <[email protected]>");
> >>> 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

2021-06-17 12:35:47

by Marcel Holtmann

[permalink] [raw]
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 <[email protected]>
>>>>> Reviewed-by: Chethan T N <[email protected]>
>>>>> Reviewed-by: Srivatsa Ravishankar <[email protected]>
>>>>> ---
>>>>> 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 <[email protected]>");
>>>>> 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

2021-06-23 03:23:19

by K, Kiran

[permalink] [raw]
Subject: RE: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data path

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <[email protected]>
> Sent: Thursday, June 17, 2021 3:50 PM
> To: K, Kiran <[email protected]>
> Cc: [email protected]
> 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 <[email protected]>
> >>>>> Reviewed-by: Chethan T N <[email protected]>
> >>>>> Reviewed-by: Srivatsa Ravishankar <[email protected]>
> >>>>> ---
> >>>>> 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 <[email protected]>");
> >>>>> 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


2021-06-23 03:25:26

by K, Kiran

[permalink] [raw]
Subject: RE: [PATCH v9 10/10] Bluetooth: Add support for msbc coding format

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <[email protected]>
> Sent: Wednesday, June 16, 2021 1:14 AM
> To: K, Kiran <[email protected]>
> Cc: [email protected]
> 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 <[email protected]>
> > Reviewed-by: Chethan T N <[email protected]>
> > Reviewed-by: Srivatsa Ravishankar <[email protected]>
> > ---
> > 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