[PATCH 0/5] [RFC] handling avdtp collisions

[PATCH 0/5] [RFC] handling avdtp collisions

[Luiz Augusto von Dentz]

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

This has been causing some problems with some devices, specially with
start and suspend commands which even when local endpoint handles it
nicelly (by accepting as a valid response) the remote stack may not
respond at all causing the resquest to timeout/abort.

The spec suggest the following about this:

"In case an INT receives a request for the same command that it is
expecting a response for, it may reject the command and may use a
random time offset for a retransmission to avoid deadlock."

But note that this is not possible for abort which cannot be rejected
and also may be too expensive in some cases e.g. switching a2dp to hfp
when a call is ringing.

So instead of doing this directly on avdtp levle as the spec suggest, I
suggest to let the local sep decide what to do with the command
indication and in case it accepts remove the pending request from the
queue so it doesn't timeout nor can be rejected.

Note that this doesn't change much the current implementation of local
sep, it was already accepting indication regardless of pending
requests, but with the proposed changes when a collision happen
callbacks are called and the indication is properly propagated to the
endpoints.

Luiz Augusto von Dentz (5):
  Add handling of avdtp command collision
  Fix handling of a2dp suspend indication
  Fix handling of a2dp open indication
  Fix handling of a2dp start indication
  Fix handling of a2dp abort indication

 audio/a2dp.c  |   62 ++++++++++++++++++++++++++++++++++++++++++---
 audio/avdtp.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 127 insertions(+), 13 deletions(-)

[PATCH 1/5] Add handling of avdtp command collision

[Luiz Augusto von Dentz]

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

