Re: [PATCH v3 11/17] ASoC: Intel: avs: Firmware resources management utilities

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

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

>>   /*
>>    * struct avs_dev - Intel HD-Audio driver data
>>    *
>>    * @dev: PCI device
>>    * @dsp_ba: DSP bar address
>>    * @spec: platform-specific descriptor
>> + * @fw_cfg: Firmware configuration, obtained through FW_CONFIG
>> message
>> + * @hw_cfg: Hardware configuration, obtained through HW_CONFIG
>> message
>> + * @mods_info: Available module-types, obtained through MODULES_INFO
>> message
>> + * @mod_idas: Module instance ID pool, one per module-type
>> + * @modres_mutex: For synchronizing any @mods_info updates
> Is this mutex really necessary? Can you please elaborate under what
> circumstances your will have parallel module updates?


Yes, we believe modres_mutex is necessary. All information regarding 
modules exposed by the firmware are stored within ->mods_info cache.

That's just a snapshot though. When a new library gets loaded, new 
modules may be available for use and so the driver updates the 
->mods_info cache to have the latest snapshot. As information found 
there is used when streaming (e.g.: instantiating modules), we enter a 
scenario when multiple threads could be reading/updating the ->mods_info 
at once. To prevent any unwanted behavior, mutex has been added.

>> +void avs_module_info_free(struct avs_dev *adev)
>> +{
>> +	mutex_lock(&adev->modres_mutex);
>> +
>> +	avs_module_ida_destroy(adev);
>> +	kfree(adev->mods_info);
>> +	adev->mods_info = NULL;
>> +
>> +	mutex_unlock(&adev->modres_mutex);
>> +}
>> +
>> +int avs_module_id_alloc(struct avs_dev *adev, u16 module_id)
>> +{
>> +	int ret, idx, max_id;
>> +
>> +	mutex_lock(&adev->modres_mutex);
>> +
>> +	idx = avs_module_id_entry_index(adev, module_id);
>> +	if (idx == -ENOENT) {
> Can you please help me understand when this can happen? If all modules
> required by the topology are already initialized, will this ever
> happen?


I want to help! Just not understanding the meaning of: "all modules 
required by the topology are already initialized".

Will answer best I can though: topology carries just patterns, it may 
happen that module found within topology file is not actually exposed by 
the firmware. In such case, we drop an error. This keeps recovery 
scenarios sane too - when recovering, libraries may have to be 
re-loaded, depending on the firmware generation and whether basefw 
recovery was successful or not.


Regards,
Czarek