2014-01-10 01:24:21

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH v2 0/7] Next skeleton patches for audio plugin

v1:
Those patches adds function to handle send/receive msg on audio IPC.
Also adds some small changes.

Not that since we do not yet support AUDIO_OP_OPEN, audio socket will be
reseted just after connect.

v2:
Fixes suggester by Luiz
Some additional fixes

Lukasz Rymanowski (7):
android/audio: Prefix error log with "audio"
android: Fix error check from pthread_create
android/audio: Refactor create_audio_ipc
android/audio: Trivial function move
android/audio: Add skeleton for ipc_open_cmd function
android/audio: Add audio_ipc_cmd
android/audio: Add audio_ipc_cleanup function

android/Makefile.am | 1 +
android/hal-audio.c | 304 ++++++++++++++++++++++++++++++++++++++++++----------
android/hal-ipc.c | 6 +-
3 files changed, 250 insertions(+), 61 deletions(-)

--
1.8.4



2014-01-10 15:49:18

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH v2 0/7] Next skeleton patches for audio plugin

Hi Lukasz,

On Fri, Jan 10, 2014 at 3:24 AM, Lukasz Rymanowski
<[email protected]> wrote:
> v1:
> Those patches adds function to handle send/receive msg on audio IPC.
> Also adds some small changes.
>
> Not that since we do not yet support AUDIO_OP_OPEN, audio socket will be
> reseted just after connect.
>
> v2:
> Fixes suggester by Luiz
> Some additional fixes
>
> Lukasz Rymanowski (7):
> android/audio: Prefix error log with "audio"
> android: Fix error check from pthread_create
> android/audio: Refactor create_audio_ipc
> android/audio: Trivial function move
> android/audio: Add skeleton for ipc_open_cmd function
> android/audio: Add audio_ipc_cmd
> android/audio: Add audio_ipc_cleanup function
>
> android/Makefile.am | 1 +
> android/hal-audio.c | 304 ++++++++++++++++++++++++++++++++++++++++++----------
> android/hal-ipc.c | 6 +-
> 3 files changed, 250 insertions(+), 61 deletions(-)
>
> --
> 1.8.4

Ive applied some of these set, any of those that involved the command
I left out for now since they are not complete and they would
otherwise just cause failures now that haltest does load
audio.a2dp.default.so.


--
Luiz Augusto von Dentz

2014-01-10 01:24:27

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH v2 6/7] android/audio: Add audio_ipc_cmd

Adds function to handle send/receive on audio_sk

Also moves ipc related functions on top of the file to avoid moving it
later on when implementing AudioFlinger functions.

---
android/Makefile.am | 1 +
android/hal-audio.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 172 insertions(+), 7 deletions(-)

diff --git a/android/Makefile.am b/android/Makefile.am
index 0a32a95..94e5329 100644
--- a/android/Makefile.am
+++ b/android/Makefile.am
@@ -117,6 +117,7 @@ android_android_tester_LDFLAGS = -pthread
noinst_LTLIBRARIES += android/libaudio-internal.la

android_libaudio_internal_la_SOURCES = android/audio-msg.h \
+ android/hal-msg.h \
android/hal-audio.c \
android/hardware/audio.h \
android/hardware/audio_effect.h \
diff --git a/android/hal-audio.c b/android/hal-audio.c
index 4c5a9d9..3b94e62 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -31,6 +31,7 @@

#include "audio-msg.h"
#include "hal-log.h"
+#include "hal-msg.h"

static int listen_sk = -1;
static int audio_sk = -1;
@@ -38,12 +39,182 @@ static bool close_thread = false;

static pthread_t ipc_th = 0;
static pthread_mutex_t close_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t sk_mutex = PTHREAD_MUTEX_INITIALIZER;

struct a2dp_audio_dev {
struct audio_hw_device dev;
struct audio_stream_out *out;
};

