Re: [PATCH v3 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() skips keying off active paired voice
--

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

ALSA: opl3: Clear voice channels before freeing their sets

This commit adds snd_opl3_clear_voices() and calls it from both the native
and OSS sequencer port teardown callbacks to key off active voices and clear
their channel pointers before freeing the MIDI channel sets.

> diff --git a/sound/drivers/opl3/opl3_midi.c b/sound/drivers/opl3/opl3_midi.c
> index 6d3c5b5a35ffc..ba807be2cf1de 100644
> --- a/sound/drivers/opl3/opl3_midi.c
> +++ b/sound/drivers/opl3/opl3_midi.c
> @@ -632,6 +632,20 @@ static void snd_opl3_kill_voice(struct snd_opl3 *opl3, int voice)
>  
>  }
>  
> +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++) {
> +		if (opl3->voices[i].state > 0)
> +			snd_opl3_kill_voice(opl3, i);
> +		opl3->voices[i].chan = NULL;
> +		opl3->voices[i].note_off_check = 0;
> +		opl3->voices[i].state = SNDRV_OPL3_ST_OFF;
> +	}
> +}

[Severity: Medium]
Can snd_opl3_clear_voices() skip keying off voice i + 3 in hardware when
voice i is in SNDRV_OPL3_ST_ON_4OP and voice i + 3 has been keyed on in
SNDRV_OPL3_MODE_SYNTH?

In sound/drivers/opl3/opl3_midi.c:snd_opl3_note_on(), when opl3->synth_mode
is SNDRV_OPL3_MODE_SYNTH, channels are mapped directly via snd_opl3_oss_map
without calling opl3_get_voice():

	if (opl3->synth_mode == SNDRV_OPL3_MODE_SEQ) {
		voice = opl3_get_voice(opl3, instr_4op, chan);
	} else {
		/* remap OSS voice */
		voice = snd_opl3_oss_map[chan->number];		
	}

If a 4-operator instrument is keyed on for channel 0..2 (or 3..5 on
oss_chset), primary voice i (0..2 or 9..11) is set to SNDRV_OPL3_ST_ON_4OP
and secondary voice i + 3 is set to SNDRV_OPL3_ST_NOT_AVAIL. If a
2-operator instrument is then keyed on for channel 12..14 (or 15..17 on
oss_chset), snd_opl3_oss_map maps directly to voice i + 3 (3..5 or 12..14),
writing OPL3_KEYON_BIT to OPL3_REG_KEYON_BLOCK for voice i + 3 and setting
opl3->voices[i + 3].state = SNDRV_OPL3_ST_ON_2OP while opl3->voices[i].state
remains SNDRV_OPL3_ST_ON_4OP.

When snd_opl3_clear_voices() iterates from 0 to MAX_OPL3_VOICES - 1, it
processes voice i first and calls snd_opl3_kill_voice(opl3, i):

	if (vp->state == SNDRV_OPL3_ST_ON_4OP) {
		vp2 = &opl3->voices[voice + 3];

		vp2->time = opl3->use_time++;
		vp2->state = SNDRV_OPL3_ST_OFF;
	}

This clears OPL3_KEYON_BIT only on voice i, while resetting
opl3->voices[i + 3].state to SNDRV_OPL3_ST_OFF. When the loop later reaches
i + 3, opl3->voices[i + 3].state > 0 is false, so snd_opl3_kill_voice() is
not called for i + 3, leaving OPL3_KEYON_BIT set in hardware while its state
is SNDRV_OPL3_ST_OFF.

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