All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] ucm: add cset-tlv
@ 2016-04-13  6:26 Hsin-Yu Chao
  2016-04-13  8:57 ` Vinod Koul
  0 siblings, 1 reply; 5+ messages in thread
From: Hsin-Yu Chao @ 2016-04-13  6:26 UTC (permalink / raw)
  To: alsa-devel; +Cc: Hsin-Yu Chao

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.
The TLV file to set to kcontrol will be checked first by file size
not larger than 16 MB, and then examine if the length field reports
correct number of bytes in the TLV file.

Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
---
 src/ucm/main.c      | 93 +++++++++++++++++++++++++++++++++++++++++++++--------
 src/ucm/parser.c    | 10 ++++++
 src/ucm/ucm_local.h |  1 +
 3 files changed, 91 insertions(+), 13 deletions(-)

diff --git a/src/ucm/main.c b/src/ucm/main.c
index 7e44603..cf9e825 100644
--- a/src/ucm/main.c
+++ b/src/ucm/main.c
@@ -161,6 +161,56 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
 	return 0;
 }
 
+static int read_tlv_file(char **res,
+			 const char *filepath)
+{
+	int err = 0;
+	int fd;
+	struct stat st;
+	size_t sz;
+	ssize_t sz_read;
+	unsigned int *tlv;
+
+	fd = open(filepath, O_RDONLY);
+	if (fd < 0) {
+		err = -errno;
+		return err;
+	}
+	if (fstat(fd, &st) == -1) {
+		err = -errno;
+		goto __fail;
+	}
+	sz = st.st_size;
+	if (sz > 16 * 1024 * 1024 || sz < 8) {
+		uc_error("File size should be less than 16 MB");
+		err = -EINVAL;
+		goto __fail;
+	}
+	*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 = -EIO;
+		free(*res);
+		*res = NULL;
+	}
+	/* Check if the tlv file specifies valid size. */
+	tlv = (unsigned int *)(*res);
+	if (tlv[1] + 2 * sizeof(unsigned int) != sz) {
+		uc_error("Invalid tlv size");
+		err = -EINVAL;
+		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 +276,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 +292,36 @@ 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) {
+		if (!snd_ctl_elem_info_is_tlv_writable(info)) {
+			err = -EINVAL;
+			goto __fail;
+		}
+		err = read_tlv_file(&res, 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 +330,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 +364,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;
-- 
2.1.2

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v4] ucm: add cset-tlv
  2016-04-13  6:26 [PATCH v4] ucm: add cset-tlv Hsin-Yu Chao
@ 2016-04-13  8:57 ` Vinod Koul
  2016-04-13 10:02   ` Takashi Iwai
  0 siblings, 1 reply; 5+ messages in thread
From: Vinod Koul @ 2016-04-13  8:57 UTC (permalink / raw)
  To: Hsin-Yu Chao; +Cc: alsa-devel

On Wed, Apr 13, 2016 at 02:26:57PM +0800, Hsin-Yu Chao wrote:
> +	sz_read = read(fd, *res, sz);
> +	if (sz_read < 0 || (size_t)sz_read != sz) {
> +		err = -EIO;
> +		free(*res);
> +		*res = NULL;
> +	}
> +	/* Check if the tlv file specifies valid size. */
> +	tlv = (unsigned int *)(*res);
> +	if (tlv[1] + 2 * sizeof(unsigned int) != sz) {

why not use snd_ctl_tlv(), my assumption is that you are expecting a type,
length and values in the buffer

> +		uc_error("Invalid tlv size");

wouldn't it help to print the size for error

> +		err = -EINVAL;
> +		free(*res);
> +		*res = NULL;
> +	}
> +
> +      __fail:

I dont know about syntax expected in UCM but this label could be left
justfied :)

> +	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 +276,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 +292,36 @@ 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) {
> +		if (!snd_ctl_elem_info_is_tlv_writable(info)) {
> +			err = -EINVAL;
> +			goto __fail;
> +		}
> +		err = read_tlv_file(&res, pos);
> +		if (err < 0)
> +			goto __fail;
> +		err = snd_ctl_elem_tlv_write(ctl, id, (unsigned int *)res);

if tlv_write expects unsigned int then wouldn't it be better to use
read_tlv_file() with unsigned int.

-- 
~Vinod

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v4] ucm: add cset-tlv
  2016-04-13  8:57 ` Vinod Koul
@ 2016-04-13 10:02   ` Takashi Iwai
  2016-04-13 10:15     ` Hsin-yu Chao
  0 siblings, 1 reply; 5+ messages in thread
