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.

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;
+	}
+
 	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);
-- 
2.39.2

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

Hey,

On 2023-07-24 13:32, Pierre-Louis Bossart wrote:
> 
> 
> 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?
> 
As I mentioned in some of the other sound driver conversions. I believe 
it's possible to completely kill off most workqueues.

However, I don´t have the hardware or knowledge to test it. I saw that 
the SOF had the non-workqueue path already, so it felt less risky to 
simply convert it to always use that path.

avs/skylake drivers should be easy to convert too. This is why I left 
the comment: "Removing the workqueue would simplify init even further, 
but is left as exercise for the reviewer."

HDA-intel has this retry-probe logic used on AMD's,
which makes me more hesitant to convert it.

I wanted to tackle one problem at a time, I believe workqueue removal 
can be done by anyone.

Cheers,
~Maarten

On Tue, 25 Jul 2023 12:29:07 +0200,
Maarten Lankhorst wrote:
> 
> Hey,
> 
> On 2023-07-24 13:32, Pierre-Louis Bossart wrote:
> > 
> > 
> > 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?
> > 
> As I mentioned in some of the other sound driver conversions. I
> believe it's possible to completely kill off most workqueues.
> 
> However, I don´t have the hardware or knowledge to test it. I saw
> that the SOF had the non-workqueue path already, so it felt less risky
> to simply convert it to always use that path.
> 
> avs/skylake drivers should be easy to convert too. This is why I left
> the comment: "Removing the workqueue would simplify init even further,
> but is left as exercise for the reviewer."
> 
> HDA-intel has this retry-probe logic used on AMD's,
> which makes me more hesitant to convert it.

Yes, HDA-Intel requires either a workqueue or async firmware-loader
callback because there is some codec module-autoload mechanism that
may happen during the probe phase.  It's not only on AMD, but it's
required in general for all codecs.


Takashi

On 19/07/2023 19: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.
> 
> 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);

While at it, can you drop thee "error:" prefix from the print?

>  		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");

From here also, let's not add them back.

> +		ret = PTR_ERR(hdev->dmic_dev);
> +		goto hdac_exit;
> +	}
> +
>  	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);

-- 
Péter