diff mbox series

[6/7] ASoC: SOF: Intel: Remove deferred probe for SOF

Message ID 20230718084522.116952-7-maarten.lankhorst@linux.intel.com (mailing list archive)
State Superseded
Headers show
Series sound: Use -EPROBE_DEFER instead of i915 module loading. | expand

Commit Message

Maarten Lankhorst July 18, 2023, 8:45 a.m. UTC
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>
---
 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(-)

Comments

Mark Brown July 18, 2023, 2:13 p.m. UTC | #1
On Tue, Jul 18, 2023 at 10:45:21AM +0200, 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.

Acked-by: Mark Brown <broonie@kernel.org>
Kai Vehmanen July 18, 2023, 5:04 p.m. UTC | #2
Hi,

thank you Maarten for doing the series! I think a lot of people will be 
happy to get rid of the 60sec timeout. 

I kicked off a CI run SOF public infra for the whole series at
https://github.com/thesofproject/linux/pull/4478
Some results still in progress but so far so good.

Some concerns inline:

On Tue, 18 Jul 2023, 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.

We actually have a request_module() for the regular HDA codec drivers as 
well (sof_probe_continue() -> snd_sof_probe() -> hda_dsp_probe() -> 
hda_init_caps() -> hda_codec_probe_bus(). But right, this is not 
necessarily a problem on its own, so it looks we indeed can drop the work 
queue. Nice!

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

My only bigger concern is corner cases where the display PCI device is on 
the bus and visible to kernel, but for some reason there is no working 
driver in the system or it is disabled.

With this patch, not having a workign display driver means that there is 
also no audio in the system as the SOF driver will never get probed.

In current mainline, one will get the 60sec timeout warning and then
audio driver will proceed to probe and you'll have audio support (minus 
HDMI/DP).

This is mostly an issue with very new hardware (e.g. hw is still 
behind force_probe flag in xe/i915 driver), but we've had some odd
cases with e.g. systems with both Intel IGFX and other vendors' DGPU. 
Audio drivers see the Intel VGA controller in system and will
call snd_hdac_i915_init(), but the audio component bind will never
succeed if the the Intel IGFX is not in actual use.

Will need a bit of time to think about possible scenarios. Possibly this 
is not an issue outside early development systems. In theory if IGFX is 
disabled in BIOS, and not visible to OS, we are good, and if it's visible, 
the i915/xe driver should be loaded, so we are good again.

Br, Kai
Takashi Iwai July 19, 2023, 6:01 a.m. UTC | #3
On Tue, 18 Jul 2023 19:04:41 +0200,
Kai Vehmanen wrote:
> 
> > 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;
> 
> My only bigger concern is corner cases where the display PCI device is on 
> the bus and visible to kernel, but for some reason there is no working 
> driver in the system or it is disabled.
> 
> With this patch, not having a workign display driver means that there is 
> also no audio in the system as the SOF driver will never get probed.
> 
> In current mainline, one will get the 60sec timeout warning and then
> audio driver will proceed to probe and you'll have audio support (minus 
> HDMI/DP).

Yeah, that was a concern in the past, too.  e.g. when you pass
"nomodeset" boot option, the driver will become unusable, even if the
bus is used generically for both analog and HDMI codecs.

> This is mostly an issue with very new hardware (e.g. hw is still 
> behind force_probe flag in xe/i915 driver), but we've had some odd
> cases with e.g. systems with both Intel IGFX and other vendors' DGPU. 
> Audio drivers see the Intel VGA controller in system and will
> call snd_hdac_i915_init(), but the audio component bind will never
> succeed if the the Intel IGFX is not in actual use.
> 
> Will need a bit of time to think about possible scenarios. Possibly this 
> is not an issue outside early development systems. In theory if IGFX is 
> disabled in BIOS, and not visible to OS, we are good, and if it's visible, 
> the i915/xe driver should be loaded, so we are good again.

The 60 seconds timeout is a thing "better than complete disablement",
so it's not ideal, either.  Maybe we can add something like the
following:

- Check when the deferred probe takes too long, and warn it
- Provide some runtime option to disable the component binding, so
  that user can work around it if needed


thanks,

Takashi
Maarten Lankhorst July 19, 2023, 9:48 a.m. UTC | #4
Hey,

