* [PATCH BlueZ 0/1] bap: Replace global bcast_pa_requests with a queue for each adapter
@ 2024-04-08 14:27 Vlad Pruteanu
2024-04-08 14:27 ` [PATCH BlueZ 1/1] " Vlad Pruteanu
0 siblings, 1 reply; 5+ messages in thread
From: Vlad Pruteanu @ 2024-04-08 14:27 UTC (permalink / raw
To: linux-bluetooth
Cc: claudia.rosu, mihai-octavian.urzica, silviu.barbulescu,
iulia.tanasescu, andrei.istodorescu, luiz.dentz, Vlad Pruteanu
This patch replaces the old global implementation of the bcast_pa_requests
queue with one that stores the queue per adapter. The pa_timer has also
been modified to be per adapter. The timer is now stopped when the queue is
empty. The bcast_pa_requests queue, along with the pa_timer_id are now
stored in the bap_data structure. Each adapter already has a coresponding
entry in the sessions queue. Operations on the old bcast_pa_requests have
been modified to be made on the appropriate bap_data entry.
The bap_bcast_remove function has also been updated to remove from the
queue entries of devices that were freed.
Vlad Pruteanu (1):
bap: Replace global bcast_pa_requests with a queue for each adapter
profiles/audio/bap.c | 109 +++++++++++++++++++++++++++++++------------
1 file changed, 79 insertions(+), 30 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH BlueZ 1/1] bap: Replace global bcast_pa_requests with a queue for each adapter
2024-04-08 14:27 [PATCH BlueZ 0/1] bap: Replace global bcast_pa_requests with a queue for each adapter Vlad Pruteanu
@ 2024-04-08 14:27 ` Vlad Pruteanu
2024-04-08 16:03 ` bluez.test.bot
2024-04-08 17:27 ` [PATCH BlueZ 1/1] " Luiz Augusto von Dentz
0 siblings, 2 replies; 5+ messages in thread
From: Vlad Pruteanu @ 2024-04-08 14:27 UTC (permalink / raw
To: linux-bluetooth
Cc: claudia.rosu, mihai-octavian.urzica, silviu.barbulescu,
iulia.tanasescu, andrei.istodorescu, luiz.dentz, Vlad Pruteanu
This patch replaces the old global implementation of the bcast_pa_requests
queue with one that stores the queue per adapter. The pa_timer has also
been modified to be per adapter. The timer is now stopped when the queue is
empty. The bcast_pa_requests queue, along with the pa_timer_id are now
stored in the bap_data structure. Each adapter already has a coresponding
entry in the sessions queue. Operations on the old bcast_pa_requests have
been modified to be made on the appropriate bap_data entry.
The bap_bcast_remove function has also been updated to remove from the
queue entries of devices that were freed.
---
profiles/audio/bap.c | 109 +++++++++++++++++++++++++++++++------------
1 file changed, 79 insertions(+), 30 deletions(-)
diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index db0af7e7c..82c6cf313 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -107,6 +107,8 @@ struct bap_data {
struct queue *snks;
struct queue *bcast;
struct queue *streams;
+ struct queue *bcast_pa_requests;
+ unsigned int pa_timer_id;
GIOChannel *listen_io;
int selecting;
void *user_data;
@@ -127,8 +129,6 @@ struct bap_bcast_pa_req {
};
static struct queue *sessions;
-static struct queue *bcast_pa_requests;
-static unsigned int pa_timer_id;
/* Structure holding the parameters for Periodic Advertisement create sync.
* The full QOS is populated at the time the user selects and endpoint and
@@ -965,15 +965,25 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
return NULL;
}
+static bool match_adapter_entry(const void *data, const void *match_data)
+{
+ const struct bap_data *bdata = data;
+ const struct btd_adapter *adapter = match_data;
+
+ return (bdata->user_data == adapter) && (bdata->device == NULL);
+}
+
static void iso_bcast_confirm_cb(GIOChannel *io, GError *err, void *user_data)
{
struct bap_bcast_pa_req *req = user_data;
struct bap_setup *setup = req->data.setup;
+ struct bap_data *bap_data = queue_find(sessions,
+ match_adapter_entry, setup->ep->data->adapter);
int fd;
DBG("BIG Sync completed");
- queue_remove(bcast_pa_requests, req);
+ queue_remove(bap_data->bcast_pa_requests, req);
/* This device is no longer needed */
btd_service_connecting_complete(setup->ep->data->service, 0);
@@ -1109,6 +1119,7 @@ static void iso_pa_sync_confirm_cb(GIOChannel *io, void *user_data)
GError *err = NULL;
struct bap_bcast_pa_req *pa_req = user_data;
struct bap_data *data = btd_service_get_user_data(pa_req->data.service);
+ struct bt_bap *bap = data->bap;
struct bt_iso_base base;
struct bt_iso_qos qos;
@@ -1130,12 +1141,13 @@ static void iso_pa_sync_confirm_cb(GIOChannel *io, void *user_data)
g_io_channel_unref(data->listen_io);
g_io_channel_shutdown(io, TRUE, NULL);
data->listen_io = NULL;
- queue_remove(bcast_pa_requests, pa_req);
+ data = queue_find(sessions, match_adapter_entry, data->adapter);
+ queue_remove(data->bcast_pa_requests, pa_req);
/* Analyze received BASE data and create remote media endpoints for each
* BIS matching our capabilities
*/
- parse_base(data->bap, &base, bap_debug);
+ parse_base(bap, &base, bap_debug);
}
static bool match_data_bap_data(const void *data, const void *match_data)
@@ -2015,25 +2027,37 @@ static void pa_and_big_sync(struct bap_bcast_pa_req *req);
static gboolean pa_idle_timer(gpointer user_data)
{
- struct bap_bcast_pa_req *req = user_data;
+ struct bap_data *bap_data = user_data;
+ struct bap_bcast_pa_req *req =
+ queue_peek_head(bap_data->bcast_pa_requests);
bool in_progress = FALSE;
+ /* If the queue is empty, stop the timer. It will be started,later on,
+ * if any new requests arrive.
+ */
+ if (req == NULL) {
+ /* Set the adapter's pa_timer id to 0. This will later be
+ * used to check if the timer is on or off.
+ */
+ bap_data->pa_timer_id = 0;
+ /* Stop the adapter's pa_timer by returning FALSE */
+ return FALSE;
+ }
+
/* Handle timer if no request is in progress */
- queue_foreach(bcast_pa_requests, check_pa_req_in_progress,
+ queue_foreach(bap_data->bcast_pa_requests, check_pa_req_in_progress,
&in_progress);
if (in_progress == FALSE) {
- req = queue_peek_head(bcast_pa_requests);
- if (req != NULL)
- switch (req->type) {
- case BAP_PA_SHORT_REQ:
- DBG("do short lived PA Sync");
- short_lived_pa_sync(req);
- break;
- case BAP_PA_BIG_SYNC_REQ:
- DBG("do PA Sync and BIG Sync");
- pa_and_big_sync(req);
- break;
- }
+ switch (req->type) {
+ case BAP_PA_SHORT_REQ:
+ DBG("do short lived PA Sync");
+ short_lived_pa_sync(req);
+ break;
+ case BAP_PA_BIG_SYNC_REQ:
+ DBG("do PA Sync and BIG Sync");
+ pa_and_big_sync(req);
+ break;
+ }
}
return TRUE;
@@ -2043,15 +2067,25 @@ static void setup_accept_io_broadcast(struct bap_data *data,
struct bap_setup *setup)
{
struct bap_bcast_pa_req *pa_req = new0(struct bap_bcast_pa_req, 1);
+ struct bap_data *bap_data = queue_find(sessions,
+ match_adapter_entry, data->adapter);
+
+ /* Timer could be stopped if all the short lived requests were treated.
+ * Check the state of the timer and turn it on so that this requests
+ * can also be treated.
+ */
+ if (bap_data->pa_timer_id == 0)
+ bap_data->pa_timer_id = g_timeout_add_seconds(
+ PA_IDLE_TIMEOUT, pa_idle_timer, bap_data);
/* Add this request to the PA queue.
- * We don't need to check the queue here and the timer, as we cannot
- * have BAP_PA_BIG_SYNC_REQ before a short PA (BAP_PA_SHORT_REQ)
+ * We don't need to check the queue here, as we cannot have
+ * BAP_PA_BIG_SYNC_REQ before a short PA (BAP_PA_SHORT_REQ)
*/
pa_req->type = BAP_PA_BIG_SYNC_REQ;
pa_req->in_progress = FALSE;
pa_req->data.setup = setup;
- queue_push_tail(bcast_pa_requests, pa_req);
+ queue_push_tail(bap_data->bcast_pa_requests, pa_req);
}
static void setup_create_ucast_io(struct bap_data *data,
@@ -2878,6 +2912,8 @@ static int bap_bcast_probe(struct btd_service *service)
{
struct btd_device *device = btd_service_get_device(service);
struct btd_adapter *adapter = device_get_adapter(device);
+ struct bap_data *bap_data = queue_find(sessions,
+ match_adapter_entry, adapter);
struct bap_bcast_pa_req *pa_req =
new0(struct bap_bcast_pa_req, 1);
@@ -2886,12 +2922,10 @@ static int bap_bcast_probe(struct btd_service *service)
return -ENOTSUP;
}
- /* First time initialize the queue and start the idle timer */
- if (bcast_pa_requests == NULL) {
- bcast_pa_requests = queue_new();
- pa_timer_id = g_timeout_add_seconds(PA_IDLE_TIMEOUT,
- pa_idle_timer, NULL);
- }
+ /* Start the PA timer if it isn't active */
+ if (bap_data->pa_timer_id == 0)
+ bap_data->pa_timer_id = g_timeout_add_seconds(
+ PA_IDLE_TIMEOUT, pa_idle_timer, bap_data);
/* Enqueue this device advertisement so that we can do short-lived
*/
@@ -2899,17 +2933,31 @@ static int bap_bcast_probe(struct btd_service *service)
pa_req->type = BAP_PA_SHORT_REQ;
pa_req->in_progress = FALSE;
pa_req->data.service = service;
- queue_push_tail(bcast_pa_requests, pa_req);
+ queue_push_tail(bap_data->bcast_pa_requests, pa_req);
return 0;
}
+static bool remove_service(const void *data, const void *match_data)
+{
+ struct bap_bcast_pa_req *pa_req = (struct bap_bcast_pa_req *)data;
+
+ if (pa_req->type == BAP_PA_SHORT_REQ &&
+ pa_req->data.service == match_data)
+ return true;
+ return false;
+}
+
static void bap_bcast_remove(struct btd_service *service)
{
struct btd_device *device = btd_service_get_device(service);
- struct bap_data *data;
+ struct btd_adapter *adapter = device_get_adapter(device);
+ struct bap_data *data = queue_find(sessions,
+ match_adapter_entry, adapter);
char addr[18];
+ queue_remove_if(data->bcast_pa_requests, remove_service, service);
+
ba2str(device_get_address(device), addr);
DBG("%s", addr);
@@ -3035,6 +3083,7 @@ static int bap_adapter_probe(struct btd_profile *p,
data->bap = bt_bap_new(btd_gatt_database_get_db(database),
btd_gatt_database_get_db(database));
+ data->bcast_pa_requests = queue_new();
if (!data->bap) {
error("Unable to create BAP instance");
free(data);
--
2.40.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* RE: bap: Replace global bcast_pa_requests with a queue for each adapter
2024-04-08 14:27 ` [PATCH BlueZ 1/1] " Vlad Pruteanu
@ 2024-04-08 16:03 ` bluez.test.bot
2024-04-08 17:27 ` [PATCH BlueZ 1/1] " Luiz Augusto von Dentz
1 sibling, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2024-04-08 16:03 UTC (permalink / raw
To: linux-bluetooth, vlad.pruteanu
[-- Attachment #1: Type: text/plain, Size: 948 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=842492
---Test result---
Test Summary:
CheckPatch PASS 0.57 seconds
GitLint PASS 0.32 seconds
BuildEll PASS 24.95 seconds
BluezMake PASS 1693.79 seconds
MakeCheck PASS 13.53 seconds
MakeDistcheck PASS 176.18 seconds
CheckValgrind PASS 245.13 seconds
CheckSmatch PASS 352.83 seconds
bluezmakeextell PASS 118.30 seconds
IncrementalBuild PASS 1544.18 seconds
ScanBuild PASS 993.72 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH BlueZ 1/1] bap: Replace global bcast_pa_requests with a queue for each adapter
2024-04-08 14:27 ` [PATCH BlueZ 1/1] " Vlad Pruteanu
2024-04-08 16:03 ` bluez.test.bot
@ 2024-04-08 17:27 ` Luiz Augusto von Dentz
1 sibling, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2024-04-08 17:27 UTC (permalink / raw
To: Vlad Pruteanu
Cc: linux-bluetooth, claudia.rosu, mihai-octavian.urzica,
silviu.barbulescu, iulia.tanasescu, andrei.istodorescu
Hi Vlad,
On Mon, Apr 8, 2024 at 10:28 AM Vlad Pruteanu <vlad.pruteanu@nxp.com> wrote:
>
> This patch replaces the old global implementation of the bcast_pa_requests
> queue with one that stores the queue per adapter. The pa_timer has also
> been modified to be per adapter. The timer is now stopped when the queue is
> empty. The bcast_pa_requests queue, along with the pa_timer_id are now
> stored in the bap_data structure. Each adapter already has a coresponding
> entry in the sessions queue. Operations on the old bcast_pa_requests have
> been modified to be made on the appropriate bap_data entry.
>
> The bap_bcast_remove function has also been updated to remove from the
> queue entries of devices that were freed.
> ---
> profiles/audio/bap.c | 109 +++++++++++++++++++++++++++++++------------
> 1 file changed, 79 insertions(+), 30 deletions(-)
>
> diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
> index db0af7e7c..82c6cf313 100644
> --- a/profiles/audio/bap.c
> +++ b/profiles/audio/bap.c
> @@ -107,6 +107,8 @@ struct bap_data {
> struct queue *snks;
> struct queue *bcast;
> struct queue *streams;
> + struct queue *bcast_pa_requests;
> + unsigned int pa_timer_id;
> GIOChannel *listen_io;
> int selecting;
> void *user_data;
> @@ -127,8 +129,6 @@ struct bap_bcast_pa_req {
> };
>
> static struct queue *sessions;
> -static struct queue *bcast_pa_requests;
> -static unsigned int pa_timer_id;
>
> /* Structure holding the parameters for Periodic Advertisement create sync.
> * The full QOS is populated at the time the user selects and endpoint and
> @@ -965,15 +965,25 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
> return NULL;
> }
>
> +static bool match_adapter_entry(const void *data, const void *match_data)
> +{
> + const struct bap_data *bdata = data;
> + const struct btd_adapter *adapter = match_data;
> +
> + return (bdata->user_data == adapter) && (bdata->device == NULL);
> +}
> +
> static void iso_bcast_confirm_cb(GIOChannel *io, GError *err, void *user_data)
> {
> struct bap_bcast_pa_req *req = user_data;
> struct bap_setup *setup = req->data.setup;
> + struct bap_data *bap_data = queue_find(sessions,
> + match_adapter_entry, setup->ep->data->adapter);
These lookups sound suspicious, couldn't you just store the bap_data
in the pa_req?
> int fd;
>
> DBG("BIG Sync completed");
>
> - queue_remove(bcast_pa_requests, req);
> + queue_remove(bap_data->bcast_pa_requests, req);
>
> /* This device is no longer needed */
> btd_service_connecting_complete(setup->ep->data->service, 0);
> @@ -1109,6 +1119,7 @@ static void iso_pa_sync_confirm_cb(GIOChannel *io, void *user_data)
> GError *err = NULL;
> struct bap_bcast_pa_req *pa_req = user_data;
> struct bap_data *data = btd_service_get_user_data(pa_req->data.service);
> + struct bt_bap *bap = data->bap;
> struct bt_iso_base base;
> struct bt_iso_qos qos;
>
> @@ -1130,12 +1141,13 @@ static void iso_pa_sync_confirm_cb(GIOChannel *io, void *user_data)
> g_io_channel_unref(data->listen_io);
> g_io_channel_shutdown(io, TRUE, NULL);
> data->listen_io = NULL;
> - queue_remove(bcast_pa_requests, pa_req);
> + data = queue_find(sessions, match_adapter_entry, data->adapter);
> + queue_remove(data->bcast_pa_requests, pa_req);
Ditto.
>
> /* Analyze received BASE data and create remote media endpoints for each
> * BIS matching our capabilities
> */
> - parse_base(data->bap, &base, bap_debug);
> + parse_base(bap, &base, bap_debug);
> }
>
> static bool match_data_bap_data(const void *data, const void *match_data)
> @@ -2015,25 +2027,37 @@ static void pa_and_big_sync(struct bap_bcast_pa_req *req);
>
> static gboolean pa_idle_timer(gpointer user_data)
> {
> - struct bap_bcast_pa_req *req = user_data;
> + struct bap_data *bap_data = user_data;
> + struct bap_bcast_pa_req *req =
> + queue_peek_head(bap_data->bcast_pa_requests);
> bool in_progress = FALSE;
>
> + /* If the queue is empty, stop the timer. It will be started,later on,
> + * if any new requests arrive.
> + */
> + if (req == NULL) {
> + /* Set the adapter's pa_timer id to 0. This will later be
> + * used to check if the timer is on or off.
> + */
> + bap_data->pa_timer_id = 0;
> + /* Stop the adapter's pa_timer by returning FALSE */
> + return FALSE;
> + }
> +
> /* Handle timer if no request is in progress */
> - queue_foreach(bcast_pa_requests, check_pa_req_in_progress,
> + queue_foreach(bap_data->bcast_pa_requests, check_pa_req_in_progress,
> &in_progress);
> if (in_progress == FALSE) {
> - req = queue_peek_head(bcast_pa_requests);
> - if (req != NULL)
> - switch (req->type) {
> - case BAP_PA_SHORT_REQ:
> - DBG("do short lived PA Sync");
> - short_lived_pa_sync(req);
> - break;
> - case BAP_PA_BIG_SYNC_REQ:
> - DBG("do PA Sync and BIG Sync");
> - pa_and_big_sync(req);
> - break;
> - }
> + switch (req->type) {
> + case BAP_PA_SHORT_REQ:
> + DBG("do short lived PA Sync");
> + short_lived_pa_sync(req);
> + break;
> + case BAP_PA_BIG_SYNC_REQ:
> + DBG("do PA Sync and BIG Sync");
> + pa_and_big_sync(req);
> + break;
> + }
> }
>
> return TRUE;
> @@ -2043,15 +2067,25 @@ static void setup_accept_io_broadcast(struct bap_data *data,
> struct bap_setup *setup)
> {
> struct bap_bcast_pa_req *pa_req = new0(struct bap_bcast_pa_req, 1);
> + struct bap_data *bap_data = queue_find(sessions,
> + match_adapter_entry, data->adapter);
Wait, you do have bap_data already passed to this function, why do you
need to lookup for another?
> +
> + /* Timer could be stopped if all the short lived requests were treated.
> + * Check the state of the timer and turn it on so that this requests
> + * can also be treated.
> + */
> + if (bap_data->pa_timer_id == 0)
> + bap_data->pa_timer_id = g_timeout_add_seconds(
> + PA_IDLE_TIMEOUT, pa_idle_timer, bap_data);
>
> /* Add this request to the PA queue.
> - * We don't need to check the queue here and the timer, as we cannot
> - * have BAP_PA_BIG_SYNC_REQ before a short PA (BAP_PA_SHORT_REQ)
> + * We don't need to check the queue here, as we cannot have
> + * BAP_PA_BIG_SYNC_REQ before a short PA (BAP_PA_SHORT_REQ)
> */
> pa_req->type = BAP_PA_BIG_SYNC_REQ;
> pa_req->in_progress = FALSE;
> pa_req->data.setup = setup;
> - queue_push_tail(bcast_pa_requests, pa_req);
> + queue_push_tail(bap_data->bcast_pa_requests, pa_req);
> }
>
> static void setup_create_ucast_io(struct bap_data *data,
> @@ -2878,6 +2912,8 @@ static int bap_bcast_probe(struct btd_service *service)
> {
> struct btd_device *device = btd_service_get_device(service);
> struct btd_adapter *adapter = device_get_adapter(device);
> + struct bap_data *bap_data = queue_find(sessions,
> + match_adapter_entry, adapter);
Don't we store the bap_data into the server user_data?
> struct bap_bcast_pa_req *pa_req =
> new0(struct bap_bcast_pa_req, 1);
>
> @@ -2886,12 +2922,10 @@ static int bap_bcast_probe(struct btd_service *service)
> return -ENOTSUP;
> }
>
> - /* First time initialize the queue and start the idle timer */
> - if (bcast_pa_requests == NULL) {
> - bcast_pa_requests = queue_new();
> - pa_timer_id = g_timeout_add_seconds(PA_IDLE_TIMEOUT,
> - pa_idle_timer, NULL);
> - }
> + /* Start the PA timer if it isn't active */
> + if (bap_data->pa_timer_id == 0)
> + bap_data->pa_timer_id = g_timeout_add_seconds(
> + PA_IDLE_TIMEOUT, pa_idle_timer, bap_data);
>
> /* Enqueue this device advertisement so that we can do short-lived
> */
> @@ -2899,17 +2933,31 @@ static int bap_bcast_probe(struct btd_service *service)
> pa_req->type = BAP_PA_SHORT_REQ;
> pa_req->in_progress = FALSE;
> pa_req->data.service = service;
> - queue_push_tail(bcast_pa_requests, pa_req);
> + queue_push_tail(bap_data->bcast_pa_requests, pa_req);
>
> return 0;
> }
>
> +static bool remove_service(const void *data, const void *match_data)
> +{
> + struct bap_bcast_pa_req *pa_req = (struct bap_bcast_pa_req *)data;
> +
> + if (pa_req->type == BAP_PA_SHORT_REQ &&
> + pa_req->data.service == match_data)
> + return true;
> + return false;
> +}
> +
> static void bap_bcast_remove(struct btd_service *service)
> {
> struct btd_device *device = btd_service_get_device(service);
> - struct bap_data *data;
> + struct btd_adapter *adapter = device_get_adapter(device);
> + struct bap_data *data = queue_find(sessions,
> + match_adapter_entry, adapter);
Ditto.
> char addr[18];
>
> + queue_remove_if(data->bcast_pa_requests, remove_service, service);
Not really sure what this would be doing, aren't you supposed to clean
up if there is pa_req pending for the given service/device and stop
the pa_sync if it is in progress? It seems you just remove it from the
list but it is not free/cancel after this point.
> ba2str(device_get_address(device), addr);
> DBG("%s", addr);
>
> @@ -3035,6 +3083,7 @@ static int bap_adapter_probe(struct btd_profile *p,
>
> data->bap = bt_bap_new(btd_gatt_database_get_db(database),
> btd_gatt_database_get_db(database));
> + data->bcast_pa_requests = queue_new();
> if (!data->bap) {
> error("Unable to create BAP instance");
> free(data);
> --
> 2.40.1
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH BlueZ 1/1] bap: Replace global bcast_pa_requests with a queue for each adapter
@ 2024-04-17 14:25 Vlad Pruteanu
2024-04-17 16:03 ` bluez.test.bot
0 siblings, 1 reply; 5+ messages in thread
From: Vlad Pruteanu @ 2024-04-17 14:25 UTC (permalink / raw
To: linux-bluetooth
Cc: claudia.rosu, mihai-octavian.urzica, silviu.barbulescu,
iulia.tanasescu, andrei.istodorescu, luiz.dentz, Vlad Pruteanu
This patch replaces the current implementation that uses a global
bcast_pa_requests queue and instead creates a queue and a timer for
each adapter.
The queue for a specific adapter, the timer's id and the adapter's id
form a new struct called pa_timer_data. For each adapter a new instance
of this struct is created and inserted into the global queue, pa_timers.
Operations on the old bcast_pa_requests queue have been modified such
that:
1) based on the adapter_id, the correct entry in the pa_timers queue is
retrieved
2) the operation can be called on the bcast_pa_requests queue from the
resulting pa_timer_data instance
The timers are created and stopped on a per adapter basis. A timer is
stopped if the adapter's pa_req queue is empty. A pa_timer_id equal
to 0 means that the timer is stopped.
The bap_bcast_remove function has been updated to remove from the
pa_req queue entries of devices that were freed. pa_req that are already
in progress are treated by the bap_data_free function.
This patch also fixes a crash that occurs when a device is freed before
the pa_idle_timer handles it's entry in the pa_req queue. The following
log was obtained while running an Unicast setup:
==105052==ERROR: AddressSanitizer: heap-use-after-free on address
0x60400001c418 at pc 0x55775caf1846 bp 0x7ffc83d9fb90 sp 0x7ffc83d9fb80
READ of size 8 at 0x60400001c418 thread T0
0 0x55775caf1845 in btd_service_get_device src/service.c:325
1 0x55775ca03da2 in short_lived_pa_sync profiles/audio/bap.c:2693
2 0x55775ca03da2 in pa_idle_timer profiles/audio/bap.c:1996
---
profiles/audio/bap.c | 115 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 97 insertions(+), 18 deletions(-)
diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index db0af7e7c..df0c35a6f 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -126,9 +126,14 @@ struct bap_bcast_pa_req {
} data;
};
+struct pa_timer_data {
+ struct btd_adapter *adapter;
+ unsigned int pa_timer_id;
+ struct queue *bcast_pa_requests;
+};
+
static struct queue *sessions;
-static struct queue *bcast_pa_requests;
-static unsigned int pa_timer_id;
+static struct queue *pa_timers;
/* Structure holding the parameters for Periodic Advertisement create sync.
* The full QOS is populated at the time the user selects and endpoint and
@@ -965,15 +970,25 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
return NULL;
}
+static bool pa_timer_match_adapter(const void *data, const void *match_data)
+{
+ struct pa_timer_data *pa_timer = (struct pa_timer_data *)data;
+
+ return pa_timer->adapter == match_data;
+}
+
static void iso_bcast_confirm_cb(GIOChannel *io, GError *err, void *user_data)
{
struct bap_bcast_pa_req *req = user_data;
struct bap_setup *setup = req->data.setup;
+ struct pa_timer_data *pa_timer = queue_find(pa_timers,
+ pa_timer_match_adapter, setup->ep->data->adapter);
int fd;
DBG("BIG Sync completed");
- queue_remove(bcast_pa_requests, req);
+ queue_remove(pa_timer->bcast_pa_requests, req);
+ free(req);
/* This device is no longer needed */
btd_service_connecting_complete(setup->ep->data->service, 0);
@@ -1111,6 +1126,8 @@ static void iso_pa_sync_confirm_cb(GIOChannel *io, void *user_data)
struct bap_data *data = btd_service_get_user_data(pa_req->data.service);
struct bt_iso_base base;
struct bt_iso_qos qos;
+ struct pa_timer_data *pa_timer = queue_find(pa_timers,
+ pa_timer_match_adapter, data->adapter);
DBG("PA Sync done");
@@ -1130,8 +1147,8 @@ static void iso_pa_sync_confirm_cb(GIOChannel *io, void *user_data)
g_io_channel_unref(data->listen_io);
g_io_channel_shutdown(io, TRUE, NULL);
data->listen_io = NULL;
- queue_remove(bcast_pa_requests, pa_req);
-
+ queue_remove(pa_timer->bcast_pa_requests, pa_req);
+ free(pa_req);
/* Analyze received BASE data and create remote media endpoints for each
* BIS matching our capabilities
*/
@@ -2015,14 +2032,15 @@ static void pa_and_big_sync(struct bap_bcast_pa_req *req);
static gboolean pa_idle_timer(gpointer user_data)
{
- struct bap_bcast_pa_req *req = user_data;
+ struct pa_timer_data *pa_timer = user_data;
+ struct bap_bcast_pa_req *req;
bool in_progress = FALSE;
/* Handle timer if no request is in progress */
- queue_foreach(bcast_pa_requests, check_pa_req_in_progress,
+ queue_foreach(pa_timer->bcast_pa_requests, check_pa_req_in_progress,
&in_progress);
if (in_progress == FALSE) {
- req = queue_peek_head(bcast_pa_requests);
+ req = queue_peek_head(pa_timer->bcast_pa_requests);
if (req != NULL)
switch (req->type) {
case BAP_PA_SHORT_REQ:
@@ -2034,6 +2052,14 @@ static gboolean pa_idle_timer(gpointer user_data)
pa_and_big_sync(req);
break;
}
+ else {
+ /* pa_req queue is empty, stop the timer by returning
+ * FALSE and set the pa_timer_id to 0. This will later
+ * be used to check if the timer is active.
+ */
+ pa_timer->pa_timer_id = 0;
+ return FALSE;
+ }
}
return TRUE;
@@ -2043,15 +2069,25 @@ static void setup_accept_io_broadcast(struct bap_data *data,
struct bap_setup *setup)
{
struct bap_bcast_pa_req *pa_req = new0(struct bap_bcast_pa_req, 1);
+ struct pa_timer_data *pa_timer = queue_find(pa_timers,
+ pa_timer_match_adapter, data->adapter);
+
+ /* Timer could be stopped if all the short lived requests were treated.
+ * Check the state of the timer and turn it on so that this requests
+ * can also be treated.
+ */
+ if (pa_timer->pa_timer_id == 0)
+ pa_timer->pa_timer_id = g_timeout_add_seconds(
+ PA_IDLE_TIMEOUT, pa_idle_timer, pa_timer);
/* Add this request to the PA queue.
- * We don't need to check the queue here and the timer, as we cannot
- * have BAP_PA_BIG_SYNC_REQ before a short PA (BAP_PA_SHORT_REQ)
+ * We don't need to check the queue here, as we cannot have
+ * BAP_PA_BIG_SYNC_REQ before a short PA (BAP_PA_SHORT_REQ)
*/
pa_req->type = BAP_PA_BIG_SYNC_REQ;
pa_req->in_progress = FALSE;
pa_req->data.setup = setup;
- queue_push_tail(bcast_pa_requests, pa_req);
+ queue_push_tail(pa_timer->bcast_pa_requests, pa_req);
}
static void setup_create_ucast_io(struct bap_data *data,
@@ -2880,18 +2916,18 @@ static int bap_bcast_probe(struct btd_service *service)
struct btd_adapter *adapter = device_get_adapter(device);
struct bap_bcast_pa_req *pa_req =
new0(struct bap_bcast_pa_req, 1);
+ struct pa_timer_data *pa_timer =
+ queue_find(pa_timers, pa_timer_match_adapter, adapter);
if (!btd_adapter_has_exp_feature(adapter, EXP_FEAT_ISO_SOCKET)) {
error("BAP requires ISO Socket which is not enabled");
return -ENOTSUP;
}
- /* First time initialize the queue and start the idle timer */
- if (bcast_pa_requests == NULL) {
- bcast_pa_requests = queue_new();
- pa_timer_id = g_timeout_add_seconds(PA_IDLE_TIMEOUT,
- pa_idle_timer, NULL);
- }
+ /* Start the PA timer if it hasn't been started yet */
+ if (pa_timer->pa_timer_id == 0)
+ pa_timer->pa_timer_id = g_timeout_add_seconds(
+ PA_IDLE_TIMEOUT, pa_idle_timer, pa_timer);
/* Enqueue this device advertisement so that we can do short-lived
*/
@@ -2899,17 +2935,38 @@ static int bap_bcast_probe(struct btd_service *service)
pa_req->type = BAP_PA_SHORT_REQ;
pa_req->in_progress = FALSE;
pa_req->data.service = service;
- queue_push_tail(bcast_pa_requests, pa_req);
+ queue_push_tail(pa_timer->bcast_pa_requests, pa_req);
return 0;
}
+static bool match_service(const void *data, const void *match_data)
+{
+ struct bap_bcast_pa_req *pa_req = (struct bap_bcast_pa_req *)data;
+
+ if (pa_req->data.service == match_data)
+ return true;
+ return false;
+}
+
static void bap_bcast_remove(struct btd_service *service)
{
struct btd_device *device = btd_service_get_device(service);
+ struct btd_adapter *adapter = device_get_adapter(device);
+ struct pa_timer_data *pa_timer =
+ queue_find(pa_timers, pa_timer_match_adapter, adapter);
+ struct bap_bcast_pa_req *pa_req;
struct bap_data *data;
char addr[18];
+ /* Remove the corresponding entry from the pa_req queue. Any pa_req that
+ * are in progress will be stopped by bap_data_remove which calls
+ * bap_data_free.
+ */
+ pa_req = queue_remove_if(pa_timer->bcast_pa_requests,
+ match_service, service);
+ if (pa_req)
+ free(pa_req);
ba2str(device_get_address(device), addr);
DBG("%s", addr);
@@ -3021,6 +3078,7 @@ static int bap_adapter_probe(struct btd_profile *p,
struct btd_gatt_database *database = btd_adapter_get_database(adapter);
struct bap_data *data;
char addr[18];
+ struct pa_timer_data *pa_timer;
ba2str(btd_adapter_get_address(adapter), addr);
DBG("%s", addr);
@@ -3055,6 +3113,14 @@ static int bap_adapter_probe(struct btd_profile *p,
bt_bap_set_user_data(data->bap, adapter);
bap_data_set_user_data(data, adapter);
+
+ if (pa_timers == NULL)
+ pa_timers = queue_new();
+ pa_timer = new0(struct pa_timer_data, 1);
+ pa_timer->adapter = adapter;
+ pa_timer->bcast_pa_requests = queue_new();
+ queue_push_tail(pa_timers, pa_timer);
+
return 0;
}
@@ -3063,11 +3129,24 @@ static void bap_adapter_remove(struct btd_profile *p,
{
struct bap_data *data = queue_find(sessions, match_data_bap_data,
adapter);
+ struct pa_timer_data *pa_timer = queue_find(pa_timers,
+ pa_timer_match_adapter, adapter);
char addr[18];
ba2str(btd_adapter_get_address(adapter), addr);
DBG("%s", addr);
+ queue_remove_all(pa_timer->bcast_pa_requests,
+ NULL, NULL, free);
+ free(pa_timer->bcast_pa_requests);
+ queue_remove(pa_timers, pa_timer);
+ free(pa_timer);
+
+ if (queue_isempty(pa_timers)) {
+ queue_destroy(pa_timers, NULL);
+ pa_timers = NULL;
+ }
+
if (!data) {
error("BAP service not handled by profile");
return;
--
2.40.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-17 16:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-08 14:27 [PATCH BlueZ 0/1] bap: Replace global bcast_pa_requests with a queue for each adapter Vlad Pruteanu
2024-04-08 14:27 ` [PATCH BlueZ 1/1] " Vlad Pruteanu
2024-04-08 16:03 ` bluez.test.bot
2024-04-08 17:27 ` [PATCH BlueZ 1/1] " Luiz Augusto von Dentz
-- strict thread matches above, loose matches on Subject: below --
2024-04-17 14:25 Vlad Pruteanu
2024-04-17 16:03 ` bluez.test.bot
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 read-only IMAP folder(s) and NNTP newsgroup(s).