2020-10-25 17:51:37

by Marijn Suijten

[permalink] [raw]
Subject: [PATCH BlueZ] audio/a2dp: a2dp_channel should have a refcount on avdtp session

a2dp_channel keeps a reference to an avdtp session without incrementing
its refcount. Not only does this appear wrong, it causes unexpected
disconnections when the remote SEP responds with rejections.

During testing with an audio application disconnections are observed
when a codec config change through MediaEndpoint1.SetConfiguration
fails. As soon as BlueZ receives this failure from the peer the
corresponding a2dp_setup object is cleaned up which holds the last
refcount to an avdtp session, in turn starting the disconnect process.
An eventual open sink/source and transport have already closed by that
time and released their refcounts.

Adding refcounting semantics around a2dp_channel resolves the
disconnections and allows future calls on MediaEndpoint1 to safely
access the sesion stored within this channel.
---
profiles/audio/a2dp.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index cc4866b5b..0eac0135f 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -1507,6 +1507,9 @@ static void channel_free(void *data)

avdtp_remove_state_cb(chan->state_id);

+ if (chan->session)
+ avdtp_unref(chan->session);
+
queue_destroy(chan->seps, remove_remote_sep);
free(chan->last_used);
g_free(chan);
@@ -2065,7 +2068,7 @@ static void avdtp_state_cb(struct btd_device *dev, struct avdtp *session,
break;
case AVDTP_SESSION_STATE_CONNECTED:
if (!chan->session)
- chan->session = session;
+ chan->session = avdtp_ref(session);
load_remote_seps(chan);
break;
}
@@ -2145,6 +2148,7 @@ found:
channel_remove(chan);
return NULL;
}
+ avdtp_ref(chan->session);

return avdtp_ref(chan->session);
}
@@ -2165,6 +2169,7 @@ static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
error("Unable to create AVDTP session");
goto fail;
}
+ avdtp_ref(chan->session);
}

g_io_channel_unref(chan->io);
--
2.29.1

Marijn Suijten


2020-10-25 17:52:27

by bluez.test.bot

[permalink] [raw]
Subject: RE: [BlueZ] audio/a2dp: a2dp_channel should have a refcount on avdtp session

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

---Test result---

##############################
Test: CheckPatch - PASS

##############################
Test: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth

2020-10-27 00:00:24

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ] audio/a2dp: a2dp_channel should have a refcount on avdtp session

Hi Marijn,

On Sun, Oct 25, 2020 at 9:27 AM Marijn Suijten <[email protected]> wrote:
>
> a2dp_channel keeps a reference to an avdtp session without incrementing
> its refcount. Not only does this appear wrong, it causes unexpected
> disconnections when the remote SEP responds with rejections.
>
> During testing with an audio application disconnections are observed
> when a codec config change through MediaEndpoint1.SetConfiguration
> fails. As soon as BlueZ receives this failure from the peer the
> corresponding a2dp_setup object is cleaned up which holds the last
> refcount to an avdtp session, in turn starting the disconnect process.
> An eventual open sink/source and transport have already closed by that
> time and released their refcounts.
>
> Adding refcounting semantics around a2dp_channel resolves the
> disconnections and allows future calls on MediaEndpoint1 to safely
> access the sesion stored within this channel.
> ---
> profiles/audio/a2dp.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index cc4866b5b..0eac0135f 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -1507,6 +1507,9 @@ static void channel_free(void *data)
>
> avdtp_remove_state_cb(chan->state_id);
>
> + if (chan->session)
> + avdtp_unref(chan->session);
> +
> queue_destroy(chan->seps, remove_remote_sep);
> free(chan->last_used);
> g_free(chan);
> @@ -2065,7 +2068,7 @@ static void avdtp_state_cb(struct btd_device *dev, struct avdtp *session,
> break;
> case AVDTP_SESSION_STATE_CONNECTED:
> if (!chan->session)
> - chan->session = session;
> + chan->session = avdtp_ref(session);

Afaik this was done on purpose since we only need a weak reference as
taking a reference would prevent the session to be disconnected when
there is no setup in place, so I pretty sure this will cause
regressions, instead we should probably add a reference when
reconfiguring is in place and have a grace period for switching to
another codec.

> load_remote_seps(chan);
> break;
> }
> @@ -2145,6 +2148,7 @@ found:
> channel_remove(chan);
> return NULL;
> }
> + avdtp_ref(chan->session);
>
> return avdtp_ref(chan->session);
> }
> @@ -2165,6 +2169,7 @@ static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
> error("Unable to create AVDTP session");
> goto fail;
> }
> + avdtp_ref(chan->session);
> }
>
> g_io_channel_unref(chan->io);
> --
> 2.29.1
>
> Marijn Suijten



