[PATCH BlueZ] shared/bap: reset local ep states on bap session detach

[PATCH BlueZ] shared/bap: reset local ep states on bap session detach

[Pauli Virtanen]

When detaching BAP session, the session is removed from the global list,
and streams do not go through normal state change cleanup, so local
endpoint states are not cleaned up. This causes problems as ASE may be
in busy state even though there is no stream.

Fix by resetting all ASE to IDLE state after detaching all streams.
---
 src/shared/bap.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index ed5c322b4..d4beb9818 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -5664,6 +5664,14 @@ static void stream_foreach_detach(void *data, void *user_data)
 	stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
 }
 
+static void ep_foreach_detach(void *data, void *user_data)
+{
+	struct bt_bap_endpoint *ep = data;
+
+	ep->state = BT_ASCS_ASE_STATE_IDLE;
+	ep->old_state = BT_ASCS_ASE_STATE_IDLE;
+}
+
 static void bap_req_detach(void *data)
 {
 	struct bt_bap_req *req = data;
@@ -5696,6 +5704,7 @@ void bt_bap_detach(struct bt_bap *bap)
 	bap->att = NULL;
 
 	queue_foreach(bap->streams, stream_foreach_detach, bap);
+	queue_foreach(bap->local_eps, ep_foreach_detach, bap);
 	queue_foreach(bap_cbs, bap_detached, bap);
 }
 
-- 
2.50.1

RE: [BlueZ] shared/bap: reset local ep states on bap session detach

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PENDING   0.31 seconds
GitLint                       PENDING   0.22 seconds
BuildEll                      PASS      20.03 seconds
BluezMake                     PASS      2566.34 seconds
MakeCheck                     PASS      20.42 seconds
MakeDistcheck                 PASS      183.99 seconds
CheckValgrind                 PASS      235.55 seconds
CheckSmatch                   WARNING   306.96 seconds
bluezmakeextell               PASS      127.71 seconds
IncrementalBuild              PENDING   0.22 seconds
ScanBuild                     PASS      912.75 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:317: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:317: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:317: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] shared/bap: reset local ep states on bap session detach

[Luiz Augusto von Dentz]

Hi Pauli,

On Sun, Aug 10, 2025 at 2:18 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> When detaching BAP session, the session is removed from the global list,
> and streams do not go through normal state change cleanup, so local
> endpoint states are not cleaned up. This causes problems as ASE may be
> in busy state even though there is no stream.

Why wouldn't they go through the normal state change cleanup though,
the stream shall be set to idle and then affect the endpoint state as
well or we do have something preventing that to happen? Or the
local_eps are not used as stream->ep for some reason?

>
> Fix by resetting all ASE to IDLE state after detaching all streams.
> ---
>  src/shared/bap.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index ed5c322b4..d4beb9818 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -5664,6 +5664,14 @@ static void stream_foreach_detach(void *data, void *user_data)
>         stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
>  }
>
> +static void ep_foreach_detach(void *data, void *user_data)
> +{
> +       struct bt_bap_endpoint *ep = data;
> +
> +       ep->state = BT_ASCS_ASE_STATE_IDLE;
> +       ep->old_state = BT_ASCS_ASE_STATE_IDLE;
> +}
> +
>  static void bap_req_detach(void *data)
>  {
>         struct bt_bap_req *req = data;
> @@ -5696,6 +5704,7 @@ void bt_bap_detach(struct bt_bap *bap)
>         bap->att = NULL;
>
>         queue_foreach(bap->streams, stream_foreach_detach, bap);
> +       queue_foreach(bap->local_eps, ep_foreach_detach, bap);

This sounds more like a workaround though, the stream_foreach_detach
should have cleaned up all existing streams and their endpoints, we
could perhaps print a message if the ep->state is not idle then it
means something is not quite right.

>         queue_foreach(bap_cbs, bap_detached, bap);
>  }
>
> --
> 2.50.1
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] shared/bap: reset local ep states on bap session detach

[Pauli Virtanen]

Hi Luiz,

ma, 2025-08-11 kello 10:38 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Sun, Aug 10, 2025 at 2:18 PM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > When detaching BAP session, the session is removed from the global list,
> > and streams do not go through normal state change cleanup, so local
> > endpoint states are not cleaned up. This causes problems as ASE may be
> > in busy state even though there is no stream.
> 
> Why wouldn't they go through the normal state change cleanup though,
> the stream shall be set to idle and then affect the endpoint state as
> well or we do have something preventing that to happen? Or the
> local_eps are not used as stream->ep for some reason?

