[PATCH BlueZ v3 0/3] audio/avrcp: Determine Absolute Volume support from feature category 2

[PATCH BlueZ v3 0/3] audio/avrcp: Determine Absolute Volume support from feature category 2

[Marijn Suijten]

Changes since v2:

- New patch to guard target-less SetAbsoluteVolume calls with main.conf
  config;
- New patch to more strictly require category-2 _and_ peer CT/TG 1.4;
- Use main.conf to relax 1.4 version constraint for TGs registering
  AbsVol notifications;

v2: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2

I'd rather revert 179ccb936 ("avrcp: Set volume if volume changed event
is registered") entirely, paired with additional checks in common
codepaths for controller and target not being NULL when sending and
receiving control messages respectively (which then don't need to be
double-checked in the AbsVol codepaths for example).

Marijn Suijten (3):
  audio/avrcp: Guard SetAbsoluteVolume without target behind config
    value
  audio/avrcp: Only allow absolute volume call/event on category-2 peers
  audio/avrcp: Determine Absolute Volume support from feature category 2

 profiles/audio/avrcp.c | 53 ++++++++++++++++++++++++++++++++++++------
 src/btd.h              |  6 +++++
 src/main.c             | 21 +++++++++++++++++
 src/main.conf          | 12 ++++++++++
 4 files changed, 85 insertions(+), 7 deletions(-)

--
2.39.2

[PATCH BlueZ v3 1/3] audio/avrcp: Guard SetAbsoluteVolume without target behind config value

[Marijn Suijten]

Commit 179ccb936 ("avrcp: Set volume if volume changed event is
registered") introduced a catch that allows SetAbsoluteVolume to be sent
to a remote device that does _not_ implement the AVRCP TG profile.  This
is strange as the TG role is required to be able to send commands to the
peer, but the commit must have been applied to the tree for a reason.

We discussed in [1] that workarounds for dubious peers and software
stacks should be guarded behind a config entry in main.conf, so this
starts out by introducing a new [AVRCP] category to to it that will
later be extended with other workarounds.

[1]: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2
---
 profiles/audio/avrcp.c | 12 +++++++++---
 src/btd.h              |  5 +++++
 src/main.c             | 13 +++++++++++++
 src/main.conf          |  6 ++++++
 4 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 80f34c7a7..5e6322916 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -48,6 +48,7 @@
 #include "src/dbus-common.h"
 #include "src/shared/timeout.h"
 #include "src/shared/util.h"
+#include "src/btd.h"
 
 #include "avctp.h"
 #include "avrcp.h"
@@ -4577,9 +4578,14 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
 								&volume);
 	}
 
-	if (!session->controller && !avrcp_event_registered(session,
-					AVRCP_EVENT_VOLUME_CHANGED))
-		return -ENOTSUP;
+	if (btd_opts.avrcp.set_absolute_volume_without_target) {
+		if (!session->controller && !avrcp_event_registered(session,
+						AVRCP_EVENT_VOLUME_CHANGED))
+			return -ENOTSUP;
+	} else {
+		if (!session->controller || session->controller->version < 0x0104)
+			return -ENOTSUP;
+	}
 
 	memset(buf, 0, sizeof(buf));
 
diff --git a/src/btd.h b/src/btd.h
index 42cffcde4..31c04a990 100644
--- a/src/btd.h
+++ b/src/btd.h
@@ -97,6 +97,10 @@ struct btd_avdtp_opts {
 	uint8_t  stream_mode;
 };
 
+struct btd_avrcp_opts {
+	gboolean set_absolute_volume_without_target;
+};
+
 struct btd_advmon_opts {
 	uint8_t		rssi_sampling_period;
 };
