Page MenuHomeFreeBSD
Differential D47199

sound: Untangle dsp_cdevs[] and dsp_unit2name() confusion
ClosedPublic
Actions

Authored by christos on Oct 19 2024, 5:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 14, 12:26 PM
Unknown Object (File)
Tue, Dec 10, 2:46 PM
Unknown Object (File)
Thu, Dec 5, 8:12 AM
Unknown Object (File)
Thu, Dec 5, 6:46 AM
Unknown Object (File)
Wed, Dec 4, 4:48 PM
Unknown Object (File)
Fri, Nov 29, 9:23 PM
Unknown Object (File)
Nov 24 2024, 3:25 PM
Unknown Object (File)
Nov 21 2024, 6:46 AM
View All 39 Files
Subscribers
des
imp

Details

Reviewers
markj
emaste
dev_submerge.ch
Commits
rGf3449b1a2115: sound: Untangle dsp_cdevs[] and dsp_unit2name() confusion
rG802c78f5194e: sound: Untangle dsp_cdevs[] and dsp_unit2name() confusion
Summary

Before de8c0d15a64fa ("sound: Get rid of snd_clone and use
DEVFS_CDEVPRIV(9)"), sound(4) would create one device for each allocated
channel. The device names would be chosen from dsp_cdevs[], and created
with dsp_unit2name(). Additionally, dsp_cdevs[] was also used to match
these devices names, as well as OSSv4 aliases in dsp_clone().

Since sound(4) does not create separate devices for each channel
anymore, the meaning and use dsp_cdevs[] has changed. Part of it no
longer corresponds to devices at all, but instead is used to create
channel names, and another part is used to match only OSSv4 aliases in
dsp_clone().

To address this confusion, separate dsp_cdevs[] into a dsp_aliases[]
array, and move dsp_unit2name() to pcm/channel.c and rename it to
chn_mkname().

While here, get rid of the SND_DEV_DSPHW_* channel types, and simply use
the existing PCMDIR_* constants as the channel types. There is no need
to duplicate the same meaning twice.

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60145
Build 57029: arc lint + arc unit

Event Timeline

christos created this revision.Oct 19 2024, 5:06 PM
Herald added a subscriber: imp. · View Herald TranscriptOct 19 2024, 5:06 PM
christos requested review of this revision.Oct 19 2024, 5:06 PM
Harbormaster completed remote builds in B60096: Diff 145164.Oct 19 2024, 5:06 PM
christos added a parent revision: D47198: sound: Change PCMDIR_* numbering.Oct 19 2024, 5:06 PM
christos updated this revision to Diff 145165.Oct 19 2024, 5:12 PM

Make some cases fallthrough in chn_init() switch statement.

Harbormaster completed remote builds in B60097: Diff 145165.Oct 19 2024, 5:13 PM
dev_submerge.ch added a comment.Oct 20 2024, 9:13 PM

This simplifies the code quite a bit, nice! Some nitpicks in the inline comments.

sys/dev/sound/pcm/channel.c
1178–1186

Is there still a reason to keep the chn_names array like this when all access goes through chn_mkname()? Could be mapped in a switch maybe?

1221–1223

If I read style(9) correctly, there should be fallthrough comment:

Elements in a switch statement that cascade should have a FALLTHROUGH comment.

1224–1227

Same here.

sys/dev/sound/pcm/dsp.c
2233–2234

Is there a need to create the channel name here? I don't see devname or buf used in any way, except for a check that it's not NULL. Like this it's just a costly check of the channel type, no?

christos edited the summary of this revision. (Show Details)Oct 21 2024, 6:48 PM
christos updated this revision to Diff 145307.Oct 21 2024, 6:49 PM
christos marked 4 inline comments as done.

Address Florian's comments:

  • Get rid of chn_names[] and integrate it into a switch statement in chn_mkname().
  • Refactor dsp_oss_engineinfo() to not use chn_mkname(). Should give us some performance boost as well.
Harbormaster completed remote builds in B60129: Diff 145307.Oct 21 2024, 6:49 PM
christos updated this revision to Diff 145308.Oct 21 2024, 6:51 PM

s/rec/record/ in chn_mkname().

Harbormaster completed remote builds in B60130: Diff 145308.Oct 21 2024, 6:51 PM
dev_submerge.ch added inline comments.Oct 22 2024, 11:25 AM
sys/dev/sound/pcm/channel.c
1188

IMHO these would be more readable as "dsp%d.play.%d" quasi template strings. Not really important though.

sys/dev/sound/pcm/dsp.c
2242–2246

Shouldn't we continue to iterate through the pcm units here?

christos added inline comments.Oct 22 2024, 11:45 AM
sys/dev/sound/pcm/channel.c
1188

This way we'd duplicate dsp%d...%d in all the switch cases, though.

sys/dev/sound/pcm/dsp.c
2242–2246

Yes, I updated the diff in my local tree yesterday, but forgot to update it here as well.

christos updated this revision to Diff 145343.Oct 22 2024, 1:26 PM

Remove redundant chn_mkname() call in vchan_passthrough().

Harbormaster completed remote builds in B60145: Diff 145343.Oct 22 2024, 1:26 PM
christos marked an inline comment as done.Oct 22 2024, 1:26 PM
dev_submerge.ch added inline comments.Oct 22 2024, 2:13 PM
sys/dev/sound/pcm/channel.c
1188

Alternatively, move the separating dots into the printf literal: "dsp%d.%s.%d"

sys/dev/sound/pcm/dsp.c
2242–2246

Ok, but don't we also have to unlock the CHN_LOCK(ch); here?

dev_submerge.ch added inline comments.Oct 22 2024, 2:21 PM
sys/dev/sound/pcm/dsp.c
2242–2246

My bad, forget about last comment - the question is whether ch is set to NULL when the channel is not found?

christos marked 2 inline comments as done.Oct 22 2024, 2:28 PM
christos added inline comments.
sys/dev/sound/pcm/channel.c
1188

I agree with this.

sys/dev/sound/pcm/dsp.c
2242–2246

It is.

christos updated this revision to Diff 145348.Oct 22 2024, 2:33 PM
christos marked 3 inline comments as done.

Address Florian's comments for chn_mkname().

Harbormaster completed remote builds in B60149: Diff 145348.Oct 22 2024, 2:33 PM
dev_submerge.ch added inline comments.Oct 22 2024, 2:55 PM
sys/dev/sound/pcm/dsp.c
2242–2246

Indeed, it's set to NULL by the SLIST implementation. Is that behavior guaranteed or documented somewhere? Don't know about FreeBSD src customs, but in other projects I wouldn't rely on that if it's not part of the documented interface.

christos added a subscriber: des.Oct 22 2024, 3:02 PM
christos added inline comments.
sys/dev/sound/pcm/dsp.c
2242–2246

I see that it's documented for TAILQ_FOREACH, but I am not sure why it's not for the other queue(3) types.
@des @imp Any info on this?

emaste added inline comments.Oct 22 2024, 4:13 PM
sys/dev/sound/pcm/channel.c
1221–1223

I guess style(9) should be more explicit - FALLTHROUGH is needed when it otherwise looks like a break is missing, but not for multiple cases that share the same body. Note that style(9)'s FALLTHROUGH example has code for case 'a': and the FALLTHROUGH indicates that it's intentional to fall through to the case 'b': code. So leave these ones out.

Submitted D47242 for a style.9 update.

christos updated this revision to Diff 145356.Oct 22 2024, 4:35 PM
christos marked an inline comment as done.

Remove "FALLTHROUGH".

Harbormaster completed remote builds in B60153: Diff 145356.Oct 22 2024, 4:35 PM
dev_submerge.ch added a comment.Oct 23 2024, 12:38 AM

@emaste: Thanks for the clarification on style(9)'s FALLTHROUGH part.

@christos: Sorry for the misleading comment.

christos added a comment.Oct 23 2024, 3:53 PM

So is the patch good to go?

dev_submerge.ch added a comment.Oct 24 2024, 10:58 AM
In D47199#1077635, @christos wrote:

So is the patch good to go?

I'm still undecided on the CHN_FOREACH() postcondition of ch == NULL. Tried to find precedence, but I only see implementations that set additional variables like found = true;. Although unlikely to change, this may result in a really hard to spot error when the postcondition doesn't hold.

christos added a comment.Oct 24 2024, 10:59 AM
In D47199#1077896, @dev_submerge.ch wrote:
In D47199#1077635, @christos wrote:

So is the patch good to go?

I'm still undecided on the CHN_FOREACH() postcondition of ch == NULL. Tried to find precedence, but I only see implementations that set additional variables like found = true;. Although unlikely to change, this may result in a really hard to spot error when the postcondition doesn't hold.

Both imp@ and markj@ confirmed that this behavior is more or less guaranteed, and plenty of code relies on that assumption already.

dev_submerge.ch accepted this revision.Oct 24 2024, 11:25 AM
This revision is now accepted and ready to land.Oct 24 2024, 11:25 AM
Closed by commit rG802c78f5194e: sound: Untangle dsp_cdevs[] and dsp_unit2name() confusion (authored by christos). · Explain WhyOct 24 2024, 11:38 AM
This revision was automatically updated to reflect the committed changes.
christos added a commit: rG802c78f5194e: sound: Untangle dsp_cdevs[] and dsp_unit2name() confusion.
christos added a commit: rGf3449b1a2115: sound: Untangle dsp_cdevs[] and dsp_unit2name() confusion.Oct 27 2024, 4:33 PM

Revision Contents
Changeset List

PathSize
sys/
dev/
sound/
pcm/
1 line
73 lines
1 line
291 lines
4 lines
11 lines

Diff 145343

View Options

sys/dev/sound/pcm/channel.h

Loading...
View Options

sys/dev/sound/pcm/channel.c

Loading...
View Options

sys/dev/sound/pcm/dsp.h

Loading...
View Options

sys/dev/sound/pcm/dsp.c

Loading...
View Options

sys/dev/sound/pcm/sound.h

Loading...
View Options

sys/dev/sound/pcm/vchan.c

Loading...