What I mean above, is that design in bt_bap_detach() appears to be to
not call the usual state change callbacks from there.

bap is first removed from sessions list, so bt_bap_ref_safe(bap) ==
NULL, so that stream_set_state() only calls ops->detach(), which does
not change ep state.

For remote EPs (iow as BAP Client), we will re-read their state when
attaching the session next time, so state cleanup on detach is not
necessary. (We also probably should not in general be updating remote
EP states ourselves, but leave it to remote via GATT.)

For local EPs, we don't reset state on detach or attach, so they stay
what they were.

I think we (as BAP Server) are not supposed to persist the local ASE
state over session detach, e.g. remote disappears and ATT disconnects,
and then reconnects later.

We could reset the endpoint state in stream_set_state() if there is no
session, if that sounds better, instead of doing it in bt_bap_detach()
like here.

IIUC stream_set_state() is supposed to be called only for streams to
local endpoints, but not 100% sure if it's so in practice (re:
broadcast or stream_io_disconnected).

> > 
> > Fix by resetting all ASE to IDLE state after detaching all streams.
> > ---
> >  src/shared/bap.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > index ed5c322b4..d4beb9818 100644
> > --- a/src/shared/bap.c
> > +++ b/src/shared/bap.c
> > @@ -5664,6 +5664,14 @@ static void stream_foreach_detach(void *data, void *user_data)
> >         stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
> >  }
> > 
> > +static void ep_foreach_detach(void *data, void *user_data)
> > +{
> > +       struct bt_bap_endpoint *ep = data;
> > +
> > +       ep->state = BT_ASCS_ASE_STATE_IDLE;
> > +       ep->old_state = BT_ASCS_ASE_STATE_IDLE;
> > +}
> > +
> >  static void bap_req_detach(void *data)
> >  {
> >         struct bt_bap_req *req = data;
> > @@ -5696,6 +5704,7 @@ void bt_bap_detach(struct bt_bap *bap)
> >         bap->att = NULL;
> > 
> >         queue_foreach(bap->streams, stream_foreach_detach, bap);
> > +       queue_foreach(bap->local_eps, ep_foreach_detach, bap);
> 
> This sounds more like a workaround though, the stream_foreach_detach
> should have cleaned up all existing streams and their endpoints, we
> could perhaps print a message if the ep->state is not idle then it
> means something is not quite right.
> 
> >         queue_foreach(bap_cbs, bap_detached, bap);
> >  }
> > 
> > --
> > 2.50.1
> > 
> > 
> 

Re: [PATCH BlueZ] shared/bap: reset local ep states on bap session detach

[Pauli Virtanen]

ma, 2025-08-11 kello 19:54 +0300, Pauli Virtanen kirjoitti:

[clip]
> We could reset the endpoint state in stream_set_state() if there is no
> session, if that sounds better, instead of doing it in bt_bap_detach()
> like here.
> 
> IIUC stream_set_state() is supposed to be called only for streams to
> local endpoints, but not 100% sure if it's so in practice (re:
> broadcast or stream_io_disconnected).

Well, now that I wrote it down, of course bt_bap_detach() itself calls
stream_set_state() also as BAP Client. That would change remote EP
states, and not sure if we want to do that.

> > > Fix by resetting all ASE to IDLE state after detaching all streams.
> > > ---
> > >  src/shared/bap.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > > index ed5c322b4..d4beb9818 100644
> > > --- a/src/shared/bap.c
> > > +++ b/src/shared/bap.c
> > > @@ -5664,6 +5664,14 @@ static void stream_foreach_detach(void *data, void *user_data)
> > >         stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
> > >  }
> > > 
> > > +static void ep_foreach_detach(void *data, void *user_data)
> > > +{
> > > +       struct bt_bap_endpoint *ep = data;
> > > +
> > > +       ep->state = BT_ASCS_ASE_STATE_IDLE;
> > > +       ep->old_state = BT_ASCS_ASE_STATE_IDLE;
> > > +}
> > > +
> > >  static void bap_req_detach(void *data)
> > >  {
> > >         struct bt_bap_req *req = data;
> > > @@ -5696,6 +5704,7 @@ void bt_bap_detach(struct bt_bap *bap)
> > >         bap->att = NULL;
> > > 
> > >         queue_foreach(bap->streams, stream_foreach_detach, bap);
> > > +       queue_foreach(bap->local_eps, ep_foreach_detach, bap);
> > 
> > This sounds more like a workaround though, the stream_foreach_detach
> > should have cleaned up all existing streams and their endpoints, we
> > could perhaps print a message if the ep->state is not idle then it
> > means something is not quite right.
> > 
> > >         queue_foreach(bap_cbs, bap_detached, bap);
> > >  }
> > > 
> > > --
> > > 2.50.1
> > > 
> > > 
> > 

