diff mbox series

[BlueZ,1/2] transport: handle BAP Enabling state correctly when resuming

Message ID 8af1dd5097cc4411ff2681ed39c49c232f817ebe.1688326228.git.pav@iki.fi (mailing list archive)
State Accepted
Commit 466fce0209a3878512672159168943047a9e2323
Headers show
Series [BlueZ,1/2] transport: handle BAP Enabling state correctly when resuming | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint fail 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 24: B1 Line exceeds max length (86>80): "profiles/audio/transport.c:bap_state_changed() stream 0x25c2880: qos(2) -> enabling(3)" 26: B1 Line exceeds max length (86>80): "profiles/audio/transport.c:bap_state_changed() stream 0x25cc590: qos(2) -> enabling(3)" 28: B1 Line exceeds max length (92>80): "src/shared/bap.c:bap_stream_state_changed() stream 0x25cc590 dir 0x01: enabling -> streaming" 30: B1 Line exceeds max length (92>80): "profiles/audio/transport.c:bap_state_changed() stream 0x25cc590: enabling(3) -> streaming(4)" 32: B1 Line exceeds max length (132>80): "profiles/audio/transport.c:transport_update_playing() /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/fd1 State=TRANSPORT_STATE_IDLE Playing=1" 33: B1 Line exceeds max length (153>80): "profiles/audio/transport.c:transport_set_state() State changed /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/fd1: TRANSPORT_STATE_IDLE -> TRANSPORT_STATE_PENDING" 34: B1 Line exceeds max length (153>80): "profiles/audio/transport.c:transport_set_state() State changed /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/fd0: TRANSPORT_STATE_IDLE -> TRANSPORT_STATE_PENDING"
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Pauli Virtanen July 2, 2023, 7:34 p.m. UTC 
If BAP stream is in Enabling state when transport acquire is attempted,
we should wait for bap_state_changed to emit the completion event.

transport->resume() is only called with new owner with no
owner->pending, and shall return nonzero completion id on success.
Currently if BAP stream is Enabling, it returns zero which fails the
acquire operation.

To fix this, return valid completion id in this case instead.  Also keep
track of the g_idle_add resume id, so that we don't try to give it to
bt_bap_stream_cancel.

Fixes sound server getting spurious Not Authorized errors when trying to
acquire a pending transport.  This can happen on BAP server: linked
transports become pending when the first of the two enters Streaming. If
sound server tries to acquire the other linked transport whose stream is
still Enabling, the acquire fails (media_owner_free +
btd_error_not_authorized).

Log:
===============================================================
profiles/audio/transport.c:bap_state_changed() stream 0x25c2880: qos(2) -> enabling(3)
...
profiles/audio/transport.c:bap_state_changed() stream 0x25cc590: qos(2) -> enabling(3)
...
src/shared/bap.c:bap_stream_state_changed() stream 0x25cc590 dir 0x01: enabling -> streaming
profiles/audio/bap.c:bap_state() stream 0x25cc590: enabling(3) -> streaming(4)
profiles/audio/transport.c:bap_state_changed() stream 0x25cc590: enabling(3) -> streaming(4)
/org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/fd1: fd(36) ready
profiles/audio/transport.c:transport_update_playing() /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/fd1 State=TRANSPORT_STATE_IDLE Playing=1
profiles/audio/transport.c:transport_set_state() State changed /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/fd1: TRANSPORT_STATE_IDLE -> TRANSPORT_STATE_PENDING
profiles/audio/transport.c:transport_set_state() State changed /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/fd0: TRANSPORT_STATE_IDLE -> TRANSPORT_STATE_PENDING
profiles/audio/transport.c:media_owner_create() Owner created: sender=:1.1242
profiles/audio/transport.c:media_owner_free() Owner :1.1242
===============================================================
---
 profiles/audio/transport.c | 67 +++++++++++++++++++++++++++++++-------
 1 file changed, 55 insertions(+), 12 deletions(-)

Comments

bluez.test.bot@gmail.com July 2, 2023, 9:32 p.m. UTC | #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=761861

---Test result---

Test Summary:
CheckPatch                    PASS      0.74 seconds
GitLint                       FAIL      0.79 seconds
BuildEll                      PASS      30.22 seconds
BluezMake                     PASS      927.56 seconds
MakeCheck                     PASS      12.58 seconds
MakeDistcheck                 PASS      174.49 seconds
CheckValgrind                 PASS      288.93 seconds
CheckSmatch                   PASS      382.64 seconds
bluezmakeextell               PASS      116.35 seconds
IncrementalBuild              PASS      1538.64 seconds
ScanBuild                     PASS      1187.46 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,1/2] transport: handle BAP Enabling state correctly when resuming

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
24: B1 Line exceeds max length (86>80): "profiles/audio/transport.c:bap_state_changed() stream 0x25c2880: qos(2) -> enabling(3)"
26: B1 Line exceeds max length (86>80): "profiles/audio/transport.c:bap_state_changed() stream 0x25cc590: qos(2) -> enabling(3)"
28: B1 Line exceeds max length (92>80): "src/shared/bap.c:bap_stream_state_changed() stream 0x25cc590 dir 0x01: enabling -> streaming"
30: B1 Line exceeds max length (92>80): "profiles/audio/transport.c:bap_state_changed() stream 0x25cc590: enabling(3) -> streaming(4)"
32: B1 Line exceeds max length (132>80): "profiles/audio/transport.c:transport_update_playing() /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/fd1 State=TRANSPORT_STATE_IDLE Playing=1"
33: B1 Line exceeds max length (153>80): "profiles/audio/transport.c:transport_set_state() State changed /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/fd1: TRANSPORT_STATE_IDLE -> TRANSPORT_STATE_PENDING"
34: B1 Line exceeds max length (153>80): "profiles/audio/transport.c:transport_set_state() State changed /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/fd0: TRANSPORT_STATE_IDLE -> TRANSPORT_STATE_PENDING"


