Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230725084431.640332-1-simon.mikuda@streamunlimited.com> <20230725084431.640332-6-simon.mikuda@streamunlimited.com>
In-Reply-To: <20230725084431.640332-6-simon.mikuda@streamunlimited.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 25 Jul 2023 12:12:22 -0700
Message-ID: <CABBYNZJ81vnZNnAhBFyu7_-5Mr0yxEtprk-7Gd6ZtzZNvx5nMA@mail.gmail.com>
Subject: Re: [PATCH BlueZ 5/8] device: Fix pairing and discovery with dual
 mode devices
To:     Simon Mikuda <simon.mikuda@streamunlimited.com>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Simon,

On Tue, Jul 25, 2023 at 1:56=E2=80=AFAM Simon Mikuda
<simon.mikuda@streamunlimited.com> wrote:
>
> We'll prefer to pair and discover services on connected bearer first.
>
> There was a problem with pairing, that select_conn_bearer returned BR/EDR
> even when we have connection to LE bearer only. In these situation we sho=
uld
> pair over connected bearer, since connection to another bearer can fail.
>
> Similar problem with discovery that after connection on LE bearer discove=
ry
> was requested on BR/EDR bearer which can cause connection error (e.g. Pag=
e
> Timeout).
> ---
>  src/device.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index 6f28e261e..446e978ee 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -2504,6 +2504,16 @@ static uint8_t select_conn_bearer(struct btd_devic=
e *dev)
>         return dev->bdaddr_type;
>  }
>
> +static uint8_t select_active_bearer(struct btd_device *dev)
> +{
> +       if (dev->bredr_state.connected)
> +               return BDADDR_BREDR;
> +       else if (dev->le_state.connected)
> +               return dev->bdaddr_type =3D=3D BDADDR_BREDR
> +                       ? BDADDR_LE_PUBLIC : dev->bdaddr_type;

The code above assumes BR/EDR has the priority in case both are
connected, perhaps we should have a clear policy in case both are
connected.

> +       return select_conn_bearer(dev);
> +}
> +
>  static DBusMessage *dev_connect(DBusConnection *conn, DBusMessage *msg,
>                                                         void *user_data)
>  {
> @@ -3018,7 +3028,7 @@ static DBusMessage *pair_device(DBusConnection *con=
n, DBusMessage *msg,
>         else if (device->le_state.bonded)
>                 bdaddr_type =3D BDADDR_BREDR;
>         else
> -               bdaddr_type =3D select_conn_bearer(device);
> +               bdaddr_type =3D select_active_bearer(device);
>
>         state =3D get_state(device, bdaddr_type);
>
> @@ -3055,7 +3065,7 @@ static DBusMessage *pair_device(DBusConnection *con=
n, DBusMessage *msg,
>                         err =3D device_connect_le(device);
>                 else
>                         err =3D adapter_create_bonding(adapter, &device->=
bdaddr,
> -                                                       device->bdaddr_ty=
pe,
> +                                                       bdaddr_type,
>                                                         io_cap);
>         } else {
>                 err =3D adapter_create_bonding(adapter, &device->bdaddr,
> @@ -6207,12 +6217,9 @@ static bool start_discovery_cb(gpointer user_data)
>  {
>         struct btd_device *device =3D user_data;
>
> -       if (device->bredr)
> -               device_browse_sdp(device, NULL);
> -       else
> -               device_browse_gatt(device, NULL);
> -
>         device->discov_timer =3D 0;
> +       device_discover_services(device, select_active_bearer(device),
> +                       NULL);

Perhaps in case of discovery we could do both in parallel, although if
the remote side supports GATT services as part of SDP we may end up
with redundant discovery but bt_gatt_client/gatt_db shall be able to
handle that.

>         return FALSE;
>  }
> --
> 2.34.1
>


--=20
Luiz Augusto von Dentz