[PATCH v3 1/4] a2dp: Fix caching endpoints for unknown version

[PATCH v3 1/4] a2dp: Fix caching endpoints for unknown version

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Don't cache the capabilities of endpoints which the version is unknown
since so capabilities may not be available in such case.
---
 profiles/audio/a2dp.c  | 11 +++++++++--
 profiles/audio/avdtp.c |  7 ++++++-
 profiles/audio/avdtp.h |  1 +
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index a2ce3204d..15e211b95 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -2667,15 +2667,22 @@ static void discover_cb(struct avdtp *session, GSList *seps,
 				struct avdtp_error *err, void *user_data)
 {
 	struct a2dp_setup *setup = user_data;
+	uint16_t version = avdtp_get_version(session);
 
-	DBG("err %p", err);
+	DBG("version 0x%04x err %p", version, err);
 
 	setup->seps = seps;
 	setup->err = err;
 
 	if (!err) {
 		g_slist_foreach(seps, register_remote_sep, setup->chan);
-		store_remote_seps(setup->chan);
+
+		/* Only store version has been initialized as features like
+		 * Delay Reporting may not be queried if the version in
+		 * unknown.
+		 */
+		if (version)
+			store_remote_seps(setup->chan);
 	}
 
 	finalize_discover(setup);
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index b632e41c5..1fd2be051 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -2256,7 +2256,7 @@ static uint16_t get_version(struct avdtp *session)
 	const sdp_record_t *rec;
 	sdp_list_t *protos;
 	sdp_data_t *proto_desc;
-	uint16_t ver = 0x0100;
+	uint16_t ver = 0x0000;
 
 	rec = btd_device_get_record(session->device, A2DP_SINK_UUID);
 	if (!rec)
@@ -2396,6 +2396,11 @@ struct avdtp *avdtp_new(GIOChannel *chan, struct btd_device *device,
 	return session;
 }
 
+uint16_t avdtp_get_version(struct avdtp *session)
+{
+	return session->version;
+}
+
 static GIOChannel *l2cap_connect(struct avdtp *session)
 {
 	GError *err = NULL;
diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
index ad2cb9bcb..f1e51d4e3 100644
--- a/profiles/audio/avdtp.h
+++ b/profiles/audio/avdtp.h
@@ -310,3 +310,4 @@ struct avdtp_server *avdtp_get_server(struct avdtp_local_sep *lsep);
 
 struct avdtp *avdtp_new(GIOChannel *chan, struct btd_device *device,
 							struct queue *lseps);
+uint16_t avdtp_get_version(struct avdtp *session);
-- 
2.25.3

[PATCH v3 2/4] doc/media-api: Add documentation for DelayReporting

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The code was expecting the endpoint to expose
MediaEndpoint.DelayReporting property in order to expose
MediaTransport.Delay property.
---
 doc/media-api.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/doc/media-api.txt b/doc/media-api.txt
index 07f7ac3e0..dabc69936 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -580,6 +580,10 @@ Properties	string UUID [readonly, optional]:
 
 			Device object which the endpoint is belongs to.
 
+		bool DelayReporting [readonly, optional]:
+
+			Indicates if endpoint supports Delay Reporting.
+
 MediaTransport1 hierarchy
 =========================
 
-- 
2.25.3

[PATCH v3 3/4] a2dp: Store Delay Reporting capability

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This stores Delay Reporting capability so it is properly restored when
loading from cache.
---
 profiles/audio/a2dp.c  | 46 ++++++++++++++++++++++++++++++++----------
 profiles/audio/avdtp.c | 13 ++++++++++--
 profiles/audio/avdtp.h |  5 ++++-
 3 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 15e211b95..0d877b132 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -1847,11 +1847,25 @@ static gboolean get_device(const GDBusPropertyTable *property,
 	return TRUE;
 }
 
+static gboolean get_delay_reporting(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct a2dp_remote_sep *sep = data;
+	dbus_bool_t delay_report;
+
+	delay_report = avdtp_get_delay_reporting(sep->sep);
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &delay_report);
+
+	return TRUE;
+}
+
 static const GDBusPropertyTable sep_properties[] = {
 	{ "UUID", "s", get_uuid, NULL, NULL },
 	{ "Codec", "y", get_codec, NULL, NULL },
 	{ "Capabilities", "ay", get_capabilities, NULL, NULL },
 	{ "Device", "o", get_device, NULL, NULL },
+	{ "DelayReporting", "b", get_delay_reporting, NULL, NULL },
 	{ }
 };
 
@@ -1942,6 +1956,7 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
 	for (; *seids; seids++) {
 		uint8_t type;
 		uint8_t codec;
+		uint8_t delay_reporting;
 		GSList *l = NULL;
 		char caps[256];
 		uint8_t data[128];
@@ -1955,11 +1970,17 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
 		if (!value)
 			continue;
 
-		if (sscanf(value, "%02hhx:%02hhx:%s", &type, &codec,
+		/* Try loading with delay_reporting first */
+		if (sscanf(value, "%02hhx:%02hhx:%02hhx:%s", &type, &codec,
+					&delay_reporting, caps) != 4) {
+			/* Try old format */
+			if (sscanf(value, "%02hhx:%02hhx:%s", &type, &codec,
 								caps) != 3) {
-			warn("Unable to load Endpoint: seid %u", rseid);
-			g_free(value);
-			continue;
+				warn("Unable to load Endpoint: seid %u", rseid);
+				g_free(value);
+				continue;
+			}
+			delay_reporting = false;
 		}
 
 		for (i = 0, size = strlen(caps); i < size; i += 2) {
@@ -1979,7 +2000,8 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
 
 		caps_add_codec(&l, codec, data, size / 2);
 
-		rsep = avdtp_register_remote_sep(chan->session, rseid, type, l);
+		rsep = avdtp_register_remote_sep(chan->session, rseid, type, l,
+							delay_reporting);
 		if (!rsep) {
 			warn("Unable to register Endpoint: seid %u", rseid);
 			continue;
@@ -2605,7 +2627,7 @@ static struct queue *a2dp_select_eps(struct avdtp *session, uint8_t type,
 static void store_remote_sep(void *data, void *user_data)
 {
 	struct a2dp_remote_sep *sep = data;
-	GKeyFile *key_file = (void *) user_data;
+	GKeyFile *key_file = user_data;
 	char seid[4], value[256];
 	struct avdtp_service_capability *service = avdtp_get_codec(sep->sep);
 	struct avdtp_media_codec_capability *codec = (void *) service->data;
@@ -2614,13 +2636,13 @@ static void store_remote_sep(void *data, void *user_data)
 
 	sprintf(seid, "%02hhx", avdtp_get_seid(sep->sep));
 
-	offset = sprintf(value, "%02hhx:%02hhx:", avdtp_get_type(sep->sep),
-						codec->media_codec_type);
+	offset = sprintf(value, "%02hhx:%02hhx:%02hhx:",
+			avdtp_get_type(sep->sep), codec->media_codec_type,
+			avdtp_get_delay_reporting(sep->sep));
 
 	for (i = 0; i < service->length - sizeof(*codec); i++)
 		offset += sprintf(value + offset, "%02hhx", codec->data[i]);
 
-
 	g_key_file_set_string(key_file, "Endpoints", seid, value);
 }
 
@@ -2644,7 +2666,8 @@ static void store_remote_seps(struct a2dp_channel *chan)
 	key_file = g_key_file_new();
 	g_key_file_load_from_file(key_file, filename, 0, NULL);
 
-	data = g_key_file_get_string(key_file, "Endpoints", "LastUsed", NULL);
+	data = g_key_file_get_string(key_file, "Endpoints", "LastUsed",
+								NULL);
 
 	/* Remove current endpoints since it might have changed */
 	g_key_file_remove_group(key_file, "Endpoints", NULL);
@@ -2652,7 +2675,8 @@ static void store_remote_seps(struct a2dp_channel *chan)
 	queue_foreach(chan->seps, store_remote_sep, key_file);
 
 	if (data) {
-		g_key_file_set_string(key_file, "Endpoints", "LastUsed", data);
+		g_key_file_set_string(key_file, "Endpoints", "LastUsed",
+						data);
 		g_free(data);
 	}
 
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 1fd2be051..45727f01e 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -3210,6 +3210,11 @@ struct avdtp_service_capability *avdtp_get_codec(struct avdtp_remote_sep *sep)
 	return sep->codec;
 }
 
+bool avdtp_get_delay_reporting(struct avdtp_remote_sep *sep)
+{
+	return sep->delay_reporting;
+}
+
 struct avdtp_service_capability *avdtp_service_cap_new(uint8_t category,
 							void *data, int length)
 {
@@ -3229,7 +3234,8 @@ struct avdtp_service_capability *avdtp_service_cap_new(uint8_t category,
 struct avdtp_remote_sep *avdtp_register_remote_sep(struct avdtp *session,
 							uint8_t seid,
 							uint8_t type,
-							GSList *caps)
+							GSList *caps,
+							bool delay_reporting)
 {
 	struct avdtp_remote_sep *sep;
 	GSList *l;
@@ -3244,6 +3250,7 @@ struct avdtp_remote_sep *avdtp_register_remote_sep(struct avdtp *session,
 	sep->type = type;
 	sep->media_type = AVDTP_MEDIA_TYPE_AUDIO;
 	sep->caps = caps;
+	sep->delay_reporting = delay_reporting;
 
 	for (l = caps; l; l = g_slist_next(l)) {
 		struct avdtp_service_capability *cap = l->data;
@@ -3252,7 +3259,9 @@ struct avdtp_remote_sep *avdtp_register_remote_sep(struct avdtp *session,
 			sep->codec = cap;
 	}
 
-	DBG("seid %d type %d media %d", sep->seid, sep->type, sep->media_type);
+	DBG("seid %d type %d media %d delay_reporting %s", sep->seid, sep->type,
+				sep->media_type,
+				sep->delay_reporting ? "true" : "false");
 
 	return sep;
 }
diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
index f1e51d4e3..011fea89e 100644
--- a/profiles/audio/avdtp.h
+++ b/profiles/audio/avdtp.h
@@ -226,7 +226,8 @@ struct avdtp_service_capability *avdtp_service_cap_new(uint8_t category,
 struct avdtp_remote_sep *avdtp_register_remote_sep(struct avdtp *session,
 							uint8_t seid,
 							uint8_t type,
-							GSList *caps);
+							GSList *caps,
+							bool delay_reporting);
 
 uint8_t avdtp_get_seid(struct avdtp_remote_sep *sep);
 
@@ -234,6 +235,8 @@ uint8_t avdtp_get_type(struct avdtp_remote_sep *sep);
 
 struct avdtp_service_capability *avdtp_get_codec(struct avdtp_remote_sep *sep);
 
+bool avdtp_get_delay_reporting(struct avdtp_remote_sep *sep);
+
 int avdtp_discover(struct avdtp *session, avdtp_discover_cb_t cb,
 			void *user_data);
 
-- 
2.25.3

[PATCH v3 4/4] doc/settings-storage: Update documentation of Endpoints

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Add documentation of DelayReporting storage as that has been added to
the cache.
---
 doc/settings-storage.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/doc/settings-storage.txt b/doc/settings-storage.txt
index 5f6d25141..d21150f09 100644
--- a/doc/settings-storage.txt
+++ b/doc/settings-storage.txt
@@ -218,9 +218,10 @@ Sample Attributes section:
 
 [Endpoints] group contains:
 
-	<xx>:<xx>:<xxxxxxxx...>	String	First field is the endpoint type,
-					followed by codec type and its
-					capabilies as hexadecimal encoded
+	<xx>:<xx>:<xx>::<xx...> String	First field is the endpoint type,
+					followed by codec type and delay
+					reporting and its
+					capabilities as hexadecimal encoded
 					string.
 	LastUsed:<xx>:<xx>	String	LastUsed has two fields which are the
 					local and remote seids as hexadecimal
-- 
2.25.3

RE: [v3,1/4] a2dp: Fix caching endpoints for unknown version

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 601 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.
While we are preparing for reviewing the patches, we found the following
issue/warning.

Test Result:
makecheck Failed

Outputs:
./test-driver: line 107: 14841 Aborted                 (core dumped) "$@" > $log_file 2>&1
make[3]: *** [Makefile:9726: test-suite.log] Error 1
make[2]: *** [Makefile:9834: check-TESTS] Error 2
make[1]: *** [Makefile:10228: check-am] Error 2
make: *** [Makefile:10230: check] Error 2



---
Regards,
Linux Bluetooth

Re: [v3,1/4] a2dp: Fix caching endpoints for unknown version

[Luiz Augusto von Dentz]

Hi Tedd,

On Tue, May 19, 2020 at 1:39 PM <bluez.test.bot@gmail.com> wrote:
>
>
> 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.
> While we are preparing for reviewing the patches, we found the following
> issue/warning.
>
> Test Result:
> makecheck Failed
>
> Outputs:
> ./test-driver: line 107: 14841 Aborted                 (core dumped) "$@" > $log_file 2>&1
> make[3]: *** [Makefile:9726: test-suite.log] Error 1
> make[2]: *** [Makefile:9834: check-TESTS] Error 2
> make[1]: *** [Makefile:10228: check-am] Error 2
> make: *** [Makefile:10230: check] Error 2

Can you give a look why CI is core dumping here, also perhaps we
should inspect the test-suite.log when this happens.

-- 
Luiz Augusto von Dentz

Re: [v3,1/4] a2dp: Fix caching endpoints for unknown version

[An, Tedd]

Hi Luiz

The failure is from "test-midi" from the output. However, the current script didn't save the test-suite.log.
I have another test setup on a different account but the test passed on that setup with same patch set. So it could be the false positive as Brian mentioned.

Let me make a change to
- capture/save the "test-suite.log" if there is a failure

Also, I ran again the it passed at the 2nd time. 
I will disable the test-midi until we fix this issue.

Regards,
Tedd

On 5/19/20, 2:07 PM, "linux-bluetooth-owner@vger.kernel.org on behalf of Luiz Augusto von Dentz" <linux-bluetooth-owner@vger.kernel.org on behalf of luiz.dentz@gmail.com> wrote:

    Hi Tedd,

    On Tue, May 19, 2020 at 1:39 PM <bluez.test.bot@gmail.com> wrote:
    >
    >
    > 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.
    > While we are preparing for reviewing the patches, we found the following
    > issue/warning.
    >
    > Test Result:
    > makecheck Failed
    >
    > Outputs:
    > ./test-driver: line 107: 14841 Aborted                 (core dumped) "$@" > $log_file 2>&1
    > make[3]: *** [Makefile:9726: test-suite.log] Error 1
    > make[2]: *** [Makefile:9834: check-TESTS] Error 2
    > make[1]: *** [Makefile:10228: check-am] Error 2
    > make: *** [Makefile:10230: check] Error 2

    Can you give a look why CI is core dumping here, also perhaps we
    should inspect the test-suite.log when this happens.

    -- 
    Luiz Augusto von Dentz

Re: [PATCH v3 1/4] a2dp: Fix caching endpoints for unknown version

[Luiz Augusto von Dentz]

Hi,

On Tue, May 19, 2020 at 1:03 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Don't cache the capabilities of endpoints which the version is unknown
> since so capabilities may not be available in such case.
> ---
>  profiles/audio/a2dp.c  | 11 +++++++++--
>  profiles/audio/avdtp.c |  7 ++++++-
>  profiles/audio/avdtp.h |  1 +
>  3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index a2ce3204d..15e211b95 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -2667,15 +2667,22 @@ static void discover_cb(struct avdtp *session, GSList *seps,
>                                 struct avdtp_error *err, void *user_data)
>  {
>         struct a2dp_setup *setup = user_data;
> +       uint16_t version = avdtp_get_version(session);
>
> -       DBG("err %p", err);
> +       DBG("version 0x%04x err %p", version, err);
>
>         setup->seps = seps;
>         setup->err = err;
>
>         if (!err) {
>                 g_slist_foreach(seps, register_remote_sep, setup->chan);
> -               store_remote_seps(setup->chan);
> +
> +               /* Only store version has been initialized as features like
> +                * Delay Reporting may not be queried if the version in
> +                * unknown.
> +                */
> +               if (version)
> +                       store_remote_seps(setup->chan);
>         }
>
>         finalize_discover(setup);
> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index b632e41c5..1fd2be051 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -2256,7 +2256,7 @@ static uint16_t get_version(struct avdtp *session)
>         const sdp_record_t *rec;
>         sdp_list_t *protos;
>         sdp_data_t *proto_desc;
> -       uint16_t ver = 0x0100;
> +       uint16_t ver = 0x0000;
>
>         rec = btd_device_get_record(session->device, A2DP_SINK_UUID);
>         if (!rec)
> @@ -2396,6 +2396,11 @@ struct avdtp *avdtp_new(GIOChannel *chan, struct btd_device *device,
>         return session;
>  }
>
> +uint16_t avdtp_get_version(struct avdtp *session)
> +{
> +       return session->version;
> +}
> +
>  static GIOChannel *l2cap_connect(struct avdtp *session)
>  {
>         GError *err = NULL;
> diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
> index ad2cb9bcb..f1e51d4e3 100644
> --- a/profiles/audio/avdtp.h
> +++ b/profiles/audio/avdtp.h
> @@ -310,3 +310,4 @@ struct avdtp_server *avdtp_get_server(struct avdtp_local_sep *lsep);
>
>  struct avdtp *avdtp_new(GIOChannel *chan, struct btd_device *device,
>                                                         struct queue *lseps);
> +uint16_t avdtp_get_version(struct avdtp *session);
> --
> 2.25.3

Pushed.

-- 
Luiz Augusto von Dentz