| Message ID | 20200921115424.4105-4-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel: Catpt - Lynx and Wildcat point | expand |
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.
On 9/21/2020 2:59 PM, Andy Shevchenko wrote: >> +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. All of __packed use in code is done on structures used to communicate with FW, which is binary interface, so it is not kernel only structure, as it is also FW one. While we can expect compiler to do the right thing, I consider it is better to be explicit about what kind of data we are handling, so there aren't any surprises.
On Mon, Sep 21, 2020 at 03:58:33PM +0200, Amadeusz Sławiński wrote: > On 9/21/2020 2:59 PM, Andy Shevchenko wrote: > > > +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. > > All of __packed use in code is done on structures used to communicate with > FW, which is binary interface, so it is not kernel only structure, as it is > also FW one. While we can expect compiler to do the right thing, I consider > it is better to be explicit about what kind of data we are handling, so > there aren't any surprises. Size of enum is compiler defined. It may not be used in the ABIs. __uXX vs. uXX I dunno.
On Mon, Sep 21, 2020 at 05:20:12PM +0300, Andy Shevchenko wrote: > On Mon, Sep 21, 2020 at 03:58:33PM +0200, Amadeusz Sławiński wrote: > > On 9/21/2020 2:59 PM, Andy Shevchenko wrote: > > > > +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. > > > > All of __packed use in code is done on structures used to communicate with > > FW, which is binary interface, so it is not kernel only structure, as it is > > also FW one. While we can expect compiler to do the right thing, I consider > > it is better to be explicit about what kind of data we are handling, so > > there aren't any surprises. > > Size of enum is compiler defined. It may not be used in the ABIs. I have to elaborate that I'm talking in ABIs which implies different compilers and even may be run on different CPU architectures. > __uXX vs. uXX I dunno. And here I'm talking about FW <--> OS interface. It's not user visible ABI, but still some Ext <--> Int protocol. I saw uXX types in data structures of FW communication protocols.
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
On 2020-09-21 4:23 PM, Andy Shevchenko wrote: > On Mon, Sep 21, 2020 at 05:20:12PM +0300, Andy Shevchenko wrote: >> On Mon, Sep 21, 2020 at 03:58:33PM +0200, Amadeusz Sławiński wrote: >>> On 9/21/2020 2:59 PM, Andy Shevchenko wrote: >>>>> +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. >>> >>> All of __packed use in code is done on structures used to communicate with >>> FW, which is binary interface, so it is not kernel only structure, as it is >>> also FW one. While we can expect compiler to do the right thing, I consider >>> it is better to be explicit about what kind of data we are handling, so >>> there aren't any surprises. >> >> Size of enum is compiler defined. It may not be used in the ABIs. > > I have to elaborate that I'm talking in ABIs which implies different compilers > and even may be run on different CPU architectures. > >> __uXX vs. uXX I dunno. > > And here I'm talking about FW <--> OS interface. It's not user visible ABI, but > still some Ext <--> Int protocol. I saw uXX types in data structures of FW > communication protocols. > Will remove enum members from IPC structs in v8 as requested. Thanks, Czarek
On Mon, Sep 21, 2020 at 06:13:59PM +0000, Rojewski, Cezary wrote: > On 2020-09-21 2:59 PM, Andy Shevchenko wrote: > > On Mon, Sep 21, 2020 at 01:54:13PM +0200, Cezary Rojewski wrote: ... > >> + 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; ...and this is repeating strnchr() / strnchrnul(). With the questions "what if...": - nul in the middle of this? - less than 4 spaces found? > 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..
On 2020-09-21 8:41 PM, Andy Shevchenko wrote:> On Mon, Sep 21, 2020 at 06:13:59PM +0000, Rojewski, Cezary wrote: >> On 2020-09-21 2:59 PM, Andy Shevchenko wrote: >>> On Mon, Sep 21, 2020 at 01:54:13PM +0200, Cezary Rojewski wrote: ... >>>> + 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; > > ...and this is repeating strnchr() / strnchrnul(). > Indeed, will incorporate into above. > With the questions "what if...": > - nul in the middle of this? > - less than 4 spaces found? > While this should never happen (means user is somehow not making use of officially released firmware binary), coredumps are useful only if you have access to debug tools. In cases you'd mentioned, invalid hash would have been dumped to coredump and crash reader simply wouldn't have been able to navigate to actual build for it. The rest of the coredump is still vital though. memcpy() could be gated behind an 'if' for safety if needed: info = cdev->ipc.config.fw_info; eof = info + FW_INFO_SIZE_MAX; /* navigate to fifth info segment (fw hash) */ for (i = 0; i < 4 && info < eof; i++, info++) /* info segments are separated by space each */ if ((info = strnchr(info, eof - info, ' ')) == NULL) break; if (i == 4 && info < eof) memcpy(pos, info, min(eof - info, CATPT_DUMP_HASH_SIZE)); Didn't compile this, some typecheck fixes might be in order and so on. >> 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.. >
On Mon, Sep 21, 2020 at 08:48:12PM +0000, Rojewski, Cezary wrote: > On 2020-09-21 8:41 PM, Andy Shevchenko wrote:> On Mon, Sep 21, 2020 at 06:13:59PM +0000, Rojewski, Cezary wrote: > >> On 2020-09-21 2:59 PM, Andy Shevchenko wrote: > >>> On Mon, Sep 21, 2020 at 01:54:13PM +0200, Cezary Rojewski wrote: ... > While this should never happen (means user is somehow not making use of > officially released firmware binary), coredumps are useful only if you > have access to debug tools. In cases you'd mentioned, invalid hash would > have been dumped to coredump and crash reader simply wouldn't have been > able to navigate to actual build for it. The rest of the coredump is still > vital though. > > memcpy() could be gated behind an 'if' for safety if needed: > > info = cdev->ipc.config.fw_info; > eof = info + FW_INFO_SIZE_MAX; > /* navigate to fifth info segment (fw hash) */ > for (i = 0; i < 4 && info < eof; i++, info++) > /* info segments are separated by space each */ > if ((info = strnchr(info, eof - info, ' ')) == NULL) > break; > if (i == 4 && info < eof) > memcpy(pos, info, min(eof - info, CATPT_DUMP_HASH_SIZE)); And here basically enough check is info against NULL, right? Just try to look at different possibilities how to make code simpler and neater. > Didn't compile this, some typecheck fixes might be in order and so on.
On 2020-09-22 11:04 AM, Andy Shevchenko wrote: > On Mon, Sep 21, 2020 at 08:48:12PM +0000, Rojewski, Cezary wrote: >> On 2020-09-21 8:41 PM, Andy Shevchenko wrote: ... >> While this should never happen (means user is somehow not making use of >> officially released firmware binary), coredumps are useful only if you >> have access to debug tools. In cases you'd mentioned, invalid hash would >> have been dumped to coredump and crash reader simply wouldn't have been >> able to navigate to actual build for it. The rest of the coredump is still >> vital though. >> >> memcpy() could be gated behind an 'if' for safety if needed: >> >> info = cdev->ipc.config.fw_info; >> eof = info + FW_INFO_SIZE_MAX; >> /* navigate to fifth info segment (fw hash) */ >> for (i = 0; i < 4 && info < eof; i++, info++) >> /* info segments are separated by space each */ >> if ((info = strnchr(info, eof - info, ' ')) == NULL) >> break; > >> if (i == 4 && info < eof) >> memcpy(pos, info, min(eof - info, CATPT_DUMP_HASH_SIZE)); > > And here basically enough check is info against NULL, right? > Just try to look at different possibilities how to make code simpler and neater. > >> Didn't compile this, some typecheck fixes might be in order and so on. > What you meant is: if (i == 4 && !info) // instead of 'info < eof' right? If 4th space is last char in this string then info would end up being non-NULL and equal to 'eof' and thus memcpy() would get invoked with size=eof-info=0. catpt_coredump() is here to gather debug info for Intel folks to analyze in case of critical error. In ideal world, it should not be required at all as when we get here, there are bigger problems on our head. Above solution is simpler than what is prevent in v7 while also maintaining good readability - variable names - plus comments which you suggested. Czarek
On Tue, Sep 22, 2020 at 11:04:31AM +0000, Rojewski, Cezary wrote: > On 2020-09-22 11:04 AM, Andy Shevchenko wrote: > > On Mon, Sep 21, 2020 at 08:48:12PM +0000, Rojewski, Cezary wrote: > >> On 2020-09-21 8:41 PM, Andy Shevchenko wrote: > > ... > > >> While this should never happen (means user is somehow not making use of > >> officially released firmware binary), coredumps are useful only if you > >> have access to debug tools. In cases you'd mentioned, invalid hash would > >> have been dumped to coredump and crash reader simply wouldn't have been > >> able to navigate to actual build for it. The rest of the coredump is still > >> vital though. > >> > >> memcpy() could be gated behind an 'if' for safety if needed: > >> > >> info = cdev->ipc.config.fw_info; > >> eof = info + FW_INFO_SIZE_MAX; > >> /* navigate to fifth info segment (fw hash) */ > >> for (i = 0; i < 4 && info < eof; i++, info++) > >> /* info segments are separated by space each */ > >> if ((info = strnchr(info, eof - info, ' ')) == NULL) > >> break; > > > >> if (i == 4 && info < eof) > >> memcpy(pos, info, min(eof - info, CATPT_DUMP_HASH_SIZE)); > > > > And here basically enough check is info against NULL, right? > > Just try to look at different possibilities how to make code simpler and neater. > > > >> Didn't compile this, some typecheck fixes might be in order and so on. > > > > What you meant is: > if (i == 4 && !info) // instead of 'info < eof' > > right? Simply if (!info)... > If 4th space is last char in this string then info would end up being > non-NULL and equal to 'eof' and thus memcpy() would get invoked with > size=eof-info=0. ...which is not a problem. > catpt_coredump() is here to gather debug info for Intel folks to analyze > in case of critical error. In ideal world, it should not be required at > all as when we get here, there are bigger problems on our head. > Above solution is simpler than what is prevent in v7 while also > maintaining good readability - variable names - plus comments which you > suggested. Thanks!
On Tue, Sep 22, 2020 at 02:29:10PM +0300, Andy Shevchenko wrote: > On Tue, Sep 22, 2020 at 11:04:31AM +0000, Rojewski, Cezary wrote: > > On 2020-09-22 11:04 AM, Andy Shevchenko wrote: > > > On Mon, Sep 21, 2020 at 08:48:12PM +0000, Rojewski, Cezary wrote: ... > > > And here basically enough check is info against NULL, right? > > > Just try to look at different possibilities how to make code simpler and neater. > > > > > >> Didn't compile this, some typecheck fixes might be in order and so on. > > > > > > > What you meant is: > > if (i == 4 && !info) // instead of 'info < eof' > > > > right? > > Simply if (!info)... if (info) memcpy(); of course, otherwise it will crash. > > If 4th space is last char in this string then info would end up being > > non-NULL and equal to 'eof' and thus memcpy() would get invoked with > > size=eof-info=0. > > ...which is not a problem.
On 2020-09-22 1:30 PM, Andy Shevchenko wrote: > On Tue, Sep 22, 2020 at 02:29:10PM +0300, Andy Shevchenko wrote: >> On Tue, Sep 22, 2020 at 11:04:31AM +0000, Rojewski, Cezary wrote: >>> On 2020-09-22 11:04 AM, Andy Shevchenko wrote: >>>> On Mon, Sep 21, 2020 at 08:48:12PM +0000, Rojewski, Cezary wrote: ... >>>> And here basically enough check is info against NULL, right? >>>> Just try to look at different possibilities how to make code simpler and neater. >>>> >>>>> Didn't compile this, some typecheck fixes might be in order and so on. >>>> >>> >>> What you meant is: >>> if (i == 4 && !info) // instead of 'info < eof' >>> >>> right? >> >> Simply if (!info)... > > if (info) > memcpy(); > > of course, otherwise it will crash. > Indeed, sorry about the typo. >>> If 4th space is last char in this string then info would end up being >>> non-NULL and equal to 'eof' and thus memcpy() would get invoked with >>> size=eof-info=0. >> >> ...which is not a problem. > And what about the case where 1st space is the last char? We quit the loop afterward, 'info' will not be NULL, 'i' on the other hand certainly won't be 4. TLDR: without checking 'i' how am I sure about whether or not cursor is actually at 5th segment? Czarek
diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h index 0953989e24b3..3c860c0645dc 100644 --- a/sound/soc/intel/catpt/core.h +++ b/sound/soc/intel/catpt/core.h @@ -9,6 +9,7 @@ #define __SND_SOC_INTEL_CATPT_CORE_H #include <linux/dma/dw.h> +#include <linux/irqreturn.h> #include "messages.h" #include "registers.h" @@ -126,4 +127,6 @@ int catpt_dsp_send_msg_timeout(struct catpt_dev *cdev, int catpt_dsp_send_msg(struct catpt_dev *cdev, struct catpt_ipc_msg request, struct catpt_ipc_msg *reply); +int catpt_coredump(struct catpt_dev *cdev); + #endif diff --git a/sound/soc/intel/catpt/dsp.c b/sound/soc/intel/catpt/dsp.c index b0a61fcca50c..cfd2fe1fc62a 100644 --- a/sound/soc/intel/catpt/dsp.c +++ b/sound/soc/intel/catpt/dsp.c @@ -5,6 +5,7 @@ // Author: Cezary Rojewski <cezary.rojewski@intel.com> // +#include <linux/devcoredump.h> #include <linux/dma-mapping.h> #include <linux/firmware.h> #include "core.h" @@ -136,3 +137,101 @@ void catpt_dmac_remove(struct catpt_dev *cdev) */ dw_dma_remove(cdev->dmac); } + +#define CATPT_DUMP_MAGIC 0xcd42 +#define CATPT_DUMP_SECTION_ID_FILE 0x00 +#define CATPT_DUMP_SECTION_ID_IRAM 0x01 +#define CATPT_DUMP_SECTION_ID_DRAM 0x02 +#define CATPT_DUMP_SECTION_ID_REGS 0x03 + +struct catpt_dump_section_hdr { + u16 magic; + u8 core_id; + u8 section_id; + u32 size; +}; + +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 */ + + 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++) { + if (cdev->ipc.config.fw_info[j] == ' ') + break; + *(pos + j - i) = cdev->ipc.config.fw_info[j]; + } + pos += 20; + + 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; +} diff --git a/sound/soc/intel/catpt/ipc.c b/sound/soc/intel/catpt/ipc.c index 500d4845a7cf..c49c9cf66075 100644 --- a/sound/soc/intel/catpt/ipc.c +++ b/sound/soc/intel/catpt/ipc.c @@ -168,6 +168,10 @@ static void catpt_dsp_process_response(struct catpt_dev *cdev, u32 header) switch (msg.type) { case CATPT_GLB_REQUEST_CORE_DUMP: + dev_err(cdev->dev, "ADSP device coredump received\n"); + ipc->ready = false; + catpt_coredump(cdev); + /* TODO: attempt recovery */ break; case CATPT_GLB_STREAM_MESSAGE: diff --git a/sound/soc/intel/catpt/messages.c b/sound/soc/intel/catpt/messages.c new file mode 100644 index 000000000000..8358300cddd2 --- /dev/null +++ b/sound/soc/intel/catpt/messages.c @@ -0,0 +1,313 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright(c) 2020 Intel Corporation. All rights reserved. +// +// Author: Cezary Rojewski <cezary.rojewski@intel.com> +// + +#include <linux/slab.h> +#include "core.h" +#include "messages.h" +#include "registers.h" + +int catpt_ipc_get_fw_version(struct catpt_dev *cdev, + struct catpt_fw_version *version) +{ + union catpt_global_msg msg = CATPT_GLOBAL_MSG(GET_FW_VERSION); + struct catpt_ipc_msg request = {{0}}, reply; + int ret; + + request.header = msg.val; + reply.size = sizeof(*version); + reply.data = version; + + ret = catpt_dsp_send_msg(cdev, request, &reply); + if (ret) + dev_err(cdev->dev, "get fw version failed: %d\n", ret); + + return ret; +} + +struct catpt_alloc_stream_input { + enum catpt_path_id path_id:8; + enum catpt_stream_type stream_type:8; + enum catpt_format_id format_id:8; + u8 reserved; + struct catpt_audio_format input_format; + struct catpt_ring_info ring_info; + u8 num_entries; + /* flex array with entries here */ + struct catpt_memory_info persistent_mem; + struct catpt_memory_info scratch_mem; + u32 num_notifications; /* obsolete */ +} __packed; + +int catpt_ipc_alloc_stream(struct catpt_dev *cdev, + enum catpt_path_id path_id, + enum catpt_stream_type type, + struct catpt_audio_format *afmt, + struct catpt_ring_info *rinfo, + u8 num_modules, + struct catpt_module_entry *modules, + struct resource *persistent, + struct resource *scratch, + struct catpt_stream_info *sinfo) +{ + union catpt_global_msg msg = CATPT_GLOBAL_MSG(ALLOCATE_STREAM); + struct catpt_alloc_stream_input input; + struct catpt_ipc_msg request, reply; + size_t size, arrsz; + u8 *payload; + off_t off; + int ret; + + off = offsetof(struct catpt_alloc_stream_input, persistent_mem); + arrsz = sizeof(*modules) * num_modules; + size = sizeof(input) + arrsz; + + payload = kzalloc(size, GFP_KERNEL); + if (!payload) + return -ENOMEM; + + memset(&input, 0, sizeof(input)); + input.path_id = path_id; + input.stream_type = type; + input.format_id = CATPT_FORMAT_PCM; + input.input_format = *afmt; + input.ring_info = *rinfo; + input.num_entries = num_modules; + input.persistent_mem.offset = catpt_to_dsp_offset(persistent->start); + input.persistent_mem.size = resource_size(persistent); + if (scratch) { + input.scratch_mem.offset = catpt_to_dsp_offset(scratch->start); + input.scratch_mem.size = resource_size(scratch); + } + + /* re-arrange the input: account for flex array 'entries' */ + memcpy(payload, &input, sizeof(input)); + memmove(payload + off + arrsz, payload + off, sizeof(input) - off); + memcpy(payload + off, modules, arrsz); + + request.header = msg.val; + request.size = size; + request.data = payload; + reply.size = sizeof(*sinfo); + reply.data = sinfo; + + ret = catpt_dsp_send_msg(cdev, request, &reply); + if (ret) + dev_err(cdev->dev, "alloc stream type %d failed: %d\n", + type, ret); + + kfree(payload); + return ret; +} + +int catpt_ipc_free_stream(struct catpt_dev *cdev, u8 stream_hw_id) +{ + union catpt_global_msg msg = CATPT_GLOBAL_MSG(FREE_STREAM); + struct catpt_ipc_msg request; + int ret; + + request.header = msg.val; + request.size = sizeof(stream_hw_id); + request.data = &stream_hw_id; + + ret = catpt_dsp_send_msg(cdev, request, NULL); + if (ret) + dev_err(cdev->dev, "free stream %d failed: %d\n", + stream_hw_id, ret); + + return ret; +} + +int catpt_ipc_set_device_format(struct catpt_dev *cdev, + struct catpt_ssp_device_format *devfmt) +{ + union catpt_global_msg msg = CATPT_GLOBAL_MSG(SET_DEVICE_FORMATS); + struct catpt_ipc_msg request; + int ret; + + request.header = msg.val; + request.size = sizeof(*devfmt); + request.data = devfmt; + + ret = catpt_dsp_send_msg(cdev, request, NULL); + if (ret) + dev_err(cdev->dev, "set device format failed: %d\n", ret); + + return ret; +} + +int catpt_ipc_enter_dxstate(struct catpt_dev *cdev, enum catpt_dx_state state, + struct catpt_dx_context *context) +{ + union catpt_global_msg msg = CATPT_GLOBAL_MSG(ENTER_DX_STATE); + struct catpt_ipc_msg request, reply; + int ret; + + request.header = msg.val; + request.size = sizeof(state); + request.data = &state; + reply.size = sizeof(*context); + reply.data = context; + + ret = catpt_dsp_send_msg(cdev, request, &reply); + if (ret) + dev_err(cdev->dev, "enter dx state failed: %d\n", ret); + + return ret; +} + +int catpt_ipc_get_mixer_stream_info(struct catpt_dev *cdev, + struct catpt_mixer_stream_info *info) +{ + union catpt_global_msg msg = CATPT_GLOBAL_MSG(GET_MIXER_STREAM_INFO); + struct catpt_ipc_msg request = {{0}}, reply; + int ret; + + request.header = msg.val; + reply.size = sizeof(*info); + reply.data = info; + + ret = catpt_dsp_send_msg(cdev, request, &reply); + if (ret) + dev_err(cdev->dev, "get mixer info failed: %d\n", ret); + + return ret; +} + +int catpt_ipc_reset_stream(struct catpt_dev *cdev, u8 stream_hw_id) +{ + union catpt_stream_msg msg = CATPT_STREAM_MSG(RESET_STREAM); + struct catpt_ipc_msg request = {{0}}; + int ret; + + msg.stream_hw_id = stream_hw_id; + request.header = msg.val; + + ret = catpt_dsp_send_msg(cdev, request, NULL); + if (ret) + dev_err(cdev->dev, "reset stream %d failed: %d\n", + stream_hw_id, ret); + + return ret; +} + +int catpt_ipc_pause_stream(struct catpt_dev *cdev, u8 stream_hw_id) +{ + union catpt_stream_msg msg = CATPT_STREAM_MSG(PAUSE_STREAM); + struct catpt_ipc_msg request = {{0}}; + int ret; + + msg.stream_hw_id = stream_hw_id; + request.header = msg.val; + + ret = catpt_dsp_send_msg(cdev, request, NULL); + if (ret) + dev_err(cdev->dev, "pause stream %d failed: %d\n", + stream_hw_id, ret); + + return ret; +} + +int catpt_ipc_resume_stream(struct catpt_dev *cdev, u8 stream_hw_id) +{ + union catpt_stream_msg msg = CATPT_STREAM_MSG(RESUME_STREAM); + struct catpt_ipc_msg request = {{0}}; + int ret; + + msg.stream_hw_id = stream_hw_id; + request.header = msg.val; + + ret = catpt_dsp_send_msg(cdev, request, NULL); + if (ret) + dev_err(cdev->dev, "resume stream %d failed: %d\n", + stream_hw_id, ret); + + return ret; +} + +struct catpt_set_volume_input { + u32 channel; + u32 target_volume; + u64 curve_duration; + enum catpt_audio_curve_type curve_type __aligned(4); +} __packed; + +int catpt_ipc_set_volume(struct catpt_dev *cdev, u8 stream_hw_id, + u32 channel, u32 volume, + u32 curve_duration, + enum catpt_audio_curve_type curve_type) +{ + union catpt_stream_msg msg = CATPT_STAGE_MSG(SET_VOLUME); + struct catpt_ipc_msg request; + struct catpt_set_volume_input input; + int ret; + + msg.stream_hw_id = stream_hw_id; + input.channel = channel; + input.target_volume = volume; + input.curve_duration = curve_duration; + input.curve_type = curve_type; + + request.header = msg.val; + request.size = sizeof(input); + request.data = &input; + + ret = catpt_dsp_send_msg(cdev, request, NULL); + if (ret) + dev_err(cdev->dev, "set stream %d volume failed: %d\n", + stream_hw_id, ret); + + return ret; +} + +struct catpt_set_write_pos_input { + u32 new_write_pos; + bool end_of_buffer; + bool low_latency; +} __packed; + +int catpt_ipc_set_write_pos(struct catpt_dev *cdev, u8 stream_hw_id, + u32 pos, bool eob, bool ll) +{ + union catpt_stream_msg msg = CATPT_STAGE_MSG(SET_WRITE_POSITION); + struct catpt_ipc_msg request; + struct catpt_set_write_pos_input input; + int ret; + + msg.stream_hw_id = stream_hw_id; + input.new_write_pos = pos; + input.end_of_buffer = eob; + input.low_latency = ll; + + request.header = msg.val; + request.size = sizeof(input); + request.data = &input; + + ret = catpt_dsp_send_msg(cdev, request, NULL); + if (ret) + dev_err(cdev->dev, "set stream %d write pos failed: %d\n", + stream_hw_id, ret); + + return ret; +} + +int catpt_ipc_mute_loopback(struct catpt_dev *cdev, u8 stream_hw_id, bool mute) +{ + union catpt_stream_msg msg = CATPT_STAGE_MSG(MUTE_LOOPBACK); + struct catpt_ipc_msg request; + int ret; + + msg.stream_hw_id = stream_hw_id; + request.header = msg.val; + request.size = sizeof(mute); + request.data = &mute; + + ret = catpt_dsp_send_msg(cdev, request, NULL); + if (ret) + dev_err(cdev->dev, "mute loopback failed: %d\n", ret); + + return ret; +}
Declare global and stream IPC message handlers for all known message types. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/intel/catpt/core.h | 3 + sound/soc/intel/catpt/dsp.c | 99 ++++++++++ sound/soc/intel/catpt/ipc.c | 4 + sound/soc/intel/catpt/messages.c | 313 +++++++++++++++++++++++++++++++ 4 files changed, 419 insertions(+) create mode 100644 sound/soc/intel/catpt/messages.c