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
