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

Commit Message

Pauli Virtanen Jan. 26, 2025, 5:47 p.m. UTC

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 sending another volume set request only after the previous has
either failed or generated a volume notification.

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 all these cases, we
exit the wait state correctly.
---
 src/shared/vcp.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

Comments

[e-mail] Jan. 26, 2025, 7:13 p.m. UTC | #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=928415

---Test result---

Test Summary:
CheckPatch                    PENDING   0.17 seconds
GitLint                       PENDING   0.23 seconds
BuildEll                      PASS      20.32 seconds
BluezMake                     PASS      1520.29 seconds
MakeCheck                     PASS      13.25 seconds
MakeDistcheck                 PASS      158.56 seconds
CheckValgrind                 PASS      213.16 seconds
CheckSmatch                   PASS      270.08 seconds
bluezmakeextell               PASS      98.28 seconds
IncrementalBuild              PENDING   0.27 seconds
ScanBuild                     PASS      856.85 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

Patch

diff --git a/src/shared/vcp.c b/src/shared/vcp.c
index 6b0f2f9db..f781b7b2b 100644
--- a/src/shared/vcp.c
+++ b/src/shared/vcp.c
@@ -203,6 +203,9 @@  struct bt_vcp {
 	uint8_t volume;
 	uint8_t volume_counter;
 
+	uint8_t pending_volume;
+	unsigned int pending_ops;
+
 	void *debug_data;
 	void *user_data;
 };
@@ -2003,6 +2006,19 @@  done:
 	return vcp;
 }
 
+static void vcp_set_pending_volume(struct bt_vcp *vcp)
+{
+	/* Send pending request if any */
+	if (vcp->pending_ops <= 1) {
+		vcp->pending_ops = 0;
+		return;
+	}
+	vcp->pending_ops = 0;
+
+	DBG(vcp, "set pending volume 0x%x", vcp->pending_volume);
+	bt_vcp_set_volume(vcp, vcp->pending_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 +2036,8 @@  static void vcp_vstate_notify(struct bt_vcp *vcp, uint16_t value_handle,
 
 	if (vcp->volume_changed)
 		vcp->volume_changed(vcp, vcp->volume);
+
+	vcp_set_pending_volume(vcp);
 }
 
 static void vcp_volume_cp_sent(bool success, uint8_t err, void *user_data)
@@ -2031,6 +2049,8 @@  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_pending_volume(vcp);
 	}
 }
 
@@ -2061,9 +2081,20 @@  static bool vcp_set_volume_client(struct bt_vcp *vcp, uint8_t volume)
 		return false;
 	}
 
-	vcp->volume = volume;
+	vcp->pending_volume = volume;
+	if (vcp->pending_ops) {
+		/* Wait for current operation to complete */
+		vcp->pending_ops++;
+		return true;
+	} else if (vcp->volume == vcp->pending_volume) {
+		/* Do not set to current value, as that doesn't generate
+		 * a notification
+		 */
+		return true;
+	}
+
 	req.op = BT_VCS_SET_ABSOLUTE_VOL;
-	req.vol_set = vcp->volume;
+	req.vol_set = vcp->pending_volume;
 	req.change_counter = vcp->volume_counter;
 
 	if (!bt_gatt_client_write_value(vcp->client, value_handle, (void *)&req,
@@ -2072,6 +2103,8 @@  static bool vcp_set_volume_client(struct bt_vcp *vcp, uint8_t volume)
 		DBG(vcp, "error writing volume");
 		return false;
 	}
+
+	vcp->pending_ops++;
 	return true;
 }