Re: [PATCH 14/17] ASoC: Intel: avs: General code loading flow

+#define AVS_FW_INIT_TIMEOUT_MS 3000

another timeout?

skl-sst.c:#define SKL_BASEFW_TIMEOUT 300 skl-sst.c:#define SKL_INIT_TIMEOUT 1000 bxt-sst.c:#define BXT_BASEFW_TIMEOUT 3000 cnl-sst.c:#define CNL_INIT_TIMEOUT 300 cnl-sst.c:#define CNL_BASEFW_TIMEOUT 3000

+#define AVS_ROOT_DIR "intel/avs" +#define AVS_BASEFW_FILENAME "dsp_basefw.bin" +#define AVS_EXT_MANIFEST_MAGIC 0x31454124 +#define SKL_MANIFEST_MAGIC 0x00000006 +#define SKL_ADSPFW_OFFSET 0x284

+static bool debug_ignore_fw_version_check;

this_is_a_very_long_variable_name_isn_t_it?

+module_param_named(ignore_fw_version, debug_ignore_fw_version_check, bool, 0444); +MODULE_PARM_DESC(ignore_fw_version, "Verify FW version 0=yes (default), 1=no");

You should clarify the purpose of the version check, and why this driver needs different firmware binaries and versions than what was already released to linux-firmware.

+static int avs_fw_manifest_strip_verify(struct avs_dev *adev, struct firmware *fw,

• 			const struct avs_fw_version *min)
  

+{

• struct avs_fw_manifest *man;
• int offset, ret;
• ret = avs_fw_ext_manifest_strip(fw);
• if (ret)

• return ret;
  

• offset = avs_fw_manifest_offset(fw);
• if (offset < 0)

• return offset;
  

• if (fw->size < offset + sizeof(*man))

• return -EINVAL;
  

• if (!min)

• return 0;
  

• man = (struct avs_fw_manifest *)(fw->data + offset);
• if (man->version.major != min->major ||

•    man->version.minor != min->minor ||
  

•    man->version.hotfix != min->hotfix ||
  

•    man->version.build < min->build) {
  

• dev_warn(adev->dev, "bad FW version %d.%d.%d.%d, expected %d.%d.%d.%d or newer\n",
  

• 	 man->version.major, man->version.minor,
  

• 	 man->version.hotfix, man->version.build,
  

• 	 min->major, min->minor, min->hotfix, min->build);
  

usually when the relevant firmware is not found, the distributions and users like to see a message informing them of the location of the firmware binaries.

see thread with Bruce Perens and Jaroslav at https://github.com/thesofproject/sof-bin/issues/22

• if (!debug_ignore_fw_version_check)
  

• 	return -EINVAL;
  

• }
• return 0;

+}

+static int avs_dsp_load_basefw(struct avs_dev *adev) +{

• const struct avs_fw_version *min_req;
• const struct avs_spec *const spec = adev->spec;
• const struct firmware *fw;
• struct firmware stripped_fw;
• char *filename;
• int ret;
• filename = kasprintf(GFP_KERNEL, "%s/%s/%s", AVS_ROOT_DIR, spec->name,

• 	     AVS_BASEFW_FILENAME);
  

• if (!filename)

• return -ENOMEM;
  

• ret = avs_request_firmware(adev, &fw, filename);
• kfree(filename);
• if (ret < 0) {

• dev_err(adev->dev, "request firmware failed: %d\n", ret);
  

• return ret;
  

• }
• stripped_fw = *fw;
• min_req = &adev->spec->min_fw_version;
• ret = avs_fw_manifest_strip_verify(adev, &stripped_fw, min_req);
• if (ret < 0) {

• dev_err(adev->dev, "invalid firmware data: %d\n", ret);
  

should you not release the firmware in all error cases?

if this is handled at a higher level, please add a comment.

• return ret;
  

• }
• ret = avs_dsp_op(adev, load_basefw, &stripped_fw);
• if (ret < 0) {

• dev_err(adev->dev, "basefw load failed: %d\n", ret);
  

• return ret;
  

• }
• ret = wait_for_completion_timeout(&adev->fw_ready,

• 			  msecs_to_jiffies(AVS_FW_INIT_TIMEOUT_MS));
  

• if (!ret) {

• dev_err(adev->dev, "firmware ready timeout\n");
  

• avs_dsp_core_disable(adev, AVS_MAIN_CORE_MASK);
  

• return -ETIMEDOUT;
  

• }
• return 0;

+}