[PATCH v2 0/3] Minor audio-related fixes

[PATCH v2 0/3] Minor audio-related fixes

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

This patch series includes minor modifications that have been useful while testing the Media API for HFP use-cases.

These three patches were acked by Luiz Augusto von Dentz the 15-12-2011.

The first patch has been removed from the previous proposal, given that it doesn't really solve the underlying problem. According to Luiz (15-12-2011):

"gateway_suspend_stream as it is useless, but I think we should fix it
to work similarly to headset_suspend_stream so we properly wait until
the socket is closed before we unlock and reply, this is necessary in
order to properly synchronize switching profiles otherwise
gateway_suspend_complete maybe called without SCO being completely
disconnected. Also when a fd is passed over to another process calling
close/g_io_channel_shutdown is not enough since this only release our
reference, in this case shutdown must be called."

This issue is not tackled here.

Mikel Astiz (3):
  audio: gateway_unlock together with in_use = FALSE
  a2dp: return NotSupported error if no server found
  audio: do not disconnect on gateway SCO failure

 audio/a2dp.c      |    2 +-
 audio/gateway.c   |    2 +-
 audio/transport.c |    3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)

-- 
1.7.6.5

[PATCH v2 1/3] audio: gateway_unlock together with in_use = FALSE

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Calling gateway_unlock seems safer in gateway_suspend_complete, in
combination with the update of transport->in_use. This avoids the
transitional state of having the gateway unlocked but the transport
still in use.

This approach is also more consistent with the way headset and a2dp
handle it.
---
 audio/transport.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/audio/transport.c b/audio/transport.c
index 7bde32d..2092644 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -510,6 +510,7 @@ static gboolean gateway_suspend_complete(gpointer user_data)
 {
 	struct media_owner *owner = user_data;
 	struct media_transport *transport = owner->transport;
+	struct audio_device *device = transport->device;
 
 	/* Release always succeeds */
 	if (owner->pending) {
@@ -518,6 +519,7 @@ static gboolean gateway_suspend_complete(gpointer user_data)
 		media_owner_remove(owner);
 	}
 
+	gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
 	transport->in_use = FALSE;
 	media_transport_remove(transport, owner);
 	return FALSE;
@@ -536,7 +538,6 @@ static guint suspend_gateway(struct media_transport *transport,
 	}
 
 	gateway_suspend_stream(device);
-	gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
 	g_idle_add(gateway_suspend_complete, owner);
 	return id++;
 }
-- 
1.7.6.5

Re: [PATCH v2 1/3] audio: gateway_unlock together with in_use = FALSE

[Luiz Augusto von Dentz]

Hi Mikel,

On Tue, Feb 14, 2012 at 11:06 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> Calling gateway_unlock seems safer in gateway_suspend_complete, in
> combination with the update of transport->in_use. This avoids the
> transitional state of having the gateway unlocked but the transport
> still in use.
>
> This approach is also more consistent with the way headset and a2dp
> handle it.
> ---
>  audio/transport.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/audio/transport.c b/audio/transport.c
> index 7bde32d..2092644 100644
> --- a/audio/transport.c
> +++ b/audio/transport.c
> @@ -510,6 +510,7 @@ static gboolean gateway_suspend_complete(gpointer user_data)
>  {
>        struct media_owner *owner = user_data;
>        struct media_transport *transport = owner->transport;
> +       struct audio_device *device = transport->device;
>
>        /* Release always succeeds */
>        if (owner->pending) {
> @@ -518,6 +519,7 @@ static gboolean gateway_suspend_complete(gpointer user_data)
>                media_owner_remove(owner);
>        }
>
> +       gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
>        transport->in_use = FALSE;
>        media_transport_remove(transport, owner);
>        return FALSE;
> @@ -536,7 +538,6 @@ static guint suspend_gateway(struct media_transport *transport,
>        }
>
>        gateway_suspend_stream(device);
> -       gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
>        g_idle_add(gateway_suspend_complete, owner);
>        return id++;
>  }
> --
> 1.7.6.5

Ack

-- 
Luiz Augusto von Dentz

[PATCH v2 2/3] a2dp: return NotSupported error if no server found

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

