[PATCH BlueZ 1/8] audio: Add handling of avdtp command collision

[PATCH BlueZ 1/8] audio: Add handling of avdtp command collision

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.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 |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index bbdae10..08fd2a7 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -314,6 +314,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 {
@@ -1646,6 +1647,69 @@ static gboolean avdtp_reconf_cmd(struct avdtp *session, uint8_t transaction,
 					AVDTP_RECONFIGURE, &rej, sizeof(rej));
 }
 
+static void check_seid_collision(struct pending_req *req, uint8_t id)
+{
+	struct seid_req *seid = req->data;
+
+	if (seid->acp_seid == id)
+		req->collided = TRUE;
+}
+
+static void check_start_collision(struct pending_req *req, uint8_t id)
+{
+	struct start_req *start = req->data;
+	struct seid *seid = &start->first_seid;
+	int count = 1 + req->data_size - sizeof(struct start_req);
+	int i;
+
+	for (i = 0; i < count; i++, seid++) {
+		if (seid->seid == id) {
+			req->collided = TRUE;
+			return;
+		}
+	}
+}
+
+static void check_suspend_collision(struct pending_req *req, uint8_t id)
+{
+	struct suspend_req *suspend = req->data;
+	struct seid *seid = &suspend->first_seid;
+	int count = 1 + req->data_size - sizeof(struct suspend_req);
+	int i;
+
+	for (i = 0; i < count; i++, seid++) {
+		if (seid->seid == id) {
+			req->collided = TRUE;
+			return;
+		}
+	}
+}
+
+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;
+
+	switch (cmd) {
+	case AVDTP_OPEN:
+	case AVDTP_CLOSE:
+		check_seid_collision(req, stream->rseid);
+		break;
+	case AVDTP_START:
+		check_start_collision(req, stream->rseid);
+		break;
+	case AVDTP_SUSPEND:
+		check_suspend_collision(req, stream->rseid);
+		break;
+	}
+}
+
 static gboolean avdtp_open_cmd(struct avdtp *session, uint8_t transaction,
 				struct seid_req *req, unsigned int size)
 {
@@ -1677,6 +1741,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;
@@ -1738,6 +1804,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);
 	}
 
@@ -1786,6 +1854,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,
@@ -1845,6 +1915,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);
 	}
 
@@ -1883,6 +1955,8 @@ static gboolean avdtp_abort_cmd(struct avdtp *session, uint8_t transaction,
 			goto failed;
 	}
 
+	avdtp_check_collision(session, AVDTP_ABORT, sep->stream);
+
 	ret = avdtp_send(session, transaction, AVDTP_MSG_TYPE_ACCEPT,
 						AVDTP_ABORT, NULL, 0);
 	if (ret)
@@ -2175,6 +2249,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;
 	}
 
@@ -2225,6 +2304,7 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
 		break;
 	}
 
+next:
 	pending_req_free(session->req);
 	session->req = NULL;
 
-- 
1.7.10.2

[PATCH BlueZ 2/8] audio: Fix handling of a2dp suspend indication

[Luiz Augusto von Dentz]

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

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

diff --git a/audio/a2dp.c b/audio/a2dp.c
index 4df7b5c..2ba6c8d 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -985,6 +985,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);
@@ -998,6 +1001,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.10.2

[PATCH BlueZ 3/8] audio: Fix handling of a2dp open indication

[Luiz Augusto von Dentz]

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

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

diff --git a/audio/a2dp.c b/audio/a2dp.c
index 2ba6c8d..cf8ffda 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -882,11 +882,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.10.2

[PATCH BlueZ 4/8] audio: Fix handling of a2dp start indication

[Luiz Augusto von Dentz]

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

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

diff --git a/audio/a2dp.c b/audio/a2dp.c
index cf8ffda..cc94fbe 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -953,10 +953,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,
@@ -964,6 +960,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;
 }
 
@@ -979,6 +984,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;
@@ -2142,6 +2149,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_RESUMING:
 		break;
