BUG: org.bluez.Device1.Connect() makes a BR/EDR connection to an LE-only Device on initial pairing

BUG: org.bluez.Device1.Connect() makes a BR/EDR connection to an LE-only Device on initial pairing

[Scott James Remnant]

The D-Bus API sequence for pairing a BR/EDR device is

  org.bluez.Device1.Pair()
  org.bluez.Device1.Connect()

The connect call is necessary after pairing successfully completes to
connect profiles, since a completed pairing does not do that
implicitly.

When this same sequence is used for an LE device, the Connect() call
fails with a Page Timeout. This is because of:

  a9f523e core: Split LE and BR/EDR states for devices

For LE devices there is only one "profile" connection to be made, ATT,
which is already established as part of the call to Pair() in order to
perform primary service discovery. And unlike the BR/EDR case, where
the ACL connection is dropped after a few seconds, the ATT connection
is then made available to other plugins (such as HoG).

Of course, the UI doesn't know this, the D-Bus API doesn't reveal that
this is really an LE device, so the UI makes the Connect() call to the
already connected device. Because of that CL, this then attempts to
bring up the BR/EDR bearer - and thus results in the Connect() call
failing because the device doesn't have one.

Scott
-- 
Scott James Remnant | Chrome OS Systems | keybuk@google.com | Google

[PATCH 1/2] core: don't set BR/EDR support when no flags present

[Scott James Remnant]

The logic for setting the BR/EDR supported flag checked for the
non-presence of the BR/EDR Unsupported flag without checking whether
any flags were present at all.

This meant all LE-only devices that returned Scan Response Data were
being marked as supporting BR/EDR, since the flag was only set in the
initial AD and not the SRD.
---
 src/adapter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/adapter.c b/src/adapter.c
