| Message ID | 20210103135257.3611821-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency | expand |
On Sun, 03 Jan 2021 14:52:32 +0100, Arnd Bergmann wrote: > > 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> IMO, the problem is rather the unconditional calls of snd_intel_dsp_driver_probe() in snd-soc-sof-pci and snd-soc-sof-acpi drivers. Those calls should be done only if Intel drivers are involved. So, wrapping the call with ifdef CONFIG_SND_SOC_SOF_INTEL_PCI or CONFIG_SND_SOC_SOF_INTEL_ACPI would work better (although those are a bit uglier). thanks, Takashi > --- > include/sound/intel-dsp-config.h | 17 ----------------- > sound/soc/sof/Kconfig | 2 ++ > 2 files changed, 2 insertions(+), 17 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 > -- > 2.29.2 >
On Mon, Jan 04, 2021 at 03:09:24PM +0100, Takashi Iwai wrote: > IMO, the problem is rather the unconditional calls of > snd_intel_dsp_driver_probe() in snd-soc-sof-pci and snd-soc-sof-acpi > drivers. Those calls should be done only if Intel drivers are > involved. So, wrapping the call with ifdef > CONFIG_SND_SOC_SOF_INTEL_PCI or CONFIG_SND_SOC_SOF_INTEL_ACPI would > work better (although those are a bit uglier). Or stubbing them which might be neater.
Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a): > 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> > --- > include/sound/intel-dsp-config.h | 17 ----------------- > sound/soc/sof/Kconfig | 2 ++ > 2 files changed, 2 insertions(+), 17 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) The SOF drivers selects the DSP config code only when required (for specific platforms - see sound/soc/sof/intel/Kconfig). It seems that the above if should be modified as: #if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) && IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG)) So the buildin drivers which do not require the DSP config probe can be compiled without this dependency. Jaroslav
On Mon, 04 Jan 2021 16:00:05 +0100, Jaroslav Kysela wrote: > > Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a): > > 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> > > --- > > include/sound/intel-dsp-config.h | 17 ----------------- > > sound/soc/sof/Kconfig | 2 ++ > > 2 files changed, 2 insertions(+), 17 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) > > The SOF drivers selects the DSP config code only when required (for specific > platforms - see sound/soc/sof/intel/Kconfig). > > It seems that the above if should be modified as: > > #if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) && > IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG)) > > So the buildin drivers which do not require the DSP config probe can be > compiled without this dependency. As I wrote in another post, a part of the problem is that SOF PCI and ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even if no Intel driver is bound. So even if changing like the above (or better to use IS_REACHABLE() macro) works around the issue, the call pattern needs to be reconsidered. thanks, Takashi
On Mon, Jan 4, 2021 at 4:00 PM Jaroslav Kysela <perex@perex.cz> wrote: > Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a): > > 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) > > The SOF drivers selects the DSP config code only when required (for specific > platforms - see sound/soc/sof/intel/Kconfig). > > It seems that the above if should be modified as: > > #if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) && > IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG)) > > So the buildin drivers which do not require the DSP config probe can be > compiled without this dependency. This would be the same as #if IS_REACHABLE(CONFIG_SND_INTEL_DSP_CONFIG) but using that macro is almost always a bad idea, as it tends to hide dependency problems and causes things to silently not work right when the Kconfig rules are incorrect. Arnd
On Mon, Jan 4, 2021 at 4:05 PM Takashi Iwai <tiwai@suse.de> wrote: > On Mon, 04 Jan 2021 16:00:05 +0100, Jaroslav Kysela wrote: > > > > Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a): > > > 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> > > > --- > > > include/sound/intel-dsp-config.h | 17 ----------------- > > > sound/soc/sof/Kconfig | 2 ++ > > > 2 files changed, 2 insertions(+), 17 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) > > > > The SOF drivers selects the DSP config code only when required (for specific > > platforms - see sound/soc/sof/intel/Kconfig). > > > > It seems that the above if should be modified as: > > > > #if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) && > > IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG)) > > > > So the buildin drivers which do not require the DSP config probe can be > > compiled without this dependency. > > As I wrote in another post, a part of the problem is that SOF PCI and > ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even > if no Intel driver is bound. Makes sense. Is there an existing Kconfig that could be used to decide whether the drivers use SND_INTEL_DSP_CONFIG or not? Could it be part of the device specific driver_data? According to sof_pci_ids[], all PCI IDs are Intel specific, but I can't tell which ones need the DSP config. > So even if changing like the above (or > better to use IS_REACHABLE() macro) works around the issue, the call > pattern needs to be reconsidered. If the callers are fixed to address this, then I would expect the IS_REACHABLE() or IS_ENABLED() to no longer be needed either. Arnd
Hey, On Tue, 5 Jan 2021, Arnd Bergmann wrote: > On Mon, Jan 4, 2021 at 4:05 PM Takashi Iwai <tiwai@suse.de> wrote: > > As I wrote in another post, a part of the problem is that SOF PCI and > > ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even > > if no Intel driver is bound. > > Makes sense. Is there an existing Kconfig that could be used to > decide whether the drivers use SND_INTEL_DSP_CONFIG or not? no, unfortunately not. This is selected per platform in sound/soc/sof/intel/Kconfig. CONFIG_SND_SOC_SOF_INTEL_PCI is close, but there is at least one platform that does not use SND_INTEL_DSP_CONFIG. > According to sof_pci_ids[], all PCI IDs are Intel specific, but I can't > tell which ones need the DSP config. Indeed currently all the ids are Intel ones (and with exception of old Merrifield, all use DSP config). But that's just how it is now. > Could it be part of the device specific driver_data? This would certainly be a clean way and allow to remove the Intel-specific calls from sof_pci_probe(). As a short-term solution, IS_REACHABLE() seems ok as well. Br, Kai
On Tue, Jan 5, 2021 at 4:39 PM Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote: > > Could it be part of the device specific driver_data? > > This would certainly be a clean way and allow to remove the Intel-specific > calls from sof_pci_probe(). As a short-term solution, IS_REACHABLE() > seems ok as well. I looked at it some more and my conclusion is that the problem is the way the drivers mix device specific and generic data: The generic acpi or pci driver should never need to know about individual devices and their dependencies. Instead of just exporting some generic helper functions, these are the top-level drivers and the device specific drivers are the ones exporting the data. It's a common mistake, but it always leads to complexity like this and we tend to end up having to undo it all. I prototyped a patch to do this for the acpi driver, and it seems much more straightforward this way, please have a look. commit a83ecfed5b31dfc862e04c9bf77d2107a1047c9b Author: Arnd Bergmann <arnd@arndb.de> Date: Tue Jan 5 19:47:35 2021 +0100 ASoC: SOF: Intel: avoid reverse module dependency The SOF-ACPI driver is backwards from the normal Linux model, it has a generic driver that knows about all the specific drivers, as opposed to having hardware specific drivers that link against a common framework. This requires ugly Kconfig magic and leads to missed dependencies as seen in this link error: arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe' Change it to use the normal probe order of starting with a specific device in a driver, turning the sof-acpi-dev.c driver into a library. Signed-off-by: Arnd Bergmann <arnd@arndb.de> sound/soc/sof/intel/Kconfig | 34 +++--------- sound/soc/sof/intel/bdw.c | 51 ++++++++++++++++-- sound/soc/sof/intel/byt.c | 104 ++++++++++++++++++++++++++++++++---- sound/soc/sof/intel/shim.h | 10 ++-- sound/soc/sof/sof-acpi-dev.c | 122 ++----------------------------------------- 5 files changed, 156 insertions(+), 165 deletions(-) The PCI driver is left as an exercise to the reader. Arnd
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