[PATCH v4 00/11] sound: Use -EPROBE_DEFER instead of i915 module loading.

[PATCH v4 00/11] sound: Use -EPROBE_DEFER instead of i915 module loading.

[Maarten Lankhorst]

Explicitly loading i915 becomes a problem when upstreaming the new intel driver
for Tiger Lake and higher graphics (xe). By loading i915, it doesn't wait for
driver load of xe, and will fail completely before it loads.

-EPROBE_DEFER has to be returned before any device is created in probe(),
otherwise the removal of the device will cause EPROBE_DEFER to try again
in an infinite loop.

The conversion is done in gradual steps. First I add an argument to
snd_hdac_i915_init to allow for -EPROBE_DEFER so I can convert each driver
separately. Then I convert each driver to move snd_hdac_i915_init out of the
workqueue. Finally I drop the ability to choose modprobe behavior after the
last user is converted.

Compared to previous version, I picked up Pierre's changes to add a
probe_no_wq, added remove_no_wq myself and then fixed the SOF changes to
use it.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Cc: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Cc: Bard Liao <yung-chuan.liao@linux.intel.com>
Cc: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Daniel Baluta <daniel.baluta@nxp.com>
Cc: alsa-devel@alsa-project.org
Cc: linux-kernel@vger.kernel.org
Cc: sound-open-firmware@alsa-project.org

Maarten Lankhorst (9):
  ALSA: hda/intel: Fix error handling in azx_probe()
  ALSA: hda/i915: Allow override of gpu binding.
  ALSA: hda/i915: Add an allow_modprobe argument to snd_hdac_i915_init
  ALSA: hda/i915: Allow xe as match for i915_component_master_match
  ASoC: Intel: avs: Move snd_hdac_i915_init to before probe_work.
  ASoC: Intel: Skylake: Move snd_hdac_i915_init to before probe_work.
  ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.
  ASoC: SOF: Intel: Move binding to display driver outside of deferred
    probe
  ALSA: hda/i915: Remove extra argument from snd_hdac_i915_init

Pierre-Louis Bossart (2):
  ASoC: SOF: core: add 'no_wq' probe and remove callbacks
  ASoC: SOF: Intel: hda: start splitting the probe

 sound/hda/hdac_i915.c                | 24 ++++++-----
 sound/pci/hda/hda_intel.c            | 60 ++++++++++++++--------------
 sound/soc/intel/avs/core.c           | 13 ++++--
 sound/soc/intel/skylake/skl.c        | 31 +++++---------
 sound/soc/sof/core.c                 | 11 ++++-
 sound/soc/sof/intel/hda-common-ops.c |  2 +
 sound/soc/sof/intel/hda.c            | 31 ++++++++------
 sound/soc/sof/intel/hda.h            |  2 +
 sound/soc/sof/ops.h                  | 16 ++++++++
 sound/soc/sof/sof-priv.h             |  2 +
 10 files changed, 112 insertions(+), 80 deletions(-)

-- 
2.39.2

[PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Maarten Lankhorst]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The existing DSP probe may be handled in a workqueue to allow for
extra time, typically for the i915 request_module and HDAudio codec
handling.

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.

Note that instead of probe_no_wq/probe we could have use a more
self-explanatory naming such as probe/probe_wq_allowed, but that would
have been a very intrusive change.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 sound/soc/sof/core.c     | 11 +++++++++--
 sound/soc/sof/ops.h      | 16 ++++++++++++++++
 sound/soc/sof/sof-priv.h |  2 ++
 3 files changed, 27 insertions(+), 2 deletions(-)

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);
-
 	/* all resources freed, update state to match */
 	sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED);
 	sdev->first_boot = true;
@@ -436,6 +434,14 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
 
 	sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED);
 
+	/*
+	 * first pass of probe which isn't allowed to run in a work-queue,
+	 * typically to rely on -EPROBE_DEFER dependencies
+	 */
+	ret = snd_sof_probe_no_wq(sdev);
+	if (ret < 0)
+		return ret;
+
 	if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) {
 		INIT_WORK(&sdev->probe_work, sof_probe_work);
 		schedule_work(&sdev->probe_work);
@@ -487,6 +493,7 @@ int snd_sof_device_remove(struct device *dev)
 		snd_sof_free_debug(sdev);
 		snd_sof_remove(sdev);
 	}
+	snd_sof_remove_no_wq(sdev);
 
 	sof_ops_free(sdev);
 
diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h
index 9ab7b9be765bc..2a03b152e9313 100644
--- a/sound/soc/sof/ops.h
+++ b/sound/soc/sof/ops.h
@@ -38,6 +38,14 @@ static inline void sof_ops_free(struct snd_sof_dev *sdev)
 /* Mandatory operations are verified during probing */
 
 /* init */
+static inline int snd_sof_probe_no_wq(struct snd_sof_dev *sdev)
+{
+	if (sof_ops(sdev)->probe_no_wq)
+		return sof_ops(sdev)->probe_no_wq(sdev);
+
+	return 0;
+}
+
 static inline int snd_sof_probe(struct snd_sof_dev *sdev)
 {
 	return sof_ops(sdev)->probe(sdev);
@@ -51,6 +59,14 @@ static inline int snd_sof_remove(struct snd_sof_dev *sdev)
 	return 0;
 }
 
