[alsa-devel] [PATCH - UCM 1/1] ucm: add binary configure file parse

[Takashi Iwai]

At Wed, 21 Jan 2015 01:12:27 +0000,
Lu, Han wrote:
> 
> Hi Takashi,
> 
> BR,
> Han Lu
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Tuesday, January 20, 2015 5:19 PM
> > To: Lu, Han
> > Cc: Girdwood, Liam R; alsa-devel at alsa-project.org
> > Subject: Re: [PATCH - UCM 1/1] ucm: add binary configure file parse
> > 
> > At Tue, 20 Jan 2015 16:33:09 +0800,
> > han.lu at intel.com wrote:
> > >
> > > From: "Lu, Han" <han.lu at intel.com>
> > >
> > > with cset command, UCM set kcontrol parameters directly:
> > >     cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
> > > This patch enables UCM to set kcontrol with parameters from configure
> > > file:
> > >     cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
> > > where "cset-bin-file" is a newly added keyword alongside of "cset", to
> > > indicate cset with binary data in file.
> > > The binary data in file is parameter for audio DSPs, and it's just
> > > passed by UCM/ALSA as raw data. The data type of the parameter element
> > > must be byte, and the count is limited by the size of struct
> > > snd_ctl_elem_value and driver definition.
> > >
> > > Signed-off-by: Lu, Han <han.lu at intel.com>
> > >
> > > diff --git a/src/ucm/main.c b/src/ucm/main.c index 37ae4c8..818465a
> > > 100644
> > > --- a/src/ucm/main.c
> > > +++ b/src/ucm/main.c
> > > @@ -160,11 +160,62 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
> > >  	return 0;
> > >  }
> > >
> > > +static int binary_file_parse(snd_ctl_elem_value_t *dst,
> > > +			      snd_ctl_elem_info_t *info,
> > > +			      const char *filepath)
> > > +{
> > > +	int err, fd;
> > > +	struct stat st;
> > > +	size_t sz;
> > > +	char *res;
> > > +	snd_ctl_elem_type_t type;
> > > +	unsigned int idx, count;
> > > +
> > > +	type = snd_ctl_elem_info_get_type(info);
> > > +	if (type != SND_CTL_ELEM_TYPE_BYTES) {
> > > +		uc_error("only support byte type!");
> > > +		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;
> > > +	count = snd_ctl_elem_info_get_count(info);
> > > +	if (sz == 0 || sz > count || sz > sizeof(dst->value.bytes)) {
> > > +		uc_error("invalid parameter size!");
> > 
> > A question here is what if the file size is smaller than the expected data size.
> > Should we allow this explicitly?  I thought it'd be safer to bail out, but I'd like
> > to know more about the scenarios behind this patch.
> > 
> 
> The scenario here is to load multiple (~160) binary configure files, but their length may be variable from about 10 to 512 bytes. (most of them is about 10 to 100 bytes) 
> In this case, the count defined in driver have to be 512. If I return when size != 512, all configure files must be extended to 512 bytes, it will take about 80 Kbytes. So I used 512 as a range. But Liam and I agreed that it will be safer to bail out in this case. I will change the patch. Thanks.

OK.  I wasn't objecting to allowing smaller sizes, but just wondered.
Meanwhile, limiting the size is obviously safer.  If this limitation
would become a real problem, we can loosen the condition somehow
later, which is much easier than making more strict later.


thanks,

Takashi