Check collision for avdtp open, close, start, suspend and abort
commands and if they collided remove the pending request if sep
has accepted the indication.
---
 audio/avdtp.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index 23281ac..db82329 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -310,6 +310,7 @@ struct pending_req {
 	size_t data_size;
 	struct avdtp_stream *stream; /* Set if the request targeted a stream */
 	guint timeout;
+	gboolean collided;
 };
 
 struct avdtp_remote_sep {
@@ -1617,6 +1618,55 @@ static gboolean avdtp_reconf_cmd(struct avdtp *session, uint8_t transaction,
 	return avdtp_unknown_cmd(session, transaction, AVDTP_RECONFIGURE);
 }
 
+static void avdtp_check_collision(struct avdtp *session, uint8_t cmd,
+					struct avdtp_stream *stream)
+{
+	struct pending_req *req = session->req;
+
+	if (req == NULL || (req->signal_id != cmd && cmd != AVDTP_ABORT))
+		return;
+
+	if (cmd == AVDTP_ABORT)
+		cmd = req->signal_id;
+
+	if (cmd == AVDTP_OPEN || cmd == AVDTP_CLOSE) {
+		struct seid_req *seid = session->req->data;
+
+		if (seid->acp_seid == stream->rseid)
+			req->collided = TRUE;
+	} else if (cmd == AVDTP_START) {
+		struct start_req *start = session->req->data;
+		struct seid *seid = &start->first_seid;
+		int count = 1 + session->req->data_size -
+						sizeof(struct start_req);
+		int i;
+
+		req->collided = FALSE;
+
+		for (i = 0; i < count; i++, seid++) {
+			if (seid->seid == stream->rseid) {
+				req->collided = TRUE;
+				break;
+			}
+		}
+	} else if (cmd == AVDTP_SUSPEND) {
+		struct suspend_req *suspend = session->req->data;
+		struct seid *seid = &suspend->first_seid;
+		int count = 1 + session->req->data_size -
+						sizeof(struct suspend_req);
+		int i;
+
+		req->collided = FALSE;
+
+		for (i = 0; i < count; i++, seid++) {
+			if (seid->seid == stream->rseid) {
+				req->collided = TRUE;
+				break;
+			}
+		}
+	}
+}
+
 static gboolean avdtp_open_cmd(struct avdtp *session, uint8_t transaction,
 				struct seid_req *req, unsigned int size)
 {
@@ -1648,6 +1698,8 @@ static gboolean avdtp_open_cmd(struct avdtp *session, uint8_t transaction,
 			goto failed;
 	}
 
+	avdtp_check_collision(session, AVDTP_OPEN, stream);
+
 	if (!avdtp_send(session, transaction, AVDTP_MSG_TYPE_ACCEPT,
 						AVDTP_OPEN, NULL, 0))
 		return FALSE;
@@ -1707,6 +1759,8 @@ static gboolean avdtp_start_cmd(struct avdtp *session, uint8_t transaction,
 				goto failed;
 		}
 
+		avdtp_check_collision(session, AVDTP_START, stream);
+
 		avdtp_sep_set_state(session, sep, AVDTP_STATE_STREAMING);
 	}
 
@@ -1753,6 +1807,8 @@ static gboolean avdtp_close_cmd(struct avdtp *session, uint8_t transaction,
 			goto failed;
 	}
 
+	avdtp_check_collision(session, AVDTP_CLOSE, stream);
+
 	avdtp_sep_set_state(session, sep, AVDTP_STATE_CLOSING);
 
 	if (!avdtp_send(session, transaction, AVDTP_MSG_TYPE_ACCEPT,
@@ -1812,6 +1868,8 @@ static gboolean avdtp_suspend_cmd(struct avdtp *session, uint8_t transaction,
 				goto failed;
 		}
 
+		avdtp_check_collision(session, AVDTP_SUSPEND, stream);
+
 		avdtp_sep_set_state(session, sep, AVDTP_STATE_OPEN);
 	}
 
@@ -1839,27 +1897,23 @@ static gboolean avdtp_abort_cmd(struct avdtp *session, uint8_t transaction,
 	}
 
 	sep = find_local_sep_by_seid(session->server, req->acp_seid);
-	if (!sep || !sep->stream) {
-		err = AVDTP_BAD_ACP_SEID;
-		goto failed;
-	}
+	if (!sep || !sep->stream)
+		return FALSE;
 
 	if (sep->ind && sep->ind->abort) {
 		if (!sep->ind->abort(session, sep, sep->stream, &err,
 					sep->user_data))
-			goto failed;
+			return FALSE;
 	}
 
+	avdtp_check_collision(session, AVDTP_ABORT, sep->stream);
+
 	ret = avdtp_send(session, transaction, AVDTP_MSG_TYPE_ACCEPT,
 						AVDTP_ABORT, NULL, 0);
 	if (ret)
 		avdtp_sep_set_state(session, sep, AVDTP_STATE_ABORTING);
 
 	return ret;
-
-failed:
-	return avdtp_send(session, transaction, AVDTP_MSG_TYPE_REJECT,
-					AVDTP_ABORT, &err, sizeof(err));
 }
 
 static gboolean avdtp_secctl_cmd(struct avdtp *session, uint8_t transaction,
@@ -2142,6 +2196,11 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
 		if (session->streams && session->dc_timer)
 			remove_disconnect_timer(session);
 
+		if (session->req && session->req->collided) {
+			DBG("Collision detected");
+			goto next;
+		}
+
 		return TRUE;
 	}
 
@@ -2192,6 +2251,7 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
 		break;
 	}
 
+next:
 	pending_req_free(session->req);
 	session->req = NULL;
 
-- 
1.7.1

[PATCH 2/5] Fix handling of a2dp suspend indication

[Luiz Augusto von Dentz]

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

When accepting the suspend indication all callbacks should be notified
that suspend completed.
---
 audio/a2dp.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index 1f3bc99..c7b6c6a 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -986,6 +986,9 @@ static gboolean suspend_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 				void *user_data)
 {
 	struct a2dp_sep *a2dp_sep = user_data;
+	struct a2dp_setup *setup;
+	gboolean start;
+	int perr;
 
 	if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK)
 		DBG("Sink %p: Suspend_Ind", sep);
@@ -999,6 +1002,29 @@ static gboolean suspend_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 		a2dp_sep->session = NULL;
 	}
 
