Re: [PATCH v2 8/9] ASoC: SOF: Intel: Remove deferred probe for SOF

On 7/19/23 18:41, Maarten Lankhorst wrote:

This was only used to allow modprobing i915, by converting to the -EPROBE_DEFER mechanism, it can be completely removed, and is in fact counterproductive since -EPROBE_DEFER otherwise won't be handled correctly.

I personally remember only that the request_module("i915") was the main motivation for the use of the workqueue, but when it comes to the HDaudio codec management we don't even know what we don't know.

I am a bit worried that the snd-hda-intel driver keeps the workqueue for HDaudio codec initialization, and this patch removes the workqueue completely for SOF. That doesn't seem right. Either both drivers need a workqueue or none need a workqueue.

Maybe what we need is to move the i915/xe initialization out of the workqueue, and see in a second pass if that workqueue can be safely removed from the SOF driver?

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Matthew Auld matthew.auld@intel.com Acked-by: Mark Brown broonie@kernel.org

---

sound/soc/sof/Kconfig | 19 ----------------- sound/soc/sof/core.c | 38 ++------------------------------- sound/soc/sof/intel/Kconfig | 1 - sound/soc/sof/intel/hda-codec.c | 2 +- sound/soc/sof/intel/hda.c | 32 ++++++++++++++++----------- sound/soc/sof/sof-pci-dev.c | 3 +-- sound/soc/sof/sof-priv.h | 5 ----- 7 files changed, 23 insertions(+), 77 deletions(-)

diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig index 80361139a49ad..8ee39e5558062 100644 --- a/sound/soc/sof/Kconfig +++ b/sound/soc/sof/Kconfig @@ -82,17 +82,6 @@ config SND_SOC_SOF_DEVELOPER_SUPPORT

if SND_SOC_SOF_DEVELOPER_SUPPORT

-config SND_SOC_SOF_FORCE_PROBE_WORKQUEUE

• bool "SOF force probe workqueue"
• select SND_SOC_SOF_PROBE_WORK_QUEUE
• help

•  This option forces the use of a probe workqueue, which is only used
  

•  when HDaudio is enabled due to module dependencies. Forcing this
  

•  option is intended for debug only, but this should not add any
  

•  functional issues in nominal cases.
  

•  Say Y if you are involved in SOF development and need this option.
  

•  If not, select N.
  

config SND_SOC_SOF_NOCODEC tristate

@@ -271,14 +260,6 @@ config SND_SOC_SOF module dependencies but since the module or built-in type is decided at the top level it doesn't matter.

-config SND_SOC_SOF_PROBE_WORK_QUEUE

• bool
• help

•  This option is not user-selectable but automagically handled by
  

•  'select' statements at a higher level.
  

•  When selected, the probe is handled in two steps, for example to
  

•  avoid lockdeps if request_module is used in the probe.
  

# Supported IPC versions config SND_SOC_SOF_IPC3 bool diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 30db685cc5f4b..cdf86dc4a8a87 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -191,7 +191,8 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) /* probe the DSP hardware */ ret = snd_sof_probe(sdev); if (ret < 0) {

• dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret);
  

• if (ret != -EPROBE_DEFER)
  

• 	dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret);
  

  goto probe_err; }

@@ -309,8 +310,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) if (plat_data->sof_probe_complete) plat_data->sof_probe_complete(sdev->dev);

• sdev->probe_completed = true;
• return 0;

sof_machine_err: @@ -336,19 +335,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) return ret; }

-static void sof_probe_work(struct work_struct *work) -{

• struct snd_sof_dev *sdev =

• container_of(work, struct snd_sof_dev, probe_work);
  

• int ret;
• ret = sof_probe_continue(sdev);
• if (ret < 0) {

• /* errors cannot be propagated, log */
  

• dev_err(sdev->dev, "error: %s failed err: %d\n", __func__, ret);
  

• }

-}

int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data) { struct snd_sof_dev *sdev; @@ -436,33 +422,16 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)

sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED);

• if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) {

• INIT_WORK(&sdev->probe_work, sof_probe_work);
  

• schedule_work(&sdev->probe_work);
  

• return 0;
  

• }
• return sof_probe_continue(sdev);

} EXPORT_SYMBOL(snd_sof_device_probe);

-bool snd_sof_device_probe_completed(struct device *dev) -{

• struct snd_sof_dev *sdev = dev_get_drvdata(dev);
• return sdev->probe_completed;

-} -EXPORT_SYMBOL(snd_sof_device_probe_completed);

int snd_sof_device_remove(struct device *dev) { struct snd_sof_dev *sdev = dev_get_drvdata(dev); struct snd_sof_pdata *pdata = sdev->pdata; int ret;

• if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))

• cancel_work_sync(&sdev->probe_work);
  