+static int audio_ipc_cmd(uint8_t service_id, uint8_t opcode, uint16_t len,
+ void *param, size_t *rsp_len, void *rsp, int *fd)
+{
+ ssize_t ret;
+ struct msghdr msg;
+ struct iovec iv[2];
+ struct hal_hdr cmd;
+ char cmsgbuf[CMSG_SPACE(sizeof(int))];
+ struct hal_status s;
+ size_t s_len = sizeof(s);
+
+ if (audio_sk < 0) {
+ error("audio: Invalid cmd socket passed to audio_ipc_cmd");
+ goto failed;
+ }
+
+ if (!rsp || !rsp_len) {
+ memset(&s, 0, s_len);
+ rsp_len = &s_len;
+ rsp = &s;
+ }
+
+ memset(&msg, 0, sizeof(msg));
+ memset(&cmd, 0, sizeof(cmd));
+
+ cmd.service_id = service_id;
+ cmd.opcode = opcode;
+ cmd.len = len;
+
+ iv[0].iov_base = &cmd;
+ iv[0].iov_len = sizeof(cmd);
+
+ iv[1].iov_base = param;
+ iv[1].iov_len = len;
+
+ msg.msg_iov = iv;
+ msg.msg_iovlen = 2;
+
+ pthread_mutex_lock(&sk_mutex);
+
+ ret = sendmsg(audio_sk, &msg, 0);
+ if (ret < 0) {
+ error("audio: Sending command failed:%s", strerror(errno));
+ pthread_mutex_unlock(&sk_mutex);
+ goto failed;
+ }
+
+ /* socket was shutdown */
+ if (ret == 0) {
+ error("audio: Command socket closed");
+ goto failed;
+ }
+
+ memset(&msg, 0, sizeof(msg));
+ memset(&cmd, 0, sizeof(cmd));
+
+ iv[0].iov_base = &cmd;
+ iv[0].iov_len = sizeof(cmd);
+
+ iv[1].iov_base = rsp;
+ iv[1].iov_len = *rsp_len;
+
+ msg.msg_iov = iv;
+ msg.msg_iovlen = 2;
+
+ if (fd) {
+ memset(cmsgbuf, 0, sizeof(cmsgbuf));
+ msg.msg_control = cmsgbuf;
+ msg.msg_controllen = sizeof(cmsgbuf);
+ }
+
+ ret = recvmsg(audio_sk, &msg, 0);
+ if (ret < 0) {
+ error("audio: Receiving command response failed:%s",
+ strerror(errno));
+ pthread_mutex_unlock(&sk_mutex);
+ goto failed;
+ }
+
+ pthread_mutex_unlock(&sk_mutex);
+
+ if (ret < (ssize_t) sizeof(cmd)) {
+ error("audio: Too small response received(%zd bytes)", ret);
+ goto failed;
+ }
+
+ if (cmd.service_id != service_id) {
+ error("audio: Invalid service id (%u vs %u)", cmd.service_id,
+ service_id);
+ goto failed;
+ }
+
+ if (ret != (ssize_t) (sizeof(cmd) + cmd.len)) {
+ error("audio: Malformed response received(%zd bytes)", ret);
+ goto failed;
+ }
+
+ if (cmd.opcode != opcode && cmd.opcode != AUDIO_OP_STATUS) {
+ error("audio: Invalid opcode received (%u vs %u)",
+ cmd.opcode, opcode);
+ goto failed;
+ }
+
+ if (cmd.opcode == AUDIO_OP_STATUS) {
+ struct hal_status *s = rsp;
+
+ if (sizeof(*s) != cmd.len) {
+ error("audio: Invalid status length");
+ goto failed;
+ }
+
+ if (s->code == AUDIO_STATUS_SUCCESS) {
+ error("audio: Invalid success status response");
+ goto failed;
+ }
+
+ return s->code;
+ }
+
+ /* Receive auxiliary data in msg */
+ if (fd) {
+ struct cmsghdr *cmsg;
+
+ *fd = -1;
+
+ for (cmsg = CMSG_FIRSTHDR(&msg); cmsg;
+ cmsg = CMSG_NXTHDR(&msg, cmsg)) {
+ if (cmsg->cmsg_level == SOL_SOCKET
+ && cmsg->cmsg_type == SCM_RIGHTS) {
+ memcpy(fd, CMSG_DATA(cmsg), sizeof(int));
+ break;
+ }
+ }
+ }
+
+ if (rsp_len)
+ *rsp_len = cmd.len;
+
+ return AUDIO_STATUS_SUCCESS;
+
+failed:
+ /* Some serious issue happen on IPC - recover */
+ shutdown(audio_sk, SHUT_RDWR);
+ audio_sk = -1;
+ return AUDIO_STATUS_FAILED;
+}
+
+static int ipc_open_cmd(void)
+{
+ struct audio_rsp_open rsp;
+ uint8_t buf[sizeof(struct audio_cmd_open) +
+ sizeof(struct audio_preset)];
+ size_t rsp_len = sizeof(rsp);
+ int result;
+
+ DBG("");
+
+ memset(&buf, 0, sizeof(buf));
+
+ /* TODO: Fill in buf here with smth */
+
+ result = audio_ipc_cmd(AUDIO_SERVICE_ID, AUDIO_OP_OPEN, sizeof(buf), buf,
+ &rsp_len, &rsp, NULL);
+
+ /*TODO: Read response and store endoint id */
+
+ return result;
+}
+
static ssize_t out_write(struct audio_stream_out *stream, const void *buffer,
size_t bytes)
{
@@ -417,13 +588,6 @@ static int audio_close(hw_device_t *device)
return 0;
}

