sound: Unlock around uiomove() in midi_{read,write}()

[christos]

Summary

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

[emaste]

sys/dev/sound/midi/midi.c

474

are there any struct snd_midimembers that could be invalid after dropping and reacquiring the lock? E.g. could m->flags & M_RX no longer be true?

[christos]

sys/dev/sound/midi/midi.c

474

The only place where M_RX gets cleared is in midi_close(), so I'm not sure we should worry about this, since by that time we'll have closed and midi_read() will just need to flush. Or am I missing something?

[markj]

sys/dev/sound/midi/midi.c

551

used is calculated above: used = MIN(MIDIQ_AVAIL(m->outq), uio->uio_resid). What if the amount of available space changed while the lock was dropped?

[christos]

sys/dev/sound/midi/midi.c

551

I'm not really sure how to address this. I suppose if the available size since then has changed to something less than used, I suppose we will discard a few bytes. Shouldn't that be the normal case though? We've got a statically-sized buffer,so if some other thread fills it first, then we won't have available space until we drain it.

[dev_nicolas-provost.fr]

Still not sure this is good. It is working as-it for me; uiomove will not sleep if the lock is taken just above!

[christos]

In D54130#1239093, @dev_nicolas-provost.fr wrote:

Still not sure this is good. It is working as-it for me; uiomove will not sleep if the lock is taken just above!

We have to unlock since we are using a non-sleepable lock.

[dev_nicolas-provost.fr]

I'm not convinced we have to unlock here, but this may give a breathe to the interrupt, so why not (during my last test I found too much latency but I don't know yet if it is relative to my userspace code or not).

[christos]

In D54130#1239116, @dev_nicolas-provost.fr wrote:

I'm not convinced we have to unlock here, but this may give a breathe to the interrupt, so why not (during my last test I found too much latency but I don't know yet if it is relative to my userspace code or not).

That's how uiomove() works. If you hold a non-sleepable lock when it runs, you'll get an error from WITNESS.

[dev_nicolas-provost.fr]

I see. You may only have to check for M_RX inside the loop and not at line 433 (file closed), in addition to the extra lock/unlock.

[christos]

In D54130#1239510, @dev_nicolas-provost.fr wrote:

I see. You may only have to check for M_RX inside the loop and not at line 433 (file closed), in addition to the extra lock/unlock.

Why? Even if M_RX gets unset during the loop, we should still drain the read().

[dev_nicolas-provost.fr]

If M_RX gets unset, the device file was closed, so no read will occur anymore and draining or keeping data is useless. I made a successful test with this extra lock/unlock.

[christos]

In D54130#1240411, @dev_nicolas-provost.fr wrote:

If M_RX gets unset, the device file was closed, so no read will occur anymore and draining or keeping data is useless. I made a successful test with this extra lock/unlock.

I agree it will essentially be "useless", but I do not get the value of introducing a check for every single loop, since read() will fail anyway if the file is closed mid-read.

[christos]

sys/dev/sound/midi/midi.c

551

@markj Bump ^

[dev_nicolas-provost.fr]

Two different processes cannot have an opened descriptor to the same MIDI device. So, I think the only problematic case is when a process opens the device and a child thread is waiting in blocking mode; if the descriptor is closed by the parent thread, will this properly stop the blocking read?

[christos]

In D54130#1241240, @dev_nicolas-provost.fr wrote:

Two different processes cannot have an opened descriptor to the same MIDI device. So, I think the only problematic case is when a process opens the device and a child thread is waiting in blocking mode; if the descriptor is closed by the parent thread, will this properly stop the blocking read?

@markj @kib Haven't had a chance to test this scenario yet, do you have any idea regarding this?

[dev_nicolas-provost.fr]

I just realized that the outer loop "while (uio->uio_resid > 0)" is NOT needed at all in midi_read, which solves the problem. We must return as soon as some data is read.

Same thing, probably, in midi_write.