• /*
  • Unregister any registered client device first before IPC and debugfs
  • to allow client drivers to be removed cleanly

@@ -501,9 +470,6 @@ int snd_sof_device_shutdown(struct device *dev) { struct snd_sof_dev *sdev = dev_get_drvdata(dev);

• if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))

• cancel_work_sync(&sdev->probe_work);
  

• if (sdev->fw_state == SOF_FW_BOOT_COMPLETE) { sof_fw_trace_free(sdev); return snd_sof_shutdown(sdev);

diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index 69c1a370d3b61..d9e87a91670a3 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -293,7 +293,6 @@ config SND_SOC_SOF_HDA_LINK config SND_SOC_SOF_HDA_AUDIO_CODEC bool "SOF support for HDAudio codecs" depends on SND_SOC_SOF_HDA_LINK

• select SND_SOC_SOF_PROBE_WORK_QUEUE help This adds support for HDAudio codecs with Sound Open Firmware for Intel(R) platforms.

diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index f1fd5b44aaac9..344b61576c0e3 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) return 0;

/* i915 exposes a HDA codec for HDMI audio */

• ret = snd_hdac_i915_init(bus, true);

• ret = snd_hdac_i915_init(bus, false); if (ret < 0) return ret;

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 64bebe1a72bbc..a8b7a68142c05 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -801,8 +801,11 @@ 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");
  

• if (ret < 0) {

• if (ret != -EPROBE_DEFER)
  

• 	dev_warn(sdev->dev, "init of i915 and HDMI codec failed: %i\n", ret);
  

• return ret;
  

• }

  /* get controller capabilities */ ret = hda_dsp_ctrl_get_caps(sdev);

@@ -1115,14 +1118,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) sdev->pdata->hw_pdata = hdev; hdev->desc = chip;

• hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec",

• 				       PLATFORM_DEVID_NONE,
  

• 				       NULL, 0);
  

• if (IS_ERR(hdev->dmic_dev)) {

• dev_err(sdev->dev, "error: failed to create DMIC device\n");
  

• return PTR_ERR(hdev->dmic_dev);
  

• }
• /*
  • use position update IPC if either it is forced
  • or we don't have other choice

@@ -1142,6 +1137,15 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) if (ret < 0) goto hdac_bus_unmap;

• hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec",

• 				       PLATFORM_DEVID_NONE,
  

• 				       NULL, 0);
  

• if (IS_ERR(hdev->dmic_dev)) {

• dev_err(sdev->dev, "error: failed to create DMIC device\n");
  

• ret = PTR_ERR(hdev->dmic_dev);
  

• goto hdac_exit;
  

• }

I am not following why we have to move dmic-related code, can we try to move this in a separate patch?

if (sdev->dspless_mode_selected) goto skip_dsp_setup;

@@ -1150,7 +1154,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) if (!sdev->bar[HDA_DSP_BAR]) { dev_err(sdev->dev, "error: ioremap error\n"); ret = -ENXIO;

• goto hdac_bus_unmap;
  

• goto platform_unreg;
  

  }

  sdev->mmio_bar = HDA_DSP_BAR;

@@ -1248,10 +1252,12 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) /* dsp_unmap: not currently used */ if (!sdev->dspless_mode_selected) iounmap(sdev->bar[HDA_DSP_BAR]); -hdac_bus_unmap: +platform_unreg: platform_device_unregister(hdev->dmic_dev);

• iounmap(bus->remap_addr);

+hdac_exit: hda_codec_i915_exit(sdev); +hdac_bus_unmap:

• iounmap(bus->remap_addr);

err: return ret; } diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c index f5ece43d0ec24..0fa424613082e 100644 --- a/sound/soc/sof/sof-pci-dev.c +++ b/sound/soc/sof/sof-pci-dev.c @@ -339,8 +339,7 @@ void sof_pci_remove(struct pci_dev *pci) snd_sof_device_remove(&pci->dev);

/* follow recommendation in pci-driver.c to increment usage counter */

• if (snd_sof_device_probe_completed(&pci->dev) &&

•    !(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME))
  

• if (!(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME)) pm_runtime_get_noresume(&pci->dev);

  /* release pci regions and disable device */

diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h index d4f6702e93dcb..71db636cfdccc 100644 --- a/sound/soc/sof/sof-priv.h +++ b/sound/soc/sof/sof-priv.h @@ -564,10 +564,6 @@ struct snd_sof_dev { enum sof_fw_state fw_state; bool first_boot;

• /* work queue in case the probe is implemented in two steps */
• struct work_struct probe_work;
• bool probe_completed;
• /* DSP HW differentiation */ struct snd_sof_pdata *pdata;

@@ -675,7 +671,6 @@ struct snd_sof_dev { int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data); int snd_sof_device_remove(struct device *dev); int snd_sof_device_shutdown(struct device *dev); -bool snd_sof_device_probe_completed(struct device *dev);

int snd_sof_runtime_suspend(struct device *dev); int snd_sof_runtime_resume(struct device *dev);