From: Luiz Augusto von Dentz <[email protected]>
This uses bt_gatt_client_idle_register to track when instance is ready
instead of using a dedicated queue to track requests.
Fixes: https://github.com/bluez/bluez/issues/485
---
src/shared/bap.c | 168 +++++++++++++++++++++--------------------------
1 file changed, 74 insertions(+), 94 deletions(-)
diff --git a/src/shared/bap.c b/src/shared/bap.c
index c0f35e1c9aa4..3ebcd81f16c1 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -127,17 +127,6 @@ struct bt_bap_req {
void *user_data;
};
-typedef void (*bap_func_t)(struct bt_bap *bap, bool success, uint8_t att_ecode,
- const uint8_t *value, uint16_t length,
- void *user_data);
-
-struct bt_bap_pending {
- unsigned int id;
- struct bt_bap *bap;
- bap_func_t func;
- void *user_data;
-};
-
typedef void (*bap_notify_t)(struct bt_bap *bap, uint16_t value_handle,
const uint8_t *value, uint16_t length,
void *user_data);
@@ -156,12 +145,13 @@ struct bt_bap {
struct bt_gatt_client *client;
struct bt_att *att;
struct bt_bap_req *req;
- unsigned int cp_id;
+ unsigned int cp_id;
unsigned int process_id;
unsigned int disconn_id;
+ unsigned int idle_id;
+
struct queue *reqs;
- struct queue *pending;
struct queue *notify;
struct queue *streams;
struct queue *local_eps;
@@ -192,6 +182,7 @@ struct bt_bap_pac {
};
struct bt_bap_endpoint {
+ struct bt_bap *bap;
struct bt_bap_db *bdb;
struct bt_bap_stream *stream;
struct gatt_db_attribute *attr;
@@ -2576,7 +2567,6 @@ static void bap_free(void *data)
queue_destroy(bap->remote_eps, free);
queue_destroy(bap->reqs, bap_req_free);
- queue_destroy(bap->pending, NULL);
queue_destroy(bap->notify, NULL);
queue_destroy(bap->streams, bap_stream_free);
@@ -2650,7 +2640,6 @@ struct bt_bap *bt_bap_new(struct gatt_db *ldb, struct gatt_db *rdb)
bap = new0(struct bt_bap, 1);
bap->ldb = bdb;
bap->reqs = queue_new();
- bap->pending = queue_new();
bap->notify = queue_new();
bap->pac_cbs = queue_new();
bap->ready_cbs = queue_new();
@@ -2735,9 +2724,6 @@ static void bap_notify_ready(struct bt_bap *bap)
{
const struct queue_entry *entry;
- if (!queue_isempty(bap->pending))
- return;
-
if (!bt_bap_ref_safe(bap))
return;
@@ -2835,10 +2821,12 @@ static void bap_parse_pacs(struct bt_bap *bap, uint8_t type,
}
}
-static void read_source_pac(struct bt_bap *bap, bool success, uint8_t att_ecode,
+static void read_source_pac(bool success, uint8_t att_ecode,
const uint8_t *value, uint16_t length,
void *user_data)
{
+ struct bt_bap *bap = user_data;
+
if (!success) {
DBG(bap, "Unable to read Source PAC: error 0x%02x", att_ecode);
return;
@@ -2847,10 +2835,12 @@ static void read_source_pac(struct bt_bap *bap, bool success, uint8_t att_ecode,
bap_parse_pacs(bap, BT_BAP_SOURCE, bap->rdb->sources, value, length);
}
-static void read_sink_pac(struct bt_bap *bap, bool success, uint8_t att_ecode,
+static void read_sink_pac(bool success, uint8_t att_ecode,
const uint8_t *value, uint16_t length,
void *user_data)
{
+ struct bt_bap *bap = user_data;
+
if (!success) {
DBG(bap, "Unable to read Sink PAC: error 0x%02x", att_ecode);
return;
@@ -2859,54 +2849,11 @@ static void read_sink_pac(struct bt_bap *bap, bool success, uint8_t att_ecode,
bap_parse_pacs(bap, BT_BAP_SINK, bap->rdb->sinks, value, length);
}
-static void bap_pending_destroy(void *data)
-{
- struct bt_bap_pending *pending = data;
- struct bt_bap *bap = pending->bap;
-
- if (queue_remove_if(bap->pending, NULL, pending))
- free(pending);
-
- bap_notify_ready(bap);
-}
-
-static void bap_pending_complete(bool success, uint8_t att_ecode,
- const uint8_t *value, uint16_t length,
- void *user_data)
-{
- struct bt_bap_pending *pending = user_data;
-
- if (pending->func)
- pending->func(pending->bap, success, att_ecode, value, length,
- pending->user_data);
-}
-
-static void bap_read_value(struct bt_bap *bap, uint16_t value_handle,
- bap_func_t func, void *user_data)
-{
- struct bt_bap_pending *pending;
-
- pending = new0(struct bt_bap_pending, 1);
- pending->bap = bap;
- pending->func = func;
- pending->user_data = user_data;
-
- pending->id = bt_gatt_client_read_value(bap->client, value_handle,
- bap_pending_complete, pending,
- bap_pending_destroy);
- if (!pending->id) {
- DBG(bap, "Unable to send Read request");
- free(pending);
- return;
- }
-
- queue_push_tail(bap->pending, pending);
-}
-
-static void read_source_pac_loc(struct bt_bap *bap, bool success,
- uint8_t att_ecode, const uint8_t *value,
- uint16_t length, void *user_data)
+static void read_source_pac_loc(bool success, uint8_t att_ecode,
+ const uint8_t *value, uint16_t length,
+ void *user_data)
{
+ struct bt_bap *bap = user_data;
struct bt_pacs *pacs = bap_get_pacs(bap);
if (!success) {
@@ -2925,14 +2872,17 @@ static void read_source_pac_loc(struct bt_bap *bap, bool success,
if (gatt_db_attribute_get_char_data(pacs->source,
NULL, &value_handle,
NULL, NULL, NULL))
- bap_read_value(bap, value_handle, read_source_pac, bap);
+ bt_gatt_client_read_value(bap->client, value_handle,
+ read_source_pac, bap,
+ NULL);
}
}
-static void read_sink_pac_loc(struct bt_bap *bap, bool success,
- uint8_t att_ecode, const uint8_t *value,
- uint16_t length, void *user_data)
+static void read_sink_pac_loc(bool success, uint8_t att_ecode,
+ const uint8_t *value, uint16_t length,
+ void *user_data)
{
+ struct bt_bap *bap = user_data;
struct bt_pacs *pacs = bap_get_pacs(bap);
if (!success) {
@@ -2951,14 +2901,17 @@ static void read_sink_pac_loc(struct bt_bap *bap, bool success,
if (gatt_db_attribute_get_char_data(pacs->sink,
NULL, &value_handle,
NULL, NULL, NULL))
- bap_read_value(bap, value_handle, read_sink_pac, bap);
+ bt_gatt_client_read_value(bap->client, value_handle,
+ read_sink_pac, bap,
+ NULL);
}
}
-static void read_pac_context(struct bt_bap *bap, bool success,
- uint8_t att_ecode, const uint8_t *value,
- uint16_t length, void *user_data)
+static void read_pac_context(bool success, uint8_t att_ecode,
+ const uint8_t *value, uint16_t length,
+ void *user_data)
{
+ struct bt_bap *bap = user_data;
struct bt_pacs *pacs = bap_get_pacs(bap);
if (!success) {
@@ -2970,10 +2923,11 @@ static void read_pac_context(struct bt_bap *bap, bool success,
NULL, NULL);
}
-static void read_pac_supported_context(struct bt_bap *bap, bool success,
- uint8_t att_ecode, const uint8_t *value,
- uint16_t length, void *user_data)
+static void read_pac_supported_context(bool success, uint8_t att_ecode,
+ const uint8_t *value, uint16_t length,
+ void *user_data)
{
+ struct bt_bap *bap = user_data;
struct bt_pacs *pacs = bap_get_pacs(bap);
if (!success) {
@@ -3015,7 +2969,8 @@ static void foreach_pacs_char(struct gatt_db_attribute *attr, void *user_data)
if (!pacs->sink)
pacs->sink = attr;
- bap_read_value(bap, value_handle, read_sink_pac, bap);
+ bt_gatt_client_read_value(bap->client, value_handle,
+ read_sink_pac, bap, NULL);
}
if (!bt_uuid_cmp(&uuid, &uuid_source)) {
@@ -3028,7 +2983,8 @@ static void foreach_pacs_char(struct gatt_db_attribute *attr, void *user_data)
if (!pacs->source)
pacs->source = attr;
- bap_read_value(bap, value_handle, read_source_pac, NULL);
+ bt_gatt_client_read_value(bap->client, value_handle,
+ read_source_pac, bap, NULL);
}
if (!bt_uuid_cmp(&uuid, &uuid_sink_loc)) {
@@ -3040,7 +2996,8 @@ static void foreach_pacs_char(struct gatt_db_attribute *attr, void *user_data)
return;
pacs->sink_loc = attr;
- bap_read_value(bap, value_handle, read_sink_pac_loc, NULL);
+ bt_gatt_client_read_value(bap->client, value_handle,
+ read_sink_pac_loc, bap, NULL);
}
if (!bt_uuid_cmp(&uuid, &uuid_source_loc)) {
@@ -3052,7 +3009,8 @@ static void foreach_pacs_char(struct gatt_db_attribute *attr, void *user_data)
return;
pacs->source_loc = attr;
- bap_read_value(bap, value_handle, read_source_pac_loc, NULL);
+ bt_gatt_client_read_value(bap->client, value_handle,
+ read_source_pac_loc, bap, NULL);
}
if (!bt_uuid_cmp(&uuid, &uuid_context)) {
@@ -3063,7 +3021,8 @@ static void foreach_pacs_char(struct gatt_db_attribute *attr, void *user_data)
return;
pacs->context = attr;
- bap_read_value(bap, value_handle, read_pac_context, NULL);
+ bt_gatt_client_read_value(bap->client, value_handle,
+ read_pac_context, bap, NULL);
}
if (!bt_uuid_cmp(&uuid, &uuid_supported_context)) {
@@ -3075,8 +3034,9 @@ static void foreach_pacs_char(struct gatt_db_attribute *attr, void *user_data)
return;
pacs->supported_context = attr;
- bap_read_value(bap, value_handle, read_pac_supported_context,
- NULL);
+ bt_gatt_client_read_value(bap->client, value_handle,
+ read_pac_supported_context,
+ bap, NULL);
}
}
@@ -3324,14 +3284,17 @@ static void bap_ep_set_status(struct bt_bap *bap, struct bt_bap_endpoint *ep,
bap_stream_state_changed(ep->stream);
}
-static void read_ase_status(struct bt_bap *bap, bool success, uint8_t att_ecode,
+static void read_ase_status(bool success, uint8_t att_ecode,
const uint8_t *value, uint16_t length,
void *user_data)
{
struct bt_bap_endpoint *ep = user_data;
+ struct bt_bap *bap = ep->bap;
- if (!success)
+ if (!success) {
+ DBG(bap, "ASE read status failed: 0x%04x", att_ecode);
return;
+ }
bap_ep_set_status(bap, ep, value, length);
}
@@ -3409,7 +3372,10 @@ static void bap_endpoint_attach(struct bt_bap *bap, struct bt_bap_endpoint *ep)
DBG(bap, "ASE handle 0x%04x", value_handle);
- bap_read_value(bap, value_handle, read_ase_status, ep);
+ ep->bap = bap;
+
+ bt_gatt_client_read_value(bap->client, value_handle, read_ase_status,
+ ep, NULL);
ep->state_id = bap_register_notify(bap, value_handle,
bap_endpoint_notify, ep);
@@ -3734,6 +3700,15 @@ static void bap_attach_att(struct bt_bap *bap, struct bt_att *att)
bap, NULL);
}
+static void bap_idle(void *data)
+{
+ struct bt_bap *bap = data;
+
+ bap->idle_id = 0;
+
+ bap_notify_ready(bap);
+}
+
bool bt_bap_attach(struct bt_bap *bap, struct bt_gatt_client *client)
{
bt_uuid_t uuid;
@@ -3769,6 +3744,9 @@ clone:
bap_attach_att(bap, bt_gatt_client_get_att(client));
+ bap->idle_id = bt_gatt_client_idle_register(bap->client, bap_idle,
+ bap, NULL);
+
if (bap->rdb->pacs) {
uint16_t value_handle;
struct bt_pacs *pacs = bap->rdb->pacs;
@@ -3778,8 +3756,10 @@ clone:
if (gatt_db_attribute_get_char_data(pacs->sink,
NULL, &value_handle,
NULL, NULL, NULL)) {
- bap_read_value(bap, value_handle,
- read_sink_pac, bap);
+ bt_gatt_client_read_value(bap->client,
+ value_handle,
+ read_sink_pac,
+ bap, NULL);
}
}
@@ -3788,8 +3768,10 @@ clone:
if (gatt_db_attribute_get_char_data(pacs->source,
NULL, &value_handle,
NULL, NULL, NULL)) {
- bap_read_value(bap, value_handle,
- read_source_pac, bap);
+ bt_gatt_client_read_value(bap->client,
+ value_handle,
+ read_source_pac,
+ bap, NULL);
}
}
@@ -3797,8 +3779,6 @@ clone:
bap_cp_attach(bap);
- bap_notify_ready(bap);
-
return true;
}
--
2.39.2
From: Luiz Augusto von Dentz <[email protected]>
If an unexpected disconnect happens while bt_bap_config is pending the
following trace can be observed, to fix it bt_bap_config is reworked so
it no longer attempts to create and config the stream in place, instead
it just return the new stream and the function is renamed to
bt_bap_stream_new:
Invalid write of size 4
at 0x3980D8: config_cb (bap.c:395)
by 0x4DF5A3: bap_req_complete (bap.c:3471)
by 0x4E9D33: bap_req_detach (bap.c:3807)
by 0x4E9D33: bt_bap_detach (bap.c:3819)
by 0x4E9D33: bt_bap_detach (bap.c:3810)
by 0x397AA9: bap_disconnect (bap.c:1342)
by 0x4223E0: btd_service_disconnect (service.c:305)
by 0x4974D8F: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.7200.3)
by 0x438FC3: att_disconnected_cb (device.c:5160)
by 0x49A6C6: queue_foreach (queue.c:207)
by 0x4B463B: disconnect_cb (att.c:701)
by 0x5054DF: watch_callback (io-glib.c:157)
by 0x495BFAE: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.7200.3)
by 0x49B12C7: ??? (in /usr/lib64/libglib-2.0.so.0.7200.3)
Address 0x6576940 is 96 bytes inside a block of size 112 free'd
at 0x48480E4: free (vg_replace_malloc.c:872)
by 0x48F78D: remove_interface (object.c:660)
by 0x490489: g_dbus_unregister_interface (object.c:1394)
by 0x397BA8: ep_remove (bap.c:1330)
by 0x49ACF4: queue_remove_if (queue.c:279)
by 0x49B0AC: queue_remove_all (queue.c:321)
by 0x397A7C: bap_disconnect (bap.c:1339)
by 0x4223E0: btd_service_disconnect (service.c:305)
by 0x4974D8F: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.7200.3)
by 0x438FC3: att_disconnected_cb (device.c:5160)
by 0x49A6C6: queue_foreach (queue.c:207)
by 0x4B463B: disconnect_cb (att.c:701)
Block was alloc'd at
at 0x484586F: malloc (vg_replace_malloc.c:381)
by 0x49B432: util_malloc (util.c:43)
by 0x39A1D9: ep_register (bap.c:563)
by 0x39A1D9: pac_found (bap.c:664)
by 0x4E5FEA: bap_foreach_pac (bap.c:3980)
by 0x4EA437: bap_notify_ready (bap.c:2736)
by 0x4EA437: bap_idle (bap.c:3711)
by 0x4B52F0: idle_notify (gatt-client.c:171)
by 0x49ACF4: queue_remove_if (queue.c:279)
by 0x49B0AC: queue_remove_all (queue.c:321)
by 0x4C092C: notify_client_idle (gatt-client.c:180)
by 0x4C092C: request_unref (gatt-client.c:199)
by 0x4AACB5: destroy_att_send_op (att.c:209)
by 0x4B2B88: handle_rsp (att.c:862)
by 0x4B2B88: can_read_data (att.c:1052)
by 0x5054DF: watch_callback (io-glib.c:157)
---
profiles/audio/bap.c | 28 ++++++++++++----------------
src/shared/bap.c | 16 ++--------------
src/shared/bap.h | 6 ++----
3 files changed, 16 insertions(+), 34 deletions(-)
diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index b8c75f195854..dfdf8725589d 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -466,15 +466,13 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
/* TODO: Check if stream capabilities match add support for Latency
* and PHY.
*/
- if (ep->stream)
- ep->id = bt_bap_stream_config(ep->stream, &ep->qos, ep->caps,
- config_cb, ep);
- else
- ep->stream = bt_bap_config(ep->data->bap, ep->lpac, ep->rpac,
- &ep->qos, ep->caps,
- config_cb, ep);
+ if (!ep->stream)
+ ep->stream = bt_bap_stream_new(ep->data->bap, ep->lpac,
+ ep->rpac, &ep->qos, ep->caps);
- if (!ep->stream) {
+ ep->id = bt_bap_stream_config(ep->stream, &ep->qos, ep->caps,
+ config_cb, ep);
+ if (!ep->id) {
DBG("Unable to config stream");
free(ep->caps);
ep->caps = NULL;
@@ -604,15 +602,13 @@ static void bap_config(void *data, void *user_data)
/* TODO: Check if stream capabilities match add support for Latency
* and PHY.
*/
- if (ep->stream)
- ep->id = bt_bap_stream_config(ep->stream, &ep->qos, ep->caps,
- config_cb, ep);
- else
- ep->stream = bt_bap_config(ep->data->bap, ep->lpac, ep->rpac,
- &ep->qos, ep->caps,
- config_cb, ep);
+ if (!ep->stream)
+ ep->stream = bt_bap_stream_new(ep->data->bap, ep->lpac,
+ ep->rpac, &ep->qos, ep->caps);
- if (!ep->stream) {
+ ep->id = bt_bap_stream_config(ep->stream, &ep->qos, ep->caps,
+ config_cb, ep);
+ if (!ep->id) {
DBG("Unable to config stream");
util_iov_free(ep->caps, 1);
ep->caps = NULL;
diff --git a/src/shared/bap.c b/src/shared/bap.c
index 3ebcd81f16c1..952b7be260ab 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -4176,18 +4176,15 @@ int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
return 0;
}
-struct bt_bap_stream *bt_bap_config(struct bt_bap *bap,
+struct bt_bap_stream *bt_bap_stream_new(struct bt_bap *bap,
struct bt_bap_pac *lpac,
struct bt_bap_pac *rpac,
struct bt_bap_qos *pqos,
- struct iovec *data,
- bt_bap_stream_func_t func,
- void *user_data)
+ struct iovec *data)
{
struct bt_bap_stream *stream;
struct bt_bap_endpoint *ep;
struct match_pac match;
- int id;
if (!bap || !bap->rdb || queue_isempty(bap->remote_eps))
return NULL;
@@ -4244,15 +4241,6 @@ struct bt_bap_stream *bt_bap_config(struct bt_bap *bap,
if (!stream)
stream = bap_stream_new(bap, ep, lpac, rpac, data, true);
- id = bt_bap_stream_config(stream, pqos, data, func, user_data);
- if (!id) {
- DBG(bap, "Unable to config stream");
- queue_remove(bap->streams, stream);
- ep->stream = NULL;
- free(stream);
- return NULL;
- }
-
return stream;
}
diff --git a/src/shared/bap.h b/src/shared/bap.h
index 47a15636ca22..bd13abef9ce5 100644
--- a/src/shared/bap.h
+++ b/src/shared/bap.h
@@ -190,13 +190,11 @@ void *bt_bap_pac_get_user_data(struct bt_bap_pac *pac);
int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
bt_bap_pac_select_t func, void *user_data);
-struct bt_bap_stream *bt_bap_config(struct bt_bap *bap,
+struct bt_bap_stream *bt_bap_stream_new(struct bt_bap *bap,
struct bt_bap_pac *lpac,
struct bt_bap_pac *rpac,
struct bt_bap_qos *pqos,
- struct iovec *data,
- bt_bap_stream_func_t func,
- void *user_data);
+ struct iovec *data);
struct bt_bap *bt_bap_stream_get_session(struct bt_bap_stream *stream);
uint8_t bt_bap_stream_get_state(struct bt_bap_stream *stream);
--
2.39.2
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=726232
---Test result---
Test Summary:
CheckPatch FAIL 1.17 seconds
GitLint PASS 0.54 seconds
BuildEll PASS 27.16 seconds
BluezMake PASS 991.34 seconds
MakeCheck PASS 12.02 seconds
MakeDistcheck PASS 149.60 seconds
CheckValgrind PASS 246.65 seconds
CheckSmatch PASS 328.80 seconds
bluezmakeextell PASS 98.79 seconds
IncrementalBuild PASS 1726.64 seconds
ScanBuild PASS 1027.73 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,2/2] bap: Fix crash on unexpected disconnect
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#103:
by 0x495BFAE: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.7200.3)
/github/workspace/src/src/13157900.patch total: 0 errors, 1 warnings, 92 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13157900.patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
Regards,
Linux Bluetooth
Hello:
This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[email protected]>:
On Thu, 2 Mar 2023 12:57:57 -0800 you wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> This uses bt_gatt_client_idle_register to track when instance is ready
> instead of using a dedicated queue to track requests.
>
> Fixes: https://github.com/bluez/bluez/issues/485
>
> [...]
Here is the summary with links:
- [BlueZ,1/2] shared/bap: Make use of bt_gatt_client_idle_register
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=07bd8e3a720a
- [BlueZ,2/2] bap: Fix crash on unexpected disconnect
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=57f15616abde
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html