+static inline int snd_sof_remove_no_wq(struct snd_sof_dev *sdev)
+{
+	if (sof_ops(sdev)->remove_no_wq)
+		return sof_ops(sdev)->remove_no_wq(sdev);
+
+	return 0;
+}
+
 static inline int snd_sof_shutdown(struct snd_sof_dev *sdev)
 {
 	if (sof_ops(sdev)->shutdown)
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index d4f6702e93dcb..addaef282ee92 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -165,6 +165,8 @@ struct sof_firmware {
 struct snd_sof_dsp_ops {
 
 	/* probe/remove/shutdown */
+	int (*probe_no_wq)(struct snd_sof_dev *sof_dev); /* optional */
+	int (*remove_no_wq)(struct snd_sof_dev *sof_dev); /* optional */
 	int (*probe)(struct snd_sof_dev *sof_dev); /* mandatory */
 	int (*remove)(struct snd_sof_dev *sof_dev); /* optional */
 	int (*shutdown)(struct snd_sof_dev *sof_dev); /* optional */
-- 
2.39.2

[PATCH v4 02/11] ASoC: SOF: Intel: hda: start splitting the probe

[Maarten Lankhorst]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

This patch moves the initial parts of the probe to the probe_no_wq()
callback, which provides a much faster decision on whether the SOF
driver shall deal with a specific platform or yield to other Intel
drivers.

This is a limited functionality change, the bigger change is to move
the i915/Xe initialization to the probe_no_wq().

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 sound/soc/sof/intel/hda-common-ops.c |  1 +
 sound/soc/sof/intel/hda.c            | 16 +++++++++++++---
 sound/soc/sof/intel/hda.h            |  1 +
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/sound/soc/sof/intel/hda-common-ops.c b/sound/soc/sof/intel/hda-common-ops.c
index 8e1cd0babd32c..803b5e9087782 100644
--- a/sound/soc/sof/intel/hda-common-ops.c
+++ b/sound/soc/sof/intel/hda-common-ops.c
@@ -16,6 +16,7 @@
 
 struct snd_sof_dsp_ops sof_hda_common_ops = {
 	/* probe/remove/shutdown */
+	.probe_no_wq	= hda_dsp_probe_no_wq,
 	.probe		= hda_dsp_probe,
 	.remove		= hda_dsp_remove,
 
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index 15e6779efaa3b..e918b5dadfa02 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -1118,11 +1118,10 @@ static irqreturn_t hda_dsp_interrupt_thread(int irq, void *context)
 	return IRQ_HANDLED;
 }
 
-int hda_dsp_probe(struct snd_sof_dev *sdev)
+int hda_dsp_probe_no_wq(struct snd_sof_dev *sdev)
 {
 	struct pci_dev *pci = to_pci_dev(sdev->dev);
 	struct sof_intel_hda_dev *hdev;
-	struct hdac_bus *bus;
 	const struct sof_intel_dsp_desc *chip;
 	int ret = 0;
 
@@ -1162,6 +1161,17 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
 	sdev->pdata->hw_pdata = hdev;
 	hdev->desc = chip;
 
+err:
+	return ret;
+}
+
+int hda_dsp_probe(struct snd_sof_dev *sdev)
+{
+	struct pci_dev *pci = to_pci_dev(sdev->dev);
+	struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata;
+	struct hdac_bus *bus;
+	int ret = 0;
+
 	hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec",
 						       PLATFORM_DEVID_NONE,
 						       NULL, 0);
@@ -1299,7 +1309,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
 	platform_device_unregister(hdev->dmic_dev);
 	iounmap(bus->remap_addr);
 	hda_codec_i915_exit(sdev);
-err:
+
 	return ret;
 }
 
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 5c517ec57d4a2..89b8c239e9a5e 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -573,6 +573,7 @@ struct sof_intel_hda_stream {
 /*
  * DSP Core services.
  */
+int hda_dsp_probe_no_wq(struct snd_sof_dev *sdev);
 int hda_dsp_probe(struct snd_sof_dev *sdev);
 int hda_dsp_remove(struct snd_sof_dev *sdev);
 int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask);
-- 
2.39.2

[PATCH v4 03/11] ALSA: hda/intel: Fix error handling in azx_probe()

[Maarten Lankhorst]

Add missing pci_set_drv to NULL call on error.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 sound/pci/hda/hda_intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 765d95e798617..bf6210506a2da 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2176,6 +2176,7 @@ static int azx_probe(struct pci_dev *pci,
 	return 0;
 
 out_free:
+	pci_set_drvdata(pci, NULL);
 	snd_card_free(card);
 	return err;
 }
-- 
2.39.2

[PATCH v4 04/11] ALSA: hda/i915: Allow override of gpu binding.

[Maarten Lankhorst]

Selecting CONFIG_DRM selects CONFIG_VIDEO_NOMODESET, which exports
video_firmware_drivers_only(). This can be used as a first
approximation on whether i915 will be available. It's safe to use as
this is only built when CONFIG_SND_HDA_I915 is selected by CONFIG_I915.

It's not completely fool proof, as you can boot with "nomodeset
i915.modeset=1" to make i915 load regardless, or use
"i915.force_probe=!*" to never load i915, but the common case of
booting with nomodeset to disable all GPU drivers this will work as
intended.

Because of this, we add an extra module parameter,
snd_hda_core.gpu_bind that can be used to signal users intent.
-1 follows nomodeset, 0 disables binding, 1 forces wait/-EPROBE_DEFER
on binding.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/hda/hdac_i915.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index b428537f284c7..a4a712c795c3d 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -10,6 +10,12 @@
 #include <sound/hdaudio.h>
 #include <sound/hda_i915.h>
 #include <sound/hda_register.h>
+#include <video/nomodeset.h>
+
+static int gpu_bind = -1;
+module_param(gpu_bind, int, 0644);
+MODULE_PARM_DESC(gpu_bind, "Whether to bind sound component to GPU "
+			   "(1=always, 0=never, -1=on nomodeset(default))");
 
 /**
  * snd_hdac_i915_set_bclk - Reprogram BCLK for HSW/BDW
@@ -122,6 +128,9 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
 {
 	struct pci_dev *display_dev = NULL;
 
+	if (!gpu_bind || (gpu_bind < 0 && video_firmware_drivers_only()))
+		return false;
+
 	for_each_pci_dev(display_dev) {
 		if (display_dev->vendor == PCI_VENDOR_ID_INTEL &&
 		    (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
-- 
2.39.2

[PATCH v4 05/11] ALSA: hda/i915: Add an allow_modprobe argument to snd_hdac_i915_init

[Maarten Lankhorst]

Xe is a new GPU driver that re-uses the display (and sound) code from
i915. It's no longer possible to load i915, as the GPU can be driven
by the xe driver instead.

The new behavior will return -EPROBE_DEFER, and wait for a compatible
driver to be loaded instead of modprobing i915.

Converting all drivers at the same time is a lot of work, instead we
will convert each user one by one.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
Changes since v1:
- Use dev_err_probe to set a probe reason for debugfs' deferred_devices.
---
 include/sound/hda_i915.h        | 4 ++--
 sound/hda/hdac_i915.c           | 8 ++++----
 sound/pci/hda/hda_intel.c       | 2 +-
 sound/soc/intel/avs/core.c      | 2 +-
 sound/soc/intel/skylake/skl.c   | 2 +-
 sound/soc/sof/intel/hda-codec.c | 2 +-
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h
index 6b79614a893b9..f91bd66360865 100644
--- a/include/sound/hda_i915.h
+++ b/include/sound/hda_i915.h
@@ -9,12 +9,12 @@
 
 #ifdef CONFIG_SND_HDA_I915
 void snd_hdac_i915_set_bclk(struct hdac_bus *bus);
-int snd_hdac_i915_init(struct hdac_bus *bus);
+int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe);
 #else
 static inline void snd_hdac_i915_set_bclk(struct hdac_bus *bus)
 {
 }
-static inline int snd_hdac_i915_init(struct hdac_bus *bus)
+static inline int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe)
 {
 	return -ENODEV;
 }
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index a4a712c795c3d..ffa35d7a367c0 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -155,7 +155,7 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
  *
  * Returns zero for success or a negative error code.
  */
-int snd_hdac_i915_init(struct hdac_bus *bus)
+int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe)
 {
 	struct drm_audio_component *acomp;
 	int err;
@@ -171,7 +171,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
 	acomp = bus->audio_component;
 	if (!acomp)
 		return -ENODEV;
-	if (!acomp->ops) {
+	if (allow_modprobe && !acomp->ops) {
 		if (!IS_ENABLED(CONFIG_MODULES) ||
 		    !request_module("i915")) {
 			/* 60s timeout */
@@ -180,9 +180,9 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
 		}
 	}
 	if (!acomp->ops) {
-		dev_info(bus->dev, "couldn't bind with audio component\n");
+		int err = allow_modprobe ? -ENODEV : -EPROBE_DEFER;
 		snd_hdac_acomp_exit(bus);
-		return -ENODEV;
+		return dev_err_probe(bus->dev, err, "couldn't bind with audio component\n");
 	}
 	return 0;
 }
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index bf6210506a2da..85fa15bbd8513 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2265,7 +2265,7 @@ static int azx_probe_continue(struct azx *chip)
 
 	/* bind with i915 if needed */
 	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
-		err = snd_hdac_i915_init(bus);
+		err = snd_hdac_i915_init(bus, true);
 		if (err < 0) {
 			/* if the controller is bound only with HDMI/DP
 			 * (for HSW and BDW), we need to abort the probe;
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 859b217fc761b..bbb40339c75f4 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -191,7 +191,7 @@ static void avs_hda_probe_work(struct work_struct *work)
 
 	pm_runtime_set_active(bus->dev); /* clear runtime_error flag */
 
-	ret = snd_hdac_i915_init(bus);
+	ret = snd_hdac_i915_init(bus, true);
 	if (ret < 0)
 		dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
 
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 77408a981b977..4f7acb4f6680b 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -791,7 +791,7 @@ static int skl_i915_init(struct hdac_bus *bus)
 	 * The HDMI codec is in GPU so we need to ensure that it is powered
 	 * up and ready for probe
 	 */
-	err = snd_hdac_i915_init(bus);
+	err = snd_hdac_i915_init(bus, true);
 	if (err < 0)
 		return err;
 
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 8a5e99a898ecb..f1fd5b44aaac9 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);
+	ret = snd_hdac_i915_init(bus, true);
 	if (ret < 0)
 		return ret;
 
-- 
2.39.2

[PATCH v4 06/11] ALSA: hda/i915: Allow xe as match for i915_component_master_match

[Maarten Lankhorst]

Xe is a new driver for intel GPU's that shares the sound related code
with i915.

The modprobe mechanism is being replaced by the -EPROBE_DEFER mechanism,
so we don't need to add a modprobe xe call. Adding this would have
required a telepathy module to correctly guess whether to load i915 or
xe.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/hda/hdac_i915.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index ffa35d7a367c0..0765e5350e7ba 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -115,7 +115,8 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
 	hdac_pci = to_pci_dev(bus->dev);
 	i915_pci = to_pci_dev(dev);
 
-	if (!strcmp(dev->driver->name, "i915") &&
+	if ((!strcmp(dev->driver->name, "i915") ||
+		 !strcmp(dev->driver->name, "xe")) &&
 	    subcomponent == I915_COMPONENT_AUDIO &&
 	    connectivity_check(i915_pci, hdac_pci))
 		return 1;
-- 
2.39.2

[PATCH v4 07/11] ASoC: Intel: avs: Move snd_hdac_i915_init to before probe_work.

[Maarten Lankhorst]

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@linux.intel.com>
Acked-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
Changes since v1:
- Rename error label.
---
 sound/soc/intel/avs/core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index bbb40339c75f4..8a20639582487 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -191,10 +191,6 @@ static void avs_hda_probe_work(struct work_struct *work)
 
 	pm_runtime_set_active(bus->dev); /* clear runtime_error flag */
 
-	ret = snd_hdac_i915_init(bus, true);
-	if (ret < 0)
-		dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
-
 	snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
 	avs_hdac_bus_init_chip(bus, true);
 	avs_hdac_bus_probe_codecs(bus);
@@ -465,10 +461,19 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	pci_set_drvdata(pci, bus);
 	device_disable_async_suspend(dev);
 
+	ret = snd_hdac_i915_init(bus, false);
+	if (ret == -EPROBE_DEFER)
+		goto err_i915_init;
+	else if (ret < 0)
+		dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
+
 	schedule_work(&adev->probe_work);
 
 	return 0;
 
+err_i915_init:
+	pci_clear_master(pci);
+	pci_set_drvdata(pci, NULL);
 err_acquire_irq:
 	snd_hdac_bus_free_stream_pages(bus);
 	snd_hdac_ext_stream_free_all(bus);
-- 
2.39.2

[PATCH v4 08/11] ASoC: Intel: Skylake: Move snd_hdac_i915_init to before probe_work.

[Maarten Lankhorst]

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@linux.intel.com>
Acked-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 sound/soc/intel/skylake/skl.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 4f7acb4f6680b..24bdbe2a53bec 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -783,23 +783,6 @@ static void skl_codec_create(struct hdac_bus *bus)
 	}
 }
 
-static int skl_i915_init(struct hdac_bus *bus)
-{
-	int err;
-
-	/*
-	 * The HDMI codec is in GPU so we need to ensure that it is powered
-	 * up and ready for probe
-	 */
-	err = snd_hdac_i915_init(bus, true);
-	if (err < 0)
-		return err;
-
-	snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
-
-	return 0;
-}
-
 static void skl_probe_work(struct work_struct *work)
 {
 	struct skl_dev *skl = container_of(work, struct skl_dev, probe_work);
@@ -807,11 +790,8 @@ static void skl_probe_work(struct work_struct *work)
 	struct hdac_ext_link *hlink;
 	int err;
 
-	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
-		err = skl_i915_init(bus);
-		if (err < 0)
-			return;
-	}
+	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
+		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
 
 	skl_init_pci(skl);
 	skl_dum_set(bus);
@@ -1075,10 +1055,17 @@ static int skl_probe(struct pci_dev *pci,
 		goto out_dsp_free;
 	}
 
+	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
+		err = snd_hdac_i915_init(bus, false);
+		if (err < 0)
+			goto out_dmic_unregister;
+	}
 	schedule_work(&skl->probe_work);
 
 	return 0;
 