--
Luiz Augusto von Dentz

2020-10-27 00:00:48

by Marijn Suijten

[permalink] [raw]
Subject: Re: [PATCH BlueZ] audio/a2dp: a2dp_channel should have a refcount on avdtp session

Hi Luiz,

On Mon, 26 Oct 2020 at 21:15, Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Marijn,
>
> On Sun, Oct 25, 2020 at 9:27 AM Marijn Suijten <[email protected]> wrote:
> >
> > a2dp_channel keeps a reference to an avdtp session without incrementing
> > its refcount. Not only does this appear wrong, it causes unexpected
> > disconnections when the remote SEP responds with rejections.
> >
> > During testing with an audio application disconnections are observed
> > when a codec config change through MediaEndpoint1.SetConfiguration
> > fails. As soon as BlueZ receives this failure from the peer the
> > corresponding a2dp_setup object is cleaned up which holds the last
> > refcount to an avdtp session, in turn starting the disconnect process.
> > An eventual open sink/source and transport have already closed by that
> > time and released their refcounts.
> >
> > Adding refcounting semantics around a2dp_channel resolves the
> > disconnections and allows future calls on MediaEndpoint1 to safely
> > access the sesion stored within this channel.
> > ---
> > profiles/audio/a2dp.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > index cc4866b5b..0eac0135f 100644
> > --- a/profiles/audio/a2dp.c
> > +++ b/profiles/audio/a2dp.c
> > @@ -1507,6 +1507,9 @@ static void channel_free(void *data)
> >
> > avdtp_remove_state_cb(chan->state_id);
> >
> > + if (chan->session)
> > + avdtp_unref(chan->session);
> > +
> > queue_destroy(chan->seps, remove_remote_sep);
> > free(chan->last_used);
> > g_free(chan);
> > @@ -2065,7 +2068,7 @@ static void avdtp_state_cb(struct btd_device *dev, struct avdtp *session,
> > break;
> > case AVDTP_SESSION_STATE_CONNECTED:
> > if (!chan->session)
> > - chan->session = session;
> > + chan->session = avdtp_ref(session);
>
> Afaik this was done on purpose since we only need a weak reference as
> taking a reference would prevent the session to be disconnected when
> there is no setup in place, so I pretty sure this will cause
> regressions, instead we should probably add a reference when
> reconfiguring is in place and have a grace period for switching to
> another codec.

Allright, so it's either an in-progress setup or ongoing transport
keeping the session alive, nothing else? I guess this is as simple as
setting a higher dc_timeout when calling set_disconnect_timer in
avdtp_unref?

>
> > load_remote_seps(chan);
> > break;
> > }
> > @@ -2145,6 +2148,7 @@ found:
> > channel_remove(chan);
> > return NULL;
> > }
> > + avdtp_ref(chan->session);
> >
> > return avdtp_ref(chan->session);
> > }
> > @@ -2165,6 +2169,7 @@ static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
> > error("Unable to create AVDTP session");
> > goto fail;
> > }
> > + avdtp_ref(chan->session);
> > }
> >
> > g_io_channel_unref(chan->io);
> > --
> > 2.29.1
> >
> > Marijn Suijten
>
>
>
> --
> Luiz Augusto von Dentz

Marijn Suijten

2020-10-27 00:01:45

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ] audio/a2dp: a2dp_channel should have a refcount on avdtp session

Hi Marijn,