With both sink and sources disabled, the A2DP server is not even
registered in audio_manager_init. When trying to register the endpoint,
this should result in the same error as if the server existed but the
profile was disabled.
---
 audio/a2dp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index bbb91ce..623a0bc 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1589,7 +1589,7 @@ struct a2dp_sep *a2dp_add_sep(const bdaddr_t *src, uint8_t type,
 	server = find_server(servers, src);
 	if (server == NULL) {
 		if (err)
-			*err = -EINVAL;
+			*err = -EPROTONOSUPPORT;
 		return NULL;
 	}
 
-- 
1.7.6.5

Re: [PATCH v2 2/3] a2dp: return NotSupported error if no server found

[Luiz Augusto von Dentz]

Hi Mikel,

On Tue, Feb 14, 2012 at 11:06 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> With both sink and sources disabled, the A2DP server is not even
> registered in audio_manager_init. When trying to register the endpoint,
> this should result in the same error as if the server existed but the
> profile was disabled.
> ---
>  audio/a2dp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/audio/a2dp.c b/audio/a2dp.c
> index bbb91ce..623a0bc 100644
> --- a/audio/a2dp.c
> +++ b/audio/a2dp.c
> @@ -1589,7 +1589,7 @@ struct a2dp_sep *a2dp_add_sep(const bdaddr_t *src, uint8_t type,
>        server = find_server(servers, src);
>        if (server == NULL) {
>                if (err)
> -                       *err = -EINVAL;
> +                       *err = -EPROTONOSUPPORT;
>                return NULL;
>        }
>
> --
> 1.7.6.5

Ack

-- 
Luiz Augusto von Dentz

[PATCH v2 3/3] audio: do not disconnect on gateway SCO failure

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Failure on BlueZ-initiated SCO requests should not drop the RFCOMM
connection to the gateway. Instead, considering the stream as suspended
should be enough.
---
 audio/gateway.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/audio/gateway.c b/audio/gateway.c
index bde3e02..163f7b5 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -255,7 +255,7 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 
 	if (err) {
 		error("sco_connect_cb(): %s", err->message);
-		gateway_close(dev);
+		gateway_suspend_stream(dev);
 		return;
 	}
 
-- 
1.7.6.5

Re: [PATCH v2 3/3] audio: do not disconnect on gateway SCO failure

[Luiz Augusto von Dentz]

Hi Mikel,

On Tue, Feb 14, 2012 at 11:06 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> Failure on BlueZ-initiated SCO requests should not drop the RFCOMM
> connection to the gateway. Instead, considering the stream as suspended
> should be enough.
> ---
>  audio/gateway.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/audio/gateway.c b/audio/gateway.c
> index bde3e02..163f7b5 100644
> --- a/audio/gateway.c
> +++ b/audio/gateway.c
> @@ -255,7 +255,7 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
>
>        if (err) {
>                error("sco_connect_cb(): %s", err->message);
> -               gateway_close(dev);
> +               gateway_suspend_stream(dev);
>                return;
>        }
>
> --
> 1.7.6.5

Ack

-- 
Luiz Augusto von Dentz

Re: [PATCH v2 0/3] Minor audio-related fixes

[Johan Hedberg]

Hi Mikel,

On Tue, Feb 14, 2012, Mikel Astiz wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
> 
> This patch series includes minor modifications that have been useful
> while testing the Media API for HFP use-cases.
> 
> These three patches were acked by Luiz Augusto von Dentz the 15-12-2011.
> 
> The first patch has been removed from the previous proposal, given
> that it doesn't really solve the underlying problem. According to Luiz
> (15-12-2011):
> 
> "gateway_suspend_stream as it is useless, but I think we should fix it
> to work similarly to headset_suspend_stream so we properly wait until
> the socket is closed before we unlock and reply, this is necessary in
> order to properly synchronize switching profiles otherwise
> gateway_suspend_complete maybe called without SCO being completely
> disconnected. Also when a fd is passed over to another process calling
> close/g_io_channel_shutdown is not enough since this only release our
> reference, in this case shutdown must be called."
> 
> This issue is not tackled here.
> 
> Mikel Astiz (3):
>   audio: gateway_unlock together with in_use = FALSE
>   a2dp: return NotSupported error if no server found
>   audio: do not disconnect on gateway SCO failure
> 
>  audio/a2dp.c      |    2 +-
>  audio/gateway.c   |    2 +-
>  audio/transport.c |    3 ++-
>  3 files changed, 4 insertions(+), 3 deletions(-)

All three patches have been applied. Thanks.

Johan