[PATCH BlueZ bluez] bap: Add idle notification for ASE State

[PATCH BlueZ bluez] bap: Add idle notification for ASE State

[Yang Li via B4 Relay]

From: Yang Li <yang.li@amlogic.com>

When the ASE state changes from releasing(6) -> idle(0),
the idle state needs to be notified to the Client.

---
Signed-off-by: Yang Li <yang.li@amlogic.com>
---
 src/shared/bap.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 650bea2f4..c40d6e051 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -1123,17 +1123,12 @@ static void stream_notify_metadata(struct bt_bap_stream *stream)
 	free(status);
 }
 
-static void stream_notify_release(struct bt_bap_stream *stream)
+static void stream_notify_ase_state(struct bt_bap_stream *stream)
 {
 	struct bt_bap_endpoint *ep = stream->ep;
 	struct bt_ascs_ase_status status;
 
-	DBG(stream->bap, "stream %p", stream);
-
-
-	memset(&status, 0, sizeof(status));
 	status.id = ep->id;
-	ep->state = BT_BAP_STREAM_STATE_RELEASING;
 	status.state = ep->state;
 
 	gatt_db_attribute_notify(ep->attr, (void *)&status, sizeof(status),
@@ -1713,6 +1708,7 @@ static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
 
 	switch (state) {
 	case BT_ASCS_ASE_STATE_IDLE:
+		stream_notify_ase_state(stream);
 		break;
 	case BT_ASCS_ASE_STATE_CONFIG:
 		stream_notify_config(stream);
@@ -1726,7 +1722,7 @@ static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
 		stream_notify_metadata(stream);
 		break;
 	case BT_ASCS_ASE_STATE_RELEASING:
-		stream_notify_release(stream);
+		stream_notify_ase_state(stream);
 		break;
 	}
 }
@@ -6397,9 +6393,8 @@ static bool stream_io_disconnected(struct io *io, void *user_data)
 	DBG(stream->bap, "stream %p io disconnected", stream);
 
 	if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING)
-		stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
+		stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
 
-	bt_bap_stream_set_io(stream, -1);
 	return false;
 }
 

---
base-commit: 0efa20cbf3fb5693c7c2f14ba8cf67053ca029e5
change-id: 20250407-bap_aes_state-9306798ff95a

Best regards,
-- 
Yang Li <yang.li@amlogic.com>

RE: [BlueZ,bluez] bap: Add idle notification for ASE State

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PENDING   0.32 seconds
GitLint                       PENDING   0.32 seconds
BuildEll                      PASS      20.39 seconds
BluezMake                     PASS      1606.58 seconds
MakeCheck                     PASS      20.68 seconds
MakeDistcheck                 PASS      157.73 seconds
CheckValgrind                 PASS      213.33 seconds
CheckSmatch                   WARNING   283.70 seconds
bluezmakeextell               PASS      97.99 seconds
IncrementalBuild              PENDING   0.34 seconds
ScanBuild                     PASS      870.13 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

Re: [PATCH BlueZ bluez] bap: Add idle notification for ASE State

[Luiz Augusto von Dentz]

Hi Yang,

On Mon, Apr 7, 2025 at 6:34 AM Yang Li via B4 Relay
<devnull+yang.li.amlogic.com@kernel.org> wrote:
>
> From: Yang Li <yang.li@amlogic.com>
>
> When the ASE state changes from releasing(6) -> idle(0),
> the idle state needs to be notified to the Client.
>
> ---
> Signed-off-by: Yang Li <yang.li@amlogic.com>
> ---
>  src/shared/bap.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 650bea2f4..c40d6e051 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -1123,17 +1123,12 @@ static void stream_notify_metadata(struct bt_bap_stream *stream)
>         free(status);
>  }
>
> -static void stream_notify_release(struct bt_bap_stream *stream)
> +static void stream_notify_ase_state(struct bt_bap_stream *stream)
>  {
>         struct bt_bap_endpoint *ep = stream->ep;
>         struct bt_ascs_ase_status status;
>
> -       DBG(stream->bap, "stream %p", stream);
> -
> -
> -       memset(&status, 0, sizeof(status));
>         status.id = ep->id;
> -       ep->state = BT_BAP_STREAM_STATE_RELEASING;

Not really sure why you are taking away releasing state, we actually
depend on it for the new tests:

https://patchwork.kernel.org/project/bluetooth/list/?series=950067

>         status.state = ep->state;
>
>         gatt_db_attribute_notify(ep->attr, (void *)&status, sizeof(status),
> @@ -1713,6 +1708,7 @@ static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
>
>         switch (state) {
>         case BT_ASCS_ASE_STATE_IDLE:
> +               stream_notify_ase_state(stream);

We need something like stream_notify_idle.

>                 break;
>         case BT_ASCS_ASE_STATE_CONFIG:
>                 stream_notify_config(stream);
> @@ -1726,7 +1722,7 @@ static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
>                 stream_notify_metadata(stream);
>                 break;
>         case BT_ASCS_ASE_STATE_RELEASING:
> -               stream_notify_release(stream);
> +               stream_notify_ase_state(stream);

This is actually wrong, we need to notify the releasing state.

>                 break;
>         }
>  }
> @@ -6397,9 +6393,8 @@ static bool stream_io_disconnected(struct io *io, void *user_data)
>         DBG(stream->bap, "stream %p io disconnected", stream);
>
>         if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING)
> -               stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
> +               stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);

