RME HDSPM MADI lock fix

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.

best,

jörn

Signature · 8 lines

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)>]

pci/rme9652/hdspm.c renamed from pci/rme9652/hdspm.c~ +1 -1

@@ -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) {

At Sun, 05 Jun 2011 11:57:57 +0200,

Quoted 23 lines · Jörn Nettingsmeier

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);
...
          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);
}

Quoted 4 lines

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.

sound/pci/rme9652/hdspm.c renamed from sound/pci/rme9652/hdspm.c.orig +1 -0

@@ -1639,12 +1639,14 @@
 		}
 	}
 	hmidi->pending = 0;
+	spin_unlock_irqrestore (&hmidi->lock, flags);
 

Quoted 30 lines · Takashi Iwai

At Sun, 05 Jun 2011 11:57:57 +0200,

Quoted 27 lines · Jörn Nettingsmeier

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);
...
          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.

Quoted 3 lines

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.

ok, here's a revised patch. has been running for 20 or so hours without problems, although the load was light most of the time.

please consider for inclusion.

Quoted 7 lines

Quoted 3 lines

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.

ok, here's a revised patch. has been running for 20 or so hours without problems, although the load was light most of the time.

please consider for inclusion.

Looks good. But please give a proper changelog and your sign-off for inclusion.

Quoted 6 lines · Takashi Iwai

Quoted 6 lines

Quoted 3 lines

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.

ok, here's a revised patch. has been running for 20 or so hours without problems, although the load was light most of the time.

Quoted 4 lines

Quoted 1 line

please consider for inclusion.

Looks good. But please give a proper changelog and your sign-off for inclusion.

I've handled this. I take care of everything, commit (with changelog and sign-off) to my git repo and will send the final patch series to the list once it's ready.

Quoted 6 lines

Quoted 2 lines

Looks good. But please give a proper changelog and your sign-off for inclusion.

I've handled this. I take care of everything, commit (with changelog and sign-off) to my git repo and will send the final patch series to the list once it's ready.

OK, thanks.

(Note that usually a patch always requires the sign-off of the
original author, no matter who commits. But in this case, it doesn't matter much because the patch is trivial.)