Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3156-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <75805a80-f714-4cc4-9929-72992399c30b@collabora.com>
Date: Wed, 3 Apr 2024 18:35:39 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH BlueZ] plugins: Add new plugin to fix airpods pairing
To: Szymon Janc <szymon.janc@codecoup.pl>,
 Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
References: <20240403110808.805771-1-frederic.danis@collabora.com>
 <CAAEJBhL6pb=PeO_mezcga+aVxDew+zQwTDhTWkHT-wnfXONHtQ@mail.gmail.com>
Content-Language: en-US
From: =?UTF-8?Q?Fr=C3=A9d=C3=A9ric_Danis?= <frederic.danis@collabora.com>
In-Reply-To: <CAAEJBhL6pb=PeO_mezcga+aVxDew+zQwTDhTWkHT-wnfXONHtQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi,

On 03/04/2024 15:50, Szymon Janc wrote:
> Hi,
>
> On Wed, 3 Apr 2024 at 13:08, Frédéric Danis
> <frederic.danis@collabora.com> wrote:
>> Apple Airpods are discoverable and pairable in BREDR mode, but also
>> advertise in unconnectable mode in LE with the same Public address, at the
>> same time. As the pairing process uses the latest seen address, sometimes
>> it uses the LE Public address to pair, which fails.
>>
>> This commit adds a new adapter driver plugin which force the BREDR last
>> seen time on LE Public address device found event related to an Apple
>> device, allowing pairing process to always use the BREDR.
>>
> While I'm fine with having plugin for this specific case, I wonder if
> select_conn_bearer() shouldn't take
> bearer's connectable state into account when selecting bearer for connection

Yes, this could be a lot simpler, but couldn't it break some other 
pairing case?

>> This commit is based on proposal
>> https://lore.kernel.org/all/20240103101328.1812899-1-clancy_shang@163.com/
>> ---
>>   Makefile.plugins  |   3 ++
>>   plugins/airpods.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 135 insertions(+)
>>   create mode 100644 plugins/airpods.c
>>
>> diff --git a/Makefile.plugins b/Makefile.plugins
>> index 4aa2c9c92..2ebd8aaf6 100644
>> --- a/Makefile.plugins
>> +++ b/Makefile.plugins
>> @@ -11,6 +11,9 @@ builtin_sources += plugins/autopair.c
>>   builtin_modules += policy
>>   builtin_sources += plugins/policy.c
>>
>> +builtin_modules += airpods
>> +builtin_sources += plugins/airpods.c
>> +
>>   if ADMIN
>>   builtin_modules += admin
>>   builtin_sources += plugins/admin.c
>> diff --git a/plugins/airpods.c b/plugins/airpods.c
>> new file mode 100644
>> index 000000000..5043f0cca
>> --- /dev/null
>> +++ b/plugins/airpods.c
>> @@ -0,0 +1,132 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + *
>> + *  BlueZ - Bluetooth protocol stack for Linux
>> + *
>> + *  Copyright (C) 2024 Frédéric Danis <frederic.danis@collabora.com>
>> + *
>> + *
>> + */
>> +
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +
>> +#include <stdbool.h>
>> +
>> +#include <glib.h>
>> +
>> +#include "bluetooth/bluetooth.h"
>> +
>> +#include "lib/mgmt.h"
>> +#include "src/plugin.h"
>> +#include "src/adapter.h"
>> +#include "src/device.h"
>> +#include "src/eir.h"
>> +#include "src/log.h"
>> +#include "src/shared/mgmt.h"
>> +#include "src/shared/util.h"
>> +
>> +#define APPLE_INC_VENDOR_ID 0x004c
>> +
>> +static struct mgmt *mgmt;
>> +
>> +static bool eir_msd_is_apple_inc(GSList *msd_list)
>> +{
>> +       GSList *msd_l, *msd_next;
>> +
>> +       for (msd_l = msd_list; msd_l != NULL; msd_l = msd_next) {
>> +               const struct eir_msd *msd = msd_l->data;
>> +
>> +               msd_next = g_slist_next(msd_l);
>> +
>> +               if (msd->company == APPLE_INC_VENDOR_ID)
>> +                       return true;
>> +       }
>> +
>> +       return false;
>> +}
>> +
>> +static void airpods_device_found_callback(uint16_t index, uint16_t length,
>> +                                       const void *param, void *user_data)
>> +{
>> +       struct btd_device *dev;
>> +       const struct mgmt_ev_device_found *ev = param;
>> +       struct btd_adapter *adapter = user_data;
>> +       uint16_t eir_len;
>> +       uint32_t flags = le32_to_cpu(ev->flags);
>> +       struct eir_data eir_data;
>> +
>> +       dev = btd_adapter_find_device(adapter,  &ev->addr.bdaddr,
>> +                                       ev->addr.type);
>> +       if (!dev)
>> +               return;
>> +
>> +       if (length < sizeof(*ev)) {
>> +               warn("Too short device found event (%u bytes)", length);
>> +               return;
>> +       }
>> +
>> +       eir_len = btohs(ev->eir_len);
>> +       if (length != sizeof(*ev) + eir_len) {
>> +               warn("Device found event size mismatch (%u != %zu)",
>> +                                       length, sizeof(*ev) + eir_len);
>> +               return;
>> +       }
>> +
>> +       if (eir_len == 0)
>> +               return;
>> +
>> +       memset(&eir_data, 0, sizeof(eir_data));
>> +       eir_parse(&eir_data, ev->eir, eir_len);
>> +
>> +       if (eir_msd_is_apple_inc(eir_data.msd_list) &&
>> +                               (flags & MGMT_DEV_FOUND_NOT_CONNECTABLE) &&
>> +                               (ev->addr.type == BDADDR_LE_PUBLIC)) {
>> +               DBG("Force BREDR last seen");
>> +               device_set_bredr_support(dev);
>> +               device_update_last_seen(dev, BDADDR_BREDR, true);
>> +       }
>> +}
>> +
>> +static int airpods_probe(struct btd_adapter *adapter)
>> +{
>> +       if (!mgmt)
>> +               mgmt = mgmt_new_default();
>> +
>> +       if (!mgmt) {
>> +               fprintf(stderr, "Failed to open management socket\n");
>> +               return 0;
>> +       }
>> +
>> +       mgmt_register(mgmt, MGMT_EV_DEVICE_FOUND,
>> +                                       btd_adapter_get_index(adapter),
>> +                                       airpods_device_found_callback,
>> +                                       adapter, NULL);
>> +
>> +       return 0;
>> +}
>> +
>> +static void airpods_remove(struct btd_adapter *adapter)
>> +{
>> +       mgmt_unregister_index(mgmt, btd_adapter_get_index(adapter));
>> +}
>> +
>> +static struct btd_adapter_driver airpods_driver = {
>> +       .name   = "airpods",
>> +       .probe  = airpods_probe,
>> +       .remove = airpods_remove,
>> +};
>> +
>> +static int airpods_init(void)
>> +{
>> +       return btd_register_adapter_driver(&airpods_driver);
>> +}
>> +
>> +static void airpods_exit(void)
>> +{
>> +       btd_unregister_adapter_driver(&airpods_driver);
>> +}
>> +
>> +BLUETOOTH_PLUGIN_DEFINE(airpods, VERSION,
>> +               BLUETOOTH_PLUGIN_PRIORITY_LOW, airpods_init, airpods_exit)
>> --
>> 2.34.1
>>
>>
>

-- 
Frédéric Danis
Senior Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, United Kingdom
Registered in England & Wales, no. 5513718