* [PATCH v4] device: Remove device after all bearers are disconnected
@ 2024-09-27 2:38 Cheng Jiang
2024-09-27 4:42 ` [v4] " bluez.test.bot
2024-09-27 14:03 ` [PATCH v4] " Luiz Augusto von Dentz
0 siblings, 2 replies; 4+ messages in thread
From: Cheng Jiang @ 2024-09-27 2:38 UTC (permalink / raw)
To: linux-bluetooth; +Cc: quic_jiaymao
For a combo mode remote, both BR/EDR and BLE may be connected.
RemoveDevice should be handled after all bearers are dropped,
otherwise, remove device is not performed in adapter_remove_connection.
---
src/device.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/src/device.c b/src/device.c
index f8f61e643..4efd755a0 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3492,8 +3492,27 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type,
DBusMessage *msg = device->disconnects->data;
if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
- "RemoveDevice"))
+ "RemoveDevice")) {
+
+ /* Don't handle the RemoveDevice msg if device is
+ * connected. For a dual-mode remote, both BR/EDR
+ * and BLE may be connected, RemoveDevice should
+ * be handled after all bearers are disconnects.
+ * Otherwise, if msg is removed, but not all
+ * connection are dropped, this function returns
+ * before *remove is updated, then after all
+ * connections are dropped, but device->disconnects
+ * is NULL, remove_device is not updated. Consequently
+ * *remove is not set to true. The device is not removed
+ * for adapter in adapter_remove_connection. Need
+ * to wait for temporary device timeout to remove
+ * the device.
+ */
+ if (device->bredr_state.connected ||
+ device->le_state.connected)
+ break;
remove_device = true;
+ }
g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
device->disconnects = g_slist_remove(device->disconnects, msg);
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [v4] device: Remove device after all bearers are disconnected
2024-09-27 2:38 [PATCH v4] device: Remove device after all bearers are disconnected Cheng Jiang
@ 2024-09-27 4:42 ` bluez.test.bot
2024-09-27 14:03 ` [PATCH v4] " Luiz Augusto von Dentz
1 sibling, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2024-09-27 4:42 UTC (permalink / raw)
To: linux-bluetooth, quic_chejiang
[-- Attachment #1: Type: text/plain, Size: 948 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=893302
---Test result---
Test Summary:
CheckPatch PASS 0.49 seconds
GitLint PASS 0.46 seconds
BuildEll PASS 24.05 seconds
BluezMake PASS 1507.72 seconds
MakeCheck PASS 13.23 seconds
MakeDistcheck PASS 174.81 seconds
CheckValgrind PASS 246.55 seconds
CheckSmatch PASS 347.47 seconds
bluezmakeextell PASS 117.03 seconds
IncrementalBuild PASS 1357.42 seconds
ScanBuild PASS 992.35 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4] device: Remove device after all bearers are disconnected
2024-09-27 2:38 [PATCH v4] device: Remove device after all bearers are disconnected Cheng Jiang
2024-09-27 4:42 ` [v4] " bluez.test.bot
@ 2024-09-27 14:03 ` Luiz Augusto von Dentz
2024-09-29 2:25 ` Cheng Jiang
1 sibling, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2024-09-27 14:03 UTC (permalink / raw)
To: Cheng Jiang; +Cc: linux-bluetooth, quic_jiaymao
Hi Cheng,
On Thu, Sep 26, 2024 at 10:39 PM Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>
> For a combo mode remote, both BR/EDR and BLE may be connected.
> RemoveDevice should be handled after all bearers are dropped,
> otherwise, remove device is not performed in adapter_remove_connection.
> ---
> src/device.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/src/device.c b/src/device.c
> index f8f61e643..4efd755a0 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -3492,8 +3492,27 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type,
> DBusMessage *msg = device->disconnects->data;
>
> if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
> - "RemoveDevice"))
> + "RemoveDevice")) {
> +
> + /* Don't handle the RemoveDevice msg if device is
> + * connected. For a dual-mode remote, both BR/EDR
> + * and BLE may be connected, RemoveDevice should
> + * be handled after all bearers are disconnects.
> + * Otherwise, if msg is removed, but not all
> + * connection are dropped, this function returns
> + * before *remove is updated, then after all
> + * connections are dropped, but device->disconnects
> + * is NULL, remove_device is not updated. Consequently
> + * *remove is not set to true. The device is not removed
> + * for adapter in adapter_remove_connection. Need
> + * to wait for temporary device timeout to remove
> + * the device.
> + */
I mean as git description, anyway Im having second thoughts about this
change, see below.
> + if (device->bredr_state.connected ||
> + device->le_state.connected)
> + break;
> remove_device = true;
> + }
>
> g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
> device->disconnects = g_slist_remove(device->disconnects, msg);
How we move the block checking for disconnect message after check all
bearers have been disconnected:
diff --git a/src/device.c b/src/device.c
index f8f61e64376c..76d2c859c747 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3488,18 +3488,6 @@ void device_remove_connection(struct btd_device
*device, uint8_t bdaddr_type,
device->connect = NULL;
}
- while (device->disconnects) {
- DBusMessage *msg = device->disconnects->data;
-
- if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
- "RemoveDevice"))
- remove_device = true;
-
- g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
- device->disconnects = g_slist_remove(device->disconnects, msg);
- dbus_message_unref(msg);
- }
-
/* Check paired status of both bearers since it's possible to be
/* Check paired status of both bearers since it's possible to be
/* Check paired status of both bearers since it's possible to be
* paired but not connected via link key to LTK conversion.
*/
@@ -3539,6 +3527,18 @@ void device_remove_connection(struct btd_device
*device, uint8_t bdaddr_type,
g_dbus_emit_property_changed(dbus_conn, device->path,
DEVICE_INTERFACE, "Connected");
+ while (device->disconnects) {
+ DBusMessage *msg = device->disconnects->data;
+
+ if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
+ "RemoveDevice"))
+ remove_device = true;
+
+ g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
+ device->disconnects = g_slist_remove(device->disconnects, msg);
+ dbus_message_unref(msg);
+ }
+
if (remove_device)
*remove = remove_device;
> --
> 2.25.1
>
>
--
Luiz Augusto von Dentz
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v4] device: Remove device after all bearers are disconnected
2024-09-27 14:03 ` [PATCH v4] " Luiz Augusto von Dentz
@ 2024-09-29 2:25 ` Cheng Jiang
0 siblings, 0 replies; 4+ messages in thread
From: Cheng Jiang @ 2024-09-29 2:25 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, quic_jiaymao
Hi Luiz,
On 9/27/2024 10:03 PM, Luiz Augusto von Dentz wrote:
> Hi Cheng,
>
> On Thu, Sep 26, 2024 at 10:39 PM Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>
>> For a combo mode remote, both BR/EDR and BLE may be connected.
>> RemoveDevice should be handled after all bearers are dropped,
>> otherwise, remove device is not performed in adapter_remove_connection.
>> ---
>> src/device.c | 21 ++++++++++++++++++++-
>> 1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/device.c b/src/device.c
>> index f8f61e643..4efd755a0 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -3492,8 +3492,27 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type,
>> DBusMessage *msg = device->disconnects->data;
>>
>> if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
>> - "RemoveDevice"))
>> + "RemoveDevice")) {
>> +
>> + /* Don't handle the RemoveDevice msg if device is
>> + * connected. For a dual-mode remote, both BR/EDR
>> + * and BLE may be connected, RemoveDevice should
>> + * be handled after all bearers are disconnects.
>> + * Otherwise, if msg is removed, but not all
>> + * connection are dropped, this function returns
>> + * before *remove is updated, then after all
>> + * connections are dropped, but device->disconnects
>> + * is NULL, remove_device is not updated. Consequently
>> + * *remove is not set to true. The device is not removed
>> + * for adapter in adapter_remove_connection. Need
>> + * to wait for temporary device timeout to remove
>> + * the device.
>> + */
>
> I mean as git description, anyway Im having second thoughts about this
> change, see below.
>
>> + if (device->bredr_state.connected ||
>> + device->le_state.connected)
>> + break;
>> remove_device = true;
>> + }
>>
>> g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
>> device->disconnects = g_slist_remove(device->disconnects, msg);
>
> How we move the block checking for disconnect message after check all
> bearers have been disconnected:
>
> diff --git a/src/device.c b/src/device.c
> index f8f61e64376c..76d2c859c747 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -3488,18 +3488,6 @@ void device_remove_connection(struct btd_device
> *device, uint8_t bdaddr_type,
> device->connect = NULL;
> }
>
> - while (device->disconnects) {
> - DBusMessage *msg = device->disconnects->data;
> -
> - if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
> - "RemoveDevice"))
> - remove_device = true;
> -
> - g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
> - device->disconnects = g_slist_remove(device->disconnects, msg);
> - dbus_message_unref(msg);
> - }
> -
> /* Check paired status of both bearers since it's possible to be
> /* Check paired status of both bearers since it's possible to be
> /* Check paired status of both bearers since it's possible to be
> * paired but not connected via link key to LTK conversion.
> */
> @@ -3539,6 +3527,18 @@ void device_remove_connection(struct btd_device
> *device, uint8_t bdaddr_type,
> g_dbus_emit_property_changed(dbus_conn, device->path,
> DEVICE_INTERFACE, "Connected");
>
> + while (device->disconnects) {
> + DBusMessage *msg = device->disconnects->data;
> +
> + if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
> + "RemoveDevice"))
> + remove_device = true;
> +
> + g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
> + device->disconnects = g_slist_remove(device->disconnects, msg);
> + dbus_message_unref(msg);
> + }
> +
> if (remove_device)
> *remove = remove_device;
>
>
Agree with you. Update the patch (v5). Thank you!
>> --
>> 2.25.1
>>
>>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-09-29 2:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-27 2:38 [PATCH v4] device: Remove device after all bearers are disconnected Cheng Jiang
2024-09-27 4:42 ` [v4] " bluez.test.bot
2024-09-27 14:03 ` [PATCH v4] " Luiz Augusto von Dentz
2024-09-29 2:25 ` Cheng Jiang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox