[BlueZ,v1] shared/bap: Fix ASE notification order

Commit Message

Luiz Augusto von Dentz July 12, 2024, 7:38 p.m. UTC

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

When processing a CP operation the CP shall be notified ahead of
the ASE itself:

  'If the server successfully completes a client-initiated ASE Control
  operation for an ASE, the server shall send a notification of the ASE
  Control Point characteristic value formatted as defined in Table 4.7.
  The server shall then perform the behavior defined in Section 5.1
  through Section 5.8 for that ASE Control operation and send
  notifications of any ASE characteristic values written during that
  ASE Control operation.'

So this delays the processing of notifications of ASE states so the CP
responses always appears first in the notification e.g:

> ACL Data RX: Handle 42 flags 0x02 dlen 59
      ATT: Handle Multiple Value Notification (0x23) len 54
        Length: 0x0008
        Handle: 0x0036 Type: ASE Control Point (0x2bc6)
          Data[8]: 0202030000010000
            Opcode: QoS Configuration (0x02)
            Number of ASE(s): 2
            ASE: #0
            ASE ID: 0x03
            ASE Response Code: Success (0x00)
            ASE Response Reason: None (0x00)
            ASE: #1
            ASE ID: 0x01
            ASE Response Code: Success (0x00)
            ASE Response Reason: None (0x00)
        Length: 0x0011
        Handle: 0x0030 Type: Source ASE (0x2bc5)
          Data[17]: 0302000010270000022800020a00409c00
            ASE ID: 3
            State: QoS Configured (0x02)
            CIG ID: 0x00
            CIS ID: 0x00
            SDU Interval: 10000 usec
            Framing: Unframed (0x00)
            PHY: 0x02
            LE 2M PHY (0x02)
            Max SDU: 40
            RTN: 2
            Max Transport Latency: 10
            Presentation Delay: 40000 us
        Length: 0x0011
        Handle: 0x002a Type: Sink ASE (0x2bc4)
          Data[17]: 0102000010270000025000020a00409c00
            ASE ID: 1
            State: QoS Configured (0x02)
            CIG ID: 0x00
            CIS ID: 0x00
            SDU Interval: 10000 usec
            Framing: Unframed (0x00)
            PHY: 0x02
            LE 2M PHY (0x02)
            Max SDU: 80
            RTN: 2
            Max Transport Latency: 10
            Presentation Delay: 40000 us
---
 src/shared/bap.c | 53 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 10 deletions(-)

Comments

[e-mail] July 12, 2024, 9:42 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=870944

---Test result---

Test Summary:
CheckPatch                    PASS      0.33 seconds
GitLint                       PASS      0.21 seconds
BuildEll                      PASS      24.82 seconds
BluezMake                     PASS      1704.02 seconds
MakeCheck                     PASS      12.96 seconds
MakeDistcheck                 PASS      178.96 seconds
CheckValgrind                 PASS      253.20 seconds
CheckSmatch                   WARNING   360.35 seconds
bluezmakeextell               PASS      120.92 seconds
IncrementalBuild              PASS      1486.13 seconds
ScanBuild                     PASS      1015.28 seconds

Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:288: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:288: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:288:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures


---
Regards,
Linux Bluetooth

[e-mail] July 15, 2024, 3:20 p.m. UTC | #2

Hello:

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

On Fri, 12 Jul 2024 15:38:20 -0400 you wrote:
> From: Luiz Augusto von Dentz <[e-mail]>
> 
> When processing a CP operation the CP shall be notified ahead of
> the ASE itself:
> 
>   'If the server successfully completes a client-initiated ASE Control
>   operation for an ASE, the server shall send a notification of the ASE
>   Control Point characteristic value formatted as defined in Table 4.7.
>   The server shall then perform the behavior defined in Section 5.1
>   through Section 5.8 for that ASE Control operation and send
>   notifications of any ASE characteristic values written during that
>   ASE Control operation.'
> 
> [...]

