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
