Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200821064927.17578-1-sonnysasaka@chromium.org>
In-Reply-To: <20200821064927.17578-1-sonnysasaka@chromium.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 21 Aug 2020 10:22:18 -0700
Message-ID: <CABBYNZKW19qDviuBM-6MdnWnvR2+JO+nZOEpSFo7tVJo80v20w@mail.gmail.com>
Subject: Re: [PATCH BlueZ] adapter: remove eir_data.flags in device_update_last_seen()
To:     Sonny Sasaka <sonnysasaka@chromium.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Joseph Hwang <josephsih@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Sonny,

On Thu, Aug 20, 2020 at 11:52 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> From: Joseph Hwang <josephsih@chromium.org>
>
> Bluez has difficulty in pairing with Apple Airpods. This issue is
> caused by the incorrect selection of BD address type due to two factors:
>
> (1) The LE advertising reports emitted by Airpods do not carry eir
>     data flags.
> (2) If the eir data flags is missing, the found device would not be
>     considered as coming with bredr address for some historical
>     obsolete reason.
>
> This patch fixes (2) above.
>
> Tested on Chrome OS by pairing with Airpods.
> Without this patch, the pairing success rate is about 20%.
> With this patch, the pairing success rate becomes almost 100%.
>
> ---
>  src/adapter.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 5e896a9f0..36bbed2dd 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -6628,14 +6628,8 @@ static void update_found_devices(struct btd_adapter *adapter,
>
>         device_update_last_seen(dev, bdaddr_type);
>
> -       /*
> -        * FIXME: We need to check for non-zero flags first because
> -        * older kernels send separate adv_ind and scan_rsp. Newer
> -        * kernels send them merged, so once we know which mgmt version
> -        * supports this we can make the non-zero check conditional.
> -        */
> -       if (bdaddr_type != BDADDR_BREDR && eir_data.flags &&
> -                                       !(eir_data.flags & EIR_BREDR_UNSUP)) {
> +       if (bdaddr_type != BDADDR_BREDR &&
> +                       !(eir_data.flags & EIR_BREDR_UNSUP)) {
>                 device_set_bredr_support(dev);
>                 /* Update last seen for BR/EDR in case its flag is set */
>                 device_update_last_seen(dev, BDADDR_BREDR);
> --
> 2.26.2

While I'm fine with this change we might need to bump the kernel
version in order to remove the FIXME, also afaik
beacons/broadcaster/non-connectable advertisement will never have any
flags set so we need to make sure that doesn't end up updating the
last seen for BREDR, though if that is how the Airpods are advertising
then we will need to rethink how we handle the last seen logic to
select which bearer should be connected first. If the Airpods are also
responding to inquiry then we can probably check if BR/EDR has been
marked as supported when the device uses a broadcast advertisement and
not just mark it right away.

-- 
Luiz Augusto von Dentz