Bluetooth: MPL: Replace busy bool with atomic

Replace the busy boolean flag with an atomic value.
This prevents any race conditions with the MPL implementation.
Modifies where the new atomic value is set and cleared
so that initialization gets to finish before allowing
any reads.

Due to how the MPL is structured, and how a select cannot
be rejected from OTS, this does not give a perfect solution.
Ideally we need a separate object per OTS object, rather than
a shared one, and/or the OTS implemenation would allow
us to reject a select if the object is not currently
available or ready.

This commit does not fix the above issues, as that is a
larger undertaking.

Signed-off-by: Emil Gydesen <[email protected]>

Author: Thalley

subsys/bluetooth/audio/mpl.c

@@ -24,6 +24,7 @@
 #include <zephyr/logging/log.h>
 #include <zephyr/net_buf.h>
 #include <zephyr/sys/__assert.h>
+#include <zephyr/sys/atomic.h>
 #include <zephyr/sys/util.h>
 #include <zephyr/sys/time_units.h>
 #include <zephyr/sys/util_macro.h>
@@ -279,6 +280,12 @@ enum mpl_objects {
 	MPL_OBJ_SEARCH_RESULTS,
 };
 
+enum mpl_obj_flag {
+	MPL_OBJ_FLAG_BUSY,
+
+	MPL_OBJ_FLAG_NUM_FLAGS, /* keep as last */
+};
+
 /* The active object */
 /* Only a single object is selected or being added (active) at a time. */
 /* And, except for the icon object, all objects can be created dynamically. */
@@ -287,8 +294,6 @@ struct obj_t {
 	/* ID of the currently selected object*/
 	uint64_t selected_id;
 
-	bool busy;
-
 	/* Type of object being added, e.g. MPL_OBJ_ICON */
 	uint8_t add_type;
 
@@ -302,15 +307,16 @@ struct obj_t {
 		struct mpl_group *add_group;
 	};
 	struct net_buf_simple *content;
+
+	ATOMIC_DEFINE(flags, MPL_OBJ_FLAG_NUM_FLAGS);
 };
 
 static struct obj_t obj = {
 	.selected_id = 0,
 	.add_type = MPL_OBJ_NONE,
-	.busy = false,
 	.add_track = NULL,
 	.add_group = NULL,
-	.content = NET_BUF_SIMPLE(CONFIG_BT_MPL_MAX_OBJ_SIZE)
+	.content = NET_BUF_SIMPLE(CONFIG_BT_MPL_MAX_OBJ_SIZE),
 };
 
 /* Set up content buffer for the icon object */
@@ -475,13 +481,6 @@ static int add_icon_object(struct mpl_mediaplayer *pl)
 	const struct bt_uuid *icon_type = BT_UUID_OTS_TYPE_MPL_ICON;
 	static char *icon_name = "Icon";
 
-	if (obj.busy) {
-		/* TODO: Can there be a collision between select and internal */
-		/* activities, like adding new objects? */
-		LOG_ERR("Object busy");
-		return 0;
-	}
-	obj.busy = true;
 	obj.add_type = MPL_OBJ_ICON;
 	obj.desc = &created_desc;
 
@@ -496,7 +495,6 @@ static int add_icon_object(struct mpl_mediaplayer *pl)
 	ret = bt_ots_obj_add(bt_mcs_get_ots(), &add_param);
 	if (ret < 0) {
 		LOG_WRN("Unable to add icon object, error %d", ret);
-		obj.busy = false;
 
 		return ret;
 	}
@@ -512,11 +510,6 @@ static int add_current_track_segments_object(struct mpl_mediaplayer *pl)
 	struct bt_ots_obj_created_desc created_desc = {};
 	const struct bt_uuid *segs_type = BT_UUID_OTS_TYPE_TRACK_SEGMENT;
 
-	if (obj.busy) {
-		LOG_ERR("Object busy");
-		return 0;
-	}
-	obj.busy = true;
 	obj.add_type = MPL_OBJ_TRACK_SEGMENTS;
 	obj.desc = &created_desc;
 
@@ -531,7 +524,6 @@ static int add_current_track_segments_object(struct mpl_mediaplayer *pl)
 	ret = bt_ots_obj_add(bt_mcs_get_ots(), &add_param);
 	if (ret < 0) {
 		LOG_WRN("Unable to add track segments object: %d", ret);
-		obj.busy = false;
 
 		return ret;
 	}
