Re: [PATCH 1/2] ALSA: ump: clear stale RPN selector on NRPN change

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] ALSA: ump: missing Fixes trailer in commit message
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] ALSA: ump: improper RPN/NRPN state reset in reset_rpn() [1]
--

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

> ALSA: ump: clear stale RPN selector on NRPN change
>
> The legacy-to-UMP converter keeps separate RPN and NRPN selector
> flags. When a stream changes selector type without sending data first,
> both flags remain set and fill_rpn() gives RPN priority, so subsequent
> NRPN data is emitted as RPN.
>
> Clear the previous selector flag after flushing any pending parameter
> data and before recording the newly selected parameter type.
>
> Signed-off-by: Alyandra Zahran Nadhir <alyandra5b@gmail.com>

[Severity: Low]
This fixes a bug in the legacy MIDI to UMP RPN/NRPN converter.  Should it
include:

  Fixes: 0b5288f5fe63 ("ALSA: ump: Add legacy raw MIDI support")

> diff --git a/sound/core/ump_convert.c b/sound/core/ump_convert.c
> index ecbd894f4c83a..fe7d3ee3c6b1f 100644
> --- a/sound/core/ump_convert.c
> +++ b/sound/core/ump_convert.c
> @@ -378,6 +378,7 @@ static int cvt_legacy_cmd_to_ump(struct ump_cvt_to_ump *cvt,
>  		switch (buf[1]) {
>  		case UMP_CC_RPN_MSB:
>  			ret = fill_rpn(cc, midi2, true);
> +			cc->nrpn_set = 0;
>  			cc->rpn_set = 1;
>  			cc->cc_rpn_msb = buf[2];
>  			if (cc->cc_rpn_msb == 0x7f && cc->cc_rpn_lsb == 0x7f)

[Severity: Medium]
This problem wasn't introduced by this patch, but does fill_rpn() calling
reset_rpn() [1] drop consecutive Data Entry updates for the same
parameter?

In MIDI 1.0, an RPN or NRPN selection stays active across multiple Data
Entry messages until a new parameter is selected or RPN Null (0x7f/0x7f)
is sent.  However, fill_rpn() calls reset_rpn(cc) after emitting the first
UMP packet, clearing cc->rpn_set, cc->nrpn_set, and cc->cc_rpn_msb/lsb
along with the data flags.  Any subsequent Data Entry message for that
same parameter then sees neither rpn_set nor nrpn_set set in fill_rpn()
and is silently dropped.

[1] https://sashiko.dev/#/bug/linux-552af5af-5e38-4c5d-9b46-76886460e72d

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