At this point, it's possible for c->trigger to have changed, no? Don't you need to re-check it before proceeding?

I don't see any reason to unlock here, given that we're about to re-acquire the mutex a couple of lines below.

Same here.

I didn't bumped into any such issue so far, even with stress2 running for a while.

True.

stress2 isn't a comprehensive sound subsystem concurrency test, and the absence of "issues" doesn't prove that multi-threaded code is correct.

The problem I'm trying to point out is very similar to the problem you're trying to fix: if you drop a mutex and re-acquire it later, you must make sure that nothing changed while the mutex was dropped. So, to "look" correct, it needs to be something like

if (c->trigger != PCMTRIG_START) {
    CHN_UNLOCK(c);
    PCM_LOCK(d);
    CHN_LOCK(c);
    if (c->trigger == PCMTRIG_START) {
        /* what to do? */
    } else {
        c->trigger = PCMTRIG_START;
        CHN_INSERT_HEAD(d, c, channels.pcm.busy); 
        PCM_UNLOCK(d);
        chn_syncstate(c);
    }

Yeah, this is right. I thought that locking PCM first would be enough, but indeed, the problem I describe in the commit message (adding/removing twice) would occur here as well, if a higher-priority thread came with the same go value while another thread was blocking on PCM.

Isn't this always true since we check

if (go == c->trigger)
        return (0);

above?

If we have races between CHN unlock and lock, this may be inconsistent with the last call to CHANNEL_TRIGGER(). In case that matters, we should probably set the value of c->trigger right after CHANNEL_TRIGGER(), check the value again here and issue a CHN_INSERT_HEAD_SAFE if it hasn't changed in the meantime. Or am I missing something?

It most likely is. I did some tests to make sure we're not missing some edge-case where this statement could be false, but I couldn't stumble upon such a case so far, so it should be fine to remove those checks.

Even though I didn't manage to encounter such a race yet (not saying it can never occur though), I think the right way to go is to modify vchan_trigger() to work the same way as chn_trigger(), that is, once we lock the parent channel, check that nothing changed, and if it didn't, then set the trigger and modify the list. Otherwise simply return.

This seems to work fine (diff'd against the original code):

diff --git a/sys/dev/sound/pcm/vchan.c b/sys/dev/sound/pcm/vchan.c
index ed13a3c55961..36527cf781f9 100644
--- a/sys/dev/sound/pcm/vchan.c
+++ b/sys/dev/sound/pcm/vchan.c
@@ -143,32 +143,40 @@ vchan_trigger(kobj_t obj, void *data, int go)
 {
 	struct vchan_info *info;
 	struct pcm_channel *c, *p;
-	int ret, otrigger;
+	int ret;
 
 	info = data;
-
-	if (!PCMTRIG_COMMON(go) || go == info->trigger)
-		return (0);
-
 	c = info->channel;
 	p = c->parentchannel;
-	otrigger = info->trigger;
-	info->trigger = go;
 
 	CHN_LOCKASSERT(c);
+	if (!PCMTRIG_COMMON(go) || go == info->trigger)
+		return (0);
 
 	CHN_UNLOCK(c);
 	CHN_LOCK(p);
+	/*
+	 * Do nothing if another thread triggered the channel already while we
+	 * had dropped the mutex.
+	 */
+	if (info->trigger == go) {
+		CHN_UNLOCK(p);
+		return (0);
+	}
+	if (snd_verbose > 3) {
+		device_printf(c->dev, "%s() %s: calling go=0x%08x , "
+		    "prev=0x%08x\n", __func__, c->name, go, c->trigger);
+	}
+	info->trigger = go;
 
 	switch (go) {
 	case PCMTRIG_START:
-		if (otrigger != PCMTRIG_START)
-			CHN_INSERT_HEAD(p, c, children.busy);
+		CHN_INSERT_HEAD(p, c, children.busy);
 		break;
 	case PCMTRIG_STOP:
 	case PCMTRIG_ABORT:
-		if (otrigger == PCMTRIG_START)
-			CHN_REMOVE(p, c, children.busy);
+		CHN_REMOVE(p, c, children.busy);
 		break;
 	default:
 		break;
@@ -181,9 +189,7 @@ vchan_trigger(kobj_t obj, void *data, int go)
 	if (ret == 0 && go == PCMTRIG_START && VCHAN_SYNC_REQUIRED(c))
 		ret = vchan_sync(c);
 
-	CHN_UNLOCK(c);
 	CHN_UNLOCK(p);
-	CHN_LOCK(c);
 
 	return (ret);
 }

I managed to crash with the patch above, so I'll work on it a bit more.

Moreover, this looks like it is meant to prevent multiple redundant calls to CHANNEL_TRIGGER(). This is not guaranteed to work currently, since we can have races between this code and the part where we actually set the c->trigger value.

We're holding a lock on the channel here, I suppose there shouldn't be any change to c->trigger? Please note that the similar check in the stop / abort case is not redundant, unlike here.

I don't see how the vchan_trigger() thing is connected, could you elaborate?

My concern would be a race of something like:

1. c->trigger has value PCMTRIG_STOP
2. Thread A enters the first part of chn_trigger() and calls CHANNEL_TRIGGER(PCMTRIG_START), unlocks the channel
3. Thread B enters chn_trigger() and calls CHANNEL_TRIGGER(PCMTRIG_ABORT), exits
4. Thread A executes the second part, locks the channel, inserts the channel into the list

Then we are inconsistent with our last call to CHANNEL_TRIGGER(PCMTRIG_ABORT). Currently we check consistency between c->trigger and the channel list, but not with CHANNEL_TRIGGER() calls.