[RFC,BlueZ,v2,04/11] shared/bap: fix ucast client ASE usage

The ucast client stream design has issues:

* Assuming lpac & rpac pair uniquely identifies a stream.  False for
  AC6(i) and other multi-stream configs.

* No way for ASE in Config state be configured with different codec.

* Assuming ASE enters idle state at end of stream life cycle.  False for
  some devices like Sony headsets, which always cache codec config so
  RELEASING -> CONFIG always, never RELEASING -> IDLE, so ASE never go
  IDLE again.

* Assuming Unicast Client upper layer always creates streams for all
  non-idle ASEs.  False, as when switching between duplex & sink-only
  modes, only some streams shall be used regardless of whether Server is
  caching config or not.

In practice, these currently prevent reconfiguring ASEs on devices which
cache codec config, and multi-stream configs work only accidentally (and
fail if server does Config codec itself, like some devices do).

Minimally fixed design for Unicast Client streams:

Streams correspond 1-to-1 to ASEs in non-IDLE state.

Track explicitly which streams upper level is using:

- bt_bap_stream_new() always makes new stream with client_active=true
- bt_bap_stream_discard() releases stream and marks client_active=false

Streams (ASEs) with no active client may be reused when upper level asks
for a new stream.

Streams with no active client may have their lpac changed.  The
need_reconfig state is a bit ugly, but not really avoidable.

Streams with no active client shall be ignored when constructing
bidirectional CIS pairs.

Streams shall clear transport and detach io on RELEASING. (cf BAP v1.0.2
§5.6.6). Also unlink them since QoS is gone at that point.

Streams shall have transport only for >= QOS state.  (Server streams
work differently, which makes sense since upper level cannot acquire
them before they are pending.)
---
 src/shared/bap.c | 170 +++++++++++++++++++++++++++++++++--------------
 src/shared/bap.h |   2 +
 2 files changed, 123 insertions(+), 49 deletions(-)

Hi Pauli,

On Sun, May 4, 2025 at 12:02 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> The ucast client stream design has issues:
>
> * Assuming lpac & rpac pair uniquely identifies a stream.  False for
>   AC6(i) and other multi-stream configs.
>
> * No way for ASE in Config state be configured with different codec.
>
> * Assuming ASE enters idle state at end of stream life cycle.  False for
>   some devices like Sony headsets, which always cache codec config so
>   RELEASING -> CONFIG always, never RELEASING -> IDLE, so ASE never go
>   IDLE again.
>
> * Assuming Unicast Client upper layer always creates streams for all
>   non-idle ASEs.  False, as when switching between duplex & sink-only
>   modes, only some streams shall be used regardless of whether Server is
>   caching config or not.

I think it would make sense to fix these points one by one, or you
tried that and ended up running into some problems?

> In practice, these currently prevent reconfiguring ASEs on devices which
> cache codec config, and multi-stream configs work only accidentally (and
> fail if server does Config codec itself, like some devices do).
>
> Minimally fixed design for Unicast Client streams:
>
> Streams correspond 1-to-1 to ASEs in non-IDLE state.
>
> Track explicitly which streams upper level is using:
>
> - bt_bap_stream_new() always makes new stream with client_active=true
> - bt_bap_stream_discard() releases stream and marks client_active=false

This perhaps could be done differently, perhaps having a lock flag, so
if the stream is locked it means its configuration cannot be changed.

> Streams (ASEs) with no active client may be reused when upper level asks
> for a new stream.
>
> Streams with no active client may have their lpac changed.  The
> need_reconfig state is a bit ugly, but not really avoidable.

Do we have a situation where client_active=false (lock=false) and
need_reconfig!=true? In other words could we just use the
client_active/lock flag instead of introducing yet another flag?

> Streams with no active client shall be ignored when constructing
> bidirectional CIS pairs.
>
> Streams shall clear transport and detach io on RELEASING. (cf BAP v1.0.2
> §5.6.6). Also unlink them since QoS is gone at that point.

I'd consider fixing this as a separate patch as well.

> Streams shall have transport only for >= QOS state.  (Server streams
> work differently, which makes sense since upper level cannot acquire
> them before they are pending.)