@@ -547,17 +539,11 @@ static int add_track_object(struct mpl_track *track)
 	const struct bt_uuid *track_type = BT_UUID_OTS_TYPE_TRACK;
 	int ret;
 
-	if (obj.busy) {
-		LOG_ERR("Object busy");
-		return 0;
-	}
 	if (!track) {
 		LOG_ERR("No track");
 		return -EINVAL;
 	}
 
-	obj.busy = true;
-
 	obj.add_type = MPL_OBJ_TRACK;
 	obj.add_track = track;
 	obj.desc = &created_desc;
@@ -573,7 +559,6 @@ static int add_track_object(struct mpl_track *track)
 	ret = bt_ots_obj_add(bt_mcs_get_ots(), &add_param);
 	if (ret < 0) {
 		LOG_WRN("Unable to add track object: %d", ret);
-		obj.busy = false;
 
 		return ret;
 	}
@@ -589,11 +574,6 @@ static int add_parent_group_object(struct mpl_mediaplayer *pl)
 	struct bt_ots_obj_created_desc created_desc = {};
 	const struct bt_uuid *group_type = BT_UUID_OTS_TYPE_GROUP;
 
-	if (obj.busy) {
-		LOG_ERR("Object busy");
-		return 0;
-	}
-	obj.busy = true;
 	obj.add_type = MPL_OBJ_PARENT_GROUP;
 	obj.desc = &created_desc;
 
@@ -608,7 +588,6 @@ static int add_parent_group_object(struct mpl_mediaplayer *pl)
 	ret = bt_ots_obj_add(bt_mcs_get_ots(), &add_param);
 	if (ret < 0) {
 		LOG_WRN("Unable to add parent group object");
-		obj.busy = false;
 
 		return ret;
 	}
@@ -624,18 +603,11 @@ static int add_group_object(struct mpl_group *group)
 	const struct bt_uuid *group_type = BT_UUID_OTS_TYPE_GROUP;
 	int ret;
 
-	if (obj.busy) {
-		LOG_ERR("Object busy");
-		return 0;
-	}
-
 	if (!group) {
 		LOG_ERR("No group");
 		return -EINVAL;
 	}
 
-	obj.busy = true;
-
 	obj.add_type = MPL_OBJ_GROUP;
 	obj.add_group = group;
 	obj.desc = &created_desc;
@@ -651,7 +623,6 @@ static int add_group_object(struct mpl_group *group)
 	ret = bt_ots_obj_add(bt_mcs_get_ots(), &add_param);
 	if (ret < 0) {
 		LOG_WRN("Unable to add group object: %d", ret);
-		obj.busy = false;
 
 		return ret;
 	}
