ASoC: Intel: Fix RT5650 SSP lookup

[cujomalainey]

8efcd4864652 ("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: 8efcd4864652 ("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: https://github.com/thesofproject/linux/pull/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?

[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.