| Message ID | 1458637834-15947-1-git-send-email-hychao@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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;
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(-)