[PATCH v3 00/10] topology: decode: Various fixes

[PATCH v3 00/10] topology: decode: Various fixes

[Piotr Maziarz]

This series fixes various problems with topology decoding mechanism.
Some of the problems were critical like improper memory management or
infinite loops that were causing undefined behaviour or program crashes,
while other resulted in losing some data during conversion.

Bugs found while testing with Intel SST topologies.

Changelog:
v2:
  -Divide into more patches, critical fixes are in separate patches now
  -More specific descriptions
  -Fix a typo UML to UCM
  -Add error reporting in topology: decode: fix channel map memory
   allocation
  -Remove goto again in topology: Make buffer for saving dynamic size
   for better readability

v3:
  -No functional changes
  -Changed UCM to more descriptive standard ALSA configuration files
  -removed Gerrit's Change-Id
  -Added missing signed-offs

Piotr Maziarz (10):
  topology: decode: Fix channel map memory allocation
  topology: decode: Fix infinite loop in decoding enum control
  topology: decode: Remove decoding  values for enum control
  topology: decode: Add enum control texts as separate element
  topology: decode: Fix printing texts section
  topology: decode: Change declaration of enum decoding function
  topology: decode: Fix decoding PCM formats and rates
  topology: decode: Print sig_bits field in PCM capabilities section
  topology: decode: Add DAI name printing
  topology: Make buffer for saving dynamic size

 src/topology/ctl.c        | 51 ++++++++++++++++++++++-------------------------
 src/topology/dapm.c       |  3 +--
 src/topology/pcm.c        | 11 +++++++---
 src/topology/save.c       | 34 ++++++++++++++++++++++++++-----
 src/topology/text.c       |  2 +-
 src/topology/tplg_local.h |  2 +-
 6 files changed, 64 insertions(+), 39 deletions(-)

-- 
2.7.4

[PATCH v3 01/10] topology: decode: Fix channel map memory allocation

[Piotr Maziarz]

Memory allocated on the stack was referenced outside of the function scope
caused undefined behaviour.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/ctl.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/topology/ctl.c b/src/topology/ctl.c
index 90241b6..6e6c1d1 100644
--- a/src/topology/ctl.c
+++ b/src/topology/ctl.c
@@ -1330,7 +1330,6 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg,
 			      void *bin, size_t size)
 {
 	struct snd_soc_tplg_enum_control *ec = bin;
-	struct snd_tplg_channel_map_template cmt;
 	int i;
 
 	if (size < sizeof(*ec)) {
@@ -1375,11 +1374,13 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg,
 		}
 	}
 
