Re: [PATCH 2/2] ALSA: ump: clear stale selectors in sequencer conversion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] ALSA: seq: SND_SEQ_UMP_KUNIT_TEST depends on KUNIT instead of
  KUNIT=y
- [Medium] ALSA: ump: stale RPN/NRPN MSB/LSB bytes kept on selector
  switch in cc_ev_to_ump_midi2()
- [Low] ALSA: seq: missing Fixes trailer for stale RPN/NRPN selector fix
--

commit 7687b62a51dd971356b5f3418762bb460cd2266c
Author: Alyandra Zahran Nadhir <alyandra5b@gmail.com>

> ALSA: ump: clear stale selectors in sequencer conversion
>
> The sequencer MIDI 2.0 converter tracks RPN and NRPN selection flags
> separately. When the controller stream switches selector type, the
> previous flag remains set; fill_rpn() checks RPN first and can encode
> subsequent NRPN data as RPN.
>
> Flush pending parameter data with the old selector, then clear the
> opposite selector before recording new state in both CC and Control14
> event paths. Add KUnit coverage for both transition directions,
> pending flushes, null RPN, channel isolation, and MIDI 1.0 conversion.
>
> Signed-off-by: Alyandra Zahran Nadhir <alyandra5b@gmail.com>

[Severity: Low]
Should this bug fix include a Fixes tag?

  Fixes: e9e02819a98a ("ALSA: seq: Automatic conversion of UMP events")

> diff --git a/sound/core/seq/Kconfig b/sound/core/seq/Kconfig
> index 0d606159103e7..8c08888902a92 100644
> --- a/sound/core/seq/Kconfig
> +++ b/sound/core/seq/Kconfig
> @@ -76,4 +76,16 @@ config SND_SEQ_UMP
>  config SND_SEQ_UMP_CLIENT
>  	def_tristate SND_UMP && SND_SEQ_UMP
>  
> +config SND_SEQ_UMP_KUNIT_TEST
> +	bool "KUnit tests for ALSA sequencer UMP conversion" if !KUNIT_ALL_TESTS
> +	depends on KUNIT
> +	depends on SND_SEQUENCER=y && SND_SEQ_UMP
> +	depends on SND_UMP=y && SND_UMP_LEGACY_RAWMIDI
> +	default KUNIT_ALL_TESTS

[Severity: Medium]
Does SND_SEQ_UMP_KUNIT_TEST need to depend on KUNIT=y instead of KUNIT?

SND_SEQ_UMP_KUNIT_TEST is a bool option compiled into built-in
seq_ump_convert.o (since SND_SEQUENCER=y and SND_UMP=y are required).
When CONFIG_KUNIT=m and CONFIG_SND_SEQ_UMP_KUNIT_TEST=y, linking vmlinux
fails with undefined references to kunit_kmalloc_array() and the KUnit
assertion helpers residing in kunit.ko.

[ ... ]

> diff --git a/sound/core/seq/seq_ump_convert.c b/sound/core/seq/seq_ump_convert.c
> index ff4ee26adad1f..5e40711303efb 100644
> --- a/sound/core/seq/seq_ump_convert.c
> +++ b/sound/core/seq/seq_ump_convert.c
[ ... ]
> @@ -847,6 +851,7 @@ static int cc_ev_to_ump_midi2(const struct snd_seq_event *event,
>  	switch (index) {
>  	case UMP_CC_RPN_MSB:
>  		ret = fill_rpn(cc, data, channel, true);
> +		cc->nrpn_set = 0;
>  		cc->rpn_set = 1;
>  		cc->cc_rpn_msb = val;
>  		if (cc->cc_rpn_msb == 0x7f && cc->cc_rpn_lsb == 0x7f)
>  			reset_rpn(cc);
>  		break;
>  	case UMP_CC_RPN_LSB:
>  		ret = fill_rpn(cc, data, channel, true);
> +		cc->nrpn_set = 0;
>  		cc->rpn_set = 1;
>  		cc->cc_rpn_lsb = val;
>  		if (cc->cc_rpn_msb == 0x7f && cc->cc_rpn_lsb == 0x7f)
>  			reset_rpn(cc);
>  		break;
>  	case UMP_CC_NRPN_MSB:
>  		ret = fill_rpn(cc, data, channel, true);
> +		cc->rpn_set = 0;
>  		cc->nrpn_set = 1;
>  		cc->cc_nrpn_msb = val;
>  		break;
>  	case UMP_CC_NRPN_LSB:
>  		ret = fill_rpn(cc, data, channel, true);
> +		cc->rpn_set = 0;
>  		cc->nrpn_set = 1;
>  		cc->cc_nrpn_lsb = val;
>  		break;

[Severity: Medium]
Can switching between RPN and NRPN in cc_ev_to_ump_midi2() still leave
stale cc_rpn_msb/cc_rpn_lsb and cc_nrpn_msb/cc_nrpn_lsb values behind?

When no data bytes are pending, fill_rpn() returns early without calling
reset_rpn(), and clearing only rpn_set or nrpn_set leaves the opposite
selector's MSB/LSB bytes intact (and reset_rpn() itself never clears
cc_nrpn_msb or cc_nrpn_lsb).

If a channel sets cc_rpn_lsb = 0x7f (such as RPN 0x00:0x7f) without
sending data, switches to NRPN, and later sends a two-message Null RPN
(UMP_CC_RPN_MSB = 0x7f followed by UMP_CC_RPN_LSB = 0x7f), the first
message sees cc_rpn_msb == 0x7f and the stale cc_rpn_lsb == 0x7f and calls
reset_rpn() prematurely.  The second message (UMP_CC_RPN_LSB = 0x7f) then
sets rpn_set = 1 and cc_rpn_lsb = 0x7f with cc_rpn_msb == 0, leaving RPN
0x00:0x7f active instead of deselecting parameters.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107111056.667-1-alyandra5b@gmail.com?part=2