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
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
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
Hi Vlad,
On Mon, Apr 8, 2024 at 10:28 AM Vlad Pruteanu <[email protected]> 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