Skip to contentSkip to conversation

shared/bap: Add unespecified bit in audio context to PAC records

PATCH In review Luiz Augusto von Dentz 3 participants 7 messages Jun 19, 2023 – Jun 20, 2023 Bluetooth drivers

lore
Share & tools
lore

Luiz Augusto von Dentz

author maintainer
#0 lore raw

PATCH 1/5 shared/bap: Add unespecified bit in audio context to PAC records

From: Luiz Augusto von Dentz <[email protected]>

This makes sure unespecified bit is properly marked in both audio contexts since that required by many platforms in order to work properly and while doing that add proper defines to the defaults values used in PACS.

---
 src/shared/bap.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
src/shared/bap.c +14 -6
index 4b31536ee..270f0fd64 100644
@@ -47,6 +47,14 @@
 
 #define BAP_PROCESS_TIMEOUT 10
 
+#define PACS_SRC_LOCATION 0x00000001
+#define PACS_SNK_LOCATION 0x00000003
+
+#define PACS_SRC_CTXT 0x000f
+#define PACS_SUPPORTED_SRC_CTXT PACS_SRC_CTXT
+#define PACS_SNK_CTXT 0x0fff
+#define PACS_SUPPORTED_SNK_CTXT PACS_SNK_CTXT
+
 struct bt_bap_pac_changed {
 	unsigned int id;
 	bt_bap_pac_func_t added;

@@ -467,12 +475,12 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
 	pacs = new0(struct bt_pacs, 1);
 
 	/* Set default values */
-	pacs->sink_loc_value = 0x00000003;
-	pacs->source_loc_value = 0x00000001;
-	pacs->sink_context_value = 0x0fff;
-	pacs->source_context_value = 0x000e;
-	pacs->supported_sink_context_value = 0x0fff;
-	pacs->supported_source_context_value = 0x000e;
+	pacs->sink_loc_value = PACS_SNK_LOCATION;
+	pacs->source_loc_value = PACS_SRC_LOCATION;
+	pacs->sink_context_value = PACS_SNK_CTXT;
+	pacs->source_context_value = PACS_SRC_CTXT;
+	pacs->supported_sink_context_value = PACS_SUPPORTED_SNK_CTXT;
+	pacs->supported_source_context_value = PACS_SUPPORTED_SRC_CTXT;
 
 	/* Populate DB with PACS attributes */
 	bt_uuid16_create(&uuid, PACS_UUID);
Signature · 1 line
2.40.1
author maintainer
#1 lore raw

PATCH 2/5 client: Print integers decimal value

From: Luiz Augusto von Dentz <[email protected]>

This make the code print D-Bus integer iterators also in decimal format in addition to hexadecimal.

---
 client/print.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
client/print.c +6 -3
index 798d22c7e..66439e541 100644
@@ -127,15 +127,18 @@ void print_iter(const char *label, const char *name, DBusMessageIter *iter)
 		break;
 	case DBUS_TYPE_UINT32:
 		dbus_message_iter_get_basic(iter, &valu32);
-		bt_shell_printf("%s%s: 0x%08x\n", label, name, valu32);
+		bt_shell_printf("%s%s: 0x%08x (%d)\n", label, name, valu32,
+								valu32);
 		break;
 	case DBUS_TYPE_UINT16:
 		dbus_message_iter_get_basic(iter, &valu16);
-		bt_shell_printf("%s%s: 0x%04x\n", label, name, valu16);
+		bt_shell_printf("%s%s: 0x%04x (%d)\n", label, name, valu16,
+								valu16);
 		break;
 	case DBUS_TYPE_INT16:
 		dbus_message_iter_get_basic(iter, &vals16);
-		bt_shell_printf("%s%s: %d\n", label, name, vals16);
+		bt_shell_printf("%s%s: 0x%04x (%d)\n", label, name, vals16,
+								vals16);
 		break;
 	case DBUS_TYPE_BYTE:
 		dbus_message_iter_get_basic(iter, &byte);
Signature · 1 line
2.40.1
author maintainer
#2 lore raw

PATCH 3/5 client/player: Fix auto registration of broadcast endpoint

In reply to Luiz Augusto von Dentz

From: Luiz Augusto von Dentz <[email protected]>

For broadcast endpoint broadcast must be set properly.

---
 client/player.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)