@@ -136,6 +140,7 @@ struct btd_opts {
 	enum mps_mode_t	mps;
 
 	struct btd_avdtp_opts avdtp;
+	struct btd_avrcp_opts avrcp;
 
 	uint8_t		key_size;
 
diff --git a/src/main.c b/src/main.c
index 99d9c508f..92f74e381 100644
--- a/src/main.c
+++ b/src/main.c
@@ -152,6 +152,11 @@ static const char *avdtp_options[] = {
 	NULL
 };
 
+static const char *avrcp_options[] = {
+	"SetAbsoluteVolumeWithoutTarget",
+	NULL
+};
+
 static const char *advmon_options[] = {
 	"RSSISamplingPeriod",
 	NULL
@@ -167,6 +172,7 @@ static const struct group_table {
 	{ "Policy",	policy_options },
 	{ "GATT",	gatt_options },
 	{ "AVDTP",	avdtp_options },
+	{ "AVRCP",	avrcp_options },
 	{ "AdvMon",	advmon_options },
 	{ }
 };
@@ -975,6 +981,13 @@ static void parse_config(GKeyFile *config)
 		g_free(str);
 	}
 
+	boolean = g_key_file_get_boolean(config, "AVRCP",
+						"SetAbsoluteVolumeWithoutTarget", &err);
+	if (err)
+		g_clear_error(&err);
+	else
+		btd_opts.avrcp.set_absolute_volume_without_target = boolean;
+
 	val = g_key_file_get_integer(config, "AdvMon", "RSSISamplingPeriod",
 									&err);
 	if (err) {
diff --git a/src/main.conf b/src/main.conf
index f187c9aaa..ca00ed03e 100644
--- a/src/main.conf
+++ b/src/main.conf
@@ -271,6 +271,12 @@
 # streaming: Use L2CAP Streaming Mode
 #StreamMode = basic
 
+[AVRCP]
+# Allow SetAbsoluteVolume calls to a peer device that
+# does not advertise the AVRCP remote control target
+# profile.
+#SetAbsoluteVolumeWithoutTarget = false
+
 [Policy]
 #
 # The ReconnectUUIDs defines the set of remote services that should try
-- 
2.39.2

[PATCH BlueZ v3 2/3] audio/avrcp: Only allow absolute volume call/event on category-2 peers

[Marijn Suijten]

Restrict the use of SetAbsoluteVolume and EVENT_VOLUME_CHANGED to peers
with at least AVRCP version 1.4 and AVRCP_FEATURE_CATEGORY_2 on their
respective target or controller profiles.
---
 profiles/audio/avrcp.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 5e6322916..c16f9cfef 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -1757,6 +1757,16 @@ static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
 	if (len != 1)
 		goto err;
 
+	/**
+	 * The controller on the remote end is only allowed to call SetAbsoluteVolume
+	 * on our target if it's at least version 1.4 and a category-2 device.
+	 */
+	if (!session->target || session->target->version < 0x0104 ||
+			!(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
+		error("Remote SetAbsoluteVolume rejected from non-category-2 peer");
+		goto err;
+	}
+
 	volume = pdu->params[0] & 0x7F;
 
 	media_transport_update_device_volume(session->dev, volume);
@@ -3728,6 +3738,16 @@ static void avrcp_volume_changed(struct avrcp *session,
 	struct avrcp_player *player = target_get_player(session);
 	int8_t volume;
 
+	/**
+	 * The target on the remote end is only allowed to reply to EVENT_VOLUME_CHANGED
+	 * on our controller if it's at least version 1.4 and a category-2 device.
+	 */
+	if (!session->controller || session->controller->version < 0x0104 ||
+			!(session->controller->features & AVRCP_FEATURE_CATEGORY_2)) {
+		error("Remote EVENT_VOLUME_CHANGED rejected from non-category-2 peer");
+		return;
+	}
+
 	volume = pdu->params[1] & 0x7F;
 
 	/* Always attempt to update the transport volume */
@@ -3981,7 +4001,7 @@ static gboolean avrcp_get_capabilities_resp(struct avctp *conn, uint8_t code,
 		case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
 		case AVRCP_EVENT_UIDS_CHANGED:
 		case AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED:
-			/* These events above requires a player */
+			/* These events above require a player */
 			if (!session->controller ||
 						!session->controller->player)
 				break;
@@ -4154,10 +4174,13 @@ static void target_init(struct avrcp *session)
 	if (target->version < 0x0104)
 		return;
 
+	if (target->features & AVRCP_FEATURE_CATEGORY_2)
+		session->supported_events |=
+				(1 << AVRCP_EVENT_VOLUME_CHANGED);
+
 	session->supported_events |=
 				(1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
-				(1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
-				(1 << AVRCP_EVENT_VOLUME_CHANGED);
+				(1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
 
 	/* Only check capabilities if controller is not supported */
 	if (session->controller == NULL)
@@ -4572,8 +4595,11 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
 		return -ENOTCONN;
 
 	if (notify) {
-		if (!session->target)
+		if (!session->target || session->target->version < 0x0104 ||
+				!(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
+			error("Can't send EVENT_VOLUME_CHANGED to non-category-2 peer");
 			return -ENOTSUP;
+		}
 		return avrcp_event(session, AVRCP_EVENT_VOLUME_CHANGED,
 								&volume);
 	}
@@ -4583,8 +4609,11 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
 						AVRCP_EVENT_VOLUME_CHANGED))
 			return -ENOTSUP;
 	} else {
-		if (!session->controller || session->controller->version < 0x0104)
+		if (!session->controller || session->controller->version < 0x0104 ||
+				!(session->controller->features & AVRCP_FEATURE_CATEGORY_2)) {
+			error("Can't send SetAbsoluteVolume to non-category-2 peer");
 			return -ENOTSUP;
+		}
 	}
 
 	memset(buf, 0, sizeof(buf));
-- 
2.39.2

[PATCH BlueZ v3 3/3] audio/avrcp: Determine Absolute Volume support from feature category 2

[Marijn Suijten]

The AVRCP spec (1.6.2) does not mention anything about a version
requirement for Absolute Volume, despite this feature only existing
since spec version 1.4.  Android reports a version of 1.3 [1] for its
"AVRCP remote" (CT) service and mentions in the comment above it itself
relies on feature bits rather than the exposed version.  As it stands
BlueZ requires at least version 1.4 making it unable to communicate
absolute volume levels with even the most recent Android phones running
Fluoride (have not checked the version on Gabeldorsche).

The spec states that supporting SetAbsoluteVolume and
EVENT_VOLUME_CHANGED are mandatory when feature level 2 is declared,
excluded otherwise.  This feature bit is set on Android and, when used
by this patch, allows for successfully communicating volume back and
forth despite the version theoretically being too low.

In order to not affect spec tests too much (which I doubt would catch
this, and should have otherwise pointed out that Android itself is out
of spec) this behaviour is guarded behind a config option in main.conf,
as discussed in [2].

[1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r28/bta/av/bta_av_main.cc#761
[2]: https://marc.info/?l=linux-bluetooth&m=163463497503113&w=2
---
 profiles/audio/avrcp.c | 16 ++++++++++------
 src/btd.h              |  1 +
 src/main.c             |  8 ++++++++
 src/main.conf          |  6 ++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index c16f9cfef..11f18f25d 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -1761,7 +1761,8 @@ static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
 	 * The controller on the remote end is only allowed to call SetAbsoluteVolume
 	 * on our target if it's at least version 1.4 and a category-2 device.
 	 */
-	if (!session->target || session->target->version < 0x0104 ||
+	if (!session->target ||
+			(session->target->version < 0x0104 && !btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct) ||
 			!(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
 		error("Remote SetAbsoluteVolume rejected from non-category-2 peer");
 		goto err;
@@ -4171,13 +4172,15 @@ static void target_init(struct avrcp *session)
 				(1 << AVRCP_EVENT_TRACK_REACHED_END) |
 				(1 << AVRCP_EVENT_SETTINGS_CHANGED);
 
-	if (target->version < 0x0104)
-		return;
-
-	if (target->features & AVRCP_FEATURE_CATEGORY_2)
+	/* Remote device supports receiving volume notifications */
+	if ((target->version >= 0x0104 || btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct) &&
+			target->features & AVRCP_FEATURE_CATEGORY_2)
 		session->supported_events |=
 				(1 << AVRCP_EVENT_VOLUME_CHANGED);
 
+	if (target->version < 0x0104)
+		return;
+
 	session->supported_events |=
 				(1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
 				(1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
@@ -4595,7 +4598,8 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
 		return -ENOTCONN;
 
 	if (notify) {
-		if (!session->target || session->target->version < 0x0104 ||
+		if (!session->target ||
+				(session->target->version < 0x0104 && !btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct) ||
 				!(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
 			error("Can't send EVENT_VOLUME_CHANGED to non-category-2 peer");
 			return -ENOTSUP;
diff --git a/src/btd.h b/src/btd.h
index 31c04a990..07d1d961f 100644
--- a/src/btd.h
+++ b/src/btd.h
@@ -99,6 +99,7 @@ struct btd_avdtp_opts {
 
 struct btd_avrcp_opts {
 	gboolean set_absolute_volume_without_target;
+	gboolean allow_volume_changed_on_pre_1_4_ct;
 };
 
 struct btd_advmon_opts {
diff --git a/src/main.c b/src/main.c
index 92f74e381..a2b81f940 100644
--- a/src/main.c
+++ b/src/main.c
@@ -154,6 +154,7 @@ static const char *avdtp_options[] = {
 
 static const char *avrcp_options[] = {
 	"SetAbsoluteVolumeWithoutTarget",
+	"AllowVolumeChangedOnPre1_4Controller",
 	NULL
 };
 
@@ -988,6 +989,13 @@ static void parse_config(GKeyFile *config)
 	else
 		btd_opts.avrcp.set_absolute_volume_without_target = boolean;
 
+	boolean = g_key_file_get_boolean(config, "AVRCP",
+						"AllowVolumeChangedOnPre1_4Controller", &err);
+	if (err)
+		g_clear_error(&err);
+	else
+		btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct = boolean;
+
 	val = g_key_file_get_integer(config, "AdvMon", "RSSISamplingPeriod",
 									&err);
 	if (err) {
diff --git a/src/main.conf b/src/main.conf
index ca00ed03e..286d092bf 100644
--- a/src/main.conf
+++ b/src/main.conf
@@ -277,6 +277,12 @@
 # profile.
 #SetAbsoluteVolumeWithoutTarget = false
 
+# Allow peer AVRCP controller with version 1.3 access to category-2 (absolute volume) features.
+# This is common for AOSP to not signal the desired minimum version of 1.4 while still supporting
+# absolute volume based on the feature category bit, as mentioned in the comment:
+# https://android.googlesource.com/platform/system/bt/+/android-12.0.0_r1/bta/av/bta_av_main.cc#621
+AllowVolumeChangedOnPre1_4Controller = true
+
 [Policy]
 #
 # The ReconnectUUIDs defines the set of remote services that should try
-- 
2.39.2

RE: audio/avrcp: Determine Absolute Volume support from feature category 2

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    FAIL      1.93 seconds
GitLint                       FAIL      1.27 seconds
BuildEll                      PASS      26.70 seconds
BluezMake                     PASS      851.96 seconds
MakeCheck                     PASS      11.17 seconds
MakeDistcheck                 PASS      150.52 seconds
CheckValgrind                 PASS      245.93 seconds
CheckSmatch                   PASS      329.03 seconds
bluezmakeextell               PASS      97.94 seconds
IncrementalBuild              FAIL      0.31 seconds
ScanBuild                     PASS      1013.02 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v3,1/3] audio/avrcp: Guard SetAbsoluteVolume without target behind config value
WARNING:UNKNOWN_COMMIT_ID: Unknown commit id '179ccb936', maybe rebased or not pulled?
#55: 
Commit 179ccb936 ("avrcp: Set volume if volume changed event is

WARNING:REPEATED_WORD: Possible repeated word: 'to'
#63: 
starts out by introducing a new [AVRCP] category to to it that will

WARNING:PREFER_LORE_ARCHIVE: Use lore.kernel.org archive links when possible - see https://lore.kernel.org/lists.html
#66: 
[1]: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2

WARNING:LONG_LINE: line length of 82 exceeds 80 columns
#98: FILE: profiles/audio/avrcp.c:4586:
+		if (!session->controller || session->controller->version < 0x0104)

WARNING:STATIC_CONST_CHAR_ARRAY: static const char * array should probably be static const char * const
#135: FILE: src/main.c:155:
+static const char *avrcp_options[] = {

WARNING:LONG_LINE: line length of 88 exceeds 80 columns
#156: FILE: src/main.c:985:
+						"SetAbsoluteVolumeWithoutTarget", &err);

/github/workspace/src/src/13170561.patch total: 0 errors, 6 warnings, 84 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13170561.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,v3,2/3] audio/avrcp: Only allow absolute volume call/event on category-2 peers
WARNING:LONG_LINE_COMMENT: line length of 85 exceeds 80 columns
#74: FILE: profiles/audio/avrcp.c:1761:
+	 * The controller on the remote end is only allowed to call SetAbsoluteVolume

WARNING:LONG_LINE: line length of 82 exceeds 80 columns
#78: FILE: profiles/audio/avrcp.c:1765:
+			!(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {

WARNING:LONG_LINE_STRING: line length of 84 exceeds 80 columns
#79: FILE: profiles/audio/avrcp.c:1766:
+		error("Remote SetAbsoluteVolume rejected from non-category-2 peer");

WARNING:LONG_LINE_COMMENT: line length of 88 exceeds 80 columns
#91: FILE: profiles/audio/avrcp.c:3742:
+	 * The target on the remote end is only allowed to reply to EVENT_VOLUME_CHANGED

WARNING:LONG_LINE_COMMENT: line length of 82 exceeds 80 columns
#92: FILE: profiles/audio/avrcp.c:3743:
+	 * on our controller if it's at least version 1.4 and a category-2 device.

WARNING:LONG_LINE: line length of 86 exceeds 80 columns
#95: FILE: profiles/audio/avrcp.c:3746:
+			!(session->controller->features & AVRCP_FEATURE_CATEGORY_2)) {

WARNING:LONG_LINE_STRING: line length of 87 exceeds 80 columns
#96: FILE: profiles/audio/avrcp.c:3747:
+		error("Remote EVENT_VOLUME_CHANGED rejected from non-category-2 peer");

WARNING:LONG_LINE: line length of 90 exceeds 80 columns
#134: FILE: profiles/audio/avrcp.c:4599:
+				!(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {

WARNING:LONG_LINE_STRING: line length of 88 exceeds 80 columns
#135: FILE: profiles/audio/avrcp.c:4600:
+			error("Can't send EVENT_VOLUME_CHANGED to non-category-2 peer");

WARNING:LONG_LINE: line length of 84 exceeds 80 columns
#146: FILE: profiles/audio/avrcp.c:4612:
+		if (!session->controller || session->controller->version < 0x0104 ||

WARNING:LONG_LINE: line length of 94 exceeds 80 columns
#147: FILE: profiles/audio/avrcp.c:4613:
+				!(session->controller->features & AVRCP_FEATURE_CATEGORY_2)) {

WARNING:LONG_LINE_STRING: line length of 85 exceeds 80 columns
#148: FILE: profiles/audio/avrcp.c:4614:
+			error("Can't send SetAbsoluteVolume to non-category-2 peer");

/github/workspace/src/src/13170562.patch total: 0 errors, 12 warnings, 79 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13170562.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,v3,3/3] audio/avrcp: Determine Absolute Volume support from feature category 2
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#77: 
[1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r28/bta/av/bta_av_main.cc#761

WARNING:PREFER_LORE_ARCHIVE: Use lore.kernel.org archive links when possible - see https://lore.kernel.org/lists.html
#78: 
[2]: https://marc.info/?l=linux-bluetooth&m=163463497503113&w=2

WARNING:LONG_LINE: line length of 116 exceeds 80 columns
#96: FILE: profiles/audio/avrcp.c:1765:
+			(session->target->version < 0x0104 && !btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct) ||

WARNING:LONG_LINE: line length of 95 exceeds 80 columns
#109: FILE: profiles/audio/avrcp.c:4176:
+	if ((target->version >= 0x0104 || btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct) &&

WARNING:LONG_LINE: line length of 124 exceeds 80 columns
#126: FILE: profiles/audio/avrcp.c:4602:
+				(session->target->version < 0x0104 && !btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct) ||

WARNING:LONG_LINE: line length of 94 exceeds 80 columns
#159: FILE: src/main.c:993:
+						"AllowVolumeChangedOnPre1_4Controller", &err);

/github/workspace/src/src/13170563.patch total: 0 errors, 6 warnings, 76 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13170563.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,v3,1/3] audio/avrcp: Guard SetAbsoluteVolume without target behind config value

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (86>80): "[BlueZ,v3,1/3] audio/avrcp: Guard SetAbsoluteVolume without target behind config value"
[BlueZ,v3,2/3] audio/avrcp: Only allow absolute volume call/event on category-2 peers

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (85>80): "[BlueZ,v3,2/3] audio/avrcp: Only allow absolute volume call/event on category-2 peers"
[BlueZ,v3,3/3] audio/avrcp: Determine Absolute Volume support from feature category 2

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (85>80): "[BlueZ,v3,3/3] audio/avrcp: Determine Absolute Volume support from feature category 2"
23: B1 Line exceeds max length (103>80): "[1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r28/bta/av/bta_av_main.cc#761"
##############################
Test: IncrementalBuild - FAIL
Desc: Incremental build with the patches in the series
Output:

error: patch failed: src/main.c:167
error: src/main.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch


---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ v3 1/3] audio/avrcp: Guard SetAbsoluteVolume without target behind config value

[Luiz Augusto von Dentz]

Hi Marijn,

On Fri, Mar 10, 2023 at 4:39 PM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Commit 179ccb936 ("avrcp: Set volume if volume changed event is
> registered") introduced a catch that allows SetAbsoluteVolume to be sent
> to a remote device that does _not_ implement the AVRCP TG profile.  This
> is strange as the TG role is required to be able to send commands to the
> peer, but the commit must have been applied to the tree for a reason.
>
> We discussed in [1] that workarounds for dubious peers and software
> stacks should be guarded behind a config entry in main.conf, so this
> starts out by introducing a new [AVRCP] category to to it that will
> later be extended with other workarounds.
>
> [1]: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2
> ---
>  profiles/audio/avrcp.c | 12 +++++++++---
>  src/btd.h              |  5 +++++
>  src/main.c             | 13 +++++++++++++
>  src/main.conf          |  6 ++++++
>  4 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 80f34c7a7..5e6322916 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -48,6 +48,7 @@
>  #include "src/dbus-common.h"
>  #include "src/shared/timeout.h"
>  #include "src/shared/util.h"
> +#include "src/btd.h"
>
>  #include "avctp.h"
>  #include "avrcp.h"
> @@ -4577,9 +4578,14 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
>                                                                 &volume);
>         }
>
> -       if (!session->controller && !avrcp_event_registered(session,
> -                                       AVRCP_EVENT_VOLUME_CHANGED))
> -               return -ENOTSUP;
> +       if (btd_opts.avrcp.set_absolute_volume_without_target) {
> +               if (!session->controller && !avrcp_event_registered(session,
> +                                               AVRCP_EVENT_VOLUME_CHANGED))
> +                       return -ENOTSUP;
> +       } else {
> +               if (!session->controller || session->controller->version < 0x0104)
> +                       return -ENOTSUP;
> +       }
>
>         memset(buf, 0, sizeof(buf));
>
> diff --git a/src/btd.h b/src/btd.h
> index 42cffcde4..31c04a990 100644
> --- a/src/btd.h
> +++ b/src/btd.h
> @@ -97,6 +97,10 @@ struct btd_avdtp_opts {
>         uint8_t  stream_mode;
>  };
>
> +struct btd_avrcp_opts {
> +       gboolean set_absolute_volume_without_target;
> +};
> +
>  struct btd_advmon_opts {
>         uint8_t         rssi_sampling_period;
>  };
> @@ -136,6 +140,7 @@ struct btd_opts {
>         enum mps_mode_t mps;
>
>         struct btd_avdtp_opts avdtp;
> +       struct btd_avrcp_opts avrcp;
>
>         uint8_t         key_size;
>
> diff --git a/src/main.c b/src/main.c
> index 99d9c508f..92f74e381 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -152,6 +152,11 @@ static const char *avdtp_options[] = {
>         NULL
>  };
>
> +static const char *avrcp_options[] = {
> +       "SetAbsoluteVolumeWithoutTarget",
> +       NULL
> +};
> +
>  static const char *advmon_options[] = {
>         "RSSISamplingPeriod",
>         NULL
> @@ -167,6 +172,7 @@ static const struct group_table {
>         { "Policy",     policy_options },
>         { "GATT",       gatt_options },
>         { "AVDTP",      avdtp_options },
> +       { "AVRCP",      avrcp_options },
>         { "AdvMon",     advmon_options },
>         { }
>  };
> @@ -975,6 +981,13 @@ static void parse_config(GKeyFile *config)
>                 g_free(str);
>         }
>
> +       boolean = g_key_file_get_boolean(config, "AVRCP",
> +                                               "SetAbsoluteVolumeWithoutTarget", &err);
> +       if (err)
> +               g_clear_error(&err);
> +       else
> +               btd_opts.avrcp.set_absolute_volume_without_target = boolean;
> +
>         val = g_key_file_get_integer(config, "AdvMon", "RSSISamplingPeriod",
>                                                                         &err);
>         if (err) {
> diff --git a/src/main.conf b/src/main.conf
> index f187c9aaa..ca00ed03e 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -271,6 +271,12 @@
>  # streaming: Use L2CAP Streaming Mode
>  #StreamMode = basic
>
> +[AVRCP]
> +# Allow SetAbsoluteVolume calls to a peer device that
> +# does not advertise the AVRCP remote control target
> +# profile.
> +#SetAbsoluteVolumeWithoutTarget = false

Let's do just VolumeWithoutTarget and we should probably mention that
it would ignore the version as well.

> +
>  [Policy]
>  #
>  # The ReconnectUUIDs defines the set of remote services that should try
> --
> 2.39.2
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v3 2/3] audio/avrcp: Only allow absolute volume call/event on category-2 peers

[Luiz Augusto von Dentz]

Hi Marijn,

On Fri, Mar 10, 2023 at 4:39 PM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Restrict the use of SetAbsoluteVolume and EVENT_VOLUME_CHANGED to peers
> with at least AVRCP version 1.4 and AVRCP_FEATURE_CATEGORY_2 on their
> respective target or controller profiles.

Hmm, couldn't this actually make things even worse since we now are
checking the category as well? I know this is by the spec but as we
already are experiencing some stacks tend to deviate a lot from the
spec, perhaps it would have been better to introduce another config
option e.g. VolumeCategory = true so people can roll back to the old
behavior if their devices don't work well with this changes.

> ---
>  profiles/audio/avrcp.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 5e6322916..c16f9cfef 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -1757,6 +1757,16 @@ static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
>         if (len != 1)
>                 goto err;
>
> +       /**
> +        * The controller on the remote end is only allowed to call SetAbsoluteVolume
> +        * on our target if it's at least version 1.4 and a category-2 device.
> +        */
> +       if (!session->target || session->target->version < 0x0104 ||
> +                       !(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
> +               error("Remote SetAbsoluteVolume rejected from non-category-2 peer");
> +               goto err;
> +       }
> +
>         volume = pdu->params[0] & 0x7F;
>
>         media_transport_update_device_volume(session->dev, volume);
> @@ -3728,6 +3738,16 @@ static void avrcp_volume_changed(struct avrcp *session,
>         struct avrcp_player *player = target_get_player(session);
>         int8_t volume;
>
> +       /**
> +        * The target on the remote end is only allowed to reply to EVENT_VOLUME_CHANGED
> +        * on our controller if it's at least version 1.4 and a category-2 device.
> +        */
> +       if (!session->controller || session->controller->version < 0x0104 ||
> +                       !(session->controller->features & AVRCP_FEATURE_CATEGORY_2)) {
> +               error("Remote EVENT_VOLUME_CHANGED rejected from non-category-2 peer");
> +               return;
> +       }
> +
>         volume = pdu->params[1] & 0x7F;
>
>         /* Always attempt to update the transport volume */
> @@ -3981,7 +4001,7 @@ static gboolean avrcp_get_capabilities_resp(struct avctp *conn, uint8_t code,
>                 case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
>                 case AVRCP_EVENT_UIDS_CHANGED:
>                 case AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED:
> -                       /* These events above requires a player */
> +                       /* These events above require a player */
>                         if (!session->controller ||
>                                                 !session->controller->player)
>                                 break;
> @@ -4154,10 +4174,13 @@ static void target_init(struct avrcp *session)
>         if (target->version < 0x0104)
>                 return;
>
> +       if (target->features & AVRCP_FEATURE_CATEGORY_2)
> +               session->supported_events |=
> +                               (1 << AVRCP_EVENT_VOLUME_CHANGED);
> +
>         session->supported_events |=
>                                 (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> -                               (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> -                               (1 << AVRCP_EVENT_VOLUME_CHANGED);
> +                               (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
>
>         /* Only check capabilities if controller is not supported */
>         if (session->controller == NULL)
> @@ -4572,8 +4595,11 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
>                 return -ENOTCONN;
>
>         if (notify) {
> -               if (!session->target)
> +               if (!session->target || session->target->version < 0x0104 ||
> +                               !(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
> +                       error("Can't send EVENT_VOLUME_CHANGED to non-category-2 peer");
>                         return -ENOTSUP;
> +               }
>                 return avrcp_event(session, AVRCP_EVENT_VOLUME_CHANGED,
>                                                                 &volume);
>         }
> @@ -4583,8 +4609,11 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
>                                                 AVRCP_EVENT_VOLUME_CHANGED))
>                         return -ENOTSUP;
>         } else {
> -               if (!session->controller || session->controller->version < 0x0104)
> +               if (!session->controller || session->controller->version < 0x0104 ||
> +                               !(session->controller->features & AVRCP_FEATURE_CATEGORY_2)) {
> +                       error("Can't send SetAbsoluteVolume to non-category-2 peer");
>                         return -ENOTSUP;
> +               }
>         }
>
>         memset(buf, 0, sizeof(buf));
> --
> 2.39.2
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v3 3/3] audio/avrcp: Determine Absolute Volume support from feature category 2

[Luiz Augusto von Dentz]

Hi Marijn,

On Fri, Mar 10, 2023 at 4:39 PM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> The AVRCP spec (1.6.2) does not mention anything about a version
> requirement for Absolute Volume, despite this feature only existing
> since spec version 1.4.  Android reports a version of 1.3 [1] for its
> "AVRCP remote" (CT) service and mentions in the comment above it itself
> relies on feature bits rather than the exposed version.  As it stands
> BlueZ requires at least version 1.4 making it unable to communicate
> absolute volume levels with even the most recent Android phones running
> Fluoride (have not checked the version on Gabeldorsche).
>
> The spec states that supporting SetAbsoluteVolume and
> EVENT_VOLUME_CHANGED are mandatory when feature level 2 is declared,
> excluded otherwise.  This feature bit is set on Android and, when used
> by this patch, allows for successfully communicating volume back and
> forth despite the version theoretically being too low.
>
> In order to not affect spec tests too much (which I doubt would catch
> this, and should have otherwise pointed out that Android itself is out
> of spec) this behaviour is guarded behind a config option in main.conf,
> as discussed in [2].
>
> [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r28/bta/av/bta_av_main.cc#761
> [2]: https://marc.info/?l=linux-bluetooth&m=163463497503113&w=2
> ---
>  profiles/audio/avrcp.c | 16 ++++++++++------
>  src/btd.h              |  1 +
>  src/main.c             |  8 ++++++++
>  src/main.conf          |  6 ++++++
>  4 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index c16f9cfef..11f18f25d 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -1761,7 +1761,8 @@ static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
>          * The controller on the remote end is only allowed to call SetAbsoluteVolume
>          * on our target if it's at least version 1.4 and a category-2 device.
>          */
> -       if (!session->target || session->target->version < 0x0104 ||
> +       if (!session->target ||
> +                       (session->target->version < 0x0104 && !btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct) ||
>                         !(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
>                 error("Remote SetAbsoluteVolume rejected from non-category-2 peer");
>                 goto err;
> @@ -4171,13 +4172,15 @@ static void target_init(struct avrcp *session)
>                                 (1 << AVRCP_EVENT_TRACK_REACHED_END) |
>                                 (1 << AVRCP_EVENT_SETTINGS_CHANGED);
>
> -       if (target->version < 0x0104)
> -               return;
> -
> -       if (target->features & AVRCP_FEATURE_CATEGORY_2)
> +       /* Remote device supports receiving volume notifications */
> +       if ((target->version >= 0x0104 || btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct) &&
> +                       target->features & AVRCP_FEATURE_CATEGORY_2)
>                 session->supported_events |=
>                                 (1 << AVRCP_EVENT_VOLUME_CHANGED);
>
> +       if (target->version < 0x0104)
> +               return;
> +
>         session->supported_events |=
>                                 (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
>                                 (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
> @@ -4595,7 +4598,8 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
>                 return -ENOTCONN;
>
>         if (notify) {
> -               if (!session->target || session->target->version < 0x0104 ||
> +               if (!session->target ||
> +                               (session->target->version < 0x0104 && !btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct) ||
>                                 !(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
>                         error("Can't send EVENT_VOLUME_CHANGED to non-category-2 peer");
>                         return -ENOTSUP;
> diff --git a/src/btd.h b/src/btd.h
> index 31c04a990..07d1d961f 100644
> --- a/src/btd.h
> +++ b/src/btd.h
> @@ -99,6 +99,7 @@ struct btd_avdtp_opts {
>
>  struct btd_avrcp_opts {
>         gboolean set_absolute_volume_without_target;
> +       gboolean allow_volume_changed_on_pre_1_4_ct;
>  };
>
>  struct btd_advmon_opts {
> diff --git a/src/main.c b/src/main.c
> index 92f74e381..a2b81f940 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -154,6 +154,7 @@ static const char *avdtp_options[] = {
>
>  static const char *avrcp_options[] = {
>         "SetAbsoluteVolumeWithoutTarget",
> +       "AllowVolumeChangedOnPre1_4Controller",
>         NULL
>  };
>
> @@ -988,6 +989,13 @@ static void parse_config(GKeyFile *config)
>         else
>                 btd_opts.avrcp.set_absolute_volume_without_target = boolean;
>
> +       boolean = g_key_file_get_boolean(config, "AVRCP",
> +                                               "AllowVolumeChangedOnPre1_4Controller", &err);
> +       if (err)
> +               g_clear_error(&err);
> +       else
> +               btd_opts.avrcp.allow_volume_changed_on_pre_1_4_ct = boolean;
> +
>         val = g_key_file_get_integer(config, "AdvMon", "RSSISamplingPeriod",
>                                                                         &err);
>         if (err) {
> diff --git a/src/main.conf b/src/main.conf
> index ca00ed03e..286d092bf 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -277,6 +277,12 @@
>  # profile.
>  #SetAbsoluteVolumeWithoutTarget = false
>
> +# Allow peer AVRCP controller with version 1.3 access to category-2 (absolute volume) features.
> +# This is common for AOSP to not signal the desired minimum version of 1.4 while still supporting
> +# absolute volume based on the feature category bit, as mentioned in the comment:
> +# https://android.googlesource.com/platform/system/bt/+/android-12.0.0_r1/bta/av/bta_av_main.cc#621
> +AllowVolumeChangedOnPre1_4Controller = true

This is too long to my liking, perhaps have VolumeVersion instead and
if it is configured as false we just check the category.

>  [Policy]
>  #
>  # The ReconnectUUIDs defines the set of remote services that should try
> --
> 2.39.2
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v3 1/3] audio/avrcp: Guard SetAbsoluteVolume without target behind config value

[Marijn Suijten]

Hi Luiz,

I'm still clueless why this reply didn't reach my inbox.  I thought it
was going into ignore-land again (sorry) but randomly found your replies
on the BlueZ mailing list.  The bot message didn't make it through
either, but it did notify me about "audio/transport: Propagate errors
from avrcp_set_volume to DBus" being applied.

On 2023-03-14 16:16:55, Luiz Augusto von Dentz wrote:
> Hi Marijn,
> 
> On Fri, Mar 10, 2023 at 4:39 PM Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > Commit 179ccb936 ("avrcp: Set volume if volume changed event is
> > registered") introduced a catch that allows SetAbsoluteVolume to be sent
> > to a remote device that does _not_ implement the AVRCP TG profile.  This
> > is strange as the TG role is required to be able to send commands to the
> > peer, but the commit must have been applied to the tree for a reason.
> >
> > We discussed in [1] that workarounds for dubious peers and software
> > stacks should be guarded behind a config entry in main.conf, so this
> > starts out by introducing a new [AVRCP] category to to it that will
> > later be extended with other workarounds.
> >
> > [1]: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2
> > ---
> >  profiles/audio/avrcp.c | 12 +++++++++---
> >  src/btd.h              |  5 +++++
> >  src/main.c             | 13 +++++++++++++
> >  src/main.conf          |  6 ++++++
> >  4 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index 80f34c7a7..5e6322916 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -48,6 +48,7 @@
> >  #include "src/dbus-common.h"
> >  #include "src/shared/timeout.h"
> >  #include "src/shared/util.h"
> > +#include "src/btd.h"
> >
> >  #include "avctp.h"
> >  #include "avrcp.h"
> > @@ -4577,9 +4578,14 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
> >                                                                 &volume);
> >         }
> >
> > -       if (!session->controller && !avrcp_event_registered(session,
> > -                                       AVRCP_EVENT_VOLUME_CHANGED))
> > -               return -ENOTSUP;
> > +       if (btd_opts.avrcp.set_absolute_volume_without_target) {
> > +               if (!session->controller && !avrcp_event_registered(session,
> > +                                               AVRCP_EVENT_VOLUME_CHANGED))
> > +                       return -ENOTSUP;
> > +       } else {
> > +               if (!session->controller || session->controller->version < 0x0104)
> > +                       return -ENOTSUP;
> > +       }
> >
> >         memset(buf, 0, sizeof(buf));
> >
> > diff --git a/src/btd.h b/src/btd.h
> > index 42cffcde4..31c04a990 100644
> > --- a/src/btd.h
> > +++ b/src/btd.h
> > @@ -97,6 +97,10 @@ struct btd_avdtp_opts {
> >         uint8_t  stream_mode;
> >  };
> >
> > +struct btd_avrcp_opts {
> > +       gboolean set_absolute_volume_without_target;
> > +};
> > +
> >  struct btd_advmon_opts {
> >         uint8_t         rssi_sampling_period;
> >  };
> > @@ -136,6 +140,7 @@ struct btd_opts {
> >         enum mps_mode_t mps;
> >
> >         struct btd_avdtp_opts avdtp;
> > +       struct btd_avrcp_opts avrcp;
> >
> >         uint8_t         key_size;
> >
> > diff --git a/src/main.c b/src/main.c
> > index 99d9c508f..92f74e381 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -152,6 +152,11 @@ static const char *avdtp_options[] = {
> >         NULL
> >  };
> >
> > +static const char *avrcp_options[] = {
> > +       "SetAbsoluteVolumeWithoutTarget",
> > +       NULL
> > +};
> > +
> >  static const char *advmon_options[] = {
> >         "RSSISamplingPeriod",
> >         NULL
> > @@ -167,6 +172,7 @@ static const struct group_table {
> >         { "Policy",     policy_options },
> >         { "GATT",       gatt_options },
> >         { "AVDTP",      avdtp_options },
> > +       { "AVRCP",      avrcp_options },
> >         { "AdvMon",     advmon_options },
> >         { }
> >  };
> > @@ -975,6 +981,13 @@ static void parse_config(GKeyFile *config)
> >                 g_free(str);
> >         }
> >
> > +       boolean = g_key_file_get_boolean(config, "AVRCP",
> > +                                               "SetAbsoluteVolumeWithoutTarget", &err);
> > +       if (err)
> > +               g_clear_error(&err);
> > +       else
> > +               btd_opts.avrcp.set_absolute_volume_without_target = boolean;
> > +
> >         val = g_key_file_get_integer(config, "AdvMon", "RSSISamplingPeriod",
> >                                                                         &err);
> >         if (err) {
> > diff --git a/src/main.conf b/src/main.conf
> > index f187c9aaa..ca00ed03e 100644
> > --- a/src/main.conf
> > +++ b/src/main.conf
> > @@ -271,6 +271,12 @@
> >  # streaming: Use L2CAP Streaming Mode
> >  #StreamMode = basic
> >
> > +[AVRCP]
> > +# Allow SetAbsoluteVolume calls to a peer device that
> > +# does not advertise the AVRCP remote control target
> > +# profile.
> > +#SetAbsoluteVolumeWithoutTarget = false
> 
> Let's do just VolumeWithoutTarget and we should probably mention that
> it would ignore the version as well.

Sure, sounds good.  We can also invert the condition to validate the
version in the event that ->controller is non-NULL, even if
VolumeWithoutTarget is set.

- Marijn

> 
> > +
> >  [Policy]
> >  #
> >  # The ReconnectUUIDs defines the set of remote services that should try
> > --
> > 2.39.2
> >
> 
> 
> -- 
> Luiz Augusto von Dentz

Re: [PATCH BlueZ v3 1/3] audio/avrcp: Guard SetAbsoluteVolume without target behind config value

[Luiz Augusto von Dentz]

Hi Marijn,

On Tue, May 30, 2023 at 3:18 PM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Hi Luiz,
>
> I'm still clueless why this reply didn't reach my inbox.  I thought it
> was going into ignore-land again (sorry) but randomly found your replies
> on the BlueZ mailing list.  The bot message didn't make it through
> either, but it did notify me about "audio/transport: Propagate errors
> from avrcp_set_volume to DBus" being applied.

Now that you mentioned it, I did receive:

The response from the remote server was:

550 5.7.1 Blocked due to message content - please contact postmaster@seeweb.it

So perhaps that is the reason you didn't get any responses for a while.


> On 2023-03-14 16:16:55, Luiz Augusto von Dentz wrote:
> > Hi Marijn,
> >
> > On Fri, Mar 10, 2023 at 4:39 PM Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> > >
> > > Commit 179ccb936 ("avrcp: Set volume if volume changed event is
> > > registered") introduced a catch that allows SetAbsoluteVolume to be sent
> > > to a remote device that does _not_ implement the AVRCP TG profile.  This
> > > is strange as the TG role is required to be able to send commands to the
> > > peer, but the commit must have been applied to the tree for a reason.
> > >
> > > We discussed in [1] that workarounds for dubious peers and software
> > > stacks should be guarded behind a config entry in main.conf, so this
> > > starts out by introducing a new [AVRCP] category to to it that will
> > > later be extended with other workarounds.
> > >
> > > [1]: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2
> > > ---
> > >  profiles/audio/avrcp.c | 12 +++++++++---
> > >  src/btd.h              |  5 +++++
> > >  src/main.c             | 13 +++++++++++++
> > >  src/main.conf          |  6 ++++++
> > >  4 files changed, 33 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > index 80f34c7a7..5e6322916 100644
> > > --- a/profiles/audio/avrcp.c
> > > +++ b/profiles/audio/avrcp.c
> > > @@ -48,6 +48,7 @@
> > >  #include "src/dbus-common.h"
> > >  #include "src/shared/timeout.h"
> > >  #include "src/shared/util.h"
> > > +#include "src/btd.h"
> > >
> > >  #include "avctp.h"
> > >  #include "avrcp.h"
> > > @@ -4577,9 +4578,14 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
> > >                                                                 &volume);
> > >         }
> > >
> > > -       if (!session->controller && !avrcp_event_registered(session,
> > > -                                       AVRCP_EVENT_VOLUME_CHANGED))
> > > -               return -ENOTSUP;
> > > +       if (btd_opts.avrcp.set_absolute_volume_without_target) {
> > > +               if (!session->controller && !avrcp_event_registered(session,
> > > +                                               AVRCP_EVENT_VOLUME_CHANGED))
> > > +                       return -ENOTSUP;
> > > +       } else {
> > > +               if (!session->controller || session->controller->version < 0x0104)
> > > +                       return -ENOTSUP;
> > > +       }
> > >
> > >         memset(buf, 0, sizeof(buf));
> > >
> > > diff --git a/src/btd.h b/src/btd.h
> > > index 42cffcde4..31c04a990 100644
> > > --- a/src/btd.h
> > > +++ b/src/btd.h
> > > @@ -97,6 +97,10 @@ struct btd_avdtp_opts {
> > >         uint8_t  stream_mode;
> > >  };
> > >
> > > +struct btd_avrcp_opts {
> > > +       gboolean set_absolute_volume_without_target;
> > > +};
> > > +
> > >  struct btd_advmon_opts {
> > >         uint8_t         rssi_sampling_period;
> > >  };
> > > @@ -136,6 +140,7 @@ struct btd_opts {
> > >         enum mps_mode_t mps;
> > >
> > >         struct btd_avdtp_opts avdtp;
> > > +       struct btd_avrcp_opts avrcp;
> > >
> > >         uint8_t         key_size;
> > >
> > > diff --git a/src/main.c b/src/main.c
> > > index 99d9c508f..92f74e381 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -152,6 +152,11 @@ static const char *avdtp_options[] = {
> > >         NULL
> > >  };
> > >
> > > +static const char *avrcp_options[] = {
> > > +       "SetAbsoluteVolumeWithoutTarget",
> > > +       NULL
> > > +};
> > > +
> > >  static const char *advmon_options[] = {
> > >         "RSSISamplingPeriod",
> > >         NULL
> > > @@ -167,6 +172,7 @@ static const struct group_table {
> > >         { "Policy",     policy_options },
> > >         { "GATT",       gatt_options },
> > >         { "AVDTP",      avdtp_options },
> > > +       { "AVRCP",      avrcp_options },
> > >         { "AdvMon",     advmon_options },
> > >         { }
> > >  };
> > > @@ -975,6 +981,13 @@ static void parse_config(GKeyFile *config)
> > >                 g_free(str);
> > >         }
> > >
> > > +       boolean = g_key_file_get_boolean(config, "AVRCP",
> > > +                                               "SetAbsoluteVolumeWithoutTarget", &err);
> > > +       if (err)
> > > +               g_clear_error(&err);
> > > +       else
> > > +               btd_opts.avrcp.set_absolute_volume_without_target = boolean;
> > > +
> > >         val = g_key_file_get_integer(config, "AdvMon", "RSSISamplingPeriod",
> > >                                                                         &err);
> > >         if (err) {
> > > diff --git a/src/main.conf b/src/main.conf
> > > index f187c9aaa..ca00ed03e 100644
> > > --- a/src/main.conf
> > > +++ b/src/main.conf
> > > @@ -271,6 +271,12 @@
> > >  # streaming: Use L2CAP Streaming Mode
> > >  #StreamMode = basic
> > >
> > > +[AVRCP]
> > > +# Allow SetAbsoluteVolume calls to a peer device that
> > > +# does not advertise the AVRCP remote control target
> > > +# profile.
> > > +#SetAbsoluteVolumeWithoutTarget = false
> >
> > Let's do just VolumeWithoutTarget and we should probably mention that
> > it would ignore the version as well.
>
> Sure, sounds good.  We can also invert the condition to validate the
> version in the event that ->controller is non-NULL, even if
> VolumeWithoutTarget is set.
>
> - Marijn
>
> >
> > > +
> > >  [Policy]
> > >  #
> > >  # The ReconnectUUIDs defines the set of remote services that should try
> > > --
> > > 2.39.2
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz