ke, 2024-10-30 kello 23:46 +0200, Pauli Virtanen kirjoitti:
> Hi Luiz,
>
> ke, 2024-10-30 kello 16:56 -0400, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> >
> > On Mon, Oct 28, 2024 at 1:48 PM Pauli Virtanen <pav@xxxxxx> wrote:
> > >
> > > 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.
> >
> > I guess you are saying that we don't want the stream to transit to
> > idle since that would cause the AVDTP session to be disconnected? It
> > seems this is the result of session->dc_timeout being set to 0 on
> > avdtp_abort, anyway now looking a little more in detail it seems
> > clearer that the transport methods shouldn't affect the state machine
> > other than for suspend/resume procedures, that said then we need to
> > check what happens if those methods are called in quick succession e.g
> > Acquire -> Release -> Acquire.
>
> It should be possible to make it work so that SetConfiguration() while
> either Acquire() or Release() is pending sends ABORT if needed, but
> Acquire()+Release() alone only does OPEN->STREAMING->OPEN.
>
> The code that closes/aborts the existing stream in a2dp:a2dp_reconfig()
> may need some tuning, I'll need to test this race condition to make
> sure.
For the rapid Acquire() / Release() / Acquire() the intended behavior
probably is:
- while Acquire() call pending:
Release() makes the pending Acquire() return error, and suspends
after pending resume completes
Acquire() returns error
- while Release() call pending:
Release() returns error
Acquire() makes the pending Release() return error, and resumes
after pending suspend completes
so they just control the target state, and the state machine emits
START/SUSPEND as needed to reach the target.
(In this patch pending Release() was supposed to block Acquire() but
maybe we want it to instead work symmetrically like above.)
>
> > > > 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
> > > > >
> > > > >
> > > >
> > > >
> > >
> >
> >
>
