Re: [PATCH v3 10/17] ASoC: Intel: avs: Add basefw runtime-parameter requests

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

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

...

>> +int avs_ipc_get_fw_config(struct avs_dev *adev, struct avs_fw_cfg
>> *cfg)
>> +{
>> +	struct avs_tlv *tlv;
>> +	size_t payload_size;
>> +	size_t offset = 0;
>> +	u8 *payload;
>> +	int ret;
>> +
>> +	ret = avs_ipc_get_large_config(adev, AVS_BASEFW_MOD_ID,
>> AVS_BASEFW_INST_ID,
>> +				       AVS_BASEFW_FIRMWARE_CONFIG,
>> NULL, 0,
>> +				       &payload, &payload_size);
>> +	if (ret)
>> +		return ret;
>> +
>> +	while (offset < payload_size) {
>> +		tlv = (struct avs_tlv *)(payload + offset);
>> +
>> +		switch (tlv->type) {
>> +		case AVS_FW_CFG_FW_VERSION:
>> +			memcpy(&cfg->fw_version, tlv->value,
>> +				sizeof(cfg->fw_version));
>> +			break;
>> +
>> +		case AVS_FW_CFG_MEMORY_RECLAIMED:
>> +			cfg->memory_reclaimed = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_SLOW_CLOCK_FREQ_HZ:
>> +			cfg->slow_clock_freq_hz = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_FAST_CLOCK_FREQ_HZ:
>> +			cfg->fast_clock_freq_hz = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_ALH_SUPPORT_LEVEL:
>> +			cfg->alh_support = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_IPC_DL_MAILBOX_BYTES:
>> +			cfg->ipc_dl_mailbox_bytes = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_IPC_UL_MAILBOX_BYTES:
>> +			cfg->ipc_ul_mailbox_bytes = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_TRACE_LOG_BYTES:
>> +			cfg->trace_log_bytes = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_MAX_PPL_COUNT:
>> +			cfg->max_ppl_count = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_MAX_ASTATE_COUNT:
>> +			cfg->max_astate_count = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_MAX_MODULE_PIN_COUNT:
>> +			cfg->max_module_pin_count = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_MODULES_COUNT:
>> +			cfg->modules_count = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_MAX_MOD_INST_COUNT:
>> +			cfg->max_mod_inst_count = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_MAX_LL_TASKS_PER_PRI_COUNT:
>> +			cfg->max_ll_tasks_per_pri_count = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_LL_PRI_COUNT:
>> +			cfg->ll_pri_count = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_MAX_DP_TASKS_COUNT:
>> +			cfg->max_dp_tasks_count = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_MAX_LIBS_COUNT:
>> +			cfg->max_libs_count = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_XTAL_FREQ_HZ:
>> +			cfg->xtal_freq_hz = *tlv->value;
>> +			break;
>> +
>> +		case AVS_FW_CFG_POWER_GATING_POLICY:
>> +			cfg->power_gating_policy = *tlv->value;
>> +			break;
>> +
>> +		/* Known but not useful to us. */
>> +		case AVS_FW_CFG_DMA_BUFFER_CONFIG:
>> +		case AVS_FW_CFG_SCHEDULER_CONFIG:
>> +		case AVS_FW_CFG_CLOCKS_CONFIG:
>> +			break;
>> +
>> +		default:
>> +			dev_info(adev->dev, "Unrecognized fw param:
>> %d\n",
>> +				 tlv->type);
>> +			break;
>> +		}
>> +
>> +		offset += sizeof(*tlv) + tlv->length;
>> +	}
>> +
>> +	kfree(payload);
> I think it would be easier to understand this kfree if payload was also
> allocated in this function in stead of inside the get_large_config().


That's a good thinking. There was an internal conversation regarding 
this back in time when we have been implementing getters for the first 
time. There are no clear victors, there are drawbacks - as you do not 
know the size upfront, caller has to guess and then reallocate the 
buffer accordingly to retrieved payload size from the firmware. So, even 
if you allocate buffer here, chances are, it's not the same buffer when 
the avs_ipc_get_large_config() returns to the caller.

We have decided to reduce the code size by letting the single, common 
handler do the allocation and leave the other responsibilities to the 
caller.


Regards,
Czarek