Bluetooth: VOCS: Replace bools with atomic

Replace the booleans used by the VOCS client to use
an atomic value instead.

The flags are modified to be used in a way that prevents
race conditions.

Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

subsys/bluetooth/audio/vocs_client.c

@@ -44,9 +44,8 @@ static struct bt_vocs_client *lookup_vocs_by_handle(struct bt_conn *conn, uint16
 
 	for (int i = 0; i < ARRAY_SIZE(insts); i++) {
 		if (insts[i].conn == conn &&
-		    insts[i].active &&
-		    insts[i].start_handle <= handle &&
-		    insts[i].end_handle >= handle) {
+		    atomic_test_bit(insts[i].flags, BT_VOCS_CLIENT_FLAG_ACTIVE) &&
+		    insts[i].start_handle <= handle && insts[i].end_handle >= handle) {
 			return &insts[i];
 		}
 	}
@@ -134,7 +133,7 @@ static uint8_t vocs_client_read_offset_state_cb(struct bt_conn *conn, uint8_t er
 	}
 
 	LOG_DBG("Inst %p: err: 0x%02X", inst, err);
-	inst->busy = false;
+	atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 
 	if (cb_err) {
 		LOG_DBG("Offset state read failed: %d", err);
@@ -175,7 +174,7 @@ static uint8_t vocs_client_read_location_cb(struct bt_conn *conn, uint8_t err,
 	}
 
 	LOG_DBG("Inst %p: err: 0x%02X", inst, err);
-	inst->busy = false;
+	atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 
 	if (cb_err) {
 		LOG_DBG("Offset state read failed: %d", err);
@@ -226,7 +225,7 @@ static uint8_t internal_read_volume_offset_state_cb(struct bt_conn *conn, uint8_
 				inst->state.change_counter);
 
 			/* clear busy flag to reuse function */
-			inst->busy = false;
+			atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 			write_err = bt_vocs_client_state_set(inst, inst->cp.offset);
 			if (write_err) {
 				cb_err = BT_ATT_ERR_UNLIKELY;
@@ -242,7 +241,7 @@ static uint8_t internal_read_volume_offset_state_cb(struct bt_conn *conn, uint8_
 	}
 
 	if (cb_err) {
-		inst->busy = false;
+		atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 
 		if (inst->cb && inst->cb->set_offset) {
 			inst->cb->set_offset(&inst->vocs, err);
@@ -272,7 +271,8 @@ static void vocs_client_write_vocs_cp_cb(struct bt_conn *conn, uint8_t err,
 	 * change counter has been read, we restart the applications write request. If it fails
 	 * the second time, we return an error to the application.
 	 */
-	if (cb_err == BT_VOCS_ERR_INVALID_COUNTER && inst->cp_retried) {
+	if (cb_err == BT_VOCS_ERR_INVALID_COUNTER &&
+	    atomic_test_bit(inst->flags, BT_VOCS_CLIENT_FLAG_CP_RETRIED)) {
 		cb_err = BT_ATT_ERR_UNLIKELY;
 	} else if (cb_err == BT_VOCS_ERR_INVALID_COUNTER && inst->state_handle) {
 		LOG_DBG("Invalid change counter. Reading volume offset state from server.");
@@ -281,18 +281,19 @@ static void vocs_client_write_vocs_cp_cb(struct bt_conn *conn, uint8_t err,
 		inst->read_params.handle_count = 1;
 		inst->read_params.single.handle = inst->state_handle;
 
+		atomic_set_bit(inst->flags, BT_VOCS_CLIENT_FLAG_CP_RETRIED);
+
 		cb_err = bt_gatt_read(conn, &inst->read_params);
 		if (cb_err) {
 			LOG_WRN("Could not read Volume offset state: %d", cb_err);
 		} else {
-			inst->cp_retried = true;
 			/* Wait for read callback */
 			return;
 		}
 	}
 
-	inst->busy = false;
-	inst->cp_retried = false;
+	atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_CP_RETRIED);
+	atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 
 	if (inst->cb && inst->cb->set_offset) {
 		inst->cb->set_offset(&inst->vocs, cb_err);
@@ -315,7 +316,7 @@ static uint8_t vocs_client_read_output_desc_cb(struct bt_conn *conn, uint8_t err
 	}
 
 	LOG_DBG("Inst %p: err: 0x%02X", inst, err);
-	inst->busy = false;
+	atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 
 	if (cb_err) {
 		LOG_DBG("Description read failed: %d", err);
@@ -358,7 +359,7 @@ static uint8_t vocs_discover_func(struct bt_conn *conn, const struct bt_gatt_att
 
 	if (!attr) {
 		LOG_DBG("Discovery complete for VOCS %p", inst);
-		inst->busy = false;
+		atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 		(void)memset(params, 0, sizeof(*params));
 
 		if (inst->cb && inst->cb->discover) {
@@ -393,7 +394,7 @@ static uint8_t vocs_discover_func(struct bt_conn *conn, const struct bt_gatt_att
 				sub_params = &inst->location_sub_params;
 			}
 			if (chrc->properties & BT_GATT_CHRC_WRITE_WITHOUT_RESP) {
-				inst->location_writable = true;
+				atomic_set_bit(inst->flags, BT_VOCS_CLIENT_FLAG_LOC_WRITABLE);
 			}
 		} else if (!bt_uuid_cmp(chrc->uuid, BT_UUID_VOCS_CONTROL)) {
 			LOG_DBG("Control point");
@@ -405,7 +406,7 @@ static uint8_t vocs_discover_func(struct bt_conn *conn, const struct bt_gatt_att
 				sub_params = &inst->desc_sub_params;
 			}
 			if (chrc->properties & BT_GATT_CHRC_WRITE_WITHOUT_RESP) {
-				inst->desc_writable = true;
+				atomic_set_bit(inst->flags, BT_VOCS_CLIENT_FLAG_DESC_WRITABLE);
 			}
 		}
 
@@ -455,8 +456,8 @@ int bt_vocs_client_state_get(struct bt_vocs_client *inst)
 		return -EINVAL;
 	}
 
-	if (inst->busy) {
-		LOG_DBG("Handle not set");
+	if (atomic_test_and_set_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY)) {
+		LOG_DBG("Instance is busy");
 		return -EBUSY;
 	}
 
@@ -466,8 +467,8 @@ int bt_vocs_client_state_get(struct bt_vocs_client *inst)
 	inst->read_params.single.offset = 0U;
 
 	err = bt_gatt_read(inst->conn, &inst->read_params);
-	if (!err) {
-		inst->busy = true;
+	if (err != 0) {
+		atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 	}
 
 	return err;
@@ -493,13 +494,16 @@ int bt_vocs_client_location_set(struct bt_vocs_client *inst, uint32_t location)
 	if (!inst->location_handle) {
 		LOG_DBG("Handle not set");
 		return -EINVAL;
-	} else if (inst->busy) {
-		return -EBUSY;
-	} else if (!inst->location_writable) {
+	} else if (!atomic_test_bit(inst->flags, BT_VOCS_CLIENT_FLAG_LOC_WRITABLE)) {
 		LOG_DBG("Location is not writable on peer service instance");
 		return -EPERM;
+	} else if (atomic_test_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY)) {
+		LOG_DBG("Instance is busy");
+		return -EBUSY;
 	}
 
+	/* When using write without response we do not set the busy flag */
+
 	return bt_gatt_write_without_response(inst->conn,
 					      inst->location_handle,
 					      &location, sizeof(location),
@@ -523,7 +527,8 @@ int bt_vocs_client_location_get(struct bt_vocs_client *inst)
 	if (!inst->location_handle) {
 		LOG_DBG("Handle not set");
 		return -EINVAL;
-	} else if (inst->busy) {
+	} else if (atomic_test_and_set_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY)) {
+		LOG_DBG("Instance is busy");
 		return -EBUSY;
 	}
 
@@ -533,8 +538,8 @@ int bt_vocs_client_location_get(struct bt_vocs_client *inst)
 	inst->read_params.single.offset = 0U;
 
 	err = bt_gatt_read(inst->conn, &inst->read_params);
-	if (!err) {
-		inst->busy = true;
+	if (err != 0) {
+		atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 	}
 
 	return err;
@@ -562,7 +567,8 @@ int bt_vocs_client_state_set(struct bt_vocs_client *inst, int16_t offset)
 	if (!inst->control_handle) {
 		LOG_DBG("Handle not set");
 		return -EINVAL;
-	} else if (inst->busy) {
+	} else if (atomic_test_and_set_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY)) {
+		LOG_DBG("Instance is busy");
 		return -EBUSY;
 	}
 
@@ -577,8 +583,8 @@ int bt_vocs_client_state_set(struct bt_vocs_client *inst, int16_t offset)
 	inst->write_params.func = vocs_client_write_vocs_cp_cb;
 
 	err = bt_gatt_write(inst->conn, &inst->write_params);
-	if (!err) {
-		inst->busy = true;
+	if (err != 0) {
+		atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 	}
 
 	return err;
@@ -601,7 +607,8 @@ int bt_vocs_client_description_get(struct bt_vocs_client *inst)
 	if (!inst->desc_handle) {
 		LOG_DBG("Handle not set");
 		return -EINVAL;
-	} else if (inst->busy) {
+	} else if (atomic_test_and_set_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY)) {
+		LOG_DBG("Instance is busy");
 		return -EBUSY;
 	}
 
@@ -611,8 +618,8 @@ int bt_vocs_client_description_get(struct bt_vocs_client *inst)
 	inst->read_params.single.offset = 0U;
 
 	err = bt_gatt_read(inst->conn, &inst->read_params);
-	if (!err) {
-		inst->busy = true;
+	if (err != 0) {
+		atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 	}
 
 	return err;
@@ -634,13 +641,16 @@ int bt_vocs_client_description_set(struct bt_vocs_client *inst,
 	if (!inst->desc_handle) {
 		LOG_DBG("Handle not set");
 		return -EINVAL;
-	} else if (inst->busy) {
-		return -EBUSY;
-	} else if (!inst->desc_writable) {
+	} else if (!atomic_test_bit(inst->flags, BT_VOCS_CLIENT_FLAG_DESC_WRITABLE)) {
 		LOG_DBG("Description is not writable on peer service instance");
 		return -EPERM;
+	} else if (atomic_test_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY)) {
+		LOG_DBG("Instance is busy");
+		return -EBUSY;
 	}
 
+	/* When using write without response we do not set the busy flag */
+
 	return bt_gatt_write_without_response(inst->conn,
 					      inst->desc_handle,
 					      description,
@@ -650,9 +660,8 @@ int bt_vocs_client_description_set(struct bt_vocs_client *inst,
 struct bt_vocs *bt_vocs_client_free_instance_get(void)
 {
 	for (int i = 0; i < ARRAY_SIZE(insts); i++) {
-		if (!insts[i].active) {
+		if (!atomic_test_and_set_bit(insts[i].flags, BT_VOCS_CLIENT_FLAG_ACTIVE)) {
 			insts[i].vocs.client_instance = true;
-			insts[i].active = true;
 			return &insts[i].vocs;
 		}
 	}
@@ -689,9 +698,11 @@ int bt_vocs_client_conn_get(const struct bt_vocs *vocs, struct bt_conn **conn)
 static void vocs_client_reset(struct bt_vocs_client *inst)
 {
 	memset(&inst->state, 0, sizeof(inst->state));
-	inst->location_writable = 0;
+	atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_LOC_WRITABLE);
+	atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_DESC_WRITABLE);
+	atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_CP_RETRIED);
+
 	inst->location = 0;
-	inst->desc_writable = 0;
 	inst->start_handle = 0;
 	inst->end_handle = 0;
 	inst->state_handle = 0;
@@ -733,12 +744,10 @@ int bt_vocs_discover(struct bt_conn *conn, struct bt_vocs *vocs,
 
 	inst = CONTAINER_OF(vocs, struct bt_vocs_client, vocs);
 
-	CHECKIF(!inst->active) {
+	if (!atomic_test_bit(inst->flags, BT_VOCS_CLIENT_FLAG_ACTIVE)) {
 		LOG_DBG("Inactive instance");
 		return -EINVAL;
-	}
-
-	if (inst->busy) {
+	} else if (atomic_test_and_set_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY)) {
 		LOG_DBG("Instance is busy");
 		return -EBUSY;
 	}
@@ -752,10 +761,9 @@ int bt_vocs_discover(struct bt_conn *conn, struct bt_vocs *vocs,
 	inst->discover_params.func = vocs_discover_func;
 
 	err = bt_gatt_discover(conn, &inst->discover_params);
-	if (err) {
+	if (err != 0) {
 		LOG_DBG("Discover failed (err %d)", err);
-	} else {
-		inst->busy = true;
+		atomic_clear_bit(inst->flags, BT_VOCS_CLIENT_FLAG_BUSY);
 	}
 
 	return err;

subsys/bluetooth/audio/vocs_internal.h

@@ -42,13 +42,20 @@ struct bt_vocs {
 	bool client_instance;
 };
 
+enum bt_vocs_client_flag {
+	BT_VOCS_CLIENT_FLAG_BUSY,
+	BT_VOCS_CLIENT_FLAG_CP_RETRIED,
+	BT_VOCS_CLIENT_FLAG_DESC_WRITABLE,
+	BT_VOCS_CLIENT_FLAG_LOC_WRITABLE,
+	BT_VOCS_CLIENT_FLAG_ACTIVE,
+
+	BT_VOCS_CLIENT_FLAG_NUM_FLAGS, /* keep as last */
+};
+
 struct bt_vocs_client {
 	struct bt_vocs vocs;
 	struct bt_vocs_state state;
-	bool location_writable;
 	uint32_t location;
-	bool desc_writable;
-	bool active;
 
 	uint16_t start_handle;
 	uint16_t end_handle;
@@ -59,15 +66,15 @@ struct bt_vocs_client {
 	struct bt_gatt_subscribe_params state_sub_params;
 	struct bt_gatt_subscribe_params location_sub_params;
 	struct bt_gatt_subscribe_params desc_sub_params;
-	bool cp_retried;
 
-	bool busy;
 	struct bt_vocs_control cp;
 	struct bt_gatt_write_params write_params;
 	struct bt_gatt_read_params read_params;
 	struct bt_vocs_cb *cb;
 	struct bt_gatt_discover_params discover_params;
 	struct bt_conn *conn;
+
+	ATOMIC_DEFINE(flags, BT_VOCS_CLIENT_FLAG_NUM_FLAGS);
 };
 
 enum bt_vocs_notify {