ASCS v1.0 Sec. 4.1: "For each ASE characteristic (distinguished by their
attribute handles), the server shall expose separate ASE characteristic
values for each client." In shared/bap.c, the ASE chrc value is
contained in bt_bap_endpoint struct.
As implemented currently, the same local ASE chrc value is shared
between all clients, in contradiction with the above. Namely, the
bt_bap_endpoint is looked up based on the gatt_db_attribute handles, in
bap_get_endpoint and bap_get_endpoint_id, but the handles correspond to
the ASCS registrations, and do not depend on which client is in
question.
Fix this by moving the endpoint states to the bt_bap BAP session struct,
so that the ASE state is associated with the sessions, and each session
has separate state.
---
Notes:
Inserting
DBG(bap, "bap:%p ep:%p -> id:%d state:%d", bap, ep, rsp.id, rsp.state);
at the end of ascs_ase_read on BlueZ running on the BAP server and see
the following:
Client A connects and starts streaming:
bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e000008d20 ep:0x603000042f70 -> id:1 state:0
bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e000008d20 ep:0x603000043330 -> id:2 state:0
bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e000008d20 ep:0x6030000436c0 -> id:3 state:0
bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e000008d20 ep:0x603000043a50 -> id:4 state:0
Client B connects (does not start streaming):
bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e0000095e0 ep:0x603000042f70 -> id:1 state:4
bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e0000095e0 ep:0x603000043330 -> id:2 state:0
bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e0000095e0 ep:0x6030000436c0 -> id:3 state:4
bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e0000095e0 ep:0x603000043a50 -> id:4 state:0
btmon for client B:
> ACL Data RX: Handle 16 flags 0x02 dlen 9 #347 [hci1] 52.809304
Channel: 64 len 5 sdu 3 [PSM 39 mode LE Flow Control (0x80)] {chan 0}
ATT: Read Response (0x0b) len 2
Value: 0104
Handle: 0x0052 Type: Sink ASE (0x2bc4)
ASE ID: 1
State: Streaming (0x04)
CIG ID: invalid size
The endpoints are the same for both sessions, and client B found out
that ASEs for client A were in streaming state, which appears to not be
per spec.
With this patch, the different bap sessions get different endpoints, and
client B starts with ASEs in idle state, while client A is still
streaming.
In theory, the code appears to otherwise be structured so that
everything should work for multiple clients. However, I don't have
enough CIS capable devices at hand now to do full testing of
simultaneous streaming from multiple clients.
src/shared/bap.c | 65 +++++++++++++++++++++++++-----------------------
1 file changed, 34 insertions(+), 31 deletions(-)
diff --git a/src/shared/bap.c b/src/shared/bap.c
index 22f2e6714..f16ba1832 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -114,7 +114,6 @@ struct bt_bap_db {
struct bt_ascs *ascs;
struct queue *sinks;
struct queue *sources;
- struct queue *endpoints;
};
struct bt_bap_req {
@@ -165,6 +164,8 @@ struct bt_bap {
struct queue *pending;
struct queue *notify;
struct queue *streams;
+ struct queue *local_eps;
+ struct queue *remote_eps;
struct queue *pac_cbs;
struct queue *ready_cbs;
@@ -615,7 +616,8 @@ static struct bt_bap_endpoint *bap_endpoint_new(struct bt_bap_db *bdb,
return ep;
}
-static struct bt_bap_endpoint *bap_get_endpoint(struct bt_bap_db *db,
+static struct bt_bap_endpoint *bap_get_endpoint(struct queue *endpoints,
+ struct bt_bap_db *db,
struct gatt_db_attribute *attr)
{
struct bt_bap_endpoint *ep;
@@ -623,7 +625,7 @@ static struct bt_bap_endpoint *bap_get_endpoint(struct bt_bap_db *db,
if (!db || !attr)
return NULL;
- ep = queue_find(db->endpoints, bap_endpoint_match, attr);
+ ep = queue_find(endpoints, bap_endpoint_match, attr);
if (ep)
return ep;
@@ -631,7 +633,7 @@ static struct bt_bap_endpoint *bap_get_endpoint(struct bt_bap_db *db,
if (!ep)
return NULL;
- queue_push_tail(db->endpoints, ep);
+ queue_push_tail(endpoints, ep);
return ep;
}
@@ -644,23 +646,22 @@ static bool bap_endpoint_match_id(const void *data, const void *match_data)
return (ep->id == id);
}
-static struct bt_bap_endpoint *bap_get_endpoint_id(struct bt_bap *bap,
- struct bt_bap_db *db,
- uint8_t id)
+static struct bt_bap_endpoint *bap_get_local_endpoint_id(struct bt_bap *bap,
+ uint8_t id)
{
struct bt_bap_endpoint *ep;
struct gatt_db_attribute *attr = NULL;
size_t i;
- if (!bap || !db)
+ if (!bap)
return NULL;
- ep = queue_find(db->endpoints, bap_endpoint_match_id, UINT_TO_PTR(id));
+ ep = queue_find(bap->local_eps, bap_endpoint_match_id, UINT_TO_PTR(id));
if (ep)
return ep;
- for (i = 0; i < ARRAY_SIZE(db->ascs->ase); i++) {
- struct bt_ase *ase = db->ascs->ase[i];
+ for (i = 0; i < ARRAY_SIZE(bap->ldb->ascs->ase); i++) {
+ struct bt_ase *ase = bap->ldb->ascs->ase[i];
if (id) {
if (ase->id != id)
@@ -669,7 +670,7 @@ static struct bt_bap_endpoint *bap_get_endpoint_id(struct bt_bap *bap,
break;
}
- ep = queue_find(db->endpoints, bap_endpoint_match, ase->attr);
+ ep = queue_find(bap->local_eps, bap_endpoint_match, ase->attr);
if (!ep) {
attr = ase->attr;
break;
@@ -679,12 +680,12 @@ static struct bt_bap_endpoint *bap_get_endpoint_id(struct bt_bap *bap,
if (!attr)
return NULL;
- ep = bap_endpoint_new(db, attr);
+ ep = bap_endpoint_new(bap->ldb, attr);
if (!ep)
return NULL;
ep->id = id;
- queue_push_tail(db->endpoints, ep);
+ queue_push_tail(bap->local_eps, ep);
return ep;
}
@@ -696,7 +697,8 @@ static void ascs_ase_read(struct gatt_db_attribute *attrib,
{
struct bt_ase *ase = user_data;
struct bt_bap *bap = bap_get_session(att, ase->ascs->bdb->db);
- struct bt_bap_endpoint *ep = bap_get_endpoint(bap->ldb, attrib);
+ struct bt_bap_endpoint *ep = bap_get_endpoint(bap->local_eps,
+ bap->ldb, attrib);
struct bt_ascs_ase_status rsp;
if (!ase || !bap || !ep) {
@@ -1470,7 +1472,7 @@ static uint8_t ascs_config(struct bt_ascs *ascs, struct bt_bap *bap,
DBG(bap, "codec 0x%02x phy 0x%02x latency %u", req->codec.id, req->phy,
req->latency);
- ep = bap_get_endpoint_id(bap, bap->ldb, req->ase);
+ ep = bap_get_local_endpoint_id(bap, req->ase);
if (!ep) {
DBG(bap, "Invalid ASE ID 0x%02x", req->ase);
ascs_ase_rsp_add(rsp, req->ase,
@@ -1552,7 +1554,7 @@ static uint8_t ascs_qos(struct bt_ascs *ascs, struct bt_bap *bap,
req->cig, req->cis, qos.interval, qos.framing, qos.phy,
qos.sdu, qos.rtn, qos.latency, qos.delay);
- ep = bap_get_endpoint_id(bap, bap->ldb, req->ase);
+ ep = bap_get_local_endpoint_id(bap, req->ase);
if (!ep) {
DBG(bap, "%s: Invalid ASE ID 0x%02x", req->ase);
ascs_ase_rsp_add(rsp, req->ase,
@@ -1675,7 +1677,7 @@ static uint8_t ascs_enable(struct bt_ascs *ascs, struct bt_bap *bap,
req = util_iov_pull_mem(iov, sizeof(*req));
- ep = bap_get_endpoint_id(bap, bap->ldb, req->meta.ase);
+ ep = bap_get_local_endpoint_id(bap, req->meta.ase);
if (!ep) {
DBG(bap, "Invalid ASE ID 0x%02x", req->meta.ase);
ascs_ase_rsp_add(rsp, req->meta.ase,
@@ -1739,7 +1741,7 @@ static uint8_t ascs_start(struct bt_ascs *ascs, struct bt_bap *bap,
req = util_iov_pull_mem(iov, sizeof(*req));
- ep = bap_get_endpoint_id(bap, bap->ldb, req->ase);
+ ep = bap_get_local_endpoint_id(bap, req->ase);
if (!ep) {
DBG(bap, "Invalid ASE ID 0x%02x", req->ase);
ascs_ase_rsp_add(rsp, req->ase,
@@ -1811,7 +1813,7 @@ static uint8_t ascs_disable(struct bt_ascs *ascs, struct bt_bap *bap,
req = util_iov_pull_mem(iov, sizeof(*req));
- ep = bap_get_endpoint_id(bap, bap->ldb, req->ase);
+ ep = bap_get_local_endpoint_id(bap, req->ase);
if (!ep) {
DBG(bap, "Invalid ASE ID 0x%02x", req->ase);
ascs_ase_rsp_add(rsp, req->ase,
@@ -1886,7 +1888,7 @@ static uint8_t ascs_stop(struct bt_ascs *ascs, struct bt_bap *bap,
req = util_iov_pull_mem(iov, sizeof(*req));
- ep = bap_get_endpoint_id(bap, bap->ldb, req->ase);
+ ep = bap_get_local_endpoint_id(bap, req->ase);
if (!ep) {
DBG(bap, "Invalid ASE ID 0x%02x", req->ase);
ascs_ase_rsp_add(rsp, req->ase,
@@ -1951,7 +1953,7 @@ static uint8_t ascs_metadata(struct bt_ascs *ascs, struct bt_bap *bap,
req = util_iov_pull_mem(iov, sizeof(*req));
- ep = bap_get_endpoint_id(bap, bap->ldb, req->ase);
+ ep = bap_get_local_endpoint_id(bap, req->ase);
if (!ep) {
DBG(bap, "Invalid ASE ID 0x%02x", req->ase);
ascs_ase_rsp_add(rsp, req->ase,
@@ -1995,7 +1997,7 @@ static uint8_t ascs_release(struct bt_ascs *ascs, struct bt_bap *bap,
req = util_iov_pull_mem(iov, sizeof(*req));
- ep = bap_get_endpoint_id(bap, bap->ldb, req->ase);
+ ep = bap_get_local_endpoint_id(bap, req->ase);
if (!ep) {
DBG(bap, "Invalid ASE ID 0x%02x", req->ase);
ascs_ase_rsp_add(rsp, req->ase,
@@ -2181,7 +2183,6 @@ static struct bt_bap_db *bap_db_new(struct gatt_db *db)
bdb->db = gatt_db_ref(db);
bdb->sinks = queue_new();
bdb->sources = queue_new();
- bdb->endpoints = queue_new();
if (!bap_db)
bap_db = queue_new();
@@ -2513,7 +2514,6 @@ static void bap_db_free(void *data)
queue_destroy(bdb->sinks, bap_pac_free);
queue_destroy(bdb->sources, bap_pac_free);
- queue_destroy(bdb->endpoints, free);
gatt_db_unref(bdb->db);
free(bdb->pacs);
@@ -2574,6 +2574,8 @@ static void bap_free(void *data)
queue_destroy(bap->pac_cbs, pac_changed_free);
queue_destroy(bap->ready_cbs, bap_ready_free);
queue_destroy(bap->state_cbs, bap_state_free);
+ queue_destroy(bap->local_eps, free);
+ queue_destroy(bap->remote_eps, free);
queue_destroy(bap->reqs, bap_req_free);
queue_destroy(bap->pending, NULL);
@@ -2656,6 +2658,7 @@ struct bt_bap *bt_bap_new(struct gatt_db *ldb, struct gatt_db *rdb)
bap->ready_cbs = queue_new();
bap->streams = queue_new();
bap->state_cbs = queue_new();
+ bap->local_eps = queue_new();
if (!rdb)
goto done;
@@ -2664,9 +2667,9 @@ struct bt_bap *bt_bap_new(struct gatt_db *ldb, struct gatt_db *rdb)
bdb->db = gatt_db_ref(rdb);
bdb->sinks = queue_new();
bdb->sources = queue_new();
- bdb->endpoints = queue_new();
bap->rdb = bdb;
+ bap->remote_eps = queue_new();
done:
return bt_bap_ref(bap);
@@ -3670,7 +3673,7 @@ static void foreach_ascs_char(struct gatt_db_attribute *attr, void *user_data)
!bt_uuid_cmp(&uuid, &uuid_source)) {
struct bt_bap_endpoint *ep;
- ep = bap_get_endpoint(bap->rdb, attr);
+ ep = bap_get_endpoint(bap->remote_eps, bap->rdb, attr);
if (!ep)
return;
@@ -3792,7 +3795,7 @@ clone:
}
}
- queue_foreach(bap->rdb->endpoints, bap_endpoint_foreach, bap);
+ queue_foreach(bap->remote_eps, bap_endpoint_foreach, bap);
bap_cp_attach(bap);
@@ -4192,7 +4195,7 @@ struct bt_bap_stream *bt_bap_config(struct bt_bap *bap,
struct match_pac match;
int id;
- if (!bap || !bap->rdb || queue_isempty(bap->rdb->endpoints))
+ if (!bap || !bap->rdb || queue_isempty(bap->remote_eps))
return NULL;
if (lpac && rpac) {
@@ -4233,10 +4236,10 @@ struct bt_bap_stream *bt_bap_config(struct bt_bap *bap,
match.rpac = rpac;
/* Check for existing stream */
- ep = queue_find(bap->rdb->endpoints, find_ep_pacs, &match);
+ ep = queue_find(bap->remote_eps, find_ep_pacs, &match);
if (!ep) {
/* Check for unused ASE */
- ep = queue_find(bap->rdb->endpoints, find_ep_unused, &match);
+ ep = queue_find(bap->remote_eps, find_ep_unused, &match);
if (!ep) {
DBG(bap, "Unable to find unused ASE");
return NULL;
--
2.39.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=721007
---Test result---
Test Summary:
CheckPatch PASS 1.20 seconds
GitLint FAIL 0.99 seconds
BuildEll PASS 32.90 seconds
BluezMake PASS 1023.32 seconds
MakeCheck PASS 12.94 seconds
MakeDistcheck PASS 183.68 seconds
CheckValgrind PASS 298.03 seconds
CheckSmatch PASS 394.87 seconds
bluezmakeextell PASS 119.38 seconds
IncrementalBuild PASS 1661.75 seconds
ScanBuild PASS 1231.11 seconds
Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,v2,1/2] shared/bap: fix local endpoint state to be per-client
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
22: B2 Line has trailing whitespace: " "
24: B2 Line has trailing whitespace: " "
27: B2 Line has trailing whitespace: " "
29: B2 Line has trailing whitespace: " "
30: B1 Line exceeds max length (109>80): " bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e000008d20 ep:0x603000042f70 -> id:1 state:0"
31: B1 Line exceeds max length (109>80): " bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e000008d20 ep:0x603000043330 -> id:2 state:0"
32: B1 Line exceeds max length (109>80): " bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e000008d20 ep:0x6030000436c0 -> id:3 state:0"
33: B1 Line exceeds max length (109>80): " bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e000008d20 ep:0x603000043a50 -> id:4 state:0"
34: B2 Line has trailing whitespace: " "
36: B2 Line has trailing whitespace: " "
37: B1 Line exceeds max length (109>80): " bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e0000095e0 ep:0x603000042f70 -> id:1 state:4"
38: B1 Line exceeds max length (109>80): " bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e0000095e0 ep:0x603000043330 -> id:2 state:0"
39: B1 Line exceeds max length (109>80): " bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e0000095e0 ep:0x6030000436c0 -> id:3 state:4"
40: B1 Line exceeds max length (109>80): " bluetoothd[139699]: src/shared/bap.c:ascs_ase_read() bap:0x60e0000095e0 ep:0x603000043a50 -> id:4 state:0"
41: B2 Line has trailing whitespace: " "
43: B2 Line has trailing whitespace: " "
52: B2 Line has trailing whitespace: " "
56: B2 Line has trailing whitespace: " "
60: B2 Line has trailing whitespace: " "
---
Regards,
Linux Bluetooth
Hello:
This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[email protected]>:
On Sat, 11 Feb 2023 19:55:52 +0000 you wrote:
> ASCS v1.0 Sec. 4.1: "For each ASE characteristic (distinguished by their
> attribute handles), the server shall expose separate ASE characteristic
> values for each client." In shared/bap.c, the ASE chrc value is
> contained in bt_bap_endpoint struct.
>
> As implemented currently, the same local ASE chrc value is shared
> between all clients, in contradiction with the above. Namely, the
> bt_bap_endpoint is looked up based on the gatt_db_attribute handles, in
> bap_get_endpoint and bap_get_endpoint_id, but the handles correspond to
> the ASCS registrations, and do not depend on which client is in
> question.
>
> [...]
Here is the summary with links:
- [BlueZ,v2,1/2] shared/bap: fix local endpoint state to be per-client
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=f56205e30564
- [BlueZ,v2,2/2] media: clear the right transport when clearing BAP endpoint
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7b1b1a499cf3
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html