| Message ID | 20241025202141.158946-2-daniel.beer@igorinstitute.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d7bb2abed626a979037a042c02b9a4027c6eb943 |
| Headers | show |
| Series | [BlueZ,1/2] sink: clean up outstanding AVDTP requests if the stream goes away. | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | fail | 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." |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
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 > >
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
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
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
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
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;