[PATCH v1] device: only use the address type selection algorithm when remote device is a dual-mode device when pair device

[PATCH v1] device: only use the address type selection algorithm when remote device is a dual-mode device when pair device

[Cheng Jiang]

---
 src/device.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/device.c b/src/device.c
index 7585184de..71fdbb145 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3077,12 +3077,21 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg,
 	if (device->bonding)
 		return btd_error_in_progress(msg);
 
-	if (device->bredr_state.bonded)
+	/* Only use this selection algorithms when device is combo
+	 * chip. Ohterwise, it will use the wrong bearer to establish
+	 * a connection if the device is already paired. which will
+	 * stall the pairing procedure.
+	 */
+	if (device->bredr && device->le) {
+		if (device->bredr_state.bonded)
+			bdaddr_type = device->bdaddr_type;
+		else if (device->le_state.bonded)
+			bdaddr_type = BDADDR_BREDR;
+		else
+			bdaddr_type = select_conn_bearer(device);
+	} else {
 		bdaddr_type = device->bdaddr_type;
-	else if (device->le_state.bonded)
-		bdaddr_type = BDADDR_BREDR;
-	else
-		bdaddr_type = select_conn_bearer(device);
+	}
 
 	state = get_state(device, bdaddr_type);
 
-- 
2.25.1

RE: [v1] device: only use the address type selection algorithm when remote device is a dual-mode device when pair device

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1673 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=903776

---Test result---

Test Summary:
CheckPatch                    PASS      0.46 seconds
GitLint                       FAIL      0.56 seconds
BuildEll                      PASS      24.45 seconds
BluezMake                     PASS      1660.51 seconds
MakeCheck                     PASS      13.31 seconds
MakeDistcheck                 PASS      178.89 seconds
CheckValgrind                 PASS      253.46 seconds
CheckSmatch                   PASS      358.61 seconds
bluezmakeextell               PASS      121.57 seconds
IncrementalBuild              PASS      1647.52 seconds
ScanBuild                     PASS      1031.62 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v1] device: only use the address type selection algorithm when remote device is a dual-mode device when pair device

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (116>80): "[v1] device: only use the address type selection algorithm when remote device is a dual-mode device when pair device"


---
Regards,
Linux Bluetooth

Re: [PATCH v1] device: only use the address type selection algorithm when remote device is a dual-mode device when pair device

[Luiz Augusto von Dentz]

Hi Cheng,

On Mon, Oct 28, 2024 at 7:37 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>
> ---
>  src/device.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index 7585184de..71fdbb145 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -3077,12 +3077,21 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg,
>         if (device->bonding)
>                 return btd_error_in_progress(msg);
>
> -       if (device->bredr_state.bonded)
> +       /* Only use this selection algorithms when device is combo
> +        * chip. Ohterwise, it will use the wrong bearer to establish
> +        * a connection if the device is already paired. which will
> +        * stall the pairing procedure.
> +        */
> +       if (device->bredr && device->le) {
> +               if (device->bredr_state.bonded)
> +                       bdaddr_type = device->bdaddr_type;
> +               else if (device->le_state.bonded)
> +                       bdaddr_type = BDADDR_BREDR;
> +               else
> +                       bdaddr_type = select_conn_bearer(device);
> +       } else {
>                 bdaddr_type = device->bdaddr_type;
> -       else if (device->le_state.bonded)
> -               bdaddr_type = BDADDR_BREDR;
> -       else
> -               bdaddr_type = select_conn_bearer(device);
> +       }

This seems weird without there being a bug with the state itself, for
instance how would it select the wrong bearer if it is not supported?
Also the lack of proper explanation in the commit message doesn't help
to grasp what is going on here, so please have backtrace or something
attached since we need to understand why it would be selecting the
wrong bearer, or perhaps the bearer is being advertised as supported
when in fact it isn't?

>         state = get_state(device, bdaddr_type);
>
> --
> 2.25.1
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v1] device: only use the address type selection algorithm when remote device is a dual-mode device when pair device

[Cheng Jiang]

Hi Luiz,

I update a new patchset with more info. 

In the current logic of pair_device (https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/device.c#n3080).

If a BLE-only device paired twice, in the second trial, it will select the bdaddr_type to BDADDR_BREDR
since device->le_state.bonded is set. We should only use the following logic for a dual-mode remote. 

	if (device->bredr_state.bonded)
		bdaddr_type = device->bdaddr_type;
	else if (device->le_state.bonded)
		bdaddr_type = BDADDR_BREDR;
	else
		bdaddr_type = select_conn_bearer(device);

	state = get_state(device, bdaddr_type);



On 10/28/2024 10:13 PM, Luiz Augusto von Dentz wrote:
> Hi Cheng,
> 
> On Mon, Oct 28, 2024 at 7:37 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>
>> ---
>>  src/device.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/device.c b/src/device.c
>> index 7585184de..71fdbb145 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -3077,12 +3077,21 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg,
>>         if (device->bonding)
>>                 return btd_error_in_progress(msg);
>>
>> -       if (device->bredr_state.bonded)
>> +       /* Only use this selection algorithms when device is combo
>> +        * chip. Ohterwise, it will use the wrong bearer to establish
>> +        * a connection if the device is already paired. which will
>> +        * stall the pairing procedure.
>> +        */
>> +       if (device->bredr && device->le) {
>> +               if (device->bredr_state.bonded)
>> +                       bdaddr_type = device->bdaddr_type;
>> +               else if (device->le_state.bonded)
>> +                       bdaddr_type = BDADDR_BREDR;
>> +               else
>> +                       bdaddr_type = select_conn_bearer(device);
>> +       } else {
>>                 bdaddr_type = device->bdaddr_type;
>> -       else if (device->le_state.bonded)
>> -               bdaddr_type = BDADDR_BREDR;
>> -       else
>> -               bdaddr_type = select_conn_bearer(device);
>> +       }
> 
> This seems weird without there being a bug with the state itself, for
> instance how would it select the wrong bearer if it is not supported?
> Also the lack of proper explanation in the commit message doesn't help
> to grasp what is going on here, so please have backtrace or something
> attached since we need to understand why it would be selecting the
> wrong bearer, or perhaps the bearer is being advertised as supported
> when in fact it isn't?
> 
>>         state = get_state(device, bdaddr_type);
>>
>> --
>> 2.25.1
>>
>>
> 
>