-static int ipc_open_cmd(void)
-{
- DBG("Not Implemented");
-
- return AUDIO_STATUS_FAILED;
-}
-
static void *ipc_handler(void *data)
{
bool done = false;
--
1.8.4


2014-01-10 01:24:25

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH v2 4/7] android/audio: Trivial function move

Just moves create_listening_audio_ipc() closer to audio_open()

---
android/hal-audio.c | 87 +++++++++++++++++++++++++++--------------------------
1 file changed, 44 insertions(+), 43 deletions(-)

diff --git a/android/hal-audio.c b/android/hal-audio.c
index b0e0b1d..0b345ed 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -417,6 +417,50 @@ static int audio_close(hw_device_t *device)
return 0;
}

+static void *ipc_handler(void *data)
+{
+ bool done = false;
+ struct pollfd pfd;
+
+ DBG("");
+
+ while (!done) {
+ DBG("Waiting for audio_ipc connection ...");
+ audio_sk = accept(listen_sk, NULL, NULL);
+ if (audio_sk < 0) {
+ int err = errno;
+ error("audio: Failed to accept socket: %d (%s)", err,
+ strerror(err));
+ continue;
+ }
+
+ DBG("Audio IPC: Connected");
+
+ /* TODO: Register ENDPOINT here */
+
+ memset(&pfd, 0, sizeof(pfd));
+ pfd.fd = audio_sk;
+ pfd.events = POLLHUP | POLLERR | POLLNVAL;
+
+ /* Check if socket is still alive. Empty while loop.*/
+ while (poll(&pfd, 1, -1) < 0 && errno == EINTR);
+
+ if (pfd.revents & (POLLHUP | POLLERR | POLLNVAL)) {
+ info("Audio HAL: Socket closed");
+ audio_sk = -1;
+ }
+
+ /*Check if audio_dev is closed */
+ pthread_mutex_lock(&close_mutex);
+ done = close_thread;
+ close_thread = false;
+ pthread_mutex_unlock(&close_mutex);
+ }
+
+ info("Closing bluetooth_watcher thread");
+ return NULL;
+}
+
static int create_listening_audio_ipc(void)
{
struct sockaddr_un addr;
@@ -461,49 +505,6 @@ failed:
return err;
}

-static void *ipc_handler(void *data)
-{
- bool done = false;
- struct pollfd pfd;
-
- DBG("");
-
- while (!done) {
- audio_sk = accept(listen_sk, NULL, NULL);
- if (audio_sk < 0) {
- int err = errno;
- error("audio: Failed to accept socket: %d (%s)", err,
- strerror(err));
- continue;
- }
-
- DBG("Audio IPC: Connected");
-
- /* TODO: Register ENDPOINT here */
-
- memset(&pfd, 0, sizeof(pfd));
- pfd.fd = audio_sk;
- pfd.events = POLLHUP | POLLERR | POLLNVAL;
-
- /* Check if socket is still alive. Empty while loop.*/
- while (poll(&pfd, 1, -1) < 0 && errno == EINTR);
-
- if (pfd.revents & (POLLHUP | POLLERR | POLLNVAL)) {
- info("Audio HAL: Socket closed");
- audio_sk = -1;
- }
-
- /*Check if audio_dev is closed */
- pthread_mutex_lock(&close_mutex);
- done = close_thread;
- close_thread = false;
- pthread_mutex_unlock(&close_mutex);
- }
-
- info("Closing bluetooth_watcher thread");
- return NULL;
-}
-
static int audio_open(const hw_module_t *module, const char *name,
hw_device_t **device)
{
--
1.8.4


2014-01-10 01:24:26

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH v2 5/7] android/audio: Add skeleton for ipc_open_cmd function

