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

[Andy Shevchenko]

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?

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

> +	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_IRAM;
> +	hdr->size = resource_size(&cdev->iram);
> +	pos += sizeof(*hdr);
> +
> +	memcpy_fromio(pos, cdev->lpe_ba + cdev->iram.start, hdr->size);
> +	pos += hdr->size;
> +
> +	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_DRAM;
> +	hdr->size = resource_size(&cdev->dram);
> +	pos += sizeof(*hdr);
> +
> +	memcpy_fromio(pos, cdev->lpe_ba + cdev->dram.start, hdr->size);
> +	pos += hdr->size;
> +
> +	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_REGS;
> +	hdr->size = regs_size;
> +	pos += sizeof(*hdr);
> +
> +	memcpy_fromio(pos, catpt_shim_addr(cdev), CATPT_SHIM_REGS_SIZE);
> +	pos += CATPT_SHIM_REGS_SIZE;
> +
> +	for (i = 0; i < CATPT_SSP_COUNT; i++) {
> +		memcpy_fromio(pos, catpt_ssp_addr(cdev, i),
> +			      CATPT_SSP_REGS_SIZE);
> +		pos += CATPT_SSP_REGS_SIZE;
> +	}
> +	for (i = 0; i < CATPT_DMA_COUNT; i++) {
> +		memcpy_fromio(pos, catpt_dma_addr(cdev, i),
> +			      CATPT_DMA_REGS_SIZE);
> +		pos += CATPT_DMA_REGS_SIZE;
> +	}
> +
> +	dev_coredumpv(cdev->dev, dump, dump_size, GFP_KERNEL);
> +
> +	return 0;
> +}

...

> +struct catpt_set_volume_input {
> +	u32 channel;
> +	u32 target_volume;
> +	u64 curve_duration;
> +	enum catpt_audio_curve_type curve_type __aligned(4);

> +} __packed;

How this __packed changes anything? In general __packed doesn't make sense for
in-kernel data structures. Otherwise you have to use proper (POD) types for
data. Ditto for all similar cases.

-- 
With Best Regards,
Andy Shevchenko