From: Luiz Augusto von Dentz <[email protected]>
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.
---
v2: Fix capitalizing some acronyms
v3: Rebase and fix not using avdtp_start return in case of error
audio/avdtp.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 80 insertions(+)
diff --git a/audio/avdtp.c b/audio/avdtp.c
index eb3f213..1287698 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 {
@@ -1643,6 +1644,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)
{
@@ -1674,6 +1738,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;
@@ -1736,6 +1802,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);
}
@@ -1783,6 +1851,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,
@@ -1842,6 +1912,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);
}
@@ -1880,6 +1952,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)
@@ -2172,6 +2246,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;
}
@@ -2222,6 +2301,7 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
break;
}
+next:
pending_req_free(session->req);
session->req = NULL;
--
1.7.10.2
Hi Luiz,
On Fri, Jun 15, 2012, Luiz Augusto von Dentz wrote:
> 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.
> ---
> v2: Fix capitalizing some acronyms
> v3: Rebase and fix not using avdtp_start return in case of error
>
> audio/avdtp.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 80 insertions(+)
All patches in this set have been applied. Thanks.
Johan
From: Luiz Augusto von Dentz <[email protected]>
Change return of avdtp_start to -EINPROGRESS so the caller can check if
the operation is in progress and don't abort because of that.
---
audio/a2dp.c | 4 ++--
audio/avdtp.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/audio/a2dp.c b/audio/a2dp.c
index fafff87..404be53 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1038,7 +1038,7 @@ static gboolean suspend_ind(struct avdtp *session, struct avdtp_local_sep *sep,
return TRUE;
start_err = avdtp_start(session, a2dp_sep->stream);
- if (start_err < 0) {
+ if (start_err < 0 && start_err != -EINPROGRESS) {
error("avdtp_start: %s (%d)", strerror(-start_err),
-start_err);
finalize_setup_errno(setup, start_err, finalize_resume);
@@ -1086,7 +1086,7 @@ static void suspend_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
}
start_err = avdtp_start(session, a2dp_sep->stream);
- if (start_err < 0) {
+ if (start_err < 0 && start_err != -EINPROGRESS) {
error("avdtp_start: %s (%d)", strerror(-start_err),
-start_err);
finalize_setup_errno(setup, start_err, finalize_suspend, NULL);
diff --git a/audio/avdtp.c b/audio/avdtp.c
index 3ba2366..eb56c7c 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -3675,7 +3675,7 @@ int avdtp_start(struct avdtp *session, struct avdtp_stream *stream)
if (stream->starting == TRUE) {
DBG("stream already started");
- return -EINVAL;
+ return -EINPROGRESS;
}
memset(&req, 0, sizeof(req));
--
1.7.10.2
From: Luiz Augusto von Dentz <[email protected]>
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 d9dcead..fafff87 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1177,7 +1177,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)
{
@@ -1193,13 +1193,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 e9dea6c..3ba2366 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -1951,11 +1951,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);
@@ -1965,10 +1963,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 5f37dc3..dac093b 100644
--- a/audio/avdtp.h
+++ b/audio/avdtp.h
@@ -198,7 +198,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
From: Luiz Augusto von Dentz <[email protected]>
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 1287698..736eed7 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -1068,12 +1068,19 @@ 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_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) {
@@ -3660,6 +3667,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
From: Luiz Augusto von Dentz <[email protected]>
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 736eed7..e9dea6c 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -1948,10 +1948,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
From: Luiz Augusto von Dentz <[email protected]>
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 64de24a..5139f61 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;
@@ -2217,6 +2224,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.10.2
From: Luiz Augusto von Dentz <[email protected]>
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 5139f61..d9dcead 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1182,6 +1182,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);
@@ -1190,6 +1191,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
From: Luiz Augusto von Dentz <[email protected]>
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 fa97645..64de24a 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.10.2
From: Luiz Augusto von Dentz <[email protected]>
When accepting the suspend indication all callbacks should be notified
that suspend completed.
In addition to this fix not using avdtp_start return in indication
callback as well as in the confirmation.
---
audio/a2dp.c | 38 +++++++++++++++++++++++++++++++++-----
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/audio/a2dp.c b/audio/a2dp.c
index cc8f139..fa97645 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 start_err;
if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK)
DBG("Sink %p: Suspend_Ind", sep);
@@ -999,6 +1002,30 @@ 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;
+
+ start_err = avdtp_start(session, a2dp_sep->stream);
+ if (start_err < 0) {
+ error("avdtp_start: %s (%d)", strerror(-start_err),
+ -start_err);
+ finalize_setup_errno(setup, start_err, finalize_resume);
+ }
+
return TRUE;
}
@@ -1009,7 +1036,7 @@ static void suspend_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
struct a2dp_sep *a2dp_sep = user_data;
struct a2dp_setup *setup;
gboolean start;
- int perr;
+ int start_err;
if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK)
DBG("Sink %p: Suspend_Cfm", sep);
@@ -1040,10 +1067,11 @@ static void suspend_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
return;
}
- 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_suspend, NULL);
+ start_err = avdtp_start(session, a2dp_sep->stream);
+ if (start_err < 0) {
+ error("avdtp_start: %s (%d)", strerror(-start_err),
+ -start_err);
+ finalize_setup_errno(setup, start_err, finalize_suspend, NULL);
}
}
--
1.7.10.2
Hi Luiz.
Regarding your patch that was applied already, I think
finalize_setup_errno() function missed NULL argument.
This is reported by Syam Sidhardhan <[email protected]>
On 06/15/2012 11:41 PM, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> 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 5139f61..d9dcead 100644
> --- a/audio/a2dp.c
> +++ b/audio/a2dp.c
> @@ -1182,6 +1182,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);
> @@ -1190,6 +1191,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);
> +
Here.
Could you give us your opinion?
Regards
Chanyeol