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 for Android: free password hash cracker in your pocket

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

Message-ID: <f253bb70-5991-4ee6-9f44-ac59751c6a38@collabora.com>
Date: Fri, 7 Feb 2025 19:00:35 +0200
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: "Mukunda,Vijendar" <vijendar.mukunda@....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 2/7/25 2:24 PM, Mukunda,Vijendar wrote:
> 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?

As mentioned in a previous reply, I couldn't enable debugging during
stress testing because it had the annoying effect of hiding the audio
breakage issue.  I will try to experiment with some more targeted
instrumentation and see if it's possible to extract some useful data.

Regardless, I think it's worth keeping this check in place as it's
definitely helpful to spot potential FW issues (for current or future
products).

>>>> 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;
>>>>  	}
>>>>  
>>>>
>