[PATCH BlueZ v1 1/4] profiles/audio/bass: Use BASS_Modify_Source when assistant is active

[PATCH BlueZ v1 1/4] profiles/audio/bass: Use BASS_Modify_Source when assistant is active

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

When a MediaAssistant is already active (streaming), a subsequent Push
should send Modify Source with PA_SYNC_NO_SYNC and bis_sync=0 to
remove the broadcast source, rather than adding a duplicate source.

Split push() into push_add_src() and push_mod_src() helpers. The push
dispatcher checks the assistant state: ACTIVE non-local assistants and
LOCAL assistants with a tracked source entry both route to
push_mod_src, while all others proceed with push_add_src.

Add per-device source tracking for LOCAL assistants via a bass_src
struct (keyed by bt_bass pointer) stored in a per-assistant srcs
queue. This is needed because LOCAL assistants never change state, so
we cannot rely on the state machine alone to detect active sources.

Also fix the bass_src_changed BID filter: the previous check rejected
non-local assistants when BID was zero, but this incorrectly excluded
LOCAL assistants in REQUESTING state whose BID happens to be non-zero.
Change to reject assistants whose own BID is non-zero when the
notification BID is zero, which correctly matches local streams.
---
 profiles/audio/bass.c | 176 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 150 insertions(+), 26 deletions(-)

diff --git a/profiles/audio/bass.c b/profiles/audio/bass.c
index 1fd7704a77a2..f018f8b3ad7f 100644
--- a/profiles/audio/bass.c
+++ b/profiles/audio/bass.c
@@ -108,9 +108,16 @@ struct bass_assistant {
 	struct iovec *meta;
 	struct iovec *caps;
 	enum assistant_state state;
+	uint8_t src_id;
+	struct queue *srcs;		/* Per-device source tracking */
 	char *path;
 };
 
+struct bass_src {
+	struct bt_bass *bass;
+	uint8_t src_id;
+};
+
 struct bass_delegator {
 	struct btd_device *device;	/* Broadcast source device */
 	struct btd_service *service;
@@ -158,6 +165,17 @@ static void bass_data_remove(struct bass_data *data);
 static void bis_probe(uint8_t sid, uint8_t bis, uint8_t sgrp,
 			struct iovec *caps, struct iovec *meta,
 			struct bt_bap_qos *qos, void *user_data);
+
+static bool match_src_data(const void *data, const void *match_data)
+{
+	const struct bass_src *src = data;
+	const struct bt_bass *bass = match_data;
+
+	return src->bass == bass;
+}
+
+static struct bass_src *assistant_add_src(struct bass_assistant *assistant,
+					struct bt_bass *bass, uint8_t id);
 static void bis_remove(struct bt_bap *bap, void *user_data);
 
 
@@ -1010,35 +1028,62 @@ static void assistant_past(struct bass_assistant *assistant)
 	free(addr);
 }
 
