sound: Refactor sndstat_get_caps()
ClosedPublic
Actions

Authored by christos on Jul 4 2024, 4:00 PM.

Details

Reviewers

markj
emaste
dev_submerge.ch

Commits

rGecb0a2ed2f40: sound: Refactor sndstat_get_caps()
rG9d8b93bc9cce: sound: Refactor sndstat_get_caps()

Summary

The current implementation of sndstat_get_caps() does not work properly
when VCHANs are enabled, as it skips all information about physical
channels, and also assigns the min/max rates and channels to same
values, which is usually not the case. A device either supports any
sample rate within the [feeder_rate_min, feeder_rate_max] range, or
[hw_rate_min, hw_rate_max] range when the device is opened in exclusive
or bitperfect mode. The number of channels can also vary and is not
always the same for both min and max.

Refactor the whole function to resemble the way we handle fetching of
these values in dsp_oss_audioinfo() and dsp_oss_engineinfo().

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

christos created this revision.Jul 4 2024, 4:00 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 4 2024, 4:00 PM

christos requested review of this revision.Jul 4 2024, 4:00 PM

Harbormaster completed remote builds in B58497: Diff 140552.Jul 4 2024, 4:00 PM

christos mentioned this in D45862: sound: Fix min/max rate for SNDCTL_AUDIOINFO and SNDCTL_ENGINEINFO.Jul 4 2024, 4:00 PM

Is there a test / application available to run this? I'd like to check the return values for my setup.

sys/dev/sound/pcm/sndstat.c
369–372	Why remove this? It doesn't hurt to vet the return values for robustness, we're relying on a check in the caller for the channel list to be non-empty.

In D45872#1045945, @dev_submerge.ch wrote:

Is there a test / application available to run this? I'd like to check the return values for my setup.

I wrote this ugly test program a while ago when I was writing bbca3a75bb41. I think it prints everything sndstat provides. Make sure you have all related patches applied and that you compile the program with -lnv.

P641

sys/dev/sound/pcm/sndstat.c
369–372	I will keep it. And also add this check in dsp_oss_audioinfo().

christos marked an inline comment as done.Jul 5 2024, 2:16 PM

Restore safety check.

Harbormaster completed remote builds in B58505: Diff 140566.Jul 5 2024, 2:19 PM

I get the following panic, but I may be missing some commit - any prerequisites for this?

KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe008eaa2900
vpanic() at vpanic+0x13f/frame 0xfffffe008eaa2a30
panic() at panic+0x43/frame 0xfffffe008eaa2a90
__mtx_assert() at __mtx_assert+0xa9/frame 0xfffffe008eaa2aa0
chn_getvolume_matrix() at chn_getvolume_matrix+0x45/frame 0xfffffe008eaa2ad0
sndstat_get_devs() at sndstat_get_devs+0x6c2/frame 0xfffffe008eaa2bd0
sndstat_ioctl() at sndstat_ioctl+0xc2/frame 0xfffffe008eaa2c00
devfs_ioctl() at devfs_ioctl+0xd1/frame 0xfffffe008eaa2c50
vn_ioctl() at vn_ioctl+0xbc/frame 0xfffffe008eaa2cc0
devfs_ioctl_f() at devfs_ioctl_f+0x1e/frame 0xfffffe008eaa2ce0
kern_ioctl() at kern_ioctl+0x286/frame 0xfffffe008eaa2d40
sys_ioctl() at sys_ioctl+0x12d/frame 0xfffffe008eaa2e00
amd64_syscall() at amd64_syscall+0x158/frame 0xfffffe008eaa2f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe008eaa2f30
--- syscall (54, FreeBSD ELF64, ioctl), rip = 0x82293c8fa, rsp = 0x8210edf68, rbp = 0x8210edfd0 ---
KDB: enter: panic

In D45872#1046113, @dev_submerge.ch wrote:

I get the following panic, but I may be missing some commit - any prerequisites for this?

KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe008eaa2900
vpanic() at vpanic+0x13f/frame 0xfffffe008eaa2a30
panic() at panic+0x43/frame 0xfffffe008eaa2a90
__mtx_assert() at __mtx_assert+0xa9/frame 0xfffffe008eaa2aa0
chn_getvolume_matrix() at chn_getvolume_matrix+0x45/frame 0xfffffe008eaa2ad0
sndstat_get_devs() at sndstat_get_devs+0x6c2/frame 0xfffffe008eaa2bd0
sndstat_ioctl() at sndstat_ioctl+0xc2/frame 0xfffffe008eaa2c00
devfs_ioctl() at devfs_ioctl+0xd1/frame 0xfffffe008eaa2c50
vn_ioctl() at vn_ioctl+0xbc/frame 0xfffffe008eaa2cc0
devfs_ioctl_f() at devfs_ioctl_f+0x1e/frame 0xfffffe008eaa2ce0
kern_ioctl() at kern_ioctl+0x286/frame 0xfffffe008eaa2d40
sys_ioctl() at sys_ioctl+0x12d/frame 0xfffffe008eaa2e00
amd64_syscall() at amd64_syscall+0x158/frame 0xfffffe008eaa2f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe008eaa2f30
--- syscall (54, FreeBSD ELF64, ioctl), rip = 0x82293c8fa, rsp = 0x8210edf68, rbp = 0x8210edfd0 ---
KDB: enter: panic

Can you try testing this on main with (I know this is boring) all the open patches here applied? I have you as a reviewer in all of them so they should be easy to find and apply.

In D45872#1046199, @christos wrote:

Can you try testing this on main with (I know this is boring) all the open patches here applied? I have you as a reviewer in all of them so they should be easy to find and apply.

Still the same panic, I already had most of your patches included. I'll try with pure main branch to see if it's a regression or something previously overlooked.

Ok, it's not a regression - same panic on the main branch as of yesterday:

KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe008ea02900
vpanic() at vpanic+0x13f/frame 0xfffffe008ea02a30
panic() at panic+0x43/frame 0xfffffe008ea02a90
__mtx_assert() at __mtx_assert+0xa9/frame 0xfffffe008ea02aa0
chn_getvolume_matrix() at chn_getvolume_matrix+0x45/frame 0xfffffe008ea02ad0
sndstat_get_devs() at sndstat_get_devs+0x6c2/frame 0xfffffe008ea02bd0
sndstat_ioctl() at sndstat_ioctl+0xc2/frame 0xfffffe008ea02c00
devfs_ioctl() at devfs_ioctl+0xd1/frame 0xfffffe008ea02c50
vn_ioctl() at vn_ioctl+0xbc/frame 0xfffffe008ea02cc0
devfs_ioctl_f() at devfs_ioctl_f+0x1e/frame 0xfffffe008ea02ce0
kern_ioctl() at kern_ioctl+0x286/frame 0xfffffe008ea02d40
sys_ioctl() at sys_ioctl+0x12d/frame 0xfffffe008ea02e00
amd64_syscall() at amd64_syscall+0x158/frame 0xfffffe008ea02f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe008ea02f30
--- syscall (54, FreeBSD ELF64, ioctl), rip = 0x8219ff8fa, rsp = 0x8207224c8, rbp = 0x820722530 ---
KDB: enter: panic
[ thread pid 979 tid 100222 ]
Stopped at      kdb_enter+0x33: movq    $0,0x105f322(%rip)

Not doing anything fancy, just have one uaudio device plugged in and then run your nvlist test program. If I remove the uaudio device (no audio devices), the test program fails with

/usr/src/sys/contrib/libnv/nvlist.c:379: Element 'dsps' of type NVLIST ARRAY doesn't exist.

In D45872#1046315, @dev_submerge.ch wrote:

Ok, it's not a regression - same panic on the main branch as of yesterday:

KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe008ea02900
vpanic() at vpanic+0x13f/frame 0xfffffe008ea02a30
panic() at panic+0x43/frame 0xfffffe008ea02a90
__mtx_assert() at __mtx_assert+0xa9/frame 0xfffffe008ea02aa0
chn_getvolume_matrix() at chn_getvolume_matrix+0x45/frame 0xfffffe008ea02ad0
sndstat_get_devs() at sndstat_get_devs+0x6c2/frame 0xfffffe008ea02bd0
sndstat_ioctl() at sndstat_ioctl+0xc2/frame 0xfffffe008ea02c00
devfs_ioctl() at devfs_ioctl+0xd1/frame 0xfffffe008ea02c50
vn_ioctl() at vn_ioctl+0xbc/frame 0xfffffe008ea02cc0
devfs_ioctl_f() at devfs_ioctl_f+0x1e/frame 0xfffffe008ea02ce0
kern_ioctl() at kern_ioctl+0x286/frame 0xfffffe008ea02d40
sys_ioctl() at sys_ioctl+0x12d/frame 0xfffffe008ea02e00
amd64_syscall() at amd64_syscall+0x158/frame 0xfffffe008ea02f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe008ea02f30
--- syscall (54, FreeBSD ELF64, ioctl), rip = 0x8219ff8fa, rsp = 0x8207224c8, rbp = 0x820722530 ---
KDB: enter: panic
[ thread pid 979 tid 100222 ]
Stopped at      kdb_enter+0x33: movq    $0,0x105f322(%rip)

Not doing anything fancy, just have one uaudio device plugged in and then run your nvlist test program. If I remove the uaudio device (no audio devices), the test program fails with

/usr/src/sys/contrib/libnv/nvlist.c:379: Element 'dsps' of type NVLIST ARRAY doesn't exist.

The latter error is not related to the panic, right? This just causes an abort trap in the test program because nvlist_get_nvlist_array(nvl, SNDST_DSPS, &nitems); fails. I could handle this but this is a draft program so I didn't care to.

I think your panic is happening here in sndstat_build_sound4_nvlist():

		nvlist_add_number(cdi, SNDST_DSPS_SOUND4_CHAN_LEFTVOL,
		    CHN_GETVOLUME(c, SND_VOL_C_PCM, SND_CHN_T_FL));
		nvlist_add_number(cdi, SNDST_DSPS_SOUND4_CHAN_RIGHTVOL,
		    CHN_GETVOLUME(c, SND_VOL_C_PCM, SND_CHN_T_FR));

According to the panic message it seems like you're hitting the lock assertion in chn_getvolume_matrix() (do you have INVARIANTS or SND_DIAGNOSTIC enabled?), and it's apparently because the channel is not being locked in the sndstat_build_sound4_nvlist() loop.

Can you test whether this fixes the issue?

diff --git a/sys/dev/sound/pcm/sndstat.c b/sys/dev/sound/pcm/sndstat.c
index 5b770810d19b..f32bcc9aafc3 100644
--- a/sys/dev/sound/pcm/sndstat.c
+++ b/sys/dev/sound/pcm/sndstat.c
@@ -456,6 +456,8 @@ sndstat_build_sound4_nvlist(struct snddev_info *d, nvlist_t **dip)
                        goto done;
                }

+               CHN_LOCK(c);
+
                nvlist_add_string(cdi, SNDST_DSPS_SOUND4_CHAN_NAME, c->name);
                nvlist_add_string(cdi, SNDST_DSPS_SOUND4_CHAN_PARENTCHAN,
                    c->parentchannel != NULL ? c->parentchannel->name : "");
@@ -537,6 +539,8 @@ sndstat_build_sound4_nvlist(struct snddev_info *d, nvlist_t **dip)
                sbuf_printf(&sb, "%s]",
                    (c->direction == PCMDIR_REC) ? "userland" : "hardware");

+               CHN_UNLOCK(c);
+
                sbuf_finish(&sb);
                nvlist_add_string(cdi, SNDST_DSPS_SOUND4_CHAN_FEEDERCHAIN,
                    sbuf_data(&sb));