Fix IPC timeout issue on APL&CNL by RanderWang · Pull Request #28 · thesofproject/linux

RanderWang · 2018-07-06T09:52:53Z

I fix merge conflict issue on latest code

The IPC algorithm depends on the status of snd_sof_ipc_reply. The failed case is processed different with success one Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

There are two direction now in cmd_done, one is reply of host to DSP, another is reply of DSP to host.The origin cmd_done is for reply of host to DSP. Cmd_done should be called after host has processed the reply msg from dsp. This function is for some platforms to do extra steps to make the following ipc msgs sent safely. Now the direction supported by cmd_done would be host reply to dsp or dsp reply to host. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

Refine the implementation of is_dsp_ready on APL&CNL BUSY bit means dsp is ready, DONE bit means host has processed the reply msg. The next ipc msg should check both bits to make sure both dsp and host is ready Signed-off-by: Rander Wang <rander.wang@linux.intel.com> Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

plbossart

Please fix multi-line comment issues.
Also please explain what you do with the results from snd_sof_ipc_reply(). It looks like you are executing the test cases only when there is an error, not sure if this is really what you intended - or the comments are not self-explanatory

plbossart · 2018-07-09T18:44:52Z

nit-pick: fix multi-line comments

plbossart · 2018-07-09T18:45:15Z

comment style here as well

plbossart · 2018-07-09T18:47:25Z

snd_soc_ipc_reply returns 0 or -EINVAL, the 1 isn't incorrect but misleading. Change to -EINVAL for consistency?

I will change it to -EINVAL

plbossart · 2018-07-09T18:48:22Z

is this what you want to do when the reply is -EINVAL?

yes, if the return value is 0, the cmd_done would be called at the end of msg processing function.
But if is -EINVAL, no msg processing function would be called, so it could be called at here to make interrupt status correct in dsp

plbossart · 2018-07-09T18:49:00Z

multiline-comments

i will refine it

plbossart · 2018-07-09T18:53:52Z

same comment as for APL, this is done when the snd_soc_ipc_reply returns -EINVAL, is this intentional?

yes, it is for unknown msgs

plbossart · 2018-07-09T18:54:16Z

multi-line comments

I will refine it

plbossart · 2018-07-09T18:56:58Z

multi-line comments

I will refine it

plbossart · 2018-07-09T18:57:08Z

multi-line comments

I will refine it

plbossart · 2018-07-09T18:58:31Z

multi-line comments

I will refine it

RanderWang · 2018-07-10T06:25:18Z

I update my patches. Why I change snd_sof_ipc_reply(), just have a look at following section.

When host receive reply messages from dsp, the done bit should be set. Before my change, it is set in IRQ function. Now I move it to cmd_done function which is called at end of message processing function. In some cases, unknown message is gotten in IRQ function and no message processing function is called for this message. So done bit can't be set in cmd_done function, and it could only be set in IRQ function

Set DONE bit in cmd_done function which processes msgs, rather than doing it in IRQ function. But if a msg is not in the rx list, it can only be done at IRQ function Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

Set DONE bit in cmd_done function if direction is DSP_REPLY rather than doing it in IRQ function. But if a msg is not in the rx list, it can only be done at IRQ function Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

(1) Set DONE bit in cmd_done function which processes msgs, rather than doing it in IRQ function. But if a msg is not in the rx list, it can only be done at IRQ function (2) Check DONE bit in hsw_is_ready function Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

(1) Set DONE bit in cmd_done function which processes msgs, rather than doing it in IRQ function. But if a msg is not in the rx list, it can only be done at IRQ function (2) Check DONE bit in bdw_is_ready function Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

(1) Set DONE bit in cmd_done function which processes msgs, rather than doing it in IRQ function. But if a msg is not in the rx list, it can only be done at IRQ function (2) Check DONE bit in byt_is_ready function Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

plbossart

Sorry, I still don't get why in the APL case you ignore the ROM messages and in CNL you don't?

I am referring to this code in hda-ipc.c:
if (msg != 0x1004000)
reply = snd_sof_ipc_reply(sdev, msg);

which isn't present for CNL

