fixup! ASoC: SOF: Introduce extended manifest

[ktrzcinx]

Function should return error code or extended manifest size.

Reported-by: Marc Herbert marc.herbert@intel.com
Signed-off-by: Karol Trzcinski karolx.trzcinski@linux.intel.com

[keyonjie]

I would suggest to change like this to make the logic more clear and simple:

diff --git a/sound/soc/sof/loader.c b/sound/soc/sof/loader.c
index a9ce2dcb747c..373c4881b3cd 100644
--- a/sound/soc/sof/loader.c
+++ b/sound/soc/sof/loader.c
@@ -264,7 +264,7 @@ static int snd_sof_fw_ext_man_parse(struct snd_sof_dev *sdev,
                if (ret < 0) {
                        dev_err(sdev->dev, "error: failed to parse sof_ext_man header type %d size 0x%X\n",
                                elem_hdr->type, elem_hdr->size);
-                       break;
+                       return ret;
                }
 
                remaining -= elem_hdr->size;
@@ -273,10 +273,10 @@ static int snd_sof_fw_ext_man_parse(struct snd_sof_dev *sdev,
 
        if (remaining) {
                dev_err(sdev->dev, "error: sof_ext_man header is inconsistent\n");
-               ret = -EINVAL;
+               return -EINVAL;
        }
 
-       return ret ? ext_man_size : ret;
+       return ext_man_size;
 }
 
 /*

[ktrzcinx]

@keyonjie good idea.

[keyonjie]

looks good.

[marc-hb]

After #2091 this is the second difference and bug compared to d8e25a1~1

There are a few more other differences in git diff d8e25a10ef87~1 -- sound/soc/sof/loader.c , @ktrzcinx did you review them all? Same question for other sound/soc/ files reverted in d8e25a1

[ktrzcinx]

@marc-hb there are full list of git diff f754b74..d8e25a1~1 -- sound/soc/sof/loader.c:

diff --git a/sound/soc/sof/loader.c b/sound/soc/sof/loader.c
index 8be30cd..b94fa5f5 100644
--- a/sound/soc/sof/loader.c
+++ b/sound/soc/sof/loader.c
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
 //
 // This file is provided under a dual BSD/GPLv2 license.  When using or
 // redistributing this file, you may do so under either license.
@@ -12,7 +12,7 @@
 
 #include <linux/firmware.h>
 #include <sound/sof.h>
-#include <uapi/sound/sof/ext_manifest.h>
+#include <sound/sof/ext_manifest.h>
 #include "ops.h"

Intended - reason of revert

  static int get_ext_windows(struct snd_sof_dev *sdev,
@@ -22,17 +22,17 @@ static int get_ext_windows(struct snd_sof_dev *sdev,
 		container_of(ext_hdr, struct sof_ipc_window, ext_hdr);
 	size_t w_size = struct_size(w, window, w->num_windows);
 
+	if (w->num_windows == 0 || w->num_windows > SOF_IPC_MAX_ELEMS)
+		return -EINVAL;
+
 	if (sdev->info_window) {
 		if (memcmp(sdev->info_window, w, w_size)) {
-			dev_err(sdev->dev, "error: mistmatch between window descriptor from extended manifest and mailbox");
+			dev_err(sdev->dev, "error: mismatch between window descriptor from extended manifest and mailbox");
 			return -EINVAL;
 		}
 		return 0;
 	}
 
-	if (w->num_windows == 0 || w->num_windows > SOF_IPC_MAX_ELEMS)
-		return -EINVAL;
-

Intended - typo (#1903 (comment)) + change order of checks #1903 (comment)

 	/* keep a local copy of the data */
 	sdev->info_window = kmemdup(w, w_size, GFP_KERNEL);
 	if (!sdev->info_window)
@@ -146,9 +146,8 @@ EXPORT_SYMBOL(snd_sof_fw_parse_ext_data);
 static int ext_man_get_fw_version(struct snd_sof_dev *sdev,
 				  const struct sof_ext_man_elem_header *hdr)
 {
-	const struct sof_ext_man_fw_version *v;
-
-	v = container_of(hdr, struct sof_ext_man_fw_version, hdr);
+	const struct sof_ext_man_fw_version *v =
+		container_of(hdr, struct sof_ext_man_fw_version, hdr);
 
 	memcpy(&sdev->fw_ready.version, &v->version, sizeof(v->version));
 	sdev->fw_ready.flags = v->flags;
@@ -160,30 +159,28 @@ static int ext_man_get_fw_version(struct snd_sof_dev *sdev,
 static int ext_man_get_windows(struct snd_sof_dev *sdev,
 			       const struct sof_ext_man_elem_header *hdr)
 {
-	const struct sof_ipc_ext_data_hdr *w_ipc;
 	const struct sof_ext_man_window *w;
 
 	w = container_of(hdr, struct sof_ext_man_window, hdr);
-	w_ipc = (const struct sof_ipc_ext_data_hdr *)&w->ipc_window;
 
-	return get_ext_windows(sdev, w_ipc);
+	return get_ext_windows(sdev, &w->ipc_window.ext_hdr);
 }
 
 static int ext_man_get_cc_info(struct snd_sof_dev *sdev,
 			       const struct sof_ext_man_elem_header *hdr)
 {
 	const struct sof_ext_man_cc_version *cc;
-	const struct sof_ipc_ext_data_hdr *cc_version;
 
 	cc = container_of(hdr, struct sof_ext_man_cc_version, hdr);
-	cc_version = (const struct sof_ipc_ext_data_hdr *)&cc->cc_version;
 
-	return get_cc_info(sdev, cc_version);
+	return get_cc_info(sdev, &cc->cc_version.ext_hdr);
 }

Intended - removing typecasting (#1903 (comment))

 static ssize_t snd_sof_ext_man_size(const struct firmware *fw)
 {
-	const struct sof_ext_man_header *head = (void *)fw->data;
+	const struct sof_ext_man_header *head;
+
+	head = (struct sof_ext_man_header *)fw->data;

Intended - remove void cast (#1818 (comment))

 	/*
 	 * assert fw size is big enough to contain extended manifest header,
@@ -220,14 +217,8 @@ static int snd_sof_fw_ext_man_parse(struct snd_sof_dev *sdev,
 	ext_man_size = snd_sof_ext_man_size(fw);
 
 	/* Assert firmware starts with extended manifest */
-	if (ext_man_size < 0) {
-		dev_err(sdev->dev, "error: exception while reading firmware extended manifest, code %d\n",
-			(int)ext_man_size);
+	if (ext_man_size <= 0)
 		return ext_man_size;
-	} else if (!ext_man_size) {
-		dev_err(sdev->dev, "error: can't parse extended manifest when it's not present\n");
-		return -EINVAL;
-	}

Intended - remove double validation (#1903 (comment))

 	/* incompatible version */
 	if (SOF_EXT_MAN_VERSION_INCOMPATIBLE(SOF_EXT_MAN_VERSION,
@@ -250,7 +241,7 @@ static int snd_sof_fw_ext_man_parse(struct snd_sof_dev *sdev,
 		    elem_hdr->size > remaining) {
 			dev_err(sdev->dev, "error: invalid sof_ext_man header size, type %d size 0x%X\n",
 				elem_hdr->type, elem_hdr->size);
-			break;
+			return -EINVAL;
 		}
 
 		/* process structure data */
@@ -273,7 +264,7 @@ static int snd_sof_fw_ext_man_parse(struct snd_sof_dev *sdev,
 		if (ret < 0) {
 			dev_err(sdev->dev, "error: failed to parse sof_ext_man header type %d size 0x%X\n",
 				elem_hdr->type, elem_hdr->size);
-			break;
+			return ret;
 		}
 
 		remaining -= elem_hdr->size;
@@ -282,10 +273,10 @@ static int snd_sof_fw_ext_man_parse(struct snd_sof_dev *sdev,
 
 	if (remaining) {
 		dev_err(sdev->dev, "error: sof_ext_man header is inconsistent\n");
-		ret = -EINVAL;
+		return -EINVAL;
 	}
 
-	return ret;
+	return ext_man_size;
 }

Intended - simplify code (#2095 (comment))

 /*
@@ -547,7 +538,7 @@ static int check_header(struct snd_sof_dev *sdev, const struct firmware *fw,
 	struct snd_sof_fw_header *header;
 	size_t fw_size = fw->size - fw_offset;
 
-	if (fw->size < fw_offset) {
+	if (fw->size <= fw_offset) {
 		dev_err(sdev->dev, "error: firmware size must be greater than firmware offset\n");
 		return -EINVAL;
 	}

Intended - adjust offset validation (#1818 (review))

@@ -660,16 +651,10 @@ int snd_sof_load_firmware_raw(struct snd_sof_dev *sdev)
 	}
 
 	/* check for extended manifest */
-	ext_man_size = snd_sof_ext_man_size(plat_data->fw);
+	ext_man_size = snd_sof_fw_ext_man_parse(sdev, plat_data->fw);
 	if (ext_man_size > 0) {
-		ret = snd_sof_fw_ext_man_parse(sdev, plat_data->fw);
-
 		/* when no error occurred, drop extended manifest */
-		if (!ret)
-			plat_data->fw_offset = ext_man_size;
-		else
-			dev_err(sdev->dev, "error: firmware %s contains unsupported or invalid extended manifest: %d\n",
-				fw_filename, ret);
+		plat_data->fw_offset = ext_man_size;
 	} else if (!ext_man_size) {
 		/* No extended manifest, so nothing to skip during FW load */
 		dev_dbg(sdev->dev, "firmware doesn't contain extended manifest\n");

Intended - remove double validation (#1903 (comment))

For ext_manifest.h there are two differences - update include list (#1818 (comment)) and removing unusable fields (#1903 (comment))

[marc-hb]

Thanks @ktrzcinx for the extremely thorough diff review, I wasn't expecting that much. I just wanted to make sure you had looked at it.