ASoC: Intel: Fix RT5650 SSP lookup

[cujomalainey]

8efcd48 ("ASoC: Intel: sof_rt5682: use common module for sof_card_private initialization") migrated the pin assignment in the context struct up to soc-acpi-intel-ssp-common.c. This uses a lookup table to see if a device has a amp/codec before assigning the pin. The issue here arises when combination parts that serve both (with 2 ports) are used.

sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:1f.3/adl_rt5682_def/SSP0-Codec'
CPU: 1 PID: 2079 Comm: udevd Tainted: G U 6.6.36-03391-g744739e00023 #1 3be1a2880a0970f65545a957db7d08ef4b3e2c0d
Hardware name: Google Anraggar/Anraggar, BIOS Google_Anraggar.15217.552.0 05/07/2024
Call Trace:

dump_stack_lvl+0x69/0xa0
sysfs_warn_dup+0x5b/0x70
sysfs_create_dir_ns+0xb0/0x100
kobject_add_internal+0x133/0x3c0
kobject_add+0x66/0xb0
? device_add+0x65/0x780
device_add+0x164/0x780
snd_soc_add_pcm_runtimes+0x2fa/0x800
snd_soc_bind_card+0x35e/0xc20
devm_snd_soc_register_card+0x48/0x90
platform_probe+0x7b/0xb0
really_probe+0xf7/0x2a0
__driver_probe_device+0x7e/0x120
driver_probe_device+0x24/0x80
? __pfx___driver_attach+0x10/0x10
__driver_attach+0x9e/0xf0
? __pfx___driver_attach+0x10/0x10
bus_for_each_dev+0x8f/0xe0
bus_add_driver+0xfc/0x210
driver_register+0x63/0x100
? __pfx_init_module+0x10/0x10 [snd_soc_sof_rt5682 a0313f2366301beadb955303f0658b8dfe75b8d4]
do_one_initcall+0xf1/0x320
? kernfs_xattr_get+0x39/0x70
? selinux_kernfs_init_security+0x5d/0x1f0
? idr_alloc_cyclic+0xb8/0x130
? security_kernfs_init_security+0x30/0x50
? __kernfs_new_node+0x190/0x210
? sysfs_create_dir_ns+0x90/0x100
? kernfs_activate+0x51/0x70
? kernfs_add_one+0x10d/0x150
? sysvec_apic_timer_interrupt+0x17/0x80
? asm_sysvec_apic_timer_interrupt+0x16/0x20
? __pfx_bpf_lsm_kernfs_init_security+0x10/0x10
? __kmem_cache_alloc_node+0x134/0x220
? do_init_module+0x27/0x310
? do_init_module+0x27/0x310
? kmalloc_trace+0x2f/0xa0
do_init_module+0x65/0x310
load_module+0x1b25/0x1d80
? __free_pages+0x260/0x8f0
__se_sys_finit_module+0x228/0x300
do_syscall_64+0x60/0x90
? fpregs_restore_userregs+0x50/0xc0
? exit_to_user_mode_prepare+0x107/0x110
? do_syscall_64+0x6f/0x90
? exit_to_user_mode_prepare+0x107/0x110
entry_SYSCALL_64_after_hwframe+0x73/0xdd
RIP: 0033:0x7cbaa445ad39
Code: 5b 41 5c 5d c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d af 40 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffc3eebb0a8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
RAX: ffffffffffffffda RBX: 00005bbdf150a560 RCX: 00007cbaa445ad39
RDX: 0000000000000004 RSI: 00007cbaa4561186 RDI: 0000000000000013
RBP: 00007ffc3eebb100 R08: 00005bbdf1470340 R09: 0000000000000000
R10: 00005bbdf146fa30 R11: 0000000000000246 R12: 0000000000020000
R13: 00000000fffffffe R14: 0000000000000004 R15: 00007cbaa4561186

kobject: kobject_add_internal failed for SSP0-Codec with -EEXIST, don't try to register things with the same name in the same directory.