On 2023-07-19 08:01, Takashi Iwai wrote:
> On Tue, 18 Jul 2023 19:04:41 +0200,
> Kai Vehmanen wrote:
>>> 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;
>> My only bigger concern is corner cases where the display PCI device is on
>> the bus and visible to kernel, but for some reason there is no working
>> driver in the system or it is disabled.
>>
>> With this patch, not having a workign display driver means that there is
>> also no audio in the system as the SOF driver will never get probed.
>>
>> In current mainline, one will get the 60sec timeout warning and then
>> audio driver will proceed to probe and you'll have audio support (minus
>> HDMI/DP).
> Yeah, that was a concern in the past, too.  e.g. when you pass
> "nomodeset" boot option, the driver will become unusable, even if the
> bus is used generically for both analog and HDMI codecs.

Yeah, I have no answer for this. My guess is that in an ideal world, the optional features
related to HDMI outputs would be put in a separate sub-driver, which could -EPROBE_DEFER.
Only when this driver loads, features related to display will work, but the main audio driver
could still load.

>> This is mostly an issue with very new hardware (e.g. hw is still
>> behind force_probe flag in xe/i915 driver), but we've had some odd
>> cases with e.g. systems with both Intel IGFX and other vendors' DGPU.
>> Audio drivers see the Intel VGA controller in system and will
>> call snd_hdac_i915_init(), but the audio component bind will never
>> succeed if the the Intel IGFX is not in actual use.
>>
>> Will need a bit of time to think about possible scenarios. Possibly this
>> is not an issue outside early development systems. In theory if IGFX is
>> disabled in BIOS, and not visible to OS, we are good, and if it's visible,
>> the i915/xe driver should be loaded, so we are good again.
> The 60 seconds timeout is a thing "better than complete disablement",
> so it's not ideal, either.  Maybe we can add something like the
> following:
>
> - Check when the deferred probe takes too long, and warn it
> - Provide some runtime option to disable the component binding, so
>    that user can work around it if needed

A module option to snd_hdac_i915_init would probably be the least of all 
evils here.

I see the removal of the 60 second timeout as a good thing regardless. 
:-) Usually when nomodeset is used, it's just for safe mode.

With the addition of  the xe driver, blindly modprobing i915 will fall 
apart regardless.

~Maarten
Takashi Iwai July 19, 2023, 11:06 a.m. UTC | #5
On Wed, 19 Jul 2023 11:48:06 +0200,
Maarten Lankhorst wrote:
> 
>     The 60 seconds timeout is a thing "better than complete disablement",
>     so it's not ideal, either.  Maybe we can add something like the
>     following:
>     
>     - Check when the deferred probe takes too long, and warn it
>     - Provide some runtime option to disable the component binding, so
>       that user can work around it if needed
>     
> A module option to snd_hdac_i915_init would probably be the least of all evils
> here.

Yes, probably it's the easiest option and sufficient.


thanks,

Takashi
Mark Brown July 19, 2023, 12:39 p.m. UTC | #6
On Wed, Jul 19, 2023 at 02:13:59PM +0200, Maarten Lankhorst wrote:
> 
> On 2023-07-19 13:06, Takashi Iwai wrote:
> > On Wed, 19 Jul 2023 11:48:06 +0200,
> > Maarten Lankhorst wrote:
> > > 
> > >      The 60 seconds timeout is a thing "better than complete disablement",
> > >      so it's not ideal, either.  Maybe we can add something like the
> > >      following:
> > >      - Check when the deferred probe takes too long, and warn it
> > >      - Provide some runtime option to disable the component binding, so
> > >        that user can work around it if needed
> > > A module option to snd_hdac_i915_init would probably be the least of all evils
> > > here.
> > 
> > Yes, probably it's the easiest option and sufficient.
> > 
> > 
> > thanks,
> > 
> > Takashi
> Hey,
> 
> Patch below, can be applied immediately iresspective of the other patches.
> 
> ---->8----------
> 
> 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

Please don't bury new patches in the middle of mails, it just makes it
hard to apply the patch since tooling doesn't understand what's going
on.

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.
Takashi Iwai July 19, 2023, 12:47 p.m. UTC | #7
On Wed, 19 Jul 2023 14:13:59 +0200,
Maarten Lankhorst wrote:
> 
> 
> On 2023-07-19 13:06, Takashi Iwai wrote:
> > On Wed, 19 Jul 2023 11:48:06 +0200,
> > Maarten Lankhorst wrote:
> >> 
> >>      The 60 seconds timeout is a thing "better than complete disablement",
> >>      so it's not ideal, either.  Maybe we can add something like the
> >>      following:
> >>           - Check when the deferred probe takes too long, and warn
> >> it
> >>      - Provide some runtime option to disable the component binding, so
> >>        that user can work around it if needed
> >>      A module option to snd_hdac_i915_init would probably be the
> >> least of all evils
> >> here.
> > 
> > Yes, probably it's the easiest option and sufficient.
> > 
> > 
> > thanks,
> > 
> > Takashi
> Hey,
> 
> Patch below, can be applied immediately iresspective of the other patches.
> 
> ---->8----------
> 
> 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.

