At Sun, 05 Jun 2011 11:57:57 +0200, Jörn Nettingsmeier wrote:
hi *!
while we were trying to debug the midi-related xruns in the current driver, i came across what looks like an unrelated locking bug to me:
static int snd_hdspm_midi_input_read (struct hdspm_midi *hmidi) ... spin_lock_irqsave (&hmidi->lock, flags); <--------------- n_pending = snd_hdspm_midi_input_available (hmidi->hdspm, hmidi->id); if (n_pending > 0) { if (hmidi->input) { if (n_pending > (int)sizeof (buf)) n_pending = sizeof (buf); for (i = 0; i < n_pending; ++i) buf[i] = snd_hdspm_midi_read_byte (hmidi->hdspm,
hmidi->id); if (n_pending) snd_rawmidi_receive (hmidi->input, buf, n_pending); } else { /* flush the MIDI input FIFO */ while (n_pending--) snd_hdspm_midi_read_byte (hmidi->hdspm, hmidi->id); } } hmidi->pending = 0;
hmidi->hdspm->control_register |= hmidi->ie;<------- !!! hdspm_write(hmidi->hdspm, HDSPM_controlRegister, hmidi->hdspm->control_register); spin_unlock_irqrestore (&hmidi->lock, flags);<--------- return snd_hdspm_midi_output_write (hmidi);}
if i understand this whole business correctly, we should be holding the hdspm lock here, not the hmidi one.
please review the attached patch and apply if you think it's ok.
Well, it's a wrong lock for snd_hdspm_midi_input_available(). So you'll need to lock twice, either once unlock and lock another or nested. Re-locking would be less messy in this case, I suppose.
thanks,
Takashi
best,
jörn
-- Jörn Nettingsmeier Lortzingstr. 11, 45128 Essen, Tel. +49 177 7937487
Meister für Veranstaltungstechnik (Bühne/Studio) Tonmeister (VDT)
http://stackingdwarves.net [2 hdspm-lockfix.diff <text/x-patch (7bit)>] --- sound/pci/rme9652/hdspm.c~ 2011-06-01 21:30:24.000000000 +0200 +++ sound/pci/rme9652/hdspm.c 2011-06-01 22:04:08.184946711 +0200 @@ -1619,7 +1619,7 @@ int n_pending; int i;
- spin_lock_irqsave (&hmidi->lock, flags);
- spin_lock_irqsave (&hmidi->hdspm->lock, flags); n_pending = snd_hdspm_midi_input_available (hmidi->hdspm, hmidi->id); if (n_pending > 0) { if (hmidi->input) {
@@ -1644,7 +1644,7 @@ hdspm_write(hmidi->hdspm, HDSPM_controlRegister, hmidi->hdspm->control_register);
- spin_unlock_irqrestore (&hmidi->lock, flags);
- spin_unlock_irqrestore (&hmidi->hdspm->lock, flags); return snd_hdspm_midi_output_write (hmidi);
}