[PATCH v2 0/3] ALSA: opl3: Fix sequencer timer and channel lifetimes

[PATCH v2 0/3] ALSA: opl3: Fix sequencer timer and channel lifetimes

[Cen Zhang]

OPL3 can receive a fixed-duration note before its timer is initialized,
and the card-owned voice array can retain channel pointers after either
sequencer port has freed its channel set. The delayed-note timer can also
remain queued after the sequencer client has been deleted.

Initialize the timer before publishing ports, clear voice aliases before
each channel set is freed, and shut down the shared timer after client
deletion has drained both event producers.

The first two patches address existing initialization and rebind issues
identified by Sashiko. The third replaces the earlier native-port-only
timer shutdown with shutdown after all sequencer ports have been drained.

---
Cen Zhang (3):
  ALSA: opl3: Initialize the timer before publishing a sequencer port
  ALSA: opl3: Clear voice channels before freeing their sets
  ALSA: opl3: Shut down the timer after draining sequencer ports

 sound/drivers/opl3/opl3_oss.c   |  1 +
 sound/drivers/opl3/opl3_seq.c   | 25 ++++++++++++++++++++-----
 sound/drivers/opl3/opl3_voice.h |  1 +
 3 files changed, 22 insertions(+), 5 deletions(-)

Changes in v2:
- Split the initialization-order and stale-channel issues into separate
  patches preceding the timer lifetime fix.
- Clear voice channel pointers, state and delayed-note flags under
  voice_lock in both native and OSS port cleanup callbacks.
- Shut down the shared timer after client deletion has drained every port,
  replacing the native-port-only shutdown.

Link to v1: https://lore.kernel.org/r/pm-pmb-4552c4e605507d8d9b16-v4-213af356c0046331c8ff@gmail.com

[PATCH v2 1/3] ALSA: opl3: Initialize the timer before publishing a sequencer port

[Cen Zhang]

snd_opl3_seq_probe() initializes the delayed-note timer and its lock
after snd_opl3_synth_create_port() has made the native port reachable.
A direct NOTEON does not require a subscription, and a fixed-duration
instrument makes it enter snd_opl3_start_timer().

On sequencer-driver rebind, the card-owned instrument bank remains
available. An event delivered as soon as the port appears can race the
late initialization:

  Sequencer driver probe             Direct NOTEON
  ----------------------             -------------
  snd_opl3_synth_create_port()
    snd_seq_event_port_attach()
      publish port
                                     snd_opl3_synth_event_input()
                                       snd_opl3_note_on()
                                         snd_opl3_start_timer()
                                           use tlist and sys_timer_lock
  timer_setup(tlist)
  spin_lock_init(sys_timer_lock)
  sys_timer_status = 0

Initialize the timer, its lock and its status before creating the client
and attaching either port. No event can then reach partially initialized
timer state or have its newly armed timer reinitialized by the probe.

Assisted-by: LLM
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 sound/drivers/opl3/opl3_seq.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/drivers/opl3/opl3_seq.c b/sound/drivers/opl3/opl3_seq.c
index b77d8e8f12fb..2bacefc3201d 100644
--- a/sound/drivers/opl3/opl3_seq.c
+++ b/sound/drivers/opl3/opl3_seq.c
@@ -214,6 +214,11 @@ static int snd_opl3_seq_probe(struct snd_seq_device *dev)
 
 	spin_lock_init(&opl3->voice_lock);
 
+	/* Initialize the timer before an attached port can receive events. */
+	timer_setup(&opl3->tlist, snd_opl3_timer_func, 0);
+	spin_lock_init(&opl3->sys_timer_lock);
+	opl3->sys_timer_status = 0;
+
 	opl3->seq_client = -1;
 
 	/* allocate new client */
@@ -232,11 +237,6 @@ static int snd_opl3_seq_probe(struct snd_seq_device *dev)
 		return err;
 	}
 
-	/* setup system timer */
-	timer_setup(&opl3->tlist, snd_opl3_timer_func, 0);
-	spin_lock_init(&opl3->sys_timer_lock);
-	opl3->sys_timer_status = 0;
-
 #if IS_ENABLED(CONFIG_SND_SEQUENCER_OSS)
 	snd_opl3_init_seq_oss(opl3, name);
 #endif
-- 
2.43.0

[PATCH v2 2/3] ALSA: opl3: Clear voice channels before freeing their sets

[Cen Zhang]

The card-owned OPL3 voice array survives sequencer-driver unbind, but
its chan pointers refer to the native or OSS port's channel set. Neither
port's private_free callback clears those pointers before freeing its set.

A direct NOTEON in the initial SYNTH mode can leave a voice allocated
without a subscription. Even with fix_dur zero and no timer, unbind and
rebind followed by direct PITCHBEND can read the old channel:

  Sequencer driver unbind/rebind      Direct PITCHBEND
  -----------------------------      ----------------
  snd_seq_delete_kernel_client()
    port_free()
      free channel set
  snd_opl3_seq_probe()
    allocate new channel set
                                     snd_opl3_pitch_ctrl()
                                       snd_opl3_update_pitch()
                                         old voice->chan is non-NULL
                                         snd_opl3_calc_pitch()
                                           read freed channel

