ASoC: SOF: amd: fix soundwire dependencies

  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.

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

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