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

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: pierre-louis.bossart@linux.intel.com,
	alsa-devel@alsa-project.org, filip.kaczmarski@intel.com,
	harshapriya.n@intel.com, gregkh@linuxfoundation.org,
	marcin.barlik@intel.com, zwisler@google.com, lgirdwood@gmail.com,
	tiwai@suse.com, filip.proborszcz@intel.com, broonie@kernel.org,
	amadeuszx.slawinski@linux.intel.com, michal.wasko@intel.com,
	cujomalainey@chromium.org, krzysztof.hejmowski@intel.com,
	ppapierkowski@habana.ai, vamshi.krishna.gopal@intel.com
Subject: Re: [PATCH v7 03/14] ASoC: Intel: catpt: Add IPC message handlers
Date: Mon, 21 Sep 2020 15:59:34 +0300	[thread overview]
Message-ID: <20200921125934.GT3956970@smile.fi.intel.com> (raw)
In-Reply-To: <20200921115424.4105-4-cezary.rojewski@intel.com>

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