client/player.c +17 -16
index 389fc1d07..a399d82ec 100644
@@ -1740,22 +1740,22 @@ struct endpoint_config {
 				0xa2, 0x65, 0xbb, 0xaf, 0xc6, 0xea, 0x03, 0xb8}
 
 static struct bt_iso_qos bcast_qos = {
-		.bcast = {
-			.big = BT_ISO_QOS_BIG_UNSET,
-			.bis = BT_ISO_QOS_BIS_UNSET,
-			.sync_interval = 0x07,
-			.packing = 0x00,
-			.framing = 0x00,
-			.encryption = 0x00,
-			.bcode = BCODE,
-			.options = 0x00,
-			.skip = 0x0000,
-			.sync_timeout = 0x4000,
-			.sync_cte_type = 0x00,
-			.mse = 0x00,
-			.timeout = 0x4000,
-		}
-	};
+	.bcast = {
+		.big = BT_ISO_QOS_BIG_UNSET,
+		.bis = BT_ISO_QOS_BIS_UNSET,
+		.sync_interval = 24,
+		.packing = 0x00,
+		.framing = 0x00,
+		.encryption = 0x00,
+		.bcode = BCODE,
+		.options = 0x00,
+		.skip = 0x0000,
+		.sync_timeout = 0x4000,
+		.sync_cte_type = 0x00,
+		.mse = 0x00,
+		.timeout = 0x4000,
+	}
+};
 
 static void append_properties(DBusMessageIter *iter,
 						struct endpoint_config *cfg)

@@ -3175,6 +3175,7 @@ static struct endpoint *endpoint_new(const struct capabilities *cap)
 
 	ep = new0(struct endpoint, 1);
 	ep->uuid = g_strdup(cap->uuid);
+	ep->broadcast = strcmp(cap->uuid, BAA_SERVICE_UUID) ? false : true;
 	ep->codec = cap->codec_id;
 	ep->path = g_strdup_printf("%s/ep%u", BLUEZ_MEDIA_ENDPOINT_PATH,
 					g_list_length(local_endpoints));
Signature · 1 line
2.40.1
author maintainer
#3 lore raw

PATCH 4/5 client/player: Fix transport.acquire for linked transports

In reply to Luiz Augusto von Dentz

From: Luiz Augusto von Dentz <[email protected]>

Linked (bi-directional) transports can be acquired on single D-Bus method call which was not being handled properly by the current code causing unexpected errors.

---
 client/player.c | 202 +++++++++++++++++++++++++-----------------------
 1 file changed, 107 insertions(+), 95 deletions(-)
