diff mbox series

[BlueZ,v2,1/2] shared/bap: detach io for source ASEs only after Stop Ready

Message ID 09443d89e7486d890b346d47ebc5c6a8f5eb30af.1688323254.git.pav@iki.fi (mailing list archive)
State Accepted
Commit 7b10e72de6f41585f087e6fc338106b44d3e69c9
Headers show
Series [BlueZ,v2,1/2] shared/bap: detach io for source ASEs only after Stop Ready | expand

Checks

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 25: B2 Line has trailing whitespace: " " 28: B2 Line has trailing whitespace: " "
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Pauli Virtanen July 2, 2023, 6:43 p.m. UTC
The Client may terminate a CIS when sink is in QOS and source in
Disabling states (BAP v1.0.1 Sec 5.6.5).  It may also terminate it when
Receiver Stop Ready has completed successfully (BAP v1.0.1 Sec 5.6.5.1).

It appears Samsung Galaxy Buds2 Pro (R510XXUOAWA5) ignores the Receiver
Stop Ready command if CIS is already disconnected, and then gets stuck
in disabling state. It works if CIS is disconnected after Receiver Stop
Ready.

For better compatibility as client for this device, and since it
shouldn't matter for us in which order we do it, disconnect CIS after
completion of Receiver Stop Ready, instead of immediately in Disabling.

We disconnect also if Receiver Stop Ready fails, given that
disconnecting in Disabled state should be OK.

Link: https://github.com/bluez/bluez/issues/516
---

Notes:
    v2: Disconnect when Receiver Stop has completed.
        Keep BAP server behavior unchanged.
    
    I don't have access to PTS, so if it's needed to verify what it does,
    someone else must do it.
    
    Assuming this device was PTS tested, I'd guess its test case does not
    disconnect CIS immediately in Disabling.

 src/shared/bap.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

Comments

bluez.test.bot@gmail.com July 2, 2023, 8:17 p.m. UTC | #1
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=761842

---Test result---

Test Summary:
CheckPatch                    PASS      1.04 seconds
GitLint                       FAIL      0.88 seconds
BuildEll                      PASS      26.33 seconds
BluezMake                     PASS      754.04 seconds
MakeCheck                     PASS      11.14 seconds
MakeDistcheck                 PASS      152.44 seconds
CheckValgrind                 PASS      246.87 seconds
CheckSmatch                   PASS      331.78 seconds
bluezmakeextell               PASS      100.51 seconds
IncrementalBuild              PASS      1272.53 seconds
ScanBuild                     PASS      992.76 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,v2,1/2] shared/bap: detach io for source ASEs only after Stop Ready

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
25: B2 Line has trailing whitespace: "    "
28: B2 Line has trailing whitespace: "    "


---
Regards,
Linux Bluetooth
patchwork-bot+bluetooth@kernel.org July 5, 2023, 6 p.m. UTC | #2
Hello:

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

On Sun,  2 Jul 2023 21:43:04 +0300 you wrote:
> The Client may terminate a CIS when sink is in QOS and source in
> Disabling states (BAP v1.0.1 Sec 5.6.5).  It may also terminate it when
> Receiver Stop Ready has completed successfully (BAP v1.0.1 Sec 5.6.5.1).
> 
> It appears Samsung Galaxy Buds2 Pro (R510XXUOAWA5) ignores the Receiver
> Stop Ready command if CIS is already disconnected, and then gets stuck
> in disabling state. It works if CIS is disconnected after Receiver Stop
> Ready.
> 
> [...]

Here is the summary with links:
  - [BlueZ,v2,1/2] shared/bap: detach io for source ASEs only after Stop Ready
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7b10e72de6f4
  - [BlueZ,v2,2/2] bap: wait for CIG to become configurable before recreating CIS
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=8c3170190d6f

You are awesome, thank you!
Luiz Augusto von Dentz Aug. 31, 2023, 9:04 p.m. UTC | #3
Hi Pauli,

