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

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-02-25 3:15 AM, Pierre-Louis Bossart wrote:
> 
> 
>> +#define AVS_FW_INIT_TIMEOUT_MS		3000
> 
> another timeout?


Different operation, different timeout. What's wrong with that? And we 
are not repeating it three times (skl/bxt/cnl) as it's done in 
skylake-driver!

> 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?


Can drop "_check", dropping anything else makes this ambiguous. Also, 
this variable is used just once, in the very line below.

>> +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.


Ack. Indeed the name by itself may not be sufficient.

>> +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


That's a helpful link and a good conversation, thank you. I see that 
Bruce mentions /lib/firmware as 3rd in presented order and that's where 
we intend to have firmware files. We are not planning to have a separate 
repo for base firmware/library binaries if that's the question.

Given the above, I'm not sure if we should be mentioning the need to 
update /lib/firmware explicitly as people already recognize it as one of 
the default locations to check.

>> +
>> +		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.


I'll sync with Amadeo regarding this. It seems it's cleared only on 
->remove().

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