client/player.c +107 -95
index a399d82ec..d8d632f84 100644
@@ -72,11 +72,11 @@ struct endpoint {
 	struct iovec *caps;
 	struct iovec *meta;
 	bool auto_accept;
-	bool acquiring;
 	uint8_t max_transports;
 	uint8_t iso_group;
 	uint8_t iso_stream;
-	char *transport;
+	struct queue *acquiring;
+	struct queue *transports;
 	DBusMessage *msg;
 	struct preset *preset;
 	bool broadcast;

@@ -1075,8 +1075,6 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn,
 	bt_shell_printf("\tTransport %s\n", path);
 	print_iter("\t", "Properties", &props);
 
-	free(ep->transport);
-
 	if (!ep->max_transports) {
 		bt_shell_printf("Maximum transports reached: rejecting\n");
 		return g_dbus_create_error(msg,

@@ -1086,7 +1084,10 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn,
 
 	ep->max_transports--;
 
-	ep->transport = strdup(path);
+	if (!ep->transports)
+		ep->transports = queue_new();
+
+	queue_push_tail(ep->transports, strdup(path));
 
 	if (ep->auto_accept) {
 		bt_shell_printf("Auto Accepting...\n");

@@ -2030,16 +2031,26 @@ static DBusMessage *endpoint_select_properties(DBusConnection *conn,
 	return reply;
 }
 
+static bool match_str(const void *data, const void *user_data)
+{
+	return !strcmp(data, user_data);
+}
+
 static DBusMessage *endpoint_clear_configuration(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
 	struct endpoint *ep = user_data;
+	DBusMessageIter args;
+	const char *path;
+
+	dbus_message_iter_init(msg, &args);
+
+	dbus_message_iter_get_basic(&args, &path);
 
 	if (ep->max_transports != UINT8_MAX)
 		ep->max_transports++;
 
-	free(ep->transport);
-	ep->transport = NULL;
+	queue_remove_if(ep->transports, match_str, (void *)path);
 
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }

@@ -2128,6 +2139,9 @@ static void endpoint_free(void *data)
 		free(ep->preset);
 	}
 
+	queue_destroy(ep->acquiring, NULL);
+	queue_destroy(ep->transports, free);
+
 	g_free(ep->path);
 	g_free(ep->uuid);
 	g_free(ep);

@@ -3431,14 +3445,14 @@ static struct endpoint *find_ep_by_transport(const char *path)
 	for (l = local_endpoints; l; l = g_list_next(l)) {
 		struct endpoint *ep = l->data;
 
-		if (ep->transport && !strcmp(ep->transport, path))
+		if (queue_find(ep->transports, match_str, path))
 			return ep;
 	}
 
 	return NULL;
 }
 
-static struct endpoint *find_link_by_proxy(GDBusProxy *proxy)
+static GDBusProxy *find_link_by_proxy(GDBusProxy *proxy)
 {
 	DBusMessageIter iter, array;
 

@@ -3450,13 +3464,13 @@ static struct endpoint *find_link_by_proxy(GDBusProxy *proxy)
 	while (dbus_message_iter_get_arg_type(&array) ==
 				DBUS_TYPE_OBJECT_PATH) {
 		const char *transport;
-		struct endpoint *link;
 
 		dbus_message_iter_get_basic(&array, &transport);
 
-		link = find_ep_by_transport(transport);
-		if (link)
-			return link;
+		proxy = g_dbus_proxy_lookup(transports, NULL, transport,
+					BLUEZ_MEDIA_TRANSPORT_INTERFACE);
+		if (proxy)
+			return proxy;
 	}
 
 	return NULL;

@@ -3543,21 +3557,49 @@ static void transport_new(GDBusProxy *proxy, int sk, uint16_t mtu[2])
 	queue_push_tail(ios, transport);
 }
 
+static void ep_set_acquiring(struct endpoint *ep, GDBusProxy *proxy, bool value)
+{
+	bt_shell_printf("Transport %s %s\n", g_dbus_proxy_get_path(proxy),
+			value ? "acquiring" : "acquiring complete");
+
+	if (value && !ep->acquiring)
+		ep->acquiring = queue_new();
+
+	if (value)
+		queue_push_tail(ep->acquiring, proxy);
+	else
+		queue_remove(ep->acquiring, proxy);
+}
+
+static void transport_set_acquiring(GDBusProxy *proxy, bool value)
+{
+	struct endpoint *ep;
+	GDBusProxy *link;
+
+	ep = find_ep_by_transport(g_dbus_proxy_get_path(proxy));
+	if (!ep)
+		return;
+
+	ep_set_acquiring(ep, proxy, value);
+
+	link = find_link_by_proxy(proxy);
+	if (link) {
+		ep = find_ep_by_transport(g_dbus_proxy_get_path(link));
+		if (!ep)
+			return;
+
+		ep_set_acquiring(ep, link, value);
+	}
+}
+
 static void acquire_reply(DBusMessage *message, void *user_data)
 {
 	GDBusProxy *proxy = user_data;
-	struct endpoint *ep, *link;
 	DBusError error;
 	int sk;
 	uint16_t mtu[2];
 
-	ep = find_ep_by_transport(g_dbus_proxy_get_path(proxy));
-	if (ep) {
-		ep->acquiring = false;
-		link = find_link_by_proxy(proxy);
-		if (link)
-			link->acquiring = false;
-	}
+	transport_set_acquiring(proxy, false);
 
 	dbus_error_init(&error);
 

@@ -3586,33 +3628,61 @@ static void acquire_reply(DBusMessage *message, void *user_data)
 	return bt_shell_noninteractive_quit(EXIT_FAILURE);
 }
 
-static void transport_acquire(const char *input, void *user_data)
+static void prompt_acquire(const char *input, void *user_data)
 {
 	GDBusProxy *proxy = user_data;
-	struct endpoint *ep, *link;
 
 	if (!strcasecmp(input, "y") || !strcasecmp(input, "yes")) {
 		if (g_dbus_proxy_method_call(proxy, "Acquire", NULL,
-						acquire_reply, proxy, NULL))
+						acquire_reply, proxy, NULL)) {
+			transport_set_acquiring(proxy, true);
 			return;
+		}
 		bt_shell_printf("Failed acquire transport\n");
 	}
+}
 
