Hi Luiz,
ma, 2024-10-28 kello 10:39 -0400, Luiz Augusto von Dentz kirjoitti:
> On Sat, Oct 26, 2024 at 6:41 AM Pauli Virtanen <pav@xxxxxx> wrote:
> >
> > User can cancel transport acquire by calling Release() while Acquire()
> > is in progress. This calls a2dp_cancel() which sends AVDTP_ABORT_CMD,
> > forcing AVDTP state transition to IDLE, and disconnecting A2DP profile,
> > which is not desired.
> >
> > Fix by instead waiting until START completes and then send SUSPEND. Make
> > Release() reply only after the whole sequence completes.
>
> Hmm, but assumes the client is not reconfiguring to another endpoint
> or just giving up on the transport since it doesn't intend to use it
> anymore, anyway we can't really send anything else other than abort if
> we don't want to wait so I think this would be better to be handled by
I guess you have in mind
Acquire(t1)
Release(t1) # canceling acquire
SetConfiguration(ep2)
I don't think this works right in current BlueZ master branch either,
the Release() results to removing the DBus endpoints & AVDTP
disconnect, so nothing to SetConfigure() after abort completes.
SetConfiguration() probably fails also if called while Abort_Cfm is
pending, since it cannot then close the existing stream.
I think (but did not test) that with the patch here, it would end up
doing START -> CLOSE -> reconfigure, so indeed has to wait for START
completion.
> the application if it intends to suspend then it should wait acquire
> to complete and then release.
I think it's a bug in BlueZ that calling Release() on transport result
to disconnect of A2DP, instead of suspend.
I have another version of this patch that does send ABORT, but recalls
it was due to canceled suspend and reconnects the sink/source when
getting to IDLE.
> > Fix also sending error reply to the canceled Acquire() call, which was
> > missing previously.
> > ---
> >
> > Notes:
> > In theory we could also send ABORT and reconfigure the SEP again after
> > that. It's more hairy though as with how the code currently works, we
> > may need to complete discovery first. This is a corner case anyway, so
> > just waiting a bit should be easier.
> >
> > profiles/audio/transport.c | 152 +++++++++++++++++++++++++++----------
> > 1 file changed, 110 insertions(+), 42 deletions(-)
> >
> > diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> > index 0f7909a94..4d5afe022 100644
> > --- a/profiles/audio/transport.c
> > +++ b/profiles/audio/transport.c
> > @@ -88,6 +88,9 @@ struct a2dp_transport {
> > uint16_t delay;
> > int8_t volume;
> > guint watch;
> > + guint resume_id;
> > + gboolean cancel_resume;
> > + guint cancel_id;
> > };
> >
> > struct bap_transport {
> > @@ -393,22 +396,82 @@ static void *transport_a2dp_get_stream(struct media_transport *transport)
> > return a2dp_sep_get_stream(sep);
> > }
> >
> > +static void a2dp_suspend_complete(struct avdtp *session, int err,
> > + void *user_data)
> > +{
> > + struct media_owner *owner = user_data;
> > + struct media_transport *transport = owner->transport;
> > + struct a2dp_transport *a2dp = transport->data;
> > + struct a2dp_sep *sep = media_endpoint_get_sep(transport->endpoint);
> > +
> > + /* Release always succeeds */
> > + if (owner->pending) {
> > + owner->pending->id = 0;
> > + media_request_reply(owner->pending, 0);
> > + media_owner_remove(owner);
> > + }
> > +
> > + a2dp_sep_unlock(sep, a2dp->session);
> > + transport_set_state(transport, TRANSPORT_STATE_IDLE);
> > + media_transport_remove_owner(transport);
> > +}
> > +
> > +static guint transport_a2dp_suspend(struct media_transport *transport,
> > + struct media_owner *owner)
> > +{
> > + struct a2dp_transport *a2dp = transport->data;
> > + struct media_endpoint *endpoint = transport->endpoint;
> > + struct a2dp_sep *sep = media_endpoint_get_sep(endpoint);
> > +
> > + if (a2dp->cancel_resume)
> > + return a2dp->resume_id;
> > +
> > + if (owner != NULL)
> > + return a2dp_suspend(a2dp->session, sep, a2dp_suspend_complete,
> > + owner);
> > +
> > + transport_set_state(transport, TRANSPORT_STATE_IDLE);
> > + a2dp_sep_unlock(sep, a2dp->session);
> > +
> > + return 0;
> > +}
> > +
> > +static gboolean a2dp_cancel_resume_cb(void *user_data)
> > +{
> > + struct media_owner *owner = user_data;
> > + struct media_transport *transport = owner->transport;
> > + struct a2dp_transport *a2dp = transport->data;
> > +
> > + a2dp->cancel_id = 0;
> > + a2dp->cancel_resume = FALSE;
> > + owner->pending->id = transport_a2dp_suspend(transport, owner);
> > + return FALSE;
> > +}
> > +
> > static void a2dp_resume_complete(struct avdtp *session, int err,
> > void *user_data)
> > {
> > struct media_owner *owner = user_data;
> > struct media_request *req = owner->pending;
> > struct media_transport *transport = owner->transport;
> > + struct a2dp_transport *a2dp = transport->data;
> > struct avdtp_stream *stream;
> > int fd;
> > uint16_t imtu, omtu;
> > gboolean ret;
> >
> > + a2dp->resume_id = 0;
> > req->id = 0;
> >
> > if (err)
> > goto fail;
> >
> > + if (a2dp->cancel_resume) {
> > + DBG("cancel resume");
> > + a2dp->cancel_id = g_idle_add(a2dp_cancel_resume_cb, owner);
> > + return;
> > + }
> > +
> > stream = transport_a2dp_get_stream(transport);
> > if (stream == NULL)
> > goto fail;
> > @@ -445,15 +508,21 @@ static guint transport_a2dp_resume(struct media_transport *transport,
> > struct a2dp_sep *sep = media_endpoint_get_sep(endpoint);
> > guint id;
> >
> > + if (a2dp->resume_id || a2dp->cancel_id)
> > + return -EBUSY;
> > +
> > if (a2dp->session == NULL) {
> > a2dp->session = a2dp_avdtp_get(transport->device);
> > if (a2dp->session == NULL)
> > return 0;
> > }
> >
> > - if (state_in_use(transport->state))
> > - return a2dp_resume(a2dp->session, sep, a2dp_resume_complete,
> > + if (state_in_use(transport->state)) {
> > + id = a2dp_resume(a2dp->session, sep, a2dp_resume_complete,
> > owner);
> > + a2dp->resume_id = id;
> > + return id;
> > + }
> >
> > if (a2dp_sep_lock(sep, a2dp->session) == FALSE)
> > return 0;
> > @@ -468,51 +537,47 @@ static guint transport_a2dp_resume(struct media_transport *transport,
> > if (transport->state == TRANSPORT_STATE_IDLE)
> > transport_set_state(transport, TRANSPORT_STATE_REQUESTING);
> >
> > + a2dp->resume_id = id;
> > return id;
> > }
> >
> > -static void a2dp_suspend_complete(struct avdtp *session, int err,
> > - void *user_data)
> > -{
> > - struct media_owner *owner = user_data;
> > - struct media_transport *transport = owner->transport;
> > - struct a2dp_transport *a2dp = transport->data;
> > - struct a2dp_sep *sep = media_endpoint_get_sep(transport->endpoint);
> > -
> > - /* Release always succeeds */
> > - if (owner->pending) {
> > - owner->pending->id = 0;
> > - media_request_reply(owner->pending, 0);
> > - media_owner_remove(owner);
> > - }
> > -
> > - a2dp_sep_unlock(sep, a2dp->session);
> > - transport_set_state(transport, TRANSPORT_STATE_IDLE);
> > - media_transport_remove_owner(transport);
> > -}
> > -
> > -static guint transport_a2dp_suspend(struct media_transport *transport,
> > - struct media_owner *owner)
> > -{
> > - struct a2dp_transport *a2dp = transport->data;
> > - struct media_endpoint *endpoint = transport->endpoint;
> > - struct a2dp_sep *sep = media_endpoint_get_sep(endpoint);
> > -
> > - if (owner != NULL)
> > - return a2dp_suspend(a2dp->session, sep, a2dp_suspend_complete,
> > - owner);
> > -
> > - transport_set_state(transport, TRANSPORT_STATE_IDLE);
> > - a2dp_sep_unlock(sep, a2dp->session);
> > -
> > - return 0;
> > -}
> > -
> > static void transport_a2dp_cancel(struct media_transport *transport, guint id)
> > {
> > + struct a2dp_transport *a2dp = transport->data;
> > +
> > + if (a2dp->resume_id && a2dp->resume_id == id) {
> > + /* a2dp_cancel() results to ABORT->IDLE->disconnect. Canceling
> > + * START can be triggered by user via Release(), and it's better
> > + * to not drop the A2DP connection then, so we just suspend
> > + * after resume completes.
> > + */
> > + a2dp->cancel_resume = TRUE;
> > + return;
> > + }
> > +
> > a2dp_cancel(id);
> > }
> >
> > +static void transport_a2dp_remove_owner(struct media_transport *transport,
> > + struct media_owner *owner)
> > +{
> > + struct a2dp_transport *a2dp = transport->data;
> > +
> > + /* Clean up callbacks that refer to the owner */
> > +
> > + if (a2dp->cancel_id) {
> > + g_source_remove(a2dp->cancel_id);
> > + a2dp->cancel_id = 0;
> > + }
> > +
> > + if (a2dp->resume_id) {
> > + a2dp_cancel(a2dp->resume_id);
> > + a2dp->resume_id = 0;
> > + }
> > +
> > + a2dp->cancel_resume = FALSE;
> > +}
> > +
> > static int8_t transport_a2dp_get_volume(struct media_transport *transport)
> > {
> > struct a2dp_transport *a2dp = transport->data;
> > @@ -773,10 +838,12 @@ static DBusMessage *release(DBusConnection *conn, DBusMessage *msg,
> >
> > member = dbus_message_get_member(owner->pending->msg);
> > /* Cancel Acquire request if that exist */
> > - if (g_str_equal(member, "Acquire"))
> > + if (g_str_equal(member, "Acquire")) {
> > + media_request_reply(owner->pending, ECANCELED);
> > media_owner_remove(owner);
> > - else
> > + } else {
> > return btd_error_in_progress(msg);
> > + }
> > }
> >
> > transport_set_state(transport, TRANSPORT_STATE_SUSPENDING);
> > @@ -2189,7 +2256,8 @@ static void *transport_asha_init(struct media_transport *transport, void *data)
> > }
> >
> > #define A2DP_OPS(_uuid, _init, _set_volume, _destroy) \
> > - TRANSPORT_OPS(_uuid, transport_a2dp_properties, NULL, NULL, _init, \
> > + TRANSPORT_OPS(_uuid, transport_a2dp_properties, NULL, \
> > + transport_a2dp_remove_owner, _init, \
> > transport_a2dp_resume, transport_a2dp_suspend, \
> > transport_a2dp_cancel, NULL, \
> > transport_a2dp_get_stream, transport_a2dp_get_volume, \
> > --
> > 2.47.0
> >
> >
>
>