---
android/hal-audio.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/android/hal-audio.c b/android/hal-audio.c
index 0b345ed..4c5a9d9 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -417,6 +417,13 @@ static int audio_close(hw_device_t *device)
return 0;
}

+static int ipc_open_cmd(void)
+{
+ DBG("Not Implemented");
+
+ return AUDIO_STATUS_FAILED;
+}
+
static void *ipc_handler(void *data)
{
bool done = false;
@@ -436,7 +443,11 @@ static void *ipc_handler(void *data)

DBG("Audio IPC: Connected");

- /* TODO: Register ENDPOINT here */
+ if (ipc_open_cmd() == AUDIO_STATUS_FAILED) {
+ error("audio: Failed to open endpoint, recover");
+ shutdown(audio_sk, SHUT_RDWR);
+ continue;
+ }

memset(&pfd, 0, sizeof(pfd));
pfd.fd = audio_sk;
--
1.8.4


2014-01-10 01:24:24

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH v2 3/7] android/audio: Refactor create_audio_ipc

This patch adds creating listening audio ipc socket in AudioFlinger
context on audio_open() and moves accepting connection to ipc_th.

---
android/hal-audio.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/android/hal-audio.c b/android/hal-audio.c
index c71b93c..b0e0b1d 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -32,6 +32,7 @@
#include "audio-msg.h"
#include "hal-log.h"

+static int listen_sk = -1;
static int audio_sk = -1;
static bool close_thread = false;

@@ -409,11 +410,14 @@ static int audio_close(hw_device_t *device)

pthread_join(ipc_th, NULL);

+ close(listen_sk);
+ listen_sk = -1;
+
free(a2dp_dev);
return 0;
}

-static bool create_audio_ipc(void)
+static int create_listening_audio_ipc(void)
{
struct sockaddr_un addr;
int err;
@@ -426,7 +430,7 @@ static bool create_audio_ipc(void)
err = errno;
error("audio: Failed to create socket: %d (%s)", err,
strerror(err));
- return false;
+ return err;
}

memset(&addr, 0, sizeof(addr));
@@ -449,19 +453,12 @@ static bool create_audio_ipc(void)
goto failed;
}

- audio_sk = accept(sk, NULL, NULL);
- if (audio_sk < 0) {
- err = errno;
- error("audio: Failed to accept socket: %d (%s)", err, strerror(err));
- goto failed;
- }
-
- close(sk);
- return true;
+ listen_sk = sk;
+ return 0;

failed:
close(sk);
- return false;
+ return err;
}

static void *ipc_handler(void *data)
@@ -472,9 +469,11 @@ static void *ipc_handler(void *data)
DBG("");

