| Message ID | 20240925090948.1540589-1-quic_chejiang@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] 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=892722 ---Test result--- Test Summary: CheckPatch PASS 0.45 seconds GitLint PASS 0.31 seconds BuildEll PASS 23.28 seconds BluezMake PASS 1492.01 seconds MakeCheck PASS 13.04 seconds MakeDistcheck PASS 168.67 seconds CheckValgrind PASS 239.28 seconds CheckSmatch PASS 337.27 seconds bluezmakeextell PASS 112.97 seconds IncrementalBuild PASS 1361.05 seconds ScanBuild PASS 960.64 seconds --- Regards, Linux Bluetooth
Hi Cheng, On Wed, Sep 25, 2024 at 5:10 AM 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 | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/src/device.c b/src/device.c > index f8f61e643..c25bf6b60 100644 > --- a/src/device.c > +++ b/src/device.c > @@ -3492,8 +3492,18 @@ 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. > + */ > + if (device->bredr_state.connected || > + device->le_state.connected) > + break; While this seems to make sense further down there is the same state: line 3531: if (device->bredr_state.connected || device->le_state.connected) So what this is doing is just to avoid removing the msg from device->disconnects but perhaps I'm missing something. > remove_device = true; > + } > > g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID); > device->disconnects = g_slist_remove(device->disconnects, msg); > -- > 2.25.1 > >
Hi Luiz, On 9/25/2024 10:56 PM, Luiz Augusto von Dentz wrote: > Hi Cheng, > > On Wed, Sep 25, 2024 at 5:10 AM 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 | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/src/device.c b/src/device.c >> index f8f61e643..c25bf6b60 100644 >> --- a/src/device.c >> +++ b/src/device.c >> @@ -3492,8 +3492,18 @@ 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. >> + */ >> + if (device->bredr_state.connected || >> + device->le_state.connected) >> + break; > > While this seems to make sense further down there is the same state: > > line 3531: if (device->bredr_state.connected || device->le_state.connected) > > So what this is doing is just to avoid removing the msg from > device->disconnects but perhaps I'm missing something. Yes, it is used to avoid remove the msg. If BR/EDR and BLE are connected, when user perform remove operation, the BR/EDR connection is disconnect first. Then it set the "remove_device" to True, and remove the msg from device->disconnects, but BLE is still connected, so this function (device_remove_connection) will return without set the *remove to True. Then the BLE is disconnected, but at this time, msg is already removed from device->disconnects, so "remove_device" is not set to True. And accordingly *remove is not true. The device is not removed for adapter in adapter_remove_connection. Need to wait for temporary device timeout to remove the device. > >> remove_device = true; >> + } >> >> g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID); >> device->disconnects = g_slist_remove(device->disconnects, msg); >> -- >> 2.25.1 >> >> > >
Hi Cheng, On Wed, Sep 25, 2024 at 9:31 PM Cheng Jiang <quic_chejiang@quicinc.com> wrote: > > Hi Luiz, > > On 9/25/2024 10:56 PM, Luiz Augusto von Dentz wrote: > > Hi Cheng, > > > > On Wed, Sep 25, 2024 at 5:10 AM 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 | 12 +++++++++++- > >> 1 file changed, 11 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/device.c b/src/device.c > >> index f8f61e643..c25bf6b60 100644 > >> --- a/src/device.c > >> +++ b/src/device.c > >> @@ -3492,8 +3492,18 @@ 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. > >> + */ > >> + if (device->bredr_state.connected || > >> + device->le_state.connected) > >> + break; > > > > While this seems to make sense further down there is the same state: > > > > line 3531: if (device->bredr_state.connected || device->le_state.connected) > > > > So what this is doing is just to avoid removing the msg from > > device->disconnects but perhaps I'm missing something. > Yes, it is used to avoid remove the msg. If BR/EDR and BLE are connected, when user perform > remove operation, the BR/EDR connection is disconnect first. Then it set the "remove_device" > to True, and remove the msg from device->disconnects, but BLE is still connected, so this function > (device_remove_connection) will return without set the *remove to True. > Then the BLE is disconnected, but at this time, msg is already removed from device->disconnects, so > "remove_device" is not set to True. And accordingly *remove is not true. The device is not removed for > adapter in adapter_remove_connection. Need to wait for temporary device timeout to remove the device. Aha, that makes a lot more sense now, could you please update this in the patch description just to make it more clear, I will push it once you do that. > > > > >> remove_device = true; > >> + } > >> > >> g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID); > >> device->disconnects = g_slist_remove(device->disconnects, msg); > >> -- > >> 2.25.1 > >> > >> > > > > >
diff --git a/src/device.c b/src/device.c index f8f61e643..c25bf6b60 100644 --- a/src/device.c +++ b/src/device.c @@ -3492,8 +3492,18 @@ 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. + */ + 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);