+out_dmic_unregister:
+	skl_dmic_device_unregister(skl);
 out_dsp_free:
 	skl_free_dsp(skl);
 out_clk_free:
-- 
2.39.2

[PATCH v4 09/11] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Maarten Lankhorst]

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.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

---
Changes since v1:
- Use dev_err_probe()
- Don't move probed_devs bitmap unnecessarily. (tiwai)
- Move snd_hdac_i915_init slightly upward, to ensure
  it's always initialised before vga-switcheroo is called.
---
 sound/pci/hda/hda_intel.c | 59 ++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 85fa15bbd8513..d529ef21f033a 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2135,6 +2135,36 @@ static int azx_probe(struct pci_dev *pci,
 
 	pci_set_drvdata(pci, card);
 
+#ifdef CONFIG_SND_HDA_I915
+	/* bind with i915 if needed */
+	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
+		err = snd_hdac_i915_init(azx_bus(chip), false);
+		if (err < 0) {
+			/* if the controller is bound only with HDMI/DP
+			 * (for HSW and BDW), we need to abort the probe;
+			 * for other chips, still continue probing as other
+			 * codecs can be on the same link.
+			 */
+			if (HDA_CONTROLLER_IN_GPU(pci)) {
+				dev_err_probe(card->dev, err,
+					     "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n");
+
+				goto out_free;
+			} else {
+				/* don't bother any longer */
+				chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT;
+			}
+		}
+
+		/* HSW/BDW controllers need this power */
+		if (HDA_CONTROLLER_IN_GPU(pci))
+			hda->need_i915_power = true;
+	}
+#else
+	if (HDA_CONTROLLER_IN_GPU(pci))
+		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n");
+#endif
+
 	err = register_vga_switcheroo(chip);
 	if (err < 0) {
 		dev_err(card->dev, "Error registering vga_switcheroo client\n");
@@ -2162,11 +2192,6 @@ static int azx_probe(struct pci_dev *pci,
 	}
 #endif /* CONFIG_SND_HDA_PATCH_LOADER */
 
-#ifndef CONFIG_SND_HDA_I915
-	if (HDA_CONTROLLER_IN_GPU(pci))
-		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n");
-#endif
-
 	if (schedule_probe)
 		schedule_delayed_work(&hda->probe_work, 0);
 
@@ -2263,30 +2288,6 @@ static int azx_probe_continue(struct azx *chip)
 	to_hda_bus(bus)->bus_probing = 1;
 	hda->probe_continued = 1;
 
-	/* bind with i915 if needed */
-	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
-		err = snd_hdac_i915_init(bus, true);
-		if (err < 0) {
-			/* if the controller is bound only with HDMI/DP
-			 * (for HSW and BDW), we need to abort the probe;
-			 * for other chips, still continue probing as other
-			 * codecs can be on the same link.
-			 */
-			if (HDA_CONTROLLER_IN_GPU(pci)) {
-				dev_err(chip->card->dev,
-					"HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n");
-				goto out_free;
-			} else {
-				/* don't bother any longer */
-				chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT;
-			}
-		}
-
-		/* HSW/BDW controllers need this power */
-		if (HDA_CONTROLLER_IN_GPU(pci))
-			hda->need_i915_power = true;
-	}
-
 	/* Request display power well for the HDA controller or codec. For
 	 * Haswell/Broadwell, both the display HDA controller and codec need
 	 * this power. For other platforms, like Baytrail/Braswell, only the
-- 
2.39.2

[PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

[Maarten Lankhorst]

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_no_wq can be used for this,
and we also use the newly added remove_no_wq for unbinding afterwards.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda-common-ops.c |  1 +
 sound/soc/sof/intel/hda.c            | 15 ++++++---------
 sound/soc/sof/intel/hda.h            |  1 +
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/sound/soc/sof/intel/hda-common-ops.c b/sound/soc/sof/intel/hda-common-ops.c
index 803b5e9087782..1e2e9b6a5c1c2 100644
--- a/sound/soc/sof/intel/hda-common-ops.c
+++ b/sound/soc/sof/intel/hda-common-ops.c
@@ -17,6 +17,7 @@
 struct snd_sof_dsp_ops sof_hda_common_ops = {
 	/* probe/remove/shutdown */
 	.probe_no_wq	= hda_dsp_probe_no_wq,
+	.remove_no_wq	= hda_dsp_remove_no_wq,
 	.probe		= hda_dsp_probe,
 	.remove		= hda_dsp_remove,
 
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index e918b5dadfa02..886073598e40f 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -1160,6 +1160,7 @@ int hda_dsp_probe_no_wq(struct snd_sof_dev *sdev)
 		return -ENOMEM;
 	sdev->pdata->hw_pdata = hdev;
 	hdev->desc = chip;
+	ret = hda_init(sdev);
 
 err:
 	return ret;
@@ -1195,9 +1196,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
 
 	/* set up HDA base */
 	bus = sof_to_bus(sdev);
-	ret = hda_init(sdev);
-	if (ret < 0)
-		goto hdac_bus_unmap;
 
 	if (sdev->dspless_mode_selected)
 		goto skip_dsp_setup;
@@ -1307,8 +1305,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
 		iounmap(sdev->bar[HDA_DSP_BAR]);
 hdac_bus_unmap:
 	platform_device_unregister(hdev->dmic_dev);
-	iounmap(bus->remap_addr);
-	hda_codec_i915_exit(sdev);
 
 	return ret;
 }
@@ -1317,7 +1313,6 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
 {
 	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
 	const struct sof_intel_dsp_desc *chip = hda->desc;
-	struct hdac_bus *bus = sof_to_bus(sdev);
 	struct pci_dev *pci = to_pci_dev(sdev->dev);
 	struct nhlt_acpi_table *nhlt = hda->nhlt;
 
@@ -1368,10 +1363,12 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
 	if (!sdev->dspless_mode_selected)
 		iounmap(sdev->bar[HDA_DSP_BAR]);
 
-	iounmap(bus->remap_addr);
-
-	sof_hda_bus_exit(sdev);
+	return 0;
+}
 
+int hda_dsp_remove_no_wq(struct snd_sof_dev *sdev)
+{
+	iounmap(sof_to_bus(sdev)->remap_addr);
 	hda_codec_i915_exit(sdev);
 
 	return 0;
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 89b8c239e9a5e..26ae11e4e1240 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -574,6 +574,7 @@ struct sof_intel_hda_stream {
  * DSP Core services.
  */
 int hda_dsp_probe_no_wq(struct snd_sof_dev *sdev);
+int hda_dsp_remove_no_wq(struct snd_sof_dev *sdev);
 int hda_dsp_probe(struct snd_sof_dev *sdev);
 int hda_dsp_remove(struct snd_sof_dev *sdev);
 int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask);
-- 
2.39.2

[PATCH v4 11/11] ALSA: hda/i915: Remove extra argument from snd_hdac_i915_init

[Maarten Lankhorst]

Now that all drivers have moved from modprobe loading to
handling -EPROBE_DEFER, we can remove the argument again.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
Changes since v1:
- Use dev_err_probe() to set reason in debugfs for deferred probe.
---
 include/sound/hda_i915.h        |  4 ++--
 sound/hda/hdac_i915.c           | 14 +++-----------
 sound/pci/hda/hda_intel.c       |  2 +-
 sound/soc/intel/avs/core.c      |  2 +-
 sound/soc/intel/skylake/skl.c   |  2 +-
 sound/soc/sof/intel/hda-codec.c |  2 +-
 6 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h
index f91bd66360865..6b79614a893b9 100644
--- a/include/sound/hda_i915.h
+++ b/include/sound/hda_i915.h
@@ -9,12 +9,12 @@
 
 #ifdef CONFIG_SND_HDA_I915
 void snd_hdac_i915_set_bclk(struct hdac_bus *bus);
-int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe);
+int snd_hdac_i915_init(struct hdac_bus *bus);
 #else
 static inline void snd_hdac_i915_set_bclk(struct hdac_bus *bus)
 {
 }
