* [PATCH BlueZ 1/3] avdtp: Fix setting disconnect timer when there is no local endpoints @ 2021-02-16 23:33 Luiz Augusto von Dentz 2021-02-16 23:33 ` [PATCH BlueZ 2/3] btio: Use G_PRIORITY_HIGH for watches Luiz Augusto von Dentz ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Luiz Augusto von Dentz @ 2021-02-16 23:33 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> If there are not local endpoints left there is no point in starting the disconnect timer as without any endpoint it is not possible to configure streams anymore so the code should proceed to disconnect immediately. --- profiles/audio/avdtp.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c index fa72ef66a..9ddcd6464 100644 --- a/profiles/audio/avdtp.c +++ b/profiles/audio/avdtp.c @@ -1226,7 +1226,13 @@ void avdtp_unref(struct avdtp *session) switch (session->state) { case AVDTP_SESSION_STATE_CONNECTED: - set_disconnect_timer(session); + /* Only set disconnect timer if there are local endpoints + * otherwise disconnect immediately. + */ + if (queue_isempty(session->lseps)) + connection_lost(session, ECONNRESET); + else + set_disconnect_timer(session); break; case AVDTP_SESSION_STATE_CONNECTING: connection_lost(session, ECONNABORTED); -- 2.29.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH BlueZ 2/3] btio: Use G_PRIORITY_HIGH for watches 2021-02-16 23:33 [PATCH BlueZ 1/3] avdtp: Fix setting disconnect timer when there is no local endpoints Luiz Augusto von Dentz @ 2021-02-16 23:33 ` Luiz Augusto von Dentz 2021-02-16 23:33 ` [PATCH BlueZ 3/3] avdtp: Remove use of G_PRIORITY_LOW Luiz Augusto von Dentz 2021-02-16 23:57 ` [BlueZ,1/3] avdtp: Fix setting disconnect timer when there is no local endpoints bluez.test.bot 2 siblings, 0 replies; 11+ messages in thread From: Luiz Augusto von Dentz @ 2021-02-16 23:33 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> This makes btio watches default to G_PRIORITY_HIGH instead of G_PRIORITY_DEFAULT so it takes priority over regular traffic or timeout handling. --- btio/btio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/btio/btio.c b/btio/btio.c index 8230212b4..1f1c374bc 100644 --- a/btio/btio.c +++ b/btio/btio.c @@ -274,7 +274,7 @@ static void server_add(GIOChannel *io, BtIOConnect connect, server->destroy = destroy; cond = G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL; - g_io_add_watch_full(io, G_PRIORITY_DEFAULT, cond, server_cb, server, + g_io_add_watch_full(io, G_PRIORITY_HIGH, cond, server_cb, server, (GDestroyNotify) server_remove); } @@ -291,7 +291,7 @@ static void connect_add(GIOChannel *io, BtIOConnect connect, bdaddr_t dst, conn->dst = dst; cond = G_IO_OUT | G_IO_ERR | G_IO_HUP | G_IO_NVAL; - g_io_add_watch_full(io, G_PRIORITY_DEFAULT, cond, connect_cb, conn, + g_io_add_watch_full(io, G_PRIORITY_HIGH, cond, connect_cb, conn, (GDestroyNotify) connect_remove); } @@ -307,7 +307,7 @@ static void accept_add(GIOChannel *io, BtIOConnect connect, gpointer user_data, accept->destroy = destroy; cond = G_IO_OUT | G_IO_ERR | G_IO_HUP | G_IO_NVAL; - g_io_add_watch_full(io, G_PRIORITY_DEFAULT, cond, accept_cb, accept, + g_io_add_watch_full(io, G_PRIORITY_HIGH, cond, accept_cb, accept, (GDestroyNotify) accept_remove); } -- 2.29.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH BlueZ 3/3] avdtp: Remove use of G_PRIORITY_LOW 2021-02-16 23:33 [PATCH BlueZ 1/3] avdtp: Fix setting disconnect timer when there is no local endpoints Luiz Augusto von Dentz 2021-02-16 23:33 ` [PATCH BlueZ 2/3] btio: Use G_PRIORITY_HIGH for watches Luiz Augusto von Dentz @ 2021-02-16 23:33 ` Luiz Augusto von Dentz 2021-02-17 20:24 ` Miao-chen Chou 2021-02-16 23:57 ` [BlueZ,1/3] avdtp: Fix setting disconnect timer when there is no local endpoints bluez.test.bot 2 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2021-02-16 23:33 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> G_PRIORITY_LOW was used in order to prioritize the AVDTP media transport channel over the signalling channel but this has the side effect of delaying the dispatching of other conditions such as HUP/NVAL, so now that BtIO use G_PRIORITY_HIGH for its watches we no longer need to deprioritize session_cb. --- profiles/audio/avdtp.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c index 9ddcd6464..088ca58b3 100644 --- a/profiles/audio/avdtp.c +++ b/profiles/audio/avdtp.c @@ -2378,19 +2378,10 @@ static void avdtp_connect_cb(GIOChannel *chan, GError *err, gpointer user_data) if (session->io_id) g_source_remove(session->io_id); - /* This watch should be low priority since otherwise the - * connect callback might be dispatched before the session - * callback if the kernel wakes us up at the same time for - * them. This could happen if a headset is very quick in - * sending the Start command after connecting the stream - * transport channel. - */ - session->io_id = g_io_add_watch_full(chan, - G_PRIORITY_LOW, + session->io_id = g_io_add_watch(chan, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL, - (GIOFunc) session_cb, session, - NULL); + (GIOFunc) session_cb, session); if (session->stream_setup) set_disconnect_timer(session); -- 2.29.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH BlueZ 3/3] avdtp: Remove use of G_PRIORITY_LOW 2021-02-16 23:33 ` [PATCH BlueZ 3/3] avdtp: Remove use of G_PRIORITY_LOW Luiz Augusto von Dentz @ 2021-02-17 20:24 ` Miao-chen Chou 2021-02-17 21:21 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 11+ messages in thread From: Miao-chen Chou @ 2021-02-17 20:24 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: Bluetooth Kernel Mailing List Hi Luiz, Thanks for reforming the fix. I tested the series on my DUT and it works as expected where the paired device persists throughout the suspend and resume. However I observed the following sequence in the logs. Given that this series raises the priority of avdtp's IO, session_cb() got poked immediately after the index was removed, and that triggers avdtp_free() early. About ~100ms later, avdtp_unregister_sep was called with a non-NULL pointer to the sep with A2DP. However, that sep is no longer valid since avdtp_free() freed all the seps. I wonder if this can be a potential issue? = Close Index: 5C:3A:45:9C:CF:8A [hci0] 2021-02-17 11:52:02.607860 @ MGMT Event: Index Removed (0x0005) plen 0 {0x0003} [hci0] 2021-02-17 11:52:02.607866 ... = Delete Index: 5C:3A:45:9C:CF:8A [hci0] 2021-02-17 11:52:02.607876 = bluetoothd: Unable to get io data for Hands-Free Voice gateway: getpeername: Transport endpoint is... 2021-02-17 11:52:02.618425 = bluetoothd: src/service.c:change_state() 0x56093254dff0: device 28:11:A5:E1:4F:67 profile Hands-Fre.. 2021-02-17 11:52:02.618480 = bluetoothd: src/service.c:btd_service_unref() 0x56093254dff0: ref=2 2021-02-17 11:52:02.618486 = bluetoothd: profiles/audio/avdtp.c:session_cb() 2021-02-17 11:52:02.618497 = bluetoothd: profiles/audio/avdtp.c:avdtp_ref() 0x560932556830: ref=2 2021-02-17 11:52:02.618585 = bluetoothd: profiles/audio/avdtp.c:connection_lost() Disconnected from 28:11:A5:E1:4F:67 2021-02-17 11:52:02.618594 = bluetoothd: profiles/audio/a2dp.c:abort_cfm() Source 0x5609324c36b0: Abort_Cfm 2021-02-17 11:52:02.618600 = bluetoothd: profiles/audio/avdtp.c:avdtp_sep_set_state() stream state changed: OPEN -> IDLE 2021-02-17 11:52:02.618605 = bluetoothd: src/service.c:change_state() 0x560932552490: device 28:11:A5:E1:4F:67 profile a2dp-sink.. 2021-02-17 11:52:02.621466 = bluetoothd: profiles/audio/avdtp.c:avdtp_unref() 0x560932556830: ref=1 2021-02-17 11:52:02.621492 = bluetoothd: profiles/audio/sink.c:sink_set_state() State changed /org/bluez/hci0/dev_28_11_A5_E1_4F.. 2021-02-17 11:52:02.621500 = bluetoothd: profiles/audio/a2dp.c:channel_remove() chan 0x5609324a7930 2021-02-17 11:52:02.621539 = bluetoothd: profiles/audio/avdtp.c:avdtp_unref() 0x560932556830: ref=0 2021-02-17 11:52:02.621546 = New Index: 00:00:00:00:00:00 (Primary,USB,hci0) [hci0] 2021-02-17 11:52:02.619195 = bluetoothd: profiles/audio/avdtp.c:avdtp_free() 0x560932556830 2021-02-17 11:52:02.621615 = bluetoothd: Endpoint unregistered: sender=:1.50 path=/org/chromium/Cras/Bluetooth/A2DPSource 2021-02-17 11:52:02.764766 = bluetoothd: profiles/audio/media.c:media_endpoint_destroy() sender=:1.50 path=/org/chromium/Cras/Bl.. 2021-02-17 11:52:02.764792 = bluetoothd: profiles/audio/avdtp.c:avdtp_unregister_sep() SEP 0x5609324c36b0 unregistered: type:0 c.. 2021-02-17 11:52:02.764797 = bluetoothd: profiles/audio/media.c:media_player_destroy() sender=:1.50 path=/org/chromium/Cras/Blue.. 2021-02-17 11:52:02.771347 On Tue, Feb 16, 2021 at 3:35 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > G_PRIORITY_LOW was used in order to prioritize the AVDTP media transport > channel over the signalling channel but this has the side effect of > delaying the dispatching of other conditions such as HUP/NVAL, so now > that BtIO use G_PRIORITY_HIGH for its watches we no longer need to > deprioritize session_cb. > --- > profiles/audio/avdtp.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c > index 9ddcd6464..088ca58b3 100644 > --- a/profiles/audio/avdtp.c > +++ b/profiles/audio/avdtp.c > @@ -2378,19 +2378,10 @@ static void avdtp_connect_cb(GIOChannel *chan, GError *err, gpointer user_data) > if (session->io_id) > g_source_remove(session->io_id); > > - /* This watch should be low priority since otherwise the > - * connect callback might be dispatched before the session > - * callback if the kernel wakes us up at the same time for > - * them. This could happen if a headset is very quick in > - * sending the Start command after connecting the stream > - * transport channel. > - */ > - session->io_id = g_io_add_watch_full(chan, > - G_PRIORITY_LOW, > + session->io_id = g_io_add_watch(chan, > G_IO_IN | G_IO_ERR | G_IO_HUP > | G_IO_NVAL, > - (GIOFunc) session_cb, session, > - NULL); > + (GIOFunc) session_cb, session); > > if (session->stream_setup) > set_disconnect_timer(session); > -- > 2.29.2 > Regards, Miao ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH BlueZ 3/3] avdtp: Remove use of G_PRIORITY_LOW 2021-02-17 20:24 ` Miao-chen Chou @ 2021-02-17 21:21 ` Luiz Augusto von Dentz 2021-02-17 21:41 ` Miao-chen Chou 0 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2021-02-17 21:21 UTC (permalink / raw) To: Miao-chen Chou; +Cc: Bluetooth Kernel Mailing List Hi Miao, On Wed, Feb 17, 2021 at 12:24 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > Hi Luiz, > > Thanks for reforming the fix. I tested the series on my DUT and it > works as expected where the paired device persists throughout the > suspend and resume. However I observed the following sequence in the > logs. > Given that this series raises the priority of avdtp's IO, session_cb() > got poked immediately after the index was removed, and that triggers > avdtp_free() early. About ~100ms later, avdtp_unregister_sep was > called with a non-NULL pointer to the sep with A2DP. However, that sep > is no longer valid since avdtp_free() freed all the seps. I wonder if > this can be a potential issue? Are you referring to the local sep or the remote ones? avdtp_free should not have free any local ones as it just have a reference to the queue and that afaik was never meant to be free by avdtp_free. > = Close Index: 5C:3A:45:9C:CF:8A > [hci0] 2021-02-17 11:52:02.607860 > @ MGMT Event: Index Removed (0x0005) plen 0 > {0x0003} [hci0] 2021-02-17 11:52:02.607866 > ... > = Delete Index: 5C:3A:45:9C:CF:8A > [hci0] 2021-02-17 11:52:02.607876 > = bluetoothd: Unable to get io data for Hands-Free Voice gateway: > getpeername: Transport endpoint is... 2021-02-17 11:52:02.618425 > = bluetoothd: src/service.c:change_state() 0x56093254dff0: device > 28:11:A5:E1:4F:67 profile Hands-Fre.. 2021-02-17 11:52:02.618480 > = bluetoothd: src/service.c:btd_service_unref() 0x56093254dff0: ref=2 > 2021-02-17 11:52:02.618486 > = bluetoothd: profiles/audio/avdtp.c:session_cb() > 2021-02-17 11:52:02.618497 > = bluetoothd: profiles/audio/avdtp.c:avdtp_ref() 0x560932556830: ref=2 > 2021-02-17 11:52:02.618585 > = bluetoothd: profiles/audio/avdtp.c:connection_lost() Disconnected > from 28:11:A5:E1:4F:67 2021-02-17 11:52:02.618594 > = bluetoothd: profiles/audio/a2dp.c:abort_cfm() Source 0x5609324c36b0: > Abort_Cfm 2021-02-17 11:52:02.618600 > = bluetoothd: profiles/audio/avdtp.c:avdtp_sep_set_state() stream > state changed: OPEN -> IDLE 2021-02-17 11:52:02.618605 > = bluetoothd: src/service.c:change_state() 0x560932552490: device > 28:11:A5:E1:4F:67 profile a2dp-sink.. 2021-02-17 11:52:02.621466 > = bluetoothd: profiles/audio/avdtp.c:avdtp_unref() 0x560932556830: > ref=1 2021-02-17 11:52:02.621492 > = bluetoothd: profiles/audio/sink.c:sink_set_state() State changed > /org/bluez/hci0/dev_28_11_A5_E1_4F.. 2021-02-17 11:52:02.621500 > = bluetoothd: profiles/audio/a2dp.c:channel_remove() chan > 0x5609324a7930 2021-02-17 > 11:52:02.621539 > = bluetoothd: profiles/audio/avdtp.c:avdtp_unref() 0x560932556830: > ref=0 2021-02-17 11:52:02.621546 > = New Index: 00:00:00:00:00:00 (Primary,USB,hci0) > [hci0] 2021-02-17 11:52:02.619195 > = bluetoothd: profiles/audio/avdtp.c:avdtp_free() 0x560932556830 > 2021-02-17 11:52:02.621615 > = bluetoothd: Endpoint unregistered: sender=:1.50 > path=/org/chromium/Cras/Bluetooth/A2DPSource 2021-02-17 > 11:52:02.764766 > = bluetoothd: profiles/audio/media.c:media_endpoint_destroy() > sender=:1.50 path=/org/chromium/Cras/Bl.. 2021-02-17 11:52:02.764792 > = bluetoothd: profiles/audio/avdtp.c:avdtp_unregister_sep() SEP > 0x5609324c36b0 unregistered: type:0 c.. 2021-02-17 11:52:02.764797 > = bluetoothd: profiles/audio/media.c:media_player_destroy() > sender=:1.50 path=/org/chromium/Cras/Blue.. 2021-02-17 > 11:52:02.771347 > > On Tue, Feb 16, 2021 at 3:35 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > G_PRIORITY_LOW was used in order to prioritize the AVDTP media transport > > channel over the signalling channel but this has the side effect of > > delaying the dispatching of other conditions such as HUP/NVAL, so now > > that BtIO use G_PRIORITY_HIGH for its watches we no longer need to > > deprioritize session_cb. > > --- > > profiles/audio/avdtp.c | 13 ++----------- > > 1 file changed, 2 insertions(+), 11 deletions(-) > > > > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c > > index 9ddcd6464..088ca58b3 100644 > > --- a/profiles/audio/avdtp.c > > +++ b/profiles/audio/avdtp.c > > @@ -2378,19 +2378,10 @@ static void avdtp_connect_cb(GIOChannel *chan, GError *err, gpointer user_data) > > if (session->io_id) > > g_source_remove(session->io_id); > > > > - /* This watch should be low priority since otherwise the > > - * connect callback might be dispatched before the session > > - * callback if the kernel wakes us up at the same time for > > - * them. This could happen if a headset is very quick in > > - * sending the Start command after connecting the stream > > - * transport channel. > > - */ > > - session->io_id = g_io_add_watch_full(chan, > > - G_PRIORITY_LOW, > > + session->io_id = g_io_add_watch(chan, > > G_IO_IN | G_IO_ERR | G_IO_HUP > > | G_IO_NVAL, > > - (GIOFunc) session_cb, session, > > - NULL); > > + (GIOFunc) session_cb, session); > > > > if (session->stream_setup) > > set_disconnect_timer(session); > > -- > > 2.29.2 > > > Regards, > Miao -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH BlueZ 3/3] avdtp: Remove use of G_PRIORITY_LOW 2021-02-17 21:21 ` Luiz Augusto von Dentz @ 2021-02-17 21:41 ` Miao-chen Chou 0 siblings, 0 replies; 11+ messages in thread From: Miao-chen Chou @ 2021-02-17 21:41 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: Bluetooth Kernel Mailing List Hi Luiz, On Wed, Feb 17, 2021 at 1:21 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Miao, > > On Wed, Feb 17, 2021 at 12:24 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > > > Hi Luiz, > > > > Thanks for reforming the fix. I tested the series on my DUT and it > > works as expected where the paired device persists throughout the > > suspend and resume. However I observed the following sequence in the > > logs. > > Given that this series raises the priority of avdtp's IO, session_cb() > > got poked immediately after the index was removed, and that triggers > > avdtp_free() early. About ~100ms later, avdtp_unregister_sep was > > called with a non-NULL pointer to the sep with A2DP. However, that sep > > is no longer valid since avdtp_free() freed all the seps. I wonder if > > this can be a potential issue? > > Are you referring to the local sep or the remote ones? avdtp_free > should not have free any local ones as it just has a reference to the > queue and that afaik was never meant to be free by avdtp_free. That actually answers my question. There might not be concern then, since avdtp_free() frees the remote one but not the local one while avdtp_unregister_sep() frees the local one. All is good. Thanks for the fix. > > > = Close Index: 5C:3A:45:9C:CF:8A > > [hci0] 2021-02-17 11:52:02.607860 > > @ MGMT Event: Index Removed (0x0005) plen 0 > > {0x0003} [hci0] 2021-02-17 11:52:02.607866 > > ... > > = Delete Index: 5C:3A:45:9C:CF:8A > > [hci0] 2021-02-17 11:52:02.607876 > > = bluetoothd: Unable to get io data for Hands-Free Voice gateway: > > getpeername: Transport endpoint is... 2021-02-17 11:52:02.618425 > > = bluetoothd: src/service.c:change_state() 0x56093254dff0: device > > 28:11:A5:E1:4F:67 profile Hands-Fre.. 2021-02-17 11:52:02.618480 > > = bluetoothd: src/service.c:btd_service_unref() 0x56093254dff0: ref=2 > > 2021-02-17 11:52:02.618486 > > = bluetoothd: profiles/audio/avdtp.c:session_cb() > > 2021-02-17 11:52:02.618497 > > = bluetoothd: profiles/audio/avdtp.c:avdtp_ref() 0x560932556830: ref=2 > > 2021-02-17 11:52:02.618585 > > = bluetoothd: profiles/audio/avdtp.c:connection_lost() Disconnected > > from 28:11:A5:E1:4F:67 2021-02-17 11:52:02.618594 > > = bluetoothd: profiles/audio/a2dp.c:abort_cfm() Source 0x5609324c36b0: > > Abort_Cfm 2021-02-17 11:52:02.618600 > > = bluetoothd: profiles/audio/avdtp.c:avdtp_sep_set_state() stream > > state changed: OPEN -> IDLE 2021-02-17 11:52:02.618605 > > = bluetoothd: src/service.c:change_state() 0x560932552490: device > > 28:11:A5:E1:4F:67 profile a2dp-sink.. 2021-02-17 11:52:02.621466 > > = bluetoothd: profiles/audio/avdtp.c:avdtp_unref() 0x560932556830: > > ref=1 2021-02-17 11:52:02.621492 > > = bluetoothd: profiles/audio/sink.c:sink_set_state() State changed > > /org/bluez/hci0/dev_28_11_A5_E1_4F.. 2021-02-17 11:52:02.621500 > > = bluetoothd: profiles/audio/a2dp.c:channel_remove() chan > > 0x5609324a7930 2021-02-17 > > 11:52:02.621539 > > = bluetoothd: profiles/audio/avdtp.c:avdtp_unref() 0x560932556830: > > ref=0 2021-02-17 11:52:02.621546 > > = New Index: 00:00:00:00:00:00 (Primary,USB,hci0) > > [hci0] 2021-02-17 11:52:02.619195 > > = bluetoothd: profiles/audio/avdtp.c:avdtp_free() 0x560932556830 > > 2021-02-17 11:52:02.621615 > > = bluetoothd: Endpoint unregistered: sender=:1.50 > > path=/org/chromium/Cras/Bluetooth/A2DPSource 2021-02-17 > > 11:52:02.764766 > > = bluetoothd: profiles/audio/media.c:media_endpoint_destroy() > > sender=:1.50 path=/org/chromium/Cras/Bl.. 2021-02-17 11:52:02.764792 > > = bluetoothd: profiles/audio/avdtp.c:avdtp_unregister_sep() SEP > > 0x5609324c36b0 unregistered: type:0 c.. 2021-02-17 11:52:02.764797 > > = bluetoothd: profiles/audio/media.c:media_player_destroy() > > sender=:1.50 path=/org/chromium/Cras/Blue.. 2021-02-17 > > 11:52:02.771347 > > > > On Tue, Feb 16, 2021 at 3:35 PM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > > > G_PRIORITY_LOW was used in order to prioritize the AVDTP media transport > > > channel over the signalling channel but this has the side effect of > > > delaying the dispatching of other conditions such as HUP/NVAL, so now > > > that BtIO use G_PRIORITY_HIGH for its watches we no longer need to > > > deprioritize session_cb. > > > --- > > > profiles/audio/avdtp.c | 13 ++----------- > > > 1 file changed, 2 insertions(+), 11 deletions(-) > > > > > > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c > > > index 9ddcd6464..088ca58b3 100644 > > > --- a/profiles/audio/avdtp.c > > > +++ b/profiles/audio/avdtp.c > > > @@ -2378,19 +2378,10 @@ static void avdtp_connect_cb(GIOChannel *chan, GError *err, gpointer user_data) > > > if (session->io_id) > > > g_source_remove(session->io_id); > > > > > > - /* This watch should be low priority since otherwise the > > > - * connect callback might be dispatched before the session > > > - * callback if the kernel wakes us up at the same time for > > > - * them. This could happen if a headset is very quick in > > > - * sending the Start command after connecting the stream > > > - * transport channel. > > > - */ > > > - session->io_id = g_io_add_watch_full(chan, > > > - G_PRIORITY_LOW, > > > + session->io_id = g_io_add_watch(chan, > > > G_IO_IN | G_IO_ERR | G_IO_HUP > > > | G_IO_NVAL, > > > - (GIOFunc) session_cb, session, > > > - NULL); > > > + (GIOFunc) session_cb, session); > > > > > > if (session->stream_setup) > > > set_disconnect_timer(session); > > > -- > > > 2.29.2 > > > > > Regards, > > Miao > > > > -- > Luiz Augusto von Dentz Regards, Miao ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [BlueZ,1/3] avdtp: Fix setting disconnect timer when there is no local endpoints 2021-02-16 23:33 [PATCH BlueZ 1/3] avdtp: Fix setting disconnect timer when there is no local endpoints Luiz Augusto von Dentz 2021-02-16 23:33 ` [PATCH BlueZ 2/3] btio: Use G_PRIORITY_HIGH for watches Luiz Augusto von Dentz 2021-02-16 23:33 ` [PATCH BlueZ 3/3] avdtp: Remove use of G_PRIORITY_LOW Luiz Augusto von Dentz @ 2021-02-16 23:57 ` bluez.test.bot 2021-02-17 19:45 ` Luiz Augusto von Dentz 2 siblings, 1 reply; 11+ messages in thread From: bluez.test.bot @ 2021-02-16 23:57 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz [-- Attachment #1: Type: text/plain, Size: 557 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=434305 ---Test result--- ############################## Test: CheckPatch - PASS ############################## Test: CheckGitLint - PASS ############################## Test: CheckBuild - PASS ############################## Test: MakeCheck - PASS --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BlueZ,1/3] avdtp: Fix setting disconnect timer when there is no local endpoints 2021-02-16 23:57 ` [BlueZ,1/3] avdtp: Fix setting disconnect timer when there is no local endpoints bluez.test.bot @ 2021-02-17 19:45 ` Luiz Augusto von Dentz 2021-02-17 20:37 ` Miao-chen Chou 0 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2021-02-17 19:45 UTC (permalink / raw) To: linux-bluetooth; +Cc: Miao-chen Chou Hi Miao, On Tue, Feb 16, 2021 at 3:57 PM <bluez.test.bot@gmail.com> wrote: > > 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=434305 > > ---Test result--- > > ############################## > Test: CheckPatch - PASS > > ############################## > Test: CheckGitLint - PASS > > ############################## > Test: CheckBuild - PASS > > ############################## > Test: MakeCheck - PASS > > > > --- > Regards, > Linux Bluetooth Can you give this set a try with the use case you had? I tested with unplugged use case and it now seems to trigger session_cb immediately so it should work for your case as well. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BlueZ,1/3] avdtp: Fix setting disconnect timer when there is no local endpoints 2021-02-17 19:45 ` Luiz Augusto von Dentz @ 2021-02-17 20:37 ` Miao-chen Chou 2021-02-17 21:24 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 11+ messages in thread From: Miao-chen Chou @ 2021-02-17 20:37 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth Hi Luiz, I was testing before seeing your email. Please take a look at my comment on the last commit of the series. Although this commit doesn't affect the symptom we observed (it was IDLE state which triggers the disconnection of IO), the change makes sense. On Wed, Feb 17, 2021 at 11:45 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Miao, > > On Tue, Feb 16, 2021 at 3:57 PM <bluez.test.bot@gmail.com> wrote: > > > > 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=434305 > > > > ---Test result--- > > > > ############################## > > Test: CheckPatch - PASS > > > > ############################## > > Test: CheckGitLint - PASS > > > > ############################## > > Test: CheckBuild - PASS > > > > ############################## > > Test: MakeCheck - PASS > > > > > > > > --- > > Regards, > > Linux Bluetooth > > Can you give this set a try with the use case you had? I tested with > unplugged use case and it now seems to trigger session_cb immediately > so it should work for your case as well. > > -- > Luiz Augusto von Dentz Thanks, Miao ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BlueZ,1/3] avdtp: Fix setting disconnect timer when there is no local endpoints 2021-02-17 20:37 ` Miao-chen Chou @ 2021-02-17 21:24 ` Luiz Augusto von Dentz 2021-02-18 1:28 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2021-02-17 21:24 UTC (permalink / raw) To: Miao-chen Chou; +Cc: linux-bluetooth Hi Miao, On Wed, Feb 17, 2021 at 12:37 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > Hi Luiz, > > I was testing before seeing your email. Please take a look at my > comment on the last commit of the series. > Although this commit doesn't affect the symptom we observed (it was > IDLE state which triggers the disconnection of IO), the change makes > sense. Yep, this doesn't actually make any difference on the matter of freeing avdtp session when the adapter is removed but as you said it makes sense on it own given that local endpoints can be unregistered. > On Wed, Feb 17, 2021 at 11:45 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Miao, > > > > On Tue, Feb 16, 2021 at 3:57 PM <bluez.test.bot@gmail.com> wrote: > > > > > > 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=434305 > > > > > > ---Test result--- > > > > > > ############################## > > > Test: CheckPatch - PASS > > > > > > ############################## > > > Test: CheckGitLint - PASS > > > > > > ############################## > > > Test: CheckBuild - PASS > > > > > > ############################## > > > Test: MakeCheck - PASS > > > > > > > > > > > > --- > > > Regards, > > > Linux Bluetooth > > > > Can you give this set a try with the use case you had? I tested with > > unplugged use case and it now seems to trigger session_cb immediately > > so it should work for your case as well. > > > > -- > > Luiz Augusto von Dentz > Thanks, > Miao -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BlueZ,1/3] avdtp: Fix setting disconnect timer when there is no local endpoints 2021-02-17 21:24 ` Luiz Augusto von Dentz @ 2021-02-18 1:28 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 11+ messages in thread From: Luiz Augusto von Dentz @ 2021-02-18 1:28 UTC (permalink / raw) To: Miao-chen Chou; +Cc: linux-bluetooth Hi, On Wed, Feb 17, 2021 at 1:24 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Miao, > > On Wed, Feb 17, 2021 at 12:37 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > > > Hi Luiz, > > > > I was testing before seeing your email. Please take a look at my > > comment on the last commit of the series. > > Although this commit doesn't affect the symptom we observed (it was > > IDLE state which triggers the disconnection of IO), the change makes > > sense. > > Yep, this doesn't actually make any difference on the matter of > freeing avdtp session when the adapter is removed but as you said it > makes sense on it own given that local endpoints can be unregistered. > > > On Wed, Feb 17, 2021 at 11:45 AM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Miao, > > > > > > On Tue, Feb 16, 2021 at 3:57 PM <bluez.test.bot@gmail.com> wrote: > > > > > > > > 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=434305 > > > > > > > > ---Test result--- > > > > > > > > ############################## > > > > Test: CheckPatch - PASS > > > > > > > > ############################## > > > > Test: CheckGitLint - PASS > > > > > > > > ############################## > > > > Test: CheckBuild - PASS > > > > > > > > ############################## > > > > Test: MakeCheck - PASS > > > > > > > > > > > > > > > > --- > > > > Regards, > > > > Linux Bluetooth > > > > > > Can you give this set a try with the use case you had? I tested with > > > unplugged use case and it now seems to trigger session_cb immediately > > > so it should work for your case as well. > > > > > > -- > > > Luiz Augusto von Dentz > > Thanks, > > Miao > > > > -- > Luiz Augusto von Dentz Pushed. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-02-18 1:29 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-16 23:33 [PATCH BlueZ 1/3] avdtp: Fix setting disconnect timer when there is no local endpoints Luiz Augusto von Dentz 2021-02-16 23:33 ` [PATCH BlueZ 2/3] btio: Use G_PRIORITY_HIGH for watches Luiz Augusto von Dentz 2021-02-16 23:33 ` [PATCH BlueZ 3/3] avdtp: Remove use of G_PRIORITY_LOW Luiz Augusto von Dentz 2021-02-17 20:24 ` Miao-chen Chou 2021-02-17 21:21 ` Luiz Augusto von Dentz 2021-02-17 21:41 ` Miao-chen Chou 2021-02-16 23:57 ` [BlueZ,1/3] avdtp: Fix setting disconnect timer when there is no local endpoints bluez.test.bot 2021-02-17 19:45 ` Luiz Augusto von Dentz 2021-02-17 20:37 ` Miao-chen Chou 2021-02-17 21:24 ` Luiz Augusto von Dentz 2021-02-18 1:28 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).