- Sound-open-firmware - mailman.alsa-project.org

Re: [PATCH v6 11/12] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe
by Takashi Iwai 09 Oct '23

09 Oct '23

On Thu, 05 Oct 2023 13:26:18 +0200, Maarten Lankhorst wrote: > > > > On 2023-10-05 12:58, Péter Ujfalusi wrote: > > > > > > On 04/10/2023 19:59, Kai Vehmanen wrote: > >> Hi, > >> > >> I'm good with rest of the series, but one patch requires work. > >> > >> On Wed, 4 Oct 2023, Maarten Lankhorst wrote: > >> > >>> Now that we can use -EPROBE_DEFER, it's no longer required to spin off > >>> the snd_hdac_i915_init into a workqueue. > >>> > >>> Use the -EPROBE_DEFER mechanism instead, which must be returned in the > >>> probe function. > >>> > >>> The previously added probe_early can be used for this, > >>> and we also use the newly added remove_late for unbinding afterwards. > >> [...] > >>> --- a/sound/soc/sof/intel/hda-common-ops.c > >>> +++ b/sound/soc/sof/intel/hda-common-ops.c > >>> @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = { > >>> .probe_early = hda_dsp_probe_early, > >>> .probe = hda_dsp_probe, > >>> .remove = hda_dsp_remove, > >>> + .remove_late = hda_dsp_remove_late, > >>> /* Register IO uses direct mmio */ > >>> diff --git a/sound/soc/sof/intel/hda.c > >>> b/sound/soc/sof/intel/hda.c > >>> index 86a2571488bc..4eb7f04b8ae1 100644 > >>> --- a/sound/soc/sof/intel/hda.c > >>> +++ b/sound/soc/sof/intel/hda.c > >>> @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev) > >>> return -ENOMEM; > >>> sdev->pdata->hw_pdata = hdev; > >>> hdev->desc = chip; > >>> + ret = hda_init(sdev); > >>> err: > >>> return ret; > >> > >> I don't think this works. The hda_codec_i915_init() errors are ignored in > >> hda_init() so this never returns -EPROBE_DEFER. > >> > >> So something like this is needed on top (tested quickly on one SOF > >> machine and this blocks SOF load until i915 or xe driver is loaded): > >> > >> --cut-- > >> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c > >> index 9025bfaf6a7e..8b17c82dcc89 100644 > >> --- a/sound/soc/sof/intel/hda.c > >> +++ b/sound/soc/sof/intel/hda.c > >> @@ -863,13 +863,20 @@ static int hda_init(struct snd_sof_dev *sdev) > >> /* init i915 and HDMI codecs */ > >> ret = hda_codec_i915_init(sdev); > >> if (ret < 0) > >> - dev_warn(sdev->dev, "init of i915 and HDMI codec > >> failed\n"); > >> + dev_warn(sdev->dev, "init of i915 and HDMI codec failed > >> (%d)\n", ret); > > > > we should not print anything or maximum dev_dbg in case of EPROBE_DEFER. > There's dev_err_probe, which is dev_err on error, or sets the reason > for deferred probe to the arguments if the error is -EPROBE_DEFER. I expect you'll respin v7 for addressing this? I'd love to merge the series for 6.7, and the time ticks... thanks, Takashi

1 0

Re: [PATCH v6 11/12] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe
by Kai Vehmanen 05 Oct '23

05 Oct '23