-static inline int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe)
+static inline int snd_hdac_i915_init(struct hdac_bus *bus)
 {
 	return -ENODEV;
 }
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 0765e5350e7ba..365c36fdf2058 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -156,7 +156,7 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
  *
  * Returns zero for success or a negative error code.
  */
-int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe)
+int snd_hdac_i915_init(struct hdac_bus *bus)
 {
 	struct drm_audio_component *acomp;
 	int err;
@@ -172,18 +172,10 @@ int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe)
 	acomp = bus->audio_component;
 	if (!acomp)
 		return -ENODEV;
-	if (allow_modprobe && !acomp->ops) {
-		if (!IS_ENABLED(CONFIG_MODULES) ||
-		    !request_module("i915")) {
-			/* 60s timeout */
-			wait_for_completion_killable_timeout(&acomp->master_bind_complete,
-							     msecs_to_jiffies(60 * 1000));
-		}
-	}
 	if (!acomp->ops) {
-		int err = allow_modprobe ? -ENODEV : -EPROBE_DEFER;
 		snd_hdac_acomp_exit(bus);
-		return dev_err_probe(bus->dev, err, "couldn't bind with audio component\n");
+		return dev_err_probe(bus->dev, -EPROBE_DEFER,
+				     "couldn't bind with audio component\n");
 	}
 	return 0;
 }
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index d529ef21f033a..e0f01a3ab231c 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2138,7 +2138,7 @@ static int azx_probe(struct pci_dev *pci,
 #ifdef CONFIG_SND_HDA_I915
 	/* bind with i915 if needed */
 	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
-		err = snd_hdac_i915_init(azx_bus(chip), false);
+		err = snd_hdac_i915_init(azx_bus(chip));
 		if (err < 0) {
 			/* if the controller is bound only with HDMI/DP
 			 * (for HSW and BDW), we need to abort the probe;
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 8a20639582487..33044f353575d 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -461,7 +461,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	pci_set_drvdata(pci, bus);
 	device_disable_async_suspend(dev);
 
-	ret = snd_hdac_i915_init(bus, false);
+	ret = snd_hdac_i915_init(bus);
 	if (ret == -EPROBE_DEFER)
 		goto err_i915_init;
 	else if (ret < 0)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 24bdbe2a53bec..f46f109d5856e 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -1056,7 +1056,7 @@ static int skl_probe(struct pci_dev *pci,
 	}
 
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
-		err = snd_hdac_i915_init(bus, false);
+		err = snd_hdac_i915_init(bus);
 		if (err < 0)
 			goto out_dmic_unregister;
 	}
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index f1fd5b44aaac9..8a5e99a898ecb 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);
 	if (ret < 0)
 		return ret;
 
-- 
2.39.2

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Pierre-Louis Bossart]

> +static inline int snd_sof_remove_no_wq(struct snd_sof_dev *sdev)
> +{
> +	if (sof_ops(sdev)->remove_no_wq)
> +		return sof_ops(sdev)->remove_no_wq(sdev);
> +
> +	return 0;
> +}

>  	/* probe/remove/shutdown */
> +	int (*probe_no_wq)(struct snd_sof_dev *sof_dev); /* optional */
> +	int (*remove_no_wq)(struct snd_sof_dev *sof_dev); /* optional */

My initial PR didn't have this remove_no_wq() callback.

For symmetry it could be useful if it is meant to undo what the
probe_no_wq() did, but conceptually the first thing we do in the remove
is make sure that workqueue is either not scheduled or we wait until it
completes.

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

Re: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

[kernel test robot]

Hi Maarten,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on tiwai-sound/for-next tiwai-sound/for-linus linus/master next-20230830]
[cannot apply to v6.5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/ASoC-SOF-core-add-no_wq-probe-and-remove-callbacks/20230831-033512
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/20230830153652.217855-11-maarten.lankhorst%40linux.intel.com
patch subject: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230831/202308310618.kqqYvniK-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230831/202308310618.kqqYvniK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308310618.kqqYvniK-lkp@intel.com/

All warnings (new ones prefixed by >>):

   sound/soc/sof/intel/hda.c: In function 'hda_dsp_probe':
>> sound/soc/sof/intel/hda.c:1173:26: warning: variable 'bus' set but not used [-Wunused-but-set-variable]
    1173 |         struct hdac_bus *bus;
         |                          ^~~


vim +/bus +1173 sound/soc/sof/intel/hda.c

47f868f27a979a Pierre-Louis Bossart  2023-08-30  1168  
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1169  int hda_dsp_probe(struct snd_sof_dev *sdev)
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1170  {
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1171  	struct pci_dev *pci = to_pci_dev(sdev->dev);
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1172  	struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata;
47f868f27a979a Pierre-Louis Bossart  2023-08-30 @1173  	struct hdac_bus *bus;
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1174  	int ret = 0;
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1175  
dd96daca6c83ec Liam Girdwood         2019-04-12  1176  	hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec",
dd96daca6c83ec Liam Girdwood         2019-04-12  1177  						       PLATFORM_DEVID_NONE,
dd96daca6c83ec Liam Girdwood         2019-04-12  1178  						       NULL, 0);
dd96daca6c83ec Liam Girdwood         2019-04-12  1179  	if (IS_ERR(hdev->dmic_dev)) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1180  		dev_err(sdev->dev, "error: failed to create DMIC device\n");
dd96daca6c83ec Liam Girdwood         2019-04-12  1181  		return PTR_ERR(hdev->dmic_dev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1182  	}
dd96daca6c83ec Liam Girdwood         2019-04-12  1183  
dd96daca6c83ec Liam Girdwood         2019-04-12  1184  	/*
dd96daca6c83ec Liam Girdwood         2019-04-12  1185  	 * use position update IPC if either it is forced
dd96daca6c83ec Liam Girdwood         2019-04-12  1186  	 * or we don't have other choice
dd96daca6c83ec Liam Girdwood         2019-04-12  1187  	 */
dd96daca6c83ec Liam Girdwood         2019-04-12  1188  #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_FORCE_IPC_POSITION)
dd96daca6c83ec Liam Girdwood         2019-04-12  1189  	hdev->no_ipc_position = 0;
dd96daca6c83ec Liam Girdwood         2019-04-12  1190  #else
dd96daca6c83ec Liam Girdwood         2019-04-12  1191  	hdev->no_ipc_position = sof_ops(sdev)->pcm_pointer ? 1 : 0;
dd96daca6c83ec Liam Girdwood         2019-04-12  1192  #endif
dd96daca6c83ec Liam Girdwood         2019-04-12  1193  
1f7b5d52be130e Peter Ujfalusi        2023-04-04  1194  	if (sdev->dspless_mode_selected)
1f7b5d52be130e Peter Ujfalusi        2023-04-04  1195  		hdev->no_ipc_position = 1;
1f7b5d52be130e Peter Ujfalusi        2023-04-04  1196  
dd96daca6c83ec Liam Girdwood         2019-04-12  1197  	/* set up HDA base */
dd96daca6c83ec Liam Girdwood         2019-04-12  1198  	bus = sof_to_bus(sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1199  
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1200  	if (sdev->dspless_mode_selected)
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1201  		goto skip_dsp_setup;
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1202  
dd96daca6c83ec Liam Girdwood         2019-04-12  1203  	/* DSP base */
dd96daca6c83ec Liam Girdwood         2019-04-12  1204  	sdev->bar[HDA_DSP_BAR] = pci_ioremap_bar(pci, HDA_DSP_BAR);
dd96daca6c83ec Liam Girdwood         2019-04-12  1205  	if (!sdev->bar[HDA_DSP_BAR]) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1206  		dev_err(sdev->dev, "error: ioremap error\n");
dd96daca6c83ec Liam Girdwood         2019-04-12  1207  		ret = -ENXIO;
dd96daca6c83ec Liam Girdwood         2019-04-12  1208  		goto hdac_bus_unmap;
dd96daca6c83ec Liam Girdwood         2019-04-12  1209  	}
dd96daca6c83ec Liam Girdwood         2019-04-12  1210  
dd96daca6c83ec Liam Girdwood         2019-04-12  1211  	sdev->mmio_bar = HDA_DSP_BAR;
dd96daca6c83ec Liam Girdwood         2019-04-12  1212  	sdev->mailbox_bar = HDA_DSP_BAR;
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1213  skip_dsp_setup:
dd96daca6c83ec Liam Girdwood         2019-04-12  1214  
dd96daca6c83ec Liam Girdwood         2019-04-12  1215  	/* allow 64bit DMA address if supported by H/W */
ab152afa2427bb Takashi Iwai          2021-01-14  1216  	if (dma_set_mask_and_coherent(&pci->dev, DMA_BIT_MASK(64))) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1217  		dev_dbg(sdev->dev, "DMA mask is 32 bit\n");
ab152afa2427bb Takashi Iwai          2021-01-14  1218  		dma_set_mask_and_coherent(&pci->dev, DMA_BIT_MASK(32));
dd96daca6c83ec Liam Girdwood         2019-04-12  1219  	}
8872fc0d045929 Takashi Iwai          2022-02-15  1220  	dma_set_max_seg_size(&pci->dev, UINT_MAX);
dd96daca6c83ec Liam Girdwood         2019-04-12  1221  
dd96daca6c83ec Liam Girdwood         2019-04-12  1222  	/* init streams */
dd96daca6c83ec Liam Girdwood         2019-04-12  1223  	ret = hda_dsp_stream_init(sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1224  	if (ret < 0) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1225  		dev_err(sdev->dev, "error: failed to init streams\n");
dd96daca6c83ec Liam Girdwood         2019-04-12  1226  		/*
dd96daca6c83ec Liam Girdwood         2019-04-12  1227  		 * not all errors are due to memory issues, but trying
dd96daca6c83ec Liam Girdwood         2019-04-12  1228  		 * to free everything does not harm
dd96daca6c83ec Liam Girdwood         2019-04-12  1229  		 */
dd96daca6c83ec Liam Girdwood         2019-04-12  1230  		goto free_streams;
dd96daca6c83ec Liam Girdwood         2019-04-12  1231  	}
dd96daca6c83ec Liam Girdwood         2019-04-12  1232  
dd96daca6c83ec Liam Girdwood         2019-04-12  1233  	/*
dd96daca6c83ec Liam Girdwood         2019-04-12  1234  	 * register our IRQ
dd96daca6c83ec Liam Girdwood         2019-04-12  1235  	 * let's try to enable msi firstly
dd96daca6c83ec Liam Girdwood         2019-04-12  1236  	 * if it fails, use legacy interrupt mode
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1237  	 * TODO: support msi multiple vectors
dd96daca6c83ec Liam Girdwood         2019-04-12  1238  	 */
bb67dd1878de57 Pierre-Louis Bossart  2019-08-06  1239  	if (hda_use_msi && pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_MSI) > 0) {
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1240  		dev_info(sdev->dev, "use msi interrupt mode\n");
7c11af9fcdc425 Bard Liao             2019-12-04  1241  		sdev->ipc_irq = pci_irq_vector(pci, 0);
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1242  		/* initialised to "false" by kzalloc() */
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1243  		sdev->msi_enabled = true;
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1244  	}
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1245  
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1246  	if (!sdev->msi_enabled) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1247  		dev_info(sdev->dev, "use legacy interrupt mode\n");
dd96daca6c83ec Liam Girdwood         2019-04-12  1248  		/*
dd96daca6c83ec Liam Girdwood         2019-04-12  1249  		 * in IO-APIC mode, hda->irq and ipc_irq are using the same
dd96daca6c83ec Liam Girdwood         2019-04-12  1250  		 * irq number of pci->irq
dd96daca6c83ec Liam Girdwood         2019-04-12  1251  		 */
dd96daca6c83ec Liam Girdwood         2019-04-12  1252  		sdev->ipc_irq = pci->irq;
dd96daca6c83ec Liam Girdwood         2019-04-12  1253  	}
dd96daca6c83ec Liam Girdwood         2019-04-12  1254  
dd96daca6c83ec Liam Girdwood         2019-04-12  1255  	dev_dbg(sdev->dev, "using IPC IRQ %d\n", sdev->ipc_irq);
7c11af9fcdc425 Bard Liao             2019-12-04  1256  	ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_interrupt_handler,
7c11af9fcdc425 Bard Liao             2019-12-04  1257  				   hda_dsp_interrupt_thread,
7c11af9fcdc425 Bard Liao             2019-12-04  1258  				   IRQF_SHARED, "AudioDSP", sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1259  	if (ret < 0) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1260  		dev_err(sdev->dev, "error: failed to register IPC IRQ %d\n",
dd96daca6c83ec Liam Girdwood         2019-04-12  1261  			sdev->ipc_irq);
7c11af9fcdc425 Bard Liao             2019-12-04  1262  		goto free_irq_vector;
dd96daca6c83ec Liam Girdwood         2019-04-12  1263  	}
dd96daca6c83ec Liam Girdwood         2019-04-12  1264  
dd96daca6c83ec Liam Girdwood         2019-04-12  1265  	pci_set_master(pci);
dd96daca6c83ec Liam Girdwood         2019-04-12  1266  	synchronize_irq(pci->irq);
dd96daca6c83ec Liam Girdwood         2019-04-12  1267  
dd96daca6c83ec Liam Girdwood         2019-04-12  1268  	/*
dd96daca6c83ec Liam Girdwood         2019-04-12  1269  	 * clear TCSEL to clear playback on some HD Audio
dd96daca6c83ec Liam Girdwood         2019-04-12  1270  	 * codecs. PCI TCSEL is defined in the Intel manuals.
dd96daca6c83ec Liam Girdwood         2019-04-12  1271  	 */
dd96daca6c83ec Liam Girdwood         2019-04-12  1272  	snd_sof_pci_update_bits(sdev, PCI_TCSEL, 0x07, 0);
dd96daca6c83ec Liam Girdwood         2019-04-12  1273  
dd96daca6c83ec Liam Girdwood         2019-04-12  1274  	/* init HDA capabilities */
dd96daca6c83ec Liam Girdwood         2019-04-12  1275  	ret = hda_init_caps(sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1276  	if (ret < 0)
dd96daca6c83ec Liam Girdwood         2019-04-12  1277  		goto free_ipc_irq;
dd96daca6c83ec Liam Girdwood         2019-04-12  1278  
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1279  	if (!sdev->dspless_mode_selected) {
1f5253b08e06bc Zhu Yingjiang         2019-05-22  1280  		/* enable ppcap interrupt */
1f5253b08e06bc Zhu Yingjiang         2019-05-22  1281  		hda_dsp_ctrl_ppcap_enable(sdev, true);
1f5253b08e06bc Zhu Yingjiang         2019-05-22  1282  		hda_dsp_ctrl_ppcap_int_enable(sdev, true);
dd96daca6c83ec Liam Girdwood         2019-04-12  1283  
dd96daca6c83ec Liam Girdwood         2019-04-12  1284  		/* set default mailbox offset for FW ready message */
dd96daca6c83ec Liam Girdwood         2019-04-12  1285  		sdev->dsp_box.offset = HDA_DSP_MBOX_UPLINK_OFFSET;
dd96daca6c83ec Liam Girdwood         2019-04-12  1286  
63e51fd33fef04 Ranjani Sridharan     2020-01-29  1287  		INIT_DELAYED_WORK(&hdev->d0i3_work, hda_dsp_d0i3_work);
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1288  	}
63e51fd33fef04 Ranjani Sridharan     2020-01-29  1289  
e2379d4a83da44 Pierre-Louis Bossart  2022-09-20  1290  	init_waitqueue_head(&hdev->waitq);
e2379d4a83da44 Pierre-Louis Bossart  2022-09-20  1291  
95fa7a62e16463 Pierre-Louis Bossart  2022-04-21  1292  	hdev->nhlt = intel_nhlt_init(sdev->dev);
95fa7a62e16463 Pierre-Louis Bossart  2022-04-21  1293  
dd96daca6c83ec Liam Girdwood         2019-04-12  1294  	return 0;
dd96daca6c83ec Liam Girdwood         2019-04-12  1295  
dd96daca6c83ec Liam Girdwood         2019-04-12  1296  free_ipc_irq:
dd96daca6c83ec Liam Girdwood         2019-04-12  1297  	free_irq(sdev->ipc_irq, sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1298  free_irq_vector:
dd96daca6c83ec Liam Girdwood         2019-04-12  1299  	if (sdev->msi_enabled)
dd96daca6c83ec Liam Girdwood         2019-04-12  1300  		pci_free_irq_vectors(pci);
dd96daca6c83ec Liam Girdwood         2019-04-12  1301  free_streams:
dd96daca6c83ec Liam Girdwood         2019-04-12  1302  	hda_dsp_stream_free(sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1303  /* dsp_unmap: not currently used */
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1304  	if (!sdev->dspless_mode_selected)
dd96daca6c83ec Liam Girdwood         2019-04-12  1305  		iounmap(sdev->bar[HDA_DSP_BAR]);
dd96daca6c83ec Liam Girdwood         2019-04-12  1306  hdac_bus_unmap:
5bb0ecddb2a7f6 Pierre-Louis Bossart  2021-03-01  1307  	platform_device_unregister(hdev->dmic_dev);
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1308  
dd96daca6c83ec Liam Girdwood         2019-04-12  1309  	return ret;
dd96daca6c83ec Liam Girdwood         2019-04-12  1310  }
dd96daca6c83ec Liam Girdwood         2019-04-12  1311  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

[kernel test robot]

Hi Maarten,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on tiwai-sound/for-next tiwai-sound/for-linus linus/master next-20230830]
[cannot apply to v6.5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/ASoC-SOF-core-add-no_wq-probe-and-remove-callbacks/20230831-033512
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/20230830153652.217855-11-maarten.lankhorst%40linux.intel.com
patch subject: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe
config: x86_64-randconfig-005-20230831 (https://download.01.org/0day-ci/archive/20230831/202308310715.lBXHTY4I-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230831/202308310715.lBXHTY4I-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308310715.lBXHTY4I-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> sound/soc/sof/intel/hda.c:1173:19: warning: variable 'bus' set but not used [-Wunused-but-set-variable]
           struct hdac_bus *bus;
                            ^
   1 warning generated.


vim +/bus +1173 sound/soc/sof/intel/hda.c

47f868f27a979a Pierre-Louis Bossart  2023-08-30  1168  
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1169  int hda_dsp_probe(struct snd_sof_dev *sdev)
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1170  {
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1171  	struct pci_dev *pci = to_pci_dev(sdev->dev);
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1172  	struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata;
47f868f27a979a Pierre-Louis Bossart  2023-08-30 @1173  	struct hdac_bus *bus;
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1174  	int ret = 0;
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1175  
dd96daca6c83ec Liam Girdwood         2019-04-12  1176  	hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec",
dd96daca6c83ec Liam Girdwood         2019-04-12  1177  						       PLATFORM_DEVID_NONE,
dd96daca6c83ec Liam Girdwood         2019-04-12  1178  						       NULL, 0);
dd96daca6c83ec Liam Girdwood         2019-04-12  1179  	if (IS_ERR(hdev->dmic_dev)) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1180  		dev_err(sdev->dev, "error: failed to create DMIC device\n");
dd96daca6c83ec Liam Girdwood         2019-04-12  1181  		return PTR_ERR(hdev->dmic_dev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1182  	}
dd96daca6c83ec Liam Girdwood         2019-04-12  1183  
dd96daca6c83ec Liam Girdwood         2019-04-12  1184  	/*
dd96daca6c83ec Liam Girdwood         2019-04-12  1185  	 * use position update IPC if either it is forced
dd96daca6c83ec Liam Girdwood         2019-04-12  1186  	 * or we don't have other choice
dd96daca6c83ec Liam Girdwood         2019-04-12  1187  	 */
dd96daca6c83ec Liam Girdwood         2019-04-12  1188  #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_FORCE_IPC_POSITION)
dd96daca6c83ec Liam Girdwood         2019-04-12  1189  	hdev->no_ipc_position = 0;
dd96daca6c83ec Liam Girdwood         2019-04-12  1190  #else
dd96daca6c83ec Liam Girdwood         2019-04-12  1191  	hdev->no_ipc_position = sof_ops(sdev)->pcm_pointer ? 1 : 0;
dd96daca6c83ec Liam Girdwood         2019-04-12  1192  #endif
dd96daca6c83ec Liam Girdwood         2019-04-12  1193  
1f7b5d52be130e Peter Ujfalusi        2023-04-04  1194  	if (sdev->dspless_mode_selected)
1f7b5d52be130e Peter Ujfalusi        2023-04-04  1195  		hdev->no_ipc_position = 1;
1f7b5d52be130e Peter Ujfalusi        2023-04-04  1196  
dd96daca6c83ec Liam Girdwood         2019-04-12  1197  	/* set up HDA base */
dd96daca6c83ec Liam Girdwood         2019-04-12  1198  	bus = sof_to_bus(sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1199  
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1200  	if (sdev->dspless_mode_selected)
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1201  		goto skip_dsp_setup;
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1202  
dd96daca6c83ec Liam Girdwood         2019-04-12  1203  	/* DSP base */
dd96daca6c83ec Liam Girdwood         2019-04-12  1204  	sdev->bar[HDA_DSP_BAR] = pci_ioremap_bar(pci, HDA_DSP_BAR);
dd96daca6c83ec Liam Girdwood         2019-04-12  1205  	if (!sdev->bar[HDA_DSP_BAR]) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1206  		dev_err(sdev->dev, "error: ioremap error\n");
dd96daca6c83ec Liam Girdwood         2019-04-12  1207  		ret = -ENXIO;
dd96daca6c83ec Liam Girdwood         2019-04-12  1208  		goto hdac_bus_unmap;
dd96daca6c83ec Liam Girdwood         2019-04-12  1209  	}
dd96daca6c83ec Liam Girdwood         2019-04-12  1210  
dd96daca6c83ec Liam Girdwood         2019-04-12  1211  	sdev->mmio_bar = HDA_DSP_BAR;
dd96daca6c83ec Liam Girdwood         2019-04-12  1212  	sdev->mailbox_bar = HDA_DSP_BAR;
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1213  skip_dsp_setup:
dd96daca6c83ec Liam Girdwood         2019-04-12  1214  
dd96daca6c83ec Liam Girdwood         2019-04-12  1215  	/* allow 64bit DMA address if supported by H/W */
ab152afa2427bb Takashi Iwai          2021-01-14  1216  	if (dma_set_mask_and_coherent(&pci->dev, DMA_BIT_MASK(64))) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1217  		dev_dbg(sdev->dev, "DMA mask is 32 bit\n");
ab152afa2427bb Takashi Iwai          2021-01-14  1218  		dma_set_mask_and_coherent(&pci->dev, DMA_BIT_MASK(32));
dd96daca6c83ec Liam Girdwood         2019-04-12  1219  	}
8872fc0d045929 Takashi Iwai          2022-02-15  1220  	dma_set_max_seg_size(&pci->dev, UINT_MAX);
dd96daca6c83ec Liam Girdwood         2019-04-12  1221  
dd96daca6c83ec Liam Girdwood         2019-04-12  1222  	/* init streams */
dd96daca6c83ec Liam Girdwood         2019-04-12  1223  	ret = hda_dsp_stream_init(sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1224  	if (ret < 0) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1225  		dev_err(sdev->dev, "error: failed to init streams\n");
dd96daca6c83ec Liam Girdwood         2019-04-12  1226  		/*
dd96daca6c83ec Liam Girdwood         2019-04-12  1227  		 * not all errors are due to memory issues, but trying
dd96daca6c83ec Liam Girdwood         2019-04-12  1228  		 * to free everything does not harm
dd96daca6c83ec Liam Girdwood         2019-04-12  1229  		 */
dd96daca6c83ec Liam Girdwood         2019-04-12  1230  		goto free_streams;
dd96daca6c83ec Liam Girdwood         2019-04-12  1231  	}
dd96daca6c83ec Liam Girdwood         2019-04-12  1232  
dd96daca6c83ec Liam Girdwood         2019-04-12  1233  	/*
dd96daca6c83ec Liam Girdwood         2019-04-12  1234  	 * register our IRQ
dd96daca6c83ec Liam Girdwood         2019-04-12  1235  	 * let's try to enable msi firstly
dd96daca6c83ec Liam Girdwood         2019-04-12  1236  	 * if it fails, use legacy interrupt mode
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1237  	 * TODO: support msi multiple vectors
dd96daca6c83ec Liam Girdwood         2019-04-12  1238  	 */
bb67dd1878de57 Pierre-Louis Bossart  2019-08-06  1239  	if (hda_use_msi && pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_MSI) > 0) {
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1240  		dev_info(sdev->dev, "use msi interrupt mode\n");
7c11af9fcdc425 Bard Liao             2019-12-04  1241  		sdev->ipc_irq = pci_irq_vector(pci, 0);
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1242  		/* initialised to "false" by kzalloc() */
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1243  		sdev->msi_enabled = true;
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1244  	}
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1245  
672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22  1246  	if (!sdev->msi_enabled) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1247  		dev_info(sdev->dev, "use legacy interrupt mode\n");
dd96daca6c83ec Liam Girdwood         2019-04-12  1248  		/*
dd96daca6c83ec Liam Girdwood         2019-04-12  1249  		 * in IO-APIC mode, hda->irq and ipc_irq are using the same
dd96daca6c83ec Liam Girdwood         2019-04-12  1250  		 * irq number of pci->irq
dd96daca6c83ec Liam Girdwood         2019-04-12  1251  		 */
dd96daca6c83ec Liam Girdwood         2019-04-12  1252  		sdev->ipc_irq = pci->irq;
dd96daca6c83ec Liam Girdwood         2019-04-12  1253  	}
dd96daca6c83ec Liam Girdwood         2019-04-12  1254  
dd96daca6c83ec Liam Girdwood         2019-04-12  1255  	dev_dbg(sdev->dev, "using IPC IRQ %d\n", sdev->ipc_irq);
7c11af9fcdc425 Bard Liao             2019-12-04  1256  	ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_interrupt_handler,
7c11af9fcdc425 Bard Liao             2019-12-04  1257  				   hda_dsp_interrupt_thread,
7c11af9fcdc425 Bard Liao             2019-12-04  1258  				   IRQF_SHARED, "AudioDSP", sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1259  	if (ret < 0) {
dd96daca6c83ec Liam Girdwood         2019-04-12  1260  		dev_err(sdev->dev, "error: failed to register IPC IRQ %d\n",
dd96daca6c83ec Liam Girdwood         2019-04-12  1261  			sdev->ipc_irq);
7c11af9fcdc425 Bard Liao             2019-12-04  1262  		goto free_irq_vector;
dd96daca6c83ec Liam Girdwood         2019-04-12  1263  	}
dd96daca6c83ec Liam Girdwood         2019-04-12  1264  
dd96daca6c83ec Liam Girdwood         2019-04-12  1265  	pci_set_master(pci);
dd96daca6c83ec Liam Girdwood         2019-04-12  1266  	synchronize_irq(pci->irq);
dd96daca6c83ec Liam Girdwood         2019-04-12  1267  
dd96daca6c83ec Liam Girdwood         2019-04-12  1268  	/*
dd96daca6c83ec Liam Girdwood         2019-04-12  1269  	 * clear TCSEL to clear playback on some HD Audio
dd96daca6c83ec Liam Girdwood         2019-04-12  1270  	 * codecs. PCI TCSEL is defined in the Intel manuals.
dd96daca6c83ec Liam Girdwood         2019-04-12  1271  	 */
dd96daca6c83ec Liam Girdwood         2019-04-12  1272  	snd_sof_pci_update_bits(sdev, PCI_TCSEL, 0x07, 0);
dd96daca6c83ec Liam Girdwood         2019-04-12  1273  
dd96daca6c83ec Liam Girdwood         2019-04-12  1274  	/* init HDA capabilities */
dd96daca6c83ec Liam Girdwood         2019-04-12  1275  	ret = hda_init_caps(sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1276  	if (ret < 0)
dd96daca6c83ec Liam Girdwood         2019-04-12  1277  		goto free_ipc_irq;
dd96daca6c83ec Liam Girdwood         2019-04-12  1278  
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1279  	if (!sdev->dspless_mode_selected) {
1f5253b08e06bc Zhu Yingjiang         2019-05-22  1280  		/* enable ppcap interrupt */
1f5253b08e06bc Zhu Yingjiang         2019-05-22  1281  		hda_dsp_ctrl_ppcap_enable(sdev, true);
1f5253b08e06bc Zhu Yingjiang         2019-05-22  1282  		hda_dsp_ctrl_ppcap_int_enable(sdev, true);
dd96daca6c83ec Liam Girdwood         2019-04-12  1283  
dd96daca6c83ec Liam Girdwood         2019-04-12  1284  		/* set default mailbox offset for FW ready message */
dd96daca6c83ec Liam Girdwood         2019-04-12  1285  		sdev->dsp_box.offset = HDA_DSP_MBOX_UPLINK_OFFSET;
dd96daca6c83ec Liam Girdwood         2019-04-12  1286  
63e51fd33fef04 Ranjani Sridharan     2020-01-29  1287  		INIT_DELAYED_WORK(&hdev->d0i3_work, hda_dsp_d0i3_work);
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1288  	}
63e51fd33fef04 Ranjani Sridharan     2020-01-29  1289  
e2379d4a83da44 Pierre-Louis Bossart  2022-09-20  1290  	init_waitqueue_head(&hdev->waitq);
e2379d4a83da44 Pierre-Louis Bossart  2022-09-20  1291  
95fa7a62e16463 Pierre-Louis Bossart  2022-04-21  1292  	hdev->nhlt = intel_nhlt_init(sdev->dev);
95fa7a62e16463 Pierre-Louis Bossart  2022-04-21  1293  
dd96daca6c83ec Liam Girdwood         2019-04-12  1294  	return 0;
dd96daca6c83ec Liam Girdwood         2019-04-12  1295  
dd96daca6c83ec Liam Girdwood         2019-04-12  1296  free_ipc_irq:
dd96daca6c83ec Liam Girdwood         2019-04-12  1297  	free_irq(sdev->ipc_irq, sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1298  free_irq_vector:
dd96daca6c83ec Liam Girdwood         2019-04-12  1299  	if (sdev->msi_enabled)
dd96daca6c83ec Liam Girdwood         2019-04-12  1300  		pci_free_irq_vectors(pci);
dd96daca6c83ec Liam Girdwood         2019-04-12  1301  free_streams:
dd96daca6c83ec Liam Girdwood         2019-04-12  1302  	hda_dsp_stream_free(sdev);
dd96daca6c83ec Liam Girdwood         2019-04-12  1303  /* dsp_unmap: not currently used */
9fc6786f549c4d Pierre-Louis Bossart  2023-04-04  1304  	if (!sdev->dspless_mode_selected)
dd96daca6c83ec Liam Girdwood         2019-04-12  1305  		iounmap(sdev->bar[HDA_DSP_BAR]);
dd96daca6c83ec Liam Girdwood         2019-04-12  1306  hdac_bus_unmap:
5bb0ecddb2a7f6 Pierre-Louis Bossart  2021-03-01  1307  	platform_device_unregister(hdev->dmic_dev);
47f868f27a979a Pierre-Louis Bossart  2023-08-30  1308  
dd96daca6c83ec Liam Girdwood         2019-04-12  1309  	return ret;
dd96daca6c83ec Liam Girdwood         2019-04-12  1310  }
dd96daca6c83ec Liam Girdwood         2019-04-12  1311  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 07/11] ASoC: Intel: avs: Move snd_hdac_i915_init to before probe_work.

