[BlueZ,v2] shared/bap: Fix swallowing states transitions

Commit Message

Luiz Augusto von Dentz April 1, 2025, 1:51 p.m. UTC

From: Luiz Augusto von Dentz <[e-mail]>

In certain cases (e.g ASCS_Enable) a Control Point operation may change
states multiple times causing states not to be notified.

To fix this attempts to queue states if timeout is pending (state_id)
and then proceed to notify them ahead of the last state set in the ASE
so the remote side is informed of all the state transitions.
---
 src/shared/bap.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

Comments

[e-mail] April 1, 2025, 3:11 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=948958

---Test result---

Test Summary:
CheckPatch                    PENDING   0.27 seconds
GitLint                       PENDING   0.26 seconds
BuildEll                      PASS      20.49 seconds
BluezMake                     PASS      1498.49 seconds
MakeCheck                     PASS      13.27 seconds
MakeDistcheck                 PASS      157.52 seconds
CheckValgrind                 PASS      217.93 seconds
CheckSmatch                   WARNING   285.93 seconds
bluezmakeextell               PASS      98.39 seconds
IncrementalBuild              PENDING   0.25 seconds
ScanBuild                     PASS      869.65 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:314:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:314:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:314:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

[e-mail] April 1, 2025, 9:40 p.m. UTC | #2

Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Tue,  1 Apr 2025 09:51:15 -0400 you wrote:
> From: Luiz Augusto von Dentz <[e-mail]>
> 
> In certain cases (e.g ASCS_Enable) a Control Point operation may change
> states multiple times causing states not to be notified.
> 
> To fix this attempts to queue states if timeout is pending (state_id)
> and then proceed to notify them ahead of the last state set in the ASE
> so the remote side is informed of all the state transitions.
> 
> [...]

Here is the summary with links:
  - [BlueZ,v2] shared/bap: Fix swallowing states transitions
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=6d20a300642f

You are awesome, thank you!

Patch

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 5d4d69d2925b..650bea2f4e8d 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -293,6 +293,7 @@  struct bt_bap_stream {
 	uint8_t old_state;
 	uint8_t state;
 	unsigned int state_id;
+	struct queue *pending_states;
 	bool client;
 	void *user_data;
 };
@@ -1205,6 +1206,7 @@  static void bap_stream_free(void *data)
 	struct bt_bap_stream *stream = data;
 
 	timeout_remove(stream->state_id);
+	queue_destroy(stream->pending_states, NULL);
 
 	if (stream->ep)
 		stream->ep->stream = NULL;
@@ -1705,19 +1707,11 @@  static bool bap_queue_req(struct bt_bap *bap, struct bt_bap_req *req)
 	return true;
 }
 
-static bool stream_notify_state(void *data)
+static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
 {
-	struct bt_bap_stream *stream = data;
-	struct bt_bap_endpoint *ep = stream->ep;
+	DBG(stream->bap, "stream %p state %d", stream, state);
 
-	DBG(stream->bap, "stream %p status %d", stream, ep->state);
-
-	if (stream->state_id) {
-		timeout_remove(stream->state_id);
-		stream->state_id = 0;
-	}
-
-	switch (ep->state) {
+	switch (state) {
 	case BT_ASCS_ASE_STATE_IDLE:
 		break;
 	case BT_ASCS_ASE_STATE_CONFIG:
@@ -1735,6 +1729,24 @@  static bool stream_notify_state(void *data)
 		stream_notify_release(stream);
 		break;
 	}
+}
+
+static bool stream_notify_state(void *data)
+{
+	struct bt_bap_stream *stream = data;
+	struct bt_bap_endpoint *ep = stream->ep;
+	uint8_t state;
+
+	if (stream->state_id) {
+		timeout_remove(stream->state_id);
+		stream->state_id = 0;
+	}
+
+	/* Notify any pending states before notifying ep->state */
+	while ((state = PTR_TO_UINT(queue_pop_head(stream->pending_states))))
+		stream_notify(stream, state);
+
+	stream_notify(stream, ep->state);
 
 	return false;
 }
@@ -1760,6 +1772,10 @@  static void bap_ucast_set_state(struct bt_bap_stream *stream, uint8_t state)
 		stream->state_id = timeout_add(BAP_PROCESS_TIMEOUT,
 						stream_notify_state,
 						stream, NULL);
+	else /* If a state_id is already pending then queue the old one */
+		queue_push_tail(stream->pending_states,
+				UINT_TO_PTR(ep->old_state));
+
 
 done:
 	bap_stream_state_changed(stream);
@@ -2716,6 +2732,7 @@  static struct bt_bap_stream *bap_stream_new(struct bt_bap *bap,
 	stream->cc = util_iov_dup(data, 1);
 	stream->client = client;
 	stream->ops = bap_stream_new_ops(stream);
+	stream->pending_states = queue_new();
 
 	queue_push_tail(bap->streams, stream);