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

[sashiko-bot@kernel.org]

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