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

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

On 2022-03-04 5:54 PM, Ranjani Sridharan wrote:
> On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski wrote:

...

>> +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) {
> Isnt this check a bit too strict? Isnt a check major enough?


Unfortunately not. I share the similar thinking but the build system has 
its history and several things which should not happen, had happened. 
There could be _large_ API changes without any meaningful version 
updates at all. To prevent any unwanted behavior, this check is as 
strict as it can get.

>> +		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);
>> +
>> +		if (!debug_ignore_fw_version)
>> +			return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}

...

>> +int avs_dsp_boot_firmware(struct avs_dev *adev, bool purge)
>> +{
>> +	int ret, i;
>> +
>> +	/* Full boot, clear cached data except for basefw (slot 0). */
> Does this mean IMR restore is only available for base FW and not for
> module libraries? Do I understand this correctly?


Loop below just clears the data. The new snapshot will be received once 
the basefw and libraries get loaded. The execution of library loading is 
not part of this patch anymore as it is dependent on the 
avs-soc-component stuff. To make things easier to review, request was to 
split main series into chucks. I do believe it is easier to read and 
review indeed.

>> +	for (i = 1; i < adev->fw_cfg.max_libs_count; i++)
>> +		memset(adev->lib_names[i], 0, AVS_LIB_NAME_SIZE);
>> +
>> +	avs_hda_clock_gating_enable(adev, false);
>> +	avs_hda_l1sen_enable(adev, false);
>> +
>> +	ret = avs_dsp_load_basefw(adev);
>> +
>> +	avs_hda_l1sen_enable(adev, true);
>> +	avs_hda_clock_gating_enable(adev, true);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* With all code loaded, refresh module information. */
>> +	ret = avs_module_info_init(adev, true);
> It is not clear if this required only after first boot or after a
> suspend/resume as well.


avs_dsp_boot_firmware() and avs_dsp_first_boot_firmware() (found just 
below this one) are two separate functions. Only the latter has things 
done once. Anything else can happen several times throughout the 
lifetime of the avs-driver.


Regards,
Czarek