[PATCH v3] device: Remove device after all bearers are disconnected

[PATCH v3] device: Remove device after all bearers are disconnected

[Cheng Jiang]

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;
 			remove_device = true;
+		}
 
 		g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
 		device->disconnects = g_slist_remove(device->disconnects, msg);
-- 
2.25.1

RE: [v3] device: Remove device after all bearers are disconnected

[bluez.test.bot]

[-- 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=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

Re: [PATCH v3] device: Remove device after all bearers are disconnected

[Luiz Augusto von Dentz]

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
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v3] device: Remove device after all bearers are disconnected

[Cheng Jiang]

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
>>
>>
> 
> 

Re: [PATCH v3] device: Remove device after all bearers are disconnected

[Luiz Augusto von Dentz]

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
> >>
> >>
> >
> >
>


-- 
Luiz Augusto von Dentz