[Amadeusz Sławiński]

On 8/30/2023 5:36 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@linux.intel.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

Re: [PATCH v4 08/11] ASoC: Intel: Skylake: Move snd_hdac_i915_init to before probe_work.

[Amadeusz Sławiński]

On 8/30/2023 5:36 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@linux.intel.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Maarten Lankhorst]

Hey,

Den 2023-08-30 kl. 19:13, skrev Pierre-Louis Bossart:
>> +static inline int snd_sof_remove_no_wq(struct snd_sof_dev *sdev)
>> +{
>> +	if (sof_ops(sdev)->remove_no_wq)
>> +		return sof_ops(sdev)->remove_no_wq(sdev);
>> +
>> +	return 0;
>> +}
>>   	/* probe/remove/shutdown */
>> +	int (*probe_no_wq)(struct snd_sof_dev *sof_dev); /* optional */
>> +	int (*remove_no_wq)(struct snd_sof_dev *sof_dev); /* optional */
> My initial PR didn't have this remove_no_wq() callback.
>
> For symmetry it could be useful if it is meant to undo what the
> probe_no_wq() did, but conceptually the first thing we do in the remove
> is make sure that workqueue is either not scheduled or we wait until it
> completes.
>
> 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);

That is exactly what I was using it for later on.

I had to have a counter to hda_init() in patch 10/11.

