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

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

>   /*
>    * 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

is this just for the base firmware? If this includes the extensions, how 
are the module types defined?

> + * @mod_idas: Module instance ID pool, one per module-type
> + * @modres_mutex: For synchronizing any @mods_info updates
> + * @ppl_ida: Pipeline instance ID pool
> + * @fw_list: List of libraries loaded, including base firmware
>    */
>   struct avs_dev {
>   	struct hda_bus base;
> @@ -68,6 +82,14 @@ struct avs_dev {
>   	const struct avs_spec *spec;
>   	struct avs_ipc *ipc;
>   
> +	struct avs_fw_cfg fw_cfg;
> +	struct avs_hw_cfg hw_cfg;
> +	struct avs_mods_info *mods_info;
> +	struct ida **mod_idas;
> +	struct mutex modres_mutex;
> +	struct ida ppl_ida;
> +	struct list_head fw_list;
> +
>   	struct completion fw_ready;
>   };

> +/* Caller responsible for holding adev->modres_mutex. */
> +static int
> +avs_module_ida_alloc(struct avs_dev *adev, struct avs_mods_info *newinfo, bool purge)
> +{
> +	struct avs_mods_info *oldinfo = adev->mods_info;
> +	struct ida **ida_ptrs;
> +	u32 tocopy_count = 0;
> +	int i;
> +
> +	if (!purge && oldinfo) {
> +		if (oldinfo->count >= newinfo->count)
> +			dev_warn(adev->dev, "refreshing %d modules info with %d\n",
> +				 oldinfo->count, newinfo->count);
> +		tocopy_count = oldinfo->count;
> +	}
> +
> +	ida_ptrs = kcalloc(newinfo->count, sizeof(*ida_ptrs), GFP_KERNEL);
> +	if (!ida_ptrs)
> +		return -ENOMEM;
> +
> +	if (tocopy_count)
> +		memcpy(ida_ptrs, adev->mod_idas, tocopy_count * sizeof(*ida_ptrs));
> +
> +	for (i = tocopy_count; i < newinfo->count; i++) {
> +		ida_ptrs[i] = kzalloc(sizeof(**ida_ptrs), GFP_KERNEL);
> +		if (!ida_ptrs[i]) {
> +			while (i--)
> +				kfree(ida_ptrs[i]);

it's a bit hairy to play with the loop counter, I would jump to an error 
handling label to make it clearer.
> +
> +			kfree(ida_ptrs);
> +			return -ENOMEM;
> +		}
> +
> +		ida_init(ida_ptrs[i]);
> +	}
> +
> +	/* If old elements have been reused, don't wipe them. */

the comment is very odd, there's either a free() or a destroy() 
happening below...

> +	if (tocopy_count)
> +		kfree(adev->mod_idas);
> +	else
> +		avs_module_ida_destroy(adev);
> +
> +	adev->mod_idas = ida_ptrs;
> +	return 0;
> +}

> +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) {
> +		WARN(1, "invalid module id: %d", module_id);

dev_err() seems to be more than enough, why would you add a complete 
call trace?

> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +	max_id = adev->mods_info->entries[idx].instance_max_count - 1;
> +	ret = ida_alloc_max(adev->mod_idas[idx], max_id, GFP_KERNEL);
> +exit:
> +	mutex_unlock(&adev->modres_mutex);
> +	return ret;
> +}
> +
> +void avs_module_id_free(struct avs_dev *adev, u16 module_id, u8 instance_id)
> +{
> +	int idx;
> +
> +	mutex_lock(&adev->modres_mutex);
> +
> +	idx = avs_module_id_entry_index(adev, module_id);
> +	if (idx == -ENOENT) {
> +		WARN(1, "invalid module id: %d", module_id);

same WARN is over-engineered.

> +		goto exit;
> +	}
> +
> +	ida_free(adev->mod_idas[idx], instance_id);
> +exit:
> +	mutex_unlock(&adev->modres_mutex);
> +}
> +
> +/*

I am running out of time and will resume this review next week.