@@ -729,13 +700,12 @@ static int on_obj_deleted(struct bt_ots *ots, struct bt_conn *conn,
 static void on_obj_selected(struct bt_ots *ots, struct bt_conn *conn,
 			    uint64_t id)
 {
-	if (obj.busy) {
+	if (atomic_test_and_set_bit(obj.flags, MPL_OBJ_FLAG_BUSY)) {
 		/* TODO: Can there be a collision between select and internal */
 		/* activities, like adding new objects? */
 		LOG_ERR("Object busy - select not performed");
 		return;
 	}
-	obj.busy = true;
 
 	LOG_DBG_OBJ_ID("Object Id selected: ", id);
 
@@ -764,18 +734,20 @@ static void on_obj_selected(struct bt_ots *ots, struct bt_conn *conn,
 		(void)setup_group_object(media_player.group);
 	} else {
 		LOG_ERR("Unknown Object ID");
-		obj.busy = false;
+		atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
 		return;
 	}
 
 	obj.selected_id = id;
-	obj.busy = false;
+	atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
 }
 
 static int on_obj_created(struct bt_ots *ots, struct bt_conn *conn, uint64_t id,
 			  const struct bt_ots_obj_add_param *add_param,
 			  struct bt_ots_obj_created_desc *created_desc)
 {
+	/* Objects are always created locally so we do not need to check for MPL_OBJ_FLAG_BUSY */
+
 	LOG_DBG_OBJ_ID("Object Id created: ", id);
 
 	*created_desc = *obj.desc;
@@ -830,24 +802,19 @@ static int on_obj_created(struct bt_ots *ots, struct bt_conn *conn, uint64_t id,
 		LOG_DBG("Unknown Object ID");
 	}
 
-	if (obj.add_type == MPL_OBJ_NONE) {
-		obj.busy = false;
-	}
 	return 0;
 }
 
-
 static ssize_t on_object_send(struct bt_ots *ots, struct bt_conn *conn,
 			      uint64_t id, void **data, size_t len,
 			      off_t offset)
 {
-	if (obj.busy) {
+	if (atomic_test_and_set_bit(obj.flags, MPL_OBJ_FLAG_BUSY)) {
 		/* TODO: Can there be a collision between select and internal */
 		/* activities, like adding new objects? */
 		LOG_ERR("Object busy");
-		return 0;
+		return -EBUSY;
 	}
-	obj.busy = true;
 
 	if (IS_ENABLED(CONFIG_BT_MPL_LOG_LEVEL_DBG)) {
 		char t[BT_OTS_OBJ_ID_STR_LEN];
@@ -857,20 +824,20 @@ static ssize_t on_object_send(struct bt_ots *ots, struct bt_conn *conn,
 
 	if (id != obj.selected_id) {
 		LOG_ERR("Read from unselected object");
-		obj.busy = false;
-		return 0;
+		atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
+		return -EINVAL;
 	}
 
 	if (!data) {
 		LOG_DBG("Read complete");
-		obj.busy = false;
+		atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
 		return 0;
 	}
 
 	if (offset >= obj.content->len) {
 		LOG_DBG("Offset too large");
-		obj.busy = false;
-		return 0;
+		atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
+		return -EINVAL;
 	}
 
 	if (IS_ENABLED(CONFIG_BT_MPL_LOG_LEVEL_DBG)) {
@@ -880,7 +847,8 @@ static ssize_t on_object_send(struct bt_ots *ots, struct bt_conn *conn,
 	}
 
 	*data = &obj.content->data[offset];
-	obj.busy = false;
+	atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
+
 	return MIN(len, obj.content->len - offset);
 }
 
@@ -2357,14 +2325,29 @@ int media_proxy_pl_init(void)
 	 */
 #ifdef CONFIG_BT_MCS
 #ifdef CONFIG_BT_MPL_OBJECTS
+	/* The test here is arguably needed as the objects cannot be accessed before bt_mcs_init is
+	 * called, but the set is to avoid the objects being accessed before properly initialized
+	 */
+	if (atomic_test_and_set_bit(obj.flags, MPL_OBJ_FLAG_BUSY)) {
+		LOG_ERR("Object busy");
+		return -EBUSY;
+	}
+
 	ret = bt_mcs_init(&ots_cbs);
+	if (ret < 0) {
+		LOG_ERR("Could not init MCS: %d", ret);
+		atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
+
+		return ret;
+	}
 #else
 	ret = bt_mcs_init(NULL);
-#endif /* CONFIG_BT_MPL_OBJECTS */
 	if (ret < 0) {
 		LOG_ERR("Could not init MCS: %d", ret);
 		return ret;
 	}
+#endif  /* CONFIG_BT_MPL_OBJECTS */
+	/* TODO: If anything below fails we should unregister MCS */
 #else
 	LOG_WRN("MCS not configured");
 #endif /* CONFIG_BT_MCS */
@@ -2380,13 +2363,15 @@ int media_proxy_pl_init(void)
 	ret = add_icon_object(&media_player);
 	if (ret < 0) {
 		LOG_ERR("Unable to add icon object, error %d", ret);
+		atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
 		return ret;
 	}
 
 	/* Add all tracks and groups to OTS */
 	ret = add_group_and_track_objects(&media_player);
 	if (ret < 0) {
 		LOG_ERR("Error adding tracks and groups to OTS, error %d", ret);
+		atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
 		return ret;
 	}
 
@@ -2396,8 +2381,11 @@ int media_proxy_pl_init(void)
 	ret = add_current_track_segments_object(&media_player);
 	if (ret < 0) {
 		LOG_ERR("Error adding Track Segments Object to OTS, error %d", ret);
+		atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
 		return ret;
 	}
+
+	atomic_clear_bit(obj.flags, MPL_OBJ_FLAG_BUSY);
 #endif /* CONFIG_BT_MPL_OBJECTS */
 
 	/* Set up the calls structure */