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

From: "Rojewski, Cezary" <cezary.rojewski@intel.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "pierre-louis.bossart@linux.intel.com"
	<pierre-louis.bossart@linux.intel.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"Kaczmarski, Filip" <filip.kaczmarski@intel.com>,
	"N, Harshapriya" <harshapriya.n@intel.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"Barlik, Marcin" <marcin.barlik@intel.com>,
	"zwisler@google.com" <zwisler@google.com>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"tiwai@suse.com" <tiwai@suse.com>,
	"Proborszcz, Filip" <filip.proborszcz@intel.com>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"amadeuszx.slawinski@linux.intel.com"
	<amadeuszx.slawinski@linux.intel.com>,
	"Wasko, Michal" <michal.wasko@intel.com>,
	"cujomalainey@chromium.org" <cujomalainey@chromium.org>,
	"Hejmowski, Krzysztof" <krzysztof.hejmowski@intel.com>,
	"Papierkowski, Piotr \(Habana\)" <ppapierkowski@habana.ai>,
	"Gopal,  Vamshi Krishna" <vamshi.krishna.gopal@intel.com>
Subject: RE: [PATCH v7 03/14] ASoC: Intel: catpt: Add IPC message handlers
Date: Mon, 21 Sep 2020 18:13:59 +0000	[thread overview]
Message-ID: <55266617b42843839fb85309d3097fd6@intel.com> (raw)
In-Reply-To: <20200921125934.GT3956970@smile.fi.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