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

[Pauli Virtanen <pav@iki.fi>]

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
> > 
> > 
>