-	et->map = &cmt;
-	memset(&cmt, 0, sizeof(cmt));
-	cmt.num_channels = ec->num_channels;
-	for (i = 0; i < cmt.num_channels; i++) {
-		struct snd_tplg_channel_elem *channel = &cmt.channel[i];
+	et->map = tplg_calloc(heap, sizeof(struct snd_tplg_channel_map_template));
+	if (!et->map)
+		return -ENOMEM;
+	et->map->num_channels = ec->num_channels;
+	for (i = 0; i < et->map->num_channels; i++) {
+		struct snd_tplg_channel_elem *channel = &et->map->channel[i];
+
 		tplg_log(tplg, 'D', pos + ((void *)&ec->channel[i] - (void *)ec),
 			 "enum: channel size %d", ec->channel[i].size);
 		channel->reg = ec->channel[i].reg;
-- 
2.7.4

[PATCH v3 02/10] topology: decode: Fix infinite loop in decoding enum control

[Piotr Maziarz]

Accessing memory outside of allocated boundaries caused segmentation fault.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/ctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/topology/ctl.c b/src/topology/ctl.c
index 6e6c1d1..0aa49ab 100644
--- a/src/topology/ctl.c
+++ b/src/topology/ctl.c
@@ -1367,7 +1367,7 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg,
 		et->texts = tplg_calloc(heap, sizeof(char *) * ec->items);
 		if (!et->texts)
 			return -ENOMEM;
-		for (i = 0; ec->items; i++) {
+		for (i = 0; i < ec->items; i++) {
 			unsigned int j = i * sizeof(int) * ENUM_VAL_SIZE;
 			et->texts[i] = ec->texts[i];
 			et->values[i] = (int *)&ec->values[j];
-- 
2.7.4

[PATCH v3 03/10] topology: decode: Remove decoding values for enum control

[Piotr Maziarz]

Values have no representation in standard ALSA configuration files,
therefore there is no need to populate them. Also memory for values
wasn't allocated which was causing undefined behaviour.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/ctl.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/topology/ctl.c b/src/topology/ctl.c
index 0aa49ab..02e482e 100644
--- a/src/topology/ctl.c
+++ b/src/topology/ctl.c
@@ -1367,11 +1367,8 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg,
 		et->texts = tplg_calloc(heap, sizeof(char *) * ec->items);
 		if (!et->texts)
 			return -ENOMEM;
-		for (i = 0; i < ec->items; i++) {
-			unsigned int j = i * sizeof(int) * ENUM_VAL_SIZE;
+		for (i = 0; i < ec->items; i++)
 			et->texts[i] = ec->texts[i];
-			et->values[i] = (int *)&ec->values[j];
-		}
 	}
 
 	et->map = tplg_calloc(heap, sizeof(struct snd_tplg_channel_map_template));
-- 
2.7.4

[PATCH v3 04/10] topology: decode: Add enum control texts as separate element

[Piotr Maziarz]

Texts are separate sections that should referenced by enum control.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/ctl.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/topology/ctl.c b/src/topology/ctl.c
index 02e482e..1f39846 100644
--- a/src/topology/ctl.c
+++ b/src/topology/ctl.c
@@ -1088,11 +1088,19 @@ int tplg_add_enum(snd_tplg_t *tplg, struct snd_tplg_enum_template *enum_ctl,
 	}
 
 	if (enum_ctl->texts != NULL) {
+		struct tplg_elem *texts = tplg_elem_new_common(tplg, NULL,
+						enum_ctl->hdr.name, SND_TPLG_TYPE_TEXT);
+
+		texts->texts->num_items = num_items;
 		for (i = 0; i < num_items; i++) {
-			if (enum_ctl->texts[i] != NULL)
-				snd_strlcpy(ec->texts[i], enum_ctl->texts[i],
-					    SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
+			if (!enum_ctl->texts[i])
+				continue;
+			snd_strlcpy(ec->texts[i], enum_ctl->texts[i],
+				    SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
+			snd_strlcpy(texts->texts->items[i], enum_ctl->texts[i],
+				    SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
 		}
+		tplg_ref_add(elem, SND_TPLG_TYPE_TEXT, enum_ctl->hdr.name);
 	}
 
 	if (enum_ctl->values != NULL) {
-- 
2.7.4

[PATCH v3 05/10] topology: decode: Fix printing texts section

[Piotr Maziarz]

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/text.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/topology/text.c b/src/topology/text.c
index 507c545..b899b28 100644
--- a/src/topology/text.c
+++ b/src/topology/text.c
@@ -103,7 +103,7 @@ int tplg_save_text(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
 		return 0;
 	err = tplg_save_printf(dst, pfx, "'%s'.values [\n", elem->id);
 	for (i = 0; err >= 0 && i < texts->num_items; i++)
-		err = tplg_save_printf(dst, pfx, "\t'%s'\n", texts->items[i][0]);
+		err = tplg_save_printf(dst, pfx, "\t'%s'\n", texts->items[i]);
 	if (err >= 0)
 		err = tplg_save_printf(dst, pfx, "]\n");
 	return err;
-- 
2.7.4

[PATCH v3 06/10] topology: decode: Change declaration of enum decoding function

[Piotr Maziarz]

Size constraints are always checked before invoking
tplg_decode_control_enum1. There is no need to validate it twice.
Alos moved debug print about size to invoking function, since now it's it
responsibility to check size.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/ctl.c        | 19 +++++--------------
 src/topology/dapm.c       |  3 +--
 src/topology/tplg_local.h |  2 +-
 3 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/src/topology/ctl.c b/src/topology/ctl.c
index 1f39846..47db400 100644
--- a/src/topology/ctl.c
+++ b/src/topology/ctl.c
@@ -1335,22 +1335,10 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg,
 			      struct list_head *heap,
 			      struct snd_tplg_enum_template *et,
 			      size_t pos,
-			      void *bin, size_t size)
+			      struct snd_soc_tplg_enum_control *ec)
 {
-	struct snd_soc_tplg_enum_control *ec = bin;
 	int i;
 
-	if (size < sizeof(*ec)) {
-		SNDERR("enum: small size %d", size);
-		return -EINVAL;
-	}
-
-	tplg_log(tplg, 'D', pos, "enum: size %d private size %d",
-		 ec->size, ec->priv.size);
-	if (size != ec->size + ec->priv.size) {
-		SNDERR("enum: unexpected element size %d", size);
-		return -EINVAL;
-	}
 	if (ec->num_channels > SND_TPLG_MAX_CHAN ||
 	    ec->num_channels > SND_SOC_TPLG_MAX_CHAN) {
 		SNDERR("enum: unexpected channel count %d", ec->num_channels);
@@ -1427,7 +1415,10 @@ next:
 		return -EINVAL;
 	}
 
-	err = tplg_decode_control_enum1(tplg, &heap, &et, pos, bin, size);
+	tplg_log(tplg, 'D', pos, "enum: size %d private size %d",
+		 ec->size, ec->priv.size);
+
+	err = tplg_decode_control_enum1(tplg, &heap, &et, pos, ec);
 	if (err >= 0) {
 		t.enum_ctl = &et;
 		err = snd_tplg_add_object(tplg, &t);
diff --git a/src/topology/dapm.c b/src/topology/dapm.c
index cd1a877..73a9390 100644
--- a/src/topology/dapm.c
+++ b/src/topology/dapm.c
@@ -972,8 +972,7 @@ next:
 				err = -EINVAL;
 				goto retval;
 			}
-			err = tplg_decode_control_enum1(tplg, &heap, et, pos,
-							bin, size2);
+			err = tplg_decode_control_enum1(tplg, &heap, et, pos, ec);
 			break;
 		case SND_SOC_TPLG_TYPE_BYTES:
 			bt = tplg_calloc(&heap, sizeof(*bt));
diff --git a/src/topology/tplg_local.h b/src/topology/tplg_local.h
index 5ace0d1..acb01a8 100644
--- a/src/topology/tplg_local.h
+++ b/src/topology/tplg_local.h
@@ -398,7 +398,7 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg,
 			      struct list_head *heap,
 			      struct snd_tplg_enum_template *et,
 			      size_t pos,
-			      void *bin, size_t size);
+			      struct snd_soc_tplg_enum_control *ec);
 int tplg_decode_control_enum(snd_tplg_t *tplg, size_t pos,
 			     struct snd_soc_tplg_hdr *hdr,
 			     void *bin, size_t size);
-- 
2.7.4

[PATCH v3 07/10] topology: decode: Fix decoding PCM formats and rates

[Piotr Maziarz]

Not checking _LAST format and rate, which are valid indexes in arrays,
makes data loss while converting binary to standard ALSA configuration
file.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/pcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/topology/pcm.c b/src/topology/pcm.c
index b15b950..db40114 100644
--- a/src/topology/pcm.c
+++ b/src/topology/pcm.c
@@ -549,7 +549,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
 	if (err >= 0 && sc->formats) {
 		err = tplg_save_printf(dst, pfx, "\tformats '");
 		first = 1;
-		for (i = 0; err >= 0 && i < SND_PCM_FORMAT_LAST; i++) {
+		for (i = 0; err >= 0 && i <= SND_PCM_FORMAT_LAST; i++) {
 			if (sc->formats & (1ULL << i)) {
 				s = snd_pcm_format_name(i);
 				err = tplg_save_printf(dst, NULL, "%s%s",
@@ -563,7 +563,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
 	if (err >= 0 && sc->rates) {
 		err = tplg_save_printf(dst, pfx, "\trates '");
 		first = 1;
-		for (i = 0; err >= 0 && i < SND_PCM_RATE_LAST; i++) {
+		for (i = 0; err >= 0 && i <= SND_PCM_RATE_LAST; i++) {
 			if (sc->rates & (1ULL << i)) {
 				s = get_rate_name(i);
 				err = tplg_save_printf(dst, NULL, "%s%s",
-- 
2.7.4

[PATCH v3 08/10] topology: decode: Print sig_bits field in PCM capabilities section

[Piotr Maziarz]

Not printing this field makes data loss while converting from binary
to standard ALSA configuration file.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/pcm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/topology/pcm.c b/src/topology/pcm.c
index db40114..49c5eab 100644
--- a/src/topology/pcm.c
+++ b/src/topology/pcm.c
@@ -604,6 +604,9 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
 	if (err >= 0 && sc->buffer_size_max)
 		err = tplg_save_printf(dst, pfx, "\tbuffer_size_max %u\n",
 				       sc->buffer_size_max);
+	if (err >= 0 && sc->sig_bits)
+		err = tplg_save_printf(dst, pfx, "\tsig_bits %u\n",
+				       sc->sig_bits);
 	if (err >= 0)
 		err = tplg_save_printf(dst, pfx, "}\n");
 	return err;
-- 
2.7.4

[PATCH v3 09/10] topology: decode: Add DAI name printing

[Piotr Maziarz]

DAI name is a part of topology binary. Not printing makes data loss while
converting from binary to standard ALSA configuration file.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/pcm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/topology/pcm.c b/src/topology/pcm.c
index 49c5eab..5a54e15 100644
--- a/src/topology/pcm.c
+++ b/src/topology/pcm.c
@@ -781,7 +781,9 @@ int tplg_save_fe_dai(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
 	struct snd_soc_tplg_pcm *pcm = elem->pcm;
 	int err = 0;
 
-	if (pcm->dai_id > 0)
+	if (strlen(pcm->dai_name))
+		err = tplg_save_printf(dst, pfx, "dai.'%s'.id %u\n", pcm->dai_name, pcm->dai_id);
+	else if (pcm->dai_id > 0)
 		err = tplg_save_printf(dst, pfx, "dai.0.id %u\n", pcm->dai_id);
 	return err;
 }
-- 
2.7.4

[PATCH v3 10/10] topology: Make buffer for saving dynamic size

[Piotr Maziarz]

Some fields can exceed size limit, e.g. private data has no size
restriction. Therefore it needs to be dynamically increased.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/save.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/src/topology/save.c b/src/topology/save.c
index 4ecf86c..9c74735 100644
--- a/src/topology/save.c
+++ b/src/topology/save.c
@@ -19,22 +19,43 @@
 #include "tplg_local.h"
 
 #define SAVE_ALLOC_SHIFT	(13)	/* 8192 bytes */
+#define PRINT_BUF_SIZE		(1024)
+#define PRINT_BUF_SIZE_MAX	(1024 * 1024)
 
 int tplg_save_printf(char **dst, const char *pfx, const char *fmt, ...)
 {
 	va_list va;
-	char buf[1024], *s;
+	char *buf, *s;
 	size_t n, l, t, pl;
+	int ret = 0;
+
+	buf = malloc(PRINT_BUF_SIZE);
+	if (!buf)
+		return -ENOMEM;
 
 	if (pfx == NULL)
 		pfx = "";
 
 	va_start(va, fmt);
-	n = vsnprintf(buf, sizeof(buf), fmt, va);
+	n = vsnprintf(buf, PRINT_BUF_SIZE, fmt, va);
 	va_end(va);
 
-	if (n >= sizeof(buf))
-		return -EOVERFLOW;
+	if (n >= PRINT_BUF_SIZE_MAX) {
+		ret = -EOVERFLOW;
+		goto end;
+	}
+
+	if (n >= PRINT_BUF_SIZE) {
+		char *tmp = realloc(buf, n + 1);
+		if (!tmp) {
+			ret = -ENOMEM;
+			goto end;
+		}
+		buf = tmp;
+		va_start(va, fmt);
+		n = vsnprintf(buf, n + 1, fmt, va);
+		va_end(va);
+	}
 
 	pl = strlen(pfx);
 	l = *dst ? strlen(*dst) : 0;
@@ -47,7 +68,8 @@ int tplg_save_printf(char **dst, const char *pfx, const char *fmt, ...)
 		if (s == NULL) {
 			free(*dst);
 			*dst = NULL;
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto end;
 		}
 	} else {
 		s = *dst;
@@ -57,6 +79,8 @@ int tplg_save_printf(char **dst, const char *pfx, const char *fmt, ...)
 		strcpy(s + l, pfx);
 	strcpy(s + l + pl, buf);
 	*dst = s;
+end:
+	free(buf);
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH v3 07/10] topology: decode: Fix decoding PCM formats and rates

[Pierre-Louis Bossart]

On 7/14/20 6:25 AM, Piotr Maziarz wrote:
> Not checking _LAST format and rate, which are valid indexes in arrays,
> makes data loss while converting binary to standard ALSA configuration
> file.

I must be really thick on this one.

alsatplg converts from alsa-conf format to binary topology file.
The binary topology file is used by drivers.

In which cases would you convert from binary to alsa-conf files? And 
what tool would you use?


> Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
> ---
>   src/topology/pcm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/topology/pcm.c b/src/topology/pcm.c
> index b15b950..db40114 100644
> --- a/src/topology/pcm.c
> +++ b/src/topology/pcm.c
> @@ -549,7 +549,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
>   	if (err >= 0 && sc->formats) {
>   		err = tplg_save_printf(dst, pfx, "\tformats '");
>   		first = 1;
> -		for (i = 0; err >= 0 && i < SND_PCM_FORMAT_LAST; i++) {
> +		for (i = 0; err >= 0 && i <= SND_PCM_FORMAT_LAST; i++) {
>   			if (sc->formats & (1ULL << i)) {
>   				s = snd_pcm_format_name(i);
>   				err = tplg_save_printf(dst, NULL, "%s%s",
> @@ -563,7 +563,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
>   	if (err >= 0 && sc->rates) {
>   		err = tplg_save_printf(dst, pfx, "\trates '");
>   		first = 1;
> -		for (i = 0; err >= 0 && i < SND_PCM_RATE_LAST; i++) {
> +		for (i = 0; err >= 0 && i <= SND_PCM_RATE_LAST; i++) {
>   			if (sc->rates & (1ULL << i)) {
>   				s = get_rate_name(i);
>   				err = tplg_save_printf(dst, NULL, "%s%s",
> 

Re: [PATCH v3 07/10] topology: decode: Fix decoding PCM formats and rates

[Piotr Maziarz]

On 2020-07-14 17:40, Pierre-Louis Bossart wrote:
> 
> 
> On 7/14/20 6:25 AM, Piotr Maziarz wrote:
>> Not checking _LAST format and rate, which are valid indexes in arrays,
>> makes data loss while converting binary to standard ALSA configuration
>> file.
> 
> I must be really thick on this one.
> 
> alsatplg converts from alsa-conf format to binary topology file.
> The binary topology file is used by drivers.
> 
> In which cases would you convert from binary to alsa-conf files? And 
> what tool would you use?
> 
./alsatplg --decode topology.bin --output decoded_topology.conf,
This feature was added around the end of 2019. And why to use it? For 
binary topologies to which conf files are lost for example. It's easier 
to analyze and edit it in conf than directly in binary.

> 
>> Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
>> ---
>>   src/topology/pcm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/topology/pcm.c b/src/topology/pcm.c
>> index b15b950..db40114 100644
>> --- a/src/topology/pcm.c
>> +++ b/src/topology/pcm.c
>> @@ -549,7 +549,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg 
>> ATTRIBUTE_UNUSED,
>>       if (err >= 0 && sc->formats) {
>>           err = tplg_save_printf(dst, pfx, "\tformats '");
>>           first = 1;
>> -        for (i = 0; err >= 0 && i < SND_PCM_FORMAT_LAST; i++) {
>> +        for (i = 0; err >= 0 && i <= SND_PCM_FORMAT_LAST; i++) {
>>               if (sc->formats & (1ULL << i)) {
>>                   s = snd_pcm_format_name(i);
>>                   err = tplg_save_printf(dst, NULL, "%s%s",
>> @@ -563,7 +563,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg 
>> ATTRIBUTE_UNUSED,
>>       if (err >= 0 && sc->rates) {
>>           err = tplg_save_printf(dst, pfx, "\trates '");
>>           first = 1;
>> -        for (i = 0; err >= 0 && i < SND_PCM_RATE_LAST; i++) {
>> +        for (i = 0; err >= 0 && i <= SND_PCM_RATE_LAST; i++) {
>>               if (sc->rates & (1ULL << i)) {
>>                   s = get_rate_name(i);
>>                   err = tplg_save_printf(dst, NULL, "%s%s",
>>

Re: [PATCH v3 07/10] topology: decode: Fix decoding PCM formats and rates

[Pierre-Louis Bossart]

On 7/15/20 4:37 AM, Piotr Maziarz wrote:
> On 2020-07-14 17:40, Pierre-Louis Bossart wrote:
>>
>>
>> On 7/14/20 6:25 AM, Piotr Maziarz wrote:
>>> Not checking _LAST format and rate, which are valid indexes in arrays,
>>> makes data loss while converting binary to standard ALSA configuration
>>> file.
>>
>> I must be really thick on this one.
>>
>> alsatplg converts from alsa-conf format to binary topology file.
>> The binary topology file is used by drivers.
>>
>> In which cases would you convert from binary to alsa-conf files? And 
>> what tool would you use?
>>
> ./alsatplg --decode topology.bin --output decoded_topology.conf,
> This feature was added around the end of 2019. And why to use it? For 
> binary topologies to which conf files are lost for example. It's easier 
> to analyze and edit it in conf than directly in binary.

I must admit I completely missed this feature, thanks for the clarification.

Re: [PATCH v3 00/10] topology: decode: Various fixes

[Pierre-Louis Bossart]

On 7/14/20 6:25 AM, Piotr Maziarz wrote:
> This series fixes various problems with topology decoding mechanism.
> Some of the problems were critical like improper memory management or
> infinite loops that were causing undefined behaviour or program crashes,
> while other resulted in losing some data during conversion.
> 
> Bugs found while testing with Intel SST topologies.

I am far from the expert on topology but the patches look fine to me, so 
adding my tag so that others may chime in:

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

> 
> Changelog:
> v2:
>    -Divide into more patches, critical fixes are in separate patches now
>    -More specific descriptions
>    -Fix a typo UML to UCM
>    -Add error reporting in topology: decode: fix channel map memory
>     allocation
>    -Remove goto again in topology: Make buffer for saving dynamic size
>     for better readability
> 
> v3:
>    -No functional changes
>    -Changed UCM to more descriptive standard ALSA configuration files
>    -removed Gerrit's Change-Id
>    -Added missing signed-offs
> 
> Piotr Maziarz (10):
>    topology: decode: Fix channel map memory allocation
>    topology: decode: Fix infinite loop in decoding enum control
>    topology: decode: Remove decoding  values for enum control
>    topology: decode: Add enum control texts as separate element
>    topology: decode: Fix printing texts section
>    topology: decode: Change declaration of enum decoding function
>    topology: decode: Fix decoding PCM formats and rates
>    topology: decode: Print sig_bits field in PCM capabilities section
>    topology: decode: Add DAI name printing
>    topology: Make buffer for saving dynamic size
> 
>   src/topology/ctl.c        | 51 ++++++++++++++++++++++-------------------------
>   src/topology/dapm.c       |  3 +--
>   src/topology/pcm.c        | 11 +++++++---
>   src/topology/save.c       | 34 ++++++++++++++++++++++++++-----
>   src/topology/text.c       |  2 +-
>   src/topology/tplg_local.h |  2 +-
>   6 files changed, 64 insertions(+), 39 deletions(-)
> 

Re: [PATCH v3 07/10] topology: decode: Fix decoding PCM formats and rates

[Cezary Rojewski]

On 2020-07-15 4:37 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 7/15/20 4:37 AM, Piotr Maziarz wrote:
>> On 2020-07-14 17:40, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 7/14/20 6:25 AM, Piotr Maziarz wrote:
>>>> Not checking _LAST format and rate, which are valid indexes in arrays,
>>>> makes data loss while converting binary to standard ALSA configuration
>>>> file.
>>>
>>> I must be really thick on this one.
>>>
>>> alsatplg converts from alsa-conf format to binary topology file.
>>> The binary topology file is used by drivers.
>>>
>>> In which cases would you convert from binary to alsa-conf files? And 
>>> what tool would you use?
>>>
>> ./alsatplg --decode topology.bin --output decoded_topology.conf,
>> This feature was added around the end of 2019. And why to use it? For 
>> binary topologies to which conf files are lost for example. It's 
>> easier to analyze and edit it in conf than directly in binary.
> 
> I must admit I completely missed this feature, thanks for the 
> clarification.

In general, the idea is to be able to validate or debug (if necessary) 
topology binaries provided by users when access to FE file e.g. conf, or 
XML in our case, is not possible.

In perfect world one can do the following and receive the exact same 
results on each iteration:


(assume FE file in XML format and FE tool e.g. itt which allows for
converting XML into conf)

XML -> itt -> UCM
UCM -> alsatplg -> bin

bin -> alsatplg -> UCM
UCM -> itt -> XML

Ability to compile and decompile was very handy in the Android world. We 
developed a simplified approach quite a while ago and finally decided to 
upstream the solution. Turned out Jaroslav pushed few commits lately 
that make a stub for the idea itself. Unfortunately, as you see in the 
series, there are several problems with the existing code rendering 
--decode unusable. Next step, after the fixes, is to allow for custom 
handlers to be provided (decompiling vendor's private data).

Czarek

Re: [PATCH v3 00/10] topology: decode: Various fixes

[Cezary Rojewski]

On 2020-07-14 1:25 PM, Piotr Maziarz wrote:
> This series fixes various problems with topology decoding mechanism.
> Some of the problems were critical like improper memory management or
> infinite loops that were causing undefined behaviour or program crashes,
> while other resulted in losing some data during conversion.
> 
> Bugs found while testing with Intel SST topologies.
> 
> Changelog:
> v2:
>    -Divide into more patches, critical fixes are in separate patches now
>    -More specific descriptions
>    -Fix a typo UML to UCM
>    -Add error reporting in topology: decode: fix channel map memory
>     allocation
>    -Remove goto again in topology: Make buffer for saving dynamic size
>     for better readability
> 
> v3:
>    -No functional changes
>    -Changed UCM to more descriptive standard ALSA configuration files
>    -removed Gerrit's Change-Id
>    -Added missing signed-offs
> 
> Piotr Maziarz (10):
>    topology: decode: Fix channel map memory allocation
>    topology: decode: Fix infinite loop in decoding enum control
>    topology: decode: Remove decoding  values for enum control
>    topology: decode: Add enum control texts as separate element
>    topology: decode: Fix printing texts section
>    topology: decode: Change declaration of enum decoding function
>    topology: decode: Fix decoding PCM formats and rates
>    topology: decode: Print sig_bits field in PCM capabilities section
>    topology: decode: Add DAI name printing
>    topology: Make buffer for saving dynamic size
> 
>   src/topology/ctl.c        | 51 ++++++++++++++++++++++-------------------------
>   src/topology/dapm.c       |  3 +--
>   src/topology/pcm.c        | 11 +++++++---
>   src/topology/save.c       | 34 ++++++++++++++++++++++++++-----
>   src/topology/text.c       |  2 +-
>   src/topology/tplg_local.h |  2 +-
>   6 files changed, 64 insertions(+), 39 deletions(-)
> 

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>