* [Bluez PATCH 0/2] Fix descriptor display issue in btmon @ 2024-04-03 11:05 Howard Chung 2024-04-03 11:05 ` [Bluez PATCH 1/2] shared/gatt: Rename some gatt insert functions to append Howard Chung 2024-04-03 11:05 ` [Bluez PATCH 2/2] shared/gatt: Add descriptor insert function Howard Chung 0 siblings, 2 replies; 9+ messages in thread From: Howard Chung @ 2024-04-03 11:05 UTC (permalink / raw) To: linux-bluetooth, Luiz Augusto von Dentz, Johan Hedberg, Marcel Holtmann Cc: CrosBT Upstreaming, Howard Chung Hi Maintainers, we found that in some cases the new feature regarding displaying the characterisitc of the descriptor could be wrong. It turns out it is because there is an assumption (please see the 2nd commit) in the gatt-db usage but not being properly maintained in monitor/att.c. These patches are aimming to fix the issue. I'd be happy to send you the btsnoop log but not sure what's the best way for you to send it to you. Thanks. Howard Chung (2): shared/gatt: Rename some gatt insert functions to append shared/gatt: Add descriptor insert function monitor/att.c | 2 +- src/gatt-database.c | 4 +- src/settings.c | 4 +- src/shared/gatt-client.c | 8 +-- src/shared/gatt-db.c | 102 ++++++++++++++++++++++++++++++++------- src/shared/gatt-db.h | 19 ++++++-- unit/test-gatt.c | 4 +- 7 files changed, 109 insertions(+), 34 deletions(-) -- 2.44.0.478.gd926399ef9-goog ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Bluez PATCH 1/2] shared/gatt: Rename some gatt insert functions to append 2024-04-03 11:05 [Bluez PATCH 0/2] Fix descriptor display issue in btmon Howard Chung @ 2024-04-03 11:05 ` Howard Chung 2024-04-03 12:59 ` Fix descriptor display issue in btmon bluez.test.bot 2024-04-03 14:29 ` [Bluez PATCH 1/2] shared/gatt: Rename some gatt insert functions to append Luiz Augusto von Dentz 2024-04-03 11:05 ` [Bluez PATCH 2/2] shared/gatt: Add descriptor insert function Howard Chung 1 sibling, 2 replies; 9+ messages in thread From: Howard Chung @ 2024-04-03 11:05 UTC (permalink / raw) To: linux-bluetooth, Luiz Augusto von Dentz, Johan Hedberg, Marcel Holtmann Cc: CrosBT Upstreaming, Howard Chung, Archie Pusaka Many of the Gatt insert functions are actually append. They append an attribute to the end of the service attributes list. This simply rename these functions. Reviewed-by: Archie Pusaka <apusaka@chromium.org> --- monitor/att.c | 4 ++-- src/gatt-database.c | 4 ++-- src/settings.c | 4 ++-- src/shared/gatt-client.c | 8 ++++---- src/shared/gatt-db.c | 38 +++++++++++++++++++------------------- src/shared/gatt-db.h | 12 ++++++------ unit/test-gatt.c | 4 ++-- 7 files changed, 37 insertions(+), 37 deletions(-) diff --git a/monitor/att.c b/monitor/att.c index 3e5d7f12d..ddeb54d9e 100644 --- a/monitor/att.c +++ b/monitor/att.c @@ -515,7 +515,7 @@ static struct gatt_db_attribute *insert_chrc(const struct l2cap_frame *frame, if (!db) return NULL; - return gatt_db_insert_characteristic(db, handle, uuid, 0, prop, NULL, + return gatt_db_append_characteristic(db, handle, uuid, 0, prop, NULL, NULL, NULL); } @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame, if (!db) return NULL; - return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); + return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); } static void att_find_info_rsp_16(const struct l2cap_frame *frame) diff --git a/src/gatt-database.c b/src/gatt-database.c index 7221ffc87..4c3554211 100644 --- a/src/gatt-database.c +++ b/src/gatt-database.c @@ -3178,7 +3178,7 @@ static bool database_add_desc(struct external_service *service, return false; } - desc->attrib = gatt_db_service_insert_descriptor(service->attrib, + desc->attrib = gatt_db_service_append_descriptor(service->attrib, handle, &uuid, desc->perm, desc_read_cb, @@ -3351,7 +3351,7 @@ static bool database_add_chrc(struct external_service *service, return false; } - chrc->attrib = gatt_db_service_insert_characteristic(service->attrib, + chrc->attrib = gatt_db_service_append_characteristic(service->attrib, handle, &uuid, chrc->perm, chrc->props, chrc_read_cb, chrc_write_cb, chrc); diff --git a/src/settings.c b/src/settings.c index 85534f2c7..025c75b62 100644 --- a/src/settings.c +++ b/src/settings.c @@ -78,7 +78,7 @@ static int load_desc(struct gatt_db *db, char *handle, char *value, if (!bt_uuid_cmp(&uuid, &ext_uuid) && !val) return -EIO; - att = gatt_db_service_insert_descriptor(service, handle_int, &uuid, + att = gatt_db_service_append_descriptor(service, handle_int, &uuid, 0, NULL, NULL, NULL); if (!att || gatt_db_attribute_get_handle(att) != handle_int) return -EIO; @@ -125,7 +125,7 @@ static int load_chrc(struct gatt_db *db, char *handle, char *value, handle_int, value_handle, properties, val_len ? val_str : "", uuid_str); - att = gatt_db_service_insert_characteristic(service, value_handle, + att = gatt_db_service_append_characteristic(service, value_handle, &uuid, 0, properties, NULL, NULL, NULL); if (!att || gatt_db_attribute_get_handle(att) != value_handle) diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c index 6340bcd85..eddbd1778 100644 --- a/src/shared/gatt-client.c +++ b/src/shared/gatt-client.c @@ -638,7 +638,7 @@ static void discover_incl_cb(bool success, uint8_t att_ecode, continue; } - attr = gatt_db_insert_included(client->db, handle, attr); + attr = gatt_db_append_included(client->db, handle, attr); if (!attr) { DBG(client, "Unable to add include attribute at 0x%04x", @@ -734,7 +734,7 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) op->cur_svc = svc; } - attr = gatt_db_insert_characteristic(client->db, + attr = gatt_db_append_characteristic(client->db, chrc_data->value_handle, &chrc_data->uuid, 0, chrc_data->properties, @@ -788,7 +788,7 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) */ bt_uuid16_create(&ccc_uuid, GATT_CLIENT_CHARAC_CFG_UUID); - attr = gatt_db_insert_descriptor(client->db, desc_start, + attr = gatt_db_append_descriptor(client->db, desc_start, &ccc_uuid, 0, NULL, NULL, NULL); if (attr) { @@ -952,7 +952,7 @@ static void discover_descs_cb(bool success, uint8_t att_ecode, DBG(client, "handle: 0x%04x, uuid: %s", handle, uuid_str); - attr = gatt_db_insert_descriptor(client->db, handle, + attr = gatt_db_append_descriptor(client->db, handle, &uuid, 0, NULL, NULL, NULL); if (!attr) { diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index 9559583d1..39bba9b54 100644 --- a/src/shared/gatt-db.c +++ b/src/shared/gatt-db.c @@ -845,7 +845,7 @@ static uint16_t get_handle_at_index(struct gatt_db_service *service, } static struct gatt_db_attribute * -service_insert_characteristic(struct gatt_db_service *service, +service_append_characteristic(struct gatt_db_service *service, uint16_t handle, const bt_uuid_t *uuid, uint32_t permissions, @@ -912,7 +912,7 @@ service_insert_characteristic(struct gatt_db_service *service, } struct gatt_db_attribute * -gatt_db_insert_characteristic(struct gatt_db *db, +gatt_db_append_characteristic(struct gatt_db *db, uint16_t handle, const bt_uuid_t *uuid, uint32_t permissions, @@ -927,14 +927,14 @@ gatt_db_insert_characteristic(struct gatt_db *db, if (!attrib) return NULL; - return service_insert_characteristic(attrib->service, handle, uuid, + return service_append_characteristic(attrib->service, handle, uuid, permissions, properties, read_func, write_func, user_data); } struct gatt_db_attribute * -gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib, +gatt_db_service_append_characteristic(struct gatt_db_attribute *attrib, uint16_t handle, const bt_uuid_t *uuid, uint32_t permissions, @@ -946,7 +946,7 @@ gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib, if (!attrib) return NULL; - return service_insert_characteristic(attrib->service, handle, uuid, + return service_append_characteristic(attrib->service, handle, uuid, permissions, properties, read_func, write_func, user_data); @@ -964,14 +964,14 @@ gatt_db_service_add_characteristic(struct gatt_db_attribute *attrib, if (!attrib) return NULL; - return service_insert_characteristic(attrib->service, 0, uuid, + return service_append_characteristic(attrib->service, 0, uuid, permissions, properties, read_func, write_func, user_data); } static struct gatt_db_attribute * -service_insert_descriptor(struct gatt_db_service *service, +service_append_descriptor(struct gatt_db_service *service, uint16_t handle, const bt_uuid_t *uuid, uint32_t permissions, @@ -1003,7 +1003,7 @@ service_insert_descriptor(struct gatt_db_service *service, } struct gatt_db_attribute * -gatt_db_insert_descriptor(struct gatt_db *db, +gatt_db_append_descriptor(struct gatt_db *db, uint16_t handle, const bt_uuid_t *uuid, uint32_t permissions, @@ -1017,13 +1017,13 @@ gatt_db_insert_descriptor(struct gatt_db *db, if (!attrib) return NULL; - return service_insert_descriptor(attrib->service, handle, uuid, + return service_append_descriptor(attrib->service, handle, uuid, permissions, read_func, write_func, user_data); } struct gatt_db_attribute * -gatt_db_service_insert_descriptor(struct gatt_db_attribute *attrib, +gatt_db_service_append_descriptor(struct gatt_db_attribute *attrib, uint16_t handle, const bt_uuid_t *uuid, uint32_t permissions, @@ -1034,7 +1034,7 @@ gatt_db_service_insert_descriptor(struct gatt_db_attribute *attrib, if (!attrib) return NULL; - return service_insert_descriptor(attrib->service, handle, uuid, + return service_append_descriptor(attrib->service, handle, uuid, permissions, read_func, write_func, user_data); } @@ -1050,7 +1050,7 @@ gatt_db_service_add_descriptor(struct gatt_db_attribute *attrib, if (!attrib) return NULL; - return service_insert_descriptor(attrib->service, 0, uuid, + return service_append_descriptor(attrib->service, 0, uuid, permissions, read_func, write_func, user_data); } @@ -1088,7 +1088,7 @@ gatt_db_service_add_ccc(struct gatt_db_attribute *attrib, uint32_t permissions) if (!value || value->notify_func) return NULL; - ccc = service_insert_descriptor(attrib->service, 0, &ccc_uuid, + ccc = service_append_descriptor(attrib->service, 0, &ccc_uuid, permissions, db->ccc->read_func, db->ccc->write_func, @@ -1121,7 +1121,7 @@ void gatt_db_ccc_register(struct gatt_db *db, gatt_db_read_t read_func, } static struct gatt_db_attribute * -service_insert_included(struct gatt_db_service *service, uint16_t handle, +service_append_included(struct gatt_db_service *service, uint16_t handle, struct gatt_db_attribute *include) { struct gatt_db_service *included; @@ -1186,22 +1186,22 @@ gatt_db_service_add_included(struct gatt_db_attribute *attrib, if (!attrib || !include) return NULL; - return service_insert_included(attrib->service, 0, include); + return service_append_included(attrib->service, 0, include); } struct gatt_db_attribute * -gatt_db_service_insert_included(struct gatt_db_attribute *attrib, +gatt_db_service_append_included(struct gatt_db_attribute *attrib, uint16_t handle, struct gatt_db_attribute *include) { if (!attrib || !handle || !include) return NULL; - return service_insert_included(attrib->service, handle, include); + return service_append_included(attrib->service, handle, include); } struct gatt_db_attribute * -gatt_db_insert_included(struct gatt_db *db, uint16_t handle, +gatt_db_append_included(struct gatt_db *db, uint16_t handle, struct gatt_db_attribute *include) { struct gatt_db_attribute *attrib; @@ -1210,7 +1210,7 @@ gatt_db_insert_included(struct gatt_db *db, uint16_t handle, if (!attrib) return NULL; - return service_insert_included(attrib->service, handle, include); + return service_append_included(attrib->service, handle, include); } bool gatt_db_service_set_active(struct gatt_db_attribute *attrib, bool active) diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h index fb939e40d..ec0eccdfc 100644 --- a/src/shared/gatt-db.h +++ b/src/shared/gatt-db.h @@ -61,7 +61,7 @@ gatt_db_service_add_characteristic(struct gatt_db_attribute *attrib, gatt_db_write_t write_func, void *user_data); struct gatt_db_attribute * -gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib, +gatt_db_service_append_characteristic(struct gatt_db_attribute *attrib, uint16_t handle, const bt_uuid_t *uuid, uint32_t permissions, @@ -71,7 +71,7 @@ gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib, void *user_data); struct gatt_db_attribute * -gatt_db_insert_characteristic(struct gatt_db *db, +gatt_db_append_characteristic(struct gatt_db *db, uint16_t handle, const bt_uuid_t *uuid, uint32_t permissions, @@ -81,7 +81,7 @@ gatt_db_insert_characteristic(struct gatt_db *db, void *user_data); struct gatt_db_attribute * -gatt_db_insert_descriptor(struct gatt_db *db, +gatt_db_append_descriptor(struct gatt_db *db, uint16_t handle, const bt_uuid_t *uuid, uint32_t permissions, @@ -98,7 +98,7 @@ gatt_db_service_add_descriptor(struct gatt_db_attribute *attrib, void *user_data); struct gatt_db_attribute * -gatt_db_service_insert_descriptor(struct gatt_db_attribute *attrib, +gatt_db_service_append_descriptor(struct gatt_db_attribute *attrib, uint16_t handle, const bt_uuid_t *uuid, uint32_t permissions, @@ -110,14 +110,14 @@ struct gatt_db_attribute * gatt_db_service_add_ccc(struct gatt_db_attribute *attrib, uint32_t permissions); struct gatt_db_attribute * -gatt_db_insert_included(struct gatt_db *db, uint16_t handle, +gatt_db_append_included(struct gatt_db *db, uint16_t handle, struct gatt_db_attribute *include); struct gatt_db_attribute * gatt_db_service_add_included(struct gatt_db_attribute *attrib, struct gatt_db_attribute *include); struct gatt_db_attribute * -gatt_db_service_insert_included(struct gatt_db_attribute *attrib, +gatt_db_service_append_included(struct gatt_db_attribute *attrib, uint16_t handle, struct gatt_db_attribute *include); diff --git a/unit/test-gatt.c b/unit/test-gatt.c index 5e06d4ed4..20b25cf8c 100644 --- a/unit/test-gatt.c +++ b/unit/test-gatt.c @@ -1236,7 +1236,7 @@ add_char_with_value(struct gatt_db_attribute *service_att, uint16_t handle, struct gatt_db_attribute *attrib; if (handle) - attrib = gatt_db_service_insert_characteristic(service_att, + attrib = gatt_db_service_append_characteristic(service_att, handle, uuid, att_permissions, char_properties, @@ -1265,7 +1265,7 @@ add_desc_with_value(struct gatt_db_attribute *att, uint16_t handle, struct gatt_db_attribute *desc_att; if (handle) - desc_att = gatt_db_service_insert_descriptor(att, handle, uuid, + desc_att = gatt_db_service_append_descriptor(att, handle, uuid, att_perms, NULL, NULL, NULL); else -- 2.44.0.478.gd926399ef9-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: Fix descriptor display issue in btmon 2024-04-03 11:05 ` [Bluez PATCH 1/2] shared/gatt: Rename some gatt insert functions to append Howard Chung @ 2024-04-03 12:59 ` bluez.test.bot 2024-04-03 14:29 ` [Bluez PATCH 1/2] shared/gatt: Rename some gatt insert functions to append Luiz Augusto von Dentz 1 sibling, 0 replies; 9+ messages in thread From: bluez.test.bot @ 2024-04-03 12:59 UTC (permalink / raw) To: linux-bluetooth, howardchung [-- Attachment #1: Type: text/plain, Size: 1262 bytes --] This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=841052 ---Test result--- Test Summary: CheckPatch PASS 1.11 seconds GitLint PASS 0.77 seconds BuildEll PASS 24.08 seconds BluezMake PASS 1626.86 seconds MakeCheck PASS 12.97 seconds MakeDistcheck PASS 174.35 seconds CheckValgrind PASS 244.01 seconds CheckSmatch WARNING 346.65 seconds bluezmakeextell PASS 117.47 seconds IncrementalBuild PASS 2928.57 seconds ScanBuild PASS 965.46 seconds Details ############################## Test: CheckSmatch - WARNING Desc: Run smatch tool with source Output: monitor/att.c: note: in included file:monitor/display.h:82:26: warning: Variable length array is used.monitor/att.c: note: in included file:monitor/display.h:82:26: warning: Variable length array is used. --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bluez PATCH 1/2] shared/gatt: Rename some gatt insert functions to append 2024-04-03 11:05 ` [Bluez PATCH 1/2] shared/gatt: Rename some gatt insert functions to append Howard Chung 2024-04-03 12:59 ` Fix descriptor display issue in btmon bluez.test.bot @ 2024-04-03 14:29 ` Luiz Augusto von Dentz 1 sibling, 0 replies; 9+ messages in thread From: Luiz Augusto von Dentz @ 2024-04-03 14:29 UTC (permalink / raw) To: Howard Chung Cc: linux-bluetooth, Johan Hedberg, Marcel Holtmann, CrosBT Upstreaming, Archie Pusaka Hi Howard, On Wed, Apr 3, 2024 at 7:06 AM Howard Chung <howardchung@google.com> wrote: > > Many of the Gatt insert functions are actually append. They append an > attribute to the end of the service attributes list. > This simply rename these functions. Those are 2 different operations, insert takes a handle and attempts to insert it at the handle position while append you just look into the next available handle. > Reviewed-by: Archie Pusaka <apusaka@chromium.org> > --- > > monitor/att.c | 4 ++-- > src/gatt-database.c | 4 ++-- > src/settings.c | 4 ++-- > src/shared/gatt-client.c | 8 ++++---- > src/shared/gatt-db.c | 38 +++++++++++++++++++------------------- > src/shared/gatt-db.h | 12 ++++++------ > unit/test-gatt.c | 4 ++-- > 7 files changed, 37 insertions(+), 37 deletions(-) > > diff --git a/monitor/att.c b/monitor/att.c > index 3e5d7f12d..ddeb54d9e 100644 > --- a/monitor/att.c > +++ b/monitor/att.c > @@ -515,7 +515,7 @@ static struct gatt_db_attribute *insert_chrc(const struct l2cap_frame *frame, > if (!db) > return NULL; > > - return gatt_db_insert_characteristic(db, handle, uuid, 0, prop, NULL, > + return gatt_db_append_characteristic(db, handle, uuid, 0, prop, NULL, > NULL, NULL); > } > > @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame, > if (!db) > return NULL; > > - return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); > + return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); > } > > static void att_find_info_rsp_16(const struct l2cap_frame *frame) > diff --git a/src/gatt-database.c b/src/gatt-database.c > index 7221ffc87..4c3554211 100644 > --- a/src/gatt-database.c > +++ b/src/gatt-database.c > @@ -3178,7 +3178,7 @@ static bool database_add_desc(struct external_service *service, > return false; > } > > - desc->attrib = gatt_db_service_insert_descriptor(service->attrib, > + desc->attrib = gatt_db_service_append_descriptor(service->attrib, > handle, &uuid, > desc->perm, > desc_read_cb, > @@ -3351,7 +3351,7 @@ static bool database_add_chrc(struct external_service *service, > return false; > } > > - chrc->attrib = gatt_db_service_insert_characteristic(service->attrib, > + chrc->attrib = gatt_db_service_append_characteristic(service->attrib, > handle, &uuid, chrc->perm, > chrc->props, chrc_read_cb, > chrc_write_cb, chrc); > diff --git a/src/settings.c b/src/settings.c > index 85534f2c7..025c75b62 100644 > --- a/src/settings.c > +++ b/src/settings.c > @@ -78,7 +78,7 @@ static int load_desc(struct gatt_db *db, char *handle, char *value, > if (!bt_uuid_cmp(&uuid, &ext_uuid) && !val) > return -EIO; > > - att = gatt_db_service_insert_descriptor(service, handle_int, &uuid, > + att = gatt_db_service_append_descriptor(service, handle_int, &uuid, > 0, NULL, NULL, NULL); > if (!att || gatt_db_attribute_get_handle(att) != handle_int) > return -EIO; > @@ -125,7 +125,7 @@ static int load_chrc(struct gatt_db *db, char *handle, char *value, > handle_int, value_handle, > properties, val_len ? val_str : "", uuid_str); > > - att = gatt_db_service_insert_characteristic(service, value_handle, > + att = gatt_db_service_append_characteristic(service, value_handle, > &uuid, 0, properties, > NULL, NULL, NULL); > if (!att || gatt_db_attribute_get_handle(att) != value_handle) > diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c > index 6340bcd85..eddbd1778 100644 > --- a/src/shared/gatt-client.c > +++ b/src/shared/gatt-client.c > @@ -638,7 +638,7 @@ static void discover_incl_cb(bool success, uint8_t att_ecode, > continue; > } > > - attr = gatt_db_insert_included(client->db, handle, attr); > + attr = gatt_db_append_included(client->db, handle, attr); > if (!attr) { > DBG(client, > "Unable to add include attribute at 0x%04x", > @@ -734,7 +734,7 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) > op->cur_svc = svc; > } > > - attr = gatt_db_insert_characteristic(client->db, > + attr = gatt_db_append_characteristic(client->db, > chrc_data->value_handle, > &chrc_data->uuid, 0, > chrc_data->properties, > @@ -788,7 +788,7 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) > */ > bt_uuid16_create(&ccc_uuid, > GATT_CLIENT_CHARAC_CFG_UUID); > - attr = gatt_db_insert_descriptor(client->db, desc_start, > + attr = gatt_db_append_descriptor(client->db, desc_start, > &ccc_uuid, 0, NULL, > NULL, NULL); > if (attr) { > @@ -952,7 +952,7 @@ static void discover_descs_cb(bool success, uint8_t att_ecode, > > DBG(client, "handle: 0x%04x, uuid: %s", handle, uuid_str); > > - attr = gatt_db_insert_descriptor(client->db, handle, > + attr = gatt_db_append_descriptor(client->db, handle, > &uuid, 0, NULL, NULL, > NULL); > if (!attr) { > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > index 9559583d1..39bba9b54 100644 > --- a/src/shared/gatt-db.c > +++ b/src/shared/gatt-db.c > @@ -845,7 +845,7 @@ static uint16_t get_handle_at_index(struct gatt_db_service *service, > } > > static struct gatt_db_attribute * > -service_insert_characteristic(struct gatt_db_service *service, > +service_append_characteristic(struct gatt_db_service *service, > uint16_t handle, > const bt_uuid_t *uuid, > uint32_t permissions, > @@ -912,7 +912,7 @@ service_insert_characteristic(struct gatt_db_service *service, > } > > struct gatt_db_attribute * > -gatt_db_insert_characteristic(struct gatt_db *db, > +gatt_db_append_characteristic(struct gatt_db *db, > uint16_t handle, > const bt_uuid_t *uuid, > uint32_t permissions, > @@ -927,14 +927,14 @@ gatt_db_insert_characteristic(struct gatt_db *db, > if (!attrib) > return NULL; > > - return service_insert_characteristic(attrib->service, handle, uuid, > + return service_append_characteristic(attrib->service, handle, uuid, > permissions, properties, > read_func, write_func, > user_data); > } > > struct gatt_db_attribute * > -gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib, > +gatt_db_service_append_characteristic(struct gatt_db_attribute *attrib, > uint16_t handle, > const bt_uuid_t *uuid, > uint32_t permissions, > @@ -946,7 +946,7 @@ gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib, > if (!attrib) > return NULL; > > - return service_insert_characteristic(attrib->service, handle, uuid, > + return service_append_characteristic(attrib->service, handle, uuid, > permissions, properties, > read_func, write_func, > user_data); > @@ -964,14 +964,14 @@ gatt_db_service_add_characteristic(struct gatt_db_attribute *attrib, > if (!attrib) > return NULL; > > - return service_insert_characteristic(attrib->service, 0, uuid, > + return service_append_characteristic(attrib->service, 0, uuid, > permissions, properties, > read_func, write_func, > user_data); > } > > static struct gatt_db_attribute * > -service_insert_descriptor(struct gatt_db_service *service, > +service_append_descriptor(struct gatt_db_service *service, > uint16_t handle, > const bt_uuid_t *uuid, > uint32_t permissions, > @@ -1003,7 +1003,7 @@ service_insert_descriptor(struct gatt_db_service *service, > } > > struct gatt_db_attribute * > -gatt_db_insert_descriptor(struct gatt_db *db, > +gatt_db_append_descriptor(struct gatt_db *db, > uint16_t handle, > const bt_uuid_t *uuid, > uint32_t permissions, > @@ -1017,13 +1017,13 @@ gatt_db_insert_descriptor(struct gatt_db *db, > if (!attrib) > return NULL; > > - return service_insert_descriptor(attrib->service, handle, uuid, > + return service_append_descriptor(attrib->service, handle, uuid, > permissions, read_func, write_func, > user_data); > } > > struct gatt_db_attribute * > -gatt_db_service_insert_descriptor(struct gatt_db_attribute *attrib, > +gatt_db_service_append_descriptor(struct gatt_db_attribute *attrib, > uint16_t handle, > const bt_uuid_t *uuid, > uint32_t permissions, > @@ -1034,7 +1034,7 @@ gatt_db_service_insert_descriptor(struct gatt_db_attribute *attrib, > if (!attrib) > return NULL; > > - return service_insert_descriptor(attrib->service, handle, uuid, > + return service_append_descriptor(attrib->service, handle, uuid, > permissions, read_func, write_func, > user_data); > } > @@ -1050,7 +1050,7 @@ gatt_db_service_add_descriptor(struct gatt_db_attribute *attrib, > if (!attrib) > return NULL; > > - return service_insert_descriptor(attrib->service, 0, uuid, > + return service_append_descriptor(attrib->service, 0, uuid, > permissions, read_func, write_func, > user_data); > } > @@ -1088,7 +1088,7 @@ gatt_db_service_add_ccc(struct gatt_db_attribute *attrib, uint32_t permissions) > if (!value || value->notify_func) > return NULL; > > - ccc = service_insert_descriptor(attrib->service, 0, &ccc_uuid, > + ccc = service_append_descriptor(attrib->service, 0, &ccc_uuid, > permissions, > db->ccc->read_func, > db->ccc->write_func, > @@ -1121,7 +1121,7 @@ void gatt_db_ccc_register(struct gatt_db *db, gatt_db_read_t read_func, > } > > static struct gatt_db_attribute * > -service_insert_included(struct gatt_db_service *service, uint16_t handle, > +service_append_included(struct gatt_db_service *service, uint16_t handle, > struct gatt_db_attribute *include) > { > struct gatt_db_service *included; > @@ -1186,22 +1186,22 @@ gatt_db_service_add_included(struct gatt_db_attribute *attrib, > if (!attrib || !include) > return NULL; > > - return service_insert_included(attrib->service, 0, include); > + return service_append_included(attrib->service, 0, include); > } > > struct gatt_db_attribute * > -gatt_db_service_insert_included(struct gatt_db_attribute *attrib, > +gatt_db_service_append_included(struct gatt_db_attribute *attrib, > uint16_t handle, > struct gatt_db_attribute *include) > { > if (!attrib || !handle || !include) > return NULL; > > - return service_insert_included(attrib->service, handle, include); > + return service_append_included(attrib->service, handle, include); > } > > struct gatt_db_attribute * > -gatt_db_insert_included(struct gatt_db *db, uint16_t handle, > +gatt_db_append_included(struct gatt_db *db, uint16_t handle, > struct gatt_db_attribute *include) > { > struct gatt_db_attribute *attrib; > @@ -1210,7 +1210,7 @@ gatt_db_insert_included(struct gatt_db *db, uint16_t handle, > if (!attrib) > return NULL; > > - return service_insert_included(attrib->service, handle, include); > + return service_append_included(attrib->service, handle, include); > } > > bool gatt_db_service_set_active(struct gatt_db_attribute *attrib, bool active) > diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h > index fb939e40d..ec0eccdfc 100644 > --- a/src/shared/gatt-db.h > +++ b/src/shared/gatt-db.h > @@ -61,7 +61,7 @@ gatt_db_service_add_characteristic(struct gatt_db_attribute *attrib, > gatt_db_write_t write_func, > void *user_data); > struct gatt_db_attribute * > -gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib, > +gatt_db_service_append_characteristic(struct gatt_db_attribute *attrib, > uint16_t handle, > const bt_uuid_t *uuid, > uint32_t permissions, > @@ -71,7 +71,7 @@ gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib, > void *user_data); > > struct gatt_db_attribute * > -gatt_db_insert_characteristic(struct gatt_db *db, > +gatt_db_append_characteristic(struct gatt_db *db, > uint16_t handle, > const bt_uuid_t *uuid, > uint32_t permissions, > @@ -81,7 +81,7 @@ gatt_db_insert_characteristic(struct gatt_db *db, > void *user_data); > > struct gatt_db_attribute * > -gatt_db_insert_descriptor(struct gatt_db *db, > +gatt_db_append_descriptor(struct gatt_db *db, > uint16_t handle, > const bt_uuid_t *uuid, > uint32_t permissions, > @@ -98,7 +98,7 @@ gatt_db_service_add_descriptor(struct gatt_db_attribute *attrib, > void *user_data); > > struct gatt_db_attribute * > -gatt_db_service_insert_descriptor(struct gatt_db_attribute *attrib, > +gatt_db_service_append_descriptor(struct gatt_db_attribute *attrib, > uint16_t handle, > const bt_uuid_t *uuid, > uint32_t permissions, > @@ -110,14 +110,14 @@ struct gatt_db_attribute * > gatt_db_service_add_ccc(struct gatt_db_attribute *attrib, uint32_t permissions); > > struct gatt_db_attribute * > -gatt_db_insert_included(struct gatt_db *db, uint16_t handle, > +gatt_db_append_included(struct gatt_db *db, uint16_t handle, > struct gatt_db_attribute *include); > > struct gatt_db_attribute * > gatt_db_service_add_included(struct gatt_db_attribute *attrib, > struct gatt_db_attribute *include); > struct gatt_db_attribute * > -gatt_db_service_insert_included(struct gatt_db_attribute *attrib, > +gatt_db_service_append_included(struct gatt_db_attribute *attrib, > uint16_t handle, > struct gatt_db_attribute *include); > > diff --git a/unit/test-gatt.c b/unit/test-gatt.c > index 5e06d4ed4..20b25cf8c 100644 > --- a/unit/test-gatt.c > +++ b/unit/test-gatt.c > @@ -1236,7 +1236,7 @@ add_char_with_value(struct gatt_db_attribute *service_att, uint16_t handle, > struct gatt_db_attribute *attrib; > > if (handle) > - attrib = gatt_db_service_insert_characteristic(service_att, > + attrib = gatt_db_service_append_characteristic(service_att, > handle, uuid, > att_permissions, > char_properties, > @@ -1265,7 +1265,7 @@ add_desc_with_value(struct gatt_db_attribute *att, uint16_t handle, > struct gatt_db_attribute *desc_att; > > if (handle) > - desc_att = gatt_db_service_insert_descriptor(att, handle, uuid, > + desc_att = gatt_db_service_append_descriptor(att, handle, uuid, > att_perms, NULL, NULL, > NULL); > else > -- > 2.44.0.478.gd926399ef9-goog > -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Bluez PATCH 2/2] shared/gatt: Add descriptor insert function 2024-04-03 11:05 [Bluez PATCH 0/2] Fix descriptor display issue in btmon Howard Chung 2024-04-03 11:05 ` [Bluez PATCH 1/2] shared/gatt: Rename some gatt insert functions to append Howard Chung @ 2024-04-03 11:05 ` Howard Chung 2024-04-03 14:36 ` Luiz Augusto von Dentz 1 sibling, 1 reply; 9+ messages in thread From: Howard Chung @ 2024-04-03 11:05 UTC (permalink / raw) To: linux-bluetooth, Luiz Augusto von Dentz, Johan Hedberg, Marcel Holtmann Cc: CrosBT Upstreaming, Howard Chung, Archie Pusaka service->attributes has an assumption that it should look like: |char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|... where desc_x_y means a descriptor of char_x. In monitor, an attribute is inserted as soon as it is found. It is not guranteed that all the descriptors of a characteristic would be found before another characteristic is found. This adds a function to insert the descriptor with the given handle to an appropriate place in its service attribute list and make monitor to use this function when a descripter is found. Reviewed-by: Archie Pusaka <apusaka@chromium.org> --- monitor/att.c | 2 +- src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++ src/shared/gatt-db.h | 9 ++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/monitor/att.c b/monitor/att.c index ddeb54d9e..503099745 100644 --- a/monitor/att.c +++ b/monitor/att.c @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame, if (!db) return NULL; - return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); + return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); } static void att_find_info_rsp_16(const struct l2cap_frame *frame) diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index 39bba9b54..f08c5afaa 100644 --- a/src/shared/gatt-db.c +++ b/src/shared/gatt-db.c @@ -1002,6 +1002,72 @@ service_append_descriptor(struct gatt_db_service *service, return service->attributes[i]; } +struct gatt_db_attribute * +gatt_db_insert_descriptor(struct gatt_db *db, + uint16_t handle, + const bt_uuid_t *uuid, + uint32_t permissions, + gatt_db_read_t read_func, + gatt_db_write_t write_func, + void *user_data) +{ + struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL; + struct gatt_db_service *service; + int i, j, num_index, char_index; + + attrib = gatt_db_get_service(db, handle); + if (!attrib) + return NULL; + + service = attrib->service; + num_index = get_attribute_index(service, 0); + if (!num_index) + return NULL; + + // Find the characteristic the descriptor belongs to. + for (i = 0; i < num_index; i++) { + iter_attr = service->attributes[i]; + if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid)) + continue; + + if (iter_attr->handle > handle) + continue; + + // Find the characteristic with the largest handle among those + // with handles less than the descriptor's handle. + if (!char_attr || iter_attr->handle > char_attr->handle) { + char_attr = iter_attr; + char_index = i; + } + } + + // There is no characteristic contain this descriptor. Something went + // wrong + if (!char_attr) + return NULL; + + // Find the end of this characteristic + for (i = char_index + 1; i < num_index; i++) { + iter_attr = service->attributes[i]; + + if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) || + !bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid)) + break; + } + + // Move all of the attributes after the end of this characteristic to + // its next index. + for (j = num_index; j > i; j--) + service->attributes[j] = service->attributes[j - 1]; + + service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0); + + set_attribute_data(service->attributes[i], read_func, write_func, + permissions, user_data); + + return service->attributes[i]; +} + struct gatt_db_attribute * gatt_db_append_descriptor(struct gatt_db *db, uint16_t handle, diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h index ec0eccdfc..4c4e88d87 100644 --- a/src/shared/gatt-db.h +++ b/src/shared/gatt-db.h @@ -80,6 +80,15 @@ gatt_db_append_characteristic(struct gatt_db *db, gatt_db_write_t write_func, void *user_data); +struct gatt_db_attribute * +gatt_db_insert_descriptor(struct gatt_db *db, + uint16_t handle, + const bt_uuid_t *uuid, + uint32_t permissions, + gatt_db_read_t read_func, + gatt_db_write_t write_func, + void *user_data); + struct gatt_db_attribute * gatt_db_append_descriptor(struct gatt_db *db, uint16_t handle, -- 2.44.0.478.gd926399ef9-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Bluez PATCH 2/2] shared/gatt: Add descriptor insert function 2024-04-03 11:05 ` [Bluez PATCH 2/2] shared/gatt: Add descriptor insert function Howard Chung @ 2024-04-03 14:36 ` Luiz Augusto von Dentz 2024-04-08 12:22 ` Yun-hao Chung 0 siblings, 1 reply; 9+ messages in thread From: Luiz Augusto von Dentz @ 2024-04-03 14:36 UTC (permalink / raw) To: Howard Chung Cc: linux-bluetooth, Johan Hedberg, Marcel Holtmann, CrosBT Upstreaming, Archie Pusaka Hi Howard, On Wed, Apr 3, 2024 at 7:06 AM Howard Chung <howardchung@google.com> wrote: > > service->attributes has an assumption that it should look like: > |char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|... > where desc_x_y means a descriptor of char_x. Will need to check the trace but I believe BlueZ always discover all characteristics before moving to descriptors, if that is not happening they there is probably a bug somewhere, that said perhaps you are doing the discovery procedure with another stack which doesn't employ the same logic, although inefficient it is possible to discover out of order but that would require reassigning the descriptors to characteristics once they are found, which is also inefficient but I would understand if you after supporting other stacks. > In monitor, an attribute is inserted as soon as it is found. It is not > guranteed that all the descriptors of a characteristic would be found > before another characteristic is found. You might want to include such a trace, don't recall seeing any stack that does out-order discover. > This adds a function to insert the descriptor with the given handle to > an appropriate place in its service attribute list and make monitor to > use this function when a descripter is found. > > Reviewed-by: Archie Pusaka <apusaka@chromium.org> > --- > > monitor/att.c | 2 +- > src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++ > src/shared/gatt-db.h | 9 ++++++ > 3 files changed, 76 insertions(+), 1 deletion(-) > > diff --git a/monitor/att.c b/monitor/att.c > index ddeb54d9e..503099745 100644 > --- a/monitor/att.c > +++ b/monitor/att.c > @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame, > if (!db) > return NULL; > > - return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); > + return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); > } > > static void att_find_info_rsp_16(const struct l2cap_frame *frame) > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > index 39bba9b54..f08c5afaa 100644 > --- a/src/shared/gatt-db.c > +++ b/src/shared/gatt-db.c > @@ -1002,6 +1002,72 @@ service_append_descriptor(struct gatt_db_service *service, > return service->attributes[i]; > } > > +struct gatt_db_attribute * > +gatt_db_insert_descriptor(struct gatt_db *db, > + uint16_t handle, > + const bt_uuid_t *uuid, > + uint32_t permissions, > + gatt_db_read_t read_func, > + gatt_db_write_t write_func, > + void *user_data) > +{ > + struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL; > + struct gatt_db_service *service; > + int i, j, num_index, char_index; > + > + attrib = gatt_db_get_service(db, handle); > + if (!attrib) > + return NULL; > + > + service = attrib->service; > + num_index = get_attribute_index(service, 0); > + if (!num_index) > + return NULL; > + > + // Find the characteristic the descriptor belongs to. > + for (i = 0; i < num_index; i++) { > + iter_attr = service->attributes[i]; > + if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid)) > + continue; > + > + if (iter_attr->handle > handle) > + continue; > + > + // Find the characteristic with the largest handle among those > + // with handles less than the descriptor's handle. > + if (!char_attr || iter_attr->handle > char_attr->handle) { > + char_attr = iter_attr; > + char_index = i; > + } > + } > + > + // There is no characteristic contain this descriptor. Something went > + // wrong > + if (!char_attr) > + return NULL; > + > + // Find the end of this characteristic > + for (i = char_index + 1; i < num_index; i++) { > + iter_attr = service->attributes[i]; > + > + if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) || > + !bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid)) > + break; > + } > + > + // Move all of the attributes after the end of this characteristic to > + // its next index. > + for (j = num_index; j > i; j--) > + service->attributes[j] = service->attributes[j - 1]; > + > + service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0); > + > + set_attribute_data(service->attributes[i], read_func, write_func, > + permissions, user_data); > + > + return service->attributes[i]; > +} > + > struct gatt_db_attribute * > gatt_db_append_descriptor(struct gatt_db *db, > uint16_t handle, > diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h > index ec0eccdfc..4c4e88d87 100644 > --- a/src/shared/gatt-db.h > +++ b/src/shared/gatt-db.h > @@ -80,6 +80,15 @@ gatt_db_append_characteristic(struct gatt_db *db, > gatt_db_write_t write_func, > void *user_data); > > +struct gatt_db_attribute * > +gatt_db_insert_descriptor(struct gatt_db *db, > + uint16_t handle, > + const bt_uuid_t *uuid, > + uint32_t permissions, > + gatt_db_read_t read_func, > + gatt_db_write_t write_func, > + void *user_data); > + > struct gatt_db_attribute * > gatt_db_append_descriptor(struct gatt_db *db, > uint16_t handle, > -- > 2.44.0.478.gd926399ef9-goog > -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bluez PATCH 2/2] shared/gatt: Add descriptor insert function 2024-04-03 14:36 ` Luiz Augusto von Dentz @ 2024-04-08 12:22 ` Yun-hao Chung 2024-04-08 14:03 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 9+ messages in thread From: Yun-hao Chung @ 2024-04-08 12:22 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: linux-bluetooth, Johan Hedberg, Marcel Holtmann, CrosBT Upstreaming, Archie Pusaka Hi Luiz, Thanks for the response. This issue was found in ChromeOS edition BlueZ. I'm not sure if any official BlueZ would do the same discovery procedure or not. I extracted relevant part of the trace as below ========================== < ACL Data TX: Handle 2048 flags 0x00 dlen 11 #43961 [hci0] 2024-03-05 07:33:55.936283 ATT: Read By Group Type Request (0x10) len 6 Handle range: 0x001f-0xffff Attribute group type: Primary Service (0x2800) ... > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #43963 [hci0] 2024-03-05 07:33:55.967020 > ACL Data RX: Handle 2048 flags 0x01 dlen 3 #43964 [hci0] 2024-03-05 07:33:55.967022 ATT: Read By Group Type Response (0x11) len 25 Attribute data length: 6 Attribute group list: 4 entries Handle range: 0x001f-0x0035 UUID: Human Interface Device (0x1812) Handle range: 0x0036-0x0044 UUID: Human Interface Device (0x1812) Handle range: 0x0045-0x0055 UUID: Human Interface Device (0x1812) Handle range: 0x0056-0x0062 UUID: Logitech International SA (0xfd72) … < ACL Data TX: Handle 2048 flags 0x00 dlen 11 #44018 [hci0] 2024-03-05 07:33:56.281436 ATT: Read By Type Request (0x08) len 6 Handle range: 0x0021-0xffff Attribute type: Characteristic (0x2803) ... > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #44023 [hci0] 2024-03-05 07:33:56.311026 ATT: Read By Type Response (0x09) len 22 Attribute data length: 7 Attribute data list: 3 entries Handle: 0x0022 Value: 122300332a Properties: 0x12 Read (0x02) Notify (0x10) Value Handle: 0x0023 Value UUID: Boot Mouse Input Report (0x2a33) Handle: 0x0025 Value: 0226004b2a Properties: 0x02 Read (0x02) Value Handle: 0x0026 Value UUID: Report Map (0x2a4b) Handle: 0x0027 Value: 1228004d2a Properties: 0x12 Read (0x02) Notify (0x10) Value Handle: 0x0028 Value UUID: Report (0x2a4d) ... < ACL Data TX: Handle 2048 flags 0x00 dlen 11 #44024 [hci0] 2024-03-05 07:33:56.311458 ATT: Read By Type Request (0x08) len 6 Handle range: 0x0028-0xffff Attribute type: Characteristic (0x2803) … > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #44029 [hci0] 2024-03-05 07:33:56.341025 ATT: Read By Type Response (0x09) len 22 Attribute data length: 7 Attribute data list: 3 entries Handle: 0x002b Value: 122c004d2a Properties: 0x12 Read (0x02) Notify (0x10) Value Handle: 0x002c Value UUID: Report (0x2a4d) Handle: 0x002f Value: 0e30004d2a Properties: 0x0e Read (0x02) Write Without Response (0x04) Write (0x08) Value Handle: 0x0030 Value UUID: Report (0x2a4d) Handle: 0x0032 Value: 0433004c2a Properties: 0x04 Write Without Response (0x04) Value Handle: 0x0033 Value UUID: HID Control Point (0x2a4c) … < ACL Data TX: Handle 2048 flags 0x00 dlen 9 #44071 [hci0] 2024-03-05 07:33:56.702334 ATT: Find Information Request (0x04) len 4 Handle range: 0x0029-0x002a … > ACL Data RX: Handle 2048 flags 0x02 dlen 14 #44073 [hci0] 2024-03-05 07:33:56.731069 ATT: Find Information Response (0x05) len 9 Format: UUID-16 (0x01) Handle: 0x0029 UUID: Client Characteristic Configuration (0x2902) Handle: 0x002a UUID: Report Reference (0x2908) ============================= When decoding packet #44073, we can't append descriptors 0x0029 and 0x002a at the tail of the |service->attributes| since the characteristics 0x002b,0x002f and 0x0032 are already in the array. On Wed, Apr 3, 2024 at 10:37 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Howard, > > On Wed, Apr 3, 2024 at 7:06 AM Howard Chung <howardchung@google.com> wrote: > > > > service->attributes has an assumption that it should look like: > > |char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|... > > where desc_x_y means a descriptor of char_x. > > Will need to check the trace but I believe BlueZ always discover all > characteristics before moving to descriptors, if that is not happening > they there is probably a bug somewhere, that said perhaps you are > doing the discovery procedure with another stack which doesn't employ > the same logic, although inefficient it is possible to discover out of > order but that would require reassigning the descriptors to > characteristics once they are found, which is also inefficient but I > would understand if you after supporting other stacks. > > > In monitor, an attribute is inserted as soon as it is found. It is not > > guranteed that all the descriptors of a characteristic would be found > > before another characteristic is found. > > You might want to include such a trace, don't recall seeing any stack > that does out-order discover. > > > This adds a function to insert the descriptor with the given handle to > > an appropriate place in its service attribute list and make monitor to > > use this function when a descripter is found. > > > > Reviewed-by: Archie Pusaka <apusaka@chromium.org> > > --- > > > > monitor/att.c | 2 +- > > src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++ > > src/shared/gatt-db.h | 9 ++++++ > > 3 files changed, 76 insertions(+), 1 deletion(-) > > > > diff --git a/monitor/att.c b/monitor/att.c > > index ddeb54d9e..503099745 100644 > > --- a/monitor/att.c > > +++ b/monitor/att.c > > @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame, > > if (!db) > > return NULL; > > > > - return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); > > + return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); > > } > > > > static void att_find_info_rsp_16(const struct l2cap_frame *frame) > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > > index 39bba9b54..f08c5afaa 100644 > > --- a/src/shared/gatt-db.c > > +++ b/src/shared/gatt-db.c > > @@ -1002,6 +1002,72 @@ service_append_descriptor(struct gatt_db_service *service, > > return service->attributes[i]; > > } > > > > +struct gatt_db_attribute * > > +gatt_db_insert_descriptor(struct gatt_db *db, > > + uint16_t handle, > > + const bt_uuid_t *uuid, > > + uint32_t permissions, > > + gatt_db_read_t read_func, > > + gatt_db_write_t write_func, > > + void *user_data) > > +{ > > + struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL; > > + struct gatt_db_service *service; > > + int i, j, num_index, char_index; > > + > > + attrib = gatt_db_get_service(db, handle); > > + if (!attrib) > > + return NULL; > > + > > + service = attrib->service; > > + num_index = get_attribute_index(service, 0); > > + if (!num_index) > > + return NULL; > > + > > + // Find the characteristic the descriptor belongs to. > > + for (i = 0; i < num_index; i++) { > > + iter_attr = service->attributes[i]; > > + if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid)) > > + continue; > > + > > + if (iter_attr->handle > handle) > > + continue; > > + > > + // Find the characteristic with the largest handle among those > > + // with handles less than the descriptor's handle. > > + if (!char_attr || iter_attr->handle > char_attr->handle) { > > + char_attr = iter_attr; > > + char_index = i; > > + } > > + } > > + > > + // There is no characteristic contain this descriptor. Something went > > + // wrong > > + if (!char_attr) > > + return NULL; > > + > > + // Find the end of this characteristic > > + for (i = char_index + 1; i < num_index; i++) { > > + iter_attr = service->attributes[i]; > > + > > + if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) || > > + !bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid)) > > + break; > > + } > > + > > + // Move all of the attributes after the end of this characteristic to > > + // its next index. > > + for (j = num_index; j > i; j--) > > + service->attributes[j] = service->attributes[j - 1]; > > + > > + service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0); > > + > > + set_attribute_data(service->attributes[i], read_func, write_func, > > + permissions, user_data); > > + > > + return service->attributes[i]; > > +} > > + > > struct gatt_db_attribute * > > gatt_db_append_descriptor(struct gatt_db *db, > > uint16_t handle, > > diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h > > index ec0eccdfc..4c4e88d87 100644 > > --- a/src/shared/gatt-db.h > > +++ b/src/shared/gatt-db.h > > @@ -80,6 +80,15 @@ gatt_db_append_characteristic(struct gatt_db *db, > > gatt_db_write_t write_func, > > void *user_data); > > > > +struct gatt_db_attribute * > > +gatt_db_insert_descriptor(struct gatt_db *db, > > + uint16_t handle, > > + const bt_uuid_t *uuid, > > + uint32_t permissions, > > + gatt_db_read_t read_func, > > + gatt_db_write_t write_func, > > + void *user_data); > > + > > struct gatt_db_attribute * > > gatt_db_append_descriptor(struct gatt_db *db, > > uint16_t handle, > > -- > > 2.44.0.478.gd926399ef9-goog > > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bluez PATCH 2/2] shared/gatt: Add descriptor insert function 2024-04-08 12:22 ` Yun-hao Chung @ 2024-04-08 14:03 ` Luiz Augusto von Dentz 2024-04-08 16:12 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 9+ messages in thread From: Luiz Augusto von Dentz @ 2024-04-08 14:03 UTC (permalink / raw) To: Yun-hao Chung Cc: linux-bluetooth, Johan Hedberg, Marcel Holtmann, CrosBT Upstreaming, Archie Pusaka Hi, On Mon, Apr 8, 2024 at 8:23 AM Yun-hao Chung <howardchung@google.com> wrote: > > Hi Luiz, > > Thanks for the response. > > This issue was found in ChromeOS edition BlueZ. I'm not sure if any > official BlueZ would do the same discovery procedure or not. > > I extracted relevant part of the trace as below > ========================== > < ACL Data TX: Handle 2048 flags 0x00 dlen 11 > > #43961 [hci0] > 2024-03-05 07:33:55.936283 > ATT: Read By Group Type Request (0x10) len 6 > Handle range: 0x001f-0xffff > Attribute group type: Primary Service (0x2800) > ... > > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #43963 [hci0] 2024-03-05 07:33:55.967020 > > ACL Data RX: Handle 2048 flags 0x01 dlen 3 #43964 [hci0] 2024-03-05 07:33:55.967022 > ATT: Read By Group Type Response (0x11) len 25 > Attribute data length: 6 > Attribute group list: 4 entries > Handle range: 0x001f-0x0035 > UUID: Human Interface Device (0x1812) > Handle range: 0x0036-0x0044 > UUID: Human Interface Device (0x1812) > Handle range: 0x0045-0x0055 > UUID: Human Interface Device (0x1812) > Handle range: 0x0056-0x0062 > UUID: Logitech International SA (0xfd72) > … > < ACL Data TX: Handle 2048 flags 0x00 dlen 11 > > #44018 [hci0] > 2024-03-05 07:33:56.281436 > ATT: Read By Type Request (0x08) len 6 > Handle range: 0x0021-0xffff > Attribute type: Characteristic (0x2803) > ... > > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #44023 [hci0] 2024-03-05 07:33:56.311026 > ATT: Read By Type Response (0x09) len 22 > Attribute data length: 7 > Attribute data list: 3 entries > Handle: 0x0022 > Value: 122300332a > Properties: 0x12 > Read (0x02) > Notify (0x10) > Value Handle: 0x0023 > Value UUID: Boot Mouse Input Report (0x2a33) > Handle: 0x0025 > Value: 0226004b2a > Properties: 0x02 > Read (0x02) > Value Handle: 0x0026 > Value UUID: Report Map (0x2a4b) > Handle: 0x0027 > Value: 1228004d2a > Properties: 0x12 > Read (0x02) > Notify (0x10) > Value Handle: 0x0028 > Value UUID: Report (0x2a4d) > ... > < ACL Data TX: Handle 2048 flags 0x00 dlen 11 > > #44024 [hci0] > 2024-03-05 07:33:56.311458 > ATT: Read By Type Request (0x08) len 6 > Handle range: 0x0028-0xffff > Attribute type: Characteristic (0x2803) > … > > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #44029 [hci0] 2024-03-05 07:33:56.341025 > ATT: Read By Type Response (0x09) len 22 > Attribute data length: 7 > Attribute data list: 3 entries > Handle: 0x002b > Value: 122c004d2a > Properties: 0x12 > Read (0x02) > Notify (0x10) > Value Handle: 0x002c > Value UUID: Report (0x2a4d) > Handle: 0x002f > Value: 0e30004d2a > Properties: 0x0e > Read (0x02) > Write Without Response (0x04) > Write (0x08) > Value Handle: 0x0030 > Value UUID: Report (0x2a4d) > Handle: 0x0032 > Value: 0433004c2a > Properties: 0x04 > Write Without Response (0x04) > Value Handle: 0x0033 > Value UUID: HID Control Point (0x2a4c) > … > < ACL Data TX: Handle 2048 flags 0x00 dlen 9 > > #44071 [hci0] > 2024-03-05 07:33:56.702334 > ATT: Find Information Request (0x04) len 4 > Handle range: 0x0029-0x002a > … > > ACL Data RX: Handle 2048 flags 0x02 dlen 14 #44073 [hci0] 2024-03-05 07:33:56.731069 > ATT: Find Information Response (0x05) len 9 > Format: UUID-16 (0x01) > Handle: 0x0029 > UUID: Client Characteristic Configuration (0x2902) > Handle: 0x002a > UUID: Report Reference (0x2908) > ============================= > > When decoding packet #44073, we can't append descriptors 0x0029 and > 0x002a at the tail of the |service->attributes| since the > characteristics 0x002b,0x002f and 0x0032 are already in the array. So bluetoothd works but the monitor doesn't? They both are using gatt_db_insert_descriptor which uses servvice_insert_descriptor, so where exactly is it failing at? The only difference I see is that in case of gatt-client.c we do: attr = gatt_db_get_attribute(client->db, handle); if (attr && !bt_uuid_cmp(&uuid, gatt_db_attribute_get_type(attr))) continue; The daemon does actually insert the CCC automatically in case there is only one handle left for the it: if (desc_start == chrc_data->end_handle && (chrc_data->properties & BT_GATT_CHRC_PROP_NOTIFY || chrc_data->properties & BT_GATT_CHRC_PROP_INDICATE)) { bt_uuid_t ccc_uuid; /* If there is only one descriptor that must be the CCC * in case either notify or indicate are supported. */ bt_uuid16_create(&ccc_uuid, GATT_CLIENT_CHARAC_CFG_UUID); attr = gatt_db_insert_descriptor(client->db, desc_start, &ccc_uuid, 0, NULL, NULL, NULL); if (attr) { free(chrc_data); continue; } } That said perhaps we need to revise the implementation of get_attribute_index, it does attempt to take the first free index which is incorrect when we are inserting characteristics it shall not allocate the indexes one after another, instead the index shall be handle - service->attribute[0].handle when the handle is set. > > > On Wed, Apr 3, 2024 at 10:37 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Howard, > > > > On Wed, Apr 3, 2024 at 7:06 AM Howard Chung <howardchung@google.com> wrote: > > > > > > service->attributes has an assumption that it should look like: > > > |char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|... > > > where desc_x_y means a descriptor of char_x. > > > > Will need to check the trace but I believe BlueZ always discover all > > characteristics before moving to descriptors, if that is not happening > > they there is probably a bug somewhere, that said perhaps you are > > doing the discovery procedure with another stack which doesn't employ > > the same logic, although inefficient it is possible to discover out of > > order but that would require reassigning the descriptors to > > characteristics once they are found, which is also inefficient but I > > would understand if you after supporting other stacks. > > > > > In monitor, an attribute is inserted as soon as it is found. It is not > > > guranteed that all the descriptors of a characteristic would be found > > > before another characteristic is found. > > > > You might want to include such a trace, don't recall seeing any stack > > that does out-order discover. > > > > > This adds a function to insert the descriptor with the given handle to > > > an appropriate place in its service attribute list and make monitor to > > > use this function when a descripter is found. > > > > > > Reviewed-by: Archie Pusaka <apusaka@chromium.org> > > > --- > > > > > > monitor/att.c | 2 +- > > > src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++ > > > src/shared/gatt-db.h | 9 ++++++ > > > 3 files changed, 76 insertions(+), 1 deletion(-) > > > > > > diff --git a/monitor/att.c b/monitor/att.c > > > index ddeb54d9e..503099745 100644 > > > --- a/monitor/att.c > > > +++ b/monitor/att.c > > > @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame, > > > if (!db) > > > return NULL; > > > > > > - return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); > > > + return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); > > > } > > > > > > static void att_find_info_rsp_16(const struct l2cap_frame *frame) > > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > > > index 39bba9b54..f08c5afaa 100644 > > > --- a/src/shared/gatt-db.c > > > +++ b/src/shared/gatt-db.c > > > @@ -1002,6 +1002,72 @@ service_append_descriptor(struct gatt_db_service *service, > > > return service->attributes[i]; > > > } > > > > > > +struct gatt_db_attribute * > > > +gatt_db_insert_descriptor(struct gatt_db *db, > > > + uint16_t handle, > > > + const bt_uuid_t *uuid, > > > + uint32_t permissions, > > > + gatt_db_read_t read_func, > > > + gatt_db_write_t write_func, > > > + void *user_data) > > > +{ > > > + struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL; > > > + struct gatt_db_service *service; > > > + int i, j, num_index, char_index; > > > + > > > + attrib = gatt_db_get_service(db, handle); > > > + if (!attrib) > > > + return NULL; > > > + > > > + service = attrib->service; > > > + num_index = get_attribute_index(service, 0); > > > + if (!num_index) > > > + return NULL; > > > + > > > + // Find the characteristic the descriptor belongs to. > > > + for (i = 0; i < num_index; i++) { > > > + iter_attr = service->attributes[i]; > > > + if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid)) > > > + continue; > > > + > > > + if (iter_attr->handle > handle) > > > + continue; > > > + > > > + // Find the characteristic with the largest handle among those > > > + // with handles less than the descriptor's handle. > > > + if (!char_attr || iter_attr->handle > char_attr->handle) { > > > + char_attr = iter_attr; > > > + char_index = i; > > > + } > > > + } > > > + > > > + // There is no characteristic contain this descriptor. Something went > > > + // wrong > > > + if (!char_attr) > > > + return NULL; > > > + > > > + // Find the end of this characteristic > > > + for (i = char_index + 1; i < num_index; i++) { > > > + iter_attr = service->attributes[i]; > > > + > > > + if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) || > > > + !bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid)) > > > + break; > > > + } > > > + > > > + // Move all of the attributes after the end of this characteristic to > > > + // its next index. > > > + for (j = num_index; j > i; j--) > > > + service->attributes[j] = service->attributes[j - 1]; > > > + > > > + service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0); > > > + > > > + set_attribute_data(service->attributes[i], read_func, write_func, > > > + permissions, user_data); > > > + > > > + return service->attributes[i]; > > > +} > > > + > > > struct gatt_db_attribute * > > > gatt_db_append_descriptor(struct gatt_db *db, > > > uint16_t handle, > > > diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h > > > index ec0eccdfc..4c4e88d87 100644 > > > --- a/src/shared/gatt-db.h > > > +++ b/src/shared/gatt-db.h > > > @@ -80,6 +80,15 @@ gatt_db_append_characteristic(struct gatt_db *db, > > > gatt_db_write_t write_func, > > > void *user_data); > > > > > > +struct gatt_db_attribute * > > > +gatt_db_insert_descriptor(struct gatt_db *db, > > > + uint16_t handle, > > > + const bt_uuid_t *uuid, > > > + uint32_t permissions, > > > + gatt_db_read_t read_func, > > > + gatt_db_write_t write_func, > > > + void *user_data); > > > + > > > struct gatt_db_attribute * > > > gatt_db_append_descriptor(struct gatt_db *db, > > > uint16_t handle, > > > -- > > > 2.44.0.478.gd926399ef9-goog > > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bluez PATCH 2/2] shared/gatt: Add descriptor insert function 2024-04-08 14:03 ` Luiz Augusto von Dentz @ 2024-04-08 16:12 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 9+ messages in thread From: Luiz Augusto von Dentz @ 2024-04-08 16:12 UTC (permalink / raw) To: Yun-hao Chung Cc: linux-bluetooth, Johan Hedberg, Marcel Holtmann, CrosBT Upstreaming, Archie Pusaka Hi Yun-hao, On Mon, Apr 8, 2024 at 10:03 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi, > > On Mon, Apr 8, 2024 at 8:23 AM Yun-hao Chung <howardchung@google.com> wrote: > > > > Hi Luiz, > > > > Thanks for the response. > > > > This issue was found in ChromeOS edition BlueZ. I'm not sure if any > > official BlueZ would do the same discovery procedure or not. > > > > I extracted relevant part of the trace as below > > ========================== > > < ACL Data TX: Handle 2048 flags 0x00 dlen 11 > > > > #43961 [hci0] > > 2024-03-05 07:33:55.936283 > > ATT: Read By Group Type Request (0x10) len 6 > > Handle range: 0x001f-0xffff > > Attribute group type: Primary Service (0x2800) > > ... > > > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #43963 [hci0] 2024-03-05 07:33:55.967020 > > > ACL Data RX: Handle 2048 flags 0x01 dlen 3 #43964 [hci0] 2024-03-05 07:33:55.967022 > > ATT: Read By Group Type Response (0x11) len 25 > > Attribute data length: 6 > > Attribute group list: 4 entries > > Handle range: 0x001f-0x0035 > > UUID: Human Interface Device (0x1812) > > Handle range: 0x0036-0x0044 > > UUID: Human Interface Device (0x1812) > > Handle range: 0x0045-0x0055 > > UUID: Human Interface Device (0x1812) > > Handle range: 0x0056-0x0062 > > UUID: Logitech International SA (0xfd72) > > … > > < ACL Data TX: Handle 2048 flags 0x00 dlen 11 > > > > #44018 [hci0] > > 2024-03-05 07:33:56.281436 > > ATT: Read By Type Request (0x08) len 6 > > Handle range: 0x0021-0xffff > > Attribute type: Characteristic (0x2803) > > ... > > > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #44023 [hci0] 2024-03-05 07:33:56.311026 > > ATT: Read By Type Response (0x09) len 22 > > Attribute data length: 7 > > Attribute data list: 3 entries > > Handle: 0x0022 > > Value: 122300332a > > Properties: 0x12 > > Read (0x02) > > Notify (0x10) > > Value Handle: 0x0023 > > Value UUID: Boot Mouse Input Report (0x2a33) > > Handle: 0x0025 > > Value: 0226004b2a > > Properties: 0x02 > > Read (0x02) > > Value Handle: 0x0026 > > Value UUID: Report Map (0x2a4b) > > Handle: 0x0027 > > Value: 1228004d2a > > Properties: 0x12 > > Read (0x02) > > Notify (0x10) > > Value Handle: 0x0028 > > Value UUID: Report (0x2a4d) > > ... > > < ACL Data TX: Handle 2048 flags 0x00 dlen 11 > > > > #44024 [hci0] > > 2024-03-05 07:33:56.311458 > > ATT: Read By Type Request (0x08) len 6 > > Handle range: 0x0028-0xffff > > Attribute type: Characteristic (0x2803) > > … > > > ACL Data RX: Handle 2048 flags 0x02 dlen 27 #44029 [hci0] 2024-03-05 07:33:56.341025 > > ATT: Read By Type Response (0x09) len 22 > > Attribute data length: 7 > > Attribute data list: 3 entries > > Handle: 0x002b > > Value: 122c004d2a > > Properties: 0x12 > > Read (0x02) > > Notify (0x10) > > Value Handle: 0x002c > > Value UUID: Report (0x2a4d) > > Handle: 0x002f > > Value: 0e30004d2a > > Properties: 0x0e > > Read (0x02) > > Write Without Response (0x04) > > Write (0x08) > > Value Handle: 0x0030 > > Value UUID: Report (0x2a4d) > > Handle: 0x0032 > > Value: 0433004c2a > > Properties: 0x04 > > Write Without Response (0x04) > > Value Handle: 0x0033 > > Value UUID: HID Control Point (0x2a4c) > > … > > < ACL Data TX: Handle 2048 flags 0x00 dlen 9 > > > > #44071 [hci0] > > 2024-03-05 07:33:56.702334 > > ATT: Find Information Request (0x04) len 4 > > Handle range: 0x0029-0x002a > > … > > > ACL Data RX: Handle 2048 flags 0x02 dlen 14 #44073 [hci0] 2024-03-05 07:33:56.731069 > > ATT: Find Information Response (0x05) len 9 > > Format: UUID-16 (0x01) > > Handle: 0x0029 > > UUID: Client Characteristic Configuration (0x2902) > > Handle: 0x002a > > UUID: Report Reference (0x2908) > > ============================= > > > > When decoding packet #44073, we can't append descriptors 0x0029 and > > 0x002a at the tail of the |service->attributes| since the > > characteristics 0x002b,0x002f and 0x0032 are already in the array. > > So bluetoothd works but the monitor doesn't? They both are using > gatt_db_insert_descriptor which uses servvice_insert_descriptor, so > where exactly is it failing at? The only difference I see is that in > case of gatt-client.c we do: > > attr = gatt_db_get_attribute(client->db, handle); > if (attr && !bt_uuid_cmp(&uuid, > gatt_db_attribute_get_type(attr))) > continue; > > > The daemon does actually insert the CCC automatically in case there is > only one handle left for the it: > > if (desc_start == chrc_data->end_handle && > (chrc_data->properties & BT_GATT_CHRC_PROP_NOTIFY || > chrc_data->properties & BT_GATT_CHRC_PROP_INDICATE)) { > bt_uuid_t ccc_uuid; > > /* If there is only one descriptor that must be the CCC > * in case either notify or indicate are supported. > */ > bt_uuid16_create(&ccc_uuid, > GATT_CLIENT_CHARAC_CFG_UUID); > attr = gatt_db_insert_descriptor(client->db, desc_start, > &ccc_uuid, 0, NULL, > NULL, NULL); > if (attr) { > free(chrc_data); > continue; > } > } > > That said perhaps we need to revise the implementation of > get_attribute_index, it does attempt to take the first free index > which is incorrect when we are inserting characteristics it shall not > allocate the indexes one after another, instead the index shall be > handle - service->attribute[0].handle when the handle is set. Here is a tentative of fixing the index logic: https://patchwork.kernel.org/project/bluetooth/patch/20240408155949.3622429-1-luiz.dentz@gmail.com/ > > > > > > On Wed, Apr 3, 2024 at 10:37 PM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Howard, > > > > > > On Wed, Apr 3, 2024 at 7:06 AM Howard Chung <howardchung@google.com> wrote: > > > > > > > > service->attributes has an assumption that it should look like: > > > > |char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|... > > > > where desc_x_y means a descriptor of char_x. > > > > > > Will need to check the trace but I believe BlueZ always discover all > > > characteristics before moving to descriptors, if that is not happening > > > they there is probably a bug somewhere, that said perhaps you are > > > doing the discovery procedure with another stack which doesn't employ > > > the same logic, although inefficient it is possible to discover out of > > > order but that would require reassigning the descriptors to > > > characteristics once they are found, which is also inefficient but I > > > would understand if you after supporting other stacks. > > > > > > > In monitor, an attribute is inserted as soon as it is found. It is not > > > > guranteed that all the descriptors of a characteristic would be found > > > > before another characteristic is found. > > > > > > You might want to include such a trace, don't recall seeing any stack > > > that does out-order discover. > > > > > > > This adds a function to insert the descriptor with the given handle to > > > > an appropriate place in its service attribute list and make monitor to > > > > use this function when a descripter is found. > > > > > > > > Reviewed-by: Archie Pusaka <apusaka@chromium.org> > > > > --- > > > > > > > > monitor/att.c | 2 +- > > > > src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++ > > > > src/shared/gatt-db.h | 9 ++++++ > > > > 3 files changed, 76 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/monitor/att.c b/monitor/att.c > > > > index ddeb54d9e..503099745 100644 > > > > --- a/monitor/att.c > > > > +++ b/monitor/att.c > > > > @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame, > > > > if (!db) > > > > return NULL; > > > > > > > > - return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); > > > > + return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL); > > > > } > > > > > > > > static void att_find_info_rsp_16(const struct l2cap_frame *frame) > > > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > > > > index 39bba9b54..f08c5afaa 100644 > > > > --- a/src/shared/gatt-db.c > > > > +++ b/src/shared/gatt-db.c > > > > @@ -1002,6 +1002,72 @@ service_append_descriptor(struct gatt_db_service *service, > > > > return service->attributes[i]; > > > > } > > > > > > > > +struct gatt_db_attribute * > > > > +gatt_db_insert_descriptor(struct gatt_db *db, > > > > + uint16_t handle, > > > > + const bt_uuid_t *uuid, > > > > + uint32_t permissions, > > > > + gatt_db_read_t read_func, > > > > + gatt_db_write_t write_func, > > > > + void *user_data) > > > > +{ > > > > + struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL; > > > > + struct gatt_db_service *service; > > > > + int i, j, num_index, char_index; > > > > + > > > > + attrib = gatt_db_get_service(db, handle); > > > > + if (!attrib) > > > > + return NULL; > > > > + > > > > + service = attrib->service; > > > > + num_index = get_attribute_index(service, 0); > > > > + if (!num_index) > > > > + return NULL; > > > > + > > > > + // Find the characteristic the descriptor belongs to. > > > > + for (i = 0; i < num_index; i++) { > > > > + iter_attr = service->attributes[i]; > > > > + if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid)) > > > > + continue; > > > > + > > > > + if (iter_attr->handle > handle) > > > > + continue; > > > > + > > > > + // Find the characteristic with the largest handle among those > > > > + // with handles less than the descriptor's handle. > > > > + if (!char_attr || iter_attr->handle > char_attr->handle) { > > > > + char_attr = iter_attr; > > > > + char_index = i; > > > > + } > > > > + } > > > > + > > > > + // There is no characteristic contain this descriptor. Something went > > > > + // wrong > > > > + if (!char_attr) > > > > + return NULL; > > > > + > > > > + // Find the end of this characteristic > > > > + for (i = char_index + 1; i < num_index; i++) { > > > > + iter_attr = service->attributes[i]; > > > > + > > > > + if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) || > > > > + !bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid)) > > > > + break; > > > > + } > > > > + > > > > + // Move all of the attributes after the end of this characteristic to > > > > + // its next index. > > > > + for (j = num_index; j > i; j--) > > > > + service->attributes[j] = service->attributes[j - 1]; > > > > + > > > > + service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0); > > > > + > > > > + set_attribute_data(service->attributes[i], read_func, write_func, > > > > + permissions, user_data); > > > > + > > > > + return service->attributes[i]; > > > > +} > > > > + > > > > struct gatt_db_attribute * > > > > gatt_db_append_descriptor(struct gatt_db *db, > > > > uint16_t handle, > > > > diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h > > > > index ec0eccdfc..4c4e88d87 100644 > > > > --- a/src/shared/gatt-db.h > > > > +++ b/src/shared/gatt-db.h > > > > @@ -80,6 +80,15 @@ gatt_db_append_characteristic(struct gatt_db *db, > > > > gatt_db_write_t write_func, > > > > void *user_data); > > > > > > > > +struct gatt_db_attribute * > > > > +gatt_db_insert_descriptor(struct gatt_db *db, > > > > + uint16_t handle, > > > > + const bt_uuid_t *uuid, > > > > + uint32_t permissions, > > > > + gatt_db_read_t read_func, > > > > + gatt_db_write_t write_func, > > > > + void *user_data); > > > > + > > > > struct gatt_db_attribute * > > > > gatt_db_append_descriptor(struct gatt_db *db, > > > > uint16_t handle, > > > > -- > > > > 2.44.0.478.gd926399ef9-goog > > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-04-08 16:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-03 11:05 [Bluez PATCH 0/2] Fix descriptor display issue in btmon Howard Chung 2024-04-03 11:05 ` [Bluez PATCH 1/2] shared/gatt: Rename some gatt insert functions to append Howard Chung 2024-04-03 12:59 ` Fix descriptor display issue in btmon bluez.test.bot 2024-04-03 14:29 ` [Bluez PATCH 1/2] shared/gatt: Rename some gatt insert functions to append Luiz Augusto von Dentz 2024-04-03 11:05 ` [Bluez PATCH 2/2] shared/gatt: Add descriptor insert function Howard Chung 2024-04-03 14:36 ` Luiz Augusto von Dentz 2024-04-08 12:22 ` Yun-hao Chung 2024-04-08 14:03 ` Luiz Augusto von Dentz 2024-04-08 16:12 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).