| Message ID | 20240219093900.644574-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a3d543b9e6599fbbb9efc1876919627960c5e97a |
| Headers | show |
| Series | ASoC: SOF: amd: fix soundwire dependencies | expand |
On 19/02/24 15:08, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The soundwire-amd driver has a bit of a layering violation requiring > the SOF driver to directly call into its exported symbols rather than > through an abstraction. > > The SND_SOC_SOF_AMD_SOUNDWIRE Kconfig symbol tries to deal with the > dependency by selecting SOUNDWIRE_AMD in a complicated set of conditions, > but gets it wrong for a configuration involving SND_SOC_SOF_AMD_COMMON=y, > SND_SOC_SOF_AMD_ACP63=m, and SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=m > SOUNDWIRE_AMD=m, which results in a link failure: > > ld.lld: error: undefined symbol: sdw_amd_get_slave_info >>>> referenced by acp-common.c > ld.lld: error: undefined symbol: amd_sdw_scan_controller > ld.lld: error: undefined symbol: sdw_amd_probe > ld.lld: error: undefined symbol: sdw_amd_exit >>>> referenced by acp.c >>>> sound/soc/sof/amd/acp.o:(amd_sof_acp_remove) in archive vmlinux.a > In essence, the SND_SOC_SOF_AMD_COMMON option cannot be built-in when > trying to link against a modular SOUNDWIRE_AMD driver. > > Since CONFIG_SOUNDWIRE_AMD is a user-visible option, it really should > never be selected by another driver in the first place, so replace the > extra complexity with a normal Kconfig dependency in SND_SOC_SOF_AMD_SOUNDWIRE, > plus a top-level check that forbids any of the AMD SOF drivers from being > built-in with CONFIG_SOUNDWIRE_AMD=m. > > In normal configs, they should all either be built-in or all loadable > modules anyway, so this simplification does not limit any real usecases. Tested this patch. SOUNWIRE_AMD flag is not selected by default causing AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire. > > Fixes: d948218424bf ("ASoC: SOF: amd: add code for invoking soundwire manager helper functions") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > sound/soc/sof/amd/Kconfig | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/sof/amd/Kconfig b/sound/soc/sof/amd/Kconfig > index c3bbe6c70fb2..2729c6eb3feb 100644 > --- a/sound/soc/sof/amd/Kconfig > +++ b/sound/soc/sof/amd/Kconfig > @@ -6,6 +6,7 @@ > > config SND_SOC_SOF_AMD_TOPLEVEL > tristate "SOF support for AMD audio DSPs" > + depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD > depends on X86 || COMPILE_TEST > help > This adds support for Sound Open Firmware for AMD platforms. > @@ -62,15 +63,14 @@ config SND_SOC_SOF_ACP_PROBES > > config SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE > tristate > - select SOUNDWIRE_AMD if SND_SOC_SOF_AMD_SOUNDWIRE != n > select SND_AMD_SOUNDWIRE_ACPI if ACPI > > config SND_SOC_SOF_AMD_SOUNDWIRE > tristate "SOF support for SoundWire based AMD platforms" > default SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE > depends on SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE > - depends on ACPI && SOUNDWIRE > - depends on !(SOUNDWIRE=m && SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=y) > + depends on ACPI > + depends on SOUNDWIRE_AMD > help > This adds support for SoundWire with Sound Open Firmware > for AMD platforms.
On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote: > On 19/02/24 15:08, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> In normal configs, they should all either be built-in or all loadable >> modules anyway, so this simplification does not limit any real usecases. > > Tested this patch. SOUNWIRE_AMD flag is not selected by default causing > AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire. Yes, that is what I described. But as SOUNWIRE_AMD is a user visible symbol, there is no problem in expecting users to enable it when they have this hardware, and distros just enable all the drivers anyway. Arnd
On 20/02/24 11:43, Arnd Bergmann wrote: > On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote: >> On 19/02/24 15:08, Arnd Bergmann wrote: >>> From: Arnd Bergmann <arnd@arndb.de> >>> In normal configs, they should all either be built-in or all loadable >>> modules anyway, so this simplification does not limit any real usecases. >> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing >> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire. > Yes, that is what I described. But as SOUNWIRE_AMD is a user visible > symbol, there is no problem in expecting users to enable it when they > have this hardware, and distros just enable all the drivers anyway. Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom platforms instead of explicitly enabling the Kconfig option. > > Arnd
On Tue, Feb 20, 2024, at 07:23, Mukunda,Vijendar wrote: > On 20/02/24 11:43, Arnd Bergmann wrote: >> On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote: >>> On 19/02/24 15:08, Arnd Bergmann wrote: >>>> From: Arnd Bergmann <arnd@arndb.de> >>>> In normal configs, they should all either be built-in or all loadable >>>> modules anyway, so this simplification does not limit any real usecases. >>> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing >>> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire. >> Yes, that is what I described. But as SOUNWIRE_AMD is a user visible >> symbol, there is no problem in expecting users to enable it when they >> have this hardware, and distros just enable all the drivers anyway. > > Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom > platforms instead of explicitly enabling the Kconfig option. Maybe use 'default SND_SOC_SOF_AMD_TOPLEVEL' then? I don't think copying the mistake from the intel driver is helpful, though I agree it would be nice to be consistent between them. As a general rule, you should not have a Kconfig symbol that is both user visible and also selected by the drivers that depend on it. To avoid the dependency problems from coming back and keep the complexity to a minimum, I think there are two logical ways to handle soundwire: a) keep the current drivers/soundwire/Kconfig contents and change all the 'select SOUNDWIRE_foo' to 'depends on'. b) keep all the 'select SOUNDWIRE_foo' but drop the prompts, requiring that all drivers that use soundwire have the correct select statements, with the main CONFIG_SOUNDWIRE getting selected by the individual drivers. Arnd
On 20/02/24 12:40, Arnd Bergmann wrote: > On Tue, Feb 20, 2024, at 07:23, Mukunda,Vijendar wrote: >> On 20/02/24 11:43, Arnd Bergmann wrote: >>> On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote: >>>> On 19/02/24 15:08, Arnd Bergmann wrote: >>>>> From: Arnd Bergmann <arnd@arndb.de> >>>>> In normal configs, they should all either be built-in or all loadable >>>>> modules anyway, so this simplification does not limit any real usecases. >>>> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing >>>> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire. >>> Yes, that is what I described. But as SOUNWIRE_AMD is a user visible >>> symbol, there is no problem in expecting users to enable it when they >>> have this hardware, and distros just enable all the drivers anyway. >> Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom >> platforms instead of explicitly enabling the Kconfig option. > Maybe use 'default SND_SOC_SOF_AMD_TOPLEVEL' then? Didn't get your point. Even with the current patch, if we select 'SOUNDWIRE_AMD' flag explicitly AMD ACP63 SOF driver Kconfig option is not visible for user configuration. This results in ACP63 SOF driver won't be built at all. > > I don't think copying the mistake from the intel driver > is helpful, though I agree it would be nice to be consistent > between them. > > As a general rule, you should not have a Kconfig symbol that > is both user visible and also selected by the drivers that > depend on it. > > To avoid the dependency problems from coming back and keep > the complexity to a minimum, I think there are two logical > ways to handle soundwire: > > a) keep the current drivers/soundwire/Kconfig contents and > change all the 'select SOUNDWIRE_foo' to 'depends on'. Current patch already using 'depends on SOUNDWIRE_AMD" for SND_SOC_SOF_AMD_SOUNDWIRE Kconfig option. Still we couldn't see SND_SOC_SOF_AMD_ACP63 Kconfig option is enabled. > > b) keep all the 'select SOUNDWIRE_foo' but drop the prompts, > requiring that all drivers that use soundwire have the > correct select statements, with the main CONFIG_SOUNDWIRE > getting selected by the individual drivers. Didn't get your point. Could you please elaborate? > Arnd
On Tue, Feb 20, 2024, at 08:54, Mukunda,Vijendar wrote: > On 20/02/24 12:40, Arnd Bergmann wrote: >> On Tue, Feb 20, 2024, at 07:23, Mukunda,Vijendar wrote: >>> On 20/02/24 11:43, Arnd Bergmann wrote: >>>> On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote: >>>>> On 19/02/24 15:08, Arnd Bergmann wrote: >>>>>> From: Arnd Bergmann <arnd@arndb.de> >>>>>> In normal configs, they should all either be built-in or all loadable >>>>>> modules anyway, so this simplification does not limit any real usecases. >>>>> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing >>>>> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire. >>>> Yes, that is what I described. But as SOUNWIRE_AMD is a user visible >>>> symbol, there is no problem in expecting users to enable it when they >>>> have this hardware, and distros just enable all the drivers anyway. >>> Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom >>> platforms instead of explicitly enabling the Kconfig option. >> Maybe use 'default SND_SOC_SOF_AMD_TOPLEVEL' then? > Didn't get your point. > > Even with the current patch, if we select 'SOUNDWIRE_AMD' flag explicitly > AMD ACP63 SOF driver Kconfig option is not visible for user configuration. > This results in ACP63 SOF driver won't be built at all. I don't understand what you mean here. What I see in linux-next both with and without my patch is config SND_SOC_SOF_AMD_ACP63 tristate "SOF support for ACP6.3 platform" depends on SND_SOC_SOF_PCI so it clearly should be visible as long as SND_SOC_SOF_PCI is enabled. There is still a problem that SND_SOC_SOF_AMD_TOPLEVEL can't use my "depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD" trick if SOUNDWIRE_AMD in turn uses "default SND_SOC_SOF_AMD_TOPLEVEL", but I don't think you meant that, right? >> I don't think copying the mistake from the intel driver >> is helpful, though I agree it would be nice to be consistent >> between them. >> >> As a general rule, you should not have a Kconfig symbol that >> is both user visible and also selected by the drivers that >> depend on it. >> >> To avoid the dependency problems from coming back and keep >> the complexity to a minimum, I think there are two logical >> ways to handle soundwire: >> >> a) keep the current drivers/soundwire/Kconfig contents and >> change all the 'select SOUNDWIRE_foo' to 'depends on'. > > Current patch already using 'depends on SOUNDWIRE_AMD" for > SND_SOC_SOF_AMD_SOUNDWIRE Kconfig option. Correct, because this is the Kconfig option that actually controls whether sound/soc/sof/amd/acp-common.c calls into the soundwire-amd module. > Still we couldn't see SND_SOC_SOF_AMD_ACP63 Kconfig option > is enabled. I need more information here. Do you have additional patches on top of what is in today's linux-next? I have it enabled on my build here. Arnd
On 20/02/24 13:51, Arnd Bergmann wrote: > On Tue, Feb 20, 2024, at 08:54, Mukunda,Vijendar wrote: >> On 20/02/24 12:40, Arnd Bergmann wrote: >>> On Tue, Feb 20, 2024, at 07:23, Mukunda,Vijendar wrote: >>>> On 20/02/24 11:43, Arnd Bergmann wrote: >>>>> On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote: >>>>>> On 19/02/24 15:08, Arnd Bergmann wrote: >>>>>>> From: Arnd Bergmann <arnd@arndb.de> >>>>>>> In normal configs, they should all either be built-in or all loadable >>>>>>> modules anyway, so this simplification does not limit any real usecases. >>>>>> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing >>>>>> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire. >>>>> Yes, that is what I described. But as SOUNWIRE_AMD is a user visible >>>>> symbol, there is no problem in expecting users to enable it when they >>>>> have this hardware, and distros just enable all the drivers anyway. >>>> Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom >>>> platforms instead of explicitly enabling the Kconfig option. >>> Maybe use 'default SND_SOC_SOF_AMD_TOPLEVEL' then? >> Didn't get your point. >> >> Even with the current patch, if we select 'SOUNDWIRE_AMD' flag explicitly >> AMD ACP63 SOF driver Kconfig option is not visible for user configuration. >> This results in ACP63 SOF driver won't be built at all. > I don't understand what you mean here. What I see > in linux-next both with and without my patch is > > config SND_SOC_SOF_AMD_ACP63 > tristate "SOF support for ACP6.3 platform" > depends on SND_SOC_SOF_PCI > > so it clearly should be visible as long as SND_SOC_SOF_PCI > is enabled. > > There is still a problem that SND_SOC_SOF_AMD_TOPLEVEL > can't use my "depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD" > trick if SOUNDWIRE_AMD in turn uses > "default SND_SOC_SOF_AMD_TOPLEVEL", but I don't think you > meant that, right? Yes, you are correct. > >>> I don't think copying the mistake from the intel driver >>> is helpful, though I agree it would be nice to be consistent >>> between them. >>> >>> As a general rule, you should not have a Kconfig symbol that >>> is both user visible and also selected by the drivers that >>> depend on it. >>> >>> To avoid the dependency problems from coming back and keep >>> the complexity to a minimum, I think there are two logical >>> ways to handle soundwire: >>> >>> a) keep the current drivers/soundwire/Kconfig contents and >>> change all the 'select SOUNDWIRE_foo' to 'depends on'. >> Current patch already using 'depends on SOUNDWIRE_AMD" for >> SND_SOC_SOF_AMD_SOUNDWIRE Kconfig option. > Correct, because this is the Kconfig option that actually > controls whether sound/soc/sof/amd/acp-common.c calls into > the soundwire-amd module. > >> Still we couldn't see SND_SOC_SOF_AMD_ACP63 Kconfig option >> is enabled. > I need more information here. Do you have additional > patches on top of what is in today's linux-next? > I have it enabled on my build here. Sorry, it's my bad. My local patches created the problem. Validated patch on our side. It's working fine. > > Arnd
On 19/02/24 15:08, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The soundwire-amd driver has a bit of a layering violation requiring > the SOF driver to directly call into its exported symbols rather than > through an abstraction. > > The SND_SOC_SOF_AMD_SOUNDWIRE Kconfig symbol tries to deal with the > dependency by selecting SOUNDWIRE_AMD in a complicated set of conditions, > but gets it wrong for a configuration involving SND_SOC_SOF_AMD_COMMON=y, > SND_SOC_SOF_AMD_ACP63=m, and SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=m > SOUNDWIRE_AMD=m, which results in a link failure: > > ld.lld: error: undefined symbol: sdw_amd_get_slave_info >>>> referenced by acp-common.c > ld.lld: error: undefined symbol: amd_sdw_scan_controller > ld.lld: error: undefined symbol: sdw_amd_probe > ld.lld: error: undefined symbol: sdw_amd_exit >>>> referenced by acp.c >>>> sound/soc/sof/amd/acp.o:(amd_sof_acp_remove) in archive vmlinux.a > In essence, the SND_SOC_SOF_AMD_COMMON option cannot be built-in when > trying to link against a modular SOUNDWIRE_AMD driver. > > Since CONFIG_SOUNDWIRE_AMD is a user-visible option, it really should > never be selected by another driver in the first place, so replace the > extra complexity with a normal Kconfig dependency in SND_SOC_SOF_AMD_SOUNDWIRE, > plus a top-level check that forbids any of the AMD SOF drivers from being > built-in with CONFIG_SOUNDWIRE_AMD=m. > > In normal configs, they should all either be built-in or all loadable > modules anyway, so this simplification does not limit any real usecases. Tested-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> > > Fixes: d948218424bf ("ASoC: SOF: amd: add code for invoking soundwire manager helper functions") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > sound/soc/sof/amd/Kconfig | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/sof/amd/Kconfig b/sound/soc/sof/amd/Kconfig > index c3bbe6c70fb2..2729c6eb3feb 100644 > --- a/sound/soc/sof/amd/Kconfig > +++ b/sound/soc/sof/amd/Kconfig > @@ -6,6 +6,7 @@ > > config SND_SOC_SOF_AMD_TOPLEVEL > tristate "SOF support for AMD audio DSPs" > + depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD > depends on X86 || COMPILE_TEST > help > This adds support for Sound Open Firmware for AMD platforms. > @@ -62,15 +63,14 @@ config SND_SOC_SOF_ACP_PROBES > > config SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE > tristate > - select SOUNDWIRE_AMD if SND_SOC_SOF_AMD_SOUNDWIRE != n > select SND_AMD_SOUNDWIRE_ACPI if ACPI > > config SND_SOC_SOF_AMD_SOUNDWIRE > tristate "SOF support for SoundWire based AMD platforms" > default SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE > depends on SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE > - depends on ACPI && SOUNDWIRE > - depends on !(SOUNDWIRE=m && SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=y) > + depends on ACPI > + depends on SOUNDWIRE_AMD > help > This adds support for SoundWire with Sound Open Firmware > for AMD platforms.
On Mon, 19 Feb 2024 10:38:45 +0100, Arnd Bergmann wrote: > The soundwire-amd driver has a bit of a layering violation requiring > the SOF driver to directly call into its exported symbols rather than > through an abstraction. > > The SND_SOC_SOF_AMD_SOUNDWIRE Kconfig symbol tries to deal with the > dependency by selecting SOUNDWIRE_AMD in a complicated set of conditions, > but gets it wrong for a configuration involving SND_SOC_SOF_AMD_COMMON=y, > SND_SOC_SOF_AMD_ACP63=m, and SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=m > SOUNDWIRE_AMD=m, which results in a link failure: > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: amd: fix soundwire dependencies commit: a3d543b9e6599fbbb9efc1876919627960c5e97a All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/sound/soc/sof/amd/Kconfig b/sound/soc/sof/amd/Kconfig index c3bbe6c70fb2..2729c6eb3feb 100644 --- a/sound/soc/sof/amd/Kconfig +++ b/sound/soc/sof/amd/Kconfig @@ -6,6 +6,7 @@ config SND_SOC_SOF_AMD_TOPLEVEL tristate "SOF support for AMD audio DSPs" + depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD depends on X86 || COMPILE_TEST help This adds support for Sound Open Firmware for AMD platforms. @@ -62,15 +63,14 @@ config SND_SOC_SOF_ACP_PROBES config SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE tristate - select SOUNDWIRE_AMD if SND_SOC_SOF_AMD_SOUNDWIRE != n select SND_AMD_SOUNDWIRE_ACPI if ACPI config SND_SOC_SOF_AMD_SOUNDWIRE tristate "SOF support for SoundWire based AMD platforms" default SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE depends on SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE - depends on ACPI && SOUNDWIRE - depends on !(SOUNDWIRE=m && SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=y) + depends on ACPI + depends on SOUNDWIRE_AMD help This adds support for SoundWire with Sound Open Firmware for AMD platforms.