Cheers,
Maarten

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Kai Vehmanen]

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

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Kai Vehmanen]

Hi,

On Wed, 30 Aug 2023, Pierre-Louis Bossart wrote:

> >  	/* probe/remove/shutdown */
> > +	int (*probe_no_wq)(struct snd_sof_dev *sof_dev); /* optional */
> > +	int (*remove_no_wq)(struct snd_sof_dev *sof_dev); /* optional */
> 
> My initial PR didn't have this remove_no_wq() callback.
> 
> For symmetry it could be useful if it is meant to undo what the
> probe_no_wq() did, but conceptually the first thing we do in the remove
> is make sure that workqueue is either not scheduled or we wait until it
> completes.

I think that's exactly what the patch ends up with, remove_no_wq releases 
resources acquired in probe_no_wq, i.e. they are symmetrical.
 
> 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);

So this seems ok as well:
 - if wq is used, at remove, we first wait wq to be finished 
   and only then proceed with removal
 - the remove_no_wq is run only when the first step is completed

Br, Kai

Re: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

[Kai Vehmanen]

Hey,

On Wed, 30 Aug 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_no_wq can be used for this,
> and we also use the newly added remove_no_wq for unbinding afterwards.
[...]
> @@ -1317,7 +1313,6 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
>  {
>  	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
>  	const struct sof_intel_dsp_desc *chip = hda->desc;
> -	struct hdac_bus *bus = sof_to_bus(sdev);
>  	struct pci_dev *pci = to_pci_dev(sdev->dev);
>  	struct nhlt_acpi_table *nhlt = hda->nhlt;
>  
> @@ -1368,10 +1363,12 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
>  	if (!sdev->dspless_mode_selected)
>  		iounmap(sdev->bar[HDA_DSP_BAR]);
>  
> -	iounmap(bus->remap_addr);
> -
> -	sof_hda_bus_exit(sdev);
> +	return 0;
> +}
>  
> +int hda_dsp_remove_no_wq(struct snd_sof_dev *sdev)
> +{
> +	iounmap(sof_to_bus(sdev)->remap_addr);
>  	hda_codec_i915_exit(sdev);

I think here we drop the call to sof_hda_bus_exit() which should be done 
in hda_dsp_remove_no_wq() to counter hda_init().

Rest looks good to me, the "no_wq" variants do symmetric ops,
so we can handle both wq an non-wq cases.

Br, Kai

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Péter Ujfalusi]