while (!done) {
- if(!create_audio_ipc()) {
- error("audio: Failed to create listening socket");
- sleep(1);
+ audio_sk = accept(listen_sk, NULL, NULL);
+ if (audio_sk < 0) {
+ int err = errno;
+ error("audio: Failed to accept socket: %d (%s)", err,
+ strerror(err));
continue;
}

@@ -519,6 +518,10 @@ static int audio_open(const hw_module_t *module, const char *name,
return -EINVAL;
}

+ err = create_listening_audio_ipc();
+ if (err)
+ return -err;
+
a2dp_dev = calloc(1, sizeof(struct a2dp_audio_dev));
if (!a2dp_dev)
return -ENOMEM;
--
1.8.4


2014-01-10 01:24:28

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH v2 7/7] android/audio: Add audio_ipc_cleanup function

---
android/hal-audio.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/android/hal-audio.c b/android/hal-audio.c
index 3b94e62..49cc375 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -46,6 +46,14 @@ struct a2dp_audio_dev {
struct audio_stream_out *out;
};

+static void audio_ipc_cleanup(void)
+{
+ if (audio_sk >= 0) {
+ shutdown(audio_sk, SHUT_RDWR);
+ audio_sk = -1;
+ }
+}
+
static int audio_ipc_cmd(uint8_t service_id, uint8_t opcode, uint16_t len,
void *param, size_t *rsp_len, void *rsp, int *fd)
{
@@ -188,8 +196,7 @@ static int audio_ipc_cmd(uint8_t service_id, uint8_t opcode, uint16_t len,

failed:
/* Some serious issue happen on IPC - recover */
- shutdown(audio_sk, SHUT_RDWR);
- audio_sk = -1;
+ audio_ipc_cleanup();
return AUDIO_STATUS_FAILED;
}

@@ -575,7 +582,7 @@ static int audio_close(hw_device_t *device)
DBG("");

pthread_mutex_lock(&close_mutex);
- shutdown(audio_sk, SHUT_RDWR);
+ audio_ipc_cleanup();
close_thread = true;
pthread_mutex_unlock(&close_mutex);

@@ -609,7 +616,7 @@ static void *ipc_handler(void *data)

if (ipc_open_cmd() == AUDIO_STATUS_FAILED) {
error("audio: Failed to open endpoint, recover");
- shutdown(audio_sk, SHUT_RDWR);
+ audio_ipc_cleanup();
continue;
}

--
1.8.4


2014-01-10 01:24:23

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH v2 2/7] android: Fix error check from pthread_create

pthread_create() returns 0 on success or errno code which is non negative
number

---
android/hal-audio.c | 4 ++--
android/hal-ipc.c | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/android/hal-audio.c b/android/hal-audio.c
index 4b48783..c71b93c 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -548,10 +548,10 @@ static int audio_open(const hw_module_t *module, const char *name,
*device = &a2dp_dev->dev.common;

err = pthread_create(&ipc_th, NULL, ipc_handler, NULL);
- if (err < 0) {
+ if (err) {
ipc_th = 0;
error("audio: Failed to start Audio IPC thread: %d (%s)",
- -err, strerror(-err));
+ err, strerror(err));
return (-err);
}

diff --git a/android/hal-ipc.c b/android/hal-ipc.c
index b19704a..97f1bcd 100644
--- a/android/hal-ipc.c
+++ b/android/hal-ipc.c
@@ -284,10 +284,10 @@ bool hal_ipc_init(void)
close(sk);

err = pthread_create(&notif_th, NULL, notification_handler, NULL);
- if (err < 0) {
+ if (err) {
notif_th = 0;
- error("Failed to start notification thread: %d (%s)", -err,
- strerror(-err));
+ error("Failed to start notification thread: %d (%s)", err,
+ strerror(err));
close(cmd_sk);
cmd_sk = -1;
close(notif_sk);
--
1.8.4


2014-01-10 01:24:22

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH v2 1/7] android/audio: Prefix error log with "audio"

---
android/hal-audio.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/android/hal-audio.c b/android/hal-audio.c
index 7d7e97e..4b48783 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -424,7 +424,8 @@ static bool create_audio_ipc(void)
sk = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
if (sk < 0) {
err = errno;
- error("Failed to create socket: %d (%s)", err, strerror(err));
+ error("audio: Failed to create socket: %d (%s)", err,
+ strerror(err));
return false;
}

@@ -436,13 +437,14 @@ static bool create_audio_ipc(void)

if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
err = errno;
- error("Failed to bind socket: %d (%s)", err, strerror(err));
+ error("audio: Failed to bind socket: %d (%s)", err,
+ strerror(err));
goto failed;
}

if (listen(sk, 1) < 0) {
err = errno;
- error("Failed to listen on the socket: %d (%s)", err,
+ error("audio: Failed to listen on the socket: %d (%s)", err,
strerror(err));
goto failed;
}
@@ -450,7 +452,7 @@ static bool create_audio_ipc(void)
audio_sk = accept(sk, NULL, NULL);
if (audio_sk < 0) {
err = errno;
- error("Failed to accept socket: %d (%s)", err, strerror(err));
+ error("audio: Failed to accept socket: %d (%s)", err, strerror(err));
goto failed;
}

@@ -471,7 +473,7 @@ static void *ipc_handler(void *data)

while (!done) {
if(!create_audio_ipc()) {
- error("Failed to create listening socket");
+ error("audio: Failed to create listening socket");
sleep(1);
continue;
}
@@ -512,7 +514,7 @@ static int audio_open(const hw_module_t *module, const char *name,
DBG("");

if (strcmp(name, AUDIO_HARDWARE_INTERFACE)) {
- error("interface %s not matching [%s]", name,
+ error("audio: interface %s not matching [%s]", name,
AUDIO_HARDWARE_INTERFACE);
return -EINVAL;
}
@@ -548,7 +550,7 @@ static int audio_open(const hw_module_t *module, const char *name,
err = pthread_create(&ipc_th, NULL, ipc_handler, NULL);
if (err < 0) {
ipc_th = 0;
- error("Failed to start Audio IPC thread: %d (%s)",
+ error("audio: Failed to start Audio IPC thread: %d (%s)",
-err, strerror(-err));
return (-err);
}
--
1.8.4