Re: [PATCH BlueZ] gatt: Clear subscriptions for device not paired

[Yunhan Wang <yunhanw@google.com>]

Hi, Luiz

On Mon, Oct 23, 2017 at 1:12 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Yunha,
>
> On Sat, Oct 21, 2017 at 3:26 AM, Yunhan Wang <yunhanw@google.com> wrote:
>> Hi, Luiz
>>
>> I assume that you wanna see that server application try to do
>> send_notification_to_device,  device is NULL/disconnected, then
>> bluetoothd would clear status for subscription.
>>
>> However, when ble is disconnected, send_notification_to_device might
>> not be triggered any more so that device state cannot be cleaned. In
>> server I have used StartNotify and AcquireNotify to verify it, and
>> send_notification_to_device cannot be triggered after ble is
>> disconnected even though server application has tried to send data.
>
> bluetoothd[28769]: src/gatt-client.c:btd_gatt_client_disconnected()
> Device disconnected. Cleaning up.
> bluetoothd[28769]: src/device.c:att_disconnected_cb() Automatic
> connection disabled
> ...
> bluetoothd[28769]: src/gatt-database.c:ccc_write_cb() External CCC
> write received with value: 0x0000
>
> It seems to be working alright here, perhaps you are saying it doesn't
> work if the notification is not called while the device is disconnect
> then we have a problem, so we need a proper hook for a device being
> disconnect not only on demand when the device attempts to send a
> notification?

Yes

>
>> Here are two ideas:
>>
>> In gatt-datebase.c, could you invoke clear_ccc_state inside pipe_up?
>> When server application destroy pipe, the corresponding pipe_up in
>> bluetoothd could remove GATT states that related to notify_io.
>
> If we got a hup while the remote believes it is subscribed this can
> cause it to go out of sync, so this should only really happen if you
> are removing the whole service so the remote can notice the change via
> service changed indication.
>
Yes, I agree.

>> Another idea:
>> When disconnected event is detected in adapter.c or device.c, could
>> you call clear_ccc_state to clear all states for all characteristic,
>> reset them to original status.
>
> Only if the device has not been paired, then yes we should clear any
> ccc state related to the device being disconnected.
>
Yes, I agree.
>>
>> Thanks
>> Best wishes
>> Yunhan
>>
>> On Fri, Oct 20, 2017 at 5:21 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> If the device is no longer valid or is not considered bonded anymore
>>> clear its CCC states before removing otherwise application may continue
>>> to notify when there are no devices listening.
>>> ---
>>>  src/gatt-database.c | 22 +++++++++++++++++++++-
>>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>>> index 47304704a..d6d1e4d13 100644
>>> --- a/src/gatt-database.c
>>> +++ b/src/gatt-database.c
>>> @@ -891,6 +891,23 @@ static void conf_cb(void *user_data)
>>>         }
>>>  }
>>>
>>> +static void clear_ccc_state(void *data, void *user_data)
>>> +{
>>> +       struct ccc_state *ccc = data;
>>> +       struct btd_gatt_database *db = user_data;
>>> +       struct ccc_cb_data *ccc_cb;
>>> +
>>> +       if (!ccc->value[0])
>>> +               return;
>>> +
>>> +       ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle,
>>> +                                               UINT_TO_PTR(ccc->handle));
>>> +       if (!ccc_cb)
>>> +               return;
>>> +
>>> +       ccc_cb->callback(NULL, 0, ccc_cb->user_data);
>>> +}
>>> +
>>>  static void send_notification_to_device(void *data, void *user_data)
>>>  {
>>>         struct device_state *device_state = data;
>>> @@ -940,8 +957,11 @@ static void send_notification_to_device(void *data, void *user_data)
>>>
>>>  remove:
>>>         /* Remove device state if device no longer exists or is not paired */
>>> -       if (queue_remove(notify->database->device_states, device_state))
>>> +       if (queue_remove(notify->database->device_states, device_state)) {
>>> +               queue_foreach(device_state->ccc_states, clear_ccc_state,
>>> +                                               notify->database);
>>>                 device_state_free(device_state);
>>> +       }
>>>  }
>>>
>>>  static void send_notification_to_devices(struct btd_gatt_database *database,
>>> --
>>> 2.13.6
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz