2021-01-08 21:18:42

by Luiz Augusto von Dentz

Subject: [PATCH BlueZ 4/4] gatt: Fix assuming service changed has been subscribed

From: Luiz Augusto von Dentz <[email protected]>

Unfortunately assuming service changed has been subscribed may cause
indication to time out in some peripherals (Logitech M720 Triathlon, Mx
Anywhere 2, Lenovo Mice N700, RAPOO BleMouse and Microsoft Designer
Mouse) even though the expect actually mandates that the client responds
with confirmation these peripherals just ignores it completely which
leads them to be disconnected whenever bluetoothd is restarted or the
system reboots.
---
src/device.c | 11 ++---------
src/gatt-database.c | 2 +-
2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/src/device.c b/src/device.c
index fe885aa64..af13badfc 100644
--- a/src/device.c
+++ b/src/device.c
@@ -5831,18 +5831,11 @@ void device_load_svc_chng_ccc(struct btd_device *device, uint16_t *ccc_le,
key_file = g_key_file_new();
g_key_file_load_from_file(key_file, filename, 0, NULL);

- /*
- * If there is no "ServiceChanged" section we may be loading data from
- * old version which did not persist Service Changed CCC values. Let's
- * check if we are bonded and assume indications were enabled by peer
- * in such case - it should have done this anyway.
- */
if (!g_key_file_has_group(key_file, "ServiceChanged")) {
if (ccc_le)
- *ccc_le = device->le_state.bonded ? 0x0002 : 0x0000;
+ *ccc_le = 0x0000;
if (ccc_bredr)
- *ccc_bredr = device->bredr_state.bonded ?
- 0x0002 : 0x0000;
+ *ccc_bredr = 0x0000;
g_key_file_free(key_file);
return;
}
diff --git a/src/gatt-database.c b/src/gatt-database.c
index b7d2bea1d..d99604826 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -333,7 +333,7 @@ static void att_disconnected(int err, void *user_data)
handle = gatt_db_attribute_get_handle(state->db->svc_chngd_ccc);

ccc = find_ccc_state(state, handle);
- if (ccc)
+ if (ccc && ccc->value)
device_store_svc_chng_ccc(device, state->bdaddr_type,
ccc->value);

--
2.26.2

2021-01-20 11:22:32

by Sathish Narasimman

Subject: Re: [PATCH BlueZ 4/4] gatt: Fix assuming service changed has been subscribed

Hi Luiz

On Sat, Jan 9, 2021 at 2:48 AM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> From: Luiz Augusto von Dentz <[email protected]>
>
> Unfortunately assuming service changed has been subscribed may cause
> indication to time out in some peripherals (Logitech M720 Triathlon, Mx
> Anywhere 2, Lenovo Mice N700, RAPOO BleMouse and Microsoft Designer
> Mouse) even though the expect actually mandates that the client responds
> with confirmation these peripherals just ignores it completely which
> leads them to be disconnected whenever bluetoothd is restarted or the
> system reboots.
> ---
> src/device.c | 11 ++---------
> src/gatt-database.c | 2 +-
> 2 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index fe885aa64..af13badfc 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -5831,18 +5831,11 @@ void device_load_svc_chng_ccc(struct btd_device *device, uint16_t *ccc_le,
> key_file = g_key_file_new();
> g_key_file_load_from_file(key_file, filename, 0, NULL);
>
> - /*
> - * If there is no "ServiceChanged" section we may be loading data from
> - * old version which did not persist Service Changed CCC values. Let's
> - * check if we are bonded and assume indications were enabled by peer
> - * in such case - it should have done this anyway.
> - */
> if (!g_key_file_has_group(key_file, "ServiceChanged")) {
> if (ccc_le)
> - *ccc_le = device->le_state.bonded ? 0x0002 : 0x0000;
> + *ccc_le = 0x0000;
> if (ccc_bredr)
> - *ccc_bredr = device->bredr_state.bonded ?
> - 0x0002 : 0x0000;
> + *ccc_bredr = 0x0000;
> g_key_file_free(key_file);
> return;
> }
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index b7d2bea1d..d99604826 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -333,7 +333,7 @@ static void att_disconnected(int err, void *user_data)
> handle = gatt_db_attribute_get_handle(state->db->svc_chngd_ccc);
>
> ccc = find_ccc_state(state, handle);
> - if (ccc)
> + if (ccc && ccc->value)
> device_store_svc_chng_ccc(device, state->bdaddr_type,
> ccc->value);
>
> --
> 2.26.2
>

Was this patch is merged?

Regards
Sathish N

2021-01-20 15:39:30

by Luiz Augusto von Dentz

Subject: Re: [PATCH BlueZ 4/4] gatt: Fix assuming service changed has been subscribed

Hi Sathish,

On Wed, Jan 20, 2021 at 1:30 AM Sathish Narasimman <[email protected]> wrote:
>
> Hi Luiz
>
> On Sat, Jan 9, 2021 at 2:48 AM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > From: Luiz Augusto von Dentz <[email protected]>
> >
> > Unfortunately assuming service changed has been subscribed may cause
> > indication to time out in some peripherals (Logitech M720 Triathlon, Mx
> > Anywhere 2, Lenovo Mice N700, RAPOO BleMouse and Microsoft Designer
> > Mouse) even though the expect actually mandates that the client responds
> > with confirmation these peripherals just ignores it completely which
> > leads them to be disconnected whenever bluetoothd is restarted or the
> > system reboots.
> > ---
> > src/device.c | 11 ++---------
> > src/gatt-database.c | 2 +-
> > 2 files changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/device.c b/src/device.c
> > index fe885aa64..af13badfc 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -5831,18 +5831,11 @@ void device_load_svc_chng_ccc(struct btd_device *device, uint16_t *ccc_le,
> > key_file = g_key_file_new();
> > g_key_file_load_from_file(key_file, filename, 0, NULL);
> >
> > - /*
> > - * If there is no "ServiceChanged" section we may be loading data from
> > - * old version which did not persist Service Changed CCC values. Let's
> > - * check if we are bonded and assume indications were enabled by peer
> > - * in such case - it should have done this anyway.
> > - */
> > if (!g_key_file_has_group(key_file, "ServiceChanged")) {
> > if (ccc_le)
> > - *ccc_le = device->le_state.bonded ? 0x0002 : 0x0000;
> > + *ccc_le = 0x0000;
> > if (ccc_bredr)
> > - *ccc_bredr = device->bredr_state.bonded ?
> > - 0x0002 : 0x0000;
> > + *ccc_bredr = 0x0000;
> > g_key_file_free(key_file);
> > return;
> > }
> > diff --git a/src/gatt-database.c b/src/gatt-database.c
> > index b7d2bea1d..d99604826 100644
> > --- a/src/gatt-database.c
> > +++ b/src/gatt-database.c
> > @@ -333,7 +333,7 @@ static void att_disconnected(int err, void *user_data)
> > handle = gatt_db_attribute_get_handle(state->db->svc_chngd_ccc);
> >
> > ccc = find_ccc_state(state, handle);
> > - if (ccc)
> > + if (ccc && ccc->value)
> > device_store_svc_chng_ccc(device, state->bdaddr_type,
> > ccc->value);
> >
> > --
> > 2.26.2
> >
>
> Was this patch is merged?

Yes, https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=39054d59c0ecdb102f8aa352cb7aa6fcbd7f2b6b

> Regards
> Sathish N



--
Luiz Augusto von Dentz