-	/* Reset acquiring */
+static void transport_acquire(GDBusProxy *proxy, bool prompt)
+{
+	struct endpoint *ep;
+	GDBusProxy *link;
+
+	/* only attempt to acquire if transport is configured with a local
+	 * endpoint.
+	 */
 	ep = find_ep_by_transport(g_dbus_proxy_get_path(proxy));
-	if (ep) {
-		ep->acquiring = false;
-		link = find_link_by_proxy(proxy);
-		if (link)
-			link->acquiring = false;
+	if (!ep || queue_find(ep->acquiring, NULL, proxy))
+		return;
+
+	link = find_link_by_proxy(proxy);
+	if (link) {
+		ep = find_ep_by_transport(g_dbus_proxy_get_path(link));
+		/* if link already acquiring wait it to be complete */
+		if (!ep && queue_find(ep->acquiring, NULL, link))
+			return;
+	}
+
+	if (ep->auto_accept || !prompt) {
+		if (!prompt)
+			bt_shell_printf("auto acquiring...\n");
+		if (!g_dbus_proxy_method_call(proxy, "Acquire", NULL,
+						acquire_reply, proxy, NULL)) {
+			bt_shell_printf("failed acquire transport\n");
+			return;
+		}
+
+		transport_set_acquiring(proxy, true);
+		return;
 	}
+
+	bt_shell_prompt_input(g_dbus_proxy_get_path(proxy), "acquire (yes/no):",
+					prompt_acquire, proxy);
 }
 
 static void transport_property_changed(GDBusProxy *proxy, const char *name,
 						DBusMessageIter *iter)
 {
 	char *str;
-	struct endpoint *ep, *link;
 
 	str = proxy_description(proxy, "Transport", COLORED_CHG);
 	print_iter(str, name, iter);

@@ -3626,38 +3696,7 @@ static void transport_property_changed(GDBusProxy *proxy, const char *name,
 	if (strcmp(str, "pending"))
 		return;
 
-	/* Only attempt to acquire if transport is configured with a local
-	 * endpoint.
-	 */
-	ep = find_ep_by_transport(g_dbus_proxy_get_path(proxy));
-	if (!ep || ep->acquiring)
-		return;
-
-	ep->acquiring = true;
-
-	link = find_link_by_proxy(proxy);
-	if (link) {
-		bt_shell_printf("Link %s found\n", link->transport);
-		/* If link already acquiring wait it to be complete */
-		if (link->acquiring)
-			return;
-		link->acquiring = true;
-	}
-
-	if (ep->auto_accept) {
-		bt_shell_printf("Auto Acquiring...\n");
-		if (!g_dbus_proxy_method_call(proxy, "Acquire", NULL,
-						acquire_reply, proxy, NULL)) {
-			bt_shell_printf("Failed acquire transport\n");
-			ep->acquiring = false;
-			if (link)
-				link->acquiring = false;
-		}
-		return;
-	}
-
-	bt_shell_prompt_input(g_dbus_proxy_get_path(proxy), "Acquire (yes/no):",
-					transport_acquire, proxy);
+	transport_acquire(proxy, true);
 }
 
 static void property_changed(GDBusProxy *proxy, const char *name,

@@ -3747,7 +3786,6 @@ static void cmd_acquire_transport(int argc, char *argv[])
 {
 	GDBusProxy *proxy;
 	int i;
-	struct endpoint *ep, *link;
 
 	for (i = 1; i < argc; i++) {
 		proxy = g_dbus_proxy_lookup(transports, NULL, argv[i],

@@ -3763,35 +3801,7 @@ static void cmd_acquire_transport(int argc, char *argv[])
 			return bt_shell_noninteractive_quit(EXIT_FAILURE);
 		}
 
-		ep = find_ep_by_transport(g_dbus_proxy_get_path(proxy));
-		if (!ep || ep->acquiring) {
-			bt_shell_printf(
-				"Transport %s already in acquiring process\n",
-				argv[i]);
-			return bt_shell_noninteractive_quit(EXIT_FAILURE);
-		}
-
-		ep->acquiring = true;
-
-		link = find_link_by_proxy(proxy);
-		if (link) {
-			bt_shell_printf("Link %s found\n", link->transport);
-			/* If link already acquiring wait it to be complete */
-			if (link->acquiring) {
-				bt_shell_printf(
-					"Link %s is in acquiring process\n",
-					argv[i]);
-				return bt_shell_noninteractive_quit(
-								EXIT_FAILURE);
-			}
-			link->acquiring = true;
-		}
-
-		if (!g_dbus_proxy_method_call(proxy, "Acquire", NULL,
-						acquire_reply, proxy, NULL)) {
-			bt_shell_printf("Failed acquire transport\n");
-			return bt_shell_noninteractive_quit(EXIT_FAILURE);
-		}
+		transport_acquire(proxy, false);
 	}
 
 	return bt_shell_noninteractive_quit(EXIT_SUCCESS);

@@ -4076,9 +4086,11 @@ static void cmd_send_transport(int argc, char *argv[])
 		memset(&qos, 0, sizeof(qos));
 		len = sizeof(qos);
 		if (getsockopt(transport->sk, SOL_BLUETOOTH, BT_ISO_QOS, &qos,
-							&len) < 0)
+							&len) < 0) {
+			bt_shell_printf("Unable to getsockopt(BT_ISO_QOS): %s",
+							strerror(errno));
 			err = transport_send(transport, fd, NULL);
-		else
+		} else
 			err = transport_send(transport, fd, &qos);
 
 		if (err < 0) {
Signature · 1 line
2.40.1
author maintainer
#4 lore raw

PATCH 5/5 shared/bap: Pass bcode as a reference instead of value

In reply to Luiz Augusto von Dentz

From: Luiz Augusto von Dentz <[email protected]>

This makes bcode field a pointer which makes it simpler to detect when it is set and also fixes the usage of util_iov_free which expects it to be allocated.

---
 profiles/audio/bap.c | 12 +++++-------
 src/shared/bap.h     |  2 +-
 2 files changed, 6 insertions(+), 8 deletions(-)
profiles/audio/bap.c +5 -7
index f3564c9e8..8e2fc1556 100644
@@ -427,12 +427,9 @@ static int parse_properties(DBusMessageIter *props, struct iovec **caps,
 			dbus_message_iter_get_basic(&value,
 							&qos->bcast.timeout);
 		} else if (!strcasecmp(key, "BroadcastCode")) {
-			struct iovec *iov;
-
 			if (var != DBUS_TYPE_ARRAY)
 				goto fail;
-			iov = &qos->bcast.bcode;
-			parse_array(&value, &iov);
+			parse_array(&value, &qos->bcast.bcode);
 		}
 
 		dbus_message_iter_next(props);

@@ -624,7 +621,7 @@ static void ep_free(void *data)
 	util_iov_free(ep->caps, 1);
 	util_iov_free(ep->metadata, 1);
 	if (bt_bap_stream_get_type(ep->stream) == BT_BAP_STREAM_TYPE_BCAST)
-		util_iov_free(&ep->qos.bcast.bcode, 1);
+		util_iov_free(ep->qos.bcast.bcode, 1);
 	free(ep->path);
 	free(ep);
 }

@@ -1268,7 +1265,8 @@ static void bap_create_bcast_io(struct bap_data *data, struct bap_ep *ep,
 	iso_qos.bcast.packing = ep->qos.bcast.packing;
 	iso_qos.bcast.framing = ep->qos.bcast.framing;
 	iso_qos.bcast.encryption = ep->qos.bcast.encryption;
-	memcpy(iso_qos.bcast.bcode, ep->qos.bcast.bcode.iov_base, 16);
+	if (ep->qos.bcast.bcode)
+		memcpy(iso_qos.bcast.bcode, ep->qos.bcast.bcode->iov_base, 16);
 	iso_qos.bcast.options = ep->qos.bcast.options;
 	iso_qos.bcast.skip = ep->qos.bcast.skip;
 	iso_qos.bcast.sync_timeout = ep->qos.bcast.sync_timeout;

@@ -1811,7 +1809,7 @@ static struct btd_profile bap_profile = {
 	.disconnect	= bap_disconnect,
 	.adapter_probe	= bap_adapter_probe,
 	.adapter_remove	= bap_adapter_remove,
-	.auto_connect	= true,
+	.auto_connect	= false,
 	.experimental	= true,
 };
src/shared/bap.h +1 -1
index 8fc41864a..50b567663 100644
@@ -79,7 +79,7 @@ struct bt_bap_bcast_qos {
 	uint8_t  packing;
 	uint8_t  framing;
 	uint8_t  encryption;
-	struct iovec bcode;
+	struct iovec *bcode;
 	uint8_t  options;
 	uint16_t skip;
 	uint16_t sync_timeout;
Signature · 1 line
2.40.1
#5 lore raw
In reply to Luiz Augusto von Dentz

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=758478

---Test result---

Test Summary:

CheckPatch                    PASS      2.89 seconds
GitLint                       PASS      1.82 seconds
BuildEll                      PASS      27.80 seconds
BluezMake                     PASS      875.71 seconds
MakeCheck                     PASS      12.05 seconds
MakeDistcheck                 PASS      156.89 seconds
CheckValgrind                 PASS      259.09 seconds
CheckSmatch                   PASS      345.82 seconds
bluezmakeextell               PASS      104.47 seconds
IncrementalBuild              PASS      3647.17 seconds
ScanBuild                     WARNING   1066.68 seconds

Details
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
client/player.c:3661:25: warning: Access to field 'acquiring' results in a dereference of a null pointer (loaded from variable 'ep')

                if (!ep && queue_find(ep->acquiring, NULL, link))
                                      ^~~~~~~~~~~~~

1 warning generated.

---
Regards,
Linux Bluetooth

CI
#6 lore raw
In reply to Luiz Augusto von Dentz
This series was applied to bluetooth/bluez.git (master) by Luiz Augusto von Dentz <[email protected]>:

Hello:

This series was applied to bluetooth/bluez.git (master) by Luiz Augusto von Dentz <[email protected]>:

Quoted 8 lines · On Mon, 19 Jun 2023 13:30:28 -0700 you wrote:

From: Luiz Augusto von Dentz <[email protected]>

This makes sure unespecified bit is properly marked in both audio contexts since that required by many platforms in order to work properly and while doing that add proper defines to the defaults values used in PACS.

[...]

Here is the summary with links:
- [BlueZ,1/5] shared/bap: Add unespecified bit in audio context to PAC records

    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=62032321bfe8
  - [BlueZ,2/5] client: Print integers decimal value
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=2c9ab2d3f411
  - [BlueZ,3/5] client/player: Fix auto registration of broadcast endpoint
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=0053bc5472b6
  - [BlueZ,4/5] client/player: Fix transport.acquire for linked transports
    (no matching commit)
  - [BlueZ,5/5] shared/bap: Pass bcode as a reference instead of value
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=95d3e66524bf

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html