linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH BlueZ v2 1/2] shared/bap: fix local endpoint state to be per-client
@ 2023-02-11 19:55 Pauli Virtanen
  2023-02-11 19:55 ` [PATCH BlueZ v2 2/2] media: clear the right transport when clearing BAP endpoint Pauli Virtanen
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Pauli Virtanen @ 2023-02-11 19:55 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

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


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

* [PATCH BlueZ v2 2/2] media: clear the right transport when clearing BAP endpoint
  2023-02-11 19:55 [PATCH BlueZ v2 1/2] shared/bap: fix local endpoint state to be per-client Pauli Virtanen
@ 2023-02-11 19:55 ` Pauli Virtanen
  2023-02-12 14:00   ` [PATCH BlueZ v3] " Pauli Virtanen
  2023-02-11 21:38 ` [BlueZ,v2,1/2] shared/bap: fix local endpoint state to be per-client bluez.test.bot
  2023-02-13 20:30 ` [PATCH BlueZ v2 1/2] " patchwork-bot+bluetooth
  2 siblings, 1 reply; 6+ messages in thread
From: Pauli Virtanen @ 2023-02-11 19:55 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

Each configured BAP stream is associated with a transport, and only that
transport should be cleared when the stream's configuration is cleared.

This is required for multiple BAP clients to use the same Media1
endpoint, which is what we should aim for to follow the spirit in which
the endpoints work in ASCS.  Sound servers generally can handle this,
since the *Configuration calls provide the transport paths, and for
different clients they are associated with different devices.
---

Notes:
    In principle multiple clients should work, but I don't have equipment
    right now to test streaming for more than one client.

 profiles/audio/media.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 505c4b3a6..3eb038cb7 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -86,7 +86,6 @@ struct endpoint_request {
 struct media_endpoint {
 	struct a2dp_sep		*sep;
 	struct bt_bap_pac	*pac;
-	void			*stream;
 	char			*sender;	/* Endpoint DBus bus id */
 	char			*path;		/* Endpoint object path */
 	char			*uuid;		/* Endpoint property UUID */
@@ -1007,9 +1006,6 @@ static void pac_config_cb(struct media_endpoint *endpoint, void *ret, int size,
 	struct pac_config_data *data = user_data;
 	gboolean *ret_value = ret;
 
-	if (ret_value)
-		endpoint->stream = data->stream;
-
 	data->cb(data->stream, ret_value ? 0 : -EINVAL);
 }
 
@@ -1089,11 +1085,20 @@ static int pac_config(struct bt_bap_stream *stream, struct iovec *cfg,
 static void pac_clear(struct bt_bap_stream *stream, void *user_data)
 {
 	struct media_endpoint *endpoint = user_data;
+	struct media_transport *transport;
+	const char *path;
+
+	path = bt_bap_stream_get_user_data(stream);
+	if (!path)
+		return;
 
-	endpoint->stream = NULL;
+	DBG("endpoint %p path %s", endpoint, path);
 
-	while (endpoint->transports != NULL)
-		clear_configuration(endpoint, endpoint->transports->data);
+	transport = find_transport(endpoint, path);
+	if (transport) {
+		clear_configuration(endpoint, transport);
+		bt_bap_stream_set_user_data(stream, NULL);
+	}
 }
 
 static struct bt_bap_pac_ops pac_ops = {
-- 
2.39.1


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

* RE: [BlueZ,v2,1/2] shared/bap: fix local endpoint state to be per-client
  2023-02-11 19:55 [PATCH BlueZ v2 1/2] shared/bap: fix local endpoint state to be per-client Pauli Virtanen
  2023-02-11 19:55 ` [PATCH BlueZ v2 2/2] media: clear the right transport when clearing BAP endpoint Pauli Virtanen
@ 2023-02-11 21:38 ` bluez.test.bot
  2023-02-13 20:30 ` [PATCH BlueZ v2 1/2] " patchwork-bot+bluetooth
  2 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2023-02-11 21:38 UTC (permalink / raw)
  To: linux-bluetooth, pav

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

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=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


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