On Mon, Oct 26, 2020 at 1:22 PM Marijn Suijten <[email protected]> wrote:
>
> Hi Luiz,
>
> On Mon, 26 Oct 2020 at 21:15, Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Marijn,
> >
> > On Sun, Oct 25, 2020 at 9:27 AM Marijn Suijten <[email protected]> wrote:
> > >
> > > a2dp_channel keeps a reference to an avdtp session without incrementing
> > > its refcount. Not only does this appear wrong, it causes unexpected
> > > disconnections when the remote SEP responds with rejections.
> > >
> > > During testing with an audio application disconnections are observed
> > > when a codec config change through MediaEndpoint1.SetConfiguration
> > > fails. As soon as BlueZ receives this failure from the peer the
> > > corresponding a2dp_setup object is cleaned up which holds the last
> > > refcount to an avdtp session, in turn starting the disconnect process.
> > > An eventual open sink/source and transport have already closed by that
> > > time and released their refcounts.
> > >
> > > Adding refcounting semantics around a2dp_channel resolves the
> > > disconnections and allows future calls on MediaEndpoint1 to safely
> > > access the sesion stored within this channel.
> > > ---
> > > profiles/audio/a2dp.c | 7 ++++++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > > index cc4866b5b..0eac0135f 100644
> > > --- a/profiles/audio/a2dp.c
> > > +++ b/profiles/audio/a2dp.c
> > > @@ -1507,6 +1507,9 @@ static void channel_free(void *data)
> > >
> > > avdtp_remove_state_cb(chan->state_id);
> > >
> > > + if (chan->session)
> > > + avdtp_unref(chan->session);
> > > +
> > > queue_destroy(chan->seps, remove_remote_sep);
> > > free(chan->last_used);
> > > g_free(chan);
> > > @@ -2065,7 +2068,7 @@ static void avdtp_state_cb(struct btd_device *dev, struct avdtp *session,
> > > break;
> > > case AVDTP_SESSION_STATE_CONNECTED:
> > > if (!chan->session)
> > > - chan->session = session;
> > > + chan->session = avdtp_ref(session);
> >
> > Afaik this was done on purpose since we only need a weak reference as
> > taking a reference would prevent the session to be disconnected when
> > there is no setup in place, so I pretty sure this will cause
> > regressions, instead we should probably add a reference when
> > reconfiguring is in place and have a grace period for switching to
> > another codec.
>
> Allright, so it's either an in-progress setup or ongoing transport
> keeping the session alive, nothing else? I guess this is as simple as
> setting a higher dc_timeout when calling set_disconnect_timer in
> avdtp_unref?

Yep, actually now that you mentioned the dc_timeout that should have
prevented us to disconnect immediately when reconfiguring as we should
have been in AVDTP_SESSION_STATE_CONNECTED state but I think the
problem is that we use avdtp_close to reconfigure which does set the
dc_timeout to 0 since that is normally used for cleaning up the
session, so I think we may need to restore the dc_timeout to
DISCONNECT_TIMEOUT if we attempt to set a configuration:

diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index ae93fb26f..97b4d1b44 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -3498,6 +3498,7 @@ int avdtp_set_configuration(struct avdtp *session,
session->streams = g_slist_append(session->streams, new_stream);
if (stream)
*stream = new_stream;
+ session->dc_timeout = DISCONNECT_TIMEOUT;
}

g_free(req);


> >
> > > load_remote_seps(chan);
> > > break;
> > > }
> > > @@ -2145,6 +2148,7 @@ found:
> > > channel_remove(chan);
> > > return NULL;
> > > }
> > > + avdtp_ref(chan->session);
> > >
> > > return avdtp_ref(chan->session);
> > > }
> > > @@ -2165,6 +2169,7 @@ static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
> > > error("Unable to create AVDTP session");
> > > goto fail;
> > > }
> > > + avdtp_ref(chan->session);
> > > }
> > >
> > > g_io_channel_unref(chan->io);
> > > --
> > > 2.29.1
> > >
> > > Marijn Suijten
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Marijn Suijten



--
Luiz Augusto von Dentz

2020-10-27 00:04:39

by Marijn Suijten

[permalink] [raw]
Subject: Re: [PATCH BlueZ] audio/a2dp: a2dp_channel should have a refcount on avdtp session

Hi Luiz,