-static DBusMessage *push(DBusConnection *conn, DBusMessage *msg,
-							  void *user_data)
+static DBusMessage *push_mod_src(struct bass_assistant *assistant,
+					DBusMessage *msg)
+{
+	struct bt_bass_bcast_audio_scan_cp_hdr hdr;
+	struct bt_bass_mod_src_params params = {0};
+	struct iovec iov = {0};
+	uint32_t bis_sync = 0;
+	uint8_t meta_len = 0;
+	int err;
+
+	hdr.op = BT_BASS_MOD_SRC;
+
+	params.id = assistant->src_id;
+	params.pa_sync = PA_SYNC_NO_SYNC;
+	params.pa_interval = PA_INTERVAL_UNKNOWN;
+	params.num_subgroups = assistant->sgrp + 1;
+
+	util_iov_append(&iov, &params, sizeof(params));
+
+	for (uint8_t sgrp = 0; sgrp < params.num_subgroups; sgrp++) {
+		util_iov_append(&iov, &bis_sync, sizeof(bis_sync));
+		util_iov_append(&iov, &meta_len, sizeof(meta_len));
+	}
+
+	err = bt_bass_send(assistant->data->bass, &hdr, &iov);
+	if (err) {
+		DBG("Unable to send BASS Write Command");
+		return btd_error_failed(msg, strerror(-err));
+	}
+
+	free(iov.iov_base);
+
+	if (assistant->device) {
+		assistant_set_state(assistant, ASSISTANT_STATE_IDLE);
+	} else {
+		struct bass_src *src;
+
+		src = queue_remove_if(assistant->srcs, match_src_data,
+						assistant->data->bass);
+		free(src);
+	}
+
+	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
+}
+
+static DBusMessage *push_add_src(struct bass_assistant *assistant,
+					DBusMessage *msg)
 {
-	struct bass_assistant *assistant = user_data;
 	struct bt_bass_bcast_audio_scan_cp_hdr hdr;
 	struct bt_bass_add_src_params params = {0};
 	struct iovec iov = {0};
 	uint32_t bis_sync = 0;
 	uint8_t meta_len = 0;
 	int err;
-	DBusMessageIter props, dict;
 	struct io *io;
 
-	DBG("");
-
-	dbus_message_iter_init(msg, &props);
-
-	if (dbus_message_iter_get_arg_type(&props) != DBUS_TYPE_ARRAY) {
-		DBG("Unable to parse properties");
-		return btd_error_invalid_args(msg);
-	}
-
-	dbus_message_iter_recurse(&props, &dict);
-
-	if (assistant_parse_props(assistant, &dict)) {
-		DBG("Unable to parse properties");
-		return btd_error_invalid_args(msg);
-	}
-
 	hdr.op = BT_BASS_ADD_SRC;
 
 	if (assistant->device) {
@@ -1130,11 +1175,52 @@ static DBusMessage *push(DBusConnection *conn, DBusMessage *msg,
 
 	free(iov.iov_base);
 
-	assistant_set_state(assistant, ASSISTANT_STATE_PENDING);
+	if (assistant->state == ASSISTANT_STATE_LOCAL)
+		assistant_add_src(assistant, assistant->data->bass, 0);
+	else
+		assistant_set_state(assistant, ASSISTANT_STATE_PENDING);
 
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
+static DBusMessage *push(DBusConnection *conn, DBusMessage *msg,
+							  void *user_data)
+{
+	struct bass_assistant *assistant = user_data;
+	DBusMessageIter props, dict;
+
+	DBG("");
+
+	dbus_message_iter_init(msg, &props);
+
+	if (dbus_message_iter_get_arg_type(&props) != DBUS_TYPE_ARRAY) {
+		DBG("Unable to parse properties");
+		return btd_error_invalid_args(msg);
+	}
+
+	dbus_message_iter_recurse(&props, &dict);
+
+	if (assistant_parse_props(assistant, &dict)) {
+		DBG("Unable to parse properties");
+		return btd_error_invalid_args(msg);
+	}
+
+	if (assistant->state == ASSISTANT_STATE_ACTIVE) {
+		return push_mod_src(assistant, msg);
+	} else if (assistant->state == ASSISTANT_STATE_LOCAL) {
+		struct bass_src *src;
+
+		src = queue_find(assistant->srcs, match_src_data,
+						assistant->data->bass);
+		if (src) {
+			assistant->src_id = src->src_id;
+			return push_mod_src(assistant, msg);
+		}
+	}
+
+	return push_add_src(assistant, msg);
+}
+
 static const GDBusMethodTable assistant_methods[] = {
 	{GDBUS_EXPERIMENTAL_ASYNC_METHOD("Push",
 					GDBUS_ARGS({ "Props", "a{sv}" }),
@@ -1235,6 +1321,7 @@ static void assistant_free(void *data)
 	g_free(assistant->path);
 	util_iov_free(assistant->meta, 1);
 	util_iov_free(assistant->caps, 1);
+	queue_destroy(assistant->srcs, free);
 
 	free(assistant);
 }
@@ -2015,9 +2102,33 @@ static void bass_handle_bcode_req(struct bass_assistant *assistant, int id)
 	free(iov.iov_base);
 }
 
+static struct bass_src *assistant_add_src(struct bass_assistant *assistant,
+					struct bt_bass *bass, uint8_t id)
+{
+	struct bass_src *src;
+
+	if (!assistant->srcs)
+		assistant->srcs = queue_new();
+
+	src = queue_find(assistant->srcs, match_src_data, bass);
+	if (src) {
+		src->src_id = id;
+		return src;
+	}
+
+	src = new0(struct bass_src, 1);
+	src->bass = bass;
+	src->src_id = id;
+
+	queue_push_tail(assistant->srcs, src);
+
+	return src;
+}
+
 static void bass_src_changed(uint8_t id, uint32_t bid, uint8_t state,
 				uint8_t enc, uint32_t bis_sync, void *user_data)
 {
+	struct bass_data *data = user_data;
 	const struct queue_entry *entry;
 
 	for (entry = queue_get_entries(assistants); entry;
@@ -2032,9 +2143,9 @@ static void bass_src_changed(uint8_t id, uint32_t bid, uint8_t state,
 			continue;
 
 		/* If BID is not set it may happen to be local stream so ignore
-		 * non-local assistants.
+		 * non-local assistants unless their BID is also not set.
 		 */
-		if (!bid && assistant->state != ASSISTANT_STATE_LOCAL)
+		if (!bid && assistant->bid)
 			continue;
 
 		if (state == BT_BASS_SYNC_INFO_RE) {
@@ -2066,21 +2177,34 @@ static void bass_src_changed(uint8_t id, uint32_t bid, uint8_t state,
 				break;
 
 			/* Match BIS index */
-			if (bis & bis_sync)
+			if (bis & bis_sync) {
+				assistant->src_id = id;
 				assistant_set_state(assistant,
 						ASSISTANT_STATE_ACTIVE);
+			}
 			break;
 		case BT_BASS_BIG_ENC_STATE_DEC:
 			/* Only handle assistant objects that
 			 * have requested a Broadcast Code
 			 */
-			if (assistant->state != ASSISTANT_STATE_REQUESTING)
+			if (assistant->state != ASSISTANT_STATE_REQUESTING &&
+				assistant->state != ASSISTANT_STATE_LOCAL)
 				break;
 
 			/* Match BIS index */
-			if (bis & bis_sync)
+			if (bis & bis_sync) {
+				assistant->src_id = id;
+
+				if (assistant->state ==
+						ASSISTANT_STATE_LOCAL) {
+					assistant_add_src(assistant, data->bass,
+								id);
+					break;
+				}
+
 				assistant_set_state(assistant,
 						ASSISTANT_STATE_ACTIVE);
+			}
 			break;
 		default:
 			continue;
-- 
2.54.0

[PATCH BlueZ v1 2/4] profiles/audio/bass: Handle PA_SYNC_NO_SYNC in handle_mod_src_req

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

When the delegator receives a Modify Source operation with pa_sync set
to PA_SYNC_NO_SYNC while already synchronized to PA, release all
setups instead of updating BIS sync which would not apply the
requested termination.

Also reset the encryption state to no encryption when the last BIS
index is cleared from a subgroup, so subsequent source additions
start with a clean encryption state.
---
 profiles/audio/bass.c | 72 ++++++++++++++++++++++++++++++++++++++++---
 src/shared/bass.c     |  6 ++++
 2 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/profiles/audio/bass.c b/profiles/audio/bass.c
index f018f8b3ad7f..5eb7bff076bb 100644
--- a/profiles/audio/bass.c
+++ b/profiles/audio/bass.c
@@ -420,7 +420,13 @@ static void setup_clear(struct bass_setup *setup, int bis)
 
 	bt_bass_clear_bis_sync(dg->src, bis);
 	setup->stream = NULL;
-	queue_remove(setup->dg->setups, setup);
+
+	if (!queue_remove(setup->dg->setups, setup))
+		/* Setup has already been removed from the queue (e.g. during
+		 * handle_mod_src_req PA_SYNC_NO_SYNC processing). Skip
+		 * disconnect check and free since the caller handles cleanup.
+		 */
+		return;
 
 	/* Remove any pending bcode request associated with setup */
 	req = queue_remove_if(dg->bcode_reqs, match_bcode_setup, setup);
@@ -596,6 +602,8 @@ static void bass_remove_bis(struct bass_setup *setup)
 {
 	struct queue *links = bt_bap_stream_io_get_links(setup->stream);
 
+	DBG("%p", setup);
+
 	queue_foreach(links, stream_unlink, setup->stream);
 	bt_bap_stream_release(setup->stream, NULL, NULL);
 }
@@ -618,6 +626,8 @@ static void setup_disable_streaming(void *data, void *user_data)
 
 static void bass_add_bis(struct bass_setup *setup)
 {
+	DBG("%p", setup);
+
 	queue_foreach(setup->dg->setups, setup_disable_streaming, NULL);
 	setup_configure_stream(setup);
 }
@@ -1941,8 +1951,40 @@ static int handle_set_bcode_req(struct bt_bcast_src *bcast_src,
 	return 0;
 }
 
+static bool check_bis_sync(struct bt_bass_mod_src_params *params, uint8_t bis)
+{
+	uint32_t bitmask = 1 << (bis - 1);
+	struct iovec iov = {
+		.iov_base = params->subgroup_data,
+		.iov_len = 0,
+	};
+
+	/* Calculate subgroup data length based on each subgroup's
+	 * bis_sync (4 bytes) + meta_len (1 byte) + meta fields.
+	 */
+	for (uint8_t i = 0; i < params->num_subgroups; i++) {
+		uint32_t bis_sync;
+		uint8_t meta_len;
+
+		iov.iov_len += sizeof(bis_sync) + sizeof(meta_len);
+
+		memcpy(&bis_sync, params->subgroup_data + iov.iov_len -
+				sizeof(bis_sync) - sizeof(meta_len),
+				sizeof(bis_sync));
+		memcpy(&meta_len, params->subgroup_data + iov.iov_len -
+				sizeof(meta_len), sizeof(meta_len));
+
+		if (le32_to_cpu(bis_sync) & bitmask)
+			return true;
+
+		iov.iov_len += meta_len;
+	}
+
+	return false;
+}
+
 static void bass_update_bis_sync(struct bass_delegator *dg,
-				struct bt_bcast_src *bcast_src)
+				struct bt_bass_mod_src_params *params)
 {
 	const struct queue_entry *entry;
 
@@ -1954,11 +1996,14 @@ static void bass_update_bis_sync(struct bass_delegator *dg,
 
 		state = bt_bap_stream_get_state(setup->stream);
 
-		if (!setup->stream && bt_bass_check_bis(bcast_src, setup->bis))
+		DBG("stream %p: BIS %d state %s(%u)", setup->stream, setup->bis,
+				bt_bap_stream_statestr(state), state);
+
+		if (!setup->stream && check_bis_sync(params, setup->bis))
 			bass_add_bis(setup);
 		else if (setup->stream &&
 				state == BT_BAP_STREAM_STATE_STREAMING &&
-				!bt_bass_check_bis(bcast_src, setup->bis))
+				!check_bis_sync(params, setup->bis))
 			bass_remove_bis(setup);
 	}
 }
@@ -1981,9 +2026,26 @@ static int handle_mod_src_req(struct bt_bcast_src *bcast_src,
 	if (err)
 		return err;
 
+	DBG("PA sync state %d", sync_state);
+
 	switch (sync_state) {
 	case BT_BASS_SYNCHRONIZED_TO_PA:
-		bass_update_bis_sync(dg, bcast_src);
+		if (params->pa_sync == PA_SYNC_NO_SYNC) {
+			/* Release all setups. Note: bass_remove_bis may
+			 * trigger synchronous state transitions that call
+			 * setup_clear which will return early since the
+			 * setup has already been removed from the queue.
+			 */
+			struct bass_setup *setup;
+
+			while ((setup = queue_pop_head(dg->setups))) {
+				bass_remove_bis(setup);
+				setup_free(setup);
+			}
+
+			delegator_disconnect(dg);
+		} else
+			bass_update_bis_sync(dg, params);
 		break;
 	case BT_BASS_NOT_SYNCHRONIZED_TO_PA:
 		if (params->pa_sync == PA_SYNC_NO_PAST) {
diff --git a/src/shared/bass.c b/src/shared/bass.c
index 19cc9531d617..65ee4a8cbb18 100644
--- a/src/shared/bass.c
+++ b/src/shared/bass.c
@@ -1930,6 +1930,12 @@ int bt_bass_clear_bis_sync(struct bt_bcast_src *bcast_src, uint8_t bis)
 		if (sgrp->bis_sync & bitmask) {
 			sgrp->bis_sync &= ~bitmask;
 
+			/* If there are no BIS index left then reset encryption
+			 * state to no encryption.
+			 */
+			if (!sgrp->bis_sync)
+				bcast_src->enc = BT_BASS_BIG_ENC_STATE_NO_ENC;
+
 			iov = bass_parse_bcast_src(bcast_src);
 			if (!iov)
 				return -ENOMEM;
-- 
2.54.0

[PATCH BlueZ v1 3/4] profiles/audio/bass: Fix delegator reprobing after disconnect

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

delegator_disconnect left the old delegator in the delegators queue
after calling device_remove_profile. When a new delegator was created
for the same device (e.g. on a subsequent Add Source after Modify
Source removal), delegator_attach would find the stale entry and
return early, leaving the new delegator without a service. This
caused the "Unable to probe service" error and prevented the BASS
bcode exchange from working on reconnection.

Fix by removing the delegator from the queue, clearing service user
data, and calling delegator_free before disconnecting the service
and removing the profile. This also fixes a potential use-after-free
where delegator_disconnect accessed dg fields after device_remove_profile
could trigger delegator_detach -> delegator_free through the
bap_detached callback chain.

Also add validation in bass_req_bcode to detect and discard invalid
(all-zeros) broadcast codes that may have been stored from a previous
connection.
---
 profiles/audio/bass.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/profiles/audio/bass.c b/profiles/audio/bass.c
index 5eb7bff076bb..a5ef80fbc835 100644
--- a/profiles/audio/bass.c
+++ b/profiles/audio/bass.c
@@ -257,9 +257,17 @@ static void bass_req_bcode(struct bt_bap_stream *stream,
 	}
 
 	if (dg->bcode) {
-		/* Broadcast Code has already been received before. */
-		setup_set_bcode(dg->bcode, setup, reply, reply_data);
-		return;
+		uint8_t empty_bcode[BT_BASS_BCAST_CODE_SIZE] = {0};
+
+		if (memcmp(dg->bcode, empty_bcode, BT_BASS_BCAST_CODE_SIZE)) {
+			/* Broadcast Code has already been received before. */
+			setup_set_bcode(dg->bcode, setup, reply, reply_data);
+			return;
+		}
+
+		error("dg %p has invalid bcode", dg);
+		free(dg->bcode);
+		dg->bcode = NULL;
 	}
 
 	/* Create a request for the Broadcast Code. The request
@@ -376,20 +384,37 @@ static void setup_free(void *data)
 	free(setup);
 }
 
+static void delegator_free(struct bass_delegator *dg);
+
 static void delegator_disconnect(struct bass_delegator *dg)
 {
 	struct btd_device *device = dg->device;
 	struct btd_service *service = dg->service;
+	struct btd_profile *profile = btd_service_get_profile(service);
 
 	DBG("%p", dg);
 
+	/* Remove delegator from the queue and free it before removing the
+	 * service profile. This prevents delegator_attach from finding a
+	 * stale entry when a new delegator is created for the same device,
+	 * and also avoids use-after-free since device_remove_profile may
+	 * trigger bap_detached -> delegator_detach which would otherwise
+	 * try to free the delegator again.
+	 */
+	queue_remove(delegators, dg);
+	btd_service_set_user_data(service, NULL);
+	delegator_free(dg);
+
 	/* Disconnect service so BAP driver is cleanup properly and bt_bap is
 	 * detached from the device.
 	 */
 	btd_service_disconnect(service);
 
-	/* Remove service since delegator shold have been freed at this point */
-	device_remove_profile(device, btd_service_get_profile(service));
+	/* Remove service since delegator has been freed at this point.
+	 * delegator_detach (triggered by bap_detached) will be a no-op
+	 * since the delegator has already been removed from the queue.
+	 */
+	device_remove_profile(device, profile);
 
 	/* If the device is no longer consider connected  it means no other
 	 * service was connected so it has no longer any use and can be safely
-- 
2.54.0

[PATCH BlueZ v1 4/4] doc: Document Push behavior when MediaAssistant is active

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Document that calling Push on an assistant already in active state uses
BASS_Modify_Source to update the existing source rather than adding a
new one.
---
 doc/org.bluez.MediaAssistant.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/doc/org.bluez.MediaAssistant.rst b/doc/org.bluez.MediaAssistant.rst
index 45143cee8dbf..2f29a7b5c253 100644
--- a/doc/org.bluez.MediaAssistant.rst
+++ b/doc/org.bluez.MediaAssistant.rst
@@ -27,6 +27,10 @@ void Push(dict properties)
 
 Send stream information to the remote device.
 
+If the assistant is in the "active" state, this method uses
+BASS_Modify_Source to update the existing source on the remote device
+instead of adding a new one with BASS_Add_Source.
+
 :dict properties:
 
 Indicate stream properties that will be sent to the peer.
-- 
2.54.0

RE: [BlueZ,v1,1/4] profiles/audio/bass: Use BASS_Modify_Source when assistant is active

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      1.75 seconds
GitLint                       FAIL      1.01 seconds
BuildEll                      PASS      20.78 seconds
BluezMake                     PASS      686.70 seconds
MakeCheck                     PASS      18.37 seconds
MakeDistcheck                 PASS      252.36 seconds
CheckValgrind                 PASS      298.96 seconds
CheckSmatch                   PASS      352.74 seconds
bluezmakeextell               PASS      185.30 seconds
IncrementalBuild              PASS      687.41 seconds
ScanBuild                     PASS      1066.94 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,v1,1/4] profiles/audio/bass: Use BASS_Modify_Source when assistant is active

1: T1 Title exceeds max length (83>80): "[BlueZ,v1,1/4] profiles/audio/bass: Use BASS_Modify_Source when assistant is active"


https://github.com/bluez/bluez/pull/2168

---
Regards,
Linux Bluetooth