+	if (!a2dp_sep->suspending)
+		return TRUE;
+
+	a2dp_sep->suspending = FALSE;
+
+	setup = find_setup_by_session(session);
+	if (!setup)
+		return TRUE;
+
+	start = setup->start;
+	setup->start = FALSE;
+
+	finalize_suspend(setup);
+
+	if (!start)
+		return TRUE;
+
+	perr = avdtp_start(session, a2dp_sep->stream);
+	if (perr < 0) {
+		error("Error on avdtp_start %s (%d)", strerror(-perr), -perr);
+		finalize_setup_errno(setup, -EIO, finalize_resume);
+	}
+
 	return TRUE;
 }
 
-- 
1.7.1

[PATCH 3/5] Fix handling of a2dp open indication

[Luiz Augusto von Dentz]

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

When accepting the open indication all config callbacks should be
notified that open completed.
---
 audio/a2dp.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index c7b6c6a..a607f06 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -883,11 +883,22 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 				void *user_data)
 {
 	struct a2dp_sep *a2dp_sep = user_data;
+	struct a2dp_setup *setup;
 
 	if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK)
 		DBG("Sink %p: Open_Ind", sep);
 	else
 		DBG("Source %p: Open_Ind", sep);
+
+	setup = find_setup_by_session(session);
+	if (!setup)
+		return TRUE;
+
+	if (setup->reconfigure)
+		setup->reconfigure = FALSE;
+
+	finalize_config(setup);
+
 	return TRUE;
 }
 
-- 
1.7.1

[PATCH 4/5] Fix handling of a2dp start indication

[Luiz Augusto von Dentz]

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

Only process callbacks if avdtp_start was sent, otherwise it may cancel
setup callbacks that were registered via g_idle_add.
---
 audio/a2dp.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index a607f06..e7234eb 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -954,10 +954,6 @@ static gboolean start_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 	else
 		DBG("Source %p: Start_Ind", sep);
 
-	setup = find_setup_by_session(session);
-	if (setup)
-		finalize_resume(setup);
-
 	if (!a2dp_sep->locked) {
 		a2dp_sep->session = avdtp_ref(session);
 		a2dp_sep->suspend_timer = g_timeout_add_seconds(SUSPEND_TIMEOUT,
@@ -965,6 +961,15 @@ static gboolean start_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 						a2dp_sep);
 	}
 
+	if (!a2dp_sep->starting)
+		return TRUE;
+
+	a2dp_sep->starting = FALSE;
+
+	setup = find_setup_by_session(session);
+	if (setup)
+		finalize_resume(setup);
+
 	return TRUE;
 }
 
@@ -980,6 +985,8 @@ static void start_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 	else
 		DBG("Source %p: Start_Cfm", sep);
 
+	a2dp_sep->starting = FALSE;
+
 	setup = find_setup_by_session(session);
 	if (!setup)
 		return;
@@ -2209,6 +2216,7 @@ unsigned int a2dp_resume(struct avdtp *session, struct a2dp_sep *sep,
 			error("avdtp_start failed");
 			goto failed;
 		}
+		sep->starting = TRUE;
 		break;
 	case AVDTP_STATE_STREAMING:
 		if (!sep->suspending && sep->suspend_timer) {
-- 
1.7.1

[PATCH 5/5] Fix handling of a2dp abort indication

[Luiz Augusto von Dentz]

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

When an abort indication is received reset all flags and finalize any
ongoing setup.
---
 audio/a2dp.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index e7234eb..719658b 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1180,6 +1180,7 @@ static gboolean abort_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 				void *user_data)
 {
 	struct a2dp_sep *a2dp_sep = user_data;
+	struct a2dp_setup *setup;
 
 	if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK)
 		DBG("Sink %p: Abort_Ind", sep);
@@ -1187,6 +1188,16 @@ static gboolean abort_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 		DBG("Source %p: Abort_Ind", sep);
 
 	a2dp_sep->stream = NULL;
+	a2dp_sep->suspending = FALSE;
+	a2dp_sep->starting = FALSE;
+
+	setup = find_setup_by_session(session);
+	if (!setup)
+		return TRUE;
+
+	finalize_setup_errno(setup, -ECONNRESET, finalize_suspend,
+							finalize_resume,
+							finalize_config);
 
 	return TRUE;
 }
-- 
1.7.1