-- 
Pauli Virtanen

Re: [PATCH BlueZ] shared/bap: reset local ep states on bap session detach

[Luiz Augusto von Dentz]

Hi Pauli,

On Mon, Aug 11, 2025 at 12:54 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> ma, 2025-08-11 kello 10:38 -0400, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> >
> > On Sun, Aug 10, 2025 at 2:18 PM Pauli Virtanen <pav@iki.fi> wrote:
> > >
> > > When detaching BAP session, the session is removed from the global list,
> > > and streams do not go through normal state change cleanup, so local
> > > endpoint states are not cleaned up. This causes problems as ASE may be
> > > in busy state even though there is no stream.
> >
> > Why wouldn't they go through the normal state change cleanup though,
> > the stream shall be set to idle and then affect the endpoint state as
> > well or we do have something preventing that to happen? Or the
> > local_eps are not used as stream->ep for some reason?
>
> What I mean above, is that design in bt_bap_detach() appears to be to
> not call the usual state change callbacks from there.
>
> bap is first removed from sessions list, so bt_bap_ref_safe(bap) ==
> NULL, so that stream_set_state() only calls ops->detach(), which does
> not change ep state.

Ok, that explains it, bu then perhaps the likes of bap_ucast_detach
shall be cleanup the ep state once it does ep->stream = NULL, that
said if it is an unexpected disconnect Id cache the codec
configuration so upon reconnection that should restore the stream
faster, if the stream has been released then it should be in idle
already.

> For remote EPs (iow as BAP Client), we will re-read their state when
> attaching the session next time, so state cleanup on detach is not
> necessary. (We also probably should not in general be updating remote
> EP states ourselves, but leave it to remote via GATT.)
>
> For local EPs, we don't reset state on detach or attach, so they stay
> what they were.

Ok, this is a bug, on disconnect we either should return it to codec
config (cache) or idle (non-cache).

> I think we (as BAP Server) are not supposed to persist the local ASE
> state over session detach, e.g. remote disappears and ATT disconnects,
> and then reconnects later.
>
> We could reset the endpoint state in stream_set_state() if there is no
> session, if that sounds better, instead of doing it in bt_bap_detach()
> like here.
>
> IIUC stream_set_state() is supposed to be called only for streams to
> local endpoints, but not 100% sure if it's so in practice (re:
> broadcast or stream_io_disconnected).
>
> > >
> > > Fix by resetting all ASE to IDLE state after detaching all streams.
> > > ---
> > >  src/shared/bap.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > > index ed5c322b4..d4beb9818 100644
> > > --- a/src/shared/bap.c
> > > +++ b/src/shared/bap.c
> > > @@ -5664,6 +5664,14 @@ static void stream_foreach_detach(void *data, void *user_data)
> > >         stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
> > >  }
> > >
> > > +static void ep_foreach_detach(void *data, void *user_data)
> > > +{
> > > +       struct bt_bap_endpoint *ep = data;
> > > +
> > > +       ep->state = BT_ASCS_ASE_STATE_IDLE;
> > > +       ep->old_state = BT_ASCS_ASE_STATE_IDLE;
> > > +}
> > > +
> > >  static void bap_req_detach(void *data)
> > >  {
> > >         struct bt_bap_req *req = data;
> > > @@ -5696,6 +5704,7 @@ void bt_bap_detach(struct bt_bap *bap)
> > >         bap->att = NULL;
> > >
> > >         queue_foreach(bap->streams, stream_foreach_detach, bap);
> > > +       queue_foreach(bap->local_eps, ep_foreach_detach, bap);
> >
> > This sounds more like a workaround though, the stream_foreach_detach
> > should have cleaned up all existing streams and their endpoints, we
> > could perhaps print a message if the ep->state is not idle then it
> > means something is not quite right.
> >
> > >         queue_foreach(bap_cbs, bap_detached, bap);
> > >  }
> > >
> > > --
> > > 2.50.1
> > >
> > >
> >



-- 
Luiz Augusto von Dentz