Clearing only note_off_check or the voice state does not protect this
path: snd_opl3_update_pitch() tests the channel pointer itself.

Clear channel pointers, delayed-note flags and voice state under
voice_lock before each port frees its set. This also excludes a timer
or pitch update already using a voice. Both private_free callbacks must
do the cleanup because an in-flight OSS event can refill the shared
voice array after the native port has been drained and freed. That event
can only reference its own still-live set; the OSS callback's final
cleanup retires that reference before freeing it as well.

Assisted-by: LLM
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 sound/drivers/opl3/opl3_oss.c   |  1 +
 sound/drivers/opl3/opl3_seq.c   | 13 +++++++++++++
 sound/drivers/opl3/opl3_voice.h |  1 +
 3 files changed, 15 insertions(+)

diff --git a/sound/drivers/opl3/opl3_oss.c b/sound/drivers/opl3/opl3_oss.c
index 6d39b2b77b80..1f01d27955fe 100644
--- a/sound/drivers/opl3/opl3_oss.c
+++ b/sound/drivers/opl3/opl3_oss.c
@@ -41,6 +41,7 @@ static void snd_opl3_oss_free_port(void *private_data)
 {
 	struct snd_opl3 *opl3 = private_data;
 
+	snd_opl3_clear_voices(opl3);
 	snd_midi_channel_free_set(opl3->oss_chset);
 }
 
