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

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.