The check of video_firmware_drivers_only() may help a bit, but I
believe we still need an option to override the behavior, from the
same reason as why i915.modeset option behaves so.  In general,
nomodeset is for a debugging purpose, and without an option, you'll
have no way to re-enable the HD-audio even if you could reload the
graphics driver.


thanks,

Takashi

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 1637dc6e630a6..90bcf84f7b2ce 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -11,6 +11,8 @@
>  #include <sound/hda_i915.h>
>  #include <sound/hda_register.h>
> 
> +#include <video/nomodeset.h>
> +
>  #define IS_HSW_CONTROLLER(pci) (((pci)->device == 0x0a0c) || \
>  				((pci)->device == 0x0c0c) || \
>  				((pci)->device == 0x0d0c) || \
> @@ -122,6 +124,9 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
>  {
>  	struct pci_dev *display_dev = NULL;
> 
> +	if (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 &&
>
Kai Vehmanen July 19, 2023, 1:32 p.m. UTC | #8
Hi,

On Wed, 19 Jul 2023, Maarten Lankhorst wrote:

> On Tue, 18 Jul 2023 19:04:41 +0200, Kai Vehmanen wrote:
>> My only bigger concern is corner cases where the display PCI device is 
on 
>> the bus and visible to kernel, but for some reason there is no working 
>> driver in the system or it is disabled.
> 
> Yeah, I have no answer for this. My guess is that in an ideal world, the optional features
> related to HDMI outputs would be put in a separate sub-driver, which could -EPROBE_DEFER.
> Only when this driver loads, features related to display will work, but the main audio driver
> could still load.

in longer term, we have ongoing work in SOF to allow exposing multiple 
cards (e.g. to have a separate card for HDMI/DP PCM devices), and we
are continuously working at improving the data we get from ACPI to 
have less guesswork in the driver. But this really doesn't help in the 
shortterm and/or cover all scenarios.

So for now, this is legacy we just need to deal with. OTOH, I do agree
that...

> A module option to snd_hdac_i915_init would probably be the least of all evils here.
> 
> I see the removal of the 60 second timeout as a good thing regardless. :-) Usually when nomodeset is used, it's just for safe
> mode.
> 
> With the addition of  the xe driver, blindly modprobing i915 will fall apart regardless.

The modprobing of i915 from the audio driver, has always felt a bit 
out-of-place, and with the xe driver, this simply won't scale anymore.

The test results so far look good and this patchset works ok even if some 
of the more complex multi-GPU configurations we have, so I think with a 
module option to snd_hdac_i915, I'd be ready to go with this.

Br, Kai
Maarten Lankhorst July 19, 2023, 3:59 p.m. UTC | #9
Hey,

On 2023-07-19 14:39, Mark Brown wrote:
> On Wed, Jul 19, 2023 at 02:13:59PM +0200, Maarten Lankhorst wrote:
>>
>> On 2023-07-19 13:06, Takashi Iwai wrote:
>>> On Wed, 19 Jul 2023 11:48:06 +0200,
>>> Maarten Lankhorst wrote:
>>>>
>>>>       The 60 seconds timeout is a thing "better than complete disablement",
>>>>       so it's not ideal, either.  Maybe we can add something like the
>>>>       following:
>>>>       - Check when the deferred probe takes too long, and warn it
>>>>       - Provide some runtime option to disable the component binding, so
>>>>         that user can work around it if needed
>>>> A module option to snd_hdac_i915_init would probably be the least of all evils
>>>> here.
>>>
>>> Yes, probably it's the easiest option and sufficient.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>> Hey,
>>
>> Patch below, can be applied immediately iresspective of the other patches.
>>
>> ---->8----------
>>
>> 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
> 
> Please don't bury new patches in the middle of mails, it just makes it
> hard to apply the patch since tooling doesn't understand what's going
> on.
> 
> Please don't send new patches in reply to old patches or serieses, this
> makes it harder for both people and tools to understand what is going
> on - it can bury things in mailboxes and make it difficult to keep track
> of what current patches are, both for the new patches and the old ones.
I will send a new version in a bit, with all comments addressed.

I need to finish testing first.

~Maarten
diff mbox series

Patch

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