[PATCH] AVDTP start/stop handling during disconnection

[PATCH] AVDTP start/stop handling during disconnection

[Daniel Örstadius]

A suggested patch for rejecting AVDTP CLOSE and START requests when a
CLOSE request has been initiated. The check is done by testing the
close_int flag in the avdtp_stream struct. The flag is reset when
receiving a timeout or rejection from the remote.

Changes in unix.c were made to call avdtp_unref() in case an AVDTP
session was marked by avdtp_ref(), but no SEP was found. This could
happen if START is called on a session being disconnected.

We found that these changes help to improve behavior both in case the
audio streaming application tries to start the stream during a
disconnection procedure, and if avdtp_close() is called twice.

Br,
Daniel Örstadius

------------------------

 audio/avdtp.c |   19 +++++++++++++++++--
 audio/unix.c  |   11 ++++++++++-
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index 8046dda..839f035 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -2223,9 +2223,12 @@ static gboolean request_timeout(gpointer user_data)
 		break;
 	case AVDTP_CLOSE:
 		error("Close request timed out");
-		if (lsep && lsep->cfm && lsep->cfm->close)
+		if (lsep && lsep->cfm && lsep->cfm->close) {
 			lsep->cfm->close(session, lsep, stream, &err,
 						lsep->user_data);
+			if (stream)
+				stream->close_int = FALSE;
+		}
 		break;
 	case AVDTP_SET_CONFIGURATION:
 		error("SetConfiguration request timed out");
