| Message ID | 20240927023854.2447283-1-quic_chejiang@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] device: Remove device after all bearers are disconnected | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| 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 |
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
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 > >
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 >> >> > >
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);