Here is the summary with links:
  - [BlueZ,v1] shared/bap: Fix ASE notification order
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=73266377b018

You are awesome, thank you!

Patch

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 3a4c1f9d3a98..d59eac8cca16 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -169,6 +169,7 @@  struct bt_bap {
 	unsigned int process_id;
 	unsigned int disconn_id;
 	unsigned int idle_id;
+	bool in_cp_write;
 
 	struct queue *reqs;
 	struct queue *notify;
@@ -266,6 +267,7 @@  struct bt_bap_stream {
 	const struct bt_bap_stream_ops *ops;
 	uint8_t old_state;
 	uint8_t state;
+	unsigned int state_id;
 	bool client;
 	void *user_data;
 };
@@ -1102,6 +1104,8 @@  static void bap_stream_free(void *data)
 {
 	struct bt_bap_stream *stream = data;
 
+	timeout_remove(stream->state_id);
+
 	if (stream->ep)
 		stream->ep->stream = NULL;
 
@@ -1579,20 +1583,17 @@  static bool bap_queue_req(struct bt_bap *bap, struct bt_bap_req *req)
 	return true;
 }
 
-static void bap_ucast_set_state(struct bt_bap_stream *stream, uint8_t state)
+static bool stream_notify_state(void *data)
 {
+	struct bt_bap_stream *stream = data;
 	struct bt_bap_endpoint *ep = stream->ep;
 
-	ep->old_state = ep->state;
-	ep->state = state;
+	DBG(stream->bap, "stream %p", stream);
 
-	DBG(stream->bap, "stream %p dir 0x%02x: %s -> %s", stream,
-			bt_bap_stream_get_dir(stream),
-			bt_bap_stream_statestr(stream->ep->old_state),
-			bt_bap_stream_statestr(stream->ep->state));
-
-	if (stream->lpac->type == BT_BAP_BCAST_SINK || stream->client)
-		goto done;
+	if (stream->state_id) {
+		timeout_remove(stream->state_id);
+		stream->state_id = 0;
+	}
 
 	switch (ep->state) {
 	case BT_ASCS_ASE_STATE_IDLE:
@@ -1610,6 +1611,31 @@  static void bap_ucast_set_state(struct bt_bap_stream *stream, uint8_t state)
 		break;
 	}
 
+	return false;
+}
+
+static void bap_ucast_set_state(struct bt_bap_stream *stream, uint8_t state)
+{
+	struct bt_bap_endpoint *ep = stream->ep;
+
+	ep->old_state = ep->state;
+	ep->state = state;
+
+	DBG(stream->bap, "stream %p dir 0x%02x: %s -> %s", stream,
+			bt_bap_stream_get_dir(stream),
+			bt_bap_stream_statestr(stream->ep->old_state),
+			bt_bap_stream_statestr(stream->ep->state));
+
+	if (stream->client)
+		goto done;
+
+	if (!stream->bap->in_cp_write)
+		stream_notify_state(stream);
+	else if (!stream->state_id)
+		stream->state_id = timeout_add(BAP_PROCESS_TIMEOUT,
+						stream_notify_state,
+						stream, NULL);
+
 done:
 	bap_stream_state_changed(stream);
 }
@@ -3069,8 +3095,15 @@  static void ascs_ase_cp_write(struct gatt_db_attribute *attrib,
 
 		DBG(bap, "%s", handler->str);
 
+		/* Set in_cp_write so ASE notification are not sent ahead of
+		 * CP notifcation.
+		 */
+		bap->in_cp_write = true;
+
 		for (i = 0; i < hdr->num; i++)
 			ret = handler->func(ascs, bap, &iov, rsp);
+
+		bap->in_cp_write = false;
 	} else {
 		DBG(bap, "Unknown opcode 0x%02x", hdr->op);
 		ascs_ase_rsp_add_errno(rsp, 0x00, -ENOTSUP);