[PATCH BlueZ v3] shared/vcp: have only one volume change in flight at a time

[PATCH BlueZ v3] shared/vcp: have only one volume change in flight at a time

[Pauli Virtanen]

If bt_vcp_set_volume() is called again before the previous operation has
completed, the requests get the same change counter, and all except the
first one fail.

Fix by waiting until the current request completes, and if volume was
set again during waiting, send a new request with the latest pending
volume value. In this definition, bt_vcp_set_volume() will skip over
intermediate volume updates if they are done too rapidly.

Send only volume requests that change the value to a different one than
last notification we have seen: in this case the request either fails,
or succeeds and generates a new notification.  In theory this guarantees
we always exit waiting, but safeguard it with a timeout.
---

Notes:
    v3: add timeout, wait separately for reply and notify
    
        In theory from VCS v1.0.1 3.2.2.5 + 3.2.2 you expect that request
        that is setting volume to different value than that of the current
        change counter, can only either fail, or succeed and produce
        notification. But it's probably safer to have a timeout regardless,
        so that we're guaranteed to stop waiting.
    
        I also changed it to keep track of which of write response & volume
        notify have arrived, to avoid a race condition where notify from
        another client's volume change caused us to send another request
        before previous completed.
    
    v2: reset pending_ops when attaching, needs to be cleared on reconnect

 src/shared/vcp.c | 86 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 82 insertions(+), 4 deletions(-)

diff --git a/src/shared/vcp.c b/src/shared/vcp.c
index 6b0f2f9db..f0887ad62 100644
--- a/src/shared/vcp.c
+++ b/src/shared/vcp.c
@@ -32,6 +32,8 @@
 
 #define VCP_STEP_SIZE 1
 
+#define VCP_CLIENT_OP_TIMEOUT		2000
+
 #define VOCS_VOL_OFFSET_UPPER_LIMIT	 255
 #define VOCS_VOL_OFFSET_LOWER_LIMIT	-255
 
@@ -176,6 +178,14 @@ struct bt_vcp_notify {
 	void *user_data;
 };
 
+struct bt_vcp_client_op {
+	uint8_t volume;
+	bool resend;
+	bool wait_reply;
+	bool wait_notify;
+	unsigned int timeout_id;
+};
+
 struct bt_vcp {
 	int ref_count;
 	struct bt_vcp_db *ldb;
@@ -203,6 +213,8 @@ struct bt_vcp {
 	uint8_t volume;
 	uint8_t volume_counter;
 
+	struct bt_vcp_client_op pending_op;
+
 	void *debug_data;
 	void *user_data;
 };
@@ -395,6 +407,14 @@ static void vcp_remote_client_detached(void *data, void *user_data)
 	cb->detached(vcp, cb->user_data);
 }
 
+static void vcp_client_op_clear(struct bt_vcp_client_op *op)
+{
+	if (op->timeout_id)
+		timeout_remove(op->timeout_id);
+
+	memset(op, 0, sizeof(*op));
+}
+
 void bt_vcp_detach(struct bt_vcp *vcp)
 {
 	if (!queue_remove(sessions, vcp))
@@ -404,6 +424,8 @@ void bt_vcp_detach(struct bt_vcp *vcp)
 		bt_gatt_client_unref(vcp->client);
 		vcp->client = NULL;
 	}
+
+	vcp_client_op_clear(&vcp->pending_op);
 }
 
 static void vcp_db_free(void *data)
@@ -2003,6 +2025,22 @@ done:
 	return vcp;
 }
 
+static void vcp_set_volume_complete(struct bt_vcp *vcp)
+{
+	bool resend = vcp->pending_op.resend;
+	uint8_t volume = vcp->pending_op.volume;
+
+	vcp_client_op_clear(&vcp->pending_op);
+
+	/* If there were more volume set ops while waiting for the one that
+	 * completes, send request to set volume to the latest pending value.
+	 */
+	if (resend) {
+		DBG(vcp, "set pending volume 0x%x", volume);
+		bt_vcp_set_volume(vcp, volume);
+	}
+}
+
 static void vcp_vstate_notify(struct bt_vcp *vcp, uint16_t value_handle,
 				const uint8_t *value, uint16_t length,
 				void *user_data)
@@ -2020,6 +2058,10 @@ static void vcp_vstate_notify(struct bt_vcp *vcp, uint16_t value_handle,
 
 	if (vcp->volume_changed)
 		vcp->volume_changed(vcp, vcp->volume);
+
+	vcp->pending_op.wait_notify = false;
+	if (!vcp->pending_op.wait_reply)
+		vcp_set_volume_complete(vcp);
 }
 
 static void vcp_volume_cp_sent(bool success, uint8_t err, void *user_data)
