audio: copier: Refactor deep copy of copier config

Code for copying copier_data is ambiguous. A comment suggests that the deep
copy (data structure + variable-length array) is not needed but the copy of
variable-length array is performed afterwards. This changet rectifies this by
placing ipc4_copier_module_cfg at the end of copier_data structure as it
contains a variable-length array. This enables single memory allocation and
single, clean 1:1 copy for the complete copier_data with the variable-length
addition.

Signed-off-by: Wojciech Jablonski <wojciech.jablonski@intel.com>

Author: wjablon1

src/audio/copier/copier.c

@@ -136,48 +136,27 @@ __cold static int copier_init(struct processing_module *mod)
 	struct comp_dev *dev = mod->dev;
 	struct module_data *md = &mod->priv;
 	struct ipc4_copier_module_cfg *copier = (struct ipc4_copier_module_cfg *)md->cfg.init_data;
-	void *gtw_cfg = NULL;
-	size_t gtw_cfg_size;
+	size_t cfg_total_size = sizeof(*copier);
+	size_t gtw_cfg_var_size = 0;
 	int i, ret = 0;
 
 	assert_can_be_cold();
 
-	cd = mod_zalloc(mod, sizeof(*cd));
+	if (copier->gtw_cfg.config_length > 1) {
+		/* one word already included in gateway_cfg struct hence subtraction */
+		gtw_cfg_var_size += (copier->gtw_cfg.config_length - 1) << 2;
+		cfg_total_size += gtw_cfg_var_size;
+	}
+
+	cd = mod_zalloc(mod, sizeof(*cd) + gtw_cfg_var_size);
 	if (!cd)
 		return -ENOMEM;
 
 	md->private = cd;
-	/*
-	 * Don't copy the config_data[] variable size array, we don't need to
-	 * store it, it's only used during IPC processing, besides we haven't
-	 * allocated space for it, so don't "fix" this!
-	 */
-	if (memcpy_s(&cd->config, sizeof(cd->config), copier, sizeof(*copier)) < 0) {
-		ret = -EINVAL;
-		goto error_cd;
-	}
 
-	/* Allocate memory and store gateway_cfg in runtime. Gateway cfg has to
-	 * be kept even after copier is created e.g. during SET_PIPELINE_STATE
-	 * IPC when dai_config_dma_channel() is called second time and DMA
-	 * config is used to assign dma_channel_id value.
-	 */
-	if (copier->gtw_cfg.config_length) {
-		gtw_cfg_size = copier->gtw_cfg.config_length << 2;
-		gtw_cfg = mod_alloc(mod, gtw_cfg_size);
-		if (!gtw_cfg) {
-			ret = -ENOMEM;
-			goto error_cd;
-		}
-
-		ret = memcpy_s(gtw_cfg, gtw_cfg_size, &copier->gtw_cfg.config_data,
-			       gtw_cfg_size);
-		if (ret) {
-			comp_err(dev, "Unable to copy gateway config from copier blob");
-			goto error;
-		}
-
-		cd->gtw_cfg = gtw_cfg;
+	if (memcpy_s(&cd->config, cfg_total_size, copier, cfg_total_size) < 0) {
+		ret = -EINVAL;
+		goto error;
 	}
 
 	for (i = 0; i < IPC4_COPIER_MODULE_OUTPUT_PINS_COUNT; i++)
@@ -257,8 +236,6 @@ __cold static int copier_init(struct processing_module *mod)
 	dev->state = COMP_STATE_READY;
 	return 0;
 error:
-	mod_free(mod, gtw_cfg);
-error_cd:
 	mod_free(mod, cd);
 	return ret;
 }
@@ -289,8 +266,6 @@ __cold static int copier_free(struct processing_module *mod)
 		break;
 	}
 
-	if (cd)
-		mod_free(mod, cd->gtw_cfg);
 	mod_free(mod, cd);
 
 	return 0;

src/audio/copier/copier.h

