Message ID | 20230724122908.42409-2-nitin.jadhav@nxp.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Fixed the crash observed with VOCS | expand |
Context | Check | Description |
---|---|---|
tedd_an/pre-ci_am | success | Success |
tedd_an/CheckPatch | success | CheckPatch PASS |
tedd_an/GitLint | success | Gitlint PASS |
tedd_an/BuildEll | success | Build ELL PASS |
tedd_an/BluezMake | success | Bluez Make PASS |
tedd_an/MakeCheck | success | Bluez Make Check PASS |
tedd_an/MakeDistcheck | success | Make Distcheck PASS |
tedd_an/CheckValgrind | success | Check Valgrind PASS |
tedd_an/CheckSmatch | success | CheckSparse PASS |
tedd_an/bluezmakeextell | success | Make External ELL PASS |
tedd_an/IncrementalBuild | success | Incremental Build PASS |
tedd_an/ScanBuild | success | Scan Build PASS |
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=768882 ---Test result--- Test Summary: CheckPatch PASS 0.51 seconds GitLint PASS 0.32 seconds BuildEll PASS 33.95 seconds BluezMake PASS 977.84 seconds MakeCheck PASS 13.43 seconds MakeDistcheck PASS 196.12 seconds CheckValgrind PASS 317.14 seconds CheckSmatch PASS 418.65 seconds bluezmakeextell PASS 129.27 seconds IncrementalBuild PASS 811.74 seconds ScanBuild PASS 1304.39 seconds --- Regards, Linux Bluetooth
Hi Nitin, On Mon, Jul 24, 2023 at 5:29 AM Nitin Jadhav <nitin.jadhav@nxp.com> wrote: > > Root cause > - There are two types of database- Remote and Local (rdb and ldb). > - In client mode currently the code was written to access ldb. > > Fix > - Correcting it, to access rdb has resolved the problem in VOCS. > - Same correction is done for VCS. > > Reported-by: Pauli Virtanen <pav@iki.fi> > --- > v2: Fixed GitLint and ScanBuild warnings > --- > src/shared/vcp.c | 52 +++++++++++++++++++++++++++--------------------- > 1 file changed, 29 insertions(+), 23 deletions(-) > > diff --git a/src/shared/vcp.c b/src/shared/vcp.c > index 74bd01729..80d4dfcd4 100644 > --- a/src/shared/vcp.c > +++ b/src/shared/vcp.c > @@ -973,7 +973,7 @@ static void vocs_voaodec_read(struct gatt_db_attribute *attrib, > struct bt_vocs *vocs = user_data; > struct iovec iov; > > - iov.iov_base = &vocs->vocs_ao_dec; > + iov.iov_base = vocs->vocs_ao_dec; > iov.iov_len = strlen(vocs->vocs_ao_dec); > > gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base, > @@ -998,10 +998,12 @@ static struct bt_vcs *vcs_new(struct gatt_db *db, struct bt_vcp_db *vdb) > > /* Populate DB with VCS attributes */ > bt_uuid16_create(&uuid, VCS_UUID); > - vcs->service = gatt_db_add_service(db, &uuid, true, 9); > + > + vcs->service = gatt_db_add_service(db, &uuid, true, 10); Not sure what this has to do with the crashes? > gatt_db_service_add_included(vcs->service, vdb->vocs->service); > gatt_db_service_set_active(vdb->vocs->service, true); > > + Please remove the extra space above. > bt_uuid16_create(&uuid, VOL_STATE_CHRC_UUID); > vcs->vs = gatt_db_service_add_characteristic(vcs->service, > &uuid, > @@ -1385,11 +1387,12 @@ static void read_vocs_audio_location(struct bt_vcp *vcp, bool success, > const uint8_t *value, uint16_t length, > void *user_data) > { > - uint32_t *vocs_audio_loc; > - struct iovec iov = { > - .iov_base = (void *) value, > - .iov_len = length, > - }; > + uint32_t vocs_audio_loc; > + > + if (!value) { > + DBG(vcp, "Unable to get VOCS Audio Location"); > + return; > + } > > if (!success) { > DBG(vcp, "Unable to read VOCS Audio Location: error 0x%02x", > @@ -1397,26 +1400,22 @@ static void read_vocs_audio_location(struct bt_vcp *vcp, bool success, > return; > } > > - vocs_audio_loc = iov_pull_mem(&iov, sizeof(uint32_t)); > - if (!*vocs_audio_loc) { > - DBG(vcp, "Unable to get VOCS Audio Location"); > - return; > - } > + memcpy(&vocs_audio_loc, value, length); > > - DBG(vcp, "VOCS Audio Loc:%x", *vocs_audio_loc); > + DBG(vcp, "VOCS Audio Loc:%x", vocs_audio_loc); We should probably store the location within vcp, that said I don't think this has anything to do with the crashes though. > } > > - > static void read_vocs_audio_descriptor(struct bt_vcp *vcp, bool success, > uint8_t att_ecode, > const uint8_t *value, uint16_t length, > void *user_data) > { > char *vocs_ao_dec_r; > - struct iovec iov = { > - .iov_base = (void *) value, > - .iov_len = length, > - }; > + > + if (!value) { > + DBG(vcp, "Unable to get VOCS Audio Descriptor"); > + return; > + } > > if (!success) { > DBG(vcp, "Unable to read VOCS Audio Descriptor: error 0x%02x", > @@ -1424,13 +1423,20 @@ static void read_vocs_audio_descriptor(struct bt_vcp *vcp, bool success, > return; > } > > - vocs_ao_dec_r = iov_pull_mem(&iov, length); > - if (!*vocs_ao_dec_r) { > + vocs_ao_dec_r = malloc(length+1); > + memset(vocs_ao_dec_r, 0, length+1); > + > + memcpy(vocs_ao_dec_r, value, length); > + > + if (!vocs_ao_dec_r) { > DBG(vcp, "Unable to get VOCS Audio Descriptor"); > return; > } > > - DBG(vcp, "VOCS Audio Descriptor:%s", *vocs_ao_dec_r); > + DBG(vcp, "VOCS Audio Descriptor: %s", vocs_ao_dec_r); > > + free(vocs_ao_dec_r); > + vocs_ao_dec_r = NULL; Ditto, not really sure what this has to do with the crashes, besides allocating memory just to free it here sounds useless. > } > > static void vcp_pending_destroy(void *data) > @@ -1719,10 +1725,10 @@ bool bt_vcp_attach(struct bt_vcp *vcp, struct bt_gatt_client *client) > return false; > > bt_uuid16_create(&uuid, VCS_UUID); > - gatt_db_foreach_service(vcp->ldb->db, &uuid, foreach_vcs_service, vcp); > + gatt_db_foreach_service(vcp->rdb->db, &uuid, foreach_vcs_service, vcp); > > bt_uuid16_create(&uuid, VOL_OFFSET_CS_UUID); > - gatt_db_foreach_service(vcp->ldb->db, &uuid, foreach_vocs_service, vcp); > + gatt_db_foreach_service(vcp->rdb->db, &uuid, foreach_vocs_service, vcp); These changes seem to be the actual problem, so I'd split them and if you still want to introduce the other changes as well please create proper commits describing why they are required. > return true; > } > -- > 2.34.1 >
diff --git a/src/shared/vcp.c b/src/shared/vcp.c index 74bd01729..80d4dfcd4 100644 --- a/src/shared/vcp.c +++ b/src/shared/vcp.c @@ -973,7 +973,7 @@ static void vocs_voaodec_read(struct gatt_db_attribute *attrib, struct bt_vocs *vocs = user_data; struct iovec iov; - iov.iov_base = &vocs->vocs_ao_dec; + iov.iov_base = vocs->vocs_ao_dec; iov.iov_len = strlen(vocs->vocs_ao_dec); gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base, @@ -998,10 +998,12 @@ static struct bt_vcs *vcs_new(struct gatt_db *db, struct bt_vcp_db *vdb) /* Populate DB with VCS attributes */ bt_uuid16_create(&uuid, VCS_UUID); - vcs->service = gatt_db_add_service(db, &uuid, true, 9); + + vcs->service = gatt_db_add_service(db, &uuid, true, 10); gatt_db_service_add_included(vcs->service, vdb->vocs->service); gatt_db_service_set_active(vdb->vocs->service, true); + bt_uuid16_create(&uuid, VOL_STATE_CHRC_UUID); vcs->vs = gatt_db_service_add_characteristic(vcs->service, &uuid, @@ -1385,11 +1387,12 @@ static void read_vocs_audio_location(struct bt_vcp *vcp, bool success, const uint8_t *value, uint16_t length, void *user_data) { - uint32_t *vocs_audio_loc; - struct iovec iov = { - .iov_base = (void *) value, - .iov_len = length, - }; + uint32_t vocs_audio_loc; + + if (!value) { + DBG(vcp, "Unable to get VOCS Audio Location"); + return; + } if (!success) { DBG(vcp, "Unable to read VOCS Audio Location: error 0x%02x", @@ -1397,26 +1400,22 @@ static void read_vocs_audio_location(struct bt_vcp *vcp, bool success, return; } - vocs_audio_loc = iov_pull_mem(&iov, sizeof(uint32_t)); - if (!*vocs_audio_loc) { - DBG(vcp, "Unable to get VOCS Audio Location"); - return; - } + memcpy(&vocs_audio_loc, value, length); - DBG(vcp, "VOCS Audio Loc:%x", *vocs_audio_loc); + DBG(vcp, "VOCS Audio Loc:%x", vocs_audio_loc); } - static void read_vocs_audio_descriptor(struct bt_vcp *vcp, bool success, uint8_t att_ecode, const uint8_t *value, uint16_t length, void *user_data) { char *vocs_ao_dec_r; - struct iovec iov = { - .iov_base = (void *) value, - .iov_len = length, - }; + + if (!value) { + DBG(vcp, "Unable to get VOCS Audio Descriptor"); + return; + } if (!success) { DBG(vcp, "Unable to read VOCS Audio Descriptor: error 0x%02x", @@ -1424,13 +1423,20 @@ static void read_vocs_audio_descriptor(struct bt_vcp *vcp, bool success, return; } - vocs_ao_dec_r = iov_pull_mem(&iov, length); - if (!*vocs_ao_dec_r) { + vocs_ao_dec_r = malloc(length+1); + memset(vocs_ao_dec_r, 0, length+1); + + memcpy(vocs_ao_dec_r, value, length); + + if (!vocs_ao_dec_r) { DBG(vcp, "Unable to get VOCS Audio Descriptor"); return; } - DBG(vcp, "VOCS Audio Descriptor:%s", *vocs_ao_dec_r); + DBG(vcp, "VOCS Audio Descriptor: %s", vocs_ao_dec_r); + + free(vocs_ao_dec_r); + vocs_ao_dec_r = NULL; } static void vcp_pending_destroy(void *data) @@ -1719,10 +1725,10 @@ bool bt_vcp_attach(struct bt_vcp *vcp, struct bt_gatt_client *client) return false; bt_uuid16_create(&uuid, VCS_UUID); - gatt_db_foreach_service(vcp->ldb->db, &uuid, foreach_vcs_service, vcp); + gatt_db_foreach_service(vcp->rdb->db, &uuid, foreach_vcs_service, vcp); bt_uuid16_create(&uuid, VOL_OFFSET_CS_UUID); - gatt_db_foreach_service(vcp->ldb->db, &uuid, foreach_vocs_service, vcp); + gatt_db_foreach_service(vcp->rdb->db, &uuid, foreach_vocs_service, vcp); return true; }