RE: [PATCH] ASoC: SOF: Intel: hda: Stop the controller before freeing its DMA buffers

[Li, Yifan2]

> -----Original Message-----
> From: Péter Ujfalusi <peter.ujfalusi@xxxxxxxxxxxxxxx>
> Sent: Thursday, October 29, 2026 3:47 PM
> To: Sasha Levin <sashal@xxxxxxxxxx>; Liam Girdwood <lgirdwood@xxxxxxxxx>;
> Mark Brown <broonie@xxxxxxxxxx>
> Cc: Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>; Kai Vehmanen
> <kai.vehmanen@xxxxxxxxxxxxxxx>; Pierre-Louis Bossart <pierre-
> louis.bossart@xxxxxxxxx>; Daniel Baluta <daniel.baluta@xxxxxxx>; Takashi
> Iwai <tiwai@xxxxxxxx>; Jaroslav Kysela <perex@xxxxxxxx>; sound-open-
> firmware@xxxxxxxxxxxxxxxx; linux-sound@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Li, Yifan2 <yifan2.li@xxxxxxxxx>
> Subject: Re: [PATCH] ASoC: SOF: Intel: hda: Stop the controller before freeing
> its DMA buffers
> 
> 
> 
> On 28/10/2026 16:44, Sasha Levin wrote:
> > hda_dsp_probe() starts the CORB/RIRB DMA engines through
> > hda_dsp_ctrl_init_chip(), but neither hda_dsp_remove() nor the error
> > path of hda_dsp_probe() stops them before hda_dsp_stream_free() frees
> > the CORB/RIRB ring buffer and the position buffer.  Unsolicited
> > responses also stay enabled, so the controller can keep writing RIRB
> > entries into the freed page.
> >
> > Until commit 9e6966f70404 ("ASoC: SOF: Intel: hda: Keep non-alt mlinks
> > powered at probe on ACE2+"), hda_init_caps() dropped the initial
> > references to the non-alternate links, and dropping the last one
> > stopped the command DMA, so a probe that failed later, e.g. because
> > the firmware is missing, did not leave it running.  Now it does, and
> > as the probe runs from a work item, the device stays bound after the
> > failure, with bus mastering enabled and the DMA engines pointed at
> > freed memory.
> >
> > Normal removal resumes the device first, which restarts the command
> > DMA, so there the ring buffer is freed while the DMA is running until
> > pci_disable_device() clears bus mastering.
> >
> > Stop the controller with hda_dsp_ctrl_stop_chip() before freeing the
> > buffers, as is done on suspend, and export it for use from hda.c.
> >
> > This was reproduced by binding SOF in DSPless mode to a QEMU
> > ich9-intel-hda: after the probe fails with "SOF firmware and/or
> > topology file not found", CORB and RIRB DMA are still running with
> > unsolicited responses and bus mastering enabled, and the page they use
> > is soon reused as a page table.
> 
> This is indeed a possibility which I have overlooked.
> FWIW, Li Yifan spotted the same issue earlier:
> https://github.com/thesofproject/linux/pull/5976
> 
> but using hda_dsp_ctrl_stop_chip() is the better way.

Thanks Péter, and thanks Sasha for the fix. Agreed, this is better than
my patch, which only stopped the CORB/RIRB DMA in hda_dsp_remove(),
so I will drop mine in favor of it.

Some data from real hardware, in case it is useful.

We hit this on NovaLake-S with a v7.3-rc3 based kernel and the SOF
firmware not installed. After the probe failure the IOMMU kept reporting
faults from the audio controller(you can also find in the sof linux pull request):

sof-audio-pci-intel-nvl 0000:80:1f.3: error: sof_probe_work failed err: -2
DMAR: DRHD: handling fault status reg 2
DMAR: [DMA Write NO_PASID] Request device [0000:80:1f.3] fault addr 0xffffe000 [fault reason 0x71] SM: Present bit in first-level paging entry is clear

and RIRB DMA and unsolicited responses were still enabled
(RIRBCTL=0x3, GCTL.UNSOL=1).

I tested Sasha's patch on NovaLake-S with a v7.3 based kernel, also with
the firmware missing. The only HDA codec there is the display codec,
which sends no unsolicited responses after the failed probe, so the
DMAR faults do not show up, but the controller state does. Without the
patch, RIRB DMA and unsolicited responses stay enabled (RIRBCTL=0x3,
GCTL.UNSOL=1; CORBRUN is already clear) with bus mastering on and
RIRBBASE pointing at the freed buffer, after every failed probe (at
boot and in 5 unbind/bind cycles). With the patch, CORB/RIRB DMA,
unsolicited responses and the position buffer are disabled in all of
these cases, and no other issues were seen.

Tested-by: Li Yifan <yifan2.li@xxxxxxxxx>

Sasha, when you send v2 without the dd96daca6c83 Fixes tag, could you
please also add:

Reported-by: Li Yifan <yifan2.li@xxxxxxxxx>
Closes: https://github.com/thesofproject/linux/pull/5976

Thanks,
Yifan

> > Fixes: dd96daca6c83 ("ASoC: SOF: Intel: Add APL/CNL HW DSP support")
> 
> Can you drop this Fixes tag, the hda-ctrl.c hunk will not apply nicely to older
> stable kernels With the removed tag:
> Acked-by: Peter Ujfalusi <peter.ujfalusi@xxxxxxxxxxxxxxx>
> 
> > Fixes: 9e6966f70404 ("ASoC: SOF: Intel: hda: Keep non-alt mlinks
> > powered at probe on ACE2+")
> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> 
> 
> 
> > ---
> >  sound/soc/sof/intel/hda-ctrl.c | 1 +
> >  sound/soc/sof/intel/hda.c      | 4 ++++
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/sound/soc/sof/intel/hda-ctrl.c
> > b/sound/soc/sof/intel/hda-ctrl.c index aeb34310eebd6..333f6bbea7539
> > 100644
> > --- a/sound/soc/sof/intel/hda-ctrl.c
> > +++ b/sound/soc/sof/intel/hda-ctrl.c
> > @@ -349,6 +349,7 @@ void hda_dsp_ctrl_stop_chip(struct snd_sof_dev
> > *sdev)
> >
> >  	bus->chip_init = false;
> >  }
> > +EXPORT_SYMBOL_NS(hda_dsp_ctrl_stop_chip,
> > +"SND_SOC_SOF_INTEL_HDA_COMMON");
> >
> >  MODULE_LICENSE("Dual BSD/GPL");
> >  MODULE_DESCRIPTION("SOF helpers for HDaudio platforms"); diff --git
> > a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index
> > 4dbba9186b29d..2a40cb9bf24d9 100644
> > --- a/sound/soc/sof/intel/hda.c
> > +++ b/sound/soc/sof/intel/hda.c
> > @@ -939,6 +939,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
> >  		hda_dsp_ctrl_ppcap_enable(sdev, false);
> >  	}
> >  free_ipc_irq:
> > +	hda_dsp_ctrl_stop_chip(sdev);
> >  	free_irq(sdev->ipc_irq, sdev);
> >  free_irq_vector:
> >  	if (sdev->msi_enabled)
> > @@ -1006,6 +1007,9 @@ void hda_dsp_remove(struct snd_sof_dev *sdev)
> >  		snd_dma_free_pages(&hda->iccmax_dmab);
> >
> >  skip_disable_dsp:
> > +	/* stop CORB/RIRB and position buffer DMA before the buffers are
> freed */
> > +	hda_dsp_ctrl_stop_chip(sdev);
> > +
> >  	free_irq(sdev->ipc_irq, sdev);
> >  	if (sdev->msi_enabled)
> >  		pci_free_irq_vectors(pci);
> 
> --
> Péter