On 01/09/2023 15:15, Kai Vehmanen wrote:
> 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?

The ops needs to be preserved even if the wq fails since the patch wants
to call snd_sof_remove_no_wq() unconditionally on remove.

> 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

I guess we can revert that in sof-dev, if this is the preferred way?

> ... not yet in Mark's tree.
> 
> Otherwise patch looks good to me.

I would have not created the snd_sof_remove_no_wq() as it makes not much
functional sense.
It might be even better if the remove in the wq would do the
hda_codec_i915_exit() as the module will remain in there until the user
removes it.

> 
> Br, Kai

-- 
Péter

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Pierre-Louis Bossart]

On 9/1/23 08:44, Péter Ujfalusi wrote:
> 
> 
> On 01/09/2023 15:15, Kai Vehmanen wrote:
>> 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?
> 
> The ops needs to be preserved even if the wq fails since the patch wants
> to call snd_sof_remove_no_wq() unconditionally on remove.
> 
>> 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
> 
> I guess we can revert that in sof-dev, if this is the preferred way?
> 
>> ... not yet in Mark's tree.
>>
>> Otherwise patch looks good to me.
> 
> I would have not created the snd_sof_remove_no_wq() as it makes not much
> functional sense.
> It might be even better if the remove in the wq would do the
> hda_codec_i915_exit() as the module will remain in there until the user
> removes it.

