[PATCH BlueZ 0/1] bap: Replace global bcast_pa_requests with a queue for each adapter

[PATCH BlueZ 0/1] bap: Replace global bcast_pa_requests with a queue for each adapter

[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

Vlad Pruteanu (1):
  bap: Replace global bcast_pa_requests with a queue for each adapter

 profiles/audio/bap.c | 115 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 97 insertions(+), 18 deletions(-)

-- 
2.40.1

[PATCH BlueZ 1/1] bap: Replace global bcast_pa_requests with a queue for each adapter

[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

RE: bap: Replace global bcast_pa_requests with a queue for each adapter

[bluez.test.bot]

[-- 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=845470

---Test result---

Test Summary:
CheckPatch                    PASS      0.60 seconds
GitLint                       PASS      0.34 seconds
BuildEll                      PASS      24.78 seconds
BluezMake                     PASS      1718.54 seconds
MakeCheck                     PASS      13.52 seconds
MakeDistcheck                 PASS      176.30 seconds
CheckValgrind                 PASS      245.93 seconds
CheckSmatch                   PASS      348.62 seconds
bluezmakeextell               PASS      118.66 seconds
IncrementalBuild              PASS      1532.26 seconds
ScanBuild                     PASS      987.28 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ 1/1] bap: Replace global bcast_pa_requests with a queue for each adapter

[Luiz Augusto von Dentz]

Hi Vlad,

On Wed, Apr 17, 2024 at 10:26 AM Vlad Pruteanu <vlad.pruteanu@nxp.com> wrote:
>
> 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);

That doesn't really seems that different since it still using a global
queue, instead Id expected to have the pa_req stored inside the
bap_data and then have the bap_data set as user_data of the service so
if the service is removed, e.g. due to user stopping scanning, the
remove callback will be called for the service to cleanup and then we
can cleanup the pa_req as well.

>
>         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);

Like Ive explained above this is probably harder than just having the
pa_req store directly into bap_data and then the bap_data be stored as
user_data of the service thus avoiding this sort of lookup pattern to
remove the service.

>         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);

I see know why you have a global queue, it is to store the pa_req on a
per adapter manner, while I agree we probably need a list containing
the data of each adapter that shall probably be name bap_adapter and
then the queue adapters, bap_data shall have bap_adapter pointer
rather that btd_adapter, than this becomes a lot more readable and you
don't have to lookups other than just adding and removing pa_req to
data->adapter->pa_q.

>         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
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/1] bap: Replace global bcast_pa_requests with a queue for each adapter

[Luiz Augusto von Dentz]

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

[PATCH BlueZ 1/1] bap: Replace global bcast_pa_requests with a queue for each adapter

[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