On Mon, 26 Oct 2020 at 21:39, Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Marijn,
>
> On Mon, Oct 26, 2020 at 1:22 PM Marijn Suijten <[email protected]> wrote:
> >
> > Hi Luiz,
> >
> > On Mon, 26 Oct 2020 at 21:15, Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > Hi Marijn,
> > >
> > > On Sun, Oct 25, 2020 at 9:27 AM Marijn Suijten <[email protected]> wrote:
> > > >
> > > > a2dp_channel keeps a reference to an avdtp session without incrementing
> > > > its refcount. Not only does this appear wrong, it causes unexpected
> > > > disconnections when the remote SEP responds with rejections.
> > > >
> > > > During testing with an audio application disconnections are observed
> > > > when a codec config change through MediaEndpoint1.SetConfiguration
> > > > fails. As soon as BlueZ receives this failure from the peer the
> > > > corresponding a2dp_setup object is cleaned up which holds the last
> > > > refcount to an avdtp session, in turn starting the disconnect process.
> > > > An eventual open sink/source and transport have already closed by that
> > > > time and released their refcounts.
> > > >
> > > > Adding refcounting semantics around a2dp_channel resolves the
> > > > disconnections and allows future calls on MediaEndpoint1 to safely
> > > > access the sesion stored within this channel.
> > > > ---
> > > > profiles/audio/a2dp.c | 7 ++++++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > > > index cc4866b5b..0eac0135f 100644
> > > > --- a/profiles/audio/a2dp.c
> > > > +++ b/profiles/audio/a2dp.c
> > > > @@ -1507,6 +1507,9 @@ static void channel_free(void *data)
> > > >
> > > > avdtp_remove_state_cb(chan->state_id);
> > > >
> > > > + if (chan->session)
> > > > + avdtp_unref(chan->session);
> > > > +
> > > > queue_destroy(chan->seps, remove_remote_sep);
> > > > free(chan->last_used);
> > > > g_free(chan);
> > > > @@ -2065,7 +2068,7 @@ static void avdtp_state_cb(struct btd_device *dev, struct avdtp *session,
> > > > break;
> > > > case AVDTP_SESSION_STATE_CONNECTED:
> > > > if (!chan->session)
> > > > - chan->session = session;
> > > > + chan->session = avdtp_ref(session);
> > >
> > > Afaik this was done on purpose since we only need a weak reference as
> > > taking a reference would prevent the session to be disconnected when
> > > there is no setup in place, so I pretty sure this will cause
> > > regressions, instead we should probably add a reference when
> > > reconfiguring is in place and have a grace period for switching to
> > > another codec.
> >
> > Allright, so it's either an in-progress setup or ongoing transport
> > keeping the session alive, nothing else? I guess this is as simple as
> > setting a higher dc_timeout when calling set_disconnect_timer in
> > avdtp_unref?
>
> Yep, actually now that you mentioned the dc_timeout that should have
> prevented us to disconnect immediately when reconfiguring as we should
> have been in AVDTP_SESSION_STATE_CONNECTED state but I think the
> problem is that we use avdtp_close to reconfigure which does set the
> dc_timeout to 0

Yep a2dp_reconfig calls that functionand sets it to 0.

> since that is normally used for cleaning up the
> session, so I think we may need to restore the dc_timeout to
> DISCONNECT_TIMEOUT if we attempt to set a configuration:

My thinking as well, assuming DISCONNECT_TIMEOUT is enough (1s).

> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index ae93fb26f..97b4d1b44 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -3498,6 +3498,7 @@ int avdtp_set_configuration(struct avdtp *session,
> session->streams = g_slist_append(session->streams, new_stream);
> if (stream)
> *stream = new_stream;
> + session->dc_timeout = DISCONNECT_TIMEOUT;
> }
>
> g_free(req);
>
>

Thanks, works like a charm!

profiles/audio/avdtp.c:avdtp_unref() 0x5565f10db6f0: ref=1
profiles/audio/avdtp.c:avdtp_set_configuration() 0x5565f10db6f0:
int_seid=8, acp_seid=1
profiles/audio/avdtp.c:session_cb()
profiles/audio/avdtp.c:avdtp_parse_rej() SET_CONFIGURATION request
rejected: Configuration not supported (41)
profiles/audio/a2dp.c:setconf_cfm() Source 0x5565f10fc620:
Set_Configuration_Cfm
profiles/audio/a2dp.c:setup_ref() 0x5565f110d540: ref=2
profiles/audio/a2dp.c:setup_unref() 0x5565f110d540: ref=1
profiles/audio/a2dp.c:setup_unref() 0x5565f110d540: ref=0
profiles/audio/a2dp.c:setup_free() 0x5565f110d540
profiles/audio/avdtp.c:avdtp_unref() 0x5565f10db6f0: ref=0
profiles/audio/avdtp.c:avdtp_ref() 0x5565f10db6f0: ref=1
profiles/audio/avdtp.c:set_disconnect_timer() timeout 1
profiles/audio/avdtp.c:avdtp_ref() 0x5565f10db6f0: ref=2
profiles/audio/avdtp.c:avdtp_unref() 0x5565f10db6f0: ref=1
profiles/audio/a2dp.c:setup_ref() 0x5565f110d540: ref=1
profiles/audio/avdtp.c:avdtp_set_configuration() 0x5565f10db6f0:
int_seid=8, acp_seid=7
profiles/audio/avdtp.c:session_cb()
profiles/audio/avdtp.c:avdtp_parse_resp() SET_CONFIGURATION
request succeeded
profiles/audio/a2dp.c:setconf_cfm() Source 0x5565f10fc620:
Set_Configuration_Cfm

Since you suggested this change I guess it's up to you to write a
commit around it and apply it or submit it for additional review, or
should I do it and combine it with the original message for a v2?

> > >
> > > > load_remote_seps(chan);
> > > > break;
> > > > }
> > > > @@ -2145,6 +2148,7 @@ found:
> > > > channel_remove(chan);
> > > > return NULL;
> > > > }
> > > > + avdtp_ref(chan->session);
> > > >
> > > > return avdtp_ref(chan->session);
> > > > }
> > > > @@ -2165,6 +2169,7 @@ static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
> > > > error("Unable to create AVDTP session");
> > > > goto fail;
> > > > }
> > > > + avdtp_ref(chan->session);
> > > > }
> > > >
> > > > g_io_channel_unref(chan->io);
> > > > --
> > > > 2.29.1
> > > >
> > > > Marijn Suijten
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > Marijn Suijten
>
>
>
> --
> Luiz Augusto von Dentz