@@ -2675,9 +2678,11 @@ static gboolean avdtp_parse_rej(struct avdtp *session,
 			return FALSE;
 		error("CLOSE request rejected: %s (%d)",
 				avdtp_strerror(&err), err.err.error_code);
-		if (sep && sep->cfm && sep->cfm->close)
+		if (sep && sep->cfm && sep->cfm->close) {
 			sep->cfm->close(session, sep, stream, &err,
 					sep->user_data);
+			stream->close_int = FALSE;
+		}
 		return TRUE;
 	case AVDTP_ABORT:
 		if (!stream_rej_to_err(buf, size, &err, &acp_seid))
@@ -3138,6 +3143,11 @@ int avdtp_start(struct avdtp *session, struct
avdtp_stream *stream)
 	if (stream->lsep->state != AVDTP_STATE_OPEN)
 		return -EINVAL;

+	if (stream->close_int == TRUE) {
+		error("avdtp_start: rejecting start since close is initiated");
+		return -EINVAL;
+	}
+
 	memset(&req, 0, sizeof(req));
 	req.first_seid.seid = stream->rseid;

@@ -3156,6 +3166,11 @@ int avdtp_close(struct avdtp *session, struct
avdtp_stream *stream)
 	if (stream->lsep->state < AVDTP_STATE_OPEN)
 		return -EINVAL;

+	if (stream->close_int == TRUE) {
+		error("avdtp_close: rejecting close since it is already inititated");
+		return -EINVAL;
+	}
+
 	memset(&req, 0, sizeof(req));
 	req.acp_seid = stream->rseid;

diff --git a/audio/unix.c b/audio/unix.c
index 0829630..61cc369 100644
--- a/audio/unix.c
+++ b/audio/unix.c
@@ -1051,14 +1051,17 @@ static void start_resume(struct audio_device
*dev, struct unix_client *client)
 	struct a2dp_data *a2dp;
 	struct headset_data *hs;
 	unsigned int id;
+	gboolean ref_session = FALSE;

 	switch (client->type) {
 	case TYPE_SINK:
 	case TYPE_SOURCE:
 		a2dp = &client->d.a2dp;

-		if (!a2dp->session)
+		if (!a2dp->session) {
 			a2dp->session = avdtp_get(&dev->src, &dev->dst);
+			ref_session = TRUE;
+		}			

 		if (!a2dp->session) {
 			error("Unable to get a session");
@@ -1067,6 +1070,12 @@ static void start_resume(struct audio_device
*dev, struct unix_client *client)

 		if (!a2dp->sep) {
 			error("seid not opened");
+
+			if (ref_session) {
+				avdtp_unref(a2dp->session);
+				a2dp->session = NULL;
+			}
+
 			goto failed;
 		}

Re: [PATCH] AVDTP start/stop handling during disconnection

[Luiz Augusto von Dentz]

Hi Daniel,

On Thu, Aug 27, 2009 at 12:24 PM, Daniel
Örstadius<daniel.orstadius@gmail.com> wrote:
> A suggested patch for rejecting AVDTP CLOSE and START requests when a
> CLOSE request has been initiated. The check is done by testing the
> close_int flag in the avdtp_stream struct. The flag is reset when
> receiving a timeout or rejection from the remote.
>
> Changes in unix.c were made to call avdtp_unref() in case an AVDTP
> session was marked by avdtp_ref(), but no SEP was found. This could
> happen if START is called on a session being disconnected.
>
> We found that these changes help to improve behavior both in case the
> audio streaming application tries to start the stream during a
> disconnection procedure, and if avdtp_close() is called twice.

It probably works better if you place it on avdtp struct so it works
even if we don't have a stream configured, I also prefer closing
instead of close_int.

The other parts looks good except this:

> diff --git a/audio/unix.c b/audio/unix.c
> index 0829630..61cc369 100644
> --- a/audio/unix.c
> +++ b/audio/unix.c
> @@ -1051,14 +1051,17 @@ static void start_resume(struct audio_device
> *dev, struct unix_client *client)
>        struct a2dp_data *a2dp;
>        struct headset_data *hs;
>        unsigned int id;
> +       gboolean ref_session = FALSE;
>
>        switch (client->type) {
>        case TYPE_SINK:
>        case TYPE_SOURCE:
>                a2dp = &client->d.a2dp;
>
> -               if (!a2dp->session)
> +               if (!a2dp->session) {
>                        a2dp->session = avdtp_get(&dev->src, &dev->dst);
> +                       ref_session = TRUE;
> +               }
>
>                if (!a2dp->session) {
>                        error("Unable to get a session");
> @@ -1067,6 +1070,12 @@ static void start_resume(struct audio_device
> *dev, struct unix_client *client)
>
>                if (!a2dp->sep) {
>                        error("seid not opened");
> +
> +                       if (ref_session) {
> +                               avdtp_unref(a2dp->session);
> +                               a2dp->session = NULL;
> +                       }
> +
>                        goto failed;
>                }

Normally we should wait for the client to disconnect then we release
its avdtp session, if it is closing/closed we shouldn't even attempt
to create another session to destroy a few lines ahead. So in this
case the client probably need fixing so it don't even try to start
while closing/closed.

ps: There is a patch to pulseaudio already that should fix this:
http://git.0pointer.de/?p=pulseaudio.git;a=commitdiff;h=8169a6a6c921215c1353e8a34fccbdc4e2e20440

-- 
Luiz Augusto von Dentz
Engenheiro de Computação

Re: [PATCH] AVDTP start/stop handling during disconnection

[Johan Hedberg]

Hi Daniel & Luiz,

On Thu, Aug 27, 2009, Luiz Augusto von Dentz wrote:
> On Thu, Aug 27, 2009 at 12:24 PM, Daniel
> Örstadius<daniel.orstadius@gmail.com> wrote:
> > A suggested patch for rejecting AVDTP CLOSE and START requests when a
> > CLOSE request has been initiated. The check is done by testing the
> > close_int flag in the avdtp_stream struct. The flag is reset when
> > receiving a timeout or rejection from the remote.
> >
> > Changes in unix.c were made to call avdtp_unref() in case an AVDTP
> > session was marked by avdtp_ref(), but no SEP was found. This could
> > happen if START is called on a session being disconnected.
> >
> > We found that these changes help to improve behavior both in case the
> > audio streaming application tries to start the stream during a
> > disconnection procedure, and if avdtp_close() is called twice.
> 
> It probably works better if you place it on avdtp struct so it works
> even if we don't have a stream configured,

But a stream can't be in OPEN state if it hasn't been configured (i.e. we
have a stream struct for it). So this part of the patch looks good to me.

> I also prefer closing instead of close_int.

That would have a different meaning though. To me closing means we're
closing but doesn't say anything about who initiated the close. close_int
otoh tells us that we initiated the close. Also, since this variable
already exists and is needed I think reusing it is fine.

So Daniel, could you split out the first part of your patch and resend it?
Thanks.

Johan

Re: [PATCH] AVDTP start/stop handling during disconnection

[Daniel Örstadius]

On Tue, Sep 1, 2009 at 10:02 AM, Johan Hedberg<johan.hedberg@nokia.com> wrote:

> So Daniel, could you split out the first part of your patch and resend it?


>From 627850090c93152b8faa3e4a724df24dbc556d4e Mon Sep 17 00:00:00 2001
From: Daniel Orstadius <daniel.orstadius@gmail.com>
Date: Tue, 1 Sep 2009 13:55:44 +0300
Subject: [PATCH] Reject AVDTP START/STOP when disconnecting

---
 audio/avdtp.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index 8046dda..839f035 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -2223,9 +2223,12 @@ static gboolean request_timeout(gpointer user_data)
 		break;
 	case AVDTP_CLOSE:
 		error("Close request timed out");
-		if (lsep && lsep->cfm && lsep->cfm->close)
+		if (lsep && lsep->cfm && lsep->cfm->close) {
 			lsep->cfm->close(session, lsep, stream, &err,
 						lsep->user_data);
+			if (stream)
+				stream->close_int = FALSE;
+		}
 		break;
 	case AVDTP_SET_CONFIGURATION:
 		error("SetConfiguration request timed out");
@@ -2675,9 +2678,11 @@ static gboolean avdtp_parse_rej(struct avdtp *session,
 			return FALSE;
 		error("CLOSE request rejected: %s (%d)",
 				avdtp_strerror(&err), err.err.error_code);
-		if (sep && sep->cfm && sep->cfm->close)
+		if (sep && sep->cfm && sep->cfm->close) {
 			sep->cfm->close(session, sep, stream, &err,
 					sep->user_data);
+			stream->close_int = FALSE;
+		}
 		return TRUE;
 	case AVDTP_ABORT:
 		if (!stream_rej_to_err(buf, size, &err, &acp_seid))
@@ -3138,6 +3143,11 @@ int avdtp_start(struct avdtp *session, struct
avdtp_stream *stream)
 	if (stream->lsep->state != AVDTP_STATE_OPEN)
 		return -EINVAL;

+	if (stream->close_int == TRUE) {
+		error("avdtp_start: rejecting start since close is initiated");
+		return -EINVAL;
+	}
+
 	memset(&req, 0, sizeof(req));
 	req.first_seid.seid = stream->rseid;

@@ -3156,6 +3166,11 @@ int avdtp_close(struct avdtp *session, struct
avdtp_stream *stream)
 	if (stream->lsep->state < AVDTP_STATE_OPEN)
 		return -EINVAL;

+	if (stream->close_int == TRUE) {
+		error("avdtp_close: rejecting close since it is already inititated");
+		return -EINVAL;
+	}
+
 	memset(&req, 0, sizeof(req));
 	req.acp_seid = stream->rseid;

-- 
1.6.0.4


Thanks for the feedback.

/Daniel

Re: [PATCH] AVDTP start/stop handling during disconnection

[Johan Hedberg]

Hi Daniel,

On Tue, Sep 01, 2009, Daniel Örstadius wrote:
> From 627850090c93152b8faa3e4a724df24dbc556d4e Mon Sep 17 00:00:00 2001
> From: Daniel Orstadius <daniel.orstadius@gmail.com>
> Date: Tue, 1 Sep 2009 13:55:44 +0300
> Subject: [PATCH] Reject AVDTP START/STOP when disconnecting
> 
> ---
>  audio/avdtp.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)

The patch has been pushed upstream. Thanks.

Johan