[PATCH 0/4] Minor audio-related fixes

[PATCH 0/4] 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.

Mikel Astiz (4):
  audio: remove unnecessary method call
  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 |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

-- 
1.7.6.4

[PATCH 1/4] audio: remove unnecessary method call

[Mikel Astiz]

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

Calling gateway_suspend_stream should not be necessary given that
gateway_unlock with rw flags already does it.
---
 audio/transport.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/audio/transport.c b/audio/transport.c
index 7bde32d..6029528 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -535,7 +535,6 @@ static guint suspend_gateway(struct media_transport *transport,
 		return 0;
 	}
 
-	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.4

Re: [PATCH 1/4] audio: remove unnecessary method call

[Luiz Augusto von Dentz]

Hi Mikel,

On Thu, Dec 15, 2011 at 12:01 PM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> Calling gateway_suspend_stream should not be necessary given that
> gateway_unlock with rw flags already does it.
> ---
>  audio/transport.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/audio/transport.c b/audio/transport.c
> index 7bde32d..6029528 100644
> --- a/audio/transport.c
> +++ b/audio/transport.c
> @@ -535,7 +535,6 @@ static guint suspend_gateway(struct media_transport *transport,
>                return 0;
>        }
>
> -       gateway_suspend_stream(device);
>        gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
>        g_idle_add(gateway_suspend_complete, owner);
>        return id++;
> --

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.

-- 
Luiz Augusto von Dentz

Re: [PATCH 1/4] audio: remove unnecessary method call

[Johan Hedberg]

Hi,

On Thu, Dec 15, 2011, Luiz Augusto von Dentz wrote:
> On Thu, Dec 15, 2011 at 12:01 PM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> > From: Mikel Astiz <mikel.astiz@bmw-carit.de>
> >
> > Calling gateway_suspend_stream should not be necessary given that
> > gateway_unlock with rw flags already does it.
> > ---
> >  audio/transport.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> >
> > diff --git a/audio/transport.c b/audio/transport.c
> > index 7bde32d..6029528 100644
> > --- a/audio/transport.c
> > +++ b/audio/transport.c
> > @@ -535,7 +535,6 @@ static guint suspend_gateway(struct media_transport *transport,
> >                return 0;
> >        }
> >
> > -       gateway_suspend_stream(device);
> >        gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
> >        g_idle_add(gateway_suspend_complete, owner);
> >        return id++;
> > --
> 
> 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.

The subsequent patches in the set fail to apply without this first one,
so the whole patch set is on hold until we get an update for it.

Johan

[PATCH 2/4] 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 6029528..fc7026e 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;
@@ -535,7 +537,6 @@ static guint suspend_gateway(struct media_transport *transport,
 		return 0;
 	}
 
-	gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
 	g_idle_add(gateway_suspend_complete, owner);
 	return id++;
 }
-- 
1.7.6.4

Re: [PATCH 2/4] audio: gateway_unlock together with in_use = FALSE

[Luiz Augusto von Dentz]

Hi Mikel,

On Thu, Dec 15, 2011 at 12:01 PM, 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 6029528..fc7026e 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;
> @@ -535,7 +537,6 @@ static guint suspend_gateway(struct media_transport *transport,
>                return 0;
>        }
>
> -       gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
>        g_idle_add(gateway_suspend_complete, owner);
>        return id++;
>  }
> --
> 1.7.6.4

Ack, but see my comments about the first patch.

-- 
Luiz Augusto von Dentz

[PATCH 3/4] 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 5ca105c..9ed2ebb 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1590,7 +1590,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.4

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

[Luiz Augusto von Dentz]

Hi Mikel,

On Thu, Dec 15, 2011 at 12:01 PM, 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 5ca105c..9ed2ebb 100644
> --- a/audio/a2dp.c
> +++ b/audio/a2dp.c
> @@ -1590,7 +1590,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.4

Ack.

-- 
Luiz Augusto von Dentz

[PATCH 4/4] 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 9b1aab3..f28b14d 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.4

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

[Luiz Augusto von Dentz]

Hi Mikel,

On Thu, Dec 15, 2011 at 12:01 PM, 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 9b1aab3..f28b14d 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.4
>

Ack, this should be up to the endpoint to decided since it may want to retry.

-- 
Luiz Augusto von Dentz