Sounds good, that said we might want to check if there aren't missing
tests that would cover such conditions, there is a whole session of
the BAP testing spec dedicated for streaming configuration (AC-#), if
will probably need some changes to test-bap to create socketpair to
emulate CIS since those tests require CIS/BIS in order to pass.

> ---
>  src/shared/bap.c | 170 +++++++++++++++++++++++++++++++++--------------
>  src/shared/bap.h |   2 +
>  2 files changed, 123 insertions(+), 49 deletions(-)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 976e3c0b1..456450d40 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -296,6 +296,8 @@ struct bt_bap_stream {
>         struct queue *pending_states;
>         bool no_cache_config;
>         bool client;
> +       bool client_active;
> +       bool need_reconfig;
>         void *user_data;
>  };
>
> @@ -1488,6 +1490,13 @@ static void bap_stream_state_changed(struct bt_bap_stream *stream)
>                 if (stream->client)
>                         bt_bap_stream_stop(stream, stream_stop_complete, NULL);
>                 break;
> +       case BT_ASCS_ASE_STATE_RELEASING:
> +               if (stream->client) {
> +                       bap_stream_clear_cfm(stream);
> +                       bap_stream_io_detach(stream);
> +                       bt_bap_stream_io_unlink(stream, NULL);
> +               }
> +               break;
>         }
>  }
>
> @@ -1898,6 +1907,9 @@ static unsigned int bap_ucast_qos(struct bt_bap_stream *stream,
>         if (!stream->client)
>                 return 0;
>
> +       if (stream->need_reconfig)
> +               return 0;
> +
>         memset(&qos, 0, sizeof(qos));
>
>         /* TODO: Figure out how to pass these values around */
> @@ -2300,7 +2312,6 @@ static unsigned int bap_ucast_release(struct bt_bap_stream *stream,
>         /* If stream does not belong to a client session, clean it up now */
>         if (!bap_stream_valid(stream)) {
>                 stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
> -               stream = NULL;
>                 return 0;
>         }
>
> @@ -2583,6 +2594,9 @@ static int bap_ucast_io_link(struct bt_bap_stream *stream,
>                         stream->ep->dir == link->ep->dir)
>                 return -EINVAL;
>
> +       if (stream->client && !(stream->client_active && link->client_active))
> +               return -EINVAL;
> +
>         if (!stream->links)
>                 stream->links = queue_new();
>
> @@ -2603,6 +2617,30 @@ static int bap_ucast_io_link(struct bt_bap_stream *stream,
>         return 0;
>  }
>
> +static void stream_unlink_ucast(void *data)
> +{
> +       struct bt_bap_stream *link = data;
> +
> +       DBG(link->bap, "stream %p unlink", link);
> +
> +       queue_destroy(link->links, NULL);
> +       link->links = NULL;
> +}
> +
> +static int bap_ucast_io_unlink(struct bt_bap_stream *stream,
> +                                               struct bt_bap_stream *link)
> +{
> +       if (!stream)
> +               return -EINVAL;
> +
> +       queue_destroy(stream->links, stream_unlink_ucast);
> +       stream->links = NULL;
> +
> +       DBG(stream->bap, "stream %p unlink", stream);
> +       return 0;
> +
> +}
> +
>  static void stream_link(void *data, void *user_data)
>  {
>         struct bt_bap_stream *stream = (void *)data;
> @@ -2708,7 +2746,7 @@ static const struct bt_bap_stream_ops stream_ops[] = {
>                         bap_ucast_release, bap_ucast_detach,
>                         bap_ucast_set_io, bap_ucast_get_io,
>                         bap_ucast_io_dir, bap_ucast_io_link,
> -                       NULL),
> +                       bap_ucast_io_unlink),
>         STREAM_OPS(BT_BAP_SOURCE, bap_ucast_set_state,
>                         bap_ucast_get_state,
>                         bap_ucast_config, bap_ucast_qos, bap_ucast_enable,
> @@ -2718,7 +2756,7 @@ static const struct bt_bap_stream_ops stream_ops[] = {
>                         bap_ucast_release, bap_ucast_detach,
>                         bap_ucast_set_io, bap_ucast_get_io,
>                         bap_ucast_io_dir, bap_ucast_io_link,
> -                       NULL),
> +                       bap_ucast_io_unlink),
>         STREAM_OPS(BT_BAP_BCAST_SINK, bap_bcast_set_state,
>                         bap_bcast_get_state,
>                         bap_bcast_config, bap_bcast_qos, bap_bcast_sink_enable,
> @@ -5015,6 +5053,8 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
>                 ep->stream->cc = new0(struct iovec, 1);
>
>         util_iov_memcpy(ep->stream->cc, cfg->cc, cfg->cc_len);
> +
> +       ep->stream->need_reconfig = false;
>  }
>
>  static void bap_stream_config_cfm_cb(struct bt_bap_stream *stream, int err)
> @@ -5922,43 +5962,6 @@ bool bt_bap_pac_bcast_is_local(struct bt_bap *bap, struct bt_bap_pac *pac)
>         return false;
>  }
>
> -static bool find_ep_unused(const void *data, const void *user_data)
> -{
> -       const struct bt_bap_endpoint *ep = data;
> -       const struct match_pac *match = user_data;
> -
> -       if (ep->stream)
> -               return false;
> -
> -       if (match->rpac)
> -               return ep->dir == match->rpac->type;
> -       else
> -               return true;
> -}
> -
> -static bool find_ep_pacs(const void *data, const void *user_data)
> -{
> -       const struct bt_bap_endpoint *ep = data;
> -       const struct match_pac *match = user_data;
> -
> -       if (!ep->stream)
> -               return false;
> -
> -       if (ep->stream->lpac != match->lpac)
> -               return false;
> -
> -       if (ep->stream->rpac != match->rpac)
> -               return false;
> -
> -       switch (ep->state) {
> -       case BT_BAP_STREAM_STATE_CONFIG:
> -       case BT_BAP_STREAM_STATE_QOS:
> -               return true;
> -       }
> -
> -       return false;
> -}
> -
>  static bool find_ep_source(const void *data, const void *user_data)
>  {
>         const struct bt_bap_endpoint *ep = data;
> @@ -6136,6 +6139,48 @@ static struct bt_bap_stream *bap_bcast_stream_new(struct bt_bap *bap,
>         return stream;
>  }
>
> +static bool find_ep_ucast(const void *data, const void *user_data)
> +{
> +       const struct bt_bap_endpoint *ep = data;
> +       const struct match_pac *match = user_data;
> +
> +       if (ep->stream) {
> +               if (!ep->stream->client)
> +                       return false;
> +               if (ep->stream->client_active)
> +                       return false;
> +               if (!queue_isempty(ep->stream->pending_states))
> +                       return false;
> +
> +               switch (ep->stream->state) {
> +               case BT_BAP_STREAM_STATE_IDLE:
> +               case BT_BAP_STREAM_STATE_CONFIG:
> +               case BT_BAP_STREAM_STATE_QOS:
> +                       break;
> +               default:
> +                       return false;
> +               }
> +       }
> +
> +       if (ep->dir != match->rpac->type)
> +               return false;
> +
> +       switch (match->lpac->type) {
> +       case BT_BAP_SOURCE:
> +               if (ep->dir != BT_BAP_SINK)
> +                       return false;
> +               break;
> +       case BT_BAP_SINK:
> +               if (ep->dir != BT_BAP_SOURCE)
> +                       return false;
> +               break;
> +       default:
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
>  static struct bt_bap_stream *bap_ucast_stream_new(struct bt_bap *bap,
>                                         struct bt_bap_pac *lpac,
>                                         struct bt_bap_pac *rpac,
> @@ -6153,21 +6198,28 @@ static struct bt_bap_stream *bap_ucast_stream_new(struct bt_bap *bap,
>         match.lpac = lpac;
>         match.rpac = rpac;
>
> -       /* Check for existing stream */
> -       ep = queue_find(bap->remote_eps, find_ep_pacs, &match);
> +       /* Get free ASE */
> +       ep = queue_find(bap->remote_eps, find_ep_ucast, &match);
>         if (!ep) {
> -               /* Check for unused ASE */
> -               ep = queue_find(bap->remote_eps, find_ep_unused, &match);
> -               if (!ep) {
> -                       DBG(bap, "Unable to find unused ASE");
> -                       return NULL;
> -               }
> +               DBG(bap, "Unable to find usable ASE");
> +               return NULL;
>         }
>
>         stream = ep->stream;
> -       if (!stream)
> +       if (stream) {
> +               /* Replace lpac: the stream generally needs to be reconfigured
> +                * after this, otherwise things like codec config not match.
> +                */
> +               bap_stream_clear_cfm(stream);
> +               stream->lpac = lpac;
> +               util_iov_free(stream->cc, 1);
> +               stream->cc = util_iov_dup(data, 1);
> +               stream->need_reconfig = true;
> +       } else {
>                 stream = bap_stream_new(bap, ep, lpac, rpac, data, true);
> +       }
>
> +       stream->client_active = true;
>         return stream;
>  }
>
> @@ -6187,6 +6239,26 @@ struct bt_bap_stream *bt_bap_stream_new(struct bt_bap *bap,
>         return bap_bcast_stream_new(bap, lpac, pqos, data);
>  }
>
> +void bt_bap_stream_discard(struct bt_bap_stream *stream)
> +{
> +       if (!stream || !stream->client)
> +               return;
> +
> +       stream->client_active = false;
> +
> +       switch (stream->ep->state) {
> +       case BT_BAP_STREAM_STATE_IDLE:
> +       case BT_BAP_STREAM_STATE_RELEASING:
> +               break;
> +       case BT_BAP_STREAM_STATE_CONFIG:
> +               if (stream->ep->old_state == BT_BAP_STREAM_STATE_RELEASING)
> +                       break;
> +               /* Fallthrough */
> +       default:
> +               bt_bap_stream_release(stream, NULL, NULL);
> +       }
> +}
> +
>  struct bt_bap *bt_bap_stream_get_session(struct bt_bap_stream *stream)
>  {
>         if (!stream)
> diff --git a/src/shared/bap.h b/src/shared/bap.h
> index d10581428..5949eb4b1 100644
> --- a/src/shared/bap.h
> +++ b/src/shared/bap.h
> @@ -183,6 +183,8 @@ struct bt_bap_stream *bt_bap_stream_new(struct bt_bap *bap,
>                                         struct bt_bap_qos *pqos,
>                                         struct iovec *data);
>
> +void bt_bap_stream_discard(struct bt_bap_stream *stream);
> +
>  struct bt_bap *bt_bap_stream_get_session(struct bt_bap_stream *stream);
>  uint8_t bt_bap_stream_get_state(struct bt_bap_stream *stream);
>
> --
> 2.49.0
>
>