On Wed, Jul 5, 2023 at 11:18 AM <patchwork-bot+bluetooth@kernel.org> wrote:
>
> Hello:
>
> This series was applied to bluetooth/bluez.git (master)
> by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
>
> On Sun,  2 Jul 2023 21:43:04 +0300 you wrote:
> > The Client may terminate a CIS when sink is in QOS and source in
> > Disabling states (BAP v1.0.1 Sec 5.6.5).  It may also terminate it when
> > Receiver Stop Ready has completed successfully (BAP v1.0.1 Sec 5.6.5.1).
> >
> > It appears Samsung Galaxy Buds2 Pro (R510XXUOAWA5) ignores the Receiver
> > Stop Ready command if CIS is already disconnected, and then gets stuck
> > in disabling state. It works if CIS is disconnected after Receiver Stop
> > Ready.
> >
> > [...]
>
> Here is the summary with links:
>   - [BlueZ,v2,1/2] shared/bap: detach io for source ASEs only after Stop Ready
>     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7b10e72de6f4
>   - [BlueZ,v2,2/2] bap: wait for CIG to become configurable before recreating CIS
>     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=8c3170190d6f
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html

Looks like this one introduces a problem when using the emulator:

https://gist.github.com/Vudentz/5c7ef940fc97b054227559dcd47b99f7?permalink_comment_id=4677775#gistcomment-4677775

If I try to release then acquire then it won't trigger recreate logic,
I suspect this is due to bap_io_disconnected being called ahead of
states changes.

>
>
Pauli Virtanen Sept. 10, 2023, 4:31 p.m. UTC | #4
Hi Luiz,

to, 2023-08-31 kello 14:04 -0700, Luiz Augusto von Dentz kirjoitti:
> On Wed, Jul 5, 2023 at 11:18 AM <patchwork-bot+bluetooth@kernel.org> wrote:
> > 
> > Hello:
> > 
> > This series was applied to bluetooth/bluez.git (master)
> > by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
> > 
> > On Sun,  2 Jul 2023 21:43:04 +0300 you wrote:
> > > The Client may terminate a CIS when sink is in QOS and source in
> > > Disabling states (BAP v1.0.1 Sec 5.6.5).  It may also terminate it when
> > > Receiver Stop Ready has completed successfully (BAP v1.0.1 Sec 5.6.5.1).
> > > 
> > > It appears Samsung Galaxy Buds2 Pro (R510XXUOAWA5) ignores the Receiver
> > > Stop Ready command if CIS is already disconnected, and then gets stuck
> > > in disabling state. It works if CIS is disconnected after Receiver Stop
> > > Ready.
> > > 
> > > [...]
> > 
> > Here is the summary with links:
> >   - [BlueZ,v2,1/2] shared/bap: detach io for source ASEs only after Stop Ready
> >     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7b10e72de6f4
> >   - [BlueZ,v2,2/2] bap: wait for CIG to become configurable before recreating CIS
> >     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=8c3170190d6f
> > 
> > You are awesome, thank you!
> > --
> > Deet-doot-dot, I am a bot.
> > https://korg.docs.kernel.org/patchwork/pwbot.html
> 
> Looks like this one introduces a problem when using the emulator:
> 
> https://gist.github.com/Vudentz/5c7ef940fc97b054227559dcd47b99f7?permalink_comment_id=4677775#gistcomment-4677775
> 
> If I try to release then acquire then it won't trigger recreate logic,
> I suspect this is due to bap_io_disconnected being called ahead of
> states changes.

