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

/*

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