From: Takashi Iwai @ 2016-04-13 10:02 UTC (permalink / raw)
  To: Vinod Koul; +Cc: alsa-devel, Hsin-Yu Chao

On Wed, 13 Apr 2016 10:57:40 +0200,
Vinod Koul wrote:
> 
> On Wed, Apr 13, 2016 at 02:26:57PM +0800, Hsin-Yu Chao wrote:
> > +	sz_read = read(fd, *res, sz);
> > +	if (sz_read < 0 || (size_t)sz_read != sz) {
> > +		err = -EIO;
> > +		free(*res);
> > +		*res = NULL;
> > +	}
> > +	/* Check if the tlv file specifies valid size. */
> > +	tlv = (unsigned int *)(*res);
> > +	if (tlv[1] + 2 * sizeof(unsigned int) != sz) {
> 
> why not use snd_ctl_tlv(), my assumption is that you are expecting a type,
> length and values in the buffer

Actually this implicitly checks the endianess of the read data.


Takashi

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v4] ucm: add cset-tlv
  2016-04-13 10:02   ` Takashi Iwai
@ 2016-04-13 10:15     ` Hsin-yu Chao
  2016-04-13 10:22       ` Takashi Iwai
  0 siblings, 1 reply; 5+ messages in thread
From: Hsin-yu Chao @ 2016-04-13 10:15 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Vinod Koul, alsa-devel

On Wed, Apr 13, 2016 at 6:02 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 13 Apr 2016 10:57:40 +0200,
> Vinod Koul wrote:
>>
>> On Wed, Apr 13, 2016 at 02:26:57PM +0800, Hsin-Yu Chao wrote:
>> > +   sz_read = read(fd, *res, sz);
>> > +   if (sz_read < 0 || (size_t)sz_read != sz) {
>> > +           err = -EIO;
>> > +           free(*res);
>> > +           *res = NULL;
>> > +   }
>> > +   /* Check if the tlv file specifies valid size. */
>> > +   tlv = (unsigned int *)(*res);
>> > +   if (tlv[1] + 2 * sizeof(unsigned int) != sz) {
>>
>> why not use snd_ctl_tlv(), my assumption is that you are expecting a type,
>> length and values in the buffer
>
> Actually this implicitly checks the endianess of the read data.
>
>
> Takashi
Thanks for this useful info!
I read the comment regarding the length of this snd_ctl_tlv struct,
saying that "in bytes aligned to 4", but I don't see anywhere in the
tlv_write path asserts this.
Should I also add a check here that files size is a multiple of 4?

Hsin-yu

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v4] ucm: add cset-tlv
  2016-04-13 10:15     ` Hsin-yu Chao
@ 2016-04-13 10:22       ` Takashi Iwai
  0 siblings, 0 replies; 5+ messages in thread
From: Takashi Iwai @ 2016-04-13 10:22 UTC (permalink / raw)
  To: Hsin-yu Chao; +Cc: Vinod Koul, alsa-devel

On Wed, 13 Apr 2016 12:15:45 +0200,
Hsin-yu Chao wrote:
> 
> On Wed, Apr 13, 2016 at 6:02 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 13 Apr 2016 10:57:40 +0200,
> > Vinod Koul wrote:
> >>
> >> On Wed, Apr 13, 2016 at 02:26:57PM +0800, Hsin-Yu Chao wrote:
> >> > +   sz_read = read(fd, *res, sz);
> >> > +   if (sz_read < 0 || (size_t)sz_read != sz) {
> >> > +           err = -EIO;
> >> > +           free(*res);
> >> > +           *res = NULL;
> >> > +   }
> >> > +   /* Check if the tlv file specifies valid size. */
> >> > +   tlv = (unsigned int *)(*res);
> >> > +   if (tlv[1] + 2 * sizeof(unsigned int) != sz) {
> >>
> >> why not use snd_ctl_tlv(), my assumption is that you are expecting a type,
> >> length and values in the buffer
> >
> > Actually this implicitly checks the endianess of the read data.
> >
> >
> > Takashi
> Thanks for this useful info!
> I read the comment regarding the length of this snd_ctl_tlv struct,
> saying that "in bytes aligned to 4", but I don't see anywhere in the
> tlv_write path asserts this.
> Should I also add a check here that files size is a multiple of 4?

Yes, it makes sense.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-04-13 10:22 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-13  6:26 [PATCH v4] ucm: add cset-tlv Hsin-Yu Chao
2016-04-13  8:57 ` Vinod Koul
2016-04-13 10:02   ` Takashi Iwai
2016-04-13 10:15     ` Hsin-yu Chao
2016-04-13 10:22       ` Takashi Iwai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.