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]>
---
Notes:
* changes in v13:
- No changes
* changes in v12:
- refactor funcions related to reading codec capabilities
* changes in v11:
- Remove Kconfig related changes
- Address minor review comments
- Move codec related functions to new file hci_codec.c
* changes in v10:
- define Kconfig to control offload feature at build time
- fix review comments
* changes in v9:
- use shortname vnd instead of ven
* changes in v8:
- add comments
- split __u8 codec_id[5] into {__u8 id; __le16 cid, vid }
- address review comment related codec caps structure
* changes in v7:
- keep codec enumeration call in hci_init instead of having a separate
function
- Remove unused bitmasks defined for LE transports
* changes in v6:
- fix compiler warning reported for ARCH=arc
* changes in v5:
- fix review comments
- move code used to read standard/vendor codecs caps into single function
* changes in v4:
- convert reading of codecs and codecs caps calls from async to sync
* changes in v3
- move codec enumeration into a new init function
* changes in v2
- add skb length check before accessing data
include/net/bluetooth/hci.h | 41 ++++++++
include/net/bluetooth/hci_core.h | 17 +++
net/bluetooth/Makefile | 2 +-
net/bluetooth/hci_codec.c | 172 +++++++++++++++++++++++++++++++
net/bluetooth/hci_codec.h | 6 ++
net/bluetooth/hci_core.c | 11 +-
6 files changed, 244 insertions(+), 5 deletions(-)
create mode 100644 net/bluetooth/hci_codec.c
create mode 100644 net/bluetooth/hci_codec.h
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index bb6b7398f490..13a93da6b76c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1308,6 +1308,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 {
@@ -1316,6 +1338,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 24ad6ebd00e2..585cbe7ff67d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -131,6 +131,17 @@ struct bdaddr_list {
u8 bdaddr_type;
};
+struct codec_list {
+ struct list_head list;
+ u8 id;
+ __u16 cid;
+ __u16 vid;
+ u8 transport;
+ u8 num_caps;
+ u32 len;
+ struct hci_codec_caps caps[];
+};
+
struct bdaddr_list_with_irk {
struct list_head list;
bdaddr_t bdaddr;
@@ -536,6 +547,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;
@@ -1870,4 +1882,9 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
#define SCO_AIRMODE_CVSD 0x0000
#define SCO_AIRMODE_TRANSP 0x0003
+#define LOCAL_CODEC_ACL_MASK BIT(0)
+#define LOCAL_CODEC_SCO_MASK BIT(1)
+
+#define TRANSPORT_TYPE_MAX 0x04
+
#endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
index cc0995301f93..f3e439d98b85 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -14,7 +14,7 @@ bluetooth_6lowpan-y := 6lowpan.o
bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \
hci_sock.o hci_sysfs.o l2cap_core.o l2cap_sock.o smp.o lib.o \
- ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o
+ ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o hci_codec.o
bluetooth-$(CONFIG_BT_BREDR) += sco.o
bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
diff --git a/net/bluetooth/hci_codec.c b/net/bluetooth/hci_codec.c
new file mode 100644
index 000000000000..f86ca6ba5814
--- /dev/null
+++ b/net/bluetooth/hci_codec.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* Copyright (C) 2021 Intel Corporation */
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+#include "hci_codec.h"
+
+static int hci_codec_list_add(struct list_head *list,
+ struct hci_op_read_local_codec_caps *sent,
+ struct hci_rp_read_local_codec_caps *rp,
+ void *caps,
+ __u32 len)
+{
+ struct codec_list *entry;
+
+ entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL);
+ if (!entry)
+ return -ENOMEM;
+
+ entry->id = sent->id;
+ if (sent->id == 0xFF) {
+ entry->cid = __le16_to_cpu(sent->cid);
+ entry->vid = __le16_to_cpu(sent->vid);
+ }
+ entry->transport = sent->transport;
+ entry->len = len;
+ entry->num_caps = rp->num_caps;
+ if (rp->num_caps)
+ memcpy(entry->caps, caps, len);
+ list_add(&entry->list, list);
+
+ return 0;
+}
+
+void hci_codec_list_clear(struct list_head *codec_list)
+{
+ struct codec_list *c, *n;
+
+ list_for_each_entry_safe(c, n, codec_list, list) {
+ list_del(&c->list);
+ kfree(c);
+ }
+}
+
+static void hci_read_codec_capabilities(struct hci_dev *hdev, __u8 transport,
+ struct hci_op_read_local_codec_caps
+ *cmd)
+{
+ __u8 i;
+
+ 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);
+ }
+ }
+}
+
+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;
+ struct hci_op_read_local_codec_caps caps;
+ __u8 i;
+
+ 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 */
+ memset(&caps, 0, sizeof(caps));
+ for (i = 0; i < std_codecs->num; i++) {
+ caps.id = std_codecs->codec[i];
+ caps.direction = 0x00;
+ hci_read_codec_capabilities(hdev, LOCAL_CODEC_ACL_MASK, &caps);
+ }
+
+ 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 */
+ for (i = 0; i < vnd_codecs->num; i++) {
+ caps.id = 0xFF;
+ caps.cid = vnd_codecs->codec[i].cid;
+ caps.vid = vnd_codecs->codec[i].vid;
+ caps.direction = 0x00;
+ hci_read_codec_capabilities(hdev, LOCAL_CODEC_ACL_MASK, &caps);
+ }
+
+error:
+ kfree_skb(skb);
+}
diff --git a/net/bluetooth/hci_codec.h b/net/bluetooth/hci_codec.h
new file mode 100644
index 000000000000..efb0df634ac6
--- /dev/null
+++ b/net/bluetooth/hci_codec.h
@@ -0,0 +1,6 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/* Copyright (C) 2014 Intel Corporation */
+
+void hci_read_supported_codecs(struct hci_dev *hdev);
+void hci_codec_list_clear(struct list_head *codec_list);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f3a18d16b81f..a80d813ef743 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -45,6 +45,7 @@
#include "leds.h"
#include "msft.h"
#include "aosp.h"
+#include "hci_codec.h"
static void hci_rx_work(struct work_struct *work);
static void hci_cmd_work(struct work_struct *work);
@@ -838,10 +839,6 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
if (hdev->commands[22] & 0x04)
hci_set_event_mask_page_2(req);
- /* Read local codec list if the HCI command is supported */
- if (hdev->commands[29] & 0x20)
- hci_req_add(req, HCI_OP_READ_LOCAL_CODECS, 0, NULL);
-
/* Read local pairing options if the HCI command is supported */
if (hdev->commands[41] & 0x08)
hci_req_add(req, HCI_OP_READ_LOCAL_PAIRING_OPTS, 0, NULL);
@@ -937,6 +934,10 @@ static int __hci_init(struct hci_dev *hdev)
if (err < 0)
return err;
+ /* Read local codec list if the HCI command is supported */
+ if (hdev->commands[29] & 0x20)
+ hci_read_supported_codecs(hdev);
+
/* This function is only called when the controller is actually in
* configured state. When the controller is marked as unconfigured,
* this initialization procedure is not run.
@@ -1848,6 +1849,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);
@@ -3848,6 +3850,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
INIT_LIST_HEAD(&hdev->adv_instances);
INIT_LIST_HEAD(&hdev->blocked_keys);
+ INIT_LIST_HEAD(&hdev->local_codecs);
INIT_WORK(&hdev->rx_work, hci_rx_work);
INIT_WORK(&hdev->cmd_work, hci_cmd_work);
INIT_WORK(&hdev->tx_work, hci_tx_work);
--
2.17.1
From: Chethan T N <[email protected]>
Currently usb tranport is not allowed to suspend when SCO over
HCI tranport is active.
This patch shall enable the usb tranport to suspend when SCO
link use non-HCI transport
Signed-off-by: Chethan T N <[email protected]>
Signed-off-by: Kiran K <[email protected]>
---
Notes:
changes in v13:
- suspend usb in HFP offload use case
drivers/bluetooth/btintel.c | 2 +-
include/net/bluetooth/bluetooth.h | 4 ++++
net/bluetooth/hci_event.c | 20 +++++++++++---------
net/bluetooth/sco.c | 2 +-
4 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 6091b691ddc2..2d64e289cf6e 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -2215,7 +2215,7 @@ static int btintel_get_codec_config_data(struct hci_dev *hdev,
static int btintel_get_data_path_id(struct hci_dev *hdev, __u8 *data_path_id)
{
/* Intel uses 1 as data path id for all the usecases */
- *data_path_id = 1;
+ *data_path_id = BT_SCO_PCM_PATH;
return 0;
}
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index c1fa90fb7502..9e2745863b33 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -177,6 +177,10 @@ struct bt_codecs {
#define CODING_FORMAT_TRANSPARENT 0x03
#define CODING_FORMAT_MSBC 0x05
+/* Audio data transport path used for SCO */
+#define BT_SCO_HCI_PATH 0x00
+#define BT_SCO_PCM_PATH 0x01
+
__printf(1, 2)
void bt_info(const char *fmt, ...);
__printf(1, 2)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index b48e24629fa4..7ff11cba89cf 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4516,15 +4516,17 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
bt_dev_dbg(hdev, "SCO connected with air mode: %02x", ev->air_mode);
- switch (ev->air_mode) {
- case 0x02:
- if (hdev->notify)
- hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
- break;
- case 0x03:
- if (hdev->notify)
- hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
- break;
+ if (conn->codec.data_path == BT_SCO_HCI_PATH) {
+ switch (ev->air_mode) {
+ case 0x02:
+ if (hdev->notify)
+ hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
+ break;
+ case 0x03:
+ if (hdev->notify)
+ hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
+ break;
+ }
}
hci_connect_cfm(conn, ev->status);
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 004bce2b5eca..f35c12ca6aa5 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -506,7 +506,7 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock,
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;
+ sco_pi(sk)->codec.data_path = BT_SCO_HCI_PATH;
bt_sock_link(&sco_sk_list, sk);
return sk;
--
2.17.1
< 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]>
---
Notes:
* changes in v13:
- No changes
* changes in v12:
- hci_dev_put needs to be called when done with hci_get_route
* changes in v11:
- Remove changes related to Kconfig
- s/use_enhanced_sco_conn/enhanced_sco_capable/g
- Minor review comments
* changes in v10:
- Fix typos and remove unwanted chunks
* changes in v9:
- Fix review comments, use bt_dev_dbg instead of BT_DBG
include/net/bluetooth/bluetooth.h | 3 +-
include/net/bluetooth/hci.h | 34 ++++++++++
include/net/bluetooth/hci_core.h | 6 +-
net/bluetooth/hci_conn.c | 105 +++++++++++++++++++++++++++++-
net/bluetooth/hci_event.c | 39 +++++++++++
net/bluetooth/sco.c | 12 +++-
6 files changed, 193 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 1a48b6732eef..f2df8bb108d9 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -173,7 +173,8 @@ struct bt_codecs {
struct bt_codec codecs[];
} __packed;
-#define CODING_FORMAT_CVSD 0x02
+#define CODING_FORMAT_CVSD 0x02
+#define CODING_FORMAT_TRANSPARENT 0x03
__printf(1, 2)
void bt_info(const char *fmt, ...);
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 8f8664bec6f2..3426ee4f0fa5 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -873,6 +873,40 @@ struct hci_cp_logical_link_cancel {
__u8 flow_spec_id;
} __packed;
+#define HCI_OP_ENHANCED_SETUP_SYNC_CONN 0x043d
+struct hci_coding_format {
+ __u8 id;
+ __le16 cid;
+ __le16 vid;
+} __packed;
+
+struct hci_cp_enhanced_setup_sync_conn {
+ __le16 handle;
+ __le32 tx_bandwidth;
+ __le32 rx_bandwidth;
+ struct hci_coding_format tx_coding_format;
+ struct hci_coding_format rx_coding_format;
+ __le16 tx_codec_frame_size;
+ __le16 rx_codec_frame_size;
+ __le32 in_bandwidth;
+ __le32 out_bandwidth;
+ struct hci_coding_format in_coding_format;
+ struct hci_coding_format out_coding_format;
+ __le16 in_coded_data_size;
+ __le16 out_coded_data_size;
+ __u8 in_pcm_data_format;
+ __u8 out_pcm_data_format;
+ __u8 in_pcm_sample_payload_msb_pos;
+ __u8 out_pcm_sample_payload_msb_pos;
+ __u8 in_data_path;
+ __u8 out_data_path;
+ __u8 in_transport_unit_size;
+ __u8 out_transport_unit_size;
+ __le16 max_latency;
+ __le16 pkt_type;
+ __u8 retrans_effort;
+} __packed;
+
struct hci_rp_logical_link_cancel {
__u8 status;
__u8 phy_handle;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 655e2a119e7f..fdfdd3bdde16 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -713,6 +713,7 @@ struct hci_conn {
struct amp_mgr *amp_mgr;
struct hci_conn *link;
+ struct bt_codec codec;
void (*connect_cfm_cb) (struct hci_conn *conn, u8 status);
void (*security_cfm_cb) (struct hci_conn *conn, u8 status);
@@ -1121,7 +1122,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,
@@ -1458,6 +1459,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
/* Use LL Privacy based address resolution if supported */
#define use_ll_privacy(dev) ((dev)->le_features[0] & HCI_LE_LL_PRIVACY)
+/* Use enhanced synchronous connection if command is supported */
+#define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
+
/* Use ext scanning if set ext scan param and ext scan enable is supported */
#define use_ext_scan(dev) (((dev)->commands[37] & 0x20) && \
((dev)->commands[37] & 0x40))
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 3ab60b75d865..b9c61fc9ff89 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -307,13 +307,103 @@ static bool find_next_esco_param(struct hci_conn *conn,
return conn->attempt <= size;
}
-bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
+static bool hci_enhanced_setup_sync_conn(struct hci_conn *conn, __u16 handle)
+{
+ struct hci_dev *hdev = conn->hdev;
+ struct hci_cp_enhanced_setup_sync_conn cp;
+ const struct sco_param *param;
+
+ bt_dev_dbg(hdev, "hcon %p", conn);
+
+ conn->state = BT_CONNECT;
+ conn->out = true;
+
+ conn->attempt++;
+
+ memset(&cp, 0x00, sizeof(cp));
+
+ cp.handle = cpu_to_le16(handle);
+
+ cp.tx_bandwidth = cpu_to_le32(0x00001f40);
+ cp.rx_bandwidth = cpu_to_le32(0x00001f40);
+
+ switch (conn->codec.id) {
+ case CODING_FORMAT_TRANSPARENT:
+ if (!find_next_esco_param(conn, esco_param_msbc,
+ ARRAY_SIZE(esco_param_msbc)))
+ return false;
+ param = &esco_param_msbc[conn->attempt - 1];
+ cp.tx_coding_format.id = 0x03;
+ cp.rx_coding_format.id = 0x03;
+ cp.tx_codec_frame_size = __cpu_to_le16(60);
+ cp.rx_codec_frame_size = __cpu_to_le16(60);
+ cp.in_bandwidth = __cpu_to_le32(0x1f40);
+ cp.out_bandwidth = __cpu_to_le32(0x1f40);
+ cp.in_coding_format.id = 0x03;
+ cp.out_coding_format.id = 0x03;
+ cp.in_coded_data_size = __cpu_to_le16(16);
+ cp.out_coded_data_size = __cpu_to_le16(16);
+ cp.in_pcm_data_format = 2;
+ cp.out_pcm_data_format = 2;
+ cp.in_pcm_sample_payload_msb_pos = 0;
+ cp.out_pcm_sample_payload_msb_pos = 0;
+ cp.in_data_path = conn->codec.data_path;
+ cp.out_data_path = conn->codec.data_path;
+ cp.in_transport_unit_size = 1;
+ cp.out_transport_unit_size = 1;
+ break;
+
+ case CODING_FORMAT_CVSD:
+ if (lmp_esco_capable(conn->link)) {
+ if (!find_next_esco_param(conn, esco_param_cvsd,
+ ARRAY_SIZE(esco_param_cvsd)))
+ return false;
+ param = &esco_param_cvsd[conn->attempt - 1];
+ } else {
+ if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
+ return false;
+ param = &sco_param_cvsd[conn->attempt - 1];
+ }
+ cp.tx_coding_format.id = 2;
+ cp.rx_coding_format.id = 2;
+ cp.tx_codec_frame_size = __cpu_to_le16(60);
+ cp.rx_codec_frame_size = __cpu_to_le16(60);
+ cp.in_bandwidth = __cpu_to_le32(16000);
+ cp.out_bandwidth = __cpu_to_le32(16000);
+ cp.in_coding_format.id = 4;
+ cp.out_coding_format.id = 4;
+ cp.in_coded_data_size = __cpu_to_le16(16);
+ cp.out_coded_data_size = __cpu_to_le16(16);
+ cp.in_pcm_data_format = 2;
+ cp.out_pcm_data_format = 2;
+ cp.in_pcm_sample_payload_msb_pos = 0;
+ cp.out_pcm_sample_payload_msb_pos = 0;
+ cp.in_data_path = conn->codec.data_path;
+ cp.out_data_path = conn->codec.data_path;
+ cp.in_transport_unit_size = 16;
+ cp.out_transport_unit_size = 16;
+ break;
+ default:
+ return false;
+ }
+
+ cp.retrans_effort = param->retrans_effort;
+ cp.pkt_type = __cpu_to_le16(param->pkt_type);
+ cp.max_latency = __cpu_to_le16(param->max_latency);
+
+ if (hci_send_cmd(hdev, HCI_OP_ENHANCED_SETUP_SYNC_CONN, sizeof(cp), &cp) < 0)
+ return false;
+
+ return true;
+}
+
+static bool hci_setup_sync_conn(struct hci_conn *conn, __u16 handle)
{
struct hci_dev *hdev = conn->hdev;
struct hci_cp_setup_sync_conn cp;
const struct sco_param *param;
- BT_DBG("hcon %p", conn);
+ bt_dev_dbg(hdev, "hcon %p", conn);
conn->state = BT_CONNECT;
conn->out = true;
@@ -359,6 +449,14 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
return true;
}
+bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
+{
+ if (enhanced_sco_capable(conn->hdev))
+ return hci_enhanced_setup_sync_conn(conn, handle);
+
+ return hci_setup_sync_conn(conn, handle);
+}
+
u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
u16 to_multiplier)
{
@@ -1324,7 +1422,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;
@@ -1349,6 +1447,7 @@ struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
hci_conn_hold(sco);
sco->setting = setting;
+ sco->codec = *codec;
if (acl->state == BT_CONNECTED &&
(sco->state == BT_OPEN || sco->state == BT_CLOSED)) {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index b1062f5cf488..b48e24629fa4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2278,6 +2278,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;
@@ -3764,6 +3799,10 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb,
hci_cs_setup_sync_conn(hdev, ev->status);
break;
+ case HCI_OP_ENHANCED_SETUP_SYNC_CONN:
+ hci_cs_enhanced_setup_sync_conn(hdev, ev->status);
+ break;
+
case HCI_OP_SNIFF_MODE:
hci_cs_sniff_mode(hdev, ev->status);
break;
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 727b766488db..004bce2b5eca 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -253,7 +253,7 @@ static int sco_connect(struct hci_dev *hdev, struct sock *sk)
return -EOPNOTSUPP;
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))
return PTR_ERR(hcon);
@@ -889,6 +889,16 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
}
sco_pi(sk)->setting = voice.setting;
+ hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src,
+ BDADDR_BREDR);
+ if (!hdev) {
+ err = -EBADFD;
+ break;
+ }
+ if (enhanced_sco_capable(hdev) &&
+ voice.setting == BT_VOICE_TRANSPARENT)
+ sco_pi(sk)->codec.id = CODING_FORMAT_TRANSPARENT;
+ hci_dev_put(hdev);
break;
case BT_PKT_STATUS:
--
2.17.1
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=539755
---Test result---
Test Summary:
CheckPatch FAIL 1.51 seconds
GitLint FAIL 0.73 seconds
BuildKernel PASS 573.84 seconds
TestRunner: Setup PASS 385.35 seconds
TestRunner: l2cap-tester PASS 2.91 seconds
TestRunner: bnep-tester PASS 1.99 seconds
TestRunner: mgmt-tester FAIL 33.36 seconds
TestRunner: rfcomm-tester PASS 2.25 seconds
TestRunner: sco-tester PASS 2.20 seconds
TestRunner: smp-tester PASS 2.21 seconds
TestRunner: userchan-tester PASS 1.98 seconds
Details
##############################
Test: CheckPatch - FAIL - 1.51 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: Enumerate local supported codec and cache details
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#126:
new file mode 100644
total: 0 errors, 1 warnings, 0 checks, 313 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] Bluetooth: Enumerate local supported codec and cache details" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL - 0.73 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 - 573.84 seconds
Build Kernel with minimal configuration supports Bluetooth
##############################
Test: TestRunner: Setup - PASS - 385.35 seconds
Setup environment for running Test Runner
##############################
Test: TestRunner: l2cap-tester - PASS - 2.91 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: bnep-tester - PASS - 1.99 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: mgmt-tester - FAIL - 33.36 seconds
Run test-runner with mgmt-tester
Total: 452, Passed: 451 (99.8%), Failed: 1, Not Run: 0
Failed Test Cases
Read Exp Feature - Success Failed 0.017 seconds
##############################
Test: TestRunner: rfcomm-tester - PASS - 2.25 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: sco-tester - PASS - 2.20 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: smp-tester - PASS - 2.21 seconds
Run test-runner with smp-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: userchan-tester - PASS - 1.98 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0
---
Regards,
Linux Bluetooth
Hi Kiran,
On Tue, Aug 31, 2021 at 4:54 AM Kiran K <[email protected]> wrote:
>
> From: Chethan T N <[email protected]>
>
> Currently usb tranport is not allowed to suspend when SCO over
> HCI tranport is active.
>
> This patch shall enable the usb tranport to suspend when SCO
> link use non-HCI transport
>
> Signed-off-by: Chethan T N <[email protected]>
> Signed-off-by: Kiran K <[email protected]>
> ---
>
> Notes:
> changes in v13:
> - suspend usb in HFP offload use case
>
> drivers/bluetooth/btintel.c | 2 +-
> include/net/bluetooth/bluetooth.h | 4 ++++
> net/bluetooth/hci_event.c | 20 +++++++++++---------
> net/bluetooth/sco.c | 2 +-
> 4 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 6091b691ddc2..2d64e289cf6e 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -2215,7 +2215,7 @@ static int btintel_get_codec_config_data(struct hci_dev *hdev,
> static int btintel_get_data_path_id(struct hci_dev *hdev, __u8 *data_path_id)
> {
> /* Intel uses 1 as data path id for all the usecases */
> - *data_path_id = 1;
> + *data_path_id = BT_SCO_PCM_PATH;
> return 0;
> }
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index c1fa90fb7502..9e2745863b33 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -177,6 +177,10 @@ struct bt_codecs {
> #define CODING_FORMAT_TRANSPARENT 0x03
> #define CODING_FORMAT_MSBC 0x05
>
> +/* Audio data transport path used for SCO */
> +#define BT_SCO_HCI_PATH 0x00
> +#define BT_SCO_PCM_PATH 0x01
If this is in fact vendor specific perhaps you should be declared in
btintel.h not here.
> +
> __printf(1, 2)
> void bt_info(const char *fmt, ...);
> __printf(1, 2)
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index b48e24629fa4..7ff11cba89cf 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4516,15 +4516,17 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
>
> bt_dev_dbg(hdev, "SCO connected with air mode: %02x", ev->air_mode);
>
> - switch (ev->air_mode) {
> - case 0x02:
> - if (hdev->notify)
> - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> - break;
> - case 0x03:
> - if (hdev->notify)
> - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> - break;
> + if (conn->codec.data_path == BT_SCO_HCI_PATH) {
> + switch (ev->air_mode) {
> + case 0x02:
> + if (hdev->notify)
> + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> + break;
> + case 0x03:
> + if (hdev->notify)
> + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> + break;
> + }
Hmm I think we might need to notify the driver to enable PCM routing
so the likes of btusb can call
usb_disable_endpoint/usb_enable_endpoint for example since in theory
userspace may choose to switch from software to hardware offload and
vice-versa, note without calling usb_disable_endpoint there might not
be much power saving after all since the endpoint will remain active
or do we actually have a good explanation why it shall not be called
when using PCM routing? Note that usb_set_interface will call
usb_enable_interface that will then call usb_enable_endpoint so
perhaps we need to call usb_disable_interface, either way we can't
assume the platform will be restricted to only use one or the other.
> }
>
> hci_connect_cfm(conn, ev->status);
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 004bce2b5eca..f35c12ca6aa5 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -506,7 +506,7 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock,
> 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;
> + sco_pi(sk)->codec.data_path = BT_SCO_HCI_PATH;
>
> bt_sock_link(&sco_sk_list, sk);
> return sk;
> --
> 2.17.1
>
--
Luiz Augusto von Dentz
Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <[email protected]>
> Sent: Thursday, September 2, 2021 5:24 AM
> To: K, Kiran <[email protected]>
> Cc: [email protected]; Tumkur Narayan, Chethan
> <[email protected]>
> Subject: Re: [PATCH v13 12/12] Bluetooth: Allow usb to auto-suspend when SCO
> use non-HCI transport
>
> Hi Kiran,
>
> On Tue, Aug 31, 2021 at 4:54 AM Kiran K <[email protected]> wrote:
> >
> > From: Chethan T N <[email protected]>
> >
> > Currently usb tranport is not allowed to suspend when SCO over HCI
> > tranport is active.
> >
> > This patch shall enable the usb tranport to suspend when SCO link use
> > non-HCI transport
> >
> > Signed-off-by: Chethan T N <[email protected]>
> > Signed-off-by: Kiran K <[email protected]>
> > ---
> >
> > Notes:
> > changes in v13:
> > - suspend usb in HFP offload use case
> >
> > drivers/bluetooth/btintel.c | 2 +-
> > include/net/bluetooth/bluetooth.h | 4 ++++
> > net/bluetooth/hci_event.c | 20 +++++++++++---------
> > net/bluetooth/sco.c | 2 +-
> > 4 files changed, 17 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 6091b691ddc2..2d64e289cf6e 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -2215,7 +2215,7 @@ static int btintel_get_codec_config_data(struct
> > hci_dev *hdev, static int btintel_get_data_path_id(struct hci_dev
> > *hdev, __u8 *data_path_id) {
> > /* Intel uses 1 as data path id for all the usecases */
> > - *data_path_id = 1;
> > + *data_path_id = BT_SCO_PCM_PATH;
> > return 0;
> > }
> >
> > diff --git a/include/net/bluetooth/bluetooth.h
> > b/include/net/bluetooth/bluetooth.h
> > index c1fa90fb7502..9e2745863b33 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -177,6 +177,10 @@ struct bt_codecs {
> > #define CODING_FORMAT_TRANSPARENT 0x03
> > #define CODING_FORMAT_MSBC 0x05
> >
> > +/* Audio data transport path used for SCO */ #define BT_SCO_HCI_PATH
> > +0x00 #define BT_SCO_PCM_PATH 0x01
>
> If this is in fact vendor specific perhaps you should be declared in btintel.h not
> here.
This is defined the Host Controller Interface assigned numbers, page no.3 "Transport Layer"- https://btprodspecificationrefs.blob.core.windows.net/assigned-numbers/Assigned%20Number%20Types/Host%20Controller%20Interface.pdf. So defined in bluetooth.h, let me know if you think otherwise.
>
> > +
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > __printf(1, 2)
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index b48e24629fa4..7ff11cba89cf 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -4516,15 +4516,17 @@ static void hci_sync_conn_complete_evt(struct
> > hci_dev *hdev,
> >
> > bt_dev_dbg(hdev, "SCO connected with air mode: %02x",
> > ev->air_mode);
> >
> > - switch (ev->air_mode) {
> > - case 0x02:
> > - if (hdev->notify)
> > - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> > - break;
> > - case 0x03:
> > - if (hdev->notify)
> > - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> > - break;
> > + if (conn->codec.data_path == BT_SCO_HCI_PATH) {
> > + switch (ev->air_mode) {
> > + case 0x02:
> > + if (hdev->notify)
> > + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> > + break;
> > + case 0x03:
> > + if (hdev->notify)
> > + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> > + break;
> > + }
>
> Hmm I think we might need to notify the driver to enable PCM routing so the
> likes of btusb can call usb_disable_endpoint/usb_enable_endpoint for example
> since in theory userspace may choose to switch from
> offload and vice-versa, note without calling usb_disable_endpoint there might
> not be much power saving after all since the endpoint will remain active or do
> we actually have a good explanation why it shall not be called when using PCM
> routing? Note that usb_set_interface will call usb_enable_interface that will
> then call usb_enable_endpoint so perhaps we need to call
> usb_disable_interface, either way we can't assume the platform will be
> restricted to only use one or the other.
Ack, Does it make sense to define and notify events "HCI_NOTIFY_DISABLE_SCO_USB_INTF ", "HCI_NOTIFY_ENABLE_SCO_USB_INTF " accordingly and handle this in btusb driver by disabling/enabling the ISOC endpoint respectively. That will serve the purpose or switch between software to hardware.
>
> > }
> >
> > hci_connect_cfm(conn, ev->status); diff --git
> > a/net/bluetooth/sco.c b/net/bluetooth/sco.c index
> > 004bce2b5eca..f35c12ca6aa5 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -506,7 +506,7 @@ static struct sock *sco_sock_alloc(struct net *net,
> struct socket *sock,
> > 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;
> > + sco_pi(sk)->codec.data_path = BT_SCO_HCI_PATH;
> >
> > bt_sock_link(&sco_sk_list, sk);
> > return sk;
> > --
> > 2.17.1
> >
>
>
> --
> Luiz Augusto von Dentz
Hi Luiz,
>> Currently usb tranport is not allowed to suspend when SCO over
>> HCI tranport is active.
>>
>> This patch shall enable the usb tranport to suspend when SCO
>> link use non-HCI transport
>>
>> Signed-off-by: Chethan T N <[email protected]>
>> Signed-off-by: Kiran K <[email protected]>
>> ---
>>
>> Notes:
>> changes in v13:
>> - suspend usb in HFP offload use case
>>
>> drivers/bluetooth/btintel.c | 2 +-
>> include/net/bluetooth/bluetooth.h | 4 ++++
>> net/bluetooth/hci_event.c | 20 +++++++++++---------
>> net/bluetooth/sco.c | 2 +-
>> 4 files changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
>> index 6091b691ddc2..2d64e289cf6e 100644
>> --- a/drivers/bluetooth/btintel.c
>> +++ b/drivers/bluetooth/btintel.c
>> @@ -2215,7 +2215,7 @@ static int btintel_get_codec_config_data(struct hci_dev *hdev,
>> static int btintel_get_data_path_id(struct hci_dev *hdev, __u8 *data_path_id)
>> {
>> /* Intel uses 1 as data path id for all the usecases */
>> - *data_path_id = 1;
>> + *data_path_id = BT_SCO_PCM_PATH;
>> return 0;
>> }
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index c1fa90fb7502..9e2745863b33 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -177,6 +177,10 @@ struct bt_codecs {
>> #define CODING_FORMAT_TRANSPARENT 0x03
>> #define CODING_FORMAT_MSBC 0x05
>>
>> +/* Audio data transport path used for SCO */
>> +#define BT_SCO_HCI_PATH 0x00
>> +#define BT_SCO_PCM_PATH 0x01
>
> If this is in fact vendor specific perhaps you should be declared in
> btintel.h not here.
>
>> +
>> __printf(1, 2)
>> void bt_info(const char *fmt, ...);
>> __printf(1, 2)
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index b48e24629fa4..7ff11cba89cf 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -4516,15 +4516,17 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
>>
>> bt_dev_dbg(hdev, "SCO connected with air mode: %02x", ev->air_mode);
>>
>> - switch (ev->air_mode) {
>> - case 0x02:
>> - if (hdev->notify)
>> - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
>> - break;
>> - case 0x03:
>> - if (hdev->notify)
>> - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
>> - break;
>> + if (conn->codec.data_path == BT_SCO_HCI_PATH) {
>> + switch (ev->air_mode) {
>> + case 0x02:
>> + if (hdev->notify)
>> + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
>> + break;
>> + case 0x03:
>> + if (hdev->notify)
>> + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
>> + break;
>> + }
>
> Hmm I think we might need to notify the driver to enable PCM routing
> so the likes of btusb can call
> usb_disable_endpoint/usb_enable_endpoint for example since in theory
> userspace may choose to switch from software to hardware offload and
> vice-versa, note without calling usb_disable_endpoint there might not
> be much power saving after all since the endpoint will remain active
> or do we actually have a good explanation why it shall not be called
> when using PCM routing? Note that usb_set_interface will call
> usb_enable_interface that will then call usb_enable_endpoint so
> perhaps we need to call usb_disable_interface, either way we can't
> assume the platform will be restricted to only use one or the other.
actually for the Intel hardware we shouldn’t do this at all. We should switch to vendor specific SCO over bulk endpoints and not claim the ISOC endpoints at all.
Regards
Marcel
Hi Marcel,
On Thu, Sep 2, 2021 at 2:37 AM Marcel Holtmann <[email protected]> wrote:
>
> Hi Luiz,
>
> >> Currently usb tranport is not allowed to suspend when SCO over
> >> HCI tranport is active.
> >>
> >> This patch shall enable the usb tranport to suspend when SCO
> >> link use non-HCI transport
> >>
> >> Signed-off-by: Chethan T N <[email protected]>
> >> Signed-off-by: Kiran K <[email protected]>
> >> ---
> >>
> >> Notes:
> >> changes in v13:
> >> - suspend usb in HFP offload use case
> >>
> >> drivers/bluetooth/btintel.c | 2 +-
> >> include/net/bluetooth/bluetooth.h | 4 ++++
> >> net/bluetooth/hci_event.c | 20 +++++++++++---------
> >> net/bluetooth/sco.c | 2 +-
> >> 4 files changed, 17 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> >> index 6091b691ddc2..2d64e289cf6e 100644
> >> --- a/drivers/bluetooth/btintel.c
> >> +++ b/drivers/bluetooth/btintel.c
> >> @@ -2215,7 +2215,7 @@ static int btintel_get_codec_config_data(struct hci_dev *hdev,
> >> static int btintel_get_data_path_id(struct hci_dev *hdev, __u8 *data_path_id)
> >> {
> >> /* Intel uses 1 as data path id for all the usecases */
> >> - *data_path_id = 1;
> >> + *data_path_id = BT_SCO_PCM_PATH;
> >> return 0;
> >> }
> >>
> >> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> >> index c1fa90fb7502..9e2745863b33 100644
> >> --- a/include/net/bluetooth/bluetooth.h
> >> +++ b/include/net/bluetooth/bluetooth.h
> >> @@ -177,6 +177,10 @@ struct bt_codecs {
> >> #define CODING_FORMAT_TRANSPARENT 0x03
> >> #define CODING_FORMAT_MSBC 0x05
> >>
> >> +/* Audio data transport path used for SCO */
> >> +#define BT_SCO_HCI_PATH 0x00
> >> +#define BT_SCO_PCM_PATH 0x01
> >
> > If this is in fact vendor specific perhaps you should be declared in
> > btintel.h not here.
> >
> >> +
> >> __printf(1, 2)
> >> void bt_info(const char *fmt, ...);
> >> __printf(1, 2)
> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> index b48e24629fa4..7ff11cba89cf 100644
> >> --- a/net/bluetooth/hci_event.c
> >> +++ b/net/bluetooth/hci_event.c
> >> @@ -4516,15 +4516,17 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
> >>
> >> bt_dev_dbg(hdev, "SCO connected with air mode: %02x", ev->air_mode);
> >>
> >> - switch (ev->air_mode) {
> >> - case 0x02:
> >> - if (hdev->notify)
> >> - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> >> - break;
> >> - case 0x03:
> >> - if (hdev->notify)
> >> - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> >> - break;
> >> + if (conn->codec.data_path == BT_SCO_HCI_PATH) {
> >> + switch (ev->air_mode) {
> >> + case 0x02:
> >> + if (hdev->notify)
> >> + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> >> + break;
> >> + case 0x03:
> >> + if (hdev->notify)
> >> + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> >> + break;
> >> + }
> >
> > Hmm I think we might need to notify the driver to enable PCM routing
> > so the likes of btusb can call
> > usb_disable_endpoint/usb_enable_endpoint for example since in theory
> > userspace may choose to switch from software to hardware offload and
> > vice-versa, note without calling usb_disable_endpoint there might not
> > be much power saving after all since the endpoint will remain active
> > or do we actually have a good explanation why it shall not be called
> > when using PCM routing? Note that usb_set_interface will call
> > usb_enable_interface that will then call usb_enable_endpoint so
> > perhaps we need to call usb_disable_interface, either way we can't
> > assume the platform will be restricted to only use one or the other.
>
> actually for the Intel hardware we shouldn’t do this at all. We should switch to vendor specific SCO over bulk endpoints and not claim the ISOC endpoints at all.
Yep, but I guess that requires switching to SCO over bulk then which
perhaps needs more changes, not sure if we should pursue that or go
with all the way with H4 mode like we did in Zephyr, anyway for the
purpose of offload I would be fine skipping the SCO over bulk since we
are already at v13 of these.
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
Hi Chethan,
On Wed, Sep 1, 2021 at 8:52 PM Tumkur Narayan, Chethan
<[email protected]> wrote:
>
> Hi Luiz,
>
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <[email protected]>
> > Sent: Thursday, September 2, 2021 5:24 AM
> > To: K, Kiran <[email protected]>
> > Cc: [email protected]; Tumkur Narayan, Chethan
> > <[email protected]>
> > Subject: Re: [PATCH v13 12/12] Bluetooth: Allow usb to auto-suspend when SCO
> > use non-HCI transport
> >
> > Hi Kiran,
> >
> > On Tue, Aug 31, 2021 at 4:54 AM Kiran K <[email protected]> wrote:
> > >
> > > From: Chethan T N <[email protected]>
> > >
> > > Currently usb tranport is not allowed to suspend when SCO over HCI
> > > tranport is active.
> > >
> > > This patch shall enable the usb tranport to suspend when SCO link use
> > > non-HCI transport
> > >
> > > Signed-off-by: Chethan T N <[email protected]>
> > > Signed-off-by: Kiran K <[email protected]>
> > > ---
> > >
> > > Notes:
> > > changes in v13:
> > > - suspend usb in HFP offload use case
> > >
> > > drivers/bluetooth/btintel.c | 2 +-
> > > include/net/bluetooth/bluetooth.h | 4 ++++
> > > net/bluetooth/hci_event.c | 20 +++++++++++---------
> > > net/bluetooth/sco.c | 2 +-
> > > 4 files changed, 17 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > > index 6091b691ddc2..2d64e289cf6e 100644
> > > --- a/drivers/bluetooth/btintel.c
> > > +++ b/drivers/bluetooth/btintel.c
> > > @@ -2215,7 +2215,7 @@ static int btintel_get_codec_config_data(struct
> > > hci_dev *hdev, static int btintel_get_data_path_id(struct hci_dev
> > > *hdev, __u8 *data_path_id) {
> > > /* Intel uses 1 as data path id for all the usecases */
> > > - *data_path_id = 1;
> > > + *data_path_id = BT_SCO_PCM_PATH;
> > > return 0;
> > > }
> > >
> > > diff --git a/include/net/bluetooth/bluetooth.h
> > > b/include/net/bluetooth/bluetooth.h
> > > index c1fa90fb7502..9e2745863b33 100644
> > > --- a/include/net/bluetooth/bluetooth.h
> > > +++ b/include/net/bluetooth/bluetooth.h
> > > @@ -177,6 +177,10 @@ struct bt_codecs {
> > > #define CODING_FORMAT_TRANSPARENT 0x03
> > > #define CODING_FORMAT_MSBC 0x05
> > >
> > > +/* Audio data transport path used for SCO */ #define BT_SCO_HCI_PATH
> > > +0x00 #define BT_SCO_PCM_PATH 0x01
> >
> > If this is in fact vendor specific perhaps you should be declared in btintel.h not
> > here.
> This is defined the Host Controller Interface assigned numbers, page no.3 "Transport Layer"- https://btprodspecificationrefs.blob.core.windows.net/assigned-numbers/Assigned%20Number%20Types/Host%20Controller%20Interface.pdf. So defined in bluetooth.h, let me know if you think otherwise.
BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 4, Part E
page 2221
Data_Path_ID:
0x01 to 0xFE Logical channel number; the meaning is vendor-specific.
BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 4, Part E
page 2022
Input_Data_Path:
0x01 to 0xFE Logical_Channel_Number. The meaning of the logical channels will be
vendor specific.
Output_Data_Path:
0x01 to 0xFE Logical_Channel_Number. The meaning of the logical channels will be
vendor specific.
> >
> > > +
> > > __printf(1, 2)
> > > void bt_info(const char *fmt, ...);
> > > __printf(1, 2)
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index b48e24629fa4..7ff11cba89cf 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -4516,15 +4516,17 @@ static void hci_sync_conn_complete_evt(struct
> > > hci_dev *hdev,
> > >
> > > bt_dev_dbg(hdev, "SCO connected with air mode: %02x",
> > > ev->air_mode);
> > >
> > > - switch (ev->air_mode) {
> > > - case 0x02:
> > > - if (hdev->notify)
> > > - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> > > - break;
> > > - case 0x03:
> > > - if (hdev->notify)
> > > - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> > > - break;
> > > + if (conn->codec.data_path == BT_SCO_HCI_PATH) {
> > > + switch (ev->air_mode) {
> > > + case 0x02:
> > > + if (hdev->notify)
> > > + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> > > + break;
> > > + case 0x03:
> > > + if (hdev->notify)
> > > + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> > > + break;
> > > + }
> >
> > Hmm I think we might need to notify the driver to enable PCM routing so the
> > likes of btusb can call usb_disable_endpoint/usb_enable_endpoint for example
> > since in theory userspace may choose to switch from
> > offload and vice-versa, note without calling usb_disable_endpoint there might
> > not be much power saving after all since the endpoint will remain active or do
> > we actually have a good explanation why it shall not be called when using PCM
> > routing? Note that usb_set_interface will call usb_enable_interface that will
> > then call usb_enable_endpoint so perhaps we need to call
> > usb_disable_interface, either way we can't assume the platform will be
> > restricted to only use one or the other.
> Ack, Does it make sense to define and notify events "HCI_NOTIFY_DISABLE_SCO_USB_INTF ", "HCI_NOTIFY_ENABLE_SCO_USB_INTF " accordingly and handle this in btusb driver by disabling/enabling the ISOC endpoint respectively. That will serve the purpose or switch between software to hardware.
Or perhaps we should switch to notify the actual data path, in fact
there could be situations where we have both hardware offload and
software based if we were dealing with multiple connections in which
case we would need to check if there is any connection using HCI
routing before disabling it.
> >
> > > }
> > >
> > > hci_connect_cfm(conn, ev->status); diff --git
> > > a/net/bluetooth/sco.c b/net/bluetooth/sco.c index
> > > 004bce2b5eca..f35c12ca6aa5 100644
> > > --- a/net/bluetooth/sco.c
> > > +++ b/net/bluetooth/sco.c
> > > @@ -506,7 +506,7 @@ static struct sock *sco_sock_alloc(struct net *net,
> > struct socket *sock,
> > > 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;
> > > + sco_pi(sk)->codec.data_path = BT_SCO_HCI_PATH;
> > >
> > > bt_sock_link(&sco_sk_list, sk);
> > > return sk;
> > > --
> > > 2.17.1
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <[email protected]>
> Sent: Friday, September 3, 2021 4:07 AM
> To: Tumkur Narayan, Chethan <[email protected]>
> Cc: K, Kiran <[email protected]>; [email protected]; Pierres,
> Arnaud <[email protected]>
> Subject: Re: [PATCH v13 12/12] Bluetooth: Allow usb to auto-suspend when SCO
> use non-HCI transport
>
> Hi Chethan,
>
> On Wed, Sep 1, 2021 at 8:52 PM Tumkur Narayan, Chethan
> <[email protected]> wrote:
> >
> > Hi Luiz,
> >
> > > -----Original Message-----
> > > From: Luiz Augusto von Dentz <[email protected]>
> > > Sent: Thursday, September 2, 2021 5:24 AM
> > > To: K, Kiran <[email protected]>
> > > Cc: [email protected]; Tumkur Narayan, Chethan
> > > <[email protected]>
> > > Subject: Re: [PATCH v13 12/12] Bluetooth: Allow usb to auto-suspend
> > > when SCO use non-HCI transport
> > >
> > > Hi Kiran,
> > >
> > > On Tue, Aug 31, 2021 at 4:54 AM Kiran K <[email protected]> wrote:
> > > >
> > > > From: Chethan T N <[email protected]>
> > > >
> > > > Currently usb tranport is not allowed to suspend when SCO over HCI
> > > > tranport is active.
> > > >
> > > > This patch shall enable the usb tranport to suspend when SCO link
> > > > use non-HCI transport
> > > >
> > > > Signed-off-by: Chethan T N <[email protected]>
> > > > Signed-off-by: Kiran K <[email protected]>
> > > > ---
> > > >
> > > > Notes:
> > > > changes in v13:
> > > > - suspend usb in HFP offload use case
> > > >
> > > > drivers/bluetooth/btintel.c | 2 +-
> > > > include/net/bluetooth/bluetooth.h | 4 ++++
> > > > net/bluetooth/hci_event.c | 20 +++++++++++---------
> > > > net/bluetooth/sco.c | 2 +-
> > > > 4 files changed, 17 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/bluetooth/btintel.c
> > > > b/drivers/bluetooth/btintel.c index 6091b691ddc2..2d64e289cf6e
> > > > 100644
> > > > --- a/drivers/bluetooth/btintel.c
> > > > +++ b/drivers/bluetooth/btintel.c
> > > > @@ -2215,7 +2215,7 @@ static int
> > > > btintel_get_codec_config_data(struct
> > > > hci_dev *hdev, static int btintel_get_data_path_id(struct hci_dev
> > > > *hdev, __u8 *data_path_id) {
> > > > /* Intel uses 1 as data path id for all the usecases */
> > > > - *data_path_id = 1;
> > > > + *data_path_id = BT_SCO_PCM_PATH;
> > > > return 0;
> > > > }
> > > >
> > > > diff --git a/include/net/bluetooth/bluetooth.h
> > > > b/include/net/bluetooth/bluetooth.h
> > > > index c1fa90fb7502..9e2745863b33 100644
> > > > --- a/include/net/bluetooth/bluetooth.h
> > > > +++ b/include/net/bluetooth/bluetooth.h
> > > > @@ -177,6 +177,10 @@ struct bt_codecs {
> > > > #define CODING_FORMAT_TRANSPARENT 0x03
> > > > #define CODING_FORMAT_MSBC 0x05
> > > >
> > > > +/* Audio data transport path used for SCO */ #define
> > > > +BT_SCO_HCI_PATH
> > > > +0x00 #define BT_SCO_PCM_PATH 0x01
> > >
> > > If this is in fact vendor specific perhaps you should be declared in
> > > btintel.h not here.
> > This is defined the Host Controller Interface assigned numbers, page no.3
> "Transport Layer"-
> https://btprodspecificationrefs.blob.core.windows.net/assigned-
> numbers/Assigned%20Number%20Types/Host%20Controller%20Interface.pdf.
> So defined in bluetooth.h, let me know if you think otherwise.
>
> BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 4, Part E page 2221
> Data_Path_ID:
> 0x01 to 0xFE Logical channel number; the meaning is vendor-specific.
>
>
> BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 4, Part E page 2022
> Input_Data_Path:
> 0x01 to 0xFE Logical_Channel_Number. The meaning of the logical channels will
> be vendor specific.
> Output_Data_Path:
> 0x01 to 0xFE Logical_Channel_Number. The meaning of the logical channels will
> be vendor specific.
>
Ack, will remove/move these #defines accordingly.
> > >
> > > > +
> > > > __printf(1, 2)
> > > > void bt_info(const char *fmt, ...); __printf(1, 2) diff --git
> > > > a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index
> > > > b48e24629fa4..7ff11cba89cf 100644
> > > > --- a/net/bluetooth/hci_event.c
> > > > +++ b/net/bluetooth/hci_event.c
> > > > @@ -4516,15 +4516,17 @@ static void
> > > > hci_sync_conn_complete_evt(struct hci_dev *hdev,
> > > >
> > > > bt_dev_dbg(hdev, "SCO connected with air mode: %02x",
> > > > ev->air_mode);
> > > >
> > > > - switch (ev->air_mode) {
> > > > - case 0x02:
> > > > - if (hdev->notify)
> > > > - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> > > > - break;
> > > > - case 0x03:
> > > > - if (hdev->notify)
> > > > - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> > > > - break;
> > > > + if (conn->codec.data_path == BT_SCO_HCI_PATH) {
> > > > + switch (ev->air_mode) {
> > > > + case 0x02:
> > > > + if (hdev->notify)
> > > > + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> > > > + break;
> > > > + case 0x03:
> > > > + if (hdev->notify)
> > > > + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> > > > + break;
> > > > + }
> > >
> > > Hmm I think we might need to notify the driver to enable PCM routing
> > > so the likes of btusb can call
> > > usb_disable_endpoint/usb_enable_endpoint for example since in theory
> > > userspace may choose to switch from offload and vice-versa, note
> > > without calling usb_disable_endpoint there might not be much power
> > > saving after all since the endpoint will remain active or do we
> > > actually have a good explanation why it shall not be called when
> > > using PCM routing? Note that usb_set_interface will call
> > > usb_enable_interface that will then call usb_enable_endpoint so
> > > perhaps we need to call usb_disable_interface, either way we can't assume
> the platform will be restricted to only use one or the other.
> > Ack, Does it make sense to define and notify events
> "HCI_NOTIFY_DISABLE_SCO_USB_INTF ",
> "HCI_NOTIFY_ENABLE_SCO_USB_INTF " accordingly and handle this in btusb
> driver by disabling/enabling the ISOC endpoint respectively. That will serve the
> purpose or switch between software to hardware.
>
> Or perhaps we should switch to notify the actual data path, in fact there could
> be situations where we have both hardware offload and software based if we
> were dealing with multiple connections in which case we would need to check if
> there is any connection using HCI routing before disabling it.
>
Actually, there are no API's from USB core exposed to disable the interface or endpoints to any device driver. However as discussed setting the altsetting to 0 to the ISOC endpoint would be feasible and by doing so no memory and bandwidth shall be allocated for the interface.
Likewise when it comes to switching from offload to non-offload or vice versa the same logic of resetting the ISOC endpoint to altsetting 0 makes reliable on SCO disconnection and also I checked the flow where on every SCO disconnection in the clean up procedure "HCI_NOTIFY_DISABLE_SCO" shall be notify to the driver that shall reset ISOC interface with altsetting 0. Having said that no additional handling of disabling the ISOC interface would be required.
I shall re-work and send the updated patch.
> > >
> > > > }
> > > >
> > > > hci_connect_cfm(conn, ev->status); diff --git
> > > > a/net/bluetooth/sco.c b/net/bluetooth/sco.c index
> > > > 004bce2b5eca..f35c12ca6aa5 100644
> > > > --- a/net/bluetooth/sco.c
> > > > +++ b/net/bluetooth/sco.c
> > > > @@ -506,7 +506,7 @@ static struct sock *sco_sock_alloc(struct net
> > > > *net,
> > > struct socket *sock,
> > > > 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;
> > > > + sco_pi(sk)->codec.data_path = BT_SCO_HCI_PATH;
> > > >
> > > > bt_sock_link(&sco_sk_list, sk);
> > > > return sk;
> > > > --
> > > > 2.17.1
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
Hi Luiz,
>>>> Currently usb tranport is not allowed to suspend when SCO over
>>>> HCI tranport is active.
>>>>
>>>> This patch shall enable the usb tranport to suspend when SCO
>>>> link use non-HCI transport
>>>>
>>>> Signed-off-by: Chethan T N <[email protected]>
>>>> Signed-off-by: Kiran K <[email protected]>
>>>> ---
>>>>
>>>> Notes:
>>>> changes in v13:
>>>> - suspend usb in HFP offload use case
>>>>
>>>> drivers/bluetooth/btintel.c | 2 +-
>>>> include/net/bluetooth/bluetooth.h | 4 ++++
>>>> net/bluetooth/hci_event.c | 20 +++++++++++---------
>>>> net/bluetooth/sco.c | 2 +-
>>>> 4 files changed, 17 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
>>>> index 6091b691ddc2..2d64e289cf6e 100644
>>>> --- a/drivers/bluetooth/btintel.c
>>>> +++ b/drivers/bluetooth/btintel.c
>>>> @@ -2215,7 +2215,7 @@ static int btintel_get_codec_config_data(struct hci_dev *hdev,
>>>> static int btintel_get_data_path_id(struct hci_dev *hdev, __u8 *data_path_id)
>>>> {
>>>> /* Intel uses 1 as data path id for all the usecases */
>>>> - *data_path_id = 1;
>>>> + *data_path_id = BT_SCO_PCM_PATH;
>>>> return 0;
>>>> }
>>>>
>>>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>>>> index c1fa90fb7502..9e2745863b33 100644
>>>> --- a/include/net/bluetooth/bluetooth.h
>>>> +++ b/include/net/bluetooth/bluetooth.h
>>>> @@ -177,6 +177,10 @@ struct bt_codecs {
>>>> #define CODING_FORMAT_TRANSPARENT 0x03
>>>> #define CODING_FORMAT_MSBC 0x05
>>>>
>>>> +/* Audio data transport path used for SCO */
>>>> +#define BT_SCO_HCI_PATH 0x00
>>>> +#define BT_SCO_PCM_PATH 0x01
>>>
>>> If this is in fact vendor specific perhaps you should be declared in
>>> btintel.h not here.
>>>
>>>> +
>>>> __printf(1, 2)
>>>> void bt_info(const char *fmt, ...);
>>>> __printf(1, 2)
>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>>> index b48e24629fa4..7ff11cba89cf 100644
>>>> --- a/net/bluetooth/hci_event.c
>>>> +++ b/net/bluetooth/hci_event.c
>>>> @@ -4516,15 +4516,17 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
>>>>
>>>> bt_dev_dbg(hdev, "SCO connected with air mode: %02x", ev->air_mode);
>>>>
>>>> - switch (ev->air_mode) {
>>>> - case 0x02:
>>>> - if (hdev->notify)
>>>> - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
>>>> - break;
>>>> - case 0x03:
>>>> - if (hdev->notify)
>>>> - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
>>>> - break;
>>>> + if (conn->codec.data_path == BT_SCO_HCI_PATH) {
>>>> + switch (ev->air_mode) {
>>>> + case 0x02:
>>>> + if (hdev->notify)
>>>> + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
>>>> + break;
>>>> + case 0x03:
>>>> + if (hdev->notify)
>>>> + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
>>>> + break;
>>>> + }
>>>
>>> Hmm I think we might need to notify the driver to enable PCM routing
>>> so the likes of btusb can call
>>> usb_disable_endpoint/usb_enable_endpoint for example since in theory
>>> userspace may choose to switch from software to hardware offload and
>>> vice-versa, note without calling usb_disable_endpoint there might not
>>> be much power saving after all since the endpoint will remain active
>>> or do we actually have a good explanation why it shall not be called
>>> when using PCM routing? Note that usb_set_interface will call
>>> usb_enable_interface that will then call usb_enable_endpoint so
>>> perhaps we need to call usb_disable_interface, either way we can't
>>> assume the platform will be restricted to only use one or the other.
>>
>> actually for the Intel hardware we shouldn’t do this at all. We should switch to vendor specific SCO over bulk endpoints and not claim the ISOC endpoints at all.
>
> Yep, but I guess that requires switching to SCO over bulk then which
> perhaps needs more changes, not sure if we should pursue that or go
> with all the way with H4 mode like we did in Zephyr, anyway for the
> purpose of offload I would be fine skipping the SCO over bulk since we
> are already at v13 of these.
actually SCO over bulk is rather simple. You jus send one extra HCI command at init (just need to figure out if it survices HCI_Reset) and then you only need to look up the handle if it is SCO or ACL.
Regards
Marcel