[PATCH 1/2] [v2] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency
From: Arnd Bergmann arnd@arndb.de
The sof-pci-dev driver fails to link when built into the kernel and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
All other drivers using this interface already use a 'select SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it seems reasonable to do the same here.
The stub implementation in the header makes the problem harder to find, as it avoids the link error when SND_INTEL_DSP_CONFIG is completely disabled, without any obvious upsides. Remove these stubs to make it clearer that the driver is in fact needed here.
Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code") Signed-off-by: Arnd Bergmann arnd@arndb.de --- v2: fix various build issues in the first version, now passes all randconfig builds I tried --- include/sound/intel-dsp-config.h | 17 ----------------- sound/soc/sof/Kconfig | 2 ++ sound/soc/sof/intel/Kconfig | 4 ++-- sound/soc/sof/intel/byt.c | 2 +- 4 files changed, 5 insertions(+), 20 deletions(-)
diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h index d4609077c258..94667e870029 100644 --- a/include/sound/intel-dsp-config.h +++ b/include/sound/intel-dsp-config.h @@ -18,24 +18,7 @@ enum { SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF };
-#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG) - int snd_intel_dsp_driver_probe(struct pci_dev *pci); int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]);
-#else - -static inline int snd_intel_dsp_driver_probe(struct pci_dev *pci) -{ - return SND_INTEL_DSP_DRIVER_ANY; -} - -static inline -int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]) -{ - return SND_INTEL_DSP_DRIVER_ANY; -} - -#endif - #endif diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig index 031dad5fc4c7..051fd3d27047 100644 --- a/sound/soc/sof/Kconfig +++ b/sound/soc/sof/Kconfig @@ -12,6 +12,7 @@ if SND_SOC_SOF_TOPLEVEL config SND_SOC_SOF_PCI tristate "SOF PCI enumeration support" depends on PCI + select SND_INTEL_DSP_CONFIG select SND_SOC_SOF select SND_SOC_ACPI if ACPI help @@ -23,6 +24,7 @@ config SND_SOC_SOF_PCI config SND_SOC_SOF_ACPI tristate "SOF ACPI enumeration support" depends on ACPI || COMPILE_TEST + select SND_INTEL_DSP_CONFIG select SND_SOC_SOF select SND_SOC_ACPI if ACPI select IOSF_MBI if X86 && PCI diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index ff9266413a06..67365ce0d86d 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -53,7 +53,7 @@ config SND_SOC_SOF_INTEL_COMMON if SND_SOC_SOF_ACPI
config SND_SOC_SOF_BAYTRAIL - bool "SOF support for Baytrail, Braswell and Cherrytrail" + tristate "SOF support for Baytrail, Braswell and Cherrytrail" select SND_SOC_SOF_INTEL_ATOM_HIFI_EP select SND_INTEL_DSP_CONFIG help @@ -70,7 +70,7 @@ config SND_SOC_SOF_BAYTRAIL If unsure select "N".
config SND_SOC_SOF_BROADWELL - bool "SOF support for Broadwell" + tristate "SOF support for Broadwell" select SND_INTEL_DSP_CONFIG select SND_SOC_SOF_INTEL_COMMON select SND_SOC_SOF_INTEL_HIFI_EP_IPC diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c index 65abcca94521..42dba3da1ca3 100644 --- a/sound/soc/sof/intel/byt.c +++ b/sound/soc/sof/intel/byt.c @@ -660,7 +660,7 @@ EXPORT_SYMBOL_NS(tng_chip_info, SND_SOC_SOF_MERRIFIELD);
#endif /* CONFIG_SND_SOC_SOF_MERRIFIELD */
-#ifdef CONFIG_SND_SOC_SOF_BAYTRAIL +#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) static void byt_reset_dsp_disable_int(struct snd_sof_dev *sdev) { /* Disable Interrupt from both sides */
From: Arnd Bergmann arnd@arndb.de
The Kconfig logic around SND_SOC_SOF_INTEL_SOUNDWIRE tries to ensure that all sound modules can be built with the minimal dependencies, but this fails in some configurations:
x86_64-linux-ld: sound/hda/intel-dsp-config.o: in function `snd_intel_dsp_driver_probe': intel-dsp-config.c:(.text+0x134): undefined reference to `sdw_intel_acpi_scan'
Specifically, this happens if the dsp-config driver is built-in but does not need to use soundwire, while CONFIG_SOUNDWIRE_INTEL is enabled as a loadable module.
An easier and more correct way to do this is to remove CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE_LINK and instead have the two drivers that can link against SOUNDWIRE_INTEL, i.e. DSP_CONFIG and SND_SOC_SOF_HDA, select that driver whenever SND_SOC_SOF_INTEL_SOUNDWIRE_LINK is set.
This however means that SND_SOC_SOF_INTEL_SOUNDWIRE cannot be selected by users when SOUNDWIRE is only usable by loadable modules and one or more drivers using SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE is built-in.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- sound/hda/Kconfig | 1 + sound/soc/sof/intel/Kconfig | 16 ++++++---------- 2 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig index 3bc9224d5e4f..ecab814d7698 100644 --- a/sound/hda/Kconfig +++ b/sound/hda/Kconfig @@ -44,5 +44,6 @@ config SND_INTEL_NHLT config SND_INTEL_DSP_CONFIG tristate select SND_INTEL_NHLT if ACPI + select SOUNDWIRE_INTEL if SND_SOC_SOF_INTEL_SOUNDWIRE # this config should be selected only for Intel DSP platforms. # A fallback is provided so that the code compiles in all cases. diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index 67365ce0d86d..df8f9760870e 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -330,13 +330,17 @@ config SND_SOC_SOF_HDA tristate select SND_HDA_EXT_CORE if SND_SOC_SOF_HDA_LINK select SND_SOC_HDAC_HDA if SND_SOC_SOF_HDA_AUDIO_CODEC + select SOUNDWIRE_INTEL if SND_SOC_SOF_INTEL_SOUNDWIRE help This option is not user-selectable but automagically handled by 'select' statements at a higher level.
-config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK +config SND_SOC_SOF_INTEL_SOUNDWIRE bool "SOF support for SoundWire" - depends on SOUNDWIRE && ACPI + depends on SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE + depends on SOUNDWIRE + depends on ACPI + depends on !(SOUNDWIRE=m && SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE=y) help This adds support for SoundWire with Sound Open Firmware for Intel(R) platforms. @@ -345,14 +349,6 @@ config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK
config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE tristate - select SND_SOC_SOF_INTEL_SOUNDWIRE if SND_SOC_SOF_INTEL_SOUNDWIRE_LINK - help - This option is not user-selectable but automagically handled by - 'select' statements at a higher level. - -config SND_SOC_SOF_INTEL_SOUNDWIRE - tristate - select SOUNDWIRE_INTEL help This option is not user-selectable but automagically handled by 'select' statements at a higher level.
On 1/12/21 2:32 PM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
The Kconfig logic around SND_SOC_SOF_INTEL_SOUNDWIRE tries to ensure that all sound modules can be built with the minimal dependencies, but this fails in some configurations:
x86_64-linux-ld: sound/hda/intel-dsp-config.o: in function `snd_intel_dsp_driver_probe': intel-dsp-config.c:(.text+0x134): undefined reference to `sdw_intel_acpi_scan'
Specifically, this happens if the dsp-config driver is built-in but does not need to use soundwire, while CONFIG_SOUNDWIRE_INTEL is enabled as a loadable module.
An easier and more correct way to do this is to remove CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE_LINK and instead have the two drivers that can link against SOUNDWIRE_INTEL, i.e. DSP_CONFIG and SND_SOC_SOF_HDA, select that driver whenever SND_SOC_SOF_INTEL_SOUNDWIRE_LINK is set.
This however means that SND_SOC_SOF_INTEL_SOUNDWIRE cannot be selected by users when SOUNDWIRE is only usable by loadable modules and one or more drivers using SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE is built-in.
The problem is real, but the proposal isn't completely right, there is absolutely no logical link or functional dependency between INTEL_DSP_CONFIG and SOUNDWIRE.
We have a similar case with HDaudio, the two buses can be selected as tristates, but the SOF configuration needs to match.
In both cases, either we add a 'depends' and users need to make sure the configurations match on the two sides. Or we use select but one of the selections will be overridden and becomes meaningless.
Arnd, do you mind if I give it a try on my side?
Signed-off-by: Arnd Bergmann arnd@arndb.de
sound/hda/Kconfig | 1 + sound/soc/sof/intel/Kconfig | 16 ++++++---------- 2 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig index 3bc9224d5e4f..ecab814d7698 100644 --- a/sound/hda/Kconfig +++ b/sound/hda/Kconfig @@ -44,5 +44,6 @@ config SND_INTEL_NHLT config SND_INTEL_DSP_CONFIG tristate select SND_INTEL_NHLT if ACPI
- select SOUNDWIRE_INTEL if SND_SOC_SOF_INTEL_SOUNDWIRE # this config should be selected only for Intel DSP platforms. # A fallback is provided so that the code compiles in all cases.
diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index 67365ce0d86d..df8f9760870e 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -330,13 +330,17 @@ config SND_SOC_SOF_HDA tristate select SND_HDA_EXT_CORE if SND_SOC_SOF_HDA_LINK select SND_SOC_HDAC_HDA if SND_SOC_SOF_HDA_AUDIO_CODEC
- select SOUNDWIRE_INTEL if SND_SOC_SOF_INTEL_SOUNDWIRE help This option is not user-selectable but automagically handled by 'select' statements at a higher level.
-config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK +config SND_SOC_SOF_INTEL_SOUNDWIRE bool "SOF support for SoundWire"
- depends on SOUNDWIRE && ACPI
- depends on SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE
- depends on SOUNDWIRE
- depends on ACPI
- depends on !(SOUNDWIRE=m && SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE=y) help This adds support for SoundWire with Sound Open Firmware for Intel(R) platforms.
@@ -345,14 +349,6 @@ config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK
config SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE tristate
- select SND_SOC_SOF_INTEL_SOUNDWIRE if SND_SOC_SOF_INTEL_SOUNDWIRE_LINK
- help
This option is not user-selectable but automagically handled by'select' statements at a higher level.-config SND_SOC_SOF_INTEL_SOUNDWIRE
- tristate
- select SOUNDWIRE_INTEL help This option is not user-selectable but automagically handled by 'select' statements at a higher level.
On Tue, Jan 12, 2021 at 10:03 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
On 1/12/21 2:32 PM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
The Kconfig logic around SND_SOC_SOF_INTEL_SOUNDWIRE tries to ensure that all sound modules can be built with the minimal dependencies, but this fails in some configurations:
x86_64-linux-ld: sound/hda/intel-dsp-config.o: in function `snd_intel_dsp_driver_probe': intel-dsp-config.c:(.text+0x134): undefined reference to `sdw_intel_acpi_scan'
Specifically, this happens if the dsp-config driver is built-in but does not need to use soundwire, while CONFIG_SOUNDWIRE_INTEL is enabled as a loadable module.
An easier and more correct way to do this is to remove CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE_LINK and instead have the two drivers that can link against SOUNDWIRE_INTEL, i.e. DSP_CONFIG and SND_SOC_SOF_HDA, select that driver whenever SND_SOC_SOF_INTEL_SOUNDWIRE_LINK is set.
This however means that SND_SOC_SOF_INTEL_SOUNDWIRE cannot be selected by users when SOUNDWIRE is only usable by loadable modules and one or more drivers using SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE is built-in.
The problem is real, but the proposal isn't completely right, there is absolutely no logical link or functional dependency between INTEL_DSP_CONFIG and SOUNDWIRE.
If that is true, would it be possible to move the call to sdw_intel_acpi_scan() out of these drivers and one layer higher where the dependency actually is?
I was indeed wondering whether the intel-dsp-config.c is just another layering violation: this is another generic piece of code that seems to contain too much knowledge about specific hardware implementations.
We have a similar case with HDaudio, the two buses can be selected as tristates, but the SOF configuration needs to match.
In both cases, either we add a 'depends' and users need to make sure the configurations match on the two sides. Or we use select but one of the selections will be overridden and becomes meaningless.
Maybe something like this:
config SND_SOC_SOF_INTEL_SOUNDWIRE - bool "SOF support for SoundWire" + tristate "SOF support for SoundWire" - depends on SOUNDWIRE && ACPI + depends on SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE + depends on SOUNDWIRE + depends on ACPI + depends on !(SOUNDWIRE=m && SND_SOC_SOF_INTEL_SOUNDWIRE_LINK_BASELINE=y) + select SOUNDWIRE_INTEL
I have not tried it, but that should keep it all in one place.
Arnd, do you mind if I give it a try on my side?
I have no specific attachment to my patch, this was just what I came up with to fix the build regression locally.
Arnd
On Tue, Jan 12, 2021 at 11:36:15PM +0100, Arnd Bergmann wrote:
I was indeed wondering whether the intel-dsp-config.c is just another layering violation: this is another generic piece of code that seems to contain too much knowledge about specific hardware implementations.
The purpose of that code is to try to figure out which of the multiple sets of drivers and firmwares available for Intel systems it's best to run on any given system by default and choose between them at runtime (or allow that choice to be overridden by users) so it's all about knowing about specific hardware implementations.
On Tue, Jan 12, 2021 at 9:32 PM Arnd Bergmann arnd@kernel.org wrote:
v2: fix various build issues in the first version, now passes all randconfig builds I tried
Please disregard this version, while I have a tree that passes the randconfig builds now, this was not the patch from it that I wanted to send...
Arnd
On 1/14/21 9:09 AM, Arnd Bergmann wrote:
On Tue, Jan 12, 2021 at 9:32 PM Arnd Bergmann arnd@kernel.org wrote:
v2: fix various build issues in the first version, now passes all randconfig builds I tried
Please disregard this version, while I have a tree that passes the randconfig builds now, this was not the patch from it that I wanted to send...
No worries, I reworked the PCI case completely. Still running tests to make sure there's no regression https://github.com/thesofproject/linux/pull/2683
On Thu, Jan 14, 2021 at 7:07 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
On 1/14/21 9:09 AM, Arnd Bergmann wrote:
On Tue, Jan 12, 2021 at 9:32 PM Arnd Bergmann arnd@kernel.org wrote:
v2: fix various build issues in the first version, now passes all randconfig builds I tried
Please disregard this version, while I have a tree that passes the randconfig builds now, this was not the patch from it that I wanted to send...
No worries, I reworked the PCI case completely. Still running tests to make sure there's no regression https://github.com/thesofproject/linux/pull/2683
Ok, I see. I had not realized that you already did the PCI bits as well, and applied my original patch to your tree, as I had meant to resend my "ASoC: SOF: ACPI: avoid reverse module dependency" patch after fixing some additional build failures in it. I have now applied the relevant commits from your branch to my randconfig tree, and will let you know if that finds anything more.
From what I can tell so far, you already included the fixups that I had
locally, and more.
Arnd
On 1/14/21 3:19 PM, Arnd Bergmann wrote:
On Thu, Jan 14, 2021 at 7:07 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
On 1/14/21 9:09 AM, Arnd Bergmann wrote:
On Tue, Jan 12, 2021 at 9:32 PM Arnd Bergmann arnd@kernel.org wrote:
v2: fix various build issues in the first version, now passes all randconfig builds I tried
Please disregard this version, while I have a tree that passes the randconfig builds now, this was not the patch from it that I wanted to send...
No worries, I reworked the PCI case completely. Still running tests to make sure there's no regression https://github.com/thesofproject/linux/pull/2683
Ok, I see. I had not realized that you already did the PCI bits as well, and applied my original patch to your tree, as I had meant to resend my "ASoC: SOF: ACPI: avoid reverse module dependency" patch after fixing some additional build failures in it. I have now applied the relevant commits from your branch to my randconfig tree, and will let you know if that finds anything more.
From what I can tell so far, you already included the fixups that I had locally, and more.
Thanks Arnd.
Yes we added a couple of things. I missed the fact that we hard-coded 'sof-acpi' to determine if we use the legacy or SOF driver, and other problems with driver names that I screwed-up.
The dependencies part should be finished now, I am just testing on my side that nothing broke on the test devices (which is slower than I wanted due to other ACPI scan things I had to revert locally). I will add a couple of suggestions from Guennadi but this should be done tomorrow.
participants (3)
-
Arnd Bergmann -
Mark Brown -
Pierre-Louis Bossart