* [PATCH] RME HDSPM MADI lock fix
@ 2011-06-05 9:57 Jörn Nettingsmeier
2011-06-06 9:05 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Jörn Nettingsmeier @ 2011-06-05 9:57 UTC (permalink / raw)
To: alsa-devel, Takashi Iwai, Adrian Knoth
[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]
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
--
Jörn Nettingsmeier
Lortzingstr. 11, 45128 Essen, Tel. +49 177 7937487
Meister für Veranstaltungstechnik (Bühne/Studio)
Tonmeister (VDT)
http://stackingdwarves.net
[-- Attachment #2: hdspm-lockfix.diff --]
[-- Type: text/x-patch, Size: 663 bytes --]
--- 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);
}
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] RME HDSPM MADI lock fix
2011-06-05 9:57 [PATCH] RME HDSPM MADI lock fix Jörn Nettingsmeier
@ 2011-06-06 9:05 ` Takashi Iwai
2011-06-07 8:33 ` Jörn Nettingsmeier
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2011-06-06 9:05 UTC (permalink / raw)
To: Jörn Nettingsmeier; +Cc: Adrian Knoth, alsa-devel
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);
> }
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] RME HDSPM MADI lock fix
2011-06-06 9:05 ` Takashi Iwai
@ 2011-06-07 8:33 ` Jörn Nettingsmeier
2011-06-07 8:38 ` Adrian Knoth
2011-06-07 9:28 ` Takashi Iwai
0 siblings, 2 replies; 8+ messages in thread
From: Jörn Nettingsmeier @ 2011-06-07 8:33 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Adrian Knoth, alsa-devel
[-- Attachment #1: Type: text/plain, Size: 1674 bytes --]
On 06/06/2011 11:05 AM, Takashi Iwai wrote:
> 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);
>> ...
>> 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.
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.
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
[-- Attachment #2: hdspm-lock-fix.diff --]
[-- Type: text/x-patch, Size: 612 bytes --]
--- linux-2.6.39/sound/pci/rme9652/hdspm.c.orig 2011-05-19 06:06:34.000000000 +0200
+++ linux-2.6.39/sound/pci/rme9652/hdspm.c 2011-06-07 10:26:35.778933230 +0200
@@ -1639,12 +1639,14 @@
}
}
hmidi->pending = 0;
+ spin_unlock_irqrestore (&hmidi->lock, flags);
+ spin_lock_irqsave (&hmidi->hdspm->lock, flags);
hmidi->hdspm->control_register |= hmidi->ie;
hdspm_write(hmidi->hdspm, HDSPM_controlRegister,
hmidi->hdspm->control_register);
+ spin_unlock_irqrestore (&hmidi->hdspm->lock, flags);
- spin_unlock_irqrestore (&hmidi->lock, flags);
return snd_hdspm_midi_output_write (hmidi);
}
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] RME HDSPM MADI lock fix
2011-06-07 8:33 ` Jörn Nettingsmeier
@ 2011-06-07 8:38 ` Adrian Knoth
2011-06-07 9:28 ` Takashi Iwai
1 sibling, 0 replies; 8+ messages in thread
From: Adrian Knoth @ 2011-06-07 8:38 UTC (permalink / raw)
To: Jörn Nettingsmeier; +Cc: Takashi Iwai, alsa-devel
On Tue, Jun 07, 2011 at 10:33:55AM +0200, Jörn Nettingsmeier wrote:
> --- linux-2.6.39/sound/pci/rme9652/hdspm.c.orig 2011-05-19 06:06:34.000000000 +0200
> +++ linux-2.6.39/sound/pci/rme9652/hdspm.c 2011-06-07 10:26:35.778933230 +0200
> @@ -1639,12 +1639,14 @@
> }
> }
> hmidi->pending = 0;
> + spin_unlock_irqrestore (&hmidi->lock, flags);
>
> + spin_lock_irqsave (&hmidi->hdspm->lock, flags);
> hmidi->hdspm->control_register |= hmidi->ie;
> hdspm_write(hmidi->hdspm, HDSPM_controlRegister,
> hmidi->hdspm->control_register);
> + spin_unlock_irqrestore (&hmidi->hdspm->lock, flags);
>
> - spin_unlock_irqrestore (&hmidi->lock, flags);
> return snd_hdspm_midi_output_write (hmidi);
> }
>
This looks sane to me. I'll add it to my git repo and will re-post it
together with the upcoming changes for the iosono guys once we're
finished.
Cheers
--
mail: adi@thur.de http://adi.thur.de PGP/GPG: key via keyserver
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] RME HDSPM MADI lock fix
2011-06-07 8:33 ` Jörn Nettingsmeier
2011-06-07 8:38 ` Adrian Knoth
@ 2011-06-07 9:28 ` Takashi Iwai
2011-06-07 9:30 ` Adrian Knoth
1 sibling, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2011-06-07 9:28 UTC (permalink / raw)
To: Jörn Nettingsmeier; +Cc: Adrian Knoth, alsa-devel
At Tue, 07 Jun 2011 10:33:55 +0200,
Jörn Nettingsmeier wrote:
>
> On 06/06/2011 11:05 AM, Takashi Iwai wrote:
> > 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);
> >> ...
> >> 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.
>
> 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.
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] RME HDSPM MADI lock fix
2011-06-07 9:28 ` Takashi Iwai
@ 2011-06-07 9:30 ` Adrian Knoth
2011-06-07 9:37 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Adrian Knoth @ 2011-06-07 9:30 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Jörn Nettingsmeier
On 06/07/2011 11:28 AM, Takashi Iwai wrote:
>>> 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.
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.
Cheers
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] RME HDSPM MADI lock fix
2011-06-07 9:30 ` Adrian Knoth
@ 2011-06-07 9:37 ` Takashi Iwai
2011-06-07 9:41 ` Adrian Knoth
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2011-06-07 9:37 UTC (permalink / raw)
To: Adrian Knoth; +Cc: alsa-devel, Jörn Nettingsmeier
At Tue, 07 Jun 2011 11:30:28 +0200,
Adrian Knoth wrote:
>
> On 06/07/2011 11:28 AM, Takashi Iwai wrote:
>
> >>> 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.
>
> 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.)
Takashi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] RME HDSPM MADI lock fix
2011-06-07 9:37 ` Takashi Iwai
@ 2011-06-07 9:41 ` Adrian Knoth
0 siblings, 0 replies; 8+ messages in thread
From: Adrian Knoth @ 2011-06-07 9:41 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Jörn Nettingsmeier
On 06/07/2011 11:37 AM, Takashi Iwai wrote:
>>> 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.)
That's exactly how I did it: Sign-off by Jörn and a second line by me.
Since it's his work and we're not talking about cryptographic
signatures, I took the liberty to provide the correct attribution.
Cheers
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-06-07 9:41 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-05 9:57 [PATCH] RME HDSPM MADI lock fix Jörn Nettingsmeier
2011-06-06 9:05 ` Takashi Iwai
2011-06-07 8:33 ` Jörn Nettingsmeier
2011-06-07 8:38 ` Adrian Knoth
2011-06-07 9:28 ` Takashi Iwai
2011-06-07 9:30 ` Adrian Knoth
2011-06-07 9:37 ` Takashi Iwai
2011-06-07 9:41 ` Adrian Knoth
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.