I think find all this very confusing, because there is no workqueue used
in the remove steps. The workqueue is only used ONCE during the probe.

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Pierre-Louis Bossart]

On 9/5/23 08:37, Pierre-Louis Bossart wrote:
> 
> 
> On 9/1/23 08:44, Péter Ujfalusi wrote:
>>
>>
>> On 01/09/2023 15:15, Kai Vehmanen wrote:
>>> 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?
>>
>> The ops needs to be preserved even if the wq fails since the patch wants
>> to call snd_sof_remove_no_wq() unconditionally on remove.
>>
>>> 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
>>
>> I guess we can revert that in sof-dev, if this is the preferred way?
>>
>>> ... not yet in Mark's tree.
>>>
>>> Otherwise patch looks good to me.
>>
>> I would have not created the snd_sof_remove_no_wq() as it makes not much
>> functional sense.
>> It might be even better if the remove in the wq would do the
>> hda_codec_i915_exit() as the module will remain in there until the user
>> removes it.
> 
> I think find all this very confusing, because there is no workqueue used
> in the remove steps. The workqueue is only used ONCE during the probe.

Maybe we should just remove any references to workqueues, and have

probe_start (cannot run in a wq)
probe (may run in a wq)
remove (cannot run in a wq, needs to call cancel_work_sync() if the
probe runs in a wq)
remove_last (cannot run in a wq, releases all resources acquired in
probe_start)

Or something similar that shows the symmetry between steps and when the
wq is allowed.

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Péter Ujfalusi]

On 07/09/2023 20:29, Pierre-Louis Bossart wrote:
>> I think find all this very confusing, because there is no workqueue used
>> in the remove steps. The workqueue is only used ONCE during the probe.
> 
> Maybe we should just remove any references to workqueues, and have
> 
> probe_start (cannot run in a wq)
> probe (may run in a wq)
> remove (cannot run in a wq, needs to call cancel_work_sync() if the
> probe runs in a wq)
> remove_last (cannot run in a wq, releases all resources acquired in
> probe_start)
> 
> Or something similar that shows the symmetry between steps and when the
> wq is allowed.

What we have atm:
snd_sof_probe - might be called from wq
snd_sof_remove - might be called from wq (cleans up the snd_sof_probe
		 step)

We want a callbacks for hardware/device probing, right, split the
snd_sof_probe (and remove) to be able to support a sane level of
deferred probing support.

With that in mind:
snd_sof_device_probe - Not called from wq (to handle deferred probing)
snd_sof_probe - might be called from wq

snd_sof_remove - might be called from wq (cleans up the snd_sof_probe
		 step)
snd_sof_device_remove - Not called from wq (to up the
			snd_sof_device_probe step)

Naming option: s/device/hardware

However, I think the snd_sof_device_remove itself is redundant and we
might not need it at all as in case we have wq and there is a failure in
there we do want to release resources as much as possible. The module
will be kept loaded (no deferred handling in wq) and that might block
PM, other devices to behave correctly. Iow, if the wq has failure we
should do a cleanup to the best effort to reach a level like the driver
is not even loaded.

Doable? I thin it is.

-- 
Péter

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Pierre-Louis Bossart]

> What we have atm:
> snd_sof_probe - might be called from wq
> snd_sof_remove - might be called from wq (cleans up the snd_sof_probe
> 		 step)

I don't think it's correct, snd_sof_remove cannot be called from a wq.
The device core knows nothing about workqueues.

> We want a callbacks for hardware/device probing, right, split the
> snd_sof_probe (and remove) to be able to support a sane level of
> deferred probing support.
> 
> With that in mind:
> snd_sof_device_probe - Not called from wq (to handle deferred probing)
> snd_sof_probe - might be called from wq
> 
> snd_sof_remove - might be called from wq (cleans up the snd_sof_probe
> 		 step)
> snd_sof_device_remove - Not called from wq (to up the
> 			snd_sof_device_probe step)
> 
> Naming option: s/device/hardware

I like the 'device' hint since it's directly related to the device (or
subsystem) callbacks.

> However, I think the snd_sof_device_remove itself is redundant and we
> might not need it at all as in case we have wq and there is a failure in
> there we do want to release resources as much as possible. The module
> will be kept loaded (no deferred handling in wq) and that might block
> PM, other devices to behave correctly. Iow, if the wq has failure we
> should do a cleanup to the best effort to reach a level like the driver
> is not even loaded.

If we have a failure in a workqueue used for probe, then we have to
clean-up everything since nothing in the device core will do so for us.

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Péter Ujfalusi]

On 12/09/2023 03:25, Pierre-Louis Bossart wrote:
> 
> 
>> What we have atm:
>> snd_sof_probe - might be called from wq
>> snd_sof_remove - might be called from wq (cleans up the snd_sof_probe
>> 		 step)
> 
> I don't think it's correct, snd_sof_remove cannot be called from a wq.
> The device core knows nothing about workqueues.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/sof/core.c#n328

it is called on the error path of sof_probe_continue(), which can be run
in a workque.

>> We want a callbacks for hardware/device probing, right, split the
>> snd_sof_probe (and remove) to be able to support a sane level of
>> deferred probing support.
>>
>> With that in mind:
>> snd_sof_device_probe - Not called from wq (to handle deferred probing)
>> snd_sof_probe - might be called from wq
>>
>> snd_sof_remove - might be called from wq (cleans up the snd_sof_probe
>> 		 step)
>> snd_sof_device_remove - Not called from wq (to up the
>> 			snd_sof_device_probe step)
>>
>> Naming option: s/device/hardware
> 
> I like the 'device' hint since it's directly related to the device (or
> subsystem) callbacks.
> 
>> However, I think the snd_sof_device_remove itself is redundant and we
>> might not need it at all as in case we have wq and there is a failure in
>> there we do want to release resources as much as possible. The module
>> will be kept loaded (no deferred handling in wq) and that might block
>> PM, other devices to behave correctly. Iow, if the wq has failure we
>> should do a cleanup to the best effort to reach a level like the driver
>> is not even loaded.
> 
> If we have a failure in a workqueue used for probe, then we have to
> clean-up everything since nothing in the device core will do so for us.

Yes, this makes the snd_sof_device_remove() redundant or at least the
definition of it is no longer a mirror of snd_sof_device_probe():

snd_sof_device_remove - might be called from wq (cleans up the
			snd_sof_device_probe step)

Any failure in sof_probe_continue() should execute the
snd_sof_device_remove(), snd_sof_remove() is only involved after the
snd_sof_probe() have returned with success.

I think this makes actually makes sense and it is well defined.
On module remove we need to take into account the case when we have
failed in wq similarly as we do currently (the resources have been freed
up already).

-- 
Péter

Re: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

[Pierre-Louis Bossart]

On 9/12/23 02:10, Péter Ujfalusi wrote:
> 
> 
> On 12/09/2023 03:25, Pierre-Louis Bossart wrote:
>>
>>
>>> What we have atm:
>>> snd_sof_probe - might be called from wq
>>> snd_sof_remove - might be called from wq (cleans up the snd_sof_probe
>>> 		 step)
>>
>> I don't think it's correct, snd_sof_remove cannot be called from a wq.
>> The device core knows nothing about workqueues.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/sof/core.c#n328
> 
> it is called on the error path of sof_probe_continue(), which can be run
> in a workque.
> 
>>> We want a callbacks for hardware/device probing, right, split the
>>> snd_sof_probe (and remove) to be able to support a sane level of
>>> deferred probing support.
>>>
>>> With that in mind:
>>> snd_sof_device_probe - Not called from wq (to handle deferred probing)
>>> snd_sof_probe - might be called from wq
>>>
>>> snd_sof_remove - might be called from wq (cleans up the snd_sof_probe
>>> 		 step)
>>> snd_sof_device_remove - Not called from wq (to up the
>>> 			snd_sof_device_probe step)
>>>
>>> Naming option: s/device/hardware
>>
>> I like the 'device' hint since it's directly related to the device (or
>> subsystem) callbacks.
>>
>>> However, I think the snd_sof_device_remove itself is redundant and we
>>> might not need it at all as in case we have wq and there is a failure in
>>> there we do want to release resources as much as possible. The module
>>> will be kept loaded (no deferred handling in wq) and that might block
>>> PM, other devices to behave correctly. Iow, if the wq has failure we
>>> should do a cleanup to the best effort to reach a level like the driver
>>> is not even loaded.
>>
>> If we have a failure in a workqueue used for probe, then we have to
>> clean-up everything since nothing in the device core will do so for us.
> 
> Yes, this makes the snd_sof_device_remove() redundant or at least the
> definition of it is no longer a mirror of snd_sof_device_probe():
> 
> snd_sof_device_remove - might be called from wq (cleans up the
> 			snd_sof_device_probe step)
> 
> Any failure in sof_probe_continue() should execute the
> snd_sof_device_remove(), snd_sof_remove() is only involved after the
> snd_sof_probe() have returned with success.
> 
> I think this makes actually makes sense and it is well defined.
> On module remove we need to take into account the case when we have
> failed in wq similarly as we do currently (the resources have been freed
> up already).

Agree, I stand corrected, thanks Peter.