---
Regards,
Linux Bluetooth
patchwork-bot+bluetooth@kernel.org July 5, 2023, 6 p.m. UTC | #2 
Hello:

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

On Sun,  2 Jul 2023 22:34:18 +0300 you wrote:
> If BAP stream is in Enabling state when transport acquire is attempted,
> we should wait for bap_state_changed to emit the completion event.
> 
> transport->resume() is only called with new owner with no
> owner->pending, and shall return nonzero completion id on success.
> Currently if BAP stream is Enabling, it returns zero which fails the
> acquire operation.
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/2] transport: handle BAP Enabling state correctly when resuming
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=466fce0209a3
  - [BlueZ,2/2] shared/bap: use only nonzero req->id
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=8fd0c76b41d3

You are awesome, thank you!
diff mbox series

Patch

diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index 77216e10b..aa3a718b0 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -86,6 +86,7 @@  struct bap_transport {
 	unsigned int		state_id;
 	bool			linked;
 	struct bt_bap_qos	qos;
+	guint			resume_id;
 };
 
 struct media_transport {
@@ -1191,17 +1192,27 @@  static void bap_enable_complete(struct bt_bap_stream *stream,
 		media_transport_remove_owner(owner->transport);
 }
 
-static gboolean resume_complete(void *data)
+static void bap_resume_complete(struct media_transport *transport)
 {
-	struct media_transport *transport = data;
+	struct bap_transport *bap = transport->data;
 	struct media_owner *owner = transport->owner;
 
+	DBG("stream %p owner %p resume complete", bap->stream, owner);
+
+	if (bap->resume_id) {
+		g_source_remove(bap->resume_id);
+		bap->resume_id = 0;
+	}
+
 	if (!owner)
-		return FALSE;
+		return;
+
+	if (owner->pending)
+		owner->pending->id = 0;
 
 	if (transport->fd < 0) {
 		media_transport_remove_owner(transport);
-		return FALSE;
+		return;
 	}
 
 	if (owner->pending) {
@@ -1215,15 +1226,13 @@  static gboolean resume_complete(void *data)
 						DBUS_TYPE_INVALID);
 		if (!ret) {
 			media_transport_remove_owner(transport);
-			return FALSE;
+			return;
 		}
 	}
 
 	media_owner_remove(owner);
 
 	transport_set_state(transport, TRANSPORT_STATE_ACTIVE);
-
-	return FALSE;
 }
 
 static void bap_update_links(const struct media_transport *transport);
@@ -1306,6 +1315,32 @@  static void bap_update_qos(const struct media_transport *transport)
 			"Delay");
 }
 
+static gboolean bap_resume_complete_cb(void *data)
+{
+	struct media_transport *transport = data;
+	struct bap_transport *bap = transport->data;
+
+	bap->resume_id = 0;
+	bap_resume_complete(transport);
+	return FALSE;
+}
+
+static gboolean bap_resume_wait_cb(void *data)
+{
+	struct media_transport *transport = data;
+	struct bap_transport *bap = transport->data;
+	struct media_owner *owner = transport->owner;
+
+	/* bap_state_changed will call completion callback when ready */
+	DBG("stream %p owner %p resume wait", bap->stream, owner);
+
+	bap->resume_id = 0;
+	if (owner && owner->pending)
+		owner->pending->id = 0;
+
+	return FALSE;
+}
+
 static guint resume_bap(struct media_transport *transport,
 				struct media_owner *owner)
 {
@@ -1315,17 +1350,19 @@  static guint resume_bap(struct media_transport *transport,
 
 	if (!bap->stream)
 		return 0;
+	if (bap->resume_id)
+		return 0;
 
 	bap_update_links(transport);
 
 	switch (bt_bap_stream_get_state(bap->stream)) {
 	case BT_BAP_STREAM_STATE_ENABLING:
 		bap_enable_complete(bap->stream, 0x00, 0x00, owner);
-		if (owner->pending)
-			return owner->pending->id;
-		return 0;
+		bap->resume_id = g_idle_add(bap_resume_wait_cb, transport);
+		return bap->resume_id;
 	case BT_BAP_STREAM_STATE_STREAMING:
-		return g_idle_add(resume_complete, transport);
+		bap->resume_id = g_idle_add(bap_resume_complete_cb, transport);
+		return bap->resume_id;
 	}
 
 	meta = bt_bap_stream_get_metadata(bap->stream);
@@ -1389,6 +1426,12 @@  static void cancel_bap(struct media_transport *transport, guint id)
 {
 	struct bap_transport *bap = transport->data;
 
+	if (id == bap->resume_id && bap->resume_id) {
+		g_source_remove(bap->resume_id);
+		bap->resume_id = 0;
+		return;
+	}
+
 	if (!bap->stream)
 		return;
 
@@ -1491,7 +1534,7 @@  static void bap_state_changed(struct bt_bap_stream *stream, uint8_t old_state,
 	transport_update_playing(transport, TRUE);
 
 done:
-	resume_complete(transport);
+	bap_resume_complete(transport);
 }
 
 static void bap_connecting(struct bt_bap_stream *stream, bool state, int fd,