[PATCH 0/2] ALSA: hdsp, hdspm: Fix unlocked control_register updates in MIDI paths

[PATCH 0/2] ALSA: hdsp, hdspm: Fix unlocked control_register updates in MIDI paths

[Yifei Gao]

hdsp and hdspm keep a software copy of the card's control register and
update it read-modify-write under the card lock, except in some MIDI
input paths, where concurrent updates can be lost.

In 2011, commit c0da00145f9a ("ALSA: hdspm - Fix locking in
snd_hdspm_midi_input_read") fixed the MIDI work side in hdspm. The
interrupt handler side was left unlocked, and hdsp has both problems.
Patch 1 fixes hdsp. Patch 2 fixes the remaining interrupt handler side
in hdspm.

Both issues were found by static analysis. The patches are
compile-tested only (W=1). I have no access to this hardware, so tests
on a real card would be very welcome.

Yifei Gao (2):
  ALSA: hdsp: Serialize control_register updates in MIDI input paths
  ALSA: hdspm: Take hdspm->lock when masking MIDI interrupts in IRQ
    handler

 sound/pci/rme9652/hdsp.c  | 16 ++++++++++++----
 sound/pci/rme9652/hdspm.c |  8 +++++---
 2 files changed, 17 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/2] ALSA: hdsp: Serialize control_register updates in MIDI input paths

[Yifei Gao]

hdsp->control_register is a software copy of a hardware register. It is
updated read-modify-write under hdsp->lock, except in two MIDI paths:
snd_hdsp_midi_input_read() holds only the per-port hmidi->lock, and
snd_hdsp_interrupt() holds no lock at all. Lock holders disable
interrupts only on the local CPU, so updates can be lost across CPUs.
A lost MIDI interrupt enable bit stops MIDI input on that port. Other
lost updates revert control or PCM settings.

Take hdsp->lock in both places. In snd_hdsp_midi_input_read(), drop
hmidi->lock before taking it, as commit c0da00145f9a ("ALSA: hdspm -
Fix locking in snd_hdspm_midi_input_read") did for hdspm.

Found by static analysis. Compile-tested only; no hardware available.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Yifei Gao <gyf161023@gmail.com>
---
 sound/pci/rme9652/hdsp.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c
index 7dce548b9f36..4e9f3a95fe0d 100644
--- a/sound/pci/rme9652/hdsp.c
+++ b/sound/pci/rme9652/hdsp.c
@@ -1347,6 +1347,10 @@ static int snd_hdsp_midi_input_read (struct hdsp_midi *hmidi)
 			}
 		}
 		hmidi->pending = 0;
+	}
+
+	/* control_register is shared by all users, serialize on hdsp->lock */
+	scoped_guard(spinlock_irqsave, &hmidi->hdsp->lock) {
 		if (hmidi->id)
 			hmidi->hdsp->control_register |= HDSP_Midi1InterruptEnable;
 		else
@@ -3869,8 +3873,10 @@ static irqreturn_t snd_hdsp_interrupt(int irq, void *dev_id)
 	if (midi0 && midi0status) {
 		if (hdsp->use_midi_work) {
 			/* we disable interrupts for this input until processing is done */
-			hdsp->control_register &= ~HDSP_Midi0InterruptEnable;
-			hdsp_write(hdsp, HDSP_controlRegister, hdsp->control_register);
+			scoped_guard(spinlock, &hdsp->lock) {
+				hdsp->control_register &= ~HDSP_Midi0InterruptEnable;
+				hdsp_write(hdsp, HDSP_controlRegister, hdsp->control_register);
+			}
 			hdsp->midi[0].pending = 1;
 			schedule = 1;
 		} else {
@@ -3880,8 +3886,10 @@ static irqreturn_t snd_hdsp_interrupt(int irq, void *dev_id)
 	if (hdsp->io_type != Multiface && hdsp->io_type != RPM && hdsp->io_type != H9632 && midi1 && midi1status) {
 		if (hdsp->use_midi_work) {
 			/* we disable interrupts for this input until processing is done */
-			hdsp->control_register &= ~HDSP_Midi1InterruptEnable;
-			hdsp_write(hdsp, HDSP_controlRegister, hdsp->control_register);
+			scoped_guard(spinlock, &hdsp->lock) {
+				hdsp->control_register &= ~HDSP_Midi1InterruptEnable;
+				hdsp_write(hdsp, HDSP_controlRegister, hdsp->control_register);
+			}
 			hdsp->midi[1].pending = 1;
 			schedule = 1;
 		} else {
-- 
2.34.1

[PATCH 2/2] ALSA: hdspm: Take hdspm->lock when masking MIDI interrupts in IRQ handler

[Yifei Gao]

snd_hdspm_interrupt() clears a MIDI interrupt enable bit in
hdspm->control_register without hdspm->lock. Every other update holds
that lock. This includes snd_hdspm_midi_input_read(), which sets the bit
again, since commit c0da00145f9a ("ALSA: hdspm - Fix locking in
snd_hdspm_midi_input_read"). Updates racing with the handler on another
CPU can be lost. This can stall MIDI input or revert a control setting.

Take hdspm->lock in the handler. All other users of the lock already
run with interrupts disabled.

Found by static analysis. Compile-tested only; no hardware available.

Fixes: 763f356cd8de ("[ALSA] Add HDSP MADI driver")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Yifei Gao <gyf161023@gmail.com>
---
 sound/pci/rme9652/hdspm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index cc4cf43e5a5b..135761278556 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -5383,9 +5383,11 @@ static irqreturn_t snd_hdspm_interrupt(int irq, void *dev_id)
 				/* we disable interrupts for this input until
 				 * processing is done
 				 */
-				hdspm->control_register &= ~hdspm->midi[i].ie;
-				hdspm_write(hdspm, HDSPM_controlRegister,
-						hdspm->control_register);
+				scoped_guard(spinlock, &hdspm->lock) {
+					hdspm->control_register &= ~hdspm->midi[i].ie;
+					hdspm_write(hdspm, HDSPM_controlRegister,
+						    hdspm->control_register);
+				}
 				hdspm->midi[i].pending = 1;
 				schedule = 1;
 			}
-- 
2.34.1