Sorry for the delay. Was this fixed by d06b912df5ab ("bap: Fix not
always calling bap_io_close on disconnect")? If not, I'll try to
reproduce.
Luiz Augusto von Dentz Sept. 12, 2023, 9:04 p.m. UTC | #5
Hi Pauli,

On Sun, Sep 10, 2023 at 9:31 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> to, 2023-08-31 kello 14:04 -0700, Luiz Augusto von Dentz kirjoitti:
> > On Wed, Jul 5, 2023 at 11:18 AM <patchwork-bot+bluetooth@kernel.org> wrote:
> > >
> > > Hello:
> > >
> > > This series was applied to bluetooth/bluez.git (master)
> > > by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
> > >
> > > On Sun,  2 Jul 2023 21:43:04 +0300 you wrote:
> > > > The Client may terminate a CIS when sink is in QOS and source in
> > > > Disabling states (BAP v1.0.1 Sec 5.6.5).  It may also terminate it when
> > > > Receiver Stop Ready has completed successfully (BAP v1.0.1 Sec 5.6.5.1).
> > > >
> > > > It appears Samsung Galaxy Buds2 Pro (R510XXUOAWA5) ignores the Receiver
> > > > Stop Ready command if CIS is already disconnected, and then gets stuck
> > > > in disabling state. It works if CIS is disconnected after Receiver Stop
> > > > Ready.
> > > >
> > > > [...]
> > >
> > > Here is the summary with links:
> > >   - [BlueZ,v2,1/2] shared/bap: detach io for source ASEs only after Stop Ready
> > >     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7b10e72de6f4
> > >   - [BlueZ,v2,2/2] bap: wait for CIG to become configurable before recreating CIS
> > >     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=8c3170190d6f
> > >
> > > You are awesome, thank you!
> > > --
> > > Deet-doot-dot, I am a bot.
> > > https://korg.docs.kernel.org/patchwork/pwbot.html
> >
> > Looks like this one introduces a problem when using the emulator:
> >
> > https://gist.github.com/Vudentz/5c7ef940fc97b054227559dcd47b99f7?permalink_comment_id=4677775#gistcomment-4677775
> >
> > If I try to release then acquire then it won't trigger recreate logic,
> > I suspect this is due to bap_io_disconnected being called ahead of
> > states changes.
>
> Sorry for the delay. Was this fixed by d06b912df5ab ("bap: Fix not
> always calling bap_io_close on disconnect")? If not, I'll try to
> reproduce.

Yeah, that should have been fixed already.

> --
> Pauli Virtanen
diff mbox series

Patch

diff --git a/src/shared/bap.c b/src/shared/bap.c
index cf5d810bb..13c76afe6 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -1168,18 +1168,6 @@  static bool match_stream_io(const void *data, const void *user_data)
 	return stream->io == io;
 }
 
-static void stream_stop_disabling(void *data, void *user_data)
-{
-	struct bt_bap_stream *stream = data;
-
-	if (stream->io || stream->ep->state != BT_ASCS_ASE_STATE_DISABLING)
-		return;
-
-	DBG(stream->bap, "stream %p", stream);
-
-	bt_bap_stream_stop(stream, NULL, NULL);
-}
-
 static bool bap_stream_io_detach(struct bt_bap_stream *stream)
 {
 	struct bt_bap_stream *link;
@@ -1198,9 +1186,6 @@  static bool bap_stream_io_detach(struct bt_bap_stream *stream)
 		/* Detach link if in QoS state */
 		if (link->ep->state == BT_ASCS_ASE_STATE_QOS)
 			bap_stream_io_detach(link);
-	} else {
-		/* Links without IO on disabling state shall be stopped. */
-		queue_foreach(stream->links, stream_stop_disabling, NULL);
 	}
 
 	stream_io_unref(io);
@@ -1244,6 +1229,15 @@  static struct bt_bap *bt_bap_ref_safe(struct bt_bap *bap)
 	return bt_bap_ref(bap);
 }
 
+static void stream_stop_complete(struct bt_bap_stream *stream, uint8_t code,
+					uint8_t reason,	void *user_data)
+{
+	DBG(stream->bap, "stream %p stop 0x%02x 0x%02x", stream, code, reason);
+
+	if (stream->ep->state == BT_ASCS_ASE_STATE_DISABLING)
+		bap_stream_io_detach(stream);
+}
+
 static void bap_stream_state_changed(struct bt_bap_stream *stream)
 {
 	struct bt_bap *bap = stream->bap;
@@ -1271,7 +1265,9 @@  static void bap_stream_state_changed(struct bt_bap_stream *stream)
 		bap_stream_update_io_links(stream);
 		break;
 	case BT_ASCS_ASE_STATE_DISABLING:
-		bap_stream_io_detach(stream);
+		/* As client, we detach after Receiver Stop Ready */
+		if (!stream->client)
+			bap_stream_io_detach(stream);
 		break;
 	case BT_ASCS_ASE_STATE_QOS:
 		if (stream->io && !stream->io->connecting)
@@ -1305,8 +1301,9 @@  static void bap_stream_state_changed(struct bt_bap_stream *stream)
 			bt_bap_stream_start(stream, NULL, NULL);
 		break;
 	case BT_ASCS_ASE_STATE_DISABLING:
-		if (!bt_bap_stream_get_io(stream))
-			bt_bap_stream_stop(stream, NULL, NULL);
+		/* Send Stop Ready, and detach IO after remote replies */
+		if (stream->client)
+			bt_bap_stream_stop(stream, stream_stop_complete, NULL);
 		break;
 	}