-- 
1.7.10.2

[PATCH BlueZ 5/8] audio: Fix handling of a2dp abort indication

[Luiz Augusto von Dentz]

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

When an abort is received all setup callbacks should return an error.
---
 audio/a2dp.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index cc94fbe..094476e 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1179,6 +1179,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,14 @@ static gboolean abort_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 
 	a2dp_sep->stream = NULL;
 
+	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.10.2

[PATCH BlueZ 6/8] audio: Wait remote side to send AVDTP_START when acting as acceptor

[Luiz Augusto von Dentz]

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

Some devices like Sony Ericsson MW600 reject AVDTP_START if it was the
initiator of the connection, apparently it follows recommendation 12 of
simultaneous use of HFP, A2DP and AVRCP profiles white paper which says:

  "If the RD has configured and opened a stream it is also responsible to
  start the streaming via GAVDP_START."

If the client is fast enough and try to acquire the transport this cause
an error, so instead of sending AVDTP_START the code now checks if it is
the acceptor of the stream and wait the remote side to send the command.
---
 audio/avdtp.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index 08fd2a7..08264f4 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -1070,13 +1070,20 @@ static void avdtp_sep_set_state(struct avdtp *session,
 		break;
 	case AVDTP_STATE_OPEN:
 		stream->starting = FALSE;
-		if (old_state > AVDTP_STATE_OPEN && session->auto_dc)
+		if ((old_state > AVDTP_STATE_OPEN && session->auto_dc) ||
+							stream->open_acp)
 			stream->idle_timer = g_timeout_add_seconds(STREAM_TIMEOUT,
 								stream_timeout,
 								stream);
 		break;
 	case AVDTP_STATE_RESUMING:
 	case AVDTP_STATE_STREAMING:
+		if (stream->idle_timer) {
+			g_source_remove(stream->idle_timer);
+			stream->idle_timer = 0;
+		}
+		stream->open_acp = FALSE;
+		break;
 	case AVDTP_STATE_CLOSING:
 	case AVDTP_STATE_ABORTING:
 		if (stream->idle_timer) {
@@ -3670,6 +3677,15 @@ int avdtp_start(struct avdtp *session, struct avdtp_stream *stream)
 	if (stream->lsep->state != AVDTP_STATE_OPEN)
 		return -EINVAL;
 
+	/* Recommendation 12:
+	 *  If the RD has configured and opened a stream it is also responsible
+	 *  to start the streaming via GAVDP_START.
+	 */
+	if (stream->open_acp) {
+		stream->starting = TRUE;
+		return 0;
+	}
+
 	if (stream->close_int == TRUE) {
 		error("avdtp_start: rejecting start since close is initiated");
 		return -EINVAL;
-- 
1.7.10.2

[PATCH BlueZ 7/8] AVDTP: Do not respond ABORT command with invalid id

[Luiz Augusto von Dentz]

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

AVDTP spec, 8.15.2 Abort Response:

  "If an AVDTP_ABORT_CMD contains an invalid SEID, no response shall be
  sent."
---
 audio/avdtp.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index 08264f4..61f5f94 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -1951,10 +1951,8 @@ 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 TRUE;
 
 	if (sep->ind && sep->ind->abort) {
 		if (!sep->ind->abort(session, sep, sep->stream, &err,
-- 
1.7.10.2

[PATCH BlueZ 8/8] AVDTP: Fix responding to ABORT with reject

[Luiz Augusto von Dentz]

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

ABORT command cannot be rejected
---
 audio/a2dp.c  |    6 +++---
 audio/avdtp.c |   12 +++---------
 audio/avdtp.h |    2 +-
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index 094476e..5b1b424 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1174,7 +1174,7 @@ static void close_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 		g_timeout_add(RECONFIGURE_TIMEOUT, a2dp_reconfigure, setup);
 }
 
-static gboolean abort_ind(struct avdtp *session, struct avdtp_local_sep *sep,
+static void abort_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 				struct avdtp_stream *stream, uint8_t *err,
 				void *user_data)
 {
@@ -1190,13 +1190,13 @@ static gboolean abort_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 
 	setup = find_setup_by_session(session);
 	if (!setup)
-		return TRUE;
+		return;
 
 	finalize_setup_errno(setup, -ECONNRESET, finalize_suspend,
 							finalize_resume,
 							finalize_config);
 
-	return TRUE;
+	return;
 }
 
 static void abort_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
diff --git a/audio/avdtp.c b/audio/avdtp.c
index 61f5f94..1366bf1 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -1954,11 +1954,9 @@ static gboolean avdtp_abort_cmd(struct avdtp *session, uint8_t transaction,
 	if (!sep || !sep->stream)
 		return TRUE;
 
-	if (sep->ind && sep->ind->abort) {
-		if (!sep->ind->abort(session, sep, sep->stream, &err,
-					sep->user_data))
-			goto failed;
-	}
+	if (sep->ind && sep->ind->abort)
+		sep->ind->abort(session, sep, sep->stream, &err,
+							sep->user_data);
 
 	avdtp_check_collision(session, AVDTP_ABORT, sep->stream);
 
@@ -1968,10 +1966,6 @@ static gboolean avdtp_abort_cmd(struct avdtp *session, uint8_t transaction,
 		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,
diff --git a/audio/avdtp.h b/audio/avdtp.h
index 1979694..8acf302 100644
--- a/audio/avdtp.h
+++ b/audio/avdtp.h
@@ -199,7 +199,7 @@ struct avdtp_sep_ind {
 	gboolean (*close) (struct avdtp *session, struct avdtp_local_sep *sep,
 				struct avdtp_stream *stream, uint8_t *err,
 				void *user_data);
-	gboolean (*abort) (struct avdtp *session, struct avdtp_local_sep *sep,
+	void (*abort) (struct avdtp *session, struct avdtp_local_sep *sep,
 				struct avdtp_stream *stream, uint8_t *err,
 				void *user_data);
 	gboolean (*reconfigure) (struct avdtp *session,
-- 
1.7.10.2

Re: [PATCH BlueZ 2/8] audio: Fix handling of a2dp suspend indication

[Johan Hedberg]

Hi Luiz,

On Fri, Jun 15, 2012, Luiz Augusto von Dentz wrote:
> @@ -985,6 +985,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;

Could you just call this start_err?

Why isn't err used in this function?

> +	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);

Why aren't you passing perr instead of -EIO to finalize_setup_errno? If
there's a valid reason for ignoring the specific value you should at
least add a code comment for it.

Johan

Re: [PATCH BlueZ 4/8] audio: Fix handling of a2dp start indication

[Johan Hedberg]

Hi Luiz,

On Fri, Jun 15, 2012, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> Only process callbacks if avdtp_start was sent, otherwise it may cancel
> setup callbacks that were registere via g_idle_add.
> ---
>  audio/a2dp.c |   16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)

This doesn't apply. After what tree exactly have you been testing this:

Applying: audio: Fix handling of a2dp start indication
error: patch failed: audio/a2dp.c:2142
error: audio/a2dp.c: patch does not apply

Johan

Re: [PATCH BlueZ 2/8] audio: Fix handling of a2dp suspend indication

[Luiz Augusto von Dentz]

Hi Johan,

On Fri, Jun 15, 2012 at 12:30 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Luiz,
>
> On Fri, Jun 15, 2012, Luiz Augusto von Dentz wrote:
>> @@ -985,6 +985,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;
>
> Could you just call this start_err?

Sounds good.

> Why isn't err used in this function?

Because we are not returning FALSE as suspend should not be reject we
are just unable to start.

>> +     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);
>
> Why aren't you passing perr instead of -EIO to finalize_setup_errno? If
> there's a valid reason for ignoring the specific value you should at
> least add a code comment for it.

Gonna fix this, there are a couple of other places that does that, so
I gonna fix them too in a separated patch.

-- 
Luiz Augusto von Dentz