@@ -2031,12 +2073,23 @@ static void vcp_volume_cp_sent(bool success, uint8_t err, void *user_data)
 			DBG(vcp, "setting volume failed: invalid counter");
 		else
 			DBG(vcp, "setting volume failed: error 0x%x", err);
+
+		vcp_set_volume_complete(vcp);
+	} else {
+		vcp->pending_op.wait_reply = false;
+		if (!vcp->pending_op.wait_notify)
+			vcp_set_volume_complete(vcp);
 	}
 }
 
-uint8_t bt_vcp_get_volume(struct bt_vcp *vcp)
+static bool vcp_set_volume_timeout(void *data)
 {
-	return vcp->volume;
+	struct bt_vcp *vcp = data;
+
+	DBG(vcp, "setting volume: timeout");
+	vcp->pending_op.timeout_id = 0;
+	vcp_set_volume_complete(vcp);
+	return false;
 }
 
 static bool vcp_set_volume_client(struct bt_vcp *vcp, uint8_t volume)
@@ -2061,9 +2114,24 @@ static bool vcp_set_volume_client(struct bt_vcp *vcp, uint8_t volume)
 		return false;
 	}
 
-	vcp->volume = volume;
+	/* If there is another set volume op in flight, just update the wanted
+	 * pending volume value. Req with the latest volume value is sent after
+	 * the current one completes. This may skip over some volume changes,
+	 * as it only sends a request for the final value.
+	 */
+	if (vcp->volume == volume) {
+		/* Do not set to current value, as that doesn't generate
+		 * a notification
+		 */
+		return true;
+	} else if (vcp->pending_op.timeout_id) {
+		vcp->pending_op.volume = volume;
+		vcp->pending_op.resend = true;
+		return true;
+	}
+
 	req.op = BT_VCS_SET_ABSOLUTE_VOL;
-	req.vol_set = vcp->volume;
+	req.vol_set = volume;
 	req.change_counter = vcp->volume_counter;
 
 	if (!bt_gatt_client_write_value(vcp->client, value_handle, (void *)&req,
@@ -2072,6 +2140,11 @@ static bool vcp_set_volume_client(struct bt_vcp *vcp, uint8_t volume)
 		DBG(vcp, "error writing volume");
 		return false;
 	}
+
+	vcp->pending_op.timeout_id = timeout_add(VCP_CLIENT_OP_TIMEOUT,
+					vcp_set_volume_timeout, vcp, NULL);
+	vcp->pending_op.wait_notify = true;
+	vcp->pending_op.wait_reply = true;
 	return true;
 }
 
@@ -2108,6 +2181,11 @@ bool bt_vcp_set_volume(struct bt_vcp *vcp, uint8_t volume)
 		return vcp_set_volume_server(vcp, volume);
 }
 
+uint8_t bt_vcp_get_volume(struct bt_vcp *vcp)
+{
+	return vcp->volume;
+}
+
 static void vcp_voffset_state_notify(struct bt_vcp *vcp, uint16_t value_handle,
 				const uint8_t *value, uint16_t length,
 				void *user_data)
-- 
2.48.1

RE: [BlueZ,v3] shared/vcp: have only one volume change in flight at a time

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1260 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=929711

---Test result---

Test Summary:
CheckPatch                    PENDING   0.20 seconds
GitLint                       PENDING   0.20 seconds
BuildEll                      PASS      20.27 seconds
BluezMake                     PASS      1521.80 seconds
MakeCheck                     PASS      13.40 seconds
MakeDistcheck                 PASS      157.27 seconds
CheckValgrind                 PASS      212.67 seconds
CheckSmatch                   PASS      268.29 seconds
bluezmakeextell               PASS      97.69 seconds
IncrementalBuild              PENDING   0.25 seconds
ScanBuild                     PASS      848.53 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ v3] shared/vcp: have only one volume change in flight at a time

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Sat,  1 Feb 2025 17:12:09 +0200 you wrote:
> If bt_vcp_set_volume() is called again before the previous operation has
> completed, the requests get the same change counter, and all except the
> first one fail.
> 
> Fix by waiting until the current request completes, and if volume was
> set again during waiting, send a new request with the latest pending
> volume value. In this definition, bt_vcp_set_volume() will skip over
> intermediate volume updates if they are done too rapidly.
> 
> [...]

Here is the summary with links:
  - [BlueZ,v3] shared/vcp: have only one volume change in flight at a time
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=e77884accdb2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html