Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 69128 Build 66011: arc lint + arc unit
Event Timeline
| 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? | |
| 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? | |
| 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? | |
| 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. | |
Comment Actions
Still not sure this is good. It is working as-it for me; uiomove will not sleep if the lock is taken just above!
Comment Actions
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).
Comment Actions
That's how uiomove() works. If you hold a non-sleepable lock when it runs, you'll get an error from WITNESS.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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?
Comment Actions
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.
Comment Actions
What is the issue with the outer loop? It checks how many bytes remain to be sent to/read from userland.
Comment Actions
I don't see how this is true? The driver prevents having multiple open file descriptions for the device, but a process can still open the device and pass the fd to a second process.
Regarding M_TX and M_RX, I think it's not necessary to change anything. midi_close() will not be called until all references to the device's file description are gone. That can't happen while a thread is using that descriptor to concurrently read or write; it holds a ref on the fd, which prevents midi_close() from being called.
| sys/dev/sound/midi/midi.c | ||
|---|---|---|
| 551 | The MIDIQ_ENQ macro has a warning comment: "No protection against overflow, underflow". So is it actually safe to write more bytes than there is space available? That is, will doing so crash the system? If not, what happens? Will we just drop user data? Looking at the code, I think we are just dropping data. I presume that is ok in this context. | |