index e396a3d..b8b9a9c 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -4342,7 +4342,8 @@ static void update_found_devices(struct btd_adapter *adapter,
 
 	device_update_last_seen(dev, bdaddr_type);
 
-	if (bdaddr_type != BDADDR_BREDR && !(eir_data.flags & EIR_BREDR_UNSUP))
+	if (bdaddr_type != BDADDR_BREDR && eir_data.flags &&
+					!(eir_data.flags & EIR_BREDR_UNSUP))
 		device_set_bredr_support(dev, true);
 
 	if (eir_data.name != NULL && eir_data.name_complete)
-- 
1.9.1.423.g4596e3a

[PATCH 2/2] core: don't try BR/EDR for LE-only devices

[Scott James Remnant]

Calling Connect() after Pair() should not fail for LE-only devices.
---
 src/device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/device.c b/src/device.c
index 56b1780..289d522 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1425,7 +1425,7 @@ static DBusMessage *dev_connect(DBusConnection *conn, DBusMessage *msg,
 
 	if (dev->bredr_state.connected)
 		bdaddr_type = dev->bdaddr_type;
-	else if (dev->le_state.connected)
+	else if (dev->le_state.connected && dev->bredr)
 		bdaddr_type = BDADDR_BREDR;
 	else
 		bdaddr_type = select_conn_bearer(dev);
-- 
1.9.1.423.g4596e3a

Re: [PATCH 1/2] core: don't set BR/EDR support when no flags present

[Johan Hedberg]

Hi Scott,

On Thu, Apr 03, 2014, Scott James Remnant wrote:
> The logic for setting the BR/EDR supported flag checked for the
> non-presence of the BR/EDR Unsupported flag without checking whether
> any flags were present at all.
> 
> This meant all LE-only devices that returned Scan Response Data were
> being marked as supporting BR/EDR, since the flag was only set in the
> initial AD and not the SRD.
> ---
>  src/adapter.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index e396a3d..b8b9a9c 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -4342,7 +4342,8 @@ static void update_found_devices(struct btd_adapter *adapter,
>  
>  	device_update_last_seen(dev, bdaddr_type);
>  
> -	if (bdaddr_type != BDADDR_BREDR && !(eir_data.flags & EIR_BREDR_UNSUP))
> +	if (bdaddr_type != BDADDR_BREDR && eir_data.flags &&
> +					!(eir_data.flags & EIR_BREDR_UNSUP))
>  		device_set_bredr_support(dev, true);
>  
>  	if (eir_data.name != NULL && eir_data.name_complete)

Both patches have been applied. Thanks.

One thing to note regarding this first one: this is actually not a
problem with current bluetooth-next since there we merge the adv_ind and
scan_rsp into a single mgmt device found event. I.e. with such a kernel
we will not need to have this extra non-zero flags check. I added an
extra code comment to this place so we don't forget to fix it.

Johan

Re: [PATCH 1/2] core: don't set BR/EDR support when no flags present

[Scott James Remnant]

On Fri, Apr 4, 2014 at 12:25 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:

> One thing to note regarding this first one: this is actually not a
> problem with current bluetooth-next since there we merge the adv_ind and
> scan_rsp into a single mgmt device found event. I.e. with such a kernel
> we will not need to have this extra non-zero flags check. I added an
> extra code comment to this place so we don't forget to fix it.
>

Makes sense, we're testing with the current
bluetooth-next/for-upstream tag, and it looks like the
ADV_IND/SCAN_RSP merging patches aren't submitted yet ;-)

Won't this mean that the flag checking will be needed on kernels prior
to 3.15 in either case?

Scott
-- 
Have you ever, ever felt like this?
Had strange things happen?  Are you going round the twist?

Re: [PATCH 1/2] core: don't set BR/EDR support when no flags present

[Marcel Holtmann]

Hi Scott,

>> One thing to note regarding this first one: this is actually not a
>> problem with current bluetooth-next since there we merge the adv_ind and
>> scan_rsp into a single mgmt device found event. I.e. with such a kernel
>> we will not need to have this extra non-zero flags check. I added an
>> extra code comment to this place so we don't forget to fix it.
>> 
> 
> Makes sense, we're testing with the current
> bluetooth-next/for-upstream tag, and it looks like the
> ADV_IND/SCAN_RSP merging patches aren't submitted yet ;-)
> 
> Won't this mean that the flag checking will be needed on kernels prior
> to 3.15 in either case?

they will be needed for kernels < 3.16 since that will be the earliest kernel this gets merged.

The 3.15 merge window is closed for us. What you have with bluetooth-next/for-upstream is what is merged into 3.15 and from now on only bug fixes are allowed.

Regards

Marcel

Re: [PATCH 1/2] core: don't set BR/EDR support when no flags present

[Scott James Remnant]

On Fri, Apr 4, 2014 at 8:57 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>>> One thing to note regarding this first one: this is actually not a
>>> problem with current bluetooth-next since there we merge the adv_ind and
>>> scan_rsp into a single mgmt device found event. I.e. with such a kernel
>>> we will not need to have this extra non-zero flags check. I added an
>>> extra code comment to this place so we don't forget to fix it.
>>>
>>
>> Makes sense, we're testing with the current
>> bluetooth-next/for-upstream tag, and it looks like the
>> ADV_IND/SCAN_RSP merging patches aren't submitted yet ;-)
>>
>> Won't this mean that the flag checking will be needed on kernels prior
>> to 3.15 in either case?
>
> they will be needed for kernels < 3.16 since that will be the earliest kernel this gets merged.
>

Right, that's what I would have meant if I'd had more coffee.

> The 3.15 merge window is closed for us. What you have with bluetooth-next/for-upstream is what is merged into 3.15 and from now on only bug fixes are allowed.
>

Great! That's what I understood, our current plan for Chrome OS 36 is
to ship with at least BlueZ 5.17, and then use the 3.15 kernel
bluetooth stack backported to 3.10 and 3.8.  (We'll also continue to
use the 3.8 stack on 3.4 on our oldest hardware.)

This backported stack is what we've been testing with

Scott
-- 
Have you ever, ever felt like this?
Had strange things happen?  Are you going round the twist?

Re: [PATCH 1/2] core: don't set BR/EDR support when no flags present

[Johan Hedberg]

Hi Scott,

On Fri, Apr 04, 2014, Scott James Remnant wrote:
> On Fri, Apr 4, 2014 at 12:25 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> 
> > One thing to note regarding this first one: this is actually not a
> > problem with current bluetooth-next since there we merge the adv_ind and
> > scan_rsp into a single mgmt device found event. I.e. with such a kernel
> > we will not need to have this extra non-zero flags check. I added an
> > extra code comment to this place so we don't forget to fix it.
> >
> 
> Makes sense, we're testing with the current
> bluetooth-next/for-upstream tag, and it looks like the
> ADV_IND/SCAN_RSP merging patches aren't submitted yet ;-)
> 
> Won't this mean that the flag checking will be needed on kernels prior
> to 3.15 in either case?

Correct. Prior to 3.16 actually since that's where the merging patches
will land. Once we know the mgmt revision that 3.16 will have (probably
the current one + 1) we can add a check for this to the user space code.

Johan