speaker(4): drop NEEDGIANT

[knz_thaumogen.net]

Summary

(NB: I also uploaded this change to github PR 1922 because I wasn't sure which one is being used more these days. That PR contains more compact commit messages.)

The calls to disable_intr() / enable_intr() in spkr.c were leftover from previous refactorings and unnecessary for correctness, so this PR removes them.

My detailed analysis of why this change is correct can be found here:

spkr.md9 KBDownload

The main related commit referenced in the analysis is this 2008 change by phk: e465985 - where a mutex was added to clock.c and the disable_intr/enable_intr pair could have been removed, but wasn't.

Motivation: I am a fervent spkr(4) user and would like it to persist beyond FreeBSD 16, so we needed to do something about its use of the GIANT mutex.

[knz_thaumogen.net]

My previous upload was incomplete.

[kevans]

I don't think this is actually sufficient, but I don't think fixing it will be all that hard. sprkopen currently uses the fact that it's locked by Giant, so you'll probably want one spkr mutex to be taken in spkropen() and spkrclose() to be sure it's only opened by a single thread (and not leaking an allocation if spkr_inbuf gets clobbered`).

spkrwrite / spkrioctl might debatably need their own synchronization to provide some ordering in case something's trying to be funky and dup the fd and use it from multiple threads, but I would rather suspect that doesn't happen much / at all in practice?

sys/dev/speaker/spkr.c

30

We generally just omit it at the point that we're not setting anything

[knz_thaumogen.net]

Make spkropen thread-safe

[knz_thaumogen.net]

Also add a mutex for ioctl/write

[knz_thaumogen.net]

forgot an include in last revision

[kevans]

I am quite confused about what happened here. Can you explain the connection between my previous note and the changes made?

[knz_thaumogen.net]

sprkopen currently uses the fact that it's locked by Giant, so you'll probably want one spkr mutex to be taken in spkropen() and spkrclose() to be sure it's only opened by a single thread

-> changed spkr_active to spkr_dev_busy, and using an atomic compare-and-set to obtain the "only one open at a time" semantics that you recommended.

spkrwrite / spkrioctl might debatably need their own synchronization to provide some ordering in case something's trying to be funky and dup the fd and use it from multiple threads

-> added a separate mutex spkr_lock to serialize calls to write/ioctl.

[kevans]

Ahh, sorry, I got confused with the missing context- can you reupload this with either git-arc or -U99999, please? It's immensely useful to be able to scroll back up and confirm where different hunks are applying

[knz_thaumogen.net]

full diff with context

[knz_thaumogen.net]

Ahh, sorry, I got confused with the missing context- can you reupload this with either git-arc or -U99999, please?

Done. But IMHO I find this workflow tremendously impractical. With github I can do 1) fine-grained commits, with separate commit per logical change and 2) git push -f to update the PR.

Here all this gets mashed up together and I need to copy-paste the diff manually in a HTML form... 😭

[knz_thaumogen.net]

forgot a return path

[kevans]

In D54168#1244291, @knz_thaumogen.net wrote:

Ahh, sorry, I got confused with the missing context- can you reupload this with either git-arc or -U99999, please?

Done. But IMHO I find this workflow tremendously impractical. With github I can do 1) fine-grained commits, with separate commit per logical change and 2) git push -f to update the PR.

Here all this gets mashed up together and I need to copy-paste the diff manually in a HTML form... 😭

Yeah, I know. =( git-arc makes phab considerably easier to cope with for the latter point, but yeah. Let's just move to GitHub for this one.