The issue is that the ALC5650 was only defined in the codec table and not the amp table which left the pin unassigned but the dai link was still created by the machine driver.

This fix has the side effect of changing the target topology for devices from sof-{chipset}-rt5650.tplg to sof-{chipset}-rt5650-rt5650.tplg

Fixes: 8efcd48 ("ASoC: Intel: sof_rt5682: use common module for sof_card_private initialization")

[cujomalainey]

An alternative fix would be to manually set the ssp in sof_rt5682.c but that is not portable, but would avoid the issue issue with the tplg.

[cujomalainey]

An alternative fix would be to manually set the ssp in sof_rt5682.c but that is not portable, but would avoid the issue issue with the tplg.

Or to add a check if CODEC == ALC5650 then also set the amp pin in soc-acpi-intel-ssp-common.c. I'll let you guys decide what you want.

[brentlu]

Patch tested on Anraggar:
#5101

Sorry the SSP port number of ALC5650's speaker path is not handled well.

[bardliao]

@brentlu @cujomalainey People might use rt5650 + external amp. In that case, ctx->amp_type will be the external amp. IOW, we should set ctx->amp_type = CODEC_RT5650; only when there is no other amp present. So, we could add rt5650 at the last item of the amps[] array. And add a comment before the rt5650 item to describe why we need to add it last. And we could add other similar codecs in the future. Does it make sense?

[brentlu]

@brentlu @cujomalainey People might use rt5650 + external amp. In that case, ctx->amp_type will be the external amp. IOW, we should set ctx->amp_type = CODEC_RT5650; only when there is no other amp present. So, we could add rt5650 at the last item of the amps[] array. And add a comment before the rt5650 item to describe why we need to add it last. And we could add other similar codecs in the future. Does it make sense?

If we add rt5650 to amplifier list, the topology name composed in hda_machine_select() will become something like "sof-rt5650-rt5650.tplg"

[cujomalainey]

@brentlu @cujomalainey People might use rt5650 + external amp. In that case, ctx->amp_type will be the external amp. IOW, we should set ctx->amp_type = CODEC_RT5650; only when there is no other amp present. So, we could add rt5650 at the last item of the amps[] array. And add a comment before the rt5650 item to describe why we need to add it last. And we could add other similar codecs in the future. Does it make sense?

If we add rt5650 to amplifier list, the topology name composed in hda_machine_select() will become something like "sof-rt5650-rt5650.tplg"

Yes this issue is mentioned in my commit message.

[cujomalainey]

@brentlu @cujomalainey People might use rt5650 + external amp. In that case, ctx->amp_type will be the external amp. IOW, we should set ctx->amp_type = CODEC_RT5650; only when there is no other amp present. So, we could add rt5650 at the last item of the amps[] array. And add a comment before the rt5650 item to describe why we need to add it last. And we could add other similar codecs in the future. Does it make sense?

If we add rt5650 to amplifier list, the topology name composed in hda_machine_select() will become something like "sof-rt5650-rt5650.tplg"

Yes this issue is mentioned in my commit message.

I might be able to add a case for the string where if amp == codec only add the part name once

[cujomalainey]

Added comment and patched the tplg suffix code

[cujomalainey]

@plbossart @sathya-nujella ping this is a pretty bad break on our end

[brentlu]

@plbossart @sathya-nujella ping this is a pretty bad break on our end

A temporal fix has been merged to Chrome-v6.6 on Wednesday:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5691052

[plbossart]

@brentlu are you ok with the suggested fix?

[plbossart]

@cujomalainey you may want to trim the trace log in the commit mexxage , Mark Brown is going to tell you it's too verbose.

[brentlu]

@brentlu are you ok with the suggested fix?

LGTM, thanks

[cujomalainey]

Trimmed down the trace, if there are more issues discovered tomorrow please feel free to fix them yourself as I am OOO starting tomorrow for the rest of the week.