Return-Path: MIME-Version: 1.0 In-Reply-To: References: <20170523195650.29695-1-mcchou@chromium.org> <20170525001054.92520-1-mcchou@chromium.org> From: =?UTF-8?Q?Giovanni_Ortu=C3=B1o?= Date: Sat, 27 May 2017 21:54:28 -0700 Message-ID: Subject: Re: [PATCH v3] device: Clear GATT DB after disconnection for unpaired device To: Miao-chen Chou Cc: "Von Dentz, Luiz" , "linux-bluetooth@vger.kernel.org" , josephsih@chromium.org, sonnysasaka@chromium.org, Rahul Chaturvedi Content-Type: text/plain; charset="UTF-8" Sender: linux-bluetooth-owner@vger.kernel.org List-ID: On Fri, May 26, 2017 at 11:10 AM, Miao-chen Chou wrote: > Hi Luiz, > > The user space apps follow the procedure defined in the spec to > re-register the notification after re-connection for the unpaired > device. Skipping the error doesn't seem to be a good workaround, since > no notification is wanted unless the user space apps explicitly ask > for it, and there can be other implicit notifications even if we skip > one. Also, the Service Changed Characteristic does not matter in the > case of unpaired devices, and there is no such check in BlueZ > currently as well. > +1 > For other system supporting auto-connect, keeping the caches for > unpaired device after disconnection can also be wrong in practice. For > instance, the local device can connect to a different device which has > the exact same address but with totally different GATT services, and > the caching doesn't help in this case. > > The first workaround that I mentioned is to prevent re-register > subscribed notification clients and clear them, and the second > workaround is to clear the subscribed notification during > disconnection so that there won't be any clients to be re-registered > for notification upon re-connection. > > Thanks, > Miao > > On Fri, May 26, 2017 at 1:42 AM, Von Dentz, Luiz > wrote: >> Hi Miao, >> >> On Fri, May 26, 2017 at 10:20 AM, Miao-chen Chou wro= te: >>> Hi, >>> >>> Thanks for your comments. Indeed the auto-connection(reconnection) >>> policy is not enabled in Chrome OS. However, the caching for unpaired >>> device is causing registration failures when users try to register for >>> the notification after reconnecting to the device which was unpaired >>> in the previous connection, and there is no such failure on Mac and >>> Android afaIk. Please correct me if I was wrong, but I didn't find any >>> logic checking whether the service changed characteristic was >>> presented in the previous connection. The problem we are trying to >>> solve here is that BlueZ returns operation failed error on registering >>> for notification after reconnecting to an unpaired device. And this is >>> caused by the caching which should be reset according to the spec. >>> >>> [Scenario] >>> connect to the remote device without pairing >>> register for notification >>> disconnect from the remote device >>> reconnect without pairing >>> register for notification --> operation failure with "Already >>> notifying" error message >> >> Well you could just ignore if already notifying, or that still doesn't >> notify? That error perhaps is misleading as it doesn't change anything >> perhaps we should return no error. Btw, if Im not mistake we do >> re-enable the notifications when reconnecting: >> >> bluetoothd[8777]: src/gatt-client.c:register_notify() Re-register >> subscribed notification client >> >> < ACL Data TX: Handle 3585 flags 0x00 dlen 9 >> ATT: Write Request (0x12) len 4 >> Handle: 0x0011 >> Data: 0100 >> >>> Other possible workarounds without clearing GATT DB are >>> 1) Skip re-register notify in btd_gatt_client_connected() and clear >>> chrc->notify_clients and client->all_notify_clients for unpaired >>> devices. >>> 2) Clear chrc->service->client->all_notify_clients in >>> btd_gatt_client_disconnected() for unpaired devices. >> >> See above, I think we can just remove the error of already registered >> since we it should already be registered is just a NOP. >> >>> Any comments are appreciated. >>> >>> Thanks, >>> Miao >>> >>> On Thu, May 25, 2017 at 12:44 AM, Von Dentz, Luiz >>> wrote: >>>> Hi, >>>> >>>> On Thu, May 25, 2017 at 3:10 AM, wrote: >>>>> From: Miao-chen Chou >>>>> >>>>> This patch adds gatt_db_cleanup() function to clear client-role GATT = DB for the >>>>> unpaired device. According to Core Specification v4.2 [1] and v5.0 [2= ], the >>>>> attributes should be cleared between unbonded devices after disconnec= tion, since >>>>> the attributes are no long valid. >>>>> >>>>> [1] Page 525, Section 2.5.2 Attribute Caching, Part G, Volume 3: "For= clients >>>>> that do not have a trusted relationship with the server, the attribut= e cache is >>>>> valid only during the connection." >>>>> [2] Page 2227, Section 2.5.2 Attribute Caching, Part G, Volume 3: "Fo= r clients >>>>> that do not have a trusted relationship with the server, the attribut= e cache is >>>>> valid only during the connection." >>>> >>>> This ignores the fact that devices which don't implement service >>>> changed characteristic shall not change its database so the discovery >>>> may be skipped: >>>> >>>> BLUETOOTH SPECIFICATION Version 5.0 | Vol 3, Part C >>>> page 2079 >>>> "If the client had previously determined that the server did not have >>>> the =C2=ABService Changed=C2=BB characteristic then service discovery = may be >>>> skipped." >>>> I think this spec quote is being taken out of context. That paragraph specifically talks about rebonding and as I understand it does not apply to all devices: > If a higher layer determines the bond no longer exists on the server, the= client must, > after user interaction to confirm the remote device, re-bond, perform ser= vice discovery > and re-configure the server. If the client had previously determined that= the server did > not have the =C2=ABService Changed=C2=BB characteristic then service disc= overy may be skipped. >>>> Also note that we do persist the cache to be able to persist >>>> configuration such as notifications and indications, perhaps in chrome >>>> you don't really have a use for it but in systems where reconnection >>>> is supported, afaik chrome OS doesn't, it is a must have since >>>> notifications and indications would not work during the discovery if >>>> we remove the cache. And please remember that discovery is a very slow >>>> procedure, in most cases it takes a good 3-5 seconds to rediscover, >>>> depending on db size it may take over 10 seconds and until it is >>>> completed no attribute can be accessed. >>>> We recognize that rediscovery is an expensive procedure and we do have projects that struggle because of this but as Miao-Chen stated not following the spec is not the right solution. Besides, there are other initiatives inside the Bluetooth SIG tha= t would allow caching of the database thus solving this problem across all platforms while still following the spec. >>>> We could, however, have a config option to select the behavior of the >>>> cache or have an option for auto-connect so disabling it also disable >>>> caching. >>>> >>>>> --- >>>>> src/device.c | 13 +++++++++++++ >>>>> 1 file changed, 13 insertions(+) >>>>> >>>>> diff --git a/src/device.c b/src/device.c >>>>> index 1b05561e4..f8cde0b43 100644 >>>>> --- a/src/device.c >>>>> +++ b/src/device.c >>>>> @@ -267,6 +267,7 @@ struct pending_sdp_query { >>>>> >>>>> static int device_browse_gatt(struct btd_device *device, DBusMessage= *msg); >>>>> static int device_browse_sdp(struct btd_device *device, DBusMessage = *msg); >>>>> +static void store_gatt_db(struct btd_device *device); >>>>> >>>>> static struct pending_sdp_query *pending_sdp_query_new(DBusConnectio= n *conn, >>>>> DBusMessage *msg, const struct btd_de= vice *dev) >>>>> @@ -546,6 +547,15 @@ static void browse_request_free(struct browse_re= q *req) >>>>> g_free(req); >>>>> } >>>>> >>>>> +static void gatt_db_cleanup(struct btd_device* device) >>>>> +{ >>>>> + if (!device->db) >>>>> + return; >>>>> + >>>>> + gatt_db_clear(device->db); >>>>> + store_gatt_db(device); >>>>> +} >>>>> + >>>>> static void gatt_client_cleanup(struct btd_device *device) >>>>> { >>>>> if (!device->client) >>>>> @@ -578,6 +588,9 @@ static void attio_cleanup(struct btd_device *devi= ce) >>>>> device->att_io =3D NULL; >>>>> } >>>>> >>>>> + if (!device->bredr_state.bonded && !device->le_state.bonded) >>>>> + gatt_db_cleanup(device); >>>>> + >>>>> gatt_client_cleanup(device); >>>>> gatt_server_cleanup(device); >>>>> >>>>> -- >>>>> 2.13.0.219.gdb65acc882-goog >>>>> > -- > 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