RE: [PATCH v7 03/14] ASoC: Intel: catpt: Add IPC message handlers

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

On 2020-09-21 2:59 PM, Andy Shevchenko wrote:
> On Mon, Sep 21, 2020 at 01:54:13PM +0200, Cezary Rojewski wrote:
>> Declare global and stream IPC message handlers for all known message
>> types.
...

>> +int catpt_coredump(struct catpt_dev *cdev)
>> +{
>> +	struct catpt_dump_section_hdr *hdr;
>> +	size_t dump_size, regs_size;
>> +	u8 *dump, *pos;
>> +	int i, j;
>> +
>> +	regs_size = CATPT_SHIM_REGS_SIZE;
>> +	regs_size += CATPT_DMA_COUNT * CATPT_DMA_REGS_SIZE;
>> +	regs_size += CATPT_SSP_COUNT * CATPT_SSP_REGS_SIZE;
>> +	dump_size = resource_size(&cdev->dram);
>> +	dump_size += resource_size(&cdev->iram);
>> +	dump_size += regs_size;
> 
>> +	dump_size += 4 * sizeof(*hdr) + 20; /* hdrs and fw hash */
> 
> Function is full of hard coded 20s. Can you provide descriptive macro?
> 

Will declare CATPT_DUMP_HASH_SIZE instead of hardcodes, sure.

>> +	dump = vzalloc(dump_size);
>> +	if (!dump)
>> +		return -ENOMEM;
>> +
>> +	pos = dump;
>> +
>> +	hdr = (struct catpt_dump_section_hdr *)pos;
>> +	hdr->magic = CATPT_DUMP_MAGIC;
>> +	hdr->core_id = cdev->spec->core_id;
>> +	hdr->section_id = CATPT_DUMP_SECTION_ID_FILE;
>> +	hdr->size = dump_size - sizeof(*hdr);
>> +	pos += sizeof(*hdr);
>> +
>> +	for (i = j = 0; i < FW_INFO_SIZE_MAX; i++)
>> +		if (cdev->ipc.config.fw_info[i] == ' ')
>> +			if (++j == 4)
>> +				break;
> 
>> +	for (j = ++i; j < FW_INFO_SIZE_MAX && j - i < 20; j++) {
> 
> This should have static_assert() at the place where you define both constants
> (2nd is mentioned above 20).
> 
>> +		if (cdev->ipc.config.fw_info[j] == ' ')
>> +			break;
>> +		*(pos + j - i) = cdev->ipc.config.fw_info[j];
>> +	}
>> +	pos += 20;
> 
> These two for-loops should have some comment to explain what's going on.
> 

Actually, after poking my FW friends again I realized that it's just
dumping 20chars from "hash" segment of fw_info (struct catpt_fw_ready,
field: fw_info[]).

So, this could be replaced by:

	/* navigate to fifth info segment (fw hash) */
	for (i = j = 0; i < FW_INFO_SIZE_MAX; i++)
		/* info segments are separated by space each */
		if (cdev->ipc.config.fw_info[i] == ' ')
			if (++j == 4)
				break;

	memcpy(pos, &cdev->ipc.config.fw_info[++i], CATPT_DUMP_HASH_SIZE);
	pos += CATPT_DUMP_HASH_SIZE;


Existing for-loops were based on internal solution. Half of the code
isn't needed afterall..

Czarek