Marijn Suijten

2020-10-27 00:11:46

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ] audio/a2dp: a2dp_channel should have a refcount on avdtp session

Hi Marijn,

On Mon, Oct 26, 2020 at 2:37 PM Marijn Suijten <[email protected]> wrote:
>
> Hi Luiz,
>
> On Mon, 26 Oct 2020 at 21:39, Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Marijn,
> >
> > On Mon, Oct 26, 2020 at 1:22 PM Marijn Suijten <[email protected]> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Mon, 26 Oct 2020 at 21:15, Luiz Augusto von Dentz
> > > <[email protected]> wrote:
> > > >
> > > > Hi Marijn,
> > > >
> > > > On Sun, Oct 25, 2020 at 9:27 AM Marijn Suijten <[email protected]> wrote:
> > > > >
> > > > > a2dp_channel keeps a reference to an avdtp session without incrementing
> > > > > its refcount. Not only does this appear wrong, it causes unexpected
> > > > > disconnections when the remote SEP responds with rejections.
> > > > >
> > > > > During testing with an audio application disconnections are observed
> > > > > when a codec config change through MediaEndpoint1.SetConfiguration
> > > > > fails. As soon as BlueZ receives this failure from the peer the
> > > > > corresponding a2dp_setup object is cleaned up which holds the last
> > > > > refcount to an avdtp session, in turn starting the disconnect process.
> > > > > An eventual open sink/source and transport have already closed by that
> > > > > time and released their refcounts.
> > > > >
> > > > > Adding refcounting semantics around a2dp_channel resolves the
> > > > > disconnections and allows future calls on MediaEndpoint1 to safely
> > > > > access the sesion stored within this channel.
> > > > > ---
> > > > > profiles/audio/a2dp.c | 7 ++++++-
> > > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > > > > index cc4866b5b..0eac0135f 100644
> > > > > --- a/profiles/audio/a2dp.c
> > > > > +++ b/profiles/audio/a2dp.c
> > > > > @@ -1507,6 +1507,9 @@ static void channel_free(void *data)
> > > > >
> > > > > avdtp_remove_state_cb(chan->state_id);
> > > > >
> > > > > + if (chan->session)
> > > > > + avdtp_unref(chan->session);
> > > > > +
> > > > > queue_destroy(chan->seps, remove_remote_sep);
> > > > > free(chan->last_used);
> > > > > g_free(chan);
> > > > > @@ -2065,7 +2068,7 @@ static void avdtp_state_cb(struct btd_device *dev, struct avdtp *session,
> > > > > break;
> > > > > case AVDTP_SESSION_STATE_CONNECTED:
> > > > > if (!chan->session)
> > > > > - chan->session = session;
> > > > > + chan->session = avdtp_ref(session);
> > > >
> > > > Afaik this was done on purpose since we only need a weak reference as
> > > > taking a reference would prevent the session to be disconnected when
> > > > there is no setup in place, so I pretty sure this will cause
> > > > regressions, instead we should probably add a reference when
> > > > reconfiguring is in place and have a grace period for switching to
> > > > another codec.
> > >
> > > Allright, so it's either an in-progress setup or ongoing transport
> > > keeping the session alive, nothing else? I guess this is as simple as
> > > setting a higher dc_timeout when calling set_disconnect_timer in
> > > avdtp_unref?
> >
> > Yep, actually now that you mentioned the dc_timeout that should have
> > prevented us to disconnect immediately when reconfiguring as we should
> > have been in AVDTP_SESSION_STATE_CONNECTED state but I think the
> > problem is that we use avdtp_close to reconfigure which does set the
> > dc_timeout to 0
>
> Yep a2dp_reconfig calls that functionand sets it to 0.
>
> > since that is normally used for cleaning up the
> > session, so I think we may need to restore the dc_timeout to
> > DISCONNECT_TIMEOUT if we attempt to set a configuration:
>
> My thinking as well, assuming DISCONNECT_TIMEOUT is enough (1s).
>
> > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> > index ae93fb26f..97b4d1b44 100644
> > --- a/profiles/audio/avdtp.c
> > +++ b/profiles/audio/avdtp.c
> > @@ -3498,6 +3498,7 @@ int avdtp_set_configuration(struct avdtp *session,
> > session->streams = g_slist_append(session->streams, new_stream);
> > if (stream)
> > *stream = new_stream;
> > + session->dc_timeout = DISCONNECT_TIMEOUT;
> > }
> >
> > g_free(req);
> >
> >
>
> Thanks, works like a charm!
>
> profiles/audio/avdtp.c:avdtp_unref() 0x5565f10db6f0: ref=1
> profiles/audio/avdtp.c:avdtp_set_configuration() 0x5565f10db6f0:
> int_seid=8, acp_seid=1
> profiles/audio/avdtp.c:session_cb()
> profiles/audio/avdtp.c:avdtp_parse_rej() SET_CONFIGURATION request
> rejected: Configuration not supported (41)
> profiles/audio/a2dp.c:setconf_cfm() Source 0x5565f10fc620:
> Set_Configuration_Cfm
> profiles/audio/a2dp.c:setup_ref() 0x5565f110d540: ref=2
> profiles/audio/a2dp.c:setup_unref() 0x5565f110d540: ref=1
> profiles/audio/a2dp.c:setup_unref() 0x5565f110d540: ref=0
> profiles/audio/a2dp.c:setup_free() 0x5565f110d540
> profiles/audio/avdtp.c:avdtp_unref() 0x5565f10db6f0: ref=0
> profiles/audio/avdtp.c:avdtp_ref() 0x5565f10db6f0: ref=1
> profiles/audio/avdtp.c:set_disconnect_timer() timeout 1
> profiles/audio/avdtp.c:avdtp_ref() 0x5565f10db6f0: ref=2
> profiles/audio/avdtp.c:avdtp_unref() 0x5565f10db6f0: ref=1
> profiles/audio/a2dp.c:setup_ref() 0x5565f110d540: ref=1
> profiles/audio/avdtp.c:avdtp_set_configuration() 0x5565f10db6f0:
> int_seid=8, acp_seid=7
> profiles/audio/avdtp.c:session_cb()
> profiles/audio/avdtp.c:avdtp_parse_resp() SET_CONFIGURATION
> request succeeded
> profiles/audio/a2dp.c:setconf_cfm() Source 0x5565f10fc620:
> Set_Configuration_Cfm
>
> Since you suggested this change I guess it's up to you to write a
> commit around it and apply it or submit it for additional review, or
> should I do it and combine it with the original message for a v2?

I went ahead and applied it myself, thanks for confirming it works properly.

> > > >
> > > > > load_remote_seps(chan);
> > > > > break;
> > > > > }
> > > > > @@ -2145,6 +2148,7 @@ found:
> > > > > channel_remove(chan);
> > > > > return NULL;
> > > > > }
> > > > > + avdtp_ref(chan->session);
> > > > >
> > > > > return avdtp_ref(chan->session);
> > > > > }
> > > > > @@ -2165,6 +2169,7 @@ static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
> > > > > error("Unable to create AVDTP session");
> > > > > goto fail;
> > > > > }
> > > > > + avdtp_ref(chan->session);
> > > > > }
> > > > >
> > > > > g_io_channel_unref(chan->io);
> > > > > --
> > > > > 2.29.1
> > > > >
> > > > > Marijn Suijten
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> > >
> > > Marijn Suijten
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Marijn Suijten



--
Luiz Augusto von Dentz