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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> +#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;
> +}