* [PATCH BlueZ v3] media: clear the right transport when clearing BAP endpoint
  2023-02-11 19:55 ` [PATCH BlueZ v2 2/2] media: clear the right transport when clearing BAP endpoint Pauli Virtanen
@ 2023-02-12 14:00   ` Pauli Virtanen
  2023-02-12 15:21     ` [BlueZ,v3] " bluez.test.bot
  0 siblings, 1 reply; 6+ messages in thread
From: Pauli Virtanen @ 2023-02-12 14:00 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

Each configured BAP stream is associated with a transport, and only that
transport should be cleared when the stream's configuration is cleared.

This is required for multiple BAP clients to use the same Media1
endpoint, which is what we should aim for to follow the spirit in which
the endpoints work in ASCS.  Sound servers generally can handle this,
since the *Configuration calls provide the transport paths, and for
different clients they are associated with different devices.
---

Notes:
    v3:
    * Match transports based on stream, not path.
    
      The path in bt_bap_stream user data is set to endpoint path on
      config, and may be freed too early, if endpoint is freed before
      clearing the PAC, as happens in bap.c:bap_disconnect
    
    In principle multiple clients should work, but I don't have equipment
    right now to test streaming for more than one client.

 profiles/audio/media.c     | 20 +++++++++++++-------
 profiles/audio/transport.c | 13 +++++++++++++
 profiles/audio/transport.h |  1 +
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 505c4b3a6..8728b69e0 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -86,7 +86,6 @@ struct endpoint_request {
 struct media_endpoint {
 	struct a2dp_sep		*sep;
 	struct bt_bap_pac	*pac;
-	void			*stream;
 	char			*sender;	/* Endpoint DBus bus id */
 	char			*path;		/* Endpoint object path */
 	char			*uuid;		/* Endpoint property UUID */
@@ -1007,9 +1006,6 @@ static void pac_config_cb(struct media_endpoint *endpoint, void *ret, int size,
 	struct pac_config_data *data = user_data;
 	gboolean *ret_value = ret;
 
-	if (ret_value)
-		endpoint->stream = data->stream;
-
 	data->cb(data->stream, ret_value ? 0 : -EINVAL);
 }
 
@@ -1089,11 +1085,21 @@ static int pac_config(struct bt_bap_stream *stream, struct iovec *cfg,
 static void pac_clear(struct bt_bap_stream *stream, void *user_data)
 {
 	struct media_endpoint *endpoint = user_data;
+	GSList *item;
 
-	endpoint->stream = NULL;
+	DBG("endpoint %p stream %p", endpoint, stream);
 
-	while (endpoint->transports != NULL)
-		clear_configuration(endpoint, endpoint->transports->data);
+	item = endpoint->transports;
+	while (item) {
+		struct media_transport *transport = item->data;
+
+		if (media_transport_get_stream(transport) == stream) {
+			clear_configuration(endpoint, transport);
+			item = endpoint->transports;
+		} else {
+			item = item->next;
+		}
+	}
 }
 
 static struct bt_bap_pac_ops pac_ops = {
diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index 5e057e2a5..b7aa5107b 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -1483,6 +1483,19 @@ const char *media_transport_get_path(struct media_transport *transport)
 	return transport->path;
 }
 
+void *media_transport_get_stream(struct media_transport *transport)
+{
+	struct bap_transport *bap;
+	const char *uuid;
+
+	uuid = media_endpoint_get_uuid(transport->endpoint);
+	if (strcasecmp(uuid, PAC_SINK_UUID) && strcasecmp(uuid, PAC_SOURCE_UUID))
+		return NULL;
+
+	bap = transport->data;
+	return bap->stream;
+}
+
 void media_transport_update_delay(struct media_transport *transport,
 							uint16_t delay)
 {
diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
index 102fc3cf1..5ca9b8f9e 100644
--- a/profiles/audio/transport.h
+++ b/profiles/audio/transport.h
@@ -19,6 +19,7 @@ struct media_transport *media_transport_create(struct btd_device *device,
 
 void media_transport_destroy(struct media_transport *transport);
 const char *media_transport_get_path(struct media_transport *transport);
+void *media_transport_get_stream(struct media_transport *transport);
 struct btd_device *media_transport_get_dev(struct media_transport *transport);
 int8_t media_transport_get_volume(struct media_transport *transport);
 void media_transport_update_delay(struct media_transport *transport,
-- 
2.39.1


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

* RE: [BlueZ,v3] media: clear the right transport when clearing BAP endpoint
  2023-02-12 14:00   ` [PATCH BlueZ v3] " Pauli Virtanen
@ 2023-02-12 15:21     ` bluez.test.bot
  0 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2023-02-12 15:21 UTC (permalink / raw)
  To: linux-bluetooth, pav

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

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=721072

---Test result---

Test Summary:
CheckPatch                    FAIL      0.76 seconds
GitLint                       FAIL      0.55 seconds
BuildEll                      PASS      32.68 seconds
BluezMake                     PASS      985.96 seconds
MakeCheck                     PASS      12.57 seconds
MakeDistcheck                 PASS      181.92 seconds
CheckValgrind                 PASS      297.79 seconds
CheckSmatch                   PASS      393.37 seconds
bluezmakeextell               PASS      119.15 seconds
IncrementalBuild              PASS      797.55 seconds
ScanBuild                     PASS      1244.97 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v3] media: clear the right transport when clearing BAP endpoint
WARNING:LONG_LINE: line length of 81 exceeds 80 columns
#138: FILE: profiles/audio/transport.c:1492:
+	if (strcasecmp(uuid, PAC_SINK_UUID) && strcasecmp(uuid, PAC_SOURCE_UUID))

/github/workspace/src/src/13137447.patch total: 0 errors, 1 warnings, 66 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.

/github/workspace/src/src/13137447.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

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


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,v3] media: clear the right transport when clearing BAP endpoint

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
16: B2 Line has trailing whitespace: "    "
20: B2 Line has trailing whitespace: "    "


---
Regards,
Linux Bluetooth


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

* Re: [PATCH BlueZ v2 1/2] shared/bap: fix local endpoint state to be per-client
  2023-02-11 19:55 [PATCH BlueZ v2 1/2] shared/bap: fix local endpoint state to be per-client Pauli Virtanen
  2023-02-11 19:55 ` [PATCH BlueZ v2 2/2] media: clear the right transport when clearing BAP endpoint Pauli Virtanen
  2023-02-11 21:38 ` [BlueZ,v2,1/2] shared/bap: fix local endpoint state to be per-client bluez.test.bot
@ 2023-02-13 20:30 ` patchwork-bot+bluetooth
  2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+bluetooth @ 2023-02-13 20:30 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

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



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

end of thread, other threads:[~2023-02-13 20:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-11 19:55 [PATCH BlueZ v2 1/2] shared/bap: fix local endpoint state to be per-client Pauli Virtanen
2023-02-11 19:55 ` [PATCH BlueZ v2 2/2] media: clear the right transport when clearing BAP endpoint Pauli Virtanen
2023-02-12 14:00   ` [PATCH BlueZ v3] " Pauli Virtanen
2023-02-12 15:21     ` [BlueZ,v3] " bluez.test.bot
2023-02-11 21:38 ` [BlueZ,v2,1/2] shared/bap: fix local endpoint state to be per-client bluez.test.bot
2023-02-13 20:30 ` [PATCH BlueZ v2 1/2] " patchwork-bot+bluetooth

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