Hi, I'm good with rest of the series, but one patch requires work. On Wed, 4 Oct 2023, Maarten Lankhorst wrote: > Now that we can use -EPROBE_DEFER, it's no longer required to spin off > the snd_hdac_i915_init into a workqueue. > > Use the -EPROBE_DEFER mechanism instead, which must be returned in the > probe function. > > The previously added probe_early can be used for this, > and we also use the newly added remove_late for unbinding afterwards. [...] > --- a/sound/soc/sof/intel/hda-common-ops.c > +++ b/sound/soc/sof/intel/hda-common-ops.c > @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = { > .probe_early = hda_dsp_probe_early, > .probe = hda_dsp_probe, > .remove = hda_dsp_remove, > + .remove_late = hda_dsp_remove_late, > > /* Register IO uses direct mmio */ > > diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c > index 86a2571488bc..4eb7f04b8ae1 100644 > --- a/sound/soc/sof/intel/hda.c > +++ b/sound/soc/sof/intel/hda.c > @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev) > return -ENOMEM; > sdev->pdata->hw_pdata = hdev; > hdev->desc = chip; > + ret = hda_init(sdev); > > err: > return ret; I don't think this works. The hda_codec_i915_init() errors are ignored in hda_init() so this never returns -EPROBE_DEFER. So something like this is needed on top (tested quickly on one SOF machine and this blocks SOF load until i915 or xe driver is loaded): --cut-- diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 9025bfaf6a7e..8b17c82dcc89 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -863,13 +863,20 @@ static int hda_init(struct snd_sof_dev *sdev) /* init i915 and HDMI codecs */ ret = hda_codec_i915_init(sdev); if (ret < 0) - dev_warn(sdev->dev, "init of i915 and HDMI codec failed\n"); + dev_warn(sdev->dev, "init of i915 and HDMI codec failed (%d)\n", ret); + + if (ret < 0 && ret != -ENODEV) + goto out; /* get controller capabilities */ ret = hda_dsp_ctrl_get_caps(sdev); if (ret < 0) dev_err(sdev->dev, "error: get caps error\n"); +out: + if (ret < 0) + iounmap(sof_to_bus(sdev)->remap_addr); + return ret; } --cut-- Br, Kai

2 1

Re: [PATCH] ASoC: SOF: Intel: Add error handling in hda_dsp_trace_release
by Pierre-Louis Bossart 03 Oct '23

03 Oct '23

