linux-kernel - Re: [PATCH 3/4] ASoC: SOF: amd: Handle IPC replies before FW_BOOT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eb1628c2-b209-40a1-9f20-1057c2815536@amd.com>
Date: Fri, 7 Feb 2025 17:54:57 +0530
From: "Mukunda,Vijendar" <vijendar.mukunda@....com>
To: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>,
 Liam Girdwood <lgirdwood@...il.com>,
 Peter Ujfalusi <peter.ujfalusi@...ux.intel.com>,
 Bard Liao <yung-chuan.liao@...ux.intel.com>,
 Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
 Daniel Baluta <daniel.baluta@....com>,
 Kai Vehmanen <kai.vehmanen@...ux.intel.com>,
 Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>,
 Mark Brown <broonie@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
 Takashi Iwai <tiwai@...e.com>,
 Venkata Prasad Potturu <venkataprasad.potturu@....com>,
 "Hiregoudar, Basavaraj" <Basavaraj.Hiregoudar@....com>,
 "Dommati, Sunil-kumar" <Sunil-kumar.Dommati@....com>
Cc: kernel@...labora.com, sound-open-firmware@...a-project.org,
 linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] ASoC: SOF: amd: Handle IPC replies before
 FW_BOOT_COMPLETE

On 07/02/25 17:46, Cristian Ciocaltea wrote:
> On 2/7/25 1:55 PM, Mukunda,Vijendar wrote:
>> On 07/02/25 17:16, Cristian Ciocaltea wrote:
>>> In some cases, e.g. during resuming from suspend, there is a possibility
>>> that some IPC reply messages get received by the host while the DSP
>>> firmware has not yet reached the complete boot state.
>>>
>>> Detect when this happens and do not attempt to process the unexpected
>>> replies from DSP.  Instead, provide proper debugging support.
>> As per our understanding, before FW boot completion there won't
>> be any IPC responses sent from Firmware.
>> In this case, do we really need such a condition check?
> During the suspend/resume stress testing I was able to get this kind of
> messages, and that's the actual reason for introducing the verification.
>
> Also it doesn't seem to be uncommon, e.g. Intel HDA IPC also provides
> similar checks.
>
Could you please share reference logs to know which IPC messages
are being received before FW_READY message/FW boot complete?
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
>>> ---
>>>  sound/soc/sof/amd/acp-ipc.c | 23 ++++++++++++++++-------
>>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/sound/soc/sof/amd/acp-ipc.c b/sound/soc/sof/amd/acp-ipc.c
>>> index 5f371d9263f3bad507236ace95b7ef323c369187..12caefd08788595be8de03a863b88b5bbc15847d 100644
>>> --- a/sound/soc/sof/amd/acp-ipc.c
>>> +++ b/sound/soc/sof/amd/acp-ipc.c
>>> @@ -167,6 +167,7 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>>>  
>>>  	if (sdev->first_boot && sdev->fw_state != SOF_FW_BOOT_COMPLETE) {
>>>  		acp_mailbox_read(sdev, sdev->dsp_box.offset, &status, sizeof(status));
>>> +
>>>  		if ((status & SOF_IPC_PANIC_MAGIC_MASK) == SOF_IPC_PANIC_MAGIC) {
>>>  			snd_sof_dsp_panic(sdev, sdev->dsp_box.offset + sizeof(status),
>>>  					  true);
>>> @@ -188,13 +189,21 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>>>  
>>>  	dsp_ack = snd_sof_dsp_read(sdev, ACP_DSP_BAR, ACP_SCRATCH_REG_0 + dsp_ack_write);
>>>  	if (dsp_ack) {
>>> -		spin_lock_irq(&sdev->ipc_lock);
>>> -		/* handle immediate reply from DSP core */
>>> -		acp_dsp_ipc_get_reply(sdev);
>>> -		snd_sof_ipc_reply(sdev, 0);
>>> -		/* set the done bit */
>>> -		acp_dsp_ipc_dsp_done(sdev);
>>> -		spin_unlock_irq(&sdev->ipc_lock);
>>> +		if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) {
>>> +			spin_lock_irq(&sdev->ipc_lock);
>>> +
>>> +			/* handle immediate reply from DSP core */
>>> +			acp_dsp_ipc_get_reply(sdev);
>>> +			snd_sof_ipc_reply(sdev, 0);
>>> +			/* set the done bit */
>>> +			acp_dsp_ipc_dsp_done(sdev);
>>> +
>>> +			spin_unlock_irq(&sdev->ipc_lock);
>>> +		} else {
>>> +			dev_dbg_ratelimited(sdev->dev, "IPC reply before FW_BOOT_COMPLETE: %#x\n",
>>> +					    dsp_ack);
>>> +		}
>>> +
>>>  		ipc_irq = true;
>>>  	}
>>>  
>>>