@@ -241,13 +241,6 @@ struct ipc4_data_segment_enabled {
 } __attribute__((packed, aligned(4)));
 
 struct copier_data {
-	/*
-	 * struct ipc4_copier_module_cfg actually has variable size, but we
-	 * don't need the variable size array at the end, we won't be copying it
-	 * from the IPC data.
-	 */
-	struct ipc4_copier_module_cfg config;
-	void *gtw_cfg;
 	enum ipc4_gateway_type gtw_type;
 	uint32_t endpoint_num;
 
@@ -277,6 +270,8 @@ struct copier_data {
 #if CONFIG_INTEL_ADSP_MIC_PRIVACY
 	struct mic_privacy_data *mic_priv;
 #endif
+	/* Has to be at the end due to variable size array */
+	struct ipc4_copier_module_cfg config;
 };
 
 int apply_attenuation(struct comp_dev *dev, struct copier_data *cd,

src/audio/copier/copier_dai.c

@@ -296,7 +296,7 @@ __cold int copier_dai_create(struct comp_dev *dev, struct copier_data *cd,
 		dai.type = SOF_DAI_INTEL_SSP;
 		dai.is_config_blob = true;
 		cd->gtw_type = ipc4_gtw_ssp;
-		ret = ipc4_find_dma_config(&dai, (uint8_t *)cd->gtw_cfg,
+		ret = ipc4_find_dma_config(&dai, (uint8_t *)copier->gtw_cfg.config_data,
 					   copier->gtw_cfg.config_length * 4);
 		if (ret != 0) {
 			comp_err(dev, "No ssp dma_config found in blob!");
@@ -315,16 +315,16 @@ __cold int copier_dai_create(struct comp_dev *dev, struct copier_data *cd,
 		dai.is_config_blob = true;
 		cd->gtw_type = ipc4_gtw_alh;
 #endif /* ACE_VERSION > ACE_VERSION_1_5 */
-		ret = copier_alh_assign_dai_index(dev, cd->gtw_cfg, node_id,
-						  &dai, dai_index, &dai_count);
+		ret = copier_alh_assign_dai_index(dev, (uint8_t *)copier->gtw_cfg.config_data,
+						  node_id, &dai, dai_index, &dai_count);
 		if (ret)
 			return ret;
 		break;
 	case ipc4_dmic_link_input_class:
 		dai.type = SOF_DAI_INTEL_DMIC;
 		dai.is_config_blob = true;
 		cd->gtw_type = ipc4_gtw_dmic;
-		ret = ipc4_find_dma_config(&dai, (uint8_t *)cd->gtw_cfg,
+		ret = ipc4_find_dma_config(&dai, (uint8_t *)copier->gtw_cfg.config_data,
 					   copier->gtw_cfg.config_length * 4);
 		if (ret != 0) {
 			comp_err(dev, "No dmic dma_config found in blob!");

src/audio/copier/copier_gain.c

@@ -34,12 +34,8 @@ __cold int copier_gain_set_params(struct comp_dev *dev, struct copier_gain_param
 	switch (dai_type) {
 	case SOF_DAI_INTEL_DMIC:
 		{
-			struct dmic_config_data *dmic_cfg = cd->gtw_cfg;
-
-			if (!dmic_cfg) {
-				comp_err(dev, "No dmic config found");
-				return -EINVAL;
-			}
+			struct dmic_config_data *dmic_cfg =
+					(void *)cd->config.gtw_cfg.config_data;
 
 			union dmic_global_cfg *dmic_glb_cfg = &dmic_cfg->dmic_blob.global_cfg;
 

src/ipc/ipc4/dai.c

@@ -91,13 +91,9 @@ int dai_config_dma_channel(struct dai_data *dd, struct comp_dev *dev, const void
 			struct processing_module *mod = comp_mod(dev);
 			struct copier_data *cd = module_get_private_data(mod);
 
-			if (!cd->gtw_cfg) {
-				comp_err(dev, "No gateway config found!");
-				return SOF_DMA_CHAN_INVALID;
-			}
-
 			channel = SOF_DMA_CHAN_INVALID;
-			const struct sof_alh_configuration_blob *alh_blob = cd->gtw_cfg;
+			const struct sof_alh_configuration_blob *alh_blob =
+							(void *)cd->config.gtw_cfg.config_data;
 
 			for (int i = 0; i < alh_blob->alh_cfg.count; i++) {
 				if (dai->host_dma_config[i]->stream_id == dai->dai_index) {

zephyr/lib/cpu.c

@@ -122,7 +122,8 @@ static void resume_dais(void)
 		if (dai_probe(dd->dai->dev) < 0) {
 			tr_err(&zephyr_tr, "DAI resume failed on probe, type %d index %d",
 			       dd->dai->type, dd->dai->index);
-		} else if (dai_set_config(dd->dai, &dd->ipc_config, cd->gtw_cfg,
+		} else if (dai_set_config(dd->dai, &dd->ipc_config,
+					  cd->config.gtw_cfg.config_data,
 					  cd->config.gtw_cfg.config_length) < 0) {
 			tr_err(&zephyr_tr, "DAI resume failed on config, type %d index %d",
 			       dd->dai->type, dd->dai->index);