[alsa-devel] [PATCH] ucm: add cset-tlv command
This patch enables UCM to set a file in TLV format to kcontrol by: cset-tlv "name='<kcontrol-name>' <path-to-file>" This new 'cset-tlv' command will be used to write audio DSP to specific alsa control, where the driver expectes a file in TLV format.
Signed-off-by: Hsin-Yu Chao hychao@chromium.org --- src/ucm/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++--------- src/ucm/parser.c | 10 +++++++ src/ucm/ucm_local.h | 1 + 3 files changed, 78 insertions(+), 13 deletions(-)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 7e44603..a4ccb65 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -161,6 +161,47 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr, return 0; }
+static int tlv_parse(char **res, + snd_ctl_elem_info_t *info, + const char *filepath) +{ + int err = 0; + int fd; + struct stat st; + size_t sz; + ssize_t sz_read; + + if (!snd_ctl_elem_info_is_tlv_writable(info)) { + err = -EINVAL; + return err; + } + fd = open(filepath, O_RDONLY); + if (fd < 0) { + err = -errno; + return err; + } + if (stat(filepath, &st) == -1) { + err = -errno; + goto __fail; + } + sz = st.st_size; + *res = malloc(sz); + if (res == NULL) { + err = -ENOMEM; + goto __fail; + } + sz_read = read(fd, *res, sz); + if (sz_read < 0 || (size_t)sz_read != sz) { + err = -errno; + free(*res); + *res = NULL; + } + + __fail: + close(fd); + return err; +} + static int binary_file_parse(snd_ctl_elem_value_t *dst, snd_ctl_elem_info_t *info, const char *filepath) @@ -226,6 +267,7 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type) snd_ctl_elem_id_t *id; snd_ctl_elem_value_t *value; snd_ctl_elem_info_t *info; + char *res = NULL;
snd_ctl_elem_id_malloc(&id); snd_ctl_elem_value_malloc(&value); @@ -241,23 +283,32 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type) err = -EINVAL; goto __fail; } - snd_ctl_elem_value_set_id(value, id); snd_ctl_elem_info_set_id(info, id); - err = snd_ctl_elem_read(ctl, value); - if (err < 0) - goto __fail; err = snd_ctl_elem_info(ctl, info); if (err < 0) goto __fail; - if (type == SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE) - err = binary_file_parse(value, info, pos); - else - err = snd_ctl_ascii_value_parse(ctl, value, info, pos); - if (err < 0) - goto __fail; - err = snd_ctl_elem_write(ctl, value); - if (err < 0) - goto __fail; + if (type == SEQUENCE_ELEMENT_TYPE_CSET_TLV) { + err = tlv_parse(&res, info, pos); + if (err < 0) + goto __fail; + err = snd_ctl_elem_tlv_write(ctl, id, (unsigned int *)res); + if (err < 0) + goto __fail; + } else { + snd_ctl_elem_value_set_id(value, id); + err = snd_ctl_elem_read(ctl, value); + if (err < 0) + goto __fail; + if (type == SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE) + err = binary_file_parse(value, info, pos); + else + err = snd_ctl_ascii_value_parse(ctl, value, info, pos); + if (err < 0) + goto __fail; + err = snd_ctl_elem_write(ctl, value); + if (err < 0) + goto __fail; + } err = 0; __fail: if (id != NULL) @@ -266,6 +317,8 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type) free(value); if (info != NULL) free(info); + if (res != NULL) + free(res);
return err; } @@ -298,6 +351,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, break; case SEQUENCE_ELEMENT_TYPE_CSET: case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE: + case SEQUENCE_ELEMENT_TYPE_CSET_TLV: if (cdev == NULL) { char *playback_ctl = NULL; char *capture_ctl = NULL; diff --git a/src/ucm/parser.c b/src/ucm/parser.c index 9e1cb41..d781e1b 100644 --- a/src/ucm/parser.c +++ b/src/ucm/parser.c @@ -316,6 +316,16 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED, continue; }
+ if (strcmp(cmd, "cset-tlv") == 0) { + curr->type = SEQUENCE_ELEMENT_TYPE_CSET_TLV; + err = parse_string(n, &curr->data.cset); + if (err < 0) { + uc_error("error: cset-tlv requires a string!"); + return err; + } + continue; + } + if (strcmp(cmd, "usleep") == 0) { curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP; err = snd_config_get_integer(n, &curr->data.sleep); diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h index 3a5d2c2..b89de2a 100644 --- a/src/ucm/ucm_local.h +++ b/src/ucm/ucm_local.h @@ -48,6 +48,7 @@ #define SEQUENCE_ELEMENT_TYPE_SLEEP 3 #define SEQUENCE_ELEMENT_TYPE_EXEC 4 #define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE 5 +#define SEQUENCE_ELEMENT_TYPE_CSET_TLV 6
struct ucm_value { struct list_head list;
On Tue, 22 Mar 2016 10:10:34 +0100, Hsin-Yu Chao wrote:
This patch enables UCM to set a file in TLV format to kcontrol by: cset-tlv "name='<kcontrol-name>' <path-to-file>" This new 'cset-tlv' command will be used to write audio DSP to specific alsa control, where the driver expectes a file in TLV format.
Signed-off-by: Hsin-Yu Chao hychao@chromium.org
One problem in this approach is that the provided TLV data file isn't portable. Since we deal TLV as int arrays, it's endian-sensitive. At least, some endian check would be needed.
Some other nitpicks:
src/ucm/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++--------- src/ucm/parser.c | 10 +++++++ src/ucm/ucm_local.h | 1 + 3 files changed, 78 insertions(+), 13 deletions(-)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 7e44603..a4ccb65 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -161,6 +161,47 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr, return 0; }
+static int tlv_parse(char **res,
snd_ctl_elem_info_t *info,
const char *filepath)
The function name is somehow confusing. It appears as if it parses TLV. And what it actually does are two things: - check snd_ctl_elem_info_is_tlv_writable(), and - allocate the whole TLV file.
IMO, snd_ctl_elem_info_is_tlv_writable() check can be in the caller side, and this function (rename whatever better) can just do allocate and read the TLV data into a buffer.
+{
- int err = 0;
- int fd;
- struct stat st;
- size_t sz;
- ssize_t sz_read;
- if (!snd_ctl_elem_info_is_tlv_writable(info)) {
err = -EINVAL;
return err;
- }
- fd = open(filepath, O_RDONLY);
- if (fd < 0) {
err = -errno;
return err;
- }
- if (stat(filepath, &st) == -1) {
err = -errno;
goto __fail;
- }
- sz = st.st_size;
- *res = malloc(sz);
What if a crazy large file was passed? A sanity check of the file size might be good.
thanks,
Takashi
On Tue, Mar 22, 2016 at 7:48 PM, Takashi Iwai tiwai@suse.de wrote:
On Tue, 22 Mar 2016 10:10:34 +0100, Hsin-Yu Chao wrote:
This patch enables UCM to set a file in TLV format to kcontrol by: cset-tlv "name='<kcontrol-name>' <path-to-file>" This new 'cset-tlv' command will be used to write audio DSP to specific alsa control, where the driver expectes a file in TLV format.
Signed-off-by: Hsin-Yu Chao hychao@chromium.org
One problem in this approach is that the provided TLV data file isn't portable. Since we deal TLV as int arrays, it's endian-sensitive. At least, some endian check would be needed.
Thanks for the review. I think by extracting the length attribute (i.e tlv[1]) and compare it with the file size is sufficient here, since there is no way to figure out the endianness of the binary file passed in. Also I agree that additional check for crazy large or small file is necessary. According to the dsp files I test with, I'll set the tlv file size limit to between 8 bytes and 1MB. Will upload a v2 patch shortly, thanks!
Hsin-yu
Some other nitpicks:
src/ucm/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++--------- src/ucm/parser.c | 10 +++++++ src/ucm/ucm_local.h | 1 + 3 files changed, 78 insertions(+), 13 deletions(-)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 7e44603..a4ccb65 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -161,6 +161,47 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr, return 0; }
+static int tlv_parse(char **res,
snd_ctl_elem_info_t *info,
const char *filepath)
The function name is somehow confusing. It appears as if it parses TLV. And what it actually does are two things:
- check snd_ctl_elem_info_is_tlv_writable(),
and
- allocate the whole TLV file.
IMO, snd_ctl_elem_info_is_tlv_writable() check can be in the caller side, and this function (rename whatever better) can just do allocate and read the TLV data into a buffer.
+{
int err = 0;
int fd;
struct stat st;
size_t sz;
ssize_t sz_read;
if (!snd_ctl_elem_info_is_tlv_writable(info)) {
err = -EINVAL;
return err;
}
fd = open(filepath, O_RDONLY);
if (fd < 0) {
err = -errno;
return err;
}
if (stat(filepath, &st) == -1) {
err = -errno;
goto __fail;
}
sz = st.st_size;
*res = malloc(sz);
What if a crazy large file was passed? A sanity check of the file size might be good.
thanks,
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Wed, 23 Mar 2016 10:15:21 +0100, Hsin-yu Chao wrote:
On Tue, Mar 22, 2016 at 7:48 PM, Takashi Iwai tiwai@suse.de wrote:
On Tue, 22 Mar 2016 10:10:34 +0100, Hsin-Yu Chao wrote:
This patch enables UCM to set a file in TLV format to kcontrol by: cset-tlv "name='<kcontrol-name>' <path-to-file>" This new 'cset-tlv' command will be used to write audio DSP to specific alsa control, where the driver expectes a file in TLV format.
Signed-off-by: Hsin-Yu Chao hychao@chromium.org
One problem in this approach is that the provided TLV data file isn't portable. Since we deal TLV as int arrays, it's endian-sensitive. At least, some endian check would be needed.
Thanks for the review. I think by extracting the length attribute (i.e tlv[1]) and compare it with the file size is sufficient here, since there is no way to figure out the endianness of the binary file passed in.
Yeah, that should be good enough.
Also I agree that additional check for crazy large or small file is necessary. According to the dsp files I test with, I'll set the tlv file size limit to between 8 bytes and 1MB.
I guess the size will grow quickly in near future, so it'd be safer to take a bit bigger.
thanks,
Takashi
On Wed, Mar 23, 2016 at 5:21 PM, Takashi Iwai tiwai@suse.de wrote:
On Wed, 23 Mar 2016 10:15:21 +0100, Hsin-yu Chao wrote:
On Tue, Mar 22, 2016 at 7:48 PM, Takashi Iwai tiwai@suse.de wrote:
On Tue, 22 Mar 2016 10:10:34 +0100, Hsin-Yu Chao wrote:
This patch enables UCM to set a file in TLV format to kcontrol by: cset-tlv "name='<kcontrol-name>' <path-to-file>" This new 'cset-tlv' command will be used to write audio DSP to specific alsa control, where the driver expectes a file in TLV format.
Signed-off-by: Hsin-Yu Chao hychao@chromium.org
One problem in this approach is that the provided TLV data file isn't portable. Since we deal TLV as int arrays, it's endian-sensitive. At least, some endian check would be needed.
Thanks for the review. I think by extracting the length attribute (i.e tlv[1]) and compare it with the file size is sufficient here, since there is no way to figure out the endianness of the binary file passed in.
Yeah, that should be good enough.
Also I agree that additional check for crazy large or small file is necessary. According to the dsp files I test with, I'll set the tlv file size limit to between 8 bytes and 1MB.
I guess the size will grow quickly in near future, so it'd be safer to take a bit bigger.
How about using 16MB as upper limit? DSP file larger than that would take 1 second or more to load through USB 2.0.
Hsin-yu
thanks,
Takashi
On Wed, 23 Mar 2016 10:44:31 +0100, Hsin-yu Chao wrote:
On Wed, Mar 23, 2016 at 5:21 PM, Takashi Iwai tiwai@suse.de wrote:
On Wed, 23 Mar 2016 10:15:21 +0100, Hsin-yu Chao wrote:
On Tue, Mar 22, 2016 at 7:48 PM, Takashi Iwai tiwai@suse.de wrote:
On Tue, 22 Mar 2016 10:10:34 +0100, Hsin-Yu Chao wrote:
This patch enables UCM to set a file in TLV format to kcontrol by: cset-tlv "name='<kcontrol-name>' <path-to-file>" This new 'cset-tlv' command will be used to write audio DSP to specific alsa control, where the driver expectes a file in TLV format.
Signed-off-by: Hsin-Yu Chao hychao@chromium.org
One problem in this approach is that the provided TLV data file isn't portable. Since we deal TLV as int arrays, it's endian-sensitive. At least, some endian check would be needed.
Thanks for the review. I think by extracting the length attribute (i.e tlv[1]) and compare it with the file size is sufficient here, since there is no way to figure out the endianness of the binary file passed in.
Yeah, that should be good enough.
Also I agree that additional check for crazy large or small file is necessary. According to the dsp files I test with, I'll set the tlv file size limit to between 8 bytes and 1MB.
I guess the size will grow quickly in near future, so it'd be safer to take a bit bigger.
How about using 16MB as upper limit? DSP file larger than that would take 1 second or more to load through USB 2.0.
Yes, 16MB should be big enough.
thanks,
Takashi
participants (2)
-
Hsin-Yu Chao
-
Takashi Iwai