[PATCH BlueZ 1/2] sink: clean up outstanding AVDTP requests if the stream goes away.

[PATCH BlueZ 1/2] sink: clean up outstanding AVDTP requests if the stream goes away.

[Daniel Beer]

If the stream goes IDLE while we have an outstanding request, connect_id
stays non-zero and is never cleared via a completion callback. As a
consequence, the profile on this device will never be connected
successfully again until BlueZ restarts.
---
 profiles/audio/sink.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/profiles/audio/sink.c b/profiles/audio/sink.c
index a547dcb41..77f195436 100644
--- a/profiles/audio/sink.c
+++ b/profiles/audio/sink.c
@@ -137,6 +137,11 @@ static void stream_state_changed(struct avdtp_stream *stream,
 	case AVDTP_STATE_IDLE:
 		btd_service_disconnecting_complete(sink->service, 0);
 
+		if (sink->connect_id > 0) {
+			a2dp_cancel(sink->connect_id);
+			sink->connect_id = 0;
+		}
+
 		if (sink->disconnect_id > 0) {
 			a2dp_cancel(sink->disconnect_id);
 			sink->disconnect_id = 0;
-- 
2.43.0

[PATCH BlueZ 2/2] source: clean up outstanding AVDTP requests if the stream goes away.

[Daniel Beer]

If the stream goes IDLE while we have an outstanding request, connect_id
stays non-zero and is never cleared via a completion callback. As a
consequence, the profile on this device will never be connected
successfully again until BlueZ restarts.
---
 profiles/audio/source.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/profiles/audio/source.c b/profiles/audio/source.c
index 9fac352c8..db777e86d 100644
--- a/profiles/audio/source.c
+++ b/profiles/audio/source.c
@@ -134,6 +134,11 @@ static void stream_state_changed(struct avdtp_stream *stream,
 	case AVDTP_STATE_IDLE:
 		btd_service_disconnecting_complete(source->service, 0);
 
+		if (source->connect_id > 0) {
+			a2dp_cancel(source->connect_id);
+			source->connect_id = 0;
+		}
+
 		if (source->disconnect_id > 0) {
 			a2dp_cancel(source->disconnect_id);
 			source->disconnect_id = 0;
-- 
2.43.0

Re: [PATCH BlueZ 2/2] source: clean up outstanding AVDTP requests if the stream goes away.

[Luiz Augusto von Dentz]

Hi Daniel,

On Fri, Oct 25, 2024 at 4:30 PM Daniel Beer
<daniel.beer@igorinstitute.com> wrote:
>
> If the stream goes IDLE while we have an outstanding request, connect_id
> stays non-zero and is never cleared via a completion callback. As a
> consequence, the profile on this device will never be connected
> successfully again until BlueZ restarts.
> ---
>  profiles/audio/source.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/profiles/audio/source.c b/profiles/audio/source.c
> index 9fac352c8..db777e86d 100644
> --- a/profiles/audio/source.c
> +++ b/profiles/audio/source.c
> @@ -134,6 +134,11 @@ static void stream_state_changed(struct avdtp_stream *stream,
>         case AVDTP_STATE_IDLE:
>                 btd_service_disconnecting_complete(source->service, 0);
>
> +               if (source->connect_id > 0) {
> +                       a2dp_cancel(source->connect_id);
> +                       source->connect_id = 0;
> +               }
> +

Is this really happening or is more of a fix based on disconnect_id?
If that really is happening then we need to fix the sink as well since
it appears to be doing the same, that said connect_id may be set with
a2dp_discover which can happen in AVDTP_STATE_IDLE so in theory that
can still be ongoing, anyway this code hasn't change in very long time
so I wonder if this is sort of workaround to specific model or
use-case we haven't tried before?

>                 if (source->disconnect_id > 0) {
>                         a2dp_cancel(source->disconnect_id);
>                         source->disconnect_id = 0;
> --
> 2.43.0
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/2] source: clean up outstanding AVDTP requests if the stream goes away.

[Daniel Beer]

On Mon, Oct 28, 2024 at 11:04:20AM -0400, Luiz Augusto von Dentz wrote:
> Hi Daniel,
> 
> On Fri, Oct 25, 2024 at 4:30 PM Daniel Beer
> <daniel.beer@igorinstitute.com> wrote:
> >
> > If the stream goes IDLE while we have an outstanding request, connect_id
> > stays non-zero and is never cleared via a completion callback. As a
> > consequence, the profile on this device will never be connected
> > successfully again until BlueZ restarts.
> > ---
> >  profiles/audio/source.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/profiles/audio/source.c b/profiles/audio/source.c
> > index 9fac352c8..db777e86d 100644
> > --- a/profiles/audio/source.c
> > +++ b/profiles/audio/source.c
> > @@ -134,6 +134,11 @@ static void stream_state_changed(struct avdtp_stream *stream,
> >         case AVDTP_STATE_IDLE:
> >                 btd_service_disconnecting_complete(source->service, 0);
> >
> > +               if (source->connect_id > 0) {
> > +                       a2dp_cancel(source->connect_id);
> > +                       source->connect_id = 0;
> > +               }
> > +
> 
> Is this really happening or is more of a fix based on disconnect_id?
> If that really is happening then we need to fix the sink as well since
> it appears to be doing the same, that said connect_id may be set with
> a2dp_discover which can happen in AVDTP_STATE_IDLE so in theory that
> can still be ongoing, anyway this code hasn't change in very long time
> so I wonder if this is sort of workaround to specific model or
> use-case we haven't tried before?

Hi Luiz,

Yes, it is really happening, and yes, the same problem occurs in sink.c
(see patch 1/1). We have a client who can reproduce the issue in
automated testing with Bluetooth A2DP devices, and who has tested the
fix above.

The symptom we notice is that PulseAudio complains about a timeout
connecting the A2DP profile, which is never attempted because
{sink,source}_setup_stream() never appears to do anything. It returns
immediately on the first line test because connect_id is non-zero and
stays that way forever.

Immediately before the sink/source code stops working we see a
communication failure in which the connection is dropped while
a2dp_discover is ongoing.

Cheers,
Daniel

> >                 if (source->disconnect_id > 0) {
> >                         a2dp_cancel(source->disconnect_id);
> >                         source->disconnect_id = 0;
> > --
> > 2.43.0
> >
> >
> 
> 
> -- 
> Luiz Augusto von Dentz

-- 
Daniel Beer
Director of Firmware Engineering at Igor Institute
daniel.beer@igorinstitute.com or +64-27-420-8101
Offices in Seattle, San Francisco, and Vancouver BC or (206) 494-3312

Re: [PATCH BlueZ 2/2] source: clean up outstanding AVDTP requests if the stream goes away.

[Luiz Augusto von Dentz]

Hi Daniel,

On Mon, Oct 28, 2024 at 1:11 PM Daniel Beer
<daniel.beer@igorinstitute.com> wrote:
>
> On Mon, Oct 28, 2024 at 11:04:20AM -0400, Luiz Augusto von Dentz wrote:
> > Hi Daniel,
> >
> > On Fri, Oct 25, 2024 at 4:30 PM Daniel Beer
> > <daniel.beer@igorinstitute.com> wrote:
> > >
> > > If the stream goes IDLE while we have an outstanding request, connect_id
> > > stays non-zero and is never cleared via a completion callback. As a
> > > consequence, the profile on this device will never be connected
> > > successfully again until BlueZ restarts.
> > > ---
> > >  profiles/audio/source.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/profiles/audio/source.c b/profiles/audio/source.c
> > > index 9fac352c8..db777e86d 100644
> > > --- a/profiles/audio/source.c
> > > +++ b/profiles/audio/source.c
> > > @@ -134,6 +134,11 @@ static void stream_state_changed(struct avdtp_stream *stream,
> > >         case AVDTP_STATE_IDLE:
> > >                 btd_service_disconnecting_complete(source->service, 0);
> > >
> > > +               if (source->connect_id > 0) {
> > > +                       a2dp_cancel(source->connect_id);
> > > +                       source->connect_id = 0;
> > > +               }
> > > +
> >
> > Is this really happening or is more of a fix based on disconnect_id?
> > If that really is happening then we need to fix the sink as well since
> > it appears to be doing the same, that said connect_id may be set with
> > a2dp_discover which can happen in AVDTP_STATE_IDLE so in theory that
> > can still be ongoing, anyway this code hasn't change in very long time
> > so I wonder if this is sort of workaround to specific model or
> > use-case we haven't tried before?
>
> Hi Luiz,
>
> Yes, it is really happening, and yes, the same problem occurs in sink.c
> (see patch 1/1). We have a client who can reproduce the issue in
> automated testing with Bluetooth A2DP devices, and who has tested the
> fix above.
>
> The symptom we notice is that PulseAudio complains about a timeout
> connecting the A2DP profile, which is never attempted because
> {sink,source}_setup_stream() never appears to do anything. It returns
> immediately on the first line test because connect_id is non-zero and
> stays that way forever.
>
> Immediately before the sink/source code stops working we see a
> communication failure in which the connection is dropped while
> a2dp_discover is ongoing.

Ok, then perhaps it is a good idea to have these applied, that said it
would have been great to have this type of test automation upstream in
the future so we can catch regressions if we ever change this logic
for some reason.

> Cheers,
> Daniel
>
> > >                 if (source->disconnect_id > 0) {
> > >                         a2dp_cancel(source->disconnect_id);
> > >                         source->disconnect_id = 0;
> > > --
> > > 2.43.0
> > >
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> --
> Daniel Beer
> Director of Firmware Engineering at Igor Institute
> daniel.beer@igorinstitute.com or +64-27-420-8101
> Offices in Seattle, San Francisco, and Vancouver BC or (206) 494-3312



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/2] source: clean up outstanding AVDTP requests if the stream goes away.

[Daniel Beer]

On Mon, Oct 28, 2024 at 01:37:30PM -0400, Luiz Augusto von Dentz wrote:
> Ok, then perhaps it is a good idea to have these applied, that said it
> would have been great to have this type of test automation upstream in
> the future so we can catch regressions if we ever change this logic
> for some reason.

Hi Luiz,

I would like to be able to share the test setup, but unfortunately it's
a difficult-to-replicate hardware setup plus some proprietary control
pieces.

I see that the patches failed a lint check due to the trailing period in
the commit message. Would you like me to resubmit, or are you happy to
edit those?

Cheers,
Daniel

-- 
Daniel Beer
Director of Firmware Engineering at Igor Institute
daniel.beer@igorinstitute.com or +64-27-420-8101
Offices in Seattle, San Francisco, and Vancouver BC or (206) 494-3312

Re: [PATCH BlueZ 2/2] source: clean up outstanding AVDTP requests if the stream goes away.

[Luiz Augusto von Dentz]

Hi Daniel,

On Mon, Oct 28, 2024 at 4:51 PM Daniel Beer
<daniel.beer@igorinstitute.com> wrote:
>
> On Mon, Oct 28, 2024 at 01:37:30PM -0400, Luiz Augusto von Dentz wrote:
> > Ok, then perhaps it is a good idea to have these applied, that said it
> > would have been great to have this type of test automation upstream in
> > the future so we can catch regressions if we ever change this logic
> > for some reason.
>
> Hi Luiz,
>
> I would like to be able to share the test setup, but unfortunately it's
> a difficult-to-replicate hardware setup plus some proprietary control
> pieces.

Ok, well I like to have it perhaps running under our test-runner
environment wit emulated controller to make it part of our CI, anyway
this probably require much more resources to put it together.

> I see that the patches failed a lint check due to the trailing period in
> the commit message. Would you like me to resubmit, or are you happy to
> edit those?

No need, Ive fixed them myself before pushing.

>
> Cheers,
> Daniel
>
> --
> Daniel Beer
> Director of Firmware Engineering at Igor Institute
> daniel.beer@igorinstitute.com or +64-27-420-8101
> Offices in Seattle, San Francisco, and Vancouver BC or (206) 494-3312



-- 
Luiz Augusto von Dentz

RE: [BlueZ,1/2] sink: clean up outstanding AVDTP requests if the stream goes away.

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

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

---Test result---

Test Summary:
CheckPatch                    PASS      0.91 seconds
GitLint                       FAIL      0.83 seconds
BuildEll                      PASS      25.55 seconds
BluezMake                     PASS      1648.59 seconds
MakeCheck                     PASS      13.47 seconds
MakeDistcheck                 PASS      180.02 seconds
CheckValgrind                 PASS      252.21 seconds
CheckSmatch                   PASS      354.83 seconds
bluezmakeextell               PASS      119.91 seconds
IncrementalBuild              PASS      2958.71 seconds
ScanBuild                     PASS      988.35 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,1/2] sink: clean up outstanding AVDTP requests if the stream goes away.

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T3 Title has trailing punctuation (.): "[BlueZ,1/2] sink: clean up outstanding AVDTP requests if the stream goes away."
[BlueZ,2/2] source: clean up outstanding AVDTP requests if the stream goes away.

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T3 Title has trailing punctuation (.): "[BlueZ,2/2] source: clean up outstanding AVDTP requests if the stream goes away."


---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ 1/2] sink: clean up outstanding AVDTP requests if the stream goes away.

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Sat, 26 Oct 2024 09:21:40 +1300 you wrote:
> If the stream goes IDLE while we have an outstanding request, connect_id
> stays non-zero and is never cleared via a completion callback. As a
> consequence, the profile on this device will never be connected
> successfully again until BlueZ restarts.
> ---
>  profiles/audio/sink.c | 5 +++++
>  1 file changed, 5 insertions(+)

Here is the summary with links:
  - [BlueZ,1/2] sink: clean up outstanding AVDTP requests if the stream goes away.
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=fa1f2e5ee14d
  - [BlueZ,2/2] source: clean up outstanding AVDTP requests if the stream goes away.
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=d7bb2abed626

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html