git: 5317480967bf - main - sound: Remove CHN_F_SLEEPING
- Reply: John Baldwin : "Re: git: 5317480967bf - main - sound: Remove CHN_F_SLEEPING"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Tue, 26 Nov 2024 14:48:58 UTC
The branch main has been updated by christos: URL: https://cgit.FreeBSD.org/src/commit/?id=5317480967bfc8bf678e4da3fce81bcb3f5b7836 commit 5317480967bfc8bf678e4da3fce81bcb3f5b7836 Author: Christos Margiolis <christos@FreeBSD.org> AuthorDate: 2024-11-26 14:48:36 +0000 Commit: Christos Margiolis <christos@FreeBSD.org> CommitDate: 2024-11-26 14:48:36 +0000 sound: Remove CHN_F_SLEEPING The KASSERT in chn_sleep() can be triggered if more than one thread wants to sleep on a given channel at the same time. While this is not really a common scenario, tools such as stress2, which use fork() and the child process(es) inherit the parent's FDs as a result, we can end up triggering such scenarios. Fix this by removing CHN_F_SLEEPING altogether, which is not very useful in the first place: - CHN_BROADCAST() checks cv_waiters already, so there is no need to check CHN_F_SLEEPING as well. - We can check whether cv_waiters is 0 in pcm_killchans(), instead of whether CHN_F_SLEEPING is not set. Reported by: dougm, pho (stress2) Sponsored by: The FreeBSD Foundation MFC after: 2 days Reviewed by: dev_submerge.ch, markj Differential Revision: https://reviews.freebsd.org/D47559 --- sys/dev/sound/pcm/channel.c | 13 +------------ sys/dev/sound/pcm/channel.h | 4 ++-- sys/dev/sound/pcm/sound.c | 4 ++-- 3 files changed, 5 insertions(+), 16 deletions(-) diff --git a/sys/dev/sound/pcm/channel.c b/sys/dev/sound/pcm/channel.c index 59cdb8cd07f5..f619ddd4bc35 100644 --- a/sys/dev/sound/pcm/channel.c +++ b/sys/dev/sound/pcm/channel.c @@ -309,14 +309,7 @@ chn_wakeup(struct pcm_channel *c) if (CHN_EMPTY(c, children.busy)) { if (SEL_WAITING(sndbuf_getsel(bs)) && chn_polltrigger(c)) selwakeuppri(sndbuf_getsel(bs), PRIBIO); - if (c->flags & CHN_F_SLEEPING) { - /* - * Ok, I can just panic it right here since it is - * quite obvious that we never allow multiple waiters - * from userland. I'm too generous... - */ - CHN_BROADCAST(&c->intr_cv); - } + CHN_BROADCAST(&c->intr_cv); } else { CHN_FOREACH(ch, c, children.busy) { CHN_LOCK(ch); @@ -332,15 +325,11 @@ chn_sleep(struct pcm_channel *c, int timeout) int ret; CHN_LOCKASSERT(c); - KASSERT((c->flags & CHN_F_SLEEPING) == 0, - ("%s(): entered with CHN_F_SLEEPING", __func__)); if (c->flags & CHN_F_DEAD) return (EINVAL); - c->flags |= CHN_F_SLEEPING; ret = cv_timedwait_sig(&c->intr_cv, c->lock, timeout); - c->flags &= ~CHN_F_SLEEPING; return ((c->flags & CHN_F_DEAD) ? EINVAL : ret); } diff --git a/sys/dev/sound/pcm/channel.h b/sys/dev/sound/pcm/channel.h index 79a8d35b22f7..d226adfba06b 100644 --- a/sys/dev/sound/pcm/channel.h +++ b/sys/dev/sound/pcm/channel.h @@ -354,7 +354,7 @@ enum { #define CHN_F_RUNNING 0x00000004 /* dma is running */ #define CHN_F_TRIGGERED 0x00000008 #define CHN_F_NOTRIGGER 0x00000010 -#define CHN_F_SLEEPING 0x00000020 +/* unused 0x00000020 */ #define CHN_F_NBIO 0x00000040 /* do non-blocking i/o */ #define CHN_F_MMAP 0x00000080 /* has been mmap()ed */ @@ -381,8 +381,8 @@ enum { "\002ABORTING" \ "\003RUNNING" \ "\004TRIGGERED" \ + /* \006 */ \ "\005NOTRIGGER" \ - "\006SLEEPING" \ "\007NBIO" \ "\010MMAP" \ "\011BUSY" \ diff --git a/sys/dev/sound/pcm/sound.c b/sys/dev/sound/pcm/sound.c index 4b7bcc64397f..218147b73db0 100644 --- a/sys/dev/sound/pcm/sound.c +++ b/sys/dev/sound/pcm/sound.c @@ -221,8 +221,8 @@ pcm_killchans(struct snddev_info *d) /* Make sure all channels are stopped. */ CHN_FOREACH(ch, d, channels.pcm) { CHN_LOCK(ch); - if ((ch->flags & CHN_F_SLEEPING) == 0 && - CHN_STOPPED(ch) && ch->inprog == 0) { + if (ch->intr_cv.cv_waiters == 0 && CHN_STOPPED(ch) && + ch->inprog == 0) { CHN_UNLOCK(ch); continue; }