On 9/23/23 05:12, liuhaoran wrote: > This patch adds error-handling for the hda_dsp_stream_put() inside > the hda_dsp_trace_release function in the hda-trace.c file. > > Signed-off-by: liuhaoran <liuhaoran14(a)163.com> > --- > sound/soc/sof/intel/hda-trace.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/sof/intel/hda-trace.c b/sound/soc/sof/intel/hda-trace.c > index cbb9bd7770e6..5dcd43b9cbe4 100644 > --- a/sound/soc/sof/intel/hda-trace.c > +++ b/sound/soc/sof/intel/hda-trace.c > @@ -73,13 +73,22 @@ int hda_dsp_trace_release(struct snd_sof_dev *sdev) > { > struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; > struct hdac_stream *hstream; > + int ret; > > if (hda->dtrace_stream) { > hstream = &hda->dtrace_stream->hstream; > - hda_dsp_stream_put(sdev, > + ret = hda_dsp_stream_put(sdev, > SNDRV_PCM_STREAM_CAPTURE, > hstream->stream_tag); > + > hda->dtrace_stream = NULL; > + > + if (ret < 0) { > + dev_dbg(sdev->dev, > + "stream put failed: %d\n", ret); > + return ret; > + } > + I don't know if returning an error helps here, this might get in the way of the suspend. Peter, what do you think? > return 0; > } >

1 0

Re: [PATCH v5 10/12] ASoC: Intel: Skylake: Move snd_hdac_i915_init to before probe_work.
by Amadeusz Sławiński 03 Oct '23

03 Oct '23

On 10/2/2023 6:52 PM, Maarten Lankhorst wrote: > Now that we can use -EPROBE_DEFER, it's no longer required to spin off > the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue > can be destroyed, but I don't have the means to test this. > > Removing the workqueue would simplify init even further, but is left > as exercise for the reviewer. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > Acked-by: Mark Brown <broonie(a)kernel.org> > Reviewed-by: Kai Vehmanen <kai.vehmanen(a)linux.intel.com> > Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski(a)linux.intel.com> > --- Isn't the convention that your Signed-off should be last when you are sending the patches? Or does it only apply to Signed-off lines themselves and other lines can be anywhere?

1 1

Re: [PATCH][next] ASoC: SOF: ipc4-topology: Use size_add() in call to struct_size()
by Mark Brown 02 Oct '23

02 Oct '23

On Fri, 15 Sep 2023 13:09:11 -0600, Gustavo A. R. Silva wrote: > If, for any reason, the open-coded arithmetic causes a wraparound, > the protection that `struct_size()` adds against potential integer > overflows is defeated. Fix this by hardening call to `struct_size()` > with `size_add()`. > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: ipc4-topology: Use size_add() in call to struct_size() commit: 3746284c233d5cf5f456400e61cd4a46a69c6e8c All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark

1 0

Re: [PATCH] ASoC: SOF: amd: fix for firmware reload failure after playback
by Mark Brown 28 Sep '23

28 Sep '23

On Wed, 27 Sep 2023 12:44:10 +0530, Vijendar Mukunda wrote: > Setting ACP ACLK as clock source when ACP enters D0 state causing > firmware load failure as mentioned in below scenario. > > - Load snd_sof_amd_rembrandt > - Play or Record audio > - Stop audio > - Unload snd_sof_amd_rembrandt > - Reload snd_sof_amd_rembrandt > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: amd: fix for firmware reload failure after playback commit: 7e1fe5d9e7eae67e218f878195d1d348d01f9af7 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark

1 0

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks
by Kai Vehmanen 12 Sep '23

12 Sep '23

Hi, On Wed, 30 Aug 2023, Maarten Lankhorst wrote: > With the upcoming changes for i915/Xe driver relying on the > -EPROBE_DEFER mechanism, we need to have a first pass of the probe > which cannot be pushed to a workqueue. Introduce 2 new optional > callbacks. [...] > diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c > index 30db685cc5f4b..54c384a5d6140 100644 > --- a/sound/soc/sof/core.c > +++ b/sound/soc/sof/core.c > @@ -327,8 +327,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) > dsp_err: > snd_sof_remove(sdev); > probe_err: > - sof_ops_free(sdev); > - this seems a bit out-of-place in this patch. It seems a valid change, but not really related to this patch, right? We seem to have a related fix waiting to be sent to alsa-devel, by Peter: "ASoC: SOF: core: Only call sof_ops_free() on remove if the probe wa" https://github.com/thesofproject/linux/pull/4515 ... not yet in Mark's tree. Otherwise patch looks good to me. Br, Kai

3 7

[PATCH AUTOSEL 6.1 11/22] ASoC: SOF: topology: simplify code to prevent static analysis warnings
by Sasha Levin 08 Sep '23

08 Sep '23

From: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> [ Upstream commit 55cb3dc271d81f1982c949a2ac483a6daf613b92 ] make KCFLAGS='-fanalyzer' sound/soc/sof/intel/ reports a possible NULL pointer dereference. sound/soc/sof/topology.c:1136:21: error: dereference of NULL ‘w’ [CWE-476] [-Werror=analyzer-null-dereference] 1136 | strcmp(w->sname, rtd->dai_link->stream_name)) The code is rather confusing and can be simplified to make static analysis happy. No functionality change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> Reviewed-by: Rander Wang <rander.wang(a)intel.com> Reviewed-by: Daniel Baluta <daniel.baluta(a)nxp.com> Reviewed-by: Yaochun Hung <yc.hung(a)mediatek.com> Link: https://lore.kernel.org/r/20230731213748.440285-4-pierre-louis.bossart@linu… Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/topology.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index 872e44408298f..e7305ce57ea1f 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -1086,16 +1086,17 @@ static void sof_disconnect_dai_widget(struct snd_soc_component *scomp, { struct snd_soc_card *card = scomp->card; struct snd_soc_pcm_runtime *rtd; + const char *sname = w->sname; struct snd_soc_dai *cpu_dai; int i; - if (!w->sname) + if (!sname) return; list_for_each_entry(rtd, &card->rtd_list, list) { /* does stream match DAI link ? */ if (!rtd->dai_link->stream_name || - strcmp(w->sname, rtd->dai_link->stream_name)) + strcmp(sname, rtd->dai_link->stream_name)) continue; switch (w->id) { -- 2.40.1

1 0

[PATCH AUTOSEL 6.4 30/31] ASoC: SOF: amd: clear panic mask status when panic occurs
by Sasha Levin 08 Sep '23

08 Sep '23

From: Vijendar Mukunda <Vijendar.Mukunda(a)amd.com> [ Upstream commit 3d02e1c439b4140215b624d423aa3c7554b17a5a ] Due to scratch memory persistence, Once the DSP panic is reported, need to clear the panic mask after handling DSP panic. Otherwise, It results in DSP panic on next reboot. Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda(a)amd.com> Link: https://lore.kernel.org/r/20230823073340.2829821-6-Vijendar.Mukunda@amd.com Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/amd/acp-ipc.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sound/soc/sof/amd/acp-ipc.c b/sound/soc/sof/amd/acp-ipc.c index 749e856dc6011..6951c67bc599f 100644 --- a/sound/soc/sof/amd/acp-ipc.c +++ b/sound/soc/sof/amd/acp-ipc.c @@ -161,6 +161,8 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context) if ((status & SOF_IPC_PANIC_MAGIC_MASK) == SOF_IPC_PANIC_MAGIC) { snd_sof_dsp_panic(sdev, sdev->dsp_box.offset + sizeof(status), true); + status = 0; + acp_mailbox_write(sdev, sdev->dsp_box.offset, &status, sizeof(status)); return IRQ_HANDLED; } snd_sof_ipc_msgs_rx(sdev); @@ -190,6 +192,8 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context) acp_mailbox_read(sdev, sdev->debug_box.offset, &status, sizeof(u32)); if ((status & SOF_IPC_PANIC_MAGIC_MASK) == SOF_IPC_PANIC_MAGIC) { snd_sof_dsp_panic(sdev, sdev->dsp_oops_offset, true); + status = 0; + acp_mailbox_write(sdev, sdev->debug_box.offset, &status, sizeof(status)); return IRQ_HANDLED; } -- 2.40.1

1 0

[PATCH AUTOSEL 6.4 15/31] ASoC: SOF: topology: simplify code to prevent static analysis warnings
by Sasha Levin 08 Sep '23

08 Sep '23

From: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> [ Upstream commit 55cb3dc271d81f1982c949a2ac483a6daf613b92 ] make KCFLAGS='-fanalyzer' sound/soc/sof/intel/ reports a possible NULL pointer dereference. sound/soc/sof/topology.c:1136:21: error: dereference of NULL ‘w’ [CWE-476] [-Werror=analyzer-null-dereference] 1136 | strcmp(w->sname, rtd->dai_link->stream_name)) The code is rather confusing and can be simplified to make static analysis happy. No functionality change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> Reviewed-by: Rander Wang <rander.wang(a)intel.com> Reviewed-by: Daniel Baluta <daniel.baluta(a)nxp.com> Reviewed-by: Yaochun Hung <yc.hung(a)mediatek.com> Link: https://lore.kernel.org/r/20230731213748.440285-4-pierre-louis.bossart@linu… Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/topology.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index f160dc454b448..8e6f6148f0a07 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -1117,10 +1117,11 @@ static void sof_disconnect_dai_widget(struct snd_soc_component *scomp, { struct snd_soc_card *card = scomp->card; struct snd_soc_pcm_runtime *rtd; + const char *sname = w->sname; struct snd_soc_dai *cpu_dai; int i, stream; - if (!w->sname) + if (!sname) return; if (w->id == snd_soc_dapm_dai_out) @@ -1133,7 +1134,7 @@ static void sof_disconnect_dai_widget(struct snd_soc_component *scomp, list_for_each_entry(rtd, &card->rtd_list, list) { /* does stream match DAI link ? */ if (!rtd->dai_link->stream_name || - strcmp(w->sname, rtd->dai_link->stream_name)) + strcmp(sname, rtd->dai_link->stream_name)) continue; for_each_rtd_cpu_dais(rtd, i, cpu_dai) -- 2.40.1

1 0