diff --git a/sound/drivers/opl3/opl3_seq.c b/sound/drivers/opl3/opl3_seq.c
index 2bacefc3201d..f711139c5067 100644
--- a/sound/drivers/opl3/opl3_seq.c
+++ b/sound/drivers/opl3/opl3_seq.c
@@ -148,10 +148,23 @@ static int snd_opl3_synth_event_input(struct snd_seq_event * ev, int direct,
 
 /* ------------------------------ */
 
+void snd_opl3_clear_voices(struct snd_opl3 *opl3)
+{
+	int i;
+
+	guard(spinlock_irqsave)(&opl3->voice_lock);
+	for (i = 0; i < MAX_OPL3_VOICES; i++) {
+		opl3->voices[i].chan = NULL;
+		opl3->voices[i].note_off_check = 0;
+		opl3->voices[i].state = SNDRV_OPL3_ST_OFF;
+	}
+}
+
 static void snd_opl3_synth_free_port(void *private_data)
 {
 	struct snd_opl3 *opl3 = private_data;
 
+	snd_opl3_clear_voices(opl3);
 	snd_midi_channel_free_set(opl3->chset);
 }
 
diff --git a/sound/drivers/opl3/opl3_voice.h b/sound/drivers/opl3/opl3_voice.h
index be9ccca2d952..aa81512d23c9 100644
--- a/sound/drivers/opl3/opl3_voice.h
+++ b/sound/drivers/opl3/opl3_voice.h
@@ -13,6 +13,7 @@ int snd_opl3_synth_use_inc(struct snd_opl3 * opl3);
 void snd_opl3_synth_use_dec(struct snd_opl3 * opl3);
 int snd_opl3_synth_setup(struct snd_opl3 * opl3);
 void snd_opl3_synth_cleanup(struct snd_opl3 * opl3);
+void snd_opl3_clear_voices(struct snd_opl3 *opl3);
 
 /* Prototypes for opl3_midi.c */
 void snd_opl3_note_on(void *p, int note, int vel, struct snd_midi_channel *chan);
-- 
2.43.0

Re: [PATCH v2 2/3] ALSA: opl3: Clear voice channels before freeing their sets

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: opl3: snd_opl3_clear_voices() clears voice state without killing hardware voice
--

commit a0dc4d938c6f259e2d8cec7c1fb6339768606511
Author: Cen Zhang <zzzccc427@gmail.com>

ALSA: opl3: Clear voice channels before freeing their sets

This commit introduces snd_opl3_clear_voices() to clear voice channel
pointers, delayed note-off flags, and voice state under voice_lock before
each sequencer port frees its channel set.

[ ... ]

> diff --git a/sound/drivers/opl3/opl3_seq.c b/sound/drivers/opl3/opl3_seq.c
> index 2bacefc3201de..f711139c50670 100644
> --- a/sound/drivers/opl3/opl3_seq.c
> +++ b/sound/drivers/opl3/opl3_seq.c
> @@ -148,10 +148,23 @@ static int snd_opl3_synth_event_input(struct snd_seq_event * ev, int direct,
>  
>  /* ------------------------------ */
>  
> +void snd_opl3_clear_voices(struct snd_opl3 *opl3)
> +{
> +	int i;
> +
> +	guard(spinlock_irqsave)(&opl3->voice_lock);
> +	for (i = 0; i < MAX_OPL3_VOICES; i++) {
> +		opl3->voices[i].chan = NULL;
> +		opl3->voices[i].note_off_check = 0;
> +		opl3->voices[i].state = SNDRV_OPL3_ST_OFF;
> +	}
> +}

[Severity: Medium]
When a voice is active via a direct note-on without a port subscription
(where snd_opl3_synth_unuse() -> snd_opl3_synth_cleanup() ->
snd_opl3_reset() does not run on unbind), does clearing note_off_check and
setting state to SNDRV_OPL3_ST_OFF here without clearing OPL3_KEYON_BIT in
hardware leave the FM voice stuck sounding and desynchronized from its
software state?

In sound/drivers/opl3/opl3_midi.c:snd_opl3_note_on(), starting a note sets
OPL3_KEYON_BIT in OPL3_REG_KEYON_BLOCK and optionally schedules a delayed
note-off via note_off_check:

	opl3_reg = reg_side | (OPL3_REG_KEYON_BLOCK + voice_offset);
	opl3->command(opl3, opl3_reg, blocknum);

	/* kill note after fixed duration (in centiseconds) */
	if (fm->fix_dur) {
		opl3->voices[voice].note_off = jiffies +
			(fm->fix_dur * HZ) / 100;
		snd_opl3_start_timer(opl3);
		opl3->voices[voice].note_off_check = 1;
	} else
		opl3->voices[voice].note_off_check = 0;

And when re-using a voice, sound/drivers/opl3/opl3_midi.c:snd_opl3_note_on()
only clears OPL3_KEYON_BIT in hardware if vp->state > 0:

	/* kill voice on channel */
	vp = &opl3->voices[voice];
	if (vp->state > 0) {
		opl3_reg = reg_side | (OPL3_REG_KEYON_BLOCK + voice_offset);
		reg_val = vp->keyon_reg & ~OPL3_KEYON_BIT;
		opl3->command(opl3, opl3_reg, reg_val);
	}

If snd_opl3_clear_voices() resets state to SNDRV_OPL3_ST_OFF and clears
note_off_check in software without writing vp->keyon_reg to hardware, any
active voice (including a fixed-duration note whose pending timer note-off
is cancelled) stays keyed on in hardware. After rebind, a subsequent
snd_opl3_note_on() on that voice will see vp->state == SNDRV_OPL3_ST_OFF
and skip clearing OPL3_KEYON_BIT before writing blocknum | OPL3_KEYON_BIT,
missing the 0-to-1 key-on transition needed to re-trigger the envelope.

Should snd_opl3_clear_voices() also silence active voices in hardware (such
as via snd_opl3_kill_voice() when state > 0)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/pm-pmb-4552c4e605507d8d9b16-v6-f3fa18aca2c7d18be670-0@gmail.com?part=2

[PATCH v2 3/3] ALSA: opl3: Shut down the timer after draining sequencer ports

[Cen Zhang]

The delayed-note timer is independent of the sequencer ports' use-locks
and runs code in snd-opl3-synth. Deleting the client drains the event
callbacks but does not cancel this timer. A direct fixed-duration NOTEON
without a subscription can arm it without any unuse callback to stop it.

A shutdown in only the native port's private_free callback is too early
to finish shared voice cleanup when the OSS port is enabled:

  Sequencer client deletion          In-flight OSS NOTEON
  -------------------------          -------------------
  drain native port
    clear native voice aliases
                                     snd_opl3_oss_event_input()
                                       refill shared voice array
  drain OSS port
    clear voice aliases again
    free OSS channel set
  client deletion returns

The per-port voice cleanup prevents the timer from dereferencing a freed
channel during that sequence, but a queued or running timer can still
outlive client deletion and the sequencer driver itself.

Shut the timer down synchronously after snd_seq_delete_kernel_client()
has drained both ports. No event can then start it again, and shutdown
waits for a callback already running and prevents callback self-rearming.
Do this outside voice_lock and sys_timer_lock, which the callback takes.
The probe initializes timer state before publishing ports on rebind.

Assisted-by: LLM
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 sound/drivers/opl3/opl3_seq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/drivers/opl3/opl3_seq.c b/sound/drivers/opl3/opl3_seq.c
index f711139c5067..6b343b73c66c 100644
--- a/sound/drivers/opl3/opl3_seq.c
+++ b/sound/drivers/opl3/opl3_seq.c
@@ -271,6 +271,8 @@ static void snd_opl3_seq_remove(struct snd_seq_device *dev)
 		snd_seq_delete_kernel_client(opl3->seq_client);
 		opl3->seq_client = -1;
 	}
+	/* Stop the shared timer after all event callbacks have been drained. */
+	timer_shutdown_sync(&opl3->tlist);
 }
 
 static struct snd_seq_driver opl3_seq_driver = {
-- 
2.43.0