Return-Path: MIME-Version: 1.0 In-Reply-To: <1420696108-29699-10-git-send-email-armansito@chromium.org> References: <1420696108-29699-1-git-send-email-armansito@chromium.org> <1420696108-29699-10-git-send-email-armansito@chromium.org> Date: Mon, 12 Jan 2015 20:04:09 -0200 Message-ID: Subject: Re: [PATCH BlueZ v2 09/14] core: device: Fix GATT profile probing From: Luiz Augusto von Dentz To: Arman Uguray Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Arman, On Thu, Jan 8, 2015 at 3:48 AM, Arman Uguray wrote: > This patch fixes a recently introduced issue which changed the behavior > of profile probing for GATT-based profiles to create a btd_service > instance for all discovered GATT services to be on a per-UUID basis > rather than per-service. > --- > src/device.c | 176 ++++++++++++++++++++++------------------------------------- > 1 file changed, 64 insertions(+), 112 deletions(-) > > diff --git a/src/device.c b/src/device.c > index 1236698..a9dc22d 100644 > --- a/src/device.c > +++ b/src/device.c > @@ -295,21 +295,15 @@ static GSList *find_service_with_state(GSList *list, > return NULL; > } > > -static GSList *find_service_with_gatt_handles(GSList *list, > - uint16_t start_handle, > - uint16_t end_handle) > +static GSList *find_service_with_uuid(GSList *list, char *uuid) > { > GSList *l; > - uint16_t svc_start, svc_end; > > for (l = list; l != NULL; l = g_slist_next(l)) { > struct btd_service *service = l->data; > + struct btd_profile *profile = btd_service_get_profile(service); > > - if (!btd_service_get_gatt_handles(service, &svc_start, > - &svc_end)) > - continue; > - > - if (svc_start == start_handle && svc_end == end_handle) > + if (bt_uuid_strcmp(profile->remote_uuid, uuid) == 0) > return l; > } > > @@ -2420,6 +2414,7 @@ static void add_primary(struct gatt_db_attribute *attr, void *user_data) > static void device_add_uuids(struct btd_device *device, GSList *uuids) > { > GSList *l; > + bool changed = false; > > for (l = uuids; l != NULL; l = g_slist_next(l)) { > GSList *match = g_slist_find_custom(device->uuids, l->data, > @@ -2427,12 +2422,14 @@ static void device_add_uuids(struct btd_device *device, GSList *uuids) > if (match) > continue; > > + changed = true; > device->uuids = g_slist_insert_sorted(device->uuids, > g_strdup(l->data), > bt_uuid_strcmp); > } > > - g_dbus_emit_property_changed(dbus_conn, device->path, > + if (changed) > + g_dbus_emit_property_changed(dbus_conn, device->path, > DEVICE_INTERFACE, "UUIDs"); > } > > @@ -2444,8 +2441,6 @@ struct gatt_probe_data { > GSList *uuids; > struct gatt_db_attribute *cur_attr; > char cur_uuid[MAX_LEN_UUID_STR]; > - uint16_t start_handle, end_handle; > - GSList *valid_services; > }; > > static bool device_match_profile(struct btd_device *device, > @@ -2473,8 +2468,7 @@ static void dev_probe_gatt(struct btd_profile *p, void *user_data) > if (!p->remote_uuid || bt_uuid_strcmp(p->remote_uuid, data->cur_uuid)) > return; > > - service = service_create_gatt(data->dev, p, data->start_handle, > - data->end_handle); > + service = service_create(data->dev, p); > if (!service) > return; > > @@ -2487,72 +2481,6 @@ static void dev_probe_gatt(struct btd_profile *p, void *user_data) > gatt_db_service_set_active(data->cur_attr, false); > > data->dev->services = g_slist_append(data->dev->services, service); > - > - if (data->all_services) > - data->valid_services = g_slist_append(data->valid_services, > - service); > -} > - > -static bool match_existing_service(struct gatt_probe_data *data) > -{ > - struct btd_device *dev = data->dev; > - struct btd_service *service; > - struct btd_profile *profile; > - uint16_t start, end; > - GSList *l, *tmp; > - > - /* > - * Check if the profiles should be probed for the service in the > - * database. > - */ > - for (l = dev->services; l != NULL;) { > - service = l->data; > - profile = btd_service_get_profile(service); > - > - /* If this is local or non-GATT based service, then skip. */ > - if (!profile->remote_uuid || > - !btd_service_get_gatt_handles(service, &start, &end)) { > - l = g_slist_next(l); > - continue; > - } > - > - /* Skip this service if the handle ranges don't overlap. */ > - if (start > data->end_handle || end < data->start_handle) { > - l = g_slist_next(l); > - continue; > - } > - > - /* > - * If there is an exact match, then there's no need to probe the > - * profiles. An exact match is when the service handles AND the > - * service UUID match. > - */ > - if (start == data->start_handle && end == data->end_handle && > - !bt_uuid_strcmp(profile->remote_uuid, data->cur_uuid)) { > - if (data->all_services) > - data->valid_services = g_slist_append( > - data->valid_services, service); > - return true; > - } > - > - /* > - * The handles overlap but there is no exact match. This means > - * that the service is no longer valid. Remove it. > - * > - * TODO: The following code is fairly inefficient, especially > - * when we consider all the extra searches that we're already > - * doing. Consider changing the services list to a GList. > - */ > - tmp = l->next; > - dev->services = g_slist_delete_link(dev->services, l); > - dev->pending = g_slist_remove(dev->pending, service); > - service_remove(service); > - > - l = tmp; > - } > - > - /* No match found */ > - return false; > } > > static void dev_probe_gatt_profile(struct gatt_db_attribute *attr, > @@ -2562,22 +2490,27 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr, > bt_uuid_t uuid; > GSList *l = NULL; > > - gatt_db_attribute_get_service_data(attr, &data->start_handle, > - &data->end_handle, NULL, > - &uuid); > + gatt_db_attribute_get_service_uuid(attr, &uuid); > bt_uuid_to_string(&uuid, data->cur_uuid, sizeof(data->cur_uuid)); > > data->cur_attr = attr; > > - /* Don't probe the profiles if a matching service already exists. */ > - if (!match_existing_service(data)) > - btd_profile_foreach(dev_probe_gatt, data); > - > - if (data->all_services) { > + /* > + * If we're probing for all services, store the UUID since device->uuids > + * was cleared. > + */ > + if (data->all_services) > data->uuids = g_slist_append(data->uuids, > g_strdup(data->cur_uuid)); > + > + /* Don't probe the profiles if a matching service already exists. */ > + if (find_service_with_uuid(data->dev->services, data->cur_uuid)) > + return; > + > + btd_profile_foreach(dev_probe_gatt, data); > + > + if (data->all_services) > return; > - } > > l = g_slist_append(l, g_strdup(data->cur_uuid)); > device_add_uuids(data->dev, l); > @@ -2600,12 +2533,15 @@ static void remove_invalid_services(struct gatt_probe_data *data) > { > struct btd_device *dev = data->dev; > struct btd_service *service; > + struct btd_profile *profile; > GSList *l, *tmp; > > for (l = dev->services; l != NULL;) { > service = l->data; > + profile = btd_service_get_profile(service); > > - if (g_slist_find(data->valid_services, service)) { > + if (g_slist_find_custom(dev->uuids, profile->remote_uuid, > + bt_uuid_strcmp)) { > l = g_slist_next(l); > continue; > } > @@ -2639,11 +2575,15 @@ static void device_probe_gatt_profiles(struct btd_device *device) > > gatt_db_foreach_service(device->db, NULL, dev_probe_gatt_profile, > &data); > + > + /* Clear the UUIDs list */ > + g_slist_free_full(device->uuids, g_free); > + device->uuids = NULL; > + This would actually cause duplicated UUIDs to be emitted, this probably won't work properly for dual-mode devices either since the whole list is cleared, also it is probably better to use a list to track the removed ones so you don't need to do a double lookup at the end, to do that copy the uuids list at the start and remove them once they are found (we might be better off with a different UUID list for Gatt based profiles in this case) which mean what remains at the must have been removed. > device_add_uuids(device, data.uuids); > g_slist_free_full(data.uuids, g_free); > > remove_invalid_services(&data); > - g_slist_free(data.valid_services); > } > > static void device_accept_gatt_profiles(struct btd_device *device) > @@ -2657,13 +2597,15 @@ static void device_accept_gatt_profiles(struct btd_device *device) > static void device_remove_gatt_profile(struct btd_device *device, > struct gatt_db_attribute *attr) > { > - uint16_t start, end; > struct btd_service *service; > + bt_uuid_t uuid; > + char uuid_str[MAX_LEN_UUID_STR]; > GSList *l; > > - gatt_db_attribute_get_service_handles(attr, &start, &end); > + gatt_db_attribute_get_service_uuid(attr, &uuid); > + bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str)); > > - l = find_service_with_gatt_handles(device->services, start, end); > + l = find_service_with_uuid(device->services, uuid_str); > if (!l) > return; > > @@ -2677,13 +2619,16 @@ static void gatt_service_added(struct gatt_db_attribute *attr, void *user_data) > { > struct btd_device *device = user_data; > GSList *new_service = NULL; > + bt_uuid_t uuid; > + char uuid_str[MAX_LEN_UUID_STR]; > uint16_t start, end; > GSList *l; > > if (!bt_gatt_client_is_ready(device->client)) > return; > > - gatt_db_attribute_get_service_handles(attr, &start, &end); > + gatt_db_attribute_get_service_data(attr, &start, &end, NULL, &uuid); > + bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str)); > > DBG("start: 0x%04x, end: 0x%04x", start, end); > > @@ -2695,14 +2640,19 @@ static void gatt_service_added(struct gatt_db_attribute *attr, void *user_data) > if (!new_service) > return; > > - device_register_primaries(device, new_service, -1); > + l = find_service_with_uuid(device->services, uuid_str); > > - device_probe_gatt_profile(device, attr); > + device_register_primaries(device, new_service, -1); > > - l = find_service_with_gatt_handles(device->services, start, end); > - if (l) > + /* > + * If the profile was probed for the first time then call accept on > + * the service. > + */ > + if (!l && (l = find_service_with_uuid(device->services, uuid_str))) > service_accept(l->data); > > + device_probe_gatt_profile(device, attr); > + > store_device_info(device); > > btd_gatt_client_service_added(device->client_dbus, attr); > @@ -2750,24 +2700,26 @@ static void gatt_service_removed(struct gatt_db_attribute *attr, > prim = l->data; > device->primaries = g_slist_delete_link(device->primaries, l); > > - /* > - * If this happend since the db was cleared for a non-bonded device, > - * then don't remove the btd_service just yet. We do this so that we can > - * avoid re-probing the profile if the same GATT service is found on the > - * device on re-connection. However, if the device is marked as > - * temporary, then we remove it anyway. > - */ > - if (device->client || device->temporary == TRUE) > - device_remove_gatt_profile(device, attr); > + /* Get the UUID entry to be removed below */ > + l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp); > > /* > - * Remove the corresponding UUIDs entry, only if this is the last > - * service with this UUID. > + * Remove the corresponding UUIDs entry and the profile, only if this > + * is the last service with this UUID. > */ > - l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp); > - > if (!g_slist_find_custom(device->primaries, prim->uuid, > prim_uuid_cmp)) { > + /* > + * If this happened since the db was cleared for a non-bonded > + * device, then don't remove the btd_service just yet. We do > + * this so that we can avoid re-probing the profile if the same > + * GATT service is found on the device on re-connection. > + * However, if the device is marked as temporary, then we remove > + * it anyway. > + */ > + if (device->client || device->temporary == TRUE) > + device_remove_gatt_profile(device, attr); > + > g_free(l->data); > device->uuids = g_slist_delete_link(device->uuids, l); > g_dbus_emit_property_changed(dbus_conn, device->path, > -- > 2.2.0.rc0.207.ga3a616c > > -- > 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