Ok, so this is sort of reverting from  bap: Fix not generating
releasing state? I was wondering how much testing you guys did to
confirm this is the right behavior, I don't think it is and Im trying
to figure out if there are any tests for the testing spec that do
properly verify this behavior.

> -       bt_bap_stream_set_io(stream, -1);
>         return false;
>  }
>
>
> ---
> base-commit: 0efa20cbf3fb5693c7c2f14ba8cf67053ca029e5
> change-id: 20250407-bap_aes_state-9306798ff95a
>
> Best regards,
> --
> Yang Li <yang.li@amlogic.com>
>
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ bluez] bap: Add idle notification for ASE State

[Yang Li]

Hi Luiz，
> [ EXTERNAL EMAIL ]
>
> Hi Yang,
>
> On Mon, Apr 7, 2025 at 6:34 AM Yang Li via B4 Relay
> <devnull+yang.li.amlogic.com@kernel.org> wrote:
>> From: Yang Li <yang.li@amlogic.com>
>>
>> When the ASE state changes from releasing(6) -> idle(0),
>> the idle state needs to be notified to the Client.
>>
>> ---
>> Signed-off-by: Yang Li <yang.li@amlogic.com>
>> ---
>>   src/shared/bap.c | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/shared/bap.c b/src/shared/bap.c
>> index 650bea2f4..c40d6e051 100644
>> --- a/src/shared/bap.c
>> +++ b/src/shared/bap.c
>> @@ -1123,17 +1123,12 @@ static void stream_notify_metadata(struct bt_bap_stream *stream)
>>          free(status);
>>   }
>>
>> -static void stream_notify_release(struct bt_bap_stream *stream)
>> +static void stream_notify_ase_state(struct bt_bap_stream *stream)
>>   {
>>          struct bt_bap_endpoint *ep = stream->ep;
>>          struct bt_ascs_ase_status status;
>>
>> -       DBG(stream->bap, "stream %p", stream);
>> -
>> -
>> -       memset(&status, 0, sizeof(status));
>>          status.id = ep->id;
>> -       ep->state = BT_BAP_STREAM_STATE_RELEASING;
> Not really sure why you are taking away releasing state, we actually
> depend on it for the new tests:
>
> https://patchwork.kernel.org/project/bluetooth/list/?series=950067
Well, I got it.
>>          status.state = ep->state;
>>
>>          gatt_db_attribute_notify(ep->attr, (void *)&status, sizeof(status),
>> @@ -1713,6 +1708,7 @@ static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
>>
>>          switch (state) {
>>          case BT_ASCS_ASE_STATE_IDLE:
>> +               stream_notify_ase_state(stream);
> We need something like stream_notify_idle.
Well, I got it.
>>                  break;
>>          case BT_ASCS_ASE_STATE_CONFIG:
>>                  stream_notify_config(stream);
>> @@ -1726,7 +1722,7 @@ static void stream_notify(struct bt_bap_stream *stream, uint8_t state)
>>                  stream_notify_metadata(stream);
>>                  break;
>>          case BT_ASCS_ASE_STATE_RELEASING:
>> -               stream_notify_release(stream);
>> +               stream_notify_ase_state(stream);
> This is actually wrong, we need to notify the releasing state.
>
>>                  break;
>>          }
>>   }
>> @@ -6397,9 +6393,8 @@ static bool stream_io_disconnected(struct io *io, void *user_data)
>>          DBG(stream->bap, "stream %p io disconnected", stream);
>>
>>          if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING)
>> -               stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
>> +               stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
> Ok, so this is sort of reverting from  bap: Fix not generating
> releasing state? I was wondering how much testing you guys did to
> confirm this is the right behavior, I don't think it is and Im trying
> to figure out if there are any tests for the testing spec that do
> properly verify this behavior.

There are some misunderstandings that need to be clarified. Initially, 
patchset V1 
https://lore.kernel.org/all/20250106-upstream-v1-1-a16879b78ffd@amlogic.com/ 
was proposed to solve the issue 
https://github.com/bluez/bluez/issues/1053, but after discussion, I felt 
that the process of Streaming->Releasing->Idle was OK, so V1 was 
abandoned. Then I submitted patchset V2/V3 
https://lore.kernel.org/all/20250213-ascs_bug-v3-1-d5594f0f20c6@amlogic.com/. 
I tested K70 and Pixel phones on V3, and both can solve the above 
issues. So my original intention was to merge V3, but after the release 
of version 5.82, I checked the code and found that V1 was merged. So I 
submitted the modification again on the latest version.

I sorted out the process of ASE state switching when the media of 
different mobile phones is paused:

1. Redmi K70+DUT: Pause operation, ASE state process is 
Streaming--->Disabling->QoS
2. Pixel+DUT: Pause operation, DUT does not cache Codec, ASE state 
process is Streaming->Releasing->Idl
3. Pixel+RedmiBud5 earbuds: Pause operation, earbuds cache Codec, ASE 
state process is Streaming->Releasing->Codec
If the DUT also caches Codec, the latest version of the process is 
Streaming->Releasing->Codec->QoS, but the QoS status will be abnormal on 
Pixel phones, causing LE disconnection, so the process without caching 
Codec is still adopted.
>
>> -       bt_bap_stream_set_io(stream, -1);
>>          return false;
>>   }
>>
>>
>> ---
>> base-commit: 0efa20cbf3fb5693c7c2f14ba8cf67053ca029e5
>> change-id: 20250407-bap_aes_state-9306798ff95a
>>
>> Best regards,
>> --
>> Yang Li <yang.li@amlogic.com>
>>
>>
>>
>
> --
> Luiz Augusto von Dentz