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