RanderWang · 2018-07-11T01:50:37Z

The code if(msg != 0x1004000) is added by Keyon. Because at boot up time, ROM send a reply msg to host, but host cant found it , so a error msg would be in dmesg. Customer complaint about error msg on APL . So Keyon did this on APL.

RanderWang · 2018-07-11T01:56:42Z

For ROM messages on CNL & APL, both would call cmd_done in IRQ function. For ROM messages on
CNL, the snd_sof_ipc_reply would return -EINVL, the cmd_done would be called. On APL, For ROM messages 0x1004000, cmd_done would called for reply = -EINVL, and other ROM msgs work as on CNL. So no difference.

plbossart · 2018-07-11T15:49:39Z

@RanderWang I did not understand your explanations at all... I was asking why there is a difference between two platforms that are mostly identical, and you didn't give me the root cause.

Anyway, I will merge this since it's been dragging for a long-time and I am tired of this thread, but be prepared for a revert if this introduces problems.

Applying dynamic usbcore quirks in early booting when the slab is not yet ready would cause kernel panic of null pointer dereference because the quirk_count has been counted as 1 while the quirk_list was failed to allocate. i.e., [ 1.044970] BUG: unable to handle kernel NULL pointer dereference at (null) [ 1.044995] IP: [<ffffffffb0953ec7>] usb_detect_quirks+0x88/0xd1 [ 1.045016] PGD 0 [ 1.045026] Oops: 0000 [#1] PREEMPT SMP [ 1.046986] gsmi: Log Shutdown Reason 0x03 [ 1.046995] Modules linked in: [ 1.047008] CPU: 0 PID: 81 Comm: kworker/0:3 Not tainted 4.4.154 #28 [ 1.047016] Hardware name: Google Coral/Coral, BIOS Google_Coral.10068.27.0 12/04/2017 [ 1.047028] Workqueue: usb_hub_wq hub_event [ 1.047037] task: ffff88017a321c80 task.stack: ffff88017a384000 [ 1.047044] RIP: 0010:[<ffffffffb0953ec7>] [<ffffffffb0953ec7>] usb_detect_quirks+0x88/0xd1 To tackle this odd, let's balance the quirk_count to 0 when the kcalloc call fails, and defer the quirk setting into a lower level callback which ensures that the kernel memory management has been initialized. Fixes: 027bd6c ("usb: core: Add "quirks" parameter for usbcore") Signed-off-by: Harry Pan <harry.pan@intel.com> Acked-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Cc: stable <stable@vger.kernel.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

RanderWang added 3 commits July 6, 2018 17:35

ASoC: SOF: return status in snd_sof_ipc_reply

9535ab2

The IPC algorithm depends on the status of snd_sof_ipc_reply. The failed case is processed different with success one Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

plbossart requested changes Jul 9, 2018

View reviewed changes

RanderWang force-pushed the ipx_fix branch from db1a5c3 to f58e491 Compare July 10, 2018 06:10

RanderWang added 5 commits July 10, 2018 14:29

ASoC: SOF: process SOF_IPC_DSP_REPLY case in cmd_done on APL

4879348

Set DONE bit in cmd_done function which processes msgs, rather than doing it in IRQ function. But if a msg is not in the rx list, it can only be done at IRQ function Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

RanderWang force-pushed the ipx_fix branch from f58e491 to d597225 Compare July 10, 2018 06:32

plbossart reviewed Jul 10, 2018

View reviewed changes

plbossart merged commit a3a39f2 into thesofproject:topic/sof-dev Jul 11, 2018

RanderWang deleted the ipx_fix branch July 24, 2018 05:35

aiChaoSONG mentioned this pull request Oct 28, 2019

[BUG][CML] Kernel NULL pointer deference in module reload stress test #1386

Closed

Conversation

RanderWang commented Jul 6, 2018

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Jul 10, 2018

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Jul 11, 2018

Uh oh!

RanderWang commented Jul 11, 2018

Uh oh!

plbossart commented Jul 11, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RanderWang Jul 10, 2018 •

edited

Loading

RanderWang Jul 10, 2018 •

edited

Loading