[alsa-devel] [PATCH] RME HDSPM MADI lock fix

[Takashi Iwai]

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